dev-review-pr

Check GitHub Actions status, review feedback from automated reviewers (Code Rabbit, Claude), implement or respond to feedback, and merge the PR when all feedback is resolved. > **MANDATORY: Run `/dev-local-review` before every commit and push.** > This applies to step 5 (commit and push fixes) and any other commit made > during this workflow. Do NOT push code that has not passed a local review. > Skipping this step wastes a GitHub review round on issues that could have > been caught locally for free. The user provides: - **PR number**: the pull request number to review (optional - infers from current branch) ## Workflow ### 1. Check GitHub Actions status Use `gh pr checks <pr-number>` to see if automated reviews are complete. **Important:** This command returns exit code 8 if any checks are pending, and outputs them to stderr. Handle both stdout and stderr to capture all check statuses. Output format: `check-name status duration url` **If any checks are still running (pending):** - Report which checks are pending - Exit with message: "GitHub Actions still running. Run this skill again when checks complete." - DO NOT sleep or wait — let the user run the skill again later **If checks failed:** - Report which checks failed - Show the failure URL from the output - **If markdown linting failed**, offer to run it locally and fix issues (see step 1.5) - Otherwise exit and ask user to investigate **If all checks passed:** - Proceed to step 2 (fetch review comments) ### 1.5. Fix markdown linting failures (if needed) If the "Markdown Lint" check failed: 1. **Run locally to see errors:** ```bash npx markdownlint-cli2 "**/*.md" ``` 2. **Attempt auto-fix:** ```bash npx markdownlint-cli2 --fix "**/*.md" ``` 3. **Manually fix remaining errors** (especially MD040 - missing language tags) 4. **Verify all errors resolved:** ```bash npx markdownlint-cli2 "**/*.md" ``` 5. **Commit and push fixes:** ```bash git add <fixed-files> git commit -m "Fix markdown linting errors Co-Authored-By: Claude <noreply@anthropic.com>" git push ``` 6. **Exit and wait for checks to re-run** — user should invoke `/dev-review-pr` again after checks pass ### 1.7. Check CodeRabbit review status CodeRabbit is configured to **auto-pause after the first reviewed commit** (`reviews.auto_review.auto_pause_after_reviewed_commits: 1` in `.coderabbit.yaml`). This means after every push, CodeRabbit reviews once and then pauses. The "Reviews paused" banner in the PR comment is **normal and expected** — it does NOT mean something went wrong or that the review is missing. **Key insight:** CodeRabbit completes its review *before* pausing. The review comments and review body are already available. **How to check:** Look at the CodeRabbit check status from step 1: - If CodeRabbit shows `pass` — the review is done. **Proceed directly to step 2** to fetch the review comments. Ignore the "Reviews paused" banner — it just means future commits won't be auto-reviewed. - If CodeRabbit shows `pending` or no CodeRabbit check yet — the review hasn't started or is still running. Exit and ask the user to re-run later. **After implementing feedback and pushing fixes (step 5):** Since auto-pause is on, CodeRabbit will NOT automatically review the new commit. Post a single comment to request a fresh review: ```bash gh pr comment <pr-number> --body "@coderabbitai review" ``` Do NOT post `@coderabbitai resume` — we *want* it to stay paused after each review to avoid review thrashing from rapid commits. **If CodeRabbit shows "Currently processing new changes":** - The review is actively running. Exit and tell the user to re-run later. - Do NOT fetch review comments yet — they may be incomplete. #### 1.7a. Detect clean re-review (no actionable comments) After CodeRabbit re-reviews a fix commit, it sometimes finds nothing new to flag. When this happens, CodeRabbit does **not** post an `APPROVED` review. Instead, it posts an **issue comment** (not a review comment) containing: > No actionable comments were generated in the recent review. The review state from step 6 may still show `CHANGES_REQUESTED` or `COMMENTED` from a previous round — that's stale and misleading. **How to detect:** ```bash # Get the latest "no actionable comments" timestamp NO_ACTION_TS=$(gh api repos/{owner}/{repo}/issues/{pr-number}/comments --paginate \ | jq -r -s '(add // []) | map(select(.user.login == "coderabbitai[bot]" and (.body | contains("No actionable comments")))) | sort_by(.created_at) | last | .created_at // empty') # Get the latest CHANGES_REQUESTED or COMMENTED review timestamp LAST_REVIEW_TS=$(gh api repos/{owner}/{repo}/pulls/{pr-number}/reviews --paginate \ | jq -r -s '(add // []) | map(select(.user.login == "coderabbitai[bot]" and (.state == "CHANGES_REQUESTED" or .state == "COMMENTED"))) | sort_by(.submitted_at) | last | .submitted_at // empty') # Compare timestamps (lexicographic works for ISO 8601 dates) if [[ -z "$NO_ACTION_TS" ]]; then echo "No 'no actionable comments' signal found" elif [[ -z "$LAST_REVIEW_TS" || "$NO_ACTION_TS" > "$LAST_REVIEW_TS" ]]; then echo "Clean re-review detected — proceeding to step 6b" else echo "Review feedback still pending" fi ``` If the "no actionable comments" comment is **newer** than the last review with feedback, CodeRabbit is satisfied. **When detected, skip straight to step 6b** (pre-merge branch update and resolve). Do not loop back to step 2 looking for feedback that does not exist. ### 2. Fetch review comments **Important:** Inline review comments (the "tasks" users see in the GitHub UI) are NOT returned by `gh pr view --json comments`. You must use the API directly: ```bash gh api repos/{owner}/{repo}/pulls/{pr-number}/comments ``` This returns an array of review comment objects with: - `path` — file path - `line` / `start_line` — line numbers - `body` — comment body (markdown, may include severity, suggestions) - `html_url` — link to view the conversation - `user.login` — reviewer (e.g., "coderabbitai[bot]", "claude-code[bot]") **CodeRabbit comment format:** - Body starts with severity: `_⚠️ Potential issue_ | _🟠 Major_`, `_🟡 Minor_`, or other markers - Includes suggested fixes in `<details><summary>Suggested fix</summary>` blocks - May include committable suggestions between `<!-- suggestion_start -->` markers - **Important:** CodeRabbit may leave nitpick/style comments alongside major issues—fetch ALL comments, not just the first few Parse and categorize feedback by: - Severity (🟠 Major, 🟡 Minor, nitpick/style) - File and line number - Reviewer (CodeRabbit, Claude, human) **Present ALL comments to the user** sorted by severity (Major → Minor → Nitpick) so security/critical issues are addressed first. #### 2a. Correctly identify which comments belong to the latest review round **CRITICAL — this is the #1 cause of missed feedback.** Do NOT filter comments by `original_commit_id` to find "new" comments. CodeRabbit posts comments against various commit SHAs depending on diff positioning, and a comment on commit A may actually be new feedback triggered by commit B's review. **The correct approach: use the review ID, not the commit ID.** 1. First, fetch all reviews and find the latest CodeRabbit review: ```bash REVIEW_ID=$(gh api --paginate repos/{owner}/{repo}/pulls/{pr-number}/reviews \ | jq -s 'add | map(select(.user.login == "coderabbitai[bot]")) | sort_by(.submitted_at) | last | .id') echo "Latest CodeRabbit review: $REVIEW_ID" ``` 2. Then, fetch comments and match them to reviews by `pull_request_review_id`: ```bash gh api --paginate repos/{owner}/{repo}/pulls/{pr-number}/comments \ | jq -s --argjson rid "$REVIEW_ID" \ 'add | map(select(.pull_request_review_id == $rid)) | map({id, path, line, body: .body[:200]})' ``` 3. Comments from the latest review are the **new** comments for this round. Comments from earlier reviews that were NOT auto-resolved are **carried forward** and still need action. **Why `original_commit_id` filtering is wrong:** - CodeRabbit may post a comment against commit X even when reviewing commit Y (if the code at that location hasn't changed between commits) - A single review round can produce comments with different `original_commit_id` values - Filtering by the latest commit SHA misses comments CodeRabbit posted against older commit positions — this causes the agent to report "only 1 new comment" when there are actually 7 **Alternative quick approach:** If you don't want to join on review IDs, simply fetch ALL CodeRabbit comments, exclude those that have a human reply dismissing them (user said "out of scope"), and present the rest grouped by file. Let the user decide which are new vs. already handled. #### 2b. Check review bodies for duplicate and nitpick comments **Critical:** CodeRabbit embeds additional feedback directly in **review bodies** — not as separate inline comment threads. These appear in collapsible sections titled "Duplicate comments" and "Nitpick comments" within the review. They do NOT create their own threads, so they will be missed if you only fetch inline comments. Fetch review bodies from CodeRabbit: ```bash gh api --paginate repos/{owner}/{repo}/pulls/{pr-number}/reviews \ | jq -s 'add | map(select(.user.login == "coderabbitai[bot]")) | sort_by(.submitted_at) | map({id: .id, state: .state, body: .body, date: .submitted_at})' ``` **Look for these sections in the review body:** - `♻️ Duplicate comments` — Issues raised previously that CodeRabbit still considers unresolved after re-review. These are NOT resolved just because the original thread was addressed — CodeRabbit is saying the fix was incomplete or the issue persists. **Treat these as active feedback.** - `🧹 Nitpick comments` — Low-severity suggestions that didn't warrant a blocking review thread. Still present them to the user. **For each duplicate/nitpick comment found:** - Extract the file path, line numbers, severity, and description - Include the suggested patch if present - Add it to the feedback summary alongside inline comments - **Do not dismiss duplicate comments** — they indicate CodeRabbit re-reviewed and still found the issue. The fix from the previous round was likely incomplete. **When a review body contains duplicate comments but no new inline threads:** - The review state will be `COMMENTED` (not `CHANGES_REQUESTED`) - The inline comment threads from the previous round may be auto-resolved - But the duplicate section means there is still work to do — do NOT skip to the merge step just because all threads show as resolved ### 3. Present feedback summary and classify into themes **Sort comments by severity:** Major → Minor → Nitpick/Style, so critical issues (especially security) are addressed first. Show the user the raw feedback list, then **group comments into themes**. A theme is a class of issue that may have multiple instances across the codebase. Examples: | Raw comments | Theme | |---|---| | "Docs say circles but widget draws rectangles" × 3 files | Doc/code mismatch: shape description | | "No error check after fclose" in writer A | Unchecked I/O pattern | | "No error check after fclose" in writer B | (same theme) | | "Test only checks return value, not side effects" × 4 tests | Weak test assertions | | "Division by zero if rect width is 0" | Missing zero-guard pattern | | "fabsf() should be SDL_fabsf()" × 5 files | Bare C stdlib calls (portability) | ```text ## PR Review Feedback Summary ### Pending conversations: X (grouped into Y themes) **Theme 1: [description] (N comments, severity)** - file.c:123 — [specific instance] - file.c:456 — [specific instance] - README.md:78 — [specific instance] **Theme 2: [description] (N comments, severity)** - ... ### Resolved conversations: Z ``` If no pending conversations, skip to step 6. ### 4. Build a verification plan (CRITICAL — do not skip) **This is the step that prevents 10+ round feedback loops.** Before touching any code, build a comprehensive plan for each theme. The goal: fix every instance in one pass so CodeRabbit sees zero duplicates on re-review. #### 4a. For each theme, run an impact analysis For each theme, **before writing any fix**, spawn an analysis agent (`subagent_type: "Explore"`) to answer: 1. **Where else does this pattern appear?** Search all files changed in this PR for the same class of issue — not just the line CodeRabbit flagged. Use grep/glob to find every instance. Examples of what "same pattern" means: - CodeRabbit says "docs say circles" → grep for "circle" in all `.md`, `.h`, and `.c` files touched by this PR - CodeRabbit says "unchecked fclose" → grep for every `fclose` call in the file and check if any are unchecked - CodeRabbit says "test only checks return value" → check ALL similar tests for the same weakness, not just the one cited - CodeRabbit says "division by zero if width is 0" → find ALL divisions by width, height, or any user-derived value in the function - CodeRabbit says "fabsf() should be SDL_fabsf()" → grep for ALL bare C stdlib calls (`fabsf`, `sinf`, `cosf`, `sqrtf`, `memset`, `memcpy`, `strcmp`, `strlen`, `malloc`, `free`) across every `.c` and `.h` file in the PR — this is a critical cross-platform portability issue 2. **What documentation describes this code?** Identify every README, API doc, header comment, and inline comment that references the behavior being changed. These MUST be updated when the code changes. 3. **What tests cover this code?** Identify existing tests. If the fix changes behavior, those tests must be updated. If no test exists, one must be written. 4. **Why did we get this wrong?** Understanding the root cause prevents adjacent bugs: - Copy-paste without adaptation → check all copies - Misunderstanding an API → check all uses of that API - Missing a case in a switch/if chain → check the full chain - Generated code from a template → check all generated instances #### 4b. Write the verification plan For each theme, produce a checklist: ```text Theme: "Radio button shape — docs say circles, code draws rectangles" Root cause: Copy-pasted checkbox description without adapting for radio buttons Code fixes: [ ] common/ui/forge_ui.h:234 — change "circle" to "rectangle" in header doc [ ] common/ui/README.md:89 — update radio button description [ ] lessons/ui/15-dev-ui/README.md:156 — fix shape description [ ] OR: actually make radio buttons round in forge_ui_ctx_radio() Doc sync: [ ] Verify all 3 doc locations match after fix [ ] Check if any diagram shows radio buttons (update if so) Tests: [ ] Add test asserting radio button geometry is rectangular (or round) [ ] Verify existing radio button tests still pass Grep verification: [ ] grep -r "circle" across all files in this PR — zero false matches remain ``` **Present the full plan to the user** before executing. The user may spot things the analysis missed or may prefer a different approach (e.g. "just make it round" vs "fix all the docs"). #### 4c. Execute the plan with a team For each theme that the user approves, spawn agents **in parallel** to handle the three concerns simultaneously: 1. **Code agent** (`subagent_type: "coder"`, `run_in_background: true`) — Fix the code issue across ALL instances found in the impact analysis. Not just the line CodeRabbit pointed at — every instance of the pattern. 2. **Test agent** (`subagent_type: "tester"`, `run_in_background: true`) — Write or update tests for the fix. For library changes in `common/`, this is mandatory. The test should: - Verify the fix works (positive case) - Verify edge cases (zero values, NULL, overflow) - Verify the old bug does not regress 3. **Doc agent** (`subagent_type: "coder"`, `run_in_background: true`) — Update ALL documentation that describes the changed behavior: - README sections ("What you'll learn", "Key concepts", code examples) - API reference docs in `common/*/README.md` - Header comments and inline comments in `.h` files - Diagram descriptions if applicable **All three agents work from the same verification plan.** Give each agent the full plan so they understand the scope, but assign them their specific