Skip to content

feat(damage-control): allowlist clearing of orphaned git lockfiles (keep package locks protected) - #1836

Merged
POWERFULMOVES merged 2 commits into
mainfrom
chore/allow-git-lockfile-rm
Jun 17, 2026
Merged

POWERFULMOVES merged 2 commits into
mainfrom
chore/allow-git-lockfile-rm

Conversation

@POWERFULMOVES

@POWERFULMOVES POWERFULMOVES commented Jun 17, 2026

Copy link
Copy Markdown
Owner

What

Adds a narrowly-scoped allowlist so the damage-control Bash guard permits clearing git's own orphaned internal lockfiles (.git/config.lock, .git/index.lock), while keeping the *.lock read-only protection on package locks intact.

Why

The *.lock readOnlyPath correctly blocks editing/deleting package locks (yarn.lock, uv.lock, Cargo.lock, …). But it also caught git's internal lockfiles, which are a different thing entirely:

  • They orphan as 0-byte sentinels when an interrupted git config/index write dies between creating the lock and the atomic rename (killed command, closed session, OneDrive/AV holding the handle).
  • While present they block every subsequent git config/index operation: branch -D, push -u, worktree add, add/commit.
  • Git cannot auto-clear them — it can't distinguish a stale lock from a live concurrent writer, so it never steals a config/index lock.

The guard previously forced an agent to defer this routine, safe cleanup to the operator (manual shell rm), even though no live git process held the lock.

How

New bashDeleteAllowlist (data + stated reason in patterns.yaml), evaluated:

  • after the destructive-pattern block (recursive/force deletes still blocked), and
  • after zeroAccessPaths (never bypassed),
  • before read-only / no-delete checks.

The single entry's regex is anchored to the whole command, so only a bare delete of git's own lockfiles passes. Adding a new exception is documented inline: anchor the command, keep it surgical, state a reason.

Safety / tests

test_gitlock_allowlist.py (new) — 10 cases:

  • ✅ allow: bare delete of .git/config.lock, .git/index.lock, both, ./-prefixed
  • 🚫 block: yarn.lock, uv.lock (package locks), recursive/force deletes (step 1), chained commands (&&, ;)

Existing test_insight_edits.py still 10/10 (no regression). patterns.yaml validates; *.lock read-only entry unchanged.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes

    • Allows safe removal of stale Git internal lockfiles (.git/config.lock and .git/index.lock) that may block Git operations.
  • Tests

    • Added comprehensive test coverage for Git internal lockfile cleanup allowlist, validating that permitted rm commands proceed while malformed operations remain blocked.

…eep package locks protected

The *.lock readOnlyPath rightly blocks editing/deleting package locks, but it also
blocked clearing git's OWN internal lockfiles (.git/config.lock, .git/index.lock).
Those orphan as 0-byte sentinels when an interrupted config/index write dies
before the atomic rename (killed cmd, closed session, OneDrive/AV holding the
handle); while present they block every subsequent git config/index op (branch -D,
push -u, worktree add, add/commit) and git cannot auto-clear them (can't tell a
stale lock from a live one). The guard then forced agents to defer to the operator
for a routine, safe cleanup.

Add a new bashDeleteAllowlist (data + reason in patterns.yaml) checked AFTER the
destructive-pattern block (recursive/force deletes still blocked) and AFTER
zeroAccessPaths (never bypassed), BEFORE read-only/no-delete. The single entry is
anchored to the WHOLE command, so only a bare delete of git's own lockfiles passes
- package locks (yarn.lock/uv.lock/...), flagged deletes, and chained commands all
stay blocked.

Verified by test_gitlock_allowlist.py (10 cases: 4 allow, 6 block) + existing
test_insight_edits.py still 10/10.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jun 17, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Warning

Review limit reached

@POWERFULMOVES, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 36 minutes and 57 seconds. Learn how PR review limits work.

Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file).

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits.

🚦 How do rate limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 9e131f28-2a14-46fc-b802-d8670af3bb42

📥 Commits

Reviewing files that changed from the base of the PR and between 8fb4775 and 1541c02.

📒 Files selected for processing (2)
  • .claude/hooks/damage-control/patterns.yaml
  • .claude/hooks/damage-control/test_gitlock_allowlist.py
📝 Walkthrough

Walkthrough

Adds a bashDeleteAllowlist mechanism to the damage-control hook. A new YAML section defines an anchored regex permitting bare rm of .git/config.lock and .git/index.lock. The hook's check_command function gains a new evaluation step that matches against those patterns and bypasses subsequent blocking logic. A standalone test script validates the allowed and blocked cases.

Changes

Git Lockfile rm Allowlist

Layer / File(s) Summary
Allowlist config entry and hook enforcement
.claude/hooks/damage-control/patterns.yaml, .claude/hooks/damage-control/bash-tool-damage-control.py
patterns.yaml gains bashDeleteAllowlist with one anchored regex for bare rm of .git/config.lock and/or .git/index.lock; check_command gains an evaluation step after zeroAccessPaths that returns (blocked=False, ask=False) on a match, skipping read-only/no-delete enforcement. Invalid regex entries are warned and skipped.
Allowlist verification test script
.claude/hooks/damage-control/test_gitlock_allowlist.py
Standalone script dynamically loads the hook, runs a fixed matrix of rm commands (bare lockfile removes, ./-prefixed forms, both locks together, flags -rf/-f, &&/; chaining, package lock files), asserts expected allowed/blocked outcomes, and exits non-zero on any mismatch.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

  • POWERFULMOVES/PMOVES.AI#1214: Extends the same damage-control hook with a chitBypassPatterns exception for broader .git rebase/merge crash artifact cleanup, following the same pattern-based allowlist approach as this PR.

Poem

🐇 Hop, hop — the lockfile's stuck,
.git/config.lock won't budge with luck.
A careful allowlist, anchored tight,
lets bare rm set the repo right.
No flags, no chains — just clean and neat,
the rabbit cheers: the tests complete! 🎉

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Description check ⚠️ Warning The description is missing required Testing and Required Checks sections specified in the template, though it provides good context via What/Why/How. Add Testing section documenting test commands/output (verify test_gitlock_allowlist.py passes) and Required Checks section confirming contract/documentation reviews.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: adding an allowlist for git lockfile deletion while protecting package locks.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch chore/allow-git-lockfile-rm

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8fb4775503

ℹ️ 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".

Comment thread .claude/hooks/damage-control/patterns.yaml Outdated
@claude

claude Bot commented Jun 17, 2026

Copy link
Copy Markdown

Triage from chatgpt-codex-connector[bot]: No P0/P1 findings. P2: 1, P3: 0. P2 — .claude/hooks/damage-control/patterns.yaml:1053 / bash-tool-damage-control.py: re.search() with backtracking whitespace absorbs newlines in allowlist match; embedded-newline injection is correctly blocked today by the anchored pattern, but adding an explicit newline-rejection guard before the regex would strengthen defense-in-depth.

Codex P2: the allowlist regex used \s* between operands, and \s matches a
newline. A newline terminates a command in Bash, so `<del> .git/config.lock\n
.git/index.lock` matched the pattern but Bash would execute the second line as
its own command — breaking the "no chaining" guarantee. Restrict all separators
to [ \t] (spaces/tabs) so any newline falls through to the read-only block.

Added two newline-injection cases to test_gitlock_allowlist.py (now 12 cases, all
pass): newline-as-separator and newline-then-command both blocked.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@POWERFULMOVES
POWERFULMOVES merged commit 145e4c1 into main Jun 17, 2026
18 checks passed
@POWERFULMOVES
POWERFULMOVES deleted the chore/allow-git-lockfile-rm branch June 17, 2026 17:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant