fix(transport): structurally exclude Brains from internal Codex runs (BRNS-DESK-054) - #90
Conversation
nir-ssvlabs
left a comment
There was a problem hiding this comment.
I enumerated the ways Brains could still reach an internal child rather than checking the ways it's blocked, and all four close:
- Plan —
mcp_endpointandbrains_tokenare bothNonewheninternal, and the branchreturns before context, canvas and MCP-schema blocks. Structural, as the title says, rather than a stack of conditionals that a later edit can slip past. - Inherited env —
spawn.rs:257isenv_remove(BRAINS_TOKEN_ENV), not merely an omission. That's the distinction that matters: a parent holdingBRAINS_API_TOKENwould otherwise pass it down invisibly. - User config — with
mcp_endpoint: None,app_supplies_brainsis false, so a user-defined enabledbrainsserver falls through to the emit branch and lands in the table asenabled=falserather than being silently left alone. The normal path still supersedes it, so the interactive behaviour is unchanged. - Resume —
internalis persisted tometa.internaland read back throughapply_resumed_internal_policy, so a resumed run cannot come back as a normal one. That was the failure I went looking for.
Keeping native_preamble_blocks on the internal path is right and worth the comment it has: Codex reads AGENTS.md from the cwd regardless, so omitting the row would have made the ledger understate the child's context rather than reduce it.
- 🔵 The boundary is opt-in —
request.internal == Some(true), so a future invisible run type that forgets the flag gets the full token and MCP server. That's the correct default for interactive chats and the internal callers are app-owned, so it's a shape to keep in mind rather than a change to make here.
Checked: the internal branch's omissions and early return, the child env removal, mcp_servers_table on both the supplied and tokenless paths, and internal persistence across resume. Not read: the 247 lines of new transport tests — the mechanism is what I wanted to see directly.
Merge: ✅ into dev.
stefan-ssv-labs
left a comment
There was a problem hiding this comment.
✅ review-pr (self-review, cannot approve own PR): clean — no blockers or criticals against BRNS-DESK-054.
Optional polish
💡 SUGGESTION: The internal app-server is built without BRAINS_API_TOKEN, but codex_spawn::spawn first runs codex mcp list --json through a separate Command::new(bin) that only removes CLAUDECODE, so that discovery process still inherits the parent token. It does not expose MCP to the model, but removing BRAINS_API_TOKEN from this probe as well would make the no-token invariant cover every Codex child process; add a focused environment assertion if you take this follow-up.
|
Took the optional hardening in c806046: the codex mcp list --json discovery process now removes BRAINS_API_TOKEN, with a focused command-environment assertion. This does not alter interactive app-server behavior. |
nir-ssvlabs
left a comment
There was a problem hiding this comment.
Re-reviewed at c8060463. The delta scrubs BRAINS_TOKEN_ENV from the codex mcp list discovery command and pins it with a test. That's a real hole and I missed it on my last pass — I enumerated "inherited env" as one path and checked only the session spawn, when the code path spawns two children. Discovery ran before it, inheriting the credential unconditionally, on every run rather than just internal ones. Worth naming since the whole point of that review was to enumerate exhaustively.
🟡 Medium · one more child on the same path still inherits it (non-blocking): @stefan-ssv-labs codex/models.rs:67's probe_models spawns codex app-server with no env scrubbing and — unlike both commands in spawn.rs — no -c mcp_servers= override, so it reads the user's config.toml as-is while holding BRAINS_API_TOKEN. A user with [mcp_servers.brains] configured has a child that could authenticate as them during what is only a model-list probe. I can't prove from here that app-server starts MCP servers during initialize, so I'm not calling it exploitable — but it's the identical shape to the one this commit just closed, and the same one-line .env_remove(BRAINS_TOKEN_ENV) settles it either way.
Checked: the new scrub and its test, and every Command::new under codex/ and claude/ for token handling — spawn.rs 212 and 244 are both explicit now, models.rs:67 is the remaining one.
Merge: ✅ into dev. The boundary this PR set out to draw holds; the probe is adjacent and cheap to fold in.
stefan-ssv-labs
left a comment
There was a problem hiding this comment.
✅ review-pr (self-review, cannot approve own PR): clean — no blockers or criticals against BRNS-DESK-054.
Re-reviewed at c806046 after the follow-up; the prior optional MCP-discovery token suggestion is fixed and covered by a focused regression test. No new actionable findings.
|
Addressed in 8ef6f2a: the short-lived codex model probe now explicitly removes BRAINS_API_TOKEN before spawning app-server, with a focused command-environment assertion. I kept the change limited to credential scrubbing; model-list behavior and user-config semantics are otherwise unchanged. |
nir-ssvlabs
left a comment
There was a problem hiding this comment.
Re-reviewed at 8ef6f2a2. The probe_models gap is closed the same way as the discovery one — extracted to build_probe_command with the scrub inside, and pinned by a test rather than left to inspection.
Since this is the third round and each of the previous two closed exactly one child process, I hunted for a fourth rather than assuming. The enumeration now comes back clean, and the two spawns that looked unhandled aren't:
claude/side_question.rsdelegates its environment to the sharedapply_env(plan)rather than building one, so a per-file grep undercounts it.- The Claude arm has no
internalstring because it draws the boundary differently —transport.rs:147gives an internal runSessionSettings::headless(&[]), an empty tool allowlist withmax_turns: 1. That's a stronger exclusion than disabling one server, and its comment says exactly why. So there's no Codex/Claude asymmetry to file.
Every remaining Command::new in the tree is either outside the credential's path or covered by one of those two mechanisms.
Merge: ✅ into dev. Nothing outstanding from me on this one now.
stefan-ssv-labs
left a comment
There was a problem hiding this comment.
Re-review at 8ef6f2a: clean. I found no new actionable findings.
The follow-up change now removes BRAINS_API_TOKEN from the short-lived codex app-server model probe, with a focused assertion. The internal Codex transport still omits the Brains endpoint/token and app-injected instructions, disables user-configured Brains entries, restores the headless boundary on resume, and leaves normal interactive Codex behavior unchanged.
Validation:
- Exact-head CI passed for Rust macOS, Rust Windows, Frontend, Lint resources, and token-less Eval; the macOS build was label-gated/skipped.
- Pixel eval reports the same five known baseline divergences (
gmail-inbox,rail-open,gmail-thread,ws-mention-picker,aim-armed) on unchanged UI surfaces. - Focused exact-head tests passed:
cargo test -p brains-model codex::models(3),cargo test -p brains-desktop transport(9),cargo test -p brains-desktop resume_keeps(2), andcargo test -p brains-model codex::spawn(8);git diff --checkpassed. - Native side-question smoke remains fixture-blocked by the unavailable Accessibility grant and missing
brains-devbuild. Codex Security scan: not needed; the credential-bound boundary is directly covered by structural tests and exact-head Rust gates.
No blocker, critical, or inline finding.
Summary
Fixes BRNS-DESK-054.
Invisible, app-owned Codex runs now receive neither an enabled Brains MCP server nor
BRAINS_API_TOKEN; they also omit app context, canvas instructions, MCP-schema guidance, remembered approvals, and selected reasoning effort. The Codex session uses its headless read-only posture. Resuming an internal run preserves the same boundary.Normal Codex chats keep their existing endpoint, token, context, canvas instructions, MCP schema, model effort, and interactive posture.
Requirements → implementation
codex_transportomits the endpoint and token; child env explicitly removesBRAINS_API_TOKEN.codex mcp list, a discoveredbrainsserver is emitted disabled in the finalmcp_serversoverride.Regression provenance
Reproduction (pre-fix, secret-safe)
brains.Expected: an internal run has no enabled Brains server, no Brains token, no app tool/context injection, and remains headless after resume.
Validation
cargo fmt --all --checkcargo clippy --workspace --all-targets -- -D warningscargo check -p brains-browser --features chromiumcargo test --workspace: two Keychain/sandbox failures reproduce identically onorigin/dev; all other tests, including this change's tests, pass.npm run check,npm run lint:size,npm run lint:imports,npm run lint:manifests,npm run lint:css-vars,npm test,npm run build,npm run lint:resourcesnpm run eval -- --no-agent: 201 pass, 6 known baseline failures, 4 pending, 17 skipped; exact summary matchesorigin/dev.npm run eval:pixels -- --no-judge: default Chrome is blocked by a golden-version mismatch. With pinned Chrome 146, the same five pre-existing golden divergences reproduce on this branch andorigin/dev.brains-devis not installed. No app was launched or driven.Review and scope
A local equivalent defect pass was completed because the
/code-reviewcapability is not available in this host. It checked final argv/environment/handshake construction, user-config merge behavior, internal resume policy, and interactive parity.No frontend, bundle, capability, updater, or persisted-data migration is included. Local validation was on macOS; CI remains the cross-platform check.
Deferred follow-ups
Merge method: squash (do not merge-commit or rebase-merge).