fix(ci): finalize PR notification design (octo-pr-result-notify + delete octo-pr-feed)

[lml2468]

Summary

Finalizes the PR notification design. Clean, no-overlap ownership:

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

Changes

• octo-pr-result-notify.yml: trigger changed to [closed] only (terminal state); removed explicit feed_group_id override (uses org default = global pr-feed)
• octo-pr-feed.yml: deleted — not needed

No application code modified.

[Jerry-Xin]

The PR is relevant to Mininglamp-OSS/octo-server and cleanly narrows this workflow to review-result notifications only.

✅ Highlights

• 🔵 Suggestion: No action required. .github/workflows/octo-pr-result-notify.yml:4 now only listens for pull_request_review: submitted, which matches the stated responsibility split.
• ✅ .github/workflows/octo-pr-result-notify.yml:11-14 keeps the existing guard for approved / changes-requested reviews and excludes forked PRs before using OCTO_BOT_TOKEN.
• ✅ .github/workflows/octo-pr-result-notify.yml:24 explicitly routes review results to the project group, avoiding the reusable workflow’s default global feed group.

I found no blocking bug, security, architecture, or test coverage issues. I also validated that the workflow YAML parses successfully.

[lml2468]

[APPROVE]

Clean dedup — removes the pull_request_target job that duplicated octo-pr-feed.yml's closed/reopened responsibility, and routes review-result to the project group via explicit feed_group_id.

✅ Verified

• No event gap: opened/closed/reopened are now handled solely by octo-pr-feed.yml (PR #202, merged). review_requested/ready_for_review stay with octo-pr-review-feed.yml. Review submissions stay here.
• feed_group_id override: the reusable workflow accepts this parameter (confirmed in .github repo), defaults to global pr-feed. Overriding to the project group means review results go only to the project channel — correct.
• Security: pull_request_review trigger (not pull_request_target) is the right choice for review events. permissions: {} unchanged. No user-controlled inputs interpolated into shell.
• Workflow Sanity (actionlint + no-tabs): both pass.

No findings.

[yujiawei]

Code Review — PR #204 (octo-server)

Summary

This PR removes the pr-result job from octo-pr-result-notify.yml (which fired on pull_request_target: [closed, reopened]) and adds an explicit feed_group_id to the remaining review-result job. The net effect is that octo-pr-result-notify.yml is now responsible only for review-result notifications (approved / changes_requested), while PR lifecycle events (opened / closed / reopened, including merge) are handled solely by octo-pr-feed.yml. Diff is +1 / -18 in a single file.

Verification

✅ No notification gap from removing the pr-result job. The deleted job emitted pr_merged / pr_closed / pr_reopened on PR close/reopen. octo-pr-feed.yml already triggers on pull_request_target: types: [opened, closed, reopened] and forwards event_action + pr_merged to the reusable feed workflow, which renders the closed/merged/reopened states. The lifecycle-notification responsibility is therefore preserved, just no longer duplicated. This resolves the event overlap described in the PR summary rather than dropping coverage.

✅ feed_group_id is a valid input. The reusable Mininglamp-OSS/.github/.github/workflows/octo-pr-result-notify.yml@main declares feed_group_id as an optional string input (default 1c303c14...). Passing it explicitly is backward compatible and does not break the workflow_call contract.

✅ Routing is consistent across the trio. The feed_group_id value 9ea115c7462b4b45b8c85d07d07e0dde matches the project_group_id used in both octo-pr-feed.yml and octo-pr-review-feed.yml, so review results now route to the same project group as the other notifications. This aligns with the stated canonical design.

✅ review-result job is unchanged otherwise. Its if guard (event name, review state, head-repo-equals-base to exclude forks) and all other inputs remain intact. permissions: {} is retained.

Findings

No P0/P1 issues. The change is a focused, correct deduplication of notification responsibilities.

Nits / suggestions (non-blocking)

• Magic group ID. 9ea115c7462b4b45b8c85d07d07e0dde is now hardcoded in three workflows in this repo. It is intrinsic to GitHub Actions YAML (no native variable sharing across workflow files without repo/org variables), so this is acceptable, but if these are templated across all 17 repos, consider sourcing the project group from a repo-level Actions variable (vars.OCTO_PROJECT_GROUP_ID) to avoid per-repo drift on future changes.
• Consistency check across repos. The PR description states this design is applied identically to all 17 repos. That cross-repo uniformity can't be verified from this single-repo diff; worth a spot-check that octo-pr-feed.yml on each repo indeed carries [opened, closed, reopened] so none lose close/reopen notifications after this job is removed.

Verdict

APPROVED. The removed job's coverage is fully absorbed by octo-pr-feed.yml, the new input is contract-safe and consistent with sibling workflows, and there are no correctness or security concerns.

[Jerry-Xin]

The PR is relevant to this repository because it changes repository-owned GitHub Actions PR notification workflows, but it currently breaks workflow parsing.

🔴 Blocking

• 🔴 Critical: .github/workflows/octo-pr-result-notify.yml contains OCTO_BOT_TOKEN: *** secrets.OCTO_BOT_TOKEN }} instead of a valid GitHub Actions expression. This is invalid YAML because *** is parsed as a malformed YAML alias. The same issue appears at line 43. This will prevent the workflow from loading at all. Restore both lines to:

  OCTO_BOT_TOKEN: ${{ secrets.OCTO_BOT_TOKEN }}

💬 Non-blocking

No additional non-blocking issues found in the changed workflow ownership model.

✅ Highlights

• Removing the reopened trigger from octo-pr-result-notify.yml aligns with the stated terminal-state-only responsibility.
• Deleting octo-pr-feed.yml appears consistent with the stated no-overlap notification design, assuming opened/reopened PR feed events are intentionally no longer sent.

[yujiawei]

Code Review — PR #204 (octo-server)

Verdict: CHANGES_REQUESTED — one P0 build-break plus a P1 notification regression.

1. Summary of what the PR actually does

The diff is not what the PR description claims. The description table states octo-pr-feed.yml is kept with types: [opened, closed, reopened]. In reality:

• octo-pr-feed.yml is deleted entirely.
• octo-pr-result-notify.yml trigger narrowed from [closed, reopened] to [closed], and event_kind simplified.
• octo-pr-review-feed.yml is unchanged.

The implementation and the description disagree; the description must be corrected before merge.

2. Findings

P0 — Broken YAML: workflow will not load (build-break)

octo-pr-result-notify.yml lines 26 and 43:

      OCTO_BOT_TOKEN: *** secrets.OCTO_BOT_TOKEN }}

This should be:

      OCTO_BOT_TOKEN: ${{ secrets.OCTO_BOT_TOKEN }}

The ${{ opener was corrupted to *** (this looks like committed output from a secret-masked gh pr diff copy/paste). YAML interprets *** as an alias reference, so the file no longer parses.

Evidence:

• Local parse fails: ScannerError: while scanning an alias ... expected alphabetic or numeric character, but found '*' (line 26, col 24).
• The repo's own sanity / actionlint check is failing on this PR with:
  .github/workflows/octo-pr-result-notify.yml: could not parse as YAML: yaml: unknown anchor '**' referenced [syntax-check]
• The parent commit adf928b has the correct ${{ secrets.OCTO_BOT_TOKEN }} on both lines, so this is a regression introduced by this PR.

Both occurrences (lines 26 and 43) must be restored to ${{ secrets.OCTO_BOT_TOKEN }}. Once fixed, the merged/closed and review-result notifications will be able to authenticate again; in the current state neither job can run.

P1 — opened (and reopened) PR events lose all notification coverage

Before this PR, octo-pr-feed.yml fired on pull_request_target: [opened, closed, reopened] and sent to the project group + global pr-feed. Deleting it removes the only handler for the opened event. The remaining workflows do not cover it:

• octo-pr-review-feed.yml fires only on [ready_for_review, review_requested].
• octo-pr-result-notify.yml now fires only on [closed] + pull_request_review.

Net effect: a PR opened directly as non-draft with no reviewer requested produces no feed notification at all. ready_for_review only fires on draft→ready transitions, and review_requested only when a reviewer is explicitly added — neither covers the plain "PR opened" case. reopened is likewise no longer notified anywhere.

If dropping "opened/reopened" notifications is the intended design, please state that explicitly in the PR description and confirm with the channel owners; right now it reads as an accidental loss of coverage hidden behind a description that claims the opposite.

P2 — PR description is misleading

The description's canonical-design table (and the "applied identically to all 17 repos" claim) does not match this diff: it shows octo-pr-feed.yml retained, but the diff deletes it. Since this PR is the template for 16 other repos, an inaccurate description risks propagating the wrong mental model. Please align the description with the actual change (feed.yml removed; result-notify is the single closed/merged handler).

3. Recommendations

1. Replace *** secrets.OCTO_BOT_TOKEN }} with ${{ secrets.OCTO_BOT_TOKEN }} on lines 26 and 43 of octo-pr-result-notify.yml. Re-run sanity / actionlint and confirm it passes.
2. Decide and document the intended behavior for PR opened/reopened. If those notifications should be kept, retain a handler for them; if they are intentionally dropped, say so in the description.
3. Correct the PR description table to reflect the deletion of octo-pr-feed.yml.

4. Additional notes

• The pr-result job's event_kind was simplified to merged == true && 'pr_merged' || 'pr_closed'. With the trigger now [closed] only, this is correct — reopened can no longer reach this expression, so there is no missing branch here (consistent with the narrowed trigger).
• octo-pr-review-feed.yml and its OCTO_BOT_TOKEN reference are intact and parse cleanly; the corruption is isolated to octo-pr-result-notify.yml.
• The other unrelated CI failure on this PR (check-sprint) appears to stem from a PROJECT_TOKEN/Projects-API issue and is outside the scope of these workflow changes; flagging it only so it is not mistaken for fallout from this diff.

[Jerry-Xin]

One-line summary: The PR is in scope for this repository and the workflow ownership change is consistent with the stated notification design.

💬 Non-blocking

• 🔵 Suggestion: .github/workflows/labeler.yml:11 still references octo-pr-feed.yml, which this PR deletes. Consider updating that comment to reference the remaining metadata-only pull_request_target workflows, such as auto-add-to-project.yml and octo-pr-review-feed.yml, to avoid stale documentation.

✅ Highlights

• 🔵 The project relevance gate passes: these are repository CI/notification workflow changes for Mininglamp-OSS/octo-server.
• 🔵 .github/workflows/octo-pr-result-notify.yml:5 now only handles terminal PR close events, and .github/workflows/octo-pr-result-notify.yml:21 correctly maps the remaining event space to pr_merged or pr_closed.
• 🔵 Deleting .github/workflows/octo-pr-feed.yml removes the overlapping closed/reopened feed path and leaves project review-feed notifications under .github/workflows/octo-pr-review-feed.yml:5.

[yujiawei]

Code Review — PR #204 (octo-server)

Summary

This PR finalizes the PR-notification workflow design with a clean, no-overlap ownership split:

• octo-pr-result-notify.yml — trigger narrowed from [closed, reopened] to [closed]; event_kind simplified to merged == true && 'pr_merged' || 'pr_closed'; routes to the global pr-feed (no feed_group_id override → reusable workflow default).
• octo-pr-feed.yml — deleted (explicitly "not needed" per the design).
• octo-pr-review-feed.yml — unchanged: handles [ready_for_review, review_requested] → project group.

Net diff at the reviewed head (f48c8e2): +2 / −29 across 2 files. No application code touched.

Verification

✅ YAML is valid / build is not broken. An earlier revision of this PR carried a corrupted *** secrets.OCTO_BOT_TOKEN }} expression that failed YAML parsing. The reviewed head commit (f48c8e2, "correct OCTO_BOT_TOKEN secret expression") restores ${{ secrets.OCTO_BOT_TOKEN }} on both jobs (lines 26 and 43). Local parse succeeds and the repo's own sanity / actionlint check passes on this PR.

✅ event_kind logic is correct for the narrowed trigger. With the pr-result job now firing only on [closed], merged == true && 'pr_merged' || 'pr_closed' covers both terminal states; the previously-needed reopened branch is correctly gone since reopened can no longer reach this expression.

✅ Routing to global pr-feed is contract-safe. The reusable Mininglamp-OSS/.github/.github/workflows/octo-pr-result-notify.yml@main declares feed_group_id as an optional input defaulting to 1c303c14… (the global pr-feed group). Dropping the override is backward compatible and matches the stated design (result/review notifications → global feed).

✅ Clean deletion of octo-pr-feed.yml. No remaining functional reference to the deleted workflow exists in .github/ (only a descriptive comment in labeler.yml still names it — see nit below).

✅ Security posture unchanged. permissions: {} retained on both workflows. The review-result job keeps its head.repo.full_name == github.repository guard to exclude forked PRs before OCTO_BOT_TOKEN is used. The review-event handler uses pull_request_review, which is the appropriate trigger here.

Findings

No P0/P1 issues at the reviewed head SHA.

Nits / suggestions (non-blocking)

• opened / reopened feed notifications are dropped — confirm this is intended. With octo-pr-feed.yml deleted, the final design has no handler for the plain "PR opened" or "PR reopened" lifecycle events: octo-pr-review-feed.yml fires only on ready_for_review / review_requested, and octo-pr-result-notify.yml now fires only on closed + review submissions. A non-draft PR opened with no reviewer requested will produce no feed notification. The PR description states octo-pr-feed.yml is "not needed," so this reads as a deliberate design decision — please just confirm the channel owners are aligned, since this is the template propagated to the other repos.
• Stale comment reference. .github/workflows/labeler.yml:11 still cites octo-pr-feed.yml as a precedent ("the pattern used by auto-add-to-project.yml and octo-pr-feed.yml"). Harmless, but worth updating the comment to avoid pointing at a deleted file.
• Magic group ID. 9ea115c7462b4b45b8c85d07d07e0dde remains hardcoded in octo-pr-review-feed.yml. Acceptable given GitHub Actions has no native cross-file variable sharing, but a repo/org-level Actions variable (vars.OCTO_PROJECT_GROUP_ID) would reduce per-repo drift if this is templated across repos.

Verdict

APPROVED. The previously-flagged YAML build break is resolved at this head SHA, the trigger/event_kind simplification is correct, routing is contract-safe, and the description now matches the diff. The only open item — dropping opened/reopened feed notifications — is an explicitly documented design choice, not a defect, so it does not block merge.

[Jerry-Xin]

The PR is in scope for this repository, but the current workflow change can break PR result notifications instead of using the reusable workflow’s default global feed.

🔴 Blocking

🔴 Critical — .github/workflows/octo-pr-result-notify.yml:25 and .github/workflows/octo-pr-result-notify.yml:43 pass feed_group_id: ${{ vars.OCTO_PR_FEED_GROUP_ID }}. This contradicts the stated design of removing the explicit override and using the reusable workflow default global pr-feed. If OCTO_PR_FEED_GROUP_ID is unset or empty, the reusable workflow receives an explicit empty value rather than applying its default, and its group-id validation fails. Remove these two feed_group_id lines unless this repository variable is intentionally required and documented.

✅ Highlights

The deleted .github/workflows/octo-pr-feed.yml is consistent with the stated ownership split, avoiding duplicate closed-event notifications.

The event_kind simplification on .github/workflows/octo-pr-result-notify.yml:21 is valid after narrowing pull_request_target to types: [closed].

[yujiawei]

Code Review — PR #204 (octo-server)

Summary

This PR finalizes the PR-notification workflow design by:

• Narrowing octo-pr-result-notify.yml's pull_request_target trigger from [closed, reopened] to [closed].
• Simplifying the event_kind expression accordingly.
• Adding feed_group_id: ${{ vars.OCTO_PR_FEED_GROUP_ID }} to both jobs in octo-pr-result-notify.yml.
• Deleting octo-pr-feed.yml.

