PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: Delivered scope is correct, the security-critical property is implemented and proven, and all ten ACs on #16672 have coverage I verified individually rather than taking from the body. No defect justifies Request Changes; nothing is deferred, so Approve+Follow-Up would be the wrong shape.

Peer-Review Opening: Cross-family seat, taken unrequested because this PR gates the containers update and sat unseated. The auth-injection path is the part that mattered and it is done carefully — cloned twice over, with a test that proves the secret never reaches shared config by string-searching for it. Two non-blocking observations below.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #16672 (Fix steps + 10 ACs + Contract Ledger), the 4-file changed list, origin/dev source of the deleted McpServersIsolation.spec.mjs, the live compose project (neo-local-agent-os, ingress container present), and ai/deploy/ composition. Not the PR body as primary premise.
• Expected Solution Shape: A nullable bearerTokenEnvVar on the client, resolved instance-env-first then process.env, with request options and headers cloned before injection so a credential never lands in the shared ClientConfig singleton; fail-closed on an empty slot; reject a row declaring both a literal Authorization header and the env slot. Must NOT hardcode: the token value, or any cross-instance header state. Test isolation required: resolution must be exercisable without a live ingress or the real credential.
• Patch Verdict: Matches. createRemoteTransportOptions() spreads transportOptions, spreads requestInit, and constructs a new Headers — the shared object is never written. What settled it was not the diff but the spec: expect(JSON.stringify(ClientConfig.mcpServers)).not.toContain('transient-secret') asserts absence of the secret anywhere in shared config, which is stronger than an equality check and is the assertion I would have asked for had it been missing.
• Premise Coherence: Coheres with verify-before-assert — it replaces an assumed-safe shared-config path with one whose safety is asserted by a falsifiable string search rather than by reasoning about object identity.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #16672
• Related Graph Nodes: Related: #16167 (the hard-cut lane) · #16671 / PR #16674 (sibling deploy-composition fix; both gate the containers update)
• Origin Session ID: 9b08b9e4-6181-416b-ac68-e9d16636cff0

🔬 Depth Floor

Challenge:

The deleted spec's property did not become obsolete — it inverted, and nothing records the trade.

McpServersIsolation.spec.mjs (−94) asserted exactly two things on origin/dev:

- Broken server (knowledge-base) should still boot in degraded mode
- Healthy server (memory-core) should boot completely unaffected by sibling failure

That is sibling-failure isolation. AC-8 removes it as "obsolete", and for its literal subject that is correct — the client no longer spawns MC and KB as independent stdio processes, so the modelled failure cannot occur.

But both rows now point at one host:port:

knowledge-base  http://127.0.0.1:3102/kb/mcp
memory-core     http://127.0.0.1:3102/mc/mcp

The old design gave MC and KB independent fate by construction; the new one gives them shared fate through a single ingress. The property the deleted spec protected is not obsolete — its polarity flipped, and the PR retires the guard without recording that a coupling was accepted in exchange.

Non-blocking, and I am not asking for a test. A shared ingress is the intended canonical topology, it has its own healthcheck, and inventing an isolation assertion against it would be theatre. What is missing is one sentence of provenance: that independent-spawn isolation was deliberately traded for canonical routing. Without it, the next reader finds a deleted isolation spec and no record of what replaced the property.

I raise it because this week has produced repeated instances of a guard being retired on the strength of its name rather than its property — and this is the well-executed version of that decision, which is exactly when the rationale is cheapest to write and most likely to be skipped.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: framing matches the diff; the Evidence: L3 declaration is scoped to routed receipts and does not promote unit coverage to deployment proof
• Anchor & Echo summaries: bearerTokenEnvVar's member doc states "never written into the shared ClientConfig singleton" — a mechanical claim, and the spec substantiates it
• [RETROSPECTIVE] tag: N/A — none added
• Linked anchors: #16672's Contract Ledger exists and the diff matches it

Findings: Pass. One near-miss worth naming: the method JSDoc says "one client connection cannot leak its resolved secret into the shared config or another client instance." I treated the second clause as a claim rather than prose and checked it — a fresh Headers per call means no instance shares header state. It holds.

🧠 Graph Ingestion Notes

• [KB_GAP]: none.
• [TOOLING_GAP]: none observed on this PR.
• [RETROSPECTIVE]: The non-mutation test is the reusable pattern. Asserting not.toContain('<the actual secret>') over the serialized shared config proves absence of the value anywhere in the structure, which survives refactors an object-equality assertion would not — equality passes if the secret is added to a key the assertion does not enumerate. Worth copying wherever a credential passes near shared state.

🎯 Close-Target Audit

• Close-targets identified: #16672 (newline-isolated Resolves); #16167 correctly demoted to Related:
• #16672 confirmed not epic-labeled — labels are enhancement, ai, testing, architecture

Findings: Pass. #16167 carries the epic-shaped hard-cut scope and is correctly non-closing.

📑 Contract Completeness Audit

• Originating ticket contains a Contract Ledger matrix
• Implemented diff matches it — bearerTokenEnvVar nullable, remote-only, cloned injection, fail-closed empty slot, ambiguity rejection, MC/KB rows on canonical ingress without command/args, Neural Link unchanged stdio

Findings: Pass — no drift.

🪜 Evidence Audit

• PR body contains an Evidence: declaration: L3 (authenticated routed list-tools and healthcheck receipts from neo-local-canonical) → L3 required
• Achieved ≥ required; AC-10's routed-probe requirement is the one AC unit tests cannot reach, and the L3 receipt covers it
• Evidence-class collapse check: the body does not promote unit coverage to deployment proof — the routed receipt is cited separately
• Deployment causality: receipts come from the canonical plane rather than this unmerged head's image, appropriate here because the change is client-side and the plane is the counterparty, not the artifact under test

Findings: Pass.

📡 MCP-Tool-Description Budget Audit

Findings: N/A — no ai/mcp/server/*/openapi.yaml in the changed-file set (verified: 0 matches). No tool descriptions added or modified.

🔗 Cross-Skill Integration Audit

§8.1 applies: this changes a consumed contract — two built-in client rows stop being spawnable and start requiring a credential.

• No skill file, startup convention, or MCP tool surface added
• Downstream consumers are not enumerated in the PR body. §8.2's last checkbox asks for this explicitly when a substrate contract changes.

Two real consumers construct clients from these rows and will now hit the new fail-closed path when NEO_MCP_REMOTE_TOKEN is absent:

ai/Agent.mjs:141-149                      builds clients per serverName from config
ai/scripts/maintenance/kbPushClient.mjs   imports Client + ClientConfig directly

Findings: Non-blocking gap. The behaviour is correct — failing closed is AC-4 — and the credential is now a managed env key, so a managed shell supplies it. What is unstated is that any consumer running outside that environment (CI job, container, a bare shell) now throws at transport construction where it previously spawned a local server. kbPushClient is the one I would watch: it is a maintenance entrypoint, and its failure mode moves from "server did not start" to "credential missing". Worth one line in the PR body rather than a code change.

🧪 Test-Evidence & Location Audit

Exact-head CI green (all checks conclusive, no failures). Placement correct — new coverage extends the existing McpClientTransportConfig.spec.mjs beside its subject rather than opening a parallel file.

Each AC verified individually rather than accepted from the body:

Correction to my own process, recorded because it nearly became a false Required Action: I first grepped the diff for toThrow with too narrow a context window, saw nothing, and was about to flag AC-5 as untested. Widening the window found both throw assertions. The gap was my instrument, not the PR.

Findings: Pass — no author evidence gap, no placement issue, no falsifier failed.

📋 Required Actions

No required actions — eligible for human merge.

Two optional, non-blocking notes: the downstream-consumer line under Cross-Skill Integration, and the absent Origin Session ID in the PR body. Neither is a merge gate; mentioned so they are cheap to fold if the body is edited for another reason.

📊 Evaluation Metrics

Verdict weights: 30% premise / right thing, 30% architecture + placement, 30% diff correctness, 10% AC/audit sanity.

• [ARCH_ALIGNMENT]: 95 — auth resolution sits on the client that owns the transport, the credential name is config data rather than code, and remote-only application leaves stdio untouched. 5 deducted for the un-recorded isolation trade, a provenance gap rather than a placement error.
• [CONTENT_COMPLETENESS]: 88 — member and method JSDoc state mechanical properties the tests substantiate, and the stale GH_TOKEN comment was replaced rather than left to rot. 12 deducted for the un-enumerated downstream consumers and the missing Origin Session ID.
• [EXECUTION_QUALITY]: 100 — checked the three failure modes this shape invites and cleared each: shared-config mutation (new Headers per call, proven by secret-absence search), whitespace-only tokens passing the empty check (.trim() on both arms), and the ambiguity check running before resolution so an empty slot cannot mask a double-authority config.
• [PRODUCTIVITY]: 100 — all ten ACs delivered, each verified independently.
• [IMPACT]: 85 — moves the two highest-traffic MCP clients onto authenticated canonical routing and unblocks the containers update; scoped to client wiring rather than server behaviour.
• [COMPLEXITY]: 45 — one new method and a config-row swap, but the credential path carries security weight disproportionate to its line count.
• [EFFORT_PROFILE]: Quick Win — small surface, high leverage, and it removes a spawn path rather than adding a code path.

Reviewed by Ada (@neo-opus-ada, Claude Opus 5) — cross-family seat.