diff --git a/src/cli.ts b/src/cli.ts index 616baa2..9d5cf13 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -1473,6 +1473,7 @@ async function handleDeadSession( await spawnDaemon({ name: session.name, command: meta.command, args: meta.args, displayCommand: meta.displayCommand, cwd: meta.cwd, tags: meta.tags, ...(meta.displayName ? { displayName: meta.displayName } : {}), + scrubEnv: RESTART_SCRUBBED_ENV, }); console.log(`Session "${session.name}" restarted.`); doAttach(session.name); @@ -2999,6 +3000,18 @@ function statefulAgentReason(meta: SessionMetadata): string | null { return null; } +/** Bus-identity env vars stripped from an operator-initiated restart's re-exec. + * `pty restart` (and the dead-session "Restart? [Y/n]" path) re-run a stored + * command under the RESTARTER's shell env. If that shell belongs to a different + * convoy agent, its ST_AGENT/ST_ROOT leak into the re-exec'd session and it + * comes back under the wrong bus identity — the cos-restart incident, where + * restarting cos from smalltalk's shell brought cos back as smalltalk-claude + * (exit 129). Scrubbing them means a restarted session never inherits the + * restarter's identity; the blessed way to restart an agent with a correct + * identity is its supervisor (convoy). Fresh `pty run` is unaffected — a + * convoy-launched create legitimately inherits its own identity. */ +const RESTART_SCRUBBED_ENV = ["ST_AGENT", "ST_ROOT"]; + async function cmdRestart( name: string, yes = false, @@ -3062,6 +3075,7 @@ async function cmdRestart( await spawnDaemon({ name, command: meta.command, args: meta.args, displayCommand: meta.displayCommand, cwd: meta.cwd, tags: restartTags, ...(meta.displayName ? { displayName: meta.displayName } : {}), + scrubEnv: RESTART_SCRUBBED_ENV, }); console.log(`Session "${name}" restarted.`); diff --git a/src/spawn.ts b/src/spawn.ts index efee970..a9de599 100644 --- a/src/spawn.ts +++ b/src/spawn.ts @@ -84,6 +84,15 @@ export interface SpawnDaemonOptions { * handler still surfaces immediate failures within milliseconds, so * this only governs the "alive but slow" case. */ startTimeoutMs?: number; + /** Env var names to DELETE from the daemon's inherited environment before it + * spawns — and therefore before the session child inherits it. Used by the + * operator-initiated restart paths to strip the *restarter's* ambient + * bus-identity vars (ST_AGENT/ST_ROOT), so a session re-exec'd from a + * different shell can't come back under that shell's identity. See the + * cos-restart incident. Applied on the spawnViaNode path; the CLI-fallback + * path can't express it (a bundled consumer that hits the fallback also + * isn't the operator-restart context this guards). */ + scrubEnv?: string[]; } /** Default time we wait for a daemon's Unix socket to appear after @@ -151,6 +160,11 @@ async function spawnViaNode(options: SpawnDaemonOptions, serverModule: string): // when its spawner is gone. Off by default — opt in via // `bindToSpawnerLifetime` when the caller owns the daemon's lifetime. const env: Record = { ...process.env, PTY_SERVER_CONFIG: config }; + // Strip caller-requested vars (e.g. the restarter's leaked bus identity) + // before the daemon — and thus the session child — can inherit them. + if (options.scrubEnv) { + for (const key of options.scrubEnv) delete env[key]; + } if (options.bindToSpawnerLifetime) env.PTY_SPAWNER_PID = String(process.pid); const child = spawn(launcherCmd, [...launcherArgs, serverModule], { detached: true, diff --git a/tests/restart-env-scrub.test.ts b/tests/restart-env-scrub.test.ts new file mode 100644 index 0000000..4ec0644 --- /dev/null +++ b/tests/restart-env-scrub.test.ts @@ -0,0 +1,91 @@ +import { describe, it, expect, afterAll } from "vitest"; +import * as fs from "node:fs"; +import * as os from "node:os"; +import * as path from "node:path"; +import { fileURLToPath } from "node:url"; +import { spawnSync } from "node:child_process"; + +const __dirname = path.dirname(fileURLToPath(import.meta.url)); +const nodeBin = process.execPath; +const cliPath = path.join(__dirname, "..", "dist", "cli.js"); +const testRoot = fs.mkdtempSync(path.join(os.tmpdir(), "pty-envscrub-")); +const bgPids: number[] = []; +afterAll(() => { + for (const pid of bgPids) { try { process.kill(pid, "SIGKILL"); } catch {} } + fs.rmSync(testRoot, { recursive: true, force: true, maxRetries: 3, retryDelay: 100 }); +}); + +interface RunOpts { env?: Record; unset?: string[]; timeout?: number } + +// PTY_SESSION set => restart takes its "already inside a session, not attaching" +// branch and returns instead of hanging on a non-TTY attach. +function runCli(dir: string, args: string[], opts: RunOpts = {}) { + const env: Record = { + ...(process.env as Record), + PTY_SESSION_DIR: dir, PTY_ROOT_LEGACY_SILENT: "1", PTY_SESSION: "outer", + ...(opts.env ?? {}), + }; + for (const k of opts.unset ?? []) delete env[k]; + return spawnSync(nodeBin, [cliPath, ...args], { env, encoding: "utf8", timeout: opts.timeout ?? 15000 }); +} + +/** A command that records the ST_AGENT/ST_ROOT it was actually launched with, + * then stays alive. `-` default => "UNSET" when the var is absent. Re-runs on + * every (re)start, so the file always reflects the current child's env. */ +function recorderCmd(outFile: string): string[] { + return ["sh", "-c", `printf '%s|%s' "\${ST_AGENT-UNSET}" "\${ST_ROOT-UNSET}" > "${outFile}"; exec sleep 300`]; +} + +function createSession(dir: string, name: string, outFile: string, opts: RunOpts): void { + const r = runCli(dir, ["run", "-d", "--id", name, "--", ...recorderCmd(outFile)], opts); + expect(r.status).toBe(0); + try { + bgPids.push(Number(fs.readFileSync(path.join(dir, `${name}.pid`), "utf8").trim())); + } catch {} +} + +const sleepSync = (ms: number) => Atomics.wait(new Int32Array(new SharedArrayBuffer(4)), 0, 0, ms); + +function waitForContent(p: string, timeoutMs = 4000): string { + const start = Date.now(); + while (Date.now() - start < timeoutMs) { + try { + const s = fs.readFileSync(p, "utf8"); + if (s.length > 0) return s; + } catch {} + sleepSync(50); + } + try { return fs.readFileSync(p, "utf8"); } catch { return ""; } +} + +describe("restart scrubs the restarter's bus-identity env", () => { + it("does NOT leak the restarter's ST_AGENT/ST_ROOT into the re-exec'd session", () => { + const dir = fs.mkdtempSync(path.join(testRoot, "d-")); + const outFile = path.join(dir, "child.env"); + // Create with no ambient identity so the first launch records UNSET|UNSET. + createSession(dir, "s", outFile, { unset: ["ST_AGENT", "ST_ROOT"] }); + expect(waitForContent(outFile)).toBe("UNSET|UNSET"); + + // Restart from a DIFFERENT agent's shell: its identity must not leak in. + fs.rmSync(outFile, { force: true }); + const r = runCli(dir, ["restart", "-y", "s"], { + env: { ST_AGENT: "smalltalk-claude", ST_ROOT: "/leaked/convoy" }, + }); + expect(r.status).toBe(0); + expect(r.stdout).toContain("restarted"); + + const recorded = waitForContent(outFile); + expect(recorded).toBe("UNSET|UNSET"); + expect(recorded).not.toContain("smalltalk-claude"); + expect(recorded).not.toContain("/leaked/convoy"); + }, 25000); + + it("still inherits the creator's ST_AGENT on a fresh `pty run` (create path unaffected)", () => { + const dir = fs.mkdtempSync(path.join(testRoot, "d-")); + const outFile = path.join(dir, "child.env"); + createSession(dir, "fresh", outFile, { + env: { ST_AGENT: "creator-abc", ST_ROOT: "/creator/convoy" }, + }); + expect(waitForContent(outFile)).toBe("creator-abc|/creator/convoy"); + }, 20000); +});