The net change is small (2 files), but it has one blocking correctness risk and a stale description.

Blocking

P1 — feed_group_id now depends on a repo/org variable that is not configured

octo-pr-result-notify.yml:25 and :43 were changed to:

feed_group_id: ${{ vars.OCTO_PR_FEED_GROUP_ID }}

The reusable workflow Mininglamp-OSS/.github/.github/workflows/octo-pr-result-notify.yml declares:

feed_group_id:
  type: string
  required: false
  default: '1c303c142e9840f2a9b46c10b0972e8d'

and validates it at runtime:

def require_group_id(name):
    val = os.environ.get(name, '').strip()
    if not val:
        print(f'ERROR: required env var {name} is missing'); sys.exit(2)
    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)
    return val
...
feed_group_id = require_group_id("FEED_GROUP_ID")

Problem: when a caller passes a with: input explicitly, GitHub uses the provided value even if it is empty — the reusable workflow's default only applies when the key is omitted entirely. So if vars.OCTO_PR_FEED_GROUP_ID is undefined, an empty string is passed, which overrides the 1c303… default and causes require_group_id to sys.exit(2). Every pr_merged / pr_closed / review_approved / review_changes_requested notification job would then fail.

I verified that OCTO_PR_FEED_GROUP_ID is not defined as a repository variable on Mininglamp-OSS/octo-server:

$ gh api repos/Mininglamp-OSS/octo-server/actions/variables
{"variables":[],"total_count":0}
$ gh api repos/Mininglamp-OSS/octo-server/actions/variables/OCTO_PR_FEED_GROUP_ID
HTTP 404 Not Found

I could not verify the organization-level variable (the token lacks org actions variables read permission, HTTP 403). So this is conditional:

• If OCTO_PR_FEED_GROUP_ID is set at the org level → the PR works as intended.
• If it is not set anywhere → notifications silently break (job fails; no message sent).

Requested action: before merge, please either (a) confirm the variable is configured at org or repo scope and paste evidence, or (b) drop the explicit feed_group_id override and rely on the reusable workflow's default. Note that the PR description's stated goal — "uses org default = global pr-feed" — is actually achieved by omitting feed_group_id, not by passing a possibly-empty variable. Passing vars.OCTO_PR_FEED_GROUP_ID is strictly riskier than the omission the description claims.

P2 — PR description does not match the head commit

The description says: "removed explicit feed_group_id override (uses org default = global pr-feed)". That matched an intermediate commit (2ea70dd), but the head commit 1c6cb10 ("use org variable OCTO_PR_FEED_GROUP_ID for feed_group_id") re-added the override via a variable. Please update the description so reviewers and future readers aren't misled about what is actually being merged.

Non-blocking observations

• reopened notifications are dropped. Removing reopened from the trigger means reopened PRs no longer notify; the reusable workflow's pr_reopened branch is now dead for this caller. This appears intentional ("terminal state only") — calling it out for confirmation.
• "opened" PR notifications to the project group are gone. The deleted octo-pr-feed.yml fired on [opened, closed, reopened] to project_group_id: 9ea115c7…. After this PR, octo-pr-review-feed.yml covers [ready_for_review, review_requested] and octo-pr-result-notify.yml covers [closed] (to the global feed). The plain opened event no longer produces any notification. Per the PR's design table this looks intentional, but please confirm dropping opened-event notifications is desired.
• event_kind simplification is correct. With the trigger reduced to [closed], ${{ github.event.pull_request.merged == true && 'pr_merged' || 'pr_closed' }} correctly distinguishes merged vs. closed; the previous three-way expression is no longer needed.

Verdict

Requesting changes on the feed_group_id variable dependency (P1) — the feature this PR is meant to "finalize" will break if the variable is unconfigured, and I confirmed it is absent at the repository level. Resolving that (confirm the org variable, or drop the override) plus correcting the description clears the path to merge.