feat(pr-review-toolkit): production-harden checkout, overlap identity, and manifest verification#83
Open
cblecker wants to merge 9 commits into
Open
feat(pr-review-toolkit): production-harden checkout, overlap identity, and manifest verification#83cblecker wants to merge 9 commits into
cblecker wants to merge 9 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 (4)
🚧 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
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.
Comment on lines
+36
to
+42
| normalized_origin="$(printf '%s' "${origin_url}" \ | ||
| | sed -e 's|/*$||' -e 's|\.git$||' -e 's|:|/|g' \ | ||
| | awk -F/ 'NF>=2 {print tolower($(NF-1))"/"tolower($NF)}')" | ||
| expected_repo="$(printf '%s/%s' "${pr_owner}" "${pr_repo}" | tr '[:upper:]' '[:lower:]')" | ||
| if [[ "${normalized_origin}" != "${expected_repo}" ]]; then | ||
| skip "origin does not match PR base repository" | ||
| fi |
Comment on lines
+99
to
+100
| while IFS= read -r -d '' added_path; do | ||
| rm -f -- "${added_path}" 2>/dev/null || true |
Comment on lines
+985
to
+1007
| function unquoteGitPath(value) { | ||
| var escapes = { t: '\t', n: '\n', r: '\r', '"': '"', '\\': '\\', a: '\x07', b: '\b', f: '\f', v: '\v' } | ||
| var out = '' | ||
| for (var i = 0; i < value.length; i++) { | ||
| var ch = value.charAt(i) | ||
| if (ch !== '\\') { | ||
| out += ch | ||
| continue | ||
| } | ||
| var next = value.charAt(i + 1) | ||
| if (next >= '0' && next <= '7') { | ||
| var oct = '' | ||
| while (oct.length < 3 && value.charAt(i + 1) >= '0' && value.charAt(i + 1) <= '7') { | ||
| oct += value.charAt(++i) | ||
| } | ||
| out += String.fromCharCode(parseInt(oct, 8)) | ||
| } else { | ||
| out += escapes[next] != null ? escapes[next] : next | ||
| i++ | ||
| } | ||
| } | ||
| return out | ||
| } |
Comment on lines
+845
to
+847
| if (!matched) { | ||
| const best = bestThreadMatch(item, threads) | ||
| matched = best ? best.thread : null |
… 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
Comment on lines
+126
to
+129
| while IFS= read -r -d '' added_path; do | ||
| if [[ -e "${added_path}" || -L "${added_path}" ]]; then | ||
| skip "untracked files would be overwritten by the merge checkout" | ||
| fi |
| @@ -1477,15 +1616,30 @@ if (localGitManifest && localGitManifest.length > 0) { | |||
| manifestSource = 'local-git' | |||
|
|
|||
| if (pr.changedFiles && files.length !== pr.changedFiles) { | |||
Comment on lines
+22
to
+26
| - Get the mainline branch: run | ||
| `git symbolic-ref --short refs/remotes/origin/HEAD 2>/dev/null` and strip | ||
| the `origin/` prefix. If that fails (origin/HEAD not set locally), fall | ||
| back to | ||
| `git ls-remote --symref origin HEAD 2>/dev/null | grep "^ref:" | awk '{print $2}' | sed 's|refs/heads/||'` |
| const threadData = await threadCollectionPromise | ||
|
|
||
| const threads = (threadData && Array.isArray(threadData.threads)) ? threadData.threads : [] | ||
| const threadCollectionFailed = !(threadData && Array.isArray(threadData.threads)) |
Comment on lines
+78
to
+79
| - `isResolved` — thread resolution state when the API response exposes it; | ||
| null when unknown (do not guess) |
| 'concurrency': /\b(mutex|Mutex|chan\s|go\s+func|async\s|await\s|WaitGroup|Semaphore|threading|concurrent)\b|Lock\(\)|RLock\(\)/, | ||
| 'public-api': /\b(export\s|pub\s+fn|public\s+func|module\.exports)\b/ | ||
| 'public-api': /\b(export\s|pub\s+fn|public\s+func|module\.exports)\b/, | ||
| 'comments': /(^|\n)\s*(\/\/|#\s|\/\*|\*\s|<!--|"""|''')/ |
- 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
| # stale-path loop below, and untracked leftovers make checkout-index fail | ||
| # into the rollback path instead of losing data. | ||
| while IFS= read -r -d '' added_path; do | ||
| if [[ -f "${added_path}" || -L "${added_path}" ]]; then |
Comment on lines
+117
to
+118
| git read-tree "${orig_head}" 2>/dev/null || return 0 | ||
| checkout_worktree 2>/dev/null || true |
| const order = { none: 0, overlaps: 1, already_covered: 2 } | ||
| const leftStatus = (left && left.status) || 'none' | ||
| const rightStatus = (right && right.status) || 'none' | ||
| const selected = (order[rightStatus] > order[leftStatus]) ? right : left |
| 'concurrency': /\b(mutex|Mutex|chan\s|go\s+func|async\s|await\s|WaitGroup|Semaphore|threading|concurrent)\b|Lock\(\)|RLock\(\)/, | ||
| 'public-api': /\b(export\s|pub\s+fn|public\s+func|module\.exports)\b/ | ||
| 'public-api': /\b(export\s|pub\s+fn|public\s+func|module\.exports)\b/, | ||
| 'comments': /(^|\n)\s*(\/\/|#(?!!|include\b|define\b|undef\b|ifdef\b|ifndef\b|if\b|elif\b|else\b|endif\b|pragma\b|error\b|line\b)|\/\*|\*\s|<!--|"""|''')/ |
…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),
Comment on lines
+1642
to
+1646
| const prDiffFileCount = asNumber(configSources ? configSources.prDiffFileCount : undefined, -1) | ||
| const mergeDiffFileCount = asNumber(configSources ? configSources.mergeDiffFileCount : undefined, -1) | ||
| let manifestProblem = '' | ||
| if (mergeDiffFileCount >= 0 && files.length !== mergeDiffFileCount) { | ||
| manifestProblem = 'local manifest parse incomplete: merge diff has ' + mergeDiffFileCount + ' file(s), parsed ' + files.length |
…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
Comment on lines
+187
to
+188
| done < <(git diff --name-only -z --no-renames --diff-filter=AM \ | ||
| "${orig_head}" "${merge_sha}" -- "${protected_match[@]}" 2>/dev/null) |
Owner
Author
There was a problem hiding this comment.
The observation is correct, but falling back to MCP would be disproportionate: the sandbox deny list makes unlinking these paths impossible by design, so a leftover untracked file is the best achievable worktree state — and it's the same documented tradeoff already accepted for modified protected paths (which keep pre-merge content). Discarding the entire verified local diff because a PR deletes .vscode/settings.json would hurt review quality far more than the leftover hurts it. Preflight and reruns are unaffected (diff-index ignores untracked files, verified in the harness), and the merge diff/manifest still carry the real deletion. ab38275 documents the deletion case explicitly in the README instead.
Generated by Claude Code
Comment on lines
+865
to
+868
| if (idsSupplied) { | ||
| matched = (threads || []).find(t => | ||
| t && ((existing.threadId && t.id === existing.threadId) || (existing.commentId && t.commentId === existing.commentId)) | ||
| ) || null |
Comment on lines
+286
to
+289
| - if there is no `commentId`, do not silently fall through to a new comment: | ||
| show `⚠ No reply target available — posting would create a new line comment | ||
| that may duplicate the existing thread.` and let the user choose line | ||
| comment, review body, or skip |
…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
Comment on lines
+185
to
+188
| while IFS= read -r -d '' protected_path; do | ||
| git update-index --skip-worktree -- "${protected_path}" 2>/dev/null || true | ||
| done < <(git diff --name-only -z --no-renames --diff-filter=AM \ | ||
| "${orig_head}" "${merge_sha}" -- "${protected_match[@]}" 2>/dev/null) |
…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),
Comment on lines
+1445
to
+1448
| omittedSummary: { | ||
| omittedFileCount: omitted.length, | ||
| byCategory: byCategory, | ||
| note: 'The complete manifest was collected; this prompt lists the ' + PROMPT_FILE_CAP + ' highest-signal files and summarizes the rest by category above. Omitted files are still part of the PR and can be inspected directly (Read/Grep on the merged checkout, or the focused patch access instructions).' |
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/address-pr-feedback/SKILL.md`:
- Around line 24-30: Update the mainline-branch lookup instructions around git
ls-remote and the cached origin/HEAD fallback to capture and inspect the remote
command’s exit status. Permit the cached fallback only for confirmed
connectivity/offline failures; treat authentication, invalid-remote, and other
lookup errors as fatal, display the existing determination error, and stop
without proceeding with an unresolved branch.
🪄 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: e09622fe-2112-4199-81b5-ff3fc495b5ee
📒 Files selected for processing (5)
pr-review-toolkit/README.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
🚧 Files skipped from review as they are similar to previous changes (2)
- pr-review-toolkit/README.md
- pr-review-toolkit/skills/review-pr/scripts/checkout.sh
…d reply targets, index omitted files Addresses CodeRabbit's first full review and Copilot's seventh round: - checkout.sh: the script only owns skip-worktree bits on sandbox-protected paths. Bits elsewhere (manual skip-worktree, sparse checkout) can hide local modifications the clean-worktree preflight cannot see, and checkout-index -f would overwrite them while reapplying the bit hid the damage — such worktrees are now refused before any mutation - review-pr.js: when the synthesizer's authoritative threadId cannot be resolved, the commentId and resolution state are dropped rather than kept — posting uses commentId, so a mangled pair could still reply to a wrong-but-valid thread; the preview now asks for a posting choice instead - review-pr.js: the truncated-manifest summary carries a compact per-directory expansion index (count, categories, get_files page range) so omitted large-PR areas are actionable on follow-up per the design docs' large-PR requirements, without reinflating specialist prompts - address-pr-feedback: mainline detection fails closed — the cached origin/HEAD fallback is permitted only for connectivity failures; authentication, invalid-remote, and configuration errors stop execution instead of letting a stale cached branch pass the safety check Validated: checkout harness adds a user-owned skip-worktree refusal scenario (hidden local edit and bit survive untouched) plus three-run protected-path and collision regressions; workflow simulation adds a ghost-threadId scenario and byDirectory index assertions (19 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 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: {} }
Comment on lines
+1092
to
+1095
| if (awaitingHeader) { | ||
| var headerMatch = /^\+\+\+ (?:"b\/(.+)"|b\/(.+?))\t?$/.exec(line) | ||
| if (headerMatch) { | ||
| currentFile = headerMatch[1] != null ? unquoteGitPath(headerMatch[1]) : headerMatch[2] |
Comment on lines
+181
to
+182
| while IFS= read -r -d '' entry; do | ||
| if [[ "${entry}" != "S "* ]]; then |
…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.
|
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.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 uses a tools allowlist (Read/Grep/Glob, PR reads, gopls read tools). GitHub write tools (add_reply_to_pull_request_comment,issue_write,push_files, …) were previously available and only discouraged by prose, contradicting the README and the safety requirements.Skills / docs
APPROVEevent, agent table);WORKFLOW_REWRITE_PLAN.mdmarked historical.address-pr-feedback: simpler mainline detection with fallback, 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