Skip to content

Consumers only trigger on opened/reopened, so pushes are never re-reviewed #5

Description

@vanducng

Every consumer of this reusable workflow that I can see subscribes to [opened, reopened], which means a pull request is reviewed once and never again. synchronize is the event that fires on push.

Affected repositories I found locally: vanducng/skills, vanducng/miu-cr, vanducng/miu-db. All three have the identical gap, which suggests it was copied from a common starting point rather than chosen per repository.

What it costs

On vanducng/skills#381, four commits over roughly three hours:

  • The reviewer ran on the first commit only. Three later commits merged unreviewed, one of them +535/-142 across 9 new files.
  • The summary comment stayed pinned to the first commit and kept displaying its finding as Open (1) well after that finding was fixed, because a finding is only resolved by being absent from a fresh run. With no run there is no resolution, so the comment silently misrepresents the current state of the branch.
  • Reopening the pull request produced no new review, so even a deliberate retrigger did not clear it.

Ask

Since this workflow is workflow_call and cannot set its own triggers, the fix belongs in whatever the org copies from. Two suggestions:

  1. Document the required trigger block in the workflow header comment (it currently documents secrets but not events), so a consumer copying it gets a complete setup:
on:
  pull_request_target:
    types: [opened, synchronize, reopened, ready_for_review]
  issue_comment:
    types: [created]

concurrency:
  group: code-review-${{ github.event.pull_request.number || github.event.issue.number }}
  cancel-in-progress: true
  1. Warn at runtime when the head is not the reviewed commit. The workflow already computes HEAD_SHA (line 118) and the posting step already reads prior comments. When a previous review comment exists for a different SHA and this run was not triggered by that push, an annotation such as Last reviewed <sha>, current head <sha> would make staleness visible instead of silent. This is the failure mode that actually bit us - the comment was not wrong when written, it just stopped being true.

The concurrency group matters once synchronize is enabled: without it a rapid push sequence produces concurrent runs racing to comment on the same pull request.

I have fixed the consumer side in vanducng/skills#383. Filing here so the other repositories, and anyone who onboards next, do not inherit it.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions