Update documentation with 11 changed files (#4809)

[rysweet]

Summary

Concise workflow-generated PR for documentation.

Issue

Closes #4809

Changed files

• docs/concepts/agentic-disk-reclamation.md
• docs/howto/configure-disk-reclamation.md
• docs/reference/disk-reclaim-api.md
• docs/reference/disk-reclaim-deterministic-enumeration.md
• docs/reference/disk-reclaim-telemetry.md
• mkdocs.yml
• src/disk_health.rs
• src/disk_reclaim/executor.rs
• src/disk_reclaim/guard.rs
• src/disk_reclaim/mod.rs
• src/disk_reclaim/reclaimable.rs

Diff stat

 docs/concepts/agentic-disk-reclamation.md          |  45 +-
 docs/howto/configure-disk-reclamation.md           |  39 +-
 docs/reference/disk-reclaim-api.md                 |  80 +++-
 .../disk-reclaim-deterministic-enumeration.md      | 297 ++++++++++++
 docs/reference/disk-reclaim-telemetry.md           |   3 +
 mkdocs.yml                                         |   1 +
 src/disk_health.rs                                 | 127 ++++-
 src/disk_reclaim/executor.rs                       |  71 +++
 src/disk_reclaim/guard.rs                          |  60 +++
 src/disk_reclaim/mod.rs                            |  65 ++-
 src/disk_reclaim/reclaimable.rs                    | 515 +++++++++++++++++++++
 11 files changed, 1284 insertions(+), 19 deletions(-)

Behavior

Implemented behavior through these branch commits:
371c8b5 test(disk_reclaim): add cognitive_memory serial key to idle-day env tests
27c91d7 fix(disk-reclaim): deterministic reclaimable floor so routine reclaim frees space before emergency (#4809)

Validation

ℹ NODE_OPTIONS=--max-old-space-size=32768 (saved preference). To change: /home/azureuser/.amplihack/config
I'll start by getting the branch name and understanding what changes have been made.

● Recent commit log (shell)
│ cd /home/azureuser/src/Simard-deploy-4049/worktrees/feat/issue-4809-fix-simards-ineffective-disk
│ -reclaim-so-routine-re && git --no-pager log --oneline -10
└ 11 lines…

● Get branch name and git status (shell)
│ cd /home/azureuser/src/Simard-deploy-4049/worktrees/feat/issue-4809-fix-simards-ineffective-disk
│ -reclaim-so-routine-re && git branch --show-current && echo "---STATUS---" && git --no-pager
│ status && echo "---DIFF STAT---" && git --no-pager diff --stat HEAD

Risk

No high-risk subsystem pattern detected from changed paths.

Checklist

• Branch has 2 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 from PR branch feat/issue-4809-fix-simards-ineffective-disk-reclaim-so-routine-re as a consumer of the disk-reclaim subsystem would — through the simard disk operator CLI boundary and the crate's public test surface.

Detected toolchain: Rust CLI (Cargo.toml at root, edition 2024, rustc 1.95.0). Package manager: cargo. Outside-in boundary: the simard disk report|reclaim operator subcommand (src/operator_cli/disk.rs) plus the guarded reclaim core.

Chosen strategy: Per the qa-team skill's Rust-CLI guidance, use native cargo test for the changed modules as the consumer-boundary parity surface, then exercise the built simard disk binary end-to-end as an external operator would. No gadugi framework required.

#	Scenario	Command	Result	Key output
1	Simple — disk report vets an idle build tree and deletes nothing	simard disk report --path <tmp>/self-deploy-target	PASS	disk reclaim: 95% -> 95% used, freed 0 bytes, 0 paths removed, 1 skipped for review; exit 0; 5 MB artifact still present
2	Edge — disk reclaim --dry-run proposes but performs zero destructive ops	simard disk reclaim --dry-run --path <tmp>/canary-repro-target	PASS	exit 0; 8 MB artifact still present after dry-run
3	Unit/integration — deterministic reclaimable floor + guard vetting	cargo test --lib disk_reclaim	PASS	90 passed; 0 failed (incl. deterministic_floor_is_non_empty_under_the_stale_scenario, idle_build_tree_contents_are_proposed_strictly_inside_the_root, live_cognitive_and_snapshot_state_is_never_enumerated)
4	Emergency-path parity with shared reclaimable set	cargo test --lib disk_health	PASS	13 passed; 0 failed (incl. emergency_cleanup_reclaims_idle_self_deploy_target_and_retains_prior_removals)
5	simard disk CLI dispatch / guard refusals	cargo test --lib operator_cli::disk::	PASS	25 passed; 0 failed (incl. dry_run_performs_zero_destructive_ops, reclaim_refuses_path_outside_allow_roots, reclaim_refuses_protected_main_even_when_named)

Safety-constraint checks: No println!/print! introduced in changed library code (structured tracing + OTel only) — verified via diff scan. Dry-run and report modes perform zero deletions. Guard re-vets every deterministic-floor candidate at the syscall boundary; live cognitive/snapshot state is never enumerated.

Fix count: 0 — all scenarios passed on the first run; no diagnose/fix/commit iterations were required.

[rysweet]

Step 17b — Comprehensive Code Review

Verdict: Approve-quality. No blocking defects. The change is well-guarded, well-tested, and its central invariant (every candidate — LLM or deterministic — is re-vetted by the guard, with no "trusted internal" shortcut) is proven by tests. Reviewed the true PR diff (merge-base 7fe267ad1..HEAD); verified all new tests green first-hand (reclaimable 17 passed, emergency_cleanup 2 passed).

Checklist

• Code quality / standards — idiomatic Rust, clear docs, single-source-of-truth build_tree_roots shared by routine + emergency tiers.
• Test coverage adequate — enumerator classification, guard re-vetting of enumerated candidates (protected-path + outside-allow-root), emergency parity, root→dry-run downgrade, env clamping. Strong seams (FakeLiveProcessProbe, MapMeasurer, ScriptedDisk, RecordingRemover, Harness).
• No TODOs/stubs/swallowed exceptions — none introduced (see minor note #2).
• No unimplemented functions.
• Logic correctness — fail-closed is_idle and env clamping (0/empty/negative → safe floor, never "purge now"); dedup preserves agent metadata; children (never roots) proposed so guard containment holds.
• Edge case handling — live-PID withholding, sub-idle-window withholding, unreadable-dir → empty, mtime-in-future → not idle.

Strengths

• No self-trust for deterministic candidates. enumerated_candidate_at_a_protected_path_is_still_rejected and ..._outside_allow_roots_is_still_rejected prove the Rust enumerator cannot widen scope or bypass rails — the key safety property.
• Tier convergence. Routine allow_roots widening and emergency_cleanup both consume build_tree_roots, structurally eliminating the divergence that let emergency re-delete only target/debug.
• Defense-in-depth root downgrade (effective_apply_mode) at the guarded core, so every caller inherits the apply→dry-run-as-root invariant.

Findings (all non-blocking)

1. [Low] emergency_cleanup_with_pct bypasses vet_candidate/is_safe_to_delete. Section 5 calls std::fs::remove_dir_all(&root) directly on build_tree_roots, guarded only by the live-PID probe — not routed through the guard the routine path uses. This is consistent with the pre-existing emergency pattern (target/debug etc. are also removed directly) and is PID-guarded last-resort at ≥95%, so acceptable here. Recommend a follow-up PR to route emergency removals through the shared guard for a single enforcement path.

2. [Low / silent-failure] Removal error is dropped without a structured log. if std::fs::remove_dir_all(&root).is_ok() { ... } discards the Err on the failure branch — no tracing::warn!. This nicks R7 ("failures surfaced via structured logs"). It mirrors the pre-existing target/debug pattern, but the PR adds a new instance; a warn!(path, error) on the else branch would keep failures observable.

3. [Informational / scope] Named artifacts from the stated requirements are intentionally not enumerated. canary-repro-target/bootstrap-target (Decision 4) and stale cognitive.* snapshot backups (Decision 3/R1a) are absent. I verified canary-repro-target/bootstrap-target are referenced nowhere in src/ and are created by no code — so substituting the real cargo-target/shared-target caches is the correct real-world choice, not a gap. Snapshot backups are deliberately deferred to MaintenanceThread (its own keep-N/age floor) to avoid double-counting/races — a safer design, and live_cognitive_and_snapshot_state_is_never_enumerated locks it in. This diverges from the written Decision 3 but is the sounder call; recommend confirming with the issue owner that MaintenanceThread ownership satisfies the AC, or filing a follow-up if snapshot reclamation is still wanted.

4. [Minor / edge] is_idle keys on the build-root's own mtime before enumerating all children. A directory's mtime refreshes only on direct child add/remove, so deep writes into an existing subtree may not update it. The live-PID probe mitigates the dangerous active-build case (an in-progress build has a live process), so this is acceptable, but worth noting for future tuning.

5. [Nit] PR title is misleading. "Update documentation with 11 changed files (#4809)" is auto-generated; this is a substantive behavior fix (deterministic reclaim floor), not a docs update. Suggest retitling to match the fix(disk-reclaim): ... commit subject.

Recommendation

Mergeable as-is. Items #1–#2 are worthwhile hardening follow-ups; #3 warrants a one-line confirmation from the issue owner on snapshot-reclamation ownership; #4–#5 are informational.

[rysweet]

Step 17c — Security Review

Verdict: No blocking security findings. Approve from a security standpoint. This change adds a deterministic reclaimable-set enumerator (issue #4809) whose central invariant is sound: every candidate it proposes — like every LLM candidate — is re-vetted at the syscall boundary by guard::vet_candidate, which composes the audited is_safe_to_delete primitive. There is no "trusted internal" bypass, and this is proven by two new negative guard tests.

Security requirements — all met

Requirement	Status	Evidence
No new deletion path bypasses the guard (routine tier)	✅	run_disk_reclaim feeds reclaimable_targets() through reclaim_candidates → vet_candidate; new tests enumerated_candidate_at_a_protected_path_is_still_rejected and enumerated_candidate_outside_allow_roots_is_still_rejected prove no self-trust.
Path traversal (..) defeated	✅	is_safe_to_delete canonicalizes before the containment check (maintenance.rs:243).
Symlink swap / TOCTOU (SR-5)	✅	symlink_metadata refuses a final-component symlink pre-canonicalization; rail_refuses_symlink_candidate covers it.
Containment cannot be widened	✅	"strictly inside" (real != r && real.starts_with(r)); allow-root widening is a closed leaf set (build_tree_roots = only self-deploy-target), asserted to never span bare state_root or $HOME.
Env-driven config cannot escalate delete scope	✅	idle_days_from_env clamps 0/empty/non-numeric/negative back to the safe floor (never "purge now"); SIMARD_GIT_PROTECTED_REPOS only widens the deny-set, never the allow-set.
Mislabelled kind cannot skip git vetoes	✅	Rail 4 forces any path with a .git entry through the tracked-worktree (dirty/unpushed/PR-state) vetoes regardless of the advisory kind, closing the "label a worktree as stale_build_cache to rm -rf unpushed work" bypass.
Live-data protection	✅	Deny-set + protected_paths shield the live cognitive/cognitive.wal/cognitive.shadow store and snapshot/backup dirs; enumerator scans only regenerable build roots + engineer-worktrees children.
Fail-closed on ambiguity	✅	is_idle, is_safe_to_delete, and every rail return not-safe/Reject on any stat/canonicalize/parse error.

Sensitive-data handling

No secrets, credentials, tokens, or PII are read, logged, or transmitted. Telemetry logs only paths and byte counts. No network I/O, no auth/authz surface, no deserialization of untrusted input. Injection vectors (command/path/env): none introduced — removals use std::fs primitives and (for worktrees) git worktree remove, not shell string interpolation.

Findings (all non-blocking)

1. [Low] Emergency tier's direct remove_dir_all bypasses is_safe_to_delete (no symlink/canonicalize refusal).
emergency_cleanup_with_pct (disk_health.rs) removes build_tree_roots(state_root) via std::fs::remove_dir_all directly, guarded only by is_dir() + the live-PID probe — it does not route through the canonical guard. is_dir() follows a symlink, so if <state_root>/self-deploy-target were replaced by a symlink to a sensitive tree, the wholesale remove could act on the link's parent-resolved target.

• Actual risk: Low. The path is a hardcoded, non-attacker-influenced join; state_root is Simard-owned; this is the pre-existing pattern used by the other emergency removals (target/debug, cargo-target, etc.) in the same function — not introduced by this PR.
• Suggested hardening (follow-up, out of scope): add a symlink_metadata().is_symlink() guard (or reuse is_safe_to_delete against a self-deploy-target allow-root) before the emergency remove_dir_all, so both tiers share the same symlink-refusal posture.

2. [Low] Removal Err is dropped via .is_ok() without a warn!.
In both the emergency build-tree loop and the enumerated-removal path, a failed removal is silently skipped (if remove_dir_all(..).is_ok()). Not a security defect, but it violates the "surface failures" rule and could mask a persistent permission/racing condition. Recommend an else { warn!(...) }.

3. [Info] No privilege/UID checks before destructive ops.
Consistent with the existing daemon model (runs as the Simard service user). No escalation introduced. Noting only that the blast radius is bounded entirely by the allow-roots + deny-set, which this review confirms are correctly constrained.

Strengths

• Guard re-vets deterministic candidates identically to LLM ones (no internal trust) — the highest-value invariant, and it's test-locked.
• Fail-closed env clamping means a misconfigured *_IDLE_DAYS=0 cannot collapse the idle window to an immediate purge.
• Single source of truth (build_tree_roots) for allow-root widening prevents routine/emergency divergence.
• Explicit, asserted exclusion of state_root and $HOME from allow-roots eliminates the most dangerous over-broad-containment class.

Recommendation: Mergeable from a security standpoint. Finding #1 (emergency symlink-refusal parity) is worth a one-line follow-up hardening PR but is pre-existing and low-risk, not a merge blocker.

[rysweet]

Step 17d — Philosophy Guardian Review

Reviewed the true PR diff (7fe267ad1..HEAD, 11 files / +1284) against PHILOSOPHY.md. Verdict: COMPLIANT — mergeable. No blocking philosophy violations.

Compliance checklist

• Ruthless simplicity — The core is one pure function tier: reclaimable_targets enumerates a closed, explicit set of leaf directories (build_tree_roots → self-deploy-target; shared cargo-target/shared-target/engineer-worktrees already in allow_roots). No speculative abstraction, no config framework — just env reads with defensive clamps. Occam-clean.
• Bricks & studs — Clean single-responsibility brick (disk_reclaim/reclaimable.rs) with a small public stud (reclaimable_targets, build_tree_roots, *_idle_days_from_env). It connects through the existing guard::vet_candidate stud rather than growing a parallel path. Snapshot/backup reclamation is explicitly delegated to MaintenanceThread — no boundary bleed, no double-ownership, documented in the module header + docs/reference/disk-reclaim-deterministic-enumeration.md.
• Zero-BS — No stubs, todo!, unimplemented!, dead code, or faked APIs in non-test source. Every proposed candidate is real and actionable. unwrap() occurrences are confined to #[cfg(test)] fixtures. Regeneration invariant holds: only cargo build-regenerable trees + idle worktrees are proposed.
• No over-engineering — Deterministic floor is additive to the agentic proposal; it did not replace or re-plumb the executor. Env clamping is a genuine safety property (0/empty/negative → safe floor, never "purge now"), not gold-plating.
• Clean module boundaries — Deterministic candidates get no self-trust: they are re-vetted at the syscall boundary identically to LLM candidates (proven by reclaimable/guard tests, 17 passed; emergency_cleanup 2 passed). Fail-closed clamping; root apply→dry-run downgrade preserved.

Non-blocking notes (philosophy-adjacent, defer to follow-up)

1. [Low] "Surface failures" nick — emergency_cleanup_with_pct removes build trees via std::fs::remove_dir_all(&root).is_ok() (disk_health.rs), dropping the Err with no warn!. PHILOSOPHY §3 says errors should be visible during development. It's the PID-guarded ≥95% last-resort tier and a pre-existing pattern, so non-blocking — but a one-line warn! on the Err arm would fully satisfy the "no swallowed exceptions" rule. Recommend as a trivial follow-up, not a merge blocker.
2. [Info] Requirement-named artifacts absent — canary-repro-target/bootstrap-target (named in the issue) exist nowhere in src/; substituting the real cargo-target/shared-target caches is the correct present-moment-focus choice, not a gap.

Confirmation: Philosophy-compliant. No code changes required for merge. The single warn! suggestion is optional hardening for a separate PR.

[rysweet]

Step 18b — Review Feedback Implemented

Applied the one unanimous pre-merge suggestion (S2) from the Code, Security, and Philosophy reviews. Commit b9b3381cb.

Addressed

#	Finding	Action	Status
S2	emergency_cleanup_with_pct dropped the remove_dir_all Err via .is_ok(), silently swallowing failures	Replaced with a match that logs warn!(path, error, ...) on removal failure — satisfies the surface-failures / no-swallowed-errors rule	✅ Fixed

Verified

• cargo test --lib disk_health → 13 passed, 0 failed
• Pre-commit: cargo fmt --check, cargo clippy --release -D warnings → passed
• Pre-push: full suite 482 passed, 0 failed, cargo clippy --all-targets --all-features -D warnings → passed

Not changed (with reasoning)

• Q1 / Q2 (scope confirmations) — No code change needed. canary-repro-target/bootstrap-target named in Fix Simard's ineffective disk-reclaim so routine reclaim actually recovers space before the root filesystem fills. DIAGNOSED ROOT CAUSE: the routine reclaim in src/disk_reclaim/executor.rs categorical #4809 exist nowhere in src/; substituting the real cargo-target/shared-target caches is the correct real-world mapping. Snapshot reclamation is deliberately delegated to MaintenanceThread to avoid races. These are ownership confirmations, not defects.
• S1 (route emergency direct-remove through is_safe_to_delete for symlink/containment hardening) — pre-existing PID-guarded last-resort pattern with a hardcoded path; not introduced by this PR. Deferred to a focused follow-up to avoid scope creep.
• S3 (is_idle keys on root mtime) — already mitigated by the live-PID probe. Deferred.
• S4 (PR title nit) — cosmetic; deferred.

No blocking issues remained; the trivial high-consensus fix is applied and merge-ready.

[rysweet]

Step 20 — Final Cleanup: F1 Consistency Fix

Applied the one non-blocking follow-up (F1) surfaced by the philosophy review. Commit db8cddab1.

F1 — Uniform error surfacing in emergency_cleanup_with_pct (src/disk_health.rs). The previous commit corrected only the newly-added build-tree removal. The four sibling remove_dir_all calls (target/debug, target/llvm-cov-target, worktrees/*/target, state-root cargo dirs) and the backup remove_file prune still dropped their Err via .is_ok(). All five now use match … warn!(path, error, …) so no removal failure is silently swallowed. Behavior is unchanged (best-effort cleanup continues on failure); failures are now visible via structured logs.

Verification:

• No .is_ok() error-drops remain in disk_health.rs.
• disk_health lib tests: 13 passed, 0 failed.
• Pre-commit (fmt + clippy release) green.
• Pre-push: full suite 482 passed, 0 failed + clippy all-targets clean.

Final cleanup audit: no temp/debug artifacts, no forbidden patterns in non-test source, working tree clean, module boundaries intact, no dead code/stubs. PR remains merge-ready.

[github-actions]

📊 Coverage Summary

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

Module	Lines	Covered	Coverage
Total	205242	172845	84.2%

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

[rysweet]

Auto-closing this superseded auto-generated documentation PR: it is an older duplicate superseded by the canonical open auto-doc PR #4826. Enforcing the single-open auto-doc PR invariant (goal_hygiene).