fix(core): harden Config.initialize() join path - #11075
Conversation
Follow-ups to the non-blocking review suggestions left on #11037: - a joining caller's options are ignored (first caller wins), so honor an already-aborted signal on the join path instead of blocking on the foreign flight - log a debug breadcrumb when a caller joins an in-flight initialization - pin the post-failure-settle contract: a call after a failed first flight still throws 'Config was already initialized' - update the OpenTUI commands-dispatch comment to describe the join semantics instead of the removed throw-then-catch mechanism
|
|
|
Thanks for the PR! Template looks good ✓ — every required heading is filled in, Problem: observed and documented, not theoretical hardening. All four items are unresolved review threads on #11037 (merged earlier today), and each was independently confirmed by a human reviewer — including a concrete mutation witness for R1-3 ( Direction: aligned. This closes gaps on a change that already merged, on the same startup path behind #11002 ("Chat not initialized"). No new surface, no new abstraction, no product-direction question to litigate. CHANGELOG: no direct reference, and none needed — this is internal startup-path hardening, not a user-facing capability. Size: core paths are touched ( Approach: scope feels right — it maps 1:1 onto the four threads and carries nothing else. I went looking for a smaller version and there isn't one worth having: the Risk: no elevated risk signals — none of the changed files match the revert-correlated path list, so ordinary review depth. One scope boundary worth naming up front, as an observation rather than an objection: Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ —— 所有必填小节都写了， 问题：是已观测且有据可查的，不是理论性加固。这四项都是 #11037（今天早些时候已合并）上未解决的评审线程，而且每一条都经人工评审者独立确认过——包括 R1-3 的具体变异见证（ 方向：对齐。这是在补齐一个已经合并的改动的缺口，且正是 #11002（"Chat not initialized"）背后的同一条启动路径。没有新增接口面、没有新抽象，也不存在需要争论的产品方向问题。CHANGELOG：没有直接对应条目，也不需要——这是内部启动路径加固，不是面向用户的能力。 规模：触及核心路径（ 方案：范围合理——与四条线程一一对应，没有夹带别的东西。我尝试找一个更小的版本，但没有值得采用的： 风险：无升级风险信号——改动文件都不匹配「回退相关」路径清单，按常规深度评审即可。 有一点范围边界想先说明，属于观察而非反对： 进入代码审查 🔍 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
Code reviewI wrote down my own answer to the four threads before reading the diff, and the diff is what I would have written — same guard, same placement, same two test assertions. So this is a confirmation pass rather than a hunt for an alternative. What I actually checked, in the order it mattered: The debug breadcrumb cannot crash the path it illuminates. This was the one real hazard in the change: The guard mirrors the leader path instead of inventing semantics. Every downstream consumer, named. The callers that can actually reach the join branch today are all options-less — The R1-3 assertion genuinely kills the mutant. After a failed first flight the The new test is load-bearing, and correctly built. I also looked for the classic failure shape in that structure: The OpenTUI comment is comment-only (8 added / 8 removed) and now tells the truth. Conventions are clean: ESM, no No critical blockers and no AGENTS.md violations. Nothing I'd ask to change. Test evidenceUnattended CI run — I did not build or execute anything from this PR. The signal below is the PR's own CI, read from the API for the reviewed commit. Zero checks are red. The unit suite, lint, and integration jobs were still in flight when I fetched this once (no polling); the Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 Two of the green checks that matter here are the OpenTUI ones — Not verified: the author's "621 config tests and 58 commands-dispatch tests pass" is their own local macOS run, not something I re-ran — the Linux unit job above is what will independently confirm it, and it had not landed at fetch time. Sandboxed verification would settle the remaining gap: 中文说明代码审查在读 diff 之前，我先针对那四条线程写下了自己的答案，结果 diff 就是我会写的样子——同样的守卫、同样的位置、同样的两条测试断言。所以这一轮是确认，而不是去找替代方案。 我真正核查过的点，按重要性排列： debug 面包屑不会在它要照亮的那条路径上崩掉。 这是整个改动里唯一真实的隐患：这条日志写在 join 分支内部，而该分支按定义运行在初始化完成之前。 守卫是复刻主调用方路径，而不是自创语义。 逐个点名下游消费方。 今天真正能走到 join 分支的调用方全都不带 options—— R1-3 那条断言确实能杀死变异体。 第一次初始化失败后， 新测试是承重的，构造也正确。 我还找了这种结构常见的翻车形态： OpenTUI 那处是纯注释改动（增 8 / 删 8），而且现在说的是真话。 约定方面干净：ESM、无 没有关键阻塞项，也没有违反 AGENTS.md 的地方。我没有要求修改的内容。 测试证据无人值守的 CI 运行——我没有构建或执行本 PR 的任何代码。下面的信号来自 PR 自身的 CI，通过 API 针对被审查的 commit 读取。没有任何检查是红的。 抓取时（只抓一次，不轮询）单元测试、lint 与集成测试仍在运行中； 这里真正相关的绿色检查中有两个是 OpenTUI 相关的—— 未验证：作者所称的「621 个 config 测试与 58 个 commands-dispatch 测试通过」是其本人在 macOS 上的本地运行结果，我没有重跑——上面那个 Linux 单元测试任务才是独立确认它的依据，而抓取时它尚未落定。 沙箱验证可以补齐剩余缺口： — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
|
Confidence: 5/5 — every stage came back clean, and the one hazard in the change (logging from a branch that runs before initialization finishes) turned out to be constructor-safe. Stepping back: this is what closing review debt properly looks like. Four threads were left unresolved on #11037 when it merged; a human reviewer had already confirmed each one and even handed over a mutation witness and a suggested one-liner. This PR takes exactly that scope, adds three production statements and two documentation corrections, and pins all of it with assertions that fail if you undo the fix. I wrote my own answer to those four threads before opening the diff and arrived at the same change, which is the strongest signal I have that there was no simpler path being missed. The part I'd want a maintainer to notice is why the small bits earn their place. The docstring line saying a joiner's options are ignored converts an implicit contract into a written one — that's the thing R1-1 was really about, since the silent-drop is only dangerous while it's undocumented. And the OpenTUI comment was describing a throw-then-catch mechanism that no longer exists; a maintainer debugging startup ordering would have reasoned from a fiction. Neither is cosmetic. What gives me pause is small and I don't think it belongs in this PR. On the "am I being worn down" question: single commit, single PR, follow-up on the author's own merged change, no volume pattern. And I verified the problem exists by reading the four threads rather than accepting the PR's framing — all four are still unresolved on a merged PR. Verdict: approve, deferred. The unit suite, lint, and integration jobs were still running on this commit when I reviewed, and approving now would attest to a result that doesn't exist yet. Approval is deferred until CI lands green on 中文说明Confidence: 5/5 —— 各阶段都干净，而改动里唯一的隐患（在初始化完成之前运行的分支里打日志）经核查是构造函数级别安全的。 退一步看：这才是正确清偿评审欠债的样子。#11037 合并时留下四条未解决线程；人工评审者已逐条确认，甚至给出了变异见证和建议的一行修复。本 PR 精确地取了这个范围，新增三条生产语句和两处文档修正，并用「撤销修复即失败」的断言把全部改动钉住。我在打开 diff 之前先自己给出了那四条线程的答案，得到的正是同一个改动——这是我能拿到的最强信号，说明没有被忽略的更简路径。 我希望 maintainer 注意的一点是：那些小改动凭什么占位置。文档注释里「加入方的 options 会被忽略」这句话，把一个隐式契约变成了写下来的契约——这才是 R1-1 真正的要害，因为静默丢弃只有在没被写进文档时才是危险的。而 OpenTUI 那处注释描述的是一个已不存在的「抛错后 catch 继续」机制；排查启动顺序的 maintainer 会基于一个虚构的机制去推理。两者都不是表面功夫。 让我略有保留的地方很小，而且我认为不属于本 PR。 关于「我是不是被磨软了」这个问题：单个 commit、单个 PR、作者对自己已合并改动的后续，不存在批量投稿的模式。而我是通过实际阅读那四条线程来确认问题存在的，不是接受 PR 的叙述——四条在一个已合并的 PR 上仍未解决。 结论：批准，但延后。 我审查时该 commit 上的单元测试、lint 与集成测试仍在运行，此刻批准等于为一个尚不存在的结果背书。批准延后至 CI 在 — 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. Suggestions are inline.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) never ran at this commit and its suite did not run locally.
Not reviewed: reverse audit — stopped before round 4 by the review time budget.
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
未审查（原文为英文）：build-and-test — Integration Tests (CLI, No Sandbox) never ran at this commit and its suite did not run locally.
未审查：反向审计——评审时间预算不足，未能开始第 4 轮。
— qwen3.8-max via Qwen Code /review (v0.23.0)
| const joining = config.initialize({ signal: controller.signal }); | ||
| release(); |
There was a problem hiding this comment.
[Suggestion] R1-1: The new 'rejects a joining caller whose signal is already aborted' test pins that the joining caller rejects with the abort reason, but not that it rejects fast — release() runs before any assertion on joining, so the fail-before-joining ordering is never checked. If options?.signal?.throwIfAborted() is later moved below await this.initializationPromise, the joiner blocks on the full foreign flight before rejecting with the identical abort reason — reintroducing exactly the "aborted-signal joiner hangs on the first flight" behaviour this PR removes — yet this test still passes green, because release() has already let the first flight settle.
Witness:
Probe matrix at PR HEAD (scratch tree):
original guard position + shipped test → PASS
mutant (guard moved below the await) + shipped test → PASS ← mutant survives
mutant + rejection asserted before release() → FAIL (Test timed out in 60000ms)
original guard + rejection asserted before release() → PASS
Assert the rejection while the gate is still held — move await expect(joining).rejects.toBe(abortReason); above release();:
const joining = config.initialize({ signal: controller.signal });
await expect(joining).rejects.toBe(abortReason);
release();
await expect(first).resolves.toBeUndefined();release() must stay in the test so the existing await expect(first).resolves.toBeUndefined(); still holds — dropping it instead of reordering the assertion would hang the test. If you apply this reorder, please prove it kills the mutant: move (or delete) the throwIfAborted() guard below the await and confirm this test then goes red — with the gate still held, the pre-release() rejection assertion never settles and the test times out.
中文说明
新增的 'rejects a joining caller whose signal is already aborted' 测试只钉住了「加入方会以中止原因 reject」，没有钉住「它 reject 得快」——release() 先于对 joining 的任何断言执行，因此「先失败、后加入」的顺序属性从未被检查。如果日后有人把 options?.signal?.throwIfAborted() 移到 await this.initializationPromise 之后，加入方会先阻塞在别人的完整初始化上、再以同一个 abortReason reject——恰好重新引入本 PR 要消除的「已中止信号的加入方挂在第一次初始化上」行为——而这条测试仍然通过，因为在断言 rejection 之前 release() 已让第一次初始化落定。
见证（PR HEAD 上的探针矩阵，独立 scratch 树）：原守卫位置 + 现有测试 → 通过；变异体（守卫移到 await 之后）+ 现有测试 → 通过（变异体存活）；变异体 + 在 release() 之前断言 rejection → 失败（60 秒超时）；原守卫 + 在 release() 之前断言 rejection → 通过。
修复：在门控仍然持有时断言 rejection——把 await expect(joining).rejects.toBe(abortReason); 移到 release(); 之前：
const joining = config.initialize({ signal: controller.signal });
await expect(joining).rejects.toBe(abortReason);
release();
await expect(first).resolves.toBeUndefined();前提约束：release() 必须保留，现有的 await expect(first).resolves.toBeUndefined(); 依赖它——只删 release() 而不是调整断言顺序会让测试挂起。修复见证：应用该重排后，请证明它能杀死变异体——把 throwIfAborted() 守卫移到 await 之后（或删除），确认该测试变红：门控仍被持有时，release() 之前的 rejection 断言永远不会落定，测试超时。
— qwen3.8-max via Qwen Code /review (v0.23.0)
There was a problem hiding this comment.
Applied in 82d82c7 — the reorder is exactly as suggested:
const joining = config.initialize({ signal: controller.signal });
await expect(joining).rejects.toBe(abortReason);
release();
await expect(first).resolves.toBeUndefined();release() stays, so await expect(first).resolves.toBeUndefined() still holds.
On the requested mutation witness: I did not run the mutant probe, so I am not claiming a reproduced red. The argument the reorder rests on is structural — options?.signal?.throwIfAborted() sits ahead of every await in initialize() (config.ts:3013, before await this.initializationPromise), so the joining promise settles on the first microtask and the pre-release() assertion resolves without the first flight finishing. Move the guard below that await and the assertion has nothing to settle it while the gate is held, which is the 60s timeout your matrix recorded. CI on this head is the check that the shipped ordering is green.
The comment above the assertions now says why the order matters, so the next reader does not "tidy" it back.
中文说明
已在 82d82c7 中按建议重排，release() 保留，原有的 await expect(first).resolves.toBeUndefined() 仍然成立。
关于要求的变异见证：我没有跑变异体探针，因此不声称复现了红。重排所依赖的是结构性理由——options?.signal?.throwIfAborted() 位于 initialize() 中所有 await 之前（config.ts:3013，在 await this.initializationPromise 之前），所以加入方的 promise 在第一个 microtask 就落定，release() 之前的断言无需等第一次初始化结束即可完成。把守卫移到该 await 之后，门控仍被持有时该断言就没有任何东西能让它落定，正是你矩阵里记录的 60 秒超时。当前 head 的 CI 负责验证重排后的测试是绿的。
断言上方的注释已写明顺序为什么重要，避免后来者把它「整理」回去。
The test asserted the abort reason but not the ordering: `release()` ran before the assertion on `joining`, so a `throwIfAborted()` moved below `await this.initializationPromise` would still reject with the identical reason and keep the test green — the exact "aborted joiner blocks on the foreign flight" behaviour this branch removes. Assert the rejection while the gate is still held. The guard sits ahead of any `await` in `initialize()`, so the joining promise settles on the first microtask without the first flight finishing; with the guard moved after the await, the assertion never settles and the test times out instead.
chiga0
left a comment
chiga0
left a comment
There was a problem hiding this comment.
No blocking findings.
Approval blockers: none.
Scope: packages/core/src/config/config.ts (initialize join path, lines 3013–3022), packages/core/src/config/config.test.ts (two test cases), packages/cli/src/ui/opentui/commands-dispatch.ts (comment-only).
What I checked:
Config.initialize()join path (config.ts:3011–3023):throwIfAborted()is correctly placed before the firstawaitin the join branch — the fast-fail fires synchronously on the joining caller's promise, before any yield to the microtask queue, so the "already-aborted signal blocks on the foreign flight" regression is structurally impossible with the guard in this position.ConfigInitializeOptionsinterface (config.ts:1563–1578):signalis typedAbortSignal | undefined;throwIfAborted()is a standardAbortSignalmethod. The?.chain is correct for the optional case.- Post-failure path: after a failed first flight,
initializationSettledistrue(set in thefinallyblock at line 3036), so a subsequent caller reachesthrow Error('Config was already initialized')instead of re-joining the stale rejected promise. The new assertion atconfig.test.ts:5212–5216pins this. - New test
'rejects a joining caller whose signal is already aborted'(config.test.ts:5160–5189): at the current HEAD (f7e45dc),await expect(joining).rejects.toBe(abortReason)appears beforerelease(). This ordering property is load-bearing — it proves the guard fires beforeawait initializationPromiserather than after — and the comment above the assertions (config.test.ts:5183–5185) documents why the order must not be changed. - OpenTUI comment (
commands-dispatch.ts:342–351): comment-only; accurately describes the new join semantics (no longer references the removed throw-then-catch mechanism). - Cross-check vs. prior review (qwen-code-ci-bot, round 1 on 689bcc2): R1-1 (test assertion ordering) is confirmed fixed at the current head. The author applied the exact reorder suggested.
Not covered: no local test run available; the PR description notes commit 82d82c7 was not re-run locally. The structural argument for assertion ordering correctness is sound (guard is before the first await), and the fix is a reorder of two lines with no logic change. CI on this head is the check the author identified.
Reviewed with AI assistance.
qwen-code-dev-bot
left a comment
qwen-code-dev-bot
left a comment
There was a problem hiding this comment.
Approved at head f7e45dc5.
History. The single open thread (Suggestion R1-1, about the test not pinning how fast the joiner rejects) is addressed on this head: 82d82c7 is an ancestor of f7e45dc5 and the rejection is now asserted while the gate is still held. I reproduced the reviewer's mutant rather than taking that on faith — on a scratch tree I moved options?.signal?.throwIfAborted() below await this.initializationPromise and ran the test: it fails with Test timed out in 8000ms. With the guard where it is now, the same test passes, as does the new later call must throw assertion added to shares a failed in-flight initialization with concurrent callers. So the test pins the ordering property, not just the rejection value.
Independent check. The only behavior change is that a joiner whose signal is already aborted rejects with its own reason instead of waiting out somebody else's flight — which is the point, since a joining caller's options cannot be honored anyway, and the docblock now says so plainly ("the first caller's options win"). Non-aborted joiners, the settled path, and the initialized / shutdownRequested / derived-config throws are untouched; the commands-dispatch.ts hunk is comment-only and its rewritten wording matches the join semantics that landed in #11037. No new Critical found.
CI facts. Lint & Static, Test (ubuntu-latest) and Integration Tests (no-AK) were still queued on this head when I submitted, with the five reported checks green and nothing red — stating it rather than implying a full-green lane.
qqqys
left a comment
qqqys
left a comment
There was a problem hiding this comment.
Reviewed at head f7e45dc5 (the merge-from-main delta touches nothing this PR owns). No historical blocking issues exist on this PR — the single round-1 Suggestion (the aborted-joiner test not pinning rejection speed) was resolved in 82d82c7 with the assertion moved before the gate release — and my independent Critical-only pass finds none.
The change, verified at this head:
config.tsjoin branch:options?.signal?.throwIfAborted()now runs before theawait this.initializationPromise, so a joining caller holding an already-aborted signal fails fast with its own abort reason instead of blocking on a foreign flight whose options it can never have honored; the first-flight path keeps its own pre-set guard, so non-joining callers are unaffected. The addeddebugLogger.debugbreadcrumb is constructor-safe (the logger exists before initialization completes), and the docstring now states the joiner-options contract ("the first caller's options win") that the #11037 round had asked to make explicit.config.test.ts: the new aborted-joiner test asserts the rejection while the gate is still held — pinning fail-before-joining placement, not just the rejection value (the post-await-guard mutant that motivated the reorder cannot pass it); and the failed-flight test now additionally asserts that a call after a failed-and-settled first flight throws "Config was already initialized" rather than re-joining the stale rejection — closing the last unpinned half of the settle contract from the previous PR's round.commands-dispatch.ts: comment-only — the startup-window self-heal rationale is updated to the join semantics that actually shipped (only a failed first flight can still land the loader in the partial-commands catch); no code change.
CI at this head: no failing or cancelled checks at review time; Test (ubuntu-latest, Node 22.x) and review-pr are still pending, which does not gate this review per policy.
…utant fails (QwenLM#11113) 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).
|
Released in v0.23.1. |
5 participants
What this PR does
Follow-up to #11037 that resolves the four non-blocking review suggestions left on the merged join-path change.
Config.initialize()'s in-flight join branch now fails fast when a joining caller's signal is already aborted (instead of blocking on the foreign flight) and logs a debug breadcrumb. The docstring documents that a joining caller's options are ignored — the first caller's options win. A regression assertion pins the post-failure-settle contract, and the OpenTUI commands-dispatch comment is updated to describe the join semantics instead of the removed throw-then-catch mechanism.Why it's needed
The four suggestions were latent gaps the original PR merged without resolving: an aborted-signal joiner would hang on the first flight, the join path had no observability (the same gap that made #11002 hard to diagnose), the post-failure-settle contract was unpinned (a one-token mutation survives the added tests), and a stale comment still described a mechanism the PR removed.
Reviewer Test Plan
How to verify
Run the config suite:
cd packages/core && npx vitest run src/config/config.test.ts. The new case proves a joining caller with an already-aborted signal rejects with the abort reason before the first flight settles — the assertion runs while the gate is still held, so it also pins the ordering; the updated failure case proves a call after a failed first flight still throwsConfig was already initialized.The first revision (689bcc2) was verified locally: all 621 config tests and 58 commands-dispatch tests passed, typecheck, lint and prettier clean. The follow-up commit 82d82c7 reorders two assertions and was not re-run locally — CI on this head is the check.
Evidence (Before & After)
N/A (non-user-visible behavior hardening plus a comment; the unit tests are the evidence).
Tested on
Risk & Scope
Linked Issues
Follow-up to #11037
中文说明
这个 PR 做了什么
#11037 的后续，解决合并后的 join 路径改动上遗留的四条非阻塞评审建议。
Config.initialize()的飞行途中 join 分支现在会在加入方信号已中止时快速失败（而不是阻塞在别人的初始化上），并输出一条 debug 面包屑。文档注释写明加入方传入的 options 会被忽略——以第一个调用方的 options 为准。新增一条回归断言钉住「失败落定后再调用」的契约，并把 OpenTUI 的 commands-dispatch 注释更新为 join 语义，替换掉已移除的「抛错后 catch 继续」机制。为什么需要它
这四条建议都是原 PR 合并时未解决的潜在缺口：已中止信号的加入方会挂在第一次初始化上；join 路径没有任何可观测性（正是 #11002 当初难以定位的原因）；「失败落定后再调用」的契约没有被钉住（单 token 变异能逃过现有测试）；以及一条过时注释仍在描述本 PR 已移除的机制。
审阅者测试计划
如何验证
运行 config 套件：
cd packages/core && npx vitest run src/config/config.test.ts。新用例证明携带已中止信号的加入方会在第一次初始化落定之前以中止原因 reject——断言在门控仍被持有时执行，因此顺序属性也被钉住；更新后的失败用例证明第一次初始化失败后再次调用仍抛出Config was already initialized。第一版（689bcc2）经过本地验证：621 个 config 测试与 58 个 commands-dispatch 测试全部通过，typecheck、lint、prettier 均干净。后续提交 82d82c7 只调整了两条断言的顺序，未在本地重跑——由当前 head 的 CI 负责验证。
证据（改动前与改动后）
N/A（非用户可见的行为加固 + 注释；单元测试即证据）。
测试环境
风险与范围
关联 Issue
#11037 的后续