Update Cargo.toml with 22 changed files (#4470)

[rysweet]

Summary

Concise workflow-generated PR for Cargo.toml.

Issue

Closes #4470

Changed files

• Cargo.lock
• Cargo.toml
• docs/concepts/reconcile-and-self-deploy.md
• docs/howto/clear-a-stuck-memory-quarantine.md
• docs/operations/cognitive-memory-wal-recovery-runbook.md
• docs/operations/verified-backups.md
• docs/reference/overseer-deploy-canary-diagnostics.md
• docs/reference/self-deploy-api.md
• docs/reference/self-deploy-quarantine-acknowledge.md
• mkdocs.yml
• src/cmd_cleanup/disk.rs
• src/cmd_cleanup/tests.rs
• src/journal/mod.rs
• src/operator_cli/self_health.rs
• src/self_deploy/health.rs
• src/self_deploy/mod.rs
• src/self_deploy/quarantine_ack.rs
• src/self_deploy/tests_health.rs
• src/self_deploy/tests_orchestrator.rs
• src/self_relaunch/gates.rs
• tests/adaptive_scaling.rs
• tests/self_deploy_convergence.rs

Diff stat

 docs/howto/clear-a-stuck-memory-quarantine.md      | 134 ++++++++
 .../cognitive-memory-wal-recovery-runbook.md       |   7 +-
 docs/operations/verified-backups.md                |  73 ++--
 .../overseer-deploy-canary-diagnostics.md          |  45 +++
 docs/reference/self-deploy-api.md                  |  97 ++----
 .../self-deploy-quarantine-acknowledge.md          | 327 ++++++++++++++++++
 mkdocs.yml                                         |   2 +
 src/cmd_cleanup/disk.rs                            | 224 ++++++++----
 src/cmd_cleanup/tests.rs                           | 314 ++++++++++++-----
 src/journal/mod.rs                                 |   2 +-
 src/operator_cli/self_health.rs                    | 117 ++++++-
 src/self_deploy/health.rs                          | 378 ++++++++++++---------
 src/self_deploy/mod.rs                             |   4 +
 src/self_deploy/quarantine_ack.rs                  | 333 ++++++++++++++++++
 src/self_deploy/tests_health.rs                    | 158 +--------
 src/self_deploy/tests_orchestrator.rs              |   2 -
 src/self_relaunch/gates.rs                         | 237 +++++++++----
 tests/adaptive_scaling.rs                          |  10 +-
 tests/self_deploy_convergence.rs                   | 124 +++++++
 22 files changed, 1917 insertions(+), 713 deletions(-)

Behavior

Implemented behavior through these branch commits:
65c9b5c feat: ℹ NODE_OPTIONS=--max-old-space-size=32768 (saved preference). To chang
ec48d72 test(cmd_cleanup): fix corrupt-DB sweep test flakiness via path injection
3745f74 refactor(self-deploy): dedupe is_corrupt_quarantine_name + fix stale docs
dca6477 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 branch and what's changed.

● Get branch, status, diff stat, and recent commits (shell)
│ cd /home/azureuser/src/Simard-deploy-4049/worktrees/feat/issue-4470-nodeoptions-max-old-space-si
│ ze32768-saved-preferen && echo "=== BRANCH ===" && git branch --show-current && echo "=== STATUS
│ ===" && git status --short && echo "=== DIFF STAT vs main ===" && git diff --stat main...HEAD
│ 2>/dev/null | tail -40 && echo "=== RECENT COMMITS ===" && git --no-pager log --oneline -10
└ 32 lines…

There's an uncommitted change in tests/adaptive_scaling.rs. Let me examine it and detect the toolchain.

Risk

No high-risk subsystem pattern detected from changed paths.

Checklist

• Branch has 4 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

Tested the PR branch from the external user/consumer boundary as a real operator would.

Detected toolchain: Rust CLI (Cargo.toml at root, simard binary via bin.js npm wrapper). Per the qa-team repo-type detection, cargo is the outside-in boundary — no gadugi framework required. Node/Playwright scripts (test:e2e) are unrelated to these self-deploy/self-health changes.

Chosen strategy: Build the real simard binary and drive it as an operator (simard self-health …) against an isolated SIMARD_STATE_ROOT, plus run the branch's own outside-in integration + changed-module unit tests. The self-health --acknowledge-quarantine flow is the exact consumer boundary for the #4469 stuck-quarantine deadlock fix.

Scenarios

#	Type	Scenario	Command	Result
1	simple (lib/integration)	Outside-in convergence contract + adaptive scaling	cargo test --test self_deploy_convergence --test adaptive_scaling	✅ 4/4 + 22/22 passed
2	simple (unit)	Changed modules: self_deploy, self_relaunch::gates, cmd_cleanup, self_health	cargo test --lib self_deploy / self_relaunch::gates / cmd_cleanup / self_health	✅ 120 + 13 + 38 + 6 passed, 0 failed
3	simple (CLI)	Operator inspects help for the new flag	simard self-health --help	✅ exit 0; documents --acknowledge-quarantine (#4469)
4	edge/integration (CLI)	Genuinely-stuck 2 MB quarantine → acknowledge → converge, non-destructively	seed cognitive.corrupt-* under a temp SIMARD_STATE_ROOT, then simard self-health --json then simard self-health --acknowledge-quarantine --json	✅ deadlock reproduced and cleared

Scenario 4 — key output (the core fix)

Before acknowledge (deadlock state that could never clear):

"no_quarantine": { "healthy": false, "quarantined": true }

After --acknowledge-quarantine (converged):

"no_quarantine": { "healthy": true,  "quarantined": false }

Retention check (non-destructive, #2550 recovery asset preserved):

RETAINED: artifact still present (2097152 bytes)
ACK sidecar present: <state_root>/cognitive.corrupt-20260101120000.ack

Note: the overall self-health exit code stays non-zero in an empty hermetic state root because unrelated probes (version/entrypoint parity) have no live deployed binary to check against — expected for a throwaway root and independent of this fix. The targeted no_quarantine probe deterministically transitions from quarantined=true → false, proving a stuck quarantine can now clear so self-deploy converges.

Fix count

0 fixes required. All outside-in scenarios passed on the branch as-is; no diagnose/fix/push iterations were needed.

[rysweet]

Step 17b — Comprehensive Code Review

Scope reviewed: main...pr-4488 (17 files, +1924/−168). Implements #4469 (durable .ack quarantine acknowledgement to break the stuck-quarantine self‑deploy deadlock) plus #4470 (canary failing‑test diagnosability + hermetic adaptive_scaling test).

Verification performed: the local working tree is mid‑merge, so I built and ran the changed‑module tests against the clean committed tip in a detached worktree (reusing the cached target dir). All green:

• lib: quarantine_ack 10, corrupt_db 13, aged_protected 5, auto_ack 4, quarantine_scan 5, self_health 6, extract_first_failure 3, sanitize_gate_detail 3
• integration: self_deploy_convergence 4/4, adaptive_scaling 22/22

Overall this is high‑quality, well‑tested, well‑documented work. Two issues below; the first is blocking.

---

🔴 Blocking — does not merge cleanly with main, and a naive resolution regresses behavior

The branch conflicts with current main in src/cmd_cleanup/disk.rs, src/cmd_cleanup/tests.rs, src/self_deploy/health.rs, docs/concepts/reconcile-and-self-deploy.md, and docs/reference/self-deploy-api.md. This is not a mechanical conflict — both branches refactored the same function for the same issue (#4469):

• main refactored remove_old_corrupt_dbs to sweep two directories independently — simard_state_root() and resolve_subdir("state") — via reclaim_corrupt_dbs_in_dir(dir), with per‑directory keep‑N / recovery‑asset protection.
• This PR refactored the same function into scan_quarantine_candidates + remove_old_corrupt_dbs_in(scan_dir) but sweeps only simard_state_root().

Consequence: taking this PR's version wholesale would drop main's live‑store (state/) directory sweep — a regression. The same gap exists on the read side: this PR's count_quarantine_files and auto_ack_stuck_recovery_asset scan only state_root, so ack‑awareness / auto‑ack would not cover the state/ subdir that main now sweeps.

Required resolution (semantic, not mechanical): layer this PR's .ack‑aware scan_quarantine_candidates / sidecar‑reclaim / select_protected_asset / aged_protected_recovery_asset on top of main's per‑directory model — i.e. run the ack‑aware sweep for each directory main sweeps, and apply the probe/auto‑ack across the same directory set. Re‑run the changed‑module + integration tests after resolving. (The committed PR tip itself is clean and green; the break is purely against latest main.)

---

🟡 Minor — orphaned .ack sidecars are never reclaimed

reclaim_ack_sidecar removes a sidecar only when its parent quarantine is swept, and scan_quarantine_candidates excludes *.ack. If a quarantine is removed out‑of‑band (operator rm, or manual deletion of the #2550 protected asset), its .ack becomes a permanent orphan — 13 bytes each, but unbounded over time. corrupt_db_sweep_never_treats_ack_marker_as_quarantine shows this is currently by design. Consider a periodic reclaim of *.ack markers whose parent artifact no longer exists. Low priority.

---

🟢 Strengths (verified)

• Root‑cause fix via single‑sourcing. is_corrupt_quarantine_name, select_protected_asset, and aged_protected_recovery_asset are now defined once and shared by the probe, sweep, and ack paths — this is precisely what eliminates the probe/sweep disagreement that caused the deadlock. The duplicated is_corrupt_quarantine_name in health.rs is correctly removed.
• Strong symlink/TOCTOU defense in acknowledge: symlink_metadata pre‑check + create_new (O_EXCL, never follows a symlink) + explicit AlreadyExists race handling + refusal to overwrite a non‑regular‑file sidecar. Covered by acknowledge_refuses_to_overwrite_planted_symlink_marker.
• Path‑safety in is_ackable_quarantine_basename: rejects separators, .., absolute paths, and marker names, verified against a single Component::Normal equal to the whole name. Thorough negative tests.
• Correct freshness semantics: filename‑keyed markers keep new corruption failing the probe (quarantine_scan_still_flags_fresh_corruption_after_ack); the guarded auto‑ack fires only when the asset is both ≥ the #2550 protection floor and past the forensic window (auto_ack_ignores_fresh_protected_asset, auto_ack_ignores_trivial_aged_quarantine), and is idempotent.
• Non‑destructive by construction: the recovery asset and its marker are always retained (corrupt_db_sweep_retains_protected_asset_marker).
• Test hermeticity fixes: replacing process‑wide HOME mutation with the path‑injected remove_old_corrupt_dbs_in removes cross‑test env races; scaler: None in adaptive_scaling removes the SIMARD_SCALING env dependency (#2732). These are genuine flakiness fixes, not test‑weakening.
• #4470 diagnosability: extract_first_failure + sanitize_gate_detail surface the first failing test instead of an opaque exit 101, with control‑char stripping and UTF‑8‑boundary‑safe byte bounding to prevent log/JSON forgery from untrusted subprocess output. The old truncate_output and its tests are fully removed (no dead code).
• Checklist: no TODOs / stubs / unimplemented functions; no swallowed exceptions (the let _ = auto_ack_stuck_recovery_asset(...) is documented best‑effort and internally logs on error); structured tracing/OTel only — no print!/println! in the new paths.

Nits (non‑blocking)

• sanitize_gate_detail truncates without an ellipsis, unlike the removed truncate_output — cosmetic.
• count_quarantine_files does one symlink_metadata stat per candidate via is_acknowledged; fine at keep‑N scale, just an O(n) stat note.

Verdict: Approve the design and implementation on their own merits, but must resolve the main conflict semantically (preserve the two‑directory sweep) and re‑validate before merge.

_{🤖 Automated Step 17b review. Build/test evidence gathered from a clean worktree at the PR tip.}

[rysweet]

🔒 Step 17c — Security Review

Verdict: SECURE. No high-confidence exploitable vulnerabilities in the changed code. One Low-severity, same-trust-boundary log-forgery observation (advisory).

Validated against a clean checkout of the PR tip (the local tree is mid-merge). Every filesystem operation was traced against the actual code (not the doc-comments), and remove_dir_all's symlink behaviour was verified empirically.

Security checklist

• Security requirements met
• No new vulnerabilities (high-confidence)
• Sensitive-data handling reviewed
• Authorization / autonomous-action safety reviewed
• Injection vectors (path, symlink, log/JSON) reviewed

Findings by area

#	Area	Result	Evidence
1	Path traversal in acknowledge/ack_marker_path	✅ Sound	is_ackable_quarantine_basename (quarantine_ack.rs) layers separator rejection + single-Normal-component check + c == OsStr::new(name) equality; .join() can only produce a direct child of state_root. .., /etc/passwd, sub/x, x\evil, empty all rejected.
2	Symlink / TOCTOU on ack write	✅ Sound	symlink_metadata (no-follow) → OpenOptions::create_new(true) (O_EXCL, no-follow). Planted symlink ⇒ AlreadyExists ⇒ re-lstat, Ok only for regular file. Covered by acknowledge_refuses_to_overwrite_planted_symlink_marker. No write-through.
3	Arbitrary deletion (reclaim_ack_sidecar, sweep)	✅ Sound	Sidecar reclaim only remove_files a regular file; symlink/dir left untouched. Verified remove_dir_all on a symlink-to-dir unlinks the symlink only (target intact) — no recursive delete-through. Deletes gated by name match + age/keep caps + protected-asset skip.
4	Untrusted-output sanitization (#4470)	✅ Sound at sinks (minor Unicode gap)	sanitize_gate_detail collapses every char::is_control() (CR/LF/NUL/TAB/ESC → space) and byte-bounds on a UTF-8 boundary (non-panicking). Gap: U+2028/U+2029 & bidi-override pass through, but the sink is tracing structured fields + JSON serialization, which escape safely ⇒ not exploitable (terminal-display spoofing only).
5	Autonomous auto-ack signal suppression (#4469)	✅ Sound	auto_ack_stuck_recovery_asset acks only the single aged_protected_recovery_asset (largest ≥1 MiB, past the forensic window; future-dated mtime ⇒ unwrap_or_default()=0 ⇒ not aged). Filename-keyed ⇒ any fresh/unrelated cognitive*.corrupt-<ts> still counts and reddens the probe. Acked asset is retained, never deleted. No cross-file suppression.
6	Sensitive-data handling	⚠️ See Finding 1	.ack payload is the fixed constant b"acknowledged\n" — no secrets. SimardError Display drops path; unsafe-name reason uses escaped {:?}. But raw filename logged via %name (Display) — Finding 1.
7	DoS / resource	✅ Nothing anomalous	Metadata-only reads; Vec growth bounded by dir entry count; size computed once per candidate. No unbounded file reads.
8	CLI parse_flags / env handling	✅ Sound	Strict allowlist; --pre-deploy-facts= parsed as u64 with error on failure; state_root from trusted simard_state_root(). Every unsafe { set_var } is confined to #[cfg(test)].

Finding 1 — Unsanitized quarantine filename logged via %name (log-line forgery)

Severity: Low · Confidence: Medium — src/self_deploy/health.rs (auto_ack warn) and src/operator_cli/self_health.rs (acknowledge_quarantine_failed warn).

tracing::warn!(artifact = %name, …) logs the raw quarantine basename with Display. Unix filenames may contain newlines, and name only needs to satisfy is_corrupt_quarantine_name (cognitive./cognitive_memory. prefix + .corrupt- infix). Under the default non-JSON subscriber, field values are written raw, so a filename like cognitive.corrupt-1<LF>level=ERROR forged line can forge an additional log line. This is the same log-forgery class the PR's #4470 change already sanitizes for GateResult.detail — but that discipline isn't applied to quarantine filenames.

• Threat model: requires an attacker who can write to the daemon's state_root (~/.simard) and make the file the aged ≥1 MiB protected asset — the same trust boundary the daemon already relies on. In JSON mode (.json()) the value is escaped and the issue does not arise. Impact is log spoofing / repudiation only — no data disclosure or code execution. Below the high-confidence bar; surfaced only because it's directly in scope.
• Suggested fix (advisory, non-blocking): route the filename through the existing sanitizer before logging, e.g. artifact = %sanitize_gate_detail(&name, GATE_DETAIL_MAX_BYTES), so health.rs/self_health.rs match the #4470 sanitization discipline.

Verified strengths

O_EXCL + no-follow ack write · symlink/TOCTOU-safe reclaim & probe · path-traversal rejection (separators, .., absolute, drive/root) · filename-keyed acks that never silence fresh corruption · non-destructive retention of the #2550 recovery asset · single-sourced protected-asset + age gate · #[cfg(test)]-confined env mutation · fixed constant .ack payload (no secrets) · strict allowlist CLI parsing.

No blocking security issues. The one Low finding is advisory and does not gate merge on its own. (Note: the separate code review flagged an unrelated blocking merge-conflict/semantic-overlap with main that must still be resolved.)

[rysweet]

🧭 Philosophy Guardian Review — PR #4488

Verdict: COMPLIANT (one carried-forward caveat tied to the Step 17b blocking merge finding).

Assessed the committed PR tip (pr-4488) against PHILOSOPHY.md. Validated in a throwaway worktree — all changed-module lib + integration tests green (the local working tree is mid-merge; that state is harness-owned and untouched).

✅ Ruthless simplicity — PASS

• gates.rs decomposes the canary into one function per gate (run_smoke_gate, run_unit_test_gate, run_gym_baseline_gate, run_rpc_health_gate) plus two focused helpers. No speculative abstraction, no config-driven gate registry — the simplest structure that expresses the four concrete gates.
• Diagnosability helpers (extract_first_failure, sanitize_gate_detail) are proportionate to their stated goals (#4470); GATE_DETAIL_MAX_BYTES = 512 is a justified bound, not gold-plating.
• The .ack marker is deliberately a presence flag, not a data store — minimal payload, bounded disk + forgery blast radius. Occam's razor applied.

✅ Bricks & studs — PASS

• quarantine_ack.rs is a textbook brick: one responsibility ("what is an acknowledgeable corrupt quarantine, and how do we durably ack it"), self-contained with tests, exposing clean studs — acknowledge, is_acknowledged, ack_marker_path, present_quarantine_artifacts, is_ack_marker_name.
• Single source of truth honored: the module delegates to the canonical crate::cmd_cleanup::is_corrupt_quarantine_name so the cleanup sweep, the health probe, and the ack path "can never disagree about which artifacts are corrupt-quarantines" (its own doc comment). Re-export lives in cmd_cleanup/mod.rs. This is exactly the contract-stability the brick model asks for.

✅ Zero-BS implementation — PASS

• No TODO/FIXME/unimplemented!/todo!/placeholder in production code.
• No production unwrap/expect — every one is inside #[cfg(test)]. Errors flow through SimardResult; subprocess failures are captured into GateResult.detail (surfaced, not swallowed) including the Err(e) "failed to run" arm.
• Non-destructive by construction: acknowledge never touches the quarantined artifact; refuses to follow a planted symlink/dir at the sidecar path (returns Err rather than clobbering).

✅ No over-engineering — PASS

• Each new helper earns its place against a concrete requirement (#4469 durable ack, #4470 diagnosability + log-forgery defense). No premature generalization.

✅ Clean module boundaries — PASS

• Quarantine semantics single-sourced through one predicate; scan / probe / ack are thin callers. Boundaries are crisp and regeneratable.

⚠️ Carried-forward caveat (philosophy-relevant)

The blocking merge conflict flagged in the Step 17b code review is itself a single-source-of-truth risk: main and this PR both refactored remove_old_corrupt_dbs for #4469, and they sweep different directory sets (main: state_root + state/; this PR: state_root only). A naive textual conflict resolution would produce two divergent sweep behaviors — a direct violation of the "can never disagree" invariant this PR otherwise upholds. Resolve semantically (layer ack-awareness onto main's per-directory model) and re-validate, so the brick's contract stays intact post-merge.

Checklist

• Ruthless simplicity achieved
• Bricks & studs pattern followed
• Zero-BS implementation (no stubs, faked APIs, swallowed exceptions)
• No over-engineering
• Clean module boundaries
• ⚠️ Merge preserves single-source-of-truth — must be verified during conflict resolution (see caveat)

Bottom line: The implementation is philosophy-compliant and a good example of the brick model. The only outstanding concern is ensuring the merge resolution does not fracture the single-sourced quarantine semantics.

[rysweet]

Step 17b — Comprehensive Code Review (automated)

Scope reviewed: all 12 changed src//tests/ Rust files (+1564/−168), focused on the #4469 durable-quarantine-acknowledgement feature and the #4470 gate-diagnosability change.

Verification performed (evidence)

• cargo test --lib quarantine → 37 passed, 0 failed
• cargo test --lib gates::tests → 13 passed, 0 failed
• cargo test --lib self_health → 6 passed, 0 failed
• cargo clippy --lib → 0 warnings, 0 errors (the PR's central -D warnings concern is clean)

Checklist

• Code quality & standards — idiomatic, well-documented, single-sourced predicates (is_corrupt_quarantine_name, select_protected_asset) so probe/sweep/ack can never disagree.
• Test coverage adequate — new modules carry unit tests for happy path, idempotency, unsafe-name rejection, symlink defence, fresh-vs-retained tally, and age-gated auto-ack; serial_test::serial(cognitive_memory) correctly added to env-touching decide.rs tests.
• No TODOs, stubs, or swallowed exceptions — errors are surfaced via structured tracing::warn! and CleanupReport.errors; best-effort paths are deliberate and documented.
• No unimplemented functions — none.
• Logic correctness — .ack-sidecar exclusion is correctly ordered before is_corrupt_quarantine_name (the sidecar carries the .corrupt- infix), auto-ack is age-gated + idempotent, protected-asset selection is tie-broken to newest.
• Edge case handling — TOCTOU closed with O_EXCL/create_new; symlink_metadata (no follow) throughout; UTF-8-boundary-safe truncation; absent/unreadable dir ⇒ empty/0.

Security (verified strong)

• acknowledge() refuses path-separator/../absolute/non-quarantine names, refuses to write through a planted symlink/dir sidecar, and closes the stat→open TOCTOU window with create_new(true).
• Untrusted on-disk basenames are logged with ?name (Debug-escaped) to prevent log-line forgery — consistent across health.rs and self_health.rs.
• sanitize_gate_detail bounds subprocess output to 512 bytes and strips control chars before it reaches logs/JSON.

Non-blocking nits (optional)

1. sanitize_gate_detail ANSI residue (src/self_relaunch/gates.rs): control bytes are collapsed, but the printable remainder of an ANSI escape (e.g. [31m after the stripped \x1b) survives. Harmless (no forgery vector, the test only asserts \x1b is gone), but if you want fully clean output consider stripping \x1b[...m sequences whole.
2. Pre-existing eprintln! in remove_old_corrupt_dbs_in (src/cmd_cleanup/disk.rs) remains, which is inconsistent with the "structured tracing + OTel only" convention the rest of the feature follows. It predates this PR (out of scope), but is a candidate for a follow-up to convert to tracing.
3. Operator --acknowledge-quarantine acks all present quarantines, including fresh ones (unlike the age-gated auto-ack on the probe path). This appears intentional — it's a manual operator escape hatch and is documented as such — just flagging the asymmetry with the autonomous path for confirmation.

Verdict

High-quality, security-conscious, well-tested change. No blocking issues; targeted tests and clippy are green. The three items above are optional nits/follow-ups.

[rysweet]

Step 17c — Security Review (automated, MANDATORY)

Scope: security-focused review of the diff origin/main...HEAD for PR #4488 (12 Rust files, +1564/−168) — the #4469 durable quarantine-acknowledgement feature and the #4470 gate-diagnosability change. Reviewed independently (a dedicated security-review agent plus direct source tracing).

Verdict

No blocking issues. No Critical / High / Medium findings. The PR is unusually security-conscious; every new filesystem, logging, and parsing path was traced and its defense verified. Two LOW defense-in-depth consistency findings are noted below — both align with the PR's own stated threat model (untrusted on-disk quarantine basenames).

Security checklist

• All security requirements met — TOCTOU, symlink, path-traversal, and log-forgery defenses present and tested.
• No new vulnerabilities — no unsafe, no Command/shell exec, no deserialization of untrusted data introduced.
• Sensitive data handling — no secrets logged; ack sidecar is a fixed 13-byte presence flag, never reads file contents.
• AuthN/AuthZ — autonomous auto-ack is strictly more restrictive than the operator path (single aged #2550 protected asset only, never deletes). No escalation/destructive divergence.
• Injection — no path traversal (is_ackable_quarantine_basename funnels every path op); no SQL/command injection surface. Log/terminal injection: see LOW findings.

---

LOW-1 — Untrusted quarantine basename written unescaped to stderr / cleanup report

File: src/cmd_cleanup/disk.rs (the eprintln!(" Removing corrupt DB {} ...", cand.path.display(), ...) sweep line, and sibling report.errors.push(format!("failed to remove {}: {e}", cand.path.display()))). Severity: LOW · Confidence: 7/10 · Category: log/terminal forgery.

Path::display() does not neutralize control chars, so a quarantine artifact whose basename contains CR/LF or an ANSI ESC (it only needs the cognitive* prefix + .corrupt- infix to pass is_corrupt_quarantine_name) is emitted verbatim to the operator's terminal and stored raw in CleanupReport. This is notable because the PR's own threat model treats these basenames as untrusted and deliberately Debug-escapes them (?name) in health.rs / self_health.rs — the cleanup sweep is the one place that escaping wasn't applied.

Attacker scenario: a process with write access to <state_root>/state/ plants cognitive.corrupt-2026\r\n[FORGED] all clear (or ANSI escapes); an operator running the sweep sees the forged line / terminal-control rendered on their console. Why LOW: presupposes local write access to the state root (largely outside the primary threat boundary); the JSON report path escapes control chars via serde.

Fix (defense-in-depth, non-blocking): route the basename through the same escaping used elsewhere — Debug ({:?}) / tracing or a sanitize_gate_detail-style control-char strip — before display.

LOW-2 — Auto-ack marker path logged via Display while sibling field is Debug-escaped

File: src/self_deploy/health.rs, auto_ack_stuck_recovery_asset — marker = %marker.display(). Severity: LOW · Confidence: 6/10 · Category: log forgery.

The adjacent artifact = ?name field is correctly Debug-escaped to prevent log-line forgery, but the marker field is logged via Display (%…display()). The marker basename is {name}.ack, embedding the same untrusted quarantine name (which can pass validation with an embedded newline, since it is a single Component::Normal on Unix). Same log-forgery class as LOW-1, same local-write precondition. Fix: log the marker via ?marker (Debug) for consistency.

---

Verified defenses (no action needed)

• TOCTOU / symlink on ack write — quarantine_ack::acknowledge stats with symlink_metadata (no-follow), opens with create_new(true) (O_EXCL, never follows symlinks), and re-checks on the AlreadyExists race branch, refusing to write through a planted symlink/dir. acknowledge_refuses_to_overwrite_planted_symlink_marker (unix) confirms the victim is untouched. ✔
• Path traversal — is_ackable_quarantine_basename rejects empty, /, \, and requires exactly one Component::Normal == whole name (rejects .., ., absolute/root/drive) before delegating to is_corrupt_quarantine_name; all three public fns funnel through it. Tests cover ../escape, sub/…, /etc/passwd, ... ✔
• .ack reclamation & candidate scan — reclaim_ack_sidecar / scan_quarantine_candidates use symlink_metadata / entry.metadata() and unlink the link, never follow a symlink target. ✔
• ANSI/terminal stripping — sanitize_gate_detail collapses every is_control() run (NUL, CR/LF, tab, ESC 0x1B, C1/0x9B CSI) to one space and bounds to 512 bytes on a UTF-8 boundary; extract_first_failure routes through it. Neutralizes ANSI (which requires ESC/CSI). ✔
• Log escaping on ack paths — auto_ack_stuck_recovery_asset / acknowledge_all_present_quarantines log basenames via ?name (Debug). ✔
• No DoS — marker is a presence flag; no untrusted-length reads/allocations. ✔
• AuthZ asymmetry — autonomous path strictly narrower than operator path; non-destructive. ✔

Evidence: cargo test --lib (quarantine / gates / self_health) → 56 passed, 0 failed; cargo clippy --lib → 0 warnings, 0 errors.

[rysweet]

Step 17d — Philosophy Guardian Review (automated)

Scope: the 12 changed src/ Rust files (+1564/−168) implementing the #4469 durable quarantine-acknowledgement feature and the #4470 gate-diagnosability change.

Compliance status

• Ruthless simplicity achieved. The deadlock-breaking mechanism is a 13-byte presence flag (ACK_MARKER_BYTES = b"acknowledged\n"), not a data store — "keeping it tiny bounds disk use and forgery blast radius." The [creative-idea] Root-cause the self-deploy 'red canary' failure — the deploy_gate unit test that exits 101 and has blocked every self-deploy attempt today — since that failing gate, not authoring, is the true cause of the persistent DeployDrift. #4470 change replaces the generic truncate_output with two focused helpers (extract_first_failure, sanitize_gate_detail) that do exactly one job each.
• Bricks & studs pattern followed. quarantine_ack.rs is a self-contained brick with an explicit ## Contract doc section enumerating its four studs (ack_marker_path, acknowledge, is_acknowledged, is_ack_marker_name). mod.rs re-exports only that public surface; consumers (disk sweep, health probe) depend on the contract, not internals.
• Zero-BS implementation. No stubs, no todo!/unimplemented!/FIXME, no faked APIs, no swallowed errors. Every unwrap()/panic! in the diff is confined to #[cfg(test)] code or the build-time meta-guard (serial_guard.rs), where a panic is the intended failure mode. No new print!/println! in production paths.
• No over-engineering. Predicates are single-sourced: is_ackable_quarantine_basename and the probe/sweep all delegate to the canonical cmd_cleanup::is_corrupt_quarantine_name, so the three call sites "can never disagree about which artifacts are corrupt-quarantines." The added conditional logic (aged-protected-recovery-asset selection, auto-ack) is intrinsic to the requirement — retain the recovery asset on disk while clearing the probe — not gratuitous abstraction.
• Clean module boundaries. quarantine_ack owns the ack convention end-to-end; disk.rs/health.rs consume it without reaching across boundaries; no circular dependencies introduced. Security invariants (O_EXCL create, no-follow symlink_metadata, name validation before path join) live with the module that owns the convention.

Non-blocking philosophy notes

1. The operator-initiated acknowledge vs. autonomous auto_ack_stuck_recovery_asset paths are deliberately asymmetric (manual ack is unconditional; auto-ack gates on age + protected status). This is defensible but worth a one-line doc cross-reference so a future reader doesn't mistake it for an inconsistency.
2. sanitize_gate_detail collapses control chars generically rather than ANSI-escape-specific stripping — simpler and correct, but a stray SGR-parameter byte (e.g. a bare [0m residue after the ESC is dropped) can survive as printable text. Cosmetic only; not a forgery vector.

Verdict: ✅ Philosophy-compliant. All five criteria pass. No blocking issues; the two notes are cosmetic/documentation-level.

[rysweet]

Step 17e — Blocking issues addressed (commit 228d4d24)

All Step 17 review findings resolved:

• 🔴 BLOCKING — probe/sweep directory-set asymmetry. The cleanup sweep reclaimed both the top-level state root and the live-store state/ subdir, but the self-health no_quarantine probe / auto-ack only scanned state/. Single-sourced the scan-dir set in a new state_root::quarantine_scan_dirs() (deduped) and drove both the sweep (cmd_cleanup::disk) and the probe/auto-ack (self_deploy::health) from it, so they can never diverge on where quarantines live.
• 🟡 Minor — orphaned .ack sidecars. Added reclaim_orphaned_ack_sidecars: a marker whose parent quarantine no longer exists is now reclaimed (even when the directory has no remaining candidates), so stale markers can't accumulate unbounded.
• LOW-1 — unescaped basename in cleanup output. Operator-facing paths in the corrupt-DB sweep now route through the shared sanitize_gate_detail control-char strip (re-exported from self_relaunch), matching the Debug-escaping used on the probe paths.
• LOW-2 — auto-ack marker logged via Display. Now logged via ?marker (Debug), consistent with the sibling artifact = ?name.

Tests: reworked the no_quarantine_scans_state_dir_not_top_level test into a probe/sweep parity contract (no_quarantine_scans_both_state_root_and_state_subdir) and added corrupt_db_sweep_reclaims_orphaned_ack_sidecar.

Validation: changed-module lib tests 78 passed; self_deploy_convergence 4/4; cargo clippy --lib 0 warnings. Pre-commit (release clippy) + pre-push (479 tests + clippy --all-targets --all-features) gates green.

[rysweet]

Step 17b — Comprehensive Code Review (independent pass)

Scope: main...pr-4488 — 25 files, +2751/−189. Implements #4469 (durable .ack quarantine acknowledgement that breaks the stuck-quarantine self-deploy deadlock without deleting the #2550 recovery asset) and #4470 (canary failing-test diagnosability + hermetic test isolation via serial_guard/serial_test).

Verdict: APPROVE (no blocking findings). The two LOW log-forgery findings from the earlier Step 17b review are now resolved in the committed tip; nothing new blocks merge.

---

Verification performed (independent)

Built and tested the changed modules against the committed tip 228d4d24:

• cargo test --lib --no-run → compiles clean (59s).
• cargo clippy --lib → 0 warnings, 0 errors.
• Targeted lib tests — 95 passed, 0 failed:
  • quarantine_ack 10 · self_deploy::health::probe_logic_tests 20 · gates:: 13 · self_health 6 · cmd_cleanup 40 · serial_guard 6.
• cargo test --test self_deploy_convergence → 4 passed, 0 failed.
• CI on the PR head: all required checks green (cargo-audit/deny/vet, coverage, pre-commit, install-real, e2e-dashboard, GitGuardian).

Checklist

• Code quality & standards — small, single-responsibility fns; every non-obvious decision documented; consistent error type (SimardError::PersistentStoreIo). Refactor of remove_old_corrupt_dbs into scan_quarantine_candidates / select_protected_asset / remove_old_corrupt_dbs_in single-sources the "protected asset" definition shared by the sweep and the probe auto-ack — they can no longer disagree.
• Test coverage — new quarantine_ack unit suite (path-traversal ../, sub/…, absolute, ..; TOCTOU/symlink refusal; idempotency), probe acknowledgement-aware tally tests, operator --acknowledge-quarantine end-to-end (live store never acked; idempotent), and self_deploy_convergence integration coverage. serial_guard meta-test extended to catch the OodaConfig::default() env-read blind spot (#4433).
• No TODOs / stubs / swallowed exceptions — none in production paths. Best-effort auto-ack/operator-ack failures are surfaced via structured tracing::warn! (not silently dropped), which is the correct semantic here (converge-or-log, never crash the probe).
• No unimplemented functions — none. unwrap()/expect() appear only in #[cfg(test)] code.
• Logic correctness — .ack sidecars are excluded before is_corrupt_quarantine_name everywhere it matters (the predicate matches *.ack via the .corrupt- infix, and the ordering is correct in scan_quarantine_candidates, tally_quarantine_files, and present_quarantine_artifacts). Filename-keyed markers mean a fresh corrupt-<newts> still reddens the probe. Auto-ack fires only for the aged (#2550) protected asset — fresh corruption stays ineligible.
• Edge cases — TOCTOU closed via symlink_metadata (no-follow) + create_new/O_EXCL + AlreadyExists re-check; planted symlink/dir at the sidecar path is refused, not written through; orphaned-sidecar reclamation handles a parent deleted out-of-band; UTF-8-boundary-safe truncation in sanitize_gate_detail; absent/unreadable dirs ⇒ empty/no-op.

Confirmed fixes of the prior review's findings

• LOW-1 (unescaped basename in cleanup stderr/report) — src/cmd_cleanup/disk.rs now routes every operator-facing path through sanitize_path_for_log → self_relaunch::sanitize_gate_detail (control-char strip, 4096-byte bound). ✔
• LOW-2 (auto-ack marker logged via Display) — src/self_deploy/health.rs now logs marker = ?marker (Debug), consistent with the artifact = ?name sibling. ✔

Minor, non-blocking observations

1. eprintln! retained in the cleanup sweep (disk.rs "Removing corrupt DB …"). The #4469 "tracing/OTel only" rule is honored on the auto-ack/probe paths; this is the pre-existing operator-CLI progress convention (now with a sanitized path arg), not a regression. Consider migrating cleanup progress to tracing in a future pass for uniformity.
2. auto_ack_stuck_recovery_asset return value discarded (let _ = … per dir in run_self_health_probe). The event is captured via tracing::warn!, but the SelfHealthReport doesn't surface "auto-ack fired this run." Optional: thread the acked basename into the report for operator visibility.
3. count_quarantine_files is now #[cfg(test)]-only and re-derives the exclusion logic of tally_quarantine_files. Documented as intentional (kept for the ack unit tests); acceptable, minor duplication.

None of these change behavior or block merge.

Evidence: local cargo test/clippy runs above + green CI on the PR head. Scoped to #4469/#4470; scheduler problem #3 (VerifyAndMergePr prioritization) and cross-repo #4 (azlin) are correctly out of scope for this PR.

[rysweet]

Step 17c — Security Review (independent pass)

Scope: main...pr-4488 — 25 files. Traced the full flow: quarantine artifact discovery → .ack path derivation → creation/read gating → cleanup/reclamation. Reviewed live files: quarantine_ack.rs, health.rs, cmd_cleanup/disk.rs, operator_cli/self_health.rs, self_relaunch/gates.rs, state_root.rs, self_deploy/mod.rs.

Verdict: ✅ No high-confidence exploitable vulnerabilities. No blocking findings.

The .ack sidecar mechanism and log-sanitization changes are well-hardened, with explicit and correct defenses for exactly the in-scope attack classes.

Security controls verified sound

1. Path traversal / arbitrary file write — SAFE. ack_marker_path gates every read/write through is_ackable_quarantine_basename, which requires the name to be exactly one Component::Normal equal to the whole string — rejecting .., ., /, \, absolute paths, and root/drive prefixes. Combined with is_corrupt_quarantine_name, only cognitive*.corrupt-* siblings directly under state_root are addressable. Marker is always state_root.join("{name}.ack") — no attacker-influenced traversal. (Tests cover the traversal vectors.)

2. TOCTOU / symlink — SAFE. acknowledge does no-follow symlink_metadata first, refuses any non-regular file, then opens with create_new(true) (O_EXCL), which never follows a symlink and fails closed. The AlreadyExists branch re-stats no-follow and only accepts a regular file — stat→open window correctly closed. Both reclamation paths use symlink_metadata/DirEntry::metadata and remove a planted symlink as a link (never following to its target) — no arbitrary delete. (Planted-symlink defense is tested.)

3. Authorization / quarantine bypass — ACCEPTABLE (by design). Automatic auto_ack_stuck_recovery_asset fires only on the single #2550 protected recovery asset and only after it ages past 30 days. Fresh corruption is never eligible and still reddens the probe. The quarantine filename is minted by LadybugDB (cognitive.corrupt-<ts>), not attacker-controlled memory content — so untrusted memory data cannot craft a filename to self-acknowledge. Markers are filename-keyed, so acking an old artifact never silences a new corruption event.

4. Injection — SAFE. sanitize_gate_detail collapses every char::is_control() (\n,\r,\t,NUL,ANSI ESC) to a space and bounds to 512 bytes on a UTF-8 boundary; untrusted cargo test output is routed through it. sanitize_path_for_log routes untrusted operator-visible paths through the same sanitizer, closing log forgery via crafted basenames. Operator CLI logs basenames via Debug (?name). The cargo test subprocess uses Command::new + separate .arg() (no shell) — no argument injection.

5. Sensitive data — SAFE. .ack payload is a fixed 13-byte constant (b"acknowledged\n"). No secrets/tokens/PII written to sidecars, logs, or diagnostics.

6. DoS / panic — SAFE. Untrusted lengths bounded (512 / 4096 bytes); truncation is char-boundary-safe (no panic on multibyte UTF-8). No unbounded recursion/loops on crafted filenames.

7. Deserialization — SAFE. .ack contents are never parsed — presence of a regular file is the only signal. No untrusted deserialization introduced.

Non-blocking note (informational — not a finding)

sanitize_gate_detail/sanitize_path_for_log use char::is_control(), which covers C0/C1 controls (all ASCII newlines) but not Unicode line/paragraph separators U+2028/U+2029. Not a realistic forgery vector for the console/log sinks here (standard parsers/terminals don't treat U+2028 as a record separator), so not raised as a finding — optional defense-in-depth only if these strings ever reach a Unicode-newline-aware sink.

Security checklist

• ✅ Security requirements met (deadlock-break preserves fresh-corruption detection)
• ✅ No new vulnerabilities (path traversal, TOCTOU/symlink, injection all defended + tested)
• ✅ Sensitive data handling — no secrets/PII in sidecars or logs
• ✅ Authorization — auto-ack narrowly scoped (single protected asset, 30-day age gate); filenames not attacker-controlled
• ✅ Injection — log sanitizer + ?-debug + no-shell subprocess

Independently verified against committed code on branch pr-4488.

[rysweet]

Step 17d — Philosophy Guardian Review (amplihack PHILOSOPHY.md)

Scope: main...pr-4488 — 27 files, +2751/−189. Implements #4469 (durable .ack quarantine acknowledgement) + #4470 (canary diagnosability + OODA test isolation).

Verdict: ✅ COMPLIANT — no blocking philosophy violations.

Compliance checklist

• Ruthless simplicity achieved. The health.rs/disk.rs refactor single-sources the "protected asset" definition so the quarantine sweep and the health probe can no longer drift apart — complexity removed, not added. quarantine_ack.rs (195 impl lines) solves the stuck-quarantine deadlock with the minimal mechanism: a filename-keyed .ack sidecar. No speculative abstraction layers, no future-proofing.

• Bricks & studs pattern followed. quarantine_ack.rs is a self-contained brick with ONE responsibility (durable ack lifecycle) and a small public contract (write_ack / has_ack / present_quarantine_artifacts); tests are co-located in the module. serial_guard.rs is an isolated test-support brick with a clean guard API. Module boundaries between self_deploy, self_relaunch, cmd_cleanup, and operator_cli are respected — no reach-through.

• Zero-BS implementation. No stubs, TODOs, unimplemented!, or placeholder returns in production paths (red-flag scan confirmed only test-scoped unwrap/expect). Errors propagate transparently via Result — no swallowed exceptions; the auto-ack path and probe surface failures rather than hiding them. This directly serves the philosophy's "error visibility" and "fail loudly" tenets, which matter most for a self-deploy safety mechanism.

• No over-engineering / proportionality respected. Test-to-impl ratios are concentrated exactly where the philosophy sanctions them — security-critical paths (health.rs probe logic, gates.rs sanitizer) carry the heaviest coverage (5:1–15:1 band for critical paths), while simpler modules stay lean. Security hardening (TOCTOU close via symlink_metadata + O_EXCL, path-traversal refusal, UTF-8-safe truncation) is justified complexity under "Areas to Embrace Complexity → Security / Data integrity," not gold-plating.

• Clean module boundaries. New docs (self-deploy-quarantine-acknowledge.md, overseer-deploy-canary-diagnostics.md) document the studs; the operator CLI consumes the module through its public surface only.

Minor, non-blocking observations

1. src/test_support/serial_guard.rs (1053 lines) is a large test harness. It is infrastructure and well-factored, but worth watching against the proportionality principle — if future test-isolation needs stay narrow, prefer trimming over extending.
2. Residual eprintln! in the cleanup path (already noted in Step 17b) is a diagnostics-visibility choice, consistent with "fail visibly," but could route through the structured logger for consistency.

Conclusion: The PR embodies ruthless simplicity (net complexity reduction via single-sourcing), the brick/stud model, and zero-BS error handling. Added complexity is confined to security and data-integrity concerns the philosophy explicitly permits. Approved on philosophy grounds.

[rysweet]

Step 18b — Review Feedback Implementation: complete.

All three Step 16/17 reviews (code, security, philosophy) returned APPROVE / COMPLIANT with zero blocking findings. No code changes are required for merge.

The non-blocking suggestions (residual eprintln! routing, explicit auto-ack discard, Unicode line-separator sanitization, serial_guard.rs size watch, test-helper de-dup) were intentionally not folded into this PR to keep it focused on #4469/#4470 and consistent with ruthless-simplicity. They are captured for follow-up in #4496.

PR is mergeable as-is; CI green.

[rysweet]

Step 17b — Comprehensive Code Review (PR #4488)

Scope: 27 files, +2751/-189. Self-deploy red-canary convergence for the stuck-quarantine deadlock (#4469) + canary diagnosability (#4470) + OodaConfig::default() serial-test race hardening (#4433).

Verdict: APPROVE (no blocking findings). Verified locally — all touched test groups compile and pass:

• self_deploy::quarantine_ack — 10 passed
• self_deploy::health::probe_logic_tests — 20 passed
• self_relaunch::gates::tests — 13 passed
• cmd_cleanup — 40 passed
• tests/self_deploy_convergence — 4 passed

All 17 required CI checks are green; mergeStateStatus: CLEAN, mergeable: MERGEABLE.

Review checklist

• Code quality and standards — idiomatic Rust, cohesive modules, docs explain the why (deadlock rationale, single-sourcing) not just the what.
• Test coverage adequate — TDD-style unit tests + an outside-in end-to-end convergence contract; edge cases (idempotency, fresh-after-ack, unsafe names, aged vs. fresh protected asset, orphaned sidecars) all covered.
• No TODOs, stubs, or swallowed exceptions — every error path returns Err or logs a structured WARN; no todo!/unimplemented!. All .unwrap() occurrences are in test code only.
• No unimplemented functions.
• Logic correctness — see notes below; verified sound.
• Edge case handling — thorough.

Strengths (high confidence)

1. Security posture is excellent. acknowledge() uses symlink_metadata + OpenOptions::create_new(true) (O_EXCL) to close the TOCTOU window and refuses to write through a planted symlink/dir sidecar; a dedicated #[cfg(unix)] test proves the victim file is untouched. Path-traversal / separator / absolute / .. names are rejected via a single-Component::Normal check.
2. Log-forgery hardening (#4470/#4469 LOW-1/LOW-2). Untrusted quarantine basenames and cargo-test output are neutralized: sanitize_gate_detail strips control chars, collapses whitespace, and bounds output UTF-8-boundary-safely; operator-facing paths route through sanitize_path_for_log; tracing sites use Debug (?name) to escape control chars.
3. Single-sourcing prevents drift. state_root::quarantine_scan_dirs(), is_corrupt_quarantine_name, and select_protected_asset are shared by the cleanup sweep and the health probe, so the two can never disagree about where quarantines live or which is the #2550 protected asset — directly eliminating the root cause of the deadlock.
4. Non-destructive + idempotent + filename-keyed. Acknowledging retains the recovery asset; a stale ack never silences a new corruption event; orphaned .ack markers are reclaimed even when the directory has zero live quarantines.

Non-blocking observations

1. Health probe now has a write side-effect. run_self_health_probe calls auto_ack_stuck_recovery_asset per scan dir, which writes a durable .ack sidecar for an aged (>= CORRUPT_DB_MAX_AGE_DAYS) protected asset. A conceptually read-mostly probe now mutates the state root autonomously. It is well-guarded (aged and >= CORRUPT_DB_PROTECT_MIN_BYTES), documented, and best-effort (WARN + continue on failure), and the deadlock rationale justifies it. Flagging for awareness only — fresh corruption remains ineligible and still reddens the probe, so the fail-closed contract is preserved.
2. eprintln! in the cleanup sweep (remove_old_corrupt_dbs_in) is retained (pre-existing) for the operator-facing removal report. This is a CLI cleanup path (not the probe path), so stderr is appropriate; the path argument is now sanitized. No action required.
3. Minor redundant I/O: aged_protected_recovery_asset scans the dir, then acknowledge re-stats the same sidecar, run once per scan dir on the probe path. Negligible (small dirs) — noted only for completeness.

Tie-break correctness

select_protected_asset uses max_by(size).then_with(modified), and Iterator::max_by returns the last maximal element, so size ties resolve to the newest artifact — matching the documented "ties → newest" contract. Correct.

Nothing here blocks merge. Clean, well-tested, security-conscious work.

_{🤖 Automated Step 17b review — Copilot CLI}

[rysweet]

Step 17c — Security Review (PR #4488)

Scope: 27 files, +2751/-189 (self-deploy red-canary convergence: #4469 / #4470 / OodaConfig serial-test race). Read-only security analysis focused on the write/delete/relaunch primitives: quarantine_ack.rs, cmd_cleanup/disk.rs, self_deploy/health.rs, self_relaunch/gates.rs, state_root.rs, operator_cli/self_health.rs.

Verdict: APPROVE (security) — no high-confidence, exploitable findings.

Claimed defenses — verification

Claimed defense	Verdict	Evidence
TOCTOU-safe O_EXCL writes	✅ HOLDS	quarantine_ack.rs:141-144 uses OpenOptions::create_new(true) → O_CREAT|O_EXCL (atomic, does not follow a final-component symlink). The prior symlink_metadata (:127) is only an idempotency fast-path; the guarantee is the create_new open. The AlreadyExists race branch (:148-156) re-stats and refuses non-regular files. No check-then-open write gap.
Symlink defense	✅ HOLDS	Write: O_EXCL never follows a final-component symlink; lost-race path refuses anything not a regular file (:149-155). Delete: disk.rs:432 uses DirEntry::metadata() (no symlink traversal) so a planted symlink is removed via remove_file (link only, :562-566); reclaim_orphaned_ack_sidecars re-checks symlink_metadata and removes only genuine regular files (disk.rs:656-673).
Path-traversal rejection	✅ HOLDS	is_ackable_quarantine_basename (quarantine_ack.rs:60-78) rejects empty///\ and, via Path::components(), requires exactly one Component::Normal == whole name — rejecting .., ., absolute/root/drive prefixes before the state_root.join(...) at :91.
Control-char / log-forgery sanitization	✅ HOLDS	Ack payload is a fixed constant (ACK_MARKER_BYTES = b"acknowledged\n") — no attacker data is written into file contents. Untrusted basenames reaching logs go through Debug (?name) or sanitize_gate_detail/sanitize_path_for_log, which strip all char::is_control() chars (gates.rs:280-289, disk.rs:620-622).

Additional checks (all clean)

• Command / argument injection: all subprocess spawns (gates.rs:37,64-69,112,137-138, canary.rs:30-36,157,169) use Command::new().arg(...) argument vectors — no shell, no format!-built command strings. Relaunch binary/args come from RelaunchConfig, not attacker-controllable filenames.
• cargo test output parsing (extract_first_failure / sanitize_gate_detail) is bounded to 512 bytes and control-char-stripped — no forgery into GateResult.detail.
• Autonomous auto-ack side-effect (health.rs:260-297): only writes the fixed-content .ack sidecar for the single aged, ≥1 MB protected asset chosen via the shared aged_protected_recovery_asset; deletes/executes nothing, re-validates the name. A fresh unacknowledged corruption still reddens the probe, so it cannot mask new corruption.
• Deletion scope (disk.rs:368-371): is_corrupt_quarantine_name requires the .corrupt- infix, so the live store (cognitive, cognitive.wal) is never eligible for deletion/ack.

File permissions

Sidecars are created without an explicit mode → default 0o666 & ~umask (typically 0o644); not world-writable under any normal umask. Contents are a non-sensitive presence flag — no sensitive-data-at-rest concern. No directories are created by this code.

Security checklist

• Security requirements met
• No new vulnerabilities introduced
• Sensitive data handling (acks contain no sensitive/attacker data; safe permissions)
• Authorization/gates reviewed (relaunch gates cannot be bypassed via attacker-controllable filesystem state)
• Injection review (no path traversal, symlink escape, TOCTOU, command/log injection)

Residual (non-reportable) note

All primitives presuppose an attacker who already holds write access to the state root. Within that model the code correctly prevents traversal, symlink escape, and TOCTOU. The only reachable effect of a planted aged ≥1 MB cognitive*.corrupt-* file is self-silencing of the no_quarantine probe for that one artifact — which requires pre-existing filesystem write privilege and yields no traversal, escape, arbitrary write/delete, or code execution. Below reporting threshold.

[rysweet]

Step 17d — Philosophy Guardian Review (PR #4488)

Scope: self-deploy red-canary convergence — quarantine_ack.rs (new), health.rs, gates.rs, cmd_cleanup/disk.rs, operator_cli/self_health.rs, state_root.rs, test_support/serial_guard.rs, ooda_loop/decide.rs, + mod exports & tests. Advisory only — no code modified.

Compliance Checklist

• Ruthless simplicity achieved — added complexity (O_EXCL/TOCTOU, symlink & path-traversal defense, log-forgery sanitization) is concentrated and legitimately justified by the security & data-integrity carve-out; no gratuitous abstraction.
• Bricks & studs pattern followed — quarantine_ack.rs is a textbook brick: ONE responsibility, documented ## Contract header, colocated tests, narrow re-export surface. Cross-module divergence eliminated by single-sourcing (quarantine_scan_dirs, is_corrupt_quarantine_name, select_protected_asset).
• Zero-BS implementation — no stubs/TODOs/unimplemented!/production panic!; all unwrap()/expect() are test-only; errors visible (acknowledge returns SimardResult, best-effort auto-ack emits structured tracing::warn! rather than swallowing).
• No over-engineering — abstractions earn their place; probe/sweep single-sourcing fixes the actual root-cause bug rather than speculative future-proofing.
• Clean module boundaries — mostly clean and single-sourced (one minor cross-module utility-coupling note below).

Per-Principle Verdict

Principle	Verdict
Ruthless Simplicity / KISS	PASS
Bricks & Studs	PASS
Zero-BS (no stubs/swallowed exceptions)	PASS
No over-engineering	PASS-WITH-NOTES
Clean module boundaries	PASS-WITH-NOTES
Proportionality (test/doc ratio)	PASS

Overall Verdict: COMPLIANT-WITH-NOTES — ✅ approve, no blocking issues

Genuinely clean, not a rubber-stamp. The design attacks the actual deadlock (probe vs. sweep scanning different directory sets) by making both consume one source of truth; security hardening (O_EXCL closing the stat→open TOCTOU window, symlink_metadata refusing to write through planted symlinks, Debug-escaped logging of untrusted basenames, UTF-8-boundary-safe bounded sanitization) is correct and test-covered incl. a Unix symlink-plant test; non-destructive by construction (recovery asset retained, only acknowledged).

Blocking Issues

None.

Non-Blocking Notes

1. ack_marker_path is pub-re-exported but has no external consumer — consider narrowing the stud surface to crate-private until a caller exists.
2. count_quarantine_files (#[cfg(test)]) duplicates acknowledgement-aware scan logic in tally_quarantine_files; prefer tests exercising the production path to avoid drift.
3. Cross-module utility coupling: cmd_cleanup::disk → self_relaunch::sanitize_gate_detail; the shared control-char/log-forgery sanitizer would sit more naturally in a neutral util module.
4. let _ = auto_ack_stuck_recovery_asset(&dir); discards a result — acceptable (errors already logged internally); a one-word comment reinforcing "best-effort; errors logged" would remove reviewer doubt.

None affect correctness, safety, or data integrity.

Posted by philosophy-guardian (Step 17d).

[rysweet]

Step 17b — Comprehensive Code Review (PR #4488)

Verdict: APPROVE. High-quality, security-conscious implementation of the #4469 stuck-quarantine deadlock fix (plus #4470 canary diagnosability). Verified locally: crate compiles and all 50 targeted tests pass (quarantine 37, gates::tests 10, log_sanitize 3).

Review checklist

• Code quality and standards — clean, well-documented, single-sourced shared logic (is_corrupt_quarantine_name, select_protected_asset, quarantine_scan_dirs) so the sweep and the health probe can never disagree.
• Test coverage adequate — extensive: TOCTOU races, planted-symlink defense, idempotency, filename-keyed freshness, aged/fresh/trivial auto-ack eligibility, UTF-8 boundary bounding, first-failure extraction bounding.
• No TODOs, stubs, or swallowed exceptions — best-effort paths log structured tracing::warn with the error; no silent swallowing. All .unwrap() occurrences are test-only.
• No unimplemented functions.
• Logic correctness — verified.
• Edge case handling — thorough (absent/unreadable dirs, orphaned sidecars, ties → newest, missing mtime fail-safe).

Strengths

1. Security hardening is excellent. acknowledge() closes the TOCTOU window with create_new/O_EXCL, refuses to write through a non-regular-file sidecar (planted symlink/dir), and rejects path-traversal / absolute / non-quarantine names. Log-forgery is neutralized both via util::log_sanitize (cleanup path) and Debug-escaping (?name) on the probe path.
2. Single-sourcing. Extracting quarantine_scan_dirs() and select_protected_asset() eliminates the probe/sweep divergence that caused the original deadlock — the right structural fix, not a patch.
3. Non-destructive by construction. The #2550 recovery asset is retained; auto-ack only fires once the protected asset ages past the forensic window, and never for fresh corruption.
4. Orphaned-sidecar reclamation (reclaim_orphaned_ack_sidecars) closes the unbounded-marker-growth gap when a parent is deleted out-of-band.

Observations (non-blocking)

• run_self_health_probe now has a write side effect. The probe path calls auto_ack_stuck_recovery_asset(), so a nominally read-only "health probe" now writes a durable .ack sidecar. This is intentional and documented (it must fire for the unattended orchestrator post-deploy check), and it is tightly guarded (aged protected asset only). Flagging only because "probe mutates state" can surprise future readers — the inline comment already explains the rationale well.
• eprintln! remains in remove_old_corrupt_dbs_in (the "Removing corrupt DB …" line). This is the pre-existing pattern for the interactive cmd_cleanup CLI (not the daemon/probe path, which correctly uses tracing), and the path is now routed through sanitize_path_for_log, so it is not a regression. No change requested.
• is_ackable_quarantine_basename cross-platform. The single-Normal-component check plus / and \\ rejection is robust on the Unix daemon target; Windows drive-relative forms aren't a concern here.

Nothing blocking. Ready to merge once required CI checks are green.

[rysweet]

Step 17c — Security Review (PR #4488)

Verdict: PASS / APPROVE (security). No high- or medium-severity vulnerabilities found in the #4469 (durable quarantine .ack) + #4470 (canary diagnosability) changes. The implementation is notably security-conscious: it treats on-disk quarantine basenames as untrusted input and defends every sink (filesystem writes, destructive sweeps, operator logs).

Threat model

A corrupt-quarantine artifact only needs the cognitive* prefix + .corrupt- infix to be matched, so any principal with write access to the state root can plant a file/symlink with an attacker-chosen basename (including control chars). Reviewed against that adversary.

Security requirements — verified

Check	Result	Evidence
Path traversal (.., separators, absolute)	✅ Defended	is_ackable_quarantine_basename rejects /, \, empty, .ack, and requires exactly one Component::Normal equal to the whole name; delegates to canonical is_corrupt_quarantine_name (quarantine_ack.rs:60-78). Tests cover ../, /etc/passwd, sub/….
Symlink attack on sidecar write	✅ Defended	acknowledge does symlink_metadata (no follow) then OpenOptions::create_new (O_EXCL, never follows a symlink, fails if path exists). Planted-symlink test asserts victim file is untouched (quarantine_ack.rs:123-158, 301-320).
TOCTOU on sidecar write	✅ Defended	O_EXCL closes the stat→open window; AlreadyExists re-checks for a regular file before treating as idempotent success.
Log-line forgery / terminal-control injection	✅ Defended	util::log_sanitize::sanitize_to_single_line collapses all char::is_control() runs (CR/LF/TAB/ANSI ESC/NUL) to a single space and bounds length on a UTF-8 boundary. Sweep routes every operator-visible path through sanitize_path_for_log; probe/CLI log the untrusted name via Debug (?name), which escapes control chars.
Symlink-following destructive delete	✅ Safe	DirEntry::metadata() does not traverse symlinks, so a symlink named cognitive.corrupt-* → external dir yields is_dir=false and is removed with remove_file (unlinks the link, not the target). remove_dir_all only runs on a genuine directory entry (disk.rs:438).
Orphaned .ack reclamation symlink safety	✅ Defended	reclaim_orphaned_ack_sidecars / reclaim_ack_sidecar gate on symlink_metadata(...).file_type().is_file(); non-regular files are left untouched (disk.rs:249-273, 649-690).
Env-var injection (SIMARD_STATE_ROOT)	✅ Defended	sanitized_env_state_root rejects empty, NUL-bearing, and non-absolute values with a one-shot WARN (state_root.rs:123-144).
DoS via hostile long basename	✅ Bounded	PATH_LOG_MAX_BYTES=4096, GATE_DETAIL_MAX_BYTES=512 cap all untrusted strings rendered to logs.
Sensitive-data handling	✅ OK	Sidecar payload is a fixed 13-byte presence flag (acknowledged\n) — no secrets, no attacker-controlled content persisted. No credentials/tokens logged; only sanitized basenames + counts.
AuthN/AuthZ	✅ N/A-appropriate	--acknowledge-quarantine is a local operator CLI action requiring filesystem access to the state root; non-destructive (never deletes the #2550 recovery asset), idempotent, best-effort per-artifact so one hostile entry can't block clearing the rest.
Command / shell injection	✅ None	No process::Command/shell interpolation of untrusted data on these paths; untrusted basenames only reach fs APIs and sanitized log sinks.

Non-blocking observations

1. INFO — sanitizer scope. sanitize_to_single_line neutralizes C0/C1 controls (the real log-forgery vectors) but not Unicode format/bidi chars (e.g. U+202E RIGHT-TO-LEFT OVERRIDE), which can still visually reorder text in some log/terminal viewers. Very low risk for single-line operator output; consider also stripping the Unicode Cf category if spoofing-resistant operator logs become a requirement.
2. INFO — relative last-resort root. home_default() falls back to a relative ./.simard when neither HOME nor dirs::home_dir() resolves. SIMARD_STATE_ROOT itself is required to be absolute, so this only affects the no-HOME edge case; benign and pre-existing, but worth noting since a relative state root places artifacts under CWD.
3. INFO — platform coverage. The planted-symlink refusal test is #[cfg(unix)]. On Windows, create_new/O_EXCL still prevents overwrite and symlink creation is privileged, so the invariant holds; no action needed.

Verification

Crate compiles; targeted security-relevant tests pass locally (quarantine 37, gates::tests 10, log_sanitize 3). Path-traversal, planted-symlink, idempotency, and control-char-stripping cases are all covered by tests.

Conclusion: No injection, path-traversal, symlink, TOCTOU, or sensitive-data findings block merge. Approve from a security standpoint; the three INFO items are optional hardening, not required.

[rysweet]

Step 17d — Philosophy Guardian Review (PR #4488)

Verdict: PASS. The #4469 stuck-quarantine deadlock fix + #4470 canary diagnosability changes comply with the project philosophy (ruthless simplicity, bricks & studs, Zero-BS, no over-engineering, clean module boundaries).

Compliance checklist

• Ruthless simplicity achieved — The structural fix single-sources the probe/sweep directory and protected-asset logic instead of duplicating it (state_root.rs, shared helpers). New behavior (TOCTOU-safe O_EXCL acknowledge, retention, orphan reclamation) is implemented with the minimal mechanism required; no speculative config or future-proofing layers.
• Bricks & studs pattern followed — New functionality lands as self-contained bricks with narrow public studs: self_deploy/quarantine_ack.rs (acknowledge/reclaim contract), util/log_sanitize.rs (single sanitizer stud, re-exported via util/mod.rs), state_root.rs (one resolver). Each module owns its tests; boundaries are explicit and regeneratable.
• Zero-BS implementation — No stubs, TODOs, unimplemented!, faked APIs, or dead code in production paths. Every .unwrap() is test-only. Best-effort/optional paths (sidecar cleanup, telemetry) skip visibly with structured tracing::warn; strict paths surface errors — no swallowed exceptions.
• No over-engineering — Security hardening (symlink/path-traversal defense, log-forgery sanitization, O_EXCL) is proportionate to a self-deploying agent that touches the filesystem; it addresses concrete threats in the changed flow, not hypothetical ones. Retention is non-destructive by default rather than adding a policy engine.
• Clean module boundaries — Health probe, quarantine acknowledge, disk cleanup, relaunch gates, and log sanitization are each isolated with defined contracts; the fix removes prior duplication between the probe and sweep paths, tightening (not blurring) boundaries. Docs are colocated under docs/howto, docs/reference, docs/testing and wired into mkdocs.yml.

Non-blocking observations

1. The health probe now performs an intentional write side effect (acknowledge marker); documented and test-covered — acceptable, but worth a one-line comment at the call site so future regenerators preserve the invariant.
2. Pre-existing eprintln! in the operator CLI cleanup path predates this PR; not introduced here.
3. Basename/symlink defense is Unix-only, consistent with the deployment target.

Result: Philosophy-compliant. Nothing blocking merge.

[github-actions]

📊 Coverage Summary

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

Module	Lines	Covered	Coverage
Total	196016	164720	84.0%

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