fix-pr-review

/fix-pr-review: Triage, Fix, and Resolve PR Review Comments

Takes a PR review from CodeRabbit, /review-pr, or pasted text. It triages each finding, applies validated fixes, runs /done, then replies to and resolves the GitHub conversations in one flow.

Use AskUserQuestion only when the run still needs a user decision: branch safety, stash confirmation, contested-item confirmation, --interactive per-fix confirmations, type-check failure triage, and post-completion next actions. Invoking /fix-pr-review or imperatively asking to fix review findings authorizes execution of the validated FIX plan. Do not ask for separate plan approval. Mark every required option as concrete and considered, strongest first with "(Recommended)".

Quick Reference

Phase Overview

R-Rubric Summary (Phase 3 STEP 4; first match wins)

R4/R5 sit above R3 by design, not by accident, and R10 sits above R6 so a technically correct finding never reaches the fix loop on the strength of being correct. The full rubric and the reason for that order live in references/triage-rubric.md.

Key Cross-References

• R-rubric definition: references/triage-rubric.md, loaded by the triage subagent at Phase 3 STEP 4, not by main
• Plan validation rules: Phase 4 (validates fields required by R-rubric)
• Fix execution routing: Phase 5 (executes R6 FIX items)
• Convergence: Phase 5.5 (consumes class_completeness + inverse_risk from the plan; feeds fix_status and the per-fix convergence: report line)
• Reply validator: references/github-reply-resolve.md Step 7b (format rules for GitHub replies)
• Report grouping: Phase 8 (groups by R-classification)
• Suppressions: Phase 3 "Load review suppressions" (main agent reads, before dispatch) + references/triage-prompt.md (subagent applies them before classifying) + Phase 8 (write)

Reference files

Each carries its firing condition in the pointer at the point of use. Load it there, not up front.

• references/fetch-review-data.md: per-input-type GraphQL/REST fetch, CodeRabbit review-body anatomy, Comment schema. Loaded by main in Phase 2, at the GitHub fetch step, and again at the Comment-schema normalisation step if the local-file path meant it was not read there.
• references/triage-prompt.md: the whole Phase 3 subagent prompt (STEP 0 → STEP 6 + output format). Read by main in Phase 3, placeholder-substituted, passed verbatim.
• references/triage-rubric.md: R1-R10 detail, NEEDS-INPUT calibration, change_class worked examples, reply formats. Loaded by the triage SUBAGENT at STEP 4.
• references/github-reply-resolve.md: Steps 7a-7d posting/resolving mechanics. Loaded by main in Phase 7; never loaded for local-file input.
• references/final-report.md: the Phase 8 report template and its rendering rules. Loaded by main in Phase 8 before printing.
• references/branch-safety.md: repo plus branch landing with detached-HEAD and different-branch questions. Loaded by main in Phase 1 for GitHub inputs; skipped for local files.
• references/per-fix-loop.md: per-fix loop, interactive confirmations, narrow type-check plus baseline compare, retry/skip/abort. Loaded by main at the Phase 5 loop.
• references/needs-input-triage.md: NEEDS-INPUT settle, per-item triage, Phase 8 fix plus verify, preflight before GitHub replies. Loaded by main in Phase 8 step 1, only when the count is nonzero.

One reference is not bundled here: ${CLAUDE_SKILL_DIR}/../review-pr/references/repo-map.md holds the repo_map_files / repo_map_exports shell, the one copy this skill shares with /review-pr. Loaded by main in Phase 1 when packages/ or apps/ exists.

Usage

/fix-pr-review https://github.com/owner/repo/pull/123
/fix-pr-review https://github.com/owner/repo/pull/123#pullrequestreview-4089716169
/fix-pr-review https://github.com/owner/repo/pull/123#discussion_r3064352825
/fix-pr-review ./review.md            # local output from /review-pr
/fix-pr-review /tmp/review-pr-123-findings.md  # local review export
/fix-pr-review                         # no arg → ask user to paste

Optional flags:

• --dry-run: stop after plan display, don't execute
• --interactive: ask before applying each FIX item
• --all-nitpicks: full-triage nitpicks instead of default-dismiss

Who gets triaged

Every reviewer's comments go through the same triage: CodeRabbit, humans, and other bots alike. The "CodeRabbit" framing throughout reflects the most common use case.

Phase 1: Prereqs + input detection + branch safety (main)

Prereq checks

command -v gh >/dev/null 2>&1 || { echo "Install gh CLI: https://cli.github.com"; exit 1; }
git rev-parse --is-inside-work-tree >/dev/null 2>&1 || { echo "Not inside a git repo. cd into your clone first."; exit 1; }
gh auth status 2>&1 | grep -q "Logged in" || { echo "Run 'gh auth login' first"; exit 1; }

Detect input type from the argument

• Contains #pullrequestreview-<id> → review URL
• Contains #discussion_r<id> → discussion URL (single comment + thread)
• Starts with ./ / / / ends with .md → local file
• Matches https://github.com/<owner>/<repo>/pull/<num> with no fragment → PR URL
• Empty → ask user to paste content or provide URL

Record execution authorization

Set EXECUTION_AUTHORIZED=true when the original request explicitly invokes /fix-pr-review or imperatively asks to fix, address, apply, handle, or resolve review findings. A bare URL or a read-only request to inspect, explain, review, or triage sets it to false. Preserve the matching request text as execution_authorization_evidence; Phase 4 may replace it with an explicit execution choice, and Phase 7 passes the final evidence to preflight-mutations.

Ensure correct repo + branch (GitHub inputs only)

Parse owner, repo, and number from the URL and land on the PR branch per ${CLAUDE_SKILL_DIR}/references/branch-safety.md. Load it now. If the tree is mid-merge, mid-rebase, or mid-cherry-pick, stop and hand conflict resolution to resolving-merge-conflicts before touching anything.

Auto-stash uncommitted work (branch safety)

This step runs for every input type, GitHub or local file. Fixes never land on top of unstashed WIP.

git status --porcelain

If non-empty, use AskUserQuestion:

Question: header: "Stash" text: "Uncommitted changes detected. Auto-stash before applying fixes? Contents will be restored at the end." options: - label: "Auto-stash" description: "Stash changes now. They'll be restored when the run completes" - label: "Abort" description: "Stop. I'll commit or stash my work manually first"

On "Auto-stash", run git stash push -u -m "fix-pr-review auto-stash $(date +%s)". Only on success, record STASH_OID=$(git rev-parse -q --verify refs/stash) and set STASH_PUSHED=true, then confirm git status --porcelain is empty. A failed push records nothing and aborts the run instead of marking a stash that was never created. If the run aborts, the user can find the work in git stash list as fix-pr-review auto-stash <timestamp>. On "Abort", print "Commit or stash your uncommitted work first." and exit.

Stash-restore guard. Every restore in this run, here and at every early exit, runs only when STASH_PUSHED=true: apply the recorded OID by OID only when the top of stack equals it (git stash apply "$STASH_OID", never a bare pop), drop only after a clean apply with the top still equal, and on any mismatch leave every entry untouched, record stash_restored: foreign-top, print stash_restored: foreign-top, stash left untouched, and continue without applying or dropping.

Compute the merge base (for already-fixed detection later)

BASE_SHA=$(git merge-base "origin/$(gh pr view <url> --json baseRefName -q .baseRefName)" HEAD)

Stash as BASE_SHA for use in Phase 3's already-fixed checks.

Pre-fix type-check baseline (what the narrow type-check compares against)

Run one baseline type-check before Phase 5, capture the set of files already failing:

bun turbo run check-types 2>&1 | tee /tmp/fix-pr-review-baseline-$$.log

Define one parser for the whole run. For each diagnostic, key its file and build the identity <diagnostic code> + <normalized message> after removing file position, line, and column data; retain duplicate identities as a multiset. Parse the Phase 1 output into baseline_errors[path] with this parser. Phase 5 and every Phase 8 baseline path must use the same parser and identity normalization. If type-check tooling is missing (no turbo, no tsc), skip the baseline and mark the narrow type-check as skipped for all fixes.

Compute shared-package repo map (for reusability-aware classification)

Inventory shared packages and apps so the Phase 3 classifier can cross-check comments about reuse and extraction against what already exists. Scan both packages/ and apps/. Cross-app helper duplication, for example apps/backend/src/modules/v1/feature-a/helpers.ts versus feature-b/helpers.ts, is common in NestJS-style monorepos and stays invisible to a packages-only scan.

Load ${CLAUDE_SKILL_DIR}/../review-pr/references/repo-map.md and run its Local mode block, the one copy of this shell, shared with /review-pr. It carries the bash -c wrapping the globs need to survive zsh, and caps each half at 500 lines with the truncation marked. Load it when packages/ or apps/ exists; when neither does there is nothing to run and the fallback below applies.

Stash both outputs as repo_map_files and repo_map_exports for the Phase 3 subagent prompt. If neither packages/ nor apps/ exists (non-monorepo), set both to N/A (not a monorepo) and flag IS_MONOREPO=false. The classifier prompt uses this to reroute greps to src/ and the repo root.

Phase 2: Fetch review data (main)

Dual-path input for /review-pr findings

/review-pr now posts findings as individual inline comments, one per finding on a specific code line. These create standard PullRequestReviewThreads on GitHub, identical to CodeRabbit threads. The existing GraphQL fetch below handles them with zero special parsing.

Manually exported or legacy /review-pr findings files use the existing local-file path. Phase 7 skips GitHub operations for those inputs.

Fetch from GitHub (PR URL / review URL / discussion URL)

Phase 1 detected exactly one of these three input types. Load ${CLAUDE_SKILL_DIR}/references/fetch-review-data.md now and run only that type's section. It holds the paginated GraphQL reviewThreads query and its isResolved filter, the /pulls/<num>/reviews/<review_id> and /pulls/comments/<comment_id> REST endpoints, the CodeRabbit review-body anatomy, and the parse for the 🤖 Prompt for all review comments with AI agents block, the only place nitpicks appear, since they never get inline threads.

A fetch that errors surfaces the error and exits, so triage never runs on a partial comment set. That covers a GraphQL rate limit, a 404, a private repo, and a per-page failure inside the pagination loop. For 404, print Couldn't access PR. Check repo access and run 'gh auth refresh -s repo'.

For local files (./review.md, /tmp/review-pr-*-findings.md, etc.)

Parse the /review-pr output format. Extract findings from the ## Findings section, preserving Severity / File / Category / Issue / Why it matters / Suggested fix plus Rule-class / Enclosing-symbol / Inverse risk / Class-sites when present. /review-pr emits these per finding and dropping them forces this skill to re-derive work the reviewer already did.

Seed, don't re-derive: when Inverse risk: is present, seed STEP 5's inverse_risk from it and VERIFY it against the code (confirm the named failure mode is real and still applies) rather than deriving a new one from scratch. When Class-sites: <A>/<N> is present, seed STEP 1.5's class_completeness with those N sites and verify the count against your own search. Re-run the search only to catch sites the reviewer missed, not to rebuild the list. Rule-class and Enclosing-symbol seed the class sweep's signature. If a seeded value contradicts what you read in the code, the code wins. Record the discrepancy in the field.

Severity mapping: /review-pr uses Critical | Serious | Moderate | Minor while CodeRabbit uses Critical | Major | Minor | Refactor | Nitpick. Both are valid. Normalize to the internal Comment schema which accepts either convention. Map for triage priority: Critical = highest, Serious/Major = high, Moderate = medium, Minor/Refactor = low, Nitpick = default-dismiss.

Normalize to internal Comment list

Every input path ends here. The Comment schema, the exact field names Phases 3-8 read, is defined in references/fetch-review-data.md. Load that file now if you took the local-file path and have not read it yet.

Short-circuit cases

• Empty list (all threads resolved, local file has no findings): print Nothing to triage. No unresolved comments found. → restore the stash under the guard → exit 0.
• Only nitpicks remain AND --all-nitpicks not set: print Only nitpicks found (N). Pass --all-nitpicks to triage them, or ignore. → restore the stash under the guard → exit 0.

Phase 3: Triage subagent (general-purpose)

Load review suppressions (main agent, before dispatch)

Before dispatching the subagent, load .claude/review-suppressions.yml at the base revision, never the worktree: a checked-out PR must not suppress its own triage. For GitHub inputs, read it at the pinned base OID: git show "$PINNED_BASE_OID:.claude/review-suppressions.yml" locally (fetch origin once when the objects are absent), or gh api repos/<owner>/<repo>/contents/.claude/review-suppressions.yml?ref=$PINNED_BASE_OID in cross-repo mode. For local-file inputs without a PR, HEAD itself may be the change under triage. Resolve the trusted base as the one candidate merge-base that contains every other candidate's: collect one base per mainline, preferring each remote alias over its local counterpart since a local mainline may itself be the change under triage, and take the base only when it is the unique one containing all the rest. Ties, incomparable pairs, and empty sets disable suppressions.

BASES=$(for remote in origin/main origin/master origin/develop; do
  bare=${remote#origin/}
  if git rev-parse --verify -q "$remote" >/dev/null 2>&1; then ref=$remote
  elif git rev-parse --verify -q "$bare" >/dev/null 2>&1; then ref=$bare
  else continue
  fi
  git merge-base HEAD "$ref" 2>/dev/null || continue
done | sort -u)
RESULT=""
if [ -n "$BASES" ]; then RESULT=$(printf '%s\n' "$BASES" | while IFS= read -r b; do
  [ -n "$b" ] || continue
  BAD=$(printf '%s\n' "$BASES" | grep -vx "$b" | while IFS= read -r o; do
    git merge-base --is-ancestor "$o" "$b" 2>/dev/null || printf 'bad\n'
  done)
  [ -z "$BAD" ] && printf 'WINNER %s\n' "$b"
done); fi

Read the file at the winning commit and log the source. Count winners in a way that stays successful on empty input, since grep -c exits 1 when it counts zero:

WINNERS=$(printf '%s\n' "$RESULT" | grep -c WINNER || true)

Take it only when WINNERS equals 1 and the winner is not HEAD itself: a winner equal to HEAD means no older base exists to trust. Otherwise set SUPPRESSIONS = "": triaging without a policy adds noise, trusting the reviewed change hides findings. An empty candidate set emits no winner lines at all. When the base has no such file, set SUPPRESSIONS = "" and log that a PR-added file was ignored.

Pass loaded suppressions into the subagent prompt as a ## Review suppressions section (same approach as CLAUDE.md content, PR diff, and repo maps; main agent fetches, subagent receives as context).

Dispatch

Dispatch one general-purpose subagent with Read, Grep, and Bash tools. The triage plan comes from this subagent alone. If it fails outright, abort the run and say so. Classifying inline skips the grounding and class-sweep passes the whole plan is built on.

Important: The Bash allowlist (git log/diff/blame/show/merge-base/rev-parse, grep, rg) is a prompt-level instruction. Claude Code's Agent tool doesn't sandbox Bash per-command. The subagent is trusted not to run other commands, not mechanically prevented from doing so.

Prompt template

The whole prompt is references/triage-prompt.md. Read it now, substitute its <...> placeholders (<SKILL_DIR> = this skill's absolute directory, plus the Phase 1 context values, the git diff <BASE_SHA>...HEAD output, the repo maps, SUPPRESSIONS, and the Phase 2 Comment[] array), and pass the result to the subagent VERBATIM. It is the string the subagent runs, not instructions for you to follow, summarise, or restate inline.

Its STEP 4 sends the subagent to references/triage-rubric.md on its own. You do not read that file; the R-Rubric Summary table above is the main-agent view. What comes back is what Phase 4 validates: per FIX the prompt emits fix_plan, change_class, test_scenario, inverse_risk: and class_completeness:; reusability_context: rides on every item, not just FIX.

Phase 4: Plan execution gate (main)

Validates against the R-rubric: summary table above, full detail in references/triage-rubric.md. Required fields per classification are defined there.

Plan validation (before display)

Before anything is shown to the user, mechanically validate the classifier's output:

• Every DISMISS with rubric: R5 MUST have non-empty claude_md_quote.
• Every DISMISS with rubric: R4 MUST have non-empty prior_commit_sha.
• Every DISAGREE MUST have non-empty disagree_rationale (and it MUST NOT be a pure style preference; check for keywords like "prefer", "cleaner", "nicer" without a concrete counter-argument).
• Every FIX MUST have fix_plan length >= 30 characters.
• Every FIX MUST have change_class set to exactly hardening or logic-change (the calibration the classifier applied is in references/triage-rubric.md; this check is purely the literal value).
• Every FIX MUST have non-empty test_scenario. For change_class: hardening, the value MUST be exactly smoke test, happy path unchanged. For change_class: logic-change, the value MUST be a 1-sentence concrete repro (not just "verify it works").
• Every FIX MUST have non-empty inverse_risk that either names a specific failure mode or is exactly none, pure addition. Hedges fail validation: an empty value, or anything of the shape "could have issues" / "minor risk" / "some risk" / "possible regression" / "none" on its own. A named failure mode says what breaks, where. Phase 5.5 consumes this field; an unnamed risk is unverifiable there.
• Every FIX MUST have a class_completeness block with a non-empty verdict starting with either COMPLETE or INCOMPLETE. INCOMPLETE MUST name the excluded sites and give a reason for each. An INCOMPLETE verdict with no per-site reason fails validation.
• Every NEEDS-INPUT with rubric: R10 MUST have a why_unclear carrying all three of its current:, proposed:, and intent evidence: parts, each non-empty. The shape itself lives in references/triage-rubric.md. found nothing is a valid evidence value; an empty one is not.
• Every item MUST have non-empty grounding_a and grounding_b.
• Every item MUST carry a reusability_context field, even when it is just { flagged: false }. That holds for every classification, NEEDS-INPUT included. Phase 7's reply validator branches on it, so a missing field silently disables the reusability gate on that reply.

On validation failure: re-dispatch the classifier with the specific missing fields listed. Max 1 retry. Second failure → abort with the validation errors printed.

Plan display

Print the plan with a header:

# Fix Plan, PR #<num>: <title>
# <N> findings triaged: <F> fix, <D> dismiss, <E> defer, <G> disagree, <I> needs-input, <n> nitpicks

Highlight DISMISS-by-CLAUDE.md prominently

If any DISMISS has rubric: R5, print a separate highlighted section BEFORE the main plan:

## ⚠ Dismissed because they contradict CLAUDE.md: review and override if any are exceptions

[D<n>] <file:line>: <comment ask>
       CLAUDE.md rule: "<verbatim quote>"
       Reply will be: "<reply>"
       If this rule has a legitimate exception in this case, select D<n>
       in the contested-item confirmation to change it to FIX or NEEDS-INPUT.

Contested-item confirmation (multiSelect)

Contested items are the ones that will post a reply and resolve a thread WITHOUT any code change: every DISMISS, DEFER, and DISAGREE. A wrong classification here silently closes a reviewer's conversation. Confirm the triage before Phase 5/7 can act on it.

Skip this step if there are zero contested items. Otherwise, use AskUserQuestion:

Question: header: "Triage" text: " item(s) will get a reply + thread resolution with no code change. Select any to RECLASSIFY. Unselected items proceed as planned." options: [one option per DISMISS/DEFER/DISAGREE item: "[] file:line: (), <reason, first ~60 chars>"] multiSelect: true

If contested items exceed the option limit, split into multiple multiSelect questions. DISMISS first (the most costly to get wrong).

For each selected item, use a follow-up AskUserQuestion:

Question: header: "Item " text: "file:line, currently : . Reclassify as?" options: - label: "FIX (Recommended)" description: "Treat as a real issue and add it to the FIX list with a fix plan" - label: "NEEDS-INPUT" description: "Park it and post nothing; it surfaces in the final report" - label: "Keep as-is" description: "Keep the original classification, since this was selected by mistake"

On "FIX": re-dispatch the classifier scoped to just this item to produce the full FIX field set: fix_plan, change_class, test_scenario, inverse_risk, class_completeness (class sweep included; a reclassified item has never been swept), and reusability_context (carry the contested item's own value through unless the sweep changed it). Then re-run plan validation on the changed item. Producing a partial field set here fails validation and burns the single retry. On "NEEDS-INPUT": move to NEEDS-INPUT with why_unclear: "user contested the <classification> classification", carrying the item's reusability_context through unchanged. On "Keep as-is": no change. On "Other": treat the freeform text as the reclassification instruction.

Nothing is posted or resolved during this step. Phase 7 remains the only place GitHub is touched, and it acts only on items that survived this confirmation. Resolving a thread is irreversible noise in the reviewer's conversation: a DISMISS, DEFER, or DISAGREE that skipped this gate stays open and unanswered until it has been through it.

Execution

If --dry-run: print the plan, print dry run, not executing, restore the stash under the guard, exit 0.

If EXECUTION_AUTHORIZED=true, proceed directly to Phase 5. The invocation already authorizes execution of every validated FIX item. If --interactive was set, ask for per-item confirmation in Phase 5. It does not add a plan-level confirmation.

Otherwise, use AskUserQuestion:

Question: header: "Execute" text: "The request did not explicitly authorize edits. Execute the validated plan? fixes, dismissals, deferrals, disagrees, needs-input." options: - label: "Execute plan (Recommended)" description: "Apply all FIX items in dependency order" - label: "Cancel" description: "Leave the worktree unchanged and restore any stash"

preedit_snapshot[path] = <full file content from Read>

fix_status[idx] = ok | retried_ok | inconclusive | skipped | aborted | type_check_skipped
                | reverted_inverse_risk | inverse_risk_applied | partial | restored_failed
landed_fix_statuses = {ok, retried_ok, inconclusive, type_check_skipped}

For each fix below, verify against the working tree, not against the fix plan's claims.

1. CLASS COMPLETENESS: every site the class sweep marked `affected` in
   `class_completeness.sites` must actually be changed. A fix that landed on 3 of 4
   sites is INCOMPLETE, not done.
   (Sites the plan's `verdict` deliberately excluded are not counted as unfixed.)
2. INVERSE RISK: the named failure mode must NOT be present in the applied code.
3. NEW SIBLINGS: did the fix itself introduce a new instance of the pattern it fixes,
   or a new branch (error state, empty state, early return) that its siblings have but
   this one lacks?

Report per fix, nothing else:
  fix: <idx>
  class_complete: yes | no, <unfixed site if no>
  inverse_risk_present: no | yes, <file:line + one sentence>
  new_siblings: none | <file:line + one sentence>

Evidence rules differ per check. "I lack evidence" is not an answer for check 1:
  - Check 1 is decided MECHANICALLY by `git diff HEAD`. Each affected site either
    appears in the diff or it does not; there is no undecidable state. Report `no`
    with the unfixed site whenever a site is absent from the diff.
  - Checks 2 and 3 default to `no` / `none` unless you can cite a concrete file:line
    in the applied code. Do not speculate, do not re-review the PR, do not report
  style issues.

self_heal_iter = 0
done_remaining = []
while self_heal_iter < 2:
    findings = run_parallel_review(diff="git diff HEAD")
    blockers = [f for f in findings if f.severity in ("Critical", "Serious")]
    if not blockers:
        break
    # Dispatch fresh subagent per blocker with the finding + the diff
    for f in blockers:
        apply_subagent_corrective_edits(f)
    run_fix_ts_errors()
    self_heal_iter += 1

# After loop: record whatever blockers remain (if any)
done_remaining = blockers

gh_status[idx] = {
  reply_state: landed | verified-existing | confirmed-absent | reconcile-required | skipped | not-applicable,
  reply_err:   <error message if any>,
  resolve_state: resolved | already-resolved | confirmed-open | reconcile-required | skipped | not-applicable,
  resolve_err: <error message if any>
}

if [ "$(git rev-parse -q --verify refs/stash)" = "$STASH_OID" ]; then
  if git stash apply "$STASH_OID"; then
    [ "$(git rev-parse -q --verify refs/stash)" = "$STASH_OID" ] && git stash drop || echo "STASH_CHANGED_MID_RESTORE"
  elif [ -n "$(git diff --name-only --diff-filter=U)" ]; then
    echo "STASH_CONFLICT"
  else
    echo "STASH_APPLY_FAILED"
  fi
else
  echo "STASH_TOP_MISMATCH"
fi

  - pattern: "<normalized pattern from finding: key phrase, not full text>"
    category: "<finding category if available>"
    file: "<finding's file path. Include only if the rationale is specific to one file, omit for generic patterns>"
    reason: "<dismiss/disagree rationale from triage>"
    added: <today's date YYYY-MM-DD>

suppressions:
  - pattern: ...

git push --force-with-lease="<exact-ref>:<expected-sha-or-empty>" "<preflighted-remote>" "<frozen-local-sha>:<exact-ref>"
git ls-remote --heads "<preflighted-remote>" "<exact-ref>"