fix(pr-review-toolkit): tolerate checkout-index partial failures - #75
Conversation
📝 WalkthroughWalkthroughThe plugin manifest version was bumped from 1.11.0 to 1.11.1. The checkout.sh script's plumbing checkout logic was refactored to exclude sandbox-protected files (dotfiles, editor configs, .claude/, .vscode/, .idea/**) using multiple ls-files exclusion patterns, with split error handling for read-tree, checkout-index, and update-ref failures. ChangesSandbox-protected checkout exclusion refactor
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant CheckoutScript
participant Git
CheckoutScript->>Git: git read-tree (merge SHA from FETCH_HEAD)
Git-->>CheckoutScript: success or read-tree failed (skip)
CheckoutScript->>Git: git ls-files with glob,exclude patterns
Git-->>CheckoutScript: filtered file list (excludes dotfiles, .claude/**, .vscode/**, .idea/**)
CheckoutScript->>Git: git checkout-index on filtered files
Git-->>CheckoutScript: success or checkout-index failed (skip)
CheckoutScript->>Git: git update-ref HEAD to merge SHA
Git-->>CheckoutScript: success or update-ref failed (skip)
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
There was a problem hiding this comment.
Pull request overview
This PR updates the pr-review-toolkit plumbing checkout so that checkout-index partial failures (e.g., from sandbox-protected files that can’t be unlinked) no longer force a fallback to MCP-only mode, while still treating read-tree / update-ref failures as fatal. It also bumps the plugin version for the release.
Changes:
- Split the previous monolithic “plumbing checkout” command chain into discrete steps, tolerating
checkout-indexnon-zero exit statuses. - Suppress
checkout-indexstderr to avoid noisy sandbox-related errors during partial checkout. - Bump
pr-review-toolkitplugin version from1.11.0to1.11.1.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| pr-review-toolkit/skills/review-pr/scripts/checkout.sh | Makes the checkout flow more tolerant of partial checkout-index failures while preserving fallback behavior for critical plumbing failures. |
| pr-review-toolkit/.claude-plugin/plugin.json | Version bump to publish the behavioral change. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # checkout-index may fail on sandbox-protected files (e.g. .gitmodules); | ||
| # partial checkout is acceptable — tolerate its exit status. | ||
| git ls-files -z -- . ':(exclude).claude/' \ | ||
| | git checkout-index -f -z --stdin 2>/dev/null || true | ||
|
|
There was a problem hiding this comment.
Good catch. Rather than adding error-filtering logic (which would be fragile across git versions and locales), the root cause is that the sandbox runtime has a mandatory deny list of files that are always write-protected (DANGEROUS_FILES + DANGEROUS_DIRECTORIES). On macOS, macGetMandatoryDenyPatterns() blocks these at any depth via **/filename globs.
Fixed by excluding all sandbox mandatory-deny files from the checkout-index pathspec using :(glob,exclude) patterns that match at any depth. With the root cause addressed, || true is replaced with || skip "checkout-index failed" so genuine errors properly trigger the MCP fallback.
The plumbing checkout treated any checkout-index failure as fatal, falling back to MCP-only mode even when most files were checked out successfully. This was triggered by sandbox-protected files like vendored .gitmodules that cannot be unlinked. Break the monolithic if-block into independent steps so read-tree and update-ref failures still trigger the fallback while checkout-index partial failures are silently tolerated. Assisted-by: Claude:claude-opus-4-6
ee9adaa to
02eebcf
Compare
| ':(glob,exclude)**/.vscode/**' \ | ||
| ':(glob,exclude)**/.idea/**' \ | ||
| | git checkout-index -f -z --stdin \ | ||
| || skip "checkout-index failed" |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pr-review-toolkit/skills/review-pr/scripts/checkout.sh`:
- Around line 38-56: The checkout logic in checkout.sh is over-excluding safe
root-level dotfiles, which can hide legitimate PR changes from disk-based
review. Update the git ls-files exclude patterns so they only skip
sandbox-protected paths when they are actually dangerous at nested locations,
and ensure top-level files like .gitmodules, .gitconfig, and shell rc files are
still checked out when part of the change. Also avoid silent fallback in the
checkout-index pipeline by aligning the exclusion list with the upstream
DANGEROUS_FILES/DANGEROUS_DIRECTORIES source used by the sandbox runtime, so
missing entries surface clearly instead of being hidden by skip.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9a9ce74b-3c56-408d-8c40-2df85d3b9017
📒 Files selected for processing (2)
pr-review-toolkit/.claude-plugin/plugin.jsonpr-review-toolkit/skills/review-pr/scripts/checkout.sh
| # Exclude files on the sandbox mandatory-deny list (DANGEROUS_FILES, | ||
| # DANGEROUS_DIRECTORIES in anthropic-experimental/sandbox-runtime). | ||
| # These are always write-protected regardless of sandbox config; | ||
| # checkout-index would fail with EPERM trying to unlink them. | ||
| git ls-files -z -- . \ | ||
| ':(glob,exclude)**/.gitconfig' \ | ||
| ':(glob,exclude)**/.gitmodules' \ | ||
| ':(glob,exclude)**/.bashrc' \ | ||
| ':(glob,exclude)**/.bash_profile' \ | ||
| ':(glob,exclude)**/.zshrc' \ | ||
| ':(glob,exclude)**/.zprofile' \ | ||
| ':(glob,exclude)**/.profile' \ | ||
| ':(glob,exclude)**/.ripgreprc' \ | ||
| ':(glob,exclude)**/.mcp.json' \ | ||
| ':(glob,exclude)**/.claude/**' \ | ||
| ':(glob,exclude)**/.vscode/**' \ | ||
| ':(glob,exclude)**/.idea/**' \ | ||
| | git checkout-index -f -z --stdin \ | ||
| || skip "checkout-index failed" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm DANGEROUS_FILES/DANGEROUS_DIRECTORIES semantics (repo-relative vs. home-dir only)
# and whether root-level matches are intentionally excluded from PR review checkouts.
gh api graphql -f query='{ search(query: "repo:anthropic-experimental/sandbox-runtime DANGEROUS_FILES", type: CODE, first: 5) { nodes { ... on CodeSearchResultItem { path } } } }' 2>/dev/null || trueRepository: cblecker/claude-plugins
Length of output: 701
🏁 Script executed:
#!/bin/bash
# Test git glob exclude pattern behavior
# Create a temporary test directory to verify if **/filename excludes root-level files
cd /tmp
mkdir -p git-glob-test
cd git-glob-test
git init
# Create test structure: files at root and nested directories
touch .gitmodules
mkdir -p subdir/nested
touch subdir/.gitmodules
touch subdir/nested/.gitmodules
touch other.txt
touch subdir/other.txt
git add -A
git commit -m "test"
# Test 1: List files WITHOUT the exclude pattern
echo "=== Files without exclude ==="
git ls-files
# Test 2: List files WITH the exclude pattern (as in the review comment)
echo -e "\n=== Files WITH ':(glob,exclude)**/.gitmodules' ==="
git ls-files ':(glob,exclude)**/.gitmodules'
# Test 3: Check if root-level .gitmodules is excluded
echo -e "\n=== Does root .gitmodules appear? ==="
git ls-files ':(glob,exclude)**/.gitmodules' | grep -c "^\\.gitmodules$" && echo "YES - root .gitmodules included" || echo "NO - root .gitmodules excluded"Repository: cblecker/claude-plugins
Length of output: 1368
🏁 Script executed:
#!/bin/bash
# Search the current repository for any reference to DANGEROUS_FILES or similar sandbox protections
echo "=== Searching for DANGEROUS_FILES reference ==="
rg -i "dangerous_files|dangerous_directories" --max-count=10 || true
echo -e "\n=== Searching for sandbox-runtime reference ==="
rg "sandbox-runtime|anthropic-experimental" --max-count=10 || true
echo -e "\n=== Looking for .gitmodules handling context ==="
rg "gitmodules" --max-count=20 || trueRepository: cblecker/claude-plugins
Length of output: 209
Root-level files matching excluded basenames will be silently hidden from disk-based review — including legitimate PR changes.
The ':(glob,exclude)**/.gitmodules' pattern excludes .gitmodules at every path depth, including the repository root. If a PR legitimately modifies the top-level .gitmodules (or .gitconfig, .bashrc, etc.), checkout-index will not write it to disk, even though the index and HEAD correctly reference the merged tree. Any reviewer using disk-based tools (rather than git show/git diff) will silently miss that change.
Additionally, this list (mirroring DANGEROUS_FILES/DANGEROUS_DIRECTORIES from the sandbox) is manually maintained and can drift from its upstream source. If checkout-index fails on a sandbox-protected path not enumerated here, the pipeline falls back to skip, making the failure silent rather than alerting reviewers to incomplete diffs.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pr-review-toolkit/skills/review-pr/scripts/checkout.sh` around lines 38 - 56,
The checkout logic in checkout.sh is over-excluding safe root-level dotfiles,
which can hide legitimate PR changes from disk-based review. Update the git
ls-files exclude patterns so they only skip sandbox-protected paths when they
are actually dangerous at nested locations, and ensure top-level files like
.gitmodules, .gitconfig, and shell rc files are still checked out when part of
the change. Also avoid silent fallback in the checkout-index pipeline by
aligning the exclusion list with the upstream
DANGEROUS_FILES/DANGEROUS_DIRECTORIES source used by the sandbox runtime, so
missing entries surface clearly instead of being hidden by skip.
Summary
checkout.shtreated anycheckout-indexfailure as fatal, falling back to MCP-only mode — even when most files were checked out successfully.gitmodulesthat cannot be unlinked (EPERM)if ! { ... && ... && ... }block into three independent steps:read-treeandupdate-reffailures still trigger the fallback, whilecheckout-indexpartial failures are silently toleratedTest plan
claude plugin validate ./pr-review-toolkitshellcheck pr-review-toolkit/skills/review-pr/scripts/checkout.sh.gitmodulesfiles (e.g. openshift/hypershift) and confirm the checkout succeeds instead of falling back to MCP-only moderead-treefails (e.g. not a git repo)Summary by CodeRabbit
1.11.1.