Skip to content

fix(skill): address Claude review comments in /review skill#543

Merged
carlos-alm merged 4 commits into
mainfrom
fix/review-skill-claude-comments
Mar 20, 2026
Merged

fix(skill): address Claude review comments in /review skill#543
carlos-alm merged 4 commits into
mainfrom
fix/review-skill-claude-comments

Conversation

@carlos-alm
Copy link
Copy Markdown
Contributor

Summary

  • Updated the /review skill to explicitly require addressing Claude's review comments, not just Greptile's
  • Added distinct reply mechanisms for each comment type (inline review comments, top-level review bodies, issue comments)
  • Added a reviewer checklist step to verify no reviewer was missed before moving on
  • Reinforced in the rules section that ALL reviewers must receive replies

Test plan

  • Run /review on a PR that has both Claude and Greptile comments and verify both are addressed

@claude
Copy link
Copy Markdown

claude Bot commented Mar 19, 2026

Claude finished @carlos-alm's task —— View job


Code Review in Progress

Conducting comprehensive PR review following the 5-phase analysis process:

Todo List:

  • Phase 1: Root Cause Analysis - Understand the problem being solved
  • Phase 2: Configuration Change Detection - Check for high-risk changes
  • Phase 3: Test Integrity - Watch for red flags in testing
  • Phase 4: Code Quality - Review implementation details
  • Phase 5: Backlog Compliance - Evaluate against BACKLOG.md criteria
  • Final Assessment: Provide rating and recommendation

Root Cause Analysis

Problem Being Solved: The /review skill was not explicitly addressing Claude's review comments—only Greptile's. This created a situation where Claude could leave feedback that went unaddressed, leading to incomplete PR reviews.

Why This Approach: The changes clarify that ALL reviewers (Claude, Greptile, and humans) must have their comments addressed. The fix adds explicit language about Claude's review patterns and provides distinct reply mechanisms for different comment types.

Risk Assessment: Low risk. This is a documentation enhancement that improves existing workflow rather than changing core functionality.

Backlog Compliance

  • Zero-dep: ✓ — No new runtime dependencies, pure documentation change
  • Foundation-aligned: ✓ — Aligns with P8 (better tooling for development workflow) and P5 (embeddable-first via improved skill documentation)
  • Problem-fit: 3 — Useful for improving PR review completeness but doesn't directly address core AI navigation problems
  • Breaking: No — Purely additive documentation improvements
  • Tier: 1

Critical Concerns

None. This is a well-targeted documentation fix.

Changes Analysis

Positive Changes:

  1. Explicit multi-reviewer requirement: Section 2e header changed from "Address every comment" to "Address every comment from EVERY reviewer" with explicit mention of Claude, Greptile, and humans
  2. Comment source documentation: Added clarification that Claude's feedback appears across /reviews, /comments, and /issues endpoints, not just one location
  3. Distinct reply mechanisms: Proper API endpoints for different comment types (inline vs. review bodies vs. issue comments)
  4. Verification checklist: Added a concrete verification step to ensure no reviewer is missed before moving on
  5. Rules section reinforcement: Updated rules to emphasize "ALL reviewers" rather than generic "ALL comments"

Quality Indicators:

  • Changes are surgical and focused on the specific problem
  • Maintains existing workflow structure while clarifying expectations
  • Provides actionable commands for each comment type
  • Documentation follows existing patterns and tone

Final Recommendation

  • Rating: ⭐⭐⭐⭐☆ (4/5)
  • Action: APPROVE
  • Reasoning: This is a focused, well-executed documentation improvement that addresses a real gap in the review workflow. The changes are clear, actionable, and properly scoped. No technical issues or concerns. Minor deduction only because it's documentation rather than feature enhancement.

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Mar 19, 2026

Greptile Summary

This PR improves the /review skill by explicitly requiring that Claude's review comments are addressed (not just Greptile's), adding distinct reply mechanisms per comment type (inline, top-level review body, issue-style), and reinforcing the all-reviewers rule in the Rules section. It also adds a three-endpoint reviewer checklist in Step 2e and extends the conflict-resolution context step to include the /reviews endpoint.

Key changes:

  • Step 2b: /reviews endpoint added to the API calls used for conflict context-gathering
  • Step 2d: Code comments updated to clarify which endpoints Claude vs Greptile use
  • Step 2e: Three distinct gh api reply templates added (inline comments, top-level review bodies, issue-style comments); $'...' ANSI-C quoting used so \n is interpreted as a real newline before being passed to gh api -f
  • Reviewer checklist: queries all three endpoints for unique logins to prevent silently missing a reviewer
  • Rules section: "Address ALL comments" and "Always reply" bullets updated to name all reviewer types explicitly

The changes are consistent with the rest of the skill and the approaches used elsewhere in the document. No critical logic or syntax issues were found.

Confidence Score: 4/5

  • Safe to merge — changes are documentation-only improvements to a skill file with no executable code paths affected.
  • The diff is limited to a single Markdown skill file. The ANSI-C quoting fix ($'...') is syntactically correct for bash and properly handles newlines and the embedded single-quote in "Claude's". The three-endpoint reviewer checklist accurately mirrors the three fetch endpoints introduced in Step 2d. The reply-mechanism split (inline vs. top-level vs. issue-style) maps correctly to the GitHub API surface. No prior review threads are repeated, and no logic or syntax errors were introduced.
  • No files require special attention.

Important Files Changed

Filename Overview
.claude/skills/review/SKILL.md Skill documentation updated to explicitly require addressing Claude's review comments, add distinct reply mechanisms per comment type, and add a three-endpoint reviewer checklist. Changes are logically sound and consistent with the rest of the skill.

Sequence Diagram

sequenceDiagram
    participant A as Skill (Claude Agent)
    participant GH as GitHub API

    A->>GH: GET /pulls/<n>/comments (inline review comments)
    GH-->>A: [{id, user, body, path, line}]

    A->>GH: GET /pulls/<n>/reviews (top-level review bodies)
    GH-->>A: [{id, user, body, state}]

    A->>GH: GET /issues/<n>/comments (issue-style comments)
    GH-->>A: [{id, user, body}]

    Note over A: Build complete list of actionable items<br/>from ALL reviewers (Claude, Greptile, humans)

    loop For each inline comment (path + line)
        A->>GH: POST /pulls/<n>/comments/<id>/replies<br/>body="Fixed — <description>"
    end

    loop For each top-level review body (no path)
        A->>GH: POST /issues/<n>/comments<br/>body=$'Addressed Claude review:\n- <items>'
    end

    loop For each issue-style comment
        A->>GH: POST /issues/<n>/comments<br/>body="Addressed: <summary>"
    end

    Note over A: Reviewer checklist:<br/>query all 3 endpoints for unique logins,<br/>confirm every reviewer was addressed

    A->>GH: POST /issues/<n>/comments — @greptileai (if no positive reaction)
    A->>GH: POST /issues/<n>/comments — @claude (if Claude feedback addressed)
Loading

Last reviewed commit: "fix(skill): add /rev..."

Comment on lines +144 to +150
**Checklist before moving on:** After addressing all comments, verify you haven't missed a reviewer:
```bash
# List all unique reviewers who left comments
gh api repos/optave/codegraph/pulls/<number>/comments --paginate --jq '[.[].user.login] | unique | .[]'
gh api repos/optave/codegraph/pulls/<number>/reviews --paginate --jq '[.[].user.login] | unique | .[]'
# Confirm you addressed comments from EVERY reviewer listed
```
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Reviewer checklist missing the third endpoint

The checklist queries only two of the three endpoints introduced in Step 2d, meaning any reviewer who exclusively left issue-style comments (e.g., @greptileai trigger responses, which the document explicitly places in the issues endpoint) will not appear in the deduplication list and could be silently missed.

The fix is to add the third endpoint to the checklist:

Suggested change
**Checklist before moving on:** After addressing all comments, verify you haven't missed a reviewer:
```bash
# List all unique reviewers who left comments
gh api repos/optave/codegraph/pulls/<number>/comments --paginate --jq '[.[].user.login] | unique | .[]'
gh api repos/optave/codegraph/pulls/<number>/reviews --paginate --jq '[.[].user.login] | unique | .[]'
# Confirm you addressed comments from EVERY reviewer listed
```
**Checklist before moving on:** After addressing all comments, verify you haven't missed a reviewer:
```bash
# List all unique reviewers who left comments
gh api repos/optave/codegraph/pulls/<number>/comments --paginate --jq '[.[].user.login] | unique | .[]'
gh api repos/optave/codegraph/pulls/<number>/reviews --paginate --jq '[.[].user.login] | unique | .[]'
gh api repos/optave/codegraph/issues/<number>/comments --paginate --jq '[.[].user.login] | unique | .[]'
# Confirm you addressed comments from EVERY reviewer listed

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 30c63a2 — added the issues endpoint to the reviewer checklist so commenters who only left issue-style comments (e.g. @greptileai trigger responses) are included in the deduplication check.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed ✓ — the issues endpoint is now included in the checklist, so reviewers who only leave issue-style comments will be caught.

Comment on lines +133 to +136
# Reply on the PR conversation thread so the reviewer sees it
gh api repos/optave/codegraph/issues/<number>/comments \
-f body="Addressed Claude's review feedback:\n- <bullet per item addressed>"
```
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Literal \n won't produce a newline via -f body=

When using gh api -f body="...", the shell does not interpret escape sequences inside double-quoted strings — \n is passed as the two characters \ and n to the GitHub API, which will appear literally in the posted comment rather than as a line break.

To produce an actual newline, use ANSI-C quoting ($'...') or a heredoc:

Suggested change
# Reply on the PR conversation thread so the reviewer sees it
gh api repos/optave/codegraph/issues/<number>/comments \
-f body="Addressed Claude's review feedback:\n- <bullet per item addressed>"
```
gh api repos/optave/codegraph/issues/<number>/comments \
-f body=$'Addressed Claude\'s review feedback:\n- <bullet per item addressed>'

Alternatively, pass the body via --input with a JSON payload for full control over escaping.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 30c63a2 — switched to ANSI-C quoting ($'...') so \n is interpreted as an actual newline by the shell before being passed to the GitHub API.

- Use ANSI-C quoting ($'...') for body with newlines so \n is not passed
  literally to the GitHub API
- Add issues endpoint to the reviewer checklist so issue-style commenters
  (e.g. @greptileai trigger responses) are not silently missed
@carlos-alm
Copy link
Copy Markdown
Contributor Author

@greptileai

@carlos-alm
Copy link
Copy Markdown
Contributor Author

Fixed the remaining Step 2b inconsistency in f8cbb0a — added the missing /pulls/<number>/reviews endpoint to the conflict-resolution context-gathering guidance, so Claude's top-level CHANGES_REQUESTED bodies are now included alongside inline comments and issue comments.

@greptileai

@carlos-alm carlos-alm merged commit 9853c35 into main Mar 20, 2026
15 checks passed
@carlos-alm carlos-alm deleted the fix/review-skill-claude-comments branch March 20, 2026 06:39
@github-actions github-actions Bot locked and limited conversation to collaborators Mar 20, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant