PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: A test-isolation change with no production surface, where the root cause was reproduced rather than theorised and the repair replaces shared-state mutation with the ADR-sanctioned construction-isolation idiom. All three close-target ACs are met with pre-merge-verifiable evidence. My one concern is a forward-looking structural note about where the new guard lives, not a defect.

Peer-Review Opening: The thing that makes this reviewable at a glance is the falsifier. A flaky-spec ticket that arrives with "run these two specs in one worker and it reproduces" converts a timing story into a mechanism, and it is also what explains why the fix lands in a file the ticket never names. Approving.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #17043's body and its three ACs; the changed-file list (which does not contain the spec the ticket names — the first thing worth explaining); dev source of ai/ConfigProvider.mjs for createConfigProxy; sibling specs already using that idiom; ADR-0019 §4 / §5.4 (the B4 safety-critical rule — mandatory read-gate for ai/ config surfaces, and this fixture manipulates config identity directly).
• Expected Solution Shape: Either a deterministic seam in the named flaky spec, or — if the flake is contamination — removal of the shared-state mutation that causes it, replaced by isolation by construction rather than by save/restore. What this must NOT do: change production behaviour, or keep mutating Neo.ai.Config / Neo.classHierarchyMap under a tidier wrapper. Test isolation is the whole subject here, so ADR-0019 B4 is the governing authority rather than a side check.
• Patch Verdict: Improves on the expected shape. I expected at best a save/restore made airtight. Instead the save/restore is deleted outright: createConfigProxy(Neo.create(ConfigBase)) + config.destroy() replaces four namespace deletions and their eight-branch restore. That is §5.4's "tests isolate by construction… never mutate the shared singleton" applied literally, and it is net-negative in code size. I verified createConfigProxy is a real sanctioned export (ai/ConfigProvider.mjs:552) with existing precedent in five sibling specs including configBase.spec.mjs and config.template.spec.mjs — an established idiom, not one invented for this fix.
• Premise Coherence: Coheres with verify-before-assert. The ticket offered a hypothesis ("observes the projection before the writer-status downgrade settles") and explicitly refused to prescribe ("owner to verify, not prescribe"). The author falsified that hypothesis and replaced it with a reproduced mechanism, then said so in "Deltas from ticket" rather than quietly fixing something else. That is the ticket-framing-is-a-hypothesis discipline working end to end.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #17043
• Related Graph Nodes: PR #17040 (the victim run that triggered the third-occurrence commitment), ADR-0019 §4/§5.4 (B4 singleton-mutation danger), check-aiconfig-test-mutation (the mechanical anchor for that rule)
• Origin Session ID: 471d17f2-777c-4676-a137-fa37a9ac834d

🔬 Depth Floor

The cross-file mismatch is the first thing a reviewer should challenge, and it resolves correctly. #17043 names MemoryCoreRecorderService.spec.mjs:704; this PR touches only compactGraphLog.spec.mjs. That is right, because the named spec was the victim and this fixture was the culprit: importing config.template.mjs while the canonical config namespaces were temporarily deleted left the cached proxy bound to a different singleton after the old runtime target was restored, so a later spec refreshed one config object while production services read another. The PR does not ask me to take that on trust — it reproduces it by running the two specs in one worker pre-fix. A flake ticket resolved by a deterministic falsifier is the strongest form this class of fix takes.

Challenge — the new regression guard is placed at the call site rather than in the helper.

runtimeConfigBefore is captured in the test body (:110) and asserted after the helper returns (:130). It is correct and it covers today, because withMemoryCoreConfigTemplate has exactly one call site. The structural weakness is that the guard protects the caller that remembered it, not the helper's contract. A second call site added later inherits the isolation but not the assertion, and the failure it would reintroduce is the one this PR exists to remove — cross-spec contamination, which by nature surfaces in a different file on a different day.

This is the same shape ADR-0019 §10.5 names in a different context: a check that covers what someone remembered to declare is weaker than one derived from the set itself, and the operation that actually happens (add a member, forget the list) passes green forever. Moving the capture/assert pair inside the helper's try/finally would make the invariant structural — this helper never mutates runtime config identity — at roughly the same line count. Non-blocking, and I would not hold a green flake-fix for it.

Two things I checked and am not raising: config.destroy() in the finally cannot mask a construction error, because Neo.create(ConfigBase) runs in the const initialiser outside the try, so a throw there propagates before the finally exists. And the colon-alignment churn in four unrelated computeCompactionPlan cases is house style, not scope creep.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: framing matches the diff, and its central claim is a reproduction rather than an inference
• "Deltas from ticket" accurately records that the ticket's stated hypothesis was falsified, instead of presenting the fix as what was asked for
• Anchor & Echo: the new inline comment explains the mechanism (ESM cache split stranding the proxy on a different instance) rather than restating the assertion
• [RETROSPECTIVE]: N/A — none claimed

Findings: Pass.

🧠 Graph Ingestion Notes

• [KB_GAP]: None. The diff demonstrates correct grasp of both the B4 rule and the construction-isolation idiom the codebase already sanctions.
• [TOOLING_GAP]: None introduced. Worth recording from the evidence section: check-aiconfig-test-mutation scanned 1,204 test files and reported zero new violations — but note it would not have caught the pre-fix code either, because the old fixture mutated Neo.ai.Config / Neo.classHierarchyMap (namespace identity) rather than assigning aiConfig.<path> = …, which is the pattern that gate matches. The gate is not weakened by this PR; the observation is that this contamination class sits just outside its grammar, which is useful context for whoever next tightens it.
• [RETROSPECTIVE]: Save/restore of shared identity is not isolation — it is a narrower window for the same contamination. The old fixture was careful: it captured four originals and restored each with an explicit undefined-branch. It still leaked, because the import that ran inside the window cached a proxy bound to the temporarily-absent target, so restoring the namespace afterwards could not un-bind what had already been captured. The durable lesson is that restore-based isolation protects the variable and not the references taken while it was swapped — and the only robust answer is to never swap the shared thing, which is exactly what ADR-0019 §5.4 prescribes and what this PR does. Also worth keeping: the flake was diagnosed by making it deterministic, and the ticket's own hypothesis was wrong, so a third occurrence earning a ticket paid off precisely because it forced the mechanism hunt.

N/A Audits — 📑 🪜 📡 🔗

N/A across listed dimensions: single spec file, no public or consumed surface, no OpenAPI or tool description, no skill/convention/primitive; the close-target ACs are entirely unit-harness determinism, which the PR's Evidence: L2 → L2 required. No residuals. line correctly matches, so no evidence-ladder gap exists to audit.

🎯 Close-Target Audit

• Close-target: Resolves #17043, newline-isolated, single delivered leaf
• #17043 is not epic-labeled
• No Closes / Fixes variants, no prose-embedded or comma-separated targets

Findings: Pass. AC-by-AC, verified independently of the PR's own checklist:

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head required CI green at 246dc77a6872b761e0275bec2af8800e8f722d4c — gh pr checks exit 0, no pending, no failures, CLEAN / MERGEABLE. Author receipts are head-appropriate and unusually complete for a flake fix: the two-spec pair at --workers=1 (38/38), both --repeat-each runs, and the AiConfig mutation gate.
• Reviewer falsifier: my named concern was whether the new idiom is sanctioned or improvised. Falsified by locating createConfigProxy at ConfigProvider.mjs:552 and confirming five existing spec consumers, rather than by rerunning tests — a rerun cannot answer an idiom question.
• Test location: no new file; the change is confined to an existing spec's fixture, which is where a fixture defect belongs.

Findings: Pass. The three unrelated failures named in the full-suite run (two repository-census specs traversing ignored deployment backups, one local Neural Link unhealthy) are declared as local-environment rather than presented as green — the honest form.

📋 Required Actions

No required actions — eligible for human merge.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 97 — Replaces shared-namespace mutation with the construction-isolation idiom the codebase already sanctions, in the exact direction ADR-0019 §5.4 prescribes, reusing an existing export rather than inventing a fixture helper. 3 deducted for the guard sitting at the call site rather than in the helper contract.
• [CONTENT_COMPLETENESS]: 96 — The new JSDoc names what the helper provides, and the inline comment explains the ESM-cache mechanism a future reader would otherwise have to re-derive. "Deltas from ticket" correctly records the falsified hypothesis.
• [EXECUTION_QUALITY]: 96 — Correct at every branch I traced, including that the finally cannot mask a construction throw. The evidence is the strong part: a deterministic reproduction plus 50× and 20× repeat runs on both sides of the contamination.
• [PRODUCTIVITY]: 100 — All three ACs delivered and independently verifiable pre-merge; the residual list is honestly empty rather than padded.
• [IMPACT]: 55 — No production behaviour changes, but it removes a cross-spec contamination source that turned a 13,000-pass run red on an unrelated PR and cost three rerun cycles in one day. CI trust is the asset being repaired.
• [COMPLEXITY]: 25 — One spec file; the reasoning load was in the diagnosis, and the resulting diff is a deletion plus a two-import construction.
• [EFFORT_PROFILE]: Quick Win — A net-negative diff that removes a whole contamination class, after the expensive part (making an intermittent failure deterministic) was already spent.

The habit worth naming for the graph: this ticket existed because a third occurrence was the agreed trigger, and the ticket's own stated cause turned out to be wrong. Both facts are in the record rather than smoothed over, which is what makes the next flake cheaper to chase.

— Grace (Claude Opus 5, Claude Code) 🖖