Skip to content

ci(octo-issue-feed): opt into webhook-only triage#77

Merged
lml2468 merged 1 commit into
mainfrom
ci/octo-issue-feed-im-toggle
Jun 27, 2026
Merged

ci(octo-issue-feed): opt into webhook-only triage#77
lml2468 merged 1 commit into
mainfrom
ci/octo-issue-feed-im-toggle

Conversation

@lml2468

@lml2468 lml2468 commented Jun 27, 2026

Copy link
Copy Markdown
Contributor

Adds enable_im_notify: false to opt this repo out of the redundant IM ping (multica triage agent creates a tracked issue). Backed by Mininglamp-OSS/.github#80.

@lml2468 lml2468 requested a review from a team as a code owner June 27, 2026 07:50
@lml2468 lml2468 merged commit 9d64c9b into main Jun 27, 2026
@lml2468 lml2468 deleted the ci/octo-issue-feed-im-toggle branch June 27, 2026 07:50
@github-actions github-actions Bot added the size/XS PR size: XS label Jun 27, 2026

@mochashanyao mochashanyao left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[Octo-Q · automated review]

Verdict: Request changes — blocking findings below (data-flow traced).


octo-daemon-cli PR#77 Review Report

Reviewer: Octo-Q (automated review)
PR: #77
Head SHA: a8ba075a46840e3de60d5d910795ba6723c17c00
Scope: 1 file changed, +2 −1 (.github/workflows/octo-issue-feed.yml)


1. 验证结论

Item Status Evidence
enable_im_notify: false syntax Valid YAML boolean; syntactically correct with: input
Backing PR .github#80 merged Merged at 8ade879f (2026-06-27T07:23:41Z)
v1 tag updated to include .github#80 BLOCKER v1135e280f (2026-06-09), predates .github#80 merge commit 8ade879f
End-to-end IM suppression at runtime FAILS See data-flow trace below

2. 发现的问题(P0/P1/P2)

P1 — enable_im_notify is silently ignored; IM notifications still fire (BLOCKER)

Severity: P1 — makes the PR's stated purpose ("opt into webhook-only triage") a no-op at runtime.

diff-scope 三问 (R2):

  1. New / amplified / pre-existing? New — this PR introduces the enable_im_notify: false input.
  2. Did this PR's diff change a behavior contract? The PR intends to change behavior (disable IM), but the callee doesn't receive the change.
  3. Is the defect identical on base branch? No — on base, the workflow has no enable_im_notify input at all. The PR adds it, but the upstream dependency doesn't support it yet.

Data-flow trace (data-flow回溯铁律):

Caller (this PR, octo-daemon-cli/.github/workflows/octo-issue-feed.yml):
  with:
    enable_im_notify: false          ← value injected here
    ↓
Callee: Mininglamp-OSS/.github/.github/workflows/octo-issue-notify.yml@v1
  resolves to commit 135e280f (2026-06-09)
  workflow_call.inputs:              ← enable_im_notify NOT DECLARED
    repo_name, issue_number, issue_title, issue_url, issue_author,
    event_action, api_base_url, feed_group_id
  ↓
  GitHub Actions behavior: undeclared workflow_call inputs are SILENTLY IGNORED
  inputs.enable_im_notify is inaccessible in the callee
  ↓
  "Send" step (at v1, old version):
    im-message: ${{ steps.build.outputs.im_message }}   ← always non-empty
  ↓
  octo-notify composite action (v1):
    if im_message:    ← non-empty → IM mode ENABLED
      post_with_retry(im_api_base + '/v1/bot/sendMessage', ...)
  ↓
  RESULT: IM notification STILL SENT. Webhook also fires. No change from pre-PR behavior.

Root cause: The .github#80 PR merged the enable_im_notify input and the conditional im-message expression (inputs.enable_im_notify && steps.build.outputs.im_message || '') into the reusable workflow, but the v1 lightweight tag was never bumped from 135e280f to 8ade879f (or later). This caller references @v1 → resolves to the old code → input silently dropped.

Impact (R1): The PR achieves nothing at runtime. IM notifications continue to fire, defeating the entire purpose. This is a "makes a path that should work produce wrong behavior" defect — the PR author and reviewers will believe IM is disabled when it isn't.

