Skip to content

fix(cli): stop the TUI startup freeze when chat recording is disabled (#10293) #10303

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
qwen-code-dev-bot wants to merge 5 commits into from
+13 −3
Closed

fix(cli): stop the TUI startup freeze when chat recording is disabled (#10293) #10303

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5122b88
fix(cli): stop the TUI startup freeze when chat recording is disabled…
qwen-code-dev-bot Aug 27, 2026
f4abf53
Merge branch 'main' into autofix/issue-10293
qwen-code-dev-bot Aug 27, 2026
c3c2146
Merge branch 'main' into autofix/issue-10293
qwen-code-dev-bot Aug 27, 2026
56c30a4
Merge branch 'main' into autofix/issue-10293
qwen-code-dev-bot Aug 28, 2026
9e58d09
Merge branch 'main' into autofix/issue-10293
qwen-code-dev-bot Aug 28, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions packages/cli/src/ui/utils/goal-runtime.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,9 +42,13 @@ describe('waitForGoalRuntime', () => {
throw new GoalPersistenceUnavailableError();
});

await expect(waitForGoalRuntime({ getGoalRuntimeReady })).resolves.toBe(
true,
);
await expect(
waitForGoalRuntime({ getGoalRuntimeReady }, { timeoutMs: 100 }),
).resolves.toBe(true);
expect(getGoalRuntimeReady).toHaveBeenCalledTimes(2);
});

it('does not hide synchronous readiness errors', async () => {
Expand All @@ -56,6 +60,9 @@ describe('waitForGoalRuntime', () => {
await expect(waitForGoalRuntime({ getGoalRuntimeReady })).rejects.toBe(
failure,
);
Comment on lines +56 to +62

@qwen-code-ci-bot qwen-code-ci-bot Aug 27, 2026

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.

[Suggestion] The new rethrow test pins error propagation only on the no-timeout path, but the production startup gate (AppContainer.tsx) always calls waitForGoalRuntime with a timeoutMs, i.e. through the Promise.race branch — and no test covers error propagation on that branch (the swallow test already asserts both). I mutation-verified the hole in an isolated scratch tree: changing the race entry to awaitReady().catch(() => 'timeout' as const) — which would turn a genuine Goal-state error (e.g. a malformed lifecycle record) at TUI startup into a silent false / "Goal features are degraded" outcome instead of surfacing it to the global error handler — leaves the whole file green (Tests 8 passed (8)), while adding the timeout-path assertion below kills the mutant (Tests 1 failed | 7 passed) and passes on the current code.

Suggested change
const getGoalRuntimeReady = vi.fn((): Promise<GoalRuntime> => {
throw failure;
});
await expect(waitForGoalRuntime({ getGoalRuntimeReady })).rejects.toBe(
failure,
);
const getGoalRuntimeReady = vi.fn((): Promise<GoalRuntime> => {
throw failure;
});
await expect(waitForGoalRuntime({ getGoalRuntimeReady })).rejects.toBe(
failure,
);
await expect(
waitForGoalRuntime({ getGoalRuntimeReady }, { timeoutMs: 100 }),
).rejects.toBe(failure);

The added assertion is its own fix witness: reintroduce the swallowing mutant on the race branch and it is the line that goes red — confirm the red after adding it.

中文说明

新增的重抛（rethrow）测试只在无超时路径上钉住了错误传播，但生产环境的启动门（AppContainer.tsx）总是带 timeoutMs 调用 waitForGoalRuntime，也就是走 Promise.race 分支 —— 而该分支的错误传播没有任何测试覆盖（吞异常（swallow）测试已经同时断言了两个分支）。我在隔离的 scratch tree 中做了变异验证：把 race 的一项改成 awaitReady().catch(() => 'timeout' as const) —— 这会把 TUI 启动时真实的 Goal 状态错误（例如损坏的生命周期记录）变成静默的 false / "Goal features are degraded"，而不是上抛给全局错误处理器 —— 整个测试文件仍然全绿（Tests 8 passed (8)）；而补上下方超时路径的断言即可杀死该变异体（Tests 1 failed | 7 passed），且在当前代码上通过。

新增的断言本身就是修复见证：在 race 分支重新引入吞异常的变异体，正是该断言会变红 —— 添加后请确认它确实变红。

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

await expect(
waitForGoalRuntime({ getGoalRuntimeReady }, { timeoutMs: 100 }),
).rejects.toBe(failure);
});

it('resolves true once the runtime settles within the timeout', async () => {
Expand Down
5 changes: 5 additions & 0 deletions packages/cli/src/ui/utils/goal-runtime.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,11 @@ export async function waitForGoalRuntime(
): Promise<boolean> {
const awaitReady = async (): Promise<void> => {
try {
// The call itself must stay inside the try: with chat recording
// disabled (--no-chat-recording, settings) getGoalRuntimeReady()
// THROWS synchronously instead of rejecting, and an escaped throw
Comment on lines +59 to +61

@qwen-code-ci-bot qwen-code-ci-bot Aug 28, 2026

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.

[Suggestion] The discipline this comment codifies is caller-side only, while the contract violation stays open at the producer: Config.getGoalRuntimeReady() (config.ts:8462-8468) is a non-async method whose first statement — this.getGoalRuntime() — throws GoalPersistenceUnavailableError synchronously when chat recording is disabled, so the Promise rejection channel never engages. That is despite the adjacent branch of the same method deliberately returning Promise.reject(new GoalPersistenceUnavailableError()) and the documented contract (goal-protocol.ts:223-224) saying the method rejects when persistence is unavailable; getGoalRuntimePrepared() leaks the same way.

This is not hypothetical: #10128 forgot exactly this discipline in this very function, the escaped throw froze the TUI on the init banner and red-lined the post-merge E2E lanes (#10293). Every future caller of either method must independently remember to keep the call itself inside a try/catch; the first one that doesn't — a new hook, command, or ACP path running with --no-chat-recording or general.chatRecording: false — turns the intended graceful degradation into an escaped throw and reproduces this incident. All 18 current call sites are wrapped, so nothing is broken today — the cost is the demonstrated repetition.

Consider normalising at the producer (here or in a follow-up, given core-config sensitivity) so the Promise contract holds for every caller and this comment's warning becomes moot:

getGoalRuntimeReady(): Promise<GoalRuntime> {
  try {
    const runtime = this.getGoalRuntime();
    // ...existing readiness check...
    return this.goalRuntimeReady.then(() => runtime);
  } catch (error) {
    return Promise.reject(error);
  }
}

Probe witness (isolated scratch tree, real new Config({ chatRecording: false })): the unmodified code prints PROBE ready: SYNC THROW GoalPersistenceUnavailableError (no Promise produced); with the try/catch normalisation applied, the same probe flips to PROBE ready: returned a Promise / promise REJECTED with GoalPersistenceUnavailableError, and all 10 existing Goal-related config tests still pass. This PR's caller-side change stays correct as defense in depth.

If the normalisation lands, the pinning test is the core-side counterpart of config.test.ts's 'does not expose volatile Goal state when chat recording is disabled': await expect(config.getGoalRuntimeReady()).rejects.toThrow(GoalPersistenceUnavailableError) with chatRecording: false — remove the normalisation and confirm that test goes red.

中文说明

这条注释所固化的约束只存在于调用方一侧，而契约违背在产生方仍然敞开：Config.getGoalRuntimeReady()（config.ts:8462-8468）是一个非 async 方法，其第一条语句 this.getGoalRuntime() 会在禁用聊天记录时同步抛出 GoalPersistenceUnavailableError，Promise 的 reject 通道根本没有机会生效——尽管同一方法的相邻分支刻意使用 return Promise.reject(new GoalPersistenceUnavailableError())，且文档契约（goal-protocol.ts:223-224）明确写明持久化不可用时该方法应当 reject；getGoalRuntimePrepared() 存在同样的泄漏。

这并非理论风险：#10128 正是在本函数中遗忘了这一约束，逃逸的抛出使 TUI 冻结在启动横幅上，并令合并后的 E2E 全线变红（#10293）。这两个方法的每一个未来调用方都必须各自记得把调用本身放进 try/catch；第一个忘记这样做的调用方——某个运行在 --no-chat-recording 或 general.chatRecording: false 下的新 hook、命令或 ACP 路径——会把预期的优雅降级变成逃逸的抛出，复现本次事故。当前全部 18 处调用点均已包裹，今天没有任何损坏——代价是这种已被证实会重演的重复劳动。

建议在生产方归一化（在本 PR 或后续跟进中，考虑到 core 配置的敏感性），使 Promise 契约对每个调用方都成立，这条注释的警告也就随之失去必要：

getGoalRuntimeReady(): Promise<GoalRuntime> {
  try {
    const runtime = this.getGoalRuntime();
    // ...existing readiness check...
    return this.goalRuntimeReady.then(() => runtime);
  } catch (error) {
    return Promise.reject(error);
  }
}

探针见证（隔离的 scratch tree，真实的 new Config({ chatRecording: false })）：未修改的代码输出 PROBE ready: SYNC THROW GoalPersistenceUnavailableError (no Promise produced)；应用 try/catch 归一化后，同一探针翻转为 PROBE ready: returned a Promise / promise REJECTED with GoalPersistenceUnavailableError，且现有 10 个 Goal 相关 config 测试全部通过。本 PR 调用方一侧的改动作为纵深防御仍然是正确的。

如果归一化落地，钉住它的测试是 config.test.ts 中 'does not expose volatile Goal state when chat recording is disabled' 的 core 侧对应版本：在 chatRecording: false 下 await expect(config.getGoalRuntimeReady()).rejects.toThrow(GoalPersistenceUnavailableError)——移除归一化后请确认该测试变红。

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

// rejects the AppContainer startup effect, freezing the TUI on the
// init banner forever.
await config.getGoalRuntimeReady();
} catch (error) {
if (!(error instanceof GoalPersistenceUnavailableError)) throw error;
Expand Down
4 changes: 1 addition & 3 deletions packages/core/src/agents/team/teamHelpers.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,9 +53,7 @@ vi.mock('../../config/storage.js', async (importOriginal) => {
// otherwise the real readFile runs.
vi.mock('node:fs/promises', async (importOriginal) => {
const actual = await importOriginal<typeof import('node:fs/promises')>();
type ReadFileHook = (
...args: Parameters<typeof actual.readFile>
) => unknown;
type ReadFileHook = (...args: Parameters<typeof actual.readFile>) => unknown;
let readFileHook: ReadFileHook | undefined;
return {
...actual,
Expand Down
Loading