From 30f0fb65ccaad5020221aca75bbc4b0e1f755f78 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 13 Aug 2026 20:40:23 +0000 Subject: [PATCH] docs(pr-review-toolkit): zero-prompt redesign proposal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Design document for the next iteration of review-pr, replacing the WORKFLOW_REWRITE_* docs as implementation direction. The design goal: fully automated review with zero permission prompts from invocation through the interactive board, with posting the approved review as the only write action ever, gated on exact preview plus explicit approval. Key decisions, each backed by empirical tests recorded in the doc: - Ephemeral workspace: a fresh mktemp-unique git worktree of the PR head per invocation (checkout/ beside toolkit artifacts), in sandbox-writable temp; the user's checkout is never mutated and recovery is never git reset --hard - Head-anchored findings by construction; the line-number translation step is deleted entirely, with a pre-posting head revalidation guard - SHA-only transport: ls-remote pins both PR SHAs before any fetch, objects are fetched by SHA with --no-tags --no-write-fetch-head, and nothing is written to the shared ref store — no shared mutable state between concurrent sessions; merge^1 resolves the base with no API dependency, and absent/stale merge refs or any base movement defer the manifest to a script entry point driven by pull_request_read's base branch name - MCP bulk-file fallback and clone-from-scratch mode deleted; matching remote (unshallow) or honest error; credentials inherited from working remotes - Git-native-only mutations (the permission layer blocks rm -rf everywhere; git subcommands run prompt-free); no environment detection (TMPDIR-then-/tmp verified against both local and web harness contracts) - Script-owned output contract (WORKSPACE_OK/WORKSPACE_ERROR, literal paths derived from git diff -z) with layered argument validation on the model-issued manifest entry point - Enforcement honesty: disallowedTools covers GitHub writes, file mutation, Task, web; specialist read-only git is instruction-level, and PR_REVIEW_REQUIREMENTS.md's safety clause is amended in the same change so requirement and design agree Validated by two independent cold-read comprehension passes and three review rounds (Copilot, CodeRabbit, and maintainer feedback); all findings resolved. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01Jok39zoTctC4dBJEAfjYEZ --- .../docs/PR_REVIEW_REQUIREMENTS.md | 7 +- .../docs/ZERO_PROMPT_REDESIGN.md | 646 ++++++++++++++++++ 2 files changed, 651 insertions(+), 2 deletions(-) create mode 100644 pr-review-toolkit/docs/ZERO_PROMPT_REDESIGN.md diff --git a/pr-review-toolkit/docs/PR_REVIEW_REQUIREMENTS.md b/pr-review-toolkit/docs/PR_REVIEW_REQUIREMENTS.md index 23cc48d..ac27e14 100644 --- a/pr-review-toolkit/docs/PR_REVIEW_REQUIREMENTS.md +++ b/pr-review-toolkit/docs/PR_REVIEW_REQUIREMENTS.md @@ -69,7 +69,9 @@ optimized first as separate user-facing commands. ### Safe And Predictable Tooling -- GitHub data should be fetched through GitHub MCP tools. +- GitHub API data (metadata, review threads) and review posting should + go through GitHub MCP tools; repository content may come from + read-only local git. - GitHub read operations and GitHub write operations should be clearly separated. - Posting should only happen after explicit user approval. - The toolkit should avoid ad-hoc, model-generated parsing scripts for GitHub @@ -276,7 +278,8 @@ The user should be able to edit or remove drafted comments before posting. - Analysis should avoid tools capable of modifying the repository unless the user explicitly requests a different workflow. -- GitHub data access should use GitHub MCP read tools. +- GitHub API data access should use GitHub MCP read tools; repository + content may come from read-only local git. - GitHub posting should use GitHub MCP write tools. - Generated parsing scripts should not be used for GitHub data. - Reusable helper scripts, if used, should be bundled with the plugin and diff --git a/pr-review-toolkit/docs/ZERO_PROMPT_REDESIGN.md b/pr-review-toolkit/docs/ZERO_PROMPT_REDESIGN.md new file mode 100644 index 0000000..83ae682 --- /dev/null +++ b/pr-review-toolkit/docs/ZERO_PROMPT_REDESIGN.md @@ -0,0 +1,646 @@ +# PR Review Toolkit: Zero-Prompt Redesign + +Status: design decisions resolved; ready for implementation. This document +supersedes `WORKFLOW_REWRITE_PLAN.md` and `WORKFLOW_REWRITE_CONTEXT.md`. +`PR_REVIEW_REQUIREMENTS.md` remains authoritative; this change amends +its two GitHub-data-transport clauses (API data via MCP; repository +content via read-only local git), recorded in Resolved Design +Decision 6. + +Audience: the engineer implementing the rewrite. This document specifies +intent — goals, invariants, component roles, and control flow. Anything +finer-grained is an implementation-time decision. Evidence for settled +choices is compressed into Resolved Design Decisions at the end; do not +re-litigate them without new evidence. + +Scope: the `review-pr` skill and its components. `address-pr-feedback` +is out of scope and unchanged. + +## Design Goal + +Run fully automated with **zero tool permission prompts** from invocation +through the review board and interactive drafting loop. The **only +sanctioned GitHub write is posting the approved review**, behind an exact +preview and explicit user approval. Everything before that gate is +read-only outside the toolkit's own footprint: fetched objects, one +worktree registration, one ephemeral workspace. + +Invariants: + +- **The user's checkout is never mutated.** `git status`, branch, and + index are identical before and after a review, including failed ones. + No refs or branches are ever created in the user's repository. +- **Every git-state mutation goes through a git subcommand.** Never + `rm -rf` (the permission layer blocks it everywhere; git subcommands + run prompt-free). Non-git writes are confined to the workspace: the + `mktemp` root the builder creates — no component writes any other + file. +- **No component pushes, commits, or writes GitHub** except the skill's + approved posting step. +- **Findings are anchored to PR-head line numbers from birth.** There is + no line-number translation anywhere. + +## Precondition + +The skill must run from a clone with a remote that resolves to the PR +base repository — any remote name; fork setups with the base repo as +`upstream` are first-class. Remote URLs come in several forms — +`https://github.com/owner/repo[.git]`, `git@github.com:owner/repo[.git]`, +`ssh://git@github.com/owner/repo[.git]` — and matching must normalize +all of them (case-insensitively) to the same owner/repo identity. No matching remote → error immediately, +naming the base repo and the fix. There is no clone-from-scratch mode +and no MCP bulk-data fallback. Authentication is inherited from the +matched remote; the toolkit never handles tokens. + +## Background + +The current implementation was built for manual permission mode and +carries three compensations that are now liabilities: an in-place +checkout script that mutates the user's worktree (~250 lines of guard +plumbing), a merge-ref checkout that forces error-prone line-number +translation before posting, and MCP as the bulk-data channel (hundreds +of lines of pagination/retry sensitive to output-token limits). The +review board taxonomy, specialist lenses, overlap classification, and +preview/approve/post discipline all work well and carry forward +unchanged. + +## Architecture + +Six components, each with one job: + +| Component | Role | Writes to | +|-----------|------|-----------| +| `skills/review-pr/scripts/workspace.sh` | Workspace builder: worktree of the PR head in temp space, pinned SHAs, merge signal | Workspace + git metadata, via git subcommands only | +| `skills/review-pr/SKILL.md` | Interactive controller: verify, launch workflow, present board, draft, preview, approve, post | GitHub (approved posting only) | +| `skills/review-pr/review-pr.js` | Analysis orchestrator: reviewer selection, agent fan-out, board assembly | Nothing | +| `agents/pr-review-collector.md` | GitHub MCP reader: review threads | Nothing | +| `agents/pr-review-specialist.md` | Read-only analysis in the workspace | Nothing | +| `agents/pr-review-synthesis.md` | Tool-denied board synthesis from prompt JSON | Nothing | + +`pr-review-collector` renames `pr-review-github-collector` (update the +workflow's `agentType` reference); `pr-review-specialist` replaces +`pr-review-analysis-readonly`; `pr-review-synthesis` is new. Renamed +agents make this a major version bump. These stay plugin-defined +agents rather than built-ins (Explore, Plan) deliberately: the whole +point of each profile is a tool boundary the plugin pins — write +tools excluded, synthesis tool-free — and built-in definitions evolve +with the harness outside the plugin's control. + +Only the skill's main conversation may invoke GitHub write tools, and +only in the approved posting step. All three agents structurally exclude +GitHub write tools: collector and synthesis via `tools:` allowlists +(collector lists its one read tool; synthesis lists none), the +specialist via a `disallowedTools` denylist (an allowlist would block +repo-varying read-only tools like language servers). Enumerate the full +current GitHub write-tool surface in that denylist at implementation +time and re-audit it when the github plugin updates. + +## Component: Workspace Builder (`scripts/workspace.sh`) + +Replaces `checkout.sh`. One small deterministic script with one job: +build the workspace during skill preprocessing and emit the pinned +facts. Everything after that — base resolution when deferred, the +changed-file list, diffs — is ordinary git run by the skill or the +specialists in the workspace. + +Invoked in the skill body exactly as `checkout.sh` is today: + +```markdown +- Workspace: !`bash "${CLAUDE_SKILL_DIR}/scripts/workspace.sh" setup "$pr-url"` +``` + +The harness runs this while assembling the prompt; the model never +invokes setup itself, and the skill body must say so. Running as one +pre-approved command is what turns sandbox EPERMs into `WORKSPACE_ERROR` +instead of permission prompts. `$pr-url` is input the user typed into +their own session — not an external injection channel; external values +(remotes, SHAs) never ride this substitution. + +Two harness properties are **gating preconditions** — test them before +shipping, because no documentation specifies them: + +1. Skill-argument substitution into preprocessing commands must be + shell-safe (test with a metacharacter-bearing argument). If it is + raw, do not ship this interface: split setup into an argument-less + preprocessing preflight plus a model-issued, pattern-allowed setup + call whose URL argument the skill validates first. +2. Preprocessing must tolerate a long-running command (the setup path + can legitimately span minutes on a large repo). If it enforces a + short ceiling, use the same contingency. + +Setup steps, in order: + +1. **Validate the PR URL**; extract owner, repo, number. Reject + anything that is not a plain GitHub PR URL. +2. **Locate the source remote.** Scan all remotes for one whose URL + resolves to the base repository — match host *and* path (PR URLs are + always `github.com`; never fetch a same-named repo on another host). + Prefer `origin`, else first in `git remote` output. Validate the + selected remote name against the remote pattern (below) here, before + it is ever interpolated — a leading-hyphen remote name would parse + as an option. Fail as `WORKSPACE_ERROR`: no match, not a git repo, + shallow clone ("run `git fetch --unshallow`" — merge-base needs full + ancestry), or partial/promisor clone (implicit lazy fetches would + violate the single-remote model). +3. **Hygiene.** Run `git worktree prune` (plain — git's default + expiry), clearing registrations left stale by temp cleanup of prior + workspaces. This is the same prune the user's own `git gc` runs. +4. **Discover and pin.** One `ls-remote` reads both PR refs without + writing any local state: + + ```bash + git ls-remote "refs/pull/N/head" "refs/pull/N/merge" + ``` + + The SHAs arrive as one snapshot. Missing merge line = conflicted or + not-yet-computed test merge (exit 0, verified). Missing head line = + PR not on this remote: `WORKSPACE_ERROR`. Pin `head_sha` (and + `merge_sha` if present) as script variables. Then fetch **by SHA**: + + ```bash + git fetch --no-tags --recurse-submodules=no \ + "$head_sha" [ "$merge_sha" ] + ``` + + GitHub accepts SHA wants and this creates zero local refs + (verified; `FETCH_HEAD` is transiently rewritten as in any fetch — + nothing in the toolkit reads it). `--no-tags` stops tag + auto-following from creating refs in the user's repo; + `--recurse-submodules=no` matters because the fetch default is + *on-demand* — fetched commits that update submodule gitlinks would + trigger fetches of other remotes. A + force-push between `ls-remote` and fetch fails the fetch honestly; + re-running picks up the new state. + + No prompt-suppression or timeout machinery: this is an interactive + flow, credentials come from the remote the user already fetches + from, and a hung network command is bounded by the harness's own + command timeout, surfacing as an honest failure. + + With objects local: `base_sha=$(git rev-parse "${merge_sha}^1")` — + the merge ref's first parent is the base tip. Check `${merge_sha}^2` + against `head_sha`: a mismatch means the test merge is **stale** + (GitHub recomputes lazily; after a rebase a stale `merge^1` predates + the true fork point and would corrupt the file list). Treat stale + exactly like absent: record it, discard `base_sha`, and leave base + resolution to the skill (step 3 of its flow). +5. **Create the workspace**, fresh every invocation: + + ```bash + ws=$(mktemp -d -p "${TMPDIR:-/tmp}" "pr-review---.XXXXXXXX") + git -c core.hooksPath=/dev/null -c core.sparseCheckout=false \ + worktree add --detach "$ws/checkout" "$head_sha" + ``` + + `--detach` is explicit intent (a raw SHA detaches anyway): no + branch is ever created, and concurrent sessions can hold the same + commit without "already checked out" conflicts. + + `worktree add` is not atomic: on failure, run + `git worktree remove --force` on the path just attempted (the one + sanctioned removal; `--force` because plain `remove` refuses a + dirty or partially-populated worktree — exactly what a failed + checkout leaves) and report manual recovery in the + `WORKSPACE_ERROR` if that also fails. + + The two overrides disable the checkout hazards that matter: + + - **Hooks**: `post-checkout` runs on `worktree add` → + `core.hooksPath=/dev/null`. + - **Sparse checkout**: repository-shared `core.sparseCheckout` + silently omits tracked files from the new worktree (verified) → + force-disabled so the analysis tree is complete. + + Content filters (LFS included) run exactly as they would for the + user's own checkout, with the user's own configuration and + credentials. + + The workspace contains only `checkout/` — the toolkit writes no + artifact files. + + `mktemp -p` works identically on GNU and BSD/macOS (verified; + modern BSD documents it). `${TMPDIR:-/tmp}` is correct in both + harness environments (verified: local Claude Code exports a + sandbox-writable `TMPDIR`; web leaves it unset with a writable, + container-isolated `/tmp`). Do not add environment detection, and + do not pre-verify writability — an unwritable root fails inside the + script as `WORKSPACE_ERROR`, never a prompt. + + Lifecycle: the builder mutates only the directory it just created. + No component calls `git worktree remove` in normal operation — an + interactive session has no well-defined end, so directory cleanup + belongs to the OS temp reaper and stale registrations to prune + (step 3 — cleared once git's normal expiry passes). `worktree + remove` is the documented manual-recovery primitive. +6. **Compute merge-base** (skipped when deferred): + `merge_base=$(git merge-base "$base_sha" "$head_sha")`, and + `baseAhead` via `git rev-list --count`. +7. **Emit the output block.** + +### Output contract + +Setup emits one machine-readable format, owned by the script: + +- Failure: one line, `WORKSPACE_ERROR: `, with an actionable + reason. An error trap guarantees the line even on unanticipated + failures (reachable: `git merge-base` fails on unrelated histories, + which are formally out of scope — the honest error is the specified + behavior). The skill treats anything that is neither `WORKSPACE_OK` + nor `WORKSPACE_ERROR` as an error. There is no degraded path — do + not reuse the name `CHECKOUT_SKIP`; nothing is skipped, it fails. +- Success: `WORKSPACE_OK`, then `key value` lines: `workspace`, + `remote`, `headSha`, `baseSha`, `mergeBase`, `baseAhead` (commits + merge-base→base tip, the board's "base has moved" count), and + `mergeRef` (`present`/`absent`/`stale`). Keys unresolved on the + deferred path emit `-`. + +### Why a script + +Not permission batching (auto mode makes direct git prompt-free). Three +reasons: remote-URL normalization must be deterministic and testable, +not re-derived by a model each run; preprocessing substitution puts +the workspace, SHAs, and merge signal in place before the model +generates a token; and the output contract needs a single owner +(instructed prose is how the current SKILL.md accumulated parsing +rules). Guard: the +script must never grow logic that competes with git — it is URL +parsing, a remote loop, a handful of git subcommands, and printf. + +## Component: Skill (`review-pr/SKILL.md`) + +Flow, in order: + +1. Parse the PR URL; read the builder output. On `WORKSPACE_ERROR`, + report and stop. +2. Fetch metadata via `pull_request_read` (`get`). Verify workspace + `headSha` against GitHub's. Mismatch = head moved during setup: + fail honestly, tell the user to re-run. +3. Resolve the base when setup deferred it, or when metadata + `base.sha` differs from setup's `baseSha` — recompute on *any* + difference (an ancestry check cannot detect the base absorbing the + PR's own commits, which moves the fork point): + + ```bash + git fetch --no-tags --recurse-submodules=no + git -C /checkout merge-base + git -C /checkout rev-list --count .. + ``` + + Same fetch flags as setup; no second `ls-remote` — `base.sha` comes + from the same API snapshot as the head verification, so nothing + races a moving ref. Before interpolating, the skill validates: + `` must byte-match setup's emitted `remote`, `` + must byte-match metadata `base.sha` and `^[0-9a-f]{40}$`. These + checks are the model following instructions — stated this precisely + for exactly that reason. +4. Build the changed-file list, in the workspace: + + ```bash + git -C /checkout -c core.quotePath=false --no-pager \ + diff --name-status --find-renames HEAD + git -C /checkout --no-pager \ + diff --numstat --find-renames HEAD + ``` + + Join into `{path, status, additions, deletions}` records for the + workflow `args` (rename records carry old and new paths; numstat + shows `-` for binary files — categorization maps those). + `core.quotePath=false` keeps non-ASCII paths literal; truly + pathological names (embedded quotes, control bytes) still C-quote + and are rare enough to accept. On PRs large enough to truncate + command output, page it (`| sed -n ',p'`) until the + list is complete — completeness is checked against + `git diff --name-status | wc -l`, and only per-file numstat detail + may be dropped, disclosed in `reviewMeta`. +5. Compute the full diff in the workspace: + + ```bash + git -C /checkout --no-pager diff \ + --no-ext-diff --no-textconv --no-color -U3 --find-renames \ + --src-prefix=a/ --dst-prefix=b/ HEAD + ``` + + The flags keep user config (`diff.external`, `diff.noprefix`, + `color.ui=always` — verified to break header parsing — pagers, + `diff.renames=false`) from reshaping the output. What must hold is + line-anchor equivalence with GitHub: both sides diff the same two + trees, so head line numbers are directly postable. The 200K-char + cap gates prompt inclusion only: truncate at a hunk boundary, flag + it in `args`, surface it in `reviewMeta`; the checkout is complete, + so specialists can recompute anything dropped. Then launch the + workflow. The skill writes no files — everything the workflow and + specialists need travels via `args` and the workflow-built + prompts. +6. Present the review board and run the interactive loop exactly as + the current SKILL.md specifies. Merge-signal notes: `mergeRef` not + `present` → interpret via API mergeability (`mergeable: null` + means still computing — say so); positive `baseAhead` → "base has + moved N commits" note. +7. Preview and approval: unchanged. Exact preview of every comment, + body, and event; explicit approval via `AskUserQuestion`. +8. Post via GitHub MCP write tools after one final guard: re-fetch + metadata; a changed `headSha` aborts outright; a changed `base.sha` + triggers the step-3 commands again — abort only if the recomputed + `mergeBase` differs from the reviewed one (fork point moved; + anchors shifted). Bind the pending review to the reviewed head via + the write tool's `commitID` parameter. The narrow check-then-act + window between guard and submission is an accepted residual. This + is the only GitHub write in the system. + +Deleted from the current SKILL.md with no replacement: the manifest +parsing and C-quoted-path rules ("read the builder's structured +output" replaces them), the entire line-number translation section +(findings are head-anchored by construction; comments that miss the +PR diff go to the review body, as today), and the manual-mode/plan-mode +preamble. + +## Component: Workflow (`review-pr.js`) + +Expected to shrink to roughly a third of its ~1,850 lines. Keeps: + +- **Reviewer selection** from the categorized file list (same rules, + same lenses; categorization stays deterministic JS, fed via `args`). +- **Collector fan-out**: one call for review threads (metadata comes + from the skill via `args`). +- **Specialist fan-out**: selected specialists in parallel; each + prompt carries the metadata subset, lens instructions, thread + context, and the workspace path. +- **Synthesis** via the `pr-review-synthesis` agent (never the + specialist profile): dedup, overlap classification, board assembly + with the existing section taxonomy. `reviewMeta` is assembled by + deterministic workflow JS after synthesis (as `finalizeBoard` does + today) from flags the skill passes — the agent cannot know what the + skill dropped. +- **Diff-header path unquoting** (`unquoteGitPath`): patch text has no + `-z` form, so `+++ b/` headers still C-quote; this decoder + stays. + +Sheds: all `get_files` pagination/recovery, the MCP-manifest fallback +path, manifest verification counts. Nothing may depend on +`MAX_MCP_OUTPUT_TOKENS`. + +Workflow `args` carries: owner/repo/number; the metadata subset +(title, body, author, state, review decision, `headSha`, `base.ref`, +`base.sha`); the file-list records `{path, status, additions, +deletions}`; the capped `fullDiff` plus truncation flag; the workspace +path; and provenance (`mergeRef`, `baseAhead`). This +is the only transport into the workflow — workflow scripts cannot read +files or run git, so `args` is the floor, and specialists reach bulk +data through the workspace directly. + +## Component: Agents + +**`pr-review-collector`** — the current collector renamed: GitHub PR +read tools only (allowlist), no shell, no files; scope shrinks to +review threads. + +**`pr-review-specialist`** — replaces `pr-review-analysis-readonly` +with one deliberate expansion: read-only git against the workspace +checkout (`git -C /checkout log|blame|show|diff` — never +`cd`), because history is genuinely valuable review context and the +old design excluded it only for permission-UX reasons. Required +command hygiene, stated verbatim in the agent definition: hardened +diff flags (`--no-ext-diff --no-textconv --no-color`), pagers off; +PR-controlled +paths go after a literal `--`, and a path may appear as a shell +operand only if it matches `^[A-Za-z0-9._/ -]+$` — otherwise inspect +it with Read/Grep, which pass paths outside the shell. The definition +hard-denies file mutation, `Task`, web tools, and all GitHub write +tools, and confines shell use to the read-only git forms above (this +subsumes the old "no generated parsing scripts" rule). + +**`pr-review-synthesis`** — new, narrowest profile: no tools at all. +Synthesis works entirely from prompt JSON and returns structured +output; it is also the agent whose input is mostly untrusted text +(finding bodies, thread comments), which is exactly the agent that +should hold no capabilities. The current workflow's prompt-level "do +not call tools" becomes structural. + +## End-to-End Flow + +```text +claude "/pr-review-toolkit:review-pr " (from any clone with a + | remote matching the base + | repo; otherwise: error) + |-- preprocessing: workspace.sh setup + | remote scan -> ls-remote (pin SHAs) -> fetch by SHA + | -> mktemp -> worktree add (hooks off, detached) + | -> merge-base (or: deferred to skill) + | emit: WORKSPACE_OK block (or WORKSPACE_ERROR) + | + |-- skill: pull_request_read(get) -> verify headSha + |-- skill: git fetch base + merge-base (if deferred or base moved) + |-- skill: git diff --name-status/--numstat (file list -> args) + |-- skill: git diff HEAD in workspace (head-anchored) + v +Workflow(review-pr.js) + |-- collector agent --------- MCP: review threads + |-- specialist agents (par) - Read/Grep + read-only git in workspace + |-- synthesis agent --------- dedup, overlap, board assembly (no tools) + v +skill: present review board (incl. merge-signal note when not clean) +skill: interactive loop (challenge / select / draft) <- user judgment +skill: exact preview + AskUserQuestion approval <- the gate +skill: post review via GitHub MCP write tools <- only write +``` + +## Merge-Result Awareness + +The head checkout gives up seeing the code as it will land. The merge +ref returns in two bounded roles only: as a **pointer** (`merge^1` +locates the base tip) and as a **signal** (absence + API mergeability → +conflict note; `baseAhead` → "base has moved N commits" note). Never as +*content*: nothing is checked out from it and no finding ever anchors +to merge-ref line numbers. The moment merge awareness needs its own +checkout, it has rebuilt the thing this redesign deleted. + +## Permission Model + +Guard outcomes, not capabilities: + +- **Target is auto permission mode.** Read-only and git-native + workspace operations run prompt-free — verified empirically, + including a 29k-file checkout and SSH fetches, from the main + conversation; verify specialist *subagents* get the same treatment + during implementation (fallback: deny specialist Bash). The skill + must not require manual mode. +- **`rm -rf` is blocked at the permission layer regardless of target; + git subcommands doing equivalent deletions run prompt-free** + (verified). Hence the git-native-mutation invariant. +- **`allowed-tools` is defense in depth**: the setup invocation (kept + so stricter modes cannot break preprocessing), a `git fetch` pattern + for the skill's base-resolution fetch, read-only git patterns by + subcommand, the GitHub + tools the skill uses, `AskUserQuestion`/`Workflow`/`ExitPlanMode` + (exit plan mode first if active). Verify `${CLAUDE_SKILL_DIR}` + expansion inside patterns during implementation. +- **The GitHub write boundary is structural** (tool exclusion in all + three agent definitions). **Specialist "read-only git" is + instruction-level** — frontmatter can't express it (Bash is + all-or-nothing; read-only subcommands accept `--output`; static + patterns can't pin an ephemeral path). Note the workspace is a + linked worktree sharing the user's object/ref store: "disposable" + bounds filesystem damage, not git-state damage. If that residual is + unacceptable in a deployment, re-denying specialist Bash is a + one-line lever. +- **Posting authorization is behavioral.** Write tools are listed for + the whole skill lifetime (the harness cannot phase tool exposure), + so they are technically callable before the gate; the approval gate + is conversational. Bounded by the agent boundary: bulk untrusted + content is processed by write-denied agents, and only their + structured output reaches the write-capable conversation. Removing + the write tools from `allowed-tools` is the structural-gate lever — + one harness prompt, at the one write. +- **Sandbox denials fail the build honestly.** If the protected-path + stress test (a PR touching a tracked `.claude/` path) shows real + EPERMs, the behavior is still `WORKSPACE_ERROR` — no mitigation is + pre-sanctioned, and sparse-checkout exclusion specifically is ruled + out (incomplete tree, and `git sparse-checkout` in a linked worktree + mutates shared repo config). What must never return is a parallel + implementation of git checkout. + +## Data Flow + +git is the bulk channel; MCP is the metadata channel. + +| Data | Source | Consumer | +|------|--------|----------| +| Changed-file list | `git diff --name-status`/`--numstat`, run by skill in workspace | Workflow via `args`; specialists re-derive as needed | +| Full PR diff | `git diff` in workspace | Specialists (capped inline; recomputable) | +| File contents, history | Workspace (Read/Grep/read-only git) | Specialists | +| Base location, merge signal | Merge ref parents; API mergeability | Builder, board note | +| PR metadata | `pull_request_read` (get) | Skill → workflow `args` → specialist prompts | +| Review threads | MCP via collector agent | Workflow synthesis | +| Review posting | MCP write tools | Skill, approved step only | + +There are no artifact files at all: metadata and the file list ride +`args`, and the workflow builds specialist prompts from them plus the +workspace path. Threads are collected inside the workflow +and reach specialists inline (they are small). Categorization is +deterministic workflow JS, never model judgment. Large-PR philosophy +is unchanged: complete file list always, low-signal areas summarized, +attention where signal is highest, truncation always disclosed. + +## What Is Deleted + +The explicit kill list, so nothing survives out of inertia: + +- `checkout.sh`: protected-path pathspecs, skip-worktree bookkeeping, + untracked-overwrite guards, restore-on-failure machinery (~250 of + 289 lines). +- SKILL.md: line-number translation, manifest parsing / C-quoted-path + rules, the manual-mode/plan-mode preamble, origin-must-match. +- `review-pr.js`: `get_files` pagination and recovery, MCP-manifest + fallback, manifest verification counts, the PR-metadata collector + call. +- README: wrapper-function-era guidance (dedicated worktree, origin + remapping) — replaced by "run from any clone with a remote for the + base repo". + +## Accepted Residuals + +Stated once, not claimed away: + +- Posting gate is behavioral (see Permission Model); structural lever + documented. +- Specialist read-only git and the skill's byte-equality argument + checks are instruction-level; structural lever documented. +- The specialist denylist admits future unlisted GitHub write tools + until re-audited. +- Checkout content transforms (line endings, LFS and other + `.gitattributes` filters) run as they would for the user's own + checkout; line anchors are unaffected because diffs and the file + list are computed tree-to-tree. +- Network hangs are bounded only by the harness's command timeout — + no toolkit-level watchdog. +- The check-then-act window between the pre-posting guard and + submission. + +## Success Criteria + +1. `claude "/pr-review-toolkit:review-pr "` produces a review + board with zero permission prompts under auto mode, from a base-repo + clone and from a fork clone with the base as `upstream`; from a + non-matching directory it errors immediately with no partial work. +2. The user's checkout is untouched — status, branch, index identical + before and after, including after mid-review failures. No refs, no + `rm -rf` anywhere. +3. No line-number translation exists anywhere; posted comments land + correctly because findings are head-anchored. +4. The `claude-review-pr` shell wrapper is obsolete (secret injection + moves to the github plugin's MCP config — companion change, out of + scope here). +5. The only GitHub write in any transcript is the approved posting + step, always preceded by exact preview and explicit approval. +6. Nothing depends on `MAX_MCP_OUTPUT_TOKENS`. +7. The review board, interactive loop, drafting, and preview + discipline satisfy `PR_REVIEW_REQUIREMENTS.md` unchanged. + +Validation: reuse the representative-PR matrix in the README, plus: +both invocation contexts and the no-remote error; head moves during +setup (honest error); re-invocation (fresh workspace; old +registrations cleared by prune after git's normal expiry); two +concurrent sessions on the same +PR; merge ref absent (skill resolves base from metadata + board +note); merge ref +stale after a push (`merge^2` ≠ head — must defer); base advanced +(note + recompute); head force-pushed before posting (guard aborts); +shallow/partial clone (precondition error); an LFS repo (checkout +smudges via the user's own LFS setup); sparse-checkout clone (complete +worktree); a control-byte/non-UTF-8 path (honest refusal); the tracked +`.claude/` sandbox stress test; and the gating checks — substitution +shell-safety, preprocessing duration, `${CLAUDE_SKILL_DIR}` pattern +expansion, specialist-subagent prompt-freedom. + +## Resolved Design Decisions + +Empirical evidence backing the settled choices (a real macOS session +in auto mode, fork clone of a large repo; live GitHub PR refs): SSH +fetch of `refs/pull/N/head`, a 29k-file worktree build in temp, and +Read/log/blame in it all ran with zero prompts; `rm -rf` was +consistently denied while `git worktree remove` ran prompt-free; +`ls-remote` snapshots tolerate a missing merge ref; fetch-by-SHA is +accepted by GitHub and creates zero local refs; unqualified refspec +destinations become visible branches (why named temp refs were +abandoned); `core.quotePath=false` yields literal non-ASCII paths +(only pathological names still C-quote); +`color.ui=always` corrupts non-tty diff output; shared +`core.sparseCheckout` makes new worktrees sparse; `mktemp -p` works on +GNU and BSD/macOS. + +1. **MCP bulk-file fallback: deleted.** Git is the only data path; + when the workspace cannot be built the review fails with the + reason. A fallback that runs rarely is broken when needed, and its + use cases (dirty worktree, wrong origin, forks) no longer exist. +2. **Workspace: fresh mktemp-unique worktree per invocation; git-native + operations only; no removal in normal operation; plain + `worktree prune` at setup for hygiene** (default expiry — the same + prune the user's own `git gc` runs; `--expire now` was rejected as + able to clear a user's own registration on a temporarily unmounted + volume). Rejected: shared-object clones (no git subcommand deletes + a clone) and deterministic PR-keyed paths (shared mutable state). +3. **Clone-from-scratch and its auth story: removed by precondition.** + Credentials are inherited from the user's working remotes; the + toolkit never touches tokens. +4. **Merge awareness: pointer and signal, never content.** Deferred: + semantic-conflict cross-referencing. +5. **SHA-only transport.** `ls-remote` pins both PR SHAs into script + variables; objects are fetched by SHA; no local refs ever exist, so + concurrent sessions cannot interfere and there is nothing to clean + up. Replaced a named-temp-ref design whose shared refs raced + concurrent sessions. Base resolution: `merge^1` when the merge ref + is present and `merge^2` matches the head; otherwise the skill + resolves the base itself, driven by API `base.sha`. Recompute on any + base movement (ancestry checks cannot detect the base absorbing PR + commits). +6. **Enforcement honesty.** Structural denial covers what tool names + can express; specialist git hygiene and argument validation are + instruction-level and labeled as such; sandbox tolerance is + fail-honest with no pre-sanctioned mitigation. The two + `PR_REVIEW_REQUIREMENTS.md` GitHub-data clauses are amended to + match (narrowed to API data; repository content travels over + read-only local git). The Safety clause "analysis should avoid + tools capable of modifying the repository unless the user + explicitly requests a different workflow" stands unamended: this + design is that explicitly requested workflow, and the specialists' + read-only-git contract honors its intent.