fix(ci): finalize PR notification design (remove octo-pr-feed + fix result-notify)#32
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.
Summary: This PR is in scope for Mininglamp-OSS/octo-cli and cleanly narrows PR result notifications to terminal/review outcomes with no blocking issues found.
💬 Non-blocking:
- 🔵 Suggestion:
.github/workflows/labeler.yml:11still referencesocto-pr-feed.yml, which this PR deletes. The comment should be updated later to reference an existing workflow pattern, such asocto-pr-review-feed.ymlorauto-add-to-project.yml.
✅ Highlights:
- The deleted
.github/workflows/octo-pr-feed.ymlmatches the stated design decision to stop monitoringopened/reopenedPR feed events. .github/workflows/octo-pr-result-notify.yml:21correctly simplifies terminal PR events topr_mergedvspr_closedafter removing thereopenedtrigger..github/workflows/octo-pr-result-notify.yml:25and.github/workflows/octo-pr-result-notify.yml:43use the org-levelOCTO_PR_FEED_GROUP_IDwith a valid fallback, aligning result and review notifications to the global PR feed.
Validation: git diff --check passed. actionlint was not installed in the environment, so I could not run a dedicated GitHub Actions linter.
yujiawei
left a comment
There was a problem hiding this comment.
Code Review — PR #32 (octo-cli)
Verdict: APPROVED
A clean, self-contained CI-configuration change (2 files, +4/−31, no application code). The notification ownership model is now coherent and non-overlapping, and every change is consistent with the stated design. No correctness, security, or blocking issues found.
1. Verification
- ✅ Delete
octo-pr-feed.yml— Removed theopened/closed/reopened → project groupnotifier. Per the design,opened/reopenedare intentionally no longer monitored, andclosed(merged/closed) is now owned solely byocto-pr-result-notify.yml. No dangling references to the deleted workflow remain in the repo's workflow set. - ✅ Trigger narrowing
[closed, reopened] → [closed](octo-pr-result-notify.yml) — Correct.reopenedis dropped by design, so thepr-resultjob now only fires on terminal close events. - ✅
event_kindsimplification — Old:
github.event.action == 'closed' && merged == true && 'pr_merged' || github.event.action == 'closed' && 'pr_closed' || 'pr_reopened'
New:
github.event.pull_request.merged == true && 'pr_merged' || 'pr_closed'
Theaction == 'closed'guards were redundant (thepr-resultjob is gated byif: github.event_name == 'pull_request_target'and the trigger is now closed-only), and thepr_reopenedbranch was dead code oncereopenedwas removed. The new expression resolves topr_mergedwhen merged andpr_closedotherwise — both branches yield truthy strings, so there is no GitHub Actions ternary empty-string footgun. - ✅
feed_group_id→${{ vars.OCTO_PR_FEED_GROUP_ID || '1c303c142e9840f2a9b46c10b0972e8d' }}— Good defensive pattern: org/repo variable override with a safe hardcoded fallback, applied consistently to both thepr-resultandreview-resultjobs. Re-targets result notifications to the global pr-feed per the design.
2. Issues (P0/P1)
None.
3. Suggestions (non-blocking)
- P2 / maintainability — mixed group-id sourcing.
octo-pr-result-notify.ymlnow reads its target viavars.OCTO_PR_FEED_GROUP_ID || 'fallback', whileocto-pr-review-feed.ymlstill hardcodes the project groupbdc0df15fdb346508e4281f3df55f749. The two IDs are deliberately different (global pr-feed vs. project group), so this is not a bug — but consider eventually promoting the project group to its own org/repo variable too, so all notification targets follow one configuration convention. Not required for this PR.
4. Additional observations
- Security posture is sound. Both remaining workflows use
pull_request_targetwithpermissions: {}and pass only PR metadata to org-owned reusable workflows (Mininglamp-OSS/.github/...@main); no untrusted PR code is checked out or executed. Thereview-resultjob additionally guards withgithub.event.pull_request.head.repo.full_name == github.repository, correctly scoping review notifications to non-fork PRs. None of this is regressed by the PR. - Behavioral note (intentional): as a result of deleting
octo-pr-feed.yml, PR-openedevents no longer generate any feed notification. This matches the PR description's explicit product decision ("opened/reopenedevents are not monitored — only terminal states and review outcomes"). Flagging only so the change in coverage is on record.
|
📖 Review note (齐静春) — could not submit formal CHANGES_REQUESTED because Finding:
Gate notes:
|
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.