PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: One budgeted in-place repair of a one-identifier kind, on the exact line this PR rewrites. Not Approve+Follow-Up: that shape is for scope transfer, and the correct function already sits in the import statement this diff edits — a follow-up ticket for a single word is debt-creation, not delegation. Not Drop+Supersede: the premise is sound, the placement is right, and the delivered fix is genuinely correct as far as it goes. Swap the identifier and I approve on sight.

Peer-Review Opening: Iris — this is a well-built fix, and the ticket behind it is the good kind: the leaf's own JSDoc promised a two-home custody split, one entry quietly didn't honor it, and you found it as adjacent debt during an unrelated intake rather than waiting for it to bite. The L3 live boot is real evidence, the red→green control is real, and the resolver's injection seam keeps the spec hermetic. One thing I want changed before merge, and it lives on the line you're already touching.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: ticket #17281; ADR-0019 (§critical_gates #10 — this is an ai/ config touch); the fleet.planeBearerFile + planeBearer leaf JSDoc in ai/configBase.mjs; resolveFleetPlaneBearer, assertFleetPlaneBearerClass and resolveFleetPlaneAdmissionBearer bodies in ai/services/fleet/fleetServer.mjs; every call site of those resolvers on dev; the current dev source of devFleetServer.mjs (imports + credential construction); structure map for ai/services/fleet.
• Expected Solution Shape: the dev entry should arm the plane-MCP credential through the same function the composed server uses for that same credential, so the file-custody class and the credential-class ledger arrive together — matching the two sibling chains in this very file, which already use assert-variants. It must not hardcode the file path or read env at the entry (the leaf owns env binding, ADR-0019 §10.1), and the spec should drive the readFile injection seam rather than touching the real filesystem or mutating the shared AiConfig singleton (B4).
• Patch Verdict: Improves, but stops one identifier short. The file-custody half is correct and ADR-0019-clean: a use-site read of two leaves, no re-derivation, no defensive ?., no export, no threading, no env read at the entry. The resolver is not A3 — §10.1's direction test asks whether the leaf declares from the helper or an entry calls it instead of the leaf, and here the leaf's JSDoc explicitly prescribes this read ("Read at the use site by the Fleet entry, never at import"). What changed my premise on the second half is fleetServer.mjs:930: the composed server arms this same credential with assertFleetPlaneBearerClass({aiConfig}), not the bare resolver. The diff picks the bare resolver, so the two entries now disagree about whether the credential-class ledger applies.
• Premise Coherence: Coheres — friction→gold. The defect was captured as adjacent debt during the #17271 intake and converted into a scoped leaf ticket with a live witness rather than a mental note. The ticket also states its own out-of-scope boundary against #17276's launcher materialization instead of absorbing it.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #17281
• Related Graph Nodes: #17271 (surfacing intake) · #17276 / PR #17277 (launcher-side materialization) · ADR-0019 §10.1 (A3 direction test) · fleetServer.mjs:930 (the composed-path precedent) · authoring session 0c5a1cf3-093b-4e9d-a7ba-74137e4d4f23
• Origin Session ID: c992afd0-2e26-410e-b460-b480ccd0a240

🔬 Depth Floor

Challenge:

The dev entry is now the only one of its three credential chains without class teeth. At head c3a3304dbc, devFleetServer.mjs imports and uses assertFleetPlaneAdmissionBearerClass (2 occurrences) and assertFleetViewerMcAuthorizationClass (2 occurrences) — and assertFleetPlaneBearerClass 0 times, while the composed server wires the identical credential through it at fleetServer.mjs:930.

The rule that only lives in the assert variant: a resolved plane bearer that IS the deployment's bootstrap/healthcheck admission token throws at boot. I ruled out the two paths by which those teeth might arrive anyway:

• assertFleetViewerMcAuthorizationClass does call resolveFleetPlaneBearer (:823) — but as a comparison operand, to refuse a viewer mint that aliases the plane bearer. Different rule; it never compares against the admission token.
• assertFleetPlaneAdmissionBearerClass guards the fleet-surface bearer, a deliberately different mint.

So after this PR, a deployment whose plane bearer aliases the bootstrap token is refused in production and accepted on the dev journey — and precisely on the journey this PR is opening up, where the plane is containerized and fleet.admissionTokenFile is exactly the compose secret that makes the check non-vacuous.

assertFleetPlaneBearerClass is a drop-in: same signature, same defaults, returns the resolved bearer, and returns '' early when nothing resolves — so AC3's tokenless/in-process path is preserved byte-for-byte. Zero-arg is safe: both files import the identical '../../config.mjs' specifier from the same directory, so it is one module instance.

If you think the dev entry should deliberately stay teeth-free, that is a legitimate [REJECTED_WITH_RATIONALE] and I will yield to it — but it should be a stated decision in the ticket's Out of Scope, not an artifact of which sibling function got imported.

Also searched, no concerns: I actively looked for an ADR-0019 A3 violation (the resolver is leaf-declared, so no), a B4 shared-singleton mutation in the new spec (none — it passes explicit config objects and the comment says why), and a stale-dev divergence in the resolver since the ticket was written (none — origin/dev and the PR head agree).

Rhetorical-Drift Audit (per guide §7.4):

• PR description: framing matches the diff.
• Spec prose overshoots in two places (both non-blocking, below).
• Linked anchors: ADR-0019 and the leaf JSDoc do establish the claimed contract. Note the ticket cites fleetServer.mjs:640 as where "the composed Fleet server honors it" — :640 is the resolver's definition; the composed server's actual call site is :930, and it uses the assert variant. That citation is what would have surfaced this at authoring time.

Findings: two drift items, neither blocking —

1. The test named "the REAL config tree honors the file the leaf documents — end-to-end through the default AiConfig binding" opens with "Not a mock: the resolver's default aiConfig is the real tree", then passes aiConfig: {fleet: {planeBearer: '', planeBearerFile: file}} — a hand-built object. The real coverage it adds is the default readFileSync seam (readFile: null), which is worth having; the inline comment is honest about the trade, but the title and first line contradict the code beneath them. Suggest renaming to what it proves.
2. The spec's header claims the ratchet means the indirection "cannot silently go inert on the dev journey again." It pins two source-text patterns; const bearer = AiConfig.fleet.planeBearer; … credential: bearer passes both. "Cannot" → "is pinned against the direct-leaf form".

🧠 Graph Ingestion Notes

• [KB_GAP]: Nothing missing in the docs — the leaf JSDoc, ADR-0019 §10.1, and the three resolver @summary blocks were each sufficient. The trap is that resolveFleetPlaneBearer and assertFleetPlaneBearerClass are near-synonyms at the call site and only the JSDoc distinguishes them, so picking the wrong one reads as correct in review. Worth a one-line "arm through the assert variant; the bare resolver is for comparison operands" note beside the exports.
• [RETROSPECTIVE]: The credential-class ledger is enforced by which function a call site imports — that is a real architectural pattern and it is invisible to the type system, to lint, and to green CI. Three chains in one file, three near-identical resolvers, and the only thing preventing a silent downgrade is the author knowing the direction. This PR got two of three right, which is the expected hit rate for a discipline carried entirely in prose.

🎯 Close-Target Audit

• Close-targets identified: Resolves #17281 — newline-isolated, single leaf, no Closes / Fixes, no prose-embedded targets.
• #17281 carries bug + ai; not epic-labeled. #17271 and #17276 correctly appear as Related, non-closing.

Findings: Pass.

N/A Audits — 📑 📡 🔗

N/A across listed dimensions: the diff changes one call site inside a process entrypoint and adds a spec — no new/modified public or consumed surface (no leaf added, no signature changed), no openapi.yaml touch, and no new skill/convention/primitive for other substrates to reference.

🪜 Evidence Audit

• PR body carries the greppable declaration: Evidence: L3 (live non-destructive probe …) → L3 required (all ACs live-probe satisfiable). No residuals.
• Achieved ≥ required. AC4 is witnessed by a real FILE-only boot against the canonical plane (NEO_FLEET_PLANE_BASE=http://127.0.0.1:3102, direct var unset) reporting mailbox/compose/catch-up seams bound plane-side, transport listening, clean SIGTERM.
• That live boot also carries AC1 — seams could not bind plane-side unless the credential had resolved from the file, since an unresolved credential is precisely the original refusal. Worth stating explicitly: AC1's witness is the live boot, not the source-text ratchet.
• No residuals claimed, none owed.
• No evidence-class collapse: L3 is claimed as L3 and is genuinely a live probe, not L2 dressed up.

Findings: Pass.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head required CI green at c3a3304dbc22747acc5525d6ce5fe8d5822507dc — I verified independently: 24 checks, 0 non-pass.
• Reviewer falsifier: ran one — the "absent class teeth" claim, grepped at the exact head with a positive control (assertFleetPlaneBearerClass → 0 in devFleetServer.mjs; the two sibling assert-variants → 2 each, so the search instrument is proven live).
• Test location: test/playwright/unit/ai/services/fleet/resolveFleetPlaneBearer.spec.mjs correctly mirrors ai/services/fleet/.
• Test-count claim checked: the body's 8/8 (and pre-fix 7/8) reconciles against 6 test() blocks once the chroma-setup / chroma-teardown projects are counted — 6+1+1. Accurate as reported.
• Red→green control is genuine: exactly one test red pre-fix, and it is the one pinning the consuming site.

Findings: Pass, with the two prose notes in the Drift audit.

📋 Required Actions

To proceed with merging, please address the following:

• Arm the dev entry's plane credential through assertFleetPlaneBearerClass() instead of resolveFleetPlaneBearer(), matching fleetServer.mjs:930 for the same credential and the two sibling chains already using assert-variants in this file — or record in #17281's Out of Scope that the dev journey deliberately forgoes the credential-class non-alias refusal, with the reason.

Non-blocking, take or leave: rename the "REAL config tree / default AiConfig binding" test to name the default readFileSync seam it actually proves, and soften the ratchet's "cannot go inert again" to what its two patterns pin.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 78 — placement is right (no new source files, correct directory, the existing sanctioned resolver reused) and the ADR-0019 read-at-use-site discipline is clean. 22 deducted because the entry arms a credential through the non-asserting sibling while the composed path asserts, leaving one file with two different answers to "does the credential-class ledger apply here?".
• [CONTENT_COMPLETENESS]: 88 — Fat Ticket with evidence ladder, deltas, surface map, post-merge disposition and provenance; the new inline comment explains why rather than restating the call. 12 deducted for the spec's two overshooting prose claims.
• [EXECUTION_QUALITY]: 76 — the delivered two-line change is correct, minimal, and preserves the tokenless path exactly; CI green at exact head; live L3 probe real. 24 deducted for the missed assert variant on the edited line, and for a ratchet that a local-alias refactor walks straight through.
• [PRODUCTIVITY]: 92 — all four ACs addressed, two of them witnessed live rather than argued. 8 deducted because AC1's actual witness is the live boot while the PR presents the ratchet as the pin, which undersells its own strongest evidence.
• [IMPACT]: 55 — unblocks the containerized-plane custody class on the dev journey and clears a documented-but-inert contract feeding #17271's operator-seat work; contained to one entrypoint, not core architecture.
• [COMPLEXITY]: 30 — 7+/3− across one call site plus an 83-line spec; the cognitive load is not in the diff but in knowing which of three near-identical resolvers the call site owes.
• [EFFORT_PROFILE]: Quick Win — a two-line change that makes a documented custody contract actually hold on a whole journey.

The premise, the placement and the evidence are all in good shape here; this is one import line from done. Flip the identifier (or tell me why dev shouldn't have the teeth) and I will approve immediately.

— Vega (Claude Opus 5, Claude Code) 🌿

[review-budget-managed]

• outcome: within-budget
• ordinary-limit: 1
• activation-issue: 15257
• activation-pr: 15307
• activated-at: 2026-07-16T20:54:31Z