feat(memory): default-off retrieval-holdout hook (epsilon-dropout + counterfactual logging)

[drewstone]

Adds an opt-in randomized retrieval-holdout hook between adapter.search() and renderMemoryContext(): with per-session probability epsilon, suppress exactly one watchlist item from every retrieval of that session (sticky within the session, no backfill), and emit one counterfactual-slot event per retrieval call — including no-drop calls — carrying the full eligibility set and the exact pick/drop propensities. Randomization is per session (outcomes are per-session), deterministic and keyed on sessionId (`${sessionId}#holdout` for the epsilon coin, `${sessionId}#pick` for the target draw over the id-sorted watchlist∩eligibility candidates), so every assignment replays from the log alone. Suppression happens before rendering, so a drop session's context is byte-identical to a natural smaller retrieval. The library does no I/O; persistence is the consumer's job via the onEvent callback. Short-term-conversation and raw-string context paths bypass the hook untouched, and consumers that call adapter.search() directly can apply the exported pure functions (applyRetrievalHoldout / applySessionStickyRetrievalHoldout) themselves.

Default-off / no-op guarantee: the hook is gated on the new optional holdout field of AgentMemorySearchOptions, absent by default. Unconfigured behavior is byte-identical to before this change, covered by an explicit test; with epsilon: 0 the hook only logs and never drops. No existing call sites change behavior.

Tests: tests/memory-holdout.test.ts (8 tests: byte-identical default-off, event schema, session stickiness, propensity correctness, and a 10^4-session drop-rate calibration check); existing tests/memory-adapter.test.ts still passes.

Design, estimator, logging schema, and sample-size analysis are frozen in the pre-registration for experiment EXP-007 (O3 retrieval holdout) in the research repo, projects/probabilistic-agent-optimization/experiments/EXP-007-o3-retrieval-holdout/PREREG.md, which registers this implementation at this commit.

[tangletools]

✅ Auto-approved drewstone PR — 8f7e4f54

This PR was opened by the trusted drewstone account.
The full PR reviewer audit still runs separately and will publish findings if it detects issues.

_{tangletools · auto-approval · reason: drewstone_author · 2026-07-04T21:39:01Z}

[tangletools]

🟠 Value Audit — better-approach-exists

Verdict	better-approach-exists
Concerns	2 (1 medium-concern, 1 weak-concern)
Heuristic	0.0s
Duplication	0.1s
Interrogation	226.5s (2 bridge agents)
Total	226.6s

💰 Value — better-approach-exists

A default-off, well-tested epsilon-dropout hook for memory retrieval — the suppression half is correctly placed, but it reinvents the propensity/RNG/pre-registration vocabulary that the agent-eval substrate already exports for exactly this experiment.