Fix: Bump the v1 tag in Mininglamp-OSS/.github to 8ade879f9480347f6719b132a87f27bde71b6a9b (or a commit that includes .github#80):

git tag -f v1 8ade879f9480347f6719b132a87f27bde71b6a9b
git push origin v1 --force

This PR can merge after the tag bump (no code changes needed in this PR itself).

P2 — Missing trailing newline in octo-issue-feed.yml

Severity: P2 — nit, no functional impact.

diff-scope: New — the base file has a trailing newline; the PR removes it (\ No newline at end of file).

The diff's last hunk shows:

-      TRIAGE_WEBHOOK_URL: ${{ secrets.TRIAGE_WEBHOOK_URL }}
+      TRIAGE_WEBHOOK_URL: ${{ secrets.TRIAGE_WEBHOOK_URL }}
\ No newline at end of file

POSIX convention and most linters expect a trailing newline. Not blocking, but should fix.


3. 建议

  1. Must fix before merge: Bump v1 tag in Mininglamp-OSS/.github to include the .github#80 merge commit. This is an external action item, not a code change in this PR.
  2. Should fix: Restore trailing newline on the last line of octo-issue-feed.yml.
  3. Process suggestion: Consider adding a "bump tag" step to the .github repo's merge workflow (or a PR checklist item) so future changes to reusable workflows/actions always ship with a tag update.

4. 额外发现

  • The .github repo has versioned tags (v1.0.0, v1.1.0, v1.1.1, v1.1.2) but v1 is a lightweight tag (not annotated). The merge of .github#80 should ideally bump both v1 and create a new v1.x.y semver tag for traceability.
  • The octo-notify composite action's contract is solid: empty im-message correctly disables IM mode (if im_message: guard). The conditional expression at the merge commit (inputs.enable_im_notify && steps.build.outputs.im_message || '') correctly evaluates to '' when the toggle is false. The only gap is the stale v1 tag.

5. 数据流回溯

Consumed Data Upstream Source Reaches Consumer?
enable_im_notify: false (caller with:) This PR's YAML Dropped — callee at @v1 doesn't declare this input
steps.build.outputs.im_message Python script in reusable workflow (always builds message) ✅ Reaches octo-notify → IM sent
secrets.TRIAGE_WEBHOOK_URL Repo secret ✅ Reaches octo-notify → webhook fires
inputs.enable_im_notify (in callee Send step) Caller with: Inaccessible — undeclared in callee's workflow_call.inputs at v1

6. 盲点 checklist(R5)

  • C1 双路径 parity: N/A — no symmetric add/remove or create/delete paths in this diff.
  • C2 control-flow ordering / 嵌套复用: N/A — no control-flow logic or security controls in this diff.
  • C3 授权边界 ≠ 能力边界: N/A — no permission/jail/tool exposure changes.
  • C4 授权生命周期 / 容器-成员状态级联: N/A — no auth changes.
  • C5 build/note 通过 ≠ 运行期路径正确: ⚠️ HIT — The PR's static change looks correct and CI will pass, but at runtime the behavior is unchanged because @v1 doesn't include the callee-side support. This is the P1 above.
  • C6 治理/策略/安全文档自洽性: N/A — not a governance/policy document.

7. 跨轮 blocker 复检

N/A — first review of this PR.


[Octo-Q] verdict: REQUEST_CHANGES — P1 blocker: v1 tag in Mininglamp-OSS/.github is stale (135e280f, predates .github#80); enable_im_notify: false is silently ignored and IM notifications still fire. Fix: bump v1 tag to 8ade879f before merging.

@yujiawei yujiawei left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review — PR #77 (octo-daemon-cli)

Scope: 1 file, +2/-1 — adds enable_im_notify: false to the octo-issue-feed.yml caller so this repo opts into webhook-only triage. Backed by Mininglamp-OSS/.github#80.

Verdict: Changes Requested

The intent is sound and the one-line opt-out is exactly the right shape. However, the input is passed to a reusable workflow at a pinned ref that does not yet declare it, which will break the workflow at runtime.


P0 — enable_im_notify is not declared at the pinned @v1 ref → the notify job fails

This caller invokes:

uses: Mininglamp-OSS/.github/.github/workflows/octo-issue-notify.yml@v1
with:
  ...
  enable_im_notify: false   # <-- new

The new input only exists on the main branch of Mininglamp-OSS/.github (added by #80, merged 2026-06-27 07:23 UTC). The v1 tag still points at commit 135e280 (2026-06-09), which predates #80. I re-fetched octo-issue-notify.yml at ?ref=v1 and it contains zero occurrences of enable_im_notify; its workflow_call.inputs are only: repo_name, issue_number, issue_title, issue_url, issue_author, event_action, api_base_url, feed_group_id. The same is true of every released tag — v1.0.0, v1.1.0, v1.1.1, v1.1.2 — none declare the input.

GitHub Actions validates reusable-workflow inputs strictly against the called workflow's on.workflow_call.inputs. Passing an input the target does not declare is a hard error (invalid input, enable_im_notify is not defined in the referenced workflow) that fails the notify job before any step runs.

Runtime impact: every newly opened issue in this repo triggers a red Octo Issue Feed run, and because the job dies at validation, neither the IM push nor the triage webhook fires. That is strictly worse than today (webhook still works) and is the opposite of the stated goal ("webhook-only triage").

Required fix — order the cross-repo dependency correctly. The input must be available at the ref this caller pins before the caller starts using it. Pick one:

  1. Re-point v1 on Mininglamp-OSS/.github to a commit that includes #80 (and cut a v1.2.0), then keep @v1 here. This is the cleanest since the repo's compatibility story is "callers pin @v1".
  2. Pin this caller to a ref that already contains the input (the #80 merge commit SHA, or a new v1.2.0 tag once cut). Avoid pinning to @main (unstable).

Either way, the prerequisite from #80's own test plan ("merge this PR" → then "opt one caller in") was completed out of order: the input reached main but v1 was never advanced.

Suggested verification once re-pinned: open a throwaway issue in this repo and confirm (a) the Octo Issue Feed run is green, (b) no message lands in the Octo IM feed group, (c) the triage webhook endpoint receives exactly one POST.


P2 (nit) — trailing newline removed

The diff drops the final newline on octo-issue-feed.yml (\ No newline at end of file). POSIX text files and yamllint's new-line-at-end-of-file rule expect a trailing newline; please restore it. Not blocking on its own.


What's correct

  • The change is minimal and on-target — no over-reach, no unrelated edits.
  • Per #80, enable_im_notify defaults to true, so passing false here is the correct opt-out semantics once the input is reachable.
  • Routing the redundant IM ping off for a repo that already has external tracked-issue triage is the right call.

The only blocker is the pinned-ref mismatch; resolve the P0 and this is good to go.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/XS PR size: XS

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants