Skip to content

feat(issue-feed): simplify to new/reopen only, remove project-group and IssueTriage#28

Merged
lml2468 merged 2 commits into
mainfrom
feat/issue-feed-simplify
May 19, 2026
Merged

feat(issue-feed): simplify to new/reopen only, remove project-group and IssueTriage#28
lml2468 merged 2 commits into
mainfrom
feat/issue-feed-simplify

Conversation

@lml2468
Copy link
Copy Markdown
Contributor

@lml2468 lml2468 commented May 19, 2026

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)

Before After
Forwards: opened, closed, reopened, labeled Forwards: opened, reopened only
Sends to issue-feed group Sends to issue-feed group (unchanged)
Sends to project_group_id (per-repo group) Removed
@[IssueTriage] mention on opened Removed
issue_labels input Removed (irrelevant without labeled event)
project_group_id input Removed

Early 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)

🆕 [octo-server] Issue #84 · Fix authentication bug
👤 someuser
🔗 https://github.com/Mininglamp-OSS/octo-server/issues/84

What callers need to update

Each business repo's octo-issue-feed.yml caller should:

  1. Change trigger: types: [opened, closed, reopened, labeled]types: [opened, reopened]
  2. Remove project_group_id: from the with: block

A follow-up PR for each repo will be submitted separately.

…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.
Copy link
Copy Markdown

@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 relevant to Mininglamp-OSS/.github, but it currently breaks existing reusable-workflow callers.

🔴 Blocking

  • 🔴 Critical — .github/workflows/octo-issue-feed.yml:8: removing issue_labels and project_group_id from the workflow_call.inputs contract 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/reopened actions. 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 opened and reopened.
  • 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
yujiawei previously approved these changes May 19, 2026
Copy link
Copy Markdown

@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 #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: 9ea115c7462b4b45b8c85d07d07e0dde

Today 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):

  1. Cut a v2 tag for the new shape and migrate callers from @v1@v2 repo by repo. This is the standard SemVer pattern for reusable workflows and removes any race condition. v1 keeps pointing at the old SHA forever; nothing breaks.
  2. Keep v1 and 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 move v1 until 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/labeled from its on.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 ## Migration note in the reusable workflow header (or a top-of-file comment) pointing callers at "drop issue_labels and project_group_id, set types: [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.

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.

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_GROUP constant extraction (L149) is cleaner than the inline string. Good.
  • Whitespace normalization (blank lines) is cosmetic but consistent.
  • require_group_id removal 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.
@lml2468
Copy link
Copy Markdown
Contributor Author

lml2468 commented May 19, 2026

Fix pushed (43bc77d) — backward compat + clarification on merge strategy

On the reviewer's concern:

The 8 caller repos' files have already been updated in separate PRs:

  • Their ref is changed @v1@main (not @v1)
  • Both deprecated inputs are removed from those caller PRs
  • Trigger types narrowed to [opened, reopened]

So the callers will never call this reusable via @v1 again after those PRs merge — the v1 tag does not need to move. There is no breaking window.

Fix applied anyway (defense-in-depth):

Added issue_labels and project_group_id back as optional deprecated no-ops (not required, default empty, silently ignored in Python). This means:

  • Callers still on @v1 → use old reusable, nothing breaks
  • Callers migrated to @main (with inputs removed) → use new reusable, nothing breaks
  • Even if v1 were accidentally moved forward, old callers passing these inputs would still not fail

The deprecated inputs will be dropped in a cleanup PR after all callers are updated.


Merge order (either works):

  1. Merge this PR (feat(issue-feed): simplify to new/reopen only, remove project-group and IssueTriage #28) → then merge 8 caller PRs, OR
  2. Merge 8 caller PRs first → then merge this PR

Both are safe. No atomicity required.

Copy link
Copy Markdown

@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 #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 exit if action not in ('opened', 'reopened'): sys.exit(0) correctly short-circuits before any side effects, so callers that still trigger on closed / labeled will no-op cleanly. Exit code 0 is the right choice (avoids spurious red checks on those events).
  • .github/workflows/octo-issue-feed.yml:33-49issue_labels and project_group_id are correctly marked required: false with safe defaults, so @v1 callers passing the old with: block keep working. This is the right backward-compat strategy given every known caller still uses @v1 and the old input set (verified against octo-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 on API_BASE_URL is preserved.

Security — preserved

  • Removal of the IssueTriage @mention also removes the mention_uids branch in send(). The remaining payload is {type, content} only — no user-controllable mention vector left, which is a small simplification win.
  • sanitize_text() still strips C0 + DEL on title / author, and require_repo_name() still gates REPO_NAME. feed_msg no longer interpolates labels, so the labels_part injection 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. Once v1 is moved, those events will spin up a runner only to sys.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.

Copy link
Copy Markdown

@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.

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 @v1 reusable 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 / reopened filter internally at .github/workflows/octo-issue-feed.yml:98, so stale callers that still trigger on closed or labeled events will exit cleanly.
  • Deprecated issue_labels and project_group_id inputs 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.

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.

Incremental Re-review (43bc77d2db042f)

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.)

@lml2468 lml2468 merged commit 5eda473 into main May 19, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants