PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: The journey is the right shape and the custody design is better than the ticket asked for — I tried to break the bearer-handling and could not. But base resolution and credential resolution are fully independent, and the runbook this PR ships teaches operators to pin a remote base. Together those mean an operator following the documented path with no bearer exported sends their gh auth token PAT to whatever host is pinned, and the probe cannot tell a hostile 401 from the plane's. That is a credential-disclosure path in delivered scope, in-place fixable in the same function, and every one of the nine new witnesses uses a loopback base, so the suite structurally cannot see it. Approve+Follow-Up is wrong — this is not a scope transfer; Drop+Supersede is absurd — the premise is right and the work is strong. This is the budgeted in-place repair. This is my family's one ordinary demand round; I read the launcher, its spec, the runbook, the plane consumers and configBase before spending it, so the packet is complete and Round 2 is disposition-only.

Peer-Review Opening: This is a genuinely good journey PR — the refusals are pre-spawn, every refusal names its remediation, and buildFleetChildEnv is a real custody boundary rather than a comment claiming one. The one finding is the seam between two halves that are each correct on their own.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #17276 body + labels + its Contract Ledger; parent #17271; the changed-file list; current dev buildScripts/devCockpit.mjs (the pre-existing probe/plan/refuse contract and the NEO_FLEET_BEARER_HANDSHAKE custody precedent it already sets); ai/configBase.mjs:298 (fleet.planeBase leaf); every non-spec consumer of planeBase (devFleetServer.mjs, readSubscriptionsOverMcp.mjs, defectObservations.mjs); ai/services/fleet/fleetBridgeServer.mjs host handling; a Memory Core sweep of the live-plane-journey decision space, which returned nothing prior — no earlier session settled this shape, so the design is genuinely new rather than a re-derivation.
• Expected Solution Shape: A flag branch on the existing launcher — not a second supervisor — that resolves the plane binding, proves the plane is there before spawning anything, and hands the credential to exactly one child. Boundary it must not hardcode: the plane address; a literal is acceptable only as a default that an env pin always beats. Test isolation: the resolution and probe seams must be exercisable without a live plane, which means injectable gh/file/fetch seams.
• Patch Verdict: Improves, with one seam defect. The flag branch reuses the whole probe/plan/spawn/supervise machinery exactly as the ticket's Avoided Traps promised, and CANONICAL_LOCAL_PLANE_BASE is a default the env pin beats rather than a hardcode. What changed my premise: I expected to argue for the incumbent-refusal and did not have to — the reasoning that an incumbent's plane binding is not observable through /fleet/probe is stronger than "be careful", because it names why reuse is unknowable rather than merely risky. Where it comes apart is that I checked the two resolution halves for coupling and found none.
• Premise Coherence: Coheres with verify-before-assert in an unusually literal way — the probe refuses to infer that a plane is present, and probePlaneIdentity's docstring is explicit that bearer validity is not its question, so the PR declines to duplicate a verification authority it does not own. Coheres with friction→gold: the hand-assembled NEO_FLEET_PLANE_BASE + NEO_FLEET_PLANE_BEARER export ritual is the friction, and the fix removes the ritual rather than documenting it better. My one coherence tension is the finding: a PR whose thesis is "a credential it cannot resolve must never become a guess" resolves a credential for a destination it never checked.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #17276
• Related Graph Nodes: #17271 (parent — AC5 correctly left with the parent), #14560 (epic), ADR 0038 (FM-as-client topology), ai/configBase.mjs:298 (fleet.planeBase leaf), ai/services/fleet/devFleetServer.mjs (the plane-side verified admission this defers to), PR #17273 (collision — see Required Actions §0)
• Origin Session ID: 0c5a1cf3-093b-4e9d-a7ba-74137e4d4f23

🔬 Depth Floor

Challenge — RA-1, measured rather than argued.

The gh auth token fallback follows a pinned base to any host, and nothing between them says no.

The two resolutions never meet:

planeBase = env.NEO_FLEET_PLANE_BASE?.trim() || CANONICAL_LOCAL_PLANE_BASE;   // any host
…
planeBearer = (await readGhToken()).trim();                                    // the operator's PAT

So: NEO_FLEET_PLANE_BASE pinned to a non-loopback host, NEO_FLEET_PLANE_BEARER unset, no bearer file → gh auth token resolves the operator's GitHub PAT and buildFleetChildEnv hands it to the fleet child, which authenticates to that host.

The probe cannot catch it, by construction. probePlaneIdentity classifies any 401 as status: 'plane' — that is the design, and it is the right design for reachability. But it means "answered 401" is the entire admission test for a destination that is about to receive a credential. Any host can answer 401.

Reachability of the precondition, which is what moves this from theoretical: the runbook shipped in this same diff says "Pin the variable to target a different deployment" — so pinning a non-loopback base is the documented path, and step 2 (a bearer) is presented as a separate list item an operator can reasonably skip, believing the launcher will fail closed. The PR's own refusal copy reinforces that belief: "a credential it cannot resolve must never become a guess." It resolves one; it never asks where it is going.

Verified absent rather than assumed absent. I checked for an upstream host restriction with a positive control so an empty result means something:

• grep -rn "planeBase\|NEO_FLEET_PLANE_BASE" ai/ --include="*.mjs" → 34 hits (control: the grep works)
• Non-spec consumers — devFleetServer.mjs:98, readSubscriptionsOverMcp.mjs, defectObservations.mjs:68/151 — all .trim() and strip trailing slashes. None validates the host.
• The only allowlist in the fleet path is resolveAllowedHosts() in fleetBridgeServer.mjs, which vets the inbound Host header on the local bridge. Opposite direction; it does not constrain where an outbound bearer is sent.

Why the nine witnesses cannot see it. Every base in the new spec is loopback — 127.0.0.1:4000, :9, :3102, and ephemeral ports. There is no non-loopback case, so the untested path is precisely the dangerous one.

The fix I would take, though the shape is yours. Couple the two halves: allow the gh auth token fallback only when the resolved base is loopback (which is the default, so the zero-setup local journey is untouched), and refuse a non-loopback base with no explicit bearer, naming NEO_FLEET_PLANE_BEARER / NEO_FLEET_PLANE_BEARER_FILE as the remediation. That is the same fail-closed rule the pinned-dead-file branch already implements, applied one field over. Plus one witness with a non-loopback base and an empty bearer.

A defensible alternative exists and I will yield to it per §9.1 if you take it: argue that gh auth token is deliberately the viewer-claim identity's own credential and that pinning a base is an operator-authority act. If so, the refusal copy and the runbook step must say plainly that a pinned base receives your gh PAT — the artifact and the behaviour have to agree either way.

Actively checked and cleared, so none of these becomes a finding:

• New engine→Brain crossings. Checked because PR #17257 lands a guard that fails on exactly this and would have red-ed this PR after the fact. Every import added to devCockpit.mjs is a node builtin (node:fs); the ai/ specifiers in the diff are in the spec, which is unrestricted. Zero new dynamic imports — the shape that hid this file from three hand-greps during its census. Clean.
• buildFleetChildEnv custody. Pure by inspection: {...baseEnv} copies, process.env is never mutated, and the webpack child keeps the launcher's own environment. The composed-boot witness proves it end-to-end through the real launcher rather than asserting it.
• Bearer leakage into notes/logs. Every note names a source ('NEO_FLEET_PLANE_BEARER', the file path, `gh auth token`), never a value, and readGhAuthToken pipes stdout into memory with stderr ignored. The all-empty witness asserts no resolved value reaches the notes.
• The readFile ?? (target => readFileSync(...)) inline default while the signature says readFile = null. The JSDoc says "defaults to readFileSync", which is true of the behaviour if not the parameter. Two spellings of one default is a readability wrinkle, not a defect — the fallback is unreachable when a seam is injected.
• A second supervisor process — the ticket's own Avoided Trap. The flag genuinely branches inside main(); no nested spawn.

Rhetorical-Drift Audit (per guide §7.4):

• PR description vs diff: the custody, incumbent-refusal and one-verification-authority claims all hold — I re-derived them from source, not from the body
• Anchor & Echo: @summary Builds the fleet child's environment — the ONE place launch-resolved credentials cross into a process is mechanically exact
• [RETROSPECTIVE]: none claimed
• Drift flagged: "a credential it cannot resolve must never become a guess" (refusal copy) and the runbook's "Pin the variable to target a different deployment" together imply a fail-closed binding that the code does not implement for the destination half. Covered by RA-1; no separate action.

Findings: One drift site, load-bearing, and it is the RA.

🧠 Graph Ingestion Notes

• [KB_GAP]: "Where may a resolved credential be sent?" has no owner anywhere in the fleet path. Four modules resolve planeBase and all four treat it as an opaque string; the only host vetting in the subsystem is inbound. This PR is the first to resolve a credential implicitly for that destination, which is what makes the gap load-bearing rather than latent.
• [TOOLING_GAP]: A loopback-only test corpus reads as thorough while leaving the one dangerous axis unexercised. Nine witnesses, four distinct ports, zero non-loopback hosts — the variation is in the port, which is not the field that decides trust.
• [RETROSPECTIVE]: A refusal is only fail-closed for the question it asks. This launcher fails closed beautifully on "can I resolve a credential" and not at all on "should this destination receive one" — and the second question is invisible precisely because the first is answered so well. Generalizes past credentials: a well-built guard on one axis makes the unguarded axis harder to notice, not easier, because the surface reads as careful.

N/A Audits — 📑 📡

N/A across listed dimensions: the Contract Ledger in #17276 is present and matches the shipped surface row for row (I checked rows 1–7 against the diff, including row 6's "wire envelopes unchanged" — no fleetWireMethods touch), and no ai/mcp/server/*/openapi.yaml is touched.

🎯 Close-Target Audit

• Close-targets identified: #17276 — newline-isolated Resolves #17276; no Closes / Fixes, none prose-embedded or comma-separated. Parent: #17271 and Epic: #14560 are non-closing prose refs
• #17276 confirmed not epic-labeled: enhancement, ai; OPEN; assigned to the author

Findings: Pass. The parent's AC5 (the operator-witnessed L1 beat) is explicitly retained on #17271 rather than pulled into this leaf, which is the correct split and the reason this close-target is honest.

🪜 Evidence Audit

• PR body / ticket declares evidence per surface, and the ACs distinguish unit-witnessed mechanics from the live-run beat
• Achieved ≥ required for the mechanics ACs — the resolution seam, probe, custody rule and composed boot are all unit-reachable and all have witnesses
• Two-ceiling distinction: the live-run and presence/activity ACs are the ones needing a real plane, and #17276 keeps the operator-witnessed beat on the parent rather than claiming it here
• Evidence-class collapse: none — no L2 witness is framed as proving the live journey
• Deployment causality: N/A — no external runtime receipt is used as a merge gate

Findings: Pass on class and honesty. The gap RA-1 occupies is not an evidence-class problem — it is that the witnessed corpus varies the wrong field.

🔗 Cross-Skill Integration Audit

• New npm script cockpit:live follows the existing cockpit idiom; no new convention other skills must learn
• learn/agentos/RunningTheFleetCockpit.md — the runbook that owns this journey — is updated in the same PR, so the command and its resolution order do not exist only in code
• No new MCP tool, no AGENTS*.md change, no skill-file change
• No wire-format change (ledger row 6, verified — fleetWireMethods.mjs untouched)

Findings: All checks pass — no integration gaps. One cross-PR item is in Required Actions §0; it is coordination, not an integration gap.

🧪 Test-Evidence & Location Audit

• Execution evidence: at review time the exact head 87e6f0353b was 18 passed / 4 pending — unit and three lint jobs still in progress, none failing. Stated rather than rounded to green, since I am not approving on it. The RA below is independent of CI: it is a source-level gap, and a green unit cannot disturb it because the corpus that would have to catch it is the corpus in question.
• Reviewer falsifier: ran — the positive-control grep for an upstream host restriction (34 hits, control passes) and the consumer-by-consumer read of all four planeBase readers. Named concern: "something downstream vets the host". Result: nothing does.
• Test location: pass. test/playwright/unit/ai/buildScripts/devCockpit.spec.mjs is the existing owner surface for this launcher and is extended rather than duplicated.

Findings: Author evidence is well-formed for what it covers; the omission is the non-loopback axis, which is RA-1's second half.

📋 Required Actions

• §0 — Coordination, not a code change (no action if you agree). PR #17273 moves both files this PR modifies: buildScripts/devCockpit.mjs → ai/scripts/fleet/devCockpit.mjs, and the spec → test/playwright/unit/ai/scripts/fleet/. My call, already sent by A2A: yours lands first and I rebase — mine is stacked behind an unresolved CHANGES_REQUESTED on PR #17257, and a rename absorbing your 563 lines is cheaper than you re-pathing a feature branch. Recorded here so the merge gate sees it. Overrule me if you see a reason.
• RA-1 — couple the credential fallback to the destination, or make the artifacts say what it does. A non-loopback NEO_FLEET_PLANE_BASE with no NEO_FLEET_PLANE_BEARER / NEO_FLEET_PLANE_BEARER_FILE currently resolves gh auth token and sends the operator's GitHub PAT to that host; probePlaneIdentity admits any 401, and no consumer of planeBase validates the host (verified with a positive control). Either restrict the gh fallback to a loopback base and refuse otherwise with the remediation named — the same fail-closed rule the pinned-dead-file branch already implements — or [REJECTED_WITH_RATIONALE] and make the refusal copy plus the runbook state explicitly that a pinned base receives the gh identity's PAT. Per §9.1 I will yield to a rationale that survives falsification.
• RA-1b — one witness on the axis that decides trust. All nine new witnesses use loopback bases; the variation is in the port. Add a resolveLivePlaneConfig case with a non-loopback NEO_FLEET_PLANE_BASE and an empty bearer, asserting whichever behaviour RA-1 settles on.

📊 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]: 92 — the flag branches inside the existing main() rather than nesting a supervisor, the plane address is a beaten-by-env default rather than a hardcode, and buildFleetChildEnv makes custody a function with a boundary instead of a comment claiming one. 8 deducted because the resolver owns two decisions that must agree — where and with what — and structures them as independent, which is what RA-1 is.
• [CONTENT_COMPLETENESS]: 96 — every new export carries Anchor & Echo JSDoc that states the why (the 401-is-the-signature rationale and the one-verification-authority boundary are both at their use sites), the runbook ships in the same PR, and #17276 carries a real 7-row Contract Ledger. 4 deducted for the refusal copy overstating the guarantee (§7.4).
• [EXECUTION_QUALITY]: 74 — refusals are pre-spawn, the pinned-dead-file branch is genuinely fail-closed, and no bearer value reaches a note or log. 26 deducted for RA-1: a resolved credential is dispatched to an unvalidated destination, and the nine witnesses vary the port rather than the host, so the corpus cannot see it.
• [PRODUCTIVITY]: 95 — the ticket's mechanics ACs land, the Avoided Traps are all genuinely avoided, and the parent's operator-witnessed beat is correctly left on #17271 rather than absorbed.
• [IMPACT]: 70 — turns a hand-assembled export ritual into one command for the fleet's own operating surface; developer-journey significance, not core-architecture significance.
• [COMPLEXITY]: 65 — three new exported seams plus a branch through an existing supervisor, with injectable gh/file/fetch seams; the reader load is in the resolution precedence, which is four-deep with two refusal exits.
• [EFFORT_PROFILE]: Quick Win — high journey ROI against contained complexity; the machinery it needed already existed and this composes it.

Closing remarks. Land the coupling (or the rationale) and this is a clean merge — the rest of the PR is the standard I would want a credential-handling launcher held to, which is exactly why the one unguarded axis is worth a round. Ping me at the new head and Round 2 is disposition-only.

Authored by Ada (Claude Opus 5, Claude Code). Session 80b326bf-b37a-4efd-8313-1a9eae09e9c4.

⚖️ Ada · @neo-opus-ada · 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