diff --git a/CHANGELOG.md b/CHANGELOG.md index 5b519c82..7bc556fc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,24 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Added +- The agent CLIs' own cross-session memory is switched off for every + candidate, sandboxed or not: `CLAUDE_CODE_DISABLE_AUTO_MEMORY=1` for `claude` + and `-c features.memories=false` for `codex` (the other backends recalled + nothing from a prior session on their own). An explicit `[env]` value for + the same key wins. This is not a session-isolation guarantee: the login + volume is shared by design, and a candidate with shell access can still + write the shared HOME's config files (`~/.claude/CLAUDE.md`, + `~/.claude/settings.json`, `~/.codex/AGENTS.md`, `~/.codex/config.toml`), + which later sessions load. Transcripts stay in the `helix-auth-` + volume. +- Sandboxed runs with more than one concurrent candidate warm the shared + `codex` credential once per generation under a single writer, verified by + reading `last_refresh` back from `auth.json`; a credential failure is now + its own error kind (`CredentialRefreshError`), a lost refresh race is + retried once from a fresh worktree, and both are named in the end-of-run + summary. + ### Changed - **BREAKING**: Removed the `gemini` mutation backend and replaced it with `agy` (Google's Antigravity CLI). Configs with `agent.backend = "gemini"` diff --git a/README.md b/README.md index 9a762d72..28298f45 100644 --- a/README.md +++ b/README.md @@ -645,6 +645,33 @@ evaluator uses a local proxy, keep that endpoint in your evaluator code as usual. Docker Desktop supports `host.docker.internal`; Linux users can set `add_host_gateway = true`. +HELIX stops the agent CLIs' own cross-session memory: it sets +`CLAUDE_CODE_DISABLE_AUTO_MEMORY=1` for `claude` (its auto-memory is keyed by +repo root and would otherwise span candidates) and passes +`-c features.memories=false` to `codex`; `agy`, `cursor`, and `opencode` were +probed and recalled nothing from a prior session on their own, so they need no +switch. Both switches apply to sandboxed and unsandboxed runs, and an explicit +`[env]` value for the same key wins. This is not a session-isolation guarantee: +the login volume is shared by design, so a candidate with shell access can +still write the shared HOME's config files (`~/.claude/CLAUDE.md`, +`~/.claude/settings.json`, `~/.codex/AGENTS.md`, `~/.codex/config.toml`), +which later sessions load. Transcripts and session databases remain in the +`helix-auth-` volume, so operators can read them after a run. + +When several candidates can write the shared login at once, HELIX refreshes +the `codex` credential once per generation under a single writer before +dispatching anything (`codex debug models`, verified by reading `last_refresh` +back from `auth.json`), so the credential is fresh at the start of the +generation. A token that crosses its refresh threshold during the generation +can still be raced by candidates in flight; a lost race is retried once from +a fresh worktree and reported in the end-of-run summary. + +The container-backed tests in `tests/integration/` run real backend images, +so a bare `pytest` does not collect them; opt in with +`pytest -m docker_integration tests/integration/` (set +`HELIX_DOCKER_TESTS_STRICT=1` to fail rather than skip when Docker or an image +is missing). + By default HELIX chooses a published backend-specific mutator image from `agent.backend`: `ghcr.io/ke7/helix-evo-runner-agy`, `ghcr.io/ke7/helix-evo-runner-claude`, diff --git a/pyproject.toml b/pyproject.toml index 1110dc24..f287a1b2 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -81,6 +81,12 @@ testpaths = ["tests"] # (a now-fixed NB-2-style infinite loop that wedged CI for 24 minutes). timeout = 60 timeout_method = "thread" +# tests/integration/ runs real backend containers and is not run in CI. Keep +# a bare `pytest` from collecting it by default; opt in with +# `pytest -m docker_integration tests/integration/` (a command-line -m +# overrides this). +addopts = "-m 'not docker_integration'" markers = [ "diff_harness: differential-testing harness (phases 2-4)", + "docker_integration: runs real containers; needs a Docker daemon and the backend images", ] diff --git a/src/helix/backends.py b/src/helix/backends.py index c724da66..a07806dd 100644 --- a/src/helix/backends.py +++ b/src/helix/backends.py @@ -73,6 +73,43 @@ "opencode": ("OPENCODE_API_KEY", "OPENAI_API_KEY", "ANTHROPIC_API_KEY"), } +# Environment that stops the agent CLIs' own cross-session memory. +# +# Candidates share one login volume per backend (mounted at /home/node), so a +# CLI that reads memory from an earlier session would carry state from +# candidate N-1 into candidate N. Probed 2026-09-10 against the real CLIs: +# plant a fact in one one-shot session, ask for it in a fresh one, same cwd, +# no tools. That probe measures the spontaneous channel only -- what a CLI +# recalls on its own. It is not a session-isolation guarantee: a candidate +# with shell access can still write the shared HOME's config files +# (``~/.claude/CLAUDE.md``, ``~/.claude/settings.json``, ``~/.codex/AGENTS.md``, +# ``~/.codex/config.toml``), which later sessions load. That channel is +# deliberately left open, because the volume is shared by design so that +# transcripts and a refreshed login persist across candidates. +# +# Applies to sandboxed and unsandboxed runs alike (``invoke_claude_code`` +# sets it on the backend environment either way). An operator who names the +# same key in ``[env]`` wins: the value here is a default, not an override. +# claude recalled it -- auto-memory, keyed by repo root, so it spans +# worktrees and the shared HOME; CLAUDE_CODE_DISABLE_AUTO_MEMORY=1 +# stops it and no memory directory is created. +# codex did not; its ``memories`` feature flag is off by default and is +# pinned off in argv (``-c features.memories=false``, see +# ``helix.mutator._build_backend_args``) because it is a config +# override, not an environment variable. +# agy, cursor, opencode did not; they read nothing from a prior session, +# so there is nothing to disable and no entry here. +# Transcripts and session databases are still written to the login volume so +# they can be read after a run. +BACKEND_FRESH_SESSION_ENV: dict[str, dict[str, str]] = { + "claude": {"CLAUDE_CODE_DISABLE_AUTO_MEMORY": "1"}, +} + +# Every shell entry below is ``sh -c``, never ``sh -lc``: a login shell sources +# ``/etc/profile`` and ``$HOME/.profile`` from the shared login volume, which +# every candidate container mounts read-write, so ``-l`` would let a candidate +# plant code that runs in the next auth command. PATH is pinned with ``-e`` by +# ``helix.sandbox.sandbox_auth_docker_args``; nothing here needs a profile. BACKEND_AUTH_COMMANDS: dict[str, dict[str, list[str]]] = { "agy": { # No dedicated non-interactive login subcommand; the bare interactive @@ -85,7 +122,7 @@ # confirmed against a completed sign-in. "status": [ "sh", - "-lc", + "-c", 'set -eu; test -s "${HOME:-/home/node}/.gemini/antigravity-cli/antigravity-oauth-token"', ], # Surgical: only remove agy's own state directory. ``~/.gemini`` also @@ -93,7 +130,7 @@ # would destroy state this backend does not own. "logout": [ "sh", - "-lc", + "-c", 'set -eu; rm -rf "${HOME:-/home/node}/.gemini/antigravity-cli"', ], }, @@ -106,7 +143,7 @@ # localised in some CLI versions). "status": [ "sh", - "-lc", + "-c", "set -eu; " "claude auth status --text 2>&1 || true; " 'test -s "${HOME:-/home/node}/.claude/.credentials.json"', @@ -117,6 +154,38 @@ "login": ["codex", "login", "--device-auth"], "status": ["codex", "login", "status"], "logout": ["codex", "logout"], + # Credential warm -- see CREDENTIAL_WARM_SKIP_REASONS below for why + # codex is the only backend with one. + # + # NOT ``codex login status``. Measured against codex-cli 0.130.0 with + # a synthetic credential in a throwaway volume: ``codex login status`` + # prints "Logged in using ChatGPT" and exits 0 without issuing a single + # request, whether the stored ``last_refresh`` is minutes or 30 days + # old. It reads auth.json; it never takes the refresh path, so warming + # with it would be a placebo. + # + # ``codex debug models`` renders the CLI's model catalog. It loads + # auth through the refreshing path, so it performs the refresh this + # warm exists to perform, and it is free: + # * with a fresh credential it completes with ``--network none`` and + # makes no request at all -- the only thing it may write to the + # login volume is its own ``~/.codex/models_cache.json`` (a + # catalog cache with a 5-minute TTL), never the credential; + # * with a stale credential its only request is the OAuth token + # exchange, which the refreshed credential is then written back + # from. No model is invoked and no quota is consumed either way. + # Its exit code says nothing about the refresh: a rejected exchange is + # logged and swallowed and the command still exits 0. That is why + # ``helix.sandbox.warm_backend_credential`` reads ``last_refresh`` + # back from ``auth.json`` and only reports ``warmed`` when the + # credential is verifiably inside codex's refresh interval. There is + # no flag to bypass the catalog cache (``--bundled`` does the + # opposite: it skips the refresh), so a cache younger than 5 minutes + # can short-circuit the refresh; the read-back catches that too. + # stdout is discarded because the catalog is ~200 KB and the command is + # run for its side effect on the credential, not for its output; + # stderr is kept so a failure stays diagnosable. + "warm": ["sh", "-c", "set -eu; codex debug models >/dev/null"], }, "cursor": { "login": ["cursor-agent", "login"], @@ -131,5 +200,87 @@ } +# --------------------------------------------------------------------------- +# Per-generation credential warm +# --------------------------------------------------------------------------- +# +# Every candidate container mounts the shared login volume read-write, which is +# what lets a backend CLI refresh its own OAuth token and keep the refreshed +# credential for the next candidate. The hazard is the *first* moment after a +# credential goes stale: several candidates start at once, each decides +# independently that a refresh is due, and each posts the same single-use +# refresh token. One wins; the rest are told the token was already consumed. +# +# ``helix.sandbox.warm_backend_credential`` narrows that window by running the +# command below once, in one container, before a generation dispatches any +# candidate -- so a refresh that is due at that moment happens under a single +# writer. The credential is then fresh at the *start* of the generation; a +# token that crosses its refresh threshold during the generation (parent +# evaluations run before each mutation, and queued slots start later still) +# can still be raced by the candidates in flight. The warm is skipped when +# at most one candidate can write the shared login at a time, since a single +# writer cannot race itself. +# +# A backend is warmed only when a command exists here that (a) actually takes +# the CLI's refresh path and (b) costs nothing. Both halves are load-bearing: +# a command that never refreshes buys no safety, and a command that bills the +# operator's account once per generation would be worse than the race it +# prevents. Backends with no entry are listed in +# CREDENTIAL_WARM_SKIP_REASONS with the reason they need none. + + +CREDENTIAL_WARM_SKIP_REASONS: dict[str, str] = { + "claude": ( + "Claude Code serialises its own refresh: it takes a real cross-process " + "lock file, retries while another process holds it, and re-reads the " + "credential afterwards, so concurrent candidates cannot consume the " + "same refresh token. Warming would add a container per generation and " + "remove no hazard." + ), + "cursor": ( + "Cursor Agent never spends its stored refresh token: it re-exchanges " + "an API key instead, so there is no single-use grant for candidates to " + "compete over." + ), + "agy": ( + "No free Antigravity CLI command is known to take the refresh path. " + "The registered status probe is a file test " + "(`test -s ~/.gemini/antigravity-cli/antigravity-oauth-token`), which " + "touches no credential path, so warming with it would be a placebo. " + "`agy models` exits 0 even when logged out, and whether it takes the " + "refresh path has not been measured; no agy credential exists to " + "measure one against. Left unwarmed deliberately rather than warmed " + "on a guess." + ), + "opencode": ( + "OpenCode refreshes an `oauth`-type credential only from inside the " + "fetch wrapper that issues a model request -- read from opencode-ai " + "1.14.24, which refreshes when `expires` has passed and writes the new " + "credential back unlocked. There is therefore no command that performs " + "that refresh without also invoking a model, and a per-generation model " + "call on the operator's account is a worse cost than the race. " + "`opencode providers list` was measured to be free -- it completes with " + "`--network none` against an expired oauth credential and leaves " + "auth.json byte-identical -- but for exactly that reason it refreshes " + "nothing. `api`-type credentials never refresh and are not at risk." + ), +} +"""Why a backend has no ``warm`` entry in :data:`BACKEND_AUTH_COMMANDS`. + +Skipping is a correctness statement, not an optimisation: each entry records +either that the backend cannot lose a refresh race, or that no free command +would win it. +""" + + +def backend_credential_warm_skip_reason(backend: str) -> str | None: + """Return why *backend* is not credential-warmed, or ``None`` if it is.""" + if "warm" in BACKEND_AUTH_COMMANDS.get(backend, {}): + return None + return CREDENTIAL_WARM_SKIP_REASONS.get( + backend, "no credential-warm command is registered for this backend" + ) + + def backend_display_name(backend: str) -> str: return BACKEND_DISPLAY_NAMES.get(backend, backend) diff --git a/src/helix/cli.py b/src/helix/cli.py index 2b30a5a0..456f2420 100644 --- a/src/helix/cli.py +++ b/src/helix/cli.py @@ -26,10 +26,15 @@ print_warning, render_frontier_table, ) -from helix.exceptions import RateLimitError, ResumeIncompatibleError, print_helix_error +from helix.exceptions import ( + CredentialRefreshError, + RateLimitError, + ResumeIncompatibleError, + print_helix_error, +) from helix.lineage import load_lineage from helix.population import EvalResult, FrontierType, ParetoFrontier, Candidate -from helix.state import load_state, save_state +from helix.state import load_state, save_state, state_file_exists from helix.worktree import remove_worktree logger = logging.getLogger(__name__) @@ -109,6 +114,28 @@ def _helix_dir(project_root: Path) -> Path: return project_root / _HELIX_DIR +def _print_credential_failure_hint( + project_root: Path, backend: str, exc: CredentialRefreshError +) -> None: + """Say what to do after a credential failure -- truthfully about state. + + Seedless seed generation is the one path that reaches the CLI handler, + and it fails before the first ``save_state``; promising ``helix resume`` + then sends the operator to a command that starts a fresh run. + """ + from helix.evolution import _credential_remedy + + remedy = _credential_remedy(backend, transient=exc.transient) + if state_file_exists(project_root): + print_error(f"Evolution state has been saved. {remedy}") + return + print_error( + "No evolution state was saved: the failure happened before the first " + "generation completed, so there is nothing to resume. " + + remedy.replace("[cyan]helix resume[/cyan]", "[cyan]helix evolve[/cyan]") + ) + + def _print_cleanup_hint() -> None: print_info( "HELIX worktrees and saved state remain on disk after the run. " @@ -695,6 +722,16 @@ def evolve( "Run [cyan]helix resume[/cyan] to continue when rate limits clear." ) raise SystemExit(2) + except CredentialRefreshError as exc: + # Every in-loop path handles this itself (the slot is skipped and the + # run continues), so reaching here means a path that does not -- in + # practice seedless seed generation, which runs before any state has + # been saved. Show the panel with its suggestion instead of a raw + # traceback, and only promise a resume when there is a state file. + logger.error("Credential failure escaped the evolution loop: %s", exc) + print_helix_error(exc) + _print_credential_failure_hint(project_root, config.agent.backend, exc) + raise SystemExit(2) except KeyboardInterrupt: _handle_keyboard_interrupt(project_root) else: @@ -1237,6 +1274,11 @@ def resume(config_path: str, project_dir: Path | None) -> None: "Run [cyan]helix resume[/cyan] again when rate limits clear." ) raise SystemExit(2) + except CredentialRefreshError as exc: + logger.error("Credential failure escaped the resumed loop: %s", exc) + print_helix_error(exc) + _print_credential_failure_hint(project_root, config.agent.backend, exc) + raise SystemExit(2) except KeyboardInterrupt: _handle_keyboard_interrupt(project_root) else: diff --git a/src/helix/evolution.py b/src/helix/evolution.py index a7926e21..086f0e91 100644 --- a/src/helix/evolution.py +++ b/src/helix/evolution.py @@ -14,10 +14,12 @@ import traceback from collections.abc import Callable from concurrent.futures import ThreadPoolExecutor, as_completed -from dataclasses import replace +from dataclasses import dataclass, field, replace from pathlib import Path from typing import Any +from rich.markup import escape + from helix.batch_sampler import ( BatchSampler, @@ -47,7 +49,9 @@ set_phase, ) +from helix.backends import backend_display_name from helix.exceptions import ( + CredentialRefreshError, HelixError, PromptArtifactCollisionError, RateLimitError, @@ -78,7 +82,11 @@ HelixResult, ParetoFrontier, ) -from helix.sandbox import start_evaluator_sidecar +from helix.sandbox import ( + CredentialWarmResult, + start_evaluator_sidecar, + warm_backend_credential, +) from helix.state import ( BudgetState, clear_eval_cache, @@ -1447,6 +1455,231 @@ def _plan_proposals( # upstream's ``ReflectiveMutationProposer.propose``, though upstream # batches these stages across all sampled tasks per iteration instead # of running one call per proposal slot. +@dataclass +class CredentialFailureLog: + """Every credential failure seen during a run, in the order observed. + + Proposal workers run in a thread pool, so the list is appended under a + lock. The log exists so a run that dies from an unusable login says so + once, plainly, in the permanent end-of-run summary -- not only in a + per-slot error that has already scrolled past by the time the run ends. + + ``recovered`` holds the invocations that lost a refresh race and + succeeded on their one retry: not failures, but the only evidence an + operator gets that the warm is not protecting the run. + """ + + entries: list[tuple[str, str, bool, str]] = field(default_factory=list) + recovered: list[tuple[str, str]] = field(default_factory=list) + _lock: threading.Lock = field(default_factory=threading.Lock, repr=False) + + def record( + self, + candidate_id: str, + message: str, + *, + transient: bool = False, + kind: str = "mutation", + ) -> None: + with self._lock: + self.entries.append((candidate_id, message, transient, kind)) + + def record_recovered(self, candidate_id: str, message: str) -> None: + with self._lock: + self.recovered.append((candidate_id, message)) + + def __len__(self) -> int: + with self._lock: + return len(self.entries) + + def candidate_ids(self) -> list[str]: + with self._lock: + return [candidate_id for candidate_id, _, _, _ in self.entries] + + def recovered_ids(self) -> list[str]: + with self._lock: + return [candidate_id for candidate_id, _ in self.recovered] + + def last_message(self) -> str: + with self._lock: + return self.entries[-1][1] if self.entries else "" + + def all_transient(self) -> bool: + """True when every failure was a lost refresh race, not a dead login.""" + with self._lock: + return bool(self.entries) and all(t for _, _, t, _ in self.entries) + + def describe_failed(self) -> str: + """``"2 mutation(s) and 1 merge(s)"`` -- what failed, by kind.""" + with self._lock: + kinds = [kind for _, _, _, kind in self.entries] + parts = [ + f"{kinds.count(kind)} {kind}(s)" + for kind in ("mutation", "merge") + if kind in kinds + ] + return " and ".join(parts) if parts else "0 invocation(s)" + + +def _credential_failure_verdict( + backend: str, *, transient: bool, subject: str +) -> str: + """The one operator-facing sentence for a credential failure. + + Used by the proposal worker and the merge gate so the cause is worded + once (the CLI and the summary share :func:`_credential_remedy`); + *subject* names what the failure is **not** about ("the candidate's + code", "the merged code"). + """ + display = backend_display_name(backend) + if transient: + cause = ( + f"the shared {display} credential was refreshed by another " + "candidate first and the retry also failed" + ) + else: + cause = f"the shared {display} credential could not be used or refreshed" + return ( + f"failed because {cause} — this is a login failure, " + f"not a failure of {subject}." + ) + + +def _credential_remedy(backend: str, *, transient: bool) -> str: + """What the operator should do about a credential failure.""" + if transient: + return ( + "The stored login is most likely usable: run " + "[cyan]helix resume[/cyan] first, and only re-authenticate if " + "this keeps recurring." + ) + return ( + f"Re-authenticate with [cyan]helix sandbox login {backend}[/cyan], " + "then [cyan]helix resume[/cyan]." + ) + + +def _generation_concurrent_writers(config: HelixConfig) -> int: + """How many candidates can write the shared login at once in a generation. + + ``num_parallel_proposals * mutations_per_parent`` slots, run under a pool + of at most ``max_workers``; a single slot takes the in-thread path with no + pool at all. The merge branch is one invocation. + """ + slots = ( + config.evolution.num_parallel_proposals + * config.evolution.mutations_per_parent + ) + return max(1, min(slots, config.evolution.max_workers)) + + +def _operator_env(config: HelixConfig) -> dict[str, str]: + """The operator's ``passthrough_env`` / ``[env]`` selection, as a mapping.""" + env = { + key: os.environ[key] + for key in config.passthrough_env + if key in os.environ + } + env.update(config.env) + return env + + +def _warm_generation_credential( + config: HelixConfig, *, gen: int, announce_skip: bool +) -> CredentialWarmResult | None: + """Refresh the agent backend's shared credential once for generation *gen*. + + Called once per generation, before any candidate is dispatched, so that a + refresh which has come due happens under one writer and the credential is + fresh at the start of the generation. That is the extent of the + protection: a token that crosses its refresh threshold *during* the + generation -- each worker runs the parent evaluation before its mutation, + and slots beyond ``max_workers`` start later still -- can still be raced + by the candidates in flight. + + Per *generation* rather than once per run on purpose: a long run outlives + any refresh interval, so a single warm at startup stops protecting the run + the moment the credential next goes stale mid-flight. + + Skipped when at most one candidate can write the shared login at a time + (``_generation_concurrent_writers``): a single writer cannot race itself, + and the warm would only add a container per generation. + + Returns ``None`` when there is nothing to warm -- an unsandboxed run has no + HELIX-managed login volume, because the backend runs directly against the + operator's own CLI state and HELIX never mounts or arbitrates it. + + Never fatal. A warm that could not run, or that left the credential + unverified, leaves exactly today's behaviour in place (candidates refresh + for themselves and may race), and candidates may still succeed on the + credential already stored -- so the run continues and the operator is + told, in those terms, what protection was lost. + """ + if not config.sandbox.enabled: + return None + + backend = config.agent.backend + display = backend_display_name(backend) + writers = _generation_concurrent_writers(config) + if writers <= 1: + result = CredentialWarmResult( + backend=backend, + warmed=False, + skip_reason=( + "at most one candidate writes the shared login at a time " + f"(num_parallel_proposals × mutations_per_parent = " + f"{config.evolution.num_parallel_proposals * config.evolution.mutations_per_parent}, " + f"max_workers = {config.evolution.max_workers}); a single " + "writer cannot lose a refresh race to itself" + ), + ) + else: + result = warm_backend_credential( + backend, sandbox=config.sandbox, env=_operator_env(config) + ) + + if result.skipped: + # The reason is a property of the backend or the run's concurrency, + # not of this generation, so say it once per run instead of once per + # generation. + if announce_skip: + logger.info( + "No credential warm for %s: %s", display, result.skip_reason + ) + return result + + if result.warmed: + logger.debug( + "Credential warm for %s completed before generation %d: %s", + display, + gen, + result.detail, + ) + return result + + detail = f" Detail: {escape(result.detail)}" if result.detail else "" + if result.timed_out: + cause = "timed out" + elif result.stale: + cause = "exited 0 but the credential is not verifiably fresh" + elif result.returncode is not None: + cause = f"exit {result.returncode}" + else: + cause = "could not start" + message = ( + f"Credential warm for {display} did not complete before generation " + f"{gen} ({cause}). Candidates in this generation will " + "each decide for themselves whether to refresh the shared login, and " + "if a refresh is due they can spend the same single-use refresh token " + "at once -- the losers of that race can fail without reporting an " + "error of their own. The run continues: the credential already stored " + f"may still be usable.{detail}" + ) + logger.warning("%s", message) + print_warning(message) + return result + + def _run_proposal_worker( pre_ctx: ProposalContext, *, @@ -1458,6 +1691,7 @@ def _run_proposal_worker( evaluator_manifest: dict[str, str], use_minibatch_gate: bool, gen: int, + credential_failures: CredentialFailureLog, ) -> ProposalResult: """Atomic proposal worker — mirrors the sample/evaluate/mutate/evaluate shape of GEPA's ``ReflectiveMutationProposer.propose``. @@ -1563,6 +1797,9 @@ def _run_proposal_worker( ) ), record_usage=_spent_usage.append, + on_refresh_race_recovered=lambda msg: ( + credential_failures.record_recovered(_new_id, msg) + ), ) except Exception as _mu_exc: # Re-raise PromptArtifactCollisionError (fatal for the whole run) @@ -1586,6 +1823,27 @@ def _run_proposal_worker( f"retries — proposal slot skipped. " f"Run [cyan]helix resume[/cyan] when rate limits clear." ) + elif isinstance(_mu_exc, CredentialRefreshError): + # Name the failure for what it is. Without this the slot is + # indistinguishable from a mutation that produced bad code, + # and a whole generation can die quietly on a broken login. + credential_failures.record( + _new_id, str(_mu_exc), transient=_mu_exc.transient + ) + logger.error( + "Mutation %s (parent: %s, gen %d) failed on the shared " + "%s credential, not on its code: %s", + _new_id, _parent.id, gen, + backend_display_name(config.agent.backend), _mu_exc, + ) + print_error( + f"Mutation [bold]{_new_id}[/bold] " + + _credential_failure_verdict( + config.agent.backend, + transient=_mu_exc.transient, + subject="the candidate's code", + ) + ) else: print_error( f"Parallel mutation {_new_id} (parent: {_parent.id}, gen {gen}) " @@ -1689,7 +1947,8 @@ def _dispatch_proposals( f"Worker for proposal {_wid} " f"(parent: {_wparent.id}, gen {gen}) " f"raised an unexpected exception: " - f"{type(_wexc).__name__}: {_wexc} — proposal slot dropped." + f"{type(_wexc).__name__}: {escape(str(_wexc))} — " + "proposal slot dropped." ) worker_results[_widx] = MutationFailedProposal( presample_ctx=_wpctx, @@ -2088,7 +2347,18 @@ def _sync_frontier_state() -> None: candidate_id=seed.id, source="seed_generation", ) - except Exception: + except Exception as _seed_exc: + # The tokens the seed invocation spent before it failed are + # still spent; charge them before the worktree goes, exactly + # as ``merge()`` / ``mutate()`` hand a failed attempt's usage + # to their sink. + if isinstance(_seed_exc, HelixError) and _seed_exc.usage is not None: + budget_api.charge_llm_usage( + state, + _seed_exc.usage, + candidate_id=seed.id, + source="seed_generation_failed", + ) _safe_remove_worktree(seed, label="failed seed generation") raise print_success("Seed generation complete.") @@ -2195,6 +2465,10 @@ def _sync_frontier_state() -> None: # Mutation counters for display mutations_attempted = 0 mutations_accepted = 0 + credential_failures = CredentialFailureLog() + # The credential warm's skip reason is a fact about the backend, not + # about any one generation; announce it once. + credential_warm_skip_announced = False gen = start_gen - 1 while gen < config.evolution.max_generations: @@ -2228,6 +2502,18 @@ def _sync_frontier_state() -> None: print_warning("Budget exhausted -- stopping early.") break + # ---- Credential warm (once per generation) ------------------- + # Sits above the merge/mutate split so it covers every path that + # dispatches a candidate this generation. It makes the shared + # login fresh at the start of the generation; a token that + # crosses its refresh threshold while candidates are in flight + # can still be raced. Skipped when only one writer exists. + _warm = _warm_generation_credential( + config, gen=gen, announce_skip=not credential_warm_skip_announced + ) + if _warm is not None and _warm.skipped: + credential_warm_skip_announced = True + # ============================================================= # GEPA parity (Fix 6/7): Merge OR mutate per iteration. # Merge fires FIRST at the start of the iteration (deferred from @@ -2390,23 +2676,58 @@ def _has_val_support_overlap(i: str, j: str) -> bool: ) merge_usage: list[UsageStats] = [] - merged = merge( - candidate_a=a, - candidate_b=b, - new_id=merge_id, - config=config, - base_dir=worktrees_dir, - background=config.agent.background, - eval_result_a=era, - eval_result_b=erb, - prepare_worktree=lambda cand: ( - _refresh_and_snapshot_protected_evaluator_files( - cand, config, project_root + merge_credential_failed = False + try: + merged = merge( + candidate_a=a, + candidate_b=b, + new_id=merge_id, + config=config, + base_dir=worktrees_dir, + background=config.agent.background, + eval_result_a=era, + eval_result_b=erb, + prepare_worktree=lambda cand: ( + _refresh_and_snapshot_protected_evaluator_files( + cand, config, project_root + ) + ), + ancestor=ancestor_candidate, + record_usage=merge_usage.append, + on_refresh_race_recovered=lambda msg: ( + credential_failures.record_recovered(merge_id, msg) + ), + ) + except CredentialRefreshError as _merge_cred_exc: + # Same treatment the proposal worker gives a + # mutation: the merge worktree is already cleaned + # up by merge(); count and name the failure once, + # then fall through to mutation so the run continues. + merged = None + merge_credential_failed = True + credential_failures.record( + merge_id, + str(_merge_cred_exc), + transient=_merge_cred_exc.transient, + kind="merge", + ) + print_helix_error(_merge_cred_exc) + logger.error( + "Merge %s (%s + %s, gen %d) failed on the shared " + "%s credential, not on its code: %s", + merge_id, a.id, b.id, gen, + backend_display_name(config.agent.backend), + _merge_cred_exc, + ) + print_error( + f"Merge [bold]{merge_id}[/bold] " + + _credential_failure_verdict( + config.agent.backend, + transient=_merge_cred_exc.transient, + subject="the merged code", ) - ), - ancestor=ancestor_candidate, - record_usage=merge_usage.append, - ) + + " Falling through to mutation." + ) if merged is None: # GEPA parity (M2/B3): merge operator failed before @@ -2420,12 +2741,17 @@ def _has_val_support_overlap(i: str, j: str) -> bool: candidate_id=merge_id, source="merge_failed", ) - print_error( - f"Merge {merge_id} failed " - f"(candidates: {a.id} + {b.id}, gen {gen}). " - f"Claude Code returned no output or the merge subprocess errored. " - f"Check the HELIX ERROR panel above for full diagnostics." - ) + # A credential failure has already been diagnosed + # above; the generic wording would contradict it. + if not merge_credential_failed: + print_error( + f"Merge {merge_id} failed " + f"(candidates: {a.id} + {b.id}, gen {gen}). " + f"{backend_display_name(config.agent.backend)} " + "returned no output or the merge subprocess " + "errored. Check the HELIX ERROR panel above " + "for full diagnostics." + ) else: if merged.usage: live.update(usage=merged.usage) @@ -2764,6 +3090,7 @@ def _has_val_support_overlap(i: str, j: str) -> bool: evaluator_manifest=evaluator_manifest, use_minibatch_gate=use_minibatch_gate, gen=gen, + credential_failures=credential_failures, ), max_workers=config.evolution.max_workers, gen=gen, @@ -3472,6 +3799,50 @@ def _drop_duplicate_child(gated: GatedProposal) -> bool: render_budget(state.budget, config.evolution) render_frontier_table(frontier, frontier._results) + # A credential failure is not a code failure, and the operator has to be + # able to tell them apart after the fact. The per-slot errors above have + # long scrolled away by now; this line is part of the permanent summary + # that outlives the live display. + _display = backend_display_name(config.agent.backend) + if credential_failures.recovered_ids(): + # Not failures -- but the only visible sign that candidates are + # still racing to refresh the shared login despite the warm. + _recovered_ids = ", ".join(credential_failures.recovered_ids()) + print_warning( + f"{len(credential_failures.recovered_ids())} invocation(s) " + f"recovered after a lost refresh race on the shared {_display} " + f"credential: {_recovered_ids}. Each was retried once from a " + "fresh worktree and succeeded; both attempts' tokens are charged " + "and the first attempt's output is kept beside the retry's " + "(`.attempt1` artifacts). The credential is being refreshed by " + "candidates in flight, which the per-generation warm does not " + "prevent." + ) + if credential_failures: + _failed_ids = ", ".join(credential_failures.candidate_ids()) + _transient = credential_failures.all_transient() + if _transient: + # Every failure was a lost refresh race: the shared login was + # refreshed by another candidate and is most likely fine. Telling + # the operator to re-login here would throw away a working + # credential and teach them to distrust a healthy run. + _cause = ( + "Each lost a refresh race (another candidate refreshed the " + "shared login first) and failed again on its one retry." + ) + else: + _cause = ( + "The backend reported that its stored login could not be " + "used or refreshed." + ) + print_error( + f"{credential_failures.describe_failed()} failed on the shared " + f"{_display} credential, not on their code: {_failed_ids}. " + f"{_cause} " + + _credential_remedy(config.agent.backend, transient=_transient) + + f" Last report: {escape(credential_failures.last_message())}" + ) + best = frontier.best() print_success(f"Evolution complete. Best candidate: {best.id}") diff --git a/src/helix/exceptions.py b/src/helix/exceptions.py index 9e026a37..01920126 100644 --- a/src/helix/exceptions.py +++ b/src/helix/exceptions.py @@ -155,6 +155,37 @@ class RateLimitError(HelixError): """ +class CredentialRefreshError(HelixError): + """Raised when a backend CLI could not use or refresh its stored credential. + + This is a *credential* failure, not a code failure and not a quota failure. + It fires when the agent CLI reports that its OAuth refresh token was + already used, expired, revoked, or is missing -- the state HELIX reaches + when several candidates sharing one login volume all try to spend the same + single-use refresh token at once, or when the operator's login has simply + lapsed. + + It exists so a run that dies this way says so. Without it the failure is + indistinguishable from "the agent wrote bad code": the mutation is + abandoned, the proposal slot is dropped, and the operator sees a generation + that produced nothing with no reason attached. + + Inherits from :class:`HelixError` (not :class:`MutationError`) so the + proposal worker in ``evolution.py`` can route it separately -- the run + continues, but every credential-classified failure is counted and named in + the end-of-run summary. Detection is anchored on distinctive wording read + out of the shipped backend CLIs; see ``helix.mutator``. + + ``transient`` is True when the backend's wording says the refresh token was + *already used* -- the lost-refresh-race outcome, where another candidate + has just stored a fresh credential. That case is retried once before it is + raised, and when it is raised the operator is pointed at ``helix resume`` + rather than at a re-login the stored credential does not need. + """ + + transient: bool = False + + # --------------------------------------------------------------------------- # Formatted error printing # --------------------------------------------------------------------------- diff --git a/src/helix/merger.py b/src/helix/merger.py index 2745b39b..070af5c7 100644 --- a/src/helix/merger.py +++ b/src/helix/merger.py @@ -5,14 +5,25 @@ import math import random from pathlib import Path -from typing import Callable, Mapping +from typing import Any, Callable, Mapping from helix.display import UsageStats from helix.population import Candidate, EvalResult from helix.config import HelixConfig from helix.worktree import clone_candidate, snapshot_candidate, remove_worktree, get_diff # noqa: F401 -from helix.exceptions import MutationError, RateLimitError, print_helix_error -from helix.mutator import invoke_claude_code, AUTONOMOUS_SYSTEM_PROMPT, _turn_budget_section +from helix.exceptions import ( + CredentialRefreshError, + MutationError, + RateLimitError, + print_helix_error, +) +from helix.backends import backend_display_name +from helix.mutator import ( # noqa: F401 + AUTONOMOUS_SYSTEM_PROMPT, + _turn_budget_section, + invoke_claude_code, + invoke_with_refresh_race_retry, +) # --------------------------------------------------------------------------- # Merge-acceptance subsample selection (GEPA parity) @@ -235,6 +246,7 @@ def merge( prepare_worktree: Callable[[Candidate], None] | None = None, ancestor: Candidate | None = None, record_usage: Callable[[UsageStats], None] | None = None, + on_refresh_race_recovered: Callable[[str], None] | None = None, ) -> Candidate | None: """Merge *candidate_a* and *candidate_b* using Claude Code. @@ -303,17 +315,25 @@ def merge( Optional sink called exactly once with the backend's token usage, whether or not the merge produced a usable candidate — the same contract as :func:`helix.mutator.mutate`'s parameter of that name. + on_refresh_race_recovered: + Optional sink told when the merge lost a refresh race on the shared + credential and succeeded on its one retry from a fresh worktree; the + same contract as :func:`helix.mutator.mutate`'s parameter. Returns ------- Candidate | None The merged candidate on success, or ``None`` on failure. """ - child = clone_candidate(candidate_a, new_id, base_dir) - child.operation = "merge" - child.parent_ids = [candidate_a.id, candidate_b.id] - if prepare_worktree is not None: - prepare_worktree(child) + def _fresh_child() -> Candidate: + fresh = clone_candidate(candidate_a, new_id, base_dir) + fresh.operation = "merge" + fresh.parent_ids = [candidate_a.id, candidate_b.id] + if prepare_worktree is not None: + prepare_worktree(fresh) + return fresh + + child = _fresh_child() # Diff-rendering mode selection. ``ancestor`` available → compute # the two ancestor-relative diffs that drive the GEPA-style @@ -342,14 +362,39 @@ def merge( diff_b_from_ancestor=diff_b_from_ancestor, ) - try: - _, usage = invoke_claude_code( - child.worktree_path, + def _invoke( + target: Candidate, retried: bool + ) -> tuple[dict[str, Any], UsageStats]: + return invoke_claude_code( + target.worktree_path, prompt, config.agent, passthrough_env=config.passthrough_env, fixed_env=config.env, sandbox=config.sandbox, + retried=retried, + ) + + def _replace(fresh: Candidate) -> None: + nonlocal child + child = fresh + + def _discard_child() -> None: + try: + remove_worktree(child) + except Exception: + pass + + try: + child, usage = invoke_with_refresh_race_retry( + child, + backend_name=backend_display_name(config.agent.backend), + invoke=_invoke, + fresh_child=_fresh_child, + remove_child=remove_worktree, + on_child_replaced=_replace, + record_usage=record_usage, + on_refresh_race_recovered=on_refresh_race_recovered, ) child.usage = usage if record_usage is not None: @@ -361,19 +406,32 @@ def merge( record_usage(exc.usage) exc.operation = f"merge {new_id} ({candidate_a.id} + {candidate_b.id})" print_helix_error(exc) - try: - remove_worktree(child) - except Exception: - pass + _discard_child() return None except RateLimitError as exc: # Rate limit — clean up orphaned worktree, then re-raise. if record_usage is not None and exc.usage is not None: record_usage(exc.usage) - try: - remove_worktree(child) - except Exception: - pass + _discard_child() + raise + except CredentialRefreshError as exc: + # The stored login, not this merge, is what failed. Mirror + # ``mutate()``: clean up the orphaned worktree and re-raise so the + # merge call site in evolution.py can count it as a credential + # failure and fall through to mutation instead of dying with a + # traceback and a leaked worktree. The tokens spent before the + # credential gave out are still spent, so hand them to the sink + # first, exactly as the other error paths do. + if record_usage is not None and exc.usage is not None: + record_usage(exc.usage) + exc.operation = f"merge {new_id} ({candidate_a.id} + {candidate_b.id})" + _discard_child() + raise + except Exception: + # A non-HELIX exception (sandbox ``TimeoutExpired``, ``OSError``) + # carries no usage; an earlier attempt's spend has already reached + # the sink. Do not leak the worktree on the way out. + _discard_child() raise # NOTE: snapshot_candidate() is intentionally NOT called here. diff --git a/src/helix/mutator.py b/src/helix/mutator.py index 407ca7cf..373992ec 100644 --- a/src/helix/mutator.py +++ b/src/helix/mutator.py @@ -6,15 +6,22 @@ import logging import os import shlex +import shutil import subprocess +import tempfile from pathlib import Path from typing import Any, Callable -from helix.backends import BACKEND_AUTH_ENV, backend_display_name +from helix.backends import ( + BACKEND_AUTH_ENV, + BACKEND_FRESH_SESSION_ENV, + backend_display_name, +) from helix.display import UsageStats from helix.population import Candidate, EvalResult from helix.config import AgentConfig, HelixConfig, SandboxConfig from helix.exceptions import ( + CredentialRefreshError, HelixError, MutationError, PromptArtifactCollisionError, @@ -23,7 +30,11 @@ ) from helix.executor import _scrub_environment from helix.lines import split_lf_lines -from helix.sandbox import resolve_sandbox_image, run_sandboxed_command +from helix.sandbox import ( + OPENCODE_STATE_DIR_NAME, + resolve_sandbox_image, + run_sandboxed_command, +) from helix.worktree import clone_candidate, snapshot_candidate, remove_worktree # noqa: F401 logger = logging.getLogger(__name__) @@ -598,6 +609,277 @@ def _looks_like_rate_limit(text: str) -> bool: return any(kw in lower for kw in _RATE_LIMIT_KEYWORDS) +# --------------------------------------------------------------------------- +# Credential / refresh-exhaustion detection +# --------------------------------------------------------------------------- +# +# Distinct from the rate-limit detection above. A rate limit means "come back +# later on the same credential"; these markers mean "the credential itself can +# no longer be used", which is what a lost refresh race looks like from inside +# a candidate. Telling them apart matters because only the second one is +# unrecoverable without operator action, and neither is a code failure. +# +# Every marker below is a phrase read out of the shipped CLI it belongs to, not +# a guess at the wording. They are deliberately whole distinctive sentences or +# clauses: matching a bare number or a single common word ("401", "token", +# "auth") is the false-positive trap this repo has already paid for once -- +# a candidate whose own diff or test output mentions tokens must never be +# reported to the operator as a broken login. +# Markers whose failure is *transient*: the credential is not broken, this +# invocation merely lost a refresh race. Checked before the general markers so +# the more specific wording is what gets reported, and so the caller can retry +# once against the credential the winner has just stored. +_TRANSIENT_CREDENTIAL_FAILURE_MARKERS: tuple[str, ...] = ( + # Codex CLI (codex-cli 0.130.0): the suffix it appends when another + # process spent the single-use refresh token first. The full sentence is + # "Your access token could not be refreshed because your refresh token + # was already used. Please log out and sign in again." -- the "log out" + # advice is the CLI's, and is wrong for this case: the shared auth.json + # already holds the refreshed credential. + "because your refresh token was already used", +) + +# KNOWN RISK: every marker is pinned to the prose of a specific CLI release +# (versions noted per entry), while the sandbox images install the unpinned +# CLI package and are rebuilt on a schedule. A wording change upstream turns +# a credential failure back into an unclassified one -- it never produces a +# false positive. No test reads real CLI output; when a marker stops matching, +# the symptom is a generic MutationError whose stderr carries the new wording. +_CREDENTIAL_FAILURE_MARKERS: tuple[str, ...] = ( + # Codex CLI (codex-cli 0.130.0). One prefix covers every suffix the CLI + # appends: "... has expired." / "... was revoked." / "... because you have + # since logged out or signed in to another account." / the bare + # "Your access token could not be refreshed. Please log out and sign in + # again." (The already-used suffix is matched first, above, as transient.) + "your access token could not be refreshed", + "failed to refresh token while getting account", + "chatgpt account id not available, please re-run `codex login`", + # OpenCode (opencode-ai 1.14.24) -- thrown by the provider fetch wrapper + # when the OAuth token exchange is rejected, e.g. "Token refresh failed: + # 400". The colon is kept so the phrase cannot match narrative prose. + "token refresh failed:", + # Claude Code (2.1.138). + "user oauth refresh failed", + "api error: 401 invalid api key", +) + + +def credential_failure_marker(text: str) -> str | None: + """Return the credential-failure marker *text* contains, or ``None``. + + Returns the matched marker rather than a bool so callers can name the + evidence in the operator-facing message instead of asserting a verdict + with nothing behind it. + """ + if not text: + return None + lower = text.lower() + for marker in _TRANSIENT_CREDENTIAL_FAILURE_MARKERS + _CREDENTIAL_FAILURE_MARKERS: + if marker in lower: + return marker + return None + + +def credential_failure_is_transient(marker: str) -> bool: + """True when *marker* names a lost refresh race rather than a dead login.""" + return marker in _TRANSIENT_CREDENTIAL_FAILURE_MARKERS + + +#: Event types under which the JSONL backends report a failure of the +#: invocation itself, as opposed to a tool call the agent made. Read from the +#: shipped CLIs' own event definitions: +#: +#: codex ``{"type": "error", "message": ...}`` and +#: ``{"type": "turn.failed", "error": {"message": ...}}`` +#: (codex-rs ``exec/src/exec_events.rs``: ``ThreadErrorEvent``, +#: ``TurnFailedEvent``). ``codex exec --json`` never emits an +#: ``is_error`` key. +#: opencode ``{"type": "error", ..., "error": {"name": ..., "data": +#: {"message": ...}}}`` -- the ``session.error`` event that +#: ``opencode run --format json`` re-emits (``cli/cmd/run.ts``). +#: cursor no documented failure event; a ``type: "error"`` line carrying +#: ``message`` / ``error`` is read the same way. +_STRUCTURED_ERROR_EVENT_TYPES: frozenset[str] = frozenset( + {"error", "turn.failed", "session.error"} +) + + +def _error_field_texts(value: Any) -> list[str]: + """Flatten an ``error``-shaped field into the message strings it carries. + + Accepts the shapes the backends use: a bare string, a list of strings + (Claude Code's ``errors``), or an object with ``message`` / ``error`` / + ``data.message`` (opencode's ``{name, data: {message}}``). Anything else + contributes nothing -- an unknown shape is not evidence. + """ + if isinstance(value, str): + return [value] if value else [] + if isinstance(value, list): + texts: list[str] = [] + for item in value: + texts.extend(_error_field_texts(item)) + return texts + if isinstance(value, dict): + texts = [] + for key in ("message", "error"): + texts.extend(_error_field_texts(value.get(key))) + data = value.get("data") + if isinstance(data, dict): + texts.extend(_error_field_texts(data.get("message"))) + return texts + return [] + + +def _structured_error_texts( + backend: str, parsed: dict[str, Any] | None, stdout: str +) -> list[str]: + """Return every failure the backend reported through a *structured* field. + + This is the only stdout-derived text the credential classifier is ever + asked about. It is deliberately narrow: the fields read here are the ones + the CLI itself uses to say "this invocation failed", never the agent's + prose and never a tool's captured output. A candidate whose test suite + prints ``Token refresh failed: 400`` into a ``command_execution`` item, or + whose final message quotes a 401, must not be able to talk HELIX into + declaring the operator's login dead. + + JSONL backends (codex, cursor, opencode): the ``message`` / ``error`` of + events whose type is in :data:`_STRUCTURED_ERROR_EVENT_TYPES`. When the + strict parse has not happened yet (non-zero exit), the stream is read + leniently so a malformed line elsewhere does not hide the error event. + + Envelope backends (claude, agy): the envelope's ``errors`` list and + ``error`` field always, and ``result`` only when the envelope flags + ``is_error`` -- on a successful turn ``result`` is the assistant's prose. + """ + if backend in {"codex", "cursor", "opencode"}: + events: Any = parsed.get("events") if parsed is not None else None + if not isinstance(events, list): + events = _parse_jsonl_output( + stdout, + backend=backend, + cmd_str="", + worktree_path="", + stderr="", + exit_code=0, + strict=False, + )["events"] + texts: list[str] = [] + for event in events: + if not isinstance(event, dict): + continue + if event.get("type") not in _STRUCTURED_ERROR_EVENT_TYPES: + continue + texts.extend(_error_field_texts(event.get("message"))) + texts.extend(_error_field_texts(event.get("error"))) + return texts + + envelope: dict[str, Any] | None = parsed + if envelope is None: + try: + loaded = json.loads(stdout) if stdout.strip() else None + except (json.JSONDecodeError, ValueError, RecursionError): + loaded = None + if isinstance(loaded, dict): + envelope = loaded + if envelope is None: + return [] + texts = [] + texts.extend(_error_field_texts(envelope.get("errors"))) + texts.extend(_error_field_texts(envelope.get("error"))) + if envelope.get("is_error") is True: + texts.extend(_error_field_texts(envelope.get("result"))) + return texts + + +def _credential_failure_evidence( + backend: str, + parsed: dict[str, Any] | None, + result: subprocess.CompletedProcess[str], +) -> tuple[str, str] | None: + """Return ``(marker, where)`` when this invocation is a credential failure. + + Structured error fields (see :func:`_structured_error_texts`) are read on + every exit code: they are how a backend that swallows its own failure and + exits 0 -- codex on a rejected refresh, measured on codex-cli 0.130.0 -- + still says what went wrong. + + On a **non-zero** exit stderr is read as well: CLIs routinely report an + unusable credential there without emitting a structured event. The raw + stdout stream is never scanned on any exit code; for the JSONL backends it + is the whole transcript, tool output and agent prose included. + """ + for text in _structured_error_texts(backend, parsed, result.stdout or ""): + marker = credential_failure_marker(text) + if marker is not None: + return marker, "structured error event" + if result.returncode == 0: + return None + marker = credential_failure_marker(result.stderr or "") + if marker is not None: + return marker, "stderr" + return None + + +def _credential_refresh_error( + *, + backend: str, + backend_name: str, + marker: str, + where: str, + cmd_str: str, + worktree_path: str, + result: subprocess.CompletedProcess[str], + retried: bool = False, +) -> CredentialRefreshError: + transient = credential_failure_is_transient(marker) + if transient: + # A lost refresh race. The winner has stored a fresh credential, so + # sending the operator to re-login would discard a working one. + message = ( + f"{backend_name} lost a refresh race on the shared credential " + f"(matched {marker!r} in {where}" + f"{'; failed again on retry' if retried else ''})" + ) + suggestion = ( + f"This is a credential failure, not a failed mutation: another " + f"candidate refreshed the shared {backend_name} login first and " + "the token this invocation held was already spent. The refreshed " + "credential is stored and should be usable" + + ( + ", but a retry with it also failed. Run `helix resume`; the " + "stored login most likely does not need to be redone, so only " + "re-authenticate if this keeps recurring." + if retried + else "." + ) + ) + else: + message = ( + f"{backend_name} could not use its stored credential " + f"(matched {marker!r} in {where})" + ) + suggestion = ( + f"This is a credential failure, not a failed mutation: {backend_name} " + "reported that its stored login could not be used or refreshed. " + f"Re-authenticate with `helix sandbox login {backend}`, then resume " + "the run; nothing is wrong with the candidate's code." + ) + error = CredentialRefreshError( + message, + operation=f"{backend_name} invocation", + phase="credential check", + command=cmd_str, + cwd=str(worktree_path), + stdout=result.stdout, + stderr=result.stderr, + exit_code=result.returncode, + suggestion=suggestion, + ) + error.transient = transient + return error + + # --------------------------------------------------------------------------- # Rendered-mutation-prompt artifact # --------------------------------------------------------------------------- @@ -614,6 +896,10 @@ def _looks_like_rate_limit(text: str) -> bool: BACKEND_STDERR_ARTIFACT_NAME = ".helix_backend_stderr.txt" BACKEND_TRANSCRIPT_ARTIFACT_DIR = ".helix_artifacts/backend_transcripts" +#: Where a sandboxed opencode candidate keeps its SQLite database: under the +#: per-candidate workspace copy, which the container sees at ``/workspace``. +OPENCODE_SANDBOX_DB_PATH = f"/workspace/{OPENCODE_STATE_DIR_NAME}/opencode/opencode.db" + def _prompt_file_instruction(prompt_artifact_name: str) -> str: return ( @@ -644,12 +930,17 @@ def _ignore_helix_artifacts(worktree_path: Path) -> None: BACKEND_RESULT_ARTIFACT_NAME, BACKEND_STDOUT_ARTIFACT_NAME, BACKEND_STDERR_ARTIFACT_NAME, + # The first attempt's copies after a lost refresh race (see + # ``invoke_with_refresh_race_retry``). + attempt_artifact_name(BACKEND_RESULT_ARTIFACT_NAME, 1), + attempt_artifact_name(BACKEND_STDOUT_ARTIFACT_NAME, 1), + attempt_artifact_name(BACKEND_STDERR_ARTIFACT_NAME, 1), ".helix_artifacts/", "helix_batch.json", - # Per-candidate OpenCode SQLite state (XDG_DATA_HOME isolation). + # Per-candidate OpenCode SQLite state (OPENCODE_DB isolation). # Each parallel opencode worker gets a fresh database here; keeps - # the candidate git tree free of opencode's session/session files. - ".helix_opencode_state/", + # the candidate git tree free of opencode's session transcripts. + f"{OPENCODE_STATE_DIR_NAME}/", ] existing = gitignore.read_text() if gitignore.exists() else "" to_append = [p for p in patterns if p not in existing] @@ -803,6 +1094,11 @@ def _build_backend_args( "exec", "--json", "--dangerously-bypass-approvals-and-sandbox", + # Pin codex's cross-session memory off (default off, but a flag) + # so no candidate reads a prior candidate's session; see + # ``helix.backends.BACKEND_FRESH_SESSION_ENV``. + "-c", + "features.memories=false", ] if config.model: args.extend(["--model", config.model]) @@ -1490,8 +1786,6 @@ def _copy_local_claude_transcript( } try: dst.parent.mkdir(parents=True, exist_ok=True) - import shutil - shutil.copy2(src, dst) except OSError as exc: return { @@ -1619,6 +1913,214 @@ def _write_backend_artifacts( ) +def _combine_usage( + first: UsageStats | None, second: UsageStats | None +) -> UsageStats | None: + """Sum two per-attempt usage records into one, tolerating ``None``. + + Used when an invocation is retried after a lost refresh race: both + attempts spent tokens, and the caller sees only one ``UsageStats``, so the + first attempt's spend has to ride along with the second's. Neither input + is mutated. ``session_id`` is taken from the later attempt, which is the + one whose output the caller receives; the earlier one is used only when + the later attempt reported none. Returns ``None`` only when both inputs + are ``None``, which keeps the "no backend ran" reading of a missing + record intact. + """ + if first is None: + return second + total = UsageStats.from_dict(first.to_dict()) + if second is not None: + total.add(second) + if second.session_id is not None: + total.session_id = second.session_id + return total + + + +# --------------------------------------------------------------------------- +# One-shot retry after a lost refresh race +# --------------------------------------------------------------------------- + + +def attempt_artifact_name(name: str, attempt: int) -> str: + """Name under which an earlier attempt's artifact is kept. + + ``.helix_backend_result.json`` -> ``.helix_backend_result.attempt1.json``; + a name without an extension gets the suffix appended. + """ + stem, dot, ext = name.rpartition(".") + if not stem: + return f"{name}.attempt{attempt}" + return f"{stem}.attempt{attempt}{dot}{ext}" + + +_ATTEMPT_ARTIFACT_NAMES: tuple[str, ...] = ( + BACKEND_RESULT_ARTIFACT_NAME, + BACKEND_STDOUT_ARTIFACT_NAME, + BACKEND_STDERR_ARTIFACT_NAME, + BACKEND_TRANSCRIPT_ARTIFACT_DIR, +) + + +def _stash_attempt_artifacts(worktree_path: str, stash_dir: Path) -> None: + """Copy the invocation artifacts of *worktree_path* into *stash_dir*.""" + wt = Path(worktree_path) + for name in _ATTEMPT_ARTIFACT_NAMES: + src = wt / name + dst = stash_dir / name + try: + if src.is_dir(): + shutil.copytree(src, dst) + elif src.is_file(): + dst.parent.mkdir(parents=True, exist_ok=True) + shutil.copy2(src, dst) + except OSError as exc: + logger.debug("could not stash %s from %s: %s", name, worktree_path, exc) + + +def _restore_attempt_artifacts( + worktree_path: str, stash_dir: Path, *, attempt: int +) -> None: + """Write stashed artifacts into *worktree_path* under their attempt names.""" + wt = Path(worktree_path) + for name in _ATTEMPT_ARTIFACT_NAMES: + src = stash_dir / name + dst = wt / attempt_artifact_name(name, attempt) + try: + if src.is_dir(): + shutil.copytree(src, dst, dirs_exist_ok=True) + elif src.is_file(): + dst.parent.mkdir(parents=True, exist_ok=True) + shutil.copy2(src, dst) + except OSError as exc: + logger.debug("could not restore %s into %s: %s", name, worktree_path, exc) + + +def _note_retry_in_result_artifact( + worktree_path: str, + *, + first_usage: UsageStats | None, + combined_usage: UsageStats, +) -> None: + """Make the final attempt's result artifact account for both attempts. + + The artifact on disk describes the retry only; the caller is charged the + sum. Record the sum and point at the first attempt's artifact so the + file stays a faithful account of what the invocation spent. + """ + path = Path(worktree_path) / BACKEND_RESULT_ARTIFACT_NAME + try: + payload = json.loads(path.read_text()) + except (OSError, ValueError): + return + if not isinstance(payload, dict): + return + payload["attempts"] = 2 + payload["retry_of"] = attempt_artifact_name(BACKEND_RESULT_ARTIFACT_NAME, 1) + payload["retry_reason"] = "lost refresh race on the shared credential" + payload["usage_first_attempt"] = ( + first_usage.to_dict() if first_usage is not None else None + ) + payload["usage_combined"] = combined_usage.to_dict() + try: + path.write_text(json.dumps(payload, indent=2)) + except OSError as exc: + logger.debug("could not annotate %s: %s", path, exc) + + +def invoke_with_refresh_race_retry( + child: Candidate, + *, + backend_name: str, + invoke: Callable[[Candidate, bool], tuple[dict[str, Any], UsageStats]], + fresh_child: Callable[[], Candidate], + remove_child: Callable[[Candidate], None], + on_child_replaced: Callable[[Candidate], None], + record_usage: Callable[[UsageStats], None] | None = None, + on_refresh_race_recovered: Callable[[str], None] | None = None, +) -> tuple[Candidate, UsageStats]: + """Run *invoke* on *child*, retrying once on a lost refresh race. + + A transient :class:`CredentialRefreshError` means another candidate + refreshed the shared login first and the token this invocation held was + already spent; the winner has stored a working credential, so a second + invocation against it is the right response. The retry must not run on + the tree the first attempt was editing: with the sandbox, ``sync_back`` + copies the first attempt's partial edits into the worktree regardless of + exit code, and the retry's workspace copy would commit them as its + synthetic baseline, hiding them from ``git diff``. So the retry starts + from a *fresh* worktree -- ``remove_child`` then ``fresh_child`` -- and + the first attempt's artifacts are kept beside the retry's under + ``attempt1`` names (:func:`attempt_artifact_name`). + + ``on_child_replaced`` is called with the fresh worktree the moment it + exists, so the caller's cleanup handlers always address the live one. + + Usage accounting: the first attempt's spend rides along. On a successful + retry the returned usage is the sum and the result artifact is annotated; + when the retry raises a :class:`HelixError` the sum is attached to it; + when it raises anything else (a sandbox ``TimeoutExpired``, an + ``OSError``) the first attempt's spend is handed to ``record_usage`` + directly before the exception propagates, since nothing else will carry it. + + A recovered race is reported through ``on_refresh_race_recovered`` so it + reaches the operator (the end-of-run summary), not only the log file. + A second loss in a row is raised as-is; there is no loop. + """ + try: + _, usage = invoke(child, False) + return child, usage + except CredentialRefreshError as exc: + if not exc.transient: + raise + first_error = exc + first_usage = exc.usage + + logger.warning( + "%s lost a refresh race on the shared credential; retrying once from " + "a fresh worktree against the refreshed credential (%s).", + backend_name, + first_error, + ) + with tempfile.TemporaryDirectory(prefix="helix-attempt1-") as stash: + _stash_attempt_artifacts(child.worktree_path, Path(stash)) + try: + remove_child(child) + except Exception as exc: # noqa: BLE001 - re-clone below reports the real problem + logger.debug("could not remove %s before retry: %s", child.worktree_path, exc) + child = fresh_child() + on_child_replaced(child) + _restore_attempt_artifacts(child.worktree_path, Path(stash), attempt=1) + + try: + _, usage = invoke(child, True) + except HelixError as retry_exc: + retry_exc.usage = _combine_usage(first_usage, retry_exc.usage) + raise + except Exception: + if record_usage is not None and first_usage is not None: + record_usage(first_usage) + raise + + combined = _combine_usage(first_usage, usage) + assert combined is not None # ``usage`` is never None + _note_retry_in_result_artifact( + child.worktree_path, first_usage=first_usage, combined_usage=combined + ) + message = ( + f"{child.id} recovered after a lost refresh race on the shared " + f"{backend_name} credential: the first attempt reported " + f"{first_error}; the retry from a fresh worktree succeeded. Both " + "attempts' tokens are charged and the first attempt's output is kept " + f"as {attempt_artifact_name(BACKEND_RESULT_ARTIFACT_NAME, 1)}." + ) + logger.warning("%s", message) + if on_refresh_race_recovered is not None: + on_refresh_race_recovered(message) + return child, combined + + def invoke_claude_code( worktree_path: str, prompt: str, @@ -1627,6 +2129,8 @@ def invoke_claude_code( fixed_env: dict[str, str] | None = None, sandbox: SandboxConfig | None = None, prompt_artifact_name: str = MUTATION_PROMPT_ARTIFACT_NAME, + *, + retried: bool = False, ) -> tuple[dict[str, Any], UsageStats]: """Invoke the configured backend CLI in *worktree_path*. @@ -1644,6 +2148,12 @@ def invoke_claude_code( fixed_env: Optional mapping of explicit env var values to inject after passthrough values. + retried: + True when this is the one-shot retry after a lost refresh race (see + :func:`invoke_with_refresh_race_retry`). Only changes what a second + credential failure says to the operator; this function itself never + retries, because a retry has to start from a fresh worktree and only + the caller owns the worktree. Returns ------- @@ -1656,14 +2166,17 @@ def invoke_claude_code( On non-zero return code or JSON decode failure. All errors include the full command, full stdout, full stderr (never truncated), exit code, and working directory. + CredentialRefreshError + When the backend reported, in its own structured error fields or on + stderr, that its stored credential could not be used or refreshed. + ``transient`` is set when it merely lost a refresh race. """ if _MUTATOR_OVERRIDE is not None: return _MUTATOR_OVERRIDE(worktree_path, prompt, config) backend = config.backend backend_name = backend_display_name(backend) - backend_worktree_path = ( - "/workspace" if sandbox is not None and sandbox.enabled else worktree_path - ) + sandboxed = sandbox is not None and sandbox.enabled + backend_worktree_path = "/workspace" if sandboxed else worktree_path args = _build_backend_args( backend_worktree_path, config, @@ -1674,7 +2187,12 @@ def invoke_claude_code( passthrough_env=passthrough_env, fixed_env=fixed_env ) _add_backend_auth_env(backend_env, backend) - if backend == "opencode" and (sandbox is None or not sandbox.enabled): + # The fresh-session switch applies to sandboxed and unsandboxed runs + # alike. An operator who names the same key in ``[env]`` has made a + # deliberate choice, so their value wins. + for key, value in BACKEND_FRESH_SESSION_ENV.get(backend, {}).items(): + backend_env.setdefault(key, value) + if backend == "opencode": # Per-candidate SQLite isolation for concurrent opencode subprocesses. # # OpenCode stores its session database at: @@ -1688,17 +2206,38 @@ def invoke_claude_code( # "Failed to run the query 'PRAGMA journal_mode = WAL'" # (observed in PR #34 E2E re-verify: g1-s1 lost to this error while g1-s2 succeeded). # - # Fix: set XDG_DATA_HOME to a per-candidate directory. OpenCode respects - # XDG_DATA_HOME and will create an isolated database at: + # Fix: point OPENCODE_DB at a per-candidate database. OPENCODE_DB + # relocates opencode.db and its -wal/-shm companions and nothing else. + # XDG_DATA_HOME would also solve the locking problem but moves + # auth.json along with the database, which makes an existing opencode + # login invisible -- verified against the real CLI, where + # `opencode auth list` then reports 0 credentials. The on-disk layout + # is unchanged from the earlier XDG_DATA_HOME approach: # /.helix_opencode_state/opencode/opencode.db # Each parallel worker gets its own fresh database; no contention. # - # The sandbox branch is excluded: container isolation already provides - # per-candidate filesystem separation, so XDG_DATA_HOME would be redundant. - opencode_state_dir = Path(worktree_path) / ".helix_opencode_state" - opencode_state_dir.mkdir(parents=True, exist_ok=True) - backend_env["XDG_DATA_HOME"] = str(opencode_state_dir) - if sandbox is not None and sandbox.enabled: + # The sandbox needs the same knob, not less: every sandboxed agent + # container mounts the one ``helix-auth-opencode`` volume at + # ``/home/node`` read-write with ``HOME=/home/node`` forced, so + # without it every concurrent container opens the same + # ``~/.local/share/opencode/opencode.db``. Container isolation + # separates ``/workspace``, which is why the database goes there: + # the container's cwd is the per-candidate workspace copy, the + # sandbox creates the directory in that copy before the run (see + # ``helix.sandbox.run_sandboxed_commands``), and ``.helix*`` paths + # are excluded from sync-back and gitignored like the unsandboxed + # layout. + if sandboxed: + backend_env["OPENCODE_DB"] = OPENCODE_SANDBOX_DB_PATH + else: + opencode_state_dir = ( + Path(worktree_path) / OPENCODE_STATE_DIR_NAME / "opencode" + ) + opencode_state_dir.mkdir(parents=True, exist_ok=True) + backend_env["OPENCODE_DB"] = str(opencode_state_dir / "opencode.db") + + if sandboxed: + assert sandbox is not None sandbox_image = resolve_sandbox_image(sandbox, backend) result = run_sandboxed_command( args, @@ -1719,13 +2258,37 @@ def invoke_claude_code( env=backend_env, ) - # Recover the usage record from the raw stream FIRST, with a parse that - # cannot raise. Everything below this line can fail — and when it does, - # the tokens have still been spent. Charging must not be conditional on - # the candidate being usable, so every error raised from here carries - # this on ``HelixError.usage`` for the caller to charge. + # Recover the usage record from the raw stream FIRST, with a parse + # that cannot raise. Everything below this line can fail -- and + # when it does, the tokens have still been spent. Charging must + # not be conditional on the candidate being usable, so every error + # raised from here carries this on ``HelixError.usage`` for the + # caller to charge. spent_usage = _salvage_backend_usage(backend, result) + def _raise_if_credential_failure(parsed: dict[str, Any] | None) -> None: + evidence = _credential_failure_evidence(backend, parsed, result) + if evidence is None: + return + marker, where = evidence + logger.error( + "Credential failure detected for %s (exit %d) in %s: matched %r", + backend_name, + result.returncode, + where, + marker, + ) + raise _credential_refresh_error( + backend=backend, + backend_name=backend_name, + marker=marker, + where=where, + cmd_str=cmd_str, + worktree_path=worktree_path, + result=result, + retried=retried, + ) + parsed: dict[str, Any] | None = None try: if result.returncode == 0: @@ -1736,6 +2299,13 @@ def invoke_claude_code( worktree_path=worktree_path, ) usage = _normalise_usage_stats(parsed) + # A backend can report an unusable credential and still exit 0 -- + # measured on codex-cli 0.130.0, whose refresh failure is swallowed + # entirely (exit 0, empty stderr, even at RUST_LOG=info). Its + # own structured error event is the only signal left on this + # path, so read it here rather than letting the failure pass as + # a successful-but-useless mutation. + _raise_if_credential_failure(parsed) if backend == "claude": error_text = str(parsed.get("error", "")) if _looks_like_rate_limit(error_text): @@ -1758,6 +2328,34 @@ def invoke_claude_code( ) return parsed, usage + # Non-zero exit. Claude's max-turns exhaustion is partial success + # and is decided FIRST: the subprocess may have already produced + # useful edits, and the envelope's ``result`` is the assistant's + # own prose, which no later classifier may read as evidence. + if backend == "claude": + try: + parsed = _parse_backend_output( + backend, + result, + cmd_str=cmd_str, + worktree_path=worktree_path, + ) + except MutationError: + parsed = None + else: + if parsed.get("subtype") == "error_max_turns": + logger.warning( + "Claude Code reached max_turns limit (%s turns) — treating as partial success.", + parsed.get("num_turns", "?"), + ) + return parsed, _normalise_usage_stats(parsed) + + # Then a credential failure, from structured error fields and stderr + # only. The markers are disjoint from the rate-limit keywords, and + # "the login is unusable" is a strictly more actionable verdict than + # "the backend exited non-zero". + _raise_if_credential_failure(parsed) + rate_limit_source = result.stderr or result.stdout if _looks_like_rate_limit(rate_limit_source): logger.error( @@ -1781,33 +2379,12 @@ def invoke_claude_code( ), ) - # Claude's max-turns exhaustion is intentionally treated as partial - # success because the subprocess may have already produced useful edits. - if backend == "claude": - try: - parsed = _parse_backend_output( - backend, - result, - cmd_str=cmd_str, - worktree_path=worktree_path, - ) - usage = _normalise_usage_stats(parsed) - if parsed.get("subtype") == "error_max_turns": - logger.warning( - "Claude Code reached max_turns limit (%s turns) — treating as partial success.", - parsed.get("num_turns", "?"), - ) - return parsed, usage - except MutationError: - parsed = None - parsed = _parse_backend_output( backend, result, cmd_str=cmd_str, worktree_path=worktree_path, ) - usage = _normalise_usage_stats(parsed) raise MutationError( f"{backend_name} exited with code {result.returncode}", @@ -1821,8 +2398,8 @@ def invoke_claude_code( suggestion="Check stderr for rate limits, permission errors, or model availability.", ) except HelixError as exc: - # Attach only when the raiser did not already supply a more precise - # record; never overwrite one. + # Attach only when the raiser did not already supply a more + # precise record; never overwrite one. if exc.usage is None: exc.usage = spent_usage raise @@ -1852,6 +2429,7 @@ def mutate( background: str | None = None, prepare_worktree: Callable[[Candidate], None] | None = None, record_usage: Callable[[UsageStats], None] | None = None, + on_refresh_race_recovered: Callable[[str], None] | None = None, ) -> Candidate | None: """Mutate *parent* using the configured backend and return the new candidate. @@ -1878,16 +2456,25 @@ def mutate( are spent either way, so this is how a caller charges the budget for an attempt that ends in ``None``. Not called when no backend invocation happened (e.g. the worktree clone raised). + on_refresh_race_recovered: + Optional sink told, in operator-facing words, when the invocation lost + a refresh race on the shared credential and succeeded on its one retry + from a fresh worktree (see :func:`invoke_with_refresh_race_retry`). Returns ------- Candidate | None The new candidate on success, or ``None`` if mutation failed. """ - child = clone_candidate(parent, new_id, base_dir) - child.operation = "mutate" - if prepare_worktree is not None: - prepare_worktree(child) + + def _fresh_child() -> Candidate: + fresh = clone_candidate(parent, new_id, base_dir) + fresh.operation = "mutate" + if prepare_worktree is not None: + prepare_worktree(fresh) + return fresh + + child = _fresh_child() prompt = build_mutation_prompt( config.objective, @@ -1902,18 +2489,47 @@ def mutate( # per-worktree ``.gitignore`` entry (see ``_ignore_helix_artifacts``) # keep it out of the candidate git tree — otherwise it'd leak into # every subsequent mutation's diff and the mutator would see its own - # prior prompt file as part of the codebase. - prompt_artifact_name = _write_mutation_prompt_artifact(child.worktree_path, prompt) - - try: - _, usage = invoke_claude_code( - child.worktree_path, + # prior prompt file as part of the codebase. Written here, before the + # first invocation, so a collision is raised as-is; the retry path + # rewrites it into its fresh worktree (the write is idempotent). + _write_mutation_prompt_artifact(child.worktree_path, prompt) + + def _invoke( + target: Candidate, retried: bool + ) -> tuple[dict[str, Any], UsageStats]: + return invoke_claude_code( + target.worktree_path, prompt, config.agent, passthrough_env=config.passthrough_env, fixed_env=config.env, sandbox=config.sandbox, - prompt_artifact_name=prompt_artifact_name, + prompt_artifact_name=_write_mutation_prompt_artifact( + target.worktree_path, prompt + ), + retried=retried, + ) + + def _replace(fresh: Candidate) -> None: + nonlocal child + child = fresh + + def _discard_child() -> None: + try: + remove_worktree(child) + except Exception: + pass + + try: + child, usage = invoke_with_refresh_race_retry( + child, + backend_name=backend_display_name(config.agent.backend), + invoke=_invoke, + fresh_child=_fresh_child, + remove_child=remove_worktree, + on_child_replaced=_replace, + record_usage=record_usage, + on_refresh_race_recovered=on_refresh_race_recovered, ) child.usage = usage if record_usage is not None: @@ -1925,10 +2541,7 @@ def mutate( record_usage(exc.usage) exc.operation = f"mutate {new_id} (parent: {parent.id})" print_helix_error(exc) - try: - remove_worktree(child) - except Exception: - pass + _discard_child() return None except RateLimitError as exc: # Rate limit — clean up orphaned worktree, then re-raise so the parallel @@ -1937,10 +2550,25 @@ def mutate( # before the limit hit, so the same handoff applies. if record_usage is not None and exc.usage is not None: record_usage(exc.usage) - try: - remove_worktree(child) - except Exception: - pass + _discard_child() + raise + except CredentialRefreshError as exc: + # The stored login, not this candidate, is what failed. The tokens + # spent before the credential gave out are still spent, so hand them + # to the sink like the other paths do; then clean up the orphaned + # worktree and re-raise so evolution.py can count it and name it as a + # credential failure instead of filing it under "the agent wrote bad + # code". + if record_usage is not None and exc.usage is not None: + record_usage(exc.usage) + _discard_child() + raise + except Exception: + # Anything else (a sandbox ``TimeoutExpired``, an ``OSError`` from + # the launch) carries no usage of its own; whatever an earlier + # attempt spent has already been handed to the sink. Do not leak + # the worktree on the way out. + _discard_child() raise # NOTE: snapshot_candidate() is intentionally NOT called here. diff --git a/src/helix/sandbox.py b/src/helix/sandbox.py index e91eba0a..eba0ecd1 100644 --- a/src/helix/sandbox.py +++ b/src/helix/sandbox.py @@ -2,12 +2,13 @@ from __future__ import annotations -from collections.abc import Iterator, Sequence +from collections.abc import Iterator, Mapping, Sequence from contextlib import contextmanager from dataclasses import dataclass import json import logging import os +from datetime import datetime, timedelta, timezone import shlex import shutil import subprocess @@ -19,7 +20,11 @@ from pathlib import Path from typing import Literal -from helix.backends import BACKEND_AUTH_COMMANDS, DEFAULT_BACKEND_IMAGES +from helix.backends import ( + BACKEND_AUTH_COMMANDS, + DEFAULT_BACKEND_IMAGES, + backend_credential_warm_skip_reason, +) from helix.config import EvaluatorSidecarConfig, SandboxConfig from helix.lines import split_lf_lines @@ -34,6 +39,13 @@ "helix_batch.json", } +#: Per-candidate OpenCode state directory (holds ``opencode/opencode.db``). +#: Under the worktree when unsandboxed; created in the per-candidate workspace +#: copy (mounted at ``/workspace``) when sandboxed. Starts with ``.helix`` so +#: it is excluded from the sandbox copy and sync-back like every other HELIX +#: artifact, and ``helix.mutator._ignore_helix_artifacts`` gitignores it. +OPENCODE_STATE_DIR_NAME = ".helix_opencode_state" + @dataclass(frozen=True) class EvaluatorSidecarRuntime: @@ -473,6 +485,30 @@ def _init_synthetic_git_repo(workspace: Path) -> None: ) +def _prepare_opencode_state_dir(workspace: Path) -> None: + """Create the per-candidate OpenCode database directory in *workspace*. + + ``invoke_claude_code`` points ``OPENCODE_DB`` at + ``/workspace/.helix_opencode_state/opencode/opencode.db`` for sandboxed + opencode runs (every container shares one ``/home/node``, so the default + location would be one database for all concurrent candidates). SQLite + does not create parent directories, and ``.helix*`` paths are excluded + from the workspace copy, so the directory has to be made here. It is + listed in the synthetic repo's local excludes so the agent's ``git + status`` never shows the database as an untracked file. + """ + (workspace / OPENCODE_STATE_DIR_NAME / "opencode").mkdir( + parents=True, exist_ok=True + ) + exclude = workspace / ".git" / "info" / "exclude" + try: + exclude.parent.mkdir(parents=True, exist_ok=True) + with exclude.open("a", encoding="utf-8") as fh: + fh.write(f"{OPENCODE_STATE_DIR_NAME}/\n") + except OSError as exc: + logger.debug("could not write %s: %s", exclude, exc) + + def _run_workspace_helper( workspace: Path, image: str, @@ -1013,6 +1049,8 @@ def run_sandboxed_commands( omit_paths=omit_paths, ) _init_synthetic_git_repo(workspace) + if scope == "agent" and agent_backend == "opencode": + _prepare_opencode_state_dir(workspace) _docker_chown_workspace(workspace, docker_image, "node:node") sidecar_runtime = ( current_evaluator_sidecar_runtime() if scope == "evaluator" else None @@ -1103,18 +1141,31 @@ def sandbox_auth_docker_args( agent_backend: str, *, image: str, - action: Literal["login", "status", "logout"], + action: Literal["login", "status", "logout", "warm"], network: str = "bridge", add_host_gateway: bool = False, extra_hosts: dict[str, str] | None = None, interactive: bool = False, + container_name: str | None = None, + env: Mapping[str, str] | None = None, + command: Sequence[str] | None = None, ) -> list[str]: - try: - command = BACKEND_AUTH_COMMANDS[agent_backend][action] - except KeyError as exc: - raise ValueError( - f"No sandbox auth {action!r} command for backend: {agent_backend}" - ) from exc + """Build the ``docker run`` argv for one auth-related command. + + *env* is forwarded with ``-e`` (``HOME`` and ``PATH`` stay pinned); it is + how the operator's ``passthrough_env`` / ``[env]`` proxy and CA settings + reach a container that has to talk to the token endpoint. *command* + replaces the registered *action* argv when given -- used for read-only + probes against the login volume that are not auth commands in their own + right. + """ + if command is None: + try: + command = BACKEND_AUTH_COMMANDS[agent_backend][action] + except KeyError as exc: + raise ValueError( + f"No sandbox auth {action!r} command for backend: {agent_backend}" + ) from exc args = [ "docker", @@ -1138,6 +1189,12 @@ def sandbox_auth_docker_args( "-e", "PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin", ] + for key, value in (env or {}).items(): + if key in {"HOME", "PATH"}: + continue + args.extend(["-e", f"{key}={value}"]) + if container_name: + args.extend(["--name", container_name]) if interactive: args.insert(2, "-it") args.append(image) @@ -1148,13 +1205,26 @@ def sandbox_auth_docker_args( def run_sandbox_auth_command( agent_backend: str, *, - action: Literal["login", "status", "logout"], + action: Literal["login", "status", "logout", "warm"], image: str | None = None, network: str = "bridge", add_host_gateway: bool = False, extra_hosts: dict[str, str] | None = None, interactive: bool = False, + timeout: float | None = None, + container_name: str | None = None, + env: Mapping[str, str] | None = None, + command: Sequence[str] | None = None, ) -> subprocess.CompletedProcess[str]: + """Run one auth-related command against the shared login volume. + + *timeout* applies to the non-interactive path only and is enforced on the + ``docker run`` client. Killing the client does not stop the container, so + when a *container_name* is given the container is force-removed on + timeout before :class:`subprocess.TimeoutExpired` propagates; without a + name there is nothing to address and the container is left to exit on its + own. + """ docker_image = image or resolve_sandbox_image( SandboxConfig(enabled=True), agent_backend ) @@ -1166,10 +1236,321 @@ def run_sandbox_auth_command( add_host_gateway=add_host_gateway, extra_hosts=extra_hosts, interactive=interactive, + container_name=container_name, + env=env, + command=command, ) if interactive: return subprocess.run(args, text=True) - return subprocess.run(args, capture_output=True, text=True) + try: + return subprocess.run( + args, capture_output=True, text=True, timeout=timeout + ) + except subprocess.TimeoutExpired: + if container_name: + _run_docker(["docker", "rm", "-f", container_name], check=False) + raise + + +@dataclass(frozen=True) +class CredentialWarmResult: + """Outcome of one per-generation credential warm. + + ``warmed`` is True only when the warm container exited cleanly **and** the + stored credential was verified fresh afterwards -- for codex, that its + ``last_refresh`` either advanced during the warm or already sits inside + the CLI's refresh interval, so no candidate will attempt a refresh. The + exit code alone cannot say that: ``codex debug models`` swallows a + rejected refresh and exits 0. + + ``skip_reason`` is set when the backend is deliberately not warmed; + ``detail`` carries the diagnosis when a warm was attempted, whether it + succeeded or not; ``stale`` marks a clean exit whose credential still + failed verification. + """ + + backend: str + warmed: bool + skip_reason: str | None = None + returncode: int | None = None + detail: str = "" + timed_out: bool = False + stale: bool = False + + @property + def skipped(self) -> bool: + return self.skip_reason is not None + + @property + def failed(self) -> bool: + return not self.warmed and self.skip_reason is None + + +#: Tail of the warm command's stderr kept for diagnosis. Backend CLIs report +#: refresh failures in prose, not by echoing the credential, but the cap keeps +#: an unexpectedly chatty CLI from pasting its whole state into the run log. +_WARM_DETAIL_CHARS = 400 + +#: Upper bound on one credential warm, in seconds. The warm is a single +#: token-refresh exchange plus, on first use, an image pull; it is never the +#: long-running agent turn that ``sandbox.timeout_seconds`` is sized for. A +#: stalled refresh must not wedge the whole evolution loop before any candidate +#: is dispatched, so the bound always applies -- ``sandbox.timeout_seconds`` +#: can only tighten it. +CREDENTIAL_WARM_TIMEOUT_SECONDS = 300 + +#: Bound on one read of ``last_refresh`` from the login volume: a ``sed`` over +#: one file in a container with no network. +_CREDENTIAL_PROBE_TIMEOUT_SECONDS = 60 + +#: How old a codex credential may be before the CLI refreshes it proactively. +#: ``TOKEN_REFRESH_INTERVAL: i64 = 8`` (days) in codex-rs +#: ``login/src/auth/manager.rs``: ``should_refresh_proactively`` returns true +#: when ``last_refresh < now - 8 days``. Newer builds also refresh when the +#: access token's JWT ``exp`` is within 5 minutes; that clock is not read +#: here, so a credential inside the 8-day interval whose access token is about +#: to expire is reported fresh although a refresh is imminent -- the +#: interval, not the JWT, is what the shipped measurement used. +CODEX_TOKEN_REFRESH_INTERVAL = timedelta(days=8) + +#: Extracts the ``last_refresh`` value -- and nothing else -- from codex's +#: ``auth.json``. The tokens beside it never leave the container. Prints +#: nothing when the file is absent. +_SED_LAST_REFRESH = r's/.*"last_refresh"[[:space:]]*:[[:space:]]*"\([^"]*\)".*/\1/p' +_CODEX_LAST_REFRESH_PROBE: tuple[str, ...] = ( + "sh", + "-c", + 'f="$HOME/.codex/auth.json"; [ -f "$f" ] || exit 0; ' + "sed -n '" + _SED_LAST_REFRESH + "' \"$f\"", +) + + +def credential_warm_timeout(sandbox: SandboxConfig) -> float: + """Return the timeout for one warm: the fixed cap, tightened by the sandbox's.""" + if sandbox.timeout_seconds is not None: + return float(min(sandbox.timeout_seconds, CREDENTIAL_WARM_TIMEOUT_SECONDS)) + return float(CREDENTIAL_WARM_TIMEOUT_SECONDS) + + +def _parse_last_refresh(text: str) -> datetime | None: + value = text.strip().splitlines()[-1].strip() if text.strip() else "" + if not value: + return None + try: + parsed = datetime.fromisoformat(value.replace("Z", "+00:00")) + except ValueError: + return None + if parsed.tzinfo is None: + parsed = parsed.replace(tzinfo=timezone.utc) + return parsed + + +def _read_codex_last_refresh( + *, image: str, sandbox: SandboxConfig, timeout: float +) -> datetime | None: + """Read ``last_refresh`` from the codex login volume, or ``None``. + + Runs under ``--network none`` through the same single-writer auth + container as the warm; reads one field and never the tokens. ``None`` + means the value could not be read (no file, no field, container failure) + and is reported as "unverified" by the caller, never as fresh. + """ + container_name = f"helix-probe-codex-{uuid.uuid4().hex[:12]}" + try: + result = run_sandbox_auth_command( + "codex", + action="status", + image=image, + network="none", + add_host_gateway=sandbox.add_host_gateway, + extra_hosts=sandbox.extra_hosts, + timeout=min(timeout, _CREDENTIAL_PROBE_TIMEOUT_SECONDS), + container_name=container_name, + command=_CODEX_LAST_REFRESH_PROBE, + ) + except (OSError, ValueError, subprocess.SubprocessError) as exc: + logger.debug("could not read codex last_refresh: %s", exc) + return None + if result.returncode != 0: + logger.debug( + "codex last_refresh probe exited %d: %s", + result.returncode, + (result.stderr or "")[-_WARM_DETAIL_CHARS:], + ) + return None + return _parse_last_refresh(result.stdout or "") + + +def _describe_age(age: timedelta) -> str: + total = int(age.total_seconds()) + if total < 0: + return "in the future" + days, rem = divmod(total, 86400) + hours, rem = divmod(rem, 3600) + minutes = rem // 60 + if days: + return f"{days}d {hours}h" + if hours: + return f"{hours}h {minutes}m" + return f"{minutes}m" + + +def verify_codex_credential_fresh( + before: datetime | None, after: datetime | None, *, now: datetime +) -> tuple[bool, str]: + """Decide whether a codex warm left the credential verifiably fresh. + + Returns ``(fresh, detail)``. Fresh means no candidate in the coming + generation will attempt a refresh: either the warm performed one + (``last_refresh`` advanced) or none was due (``last_refresh`` is inside + :data:`CODEX_TOKEN_REFRESH_INTERVAL`). A ``last_refresh`` that is past + the interval and did not move means the exchange was rejected -- ``codex + debug models`` exits 0 either way -- or that a ``models_cache.json`` + younger than its 5-minute TTL short-circuited the refresh path; both + leave every candidate to attempt the refresh itself. + """ + if after is None: + return ( + False, + "the warm exited 0 but last_refresh could not be read back from " + "the login volume, so the credential is unverified", + ) + age = now - after + if before is not None and after > before: + return ( + True, + "refresh performed: last_refresh advanced to " + f"{after.isoformat(timespec='seconds')}", + ) + if age < CODEX_TOKEN_REFRESH_INTERVAL: + return ( + True, + f"credential fresh: last_refresh is {_describe_age(age)} old, inside " + f"codex's {CODEX_TOKEN_REFRESH_INTERVAL.days}-day refresh interval, " + "so no candidate will refresh it", + ) + return ( + False, + f"last_refresh is {_describe_age(age)} old, past codex's " + f"{CODEX_TOKEN_REFRESH_INTERVAL.days}-day refresh interval, and did not " + "advance during the warm: the token exchange was most likely rejected " + "(codex debug models exits 0 either way), or a models_cache.json " + "younger than 5 minutes short-circuited the refresh", + ) + + +def warm_backend_credential( + agent_backend: str, + *, + sandbox: SandboxConfig, + env: Mapping[str, str] | None = None, +) -> CredentialWarmResult: + """Refresh *agent_backend*'s shared credential once, under a single writer. + + Runs the backend's registered ``warm`` command through the same sandboxed + auth container that ``helix sandbox status`` uses: one container, the + shared login volume mounted read-write, nothing else running against it. + Any refresh the CLI decides is due therefore happens exactly once and is + written back before candidates start, instead of N candidates racing to + spend the same single-use refresh token. + + *env* is the operator's ``passthrough_env`` / ``[env]`` selection, so the + warm reaches the token endpoint through the same proxy and CA settings + the candidates get. + + ``warmed`` means the credential was **verified** fresh after the warm, + not merely that the command exited 0 (see :class:`CredentialWarmResult`). + For codex, ``last_refresh`` is read from ``auth.json`` before and after + -- one field, no network -- and judged by + :func:`verify_codex_credential_fresh`. + + Never raises: a warm that cannot run is reported, not fatal. Candidates + may still succeed on the credential that is already there, so the run + continues either way and the caller decides how loudly to say so. + """ + skip_reason = backend_credential_warm_skip_reason(agent_backend) + if skip_reason is not None: + return CredentialWarmResult( + backend=agent_backend, warmed=False, skip_reason=skip_reason + ) + + timeout = credential_warm_timeout(sandbox) + # Named so a timed-out warm can be stopped rather than left running + # against the shared login volume after the client has given up on it. + container_name = f"helix-warm-{agent_backend}-{uuid.uuid4().hex[:12]}" + try: + image = resolve_sandbox_image(sandbox, agent_backend) + except ValueError as exc: + return CredentialWarmResult( + backend=agent_backend, warmed=False, detail=f"ValueError: {exc}" + ) + before = ( + _read_codex_last_refresh(image=image, sandbox=sandbox, timeout=timeout) + if agent_backend == "codex" + else None + ) + try: + result = run_sandbox_auth_command( + agent_backend, + action="warm", + image=image, + network=sandbox.network, + add_host_gateway=sandbox.add_host_gateway, + extra_hosts=sandbox.extra_hosts, + timeout=timeout, + container_name=container_name, + env=env, + ) + except subprocess.TimeoutExpired: + # Non-fatal, like every other warm failure: the candidates fall back + # to refreshing for themselves. Reported distinctly because "the + # refresh hung" points somewhere different from "the refresh failed". + return CredentialWarmResult( + backend=agent_backend, + warmed=False, + timed_out=True, + detail=( + f"warm did not finish within {timeout:.0f}s; the warm " + "container was stopped" + ), + ) + except (OSError, ValueError, subprocess.SubprocessError) as exc: + return CredentialWarmResult( + backend=agent_backend, + warmed=False, + detail=f"{type(exc).__name__}: {exc}", + ) + + if result.returncode != 0: + stderr = (result.stderr or "").strip() + return CredentialWarmResult( + backend=agent_backend, + warmed=False, + returncode=result.returncode, + detail=stderr[-_WARM_DETAIL_CHARS:], + ) + + if agent_backend != "codex": + # Only codex has a warm command today; a future backend needs its own + # verifier before its exit code may be trusted. + return CredentialWarmResult( + backend=agent_backend, + warmed=False, + returncode=0, + stale=True, + detail="the warm exited 0 but no freshness verifier exists for this backend", + ) + after = _read_codex_last_refresh(image=image, sandbox=sandbox, timeout=timeout) + fresh, detail = verify_codex_credential_fresh( + before, after, now=datetime.now(timezone.utc) + ) + return CredentialWarmResult( + backend=agent_backend, + warmed=fresh, + returncode=0, + stale=not fresh, + detail=detail, + ) def run_command( diff --git a/src/helix/state.py b/src/helix/state.py index 5293915e..97bc4df3 100644 --- a/src/helix/state.py +++ b/src/helix/state.py @@ -156,6 +156,11 @@ def _eval_cache_path(base_dir: Path) -> Path: return base_dir / _STATE_DIR / _EVAL_CACHE_FILENAME +def state_file_exists(base_dir: Path) -> bool: + """True when ``/.helix/state.json`` has been written.""" + return _state_path(base_dir).is_file() + + def save_state(state: EvolutionState, base_dir: Path) -> None: """Atomically write the evolution state to .helix/state.json.""" target = _state_path(base_dir) diff --git a/tests/integration/__init__.py b/tests/integration/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/tests/integration/conftest.py b/tests/integration/conftest.py new file mode 100644 index 00000000..be1c91ee --- /dev/null +++ b/tests/integration/conftest.py @@ -0,0 +1,130 @@ +"""Shared fixtures for container-backed integration tests. + +These tests run real backend containers. They never touch a real +``helix-auth-*`` login volume and never perform a login: every credential they +write is synthetic and lives in a throwaway volume that is removed again in +teardown. +""" + +from __future__ import annotations + +import os +import subprocess +import uuid +from collections.abc import Iterator + +import pytest + + +REAL_AUTH_VOLUME_PREFIX = "helix-auth-" +TEST_VOLUME_PREFIX = "helix-integration-test-" + + +def _strict() -> bool: + return os.environ.get("HELIX_DOCKER_TESTS_STRICT") == "1" + + +def _unavailable(reason: str) -> None: + """Fail under ``HELIX_DOCKER_TESTS_STRICT=1``, otherwise skip. + + Strict mode exists so CI cannot silently turn this suite into a no-op: + a missing daemon or image becomes a failure to fix rather than a green run. + """ + if _strict(): + pytest.fail(f"HELIX_DOCKER_TESTS_STRICT=1 but {reason}") + pytest.skip(reason) + + +def _docker(*args: str, check: bool = True) -> subprocess.CompletedProcess[str]: + return subprocess.run( + ["docker", *args], capture_output=True, text=True, check=check, timeout=300 + ) + + +@pytest.fixture(scope="session") +def docker_available() -> None: + try: + _docker("info") + except (OSError, subprocess.SubprocessError) as exc: + _unavailable(f"Docker daemon is not usable: {exc}") + + +@pytest.fixture +def require_image(docker_available: None): + def _require(image: str) -> str: + result = _docker("image", "inspect", image, check=False) + if result.returncode != 0: + _unavailable(f"image {image} is not present locally") + return image + + return _require + + +@pytest.fixture +def throwaway_volume(docker_available: None) -> Iterator[object]: + """Create synthetic-credential volumes and guarantee their removal. + + The name prefix is asserted on every call so a bug in a test can never + address, mutate, or delete one of the real ``helix-auth-*`` volumes. + """ + created: list[str] = [] + + def _create(image: str, seed_script: str) -> str: + name = f"{TEST_VOLUME_PREFIX}{uuid.uuid4().hex[:12]}" + assert not name.startswith(REAL_AUTH_VOLUME_PREFIX) + _docker("volume", "create", name) + created.append(name) + # Seed as root, then hand the tree to the container user the backends + # run as, mirroring how a real login volume ends up owned. + _docker( + "run", + "--rm", + "--network", + "none", + "-v", + f"{name}:/home/node", + "--user", + "root", + image, + "sh", + "-c", + f"set -eu; {seed_script}; chown -R node:node /home/node", + ) + return name + + try: + yield _create + finally: + for name in created: + assert name.startswith(TEST_VOLUME_PREFIX) + _docker("volume", "rm", "-f", name, check=False) + + +@pytest.fixture +def volume_listing(docker_available: None): + """Return names/modes/sizes of a volume's contents -- never file contents.""" + + def _list(volume: str, image: str) -> set[str]: + # ``~/.cache`` is pruned: the CLIs drop version-keyed caches there (for + # example cursor's V8 compile cache). Those are rebuilt from the image + # and carry no candidate state, so including them would make the + # comparison flap without telling us anything about contamination. + result = _docker( + "run", + "--rm", + "--network", + "none", + "-v", + f"{volume}:/home/node:ro", + "--user", + "node", + image, + "sh", + "-c", + 'find /home/node -mindepth 1 ' + '-not -path /home/node/.cache -not -path "/home/node/.cache/*" ' + '-printf "%M %s %p\\n" | sort -k3', + ) + return {line for line in result.stdout.splitlines() if line.strip()} + + return _list diff --git a/tests/integration/test_credential_warm_docker.py b/tests/integration/test_credential_warm_docker.py new file mode 100644 index 00000000..d0eabfd2 --- /dev/null +++ b/tests/integration/test_credential_warm_docker.py @@ -0,0 +1,214 @@ +"""Container proof that the credential warm is real, and that it is free. + +The warm exists to move a due token refresh out of N racing candidates and into +one writer. Two claims have to hold against a real backend container or the +change is worse than useless: + +(a) it costs nothing -- no model call, no quota, on an operator's paid account + that would otherwise be charged once per generation; and +(b) it does not touch the credential when it is already fresh. + +Both are asserted here by running the registered warm command with +``--network none``. A command that completes with no network cannot have +reached a model; a warm that leaves ``auth.json`` byte-identical spent no +refresh token. (The CLI may still write its own catalog cache, +``~/.codex/models_cache.json``, beside it; that file is not the credential.) + +Credentials are synthetic and live in throwaway volumes (see ``conftest``); no +test logs in, and none can reach a real ``helix-auth-*`` volume. +""" + +from __future__ import annotations + +import subprocess +import uuid + +import pytest + +from helix.backends import ( + BACKEND_AUTH_COMMANDS, + BACKENDS, + backend_credential_warm_skip_reason, +) + + +pytestmark = pytest.mark.docker_integration + + +CODEX_IMAGE = "ghcr.io/ke7/helix-evo-runner-codex:latest" + +# A well-shaped credential that no real service would ever accept. Only two +# things matter: that codex reads it as a stored ChatGPT login, and that its +# ``last_refresh`` is stamped now, so no refresh is due. The id_token is an +# unsigned JWT over invented claims -- codex parses it for the account fields +# and nothing here is a real token or can rotate a real grant. +SYNTHETIC_CODEX_ID_TOKEN = ( + "eyJhbGciOiJub25lIiwidHlwIjoiSldUIn0.eyJzdWIiOiJTWU5USEVUSUMiLCJleHAiOjQxMDI0NDQ4MDAsImVtYWlsIjoic3ludGhldGljQGV4YW1wbGUuaW52YWxpZCIsImh0dHBzOi8vYXBpLm9wZW5haS5jb20vYXV0aCI6eyJjaGF0Z3B0X3BsYW5fdHlwZSI6InBsdXMiLCJjaGF0Z3B0X2FjY291bnRfaWQiOiJTWU5USEVUSUMiLCJjaGF0Z3B0X3VzZXJfaWQiOiJTWU5USEVUSUMiLCJ1c2VyX2lkIjoiU1lOVEhFVElDIn19.SYNTHETIC" +) + +SYNTHETIC_FRESH_CODEX_AUTH = ( + "mkdir -p /home/node/.codex; " + "NOW=$(date -u +%Y-%m-%dT%H:%M:%S.000000Z); " + "printf '{\"OPENAI_API_KEY\":null,\"tokens\":{" + "\"id_token\":\"%s\",\"access_token\":\"SYNTHETIC\"," + "\"refresh_token\":\"SYNTHETIC\",\"account_id\":\"SYNTHETIC\"}," + "\"last_refresh\":\"%s\"}' " + f"\"{SYNTHETIC_CODEX_ID_TOKEN}\" \"$NOW\" " + "> /home/node/.codex/auth.json" +) + + +def _run_warm(*, image: str, volume: str, backend: str, timeout: int = 180): + """Run the backend's registered warm command the way ``helix.sandbox`` does. + + Same single-writer shape as ``sandbox_auth_docker_args``: one container, + the login volume read-write at ``/home/node``, nothing else attached. The + only deliberate difference is ``--network none``, which turns "this makes + no model call" from a claim into something the container can prove. + """ + args = [ + "docker", + "run", + "--rm", + # Named so a hang can be stopped by name; the volume fixture removes + # its volume in teardown, which fails while a container holds it. + "--name", + f"helix-integration-warm-{uuid.uuid4().hex[:12]}", + "--network", + "none", + "--security-opt", + "no-new-privileges", + "--user", + "node", + "-v", + f"{volume}:/home/node:rw", + "-e", + "HOME=/home/node", + "-e", + "PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin", + image, + *BACKEND_AUTH_COMMANDS[backend]["warm"], + ] + return subprocess.run( + args, capture_output=True, text=True, check=False, timeout=timeout + ) + + +def _files(listing: set[str]) -> set[str]: + """Keep only regular files from a ``volume_listing`` result. + + Directories are filtered out deliberately: the codex CLI scaffolds a few + empty ones (``.codex/tmp``, ``.codex/memories``) the first time it starts + in a fresh volume, and every candidate would create the same ones on its + own. What must not move is the credential and the files beside it. + """ + return {line for line in listing if line.startswith("-")} + + +def _auth_digest(volume: str, image: str) -> str: + """Hash the stored credential without reading it. + + The digest is the whole point: it proves the file did or did not change + without anything ever printing, decoding, or logging its contents. + """ + result = subprocess.run( + [ + "docker", + "run", + "--rm", + "--network", + "none", + "-v", + f"{volume}:/home/node:ro", + "--user", + "node", + image, + "sh", + "-c", + "sha256sum /home/node/.codex/auth.json | cut -d' ' -f1", + ], + capture_output=True, + text=True, + check=True, + timeout=120, + ) + return result.stdout.strip() + + +@pytest.mark.timeout(300) +def test_codex_warm_is_free(require_image, throwaway_volume) -> None: + """No model call, no quota -- proved by completing with no network at all. + + This is the constraint that decides whether the warm may ship: it runs on + the operator's paid account once per generation, so a status/whoami command + that quietly cost a request would be worse than the race it prevents. A + container with no network namespace cannot reach any endpoint, so a clean + exit here is proof rather than assertion. + """ + image = require_image(CODEX_IMAGE) + volume = throwaway_volume(image, SYNTHETIC_FRESH_CODEX_AUTH) + + result = _run_warm(image=image, volume=volume, backend="codex") + + assert result.returncode == 0, result.stderr + + +@pytest.mark.timeout(300) +def test_codex_warm_is_a_no_op_on_a_fresh_credential( + require_image, throwaway_volume, volume_listing +) -> None: + """A credential that is not due for refresh must come back untouched. + + If the warm rewrote a fresh credential it would be doing the very thing it + exists to prevent -- spending a single-use refresh token nobody needed to + spend -- once per generation. + """ + image = require_image(CODEX_IMAGE) + volume = throwaway_volume(image, SYNTHETIC_FRESH_CODEX_AUTH) + before_files = _files(volume_listing(volume, image)) + before_digest = _auth_digest(volume, image) + + result = _run_warm(image=image, volume=volume, backend="codex") + + assert result.returncode == 0, result.stderr + assert _auth_digest(volume, image) == before_digest + assert _files(volume_listing(volume, image)) == before_files + + +@pytest.mark.timeout(300) +def test_repeated_codex_warms_leave_no_residue( + require_image, throwaway_volume, volume_listing +) -> None: + """The warm runs every generation, so it must not accumulate. + + It also must not behave like a candidate: a command that opened a session + or wrote a rollout into the shared volume would hand the next candidate + state it did not create -- the contamination the per-candidate state work + on this branch removes. + """ + image = require_image(CODEX_IMAGE) + volume = throwaway_volume(image, SYNTHETIC_FRESH_CODEX_AUTH) + + first = _run_warm(image=image, volume=volume, backend="codex") + assert first.returncode == 0, first.stderr + after_first = volume_listing(volume, image) + digest_first = _auth_digest(volume, image) + + for _ in range(2): + again = _run_warm(image=image, volume=volume, backend="codex") + assert again.returncode == 0, again.stderr + + assert volume_listing(volume, image) == after_first + assert _auth_digest(volume, image) == digest_first + + +@pytest.mark.timeout(120) +def test_skipped_backends_have_no_warm_command_to_run() -> None: + """Skipping is a decision this suite is allowed to see. + + If a backend ever gains a warm command, it must gain container proof in + this file at the same time -- this assertion is what makes that fail loudly + instead of shipping an unmeasured per-generation call on a paid account. + """ + warmed = [b for b in BACKENDS if backend_credential_warm_skip_reason(b) is None] + assert warmed == ["codex"] diff --git a/tests/unit/test_cli_credential_failure.py b/tests/unit/test_cli_credential_failure.py new file mode 100644 index 00000000..110ddd60 --- /dev/null +++ b/tests/unit/test_cli_credential_failure.py @@ -0,0 +1,80 @@ +"""``helix evolve`` / ``helix resume`` never let a credential failure escape +as a raw traceback. + +Every in-loop path handles :class:`CredentialRefreshError` itself, so these +handlers are the last line: if a future path forgets, the operator still gets +the error panel with its suggestion and a resume hint, not a stack dump. +""" + +from __future__ import annotations + +from pathlib import Path + +import pytest +from click.testing import CliRunner + +from helix.cli import cli +from helix.exceptions import CredentialRefreshError + + +def _make_project(tmp_path: Path) -> Path: + (tmp_path / "helix.toml").write_text( + 'objective = "test"\n\n[evaluator]\ncommand = "true"\n' + ) + return tmp_path + + +@pytest.mark.parametrize("command", ["evolve", "resume"]) +@pytest.mark.parametrize("state_saved", [True, False]) +def test_escaped_credential_error_is_a_panel_not_a_traceback( + mocker, tmp_path: Path, command: str, state_saved: bool +) -> None: + project = _make_project(tmp_path) + if state_saved: + (project / ".helix").mkdir() + (project / ".helix" / "state.json").write_text("{}") + # Both commands import run_evolution lazily inside the function body. + mocker.patch( + "helix.evolution.run_evolution", + side_effect=CredentialRefreshError( + "Codex CLI could not use its stored credential", + suggestion="Re-authenticate with `helix sandbox login codex`.", + ), + ) + if command == "resume": + mocker.patch("helix.cli.load_state", return_value=None) + + result = CliRunner().invoke(cli, [command, "--dir", str(project)]) + + assert result.exit_code == 2, result.output + assert result.exception is None or isinstance(result.exception, SystemExit) + assert "Traceback" not in result.output + out = " ".join(result.output.lower().split()) + assert "credential" in out + assert "helix sandbox login" in out + if state_saved: + assert "state has been saved" in out + assert "helix resume" in out + else: + # The only path that reaches this handler is seedless seed + # generation, which fails before the first save; ``helix resume`` + # would start a fresh run and repeat the failure. + assert "no evolution state was saved" in out + assert "helix resume" not in out + assert "helix evolve" in out + + +def test_transient_failure_does_not_demand_a_relogin(mocker, tmp_path: Path) -> None: + project = _make_project(tmp_path) + (project / ".helix").mkdir() + (project / ".helix" / "state.json").write_text("{}") + exc = CredentialRefreshError("lost a refresh race", suggestion="resume") + exc.transient = True + mocker.patch("helix.evolution.run_evolution", side_effect=exc) + + result = CliRunner().invoke(cli, ["evolve", "--dir", str(project)]) + + out = " ".join(result.output.lower().split()) + assert result.exit_code == 2 + assert "helix resume" in out + assert "helix sandbox login" not in out diff --git a/tests/unit/test_credential_failure_classification.py b/tests/unit/test_credential_failure_classification.py new file mode 100644 index 00000000..926290a2 --- /dev/null +++ b/tests/unit/test_credential_failure_classification.py @@ -0,0 +1,911 @@ +"""A dead credential must not read as a candidate that wrote bad code. + +Before this, a run whose shared login stopped working looked exactly like a run +whose agent kept producing broken diffs: the mutation was abandoned, the slot +was dropped, and nothing said why. These tests pin the two halves of the fix +-- that the real wording the shipped CLIs emit is recognised, and that ordinary +agent output is not. + +Every positive string below is a phrase read out of the shipped binary of the +CLI it is attributed to, not invented for the test. The negatives are the +false-positive trap: candidate work that merely *mentions* tokens, auth, or a +401 must never be reported to an operator as a broken login. +""" + +from __future__ import annotations + +import json +import subprocess +from pathlib import Path +from typing import Any + +import pytest + +from helix.config import AgentConfig, EvaluatorConfig, HelixConfig +from helix.display import UsageStats +from helix.exceptions import ( + CredentialRefreshError, + HelixError, + MutationError, + RateLimitError, +) +from helix.mutator import ( + credential_failure_is_transient, + credential_failure_marker, + invoke_claude_code, + mutate, +) +from helix.population import Candidate +from tests.unit.test_mutator import make_eval_result # type: ignore[import-untyped] + + +# Codex CLI 0.130.0 -- the four suffixes it appends to one prefix, plus the +# bare form. The "already used" variant is what losing a refresh race looks +# like from inside a candidate. +CODEX_ALREADY_USED = ( + "Your access token could not be refreshed because your refresh token was " + "already used. Please log out and sign in again." +) +CODEX_EXPIRED = ( + "Your access token could not be refreshed because your refresh token has " + "expired. Please log out and sign in again." +) +CODEX_REVOKED = ( + "Your access token could not be refreshed because your refresh token was " + "revoked. Please log out and sign in again." +) +CODEX_OTHER_ACCOUNT = ( + "Your access token could not be refreshed because you have since logged " + "out or signed in to another account. Please sign in again." +) +CODEX_BARE = ( + "Your access token could not be refreshed. Please log out and sign in again." +) +CODEX_GET_ACCOUNT = "failed to refresh token while getting account: 401" +CODEX_NO_ACCOUNT = ( + "ChatGPT account ID not available, please re-run `codex login`" +) +# OpenCode 1.14.24 -- thrown by the provider fetch wrapper. +OPENCODE_REFRESH_FAILED = "Token refresh failed: 400" +# Claude Code 2.1.138. +CLAUDE_OAUTH_REFRESH = "User OAuth refresh failed (HTTP 401): invalid_grant" +CLAUDE_INVALID_KEY = "API Error: 401 Invalid API key · Please run /login" + +CREDENTIAL_STRINGS = [ + CODEX_ALREADY_USED, + CODEX_EXPIRED, + CODEX_REVOKED, + CODEX_OTHER_ACCOUNT, + CODEX_BARE, + CODEX_GET_ACCOUNT, + CODEX_NO_ACCOUNT, + OPENCODE_REFRESH_FAILED, + CLAUDE_OAUTH_REFRESH, + CLAUDE_INVALID_KEY, +] + +# Things a candidate legitimately produces while working on code. None of them +# is a statement about HELIX's own login. +INNOCENT_STRINGS = [ + "", + "401", + "token", + "auth", + "refresh", + "TypeError: 'NoneType' object is not subscriptable", + "FAILED tests/test_auth.py::test_refresh_token_rotation - assert 0 == 1", + "+ def refresh_token(self) -> str:\n+ raise NotImplementedError", + "The test suite returned 401 for 3 requests; see auth.py line 88.", + "Added a refresh token cache so the client stops re-authenticating.", + "Error: 529 overloaded, please retry", + "You have exceeded your usage limit", +] + + +class TestMarkerRecognition: + @pytest.mark.parametrize("text", CREDENTIAL_STRINGS) + def test_real_cli_wording_is_recognised(self, text: str) -> None: + assert credential_failure_marker(text) is not None + + @pytest.mark.parametrize("text", CREDENTIAL_STRINGS) + def test_recognition_is_case_insensitive(self, text: str) -> None: + assert credential_failure_marker(text.upper()) is not None + + @pytest.mark.parametrize("text", INNOCENT_STRINGS) + def test_ordinary_candidate_output_is_not_a_credential_failure( + self, text: str + ) -> None: + assert credential_failure_marker(text) is None + + def test_marker_is_returned_as_evidence(self) -> None: + """The caller names what matched instead of asserting a bare verdict.""" + marker = credential_failure_marker(CODEX_EXPIRED) + assert marker == "your access token could not be refreshed" + + def test_already_used_is_the_transient_marker(self) -> None: + """Losing a refresh race is named as such, not as a dead login. + + The already-used suffix is the exact outcome a lost race produces; it + must be matched ahead of the generic prefix so it can be retried and + so the operator is not sent to re-login over a working credential. + """ + marker = credential_failure_marker(CODEX_ALREADY_USED) + assert marker == "because your refresh token was already used" + assert credential_failure_is_transient(marker) + + @pytest.mark.parametrize( + "text", + [CODEX_EXPIRED, CODEX_REVOKED, CODEX_OTHER_ACCOUNT, CODEX_BARE, + CODEX_GET_ACCOUNT, CODEX_NO_ACCOUNT, OPENCODE_REFRESH_FAILED, + CLAUDE_OAUTH_REFRESH, CLAUDE_INVALID_KEY], + ) + def test_every_other_wording_is_not_transient(self, text: str) -> None: + marker = credential_failure_marker(text) + assert marker is not None + assert not credential_failure_is_transient(marker) + + def test_embedded_in_a_larger_stream_is_still_found(self) -> None: + stream = "\n".join( + ["running 3 tests", CODEX_ALREADY_USED, "process exited"] + ) + assert credential_failure_marker(stream) is not None + + +class TestExceptionTaxonomy: + def test_credential_error_is_a_helix_error(self) -> None: + assert isinstance(CredentialRefreshError("x"), HelixError) + + def test_credential_error_is_not_a_mutation_error(self) -> None: + """``except MutationError`` in mutate() must not swallow it as a + failed mutation -- that is the exact conflation being removed.""" + assert not isinstance(CredentialRefreshError("x"), MutationError) + + def test_credential_error_is_not_a_rate_limit(self) -> None: + """A rate limit clears on its own; an unusable credential does not.""" + assert not isinstance(CredentialRefreshError("x"), RateLimitError) + + +def _patch_backend( + mocker: Any, *, returncode: int, stdout: str = "", stderr: str = "" +) -> None: + result = subprocess.CompletedProcess( + args=["backend"], returncode=returncode, stdout=stdout, stderr=stderr + ) + mocker.patch("helix.mutator.subprocess.run", return_value=result) + + +def _jsonl(*events: dict[str, Any]) -> str: + return "\n".join(json.dumps(event) for event in events) + + +# The real shapes, read from the CLIs' own event definitions: +# codex ``ThreadErrorEvent`` / ``TurnFailedEvent`` in +# codex-rs/exec/src/exec_events.rs -- no ``is_error`` key exists +# anywhere in ``codex exec --json`` output. +# opencode ``emit("error", { error: props.error })`` in cli/cmd/run.ts, +# where ``props.error`` is ``{ name, data: { message } }``. +def _codex_error_event(message: str) -> dict[str, Any]: + return {"type": "error", "message": message} + + +def _codex_turn_failed(message: str) -> dict[str, Any]: + return {"type": "turn.failed", "error": {"message": message}} + + +def _opencode_error_event(message: str) -> dict[str, Any]: + return { + "type": "error", + "timestamp": 1757500000000, + "sessionID": "ses_x", + "error": {"name": "UnknownError", "data": {"message": message}}, + } + + +def _codex_command_output(output: str) -> dict[str, Any]: + """A completed ``command_execution`` item: the candidate's own tool output.""" + return { + "type": "item.completed", + "item": { + "id": "item_1", + "type": "command_execution", + "command": "pytest -q", + "aggregated_output": output, + "exit_code": 1, + "status": "completed", + }, + } + + +def _codex_agent_message(text: str) -> dict[str, Any]: + return { + "type": "item.completed", + "item": {"id": "item_2", "type": "agent_message", "text": text}, + } + + +class TestInvocationClassification: + def test_non_zero_exit_with_cli_wording_on_stderr( + self, mocker: Any, tmp_path: Path + ) -> None: + _patch_backend(mocker, returncode=1, stderr=CODEX_EXPIRED) + with pytest.raises(CredentialRefreshError) as exc: + invoke_claude_code( + str(tmp_path), "p", AgentConfig(backend="codex") + ) + err = exc.value + assert err.exit_code == 1 + assert err.stderr == CODEX_EXPIRED + assert err.transient is False + assert "credential" in err.suggestion.lower() + assert "helix sandbox login codex" in err.suggestion + + def test_zero_exit_codex_error_event_is_read( + self, mocker: Any, tmp_path: Path + ) -> None: + """Codex swallows its own refresh failure: exit 0, empty stderr. + + Measured on codex-cli 0.130.0 against a synthetic credential whose + refresh was rejected -- the process exits 0 and prints nothing on + stderr, even at RUST_LOG=info. What it does emit is its own + ``{"type": "error", "message": ...}`` event, so that is what has to + be read. + """ + stream = _jsonl( + {"type": "thread.started", "thread_id": "t1"}, + _codex_error_event(CODEX_ALREADY_USED), + ) + _patch_backend(mocker, returncode=0, stdout=stream) + with pytest.raises(CredentialRefreshError) as exc: + invoke_claude_code( + str(tmp_path), "p", AgentConfig(backend="codex") + ) + assert exc.value.exit_code == 0 + assert exc.value.transient is True + assert "structured error event" in str(exc.value) + + def test_zero_exit_codex_turn_failed_is_read( + self, mocker: Any, tmp_path: Path + ) -> None: + stream = _jsonl( + {"type": "thread.started", "thread_id": "t1"}, + {"type": "turn.started"}, + _codex_turn_failed(CODEX_EXPIRED), + ) + _patch_backend(mocker, returncode=0, stdout=stream) + with pytest.raises(CredentialRefreshError) as exc: + invoke_claude_code( + str(tmp_path), "p", AgentConfig(backend="codex") + ) + assert exc.value.transient is False + + def test_zero_exit_opencode_error_event_is_read( + self, mocker: Any, tmp_path: Path + ) -> None: + stream = _jsonl( + {"type": "step_start", "sessionID": "ses_x"}, + _opencode_error_event(OPENCODE_REFRESH_FAILED), + ) + _patch_backend(mocker, returncode=0, stdout=stream) + with pytest.raises(CredentialRefreshError): + invoke_claude_code( + str(tmp_path), "p", AgentConfig(backend="opencode") + ) + + def test_claude_errors_list_is_read( + self, mocker: Any, tmp_path: Path + ) -> None: + """Claude Code's error envelope carries an ``errors`` list, not a string.""" + envelope = json.dumps( + { + "type": "result", + "subtype": "error_during_execution", + "is_error": True, + "errors": [CLAUDE_OAUTH_REFRESH], + } + ) + _patch_backend(mocker, returncode=0, stdout=envelope) + with pytest.raises(CredentialRefreshError): + invoke_claude_code( + str(tmp_path), "p", AgentConfig(backend="claude") + ) + + def test_claude_errored_result_is_read( + self, mocker: Any, tmp_path: Path + ) -> None: + envelope = json.dumps( + { + "type": "result", + "subtype": "error_during_execution", + "is_error": True, + "result": CLAUDE_INVALID_KEY, + } + ) + _patch_backend(mocker, returncode=0, stdout=envelope) + with pytest.raises(CredentialRefreshError): + invoke_claude_code( + str(tmp_path), "p", AgentConfig(backend="claude") + ) + + def test_claude_successful_result_prose_is_not_read( + self, mocker: Any, tmp_path: Path + ) -> None: + """On a successful turn ``result`` is the assistant's own words.""" + envelope = json.dumps( + { + "type": "result", + "subtype": "success", + "is_error": False, + "result": f"I fixed the handler that raised '{CLAUDE_INVALID_KEY}'.", + } + ) + _patch_backend(mocker, returncode=0, stdout=envelope) + parsed, _usage = invoke_claude_code( + str(tmp_path), "p", AgentConfig(backend="claude") + ) + assert parsed["subtype"] == "success" + + def test_tool_result_is_not_evidence( + self, mocker: Any, tmp_path: Path + ) -> None: + """A failing tool call is the agent's own failing command -- an + ordinary code failure -- whatever flag the backend puts on it.""" + stream = _jsonl( + { + "type": "tool_result", + "is_error": True, + "content": f"pytest exited 1: {CODEX_ALREADY_USED}", + }, + {"type": "turn.completed"}, + ) + _patch_backend(mocker, returncode=0, stdout=stream) + parsed, _usage = invoke_claude_code( + str(tmp_path), "p", AgentConfig(backend="codex") + ) + assert parsed["events"] + + def test_agent_prose_with_marker_on_zero_exit_is_not_classified( + self, mocker: Any, tmp_path: Path + ) -> None: + """The agent quoting the CLI's own wording is still prose.""" + stream = _jsonl( + {"type": "thread.started", "thread_id": "t1"}, + _codex_agent_message( + f"The old client printed '{CODEX_ALREADY_USED}'; I removed it." + ), + {"type": "turn.completed", "usage": {"input_tokens": 1}}, + ) + _patch_backend(mocker, returncode=0, stdout=stream) + parsed, _usage = invoke_claude_code( + str(tmp_path), "p", AgentConfig(backend="codex") + ) + assert parsed["events"] + + def test_candidate_editing_oauth_code_is_not_classified( + self, mocker: Any, tmp_path: Path + ) -> None: + """A candidate whose own work is about refresh tokens must not be able + to talk HELIX into declaring the operator's login broken.""" + stream = _jsonl( + _codex_command_output( + "FAILED tests/test_oauth.py::test_reuse - " + "expected the refresh token to be rejected" + ), + {"type": "turn.completed"}, + ) + _patch_backend(mocker, returncode=0, stdout=stream) + parsed, _usage = invoke_claude_code( + str(tmp_path), "p", AgentConfig(backend="codex") + ) + assert parsed["events"] + + def test_non_zero_exit_never_scans_tool_output_in_the_transcript( + self, mocker: Any, tmp_path: Path + ) -> None: + """Exit 1 on a 502 after the candidate's tests printed opencode's + refresh wording into a command's output is a generic failure.""" + stream = _jsonl( + {"type": "thread.started", "thread_id": "t1"}, + _codex_command_output(f"E RuntimeError: {OPENCODE_REFRESH_FAILED}"), + ) + _patch_backend( + mocker, returncode=1, stdout=stream, stderr="error: 502 Bad Gateway" + ) + with pytest.raises(MutationError) as exc: + invoke_claude_code( + str(tmp_path), "p", AgentConfig(backend="codex") + ) + assert not isinstance(exc.value, CredentialRefreshError) + + def test_non_zero_exit_never_scans_raw_stdout_prose( + self, mocker: Any, tmp_path: Path + ) -> None: + _patch_backend( + mocker, + returncode=1, + stdout=f"note: {CODEX_EXPIRED}", + stderr="Traceback (most recent call last): SyntaxError", + ) + with pytest.raises(MutationError) as exc: + invoke_claude_code( + str(tmp_path), "p", AgentConfig(backend="codex") + ) + assert not isinstance(exc.value, CredentialRefreshError) + + @pytest.mark.parametrize("is_error", [False, True]) + def test_claude_max_turns_with_auth_wording_in_prose_stays_partial_success( + self, mocker: Any, tmp_path: Path, is_error: bool + ) -> None: + """Pre-existing contract: max-turns exhaustion is partial success + because the edits may be useful. It is decided before any credential + check, so the assistant's prose in ``result`` is never read.""" + envelope = json.dumps( + { + "type": "result", + "subtype": "error_max_turns", + "is_error": is_error, + "num_turns": 30, + "result": f"I hit '{CLAUDE_INVALID_KEY}' in the old test fixture.", + } + ) + _patch_backend(mocker, returncode=1, stdout=envelope) + parsed, _usage = invoke_claude_code( + str(tmp_path), "p", AgentConfig(backend="claude") + ) + assert parsed["subtype"] == "error_max_turns" + + def test_ordinary_non_zero_exit_stays_a_mutation_error( + self, mocker: Any, tmp_path: Path + ) -> None: + _patch_backend( + mocker, + returncode=1, + stderr="Traceback (most recent call last): SyntaxError", + ) + with pytest.raises(MutationError): + invoke_claude_code( + str(tmp_path), "p", AgentConfig(backend="codex") + ) + + def test_rate_limit_still_wins_its_own_classification( + self, mocker: Any, tmp_path: Path + ) -> None: + _patch_backend( + mocker, returncode=1, stderr="Error: 529 overloaded please retry" + ) + with pytest.raises(RateLimitError): + invoke_claude_code( + str(tmp_path), "p", AgentConfig(backend="codex") + ) + + def test_invoke_itself_never_retries( + self, mocker: Any, tmp_path: Path + ) -> None: + """The retry needs a fresh worktree, which only the caller owns.""" + run = mocker.patch( + "helix.mutator.subprocess.run", + return_value=_completed(1, stderr=CODEX_ALREADY_USED), + ) + with pytest.raises(CredentialRefreshError) as exc: + invoke_claude_code( + str(tmp_path), "p", AgentConfig(backend="codex") + ) + assert run.call_count == 1 + assert exc.value.transient is True + assert "retry" not in str(exc.value).lower() + + def test_retried_flag_changes_the_second_loss_wording( + self, mocker: Any, tmp_path: Path + ) -> None: + _patch_backend(mocker, returncode=1, stderr=CODEX_ALREADY_USED) + with pytest.raises(CredentialRefreshError) as exc: + invoke_claude_code( + str(tmp_path), "p", AgentConfig(backend="codex"), retried=True + ) + assert "failed again on retry" in str(exc.value) + assert "helix resume" in exc.value.suggestion + assert "sandbox login" not in exc.value.suggestion + + +def _completed( + returncode: int, stdout: str = "", stderr: str = "" +) -> subprocess.CompletedProcess[str]: + return subprocess.CompletedProcess( + args=["backend"], returncode=returncode, stdout=stdout, stderr=stderr + ) + + +def _codex_stream(*, session: str, input_tokens: int, output_tokens: int) -> str: + return _jsonl( + {"type": "session.started", "session_id": session}, + { + "type": "turn.completed", + "usage": { + "input_tokens": input_tokens, + "output_tokens": output_tokens, + }, + }, + ) + + +CODEX_SUCCESS_STREAM = _codex_stream(session="won", input_tokens=12, output_tokens=8) +CODEX_LOST_STREAM = _codex_stream(session="lost", input_tokens=5, output_tokens=2) + + +def _codex_config() -> HelixConfig: + return HelixConfig( + objective="Improve the code", + evaluator=EvaluatorConfig(command="true"), + agent=AgentConfig(backend="codex"), + ) + + +def _candidate(cid: str, path: Path) -> Candidate: + return Candidate( + id=cid, + worktree_path=str(path), + branch_name=f"helix/{cid}", + generation=0, + parent_id=None, + parent_ids=[], + operation="seed", + ) + + +class _RetryHarness: + """``mutate()`` with a clone that hands out a fresh directory per call. + + Each ``clone_candidate`` call creates ``/clone`` so the test can + see which tree each attempt ran in, what it left behind, and which trees + were removed. + """ + + def __init__(self, tmp_path: Path, mocker: Any) -> None: + self.tmp_path = tmp_path + self.clones: list[Candidate] = [] + self.removed: list[Candidate] = [] + self.run_cwds: list[str] = [] + self.spent: list[UsageStats] = [] + self.recovered: list[str] = [] + self.parent = _candidate("g0-s0", tmp_path / "parent") + + def _clone(parent: Candidate, new_id: str, base_dir: Path) -> Candidate: + path = tmp_path / f"clone{len(self.clones) + 1}" + path.mkdir() + child = _candidate(new_id, path) + self.clones.append(child) + return child + + mocker.patch("helix.mutator.clone_candidate", side_effect=_clone) + mocker.patch( + "helix.mutator.remove_worktree", side_effect=self.removed.append + ) + mocker.patch("helix.mutator.snapshot_candidate") + self.mocker = mocker + + def run_backend(self, outcomes: list[Any]) -> Any: + """Patch ``subprocess.run`` with per-attempt outcomes. + + An outcome is a ``CompletedProcess`` to return or an exception to + raise. Every attempt first drops ``partial.py`` into its cwd, the way + a backend that lost mid-turn leaves half-applied edits behind. + """ + outcomes = list(outcomes) + + def _run(args: list[str], **kwargs: Any) -> subprocess.CompletedProcess[str]: + cwd = kwargs["cwd"] + self.run_cwds.append(cwd) + (Path(cwd) / "partial.py").write_text("# half-applied edit\n") + outcome = outcomes.pop(0) + if isinstance(outcome, BaseException): + raise outcome + return outcome + + return self.mocker.patch("helix.mutator.subprocess.run", side_effect=_run) + + def mutate(self) -> Candidate | None: + return mutate( + self.parent, + make_eval_result("g0-s0"), + "g1-s0", + _codex_config(), + self.tmp_path, + record_usage=self.spent.append, + on_refresh_race_recovered=self.recovered.append, + ) + + +class TestLostRefreshRaceIsRetried: + """The already-used outcome is the race this work exists to handle. + + When it happens, the *winner* has just written a refreshed credential to + the shared volume, so the right response is to invoke again against it, + not to drop the slot and tell the operator to redo a login that is fine. + The retry lives in ``mutate()`` / ``merge()`` rather than in + ``invoke_claude_code`` because it must start from a fresh worktree. + """ + + def test_retry_runs_in_a_fresh_worktree( + self, mocker: Any, tmp_path: Path + ) -> None: + """Attempt 1's half-applied edits must not be under the retry.""" + h = _RetryHarness(tmp_path, mocker) + run = h.run_backend( + [ + _completed(1, stdout=CODEX_LOST_STREAM, stderr=CODEX_ALREADY_USED), + _completed(0, stdout=CODEX_SUCCESS_STREAM), + ] + ) + + child = h.mutate() + + assert run.call_count == 2 + assert len(h.clones) == 2 + assert h.run_cwds == [h.clones[0].worktree_path, h.clones[1].worktree_path] + assert child is h.clones[1] + # The first tree was discarded before the retry, and the retry's tree + # was clean when the backend started in it. + assert h.removed == [h.clones[0]] + retry_tree = Path(h.clones[1].worktree_path) + assert sorted(p.name for p in retry_tree.iterdir() if p.name == "partial.py") == [ + "partial.py" + ], "only the retry's own edit is present" + assert not (retry_tree / ".helix_backend_result.attempt1.json").read_text().count( + "partial" + ) + + def test_both_attempts_artifacts_are_kept( + self, mocker: Any, tmp_path: Path + ) -> None: + h = _RetryHarness(tmp_path, mocker) + h.run_backend( + [ + _completed(1, stdout=CODEX_LOST_STREAM, stderr=CODEX_ALREADY_USED), + _completed(0, stdout=CODEX_SUCCESS_STREAM), + ] + ) + + child = h.mutate() + assert child is not None + tree = Path(child.worktree_path) + + # Attempt 1, under its own names, with the marker that explains why + # there was a retry. + assert (tree / ".helix_backend_stderr.attempt1.txt").read_text() == CODEX_ALREADY_USED + assert (tree / ".helix_backend_stdout.attempt1.txt").read_text() == CODEX_LOST_STREAM + first = json.loads((tree / ".helix_backend_result.attempt1.json").read_text()) + assert first["returncode"] == 1 + assert first["usage"]["input_tokens"] == 5 + + # The final attempt's artifact accounts for both. + final = json.loads((tree / ".helix_backend_result.json").read_text()) + assert final["returncode"] == 0 + assert final["attempts"] == 2 + assert final["retry_of"] == ".helix_backend_result.attempt1.json" + assert final["usage_first_attempt"]["input_tokens"] == 5 + assert final["usage_combined"]["input_tokens"] == 5 + 12 + assert final["usage_combined"]["output_tokens"] == 2 + 8 + + # And none of it can leak into the candidate's git tree. + gitignore = (tree / ".gitignore").read_text() + for name in ( + ".helix_backend_result.attempt1.json", + ".helix_backend_stdout.attempt1.txt", + ".helix_backend_stderr.attempt1.txt", + ): + assert name in gitignore + + def test_recovered_race_is_reported_and_charged_once( + self, mocker: Any, tmp_path: Path + ) -> None: + h = _RetryHarness(tmp_path, mocker) + h.run_backend( + [ + _completed(1, stdout=CODEX_LOST_STREAM, stderr=CODEX_ALREADY_USED), + _completed(0, stdout=CODEX_SUCCESS_STREAM), + ] + ) + + child = h.mutate() + assert child is not None + + # The caller sees the sum, exactly once, and the child carries it. + assert len(h.spent) == 1 + assert h.spent[0].input_tokens == 5 + 12 + assert h.spent[0].output_tokens == 2 + 8 + assert h.spent[0].session_id == "won" + assert child.usage.input_tokens == 5 + 12 + # The race is visible to whoever keeps the end-of-run summary. + assert len(h.recovered) == 1 + assert "recovered after a lost refresh race" in h.recovered[0] + assert "g1-s0" in h.recovered[0] + + def test_zero_exit_codex_error_event_is_retried_too( + self, mocker: Any, tmp_path: Path + ) -> None: + """Codex swallows the failure on exit 0; the event path retries as well.""" + h = _RetryHarness(tmp_path, mocker) + run = h.run_backend( + [ + _completed( + 0, + stdout=_jsonl( + {"type": "thread.started", "thread_id": "t1"}, + _codex_error_event(CODEX_ALREADY_USED), + ), + ), + _completed(0, stdout=CODEX_SUCCESS_STREAM), + ] + ) + assert h.mutate() is h.clones[1] + assert run.call_count == 2 + + def test_second_loss_is_raised_with_the_sum_and_no_relogin_instruction( + self, mocker: Any, tmp_path: Path + ) -> None: + h = _RetryHarness(tmp_path, mocker) + run = h.run_backend( + [ + _completed(1, stdout=CODEX_LOST_STREAM, stderr=CODEX_ALREADY_USED), + _completed( + 1, + stdout=_codex_stream(session="lost-2", input_tokens=3, output_tokens=1), + stderr=CODEX_ALREADY_USED, + ), + ] + ) + with pytest.raises(CredentialRefreshError) as exc: + h.mutate() + err = exc.value + assert run.call_count == 2 # exactly one retry, no loop + assert err.transient is True + assert "failed again on retry" in str(err) + # The stored credential is the winner's fresh one; do not tell the + # operator to throw it away. + assert "sandbox login" not in err.suggestion + assert "helix resume" in err.suggestion + # Both attempts' spend, attached to the error and handed to the sink. + assert err.usage is not None + assert err.usage.input_tokens == 5 + 3 + assert err.usage.output_tokens == 2 + 1 + assert err.usage.session_id == "lost-2" + assert h.spent == [err.usage] + # Neither worktree leaks. + assert h.removed == h.clones + assert h.recovered == [] + + def test_retry_failing_for_another_reason_still_carries_the_sum( + self, mocker: Any, tmp_path: Path + ) -> None: + """The retry's error class does not matter; the first spend rides along.""" + h = _RetryHarness(tmp_path, mocker) + h.run_backend( + [ + _completed(1, stdout=CODEX_LOST_STREAM, stderr=CODEX_ALREADY_USED), + _completed( + 2, + stdout=_codex_stream(session="crashed", input_tokens=4, output_tokens=0), + stderr="segfault", + ), + ] + ) + assert h.mutate() is None # MutationError -> None, by contract + assert len(h.spent) == 1 + assert h.spent[0].input_tokens == 5 + 4 + assert h.spent[0].output_tokens == 2 + assert h.removed == h.clones + + def test_timeout_on_the_retry_records_the_first_spend_and_cleans_up( + self, mocker: Any, tmp_path: Path + ) -> None: + """A sandbox ``TimeoutExpired`` is not a HelixError and carries no + usage: the first attempt's spend must still reach the sink, and the + retry's worktree must not leak.""" + h = _RetryHarness(tmp_path, mocker) + h.run_backend( + [ + _completed(1, stdout=CODEX_LOST_STREAM, stderr=CODEX_ALREADY_USED), + subprocess.TimeoutExpired(cmd=["codex"], timeout=30), + ] + ) + with pytest.raises(subprocess.TimeoutExpired): + h.mutate() + assert len(h.spent) == 1 + assert h.spent[0].input_tokens == 5 + assert h.spent[0].output_tokens == 2 + assert h.removed == h.clones + + def test_timeout_on_the_first_attempt_cleans_up( + self, mocker: Any, tmp_path: Path + ) -> None: + h = _RetryHarness(tmp_path, mocker) + h.run_backend([subprocess.TimeoutExpired(cmd=["codex"], timeout=30)]) + with pytest.raises(subprocess.TimeoutExpired): + h.mutate() + assert h.spent == [] + assert h.removed == h.clones == h.clones[:1] + + @pytest.mark.parametrize("text", [CODEX_EXPIRED, CODEX_REVOKED, CODEX_BARE]) + def test_a_dead_login_is_not_retried( + self, mocker: Any, tmp_path: Path, text: str + ) -> None: + """Retrying an expired or revoked credential only burns a turn.""" + h = _RetryHarness(tmp_path, mocker) + run = h.run_backend([_completed(1, stdout=CODEX_LOST_STREAM, stderr=text)]) + with pytest.raises(CredentialRefreshError) as exc: + h.mutate() + assert run.call_count == 1 + assert len(h.clones) == 1 + assert exc.value.transient is False + assert "helix sandbox login codex" in exc.value.suggestion + # Item: the credential path records usage like its siblings. + assert len(h.spent) == 1 + assert h.spent[0].input_tokens == 5 + assert h.removed == h.clones + + +class TestRetryOnARealWorktree: + """The same contract against real git worktrees, not a stubbed clone.""" + + def test_retry_starts_from_the_parent_commit( + self, mocker: Any, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + from helix.worktree import create_seed_worktree + + for key, value in { + "GIT_AUTHOR_NAME": "HELIX Test", + "GIT_AUTHOR_EMAIL": "helix@test.local", + "GIT_COMMITTER_NAME": "HELIX Test", + "GIT_COMMITTER_EMAIL": "helix@test.local", + }.items(): + monkeypatch.setenv(key, value) + project = tmp_path / "project" + project.mkdir() + (project / "main.py").write_text("print('hello')\n") + base_dir = tmp_path / "worktrees" + seed = create_seed_worktree(project, base_dir) + + seen: list[dict[str, Any]] = [] + outcomes = [ + _completed(1, stdout=CODEX_LOST_STREAM, stderr=CODEX_ALREADY_USED), + _completed(0, stdout=CODEX_SUCCESS_STREAM), + ] + + real_run = subprocess.run + + def _run(args: list[str], **kwargs: Any) -> subprocess.CompletedProcess[str]: + if args[0] != "codex": + # git, driven by the real clone_candidate / remove_worktree + return real_run(args, **kwargs) + cwd = Path(kwargs["cwd"]) + seen.append( + { + "cwd": str(cwd), + "partial_present": (cwd / "partial.py").exists(), + "main": (cwd / "main.py").read_text(), + } + ) + (cwd / "partial.py").write_text("# half-applied\n") + (cwd / "main.py").write_text("print('half edited')\n") + return outcomes.pop(0) + + mocker.patch("helix.mutator.subprocess.run", side_effect=_run) + recovered: list[str] = [] + child = mutate( + seed, + make_eval_result(seed.id), + "g1-s0", + _codex_config(), + base_dir, + on_refresh_race_recovered=recovered.append, + ) + + assert child is not None + assert len(seen) == 2 + assert seen[0]["cwd"] == seen[1]["cwd"] == str(base_dir / "g1-s0") + # The retry saw the parent's tree, not attempt 1's half-applied edits. + assert seen[1]["partial_present"] is False + assert seen[1]["main"] == "print('hello')\n" + tree = Path(child.worktree_path) + assert (tree / ".helix_backend_result.attempt1.json").is_file() + assert (tree / ".helix_backend_result.json").is_file() + assert recovered and "recovered after a lost refresh race" in recovered[0] diff --git a/tests/unit/test_credential_warm.py b/tests/unit/test_credential_warm.py new file mode 100644 index 00000000..8ebee4c8 --- /dev/null +++ b/tests/unit/test_credential_warm.py @@ -0,0 +1,689 @@ +"""The per-generation credential warm: who gets one, and what it costs. + +Candidates share one login volume read-write. When a refresh comes due they +can each decide to refresh independently and spend the same single-use refresh +token; whoever loses that race can fail without saying anything. The warm +closes the window by doing the refresh once, in one container, before a +generation dispatches anything. + +Two properties are load-bearing and both are asserted here: the warm runs +through the existing single-writer auth path, and a backend is warmed only when +warming it is both useful and free. A backend with no warm command must carry +a written reason -- skipping is a claim about correctness, not an omission. +""" + +from __future__ import annotations + +import subprocess +from datetime import datetime, timedelta, timezone +from typing import Any + +import pytest + +from helix.backends import ( + BACKEND_AUTH_COMMANDS, + BACKENDS, + CREDENTIAL_WARM_SKIP_REASONS, + backend_credential_warm_skip_reason, +) +from helix.config import ( + AgentConfig, + EvaluatorConfig, + EvolutionConfig, + HelixConfig, + SandboxConfig, +) +from helix.evolution import _warm_generation_credential +from helix.sandbox import ( + CODEX_TOKEN_REFRESH_INTERVAL, + CREDENTIAL_WARM_TIMEOUT_SECONDS, + CredentialWarmResult, + credential_warm_timeout, + sandbox_auth_docker_args, + verify_codex_credential_fresh, + warm_backend_credential, +) + + +WARMED_BACKENDS = ("codex",) +SKIPPED_BACKENDS = ("agy", "claude", "cursor", "opencode") + + +def _completed( + returncode: int, stderr: str = "", stdout: str = "" +) -> subprocess.CompletedProcess[str]: + return subprocess.CompletedProcess( + args=["docker"], returncode=returncode, stdout=stdout, stderr=stderr + ) + + +NOW = datetime(2026, 9, 10, 12, 0, 0, tzinfo=timezone.utc) + + +def _stamp(dt: datetime) -> str: + return dt.strftime("%Y-%m-%dT%H:%M:%S.%fZ") + "\n" + + +class _FakeAuthCommands: + """Stand-in for ``run_sandbox_auth_command`` that answers the + ``last_refresh`` probes from a script and records the warm call. + + ``timeline`` is the sequence of values the probe returns (before, after); + ``warm`` is the warm command's result. + """ + + def __init__( + self, + timeline: list[str | None], + warm: subprocess.CompletedProcess[str] | BaseException | None = None, + ) -> None: + self.timeline = list(timeline) + self.warm = warm if warm is not None else _completed(0) + self.calls: list[dict[str, Any]] = [] + + def __call__(self, backend: str, **kwargs: Any) -> subprocess.CompletedProcess[str]: + record = dict(kwargs) + record["backend"] = backend + self.calls.append(record) + if kwargs.get("command") is not None: + value = self.timeline.pop(0) + return _completed(0, stdout="" if value is None else value) + if isinstance(self.warm, BaseException): + raise self.warm + return self.warm + + @property + def warm_calls(self) -> list[dict[str, Any]]: + return [c for c in self.calls if c.get("command") is None] + + @property + def probe_calls(self) -> list[dict[str, Any]]: + return [c for c in self.calls if c.get("command") is not None] + + +def _fresh_timeline() -> list[str | None]: + """A credential refreshed an hour ago: nothing to do, verifiably fresh.""" + stamp = _stamp(NOW - timedelta(hours=1)) + return [stamp, stamp] + + +def _install(monkeypatch: pytest.MonkeyPatch, fake: _FakeAuthCommands) -> None: + monkeypatch.setattr("helix.sandbox.run_sandbox_auth_command", fake) + monkeypatch.setattr("helix.sandbox.datetime", _FrozenDatetime) + + +class _FrozenDatetime(datetime): + @classmethod + def now(cls, tz: Any = None) -> "datetime": # type: ignore[override] + return NOW if tz is not None else NOW.replace(tzinfo=None) + + +# --------------------------------------------------------------------------- +# Registry: every backend is either warmed or explained +# --------------------------------------------------------------------------- + + +class TestWarmRegistry: + def test_every_backend_is_warmed_or_has_a_reason(self) -> None: + """No backend may fall through silently. + + A missing warm command with no recorded reason is indistinguishable + from an oversight, which is exactly the state this change exists to + leave behind. + """ + for backend in BACKENDS: + has_warm = "warm" in BACKEND_AUTH_COMMANDS[backend] + has_reason = backend in CREDENTIAL_WARM_SKIP_REASONS + assert has_warm != has_reason, backend + + @pytest.mark.parametrize("backend", WARMED_BACKENDS) + def test_warmed_backend_has_no_skip_reason(self, backend: str) -> None: + assert backend_credential_warm_skip_reason(backend) is None + + @pytest.mark.parametrize("backend", SKIPPED_BACKENDS) + def test_skipped_backend_states_why(self, backend: str) -> None: + reason = backend_credential_warm_skip_reason(backend) + assert reason is not None + # A reason a maintainer cannot act on is not a reason. + assert len(reason) > 60 + + def test_unknown_backend_is_not_warmed(self) -> None: + assert backend_credential_warm_skip_reason("nope") is not None + + def test_codex_warm_is_not_login_status(self) -> None: + """``codex login status`` never takes the refresh path. + + Measured against codex-cli 0.130.0 with a synthetic credential in a + throwaway volume: it exits 0 and issues no request whether the stored + ``last_refresh`` is minutes or 30 days old. Warming with it would look + like protection while providing none, so the registry must not drift + back to it. + """ + warm = BACKEND_AUTH_COMMANDS["codex"]["warm"] + assert warm != BACKEND_AUTH_COMMANDS["codex"]["status"] + assert "login" not in " ".join(warm) + + +# --------------------------------------------------------------------------- +# The warm reuses the single-writer sandboxed auth path +# --------------------------------------------------------------------------- + + +class TestWarmUsesSingleWriterPath: + def test_warm_action_mounts_the_shared_login_volume_read_write(self) -> None: + args = sandbox_auth_docker_args( + "codex", image="img:latest", action="warm" + ) + assert "-v" in args + assert "helix-auth-codex:/home/node:rw" in args + # One container, one command, nothing else attached to the volume. + assert args[:3] == ["docker", "run", "--rm"] + + def test_warm_action_runs_the_registered_warm_command(self) -> None: + args = sandbox_auth_docker_args( + "codex", image="img:latest", action="warm" + ) + assert args[-len(BACKEND_AUTH_COMMANDS["codex"]["warm"]) :] == ( + BACKEND_AUTH_COMMANDS["codex"]["warm"] + ) + + def test_warm_action_is_rejected_for_a_backend_without_one(self) -> None: + with pytest.raises(ValueError): + sandbox_auth_docker_args("cursor", image="img:latest", action="warm") + + def test_warm_forwards_sandbox_network_settings( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + fake = _FakeAuthCommands(_fresh_timeline()) + _install(monkeypatch, fake) + sandbox = SandboxConfig( + enabled=True, + network="none", + add_host_gateway=True, + extra_hosts={"h": "1.2.3.4"}, + image="custom:tag", + ) + result = warm_backend_credential("codex", sandbox=sandbox) + + assert result.warmed is True + [seen] = fake.warm_calls + assert seen["backend"] == "codex" + assert seen["action"] == "warm" + assert seen["network"] == "none" + assert seen["add_host_gateway"] is True + assert seen["extra_hosts"] == {"h": "1.2.3.4"} + # A configured image must win, or the warm would refresh the credential + # with a different CLI build than the candidates use. + assert seen["image"] == "custom:tag" + + def test_operator_env_reaches_the_warm_container( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + """Proxy / CA settings the candidates get must reach the warm too, or + it cannot reach the token endpoint the candidates can.""" + fake = _FakeAuthCommands(_fresh_timeline()) + _install(monkeypatch, fake) + env = {"HTTPS_PROXY": "http://proxy:3128", "SSL_CERT_FILE": "/ca.pem"} + warm_backend_credential( + "codex", sandbox=SandboxConfig(enabled=True), env=env + ) + [seen] = fake.warm_calls + assert seen["env"] == env + args = sandbox_auth_docker_args( + "codex", image="img:latest", action="warm", env=env + ) + joined = " ".join(args) + assert "-e HTTPS_PROXY=http://proxy:3128" in joined + assert "-e SSL_CERT_FILE=/ca.pem" in joined + # HOME and PATH stay pinned to the container's own values. + assert "-e HOME=/home/node" in joined + assert sandbox_auth_docker_args( + "codex", image="img", action="warm", env={"HOME": "/x", "PATH": "/y"} + ).count("-e") == 2 + + def test_no_auth_command_uses_a_login_shell(self) -> None: + """``sh -l`` sources ``$HOME/.profile`` from the shared login volume + that every candidate can write; PATH is pinned with ``-e`` instead.""" + for backend, actions in BACKEND_AUTH_COMMANDS.items(): + for action, argv in actions.items(): + if argv[0] != "sh": + continue + assert argv[1] == "-c", (backend, action, argv) + assert not any( + flag.startswith("-") and "l" in flag for flag in argv[1:-1] + ), (backend, action, argv) + + def test_last_refresh_probe_runs_without_network_and_reads_one_field( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + fake = _FakeAuthCommands(_fresh_timeline()) + _install(monkeypatch, fake) + warm_backend_credential("codex", sandbox=SandboxConfig(enabled=True)) + before, after = fake.probe_calls + for probe in (before, after): + assert probe["network"] == "none" + script = probe["command"][-1] + assert "last_refresh" in script + assert "access_token" not in script and "cat " not in script + + +# --------------------------------------------------------------------------- +# Failure of the warm is reported, never fatal +# --------------------------------------------------------------------------- + + +class TestWarmFailureIsNotFatal: + def test_skipped_backend_starts_no_container( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + def _boom(*_a: Any, **_k: Any) -> None: + raise AssertionError("a skipped backend must not run a container") + + monkeypatch.setattr("helix.sandbox.run_sandbox_auth_command", _boom) + result = warm_backend_credential( + "cursor", sandbox=SandboxConfig(enabled=True) + ) + assert result.skipped is True + assert result.warmed is False + assert result.failed is False + + def test_non_zero_exit_is_reported_not_raised( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + fake = _FakeAuthCommands(_fresh_timeline(), warm=_completed(3, "warm blew up")) + _install(monkeypatch, fake) + result = warm_backend_credential( + "codex", sandbox=SandboxConfig(enabled=True) + ) + assert result.failed is True + assert result.returncode == 3 + assert "warm blew up" in result.detail + + def test_docker_exception_is_reported_not_raised( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + fake = _FakeAuthCommands([None, None], warm=OSError("no docker here")) + _install(monkeypatch, fake) + result = warm_backend_credential( + "codex", sandbox=SandboxConfig(enabled=True) + ) + assert result.failed is True + assert "no docker here" in result.detail + + def test_detail_is_capped(self, monkeypatch: pytest.MonkeyPatch) -> None: + fake = _FakeAuthCommands(_fresh_timeline(), warm=_completed(1, "x" * 5000)) + _install(monkeypatch, fake) + result = warm_backend_credential( + "codex", sandbox=SandboxConfig(enabled=True) + ) + assert 0 < len(result.detail) <= 400 + + +# --------------------------------------------------------------------------- +# ``warmed`` means verified fresh, not "exited 0" +# --------------------------------------------------------------------------- + + +class TestWarmVerifiesFreshness: + """``codex debug models`` swallows a rejected refresh and exits 0. + + The exit code therefore proves nothing. ``last_refresh`` is read back + from ``auth.json`` (one field, no network) and the warm is reported as + such only when the credential is verifiably inside codex's refresh + interval afterwards -- so no candidate will attempt a refresh. + """ + + def test_advanced_last_refresh_is_warmed( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + stale = _stamp(NOW - CODEX_TOKEN_REFRESH_INTERVAL - timedelta(days=1)) + fake = _FakeAuthCommands([stale, _stamp(NOW - timedelta(seconds=2))]) + _install(monkeypatch, fake) + result = warm_backend_credential( + "codex", sandbox=SandboxConfig(enabled=True) + ) + assert result.warmed is True + assert result.stale is False + assert "refresh performed" in result.detail + + def test_stale_and_unchanged_is_not_warmed( + self, monkeypatch: pytest.MonkeyPatch, capsys: pytest.CaptureFixture[str] + ) -> None: + """A rejected refresh: exit 0, ``last_refresh`` still past the interval.""" + stale = _stamp(NOW - CODEX_TOKEN_REFRESH_INTERVAL - timedelta(days=1)) + fake = _FakeAuthCommands([stale, stale]) + _install(monkeypatch, fake) + result = warm_backend_credential( + "codex", sandbox=SandboxConfig(enabled=True) + ) + assert result.warmed is False + assert result.stale is True + assert result.failed is True + assert result.returncode == 0 + assert "rejected" in result.detail + assert "models_cache.json" in result.detail + + # The loop turns it into a WARNING that names the cause and the race. + monkeypatch.setattr( + "helix.evolution.warm_backend_credential", lambda backend, **_k: result + ) + _warm_generation_credential( + _config("codex", sandboxed=True), gen=2, announce_skip=False + ) + out = " ".join(capsys.readouterr().out.lower().split()) + assert "not verifiably fresh" in out + assert "rejected" in out + assert "same single-use refresh token" in out + + def test_unchanged_but_inside_the_interval_is_fresh( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + """No refresh was due (or a fresh catalog cache short-circuited one): + either way no candidate will refresh, so there is nothing to race.""" + fake = _FakeAuthCommands(_fresh_timeline()) + _install(monkeypatch, fake) + result = warm_backend_credential( + "codex", sandbox=SandboxConfig(enabled=True) + ) + assert result.warmed is True + assert "inside codex's 8-day refresh interval" in result.detail + + def test_unreadable_last_refresh_is_unverified_not_warmed( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + fake = _FakeAuthCommands([None, None]) + _install(monkeypatch, fake) + result = warm_backend_credential( + "codex", sandbox=SandboxConfig(enabled=True) + ) + assert result.warmed is False + assert result.stale is True + assert "unverified" in result.detail + + def test_verdict_function(self) -> None: + old = NOW - CODEX_TOKEN_REFRESH_INTERVAL - timedelta(hours=1) + recent = NOW - timedelta(minutes=5) + assert verify_codex_credential_fresh(old, recent, now=NOW)[0] is True + assert verify_codex_credential_fresh(old, old, now=NOW)[0] is False + assert verify_codex_credential_fresh(recent, recent, now=NOW)[0] is True + assert verify_codex_credential_fresh(None, recent, now=NOW)[0] is True + assert verify_codex_credential_fresh(None, None, now=NOW)[0] is False + assert verify_codex_credential_fresh(old, None, now=NOW)[0] is False + + +# --------------------------------------------------------------------------- +# A stalled warm cannot wedge the loop +# --------------------------------------------------------------------------- + + +class TestWarmIsBounded: + """The warm runs on the main thread before any candidate is dispatched. + + Without a timeout, a token-refresh HTTP call that blackholes blocks the + whole evolution loop indefinitely with nothing but a debug log line, and + the unnamed container keeps running against the shared login volume + after the operator gives up and hits Ctrl-C. + """ + + def test_timeout_always_applies(self) -> None: + assert credential_warm_timeout(SandboxConfig(enabled=True)) == ( + CREDENTIAL_WARM_TIMEOUT_SECONDS + ) + + def test_sandbox_timeout_can_only_tighten_the_cap(self) -> None: + assert credential_warm_timeout( + SandboxConfig(enabled=True, timeout_seconds=30) + ) == 30.0 + assert credential_warm_timeout( + SandboxConfig(enabled=True, timeout_seconds=10_000) + ) == CREDENTIAL_WARM_TIMEOUT_SECONDS + + def test_warm_runs_with_a_timeout_and_a_named_container( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + seen: dict[str, Any] = {} + + def _fake_run(args: list[str], **kwargs: Any) -> subprocess.CompletedProcess[str]: + if "last_refresh" in args[-1]: + return _completed(0, stdout=_stamp(NOW - timedelta(hours=1))) + seen["args"] = args + seen.update(kwargs) + return _completed(0) + + monkeypatch.setattr("helix.sandbox.subprocess.run", _fake_run) + monkeypatch.setattr("helix.sandbox.datetime", _FrozenDatetime) + result = warm_backend_credential( + "codex", sandbox=SandboxConfig(enabled=True, timeout_seconds=45) + ) + + assert result.warmed is True + assert seen["timeout"] == 45.0 + name = seen["args"][seen["args"].index("--name") + 1] + assert name.startswith("helix-warm-codex-") + + def test_timeout_is_reported_not_raised_and_the_container_is_stopped( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + removed: list[list[str]] = [] + + def _hang(args: list[str], **kwargs: Any) -> None: + raise subprocess.TimeoutExpired(cmd=args, timeout=kwargs["timeout"]) + + def _fake_docker(args: list[str], **_k: Any) -> subprocess.CompletedProcess[str]: + removed.append(args) + return _completed(0) + + monkeypatch.setattr("helix.sandbox.subprocess.run", _hang) + monkeypatch.setattr("helix.sandbox._run_docker", _fake_docker) + + result = warm_backend_credential( + "codex", sandbox=SandboxConfig(enabled=True, timeout_seconds=7) + ) + + assert result.failed is True + assert result.timed_out is True + assert result.warmed is False + assert result.skipped is False + assert "7s" in result.detail + # Killing the docker client does not stop the container; the named + # container must be force-removed so it stops touching the volume. + # (The probe before the warm hung too under this fake and was removed + # the same way.) + assert all(r[:3] == ["docker", "rm", "-f"] for r in removed) + assert any(r[3].startswith("helix-warm-codex-") for r in removed) + + def test_timed_out_warm_is_a_warning_in_the_loop( + self, monkeypatch: pytest.MonkeyPatch, capsys: pytest.CaptureFixture[str] + ) -> None: + monkeypatch.setattr( + "helix.evolution.warm_backend_credential", + lambda backend, **_k: CredentialWarmResult( + backend=backend, warmed=False, timed_out=True, detail="slow" + ), + ) + result = _warm_generation_credential( + _config("codex", sandboxed=True), gen=3, announce_skip=False + ) + assert result is not None and result.failed and result.timed_out + printed = " ".join(capsys.readouterr().out.lower().split()) + assert "timed out" in printed + assert "exit none" not in printed + assert "run continues" in printed + + +# --------------------------------------------------------------------------- +# Once per generation, and only where there is a volume to warm +# --------------------------------------------------------------------------- + + +def _config( + backend: str, + *, + sandboxed: bool, + num_parallel_proposals: int = 2, + mutations_per_parent: int = 1, + max_workers: int = 4, +) -> HelixConfig: + return HelixConfig( + objective="Improve the code", + evaluator=EvaluatorConfig(command="pytest -q"), + agent=AgentConfig(backend=backend), # type: ignore[arg-type] + sandbox=SandboxConfig(enabled=sandboxed), + evolution=EvolutionConfig( + num_parallel_proposals=num_parallel_proposals, + mutations_per_parent=mutations_per_parent, + max_workers=max_workers, + ), + ) + + +class TestWarmIsGatedOnConcurrency: + """A single writer cannot lose a refresh race to itself.""" + + @pytest.mark.parametrize( + "kwargs", + [ + dict(num_parallel_proposals=1, mutations_per_parent=1, max_workers=8), + dict(num_parallel_proposals=4, mutations_per_parent=1, max_workers=1), + dict(num_parallel_proposals=1, mutations_per_parent=1, max_workers=1), + ], + ) + def test_single_writer_skips_the_warm_and_says_why( + self, + monkeypatch: pytest.MonkeyPatch, + caplog: pytest.LogCaptureFixture, + kwargs: dict[str, int], + ) -> None: + def _boom(*_a: Any, **_k: Any) -> None: + raise AssertionError("no warm with a single writer") + + monkeypatch.setattr("helix.evolution.warm_backend_credential", _boom) + with caplog.at_level("INFO", logger="helix.evolution"): + result = _warm_generation_credential( + _config("codex", sandboxed=True, **kwargs), gen=1, announce_skip=True + ) + assert result is not None and result.skipped + assert "single writer" in (result.skip_reason or "") + assert "single writer" in caplog.text + + @pytest.mark.parametrize( + "kwargs", + [ + dict(num_parallel_proposals=2, mutations_per_parent=1, max_workers=8), + dict(num_parallel_proposals=1, mutations_per_parent=2, max_workers=2), + ], + ) + def test_two_writers_warm( + self, monkeypatch: pytest.MonkeyPatch, kwargs: dict[str, int] + ) -> None: + calls: list[str] = [] + monkeypatch.setattr( + "helix.evolution.warm_backend_credential", + lambda backend, **_k: ( + calls.append(backend), + CredentialWarmResult(backend=backend, warmed=True, returncode=0), + )[1], + ) + _warm_generation_credential( + _config("codex", sandboxed=True, **kwargs), gen=1, announce_skip=True + ) + assert calls == ["codex"] + + def test_operator_env_is_passed_to_the_warm( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + seen: dict[str, Any] = {} + monkeypatch.setattr( + "helix.evolution.warm_backend_credential", + lambda backend, **k: ( + seen.update(k), + CredentialWarmResult(backend=backend, warmed=True, returncode=0), + )[1], + ) + monkeypatch.setenv("HTTPS_PROXY", "http://proxy:3128") + monkeypatch.delenv("NOT_SET", raising=False) + config = _config("codex", sandboxed=True).model_copy( + update={ + "passthrough_env": ["HTTPS_PROXY", "NOT_SET"], + "env": {"SSL_CERT_FILE": "/ca.pem"}, + } + ) + _warm_generation_credential(config, gen=1, announce_skip=True) + assert seen["env"] == { + "HTTPS_PROXY": "http://proxy:3128", + "SSL_CERT_FILE": "/ca.pem", + } + + +class TestGenerationWarm: + def test_unsandboxed_run_warms_nothing( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + """There is no HELIX-managed login volume to arbitrate without the sandbox.""" + + def _boom(*_a: Any, **_k: Any) -> None: + raise AssertionError("no warm without a sandbox") + + monkeypatch.setattr("helix.evolution.warm_backend_credential", _boom) + assert ( + _warm_generation_credential( + _config("codex", sandboxed=False), gen=1, announce_skip=True + ) + is None + ) + + def test_sandboxed_run_warms_the_configured_backend( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + calls: list[str] = [] + + def _fake(backend: str, **_k: Any) -> CredentialWarmResult: + calls.append(backend) + return CredentialWarmResult(backend=backend, warmed=True, returncode=0) + + monkeypatch.setattr("helix.evolution.warm_backend_credential", _fake) + result = _warm_generation_credential( + _config("codex", sandboxed=True), gen=4, announce_skip=False + ) + assert calls == ["codex"] + assert result is not None and result.warmed + + def test_failed_warm_returns_and_does_not_raise( + self, monkeypatch: pytest.MonkeyPatch, capsys: pytest.CaptureFixture[str] + ) -> None: + monkeypatch.setattr( + "helix.evolution.warm_backend_credential", + lambda backend, **_k: CredentialWarmResult( + backend=backend, warmed=False, returncode=1, detail="boom" + ), + ) + result = _warm_generation_credential( + _config("codex", sandboxed=True), gen=2, announce_skip=False + ) + assert result is not None and result.failed + # The operator is told what protection was lost, not just that a + # command exited non-zero. + printed = capsys.readouterr().out + assert "refresh" in printed.lower() + assert "run continues" in printed.lower() + + def test_container_output_with_rich_markup_does_not_crash_the_run( + self, monkeypatch: pytest.MonkeyPatch, capsys: pytest.CaptureFixture[str] + ) -> None: + """A stderr tail like ``could not read [/home/node/.codex/auth.json]`` + or a truncated ``[/bold]`` raised ``rich.errors.MarkupError`` on the + main thread before any candidate was dispatched.""" + detail = "could not read [/home/node/.codex/auth.json] [/] [/bold] [auth]" + monkeypatch.setattr( + "helix.evolution.warm_backend_credential", + lambda backend, **_k: CredentialWarmResult( + backend=backend, warmed=False, returncode=1, detail=detail + ), + ) + result = _warm_generation_credential( + _config("codex", sandboxed=True), gen=2, announce_skip=False + ) + assert result is not None and result.failed + out = capsys.readouterr().out + assert "[/bold]" in out + assert "[auth]" in out diff --git a/tests/unit/test_credential_warm_loop.py b/tests/unit/test_credential_warm_loop.py new file mode 100644 index 00000000..2bd111d9 --- /dev/null +++ b/tests/unit/test_credential_warm_loop.py @@ -0,0 +1,391 @@ +"""The warm's placement in the loop, and how a credential failure ends up +somewhere an operator will actually read it. + +Two things are asserted end-to-end against the real evolution loop: + +* the warm fires once per generation -- not once per run, because a run + outlives any refresh interval, and not once per candidate, because that is + the race it exists to prevent; and +* when a mutation dies on the shared credential, the run says so in its own + terms and keeps going, instead of filing the slot under "the agent wrote bad + code" and finishing silently. +""" + +from __future__ import annotations + +from typing import Any + +import pytest + +from helix.config import AgentConfig, SandboxConfig +from helix.evolution import run_evolution +from helix.exceptions import CredentialRefreshError +from helix.sandbox import CredentialWarmResult +from tests.unit.test_evolution import ( # type: ignore[import-untyped] + all_mocks, # noqa: F401, F811 — re-exported pytest fixture + make_candidate, + make_config, + make_eval_result, +) + + +def _sandboxed(config: Any) -> Any: + """Sandboxed, on the one backend the code actually warms (codex).""" + return config.model_copy( + update={ + "sandbox": SandboxConfig(enabled=True), + "agent": AgentConfig(backend="codex"), + } + ) + + +@pytest.fixture(autouse=True) +def two_writers(mocker: Any) -> None: + """The warm is skipped for a single writer; that gate has its own tests + (``test_credential_warm.py``). Here the loop shape is what matters, so + pretend every generation has two concurrent writers.""" + mocker.patch("helix.evolution._generation_concurrent_writers", return_value=2) + + +@pytest.fixture() +def warm_calls(mocker: Any) -> list[str]: + calls: list[str] = [] + + def _fake(backend: str, **_kwargs: Any) -> CredentialWarmResult: + calls.append(backend) + return CredentialWarmResult(backend=backend, warmed=True, returncode=0) + + mocker.patch("helix.evolution.warm_backend_credential", side_effect=_fake) + return calls + + +class TestWarmRunsOncePerGeneration: + def test_one_warm_per_generation( + self, mocker, tmp_path, all_mocks, warm_calls # noqa: F811 + ) -> None: + """Three generations, three warms. + + A single warm at startup would leave a long run unprotected the moment + the credential next goes stale mid-flight, so the count must track + generations rather than runs. + """ + seed = make_candidate("g0-s0") + all_mocks["create_seed_worktree"].return_value = seed + all_mocks["mutate"].return_value = None + all_mocks["run_evaluator"].side_effect = ( + lambda candidate, *a, **k: make_eval_result( + candidate.id, {"i1": 0.5, "i2": 0.5} + ) + ) + + config = _sandboxed( + make_config(max_generations=3, perfect_score_threshold=None) + ) + run_evolution(config, tmp_path, tmp_path / ".helix") + + assert warm_calls == ["codex", "codex", "codex"] + + def test_warm_precedes_every_mutation( + self, mocker, tmp_path, all_mocks # noqa: F811 + ) -> None: + """Ordering, not just counting: the credential is fresh before any + candidate could start refreshing it for itself.""" + order: list[str] = [] + mocker.patch( + "helix.evolution.warm_backend_credential", + side_effect=lambda backend, **_k: ( + order.append("warm"), + CredentialWarmResult(backend=backend, warmed=True, returncode=0), + )[1], + ) + + seed = make_candidate("g0-s0") + all_mocks["create_seed_worktree"].return_value = seed + all_mocks["mutate"].side_effect = lambda *a, **k: ( + order.append("mutate"), + None, + )[1] + all_mocks["run_evaluator"].side_effect = ( + lambda candidate, *a, **k: make_eval_result( + candidate.id, {"i1": 0.5, "i2": 0.5} + ) + ) + + config = _sandboxed( + make_config(max_generations=2, perfect_score_threshold=None) + ) + run_evolution(config, tmp_path, tmp_path / ".helix") + + assert order[0] == "warm" + assert order.count("warm") == 2 + for index, event in enumerate(order): + if event == "mutate": + assert "warm" in order[:index] + + def test_unsandboxed_run_warms_nothing( + self, mocker, tmp_path, all_mocks, warm_calls # noqa: F811 + ) -> None: + seed = make_candidate("g0-s0") + all_mocks["create_seed_worktree"].return_value = seed + all_mocks["mutate"].return_value = None + all_mocks["run_evaluator"].side_effect = ( + lambda candidate, *a, **k: make_eval_result( + candidate.id, {"i1": 0.5, "i2": 0.5} + ) + ) + + config = make_config(max_generations=2, perfect_score_threshold=None) + run_evolution(config, tmp_path, tmp_path / ".helix") + + assert warm_calls == [] + + +class TestCredentialFailureIsVisible: + def test_failure_is_named_and_the_run_survives( + self, + mocker, # noqa: F811 + tmp_path, + all_mocks, # noqa: F811 + warm_calls, + capsys: pytest.CaptureFixture[str], + ) -> None: + """A dead credential must not read as a run that merely found nothing. + + The per-slot error names the cause, and the permanent end-of-run + summary repeats it after the live display is gone -- an operator who + looks only at the tail of the run still learns that the login, not the + code, is what failed. + """ + seed = make_candidate("g0-s0") + all_mocks["create_seed_worktree"].return_value = seed + all_mocks["run_evaluator"].side_effect = ( + lambda candidate, *a, **k: make_eval_result( + candidate.id, {"i1": 0.5, "i2": 0.5} + ) + ) + all_mocks["mutate"].side_effect = CredentialRefreshError( + "Codex CLI could not use its stored credential " + "(matched 'your access token could not be refreshed' in stderr)", + suggestion="Re-authenticate with `helix sandbox login codex`.", + ) + + config = _sandboxed( + make_config(max_generations=2, perfect_score_threshold=None) + ) + # The run completes rather than crashing: candidates may still work. + result = run_evolution(config, tmp_path, tmp_path / ".helix") + assert result.best_candidate.id == "g0-s0" + + # Rich wraps the console output; compare on collapsed whitespace. + out = " ".join(capsys.readouterr().out.lower().split()) + assert "credential" in out + assert "not a failure of the candidate's code" in out + assert "helix sandbox login" in out + + def test_merge_credential_failure_is_named_and_the_run_survives( + self, + mocker, # noqa: F811 + tmp_path, + all_mocks, # noqa: F811 + warm_calls, + capsys: pytest.CaptureFixture[str], + ) -> None: + """The merge gate fires before any mutation in a generation, so a + stale login can surface there first. It must get the same treatment + as a mutation: counted, named, and the run continues into mutation + rather than dying with a traceback.""" + seed = make_candidate("g0-s0") + child = make_candidate("g1-s1", generation=1) + all_mocks["create_seed_worktree"].return_value = seed + all_mocks["mutate"].return_value = child + all_mocks["merge"].side_effect = CredentialRefreshError( + "Codex CLI could not use its stored credential " + "(matched 'your access token could not be refreshed' in stderr)", + suggestion="Re-authenticate with `helix sandbox login codex`.", + ) + all_mocks["find_merge_triplet"].return_value = ("g0-s0", "g1-s1", "g0-s0") + + def run_eval(candidate, config, split=None, instances=None, **kwargs): + if candidate.id == "g1-s1": + return make_eval_result("g1-s1", {"i1": 0.9, "i2": 0.5}) + return make_eval_result(candidate.id, {"i1": 0.5, "i2": 0.8}) + + all_mocks["run_evaluator"].side_effect = run_eval + + config = _sandboxed( + make_config( + max_generations=2, + merge_enabled=True, + max_merge_invocations=5, + merge_val_overlap_floor=1, + max_evaluations=10000, + ) + ) + result = run_evolution(config, tmp_path, tmp_path / ".helix") + + all_mocks["merge"].assert_called_once() + # The run went on to mutate after the merge died on the credential. + assert all_mocks["mutate"].call_count >= 1 + assert result.best_candidate is not None + + out = " ".join(capsys.readouterr().out.lower().split()) + assert "merge" in out + assert "credential" in out + assert "not a failure of the merged code" in out + # Exactly one diagnosis at the merge site: the generic "returned no + # output" wording must not follow and contradict it. + assert out.count("not a failure of the merged code") == 1 + assert "returned no output" not in out + # The end-of-run summary labels the merge slot as a merge. + assert "1 merge(s) failed on the shared" in out + assert "mutation(s) failed" not in out + assert "helix sandbox login" in out + + def test_merge_lost_refresh_race_is_worded_as_transient( + self, + mocker, # noqa: F811 + tmp_path, + all_mocks, # noqa: F811 + warm_calls, + capsys: pytest.CaptureFixture[str], + ) -> None: + seed = make_candidate("g0-s0") + child = make_candidate("g1-s1", generation=1) + all_mocks["create_seed_worktree"].return_value = seed + all_mocks["mutate"].return_value = child + exc = CredentialRefreshError( + "Codex CLI lost a refresh race on the shared credential " + "(matched 'because your refresh token was already used' in " + "stderr; failed again on retry)", + suggestion="Run `helix resume`.", + ) + exc.transient = True + all_mocks["merge"].side_effect = exc + all_mocks["find_merge_triplet"].return_value = ("g0-s0", "g1-s1", "g0-s0") + + def run_eval(candidate, config, split=None, instances=None, **kwargs): + if candidate.id == "g1-s1": + return make_eval_result("g1-s1", {"i1": 0.9, "i2": 0.5}) + return make_eval_result(candidate.id, {"i1": 0.5, "i2": 0.8}) + + all_mocks["run_evaluator"].side_effect = run_eval + config = _sandboxed( + make_config( + max_generations=2, + merge_enabled=True, + max_merge_invocations=5, + merge_val_overlap_floor=1, + max_evaluations=10000, + ) + ) + run_evolution(config, tmp_path, tmp_path / ".helix") + + out = " ".join(capsys.readouterr().out.lower().split()) + assert "refreshed by another candidate first" in out + assert "could not be used or refreshed" not in out + assert "helix sandbox login" not in out + assert "helix resume" in out + + def test_lost_refresh_race_does_not_demand_a_relogin( + self, + mocker, # noqa: F811 + tmp_path, + all_mocks, # noqa: F811 + warm_calls, + capsys: pytest.CaptureFixture[str], + ) -> None: + """When every failure was a lost refresh race, the winner has stored a + working credential. The summary must say resume, not re-login.""" + seed = make_candidate("g0-s0") + all_mocks["create_seed_worktree"].return_value = seed + all_mocks["run_evaluator"].side_effect = ( + lambda candidate, *a, **k: make_eval_result( + candidate.id, {"i1": 0.5, "i2": 0.5} + ) + ) + exc = CredentialRefreshError( + "Codex CLI lost a refresh race on the shared credential " + "(matched 'because your refresh token was already used' in " + "stderr; failed again on retry)", + suggestion="Run `helix resume`.", + ) + exc.transient = True + all_mocks["mutate"].side_effect = exc + + config = _sandboxed( + make_config(max_generations=2, perfect_score_threshold=None) + ) + result = run_evolution(config, tmp_path, tmp_path / ".helix") + assert result.best_candidate.id == "g0-s0" + + out = " ".join(capsys.readouterr().out.lower().split()) + assert "credential" in out + assert "refresh race" in out + assert "helix resume" in out + assert "helix sandbox login" not in out + + def test_clean_run_says_nothing_about_credentials( + self, + mocker, # noqa: F811 + tmp_path, + all_mocks, # noqa: F811 + warm_calls, + capsys: pytest.CaptureFixture[str], + ) -> None: + seed = make_candidate("g0-s0") + all_mocks["create_seed_worktree"].return_value = seed + all_mocks["mutate"].return_value = None + all_mocks["run_evaluator"].side_effect = ( + lambda candidate, *a, **k: make_eval_result( + candidate.id, {"i1": 0.5, "i2": 0.5} + ) + ) + + config = _sandboxed( + make_config(max_generations=2, perfect_score_threshold=None) + ) + run_evolution(config, tmp_path, tmp_path / ".helix") + + assert "credential" not in capsys.readouterr().out.lower() + + def test_recovered_refresh_race_reaches_the_summary( + self, + mocker, # noqa: F811 + tmp_path, + all_mocks, # noqa: F811 + warm_calls, + capsys: pytest.CaptureFixture[str], + ) -> None: + """A race the retry recovered from is not a failure, but it is the + only sign the operator gets that candidates are still refreshing the + shared login for themselves.""" + seed = make_candidate("g0-s0") + all_mocks["create_seed_worktree"].return_value = seed + all_mocks["run_evaluator"].side_effect = ( + lambda candidate, *a, **k: make_eval_result( + candidate.id, {"i1": 0.5, "i2": 0.5} + ) + ) + + def _mutate(*args: Any, **kwargs: Any) -> None: + kwargs["on_refresh_race_recovered"]( + f"{kwargs['new_id']} recovered after a lost refresh race on the " + "shared Codex CLI credential" + ) + return None + + all_mocks["mutate"].side_effect = _mutate + config = _sandboxed( + make_config(max_generations=2, perfect_score_threshold=None) + ) + run_evolution(config, tmp_path, tmp_path / ".helix") + + assert all_mocks["mutate"].call_count >= 1 + # ``all_mocks`` stubs ``print_warning``; the summary goes through it + # because a recovered race is a warning, not a failure. + warnings = " ".join( + str(call.args[0]) for call in all_mocks["print_warning"].call_args_list + ).lower() + assert "recovered after a lost refresh race" in warnings + assert "g1-s1" in warnings + assert "failed on the shared" not in capsys.readouterr().out.lower() diff --git a/tests/unit/test_evolution_seedless.py b/tests/unit/test_evolution_seedless.py index 72cdcfd5..6db015c1 100644 --- a/tests/unit/test_evolution_seedless.py +++ b/tests/unit/test_evolution_seedless.py @@ -220,6 +220,29 @@ def test_raises_immediately_when_generate_seed_fails(self, tmp_path, seedless_mo assert exc_info.value is exc + def test_failed_seed_generation_still_charges_its_usage( + self, tmp_path, seedless_mocks, mocker + ): + """A seed invocation that dies on the shared credential spent tokens + first; they are charged before the worktree goes and the error + propagates, mirroring ``merge()`` / ``mutate()``.""" + from helix.exceptions import CredentialRefreshError + + config = make_config(seedless=True) + usage = UsageStats(input_tokens=9, output_tokens=3) + seedless_mocks["generate_seed"].side_effect = CredentialRefreshError( + "login is dead", usage=usage + ) + charge = mocker.patch("helix.evolution.budget_api.charge_llm_usage") + + with pytest.raises(CredentialRefreshError): + run_evolution(config, tmp_path, tmp_path / ".helix") + + [call] = [c for c in charge.call_args_list if c.args[1] is usage] + assert call.kwargs["candidate_id"] == "g0-s0" + assert call.kwargs["source"] == "seed_generation_failed" + seedless_mocks["remove_worktree"].assert_called_once() + def test_generate_seed_called_only_once_on_failure(self, tmp_path, seedless_mocks): """generate_seed must only be called once even on failure (no retry).""" config = make_config(seedless=True) diff --git a/tests/unit/test_merger.py b/tests/unit/test_merger.py index 49156291..a954f5a5 100644 --- a/tests/unit/test_merger.py +++ b/tests/unit/test_merger.py @@ -5,8 +5,11 @@ import random from pathlib import Path +import pytest + from helix.population import Candidate, EvalResult from helix.config import HelixConfig, EvaluatorConfig +from helix.exceptions import CredentialRefreshError from helix.mutator import MutationError from helix.merger import ( build_merge_prompt, @@ -305,6 +308,129 @@ def test_removes_worktree_on_failure(self, mocker): mock_remove.assert_called_once_with(child) + def test_credential_error_removes_worktree_and_reraises(self, mocker): + """A dead login must not leak the merge worktree or become a traceback. + + ``CredentialRefreshError`` is deliberately not a ``MutationError``, so + without its own clause it escaped ``merge()`` with the child worktree + still on disk. Mirror ``mutate()``: clean up, label the operation, + re-raise so evolution.py can count it as a credential failure. + """ + ca = make_candidate("g0-s0") + cb = make_candidate("g0-s1") + config = make_config() + + child = make_candidate("g1-m0") + mocker.patch("helix.merger.clone_candidate", return_value=child) + mocker.patch("helix.merger.get_diff", return_value="some diff") + mocker.patch( + "helix.merger.invoke_claude_code", + side_effect=CredentialRefreshError("login is dead"), + ) + mock_remove = mocker.patch("helix.merger.remove_worktree") + mock_snapshot = mocker.patch("helix.merger.snapshot_candidate") + + with pytest.raises(CredentialRefreshError) as exc: + merge(ca, cb, "g1-m0", config, Path("/tmp")) + + assert exc.value.operation == "merge g1-m0 (g0-s0 + g0-s1)" + mock_remove.assert_called_once_with(child) + mock_snapshot.assert_not_called() + + def test_credential_error_hands_spent_usage_to_the_sink(self, mocker): + """Tokens spent before the credential gave out still reach the budget. + + ``invoke_claude_code`` attaches the salvaged usage to every + ``HelixError`` it raises; the credential path must forward it through + ``record_usage`` exactly as the ``MutationError`` and ``RateLimitError`` + paths do, or a credential failure becomes a free merge. + """ + from helix.display import UsageStats + + ca = make_candidate("g0-s0") + cb = make_candidate("g0-s1") + config = make_config() + + child = make_candidate("g1-m0") + mocker.patch("helix.merger.clone_candidate", return_value=child) + mocker.patch("helix.merger.get_diff", return_value="some diff") + spent_before_failure = UsageStats(input_tokens=7, output_tokens=3) + mocker.patch( + "helix.merger.invoke_claude_code", + side_effect=CredentialRefreshError( + "login is dead", usage=spent_before_failure + ), + ) + mocker.patch("helix.merger.remove_worktree") + mocker.patch("helix.merger.snapshot_candidate") + + spent: list[UsageStats] = [] + with pytest.raises(CredentialRefreshError): + merge( + ca, cb, "g1-m0", config, Path("/tmp"), record_usage=spent.append + ) + + assert spent == [spent_before_failure] + + def test_lost_refresh_race_is_retried_once_from_a_fresh_worktree( + self, mocker, tmp_path: Path + ): + """``merge()`` gets the same one-shot retry as ``mutate()``: a + transient credential failure re-clones the merge worktree and + invokes again; the first attempt's spend rides along.""" + from helix.display import UsageStats + + ca = make_candidate("g0-s0") + cb = make_candidate("g0-s1") + config = make_config() + clones: list = [] + + def _clone(parent, new_id, base_dir): + path = tmp_path / f"clone{len(clones) + 1}" + path.mkdir() + child = make_candidate(new_id) + child.worktree_path = str(path) + clones.append(child) + return child + + mocker.patch("helix.merger.clone_candidate", side_effect=_clone) + mocker.patch("helix.merger.get_diff", return_value="some diff") + lost = CredentialRefreshError( + "lost a refresh race", usage=UsageStats(input_tokens=5, output_tokens=2) + ) + lost.transient = True + invoke = mocker.patch( + "helix.merger.invoke_claude_code", + side_effect=[lost, ({}, UsageStats(input_tokens=12, output_tokens=8))], + ) + removed: list = [] + mocker.patch("helix.merger.remove_worktree", side_effect=removed.append) + mocker.patch("helix.merger.snapshot_candidate") + recovered: list[str] = [] + spent: list[UsageStats] = [] + + result = merge( + ca, + cb, + "g1-m0", + config, + Path("/tmp"), + record_usage=spent.append, + on_refresh_race_recovered=recovered.append, + ) + + assert invoke.call_count == 2 + assert len(clones) == 2 + assert result is clones[1] + assert result.operation == "merge" + assert result.parent_ids == ["g0-s0", "g0-s1"] + # The retry ran in the fresh clone, and the first one was removed. + assert invoke.call_args_list[1].args[0] == clones[1].worktree_path + assert invoke.call_args_list[1].kwargs["retried"] is True + assert removed == [clones[0]] + assert spent[0].input_tokens == 5 + 12 and spent[0].output_tokens == 2 + 8 + assert recovered and "g1-m0" in recovered[0] + def test_snapshot_not_called_by_merge_on_success(self, mocker): """merge() must NOT call snapshot_candidate — the caller owns that step. diff --git a/tests/unit/test_mutator.py b/tests/unit/test_mutator.py index 2b3baee4..fe9dcecd 100644 --- a/tests/unit/test_mutator.py +++ b/tests/unit/test_mutator.py @@ -845,6 +845,34 @@ def test_codex_effort_config_value_is_json_quoted(self, mocker): args_list = mock_run.call_args[0][0] assert 'model_reasoning_effort="high\\"quoted"' in args_list + def test_codex_cli_args_pin_memories_off(self, mocker): + mock_run = mocker.patch("helix.mutator.subprocess.run") + mock_run.return_value = MagicMock(stdout="", stderr="", returncode=0) + + invoke_claude_code("/tmp/wt", "the prompt", AgentConfig(backend="codex")) + + args_list = mock_run.call_args[0][0] + idx = args_list.index("features.memories=false") + assert args_list[idx - 1] == "-c" + + def test_claude_env_disables_auto_memory(self, mocker): + mock_run = mocker.patch("helix.mutator.subprocess.run") + mock_run.return_value = MagicMock(stdout="{}", stderr="", returncode=0) + + invoke_claude_code("/tmp/wt", "the prompt", AgentConfig(backend="claude")) + + env = mock_run.call_args[1]["env"] + assert env["CLAUDE_CODE_DISABLE_AUTO_MEMORY"] == "1" + + def test_fresh_session_env_not_injected_for_other_backends(self, mocker): + mock_run = mocker.patch("helix.mutator.subprocess.run") + mock_run.return_value = MagicMock(stdout="", stderr="", returncode=0) + + invoke_claude_code("/tmp/wt", "the prompt", AgentConfig(backend="codex")) + + env = mock_run.call_args[1]["env"] + assert "CLAUDE_CODE_DISABLE_AUTO_MEMORY" not in env + def test_cursor_cli_args_use_stream_json(self, mocker): mock_run = mocker.patch("helix.mutator.subprocess.run") mock_run.return_value = MagicMock( @@ -1846,18 +1874,22 @@ def test_claude_tool_counts_patched_from_transcript( # --------------------------------------------------------------------------- -# Tests: OpenCode per-candidate SQLite isolation (XDG_DATA_HOME) +# Tests: OpenCode per-candidate SQLite isolation (OPENCODE_DB) # --------------------------------------------------------------------------- class TestOpenCodeSubprocessIsolation: - """Verify that concurrent opencode subprocesses receive isolated XDG_DATA_HOME + """Verify that concurrent opencode subprocesses receive isolated OPENCODE_DB values so each worker opens its own SQLite database and the shared-database 'PRAGMA journal_mode = WAL' contention observed in PR #34 cannot recur. + + OPENCODE_DB rather than XDG_DATA_HOME: the latter relocates opencode's + ``auth.json`` along with the database, which makes an existing login + invisible to the CLI. """ def test_opencode_subprocess_isolation_env_set(self, tmp_path: Path, mocker): - """invoke_claude_code sets XDG_DATA_HOME for opencode backend.""" + """invoke_claude_code sets OPENCODE_DB for opencode backend.""" mock_run = mocker.patch("helix.mutator.subprocess.run") mock_run.return_value = MagicMock( stdout='{"type":"result","sessionID":"ses_abc"}\n', @@ -1871,20 +1903,55 @@ def test_opencode_subprocess_isolation_env_set(self, tmp_path: Path, mocker): call_kwargs = mock_run.call_args[1] env = call_kwargs["env"] - assert "XDG_DATA_HOME" in env, ( - "XDG_DATA_HOME must be set for opencode to isolate its SQLite database " + assert "OPENCODE_DB" in env, ( + "OPENCODE_DB must be set for opencode to isolate its SQLite database " "from other concurrent opencode workers" ) # Must point inside the candidate worktree so cleanup is automatic - xdg = env["XDG_DATA_HOME"] - assert xdg.startswith(str(tmp_path)), ( - f"XDG_DATA_HOME={xdg!r} should be under the candidate worktree {tmp_path}" + db_path = env["OPENCODE_DB"] + assert db_path.startswith(str(tmp_path)), ( + f"OPENCODE_DB={db_path!r} should be under the candidate worktree {tmp_path}" + ) + # The credential knob must stay untouched: relocating XDG_DATA_HOME + # would move auth.json with the database and hide an existing login. + assert "XDG_DATA_HOME" not in env, ( + "XDG_DATA_HOME must not be set for opencode; it moves auth.json too" + ) + + def test_sandboxed_opencode_gets_a_per_candidate_database_too( + self, tmp_path: Path, mocker + ): + """Every sandboxed container shares one ``/home/node`` (the + ``helix-auth-opencode`` volume with ``HOME`` forced), so without the + knob all concurrent candidates open the same ``opencode.db``. The + database goes under the per-candidate workspace copy instead.""" + mock_run = mocker.patch("helix.mutator.run_sandboxed_command") + mock_run.return_value = MagicMock( + stdout='{"type":"result","sessionID":"ses_abc"}\n', + stderr="", + returncode=0, ) + invoke_claude_code( + str(tmp_path), + "fix the bug", + AgentConfig(backend="opencode"), + sandbox=SandboxConfig(enabled=True, image="img:latest"), + ) + + env = mock_run.call_args[1]["env"] + assert env["OPENCODE_DB"] == ( + "/workspace/.helix_opencode_state/opencode/opencode.db" + ) + assert "XDG_DATA_HOME" not in env + # The host worktree is untouched: the directory is created in the + # workspace copy by the sandbox, not here. + assert not (tmp_path / ".helix_opencode_state").exists() + def test_opencode_subprocess_isolation_unique_per_candidate( self, tmp_path: Path, mocker ): - """Two different candidates get different XDG_DATA_HOME values.""" + """Two different candidates get different OPENCODE_DB values.""" mock_run = mocker.patch("helix.mutator.subprocess.run") mock_run.return_value = MagicMock( stdout='{"type":"result","sessionID":"ses_abc"}\n', @@ -1898,18 +1965,18 @@ def test_opencode_subprocess_isolation_unique_per_candidate( wt_b.mkdir() invoke_claude_code(str(wt_a), "prompt", AgentConfig(backend="opencode")) - env_a = mock_run.call_args[1]["env"]["XDG_DATA_HOME"] + env_a = mock_run.call_args[1]["env"]["OPENCODE_DB"] invoke_claude_code(str(wt_b), "prompt", AgentConfig(backend="opencode")) - env_b = mock_run.call_args[1]["env"]["XDG_DATA_HOME"] + env_b = mock_run.call_args[1]["env"]["OPENCODE_DB"] assert env_a != env_b, ( - "Each candidate worktree must produce a distinct XDG_DATA_HOME so their " + "Each candidate worktree must produce a distinct OPENCODE_DB so their " "opencode SQLite databases don't collide" ) def test_opencode_subprocess_inherits_other_env(self, tmp_path: Path, mocker): - """Non-isolation env vars (PATH, HOME) are still present after XDG injection.""" + """Non-isolation env vars (PATH, HOME) survive the OPENCODE_DB injection.""" mock_run = mocker.patch("helix.mutator.subprocess.run") mock_run.return_value = MagicMock( stdout='{"type":"result","sessionID":"ses_abc"}\n', @@ -1929,13 +1996,13 @@ def test_opencode_subprocess_inherits_other_env(self, tmp_path: Path, mocker): assert "PATH" in env, "PATH must survive the env scrub for opencode" if "HOME" in os.environ: assert "HOME" in env, "HOME must survive the env scrub for opencode" - # And XDG_DATA_HOME is the *only* new opencode-specific addition - assert "XDG_DATA_HOME" in env + # And OPENCODE_DB is the *only* new opencode-specific addition + assert "OPENCODE_DB" in env def test_opencode_isolation_not_applied_to_other_backends( self, tmp_path: Path, mocker ): - """XDG_DATA_HOME must NOT be injected for agy/claude/codex/cursor.""" + """OPENCODE_DB must NOT be injected for agy/claude/codex/cursor.""" mock_run = mocker.patch("helix.mutator.subprocess.run") mock_run.return_value = MagicMock(stdout="{}", stderr="", returncode=0) @@ -1944,8 +2011,8 @@ def test_opencode_isolation_not_applied_to_other_backends( str(tmp_path), "prompt", AgentConfig(backend=backend) ) env = mock_run.call_args[1]["env"] - assert "XDG_DATA_HOME" not in env, ( - f"XDG_DATA_HOME must not be injected for {backend} backend" + assert "OPENCODE_DB" not in env, ( + f"OPENCODE_DB must not be injected for {backend} backend" ) def test_opencode_isolation_dir_gitignored(self, tmp_path: Path, mocker): @@ -2070,6 +2137,60 @@ def test_nothing_is_reported_when_the_error_carries_no_usage( assert result is None assert spent == [] + def test_usage_is_reported_when_the_credential_fails( + self, tmp_path: Path, mocker + ): + """A credential failure is not free: the tokens spent before the + login gave out reach the sink like the sibling error paths.""" + from helix.exceptions import CredentialRefreshError + + usage = UsageStats(input_tokens=11, output_tokens=4) + parent = make_candidate("g0-s0") + er = make_eval_result() + config = make_config() + child_path = tmp_path / "g1-s0" + child_path.mkdir() + child = make_candidate("g1-s0", str(child_path)) + mocker.patch("helix.mutator.clone_candidate", return_value=child) + mocker.patch( + "helix.mutator.invoke_claude_code", + side_effect=CredentialRefreshError("login is dead", usage=usage), + ) + mock_remove = mocker.patch("helix.mutator.remove_worktree") + mocker.patch("helix.mutator.snapshot_candidate") + + spent: list[UsageStats] = [] + with pytest.raises(CredentialRefreshError): + mutate( + parent, er, "g1-s0", config, Path("/tmp"), record_usage=spent.append + ) + + assert spent == [usage] + mock_remove.assert_called_once_with(child) + + def test_worktree_is_removed_on_a_non_helix_exception( + self, tmp_path: Path, mocker + ): + import subprocess as _sp + + parent = make_candidate("g0-s0") + er = make_eval_result() + config = make_config() + child_path = tmp_path / "g1-s0" + child_path.mkdir() + child = make_candidate("g1-s0", str(child_path)) + mocker.patch("helix.mutator.clone_candidate", return_value=child) + mocker.patch( + "helix.mutator.invoke_claude_code", + side_effect=_sp.TimeoutExpired(cmd=["codex"], timeout=1), + ) + mock_remove = mocker.patch("helix.mutator.remove_worktree") + + with pytest.raises(_sp.TimeoutExpired): + mutate(parent, er, "g1-s0", config, Path("/tmp")) + + mock_remove.assert_called_once_with(child) + class TestSalvageBackendUsage: """The lenient recovery used before the strict parse can fail.""" diff --git a/tests/unit/test_sandbox.py b/tests/unit/test_sandbox.py index 640a2148..9f6262f9 100644 --- a/tests/unit/test_sandbox.py +++ b/tests/unit/test_sandbox.py @@ -907,7 +907,7 @@ def test_sandbox_auth_status_command_uses_backend_command(): ) assert "helix-auth-claude:/home/node:rw" in args - assert args[-3:-1] == ["sh", "-lc"] + assert args[-3:-1] == ["sh", "-c"] script = args[-1] assert script.startswith("set -eu; ") assert "claude auth status --text" in script @@ -965,7 +965,7 @@ def test_sandbox_auth_agy_status_uses_credential_file_probe(): ) assert "helix-auth-agy:/home/node:rw" in args - assert args[-3:-1] == ["sh", "-lc"] + assert args[-3:-1] == ["sh", "-c"] script = args[-1] assert ( 'test -s "${HOME:-/home/node}/.gemini/antigravity-cli/antigravity-oauth-token"' @@ -982,7 +982,7 @@ def test_sandbox_auth_agy_logout_only_removes_its_own_state_directory(): action="logout", ) - assert args[-3:-1] == ["sh", "-lc"] + assert args[-3:-1] == ["sh", "-c"] script = args[-1] assert '"${HOME:-/home/node}/.gemini/antigravity-cli"' in script assert script.count(".gemini") == 1 @@ -1117,3 +1117,78 @@ def test_captured_output_is_left_verbatim(self, mocker) -> None: result = sandbox_module._run_docker(args, check=False) assert result.stderr == traceback_text + + +def test_sandboxed_opencode_workspace_carries_its_own_state_dir( + tmp_path: Path, mocker +): + """The database directory named by ``OPENCODE_DB`` must exist inside the + workspace copy before the container starts (SQLite does not create + parent directories), and must not show up as an untracked file to the + agent.""" + source = tmp_path / "candidate" + source.mkdir() + (source / "main.py").write_text("print('hi')\n") + + seen: dict[str, object] = {} + + def fake_run(args, **kwargs): + if args[:2] == ["docker", "run"] and "--user" in args and "-e" in args: + mount = next(a for a in args if a.endswith(":/workspace:rw")) + workspace = Path(mount.split(":")[0]) + seen["state_dir_exists"] = ( + workspace / ".helix_opencode_state" / "opencode" + ).is_dir() + exclude = workspace / ".git" / "info" / "exclude" + seen["excluded"] = exclude.is_file() and ( + ".helix_opencode_state/" in exclude.read_text() + ) + seen["env"] = [a for a in args if a.startswith("OPENCODE_DB=")] + return subprocess.CompletedProcess(args, 0, stdout="", stderr="") + + mocker.patch("helix.sandbox.subprocess.run", side_effect=fake_run) + mocker.patch("helix.sandbox._host_owner", return_value="1000:1000") + + run_sandboxed_command( + ["opencode", "run", "prompt"], + cwd=source, + env={"OPENCODE_DB": "/workspace/.helix_opencode_state/opencode/opencode.db"}, + sandbox=SandboxConfig(enabled=True, image="helix-test:latest"), + scope="agent", + sync_back=True, + agent_backend="opencode", + ) + + assert seen["state_dir_exists"] is True + assert seen["excluded"] is True + assert seen["env"] == [ + "OPENCODE_DB=/workspace/.helix_opencode_state/opencode/opencode.db" + ] + # Nothing came back to the candidate's worktree. + assert not (source / ".helix_opencode_state").exists() + + +def test_other_backends_get_no_opencode_state_dir(tmp_path: Path, mocker): + source = tmp_path / "candidate" + source.mkdir() + seen: dict[str, object] = {} + + def fake_run(args, **kwargs): + if args[:2] == ["docker", "run"] and "--user" in args and "-e" in args: + mount = next(a for a in args if a.endswith(":/workspace:rw")) + workspace = Path(mount.split(":")[0]) + seen["state_dir_exists"] = (workspace / ".helix_opencode_state").exists() + return subprocess.CompletedProcess(args, 0, stdout="", stderr="") + + mocker.patch("helix.sandbox.subprocess.run", side_effect=fake_run) + mocker.patch("helix.sandbox._host_owner", return_value="1000:1000") + run_sandboxed_command( + ["codex", "exec", "prompt"], + cwd=source, + env={}, + sandbox=SandboxConfig(enabled=True, image="helix-test:latest"), + scope="agent", + sync_back=False, + agent_backend="codex", + ) + assert seen["state_dir_exists"] is False