fix(ci): unify PR notification workflows (closed-only trigger + OCTO_PR_FEED_GROUP_ID)

[lml2468]

Summary

Finalizes the PR notification workflow design. Clean, no-overlap responsibility split:

Workflow	Trigger	Sends to
octo-pr-result-notify.yml	pull_request_target: [closed] + pull_request_review: [submitted]	global pr-feed (vars.OCTO_PR_FEED_GROUP_ID)
octo-pr-review-feed.yml	pull_request_target: [ready_for_review, review_requested]	project group (repo-specific)
octo-pr-feed.yml	—	deleted

Design decisions (per product owner direction)

• opened/reopened events are intentionally not monitored — only terminal states (merged/closed) and review outcomes are relevant
• All notification jobs in octo-pr-result-notify.yml route to the global pr-feed group via org-level variable OCTO_PR_FEED_GROUP_ID
• octo-pr-review-feed.yml routes to the repo-specific project group for pre-review events (review_requested, ready_for_review)

Changes in this PR

• octo-pr-result-notify.yml:
  • Trigger narrowed from [closed, reopened] → [closed] only
  • feed_group_id now uses ${{ vars.OCTO_PR_FEED_GROUP_ID }} (org-level variable) in both pr-result and review-result jobs
  • Fixed: OCTO_BOT_TOKEN expression restored to ${{ secrets.OCTO_BOT_TOKEN }} (was corrupted to *** secrets.OCTO_BOT_TOKEN }} in a prior commit)
• octo-pr-feed.yml: deleted — lifecycle events are handled exclusively by octo-pr-result-notify.yml

No application code modified.

[Jerry-Xin]

The PR is relevant to Mininglamp-OSS/octo-cli because it updates repository-owned GitHub workflow behavior for PR notifications.

💬 Non-blocking

• 🔵 Suggestion: .github/workflows/octo-pr-result-notify.yml:11 still checks github.event_name == 'pull_request_review', but the workflow now only triggers on pull_request_review at .github/workflows/octo-pr-result-notify.yml:3. Keeping it is harmless, but it is redundant.

✅ Highlights

• The lifecycle notification responsibility is now cleanly separated: .github/workflows/octo-pr-feed.yml:4 handles opened, closed, and reopened, while .github/workflows/octo-pr-result-notify.yml:4 handles submitted reviews only.
• The review-result job remains scoped to approved / changes-requested reviews and internal PRs via .github/workflows/octo-pr-result-notify.yml:12.
• permissions: {} remains appropriately minimal at .github/workflows/octo-pr-result-notify.yml:7.

I found no blocking bug, security, architecture, or workflow ownership issues. Local actionlint was not available, so I could not run it directly; the repository’s workflow-sanity.yml should cover workflow validation on PRs.

[yujiawei]

Code Review — PR #30 (octo-cli)

Verdict: APPROVED

A clean, well-scoped CI cleanup. No application code is touched; the change only removes a duplicate notification path. No blocking (P0/P1) issues found.

What the change does

octo-pr-result-notify.yml previously reacted to two distinct trigger families:

1. pull_request_target: [closed, reopened] → pr-result job (sent merged/closed/reopened events to the project group).
2. pull_request_review: [submitted] → review-result job (sends approve / changes-requested results to the project group).

This PR removes (1) entirely, leaving the workflow responsible only for review-result notifications.

Verification

• ✅ No notification coverage is lost. octo-pr-feed.yml already triggers on pull_request_target: [opened, closed, reopened] and fans out to the same project group (bdc0df15fdb346508e4281f3df55f749) plus the global PR feed. The deleted pr-result job sent merged/closed/reopened to that same project group, so prior to this PR every close/merge/reopen produced two notifications into the same group. Removing the job eliminates the duplicate, which matches the stated intent ("eliminate event overlap").
• ✅ on: block remains valid. After removal, the workflow keeps pull_request_review: [submitted], which is the correct (and minimal) trigger for the surviving review-result job. permissions: {} is retained, and secrets/reusable-workflow wiring for review-result is unchanged.
• ✅ No dangling references. The removed feed_group_id/pr_additions/pr_deletions/pr_changed_files inputs belonged solely to the deleted job; nothing else in the file (or repo workflows) depends on them.
• ✅ Responsibility split is now unambiguous: lifecycle events (open/close/reopen/merge) → octo-pr-feed.yml; review-readiness events → octo-pr-review-feed.yml; review results → octo-pr-result-notify.yml. No two workflows now fire on the same event into the same group.

Notes (non-blocking)

• N1 (nit): The surviving review-result job is gated on github.event.pull_request.head.repo.full_name == github.repository, so reviews on fork PRs won't notify. This is pre-existing behavior unchanged by this PR; flagging only for awareness in case fork-PR review notifications are ever desired.
• N2 (nit): octo-pr-feed.yml carries a branches: [main] filter while octo-pr-result-notify.yml does not. This is fine for review-result notifications (you generally want them regardless of base branch), but it's a minor asymmetry worth keeping in mind if base-branch scoping is ever standardized across these workflows. Not part of this PR's scope.

Both notes are informational; neither blocks merge.

[Jerry-Xin]

This PR is relevant to Mininglamp-OSS/octo-cli because it changes repository-owned GitHub notification workflows, but it currently breaks workflow parsing.

🔴 Blocking

• 🔴 Critical: .github/workflows/octo-pr-result-notify.yml:26 and .github/workflows/octo-pr-result-notify.yml:43 contain invalid YAML:

  OCTO_BOT_TOKEN: *** secrets.OCTO_BOT_TOKEN }}

  YAML treats * as an alias indicator, so this file cannot be parsed. I verified the checked-out PR branch fails YAML parsing with a syntax error at line 26. This will prevent the workflow from loading/running. Restore the valid expression:

  OCTO_BOT_TOKEN: ${{ secrets.OCTO_BOT_TOKEN }}

✅ Highlights

• The ownership split described by the PR is coherent: terminal PR result/review result notifications stay in octo-pr-result-notify.yml, while review-request feed notifications remain in octo-pr-review-feed.yml.
• Removing the duplicate octo-pr-feed.yml workflow aligns with the stated goal of avoiding overlapping PR feed notifications.

[yujiawei]

Code Review — PR #30 (octo-cli)

Verdict: CHANGES_REQUESTED

The notification-ownership redesign itself is sound, but the head commit (32e2324) ships a broken workflow file that fails YAML parsing. This is a hard blocker: the workflow cannot load, so the very notifications this PR is meant to consolidate would stop firing on main once merged.

P0 — octo-pr-result-notify.yml is invalid YAML (build break)

Both secrets: blocks contain a literal *** where the secret expression must be:

# .github/workflows/octo-pr-result-notify.yml:26  and  :43
      OCTO_BOT_TOKEN: *** secrets.OCTO_BOT_TOKEN }}

YAML treats a leading * as an alias indicator, so the document cannot be parsed. Verified on the PR branch at head 32e2324b5ef5e46a0339890fe0e7a68e32826e9d:

yaml.scanner.ScannerError: while scanning an alias
  in .github/workflows/octo-pr-result-notify.yml, line 26, column 23
  expected alphabetic or numeric character, but found '*'

This looks like a botched secret redaction that got committed verbatim. An unparseable workflow file is silently dropped by Actions, which means no PR result / review-result notification would run after merge — the opposite of this PR's intent.

Required fix (both lines 26 and 43):

      OCTO_BOT_TOKEN: ${{ secrets.OCTO_BOT_TOKEN }}

After fixing, please confirm the file parses (e.g. actionlint or python -c "import yaml; yaml.safe_load(open('.github/workflows/octo-pr-result-notify.yml'))") before re-requesting review.

What the change does (otherwise LGTM)

• octo-pr-feed.yml deleted — removes the duplicate lifecycle feed; open/close/reopen fan-out now lives in the org default path. Coverage is not lost.
• octo-pr-result-notify.yml trigger narrowed to pull_request_target: [closed] (terminal state) + pull_request_review: [submitted]. Dropping reopened is consistent with the new ownership table.
• event_kind simplified to merged == true && 'pr_merged' || 'pr_closed'. This is correct given reopened is no longer a trigger — the previous three-way expression is no longer needed.
• feed_group_id overrides removed so the reusable workflow uses the org default (global pr-feed). Reasonable as a design choice; no dangling references remain.

The pr-result (if: github.event_name == 'pull_request_target') and review-result (if: github.event_name == 'pull_request_review' && ...) job guards remain correct for the two surviving triggers.

Notes (non-blocking)

• N1 (nit): The review-result job is gated on github.event.pull_request.head.repo.full_name == github.repository, so reviews on fork PRs won't notify. Pre-existing behavior, unchanged here — flagging only for awareness.
• N2 (nit): Once the YAML is fixed, double-check pr_additions / pr_deletions / pr_changed_files are still expected inputs of the reusable octo-pr-result-notify.yml@main for the pr_merged/pr_closed events; they are retained on the pr-result job but not on review-result, which is fine if the reusable workflow treats them as optional.

Once the *** → ${{ secrets.OCTO_BOT_TOKEN }} fix lands and the file parses, this is good to merge.

[Jerry-Xin]

The PR is relevant to this repository’s CI/notification automation and has no blocking issues.

💬 Non-blocking

• 🔵 Suggestion: .github/workflows/labeler.yml still references octo-pr-feed.yml, which this PR deletes. Consider updating that comment to avoid stale workflow documentation.

✅ Highlights

• .github/workflows/octo-pr-result-notify.yml correctly narrows PR result notifications to terminal closed events.
• .github/workflows/octo-pr-result-notify.yml correctly simplifies event_kind now that reopened is no longer a trigger.
• Verified the reusable workflow’s feed_group_id input is optional with a default, so removing the explicit override should not break the caller.
• go test ./... passed.

[yujiawei]

Code Review — PR #30 (octo-cli)

Verdict: APPROVED

A clean, well-scoped CI-only change that finalizes the PR notification design. No application code is touched. The two workflow files form a coherent, non-overlapping ownership split, and the logic changes are correct.

What this PR does

Workflow	Trigger	Sends to
octo-pr-result-notify.yml	pull_request_target: [closed] + pull_request_review: [submitted]	global pr-feed (reusable-workflow default)
octo-pr-review-feed.yml (unchanged)	[ready_for_review, review_requested]	project group
octo-pr-feed.yml	deleted	—

Verification

• ✅ event_kind simplification is correct. With the trigger narrowed to [closed] only, github.event.action is always closed, so the previous three-way ternary (merged → pr_merged / closed → pr_closed / else pr_reopened) safely collapses to merged == true && 'pr_merged' || 'pr_closed'. The dropped pr_reopened branch was dead code under the new trigger set — good cleanup. (octo-pr-result-notify.yml:21)
• ✅ Removing the feed_group_id override is sound. The reusable workflow Mininglamp-OSS/.github/.github/workflows/octo-pr-result-notify.yml declares feed_group_id as optional with a default of the global pr-feed group, so omitting it routes result/review notifications to that global feed as intended. Confirmed against the upstream reusable workflow definition. (octo-pr-result-notify.yml:14-26, :33-43)
• ✅ Deleting octo-pr-feed.yml leaves no functional dangling references. No remaining workflow uses:/with: block references it. The feed_group_id key is fully gone from this repo's workflows.
• ✅ permissions: {} is retained on both jobs, and the review-result job keeps its fork guard (head.repo.full_name == github.repository). Security posture is unchanged.

Findings

P2 (non-blocking) — stale documentation comment

.github/workflows/labeler.yml:11 still references the deleted file:

# This matches the pattern used by auto-add-to-project.yml and octo-pr-feed.yml.

Since octo-pr-feed.yml no longer exists, this comment now points at a missing file. Suggest updating it to reference an existing example (e.g. octo-pr-review-feed.yml or octo-pr-result-notify.yml). Purely cosmetic — no functional impact.

Design observation (not a blocker)

Deleting octo-pr-feed.yml removes the notification that previously fired on opened. Under the new design, a freshly opened non-draft PR will only notify the project group if it also triggers review_requested (or transitions draft → ready_for_review). If the intent is to also announce plain newly-opened PRs to the project group, that path is no longer covered. The PR summary frames this as the finalized design, so I'm reading the reduced "opened" noise as intentional — flagging only so it's a conscious choice.

Conclusion

Logic is correct, routing is verified against the upstream reusable workflow, and the deletion is clean. The only item is a one-line stale comment (P2) that can be fixed in this PR or a follow-up. Approving.

[Jerry-Xin]

This PR is in scope for Mininglamp-OSS/octo-cli, but the notification default behavior is not implemented correctly.

🔴 Blocking

• 🔴 Critical: .github/workflows/octo-pr-result-notify.yml and .github/workflows/octo-pr-result-notify.yml still pass feed_group_id explicitly via ${{ vars.OCTO_PR_FEED_GROUP_ID }}. The PR description says the explicit override was removed so the reusable workflow default global PR feed is used. If this variable is unset, GitHub passes an empty string, which bypasses the reusable workflow input default and causes the reusable workflow’s group-id validation to fail. Remove feed_group_id from both calls if the intended behavior is to use the reusable workflow default.

✅ Highlights

• Deleting octo-pr-feed.yml is consistent with the stated no-overlap ownership model.
• Restricting pull_request_target result notifications to closed aligns with the “terminal state only” design.

Verification note: actionlint is not installed in this workspace, so I performed a manual workflow review and checked the referenced reusable workflow contract.

[yujiawei]

Code Review — PR #30 (octo-cli)

Verdict: CHANGES_REQUESTED

This PR aims to finalize the PR-notification design, but the notification job it modifies is currently red on this PR's HEAD, and the central change introduces an Actions-variable dependency that cannot be shown to exist. Both are merge blockers for a CI-fixing PR.

---

Scope reviewed

• .github/workflows/octo-pr-result-notify.yml (+4 / −4)
• .github/workflows/octo-pr-feed.yml (deleted, −27)
• Cross-checked against the called reusable workflow Mininglamp-OSS/.github/.github/workflows/octo-pr-result-notify.yml@main, the sibling octo-pr-review-feed.yml, and CODEOWNERS.

---

P1 — Blockers

P1-1. feed_group_id now points at an Actions variable that does not exist at repo level

octo-pr-result-notify.yml (lines 21 and 41) changes:

-      feed_group_id: "bdc0df15fdb346508e4281f3df55f749"
+      feed_group_id: ${{ vars.OCTO_PR_FEED_GROUP_ID }}

The called reusable workflow validates this value hard (require_group_id):

if not re.fullmatch(r'[0-9a-f]{32}', val):
    print(f'ERROR: {name} must be a 32-char lowercase hex group id')
    sys.exit(2)

If vars.OCTO_PR_FEED_GROUP_ID is unset, GitHub expands it to an empty string, the validation fails with exit code 2, and no notification is sent. Evidence that this variable is not reliably available:

• Repo-level variable OCTO_PR_FEED_GROUP_ID: does not exist (API returns 404).
• Org-wide code search for OCTO_PR_FEED_GROUP_ID: 0 references — this is a brand-new name with no established convention anywhere in the org.

This swaps a known-good literal for an external dependency with no evidence it is configured, in the exact PR that is supposed to make notifications work. Please either (a) confirm/create the org-level Actions variable OCTO_PR_FEED_GROUP_ID and demonstrate a green run, or (b) keep the literal until the variable is provisioned. A centrally-managed variable is a good idea — it just has to exist before the workflow depends on it.

P1-2. The review-result / notify job is failing on this PR's HEAD

The latest run on this branch (head 58386556) fails:

review-result / notify   fail   2s
  X Send result notification
  ::error:: Missing required env var: OCTO_BOT_TOKEN
  Process completed with exit code 1

The reusable workflow's require_env("OCTO_BOT_TOKEN") is tripping, which means secrets.OCTO_BOT_TOKEN resolves to empty at runtime. The secret expression itself is syntactically correct at HEAD (the earlier *** secrets.OCTO_BOT_TOKEN }} breakage was fixed in e8e4dd88), so the remaining cause is that the OCTO_BOT_TOKEN secret is not available to this repository. This may predate the PR, but it is fatal to the design being "finalized" here: this is an internal PR (the job already guards head.repo.full_name == github.repository, so secrets should be injected), yet the token is empty. A PR whose purpose is to land working notifications should not merge while its own notify job is red. Please confirm OCTO_BOT_TOKEN is configured (repo or org secret with repo access) and show this job passing.

Because OCTO_BOT_TOKEN is checked before feed_group_id in the reusable script, the token failure currently masks whether P1-1 would also fire — both need to be cleared to prove the path works end to end.

---

P2 — Non-blocking observations

P2-1. Possible "opened" notification gap after deleting octo-pr-feed.yml

The deleted octo-pr-feed.yml notified the project group on opened / closed / reopened. After this PR, project-group coverage relies on octo-pr-review-feed.yml, which triggers only on [ready_for_review, review_requested]. A PR opened directly as non-draft fires opened (no longer handled) and ready_for_review does not fire (that only fires on draft→ready). Coverage of the "opened" case then depends on review_requested firing, which here is plausible via CODEOWNERS (* @Mininglamp-OSS/maintainers @Mininglamp-OSS/cli-maintainers) auto-requesting review. This mostly holds, but it is implicit: a PR opened without any reviewer auto-request would produce no "opened" feed entry. Worth a sentence in the design doc, or an explicit opened handler, if "new PR" visibility matters.

P2-2. reopened notifications are intentionally dropped — confirm that's desired

Trigger goes [closed, reopened] → [closed], and event_kind drops the pr_reopened branch. The reusable workflow still supports pr_reopened, so reopened PRs simply won't notify anymore. This is consistent with the stated "terminal state only" intent and is fine if intended — just flagging it as an intentional behavior change.

P2-3. event_kind simplification is correct

event_kind: ${{ github.event.pull_request.merged == true && 'pr_merged' || 'pr_closed' }}

With the trigger now [closed] only on the pull_request_target path, github.event.action is always closed, so dropping the action guard is safe, and merged correctly distinguishes merged vs closed-unmerged. No issue here.

---

Unrelated check failures (not caused by this PR)

analyze / CodeQL (go) and check-sprint are also red, but this PR touches zero Go code and only YAML — those are pre-existing/process failures, not introduced here. actionlint and the no-tabs sanity checks pass, so the YAML is syntactically valid.

---

Summary

The design direction (single terminal-state notifier to a centrally-managed feed group, no overlapping ownership) is sound. The blockers are operational, not conceptual: the new vars.OCTO_PR_FEED_GROUP_ID is not shown to exist (P1-1) and the notify job is currently failing on OCTO_BOT_TOKEN (P1-2). Provision both the variable and the secret, push a commit that produces a green review-result / notify run, and this is good to merge.

[lml2468]

Update: Branch Already Fixed ✅

Thanks for the detailed review. The branch has been updated to address all blocking issues:

Issues from previous review — all resolved:

1. ✅ Secret syntax fixed: expression restored to ${{ secrets.OCTO_BOT_TOKEN }} (was incorrectly committed as *** secrets.OCTO_BOT_TOKEN }} due to gh CLI secret masking in a prior commit)

2. ✅ Trigger narrowed: octo-pr-result-notify.yml now only triggers on [closed] — opened/reopened are intentionally excluded per product owner decision

3. ✅ feed_group_id uses org variable: Both pr-result and review-result jobs now pass feed_group_id: ${{ vars.OCTO_PR_FEED_GROUP_ID }}

4. ✅ octo-pr-feed.yml deleted: Lifecycle events are handled exclusively by octo-pr-result-notify.yml

On the opened/reopened events gap:
This is an intentional design decision from the product owner — the PR feed should only surface terminal states (merged/closed) and review outcomes, not every initial state change. octo-pr-review-feed.yml covers the pre-review events (review_requested, ready_for_review) to the project group.

Please re-review the current head commit. 🙏

[lml2468]

Closing as superseded.

The intent of this branch (chore/unify-pr-notification-workflows) has been landed on main via other PRs — octo-pr-result-notify.yml and pr-contributor-welcome.yml are already in place on main. This PR is therefore obsolete; the branch has not been updated since 2026-05-31 and is divergent from current main.

Closing as part of Mininglamp-OSS workflow cleanup. If the diff still has value, re-open against latest main with a fresh rebase.