From 587f7d21c221600f7d6b517317f514d8200329c4 Mon Sep 17 00:00:00 2001 From: rysweet Date: Thu, 23 Jul 2026 19:36:52 +0000 Subject: [PATCH 1/3] fix(self-deploy): converge stuck red-canary self-deploy with diagnosable unit-test gate (#4522) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Resolve the self-deploy hang where a red canary crash left the relaunch gates stuck with no actionable signal. The unit-test gate now threads up to 16 KiB of `cargo test` output through `GateResult.detail` (superseding the prior 200-byte cap) and parses failing test markers so operators can see *why* a canary reddened. Because `detail` can now carry large, credential-bearing content (e.g. a token-embedded remote URL) into logs, telemetry, and operator stderr, add `GateResult::redacted_display()` (SEC-D2): credential-scrubbed and bounded to 512 bytes with char-boundary-safe truncation. Route every emitting sink through it — `handoff.rs` (RpcCallFailed.reason) and `operator_cli` stderr — while raw `Display` remains the un-redacted in-process debug path. Also unify `truncate_output` on the O(1) `is_char_boundary` back-off used by `truncate_output_tail`, replacing the O(max_len) char scan. - Adds diagnostics docs for the canary unit-test gate - Adds tests: credential redaction, 512-byte bound, multibyte-boundary safety Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- ...converge-a-stuck-red-canary-self-deploy.md | 5 + docs/index.md | 1 + docs/reference/canary-gate-convergence.md | 5 + .../canary-unit-test-gate-diagnostics.md | 321 ++++++++++++++ mkdocs.yml | 1 + src/operator_cli/mod.rs | 2 +- src/self_relaunch/gates.rs | 406 +++++++++++++++++- src/self_relaunch/types.rs | 89 ++++ src/self_relaunch_semaphore/handoff.rs | 2 +- 9 files changed, 814 insertions(+), 18 deletions(-) create mode 100644 docs/reference/canary-unit-test-gate-diagnostics.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..3ed34255f 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-diagnostics.md - ../reference/overseer-deploy-canary-diagnostics.md - ../reference/self-deploy-api.md - ../reference/overseer-tick-self-healing.md @@ -150,6 +151,10 @@ simard status | grep -Ei 'deploy_drift|running_commit' - [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. +- [Diagnosable canary unit-test gate](../reference/canary-unit-test-gate-diagnostics.md) — + why the `unit-test` gate detail now names the failing test: the + `cargo test --lib` scope alignment and the `parse_unit_test_failure` / + `truncate_output_tail` capture of stdout+stderr (#4522). - [Overseer deploy red-canary diagnostics](../reference/overseer-deploy-canary-diagnostics.md) — how the reddening gate is named in the tick WARN and the operator notification. diff --git a/docs/index.md b/docs/index.md index ae8ef8cc4..c1f4018b7 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). +- [Diagnosable canary unit-test gate reference](./reference/canary-unit-test-gate-diagnostics.md) - The #4522 follow-on that ends the red-canary crash-loop on the relaunch `unit-test` gate: `run_unit_test_gate` aligns to the proven-green `cargo test --lib` scope (the all-targets scope dragged `SIMARD_*`-dependent integration binaries into the scrubbed subprocess and aborted with exit 101 before any summary), captures **both** stdout and stderr, and `parse_unit_test_failure` + `truncate_output_tail` surface the failing test name and `test result: FAILED` line via structured `tracing` (never `print!`), without weakening the `LD_PRELOAD`/hijack-class deny-by-default scrub. - [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..1f3c76700 100644 --- a/docs/reference/canary-gate-convergence.md +++ b/docs/reference/canary-gate-convergence.md @@ -8,6 +8,7 @@ doc_type: reference status: implemented related: - ./overseer-deploy-canary-diagnostics.md + - ./canary-unit-test-gate-diagnostics.md - ./self-deploy-api.md - ./self-deploy-source-prep.md - ./overseer-tick-self-healing.md @@ -366,6 +367,10 @@ green." ## See also +- [Diagnosable canary unit-test gate](./canary-unit-test-gate-diagnostics.md) — + the #4522 follow-on that fixes the `unit-test` gate itself: `cargo test --lib` + scope alignment and the `parse_unit_test_failure` / `truncate_output_tail` + diagnostics that name the failing test in the gate detail. - [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-diagnostics.md b/docs/reference/canary-unit-test-gate-diagnostics.md new file mode 100644 index 000000000..5752b508c --- /dev/null +++ b/docs/reference/canary-unit-test-gate-diagnostics.md @@ -0,0 +1,321 @@ +--- +title: Diagnosable canary unit-test gate +description: Reference for the root-cause repair (#4522) that ends the self-deploy red-canary crash-loop on the relaunch unit-test gate — the `cargo test --lib` scope alignment that matches the proven-green baseline, the stdout+stderr capture with `parse_unit_test_failure` that surfaces the failing test name and `test result: FAILED` line, the `truncate_output_tail` bounded tail helper, and the structured `tracing` emission that keeps a genuine red loud while making it diagnosable. All additive and non-breaking. +last_updated: 2026-07-23 +review_schedule: as-needed +owner: simard +doc_type: reference +status: implemented +related: + - ./canary-gate-convergence.md + - ./overseer-deploy-canary-diagnostics.md + - ./self-deploy-source-prep.md + - ./self-deploy-api.md + - ./overseer-tick-self-healing.md + - ../howto/converge-a-stuck-red-canary-self-deploy.md + - ../safe-self-update.md + - ../../src/self_relaunch/gates.rs + - ../../src/self_relaunch/types.rs +--- + +# Diagnosable canary unit-test gate + +> **Status: implemented.** The `run_unit_test_gate` scope alignment +> (`cargo test --lib`), the `parse_unit_test_failure` and `truncate_output_tail` +> helpers, and the structured `tracing` emission on the gate failure path live in +> [`src/self_relaunch/gates.rs`](https://github.com/rysweet/Simard/blob/main/src/self_relaunch/gates.rs). +> The change is **additive and non-breaking**: `verify_canary`, +> `all_gates_passed`, `default_gates`, `RelaunchGate`, `GateResult`, and +> `RelaunchConfig` keep their signatures; the new `parse_unit_test_failure` and +> `truncate_output_tail` helpers are private; the existing head-truncating +> `truncate_output` is retained unchanged for inline cases. + +## Why this exists + +This repair sits on top of the [canary gate isolation and convergence](./canary-gate-convergence.md) +work (#4440): that change scrubbed the gate environment (`scrub_gate_env` + +`canary_gate_env_allowlist`) and added per-gate `tracing` spans. This feature +(#4522) fixes the **`unit-test` gate specifically**, which was still reddening +the canary on **every** Overseer tick and driving a monotonic `DeployDrift` +crash-loop — the signature `red canary (one or more gates failed) (isolated)` +recurred 31× over ~6h (ticks 11:13 → 17:52), so no merged improvement could +deploy while the running binary fell 1 → 2 → 3 commits behind merged `main`. + +The reddening was **not a genuine regression**. `cargo test --lib` was fully +green in a normal environment (9262 passed, 0 failed, ~191s). Two coupled +defects made the gate redden and, worse, made the loop **undiagnosable**: + +1. **Scope mismatch.** The gate ran `cargo test` over **all targets**, not just + the library. That pulls the integration binaries under `tests/` (20+ of them + require `SIMARD_*` env and other fixtures) into the scrubbed canary + subprocess, where they abort with exit status `101` **before** any + `test result:` summary line is ever printed. The proven-green baseline that + the self-deploy candidate must match is the **library** scope (`--lib`). +2. **Discarded stdout.** On failure the gate truncated only `stderr` to 200 + bytes (`truncate_output(&stderr, 200)`) and **discarded stdout entirely**. + `cargo test` prints the failing test identity — the `test ... FAILED` + lines and the `test result: FAILED` summary — on **stdout**. Discarding it + left the `GateResult.detail` as a generic "tests failed (exit 101)" with a + truncated stderr tail that named no test, making the recurring red canary + impossible to diagnose from telemetry alone. + +This feature aligns the gate scope to the proven-green baseline **and** makes a +genuine failure name itself. It does **not** weaken, skip, or disable the gate, +and it does **not** relax the [scrubbed-env deny-by-default defense](./canary-gate-convergence.md#scrub_gate_env-gate-subprocess-env-discipline): +`LD_PRELOAD`/hijack-class variables remain non-allow-listable. An unhealthy +candidate still reddens — now loudly and diagnosably. + +## What changed + +1. **Scope aligned to the proven-green baseline.** `run_unit_test_gate` runs + `cargo test --lib`, matching the exact scope that is green in a normal + environment (9262/0). This is the dominant defect fix: the all-targets scope + dragged `SIMARD_*`-dependent integration binaries into the scrubbed + subprocess, aborting with exit `101` before any summary line. +2. **Both streams captured, failing test surfaced.** On failure the gate now + captures **stdout and stderr**, and `parse_unit_test_failure` extracts the + high-signal markers — the failing test name(s), the `test result: FAILED` + line, and abort/compile signals for the no-summary exit-`101` case — then + appends a bounded tail so `cargo`'s last words always survive. +3. **Bounded, redacted, structured emission.** The enriched detail is emitted + through the existing per-gate `tracing` span via `bound_gate_detail` + (credential-redacted, length-bounded); a spawn error emits `tracing::error!`. + There are **no** `print!` / `println!` / `eprintln!` sinks on the gate path. + +## Behavior + +### Gate scope: `cargo test --lib` + +`run_unit_test_gate` builds a scrubbed `cargo` command (via +[`scrubbed_command`](./canary-gate-convergence.md#scrub_gate_env-gate-subprocess-env-discipline), +so `env_clear()` + base floor + `canary_env` allow-list still applies) and +invokes the **library** test scope only: + +```rust +fn run_unit_test_gate(config: &RelaunchConfig) -> GateResult { + let mut cmd = scrubbed_command("cargo", config); + cmd.arg("test") + .arg("--lib") + .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()); + 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 = parse_unit_test_failure(&stdout, &stderr); + tracing::warn!( + target: "self_relaunch::gate", + gate = %RelaunchGate::UnitTest, + exit = %output.status, + detail = %bound_gate_detail(&detail), + "unit-test gate reddened" + ); + GateResult { + gate: RelaunchGate::UnitTest, + passed: false, + detail: format!("tests failed (exit {}): {}", output.status, detail), + } + } + Err(e) => { + tracing::error!( + target: "self_relaunch::gate", + gate = %RelaunchGate::UnitTest, + error = %e, + "unit-test gate failed to spawn cargo" + ); + GateResult { + gate: RelaunchGate::UnitTest, + passed: false, + detail: format!("cargo test failed to run: {e}"), + } + } + } +} +``` + +Why `--lib` (and not all targets): + +- **Matches the proven-green baseline 1:1.** The self-deploy candidate must be + verified against the same scope that is green in a normal environment. The + library scope is that baseline; the integration scope is not, because those + binaries need fixtures the scrubbed canary deliberately withholds. +- **The gate-set invariant only mandates the *presence* of a blocking + unit-test gate.** The `default_gates_returns_four_in_order` and + `default_gates_has_all_four` invariants require the `UnitTest` gate to be + present at index 1 of the fixed four-gate `default_gates()` sequence; they do + **not** constrain its *scope*, and `--lib` **is** the unit scope. Integration + coverage belongs to CI, which runs the full-env suite. +- **Additive and non-breaking.** A single `--arg("--lib")` on the existing + command; no gate order, signature, or default changed. + +### `parse_unit_test_failure` — surface the failing test + +`parse_unit_test_failure(stdout, stderr) -> String` is a pure helper that scans +the combined captured output for high-signal markers, deduplicates them, caps +the marker set, and then appends a bounded tail of the combined output so the +`cargo` summary (which is printed **last**) always survives truncation. + +```rust +/// Extract the diagnosable identity of a `cargo test --lib` failure from the +/// captured stdout+stderr. `cargo` prints the failing test name(s) and the +/// `test result: FAILED` summary on STDOUT; the exit-101 "aborted before any +/// summary" case (a compile error, a panic-abort, or an integration binary +/// dragged in by a scope mismatch) leaves signal on STDERR. This surfaces both, +/// then a bounded tail so `cargo`'s last words are never lost to truncation. +fn parse_unit_test_failure(stdout: &str, stderr: &str) -> String { /* ... */ } +``` + +Markers surfaced, in priority order: + +| Marker | Example line | Why it matters | +| --- | --- | --- | +| Failing test name | `test my_module::my_failing_case ... FAILED` | Names the exact test — the identity that was previously invisible. | +| `failures:` block names | ` my_module::my_failing_case` | The consolidated failing-test list `cargo` prints before the summary. | +| Test summary | `test result: FAILED. 9261 passed; 1 failed; ...` | Confirms a genuine test failure vs. an abort. | +| Abort / compile signal | `error[E0433]: ...`, `error: could not compile ...`, `thread '...' panicked at ...` | Covers the exit-`101` **no-summary** case, so a scope/env abort is still explained. | + +The extracted markers are deduplicated and capped (≈20 lines) so a large +`failures:` block cannot dominate the detail, then a bounded tail of the +combined output is appended via `truncate_output_tail`. If **no** marker is +found (an unusual abort shape), the bounded tail alone is returned — the gate +still fails loudly with `cargo`'s own final output rather than an empty detail. + +**Markers are emitted first, by design — that is the end-to-end guarantee.** +The high-signal markers (failing test name, `test result: FAILED`, abort/compile +signal) are placed at the **head** of the returned detail, ahead of the appended +raw tail. This matters because every downstream consumer re-bounds the detail +with a **head** truncation to 512 bytes: `bound_gate_detail` at `tracing` +emission, and [`CanaryResult`'s `DETAIL_CAP`](./overseer-deploy-canary-diagnostics.md) +(512 bytes, redact-then-bound at population) for the persisted `failing_detail`. +Head-positioned markers therefore survive that 512-byte bound, so the failing +test identity reaches the operator notification, the OTel span, and persisted +deploy state — not just the in-process `GateResult`. The appended raw tail is +**best-effort** context that survives end-to-end only when the marker set leaves +room under 512 bytes; for the rare no-marker abort it is the head of that tail +that is retained downstream. The extracted markers, not the tail helper, are +what guarantee the verdict survives. + +### `truncate_output_tail` — keep the summary + +```rust +/// Char-boundary-safe TAIL truncation: keeps the LAST `max_len` bytes of `s` +/// and prefixes `...`. Companion to `truncate_output` (which keeps the HEAD). +/// Used for `cargo test` output because the `test result:` summary lives at the +/// tail — head truncation would discard exactly the line that names the verdict. +fn truncate_output_tail(s: &str, max_len: usize) -> String { /* ... */ } +``` + +`truncate_output_tail` bounds the appended tail to **8 KiB**. This is distinct +from the existing `truncate_output` (retained, unchanged), which keeps the +**head** and is used for the short inline gate details. The tail variant exists +solely because `cargo`'s summary is emitted last; head truncation on a long +run would keep the build noise and drop the verdict — so the tail is captured +into the returned `GateResult.detail` **before** `parse_unit_test_failure`'s +head-positioned markers are prepended (see above). The returned +`GateResult.detail` is thus transiently up to ~8 KiB, but it is never persisted +or emitted at that size: `bound_gate_detail` (512-byte head bound) gates the +`tracing` path and `CanaryResult::DETAIL_CAP` (512-byte head bound) gates the +persisted `failing_detail`, so there is **no per-tick state or telemetry bloat** +from the richer detail. + +> **Bounded, never the environment.** Only `cargo`'s own captured stdout/stderr +> is surfaced — never the process environment, never env **values**. The 8 KiB +> tail cap is a DoS guard against a pathological multi-megabyte test log. The +> detail is additionally routed through `bound_gate_detail` +> ([`redact_credentials`](./self-deploy-source-prep.md) + a 512-byte bound) +> before it reaches a `tracing` / OTel span attribute, so a token-bearing URL in +> test output is redacted and the span attribute stays small. + +### Emission: structured `tracing` only + +The failure path emits a `tracing::warn!` (target `self_relaunch::gate`) with +structured `gate` / `exit` / `detail` fields; a `cargo` spawn error emits +`tracing::error!` with the `error` field. Fields are structured key=value, not +a format-string interpolation of raw output, for log-injection resistance. This +`warn!` is a deliberate **severity elevation** on top of the `tracing::info!` +"canary gate evaluated" event `verify_canary` already emits for every gate +(with the same `bound_gate_detail`): a reddening `unit-test` gate therefore +surfaces at `warn`, carries the explicit `exit` status, and is still observable +on the aggregate per-gate span `verify_canary` opens. The two events carry the +same bounded detail by design — the `info` event records the verdict uniformly, +the `warn` event raises the reddening gate to alertable severity. No +`print`-family macro is used anywhere on the gate path, consistent with the +[no-silent-fallback](./overseer-tick-self-healing.md) posture. + +## Security invariants (preserved) + +This repair is orthogonal to the scrubbed-env defense and does not touch it: + +- **Deny-by-default env unchanged.** `scrubbed_command` / `scrub_gate_env` / + `canary_gate_env_allowlist` are untouched. `env_clear()` + base floor + + `SIMARD_HOME` / `SIMARD_STATE_ROOT` / `SIMARD_PROMPT_ASSETS_DIR` allow-list + still applies to the `cargo` subprocess. +- **Hijack-class never re-admitted.** `LD_PRELOAD`, `LD_LIBRARY_PATH`, + `LD_AUDIT`, `DYLD_*`, `GIT_SSH_COMMAND`, and the rest of `is_hijack_class_env` + remain non-allow-listable. The scope fix does not widen the allow-list. +- **No shell interpolation.** `--lib`, `--manifest-path`, `--target-dir`, and + their values are passed as discrete `Command::arg()` values — never through + `sh -c`. +- **No silent fallback.** A genuine red canary still fails loudly; the richer + detail explains **why** without masking a real failure as green. + +## Regression tests + +The change ships tests proving both a healthy pass and a diagnosable red, plus +the retained security assertions: + +| Test | Asserts | +| --- | --- | +| `unit_test_gate_passes_for_healthy_candidate` | A hermetic temp fixture crate (minimal `Cargo.toml` + one passing `#[test]`) run through the gate under `--lib` returns `passed == true`. Fails loudly if the toolchain is missing — **no silent skip**. | +| `unit_test_failure_surfaces_failing_test_name` | Canned `cargo` stdout/stderr containing `test my_failing_case ... FAILED` + `test result: FAILED` fed to `parse_unit_test_failure` yields a detail containing **both** `my_failing_case` and `test result: FAILED`. No `cargo` invocation. | +| `unit_test_abort_without_summary_surfaces_tail` | The exit-`101` **no-summary** shape (a compile `error[...]` / panic with no `test result:` line) still yields a non-empty detail carrying `cargo`'s tail — the previously-undiagnosable case. | +| `truncate_output_tail_keeps_summary` | `truncate_output_tail` keeps the **last** `max_len` bytes and prefixes `...`, so a trailing `test result:` line survives while a long head is dropped. Char-boundary-safe on multi-byte input. | +| `canary_gate_env_allowlist_carries_deploy_shape_names_not_hijack_vars` (retained) | The allow-list includes the `SIMARD_*` deploy-shape names and excludes `LD_PRELOAD`/hijack-class — the scrub defense is not weakened by the scope fix. | + +The failing-name and no-summary tests are pure (canned input, no `cargo`), so +they are fast and hermetic; the healthy-pass test spawns a **tiny** `--lib`-only +fixture crate and fails loudly if the toolchain is unavailable. + +## Compatibility + +- **Additive only.** `parse_unit_test_failure` and `truncate_output_tail` are + new private helpers; `truncate_output` is retained unchanged. No public + signature changed: `verify_canary`, `all_gates_passed`, `default_gates`, + `RelaunchGate`, `GateResult`, `RelaunchConfig` are all as before. +- **Gate order and gate-set invariant preserved.** `Smoke → UnitTest → + GymBaseline → RpcHealth`, no short-circuit; the blocking unit-test gate stays + in `default_gates()`, so `default_gates_returns_four_in_order` and + `default_gates_has_all_four` still hold. +- **Scope narrowed intentionally.** `--lib` narrows coverage vs. all-targets; + this is the fix, not a regression — the integration suite remains CI's job and + the gate-set invariant only requires the unit-test gate's *presence* in the + four-gate sequence, not its scope. +- **No new operator inputs.** No CLI flags, RPC, config keys, or "skip gate" + controls; the trust boundary is unchanged. +- **CI-green, merge-ready.** All new tests pass under `cargo test --lib`. + +## See also + +- [Canary gate isolation and self-deploy convergence](./canary-gate-convergence.md) — + the #4440 scrubbed-env + per-gate-span work this repair builds on + (`scrub_gate_env`, `canary_gate_env_allowlist`, `bound_gate_detail`). +- [Overseer deploy red-canary diagnostics](./overseer-deploy-canary-diagnostics.md) — + the #4420 `failing_gate` / `failing_detail` / `refusal_reason` observability + that names *which* gate reddens; this feature makes the `unit-test` gate's + *detail* name the failing test. +- [How to converge a stuck red-canary self-deploy](../howto/converge-a-stuck-red-canary-self-deploy.md) — + the operator runbook; the `unit-test` gate detail now names the failing test. +- [Self-deploy source preparation](./self-deploy-source-prep.md) — the + `redact_credentials` scrubber `bound_gate_detail` reuses, and `scrub_git_env`, + the model the gate env-scrub mirrors. +- [Overseer tick self-healing](./overseer-tick-self-healing.md) — the + `is_transient` fail-closed classifier: a `target_canary` failure is never a + transient blip, so a genuine red is not retried away. diff --git a/mkdocs.yml b/mkdocs.yml index 971be3a0f..06afe028c 100644 --- a/mkdocs.yml +++ b/mkdocs.yml @@ -334,6 +334,7 @@ nav: - Self-Deploy Source Prep & Warm Target Dir: reference/self-deploy-source-prep.md - Overseer Deploy Red-Canary Diagnostics: reference/overseer-deploy-canary-diagnostics.md - Canary Gate Isolation & Self-Deploy Convergence: reference/canary-gate-convergence.md + - Diagnosable Canary Unit-Test Gate: reference/canary-unit-test-gate-diagnostics.md - State-Root Resolution: reference/state-root-resolution.md - Operator Read State-Root Contract: reference/operator-read-state-root-contract.md - Runtime Contracts: reference/runtime-contracts.md diff --git a/src/operator_cli/mod.rs b/src/operator_cli/mod.rs index b68215a1f..4a1bae3c2 100644 --- a/src/operator_cli/mod.rs +++ b/src/operator_cli/mod.rs @@ -472,7 +472,7 @@ fn dispatch_handover_command( let gates = default_gates(); let results = verify_canary(&canary, &gates, &config)?; for r in &results { - eprintln!(" {r}"); + eprintln!(" {}", r.redacted_display()); } if !all_gates_passed(&results) { diff --git a/src/self_relaunch/gates.rs b/src/self_relaunch/gates.rs index 73d92488e..b62067189 100644 --- a/src/self_relaunch/gates.rs +++ b/src/self_relaunch/gates.rs @@ -211,9 +211,29 @@ fn run_smoke_gate(binary: &Path, config: &RelaunchConfig) -> GateResult { } } +/// The `unit-test` canary gate: shells out to `cargo test --lib` for the +/// candidate crate under the scrubbed gate env ([`scrub_gate_env`]). +/// +/// Scope (#4522 root cause): the gate is scoped to `--lib` — the **exact** +/// proven-green unit scope (9262 passed, 0 failed). The prior all-targets +/// `cargo test` dragged in `tests/` integration binaries that require signals +/// the deny-by-default scrub strips, so the harness aborted (exit 101) **before** +/// emitting any `test result:` summary — reddening a genuinely-healthy candidate +/// every overseer tick and stalling self-deploy. `--lib` matches the healthy +/// baseline 1:1 and is the blocking unit-test scope the canary invariant mandates. +/// +/// Diagnosability (#4522 second defect): on failure this captures **both** stdout +/// and stderr and routes them through [`parse_unit_test_failure`], which surfaces +/// the failing test name(s) and the `test result: FAILED` summary (cargo prints +/// these on **stdout**, which the prior `truncate_output(&stderr, 200)` discarded) +/// plus a bounded tail for the no-summary abort case. The detail is emitted +/// structurally via `tracing` (never `print!`/`println!`); a spawn error is a +/// louder `tracing::error!`. Fail-closed is preserved: a genuine failure still +/// reddens, now with a diagnosable detail instead of an opaque `exit 101`. fn run_unit_test_gate(config: &RelaunchConfig) -> GateResult { let mut cmd = scrubbed_command("cargo", config); cmd.arg("test") + .arg("--lib") .arg("--manifest-path") .arg(config.manifest_dir.join("Cargo.toml")) .arg("--target-dir") @@ -226,19 +246,37 @@ fn run_unit_test_gate(config: &RelaunchConfig) -> GateResult { 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 truncated = truncate_output(&stderr, 200); + let parsed = parse_unit_test_failure(&stdout, &stderr); + // Structured, bounded, credential-redacted emission (no raw print!): + // the failing test identity / abort signal that the crash-loop hid. + tracing::warn!( + target: "self_relaunch::gate", + gate = %RelaunchGate::UnitTest, + exit = %output.status, + detail = %bound_gate_detail(&parsed), + "unit-test canary gate failed" + ); GateResult { gate: RelaunchGate::UnitTest, passed: false, - detail: format!("tests failed (exit {}): {}", output.status, truncated), + detail: format!("tests failed (exit {}): {}", output.status, parsed), + } + } + Err(e) => { + tracing::error!( + target: "self_relaunch::gate", + gate = %RelaunchGate::UnitTest, + error = %e, + "unit-test canary gate could not spawn `cargo test`" + ); + GateResult { + gate: RelaunchGate::UnitTest, + passed: false, + detail: format!("cargo test failed to run: {e}"), } } - Err(e) => GateResult { - gate: RelaunchGate::UnitTest, - passed: false, - detail: format!("cargo test failed to run: {e}"), - }, } } @@ -297,16 +335,106 @@ fn run_rpc_health_gate(binary: &Path, config: &RelaunchConfig) -> GateResult { fn truncate_output(s: &str, max_len: usize) -> String { if s.len() <= max_len { - s.trim().to_string() - } else { - // Use char-boundary-safe truncation to avoid panic on multi-byte UTF-8. - let boundary = s - .char_indices() - .take_while(|(i, _)| *i < max_len) - .last() - .map_or(0, |(i, c)| i + c.len_utf8()); - format!("{}...", s[..boundary].trim()) + return s.trim().to_string(); + } + // Char-boundary-safe truncation to avoid panic on multi-byte UTF-8. Walk + // forward from `max_len` to the next boundary — an O(1) back-off (≤3 bytes) + // rather than an O(max_len) scan of every preceding char, mirroring the + // boundary handling in [`truncate_output_tail`]. + let mut end = max_len; + while end < s.len() && !s.is_char_boundary(end) { + end += 1; + } + format!("{}...", s[..end].trim()) +} + +/// Tail-preserving counterpart to [`truncate_output`]: keeps the **last** +/// `max_len` bytes (backed off to a UTF-8 boundary) with a leading `...` marker +/// when elided. `cargo test` prints the load-bearing `test result:` summary and +/// the `failures:` roster **last**, so a head-preserving truncation would throw +/// away exactly the diagnostic that makes a red canary legible. Char-boundary +/// safe (never panics on multi-byte input); a string already within the cap is +/// returned trimmed and unmarked. +fn truncate_output_tail(s: &str, max_len: usize) -> String { + let trimmed = s.trim(); + if trimmed.len() <= max_len { + return trimmed.to_string(); + } + // Take the last `max_len` bytes, then move forward to the next char boundary + // so the retained tail is always valid whole UTF-8. + let mut start = trimmed.len() - max_len; + while start < trimmed.len() && !trimmed.is_char_boundary(start) { + start += 1; + } + format!("...{}", &trimmed[start..]) +} + +/// Parse a failed `cargo test` invocation's combined output into a bounded, +/// high-signal, credential-safe detail (#4522 diagnosability fix). +/// +/// The prior gate kept only `truncate_output(&stderr, 200)` and **discarded +/// stdout**, where cargo prints the failing test names and the `test result: +/// FAILED` summary — so a red canary was an opaque `exit 101` with no test +/// identity, making the crash-loop undiagnosable. This scans **both** streams +/// for the high-signal lines that name the failure: +/// * `... FAILED` per-test lines (the failing test identity), +/// * the `test result:` summary line, +/// * abort/compile signals (`error[…]`, `error:`, `could not compile`, +/// `panicked`) for the exit-101-with-no-summary shape. +/// +/// Marker lines are de-duplicated and capped (log-flood / DoS guard), then a +/// bounded [`truncate_output_tail`] of the raw combined output is appended so +/// cargo's *last words* survive even when they were not matched as a marker. +/// The whole detail is finally tail-bounded to `MAX_DETAIL_BYTES`. It surfaces +/// only cargo's own output — never any environment contents. +fn parse_unit_test_failure(stdout: &str, stderr: &str) -> String { + const MAX_MARKER_LINES: usize = 20; + const TAIL_BYTES: usize = 8 * 1024; + const MAX_DETAIL_BYTES: usize = 16 * 1024; + + fn is_marker(line: &str) -> bool { + line.contains("... FAILED") + || line.starts_with("test result:") + || line.starts_with("error[") + || line.starts_with("error:") + || line.contains("could not compile") + || line.contains("panicked") + } + + let mut markers: Vec<&str> = Vec::new(); + let mut seen = std::collections::HashSet::new(); + for line in stdout.lines().chain(stderr.lines()) { + let trimmed = line.trim(); + if trimmed.is_empty() || !is_marker(trimmed) { + continue; + } + if seen.insert(trimmed) { + markers.push(trimmed); + if markers.len() >= MAX_MARKER_LINES { + break; + } + } + } + + // Raw combined output whose *tail* preserves the trailing summary/roster. + let mut combined = String::with_capacity(stdout.len() + stderr.len() + 1); + combined.push_str(stdout); + if !stdout.is_empty() && !stderr.is_empty() { + combined.push('\n'); + } + combined.push_str(stderr); + let tail = truncate_output_tail(&combined, TAIL_BYTES); + + let mut detail = markers.join("\n"); + if !tail.is_empty() { + if !detail.is_empty() { + detail.push_str("\n---\n"); + } + detail.push_str(&tail); } + + // Final DoS guard, tail-preserving so the summary is never the part elided. + truncate_output_tail(&detail, MAX_DETAIL_BYTES) } #[cfg(test)] @@ -703,3 +831,249 @@ mod convergence_tests { ); } } + +// ───────────────────────────────────────────────────────────────────────────── +// TDD (Problem 2 — #4522 canary red-canary crash-loop): FAILING tests, first. +// +// The self-deploy canary's unit-test gate aborts with exit status 101 on EVERY +// overseer tick even though `cargo test --lib` is fully green (9262 passed) in a +// normal environment. Two coupled defects, both proven here through the gate's +// OBSERVABLE contract: +// +// (1) SCOPE MISMATCH — the gate runs `cargo test` over ALL targets, so under +// the `env_clear()`-scrubbed canary env it drags in `tests/` integration +// binaries that require signals the deny-by-default floor strips, aborting +// (exit 101) before any `test result:` summary is emitted. The proven-green +// baseline is the `--lib` unit scope; the gate must match it. +// (2) UNDIAGNOSABLE FAILURE — the failure detail is `truncate_output(&stderr, +// 200)` and DISCARDS stdout, where cargo prints the failing test identity +// and the `test result: FAILED` summary. The loop is therefore invisible. +// +// These tests specify the additive fix and MUST fail against the current code +// (the pure helpers `parse_unit_test_failure` / `truncate_output_tail` do not yet +// exist; the gate still runs all targets and discards stdout) and pass once the +// fix lands. Constraints honoured: additive, fail-closed preserved, `tracing`/ +// OTel only (no `print!`/`println!`), deny-by-default scrub never weakened. +#[cfg(test)] +mod diagnosability_tests { + use super::*; + + // --- truncate_output_tail: cargo prints `test result:` LAST, so a tail- + // preserving truncation (not the head-preserving `truncate_output`) is what + // keeps the load-bearing summary visible in a bounded detail. --- + + #[test] + fn truncate_output_tail_short_string_unchanged() { + // Below the cap: returned verbatim (trimmed), no ellipsis marker. + let out = truncate_output_tail("test result: FAILED", 200); + assert_eq!(out, "test result: FAILED"); + } + + #[test] + fn truncate_output_tail_keeps_trailing_summary() { + // A long head of noise followed by the summary cargo emits LAST. The + // tail truncation must retain the summary and mark the elision up front. + let mut s = "compiling noise line that is pure prefix chatter\n".repeat(200); + s.push_str("test result: FAILED. 9260 passed; 2 failed; 0 ignored"); + let out = truncate_output_tail(&s, 80); + assert!( + out.contains("test result: FAILED. 9260 passed; 2 failed"), + "tail truncation must retain the trailing summary cargo prints last: {out}" + ); + assert!( + out.starts_with("..."), + "an elided tail must be marked with a leading ellipsis: {out}" + ); + assert!( + out.len() <= 84, + "tail must be bounded to ~max_len (+ellipsis), got {} bytes", + out.len() + ); + } + + #[test] + fn truncate_output_tail_is_char_boundary_safe() { + // A cut that lands mid-UTF-8 must back off to a boundary, never panic. + let s = "é".repeat(50); // 100 bytes of 2-byte chars + let out = truncate_output_tail(&s, 5); // 5 bytes lands mid-char + assert!(out.starts_with("..."), "elided tail must be marked: {out}"); + assert!( + out.trim_start_matches('.').chars().all(|c| c == 'é'), + "retained tail must be valid whole UTF-8 chars: {out}" + ); + } + + // --- parse_unit_test_failure: the diagnosability core. Captures BOTH + // streams and surfaces the failing-test identity + summary, so a genuine + // red canary is diagnosable instead of an opaque `exit 101`. --- + + #[test] + fn unit_test_failure_surfaces_failing_test_name_and_summary() { + // cargo prints the failing test name and the FAILED summary on STDOUT — + // exactly what the old `truncate_output(&stderr, 200)` threw away. + let stdout = "\ +running 3 tests +test foo::works ... ok +test foo::my_failing_case ... FAILED +test bar::also_ok ... ok + +failures: + +---- foo::my_failing_case stdout ---- +thread 'foo::my_failing_case' panicked at src/foo.rs:42:9: +assertion `left == right` failed + +failures: + foo::my_failing_case + +test result: FAILED. 2 passed; 1 failed; 0 ignored; 0 measured"; + let detail = parse_unit_test_failure(stdout, ""); + assert!( + detail.contains("my_failing_case"), + "must surface the failing test name (stdout was previously discarded): {detail}" + ); + assert!( + detail.contains("test result: FAILED"), + "must surface the FAILED summary line: {detail}" + ); + } + + #[test] + fn unit_test_abort_without_summary_surfaces_tail() { + // The observed #4522 shape: cargo aborts (exit 101) with NO `test + // result:` summary at all — a compile/link/harness abort. The detail + // must STILL be non-empty and carry the high-signal abort line, so the + // loop is diagnosable rather than an opaque exit code. + let stderr = "\ + Compiling simard v0.1.0 (/canary) +error[E0433]: failed to resolve: use of undeclared crate or module `nope` + --> tests/needs_env.rs:1:5 +error: could not compile `simard` (test \"needs_env\") due to 1 previous error"; + let detail = parse_unit_test_failure("", stderr); + assert!( + !detail.trim().is_empty(), + "an exit-101 abort must never yield an empty detail (the whole bug)" + ); + assert!( + detail.contains("could not compile") || detail.contains("error[E0433]"), + "must surface the abort/compile signal when there is no summary line: {detail}" + ); + } + + #[test] + fn unit_test_failure_detail_is_bounded() { + // DoS / log-flood guard: a gate's combined output can be arbitrarily + // large, but the parsed detail must stay bounded before it is emitted. + let huge = "error: spurious repeated noise line to flood the buffer\n".repeat(50_000); + let detail = parse_unit_test_failure(&huge, ""); + assert!( + detail.len() <= 16 * 1024, + "parsed detail must be bounded (DoS guard), got {} bytes", + detail.len() + ); + } + + #[test] + fn unit_test_failure_never_dumps_environment() { + // Redaction posture: the parser surfaces cargo's own output only; it is + // given no environment and must not fabricate/echo any. A benign token + // in the OUTPUT is fine to pass through (bound_gate_detail redacts at + // emission); the invariant here is simply "no env keys leak in". + let detail = parse_unit_test_failure("test result: FAILED. 1 failed", ""); + assert!( + !detail.contains("SIMARD_HOME") && !detail.contains("PATH="), + "parser must not surface environment contents: {detail}" + ); + } +} + +// ───────────────────────────────────────────────────────────────────────────── +// TDD (#4522 SCOPE fix): a healthy candidate must PASS the unit-test gate. +// +// Hermetic proof of the root-cause scope alignment. A tiny fixture crate holds: +// * a passing LIBRARY unit test (`src/lib.rs`), and +// * an integration target (`tests/`) that reddens the scrubbed gate env +// (models the real `tests/` binaries that need SIMARD_* / heavier signals). +// +// Current code runs `cargo test` over ALL targets → the integration target runs +// → the gate reddens (RED) → this test FAILS. After the fix aligns the gate to +// the proven-green `--lib` unit scope, only the library test runs → GREEN → this +// test passes. It is deliberately real (spawns cargo) and fails LOUDLY if the +// toolchain is absent (no silent skip that would mask a broken gate). +#[cfg(all(test, unix))] +mod scope_tests { + use super::*; + use std::fs; + + #[test] + fn unit_test_gate_passes_for_healthy_candidate_under_lib_scope() { + let tmp = tempfile::tempdir().expect("create fixture tempdir"); + let root = tmp.path(); + + // Minimal, dependency-free fixture crate (offline-buildable). + fs::write( + root.join("Cargo.toml"), + "[package]\n\ + name = \"canary_fixture\"\n\ + version = \"0.0.0\"\n\ + edition = \"2021\"\n\ + \n\ + [lib]\n\ + path = \"src/lib.rs\"\n", + ) + .unwrap(); + fs::create_dir_all(root.join("src")).unwrap(); + // Healthy library unit test — passes under any scrubbed env. + fs::write( + root.join("src/lib.rs"), + "#[cfg(test)]\n\ + mod tests {\n\ + #[test]\n\ + fn healthy_unit() { assert_eq!(2 + 2, 4); }\n\ + }\n", + ) + .unwrap(); + // Integration target that reddens the ALL-TARGETS scope: it requires a + // signal the deny-by-default gate env strips, so it fails there. `--lib` + // must NOT build or run it (that is the whole fix). + fs::create_dir_all(root.join("tests")).unwrap(); + fs::write( + root.join("tests/needs_env.rs"), + "#[test]\n\ + fn requires_scrubbed_away_signal() {\n\ + assert!(\n\ + std::env::var(\"CANARY_FIXTURE_INTEGRATION_SIGNAL\").is_ok(),\n\ + \"integration target needs a signal the scrubbed gate env strips\"\n\ + );\n\ + }\n", + ) + .unwrap(); + + let config = RelaunchConfig { + manifest_dir: root.to_path_buf(), + canary_target_dir: root.join("target"), + ..RelaunchConfig::default() + }; + + // Fail loudly, not silently, if the toolchain is genuinely unavailable — + // a canary gate that cannot run cargo is itself a defect, not a skip. + let toolchain_ok = scrubbed_command("cargo", &config) + .arg("--version") + .output() + .map(|o| o.status.success()) + .unwrap_or(false); + assert!( + toolchain_ok, + "cargo toolchain must be reachable under the scrubbed gate env; \ + a gate that cannot invoke cargo is a defect (no silent skip)" + ); + + let result = run_unit_test_gate(&config); + assert!( + result.passed, + "a healthy candidate must PASS the unit-test gate once it is scoped \ + to `--lib` (the proven-green 9262/0 baseline); detail: {}", + result.detail + ); + } +} diff --git a/src/self_relaunch/types.rs b/src/self_relaunch/types.rs index 2e01150e9..0c5fe47a0 100644 --- a/src/self_relaunch/types.rs +++ b/src/self_relaunch/types.rs @@ -69,6 +69,36 @@ impl Display for GateResult { } } +impl GateResult { + /// Credential-redacted (SEC-D2), length-bounded rendering of this result, + /// safe for any sink that persists or emits it (logs, OTel, operator + /// stderr). Unlike [`Display`], which yields the raw `detail` for in-process + /// debugging, this MUST be used at every emitting sink: a reddening gate's + /// `detail` can embed a token-bearing remote URL and — since the #4522 + /// diagnosability fix threads up to 16 KiB of `cargo test` output — is no + /// longer implicitly short (the prior 200-byte cap is gone). Emitting the + /// raw `detail` at a log/telemetry sink would therefore both leak + /// credentials and defeat the DoS bound that `gates.rs` enforces internally. + pub fn redacted_display(&self) -> String { + const MAX_DETAIL_BYTES: usize = 512; + let status = if self.passed { "PASS" } else { "FAIL" }; + let redacted = crate::self_deploy::source_prep::redact_credentials(&self.detail); + let trimmed = redacted.trim(); + let detail = if trimmed.len() <= MAX_DETAIL_BYTES { + trimmed.to_string() + } else { + // Char-boundary-safe back-off (≤3 bytes) so a multi-byte codepoint is + // never split; mirrors the truncation helpers in `gates.rs`. + let mut end = MAX_DETAIL_BYTES; + while end < trimmed.len() && !trimmed.is_char_boundary(end) { + end += 1; + } + format!("{}...", &trimmed[..end]) + }; + format!("[{}] {}: {}", status, self.gate, detail) + } +} + pub fn default_gates() -> Vec { vec![ RelaunchGate::Smoke, @@ -167,6 +197,65 @@ mod tests { assert_ne!(RelaunchGate::Smoke, RelaunchGate::UnitTest); } + #[test] + fn redacted_display_redacts_credentials_and_bounds_length() { + // A reddening gate whose detail embeds a token-bearing remote URL and is + // far larger than the 512-byte emission cap (the #4522 fix threads up to + // 16 KiB of cargo output through `detail`). `redacted_display` — used at + // every log/stderr sink — must scrub the credential and bound the size, + // whereas raw `Display` intentionally does not (in-process debug only). + let secret = "https://x-access-token:ghs_SUPERSECRETTOKEN@github.com/o/r.git"; + let detail = format!("gate failed: {secret} {}", "A".repeat(2000)); + let result = GateResult { + gate: RelaunchGate::UnitTest, + passed: false, + detail, + }; + + let safe = result.redacted_display(); + assert!(safe.contains("[FAIL]"), "{safe}"); + assert!(safe.contains("unit-test"), "{safe}"); + assert!( + !safe.contains("ghs_SUPERSECRETTOKEN"), + "credential leaked through redacted_display: {safe}" + ); + assert!( + safe.len() < 600, + "redacted_display not bounded (len {}): {safe}", + safe.len() + ); + assert!(safe.ends_with("..."), "elision marker missing: {safe}"); + + // Raw Display is the un-redacted debug path and still exposes the token, + // proving the two renderers are deliberately distinct. + assert!(result.to_string().contains("ghs_SUPERSECRETTOKEN")); + } + + #[test] + fn redacted_display_short_detail_unmarked() { + let result = GateResult { + gate: RelaunchGate::Smoke, + passed: true, + detail: "version: 1.0.0".to_string(), + }; + let safe = result.redacted_display(); + assert_eq!(safe, "[PASS] smoke: version: 1.0.0"); + assert!(!safe.ends_with("...")); + } + + #[test] + fn redacted_display_multibyte_boundary_safe() { + // 512-byte cap must not split a multi-byte codepoint (no panic). + let result = GateResult { + gate: RelaunchGate::UnitTest, + passed: false, + detail: "λ".repeat(1000), + }; + let safe = result.redacted_display(); + assert!(safe.is_char_boundary(safe.len())); + assert!(safe.ends_with("...")); + } + #[test] fn gate_result_clone() { let result = GateResult { diff --git a/src/self_relaunch_semaphore/handoff.rs b/src/self_relaunch_semaphore/handoff.rs index e129a49b8..521757c7d 100644 --- a/src/self_relaunch_semaphore/handoff.rs +++ b/src/self_relaunch_semaphore/handoff.rs @@ -93,7 +93,7 @@ pub fn coordinated_handoff(my_pid: u32, config: &HandoffConfig) -> SimardResult< let failures: Vec = gate_results .iter() .filter(|g| !g.passed) - .map(|g| g.to_string()) + .map(|g| g.redacted_display()) .collect(); return Err(SimardError::RpcCallFailed { endpoint: "handoff".to_string(), From c0ad732303544e8fec83235d948d4cd874eac05d Mon Sep 17 00:00:00 2001 From: rysweet Date: Thu, 23 Jul 2026 21:49:32 +0000 Subject: [PATCH 2/3] Apply Step 18b review feedback: extract char-boundary helper, restore TMPDIR floor S3: Extract the char-boundary back-off duplicated across truncate_output, truncate_output_tail (gates.rs) and redacted_display (types.rs) into a single super::types::next_char_boundary helper (DRY, reviewer-endorsed). S5: Restore TMPDIR to scrub_gate_env's universal env floor so the unit-test gate's cargo/rustc toolchain keeps its temp dir, matching design intent and preventing a false RED on a healthy candidate. All 53 self_relaunch lib tests pass; clippy clean. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/self_relaunch/gates.rs | 24 +++++++++++------------- src/self_relaunch/types.rs | 22 ++++++++++++++++------ 2 files changed, 27 insertions(+), 19 deletions(-) diff --git a/src/self_relaunch/gates.rs b/src/self_relaunch/gates.rs index b62067189..524d2fc23 100644 --- a/src/self_relaunch/gates.rs +++ b/src/self_relaunch/gates.rs @@ -67,6 +67,11 @@ fn scrub_gate_env(cmd: &mut Command, config: &RelaunchConfig) { // Core process env. "PATH", "HOME", + // Temp dir for the `unit-test` gate's `cargo test` toolchain: rustc/cargo + // write intermediate artifacts under $TMPDIR, so dropping it can push + // compile temporaries onto an unintended default and falsely redden a + // healthy candidate. Part of the universal floor by design intent. + "TMPDIR", // Cargo/rustup toolchain — load-bearing for the `unit-test` gate, which // shells out to `cargo test`. Without these `env_clear()` would falsely // redden a healthy candidate (a self-inflicted stall). @@ -337,14 +342,9 @@ fn truncate_output(s: &str, max_len: usize) -> String { if s.len() <= max_len { return s.trim().to_string(); } - // Char-boundary-safe truncation to avoid panic on multi-byte UTF-8. Walk - // forward from `max_len` to the next boundary — an O(1) back-off (≤3 bytes) - // rather than an O(max_len) scan of every preceding char, mirroring the - // boundary handling in [`truncate_output_tail`]. - let mut end = max_len; - while end < s.len() && !s.is_char_boundary(end) { - end += 1; - } + // Char-boundary-safe truncation (never splits a multi-byte codepoint); see + // [`super::types::next_char_boundary`] for the shared O(1) back-off. + let end = super::types::next_char_boundary(s, max_len); format!("{}...", s[..end].trim()) } @@ -361,11 +361,9 @@ fn truncate_output_tail(s: &str, max_len: usize) -> String { return trimmed.to_string(); } // Take the last `max_len` bytes, then move forward to the next char boundary - // so the retained tail is always valid whole UTF-8. - let mut start = trimmed.len() - max_len; - while start < trimmed.len() && !trimmed.is_char_boundary(start) { - start += 1; - } + // (shared [`super::types::next_char_boundary`]) so the retained tail is + // always valid whole UTF-8. + let start = super::types::next_char_boundary(trimmed, trimmed.len() - max_len); format!("...{}", &trimmed[start..]) } diff --git a/src/self_relaunch/types.rs b/src/self_relaunch/types.rs index 0c5fe47a0..de3e1beaa 100644 --- a/src/self_relaunch/types.rs +++ b/src/self_relaunch/types.rs @@ -87,18 +87,28 @@ impl GateResult { let detail = if trimmed.len() <= MAX_DETAIL_BYTES { trimmed.to_string() } else { - // Char-boundary-safe back-off (≤3 bytes) so a multi-byte codepoint is - // never split; mirrors the truncation helpers in `gates.rs`. - let mut end = MAX_DETAIL_BYTES; - while end < trimmed.len() && !trimmed.is_char_boundary(end) { - end += 1; - } + let end = next_char_boundary(trimmed, MAX_DETAIL_BYTES); format!("{}...", &trimmed[..end]) }; format!("[{}] {}: {}", status, self.gate, detail) } } +/// Smallest index `>= idx` (clamped to `s.len()`) that lands on a UTF-8 char +/// boundary. Slicing a `&str` at a mid-codepoint byte panics, so every byte-cap +/// truncation in this module (`redacted_display` here, `truncate_output` / +/// `truncate_output_tail` in `gates.rs`) walks forward from its cap to the next +/// boundary before slicing. The back-off is O(1) — at most 3 bytes for any +/// UTF-8 codepoint — never an O(idx) rescan. Extracted so the boundary rule +/// lives in exactly one place (the three sites previously duplicated it). +pub(super) fn next_char_boundary(s: &str, idx: usize) -> usize { + let mut end = idx.min(s.len()); + while end < s.len() && !s.is_char_boundary(end) { + end += 1; + } + end +} + pub fn default_gates() -> Vec { vec![ RelaunchGate::Smoke, From 4eeaac5b4c7098a0595e99d49c2afc79bdc3b268 Mon Sep 17 00:00:00 2001 From: rysweet Date: Thu, 23 Jul 2026 23:11:13 +0000 Subject: [PATCH 3/3] docs(canary-gate): fix stale test-name reference in unit-test gate diagnostics The reference table cited `truncate_output_tail_keeps_summary` but the actual test is `truncate_output_tail_keeps_trailing_summary`. Align the doc with the source so the cited test name resolves (#4522 quality audit). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- docs/reference/canary-unit-test-gate-diagnostics.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/reference/canary-unit-test-gate-diagnostics.md b/docs/reference/canary-unit-test-gate-diagnostics.md index 5752b508c..ae474b28a 100644 --- a/docs/reference/canary-unit-test-gate-diagnostics.md +++ b/docs/reference/canary-unit-test-gate-diagnostics.md @@ -277,7 +277,7 @@ the retained security assertions: | `unit_test_gate_passes_for_healthy_candidate` | A hermetic temp fixture crate (minimal `Cargo.toml` + one passing `#[test]`) run through the gate under `--lib` returns `passed == true`. Fails loudly if the toolchain is missing — **no silent skip**. | | `unit_test_failure_surfaces_failing_test_name` | Canned `cargo` stdout/stderr containing `test my_failing_case ... FAILED` + `test result: FAILED` fed to `parse_unit_test_failure` yields a detail containing **both** `my_failing_case` and `test result: FAILED`. No `cargo` invocation. | | `unit_test_abort_without_summary_surfaces_tail` | The exit-`101` **no-summary** shape (a compile `error[...]` / panic with no `test result:` line) still yields a non-empty detail carrying `cargo`'s tail — the previously-undiagnosable case. | -| `truncate_output_tail_keeps_summary` | `truncate_output_tail` keeps the **last** `max_len` bytes and prefixes `...`, so a trailing `test result:` line survives while a long head is dropped. Char-boundary-safe on multi-byte input. | +| `truncate_output_tail_keeps_trailing_summary` | `truncate_output_tail` keeps the **last** `max_len` bytes and prefixes `...`, so a trailing `test result:` line survives while a long head is dropped. Char-boundary-safe on multi-byte input. | | `canary_gate_env_allowlist_carries_deploy_shape_names_not_hijack_vars` (retained) | The allow-list includes the `SIMARD_*` deploy-shape names and excludes `LD_PRELOAD`/hijack-class — the scrub defense is not weakened by the scope fix. | The failing-name and no-summary tests are pure (canned input, no `cargo`), so