From 0d9096ad83049b1c420a697c6c04ff10801efe4c Mon Sep 17 00:00:00 2001 From: Ian Rowe <87292868+ianrowe12@users.noreply.github.com> Date: Wed, 28 Oct 2026 14:34:27 -0400 Subject: [PATCH] fix(acp): Devin terminals no longer inherit the launching thread's T3 MCP credentials ACP client terminals built their environment from the server's own environment, then layered the session's T3_ACP_MCP_* variables on top. When the T3 server was itself started from an agent's shell inside T3 Code, its environment already held that outer thread's MCP endpoint and token, so a terminal for a session with no environment of its own ran with them and T3 tool calls from it landed on the outer thread. Drop T3_ACP_MCP_* from the inherited environment so those variables only ever come from the session or the request. On Windows, merge the inherited, session and request layers by case-insensitive name so a later layer's `Path` replaces an inherited `PATH`; Node would otherwise pass only one spelling, chosen by sort order. Co-authored-by: Claude Opus 5.5 --- .../src/server/clientTerminals.test.ts | 36 +++++++++++++--- .../src/server/clientTerminals.ts | 43 ++++++++++++++----- 2 files changed, 61 insertions(+), 18 deletions(-) diff --git a/packages/provider-acp/src/server/clientTerminals.test.ts b/packages/provider-acp/src/server/clientTerminals.test.ts index 791bbc22a6a03..49cc9c05a9558 100644 --- a/packages/provider-acp/src/server/clientTerminals.test.ts +++ b/packages/provider-acp/src/server/clientTerminals.test.ts @@ -55,26 +55,48 @@ describe("AcpClientTerminals", () => { Effect.gen(function* () { const terminals = yield* makeAcpClientTerminals({ defaultCwd: process.cwd(), - environmentForSession: (sessionId) => ({ - T3_TEST_ACP_SESSION_TOKEN: sessionId === "session-a" ? "token-a" : "token-b", - }), + environmentForSession: (sessionId) => + sessionId === "unknown" + ? undefined + : { T3_ACP_MCP_AUTHORIZATION: sessionId === "session-a" ? "token-a" : "token-b" }, }); yield* Effect.addFinalizer(() => terminals.disposeAll); for (const [sessionId, expected] of [ - ["session-a", "token-a"], - ["session-b", "token-b"], + ["session-a", "token-a||kept|T3_TEST_OVERRIDE=request"], + ["session-b", "token-b||kept|T3_TEST_OVERRIDE=request"], + ["unknown", "||kept|T3_TEST_OVERRIDE=request"], ] as const) { const created = yield* terminals.create({ sessionId, command: process.execPath, - args: ["-e", "process.stdout.write(process.env.T3_TEST_ACP_SESSION_TOKEN ?? '')"], + args: [ + "-e", + "const env = process.env; process.stdout.write([env.T3_ACP_MCP_AUTHORIZATION ?? '', env.t3_Acp_Mcp_Endpoint ?? '', env.T3_TEST_INHERITED, ...Object.keys(env).filter((name) => name.toUpperCase() === 'T3_TEST_OVERRIDE').map((name) => name + '=' + env[name])].join('|'))", + ], + env: [ + { name: "T3_TEST_OVERRIDE", value: "first" }, + { name: "t3_Test_Override", value: "second" }, + { name: "T3_TEST_OVERRIDE", value: "request" }, + ], }); yield* terminals.waitForExit({ sessionId, terminalId: created.terminalId }); const output = yield* terminals.output({ sessionId, terminalId: created.terminalId }); expect(output.output).toBe(expected); } - }).pipe(Effect.provide(NodeServices.layer), Effect.scoped), + }).pipe( + Effect.provideService(HostProcess.Environment, { + ...process.env, + T3_ACP_MCP_AUTHORIZATION: "launching-thread-token", + t3_Acp_Mcp_Endpoint: "launching-thread-endpoint", + T3_TEST_INHERITED: "kept", + T3_TEST_OVERRIDE: "inherited", + }), + // Exercise Windows' case-insensitive variable names on every OS. + Effect.provideService(HostProcess.Platform, "win32"), + Effect.provide(NodeServices.layer), + Effect.scoped, + ), ); it.live("runs Devin shell source with arguments, pipes, and session fallback variables", () => diff --git a/packages/provider-acp/src/server/clientTerminals.ts b/packages/provider-acp/src/server/clientTerminals.ts index 2c3278d3c144d..eb789f02ba8c9 100644 --- a/packages/provider-acp/src/server/clientTerminals.ts +++ b/packages/provider-acp/src/server/clientTerminals.ts @@ -11,6 +11,7 @@ import * as ChildProcessSpawner from "effect/process/ChildProcessSpawner"; import * as EffectAcpErrors from "effect-acp/errors"; import type * as EffectAcpSchema from "effect-acp/compat"; import * as NodeBuffer from "node:buffer"; +import * as HostProcess from "@t3tools/shared/HostProcess"; const MAX_LIVE_TERMINALS = 16; const MAX_UNRELEASED_TERMINALS = 64; @@ -135,22 +136,46 @@ function acpTerminalCommand(input: { readonly request: EffectAcpSchema.CreateTerminalRequest; readonly shellCommands?: boolean | undefined; readonly defaultCwd: string; - readonly environment?: NodeJS.ProcessEnv | undefined; + readonly environment: NodeJS.ProcessEnv; readonly forceKillAfter: Duration.Input; }) { return ChildProcess.make(input.request.command, input.request.args ?? [], { cwd: input.request.cwd ?? input.defaultCwd, - ...(input.environment === undefined ? {} : { env: input.environment }), + env: input.environment, shell: input.shellCommands === true && (input.request.args?.length ?? 0) === 0, forceKillAfter: input.forceKillAfter, }); } +// Windows variable names are case-insensitive and Node passes only one spelling to the child, +// so a later layer's `Path` must replace an earlier `PATH` rather than sit beside it. +function mergeEnvironments( + isWindows: boolean, + layers: ReadonlyArray>, +) { + const variables = new Map(); + for (const layer of layers) { + for (const [name, value] of layer) { + variables.set(isWindows ? name.toUpperCase() : name, [name, value]); + } + } + return Object.fromEntries(variables.values()); +} + export const makeAcpClientTerminals = Effect.fn("makeAcpClientTerminals")(function* ( options: AcpClientTerminalsOptions, ): Effect.fn.Return { const spawner = yield* ChildProcessSpawner.ChildProcessSpawner; const forceKillAfter = options.forceKillAfter ?? "5 seconds"; + const isWindows = yield* HostProcess.isWindows; + // T3 MCP credentials come from environmentForSession; inherited ones belong to whichever + // thread launched this server, so a terminal must never fall back to them. Windows treats + // variable names case-insensitively, so match the prefix regardless of case. + const inheritedEnvironment = Object.fromEntries( + Object.entries(options.environment ?? (yield* HostProcess.Environment)).filter( + ([name]) => !name.toUpperCase().startsWith("T3_ACP_MCP_"), + ), + ); const creationPermit = yield* Semaphore.make(1); const terminals = new Map(); let nextTerminalNumber = 1; @@ -213,16 +238,12 @@ export const makeAcpClientTerminals = Effect.fn("makeAcpClientTerminals")(functi `ACP terminal/create exceeded the limit of ${MAX_LIVE_TERMINALS} concurrent terminals.`, ); } - const requestEnvironment = Object.fromEntries( - (request.env ?? []).map((variable) => [variable.name, variable.value] as const), - ); const sessionEnvironment = options.environmentForSession?.(request.sessionId); - const environment = - options.environment === undefined && - sessionEnvironment === undefined && - (request.env?.length ?? 0) === 0 - ? undefined - : { ...options.environment, ...sessionEnvironment, ...requestEnvironment }; + const environment = mergeEnvironments(isWindows, [ + Object.entries(inheritedEnvironment), + Object.entries(sessionEnvironment ?? {}), + (request.env ?? []).map((variable) => [variable.name, variable.value] as const), + ]); const terminalScope = yield* Scope.make(); const child = yield* restore( spawner