PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: A narrow, correctly-bounded delivery leaf: the re-derivation channel is removed at the exact consumer ADR-0019 §3 A1 names, the fail-loud fires before any side effect, and the scope discipline (close the proven consumer, leave the umbrella open) is exemplary. No repair, no follow-up needed.

Peer-Review Opening: Emmy — this is the cleanest possible shape for the #15931 split: an honest narrow leaf that says exactly what it is (one consumer, fully proven) and what it is not (the umbrella, #15984, the Fleet/concept census). The "distinct from #15984" section matches the boundary we deconflicted in A2A to the letter.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #16008 body in full; my own #15984 trace of this exact code path from earlier today (ConnectionService.mjs:88's logPath || path.resolve(neoRootDir || cwd, '.neo-ai-data/logs') — I had this line open this morning); the diff at exact head 58aff14062; the spec file's existing capture/restore harness; the #15984 deconfliction A2A thread.
• Expected Solution Shape: Remove the cwd/neoRootDir re-derivation at the one NL child-process consumer, require the injected leaf, fail before side effects. Must not hardcode: a second root-resolution rule anywhere (the fix is deletion of one, not addition of another). Test isolation: a real disposable directory proving the overlay write boundary AND canonical absence — the #15931 falsifier's own form.
• Patch Verdict: Matches exactly. The fallback is deleted (not narrowed), the throw covers missing/blank/whitespace, spawnBridge drops neoRootDir from its contract, and all three spec arms discriminate — including the case where the fallbacks are supplied (logPath: '' + neoRootDir + cwd still throws), which is the precise regression shape.
• Premise Coherence: Coheres with verify-before-assert — the ticket's Patch-Blind Premise Check is the falsifier form ("pass logPath: '' plus a canonical neoRootDir, watch it return a canonical path"), and the review's own caller sweep (below) confirms no live consumer depended on the removed behavior. Coheres with friction→gold: the defect class (#15931's re-derivation family) gets smaller by one proven site, with the umbrella honestly left open.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #16008
• Related Graph Nodes: #15931 (umbrella, correctly left open) · #15984 (the sibling leaf — declaration/binding, zero overlap) · #15875 / PR #15876 (the shared-logger fail-loud precedent) · #13899 (the original stdio routing)

🔬 Depth Floor

Challenge (non-blocking): the fail-before-side-effects test's spies intercept via static assignment on the class (ConnectionService.openBridgeLogFile = …), which only intercepts because the tests invoke the prototype methods statically (this = the class). The harness's capture/restore discipline (spec :46-56) makes this correct today — but a future reader converting those calls to an instance (Neo.get(ConnectionService)) would silently inert both spies while the test keeps passing on the strength of the throw alone. One comment naming that the assertions' load-bearing half is the reject + call order would armor it. Offer, not demand — the throw-before-side-effects contract itself is proven either way.

Caller sweep (verified, not assumed): spawnBridge / getBridgeStdioLogPath have no production callers outside this file; ComponentService.spec and DockService.spec mention the path only in avoidance comments; the e2e MultiWriterNL.spec uses the default aiConfig.logPath (always set by the config leaf). No live consumer relied on the removed fallback — the fail-loud fires only on explicit blank injection or an unresolvable leaf, which is precisely the intended contract.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: "require injected bridge log path" matches the diff exactly (fallback deleted, not narrowed)
• Anchor & Echo summaries: the ticket's "distinct from #15984" framing matches the actual code boundary (leaf declaration/binding vs consumer fail-loud) — verified against my own trace, not taken on faith
• [RETROSPECTIVE] tag: n/a — author claimed none
• Linked anchors: #15876's fail-loud precedent and ADR-0019 §3 A1 both verified as the actual rules being applied

Findings: Pass.

🧠 Graph Ingestion Notes

• [KB_GAP]: none — the author demonstrably holds the ADR-0019 §3 A1 boundary (she applies it as a deletion, not a new rule).
• [TOOLING_GAP]: none.
• [RETROSPECTIVE]: The umbrella-to-leaf split pattern executed here is the model for the whole census class: when the full enumeration exceeds the evidence, close the proven consumer with an honest leaf and keep the umbrella open — rather than letting the umbrella's breadth water down what the evidence actually proves. #15931 → #16008 is the second clean instance today (after #15975 → my watchdog per-facet leaf).

N/A Audits — 📡 🔗 🛂

N/A across listed dimensions: no OpenAPI surface, no new workflow convention or cross-substrate primitive, no major abstraction (a fallback deletion + guard in an existing consumer).

🎯 Close-Target Audit

• Close-target identified: Resolves #16008 (newline-isolated). Commit subject carries (#16008) as suffix.
• #16008 is not epic-labeled; the umbrella #15931 is referenced non-closing, correctly left open.

Findings: Pass.

📑 Contract Completeness Audit

• Ticket carries a 3-row Contract Ledger (helper contract / spawnBridge contract / real-dir overlay boundary)
• Diff matches all three rows exactly — the fallback row's "no root derivation" and the boundary row's "canonical root remains absent" are both spec-witnessed

Findings: Pass — ledger present and honored to the row.

🪜 Evidence Audit

• Evidence: shape is correct for the change class: pure-function + filesystem-unit witnesses are the reachable ceiling for a path-resolution contract; no runtime residual (the Bridge spawn path is e2e-covered by MultiWriterNL under the default leaf)
• The ticket's Evidence Plan names the exact commands and the AiConfig template-SSOT lint; both are green at head

Findings: Pass.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head required CI all green at 58aff14062; author's 13/13 focused receipt matches the CI result
• Reviewer falsifier: caller sweep (above) — no live consumer relied on the fallback; and the spec-harness restore discipline verified at :46-56 (no cross-test stub pollution)
• Test location: test/playwright/unit/ai/services/neural-link/ConnectionService.spec.mjs — canonical co-located path

Findings: Pass.

📋 Required Actions

No required actions — eligible for human merge.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 95 - ADR-0019 §3 A1 applied as a deletion at the exact consumer; no new rule invented; matches the #15876 precedent's shape. 5 deducted because the spy-interception mechanism (static-call dependence) is more fragile than the contract it guards, though currently correct.
• [CONTENT_COMPLETENESS]: 95 - Fat ticket with a real ledger, falsifier-form premise check, avoided traps, and the explicit #15984 distinction; JSDoc updated in step with the removed parameters.
• [EXECUTION_QUALITY]: 92 - All green at exact head; three discriminating witnesses including the fallback-supplied throw case and the real-dir canonical-absence form; restore discipline correct. 8 deducted for the spy fragility noted above (non-blocking).
• [PRODUCTIVITY]: 97 - All four ACs met with witnesses; scope discipline exemplary — the umbrella stays open by design, not by accident.
• [IMPACT]: 78 - One live re-derivation channel closed in the cross-plane write-leak family; narrow surface, meaningful class.
• [COMPLEXITY]: 25 - Two files, one guard, one removed fallback; minimal reader load.
• [EFFORT_PROFILE]: Quick Win - High-discipline small fix with complete witnesses and zero scope leakage.

The umbrella-to-leaf discipline is the real artifact here — #15931 gets to keep its breadth because this leaf refused to pretend to it. Merge when ready.

— Phoebe 🔆