Skip to content

fix(providers): keep the MCP bearer out of ACP and Pi child environments - #17957

Open
only21mil wants to merge 2 commits into
pingdotgg:mainfrom
only21mil:fix/acp-pi-mcp-token-off-env
Open

only21mil wants to merge 2 commits into
pingdotgg:mainfrom
only21mil:fix/acp-pi-mcp-token-off-env

Conversation

@only21mil

Copy link
Copy Markdown
Contributor

ACP and Pi still hand each provider session's MCP bearer token to their children in environment variables, so it lands in every copy of those environments, crash-dump journal entries included. This PR moves both to an owner-only file per thread and puts only the file's path in the environment.

Problem

Each provider session gets a bearer token for the t3-code MCP server. The /mcp endpoint sits outside the environment auth stack, so this token is the only thing guarding the session's orchestrator tools (#12031 triage).

Codex and Claude no longer put the token in a child environment on main. Codex receives http_headers.Authorization over its app-server JSON-RPC connection. Claude moved from argv to an env var in #17408, then to the Agent SDK control channel in #17898. ACP joined the env channel with the V2 orchestrator (#2829), after the triage was written. ACP and Pi still pass the token itself through environment variables:

  • Registry ACP agents get T3_ACP_MCP_AUTHORIZATION in their own environment, and each of their client terminals gets it in the terminal's environment, through the adapter's processEnvironment overlay (packages/provider-acp/src/server/adapter.ts:714 on main).
  • Every ACP agent, Grok and Antigravity included, is told in session/new to spawn the t3 acp-mcp-bridge stdio server with T3_ACP_MCP_AUTHORIZATION in that server's environment (mcpServers, adapter.ts:705). Grok and Antigravity ignore processEnvironment, so this is the one place their sessions put the token.
  • Pi gets T3_MCP_BEARER_TOKEN in its environment (packages/provider-pi/src/server/mcpInjection.ts:313).

An environment variable gets copied far beyond the process it was meant for:

  • Every process those children start inherits it: shell and terminal commands, test suites, package scripts. Anything among them that prints or reports its environment (a log line, test output, an error reporter) copies the token with it.
  • On Linux hosts where core_pattern pipes crashes to systemd-coredump, a crash of any of these processes writes its whole environment to the journal as COREDUMP_ENVIRON (MCP bearer token passed via environment is captured in systemd-coredump journal entries on provider crashes #12031). That entry outlives the process, and journal-reading admin groups and anyone holding a copy of the journal can read it. The core size limit does not reliably prevent it. For a piped dump the kernel ignores RLIMIT_CORE except for the value 1, which aborts the dump. At 0, systemd-coredump declines to store the core file but still sends the journal entry, environment included. A crashing subprocess does not end the provider session, so the token in that entry stays valid until T3 releases, rotates or revokes the session.

The #12031 triage already asked for a non-env channel: item 1 proposes a 0600 file for Codex, and item 5 says that Claude, leaving argv, should not stop at a child env var on Linux coredump hosts. The same reasoning applies to ACP and Pi.

Change

ACP and Pi now get the Authorization header in an owner-only file. Only the file's path goes into the environment: T3_ACP_MCP_AUTHORIZATION_FILE for the ACP agent, its bridge and its client terminals, and T3_MCP_AUTHORIZATION_FILE for Pi. Both files hold the full header value (Bearer ...), hence the names. The old variables are gone as channels.

The files. makeMcpCredentialFiles in packages/provider-core/src/server/mcpSession.ts owns the files for one adapter session.

  • Each thread gets one path for the life of the provider session: a file in its own fresh mkdtemp directory (0700) under the OS temp dir, created with Effect's makeTempFileScoped.
  • A new header is written to <path>.tmp with mode 0600 and renamed over the file, so the path never goes missing and a reader never sees half a header. If the write or the rename fails, the .tmp is deleted.
  • Removing a thread's credential deletes the file and any .tmp, also after a failed first write. A later write puts it back at the same path.
  • Closing the session scope removes every file and its directory.
  • Writes and removals take one semaphore permit, so a file always holds the header recorded for it.

ACP syncs the file each time the adapter reads the thread's credential: on a turn, a resume, a snapshot read, a fork or a rollback. A rotated header is written over the file on that read, and the next read after a clear deletes it. An idle session keeps the file until that read or until the session closes; the token in it was already revoked by the clear. The adapter's credential reads for one session never interleave either. Every read after open holds the session's runtime transition lock, so an older header cannot overwrite a newer one.

The path has to stay put. The agent's environment, the bridge it registered and the terminal environments the adapter remembers are all fixed when the native session starts. When the credential rotates and the next turn runs on the same native session, the adapter rewrites the file those processes already name. Pi reads the registry once per openSession, so it uses the same helper with one key.

The readers.

  • t3 acp-mcp-call, the fallback an agent runs from a client terminal, reads the file on every call, so it follows rotation.
  • t3 acp-mcp-bridge and the Pi extension read it at start, as they read the env var before. "What this does not fix" covers what that means for the bridge after a rotation.
  • Without a readable file the bridge exits with acp-mcp-bridge requires T3_ACP_MCP_ENDPOINT and a readable T3_ACP_MCP_AUTHORIZATION_FILE. It no longer reads the old variable.
  • The in-process MCP-over-ACP bridge keeps the header in T3's memory, as before. It never reaches a child.

Old variables. A T3 server started from the terminal of an older build's provider child can carry a raw credential in its own environment. On main that copy reached ACP agents and their terminals even in sessions with no credential. LEGACY_RAW_MCP_CREDENTIAL_ENV in provider-core lists the three names older builds used (T3_ACP_MCP_AUTHORIZATION, T3_MCP_BEARER_TOKEN and Claude's T3_CODE_MCP_AUTHORIZATION), and withoutRawMcpCredentials drops all three in three places:

  • AcpSessionRuntime.make, where every ACP flavor spawns its agent (registry, Grok, Antigravity, and the registry probe and auth runtimes).
  • makeAcpClientTerminals.
  • Pi's launch environment, which also drops its own stale T3_MCP_URL and T3_MCP_AUTHORIZATION_FILE before it adds the current session's values.

This applies whether or not the session has a credential and whether or not the file write worked. When no environment is configured the helper starts from process.env, which Node would pass on anyway, so a terminal keeps the server's environment minus those names.

When the file cannot be written.

  • ACP logs Could not write the T3 Code MCP credential file. and opens the session without the t3-code MCP server and without the file variable. T3's tools are an addition to the session, and two ACP call sites have error channels that cannot take a new error.
  • Pi fails openSession through its existing provideCacheFs mapping, the same way it fails when it cannot write its extension file.

Where the file lives. The file is in a fresh private mkdtemp directory under the OS temp dir rather than secretsDir. Files left behind by a crashed server go when the OS cleans its temp dir instead of piling up in the T3 home. One directory per file needs no shared directory and no cleanup across sessions. Where the temp dir is tmpfs, the file is never written to a disk file system. The adapters own the files, not McpSessionRegistry: writing them from the registry would put every provider session's bearer on disk, including Codex, Claude, Cursor, OpenCode and Muse, which never read it.

Docs. docs/orchestration-v2/orchestrator-mcp-server.md shows T3_MCP_AUTHORIZATION_FILE in Pi's environment block and says commands Pi runs do not inherit the token, so it is not in their COREDUMP_ENVIRON. Its Codex section still described bearer_token_env_var and T3_MCP_BEARER_TOKEN, which main no longer uses. It now says Codex gets http_headers.Authorization in the config of thread/start, thread/resume and thread/fork over JSON-RPC.

Why a file and not a protocol channel

Channel Token in the env of the child and its subprocesses Token in COREDUMP_ENVIRON Same user can read the token Available for ACP stdio servers and Pi Secret file to manage
Env var (main) yes yes yes, from /proc/<pid>/environ yes no
Protocol or control channel (Codex JSON-RPC, Claude mcp_set_servers) no no from process memory only no, see below no
Owner-only file, path in env (this PR) no, path only no, path only yes, from the file, by design yes yes: one per thread, removed after a clear and on session close
  • ACP: a stdio entry in mcpServers is a process spec (command, args, env). It has no header field, so the bridge can only receive the header through its environment, its argv or a file.
  • Pi: --mode rpc has no command that hands an extension a value. The extension could request one with an extension_ui_request that the adapter answers over stdin, but that is new protocol on both sides, and the token would then travel in the RPC stream that native protocol logging records.

What this does not fix

  • Processes running as the same user, the agent's own commands included, can still read the file through the path in their environment. acp-mcp-call depends on that. The change takes the token out of every copy of the environment and off disk once the session closes, not out of the user's reach. The triage's 0600 file has the same property.
  • A t3 acp-mcp-bridge started before a rotation keeps the old header in memory, as on main (ProviderSessionManager.ts:445: rotating afterwards cannot repair an already-configured process). Its MCP HTTP session was opened under that header. acp-mcp-call follows the rotation.
  • Stale T3_ACP_MCP_ENDPOINT and T3_ACP_MCP_AUTHORIZATION_FILE inherited by the server pass through when a session has no credential. At the spawn point the instance environment and the session overlay are already merged, so an inherited path looks like the current one. They hold an endpoint and a path, not a token. The server's own environment is not touched either, so children of other providers still pass on any legacy variable the server inherited, as on main.
  • The token is still in the memory of the bridge, acp-mcp-call and the Pi extension, so a full core image can contain it if the core file itself is stored. COREDUMP_ENVIRON no longer contains it.
  • If the T3 server itself dies, scope finalizers do not run and the files stay in the OS temp dir until the OS cleans it. The tokens in them are dead, because McpSessionRegistry keeps credentials in memory only. The directories are 0700.
  • Revoking the credential when an agent subprocess crashes or the provider exits without a stream failure (MCP bearer token passed via environment is captured in systemd-coredump journal entries on provider crashes #12031 triage item 3). Main already releases the session on a provider event-stream failure (ProviderSessionManager.ts:1978).

Scope and approval

  • #12031 is triaged and labeled accepted. The maintainer triage confirmed the coredump exposure and asked for a non-env channel.
  • One problem: ACP and Pi pass the bearer through child environment variables. Both move to the same helper.
  • One commit on 477282263a, 20 files, +798 / -107, most of it tests.

Part of #12031.

Verification

Tests, red/green, mutation checks, typecheck, lint and format

Focused tests, run from the repo root:

vp test run apps/server/src/orchestration-v2/Adapters/AcpAdapterV2.test.ts apps/server/src/provider/acp/AcpJsonRpcConnection.test.ts apps/server/src/mcp/AcpMcpStdioBridge.test.ts packages/provider-acp/src/server/clientTerminals.test.ts packages/provider-pi/src/server/adapter.test.ts packages/provider-pi/src/server/mcpInjection.test.ts packages/provider-pi/src/server/mcpExtensionSource.test.ts packages/provider-core/src/server/mcpSession.test.ts
 Test Files  8 passed (8)
      Tests  308 passed (308)

New and changed tests:

  • keeps an ACP thread's MCP credential file at one path through rotation and clear: after a rotation and a turn on the same native session there is still one runtime, the same path holds the new header, and a terminal started afterwards reads it through its remembered environment. The next read after a clear removes the file, a reissue restores it at the same path, and closing the session removes it.
  • opens an ACP session without T3's MCP server when its credential file cannot be written: no t3-code server and no file variable, and the agent does not see a stale raw variable.
  • drops a raw MCP credential the server inherited from an older build: a raw value seeded in the parent process.env does not reach the spawned agent.
  • The rotation test also seeds a stale raw value in the agent's base environment for a Grok-like flavor. The native fork test seeds one in clientTerminals.environment; terminals of a session without a credential and of a session with one both print it empty.
  • reads the credential from the file the environment names for the bridge and acp-mcp-call.
  • hands Pi the MCP credential as a session-owned file, not in its environment: the file is 0600 and gone after close. The Pi launch test in mcpInjection.test.ts seeds all three legacy names plus a stale URL and file variable and asserts each is gone.
  • mcpSession.test.ts, 6 new tests. For makeMcpCredentialFiles on the real filesystem: an unchanged header is a no-op (same inode); rotation keeps the path and mode 0600 and replaces the inode; remove then write puts the file back at the same path; an injected rename failure leaves no .tmp, remove clears a key with no recorded header, and a failed rotation keeps the old header; closing the scope removes every key's directory. For withoutRawMcpCredentials: all three names masked and the rest kept; an unset environment starts from process.env.

Related suites, every test that builds AcpSessionRuntime since its spawn environment changed:

vp test run apps/server/src/orchestration-v2/Adapters/AcpRegistryAdapterV2.test.ts apps/server/src/orchestration-v2/Adapters/AntigravityAdapterV2.test.ts apps/server/src/orchestration-v2/Adapters/GrokAdapterV2.test.ts packages/provider-acp/src/server packages/provider-acp-registry/src/server apps/server/src/provider/acp apps/server/src/provider/AcpRegistryProbe.test.ts apps/server/src/provider/AntigravityAuth.test.ts apps/server/src/provider/AntigravityProvider.test.ts apps/server/src/textGeneration/AntigravityTextGeneration.test.ts
 Test Files  1 failed | 23 passed (24)
      Tests  1 failed | 429 passed (430)

vp test run apps/server/src/orchestration-v2/testkit/OrchestratorReplayFixtures.integration.test.ts packages/provider-pi/src/server packages/provider-grok/src/server packages/provider-core/src/server
 Test Files  35 passed | 1 skipped (36)
      Tests  579 passed | 4 skipped (583)

The one failure is AcpSessionRuntime.processTree.test.ts "contains a packaged-runtime command without requiring cgroup delegation" (expected 127 to be 125). This PR does not touch that test or the cgroup wrapper it exercises. Run alone on a8bdfdb5 on a host whose /bin/sh is bash, the file fails the same way (1 failed, 22 passed). The cause is the host's /bin/sh: bash skips the wrapper's EXIT trap after a failed exec, so a missing target exits 127 instead of 125. With dash bound over /bin/sh and nothing else changed, the same file passes 23 of 23 on a8bdfdb5. Upstream CI ran it green on a8bdfdb5 on Ubuntu, where /bin/sh is dash. #17949 fixes it separately, carrying forward #15344.

Red/green: with the new strip tests on the source from before the strip existed (file channel only), all four fail: the parent-seed test (the agent sees the raw variable), the rotation test (the agent sees the stale raw value, the Grok case), the write-failure test (same), and the fork test (the terminal of a session without a credential prints Bearer stale-dummy-credential|). I did not run the new tests against a8bdfdb5, where the *_AUTHORIZATION_FILE assertions would fail too.

Mutation checks, one per new behavior, each source file restored and checksum-verified afterwards. The first five ran before the strip helper moved to provider-core, when it was withoutRawMcpCredential in provider-acp at the same two call sites. The last five ran on the final code; the two that touch Pi's launch environment were rerun after rebasing onto 477282263a.

Mutation Result
Agent spawn without the strip helper 3 fail: the parent-seed, rotation and write-failure tests (the agent sees the raw variable)
Client terminals without the strip helper fork test fails: expected 'Bearer stale-dummy-credential|' to equal '|'
Rotation deletes the old file and writes a new path rotation test fails: NotFound: FileSystem.readFile on the path the agent and terminals were given
remove leaves the file on clear rotation test fails: expected true to be false on exists(file) after clear
ACP write failure raised instead of logged write-failure test fails: ProviderAdapterOpenSessionError ... PermissionDenied: FileSystem.makeTempFileScoped
A failed write keeps its .tmp "leaves no temporary file behind when a write fails" fails: expected true to be false on exists(<path>.tmp)
remove returns early when no header is recorded same test fails: the empty file still exists after remove
Pi deletes only T3_MCP_BEARER_TOKEN Pi launch test fails: expected 'Bearer stale-token' to equal undefined
The shared list loses T3_CODE_MCP_AUTHORIZATION 2 fail: the strip helper's strict-shape test and the Pi launch test
write drops the unchanged-header check "rewrites a key's file in place and skips an unchanged header" fails on the inode

The rename step and the semaphore have no mutation check, because no test races a reader or a second writer against a write.

  • tsc --noEmit through each package's typecheck script in provider-core, provider-acp, provider-pi and apps/server: exit 0, 0 errors.
  • vp lint --report-unused-disable-directives on the changed .ts files: 0 errors. 4 warnings, all on lines this PR does not change.
  • vp fmt --check on the changed files: All matched files use the correct format.

Manual checks

Child environments, crash dumps, the credential at work, and a live T3 thread

Main (a8bdfdb5) and this change on that base (36da67d1, before a conflict-only rebase onto 477282263a; the ACP and core code is byte-identical, and Pi's launch code differs only by upstream's new extension-path lines), each in a throwaway clone, under env -i with a private HOME and TMPDIR. A temporary test file that is not part of this PR drove the real code: makeAcpRegistryAdapterV2 spawning the repo's acp-mock-agent.ts, makeAcpAdapterV2 with mcpServers pointing at the clone's real t3 acp-mcp-bridge, and buildPiRpcLaunch. Each child's /proc/<pid>/environ was read and filtered to T3 MCP variables. The credential was a dummy, Bearer dummy-e1-00112233445566778899aabbccddeeff, and the MCP endpoint was a local dummy HTTP server that logs each request's Authorization header. Both clones were clean afterwards.

Child environments. ACP agent process (the T3_ACP_MCP_NODE and T3_ACP_MCP_ENTRYPOINT lines omitted):

# before (main)
T3_ACP_MCP_AUTHORIZATION=Bearer dummy-e1-00112233445566778899aabbccddeeff
T3_ACP_MCP_ENDPOINT=http://127.0.0.1:43123/mcp

# after (this branch)
T3_ACP_MCP_AUTHORIZATION_FILE=$TMPDIR/t3-mcp-XXXXXX/<random>
T3_ACP_MCP_ENDPOINT=http://127.0.0.1:43123/mcp

The bridge, the client terminal and the Pi child show the same change (T3_MCP_BEARER_TOKEN becomes T3_MCP_AUTHORIZATION_FILE for Pi). Two more runs put stale raw copies in the environment of the process standing in for the T3 server and opened a session with no credential:

# before, server env has T3_ACP_MCP_AUTHORIZATION: ACP agent and its client terminal
T3_ACP_MCP_AUTHORIZATION=Bearer dummy-stale-inherited

# before, server env has T3_MCP_BEARER_TOKEN and T3_CODE_MCP_AUTHORIZATION: ACP agent and its client terminal
T3_CODE_MCP_AUTHORIZATION=Bearer dummy-stale-inherited
T3_MCP_BEARER_TOKEN=dummy-stale-inherited

# after, both runs: ACP agent and its client terminal
<no T3 MCP variables>

T3 MCP variables in each child's environment, before and after

Crash dumps. For each captured environment, a dummy /usr/bin/sleep ran as a transient user unit with exactly those T3 MCP variables, was crashed with SIGSEGV, and COREDUMP_ENVIRON was filtered to T3 MCP lines. The dummy core files were deleted afterwards.

$ systemd-run --user --collect --unit=$UNIT -p LimitCORE=infinity -E T3_...=... /usr/bin/sleep 300
$ systemctl --user kill -s SEGV $UNIT
$ journalctl --user COREDUMP_USER_UNIT=$UNIT.service -o json --all --output-fields=COREDUMP_ENVIRON

# before, t3 acp-mcp-bridge environment
T3_ACP_MCP_AUTHORIZATION=Bearer dummy-e1-00112233445566778899aabbccddeeff
T3_ACP_MCP_ENDPOINT=http://127.0.0.1:33005/mcp

# after
T3_ACP_MCP_AUTHORIZATION_FILE=$TMPDIR/t3-mcp-XXXXXX/<random>
T3_ACP_MCP_ENDPOINT=http://127.0.0.1:46271/mcp

All four channels (agent, bridge, terminal, Pi) held the dummy bearer before and only the path after.

COREDUMP_ENVIRON after a SIGSEGV, before and after

The credential still works. On this branch, the bridge started from the exact mcpServers spec, acp-mcp-call from a client terminal, and the Pi extension (loaded with Pi's environment, real fs and real fetch) each sent the issued header. After a rotation the file at the same path held the new header and acp-mcp-call sent it:

native session: mock-session-1; agent processes started: 1
file before rotation: Bearer dummy-... (matches dummy: true)
file after rotation:  Bearer dummy-... (matches rotated dummy: true)
new terminal's T3_ACP_MCP_AUTHORIZATION_FILE is the same path: true
acp-mcp-call stdout: {"content":[{"type":"text","text":"e1 probe ok"}]}
server received tools/call: Authorization: Bearer dummy-... (matches rotated dummy: true)

The file was 0600 in a 0700 directory, and both were gone once the owning scope closed. No t3-mcp-* directory was left in TMPDIR. The bridge given only the old raw variable exits with code 2.

The bridge, acp-mcp-call and the Pi extension authenticating from the file, rotation, file mode and cleanup

A live T3 thread. A dev server from this branch, loopback only, with an isolated T3 home and an ACP registry instance running codex-acp. One prompt asked the agent to call orchestrator_capabilities. The call went through t3 acp-mcp-bridge, completed, and the agent reported 10 providers. While the agent was alive I listed only the variable names in its environment and the bridge's. The codex-acp process had T3_ACP_MCP_AUTHORIZATION_FILE, T3_ACP_MCP_ENDPOINT, T3_ACP_MCP_ENTRYPOINT and T3_ACP_MCP_NODE. The bridge had T3_ACP_MCP_AUTHORIZATION_FILE and T3_ACP_MCP_ENDPOINT. Neither had T3_ACP_MCP_AUTHORIZATION.

ACP thread on this branch: orchestrator_capabilities completed

The orchestrator_capabilities result in the ACP thread

Not checked:

  • macOS and Windows. os.tmpdir() is per user on both and rename replaces an existing file on both, but I ran neither. On Windows, mode 0600 is not an owner-only ACL.
  • A real pi binary. The Pi environment came from buildPiRpcLaunch without starting Pi, and the extension ran in a vm context, as the repo's testkit does.
  • Real Grok, Antigravity or other registry agent binaries. The manual checks used the repo's mock agent, and the live thread used codex-acp through the registry driver.
  • A real crash of a real provider. The crash dumps came from a dummy sleep carrying each captured environment.
  • A T3 server crash, and a reader or second writer racing a write.

Made with Claude Opus 5.5 in T3 Code (Claude Code harness).

🤖 Generated with Claude Code

ACP agents, the `t3 acp-mcp-bridge` stdio server they spawn, ACP client
terminals and Pi received the thread's MCP bearer in an environment
variable. Everything those processes spawn inherits it, and on Linux
systemd-coredump writes the environment of a crashing process to the
journal as COREDUMP_ENVIRON.

ACP and Pi now write the Authorization header to an owner-only file in a
private temporary directory and pass only its path
(T3_ACP_MCP_AUTHORIZATION_FILE, T3_MCP_AUTHORIZATION_FILE). The bridge,
`t3 acp-mcp-call` and the Pi extension read the file.

ACP keeps one path per thread for the life of the provider session. A
rotated header is written beside the file and renamed over it. Once the
credential is cleared, the next read deletes the file, and a reissued
credential comes back at the same path. A failed write leaves no
temporary file. Pi writes its file once when the session opens. Closing
the session removes the files. If the file cannot be written, ACP runs
the session without T3's MCP server and Pi fails to open the session, as
it does when it cannot write its extension.

The raw-credential variables older builds exported
(T3_ACP_MCP_AUTHORIZATION, T3_MCP_BEARER_TOKEN, T3_CODE_MCP_AUTHORIZATION)
are dropped where every ACP flavor spawns its agent, where ACP client
terminals start, and from Pi's launch environment, whether or not the
session has a credential.

The MCP server doc now describes how Codex receives its header (JSON-RPC
thread config, not an environment variable).

Refs pingdotgg#12031

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@github-actions github-actions Bot added the vouch:unvouched PR author is not yet trusted in the VOUCHED list. label Oct 11, 2026
@github-actions github-actions Bot added the size:L 100-499 changed lines (additions + deletions). label Oct 11, 2026
@macroscopeapp

macroscopeapp Bot commented Oct 11, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Not approved

Macroscope's review found this PR not approvable — This PR changes the default authentication-credential transport for ACP and Pi processes, adding session-owned files, rotation and cleanup logic, and broad child-environment sanitization. Because it affects security-sensitive production behavior across multiple provider paths, human review is required.

You can add or adjust custom eligibility rules. Learn more.

knip flagged the export; it is only used inside mcpSession.ts.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Oct 11, 2026

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Path: .coderabbit.config.ts
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: af8d5b98-ce9e-4665-acb4-63d10bd94783

📥 Commits

Reviewing files that changed from the base of the PR and between e4ef8d3 and 0b0028b.


📒 Files selected for processing (20)
  • apps/server/scripts/acp-mock-agent.ts
  • apps/server/src/cli/acpMcpBridge.ts
  • apps/server/src/mcp/AcpMcpStdioBridge.test.ts
  • apps/server/src/mcp/AcpMcpStdioBridge.ts
  • apps/server/src/orchestration-v2/Adapters/AcpAdapterV2.test.ts
  • apps/server/src/provider/acp/AcpJsonRpcConnection.test.ts
  • docs/orchestration-v2/orchestrator-mcp-server.md
  • packages/provider-acp/src/server/AcpSessionRuntime.ts
  • packages/provider-acp/src/server/adapter.ts
  • packages/provider-acp/src/server/clientTerminals.ts
  • packages/provider-core/src/server/mcpSession.test.ts
  • packages/provider-core/src/server/mcpSession.ts
  • packages/provider-pi/src/server/adapter.test.ts
  • packages/provider-pi/src/server/adapter.ts
  • packages/provider-pi/src/server/mcpBridge.testkit.ts
  • packages/provider-pi/src/server/mcpExtensionSource.ts
  • packages/provider-pi/src/server/mcpInjection.test.ts
  • packages/provider-pi/src/server/mcpInjection.ts
  • packages/provider-pi/src/server/status.ts
  • packages/provider-pi/src/server/textGeneration.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 8 remain after this review.



📝 Walkthrough

Walkthrough

ACP and Pi now pass MCP authorization through scoped credential files instead of raw environment values. Provider environments mask legacy MCP credentials. ACP bridge handling, launch configuration, tests, and documentation reflect the file-based flow.

Changes

MCP credential-file flow

Layer / File(s) Summary
Credential files and environment sanitization
packages/provider-core/src/server/mcpSession.ts, packages/provider-core/src/server/mcpSession.test.ts
Provider core adds scoped credential-file management and masks three legacy raw MCP credential variables. Tests cover credential updates, removal, cleanup, permissions, and environment fallback.
ACP credential-file integration
packages/provider-acp/src/server/adapter.ts, packages/provider-acp/src/server/AcpSessionRuntime.ts, packages/provider-acp/src/server/clientTerminals.ts, apps/server/src/orchestration-v2/Adapters/AcpAdapterV2.test.ts, apps/server/src/provider/acp/AcpJsonRpcConnection.test.ts, docs/orchestration-v2/orchestrator-mcp-server.md
ACP session setup passes credential-file paths to MCP consumers and filters child and terminal environments. Tests cover session updates, forks, rollback replacement, and credential-file failures. The documentation describes Codex MCP configuration in JSON-RPC requests.
ACP bridge credential-file reader
apps/server/src/mcp/AcpMcpStdioBridge.ts, apps/server/src/mcp/AcpMcpStdioBridge.test.ts, apps/server/src/cli/acpMcpBridge.ts, apps/server/scripts/acp-mock-agent.ts
The bridge reads and trims authorization from the configured file. Tests check missing and unreadable credentials. The command documentation and mock-agent environment response reflect the file-based configuration.
Pi credential-file integration
packages/provider-pi/src/server/adapter.ts, packages/provider-pi/src/server/mcpInjection.ts, packages/provider-pi/src/server/mcpExtensionSource.ts, packages/provider-pi/src/server/status.ts, packages/provider-pi/src/server/textGeneration.ts, packages/provider-pi/src/server/*test*, docs/orchestration-v2/orchestrator-mcp-server.md
Pi launch configuration now carries an authorization-file path. The extension reads authorization from that file. Tests check that the token is absent from arguments and environment and that the file is removed after the scoped runtime ends. The documentation describes the new environment setting.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Bug fix

Sequence Diagram(s)

sequenceDiagram
  participant AcpAdapter
  participant makeMcpCredentialFiles
  participant AcpMcpStdioBridge
  participant CredentialFile
  AcpAdapter->>makeMcpCredentialFiles: Write authorization for thread
  makeMcpCredentialFiles-->>AcpAdapter: Return credential-file path
  AcpAdapter->>AcpMcpStdioBridge: Pass endpoint and credential-file path
  AcpMcpStdioBridge->>CredentialFile: Read and trim authorization
Loading
sequenceDiagram
  participant PiAdapter
  participant makeMcpCredentialFiles
  participant buildPiRpcLaunch
  participant PiExtension
  participant CredentialFile
  PiAdapter->>makeMcpCredentialFiles: Write session authorization
  makeMcpCredentialFiles-->>PiAdapter: Return credential-file path
  PiAdapter->>buildPiRpcLaunch: Pass endpoint and file path
  buildPiRpcLaunch->>PiExtension: Set endpoint and file-path environment
  PiExtension->>CredentialFile: Read and trim authorization
Loading

Suggested reviewers: juliusmarminge, stienswout


Merge Risk | ⚪ Minimal · up to 0b002

Merge Risk: ⚪ Minimal · up to 0b002

ACP and Pi now pass MCP credentials through private files rather than child environments, and inherited legacy raw credentials are removed. No concrete regression in credential delivery or environment isolation was found, so the change appears ready to merge.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 0b002

The change reduces credential exposure in child environments without demonstrating broader tool authority or an authorization bypass. Remaining uncertainty concerns platform permissions, interrupted cleanup, and credential refresh in running clients.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — A stolen live bearer reaches the capabilities attached to its environment, thread, and provider identity, including orchestration, worktree, pull-request, and optionally preview or device authority. Credential identity binding is evidenced; complete asset and data reach across every downstream tool handler was not independently established. This authority predates the credential-delivery change.

Security Findings and Attack Paths

  • inferred — The deferred environment-exposure candidate does not demonstrate newly reachable secret exposure. ACP preserves its default inherited baseline while masking legacy MCP credentials, and the inspected production caller selecting extendEnv=false supplies an explicit environment. The unresolved omitted-environment dependency case remains a coverage limitation, not a verified finding.

Trust Boundaries and Controls

  • inferred — Private files and raw-variable masking reduce accidental credential replication into descendant environments. They do not sandbox the provider or same-user descendants that can read the supplied path. This preserves an existing provider trust relationship rather than establishing a new isolation boundary. POSIX mode checks are evidenced; equivalent Windows access restrictions are not.

Resilience and Maintainability Implications

  • inferred — If stable-file deletion succeeds but subsequent temporary-file deletion fails or is interrupted, removal can leave the cached authorization unchanged. A later identical-header write could then return a missing path. Consumers reject missing files, and production clearing revokes the old credential before configuration removal; an exploitable authorization bypass or reachable same-header recovery failure was not established.

Hardening Proposals

  • proposed — Make cached authorization invalidation retry-safe across partial deletion and interruption, so future callers cannot mistake a remembered header for an existing credential file. Exercise those transitions explicitly.
  • proposed — Validate owner-only access under supported Windows temporary-directory ACLs and document abrupt-termination residue behavior before treating scoped cleanup and POSIX mode 0600 as equivalent cross-platform guarantees.

Pre-merge checks | Passed 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check Passed The title clearly and concisely identifies the main change: preventing MCP bearer credentials from entering ACP and Pi child environments.
Description check Passed The description is complete and structured with Problem, Change, Scope and approval, and Verification sections. It explains the security issue, implementation, approval reference, test results, manual…
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR

  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:L 100-499 changed lines (additions + deletions). vouch:unvouched PR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant