Skip to content

fix(transport): remove Brains from internal Claude runs (BRNS-DESK-079) - #94

Merged
stefan-ssv-labs merged 1 commit into
devfrom
fix/desk-079-internal-claude-boundary
Aug 14, 2026
Merged

fix(transport): remove Brains from internal Claude runs (BRNS-DESK-079)#94
stefan-ssv-labs merged 1 commit into
devfrom
fix/desk-079-internal-claude-boundary

Conversation

@stefan-ssv-labs

Copy link
Copy Markdown

Summary

Fixes BRNS-DESK-079.

Invisible, app-owned Claude runs now receive neither the app's Brains MCP configuration nor BRAINS_API_TOKEN. They also omit app context, canvas instructions, MCP-schema guidance, and remembered approvals while retaining the existing dontAsk, empty-allowlist, one-turn, non-persistent posture. Native CLAUDE.md accounting remains because Claude reads that file independently.

Normal interactive Claude sessions keep their endpoint, token, approvals, effort, context, canvas instructions, and MCP schema.

Requirements → implementation

Requirement Change
No app Brains MCP or token in internal Claude runs A pure claude_transport planner returns mcp_config: None and brains_token: None; the child environment explicitly removes BRAINS_API_TOKEN.
Preserve the primary no-tool policy Internal settings remain dontAsk, empty allowlist, max_turns: 1, and no_session_persistence: true; remembered approvals cannot widen them.
Omit unnecessary private context Internal planning skips app context, canvas instructions, and MCP-schema blocks while retaining native preamble accounting.
Interactive behavior unchanged Structural regression coverage asserts the normal Claude plan still carries Brains, remembered approvals, effort, and instructions exactly once.
Secret-safe proof Tests use only brn_not_a_real_token and inspect pure argv/environment/settings without launching a CLI or using an account.

Regression provenance

Reproduction (pre-fix, secret-safe)

  • Environment: source inspection on macOS arm64 at brains-desktop@3bca16391fe9626767e2e6b0ac3568988fa291b0; no live token or account interaction.
  • Preconditions: construct TransportPlan { provider: Anthropic, internal: true } with a fake token and app context.
  1. Follow the Anthropic arm in src-tauri/src/transport.rs to its SpawnPlan.
  2. Inspect its Claude argv, child environment, settings, and injected ledger blocks.

Expected: no app Brains MCP configuration, no BRAINS_API_TOKEN, and no app-injected private context reaches the hidden child; one-shot no-tool policy remains.

Actual before this PR: mcp_config and brains_token were populated and app context/canvas/MCP-schema blocks were appended; no-tool behavior depended on the empty allowlist.

Evidence: ticket detail, parent PR, cited review, and structural tests in this PR.

Validation

  • cargo fmt --all --check — pass.
  • git diff --check — pass.
  • cargo test -p brains-desktop transport — 13 passed, 0 failed; covers internal Claude argv/env/settings/injection, native preamble accounting, normal Claude behavior, and the parent Codex boundary.
  • cargo clippy --workspace --all-targets -- -D warnings — pass; only Cargo's existing future-incompatibility notice for block 0.1.6 remains.
  • ⚠️ cargo test --workspace — all BRNS-DESK-079 tests pass; 2 host-Keychain sandbox integration tests fail. The same two tests fail with identical output on unchanged origin/dev (securityd Module Directory Service error / KEYCHAIN-DENIED), proving they are inherited.
  • cargo check -p brains-browser --features chromium — pass with the three existing Chromium warnings.
  • npm run check — 0 errors, 0 warnings.
  • npm run lint:size — pass (655 files checked; ticket files remain below 400 lines).
  • npm run lint:imports — pass.
  • npm run lint:manifests — pass.
  • npm run lint:css-vars — pass.
  • npm test — 142 files passed, 4 skipped; 1,721 tests passed, 17 skipped; drive tests 6/6 and eval harness tests 24/24 passed.
  • npm run build — pass; both committed manifests regenerated without diff and six resources staged.
  • npm run lint:resources — pass.
  • ⚠️ npm run eval -- --no-agent — 201 pass, 6 inherited failures, 4 pending, 17 skipped, matching the merged parent PR's baseline: duplicated Keychain/sandbox checks, token-less live mention read, and the known pixel check.
  • ⚠️ npm run eval:pixels -- --no-judge — Chrome 152 correctly refused comparison with Chrome 146 goldens. Pinned Chrome 146 timed out on the first capture (Runtime.callFunctionOn timed out), the same environment-sensitive lane documented by PR fix(transport): structurally exclude Brains from internal Codex runs (BRNS-DESK-054) #90. No frontend files changed.
  • ⚠️ npm run dev:app — debug binary built and launched, context materialized, scheduler armed, and no new startup error appeared. The debug identity had no Brains token, so a live Claude side-question could not be exercised; the process was stopped and port 1430 released.

Risk, scope, and shipping

  • Security: reduces defense-in-depth exposure by withholding a credential and remote MCP definition from invisible no-tool Claude children. No active CLI bypass is claimed.
  • Non-goals: no side-question UX changes, no Codex changes, no provider protocol changes, and no persisted-run lifecycle changes.
  • Shipping: Rust source and tests only. No IPC shape, Tauri capability, bundle resource, sidecar, manifest, signing, updater, or persisted-state migration change; no packaged build is required.
  • Platforms: implementation and local validation ran on macOS arm64. CI's Rust matrix provides macOS and Windows coverage; frontend CI runs on Ubuntu. Linux Rust remains unexercised by repository CI.
  • Review: the host has no /code-review capability, so a local committed-diff defect pass checked the credential/config boundary, final argv/environment, interactive non-regression, current internal callers, and native preamble exception; no blocker, critical, or high-value finding remained.

Merge method: squash (do not merge-commit or rebase-merge).

@sebastian-ssvlabs sebastian-ssvlabs left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed-at: dec99d2

@nir-ssvlabs nir-ssvlabs left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This closes a gap I looked straight at and waved through. Reviewing #90 I checked whether the Claude arm had an equivalent boundary, found SessionSettings::headless(&[]) with max_turns: 1, and concluded there was "no Codex/Claude asymmetry to file". That was accepting a policy control as equivalent to structural absence — the empty allowlist denies tool calls, but the credential and the MCP definition were still handed to the child. The framing in this description ("policy-versus-credential-absence gap") is the right one, and it's the distinction I should have drawn.

The fix mirrors the Codex arm exactly, which is what I'd want: mcp_config: (!internal).then(...) and brains_token: brains_token.filter(|_| !internal) both go None, the if !internal block skips context, canvas and MCP-schema, and allow_remembered stays in the interactive branch so a remembered approval can't widen an internal run. Crucially the empty allowlist is kept rather than replaced — the policy gate and the credential absence now both hold, which is the point of doing this at all.

The env removal is real and pre-existing rather than newly claimed: claude/spawn.rs:205 takes the else branch on a None token and calls env_remove(BRAINS_TOKEN_ENV), with a comment about not inheriting a stale developer-shell token. So passing None genuinely scrubs it rather than merely omitting an override.

Keeping the native CLAUDE.md rows is right for the same reason it was on the Codex side — the CLI reads those files independently, so dropping the accounting would understate the child's context rather than reduce it.

Checked: both SpawnPlan fields on the internal path, the !internal guards around the injected blocks, allow_remembered's placement, and the spawn-side env removal that None reaches. Not read: the 132 lines of new structural tests — the planner is pure now, which is what makes them possible.

Merge: ✅ into dev.

@stefan-ssv-labs stefan-ssv-labs left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ review-pr (self-review, cannot approve own PR): clean — no blockers or criticals against BRNS-DESK-079.

@stefan-ssv-labs
stefan-ssv-labs merged commit f5c203b into dev Aug 14, 2026
6 of 7 checks passed
@stefan-ssv-labs
stefan-ssv-labs deleted the fix/desk-079-internal-claude-boundary branch August 14, 2026 13:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants