Skip to content

fix: trigger review skill failed to ack #192

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
louisliu2048 merged 4 commits into from
Mar 12, 2026
Merged

fix: trigger review skill failed to ack #192

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4c47b60
report back to user once done
Vui-Chee Mar 12, 2026
168517f
reorg skills
Vui-Chee Mar 12, 2026
76e8ba9
properly install skills
Vui-Chee Mar 12, 2026
47f8727
rename
Vui-Chee Mar 12, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 0 additions & 71 deletions .claude/skills/commit.md
View file
Open in desktop

This file was deleted.

9 changes: 3 additions & 6 deletions .claude/skills/review.md → .claude/skills/pr-review/SKILL.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,9 @@
---
name: review
description: >
Perform a thorough code review of changed files. Use when the user asks
to "review" code, requests a code review, or uses @review. Do NOT trigger
when the user only asks a question about code without requesting a review.
name: pr-review
Copy link
Contributor Author

@Vui-Chee Vui-Chee Mar 12, 2026

Choose a reason for hiding this comment

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

Prevent name clashes with default skills.

description: Perform a code review of a pull request following team standards
---

# Skill: review
# Skill: pr-review

Perform a thorough code review of changed files, checking for correctness, Rust best practices, XLayer-specific conventions, security issues, and test coverage.

Expand Down
41 changes: 29 additions & 12 deletions .github/workflows/claude.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,27 +48,44 @@ jobs:
id-token: write
actions: read
steps:
- name: Acknowledge trigger
uses: actions/github-script@v7
with:
script: |
const isReviewComment = context.eventName === 'pull_request_review_comment';
await github.rest.reactions.createForIssueComment({
owner: context.repo.owner,
repo: context.repo.repo,
comment_id: isReviewComment
? context.payload.comment.id
Comment on lines +54 to +60
Copy link

@claude claude bot Mar 12, 2026

Choose a reason for hiding this comment

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

🔴 The "Acknowledge trigger" step always calls reactions.createForIssueComment(), but for pull_request_review_comment events the correct API is reactions.createForPullRequestReviewComment() — this will 404 on inline review comments. Additionally, the ternary on lines 59-60 is dead code (both branches return context.payload.comment.id). The fix should branch on isReviewComment to call the appropriate reactions API method.

Extended reasoning...

What the bug is

The new "Acknowledge trigger" step (lines 54-60) computes isReviewComment to detect whether the event is a pull_request_review_comment, but then unconditionally calls github.rest.reactions.createForIssueComment(). This is the wrong GitHub API endpoint for PR review comments (inline code comments). The GitHub REST API has two separate endpoints:

  • Issue comments: POST /repos/{owner}/{repo}/issues/comments/{comment_id}/reactions → reactions.createForIssueComment()
  • PR review comments: POST /repos/{owner}/{repo}/pulls/comments/{comment_id}/reactions → reactions.createForPullRequestReviewComment()

These operate on different resource namespaces. A review comment ID passed to the issue-comment endpoint will return a 404 Not Found.

The dead ternary

On lines 59-60, the ternary expression is:

comment_id: isReviewComment
  ? context.payload.comment.id
  : context.payload.comment.id,

Both branches are identical — this is a copy-paste error. The isReviewComment boolean is computed but never influences behavior. While context.payload.comment.id happens to be the correct payload path for both event types, the ternary is misleading dead code that suggests branching was intended but never completed.

Step-by-step proof

  1. A user posts an inline review comment on a PR containing @review.
  2. The pull_request_review_comment event fires, matching the claude-review job condition (line 43).
  3. The "Acknowledge trigger" step runs. isReviewComment is set to true.
  4. The code calls github.rest.reactions.createForIssueComment() with the review comment’s ID.
  5. GitHub looks up /repos/{owner}/{repo}/issues/comments/{comment_id}/reactions — but the comment ID belongs to a pull request review comment, not an issue comment.
  6. GitHub returns 404 Not Found, and the step fails. The eyes reaction is never added.

Impact

Whenever the review workflow is triggered by an inline PR review comment (as opposed to a top-level issue comment), the acknowledge step will fail with a 404. This defeats the purpose of the acknowledgment — the user gets no visual feedback that their review request was received. Depending on the workflow’s continue-on-error configuration (not set here), this could also cause the entire job to fail, preventing the review from running at all.

How to fix

Branch on isReviewComment to call the correct API method:

const isReviewComment = context.eventName === pull_request_review_comment;
const params = {
  owner: context.repo.owner,
  repo: context.repo.repo,
  comment_id: context.payload.comment.id,
  content: eyes
};
if (isReviewComment) {
  await github.rest.reactions.createForPullRequestReviewComment(params);
} else {
  await github.rest.reactions.createForIssueComment(params);
}

This also eliminates the dead ternary, since comment_id no longer needs conditional logic (the payload path is the same for both event types — only the API method differs).

: context.payload.comment.id,
content: 'eyes'
});

- name: Checkout repository
uses: actions/checkout@v6
with:
fetch-depth: 1

- name: Load review skill
id: skill
- name: Install project skills globally
run: |
if [ -f ".claude/skills/review.md" ]; then
SKILL_CONTENT=$(cat .claude/skills/review.md)
echo "skill_content<<EOF" >> $GITHUB_OUTPUT
echo "$SKILL_CONTENT" >> $GITHUB_OUTPUT
echo "EOF" >> $GITHUB_OUTPUT
else
echo "skill_content=Perform a thorough code review." >> $GITHUB_OUTPUT
fi

mkdir -p ~/.claude/skills
cp -rfv .claude/skills/* ~/.claude/skills/

- name: Run Claude Review
id: claude
uses: anthropics/claude-code-action@v1
with:
anthropic_api_key: ${{ secrets.ANTHROPIC_API_KEY }}
github_token: ${{ secrets.GITHUB_TOKEN }}
track_progress: true
show_full_output: true
prompt: |
${{ steps.skill.outputs.skill_content }}
REPO: ${{ github.repository }}
PR NUMBER: ${{ github.event.pull_request.number || github.event.issue.number }}

Use the pr-review skill to perform a code review of this PR.

Comment on lines +81 to +88
Copy link

@claude claude bot Mar 12, 2026

Choose a reason for hiding this comment

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

🔴 github.event.issue.number on lines 90 and 95 is undefined for pull_request_review_comment events — the PR number lives at github.event.pull_request.number for that event type. Since claude-review triggers on both issue_comment and pull_request_review_comment, an inline review comment will produce an empty PR number, breaking both the prompt context and the gh pr comment command. Use ${{ github.event.issue.number || github.event.pull_request.number }} in both places.

Extended reasoning...

Bug Description

The claude-review job triggers on two event types (lines 41-42):

  • issue_comment — where the event payload includes github.event.issue.number
  • pull_request_review_comment — where the event payload does not include an issue object; the PR number is at github.event.pull_request.number

However, the workflow uses github.event.issue.number in two places:

  • Line 90 (prompt): PR NUMBER: ${{ github.event.issue.number }}
  • Line 95 (command): gh pr comment ${{ github.event.issue.number }} --body "<your review>"

Step-by-step proof

  1. A user posts an inline review comment on a PR containing @review.
  2. GitHub fires a pull_request_review_comment event.
  3. The claude-review job matches the if condition on line 42.
  4. The job reaches the "Run Claude Review" step.
  5. github.event.issue is undefined for this event type, so github.event.issue.number evaluates to an empty string.
  6. The prompt becomes PR NUMBER: (empty), giving Claude no context about which PR to review.
  7. The gh pr comment command becomes gh pr comment --body "..." (missing argument), which will fail.

Impact

When triggered by a PR review comment (as opposed to a regular issue comment on a PR), the entire review workflow silently breaks: Claude receives no PR number in its prompt and cannot post the review summary back because gh pr comment fails without a valid PR number argument.

Fix

Replace both occurrences of github.event.issue.number (lines 90 and 95) with:

${{ github.event.issue.number || github.event.pull_request.number }}

This falls back to github.event.pull_request.number when github.event.issue.number is not present, covering both event types correctly.

When done, post your full review summary as a PR comment using:
gh pr comment ${{ github.event.pull_request.number || github.event.issue.number }} --body "<your review>"
claude_args: "--allowedTools Skill,SlashCommand,Bash(gh pr comment:*),Bash(gh pr diff:*),Bash(gh pr view:*),Read,Glob,Grep"
3 changes: 1 addition & 2 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
/target
.claude
.claude/*
!.claude/skills/
!.claude/skills/*.md
.vscode

tests/data
Expand Down