feat(pr-review-toolkit): production-harden checkout, overlap identity, and manifest verification - #83
Open
cblecker wants to merge 13 commits into
Open
feat(pr-review-toolkit): production-harden checkout, overlap identity, and manifest verification#83cblecker wants to merge 13 commits into
cblecker wants to merge 13 commits into
Conversation
…, and manifest verification Fixes from a design-doc-driven audit of the review-pr flow, each validated with a functional git harness (13 scenarios) and a stubbed end-to-end run of the workflow script (11 scenarios), then adversarially re-verified: checkout.sh - verify origin matches the PR base repository before fetching (per the rewrite plan's eligibility ordering), tolerating URL query/fragment tails - apply merge deletions and prune emptied directories (plumbing checkout previously left deleted files readable by specialists) - restore the original index and tracked worktree content on mid-checkout failure, including removing partially-written merge additions - mark protected paths the merge touched as skip-worktree so a successful checkout no longer leaves the repo permanently dirty on reruns - emit prDiffFileCount and mergeDiffFileCount so the workflow can keep a verified local manifest when the base branch has advanced, and reject an incompletely parsed one - run from the repository toplevel; refresh the index after checkout review-pr.js - preserve the synthesizer's overlap classification instead of downgrading it via token matching; attach thread identity as a consistent triple so threadId/commentId/isResolved always describe one thread (also when merging duplicate findings) - propagate isResolved as tri-state; unknown resolution is no longer recorded as false, and the posting preview says when it is unknown - surface review-thread collection failure in logs and reviewMeta instead of silently disabling overlap classification - cap specialist prompt manifests for very large PRs with an explicit per-category omission summary (complete manifest still collected) - parse diff headers from +++ b/ lines handling trailing tabs and C-quoted paths; decode quoted paths so hunk keys match manifest paths - add a comments signal so comment-only changes select comment-analyzer when the full diff is available agents - enforce read-only analysis via a tools allowlist (GitHub write tools were previously only discouraged by prose) skills/docs - update SKILL contracts for the new checkout output, C-quoted path decoding, and truncation/thread-failure warnings - address-pr-feedback: simpler mainline detection, plan-mode wording fix, resolved-thread awareness in both collection paths - README: correct checkout description, permissions (including reply and issue-comment capabilities), APPROVE event, agent table; mark the rewrite plan as historical - bump plugin to 1.13.0 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AjwsgsMM77sFNC6GvGjz1P
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR hardens local merge checkout and manifest verification, preserves tri-state review-thread resolution, adds review-board diagnostics and manifest truncation metadata, narrows specialist permissions, updates feedback workflows, and increments the plugin version. ChangesReview workflow hardening
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Reviewer
participant ReviewWorkflow
participant checkout.sh
participant GitHubMCP
Reviewer->>ReviewWorkflow: start PR review
ReviewWorkflow->>checkout.sh: validate and prepare synthetic merge
checkout.sh-->>ReviewWorkflow: return manifest and diff counts
ReviewWorkflow->>GitHubMCP: collect threads and fallback data when needed
GitHubMCP-->>ReviewWorkflow: return review context
ReviewWorkflow-->>Reviewer: present review board and posting options
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/review-pr.js`:
- Around line 985-1007: Update unquoteGitPath to accumulate octal escape values
as raw bytes and decode the complete byte sequence as UTF-8, rather than
converting each octal escape directly with String.fromCharCode. Preserve
existing handling for ordinary characters and non-octal escapes, and ensure
paths such as escaped UTF-8 filenames match manifest keys used by
hunks[currentFile].
In `@pr-review-toolkit/skills/review-pr/scripts/checkout.sh`:
- Around line 25-27: Extend the checkout preflight after the existing git
diff-index check to detect untracked files as well as tracked modifications, and
call the existing skip path with the clean-worktree message when any are
present. Use Git’s standard untracked-file listing or status check, preserving
the current behavior for clean worktrees and avoiding changes to checkout-index
or restore_original.
🪄 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 Plus
Run ID: 8d87fd3f-108a-4ed3-919d-85bd905da25d
📒 Files selected for processing (8)
pr-review-toolkit/.claude-plugin/plugin.jsonpr-review-toolkit/README.mdpr-review-toolkit/agents/pr-review-analysis-readonly.mdpr-review-toolkit/docs/WORKFLOW_REWRITE_PLAN.mdpr-review-toolkit/skills/address-pr-feedback/SKILL.mdpr-review-toolkit/skills/review-pr/SKILL.mdpr-review-toolkit/skills/review-pr/review-pr.jspr-review-toolkit/skills/review-pr/scripts/checkout.sh
There was a problem hiding this comment.
Pull request overview
Hardens pr-review-toolkit checkout safety, manifest verification, overlap handling, and reviewer permissions.
Changes:
- Adds rollback-aware local merge checkout and manifest verification.
- Improves thread identity, large-PR prompts, and diff-path parsing.
- Updates agent restrictions, skill contracts, and documentation.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
pr-review-toolkit/.claude-plugin/plugin.json |
Bumps plugin minor version. |
pr-review-toolkit/agents/pr-review-analysis-readonly.md |
Adds an explicit read-only tool allowlist. |
pr-review-toolkit/README.md |
Documents checkout, workflow, and permission changes. |
pr-review-toolkit/docs/WORKFLOW_REWRITE_PLAN.md |
Marks the rewrite plan as historical. |
pr-review-toolkit/skills/address-pr-feedback/SKILL.md |
Improves mainline detection and resolved-thread handling. |
pr-review-toolkit/skills/review-pr/SKILL.md |
Updates checkout, manifest, and preview contracts. |
pr-review-toolkit/skills/review-pr/scripts/checkout.sh |
Implements origin checks, rollback, deletions, and verification counts. |
pr-review-toolkit/skills/review-pr/review-pr.js |
Hardens overlap, manifest, thread, and diff processing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
… and path decoding Addresses the four valid items from CodeRabbit and Copilot review on #83: - checkout.sh: refuse to overwrite untracked local files — paths the merge adds that already exist on disk now skip before any mutation, matching git checkout's own guard (targeted collision check rather than rejecting all untracked files, which would disable the local path for most real worktrees) - checkout.sh: require the effective origin host to be github.com before the owner/repo comparison; PR URLs are always github.com, so a same-named repository on another host is never fetched - review-pr.js: decode octal path escapes as UTF-8 byte sequences via decodeURIComponent (Buffer is unavailable in the Workflow sandbox, so the bots' suggested fix is adapted), with a per-byte fallback for malformed sequences - review-pr.js/SKILL.md: instruct the synthesizer to copy thread ids from its input when classifying overlaps, and make the posting preview flag overlap findings with no reply target instead of silently falling back to a new line comment Validated: checkout harness extended with untracked-collision, evil-host, and host-extraction scenarios (all green, plus regressions); workflow simulation extended with a UTF-8 quoted-path scenario; decoder unit checks including malformed-sequence fallback; plugin validate and markdownlint clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AjwsgsMM77sFNC6GvGjz1P
- checkout.sh: collision guard only treats regular files and symlinks as untracked collisions, so a tracked directory replaced by a file (dir-to-file type change) is no longer spuriously refused; genuine untracked collisions still skip before mutation - review-pr.js: validate the parsed manifest against mergeDiffFileCount unconditionally — a lossy parse that coincidentally equals GitHub's changedFiles no longer bypasses verification - review-pr.js: thread collector reports collectionFailed explicitly, so a failed read is distinguishable from a PR with no review threads and the board warning fires reliably - review-pr.js: comments signal matches #TODO/#noqa-style line comments while still excluding shebangs and C-preprocessor directives (line-start anchoring is kept deliberately so CSS colors and URL fragments do not trigger it) - address-pr-feedback: mainline detection queries the remote first (cached origin/HEAD can be stale after a default-branch rename) with the local symbolic ref as offline fallback; deduplicated items track isResolved per reply target so resolution warnings apply per thread - review-pr SKILL.md: trimmed under the skillsaw context budget (was 3,089 tokens against a 3,000 warn limit) with no semantic changes Validated: checkout harness adds a dir-to-file type-change scenario plus collision and protected-path regressions; workflow simulation adds lossy parse and collector-reported-failure scenarios (14 total green); comments regex unit checks; plugin validate, markdownlint, and skillsaw (A+, 0 warnings) all clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AjwsgsMM77sFNC6GvGjz1P
…ut and merge edge cases - checkout.sh: collision guard treats any non-directory inode (FIFO, socket, device node, not just regular files and symlinks) as untracked data that must not be overwritten, while still allowing dir-to-file type changes through - checkout.sh: rollback failures propagate — restore_original reports whether tracked state was confirmed restored, and every mutation-phase failure goes through fail_checkout, whose skip reason tells the user to run git reset --hard when the restore was incomplete instead of pretending the fallback repository is clean - review-pr.js: bestOverlap breaks equal-status ties in favor of the duplicate that carries a reply target, so merging can no longer discard a confirmed threadId/commentId and then claim no reply target exists - review-pr.js: comments signal also matches inline/trailing comments (whitespace-delimited // and #, inline /* */) while keeping CSS colors, URLs, shebangs, and preprocessor directives excluded Validated: checkout harness adds a FIFO-collision scenario plus dir-to-file, file-collision, protected-path, and happy-path regressions; workflow simulation adds an equal-status tie-break scenario (15 total green); inline comment regex unit checks; plugin validate and skillsaw (A+, 0 warnings) clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AjwsgsMM77sFNC6GvGjz1P
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (5)
pr-review-toolkit/skills/review-pr/scripts/checkout.sh:146
- This guard still allows an existing symlink whose target is a directory because
-dfollows symlinks. For a merge-added path,checkout-index -fcan then replace that untracked symlink, and rollback removes the replacement rather than restoring the symlink. Test-Lindependently so untracked symlinks are never overwritten.
if [[ (-e "${added_path}" || -L "${added_path}") && ! -d "${added_path}" ]]; then
pr-review-toolkit/skills/review-pr/review-pr.js:788
- On equal overlap status this prefers a left-side
threadIdover a right-sidecommentId, even though onlycommentIdis a usable reply target. A duplicate merge can therefore discard a valid reply target and later report that none is available. Prefer either side'scommentIdbefore falling back to a thread-only identity.
selected = (left && (left.commentId || left.threadId)) ? left : (right || left)
pr-review-toolkit/skills/review-pr/review-pr.js:858
- If the synthesizer supplies an identity that does not resolve, this fallback can attach a different token-matched thread's identity—the exact mismatch the following comment says must be avoided. Only infer a thread by content when no identity was supplied; otherwise keep the unresolved identity together rather than redirecting a reply.
if (!matched) {
const best = bestThreadMatch(item, threads)
matched = best ? best.thread : null
}
pr-review-toolkit/skills/review-pr/review-pr.js:153
- The schema makes
collectionFailedoptional while the consumer treats an omitted flag plusthreads: []as a successful collection. A collector can therefore return schema-valid output after a failed read and silently disable the new warning. Require the flag so success versus failure is explicit.
required: ['id', 'path', 'author', 'body']
pr-review-toolkit/skills/review-pr/review-pr.js:1700
- These metadata fields still serialize explicit
nullcounts as0, soreviewMeta.sourcesclaims a known zero instead of the documented-1unknown sentinel. Apply the same null-aware conversion used for manifest validation.
prDiffFileCount: asNumber(configSources ? configSources.prDiffFileCount : undefined, -1),
mergeDiffFileCount: asNumber(configSources ? configSources.mergeDiffFileCount : undefined, -1),
…ity and sentinel edge cases - checkout.sh: collision guard tests -L independently of -d, so an untracked symlink pointing at a directory is protected instead of being replaced by checkout-index - review-pr.js: sourceCount helper maps null/undefined verification counts to the -1 unknown sentinel everywhere (Number(null) is 0), and the local manifest is no longer trusted at all when mergeDiffFileCount is missing — checkout.sh always emits it, so absence means the contract was broken - review-pr.js: equal-status duplicate merges prefer the side with a commentId (the id posting actually uses) over a thread-only identity - review-pr.js: content matching attaches a thread only when the synthesizer supplied no identity; a supplied-but-unresolved identity is kept as-is so a reply is never silently redirected to a token-matched thread that may be the wrong conversation - review-pr.js: THREAD_SCHEMA requires collectionFailed (removing a duplicate required key that made the earlier addition ineffective), so a failed read can never be schema-valid while resembling a comment-free PR Validated: checkout harness adds a symlink-to-directory collision scenario plus FIFO, dir-to-file, file-collision, protected-path, and happy-path regressions; workflow simulation adds missing-count fallback with -1 provenance and commentId-preference tie-break scenarios (17 total green); plugin validate, markdownlint, and skillsaw (A+, 0 warnings) clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AjwsgsMM77sFNC6GvGjz1P
…targeting - SKILL.md: a reply target the API rejects at posting time no longer falls back to a new line comment silently — the finding is converted, the revised preview is shown, and approval is requested again, matching the invalid-line-location flow - review-pr.js: when the synthesizer supplies both thread ids, threadId is authoritative — commentId is used for lookup only when no threadId was supplied, so a mangled id pair can never resolve to whichever thread happens to appear first and misdirect an approved reply - README: document that a protected path deleted by a PR remains on disk as an untracked leftover (the sandbox blocks unlinking it); preflight and reruns are unaffected and the merge diff still carries the real change Validated: workflow simulation adds a conflicting-id scenario asserting the identity triple stays within the threadId-resolved conversation (18 total green); plugin validate, markdownlint, and skillsaw (A+, 0 warnings) clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AjwsgsMM77sFNC6GvGjz1P
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
pr-review-toolkit/skills/address-pr-feedback/SKILL.md:27
- The offline fallback is not reliably reached: in a normal shell, this pipeline’s exit status is
sed’s, so a failedgit ls-remotestill typically exits 0 with empty output. That leavesmainlineempty and can let the skill continue while checked out on the mainline branch—the safety check this phase is meant to enforce. Trigger the fallback when the command returns an empty branch name (not only on a nonzero pipeline status), and stop safely if both lookups are empty.
`git ls-remote --symref origin HEAD 2>/dev/null | grep "^ref:" | awk '{print $2}' | sed 's|refs/heads/||'`.
If that fails (offline), fall back to
`git symbolic-ref --short refs/remotes/origin/HEAD 2>/dev/null` and strip
the `origin/` prefix
…den mainline detection - checkout.sh: read-tree clears skip-worktree bits, and on an idempotent rerun the orig->merge diff is empty so the marking loop never restores them — a third run then saw a dirty tracked worktree and silently fell back to MCP. Bits carried by protected paths are now captured before any read-tree and reapplied after the checkout (and after rollback, whose restore read-tree clears them too) - address-pr-feedback: the ls-remote mainline pipeline exits 0 with empty output when the remote is unreachable (the pipeline status is sed's), so the fallback is now triggered on empty output rather than exit status, and execution stops safely when both lookups return nothing instead of proceeding with the mainline safety check unresolved Validated: checkout harness adds a three-run idempotency scenario asserting the skip-worktree bit and clean status survive every rerun, plus collision, dir-to-file, and happy-path regressions; plugin validate, markdownlint, and skillsaw (A+, 0 warnings) clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AjwsgsMM77sFNC6GvGjz1P
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
pr-review-toolkit/skills/review-pr/scripts/checkout.sh:168
- This captures every pre-existing
skip-worktreeentry as though the script owned it. The clean-worktree preflight does not see local edits behind that bit;read-treethen clears the bit andcheckout-index -foverwrites those paths before the bit is reapplied, which can destroy user changes (and also materialize sparse-checkout paths). Treat only sandbox-protected entries as script-owned, and abort or separately verify any non-protectedskip-worktreeentries before mutating the index.
skip_worktree_paths=()
while IFS= read -r -d '' entry; do
if [[ "${entry}" == "S "* ]]; then
skip_worktree_paths+=("${entry:2}")
fi
done < <(git ls-files -t -z 2>/dev/null)
pr-review-toolkit/skills/review-pr/review-pr.js:889
- When an authoritative
threadIdfails lookup, this still preserves the synthesizer'scommentId. Posting uses onlycommentId, so a mangled pair whose comment ID belongs to another valid thread will still send the approved reply to that wrong thread—the exact identity-mixing failure this branch is intended to prevent. Drop the comment target and resolution state when a suppliedthreadIdcannot be resolved so the preview requires a new posting choice.
threadId: matched ? (matched.id || '') : (existing.threadId || ''),
commentId: matched ? (matched.commentId || undefined) : (existing.commentId || undefined),
isResolved: matched ? knownResolved(matched.isResolved) : knownResolved(existing.isResolved),
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
pr-review-toolkit/skills/review-pr/review-pr.js:1463
- The 400-file cap does not actually bound prompt size because
byDirectorycreates one entry per distinct two-segment directory among every omitted file. A very large PR with files spread across many directories can therefore still inject an O(total files) omission index into every specialist prompt, defeating the hardening this function is intended to provide. Cap directory entries too and aggregate the remainder into counts/categories.
const byDirectory = {}
omitted.forEach(file => {
const segments = String(file.path || '').split('/')
const dir = segments.length > 1 ? segments.slice(0, Math.min(2, segments.length - 1)).join('/') : '.'
if (!byDirectory[dir]) byDirectory[dir] = { count: 0, categories: {} }
…n state and prompt bounds - checkout.sh: the hidden-state guard also refuses assume-unchanged entries outside the sandbox-protected set — diff-index skips them like skip-worktree paths, so checkout-index -f could overwrite local edits the preflight cannot see - review-pr.js: the omitted-file directory index is capped at 100 entries (largest first, deterministic ordering) with the remainder aggregated into counts and categories, so a PR spread across thousands of directories cannot reintroduce the O(files) prompt payload the file cap exists to prevent - SKILL.md: the full-diff command pins --src-prefix=a/ --dst-prefix=b/ so header parsing (and every diff-derived reviewer signal) survives diff.noprefix and diff.mnemonicPrefix configuration Validated: checkout harness adds assume-unchanged refusal and a live noprefix-config prefix-override check, plus skip-worktree, three-run, and happy-path regressions; workflow simulation adds a 600-file many-directory scenario asserting the capped index and aggregate counts reconcile (20 total green); plugin validate, markdownlint, and skillsaw (A+, 0 warnings) clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AjwsgsMM77sFNC6GvGjz1P
Owner
Author
|
@coderabbitai review Generated by Claude Code |
|
✅ Action performedReview finished.
|
cblecker
commented
Jul 29, 2026
…tions, dynamic mainline context - pr-review-analysis-readonly: drop the tools allowlist (it hardcoded gopls and specific plugin installs, locking out other languages' read tools) and return to disallowedTools, extended with the known GitHub write tools; restore the original language-agnostic prose - address-pr-feedback: collect branch and remote-mainline state as dynamic context at invocation instead of instructing multi-step shell orchestration; same fail-closed semantics over the embedded output - README: describe the denylist posture instead of the allowlist Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AjwsgsMM77sFNC6GvGjz1P
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/agents/pr-review-analysis-readonly.md`:
- Around line 16-21: Update the read-only policy for the agent configuration to
fail closed for GitHub MCP write capabilities, rather than relying only on the
named denylist entries. Add a capability/session-level restriction or equivalent
validation that rejects any write-capable GitHub tool not explicitly covered,
including future branch and label write tools, while preserving access to
read-only tools.
In `@pr-review-toolkit/skills/address-pr-feedback/SKILL.md`:
- Around line 32-38: Update the error classification instructions near the
cached mainline fallback: remove the broad “unable to access” connectivity match
and require confirmed DNS, transport, or offline indicators only. Ensure HTTP
authentication failures, including 401/403 responses, remain in the fail-closed
path that displays the captured git error and stops without using cached
origin/HEAD.
🪄 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 Plus
Run ID: 63bd2f9b-9664-4453-80fb-0d863bea6027
📒 Files selected for processing (3)
pr-review-toolkit/README.mdpr-review-toolkit/agents/pr-review-analysis-readonly.mdpr-review-toolkit/skills/address-pr-feedback/SKILL.md
🚧 Files skipped from review as they are similar to previous changes (1)
- pr-review-toolkit/README.md
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
pr-review-toolkit/skills/review-pr/scripts/checkout.sh:177
- Assume-unchanged state on protected paths is allowed through the hidden-state check below, but this capture only saves
S/s(skip-worktree).read-treeclears the other lowercase tags, so a protected path with hidden local edits can become visibly dirty after checkout or rollback whilerestore_originalstill reports success. Capture assume-unchanged paths separately and reapply them alongside skip-worktree on both paths.
while IFS= read -r -d '' entry; do
protected_files+=("${entry:2}")
if [[ "${entry:0:1}" == "S" || "${entry:0:1}" == "s" ]]; then
skip_worktree_paths+=("${entry:2}")
fi
…paths across checkout read-tree clears assume-unchanged bits along with skip-worktree, but the pre-mutation capture only saved S/s tags. A protected path hidden with assume-unchanged alone (tag h) lost its flag after a successful checkout, turning visibly dirty and degrading every later run to MCP fallback. Capture lowercase-tagged protected entries separately and reapply both flags after any read-tree, in checkout and rollback alike. Verified: h tag and local edit survive checkout and idempotent rerun; hidden-state refusal outside the protected set unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AjwsgsMM77sFNC6GvGjz1P
… mainline detection "unable to access" is not connectivity-specific — git emits it for 401/403 HTTP failures too, which would have routed authentication errors to the stale cached-fallback path. Restrict the fallback to transport-level causes and state that "unable to access" with an HTTP status fails closed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AjwsgsMM77sFNC6GvGjz1P
Owner
Author
|
@coderabbitai review Generated by Claude Code |
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
pr-review-toolkit/agents/pr-review-analysis-readonly.md:21
- This is still a denylist, so specialist agents retain every GitHub MCP write capability not named here. The configured GitHub plugin always loads broad management toolsets (including discussions, gists, and projects), and future write tools would also be permitted by default; therefore the workflow is not actually read-only and this contradicts the PR's stated allowlist hardening. Replace the denylist with an explicit allowlist of repository reads and known read-only MCP tools.
- mcp__plugin_github_github__add_reply_to_pull_request_comment
- mcp__plugin_github_github__add_issue_comment
- mcp__plugin_github_github__issue_write
- mcp__plugin_github_github__push_files
- mcp__plugin_github_github__create_or_update_file
- mcp__plugin_github_github__merge_pull_request
- mcp__plugin_github_github__resolve_review_thread
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
pr-review-toolkit/skills/review-pr/scripts/checkout.sh:251
- Include protected type changes in this marking pass. Git reports regular-file↔symlink/gitlink changes as
T, so the protected worktree keeps the old type while the merge index gets the new one, but noskip-worktreebit is set. The script then reportsCHECKOUT_OKwith a dirty index/worktree and the next run falls back at preflight.
done < <(git diff --name-only -z --no-renames --diff-filter=AM \
"${orig_head}" "${merge_sha}" -- "${protected_match[@]}" 2>/dev/null)
pr-review-toolkit/skills/review-pr/review-pr.js:1510
- For PRs above the cap, this is only a 400-file subset, but
analysisPromptstill tells every specialist that the shared context contains the “complete changed-file manifest” and not to refetch the full list (line 1546). That conflicts withmanifestTruncationand can make omitted files look absent rather than summarized. Make that prompt text conditional so capped runs explicitly describe the prioritized subset and targeted expansion path.
files: manifest.files
Owner
Author
|
@coderabbitai review Generated by Claude Code |
|
✅ Action performedReview finished.
|
cblecker
commented
Jul 29, 2026
…erge-added paths The untracked-collision guard was leaf-only: with no tracked path at a, a merge adding a/new, and an untracked file or symlink at a, the added path itself does not exist so the guard passed — and checkout-index -f replaced a with a directory, destroying the untracked data with no rollback since the checkout succeeded. Walk each added path's ancestors before mutation and refuse on any untracked non-directory inode; tracked ancestors stay allowed as legitimate file-to-directory type changes that the stale-path loop deletes first. Verified: untracked file and symlink ancestors skip pre-mutation with data intact and repo unmutated; collision-free checkout and a deep file-to-directory type change still succeed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AjwsgsMM77sFNC6GvGjz1P
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
pr-review-toolkit/skills/review-pr/review-pr.js:902
- When the synthesizer supplies only a
commentIdand it does not match any collected thread, this branch preserves that unverified ID. If a mangled ID happens to identify another valid review comment, posting can reply to the wrong thread rather than trigger the invalid-target re-preview. Drop the target and resolution state whenmatchedis null, as the unmatched-threadIdbranch already does.
commentId: matched ? (matched.commentId || undefined) : (existing.commentId || undefined),
isResolved: matched ? knownResolved(matched.isResolved) : knownResolved(existing.isResolved),
pr-review-toolkit/skills/review-pr/scripts/checkout.sh:265
- Failure to mark a modified/added protected path is silently ignored, yet that flag is what hides the intentional index/worktree mismatch. An index-lock or filesystem error therefore still produces
CHECKOUT_OKwith a dirty checkout and causes subsequent runs to fall back. Treat this as a checkout failure; becauseHEADis updated just above and the current rollback does not restore it, the update/rollback ordering also needs to be adjusted.
git update-index --skip-worktree -- "${protected_path}" 2>/dev/null || true
pr-review-toolkit/skills/review-pr/review-pr.js:1710
- This verification is never reached for a valid zero-file merge manifest because the outer branch still requires
localGitManifest.length > 0. When base advancement has already absorbed the PR changes, checkout can legitimately emitlocalGitManifest: [],mergeDiffFileCount: 0, and a matching nonzeroprDiffFileCount; the workflow instead discards that verified merge view and silently collects the PR-head manifest from MCP. Treat any supplied array, including an empty one, as a local manifest and run these count checks.
// The local manifest diffs the merge result against the current base
// tip, while GitHub's changedFiles counts the merge-base..head PR diff.
// When the base branch has advanced, the counts legitimately differ;
// prDiffFileCount (the local merge-base..head count) is the like-for-like
// verification against GitHub's number, and mergeDiffFileCount (computed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Production-hardening pass over
pr-review-toolkitfrom a design-doc-driven audit. Every change was checked against the intent indocs/PR_REVIEW_REQUIREMENTS.mdanddocs/WORKFLOW_REWRITE_PLAN.md, validated functionally, and then adversarially re-verified by independent review passes (which caught and fixed four additional defects in the first iteration of this branch, noted below).checkout.sh
read-tree+checkout-index) never deleted files, so files removed by the PR stayed in the worktree and specialists could Read/Grep code the PR deletes. Deletions now run first (handling file→dir type changes and rename sources via--no-renames), with emptied directories pruned.read-treeandupdate-refpreviously left the index at the merge tree with HEAD on the original branch — permanently blocking future runs on the "dirty worktree" check. Merge parents are now resolved before any mutation, and failures restore the original index and tracked worktree content, including removing partially-written merge additions..vscode/,.claude/,.gitmodules, …) left index ≠ worktree after a successful checkout, so every later run silently degraded to MCP fallback. Touched protected paths are now markedskip-worktree, and skip-worktree/assume-unchanged flags on protected paths survive checkout, rerun, and rollback.prDiffFileCount(merge-base..head, matches GitHub'schangedFilessemantics) andmergeDiffFileCount(guards against incomplete manifest parsing). Runs from the repo toplevel; index refreshed after checkout.review-pr.js
changedFileswhen the base has moved; the workflow now keeps the local manifest when the like-for-like counts verify, instead of discarding accurate local data for MCP fallback.threadIdwith another'scommentId, pointing endorsement replies at the wrong thread. Identity (threadId/commentId/isResolved) now always comes from a single thread, both in merging and in overlap inference.none(which routed already-covered findings into "recommended to post"); it only attaches thread identity or classifies when the synthesizer saidnone.isResolvedis honestly tri-state. The schema no longer forces the collector to fabricate resolution state; unknown stays absent rather than being coerced tofalse, and the posting preview says when resolution is unknown.reviewMeta.threadCollectionFailed+ board warning) instead of silently disabling overlap classification.reviewMeta.manifestPromptTruncation); the complete manifest is still collected, per the large-PR honesty requirements.+++ b/headers with trailing tabs (space-containing paths) and C-quoted paths now key hunks by the real path, so diff-content signals aren't silently lost; acommentssignal selects comment-analyzer for comment-only source changes.Agents
pr-review-analysis-readonlynow disallows the known GitHub write tools (add_reply_to_pull_request_comment,add_issue_comment,issue_write,push_files,create_or_update_file,merge_pull_request,resolve_review_thread) alongside shell, file mutation, and web tools — previously several of these were available and only discouraged by prose, contradicting the README and the safety requirements. An explicittools:allowlist was tried and deliberately reverted per owner review: it hardcoded one language's tooling (gopls) and specific plugin install names, locking out read-only language-server tools for every other language. The denylist keeps read-only MCP tools usable regardless of language or installed plugins; unenumerated third-party write tools remain covered by agent prose, an accepted tradeoff.Skills / docs
APPROVEevent, agent table);WORKFLOW_REWRITE_PLAN.mdmarked historical.address-pr-feedback: mainline detection collected as dynamic context at invocation with fail-closed interpretation rules (transport-level errors fall back to the cached ref; authentication and configuration errors stop), plan-mode wording fix, resolved-thread awareness in both collection paths.Validation
review-pr.jsexecuted with stubbedagent()/parallel()— verified manifest keep/fallback decisions, thread-failure visibility, overlap preservation and identity consistency, fallback board, prompt capping, comment-signal selection, TAB/C-quoted header handling, tri-stateisResolved, incomplete-parse fallback.asNumber(null, -1) === 0sentinel bug, and theisResolvedcoercion — are fixed in this branch.claude plugin validate✓ (marketplace + plugin), markdownlint ✓ (0 issues), skillsaw --strict ✓ (A+).🤖 Generated with Claude Code
https://claude.ai/code/session_01AjwsgsMM77sFNC6GvGjz1P
Generated by Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Summary by CodeRabbit
New Features
Bug Fixes
Documentation