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..cd55bde 100644 --- a/pr-review-toolkit/README.md +++ b/pr-review-toolkit/README.md @@ -45,34 +45,55 @@ 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 — modified or +added protected paths are marked `skip-worktree`, and a protected path the PR +deletes remains on disk as an untracked leftover (the sandbox blocks +unlinking it). Preflight and reruns are unaffected; the full merge diff and +manifest still carry the real change. -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, non-github.com +origin host, origin mismatch, merge ref fetch failure, untracked files that +the merge would overwrite, checkout failure (after restoring the original +state — the reason says so explicitly if the restore itself was incomplete), +missing or failed manifest verification counts, or merge parent mismatch. ### Workflow @@ -117,7 +138,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 +162,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 +179,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 +216,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 +230,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..30da769 100644 --- a/pr-review-toolkit/skills/address-pr-feedback/SKILL.md +++ b/pr-review-toolkit/skills/address-pr-feedback/SKILL.md @@ -19,15 +19,30 @@ 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 from the remote (authoritative — the cached local + `origin/HEAD` can be stale after a default-branch rename). Run + `git ls-remote --symref origin HEAD` and check its exit status directly + (do not discard stderr; a pipeline's status would be the last command's, + not git's): + - Exit 0 → the mainline is the `ref: refs/heads/` line's branch + name. If no such line exists, display "Error: Could not determine the + mainline branch." and stop execution. + - Non-zero with a connectivity error (e.g. "Could not resolve host", + "unable to access") → fall back to + `git symbolic-ref --short refs/remotes/origin/HEAD` and strip the + `origin/` prefix; if that also produces nothing, display the error above + and stop. + - Non-zero for any other reason (authentication, invalid remote, + configuration) → fail closed: display the git error and stop. A stale + cached branch must not become authoritative for this safety check - 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 +86,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 +101,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 @@ -97,8 +115,9 @@ Before analyzing, **deduplicate related items.** Multiple comments often address the same underlying concern (e.g., two reviewers flag the same issue, or one reviewer comments on both the declaration and usage of the same problem). Group these into a single logical item — track all associated -`commentId`/`threadId` values so replies can be posted to each thread in -Phase 6. +`commentId`/`threadId` values, each with its own `isResolved` state, so +replies can be posted to each thread in Phase 6 and resolution warnings apply +per target rather than to the group. **For 5 or fewer items**, analyze and score inline — the context is small enough that agents add latency without improving quality. @@ -233,7 +252,10 @@ 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). + For each reply target whose thread is known to be resolved (`isResolved` + is true for that specific target), note it: a reply there 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..f3917ac 100644 --- a/pr-review-toolkit/skills/review-pr/SKILL.md +++ b/pr-review-toolkit/skills/review-pr/SKILL.md @@ -27,20 +27,15 @@ 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 Use only `allowed-tools`. Do not generate ad-hoc processing scripts. Workflow -return values and MCP responses are structured JSON; read them directly. Bash is -limited to the git patterns in `allowed-tools` for diff collection and -line-number translation below. - -The workflow and its agents are read-only — they must not call GitHub write tools. - -GitHub write tools may be used only after an exact preview and explicit final -posting approval from the user. +return values and MCP responses are structured JSON; read them directly. Bash +is limited to the `git diff` patterns used below. The workflow and its agents +are read-only. GitHub write tools may be used only after an exact preview and +explicit final posting approval from the user. ## Exit Plan Mode @@ -61,7 +56,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,36 +64,37 @@ 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: - -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". +`CHECKOUT_OK` → parse `mergeCommit`, `baseSha`, `headSha`, `mergeDiffFileCount`, +and (when present) `prDiffFileCount` from the `key value` lines. The script has +already verified `origin` matches the PR base repository. Then verify `headSha` +matches PR metadata `headSha`; on mismatch record "HEAD^2 does not match PR +headSha" as `fallbackReason` and skip to workflow launch without +`localGitManifest` or `fullDiff` — do not trust local diff data. ## 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` (tab-separated), 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. Paths with special +characters appear C-quoted (double quotes, `\t`/`\n`/`\"`/`\\`/octal escapes) — +strip the quotes and unescape 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 -additions/deletions; store as 0. Each entry: `{path, status, additions, -deletions}`. - -The resulting array is the `localGitManifest`. +From `NUMSTAT`, merge additions and deletions per file into the status list +(destination path as key; binary files show `-`, store as 0). Each entry: +`{path, status, additions, deletions}`. The resulting array is the +`localGitManifest`. ## Collect Full Diff (Optional) -If the manifest was built, collect the full merge diff: +If the manifest was built, collect the full merge diff (the explicit +prefixes keep header parsing stable under `diff.noprefix` or +`diff.mnemonicPrefix` configuration): ```bash -git diff --no-ext-diff --no-textconv HEAD^1 HEAD +git diff --no-ext-diff --no-textconv --src-prefix=a/ --dst-prefix=b/ HEAD^1 HEAD ``` Store as `fullDiff` if 200,000 characters or fewer; otherwise omit. @@ -112,6 +107,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) @@ -134,8 +131,14 @@ N findings recommended, M overlap existing threads, P discussion-worthy. Reviewers: code-reviewer, pr-test-analyzer, silent-failure-hunter. ``` -The reviewer list comes from `reviewMeta.selectedReviewers` which stores full -agent names. +The reviewer list comes from `reviewMeta.selectedReviewers` (full agent names). + +If `reviewMeta.threadCollectionFailed` is true, warn: 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 line: specialist prompts +listed only the highest-signal files (omitted count and categories are in that +field); the complete manifest was still collected. ### 2. Recommended to post (full detail) @@ -277,9 +280,15 @@ 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: - `⚠ Target thread is resolved — reply will stay collapsed and the PR author may - not see it.` +- if `isResolved` is true: `⚠ Target thread is resolved — reply will stay + collapsed and the PR author may not see it.` +- if `isResolved` is absent (resolution state not exposed by the read tools): + `(thread resolution state unknown — a resolved thread keeps this reply + collapsed)` +- 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 For review body text (non-line findings), show the review body. @@ -307,26 +316,25 @@ Use GitHub write tools only in this final approved step. If the approved preview has new line comments: -1. Create a pending review with - `mcp__plugin_github_github__pull_request_review_write`. -2. Add approved line comments with - `mcp__plugin_github_github__add_comment_to_pending_review`. -3. Submit the pending review with - `mcp__plugin_github_github__pull_request_review_write` using the approved - event and review body. +1. Create a pending review with `pull_request_review_write`. +2. Add approved line comments with `add_comment_to_pending_review`. +3. Submit the pending review with `pull_request_review_write` using the + approved event and review body. ### Posting thread replies for overlap findings Post overlapping findings as replies using `add_reply_to_pull_request_comment` with the numeric `commentId` and -`pullNumber`. If `commentId` is missing or invalid, post as a new line comment -via the pending review flow instead. Thread replies are independent of the -pending review submission. +`pullNumber`. If the reply API rejects the target as invalid, do not silently +change the posting location: convert the finding to a proposed new line +comment, show the revised preview, and ask for approval again — same as +invalid line locations below. Thread replies are independent of the pending +review submission. ### Review body only -If the approved preview has only review-body text, submit the review body with -`mcp__plugin_github_github__pull_request_review_write` using the approved event. +If the approved preview has only review-body text, submit it with +`pull_request_review_write` using the approved event. ### Invalid locations diff --git a/pr-review-toolkit/skills/review-pr/review-pr.js b/pr-review-toolkit/skills/review-pr/review-pr.js index 3e866cd..be04411 100644 --- a/pr-review-toolkit/skills/review-pr/review-pr.js +++ b/pr-review-toolkit/skills/review-pr/review-pr.js @@ -122,9 +122,13 @@ const FINDING_SCHEMA = { required: ['findings', 'positiveObservations'] } +// collectionFailed is required so a failed read can never be schema-valid +// while looking identical to a PR that simply has no review threads. const THREAD_SCHEMA = { type: 'object', + required: ['collectionFailed', 'threads'], properties: { + collectionFailed: { type: 'boolean' }, threads: { type: 'array', items: { @@ -149,11 +153,10 @@ const THREAD_SCHEMA = { } } }, - required: ['id', 'path', 'author', 'body', 'isResolved'] + required: ['id', 'path', 'author', 'body'] } } - }, - required: ['threads'] + } } const BOARD_ITEM_SCHEMA = { @@ -620,6 +623,14 @@ function asNumber(value, fallback) { return Number.isFinite(parsed) ? parsed : fallback } +// Verification counts from checkout output: -1 means unknown. Number(null) +// is 0, so null/undefined must be mapped to the sentinel explicitly or an +// absent count would masquerade as a known zero. +function sourceCount(sources, key) { + const value = sources ? sources[key] : undefined + return value == null ? -1 : asNumber(value, -1) +} + function firstString(values, fallback) { for (const value of values || []) { if (typeof value === 'string' && value.trim()) return value.trim() @@ -765,18 +776,41 @@ 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' const rightStatus = (right && right.status) || 'none' - const selected = (order[rightStatus] > order[leftStatus]) ? right : left + // On equal status, prefer the side with a usable reply target: commentId + // is what posting actually uses, so it outranks a thread-only identity. + let selected + if (order[rightStatus] > order[leftStatus]) { + selected = right + } else if (order[leftStatus] > order[rightStatus]) { + selected = left + } else if (left && left.commentId) { + selected = left + } else if (right && right.commentId) { + selected = right + } else { + selected = (left && left.threadId) ? left : (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 +832,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 +849,71 @@ 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') { + const idsSupplied = Boolean(existing.threadId || existing.commentId) + let matched = null + if (existing.threadId) { + // threadId is authoritative when supplied; matching commentId as a + // fallback here could resolve a mangled id pair to a different + // conversation and send an approved reply there. + matched = (threads || []).find(t => t && t.id === existing.threadId) || null + if (!matched) { + // The authoritative thread cannot be resolved: drop the comment + // target and resolution state too, because posting uses commentId + // and a mangled pair could still reply to the wrong thread. The + // preview flags the missing target and asks for a posting choice. + return { + status: existing.status, + threadId: existing.threadId, + commentId: undefined, + isResolved: undefined, + rationale: existing.rationale || 'Overlap classified during review synthesis.' + } + } + } else if (existing.commentId) { + matched = (threads || []).find(t => t && t.commentId === existing.commentId) || null + } else { + // Content matching attaches a thread only when no identity was + // supplied. A supplied identity that does not resolve is kept as-is + // (posting handles invalid targets and the preview flags missing + // ones) — redirecting the reply to a token-matched thread could + // target the wrong conversation. + 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 +924,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 +1027,77 @@ 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 = '' + var bytes = [] + // Octal escapes encode UTF-8 bytes, so a run of them must be decoded as + // one byte sequence, not per-byte code points ("caf\303\251" is café). + // decodeURIComponent is the sandbox-safe UTF-8 decoder — workflow scripts + // have no Buffer or Node APIs. Fall back per-byte on malformed sequences. + function flushBytes() { + if (bytes.length === 0) return + var encoded = bytes.map(function(b) { + return '%' + (b < 16 ? '0' : '') + b.toString(16) + }).join('') + try { + out += decodeURIComponent(encoded) + } catch (err) { + bytes.forEach(function(b) { out += String.fromCharCode(b) }) + } + bytes = [] + } + for (var i = 0; i < value.length; i++) { + var ch = value.charAt(i) + if (ch !== '\\') { + flushBytes() + 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) + } + bytes.push(parseInt(oct, 8)) + } else { + flushBytes() + out += escapes[next] != null ? escapes[next] : next + i++ + } + } + flushBytes() + 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 +1108,12 @@ 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/, + // Line-start comment markers, plus inline markers surrounded by + // whitespace (`x = 1 # note`, `y = 2 // note`) — the space-after + // requirement keeps CSS colors (#fff) and URLs (https://) out. String + // contents can false-positive; selection is liberal by design. + '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|