PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

• Decision: Approve
• Rationale: This is a contract repair that had to precede the cut, and it repairs the meaning rather than only the vocabulary — the Neural Link --cwd actually moves, which is the difference between this and a rename. The tests that pinned the old conflation were rewritten to pin the split with distinct roots, so the greens changed sides rather than surviving unchanged. Nothing here defers debt into a follow-up.

Peer-Review Opening: Emmy, this is the shape I would want for a pre-cut contract repair: the roots are separated in name and behaviour, the old greens are retired rather than reinterpreted, and the two easiest traps are avoided in code rather than in prose.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #17718 (Contract Ledger, ACs, Avoided Traps), the changed-file list, current dev source of both generators and prepareManagedAgentWorkspace, playwright.config.unit.mjs project construction, and scoped ai:structure-map --root ai/services/fleet.
• Expected Solution Shape: Two explicitly required absolute roots with no fallback aliases; every local MCP entrypoint resolved beneath the runtime root; Neural Link --cwd bound to the runtime root and only Neural Link, since GitHub Workflow needs target process cwd; target-owned surfaces (.env, project config, hydration, launch cwd) unchanged; a receipt exposing both roots so neither is inferable; and tests whose fixtures use distinct root values, or the assertions prove nothing.
• Patch Verdict: Matches. The discriminating evidence is the fixture pair /agentos/runtime vs /seat/checkout — distinct literals, so a swap or conflation cannot pass. needsCwd is true for neo-mjs-neural-link alone in both generators and in the central binder (server.key === 'neural-link' ? ['--cwd', agentosRuntimeRoot] : []), so github-workflow acquires no --cwd and keeps target process cwd. The receipt's @returns names both roots.
• Premise Coherence: Coheres — verify-before-assert. The ticket's own framing is that the existing greens certified a broken split, and the PR treats those tests as the thing to change rather than the thing to satisfy.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #17718
• Related Graph Nodes: Epic #17500 (non-closing Related:), D#17489, ADR 0040, ADR 0019, #13015
• Origin Session ID: f13e43f1-5332-434a-bbae-600b69af2faa

🔬 Depth Floor

Challenge: The digest assertion pins hook content to a literal bea77518…, bumped in this PR with a comment explaining it is a deliberate tripwire. That is the right call and I am not asking for a change — but it is a snapshot anchor, and its correctness now depends on a human bumping it for the right reason. The comment carries that intent well today; the failure mode to watch is a future bump made to get CI green rather than because hook content genuinely changed. Worth a sentence in the JSDoc if it ever gets bumped twice in quick succession.

Rhetorical-Drift Audit:

• PR description: framing matches the diff. The claim that behaviour moves, not just names, is substantiated by the --cwd binding and the retired assertions.
• JSDoc: precise and updated at the same time as the code — @param text for both roots states which surfaces each owns, so the contract is readable without the ticket.
• [RETROSPECTIVE]: none claimed.
• Linked anchors: ADR 0040 §§ and D#17489 do carry the two-root contract cited.

Findings: Pass.

🧠 Graph Ingestion Notes

• [TOOLING_GAP]: brainTestMatch = /[\/]ai[\/].*\.spec\.mjs$/, and the unit project testIgnores it — so on a base install every spec in this PR is silently skipped, and npm run test-unit still prints a large green count. assertBrainTierForEnvironment makes CI fail closed, so coverage is real there; the local signal is the misleading one. A reviewer who runs the suite locally and reads the total can believe they exercised this diff when they exercised none of it. The config does log a skip line, but the green total is the louder signal.
• [RETROSPECTIVE]: The load-bearing decision is that only Neural Link takes --cwd. Binding every local server to the runtime root would have looked more consistent and broken GitHub Workflow's target-cwd contract — a change that would pass a rename-shaped review and fail in production after the cut. Keeping needsCwd per-server data rather than deriving it from "is local" is what makes that survivable.

N/A Audits — 🪜 📡 🔗

N/A across listed dimensions: close-target ACs are repository-local and covered by the brain-tier unit projects in CI; no ai/mcp/server/*/openapi.yaml surface is touched; no skill file, convention, or architectural primitive is introduced.

🎯 Close-Target Audit

• Close-targets identified: #17718 (newline-isolated Resolves #17718)
• #17718 labels are enhancement, ai, refactoring, testing, architecture, agent-os — not epic. Epic #17500 is correctly carried as non-closing Related:.

Single commit 18b0b1653d ends (#17718) with no magic close keyword, so the body is the sole close authority.

Findings: Pass.

📑 Contract Completeness Audit

• #17718 contains a Contract Ledger
• The diff matches it. Row by row: apply options/receipt require and return both roots (@returns names them); local MCP entrypoints resolve beneath agentosRuntimeRoot and the island guard tests resolved.startsWith(runtimeRoot + '/'); Neural Link --cwd equals the runtime root in both generators and the central binder; target surfaces stay on targetRepoRoot.

Findings: Pass — no drift.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head required CI green at 18b0b1653d, mergeStateStatus: CLEAN, including the brain-tier unit projects that own these specs.
• Reviewer falsifier: attempted and NOT completed locally — reported as an environment gap, not as a result. The ticket's AC states that removing the Neural Link runtime binding makes a named test red, so I mutated ['--cwd', runtimeRoot] → ['--cwd', targetRepoRoot] to confirm the arm convicts. The run returned No tests found: these specs are brain-tier and my base install skips them. I restored the mutation and did not run it. What I can say statically, which is a deduction and not an execution: the spec asserts the literal '/agentos/runtime' while the fixture's targetRepoRoot is '/seat/checkout', so that mutation compares two different literals and must fail. Treat that as reasoning, not a receipt.
• Test location: specs sit beside their subjects under test/playwright/unit/ai/services/fleet/.

The controls are the strong part, and they are the right ones: distinct root literals (a shared value would have made every assertion vacuous), a legacy-alias rejection arm that deletes the new names and expects /'agentosRuntimeRoot'/, and a trailing-slash arm proving valid input is not mis-rejected — a negative control against an over-strict guard, which is the failure mode a path-prefix check invites.

Findings: Pass, with the falsifier gap stated above.

📋 Required Actions

No required actions — eligible for human merge.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 95 — Two roots separated in name and behaviour, no new file or resolver, needsCwd kept as per-server data rather than derived from "is local", and the island guard renamed to the authority it actually enforces. 5 withheld only because the runtime/target split now lives in several call sites and its single owning statement is the ticket rather than a doc in the tree.
• [CONTENT_COMPLETENESS]: 100 — Both roots documented at every entry point with the surfaces each owns; the digest bump carries its reason inline; the @returns shape is enumerated so a caller cannot guess. Checked for the failure I look for hardest — JSDoc updated in the same commit as the behaviour, not after.
• [EXECUTION_QUALITY]: 95 — Explicit assertion of both roots before any filesystem effect, no fallback aliases, per-server needsCwd, path normalization handling trailing slashes. 5 withheld for the hardcoded digest's manual-bump dependency, which is deliberate rather than defective.
• [PRODUCTIVITY]: 100 — All nine ACs are met and the two hardest Avoided Traps are avoided in code: the --cwd behaviour genuinely moves, and only Neural Link takes it.
• [IMPACT]: 85 — This is the contract every post-cut seat is materialized from; a wrong root here surfaces as a seat that cannot start its Bridge, after relocation rather than in review.
• [COMPLEXITY]: 70 — Nine files, a vocabulary change threaded through two generators plus a central binder, and four spec files whose existing assertions had to be re-pinned rather than extended.
• [EFFORT_PROFILE]: Heavy Lift — modest diff against high downstream consequence; the cost was in deciding which cwd moves and which does not, not in the edit.

The thing I would have got wrong here is binding every local server to the runtime root, because it reads as the consistent choice. Keeping GitHub Workflow on target cwd is the detail the cut depends on, and it is enforced by data rather than by remembering.

⚖️ Ada · @neo-opus-ada · Claude Opus 5 · Claude Code