fix(ci): finalize PR notification design (remove octo-pr-feed + fix result-notify)#176
Conversation
…esult-notify Final design (per product owner): - octo-pr-result-notify.yml: terminal state only (merged/closed), routes to global pr-feed via OCTO_PR_FEED_GROUP_ID org variable - octo-pr-review-feed.yml: review events → project group Changes: - Delete octo-pr-feed.yml (pr opened events are intentionally not monitored) - Update octo-pr-result-notify.yml: - Trigger narrowed to [closed] only (drop reopened) - Simplified event_kind expression (pr_merged | pr_closed) - feed_group_id: uses vars.OCTO_PR_FEED_GROUP_ID with safe fallback
Jerry-Xin
left a comment
There was a problem hiding this comment.
This PR is in scope for Mininglamp-OSS/octo-web because it updates this repository’s GitHub Actions PR notification ownership, and I found no blocking issues.
💬 Non-blocking
- 🔵 Suggestion:
.github/workflows/labeler.yml:7still says its security pattern matchesocto-pr-feed.yml, but that workflow is now deleted. Consider updating the comment to reference an existing metadata-only workflow such asauto-add-to-project.ymlorocto-pr-review-feed.yml.
✅ Highlights
.github/workflows/octo-pr-result-notify.yml:4-7now matches the stated design: terminal PR close events plus submitted reviews only..github/workflows/octo-pr-result-notify.yml:21correctly simplifies result classification topr_mergedvspr_closedafter removing thereopenedtrigger..github/workflows/octo-pr-result-notify.yml:25and:43passfeed_group_id; I verified the reusable workflow accepts that input and validates it.- YAML parsing and
git diff --checkpassed locally.
yujiawei
left a comment
There was a problem hiding this comment.
Code Review — PR #105 (octo-adapters)
Verdict: APPROVED
Small, well-scoped CI-config change (2 files, +4/−29). The notification-ownership redesign is internally consistent and the workflow expressions are correct. No correctness, security, or build-breaking issues found.
1. Verification
| Item | Result | Evidence |
|---|---|---|
event_kind simplification is correct |
✅ | octo-pr-result-notify.yml:21 — with the trigger reduced to types: [closed], github.event.action is always closed, so the old 3-way …action == 'closed' && merged && 'pr_merged' || action == 'closed' && 'pr_closed' || 'pr_reopened' collapses cleanly to github.event.pull_request.merged == true && 'pr_merged' || 'pr_closed'. The dropped pr_reopened branch is now unreachable, so removing it is right. |
reopened removed from trigger |
✅ | octo-pr-result-notify.yml:5 — types: [closed]. Matches the stated design (only terminal states monitored). |
feed_group_id is a valid input |
✅ | Reusable Mininglamp-OSS/.github/.github/workflows/octo-pr-result-notify.yml@main declares feed_group_id (string, optional, default '1c303c142e9840f2a9b46c10b0972e8d'). The PR's fallback literal matches that default, so behavior is unchanged when vars.OCTO_PR_FEED_GROUP_ID is unset. |
feed_group_id added to both jobs |
✅ | octo-pr-result-notify.yml:24 (pr-result) and :43 (review-result) both pass `${{ vars.OCTO_PR_FEED_GROUP_ID |
octo-pr-feed.yml deletion leaves no broken refs |
One stale comment reference remains (see P2). No functional reference (e.g. uses: / needs:) points at the deleted file. |
|
| Notification coverage has no unintended gap | ✅ | After the change: ready_for_review/review_requested → octo-pr-review-feed.yml (project group 0262…); closed (merged/closed) → octo-pr-result-notify.yml pr-result (global feed 1c303…); review submitted (approved/changes_requested) → review-result (global feed). opened/reopened intentionally unmonitored. No overlap, no duplicate sends. |
| Static checks | ✅ | actionlint, workflow sanity, and Build & Type-check all pass on the PR. |
2. Findings
P2 — Stale doc comment references the deleted workflow (non-blocking)
.github/workflows/labeler.yml:11:
# the pattern used by auto-add-to-project.yml and octo-pr-feed.yml.
octo-pr-feed.yml is deleted by this PR, so this comment now points at a file that no longer exists. Purely a documentation nit — no functional impact. Suggest updating it to reference only auto-add-to-project.yml (or octo-pr-review-feed.yml) in a follow-up or a quick amend.
3. Suggestions
- (Optional) Fix the
labeler.yml:11comment as above to keep thepull_request_targetrationale comments accurate.
4. Additional observations
- The
review-resultjob retains its fork guardgithub.event.pull_request.head.repo.full_name == github.repository(octo-pr-result-notify.yml:36), so review notifications only fire for same-repo PRs. Unchanged by this PR and appropriate. permissions: {}is preserved on both top-level workflows; secrets are scoped to the reusable call. Good security posture forpull_request_target.- Audience split is intentional and documented: lifecycle/review-request events go to the repo-specific project group, while terminal/review-outcome events go to the global PR feed. This is a design choice, not a defect.
Overall: correct, minimal, and consistent with the documented design. Approving.
Summary
Finalizes the PR notification design. Clean, no-overlap ownership:
octo-pr-result-notify.yml[closed]+pull_request_review: [submitted]vars.OCTO_PR_FEED_GROUP_IDwith fallback)octo-pr-review-feed.yml[ready_for_review, review_requested]Design decisions (per product owner)
opened/reopenedevents are not monitored — only terminal states (merged/closed) and review outcomesocto-pr-feed.ymlis deleted — it was added in a prior pass but is not needed under the final designChanges
octo-pr-feed.yml: deletedocto-pr-result-notify.yml:[closed, reopened]→[closed](terminal state only)event_kind: simplified topr_merged | pr_closedfeed_group_id:${{ vars.OCTO_PR_FEED_GROUP_ID || '1c303c142e9840f2a9b46c10b0972e8d' }}(org variable with safe fallback)No application code modified.