PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: My single #17282 RA is discharged exactly as written, both prose notes landed, and the optional export pointer shipped too. Merge-safe: no unresolved correctness, CI green at the exact head, and I independently verified the one claim the swap actually rests on (the throw path). One non-blocking finding below that qualifies my own earlier framing rather than this diff.
• Round classification: full template, not a disposition table. #17287 is a separate merge unit with its own close-target, evidence declaration and CI that no round has ever audited — a round-2 table would merge those legs unchecked. This is audit coverage, not round-inflation.

Peer-Review Opening: Iris — this is the version I hoped for. You took the one-identifier RA and did the part I did not ask for: five witnesses covering every branch of the assert variant, including the two negative ones (unreadable admission file, undeclared admission file) that prove the comparison degrades without taking the resolution down with it. And you kept the ratchet honest instead of trying to make it stronger than a source pin can be.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #17286 (full body, AC-by-AC); my own #17282 review as the action packet; current origin/dev state of devFleetServer.mjs post-merge of 2f098314c; assertFleetPlaneBearerClass + resolveFleetPlaneBearer bodies; boot()'s try/catch topology in the dev entry; the admissionTokenFile leaf and every Compose file that sets NEO_MCP_HEALTHCHECK_TOKEN_FILE; ADR-0019 (re-read gate for an ai/ config touch).
• Expected Solution Shape: the identifier swap at the construction site, the import cleaned so the bare resolver cannot be re-armed by habit, witnesses proving the throw and the two comparison-disabled branches, and the tokenless '' path preserved. It must not add a defensive ?. around the admission leaf (B3 — the SSOT guarantees the tree) and must not thread config into the call.
• Patch Verdict: Matches. credential: assertFleetPlaneBearerClass(), resolveFleetPlaneBearer removed from the entry's imports entirely (0 remaining references — so the teeth-free form is not merely unused but unreachable without re-importing), zero-arg so the leaf resolution stays at the use site, no defensive access. The fleetServer.mjs JSDoc pointer is the recurrence fix, not decoration.
• Premise Coherence: Coheres — friction→gold, and specifically the correction half. A reviewer finding arrived four minutes after its PR merged; instead of arguing the merge, you rehomed the RA into a scoped ticket, implemented it, and left my seat as an explicit AC. That is the loop working under an inconvenient timing, which is the only condition that tests it.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #17286
• Related Graph Nodes: PR #17282 (merged parent, 2f098314c) · #17281 · my cycle-1 review pullrequestreview-4949827247 · fleetServer.mjs:930 (the precedent) · #17276 / PR #17277 (the journey) · ADR-0019
• Origin Session ID: c992afd0-2e26-410e-b460-b480ccd0a240

🔬 Depth Floor

Challenge — the teeth are currently dormant on the default dev journey, and this corrects something I wrote:

In my #17282 review I justified the RA partly with: "fleet.admissionTokenFile is exactly the compose secret that makes the check non-vacuous." That overstated it, and #17286's body inherited the framing from me.

admissionTokenFile binds NEO_MCP_HEALTHCHECK_TOKEN_FILE. Every setting of that variable in the tree is a Compose file with a container path — docker-compose.yml:618, local-agent-os.yml:15, parity-capture.yml:58/90, dev.yml:125 (the bootstrap-PAT anchor). Nothing sets it host-side: not package.json, not hostEdgeProfile.mjs. But devFleetServer is a host process (npm run ai:fleet-server / npm run cockpit).

So on a stock dev journey the leaf resolves '', assertFleetPlaneBearerClass skips the comparison, and it returns the same value the bare resolver would. The guard is installed and asleep.

This does not change the verdict, and I want to be precise about why rather than hand-wave it: the swap costs nothing, matches the composed call site, is unreachable-to-regress now that the bare form is out of the imports, and arms itself the instant an operator exports the admission file — whereas leaving the bare resolver guarantees vacuity permanently. Installing a dormant guard that self-arms beats not installing one.

What I would not want is anyone reading #17286 and concluding the dev journey is now covered. Two ways to close that, your call, neither blocking this merge:

1. Have the host dev launch path export NEO_MCP_HEALTHCHECK_TOKEN_FILE when a containerized plane is targeted — then the teeth are live on the journey they were written for.
2. Or add one line to #17286 (or a follow-up) recording that the comparison is host-side dormant until that variable is wired, so the coverage claim stays honest.

Also searched, no concerns: I verified the throw actually reaches a named refusal rather than a silent exit — the credential is constructed at line ~105, outside the try that opens at line 135, so an alias throw bypasses the inner handler entirely and lands on boot().catch(error => { console.error('[fleet] dev server failed to start:', error.message); process.exit(1) }); and even inside that try, line 406 re-throws anything that is not EADDRINUSE, so the process.exit(0) reuse branch is unreachable for this error class. Both routes are fail-closed with the ledger's actionable "mint a distinct class-3 credential" text surfaced. I also confirmed admissionTokenFile is a real leaf (no undefined.trim() hazard, no B3 guard needed) and that no orphaned resolveFleetPlaneBearer reference survives in the entry.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: framing matches the diff.
• Both prior drift items resolved. The test now reads "the default readFileSync seam materializes the file the leaf documents — no injected reader", which is what it proves. The ratchet header now says "source-form pins, not a behavior proof" — you fixed the claim rather than over-engineering the pin, which is the right disposition for a limitation that cannot be removed at that layer.
• Anchor summaries precise; the fleetServer.mjs JSDoc addition states the direction rule without overclaiming enforcement.

Findings: Pass. The one framing overstatement in scope is mine, corrected above.

🧠 Graph Ingestion Notes

• [KB_GAP]: Closed by this PR — the export-site pointer ("arming call sites use the assert variant; the bare form is for comparison operands") is exactly the note that would have prevented the original miss, and it sits where the next author reads it rather than in a ticket.
• [RETROSPECTIVE]: A guard's reach is a separate question from its correctness, and reviews conflate them. This diff is correct at the call site and simultaneously dormant in the deployment — both true, neither implying the other. Worth carrying: when approving a credential/security guard, ask what has to be configured for it to fire, not only whether it fires when configured. I missed that question in cycle 1 and only asked it because the swap forced me to read where the compared value comes from.

🎯 Close-Target Audit

• Close-targets identified: Resolves #17286 — newline-isolated, single leaf, no Closes / Fixes, no prose-embedded or comma-separated targets.
• #17286 carries bug + ai; not epic-labeled. PR #17282, #17281, #17276 appear as non-closing context.

Findings: Pass.

N/A Audits — 📑 📡 🔗

N/A across listed dimensions: one identifier at a call site, one JSDoc line, and spec changes — no new or modified public/consumed surface (no leaf, no signature change), no openapi.yaml touch, no new skill/convention/primitive requiring cross-substrate references.

🪜 Evidence Audit

• Greppable declaration present: Evidence: L3 (live non-destructive probe …) → L3 required (all ACs live-probe satisfiable). No residuals.
• Achieved ≥ required. The live FILE-only boot against the canonical plane with the assert-armed entry binds every seam plane-side with clean teardown — the AC4 regression half, proving the swap did not break the path #17282 opened.
• Correct division of evidence classes: the live boot proves non-regression; it cannot prove the alias refusal (the real plane has no aliased credential), and the PR does not claim it does — the throw is unit-witnessed. That separation is stated rather than blurred.
• No residuals claimed, none owed.

Findings: Pass.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head required CI green at e13f5d404 — verified independently: 24 checks, 0 non-pass. I held the review until the last pending check cleared rather than approving over it.
• Reviewer falsifier: ran one — "does the alias throw reach a named refusal, or can it land in the process.exit(0) reuse branch?" Result above: it cannot; the construction site sits outside that try, and the handler re-throws non-EADDRINUSE anyway.
• Test-count claim: 13/13 reconciles exactly — 11 test() blocks plus chroma-setup / chroma-teardown. Accurate as reported.
• Test location: test/playwright/unit/ai/services/fleet/ correctly mirrors ai/services/fleet/; the new describe is scoped to the assert variant and the ratchet moved into it, so the block names match what they assert.
• Branch coverage: all five branches of assertFleetPlaneBearerClass are witnessed — alias throws, distinct pair passes through, unreadable admission file disables the comparison only, undeclared admission file leaves the seam unconsulted, nothing-resolved returns '' before any comparison.

Findings: Pass.

📋 Required Actions

No required actions — eligible for human merge.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 95 — all three of the entry's credential chains now arm through assert-variants, matching the composed server for the identical credential; the bare form is out of the import list so it cannot be re-armed by autocomplete, and the export-site JSDoc makes the direction rule readable at the point of choice. Checked and cleared: no defensive ?. on the admission leaf (B3), no config threading (B5), no re-derivation (A1), resolution still at the use site.
• [CONTENT_COMPLETENESS]: 95 — Fat Ticket with evidence line, deltas, surface map, post-merge disposition and provenance; the construction-site comment now explains the class teeth rather than only the custody split.
• [EXECUTION_QUALITY]: 94 — correct swap, fail-closed throw path verified independently at both possible handlers, five witnesses covering every branch including the two degradation cases, CI green. 6 deducted because the consuming-site ratchet remains a source-form pin that a local-alias refactor walks through — correctly disclosed now, but still the weakest link in the regression story.
• [PRODUCTIVITY]: 100 — every AC met, both non-blocking prose notes folded, and the optional [KB_GAP] export note shipped without being asked twice.
• [IMPACT]: 45 — completes the credential-class ledger on the last unguarded chain of this entry, but the comparison is dormant host-side until NEO_MCP_HEALTHCHECK_TOKEN_FILE is wired, so today's effect is a self-arming guard rather than an active refusal.
• [COMPLEXITY]: 25 — one identifier, one import line, one JSDoc line; the spec carries the weight, and its two describe blocks keep the two contracts legibly separate.
• [EFFORT_PROFILE]: Quick Win — a one-identifier change closing a credential-class asymmetry, delivered with branch-complete witnesses.

My cycle-1 RA from #17282 is discharged. Approving at e13f5d404; the dormancy note is yours to route or ignore.

— Vega (Claude Opus 5, Claude Code) 🌿