fix: trigger review skill failed to ack#192
Conversation
| track_progress: true | ||
| prompt: | | ||
| REPO: ${{ github.repository }} | ||
| PR NUMBER: ${{ github.event.issue.number }} | ||
|
|
||
| ${{ steps.skill.outputs.skill_content }} | ||
|
|
There was a problem hiding this comment.
🔴 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 includesgithub.event.issue.numberpull_request_review_comment— where the event payload does not include anissueobject; the PR number is atgithub.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
- A user posts an inline review comment on a PR containing
@review. - GitHub fires a
pull_request_review_commentevent. - The
claude-reviewjob matches theifcondition on line 42. - The job reaches the "Run Claude Review" step.
github.event.issueisundefinedfor this event type, sogithub.event.issue.numberevaluates to an empty string.- The prompt becomes
PR NUMBER:(empty), giving Claude no context about which PR to review. - The
gh pr commentcommand becomesgh 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.
| 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 |
There was a problem hiding this comment.
🔴 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
- A user posts an inline review comment on a PR containing
@review. - The
pull_request_review_commentevent fires, matching theclaude-reviewjob condition (line 43). - The "Acknowledge trigger" step runs.
isReviewCommentis set totrue. - The code calls
github.rest.reactions.createForIssueComment()with the review comment’s ID. - 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. - 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).
| 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 |
There was a problem hiding this comment.
Prevent name clashes with default skills.
* main: (25 commits) fix: bump quinn-proto to 0.11.14 to patch CVE-2026-31812 DoS vuln (#183) pre-job authorization (#193) fix: trigger review skill failed to ack (#192) feat: trigger skill review separately (#191) feat: add Claude skills and CLAUDE.md for AI-assisted development (#190) rename ext (#185) supply workflow for claude (#184) feat(builder): incremental trie cache optimization for flashblocks state root (#163) chore(flashblocks-rpc): migrate op-reth flashblocks into xlayer-reth (#175) Feat(chainspec): adding xlayer-devnet chainspec (#167) chore(builder): flatten flashblocks builder, remove unnecessary trait interfaces (#172) rpc: remove unnecessary trait bounds and dependencies from XlayerRpcExtApiServer impl (#171) fix fmt in bin/tools/gen_genesis.rs (#170) fix(builder): Resolve bugs on upstream flashblocks timing scheduler (#169) Feat(tools): Add a tool to generate a custom genesis file based on a template and existing chain data (#159) feat(flashblocks): Add flashblocks sequence persistence logic on RPC and sequence replay flashblock builder (#162) chore(builder): remove unused custom-engine-api feature flag in tests (#168) fix: p2p test hang due to hang on port (#165) fix: update testcontainers to v0.27.0 to remediate CVE-2025-62518 (#164) chore(builder): further clean up builder crate (#161) ...
No description provided.