diff --git a/docs/howto/diagnose-a-red-canary-unit-test-gate.md b/docs/howto/diagnose-a-red-canary-unit-test-gate.md new file mode 100644 index 000000000..9726ff44b --- /dev/null +++ b/docs/howto/diagnose-a-red-canary-unit-test-gate.md @@ -0,0 +1,131 @@ +--- +title: How to diagnose a red-canary unit-test gate +description: Operator runbook for the case where every self-deploy is refused on failing_gate="unit-test" while `cargo test --lib` passes clean standalone — recognise the environment-induced false red, read the named failing test from the enriched failing_detail, confirm the gate now runs hermetically in an isolated state root, and verify the self-deploy loop advances past the stuck SHA. +last_updated: 2026-07-24 +review_schedule: as-needed +owner: simard +doc_type: howto +status: active +related: + - ../reference/hermetic-unit-test-gate.md + - ../reference/canary-gate-convergence.md + - ../reference/overseer-deploy-canary-diagnostics.md + - ../reference/state-root-resolution.md + - ./converge-a-stuck-red-canary-self-deploy.md + - ../safe-self-update.md +--- + +# How to diagnose a red-canary unit-test gate + +> **Status: active.** This describes shipped behaviour: the hermetic +> `unit-test` gate and the named `failing_detail`. For the full design and API, +> see [Hermetic unit-test canary gate](../reference/hermetic-unit-test-gate.md). + +Use this runbook when the Overseer refuses **every** self-deploy on the +`unit-test` gate — `DeployDrift` climbs, `running_commit` is pinned — yet the +test suite passes when you run it by hand. + +## 1. Confirm the symptom + +The #4558 signature is an identical `unit-test` refusal on every tick, with a +fast exit-101 abort: + +```bash +journalctl --user -u simard -o cat \ + | grep -E 'overseer::deploy' | tail -n 40 +``` + +You are looking for a repeating refusal against the **same** `target_commit`: + +``` +WARN overseer::deploy: self-deploy refused by deploy gate + target_commit=7d0964f running_commit=7d0964f + failing_gate="unit-test" + failing_detail="tests failed (exit exit status: 101): " +``` + +## 2. Rule out a genuine regression + +Run the same suite the gate runs, **standalone**, from the repo root: + +```bash +cargo test --lib +``` + +- **If it fails** — this is a real regression. Read the named test from + `failing_detail` (see step 3) and fix the source. Stop here. +- **If it passes clean** (e.g. `9279 passed; 0 failed`) but the gate reddens — + this is the **environment-induced false red** the hermetic gate fixes. The + in-process lib-test was aborting because it bound the live daemon's socket or + locked its shared WAL / cognitive-store under the daemon's state root. With the + hermetic gate shipped, this no longer happens; if you still see it, continue. + +## 3. Read the named failing test + +The gate now captures **both** stdout and stderr and extracts the failing test +name into `failing_detail` (clamped to 4096B at the gate, 512B downstream). You +should see a real marker block, **not** a truncated `Drop t…` spinner fragment: + +``` +failing_detail="tests failed (exit status: 101): failures: + self_relaunch::gates::tests::extract_failure_detail_names_test +panicked at src/self_relaunch/gates.rs:412: assertion failed …" +``` + +If `failing_detail` still shows only `tests failed (exit …)` with no test name, +the running binary predates #4558 — deploy a build that includes the hermetic +gate. + +## 4. Confirm the gate runs hermetically + +The `unit-test` gate spawns `cargo test` with the four isolation keys overridden +to a fresh per-run temp dir and `current_dir` set to the manifest dir: + +`SIMARD_STATE_ROOT`, `SIMARD_HOME`, `HOME`, `TMPDIR` → a private +`tempfile::TempDir`. + +`CARGO_HOME` / `RUSTUP_HOME` are **pinned** to absolute paths resolved from the +real (pre-override) `HOME` *before* `HOME` is redirected, so cargo/rustup still +find the toolchain — without this, the `HOME` override would itself cause a fresh +exit-101 abort of the same class. + +That temp state root is empty, so the in-process suite resolves its own WAL / +cognitive-store / socket path +([state-root resolution](../reference/state-root-resolution.md)) and cannot +collide with the live daemon. To verify locally that a running daemon no longer +red-canaries a green tree, run the gate's fixture test: + +```bash +# Green fixture passes even with a simulated live daemon holding the shared root; +# red fixture's failing_detail names the failing test. +cargo test -p simard self_relaunch::gates +cargo test --test unit_test_gate_fixture # integration fixture +``` + +If the temp-dir/env setup ever fails, the gate **fails closed** — it returns a +`unit-test gate could not create an isolated state root: …` failure rather than +silently falling back to the live daemon's state root. That is expected +fail-closed behaviour, not the #4558 bug; fix the temp/disk condition and retry. + +## 5. Verify convergence + +Once the gate renders a true green verdict, the guarded deploy gate stops +returning `RedCanary`, the swap proceeds, and drift returns to 0: + +```bash +journalctl --user -u simard -o cat | grep -E 'overseer::(tick|deploy)' | tail -n 20 +``` + +You should see the deploy **succeed** and the next drift observation report +`DeployDrift == 0` — the loop advances past the previously stuck target SHA +instead of re-queuing the identical `unit-test` refusal. + +## See also + +- [Hermetic unit-test canary gate](../reference/hermetic-unit-test-gate.md) — + the design, the four isolation keys, `extract_failure_detail`, and the + fail-closed / truncation contracts. +- [How to converge a stuck red-canary self-deploy](./converge-a-stuck-red-canary-self-deploy.md) — + the sibling runbook for a red canary on any gate. +- [Overseer deploy red-canary diagnostics](../reference/overseer-deploy-canary-diagnostics.md) — + the `failing_gate` / `failing_detail` surface this runbook reads. diff --git a/docs/index.md b/docs/index.md index ae8ef8cc4..2c2fb9e86 100644 --- a/docs/index.md +++ b/docs/index.md @@ -160,6 +160,8 @@ 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). +- [Hermetic unit-test canary gate reference](./reference/hermetic-unit-test-gate.md) - The #4558 repair that stops the live daemon from red-canarying a green tree: `run_unit_test_gate` runs `cargo test` in a fail-closed, per-run isolated state root (`SIMARD_STATE_ROOT`/`SIMARD_HOME`/`HOME`/`TMPDIR` → a fresh `tempfile::TempDir` + manifest `current_dir`) so an in-process lib-test cannot bind the daemon's socket or lock its shared WAL/cognitive-store, plus `extract_failure_detail` capturing BOTH stdout and stderr and lifting the failing test name into `failing_detail` (4096B gate clamp, 512B downstream) instead of a truncated `Drop t…` fragment. See the [diagnosis runbook](./howto/diagnose-a-red-canary-unit-test-gate.md). +- [How to diagnose a red-canary unit-test gate](./howto/diagnose-a-red-canary-unit-test-gate.md) - Operator runbook for a self-deploy refused on `failing_gate="unit-test"` while `cargo test --lib` passes standalone: recognise the environment-induced false red, read the named failing test from the enriched `failing_detail`, confirm the gate now runs hermetically in an isolated state root, and verify the self-deploy loop advances past the stuck SHA (#4558). - [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/hermetic-unit-test-gate.md b/docs/reference/hermetic-unit-test-gate.md new file mode 100644 index 000000000..e7e3779f9 --- /dev/null +++ b/docs/reference/hermetic-unit-test-gate.md @@ -0,0 +1,357 @@ +--- +title: Hermetic unit-test canary gate +description: Reference for the #4558 root-cause repair that stops the live daemon from red-canarying a green tree. The unit-test gate runs `cargo test` in an isolated per-run temp state root (SIMARD_STATE_ROOT/SIMARD_HOME/HOME/TMPDIR + manifest current_dir) with CARGO_HOME/RUSTUP_HOME pinned from the real HOME so an in-process lib-test cannot bind the daemon's socket or lock its WAL/cognitive-store and the toolchain still resolves, and captures BOTH stdout and stderr, extracting the failing test name into failing_detail via extract_failure_detail with a 4096-byte structured-marker clamp. +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 + - ./state-root-resolution.md + - ./string-truncation-helpers.md + - ./overseer-tick-self-healing.md + - ../howto/diagnose-a-red-canary-unit-test-gate.md + - ../safe-self-update.md + - ../../src/self_relaunch/gates.rs + - ../../src/self_relaunch/types.rs +--- + +# Hermetic unit-test canary gate + +> **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` in a **hermetic, per-run isolation directory** and +> extracts the failing test name into `failing_detail`. Both changes live at the +> smallest responsible site +> ([`src/self_relaunch/gates.rs`](https://github.com/rysweet/Simard/blob/main/src/self_relaunch/gates.rs)); +> `canary.rs`, `types.rs`, and `overseer/deploy.rs` are unchanged (deploy still +> wires results/refusal only). The change is **additive and non-breaking**: +> `verify_canary`, `all_gates_passed`, `default_gates`, `RelaunchConfig`, and the +> four-gate no-short-circuit sequence keep their signatures and semantics. + +## Why this exists + +The [canary gate convergence repair](./canary-gate-convergence.md) (#4440) +scrubbed the gate environment and supplied deploy-shape signals so a healthy +candidate could render a true verdict. It did **not** isolate the one gate that +runs a full in-process test binary. That left a systemic crash-loop: + +- **Symptom (#4558).** Every self-deploy for 6+ hours was refused on the + `unit-test` gate — 20–21 consecutive red-canary refusals — pinning + `running_commit` while `DeployDrift` grew from 7 to 8 commits behind and + stranding all merged work. The journal signature was: + + ``` + WARN overseer::deploy: self-deploy refused by deploy gate + failing_gate="unit-test" + failing_detail="tests failed (exit exit status: 101)" + ``` + +- **Root cause.** `run_unit_test_gate` invoked + `cargo test --manifest-path /Cargo.toml --target-dir ` + as a **child of the live daemon**. The lib-test binary aborted with exit + status `101` in under 1.3s (`Running unittests src/lib.rs …` then exit) — even + though `cargo test --lib` on the *identical* source passed clean standalone + (9279 passed / 0 failed / 133s). The tree was green; the **deploy-gate + environment** reddened it: statics in the in-process suite bound the daemon's + fixed socket / port or took a lock on the shared cognitive-store / WAL under + the daemon's state root, and aborted immediately. + +- **Second failure — undiagnosable.** The gate captured only `stderr` and + truncated it to 200 bytes. On a red tree the 200-byte head landed on a + progress-spinner fragment (`Drop t…`), hiding *which* test failed. The + operator saw `tests failed (exit …)` with no test name. + +This feature fixes both: the gate is made **hermetic** (a running daemon can no +longer red-canary a passing suite) and **diagnosable** (a real failure names the +failing test). + +This does **not** weaken, skip, or disable the gate. A genuinely failing test +still reddens; only the environment-induced false red is removed. + +## What changed + +1. **Hermetic execution.** After `scrub_gate_env`, `run_unit_test_gate` + overrides four isolation keys — `SIMARD_STATE_ROOT`, `SIMARD_HOME`, `HOME`, + `TMPDIR` — to a fresh per-run temp directory, and sets `current_dir` to the + manifest dir. The in-process test suite therefore resolves an **empty, + private** state root ([`default_state_root`](./state-root-resolution.md): + `SIMARD_STATE_ROOT` else `$HOME/.simard`) and cannot open the live daemon's + WAL / cognitive-store or bind its socket. +2. **Toolchain pin.** Because the `HOME` override would otherwise strand + `cargo`/`rustup` (they fall back to `$HOME/.cargo` / `$HOME/.rustup` when + `CARGO_HOME` / `RUSTUP_HOME` are unset), a new private helper + `resolve_toolchain_home()` computes absolute `CARGO_HOME` / `RUSTUP_HOME` + from the **real, pre-override** `HOME` (preferring ambient values when set) + and pins them on the child **before** `HOME` is redirected. See the + load-bearing invariant under [Isolation keys](#isolation-keys). +3. **Fail-closed isolation.** The temp dir is created via a new private helper + `unit_test_isolation_dir() -> Result`. If temp-dir setup fails the + gate returns a **failing** `GateResult` — never a silent non-hermetic + fallback to the daemon's live state root. +4. **Diagnosable failure.** On non-zero exit the gate now captures **both** + stdout and stderr and feeds them to a new pure helper + `extract_failure_detail(stdout, stderr)`, which pulls the first structured + marker block (`failures:` / `panicked at …` / `test … FAILED`, test-name + first) and UTF-8-safely clamps it to **4096 bytes** — raised from the old + 200-byte stderr-only head — so the failing test **name** survives into + `failing_detail`. + +## Data model + +No new or changed public types. `RelaunchConfig` +([`src/self_relaunch/types.rs`](https://github.com/rysweet/Simard/blob/main/src/self_relaunch/types.rs)) +is byte-for-byte unchanged, so the `Default`/byte-layout compatibility guard test +stays green. The isolation directory is an ephemeral RAII +[`tempfile::TempDir`](https://docs.rs/tempfile) (already a full `[dependencies]` +entry, `=3.27.0`) that is cleaned up when the gate returns. + +### Isolation keys + +The four environment names overridden **per child** (never in the parent +process — no `set_var`, so no `ENV_LOCK`/`unsafe` hazard): + +| Key | Overridden to | Why | +| --- | --- | --- | +| `SIMARD_STATE_ROOT` | fresh temp dir | governs WAL / cognitive-store / socket paths ([state-root resolution](./state-root-resolution.md)) | +| `SIMARD_HOME` | fresh temp dir | deploy-shape home; kept off the live tree | +| `HOME` | fresh temp dir | fallback state root (`$HOME/.simard`) when `SIMARD_STATE_ROOT` unset in a sub-suite | +| `TMPDIR` | fresh temp dir | scratch / socket-dir isolation | + +Plus two **pinned** (not temp-redirected) toolchain keys, set on the child +**before** the `HOME` override so `cargo`/`rustup` can still find the toolchain: + +| Key | Pinned to | Why | +| --- | --- | --- | +| `CARGO_HOME` | absolute path from the real pre-override `HOME` (or ambient value) | prevents cargo hunting the toolchain under the empty temp `$HOME/.cargo` | +| `RUSTUP_HOME` | absolute path from the real pre-override `HOME` (or ambient value) | same, for the rustup toolchain root | + +These override the same-named values that `scrub_gate_env` re-injects from the +[`canary_gate_env_allowlist`](./canary-gate-convergence.md) (`SIMARD_HOME`, +`SIMARD_STATE_ROOT`). Ordering is load-bearing: the override is applied **after** +`scrub_gate_env`, via per-child `Command::env(...)`, so the isolated temp path +wins. + +> **Load-bearing invariant — toolchain resolution must survive the `HOME` +> override.** `cargo`/`rustup` resolve their toolchain from `CARGO_HOME` / +> `RUSTUP_HOME` and, *only when those are unset*, fall back to `$HOME/.cargo` / +> `$HOME/.rustup`. `scrub_gate_env`'s base floor re-injects +> `CARGO_HOME` / `RUSTUP_HOME` / `RUSTUP_TOOLCHAIN` **only if they are present in +> the daemon's ambient env** (`if let Ok(val) = env::var(..)`). Under a clean +> systemd unit they are frequently *absent* — the daemon relies on the +> `$HOME/.cargo` default. If `HOME` is then overridden to an **empty** temp dir +> while `CARGO_HOME` / `RUSTUP_HOME` are unset, `cargo test` looks for the +> toolchain under the empty temp `$HOME/.cargo` and aborts — a **new +> self-inflicted false red of the exact #4558 class**. Therefore the isolation +> step MUST pin `CARGO_HOME` and `RUSTUP_HOME` to absolute paths **resolved from +> the real pre-override `HOME`** (or the ambient values) and set them explicitly +> on the child *before* overriding `HOME` — never leave them to the +> pass-through-if-present floor. `RUSTUP_TOOLCHAIN` is passed through unchanged. + +## Behavior + +### `run_unit_test_gate` (hermetic) + +```rust +fn run_unit_test_gate(config: &RelaunchConfig) -> GateResult { + // Fail closed: no isolation dir -> failing GateResult, never a live-root fallback. + let isolation = match unit_test_isolation_dir() { + Ok(dir) => dir, + Err(e) => { + return GateResult { + gate: RelaunchGate::UnitTest, + passed: false, + detail: format!("unit-test gate could not create an isolated state root: {e}"), + }; + } + }; + let iso = isolation.path(); + + // Pin the toolchain to absolute paths resolved from the REAL (pre-override) + // HOME *before* HOME is redirected to the temp dir. Prefer the ambient + // CARGO_HOME / RUSTUP_HOME when present; otherwise derive $HOME/.cargo and + // $HOME/.rustup from the real HOME. Without this, overriding HOME to an + // empty temp dir makes cargo/rustup hunt the toolchain under the empty + // temp $HOME/.cargo and abort — a fresh #4558-class self-inflicted red. + let (cargo_home, rustup_home) = resolve_toolchain_home(); + + 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()) + // Toolchain pin — absolute, resolved from the real HOME, set explicitly + // so the HOME override below cannot strand cargo/rustup. + .env("CARGO_HOME", &cargo_home) + .env("RUSTUP_HOME", &rustup_home) + // Hermetic override — applied AFTER scrub_gate_env so it wins. + .env("SIMARD_STATE_ROOT", iso) + .env("SIMARD_HOME", iso) + .env("HOME", iso) + .env("TMPDIR", iso) + .current_dir(&config.manifest_dir); + + match cmd.output() { + Ok(output) if output.status.success() => GateResult { + gate: RelaunchGate::UnitTest, + passed: true, + detail: "all tests passed".to_string(), + }, + Ok(output) => { + let stdout = String::from_utf8_lossy(&output.stdout); + let stderr = String::from_utf8_lossy(&output.stderr); + let detail = extract_failure_detail(&stdout, &stderr); + GateResult { + gate: RelaunchGate::UnitTest, + passed: false, + detail: format!("tests failed (exit {}): {}", output.status, detail), + } + } + Err(e) => GateResult { + gate: RelaunchGate::UnitTest, + passed: false, + detail: format!("cargo test failed to run: {e}"), + }, + } +} +``` + +- **Explicit argv only.** `cargo test` is spawned via `Command` with an explicit + argument vector — never `sh -c` — and the manifest dir comes from + `config.manifest_dir` only, never a daemon- or network-controlled path. +- **Toolchain pin before `HOME` override.** `resolve_toolchain_home()` returns + absolute `CARGO_HOME` / `RUSTUP_HOME` derived from the real pre-override + `HOME` (preferring ambient values), set on the child so redirecting `HOME` to + the temp dir cannot strand cargo/rustup. `RUSTUP_TOOLCHAIN` is passed through + unchanged. +- **Canonicalized, in-root temp path.** The isolation path is canonicalized and + rejected if it resolves outside the temp root, so a test cannot rediscover the + production state root through a symlink. +- **Residual risk — env-derived paths only.** The four overrides redirect every + path the runtime derives from `SIMARD_STATE_ROOT` / `$HOME` / `TMPDIR` + (WAL, cognitive-store, default socket dir). They do **not** neutralize a test + that hardcodes an absolute socket path or a fixed TCP port. The implementation + step therefore grep-audits the in-process suite for fixed-port `bind` / hardcoded + absolute socket paths and, if any is found, adds an additive per-child override + at this same site rather than relying on the state-root redirect alone. + +### `extract_failure_detail` (diagnosable) + +A pure function over `(stdout, stderr)` — no subprocess, unit-testable in +isolation — that selects the first structured marker block, test-name first, and +clamps it UTF-8-safely to 4096 bytes: + +```rust +/// Extract the operator-actionable failure block from a `cargo test` run. +/// Scans the COMBINED stdout+stderr stream (stdout carries `test … FAILED` / +/// `failures:`; stderr carries `panicked at …`) and returns the first matching +/// marker block with the failing test NAME preserved. Returns a bounded, +/// UTF-8-safe string (<= 4096 bytes). Marker/name selection only — never a raw +/// dump — so enlarged detail cannot leak unrelated output into logs. +fn extract_failure_detail(stdout: &str, stderr: &str) -> String { /* … */ } +``` + +Marker precedence (first match wins): + +1. `failures:` — the cargo/libtest failure block. Note libtest emits + `failures:` twice: the **first** heads the detailed `---- stdout ----` + panic dumps (test name **and** panic message), the **second** lists the bare + failing test names. First-match capture therefore lands on the richer + name+panic block — a superset of just the name list. +2. `panicked at …` — the panic site (carries the assertion / message). +3. `test … FAILED` — the per-test result line. + +If none of the markers is present (e.g. a linker OOM or an abort before any +test line), the extractor falls back to the tail of the combined stream so the +detail is never empty, still clamped to 4096 bytes. + +**Truncation chain.** The gate clamps to **4096 bytes** (raised from 200). The +downstream deploy composer's `bound_detail` (see +[red-canary diagnostics](./overseer-deploy-canary-diagnostics.md)) still applies +its **512-byte** governing cap — but because the gate now extracts the +marker+name block *before* clamping, the failing test name survives both bounds +instead of being lost to a 200-byte spinner fragment. See +[string-truncation helpers](./string-truncation-helpers.md) for the +char-boundary-safe `truncate_output` used at each step. + +### Before / after + +| | Before (#4558 crash-loop) | After (hermetic gate) | +| --- | --- | --- | +| State root | live daemon's (shared WAL/socket) | fresh per-run temp dir | +| Green tree under running daemon | red-canary (exit 101 in <1.3s) | **passes** | +| Capture on failure | stderr only | **stdout + stderr** | +| Detail bound | 200-byte raw head | 4096-byte extracted marker block | +| Failing test name in `failing_detail` | lost (`Drop t…`) | **present** (`failures:` / `panicked at` block) | + +## Fail-closed invariants (preserved) + +- **Canary is the authorization boundary.** The four gates still gate promotion + and still fail closed. A genuinely failing test still reddens. +- **No silent fallback.** A temp-dir/env setup failure returns a **failing** + `GateResult`, never a non-hermetic run against the live state root + (prevents a gate writing production runtime state). +- **No short-circuit.** Gate order `Smoke → UnitTest → GymBaseline → RpcHealth` + runs to completion; `all_gates_passed` still requires every gate to pass. +- **Deny-by-default env unchanged.** `scrub_gate_env` still `env_clear()`s and + re-injects only the base floor + `canary_env` allow-list; the isolation + override adds **only** the four keys above — the allow-list is not widened and + no daemon secret propagates into the child. +- **No privilege change.** The child inherits, never escalates, privilege (no + `sudo`/`setuid`). +- **Detail routes through tracing/OTel only.** The enlarged detail is emitted at + the existing gate log level as structured markers (not a raw dump) and is + bounded by the 512-byte `bound_detail` secondary cap; no `print!`/`println!`. + +## Regression tests + +The change ships bidirectional tests proving the gate passes a green tree under a +simulated live daemon and names the failing test on a red tree. + +| Test surface | Asserts | +| --- | --- | +| [`src/self_relaunch/gates.rs`](https://github.com/rysweet/Simard/blob/main/src/self_relaunch/gates.rs) `#[cfg(test)]` (unit, no subprocess) | `extract_failure_detail` returns the failing test **name** from a `failures:` block and from a `panicked at …` block; UTF-8-boundary clamp at 4096B never splits a char; empty / no-marker input falls back to a bounded tail. | +| [`tests/fixtures/unit_test_gate_fixture/`](https://github.com/rysweet/Simard/blob/main/tests/fixtures) (integration) | A minimal fixture crate (one passing + one panicking `#[test]`) run through the gate: **(a)** the green fixture **passes** even when a simulated live daemon holds the shared `SIMARD_STATE_ROOT` (socket bound / WAL locked), proving isolation; **(b)** the red fixture's `failing_detail` **contains the failing test name** and a `FAILED`/`panicked at`/`failures:` marker — asserted **not** to be a truncated `Drop t…` fragment; **(c)** the green fixture **still passes when the daemon env has `CARGO_HOME` / `RUSTUP_HOME` unset**, proving the toolchain pin resolves them from the real `HOME` rather than the empty temp `$HOME`. | + +The fixture crate is a tiny standalone crate invoked directly, so the gate test +does **not** trigger a recursive full-suite `cargo test`. + +## Compatibility + +- **No public API change.** `run_unit_test_gate` is private; the three new helpers + (`unit_test_isolation_dir`, `resolve_toolchain_home`, `extract_failure_detail`) + are private. Public types and the gate sequence are unchanged. +- **Smallest surface.** Only + [`src/self_relaunch/gates.rs`](https://github.com/rysweet/Simard/blob/main/src/self_relaunch/gates.rs) + is edited. `canary.rs`, `types.rs`, and `overseer/deploy.rs` are untouched. +- **Diagnostics reused.** `CanaryResult.failing_gate` / `failing_detail`, + `refusal_reason`, and the `overseer::deploy` WARN + ([#4420](./overseer-deploy-canary-diagnostics.md)) are reused, not + reimplemented — the WARN simply now carries a named test instead of a spinner + fragment. +- **No `print`-family macros; no silent fallbacks.** All emission is `tracing` + structured key=value. +- **No `Bridge` naming.** New identifiers follow the + [no-Bridge-naming guard](./no-bridge-naming-guard.md). + +## See also + +- [How to diagnose a red-canary unit-test gate](../howto/diagnose-a-red-canary-unit-test-gate.md) — + the operator runbook for reading the named failure and confirming isolation. +- [Canary gate isolation and self-deploy convergence](./canary-gate-convergence.md) — + the #4440 `scrub_gate_env` / `canary_env` repair this builds on. +- [Overseer deploy red-canary diagnostics](./overseer-deploy-canary-diagnostics.md) — + the `failing_gate` / `failing_detail` / `refusal_reason` surface. +- [State-root resolution](./state-root-resolution.md) — how + `SIMARD_STATE_ROOT` / `$HOME/.simard` selects the WAL / cognitive-store / + socket paths the isolation override redirects. +- [String truncation helpers](./string-truncation-helpers.md) — the + char-boundary-safe `truncate_output` used for the 4096B and 512B bounds. +- [Self-deploy API reference](./self-deploy-api.md) — the guarded deploy path + the green canary now unblocks. diff --git a/src/operator_cli/mod.rs b/src/operator_cli/mod.rs index b68215a1f..371baa480 100644 --- a/src/operator_cli/mod.rs +++ b/src/operator_cli/mod.rs @@ -472,7 +472,13 @@ fn dispatch_handover_command( let gates = default_gates(); let results = verify_canary(&canary, &gates, &config)?; for r in &results { - eprintln!(" {r}"); + // Redact URL-embedded credentials (SEC-D2) before printing the raw gate + // detail — a failing gate's captured output can carry a token-bearing + // remote URL, matching the `overseer::deploy` redact-at-emission pattern. + eprintln!( + " {}", + crate::self_deploy::source_prep::redact_credentials(&r.to_string()) + ); } if !all_gates_passed(&results) { diff --git a/src/self_relaunch/gates.rs b/src/self_relaunch/gates.rs index 73d92488e..f04cc8585 100644 --- a/src/self_relaunch/gates.rs +++ b/src/self_relaunch/gates.rs @@ -1,4 +1,4 @@ -use std::path::Path; +use std::path::{Path, PathBuf}; use std::process::Command; use super::types::{GateResult, RelaunchConfig, RelaunchGate}; @@ -211,14 +211,193 @@ fn run_smoke_gate(binary: &Path, config: &RelaunchConfig) -> GateResult { } } +/// Create a fresh, private RAII isolation directory for one `unit-test` gate +/// run. The gate overrides `SIMARD_STATE_ROOT` / `SIMARD_HOME` / `HOME` / +/// `TMPDIR` to this dir so the in-process lib-test suite resolves an **empty, +/// private** state root ([`crate::runtime_config`]: `SIMARD_STATE_ROOT` else +/// `$HOME/.simard`) instead of the live daemon's — it therefore cannot open the +/// daemon's WAL / cognitive-store or bind its socket (the #4558 abort). The dir +/// is randomized (`tempfile`, `O_EXCL`) and canonicalized; if it resolves +/// **outside** the temp root (a symlink escape) the call fails so the caller can +/// fail closed rather than run against a rediscovered production state root. +/// +/// Returns `Err` on any setup failure. The caller ([`run_unit_test_gate`]) then +/// returns a **failing** `GateResult` — never a silent non-hermetic fallback to +/// the daemon's live state root (a gate must never write production runtime +/// state). +fn unit_test_isolation_dir() -> std::io::Result { + let dir = tempfile::Builder::new() + .prefix("simard-unit-test-gate-") + .tempdir()?; + // Symlink-escape defense: the isolation path must stay inside the temp root + // so a test cannot rediscover the production state root through a symlink. + let canon = dir.path().canonicalize()?; + let temp_root = std::env::temp_dir().canonicalize()?; + if !canon.starts_with(&temp_root) { + return Err(std::io::Error::other(format!( + "isolation dir {} escaped temp root {}", + canon.display(), + temp_root.display() + ))); + } + Ok(dir) +} + +/// The real, pre-override `HOME` — the value used to derive the toolchain roots +/// **before** the hermetic `HOME` override redirects `HOME` to the empty temp +/// dir. Falls back to `/` (an absolute, non-writable root) only if `HOME` is +/// entirely unset, which keeps the derived toolchain paths absolute. +fn real_home() -> PathBuf { + std::env::var_os("HOME") + .map(PathBuf::from) + .unwrap_or_else(|| PathBuf::from("/")) +} + +/// Resolve absolute `CARGO_HOME` / `RUSTUP_HOME` for the gate child, pinned from +/// the **real, pre-override** `HOME` (or the ambient values when set), so the +/// hermetic `HOME` override cannot strand `cargo`/`rustup`. +/// +/// Load-bearing invariant (see `docs/reference/hermetic-unit-test-gate.md`): +/// `cargo`/`rustup` resolve their toolchain from `CARGO_HOME` / `RUSTUP_HOME` +/// and, *only when those are unset*, fall back to `$HOME/.cargo` / +/// `$HOME/.rustup`. Under a clean systemd unit those vars are frequently absent +/// (the daemon relies on the `$HOME/.cargo` default). If `HOME` is then +/// overridden to an **empty** temp dir while they are unset, `cargo test` hunts +/// the toolchain under the empty temp `$HOME/.cargo` and aborts — a fresh +/// #4558-class self-inflicted red. Pinning them here from the real `HOME` +/// (preferring ambient values) prevents that. +fn resolve_toolchain_home() -> (PathBuf, PathBuf) { + let cargo_home = std::env::var_os("CARGO_HOME") + .map(PathBuf::from) + .unwrap_or_else(|| real_home().join(".cargo")); + let rustup_home = std::env::var_os("RUSTUP_HOME") + .map(PathBuf::from) + .unwrap_or_else(|| real_home().join(".rustup")); + (cargo_home, rustup_home) +} + +/// Upper bound (bytes) on the extracted `unit-test` failure detail. Raised from +/// the old 200-byte stderr-only head so the failing test NAME survives; the +/// downstream deploy `bound_detail` 512-byte cap still governs the final size. +const FAILURE_DETAIL_MAX_BYTES: usize = 4096; + +/// Extract the operator-actionable failure block from a `cargo test` run. +/// +/// Scans the COMBINED stdout+stderr stream (stdout carries `test … FAILED` / +/// `failures:`; stderr carries `panicked at …`) and returns the first matching +/// marker block with the failing test NAME preserved, in precedence order: +/// `failures:` > `panicked at …` > `test … FAILED`. The block is captured from +/// the **start of the marker's line** so a name that precedes `panicked at` +/// (`thread '…' panicked at …`) or heads the `failures:` dump survives. With no +/// marker at all (e.g. a linker OOM or an abort before any test line) it falls +/// back to the tail of the combined stream so the detail is never empty. The +/// result is UTF-8-safely clamped to [`FAILURE_DETAIL_MAX_BYTES`] — never a raw +/// dump — so an enlarged detail cannot leak unrelated output into logs. +fn extract_failure_detail(stdout: &str, stderr: &str) -> String { + let combined = if stdout.is_empty() { + stderr.to_string() + } else if stderr.is_empty() { + stdout.to_string() + } else { + format!("{stdout}\n{stderr}") + }; + + // First match wins, in precedence order. `failures:` heads the richest + // libtest block (test name AND panic message); `panicked at` carries the + // panic site (name precedes it on the same line); `FAILED` is the bare + // per-test result line (`test … FAILED`). + for marker in ["failures:", "panicked at", "FAILED"] { + if let Some(pos) = combined.find(marker) { + let line_start = combined[..pos].rfind('\n').map_or(0, |nl| nl + 1); + let block = combined[line_start..].trim(); + return clamp_utf8(block, FAILURE_DETAIL_MAX_BYTES); + } + } + + // No structured marker: bounded, non-empty tail so the operator sees the + // real output (e.g. a linker/OOM abort) rather than an empty detail. + let trimmed = combined.trim(); + clamp_utf8_tail(trimmed, FAILURE_DETAIL_MAX_BYTES) +} + +/// UTF-8-safe head clamp to at most `max` bytes (never splits a codepoint, +/// never appends an ellipsis that could exceed `max`). +fn clamp_utf8(s: &str, max: usize) -> String { + if s.len() <= max { + return s.to_string(); + } + let mut end = max; + while end > 0 && !s.is_char_boundary(end) { + end -= 1; + } + s[..end].to_string() +} + +/// UTF-8-safe tail clamp to at most `max` bytes (keeps the END of `s`, which is +/// where an abort's real error typically lands). +fn clamp_utf8_tail(s: &str, max: usize) -> String { + if s.len() <= max { + return s.to_string(); + } + let mut start = s.len() - max; + while start < s.len() && !s.is_char_boundary(start) { + start += 1; + } + s[start..].to_string() +} + +/// Run the `unit-test` canary gate: `cargo test` against `config.manifest_dir`, +/// **hermetically** isolated so a running daemon cannot red-canary a passing +/// suite (#4558), and **diagnosably** — a real failure names the failing test. +/// +/// Isolation: after [`scrub_gate_env`], four keys (`SIMARD_STATE_ROOT`, +/// `SIMARD_HOME`, `HOME`, `TMPDIR`) are overridden **per child** to a fresh +/// [`unit_test_isolation_dir`] and `current_dir` is set to the manifest dir, so +/// the in-process test suite resolves an empty private state root and cannot +/// open the live daemon's WAL / cognitive-store or bind its socket. The +/// toolchain (`CARGO_HOME` / `RUSTUP_HOME`) is pinned from the real +/// pre-override `HOME` **before** the `HOME` override so the redirect cannot +/// strand `cargo`/`rustup` (see [`resolve_toolchain_home`]). Fail-closed: a +/// temp-dir setup failure returns a failing `GateResult`, never a non-hermetic +/// run against the live state root. fn run_unit_test_gate(config: &RelaunchConfig) -> GateResult { + // Fail closed: no isolation dir -> failing GateResult, never a live-root fallback. + let isolation = match unit_test_isolation_dir() { + Ok(dir) => dir, + Err(e) => { + return GateResult { + gate: RelaunchGate::UnitTest, + passed: false, + detail: format!("unit-test gate could not create an isolated state root: {e}"), + }; + } + }; + let iso = isolation.path(); + + // Pin the toolchain from the REAL (pre-override) HOME before HOME is + // redirected to the empty temp dir (otherwise cargo/rustup hunt the + // toolchain under the empty temp $HOME/.cargo and abort — a #4558-class red). + let (cargo_home, rustup_home) = resolve_toolchain_home(); + 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()) + // Toolchain pin — absolute, resolved from the real HOME, set explicitly + // so the HOME override below cannot strand cargo/rustup. + .env("CARGO_HOME", &cargo_home) + .env("RUSTUP_HOME", &rustup_home) + // Hermetic override — applied AFTER scrub_gate_env so the isolated temp + // path wins over the allow-listed SIMARD_HOME / SIMARD_STATE_ROOT. + .env("SIMARD_STATE_ROOT", iso) + .env("SIMARD_HOME", iso) + .env("HOME", iso) + .env("TMPDIR", iso) + .current_dir(&config.manifest_dir); + match cmd.output() { Ok(output) if output.status.success() => GateResult { gate: RelaunchGate::UnitTest, @@ -226,12 +405,17 @@ fn run_unit_test_gate(config: &RelaunchConfig) -> GateResult { detail: "all tests passed".to_string(), }, Ok(output) => { + // Capture BOTH streams (#4558): the failing test name lives on + // stdout (`failures:` / `test … FAILED`); a panic site lives on + // stderr (`panicked at …`). The old stderr-only, 200-byte head + // landed on a progress-spinner fragment (`Drop t…`) and hid it. + let stdout = String::from_utf8_lossy(&output.stdout); let stderr = String::from_utf8_lossy(&output.stderr); - let truncated = truncate_output(&stderr, 200); + let detail = extract_failure_detail(&stdout, &stderr); GateResult { gate: RelaunchGate::UnitTest, passed: false, - detail: format!("tests failed (exit {}): {}", output.status, truncated), + detail: format!("tests failed (exit {}): {}", output.status, detail), } } Err(e) => GateResult { @@ -519,6 +703,208 @@ mod tests { let results = verify_canary(Path::new("/no-such-binary"), &[], &config).unwrap(); assert!(results.is_empty()); } + + // ───────────────────────────────────────────────────────────────────────── + // TDD (Problem 2 — #4558 diagnosable unit-test gate): FAILING tests, written + // first. These specify `extract_failure_detail(stdout, stderr)`, the pure + // helper the hermetic gate uses so a red tree's failing test NAME survives + // into `failing_detail` instead of being lost to the old stderr-only, + // 200-byte spinner-fragment head (`Drop t…`). + // + // Contract (see docs/reference/hermetic-unit-test-gate.md): + // * Scans the COMBINED stdout+stderr stream (libtest writes `failures:` / + // `test … FAILED` to stdout; a panic writes `panicked at …` to stderr). + // * Extracts the FIRST structured marker block, test-name first, in + // precedence order: `failures:` > `panicked at …` > `test … FAILED`. + // * UTF-8-safely clamps the result to 4096 bytes (raised from 200). + // * No-marker input falls back to a bounded, non-empty tail — never a + // raw dump, never empty for non-empty input. + // + // They MUST fail to compile/run against the current code (the function does + // not exist yet) and pass once the fix lands. Pure — no subprocess, no env + // mutation, so no `serial(cognitive_memory)` key is required. `tracing`/OTel + // discipline is a runtime concern; these assert only the extractor's return. + // ───────────────────────────────────────────────────────────────────────── + + /// A realistic libtest `failures:` block on **stdout** — the richest marker + /// (test name AND panic message) and the one the old stderr-only capture + /// missed entirely. + const FAILURES_BLOCK_STDOUT: &str = "\ +running 2 tests +test tests::fixture_passes_cleanly ... ok +test tests::fixture_panics_when_toggled ... FAILED + +failures: + +---- tests::fixture_panics_when_toggled stdout ---- +thread 'tests::fixture_panics_when_toggled' panicked at tests/fixtures/unit_test_gate_fixture/lib.rs:44:13: +intentional fixture failure for red-canary detail extraction +note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace + + +failures: + tests::fixture_panics_when_toggled + +test result: FAILED. 1 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s +"; + + /// What cargo itself prints to **stderr** for a failed test run — carries no + /// test name, so a stderr-only capture cannot name the failure. + const CARGO_STDERR: &str = "error: test failed, to rerun pass `--lib`\n"; + + #[test] + fn extract_failure_detail_names_failing_test_from_failures_block() { + let detail = extract_failure_detail(FAILURES_BLOCK_STDOUT, CARGO_STDERR); + assert!( + detail.contains("fixture_panics_when_toggled"), + "failing test NAME must survive into the detail; got: {detail}" + ); + assert!( + detail.contains("failures:"), + "the `failures:` marker block must be selected; got: {detail}" + ); + // The #4558 regression: the detail must NOT be a truncated progress + // spinner fragment that hides which test failed. + assert!( + !detail.trim().starts_with("Drop t"), + "detail must not be a spinner fragment; got: {detail}" + ); + } + + #[test] + fn extract_failure_detail_names_failing_test_from_panicked_at_on_stderr() { + // No `failures:` anywhere; the panic site is on stderr (uncaptured + // panic / abort path). The extractor must still surface it. + let stderr = "\ +thread 'tests::open_wal_lock' panicked at src/cognitive_memory/open_guard.rs:88:9: +state root already locked by the live daemon +note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace +"; + let detail = extract_failure_detail("", stderr); + assert!( + detail.contains("panicked at"), + "the `panicked at` marker must be selected; got: {detail}" + ); + assert!( + detail.contains("open_wal_lock"), + "the panicking test/context name must survive; got: {detail}" + ); + } + + #[test] + fn extract_failure_detail_names_failing_test_from_failed_line() { + // Lowest-precedence marker: only a per-test `... FAILED` result line, + // no `failures:` block and no `panicked at` (e.g. a `#[should_panic]` + // that did not panic, or a non-panicking assertion harness). + let stdout = "\ +running 1 test +test tests::rpc_dials_live_socket ... FAILED +"; + let detail = extract_failure_detail(stdout, ""); + assert!( + detail.contains("rpc_dials_live_socket"), + "the FAILED test name must survive; got: {detail}" + ); + assert!( + detail.contains("FAILED"), + "the `FAILED` marker must be present; got: {detail}" + ); + } + + #[test] + fn extract_failure_detail_prefers_failures_block_over_panicked_at() { + // Both markers present with DIFFERENT names: `failures:` wins, so the + // richer name+panic block is returned rather than a bare panic site. + let stdout = "\ +test tests::the_failures_block_test ... FAILED + +failures: + +---- tests::the_failures_block_test stdout ---- +assertion `left == right` failed + +failures: + tests::the_failures_block_test +"; + let stderr = "thread 'tests::a_different_panic_test' panicked at src/x.rs:1:1:\nboom\n"; + let detail = extract_failure_detail(stdout, stderr); + assert!( + detail.contains("the_failures_block_test"), + "the `failures:` block must take precedence; got: {detail}" + ); + } + + #[test] + fn extract_failure_detail_clamps_to_4096_bytes() { + // A marker followed by a very long body must be bounded to 4096 bytes + // (raised from the old 200) — the failing name still fits well inside. + let mut stdout = String::from("failures:\n tests::huge_output_test\n"); + stdout.push_str(&"x".repeat(20_000)); + let detail = extract_failure_detail(&stdout, ""); + assert!( + detail.len() <= 4096, + "detail must be clamped to <= 4096 bytes; got {} bytes", + detail.len() + ); + assert!( + detail.contains("huge_output_test"), + "the failing name must survive the clamp; got: {detail}" + ); + } + + #[test] + fn extract_failure_detail_is_utf8_boundary_safe() { + // Multi-byte characters straddling the 4096-byte clamp must never panic + // and must yield valid UTF-8 (guaranteed by the `String` return, but + // the clamp must not truncate mid-codepoint and lose data or abort). + let mut stdout = String::from("failures:\n tests::utf8_boundary_test\n"); + stdout.push_str(&"é".repeat(4000)); // 2 bytes each → ~8000 bytes body + let detail = extract_failure_detail(&stdout, ""); + assert!( + detail.len() <= 4096, + "clamped detail must be <= 4096 bytes; got {} bytes", + detail.len() + ); + // Round-trips as valid UTF-8 without panicking (implicit in `String`). + assert!( + detail.contains("utf8_boundary_test"), + "the failing name must survive the UTF-8-safe clamp; got: {detail}" + ); + } + + #[test] + fn extract_failure_detail_no_marker_falls_back_to_bounded_nonempty_tail() { + // No structured marker at all (e.g. a linker OOM or an abort before any + // test line). The detail must be non-empty (so the operator sees + // *something*) and still bounded. + let stdout = ""; + let stderr = "collect2: fatal error: ld terminated with signal 9 [Killed]\n"; + let detail = extract_failure_detail(stdout, stderr); + assert!( + !detail.trim().is_empty(), + "no-marker input must still yield a non-empty detail" + ); + assert!( + detail.len() <= 4096, + "fallback detail must be bounded; got {} bytes", + detail.len() + ); + assert!( + detail.contains("ld terminated") || detail.contains("fatal error"), + "fallback must carry a tail of the real output; got: {detail}" + ); + } + + #[test] + fn extract_failure_detail_scans_stdout_when_stderr_empty() { + // Proves stdout is scanned even when stderr is empty — the exact #4558 + // gap (the failing name lives on stdout; the old code read only stderr). + let detail = extract_failure_detail(FAILURES_BLOCK_STDOUT, ""); + assert!( + detail.contains("fixture_panics_when_toggled"), + "stdout must be scanned for the failing name; got: {detail}" + ); + } } // ───────────────────────────────────────────────────────────────────────────── diff --git a/src/self_relaunch_semaphore/handoff.rs b/src/self_relaunch_semaphore/handoff.rs index e129a49b8..ad8290cdf 100644 --- a/src/self_relaunch_semaphore/handoff.rs +++ b/src/self_relaunch_semaphore/handoff.rs @@ -90,10 +90,16 @@ pub fn coordinated_handoff(my_pid: u32, config: &HandoffConfig) -> SimardResult< // 3. Verify gates. let gate_results = verify_canary(&canary_path, &config.gates, &config.relaunch)?; if !all_gates_passed(&gate_results) { + // Redact URL-embedded credentials (SEC-D2) before the raw gate detail + // reaches the error `reason`, which propagates through + // `coordinated_relaunch` into the daemon's restart-error logging. The + // #4558 detail enlargement (200B→4096B + stdout capture) widened what a + // failing test's output can carry here, so this must be credential-safe + // like the `overseer::deploy` path, not a raw `Display`. let failures: Vec = gate_results .iter() .filter(|g| !g.passed) - .map(|g| g.to_string()) + .map(|g| crate::self_deploy::source_prep::redact_credentials(&g.to_string())) .collect(); return Err(SimardError::RpcCallFailed { endpoint: "handoff".to_string(), diff --git a/tests/adaptive_scaling.rs b/tests/adaptive_scaling.rs index d3aa38961..f5ad05817 100644 --- a/tests/adaptive_scaling.rs +++ b/tests/adaptive_scaling.rs @@ -312,9 +312,19 @@ fn scaler_current_max_can_override_config() { }) .collect(); - // Use scaler's current_max as the config limit. + // Use scaler's current_max as the config limit. Set `scaler: None` + // explicitly instead of relying on `..OodaConfig::default()` alone: + // `OodaConfig::default()` consults the process env (`SIMARD_SCALING`), so on + // a host with `SIMARD_SCALING=auto` the default AIMD scaler (seeded to the + // env ceiling) would override the explicit `max_concurrent_actions` under + // test and `decide` would cap by live system pressure rather than the + // configured limit — the same non-hermetic env leak issue #2732 fixed for + // `decide_respects_max_concurrent_actions` and the class of environment-leak + // false failure issue #4558 hardens the deploy gate against. Pinning + // `scaler: None` keeps the scaler's `current_max` the sole cap under test. let config = OodaConfig { max_concurrent_actions: scaler.current_max(), + scaler: None, ..OodaConfig::default() }; diff --git a/tests/fixtures/unit_test_gate_fixture/Cargo.lock b/tests/fixtures/unit_test_gate_fixture/Cargo.lock new file mode 100644 index 000000000..377176332 --- /dev/null +++ b/tests/fixtures/unit_test_gate_fixture/Cargo.lock @@ -0,0 +1,7 @@ +# This file is automatically @generated by Cargo. +# It is not intended for manual editing. +version = 4 + +[[package]] +name = "unit_test_gate_fixture" +version = "0.0.0" diff --git a/tests/fixtures/unit_test_gate_fixture/Cargo.toml b/tests/fixtures/unit_test_gate_fixture/Cargo.toml new file mode 100644 index 000000000..c99ab9437 --- /dev/null +++ b/tests/fixtures/unit_test_gate_fixture/Cargo.toml @@ -0,0 +1,15 @@ +[package] +name = "unit_test_gate_fixture" +version = "0.0.0" +edition = "2021" +publish = false + +# Standalone workspace root: the fixture is NOT a member of the `simard` +# package. The `unit-test` canary gate builds it in isolation via +# `--manifest-path`, so it must never inherit a parent workspace (which would +# make `cargo test --manifest-path /Cargo.toml` try to resolve the whole +# simard tree). An empty `[workspace]` table pins it as its own root. +[workspace] + +[lib] +path = "lib.rs" diff --git a/tests/fixtures/unit_test_gate_fixture/lib.rs b/tests/fixtures/unit_test_gate_fixture/lib.rs new file mode 100644 index 000000000..af35413c1 --- /dev/null +++ b/tests/fixtures/unit_test_gate_fixture/lib.rs @@ -0,0 +1,51 @@ +//! Minimal fixture crate for the hermetic unit-test canary gate regression +//! tests (rysweet/Simard#4558). +//! +//! This crate is deliberately **not** part of the `simard` package build. The +//! `unit-test` canary gate ([`simard::self_relaunch::verify_canary`] driving +//! [`RelaunchGate::UnitTest`]) compiles and runs it in isolation via +//! `--manifest-path`, exactly as it runs the real tree. A regression test +//! (`tests/self_relaunch_hermetic_unit_test_gate.rs`) points the gate's +//! `manifest_dir` at this crate to prove, without a recursive full-suite run: +//! +//! * **green:** with the toggle unset, both tests pass, so the gate goes +//! GREEN even when a simulated live daemon holds the shared +//! `SIMARD_STATE_ROOT` (proving the hermetic per-run temp state root, and +//! that the `CARGO_HOME`/`RUSTUP_HOME` toolchain pin survives the `HOME` +//! override); and +//! * **red:** with the toggle set, [`tests::fixture_panics_when_toggled`] +//! panics, so the gate goes RED and its `failing_detail` must carry the +//! failing test **name** (not a truncated spinner fragment). +//! +//! The SAME crate serves as both the green and the red tree via a runtime +//! toggle (`SIMARD_GATE_FIXTURE_FAIL`) rather than a compile-time feature, +//! because the gate invokes a plain `cargo test` with no extra `--features`. +//! The toggle reaches the child through the gate's `canary_env` allow-list. + +/// Trivial exported item so the fixture lib is never an empty crate. Not used +/// by the gate; present only to keep the fixture a well-formed `[lib]` target. +pub fn fixture_marker() -> u8 { + 42 +} + +#[cfg(test)] +mod tests { + /// The green half: always passes. Present so a "green tree" run has at + /// least one genuinely passing test alongside the (untriggered) toggle. + #[test] + fn fixture_passes_cleanly() { + assert_eq!(super::fixture_marker(), 42); + } + + /// The red half: panics **only** when `SIMARD_GATE_FIXTURE_FAIL` is present + /// in the environment. The panic message and this test's fully-qualified + /// name are what the gate's `extract_failure_detail` must surface into + /// `failing_detail` on a red tree. With the toggle unset this is a no-op, + /// so the same crate is a clean green tree. + #[test] + fn fixture_panics_when_toggled() { + if std::env::var_os("SIMARD_GATE_FIXTURE_FAIL").is_some() { + panic!("intentional fixture failure for red-canary detail extraction"); + } + } +} diff --git a/tests/self_relaunch_hermetic_unit_test_gate.rs b/tests/self_relaunch_hermetic_unit_test_gate.rs new file mode 100644 index 000000000..beb333457 --- /dev/null +++ b/tests/self_relaunch_hermetic_unit_test_gate.rs @@ -0,0 +1,296 @@ +//! End-to-end regression for the hermetic unit-test canary gate +//! (rysweet/Simard#4558). +//! +//! ## What #4558 was +//! +//! `run_unit_test_gate` ran `cargo test` as a child of the **live daemon**. The +//! in-process lib-test suite bound the daemon's fixed socket / locked the shared +//! WAL under the daemon's `SIMARD_STATE_ROOT` and aborted with exit `101` in +//! under 1.3s — reddening a tree that passed clean standalone (9279/0). Every +//! self-deploy for 6+ hours was refused on the `unit-test` gate, pinning +//! `running_commit` while `DeployDrift` grew. A second failure made it +//! undiagnosable: the gate captured only stderr, truncated to 200 bytes, so the +//! operator saw a spinner fragment (`Drop t…`) instead of the failing test name. +//! +//! ## What these tests pin +//! +//! They drive the REAL public gate (`verify_canary` with `RelaunchGate::UnitTest`) +//! against a tiny standalone fixture crate +//! (`tests/fixtures/unit_test_gate_fixture/`) — never the full `simard` suite, +//! so there is no recursive 30-minute run: +//! +//! * **green** — with the toggle unset the fixture is a clean green tree; the +//! gate must go GREEN even when a **simulated live daemon** holds the shared +//! `SIMARD_STATE_ROOT` (proving the hermetic per-run temp state root wins); +//! * **toolchain pin** — the green tree must still pass when the daemon env has +//! `CARGO_HOME` / `RUSTUP_HOME` **unset**, proving the gate resolves the +//! toolchain from the real `HOME` before the hermetic `HOME` override rather +//! than stranding `cargo`/`rustup` under an empty temp `$HOME/.cargo` (a +//! fresh #4558-class self-inflicted red); +//! * **red / diagnosable** — with the toggle set the named test panics; the +//! gate must go RED and its `failing_detail` must carry the failing test +//! **name** and a `FAILED` / `panicked at` / `failures:` marker — asserted +//! NOT to be a truncated `Drop t…` spinner fragment. +//! +//! ## Why these three are `#[ignore]` by default +//! +//! Driving `RelaunchGate::UnitTest` end-to-end spawns a **nested `cargo test`** +//! (the gate's whole job) from *inside* this `cargo test` run. Nested cargo +//! serializes on the global `~/.cargo/.package-cache` lock, so running these in +//! the default suite makes them slow and lock-contended — the exact reason the +//! in-tree `gates::tests::verify_canary_runs_all_gates_without_short_circuit` +//! deliberately EXCLUDES `UnitTest` (its comment cites a 30-minute recursive +//! run). They are therefore `#[ignore]`d so the default `cargo test` stays green +//! and fast, and are run explicitly in a dedicated lane: +//! +//! ```text +//! cargo test --test self_relaunch_hermetic_unit_test_gate -- --ignored --test-threads=1 +//! ``` +//! +//! The deterministic, subprocess-free half of the #4558 diagnosability contract +//! (a red tree's failing test NAME survives into `failing_detail`) is proven +//! WITHOUT nested cargo by the `extract_failure_detail` unit tests in +//! `src/self_relaunch/gates.rs` — those are the primary always-run red-phase +//! tests. The `#[ignore]`d tests below are the true end-to-end regression guards. +//! +//! ## TDD status +//! +//! Written before the fix. The **red / diagnosable** assertion FAILS against the +//! current code (the current gate reads only stderr; the failing name is on +//! stdout) and passes once the stdout+stderr capture + `extract_failure_detail` +//! land. The **green** and **toolchain-pin** assertions are regression guards +//! for the hermetic `HOME`/state-root override + toolchain pin. Run them with +//! `-- --ignored` (see above) to exercise the end-to-end gate. +//! +//! Constraints honoured: additive; drives only public API; emits nothing itself +//! (the crate logs via `tracing`/OTel); intent-revealing names only. + +use std::path::{Path, PathBuf}; +use std::sync::atomic::{AtomicU32, Ordering}; + +use simard::self_relaunch::{RelaunchConfig, RelaunchGate, verify_canary}; + +/// The runtime toggle the fixture reads to become a red tree. Allow-listed into +/// the gate's `canary_env` so the deny-by-default scrub re-injects it into the +/// child; its value is read live from this process's env at spawn time. +const FIXTURE_FAIL_TOGGLE: &str = "SIMARD_GATE_FIXTURE_FAIL"; + +/// The fully-qualified name of the fixture's panicking test — the name that must +/// survive into `failing_detail` on a red tree. +const RED_TEST_NAME: &str = "fixture_panics_when_toggled"; + +/// Monotonic suffix so each test's isolated dirs never collide within this +/// process, even though the env-mutating tests are serialized. +static SEQ: AtomicU32 = AtomicU32::new(0); + +fn fixture_manifest_dir() -> PathBuf { + Path::new(env!("CARGO_MANIFEST_DIR")).join("tests/fixtures/unit_test_gate_fixture") +} + +fn unique_dir(tag: &str) -> PathBuf { + let dir = std::env::temp_dir().join(format!( + "simard-hermetic-gate-{}-{}-{}", + tag, + std::process::id(), + SEQ.fetch_add(1, Ordering::Relaxed) + )); + std::fs::create_dir_all(&dir).expect("create isolated test dir"); + dir +} + +/// A gate config pointed at the fixture crate, with the toggle and +/// `SIMARD_STATE_ROOT` allow-listed so a simulated-live-daemon state root and +/// the red toggle both reach (or are overridden in) the child as the scenario +/// requires. A fresh `canary_target_dir` per call keeps the fixture build +/// isolated. +fn fixture_gate_config(tag: &str) -> RelaunchConfig { + RelaunchConfig { + manifest_dir: fixture_manifest_dir(), + canary_target_dir: unique_dir(&format!("target-{tag}")), + canary_env: vec![ + "SIMARD_STATE_ROOT".to_string(), + FIXTURE_FAIL_TOGGLE.to_string(), + ], + ..RelaunchConfig::default() + } +} + +/// GREEN: a clean green fixture must pass the `unit-test` gate even while a +/// simulated live daemon holds the shared `SIMARD_STATE_ROOT` — proving the gate +/// isolates into its own per-run temp state root instead of colliding with the +/// daemon's WAL/socket (the #4558 abort). +/// +/// Serialized: mutates process-global env (`SIMARD_STATE_ROOT`, and clears the +/// toggle) which a concurrent test's env read could tear. +#[test] +#[ignore = "spawns a nested `cargo test` (fixture build); run in a dedicated lane via `-- --ignored` to avoid ~/.cargo/.package-cache lock contention in the default suite"] +#[serial_test::serial(hermetic_gate_env)] +fn green_fixture_passes_gate_under_simulated_live_daemon() { + let config = fixture_gate_config("green"); + // Simulate the live daemon: a shared state root the daemon "owns". + let daemon_state_root = unique_dir("daemon-state-root"); + + // SAFETY: serialized under the `hermetic_gate_env` key; no concurrent test + // in this binary reads these vars while this test runs. + unsafe { + std::env::set_var("SIMARD_STATE_ROOT", &daemon_state_root); + std::env::remove_var(FIXTURE_FAIL_TOGGLE); + } + + let results = verify_canary( + Path::new("/unused-by-unit-test-gate"), + &[RelaunchGate::UnitTest], + &config, + ) + .expect("verify_canary should not error"); + + // SAFETY: see above. + unsafe { + std::env::remove_var("SIMARD_STATE_ROOT"); + } + + assert_eq!(results.len(), 1); + assert!( + results[0].passed, + "a green fixture must pass the hermetic unit-test gate even under a \ + simulated live daemon holding the shared SIMARD_STATE_ROOT; got: {}", + results[0].detail + ); +} + +/// TOOLCHAIN PIN: the green tree must still pass when the daemon env has +/// `CARGO_HOME` / `RUSTUP_HOME` **unset**. Under the hermetic `HOME` override +/// this only holds if the gate pins the toolchain from the real pre-override +/// `HOME`; without the pin, `cargo`/`rustup` hunt an empty temp `$HOME/.cargo` +/// and abort — a fresh #4558-class self-inflicted red. +/// +/// Serialized: mutates process-global env (`CARGO_HOME`/`RUSTUP_HOME`). +#[test] +#[ignore = "spawns a nested `cargo test` (fixture build); run in a dedicated lane via `-- --ignored` to avoid ~/.cargo/.package-cache lock contention in the default suite"] +#[serial_test::serial(hermetic_gate_env)] +fn green_fixture_passes_when_daemon_env_has_no_cargo_or_rustup_home() { + let config = fixture_gate_config("toolchain"); + + // Save and clear CARGO_HOME/RUSTUP_HOME to model a clean systemd unit where + // the daemon relies on the `$HOME/.cargo` default. Restored after the run. + let saved_cargo = std::env::var_os("CARGO_HOME"); + let saved_rustup = std::env::var_os("RUSTUP_HOME"); + + // SAFETY: serialized under the `hermetic_gate_env` key. + unsafe { + std::env::remove_var("CARGO_HOME"); + std::env::remove_var("RUSTUP_HOME"); + std::env::remove_var(FIXTURE_FAIL_TOGGLE); + } + + let results = verify_canary( + Path::new("/unused-by-unit-test-gate"), + &[RelaunchGate::UnitTest], + &config, + ); + + // SAFETY: restore the original toolchain env before asserting so a panic + // cannot leak the cleared state into other tests. + unsafe { + match saved_cargo { + Some(v) => std::env::set_var("CARGO_HOME", v), + None => std::env::remove_var("CARGO_HOME"), + } + match saved_rustup { + Some(v) => std::env::set_var("RUSTUP_HOME", v), + None => std::env::remove_var("RUSTUP_HOME"), + } + } + + let results = results.expect("verify_canary should not error"); + assert_eq!(results.len(), 1); + assert!( + results[0].passed, + "the green fixture must still pass with CARGO_HOME/RUSTUP_HOME unset in \ + the daemon env — the gate must resolve the toolchain from the real HOME \ + before the hermetic HOME override; got: {}", + results[0].detail + ); +} + +/// RED / DIAGNOSABLE: a genuinely failing test must redden the gate AND the +/// `failing_detail` must name the failing test with a structured marker. This is +/// the #4558 diagnosability regression — it FAILS against the current +/// stderr-only, 200-byte gate (the failing name is on stdout) and passes once +/// the stdout+stderr capture + `extract_failure_detail` land. +/// +/// Serialized: mutates process-global env (the fail toggle). +#[test] +#[ignore = "spawns a nested `cargo test` (fixture build); run in a dedicated lane via `-- --ignored` to avoid ~/.cargo/.package-cache lock contention in the default suite"] +#[serial_test::serial(hermetic_gate_env)] +fn red_fixture_failing_detail_names_the_failing_test() { + let config = fixture_gate_config("red"); + + // SAFETY: serialized under the `hermetic_gate_env` key. + unsafe { + std::env::set_var(FIXTURE_FAIL_TOGGLE, "1"); + } + + let results = verify_canary( + Path::new("/unused-by-unit-test-gate"), + &[RelaunchGate::UnitTest], + &config, + ); + + // SAFETY: clear the toggle before asserting so a panic cannot leak it. + unsafe { + std::env::remove_var(FIXTURE_FAIL_TOGGLE); + } + + let results = results.expect("verify_canary should not error"); + assert_eq!(results.len(), 1); + let detail = &results[0].detail; + + assert!( + !results[0].passed, + "a genuinely failing fixture test must redden the gate (fail-closed); \ + got PASS with detail: {detail}" + ); + assert!( + detail.contains(RED_TEST_NAME), + "failing_detail must NAME the failing test (`{RED_TEST_NAME}`) — the \ + #4558 diagnosability fix; got: {detail}" + ); + assert!( + detail.contains("FAILED") || detail.contains("panicked at") || detail.contains("failures:"), + "failing_detail must carry a structured failure marker \ + (FAILED / panicked at / failures:); got: {detail}" + ); + assert!( + !detail.contains("Drop t"), + "failing_detail must not be a truncated progress-spinner fragment \ + (the #4558 `Drop t…` symptom); got: {detail}" + ); +} + +/// Fast, always-run wiring guard (no subprocess): the fixture crate the +/// `#[ignore]`d end-to-end tests point the gate at must exist and be a +/// well-formed standalone package. Keeps the fixture path honest so the +/// dedicated `--ignored` lane never silently no-ops on a moved/renamed fixture. +#[test] +fn fixture_crate_is_present_and_standalone() { + let dir = fixture_manifest_dir(); + let cargo_toml = dir.join("Cargo.toml"); + assert!( + cargo_toml.is_file(), + "fixture Cargo.toml must exist at {}", + cargo_toml.display() + ); + let manifest = std::fs::read_to_string(&cargo_toml).expect("read fixture Cargo.toml"); + // The empty `[workspace]` table pins the fixture as its own workspace root so + // the gate can build it in isolation via `--manifest-path`. + assert!( + manifest.contains("[workspace]"), + "fixture must declare an empty [workspace] so it builds standalone; got:\n{manifest}" + ); + assert!( + dir.join("lib.rs").is_file(), + "fixture lib.rs (the green/red tree) must exist at {}", + dir.join("lib.rs").display() + ); +}