Skip to content

Update documentation with 11 changed files (#4558) - #4567

Draft
rysweet wants to merge 3 commits into
mainfrom
feat/issue-4558-fix-the-systemic-self-deploy-unit-test-gate-crash
Draft

Update documentation with 11 changed files (#4558)#4567
rysweet wants to merge 3 commits into
mainfrom
feat/issue-4558-fix-the-systemic-self-deploy-unit-test-gate-crash

Conversation

@rysweet

@rysweet rysweet commented Jul 24, 2026

Copy link
Copy Markdown
Owner

Summary

Concise workflow-generated PR for documentation.

Issue

Closes #4558

Changed files

  • docs/howto/diagnose-a-red-canary-unit-test-gate.md
  • docs/index.md
  • docs/reference/hermetic-unit-test-gate.md
  • src/operator_cli/mod.rs
  • src/self_relaunch/gates.rs
  • src/self_relaunch_semaphore/handoff.rs
  • tests/adaptive_scaling.rs
  • tests/fixtures/unit_test_gate_fixture/Cargo.lock
  • tests/fixtures/unit_test_gate_fixture/Cargo.toml
  • tests/fixtures/unit_test_gate_fixture/lib.rs
  • tests/self_relaunch_hermetic_unit_test_gate.rs

Diff stat

 docs/howto/diagnose-a-red-canary-unit-test-gate.md | 131 +++++++
 docs/index.md                                      |   2 +
 docs/reference/hermetic-unit-test-gate.md          | 357 +++++++++++++++++++
 src/operator_cli/mod.rs                            |   8 +-
 src/self_relaunch/gates.rs                         | 394 ++++++++++++++++++++-
 src/self_relaunch_semaphore/handoff.rs             |   8 +-
 tests/adaptive_scaling.rs                          |  12 +-
 tests/fixtures/unit_test_gate_fixture/Cargo.lock   |   7 +
 tests/fixtures/unit_test_gate_fixture/Cargo.toml   |  15 +
 tests/fixtures/unit_test_gate_fixture/lib.rs       |  51 +++
 tests/self_relaunch_hermetic_unit_test_gate.rs     | 296 ++++++++++++++++
 11 files changed, 1274 insertions(+), 7 deletions(-)

Behavior

Implemented behavior through these branch commits:
e61a53e test(ooda): make scaler_current_max_can_override_config hermetic
2d26ae6 fix(self-relaunch): redact credentials in gate-failure detail on both consumer paths (SEC-D2)
24ce6d3 wip: checkpoint after implementation (steps 7-8)

Validation

ℹ NODE_OPTIONS=--max-old-space-size=32768 (saved preference). To change: /home/azureuser/.amplihack/config
I'll start by understanding the current state of the changes and the affected toolchains.

● Get branch, status, diff stat, and recent commits (shell)
│ cd /home/azureuser/src/Simard-deploy-4049/worktrees/feat/issue-4558-fix-the-systemic-self-deploy
│ -unit-test-gate-crash && git branch --show-current && echo "---STATUS---" && git --no-pager
│ status && echo "---DIFF STAT---" && git --no-pager diff --stat HEAD && echo "---LOG---" && git
│ --no-pager log --oneline -10
└ 20 lines…

Working tree is clean with 3 commits ahead. Let me examine the changes made in these commits.

Risk

No high-risk subsystem pattern detected from changed paths.

Checklist

  • Branch has 3 commit(s) ahead of main
  • Code review completed
  • Philosophy check passed

This PR was created as a draft for review before merging.


Step 16b: Outside-In Testing Results

Detected toolchain: Rust CLI (Cargo.toml at repo root, cargo 1.95.0 / rustc 1.95.0). Per the qa-team skill's repo-type table, cargo test is the outside-in consumer boundary for this repo — the gate itself shells out to cargo test, so the user boundary is native Rust tests, not gadugi/uvx. Changed files: src/self_relaunch/gates.rs, src/operator_cli/mod.rs, src/self_relaunch_semaphore/handoff.rs, tests/adaptive_scaling.rs, new fixture crate tests/fixtures/unit_test_gate_fixture/, new integration test tests/self_relaunch_hermetic_unit_test_gate.rs.

Chosen strategy: Drive the real public gate API (verify_canary + RelaunchGate::UnitTest) and the pure extract_failure_detail extractor exactly as the daemon does — one simple always-run scenario (the deterministic, subprocess-free diagnosability contract) and one edge/integration scenario (end-to-end nested-cargo test gate under a simulated live daemon, including the #[ignore]d lane). Host env matched the #4558 field condition: CARGO_HOME/RUSTUP_HOME unset, HOME=/home/azureuser.

Scenario 1 — simple, always-run: failing test NAME survives into failing_detail

  • Command: cargo test --lib 'self_relaunch::gates::tests::extract_failure_detail' -- --test-threads=4
  • Result: ✅ PASS
  • Key output:
    running 8 tests
    test ...extract_failure_detail_names_failing_test_from_failures_block ... ok
    test ...extract_failure_detail_names_failing_test_from_panicked_at_on_stderr ... ok
    test ...extract_failure_detail_names_failing_test_from_failed_line ... ok
    test ...extract_failure_detail_prefers_failures_block_over_panicked_at ... ok
    test ...extract_failure_detail_clamps_to_4096_bytes ... ok
    test ...extract_failure_detail_is_utf8_boundary_safe ... ok
    test ...extract_failure_detail_no_marker_falls_back_to_bounded_nonempty_tail ... ok
    test ...extract_failure_detail_scans_stdout_when_stderr_empty ... ok
    test result: ok. 8 passed; 0 failed; ... 9286 filtered out
    
    Confirms the fix scans combined stdout+stderr (the failing name is on stdout; the old code read only stderr), preserves the failing test NAME, applies the failures: > panicked at > FAILED precedence, and clamps UTF-8-safely at 4096B (up from the 200B stderr head that surfaced the Drop t… spinner fragment).

Scenario 2 — edge/integration: hermetic gate under a simulated live daemon (incl. nested cargo)

  • Command: cargo test --test self_relaunch_hermetic_unit_test_gate -- --include-ignored --test-threads=1
  • Result: ✅ PASS
  • Key output:
    running 4 tests
    test fixture_crate_is_present_and_standalone ... ok
    test green_fixture_passes_gate_under_simulated_live_daemon ... ok
    test green_fixture_passes_when_daemon_env_has_no_cargo_or_rustup_home ... ok
    test red_fixture_failing_detail_names_the_failing_test ... ok
    test result: ok. 4 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out
    

Fix count during outside-in testing: 0. Both the simple and the edge/integration scenarios passed on the first run against the current branch head (e61a53ec7); no diagnose-fix-commit-push iterations were required. Working tree clean, local HEAD in sync with origin.

rysweet and others added 3 commits July 24, 2026 13:30
Automatic checkpoint to preserve work in progress.
Tests and implementation saved before refactoring phase.
… consumer paths (SEC-D2)

The #4558 unit-test-gate detail enlargement (200B stderr head -> 4096B
combined stdout+stderr capture) widened what a failing gate's detail can
carry. Two consumers of that detail printed/propagated it via raw Display:

  * operator_cli::dispatch_handover_command -> eprintln! to the operator
    terminal
  * self_relaunch_semaphore::handoff::coordinated_handoff -> error reason
    propagated into the daemon restart-error log

Apply redact_credentials at emission on both paths, matching the existing
overseer::deploy redact-at-emission pattern, so an enlarged detail that
happens to contain a token-bearing remote URL never surfaces a live token.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
`scaler_current_max_can_override_config` in tests/adaptive_scaling.rs built
its OodaConfig with `..OodaConfig::default()`, which consults the process env
(`SIMARD_SCALING`). On a host with `SIMARD_SCALING=auto` (the live simard-ooda
daemon's environment), `default()` seeds an AIMD scaler at the env ceiling that
overrides the explicit `max_concurrent_actions` under test, so `decide` capped
by live system pressure (5 actions) instead of the configured limit (2) — an
exit-101 false failure whenever the suite runs under the daemon env.

Pin `scaler: None` so the scaler's `current_max` is the sole cap under test,
matching the hermeticity fix issue #2732 already applied to the sibling
`decide_respects_max_concurrent_actions`. This is the same class of
environment-leak false failure that issue #4558 hardens the deploy gate
against via env scrubbing; the test is now deterministic under either env.

Verified: all 22 tests/adaptive_scaling tests pass under SIMARD_SCALING=auto.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@github-actions

Copy link
Copy Markdown

📊 Coverage Summary

Generated by cargo llvm-cov --workspace --summary-only (nightly, excluding test files)

Module Lines Covered Coverage
Total 196687 165339 84.1%

Coverage data from CI run. Test files matching tests?/ are excluded from line counts.

@rysweet

rysweet commented Jul 24, 2026

Copy link
Copy Markdown
Owner Author

Step 17b — Comprehensive Code Review (#4558 hermetic unit-test gate)

Verdict: APPROVE. The change is correct, well-scoped, thoroughly tested, and documented. No blocking issues.

Scope reviewed

src/self_relaunch/gates.rs (+394), two consumer paths (operator_cli/mod.rs, self_relaunch_semaphore/handoff.rs), the hermetic-gate integration test + fixture crate, an OODA test hardening, and two docs.

Checklist

  • Code quality & standards — deny-by-default env scrub, RAII isolation dir, toolchain-pin, and extraction helpers are small, single-purpose, and heavily documented with root-cause rationale.
  • Test coverage adequate — 8 always-run extract_failure_detail unit tests cover every marker (failures:/panicked at/FAILED), precedence, 4096B clamp, UTF-8 boundary safety, and no-marker tail fallback. Three #[ignore]d end-to-end tests drive the real public gate against a standalone fixture (green under simulated live daemon, toolchain-pin, red/diagnosable) — no recursive full-suite run. A fast always-run guard keeps the fixture path honest.
  • No TODOs, stubs, or swallowed exceptions — none. All .unwrap()/panic! occurrences are confined to #[cfg(test)] (or the intentional test fixture).
  • No unimplemented functions — none.
  • Logic correctness — verified:
  • Edge case handling — symlink-escape defense (canonicalize + starts_with(temp_root)), UTF-8 boundary clamps (head & tail), empty-stream branches, zero-length/over-limit truncation all covered by tests.

Security (SEC-D2 / SEC-D3)

  • The detail enlargement (200B stderr-only → 4096B stdout+stderr) widened what a failing gate's detail can carry, so redact_credentials is now applied at emission on both previously-raw consumer paths (operator_cli terminal print, handoff error reason → daemon restart log), matching the existing overseer::deploy redact-at-emission pattern. bound_gate_detail also redacts+bounds before tracing/OTel.
  • SEC-D3 defense-in-depth: is_hijack_class_env code-refuses LD_*/DYLD_*/GIT_SSH*/BASH_ENV/ENV/SHELLOPTS/BASHOPTS/IFS from allow-list re-injection (case-insensitive; ENVOY-style superstrings not false-positived — tested).

Constraints honored

Additive/non-breaking; tracing/OTel discipline preserved (operator-CLI print!/eprintln! are the CLI's legitimate terminal I/O, now redacted); no "Bridge" naming; no Python dependency; fixture is a standalone [workspace] with publish = false.

Validation (fresh this pass)

Check Result
cargo fmt --check clean
cargo clippy --lib no warnings
self_relaunch::gates lib tests 24 passed
redact_credentials tests 3 passed

No changes requested. Merge-ready.

Co-authored-by: Copilot

@rysweet

rysweet commented Jul 24, 2026

Copy link
Copy Markdown
Owner Author

Step 17c — Security Review (#4558 hermetic unit-test gate)

Verdict: APPROVE with two non-blocking, defense-in-depth follow-ups. No currently-exploitable vulnerability. All findings verified against actual code (not the PR description).

Checklist

  • Security requirements met — SEC-D2 (credential redaction) and SEC-D3 (hijack-class env refusal) are implemented and covered by tests.
  • New vulnerabilities — none exploitable in the current wiring; two latent/defense-in-depth gaps below.
  • Sensitive-data handling — no unredacted GateResult/detail emission path remains (see "Clean" below).
  • AuthZ/AuthN — N/A (no auth surface changed).
  • Injection — child process built via std::process::Command discrete .arg() (direct exec, no shell); hijack check case-insensitive, no substring bypass.

Finding 1 — SEC-D3 hijack deny-list omits cargo/rustc RCE env vars — MEDIUM / defense-in-depth

Location: src/self_relaunch/gates.rs:116-119 (is_hijack_class_env), enforced in scrub_gate_env (gates.rs:95-101).

The deny-list covers loader (LD_, DYLD_), git (GIT_SSH) and shell vars (BASH_ENV,ENV,SHELLOPTS,BASHOPTS,IFS) but omits the primary arbitrary-code-execution vectors for the one gate that shells out to cargo test:

  • RUSTC_WRAPPER / RUSTC_WORKSPACE_WRAPPER — program cargo runs wrapping every rustc → RCE at build.
  • CARGO_TARGET_<TRIPLE>_RUNNER — program cargo uses to run the compiled test binary → direct RCE.
  • RUSTFLAGS / CARGO_ENCODED_RUSTFLAGS — inject -C linker=/-C link-arg= → RCE at link.
  • CARGO_BUILD_RUSTC / CARGO_BUILD_RUSTC_WRAPPER.

Exploitability today: NONEconfig.canary_env is only ever the trusted 3-name allow-list canary_gate_env_allowlist() = SIMARD_HOME, SIMARD_PROMPT_ASSETS_DIR, SIMARD_STATE_ROOT (gates.rs:24, wired at source_prep.rs:677, operator_cli/mod.rs:457). So no cargo/rust var can be re-injected. However, the docstring at gates.rs:60-62 explicitly promises the guarantee "holds even if a future caller populates config.canary_env from a less-trusted source." For the gate that runs cargo, that stated guarantee is not met, and RUSTC_WRAPPER/*_RUNNER are a strictly worse RCE primitive than the LD_PRELOAD case already blocked. The test at gates.rs:522-556 encodes the blind spot (asserts loader/shell/git refused + CARGO_HOME allowed, never the exec-hijack cargo vars).

Remediation (follow-up issue, not blocking): add RUSTC_WRAPPER, RUSTC_WORKSPACE_WRAPPER, RUSTFLAGS, CARGO_ENCODED_RUSTFLAGS, CARGO_BUILD_RUSTC*, and the CARGO_TARGET_*_RUNNER pattern to the deny-list (prefix-match CARGO_ with an explicit carve-out for the load-bearing CARGO_HOME/CARGO_BUILD_JOBS the gate sets itself), and extend the gates.rs:522 test.

Finding 2 — Gate detail not stripped of control/ANSI chars before terminal + log emission — LOW / defense-in-depth

Location: src/operator_cli/mod.rs:478-481 (eprintln! to operator terminal) and src/self_relaunch_semaphore/handoff.rs:102-107 (error reason → daemon restart-error log).

The widened detail now carries up to 4096 B of raw cargo test stdout+stderr. Before emission only redact_credentials is applied, which neutralizes only URL userinfo (scheme://user@host; source_prep.rs:153-178) — it passes newlines, CR, and ANSI/terminal escapes through unchanged. Since the canary is built from a candidate commit, an actor able to land a candidate commit controls test output and can inject forged log lines (newline splicing) or terminal escape sequences into an operator session / daemon log. The repo already ships a control-strip sanitizer for exactly this (src/ooda_brain/sanitize.rs::sanitize_context_var, issue #2751) that is not applied here. Secondary: redact_credentials coverage is URL-only — an Authorization:/bearer token echoed by a test is not redacted (residual is small because the scrubbed child env keeps high-value secrets out, and the realistic case — a token in a git remote URL — is covered).

Remediation (follow-up, not blocking): run the detail through ooda_brain::sanitize (control/ANSI strip) in addition to redact_credentials at both sinks; optionally extend redaction to Authorization:/bearer patterns.


Dimensions assessed CLEAN (with evidence)

  • No missing-redaction path (sensitive data): both new sinks redact (operator_cli/mod.rs:480, handoff.rs:105); verify_canary tracing sink redacts+bounds via bound_gate_detail (gates.rs:153,165-170); overseer path redacts at population (overseer/deploy.rs:585). GateResult derives only Clone, Debug (no serde::Serialize, types.rs:58) → no serialization leak. Deploy success detail is the "N/N gates" tally, not raw output.
  • No shell injection: child built with discrete .arg(...) direct-exec (gates.rs:382-399); no attacker-influenced concatenation into a command line. Case-insensitive hijack check (to_ascii_uppercase, gates.rs:116) blocks more, not less; ENVOY-vs-ENV superstring test (gates.rs:549) confirms no substring mismatch.
  • No path/symlink/TOCTOU: isolation dir via tempfile (O_EXCL) + canonicalize + fail-closed on escape (gates.rs:228-244); HOME/TMPDIR/SIMARD_STATE_ROOT/SIMARD_HOME redirected into that private dir (gates.rs:395-398); toolchain pinned from pre-override real HOME (gates.rs:380).
  • Fail-closed confirmed: isolation-setup failure → failing GateResult, no fallback (gates.rs:365-374); hijack name dropped via continue and gate still runs scrubbed — no silent non-hermetic fallback branch.

Recommend filing the two follow-ups as separate hardening issues; neither blocks this merge.

@rysweet

rysweet commented Jul 24, 2026

Copy link
Copy Markdown
Owner Author

Step 17d — Philosophy Guardian Review (#4558 hermetic unit-test gate)

Verdict: COMPLIANT — all philosophy criteria pass. No changes requested.

Compliance checklist

  • Ruthless simplicity achieved — The root-cause repair supplies the missing canary signal through an audited allow-list of env names rather than widening the deny-by-default floor or inheriting the daemon's whole ambient env. Helpers are small and single-purpose (scrub_gate_env, is_hijack_class_env, extract_failure_detail, clamp_utf8/clamp_utf8_tail, run_unit_test_gate). No speculative future-proofing.
  • Bricks & studs pattern followed — Gate logic stays self-contained in self_relaunch/gates.rs with a minimal public contract (canary_gate_env_allowlist, verify_canary, all_gates_passed). Test fixtures live in their own isolated crate under tests/fixtures/unit_test_gate_fixture/. The SEC-D2 redaction reuses the existing self_deploy::source_prep::redact_credentials stud instead of inventing a parallel abstraction — connecting to a stable connector, exactly the brick model.
  • Zero-BS implementation — Scan of changed production files (gates.rs, operator_cli/mod.rs, handoff.rs) shows no todo!/unimplemented!/unreachable!()/FIXME/TODO, no stubs, no faked APIs. All .unwrap()/panic! are confined to #[cfg(test)]. The gate is fail-closed: a genuinely missing signal still yields a red verdict — no silent non-hermetic fallback, no swallowed errors. eprintln! uses are the operator CLI's legitimate terminal I/O (now credential-redacted).
  • No over-engineering — The four bounding/truncation helpers each carry distinct, justified semantics (head-clamp, tail-clamp, ellipsis-truncate, detail wrapper) driven by UTF-8-boundary safety, not gratuitous layering. Env allow-list is names-only, values read live at spawn — minimal surface.
  • Clean module boundaries — Redact-at-emission is applied at the two consumer paths that widened (operator_cli::dispatch_handover_command, self_relaunch_semaphore::handoff::coordinated_handoff), each with a comment tying it to the overseer::deploy pattern and the Fix the systemic self-deploy unit-test-gate crash-loop that has blocked EVERY self-deploy for 6+ hours (20-21 consecutive red-canary refusals; journal signature: overseer::deploy 'self-deploy refused  #4558 detail enlargement. Cross-module call compiles cleanly via pub(crate) visibility. No boundary leakage.

Evidence

  • Zero-BS scan of prod files: no stubs/TODOs/unimplemented.
  • Public contract: 3 focused pub fn studs; fixtures isolated in own crate.
  • Redaction reuses existing brick rather than duplicating logic.
  • Fail-closed behavior verified by unhealthy_candidate_stays_fail_closed_red + hijack_class_name_in_canary_env_is_never_reinjected tests.

Philosophy-compliant and merge-ready from the guardian's perspective.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant