PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: Every §10.5 requirement is met in the same commit — explicit planeMember decision, declared-list entry, default derived from the single anchor — and the consumer change removes a genuine A1 that would have misbehaved in relocated profiles rather than merely reading impolitely. My primary finding is a cross-PR pattern observation, not a defect in this diff. Not Request Changes: the two things that would have made this cosmetic-only (a missing list entry, or a seam that becomes a second default) are both explicitly handled.

Peer-Review Opening: The old line was not just an ADR style violation — path.resolve(aiConfig.neoRootDir, '.neo-ai-data', 'kb-sync') would have written KB resume state into the canonical plane from a relocated one. That makes this a correctness fix wearing a config-hygiene hat.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: ADR 0019 before the diff per §critical_gates rule 10 — §3's catalog, §5's sanctioned patterns, and §10.5's member-derivation rule including the #15932 completeness half, all read repeatedly today across #16155, #16172, and now this — plus the KB configBase.mjs anchor context, the existing getResumeStateDir implementation, and a freshly-fetched origin/dev before comparing (it had moved twice already this session).
• Expected Solution Shape: a leaf whose default derives from the single KB plane anchor, carrying an explicit planeMember decision, added to the declared list in the same commit because declaration and membership are one act. The consumer must read the resolved leaf at the use site — not reconstruct the data root from the checkout — and the existing instance seam must stay a test override rather than becoming a competing default. Missing or blank must fail loudly, since a silent empty string turns a path operation into a cwd write.
• Patch Verdict: Matches on every point. leaf(path.resolve(planeDataRoot, 'kb-sync'), 'NEO_KB_EMBEDDING_RESUME_STATE_DIR', 'string', {planeMember: true}) — anchor-derived default (A9's sanctioned form, not per-leaf re-derivation), explicit decision so derivePlaneMemberPaths cannot fail closed on it, env binding present so relocated profiles can place it. PLANE_MEMBER_PATHS gains 'embeddingResumeStateDir' in the same commit, which the set-equality spec requires. And the consumer now reads aiConfig.embeddingResumeStateDir directly with no defensive ?..
• Premise Coherence: Coheres with verify-before-assert in the evidence rather than the code, and unusually well — see the Test-Evidence audit. Declining to claim a green local gate while reporting 17 reds, and then removing your own additions to reproduce a pre-existing failure, is the discipline stated rather than performed.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #16174
• Related Graph Nodes: #15931 (the Neural Link logPath fail-loud parity lineage this KB base already carried) · #15932 (declaration-and-membership-are-one-act) · ADR 0019 §10.5 · PR #16172 (the other plane-member leaf routed to worker storage today) · PR #16155 (per-profile placement)
• Origin Session ID: 0a7f5f1d-cf12-4698-984c-17b64eea5178

🔬 Depth Floor

Challenge OR documented search (per guide §7.1):

• Challenge (primary — a cross-PR pattern, and this is its second instance today): embeddingResumeStateDir is planeMember: true, and test/playwright/configTemplateResolver.mjs now routes it to worker-local storage. So in every Playwright worker this declared member resolves to a tmpdir path not beneath the resolved plane.dataRoot, and assertPlaneMemberCoherence passes it only via §10.5's "or be explicitly placed (resolved ≠ its declared default)" clause.

  That is exactly the dependency I flagged on PR #16172 this morning for orchestrator.deploymentStateBridge.snapshotPath. Two independent PRs, one day, both relying on the same escape for test isolation — which means it is no longer an exception, it is the pattern for isolating a plane member in tests, and it is still recorded nowhere.

  My #16172 note suggested a clause on that ticket. With a second instance I would revise that: this belongs in ADR 0019 §10.5 itself, one sentence saying that an env-placed member in a disposable test root satisfies the explicitly-placed clause by design. Otherwise the next person to tighten member coherence — and §10.5 was edited today by #16155 — breaks every Playwright worker across two subsystems, with a failure whose cause is invisible from the assertion. Not this PR's job to write it; naming it because I have now seen it twice and the second sighting is what makes it a pattern rather than a quirk.

• Where I looked and the code was better than "config hygiene": the removed line was path.resolve(aiConfig.neoRootDir, '.neo-ai-data', 'kb-sync'). That is A1 — re-deriving a plane path from the checkout root with .neo-ai-data hardcoded — but the consequence matters more than the classification. In a parity container or a Playwright worker, the KB's resolved plane root is not <checkout>/.neo-ai-data, so the old code would have written resume state into the canonical plane while every other KB path went to the relocated one. A split-brain resume marker is worse than a missing one: a shadow-swap embedding could resume from another plane's progress. The fix is therefore a correctness repair, and I would not have found that by pattern-matching the antipattern ID alone.

• Two smaller things I checked and found deliberate: the JSDoc says "VectorService.resumeStateDir remains an explicit test seam, never a second default" — pre-empting the A6 leaf-plus-duplicate-definition hazard by declaring the seam's status rather than leaving a reader to infer it. And the fail-loud guard tests typeof !== 'string' || trim() === '', so a blank or whitespace-only env binding throws instead of resolving to an empty string that would silently make every path operation relative to the cwd. That whitespace half is the same guard shape Euclid added to isOpaquePlaneId in #16175 today; two seats converging independently on it is a decent signal it belongs in the house style.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: accurate, including the negative claim — "the resume-state path no longer contains a .neo-ai-data or neoRootDir construction; the remaining neoRootDir read in VectorService belongs to unrelated operation metadata." That is the right way to state a partial removal: name what remains and why, rather than implying the file is clean.
• Anchor & Echo summaries: the @summary changed from "gitignored directory" to "configured directory", which is the substantive shift — the path is no longer a location the code knows about, it is a value the config owns.
• [RETROSPECTIVE] tag: N/A — none claimed.
• Linked anchors: #15931 genuinely established the fail-loud parity precedent this KB base follows.

Findings: Pass.

🧠 Graph Ingestion Notes

• [KB_GAP]: None.
• [TOOLING_GAP]: Carried. merge-readiness projection returns IDENTITY_BINDING_MISSING from my seat; get_pull_request_diff's file parameter is ignored (reported on #16173), so I used local per-file git diff throughout.
• [RETROSPECTIVE]: A hardcoded data-root reconstruction is a relocation bug, not a style violation. path.resolve(root, '.neo-ai-data', 'sub') reads as harmless — it produces the correct path on the canonical plane, which is where it is written and tested. It only becomes wrong once a profile relocates the plane, and then it fails partially: every leaf-derived path moves, and this one does not, so one subsystem writes into two planes at once. That partial failure is much harder to see than a total one, because nothing errors and most paths are right. The general form: when a plane relocates, the paths that break are the ones that never asked the config where the plane is — so the antipattern's real cost is proportional to how many profiles exist, and it was zero until relocation shipped.

N/A Audits — 📡 🔗 🧠

N/A across listed dimensions: no OpenAPI tool description touched, no skill file or workflow convention introduced, and no turn-loaded or skill-loaded substrate modified.

🎯 Close-Target Audit

• Close-targets identified: Resolves #16174, newline-isolated. Related: #15931 non-closing and correctly formed.
• For each #N: #16174 is the delivered leaf.

Findings: Pass. Single commit bbc80f2209 carrying (#16174). True surface verified against a freshly-fetched dev: 10 files, +70/-12, matching the commit count.

📑 Contract Completeness Audit

• New public config path recorded: ai/scripts/lint/config-leaf-parity.json gains embeddingResumeStateDir in the same commit — the mechanical record the body names as required, and the guard that would otherwise have gone red.
• Declared membership consistent: {planeMember: true} on the leaf and 'embeddingResumeStateDir' in this base's PLANE_MEMBER_PATHS. Per #15932 the spec asserts set-equality rather than a pinned count, so both halves are required and both are present; a leaf marked planeMember but absent from the list would have failed that assertion rather than passing quietly.
• Placement for relocated profiles: docker-compose.dev.yml and configTemplateResolver.mjs each place it explicitly, which is what the parity profile and the Playwright workers respectively need.

Findings: Pass — the completeness half applies to per-server declaring bases as well as Tier-1, and this is a per-server base handled correctly.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head CI at bbc80f22096c1c493023dfa98c8adc9231cd5c4d — 16 checks passing, none pending, none failing, verified live. Base dev, one commit.
• Author per-surface non-CI receipt: 18 on the KB consumer, 59 across plane/profile/worker placement, 3 on the KB env binding, plus ai:lint-config-template-ssot, check-aiconfig-antipatterns, and check-aiconfig-test-mutation all passing. Those last two are the mechanical backstops rule 10 exists for, and both passing on a leaf addition plus a consumer rewrite is the strongest single item.
• Reviewer falsifier: none run. I audited the leaf's four leaf(...) arguments against §10.5's requirements, confirmed the declared-list entry is in the same commit, and traced the consumer's before/after to establish that the removed construction was a relocation bug rather than a style issue. Source audit, stated as such.
• Test location: pass — consumer behaviour in VectorService.WorkVolumeBranching.spec.mjs, placement in ParityPlaneVolumeScoping.spec.mjs and ConfigTemplateResolver.spec.mjs, env binding in the KB config.template.spec.mjs, membership in planeConfig.spec.mjs. Each assertion with the surface it constrains.

Findings: Pass, and the evidence discipline deserves specific credit beyond the counts. Two things most PRs get wrong and this one does not:

First, you report the full local ledger as 10,338 passed; 17 failed and then write "The full local gate is therefore not claimed green; hosted exact-head CI remains required." Reporting a red count you are not claiming is the opposite of the usual move, and it lets a reviewer weigh it instead of discovering it.

Second, and better: the baseline falsifier. "Temporarily removing this PR's KB leaf/assertion additions reproduced the identical failure." That is a real control — it distinguishes "my change broke this" from "this was already broken" by intervention rather than by assertion. Most pre-existing-failure claims are offered as assurances; this one was tested. I did not need to re-derive it because the control is the evidence.

📋 Required Actions

No required actions — eligible for human merge.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 98 — anchor-derived default from the single planeDataRoot, explicit planeMember decision, declared list updated in the same act, env binding for relocated profiles, and the consumer reading the resolved leaf at the use site with the test seam explicitly demoted to a seam. 2 deducted only for the undocumented §10.5 escape this now shares with #16172, which is a substrate gap rather than a fault in this diff.
• [CONTENT_COMPLETENESS]: 98 — the @summary shift from "gitignored" to "configured" names the real change, the JSDoc pre-empts the seam-as-second-default reading, and the body's negative claim about the remaining neoRootDir read is scoped honestly. 2 deducted: nothing records why a worker-local resolution is legitimate for a declared member.
• [EXECUTION_QUALITY]: 98 — fails loud on missing, blank, and whitespace-only; no defensive ?. on the config read; the removed construction was a genuine relocation bug rather than a style nit. 2 deducted for the same undocumented coupling.
• [PRODUCTIVITY]: 98 — all close-target ACs are config/source/unit-verifiable and covered, with Post-Merge Validation: none required legitimately earned rather than asserted.
• [IMPACT]: 74 — closes a split-brain resume-marker path where a relocated KB would have resumed shadow-swap embedding from the canonical plane's progress. Bounded to one subsystem, but the failure it prevents is silent and data-shaped.
• [COMPLEXITY]: 34 — one leaf, one list entry, one consumer guard, two placement lines; the only load is knowing why a declared member may resolve outside its plane in tests.
• [EFFORT_PROFILE]: Quick Win — small, complete, mechanically guarded, with the evidence honesty doing as much work as the diff.

Approved at bbc80f2209, 16 checks green, no required actions. The one thing I would carry forward is not about this PR: with two leaves now depending on §10.5's explicitly-placed clause for test isolation, that clause has become load-bearing infrastructure and should be documented in the ADR rather than rediscovered by whoever tightens member coherence next.

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

🌿