PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: The premise is correct, the enforcement runs in both directions, and I found no delivered-scope defect. My one challenge is a diagram omission in a teaching doc — non-blocking, no correctness impact, and not worth a return cycle. Request Changes needs a correctness/safety/code-shape defect and there is none; Approve+Follow-Up needs scope transfer with a day-after-merge counterfactual, and the only transferable item (#16189) is already filed and owned.

Peer-Review Opening: This is the rare vocabulary PR that proves it is not a rename. I came in with two falsifiable questions — does the axis split buy expressiveness the conflated string could not express, and did a dual-name compatibility layer survive — and both came back stronger than I asked. Details below.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: Ticket #16184 in full (Context / Problem / Architectural Reality / Fix / the three motivating combinations); #16189's Context + Problem + Fix (the deletion gate); the changed-file list; origin/dev source of FleetRegistryService.mjs and prepareManagedAgentWorkspace.mjs; #15188/#15191's canonical stdio / streamable-http naming as restated by #16184; and my own earlier findings on this surface, sent to the author before she claimed the lane.
• Expected Solution Shape: mcpTarget: {kind, tenantId?} for selection only; canonical stdio / streamable-http on the resolved plan; vendor spellings confined to adapter boundaries; and a one-shot, deletable persisted migration rather than a permanent dual-name layer. Must NOT hardcode: a vendor grammar in the domain layer, or remote-http surviving as a transport synonym. Test isolation expected: at least one fixture demonstrating a target/transport combination the single mcpTransport string structurally could not express — otherwise the split is unwitnessed and the PR is a rename with extra steps.
• Patch Verdict: Matches, and exceeds on both questions. Axis independence is witnessed in the strongest available form — prepareManagedAgentWorkspace.spec.mjs:798-802 resolves ONE mcpTarget and then asserts, within that single plan, memory-core and knowledge-base as {target: 'tenant', transport: 'streamable-http'} while neural-link is {target: 'resident', transport: 'stdio'}. Per-server divergence inside one seat selection is exactly what a single {mode} string could not carry. Enforcement also runs the other way: FleetRegistryService.spec.mjs:383 rejects transport, url, headers.Authorization, env, command, credential, a whitespace tenantId, and an unknown kind from the target grammar — and captures registry.json beforehand to prove the rejection performs no write.
• Premise Coherence: Coheres with the two-hemisphere model specifically: the correction is that Brain-side domain vocabulary stopped borrowing a wire-protocol word for a placement decision, and Body-side surfaces (src/ai/fleet/mcpServers.mjs, apps/agentos/**) follow rather than lead. apps/ drops to zero mcpTransport occurrences, so the Body carries none of the retired vocabulary.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #16184
• Related Graph Nodes: #16189 (the gated deletion of this PR's migration), #15188 / #15191 (canonical transport naming), #16167 / #16188 (the cutover this vocabulary serves), #16182 / #16186 (the Claude Desktop command bridge the diagram's third adapter names)
• Origin Session ID: 0a7f5f1d-cf12-4698-984c-17b64eea5178

🔬 Depth Floor

Challenge OR documented search (per guide §7.1):

• Challenge (non-blocking, documentation): the new Mermaid renders plan --> stdio["stdio"] as a terminal node, while streamable fans out to all three adapters. The section's own opening sentence is "Fleet keeps three layers explicit" and its prose says "Only the final harness adapter translates that plan into product grammar" — but a reader tracing the stdio path sees two layers and no adapter, which reads as "stdio bypasses adapter translation." It does not: prepareManagedAgentWorkspace.spec.mjs:244 branches on server.transport === 'stdio' inside adapter rendering, and the plan assertion above shows neural-link carrying a stdio row through the same projection. One edge from stdio to a shared adapter node would make the diagram teach what the prose already says. Cosmetic, in a doc whose purpose is precisely to make the layering legible.

• And the documented search, because my two substantive hypotheses both died: I actively looked for (a) a surviving dual-name compatibility path, (b) an unobservable deletion gate for the one-shot migration, and (c) a target/transport combination that the specs assert but the old vocabulary could equally have expressed. All three came back clean. On (a), mcpTransport drops 116 → 8 occurrences, and all four survivors sit in FleetRegistryService.mjs as migration-only reads — :164's summary calls the shape retired, :197 guards with Object.hasOwn, :202 destructures it out via {mcpTransport, ...rest}, :205 translates it. No acceptance path, and the persisted output cannot retain the key. On (b), #16189 names the receipt exactly — "the active registry has loaded under the new implementation and its persisted output contains only the canonical mcpTarget shape" — so the deletion is gated on an observable rather than on someone remembering. On (c), the per-server split above is genuinely inexpressible under one {mode} string.

• One guard I did not ask for and want on the record: expect(JSON.stringify(result.mcpPlan)).not.toContain(['remote', 'http'].join('-')). Building the token at runtime means the literal never appears in the spec source — so the assertion cannot be satisfied by its own text, and a source grep for the retired token does not false-positive on the guard that forbids it. That is the use-vs-mention trap avoided deliberately, and it is the kind of thing that usually has to be pointed out in review rather than found already done.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: framing matches the diff; the hard-cut claim is substantiated by the 116 → 8 reduction with apps/ and src/ at zero.
• Anchor & Echo summaries: precise. @summary Translate the single retired mcpTransport registry shape into target intent names the shape as retired and scopes itself to one translation, which is what the code does.
• [RETROSPECTIVE] tag: N/A — none claimed.
• Linked anchors: the doc's claim "Vendor spellings never enter the public registry, and target placement is never named after a wire protocol" is backed by an enforced guard, not prose — the target grammar rejects transport outright and fails closed without a write. Citation verified, not inferred.

Findings: Pass.

🧠 Graph Ingestion Notes

• [KB_GAP]: None. The reusable lesson is the inverse: one string answering two questions is discoverable by asking which combinations it cannot express. #16184 named three such combinations before any code moved, and this PR's decisive fixture is simply one of them made executable. That is a cheap, repeatable test for suspected conflation anywhere else in the vocabulary.
• [TOOLING_GAP]: None encountered.
• [RETROSPECTIVE]: The durable pattern is two-direction enforcement of an axis split. Splitting a conflated field is only half the work; without a guard that rejects the other axis's words from the narrower surface, the conflation grows back one convenience field at a time. Here the plan asserts both axes independently AND the registry refuses transport/secret/placement vocabulary without writing — so neither direction can silently re-merge. Worth reaching for whenever a domain field is being narrowed.

🎯 Close-Target Audit

• Close-targets identified: #16184 (newline-isolated Resolves #16184; no Closes / Fixes; no prose-embedded or comma-separated targets)
• For each #N: #16184 carries enhancement / ai / architecture / model-experience — not epic. It is a delivered leaf, and the cleanup limb it deliberately excludes is already carried by #16189 rather than left implicit.

Findings: Pass.

📑 Contract Completeness Audit

• Originating ticket (or parent epic) contains a Contract Ledger matrix — #16184 carries one, and #16189 carries its own for the deletion.
• Implemented PR diff matches the Contract Ledger exactly (no drift). The public shape is mcpTarget: {kind, tenantId?}; the resolved plan carries target + canonical transport; adapters translate only at their boundary (Codex streamable_http, Claude Code http, Claude Desktop local bridge — all three present in both the doc and the adapter fixtures); and the migration is marked as an explicit deletion boundary rather than a compatibility layer.

Findings: Pass.

N/A Audits — 🪜 📡

N/A across listed dimensions: the close-target ACs are config/vocabulary contracts fully covered by the exact-head unit suite, so no evidence-ladder residual applies; and no ai/mcp/server/*/openapi.yaml tool description is touched — the diff moves Fleet service vocabulary, not an MCP tool surface.

🔗 Cross-Skill Integration Audit

• Does any existing skill document a predecessor step that should now fire this new pattern? No — this narrows an existing domain field rather than adding a workflow step.
• Does AGENTS_STARTUP.md §9 Workflow skills list need updating? No.
• Does any reference file mention a predecessor pattern that should now also mention the new one? The one substantive consumer-facing surface is learn/agentos/cloud-deployment/ClientAuthentication.md, and this PR updates it in the same commit rather than leaving the doc describing the retired vocabulary.
• If a new MCP tool is added, is it documented? N/A — none added.
• If a new convention is introduced, is it documented? Yes — the target/transport/adapter three-layer rule is stated in prose and diagrammed, with the enforcement guard as its mechanical backing.

Findings: All checks pass — no integration gaps. One observation, explicitly not a gap: the persisted receipt file is still named .neo-fleet-mcp-transport.json while its contents are {adapter, artifact, projectionSha256, version} — a projection receipt rather than a transport record. It is pre-existing and untouched by this PR (identical occurrence counts on dev and at this head), and renaming a file written into instanceHome is its own migration, so deferring it is the right call. Recording it only so the last surface carrying the retired word is not lost.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head required CI green at 73625e89143d5dddc96fd383e87f21930d6c7b88 — 15 checks, all SUCCESS, zero non-pass (read from the status column, not a word position). Author non-CI receipt: N/A — the whole surface is unit-covered inside unit.
• Reviewer falsifier: N/A — no named behavioral concern survived the premise checks; my three hypotheses were answered from source and fixtures rather than needing a run.
• Test location: pass — every added spec mirrors its implementation path under test/playwright/unit/ai/services/fleet/, extending existing sibling specs rather than adding parallel files. Ratio is ~+250 implementation against ~+290 test, with FleetRegistryService.spec.mjs at +177/-43 on the service that owns the public grammar.

Findings: Pass.

📋 Required Actions

No required actions — eligible for human merge.

📊 Evaluation Metrics

Verdict weights: 30% premise / right thing, 30% architecture + placement, 30% diff correctness, 10% AC/audit sanity. These are importance-to-verdict weights, not effort budgets.

• [ARCH_ALIGNMENT]: 99 - The split is enforced in both directions, placement stays in the owning ai/services/fleet/ service with no new subsystem, and the Body follows rather than leads (apps/ and src/ carry zero retired vocabulary). 1 deducted for the persisted receipt filename still naming a transport — pre-existing, correctly deferred, but it is the last surface citing the retired word.
• [CONTENT_COMPLETENESS]: 97 - @summary present and precise on the migration helper; the consumer doc is updated in the same commit; lint-pr-body green on the body anchors. 3 deducted for the Mermaid's terminal stdio node, which under-teaches the three-layer model the section exists to explain.
• [EXECUTION_QUALITY]: 100 - Actively checked and cleared: no dual-name acceptance path (116 → 8, all survivors migration-only and stripped from persisted output); the deletion gate is an observable, not a memory; axis independence proven per-server within one plan; the target grammar fails closed on eight hostile shapes without writing; and remote-http is asserted absent from the resolved plan by a guard whose token cannot match its own source.
• [PRODUCTIVITY]: 100 - #16184's Fix is delivered end to end, and the limb it deliberately excludes was filed as #16189 with a named receipt rather than left as an implicit intention.
• [IMPACT]: 75 - Corrects the domain model that the Docker cutover and Fleet-managed provisioning both build on; a conflated placement/protocol field would have propagated into every adapter added later. Not user-visible, which caps it below the core-architecture band.
• [COMPLEXITY]: 55 - Twenty files across Brain services, Body config, and one doc, plus a persisted-shape migration and a three-adapter translation matrix; the reader load is breadth rather than depth, and each file's change is mechanical once the vocabulary is understood.
• [EFFORT_PROFILE]: Architectural Pillar - Separating two axes that a single public field had conflated is a domain-model correction, not a feature: it changes what the registry is permitted to express, and every future harness adapter inherits the narrower contract.

The thing I will carry from this review is the runtime-built forbidden token. I have spent a lot of today catching assertions that could not fail — including two of my own an hour ago — and that one line is the first time I have seen the mention-versus-use problem pre-empted in the guard itself rather than found in review.

Origin Session ID: 0a7f5f1d-cf12-4698-984c-17b64eea5178