Skip to content

chore(ci): add octo-pr-feed workflow to activate global pr-feed channel#202

Merged
lml2468 merged 1 commit into
mainfrom
chore/add-octo-pr-feed
May 30, 2026
Merged

chore(ci): add octo-pr-feed workflow to activate global pr-feed channel#202
lml2468 merged 1 commit into
mainfrom
chore/add-octo-pr-feed

Conversation

@lml2468
Copy link
Copy Markdown
Contributor

@lml2468 lml2468 commented May 30, 2026

Summary

Adds octo-pr-feed.yml β€” the final missing workflow from the org's standard CI suite.

What it does: Sends PR lifecycle events (opened / closed / reopened) to:

  1. This repo's dedicated Octo group (project_group_id)
  2. The global pr-feed monitoring channel (1c303c142e9840f2a9b46c10b0972e8d)

Why it was missing: The octo-pr-feed.yml reusable existed in Mininglamp-OSS/.github but no caller had ever been deployed to any repo, leaving the pr-feed channel empty.

Design note: opened events are not covered by any existing workflow. closed/reopened are also handled here (for the global pr-feed); octo-pr-result-notify.yml continues to handle these for the project group with more detailed review context.

No application code modified β€” thin caller only.

Adds octo-pr-feed.yml caller triggering on PR opened/closed/reopened events.
Sends notifications to both the repo's dedicated Octo group and the global
pr-feed channel (1c303c142e9840f2a9b46c10b0972e8d).

This workflow was the last missing piece of the org's standard CI suite β€”
without it, the pr-feed monitoring channel received no events.
@lml2468 lml2468 requested a review from a team as a code owner May 30, 2026 09:11
@github-actions github-actions Bot added the size/S PR size: S label May 30, 2026
Copy link
Copy Markdown
Contributor Author

@lml2468 lml2468 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[APPROVE]

Clean thin caller β€” no application code, no script injection risk.

βœ… Security Checklist

  • pull_request_target: trigger is appropriate since this is metadata-only notification automation (no PR code checked out or executed). zizmor: ignore annotation is correct.
  • No script injection: all user-controllable inputs (pr_title, pr_author, etc.) are passed as with: inputs to the reusable workflow, which binds them to env: variables and reads via os.environ.get() in Python β€” never interpolated into run: shell.
  • Reusable workflow sanitizes text (sanitize_text strips control chars), validates group IDs (32-char hex), validates repo name (no path traversal), and allowlists the API base URL.
  • permissions: {}: no GITHUB_TOKEN permissions requested β€” correct for a notification-only workflow.
  • Secret handling: OCTO_BOT_TOKEN passed through secrets:, consumed via env: in the reusable workflow β€” never logged or interpolated into shell.

βœ… Behavior

  • Triggers on opened, closed, reopened for non-draft PRs targeting main β€” correct scope.
  • Sends to both project group and global pr-feed channel β€” matches PR description.
  • Draft PRs excluded via if: ${{ !github.event.pull_request.draft }} β€” good.

No findings.

Copy link
Copy Markdown
Contributor

@yujiawei yujiawei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review β€” PR #202 (octo-server)

Verdict: APPROVED

A clean, well-scoped thin caller that adds the final missing workflow (octo-pr-feed.yml) from the standard CI suite. It follows the established sibling-caller patterns in this repo and the reusable contract in Mininglamp-OSS/.github. No P0/P1 issues found. One P2 note on notification overlap.

What it does

Forwards PR opened/closed/reopened lifecycle events to the reusable octo-pr-feed.yml, which notifies both the global pr-feed channel (default 1c30…2e8d) and this repo's project group (9ea1…0dde). Closes the gap where opened events were not covered by any existing workflow.

Security review βœ…

The pull_request_target trigger is the one thing that warrants scrutiny here, and it is handled correctly:

  • No untrusted code execution. The reusable callee does not check out the PR head; it only sends an IM notification. The zizmor: ignore[dangerous-triggers] annotation is justified.
  • Least privilege. permissions: {} grants the workflow's GITHUB_TOKEN no scopes. βœ…
  • No script injection. Attacker-controlled fields (pr_title, pr_author) are passed as with: inputs and consumed in the reusable as environment variables (os.environ.get), never interpolated into a shell run: line. The reusable additionally sanitize_text()s them (strips C0 control chars, length-caps) before building the message. βœ…
  • Input hardening downstream. The reusable validates group IDs (32-char hex) and the repo name (anti-path-traversal), and allowlists the API base URL. βœ…

Correctness βœ…

  • pr_merged: ${{ github.event.pull_request.merged }} is correctly passed so the callee can pick the 🟒 (merged) vs πŸ”΄ (closed-unmerged) emoji on closed.
  • if: ${{ !github.event.pull_request.draft }} mirrors octo-pr-review-feed.yml, so draft PRs don't spam the feed. Consistent.
  • project_group_id value matches the value used by the two sibling callers (octo-pr-review-feed.yml, octo-pr-result-notify.yml). Consistent.
  • feed_group_id is intentionally omitted to fall back to the reusable's shared default β€” acceptable.

P2 β€” Duplicate project-group notification on closed/reopened

The reusable always sends to both the feed group and the project group (send(feed_gid, …); send(proj_gid, …) β€” there is no feed-only toggle). Meanwhile octo-pr-result-notify.yml already fires on pull_request_target: [closed, reopened] and notifies the same project group. Net effect per event to the project group 9ea1…0dde:

  • opened β†’ 1 message (this PR fills the gap β€” good).
  • closed / reopened β†’ 2 messages: one from octo-pr-result-notify.yml (detailed) and one from this new feed caller.

The PR description acknowledges that octo-pr-result-notify.yml "continues to handle these for the project group," but in practice the project group now receives a duplicate, less-detailed feed message on every close/reopen. This is a noise/UX concern, not a correctness or security defect, and the current reusable design offers no clean way to suppress the project-group send from the feed path. Not a merge blocker.

Suggestions (any of these, non-blocking):

  • Scope this caller to types: [opened] only and let octo-pr-result-notify.yml own closed/reopened for the project group (the global feed would then miss close/reopen β€” confirm whether the pr-feed channel needs those).
  • Or add an optional feed-only mode to the reusable so a caller can target only the broadcast channel.
  • At minimum, capture this known overlap in a comment in the workflow file so a future maintainer doesn't treat the double-send as a bug.

Nits (non-blocking)

  • The branches: [main] base-branch filter is present here but absent on the sibling callers (octo-pr-result-notify.yml, octo-pr-review-feed.yml). The behavior (only PRs targeting main) is reasonable; just flagging the cross-file inconsistency in case org-wide uniformity is desired.

Copy link
Copy Markdown
Contributor

@Jerry-Xin Jerry-Xin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR is in scope for Mininglamp-OSS/octo-server: it adds repository CI/notification automation and does not touch unrelated project content.

πŸ’¬ Non-blocking

🟑 Warning β€” .github/workflows/octo-pr-feed.yml:5 adds closed and reopened handling, while .github/workflows/octo-pr-result-notify.yml:4-5 already handles those same PR lifecycle events. Based on the current reusable workflows, this can produce duplicate close/reopen notifications in the PR feed. If that noise is undesirable, consider limiting this new caller to opened or adding a reusable-workflow option to suppress one destination.

πŸ”΅ Suggestion β€” .github/workflows/octo-pr-feed.yml:12 skips draft PRs, so PRs opened as drafts will not emit an opened feed event. That matches the existing review-feed pattern, but it is worth confirming if the global feed is expected to track draft PR creation too.

βœ… Highlights

The workflow is a thin caller, uses permissions: {} at .github/workflows/octo-pr-feed.yml:8, does not check out or execute PR code under pull_request_target, and passes only PR metadata plus the bot secret to the reusable workflow. The project group ID is consistent with the existing Octo workflows.

@lml2468 lml2468 merged commit adf928b into main May 30, 2026
23 of 24 checks passed
@lml2468 lml2468 deleted the chore/add-octo-pr-feed branch May 30, 2026 14:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/S PR size: S

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants