Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
27 changes: 21 additions & 6 deletions packages/cli/src/ui/utils/goal-runtime.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,16 +28,22 @@ 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);
// Config.getGoalRuntimeReady() throws synchronously (not a rejected
// promise) when persistence is unavailable; the gate must swallow that
// shape too or it escapes the startup effect as an unhandled rejection
// and the TUI never leaves its init banner (#10272).
it('allows Goal-less sessions when readiness throws synchronously', async () => {
const getGoalRuntimeReady = vi.fn((): Promise<GoalRuntime> => {
throw new GoalPersistenceUnavailableError();
});

await expect(waitForGoalRuntime({ getGoalRuntimeReady })).rejects.toBe(
failure,
await expect(waitForGoalRuntime({ getGoalRuntimeReady })).resolves.toBe(
true,
);
expect(getGoalRuntimeReady).toHaveBeenCalledTimes(1);
});

it('allows Goal-less sessions when readiness throws synchronously', async () => {
it('swallows a synchronous readiness throw inside the bounded wait too', async () => {
const getGoalRuntimeReady = vi.fn((): Promise<GoalRuntime> => {
throw new GoalPersistenceUnavailableError();
});
Expand All @@ -47,6 +53,15 @@ describe('waitForGoalRuntime', () => {
).resolves.toBe(true);
});

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('does not hide synchronous readiness errors', async () => {
const failure = new Error('unsupported Goal lifecycle record');
const getGoalRuntimeReady = vi.fn((): Promise<GoalRuntime> => {
Expand Down
4 changes: 4 additions & 0 deletions packages/cli/src/ui/utils/goal-runtime.ts
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,10 @@ export async function waitForGoalRuntime(
): Promise<boolean> {
const awaitReady = async (): Promise<void> => {
try {
// The call must stay inside the try: Config.getGoalRuntimeReady()
// THROWS synchronously (rather than returning a rejected promise)
// when persistence is unavailable, and escaping the catch turned it
// into an unhandled rejection that killed the startup effect.
await config.getGoalRuntimeReady();
Comment on lines +59 to 63

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] R2-1: The production fix this PR describes no longer comes from this PR — it already landed on main via PR 10290 (commit 1637aa968b), which this branch merged during conflict resolution. Against the current merge base the net production delta is zero: this PR now contributes the explanatory comment above, the witness-test reorganization plus one new unbounded-path test, and a prettier reflow in teamHelpers.test.ts. The closing issue 10298 was already fixed on the base before this PR's merge commit.

The description is now inverted in two places: it says PR 10290 "can be dropped or reverted in favour of this fix" and that this PR "supersedes the quarantine approach" there — but PR 10290 is merged and carries the identical fix. Verification step 1 ("On main (pre-fix)… Observe the test hang") no longer reproduces on current main, so the evidence is unfalsifiable for future readers; and anyone later reverting "the 10307 fix" to undo the behaviour would revert only a comment and tests while believing the production fix is gone, leaving the attribution of the actual fix ambiguous.

Update the description to state that the production fix came via the PR 10290 merge and frame this PR's net contribution as the comment plus the witness tests — or close as superseded by PR 10290.

Witness: not run — the claim is git-history/PR-metadata fidelity, settled by git ancestry (PR 10290's commit is an ancestor of the merge base; the net diff contains zero production-behavior lines) and by the live PR/issue states (PR 10290 merged, issue 10298 closed at the same time).

中文说明

本 PR 描述中的生产修复已不再来自本 PR —— 该修复已通过 PR 10290(提交 1637aa968b)合入 main,而本分支在解决冲突时合入了该提交。相对当前合并基点,本 PR 的生产代码净差量为零:现在只贡献上面这段说明性注释、见证测试的重组加一个新增的无界路径测试,以及 teamHelpers.test.ts 中的一处 prettier 重排。关闭的 issue 10298 在本 PR 的合并提交之前就已经在基点上被修复。

描述现在有两处与事实相反:描述称 PR 10290 "可以放弃或回退,改用本修复",并称本 PR "取代其中的隔离方案" —— 但 PR 10290 已经合入并且携带了完全相同的修复。验证步骤 1("在 main(修复前)上……观察测试挂起")在当前 main 上不再复现,因此这些证据对后来的读者不可证伪;而且如果将来有人想通过回退 "10307 的修复" 来撤销该行为,实际只会回退一段注释和测试,却以为生产修复被移除了,从而让实际修复的归属变得含糊。

建议更新描述,说明生产修复是通过 PR 10290 的合入带来的,并将本 PR 的净贡献定位为注释加见证测试 —— 或者直接以被 PR 10290 取代为由关闭本 PR。

见证:未运行 —— 该结论属于 git 历史 / PR 元数据的保真度问题,由 git 祖先关系(PR 10290 的提交是合并基点的祖先;净差量不含任何生产行为代码行)与线上 PR/issue 状态(PR 10290 已合并、issue 10298 同时关闭)确定。

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

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 against git: 1637aa968b (PR 10290) is an ancestor of the current merge base and carries the identical production fix (the call moved inside the try); this branch's net diff against the merge base contains zero production-behavior lines. The finding stands.

Both remedies are GitHub-level actions this bot cannot perform, and the choice between them is a maintainer decision, so this thread is left unresolved:

  • A — close as superseded by PR 10290 (cleanest attribution, zero net delta).
  • B — keep this PR with a corrected description: the production fix came via the PR 10290 merge, and this PR contributes the invariant-pinning comment (the call must stay inside the try because Config.getGoalRuntimeReady() throws synchronously) plus the unbounded-path witness test (main's synchronous-throw test only exercises the timeoutMs path).

Recommendation: B for the comment and the extra witness; A if a zero-delta history is preferred. Which way should this go? (For B the PR body also needs editing — the bot cannot do that.)

中文说明

已通过 git 核实:1637aa968b(PR 10290)是当前合并基点的祖先,且携带完全相同的生产修复(调用已移入 try 内部);本分支相对合并基点的净差量不含任何生产行为代码行。该发现成立。

两种处理方式都是本机器人无法执行的 GitHub 层面操作,且二者之间的取舍属于维护者决策,因此本线程保持未解决状态:

  • A — 以被 PR 10290 取代为由关闭(归属最清晰,净差量为零)。
  • B — 保留本 PR 但更正描述:生产修复是通过 PR 10290 的合入带来的,本 PR 贡献的是固定约束的注释(调用必须留在 try 内,因为 Config.getGoalRuntimeReady() 会同步抛出异常)以及无界路径的见证测试(main 上的同步抛出测试只覆盖了 timeoutMs 路径)。

建议:选 B,以保留该注释和额外的见证测试;若偏好零差量的历史则选 A。这个问题应如何处理?(若选 B,还需编辑 PR 正文 —— 机器人无法执行该操作。)

} catch (error) {
if (!(error instanceof GoalPersistenceUnavailableError)) throw error;
Expand Down
4 changes: 1 addition & 3 deletions packages/core/src/agents/team/teamHelpers.test.ts
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