tools: claude: update workflow with systemd changes#472
tools: claude: update workflow with systemd changes#472pzmarzly wants to merge 1 commit intofacebook:mainfrom
Conversation
systemd recently adopted this workflow, improving it in the process. New workflow is more secure, and has more stable output format.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| owner, | ||
| repo, | ||
| issue_number: prNumber, | ||
| per_page: 100, |
There was a problem hiding this comment.
Claude: suggestion: listComments uses per_page: 100 without pagination. On a long-lived PR with 100+ top-level comments, the tracking comment could be missed, causing a duplicate. Consider using github.paginate():
| per_page: 100, | |
| const issueComments = await github.paginate( | |
| github.rest.issues.listComments, | |
| { owner, repo, issue_number: prNumber, per_page: 100 } | |
| ); |
Then use issueComments directly (it returns a flat array, no .data wrapper).
| if (typeof review.summary === "string") | ||
| summary = review.summary; | ||
| } catch (e) { | ||
| console.log(`Failed to parse structured output: ${e.message}`); |
There was a problem hiding this comment.
Claude: suggestion: When JSON.parse fails, the error is logged but execution continues silently — the tracking comment ends up saying "no issues found" which hides the real problem. Consider using core.warning() so the workflow run status reflects the parse failure:
| console.log(`Failed to parse structured output: ${e.message}`); | |
| } catch (e) { | |
| core.warning(`Failed to parse structured output: ${e.message}`); | |
| } |
| "description": "Path relative to repo root" | ||
| }, | ||
| "line": { | ||
| "type": "integer", |
There was a problem hiding this comment.
Claude: suggestion: The schema requires line as a non-nullable integer. Issues that aren't tied to a specific line (commit message problems, general documentation observations, CLAUDE.md improvement suggestions) can't be represented — Claude will either fabricate a line number or drop the finding.
Consider making line optional by removing it from the required array and adding a guard in the post job to skip createReviewComment when line is absent (those findings would still appear in the tracking comment summary).
| const prevBody = existing.body.replace(/\n\n\[Workflow run\]\([^)]*\)$/, ""); | ||
| await github.rest.issues.updateComment({ | ||
| owner, | ||
| repo, | ||
| comment_id: existing.id, | ||
| body: `> **Claude is re-reviewing this PR…** ([workflow run](${runUrl}))\n\n${prevBody}`, |
There was a problem hiding this comment.
Claude: nit: When a re-run is triggered while a previous "re-reviewing" banner is already present, the new update prepends another > **Claude is re-reviewing…** on top of the previous one. Multiple rapid synchronize events (e.g. force-push + immediate amend) will accumulate layers of nested blockquote headers.
Consider stripping any existing re-reviewing prefix from prevBody before wrapping again, or replacing the body with just the status line + the last actual review content.
There was a problem hiding this comment.
It should eventually fix itself though, once workflow succeeds
|
|
||
| /* If the review job failed or was cancelled, update the tracking | ||
| * comment to reflect that and bail out. */ | ||
| if ("${{ needs.review.result }}" !== "success") { |
There was a problem hiding this comment.
Claude: nit: "${{ needs.review.result }}" is injected directly into the JavaScript source via GitHub Actions expression expansion. While needs.*.result is a GitHub-controlled enum and safe here, the rest of the file consistently uses environment variables for expression values (e.g. process.env.PR_NUMBER, process.env.HEAD_SHA). For consistency:
env:
REVIEW_RESULT: ${{ needs.review.result }}if (process.env.REVIEW_RESULT !== "success") {
|
Claude: review of facebook/bpfilter #472 (8ac27f6) Suggestions
Nits
|
|
@daandemeyer do you mind taking a look at the issues that the bot found? |
qdeslandes
left a comment
qdeslandes
left a comment
There was a problem hiding this comment.
Reviewing AI instructions really is not fun. I had a quick look and it LGTM, letting it run on PRs will be the proper way to actually check if it does what we expect it to do. I'll let @daandemeyer have a look at your comment if he wants to.
2 participants
systemd project recently adopted this workflow, improving it in the process.
New workflow is more secure, and has more stable output format.
Thanks @daandemeyer!
I kept some of the differences:
Demo: