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,23 @@ 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
-on the merged checkout to inspect changed files and trace cross-file effects.
+Specialist reviewers run through `pr-review-analysis-readonly`, which
+disallows shell, file mutation, web, and known GitHub write tools so those are
+unavailable rather than merely discouraged, while read-only MCP tools (e.g.
+language servers for any language) remain usable when installed. In the local
+git path, specialists can use Read and Grep on the merged checkout to inspect
+changed files and trace cross-file effects.
 
 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

@@ -12,6 +12,13 @@ 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
+  - mcp__plugin_github_github__add_issue_comment
+  - mcp__plugin_github_github__issue_write
+  - mcp__plugin_github_github__push_files
+  - mcp__plugin_github_github__create_or_update_file
+  - mcp__plugin_github_github__merge_pull_request
+  - mcp__plugin_github_github__resolve_review_thread
 ---
 
 Analyze the PR using read-only context only.

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

@@ -16,18 +16,37 @@ review feedback, then post reply comments to GitHub.
 
 ## Phase 0: Branch Validation
 
-**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
+Branch context, collected at invocation:
+
+- Current branch: !`git branch --show-current`
+- Remote mainline lookup: !`bash -c 'out=$(git ls-remote --symref origin HEAD 2>&1); printf "%s\nexit_status:%s\n" "$out" $?'`
+- Cached mainline fallback: !`git symbolic-ref --short refs/remotes/origin/HEAD 2>/dev/null || echo none`
+
+**Determine the mainline branch from the remote lookup output above**
+(authoritative — the cached local `origin/HEAD` can be stale after a
+default-branch rename):
+
+- `exit_status:0` → the mainline is the branch name in the
+  `ref: refs/heads/<branch>` line. If no such line exists, display "Error:
+  Could not determine the mainline branch." and stop execution.
+- Non-zero exit status with a transport-level connectivity error in the
+  output ("Could not resolve host", "Connection refused", "Connection timed
+  out", "Network is unreachable", "Operation timed out") → use the cached
+  mainline fallback, stripping the `origin/` prefix; if it shows `none`,
+  display the error above and stop. "unable to access" qualifies only when
+  paired with such a transport cause — with an HTTP status (401, 403, etc.)
+  it is an authentication failure, not connectivity.
+- Non-zero exit status for any other reason (authentication, invalid remote,
+  configuration) → fail closed: display the captured 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 +90,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 +105,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 +119,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 +256,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