From da5d88221d3aecd77416617a551105547cbbbf4a Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 25 Jul 2026 03:11:41 +0000 Subject: [PATCH 1/9] feat(pr-review-toolkit): production-harden checkout, overlap identity, 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 Claude-Session: https://claude.ai/code/session_01AjwsgsMM77sFNC6GvGjz1P --- pr-review-toolkit/.claude-plugin/plugin.json | 2 +- pr-review-toolkit/README.md | 98 +++++--- .../agents/pr-review-analysis-readonly.md | 22 +- .../docs/WORKFLOW_REWRITE_PLAN.md | 6 + .../skills/address-pr-feedback/SKILL.md | 21 +- pr-review-toolkit/skills/review-pr/SKILL.md | 44 +++- .../skills/review-pr/review-pr.js | 225 ++++++++++++++---- .../skills/review-pr/scripts/checkout.sh | 161 ++++++++++--- 8 files changed, 449 insertions(+), 130 deletions(-) diff --git a/pr-review-toolkit/.claude-plugin/plugin.json b/pr-review-toolkit/.claude-plugin/plugin.json index a00dd92..01ec9f8 100644 --- a/pr-review-toolkit/.claude-plugin/plugin.json +++ b/pr-review-toolkit/.claude-plugin/plugin.json @@ -1,6 +1,6 @@ { "name": "pr-review-toolkit", - "version": "1.12.1", + "version": "1.13.0", "description": "Comprehensive PR review board using shared workflow context", "author": { "name": "cblecker", diff --git a/pr-review-toolkit/README.md b/pr-review-toolkit/README.md index 634f667..9fb6bb4 100644 --- a/pr-review-toolkit/README.md +++ b/pr-review-toolkit/README.md @@ -45,34 +45,49 @@ falls back to GitHub MCP-based file collection. ### Local Git Diff Provider (Preferred) -When the current directory is a git repository with a clean worktree and -`origin` matches the PR base repository, the skill: - -1. Fetches GitHub's `refs/pull/N/merge` synthetic merge ref from `origin` -2. Checks out the merge result as a detached HEAD -3. Verifies the merge commit's second parent (`HEAD^2`) matches the PR's - `headSha` from GitHub metadata -4. Builds a compact file manifest from `git diff --name-status` and - `git diff --numstat` (NUL-delimited for safe parsing of special characters) -5. Optionally collects the full merge diff when it fits within a 200K character - cap +The bundled `checkout.sh` script runs during skill preprocessing. When the +current directory is a git repository with a clean worktree, it: + +1. Verifies `origin` matches the PR base repository (parsed from the PR URL) + before fetching anything +2. Fetches GitHub's `refs/pull/N/merge` synthetic merge ref from `origin` +3. Checks out the merge result as a detached HEAD using plumbing commands + (`read-tree`, `checkout-index`, `update-ref`) so sandbox-protected files + (shell profiles, `.mcp.json`, `.claude/`, editor config) are skipped + instead of failing the checkout; files deleted by the merge are removed + from the worktree, and a mid-checkout failure restores the original index + and tracked worktree content (including removing files the partial + checkout added) before falling back to MCP +4. Emits a compact file manifest from `git diff --name-status` and + `git diff --numstat` (paths with special characters appear C-quoted, as in + normal git output, and are decoded during parsing), plus two verification + counts: `prDiffFileCount` — the merge-base..head file count cross-checked + against GitHub's `changedFiles` when the base branch has advanced — and + `mergeDiffFileCount`, which the workflow compares against the parsed + manifest length before trusting it (both recorded as `-1` in + `reviewMeta.sources` when unavailable) +5. The skill then verifies the merge commit's second parent (`HEAD^2`) + matches the PR's `headSha` from GitHub metadata, and optionally collects + the full merge diff when it fits within a 200K character cap The local manifest and optional full diff are passed to the workflow via `args`. Specialist agents can also use Read and Grep on the merged checkout to inspect -files in their merged state. +files in their merged state. Sandbox-protected files listed above are the one +exception: their worktree content stays at the pre-merge state. -The skill does NOT auto-restore the original git ref after checkout. The merged -checkout state keeps the Read tool useful for workflow subagents. Running in a -dedicated worktree is recommended. +After a successful checkout the skill does NOT auto-restore the original git +ref. The merged checkout state keeps the Read tool useful for workflow +subagents. Running in a dedicated worktree is recommended. ### MCP Fallback When local git is unavailable, the workflow collects changed files via GitHub MCP `get_files` with pagination and recovery retries, as in previous versions. -Fallback reasons are recorded in `reviewMeta.sources.fallbackReason` and include: -not a git repository, dirty worktree, origin mismatch, merge ref fetch failure, -or merge parent mismatch. +Fallback reasons are recorded in `reviewMeta.sources.fallbackReason` and +include: not a git repository, dirty worktree, missing origin, origin +mismatch, merge ref fetch failure, checkout failure (after restoring the +original state), or merge parent mismatch. ### Workflow @@ -117,7 +132,7 @@ repository files and use available read-only MCP tools to verify findings. | code-reviewer | Always | Reviews code for bugs, style, and guideline adherence (runs on Opus) | | silent-failure-hunter | Changes touch error handling, try/catch, or fallback logic | Identifies silent failures and inadequate error handling | | pr-test-analyzer | Functional code that should have corresponding tests | Analyzes test coverage completeness | -| comment-analyzer | Changes add or modify comments, docstrings, or docs | Checks comment accuracy and maintainability | +| comment-analyzer | Changes touch docs files, or — when the local full diff is available — add or modify comments or docstrings | Checks comment accuracy and maintainability | | type-design-analyzer | Changes introduce or modify type definitions in typed languages | Evaluates type design and invariant quality | | security-reviewer | Changes touch auth, crypto, tokens, credentials, or security-related code | Reviews for security vulnerabilities and unsafe patterns | | api-compat-reviewer | Changes touch public APIs, exports, or client-facing interfaces | Checks API compatibility and breaking changes | @@ -141,7 +156,14 @@ The workflow returns a review board grouped by outcome: Each finding preserves the specialist's claim, evidence, reasoning, suggested fix, confidence, source lens, and existing-review overlap rationale. The board -also includes positive observations, PR metadata, and review metadata. +also includes positive observations, PR metadata, and review metadata. Thread +resolution state (`isResolved`) is recorded only when the GitHub read tools +expose it. If review-thread collection fails, the board says so +(`reviewMeta.threadCollectionFailed`) instead of silently skipping overlap +classification. For very large PRs, specialist prompts carry the +highest-signal subset of the manifest with an explicit per-category summary of +the rest (`reviewMeta.manifestPromptTruncation`); the complete manifest is +always collected. ## Interaction And Posting @@ -151,26 +173,23 @@ show already-covered findings, or cancel. Drafts are plain conversation text until the user approves a preview. The skill previews each line comment, review-body text, and the proposed review event -(`COMMENT` or `REQUEST_CHANGES`) before any GitHub write tool is used. +(`COMMENT`, `REQUEST_CHANGES`, or `APPROVE`) before any GitHub write tool is +used. ## Permissions ### Local Git Commands -The skill's `allowed-tools` frontmatter intentionally uses a small set of git -command patterns for preflight and diff collection: +Preflight, fetch, and checkout run inside the bundled, reviewable +`scripts/checkout.sh` helper during skill preprocessing — they are not +model-issued Bash calls. The skill's `allowed-tools` frontmatter then permits +only one git command pattern: -- `git fetch origin refs/pull/*/merge` — fetch merge ref -- `git checkout --detach FETCH_HEAD` — checkout merge result -- `git rev-parse *` — record merge parents and related refs -- `git diff *` — verify clean state and collect status, numstat, and full diff +- `git diff *` — collect the full merge diff and translate merge-result line + numbers to PR HEAD line numbers before posting -The repository root, worktree state, and origin URL checks are injected into the -skill prompt during preprocessing. The skill instructions still constrain actual -Bash use to the documented preflight and diff commands; workflow-spawned agents -have no Bash access. - -All other Bash commands are denied. +All other Bash commands are denied; workflow-spawned agents have no Bash +access. ### GitHub MCP Permissions @@ -191,10 +210,13 @@ type. That agent allows GitHub PR reads and disallows shell, local file, web, an file mutation tools so large MCP responses do not lead to generated Python, `jq`, `gh`, or other ad-hoc parsing scripts. -Specialist reviewers run through `pr-review-analysis-readonly`, which blocks -shell and mutation tools while allowing read-only repository inspection and -read-only MCP tools. In the local git path, specialists can use Read and Grep +Specialist reviewers run through `pr-review-analysis-readonly`, which uses a +tool allowlist (Read, Grep, Glob, GitHub PR reads, and gopls read-only tools) +so shell, file mutation, and GitHub write tools are unavailable rather than +merely discouraged. In the local git path, specialists can use Read and Grep on the merged checkout to inspect changed files and trace cross-file effects. +To let specialists use additional read-only MCP tools (e.g. another language +server), extend that agent's allowlist. Approved posting, if the user chooses to post, requires these write capabilities: @@ -202,6 +224,10 @@ capabilities: - `pull_request_review_write` to create and submit a review - `add_comment_to_pending_review` to add approved line comments to a pending review +- `add_reply_to_pull_request_comment` to post approved endorsements as + replies on existing review threads +- `add_issue_comment` (address-pr-feedback only) to reply to review-body and + conversation comments Write tools are used only after the skill has shown the exact preview and the user has explicitly approved posting it. diff --git a/pr-review-toolkit/agents/pr-review-analysis-readonly.md b/pr-review-toolkit/agents/pr-review-analysis-readonly.md index c6884c4..9cd0840 100644 --- a/pr-review-toolkit/agents/pr-review-analysis-readonly.md +++ b/pr-review-toolkit/agents/pr-review-analysis-readonly.md @@ -1,6 +1,18 @@ --- name: pr-review-analysis-readonly description: Read-only PR analysis agent for pr-review-toolkit specialist reviews. +tools: + - Read + - Grep + - Glob + - mcp__plugin_github_github__pull_request_read + - mcp__plugin_golang_gopls__go_diagnostics + - mcp__plugin_golang_gopls__go_file_context + - mcp__plugin_golang_gopls__go_package_api + - mcp__plugin_golang_gopls__go_search + - mcp__plugin_golang_gopls__go_symbol_references + - mcp__plugin_golang_gopls__go_vulncheck + - mcp__plugin_golang_gopls__go_workspace disallowedTools: - Bash - Write @@ -12,14 +24,16 @@ disallowedTools: - WebSearch - mcp__plugin_github_github__pull_request_review_write - mcp__plugin_github_github__add_comment_to_pending_review + - mcp__plugin_github_github__add_reply_to_pull_request_comment --- Analyze the PR using read-only context only. -You may inspect repository files and use available read-only MCP tools when they -help verify a finding, including language-server tools if available. Do not run -shell commands, Python, jq, gh, or generated scripts. Do not modify files, draft -reviews, post comments, submit reviews, or call GitHub write tools. +You may inspect repository files with Read, Grep, and Glob, fetch PR data with +GitHub PR read tools, and use the listed language-server tools when available +to verify a finding. Do not run shell commands, Python, jq, gh, or generated +scripts. Do not modify files, draft reviews, post comments, submit reviews, or +call GitHub write tools. If a GitHub MCP response is too large, truncated, or saved to a local file by the runtime, do not inspect the saved file with local tools. Use repository reads diff --git a/pr-review-toolkit/docs/WORKFLOW_REWRITE_PLAN.md b/pr-review-toolkit/docs/WORKFLOW_REWRITE_PLAN.md index d095e0e..ac8d5ee 100644 --- a/pr-review-toolkit/docs/WORKFLOW_REWRITE_PLAN.md +++ b/pr-review-toolkit/docs/WORKFLOW_REWRITE_PLAN.md @@ -1,5 +1,11 @@ # PR Review Toolkit Workflow Rewrite Plan +> **Historical design document.** This plan guided the original rewrite and is +> kept for design rationale. Where it differs from the implementation (for +> example, the plumbing-based `scripts/checkout.sh` replacing +> `git checkout --detach`, the narrower `allowed-tools` list, and non-NUL +> manifest output), the implementation and `README.md` are authoritative. + This plan describes how to implement the requirements in `docs/PR_REVIEW_REQUIREMENTS.md` using a Workflow-backed analysis engine and a skill-driven interactive review loop. diff --git a/pr-review-toolkit/skills/address-pr-feedback/SKILL.md b/pr-review-toolkit/skills/address-pr-feedback/SKILL.md index 55ef052..9299ef1 100644 --- a/pr-review-toolkit/skills/address-pr-feedback/SKILL.md +++ b/pr-review-toolkit/skills/address-pr-feedback/SKILL.md @@ -19,15 +19,19 @@ review feedback, then post reply comments to GitHub. **Validate branch first:** - Run `git branch --show-current` to get the current branch -- Run `git ls-remote --symref origin HEAD 2>/dev/null | grep "^ref:" | awk '{print $2}' | sed 's|refs/heads/||'` to get the mainline branch +- 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/||'` - If the current branch matches the mainline branch: - Display: "Error: You're on the mainline branch. Please checkout a feature branch and retry this skill." - Stop execution **Then ensure plan mode is active:** -- If plan mode is not already active, call `EnterPlanMode` and wait for user - approval +- If plan mode is not already active, call `EnterPlanMode` to switch into + planning before collecting and analyzing feedback - If plan mode is already active (check for "Plan mode is active" in system context), proceed directly @@ -71,6 +75,8 @@ Check if the argument contains `--interactive`. If it does, use the - `commentId` — numeric comment ID (for reply posting) - `threadId` — thread node ID (inline comments) or null - `type` — `"review"` | `"inline"` | `"conversation"` + - `isResolved` — thread resolution state when the API response exposes it; + null when unknown (do not guess) ### Interactive Collection Path (`--interactive`) @@ -84,8 +90,9 @@ Loop until user says "done": - Extract feedback text - Extract author (if available) - Extract file/line location (if available) - - Normalize into structured format with `commentId`, `threadId`, and `type` - fields (set to null if not available from pasted content) + - Normalize into structured format with `commentId`, `threadId`, `type`, + and `isResolved` fields (set to null if not available from pasted + content or the fetched comment data) 3. Acknowledge: "Recorded item [N]: [brief summary]" 4. Continue loop @@ -233,7 +240,9 @@ item that has a draft reply. For each item: -1. Present the draft reply text (generated in Phase 2, refined in Phase 4) +1. Present the draft reply text (generated in Phase 2, refined in Phase 4). + If the item's thread is known to be resolved (`isResolved` is true), note + it: a reply will stay collapsed and the reviewer may not see it. 2. Use `AskUserQuestion`: ```text diff --git a/pr-review-toolkit/skills/review-pr/SKILL.md b/pr-review-toolkit/skills/review-pr/SKILL.md index 607f206..ed9af2d 100644 --- a/pr-review-toolkit/skills/review-pr/SKILL.md +++ b/pr-review-toolkit/skills/review-pr/SKILL.md @@ -27,7 +27,6 @@ allowed-tools: ## Git Environment -- Origin URL: !`git remote get-url origin 2>/dev/null || echo __NO_ORIGIN_REMOTE__` - Checkout: !`bash "${CLAUDE_SKILL_DIR}/scripts/checkout.sh" "$pr-url"` ## Constraints @@ -61,7 +60,6 @@ Call `pull_request_read` with method `get`. Extract and record: - `headSha`: the current head commit SHA of the PR - `changedFiles`: the number of changed files -- base repository clone URL (typically `https://github.com/{owner}/{repo}`) ## Local Git Preflight @@ -70,22 +68,28 @@ Read the Checkout output from Git Environment above. `CHECKOUT_SKIP:` → record the reason as `fallbackReason`, skip to workflow launch without `localGitManifest` or `fullDiff`. -`CHECKOUT_OK` → parse `mergeCommit`, `baseSha`, `headSha` from the subsequent -`key value` lines. Then: +`CHECKOUT_OK` → parse `mergeCommit`, `baseSha`, `headSha`, `mergeDiffFileCount`, +and (when present) `prDiffFileCount` from the subsequent `key value` lines. The +checkout script has already verified that `origin` matches the PR base +repository. Then: -1. Verify `headSha` matches PR metadata `headSha`. Mismatch → record "HEAD^2 - does not match PR headSha". Do not trust local diff data on mismatch. -2. Normalize origin URL (from Git Environment) and PR base URL to - `{owner}/{repo}` (strip protocol, `.git` suffix; case-insensitive). - Mismatch → "origin does not match PR base repository". +Verify `headSha` matches PR metadata `headSha`. Mismatch → record "HEAD^2 +does not match PR headSha" as `fallbackReason` and do not trust local diff +data: skip to workflow launch without `localGitManifest` or `fullDiff`. ## Build Local Git Manifest Parse the `NAME_STATUS` and `NUMSTAT` sections from the checkout output. -From `NAME_STATUS`, parse each line into `{path, status}`. Map `A`, `M`, `D`, -`R*`, `C*` to `added`, `modified`, `deleted`, `renamed`, `copied`. For renames -and copies, use the destination path. +From `NAME_STATUS`, parse each line into `{path, status}`. Fields are +tab-separated. Map `A`, `M`, `D`, `R*`, `C*` to `added`, `modified`, +`deleted`, `renamed`, `copied`. For renames and copies, use the destination +path. + +Paths containing special characters appear C-quoted (wrapped in double +quotes, with `\t`, `\n`, `\"`, `\\`, and octal escapes). Decode them to the +real path — strip the quotes and unescape — before storing, so manifest paths +match what Read, Grep, and comment posting need. From `NUMSTAT`, parse additions and deletions per file and merge with the status list. For renames/copies use destination path as key. Binary files show `-` for @@ -112,6 +116,8 @@ Invoke the Workflow tool with: - `args`: `owner`, `repo`, `pullNumber`, `localGitManifest` (omit if preflight failed), `fullDiff` (omit if not collected), and `sources`: - `mergeCommit`, `baseSha`, `headSha` (empty strings if preflight failed) + - `prDiffFileCount` and `mergeDiffFileCount`: the parsed numbers from + checkout output (omit when absent) - `fullDiffIncluded`: whether fullDiff is included - `fallbackReason`: reason preflight failed (or empty string) @@ -137,6 +143,14 @@ Reviewers: code-reviewer, pr-test-analyzer, silent-failure-hunter. The reviewer list comes from `reviewMeta.selectedReviewers` which stores full agent names. +If `reviewMeta.threadCollectionFailed` is true, add a warning line: existing +review threads could not be collected, so overlap classification is +unavailable and recommended findings may duplicate existing comments. + +If `reviewMeta.manifestPromptTruncation` is set, add a note: specialist +prompts listed only the highest-signal files; the omitted count and +categories come from that field. The complete manifest was still collected. + ### 2. Recommended to post (full detail) For each finding, include: @@ -277,9 +291,13 @@ For each finding being posted as a new line comment, show: For each overlap finding being posted as a thread reply, show: - finding id, "Reply to thread on path:line", and body -- if the target thread is resolved, append a warning: +- if the target thread is known to be resolved (`isResolved` is true), append + a warning: `⚠ Target thread is resolved — reply will stay collapsed and the PR author may not see it.` +- if `isResolved` is absent (the GitHub read tools did not expose resolution + state), append: `(thread resolution state unknown — if the thread is + resolved, this reply will stay collapsed)` For review body text (non-line findings), show the review body. diff --git a/pr-review-toolkit/skills/review-pr/review-pr.js b/pr-review-toolkit/skills/review-pr/review-pr.js index 3e866cd..ca8fc7c 100644 --- a/pr-review-toolkit/skills/review-pr/review-pr.js +++ b/pr-review-toolkit/skills/review-pr/review-pr.js @@ -149,7 +149,7 @@ const THREAD_SCHEMA = { } } }, - required: ['id', 'path', 'author', 'body', 'isResolved'] + required: ['id', 'path', 'author', 'body'] } } }, @@ -765,6 +765,13 @@ function bestSeverity(left, right) { return (SEVERITY_ORDER[left] ?? 3) <= (SEVERITY_ORDER[right] ?? 3) ? left : right } +// Resolution state is tri-state: true, false, or undefined when the GitHub +// read tools did not expose it. Coercing unknown to false would hide the +// uncertainty from the posting preview. +function knownResolved(value) { + return typeof value === 'boolean' ? value : undefined +} + function bestOverlap(left, right) { const order = { none: 0, overlaps: 1, already_covered: 2 } const leftStatus = (left && left.status) || 'none' @@ -772,11 +779,14 @@ function bestOverlap(left, right) { const selected = (order[rightStatus] > order[leftStatus]) ? right : left if (!selected) return { status: 'none', threadId: '', rationale: '' } + // threadId, commentId, and isResolved must all come from the selected + // overlap: mixing ids across merged findings can point replies at the + // wrong thread. return { status: selected.status || 'none', threadId: selected.threadId || '', - commentId: selected.commentId || (right && right.commentId) || (left && left.commentId) || undefined, - isResolved: selected.isResolved || false, + commentId: selected.commentId || undefined, + isResolved: knownResolved(selected.isResolved), rationale: combineText(left && left.rationale, right && right.rationale) } } @@ -798,20 +808,7 @@ function mergeBoardItem(base, next) { } } -function inferThreadOverlap(item, threads) { - const existing = item.existingReviewOverlap || {} - if (existing.status && existing.status !== 'none') { - if (existing.threadId || existing.commentId) { - const matched = (threads || []).find(t => - t && (t.id === existing.threadId || (existing.commentId && t.commentId === existing.commentId)) - ) - return Object.assign({}, existing, { - commentId: existing.commentId || (matched && matched.commentId) || undefined, - isResolved: matched ? matched.isResolved || false : false - }) - } - } - +function bestThreadMatch(item, threads) { const location = item.location || {} const itemText = combinedItemText(item) let best = null @@ -828,13 +825,50 @@ function inferThreadOverlap(item, threads) { best = { thread: thread, overlap: overlap, sameLine: sameLine, nearLine: nearLine, score: score } } }) + return best +} + +function inferThreadOverlap(item, threads) { + const existing = item.existingReviewOverlap || {} + const location = item.location || {} + + // The synthesizer classifies overlap by logical concern with full thread + // bodies in context; keep its non-none status and only attach thread + // identity here. Token matching is a fallback classifier, not an override. + if (existing.status && existing.status !== 'none') { + let matched = null + if (existing.threadId || existing.commentId) { + matched = (threads || []).find(t => + t && ((existing.threadId && t.id === existing.threadId) || (existing.commentId && t.commentId === existing.commentId)) + ) || null + } + if (!matched) { + const best = bestThreadMatch(item, threads) + matched = best ? best.thread : null + } + // When a thread is matched, take the whole identity triple from it so + // threadId, commentId, and isResolved always describe one thread; a + // synthesizer-provided id that resolved to nothing must not be paired + // with a different thread's commentId. + return { + status: existing.status, + threadId: matched ? (matched.id || '') : (existing.threadId || ''), + commentId: matched ? (matched.commentId || undefined) : (existing.commentId || undefined), + isResolved: matched ? knownResolved(matched.isResolved) : knownResolved(existing.isResolved), + rationale: existing.rationale + || (matched + ? 'Overlaps an existing review thread on ' + (matched.path || location.path || 'PR') + (matched.line != null ? ':' + matched.line : '') + '.' + : 'Overlap classified during review synthesis.') + } + } + const best = bestThreadMatch(item, threads) if (!best) { return { status: 'none', - threadId: existing.threadId || '', - commentId: existing.commentId || undefined, - isResolved: false, + threadId: '', + commentId: undefined, + isResolved: undefined, rationale: existing.rationale || 'No existing review overlap was classified.' } } @@ -845,7 +879,7 @@ function inferThreadOverlap(item, threads) { status: status, threadId: best.thread.id || '', commentId: best.thread.commentId || undefined, - isResolved: best.thread.isResolved || false, + isResolved: knownResolved(best.thread.isResolved), rationale: 'Inferred overlap with an existing review thread on ' + location.path + (best.thread.line != null ? ':' + best.thread.line : '') + '.' } } @@ -948,17 +982,57 @@ function signalsForFile(file) { return uniq(signals) } +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 +} + function enrichSignalsFromDiff(files, fullDiff) { if (!fullDiff) return files var hunks = {} var currentFile = null + var awaitingHeader = false fullDiff.split('\n').forEach(function(line) { - var diffMatch = /^diff --git a\/.+ b\/(.+)$/.exec(line) - if (diffMatch) { - currentFile = diffMatch[1] - if (!hunks[currentFile]) hunks[currentFile] = [] + // Track the current file from the `+++ b/` header directly after each + // `diff --git` line: the header carries one unambiguous path where the + // `diff --git` line has two. Space-containing paths get a trailing TAB + // and special characters arrive C-quoted; both are normalized so hunk + // keys equal manifest paths. `+++ /dev/null` (deletions) never matches, + // and added content lines that look like headers are ignored because a + // header is only accepted while one is expected. + if (line.indexOf('diff --git ') === 0) { + currentFile = null + awaitingHeader = true return } + if (awaitingHeader) { + var headerMatch = /^\+\+\+ (?:"b\/(.+)"|b\/(.+?))\t?$/.exec(line) + if (headerMatch) { + currentFile = headerMatch[1] != null ? unquoteGitPath(headerMatch[1]) : headerMatch[2] + awaitingHeader = false + if (!hunks[currentFile]) hunks[currentFile] = [] + return + } + } if (currentFile && (line.charAt(0) === '+' && line.charAt(1) !== '+')) { hunks[currentFile].push(line.substring(1)) } @@ -969,7 +1043,8 @@ function enrichSignalsFromDiff(files, fullDiff) { 'types': /\b(interface|struct|class\s|enum\s)\b|@dataclass/, 'security': /\b(auth|password|token|secret|credential|jwt|bcrypt|hash|encrypt|decrypt|certificate)\b/i, '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|