fix(ci): gate the attachment guard before it allocates a runner - #8095
Merged
Conversation
Measured on a congested pool: 88 active jobs, 72 hosted and 15
self-hosted. The self-hosted 15 were all running with zero queued; the
hosted 72 were contending, and 20 of them were the SAME job —
`remove-suspicious-attachments`, all queued, none running.
Its real cost is not the work. Recent completed runs:
queue=629s run=5s queue=568s run=2s
queue=518s run=2s queue=340s run=3s
Two to five seconds of API calls behind up to ten minutes of queueing.
And almost none of it needed to happen. The trust check lived INSIDE the
github-script, so a runner was queued, allocated and started before the
job could decide it had nothing to do. Over the 200 most recent comments
on this repo: 184 from trusted associations, 9 from bots, 7 actually
needing a scan. 96.5% of these runs existed to print "Trusted author;
skipping".
Two changes:
- Hoist the association and bot checks into the job `if:`. GitHub
evaluates `if:` BEFORE allocating a runner, so a trusted comment now
costs nothing. The script keeps its own copies: the gate is an
optimisation, not the control, and the two must be able to disagree
without becoming unsafe. Every ambiguity therefore resolves toward
RUNNING the scan — an unrecognised payload yields an empty
association, which is not in the trusted list, so the job runs.
- Add a per-comment concurrency group with cancel-in-progress. The
workflow listens on `edited` as well as `created`, and the bot PATCHes
its own comments constantly, so repeated edits of one comment stacked.
The scan reads the comment's CURRENT body, so a queued earlier scan is
already stale and cancelling it loses nothing. (Contrast the verify
lane, where cancel-in-progress is deliberately false because a
cancelled run destroys evidence.) The key falls back to run_id so an
unexpected payload gets its own group instead of serialising every
scan into one.
Deliberately NOT moved to the self-hosted pool, though it would fit
technically (no checkout, no PR code, API calls only): the 20 stacked
jobs were duplicates, so relocating them just fills the ECS pool
instead — and that pool is what /verify and /triage depend on. It also
holds issues:write while processing untrusted comment bodies, which
belongs on ephemeral hardware rather than reused machines.
The `if:` semantics are verified against all payload shapes — 12 cases
covering both `comment.*` and `review.*` associations, bots, and
missing/empty payloads, each asserting which direction it resolves.
CONTRIBUTOR is deliberately NOT trusted: a merged PR does not make
someone's links safe.
Mutation-verified 6/6: dropping the review payload path, dropping the
bot check, adding CONTRIBUTOR to the trusted list, turning off
cancel-in-progress, collapsing the group to a global key, and inverting
the gate so untrusted comments are the ones skipped — each turns a test
red. The last is the one that matters; it is the only mutation here that
would be a security regression rather than a cost regression.
148/148 tests across both suites; actionlint exit 0; prettier and eslint
clean.
Collaborator
|
✅ Qwen Triage finished — CI landed green on ✅ Qwen Triage 已完成 —— |
Collaborator
|
Thanks for the PR — the queue data makes the problem very concrete. Template looks good ✓ (the "why" lives under "The measurement" rather than a separate heading, but the substance is all there) Problem: observed and well-measured. 20 stacked Direction: aligned. This is the repo's own CI infrastructure, and the fix directly addresses runner waste that was crowding out Size: not applicable — no core paths touched. Two files: the workflow YAML and its test. Approach: the scope feels right. Two changes, both minimal and directly motivated by the measurement: (1) hoist the trust check into Risk: no elevated risk signals. Moving on to code review. 🔍 中文说明感谢贡献！队列数据让问题非常具体。 模板完整 ✓（"为什么需要"的内容放在"测量"小节下而非独立标题，但实质内容齐全） 问题：已观测且有充分测量数据。托管池上 20 个堆积的 方向：对齐。这是仓库自身的 CI 基础设施，修复直接解决了挤占 规模：不适用——未触及核心路径。两个文件：工作流 YAML 及其测试。 方案：范围合理。两处改动，都是最小且直接由测量数据驱动的：(1) 把信任检查提升到 风险：无升级风险信号。 进入代码审查 🔍 — Qwen Code · qwen3.8-max-preview Reviewed at |
Collaborator
Code reviewIndependent proposal: given 96.5% of runs skip after starting, I would (1) hoist the trust check into the job-level The
The gate can only add skipping — it cannot remove scanning the script would do, because the script retains its own The concurrency group is well-designed. Keyed per comment/review ID with Tests are thorough. The No critical blockers. No AGENTS.md violations. No reuse concerns — the changes are minimal and self-contained. CI test evidenceFinal CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 The ubuntu unit suite is still running; macOS/Windows/Integration are skipped (conditional triggers). No failures so far. The PR author reports 148/148 tests and actionlint exit 0 — that is the author's claim; the CI run above is the independent signal, still pending. Not verified: actual queue relief is only measurable post-merge (the number to watch is 中文说明代码审查独立方案： 鉴于 96.5% 的运行在启动后跳过，我会 (1) 把信任检查提升到作业级
Concurrency 分组设计合理。 按评论/review ID 分组， 测试充分。 复制 GitHub 表达式语义并断言每种载荷形态（含 fail-safe）。6/6 变异验证钉住极性。与文件现有测试风格一致。 无关键阻塞。无 AGENTS.md 违规。无复用问题。 CI 测试证据ubuntu 单元测试套件仍在运行；macOS/Windows/集成为条件跳过。目前无失败。作者报告 148/148 测试和 actionlint exit 0——这是作者的声明，CI 是独立信号，仍在等待中。 未验证：实际队列缓解只能在合入后测量。这是变更的固有特性—— — Qwen Code · qwen3.8-max-preview Reviewed at |
Collaborator
|
Confidence: 5/5 — clean across every stage; would merge without hesitation. This is what a good CI fix looks like: measured first (live queue sample, 200-comment audit), then two minimal changes that each address a specific waste, with the security-critical part ( Walked the No simpler path exists. No unrelated changes. No scope creep. In six months I'd thank the author, not curse them. Approval deferred until CI lands green on 中文说明置信度：5/5 —— 每个阶段都干净，毫不犹豫可以合入。 这是一个优秀的 CI 修复的样子：先测量（实时队列采样、200 条评论审计），然后两处最小改动各自解决一个具体浪费，安全关键部分（ 独立验证了 无更简路径。无无关改动。无范围蔓延。六个月后会感谢作者，而非咒骂。 审批延迟至 CI 在 — Qwen Code · qwen3.8-max-preview Reviewed at |
yiliang114
approved these changes
Jul 30, 2026
yiliang114
approved these changes
Jul 30, 2026
yiliang114
left a comment
yiliang114
left a comment
Collaborator
There was a problem hiding this comment.
LGTM. Hoisting the trust gate to job-level if: is the right fix — 96.5% of runs were no-ops burning queue time. Script retains its own checks as defense-in-depth, fail-safe polarity is correct, and the per-comment concurrency group cleanly dedupes stale scans. Nice mutation testing coverage on the expression.
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
qwen-code-ci-bot
approved these changes
Jul 30, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship — CI landed green after the review. ✅
Collaborator
|
Released in v0.21.2. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does
Stops
Comment Attachment Guardfrom allocating a runner for work it will immediately skip, and collapses duplicate scans of the same comment.The measurement
While investigating why two
/verifytriggers sat queued for 5+ minutes, I sampled the live queue: 88 active jobs — 72 hosted, 15 self-hosted. The self-hosted 15 were all running, zero queued. The hosted 72 were contending, and 20 of them were the same job:remove-suspicious-attachments, allqueued, none running.Its cost is not the work. Recent completed runs:
Two to five seconds of API calls behind up to ten minutes of queueing.
And almost none of it needed to happen. The trust check lived inside the github-script, so a runner was queued, allocated and started before the job could decide it had nothing to do. Over the 200 most recent comments on this repo:
96.5% of these runs existed to print
Trusted author; skipping.Two changes
1. Hoist the association and bot checks into the job
if:. GitHub evaluatesif:before allocating a runner, so a trusted comment now costs nothing at all.The script keeps its own copies. This is important: the gate is an optimisation, not the control, and the two must be able to disagree without becoming unsafe. So every ambiguity resolves toward running the scan — an unrecognised payload yields an empty association, which is not in the trusted list, so the job runs.
2. Add a per-comment concurrency group with
cancel-in-progress: true. The workflow listens oneditedas well ascreated, and the bot PATCHes its own comments constantly, so repeated edits of one comment stacked. The scan reads the comment's current body, so a queued earlier scan is already stale and cancelling it loses nothing.The key falls back to
run_idso an unexpected payload gets its own group instead of serialising every scan into one.Why not move it to the self-hosted pool
It would fit technically — no checkout, no
run:, no PR code, only API calls — and the ECS pool had zero queue at the time. I did not, for two reasons:/verifyand/triagedepend on. Fixing the duplication is the actual repair; moving it would hide the symptom in a place where it hurts more.issues: writewhile processing untrusted comment bodies. That belongs on ephemeral hardware rather than reused machines — the same persistence argument as feat(triage): run external /verify on ECS behind a workspace wipe #7985, pointed the other way.Reviewer Test Plan
How to verify
The
if:expression is the risky part: written wrong in the permissive direction it is a security regression, not a cost regression. So its semantics are replicated in a test and asserted per payload shape —a || byields the first truthy value, a missing path is'', andcontains(list, '')is false:comment.author_association∈ {OWNER, MEMBER, COLLABORATOR}review.author_association∈ samesender.type == 'Bot'''CONTRIBUTORis deliberately not trusted: a merged PR does not make someone's links safe. That matches the script's existing list exactly.Mutation-verified 6/6, each with landing proof:
review.*payload pathcancel-in-progress: falseMutation 2 first reported
landed=False— the anchor's indentation did not match, so nothing changed and the green proved nothing. Re-run against the exact line, it kills.148/148 tests across both suites; actionlint exit 0 (it validates the expression syntax, which is the failure mode a YAML-only check would miss); prettier and eslint clean.
Tested on
Workflow expression and its tests; no platform-dependent behaviour. The
if:semantics are asserted against a replication of GitHub's expression rules rather than executed on GitHub — the acceptance check is the first trusted comment after merge producing no run at all rather than a skipped one.Risk & Scope
if:that is wrong in the permissive direction silently disables a security control. Mitigated three ways — the script retains its own checks (so the gate can only add skipping, never remove scanning that the script would do), every ambiguous payload resolves toward running, and mutation 6 pins the polarity. Worth a reviewer's own read of the expression regardless; that is the one line in this PR where being wrong is expensive.remove-suspicious-attachmentsqueue depth, which was 20 at the time of writing. I have not touched the guard's detection logic, its extension list, or its moderation behaviour.Linked Issues
Found while investigating queue latency on the
/verifylane (#8014, #8016). No issues closed.中文说明
本 PR 做了什么
让
Comment Attachment Guard不再为"马上就会跳过"的工作分配 runner，并合并对同一条评论的重复扫描。测量
在排查两次
/verify触发为何排队 5 分钟以上时，我采样了实时队列：88 个活跃作业——72 个托管、15 个自建。 自建的 15 个全部在运行、零排队；托管的 72 个在争抢，其中 20 个是同一个作业：remove-suspicious-attachments，全部queued，无一在运行。它的代价不在于工作量。近期已完成的运行：排队 629 秒 / 执行 5 秒；排队 568 秒 / 执行 2 秒；排队 518 秒 / 执行 2 秒；排队 340 秒 / 执行 3 秒。2–5 秒的 API 调用，排在最多十分钟的队列之后。
而这里面几乎没有一次是必要的。 信任检查写在 github-script 内部，因此 runner 已排队、已分配、已启动，作业才能判断自己无事可做。对本仓库最近 200 条评论的统计：
96.5% 的运行只是为了打印
Trusted author; skipping。两处改动
1. 把关联与 bot 检查提升到作业级
if:。 GitHub 在分配 runner 之前求值if:，因此受信任的评论现在完全不产生成本。脚本保留了自己的那份检查。这一点很重要：这道门是优化，不是控制，二者必须能够分歧而不至于变得不安全。因此所有歧义都倒向执行扫描——无法识别的载荷会得到空关联，而空值不在受信任列表中，于是作业照常运行。
2. 增加按评论维度的 concurrency 分组 +
cancel-in-progress: true。 该工作流除created外还监听edited，而 bot 会频繁 PATCH 自己的评论，因此对同一条评论的重复编辑会不断堆叠。扫描读取的是评论的当前正文，所以排队中的早先扫描本已过期，取消它不损失任何东西。分组 key 回退到
run_id，使异常载荷各自成组，而不是把所有扫描串行化进同一组。为什么不搬到自建池
技术上完全可行——不 checkout、无
run:、不执行 PR 代码、只调 API——而且当时 ECS 池零排队。我没有这么做，有两个理由：/verify与/triage所依赖的。修掉重复才是真正的修复；搬迁只是把症状藏到一个危害更大的地方。issues: write且处理的是不可信的评论正文。 这类角色更适合运行在用后即毁的机器上，而非复用机器——与 feat(triage): run external /verify on ECS behind a workspace wipe #7985 同一条"持久性"论证，方向相反。评审验证方案
if:表达式是风险所在：一旦朝放行方向写错，就是安全回退而不仅是成本回退。因此其语义在测试中被复现并按载荷形态逐一断言（a || b取第一个真值、缺失路径为''、contains(list, '')为 false）：comment.author_association∈ {OWNER, MEMBER, COLLABORATOR}review.author_association∈ 同上sender.type == 'Bot'''CONTRIBUTOR刻意不被信任：合并过一个 PR 并不能让某人的链接变得安全。这与脚本中既有的列表完全一致。6/6 变异验证，每项均带落地证明：删除
review.*载荷路径（PR review 事件被静默放行）；删除 bot 检查（200 次中的 9 次回归）；把 CONTRIBUTOR 加入受信任列表（对真实风险类别停止扫描）；cancel-in-progress: false（重复编辑重新堆叠）；塌缩为全局 concurrency key（全仓扫描串行到一个 runner）；反转门禁（跳过不可信、扫描可信）——这是本 PR 中唯一属于安全回退而非成本回退的变异。变异 2 起初报告
landed=False——锚点缩进不匹配，什么也没改，绿色结果毫无意义。改用精确行重跑后致死。两个测试套件共 148/148 通过；actionlint exit 0（它会校验表达式语法，而这正是仅做 YAML 检查会漏掉的失败模式）；prettier 与 eslint 干净。
测试平台
macOS ✅；Windows N/A；Linux N/A——工作流表达式及其测试，无平台相关行为。
if:语义是针对 GitHub 表达式规则的复现进行断言，而非在 GitHub 上实际执行——验收标准是合入后第一条受信任评论完全不产生运行，而不是产生一次被跳过的运行。风险与范围
if:会静默停用一项安全控制。三重缓解——脚本保留自己的检查（因此这道门只能增加跳过，绝不能移除脚本本会执行的扫描）、所有歧义载荷都倒向执行、变异 6 钉住了极性。无论如何仍值得评审者亲自读一遍那个表达式；这是本 PR 中唯一"写错代价高昂"的一行。remove-suspicious-attachments的排队深度，撰写本文时为 20。我未改动守卫的检测逻辑、扩展名列表或处置行为。关联 Issue
在排查
/verify车道（#8014、#8016）的队列延迟时发现。不关闭任何 issue。