Skip to content

fix(cli): swallow synchronous goal-persistence throws at the startup gate (#10311) #10317

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 2 commits into from
Closed

fix(cli): swallow synchronous goal-persistence throws at the startup gate (#10311) #10317

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
274d0d0
fix(cli): swallow synchronous goal-persistence throws at the startup …
qwen-code-dev-bot Aug 27, 2026
4040e4e
fix(cli): consolidate startup-gate sync-throw coverage with main (#10…
qwen-code-dev-bot Aug 27, 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
34 changes: 23 additions & 11 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 @@ -28,31 +28,43 @@ describe('waitForGoalRuntime', () => {
expect(getGoalRuntimeReady).toHaveBeenCalledTimes(1);
});

it('does not hide malformed or unsupported persisted Goal state', async () => {
const failure = new Error('unsupported Goal lifecycle record');
const getGoalRuntimeReady = vi.fn().mockRejectedValue(failure);

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

it('allows Goal-less sessions when readiness throws synchronously', async () => {
it('treats a synchronous persistence-unavailable throw as settled', async () => {
// Config.getGoalRuntimeReady() THROWS synchronously (rather than
// rejecting) when chat recording is disabled, e.g. --no-chat-recording.
// The startup gate must swallow that throw exactly like the rejected
// promise above; an escaped rejection kills the AppContainer init
// effect and freezes the TUI on the init banner forever (#10311).
const getGoalRuntimeReady = vi.fn((): Promise<GoalRuntime> => {
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 () => {
it('does not hide a synchronous throw of any other error', async () => {

@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] This test pins the "other synchronous throws must escape" half only on the unbounded branch; the timeout/Promise.race branch — the one the AppContainer startup gate actually uses — has no assertion that the rethrown error propagates, while the sibling swallow test above pins both branches. A future change hardening the race path against freezes (e.g. wrapping Promise.race(...) in .catch(() => 'timeout')) would pass the entire existing suite while silently converting genuinely fatal startup errors into AppContainer's degraded-warning path — the exact gate this PR repairs. Probe-verified at the reviewed commit: with that mutant in place, a probe asserting rejection on the race branch flips from pass to fail while all 8 existing tests stay green (Tests 2 failed | 8 passed). Mirror the first new test's double assertion by adding the timed leg before the test's closing brace:

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

Fix witness: the added assertion itself — removing rejection propagation from the race branch makes it fail while today's suite stays green.

中文说明

该测试只在无超时分支上钉住了"其他同步抛出必须逃逸"这一半；超时/Promise.race 分支 —— 也就是 AppContainer 启动门禁实际使用的分支 —— 没有断言被重新抛出的错误会继续传播，而上方的吞异常兄弟测试在两个分支上都做了钉住。未来若有人加固 race 路径以防卡死（例如把 Promise.race(...) 包进 .catch(() => 'timeout')），整个现有测试套件会全部通过，同时真正的致命启动错误会被悄悄转换成 AppContainer 的降级告警路径 —— 恰恰是本 PR 修复的那个门禁。已在被审提交上通过探针验证：放入该变异体后，断言 race 分支应当拒绝的探针由通过变为失败，而现有 8 个测试全部保持绿色（Tests 2 failed | 8 passed）。请参照第一个新增测试的双重断言，在该测试的结束花括号之前补上有超时的分支：

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

修复见证：新增的这条断言本身就是见证 —— 移除 race 分支上的拒绝传播会使它失败，而当前套件保持全绿。

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

const failure = new Error('unsupported Goal lifecycle record');
const getGoalRuntimeReady = vi.fn((): Promise<GoalRuntime> => {
throw failure;
});

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

it('does not hide malformed or unsupported persisted Goal state', async () => {
const failure = new Error('unsupported Goal lifecycle record');
const getGoalRuntimeReady = vi.fn().mockRejectedValue(failure);

await expect(waitForGoalRuntime({ getGoalRuntimeReady })).rejects.toBe(
failure,
);
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 must stay inside the try: with chat recording disabled
// (--no-chat-recording, settings) getGoalRuntimeReady() THROWS
// synchronously instead of rejecting, and an escaped rejection kills
// the AppContainer init effect, freezing the TUI on the init banner
// forever.
await config.getGoalRuntimeReady();

@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 gate fix compensates downstream for a producer contract violation: Config.getGoalRuntimeReady() (packages/core/src/config/config.ts:8462) throws GoalPersistenceUnavailableError synchronously when chat recording is disabled, while its documented contract (packages/core/src/goals/goal-protocol.ts:224) says it rejects. The gate was the only one of ~18 production call sites invoking it outside a try; every other site already catches the sync throw, and getGoalRuntimePrepared() carries the identical sync-throw shape. The trap that produced issue 10311 therefore stays live: any future refactor that calls getGoalRuntimeReady() outside a try — exactly what the timeout refactor did when it hoisted this call above awaitReady — reintroduces the escaped throw under --no-chat-recording and freezes the TUI on the init banner again. Worth a maintainer follow-up in core (out of scope for this PR — the minimal regression fix is right as is): make getGoalRuntimeReady() and getGoalRuntimePrepared() async so the synchronous throw becomes a rejection matching the documented contract; this PR's gate fix and tests then stay as defense-in-depth, and the "must stay inside the try" comment stops being load-bearing. Fix witness: a packages/core/src/config/config.test.ts case constructing Config with chat recording disabled and asserting await expect(config.getGoalRuntimeReady()).rejects.toBeInstanceOf(GoalPersistenceUnavailableError) without a try — removing the async normalization restores the sync throw and makes the test fail.

中文说明

门禁修复是在下游补偿一个生产端契约违背：聊天记录被禁用时，Config.getGoalRuntimeReady()（packages/core/src/config/config.ts:8462）会同步抛出 GoalPersistenceUnavailableError，而其文档契约（packages/core/src/goals/goal-protocol.ts:224）写的是"拒绝（reject）"。在约 18 个生产调用点中，门禁是唯一在 try 之外调用它的；其余调用点都已经捕获了这个同步抛出，而 getGoalRuntimePrepared() 带有完全相同的同步抛出形态。因此，产生 issue 10311 的陷阱仍然活着：未来任何在 try 之外调用 getGoalRuntimeReady() 的重构 —— 正如超时重构当年把这个调用提出 awaitReady 那样 —— 都会在 --no-chat-recording 下重新引入逃逸的抛出，让 TUI 再次冻结在初始化横幅上。建议作为核心模块的维护者后续跟进（不在本 PR 范围内 —— 作为最小回归修复，本 PR 的处理是正确的）：把 getGoalRuntimeReady() 和 getGoalRuntimePrepared() 改为 async，让同步抛出变成符合文档契约的拒绝；本 PR 的门禁修复与测试则作为纵深防御保留，届时"必须留在 try 内"的注释也不再承担关键职责。修复见证：在 packages/core/src/config/config.test.ts 中构造禁用聊天记录的 Config，不用 try 包裹，断言 await expect(config.getGoalRuntimeReady()).rejects.toBeInstanceOf(GoalPersistenceUnavailableError) —— 移除 async 归一化会恢复同步抛出，使该测试失败。

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

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

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Verified real against the code: Config.getGoalRuntimeReady() (packages/core/src/config/config.ts:8462) reaches a synchronous throw new GoalPersistenceUnavailableError() via getGoalRuntime() when chat recording is disabled, while the documented contract (packages/core/src/goals/goal-protocol.ts:224) says it rejects — and getGoalRuntimePrepared() carries the same shape. The fix (making both methods async so the sync throw becomes a rejection) lives in packages/core, outside this PR's footprint, and the finding itself marks it a maintainer follow-up — so it is deferred to the follow-up queue rather than implemented here. This PR's gate fix, the "must stay inside the try" comment, and the tests remain as defense-in-depth until that core change lands.

中文说明

已对照代码核实为真实问题：聊天记录被禁用时，Config.getGoalRuntimeReady()（packages/core/src/config/config.ts:8462）会经由 getGoalRuntime() 同步 throw new GoalPersistenceUnavailableError，而文档契约（packages/core/src/goals/goal-protocol.ts:224）写的是拒绝（reject）—— getGoalRuntimePrepared() 也带有同样的形态。修复方案（把两个方法改为 async，让同步抛出变成拒绝）位于 packages/core，超出本 PR 的足迹，且该意见本身也标注为维护者后续跟进事项 —— 因此将其延后到后续队列，而不在本 PR 实现。在 core 改动落地之前，本 PR 的门禁修复、“必须留在 try 内”注释与测试作为纵深防御保留。

} 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