fix(test): stop measuring model latency on the shared E2E pool - #11004
Conversation
…10994) The `E2E Test (Linux) - sandbox:none - shard 2/3` leg fails intermittently on the shared pool — runs 33831058473 (both attempts), 33829764813, 33757746363, 33752646002 and 33741096098 — while the same commit's sandbox:docker sibling and the macOS legs stay green. Its logs are admin-gated, so the diagnosis comes from the shard's own shape: the failing step runs 22.4-25.5min against an 11.8-16.7min healthy baseline, and every one of the five failures exited 1 rather than hanging, which is retries stacking on a real assertion failure rather than a timeout. The GenAI telemetry cases read `telemetry.log` the instant the CLI child exits, with no readiness wait, and then assert exact span counts. The exporter writes that file during shutdown, so a flush that loses the race leaves the reader with nothing: reproduced here by dropping the wait, all three cases fail with `expected [] to have a length of 2 but got +0` and burn all three of vitest's retries. `waitForTelemetryEvent` and `waitForToolCall` already wait for telemetry readiness before asserting for exactly this reason; these three did not. Seven of the shard's 25 files skip themselves unless the sandbox is off, which is why the docker leg cannot see this one. Poll for the spans each case asserts on before reading them, and add a witness that writes the log after the read starts so the wait stays pinned. Co-Authored-By: qwen-code-dev-bot <qwen-code-dev@service.alibaba.com>
`E2E Test (Linux) - sandbox:none - shard 2/3` fails intermittently on the shared pool — runs 33831058473 (attempt and bounded retry), 33829764813, 33757746363, 33752646002, 33741096098 — while the sandbox:docker leg running the identical shard and both macOS legs stay green. The job logs are admin-gated, so the cause comes from the shard's shape plus its own code. Failing steps run 22.4-25.5min against an 11.8-16.7min healthy baseline and all exit 1. Recomputing the shard with vitest's own sequencer (sha1 of the spec path, sorted, sliced) gives 25 files, and exactly one carries a budget that matches the delta: the daemon baseline's prompt-latency probe gets an explicit 10-minute timeout, issues 20 sequential real model prompts, and then asserts that the slowest of them stayed under 60s. p99 over 20 samples is the single worst round-trip, so what it measures on a 128-core ECS host shared with ~30 jobs is contention, not the daemon — and acp-integration.test.ts already documents real turns here taking 30-60+ seconds. Each of vitest's attempts re-issues all 20 prompts, which is where the extra minutes go. The probe is pool-only in effect: the file skips itself unless the sandbox is off, so the docker leg never runs it, while the dedicated macOS legs do and keep recording the baseline. Skip it on self-hosted runners the way integration-tests/vitest.config.ts already exempts them from the analogous pressure class, keeping QWEN_BASELINE_ENABLE_PROMPT_LATENCY=1 as the force-run override and recording why in the snapshot artifact. Measured: on a pool runner the file now registers 8 tests and the placeholder records the skip; with the clause removed it registers 7 and the probe runs again; on a dedicated runner the probe still runs and passes in 32.0s. Co-Authored-By: qwen-code-dev-bot <qwen-code-dev@service.alibaba.com>
The pool skip added in the previous commit tested the override for mere presence, so QWEN_BASELINE_ENABLE_PROMPT_LATENCY=0 — a maintainer saying "do not force this" — would have force-run the twenty-prompt probe on the shared pool. HAS_PROMPT_LATENCY_CREDENTIAL three lines above already reads the same variable as ===1; match it. Measured on a self-hosted runner: unset and =0 both register 8 tests with the skip placeholder passing, =1 registers 7 so the probe runs again. Co-Authored-By: qwen-code-dev-bot <qwen-code-dev@service.alibaba.com>
E2E report — issue #10994 (Main CI failed: E2E Tests on d4e3e4f)What the issue reportedE2E Tests run 33831058473 on Evidence gatheredThe job log is not obtainable here: the Actions logs endpoint returns Annotations for job Attempt 1 failed 1444s into a job whose setup took 177s — a 21.1 minute test phase — the workflow's bounded retry fired, and the retry ran 20.5 minutes and failed too. Step durations across the last 17 runs of this exact job:
Every failure exits 1 rather than running to The shard's exact contents were recomputed locally with vitest's own sequencer ( Root cause
Fix
No production code, no CI workflow, and no shared harness API changed. A first attempt in this round was revertedThe round's first commit (
Verification
Honest limit: without the log, no line names the failing test. The case for this file is that it is the only one in the shard whose budget equals the observed delta, whose execution is confined to the failing leg by a sandbox gate, and whose assertion is a wall-clock function of machine load. Three further load-sensitive assertions in the same file (a 1s attach-latency budget, an RSS check that fails above a 20% dropped-sample ratio from 100 ms-interval Commits on 中文说明E2E 报告 —— issue #10994（main 分支 CI 在 d4e3e4f 上 E2E Tests 失败）Issue 报告了什么
收集到的证据任务日志在这里拿不到：Actions 日志接口在没有凭证时返回 任务 首次尝试在任务进行到 1444 秒时失败，而该任务准备阶段耗时 177 秒——测试阶段为 21.1 分钟——随后工作流的受限重试触发，重试又跑了 20.5 分钟并同样失败。该任务最近 17 次运行的步骤耗时：
每次失败都以退出码 1 结束，而不是跑到 分片的确切内容用 vitest 自身的 sequencer 在本地重算（对 spec 路径取 根因
修复仅改动
未改动任何生产代码、CI 工作流或共享脚手架 API。 本轮的第一次尝试已被回滚本轮的第一个提交（
验证
诚实说明其局限：没有日志，就没有任何一行输出指名失败的测试。支持这个文件的理由是：它是分片中唯一预算恰好等于观察差值的文件、其执行范围被沙箱开关限制在失败的那条腿上、且其断言是机器负载的墙钟函数。同一文件中另外三个对负载敏感的断言（1 秒的 attach 延迟预算、一个在 100 毫秒间隔
🧠 Handled by Qwen Code · model/模型 |
github-actions
Bot
added
the
review/self-reported
|
|
|
Thanks for the PR! Template looks good ✓ Problem: observed, not theoretical. I pulled the five cited runs myself rather than taking the list on faith — all five are real Direction: aligned. Making a wall-clock percentile assertion conditional on whether the host is dedicated is the same move Size: not applicable — one file, Approach: the scope feels right and I don't see a smaller version of it. I considered the obvious alternatives before reading the diff — loosening Two claims in the description don't fully survive checking, neither of which touches the diff:
Risk: no elevated risk signals — Stage 1e matched nothing (single Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题：是已观测到的问题，不是理论性加固。我自己去拉了 PR 列出的五个 run，而不是直接采信这份清单——五个都是 方向：对齐。让一个 wall-clock 百分位断言取决于主机是否独占，正是 规模：不适用——单个文件 方案：范围合理，我想不到更小的版本。在读 diff 之前我考虑过几个显而易见的替代方案——放宽 描述中有两处说法经不起核对，但都不影响 diff 本身：
风险：无升级风险信号——Stage 1e 未命中任何项（单个 进入代码审查 🔍 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
Code reviewThe diff is small and I could check essentially all of it statically, so I did rather than reasoning from the description. The mechanism the fix depends on is real. The boolean is right on all four combinations. The rename is safe. Two non-blocking nits:
The one thing worth knowing before mergingNo PR-triggered CI job executes the new branch. I traced all three paths rather than inferring it from a single skipped check:
So green PR CI cannot confirm this fix works. The first real evidence is the next This is context, not a blocker: the change is test-only and reversible, and if the diagnosis turns out to be wrong the cost is a lost untrustworthy number on the pool legs plus a flake that persists — not a broken product. CI test evidenceFrom this PR's own checks on the reviewed commit, fetched via the API — I did not build or run anything.
Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 Sandboxed verification would settle this: Not verified: the causal attribution of the shard 2/3 failures to this specific probe (job logs are admin-gated; no annotation names a test), and the runtime behaviour of both branches of the new clause (no CI job reaches either). The author's local runs in the description are the author's claim on one Linux pool host, not independently re-run here. 中文说明代码审查diff 很小，几乎全部内容都能静态核对，所以我直接核了，没有只从描述推断。 修复所依赖的机制是真实存在的。 四种环境变量组合下的布尔逻辑都是对的。 重命名是安全的。 两个非阻塞的小问题：
合并前值得知道的一件事没有任何由 PR 触发的 CI job 会执行新增的分支。我把三条路径都追了一遍，而不是从某个 skipped 检查直接推断：
所以 PR CI 全绿无法证明这个修复有效。第一手真实证据来自合并后下一次 这是背景信息，不是阻塞项：改动仅涉及测试且可回退，如果诊断最终被证明是错的，代价只是池上的腿少了一个本就不可信的数字、flake 依旧存在——不会弄坏产品。 CI 测试证据以下取自本 PR 在被审 commit 上的检查，通过 API 获取——我没有构建或运行任何东西。
审查时 上方表格由 CI 区域标记包裹，CI 跑完后会由 finalize 流程原地更新。 沙箱验证可以定这件事： 未验证：把 shard 2/3 的失败归因到这个具体探测的因果关系（job 日志需管理员权限，没有 annotation 点名测试），以及新子句两个分支的运行时行为（没有 CI job 能触及任一方）。描述中作者的本地运行是作者在单台 Linux 池主机上的声明，此处未独立重跑。 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
|
Confidence: 4/5 — the mechanism checks out end to end and the diff is as small as this fix can be; the reservation is that PR CI cannot exercise either branch of it, so "green" here means nothing about whether the flake is actually gone. My independent proposal before reading the diff was the same one this PR makes: stop measuring a wall-clock percentile on a host you don't own, keep measuring it where you do, and record the skip rather than silently dropping the field. The alternatives I reached for first — loosening What I'd thank the author for in six months is the comment. Seven lines explaining why the pool is exempt, pointing at the Two things I'd want a maintainer to weigh, neither of which I think blocks this PR: The diagnosis is a hypothesis, and merging will close #10994 on it. I verified the symptom independently — all five cited runs are real, and the failing job is This is the fourth PR in a series that each teaches one more test to tolerate the pool. The author has ten PRs open, and #11001, #10858 and #10758 are the same class of shared-ECS flake mitigation as this one. This PR's own description points at three more load-sensitive assertions in the same file that "are worth their own pass", plus an unbounded teardown wait in a neighbouring file — so the queue of candidates is already written down. Each of these is individually correct and cheap, and I'm judging this one on its merits rather than on fatigue. But the direction of travel is a per-assertion exemption campaign against a 128-core host running ~30 concurrent jobs, and at some point that's a capacity or isolation question rather than a test question. Worth a maintainer deciding deliberately which it is, before the fifth and sixth of these land. I'm approving rather than deferring because the change is test-only, thirteen lines, fully reversible, follows a commented precedent in the same directory, and I could verify every branch of it statically — the env plumbing in
中文说明Confidence: 4/5 —— 机制从头到尾都核对得上，diff 也已经是这个修复能做到的最小形态；保留意见在于 PR CI 无法执行它的任一分支，所以这里的「绿」对 flake 是否真的消失没有任何说明力。 在读 diff 之前，我的独立方案与这个 PR 一致：在你不拥有的主机上停止测量 wall-clock 百分位，在你拥有的地方继续测，并把跳过记录下来，而不是静默丢掉这个字段。我最初想到的替代方案——放宽 六个月后我会感谢作者的是那段注释。七行说明了池为什么被豁免、指向 有两点希望维护者权衡，我认为都不构成对本 PR 的阻塞： 诊断是一个假设，而合并会据此关闭 #10994。 我独立核实了症状——五个被引用的 run 都是真实的，且每一次失败的 job 都是 ECS 池主机上的 这是同一系列里的第四个 PR，每一个都教会了一个测试去容忍这个池。 作者有十个 PR 处于 open 状态，其中 #11001、#10858、#10758 与本 PR 属于同一类共享 ECS flake 缓解。本 PR 的描述自己也指出了同一文件里另外三个「值得单独处理」的负载敏感断言，外加相邻文件里一个无上限的 teardown 等待——也就是说候选队列已经写好了。这些改动单独看都是正确且廉价的，我也是就本 PR 本身论断，而不是因为疲劳。但整体走向是：针对一台跑着约 30 个并发任务的 128 核主机，逐个断言地添加豁免。到某个点上，这就变成容量或隔离问题，而不是测试问题。在第五、第六个这类 PR 落地之前，值得维护者明确决定它属于哪一类。 我选择批准而不是暂缓，因为改动仅涉及测试、十三行、完全可回退、遵循同目录下一段带注释的先例，而且我能静态核对它的每一个分支——
— Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: reverse audit — stopped before round 1 by the review time budget.
Test Plan (not a blocker): cli/qwen-serve-baseline.test.ts — no such file or directory.
中文说明
仅完成部分审查，审查缺口已披露。
未审查：反向审计——评审时间预算不足，未能开始第 1 轮。
Test Plan（非阻断）：cli/qwen-serve-baseline.test.ts — no such file or directory。
— qwen3.8-max via Qwen Code /review (v0.23.0)
|
🔀 Base updated: red check(s) [Test (ubuntu-latest, Node 22.x)] pass on current main — merged current main via update-branch; CI will re-run. 中文说明🔀 已更新 base：红色检查 [Test (ubuntu-latest, Node 22.x)] 在当前 main 上通过 —— 已通过 update-branch 合入当前 main，CI 将重新运行。 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
1 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- R2-1 hardcoded "20" in skipReason — dropped as same-line overlap with comment 3931604834 (the R1-2 thread) at integration-tests/cli/qwen-serve-baseline.test.ts:692; distinct finding, its fix is folded into the R1-2 suggestion block and its …
Not reviewed: reverse audit — stopped before round 1 by the review time budget.
Test Plan (not a blocker): cli/qwen-serve-baseline.test.ts — no such file or directory.
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 1 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查：反向审计——评审时间预算不足，未能开始第 1 轮。
Test Plan（非阻断）：cli/qwen-serve-baseline.test.ts — no such file or directory。
— qwen3.8-max via Qwen Code /review (v0.23.0)
|
|
||
| if (SKIP_PROMPT_LATENCY) { | ||
| it('prompt latency skipped (no model credential env)', () => { | ||
| it('prompt latency skipped', () => { |
There was a problem hiding this comment.
[Suggestion] This changed test file runs in no pull_request-triggered CI lane — its earliest execution is the merge-queue integration_cli job (merge_group only), then post-merge/nightly e2e.yml.
The file sits outside every npm workspace, so no workspace test command collects it; the PR-lane gate integration_no_ak runs an explicit file list that does not include it, and e2e.yml has no pull_request trigger. A syntax, import, or logic error in this file therefore leaves every PR-lane check green through review and approval; the breakage first surfaces when the merge queue runs integration_cli — the same lane the open Critical (R1-1) shows still runs the 20-prompt probe, because it never maps RUNNER_ENVIRONMENT — turning the queue red for every queued PR until a follow-up fix lands.
Witness:
build-test (scoped): affected [], buildSet [], test [] — the file sits outside
every npm workspace (efficacy probe classified it unreachable).
PR-lane gate integration_no_ak runs an explicit file list without this file;
e2e.yml triggers: push / nightly / workflow_dispatch only — no pull_request.
vitest list --root ./integration-tests confirms the file collects cleanly;
earliest CI execution: ci.yml integration_cli (if: github.event_name == 'merge_group').
If PR-lane signal is wanted, add this file to the test:integration:no-ak:sandbox:none set in root package.json — without credentials it runs the non-model probes plus the placeholder (~31s measured on the pool). Otherwise accept merge-queue + post-merge coverage as the gate for this file, knowingly.
中文说明
本 PR 修改的测试文件不会在任何 pull_request 触发的 CI 腿上运行——它最早被执行的地方是合并队列的 integration_cli 任务（仅 merge_group 触发），其次是合并后/每夜的 e2e.yml。
该文件位于所有 npm workspace 之外，因此没有任何 workspace 测试命令会收集它；PR 腿的门禁 integration_no_ak 运行的是一个不包含该文件的显式文件列表，而 e2e.yml 没有 pull_request 触发器。因此该文件中的语法、导入或逻辑错误会让所有 PR 腿检查在评审与批准期间保持绿色；损坏首次暴露是在合并队列运行 integration_cli 时——而仍未关闭的 Critical（R1-1）表明，恰恰是这条腿从未映射 RUNNER_ENVIRONMENT、因而仍会运行那个 20 次请求的探测——结果是队列中每个排队的 PR 都会把队列染红，直到跟进修复落地。
见证：
build-test（作用域内）：affected []、buildSet []、test [] —— 该文件位于所有
npm workspace 之外（有效性探针将其归类为 unreachable）。
PR 腿门禁 integration_no_ak 运行的显式文件列表不含该文件；
e2e.yml 触发器：仅 push / 每夜 / workflow_dispatch —— 没有 pull_request。
vitest list --root ./integration-tests 确认该文件可被正常收集；
最早的 CI 执行：ci.yml integration_cli（if: github.event_name == 'merge_group'）。
如果希望 PR 腿上有信号，把该文件加入根 package.json 的 test:integration:no-ak:sandbox:none 集合——没有凭证时它会运行非模型探测加占位测试（在池上实测约 31 秒）。否则请知情地接受以合并队列 + 合并后覆盖作为该文件的门禁。
— qwen3.8-max via Qwen Code /review (v0.23.0)
There was a problem hiding this comment.
Escalated to a maintainer — not implemented this round. The finding checks out; the choice it poses is not ours to make.
Verified: integration-tests/ is outside every npm workspace (root package.json workspaces covers packages/*, packages/channels/* and integrations/*), so npm run test never collects this file; test:integration:no-ak:sandbox:none is an explicit file list that does not include it; e2e.yml has no pull_request trigger. The earliest CI execution really is ci.yml's merge_group-only integration_cli.
Two reasons this round cannot take the suggested fix. The change edits the root package.json scripts field, which is an explicit boundary for this automation and outside the PR's footprint. And the finding is itself framed as an either/or — "if PR-lane signal is wanted, add this file … otherwise accept merge-queue + post-merge coverage as the gate for this file, knowingly" — which is a coverage-versus-cost tradeoff for a maintainer, not something to settle silently in one direction.
One new data point for that decision. This round added integration-tests/cli/_prompt-latency-policy.test.ts, a pure-unit pin for the skip predicate: ~5 ms, no daemon, no credentials, and deterministic under any ambient env because the policy takes env as a parameter instead of reading process.env at import time. It sits in exactly the position you describe — no PR-lane collects it either. So if you do want PR-lane signal, adding just that file to the no-ak list is near-free and would guard the predicate this PR introduced, while adding qwen-serve-baseline.test.ts costs the ~31 s you measured on the pool. Say which and it is a one-line follow-up.
中文说明
已升级给维护者——本轮未实施。 该发现核实无误；但它提出的选择不由我们来做。
已核实：integration-tests/ 位于所有 npm workspace 之外（根 package.json 的 workspaces 覆盖 packages/*、packages/channels/* 与 integrations/*），因此 npm run test 永远不会收集本文件；test:integration:no-ak:sandbox:none 是一个不包含它的显式文件列表；e2e.yml 没有 pull_request 触发器。最早的 CI 执行确实就是 ci.yml 中仅由 merge_group 触发的 integration_cli。
本轮无法采纳建议修复的原因有两点。该改动会修改根 package.json 的 scripts 字段，这对本自动化是一条明确的边界，且不在本 PR 的 footprint 之内。而且该发现本身就是以二选一的形式提出的——「如果希望 PR 腿上有信号，就把该文件加入……否则请知情地接受以合并队列 + 合并后覆盖作为该文件的门禁」——这是一个覆盖与成本之间的取舍，应由维护者决定，而不是由我们悄悄选定一个方向。
为这个决策补充一个新的数据点。本轮新增了 integration-tests/cli/_prompt-latency-policy.test.ts，它是针对跳过谓词的纯单元固化测试：约 5 毫秒，不启动 daemon，不需要凭证，并且在任何外部环境变量下都是确定的——因为该策略把 env 作为参数接收，而不是在 import 时读取 process.env。它恰好处于你所描述的位置——同样没有任何 PR 腿会收集它。因此，如果你确实希望获得 PR 腿信号，只把这一个文件加入 no-ak 列表几乎零成本，并且能守护本 PR 引入的谓词；而把 qwen-serve-baseline.test.ts 加入则会带来你在池上实测到的约 31 秒开销。你决定采用哪一种，后续就是一行改动。
…10994) The snapshot's skipReason keyed only on credentials while the skip predicate has three disjuncts, so an explicit QWEN_BASELINE_SKIP_PROMPT_LATENCY=1 skip on a machine that does have a model credential was recorded as a shared-pool contention skip, together with a force-run switch the explicit flag outranks. The pool message also stated 20 round-trips whatever QWEN_BASELINE_PROMPT_ITERATIONS said. Move the predicate and the reason into one policy module that takes env as a parameter, and pin both with a unit test. The predicate mutated once on this branch already — the force-run override read as mere presence, which made =0 mean force-run — and no test went red.
|
🤖 Addressed the latest review feedback (round 1/10). What changed, and what I pushed back on: · 已处理最新评审反馈（第 1/10 轮）。改动内容与我反驳保留之处如下： Autofix round 3 — PR #11004 (issue #10994)Commit Dispositions
What changed
EvidenceR1-2 reproduced before implementing. HEAD's committed consts were extracted verbatim and evaluated under the reviewer's env matrix:
That is the false pool claim on a non-pool machine plus advice that cannot work, exactly as reported. Post-fix, end-to-end through the real harness (
The last row is the hardcoded-20 half of the finding: the reason now states the count the run actually used and matches Extraction did not shift behavior. Mutation probesEach mutation was applied to
P1 is the exact mutation this branch's own history contains ( Escalated: R1-1 (Critical)Confirmed independently, not taken on trust: Not implemented because Both R1-1 threads are left unresolved. Declined: R1-4The duplication is one line reading a GitHub-defined value ( The drift case is also not silent: The site count is unchanged by this round — the inline copy moved into the new pinned module. Reason recorded on both R1-4 threads; both left open. Escalated: R2-1 and the failed CVE checkR2-1 is accurate: Dependency CVE audit: FAILURE is not attributable to this PR. The job ( VerificationCommands actually run, at the committed tree state unless noted:
One transient failure worth recording: an early Not run: the full integration suite. The baseline harness issues real model round-trips and spawns daemons, so it is not runnable here without credentials; the touched behavior is covered by the focused runs and artifact reads above. Per the workflow's rules the independent CI remains the final gate. 中文说明Autofix 第 3 轮 — PR #11004（issue #10994）提交 处置结论
改动内容
证据R1-2 在实施前先复现。 将 HEAD 已提交的常量原文提取出来，在评审给出的环境矩阵下求值：
这正是所报告的问题：在非池机器上给出了错误的池归因，并附带一条无法生效的建议。 修复后，通过真实脚手架端到端验证（
最后一行对应发现中硬编码 20 的那一半：原因文本现在写出的是本次运行实际使用的次数，并与 抽取没有改变行为。 变异探测每一处变异都先施加到
P1 正是本分支自身历史中出现过的那次变异（ 升级事项：R1-1（Critical）已独立确认，而非照单采信： 未实施的原因： 两条 R1-1 讨论串均保持未解决状态。 拒绝事项：R1-4重复的只是一行读取 GitHub 定义取值的代码（ 而且漂移场景并非无声发生： 本轮也没有改变站点数量——内联副本已移入新的被固化模块。理由已记录在两条 R1-4 讨论串上；两条均保持打开。 升级事项：R2-1 与失败的 CVE 检查R2-1 描述准确： Dependency CVE audit: FAILURE 不可归因于本 PR。该任务（ 验证实际执行过的命令，除特别说明外均在已提交的树状态下运行：
有一处值得记录的瞬时失败：早期一次 未运行：完整的集成测试套件。基线脚手架会发出真实模型请求并启动 daemon，因此在没有凭证的环境下无法运行；上述聚焦运行与产物回读已覆盖本轮改动的行为。按工作流规则，独立的 CI 仍是最终门禁。 🦷 Gate advisory — this round resolves a Critical/Request-changes finding with test-only changes (machine-measured): the bite check cannot verify a test-side fix, so the resolution rests on the round summary alone. · 本轮以纯测试改动解决 Critical/Request-changes 反馈（门自动测量）：bite 检查无法验证测试侧修复，该解决仅以轮次摘要为凭。 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧵 Resolved all 4 selected review thread(s). · 已关闭全部选中的 4 条评审线程。 Re-review when you have a moment. After round 10 this bot stops and leaves the PR for a human. · 有空请复审；第 10 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
1 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- the merge-queue RUNNER_ENVIRONMENT gap re-derived by the oncall-persona audit — duplicate of carried R1-1 (comment 3932900497)
Not explored to full depth (tool budget reached): "agent 1a": none — no check was cut short.; "agent 1d": none — no check was cut short..
Test Plan (not a blocker): cli/qwen-serve-baseline.test.ts — no such file or directory.
Convergence: round 3 posted 4 inline comment(s), 1 of them reported for the first time; the previous round posted 5 (1 new). The rate of new findings is not falling. Batching the remaining fixes and verifying them before the next push, or dropping this PR's reviews to --severity-floor critical, keeps the loop from re-deriving the same set. (Observation only — nothing was withheld from this review because of this observation.)
中文说明
本轮确认的 1 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未探索到全部深度（达到工具调用预算）："agent 1a"：none — no check was cut short.；"agent 1d"：none — no check was cut short.。
Test Plan（非阻断）：cli/qwen-serve-baseline.test.ts — no such file or directory。
收敛情况：第 3 轮发布了 4 条行内评论，其中 1 条是首次提出；上一轮发布了 5 条（其中 1 条首次提出）。新发现的产出速度没有下降。把剩余修复攒成一批、验证后再推送，或将本 PR 的评审降到 --severity-floor critical，可以避免循环反复推导同一组发现。（仅为观察——本轮评审未因此扣留任何内容。）
— qwen3.8-max via Qwen Code /review (v0.23.0)
| (env['QWEN_BASELINE_ENABLE_PROMPT_LATENCY'] !== '1' && | ||
| env['RUNNER_ENVIRONMENT'] === 'self-hosted') || |
There was a problem hiding this comment.
[Critical] R1-1: [certifies-falsely] Still stands — re-checked at this commit; nothing in this round addresses it. The pool skip keys on RUNNER_ENVIRONMENT, but ci.yml's merge-queue integration_cli job (ci.yml:1960 — merge_group-only, routed onto the same self-hosted ECS pool via classify_pr, OPENAI_API_KEY set in the job env) runs this exact suite and never maps that variable: grep -n RUNNER_ENVIRONMENT .github/workflows/ci.yml returns 0 matches, while e2e.yml:256 remains the only mapping. On that leg shouldSkipPromptLatency(process.env) returns false, so the probe issues PROMPT_ITERATIONS real model round-trips under ~30-way host contention and asserts p99 under threshold inside the 10-minute budget, with vitest retry: 2 re-issuing all prompts on every attempt — the merge queue goes red with the same #10994 class this PR is meant to kill. It is latent only while the merge queue stays disabled repo-wide, and fires the moment a queue is switched on — which is exactly when integration_cli's only trigger is live.
Witness:
grep -n RUNNER_ENVIRONMENT .github/workflows/ci.yml -> 0 matches (e2e.yml:256 is the only mapping)
vitest list --root ./integration-tests cli/qwen-serve-baseline.test.ts (this round's code, da24577a8):
ARM A (OPENAI_API_KEY set, RUNNER_ENVIRONMENT unset — integration_cli's env):
real probe 'prompt latency > p50 / p99 over 20 prompts' registered
ARM B (same + RUNNER_ENVIRONMENT=self-hosted — the e2e pool's env):
'prompt latency skipped' placeholder registered instead
Fix — maintainer decision needed, escalated in round 2 (two candidate fixes posted there): map the variable on the leg the guard is meant to protect — add RUNNER_ENVIRONMENT: '${{ runner.environment }}' to the integration_cli test-step env in .github/workflows/ci.yml; the same line also restores integration-tests/vitest.config.ts's self-hosted accommodations (fork caps, unhandled-error exemption) on that leg. The in-footprint alternative — dropping the RUNNER_ENVIRONMENT disjunct and making the probe opt-in via QWEN_BASELINE_ENABLE_PROMPT_LATENCY=1 — is the only variant this PR can implement itself, and pays with baseline coverage on every leg that does not set the flag, including the dedicated macOS legs this PR deliberately keeps measuring. The mapping must use exactly the spelling/value established at .github/workflows/e2e.yml:256, since integration-tests/vitest.config.ts:14 and this policy are its two readers. If an assertion is added to scripts/tests/no-ak-integration-ci.test.js (which already parses the integration_cli job) that the job maps RUNNER_ENVIRONMENT, removing the mapping must make it red — analogous to scripts/tests/e2e-workflow.test.js:60 pinning the e2e.yml mapping.
中文说明
依然成立——已在本提交处复核，本轮没有任何改动处理该问题。池跳过逻辑以 RUNNER_ENVIRONMENT 为键，但 ci.yml 中合并队列的 integration_cli 任务（ci.yml:1960——仅 merge_group 触发、经 classify_pr 落到同一自建 ECS 池、任务 env 中设置了 OPENAI_API_KEY）运行着同一个测试套件，却从未映射该变量：grep -n RUNNER_ENVIRONMENT .github/workflows/ci.yml 返回 0 处匹配，而 e2e.yml:256 仍是唯一的映射。在该腿上 shouldSkipPromptLatency(process.env) 返回 false，探测会在约 30 个并发任务争用的主机上发出 PROMPT_ITERATIONS 次真实模型往返，并在十分钟预算内断言 p99 低于阈值，而 vitest 的 retry: 2 每次重试都会重新发出全部请求——合并队列会以与本 PR 要消灭的 #10994 同类的失败变红。它之所以只是潜伏的，是因为合并队列目前在仓库范围内处于禁用状态；一旦某个队列被启用它就会触发——而那正是 integration_cli 唯一触发条件生效的时候。
见证：
grep -n RUNNER_ENVIRONMENT .github/workflows/ci.yml -> 0 处匹配（e2e.yml:256 是唯一映射）
vitest list --root ./integration-tests cli/qwen-serve-baseline.test.ts（本轮代码，da24577a8）：
ARM A（设置 OPENAI_API_KEY、不设置 RUNNER_ENVIRONMENT —— 即 integration_cli 的环境）：
真实探测 'prompt latency > p50 / p99 over 20 prompts' 被注册
ARM B（同上 + RUNNER_ENVIRONMENT=self-hosted —— 即 e2e 池的环境）：
改为注册 'prompt latency skipped' 占位测试
修复——需要维护者决策，已在第 2 轮升级（那里贴出了两种候选修复）：把该变量映射到守卫本要保护的那条腿上——在 .github/workflows/ci.yml 的 integration_cli 测试步骤 env 中加入 RUNNER_ENVIRONMENT: '${{ runner.environment }}'；同一行还会在该腿上恢复 integration-tests/vitest.config.ts 对自建 runner 的照顾措施（fork 上限、未处理错误豁免）。footprint 内的替代方案——去掉 RUNNER_ENVIRONMENT 析取项、把探测改为仅由 QWEN_BASELINE_ENABLE_PROMPT_LATENCY=1 显式开启——是本 PR 自身唯一能实施的变体，代价是所有未设置该标志的腿（包括本 PR 特意保留测量的专用 macOS 腿）都丢失基线覆盖。映射必须完全沿用 .github/workflows/e2e.yml:256 处既有的拼写/取值，因为 integration-tests/vitest.config.ts:14 和本策略是它的两个读取方。如果在 scripts/tests/no-ak-integration-ci.test.js（它已在解析 integration_cli 任务）中增加断言要求该任务映射 RUNNER_ENVIRONMENT，那么删除该映射必须让测试变红——类似于 scripts/tests/e2e-workflow.test.js:60 对 e2e.yml 映射的固化。
— qwen3.8-max via Qwen Code /review (v0.23.0)
There was a problem hiding this comment.
Refuted — not implemented, and the round-2 escalation is withdrawn. The finding rests on one premise: that RUNNER_ENVIRONMENT reaches a step only if a workflow maps it. It does not. GitHub sets it as a default environment variable for every step on every runner, so the merge-queue leg this finding is about already sees it.
Official reference (Variables reference → Default environment variables), quoted verbatim:
The default environment variables that GitHub sets are available to every step in a workflow.
Because default environment variables are set by GitHub and not defined in a workflow, they are not accessible through the
envcontext. However, most of the default variables have a corresponding, and similarly named, context property.
|
RUNNER_ENVIRONMENT| The environment of the runner executing the job. Possible values are:github-hostedfor GitHub-hosted runners provided by GitHub, andself-hostedfor self-hosted runners configured by the repository owner. |
That second paragraph is exactly the runner.environment-context-vs-env-var distinction, and I think it is where both of us went wrong: the context property needs ${{ runner.environment }} in an if:, which makes the step env var look like it needs the same treatment. It does not.
So on ci.yml:1960 integration_cli — which its own if: conditions route onto the ecs-qwen pool — the vitest worker's process.env['RUNNER_ENVIRONMENT'] is self-hosted, shouldSkipPromptLatency(process.env) returns true at qwen-serve-baseline.test.ts:97, and the placeholder is registered. No probe, no p99 assertion, no #10994-class flake on that leg.
Why the two witnesses do not show otherwise:
grep -n RUNNER_ENVIRONMENT .github/workflows/ci.yml→ 0 matches measures only explicit restatements of a value the runner exports anyway. The mappings ate2e.yml:256,release.yml:698/704/750,qwen-autofix.yml:835/3956andrepo-hygiene.yml:300are all equally redundant — harmless, and they read naturally as the reason the absence inci.ymllooked like a gap.- ARM A / ARM B is a true statement about two env states, but ARM A is not a leg that exists. There is no runner where the variable is absent: GitHub sets it to one of exactly two values on every step.
In-repo corroboration that the default is real and load-bearing elsewhere: packages/cua-driver/rust/crates/cua-driver-testkit/src/windows_setup.rs:23 reads std::env::var("RUNNER_ENVIRONMENT") and branches on "github-hosted", and grep -c RUNNER_ENVIRONMENT .github/workflows/cd-cua-driver.yml → 0. Vendored upstream code already depends on the default being present with no mapping.
And nothing between the step and the assertion strips it: integration_cli has no container:; the step runs npm run test:integration:cli:sandbox:none → cross-env QWEN_SANDBOX=false vitest run --root ./integration-tests cli (cross-env preserves the parent env); integration-tests/globalSetup.ts deletes only NO_COLOR and test-helper.ts only NO_PROXY/no_proxy — there is no delete process.env['RUNNER_ENVIRONMENT'] anywhere in the harness.
Round-2 escalation withdrawn. My reply on comment 3932900497 asked a maintainer to choose between (a) mapping the variable on the leg and (b) making the probe opt-in. Neither is needed, and please do not spend a commit on (a): it would restate a value that is already exported. (b) would have cost baseline coverage on every leg that does not set the flag — including the dedicated macOS legs this PR deliberately keeps measuring — to fix a gap that does not exist. The grep-based inference was mine and it was wrong; sorry for the wasted question.
One-line falsifiable check for anyone who wants to settle it on the pool rather than from docs: add printenv RUNNER_ENVIRONMENT to any ci.yml step that does not map it.
Nothing changed in code for this finding, so the thread stays unresolved on purpose.
中文说明
已推翻——未实施，且第 2 轮的升级请求予以撤回。 该发现依赖一个前提：只有工作流显式映射，RUNNER_ENVIRONMENT 才会出现在步骤里。事实并非如此。GitHub 会为每个 runner 上的每个步骤把它设为默认环境变量，因此本发现所指的那条合并队列腿本来就能读到它。
官方参考（Variables reference → Default environment variables）原文引用：
The default environment variables that GitHub sets are available to every step in a workflow.（GitHub 设置的默认环境变量在工作流的每个步骤中都可用。）
Because default environment variables are set by GitHub and not defined in a workflow, they are not accessible through the
envcontext. However, most of the default variables have a corresponding, and similarly named, context property.（由于默认环境变量由 GitHub 设置而非在工作流中定义，它们无法通过env上下文访问；但大多数默认变量都有一个对应的、名称相似的上下文属性。）
|
RUNNER_ENVIRONMENT| The environment of the runner executing the job. Possible values are:github-hostedfor GitHub-hosted runners provided by GitHub, andself-hostedfor self-hosted runners configured by the repository owner. |（执行该任务的 runner 环境。取值只可能是github-hosted（GitHub 提供）或self-hosted（仓库所有者自建）。）
上面第二段恰恰就是 runner.environment 上下文与同名环境变量之间的区别，我认为这正是我们双方出错的地方：上下文属性必须在 if: 中写成 ${{ runner.environment }}，这就让人误以为步骤环境变量也需要同样的处理。其实不需要。
因此在 ci.yml:1960 的 integration_cli 上——它自身的 if: 条件就把它路由到 ecs-qwen 池——vitest worker 的 process.env['RUNNER_ENVIRONMENT'] 就是 self-hosted，qwen-serve-baseline.test.ts:97 处的 shouldSkipPromptLatency(process.env) 返回 true，注册的是占位测试。那条腿上不会有探测、不会有 p99 断言、也不会出现 #10994 同类的 flake。
为什么两条见证并不能说明相反的情况：
grep -n RUNNER_ENVIRONMENT .github/workflows/ci.yml→ 0 处匹配，衡量的只是对「runner 本来就会导出的取值」的显式重述次数。e2e.yml:256、release.yml:698/704/750、qwen-autofix.yml:835/3956与repo-hygiene.yml:300处的映射同样是冗余的——无害，而且它们的存在恰好让ci.yml里的「缺失」看起来像一个缺口。- ARM A / ARM B 对两种环境变量状态的描述是真的，但 ARM A 并不是现实中存在的某条腿。不存在该变量缺失的 runner：GitHub 会在每个步骤把它设为两个取值之一。
仓库内部也印证了这个默认值确实存在、且在别处被真实依赖：packages/cua-driver/rust/crates/cua-driver-testkit/src/windows_setup.rs:23 读取 std::env::var("RUNNER_ENVIRONMENT") 并按 "github-hosted" 分支，而 grep -c RUNNER_ENVIRONMENT .github/workflows/cd-cua-driver.yml → 0。上游引入的代码本来就依赖该默认值存在、且无需任何映射。
而且从步骤到断言之间没有任何环节会剥掉它：integration_cli 没有 container:；该步骤执行 npm run test:integration:cli:sandbox:none → cross-env QWEN_SANDBOX=false vitest run --root ./integration-tests cli（cross-env 会保留父进程环境）；integration-tests/globalSetup.ts 只删除 NO_COLOR，test-helper.ts 只删除 NO_PROXY/no_proxy——整个脚手架里没有任何 delete process.env['RUNNER_ENVIRONMENT']。
撤回第 2 轮的升级请求。 我在评论 3932900497 上的回复请维护者在 (a) 在该腿上映射该变量与 (b) 把探测改为显式开启之间做选择。两者都不需要，也请不要为 (a) 花一次提交：那只是重述一个已经导出的取值。(b) 则会为了修复一个并不存在的缺口，让所有未设置该标志的腿（包括本 PR 特意保留测量的专用 macOS 腿）都丢失基线覆盖。基于 grep 的那个推断是我做出的，而且它是错的；为这个多余的问题致歉。
如果有人在池上而不是从文档出发来做一次可证伪的检查，一行即可：在任何未映射该变量的 ci.yml 步骤里加上 printenv RUNNER_ENVIRONMENT。
本发现没有任何代码改动，因此该 thread 有意保持未解决状态。
| return ( | ||
| env['QWEN_BASELINE_SKIP_PROMPT_LATENCY'] === '1' || | ||
| (env['QWEN_BASELINE_ENABLE_PROMPT_LATENCY'] !== '1' && | ||
| env['RUNNER_ENVIRONMENT'] === 'self-hosted') || |
There was a problem hiding this comment.
[Suggestion] R1-4: Still stands at this commit — the location moved with the extraction. The self-hosted-runner detection is inlined here although integration-tests/vitest.config.ts:14 already holds the identical expression (const isSelfHostedRunner = process.env['RUNNER_ENVIRONMENT'] === 'self-hosted';, module-local, not exported). The round-2 reply declined this citing diff growth and pointed at scripts/tests/integration-vitest-config.test.js as the drift catch — but that test pins only the config's own use of the value, not this copy. If pool-runner classification ever changes (a renamed RUNNER_ENVIRONMENT value, or an added condition at vitest.config.ts:14), one site gets updated and the other is missed: either the baseline re-issues its real model round-trips on the contended pool — re-creating exactly the #10994 flake this PR fixes — or the probe is silently skipped on legs the config does not consider contended, losing baseline coverage with no failure signal. This round's extraction created the natural shared module; the policy can host the export at near-zero extra diff.
Witness:
witness: not run — structural-duplication claim settled by quoting both sites (integration-tests/vitest.config.ts:14 and integration-tests/cli/_prompt-latency-policy.ts:48); no run capability applies.
Single source for the check: export the constant from this module (e.g. export const isSelfHostedRunner = ...) or from integration-tests/vitest.config.ts, and use it in both places. The shared expression must keep exactly RUNNER_ENVIRONMENT === 'self-hosted' as established at integration-tests/vitest.config.ts:14 — runner.environment is either github-hosted or self-hosted, and both this policy and the config's exemptions read that value.
中文说明
在本提交处依然成立——位置随本轮抽取移动。自建 runner 的检测在此处内联，尽管 integration-tests/vitest.config.ts:14 已持有完全相同的表达式（const isSelfHostedRunner = process.env['RUNNER_ENVIRONMENT'] === 'self-hosted';，模块内私有、未导出）。第 2 轮的回复以 diff 增长为由拒绝了本项，并指出 scripts/tests/integration-vitest-config.test.js 可以捕获漂移——但该测试固化的只是配置自身对该取值的使用，并不覆盖这里的副本。如果池 runner 的判定方式发生变化（RUNNER_ENVIRONMENT 的取值改名，或 vitest.config.ts:14 处增加新条件），会出现一处更新而另一处被遗漏：要么基线在争用的池上重新发出真实模型往返——重新制造本 PR 正在修复的 #10994 flake——要么探测在配置并不认为争用的腿上被悄然跳过，基线覆盖在没有任何失败信号的情况下丢失。本轮的抽取已经创建了天然的共享模块；由该策略承载这个导出几乎不增加额外 diff。
见证：
witness: not run —— 结构性重复的论断通过引用两处站点（integration-tests/vitest.config.ts:14 与 integration-tests/cli/_prompt-latency-policy.ts:48）即可定案；没有可适用的运行能力。
为该检测建立单一来源：从本模块导出该常量（例如 export const isSelfHostedRunner = ...），或从 integration-tests/vitest.config.ts 导出，并在两处使用。共享表达式必须精确保留 integration-tests/vitest.config.ts:14 处既有的 RUNNER_ENVIRONMENT === 'self-hosted'——runner.environment 的取值非 github-hosted 即 self-hosted，本策略与配置的豁免措施都读取该值。
— qwen3.8-max via Qwen Code /review (v0.23.0)
There was a problem hiding this comment.
Declined again — recorded, not dropped. Round 2's reasoning stands, and two things this round sharpen it.
The value is GitHub's, not the repo's. As documented in the sibling reply on this file, RUNNER_ENVIRONMENT is a GitHub default environment variable whose only two values are github-hosted and self-hosted (Variables reference). The first drift case you name — "a renamed RUNNER_ENVIRONMENT value" — is an ecosystem-wide GitHub change that would hit both sites identically and simultaneously, not a local edit one site could miss.
Both dedupe directions cost more than the duplicated line. integration-tests/vitest.config.ts imports exactly three modules today — vitest/config, node:path, node:url — and refers to ./globalSetup.ts only as a config string. It imports no repo-local module at all. Exporting from the policy would make the config that gates every integration lane depend on a helper that exists for one test file under cli/: delete that file with its test later and the config breaks, taking every lane with it. That is a worse coupling than a duplicated literal. The other direction — export from the config, import into the policy — makes the policy import a module that calls defineConfig and reads process.env at import time, which is precisely what this round's extraction removed: shouldSkipPromptLatency(env) takes env as a parameter so the whole matrix is testable under a controlled environment.
Both copies are now individually pinned. The config's copy by scripts/tests/integration-vitest-config.test.ts, which re-imports the config under controlled RUNNER_ENVIRONMENT values and asserts the fork caps and the unhandled-error exemption; it runs in the PR lane (npm run test:scripts, ci.yml:759, inside the test job). The policy's copy by _prompt-latency-policy.test.ts — mutation-probed at this commit: renaming the 'self-hosted' literal inside shouldSkipPromptLatency turns 2 of its 11 tests red (skips on the shared self-hosted pool, force-runs on the pool for ENABLE=1 only, not mere presence).
Your narrow point is correct and I am not disputing it: those two pins are independent, so a repo-local change to the classification rule at one site would not redden the other's test. That is the real cost of not deduping, paid against a one-line comparison with an externally fixed value. If a maintainer reads that tradeoff the other way, say so and I will implement the shared constant as a follow-up rather than decline it a third time.
中文说明
再次拒绝——已记录理由，并非悄悄丢弃。 第 2 轮的推理依然成立，本轮有两点让它更清晰。
这个取值属于 GitHub，不属于本仓库。 正如同一文件旁那条回复中引用的文档，RUNNER_ENVIRONMENT 是 GitHub 的默认环境变量，取值只有 github-hosted 与 self-hosted 两种（Variables reference）。你提到的第一种漂移情形——「RUNNER_ENVIRONMENT 的取值被改名」——是波及整个生态的 GitHub 侧变更，会同时、同样地命中两处站点，而不是某处可能漏改的本地编辑。
两个去重方向的代价都大于那一行重复。 integration-tests/vitest.config.ts 目前只 import 三个模块——vitest/config、node:path、node:url——并且只以配置字符串的形式引用 ./globalSetup.ts。它完全没有 import 任何仓库内模块。若从策略模块导出，就会让把控每条集成测试腿的配置去依赖一个只为 cli/ 下某一个测试文件而存在的辅助模块：将来把那个文件连同其测试一起删掉，配置就会崩，并连带所有腿一起崩。这比一行重复的字面量是更糟的耦合。反方向——从配置导出、由策略 import——则会让策略去 import 一个调用 defineConfig 且在 import 时读取 process.env 的模块，而这恰恰是本轮抽取所要消除的：shouldSkipPromptLatency(env) 把 env 作为参数接收，正是为了让整个判定矩阵能在受控环境下被测试。
两份副本现在各自都有固化测试。 配置那份由 scripts/tests/integration-vitest-config.test.ts 固化：它在受控的 RUNNER_ENVIRONMENT 取值下重新 import 配置，并断言 fork 上限与未处理错误豁免；该测试在 PR 腿上运行（npm run test:scripts，ci.yml:759，位于 test 任务内）。策略那份由 _prompt-latency-policy.test.ts 固化——已在当前提交做变异探测：把 shouldSkipPromptLatency 内部的 'self-hosted' 字面量改名，会使其 11 个测试中的 2 个变红（skips on the shared self-hosted pool、force-runs on the pool for ENABLE=1 only, not mere presence）。
你那个更细的论点是对的，我不否认：这两处固化彼此独立，因此若有人只对其中一处做仓库本地的判定规则改动，另一处的测试不会变红。这就是不去重的真实代价，而它换来的是一行与外部固定取值做比较的代码。如果维护者对这个取舍有相反判断，请说明，我会把共享常量作为后续改动实施，而不是第三次拒绝它。
…10994) Two mutations of the prompt-latency policy survived the suite: renaming any credential key other than OPENAI_API_KEY, and deleting the trailing force-run advice from the no-credential reason, which a prefix regex did not reach. Iterate a hardcoded list of all six keys so a mangled entry names itself in the failure, and assert that reason verbatim.
|
🤖 Addressed the latest review feedback (round 2/10). What changed, and what I pushed back on: · 已处理最新评审反馈（第 2/10 轮）。改动内容与我反驳保留之处如下： Round 4 — review feedback addressedCommit
Dispositions
Also noted for the next round, from the review body ([rv:5112834660]): its Test Plan line Environment noteThe checkout had no git identity configured, so the first commit attempt failed with "Author identity unknown". Verification
Mutation probes. Every mutation was applied to a file this round does not change, reverted before the next probe, and the file was then confirmed byte-identical to
Threads left unresolved on purpose: R1-1 (declined), R1-4 (declined), R2-1 (escalated). 中文说明第 4 轮——评审反馈处理结果提交
各项处置
另外为下一轮记录一条，来自评审正文（[rv:5112834660]）：其 Test Plan 中的 环境说明该 checkout 没有配置 git 身份，因此第一次提交尝试以 "Author identity unknown" 失败。 验证
变异探测。每次变异都施加在本轮不改动的文件上，在下一次探测前还原，随后确认该文件与
有意保持未解决的 thread：R1-1（拒绝）、R1-4（拒绝）、R2-1（升级）。 🦷 Gate advisory — this round resolves a Critical/Request-changes finding with test-only changes (machine-measured): the bite check cannot verify a test-side fix, so the resolution rests on the round summary alone. · 本轮以纯测试改动解决 Critical/Request-changes 反馈（门自动测量）：bite 检查无法验证测试侧修复，该解决仅以轮次摘要为凭。 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧵 Resolved all 1 selected review thread(s). · 已关闭全部选中的 1 条评审线程。 Re-review when you have a moment. After round 10 this bot stops and leaves the PR for a human. · 有空请复审；第 10 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed.
1 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- the new unit test file's PR-gating CI unreachability (Agent 7 [test] finding) — already reported as R2-1 (comment 3933872624)
Test Plan (not a blocker): cli/qwen-serve-baseline.test.ts — no such file or directory.
2 Suggestion(s) were drafted inline past the resolved critical posting floor — the floor engaged early: the first-time-finding rate has not fallen for 2 consecutive round(s); the CLI moved them into the deferral list below (floor enforcement).
Deferred under the convergence posture (round 4, not a blocker) — the floor engaged early: the first-time-finding rate has not fallen for 2 consecutive round(s) — recorded, not requested in this round:
integration-tests/cli/_prompt-latency-policy.test.ts:123 — [review] R4-4: The credential-before-pool branch order in promptLatencySkipReason is pinned by no test. There is no pool-without-credential case: the SKIP cases short-circuit at t…integration-tests/cli/_prompt-latency-policy.test.ts:46 — [review] R4-5: Credential recognition is pinned only in the positive direction — no test asserts that an unrecognized env key does NOT count as a credential, so broadened matching (e…integration-tests/cli/_prompt-latency-policy.test.ts:73 — [probe] R4-1 SKIP flag's strict '=== 1' comparison pinned by no test (deferred: anchored on code unchanged since the previous round)integration-tests/cli/_prompt-latency-policy.test.ts:111 — [probe] R4-2 branch-swap mutation in promptLatencySkipReason survives every test (deferred: anchored on code unchanged since the previous round)integration-tests/cli/_prompt-latency-policy.test.ts:30 — [probe] R4-3 ENABLE '=== 1' strictness inside hasCredential pinned by no test (deferred: anchored on code unchanged since the previous round)integration-tests/cli/_prompt-latency-policy.test.ts:86 — [probe] R4-6 QWEN_CUSTOM_API_KEY_* prefix pinned by exactly one suffix (deferred: anchored on code unchanged since the previous round)integration-tests/cli/_prompt-latency-policy.ts:48 — [review] R1-4 self-hosted-runner detection duplicated inline despite vitest.config.ts:14 holding the identical expressionintegration-tests/cli/qwen-serve-baseline.test.ts:663 — [review] R2-1 none of the files this PR touches runs in any pull_request-triggered CI lane
中文说明
已审查。
本轮确认的 1 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
Test Plan（非阻断）：cli/qwen-serve-baseline.test.ts — no such file or directory。
2 条 Suggestion 在已解析的 critical 发布下限之外被起草为行内评论——发布下限因首次发现速率连续 2 轮未下降而提前生效；CLI 已将其移入下方延后清单（下限强制执行）。
收敛姿态下延后（第 4 轮，非阻断）——发布下限因首次发现速率连续 2 轮未下降而提前生效——已记录，本轮不要求修改：共 8 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.23.0)
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
Closeout pass:
The fixed thread and outdated duplicate threads have been resolved. I left the two evidence-based disagreement threads open for reviewer confirmation. |
chiga0
left a comment
chiga0
left a comment
There was a problem hiding this comment.
Reviewed at 1c8009e — no blocker found; approving.
Cross-check against prior rounds
The only Critical carried through rounds 1–3 was R1-1 (RUNNER_ENVIRONMENT not mapped in ci.yml's merge-queue leg). The round-3 dev-bot reply at comment 3934343070 correctly refutes it: RUNNER_ENVIRONMENT is a GitHub default environment variable set on every step by the Actions runner itself — no workflow mapping is required, and the self-hosted ECS pool already sets it to self-hosted without any ci.yml change.
R1-2 (skipReason misrecording explicit-skip case): fixed in da24577 — promptLatencySkipReason now guards the explicit flag first.
R2-1 (no PR-triggered CI lane): fixed in 06c6dec — _prompt-latency-policy.test.ts is now listed in test:integration:no-ak:sandbox:none.
R1-3/R1-4 and the round-4 deferred suggestions are all Suggestion-level; the round-4 COMMENTED "Reviewed" confirms no new Critical was found after the batch of fixes.
What I checked independently
shouldSkipPromptLatencythree-disjunct logic at HEAD: explicit-skip → force-run-vs-self-hosted → no-credential; correct.CREDENTIAL_ENV_KEYSlist and theQWEN_CUSTOM_API_KEY_*prefix walk: recognises all documented auth patterns.promptLatencySkipReasonbranch order: explicit flag → no-credential → pool; comment explains the ordering relative to the predicate; correct.package.jsonno-ak test list at HEAD:_prompt-latency-policy.test.tsis present.- Merge-queue
integration_cliinci.yml: noRUNNER_ENVIRONMENTmapping (as expected — GitHub injects it as a default).
|
🤖 Addressed the latest review feedback (round 3/10). What changed, and what I pushed back on: · 已处理最新评审反馈（第 3/10 轮）。改动内容与我反驳保留之处如下： Round 5 — review feedback addressedCommit
What this round actually fixedThe round's one substantive item was not in The closeout commit Reproduced before changing anything: The fix is the single missing array element, in the position that matches Dispositions
Footprint expansion — required, not chosenThis round touches top-level
Mutation probeThe pin's new entry is load-bearing in both directions, verified rather than assumed:
Verification
中文说明第 5 轮 —— 已处理评审反馈提交
本轮实际修复了什么本轮唯一的实质问题并不在 收尾提交 在动手之前先复现： 修复就是补上那一个缺失的数组元素，位置与 处理结论
足迹扩张 —— 是必需，不是选择本轮触及了顶层
变异探针钉中新增的这一项在两个方向上都是有效的，这一点经过验证而非假设：
探针前已对 验证
🧭 Gate advisory — this round modified areas outside the PR footprint (machine-measured, not agent-authored):
Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 Re-review when you have a moment. After round 10 this bot stops and leaves the PR for a human. · 有空请复审；第 10 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
#11004 added ./cli/_prompt-latency-policy.test.ts to test:integration:no-ak:sandbox:none but left the byte-exact pin in no-ak-integration-ci.test.js without it, so `npm run test:scripts` — and with it the required `Test (ubuntu-latest, Node 22.x)` check — fails on every branch whose base includes that commit, this one included.
#11004 added ./cli/_prompt-latency-policy.test.ts to the no-AK integration script in package.json without updating the pin that asserts that script byte-for-byte, so the scripts suite has been red ever since. The no-AK gate itself already runs the new test and passes, so the script is the correct side and the pin was stale.
* fix(test): stop measuring model latency anywhere under CI (QwenLM#11271) The prompt-latency probe in qwen-serve-baseline sends 20 real prompts through the shared OpenAI-compatible gateway and asserts p99 < 60s. One gateway-queued prompt in twenty fails it, and each vitest retry re-issues all 20 prompts into the same degraded window, so a single slow window fails every attempt and turns the shard red. Since the pool skip (QwenLM#11004) the probe runs on exactly one E2E leg — macOS shard 2/2, the only shard holding the file — and that leg failed twice within six hours on unrelated commits (runs 34070970091 and 34088422718, both full-duration with retries consumed) while every Linux leg, macOS shard 1/2, and the OpenTUI leg stayed green. The measured quantity is gateway weather, not the daemon; the same argument the pool skip made for host contention applies to the shared gateway. Skip the probe whenever CI is set (any populated CI marker, matching the repo's other CI checks), keeping the self-hosted disjunct so a pool-shaped shell outside CI keeps its specific skip reason. The probe still runs off CI on a credential, and QWEN_BASELINE_ENABLE_PROMPT_LATENCY=1 force-runs it anywhere. The skip reason distinguishes CI gateway contention from pool host contention. * fix(test): tolerate transient model-serving errors in the acp plan-mode case (QwenLM#11271) The CI-wide prompt-latency skip rested on a misattribution: in both cited runs the latency probe ran and passed, and the macOS leg went red on cli/acp-integration.test.ts > blocks write tools in plan mode — killed by a transient model-serving -32603 returned over ACP, with the retry attempt then tripping ENOTEMPTY on the previous attempt's leftover .qwen-home. Revert the CI skip, re-issue the plan-mode session/prompt on the observed gateway -32603 shape (bounded at 3 attempts, 2s apart, inside the existing per-request timeout), and let the TestRig.setup directory reset retry through the mid-delete refill race the way globalSetup's teardown already does. * test(e2e): isolate protocol checks from model service * fix(test): surface fake-server setup failures in json-output teardown (QwenLM#11271) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(test): close ACP teardown race and tighten fake-server E2E (QwenLM#11271) - Move per-agent QWEN_HOME out of rig.testDir: the agent keeps writing there ~300ms after exit, racing global teardown's recursive rm (ENOTEMPTY). cleanup() now removes it with retries. - Drop the plan-mode test's permissionHandler: the plan-mode guard short-circuits before any permission request, so it could never run and its comment claimed coverage the test does not have. - Name the collected tool-call events in the plan-mode assertion so a wire-shape drift is distinguishable from a plan-mode let-through. - Stub loopback NO_PROXY in json-output tests: an inherited HTTP(S)_PROXY otherwise tunnels the fake-server POST and the cases time out. Two-chunk fixture restores multi-delta accumulator coverage. - Make the auth-mismatch case hermetic via fakeModelArgs instead of the ambient OPENAI_* secrets. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(test): reclaim leaked ACP qwen homes and unify scratch teardown (QwenLM#11271) --------- Co-authored-by: yiliang114 <effortyiliang@gmail.com> Co-authored-by: 易良 <1204183885@qq.com> Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
Released in v0.23.1. |
4 participants
What this PR does
Stops the daemon performance baseline from asserting a model-latency percentile on the shared self-hosted CI pool, where that number measures host contention rather than the daemon. The probe still runs everywhere the machine is dedicated — the macOS legs and any local or forced run — and still records a skip with a reason in the snapshot artifact when it does not.
Why it's needed
The
E2E Test (Linux) - sandbox:none - shard 2/3leg has failed intermittently on the shared pool: runs 33831058473 (both the attempt and its bounded retry), 33829764813, 33757746363, 33752646002 and 33741096098 all exited 1 inRun E2E tests, while thesandbox:dockerleg running the identical shard and both macOS legs stayed green in the same runs. Those job logs are admin-gated and cannot be downloaded without credentials, so the cause was reconstructed from public check-run metadata plus the shard's own code.Two measurements frame it. Failing steps run 22.4–25.5 minutes against an 11.8–16.7 minute healthy baseline for the same job, and every failure exits 1 rather than hanging — retries paying for a genuine assertion failure, not a wedged process. Recomputing the shard with vitest's own sequencer algorithm gives its exact 25 files, and one of them carries a budget that matches the ten-minute delta precisely: the prompt-latency probe gets an explicit ten-minute timeout, issues twenty sequential real model prompts, then asserts that the slowest of them stayed under sixty seconds.
A p99 over twenty samples is the single worst round-trip, so on a 128-core ECS host shared with roughly thirty concurrent jobs it measures contention. Another file in the same shard already documents real turns against this endpoint taking 30–60+ seconds, which makes the threshold a coin flip under load — and each of vitest's three attempts re-issues all twenty prompts, which is where the extra minutes go. The probe is effectively pool-only in its failure: the file skips itself unless the sandbox is off, so the docker leg never executes it, while the dedicated macOS legs do.
Reviewer Test Plan
How to verify
On a simulated pool runner the probe must be skipped and the skip must be recorded:
Expected: the file registers 8 tests and the run reports
1 passed | 7 skipped (7).Then confirm coverage is not lost where the machine is dedicated — drop
RUNNER_ENVIRONMENTand run the probe itself:Expected: the file registers 7 tests (no placeholder, because nothing is skipped) and
prompt latency > p50 / p99 over 20 promptspasses, taking roughly 32s for the twenty real round-trips.Finally, confirm the new clause is load-bearing rather than decorative: delete the
RUNNER_ENVIRONMENT === 'self-hosted'condition fromSKIP_PROMPT_LATENCYand re-run the first command. Expected: the file registers 7 tests, nothing matches the filter, and the run reports1 skipped (1)/7 skipped (7)— i.e. on a pool runner the twenty-prompt probe would execute again and assert its percentile. Restoring the clause returns the first command to1 passed | 7 skipped.Also worth confirming the force-run override, which is read as exactly
1to match how the credential check three lines above reads the same variable:QWEN_BASELINE_ENABLE_PROMPT_LATENCY=1on a self-hosted runner drops the placeholder back to 7 registered tests so the probe runs, while=0keeps it skipped at 8 — a presence test there would have made=0mean "force", the opposite of what a maintainer setting it intends.Evidence (Before & After)
N/A — test-harness change, no user-visible or TUI surface.
Pool runner, before (clause removed — the pre-change behaviour):
Pool runner, after:
Pool runner, override semantics —
=0keeps the skip,=1restores the probe:Dedicated runner, after — the probe still runs and still measures:
Tested on
Environment (optional)
Linux self-hosted ECS pool host,
QWEN_SANDBOX=false, against the bundled CLI fromnpm run build && npm run bundle. Both the pool-runner and dedicated-runner behaviours above were measured on this machine by setting and unsettingRUNNER_ENVIRONMENT, which is the same variablee2e.ymlalready maps in forintegration-tests/vitest.config.ts.Risk & Scope
QWEN_BASELINE_ENABLE_PROMPT_LATENCY=1.pscalls drop, and a ten-second budget for MCP grandchildren to appear. Elsewhere in the shard, the ACP integration file has an unbounded teardown wait that escalates only to SIGTERM, and three files skip themselves on the pool for the same reason this one now does.Linked Issues
Fixes #10994
中文说明
这个 PR 做了什么
让 daemon 性能基线不再在共享的自建 CI runner 池上断言模型延迟百分位——在那种环境里这个数字衡量的是主机争用，而不是 daemon 本身。该探测在机器独占的地方依然会运行（macOS 那几条腿，以及任何本地或强制运行的场景），并且在确实跳过时仍会在快照产物中记录跳过及其原因。
为什么需要
E2E Test (Linux) - sandbox:none - shard 2/3这条腿在共享池上间歇性失败：run 33831058473（首次尝试与其受限重试）、33829764813、33757746363、33752646002 和 33741096098 都在Run E2E tests步骤以退出码 1 结束，而同一次运行中跑着完全相同分片的sandbox:docker腿以及两条 macOS 腿都是绿的。这些任务日志需要管理员权限，没有凭证无法下载，因此原因是通过公开的 check-run 元数据加上分片自身代码还原出来的。有两个测量结果界定了问题。失败步骤耗时 22.4–25.5 分钟，而同一任务的健康基线是 11.8–16.7 分钟，且每次失败都以退出码 1 结束而不是卡死——这是重试在为真实的断言失败付出代价，而不是进程被卡住。用 vitest 自身的 sequencer 算法重新计算分片，可以得到确切的 25 个文件，其中恰好有一个的预算与这十分钟的差值精确吻合：prompt 延迟探测有明确的十分钟超时，会串行发出二十次真实模型请求，然后断言其中最慢的一次必须在六十秒以内。
二十个样本的 p99 就是最差的那一次往返，所以在一台与大约三十个并发任务共享的 128 核 ECS 主机上，它衡量的是争用。同一分片中的另一个文件已经记录了这个端点上真实对话可能耗时 30–60 秒以上，这使得该阈值在负载下形同抛硬币——而 vitest 的每一次尝试都会重新发出全部二十个请求，多出来的分钟数正是这样产生的。这个探测在失败面上实际上只属于池 runner：该文件在沙箱未关闭时会跳过自身，所以 docker 腿根本不会执行它，而独占的 macOS 腿会。
评审测试计划
如何验证
在模拟的池 runner 上，探测必须被跳过，且跳过必须被记录下来：
预期：该文件注册 8 个测试，运行结果为
1 passed | 7 skipped (7)。然后确认在机器独占的地方没有丢失覆盖率——去掉
RUNNER_ENVIRONMENT，直接运行探测本身：预期：该文件注册 7 个测试（没有占位测试，因为没有任何跳过），且
prompt latency > p50 / p99 over 20 prompts通过，二十次真实往返大约耗时 32 秒。最后确认新增的条件是承重的、而非装饰性的：从
SKIP_PROMPT_LATENCY中删掉RUNNER_ENVIRONMENT === 'self-hosted'这个条件，再跑第一条命令。预期：该文件注册 7 个测试，过滤器匹配不到任何测试，运行结果为1 skipped (1)/7 skipped (7)——也就是说在池 runner 上，那个二十次请求的探测会重新执行并断言它的百分位。把条件改回去，第一条命令恢复为1 passed | 7 skipped。另外值得确认强制运行覆盖变量，它被读取为恰好等于
1，以与上方三行凭证检查对同一变量的读法保持一致：在自建 runner 上QWEN_BASELINE_ENABLE_PROMPT_LATENCY=1会让占位测试消失、注册数回到 7 个从而运行探测，而=0保持跳过、注册数为 8。若那里只判断变量是否存在，=0就会变成「强制运行」，与设置它的维护者意图完全相反。证据（改动前与改动后）
N/A —— 测试脚手架改动，没有用户可见或 TUI 层面的变化。
池 runner，改动前（移除该条件——即改动前的行为）：
池 runner，改动后：
池 runner，覆盖变量语义 ——
=0保持跳过，=1恢复探测：独占 runner，改动后 —— 探测仍然运行并仍然测量：
测试环境
环境（可选）
Linux 自建 ECS 池主机，
QWEN_SANDBOX=false，针对由npm run build && npm run bundle产出的 CLI bundle 运行。上面池 runner 与独占 runner 两种行为都是在同一台机器上通过设置与取消RUNNER_ENVIRONMENT测得的，而这正是e2e.yml已经为integration-tests/vitest.config.ts映射进来的同一个变量。风险与范围
QWEN_BASELINE_ENABLE_PROMPT_LATENCY=1即可。ps调用丢失超过 20% 采样时就失败的 RSS 采样检查，以及等待 MCP 孙进程出现的十秒预算。分片中的其他地方，ACP 集成文件有一个无上限的 teardown 等待、且只升级到 SIGTERM；另有三个文件出于与此处相同的原因在池上跳过自身。关联 Issue
Fixes #10994