ci: Fix several issues in claude-review workflow
Address feedback from facebook/bpfilter#472: - Fix setFailed error message counting file-level comments (without line numbers) that are intentionally skipped, use inlineComments.length instead of comments.length - Fix double severity prefix in inline comments: the prompt told Claude to prefix body with **must-fix**/etc but the post job also prepended "Claude: ", producing "Claude: **must-fix**: ...". Now the prompt says not to prefix and the post job adds "Claude **severity**: " using the structured severity field - Move error tracking instructions to a top-level section after all phases so they apply to all runs, not just the first run - Clarify that line is optional: use "should be" instead of "must be" and document that omitting line still surfaces the comment in the tracking comment summary - Distinguish cancelled vs failed in tracking comment message - Add side: "RIGHT" and subject_type: "line" to createReviewComment per GitHub API recommendations - Downgrade partial inline comment posting failures to warnings; only fail the job when no comments at all could be posted Co-developed-by: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
committed by
Zbigniew Jędrzejewski-Szmek
parent
a9ac5cdf18
commit
3a76c0959f
@@ -209,7 +209,7 @@ jobs:
|
||||
HEAD SHA: ${{ needs.setup.outputs.head_sha }}
|
||||
|
||||
You are a code reviewer for the ${{ github.repository }} project. Review this pull request and
|
||||
produce a structured JSON result containing your review comments. Do NOT attempt
|
||||
produce a structured JSON result containing your review. Do NOT attempt
|
||||
to post comments yourself — just return the JSON. You are in the upstream repo
|
||||
without the patch applied. Do not apply it.
|
||||
|
||||
@@ -251,12 +251,13 @@ jobs:
|
||||
Give each subagent the PR title, description, full patch, and the list of changed files.
|
||||
|
||||
Each subagent must return a JSON array of issues:
|
||||
`[{"file": "path", "line": <number>, "severity": "must-fix|suggestion|nit", "body": "..."}]`
|
||||
`[{"file": "path", "line": <number> (optional), "severity": "must-fix|suggestion|nit", "body": "..."}]`
|
||||
|
||||
`line` must be a line number from the NEW side of the diff **that appears inside
|
||||
`line` should be a line number from the NEW side of the diff **that appears inside
|
||||
a diff hunk** (i.e. a line that is shown in the patch output). GitHub's review
|
||||
comment API rejects lines outside the diff context, so never reference lines
|
||||
that are not visible in the patch.
|
||||
that are not visible in the patch. If you cannot determine a valid diff line,
|
||||
omit `line` — the comment will still appear in the tracking comment summary.
|
||||
|
||||
Each subagent MUST verify its findings before returning them:
|
||||
- For style/convention claims, check at least 3 existing examples in the codebase to confirm
|
||||
@@ -273,8 +274,9 @@ jobs:
|
||||
comment if one already exists on the same file and line about the same problem.
|
||||
Also check for author replies that dismiss or reject a previous comment — do NOT
|
||||
re-raise an issue the PR author has already responded to disagreeing with.
|
||||
4. Prefix ALL comment bodies with a severity tag: `**must-fix**: `, `**suggestion**: `,
|
||||
or `**nit**: `.
|
||||
4. Do NOT prefix `body` with a severity tag — the severity is already
|
||||
captured in the `severity` field and will be added automatically when
|
||||
posting inline comments.
|
||||
5. Write a `summary` field in markdown for a top-level tracking comment.
|
||||
|
||||
**If no existing tracking comment was found (first run):**
|
||||
@@ -298,13 +300,6 @@ jobs:
|
||||
Omit empty sections. Each checkbox item must correspond to an entry in `comments`.
|
||||
If there are no issues at all, write a short message saying the PR looks good.
|
||||
|
||||
Throughout all phases, track any errors that prevented you from doing
|
||||
your job fully: permission denials (403, "Resource not accessible by
|
||||
integration"), tools that were not available, rate limits, or any other
|
||||
failures that degraded the review quality. If there were any, append a
|
||||
`### Errors` section listing each failed tool/action and the error
|
||||
message, so maintainers can fix the workflow configuration.
|
||||
|
||||
**If an existing tracking comment was found (subsequent run):**
|
||||
Use the existing comment as the starting point. Preserve the order and wording
|
||||
of all existing items. Then apply these updates:
|
||||
@@ -318,6 +313,15 @@ jobs:
|
||||
in the appropriate severity section, after the existing items.
|
||||
- Do NOT reorder, reword, or remove existing items.
|
||||
|
||||
## Error tracking
|
||||
|
||||
Throughout all phases, track any errors that prevented you from doing
|
||||
your job fully: permission denials (403, "Resource not accessible by
|
||||
integration"), tools that were not available, rate limits, or any other
|
||||
failures that degraded the review quality. If there were any, append a
|
||||
`### Errors` section to the summary listing each failed tool/action and
|
||||
the error message, so maintainers can fix the workflow configuration.
|
||||
|
||||
## CRITICAL: Return structured JSON output
|
||||
|
||||
Your FINAL action must be to return a JSON object matching the following
|
||||
@@ -361,11 +365,12 @@ jobs:
|
||||
/* If the review job failed or was cancelled, update the tracking
|
||||
* comment to reflect that and bail out. */
|
||||
if (process.env.REVIEW_RESULT !== "success") {
|
||||
const verb = process.env.REVIEW_RESULT === "cancelled" ? "was cancelled" : "failed";
|
||||
await github.rest.issues.updateComment({
|
||||
owner,
|
||||
repo,
|
||||
comment_id: commentId,
|
||||
body: `Claude review failed — see [workflow run](${runUrl}) for details.\n\n${MARKER}`,
|
||||
body: `Claude review ${verb} — see [workflow run](${runUrl}) for details.\n\n${MARKER}`,
|
||||
});
|
||||
core.setFailed("Review job did not succeed.");
|
||||
return;
|
||||
@@ -412,7 +417,9 @@ jobs:
|
||||
commit_id: headSha,
|
||||
path: c.file,
|
||||
line: c.line,
|
||||
body: `Claude: ${c.body}`,
|
||||
side: "RIGHT",
|
||||
subject_type: "line",
|
||||
body: `Claude: **${c.severity}**: ${c.body}`,
|
||||
});
|
||||
posted++;
|
||||
} catch (e) {
|
||||
@@ -429,8 +436,6 @@ jobs:
|
||||
else
|
||||
console.log("No inline comments to post.");
|
||||
|
||||
const failed = inlineComments.length > 0 && posted < inlineComments.length;
|
||||
|
||||
/* Update the tracking comment with Claude's summary. */
|
||||
if (!summary)
|
||||
summary = "Claude review: no issues found :tada:\n\n" + MARKER;
|
||||
@@ -447,5 +452,7 @@ jobs:
|
||||
|
||||
console.log("Tracking comment updated successfully.");
|
||||
|
||||
if (failed)
|
||||
core.setFailed(`Failed to post ${comments.length - posted}/${comments.length} inline comment(s).`);
|
||||
if (inlineComments.length > 0 && posted === 0)
|
||||
core.setFailed(`Could not post any of ${inlineComments.length} inline comment(s) — see warnings above.`);
|
||||
else if (posted < inlineComments.length)
|
||||
core.warning(`${inlineComments.length - posted}/${inlineComments.length} inline comment(s) could not be posted.`);
|
||||
|
||||
Reference in New Issue
Block a user