fix(ci): update octo-pr-result-notify.yml (closed-only trigger + OCTO_PR_FEED_GROUP_ID)

[lml2468]

Summary

Updates octo-pr-result-notify.yml to align with the finalized PR notification design.

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 with hardcoded fallback)
octo-pr-review-feed.yml	pull_request_target: [ready_for_review, review_requested]	project group (repo-specific)

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 route to the global pr-feed group via org-level variable OCTO_PR_FEED_GROUP_ID

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 || '1c303c142e9840f2a9b46c10b0972e8d' }} — org-level variable with safe fallback to prevent empty-string override of the reusable workflow's built-in default
  • event_kind expression simplified to pr_merged | pr_closed (removed pr_reopened case)

No application code modified.

[lml2468]

Summary

Clean thin caller for the org-wide octo-pr-feed.yml reusable workflow. No application code touched.

Findings

No blocking issues.

Security:

• pull_request_target with permissions: {} and metadata-only inputs — no PR code checkout, safe ✅
• zizmor: ignore[dangerous-triggers] annotation correctly documents the intent
• Draft PRs filtered out via !github.event.pull_request.draft ✅

Consistency:

• All 10 workflow_call inputs match the reusable workflow's declared schema (verified against Mininglamp-OSS/.github/.github/workflows/octo-pr-feed.yml@main) ✅
• feed_group_id uses the reusable's default (1c303c142e9840f2a9b46c10b0972e8d), so not passed explicitly — correct, avoids drift ✅
• project_group_id hardcoded to this repo's group — expected pattern ✅
• branches: [main] scoping consistent with other workflow callers ✅

Relationship to prior PRs:

• PR feat: add pr-review-feed and pr-result-notify caller workflows #50 added octo-pr-result-notify.yml (review-feed with detailed review context)
• PR fix: restrict review-feed to ready_for_review and review_requested #51 restricted review-feed triggers to ready_for_review / review_requested
• This PR adds the separate lifecycle feed (opened/closed/reopened) — no overlap ✅

CI: Build ✅, actionlint ✅, no-tabs ✅ (Test/Vet pending but no Go code changed)

Verdict

APPROVED — minimal, correctly scoped workflow caller with proper security posture.

[lml2468]

[APPROVE]

Clean, minimal caller workflow — metadata-only pull_request_target with permissions: {} and no run: steps.

Security checklist:

• ✅ permissions: {} — no token permissions granted to caller
• ✅ No run: steps — no script injection surface in this file
• ✅ pull_request_target justified: metadata-only automation, no PR code checkout
• ✅ User-controlled pr_title passed via with: input (safe caller pattern; injection responsibility is on the reusable workflow)
• ✅ Draft PRs skipped

🔵 Suggestion (non-blocking):

1. .github/workflows/octo-pr-feed.yml:13 — Reusable workflow is referenced at @main rather than a pinned SHA. For org-internal repos this is a common and acceptable trade-off, but SHA pinning would harden against supply-chain compromise of the .github repo.

✅ Highlights:

• Exactly what a caller workflow should look like: thin, no permissions, no code execution
• Consistent with the existing octo-pr-result-notify.yml pattern in this repo

[yujiawei]

Code Review — PR #53 (octo-lib)

Summary

This PR adds a single caller workflow (.github/workflows/octo-pr-feed.yml, 27 lines) that forwards PR lifecycle events to the org-shared reusable workflow Mininglamp-OSS/.github/.github/workflows/octo-pr-feed.yml@main, which posts a notification to the Octo IM pr-feed and project channels. The change is well-scoped and security-aware.

Verification

• ✅ pull_request_target usage is justified. The trigger is required (not pull_request) because fork PRs need access to secrets.OCTO_BOT_TOKEN to reach the IM API. The danger of pull_request_target is running untrusted PR code with secrets — but neither the caller nor the reusable workflow checks out or executes PR code; only event metadata is passed. The zizmor: ignore[dangerous-triggers] annotation with its justification is accurate.
• ✅ Least privilege. permissions: {} at the top level grants the job no GITHUB_TOKEN scopes. Correct for a metadata-only notifier.
• ✅ Injection-safe input handling. Untrusted fields (pr_title, pr_author) flow caller with: → reusable env: → os.environ in the Python step. Passing via env vars (not string-interpolating into run:) is the recommended pattern and avoids shell/script injection. The reusable side additionally calls sanitize_text() to strip C0/DEL control characters, preventing IM message-body injection.
• ✅ Defense in depth downstream. The reusable workflow validates repo_name (anti path-traversal), group IDs (32-char hex), and pins the IM API base to an allowlist. Nothing in this caller weakens those checks.
• ✅ Draft handling. if: ${{ !github.event.pull_request.draft }} correctly suppresses notifications for draft PRs.
• ✅ Event coverage is coherent. types: [opened, closed, reopened] with branches: [main]. closed covers both merge and non-merge outcomes; pr_merged disambiguates them downstream (emoji 🟢/🔴).

Findings

No P0 (correctness/security/data-loss/build-break) or P1 (functional blocker) issues found.

Suggestions (non-blocking)

• (P2) Pin the reusable workflow to a commit SHA. The reference ...octo-pr-feed.yml@main tracks a mutable branch. Pinning to a SHA (e.g. @<sha>) is the supply-chain-hardening best practice, since the reusable workflow holds the IM token and runs in a pull_request_target context. Mutable @main is a common and accepted convention for an org-internal .github reusable workflow, so this is a judgment call rather than a defect — flagging for awareness.
• (P2 / nit) No notification when a draft becomes ready. Drafts are skipped at opened, and ready_for_review is not in types, so a PR that starts as a draft and is later marked ready never produces an "opened"-style notification. If silent draft promotion is intended, this is fine; if not, consider adding ready_for_review to types (the reusable workflow already maps that action to a ✅ emoji).

Verdict

No blocking issues. The workflow follows GitHub Actions security best practices for pull_request_target notifiers. Approving; the two P2 items are optional hardening/UX improvements at the author's discretion.

[Jerry-Xin]

This workflow is project-relevant, but it introduces duplicate PR lifecycle notifications for existing closed and reopened events.

🔴 Blocking

🔴 Critical — .github/workflows/octo-pr-feed.yml:5 adds closed and reopened triggers, but .github/workflows/octo-pr-result-notify.yml:4-5 already runs on pull_request_target for those same actions. This means a closed or reopened PR will now trigger both workflows. Since this PR’s stated missing coverage is opened, the new caller should avoid duplicating existing lifecycle notifications, or the existing result workflow should be adjusted in the same PR so each event is emitted once.

💬 Non-blocking

🟡 Warning — .github/workflows/octo-pr-feed.yml:13 pins the reusable workflow to @main, matching existing repo patterns, but it means notification behavior can change without a corresponding change in this repository. A pinned SHA would be safer if reproducibility is required.

✅ Highlights

The PR passes the project relevance gate: GitHub workflow maintenance belongs in this repository.

The workflow follows existing local conventions: permissions: {}, reusable workflow caller style, metadata-only pull_request_target, and no checkout or PR code execution.

[Jerry-Xin]

This PR is project-relevant: it updates repository CI/notification automation, which is part of this repo’s operational surface.

💬 Non-blocking

🟡 Warning: .github/workflows/octo-pr-result-notify.yml now passes the project group ID through an input named feed_group_id. That matches the reusable workflow interface, but the naming is easy to misread as the global feed channel. A short comment would reduce future confusion.

✅ Highlights

🔵 Suggestion: The new .github/workflows/octo-pr-feed.yml follows the existing metadata-only pull_request_target pattern and does not check out or execute PR code.

🔵 Suggestion: .github/workflows/octo-pr-feed.yml keeps default permissions empty, consistent with the repo’s other reusable workflow callers.

I checked the local workflow syntax with Ruby YAML parsing; both changed workflow files parse successfully.

[yujiawei]

Code Review — PR #53 (octo-lib)

Verdict: APPROVED

A clean, well-scoped CI-only change. It adds the octo-pr-feed.yml caller (PR opened/closed/reopened → global pr-feed + project group) and removes the now-redundant pr-result job from octo-pr-result-notify.yml. No application code is touched. No P0/P1 issues found.

1. Correctness

✅ Reusable contract satisfied. The caller supplies every required input of the reusable octo-pr-feed.yml@main (repo_name, pr_number, pr_title, pr_url, pr_author, event_action, project_group_id) and the relevant optional stats. feed_group_id is intentionally omitted so the reusable's default (the global pr-feed group) is used, while project_group_id is passed explicitly — so each event fans out to both the global feed and the project group, exactly as the summary describes.

✅ No duplicate notifications. The old pr-result job (which fired on closed/reopened) is removed in the same diff, so lifecycle events are not double-sent. Other PR-triggered workflows in the repo cover disjoint event types (octo-pr-review-feed.yml → ready_for_review/review_requested; octo-pr-result-notify.yml now → pull_request_review.submitted only), so there is no overlap on opened/closed/reopened.

✅ Lifecycle coverage is additive, not lossy. Previously the deleted pr-result job sent closed/reopened/merged to the global feed only (it passed no feed_group_id). The new workflow keeps the global feed and additionally delivers to the project group, plus adds the previously-uncovered opened event. No regression for lifecycle notifications.

2. Security

✅ pull_request_target is used safely. permissions: {} at workflow level, no checkout of PR head code, and only trusted metadata fields are forwarded to a same-org reusable. The zizmor: ignore[dangerous-triggers] annotation is justified and matches the existing pattern in labeler.yml / pr-contributor-welcome.yml. The reusable independently sanitizes free-text fields (strips C0 control chars to prevent IM-message injection), validates repo_name against path traversal, validates group IDs as 32-char hex, and allowlists the API base URL. Good defense in depth.

3. Observations (non-blocking — nits / clarifications)

• (P2) Undocumented behavior change: review events leave the global feed. Adding feed_group_id: "6095b6ded00046009e72882ce4067289" to the review-result job in octo-pr-result-notify.yml redirects review_approved / review_changes_requested notifications from the global pr-feed (the previous default) to the project group only. The global pr-feed will no longer receive review-state events. This looks like an intentional split (global feed = PR lifecycle; project group = reviews + lifecycle), but it is not mentioned in the PR summary — please confirm it's intended.

• (P2) Summary wording is imprecise. The description states "octo-pr-result-notify.yml continues to handle these [closed/reopened] for the project group with more detailed review context." In the diff that job is deleted; closed/reopened are now handled by the new octo-pr-feed.yml for both groups. Net behavior is fine (and arguably better), but the description's mechanism is inaccurate — worth correcting so future readers aren't misled.

• (P2) Draft-opened PRs never produce a global pr-feed "opened" entry. The if: ${{ !github.event.pull_request.draft }} guard means a PR opened as a draft is skipped for the opened event; when it later transitions to ready, that's a ready_for_review event handled by octo-pr-review-feed.yml (project group), not by this feed. So such PRs first appear on the global pr-feed only at close/merge. This is consistent with the existing review-feed convention, so likely acceptable — flagging only as a known coverage gap, not a defect.

• (P2 / nit) Reusable is pinned to @main, not a SHA. Standard supply-chain hardening would pin the called reusable to a commit SHA. However this matches the existing convention across all callers in this repo, so changing it here alone would be inconsistent — out of scope for this PR.

4. Conclusion

The change is correct, secure, and achieves its stated goal of activating the global pr-feed channel without regressing existing notifications. The items above are clarifications/suggestions, none of which block merge.

[Jerry-Xin]

This PR is in scope for Mininglamp-OSS/octo-lib, but the changed workflow is currently invalid and cannot be merged as-is.

🔴 Blocking

• 🔴 Critical: .github/workflows/octo-pr-result-notify.yml:26 and .github/workflows/octo-pr-result-notify.yml:43 replaced the GitHub Actions expression with the literal text *** secrets.OCTO_BOT_TOKEN }}. This is invalid YAML because *** is parsed as a malformed alias, so GitHub will not load the workflow at all. I also verified this locally with a YAML parser, which fails at line 26. These lines should remain:

  OCTO_BOT_TOKEN: ${{ secrets.OCTO_BOT_TOKEN }}

✅ Highlights

• The PR is relevant to this repository because it updates repository CI/notification workflow ownership.
• Removing the reopened trigger and simplifying event_kind is consistent with the stated terminal-state-only design once the YAML syntax is fixed.

[yujiawei]

Code Review — PR #53 (octo-lib)

Summary

This PR finalizes the PR-notification design in .github/workflows/octo-pr-result-notify.yml:

• drops the reopened trigger (now only closed),
• simplifies the event_kind expression accordingly.

The intent is sound, but the committed file contains a syntax-breaking corruption in both secrets: blocks that must be fixed before merge.

Blocking issue (P0)

Invalid YAML — ${{ replaced by *** in both OCTO_BOT_TOKEN secret mappings.

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

    secrets:
      OCTO_BOT_TOKEN: *** secrets.OCTO_BOT_TOKEN }}

The expression should be ${{ secrets.OCTO_BOT_TOKEN }}. In YAML, a bare * begins an alias, so *** secrets.OCTO_BOT_TOKEN }} is a scanner error:

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

Impact:

• The workflow file no longer parses as valid YAML — GitHub Actions will flag it as an invalid workflow and it will not run. The whole notification pipeline this PR is meant to "finalize" is dead on main after merge.
• Even if it parsed, the secret would no longer be forwarded to the reusable workflow, so OCTO_BOT_TOKEN would be empty.

This looks like masked tool output (where secrets render as ***) was pasted back into the source file. The base branch (main) is correct:

      OCTO_BOT_TOKEN: ${{ secrets.OCTO_BOT_TOKEN }}

Fix: restore both lines (26 and 43) to OCTO_BOT_TOKEN: ${{ secrets.OCTO_BOT_TOKEN }}.

Non-blocking observations

The two intentional logic changes are correct:

• Trigger narrowed to [closed] — removing reopened is a deliberate design choice; no leftover references to the reopen path remain in this file.
• event_kind simplification is logically equivalent given the narrowed trigger. Since the job now only runs on pull_request_target with action == 'closed', the previous github.event.action == 'closed' guards were always true, so collapsing to github.event.pull_request.merged == true && 'pr_merged' || 'pr_closed' produces the same pr_merged / pr_closed values. The dropped pr_reopened branch is consistent with removing the reopened trigger. The reusable workflow in Mininglamp-OSS/.github still accepts pr_reopened in its ALLOWED_KINDS, but it is harmless that the caller no longer emits it.

Verdict

CHANGES_REQUESTED — the YAML corruption on lines 26 and 43 is a build/functional breakage and must be corrected. Once both secret lines are restored to ${{ secrets.OCTO_BOT_TOKEN }}, the change is good to merge.

[Jerry-Xin]

One-line summary: The PR is relevant to this repository and cleanly narrows PR result notifications to terminal close events while preserving review-result notifications.

💬 Non-blocking

• 🔵 Suggestion: .github/workflows/labeler.yml:11 still references octo-pr-feed.yml in a comment, but that workflow is no longer present. This is not caused by the changed file, but updating the comment would reduce future confusion.

✅ Highlights

• .github/workflows/octo-pr-result-notify.yml:4-7: The trigger split remains clear: PR close events use pull_request_target, while submitted reviews continue through pull_request_review.
• .github/workflows/octo-pr-result-notify.yml:21: Simplifying event_kind is correct now that reopened no longer triggers this workflow.
• .github/workflows/octo-pr-result-notify.yml:29-32: Review-result notifications still filter to approved / changes-requested reviews and same-repository PRs, which avoids exposing the bot secret path to fork review events.

[yujiawei]

Code Review — PR #53 (octo-lib)

Verdict: APPROVED

Clean, minimal, correctly-scoped CI change. No blocking issues found.

Summary of net effect

Although the PR history touches octo-pr-feed.yml and an OCTO_BOT_TOKEN expression, the net diff against main is just 2 lines in .github/workflows/octo-pr-result-notify.yml:

1. on.pull_request_target.types: [closed, reopened] → [closed]
2. event_kind expression simplified to ${{ github.event.pull_request.merged == true && 'pr_merged' || 'pr_closed' }}

The octo-pr-feed.yml file added earlier in the branch is deleted again (it never existed on main), and the malformed secret expression introduced mid-branch was fixed in the head commit — both net to zero. The final tree is clean.

Verification

Check	Result
YAML parses, jobs/triggers intact (pr-result, review-result)	✅
event_kind logic correct after dropping reopened	✅ — with pull_request_target now firing only on closed, distinguishing merged → pr_merged else pr_closed is exhaustive; the old pr_reopened branch is correctly removed as dead code
No dangling references to a deleted workflow	✅ — octo-pr-feed.yml never existed on main; nothing references it as a caller
OCTO_BOT_TOKEN secret expression valid in final state	✅ — ${{ secrets.OCTO_BOT_TOKEN }} on both jobs
Ownership split with sibling octo-pr-review-feed.yml	✅ — no trigger overlap ([closed] + review-submitted vs [ready_for_review, review_requested])
Job guards (if:) gate each job to the correct event	✅ — pr-result only on pull_request_target; review-result only on approved/changes_requested reviews from same-repo head

Behavioral note (non-blocking)

Dropping reopened means reopening a PR no longer emits a notification. This is an intentional design decision per the PR description ("terminal state only"), not a defect.

Nits (non-blocking, not introduced by this PR)

• labeler.yml:11 contains a comment referencing octo-pr-feed.yml, a file that does not exist on main. This is pre-existing and outside this PR's diff, but since this PR finalizes away the octo-pr-feed concept, a follow-up could clean up that stale comment.

No changes required for merge.

[Jerry-Xin]

The PR is in scope, but the current diff breaks the intended default global PR feed routing.

🔴 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 }} to the reusable workflow. This overrides the reusable workflow’s built-in default global PR feed. If OCTO_PR_FEED_GROUP_ID is unset, the callee receives an empty string and fails its group-id validation instead of using the default. This also contradicts the PR description’s stated design of removing the explicit override and using the default global pr-feed. Remove both feed_group_id lines so the reusable workflow default is used.

💬 Non-blocking

• 🔵 Suggestion: .github/workflows/labeler.yml:11 still references octo-pr-feed.yml in a comment. If that workflow is now intentionally gone, update the comment in a follow-up to avoid stale operational documentation.

✅ Highlights

• The PR correctly narrows pull_request_target result notifications to the terminal closed event.
• The review-result job continues to filter fork PRs before using notification secrets.

[yujiawei]

Code Review — PR #53 (octo-lib)

Summary

This change finalizes the PR-result notification workflow:

• octo-pr-result-notify.yml: pull_request_target trigger narrowed from [closed, reopened] → [closed] (terminal state only).
• event_kind expression simplified from a 3-way (pr_merged / pr_closed / pr_reopened) to a 2-way (pr_merged / pr_closed), consistent with dropping the reopened trigger.
• feed_group_id: ${{ vars.OCTO_PR_FEED_GROUP_ID }} added to both the pr-result and review-result callers.

No application code is touched. Net diff is +4 / -2 in one file.

Verification

• Trigger / event_kind consistency ✅ — With reopened removed from the trigger, the old expression's pr_reopened branch was unreachable, so collapsing to merged == true ? 'pr_merged' : 'pr_closed' is correct and produces only event_kind values in the reusable workflow's ALLOWED_KINDS set.
• Runtime behavior at head SHA ✅ — The latest pull_request_review run at 8acafc58 completed successfully: feed_group_id resolved to a valid 32-char hex group id and the notification step returned HTTP 200. The earlier failure run on the branch was at an older commit (74929d63) and is not present at the current head.
• Reusable input contract ✅ — feed_group_id is an optional input on the reusable workflow; the value passed satisfies its require_group_id hex validation.

Findings

P2 — PR description does not match the diff

The description states "removed explicit feed_group_id override" and "octo-pr-feed.yml: deleted". Neither is reflected in the net diff against main:

• The diff adds feed_group_id: ${{ vars.OCTO_PR_FEED_GROUP_ID }} in two places (lines added under both jobs), rather than removing an override.
• octo-pr-feed.yml does not exist on main or on the PR head, so there is nothing to delete. The commit history shows the file was added and then dropped within the branch, so the description appears to describe an intermediate branch state, not the final diff.

This is a documentation accuracy issue, not a functional defect, but it makes the PR harder to audit. Please align the description with the actual final diff before merge.

P2 — Explicitly passing feed_group_id duplicates the reusable workflow's default

The reusable octo-pr-result-notify.yml already defaults feed_group_id to the global pr-feed group (1c303c14…), and vars.OCTO_PR_FEED_GROUP_ID currently resolves to that same value. Passing it explicitly is therefore functionally a no-op today, but introduces two minor risks:

• Drift: the org var and the reusable default are now two independent sources of truth for the same group; if one is changed without the other, behavior diverges silently.
• Empty-string fragility: if OCTO_PR_FEED_GROUP_ID is ever unset, the caller passes an empty string (not an omitted input), which bypasses the reusable workflow's default and trips require_group_id → sys.exit(2), breaking notifications. An omitted input would instead fall back to the default safely.

If the intent is "use the global default," consider simply omitting feed_group_id here and relying on the reusable default. This also matches the description's stated intent of removing the override. Non-blocking — current behavior is correct because the var is defined.

Verdict

No P0/P1 issues. The workflow is internally consistent and verified working at the head SHA. The two findings above are P2 (PR description accuracy and a redundant/slightly fragile explicit input) and do not block 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]

Update: feed_group_id fallback added ✅

Addressed the blocking review concern: passing ${{ vars.OCTO_PR_FEED_GROUP_ID }} directly would override the reusable workflow's built-in default with an empty string if the variable is unset, causing require_group_id validation to fail.

Fix applied: changed to ${{ vars.OCTO_PR_FEED_GROUP_ID || '1c303c142e9840f2a9b46c10b0972e8d' }}

This ensures:

1. ✅ When org variable is set (current state) — uses OCTO_PR_FEED_GROUP_ID as configured
2. ✅ When org variable is unset — falls back to the global pr-feed group ID instead of passing empty string
3. ✅ Never bypasses the reusable workflow's safe default

The org-level variable OCTO_PR_FEED_GROUP_ID is confirmed to be set at org scope (per product owner). The fallback is a defensive measure only.

[Jerry-Xin]

Summary: The PR is relevant to this repository and the workflow changes align with the stated closed-only PR result notification split.

💬 Non-blocking

• 🟡 Warning: .github/workflows/octo-pr-result-notify.yml:25 and .github/workflows/octo-pr-result-notify.yml:43 now fall back to a hard-coded feed group ID when vars.OCTO_PR_FEED_GROUP_ID is unset. That prevents empty-string failures, but it also means org-level variable misconfiguration will be silently masked and notifications may still route to a baked-in channel. If central org configuration is the intended source of truth, consider removing the fallback or documenting that fallback behavior explicitly.

✅ Highlights

• The relevance gate passes: this is repository-owned CI notification configuration for Mininglamp-OSS/octo-lib.
• .github/workflows/octo-pr-result-notify.yml:4-7 correctly narrows PR result events to closed while preserving review submissions.
• .github/workflows/octo-pr-result-notify.yml:21 is simpler and still correct under the closed-only trigger.
• .github/workflows/octo-pr-result-notify.yml:30-33 keeps the review notification guard scoped to approval/change-request reviews from same-repository PRs, limiting secret exposure risk.

[yujiawei]

Code Review — PR #53 (octo-lib)

Verdict: APPROVED — scoped, correct CI-config change with no blocking issues.

Summary of changes

A single workflow file (.github/workflows/octo-pr-result-notify.yml, +4/-2):

1. pull_request_target trigger narrowed from [closed, reopened] → [closed].
2. event_kind expression simplified to ${{ github.event.pull_request.merged == true && 'pr_merged' || 'pr_closed' }} (dropped the pr_reopened case).
3. feed_group_id: ${{ vars.OCTO_PR_FEED_GROUP_ID || '1c303c142e9840f2a9b46c10b0972e8d' }} added to both the pr-result and review-result jobs.

Verification

• ✅ Trigger narrowing is consistent with the logic change. Once reopened is removed, github.event.action is always closed for the pull_request_target path, so the previous action == 'closed' && ... guards in event_kind were redundant. The simplified expression — merged → pr_merged, otherwise pr_closed — is logically equivalent for the events that still fire, and the now-unreachable pr_reopened branch is correctly dropped. No dead code left behind.
• ✅ Cross-repo input contract holds. The new feed_group_id is passed to the reusable workflow Mininglamp-OSS/.github/.github/workflows/octo-pr-result-notify.yml@main. I confirmed that reusable workflow declares feed_group_id as an optional workflow_call input (type: string, required: false) with the identical default '1c303c142e9840f2a9b46c10b0972e8d'. The caller therefore will not fail with an unknown-input error, and the fallback value matches the callee's default, so behavior is unchanged when vars.OCTO_PR_FEED_GROUP_ID is unset.
• ✅ Job guards unaffected. pr-result remains gated on github.event_name == 'pull_request_target' and review-result on the pull_request_review / same-repo conditions, so the new input is only evaluated in the correct contexts.

Non-blocking notes (nits — not required for merge)

• Default group ID duplicated in two places. The literal 1c303c142e9840f2a9b46c10b0972e8d now lives both here (as the || ... fallback) and as the callee's input default. Since the reusable workflow already defaults to the same value, the caller could omit the fallback (or even omit feed_group_id entirely when the org var is unset) and rely on the callee default — that would keep the canonical value in a single location and avoid future drift. Passing it explicitly is also defensible for readability; flagging only as a maintainability consideration.

No correctness, security, or build-breaking concerns. Approving.