feat(cli): adopt Goal v3 in ACP sessions - #8732
Conversation
Goal v3 ACP E2E 验证报告验证范围
手工端到端链路
浏览器证据
自动化验证
验证中发现并修复
环境观察实测模型 |
🩺 serve daemon A/BBuilt the PR base vs this PR head ✅ No response changes against the PR base across 4 scenario(s). — Qwen Code · serve A/B |
The Goal v3 adoption made the Session constructor bind the canonical Goal runtime, and Session.prompt reserve a turn on it. Both call sites tolerate GoalPersistenceUnavailableError and nothing else, so the two Session test files that were not part of this change — Session.worktree.test.ts and Session.review-lease.test.ts — died in the constructor with "this.config.getGoalRuntime is not a function", failing all 12 of their tests on Test (ubuntu-latest, Node 22.x). Teach both config fakes to answer getGoalRuntime/getGoalRuntimeReady the way a real Config does when Goal persistence is off: throw (reject with) GoalPersistenceUnavailableError. That exercises the intended fall-through and leaves what the tests actually assert — worktree notices and review lease sweeps — untouched.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: reverse audit — stopped before round 2 by the review time budget.
中文说明
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查：反向审计——评审时间预算不足，未能开始第 2 轮。
— qwen3.8-max via Qwen Code /review (v0.21.7)
| @@ -1441,9 +1440,8 @@ export interface AcpSessionBridge { | |||
| ): Promise<{ cleared: boolean; condition?: string }>; | |||
There was a problem hiding this comment.
[Suggestion] R1-7: The clear-goal route now returns snapshot on the wire, but this typed contract — and the bridge impl's requestSessionStatus generic plus SDK DaemonClient.sessionGoalClear — was left without it, unlike BridgeSessionGoal, which this PR updated to carry snapshot. — Concrete cost: a typed consumer that wants to update its goal card from the post-clear snapshot (instead of a second sessionGoalGet round-trip) needs an as cast; the get and clear contracts now disagree about the same canonical field, and this PR's own test (snapshot: after in the clear response) codifies a wire shape no typed layer declares.
| ): Promise<{ cleared: boolean; condition?: string }>; | |
| ): Promise<{ cleared: boolean; condition?: string; snapshot: GoalSnapshotV2 }>; |
(If the snapshot is deliberately not part of the clear contract, strip it from the handler response and the test instead.)
中文说明
clear-goal 路由现在会在线上返回 snapshot 字段，但这个类型契约 —— 以及 bridge 实现的 requestSessionStatus 泛型和 SDK 的 DaemonClient.sessionGoalClear —— 都没有加上它；而本 PR 却更新了 BridgeSessionGoal 使其携带 snapshot。— 具体代价：想利用 clear 后的 snapshot 更新 goal 卡片（避免再发一次 sessionGoalGet）的类型化消费者必须使用 as 强转；get 与 clear 的契约现在对同一个 canonical 字段不一致，且本 PR 自己的测试（clear 响应中的 snapshot: after）把一个没有任何类型层声明的线上形状固化了下来。修复：按 suggestion 拓宽类型；如果 snapshot 有意不属于 clear 契约，则应从 handler 响应和测试中移除它。
— qwen3.8-max via Qwen Code /review (v0.21.7)
| }); | ||
|
|
||
| it('loadSession leaves the goal hook alone when hooks are disabled by policy', async () => { | ||
| it('leaves canonical Goal recovery to Config initialization', async () => { |
There was a problem hiding this comment.
[Suggestion] R1-8: This replacement test only exercises loadSession, but the deleted suite explicitly covered both loadSession and unstable_resumeSession — separate parallel implementations in acpAgent.ts, both changed by this diff (#restoreGoalOnResume was removed from both). — Failure scenario: if legacy goal-hook restoration were reintroduced (or left) on only the resume path, no test asserts registerGoalHook/unregisterGoalHook are not called on resume, so the suite passes and the regression ships. The neighboring tests in this describe block use it.each(['load', 'resume']) for exactly this drift risk.
Convert to it.each(['load', 'resume'] as const)(...) dispatching to agent.loadSession(...) / agent.unstable_resumeSession(...), as the adjacent tests do.
中文说明
这个替代测试只执行了 loadSession，但被删除的测试套件明确同时覆盖了 loadSession 和 unstable_resumeSession —— 它们是 acpAgent.ts 中两个并行的独立实现，且本 diff 对两者都做了修改（两条路径上的 #restoreGoalOnResume 都被移除）。— 失败场景：如果旧式 goal hook 恢复逻辑被重新引入（或遗留）在 resume 路径上，没有任何测试断言 resume 时不会调用 registerGoalHook/unregisterGoalHook，测试套件照样通过、回归就此上线。同 describe 块中的相邻测试正是为了这种漂移风险而使用 it.each(['load', 'resume'])。修复：同样改为 it.each(['load', 'resume'] as const) 并分别调用两个方法。
— qwen3.8-max via Qwen Code /review (v0.21.7)
| const activeGoal = | ||
| snapshot.goal?.status === 'active' ? snapshot.goal : null; |
There was a problem hiding this comment.
[Suggestion] R1-12: sessionGoalGet's active projection is only tested for status === 'active' and goal: null; the middle case — a goal that exists but is not active (paused/complete/blocked) — has no test. Non-active goals are a live state in this system: /goal pause produces them, and canonical recovery of legacy transcripts creates status: 'paused' goals. — Failure scenario: after /goal pause, a host calls sessionGoalGet and must see active: null while snapshot.goal stays populated. If this condition regresses to snapshot.goal ?? null, every legacy host reading active shows a paused goal as running with nothing driving it — the phantom-active-goal class the removed supersedeUnrestorableGoal cards existed to correct — and no test turns red.
Add an acpAgent test where getSnapshot() returns a goal with status: 'paused' and assert the response is { snapshot, active: null }.
中文说明
sessionGoalGet 的 active 投影只测试了 status === 'active' 和 goal: null 两种情况；中间情况 —— goal 存在但不活动（paused/complete/blocked）—— 没有测试。非活动 goal 在本系统中是真实存在的状态：/goal pause 会产生它，legacy transcript 的 canonical 恢复也会创建 status: 'paused' 的 goal。— 失败场景：/goal pause 之后，host 调用 sessionGoalGet 应当看到 active: null 而 snapshot.goal 仍有值。如果这个条件回归成 snapshot.goal ?? null，所有读取 active 的 legacy host 都会把一个暂停的 goal 显示为正在运行且无任何驱动 —— 正是被移除的 supersedeUnrestorableGoal 卡片所要纠正的幽灵活动 goal 一类问题 —— 而且没有测试会变红。修复：补一个 getSnapshot() 返回 status: 'paused' goal 的测试并断言响应为 { snapshot, active: null }。
— qwen3.8-max via Qwen Code /review (v0.21.7)
| if (this.pendingPrompt) return; | ||
| if (this.goalProcessing) return; | ||
| if (this.notificationProcessing) return; |
There was a problem hiding this comment.
[Suggestion] R1-13: Cron↔Goal serialization is untested in both directions — the new goalProcessing guards in #drainCronQueue/processCronQueue and the cron gates in #drainGoalQueue have no test, while the analogous notification↔goal direction is covered by 'drains a queued Goal turn after a background notification settles'. — Failure scenario: a cron job fires while an automatic Goal turn is mid-flight; if the goalProcessing guard regresses, #drainCronQueue calls prompt() concurrently and the prompt-admission machinery aborts the active Goal turn with NEW_PROMPT_ABORT_REASON — a scheduled cron prompt silently cancels in-progress Goal work (the turn settles as cancelled rather than completing), and no test would go red.
Add a Session test modeled on the notification test: arm a cron queue entry while a host-scheduled Goal turn is streaming, assert the cron prompt does not start until the Goal turn settles, then drains.
中文说明
Cron↔Goal 的串行化在两个方向上都没有测试 —— #drainCronQueue/processCronQueue 中新加的 goalProcessing 守卫以及 #drainGoalQueue 中的 cron 门都没有测试，而类似的 notification↔goal 方向却有 'drains a queued Goal turn after a background notification settles' 覆盖。— 失败场景：Goal 自动轮次执行中 cron 任务触发；若 goalProcessing 守卫回归，#drainCronQueue 会并发调用 prompt()，prompt 准入机制会以 NEW_PROMPT_ABORT_REASON 中止活动的 Goal 轮次 —— 一个计划内 cron prompt 静默取消了正在进行的 Goal 工作（该轮次以取消而非完成结算），且没有测试会变红。修复：仿照 notification 测试补一个 Session 测试：在 host 调度的 Goal 轮次流式执行期间塞入 cron 队列项，断言 cron prompt 在 Goal 轮次结算前不会启动、之后才被排空。
— qwen3.8-max via Qwen Code /review (v0.21.7)
| if (!result.cause) { | ||
| await this.#queueGoalState( |
There was a problem hiding this comment.
[Suggestion] R1-14: A bare /goal status query on ACP no longer produces any client-visible output — this branch emits an agent_message_chunk with empty text whose only payload is _meta.goalState (the legacy projection attaches only when a cause is present), and no in-tree host consumes goalState. The removed legacy path returned a human-readable status summary. — Failure scenario: a raw ACP client (e.g. Zed — the Web Shell intercepts bare /goal locally and opens its Goals page instead) sends /goal; the user receives no reply at all, where previously they got "Goal active: … (n turns) / Last check: …" or "No goal set. Usage: …" — a wire-format UX regression for third-party ACP hosts.
For cause-less (status) publications, either attach the same legacy projection (projectGoalStateToLegacy with the snapshot's effective state) or emit a short text summary so existing hosts render something.
中文说明
在 ACP 上单独发送 /goal 状态查询不再产生任何客户端可见输出 —— 这个分支发出的是文本为空的 agent_message_chunk，唯一载荷是 _meta.goalState（legacy 投影只在有 cause 时才附加），而树内没有任何 host 消费 goalState。被移除的 legacy 路径会返回人类可读的状态摘要。— 失败场景：原生 ACP 客户端（例如 Zed —— Web Shell 会在本地拦截单独的 /goal 并改为打开 Goals 页面）发送 /goal；用户完全收不到任何回复，而之前会得到 "Goal active: … (n turns) / Last check: …" 或 "No goal set. Usage: …" —— 对第三方 ACP host 是线上格式的 UX 回归。修复：对无 cause 的（status）发布附加同样的 legacy 投影，或发一条简短文本摘要，让现有 host 有内容可渲染。
— qwen3.8-max via Qwen Code /review (v0.21.7)
| this.historyReplayer = new HistoryReplayer(this, { | ||
| supersedeUnrestorableGoal: true, | ||
| }); | ||
| this.historyReplayer = new HistoryReplayer(this); |
There was a problem hiding this comment.
[Suggestion] R1-15: Dropping supersedeUnrestorableGoal (and the hook re-registration it was paired with) leaves resumed pre-change transcripts showing an active goal that nothing drives and that no wire surface can describe. Every link verified in code: canonical recovery migrates the legacy goal to status: 'paused' (so no continuation is ever scheduled), the 'migrated' broadcast projects to legacy kind 'set', the web-shell banner treats set/checking as active (VALID_GOAL_KINDS has no 'paused'), while sessionGoalGet projects active: null for a paused goal so the Goals page omits it entirely. The removed mechanism's own JSDoc warned about exactly this display. — Failure scenario: a session created before this change whose transcript ends on an active legacy goal card (set/checking) is resumed after the upgrade — replay renders the legacy card, the banner shows a running goal, nothing works on it, the management surface can't see it, and the only escape is typing /goal resume in-session.
Surface the migrated/paused state to legacy clients — e.g. project 'migrated' as 'paused' and teach the web-shell/webui parsers that kind (clearing the active banner), or emit a corrective compat card on resume when recovery yields a paused goal.
中文说明
移除 supersedeUnrestorableGoal（以及与之配对的 hook 重注册）后，恢复升级前的 transcript 会显示一个没有任何东西驱动、也没有任何线上接口能描述的活动 goal。每一环都已在代码中核实：canonical 恢复把 legacy goal 迁移为 status: 'paused'（因此永远不会调度 continuation），'migrated' 广播投影成 legacy kind 'set'，web-shell 横幅把 set/checking 当作活动状态（VALID_GOAL_KINDS 没有 'paused'），而 sessionGoalGet 对 paused goal 投影出 active: null，Goals 页面因此完全看不到它。被移除机制自身的 JSDoc 恰好警告过这种显示问题。— 失败场景：升级前创建、transcript 以活动 legacy goal 卡片结尾的会话在升级后恢复 —— replay 渲染 legacy 卡片，横幅显示 goal 运行中，但它无法工作、管理界面看不到它，唯一解法是在会话内输入 /goal resume。修复：把 migrated/paused 状态透传给 legacy 客户端 —— 例如把 'migrated' 投影为 'paused' 并让 web-shell/webui 解析器认识该 kind（清除活动横幅），或在恢复得到 paused goal 时发一张纠正用的兼容卡片。
— qwen3.8-max via Qwen Code /review (v0.21.7)
| await boundGoalHost!.startGoalTurn({ | ||
| permit, | ||
| continuationContext: 'check weather', | ||
| }); | ||
| await Promise.resolve(); | ||
| expect(mockChat.sendMessageStream).toHaveBeenCalledTimes(1); |
There was a problem hiding this comment.
[Suggestion] R1-16: This regression test has no teeth — mutation-tested empirically: removing the notificationProcessing/notificationAbortController conditions from #drainGoalQueue's gate leaves every assertion here passing, because the mocked notification stream is abort-blind (unlike the priority test's stream) and the single-microtask window is too shallow, so a regressed gate that preempts the in-flight notification turn produces the same observable trace as the correct queued behavior. With the fix below instrumented, the mutated gate fails and the correct gate passes. — Failure scenario: with the gate regressed, a host-scheduled Goal turn aborts an in-flight background notification turn (notification model work interrupted mid-turn, partial recording) while this test stays green.
Make the notification mock stream abort-aware (resolve on notificationGate OR the request's abortSignal, mirroring the automatic-turn stream in the priority test), capture that first request's abortSignal, and after startGoalTurn assert expect(notificationSignal.aborted).toBe(false) — a broken gate's prompt() aborts it, which is the observable that distinguishes queueing from preemption.
中文说明
这个回归测试没有牙齿 —— 已用变异实测验证：把 notificationProcessing/notificationAbortController 条件从 #drainGoalQueue 的门里去掉，这里的所有断言依然通过，因为 mock 的 notification 流对中止不敏感（不同于优先级测试里的流），且单个微任务的窗口太浅，于是"回归后的门抢占进行中的 notification 轮次"与"正确排队行为"产生完全相同的可观测轨迹。按下面的修复加上检测后，变异后的门会失败、正确的门会通过。— 失败场景：门回归后，host 调度的 Goal 轮次会中止正在进行的后台 notification 轮次（notification 的模型工作被中途打断、记录不完整），而本测试依然绿色。修复：让 notification mock 流感知中止（在 notificationGate 或请求的 abortSignal 上任一完成即返回），捕获首个请求的 abortSignal，并在 startGoalTurn 之后断言它未被中止 —— 坏门的 prompt() 会中止它，这正是区分"排队"与"抢占"的可观测点。
— qwen3.8-max via Qwen Code /review (v0.21.7)
…ersistence Addresses the three [Critical] findings from review 4888864998. - R1-1 `#drainGoalQueue`: settle the turn in the catch. `prompt()` can reject before reaching the try whose finally settles it -- most easily via `assertCanStartTurn` throwing 'Session is closing' when `beginClose()` lands mid-await. The turn is already off `goalQueue` at that point, so the runtime kept `currentPermit` and `activity: 'running'` forever: no further continuations were scheduled and every later prompt with an active goal hung in `claimGoalTurn` behind the leaked permit. `#settleGoalTurn` no-ops once the permit is no longer current and swallows its own errors, so calling it here is safe to repeat. - R1-2 `sessionGoalGet`/`sessionGoalClear`: catch `GoalPersistenceUnavailableError` and fall back to the empty answers. `getGoalRuntimeReady()` rejects when `general.chatRecording` is off, and rejects permanently once `restore()` has set a sticky `recoveryError`; `extMethod` only converts `SessionWriterError`. That made `GET /goals` drop the session on every poll with a permanent non-zero `droppedCount` -- rendered as "couldn't probe N sessions", misreading a config choice as a wedged child -- and broke goal get/clear for a session with a malformed trailing goal record forever. Any other error still propagates. - R1-3 `#settleGoalTurn`: a turn preempted by a newly arrived user prompt now settles via `finishTurn`, not pause. A goal turn's controller is `this.pendingPrompt`, so a new prompt aborts it with `NEW_PROMPT_ABORT_REASON`; whether that surfaced as a clean `cancelled` stop reason or as a throw from the model network await was pure timing, so the same user action persisted the goal as either active-with-handoff or paused. The paused branch also cleared the incoming prompt's queued reservation and silently stopped the autonomous loop. Scoped to the settle path rather than widening `isControlledCancellation`, which is shared with non-goal turns. Tests: `releases the permit when the prompt rejects before the model starts` (dropping the settle call fails it); two acpAgent cases pinning the empty goal get/clear answers plus one pinning that a non-persistence error still propagates (rethrowing in either handler fails the matching case); `hands off a preempted Goal turn whose stream throws on abort`, the throwing-stream twin of the shipped priority test (dropping the superseded check fails it). `mockGoalRuntime` gains `dispatch` so pause is assertable at all.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: reverse audit — stopped before round 2 by the review time budget.
中文说明
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查：反向审计——评审时间预算不足，未能开始第 2 轮。
— qwen3.8-max via Qwen Code /review (v0.21.7)
| expect(registerGoalHook).not.toHaveBeenCalled(); | ||
| // `registerGoalHook` not being called is not enough on its own: anything | ||
| // that throws inside `#restoreGoalOnResume` skips it too, so a broken | ||
| // config mock would satisfy the assertion above while never reaching the | ||
| // hooks-disabled branch this test is named for. Pin the branch. | ||
| const written = stderr.mock.calls.map((c) => String(c[0])).join(''); | ||
| expect(written).toContain('hooks-disabled'); | ||
| expect(written).not.toContain('goal restore failed'); | ||
| stderr.mockRestore(); | ||
| expect(unregisterGoalHook).not.toHaveBeenCalled(); |
There was a problem hiding this comment.
[Suggestion] R2-14: The PR deletes the only test pinning 'session load completes even when Goal restore throws' (whose comment stated the invariant: 'it must never take the session down with it') and provides no replacement — the new test's goal_state fixture is inert (loadCliConfig, Config and Session are all mocked), so its assertions only prove the legacy hook module went untouched. — Failure scenario: the best-effort guarantee now rests on three unpinned contracts: initializeGoalRuntime swallowing the restore rejection, nothing in the init path awaiting goalRuntimeReady, and Session.#bindGoalRuntime catching the GoalPersistenceUnavailableError that bindHost rethrows when recoveryError is sticky. A plausible hardening change — awaiting getGoalRuntimeReady() before serving a loaded session, or #bindGoalRuntime rethrowing an error it doesn't classify — would make loadSession/unstable_resumeSession reject outright for any transcript holding a malformed/unsupported goal record (a version downgrade after a newer build wrote v3 records would make such sessions unloadable), and no test would fail (Session.review-lease/worktree tests only exercise the shallower getGoalRuntime()-throws path).
Suggested fix: add a successor pin where recovery is real — in config.test.ts, construct a Config with chatRecording: true and sessionData containing a malformed goal_state record, assert initialize() resolves while getGoalRuntimeReady() rejects; and/or give a Session test a runtime whose bindHost throws GoalPersistenceUnavailableError and assert the Session constructs and serves prompts.
中文说明
本 PR 删除了唯一钉住「即使 Goal restore 抛出，会话加载也要完成」的测试（其注释写明不变量：『绝不能因此拖垮会话』），且没有替代——新测试的 goal_state fixture 是惰性的（loadCliConfig、Config、Session 全部被 mock），其断言只证明 legacy hook 模块未被触碰。失败场景：这个尽力保证现在依赖三个未被钉住的契约：initializeGoalRuntime 吞掉 restore 的 rejection、init 路径不 await goalRuntimeReady、Session.#bindGoalRuntime 捕获 bindHost 在粘性 recoveryError 时重新抛出的 GoalPersistenceUnavailableError。一个合理的加固改动——在服务已加载会话前 await getGoalRuntimeReady()，或 #bindGoalRuntime 重新抛出未分类的错误——会让任何含畸形/不支持 goal 记录的 transcript 的 loadSession/unstable_resumeSession 直接 reject（更新版本写入 v3 记录后降级会使这类会话无法加载），且没有任何测试失败（Session.review-lease/worktree 测试只覆盖较浅的 getGoalRuntime() 抛出路径）。修复建议：在恢复是真实发生的层补一个替代钉住——在 config.test.ts 中构造 chatRecording: true 且 sessionData 含畸形 goal_state 记录的 Config，断言 initialize() resolve 而 getGoalRuntimeReady() reject；和/或在 Session 测试中给一个 bindHost 抛 GoalPersistenceUnavailableError 的 runtime，断言 Session 能构造并服务 prompt。
— qwen3.8-max via Qwen Code /review (v0.21.7)
| ): Promise<{ cleared: boolean; condition?: string }>; | ||
|
|
||
| /** | ||
| * Read a live session's goal state. Throws `SessionNotFoundError` when the | ||
| * session is not resident — goals live in the child's memory, so a | ||
| * non-resident session has no goal to report. | ||
| * Read a live session's Goal state. Throws `SessionNotFoundError` when the | ||
| * session is not resident because this route addresses the selected runtime. | ||
| */ | ||
| getSessionGoal(sessionId: string): Promise<BridgeSessionGoal>; |
There was a problem hiding this comment.
[Suggestion] R1-7 (still standing at this commit): the child's sessionGoalClear ext method now returns {cleared, condition, snapshot} and the sibling BridgeSessionGoal/getSessionGoal contract was updated to carry snapshot, but the declared clearSessionGoal return type above, the bridge pass-through, and SDK DaemonClient.sessionGoalClear were left without it — so the clear route ships an undeclared wire field. — Failure scenario: the bridge forwards the child response verbatim (requestSessionStatus casts response as unknown as T with no shaping), so every POST /session/:id/goal/clear response carries the full GoalSnapshotV2 (a GoalRecord whose lastReason alone may reach ~16KB per GOAL_PROPOSAL_REASON_MAX_BYTES) that no typed consumer can see: webui clearGoal reads only {cleared}, the SDK type omits it. Dead wire payload, and any consumer wanting the post-clear state must cast around the declared API.
Suggested fix: either declare snapshot: GoalSnapshotV2 in the clearSessionGoal return type here (and the SDK's sessionGoalClear), or drop snapshot from the clear ext-method response if no consumer is meant to read it there.
中文说明
R1-7（在本 commit 仍然存在）：child 的 sessionGoalClear ext 方法现在返回 {cleared, condition, snapshot}，兄弟契约 BridgeSessionGoal/getSessionGoal 已更新为携带 snapshot，但上方的 clearSessionGoal 返回类型、bridge 透传以及 SDK DaemonClient.sessionGoalClear 都没有加——于是 clear 路由在线上携带了一个未声明的字段。失败场景：bridge 原样转发 child 响应（requestSessionStatus 以 response as unknown as T 强转、不做整形），因此每个 POST /session/:id/goal/clear 响应都携带完整 GoalSnapshotV2（其中仅 lastReason 就可达约 16KB，按 GOAL_PROPOSAL_REASON_MAX_BYTES），而没有任何类型化 consumer 能看到：webui clearGoal 只读 {cleared}，SDK 类型也没有它。死线上载荷；任何想要清除后状态的 consumer 都必须绕过声明的 API 强转。修复建议：要么在此处的 clearSessionGoal 返回类型（及 SDK 的 sessionGoalClear）声明 snapshot: GoalSnapshotV2，要么如果不打算有 consumer 读取，就从 clear ext 方法响应中移除 snapshot。
— qwen3.8-max via Qwen Code /review (v0.21.7)
| const shouldPause = | ||
| !supersededByNewPrompt && | ||
| (failed || | ||
| result?.stopReason === 'max_tokens' || | ||
| cancelledByUser || | ||
| turn.controller.signal.reason === SESSION_DISPOSE_ABORT_REASON); |
There was a problem hiding this comment.
[Suggestion] R1-11 (still standing at this commit — mutation re-verified): every settlement branch of #settleGoalTurn that pauses the goal has zero positive assertions — the only action: 'pause' expectation in Session.test.ts is the negative one (handoff must not pause). Deleting the entire pause block leaves 556 tests green. — Failure scenario: a future edit that drops or inverts the shouldPause dispatch ships silently: a user cancel (session/cancel), a failed turn, a max_tokens turn, or session dispose would settle via runtime.finishTurn() instead of pausing, and the runtime would keep scheduling autonomous Goal continuation turns against a goal the user just cancelled — unbounded model/tool execution the cancel was supposed to stop. The mock's own comment ('Present so settlement branches that pause the goal are assertable') states intent no test exercises.
Suggested fix: add a test per trigger (at minimum user-cancel): run a host-scheduled Goal turn whose stream ends with stopReason: 'cancelled' and turn.controller aborted with USER_CANCEL_ABORT_REASON, then expect(mockGoalRuntime.dispatch).toHaveBeenCalledWith(expect.objectContaining({ action: 'pause' })) and expect(finishTurn).not.toHaveBeenCalled().
中文说明
R1-11（在本 commit 仍然存在——已重新做变异验证）：#settleGoalTurn 中所有 pause Goal 的结算分支都没有正向断言——Session.test.ts 中唯一的 action: 'pause' 期望是否定式的（handoff 不得 pause）。删除整个 pause 块后 556 个测试仍全绿。失败场景：未来丢弃或反转 shouldPause dispatch 的修改会静默通过：用户取消（session/cancel）、失败轮次、max_tokens 轮次或会话 dispose 都会改走 runtime.finishTurn() 而非 pause，runtime 会继续为用户刚刚取消的 Goal 调度自动 continuation 轮次——取消本应停止的无界模型/工具执行。mock 自己的注释（『为了让 pause Goal 的结算分支可断言而存在』）声明了没有测试行使的意图。修复建议：为每个触发补一个测试（至少 user-cancel）：运行一个 host 调度的 Goal 轮次，其流以 stopReason: 'cancelled' 结束且 turn.controller 以 USER_CANCEL_ABORT_REASON 中止，然后断言 dispatch 以 action: 'pause' 被调用且 finishTurn 未被调用。
— qwen3.8-max via Qwen Code /review (v0.21.7)
| if ( | ||
| this.pendingPrompt || | ||
| this.goalProcessing || | ||
| this.cronProcessing || | ||
| this.cronAbortController | ||
| ) { |
There was a problem hiding this comment.
[Suggestion] R1-13 (still standing at this commit — mutation re-verified): the new goalProcessing guards in the cron/notification drain paths (six sites in #drainCronQueue/#drainNotificationQueue, of which this is one) are tested in only one direction — removing all six leaves 558 tests green. The existing notification→goal test exercises #drainGoalQueue's own guard, not these. — Failure scenario: a background-task notification (or a cron prompt) arriving while an automatic Goal turn is mid-turn would, without the guard, start a second model turn on the same session: two concurrent sendMessageStream flows interleaving transcript records and stream updates. The inverse ordering is proven; this ordering is asserted by nothing.
Suggested fix: mirror the existing test — start a host-scheduled Goal turn, fire notificationCallback(...) while the stream is in flight, assert sendMessageStream is called only once until the goal turn settles, then that the notification drains afterwards.
中文说明
R1-13（在本 commit 仍然存在——已重新做变异验证）：cron/通知 drain 路径中新增的 goalProcessing 闸门（#drainCronQueue/#drainNotificationQueue 共六处，此为其中之一）只在一个方向有测试——把六处全部移除后 558 个测试仍全绿。现有的 notification→goal 测试行使的是 #drainGoalQueue 自己的闸门，不是这些。失败场景：若没有该闸门，后台任务通知（或 cron prompt）在自动 Goal 轮次进行中到达会在同一会话启动第二个模型轮次：两个并发 sendMessageStream 流交错 transcript 记录与流式更新。反方向的顺序已有证明；这个顺序没有任何断言。修复建议：镜像现有测试——启动一个 host 调度的 Goal 轮次，在流进行中触发 notificationCallback(...)，断言 Goal 轮次结算前 sendMessageStream 只被调用一次，之后通知才被取出。
— qwen3.8-max via Qwen Code /review (v0.21.7)
| if (!result.cause) { | ||
| await this.#queueGoalState( | ||
| result.response.snapshot, |
There was a problem hiding this comment.
[Suggestion] R1-14 (still standing at this commit — re-verified end-to-end): causeless goal_control results (bare /goal status reads and /goal clear with no active goal) answer with an empty-text agent_message_chunk carrying only _meta.goalState — a field no production consumer reads — where pre-diff runLegacyGoalCommand returned visible text. — Failure scenario: a user typing /goal or /goal clear (with no goal) in webui/Zed gets a silently empty response and concludes the command is broken: webui normalizeGoalStatusEvent reads only goalStatus/goalTerminal/stopHookLoop, the string goalState appears nowhere in packages/webui, web-shell parses sentinel-serialized goal-status text, and acp-bridge consumes goalState only from persisted records during replay — no surface renders live _meta.goalState. Session.test.ts 'emits canonical Goal state for an ACP /goal status query' blesses exactly this consumer-less shape. Round 2's audit independently rates the harm Critical-level; severity kept at Suggestion per the round-1 filing, given the PR defers Web Shell canonical-snapshot consumption to a follow-up UI PR.
Suggested fix: for causeless results, emit what existing surfaces already render — pass a synthetic cause through projectGoalStateToLegacy so _meta.goalStatus rides the update, or emit a text summary via emitSlashCommandOutput like the non-interactive path — or land the webui goalState consumer before removing the old text response.
中文说明
R1-14（在本 commit 仍然存在——已端到端重新验证）：无 cause 的 goal_control 结果（裸 /goal 状态查询、无活动 goal 时的 /goal clear）以空文本的 agent_message_chunk 应答，唯一载荷是 _meta.goalState——一个没有生产 consumer 读取的字段——而改动前 runLegacyGoalCommand 返回可见文本。失败场景：用户在 webui/Zed 输入 /goal 或 /goal clear（无 goal 时）会得到静默的空响应并认为命令坏了：webui normalizeGoalStatusEvent 只读 goalStatus/goalTerminal/stopHookLoop，字符串 goalState 在 packages/webui 中无处可见，web-shell 解析 sentinel 序列化的 goal-status 文本，acp-bridge 只在回放期间消费持久化记录中的 goalState——没有任何界面渲染实时 _meta.goalState。Session.test.ts 的『emits canonical Goal state for an ACP /goal status query』恰恰钉住了这个无 consumer 的形状。第 2 轮审计独立评估该危害达 Critical 级别；鉴于 PR 将 Web Shell 对 canonical snapshot 的消费推迟到后续 UI PR，按第 1 轮的定级保留 Suggestion。修复建议：对无 cause 结果发出既有界面已渲染的内容——传一个合成 cause 经 projectGoalStateToLegacy 使 _meta.goalStatus 随更新发出，或像非交互路径那样经 emitSlashCommandOutput 发出文本摘要——或在移除旧文本应答前先落地 webui 的 goalState consumer。
— qwen3.8-max via Qwen Code /review (v0.21.7)
…paused Addresses four of the six Critical findings from review round 2 on QwenLM#8732. R2-2 (the `claimGoalTurn` / `#drainGoalQueue` deadlock) is not in this commit. R2-1: `#settleGoalTurn` awaited the recording-service `flush()` inside the outer try, so a latched write failure — a taken-over transcript lease, for one — skipped finishTurn/pause/releaseTurn entirely and stranded the runtime's current permit. Isolates the flush, as the headless path's `failClosedActiveGoalTurn` already does. R2-4: `claimGoalTurn`'s immediate path never consulted `signal.aborted`, so a prompt aborted while its preempted turn settled as a handoff claimed the permit that handoff had just promoted to it, then took `prompt()`'s aborted early-exit — which releases only when no goal turn was claimed. Checks the signal before the immediate path. R2-5: goal-persistence unavailability hard-failed `/goal` and `/goal clear`, and in ACP an error return throws, failing the user's whole prompt request for the rest of the session — while `sessionGoalGet`/`sessionGoalClear` answer the same question fine. Those two operations now degrade to an empty snapshot; set/edit/resume still fail, since they need persistence. `emptyGoalSnapshot()` moves to core so both paths share one definition. R2-3: the pause this PR introduces projects as `goalStatus {kind:'paused'}`, which neither webui's `normalizeGoalStatus` nor web-shell's `VALID_GOAL_KINDS` accepted. The card never rendered and the active-goal derivation fell through to the previous `set` card, so every surface kept showing a paused goal as actively running. Teaches both consumers the state and renders it, rather than projecting `pause` to `aborted` — the CLI's own GoalStatusMessage already renders `paused`, so this makes the web surfaces match instead of inventing a third answer. R2-6: a UserPromptSubmit hook that blocks the deterministic goal continuation trapped an active goal in an unbounded release -> requeue -> continuation loop, persisting a transcript record per lap with no model call and no state change. A runtime continuation is machine-generated, so it is now exempt from the hook for the same reason `isContinue` already is. Tests: goal turns settle when the transcript flush rejects; `/goal` and `/goal clear` answer with an empty snapshot when persistence is unavailable, while set/edit/resume still fail.
|
Round 2 Criticals: five of six are fixed in b0f78ad. R2-2 is not — details at the bottom. R2-1 — flush aborting settlement. Applied your suggestion. The isolated flush matches R2-4 — aborted prompt claiming the promoted permit. Applied your suggestion: R2-5 — R2-3 — R2-6 — hook block spin loop. Took the first suggestion: a runtime continuation is machine-generated, so it is exempt from R2-2 — not fixed, deliberately. Your suggested shape (an Verification. Two probes reverting a fix to confirm the new tests are load-bearing:
R2-3, R2-4 and R2-6 ride on the existing suites rather than new tests of their own; R2-4 and R2-6 both need a multi-prompt interleaving to pin, which is the same harness R2-2 needs, so I will add all three together with the R2-2 fix. Suggestions from this round are untouched — Criticals only. |
🖼️ web-shell visual previewRendered against a mock daemon (no real backend): the PR base vs this PR head Screenshots · before / afterℹ️ No screenshot changed against the PR base — but this PR edits 3 render-shaping files:
Either the change has no visual effect (logic, plumbing, a state the scenarios never reach), or no scenario renders this UI — in which case the preview cannot see it, and an empty result is a coverage gap rather than a clean bill of health. To make it visible, add a scenario to Full-resolution recordings (.webm) are attached to the workflow run. — Qwen Code · web-shell visuals |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not explored to full depth (tool budget reached): chunk 3: did not run the two test files (no node_modules in the worktree; skipped cold install of the monorepo to stay within budget).; chunk 3: did not execute Session.review-lease.test.ts / Session.test.ts — worktree has no node_modules and a cold monorepo install was out of budget; all mock surfaces w…; You are review agent reverse-audit — Reverse audit agen...: (none).
Not reviewed: reverse audit — stopped before round 4 by the review time budget.
中文说明
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）：chunk 3：did not run the two test files (no node_modules in the worktree; skipped cold install of the monorepo to stay within budget).；chunk 3：did not execute Session.review-lease.test.ts / Session.test.ts — worktree has no node_modules and a cold monorepo install was out of budget; all mock surfaces w…；You are review agent reverse-audit — Reverse audit agen...：(none)。
未审查：反向审计——评审时间预算不足，未能开始第 4 轮。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| activeTimeMs: 0, | ||
| createdAt: 123, | ||
| updatedAt: 123, |
There was a problem hiding this comment.
[Suggestion] R3-8: the goalSnapshot helper sets createdAt and updatedAt to the same value, so the rewritten "reads a live session goal" test's setAt: 123 assertion cannot distinguish the projection source (setAt: activeGoal.createdAt vs updatedAt). — Concrete cost: a future edit flipping the mapping to updatedAt ships green; the reducer bumps updatedAt (not createdAt) on every state transition and GET /goals sorts rows by setAt (goals.sort((a, b) => b.setAt - a.setAt)), so the Goals page would re-order itself on every state change. Fix: give the helper distinct timestamps, e.g.
activeTimeMs: 0,
createdAt: 123,
updatedAt: 456,(any assertions elsewhere that pin updatedAt: 123 on full-snapshot equality would need updating in the same pass). The serve/routes/goals.test.ts fixture is unaffected — that route consumes active.setAt directly.
中文说明
[Suggestion] R3-8：goalSnapshot 助手把 createdAt 与 updatedAt 设为相同值，因此重写后的 "reads a live session goal" 测试里 setAt: 123 断言无法区分投影来源（setAt: activeGoal.createdAt 还是 updatedAt）。— 具体成本：未来把映射改成 updatedAt 时所有测试绿灯通过；reducer 每次状态迁移只 bump updatedAt（不 bump createdAt），而 GET /goals 按 setAt 排序（goals.sort((a, b) => b.setAt - a.setAt)），Goals 页面将在每次状态变化时重新排序自己。修复：给助手不同的时间戳，如上方代码块；（同一轮里需要同步更新其他按 updatedAt: 123 断言完整 snapshot 相等的位置）。serve/routes/goals.test.ts 的 fixture 不受影响——该路由直接消费 active.setAt。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| it.each(['Ship it', 'edit revised', 'resume'])( | ||
| 'still fails %j, which genuinely needs persistence', |
There was a problem hiding this comment.
[Suggestion] R3-9: the persistence-unavailable degradation boundary is pinned for five of the six operations; pause appears on neither side of these tables. Today the catch degrades only status/clear and pause errors (consistent with the healthy no-goal path), but the code comment describes the degraded class as "operations that only read or reduce state" — which pause reads like. — Concrete cost: a future widening of the degrade condition to include pause (a plausible change) makes /goal pause on a persistence-unavailable session answer with a successful empty-snapshot goal_control instead of an error, and no test fails. Fix: add 'pause' to the still-fails table.
| it.each(['Ship it', 'edit revised', 'resume'])( | |
| 'still fails %j, which genuinely needs persistence', | |
| it.each(['Ship it', 'edit revised', 'resume', 'pause'])( | |
| 'still fails %j, which genuinely needs persistence', |
中文说明
[Suggestion] R3-9：持久化不可用降级边界为六个操作中的五个钉死了测试；pause 在这些表的两侧都没有出现。当前 catch 只降级 status/clear，pause 走错误路径（与健康无 goal 路径一致），但代码注释把降级类描述为"只读或削减状态的操作"——pause 读起来正属于此类。— 具体成本：未来把降级条件放宽到包含 pause（一个看似合理的改动）时，/goal pause 在持久化不可用会话上会开始以成功的空 snapshot goal_control 应答而不是报错，没有任何测试失败。修复：把 'pause' 加进 still-fails 表（见 suggestion）。
— qwen3.8-max via Qwen Code /review (v0.21.8)
Addresses four Critical findings from the round-3 review on QwenLM#8732. R2-2 — a prompt arriving while a goal-activating prompt is still unwinding blocked forever in `claimGoalTurn`. Two entrances, both fixed: - `#drainGoalQueue` is gated on `pendingPrompt`, so a continuation the runtime queued mid-prompt could not start until the waiting prompt finished, while the waiting prompt could not start until that continuation released the permit. An arriving prompt now drops un-started continuations, the way it already drops queued cron and notification work. - `GoalRuntime.releaseTurn` minted a fresh continuation instead of promoting a waiting reservation, so a turn preempted before the model started handed the permit to a turn nobody could drain. It now promotes `queuedTurnKey` exactly as `finishTurn` does. R3-2 — `#settleGoalTurn` isolated `flush()` from a latched recording write failure but not the `pause` dispatch or `finishTurn` that follow, both of which persist through `appendRecordStrict` and re-throw it. The escaping error stranded `currentPermit`, the very state the flush isolation exists to prevent. Both now fall back to the in-memory `releaseTurn`. R3-6 — `legacyStatusKind` projected cause `migrated` as `set`. `createMigratedGoalState` only ever persists `paused`, so resuming a pre-v2 transcript re-asserted an active goal to every client that derives the live goal from the newest card. R3-7 — the Stop-hook blocking cap called `abortGoalForStopHookCap`, which reads only the legacy `activeGoalStore`; that store has no writer for daemon sessions, so the cap stopped nothing and a blocking Stop hook looped the goal forever. ACP now pauses the canonical runtime when the legacy abort reports it did nothing. Verified: npm run build, npm run typecheck, eslint on the six changed files, vitest packages/core/src/goals + client-goal (316 passed), packages/cli/src/acp-integration/session (898 passed), acp-bridge transcript-replay (26 passed). Each new test mutation-probed: reverting its fix turns it red.
…er /clear Round-3 review findings R3-1 and R3-3. R3-1: `goalCommand` degrades a persistence-unavailable `/goal status` and `/goal clear` into a successful empty snapshot, but the headless `goal_control` consumer immediately re-requested the very runtime that had just failed, so `qwen -p "/goal status"` with `general.chatRecording: false` (or a sticky `recoveryError`) exited 1 through the run-level crash path instead of answering "No Goal is set.". Interactive and ACP already degraded correctly; only the third mode crashed. The consumer now swallows `GoalPersistenceUnavailableError` when the snapshot has no goal to drive, and still rethrows when a worker is required. R3-3: the ACP session subscribed to the Goal runtime once in its constructor. `/clear` makes `Config.startNewSession()` dispose that runtime and build a new one under the same long-lived `Session`, leaving the subscription — the only path to `MessageEmitter.emitGoalState` — registered on the abandoned instance, so no `_meta.goalState` update was ever delivered again. `handleSlashCommand` now accepts session hooks, the ACP session passes a `startNewSession` hook the way `clearCommand` expects, and the hook unsubscribes, resets the publication de-duplication state and rebinds. Both are pinned by tests that fail without the change.
|
Round 3's two remaining Criticals are fixed in R3-1 — headless R3-3 — the ACP goal subscription was orphaned by Verification — both fixes are pinned by tests that fail on the parent commit:
Commands run: Suggestions R3-8 and R3-9 (test-fixture timestamps, |
|
Qwen Code review timed out. Qwen review timed out after 21600 seconds (of the 360-minute budget). This run already used the maximum 360 minute timeout. See workflow logs. |
Resolves the conflict in acpAgent.ts left by 60458f5 (fix(serve): Coordinate caller-supplied session IDs, QwenLM#8415). Both sides touched `loadSession` and `unstable_resumeSession`. Upstream wrapped each in a caller-supplied-id reservation — `reserveStartingSessionId` plus a `normalizeSessionIdForLookup` / `findSessionIdIgnoringCase` preamble — which re-indented the whole body into a new try/finally. This branch's change to the same two functions is the Goal v3 migration: drop the `#restoreGoalOnResume` hook, its two call sites, and the `supersedeUnrestorableGoal` replay option, since the Goal runtime now owns restore and the option no longer exists on `collectHistoryReplayUpdates`. The two are orthogonal, so the resolution keeps upstream's control flow verbatim and re-applies this branch's deletions inside it. Verified: npm run build, npm run typecheck, packages/cli src/acp-integration/ (28 files, 1344 tests) and packages/core src/goals/ (15 files, 353 tests) all pass.
|
Merge conflict with Cause. Resolution. The two changes are orthogonal, so upstream's control flow is kept verbatim and this branch's three deletions are re-applied inside it: the Verification. Separately, for the record: review run 31294725239 ran for 6h01m on the previous head and completed with |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: reverse audit — stopped before round 2 by the review time budget.
中文说明
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查：反向审计——评审时间预算不足，未能开始第 2 轮。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| /** | ||
| * Session-scoped callbacks a caller can expose to the commands it runs. | ||
| * Only the ACP host supplies these: it keeps one long-lived session object |
There was a problem hiding this comment.
[Suggestion] R4-3: the new NonInteractiveSlashCommandSessionHooks interface was inserted between handleSlashCommand's existing JSDoc and the function, orphaning that doc comment — verified with the TS compiler API: both JSDoc blocks now attach to the interface, handleSlashCommand has no documentation at all, and the new fifth parameter sessionHooks is undocumented. — Concrete cost: editor hover/TypeDoc associate the old JSDoc (with @param rawQuery/abortController/config/settings and @returns) with the interface, which has none of those parameters, while the public cross-call-site function shows no docs. Suggested fix: move the interface (with its JSDoc) above the existing /** Processes a slash command… */ block so that block re-attaches to handleSlashCommand, and add a @param sessionHooks line to it.
中文说明
[Suggestion] R4-3：新接口 NonInteractiveSlashCommandSessionHooks 被插在 handleSlashCommand 现有的 JSDoc 与函数之间，使该文档注释悬空——已用 TS 编译器 API 验证：两个 JSDoc 块现在都挂到接口上，handleSlashCommand 完全没有文档，新增的第五个参数 sessionHooks 也没有文档。— 具体代价：编辑器悬停/TypeDoc 把旧 JSDoc（含 @param rawQuery/abortController/config/settings 与 @returns）关联到没有任何这些参数的接口，而公开的跨调用点函数却无任何文档。修复建议：把接口（连同其 JSDoc）移到现有 /** Processes a slash command… */ 块之上，使该块重新挂到 handleSlashCommand，并为其补充 @param sessionHooks。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| const nextTurnKey = queuedTurnKey; | ||
| if ( | ||
| nextTurnKey && | ||
| snapshot.goal?.status === 'active' && | ||
| !pendingProposal && | ||
| !verificationAttempt | ||
| ) { |
There was a problem hiding this comment.
[Suggestion] R4-4: this promotion block is a third inline copy of the queued-turn promotion — the existing promoteQueuedUserTurn() helper performs exactly this transition, and its !currentPermit precondition holds here because this branch just cleared the permit. The copies' guard sets have already drifted: the helper checks no attempt states, finishTurn gates on composite verifying (including checkpointAttempt), this block checks !pendingProposal && !verificationAttempt (omitting checkpointAttempt) — all dead weight, since attempt states are only ever set atomically with clearing currentPermit inside serialized enqueue blocks. — Concrete cost: the next edit to the promotion transition must find and reconcile 3-4 copies; a miss in one reintroduces exactly the strand/deadlock class this PR fixes. Suggested fix: replace the inline block with promoteQueuedUserTurn(); (keep the trailing broadcast() and the !currentPermit continuation guard), matching how settleDanglingAttempt, finishCheckpointCheck, and recordCheckpoint already use it.
中文说明
[Suggestion] R4-4：此提升块是排队轮次提升的第三份内联拷贝——既有的 promoteQueuedUserTurn() 辅助函数执行完全相同的转换，且其 !currentPermit 前置条件在此处成立（本分支刚清除 permit）。各拷贝的守卫集合已经漂移：辅助函数不检查任何 attempt 状态，finishTurn 用复合 verifying（含 checkpointAttempt），本块检查 !pendingProposal && !verificationAttempt（漏掉 checkpointAttempt）——全是死重，因为 attempt 状态只会在序列化 enqueue 块内与清除 currentPermit 原子地设置。— 具体代价：下次修改提升转换时必须找到并调和 3-4 份拷贝；漏改一份就会重新引入本 PR 正在修复的卡死/死锁类别。修复建议：把内联块替换为 promoteQueuedUserTurn();（保留尾部 broadcast() 与 !currentPermit continuation 守卫），与 settleDanglingAttempt、finishCheckpointCheck、recordCheckpoint 的用法一致。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| kind !== 'aborted' && | ||
| // Rejecting 'paused' made every surface keep showing a paused goal as | ||
| // actively running: the card never rendered and the active-goal | ||
| // derivation fell back to the previous 'set' card. | ||
| kind !== 'paused' |
There was a problem hiding this comment.
[Suggestion] R4-5: the new 'paused' goal-status acceptance — here in webui normalizeGoalStatus and in web-shell GoalStatusMessage.tsx VALID_GOAL_KINDS/rendering — has no regression test on either client surface; the only goalStatus transcript tests exercise kind: 'set'. — Failure scenario: reverting either acceptance ships silently — paused cards are dropped, getLatestActiveGoalFromBlocks falls back to the previous set card, and both UIs again show a paused goal as actively running — the exact regression this diff's own comment at this line describes. The runtime half of this change is tested; the display half is not. Suggested fix: add a kind: 'paused' case to the DaemonSessionProvider goalStatus transcript test and a serialize/deserialize round-trip for 'paused' in the web-shell tests; optionally assert GoalStatusMessage renders it inactive.
中文说明
[Suggestion] R4-5：新增的 'paused' goal 状态接受——webui normalizeGoalStatus 此处与 web-shell GoalStatusMessage.tsx 的 VALID_GOAL_KINDS/渲染——在两个客户端界面都没有回归测试；现有的 goalStatus transcript 测试只验证 kind: 'set'。— 失败场景：回退任一接受都会静默上线——paused 卡片被丢弃，getLatestActiveGoalFromBlocks 回落到之前的 set 卡片，两个 UI 再次把暂停的 Goal 显示为运行中——正是此处 diff 注释所描述的回归。本改动的 runtime 半边有测试；显示半边没有。修复建议：在 DaemonSessionProvider 的 goalStatus transcript 测试中加入 kind: 'paused' 用例，并在 web-shell 测试中加入 'paused' 的序列化/反序列化往返；可选断言 GoalStatusMessage 以非活动状态渲染。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| const isRuntimeContinuation = goalTurn?.origin === 'runtime'; | ||
| if ( | ||
| !isContinue && | ||
| !isRuntimeContinuation && |
There was a problem hiding this comment.
[Suggestion] R4-6: the UserPromptSubmit-hook exemption for machine-generated Goal continuations (!isRuntimeContinuation) has no test — mutation-run verified: removing the condition keeps the full 560-test Session.test.ts green, and a probe test (UserPromptSubmit hook enabled + boundGoalHost.startGoalTurn(...)) fails with the mutation and passes with the exemption restored. The exemption is behaviorally load-bearing and covered only by a test that does not exist. — Failure scenario: if the exemption regresses, a UserPromptSubmit hook returning block makes the continuation release before modelStarted, releaseTurn re-queues the identical continuation, and the session spins — no model call, one persisted transcript record per lap — until the goal is paused/cleared; nothing in the suite would fire (the diff's own comment above this line describes this exact spin shape). Suggested fix: add a Session.test.ts case — UserPromptSubmit hook registered (hasHooksForEvent true for it), host-scheduled goal turn runs; assert messageBus.request is never called with UserPromptSubmit and the turn still settles via finishTurn.
中文说明
[Suggestion] R4-6：针对机器生成的 Goal continuation 的 UserPromptSubmit hook 豁免（!isRuntimeContinuation）没有测试——已突变验证：移除该条件后完整的 560 个 Session.test.ts 测试仍全绿；探针测试（启用 UserPromptSubmit hook + boundGoalHost.startGoalTurn(...)）在突变下失败、恢复豁免后通过。该豁免在行为上承重，却只被一个不存在的测试覆盖。— 失败场景：若豁免回归，返回 block 的 UserPromptSubmit hook 会使 continuation 在 modelStarted 之前被 release，releaseTurn 重新排队同一个 continuation，会话空转——无模型调用、每圈持久化一条 transcript 记录——直到 Goal 被 pause/clear；测试套件不会有任何失败（此线上方的 diff 注释描述的正是这种空转形态）。修复建议：新增 Session.test.ts 用例——注册 UserPromptSubmit hook（hasHooksForEvent 对它返回 true），运行 host 调度的 Goal 轮次；断言 messageBus.request 从未以 UserPromptSubmit 被调用，且轮次仍经 finishTurn 结算。
— qwen3.8-max via Qwen Code /review (v0.21.8)
`a810f7e16` (fix(serve): Make session restore timeouts safe and observable, QwenLM#8691) was the only upstream commit to touch `acpAgent.ts` since the merge base, and it collided with this PR in three hunks of `loadSession`/`unstable_resumeSession`. The two sides are orthogonal. Upstream generalized the session-start profiler into `createAcpSessionProfiler` and wrapped the restore stages in it, so `ensureAuthenticated`, `setupFileSystem` and `createAndStoreSession` moved inside `profiler.time(...)` calls and their bodies gained an indentation level. This PR deletes goal restore from those same bodies: `supersedeUnrestorableGoal: true` on the replay collector and both `#restoreGoalOnResume` call sites, because Goal v3 recovers through `getGoalRuntimeReady()` instead of replaying history. Resolved by taking main's profiler structure verbatim and re-applying this PR's three deletions on top, so the merge adds only the profiler wrapping. Against `upstream/main` the file now differs by exactly the four goal-related removals this PR owns, and nothing else. Verified: `npm run build` and `npm run typecheck` both clean, plus the suites covering every file the two sides both touched — `acpAgent.test.ts` + `acpAgent.worktree.test.ts` (393), `Session.test.ts` (570) and `nonInteractiveCli.test.ts` (125), 1088 passed / 1 skipped. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Merge conflict with Cause. Resolution. The two sides are orthogonal, so I took main’s profiler structure verbatim and re-applied this PR’s three deletions on top: Verification.
The remaining two files they both touched, Round 4’s three Criticals (R4-1, R4-2 and the still-standing R3-6) are not addressed here — this push is the conflict resolution only, and they are next. |
Three Criticals from review round 4, all on the resume path. R4-2: with `experimental.sessionWriterLease` on, the recorder starts `inactive` and rejects every write until `activateChatRecording()` hands it the lease — but the Config constructor kicks `runtime.restore()` off immediately. The legacy-migration journal write lands in that guard, `restore()` latches the failure as `recoveryError` for the life of the runtime, the migrated goal is dropped, and every later dispatch/ finishTurn/releaseTurn rethrows. Probe-confirmed before fixing: `getGoalRuntimeReady()` rejected with `GoalPersistenceUnavailableError: Session write ownership could not be verified.` The restore is now deferred while the writer cannot accept writes and run from `activateChatRecording()` once the lease is in hand — against the authoritative transcript activation just loaded, which is fresher than what the constructor had. Paths where the writer never arrives (activation failure, shutdown, a superseding session) settle the deferred restore with that error rather than leaving awaiters pending forever. R4-1: Goal recovery's correction broadcast fires from the Config constructor, before `Session.#bindGoalRuntime` exists to subscribe, and replay streams the pre-migration records — so the newest goal card the client sees is the legacy `set` one and card-derivation clients showed a phantom running goal until a second reload. `GoalRuntime` now retains the cause its restore broadcast carried, and `createAndStoreSession` republishes the recovered state after `replayHistory`, which puts the `migrated -> paused` projection after the replayed card. `#publishGoalState` already de-duplicates on `(cause, snapshot)`, so this is a no-op when the subscription did deliver it. R3-6 (second trigger): when `recoverGoalFromRecords` returns `'unsupported'`, there is no state to publish and no in-session command corrects the stream — a degraded `/goal` answers without a cause, which no client card-derivation reads. That case now emits the same trailing `cleared` card the removed `supersedeUnrestorableGoal` did, emitted and not recorded so a later resume can still restore the goal. Six new tests, each mutation-verified against the regression it names.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Unresolved, please confirm:
- [Critical] R5-4 (thread 3745570376): causeless status cache write-back race — the publication serialization was traced as order-safe by one reviewer and as race-prone by the filing reviewer; could not settle whether a stale snapshot write-back is reachable at this commit.
- [Critical] R5-5 (thread 3745570378): follow-up suggestion generation on intermediate end_turn of Goal continuations — not traced to a concrete trigger at this commit.
- [Critical] R5-7 (thread 3745570381): trust recheck on Goal recovery — no trust gate exists in the recovery code, but session load routes reject untrusted workspaces and the two traces conflict on reachability; the framing is disputed by the author as a product decision.
- [Critical] R5-8 (thread 3745570384): clear preempting a live model stream — the preemption is by design (dispatch invalidates the permit and preempts the host); whether that is a defect is an unresolved design dispute between maintainer and author.
- [Critical] R5-11 (thread 3745570389): Goal drainer starving cron/notification admission races — queue-priority starvation not traced to a concrete outcome at this commit.
- [Critical] R5-14 (thread 3745570398): Goal lineage for mid-turn drained user input — this PR adds permit-carrying recordUserMessage, but the mid-turn drain path's Goal context could not be fully traced.
- [Critical] R5-17 (thread 3745570404): daemon liveness accounting for internal Goal continuations — detached-session reaper interaction not traced at this commit.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not explored to full depth (tool budget reached): chunk 4: none — all checks I intended completed; cross-file consumer tracing of the ext-method response shapes was excluded as out-of-territory per the brief.; PR #8732 adopts the canonical Goal v3 runtime in ACP/Web ...: didn't deeply trace the goal-journal persistence failure mode in finishTurn (the fallback releaseTurn in #settleGoalTurn 's catch could also re-arm a spin in…; PR #8732 adopts the canonical Goal v3 runtime in ACP/Web ...: did not fully trace the Goal *journal* failure mode inside runtime.finishTurn — if the journal persistently fails while model calls succeed, the releaseTurn …; chunk 3: executing acpAgent.test.ts — the review worktree has no node_modules , so running the suite would require a full monorepo install; verification above is stat…; chunk 10: none (all planned checks completed; ~28 of ~50 tool calls used)., and 1 more.
[Critical] R5-1 (thread 3745570368): Rebinding after /clear caches the replacement runtime's empty snapshot but never publishes it, and goalPublicationTail is neither drained nor generation-gated — after a previously active Goal, clients can stay stuck on the old active card, and a stale queued publication from the abandoned runtime can emit into the fresh session. Author-confirmed real in comment 5236411428; mechanism re-verified at this commit (rebindGoalRuntimeForNewSession ~1797-1806 publishes nothing; no generation gate).
[Critical] R5-3 (thread 3745570373): reservedGoalRuntime is captured before the previous-prompt wait; if /clear disposes it mid-wait, claimGoalTurn/releaseTurn throw 'Goal runtime has been disposed' out of prompt() uncaught (before the main try/finally), leaving pendingPrompt and admission listeners installed. Re-verified at this commit (assertOperational throw at goal-runtime.ts ~244-251; no try/catch or runtime-identity recheck around Session.ts ~3511-3547 and ~3628).
[Critical] R5-9 (thread 3745570385): preemptGoalTurn only aborts queued turns and activeGoalTurn (runtime-origin); a user-origin prompt holding a Goal permit keeps running after clear/pause/edit/replace invalidates the permit, allowing obsolete tool and output side effects. Mechanism present at this commit (Session.ts ~1769-1773).
[Critical] R5-12 (thread 3745570393): binding the recovered Goal runtime in the Session constructor can start a live continuation before history replay and recovered-state publication finish — restore() keeps an active v2 goal active and calls queueContinuation (goal-runtime.ts ~921-971), and bindHost flushes the latched continuation in the constructor, so live goal-turn updates can precede replayed history and regress the newest goalState card. Author-acknowledged structural claim in 5236411428; unfixed at this commit.
[Critical] R5-13 (thread 3745570395): if persistence failure makes the Stop-hook-cap pause reject, #pauseGoalForStopHookCap only logs the error; settlement can then release the still-active permit and schedule another continuation, so an always-blocking Stop hook continues beyond the cap. Mechanism present at this commit (Session.ts ~2115-2132: catch warns only, no in-memory fail-closed).
[Critical] R5-19 (thread 3745570405): when finishTurn fails because transcript persistence latched a permanent write error, the releaseTurn fallback in #settleGoalTurn releases the permit while the Goal remains active — release schedules another autonomous turn whose settlement fails identically, an unbounded model/token loop with no durable progress. Mechanism present at this commit (Session.ts ~2086-2104).
[Critical] R5-21 (thread 3745570407): a draining goal continuation enters prompt()'s preamble as an equal contender and unconditionally aborts pendingPrompt (Session.ts ~3444), which can be a concurrently-admitted user prompt's controller — the user message is silently dropped on a resume-ordering race; nothing re-checks ownership after admission at this commit.
[Critical] R5-22 (thread 3745570409): a user prompt can wait on a reserved queuedTurnKey while a concurrent create/edit/replace invalidates that key and schedules a new continuation; claimGoalTurn never re-registers the reservation and pendingPrompt blocks the continuation — both sides deadlock. Single queuedTurnKey slot plus no re-registration confirmed at this commit (goal-runtime.ts ~1011-1017; Session.ts ~3511-3531).
中文说明
未决，请确认：共 7 条（原文未翻译，列表见上方英文部分）。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）：chunk 4：none — all checks I intended completed; cross-file consumer tracing of the ext-method response shapes was excluded as out-of-territory per the brief.；PR #8732 adopts the canonical Goal v3 runtime in ACP/Web ...：didn't deeply trace the goal-journal persistence failure mode in finishTurn (the fallback releaseTurn in #settleGoalTurn 's catch could also re-arm a spin in…；PR #8732 adopts the canonical Goal v3 runtime in ACP/Web ...：did not fully trace the Goal *journal* failure mode inside runtime.finishTurn — if the journal persistently fails while model calls succeed, the releaseTurn …；chunk 3：executing acpAgent.test.ts — the review worktree has no node_modules , so running the suite would require a full monorepo install; verification above is stat…；chunk 10：none (all planned checks completed; ~28 of ~50 tool calls used).，另有 1 条。
[Critical] R5-1 (thread 3745570368): Rebinding after /clear caches the replacement runtime's empty snapshot but never publishes it, and goalPublicationTail is neither drained nor generation-gated — after a previously active Goal, clients can stay stuck on the old active card, and a stale queued publication from the abandoned runtime can emit into the fresh session. Author-confirmed real in comment 5236411428; mechanism re-verified at this commit (rebindGoalRuntimeForNewSession ~1797-1806 publishes nothing; no generation gate).
[Critical] R5-3 (thread 3745570373): reservedGoalRuntime is captured before the previous-prompt wait; if /clear disposes it mid-wait, claimGoalTurn/releaseTurn throw 'Goal runtime has been disposed' out of prompt() uncaught (before the main try/finally), leaving pendingPrompt and admission listeners installed. Re-verified at this commit (assertOperational throw at goal-runtime.ts ~244-251; no try/catch or runtime-identity recheck around Session.ts ~3511-3547 and ~3628).
[Critical] R5-9 (thread 3745570385): preemptGoalTurn only aborts queued turns and activeGoalTurn (runtime-origin); a user-origin prompt holding a Goal permit keeps running after clear/pause/edit/replace invalidates the permit, allowing obsolete tool and output side effects. Mechanism present at this commit (Session.ts ~1769-1773).
[Critical] R5-12 (thread 3745570393): binding the recovered Goal runtime in the Session constructor can start a live continuation before history replay and recovered-state publication finish — restore() keeps an active v2 goal active and calls queueContinuation (goal-runtime.ts ~921-971), and bindHost flushes the latched continuation in the constructor, so live goal-turn updates can precede replayed history and regress the newest goalState card. Author-acknowledged structural claim in 5236411428; unfixed at this commit.
[Critical] R5-13 (thread 3745570395): if persistence failure makes the Stop-hook-cap pause reject, #pauseGoalForStopHookCap only logs the error; settlement can then release the still-active permit and schedule another continuation, so an always-blocking Stop hook continues beyond the cap. Mechanism present at this commit (Session.ts ~2115-2132: catch warns only, no in-memory fail-closed).
[Critical] R5-19 (thread 3745570405): when finishTurn fails because transcript persistence latched a permanent write error, the releaseTurn fallback in #settleGoalTurn releases the permit while the Goal remains active — release schedules another autonomous turn whose settlement fails identically, an unbounded model/token loop with no durable progress. Mechanism present at this commit (Session.ts ~2086-2104).
[Critical] R5-21 (thread 3745570407): a draining goal continuation enters prompt()'s preamble as an equal contender and unconditionally aborts pendingPrompt (Session.ts ~3444), which can be a concurrently-admitted user prompt's controller — the user message is silently dropped on a resume-ordering race; nothing re-checks ownership after admission at this commit.
[Critical] R5-22 (thread 3745570409): a user prompt can wait on a reserved queuedTurnKey while a concurrent create/edit/replace invalidates that key and schedules a new continuation; claimGoalTurn never re-registers the reservation and pendingPrompt blocks the continuation — both sides deadlock. Single queuedTurnKey slot plus no re-registration confirmed at this commit (goal-runtime.ts ~1011-1017; Session.ts ~3511-3531).
— qwen3.8-max via Qwen Code /review (v0.21.9)
| if ( | ||
| error instanceof GoalPersistenceUnavailableError && | ||
| (operation.kind === 'status' || operation.kind === 'clear') | ||
| ) { | ||
| return goalControl(operation, { snapshot: emptyGoalSnapshot() }); | ||
| } |
There was a problem hiding this comment.
[Critical] The degradation branch treats every GoalPersistenceUnavailableError from getGoalRuntimeReady() as "this session has no Goal", but the legacy-migration journal-write-failure producer in restore() (goal-runtime.ts ~949-958) fires while a well-formed legacy goal record still sits in the transcript — so /goal clear reports success without clearing anything, and the Goal resurrects on the next resume. — Failure scenario: resume a session whose newest Goal record is a legacy goal_status card; the migration journal write fails (disk error / latched writeFailure); restore() wraps it as GoalPersistenceUnavailableError and latches recoveryError, appending nothing. /goal clear then hits this branch and returns the empty-snapshot success ("Goal cleared.", exit 0), yet the legacy record survives on disk; on the next --resume, recoverGoalFromRecords re-migrates it and the "cleared" Goal reappears as paused. Distinct from the fixed R5-1 (dispatch failures now surface honestly via the inner catch, commit 8218067f6) and from the accepted chat-recording-disabled design: here persistence exists and still holds the Goal. Fix direction: distinguish "no persistence was ever available" from "restore failed with records present" — e.g. carry a flag on GoalPersistenceUnavailableError (set when restore() latches recoveryError with records on disk), or have Config expose whether the runtime was ever constructed — and return errorMessage(...) for clear (and status) in the restore-failure case.
中文说明
降级分支把来自 getGoalRuntimeReady() 的所有 GoalPersistenceUnavailableError 都当作"本会话没有 Goal"，但 restore()（goal-runtime.ts 约 949-958 行）中 legacy 迁移 journal 写入失败这一产生源触发时，格式完好的 legacy goal 记录仍然留在 transcript 里——因此 /goal clear 会在没有清除任何东西的情况下报告成功，Goal 会在下次恢复时复活。— 失败场景：恢复一个最新 Goal 记录是 legacy goal_status 卡片的会话；迁移 journal 写入失败（磁盘错误 / 锁存的 writeFailure）；restore() 将其包装为 GoalPersistenceUnavailableError 并锁存 recoveryError，没有追加任何记录。此时 /goal clear 命中本分支并返回空快照成功（"Goal cleared."，exit 0），但 legacy 记录仍在磁盘上；下次 --resume 时 recoverGoalFromRecords 重新迁移它，被"清除"的 Goal 以 paused 状态重新出现。与已修复的 R5-1 不同（dispatch 失败现在通过内层 catch 诚实报错，commit 8218067f6），也不同于已接受的关闭聊天记录的设计：此场景下持久化存在且仍持有 Goal。修复方向：区分"从未有持久化"与"恢复失败但记录仍在"——例如在 GoalPersistenceUnavailableError 上携带标志（当 restore() 在磁盘有记录时锁存 recoveryError 时设置），或让 Config 暴露 runtime 是否曾被构建——并在恢复失败场景对 clear（和 status）返回 errorMessage(...)。
— qwen3.8-max via Qwen Code /review (v0.21.9)
There was a problem hiding this comment.
已修复。验证证据：仅在录制服务确实缺失时将 status/clear 降级为空；持久化存在但恢复失败会返回原错误，goalCommand.test.ts 65/65 通过。commit 56d9d4f。
Superseded by later commits through d1bb047. Clearing the stale CHANGES_REQUESTED state; the current head must be reviewed separately.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Unresolved, please confirm:
- [Critical] R5-22 (thread 3745570409): user-prompt deadlock via queuedTurnKey invalidation — round 6 verified the single-slot mechanism present; this round's trace concluded the slot self-heals via claimGoalTurn retry — conflicting evidence, cannot settle at this commit
- [Critical] R5-4 (thread 3745570376): causeless status cache write-back race — reviewer traces conflict; could not settle whether a stale snapshot write-back is reachable at this commit
- [Critical] R5-5 (thread 3745570378): follow-up suggestion generation on intermediate end_turn of Goal continuations — not traced to a concrete trigger this round
- [Critical] R5-7 (thread 3745570381): trust recheck on Goal recovery — no trust gate exists anywhere in the recovery/restore chain (two auditors re-raised it this round), but reachability through daemon route-level trust checks is unresolved and the author disputes the framing as a product decision
- [Critical] R5-8 (thread 3745570384): clear preempting a live model stream — unresolved design dispute between maintainer and author (preemption by design vs truncation defect)
- [Critical] R5-11 (thread 3745570389): Goal drainer starving cron/notification admission races — queue-priority starvation not traced to a concrete outcome at this commit
- [Critical] R5-14 (thread 3745570398): Goal lineage for mid-turn drained user input — the mid-turn drain path's Goal context could not be fully traced this round
- [Critical] R5-17 (thread 3745570404): daemon liveness accounting for internal Goal continuations — detached-session reaper interaction not traced at this commit
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not explored to full depth (tool budget reached): Change under review: PR #8732 adopts the canonical Goal v...: did not read createMigratedGoalState to confirm whether a migrated (projected-as- paused ) goal carries a lastReason ; this is in the legacy-projection path …; Change under review: PR #8732 adopts the canonical Goal v...: did not trace whether the webui daemon itself sets experimental.sessionWriterLease (the finding holds at Config level regardless of which ACP runtime enables …; chunk 7: did not execute the new test — the worktree has no node_modules and I declined a full monorepo install within budget; contract verification was done staticall…; Change under review: PR #8732 adopts the canonical Goal v...: none — all checks above completed within budget.; Change under review: PR #8732 adopts the canonical Goal v...: did not trace the ACP /clear handler ordering (whether it aborts the pending prompt before Config.startNewSession() disposes the runtime) to fully pin Findi…, and 13 more.
中文说明
未决，请确认：共 8 条（原文未翻译，列表见上方英文部分）。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）：Change under review: PR #8732 adopts the canonical Goal v...：did not read createMigratedGoalState to confirm whether a migrated (projected-as- paused ) goal carries a lastReason ; this is in the legacy-projection path …；Change under review: PR #8732 adopts the canonical Goal v...：did not trace whether the webui daemon itself sets experimental.sessionWriterLease (the finding holds at Config level regardless of which ACP runtime enables …；chunk 7：did not execute the new test — the worktree has no node_modules and I declined a full monorepo install within budget; contract verification was done staticall…；Change under review: PR #8732 adopts the canonical Goal v...：none — all checks above completed within budget.；Change under review: PR #8732 adopts the canonical Goal v...：did not trace the ACP /clear handler ordering (whether it aborts the pending prompt before Config.startNewSession() disposes the runtime) to fully pin Findi…，另有 13 条。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| if (!turn.modelStarted) { | ||
| await runtime.releaseTurn(turn.turnKey); |
There was a problem hiding this comment.
[Critical] R7-3: A user cancel that lands before modelStarted settles via this unconditional releaseTurn branch, and releaseTurn re-queues a continuation — the autonomous Goal loop keeps running despite the cancel, while the same cancel one await later pauses the goal (timing-dependent persisted outcome for the identical user action — the exact class the supersededByNewPrompt comment condemns for NEW_PROMPT_ABORT_REASON). — Failure scenario: session/cancel while a runtime Goal turn is in its pre-send assembly window (assertCanStartTurn / recordGoalRuntimeMessage / snapshot / reminders, all before modelStarted): the turn returns cancelled with modelStarted === false, this branch never consults USER_CANCEL_ABORT_REASON, and the runtime mints the next continuation. Probe-verified against the real createGoalRuntime: releaseTurn re-minted a continuation (activity: 'running'), while the post-modelStarted pause path settled to 'paused'.
| if (!turn.modelStarted) { | |
| await runtime.releaseTurn(turn.turnKey); | |
| if (!turn.modelStarted) { | |
| const cancelledByUser = | |
| turn.controller.signal.reason === USER_CANCEL_ABORT_REASON; | |
| if (cancelledByUser) { | |
| await this.#pauseGoalOnCancel(turn); | |
| return; | |
| } | |
| await runtime.releaseTurn(turn.turnKey); |
中文说明
[Critical] R7-3：在 modelStarted 之前到达的用户取消会走到这个无条件 releaseTurn 分支，而 releaseTurn 会重新排队一个 continuation——尽管用户已取消，自动 Goal 循环仍继续运行；同样的取消晚一个 await 到达时却会 pause Goal（同一用户行为因时机不同产生不同的持久化结果——正是 supersededByNewPrompt 注释为 NEW_PROMPT_ABORT_REASON 所谴责的那类问题）。— 失败场景：runtime Goal 回合处于发送前组装窗口（assertCanStartTurn / recordGoalRuntimeMessage / snapshot / reminders，均在 modelStarted 之前）时用户 session/cancel：回合以 modelStarted === false 返回 cancelled，本分支从不检查 USER_CANCEL_ABORT_REASON，runtime 随即铸造下一个 continuation。已用真实 createGoalRuntime 探针验证：releaseTurn 重新铸造了 continuation（activity: 'running'），而 modelStarted 之后的 pause 路径正常落为 'paused'。
— qwen3.8-max via Qwen Code /review (v0.21.9)
There was a problem hiding this comment.
已修复 + 验证证据：3c8c8481e4 在 model request 前收到用户取消时改为 pause 当前 Goal，不再 release 后重启 continuation，且不计 finishTurn。Node 22 下 Session.test.ts 584/584、CLI build/typecheck、ESLint、Prettier 均通过。
| replayUpdates = replayUpdates.concat( | ||
| await createdSession.renderRecoveredGoalUpdates(records), |
There was a problem hiding this comment.
[Critical] R7-6: The recovered-goal correction is wired into all three cold restore paths (this bulk path, the resume path, and createAndStoreSession) but not into the live-session branch of loadSession (~4854-4935) — a resident session whose Goal runtime is unavailable replays its unterminated legacy set card with nothing superseding it. — Failure scenario: a session whose runtime is unavailable (general.chatRecording: false, or a latched recoveryError from a malformed goal_state record) whose transcript ends on an unterminated legacy set card; the session is resident and the client reloads, so loadSession takes the live branch. Replay re-emits the set card; no correction exists on that branch and none can ever publish (runtime gone); web-shell derives the live goal from the newest card and shows autonomous work running indefinitely for a goal nothing drives. Healthy migrated sessions escape via the persisted paused card; only unavailable-runtime sessions are exposed.
| replayUpdates = replayUpdates.concat( | |
| await createdSession.renderRecoveredGoalUpdates(records), | |
| replayUpdates = replayUpdates.concat( | |
| await createdSession.renderRecoveredGoalUpdates(records), | |
| ); | |
| // TODO(R7-6): mirror this correction on the live-session branch of | |
| // loadSession (~4854-4935), which currently replays without it. |
中文说明
[Critical] R7-6：恢复后 Goal 状态的纠正逻辑接入了全部三条冷恢复路径（本 bulk 路径、resume 路径和 createAndStoreSession），但没有接入 loadSession 的 live-session 分支（约 4854-4935）——Goal runtime 不可用的驻留会话会重新回放其未终结的 legacy set 卡片，且没有任何东西取代它。— 失败场景：runtime 不可用（general.chatRecording: false，或畸形 goal_state 记录导致锁存的 recoveryError）且 transcript 以未终结的 legacy set 卡片结尾的会话；会话驻留在 daemon 中，客户端重新加载，loadSession 走 live 分支。回放重新发出该 set 卡片；该分支上不存在纠正调用，且永远不会有发布（runtime 已不可用）；web-shell 从最新卡片推导活动 Goal，于是显示一个没有任何东西驱动的 Goal 在无限运行。健康迁移的会话通过持久化的 paused 卡片逃脱；只有 runtime 不可用的会话暴露。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| if (goalTurn) { | ||
| await this.#settleGoalTurn(goalTurn, promptResult, promptFailed); |
There was a problem hiding this comment.
[Critical] R7-7: #settleGoalTurn is awaited inside prompt()'s finally, and finishTurn itself awaits the verifier and/or checkpoint model calls — so the session/prompt RPC is withheld for the entire background verification (seconds-to-tens-of-seconds of model time after the stream ended). — Failure scenario: a Goal turn completes with a terminal proposal: finishTurn awaits runVerification (a full verifier model call), potentially chained into runCheckpoint (a second one). During that window pendingPromptCompletion stays set (the next prompt and all drains block), #hasActiveTurn() is true, yet releasePendingSend() already ran — so cancelPendingPrompt throws "Not currently generating": a busy turn that cannot be cancelled and a composer blocked on a completed turn.
| if (goalTurn) { | |
| await this.#settleGoalTurn(goalTurn, promptResult, promptFailed); | |
| if (goalTurn) { | |
| void this.#settleGoalTurn(goalTurn, promptResult, promptFailed); |
中文说明
[Critical] R7-7：#settleGoalTurn 在 prompt() 的 finally 中被 await，而 finishTurn 本身会 await 验证器（verifier）和/或检查点（checkpoint）模型调用——因此 session/prompt RPC 会被整个后台验证过程扣住（流结束后仍有数秒到数十秒的模型时间）。— 失败场景：Goal 回合以终局提案正常结束：finishTurn await runVerification（一次完整的 verifier 模型调用），还可能链式 await runCheckpoint（第二次）。该窗口内 pendingPromptCompletion 保持已设置（下一个 prompt 和所有 drain 被阻塞），#hasActiveTurn() 为 true，但 releasePendingSend() 已经执行——于是 cancelPendingPrompt 抛出 "Not currently generating"：一个无法取消的忙碌回合，输入框被一个已完成的回合阻塞。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| // failure as `recoveryError` for the life of the runtime — the | ||
| // migrated goal is dropped and goal persistence is bricked for the | ||
| // whole resumed session. Wait for the writer instead. | ||
| if (this.sessionWriterLeaseEnabled && !recorder.hasWriteOwnership()) { |
There was a problem hiding this comment.
[Critical] R7-11: A goal restore deferred under the session-writer lease is never started after Config.startNewSession(): startPendingGoalRestore() is called only from activateChatRecording() inside initializeOnce() — once per Config lifetime, already past by the time /clear runs — so the replacement session's getGoalRuntimeReady() stays pending forever. — Failure scenario: experimental.sessionWriterLease on; the recorder loses its writer lease mid-session (the "lost session writer" state this PR's dispatch wrapper degrades for), so the guard lets /clear through startNewSession(); initializeGoalRuntime takes this deferred branch for the fresh inactive recorder, creating a pendingGoalRestore that nothing ever starts. /goal status|clear|pause hang forever, sessionGoalGet/sessionGoalClear hang (their catches handle rejection, not pending), and TUI /resume//branch hang in waitForGoalRuntime — until shutdown. Pre-PR this path restored immediately (restore([]) resolved at once). The flag is Experimental (default false), but lease takeover is the feature's own raison d'être and the outcome is a permanent functional hang, not degradation.
| if (this.sessionWriterLeaseEnabled && !recorder.hasWriteOwnership()) { | |
| if ( | |
| this.sessionWriterLeaseEnabled && | |
| !recorder.hasWriteOwnership() && | |
| !this.initializationSettled | |
| ) { |
中文说明
[Critical] R7-11：在 session-writer lease 下被延迟的 Goal 恢复，在 Config.startNewSession() 之后永远不会被启动：startPendingGoalRestore() 只在 initializeOnce() 内的 activateChatRecording() 中调用——每个 Config 生命周期仅一次，/clear 执行时早已过去——因此替代会话的 getGoalRuntimeReady() 永远保持 pending。— 失败场景：experimental.sessionWriterLease 开启；录制器在会话中途失去 writer lease（本 PR 的 dispatch 包装器专门为其降级的 "lost session writer" 状态），守卫放行 /clear 进入 startNewSession()；initializeGoalRuntime 对新的未激活录制器走这个延迟分支，创建了一个没有任何代码会启动的 pendingGoalRestore。/goal status|clear|pause 永远挂起，sessionGoalGet/sessionGoalClear 挂起（它们的 catch 只处理 rejection，不处理 pending），TUI 的 /resume//branch 挂在 waitForGoalRuntime——直到 shutdown。改动前该路径立即恢复（restore([]) 立刻 resolve）。该标志是 Experimental（默认 false），但 lease 接管正是该功能存在的理由，且后果是永久性功能挂起而非降级。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| const active = findGoalToRestore( | ||
| collectGoalStatusItemsFromRecords(replayedRecords), |
There was a problem hiding this comment.
[Critical] R7-16: #unrestorableGoalStatus scans only legacy slash_command goal cards — collectGoalStatusItemsFromRecords skips records with subtype !== 'slash_command' — but this PR persists goal state as v2 system/goal_state records that replay re-emits as active cards; on a v3-era transcript whose recovery is unavailable, no correction card is produced. — Failure scenario: probe-verified against the unmodified PR — renderRecoveredGoalUpdates given an active v2 goal_state record with getGoalRuntimeReady rejecting GoalPersistenceUnavailableError returned [] (no correction card), while the identical scenario with a legacy slash_command record emits the superseding cleared card. Replay re-emits the v2 record as a set/checking card; recovery is unavailable (recording disabled after records were written, or a latched recoveryError); the client's newest goal card stays active — phantom running goal. Even /goal clear cannot correct the stream: the degraded goal_control result carries no cause and produces no goalStatus projection.
| const active = findGoalToRestore( | |
| collectGoalStatusItemsFromRecords(replayedRecords), | |
| const active = findGoalToRestore( | |
| collectGoalStatusItemsFromRecords(replayedRecords), | |
| ) ?? this.#findUnrestorableV2Goal(replayedRecords); |
中文说明
[Critical] R7-16：#unrestorableGoalStatus 只扫描 legacy slash_command Goal 卡片——collectGoalStatusItemsFromRecords 跳过 subtype !== 'slash_command' 的记录——但本 PR 把 Goal 状态持久化为 v2 system/goal_state 记录，回放会把它们重新发为活动卡片；在恢复不可用的 v3 时代 transcript 上，不会产生任何纠正卡片。— 失败场景：已在未改动的 PR 上探针验证——给定活动 v2 goal_state 记录且 getGoalRuntimeReady 以 GoalPersistenceUnavailableError reject 时，renderRecoveredGoalUpdates 返回 []（无纠正卡片），而同样场景下 legacy slash_command 记录会发出取代性的 cleared 卡片。回放把 v2 记录重新发为 set/checking 卡片；恢复不可用（记录写入后关闭了录制，或锁存的 recoveryError）；客户端最新的 Goal 卡片保持活动——幽灵运行中的 Goal。即使 /goal clear 也无法纠正卡片流：降级的 goal_control 结果不带 cause，不产生 goalStatus 投影。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| // the next reload. Never fatal — a session that cannot publish its | ||
| // goal state must still open. | ||
| try { | ||
| await session.publishRecoveredGoalState( |
There was a problem hiding this comment.
[Suggestion] R7-5: The recovered-goal publication on this streamed (non-bulk) loadSession path has no test assertion — the bulk render and the resume publish are both asserted in acpAgent.test.ts, this one is not. — Concrete cost: mutation probe — deleting this call keeps the entire acpAgent.test.ts suite green (392/392 passed), and a web-shell primary (streamed) load of a pre-migration transcript again replays the legacy set card as newest with nothing correcting it: the phantom-running-goal regression this PR exists to fix, silently reintroducible by any future refactor of createAndStoreSession.
| await session.publishRecoveredGoalState( | |
| await session.publishRecoveredGoalState( | |
| sessionData.conversation.messages, | |
| ); | |
| // Asserted in acpAgent.test.ts (streamed-load recovery publication). |
中文说明
[Suggestion] R7-5：这条 streamed（非 bulk）loadSession 路径上的恢复后 Goal 状态发布没有任何测试断言——bulk 渲染和 resume 发布在 acpAgent.test.ts 中都有断言，唯独这里没有。— 具体代价：变异探针——删除此调用后整个 acpAgent.test.ts 套件仍然全绿（392/392 通过），而 web-shell 主（streamed）加载迁移前 transcript 时会再次把 legacy set 卡片回放为最新卡片且无任何纠正：本 PR 要修复的幽灵运行中 Goal 回归，可被未来对 createAndStoreSession 的任何重构静默重新引入。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| // open. | ||
| try { | ||
| replayUpdates = replayUpdates.concat( | ||
| await createdSession.renderRecoveredGoalUpdates(records), |
There was a problem hiding this comment.
[Suggestion] R7-8: The replacement supersede scans the caller's FULL record list while replay renders only the paged/visible subset — with recovery unavailable, a last active legacy goal card dropped by paging still yields a cleared card naming a goal the client never saw. — Concrete cost: bulk loadSession replays selectRecentHistoryRecords(visibleRecords, replayPageSize) (web-shell page size 100), but renderRecoveredGoalUpdates(records) receives the full sessionData.conversation.messages; if goal recovery fails and the transcript's last active legacy goal card sits in the dropped portion, #unrestorableGoalStatus still finds it and appends an orphan cleared card — the removed replay-time supersede operated on exactly the records it replayed. Pass the actually-replayed records, or intersect #unrestorableGoalStatus with what replay rendered.
中文说明
[Suggestion] R7-8：替代的 supersede 扫描调用方的完整记录列表，而回放只渲染分页/可见的子集——在恢复不可用时，被分页丢掉的最后一张活动 legacy Goal 卡片仍会生成一张 cleared 卡片，指向客户端从未见过的 Goal。— 具体代价：bulk loadSession 回放 selectRecentHistoryRecords(visibleRecords, replayPageSize)（web-shell 页大小 100），但 renderRecoveredGoalUpdates(records) 收到的是完整的 sessionData.conversation.messages；若 Goal 恢复失败且 transcript 最后一张活动 legacy Goal 卡片位于被丢弃的部分，#unrestorableGoalStatus 仍会找到它并追加一张孤立的 cleared 卡片——被移除的回放时 supersede 只对它实际回放的记录操作。应传入实际回放的记录，或让 #unrestorableGoalStatus 与回放渲染的内容求交集。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| // stays newest and every surface keeps claiming autonomous work is under way. | ||
| // Kept in step with `GOAL_STATUS_KINDS`, which the daemon-side reader | ||
| // (`parseGoalStatusItem`) validates the same on-disk cards against. | ||
| 'paused', |
There was a problem hiding this comment.
[Suggestion] R7-12: The legacy goal-card kind allowlist is maintained as four unlinked manual copies — cli GOAL_STATUS_KINDS (ui/types.ts), acp-bridge TRANSCRIPT_GOAL_STATUS_KINDS (here), web-shell VALID_GOAL_KINDS (GoalStatusMessage.tsx), and webui normalizeGoalStatus (DaemonSessionProvider.tsx) — guarded only by prose comments. — Concrete cost: the drift this PR repairs already happened once — 'paused' existed in the canonical cli list while missing from the other three copies, which is precisely the phantom-running-goal bug being fixed here (dropped paused card → older set card stays newest → every surface claims autonomous work is under way). When the next kind is added, missing one copy silently regresses replay or rendering with no compiler or test signal; no cross-package equality test exists (GoalStatusMessage.test.tsx pins only the cli list). Export a shared legacy-kinds list from core and consume it at all four sites, or add a drift test.
中文说明
[Suggestion] R7-12：legacy Goal 卡片类型允许列表以四份互不关联的手工副本维护——cli GOAL_STATUS_KINDS（ui/types.ts）、acp-bridge TRANSCRIPT_GOAL_STATUS_KINDS（此处）、web-shell VALID_GOAL_KINDS（GoalStatusMessage.tsx）、webui normalizeGoalStatus（DaemonSessionProvider.tsx）——仅靠注释守护。— 具体代价：本 PR 修复的漂移已经发生过一次——'paused' 存在于 cli 规范列表却缺失于其他三份副本，这正是此处要修复的幽灵运行中 Goal 缺陷（暂停卡片被丢弃 → 旧 set 卡片保持最新 → 所有界面声称自动工作在进行）。下次新增类型时，漏改任一副本都会在没有编译器或测试信号的情况下静默回归回放或渲染；目前不存在跨包一致性测试（GoalStatusMessage.test.tsx 只钉住 cli 列表）。应从 core 导出共享的 legacy 类型列表并在四处消费，或添加漂移测试。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| // replays nothing, so there is no envelope this card could | ||
| // sort ahead of. Never fatal. | ||
| try { | ||
| await createdSession.publishRecoveredGoalState( |
There was a problem hiding this comment.
[Suggestion] R7-14: The recovered-state publication fires for every recovery cause, including sessions whose transcript already ends on the v2 goal_state record that replay just re-emitted — every load of a v3-era goal session appends a duplicate goal card (degraded for cleared/terminal states). — Concrete cost: recoverGoalFromRecords returns the last goal_state record regardless of terminality, so getRecoveryCause() is truthy after an ordinary clear/complete; replay already re-emitted that record's card, and lastGoalPublicationKey is only set by runtime publications, so nothing dedupes the duplicate. For a cleared goal the second card is degraded — empty condition, no iterations/duration (previousGoal captured from the post-clear snapshot). Resume is clean (replays nothing); the duplicate belongs to the load paths (this hook and the bulk/streamed sites). Restrict the publication to cause 'migrated' (keeping the unsupported/degraded supersede branch), or skip when the recovered snapshot matches the last replayed goal_state record.
中文说明
[Suggestion] R7-14：恢复状态的发布对每个恢复 cause 都会触发，包括 transcript 已经以回放刚刚重新发出的 v2 goal_state 记录结尾的会话——v3 时代 Goal 会话的每次加载都会追加一张重复的 Goal 卡片（cleared/终态时还是退化的）。— 具体代价：recoverGoalFromRecords 无论终态与否都返回最后一条 goal_state 记录，因此普通 clear/complete 之后 getRecoveryCause() 仍为真；回放已经重新发出了该记录的卡片，而 lastGoalPublicationKey 只由 runtime 发布设置，没有任何东西能去重。对 cleared 的 Goal，第二张卡片是退化的——空 condition、无 iterations/duration（previousGoal 取自 clear 之后的快照）。resume 路径干净（不回放任何东西）；重复属于加载路径（本 hook 与 bulk/streamed 调用点）。应将发布限制为 cause 'migrated'（保留 unsupported/降级的 supersede 分支），或在恢复快照与最后一条回放的 goal_state 记录一致时跳过。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| status.kind === 'aborted' || | ||
| status.kind === 'paused') && |
There was a problem hiding this comment.
[Suggestion] R7-18: Every live goal card emitted through buildGoalStateUpdate carries durationMs: activeTimeMs (0 at set time), and the stats block above renders durationMs for every non-checking kind guarded only by typeof status.durationMs === 'number' — so every goal-set card (and every reload's v2 replay re-projection) shows a bogus "Goal set · 0s" subtitle the legacy cards never had. — Concrete cost: projectGoalStateToLegacy stamps durationMs unconditionally (this PR's own Session.test.ts asserts durationMs: 0 on the set card), and formatRuntime(0) returns "0s"; iterations is guarded by > 0 but durationMs is not — the asymmetry reads as an oversight. Purely visual, recurs on every reload.
| status.kind === 'aborted' || | |
| status.kind === 'paused') && | |
| status.kind === 'aborted' || | |
| status.kind === 'paused') && |
中文说明
[Suggestion] R7-18：通过 buildGoalStateUpdate 发出的每张实时 Goal 卡片都带 durationMs: activeTimeMs（set 时为 0），上方的统计块对每个非 checking 类型都渲染 durationMs，且只以 typeof status.durationMs === 'number' 为门——因此每张 Goal set 卡片（以及每次重新加载时 v2 回放的重新投影）都会显示一个 legacy 卡片从未有过的假 "Goal set · 0s" 副标题。— 具体代价：projectGoalStateToLegacy 无条件打上 durationMs（本 PR 自己的 Session.test.ts 断言 set 卡片 durationMs: 0），formatRuntime(0) 返回 "0s"；iterations 有 > 0 门而 durationMs 没有——这种不对称读起来像疏漏。纯视觉问题，每次重新加载都会重现。
— qwen3.8-max via Qwen Code /review (v0.21.9)
已被后续 commit 取代，当前 head 需重新 review
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. Not reviewed: reverse audit — no auditor was launched with a prompt this skill builds — the pass that hunts what the rest of the review missed ran, if at all, without the method its brief carries.
中文说明
已审查。 建议见行内评论。 未审查：反向审计——没有审计 agent 是用本 skill 构建的 prompt 启动的——负责搜寻评审其余部分遗漏问题的这道工序，即便运行过，也缺失了 brief 承载的方法。
— deepseek-v4-flash via Qwen Code /review (v0.21.8)
| kind !== 'paused' | ||
| ) { | ||
| return null; |
There was a problem hiding this comment.
[Suggestion] normalizeGoalStatus has no test for 'paused' kind — The 'paused' kind was added to the normalizeGoalStatus allowlist, but the function has no test coverage. A mutation that removes 'paused' from this allowlist would not be caught, and paused goal cards would be rejected as unknown, showing a phantom running goal.
Failure scenario: A future change removes 'paused' from the kind filter, paused goal cards are rejected as unknown, and the active-goal derivation falls back to the previous 'set' card. No test fails.
Suggested fix: Add a test covering normalizeGoalStatus with kind: 'paused' asserting it returns the normalized record, not null.
— deepseek-v4-flash via Qwen Code /review (v0.21.8)
中文说明
[Suggestion] normalizeGoalStatus 缺少对 'paused' 类型的测试——'paused' 类型已被加入 normalizeGoalStatus 的允许列表，但该函数没有任何测试覆盖。如果未来的修改将 'paused' 从此允许列表中移除，不会有任何测试失败，已暂停的 Goal 卡片会被当作未知类型拒绝，从而显示一个幽灵运行中的 Goal。
失败场景： 未来修改移除了 'paused' 类型过滤器，已暂停的 Goal 卡片被拒绝为未知类型，活动 Goal 推导回退到之前的 'set' 卡片。没有测试会失败。
修复建议： 添加一个测试，覆盖 normalizeGoalStatus 在 kind: 'paused' 时返回规范记录而非 null。
— deepseek-v4-flash via Qwen Code /review (v0.21.8)
There was a problem hiding this comment.
已修复。验证证据：临时移除 paused allowlist 后新增回归按预期失败；恢复后 focused case 1/1、完整 DaemonSessionProvider.test.tsx 200/200 通过，ESLint、Prettier、git diff --check 通过。
|
Qwen Code review timed out. Qwen review timed out after 21600 seconds (of the 360-minute budget). This run already used the maximum 360 minute timeout. See workflow logs. |
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
LGTM. The current head addresses the critical findings from prior review rounds, and the implementation is well-structured with comprehensive test coverage. The only CI failure is a transient network issue (shellcheck download), not a code problem.
|
Released in v0.21.11. |
6 participants
What this PR does
This replaces the ACP/Web Shell session's legacy Stop-hook implementation of
/goalwith the canonical Goal v3 runtime already used by the CLI. ACP sessions now support create, status, edit, pause, resume, replace, and clear through one persisted state machine, publish the canonical snapshot while retaining the existing card projection for compatibility, and execute both automatic continuations and real user input with Goal turn permits.The session scheduler now serializes Goal turns with ordinary prompts, cron work, and background notifications. Real user input takes priority over an automatic Goal continuation, while every automatic queue wakes the others when it settles. Goal history is recorded with its actual runtime provenance, and transcript replay no longer rewrites persisted Goal state according to the removed Stop-hook restore gates.
Why it's needed
ACP previously had a separate
/goallifecycle from the CLI. That caused duplicated live cards, duplicate synthetic user records, inconsistent restore behavior, missing pause/edit/resume controls, and queue races where a Goal could remain dormant after another automatic turn completed. It also made the Web Shell and CLI disagree about what an active Goal meant.Using the same canonical runtime gives all surfaces one objective, revision, permit, verification, persistence, and terminal-state contract. The compatibility projection keeps current hosts working while the Web Shell UI moves to the canonical snapshot in a follow-up PR.
Reviewer Test Plan
How to verify
Start a fresh ACP/Web Shell session and submit
/goal 回复 E2E_GOAL，直到我输入 E2E_STOPas the first message. Confirm that exactly one active Goal card appears, that automatic continuations proceed without duplicate user messages, and that status, edit, pause, resume, and clear reflect the same persisted Goal state. While an automatic continuation is running, submit an ordinary user message and confirm that it preempts the automatic turn and becomes the next permitted Goal turn. Resume the session and confirm that replay preserves the canonical state without appending a legacy “not restored” clear event.The automated regression suite also exercises a Goal queued behind a background notification and verifies that it starts immediately after the notification settles, along with publication ordering, single-publication mutation behavior, provenance, user preemption, compatibility projection, and route behavior.
Evidence (Before & After)
Before: creating a Goal could display the same live state twice, and automatic continuations were persisted once as Goal runtime work and again as ordinary user input. A Goal queued during a background notification could remain dormant.
After: browser E2E shows a single active Goal card; the persisted transcript contains one runtime-provenance continuation; ordinary user input safely takes the next Goal permit; and the scheduler regression confirms notification-to-Goal wake-up. A separate E2E report with screenshots will be posted as a PR comment.
Tested on
Environment (optional)
Local bundled Web Shell over ACP with no sandbox, plus focused Vitest coverage.
npm run build,npm run bundle,npm run typecheck, andnpm run lintpass locally.Risk & Scope
Linked Issues
N/A
中文说明
本 PR 做了什么
本 PR 将 ACP/Web Shell 会话中的
/goal从旧 Stop hook 实现切换到 CLI 已使用的 canonical Goal v3 runtime。ACP 会话现在通过同一个持久化状态机支持创建、查询、编辑、暂停、恢复、替换和清除；对外发布 canonical snapshot，同时保留现有卡片所需的兼容投影；自动续跑与真实用户输入都使用 Goal turn permit 执行。会话调度器现在会将 Goal 轮次、普通 prompt、cron 和后台通知串行化。真实用户输入优先于自动 Goal continuation，各类自动队列在结束时会互相唤醒。Goal 历史按真实 runtime 来源记录，历史回放也不再根据已移除的 Stop hook 恢复条件改写持久化 Goal 状态。
为什么需要
ACP 原先拥有一套与 CLI 不同的
/goal生命周期，会造成实时卡片重复、合成用户消息重复记录、恢复语义不一致、缺少暂停/编辑/恢复控制，以及 Goal 在其他自动轮次结束后无法继续的队列竞争。它也导致 Web Shell 与 CLI 对“活动 Goal”的含义不一致。共享 canonical runtime 后，所有界面使用同一套 objective、revision、permit、verification、persistence 和终态契约。兼容投影保证现有 host 继续工作，Web Shell UI 会在后续 PR 中正式消费 canonical snapshot。
Reviewer Test Plan
如何验证
启动一个全新的 ACP/Web Shell 会话，并把
/goal 回复 E2E_GOAL，直到我输入 E2E_STOP作为第一条消息发送。确认只出现一张活动 Goal 卡片，自动续跑不会产生重复用户消息，status、edit、pause、resume 和 clear 都反映同一个持久化 Goal 状态。自动 continuation 执行期间发送普通用户消息，确认它会抢占自动轮次并成为下一个获得 permit 的 Goal 轮次。恢复会话后，确认回放保留 canonical 状态，不会追加旧式“无法恢复”的 clear 事件。自动化回归还覆盖了 Goal 排在后台通知之后的场景，并验证通知结束后 Goal 会立即启动；同时覆盖发布顺序、mutation 单次发布、来源标记、用户抢占、兼容投影和路由行为。
证据（Before & After）
Before：创建 Goal 可能重复显示相同的实时状态；自动 continuation 会同时按 Goal runtime 工作和普通用户输入各记录一次；后台通知期间排入的 Goal 可能一直休眠。
After：浏览器 E2E 只显示一张活动 Goal 卡片；持久化会话只包含一条 runtime 来源的 continuation；普通用户输入能安全获得下一个 Goal permit；调度器回归测试确认后台通知结束后会唤醒 Goal。PR 创建后会另发一条包含截图的 E2E 报告评论。
测试平台
环境（可选）
本地 bundled Web Shell + ACP、无 sandbox，并运行聚焦 Vitest。
npm run build、npm run bundle、npm run typecheck和npm run lint均通过。风险与范围
关联 Issue
N/A