feat(cli,web-shell): persist goal status in daemon transcript events - #5098
Conversation
Previously /goal state lived only in frontend memory — page refresh or
multi-device sessions lost the active goal. Now the CLI emits goal status
updates as structured daemon events (_meta.goalStatus), which flow through
the transcript as status blocks (source: 'goal', data: {...}). The web-shell
rebuilds goal state from transcript blocks on connect, making goal status
survivable across page refreshes and syncable across devices.
- CLI: emitGoalStatus on goal set/clear, pass outputHistoryItems through
nonInteractiveCliCommands, add setAt to goalCommand output
- SDK: widen DaemonUiStatusEvent source/data types, preserve them in
transcript blocks
- webui: normalize _meta.goalStatus in DaemonSessionProvider, replace
sentinel-prefix text encoding with structured data
- web-shell: derive activeGoal from transcript blocks (getLatestActiveGoalFromBlocks),
remove optimistic client-side goal dispatch, parse structured goal data
in GoalStatusMessage/SystemMessage
- Tests: cover emitGoalStatus, outputHistoryItems passthrough, transcript
block serialization, DaemonSessionProvider event conversion
- Also: harden McpDialog restart result type check with isRestartEntriesResult
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
No correctness issues found. Downgraded from Approve to Comment: CI still running.
Reviewed the goal-status persistence refactor (sentinel-text → structured source:'goal'+data blocks). Traced the kind:'checking' divergence definitively: it is produced only in the interactive Ink TUI (useGeminiStream.ts handleStopHookLoopEvent), never via either daemon emit path (acpAgent.ts sessionGoalClear → cleared, or Session.#emitGoalStatusItems → only set/cleared from goalCommand's captured outputHistoryItems). Terminal kinds reach the daemon through the separate _meta.goalTerminal channel. The daemon-side suppression of per-iteration checking is intentional and documented. Backward-compat reading of old sentinel-text blocks still works via parseGoalStatusMessage(text). appendStatusBlock copying source/data for status/debug is safe — no other status/debug producer sets those fields (NormalizedEventBase excludes them; the only producer is createGoalStatusUiEvent). tsc clean on changed files; all changed-package tests pass. — claude-opus-4-8[1m] via Qwen Code /qreview
| store.appendLocalUserMessage(text); | ||
| dispatchGoalSet(optimisticGoal.condition, optimisticGoal.setAt); | ||
| if (!sendToDaemon) { | ||
| dispatchGoalSet(goalArg, Date.now()); |
There was a problem hiding this comment.
[Suggestion] The daemon-path /goal indicator lost its optimistic update. Previously the goal pill was set instantly (client-optimistic) on every path; this PR keeps the optimistic dispatchGoalSet only inside the !sendToDaemon branch. For the daemon path now:
- With a matched daemon, the pill stays blank from keypress until the daemon round-trips a
goalStatusblock (a short lag — thesetis emitted during slash-command processing — but no longer instant). - With an older daemon that doesn't emit
goalStatus(version skew between the web bundle and the daemon binary), the pill never appears, because the old daemon never emitted goal-set — the indicator was purely client-optimistic before this PR.
Since getLatestActiveGoalFromBlocks reconciles the active goal by condition+setAt, you can keep the optimistic dispatch on the daemon path too and let the transcript-derived effect confirm/replace it:
store.appendLocalUserMessage(text);
dispatchGoalSet(goalArg, Date.now()); // optimistic; reconciled by the [blocks] effect
if (!sendToDaemon) return true;
sendPrompt(text, images, { optimisticUserMessage: false }).catch(...);If the round-trip-only behavior is intentional, the version-skew "pill never appears" case is still worth guarding (e.g. a daemon capability check).
— claude-opus-4-8[1m] via Qwen Code /qreview
| }); | ||
| }; | ||
| return ( | ||
| sessionActions.sendPrompt as ( |
There was a problem hiding this comment.
[Suggestion] This SendPromptOptionsWithRetry cast (and the interface at line 184) is unnecessary and silences a real type check. The actual SendPromptOptions from the daemon SDK already declares retry, images, and optimisticUserMessage (webui/src/daemon/session/types.ts:215), and pre-PR this call passed { images, optimisticUserMessage, retry } directly with no cast and compiled — so nothing required the cast. Worse, SendPromptOptionsWithRetry.images is typed PromptImage[] while the real option is DaemonPromptImage[], so the cast forces the local type and suppresses the PromptImage → DaemonPromptImage assignability check here (future drift between the two would go unnoticed).
Drop the interface and cast, restore the direct call:
return sessionActions.sendPrompt(text, {
images,
optimisticUserMessage: opts?.optimisticUserMessage,
retry: opts?.retry,
});— claude-opus-4-8[1m] via Qwen Code /qreview
| if (!isRecord(value)) return null; | ||
| const kind = getString(value, 'kind'); | ||
| if ( | ||
| kind !== 'set' && |
There was a problem hiding this comment.
[Suggestion] This validator accepts set | cleared | achieved | failed | aborted but omits 'checking', whereas GoalStatusKind, GoalStatusMessage.VALID_GOAL_KINDS, and App.getLatestActiveGoalFromBlocks all include 'checking'. So the goal-kind allow-list is now encoded in 4 places that disagree.
It's latent today (nothing emits a checking goalStatus to the daemon — the TUI's per-iteration checking is interactive-only and the daemon represents continuation via the suppressed stopHookLoop). But Session.#emitGoalStatusItems forwards any item.kind (typed as the full 6-kind union) into this 5-kind receiver without validation, so the moment a checking goal-status item flows through it, the daemon silently drops it (no block, no log) while the consumers are coded to render it.
Consider a single shared GoalStatusKind allow-list consumed by all four sites; if dropping checking here is deliberate, express it as "all kinds minus checking" with a comment pointing at the stopHookLoop suppression, and add an exhaustiveness guard so adding a kind forces a compile error at this gate. A debug log on the dropped-kind path would surface future mismatches.
— claude-opus-4-8[1m] via Qwen Code /qreview
| data?: unknown; | ||
| }; | ||
|
|
||
| function getLatestActiveGoalFromBlocks( |
There was a problem hiding this comment.
[Suggestion] getLatestActiveGoalFromBlocks is the core new "derive the active goal from the transcript" logic, but there is no App.test.* in packages/web-shell, so none of its branches are exercised: latest-set-wins (backward scan), terminal-kind → null (a stale goal must clear — a regression here pins a dead goal in the status bar), source==='goal' → parseGoalStatusMessage(data) vs the legacy text sentinel fallback, and the status.setAt ?? block.serverTimestamp ?? block.createdAt fallback chain.
The same goal-parse logic is untested elsewhere it was added: SystemMessage's new source==='goal' ? parseGoalStatusMessage(data) : parseGoalStatusMessage(content) branch, and GoalStatusMessage.parseGoalStatusMessage(unknown) / normalizeGoalStatus / getNumber (object-vs-string input, non-finite number rejection). packages/web-shell already has a jsdom + Testing Library harness, so these are straightforward to cover.
— claude-opus-4-8[1m] via Qwen Code /qreview
Local verification (merge reference)I built this PR and verified the new goal-status mechanism live over the real ACP daemon protocol (a real Verdict: the core claim — 1. PR test plan — all green
2. Live ACP daemon test — the structured event on the wireI wrote a real ACP client ( {"sessionUpdate":"agent_message_chunk","contentText":"",
"goalStatus":{"kind":"set","condition":"write a hello world script","setAt":1781427005833}}That is exactly the new design: an empty Caveat (test-harness, not a PR issue): I drove set and clear, but only 3. Mutation testing — the new tests are non-vacuousReverting each source file to merge-base (keeping the PR's tests) fails exactly the new tests, across all three layers:
So the emission, the 4. Scope & notes
Good to merge: the goal lifecycle is now a first-class, structured daemon-transcript event, verified on the wire, with green and non-vacuous tests and preserved backward compatibility. 中文版（Chinese version）本地验证（合并参考）我构建了此 PR，并在真实 ACP daemon 协议上验证了新的 goal-status 机制（用真实的 结论：核心主张—— 1. PR 测试计划 —— 全绿
2. 真实 ACP daemon 测试 —— 线缆上的结构化事件我写了一个真实 ACP 客户端（ {"sessionUpdate":"agent_message_chunk","contentText":"",
"goalStatus":{"kind":"set","condition":"write a hello world script","setAt":1781427005833}}这正是新设计：一个空的 注意（测试夹具限制，非 PR 问题）： 我同时驱动了 set 和 clear，但线上只捕获到 3. 变异测试 —— 新测试非空壳将每个源文件回退到 merge-base（保留 PR 测试）后，恰好让对应新测试失败，覆盖三层：
因此发出、 4. 范围与说明
可以合并： goal 生命周期现在是一等的、结构化的 daemon-transcript 事件，已在协议线缆上验证，测试全绿且非空壳，并保留了向后兼容。 |
| data?: unknown; | ||
| }; | ||
|
|
||
| function getLatestActiveGoalFromBlocks( |
There was a problem hiding this comment.
[Suggestion] getLatestActiveGoalFromBlocks is a new pure function with 5+ branches (source-based dispatch to data vs legacy text parsing, kind filtering for set/checking vs terminal, timestamp fallback chain setAt → serverTimestamp → createdAt) — but has no unit tests.
This function is the core mechanism for restoring goal state from the transcript on page refresh, which is the headline feature of this PR. The backward-compatibility text-parsing path (when source !== 'goal') is especially fragile since it relies on the old sentinel-prefix format still being parseable.
Consider extracting this to a standalone module and adding tests covering: structured data path (source='goal'), legacy text path (sentinel-prefixed string), each terminal kind returning null, checking kind treated as active, empty blocks array, and the timestamp fallback chain.
— qwen3.7-max via Qwen Code /review
|
|
||
| export { serializeGoalStatusMessage, parseGoalStatusMessage }; | ||
|
|
||
| function normalizeGoalStatus( |
There was a problem hiding this comment.
[Suggestion] normalizeGoalStatus and the updated parseGoalStatusMessage (now accepts unknown instead of string) have no unit tests. The dual-input-type design — string goes through sentinel parser, object goes through direct normalization — makes testing both paths important.
Additionally, the validation is inconsistent with DaemonSessionProvider.tsx's normalizeGoalStatus: this version allows empty-string condition (typeof condition !== 'string' passes for ''), while the provider correctly rejects it via if (!condition) return null. Consider adding || !condition to the check.
| function normalizeGoalStatus( | |
| if (typeof condition !== 'string' || !condition) return null; |
— qwen3.7-max via Qwen Code /review
| } | ||
| } | ||
|
|
||
| #emitGoalStatusItems(result: NonInteractiveSlashCommandResult): void { |
There was a problem hiding this comment.
[Suggestion] #emitGoalStatusItems — the glue between slash command outputHistoryItems and session.emitGoalStatus — is only tested indirectly. The /goal clear path is covered via acpAgent.test.ts (which calls emitGoalStatus directly), and outputHistoryItems population is covered in nonInteractiveCliCommands.test.ts. But the full /goal set chain (handleSlashCommand → goalCommand → outputHistoryItems → #emitGoalStatusItems → emitGoalStatus → MessageEmitter) is never verified end-to-end.
A bug in the field-mapping logic (e.g., a missing conditional spread for setAt, durationMs, or lastReason) would silently produce incomplete transcript data, and getLatestActiveGoalFromBlocks would reconstruct an incorrect activeGoal after page refresh.
— qwen3.7-max via Qwen Code /review
|
@qwen-code /triage |
|
Thanks for the PR! Template looks good ✓ — all required sections are filled in, bilingual body present, reviewer test plan is concrete and actionable. On direction: This solves a real UX pain point in the web-shell. Losing On approach: The scope feels right for what's being delivered. 21 files across three packages (cli, web-shell, webui) is reasonable when you're threading a new structured event type through the transcript pipeline. The replacement of the sentinel-prefix text encoding with structured One honest question for the author to think about: the old sentinel format is retained as parseable for backward compat — is there a plan to eventually migrate existing transcripts, or is the parser code intended to live forever? Not a blocker, but worth naming the long-term maintenance cost. Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ —— 所有必填段落齐全，双语正文齐备，reviewer 测试计划具体且可执行。 方向： 这解决的是 web-shell 里一个真实存在的 UX 痛点。长会话中页面刷新就丢失 方案： 交付范围与所解决的问题匹配。跨三个包（cli、web-shell、webui）的 21 个文件，在你要把一个新结构化事件类型穿透进 transcript 管道时是合理的。用结构化的 一个值得作者思考的问题：旧的 sentinel 格式为了向后兼容被保留为可解析——是否有计划最终迁移既有 transcript，还是这份解析代码会一直保留？不是阻塞项，但值得点出长期维护成本。 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
2a. Code ReviewI read the full diff (932 lines, 21 files across FindingsNo critical blockers. The implementation is clean, well-layered, and the tests genuinely guard each new behavior (verified by @wenshao's mutation testing — reverting any of the three source layers breaks the matching test). A few observations worth noting:
AGENTS.md conventionsThe PR follows project conventions: collocated tests ( 2b. Real-Scenario TestingNot run in tmux — and correctly so. This feature's observable behavior (goal pill surviving page refresh, syncing across browser tabs) requires a live browser interacting with the daemon. A terminal tmux session cannot exercise the web-shell's React rendering, transcript replay on reconnect, or cross-tab state sync. The behavior is covered by:
Unit test commands from the PR (I did not re-run due to worktree install timeout, but @wenshao's verification is thorough and publicly posted): 中文说明2a. 代码审查我在形成自己"如何解决刷新丢失 / 跨标签不同步"的方案后，阅读了完整 diff（932 行、21 个文件，跨 发现无关键阻塞。 实现干净、分层合理，每个新行为都有测试真正守护（@wenshao 的变异测试证实：回退三层源文件中的任意一层都会让对应测试失败）。 值得点出的观察：
AGENTS.md 规范PR 遵循项目规范：测试共置（ 2b. 真实场景测试未在 tmux 中运行——且这是正确的。 该特性的可观察行为（goal pill 在页面刷新后保留、跨浏览器标签同步）需要一个真实浏览器与 daemon 交互。终端 tmux 会话无法驱动 web-shell 的 React 渲染、重连时的 transcript 回放、或跨标签状态同步。该行为由以下覆盖：
PR 中的单元测试命令（因 worktree install 超时我未重跑，但 @wenshao 的验证彻底且公开发布）： — Qwen Code · qwen3.7-max |
|
Stepping back: this is a well-executed cross-package feature that solves a real UX defect (goal state vanishing on refresh, no cross-tab sync) the right way — by making goal lifecycle a first-class transcript event instead of frontend-only memory. The architecture is sound: CLI emits a structured Comparing against my independent proposal (which I wrote before reading the diff): the PR matches it closely, and actually did one thing better than I'd have thought of — carrying The maintainer verification from @wenshao is unusually thorough: live ACP daemon test capturing the event on the wire, mutation testing proving the new tests aren't vacuous, all 322 unit tests green. My own code review of the full 932-line diff found no correctness bugs, no security holes, no AGENTS.md violations. The two minor nits I flagged ( The acknowledged risk — dropping optimistic rollback on If I had to maintain this in six months: I'd thank the author. The code is well-layered, the backward-compat seam ( Verdict: approve. ✅ 中文说明退一步看：这是一个执行良好的跨包特性，用正确的方式解决了一个真实的 UX 缺陷（刷新丢失 goal 状态、无跨标签同步）——把 goal 生命周期变成一等 transcript 事件，而不是只放在前端内存里。架构是扎实的：CLI 在既有事件类型上发出结构化的 对比我在阅读 diff 之前写的独立方案：PR 与之高度一致，并且在一处做得比我想到的更好——在 @wenshao 的维护者验证异常彻底：真实 ACP daemon 测试在线缆上捕获到事件、变异测试证明新测试非空壳、322 个单元测试全绿。我对完整 932 行 diff 的代码审查未发现正确性 bug、安全漏洞或 AGENTS.md 违规。我点出的两个小 nit（ 已确认的风险—— 如果六个月后我要维护这段代码：我会感谢作者。代码分层良好，向后兼容接缝（ 结论：通过。 ✅ — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
…5098) Previously /goal state lived only in frontend memory — page refresh or multi-device sessions lost the active goal. Now the CLI emits goal status updates as structured daemon events (_meta.goalStatus), which flow through the transcript as status blocks (source: 'goal', data: {...}). The web-shell rebuilds goal state from transcript blocks on connect, making goal status survivable across page refreshes and syncable across devices. - CLI: emitGoalStatus on goal set/clear, pass outputHistoryItems through nonInteractiveCliCommands, add setAt to goalCommand output - SDK: widen DaemonUiStatusEvent source/data types, preserve them in transcript blocks - webui: normalize _meta.goalStatus in DaemonSessionProvider, replace sentinel-prefix text encoding with structured data - web-shell: derive activeGoal from transcript blocks (getLatestActiveGoalFromBlocks), remove optimistic client-side goal dispatch, parse structured goal data in GoalStatusMessage/SystemMessage - Tests: cover emitGoalStatus, outputHistoryItems passthrough, transcript block serialization, DaemonSessionProvider event conversion - Also: harden McpDialog restart result type check with isRestartEntriesResult Co-authored-by: ytahdn <ytahdn@gmail.com>
3 participants
What this PR does
Move
/goalstate from frontend-only memory into daemon transcript events so it survives page refresh and syncs across multiple devices. The CLI now emits goal status updates as structured daemon events (_meta.goalStatus), which flow through the transcript as status blocks withsource: 'goal'and a structureddatapayload. The web-shell derives active goal state from transcript blocks on connect, replacing the previous client-side optimistic dispatch that was lost on reload.The old sentinel-prefix text encoding (
DAEMON_GOAL_STATUS_SENTINEL_PREFIX + JSON.stringify(...)) is replaced with structuredsource/datafields on status transcript blocks, which is cleaner and more extensible. The old format is still parseable for backward compatibility.Why it's needed
Previously,
/goalstate was written only to the web-shell's frontend memory via custom DOM events. This meant: (1) refreshing the page lost the active goal display, (2) multiple browser tabs or devices could not see each other's goal state, and (3) the goal status was not part of the session transcript, making it impossible to audit or replay. By persisting goal status as first-class transcript events, the goal lifecycle (set → checking → achieved/cleared/failed) becomes fully traceable and multi-device consistent.Reviewer Test Plan
How to verify
/goal write a hello world script/goal clear— verify the pill disappears on both tabscd packages/cli && npx vitest run src/acp-integration/acpAgent.test.ts src/acp-integration/session/emitters/MessageEmitter.test.ts src/nonInteractiveCliCommands.test.tsandcd packages/webui && npx vitest run src/daemon/session/DaemonSessionProvider.test.tsxandcd packages/web-shell && npx vitest run client/adapters/transcriptToMessages.test.tsEvidence (Before & After)
Before: page refresh loses goal pill, second tab never sees goal state.
After: goal pill restored from transcript on refresh, all tabs stay in sync.
Tested on
Environment (optional)
npm run devwith local daemon, web-shell connected via browser.Risk & Scope
sendPrompterror path for/goal setno longer does optimistic rollback — if the daemon call fails, the goal pill won't appear. This is acceptable since a failed goal-set means no hook was registered, so showing a pill would be misleading.parseGoalStatusMessagefor backward compatibility with existing transcripts.功能截图：
Linked Issues
N/A
中文说明
变更内容
将
/goal状态从前端内存写入迁移到 daemon transcript 事件，使 goal 状态在页面刷新和多端场景下可持久化和同步。CLI 现在将 goal 状态更新作为结构化 daemon 事件（_meta.goalStatus）发出，这些事件以source: 'goal'和结构化data的形式流经 transcript。web-shell 在连接时从 transcript blocks 派生活跃 goal 状态，替代了之前页面刷新即丢失的客户端乐观派发方式。旧的 sentinel 前缀文本编码（
DAEMON_GOAL_STATUS_SENTINEL_PREFIX + JSON.stringify(...)）被替换为 status transcript block 上的结构化source/data字段，更清晰且更易扩展。旧格式仍可解析以保持向后兼容。为什么需要这个变更
之前
/goal状态仅通过自定义 DOM 事件写入 web-shell 前端内存，导致：(1) 刷新页面丢失活跃 goal 显示，(2) 多个浏览器标签页或设备无法看到彼此的 goal 状态，(3) goal 状态不在 session transcript 中，无法审计或回放。通过将 goal 状态持久化为一等 transcript 事件，goal 生命周期（set → checking → achieved/cleared/failed）变得完全可追溯且多端一致。风险与范围
/goal set的sendPrompt错误路径不再做乐观回滚——如果 daemon 调用失败，goal 不会出现。这是可接受的，因为失败的 goal-set 意味着没有注册 hook，显示 goal 反而会误导用户。parseGoalStatusMessage解析，保持与现有 transcript 的向后兼容。