feat(pr-review-toolkit): production-harden checkout, overlap identity, and manifest verification

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",

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,17 +216,24 @@ 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:
 
 - `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.

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

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.

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/<branch>` 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