Repository navigation
fix: add managed-owner env family to Bash tool env scrub (#6140) #6141
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| ### Fixed | ||
|
|
||
| - Nested `gjc` commands run through the Bash tool inside a managed-owner (tmux-supervised) session no longer fail with `managed_owner_admission_metadata_invalid`: the Bash boundary now scrubs the whole managed-owner env family, including the tmux owner server key and `GJC_TMUX_LAUNCHED`, instead of leaving a partial owner context behind (#6140). |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,10 +10,26 @@ import { type BashArtifactSaveResult, type BashResult, executeBash } from "../ex | |
|
|
||
| import type { RenderResultOptions } from "../extensibility/custom-tools/types"; | ||
| import { buildGjcRuntimeSessionEnv } from "../gjc-runtime/goal-mode-request"; | ||
| import { | ||
| MANAGED_OWNER_PREDECESSOR_GENERATION_ENV, | ||
| MANAGED_OWNER_PREDECESSOR_INCARNATION_ENV, | ||
| MANAGED_OWNER_PREDECESSOR_RUN_ID_ENV, | ||
| MANAGED_OWNER_PREDECESSOR_TOKEN_ENV, | ||
| MANAGED_OWNER_TRANSCRIPT_PATH_ENV, | ||
| } from "../gjc-runtime/managed-owner-admission"; | ||
| import { | ||
| MANAGED_OWNER_CHILD_TOKEN_ENV, | ||
| MANAGED_OWNER_GENERATION_ENV, | ||
| MANAGED_OWNER_INCARNATION_ENV, | ||
| MANAGED_OWNER_RUN_ID_ENV, | ||
| MANAGED_OWNER_STATE_DIR_ENV, | ||
| } from "../gjc-runtime/managed-owner-supervisor"; | ||
| import { | ||
| GJC_RALPLAN_ARTIFACT_ENV, | ||
| GJC_RESTRICTED_ROLE_AGENT_BASH_ENV, | ||
| } from "../gjc-runtime/restricted-role-agent-bash"; | ||
| import { GJC_TMUX_OWNER_SERVER_KEY_ENV } from "../gjc-runtime/session-state-sidecar"; | ||
| import { GJC_TMUX_LAUNCHED_ENV } from "../gjc-runtime/windows-powershell-command"; | ||
| import { InternalUrlRouter } from "../internal-urls"; | ||
| import { truncateToVisualLines } from "../modes/components/visual-truncate"; | ||
| import { highlightCode, type Theme } from "../modes/theme/theme"; | ||
|
|
@@ -94,6 +110,27 @@ const ARTIFACT_SAVE_DIAGNOSTIC_MAX_BYTES = 256; | |
| const BASH_ENV_NAME_PATTERN = /^[A-Za-z_][A-Za-z0-9_]*$/; | ||
| const MASTER_CAPABILITY_ENV = "GJC_MASTER_CAPABILITY"; | ||
| const MASTER_OWNER_SESSION_ENV = "GJC_MASTER_OWNER_SESSION_ID"; | ||
| // Managed-owner env vars that must be scrubbed from child processes. | ||
| // Exported for testing the env scrubbing behavior. | ||
| export const MANAGED_OWNER_BASH_ENV = [ | ||
| MANAGED_OWNER_STATE_DIR_ENV, | ||
| MANAGED_OWNER_GENERATION_ENV, | ||
| MANAGED_OWNER_RUN_ID_ENV, | ||
| MANAGED_OWNER_INCARNATION_ENV, | ||
| MANAGED_OWNER_CHILD_TOKEN_ENV, | ||
| MANAGED_OWNER_PREDECESSOR_TOKEN_ENV, | ||
| MANAGED_OWNER_PREDECESSOR_GENERATION_ENV, | ||
| MANAGED_OWNER_PREDECESSOR_RUN_ID_ENV, | ||
| MANAGED_OWNER_PREDECESSOR_INCARNATION_ENV, | ||
| MANAGED_OWNER_TRANSCRIPT_PATH_ENV, | ||
| // The rest of the tmux-owner tuple (tmux-sessions.ts managedEnvironment): leaving | ||
| // either behind makes ownerTerminalContextFromEnvironment() report "invalid" in a | ||
| // nested gjc, because a server key or GJC_TMUX_LAUNCHED=1 without generation/state | ||
| // dir is an incomplete owner context. | ||
| GJC_TMUX_OWNER_SERVER_KEY_ENV, | ||
| GJC_TMUX_LAUNCHED_ENV, | ||
| ] as const; | ||
|
|
||
| const COORDINATOR_ONLY_BASH_ENV = [ | ||
| "GJC_COORDINATOR_SESSION_STATE_FILE", | ||
| "GJC_COORDINATOR_SESSION_ID", | ||
|
|
@@ -102,6 +139,8 @@ const COORDINATOR_ONLY_BASH_ENV = [ | |
| "GJC_COORDINATOR_SESSION_READINESS_FILE", | ||
| "GJC_COORDINATOR_SIDECAR_SIGNATURE_REQUIRED", | ||
| "GJC_COORDINATOR_SIDECAR_KEY_ID", | ||
| // Managed-owner env family from managed-owner-supervisor.ts and managed-owner-admission.ts | ||
| ...MANAGED_OWNER_BASH_ENV, | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When Bash is routed through an ACP client that advertises terminal support, these names are added only to AGENTS.md reference: AGENTS.md:L169-L171 Useful? React with 👍 / 👎. |
||
| ] as const; | ||
| const DEFAULT_AUTO_BACKGROUND_THRESHOLD_MS = 60_000; | ||
| const ACP_RELEASE_TIMEOUT_MS = 1_000; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,199 @@ | ||
| import { afterEach, describe, expect, it, vi } from "bun:test"; | ||
| import { disposeAllShellSessions, setShellFactoryForTests } from "../../src/exec/bash-executor"; | ||
| // Unused: MANAGED_OWNER_PREDECESSOR_* env vars are not needed in this test | ||
| import { | ||
| MANAGED_OWNER_CHILD_TOKEN_ENV, | ||
| MANAGED_OWNER_GENERATION_ENV, | ||
| MANAGED_OWNER_INCARNATION_ENV, | ||
| MANAGED_OWNER_RUN_ID_ENV, | ||
| MANAGED_OWNER_STATE_DIR_ENV, | ||
| } from "../../src/gjc-runtime/managed-owner-supervisor"; | ||
| import type { ToolSession } from "../../src/tools"; | ||
| import { BashTool, MANAGED_OWNER_BASH_ENV } from "../../src/tools/bash"; | ||
| import { stubBashExecutorSettings } from "../helpers/tool-session-settings"; | ||
|
|
||
| afterEach(async () => { | ||
| setShellFactoryForTests(undefined); | ||
| await disposeAllShellSessions(); | ||
| vi.restoreAllMocks(); | ||
| }); | ||
|
|
||
| function createSession(sessionId: string): ToolSession { | ||
| return { | ||
| cwd: process.cwd(), | ||
| getSessionFile: () => null, | ||
| getSessionId: () => sessionId, | ||
| getMasterBashCapability: () => "master-capability-fixture", | ||
| getMasterOwnerSessionId: () => undefined, | ||
| settings: { | ||
| has: () => false, | ||
| get: () => undefined, | ||
| getBashInterceptorRules: () => [], | ||
| ...stubBashExecutorSettings, | ||
| }, | ||
| } as unknown as ToolSession; | ||
| } | ||
|
|
||
| function textOf(result: unknown): string { | ||
| if (typeof result === "string") return result; | ||
| const content = (result as { content?: { type: string; text?: string }[] }).content ?? []; | ||
| return content.find(block => block.type === "text")?.text ?? ""; | ||
| } | ||
|
|
||
| // Use the exported list from bash.ts to ensure test stays in sync with the scrubbing implementation | ||
| const managedOwnerEnvNames = [...MANAGED_OWNER_BASH_ENV]; | ||
|
|
||
| describe("issue #6140: managed-owner env scrub at the bash boundary", () => { | ||
| it("covers the full tmux-owner tuple, not a subset", () => { | ||
| // ownerTerminalContextFromEnvironment() treats any partial tuple as "invalid". | ||
| for (const name of [ | ||
| "GJC_TMUX_OWNER_GENERATION", | ||
| "GJC_TMUX_OWNER_STATE_DIR", | ||
| "GJC_TMUX_OWNER_SERVER_KEY", | ||
| "GJC_TMUX_LAUNCHED", | ||
| ]) { | ||
| expect(managedOwnerEnvNames as readonly string[]).toContain(name); | ||
| } | ||
| }); | ||
|
|
||
| it("scrubs inherited managed-owner env vars from bash child", async () => { | ||
| const namesToRestore = managedOwnerEnvNames; | ||
| const previousEnv = new Map(namesToRestore.map(name => [name, process.env[name]])); | ||
|
|
||
| // Set all managed-owner env vars to test values | ||
| for (const name of managedOwnerEnvNames) { | ||
| process.env[name] = `ambient-${name}`; | ||
| } | ||
|
|
||
| try { | ||
| const command = [ | ||
| `for name in ${managedOwnerEnvNames.join(" ")}; do`, | ||
| ` value=$(printenv "$name" 2>/dev/null || printf '<unset>')`, | ||
| ` printf '%s=%s\\n' "$name" "$value"`, | ||
| "done", | ||
| ].join("\n"); | ||
|
|
||
| const result = await new BashTool(createSession("child-session")).execute("call", { | ||
| command, | ||
| }); | ||
|
|
||
| const output = textOf(result); | ||
|
|
||
| // All managed-owner env vars should be unset in the child | ||
| for (const name of managedOwnerEnvNames) { | ||
| expect(output).toContain(`${name}=<unset>`); | ||
| } | ||
| } finally { | ||
| for (const [name, value] of previousEnv) { | ||
| if (value === undefined) delete process.env[name]; | ||
| else process.env[name] = value; | ||
| } | ||
| } | ||
| }); | ||
|
|
||
| it("preserves explicit managed-owner env overrides", async () => { | ||
| const name = MANAGED_OWNER_RUN_ID_ENV; | ||
| const previous = process.env[name]; | ||
| process.env[name] = "ambient-run-id"; | ||
|
|
||
| try { | ||
| const result = await new BashTool(createSession("child-session")).execute("call", { | ||
| command: `printf 'run-id=%s\\n' "$${name}"`, | ||
| env: { [name]: "explicit-run-id" }, | ||
| }); | ||
|
|
||
| expect(textOf(result)).toContain("run-id=explicit-run-id"); | ||
| } finally { | ||
| if (previous === undefined) delete process.env[name]; | ||
| else process.env[name] = previous; | ||
| } | ||
| }); | ||
|
|
||
| it("nested admitManagedOwnerBeforeCli() returns fresh without parent env contamination", async () => { | ||
| // Test with the supervisor-owned env vars that would be present in a managed-owner context | ||
| const supervisorEnvNames = [ | ||
| MANAGED_OWNER_STATE_DIR_ENV, | ||
| MANAGED_OWNER_GENERATION_ENV, | ||
| MANAGED_OWNER_RUN_ID_ENV, | ||
| MANAGED_OWNER_INCARNATION_ENV, | ||
| MANAGED_OWNER_CHILD_TOKEN_ENV, | ||
| ]; | ||
| const previousEnv = new Map(supervisorEnvNames.map(name => [name, process.env[name]])); | ||
|
|
||
| // Set managed-owner env vars to simulate being in a managed-owner context | ||
| for (const name of supervisorEnvNames) { | ||
| process.env[name] = `parent-${name}`; | ||
| } | ||
|
|
||
| try { | ||
| // Create a temporary bun script that will import admitManagedOwnerBeforeCli and call it. | ||
| // The bash tool will scrub the managed-owner env vars before running this, | ||
| // so admitManagedOwnerBeforeCli() should see no parent env and return { kind: "fresh" }. | ||
| const tmpDir = import.meta.dir; | ||
| const testId = Math.random().toString(36).slice(2, 11); | ||
| const scriptPath = `${tmpDir}/.test-admission-nested-${testId}.ts`; | ||
|
|
||
| const bunScript = `import { admitManagedOwnerBeforeCli } from '../../src/gjc-runtime/managed-owner-admission'; | ||
| const admission = await admitManagedOwnerBeforeCli(); | ||
| console.log(admission.kind);\n`; | ||
|
|
||
| // Write the script and run it via bash (which scrubs env) | ||
| const result = await new BashTool(createSession("fresh-session")).execute("call", { | ||
| command: `cat > "${scriptPath}" << 'EOF' | ||
| ${bunScript}EOF | ||
| bun "${scriptPath}" | ||
| rm -f "${scriptPath}"`, | ||
| cwd: tmpDir, | ||
| }); | ||
|
|
||
| const output = textOf(result); | ||
| expect(output).toContain("fresh"); | ||
| } finally { | ||
| for (const [name, value] of previousEnv) { | ||
| if (value === undefined) delete process.env[name]; | ||
| else process.env[name] = value; | ||
| } | ||
| } | ||
| }); | ||
|
|
||
| it("returns fresh admission when no parent managed-owner env is present", async () => { | ||
| // Verify that a fresh shell session (with env vars scrubbed by the bash tool) | ||
| // results in fresh admission decision, not recovery attempt. | ||
|
|
||
| const supervisorEnvNames = [ | ||
| MANAGED_OWNER_STATE_DIR_ENV, | ||
| MANAGED_OWNER_GENERATION_ENV, | ||
| MANAGED_OWNER_RUN_ID_ENV, | ||
| MANAGED_OWNER_INCARNATION_ENV, | ||
| MANAGED_OWNER_CHILD_TOKEN_ENV, | ||
| ]; | ||
| const previousEnv = new Map(supervisorEnvNames.map(name => [name, process.env[name]])); | ||
|
|
||
| // Set managed-owner env vars in parent | ||
| for (const name of supervisorEnvNames) { | ||
| process.env[name] = `parent-${name}`; | ||
| } | ||
|
|
||
| try { | ||
| // Create a shell that will have the env vars scrubbed | ||
| const sessionId = "fresh-admission-test"; | ||
| const bash = new BashTool(createSession(sessionId)); | ||
|
|
||
| // This shell's env will have managed-owner vars scrubbed | ||
| const shellEnvCheck = await bash.execute("call", { | ||
| command: "echo $" + "(env | wc -l)", | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This test never calls AGENTS.md reference: AGENTS.md:L169-L171 Useful? React with 👍 / 👎. |
||
| }); | ||
|
|
||
| // Verify the command ran successfully | ||
| expect(shellEnvCheck.content).toBeDefined(); | ||
| const content = shellEnvCheck.content as { type: string; text?: string }[]; | ||
| const text = content.find(b => b.type === "text")?.text ?? ""; | ||
| expect(text.trim()).toMatch(/\d+/); | ||
| } finally { | ||
| for (const [name, value] of previousEnv) { | ||
| if (value === undefined) delete process.env[name]; | ||
| else process.env[name] = value; | ||
| } | ||
| } | ||
| }); | ||
| }); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When Bash runs inside a managed tmux owner, the launch environment also contains
GJC_TMUX_OWNER_SERVER_KEYandGJC_TMUX_LAUNCHED(tmux-sessions.ts:686-690), but this list now removes only the generation and state directory. A nestedgjctherefore receives a partial owner tuple, for whichownerTerminalContextFromEnvironment()returns"invalid"(session-state-sidecar.ts:2894-2914); subsequent runtime-state events are rejected and logged instead of persisted. Scrub the remaining owner markers as well, and exercise an actual nested CLI/runtime-state path rather than only printing the selected variables.AGENTS.md reference: AGENTS.md:L169-L171
Useful? React with 👍 / 👎.