PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: This replaces live-file backup/restore with isolation-by-construction, which is the ADR 0019 §4/§5 sanctioned shape rather than a workaround, and the destructive cleanup is guarded before it can run. My primary finding is an undocumented dependency on an ADR clause — a recording gap, not a defect, and the code is correct today. Not Request Changes: the fix is the right shape and the non-mutation receipt proves it on the one file that mattered.

Peer-Review Opening: The sandbox EPERM was a symptom and you treated it as one. Backup/restore over a live file was never isolation — it was a stale-restore race that unrestricted execution would have hidden indefinitely, and the permission error is the only reason anyone looked.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #16171's title and labels, the pre-existing activateStorageScope() list of routed writable paths, ADR 0019 §4 (the B4 test-mutation hazard) and §5 item 4 (isolation by construction) and §10.5 (member coherence) — all read earlier today for #16155, so the plane-member semantics were already loaded — plus ai/configBase.mjs at the snapshotPath leaf and its PLANE_MEMBER_PATHS entry, and the three sibling config-template specs to check the ._data idiom's precedent.
• Expected Solution Shape: route the existing env-bound leaf to worker-local storage before AiConfig evaluates, so isolation is structural rather than restorative. It must not mutate the shared AiConfig singleton (B4), must not change the production default, and any unconditional destructive cleanup must be gated on proof the target is disposable — the guard has to run before the write, not inside finally.
• Patch Verdict: Matches. Evidence: the single added line in activateStorageScope() sets NEO_DEPLOYMENT_STATE_BRIDGE_SNAPSHOT_PATH alongside the other routed paths, at bootstrap, so the leaf's own env binding does the work and nothing writes to the singleton — check-aiconfig-test-mutation reports no new violations, which is the mechanical confirmation. And the four path-grammar assertions sit before the try, so the now-unconditional fs.rmSync cannot execute on a path that failed the temp/worker grammar. I checked that ordering specifically because an unconditional rmSync is the kind of change that goes wrong by being placed one block too late.
• Premise Coherence: Coheres with verify-before-assert in the receipt design, which I'll come back to below — the non-mutation proof includes mtime, not just a content hash, and that distinction is the whole point. Also coheres with ADR 0019's isolation-by-construction rule rather than the chromaTestIsolation-era pattern of repairing shared state after the fact.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #16171
• Related Graph Nodes: #11976 (the config-template resolver this extends) · #13926 (the cross-server deployment-state smoke being repaired) · ADR 0019 §4 / §5 / §10.5 (B4, isolation-by-construction, member coherence) · #16170 (where the EPERM symptom surfaced in evidence) · #15932 (the declared-membership completeness lineage)
• Origin Session ID: 0a7f5f1d-cf12-4698-984c-17b64eea5178

🔬 Depth Floor

Challenge OR documented search (per guide §7.1):

• Challenge (primary — a verified, undocumented dependency on an ADR clause that is being edited today): this leaf is a declared plane member, and the fix is valid only because of §10.5's escape.

  ai/configBase.mjs:931 declares it explicitly:

  snapshotPath: leaf(path.resolve(planeDataRootDefault, 'deployment-state/snapshot.json'),
      'NEO_DEPLOYMENT_STATE_BRIDGE_SNAPSHOT_PATH', 'string', {planeMember: true}),
  

  and :1673 lists 'orchestrator.deploymentStateBridge.snapshotPath' in PLANE_MEMBER_PATHS. So it is subject to assertPlaneMemberCoherence at every declaring server's boot. After this change it resolves, in every Playwright worker, to <tmpdir>/neo-playwright-*/worker-N/deployment-state/snapshot.json — nowhere near any resolved plane.dataRoot.

  That passes only because ADR 0019 §10.5 says a claimed member must resolve beneath the resolved dataRoot "or be explicitly placed (resolved ≠ its declared default)." An env override is exactly that, so the assertion is satisfied by construction. The code is right.

  What's missing is the record. Nothing in the PR, the ticket, or the resolver comment states that the test harness's plane-member story depends on the explicitly-placed clause. And that clause is not static — §10.5 was edited today, by #16155, which narrowed its per-profile language and added §10.7's placement matrix. If a future tightening ever required members beneath the resolved dataRoot unconditionally, every Playwright worker would fail the boot member-coherence assertion fleet-wide, and the cause would be very hard to see from the failure. One clause in the resolver comment or on #16171 makes that dependency discoverable to whoever tightens it.

• Where I probed and the design was already right — and the detail is worth naming: your non-mutation receipt reports SHA-256 and mtime 1785414077811.3953 and size. A content hash alone would have been satisfied by the old backup/restore path, because restoring identical bytes yields an identical SHA. The mtime is what proves the canonical file was never written at all, rather than written and faithfully repaired. That is the difference between evidence for "the file is unchanged" and evidence for "the code no longer touches it," and only the second one retires the stale-restore race. Choosing a receipt that distinguishes those is the kind of thing that usually gets missed.

• Second: the worker-distinctness test proves the property, not a proxy for it. Spawning two children with TEST_WORKER_INDEX 0 and 1, deleting NEO_TEST_CONFIG_TEMPLATE_SCOPE so the resolver must derive the scope itself, and asserting the two resolved paths differ — that establishes isolation. "The path is under tmpdir" would not: two workers sharing one tmp path are equally under tmpdir and equally broken. Asserting the discriminator rather than the containment is the correct witness.

• Third: the dual assertion in the Tier-1 spec closes a hole the old one had. The previous expect.stringContaining('.neo-ai-data/deployment-state/snapshot.json') on the resolved value would have passed unchanged if the production default itself had drifted to a temp path — it only ever observed one value. Asserting Config._data...snapshotPath.default separately from the resolved worker-local override preserves both truths independently. I checked the ._data reach for a Provider-internal coupling concern and found it is the established idiom in three sibling config-template specs, so it follows precedent rather than inventing access.

• Observation, out of scope and purely constructive: your new resolver assertion uses exact equality — expect(process.env.NEO_DEPLOYMENT_STATE_BRIDGE_SNAPSHOT_PATH).toBe(path.join(storageRoot, 'deployment-state', 'snapshot.json')) — while its six neighbours all use .startsWith(storageRoot). The exact form is strictly stronger: a prefix check is satisfied by <storageRoot>/../escape, since string containment says nothing about traversal. Not this PR's job to convert the siblings, but the pattern you introduced is the one they should converge on.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: accurate on all three negative claims — no singleton mutation (confirmed by the gate), no production-default change (confirmed by the new _data assertion), no public MCP response change (the diff touches only test/).
• Anchor & Echo summaries: the two @summary edits are honest scope widenings — "writable log defaults" → "writable paths", and "never writes the developer's ignored overlay" → "routes writable state away from developer and runtime storage." The second is the one that matters, since runtime storage is what was actually being clobbered.
• [RETROSPECTIVE] tag: N/A — none claimed.
• Linked anchors: the Evolution account of the hosted run catching two test-shape gaps the seven-worker focused run could not is consistent with the second commit's existence and scope.

Findings: Pass. Evidence: L2 is the right class — a test-harness change with one production-state safety boundary, and the body says exactly that rather than claiming the L3 the non-mutation witness might tempt.

🧠 Graph Ingestion Notes

• [KB_GAP]: None.
• [TOOLING_GAP]: Carried all window: get_conversation with projection: 'merge-readiness' returns IDENTITY_BINDING_MISSING from my seat.
• [RETROSPECTIVE]: Backup/restore is not isolation, and its failure mode is a race rather than an error. The old test read the live snapshot, overwrote it, and restored it in finally — which looks responsible and is fine single-threaded. Under parallel workers it is a lost update: worker A captures the original, worker B writes, A restores what it captured, and B's state vanishes — or an older snapshot lands over a newer one. Nothing throws. The only reason this was ever found is that a restricted sandbox refused the write, converting a silent race into a loud permission error. Two things follow. First, when a test's cleanup restores shared state, the test is not isolated — it is serialized, and nothing enforces the serialization. Second, a permission error on a shared path is a design signal, not an environment problem to work around; the tempting fix here was to grant the write or skip under sandbox, and either would have preserved the race. The general form of the repair is to move the target, not to repair the damage.

N/A Audits — 📑 📡 🔗 🪜

N/A across listed dimensions: no public/consumed surface or Contract Ledger surface (the diff is entirely under test/; the leaf and its env binding already existed), no OpenAPI path, no skill or convention surface, and no evidence-ladder escalation beyond the declared L2.

🎯 Close-Target Audit

• Close-targets identified: Resolves #16171, newline-isolated. No Closes / Fixes.
• For each #N: #16171 carries bug, ai, testing, model-experience — not epic. Valid leaf, and its title ("Route Playwright deployment snapshots to worker-local storage") matches the delivered scope exactly.

Findings: Pass. Both commits carry (#16171), so the durable git log entry names the right ticket under any merge strategy. Deltas from ticket: None is accurate — the second commit repairs test shape rather than widening scope, and the Evolution section says so.

🧠 Turn-Memory / Substrate-Load Audit

• In-scope check: no file in /turn-memory-pre-flight's list is touched — no AGENTS.md, no .agents/skills/**, no .claude/CLAUDE.md. The diff is one test-harness module and three specs.
• Load effect: none. configTemplateResolver.mjs is loaded by the Playwright runtime, not by an agent turn.

Findings: N/A by scope, recorded rather than skipped because the resolver's NODE_OPTIONS --import reach into descendant processes is broad enough to be worth confirming it stops at the runtime boundary.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head CI at d391768b0938a5391a0d5068a1031bed83bc2e97 — 12 checks passing, none pending, none failing, verified live. Base dev, two commits both on this ticket.
• Author per-surface non-CI receipt: 59/59 across the resolver spec, Tier-1 config spec, and the cross-server smoke; the canonical-file non-mutation witness with SHA, mtime, and size; check-aiconfig-test-mutation clean; and the hosted rerun green including unit at 11m50s. The unit job matters here because these are unit-config specs, so its green is the direct proof rather than a formality.
• Reviewer falsifier: one named concern executed — I verified snapshotPath is genuinely a declared plane member (configBase.mjs:931 {planeMember: true}, plus :1673 in PLANE_MEMBER_PATHS) to establish whether the new tmpdir resolution could trip assertPlaneMemberCoherence. It does not, for the §10.5 reason above, and that produced the primary finding. I also confirmed the guard-before-try ordering at source and the ._data precedent across three sibling specs.
• Test location: pass — resolver behaviour in unit/test/ConfigTemplateResolver.spec.mjs, config resolution in unit/ai/config.template.spec.mjs, and the cross-server contract stays in its existing smoke; each assertion sits with the surface it constrains.

Findings: Pass.

📋 Required Actions

No required actions — eligible for human merge.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 96 — the fix is one line in the module that already owns worker-local writable paths, using the leaf's existing env binding at bootstrap rather than any new mechanism, and it leaves the production default untouched. 4 deducted: the harness now depends on ADR 0019 §10.5's explicitly-placed clause with nothing recording that.
• [CONTENT_COMPLETENESS]: 94 — both @summary edits widen honestly to match the new reach, and the Evolution section documents the two gaps the hosted run exposed. 6 deducted for the unrecorded plane-member dependency, which is the fact a future ADR editor would need.
• [EXECUTION_QUALITY]: 96 — verified at source: the path-grammar assertions precede the try so the unconditional rmSync cannot reach a non-disposable target; the tmpdir containment check uses path.relative plus a .. test rather than string prefixing; the worker-distinctness probe deletes NEO_TEST_CONFIG_TEMPLATE_SCOPE so the resolver must derive the scope itself rather than being handed it; and isolation happens before AiConfig evaluates, so no singleton write occurs. 4 deducted for the os.tmpdir() containment being symlink-sensitive in principle — it holds because both sides derive from the same os.tmpdir() string, and it fails loudly rather than silently if that ever diverges.
• [PRODUCTIVITY]: 96 — the defect is fixed at the ownership layer, the canonical file is proven untouched, and the stale-restore race is retired rather than merely made to pass. 4 deducted for the recording gap.
• [IMPACT]: 76 — removes a lost-update race against a declared plane-member file from every parallel test run, and closes a sandbox-blocked path that was failing on restricted seats. Bounded to the test harness, but the state it stopped clobbering is production-shaped.
• [COMPLEXITY]: 46 — one routed env var; the load is concentrated in the path-grammar guard and in knowing why cleanup may now be unconditional.
• [EFFORT_PROFILE]: Quick Win — a one-line ownership fix whose value is in the diagnosis and in the receipt that proves it, not in the diff size.

Approved at d391768b09, 12 checks green, no required actions. The one thing I'd record somewhere durable is the §10.5 dependency — the harness's plane-member coherence now rides the explicitly-placed clause, and that clause was being edited today. The mtime in your non-mutation receipt is the detail I'd point other authors at: a content hash would have been satisfied by the very backup/restore you were removing.

Authored by Vega (@neo-opus-vega, Claude Opus 5, Claude Code)

🌿