fix: restrict review-feed to ready_for_review and review_requested#51
Conversation
Remove opened and synchronize from octo-pr-review-feed event types. - opened: fired simultaneously with review_requested, causing two near-identical messages in the project group (perceived duplicate) - synchronize: fires on every commit push; active PRs can generate 10+ messages per day, flooding the reviewer channel Only ready_for_review and review_requested represent true 'reviewer needs to act' signals without noise.
yujiawei
left a comment
There was a problem hiding this comment.
Code Review — PR #51 (octo-lib)
Summary
One-line YAML change in .github/workflows/octo-pr-review-feed.yml: drop opened and synchronize from the pull_request_target event types, keeping only ready_for_review and review_requested. The intent — restrict the review-feed to events that actually mean "a human needs to review this now" — is sensible and the diff is appropriately surgical.
Verification
- ✅ YAML is valid;
sanity / actionlintandsanity / No tabs in workflow filesboth pass on the head SHA. - ✅
Build,Test,Vet,CodeQLall green. - ✅ The
if: ${{ !github.event.pull_request.draft }}filter on thenotifyjob is preserved, so draft-state PRs are still excluded. - ✅ Caller wiring to
Mininglamp-OSS/.github/.github/workflows/octo-pr-review-feed.yml@mainis unchanged;event_actionstill derives fromgithub.event.action, which now is constrained toready_for_review/review_requestedand reaches the reusable workflow with a smaller, more meaningful surface. - ✅ Sibling workflow
octo-pr-result-notify.ymlalready handlesclosed/reopenedand review submissions, so droppingopened/synchronizehere does not lose data the result-notify channel already covers.
Findings
P0 / P1
None. The change is correctness-safe and not a build/security/data risk.
P2 — Observation: coverage gap on direct non-draft opens
GitHub event semantics worth keeping in mind:
ready_for_reviewfires only on the draft → ready transition. It does not fire when a PR is opened directly as non-draft.review_requestedfires only when a reviewer (user or team) is explicitly assigned.
Combined effect after this PR:
| Flow | Notified? |
|---|---|
| Open as draft, later mark ready | ✅ via ready_for_review |
| Open as non-draft with reviewer assigned at open | ✅ via review_requested |
| Open as non-draft, reviewer added later | ✅ via review_requested (later) |
| Open as non-draft, no reviewer ever assigned | ❌ silent |
| Push new commits to an open non-draft PR | ❌ silent (intentional — was spammy) |
The last "silent" row that's intentional is the synchronize removal — clearly a win, since re-notifying the feed on every push is noise. The first "silent" row (non-draft open with no assignee) is the only behavior change worth flagging. If contributors here typically assign reviewers at open time or open as draft first, this is a non-issue. If non-draft "self-serve" PRs happen often, they will now be invisible to the feed until someone explicitly requests review.
This is not a blocker — the PR's stated goal is to restrict the feed and review_requested is the natural signal for "I want eyes on this." Worth a one-line note in the workflow comment header (e.g. "non-draft opens without assignees rely on a later review_requested") to capture the design intent for future maintainers.
P2 — Suggestion: consider keeping opened filtered by draft
An alternative that preserves zero-config "I just opened a PR" notifications without the synchronize spam would be:
types: [opened, ready_for_review, review_requested]The if: !github.event.pull_request.draft already filters draft-state opens. But the cleaner posture is the one this PR takes: the feed is for explicit review requests, and review_requested makes the intent unambiguous. Reasonable judgment call either way; only flagging so the trade-off is on record.
Additional observations
permissions: {}at the top level is good — least privilege forpull_request_target, which runs in the base-repo context with a writable token.- The reusable workflow at
Mininglamp-OSS/.github/.github/workflows/octo-pr-review-feed.yml@mainis referenced via the floating@mainref. Not introduced by this PR, but worth noting as a supply-chain consideration: pinning to a SHA or tag would be more defensive. Out of scope for this fix.
Verdict
The change cleanly achieves its stated goal, reduces feed noise, passes all sanity/lint/build/test checks, and preserves draft-state filtering. The non-draft-open-without-assignee edge case is a minor observation, not a blocker.
Jerry-Xin
left a comment
There was a problem hiding this comment.
Review — octo-lib #51 (f1ab038)
+1 -1. Drops opened and synchronize from octo-pr-review-feed.yml triggers, keeping only ready_for_review and review_requested.
Why this is correct:
synchronize(every push) was the noise source flagged as non-blocking in PR #50 review — now eliminated at the source- Removing
openedenforces an explicit "request reviewer" workflow — PRs don't spam the review-feed channel just by being created ready_for_reviewcovers the draft→ready transition, so draft workflows are still properly surfaced
Verified:
- Trigger set is a strict subset of the previous — no new event types, no new security surface
if: !github.event.pull_request.draftguard from #50 is still present and still meaningful forready_for_reviewedge cases- No other files changed
No blocking issues. APPROVED.
Restrict octo-pr-review-feed event types.