Skip to content

test(core): pin Config.initialize() join ordering so the await-drop mutant fails - #11113

Merged
wenshao merged 1 commit into
QwenLM:mainfrom
yiliang114:test/core-pin-initialize-join-ordering
Sep 5, 2026
Merged

test(core): pin Config.initialize() join ordering so the await-drop mutant fails#11113
wenshao merged 1 commit into
QwenLM:mainfrom
yiliang114:test/core-pin-initialize-join-ordering

Conversation

@yiliang114

Copy link
Copy Markdown
Collaborator

What this PR does

Hardens the makes a concurrent caller join the in-flight initialization test in packages/core/src/config/config.test.ts so it actually pins the ordering property it is named for. The test previously released the gate before asserting anything and never checked that the joining caller was still pending while the first flight was in progress. This adds an ordering assertion: track settlement of both callers and assert nothing has settled before the gate is released.

Test-only change; no production code touched.

Why it's needed

This closes the one remaining review finding from #11037 that #11075 did not cover (the post-merge review's §3, a test-quality gap).

A join branch that drops the awaitif (!this.initializationSettled) { return; } instead of awaiting this.initializationPromise — passed the old test, because the test released the gate before asserting, so by the time it checked, the joiner had already (wrongly) resolved and initializeInternal had run exactly once. That mutant silently reproduces #11002: the joiner proceeds before initialization completes and dies on Chat not initialized. It was killed only incidentally by shares a failed in-flight initialization, for an unrelated reason (error-identity), so the test that names the join behavior never actually guarded it.

Reviewer Test Plan

How to verify

  1. cd packages/core && npx vitest run src/config/config.test.ts — full suite green (621 passed locally).
  2. Mutation witness: remove the await this.initializationPromise; line from the join branch in config.ts. The makes a concurrent caller join the in-flight initialization test now fails — the joiner settled while the gate was still held. Restore the line and it is green again.

Evidence (Before & After)

Before (join branch with the await dropped, i.e. the #11002-restoring mutant):

FAIL  src/config/config.test.ts > Server Config (config.ts) > initialize > makes a concurrent caller join the in-flight initialization
AssertionError: expected [ 'second' ] to deeply equal []

After (await intact): 621 passed (621).

Tested on

OS Status
🍏 macOS ✅ tested
🪟 Windows ⚠️ not tested
🐧 Linux ⚠️ not tested

Risk & Scope

  • Main risk or tradeoff: none; the change adds one assertion to an existing test. The setTimeout(0) flush drains the microtask queue before the ordering check, so the assertion is deterministic rather than timing-dependent.
  • Not validated / out of scope: the OpenTUI unbounded-and-silent join UX noted in the same review (§4) is left for a separate follow-up.
  • Breaking changes / migration notes: none.

Linked Issues

Follow-up to #11037 (review finding §3). Complements #11075, which resolved R1-1 through R1-4.

中文说明

这个 PR 做了什么

加固 packages/core/src/config/config.test.ts 中的 makes a concurrent caller join the in-flight initialization 用例,让它真正钉住它所命名的顺序性质。此前该用例在断言任何内容之前就释放了门闩,也从未检查「加入方在第一次初始化仍在进行时是否仍未落定」。本改动新增一条顺序断言:跟踪两个调用方的落定情况,并在释放门闩之前断言没有任何一方落定。

仅测试改动,未触碰任何生产代码。

为什么需要它

这闭合了 #11037#11075 未覆盖的那一条遗留评审发现(合并后评审的 §3,一处测试质量缺口)。

一个去掉 await 的 join 分支——if (!this.initializationSettled) { return; } 而非 await this.initializationPromise——在旧用例下会通过:因为用例在断言前就释放了门闩,等它检查时,加入方已经(错误地)resolve,且 initializeInternal 恰好运行一次。该变异体会悄悄重现 #11002:加入方在初始化完成前就继续执行,死于 Chat not initialized。它只是被 shares a failed in-flight initialization 因另一个无关理由(错误对象同一性)顺带杀掉,因此那个为 join 行为命名的用例从未真正守住它。

审阅者测试计划

如何验证

  1. cd packages/core && npx vitest run src/config/config.test.ts —— 全套绿(本地 621 通过)。
  2. 变异见证:从 config.ts 的 join 分支移除 await this.initializationPromise; 一行。makes a concurrent caller join the in-flight initialization 用例随即失败——加入方在门闩仍被挂起时就落定了。还原该行则重新变绿。

风险与范围

  • 主要风险或取舍:无;本改动只为既有用例新增一条断言。setTimeout(0) 冲刷会在顺序检查前排空微任务队列,因此该断言是确定性的,而非依赖时序。
  • 未验证 / 范围之外:同一评审指出的 OpenTUI「无界且无声」join 的 UX 问题(§4)留给单独的后续 PR。
  • 破坏性变更 / 迁移说明:无。

关联 Issue

#11037 的后续(评审发现 §3)。与已解决 R1-1 ~ R1-4 的 #11075 互补。

…utant fails

The 'makes a concurrent caller join the in-flight initialization' test released
the gate before asserting anything and never checked that the joining caller was
still pending mid-flight. A join branch that drops the await (returning
immediately) therefore passed this test while silently reproducing QwenLM#11002: the
joiner proceeds before initialization completes and dies on 'Chat not
initialized'. It was killed only incidentally by the failure-sharing test, for an
unrelated reason.

Assert nothing has settled while the first flight is still gated, so the named
test actually pins the ordering property it describes. Verified: full config
suite 621 passed; removing the await now fails this test with ['second'] vs [].

Follow-up to QwenLM#11037 (review finding 3, not covered by QwenLM#11075).
@qwen-code-ci-bot

qwen-code-ci-bot commented Sep 5, 2026

Copy link
Copy Markdown
Collaborator

⚠️ Deferred approval withheld — 1 PR CI workflow run(s) on 2292189 did not finish green; see the updated table in the Stage 2 comment. Re-run @qwen-code /triage after fixes. finalize run

⚠️ 延迟审批已搁置 —— 2292189 有 1 个 PR CI workflow 未以绿色完成,详见 Stage 2 评论中已更新的表格。修复后可重新运行 @qwen-code /triage查看 finalize 运行

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Thanks for the PR!

Template looks good ✓ — every required heading is there, the Tested-on table is filled in, and the Chinese section mirrors the English.

Problem: demonstrated rather than theoretical, which is the part I checked hardest. The claim is that a join branch with the await dropped — if (!this.initializationSettled) { return; } — passes the test as it stands today. I traced that mutant through the existing test rather than taking it on faith, and it holds: release() runs before the first assertion, so by the time the test looks, the joiner has already resolved, initializeInternal has still been called exactly once, and the trailing rejects.toThrow('Config was already initialized') still passes because initializationSettled is flipped in the finally. Nothing in the test objects. The claim that the mutant is only killed incidentally by shares a failed in-flight initialization is also right, and for the unrelated reason stated — with the mutant the joiner resolves to undefined, so it is expect(secondError).toBe(firstError) that goes red, an error-identity property, not the ordering one. So the test named for the join behaviour was not the one guarding it.

Direction: aligned. This sits at the end of a real chain — #11002 was a main-branch OpenTUI interactive E2E failure where a submitted prompt was lost to Chat not initialized, root-caused and fixed by #11037 and then hardened by #11075. A fix whose regression guard does not actually fail when the fix is removed is a fix that can silently regress, so closing that gap is worth the PR rather than being code hygiene.

Size: the file is under packages/core/src/config/, so the core-path gate applies, but the breakdown is 0 production logic lines / 15 test lines / 0 generated-or-schema lines — the single changed file matches *.test.ts and is excluded from the count. No Tier 1 block (the title is test(core):, not a refactor), no 500-line maintainer escalation, and no 1000-line splitting advisory. Nothing downstream consumes a .test.ts file, so there is no consumer list to name.

Approach: the scope feels exactly right, and I arrived at the same shape independently — track when each caller settles, flush the microtask queue with a macrotask, assert nothing has settled, and only then release the gate. It is also the pattern the neighbouring rejects a joining caller whose signal is already aborted test already uses, down to the comment explaining why the assertion has to land while the gate is held, so this reads as consistent with the file rather than bolted on. Tracking first alongside second is not scope creep either: it is what makes the test self-validating, since if the gate ever stopped blocking, the second assertion would be vacuously true and first still unsettled is what proves the premise holds. No unrelated edits, no drive-by refactor, no formatting churn — I could not cut anything from this diff without losing the point.

Risk: no elevated risk signals. The revert-history path check matches nothing here (the only changed file is a test).

Moving on to code review. 🔍

中文说明

感谢贡献!

模板完整 ✓ —— 所有必需标题都在,Tested on 表格已填写,中文部分与英文逐段对应。

问题: 这是已被证实的问题,而不是理论性加固,也是我核查最用力的一点。PR 的主张是:去掉 await 的 join 分支——if (!this.initializationSettled) { return; }——在现有用例下会通过。我没有直接采信,而是把这个变异体沿着现有用例走了一遍,主张成立:release() 在第一条断言之前执行,所以等用例去检查时,加入方早已 resolve,initializeInternal 仍恰好被调用一次,而结尾的 rejects.toThrow('Config was already initialized') 依然通过,因为 initializationSettled 是在 finally 里翻转的。用例没有任何一处会反对。关于「该变异体只是被 shares a failed in-flight initialization 顺带杀掉」的说法同样正确,且原因与所述一致地无关——在变异体下加入方 resolve 成 undefined,因此变红的是 expect(secondError).toBe(firstError),那是错误对象同一性,而不是顺序性质。所以那个为 join 行为命名的用例,并不是真正守住它的用例。

方向: 对齐。这处在一条真实链路的末端——#11002 是 main 分支上一次 OpenTUI 交互式 E2E 失败,提交的 prompt 死于 Chat not initialized,由 #11037 定位并修复,再由 #11075 加固。一个「把修复删掉也不会失败」的回归护栏,等于一个可以悄悄回退的修复,因此补上这个缺口值得一个 PR,而不只是代码卫生。

规模: 该文件位于 packages/core/src/config/ 下,所以核心路径门禁适用,但拆分结果是:生产逻辑 0 行 / 测试 15 行 / 生成或 schema 0 行——唯一改动的文件匹配 *.test.ts,不计入生产行数。无 Tier 1 硬阻断(标题是 test(core):,不是 refactor),无 500 行维护者关注,也无 1000 行拆分建议。没有任何下游会消费一个 .test.ts 文件,所以这里没有需要点名的消费者清单。

方案: 范围恰当,我独立推导出的形状也一致——记录每个调用方何时落定,用一个宏任务冲刷微任务队列,断言无人落定,然后才释放门闩。这也正是相邻的 rejects a joining caller whose signal is already aborted 用例已经在用的模式,连「断言必须在门闩仍被挂起时落地」的解释性注释都一致,因此读起来是与此文件相符,而非硬加上去的。同时跟踪 firstsecond 也不是范围蔓延:它让用例自我验证——万一门闩哪天不再真的阻塞,second 的断言会空洞地为真,而「first 仍未落定」正是证明前提成立的那一条。无无关改动、无顺手重构、无格式抖动——这个 diff 里我砍不掉任何东西而不损失其意义。

风险: 无升级风险信号。回退历史路径检查在此没有命中(唯一改动的文件是测试)。

进入代码审查 🔍

Qwen Code · qwen3.8-max-2026-09-02

Reviewed at 2292189e8f2fc77099a0f8a443d1570973ff011f · re-run with @qwen-code /triage

@qwen-code-ci-bot

qwen-code-ci-bot commented Sep 5, 2026

Copy link
Copy Markdown
Collaborator

Code review

I reviewed this statically against the merge-base code — the diff plus Config.initialize() and the surrounding describe('initialize') block. No blockers, and no AGENTS.md violations. What I actually checked, since a test-only diff is easy to wave through:

The determinism claim holds. await new Promise((resolve) => setTimeout(resolve, 0)) parks on a macrotask, and the event loop drains the whole microtask queue before running the next macrotask, so both .then callbacks have necessarily fired by the time the assertion runs. That makes the check a statement about ordering rather than about how long a tick took — the opposite of a sleep-and-hope. Worth being explicit about because it is the difference between a regression guard and a flake.

The fake-timer hazard is absent. This was the one way the change could have quietly introduced a hang rather than a failure: if fake timers were active in this file, setTimeout(0) would never fire and the test would sit until testTimeout. I grepped packages/core/src/config/config.test.ts for useFakeTimers / useRealTimers / fakeTimers — no matches. And packages/core/vitest.config.ts sets testTimeout to 15s (60s on the ecs-qwen- runners), so even a pathological case fails fast instead of wedging CI.

The unhandled-rejection hazard is absent too, and it was worth checking. The two .then(...) calls create derived promises with no rejection handler, and the same vitest config sets dangerouslyIgnoreUnhandledErrors: process.platform !== 'linux' — so on the Linux CI runners an unhandled rejection is not swallowed and would fail the run. Neither promise can reject here: initializeInternal is mocked to gate, release() resolves it, and the pre-existing await Promise.all([first, second]) already asserts both resolve. Fine as written.

Nothing was weakened. The change is purely additive (+15/−0) — release(), Promise.all, toHaveBeenCalledOnce() and the trailing rejects.toThrow('Config was already initialized') all survive untouched, and the new assertion is inserted before release() so the existing sequence is preserved rather than reordered around it. This is assertion-preserving hardening, not a test made green by loosening it.

The comment earns its 9 lines. AGENTS.md defaults comments to none, and nine comment lines against six code lines would normally make me look twice. Here the why is genuinely non-obvious — which mutant it kills, and why the old ordering let that mutant through — and the two neighbouring tests in this block already carry the same kind of explanatory comment for the same reason. Consistent with the file, not a departure from it.

Test evidence

This is an unattended CI run, so per the triage rules I did not build, run, or execute anything from this PR — the evidence below is the PR's own CI, read off the check-runs API for the reviewed commit. Nothing was re-run locally.

There are no failures on this commit. The unit suite, lint, and the no-AK integration run were still in flight at review time, so they are listed as pending rather than guessed at; the table is refreshed in place once CI settles. Test (macos-latest) and Test (windows-latest) are skipped, not red — the Linux job is the one that runs the suite here. The author reports 621 passing locally on macOS; that is the author's claim, not something this review re-ran.

Final CI results for 2292189 (auto-updated by the triage finalize job after CI completed):

Check Conclusion
web-shell E2E Smoke (ubuntu-latest, Node 22.x) 🚫 cancelled
Classify PR ✅ success
Desktop Shell (ubuntu-22.04) ✅ success
Desktop Shell (windows-2022) ✅ success
Integration Tests (no-AK, No Sandbox) ✅ success
Lint & Static (ubuntu-latest, Node 22.x) ✅ success
Test (ubuntu-latest, Node 22.x) ✅ success

One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行(取最新一次运行),省略 skipped,失败项排在最前。

A green suite cannot substantiate this PR's actual claim, and that is worth naming. The point of the change is that the test now fails when the await is dropped from the join branch. CI runs the test against unmutated production code, so it passes identically with and without these 15 lines — green here proves the assertion does not break the build, not that it kills the mutant. Not verified in this review: the mutation witness itself. I confirmed by tracing the code that the mutant would resolve second immediately and therefore hit expected [ 'second' ] to deeply equal [], which matches the reported failure, but that is static reasoning, not an executed run — and the author's before/after is a single-platform (macOS) result I did not re-run.

Sandboxed verification would settle it: @qwen-code /verify — that dropping await this.initializationPromise; from the join branch in config.ts turns makes a concurrent caller join the in-flight initialization red is the whole load-bearing claim of this PR, and it is exactly the kind of thing a passing suite says nothing about. /tmux is not the lane here — there is no TUI surface in the diff.

Real-scenario testing: N/A — unattended CI run, and nothing user-visible changes. No tmux capture was driven, per the CI-path rules.

中文说明

代码审查

我以静态方式对照 merge-base 代码做了审查——diff 加上 Config.initialize() 与所在的 describe('initialize') 块。没有阻断项,也没有违反 AGENTS.md。以下是我真正核查的内容,因为纯测试 diff 很容易被挥手放行:

确定性主张成立。 await new Promise((resolve) => setTimeout(resolve, 0)) 停在一个宏任务上,而事件循环会在处理下一个宏任务之前排空整个微任务队列,所以断言执行时两个 .then 回调必然已经触发。这让该检查成为关于顺序的陈述,而不是关于「一个 tick 有多长」的陈述——与 sleep 碰运气正相反。这一点值得写明,因为它正是「回归护栏」与「偶发失败」的区别。

不存在 fake timer 隐患。 这是本改动可能悄悄引入「挂起」而非「失败」的唯一路径:若本文件启用了 fake timers,setTimeout(0) 永不触发,用例会一直等到 testTimeout。我在 packages/core/src/config/config.test.ts 中检索了 useFakeTimers / useRealTimers / fakeTimers——无匹配。且 packages/core/vitest.config.tstestTimeout 设为 15s(ecs-qwen- runner 上为 60s),因此即便出现极端情况也是快速失败,而不会卡死 CI。

同样不存在未处理 rejection 隐患,而这一点值得查。 两个 .then(...) 创建了没有 rejection 处理器的派生 promise,而同一份 vitest 配置里有 dangerouslyIgnoreUnhandledErrors: process.platform !== 'linux'——也就是说在 Linux CI runner 上,未处理的 rejection 不会被吞掉,会让本次运行失败。此处两个 promise 都不可能 reject:initializeInternal 被 mock 成 gaterelease() 会 resolve 它,且既有的 await Promise.all([first, second]) 本就断言二者都 resolve。按现状写没问题。

没有任何断言被削弱。 改动是纯增量的(+15/−0)——release()Promise.alltoHaveBeenCalledOnce() 以及结尾的 rejects.toThrow('Config was already initialized') 全部原样保留,且新断言插入在 release() 之前,因此既有序列被保留,而不是被围绕它重排。这是保留断言的加固,不是靠放松用例换来的绿。

那段注释对得起它的 9 行。 AGENTS.md 默认不写注释,9 行注释对 6 行代码通常会让我多看两眼。这里的 why 确实不显然——它杀掉的是哪个变异体、以及旧的顺序为何会放它过去——而且本块中相邻的两个用例已经因为同样的理由带着同类解释性注释。与本文件一致,并非偏离。

测试证据

这是一次无人值守的 CI 运行,因此按 triage 规则,我没有构建、运行或执行本 PR 的任何代码——下面的证据是 PR 自身的 CI,通过 check-runs API 针对被审查的提交读取。本地未重跑任何内容。

该提交上没有失败项。审查时单元套件、lint 与 no-AK 集成运行仍在进行中,因此列为 pending 而非猜测结果;CI 落定后表格会就地刷新。Test (macos-latest)Test (windows-latest) 是 skipped,不是 red——这里跑套件的是 Linux job。作者报告本地 macOS 上 621 通过;那是作者的说法,不是本次审查重跑的结果。

绿色套件无法证实本 PR 真正的主张,这一点值得点名。 本改动的意义在于:当 join 分支的 await 被去掉时,用例现在会失败。CI 是针对未变异的生产代码运行该用例的,所以有无这 15 行它都以同样方式通过——此处的绿只证明该断言没有破坏构建,并不证明它杀掉了变异体。本次审查未验证:变异见证本身。我通过代码走查确认了该变异体立即 resolve second,因此会命中 expected [ 'second' ] to deeply equal [],与所报失败一致,但那是静态推理,不是实际执行——而作者的 before/after 是我未重跑的单平台(macOS)结果。

沙箱化验证可以定这件事:@qwen-code /verify —— 「从 config.ts 的 join 分支移除 await this.initializationPromise; 会让 makes a concurrent caller join the in-flight initialization 变红」正是本 PR 的承重主张,也正是一套通过的套件完全说不出名堂的那类事情。这里不该用 /tmux——diff 中没有 TUI 界面。

真实场景测试:N/A —— 无人值守 CI 运行,且无任何用户可见变化。按 CI 路径规则未驱动 tmux 抓取。

Qwen Code · qwen3.8-max-2026-09-02

Reviewed at 2292189e8f2fc77099a0f8a443d1570973ff011f · re-run with @qwen-code /triage

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Confidence: 5/5 — I went looking for the two ways a setTimeout(0) test change goes wrong and both are absent; the mutant claim I traced myself and it is exactly as described.

Stepping back: this is the third PR in the #11002#11037#11075 chain, and chain fatigue is a real way to wave something through, so I tried to judge it on its own. It survives that. The two merged PRs fixed the bug and hardened the join path, but neither left behind a test that fails when the fix is removed — I confirmed that by walking the await-drop mutant through the existing test, where it passes every assertion including the trailing rejects.toThrow, and through shares a failed in-flight initialization, where it is caught only by error identity rather than by the ordering property anyone would go looking for. So the gap is real and specific, not a general sense that coverage could be better.

The implementation matches what I would have written independently, down to flushing the microtask queue with a macrotask and asserting before releasing the gate — which is also what the neighbouring aborted-signal test already does, so this reads as the file's own idiom rather than an imported one. Six lines of test code and nine lines explaining which mutant they exist to kill. If I come back to this in six months and feel tempted to tidy the assertion into something shorter, that comment is what stops me, which is the whole argument for it.

I checked the two failure modes that would not show up in a passing run. Fake timers would turn setTimeout(0) into a hang rather than a failure — none are enabled in this file. And the unhandled .then derivations would matter specifically on Linux, where dangerouslyIgnoreUnhandledErrors is off — but neither promise can reject here, since the mock is the gate and the pre-existing Promise.all already requires both to resolve. Neither is load-sensitive either: a slower timer only drains more microtasks, which strengthens the assertion, and the correct code cannot settle before release() regardless of scheduling.

Nothing is deferred, nothing is weakened, and there is no production surface to regress. The one thing I could not establish by execution is the mutation witness itself, and I would rather be plain about that than let a green suite stand in for it: CI runs this test against unmutated code, so it passes with or without these 15 lines. I verified the witness statically and named /verify above as the lane that would settle it by running. That is a gap in what this review could execute, not a doubt about the change.

Approval is deferred until CI lands green on 2292189e8f2fc77099a0f8a443d1570973ff011f — the unit suite, lint, and the no-AK integration run were still in flight at review time. The finalize job will post the commit-pinned approval once they complete, and will withhold it if anything lands red or the head moves.

中文说明

信心度:5/5 —— 我专门去找「用 setTimeout(0) 改测试」会出问题的两种路径,两种都不存在;变异体主张是我自己走查的,与描述完全一致。

退一步看:这是 #11002#11037#11075 这条链上的第三个 PR,而链路疲劳确实会让人顺手放行,所以我试着单独评判它。它经得起。前两个已合并的 PR 修了 bug、加固了 join 路径,但都没有留下一个「把修复删掉就会失败」的用例——我把去掉 await 的变异体沿现有用例走了一遍确认了这点:它通过每一条断言,包括结尾的 rejects.toThrow;而在 shares a failed in-flight initialization 里它被抓住靠的是错误对象同一性,而不是任何人会去查的那个顺序性质。所以这个缺口是具体且真实的,不是「覆盖率还能更好」这类泛泛之感。

实现与我独立会写出的方案一致,细到用宏任务冲刷微任务队列、并在释放门闩之前断言——这也正是相邻的 aborted-signal 用例已经在做的,所以读起来是本文件自己的写法,而不是外来的一套。6 行测试代码,9 行说明它们是为了杀掉哪个变异体而存在。半年后我若回来,想把这条断言「收拾」得更短,拦住我的正是那段注释——这也正是它存在的理由。

我查了两种在绿色运行里不会暴露的失败模式。fake timers 会把 setTimeout(0) 变成挂起而不是失败——本文件没有启用。未处理的 .then 派生 promise 恰恰在 Linux 上才要紧,因为那里 dangerouslyIgnoreUnhandledErrors 是关闭的——但此处两个 promise 都不可能 reject:mock 就是门闩,而既有的 Promise.all 本就要求二者都 resolve。两者也都与负载无关:定时器更慢只会排空更多微任务,那反而加强断言;而正确的代码无论调度如何都不可能在 release() 之前落定。

没有任何东西被推迟处理、被削弱,也没有可回退的生产面。唯一我无法通过执行确立的是变异见证本身,我宁可把这点讲明白,也不愿让一套绿色套件替它作证:CI 是针对未变异的代码运行这个用例的,所以有无这 15 行它都通过。我以静态方式验证了该见证,并在上面点名 /verify 作为能通过实际运行来定这件事的路径。这是本次审查能执行的范围上的缺口,而不是对改动本身的疑虑。

批准推迟至 CI 在 2292189e8f2fc77099a0f8a443d1570973ff011f 上全绿——审查时单元套件、lint 与 no-AK 集成运行仍在进行中。finalize job 会在它们完成后发布绑定该提交的批准;若有任一项变红或 head 发生移动,则不予批准。

Qwen Code · qwen3.8-max-2026-09-02

Reviewed at 2292189e8f2fc77099a0f8a443d1570973ff011f · re-run with @qwen-code /triage

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

⚠️ Downgraded from Approve to Comment: CI still running. Partially reviewed — gaps disclosed.

Not reviewed: reverse audit — stopped before round 1 by the review time budget.

Test Plan (not a blocker): src/config/config.test.tsno such file or directory; 621 passed — this review observed 23448, 1946, 28642, 298, 1816, 504, 6013 passed.

中文说明

⚠️ 已从批准降级为评论:CI still running。 仅完成部分审查,审查缺口已披露。

未审查:反向审计——评审时间预算不足,未能开始第 1 轮。

Test Plan(非阻断):src/config/config.test.tsno such file or directory; 621 passed — this review observed 23448, 1946, 28642, 298, 1816, 504, 6013 passed

— qwen3.8-max via Qwen Code /review (v0.23.0)

@wenshao

wenshao commented Sep 5, 2026

Copy link
Copy Markdown
Collaborator

Maintainer verification — built and run locally on Linux

I rebuilt the change in a clean worktree and drove every claim in the PR body end to end, plus the two things a test-only diff has to prove on its own: that the new assertion kills the mutant it names, and that the pre-PR test did not. Both hold. Recommend merge.

Environment — Debian 13 (trixie), kernel 6.12.63, 16 vCPU, Node v22.22.2, npm 10.9.7, vitest 3.2.7. Worktree at PR head 2292189e8f, merge-base f747999539. This fills in the 🐧 Linux row the PR body left as not tested.


1. The PR body's numbers reproduce exactly

npx vitest run src/config/config.test.ts on PR head → 621 passed (621) — the same count the PR claims. Prettier, ESLint and tsc --noEmit on packages/core are all clean.

2. The mutation witness holds, and the counterfactual proves it is new

Removing await this.initializationPromise; from the join branch of Config.initialize() (§3's M3) and running only the named test:

with the PR's test file with the pre-PR test file
makes a concurrent caller join the in-flight initialization FAILAssertionError: expected [ 'second' ] to deeply equal [] pass

That right-hand column is the load-bearing half: the mutant survives the case that names the behaviour on f747999539, so the assertion is genuinely new coverage, not a restatement. The failure text matches the PR body character for character.

mutation A/B

Running the whole file under M3 confirms §3's other claim too. On the pre-PR file exactly one test goes red — shares a failed in-flight initialization with concurrent callers, and it fails on expected undefined to be Error: startup discovery exploded, i.e. error identity, not ordering. On PR head both tests go red. So the file was never green under M3; what changes is that the failure now lands on the test that describes the property, with a message that says what actually broke.

3. The assertion generalises past the one mutant it was written for

I ran four more "joins the flight but doesn't wait for it" mutants through the same command. All four are newly killed — the assertion pins ordering, not one particular token:

join-branch mutant with PR test with pre-PR test newly killed
M3 — drop the await FAIL pass
await Promise.resolve() then return FAIL pass
await new Promise(r => setTimeout(r, 0)) then return FAIL pass
await Promise.resolve(undefined) then return FAIL pass
throw instead of joining FAIL FAIL already covered
drop signal.throwIfAborted() pass pass n/a — killed by the sibling abort test (verified)

mutant matrix

The macrotask row matters: the mutant's own setTimeout(0) is registered before the test's, so this is not an artifact of microtask depth — the assertion still sees the joiner settle early.

4. The determinism claim survives CPU starvation

setTimeout(0) parking is the one thing that could make this flaky on a loaded runner, so I measured it rather than reasoned about it: 40/40 green — 25 plain runs, plus 15 runs pinned to a single core (taskset -c 0) with four busy-loop spinners contending for that same core. No failures, no retries.

5. What the pinned property actually buys, on the real built core

To confirm the consequence is what the PR says it is, I drove the caller shape from packages/cli/src/ui/opentui/live-session.ts:562-568 (try { await config.initialize(); } catch {}getGeminiClient()getChat()) against packages/core/dist, with the first flight starting the chat at the end of initialization — the real ordering. Only the join branch differs between the two runs:

#11002 mechanism

Shipped code: the joiner waits 180 ms and gets a started chat. With the await dropped: it returns in 0 ms and the real LlmClient.getChat() throws Chat not initialized#11002, from a change that the pre-PR suite let through this test.


Two non-blocking notes

Neither affects the green path; both are one edit in the same block, so I mention them while the code is open.

N1 — settled records fulfilment, not settlement. first.then(cb) fires only on resolve, so a joiner that rejects has settled but is not recorded, and expect(settled).toEqual([]) passes vacuously. Observable today: under the throw-instead-of-join mutant the run fails with Error: join disabled from the later await Promise.all(...), never with an AssertionError — line 5164 passed while the joiner had already settled.

N2 — the two floating .then() promises add an unhandled-rejection channel. packages/core/vitest.config.ts:44 sets dangerouslyIgnoreUnhandledErrors: process.platform !== 'linux', so on the ubuntu leg an unhandled rejection reddens the run even when every test passes. I probed it directly: a throwaway test that passes while leaving one floating rejected promise gives Tests 1 passed (1) / Errors 1 error / exit 1. Under the throw mutant, PR head reports Unhandled Rejection on top of the test failure; the pre-PR file reports none. Today this can only fire on a run that is already red — it is noise, not a false failure — but it points at production code rather than at the assertion.

Both are covered by the two-argument .then form. I applied it locally and re-ran everything:

const record = (who: string) => () => {
  settled.push(who);
};
first.then(record('first'), record('first'));
second.then(record('second'), record('second'));

621 passed, prettier clean, M3 still dies on the same AssertionError — and the throw mutant now dies on that assertion (expected [ 'second' ] to deeply equal []) instead of on Promise.all, with zero unhandled rejections. Genuinely optional; the PR is correct as written.


Status note

Test (ubuntu-latest, Node 22.x) was still in_progress when I wrote this (~1h20m inside Run tests and generate reports); it and REVIEW_REQUIRED are what leave the PR BLOCKED. Nothing in my local run suggests a problem with it — the file this PR touches passes, including under contention.

Verdict: approve / merge. The claim is demonstrated, the counterfactual is demonstrated, and the assertion is stable under load.

中文说明

维护者验证 —— 在本地 Linux 上真实构建并运行

我在干净的 worktree 中重建了这个改动,把 PR 描述里的每一条断言都端到端跑了一遍;另外补上了纯测试改动必须自证的两点:新断言确实杀掉它所指名的变异体,而改动前的用例并没有。两点都成立。建议合并。

环境 —— Debian 13 (trixie)、内核 6.12.63、16 vCPU、Node v22.22.2、npm 10.9.7、vitest 3.2.7。worktree 位于 PR head 2292189e8f,merge-base 为 f747999539。这也补上了 PR 描述中标为「未测试」的 🐧 Linux 一行。


1. PR 描述里的数字精确复现

在 PR head 上执行 npx vitest run src/config/config.test.ts621 passed (621),与 PR 声称的数字一致。packages/core 的 prettier、ESLint、tsc --noEmit 全部干净。

2. 变异见证成立,且反事实对照证明它是新增覆盖

Config.initialize() 的 join 分支移除 await this.initializationPromise;(即 §3 的 M3),只跑那条具名用例:

使用 PR 的测试文件 使用改动前的测试文件
makes a concurrent caller join the in-flight initialization FAIL —— AssertionError: expected [ 'second' ] to deeply equal [] pass

右边那一列才是承重的一半:在 f747999539 上,该变异体从这条为其行为命名的用例下存活,因此这条断言确实是新增覆盖,而不是既有覆盖的重述。失败文本与 PR 描述逐字一致。

mutation A/B

在 M3 下跑整个文件也印证了 §3 的另一条判断:改动前的文件里只有一条用例变红 —— shares a failed in-flight initialization with concurrent callers,且失败于 expected undefined to be Error: startup discovery exploded,也就是错误对象同一性,而非顺序性。在 PR head 上两条都会红。所以文件在 M3 下从来不是绿的;变化在于失败现在落到描述该性质的那条用例上,并且报错信息说清了真正坏掉的是什么。

3. 这条断言的覆盖能力不止于它所针对的那一个变异体

我用同一条命令又跑了四个「加入了这次 flight 但不等待它」的变异体,四个全部是新增击杀 —— 断言钉住的是顺序,而不是某个具体 token:

join 分支变异体 PR 测试 改动前测试 新增击杀
M3 —— 去掉 await FAIL pass
await Promise.resolve() 后 return FAIL pass
await new Promise(r => setTimeout(r, 0)) 后 return FAIL pass
await Promise.resolve(undefined) 后 return FAIL pass
直接 throw 而不 join FAIL FAIL 原本已覆盖
去掉 signal.throwIfAborted() pass pass 不适用 —— 由相邻的 abort 用例杀掉(已验证)

mutant matrix

「macrotask」那一行尤其说明问题:该变异体自己的 setTimeout(0) 注册早于测试里的那个,所以这不是微任务深度带来的巧合 —— 断言依然能看到加入方提前落定。

4. 确定性结论在 CPU 饥饿下依然成立

setTimeout(0) 挂起是这条用例在高负载 runner 上唯一可能变 flaky 的地方,所以我选择实测而非推理:40/40 全绿 —— 25 次常规运行,外加 15 次固定在单核(taskset -c 0)、并有 4 个忙循环进程争抢同一核心的运行。零失败、零重试。

5. 这条被钉住的性质究竟买到了什么 —— 在真实构建产物上

为了确认后果确实如 PR 所述,我按 packages/cli/src/ui/opentui/live-session.ts:562-568 的调用形状(try { await config.initialize(); } catch {}getGeminiClient()getChat())驱动 packages/core/dist,并让第一次 flight 在初始化末尾才启动 chat —— 这正是真实顺序。两次运行之间只有 join 分支不同:

#11002 mechanism

线上代码:加入方等待 180 ms,拿到已启动的 chat。去掉 await 后:它在 0 ms 返回,真实的 LlmClient.getChat() 抛出 Chat not initialized —— 也就是 #11002,而这样的改动在改动前的测试套件里能从这条用例下溜过去。


两点非阻塞说明

两点都不影响绿色路径,且落在同一段代码里,所以顺带提一下。

N1 —— settled 记录的是 fulfilled,而不是 settled。 first.then(cb) 只在 resolve 时触发,因此一个被 reject 的加入方虽然已经落定却不会被记录,expect(settled).toEqual([]) 会空过。今天就能观察到:在「以 throw 取代 join」的变异体下,运行失败于后面 await Promise.all(...) 抛出的 Error: join disabled,而从来不是 AssertionError —— 也就是说加入方已经落定,第 5164 行却通过了。

N2 —— 那两个游离的 .then() promise 引入了一条 unhandled rejection 通道。 packages/core/vitest.config.ts:44 设置了 dangerouslyIgnoreUnhandledErrors: process.platform !== 'linux',因此在 ubuntu 这条腿上,即便所有用例都通过,一个未处理的 rejection 也会让整次运行变红。我直接探测过:一个自身通过、但留下一个游离 rejected promise 的临时用例,输出是 Tests 1 passed (1) / Errors 1 error / 退出码 1。在 throw 变异体下,PR head 会在测试失败之外额外报 Unhandled Rejection,而改动前的文件不会。目前它只可能在本就已经变红的运行里触发 —— 属于噪音而非假失败 —— 但它指向的是生产代码,而不是那条断言。

两点都能用双参数的 .then 形式一并解决。我在本地应用后重跑了全部验证:

const record = (who: string) => () => {
  settled.push(who);
};
first.then(record('first'), record('first'));
second.then(record('second'), record('second'));

621 passed、prettier 干净、M3 依旧死于同一条 AssertionError;并且 throw 变异体现在死于那条断言本身expected [ 'second' ] to deeply equal [])而不是 Promise.all,unhandled rejection 数量为 0。纯属可选项,PR 现有写法本身是正确的。


状态说明

我写这份报告时 Test (ubuntu-latest, Node 22.x) 仍处于 in_progress(已在 Run tests and generate reports 步骤里跑了约 1 小时 20 分);它与 REVIEW_REQUIRED 一起构成了当前的 BLOCKED 状态。我的本地运行没有任何迹象表明它有问题 —— 本 PR 所触及的文件是通过的,包括在资源争抢下。

结论:批准 / 可合并。 论断得到了演示,反事实对照得到了演示,且该断言在负载下稳定。

@wenshao

wenshao commented Sep 5, 2026

Copy link
Copy Markdown
Collaborator

Follow-up: the red Test (ubuntu-latest, Node 22.x) is the known shared-runner flake, not this PR

Attempt 1 (job 101315879912, runner ecs-qwen-hk3-24) failed — I pulled the log. Not one test failed anywhere in the job. Every vitest summary line in all 21 workspaces reads passed; the job exited 1 purely on unhandled errors:

workspace result why the run still went red
@qwen-code/qwen-code-core 643 passed | 1 skipped (644) files, 23523 passed | 10 skipped tests Errors 1 error
@qwen-code/qwen-code (cli) 1014 passed (1014) files, 28638 passed | 90 skipped tests Errors 3 errors

All four errors are the same one:

Error: [vitest-worker]: Timeout calling "onTaskUpdate"
 ❯ Object.onTimeoutError ../../node_modules/vitest/dist/chunks/rpc.-pEldfrD.js:53:10

That is worker↔main RPC starvation, and the runner was saturated when it happened — the job's own DFSAMPLE lines show host load ranging 166 → 302 with 46–177 concurrent test jobs on the same box. This is exactly #10490 ("Test (ubuntu) fails non-deterministically on shared runners"), which names this signature and this mechanism: the ubuntu lane is the one lane that keeps the unhandled-error signal (packages/core/vitest.config.ts:44, dangerouslyIgnoreUnhandledErrors: process.platform !== 'linux'), so an infra hiccup with zero failing assertions still reddens the whole job.

The file this PR touches passed in that very job:

✓ src/config/config.test.ts (621 tests) 13409ms

621 — the same count as the PR body and the same count as my local Linux run. Nothing in the failure touches packages/core/src/config.

Attempt 2 is already running (started 18:00 UTC on ecs-qwen-hk4-22). No action needed from the author; my recommend merge stands unchanged.

中文说明

补充:变红的 Test (ubuntu-latest, Node 22.x) 是已知的共享 runner flake,与本 PR 无关

第 1 次尝试(job 101315879912,runner ecs-qwen-hk3-24)失败了,我把日志拉下来看了。整个 job 里没有任何一条用例失败。 全部 21 个 workspace 的 vitest 汇总行都是 passed;job 退出码 1 完全来自未处理错误:

workspace 结果 为何仍然变红
@qwen-code/qwen-code-core 文件 643 passed | 1 skipped (644),用例 23523 passed | 10 skipped Errors 1 error
@qwen-code/qwen-code(cli) 文件 1014 passed (1014),用例 28638 passed | 90 skipped Errors 3 errors

四个错误是同一个:

Error: [vitest-worker]: Timeout calling "onTaskUpdate"
 ❯ Object.onTimeoutError ../../node_modules/vitest/dist/chunks/rpc.-pEldfrD.js:53:10

这是 worker 与主进程之间的 RPC 饥饿,而事发时 runner 已经饱和 —— job 自己打的 DFSAMPLE 行显示宿主 load 在 166 → 302 之间,同一台机器上并发着 46–177 个测试任务。这正是 #10490「Test (ubuntu) 在共享 runner 上非确定性失败」)所描述的签名与机制:ubuntu 这条腿是唯一保留 unhandled-error 信号的腿(packages/core/vitest.config.ts:44dangerouslyIgnoreUnhandledErrors: process.platform !== 'linux'),因此一次零断言失败的基础设施抖动也会让整个 job 变红。

本 PR 所触及的文件在那次 job 里是通过的:

✓ src/config/config.test.ts (621 tests) 13409ms

621 —— 与 PR 描述、以及我本地 Linux 运行的数字完全一致。这次失败与 packages/core/src/config 毫无关系。

第 2 次尝试已在运行(18:00 UTC 起,runner ecs-qwen-hk4-22)。作者无需做任何处理;我此前的建议合并结论不变。

@wenshao
wenshao enabled auto-merge September 5, 2026 23:35

@qqqys qqqys left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

APPROVE (verified at head 2292189)

What the 15 lines do and why they hold

The addition pins the property the surrounding test is named for: while the first initialize() flight sits behind the gate, the joining second call must remain UNSETTLED. I traced both arms: with the join branch's await dropped (the mutant that reproduces #11002 — a joiner proceeding before initialization completes and dying on "Chat not initialized"), second resolves immediately, its .then runs as a microtask strictly before the setTimeout(0) macrotask flush, settled is non-empty, and the new assertion goes red; with correct code both promises are gated and the flush proves nothing settled — deterministic, no timing flake window. The pre-existing assertions (once-called, both-await) demonstrably survive that mutant, so this is exactly the missing pin, not a redundant one.

Gates

Test-only diff in packages/core; the review round at this head posted zero findings, the maintainer approved it, and CI shows 17 green with zero failures (the lone completed non-green is the fleet-side web-shell E2E Smoke cancellation the whole pool has been showing, which never touches this lane's code).

@wenshao
wenshao added this pull request to the merge queue Sep 5, 2026
Merged via the queue into QwenLM:main with commit 03c0909 Sep 5, 2026
138 of 140 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants