PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: Merge-safe head, green exact-head CI, no deferred correctness, and nothing to transfer. Not Request Changes because the one behavioural question I raised resolved in the strengthening direction under probing. Not Approve+Follow-Up because there is no scope to move — the single non-blocking observation is a null-set consequence on a provably empty registry and already has a Post-Merge census that would surface it.

Peer-Review Opening: This is the good version of a deletion PR: the receipt was corrected before the code was written, so the 143 lines came out on proven absence rather than on a claim the evidence could not support. Two of the changes are stronger than the body says — details below.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: Ticket #16189 in full — Context, Architectural Reality, the 4-row Contract Ledger, all 8 ACs, Out of Scope, Avoided Traps. The changed-file list (2 files). Current dev source of FleetRegistryService.mjs: the marked migration block and readRegistry(). Sibling precedent in ai/services/fleet/ via npm run ai:structure-map -- --files --loc (FleetControlBridge.mjs, FleetLifecycleService.mjs et al.). And my own independent runtime read of this installation's Fleet registry, made before this PR existed and separate from the author's census.
• Expected Solution Shape: Delete the read-path translator and its synthetic fixture; keep the define-time and Body↔Brain rejection guards executable without that fixture propping them up; state the receipt as proven-absence, never as proven-rewrite. The boundary this must not hardcode: an assumption that no legacy-shaped row can ever be read again — after the deletion the read path still has to fail closed rather than silently mis-hydrate. Test isolation: the vocabulary sweep must not lose coverage as a side effect of losing its fixture.
• Patch Verdict: Improves. Two pieces of evidence moved my premise, both in the strengthening direction — the surviving fail-closed boundary (four live call sites) and the widened vocabulary-guard denominator. Both are recorded under Depth Floor rather than asserted here.
• Premise Coherence: Coheres with verify-before-assert, and at the layer that is usually skipped. The author applied it to her own ticket's acceptance criterion, not to a code claim: an empty registry satisfies "the rewrite succeeded" and "no row was ever traversed" identically, so the original receipt could not discriminate between them. Amending the authority before editing is the discipline working upstream of the diff.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #16189
• Related Graph Nodes: #16184 (established the deletion boundary), #16190 (the hard cut this completes), #15188 / #15191 (canonical stdio / streamable-http vocabulary), #16167 (the cutover lane this clears surface for)
• Origin Session ID: 0a7f5f1d-cf12-4698-984c-17b64eea5178

🔬 Depth Floor

Challenge OR documented search (per guide §7.1):

• Challenge: My §0 concern was an asymmetry: the guards the ticket promises to preserve sit on the input path (defineAgent, the Body↔Brain wire), while the deleted code sat on the read path. If a legacy-shaped row ever arrived from disk, would it now silently mis-hydrate?

  I probed it, and the answer inverts the concern. normalizeStoredMcpTarget() survives at the PR head with four live call sites — :480 (configure), :599 getDefinition() (the spawn path), :683 findMcpTenantAssignee(), :706 toPublic() (the get/list/wire projection) — and its contract is explicit: "Absence and invalid stored shapes both fail closed to the resident target." The fail-closed guarantee lives at the projection boundary, not the hydration boundary, and this PR does not touch it. A hypothetical mcpTransport-only row read raw from disk resolves mcpTarget: undefined → normalizeMcpTarget(null) → resident.

  Now compare against what was deleted. migrateLegacyMcpTransport() already failed malformed rows closed to resident. Its only net capability was promoting a well-formed remote-http row to {kind: 'tenant', tenantId}. So the deletion removes the one path by which a persisted row could acquire tenant authority without passing the define-time guards. The ticket's Avoided Traps frames the risk as "weakening wire rejection"; mechanically the effect is the opposite — strictly more fail-closed. The body undersells this and it is worth having in the graph.

  Second undersell, in the test surface. Before, the retired-vocabulary sweep ran every source file through stripVocabularyExceptions(), carving two marker-delimited regions out before expect(source).not.toMatch(retiredPattern) — and the markers were themselves assembled via ['// Legacy MCP target ', 'migration begin'].join('') precisely so the marker text would not trip its own scan. After, it is source = fs.readFileSync(filePath, 'utf8') with no exception mechanism at all. The guard's denominator went from all files minus two carve-outs to all files. "Vocabulary exceptions are deleted" reads as cleanup; what actually happened is the guard got stricter.

  One thing to watch, explicitly non-blocking and I am asking for nothing. An mcpTransport-bearing row is no longer stripped on read. If such a row existed and any later write persisted the hydrated definition, the dead key would survive in registry.json indefinitely — where the old migration removed it and rewrote once. On a provably empty registry this is the null set, and the ticket's ledger already routes any newly discovered row back through the premise. If the Post-Merge census ever finds one, this is the second thing to check after target resolution.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: framing matches what the diff substantiates — and where it diverges it undershoots, which is not drift. "Bounded runtime census" is stated as bounded, never as universal; the ticket declines hypothetical external installations in Out of Scope rather than claiming they cannot exist.
• Anchor & Echo summaries: no new @summary added; the two deleted ones left no orphaned references.
• [RETROSPECTIVE]-class prose: the "Evolution" section characterizes the ticket correction accurately and does not inflate a deletion into an architectural shift.
• Linked anchors: #16184 and #16190 do establish the deletion boundary and the hard cut this completes; #15188 / #15191 do own the canonical vocabulary. No borrowed authority.

Findings: Pass. Two conservative-direction underclaims noted above; neither is a required action.

🧠 Graph Ingestion Notes

• [KB_GAP]: None. The retired-vs-canonical target vocabulary is correctly sourced to #15188 / #15191 throughout.
• [TOOLING_GAP]: None new from this PR.
• [RETROSPECTIVE]: The durable lesson is upstream of the code. A receipt that is satisfied identically by two incompatible world-states cannot serve as an acceptance criterion — an empty registry proves "the rewrite succeeded" exactly as well as it proves "nothing was ever there to rewrite." The author caught this in her own AC and amended the authority before implementing. This is the same failure class as an absence-claim without a positive control, relocated from a grep to an acceptance criterion, and it is the more expensive location because a bad AC gets built rather than merely asserted.

N/A Audits — 📡 🔗

N/A across listed dimensions: deletion-only change inside one owning service plus its canonical spec — no openapi.yaml surface, no skill file, convention, MCP tool, or architectural primitive introduced.

🎯 Close-Target Audit

• Close-targets identified: #16189 — newline-isolated Resolves #16189 in the PR body; commit subject fix(fleet): remove unexercised target migration (#16189) carries the ticket-ID suffix. No Closes / Fixes anywhere in body or commit. Four Related: refs (#16184, #16190, #15188, #15191) are correctly non-closing.
• For each #N: #16189 labels are enhancement, ai, refactoring, testing, architecture — not epic-labeled. It is a delivered leaf.

Findings: Pass.

📑 Contract Completeness Audit

• Originating ticket contains a Contract Ledger matrix — 4 rows.
• Implemented PR diff matches the Contract Ledger. Verified row by row:
  • Runtime Fleet registry set — the negative receipt is recorded in the ticket Context and restated in the PR body in the discriminating form ("never traversed or rewrote a row").
  • readRegistry() — shipped as return new Map(Object.entries(data.agents || {})); no translator, no rewrite branch. The row's "existing missing/corrupt-file behavior remains unchanged" holds: the early return new Map() guard sits above the hunk and is untouched.
  • Public define/configure boundary — the retired-input rejection assertions survive the fixture deletion (getAgent('legacy-wire') → null is retained directly above the removed block).
  • Retired-vocabulary guard — no production migration exception remains. The PR removed the test-fixture exception too, so the exception mechanism is gone rather than its two instances. That is over-delivery against the row, not drift: no consumed surface changed shape.

Findings: Pass.

🪜 Evidence Audit

• PR body contains an Evidence: declaration line — Evidence: L2 (bounded runtime absence receipt plus direct-registry and real-wire executable assertions) → L2 required.
• Achieved ≥ required. The ACs are discharged by the focused unit substrate plus one runtime observation; nothing here needs a deployed surface.
• Residuals: the one genuinely open item is correctly a ## Post-Merge Validation checkbox — re-run the bounded census and confirm no pre-cut row appeared between the deletion receipt and merge. That is the right classification: it cannot be proven from this head because it is a statement about a future window.
• Two-ceiling distinction: the census is declared bounded ("across the known Neo clones and local harness-instance roots"), so the L2 ceiling is stated as a scope limit rather than dressed as universality.
• Deployment causality: N/A — no external runtime receipt used as a merge gate.

Findings: Pass.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head required CI green at 5eedea340f — 14 checks, all in the pass bucket, zero pending or failing, read via gh pr checks --json name,state,bucket rather than positional field extraction. Worth stating explicitly because I got this wrong on a recent PR of my own: .github/workflows/test.yml builds its suites from a matrix whose include: lists integration-unified, integration-parity, unit, components — and unit does cover test/playwright/unit/**, so CI is a genuine witness for this spec, unlike the render suites that have no CI entry. Author receipt present and current-head-appropriate: 60 passed on Registry + Control Bridge, JSDoc 1,917 files / 0 violations, git diff --check clean.
• Reviewer falsifier: run, with a positive control, because a bare zero-match proves nothing. git grep -n 'mcpTransport' <head> -- ai/ apps/ learn/ src/ test/ → zero matches; the same query at origin/dev → 8 matches (4 in FleetRegistryService.mjs, 4 in its spec). The control establishes that the query works against these trees, so the zero is a real absence rather than a broken pattern or a wrong ref. Named concern: a residual token surviving in a file the sweep's deleted carve-outs used to hide. Result: none.
• Test location: deletion-only inside the existing canonical spec at test/playwright/unit/ai/services/fleet/FleetRegistryService.spec.mjs; no files added or moved.

Findings: Pass.

📋 Required Actions

No required actions — eligible for human merge.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 100 — the deletion lands in the owning service, confirmed against its siblings via ai:structure-map; the fail-closed guarantee stays at the projection boundary where it already lived, so no responsibility migrated. 100 names what was actively checked and cleared: AC-8 forbids a generic schema-version framework, dual-read alias, migration script, or compatibility timer, and none appears — the replacement is one raw Object.entries hydration, which is the simplest shape that can be correct here.
• [CONTENT_COMPLETENESS]: 100 — net −143 introduces no surface needing JSDoc, and the two deleted @summary blocks left no orphaned references (verified in the full guard scope, not just ai/). The body states the negative receipt in its discriminating form, declares Evidence: in greppable shape, and files the only unprovable-from-this-head item as Post-Merge Validation.
• [EXECUTION_QUALITY]: 100 — CI green at the exact head; the one behavioural question was resolved by tracing four live call sites rather than by reasoning about them; residual absence carries a positive control. 100 names the cleared failure mode: a read-path row silently mis-hydrating after the translator's removal, which the surviving normalizeStoredMcpTarget() boundary prevents.
• [PRODUCTIVITY]: 100 — all eight ACs met, including the negative-receipt AC the author rewrote herself before implementing.
• [IMPACT]: 45 — removes 143 lines of compatibility substrate and closes one latent tenant-authority elevation path, plus widens a source guard's denominator. Real, but scoped to one installation with no consumer-visible behaviour change on any row that actually exists.
• [COMPLEXITY]: 25 — two files, deletion-only, one hunk each in production and spec. The only non-trivial reader cost is the four-call-site trace needed to confirm the fail-closed boundary survived; low load otherwise.
• [EFFORT_PROFILE]: Quick Win — permanent compatibility debt avoided and a guard made stricter, at two-file deletion complexity.

The part I would keep is the sequencing: the falsifier ran against the acceptance criterion, before any code existed. That is the cheapest place in the lifecycle to catch a receipt that cannot discriminate, and it is the place we most often skip.

🌿