Skip to content

Update Cargo.toml with 40 changed files (#4809) - #4833

Draft
rysweet wants to merge 6 commits into
mainfrom
feat/issue-4809-nodeoptions-max-old-space-size32768-saved-preferen
Draft

Update Cargo.toml with 40 changed files (#4809)#4833
rysweet wants to merge 6 commits into
mainfrom
feat/issue-4809-nodeoptions-max-old-space-size32768-saved-preferen

Conversation

@rysweet

@rysweet rysweet commented Jul 27, 2026

Copy link
Copy Markdown
Owner

Summary

Concise workflow-generated PR for Cargo.toml.

Issue

Closes #4809

Changed files

  • Cargo.lock
  • Cargo.toml
  • docs/concepts/agentic-disk-reclamation.md
  • docs/howto/configure-disk-reclamation.md
  • docs/index.md
  • docs/reference/cognitive-thread-observability.md
  • docs/reference/cognitive-threads-catalog.md
  • docs/reference/disk-reclaim-api.md
  • docs/reference/ooda-decide-prompt.md
  • docs/reference/ooda-orient-recipe.md
  • docs/reference/ooda-record-orient-decide-cli.md
  • docs/reference/recipe-brain-verdict-parsing.md
  • docs/reference/telemetry-metrics.md
  • docs/reference/text-parsing-wire-formats.md
  • mkdocs.yml
  • prompt_assets/simard/recipes/disk-reclaim.yaml
  • prompt_assets/simard/recipes/ooda-decide.yaml
  • prompt_assets/simard/recipes/ooda-orient.yaml
  • src/cognitive_threads/mind.rs
  • src/cognitive_threads/mod.rs
  • src/cognitive_threads/telemetry.rs
  • src/cognitive_threads/tests.rs
  • src/cognitive_threads/tests_thread_telemetry.rs
  • src/cognitive_threads/thread.rs
  • src/cognitive_threads/threads/analogy.rs
  • src/cognitive_threads/threads/consolidation.rs
  • src/cognitive_threads/threads/creative_ideas.rs
  • src/cognitive_threads/threads/engineer_log_analysis.rs
  • src/cognitive_threads/threads/interoception.rs
  • src/cognitive_threads/threads/maintenance.rs
  • src/cognitive_threads/threads/metacognition.rs
  • src/cognitive_threads/threads/narrative.rs
  • src/cognitive_threads/threads/ooda.rs
  • src/cognitive_threads/threads/operator_model.rs
  • src/cognitive_threads/threads/prospection.rs
  • src/cognitive_threads/threads/reflection.rs
  • src/cognitive_threads/threads/salience.rs
  • src/cognitive_threads/threads/values_deliberation.rs
  • src/disk_reclaim/executor.rs
  • src/disk_reclaim/guard.rs

Diff stat

 src/operator_cli/tests_record_orient_decide.rs     |  807 ---------
 src/operator_commands_ooda/daemon/mod.rs           |   18 +-
 src/overseer/activity.rs                           |    2 -
 src/overseer/diagnosis.rs                          |    5 -
 src/overseer/mod.rs                                |   61 -
 src/overseer/sensor.rs                             |    6 -
 src/overseer/tests_thread_oversight.rs             |  320 ----
 src/overseer/thread_oversight.rs                   |  229 ---
 src/overseer/wiring.rs                             |    5 -
 src/telemetry/names.rs                             |   40 -
 tests/gadugi/decide-orient-brain-parse.sh          |  203 ++-
 tests/gadugi/decide-orient-brain-parse.yaml        |    6 +-
 tests/qa-scenarios/reasoner-comment-recovery.yaml  |    2 +-
 .../reasoner-control-char-recovery.yaml            |    2 +-
 .../reasoner-invalid-escape-recovery.yaml          |    2 +-
 .../reasoner-number-special-recovery.yaml          |    2 +-
 .../reasoner-python-literal-recovery.yaml          |    2 +-
 .../reasoner-trailing-comma-recovery.yaml          |    2 +-
 tests/recipe_brain_verdict_assets.rs               |   24 +-
 70 files changed, 2249 insertions(+), 5285 deletions(-)

Behavior

Implemented behavior through these branch commits:
c32790e 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 inspecting the branch and changes to understand what this PR touches.

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

This is a Rust change to disk-reclaim (issue #4809). Let me examine the actual code changes.

Risk

No high-risk subsystem pattern detected from changed paths.

Checklist

  • Branch has 1 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 the PR branch as a consumer of the disk_reclaim behavior, using the qa-team skill's Rust-CLI path (cargo test, native test fixtures) rather than the gadugi framework.

Detected toolchains

  • Primary: Rust / Cargo (Cargo.toml, crate simard). Changed code is src/disk_reclaim/{executor,guard,mod}.rs.
  • Secondary: npm (package.json, CLI wrapper bin.js) — no JS changed in this PR.
  • Changed non-code: docs (docs/**) and recipe (prompt_assets/simard/recipes/disk-reclaim.yaml).

Chosen strategy
Rust CLI repo → cargo test against the affected module. Note: root fs / was 100% full (28G, 0 avail), but the worktree lives on /dev/sda (/home/azureuser, 209G free), so compilation succeeded once TMPDIR was redirected off the full root fs.

Scenarios

# Type Scenario Command Result Key output
1 Simple Regression: routine reclaim frees target/debug once target/ is an allow-root (issue #4809 no-op) cargo test --lib disk_reclaim::executor::tests::routine_reclaim_frees_target_only_when_target_root_is_in_scope ✅ pass test result: ok. 1 passed; 0 failed
2 Edge/Integration Full disk_reclaim module: allow-root scope (mod.rs), guard safety boundary (candidate == allow-root refused, child reclaimable), executor skip-tracing cargo test --lib disk_reclaim:: ✅ pass test result: ok. 75 passed; 0 failed

Fix count: 0. No product code changes were required — both the simple and edge/integration scenarios passed.

Note on a transient false failure: An initial run of scenario 2 showed 4 failures (worktree_branch_none_on_missing_repo, deriving_remover_rejects_unresolvable_worktree_removal, exec_refuses_path_outside_allow_roots, exec_allows_orphan_inside_allow_root_but_dry_run_deletes_nothing). Root cause was environmental: TMPDIR had been set inside the git worktree, so these tests' git-worktree/allow-root detection saw the enclosing repo. Re-running with TMPDIR outside any git repo produced 75 passed; 0 failed. This is a test-harness placement artifact, not a defect in the PR.

Automatic checkpoint to preserve work in progress.
Tests and implementation saved before refactoring phase.
@rysweet rysweet added the simard-autonomous Simard-authored PR eligible for gated autonomous self-merge label Jul 27, 2026

@rysweet rysweet left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comprehensive Code Review — Step 17b

Scope: src/disk_reclaim/{mod,guard,executor}.rs + docs + disk-reclaim.yaml (widen routine reclaim to per-repo target/, add per-candidate skip tracing). Verified against PR head c32790e48; all 75 disk_reclaim tests pass locally, including the two new regressions.

Verdict: Approve — one substantive non-blocking finding (doc/guarantee accuracy)

The core design is sound: rooting the allow-root at the target/ parent (not target/debug) is the minimal change that respects is_safe_to_delete's strict-inside containment, and guard.rs::rail_refuses_candidate_equal_to_allow_root_but_allows_its_child locks that boundary. Deny-set precedence, symlink refusal, and fields-only (log-forging-safe) tracing are all correct. The before/after regression test (routine_reclaim_frees_target_only_when_target_root_is_in_scope) is exactly the right proof.


🟡 Finding 1 (Medium, non-blocking): the LiveProcess rail does not reliably protect an in-flight cargo build of a repo's own target/ — but the docs/comments claim it does

The live-process rail is the only thing standing between routine reclaim and an active build now that bare target/debug is a direct candidate. But ProcfsLiveProcessProbe::worktree_has_live_process (src/worktree_gc/liveness.rs:100-131) matches a process only when /proc/<pid>/cwd is at/under the candidate path:

if target.starts_with(&canon) { return true; }   // canon = the candidate, e.g. <repo>/target/debug

A cargo build (and the rustc children it spawns) runs with cwd = the repo root (<repo>), never under <repo>/target/debug. So <repo> does not starts_with <repo>/target/debug, the probe returns false, and the candidate is not refused. There is no compensating check — I grepped for a .cargo-lock/flock, /proc/<pid>/exe, or open-FD check in disk_reclaim/ and found none.

Before this PR <repo>/target was out of scope, so bare target/debug was never a direct candidate for the primary checkout; the widened allow-root now makes it one, so routine reclaim can delete target/debug mid-build.

The stated guarantee is inaccurate for this common case:

  • docs/reference/disk-reclaim-api.md:233-234 — "the live-PID rail (an in-flight cargo build holding target/debug is refused with LiveProcess)"
  • docs/concepts/agentic-disk-reclamation.md:151 — "an in-flight cargo build holding target/debug is … gated by every hard rail below"

Why non-blocking: impact is bounded — the recipe itself argues "a running cargo build simply rebuilds a deleted artifact directory, so this is only a cost, never a correctness risk," and emergency_cleanup already removes these same dirs. A killed build is recoverable.

Recommend one of:

  1. Correct the docs/comments to state the rail keys on process cwd and therefore does not guarantee mid-build protection for a build launched from the repo root — reclaim relies on cargo's own rebuild resilience, not the rail. (Cheapest; removes the false guarantee.)
  2. Strengthen the rail to honor Cargo's advisory target/.cargo-lock (try_lock fails ⇒ build active ⇒ skip). This reliably detects an in-flight build regardless of cwd and would make the existing guarantee true.

Option 1 is sufficient to merge; option 2 is the more robust follow-up.


Checklist

  • Code quality / standards — idiomatic; tracing fields-only, no print!/println!, no "Bridge" naming (grep-confirmed).
  • Test coverage — strong: before/after routine regression + equal-vs-child boundary + allow_roots and bare-$HOME unit assertions. Gap: no test exercises the live-build-vs-cwd scenario in Finding 1 (would be the natural test if option 2 is taken).
  • No TODOs/stubs/swallowed exceptions — none added.
  • No unimplemented functions — n/a.
  • Logic correctness — allow-root widening, strict-inside containment, deny-set precedence all correct.
  • Edge cases — bare-target/ equality refused; <repo>/src & .git still OutsideAllowRoot; symlink refused; worktrees/main deny-set overrides allow-root. Uncovered edge = Finding 1.

Additive, non-breaking; EMERGENCY path untouched. Approve pending a doc-accuracy fix for Finding 1 (or acceptance of the bounded risk with docs corrected).

@rysweet rysweet left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Security Review — Step 17c (MANDATORY)

Scope: PR #4833 (issue #4809) — widen routine disk-reclaim allow-roots to per-repo <repo>/target, add per-candidate structured skip tracing. Reviewed against PR head c32790e48; src/disk_reclaim/{mod,guard,executor}.rs + docs + disk-reclaim.yaml. 75/75 disk_reclaim tests pass.

Checklist

  • Security requirements met — fail-closed destructive-path primitive (is_safe_to_delete) left untouched; widening is additive and still gated by every rail.
  • No new vulnerabilities — new allow-root cannot escalate deletion beyond rebuildable Cargo children (see analysis).
  • Sensitive-data handling — no secrets/tokens/PII read, logged, or written; tracing emits only path/reason/kind.
  • AuthZ / capability boundary — deletion capability defined by allow-root ∧ ¬deny-set containment; precedence preserved (deny checked first).
  • Injection vectors — no shell/SQL/path-injection surface introduced; log-forging specifically mitigated.

Analysis of the security boundary (unchanged, re-verified)

is_safe_to_delete (maintenance.rs:226) remains the single audited fail-closed gate. Every property the fix depends on holds:

  1. Symlink refusal (SR-5)symlink_metadata on the final component; is_symlink() → refuse. A swapped target/debug symlink cannot redirect a delete outside the root.
  2. Traversal defeatcanonicalize resolves ../links before the containment test, so <repo>/target/../../etc cannot escape.
  3. Strictly-inside containmentreal != r && real.starts_with(&r) (maintenance.rs:252). This is the crux of the widening: rooting at <repo>/target (the parent) makes target/debug deletable but the bare target/ root itself is refused (OutsideAllowRoot), so a hostile/hallucinated candidate equal to the allow-root can never trigger rm -rf target/. New regression rail_refuses_candidate_equal_to_allow_root_but_allows_its_child locks this.
  4. Deny-set precedencevet_candidate checks protected.contains() first (guard.rs:234), so protected paths win even inside the new allow-root. ProtectedDenySet::resolve unions daemon cwds + worktrees/main + operator env; confirmed it does not include managed-repo roots, so the new <repo>/target root is not shadowed and, conversely, no protected location is exposed by it.
  5. Symlink/.git worktree railpath.join(".git").symlink_metadata() still gates each dir; unaffected.

Log-forging (injection) — mitigated

The new tracing::info! in executor.rs is fields-only (path = %…, reason = ?…, kind = ?…) with a static message. The attacker-influenceable reason/path are structured field values, never interpolated into the format string, so a candidate path containing newlines/ANSI/level=ERROR cannot forge or inject audit-log lines. No print!/println! added.

Non-blocking observation (defense-in-depth, carried from Step 17b)

The LiveProcess rail keys only on /proc/<pid>/cwd (liveness.rs:126). cargo build runs with cwd at the repo root, not under target/debug, so the liveness rail does not by itself prevent deleting target/debug during an in-flight build. This is a safety/availability concern (rebuildable artifacts, bounded impact) rather than a confidentiality/integrity vulnerability — data loss is limited to regenerable Cargo output. Recommend either honoring Cargo's target/.cargo-lock or correcting the docs that claim such a candidate is "refused with LiveProcess". Non-blocking.

Verdict

No blocking security findings. The destructive-path gate is unchanged and fail-closed; the allow-root widening is provably confined to rebuildable children by the strictly-inside rule; tracing is injection-safe. Approve from a security standpoint, with the one non-blocking availability hardening note above.

@rysweet

rysweet commented Jul 27, 2026

Copy link
Copy Markdown
Owner Author

Philosophy Guardian Review — Step 17d

Verdict: ✅ COMPLIANT — approved, one non-blocking Zero-BS observation on documentation accuracy.

Assessed the committed diff at c32790e48 (+333 / −23, 7 files: 3 Rust, 3 docs, 1 recipe yaml) against PHILOSOPHY.md.

Compliance checklist

  • Ruthless simplicity achieved — The functional fix is two lines in mod.rs (roots.push(repo.join("target"))) plus one tracing::info! skip event in executor.rs. This is the smallest change that resolves the regression: root at the target/ parent so the existing strict-inside containment (is_safe_to_delete) confines removal to target/ descendants without touching the guard's semantics. No new abstractions, no new candidate category, no config surface added. Occam's-razor correct.
  • Bricks & studs pattern followed — Change is fully isolated inside the src/disk_reclaim/ brick. The public stud (allow_roots(&Path) -> Vec<PathBuf>) keeps its signature; only its returned set widens. No cross-module coupling introduced; the guard/executor/mod split is preserved. Tests, fixtures, and docs for the module travel with it (regeneration-ready).
  • Zero-BS implementation — No stubs, no TODOs, no dead code, no faked APIs. The skip-reason path emits real tracing (fields-only, anti-log-forging — the agent's free-text reason is never interpolated) rather than swallowing the rejection silently. Rejections remain visible via skipped[] and the new per-candidate event. Errors are surfaced, not hidden.
  • No over-engineering — The verbose doc-comments and the two new regression tests are justified, not gold-plating: they document and pin a load-bearing security boundary (why the root is target/'s parent not target/debug; why a candidate equal to an allow-root stays refused while its child is reclaimable). rail_refuses_candidate_equal_to_allow_root_but_allows_its_child guards against a regression that could rm -rf an entire target/. This is appropriate rigor for a delete path, not speculative future-proofing.
  • Clean module boundaries — Additive and non-breaking. ProtectedDenySet precedence, live-PID rail, symlink refusal, and the TOCTOU re-assert are all untouched and still gate every deletion. The shared, audited is_safe_to_delete primitive is deliberately left unchanged.

🟡 Non-blocking observation (Zero-BS / documentation accuracy)

The docs assert a safety guarantee — "an in-flight cargo build holding target/debug is refused with Live process" (agentic-disk-reclamation.md, disk-reclaim-api.md) — but the LiveProcess rail keys only on /proc/<pid>/cwd, and cargo build runs with cwd at the repo root, not under target/debug. Per the Zero-BS principle (no faked or inaccurate claims about behavior), a documented safety rail should match actual behavior. This overlaps the Step 17b code-review finding and is non-blocking (impact is bounded to rebuildable artifacts). Recommend either correcting the doc wording or honoring Cargo's target/.cargo-lock in a follow-up.

Conclusion

The implementation embodies ruthless simplicity and Zen-like minimalism: a two-line functional fix, thoroughly tested at the exact safety boundary, confined to one brick, with observability that makes rejections auditable rather than silent. No philosophy-blocking changes required.

rysweet and others added 2 commits July 27, 2026 03:48
The live-PID rail keys on /proc/<pid>/cwd, so an in-flight cargo build
(cwd at repo root) is NOT caught by LiveProcess for a target/ candidate.
Corrected the docs to describe the actual cwd-inside-candidate guarantee
and clarify that build artifacts are safe as the rebuildable
StaleBuildCache class. Addresses non-blocking doc-accuracy finding from
code/security/philosophy reviews (Step 17e).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Implemented reviewer suggestions; fixed identified issues; updated per security review; addressed philosophy compliance items
@github-actions

Copy link
Copy Markdown

📊 Coverage Summary

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

Module Lines Covered Coverage
Total 204900 172495 84.2%

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

rysweet and others added 3 commits July 27, 2026 04:12
Point cross-references to the correct '#configuration-accessors' anchor
(heading is 'Configuration accessors') in overseer-backoff-gate-api.md,
resolving the two broken intra-doc links flagged under mkdocs --strict.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…suite (#4809)

The reclaim-effectiveness reference/concept docs declared
ReclaimEffectivenessGate / EffectivenessDecision / src/disk_reclaim/
effectiveness.rs as implemented, but the module and types did not exist —
docs ahead of code, and the actual churn fix for #4809/#4825/#4810 was
missing.

Add src/disk_reclaim/effectiveness.rs: the suppress-only per-key
exponential-backoff cooldown that stops the OODA daemon re-firing a
proven-ineffective reclaim run every ~15-min cycle. Mirrors the already
-tested overseer BackoffGate peek/record split, with a hard %-used
ceiling bypass so a genuinely filling disk always reclaims.

Encodes the documented contract as 21 unit tests (TDD, Step 7):
ceiling bypass, unseen/elapsed/backwards-clock admission, in-cooldown
suppression, exponential growth + cap, long-silence reset, effective-run
immediate re-admit, independent per-key backoff, saturating no-overflow,
and injectable config accessors (defaults + clamps). Additive telemetry
name simard.disk.reclaim.suppressed_cycles.

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

Step 8 implementation. The ReclaimEffectivenessGate module + its 21 TDD
tests landed in step 7, but the gate was never called: the Tier-3
disk-reclaim block in run_ooda_daemon re-fired run_disk_reclaim every
~15-min cycle whenever %-used crossed SIMARD_DISK_RECLAIM_PCT, regardless
of whether prior runs freed a byte — the churn on the ~94%-full host
(#4809 / #4825 / #4810).

Wire the suppress-only backoff in front of the (unchanged) propose/dispose
path:
- construct the gate once as daemon loop state (from_env + kill switch),
- peek(key, used_pct, now) before each would-be run; on Suppress, emit the
  additive simard.disk.reclaim.suppressed_cycles counter, trace at info,
  and skip — a disk at/above the hard ceiling always bypasses,
- record(key, was_effective, now) after a real run so a no-op arms/grows
  the per-partition cooldown and an effective run re-admits next cycle; a
  failed run is left unrecorded so a transient error never arms a cooldown.

Add ReclaimReport::was_effective() (bytes_freed>0 || used_pct dropped) as
the documented gate signal, with a unit test; a dry-run frees nothing and
is therefore not effective, so the default dry-run daemon backs off the
proven no-op churn. Align the API doc's suppress log level (info, routine)
and dedup-key prefix with the implementation.

Additive / non-breaking: every safety rail and the dry-run default are
unchanged; the gate is on by default and fully disabled by
SIMARD_DISK_RECLAIM_EFFECTIVENESS_GATE=off.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

simard-autonomous Simard-authored PR eligible for gated autonomous self-merge

Projects

None yet

1 participant