fix(doctor): race-free branch delete + ignored-secret guard (review findings on #2712) - #2713
Conversation
…(review on #2712) Both bots on the promotion PR caught that the ancestry re-proof ran BEFORE 'git worktree remove', leaving the same window open between the probe and the delete. The authoritative probe now runs AFTER removal — the only race-free point: with the worktree gone no checkout holds the branch, so the answer cannot be invalidated before 'branch -D'. The pre-removal probe stays as an early refusal that preserves the worktree. Codex P1: gitignored files are deleted by 'worktree remove' (git semantics) and are usually the reclaim's point (node_modules), but user-owned secret material must never ride a cleanup — classification now refuses when an ignored basename matches a sensitive-pattern set (.env*, *.pem, *.key, id_rsa*, credential/secret), listed via --ignored=traditional so fully-ignored dirs stay one bounded line. Also from the pass: git probes bounded (10s timeout, 8MB maxBuffer); IntegrationBranch exported alongside removeLaunchWorktree; reviewer snapshot paths gain a required <unique> suffix (collision across concurrent reviews/repos sharing a basename) in skill+mirror+wish; native-surfaces pointers name dispatch-contract.md rule 3; wish validation blocks record full 'bun run check' on CI as the merge gate. 619 tests green. Co-authored-by: Liraz Siri <liraz@liraz.org>
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![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: cd958d6d1a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // checked-out branch, and no checkout holds it now), so an ancestry answer | ||
| // taken HERE cannot be invalidated before `-D` — this is the only point in | ||
| // the sequence where the proof and the delete are race-free. | ||
| const frozen = git(root, ['merge-base', '--is-ancestor', `refs/heads/${entry.branch}`, integration.ref]); |
There was a problem hiding this comment.
When another process advances this now-unchecked-out branch after the ancestry probe but before git branch -D, the forced deletion removes the new, unmerged tip. Removing the worktree does not freeze the branch; it makes the branch available for another checkout or direct ref update. git branch -h describes -D as deleting even when unmerged, whereas git update-ref -h supports deletion with an <old-val>; use an atomic compare-and-delete against the exact tip that passed the proof.
Useful? React with 👍 / 👎.
| function findIgnoredSecret(path: string): string | null { | ||
| // `traditional` collapses a fully-ignored directory (node_modules/) to one | ||
| // line, keeping the listing bounded on exactly the worktrees worth reclaiming. | ||
| const listing = git(path, ['status', '--porcelain', '--ignored=traditional']); |
There was a problem hiding this comment.
When a secret is inside an ignored directory whose own basename is not sensitive, such as cache/.env with cache/ ignored, this command emits only !! cache/, so the matcher never sees .env and --fix deletes it. The --ignored=traditional documentation states that ignored directories are collapsed unless --untracked-files=all is specified, which is also reproducible with this exact command. The guard must inspect nested ignored files before declaring the worktree removable.
AGENTS.md reference: AGENTS.md:L31-L31
Useful? React with 👍 / 👎.
| const rel = line.slice(2).trim().replace(/\/$/, ''); | ||
| const base = rel.split('/').pop() ?? rel; | ||
| if (SENSITIVE_IGNORED.some((re) => re.test(base))) return rel; |
There was a problem hiding this comment.
When a sensitive ignored filename requires Git quoting, such as private cert.pem, porcelain emits !! "private cert.pem"; the retained closing quote makes /\.pem$/ and /\.key$/ fail, so the worktree remains removable and the secret is deleted. git status -h exposes -z specifically for NUL-terminated machine-readable entries; parse that form rather than trimming line-oriented quoted output.
AGENTS.md reference: AGENTS.md:L31-L31
Useful? React with 👍 / 👎.
| const secret = findIgnoredSecret(path); | ||
| if (secret !== null) return { ...entry, disposition: 'dirty', reason: `ignored secret present (${secret})` }; |
There was a problem hiding this comment.
When a process writes an ignored .env after classification—particularly while cleanup is still scanning and sizing other worktrees—the new guard is never run again. A non-forced git worktree remove still deletes ignored files (confirmed with the CLI; --force is only described as overriding dirty or locked worktrees), so the newly created secret is lost. Repeat the secret check at the removal boundary and refuse if it changes or cannot be listed.
AGENTS.md reference: AGENTS.md:L31-L31
Useful? React with 👍 / 👎.
| // Bounded: a probe hanging on a lock, credential prompt, or network mount | ||
| // must not wedge `genie doctor`; a timeout surfaces as a refusal. | ||
| timeout: 10_000, | ||
| maxBuffer: 8 * 1024 * 1024, |
There was a problem hiding this comment.
When removing a large worktree or one on a slow filesystem takes more than ten seconds, this shared wrapper terminates git worktree remove midway because the timeout applies to mutations as well as probes. That can leave a partially deleted checkout and stale worktree registration while reporting that removal was refused. Keep the bound on read-only probes, but do not kill an in-progress destructive operation at the same fixed deadline.
Useful? React with 👍 / 👎.
2 participants
Addresses all 8 review threads on promotion #2712 — the two real code findings plus the quick wins:
worktree remove, so the probe→delete window stayed open. The authoritative probe now runs AFTER removal — the only race-free point (no checkout holds the branch once the worktree is gone) — with the pre-removal probe kept as an early, worktree-preserving refusal..env*,*.pem,*.key,id_rsa*, credential/secret) now refuse removal with the file named; ordinary ignored content (node_modules) stays disclosed-but-removable — refusing on any ignored file would nullify the feature.--ignored=traditionalkeeps the probe bounded.IntegrationBranchexported (CodeRabbit); snapshot paths require a<unique>suffix in skill+mirror+wish (CodeRabbit Major); native-surfaces pointers namedispatch-contract.md(Minor); wish validation blocks record the full-check CI gate (Minor).619 tests green in src/genie-commands/ (new: secret-refusal + node_modules-passes), check:fast exit 0, mirrors byte-identical. #2712 will need update-branch + re-approval after this merges.