Conversation
Automatic checkpoint to preserve work in progress. Tests and implementation saved before refactoring phase.
…docs Step 9 refactor/simplify of the #4470/#4469 fix. Additive, non-breaking. - Delegate the quarantine-name predicate in self_deploy/health.rs and self_deploy/quarantine_ack.rs to the canonical crate::cmd_cleanup::is_corrupt_quarantine_name, removing two identical copies that the comments already warned had to be kept in sync (drift hazard eliminated; single source of truth). - Fix stale doc comments in quarantine_ack.rs: drop the obsolete "implementation is TODO" note (now implemented) and correct the ACK_MARKER_BYTES comment that mislabeled the payload as an upper bound. No behavior change. Build clean; clippy clean; self_deploy, cmd_cleanup, quarantine_ack, self_relaunch lib tests and self_deploy_convergence integration test all pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…tion The #4469 change made `remove_old_corrupt_dbs` resolve its scan directory through `simard_state_root()` (honoring `SIMARD_STATE_ROOT`) instead of the hardcoded `$HOME/.simard`. That is correct for production — the sweep and the self-health `no_quarantine` probe must scan the same resolved root — but it coupled the cmd_cleanup sweep tests to the process-global `SIMARD_STATE_ROOT` env, which many lib tests mutate under unrelated serial keys. Under the full parallel suite a concurrent setter redirected the scan dir, so the sweep found nothing and four tests failed (aged/keep-last-N quarantines "not swept"). Decouple the sweep logic from state-root resolution: add path-injected `remove_old_corrupt_dbs_in(scan_dir, report)` and have the public `remove_old_corrupt_dbs` delegate to it with the resolved root. Drive all sweep tests through the injected variant against a tempdir, removing every `HOME`/`SIMARD_STATE_ROOT` mutation from them. The tests are now deterministic and free of cross-test env races. The `corrupt_db_sweep_scans_resolved_state_root` wiring test still exercises the public wrapper's env resolution. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Resolve merge conflicts for the self-deploy quarantine-acknowledge work (#4488): - docs/reference/self-deploy-api.md: combine main's full NoQuarantineProbe reference section (window-scoped fresh/retained counts) with the PR's acknowledgement-aware counting note (#4469). - src/cmd_cleanup/disk.rs & src/self_deploy/health.rs: keep the semantic union of main's corrupt-DB sweep (top-level + live-store) and the PR's .ack sidecar awareness / protected-asset guard. Also unstage and gitignore the stray gym_history.db runtime artifact that was accidentally staged during conflict resolution. Validated: quarantine_ack (10), corrupt_db (14), self_health (6), auto_ack (4), aged_protected (5), quarantine_scan (9) — all passing on the merged tree. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…apper Step 9 refactor/simplify follow-up for the #4470/#4469 fix. Additive, non-breaking, no behavior change. - Remove the single-use private is_corrupt_quarantine_name wrapper in quarantine_ack.rs and call the canonical crate::cmd_cleanup::is_corrupt_quarantine_name directly, matching the pattern already used in self_deploy/health.rs. Removes redundant indirection; the delegation rationale is preserved as an inline comment. Build clean; clippy --all-targets --all-features clean; fmt clean; quarantine_ack lib tests (10) and self_deploy_convergence integration tests (4) all pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…anners Step 9b performance pass for the #4470/#4469 fix. Both production directory scanners (scan_quarantine_candidates, tally_quarantine_files) forced a heap String via .to_string_lossy().to_string() for every directory entry, even though most entries are immediately skipped by the corrupt-quarantine / ack-marker predicates. Borrow the lossy Cow<str> instead so no String is allocated for the common skipped-entry case. Non-breaking, no behavior change (predicates take &str; Cow derefs). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Two review-pass fixes: 1. Security (#4469): log auto-ack and manual-ack quarantine basenames via `?name` (Debug) instead of `%name` (Display) in self_deploy::health and operator_cli::self_health. A quarantine basename is an untrusted on-disk filename that may contain newlines/control chars; under the default non-JSON tracing subscriber, logging it raw would permit log-line forgery. 2. CI portability: the merge commit accidentally captured machine-specific absolute paths (/home/azureuser/...) in .github/hooks/amplihack-hooks.json, which break the hooks on CI and other checkouts. Restore the portable repo-relative paths from main. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Address PR #4488 review findings: BLOCKING — probe/sweep directory-set asymmetry. The cleanup sweep reclaims both the top-level state root and the live-store `state/` subdir, but the self-health `no_quarantine` probe / auto-ack only scanned `state/`, so the probe and sweep could disagree on where quarantines live. Single-source the scan-dir set in `state_root::quarantine_scan_dirs` (deduped) and drive BOTH the sweep and the probe/auto-ack from it, so they can never diverge. Minor — orphaned `.ack` sidecars never reclaimed. Add `reclaim_orphaned_ack_sidecars`: a marker whose parent quarantine no longer exists is now reclaimed (regardless of remaining candidates), so stale markers cannot accumulate unbounded. LOW-1 — untrusted quarantine basename written unescaped to stderr / CleanupReport. Route operator-facing paths in the corrupt-DB sweep through the shared `sanitize_gate_detail` control-char strip (re-exported from `self_relaunch`) to prevent terminal/log forgery. LOW-2 — auto-ack `marker` field logged via Display while the sibling `artifact` is Debug-escaped. Log `marker` via `?marker` (Debug) for consistent control-char escaping. Tests: rework the top-level-scan test into a probe/sweep parity contract, add an orphan-`.ack`-reclaim test. All changed-module lib tests (78) + self_deploy_convergence (4) pass; clippy clean. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
rysweet
left a comment
There was a problem hiding this comment.
Step 17b — Comprehensive Code Review (PR #4498)
Verdict: ✅ Approve. High-quality, security-conscious implementation of the #4469 stuck-quarantine deadlock fix (durable .ack sidecars), the #4470 gate-failure diagnosability improvement, and the #4433 OodaConfig::default() serial-guard blind-spot fix. Reviewed the committed diff at HEAD 228d4d24 (origin/main...HEAD, +2751/−189, 27 files).
Scope reviewed
src/self_deploy/quarantine_ack.rs (new), src/self_deploy/health.rs, src/cmd_cleanup/disk.rs, src/self_relaunch/gates.rs, src/operator_cli/self_health.rs, src/state_root.rs, src/self_deploy/mod.rs, src/ooda_loop/decide.rs, src/test_support/serial_guard.rs, plus tests and docs.
Checklist
- ✅ Code quality & standards — excellent module/function docs, single-sourced invariants (
quarantine_scan_dirs,select_protected_asset,is_corrupt_quarantine_name) so the probe, sweep, and auto-ack can never disagree on where quarantines live or which asset is protected. - ✅ Test coverage — extensive: idempotency, TOCTOU/
O_EXCL, planted-symlink refusal, UTF-8-boundary truncation, path-separator/../absolute rejection, aged-vs-fresh eligibility, per-directory bounds, orphaned-sidecar reclaim.quarantine_ack(10),self_relaunch::gates(13),cmd_cleanup(40) all green locally;cargo check --lib --testscompiles. - ✅ No TODOs / stubs / swallowed exceptions — every
.unwrap()is confined to#[test]code. Best-effort auto-ack failures are surfaced via structuredtracing::warn!(documented), not silently dropped. - ✅ No unimplemented functions.
- ✅ Logic correctness — verified fail-safe branches: future-dated mtime →
duration_since().unwrap_or_default()= 0 → not aged → not eligible; unreadable mtime entries are skipped (never counted fresh); tie-breaksize then modified= newest wins; sidecar exclusion precedesis_corrupt_quarantine_name(which matches.ackvia the.corrupt-infix). - ✅ Edge cases — symlink/dir at sidecar path refused (no write-through),
AlreadyExistsrace re-verified as regular file, orphaned.ackreclaimed even when a directory has zero live quarantines.
Security (strong)
acknowledge()opens withcreate_new(true)(O_EXCL, no symlink follow) and pre-checks viasymlink_metadata, closing the TOCTOU window and defending against planted-symlink sidecars — with a dedicated#[cfg(unix)]test asserting the victim file is untouched.- Untrusted quarantine basenames (attacker-writable on-disk filenames that may embed CR/LF/ANSI) are routed through
sanitize_gate_detail/ Debug-escaping (?name) before reaching operator-facing stderr,CleanupReport, ortracing— closing the log-forgery vector (LOW-1/LOW-2).
Observations (non-blocking)
eprintln!in the cleanup sweep (cmd_cleanup/disk.rs) is pre-existing operator-CLI convention; this PR only hardens it by sanitizing the path argument. No newprint!/println!/eprintln!is introduced in production code. Fine as-is; optionally migrate totracingin a future pass for consistency.- Auto-ack on the probe path runs a
read_dir(and at most one idempotent sidecar write) per scan dir on everyrun_self_health_probe. Cost is negligible and the operation is idempotent + age/protection-gated; no change needed.
Reviewer note (environment)
While reviewing, a concurrent process is actively modifying this working tree (uncommitted M/A, incl. a new src/util/log_sanitize.rs refactor that relocates sanitize_gate_detail). Those changes are not part of the committed PR and briefly produced a transient compile mismatch when a test build caught the edit mid-flight. The committed HEAD (228d4d24) is internally consistent (self_relaunch::mod re-exports sanitize_gate_detail; disk.rs consumes it; no util::log_sanitize reference) and compiles. If that refactor is intended for this PR, ensure it lands atomically and re-run CI.
Recommendation: merge once CI is green. No blocking issues found.
🤖 Automated comprehensive review (Step 17b).
Address optional philosophy/code-review notes from the Step 16 reviews. All items are non-blocking; the PR was already APPROVE with 0 blocking issues. S6: relocate the shared control-char log sanitizer out of `self_relaunch::gates` into a neutral `util::log_sanitize` module (`sanitize_to_single_line`). This removes the `cmd_cleanup::disk` -> `self_relaunch` cross-module coupling for a generic sanitizer; both the canary gate detail (#4470) and the cleanup path log (#4469, LOW-1) now depend on util instead. Sanitizer unit tests moved with it. S5: the test-only `count_quarantine_files` helper now delegates to the production `tally_quarantine_files` scan (summing fresh + retained) instead of duplicating the read_dir / acknowledgement filter logic, eliminating drift risk between the test helper and the live probe path. S7: document the intentional `let _ =` discard of `auto_ack_stuck_recovery_asset` in the no_quarantine probe loop (best-effort; errors logged internally). Also refresh docs/reference/self-deploy-quarantine-acknowledge.md to describe the actual production `tally_quarantine_files` scan rather than the now test-only helper. S4 (narrow `ack_marker_path` visibility) intentionally deferred: it is a documented public API and is consumed by the `self_deploy_convergence` integration test (a separate crate that requires `pub`), so narrowing to `pub(crate)` would break both. Verified: cargo test log_sanitize (3), self_relaunch::gates (10), self_deploy::health (20), quarantine_ack (10), cmd_cleanup (40), self_deploy_convergence e2e (4) — all pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Step 17c — Security Review (PR #4498)Verdict: ✅ No security findings that block merge. Reviewed the actual committed diff ( Checklist
Verified defenses (with evidence)1. Path traversal — 2. Symlink / TOCTOU — 3. Destructive-sweep safety — 4. Log injection / forgery. 5. DoS / resource bounds. Static scan (added lines only)
Test evidence
Notes (non-blocking)
Conclusion: No exploitable vulnerabilities identified in the committed diff. Approve from a security standpoint. |
Step 17d — Philosophy Guardian Review (PR #4498)Verdict: ✅ Compliant. Reviewed the committed diff (HEAD, +2808/−195 across 27 files) against the amplihack development philosophy. Compiles clean ( Compliance checklist
Reviewer notes (non-blocking)
Philosophy compliance: PASS. No changes required. |
…hooks.json The Step 18b commit (d1163b4) inadvertently staged an unrelated change that rewrote the six committed hook `bash` entries from repo-relative paths (e.g. `.github/hooks/stop`) to an absolute, machine-specific path rooted at `/home/azureuser/src/Simard-deploy-4049/...`. Those absolute paths are a config divergence that would break the hooks on every other checkout and in CI. Restore the portable repo-relative paths. No source or behavior change. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…t, and ack_marker_path visibility rationale Documentation-only comment additions addressing Step 19 review notes: - auto_ack_stuck_recovery_asset: document deliberate asymmetry vs manual acknowledge path (narrows to aged #2550 protected asset; never acks fresh corruption) - health probe: document the intentional durable .ack write side-effect - ack_marker_path: document why kept pub (self_deploy_convergence integration test in a separate crate asserts against it; narrowing breaks the build) No logic changes; affected suites green (quarantine_ack 10, health 20). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…d ack_marker_path visibility (#4488 Step 18b) Apply non-blocking Step 16 review feedback (all reviews PASS; zero blocking issues). Doc/comment-only; no behavior change. - S3: document the no_quarantine probe's intentional .ack write side-effect on run_self_health_probe, and the deliberate manual-vs-auto acknowledge asymmetry on auto_ack_stuck_recovery_asset. - S1 (disagreement): keep ack_marker_path pub instead of narrowing to pub(crate) as suggested -- the self_deploy_convergence integration test (a separate crate) consumes it, so narrowing would break the build. Added a note at the definition to prevent the suggestion recurring. S2 (best-effort/error-visibility) was already covered by the existing comment at the auto_ack call site; no change needed. Verified: cargo build clean, cargo doc introduces no new warnings, 37 lib quarantine tests + 4 self_deploy_convergence integration tests pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
… + correct auto-ack docs Quality-audit findings on PR #4498: F1 (HIGH): the operator `self-health --acknowledge-quarantine` remediation scanned only the top-level state root, while the no_quarantine probe and the cleanup sweep both scan quarantine_scan_dirs() = [state_root, state_root/state]. A stuck quarantine under <state_root>/state/ (the primary location the de-forked backend drops corrupt snapshots) could therefore never be cleared manually, leaving self-deploy frozen despite a 'success' from the command. Fix: add a path-injected state_root::quarantine_scan_dirs_under and iterate it in acknowledge_all_present_quarantines so all three consumers share the identical directory set. Adds acknowledge_all_covers_state_subdir regression test. F2 (MEDIUM): the automatic auto-ack's aged-gate (>= CORRUPT_DB_MAX_AGE_DAYS) is mutually exclusive with the probe's fresh-gate (mtime >= recent window_start), so the aged asset it targets is always counted 'retained', never 'fresh', and cannot change the no_quarantine verdict. Comments/docs claiming it 'breaks the stuck deadlock so the probe can converge' contradicted the probe's own fresh-window semantics (proven by no_quarantine_passes_with_only_historical_quarantines_in_state_dir). Corrected the code comments and reference/howto docs to describe the auto-ack accurately as defense-in-depth (the fresh-window logic is the primary convergence mechanism; the manual command clears genuinely-stuck fresh quarantines). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Ready for Final ReviewWorkflow steps completed: requirements, design, implementation, tests, code review, philosophy compliance, cleanup, and quality audit. Ready for merge approval. |
📊 Coverage Summary
Coverage data from CI run. Test files matching |
Summary
Concise workflow-generated PR for .gitignore.
Issue
Closes #4477
Changed files
Diff stat
Behavior
Implemented behavior through these branch commits:
228d4d2 fix: resolve Step 17 blocking + low-severity review findings (#4469)
401b3fb fix: log-forgery hardening (#4469) + restore relative hook paths
c39b9e9 perf(self-deploy): avoid per-entry String allocation in quarantine scanners
e55f120 refactor(self-deploy): inline single-use quarantine-name predicate wrapper
0229a51 Merge origin/main into pr-4488
65c9b5c feat: ℹ NODE_OPTIONS=--max-old-space-size=32768 (saved preference). To chang
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 this branch and the changes made.
✗ Get branch name and change summary (shell)
│ cd /home/azureuser/src/Simard-deploy-4049/worktrees/feat/issue-4477-nodeoptions-max-old-space-si
│ ze32768-saved-preferen && git branch --show-current && echo "---DIFF STAT---" && git --no-pager
│ diff main...HEAD --stat 2>/dev/null | tail -40 && echo "---UNCOMMITTED---" && git --no-pager
│ status --short
└ Denied by preToolUse hook from
"/home/azureuser/src/Simard-deploy-4049/.github/hooks/amplihack-hooks.json" (hook errored)
✗ Get current branch name (shell)
Risk
No high-risk subsystem pattern detected from changed paths.
Checklist
This PR was created as a draft for review before merging.