Skip to content

fix(ci): keep successful external sweep dispatch green / 修复 CI：避免通知失败覆盖扫描调度成功 #2455

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
cquil11 merged 1 commit into from
Aug 2, 2026
Merged

fix(ci): keep successful external sweep dispatch green / 修复 CI：避免通知失败覆盖扫描调度成功 #2455

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 13 additions & 8 deletions .github/workflows/trusted-external-sweep.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,7 @@
permissions:
actions: write
contents: read
issues: write
pull-requests: read
pull-requests: write

jobs:
dispatch:
Expand Down Expand Up @@ -132,12 +131,18 @@
`Dispatched ${runLink} for approved external revision \`${pull.head.sha}\`.`,
'New commits are not trusted automatically; remove and re-add the primary sweep label to approve a new SHA.',
'',
`已为获批的外部提交 \`${pull.head.sha}\` 调度${runLink}。`,
'后续新提交不会自动获得信任；如需批准新的 SHA，请移除并重新添加主扫描标签。',
].join('\n');
await github.rest.issues.createComment({
owner: context.repo.owner,
repo: context.repo.repo,
issue_number: pull.number,
body,
});
try {
await github.rest.issues.createComment({
owner: context.repo.owner,
repo: context.repo.repo,
issue_number: pull.number,
body,
});
} catch (error) {
core.warning(
`Sweep dispatched successfully, but the PR comment failed: ${error.message}`,
);
}

Check warning on line 148 in .github/workflows/trusted-external-sweep.yml

View check run for this annotation

Claude / Claude Code Review

Run-lookup polling loop still unprotected, can fail job after successful dispatch 

The fix only wraps `issues.createComment` in try/catch, but the `listWorkflowRuns` polling loop just above it (also running after the successful `createWorkflowDispatch`) is left unwrapped. If that call throws (transient 5xx, secondary rate limit, network blip), the step still fails uncaught despite the sweep already being dispatched — the same failure class this PR sets out to fix. Consider wrapping the polling loop (or the whole post-dispatch block) the same way for full coverage.
Comment on lines 134 to +148

@claude claude Bot Aug 2, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 The fix only wraps issues.createComment in try/catch, but the listWorkflowRuns polling loop just above it (also running after the successful createWorkflowDispatch) is left unwrapped. If that call throws (transient 5xx, secondary rate limit, network blip), the step still fails uncaught despite the sweep already being dispatched — the same failure class this PR sets out to fix. Consider wrapping the polling loop (or the whole post-dispatch block) the same way for full coverage.

Extended reasoning...

This PR's stated goal is to ensure "a notification-policy failure cannot make an already-dispatched sweep appear unsuccessful," and it achieves that for one specific call: github.rest.issues.createComment (now wrapped in try/catch at the end of the script). However, the github.rest.actions.listWorkflowRuns polling loop that runs immediately after createWorkflowDispatch succeeds — and before the comment call — is not given the same protection.

Code path: createWorkflowDispatch succeeds (line ~109, the irreversible action that starts the e2e run), then the script enters a loop that polls listWorkflowRuns up to 15 times to resolve dispatchedRun for the run-link comment. This loop already tolerates the "run not found" case gracefully — if no matching run turns up after 15 attempts, dispatchedRun stays undefined and the code falls back to a generic workflow link. But that graceful handling only covers a benign not-found outcome, not an actual API failure. If listWorkflowRuns itself rejects (a transient 5xx, a secondary rate limit, or a network blip), the await throws inside the loop, propagates uncaught, and the whole github-script step fails — exactly the "green dispatch, red job" scenario this PR was written to eliminate.

Why the existing fix doesn't prevent it: the new try/catch block only surrounds the createComment call, which executes after this loop. Anything that throws during the loop itself never reaches that try/catch, so it's outside the scope of the current fix even though it shares the identical failure class (a non-dispatch API call failing after the dispatch already succeeded).

Step-by-step reproduction of the gap:

  1. Maintainer applies the sweep label; the workflow authorizes the actor and calls createWorkflowDispatch — this succeeds and the e2e run is now queued/running.
  2. The script enters the polling loop and calls listWorkflowRuns to find the new run's URL for the notification comment.
  3. GitHub's Actions API returns a transient 502/503, or the token hits a secondary rate limit on this call (both are realistic under load, since this endpoint is being polled up to 15 times).
  4. The await github.rest.actions.listWorkflowRuns(...) throws. There is no try/catch around it, so the exception unwinds out of the loop and out of the script.
  5. The github-script step fails, the job goes red — even though the sweep was already successfully dispatched in step 1 — reproducing the precise inconsistency (successful dispatch, failed-looking job) that this PR's description calls out as the bug being fixed.

Impact: this is a genuine gap relative to the PR's own stated invariant, but it's a lower-probability trigger than the deterministic 403 this PR was written to fix (that 403 was on the comment endpoint specifically, due to a permissions mismatch that's now corrected). listWorkflowRuns only needs actions: read/write, which this workflow already has, so it won't hit the same permissions failure — it would need an actual transient GitHub API error to fail. The consequence is also contained: a misleading red job status that could at worst prompt a maintainer to remove/re-add the sweep label and trigger a redundant dispatch, not data loss or a broken sweep.

Suggested fix: wrap the polling loop (or the entire post-dispatch block from after createWorkflowDispatch through the comment) in the same try/catch-and-core.warning pattern already used for the comment call, so any post-dispatch API failure is treated as best-effort and doesn't flip the job status.