feat(issue-feed): simplify to new/reopen only, remove project-group and IssueTriage#28
Conversation
…nd IssueTriage - Remove project_group_id input and the per-repo group notification. Only the shared issue-feed group receives messages going forward. - Remove IssueTriage bot mention/trigger (deprecated in favour of IssueTriage monitoring the issue-feed group directly). - Remove issue_labels input — label display is irrelevant when the labeled event is no longer forwarded. - Add early exit for any event_action not in (opened, reopened) so callers don't need to filter; the reusable itself enforces the policy. - Remove mention_uids from send() — no longer needed. - Remove require_group_id() helper — no longer needed.
Jerry-Xin
left a comment
There was a problem hiding this comment.
This PR is relevant to Mininglamp-OSS/.github, but it currently breaks existing reusable-workflow callers.
🔴 Blocking
- 🔴 Critical —
.github/workflows/octo-issue-feed.yml:8: removingissue_labelsandproject_group_idfrom theworkflow_call.inputscontract is a breaking change for every existing caller that still passes those inputs. GitHub Actions rejects undefined inputs before the reusable job can run, so those repos will fail even though the Python now has an early exit for non-opened/reopenedactions. The PR description says caller updates will happen in follow-up PRs, which means merging this first will break issue-feed workflows in the interim. Keep these inputs as deprecated optional inputs and ignore them, or merge only after all callers are updated atomically.
💬 Non-blocking
- 🟡 Warning —
.github/workflows/octo-issue-feed.yml:84: the early action filter is good, but it only helps after the workflow invocation is accepted. It does not protect old callers from the removed-input failure above.
✅ Highlights
- The PR correctly narrows runtime behavior to
openedandreopened. - The project-group send and IssueTriage mention are removed from the Python body as described.
- Local checks: workflow YAML parses, embedded Python compiles, and the repo’s no-tabs sanity check passes.
yujiawei
left a comment
There was a problem hiding this comment.
Code Review — PR #28 (.github)
Summary
Single-file change to the reusable workflow .github/workflows/octo-issue-feed.yml. The scope matches the PR description: only opened and reopened events are forwarded; per-repo project group fan-out and the @[IssueTriage] mention are removed; inputs issue_labels and project_group_id are dropped from the workflow signature.
The code itself is clean and CI (actionlint, No tabs in workflow files) passes for this file. The unrelated add-to-project check failure is from a different workflow and is not caused by this PR.
Verdict: APPROVED — code is correct. A single P1 rollout/coordination concern is called out below; the team must address it operationally before/at tag move, but it does not need to gate this PR.
Validation
| Item | Status | Evidence |
|---|---|---|
| Only opened/reopened are forwarded | ✅ | .github/workflows/octo-issue-feed.yml:73-75 (if action not in ('opened', 'reopened'): … sys.exit(0)) |
| Early exit lives inside the reusable so callers don't need to filter | ✅ | Same block — runs before any side effect, after env-var require |
project_group_id input + proj_msg fan-out removed |
✅ | Diff removes project_group_id input, require_group_id, second send(proj_gid, …) call |
@[IssueTriage] mention removed |
✅ | mention_uids parameter removed from send(); trailing if action == 'opened': triage_msg … block deleted |
issue_labels input + label rendering removed |
✅ | Input + labels_part formatting + label slot in feed_msg all removed consistently |
| Message format matches PR description | ✅ | feed_msg = f"{emoji} [{repo}] Issue #{num} · {title}\n👤 {author}\n🔗 {url}" |
Allowlist on api_base_url preserved |
✅ | ALLOWED_API_BASES = {'https://im.deepminer.com.cn/api'} retained |
| Repo-name validation preserved (path-traversal guard) | ✅ | require_repo_name retained — and the only remaining caller of repo is the IM message body (the GitHub URL construction that previously required this guard is gone with the IssueTriage removal, so the guard is now defense-in-depth rather than strictly required, but keeping it is the right call) |
| Retry/backoff + failure accounting preserved | ✅ | send() retry loop unchanged in behavior; failed.append(group_id) + sys.exit(1) on failure retained |
| Single recipient = single failure path | ✅ | failed semantics still correct with only one send() call |
Findings
P1 — Reusable-workflow input signature is a breaking change; coordinate the v1 tag move with caller updates
Removing issue_labels and project_group_id from the reusable workflow's inputs: is a backward-incompatible signature change. GitHub validates with: against the called workflow's declared inputs, so any caller that still passes a now-removed input will fail with an "unexpected input" error and the notification will simply stop firing for that repo.
Eight caller repos currently pin @v1 and still pass both removed inputs:
Mininglamp-OSS/octo-web .github/workflows/octo-issue-feed.yml
Mininglamp-OSS/octo-deployment .github/workflows/octo-issue-feed.yml
Mininglamp-OSS/octo-admin .github/workflows/octo-issue-feed.yml
Mininglamp-OSS/octo-adapters .github/workflows/octo-issue-feed.yml
Mininglamp-OSS/octo-server .github/workflows/octo-issue-feed.yml
Mininglamp-OSS/octo-smart-summary .github/workflows/octo-issue-feed.yml
Mininglamp-OSS/octo-matter .github/workflows/octo-issue-feed.yml
Mininglamp-OSS/octo-lib .github/workflows/octo-issue-feed.yml
Example, octo-server/.github/workflows/octo-issue-feed.yml:9-18:
uses: Mininglamp-OSS/.github/.github/workflows/octo-issue-feed.yml@v1
with:
…
issue_labels: ${{ toJson(github.event.issue.labels.*.name) }}
…
project_group_id: 9ea115c7462b4b45b8c85d07d07e0ddeToday v1 points at 0a0ec95a80b32146cf550ccf63977b8a907d4002 (the old reusable), so merging this PR to main is safe in isolation — nothing breaks at merge time. The risk is purely on the subsequent v1 tag move. The PR description does call out the follow-up caller PRs, but doesn't pin a strategy.
Pick one of (recommended order):
- Cut a
v2tag for the new shape and migrate callers from@v1→@v2repo by repo. This is the standard SemVer pattern for reusable workflows and removes any race condition.v1keeps pointing at the old SHA forever; nothing breaks. - Keep
v1and fast-forward it only after all 8 caller PRs are merged. Track the 8 follow-up PRs as an explicit checklist on a tracking issue; do not movev1until every box is checked.
A pure "merge to main, then update callers, then move v1" cadence works only if step 3 is gated on step 2 completing for every repo. Without that discipline, the first repo whose issue fires after v1 is moved will silently stop notifying — there is no integration test that would catch it, and the failure surfaces only as "we stopped getting issue notifications", which is often noticed days later.
Option 1 (v2) is strictly safer and is the convention for shared reusable workflows; recommend going that route unless there is a specific reason to keep a single floating tag.
P2 — Minor: require_repo_name is now overdefensive but harmless
With the IssueTriage block removed, the only consumer of repo is the IM message body. There is no longer a repo-interpolated URL that path-traversal could escape into the GitHub API, so the strict character set check is no longer load-bearing. It's still fine to keep — defense in depth costs nothing and it's a reasonable invariant on a value that came from ${{ github.event.repository.name }}. No action required; flagging only so a future reader doesn't wonder why the strictness exists.
P2 — Nit: docstring on the API-base allowlist description was tightened
The old description ended with "any other value will cause the workflow to fail." The new one drops that clause. The behavior is unchanged (if _api_base not in ALLOWED_API_BASES: … sys.exit(2) is still there a few lines below), so this is purely cosmetic — but the old phrasing was useful for callers reading the input docs. Consider restoring the failure clause in the description so callers aren't surprised when they try to point at a staging endpoint.
Out-of-scope observations (not blocking)
- The caller updates being filed as separate PRs is the correct decomposition; please make sure each caller PR also drops
closed/labeledfrom itson.issues.types:so caller workflows stop scheduling jobs that the reusable will just early-exit from. Functionally identical, but saves a small number of no-op job starts per repo. - Consider adding a brief
## Migrationnote in the reusable workflow header (or a top-of-file comment) pointing callers at "dropissue_labelsandproject_group_id, settypes: [opened, reopened]" so anyone landing on the file via blame can self-serve.
Conclusion
The code change is small, correct, and consistent with the PR description. CI is green for the touched file. The one real concern — caller breakage on a v1 tag move — is a release-coordination decision that lives outside the diff. Recommend v2 for the cleanest rollout.
lml2468
left a comment
There was a problem hiding this comment.
Summary
Simplifies the issue-feed workflow: only forwards opened/reopened events, removes project-group dual-send, removes IssueTriage bot trigger, removes labels from notification messages.
Findings
P0 — Removing inputs breaks all 8 downstream callers
I verified every caller repo — all pass the removed inputs:
| Repo | issue_labels |
project_group_id |
|---|---|---|
| octo-server | ✅ passes | 9ea115c7... |
| octo-web | ✅ passes | c6717c01... |
| octo-adapters | ✅ passes | 0262cefe... |
| octo-matter | ✅ passes | 4ba5c41d... |
| octo-deployment | ✅ passes | (present) |
| octo-smart-summary | ✅ passes | (present) |
| octo-admin | ✅ passes | (present) |
| octo-lib | ✅ passes | (present) |
Since Nov 2023, GitHub Actions validates workflow_call inputs — passing an undefined input causes a validation error. All 8 callers will fail once v1 tag is moved.
Fix: Keep the inputs as deprecated optional no-ops:
issue_labels:
type: string
required: false
default: '[]'
description: 'DEPRECATED — no longer used. Retained for backward compatibility.'
project_group_id:
type: string
required: false
default: ''
description: 'DEPRECATED — no longer used. Retained for backward compatibility.'Then update callers in a follow-up PR to remove these inputs, and only then drop the deprecated inputs from the reusable workflow.
P1 — Caller trigger types mismatch
All callers use types: [opened, closed, reopened, labeled]. The reusable workflow now gracefully exits for non-opened/reopened events (sys.exit(0)), which is correct — but callers are doing unnecessary workflow runs for closed and labeled events. A follow-up should narrow caller triggers to [opened, reopened] to avoid wasted Actions minutes.
P2 — IssueTriage removal is a feature regression
The @[27pmzxX8NAD78c9d01e_bot:Octo 助理-IssueTriage] trigger on new issues is removed. If this is intentional (triage bot deprecated), fine — but if the triage bot is still active, this silently breaks its intake pipeline. Worth confirming with the team.
Non-blocking observations
ISSUE_FEED_GROUPconstant extraction (L149) is cleaner than the inline string. Good.- Whitespace normalization (blank lines) is cosmetic but consistent.
require_group_idremoval is correct given no more group-id inputs.- Event filter early-exit pattern (
sys.exit(0)) is the right approach for soft deprecation of event types.
Verdict
CHANGES_REQUESTED — the input removal (P0) will break all 8 downstream callers on next v1 tag update. Keep inputs as deprecated no-ops and coordinate removal. (Submitted as COMMENTED due to GitHub limitation.)
Add issue_labels and project_group_id back as optional no-ops. Callers on @v1 that still pass these inputs will not break; the reusable accepts but silently ignores the values. This removes the need to coordinate the v1 tag move with caller updates — callers can be migrated individually (or via the 8 follow-up caller PRs that switch to @main and drop these inputs) without any breaking window. The inputs will be dropped in a later cleanup PR after all callers are updated.
Fix pushed (43bc77d) — backward compat + clarification on merge strategyOn the reviewer's concern: The 8 caller repos' files have already been updated in separate PRs:
So the callers will never call this reusable via Fix applied anyway (defense-in-depth): Added
The deprecated inputs will be dropped in a cleanup PR after all callers are updated. Merge order (either works):
Both are safe. No atomicity required. |
yujiawei
left a comment
There was a problem hiding this comment.
Code Review — PR #28 (.github)
Summary
Simplifies the reusable octo-issue-feed.yml so it only forwards opened / reopened issue events to the shared issue-feed group. Drops the per-repo project group send, the IssueTriage bot @-mention, and the issue_labels rendering. Deprecated inputs are kept as optional shims so existing @v1 callers do not break before they are updated.
Change is small, focused, and matches the description. Security posture (input allowlist, control-char sanitization, permissions: {}, repo name regex) is preserved.
Verdict
No P0 / P1 blockers. A few nits and one rollout note below — none are merge-blocking.
Findings
Correctness — looks good
.github/workflows/octo-issue-feed.yml:97-101— Early exitif action not in ('opened', 'reopened'): sys.exit(0)correctly short-circuits before any side effects, so callers that still trigger onclosed/labeledwill no-op cleanly. Exit code0is the right choice (avoids spurious red checks on those events)..github/workflows/octo-issue-feed.yml:33-49—issue_labelsandproject_group_idare correctly markedrequired: falsewith safe defaults, so@v1callers passing the oldwith:block keep working. This is the right backward-compat strategy given every known caller still uses@v1and the old input set (verified againstocto-server,octo-web,octo-deployment,octo-admin,octo-adapters,octo-smart-summary,octo-matter,octo-lib)..github/workflows/octo-issue-feed.yml:121-122— Allowlist check onAPI_BASE_URLis preserved.
Security — preserved
- Removal of the
IssueTriage@mentionalso removes themention_uidsbranch insend(). The remainingpayloadis{type, content}only — no user-controllable mention vector left, which is a small simplification win. sanitize_text()still strips C0 + DEL ontitle/author, andrequire_repo_name()still gatesREPO_NAME.feed_msgno longer interpolates labels, so thelabels_partinjection surface is gone.
Nit — unreachable fallback (.github/workflows/octo-issue-feed.yml:103)
emoji = {'opened': '🆕', 'reopened': '🔄'}.get(action, 'ℹ️')After the early exit at line 99, action can only be opened or reopened, so the 'ℹ️' default is dead. Either drop the .get default or just index the dict — minor, not blocking.
Nit — docstring lost a useful hint (.github/workflows/octo-issue-feed.yml:82)
def sanitize_text(s, max_len=300):
"""Strip control characters to prevent IM message injection."""Old version called out (CR, LF, tabs, etc.) and an inline # Strip all C0 control characters (U+0000-U+001F) and DEL (U+007F). The new version keeps the regex but drops the explanatory hint. Re-adding the U+0000-U+001F + U+007F note would help future readers understand why the regex looks the way it does.
Nit — step name uses an em dash (.github/workflows/octo-issue-feed.yml:57)
name: Notify Octo IM — issue-feed — cosmetic only, but step names show up in the Actions UI; ASCII (-) is slightly friendlier when grepping logs.
Rollout note — v1 tag move (non-blocking, but please plan)
Every caller pins @v1, and refs/tags/v1 currently points at 0a0ec95a80b32146cf550ccf63977b8a907d4002 (pre-PR). Merging this PR to main will not retroactively change caller behavior; someone needs to move the v1 tag (or cut v2 and have callers re-pin). The PR description says "A follow-up PR for each repo will be submitted separately" for caller updates — please make sure the v1 retag (or v2 cut + caller bump) is part of the rollout sequence, otherwise the simplification ships only on paper.
Out-of-scope but worth a thought (future PR, not required for this one)
- Callers still trigger on
closed/labeled. Oncev1is moved, those events will spin up a runner only tosys.exit(0). Cheap, but the planned caller-side trigger trim (types: [opened, reopened]) will eliminate the wasted runs — keep that follow-up sequence in mind.
Jerry-Xin
left a comment
There was a problem hiding this comment.
Scope check passed: this PR updates the repository’s shared reusable GitHub Actions workflow, which is in scope for Mininglamp-OSS/.github. The change matches the stated product decision and is safe to merge.
💬 Non-blocking
- 🔵 Suggestion: .github/workflows/octo-issue-feed.yml:69 still imports
json, but label parsing was removed. Dropping the unused import would keep the script tidy. - 🔵 Suggestion: .github/workflows/octo-issue-feed.yml:33 says callers using the old
@v1reusable will not break. More precisely, this compatibility applies when callers still pass old inputs while referencing this updated reusable workflow.
✅ Highlights
- The reusable workflow now enforces the
opened/reopenedfilter internally at .github/workflows/octo-issue-feed.yml:98, so stale callers that still trigger onclosedorlabeledevents will exit cleanly. - Deprecated
issue_labelsandproject_group_idinputs are retained as optional inputs, avoiding workflow-call failures while downstream repositories migrate. - Removed project-group sends and IssueTriage mentions align with the PR description.
Validation: git diff --check passed, and local actionlint v1.7.12 on .github/workflows/octo-issue-feed.yml passed.
lml2468
left a comment
There was a problem hiding this comment.
Incremental Re-review (43bc77d ← 2db042f)
P0 — Breaking input removal → Fixed ✅
issue_labels and project_group_id retained as deprecated optional no-ops with clear documentation:
# ── Deprecated inputs kept for backward compatibility ──
# Callers still passing these will not break; values are silently ignored.
# Remove from callers first, then remove from this reusable in a follow-up PR.This is exactly the backward-compatible deprecation pattern I suggested. All 8 downstream callers are now safe.
Previous P1/P2 status
- P1 (caller trigger mismatch): out-of-scope for this PR, acknowledged as follow-up
- P2 (IssueTriage removal): intentional per PR title, no action needed
No new issues found.
Verdict
APPROVED — breaking change resolved with proper deprecation. (Submitted as COMMENTED due to GitHub limitation.)
Changes
Per product decision: issue-feed should only notify on new/reopen events and deliver to the shared issue-feed group only. The per-repo project group notification and IssueTriage bot mention are removed.
What changed in
octo-issue-feed.yml(reusable)project_group_id(per-repo group)@[IssueTriage]mention on openedissue_labelsinputproject_group_idinputEarly exit for any action not in
{opened, reopened}is enforced in the reusable itself — callers don't need to add their own filter.Message format (new issues / reopened)
What callers need to update
Each business repo's
octo-issue-feed.ymlcaller should:types: [opened, closed, reopened, labeled]→types: [opened, reopened]project_group_id:from thewith:blockA follow-up PR for each repo will be submitted separately.