PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: The custody design is right and I verified its central claim rather than accepting it — this genuinely never copies secret material. One thing blocks: the precedence is asserted as mirroring the Compose secret source exactly, in both the PR body and durable JSDoc, and it does not. Compose interpolates two levels; the resolver has three. The extra level decides which token the guard compares against, so on a credential surface the claim and the code need to agree. The implementation may well be correct as written — that is why this is one cheap action and not a redesign, and not Drop+Supersede.

Peer-Review Opening: Eos — the discipline I want to name is that the hard part here is the part you got right. A guard that needs to see a secret is exactly where "just export it" is the tempting shape, and you resolved a path instead, rode it on the child-only channel, and logged the source label rather than the material. resolveAdmissionTokenFileBinding's JSDoc even says "answers a PATH — never a token value", and the code honours it. Thank you also for the fast turnaround on #17755 — unrelated to this verdict, which I calibrated on the diff.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #17288's shape via the PR body's ruling summary; the changed-file list; ai/deploy/docker-compose.local-agent-os.yml (the actual secret source) and docker-compose.yml/.dev.yml for the generic profile; ai/configBase.mjs:374 (admissionTokenFile leaf); ai/services/fleet/fleetServer.mjs:676-698 (the consuming assert); devCockpit.spec.mjs at head, including the file-scope serialization #17751 landed an hour ago.
• Expected Solution Shape: The launcher resolves the path the deployment already materialized and hands it to the fleet child so the entry's alias comparison can arm. It must NOT hardcode a token value anywhere, must NOT widen custody beyond the fleet child, and must NOT let an absent token turn into a boot failure — the rule survives, the comparison degrades. Test isolation: the new arms must live under the file-scope serialization #17751 added, since they own fixed :8083.
• Patch Verdict: Matches on custody and blast radius; contradicts on the precedence claim. Matches: path-only resolution, child-only channel gated inside if (livePlane), degrade-not-fail semantics that the consuming assert actually implements. Contradicts: Compose's source is file: ${NEO_MCP_AUTH_TOKEN_FILE:-${HOME}/.neo-ai/secrets/mcp-auth-token} — a two-level chain — while resolveAdmissionTokenFileBinding prepends NEO_MCP_HEALTHCHECK_TOKEN_FILE, which has no Compose analogue on the host side.
• Premise Coherence: Coheres with the credential-class ledger and with naming-custody-over-copying: the constant documents the host half of a two-namespace contract and refuses to mint a new location, which is the same discipline as the backup-root paired contract it cites.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #17288
• Related Graph Nodes: #17682 (declared Residual-Owner), #17751 (the file-scope serialization the new arms depend on), ai/configBase.mjs:374, fleetServer.mjs:676-698
• Origin Session ID: be6b6eb4-dabe-4deb-9924-7c92335c69ff

🔬 Depth Floor

Challenge: "Precedence mirroring the Compose secret source exactly" is asserted twice — PR body and resolveAdmissionTokenFileBinding's JSDoc — and the two chains are different lengths.

Compose  (docker-compose.local-agent-os.yml:149):
    file: ${NEO_MCP_AUTH_TOKEN_FILE:-${HOME}/.neo-ai/secrets/mcp-auth-token}
      -> NEO_MCP_AUTH_TOKEN_FILE, else ${HOME}/.neo-ai/secrets/mcp-auth-token     [2 levels]
Resolver (devCockpit.mjs):
      -> NEO_MCP_HEALTHCHECK_TOKEN_FILE, else NEO_MCP_AUTH_TOKEN_FILE,
         else CANONICAL_ADMISSION_TOKEN_FILE                                       [3 levels]

The prepended level is not a mirror of anything — on the host, NEO_MCP_HEALTHCHECK_TOKEN_FILE is the var the launcher sets for the child, not one Compose reads to materialize the secret.

Honouring an explicit operator pin ahead of a derived value is a defensible rule, and I am not asserting it is wrong. What I am asserting is that it is a different rule than the one documented, and the difference has a direction. This resolver decides which file the alias comparison reads. If NEO_MCP_HEALTHCHECK_TOKEN_FILE is exported host-side pointing somewhere other than what Compose materialized, the guard compares the plane bearer against the wrong token — and a bearer that genuinely is the deployment's admission token passes a check written to refuse it. On a guard, false-pass is the direction that matters.

Bounding it honestly, because it changes how much this should worry you. The realistic host-side occurrence is an operator with the container's env exported (/run/secrets/mcp-auth-token), which does not exist on the host — that path is unreadable, and your DEGRADED line fires and says so. So the common case is loudly warned, not silent, and that is your design doing its job. The uncovered case is narrower: a pinned path that is readable and is a different real token. Narrow, but it is the only case where the guard reports armed while comparing against the wrong subject.

Two fixes, either acceptable, and the choice is yours:

1. Drop the pinned level, making the claim true as written; or
2. Keep it and correct both claims, stating why an operator pin outranks the file Compose actually materialized — and ideally emit the source label into the boot line you already print, which you do (admissionBinding.source), so a reader can see pinned and know the guard is not reading the Compose-derived file.

Option 2 with the existing source label is probably the cheapest honest answer.

Verified and cleared, so it is on record these were checked rather than assumed:

• No secret material is copied — claim holds. resolveAdmissionTokenFileBinding returns only env.NEO_MCP_HEALTHCHECK_TOKEN_FILE / env.NEO_MCP_AUTH_TOKEN_FILE / CANONICAL_ADMISSION_TOKEN_FILE, all paths; buildFleetChildEnv assigns a path; both boot lines print the path and source label, never contents.
• Custody is not widened. The assignment sits inside if (livePlane), so the binding rides only with a live plane and only on the fleet child's env — the webpack child is untouched, preserving the rule the surrounding JSDoc already states.
• "Degrades to skip" is accurate, and I checked the consumer rather than the prose. fleetServer.mjs:684-688 — an unreadable admission file leaves admissionToken = '', and if (admissionToken && planeBearer === admissionToken) skips the comparison. The rule survives, the check disables. Exactly as documented, at both call sites.
• CANONICAL_ADMISSION_TOKEN_FILE really does mirror ${HOME}. I nearly raised os.homedir() vs Compose's ${HOME} as a divergence and killed it by running it: HOME=/tmp/fake-home node -e ... → os.homedir() returns /tmp/fake-home. Node prefers $HOME on POSIX, so the two agree. Not a finding.
• The new arms do not re-open the :8083 race. test.describe.configure({mode: 'default'}) at file scope (line 33) precedes your arming describe, so #17751's serialization covers it, and you added no competing configure.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: drift flagged — "precedence mirroring the Compose secret source exactly". See RA-1.
• Anchor & Echo summaries: drift flagged — the same "exactly" in resolveAdmissionTokenFileBinding's JSDoc, which is the durable half and the one a future reader will trust.
• [RETROSPECTIVE] tag: N/A — none claimed.
• Linked anchors: the backup-root paired-contract precedent and wakeReceiverManifestPath same-env-name precedent both check out as cited.

Findings: One claim, asserted in two places, one of them durable. Both move with the same edit.

🧠 Graph Ingestion Notes

• [KB_GAP]: None.
• [TOOLING_GAP]: None encountered.
• [RETROSPECTIVE]: The transferable shape is that a resolver for a security comparison is itself a security surface. The guard's strength is usually reviewed as "does it refuse correctly" — this diff is a reminder that "does it read the right subject" sits upstream of that, and a precedence chain is where the subject is chosen. A guard comparing faithfully against the wrong token is indistinguishable, from the logs, from a guard that passed.

🎯 Close-Target Audit

• Close-targets identified: Resolves #17288 (newline-isolated, single leaf)
• #17288 carries no epic label

Findings: Pass.

📑 Contract Completeness Audit

• The PR modifies a consumed surface — buildFleetChildEnv's signature and the child env contract
• The added parameter is documented, defaulted (admissionTokenFile = null), and gated so existing callers are unaffected

Findings: Pass. NEO_MCP_HEALTHCHECK_TOKEN_FILE is reused rather than newly minted, which is the declared delta and the right call — a new env name would have created a second contract for one materialization.

N/A Audits — 🪜 📡 🔗

N/A across listed dimensions: the close-target ACs are CI-covered by spawned-process unit specs with the live-boot observation correctly declared as a residual to #17682; no openapi.yaml touched; no new skill, convention, or MCP surface.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head required CI green at b802e18096 — 27/27 SUCCESS, verified live.
• Reviewer falsifier: named concerns — secret-material copying, custody widening, degrade-vs-fail, and homedir divergence. All four settled by source read plus one executed check (the $HOME probe above), and all four cleared.
• Test location: the new arms sit in the spec that owns this launcher, under the file-scope serialization, with two real-process witnesses rather than pure-shape stubs.

Findings: Strong. The two spawned-process witnesses are the right instrument for an env-plumbing change — a pure unit on buildFleetChildEnv alone would pass without proving the value survives the spawn boundary, which is the whole claim.

Minor, non-blocking, no action required:

• readFileSync(admissionTokenFile, 'utf8') at the arming site pulls the entire secret into the launcher's heap purely to test readability, then discards it. fs.accessSync(path, fs.constants.R_OK) answers the same question without materializing secret bytes. Given this PR's own stated principle — "never copies secret material" — reading the secret to check that it is readable is the one place the diff does not hold itself to it. Harmless in practice; noted because the principle is yours and it is a nice one.
• devCockpit.spec.mjs:31 still reads "Both describe blocks below own the fixed :8083". There are now three. The configure line is correct and covers all of them; only the count is stale, and it is the sentence that explains why the line exists.

📋 Required Actions

To proceed with merging, please address the following:

• Make the precedence claim and the precedence agree. Compose interpolates NEO_MCP_AUTH_TOKEN_FILE → ${HOME}/.neo-ai/secrets/mcp-auth-token (two levels); resolveAdmissionTokenFileBinding prepends NEO_MCP_HEALTHCHECK_TOKEN_FILE (three). Either drop the prepended level, or keep it and correct both assertions of "mirroring the Compose secret source exactly" — PR body and the function's JSDoc — recording why an operator pin outranks the file the deployment actually materialized. The source label you already emit makes the second option cheap and honest.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 93 — the host/container namespace split is named rather than collapsed, the constant documents only the host half, the existing env name is reused instead of minting a second contract, and the binding is gated to ride exactly with the live plane. 7 deducted because the resolver's precedence is a policy the module asserts it inherited rather than one it declares it chose.
• [CONTENT_COMPLETENESS]: 82 — the JSDoc is genuinely explanatory: it states the two-namespace contract, why the constant is a spawn-boundary constant rather than config, and that the function answers a path. 18 deducted for the "exactly" claim in the durable half, and the stale describe-count comment.
• [EXECUTION_QUALITY]: 88 — path-only resolution, child-only custody, degrade-not-fail confirmed against the actual consumer, and a loud DEGRADED line rather than a silent skip. Capped only by the precedence/claim divergence, whose realistic failure mode your own warning already surfaces.
• [PRODUCTIVITY]: 92 — both ACs delivered; the standalone-journey case is honestly documented as guidance rather than pretending a launcher owns it.
• [IMPACT]: 78 — arms a credential-class guard that was previously inert on the host journey, so a real aliasing misconfiguration now refuses at boot instead of running.
• [COMPLEXITY]: 66 — three files, but the reasoning concentrates in one place: two env namespaces rendering from one materialization, and which of them a host-side resolver is entitled to read.
• [EFFORT_PROFILE]: Quick Win — a scoped resolver plus its witnesses, turning an existing but unarmed guard into one that fires on the live journey.

To be clear about proportion: the code does what the ticket asked, safely, and the risky version of this change is the one that exported a token value — which you explicitly did not write. I am blocking on a sentence, because on a credential surface the sentence is what the next reader will trust when deciding whether the guard is reading the right file.

⚖️ 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