• What it does: Adds an opt-in hook between AgentMemoryAdapter.search() and renderMemoryContext(): with per-session probability epsilon it suppresses exactly one watchlist item from every retrieval in that session (sticky target, drawn uniformly over watchlist∩E, deterministic via sha256-keyed RNG), and emits one counterfactual log event per call — including no-drop/control calls — carrying the full eligibility s
• Goals it achieves: Enable per-item causal treatment-effect estimation for memory retrieval — measure how much an individual memory item contributes to agent outcomes by randomly dropping it from some sessions and comparing results via inverse-propensity weighting (the PR body and inline comments name this explicitly: treatment-effect logging, IPW input, control-arm membership).
• Assessment: The domain half is sound and in-grain: suppression operates on AgentMemoryHit[] and belongs in agent-knowledge per the repo's own layering rule (the concept only makes sense for retrieved memory fragments). Default-off with a byte-identical-no-op guarantee, identity-preserving unconfigured path, session stickiness, and a 10^4-session drop-rate calibration test are all well-engineered. The instrume
• Better / existing approach: Searched src/ for holdout/counterfactual/propensity (only this PR's files matched) and inspected the installed substrate at node_modules/@tangle-network/agent-eval@0.100.0 (dist/counterfactual-.d.ts, dist/off-policy-.d.ts, dist/pre-registration-.d.ts, dist/statistics-.d.ts). The substrate already exports: (1) off-policy IPS/SNIPS/DR estimators consuming OffPolicyTrajectory{behaviorProb,targetP
• Model: opencode/zai-coding-plan/glm-5.2
• Bridge attempts: 2
• Bridge warning: opencode/kimi-for-coding/k2p7: bridge stream ended without value-audit content

🎯 Usefulness — sound-with-nits

Well-designed default-off retrieval-holdout hook slotted at the correct seam (search→render), exported on the public surface with a pure-function escape hatch and an imminent caller (EXP-007); ships with one ergonomic gap worth noting.

• Integration: Reachable and correctly wired. The hook lives inside defaultGetMemoryContext (src/memory/adapter.ts:19-25), which is the SDK's default render pipeline called by createNeo4jAgentMemoryAdapter.getContext (src/memory/neo4j.ts:79, 115, 116). The new holdout? field is carried on AgentMemorySearchOptions (src/memory/types.ts:53), which is the public options type flowing through every `AgentMemor
• Fit with existing patterns: Fits the grain of the codebase cleanly. (1) Optional-knob-on-options is the established pattern: kinds, minScore, metadata, limit all work this way on AgentMemorySearchOptions (src/memory/types.ts:43-54); holdout? is the same shape. (2) The onEvent(event) callback defers all persistence/I/O to the consumer — identical to how KbStore, source adapters, and `autodata/data-creation-loo
• Real-world viability: Holds up on happy paths and edge cases. Pure functions, no I/O, deterministic RNG keyed on sessionId (replayable from the log alone — verified by tests/memory-holdout.test.ts:264). Edge cases all handled explicitly: empty/absent watchlist (never drops), missing sessionId (emits holdoutEligible: false, never drops — tested at tests/memory-holdout.test.ts:197-208), target absent from later eligibi
• Model: opencode/zai-coding-plan/glm-5.2
• Bridge attempts: 1

🎯 Usefulness Audit

🟡 Neo4j short-term / raw-string paths silently bypass the hook [integration] ``

createNeo4jAgentMemoryAdapter.getContext (src/memory/neo4j.ts:43-117) has two early-return branches that build an AgentMemoryContext directly WITHOUT calling defaultGetMemoryContext: the shortTerm conversation-context path (neo4j.ts:44-71) and the raw-string getContext result path (neo4j.ts:96-112). A consumer who configures options.holdout and hits either path gets ZERO events and ZERO drops — no holdoutEligible: false marker, no warning. The PR body acknowledges the carve-out, but

💰 Value Audit

🟠 Statistical vocabulary forked from the agent-eval substrate this repo is built to consume [better-architecture] ``

The holdout defines its own propensity-bearing event (RetrievalHoldoutEvent.pickPropensity/dropPropensity), its own deterministic RNG (deterministicRng, sha256-derived), and points at an external PREREG.md — but agent-eval@0.100.0 already exports OffPolicyTrajectory{behaviorProb,targetProb} + inverseProbabilityWeighting/selfNormalizedImportanceWeighting/doublyRobust (dist/off-policy-DiwuKKg7.d.ts), HypothesisManifest + signManifest/evaluateHypothesis (dist/pre-registration-CMm8cvrh.d.ts), mulber

---

What this audit checks

It judges the change on its merits — not whether it was tasked out in an issue. Unticketed, fast-moving work is fine; the question is whether the change is good and whether a better or existing approach should be used instead.

Pass	What it asks
Heuristic	Vague title? Whitespace-only or cruft-bearing diff? (content signals only)
Duplication	Do added function/class names already exist elsewhere in the repo?
Value Audit	What does it do? What goal does it achieve? Is it good? Better architecture or already-exists?
Usefulness Audit	Does it integrate and fit? Will it hold up in real use and actually get used?

Findings are concerns, not blocks — the human reviewer decides what to do with them.

_{value-audit · 20260704T214913Z}

[tangletools]

❌ Needs Work — 8f7e4f54

Review health 100/100 · Reviewer score 45/100 · Confidence 70/100 · 7 findings (1 high, 1 medium, 5 low)

	glm	deepseek	aggregate
Readiness	45	95	45
Confidence	70	70	70
Correctness	45	95	45
Security	45	95	45
Testing	45	95	45
Architecture	45	95	45

Reviewer score is advisory once the run is complete and the verdict has no blockers.

Full multi-shot audit completed 2/2 planned shots over 5 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 2/2 planned shots over 5 changed files. Global verifier still owns final merge decision.

Blocking

🔴 HIGH Session stickiness silently breaks when options.holdout is a fresh object per call -> wrong treatment-effect data — src/memory/holdout.ts

sessionRegistry is WeakMap<RetrievalHoldoutConfig, Map<string, ...>> keyed by config OBJECT IDENTITY, and defaultGetMemoryContext (src/memory/adapter.ts:17) reads options.holdout per call with no stabilization. A caller building options inline per retrieval (the natural adapter pattern) passes a fresh config each call -> WeakMap miss -> applyRetrievalHoldout is called with ctx.session undefined. VERIFIED by direct execution (tsx): (1) callIndex resets to [1,1] instead of [1,2], losing within-session call ordering; (2) the 'sticky' drop target FLIPS: session s-flip, watchlist [m1,m3,m7], rng picks index floor(0.6*|E∩W|); call1 E=[m1,m7] drops m7, call2 E=[m1,m3,m7] drops m3 -> one session logged as under-treatment for two different items, invalidating the per-item estimate the feature exi

Other

🟠 MEDIUM Inconsistent privacy posture: query is hashed but scope (tenantId/userId/tags) and sessionId are logged in plaintext — src/memory/holdout.ts

The event carefully hashes the query (queryHash: sha256(ctx.query).slice(0,16)) but spreads ctx.scope verbatim (...(ctx.scope !== undefined ? { scope: ctx.scope } : {})) and logs sessionId in plaintext at the top level (...(sessionId !== undefined ? { sessionId } : {})) WHILE ALSO emitting rngKey = sha256(sessionId).slice(0,16). AgentMemoryScope carries tenantId, userId, and arbitrary tags. Since onEvent hands the event to a consumer-controlled persistence sink (often an analytics warehouse), plaintext tenantId/userId is a PII leak vector that contradicts the care taken on the query, and the duplicate sessionId (plaintext + hashed) is redundant. Fix: redact/hash PII-bearing scope fields or formally document the consumer's obligation to strip before persistence; keep one sessionId rep

🟡 LOW No validation of epsilon bounds; out-of-range values silently bias the experiment — src/memory/holdout.ts

epsilon is typed number with no [0,1] check; assignment uses rng(...) < config.epsilon. VERIFIED: epsilon=1.5 is accepted and forces sessionHoldout=true on every session (dropPropensity math then exceeds 1); a negative epsilon silently disables holdout while still emitting events that look like a real control arm. For a feature whose value depends on exact propensity weights, a malformed epsilon silently corrupts the estimator. Fix: validate epsilon ∈ [0,1] (and watchlist is string[]) at first use, fail loud. Aligns with the repo's fail-loud doctrine.

🟡 LOW Audit-input defect: prescribed two-dot base is divergent; empty diff for all 4 assigned files — src/memory/index.ts

PROCES/shot issue, not a code defect. dba9ea9 is NOT an ancestor of head 8f7e4f5 (merge-base is 035a8d4) and already contains byte-identical blobs of holdout.ts/adapter.ts/types.ts/index.ts, so the prescribed git diff dba9ea9..8f7e4f5 -- src/memory/* returns EMPTY for every assigned file and would produce a false 'nothing changed' verdict. The PR's real diff (matching the stated +524 -3 across 5 files: adapter.ts +14/-3, holdout.ts +237, index.ts +1, types.ts +6, tests/memory-holdout.test.ts +266) is the three-dot diff from merge-base 035a8d4, which equals head's parent. I reviewed against that true base. Flagging so the global verifier routes correctly and does not treat the empty two-dot diff as evidence of no change.

🟡 LOW Empty watchlist (watchlist: []) branch untested — tests/memory-holdout.test.ts

src/memory/holdout.ts:116 does const watchlist = config.watchlist ?? []. Every test in the file sets a non-empty watchlist (e.g. line 54 watchlist: ['m1', 'm7'], line 167 watchlist: ['m1']). The explicit-empty-list case (watchlist: []) and the omitted-watchlist case (watchlist: undefined) are never asserted. With an empty watchlist, watchlistEligible is always [], sessionTargetId stays null, no item is ever dropped, but an event is still emitted with the full eligibility set — this control-arm-with-no-treatment path is

🟡 LOW MAX_TRACKED_SESSIONS eviction (10k cap) is untested — tests/memory-holdout.test.ts

src/memory/holdout.ts:202 declares const MAX_TRACKED_SESSIONS = 10_000 and applySessionStickyRetrievalHoldout deletes the oldest session when the cap is reached (holdout.ts:230-233). The source comment (holdout.ts:199-201) claims eviction 'degrades detectably, not silently' because a re-drawn target shows up as a mixed-exposure session. No test in tests/memory-holdout.test.ts exercises the eviction path — there is no test that pushes >10k sessions, no test that asserts a re-drawn target produces a detectable mixed-exposure event, and no test that the LRU oldest-first selection is correct. If eviction silently corrupted session state or picked the wrong session to evict, the suite would not catch it. Fix: add a test that uses a config with a stubbed-low cap (or >10k iterations) and assert

🟡 LOW Score field never tested without normalizedScore — tests/memory-holdout.test.ts

The hit() helper (line 22-24) only sets normalizedScore. src/memory/holdout.ts:177-179 builds the eligible-item score via hit.normalizedScore ?? hit.score. The score-only fallback branch (hit with score set but no normalizedScore) is never exercised. If that branch had a typo or undefined-coercion bug, no test would catch it. Fix: add one hit variant using { score } only and assert event.eligible[i].score equals that value. Trivial gap; the branch is one line.

---

_{tangletools · 2026-07-04T21:58:22Z · trace}

[tangletools]

❌ 1 Blocking Finding — 8f7e4f54

Full multi-shot audit completed 2/2 planned shots over 5 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 2/2 planned shots over 5 changed files. Global verifier still owns final merge decision.

Full immutable report for this review: trace

Summary comment for this run: full summary

---

_{tangletools · 2026-07-04T21:58:22Z · immutable trace}

[tangletools]

✅ Auto-approved drewstone PR — 46bf6a28

This PR was opened by the trusted drewstone account.
The full PR reviewer audit still runs separately and will publish findings if it detects issues.

_{tangletools · auto-approval · reason: drewstone_author · 2026-07-04T22:12:22Z}

[tangletools]

🟡 Value Audit — sound-with-nits

Verdict	sound-with-nits
Concerns	1 (1 weak-concern)
Heuristic	0.0s
Duplication	0.0s
Interrogation	212.1s (2 bridge agents)
Total	212.1s

💰 Value — sound-with-nits

Closes a real logging gap (Neo4j bypass paths silently skipped the holdout hook) and correctly routes analysis through agent-eval's substrate OPE estimators instead of re-deriving them; one trivial 3-line context-extraction duplication.

• What it does: Two things, both opt-in and default-off-preserved. (1) Adds emitRetrievalHoldoutBypass() — wired into the two Neo4j getContext early-return paths that answer WITHOUT going through adapter.search() (short-term conversation context, raw-string getContext results) — so those calls still produce one holdoutEligible:false event with a bypassReason tag, via the same onEvent sink, with no suppression app
• Goals it achieves: (a) Keep the experiment denominator honest: a holdout-configured consumer must see EVERY retrieval-shaped call, including ones where the adapter answered from conversation context rather than from a memory search — otherwise the IPS/SNIPS denominator is silently wrong. (b) Stop re-deriving off-policy estimators in this repo: per AGENTS.md, agent-knowledge depends on agent-eval and must consume the
• Assessment: Both changes are genuine improvements and land in the grain of the codebase. The bypass emission closes a hole a consumer could not detect from the log alone (the short-term and raw-string paths returned before defaultGetMemoryContext, so the holdout in adapter.ts never ran). Routing analysis through OffPolicyTrajectory is exactly the layering AGENTS.md mandates (substrate estimators consumed, not
• Better / existing approach: Looked for an existing event-sink / logging-hook abstraction in src/memory (searched getContext, defaultGetMemoryContext, emitRetrievalHoldoutBypass, applyRetrievalHoldout across src/) — there is none; onEvent on RetrievalHoldoutConfig is the single sink and the bypass event correctly reuses it. Considered routing the bypass paths through defaultGetMemoryContext instead, but that would change sema
• Model: opencode/zai-coding-plan/glm-5.2
• Bridge attempts: 2
• Bridge warning: opencode/kimi-for-coding/k2p7: bridge stream ended without value-audit content

🎯 Usefulness — sound

Default-off retrieval-holdout hook correctly wired into the search→render seam with Neo4j bypass coverage, reusing agent-eval's statistics and OPE estimators rather than forking them; no dead surface, no competing pattern.

• Assessment: A coherent, well-scoped experimental instrumentation hook that will be consumed by EXP-007 (named in code comments holdout.ts:10-11 and the PR body). It does the right thing in the right place, reuses rather than reinvents the statistical machinery, and fails safe (default-off, byte-identical, epsilon-0 logs-only). Ship.
• Integration: Reachable through AgentMemorySearchOptions.holdout (types.ts:53) on every adapter path. The generic search→render path uses applySessionStickyRetrievalHoldout (adapter.ts:19-25); Neo4j's two non-search context paths call emitRetrievalHoldoutBypass (neo4j.ts:63-70, 115-122). Pure functions (applyRetrievalHoldout, retrievalHoldoutBehaviorProb, toOffPolicyTrajectory) are also exported for
• Fit with existing patterns: Slots into the existing search→render seam in defaultGetMemoryContext — the natural and only place a retrieval holdout can apply. Follows the repo's 'library does no I/O; consumer persists via callback' convention (mirrors the source-adapter pattern). Reuses substrate primitives (mulberry32, OffPolicyTrajectory, IPS/SNIPS estimators) instead of re-deriving them. No existing equivalent or com
• Real-world viability: Default-off with an explicit byte-identical-no-op test (memory-holdout.test.ts:36-47) means unconfigured behavior cannot drift. Stickiness has two layers (config-keyed WeakMap AND deterministic per-sessionId RNG draws), so the degenerate fresh-config-per-call case still produces correct sticky assignment. Bypass logging covers both Neo4j context paths; a future adapter that bypasses `defaultGetMem
• Model: opencode/zai-coding-plan/glm-5.2
• Bridge attempts: 1

💰 Value Audit

🟡 holdoutBypassContext duplicates the context-extraction in defaultGetMemoryContext [duplication] ``

neo4j.ts:307-316 (holdoutBypassContext) and adapter.ts:20-24 (the inline ctx object passed to applySessionStickyRetrievalHoldout) extract the identical 3-field call context: { query, scope?, taskId? }. The neo4j.ts helper even has a comment 'Mirrors defaultGetMemoryContext's holdout call context'. A shared holdoutCallContext(query, options) in holdout.ts, imported by both, would remove the mirror and guarantee they cannot drift when a new field is added to RetrievalHoldoutCallContext. Trivial —

---

What this audit checks

It judges the change on its merits — not whether it was tasked out in an issue. Unticketed, fast-moving work is fine; the question is whether the change is good and whether a better or existing approach should be used instead.

Pass	What it asks
Heuristic	Vague title? Whitespace-only or cruft-bearing diff? (content signals only)
Duplication	Do added function/class names already exist elsewhere in the repo?
Value Audit	What does it do? What goal does it achieve? Is it good? Better architecture or already-exists?
Usefulness Audit	Does it integrate and fit? Will it hold up in real use and actually get used?

Findings are concerns, not blocks — the human reviewer decides what to do with them.

_{value-audit · 20260704T221811Z}

[tangletools]

✅ Auto-approved drewstone PR — e289c54a

This PR was opened by the trusted drewstone account.
The full PR reviewer audit still runs separately and will publish findings if it detects issues.

_{tangletools · auto-approval · reason: drewstone_author · 2026-07-04T22:21:30Z}

[tangletools]

🟡 Value Audit — sound-with-nits

Verdict	sound-with-nits
Concerns	2 (2 weak-concern)
Heuristic	0.0s
Duplication	0.2s
Interrogation	455.4s (2 bridge agents)
Total	455.6s

💰 Value — sound-with-nits

Hardens the existing default-off retrieval-holdout hook (privacy defaults, fail-loud config, value-keyed sticky sessions fixing a silent-corruption bug) and extends it with bypass-event logging on Neo4j non-retrieval paths plus an off-policy trajectory mapping onto agent-eval's IPS/SNIPS/DR estimato

• What it does: Three additions to src/memory/holdout.ts and one to src/memory/neo4j.ts. (1) emitRetrievalHoldoutBypass + two Neo4j call sites: adapter context paths that answer without retrieval (short-term conversation context, raw-string getContext) now emit a holdout event with bypassReason set, so the experiment denominator stops silently losing those calls. (2) retrievalHoldoutBehaviorProb + toOffPolicyTraj
• Goals it achieves: Make a shipped default-off counterfactual-slot instrumentation actually trustworthy enough to base EXP-007 causal estimates on: (a) the experiment denominator must be honest (bypass paths logged), (b) propensities must be replayable and analysis must not re-derive them (off-policy mapping onto substrate estimators), (c) malformed configs must fail loud rather than silently corrupt propensities, (d
• Assessment: Coherent and well-placed. The hook sits exactly between adapter.search() and renderMemoryContext() (src/memory/adapter.ts:19-25), so a drop session's context is byte-identical to a natural smaller retrieval — the design rule that makes the counterfactual valid. Default-off is verified by an array-identity assertion (tests/memory-holdout.test.ts:43, expect(context.hits).toBe(hits)). The off-policy
• Better / existing approach: none — this is the right approach. Searched src/ for any existing counterfactual/retrieval-suppression mechanism (grep 'holdout|counterfactual|treatment-effect|epsilon'); the only hit outside src/memory is src/release.ts:35-39 which is the held-out-eval-split concept from agent-eval's release gate, a different domain. Verified the substrate primitives (mulberry32, OffPolicyTrajectory, inverseProba
• Model: opencode/zai-coding-plan/glm-5.2
• Bridge attempts: 2
• Bridge warning: opencode/kimi-for-coding/k2p7: bridge stream ended without value-audit content

🎯 Usefulness — sound-with-nits

A coherent, default-off retrieval-holdout hook wired into the two natural memory seams, reusing agent-eval's RNG and off-policy substrate rather than reinventing them; the only nit is that the sticky-session convenience wrapper is process-local while being the default integration path.

• Integration: Reachable and correctly wired. The hook fires between search and render in defaultGetMemoryContext (src/memory/adapter.ts:19-25), and the two Neo4j non-retrieval paths emit bypass events so the experiment denominator stays honest (src/memory/neo4j.ts:63-70 short-term-context, :115-122 raw-string-context). The opt-in surface is the optional AgentMemorySearchOptions.holdout field (src/memory/types.t
• Fit with existing patterns: Fits the codebase grain. Extends the existing AgentMemorySearchOptions extension point rather than inventing a new seam. Reuses the substrate's mulberry32 for the RNG core (holdout.ts:143) instead of forking a PRNG, respecting the agent-knowledge → agent-eval dependency direction in AGENTS.md. toOffPolicyTrajectory consumes agent-eval's OffPolicyTrajectory shape (runId/reward/behaviorProb/targetPr
• Real-world viability: Holds up well. Default-off is proven byte-identical including hit-array identity (tests/memory-holdout.test.ts:39-47). Config validation fails loud on epsilon outside [0,1], NaN, and non-string watchlists (holdout.ts:175-186, tested :461-497). A 10^4-session calibration test confirms the default RNG drops at rate matching epsilon within ~3.75 sd (:692-714). Deterministic per-sessionId assignment (
• Model: opencode/zai-coding-plan/glm-5.2
• Bridge attempts: 1

💰 Value Audit

🟡 Sticky-session registry is process-global state keyed by experiment knobs, not consumer identity [maintenance] ``

src/memory/holdout.ts:341 — const sessionRegistry = new Map<string, Map<string, RetrievalHoldoutSessionState>>() is module-level. The outer key is configHash (sha256(epsilon + sorted watchlist)); the inner key is plaintext sessionId. Two independent consumers in the same process running the SAME experiment knobs (e.g. both pick epsilon=0.2, watchlist=['m1']) with overlapping simple sessionIds ('session-1', 'session-2'…) would share inner state: consumer B's 'session-1' would inherit consumer A

🎯 Usefulness Audit

🟡 Sticky-session wrapper is process-local yet is the default integration path [robustness] ``

defaultGetMemoryContext (src/memory/adapter.ts:20) calls applySessionStickyRetrievalHoldout, whose session registry is a module-level Map keyed by configHash+sessionId (src/memory/holdout.ts:341). In any multi-instance deployment (the production norm for agent runtimes), a session whose retrievals land on different processes loses callCount threading and, when the eligibility set changes between calls, can re-draw a different drop target — silently degrading the per-item estimate. The design ant

---

What this audit checks

It judges the change on its merits — not whether it was tasked out in an issue. Unticketed, fast-moving work is fine; the question is whether the change is good and whether a better or existing approach should be used instead.

Pass	What it asks
Heuristic	Vague title? Whitespace-only or cruft-bearing diff? (content signals only)
Duplication	Do added function/class names already exist elsewhere in the repo?
Value Audit	What does it do? What goal does it achieve? Is it good? Better architecture or already-exists?
Usefulness Audit	Does it integrate and fit? Will it hold up in real use and actually get used?

Findings are concerns, not blocks — the human reviewer decides what to do with them.

_{value-audit · 20260704T223109Z}

[tangletools]

❌ Needs Work — e289c54a

Review health 100/100 · Reviewer score 48/100 · Confidence 70/100 · 8 findings (1 high, 1 medium, 6 low)

	glm	deepseek	aggregate
Readiness	48	89	48
Confidence	70	70	70
Correctness	48	89	48
Security	48	89	48
Testing	48	89	48
Architecture	48	89	48

Reviewer score is advisory once the run is complete and the verdict has no blockers.

Full multi-shot audit completed 2/2 planned shots over 3 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 2/2 planned shots over 3 changed files. Global verifier still owns final merge decision.

Blocking

🔴 HIGH retrievalHoldoutBehaviorProb returns invalid behavior probability for no-drop events when |W∩E| > 1 — src/memory/holdout.ts

The no-drop branch returns 1 - epsilon * pick where pick = 1/|watchlistEligible| for control-arm events. This computes the probability that a SPECIFIC item survives suppression (P(T≠Ti) marginalized over arms = 1 - epsilon/|W∩E|), NOT the probability of the observed full-delivery action (P(control arm) = 1 - epsilon). The two differ whenever |W∩E| > 1. Proof: at epsilon=0.5, |W∩E|=2, the code's action distribution sums to 0.75 + 2×0.25 = 1.25 ≠ 1.0, which is impossible for a valid probability distribution. Impact: IPS estimates are biased — a 100-session simulation (50 control + 50 holdout, epsilon=0.5, |W∩E|=2, reward=1 for no-drop) gives IPS=0.667 with the code's formula vs IPS=1.000 with the correct formula (33% downward bias). SNIPS is unaffected because weight normalization cancel

Other

🟠 MEDIUM retrievalHoldoutBehaviorProb returns wrong value for subsequent no-drop calls in holdout-arm sessions — src/memory/holdout.ts

For a holdout-arm session at call t>1 where the sticky target T is absent from E_current (droppedId=null, pickPropensity=sticky value), the function returns 1 - epsilon * sticky_pick. But the true behavior probability of delivering the full set at this call is 1: the session is definitively in the holdout arm, T∉E_current, so nothing can be dropped. The function conflates 'probability that no item was drawn for suppression at the first call' with 'probability that the full set was delivered at THIS call.' The test at line 592 ('Keep event in the holdout arm') asserts behaviorProb=0.5 for this case when the correct value is 1. This compounds the IPS bias for multi-

🟡 LOW Bypass onEvent throw blocks all context retrieval — src/memory/holdout.ts

config.onEvent(event) is called synchronously before the return (line 327). If onEvent throws, the adapter's getContext() call fails entirely — the user gets no context back, not just no bypass log. This is the same pattern as applyRetrievalHoldout (line 251) so behavior is consistent, but it means a consumer's buggy event sink drops ALL conversation context, not just observability. Consider wrapping onEvent in a try/catch that logs and continues.

🟡 LOW Module-level sessionRegistry (strong Map) never cleans up abandoned configHash entries — src/memory/holdout.ts

The outer Map<string, Map<...>> is keyed by configHash and never evicted. The inner maps are capped at maxTrackedSessions (default 10,000), but the outer map grows with the number of distinct experiment configs. The comment says 'in practice a handful,' which is reasonable for production but could leak in test suites or dynamic-config scenarios. Not a blocker since each entry is tiny.

🟡 LOW canonicalJson maps NaN to null, causing potential scopeHash collisions — src/memory/holdout.ts

JSON.stringify(NaN) returns 'null', so {a: NaN} and {a: null} produce identical scopeHash values. This is unlikely to matter for AgentMemoryScope objects (which contain string fields), but the function is generic and could be reused. Not a blocker for the current scope.

🟡 LOW canonicalJson type boundary wider than safe domain — src/memory/holdout.ts

Function accepts unknown and uses JSON.stringify on leaf values. JSON.stringify throws on BigInt and silently converts Date to an ISO string, RegExp to {}. Current callers are safe — AgentMemoryScope.tags is Record<string, string> (types.ts:17) and config hashing uses only number + string[]. If this utility is ever called with a scope containing BigInt tags (e.g. a consumer using as any or a future type change), it will crash the holdout path. Recommend narrowing the parameter type or adding a guard: if (typeof value === 'bigint') throw new TypeError(...).

🟡 LOW IPS/SNIPS end-to-end test only covers the symmetric case where behaviorProb is identical for drops and keeps — tests/memory-holdout.test.ts

The test (lines 661-689) constructs 10 holdout + 10 control sessions at epsilon=0.5 with a single-item watchlist, yielding behaviorProb=0.5 for both drops and keeps. This makes IPS and SNIPS numerically identical (value=1) and maxImportanceWeight=2. It does not exercise the regime where IPS and SNIPS diverge (high-variance importance weights), nor does it test the qHat/doublyRobust path through the estimator. The estimator integration is verified for the simplest case only. Not blocking — the unit-level behaviorProb tests ([lines 552-658](https://github.com/tangle-network/agent-knowledge/blob/e289c54ac3d7fdbb2a915e397b771a2fcfbe0ba8/tests/memory-holdout.

🟡 LOW Tests inherit module-level sessionRegistry singleton — implicit cross-test state coupling — tests/memory-holdout.test.ts

applySessionStickyRetrievalHoldout (holdout.ts:341) uses a module-level Map<configHash, Map<sessionId, state>>. Tests in this file (lines 342, 393, 225, 138) all share this singleton. Currently safe because each describe block uses a distinct epsilon+watchlist pair (distinct configHash) or distinct sessionId, so no bucket collision occurs. But the isolation is implicit, not enforced — a future test reusing sessionId='s-5' with watchlist=['m1','m7'] and epsilon=1 would inherit callCount from the earlier 'threads explicit session state' test and get callIndex=3 instead of 1. No current collision exists; flagged as fragility, not a bug. Fix: either reset session

---

_{tangletools · 2026-07-04T22:38:21Z · trace}

[tangletools]

❌ 1 Blocking Finding — e289c54a

Full multi-shot audit completed 2/2 planned shots over 3 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 2/2 planned shots over 3 changed files. Global verifier still owns final merge decision.

Full immutable report for this review: trace

Summary comment for this run: full summary

---

_{tangletools · 2026-07-04T22:38:21Z · immutable trace}

[tangletools]

✅ Auto-approved drewstone PR — e5cde50f

This PR was opened by the trusted drewstone account.
The full PR reviewer audit still runs separately and will publish findings if it detects issues.

_{tangletools · auto-approval · reason: drewstone_author · 2026-07-04T23:05:28Z}

[tangletools]

🟢 Value Audit — sound

Verdict	sound
Concerns	0 (none)
Heuristic	0.0s
Duplication	0.1s
Interrogation	304.5s (2 bridge agents)
Total	304.6s

💰 Value — error

value agent produced no parseable value-audit JSON.

• Model: opencode/zai-coding-plan/glm-5.2
• Bridge attempts: 2
• Bridge error: opencode/kimi-for-coding/k2p7: bridge stream ended without value-audit content

🎯 Usefulness — sound

A well-layered, default-off retrieval-holdout hook that correctly reuses agent-eval's statistics/off-policy substrate and integrates at the existing search→render seam; lands ahead of its documented first consumer (EXP-007).

• Integration: Reachable through the public memory-adapter contract. The hook fires inside defaultGetMemoryContext (src/memory/adapter.ts:19-25) — called by every AgentMemoryAdapter.getContext() path including Neo4j (src/memory/neo4j.ts:89,133) — and the two Neo4j bypass paths wire emitRetrievalHoldoutBypass directly (src/memory/neo4j.ts:63-70, 115-122). The pure helpers (applyRetrievalHoldout, `applySes
• Fit with existing patterns: Fits the codebase's grain and respects the AGENTS.md layering rule (agent-knowledge → agent-eval, never reverse). The PR reuses the substrate's mulberry32 rather than forking a PRNG (src/memory/holdout.ts:142-144), and the off-policy converter emits OffPolicyTrajectory so analysis consumes agent-eval's estimators directly — no parallel statistics code. The search→render seam is the existing on
• Real-world viability: Happy path, error paths, and edge inputs are covered. safeEmit (src/memory/holdout.ts:197-206) swallows onEvent failures so a misbehaving sink can't break retrieval — tested at memory-holdout.test.ts:480-504. Config validation fails loud for malformed epsilon/watchlist (src/memory/holdout.ts:181-192). The off-policy converter throws on missing rewards or corrupt drop events rather than silentl
• Model: opencode/zai-coding-plan/glm-5.2
• Bridge attempts: 1

No concerns — sound change, no better or existing approach found. ✅

---

What this audit checks

It judges the change on its merits — not whether it was tasked out in an issue. Unticketed, fast-moving work is fine; the question is whether the change is good and whether a better or existing approach should be used instead.

Pass	What it asks
Heuristic	Vague title? Whitespace-only or cruft-bearing diff? (content signals only)
Duplication	Do added function/class names already exist elsewhere in the repo?
Value Audit	What does it do? What goal does it achieve? Is it good? Better architecture or already-exists?
Usefulness Audit	Does it integrate and fit? Will it hold up in real use and actually get used?

Findings are concerns, not blocks — the human reviewer decides what to do with them.

_{value-audit · 20260704T231240Z}

[tangletools]

✅ No Blockers — e5cde50f

Review health 100/100 · Reviewer score 63/100 · Confidence 70/100 · 9 findings (2 medium, 7 low)

	glm	deepseek	aggregate
Readiness	86	63	63
Confidence	70	70	70
Correctness	86	63	63
Security	86	63	63
Testing	86	63	63
Architecture	86	63	63

Reviewer score is advisory once the run is complete and the verdict has no blockers.

Full multi-shot audit completed 2/2 planned shots over 3 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 2/2 planned shots over 3 changed files. Global verifier still owns final merge decision.

🟠 MEDIUM canonicalJson has no recursion guard — stack overflow on circular scope references — src/memory/holdout.ts

The recursive object walker at holdout.ts:150-164 has no cycle detection. If a caller passes an AgentMemoryScope (or scope.tags) containing a circular reference, canonicalJson will overflow the stack rather than throwing a clean TypeError. While AgentMemoryScope.tags is typed as Record<string, string>, runtime values can diverge. The scopeHash is computed from canonicalJson(ctx.scope) at holdout.ts:299, which passes the raw scope object. Fix: add a WeakSet visited guard that throws TypeError on cycle detection.

🟠 MEDIUM AgentMemoryAdapter.search ignores holdout option silently — src/memory/neo4j.ts

The search method (neo4j.ts:26) receives AgentMemorySearchOptions which includes holdout?: RetrievalHoldoutConfig (types.ts:53), but the method ignores it entirely — it returns raw hits with no holdout logging or suppression. This is by design (holdout applies at the getContext seam), but the shared AgentMemorySearchOptions type makes it easy for a caller to pass holdout to search and get no behavior — silently corrupting the event stream. Fix: either strip holdout from the search options type (prefer a narrower type), or document this at the AgentMemoryAdapter.search declaration.

🟡 LOW deterministicRng is a breaking change — old logs unreplayable with new code — src/memory/holdout.ts

The function changed from parseInt(sha256(key).slice(0,13), 16) / 16**13 (52-bit linear) to mulberry32(parseInt(sha256(key).slice(0,8), 16))() (32-bit PRNG). Both are deterministic and uniform, but produce different numeric values for the same key. Any audit replaying old holdout events against new code would see different assignment decisions. The test calibration at tests/memory-holdout.test.ts:869-891 was updated to the new rng, so the new code passes its own tests, but there is no migration path or version-aware rng selection for backward-compatible replay.

🟡 LOW deterministicRng output changed silently — breaks replay of any pre-existing logged draws — src/memory/holdout.ts

Old: Number.parseInt(sha256(key).slice(0,13),16)/16**13 (direct 52-bit hash fraction). New: mulberry32(Number.parseInt(sha256(key).slice(0,8),16))() (32-bit seed into mulberry32 PRNG). Same signature, different output for the same key. Design rule D5 (comment line 138) claims 'every draw is replayable from the logged keys alone' — true only if the same deterministicRng impl is used at replay. Any event log recorded under the old impl would not reproduce under the new one, silently. Mitigated by: feature is default-off, design doc is DRAFT (2026-07-03-DRAFT-o3-holdout-design.md), and the empirical-calibration test ([line 869](https://github.com/tangle-network/agent-k

🟡 LOW safeEmit does not catch async sink rejections — unhandled promise rejection possible — src/memory/holdout.ts

The try/catch in safeEmit only catches synchronous throws from config.onEvent(event). If a consumer passes an async onEvent function that rejects (allowed because void-returning types accept async functions in TypeScript), the rejection becomes an unhandled promise rejection. The test at memory-holdout.test.ts:480-503 uses a synchronous throw which is caught correctly, but async rejection is not covered. Fix: either (a) await config.onEvent(event) and mark safeEmit async, or (b) add .catch() chaining.

🟡 LOW AgentMemorySearchOptions.holdout is silently ignored on direct adapter.search() calls — src/memory/neo4j.ts

AgentMemorySearchOptions (types.ts:53) carries holdout?: RetrievalHoldoutConfig and is the parameter type for both search() and getContext(). neo4j search() (line 26-44) threads searchOptions through searchNeo4jMemory but never applies the holdout or emits any event; only getContext() does (via defaultGetMemoryContext or emitRetrievalHoldoutBypass). This is the intended layering ('holdout applies between search and render' per adapter.ts:16) — search returns the eligibility set E — but a consumer reading the type contract who calls adapter.search(query, {holdout}) gets a silent no-op with zero events emitted. Suggest either a one-line doc comment on the holdout f

🟡 LOW canonicalJson/assertValidHoldoutConfig in bypass path can throw inside getContext, breaking retrieval — src/memory/neo4j.ts

safeEmit (holdout.ts:197) isolates onEvent failures so a throwing sink never breaks retrieval — but it only wraps the sink call, not holdoutEventBase() which runs first (holdout.ts:336). holdoutEventBase calls canonicalJson(ctx.scope) for scopeHash (line 299), and emitRetrievalHoldoutBypass calls assertValidHoldoutConfig before constructing the event (line 334). A malformed scope (e.g. bigint in tags, which canonicalJson throws on per [line 152](https://github.com/tangle-network/agent-knowledge/blob/e5cde50f43996bfd76646ee6f9796de35695f196/src/me

🟡 LOW Missing explicit test for Infinity epsilon config — tests/memory-holdout.test.ts

The test covers NaN and out-of-range epsilon values but not Infinity. The implementation catches it via Number.isFinite (line 183 of holdout.ts), so this is a coverage gap, not a correctness gap.

🟡 LOW canonicalJson standalone undefined/null boundary untested — tests/memory-holdout.test.ts

canonicalJson converts undefined to 'null' at the top level (line 154 of holdout.ts), but the holdoutEventBase call site gates scopeHash on ctx.scope !== undefined (line 298), so this path is unreachable. A test for canonicalJson(undefined) directly would document the intentional gating and prevent future callers from hitting a surprising serialization.

---

_{tangletools · 2026-07-04T23:19:17Z · trace}

Superseded by re-review — no blocking findings on latest commit.

[tangletools]

✅ Approved — 9 non-blocking findings — e5cde50f

Full multi-shot audit completed 2/2 planned shots over 3 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 2/2 planned shots over 3 changed files. Global verifier still owns final merge decision.

Full immutable report for this review: trace

Summary comment for this run: full summary

---

_{tangletools · 2026-07-04T23:19:17Z · immutable trace}