fix(tmux): deliver session env via set-environment, not -e argv (ADR-0051) - #127
fix(tmux): deliver session env via set-environment, not -e argv (ADR-0051)#127voxist-merge-bot[bot] wants to merge 4 commits into
Conversation
…0051) NewSessionWithCommandAndEnv appended every env var (including all secret credentials) as '-e KEY=VALUE' argv on the tmux new-session command. The new-session command becomes the persistent tmux *server* process, whose argv is world-readable via ps for the server's full lifetime - and, because orphaned servers reparent to pid 1 (ADR-0029), long after the launching session ends. Live measurement (vc-cgp) found 39 tmux servers carrying ANTHROPIC_AUTH_TOKEN, OPENROUTER_API_KEY, GC_INSTANCE_TOKEN and aliases in argv; every same-uid agent session could ps every other's provider creds. Replace the -e transport with tmux set-environment over the socket (ADR-0051, Accepted). set-environment runs as a short-lived tmux *client* whose argv persists only for the call's sub-second lifetime, so no secret value is pinned to a long-lived process's argv (C2). ORDERING (empirically verified against tmux 3.7b): session env set via set-environment applies to a pane process when it STARTS. The initial shell starts at new-session time, so env set after a bare new-session is not retroactively exported, and send-keys into that shell inherits stale env. Instead: create the session bare, set env on the session, then respawn-pane -k with the command - which re-reads the session environment (C1). Empty values unset via set-environment -u. Tests: executor_test C2 regression (no secret in persistent argv, no -e); tmux_test integration C1 (command process receives env + special-char sentinel); agent_slice_test updated for the transport. Motivating finding: vc-cgp. ADR: voxist-city/docs/decisions/ADR-0051. Bead: vp-6tdpf.
…x manifest PR #127 (ADR-0051) added one fixed_sleep call (437->438) and a new fake-executor test TestNewSessionWithCommandAndEnvNoSecretInArgv, tripping two source-first ledger invariants: - resourcecensus: bump fixed_sleep ScopeAll baseline 437->438 across the three agreeing tables (census.go, TESTING.md, test/test-resources.toml). File count unchanged (161==161). - runtime-tmux manifest: register the new test in source-discovery order (after TestNewSessionWithCommandAndEnvClearsEmptyVars) and update the count guards (manifest 346->347, untagged 232->233, shard counts to {58,58,58,58,58,57}). Integration-only split stays 114. No tmux.go / set-environment / respawn-pane logic touched.
bourgois
left a comment
bourgois
left a comment
There was a problem hiding this comment.
Architect validation (ADR-0051) — CHANGES REQUESTED
Validated against ADR-0051 as ratified (Accepted 2026-08-02). The security core of this
change is correct and I verified it empirically, not by reading the tests. One blocking
defect in the unset path, plus one non-blocking robustness finding.
What I verified as CORRECT (live, tmux 3.7b on the fleet host)
- The C2 invariant is met.
new-sessionis now bare,respawn-panecarries only the
wrapped command; no secret value lands in a long-lived process's argv. The transient
set-environmentclient argv is the residual ADR-0051 explicitly re-scoped as
acknowledged-and-bounded ("C2 scope correction", 2026-08-01) — so this is implementing the
ADR as ratified, not working around it. - The respawn transport actually delivers env, including special characters. Reproduced
the full 3-step sequence on an isolated socket; sentinela b=c;"$xarrived intact at the
command process. respawn-panewithout-cpreserves the pane start directory set bynew-session -c.
This is load-bearing for every agent session and is not asserted anywhere in the suite
(the integration test passesworkDir=""), so I checked it directly: pane cwd survived.- CI is green across the full matrix (runtime-tmux 6/6 shards, cmd/gc 12/12, CodeQL).
BLOCKING — the unset path silently lost its defense
RemoveEnvironment (internal/runtime/tmux/tmux.go:2861) issues:
t.run("set-environment", "-t", session, "-u", key)set-environment -u does not remove the variable from the environment of processes the
session starts. It deletes the session-scope entry; tmux then still merges the
server-global environment when building the new process's env, so the stale value comes
straight back.
This matters because the code this PR deletes was doing the job correctly. The old path built
an env -u LC_ALL -u LC_CTYPE <command> prefix, and its comment said exactly why:
tmux -esets session-level env but the shell process still inherits from the tmux
server's global environment.env -uensures the var is actually absent from the child
process.
That mechanism is gone; set-environment -u was substituted for it and does not have the
same semantics. Root cause: -u is "unset the session entry", not "remove before
starting a new process" — the latter is a different flag, -r. From man tmux:
The
-uflag unsets a variable.-rindicates the variable is to be removed from the
environment before starting a new process.
Reproduction (isolated socket, tmux 3.7b, exactly this PR's 3-step sequence; a
server-global value seeded first to model a stale GT_ROLE/LC_ALL from the supervisor):
set-environment -g GT_ROLE STALE_MAYOR_VALUE # server-global, as inherited by the tmux server
new-session -d -s probe -c $WD # step 1 (this PR)
set-environment -t probe -u GT_ROLE # step 2 (this PR's unset path)
respawn-pane -k -t probe '<cmd printing $GT_ROLE>'
with -u : GT_ROLE=[STALE_MAYOR_VALUE] <-- leaked; identical to issuing nothing at all
control : GT_ROLE=[STALE_MAYOR_VALUE] <-- no -u issued; same result => -u is a no-op here
with -r : GT_ROLE=[<absent>] <-- correct
So the empty-value contract ("" means "unset this var") is now unenforced, and the
regression is invisible to the suite: TestNewSessionWithCommandAndEnvClearsEmptyVars and
the agent-slice test both assert only that a set-environment ... -u KEY call was made,
never that the child process's environment lacks the key. TestNewSessionWithCommandAndEnv
exercises only vars that are set. Every test passes while the behaviour is wrong.
Practical impact: LC_ALL/LC_CTYPE are the two keys gc sends empty today, and the tmux
server's global env is inherited from whatever started it (the gc supervisor). A stale
LC_ALL therefore reaches agent panes and breaks the UTF-8 handling -u/LANG exist to
guarantee — and per the deleted comment the same hazard applies to GT_ROLE bleeding from a
parent mayor session into crew/polecat shells, which is the bug the -e transport was
originally introduced to fix.
Required change
- Use the removal flag on the new-process path —
-r(verified above), not-u. Prefer a
distinct helper (e.g.RemoveEnvironmentForNewProcess) rather than changing
RemoveEnvironmentin place, so existing-ucallers that only want the session entry
dropped keep their semantics. - Add a test that seeds a server-global value, runs the full sequence, and asserts the
var is absent from the command process — reading it back from the process, not from
show-environment. The current tests cannot fail on this defect; a call-shape assertion
is not a behaviour assertion.
NON-BLOCKING — new partial-failure mode, please file rather than fix here
The launch stopped being atomic. new-session now happens first (line ~566), then N
set-environment calls, then respawn-pane; a failure in step 2 or 3 returns the error
without killing the session step 1 created (KillSession exists at :662 and is not
called). The old single new-session was all-or-nothing.
The residue is a session that exists, runs a default shell instead of the agent, and
carries partial env — i.e. a seat that answers "alive" to every existence probe while
being structurally incapable of doing work, and whose name now collides with any retry. On a
host that has been running at load ~287 with swap exhausted, a timeout mid-sequence is
plausible rather than theoretical, and this change multiplies the opportunities from 1 per
launch to ~30 (one per env var). Suggest a defer-style rollback that kills the session on
any error after step 1. Happy for that to land separately — it should not hold the security
fix.
Disposition
The -e argv exposure is real and still live on this host today (re-confirmed by ps
this session: a voxist-city tmux server carrying provider tokens and instance/holder tokens
in cleartext argv). I want this transport landed quickly. But the -u/-r defect ships a
silent correctness regression alongside the security fix, and the suite is green over it — so
it needs the one-line flag change plus a behavioural test before merge, not after.
…-0051) Addresses the blocking finding on PR #127: the ADR-0051 transport shipped a silent correctness regression in the *unset* path alongside the security fix. `set-environment -u` deletes the SESSION-SCOPE entry only. tmux then re-merges the server-global environment when respawn-pane starts the command, so a stale server-global value (LANG/LC_ALL/LC_CTYPE, or GT_ROLE bleeding from a parent mayor session) reaches the agent pane anyway — the exact hazard the empty-value convention exists to prevent. `-r` is documented as "removed from the environment before starting a new process", which is precisely the respawn-pane in step 3. Measured on tmux 3.7b, isolated socket, server-global seeded: -u -> command process reads "STALE-GLOBAL-VALUE" -r -> command process reads <UNSET> Changes: - Add RemoveEnvironmentForNewProcess (-r) and call it from the launch path. RemoveEnvironment (-u) is left in place and documented, so the existing Provider.RemoveMeta caller — session metadata removal, not a launch — keeps its semantics. - Add TestNewSessionWithCommandAndEnvRemovesStaleGlobalFromCommandProcess: it seeds a server-global value and reads the result back FROM THE COMMAND PROCESS, not from show-environment. Verified it fails on the -u regression. - Fix a vacuous assertion in TestNewSessionWithCommandAndEnvClearsEmptyVars: runCtx prepends tmux's own global -u (force UTF-8) to every call, so contains(args, "-u") matched the wrapper flag and was true even when set-environment never received -u. The check now scans only the args after the subcommand token, and guards against a -u regression. - Update the runtime-tmux manifest and its policy counts for the new test. The non-blocking partial-failure finding from the same review (a failed step 2 or 3 leaves an orphaned session running a default shell) is tracked separately as vp-5dk0b and is deliberately not addressed here. Bead: vp-6tdpf
Rework pushed — blocking
|
…est (ADR-0051) Addresses both blocking findings from the independent re-review of e88a24a. 1. agent_slice_test.go pinned the disproven mechanism. Line 78 asserted `set-environment -u` — the exact flag e88a24a exists to remove — and passed only because of the same runCtx global--u vacuity that commit fixed in executor_test.go. slices.Contains(call, "-u") matched the force-UTF-8 -u that runCtx prepends to EVERY invocation, so the assertion was true no matter which flag set-environment actually received. The centrepiece correction (-u -> -r) therefore had negative regression cover in one of its two test sites: repairing the vacuity made the test fail against correct code. Both sites now go through one shared helper (setEnvironmentArgs + assertEnvRemovedForNewProcess) that scans only the subcommand's own args and asserts -r present / -u absent. Single home, so a fix cannot land in half the call sites again — which is precisely how this instance survived. 2. The command=="" branch delivered 0 vars to the pane, and a test ratified it. respawn-pane was gated behind `if command != ""`, so an env-only session kept the throwaway shell that `new-session -d` had already started BEFORE set-environment ran. The shell captured its env at start, so nothing reached it. show-environment still reported success — the same false-positive instrument that hid the original -e defect — and TestAgentSliceEmptyCommandNotWrapped asserted the broken shape ("NO respawn-pane is issued") and verified delivery with it. Measured on tmux 3.7b, isolated socket, env-only session: before respawn : shell env <ABSENT> / show-environment: CRITIC_VAR=... (!) bare respawn -k: shell env delivered / pane cwd preserved The respawn is now unconditional: with a command it respawns that command, otherwise it respawns bare so tmux re-runs the pane's original default shell with the env applied. Passing no shell-command means wrapPaneCommand does not apply, so the systemd wrapper is still correctly absent. Chose "make the branch honest" over "make createSession refuse command=='' && len(env)>0": a function named NewSessionWithCommandAndEnv that silently drops env is a broken contract, and the guard at adapter.go:845 (`command != "" || len(env) > 0`) routes this case deliberately — leaving it undelivered sets a trap for the next caller rather than removing it. Negative controls (green is uninformative without them): - regress -r -> -u : TestAgentSliceWrapsNewSessionWithCommandAndEnv now FAILS (it passed before this commit) and TestNewSessionWithCommandAndEnvClearsEmptyVars FAILS. - restore the `if command != ""` gate : TestAgentSliceEmptyCommandNotWrapped FAILS, and the new behavioral test FAILS with "<UNSET>". New test TestNewSessionWithCommandAndEnvDeliversEnvToEnvOnlyShell asks the pane's own SHELL to report the variable rather than reading show-environment, so it cannot pass on the false-positive instrument. Manifest + policy counts updated (349 tests, shard 1 -> 59, integration-only -> 116). Gates: go vet ./... clean; ./internal/runtime/tmux unit + integration green; ./internal/testpolicy/... green; scripts manifest green. Two pre-existing host-local failures (TestGetKeyBinding_CapturesDefaultBinding[WithArgs]) reproduce unchanged at baseline HEAD with these changes stashed. The non-atomic-launch finding remains owned by vp-5dk0b, not duplicated here. Bead: vp-6tdpf
voxist-bot
left a comment
voxist-bot
left a comment
There was a problem hiding this comment.
infra-reviewer — APPROVE at 770ea01
Independent review (voxist.infra-reviewer; distinct from platform-architect-1 who authored this PR, and from voxist.critic who filed the 2026-08-12 CHANGES_REQUESTED objections this commit addresses).
What I verified, not just read:
- Both objections from the critic's e88a24a review are resolved by commit
770ea01dd(found committed locally but never pushed — pushed as part of this review so the PR reflects it):- Strongest objection (vacuous
set-environment -uassertion inagent_slice_test.go— passed only becauserunCtxprepends its own global-uto every call): fixed via a sharedsetEnvironmentArgs/assertEnvRemovedForNewProcesshelper. - Objection 1 (
command == "" && len(env) > 0path delivered 0/1 vars —respawn-panewas gated behindcommand != "", so an env-only session kept the pre-set-environmentdefault shell): fixed via unconditional respawn (RespawnPaneDefaultCommandfor the no-command case), verified by a new behavioral test that reads the var back from the actual shell process, notshow-environment.
- Strongest objection (vacuous
- Negative controls I ran myself (not just cited from the commit message): reverted
RemoveEnvironmentForNewProcessto-u→TestAgentSliceWrapsNewSessionWithCommandAndEnvandTestNewSessionWithCommandAndEnvClearsEmptyVarscorrectly fail. Removed thecase len(env) > 0respawn branch →TestNewSessionWithCommandAndEnvDeliversEnvToEnvOnlyShellcorrectly fails (times out, marker absent). Both confirm the tests are not vacuous. Reverted both mutations; worktree matches 770ea01 exactly. - Local (not CI — this repo's PR CI runs zero unit tests per push-only gate, vp-qm8l7): full non-integration suite green (
go test ./internal/runtime/tmux/...), targeted real-tmux integration suite green (tmux 3.7b,-tags=integration, allTestNewSessionWithCommandAndEnv*/TestAgentSlice*/TestRespawnPane*), manifest/shard policy tests green,go vetclean. - C2 invariant (no secret in persistent argv) was already independently verified by the 08-11 architect review and is unchanged by this commit — not re-litigated here.
Not addressed (non-blocking, per the critic's own disposition): Objection 3 (attribute the 2 red Integration/packages-core-* CI jobs to pre-existing/unrelated failures in the PR body) and Objection 4 (distinguish "probe absent" vs "probe present with wrong content" in the C1 integration test's failure message) — both cosmetic, neither blocks merge.
Blocking non-code issue, separate from this approval: PR still carries a CHANGES_REQUESTED review from bourgois (2026-08-11T13:15:42Z, at commit 0688e7c4, one commit before the fix it objected to). dismiss_stale_reviews only auto-clears stale approvals on push, never CHANGES_REQUESTED — and per policy an agent must never dismiss a human-identity review. That review needs to be dismissed or re-submitted by that same identity before merge is possible; flagging to karel separately.
Full verification detail posted to bead vp-6tdpf.
|
@bourgois — the finding in your 2026-08-11 CHANGES_REQUESTED review ( Could you dismiss this review, or submit a fresh one? That's the only remaining blocker to merge — |
2 participants
What
NewSessionWithCommandAndEnvappended every env var — including all secret credentials (Anthropic tokens, OpenRouter key, GC_INSTANCE/BEADS_HOLDER tokens) — as-e KEY=VALUEargv on thetmux new-sessioncommand.The
new-sessioncommand becomes the persistent tmux server process, whose argv is world-readable viapsfor the server's full lifetime — and, because orphaned tmux servers reparent to pid 1 (ADR-0029), long after the launching session ends. Live measurement (finding beadvc-cgp) found 39 tmux servers carryingANTHROPIC_AUTH_TOKEN(x18),OPENROUTER_API_KEY(x14),GC_INSTANCE_TOKEN/BEADS_HOLDER_TOKEN(x39) in argv. Every same-uid agent session couldpsevery other session's provider credentials.Fix (ADR-0051 — Accepted)
Replace the
-etransport withtmux set-environmentover the socket.set-environmentruns as a short-lived tmux client whose argv persists only for the call's sub-second lifetime, so no secret value is pinned to any long-lived process's argv (C2).Ordering (empirically verified against tmux 3.7b)
Session env set via
set-environmentis applied to a pane process when it starts. The pane's initial shell starts atnew-sessiontime, so env set after a barenew-sessionis not retroactively exported — and typing the command into that shell (send-keys) would inherit stale env (C1 break). The ADR's literal pseudocode (new-session -> set-env -> send-keys) fails this; verified by direct tmux probe.Correct sequence: create the session bare (no command, no
-e) -> set env on the session viaset-environment->respawn-pane -kwith the command, which re-reads the session environment.respawn-paneis the load-bearing step that makes env reach the command. The existingSetEnvironment/RemoveEnvironmenthelpers are reused; no new machinery.Empty env values are now unset via
set-environment -u(session-level), replacing the oldenv -ucommand prefix. Equivalent semantics; the systemd agent-slice wrapper still wraps the respawned command.Tests
-eflag anywhere. The set-environment transient client argv is the ADR-acknowledged bounded residual, intentionally excluded.Verification
Refs