Repository navigation
feat(worktree): reinforce safe cleanup guards before removal - #455
seonghobae wants to merge 6 commits into
Conversation
… sessions Reinforce git_worktree audit so ignored debris, incomplete merged/closed evidence, and Orca sleep sessions cannot silently become removal candidates, while keeping live re-audit and distinct plan/result verification (#454). Co-authored-by: Cursor <cursoragent@cursor.com>
|
Warning Review limit reachedNext included review available in 59 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (9)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Match vitest and node contract tests to release.yml's pull_request-gated cancel-in-progress expression so CI no longer fails on stale expectations. Co-authored-by: Cursor <cursoragent@cursor.com>
seonghobae
left a comment
seonghobae
left a comment
There was a problem hiding this comment.
P1 deletion-authority boundary — closed_merged_head_oids / closed_merged_branches are currently caller-provided strings, but a match sets merged_closed_evidence = caller-admitted-closed-merged and then forces classification_contained = Some(true). The focused test explicitly proves an actually-unmerged worktree becomes RemovalCandidate when its OID is supplied. That is not durable merged/closed evidence; it is an assertion by the same caller asking for eligibility.
The shipped remove CLI currently rebuilds GitWorktreeAuditOptions::default(), so its live re-audit will not reproduce those flags and should fail closed on plan drift. That mitigates this one binary path, but it does not make the library contract safe: execute_stale_worktree_removal accepts GitWorktreeAuditOptions and re-audits with the supplied options, so another current/future consumer can propagate the same untrusted assertion through mutation.
Repair this on a canonical non-force descendant, not by weakening fingerprint/live re-audit. Raw OID/branch input must remain non-authoritative unless it is resolved from or bound to an immutable/authenticated forge/owner evidence record. Add a realistic negative regression: an unmerged branch plus exact matching CLI/caller OID/branch still must not become RemovalCandidate without that evidence. Preserve the valid ignored-artifact, Sleep, incomplete-dispatch, result-record, active-use and Restart Manager protections.
|
Canonical transfer has started on dependent Draft #457, based directly on #444 head Transferred in the first tranche:
#457 does not transfer |
Caller-supplied closed/merged OIDs and branch names are hints only until an authenticated evidence owner is bound; retention-reference containment remains the sole durable merged/closed evidence source for removal candidates. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Canonical carryover audit against current #444/#457 (fresh 2026-09-20 state):
This keeps #455 as evidence/source of valid remaining deltas while foundation-first repair continues; no wholesale transplant of its older |
Inventory and audit can observe the same Google Drive path in NFC vs NFD on macOS, which caused path_fingerprint mismatches and no_audit_match for clean-publication-snapshot worktrees. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Fresh successor carryover audit against #444 |
Prefer device/inode (or canonical path) when paths resolve on disk so distinct NFC/NFD directory entries on Linux cannot collide; keep NFC as the bounded fallback for macOS File Provider / Hangul spellings. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Fresh successor audit against #444
#455 therefore remains open. Do not merge it directly or transplant its older |
|
Successor audit update: path/object identity and admin-fallback evidence have stronger #460 successors, but #455 still must remain open. Current successor gap is now explicit as (1) ignored-artifact evidence bound into core entry integrity and per-candidate live re-audit, (2) Orca Sleep still-bound sessions preserved, (3) incomplete orchestration dispatch stable fail-closed, and (4) fingerprint provenance migrated to immutable audit v5 with signed successor + same-head GREEN. Current #460 |
|
Fresh successor source audit on #460 exact |
|
Scheduled review-feedback autofix for this PR head.
|
|
Admission correction — exact current head |
|
Scheduled review-feedback autofix for this PR head.
|
|
Scheduled review-feedback autofix for this PR head.
|
|
Scheduled review-feedback autofix for this PR head.
|
|
Scheduled review-feedback autofix for this PR head.
|
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d2dc6cdf7c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for key in ["path", "worktreePath"] { | ||
| if let Some(path) = object.get(key).and_then(|value| value.as_str()) { | ||
| if !path.is_empty() { | ||
| paths.insert(PathBuf::from(path)); | ||
| } |
There was a problem hiding this comment.
When Orca reports a sleeping worktree through an absolute symlinked or ..-containing path, this stores that spelling verbatim, but audit_git_worktrees canonicalizes registered worktree paths before assess_worktree_protections calls path_is_under_any. The two paths then do not compare as ancestors/equal, so the orca-session-sleeping blocker is omitted and an otherwise clean, merged worktree can be approved and removed despite its bound sleeping session. Canonicalize/validate the parsed paths (and the direct incomplete-dispatch paths) before matching them.
Useful? React with 👍 / 👎.
| let bytes = std::fs::read(&path) | ||
| .map_err(|_| "orca-worktree-json-read-failed".to_string())?; | ||
| options.protection.orca_sleep_worktree_paths = | ||
| disksage_lib::reclaim_protection::parse_orca_sleep_worktree_paths(&bytes)?; |
There was a problem hiding this comment.
When --orca-worktree-json is pointed at a very large or malformed export, this reads the entire file and then builds an unconstrained serde_json::Value plus an unconstrained path set before the audit's existing worktree limits apply. A large report can therefore exhaust memory or stall the cleanup command instead of returning a bounded evidence failure; enforce a byte limit before read/deserialization and cap accepted worktree entries.
Useful? React with 👍 / 👎.
| options.protection.orca_sleep_worktree_paths = | ||
| disksage_lib::reclaim_protection::parse_orca_sleep_worktree_paths(&bytes)?; |
There was a problem hiding this comment.
When a caller supplies --orca-worktree-json more than once—for example, to provide snapshots from multiple Orca workspaces—each occurrence is accepted but this assignment replaces the preceding sleep-path set. A worktree present only in an earlier snapshot then loses its orca-session-sleeping blocker and can become removable. Match --orca-terminal-json by rejecting duplicates, or merge all parsed path sets.
Useful? React with 👍 / 👎.
|
Scheduled review-feedback autofix for this PR head.
|
Integration repair required
This Draft remains a divergent predecessor and must not merge from
mainas-is. Preserve valid deltas only through canonical #457/#460 successors; no force-rebase, wholesale core transplant, or simple Close before verified carryover.Divergent source exact
#455 head remains
d2dc6cdf7cf4a1e2224f66bf36b545610a984e2a. Its oldergit_worktree.rsis not runtime authority because #444/#457/#460 carry newer Restart Manager, authenticated same-repository PR membership/completion evidence, approval/result separation, per-candidate live re-audit, filesystem-object identity, audit-v5 provenance, and the current ignored-artifact core authority.Exhaustive changed-file carryover audit
All 9 changed files in #455 were re-read against #460 exact
16eb832df0e3a37076e848e69fd6a9c06ee7d58b.scripts/ci/workflow-concurrency-contract.test.mjs: release first-attempt/rerun concurrency contract is present in the successor and is extended with Test workflow coverage.src/lib/releaseWorkflowRetryContract.test.ts: the same trigger-aware release retry contract is present, with additional run-identity artifact provenance assertions.src-tauri/src/bin/disksage-git-worktree-audit.rs+git_worktree_audit_help_exit.rs: Orca sleeping/incomplete-dispatch acquisition is preserved. Raw--closed-merged-*caller hints are intentionally absent; the successor uses authenticated GitHub PR evidence instead.src-tauri/src/reclaim_protection.rs: sleeping-session and incomplete-dispatch blockers are preserved; the Orca worktree parser is now byte-bounded. Ignored-artifact authority is handled in core audit/fingerprint classification rather than a caller hint surface.src-tauri/src/git_worktree.rs: feat(worktree): reinforce safe cleanup guards before removal #455 path identity, ignored-artifact protection, result/plan separation, sleeping cleanup semantics, and anti-spoof intent are superseded by the current v5 path/entry fingerprint, filesystem-object identity, authenticated PR membership/completion fields, one core ignored-artifact authority, and live re-audit.src-tauri/tests/git_worktree_closed_merged_spoof_authority.rs: its valid invariant (caller values cannot manufacture deletion authority) is stronger in the successor because those raw hint fields are removed entirely.src-tauri/tests/git_worktree_safe_cleanup_guards.rs: ignored-artifact, sleeping-session, approval/result, and real filesystem behavior are carried by the current focused fixtures/contracts; no old caller-hint API is restored.src/lib/api.ts: the old optionalmerged_closed_evidenceprojection is superseded by explicitclosed_pull_request_head,completed_pull_request_commit,open_pull_request_commit, andstale_open_pull_request_headevidence fields.The old backend
disposition_class_kohelper is not a required compatibility surface: current product UI renders buyer-facing classes directly, and no serialized/API contract depended on that helper. Localization of the remaining direct Korean UI copy is separately owned by #391 and must not be reintroduced as backend domain truth.Verified successor stack
Canonical #457 remains
b8bc816d0f929ab0e1f23b85e99e3dfce4f5d8c4. #460 is16eb832df0e3a37076e848e69fd6a9c06ee7d58b, a normal non-force 48-ahead / 0-behind descendant with merge base exactly #457.Current Test
35566885264now proves both available Windows lanes on the current successor exact: frontend106230402861SUCCESS and Rust/deletion-safety106230402885SUCCESS. Ubuntutest106230402900andllm-engine-build106230402745remain runnerless queued (runner_id=0, no steps); central runner capacity is tracked atContextualWisdomLab/.github#712and is not a reason to weaken or relabel consumer gates.Retirement condition
No remaining #455 source/API semantic needs to be transplanted. Keep this PR open until the current successor exact completes its required canonical evidence and is normally adopted into #457. Then #455 may be closed as a verified fully inherited predecessor. If the final exact exposes a regression in any mapped invariant above, repair the successor; do not revive the divergent core or caller-authoritative closed/merged hints.
Do not force-push/destructively rebase, transplant the divergent core wholesale, weaken fixtures/gates, self-approve, or simple-Close before canonical adoption.