diff --git a/CHANGELOG.md b/CHANGELOG.md index 03e4503..7086bd2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,34 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [0.2.3] - 2026-06-09 + +### Fixed + +- **Spawned agents now persist their conversation transcript when claudemux is + itself run from inside another Claude Code session.** A child `claude` that + inherited the parent's `CLAUDECODE` / `CLAUDE_CODE_*` / `AI_AGENT` environment + variables tripped claude's nested-session detection and wrote only an + `ai-title` record — no `user`/`assistant` turns — leaving claudemux (which + drives agents by *reading* the transcript) to see an empty conversation: + stalled gating, empty `messagesSince`, delivery never confirmed. claudemux now + scrubs those variables at the pane-launch boundary (`env -u … -- claude …`, a + true unset — `-e VAR=` only blanks, which still trips the heuristic, and the + shared persistent tmux server makes `process.env` mutation unreliable) so a + spawned agent is always a top-level session that owns its own transcript. The + existing `env` passthrough remains the escape hatch (a deliberate re-set wins). + Root-caused via A/B across claude 2.1.168/2.1.169 — not a version regression. + See [ADR 0008](docs/decisions/0008-scrub-parent-agent-env-for-transcript-persistence.md). + +### Security + +- **Validate `unsetEnv` names at the backend boundary** (`InvalidEnvVarName`). + Env-var names spliced into the `env -u` launch prefix must be POSIX + identifiers (`[A-Za-z_][A-Za-z0-9_]*`). Not consumer-exploitable today — the + sole producer is a trusted constant and the spawn path is shell-free + `execFile` argv — but it closes the seam against any future untrusted producer + (complete mediation at the boundary). ADR 0008 follow-up. + ## [0.2.2] - 2026-06-07 ### Added diff --git a/docs/decisions/0008-scrub-parent-agent-env-for-transcript-persistence.md b/docs/decisions/0008-scrub-parent-agent-env-for-transcript-persistence.md new file mode 100644 index 0000000..d4876b9 --- /dev/null +++ b/docs/decisions/0008-scrub-parent-agent-env-for-transcript-persistence.md @@ -0,0 +1,238 @@ +# 0008. Scrub the parent agent's env so the spawned `claude` persists its transcript + +**Status:** accepted +**Date:** 2026-06-09 + +## Context + +claudemux **drives** a spawned `claude` by reading the agent's on-disk transcript JSONL +(`~/.claude/projects//.jsonl`). `messagesSince`, `turnComplete`, and the gating that +sits on top of them all read that file (the `AgentDef.transcript.*` channel in `src/agents/claude.ts`). +A spawned agent whose transcript never lands on disk is, by claudemux's core invariant, **broken** — +the substrate has nothing to observe. + +We root-caused (A/B, today) a recurring "0 messages / gate stalls" failure to claude's own +**nested-session detection**. When the spawned `claude` *inherits* the parent Claude Code's environment +— `CLAUDECODE`, `CLAUDE_CODE_ENTRYPOINT`, `CLAUDE_CODE_SESSION_ID`, `CLAUDE_CODE_EXECPATH`, `AI_AGENT` +— claude concludes it is running *inside* another agent and **suppresses its own transcript +persistence**: it writes only an async `ai-title` record, with zero `user`/`assistant`/`system` records +on disk. The turn runs and renders in the pane, but claudemux's transcript reads come back empty. This +fires whenever claudemux is operated from inside a Claude Code session — interactive dev, CI under a +Claude runner, and nested agents — i.e. exactly the development and automation contexts the project +runs in. It is **not** a claude-version regression (2.1.168 and 2.1.169 behave identically) and **not** +a flush-timing race. + +The proven fix is to launch claude with those variables genuinely **unset** for the pane process. +A/B-validated: an identical spawn prefixed with +`env -u CLAUDECODE -u CLAUDE_CODE_ENTRYPOINT -u CLAUDE_CODE_SESSION_ID -u CLAUDE_CODE_EXECPATH -u AI_AGENT claude --session-id …` +restores the full transcript (10 records including user/assistant/system/bridge-session). Unsetting any +**single** variable did not restore persistence — the detection trips on any remaining signal, so the +**whole set** must go. + +Two mechanical realities constrain the fix: + +1. **The existing env channel is set-only.** `buildArgv` returns `env`, threaded via `spawn-boot.ts`'s + `mergedEnv` into `newSession` (`src/backends/tmux/sessions.ts`), which emits `tmux new-session -e + KEY=VAL`. tmux `-e` can **set** but never **unset**. Blanking with `-e VAR=` leaves the variable + *present but empty*; claude's heuristic presence-checks several of these, so a blank is unreliable. + A **true unset** is required. +2. **The tmux server is shared and persistent** (`/tmp/tmux-$UID/claudemux`, ADR 0006; `exec.ts` spawns + tmux with `env: process.env`). The *first* process to start the server bakes *its* `process.env` into + the server globally for the server's lifetime. So mutating `process.env` before spawn is a no-op when + the server was already started from a nested (CLAUDECODE=1) shell. The scrub must therefore act at the + **pane/exec layer** — on the command that launches the pane process — not via `process.env` and not + via a server-global. + +Product framing (decided upstream, designed within here): the scrub is **unconditional by default** — no +persona legitimately wants a suppressed-transcript agent. We add **no new flag** (`--keep-parent-env` was +rejected). The escape hatch is the *existing* `env`/`extraArgs` passthrough, the same shape as the +existing `hooks: false` opt-out and the "mechanism-not-policy / claudemux-owns-no-config" principles +(ADR 0006): a deliberate `create({ env: { CLAUDECODE: "1" } })` re-set must **win** over the scrub. + +By the T12 layering-grep rule, the claude-specific variable **list** and the decision to scrub belong in +`src/agents/claude.ts` — the only file permitted to know claude's vocabulary (verified: the +`SESSION_ID_FLAG`/transcript/hook vocabulary all live there, grep-enforced by +`scripts/layering-grep.sh`). + +## Decision + +1. **Add an explicit `unsetEnv?: string[]` to the agent `buildArgv` contract**, threaded + `buildArgv → SpawnBootInput → Backend.spawn → newSession`, and emitted by the tmux backend as a real + unset. `claude.ts` returns + `unsetEnv: ["CLAUDECODE", "CLAUDE_CODE_ENTRYPOINT", "CLAUDE_CODE_SESSION_ID", "CLAUDE_CODE_EXECPATH", "AI_AGENT"]` + on every fresh **and** resume/fork spawn (the suppression is path-independent). `cmd` stays `"claude"` + and `argv` stays `["--session-id", "", …]` — both unchanged. + +2. **The tmux backend emits the unset as an `env -u …` pane-command prefix**, not via `-e`. `newSession` + builds the pane command as + `env <-u NAME …per unsetEnv> -- ` while keeping the existing `-e KEY=VAL` flags for the + *set* env. Rationale, mirroring the existing `-e`-not-`set-environment` comment in `sessions.ts`: + `set-environment -u -t ` mutates the session's env *after* the pane process has already been + spawned, so it cannot affect that process; and `-e VAR=` cannot truly unset. Wrapping the launch + command in coreutils/BSD `env -u` is the only mechanism that produces a genuinely-absent variable in + the pane process, and it is immune to the baked-server trap because it acts at exec time on the pane + command itself, not on any server/global env. + +3. **Precedence: unset is applied BEFORE the explicit `env` merge wins.** The pane command must read as + "unset these, then set those," so a key named by *both* `unsetEnv` and the merged `env` ends up + **set** (the consumer's re-set wins). Concretely, the `-u NAME` for a key that also appears in the + `-e KEY=VAL` set is dropped at emission, so `env` never both unsets and re-sets the same name and the + re-set is unambiguous. `LC_ALL` (claude's existing set-env) is untouched. + +4. **The list lives only in `claude.ts`.** Neither the variable names nor `"env"`/`-u` semantics encode + any claude knowledge in `src/backends/**` — the backend receives an opaque `string[]` of names to + unset and knows nothing about why. This respects the layering grep: `claude.ts` owns the *what*, the + backend owns the *how* (the `env -u` mechanism), `unsetEnv` is the neutral seam between them. + +### Rejected: have `claude.ts` return a wrapped argv (`cmd: "env", argv: ["-u", …, "claude", …]`) + +This was the tempting one-file change. Rejected because it defeats invariants the substrate already +enforces and tests already pin: + +- **It breaks the documented `cmd: "claude"` expectation** and the argv shape. `create-identity.test.ts` + asserts `claude.buildArgv({ sessionId }).argv` deep-equals `["--session-id", id]` and that + `--session-id` and its value are **two adjacent argv elements** — a *security invariant* (every element + reaches the backend verbatim with no shell, so a value can never be re-parsed as a flag or, in tmux's + argv grammar, a second command). Folding `env -u … claude` into `argv` pushes the real command and its + flags deep into a positional list and shifts the `--session-id` adjacency the tests and the security + argument depend on. +- **It leaks the `env` launcher into the neutral identity contract.** `cmd` means "the agent binary." + Making it sometimes `"env"` forces every consumer of `buildArgv`'s return (and `adopt`/observability, + which surface `cmd`) to special-case a wrapper. `unsetEnv` keeps `cmd` honest. +- **It puts a process-launcher (`env`) where the agent should only describe itself.** The mechanism of + *how* a variable gets unset for a pane is a substrate/backend concern; `claude.ts` should declare the + *names*, not choose the launcher binary or its flag grammar. + +`unsetEnv` costs one optional field on a contract we already thread, and changes one `newSession` command +assembly. It keeps the irreversible surface — the public `buildArgv` return shape — additive and +backward-compatible (the field is optional; agents and backends that ignore it are unaffected). + +## Consequences + +- **The transcript channel works from inside a Claude Code session** — dev, CI, and nested agents — which + is the substrate's core read path. The gremlin ("0 messages / gate stalls") is closed at its root, not + papered over with flush retries. +- **`env -u` of a not-present variable is a harmless no-op**, by both GNU coreutils and BSD/macOS `env`: + `-u NAME` removes `NAME` if present and is silent if not. So scrubbing the full set is safe even when + claudemux is run from a *clean* (non-nested) shell where some or all of the variables are absent — no + conditional "only unset what's present" logic is needed, and the behavior is identical across the + Linux/macOS support matrix. +- **`env` must be on PATH and support `-u`.** `env` is mandated by POSIX and present on every platform + the project supports; `-u` is supported by both GNU coreutils and BSD `env`. We invoke it **unqualified** + (`env`, resolved on the pane's PATH) consistent with how the backend already invokes `tmux`. The pane + runs under the substrate's PATH; if `env` were somehow absent the pane command fails loudly at spawn + (a visible boot failure), never a silent transcript-suppression — fail-loud over fail-silent. +- **The baked-server trap is structurally avoided.** Because the scrub is part of the pane command, it is + correct whether the shared tmux server was first started from a nested or a clean context. A + `process.env`-mutation fix or an `-e VAR=` blank fix would both pass in a clean dev run and silently + regress under a server first booted from CLAUDECODE=1 — which is precisely why the regression test + (below) must boot the server from a nested context first. +- **Resume / fork paths are covered.** The suppression is about the *spawned* process's inherited env, + independent of `--session-id` vs `--resume`/`--fork-session`, so `unsetEnv` is returned on every + `buildArgv` path. A resumed/forked claude that re-reads history still needs to *persist* the continuing + conversation; without the scrub it would suppress just as a fresh session does. +- **Scrubbing `CLAUDE_CODE_SESSION_ID` does not break claudemux.** claudemux never reads the parent's + `CLAUDE_CODE_SESSION_ID`; it mints/owns its own conversation id and passes it as `--session-id` + (`src/session/create.ts` → `claude.ts`). The scrubbed variables are the *parent* agent's identity, of + which the spawned child must be ignorant — that ignorance is the fix. +- **The consumer escape hatch is real and precedence-correct.** A consumer who genuinely wants the nested + behavior re-sets the variable via `create({ env: { CLAUDECODE: "1" } })`; because the set merge wins + over the unset (Decision 3), their value survives. No new flag, no new policy surface — the same opt-out + shape as `hooks: false`. +- **Idempotency / ordering is defined for the unset∩set overlap.** When a key appears in both `unsetEnv` + and the merged `env`, the backend drops the `-u` and emits only the `-e KEY=VAL`. This makes the + `env` invocation deterministic regardless of argument order and encodes "explicit set wins" mechanically + rather than relying on `env`'s left-to-right evaluation. +- **Layering stays intact.** The variable names and the scrub decision are confined to `claude.ts`; the + backend gains only a generic `unsetEnv: string[]` → `env -u` capability with no claude vocabulary. The + layering grep's banned-string sets are unaffected (the names are not tmux command names and live in the + permitted file). +- **Cost of the wrong fix, named.** A blank-via-`-e` or `process.env`-only fix is *cheaper to write* and + *passes a naive happy-path test*, then silently fails in CI/nested operation — the highest-cost outcome + (an intermittent, environment-dependent data-loss bug). The `env -u` pane-prefix is the more expensive + build by a small margin and the only one that survives the shared-server reality; the regression test + is what holds the line. + +## Evidence + +Verified against the code at HEAD. The set-only env path: `buildArgv` returns `env` +(`src/agents/claude.ts`), merged in `spawnBootHandle` as `mergedEnv` (`src/session/spawn-boot.ts`) and +emitted as `new-session -e KEY=VAL` (`src/backends/tmux/sessions.ts`), whose own comment already records +why `set-environment` after `new-session` cannot affect the spawned pane. The shared/persistent server +and its baked `env: process.env` are in `src/backends/tmux/exec.ts` + `socket.ts` (ADR 0006). The +`cmd: "claude"` / two-adjacent-element `--session-id` invariant is pinned by +`test/session/create-identity.test.ts`. The claude-vocabulary-only-in-`claude.ts` rule is enforced by +`scripts/layering-grep.sh` (meta-tested in `test/scripts/safety-grep.test.ts`). The fix mechanism (full +five-variable `env -u` prefix restoring a 10-record transcript; single-variable unset insufficient) was +A/B-validated on 2026-06-09 against claude 2.1.168/2.1.169. + +**Refinement (2026-06-09): claude's suppression is environment-fragile; the regression test asserts the +scrub, not the suppression.** A deeper A/B (claude 2.1.168/2.1.169/2.1.170 — the binary auto-updated mid- +investigation) found the *same* binary both suppresses and persists depending on which **other** env vars +are present alongside `CLAUDECODE` (a node/npm launch context, for instance, flips it back to persisting). +The nested-detection is an opaque, version- and environment-dependent heuristic — which both explains the +*intermittent* history of the gremlin and means a behavioural assertion ("did claude suppress the +transcript?") is a **non-deterministic guard**: it can pass even with the fix reverted, under a non- +suppressing env. The first draft of the live test asserted persistence behaviourally and indeed gave a +**false pass** (its negative control did not fail). The shipped live regression +(`test/session/transcript-persistence.live.test.ts`, gated `CLAUDEMUX_LIVE_TRANSCRIPT_PERSISTENCE=1`, +excluded from CI) instead asserts the fix's **own observable effect**: under a tmux server whose global env +carries all five nested vars, a claudemux-spawned claude process has all five **scrubbed** from its +`/proc//environ`. That absence is the necessary-and-sufficient cause of correct persistence, it is +version-independent, and its negative control is real — reverting the `env -u` emission leaks all five into +the pane (verified: test passes with the fix, fails without). The `env -u` prefix and the structural +avoidance of the baked-server trap (Consequences) are unchanged; only the *form of the proof* is refined +from "observe claude persist" to "observe claudemux scrub." + +## Follow-up (2026-06-09): validate `unsetEnv` names at the backend boundary + +A security review of the shipped scrub accepted, named, and deemed **non-blocking** a defense-in-depth +gap in Decision-4's seam. This follow-up **refines, and does not reverse, that decision** — which is why +it is recorded here rather than as a superseding ADR: no accepted decision changes, the public +`buildArgv`/`unsetEnv` contract is untouched, and `unsetEnv` stays a neutral `string[]` the backend +"knows nothing about." It is purely additive hardening of the same seam. + +**The gap.** Decision 4 makes the backend receive an **opaque `string[]`** of names — correct for +*layering*, but it means the backend performs **no shape validation** of the names it splices into the +`env -u NAME …` pane prefix. Today the **sole producer** is the trusted constant `NESTED_AGENT_ENV` in +`claude.ts`, so there is **no exploit**. The gap is latent: if a future agent (or a direct +`tmuxBackend.spawn` consumer) ever derived `unsetEnv` from untrusted input, a name like `"-X"`, `""`, or +one containing `=`/whitespace could **mis-instruct `env`**. It stays a single argv element — no shell, so +**no shell-escape and no second-command** risk (the `--` terminator and the no-shell `execFile` path both +hold) — but a crafted name could make `env` set/misparse a variable or consume the next token as a flag +for that one spawn. + +**Why the boundary, not `claude.ts`.** The check asserts that each name is a **well-formed POSIX +environment-variable name** — a property of the *seam's own grammar* (what `env -u` can safely accept), +**not** claude knowledge. So it belongs at the boundary, runs for **any** producer (defense at the seam, +not trust in the agent), and keeps the layering grep clean. It mirrors exactly where `validateNamePart` +is already called on the same `spawn` method. + +**Decision (follow-up):** + +1. **A neutral validator `validateEnvVarName(name: string): void`** in `src/session/validate.ts`, + alongside `validateNamePart` / `validateAgentSessionId` (this file is the substrate's boundary-validator + home; the backend already imports `validateNamePart` from here). It enforces the POSIX + portable-name shape **`/^[A-Za-z_][A-Za-z0-9_]*$/`** — anchored, so it rejects the empty string, a + leading `-` (the `env`-flag hazard), and any name containing `=`, whitespace, or a control char (all + structurally excluded by the character class; `=` and `-` need no separate check because the anchored + regex already forbids them). This is the **stricter** sibling of `validateNamePart`: an env name is a + pure identifier, so an allow-list regex is simpler and tighter than the deny-list used for tmux target + names. The trusted `NESTED_AGENT_ENV` set (`CLAUDECODE`, `CLAUDE_CODE_ENTRYPOINT`, + `CLAUDE_CODE_SESSION_ID`, `CLAUDE_CODE_EXECPATH`, `AI_AGENT`) **all match**, so no existing caller breaks. + +2. **Enforced at the tmux backend boundary** in `tmuxBackend.spawn` (`src/backends/tmux/index.ts`), + beside the existing `validateNamePart` calls. It runs for **any** `unsetEnv` reaching the backend + regardless of producer, only on `spawn` (the sole mutating verb that carries `unsetEnv`), and + **before** `newSession` — so it composes cleanly with the existing unset∩set overlap filter + (validation gates the *names*; the overlap filter decides set-vs-unset *precedence* — orthogonal). + O(n) over a ≤5-element list on the cold spawn path — negligible. + +3. **A new typed error `InvalidEnvVarName extends ClaudemuxError`** in `src/errors.ts`, matching the + `InvalidAgentSessionId` shape (carries the offending `value`; `sessionName` placeholder + `""` since no session is created). It fails **closed** — thrown synchronously inside + the `async spawn`, surfacing as a promise rejection before any tmux invocation, consistent with how + `validateNamePart`'s throw is surfaced. No bare `Error`, per the taxonomy's contract. + +This is purely additive and backward-compatible: the validator only *rejects* malformed names that the +sole current producer never emits — a no-op for every shipping path, a guard for every future one. diff --git a/docs/decisions/README.md b/docs/decisions/README.md index a287e35..0ea50fc 100644 --- a/docs/decisions/README.md +++ b/docs/decisions/README.md @@ -19,3 +19,4 @@ record, it cites one of these repo-relative paths, which resolve on GitHub. | [0005](0005-adopt-reuses-sessiongone.md) | `adopt()` reuses `SessionGone` for an absent session (symmetric with `create`/`SessionExists`); no new error class. | | [0006](0006-adopt-single-writer-invariant-no-lock.md) | `adopt()` documents a single-writer invariant, adds no cross-process lock, and is a pure attach (no dialog dismissal); `state()`-after-adopt carries the safety. | | [0007](0007-pane-dead-detection-and-signal-representation.md) | _(superseded)_ `PaneDead` detection + signal representation. `PaneDead` was removed (`remain-on-exit off` reaps the pane → `SessionGone`). | +| [0008](0008-scrub-parent-agent-env-for-transcript-persistence.md) | The spawned `claude` runs under an `env -u` scrub of the parent agent's env vars (`CLAUDECODE`, `CLAUDE_CODE_*`, `AI_AGENT`) so its transcript persists; threaded via a new optional `unsetEnv` on `buildArgv`; consumer `env` re-set wins. | diff --git a/package.json b/package.json index 81d9c88..08b5bca 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@wastedcode/claudemux", - "version": "0.2.2", + "version": "0.2.3", "publishConfig": { "access": "public" }, diff --git a/src/agents/claude.ts b/src/agents/claude.ts index 312767e..5f733e5 100644 --- a/src/agents/claude.ts +++ b/src/agents/claude.ts @@ -231,6 +231,22 @@ const SESSION_ID_FLAG = "--session-id"; const RESUME_FLAGS = ["--resume", "-r"]; const FORK_FLAG = "--fork-session"; +/** + * Parent-agent env vars that make a spawned claude believe it is NESTED and + * SUPPRESS its own transcript persistence (writes only an async ai-title + * record). claudemux drives the agent by READING that transcript, so a + * suppressed agent is broken (ADR 0008). The SOLE file that knows these names + * (layering grep T12). A/B-proven (2026-06-09, claude 2.1.168/.169): the FULL + * set must be unset — the detection trips on any single remaining signal. + */ +const NESTED_AGENT_ENV: readonly string[] = [ + "CLAUDECODE", + "CLAUDE_CODE_ENTRYPOINT", + "CLAUDE_CODE_SESSION_ID", + "CLAUDE_CODE_EXECPATH", + "AI_AGENT", +]; + /** * What identity flag (if any) the caller already put in `extraArgs`, and the * id it selects. For `session-id`/`resume`, `value === undefined` means "an id @@ -312,6 +328,7 @@ function buildArgv(o: { cmd: string; argv: string[]; env: Record; + unsetEnv: string[]; agentSessionId?: string; } { void o.cwd; // cwd is plumbed by the session/backend layer at spawn time @@ -328,6 +345,7 @@ function buildArgv(o: { cmd: "claude", argv: [RESUME_FLAGS[0] as string, o.resumeFrom, ...extraArgs], env, + unsetEnv: [...NESTED_AGENT_ENV], agentSessionId: o.resumeFrom, }; } @@ -347,6 +365,7 @@ function buildArgv(o: { cmd: "claude", argv: extraArgs, env, + unsetEnv: [...NESTED_AGENT_ENV], ...(surfaced === undefined ? {} : { agentSessionId: surfaced }), }; } @@ -358,12 +377,13 @@ function buildArgv(o: { cmd: "claude", argv: [SESSION_ID_FLAG, o.sessionId, ...extraArgs], env, + unsetEnv: [...NESTED_AGENT_ENV], agentSessionId: o.sessionId, }; } // No id at all (e.g. an internal caller that didn't mint one) — pass through. - return { cmd: "claude", argv: extraArgs, env }; + return { cmd: "claude", argv: extraArgs, env, unsetEnv: [...NESTED_AGENT_ENV] }; } /** The `claude` agent definition. */ diff --git a/src/agents/types.ts b/src/agents/types.ts index 8c6ad5d..2ff893d 100644 --- a/src/agents/types.ts +++ b/src/agents/types.ts @@ -90,6 +90,15 @@ export interface AgentDef { cmd: string; argv: string[]; env?: Record; + /** + * Env var NAMES to genuinely UNSET for the spawned process (a TRUE unset, + * not a blank). Applied BEFORE the session-layer env merge, so a name that + * also appears in the merged `env` ends up SET (consumer re-set wins). The + * backend emits this however its substrate truly unsets a var (tmux: an + * `env -u NAME` launch prefix — `-e VAR=` only blanks, it cannot unset). The + * agent owns the LIST; the backend owns the mechanism. + */ + unsetEnv?: string[]; agentSessionId?: string; }; diff --git a/src/backends/tmux/index.ts b/src/backends/tmux/index.ts index a8e896b..48ea6fd 100644 --- a/src/backends/tmux/index.ts +++ b/src/backends/tmux/index.ts @@ -1,5 +1,5 @@ import { InvalidSessionName } from "../../errors.js"; -import { validateNamePart } from "../../session/validate.js"; +import { validateEnvVarName, validateNamePart } from "../../session/validate.js"; import { type Backend, type BackendEvent, @@ -62,11 +62,13 @@ export function tmuxBackend(opts: { socket: string }): Backend { spawn: async (o) => { validateNamePart("namespace", o.namespace); validateNamePart("name", o.name); + for (const name of o.unsetEnv ?? []) validateEnvVarName(name); // ADR 0008 follow-up await newSession(exec, { namespace: o.namespace, name: o.name, cwd: o.cwd, ...(o.env ? { env: o.env } : {}), + ...(o.unsetEnv ? { unsetEnv: o.unsetEnv } : {}), cmd: o.cmd, argv: o.argv, label: formatSessionLabel({ namespace: o.namespace, name: o.name }), diff --git a/src/backends/tmux/sessions.ts b/src/backends/tmux/sessions.ts index 08deebb..1dbcd05 100644 --- a/src/backends/tmux/sessions.ts +++ b/src/backends/tmux/sessions.ts @@ -27,6 +27,14 @@ export function targetOf(namespace: string, name: string): string { * substrate's supported floor (see README §Compatibility / details.md * §Quality). `set-environment` after `new-session` is not an option — it * doesn't affect the already-spawned pane process. + * + * `o.unsetEnv` names env vars to genuinely UNSET for the pane process. tmux's + * `-e` can only set (`-e VAR=` blanks, it cannot unset), so the launch command + * is wrapped in an `env -u NAME … --` prefix — the only mechanism that yields a + * genuinely-absent variable in the pane process, and one immune to the + * shared/baked-server trap because it acts at exec time on the pane command + * itself (ADR 0008). A name that also appears in the set `env` keeps its `-e` + * and drops its `-u` (explicit set wins). */ export async function newSession( exec: TmuxExec, @@ -37,6 +45,8 @@ export async function newSession( env?: Record; cmd: string; argv: string[]; + /** Env var names to genuinely unset for the pane process (an `env -u` prefix). */ + unsetEnv?: string[]; /** User-facing label for error messages (defaults to tmux target encoding). */ label?: string; }, @@ -49,6 +59,18 @@ export async function newSession( // `env: { LC_ALL }` (claude does) doesn't produce a duplicate `-e` flag. const env: Record = { LC_ALL: "C.UTF-8", ...o.env }; const envFlags = Object.entries(env).flatMap(([k, v]) => ["-e", `${k}=${v}`]); + + // Resolve the unset∩set overlap HERE: drop `-u` for any name also `-e` set, + // so the explicit set wins and `env` never both unsets and re-sets a name. + // The `--` terminator stops `env` from parsing cmd/argv as its own options. + // Empty/absent unsetEnv → `launch = [cmd, ...argv]`, byte-identical to before. + const setKeys = new Set(Object.keys(env)); + const unsetNames = (o.unsetEnv ?? []).filter((n) => !setKeys.has(n)); + const launch = + unsetNames.length > 0 + ? ["env", ...unsetNames.flatMap((n) => ["-u", n]), "--", o.cmd, ...o.argv] + : [o.cmd, ...o.argv]; + const newSessionCmd = [ "new-session", "-d", @@ -62,8 +84,7 @@ export async function newSession( ...envFlags, "-c", o.cwd, - o.cmd, - ...o.argv, + ...launch, ]; // Combine server-options + new-session into ONE tmux invocation so the diff --git a/src/backends/types.ts b/src/backends/types.ts index 304d9af..c680884 100644 --- a/src/backends/types.ts +++ b/src/backends/types.ts @@ -75,6 +75,12 @@ export interface Backend { env?: Record; cmd: string; argv: string[]; + /** + * Env var names to genuinely UNSET for the spawned process — see + * {@link AgentDef.buildArgv}'s `unsetEnv`. The backend emits this however + * its substrate truly unsets a var (a name also in `env` stays SET). + */ + unsetEnv?: string[]; }, ): Promise; diff --git a/src/errors.ts b/src/errors.ts index 117bbc1..73e79c3 100644 --- a/src/errors.ts +++ b/src/errors.ts @@ -60,6 +60,33 @@ export class InvalidAgentSessionId extends ClaudemuxError { } } +/** + * Thrown at the backend boundary when an `unsetEnv` name is not a well-formed + * POSIX environment-variable name (`[A-Za-z_][A-Za-z0-9_]*`). Thrown *before + * spawn*, at the substrate boundary. + * + * @remarks + * Defense-in-depth (ADR 0008 follow-up). The name is spliced into the backend's + * `env -u NAME …` pane-launch prefix. It always stays a single argv element — no + * shell, so no shell-escape and no second command — but a malformed name (a + * leading `-`, an embedded `=`/space, the empty string) could mis-instruct `env` + * for that one spawn. The sole current producer is a trusted constant, so this + * never fires today; the guard protects the seam against a future untrusted + * producer. Do not relax to a loose check — the allow-list shape is the point. + */ +export class InvalidEnvVarName extends ClaudemuxError { + /** The malformed env-var name the caller passed. */ + readonly value: string; + + constructor(value: string) { + super( + `invalid unsetEnv name ${JSON.stringify(value)}: must be a POSIX env var name ([A-Za-z_][A-Za-z0-9_]*)`, + "", + ); + this.value = value; + } +} + /** * Thrown by {@link create} when the caller passes an explicit * `agentSessionId` **and** an identity flag in `extraArgs` that also selects a diff --git a/src/session/spawn-boot.ts b/src/session/spawn-boot.ts index dcf2008..33ba400 100644 --- a/src/session/spawn-boot.ts +++ b/src/session/spawn-boot.ts @@ -75,6 +75,9 @@ export async function spawnBootHandle(o: SpawnBootInput): Promise env: mergedEnv, cmd: argvBuild.cmd, argv: argvBuild.argv, + ...(argvBuild.unsetEnv && argvBuild.unsetEnv.length > 0 + ? { unsetEnv: argvBuild.unsetEnv } + : {}), }); // The caller-known id to attribute a boot-death to: the resume id, or an diff --git a/src/session/validate.ts b/src/session/validate.ts index ab3bd58..22acfd6 100644 Binary files a/src/session/validate.ts and b/src/session/validate.ts differ diff --git a/test/backends/tmux/hasSession-edge.test.ts b/test/backends/tmux/hasSession-edge.test.ts index fb9023c..79c5e26 100644 --- a/test/backends/tmux/hasSession-edge.test.ts +++ b/test/backends/tmux/hasSession-edge.test.ts @@ -2,7 +2,7 @@ import { afterEach, beforeEach, describe, expect, it } from "vitest"; import { TmuxExec } from "../../../src/backends/tmux/exec.js"; import { tmuxBackend } from "../../../src/backends/tmux/index.js"; import { hasSession, newSession, targetOf } from "../../../src/backends/tmux/sessions.js"; -import { InvalidSessionName } from "../../../src/errors.js"; +import { InvalidEnvVarName, InvalidSessionName } from "../../../src/errors.js"; import { Harness } from "../../harness/index.js"; /** @@ -106,4 +106,18 @@ describe("tmuxBackend wrapper — exists/kill total for reserved-char names (QA }), ).rejects.toBeInstanceOf(InvalidSessionName); }); + + it("spawn() rejects a malformed unsetEnv name with InvalidEnvVarName", async () => { + const backend = tmuxBackend({ socket: h.socket }); + await expect( + backend.spawn({ + namespace: "ns", + name: "ok", + cwd: h.sandbox.home, + cmd: "sleep", + argv: ["60"], + unsetEnv: ["-X"], + }), + ).rejects.toBeInstanceOf(InvalidEnvVarName); + }); }); diff --git a/test/backends/tmux/sessions.test.ts b/test/backends/tmux/sessions.test.ts index 3f8d0b4..1c6ae1c 100644 --- a/test/backends/tmux/sessions.test.ts +++ b/test/backends/tmux/sessions.test.ts @@ -191,6 +191,79 @@ describe("sessions module — namespace-isolated CRUD", () => { expect(await hasSession(exec, target)).toBe(false); }); + // ── unsetEnv → `env -u` launch prefix (ADR 0008) ───────────────────────── + // Assert the EMITTED new-session argv via onCommand (no dependence on the + // wrapped command's runtime behavior): the scrub is a command-assembly + // concern, so inspecting the issued argv is the right unit. + describe("unsetEnv emits an `env -u … --` launch prefix", () => { + /** Capture the new-session argv issued for one spawn. */ + async function spawnAndCapture(o: { + name: string; + cmd: string; + argv: string[]; + env?: Record; + unsetEnv?: string[]; + }): Promise { + let issued: string[] | undefined; + const off = exec.onCommand((e) => { + if (e.argv.includes("new-session")) issued = e.argv; + }); + await newSession(exec, { + namespace: NS, + name: o.name, + cwd: h.sandbox.home, + cmd: o.cmd, + argv: o.argv, + ...(o.env ? { env: o.env } : {}), + ...(o.unsetEnv ? { unsetEnv: o.unsetEnv } : {}), + }); + off(); + expect(issued).toBeDefined(); + return issued ?? []; + } + + it("wraps the launch in `env -u NAME … -- ` for each unset name", async () => { + const argv = await spawnAndCapture({ + name: "scrub", + cmd: "sleep", + argv: ["60"], + unsetEnv: ["CLAUDECODE", "AI_AGENT"], + }); + // The pane command must read: env -u CLAUDECODE -u AI_AGENT -- sleep 60 + const launch = argv.slice(argv.indexOf("env")); + expect(launch).toEqual(["env", "-u", "CLAUDECODE", "-u", "AI_AGENT", "--", "sleep", "60"]); + }); + + it("drops `-u NAME` for a name also in the set env — explicit set wins, emits `-e`", async () => { + const argv = await spawnAndCapture({ + name: "overlap", + cmd: "sleep", + argv: ["60"], + env: { CLAUDECODE: "1" }, + unsetEnv: ["CLAUDECODE"], + }); + // The set value rides as `-e CLAUDECODE=1` … + expect(argv).toContain("-e"); + expect(argv).toContain("CLAUDECODE=1"); + // … and the `-u CLAUDECODE` is dropped, so no `env` wrapper at all + // (it was the only unset name). + expect(argv).not.toContain("-u"); + expect(argv).not.toContain("env"); + }); + + it("empty/absent unsetEnv → exactly ` ` with no `env` wrapper", async () => { + const argv = await spawnAndCapture({ + name: "noscrub", + cmd: "sleep", + argv: ["60"], + }); + expect(argv).not.toContain("env"); + expect(argv).not.toContain("-u"); + // The command tail is the bare cmd + argv, byte-identical to before. + expect(argv.slice(argv.length - 2)).toEqual(["sleep", "60"]); + }); + }); + it("applies the five per-session options (verifies escape-time + history-limit at minimum)", async () => { const NAME = "opts"; const target = targetOf(NS, NAME); diff --git a/test/session/create-identity.test.ts b/test/session/create-identity.test.ts index 2bea907..9af366f 100644 --- a/test/session/create-identity.test.ts +++ b/test/session/create-identity.test.ts @@ -242,3 +242,44 @@ describe("create() uses the claude agent's buildArgv by default", () => { expect(r.argv).toEqual(["--session-id", CALLER_UUID]); }); }); + +/** + * The parent-agent env scrub (ADR 0008): every buildArgv path returns the same + * five names to unset so a spawned claude never trips its own nested-session + * detection and suppresses its transcript — the channel claudemux drives on. + * The scrub is additive: `cmd`/`argv` (the security-pinned identity shape) + * stay byte-identical to before. + */ +describe("claude.buildArgv — unsetEnv scrubs the parent-agent env (ADR 0008)", () => { + const NESTED = [ + "CLAUDECODE", + "CLAUDE_CODE_ENTRYPOINT", + "CLAUDE_CODE_SESSION_ID", + "CLAUDE_CODE_EXECPATH", + "AI_AGENT", + ]; + + it("fresh --session-id path returns the five names AND keeps cmd/argv unchanged", () => { + const r = claude.buildArgv({ cwd: "/tmp", sessionId: CALLER_UUID }); + expect(r.unsetEnv).toEqual(NESTED); + // The existing identity invariant must remain green. + expect(r.cmd).toBe("claude"); + expect(r.argv).toEqual(["--session-id", CALLER_UUID]); + }); + + it("resume path returns the same five names", () => { + const r = claude.buildArgv({ cwd: "/tmp", resumeFrom: CALLER_UUID }); + expect(r.unsetEnv).toEqual(NESTED); + expect(r.cmd).toBe("claude"); + expect(r.argv).toEqual(["--resume", CALLER_UUID]); + }); + + it("caller-identity (extraArgs) path returns the same five names", () => { + const r = claude.buildArgv({ + cwd: "/tmp", + extraArgs: ["--resume", CALLER_UUID], + }); + expect(r.unsetEnv).toEqual(NESTED); + expect(r.cmd).toBe("claude"); + }); +}); diff --git a/test/session/transcript-persistence.live.test.ts b/test/session/transcript-persistence.live.test.ts new file mode 100644 index 0000000..d9bea8c --- /dev/null +++ b/test/session/transcript-persistence.live.test.ts @@ -0,0 +1,177 @@ +import { spawn } from "node:child_process"; +import { mkdtempSync, readFileSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { afterAll, beforeAll, describe, expect, it } from "vitest"; +import { tmuxBackend } from "../../src/backends/tmux/index.js"; +import { mintSocket } from "../../src/backends/tmux/socket.js"; +import type { Backend } from "../../src/backends/types.js"; +import { create } from "../../src/session/create.js"; +import type { SessionHandle } from "../../src/types.js"; + +/** + * Live regression for the parent-agent env scrub (ADR 0008). + * + * The bug: a `claude` spawned by claudemux that inherits the parent Claude + * Code's env (`CLAUDECODE`, `CLAUDE_CODE_ENTRYPOINT`, `CLAUDE_CODE_SESSION_ID`, + * `CLAUDE_CODE_EXECPATH`, `AI_AGENT`) can trip claude's nested-session detection + * and SUPPRESS its own transcript persistence — leaving claudemux, which drives + * the agent by READING that transcript, with an empty conversation. The fix + * launches the pane under an `env -u --` prefix so those vars are + * genuinely ABSENT from the claude process. + * + * Why this test asserts the SCRUB, not claude's suppression behaviour: + * claude's nested-detection is an opaque, version- and environment-fragile + * heuristic. An on-box A/B (2026-06-09, claude 2.1.168/2.1.169/2.1.170) found + * the SAME binary both suppresses and persists depending on which *other* env + * vars happen to be present alongside `CLAUDECODE` (e.g. a node/npm launch + * context flips it). So a behavioural assertion — "did claude suppress?" — is + * non-deterministic and would be a false-confidence guard (it passes even with + * the fix reverted, under a non-suppressing env). The deterministic, + * version-independent regression is the fix's OWN observable effect: under a + * tmux server whose global env carries all five nested vars (tmux seeds a + * server's global env from the process that starts it and copies it into every + * later pane — `update-environment` would NOT propagate these, so baking at + * server-start is the faithful repro), a claudemux-spawned claude process must + * have all five vars SCRUBBED. That absence is the necessary-and-sufficient + * cause of correct persistence; reverting the `env -u` emission makes all five + * leak into the pane (verified), so this is a real regression guard. + * + * Like the other `*.live.test.ts`, this spawns a real authenticated claude, so + * it is excluded from the gate suite (`vitest.config.ts`) AND self-skips unless + * `CLAUDEMUX_LIVE_TRANSCRIPT_PERSISTENCE=1`. It never runs in CI — it is a + * maintainer acceptance test for the ADR 0008 fix. It reads the pane process's + * environment via `/proc`, so it also self-skips on non-Linux. + */ +const LIVE = process.env.CLAUDEMUX_LIVE_TRANSCRIPT_PERSISTENCE === "1"; +const HAVE_PROC = process.platform === "linux"; + +/** + * The five parent-agent env vars that claudemux must scrub (ADR 0008). + * Deliberately duplicated here rather than imported from `src/agents/claude.ts`: + * the test pins the OBSERVED set independently of the production constant — if + * they drift, that is a finding, not a silently-passing test. Values are + * realistic-but-inert; only their presence/absence in the pane matters. + */ +const NESTED_VARS = [ + "CLAUDECODE", + "CLAUDE_CODE_ENTRYPOINT", + "CLAUDE_CODE_SESSION_ID", + "CLAUDE_CODE_EXECPATH", + "AI_AGENT", +] as const; +const NESTED_ENV: Record = { + CLAUDECODE: "1", + CLAUDE_CODE_ENTRYPOINT: "cli", + CLAUDE_CODE_SESSION_ID: "00000000-0000-4000-8000-000000000000", + CLAUDE_CODE_EXECPATH: "/nonexistent/claude-parent", + AI_AGENT: "claude-code_test_agent", +}; + +/** + * Boot the tmux server on `socket` with the nested env baked into its global + * environment, then leave a holder session so the server stays up. Matches the + * backend's `-L -f /dev/null` form so `tmuxBackend({ socket })` + * attaches to THIS server and its later panes inherit the nested global env. + */ +function bootNestedServer(socket: string): Promise { + return new Promise((resolve, reject) => { + const child = spawn( + "tmux", + // biome-ignore format: argv reads clearer one-per-line + [ + "-L", socket, "-f", "/dev/null", + "new-session", "-d", "-s", "nested-boot-holder", + "sleep", "100000", + ], + { env: { ...process.env, ...NESTED_ENV }, stdio: "ignore" }, + ); + child.on("error", reject); + child.on("close", (code) => + code === 0 ? resolve() : reject(new Error(`nested-server boot exited ${code}`)), + ); + }); +} + +/** SIGKILL-free server teardown: `kill-server` reaps the holder + every pane. */ +function killServer(socket: string): Promise { + return new Promise((resolve) => { + const child = spawn("tmux", ["-L", socket, "-f", "/dev/null", "kill-server"], { + stdio: "ignore", + }); + child.on("close", () => resolve()); + child.on("error", () => resolve()); + }); +} + +/** The foreground pane process pid for a tmux session target. */ +function panePid(socket: string, target: string): Promise { + return new Promise((resolve) => { + let out = ""; + const child = spawn( + "tmux", + ["-L", socket, "-f", "/dev/null", "list-panes", "-t", target, "-F", "#{pane_pid}"], + { stdio: ["ignore", "pipe", "ignore"] }, + ); + child.stdout.on("data", (b) => { + out += b.toString("utf8"); + }); + child.on("close", () => resolve(out.trim())); + }); +} + +/** The env var NAMES of a live process, read from `/proc//environ`. */ +function processEnvNames(pid: string): Set { + const raw = readFileSync(`/proc/${pid}/environ`, "utf8"); + return new Set(raw.split("\0").map((entry) => entry.split("=")[0] ?? "")); +} + +describe("parent-agent env scrub on a nested-booted shared server (ADR 0008)", () => { + if (!LIVE || !HAVE_PROC) { + const why = !LIVE + ? "set CLAUDEMUX_LIVE_TRANSCRIPT_PERSISTENCE=1 to enable (maintainer acceptance test)" + : "requires Linux /proc to read the pane process environment"; + it.skip(`auth-gated — ${why}`, () => {}); + return; + } + + let socket: string; + let backend: Backend; + let cwd: string; + const sessions: SessionHandle[] = []; + + beforeAll(async () => { + socket = mintSocket(); + await bootNestedServer(socket); + backend = tmuxBackend({ socket }); + cwd = mkdtempSync(join(tmpdir(), "claudemux-scrub-live-")); + }); + + afterAll(async () => { + for (const s of sessions) await s.kill().catch(() => undefined); + await killServer(socket); + if (cwd) rmSync(cwd, { recursive: true, force: true }); + }); + + it("a claudemux-spawned claude has all five nested-agent vars scrubbed from its process env", async () => { + const session = await create({ name: "scrub-1", cwd, backend, trustWorkspace: true }); + sessions.push(session); + + const pid = await panePid(socket, `${session.namespace}--${session.name}`); + expect(pid).toMatch(/^\d+$/); // a real pane process, not an empty target + + const names = processEnvNames(pid); + const leaked = NESTED_VARS.filter((v) => names.has(v)); + + // The load-bearing assertion: the fix (`env -u … --`) removed every nested + // var from the live claude process. Reverting the emission leaks all five + // (the negative control), which is what would re-suppress the transcript. + expect(leaked).toEqual([]); + + // Sanity: the holder pane DID carry the nested env, proving the server + // global env is the suppressing source the scrub had to defeat (not a + // vacuously-clean environment). + const holderPid = await panePid(socket, "nested-boot-holder"); + expect(processEnvNames(holderPid).has("CLAUDECODE")).toBe(true); + }, 120_000); +}); diff --git a/test/session/validate.test.ts b/test/session/validate.test.ts new file mode 100644 index 0000000..2347580 --- /dev/null +++ b/test/session/validate.test.ts @@ -0,0 +1,48 @@ +import { describe, expect, it } from "vitest"; +import { ClaudemuxError, InvalidEnvVarName } from "../../src/errors.js"; +import { validateEnvVarName } from "../../src/session/validate.js"; + +/** + * Boundary validation of `unsetEnv` names (ADR 0008 follow-up). The allow-list + * shape (`[A-Za-z_][A-Za-z0-9_]*`) is the point — it keeps a name `env` cannot + * misparse regardless of producer. + */ +describe("validateEnvVarName", () => { + // Regression guard for the real producer: every NESTED_AGENT_ENV name in + // src/agents/claude.ts must validate, or we'd break the trusted nested-agent + // launch path. + it.each([ + "CLAUDECODE", + "CLAUDE_CODE_ENTRYPOINT", + "CLAUDE_CODE_SESSION_ID", + "CLAUDE_CODE_EXECPATH", + "AI_AGENT", + ])("accepts the NESTED_AGENT_ENV name %s", (name) => { + expect(() => validateEnvVarName(name)).not.toThrow(); + }); + + it.each(["_FOO", "X"])("accepts the identifier-shaped name %s", (name) => { + expect(() => validateEnvVarName(name)).not.toThrow(); + }); + + it.each(["", "-X", "A=B", "A B", "A.B", "1ABC", "A\nB", "A\0B"])( + "rejects the malformed name %j with InvalidEnvVarName", + (name) => { + expect(() => validateEnvVarName(name)).toThrow(InvalidEnvVarName); + }, + ); + + it("carries the offending input on .value and is a ClaudemuxError", () => { + for (const bad of ["", "-X", "A=B", "A B", "A.B", "1ABC", "A\nB", "A\0B"]) { + let caught: unknown; + try { + validateEnvVarName(bad); + } catch (err) { + caught = err; + } + expect(caught).toBeInstanceOf(InvalidEnvVarName); + expect(caught).toBeInstanceOf(ClaudemuxError); + expect((caught as InvalidEnvVarName).value).toBe(bad); + } + }); +}); diff --git a/vitest.config.ts b/vitest.config.ts index e18c1a4..9a08c2b 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -22,6 +22,12 @@ export default defineConfig({ // by CLAUDEMUX_LIVE_AGENT_SESSION_ID=1) to prove the id round-trip, // resume-via-extraArgs, and the collision error — same isolation reason. "test/session/agent-session-id.live.test.ts", + // transcript-persistence.live.test.ts spawns a REAL authenticated claude + // (gated by CLAUDEMUX_LIVE_TRANSCRIPT_PERSISTENCE=1) on a tmux server + // booted with the nested-agent env baked in, to prove the ADR 0008 scrub + // restores transcript persistence — same isolation reason. Maintainer + // acceptance test; never a CI gate. + "test/session/transcript-persistence.live.test.ts", ], testTimeout: 30_000, hookTimeout: 30_000,