Skip to content

chore: drop synchronize trigger, add ready_for_review + @claude review#25

Merged
lpillmann merged 1 commit intomainfrom
chore/refine-review-triggers
Apr 22, 2026
Merged

chore: drop synchronize trigger, add ready_for_review + @claude review#25
lpillmann merged 1 commit intomainfrom
chore/refine-review-triggers

Conversation

@lpillmann
Copy link
Copy Markdown
Collaborator

Summary

Aligns with Gemma-Analytics/.github PR #20.

  • Drop synchronize trigger — Claude no longer re-reviews on every commit push
  • Add ready_for_review trigger — draft→ready transitions get reviewed
  • Add issue_comment trigger + job-level if: gate for @claude review comment
  • Add !startsWith(body, '@claude review') negation in claude.yml (where applicable) so @claude review routes to the review workflow, not the generic responder

Merge order

Merge Gemma-Analytics/.github#20 first.

🤖 Generated with Claude Code

Aligns with Gemma-Analytics/.github PR #20:
- Drops synchronize to stop re-reviewing on every push
- Adds ready_for_review for draft→ready transitions
- Adds issue_comment trigger + job if-gate for @claude review comments
- Adds !startsWith negation in claude.yml to route @claude review
  to the review workflow, not the generic responder
on:
pull_request:
types: [opened, synchronize]
types: [opened, ready_for_review]
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[BLOCKING] synchronize was removed from the trigger list, so the review workflow will no longer fire automatically when new commits are pushed to an open PR. The only way to re-trigger a review after a push is now to manually comment @claude review. This is almost certainly a regression — most teams expect the bot to re-review updated code without a manual prompt.

Suggested change
types: [opened, ready_for_review]
types: [opened, synchronize, ready_for_review]


jobs:
review:
if: |
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[Suggestion] The if condition only checks github.event.issue.pull_request to confirm the comment is on a PR, but it does not guard against comments from untrusted actors (e.g. external contributors posting @claude review on a public repo). Consider adding github.event.sender.type == 'User' or gating on repo membership/collaborator status to avoid triggering a potentially costly (and secret-exposing) workflow from an arbitrary commenter.

@gemma-claude-assistant
Copy link
Copy Markdown

Review Summary

This PR splits the Claude review trigger from the general Claude comment trigger so that @claude review is routed to the review workflow and not double-handled by the general claude.yml workflow. The routing logic in claude.yml is correct and clean.

One blocking issue:

  • synchronize was dropped from the claude-review.yml trigger, meaning the review bot will no longer fire automatically when new commits are pushed to an open PR. Restoring it (alongside ready_for_review) is needed to preserve the existing behaviour.

One suggestion:

  • The new issue_comment trigger does not restrict which users can invoke @claude review. On a public repo this means any external contributor can trigger the workflow, including secrets access. Consider gating on collaborator/org membership or at minimum checking github.event.sender.type.

Otherwise the change is straightforward and the !startsWith exclusion logic in claude.yml correctly prevents double-handling.

@gemma-claude-assistant
Copy link
Copy Markdown

Claude review cost: $0.1801 · 7 turns · 102s · eu.anthropic.claude-sonnet-4-6

@lpillmann lpillmann merged commit e69cb5c into main Apr 22, 2026
3 of 5 checks passed
@lpillmann lpillmann deleted the chore/refine-review-triggers branch April 22, 2026 14:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant