Update .github with 11 changed files (#4522) - #4542
Conversation
…IMARD_STATE_ROOT (#4522) Run the deploy-canary unit-test gate against an isolated, per-run state root so the non-hermetic cargo test suite can no longer be poisoned by the live daemon's SIMARD_STATE_ROOT. Implements branch (c) of the A2 decision rule: non-hermetic test -> isolated state root. - Add IsolatedStateRoot (RAII, unique tempdir, 0700, auto-cleanup on drop) - Apply the isolated SIMARD_STATE_ROOT override after scrub_gate_env so the isolated root always wins (last env() wins); deny-over-allow preserved - Fail closed: create() failure returns a RED GateResult, never a skip - Leave canary_gate_env_allowlist and is_hijack_class_env unchanged (additive) - 25/25 gates tests green (5 new #4522 tests: 0700+drop lifecycle, override ordering, hijack-var still stripped, full-suite invocation preserved) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
📊 Coverage Summary
Coverage data from CI run. Test files matching |
rysweet
left a comment
There was a problem hiding this comment.
Step 17b — Comprehensive Code Review (#4522 hermetic unit-test canary gate)
Verdict: APPROVE — no blocking issues. Reviewed the full diff (5 files; core logic in src/self_relaunch/gates.rs, +294) at PR head 95a7401.
Review checklist
| Item | Result | Notes |
|---|---|---|
| Code quality & standards | ✅ | Intent-revealing names; RAII IsolatedStateRoot; no Bridge naming; no print!/println! (structured discipline preserved). |
| Test coverage adequate | ✅ | 5 TDD tests cover the override, apply-after-scrub ordering, hijack-guard preservation, full-suite invariant, and 0700+drop lifecycle. 25/25 lib gate tests pass; clippy --lib --all-features clean. |
| No TODOs / stubs / swallowed exceptions | ✅ | None. IsolatedStateRoot::create() failure returns a fail-closed RED GateResult (never silently runs against the live root). |
| No unimplemented functions | ✅ | None. |
| Logic correctness | ✅ | See below. |
| Edge case handling | ✅ | tempdir-create failure, hijack re-injection, missing ambient var all handled. |
Logic correctness — verified
- Ordering guarantee holds.
build_unit_test_commandbuilds viascrubbed_command("cargo", …)(which runsenv_clear()+ re-injects the allow-listed liveSIMARD_STATE_ROOT), then applies.env("SIMARD_STATE_ROOT", state_root)last.Commandcollapses duplicate keys to the final value, soget_envs()yields the isolated root — the tests assert exactly this, not an incidental value. - Fail-closed preserved. tempdir creation error ⇒
passed: falsewith a clear detail./tmpunwritable reddens the canary rather than colliding with the live daemon. Correct. - Security guard intact. The new override does not touch
is_hijack_class_env;LD_PRELOAD-class vars remain denied even when listed incanary_env(isolation_does_not_reinject_a_hijack_var_into_unit_test_gateproves it). Deny-by-default floor + deny-over-allow unchanged. - Gate not weakened. Still invokes the full
cargo test --manifest-path … --target-dir … CARGO_BUILD_JOBS; no narrowing/skip. Asserted structurally. unwrap/expectconfined to tests — zero in the non-test region.
Non-blocking observations (no action required)
- The
0700set_permissionshardening is#[cfg(unix)]; on non-unix the explicit mode-pin is skipped (tempfile still creates0700on unix by default). The gate is a systemd/unix deploy path, so this is appropriate — noted only for completeness. - Since the unit-test gate now always overrides
SIMARD_STATE_ROOT, keeping that name in the sharedcanary_envallow-list is redundant for this gate (still required for the process-probe gates). Harmless and intentional given the allow-list is cross-gate. - Pre-existing (not introduced here): the failure
detailusesformat!("tests failed (exit {}): …", output.status)whereExitStatusDisplay already printsexit status: N, yielding a mild "exit exit status:" doubling. Out of scope for this PR.
Docs
Reference doc canary-unit-test-gate-hermetic-isolation.md (+313) plus cross-links in the convergence reference, howto runbook (step 4b), and index are accurate and consistent with the code. Good root-cause narrative (exit 101 = test-binary abort vs 1 = assertion).
Conclusion: Additive, non-breaking, root-cause fix with strong TDD coverage and preserved fail-closed/security invariants. All checklist items pass. Ready to proceed.
rysweet
left a comment
There was a problem hiding this comment.
Step 17c — Security Review (#4522 hermetic unit-test canary gate)
Verdict: APPROVE — no security findings that block merge. This change is net security-positive: it closes an ambient-environment hijack vector in canary gate subprocesses and adds owner-only temp isolation. Reviewed PR head at src/self_relaunch/gates.rs.
Security requirements — all met
| Area | Result |
|---|---|
| Injection (command/shell) | ✅ cargo test args passed via .arg(...), no shell interpolation; state_root is a tempfile-generated path, not attacker-influenced |
| Env-hijack hardening | ✅ env_clear() + deny-by-default allow-list; is_hijack_class_env (case-insensitive) refuses LD_*/DYLD_*/GIT_SSH*/BASH_ENV/ENV/SHELLOPTS/BASHOPTS/IFS even if re-listed in canary_env (SEC-D3 defense-in-depth) |
| TOCTOU / symlink | ✅ Isolated state root is a unique random-named tempfile::TempDir, pinned 0700 (owner-only) so no other user can pre-seed/observe it |
| Sensitive-data handling | ✅ No env values logged; gate detail is truncated stderr routed through bound_gate_detail (credential-redacted, bounded). scrub_gate_env logs nothing |
| Fail-closed | ✅ Tempdir-create failure returns a RED GateResult, not a swallowed error — no silent-pass path |
| Lifecycle / leak | ✅ RAII drop removes the per-run dir; no per-tick temp-state accumulation |
| AuthN / AuthZ | N/A — no auth surface touched |
Positive security properties introduced
- Deny-by-default env floor: only a fixed
BASEset + trustedcanary_gate_env_allowlist()(SIMARD_HOME,SIMARD_PROMPT_ASSETS_DIR,SIMARD_STATE_ROOT) survive; everything else (including loader/shell hijack vars) is dropped. - Last-wins override pins the isolated
SIMARD_STATE_ROOTafter scrub re-injects the live value, so the live cognitive-memory root can never leak intocargo test— verified by the hijack-guard + apply-after-scrub tests. - Code-enforced hijack denylist backs the docstring guarantee, so the guarantee survives a future caller sourcing
canary_envfrom a less-trusted origin.
Non-blocking notes (no action required to merge)
- Hijack denylist is not exhaustive of build-influencing vars — it omits Rust/Cargo build wrappers (
RUSTC_WRAPPER,RUSTFLAGS,CARGO_BUILD_RUSTC) and other loader-ish vars (PYTHONPATH,PERL5LIB,NODE_OPTIONS). Harmless today because deny-by-default only admitsBASE+ the trusted allow-list; the guard is defense-in-depth for a future untrustedcanary_env. Ifcanary_envever sources from operator input, consider extendingis_hijack_class_envto coverRUSTC_WRAPPER/RUSTFLAGS/CARGO_BUILD_RUSTC. CARGO_HOME/RUSTUP_HOMEinherited from ambient — a poisonedCARGO_HOME/.cargo/config.toml(e.g.[build] rustc-wrapper=…) could steer the unit-test build. In-scope threat model is the daemon's own trusted systemd env, so acceptable; worth a one-line doc note thatCARGO_HOMEmust be trusted.- Redundant
set_permissions(0700)aftertempdir()has a negligible TOCTOU window, buttempfilealready creates0700, so it is a safe no-op re-affirmation (explicitly documented as defense-in-depth).
Conclusion: No new vulnerabilities. Sensitive data is not exposed. Injection and env-hijack surfaces are hardened, not widened. Approved from a security standpoint.
Step 17d — Philosophy Guardian Review (#4522 hermetic unit-test canary gate)Verdict: COMPLIANT — approved. Reviewed the full diff (5 files; core logic in Compliance checklist — all pass
Non-blocking observations (no action required)
Philosophy status: COMPLIANT. The change embodies ruthless simplicity, fail-closed safety, and a clean regeneratable brick. Ready to proceed. |
Summary
Concise workflow-generated PR for .github.
Issue
Closes #4522
Changed files
Diff stat
Behavior
Implemented behavior through these branch commits:
95a7401 fix(self-deploy): hermetic unit-test canary gate — isolated per-run SIMARD_STATE_ROOT (#4522)
Validation
ℹ NODE_OPTIONS=--max-old-space-size=32768 (saved preference). To change: /home/azureuser/.amplihack/config
I'll start by identifying the branch and the actual changes made in this worktree.
● Get branch name and changed files (shell)
│ cd /home/azureuser/src/Simard-deploy-4049/worktrees/feat/issue-4522-nodeoptions-max-old-space-si
│ ze32768-saved-preferen && git branch --show-current && echo "---DIFF STAT vs main---" && git
│ --no-pager diff --stat main...HEAD 2>/dev/null | tail -40 && echo "---STATUS---" && git
│ --no-pager status --short | head -40
└ 40 lines…
● Inspect commits and changed functions in gates.rs (shell)
│ cd /home/azureuser/src/Simard-deploy-4049/worktrees/feat/issue-4522-nodeoptions-max-old-space-si
Risk
No high-risk subsystem pattern detected from changed paths.
Checklist
This PR was created as a draft for review before merging.
Step 16b: Outside-In Testing Results
Detected toolchain: Rust (single crate
simard, edition 2024,Cargo.toml/Cargo.lockat root, cargo 1.95.0). Per the qa-team skill's repo-type detection, Rust CLI repos substitutecargo testfor the gadugi-agentic-test harness; no gadugi framework required.Chosen strategy: Exercise the changed outside-in boundary — the self-deploy
unit-testcanary gate insrc/self_relaunch/gates.rs— via native Rust tests. Two scenarios: (1) a simple run of the new hermetic-isolation convergence tests, and (2) an edge/integration run of the whole gate suite under the hostile ambient environment that originally triggered the exit-101 crash-loop (SIMARD_STATE_ROOTpointed at a busy "live" root, exactly as the running daemon sets it).cargo test --lib self_relaunch::gates::convergence_teststest result: ok. 9 passed; 0 failed(includesunit_test_gate_overrides_allowlisted_live_state_root_with_isolated_root,isolated_state_root_wins_over_reinjected_allowlist_by_ordering,isolated_state_root_is_private_0700_and_removed_on_drop)SIMARD_STATE_ROOT(reproduces the #4522 exit-101 condition)SIMARD_STATE_ROOT=/tmp/simard-busy-live-root.XXXX cargo test --lib self_relaunch::gatestest result: ok. 25 passed; 0 failed; gate no longer collides with the live daemon root — the isolatedIsolatedStateRootwinsConstraint checks: no stray
print!/println!in changed code (only doc/comment references); noBridgenaming in new identifiers; structuredtracing/OTel discipline preserved; fix is additive and scoped to theunit-testgate.Fix count during outside-in testing: 0 — both scenarios passed on the first run. The PR branch behaves correctly:
cargo testinside the canary gate now runs under a private, owner-only (0700) empty state root, so it can no longer race the live daemon'sSIMARD_STATE_ROOTand abort with exit status 101. The red-canary crash-loop condition is resolved at the outside-in boundary.