feat(sandbox): isolate per-candidate agent state, and stop candidates racing to refresh one credential - #72
Open
KE7 wants to merge 7 commits into
Open
feat(sandbox): isolate per-candidate agent state, and stop candidates racing to refresh one credential#72KE7 wants to merge 7 commits into
KE7 wants to merge 7 commits into
Conversation
Every candidate container mounts the shared helix-auth-<backend> login volume at /home/node read-write. That is deliberate and unchanged: it is what lets a CLI refresh its token and take its cross-process refresh lock. But the CLIs also write their working state there, so candidate N started life reading candidate N-1's transcripts, session databases, memories and to-do lists. For an optimizer whose candidates are meant to be independent samples that contaminates the experiment, and some of that state (opencode's opencode.db) carries token columns as well. Mount a second, per-candidate directory at /helix-state -- outside /home/node, so the auth mount is untouched -- and point each backend's state at it. It lives in the sandbox's existing temporary tree, so it is created and removed with the candidate. Three backends have a knob that moves state without moving the credential; all three were verified against the real CLI in a container with a synthetic credential: codex -c sqlite_home=... state_5.sqlite, logs_2.sqlite opencode OPENCODE_DB=... opencode.db (+ -wal/-shm) cursor CURSOR_CONFIG_DIR= the whole ~/.cursor tree Choosing the knob is the whole problem: the obvious environment variable usually relocates the credential too and silently breaks login. XDG_DATA_HOME does this to opencode (auth list then reports 0 credentials) and XDG_CONFIG_HOME does it to cursor (status then reports Not logged in). Both are recorded in REJECTED_AGENT_STATE_KNOBS so they are not re-tried, and pinned by container tests. Unsandboxed opencode runs used XDG_DATA_HOME for per-candidate SQLite isolation, which hid any existing opencode login. Switch them to OPENCODE_DB; the on-disk layout is unchanged. claude is deliberately not isolated. CLAUDE_CONFIG_DIR moves the credential with the transcripts, and masking its state subdirectories was evaluated and rejected -- the mask list is already stale against the shipped CLI, it writes to the shared volume, and it silently breaks transcript preservation. docs/agent-state-isolation.md has the detail. Residue that still crosses candidates, including codex's session rollouts, is named in UNRELOCATED_AGENT_STATE rather than left implicit. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…name credential failures
Candidates share one login volume read-write, and that stays. What is not
safe is the first moment after the shared credential goes stale: every
candidate in the generation decides independently that a refresh is due
and posts the same single-use refresh token. Measured against the shipped
codex CLI with a synthetic credential in a throwaway volume and a local
single-use token endpoint: five simultaneous candidates produce five
exchanges, one grant, and four "your refresh token was already used" --
and all five exit 0 with empty stderr. A whole generation can die without
saying anything. Today the only thing standing between a run and that
outcome is codex's own last_refresh field happening to keep the window
narrow.
Warm the credential once per generation, before any candidate is
dispatched, through the sandboxed auth container that already exists and
is already a single writer. Same measurement with the warm in front: one
exchange, granted, and the five candidates then perform none, because
there is nothing left for them to refresh.
Per generation and not per run: a run outlives any refresh interval, so a
single warm at startup stops protecting it the moment the credential next
goes stale mid-flight.
Only codex is warmed, and NOT with its registered status command:
codex `codex login status` never takes the refresh path -- it exits
0 without issuing a request whether last_refresh is minutes
or 30 days old, so warming with it would be a placebo. The
warm runs `codex debug models`, which loads auth through the
refreshing path and is free: it completes with --network none
on a fresh credential, writes nothing, and its only request
when a refresh IS due is the OAuth token exchange. No model is
invoked either way. That "free" check is the constraint that
decides whether a warm may ship at all -- this runs on the
operator's paid account every generation.
claude skipped: takes a real cross-process lock, retries, re-reads.
cursor skipped: never spends its stored refresh token.
opencode skipped: refreshes only from inside the fetch wrapper that
issues a model request, so no free command performs it.
`opencode providers list` was measured free -- and for the same
reason refreshes nothing.
gemini skipped: no free command known to take the refresh path, and
no credential to measure one against. Not warmed on a guess.
Each reason is recorded in CREDENTIAL_WARM_SKIP_REASONS, and a test
asserts every backend is either warmed or explained, so a new backend
cannot fall through silently. A failed warm is logged and the run
continues -- candidates may still work on the credential already stored.
Second half: make the failure visible. There was no auth-failure
detection at all, and the structured result envelope's is_error field was
never read anywhere in src/, so an unusable login was indistinguishable
from an agent that wrote bad code. Classify credential exhaustion as its
own failure kind, anchored on wording read out of the shipped binaries
rather than guessed ("Your access token could not be refreshed" and its
four suffixes from codex, "Token refresh failed:" from opencode, "User
OAuth refresh failed" from claude). is_error narrows what is scanned but
never classifies on its own: a tool_result carrying is_error is usually
the agent's own failing command, and a candidate whose work happens to be
about refresh tokens must never be able to report the operator's login as
broken. On a zero exit only is_error-flagged envelope text is considered
for that reason; on a non-zero exit the raw streams are read too.
The result surfaces where an operator sees it: the slot says the login
failed rather than the code, and the permanent end-of-run summary repeats
it after the live display is gone.
No config knob: a credential that cannot be refreshed is never the
behaviour anyone wants, and a warm that costs nothing needs no opt-out.
… mutate() does CredentialRefreshError is a HelixError, not a MutationError, so merge()'s existing clauses let it escape with the merge worktree still on disk, and neither the merge call site in run_evolution nor the CLI caught it: a stale login that surfaced at the merge gate killed the run with a raw traceback and no end-of-run credential summary. - merger.merge(): new clause labels the operation, removes the child worktree and re-raises, mirroring mutate(). - evolution.run_evolution(): the merge call site catches it, records the slot in the run's CredentialFailureLog, prints the panel, and falls through to mutation so the generation continues. - cli.evolve / cli.resume: last-resort handler renders the panel plus a re-login/resume hint and exits 2 instead of dumping a traceback. Tests cover the merge() cleanup+re-raise, the end-to-end merge path naming the failure in the summary, and both CLI commands. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015HBBoDVQK7baNMQBhgRkh4
…ontainer on timeout The warm ran on the main thread through run_sandbox_auth_command, whose non-interactive path was a bare subprocess.run with no timeout and a docker run with no --name. A token-refresh exchange that blackholed therefore blocked the whole evolution loop before any candidate was dispatched, and Ctrl-C left the unnamed container running against the shared login volume. - run_sandbox_auth_command: accepts timeout and container_name; on TimeoutExpired the named container is force-removed before re-raising, since killing the docker client does not stop the container. - sandbox_auth_docker_args: emits --name when a container name is given. - warm_backend_credential: always runs under credential_warm_timeout() (a 300s cap that sandbox.timeout_seconds can only tighten), names its container helix-warm-<backend>-<id>, and reports a timeout as a distinct, non-fatal CredentialWarmResult(timed_out=True). - _warm_generation_credential: says "timed out" instead of "(exit None)". Tests mock subprocess.run raising TimeoutExpired and assert the result is reported (not raised), the timeout applied, and the container removed. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015HBBoDVQK7baNMQBhgRkh4
…a dead login Codex's "...because your refresh token was already used" is the exact outcome of losing the refresh race this PR exists to handle: another candidate has just stored a refreshed credential in the shared volume. It was classified identically to an expired or revoked credential -- slot dropped, worktree deleted, no retry, and the operator told to re-run `helix sandbox login codex` over a login that was fine. - mutator: the already-used suffix is a separate, transient marker matched ahead of the generic prefix; CredentialRefreshError carries `transient`. invoke_claude_code() wraps the run+classify step in one _attempt() and, on a transient failure, retries exactly once against the now-refreshed credential. A second loss is raised with a suggestion that points at `helix resume` and does not instruct a re-login. - evolution: CredentialFailureLog records transience; the per-slot message and the end-of-run summary say "lost a refresh race ... run helix resume" when every failure was transient, and keep the re-login advice otherwise. Tests cover the marker split, retry-then-success (exit 1 and the exit-0 envelope path), retry-then-failure without a re-login instruction, that expired/revoked/bare wording is never retried, and the loop-level summary. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015HBBoDVQK7baNMQBhgRkh4
opencode.db, which lives in this tree while a candidate runs, carries OAuth access and refresh tokens. The tree already sat under a tempfile.mkdtemp directory (0700) and was removed with it in run_sandboxed_commands' finally, but the state directory itself was created with the umask default, so the privacy guarantee rested on the parent alone. Create it and its per-backend subdirectory 0700 explicitly, document the host-side location and lifetime, and pin both the mode and the removal in tests. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015HBBoDVQK7baNMQBhgRkh4
tests/integration/ runs real backend containers and is not part of CI, yet a bare `pytest` collected it (skipping when Docker or an image was missing, running real containers otherwise). Deselect the marker by default via addopts; a command-line `-m docker_integration` still opts in. README says how. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015HBBoDVQK7baNMQBhgRkh4
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR does two related things to the one shared login volume. Both come out of the same fact — every candidate container mounts
helix-auth-<backend>at/home/noderead-write, and that mount is deliberate and unchanged — and each is its own commit.1. Per-candidate agent state
The problem
Every candidate container mounts the shared
helix-auth-<backend>login volume at/home/noderead-write. That is deliberate and stays exactly as it is — it is what lets a CLI refresh its token and take its cross-process refresh lock.The problem is what else rides along in that volume. The CLIs also write their working state there, so candidate N started life reading candidate N-1's transcripts, session databases, memories and to-do lists. For an evolutionary optimizer whose candidates are meant to be independent samples, that contaminates the experiment — the primary motivation here. Secondarily, some of that state (opencode's
opencode.db) carriesaccess_token/refresh_tokencolumns, so it is a second credential store lying around in a shared location.The change
Mount a second, per-candidate directory at
/helix-state— deliberately outside/home/node, so the shared auth mount is untouched — and point each backend's state at it. It lives in the sandbox's existing temporary tree (helix.sandbox.run_sandboxed_commands), so it is created and removed with the candidate by the existing_safe_rmtree.codex-c sqlite_home=…state_5.sqlite,logs_2.sqlite(+-wal/-shm).codex/auth.jsonstays sharedopencodeOPENCODE_DB=…opencode.db(+-wal/-shm)auth.jsonand the lock dir stay sharedcursorCURSOR_CONFIG_DIR=…~/.cursortree~/.config/cursor/auth.jsonstays sharedclaudePicking the knob is the whole problem
The obvious environment variable is usually the wrong one, because it relocates the credential too and silently makes an existing login invisible. Verified against the real CLIs:
XDG_DATA_HOMEon opencode →opencode auth listreports0 credentialsXDG_CONFIG_HOMEon cursor →cursor-agent statusreportsNot logged inCURSOR_DATA_DIRis accepted but relocates nothingThese are recorded in
helix.agent_state.REJECTED_AGENT_STATE_KNOBSso they are not re-tried, and pinned by container tests. HELIX never sets either variable; if a user routesXDG_CONFIG_HOMEthroughpassthrough_envor[env], the cursor backend now logs a warning.Drive-by fix
Unsandboxed opencode runs used
XDG_DATA_HOMEfor per-candidate SQLite isolation, which hid any existing opencode login. Switched toOPENCODE_DB; the on-disk layout is unchanged (<worktree>/.helix_opencode_state/opencode/opencode.db).claude is deliberately not isolated
CLAUDE_CONFIG_DIRis all-or-nothing — it moves.credentials.jsontogether with the transcripts, and pulls.claude.jsonin as well. The masking alternative was evaluated against Claude Code 2.1.138 and rejected for four reasons, indocs/agent-state-isolation.md:todos/, buttelemetry/andbackups/both carry per-session identifiers. Future drift is silent..claude.jsonsits at$HOME, outside.claude/, so it needs a file-level bind mount, and the CLI rewrites it via backup-and-replace..claude.jsoninto a 0-byte file). The knob-based approach leaves it byte-for-byte unchanged._copy_claude_transcript_from_auth_volume, which reads from the auth volume in a separate container that does not carry the masks — and whose[ -f "$src" ] || exit 0guard means it fails silently.Separately, requirement "the CLI still reports itself authenticated" cannot be met for claude without a real grant, since it validates the credential shape. Proving isolation by risking the credential it protects is a bad trade.
Residue
Relocation is partial, and what still crosses candidates is named per backend in
helix.agent_state.UNRELOCATED_AGENT_STATErather than left implicit. The significant case is codex's session rollout transcript (.codex/sessions/<date>/rollout-*.jsonl, ~28 KB/run) plusshell_snapshots/andmemories/:sqlite_homedoes not cover them and the CLI exposes no separate knob — onlyCODEX_HOME, which movesauth.jsontoo.Verification
Real containers, synthetic credentials in throwaway volumes, no login, no model calls, network
none. Before/after listings are names/modes/sizes only./helix-state/codex/; no.sqlitein the shared volume;codex login status→Logged in using an API keycli-config.jsonlands in/helix-state/cursor/; shared volume byte-for-byte unchanged; status reports authenticatedopencode.db+-wal/-shmland in/helix-state/opencode/;auth listreads~/.local/share/opencode/auth.json→1 credentials; thelocks/dir stays in the shared volumeThe five real
helix-auth-*volumes were confirmed intact by name count (5 → 5); every volume created during verification was removed.New:
tests/integration/test_agent_state_isolation.py(markerdocker_integration) pins all of the above, including both rejected knobs, and asserts by source inspection that no test can name a real login volume.2. Warm the shared credential once per generation, and name credential failures
The problem
The shared mount is what lets a CLI refresh its token — but not every CLI serialises that refresh. Measured against the shipped
codexCLI (codex-cli 0.130.0) with a synthetic credential in a throwaway volume and a local single-use token endpoint, five simultaneous candidates against one stale credential produce:A whole generation can die without saying anything. Today the only thing keeping that window narrow is codex's own
last_refreshfield acting as an accidental serialiser.The change — warm once per generation
Run the refresh once, before the generation dispatches anything, through the sandboxed auth container that already exists and is already a single writer (
helix.sandbox.run_sandbox_auth_command). Same measurement with the warm in front:Per generation, not per run. A run outlives any refresh interval, so a single warm at startup stops protecting it the moment the credential next goes stale mid-flight. It sits above the merge/mutate split so it covers every path that dispatches a candidate.
It must be free. This runs on the operator's paid account every generation, so a status/whoami command that quietly cost a request would be worse than the race. That check is what decides whether a backend is warmed at all.
Which backends are warmed, and why the others are not
codexcodex debug models— see belowclaudecursoropencodeoauth-type credential only from inside the fetch wrapper that issues a model request (read from opencode-ai 1.14.24). No command performs that refresh without also invoking a model.opencode providers listwas measured free — it completes with--network noneagainst an expired credential and leavesauth.jsonbyte-identical — and for exactly that reason refreshes nothing.api-type credentials never refresh.geminigemini --version, which touches no credential; no free command is known to take the refresh path, and there is no credential to measure one against. Not warmed on a guess.Every reason is recorded in
helix.backends.CREDENTIAL_WARM_SKIP_REASONS, and a test asserts each backend is either warmed or explained — a new backend cannot fall through silently.The warm command is not
codex login statuscodex login statusnever takes the refresh path. Measured with a synthetic credential in a throwaway volume, it printsLogged in using ChatGPTand exits 0 without issuing a single request, whether the storedlast_refreshis minutes or 30 days old. Warming with it would look like protection while providing none.codex debug modelsrenders the CLI's built-in model catalog and loads auth through the refreshing path. It is free:--network none, writes nothing to the login volume, and makes no request at all — three consecutive warms produced 0 token exchanges and a byte-identicalauth.json;No model is invoked and no quota is consumed either way. stdout is discarded (~200 KB of catalog) since the command is run for its side effect; stderr is kept so a failure stays diagnosable.
A failed warm is logged and the run continues — candidates may still succeed on the credential already stored — but the log says what protection was lost, not just that a command exited non-zero.
The second half — make the failure visible
There was no auth-failure detection at all, and the structured result envelope's
is_errorfield was never read anywhere insrc/. A credential-exhaustion failure was indistinguishable from "the agent wrote bad code": the mutation was abandoned, the slot dropped, and nothing said why.CredentialRefreshErroris now its own failure kind — a sibling ofMutationError(a code failure) andRateLimitError(a quota failure that clears on its own). Detection is anchored on wording read out of the shipped binaries, not guessed:your access token could not be refreshedfailed to refresh token while getting accountchatgpt account id not available, please re-run `codex login`token refresh failed:user oauth refresh failedapi error: 401 invalid api keyEvery marker is a whole distinctive clause. Matching a bare number or a common word (
401,token,auth) is the false-positive trap this repo has already paid for once.is_errornarrows, it never classifies. Atool_resultcarryingis_erroris usually just the agent's own failing shell command — an ordinary code failure. So on a zero exit onlyis_error-flagged envelope text is scanned; scanning the raw streams there would let a candidate that merely edited an OAuth code path talk HELIX into declaring the operator's login broken. On a non-zero exit the raw streams are read too: the invocation already failed, and the only question left is which kind of failure it was.The verdict surfaces where an operator sees it — the proposal slot says the login failed rather than the code, and the permanent end-of-run summary repeats it after the live display is gone, naming the affected candidates and the remedy.
No config knob: a credential that cannot be refreshed is never the behaviour anyone wants, and a warm that costs nothing needs no opt-out.
Verification
Real containers, synthetic credentials in throwaway volumes, a local fake token endpoint, no login and no model calls. New tests:
tests/unit/test_credential_warm.py— registry completeness, single-writer args, failure is never fataltests/unit/test_credential_warm_loop.py— one warm per generation, warm precedes every mutation, the run survives a credential failure and says sotests/unit/test_credential_failure_classification.py— every real CLI string classified; ordinary candidate output (401,token, a failingtest_oauth.py, a diff addingrefresh_token) classified as not a credential failuretests/integration/test_credential_warm_docker.py(docker_integration) — the warm is free (clean exit under--network none), is a no-op on a fresh credential (auth.jsondigest unchanged, no files added), and leaves no residue when repeatedThe five real
helix-auth-*volumes were confirmed intact by name; every volume, network and container created during verification was removed.Gates
Review fixes
Five focused commits on top of
3af85bd, one per review finding:CredentialRefreshErrorescapingmerge()(18a19fb) —merger.merge()now labels the operation, removes the merge worktree and re-raises, exactly asmutate()does. The merge call site inrun_evolutioncatches it, records the slot inCredentialFailureLog, prints the panel and falls through to mutation.cli.evolve/cli.resumegain a last-resort handler (panel + resume hint, exit 2) so it can never surface as a raw traceback. Tests:test_merger.py,test_credential_warm_loop.py(end-to-end via the merge gate), newtest_cli_credential_failure.py.c69ff7c) —run_sandbox_auth_commandacceptstimeoutandcontainer_name, emits--name, and onTimeoutExpiredforce-removes the named container before re-raising (killing the docker client does not stop the container).warm_backend_credentialalways runs undercredential_warm_timeout()— a 300 s cap thatsandbox.timeout_secondscan only tighten — and reports a hang as a distinct, non-fatalCredentialWarmResult(timed_out=True); the loop-level warning says "timed out" instead of "(exit None)". Tests mocksubprocess.runraisingTimeoutExpired.0f66bc7) — that suffix is now a separate transient marker matched ahead of the generic prefix, andCredentialRefreshError.transientcarries it.invoke_claude_coderetries exactly once against the credential the race winner just stored (both the exit-1 stderr path and the exit-0is_errorenvelope path). A second loss is raised with a suggestion that points athelix resumeand does not tell the operator to re-login; the per-slot message and end-of-run summary use the same distinction (re-login advice is kept for expired/revoked/bare wording). Tests cover the marker split, retry-then-success, retry-then-failure, and that non-transient wording is never retried.ac9da92— the per-candidate agent-state tree (whereopencode.dbwith its OAuth tokens lives while a candidate runs) is created mode0700in its own right, on top of themkdtemp(0700) parent it already sat in and thefinally-time removal it already had. Documented indocs/agent-state-isolation.md; mode and removal pinned intest_agent_state.py.CURSOR_CONFIG_DIRseeding from login-time config: not changed. The condition in the finding ("if that is what the credential-copy path already does for other backends") does not hold — this PR deliberately copies no credential or config for any backend; everything stays in the shared volume. Seeding would need an extradocker runagainst the auth volume per candidate and verification against the realcursor-agent, so it is left for a follow-up.18fd2a0—addopts = "-m 'not docker_integration'"inpyproject.toml, so a barepytestno longer collectstests/integration/;pytest -m docker_integration tests/integration/still opts in (verified: 10 collected). README says how.Gates (after fixes)
Branch already contains
origin/main(0 commits behind). Conflicts with PR #71 (feat/agy-backend-and-sandbox-fixes) inREADME.md,src/helix/mutator.py,tests/unit/test_mutator.py— to be sequenced by the owner.