ci: route trusted-author fork PRs and no-checkout jobs to the ECS pool - #8502
Conversation
Fork PRs whose author has write access (OWNER/MEMBER/COLLABORATOR association) now run Linux CI on the self-hosted ECS pool instead of the saturated GitHub-hosted quota, and bot workflows that check out no code move to ECS unconditionally. Everything stays gated on the MAINTAINER_ECS_RUNNER_DISABLED kill-switch.
|
|
|
@qwen-code /takeover |
Review: route trusted-author fork PRs and no-checkout jobs to the ECS poolOverview. Two independent changes: (1) the ECS routing predicate gains a 1. (High)
|
|
Thanks for the PR! Template looks good ✓ Problem: Observed, not theoretical — the hosted-runner quota is measurably saturated (100+ queued vs ~35 running, PR CI waiting over an hour per the #8496 queue investigation) while the 55-machine ECS pool sits ~60% idle. This is a follow-up to that real incident, with concrete numbers. Direction: Aligned. Keying trust to write access rather than branch location is the right boundary: an author who can push to the repo gains no new capability from running on the pool, which is already network-isolated per the existing verify-lane decision. Keeping macOS/Windows hosted (no self-hosted pool for them) and leaving the autofix command jobs out (fork-YAML-reachable + secret-bearing) is the right call. Size: Not applicable — no core paths touched. 19 files, +150/−42, all under Approach: Minimal and well scoped. Both routing changes ride behind the existing Risk: No high-risk-path signals (no Moving on to code review. 🔍 中文说明感谢贡献! 模板完整 ✓ 问题: 已观测到、非理论性问题——hosted runner 配额已实际饱和(排队 100+、在跑仅 ~35,PR CI 等待超过 1 小时,见 #8496 的队列排查),而 55 台 ECS 池约 60% 空闲。有具体数据支撑。 方向: 对齐。信任边界按写权限划分而非分支位置是正确的:能直接推仓库的作者跑在 ECS 上不会获得新能力,且该池按现有 verify lane 的决策已是网络隔离的。macOS/Windows 保持 hosted(无对应自建池)、autofix 命令 job 明确排除(可经 fork-YAML 触达且携带 secrets),判断正确。 规模: 不适用——未触及核心路径。19 个文件,+150/−42,全部在 方案: 最小化且范围恰当。两处路由调整均在现有 风险: 无高风险路径信号(未触及 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewI verified the PR's central security claim job-by-job: nothing newly routed to the persistent ECS pool executes untrusted code, and everything that checks out or downloads artifacts carries the workspace-ownership restore pre-step.
One non-blocking observation: the updated routing comment in qwen-triage.yml still says comment/dispatch-triggered reruns go to hosted — that was already a simplification before this PR (comment reruns on same-repo PRs have always landed on ECS) and reads slightly looser now for trusted-author forks. The expression is the spec and it's correct; pure wording. TestingUnattended CI run — evidence below is the PR's own CI on the reviewed commit, fetched via the API; no PR code was executed here, and there is no user-visible product behavior to drive in tmux (N/A). Worth noting: Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行(取最新一次运行),省略 skipped,失败项排在最前。 中文说明代码审查逐 job 核验了本 PR 的核心安全主张:新迁入持久化 ECS 池的任务都不会执行不可信代码;所有需要 checkout 或下载 artifact 的任务都带上了 workspace 属主恢复前置步骤。
一个非阻塞观察:qwen-triage.yml 更新后的路由注释仍写着"评论/dispatch 触发的重跑走 hosted"——这句在本 PR 之前就已是简化表述(同仓库 PR 的评论重跑一向落在 ECS),对可信作者 fork 现在读起来更松。以表达式为准,表达式是对的;纯措辞问题。 测试无人值守 CI 运行——下方证据为被审 commit 上 PR 自身 CI 经 API 拉取的结果;本审查未执行任何 PR 代码,也没有用户可见的产品行为需要 tmux 验证(N/A)。值得注意: — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 5/5 — clean across every stage; the trust model is sound and I verified it job-by-job rather than taking the description at its word. This is infrastructure plumbing with a clear, quantified motivation, and it does exactly one thing per hunk. My independent take before reading the diff was the same shape — extend the existing same-repo routing expressions with a write-access association clause behind the existing kill-switch, and move genuinely code-free bot jobs over — which is what the PR does, with the workspace-restore pre-steps where the persistent runners need them. The scoping discipline is the part I'd thank the author for in six months: the autofix command jobs (fork-YAML-reachable, secret-bearing) are explicitly left for a separate review instead of being swept in. The one thing nobody can know pre-merge is how the newly migrated jobs behave on persistent runners in anger; that's intrinsic to CI changes, it's mitigated by the kill-switch plus the restore pre-steps, and the PR says so itself. Approval deferred only because CI hasn't settled: two 中文说明置信度:5/5 —— 各阶段全部干净;信任模型成立,且是逐 job 核验过的,不是照单全收 PR 描述。 这是动机明确、有量化数据支撑的基础设施疏通,每个改动块只做一件事。我在读 diff 之前的独立方案与其形状一致——在现有开关后,把既有的同仓库路由表达式扩展写权限 association 条款,并把真正不接触代码的 bot job 迁过去——PR 正是这么做的,且在持久化 runner 需要之处补了 workspace 恢复前置步骤。六个月后回看最值得感谢的是范围纪律:autofix 命令 job(可经 fork-YAML 触达、携带 secrets)被明确留给单独评审,而不是顺手带进来。 唯一合并前无法确知的是新迁入任务在持久化 runner 上真实承压时的表现;这对 CI 改动是固有的,已有 kill-switch 与恢复前置步骤兜底,PR 自己也如实说明。暂缓批准仅因 CI 尚未收敛:被审 commit 上仍有两条 — Qwen Code · qwen3.8-max Reviewed at |
…ence, timeouts Route the triage agent on the collaborator-permission API result computed by authorize instead of the coarse author_association, which admits org members and read-only collaborators; the two permission-gate jobs revert to the same-repo guard. Keep the fleet watchdog and the CI-failure reporter hosted so they stay independent of the pool they watch. Add missing timeouts, wipe serve-ab's reused workspace, and pin the routing logic with drift and negative-case tests.
|
Review addressed in d32c4aa — section by section: §1 (High) — fixed as suggested. §2 (Medium) — fixed. serve-ab header rewritten: the “on an ephemeral runner” claim is gone, replaced by a runner-routing paragraph (same-repo + write-access fork PRs on ECS with a pre-checkout wipe; others hosted; clause is routing, not boundary). Added a self-hosted-only “Wipe stale workspace before checkout” step so head/ and base/ leftovers cannot bleed into the next A/B diff; npm-cache bleed considered not a vector (entries keyed by lockfile hash). serve-ab is now explicitly listed in the PR body, including the same-repo broadening. §3 (Medium) — fixed. §4 (Medium) — fixed. Fleet shepherd reverted to hosted with a comment naming the failure mode (a wedged pool would queue the very job that recovers it). §5 (Low) — deferred, recorded here. Collapsing the 16 ownership-restore copies into a composite action (and unifying the self-hosted-gating inconsistency) is a mechanical sweep across stable files; keeping it out of this PR to hold the diff to the routing change. This comment is the record so it isn't dropped; happy to take it as a follow-up. §6 (Low) — fixed. New §7 — fixed. All occurrences unified on the compact Verification: actionlint + yamllint clean; workflow vitest suites 392/392; node:test helper suites 144 + 56 (triage) + 6 (new routing) all passing. |
|
🤝 Takeover engaged: the autofix loop now manages this PR — it will address new review feedback and resolve base conflicts until the label is removed or the round cap is reached. Remove the 中文说明🤝 已接管:autofix 循环现在管理此 PR —— 将持续处理新的评审反馈与 base 冲突,直到移除标签或达到轮次上限。移除 |
🩺 serve daemon A/BBuilt the PR base vs this PR head ✅ No response changes against the PR base across 4 scenario(s). — Qwen Code · serve A/B |
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. |
yiliang114
left a comment
There was a problem hiding this comment.
LGTM — security model is sound. The authorize job's real permission-API check (not coarse author_association) is the correct trust bar for the secret-bearing triage agent. No-checkout bot workflows moving to ECS is safe (base-repo YAML, no code execution). Fleet shepherd and CI-failure reporter correctly stay hosted for independence. Kill-switch and timeouts on all newly-routed jobs.
Non-blocking suggestions:
- (P2) ci.yml/sdk-java.yml comments claim OWNER/MEMBER/COLLABORATOR = 'write access' — but MEMBER admits any org member and COLLABORATOR admits read-only invitees. Reword to match serve-ab.yml's honest 'routing convenience, not a security boundary' framing, or record the broader trust bar as an explicit maintainer decision.
- (P2) Verify the org fork-approval setting is 'Require approval for all outside collaborators' (not just first-time), since approving a fork workflow now means pool execution.
- (P3) ci.yml Test and sdk-java jobs get ownership restore but no workspace wipe (serve-ab has one) — leftovers could bleed between trusted fork PRs.
- (P3) classify_pr runs-on lacks the github.repository == 'QwenLM/qwen-code' pin other workflows have.
|
Released in v0.21.6. |
What this PR does
Two runner-routing changes, both behind the existing
MAINTAINER_ECS_RUNNER_DISABLEDkill-switch:pull_request_targettriage agent), routing keys on a REAL write-permission check — the collaborator-permission API lookup the authorize job performs, exposed asauthor_can_write— becauseauthor_associationover-admits (MEMBER is any org member; COLLABORATOR includes read-only invitees). Where the YAML comes from the fork anyway (pull_requestworkflows: main CI, SDK Java, Serve A/B), an OWNER/MEMBER/COLLABORATOR association clause routes trusted authors off the hosted queue — convenience, not a boundary. The permission-gate jobs themselves stay on the same-repo guard. This also moves same-repo Serve A/B runs to ECS (disclosed: broader than fork parity), with a pre-checkout workspace wipe so one PR's build cannot bleed into the next.Jobs newly routed to ECS that check out code or download artifacts get the same workspace-ownership restore pre-step the existing ECS-bound jobs use; newly routed jobs also get explicit timeouts so a hang cannot pin a persistent machine for the 360-minute default.
Why it's needed
The GitHub-hosted runner quota (shared org-wide) is saturated: 100+ queued runs while ~35 run, with PR CI waiting over an hour. Meanwhile the 55-machine self-hosted ECS pool sits ~60% idle because only in-repo branches may use it. The queue is dominated by (a) bot workflows hardcoded to hosted runners and (b) fork PRs from the maintainers themselves. Trust is keyed to write access, not to where the branch lives: an author who can push to the repo gains nothing new from running on the pool (which is also network-isolated, per the existing verify-lane decision). macOS/Windows jobs stay hosted — there is no self-hosted pool for them.
Deliberately out of scope: autofix routing/command jobs (reachable from fork-YAML label events and secret-bearing — separate review), the web-shell visuals capture job (Playwright browser install needs a hosted environment), release/CD pipelines, and anything requiring a container runtime the ECS pool lacks.
Reviewer Test Plan
How to verify
node scripts/lint.js --actionlintand yamllint both pass locally; CI's lint job will re-run them.npx vitest run --config ./scripts/tests/vitest.config.tsover the workflow suites (392 passed) andnode --testover the helper suites (144 + 56 triage + 6 new routing passed). The newci-runner-routing.test.mjsexecutes the real pick_runner shell and simulates the runs-on expression over the full association matrix, asserting they agree (drift guard) and that non-write associations stay hosted (negative cases).ecs-qwenrunners (check the job's runner labels), and the next cron/comment burst of bot workflows should skip the hosted queue. FlippingMAINTAINER_ECS_RUNNER_DISABLED=truemust revert everything to hosted.ubuntu-latest— the association check only admits OWNER/MEMBER/COLLABORATOR, which GitHub computes from the event payload (not attacker-controllable from the fork).Evidence (Before & After)
N/A (CI infrastructure change; evidence is the test output above and post-merge runner labels).
Tested on
Environment (optional)
Local lint + workflow test suites only; runtime behavior verifies in CI after merge.
Risk & Scope
pull_request_targetboundary the trust decision is the real collaborator-permission check, notauthor_association.Linked Issues
Follow-up to the queue investigation on PR #8496 (maintainer fork PR stuck behind the hosted-runner quota).
中文说明
这个 PR 做了什么
两处 runner 路由调整,均受现有
MAINTAINER_ECS_RUNNER_DISABLED开关控制:author_can_write输出——因为author_association过度放行(MEMBER 是任意 org 成员,COLLABORATOR 含只读受邀者)。在 YAML 本来就来自 fork 的场景(pull_request 类 workflow:主 CI、SDK Java、Serve A/B),用 OWNER/MEMBER/COLLABORATOR association 条款把可信作者送出 hosted 队列——是便利而非边界。权限门禁 job 本身保持同仓库守卫。同时披露:这也会把同仓库的 Serve A/B 迁到 ECS(比"fork 对等"更宽),并加了 checkout 前 workspace 清理,避免上一个 PR 的构建污染下一个 A/B diff。新迁入 ECS 且需要 checkout 或下载 artifact 的 job,补上了现有 ECS job 同款的 workspace 属主恢复前置步骤;新迁入的 job 还补了显式 timeout,避免挂起时按 360 分钟默认值占用持久化机器。
为什么需要
GitHub-hosted runner 配额(全 org 共享)已饱和:100+ run 排队、仅 ~35 个在跑,PR CI 等待超过 1 小时;而 55 台自建 ECS 池约 60% 空闲,因为只允许仓库内分支使用。队列大头正是 (a) 硬编码 hosted 的 bot workflow 和 (b) 维护者自己 fork 提的 PR。信任边界应按写权限划分,而不是分支在哪:能直接推仓库的作者跑在 ECS 上不会获得任何新能力(且该池已是网络隔离的,与现有 verify lane 的决策一致)。macOS/Windows job 保持 hosted——没有对应的自建池。
明确不在本次范围:autofix 的路由/命令 job(可经 fork-YAML 的 label 事件触达且携带 secrets,需单独评审)、web-shell 截图采集 job(Playwright 浏览器安装依赖 hosted 环境)、发布/CD 流水线,以及依赖 ECS 池所没有的容器运行时的任务。
审阅者测试计划
如何验证
node scripts/lint.js --actionlint与 yamllint 本地通过,CI lint 会再跑一遍。node --testhelper 套件 144 + triage 56 + 新增路由 6 全部通过。新增的ci-runner-routing.test.mjs会真实执行 pick_runner shell 并对 runs-on 表达式做全 association 矩阵模拟,断言两者一致(防漂移),且非写权限 association 一律留在 hosted(负例)。ecs-qwenrunner(查看 job 的 runner 标签),下一轮 cron/评论触发的 bot workflow 应绕过 hosted 队列。将MAINTAINER_ECS_RUNNER_DISABLED=true后必须全部回退 hosted。ubuntu-latest——association 检查只放行 OWNER/MEMBER/COLLABORATOR,该值由 GitHub 从事件载荷计算,fork 无法伪造。前后对比证据
N/A(CI 基建改动;证据为上述测试输出与合并后的 runner 标签)。
测试环境
运行环境(可选)
仅本地 lint + workflow 测试套件;运行时行为合并后由 CI 验证。
风险与范围
author_association。关联 Issue
源自 #8496 的队列排查(维护者 fork PR 卡在 hosted runner 配额之后)。