Skip to content

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

Merged
cquil11 merged 1 commit into
mainfrom
agent/fix-external-sweep-comment
Aug 2, 2026
Merged

fix(ci): keep successful external sweep dispatch green / 修复 CI:避免通知失败覆盖扫描调度成功#2455
cquil11 merged 1 commit into
mainfrom
agent/fix-external-sweep-comment

Conversation

@cquil11

@cquil11 cquil11 commented Aug 2, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • grant the trusted external-sweep dispatcher pull-requests: write for PR run-link comments
  • treat comment creation as best-effort after a successful workflow dispatch
  • preserve the successful dispatch result even if GitHub rejects the notification call

Root cause

In run 30764133602, authorization and workflow_dispatch succeeded, creating e2e run 30764143282. The dispatcher failed only afterward when the PR-comment endpoint returned 403 Resource not accessible by integration despite the workflow requesting issues: write.

The endpoint advertises pull_requests=write as an accepted permission, so this switches to that repository-consistent permission. The fallback warning ensures a notification-policy failure cannot make an already-dispatched sweep appear unsuccessful.

Validation

  • actionlint .github/workflows/trusted-external-sweep.yml
  • changelog, reuse, sweep-gating, priority, and eval tests: 171 passed

摘要

  • 为可信外部扫描调度器授予 pull-requests: write,用于在 PR 中发布运行链接
  • 工作流调度成功后,将评论创建视为尽力而为的通知
  • 即使 GitHub 拒绝通知请求,也保留成功的扫描调度结果

根本原因

运行 30764133602中,授权和 workflow_dispatch 均成功,并创建了 e2e 运行 30764143282。调度器只在随后调用 PR 评论接口时失败;尽管工作流请求了 issues: write,该接口仍返回 403 Resource not accessible by integration

该接口将 pull_requests=write 列为可接受权限,因此本修复改用与仓库现有工作流一致的权限。警告回退保证通知策略失败不会让已成功调度的扫描显示为失败。

验证

  • actionlint .github/workflows/trusted-external-sweep.yml
  • changelog、复用、扫描门控、优先级和评测测试:171 项通过

Use pull-request write permission for run-link comments and keep a successful sweep dispatch green if notification still fails.\n\n中文:使用 pull request 写权限发布运行链接,并在通知仍失败时保持已成功调度的扫描为成功状态。
@cquil11
cquil11 marked this pull request as ready for review August 2, 2026 19:54
@cquil11
cquil11 requested a review from a team August 2, 2026 19:54
@cquil11
cquil11 merged commit d46e468 into main Aug 2, 2026
7 checks passed
@cquil11
cquil11 deleted the agent/fix-external-sweep-comment branch August 2, 2026 19:54
@claude

claude Bot commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

Claude finished @cquil11's task in 1m 9s —— View job


Review of PR #2455

  • Read PR diff (.github/workflows/trusted-external-sweep.yml)
  • Verify permission change doesn't break other API calls in the workflow
  • Check the try/catch fallback logic

LGTM - no blocking issues found.

Verified details:

  • Swapping issues: write + pull-requests: read for pull-requests: write is safe: the workflow's other API calls (getCollaboratorPermissionLevel, pulls.get, createWorkflowDispatch, listWorkflowRuns) only need pull-requests: read (implied by write) and the retained actions: write. Commenting on a PR via the issues endpoint is covered by pull-requests: write, matching the 403 seen in the failing run.
  • The try/catch correctly scopes only the notification call, so a comment failure downgrades to core.warning without masking dispatch errors, which still fail the job as before.

@claude claude Bot left a comment

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.

Beyond the inline nit about the unprotected listWorkflowRuns polling loop, I also checked the permission swap itself (dropping issues: write/pull-requests: read for pull-requests: write) — issues.createComment on a PR issue number is covered by the pull-requests scope in the GITHUB_TOKEN permission model, so the new permission set is correct for this call, not a regression.

Extended reasoning...

One short paragraph per instructions for this exception case.

Comment on lines 134 to +148
`已为获批的外部提交 \`${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}`,
);
}

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.

@claude claude Bot mentioned this pull request Aug 3, 2026
charxwu pushed a commit that referenced this pull request Aug 6, 2026
Use pull-request write permission for run-link comments and keep a successful sweep dispatch green if notification still fails.\n\n中文:使用 pull request 写权限发布运行链接,并在通知仍失败时保持已成功调度的扫描为成功状态。
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Development

Successfully merging this pull request may close these issues.

1 participant