feat(serve): Add live-state session activity watermark - #9396
Conversation
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Design review, verified against the code at the PR base (2af4f903de, the #9366 merge commit). Every factual claim in the document checks out: the declared-but-unpopulated BridgeSessionSummary.updatedAt, mtime-derived persisted summaries, the live.updatedAt ?? existing.updatedAt merge, the publishPromptTerminal latch + running-state gate ordering, heartbeat-driven sessionLastSeenAt, the memory-only route with version-fenced cache invalidation, the SDK single-REST/no-probe contract, the serialized async recording queue, and the current Web Shell 2s poll + rate-limited post-turn reconcile. An independent grep of all getSessionSummary / listWorkspaceSessions read sites found no consumer missing from the audit table — the ACP-transport session/list dispatch shares listWorkspaceSessionsForResponse, so it is functionally covered by the "Full workspace session lists" row.
Direction is sound: terminal-latch authority, the catalog-version boundary, the later-valid-timestamp merge correction, and the two-PR split are the right calls. Four inline suggestions below — all wording/completeness level, none blocking.
CI note: Test (ubuntu-latest, Node 22.x) failed with ENOSPC: no space left on device during npm install — runner infrastructure flake, unrelated to this docs-only change; a rerun should clear it.
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Advance a bridge-local per-session activity watermark once when a prompt that reached the running state publishes its formal terminal, project it as the existing optional BridgeSessionSummary.updatedAt, and expose it on the workspace live-state route. The advance is written before the terminal is published so a client that observes the terminal cannot read a stale value, and the extra millisecond keeps the watermark strictly increasing when several terminals share a wall-clock millisecond or the clock moves backward. A queued-only terminal, heartbeat, attach/detach, or streamed update never advances it, and turn activity does not change the session catalog version. Populating the already-typed summary field lets full workspace session lists merge live and persisted timestamps. Because the mtime and the running-turn watermark are different authorities and the recorder writes asynchronously, the merge picks the later valid timestamp instead of blindly preferring the live value, so a row cannot move backward when an async transcript write lands after the terminal. Extend the response schema documentation for the live-state route and GET /session/:id/status, and add the optional field on the TypeScript SDK DaemonSessionLiveState type so consumers can pre-flight the tag once and read the recency directly. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
doudouOUC
changed the title
🩺 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 |
|
Re-run at an unchanged head ( Template ✓ — all sections present, bilingual, reviewer test plan included. Problem: real, and evidenced in already-merged code. The Web Shell consumer shipped in #9366 still carries its workaround comment — "the live-state overlay carries no updatedAt — flag a rate-limited reconcile so activity stamps keep refreshing." This PR fills that gap server-side. Not theoretical. Direction: aligned. It completes the typed-but-unpopulated Size: 2,506 changed lines break down as 377 production logic lines (bridge watermark, session-list cursor/merge, the Approach: every commit maps to a named finding. Risk: no high-risk path matches. Moving on to code review. 🔍 中文说明在不变的 head（ 模板 ✓ —— 各节齐全、中英双语、附 reviewer 测试方案。 问题：真实存在，且有已合入代码佐证。 #9366 合入的 Web Shell 消费端仍带着那条绕行注释——"the live-state overlay carries no updatedAt — flag a rate-limited reconcile so activity stamps keep refreshing"。本 PR 在服务端填上该缺口。不是理论性问题。 方向：对齐。 补全 #9261 留下的"已声明未填充"契约 规模： 2,506 行改动分为：377 行生产逻辑（bridge 水位、session-list 游标/合并、 方案：每个 commit 都对应一条指名发现。 风险： 未命中高风险路径。 进入代码审查。🔍 — Qwen Code · qwen3.8-max Reviewed at |
|
Re-run at an unchanged head ( Code reviewThe implementation is the shape an independent proposal lands on: advance a strictly monotonic per-session watermark ( The two round-1 Critical findings remain closed, and this pass re-verified the mechanisms rather than taking the prior pass's word:
The docs tell the truth about the guarantee — the protocol doc states the residual duplication modes (persisted-only rows are never carried; callers should key by Residual observations, all previously deferred as Suggestion-class and unchanged: the teardown-flush advance is untestable through the public interface; Test evidence — the PR's own CI (unattended run)Unattended CI run: no code from this PR was built or executed by the triage agent. Evidence below is the PR's own check-runs on
Serve A/B at this head (4 scenarios, no response changes against the PR base) confirms the additive wire surface; the watermark behaviour itself is the unit suite's job above. Sandboxed verification ( Real-scenario testingN/A for this run — unattended CI never executes PR code, and the change has no user-visible TUI surface (server-side protocol field and cursor internals). Not verified by this agent: watermark and cursor-dedup behaviour under a real daemon — covered instead by the maintainer-run runtime verification cited above. 中文说明在不变的 head（ 代码审查实现即是独立方案会落到的形态：在唯一终态漏斗 round-1 的两条 Critical 发现仍然关闭，本轮复核了机制本身而非转述上次结论：
文档如实描述保证范围——协议文档写明残余重复模式（persisted-only 行从不被携带；调用方应按 残余观察，均为先前已按建议级延后且无变化：teardown-flush 推进无法经公开接口测试； 测试证据 —— PR 自己的 CI（无人值守运行）无人值守 CI 运行：triage agent 未构建、未执行本 PR 的任何代码。以下证据为本轮通过 API 重新获取的 Serve A/B 在本 head 为绿（4 场景、相对 PR base 无响应变化），确认增量线协议面；水位行为本身由上方单测套件覆盖。 沙箱验证（ 真实场景测试本次运行不适用——无人值守 CI 从不执行 PR 代码，且改动无用户可见 TUI 面（服务端协议字段与 cursor 内部结构）。本 agent 未验证：真实 daemon 下的水位与游标去重行为——改由上文引用的 maintainer 运行时验证覆盖。 — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 4/5 — clean across every stage on this head; the only remaining items are the recorded Suggestion-class deferrals (test-pinning probes from review rounds 4–5 and @ytahdn's synchronous-subscriber assertion), all confirmed non-blocking and routed to a follow-up. The prior pass deferred for one concrete reason: the maintainer who requested changes had not seen the three fix commits, and the end-to-end signal was unsettled. Both are now resolved on this exact head. @wenshao re-reviewed (round 5: zero new findings — six probes, all deferred as non-blocking) and went further than the gate could ask: a runtime verification with both arms built from source, 59/59 assertions against the reviewer test plan, the PR's own 1,923-test suite green, and 9/9 source mutants killed. @ytahdn independently approved after verifying the contract point by point, including closure of all four earlier blockers. Stepping back: my independent proposal before reading the diff was the shape this PR implements — one strictly-monotonic watermark advanced in the single terminal funnel, later-valid merge, identity-carrying cursors. Where the PR exceeds it is still the failure-mode discipline: each review round got a named commit instead of an argument, the carry degrades to at-most-once duplicates instead of errors, and the docs state the residual duplication modes instead of hiding them. When the author chose to record @ytahdn's last suggestion as a follow-up rather than push again and void two sign-offs, that was the right call for the same reason. If I had to maintain this in six months, the shared I am approving because the evidence is affirmative, not because I ran out of objections: the problem is real and evidenced in merged code, the diff is minimal and matches its stated goal, CI is fully green, and the two humans best placed to judge it have signed off on exactly this commit. One mechanics note for the merge: @wenshao's change request from 中文说明置信度：4/5 —— 本 head 上各阶段均干净；剩余项仅为已记录的 Suggestion 级延后项（review round 4–5 的测试钉住探针与 @ytahdn 的同步订阅者断言），均已确认非阻塞并转入后续跟进。 上一轮 defer 出于一个具体原因：提出修改要求的 maintainer 还没有看到三个 fix commit，端到端信号未落定。两者如今都在本 head 上解决。@wenshao 已复审（round 5：零新发现——六条探针全部按非阻塞延后），并做到了门槛无法要求的地步：双臂均从源码构建的运行时验证，59/59 断言对照 reviewer 测试方案，PR 自带 1,923 测试套件全绿，9/9 源码变异体被杀。@ytahdn 在逐点核验契约（含四条先前阻塞项的关闭）后独立批准。 退一步看：我在读 diff 前的独立方案就是本 PR 实现的形态——在唯一终态漏斗推进一个严格单调水位、later-valid 合并、cursor 携带身份。PR 超出独立方案之处仍是失败模式纪律：每一轮 review 得到的是以其命名的 commit 而不是辩解，携带机制超限时降级为至多一次重复而非报错，文档写明残余重复模式而不是藏起来。作者选择把 @ytahdn 的最后一条建议记录为后续跟进、而不是再推一次 commit 使两份签核作废——出于同样的理由，这是对的。若六个月后由我维护，共享的 我批准是因为证据是肯定的，而不是因为提不出反对意见：问题真实存在且有已合入代码佐证，diff 最小且与其目标一致，CI 全绿，最有资格判断它的两个人类都已在同一 commit 上签核。合入机制上的一条提醒：@wenshao 立于 — Qwen Code · qwen3.8-max Reviewed at |
|
@qwen-code /triage |
|
Sandboxed verification: ✅ passed — merge-ready (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 48 passed · 0 failed · 48 total 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：48 通过 · 0 失败 · 48 总计 Verification reportPR #9396 verification — feat(serve): Add live-state session activity watermarkVerdict: 中文摘要
ScopeCentral claim — the bridge advances a strictly monotonic per-session watermark exactly once per prompt terminal that reached Secondary claims — (1) full workspace session lists merge live and persisted activity timestamps keeping the later valid value, so an async transcript write can never move a row backward; (2) turn activity never advances the catalog version, and queued-only terminals, heartbeats, and admission never advance the watermark. A/B load-bearing proofBase arm = scratch worktree at
Witnesses: Regression gates at head: Independent harness (mock-free, real code paths)Canonical copies in this artifact dir (
Mutation matrixApplied in a scratch worktree at the merge commit; positive controls are M1/M2 (caught). Raw log:
No mutant regressed a killed cell to survived; the unmutated control is green. Findings (non-blocking)F1 — the teardown-flush watermark advance is write-only (Suggestion, doc/plan accuracy). All four flush paths remove the session entry from the bridge map before any public read can observe the advanced value: F2 — two design-test-plan items ship unpinned (Suggestion, coverage). (a) "Live-only session list and cursor keys use the populated timestamp" — the behavior change is real and intended ( F3 — mutation survivors classified (completeness, not merge conditions). M3 and M5 above are adjudicated harmless with independent evidence (M-TIE run under M3; M5 run under both suites plus the async-bus argument). Neither is dead code in a risky sense: M3's Not covered
MethodologyEnvironment: CI verify container ( Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention). — Qwen Code · sandboxed verification |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
中文说明
已审查。 建议见行内评论。
— qwen3.8-max via Qwen Code /review (v0.21.13)
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
2 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- R1-2 downstream-surface test deliverables unshipped — already reported at packages/acp-bridge/src/bridge.ts:3573 (comment 3803671629)
- R1-4 docs/users/qwen-serve.md stale status-route claim — already reported at packages/acp-bridge/src/bridge.ts:3573 (comment 3803671629)
中文说明
已审查。 建议见行内评论。
本轮确认的 2 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.21.13)
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
1 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- R1-3 design-doc-promised watermark tests missing (deadline/teardown/duplicate-suppression/clock-jump) — already reported at packages/acp-bridge/src/bridge.test.ts:17693 (comment 3803671612, by qwen-code-ci-bot)
Not explored to full depth (tool budget reached): "agent 2": none — no check was cut short..
中文说明
本轮确认的 1 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未探索到全部深度（达到工具调用预算）："agent 2"：none — no check was cut short.。
— qwen3.8-max via Qwen Code /review (v0.21.13)
read_thread and wait_threads read the bridge summary directly, so once the summary began carrying the running-turn watermark their fallbacks stopped consulting the persisted transcript timestamp. Because the recorder writes asynchronously, one task could report a later recency from the thread list than from a thread read, and a wait cursor keyed on the live value alone stopped changing when only the transcript advanced, so a consumer waiting for that flush saw an unchanged cursor and exited early. Move the merge rule into a shared helper and apply it at all three read points, including the revision fallback used for a session with no attached client. Add the watermark cases the design doc enumerates but the previous commit did not ship: the deadline path publishes its terminal twice and must still advance exactly once, a corrected forward clock jump must never decrease the value, and a clock that advances between terminals must be reported instead of the logical tie-breaker. Cover the single-session status route's verbatim pass-through of the field, and cover the helper's both-invalid tail directly because no route can supply two invalid candidates. Correct two design-doc test-plan claims that did not match the code: the teardown paths advance a watermark no consumer can read, because the entry leaves live state in the same operation, and the duplicate deadline terminal comes from the raced rejection reaching the settle handler rather than from a late agent result. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
The organized view applied the live merge only on the first page, so page 1 sorted rows and encoded its cursor from merged activity keys while later pages keyed the same rows by persisted mtime alone. That was harmless while bridge summaries never carried an activity timestamp, because both keys were the mtime. Now that a settled turn advances a watermark that leads storage until the recorder flushes, a live row ordered onto page 1 by its watermark falls behind the page-1 cursor boundary on page 2 and is returned a second time, displacing a genuinely new row. Merge live state on every page so both pages key rows the same way; a live-only row still has no persisted key to page by and stays a first-page insertion. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
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): "agent 3a": none — no checks were cut short..
Deferred under the convergence posture (round 2, not a blocker) — recorded, not requested in this round:
packages/sdk-typescript/test/unit/daemon-public-surface.test.ts:446 — [test] SDK public-surface assertion for updatedAt is enforced only by typecheck, not the test runner
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）："agent 3a"：none — no checks were cut short.。
收敛姿态下延后（第 2 轮，非阻断）——已记录，本轮不要求修改：共 1 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.21.13)
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Not explored to full depth (tool budget reached): "agent 6a": none — no check was cut short..
Deferred under the convergence posture (round 2, not a blocker) — recorded, not requested in this round:
packages/cli/src/serve/server/session-list.ts:407 — [review] Default session list: cross-page ordering divergence between mtime partition and merged-key sortpackages/sdk-typescript/test/unit/daemon-public-surface.test.ts:446 — [test] SDK public-surface assertion for updatedAt is inert at test-run time
中文说明
未探索到全部深度（达到工具调用预算）："agent 6a"：none — no check was cut short.。
收敛姿态下延后（第 2 轮，非阻断）——已记录，本轮不要求修改：共 2 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.21.11)
This comment has been minimized.
This comment has been minimized.
An activity key merges the bridge's in-memory watermark, so it is not a stable property of a row: when a live entry retires mid-pagination the key regresses to the transcript mtime, and a live-only row that persists mid-pass re-enters the scan keyed by its first flush. Either way a row already emitted on an earlier page could pass the strictly-older cursor filter again and displace a genuinely new row, which was structurally impossible while activity keys came from mtime alone. The organized and metadata activity cursors now carry the identities already emitted at a live-derived key, and the after-cursor filter excludes them, so one pass returns a session at most once. The list prunes itself: an identity is dropped once its persisted floor alone can no longer pass the key filter or once the row leaves the filtered collection while not live. Past a 64-identity cap the highest floors are dropped first, degrading to the previous at-most- once duplicate instead of failing the pass. Cursors minted before the field existed stay valid, and the field is omitted when empty.
|
@qwen-code /triage |
|
Sandboxed verification: The verification job did not complete (checkout, runner, or setup error) and produced no report. See the workflow run for details. 中文 — 判定：
|
|
Triage re-run completed without a new review.
The stage comments above were updated with the latest result. View workflow run. 上方各阶段评论已更新为最新结果。查看工作流运行。 |
|
Sandboxed verification: The verification job did not complete (checkout, runner, or setup error) and produced no report. See the workflow run for details. 中文 — 判定：
|
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
Not explored to full depth (tool budget reached): chunk 5: could not execute the new/changed unit tests (live-task-service.test.ts, multi-workspace-sessions.test.ts, server.test.ts) — the review worktree has no node_mod….
Deferred under the convergence posture (round 3, not a blocker) — recorded, not requested in this round:
packages/acp-bridge/src/bridge.ts:2027 — [review] watermark-before-broadcast happens-before is unpinned by any testdocs/design/2026-08-18-workspace-session-live-state-updated-at.md:302 — [review] 'same effective recency' claim falsified by the bare-watermark surfacesdocs/design/2026-08-18-workspace-session-live-state-updated-at.md:531 — [review] Test Plan stability promises (attach/detach, interaction wait, streamed updates) unpinneddocs/design/2026-08-18-workspace-session-live-state-updated-at.md:556 — [review] Test Plan 'Live Task cursor stable across heartbeats' half untesteddocs/developers/qwen-serve-protocol.md:2312 — [review] live-state refresh guidance omits the keep-the-later rule (SDK doc twin)packages/cli/src/serve/server.test.ts:15815 — [review] 'watermark leads mtime' test under-proves its title; no discriminating every-page-merge geometrypackages/cli/src/serve/server/activity-timestamp.ts:19 — [review] module docstring overclaims cross-route recency consistency
中文说明
已审查。 建议见行内评论。
未探索到全部深度（达到工具调用预算）：chunk 5：could not execute the new/changed unit tests (live-task-service.test.ts, multi-workspace-sessions.test.ts, server.test.ts) — the review worktree has no node_mod…。
收敛姿态下延后（第 3 轮，非阻断）——已记录，本轮不要求修改：共 7 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.21.13)
…nation The emitted-identity carry could still drop a carried session mid-pass and re-admit it later: an identity absent from a page's collection was discarded even though absence can be transient (pre-flush TTL cache, mid-pass group movement), organized re-entry was evaluated under the row's current pin state only, and the live-only cursor key could move backward when a wall-clock rollback landed the first watermark behind createdAt. Retain absent carried identities at a negative-infinity floor, test organized re-entry under both pin states, and floor the first watermark advance at the entry's createdAt. Extend the retire test to a three-page pass so carried-set propagation through an intermediate cursor is pinned, probe scan visibility in the mid-pass-flush tests, and cover the live-list-failure and unpin paths. Scope the at-most-once pagination wording in the design and protocol docs to what the carry actually guarantees.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Deferred under the convergence posture (round 4, not a blocker) — recorded, not requested in this round:
packages/acp-bridge/src/bridge.test.ts:17735 — [probe] watermark-before-broadcast happens-before unpinned by any test
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
收敛姿态下延后（第 4 轮，非阻断）——已记录，本轮不要求修改：共 1 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.21.13)
|
@qwen-code /triage |
|
Sandboxed verification: The verification job did not complete (checkout, runner, or setup error) and produced no report. See the workflow run for details. 中文 — 判定：
|
|
Triage re-run completed without a new review.
The stage comments above were updated with the latest result. View workflow run. 上方各阶段评论已更新为最新结果。查看工作流运行。 |
Runtime verification report — LGTM ✅I built a real end-to-end environment for this PR and ran the reviewer test plan against it rather than reading the diff. Everything in the plan holds; 59/59 runtime assertions pass on the shipped build, the PR's own suites are green (1923 tests), and every production hunk is load-bearing (9/9 source mutations killed, plus 4 compiled-dist A/B arms that reproduce the exact bugs the follow-up commits fix). Environment
Both arms were built from source into real Reviewer test plan — item by item
Item 2 in detail — what does and does not advance the watermark (all observed on the wire)
Write-before-publish was tested as a genuine race, not by reading the code: I subscribe to Clock safety was tested by driving the shipped compiled Lifecycle: after a daemon restart the generation changes, a session restored with Evidence
Are the extra guards load-bearing?The base build never populates
One measurement is worth calling out because it changes how these guards should be read: on this machine the transcript mtime consistently lands 3–8 ms before the terminal that mints the watermark. So "live watermark leads storage" is not a rare race — it is the ordinary post-turn state of every session, which means the duplicate-row hazard would have been hit routinely, not occasionally. The three follow-up commits are not over-engineering. Test suites and mutation teethAll six suites the PR touches are green on the PR build: To check the new coverage actually bites, I reverted each production hunk in source and re-ran the mapped suite. 9/9 killed, including the ones that are easy to leave untested: the queued-only gate, the One note for future refactors: the live-state route's Cursor compatibility (checked explicitly, since it is a wire change)
Not covered by this verificationWeb Shell request-count reduction (out of scope by design), sustained turn-churn performance, multi-daemon runtime replacement beyond a plain restart, and Windows/macOS — this run is Linux only. The residual duplicate the protocol doc already calls out (a persisted-only row unpinned between page fetches) is unchanged from before this PR and correctly documented. Recommendation: merge. The contract is implemented as designed, the additive field is genuinely additive, the guards are all load-bearing, and the documentation matches the observed behaviour line for line. 中文版报告运行时验证报告 — 建议合并 ✅我没有只看 diff，而是在本地搭建了真实的端到端环境，按 Reviewer 测试方案逐条跑了一遍。结论：方案中的每一条都成立，shipped 构建上 59/59 条运行时断言全部通过，PR 自带的测试套件全绿（1923 条），并且每一处生产代码改动都是"有承重作用"的（9/9 源码变异全部被杀，另有 4 个编译产物 A/B 臂精确复现了后续 commit 所修复的那几个 bug）。 环境
两个臂都从源码构建成真实 Reviewer 测试方案逐条结果
第 2 条明细 —— 什么会 / 不会推进水位（全部为线上观测）
"先写水位、后发终态"是按真实竞态测的，不是读代码得出的：我订阅 时钟安全性是直接驱动 生命周期： daemon 重启后 generation 变化；用 关键截图见上方英文部分的四张图（分别是：水位语义、隔离性与时钟安全性、编译产物变异 A/B、测试套件与变异测试）。 那些额外的守卫真的有用吗？base 构建根本不会填充
有一个测量结果值得单独说，因为它改变了这些守卫的定位：在这台机器上，transcript mtime 稳定地比产生水位的那次终态早 3–8 毫秒。也就是说"live 水位领先存储"并不是罕见竞态，而是每个会话每次 turn 之后的常态——意味着重复行的问题会被经常性触发，而不是偶发。那三个后续 commit 不是过度设计。 测试套件与变异测试PR 触及的六个套件在 PR 构建上全绿： 为了确认新增覆盖真的有牙齿，我在源码层面逐个回退生产改动并重跑对应套件，9/9 全部被杀，包括那些最容易漏测的点：仅排队终态的门禁、 给后续重构留一条提示：live-state 路由里 游标兼容性（因为这是线上格式变更，专门验证）
本次未覆盖的范围Web Shell 请求数下降（设计上属于范围外）、持续 turn 压力下的性能、除普通重启之外的多 daemon runtime 替换，以及 Windows/macOS —— 本次仅在 Linux 上运行。协议文档已经写明的残余情况（仅持久化的行在两次翻页之间被取消置顶仍可能重复）在本 PR 前后没有变化，且文档描述准确。 结论：建议合并。 契约按设计实现，增量字段确实是纯增量的，所有守卫都有承重作用，文档与实测行为逐句吻合。 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
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 at the 5-round plan cap without two consecutive dry rounds (round 5 still reported two confirmed findings).
Deferred under the convergence posture (round 5, not a blocker) — recorded, not requested in this round:
packages/sdk-typescript/test/unit/daemon-public-surface.test.ts:446 — [test] SDK public-surface assertion for updatedAt is enforced only by typecheck, not the test runnerpackages/cli/src/serve/server.test.ts:15991 — [probe] No test pins the removal (self-pruning) side of the carried-emitted-identity state machinedocs/design/2026-08-18-workspace-session-live-state-updated-at.md:452 — [review] Design doc's follow-up contract rule 1 claims turnCompleted carries the workspace cwd; the shipped event has no cwd fieldpackages/cli/src/serve/server.test.ts:16417 — [probe] Cap-overflow drop order between floored and floor-less (-Infinity) carried identities is unpinned by any test
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查：reverse audit — stopped at the 5-round plan cap without two consecutive dry rounds (round 5 still reported two confirmed findings)。
收敛姿态下延后（第 5 轮，非阻断）——已记录，本轮不要求修改：共 4 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.21.13)
ytahdn
left a comment
ytahdn
left a comment
There was a problem hiding this comment.
审查结论 / Review Verdict: ✅ 批准 / Approve
（纯静态审查，未运行测试或构建。Static review; no tests or builds were run.）
总体评价 / Overall
本 PR 为 acp-bridge 引入了按会话严格单调的活动水位线，并以可选 updatedAt 字段投影到会话列表与 SDK。审查在 head（731d9b0c）上逐点验证了核心契约：
This PR introduces a per-session strictly monotonic activity watermark in acp-bridge, projected as an optional updatedAt field onto session summaries and the SDK. Reviewed against the head commit (731d9b0); core contract points verified one by one:
-
恰好一次推进 / Exactly-once advance：水位线仅在进入过
running的 prompt 赢得正式终态闩锁时推进一次；publishPromptTerminal是唯一终态漏斗，5 个调用点（teardown flush、deadline、natural settle ×3、cancelSession）全部核对一致。
The watermark advances exactly once when a prompt that reachedrunningwins its formal terminal latch;publishPromptTerminalis the sole terminal funnel, and all five call sites were checked. -
先写后发布 / Write-before-publish：水位线推进发生在终态向订阅者广播之前（
terminalPublished标记顺序正确）。
The watermark is advanced before the terminal state is broadcast to subscribers. -
严格单调 / Strict monotonicity：
Math.max(Date.now(), previous + 1)覆盖同毫秒竞争与钟回拨；首次推进以createdAt + 1为下限；钟前跳不破坏单调性。
The formula covers same-millisecond races and clock rollback; the first advance is floored atcreatedAt + 1; forward clock jumps do not break monotonicity. -
later-valid 合并一致 / Consistent later-valid merge：所有会话列表读取路径（含分页 re-enter 双固定场景）统一用
laterActivityTimestamp合并 live 水位与持久化 mtime；archived 会话不参与合并；updatedAt采用键缺失（而非 null）语义，SDK 类型注释明确其仅作排序信号、不作为 transcript 已 flush 的证明。
All session-list read paths uniformly merge the live watermark with persisted mtime vialaterActivityTimestamp(including pagination re-entry); archived sessions are excluded;updatedAtuses absent-key (not null) semantics, documented in the SDK types as ordering-only, not proof that the transcript was flushed. -
此前讨论的四个阻塞项已在 head 上确认关闭：逐页合并、活动游标 carried identities（防止游标键回归）、dual-invalid 回退（
live ?? persisted，含测试）、文档重复内容清理。
All four previously raised blockers are confirmed closed at head: per-page merge, carried identities in activity cursors (preventing cursor-key regression), the dual-invalid fallback (tested), and the doc deduplication. -
catalog 不变量保持 / Catalog invariant held：目录生成与修订不因水位线推进而改变；两份协议文档与实现一致且足够完整。
Catalog generation and revision are unchanged by watermark advances; both protocol docs are thorough and consistent with the implementation.
本次独立静态审查与维护者在同一 head 上的运行时验证（59/59 断言、9/9 变异体被杀）结论一致。
This independent static review is consistent with the maintainer's runtime verification on the same head (59/59 assertions, 9/9 mutants killed).
🟢 一个非阻塞建议 / One non-blocking suggestion
bridge.test.ts 的水位线 describe 块（8 个用例）都在异步迭代完成后才读取 updatedAt，因此“把 advanceTurnActivity 移到广播之后”的变异体仍可通过测试——即“先写后发布”的时序契约目前缺少同步订阅者视角的断言。实现本身是正确的，建议后续补一个在同步订阅回调内断言水位线已推进的用例，把该契约钉死。
The watermark describe block in bridge.test.ts reads updatedAt only after async iteration completes, so a mutant that moves advanceTurnActivity after the broadcasts would still pass — the write-before-publish timing contract lacks a synchronous-subscriber assertion. The implementation is correct; consider adding a case that asserts the watermark inside a synchronous subscriber callback to pin this contract.
|
Thanks @ytahdn for the thorough static review and the approval. ✅ I verified the non-blocking suggestion against this head ( Recording it as a deferred follow-up rather than pushing now, per the convergence posture (round 5+, suggestion-level, implementation confirmed correct): this exact head carries both the maintainer's runtime verification (59/59 assertions, 9/9 mutants killed) and your approval, and any push would void them via stale-review dismissal and restart the multi-hour CI/bot cycle. The synchronous-subscriber assertion that pins the write-before-publish contract will go into the follow-up alongside the bot's recorded deferrals. |
|
@qwen-code /triage |
|
Sandboxed verification: The verification job did not complete (checkout, runner, or setup error) and produced no report. See the workflow run for details. 中文 — 判定：
|
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
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 7: run the three new server.test.ts cases end-to-end — the review worktree has no installed workspace deps ( @qwen-code/channel-base fails to resolve), so the fil….
Deferred under the convergence posture (round 5, not a blocker) — recorded, not requested in this round:
packages/cli/src/serve/server/session-list.ts:327 — [probe] metadata forged-cursor rejection untested (mutant survives)packages/sdk-typescript/test/unit/daemon-public-surface.test.ts:446 — [probe] SDK public-surface updatedAt assertion inert at test-run time; no typecheck step in PR CIpackages/cli/src/serve/server/activity-timestamp.ts:17 — [probe] laterActivityTimestamp docstring overclaims cross-route recency consistencypackages/cli/src/serve/server/session-list.ts:170 — [probe] unbounded emitted array accepted at cursor decode (encode caps at 64)packages/cli/src/serve/server.test.ts:15991 — [probe] carried-identity shrink path untested (never-shrink mutant survives)packages/acp-bridge/src/bridge.test.ts:17693 — [probe] per-session watermark isolation untested (bridge-scoped hoist mutant survives)
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）：chunk 7：run the three new server.test.ts cases end-to-end — the review worktree has no installed workspace deps ( @qwen-code/channel-base fails to resolve), so the fil…。
收敛姿态下延后（第 5 轮，非阻断）——已记录，本轮不要求修改：共 6 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.21.11)
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. ✅
chiga0
left a comment
chiga0
left a comment
There was a problem hiding this comment.
Code Review Overview (AI Generated)
PR: #9396 feat(serve): Add live-state session activity watermark
Type: New Feature + Bug Fix (pagination correctness)
Change size: +2451/-55 across 15 files
Reviewed at HEAD: 731d9b0c36a1a803d8d420c20513fb944f937849
Findings Summary
- Critical/Major: 0
- Minor: 0
- Nit: 1 (see below)
Key Observations
The implementation is architecturally clean and well-defended. The advanceTurnActivity strictly-monotonic design (Math.max(Date.now(), previous + 1)) correctly handles same-millisecond terminals, clock rollback, and forward jump/correction in all four cases. The laterActivityTimestamp helper is precisely scoped with correct tie-breaking semantics (live wins on equal instants via liveTime >= persistedTime, preventing string-encoding flip-flop across two representations of the same time). The emitted-identity cursor carry mechanism in nextEmittedSessionIds is a principled solution to the pagination duplication problem introduced by live-derived activity keys — the Number.NEGATIVE_INFINITY floor for absent and live-only sessions ensures they are retained in the carry list (prioritized against the 64-cap drop) since their re-admission risk is highest.
The design document (695 lines) provides exceptional context: transition matrix, monotonicity proof, downstream consumer audit, and test plan — all verified against the implementation.
Nit: parseEmittedSessionIds does not validate carry-list length on inbound path
nextEmittedSessionIds caps the outbound emitted-identity list at MAX_EMITTED_CURSOR_SESSION_IDS = 64, but parseEmittedSessionIds accepts arrays of any length on the inbound path. A client that modifies a base64url cursor could submit a carry list larger than 64, bypassing the design-time cap. The practical impact is low — this is an internal daemon, URL/query-parameter length limits (~2 KB for 64 UUIDs at 36 chars each) provide a physical bound, and the worst outcome is a client excluding their own sessions from a paginated result. Nonetheless, making the invariant symmetric would be tighter:
if (
!Array.isArray(value) ||
value.length > MAX_EMITTED_CURSOR_SESSION_IDS ||
value.some((id) => typeof id !== 'string' || id.length === 0)
) {
return undefined; // cursor rejected
}Cross-Validation
Phase 1 (blind review) completed before reading prior reviews. Phase 2 cross-validation against design-review by doudouOUC, five automated probe rounds by qwen-code-ci-bot, one round by wenshao, and approval by ytahdn.
| Finding | Reviewer | My Assessment |
|---|---|---|
| Organized-list every-page merge not applied | qwen-code-ci-bot | Confirmed fixed — commit ba3f9e2; re-verified in listOrganizedWorkspaceSessionsForResponse: merge runs outside isFirstPage gate |
| Cursor-key regression on live-entry retirement (organized + metadata) | qwen-code-ci-bot, wenshao | Confirmed fixed — commit 81ff599; emittedBeforePage exclusion verified in after-cursor filter for both paths |
| Live-only row re-admitted after mid-pass persist | qwen-code-ci-bot | Confirmed fixed — commit 81ff599; row gains a persisted key and passes reenters check |
| Carried identity dropped on transient mid-pass absence | qwen-code-ci-bot | Confirmed fixed — commit be744e4; !row branch now retains at Number.NEGATIVE_INFINITY floor unconditionally |
reenters evaluated under current pin state only |
qwen-code-ci-bot | Confirmed fixed — commit be744e4; [true, false].some(...) evaluates both pin states |
Date.now() half of Math.max not independently tested |
qwen-code-ci-bot | Confirmed fixed — clock-advance-between-terminals test verifies wall-clock path; pinned-clock tests verify logical +1 path |
live-task-service.ts ?? fallbacks not updated with laterActivityTimestamp |
qwen-code-ci-bot | Confirmed fixed — commit 7af6bca; all three read points updated |
Both-invalid laterActivityTimestamp fallback untested |
qwen-code-ci-bot | Confirmed fixed — activity-timestamp.test.ts covers (undefined, 'not-a-timestamp'), ('not-a-timestamp', undefined), ('live-garbage', 'persisted-garbage'), and (undefined, undefined) |
session_status verbatim pass-through untested |
qwen-code-ci-bot | Confirmed fixed — server.test.ts "200 passes the activity watermark through verbatim" test |
Design doc normative formula missing createdAt floor |
qwen-code-ci-bot | Confirmed fixed — HEAD commit 731d9b0; formula now matches advanceTurnActivity implementation |
| Wire-contract "at-most-once" claim too strong | qwen-code-ci-bot | Confirmed scoped — be744e4 narrows claim to what the carry actually guarantees |
| Three-page carry-set propagation untested | qwen-code-ci-bot | Confirmed fixed — retire test extended to three-page pass; intermediate-cursor carry asserted |
Unique-1: parseEmittedSessionIds no inbound length cap |
— | New Nit — internal daemon context, practical impact low |
Additional Audit Coverage
Areas independently verified beyond existing findings:
advanceTurnActivityformula: Traced all four cases — (1) normal advance:Math.max(Date.now(), prev+1)whereDate.now() > prev+1→ wall clock wins; (2) same-ms:Date.now() == prev→prev+1wins; (3) rollback:Date.now() < prev+1→prev+1wins; (4) forward-jump-then-correction: prev set to jump value, correction hasDate.now() < prev+1→prev+1wins. First-advance floor:previous = lastTurnEndedAtMs ?? (isFinite(createdAtMs) ? createdAtMs : undefined)correctly usescreatedAtMs+1as floor when no prior watermark exists.mergeLiveSessionSummaryspread interaction:...existingthen...livewould setupdatedAttolive.updatedAt; the explicitupdatedAt: laterActivityTimestamp(...)overrides this correctly. No semantic conflict.- Default (numeric cursor) path deduplication safety: Only adds live-only rows on
isFirstPage; paginated by storage offset for persisted rows; emitted-identity carry is not needed because no live-key-derived cursor is emitted on this path. Confirmed correct. publishPromptTerminalwrite-before-broadcast ordering:advanceTurnActivity(entry)executes insideif (mutateTurnState)before anybroadcastTurn*call. Node.js single-threaded event loop ensures the watermark is observable viagetSessionSummaryimmediately after the terminal is published. Confirmed.listLiveWorkspaceSessionsForResponsecursor stability: The live-only path sorts byupdatedAt ?? createdAt. Retiring sessions disappear from the in-memory live list entirely (no persisted floor to regress to), so key regression is structurally impossible. No emitted-identity carry needed. Confirmed.- SDK type completeness:
DaemonSessionLiveState.updatedAt?: stringis verified bydaemon-public-surface.test.tstype assertion. The JSDoc correctly scopes the field as "recency for ordering only" and "not proof that the transcript was flushed". laterActivityTimestampequal-instant tie:liveTime >= persistedTime(not>) correctly returnsliveon equal instants, preventing string-encoding flip-flop between two timezone representations of the same moment.
Final Verdict
APPROVE — The implementation is correct and complete at HEAD 731d9b0c. All Critical and Major issues found across five prior review rounds are confirmed fixed. The design handles all documented edge cases with dedicated tests. The single Nit (cursor inbound length validation) has negligible real-world impact given the internal-daemon context and URL-length physical bounds.
This review was generated by QoderWork AI
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
Independent review of the source (activity-timestamp.ts, session-list.ts, session.ts).
Approve. The watermark contract is implemented correctly:
laterActivityTimestampis correct.Date.parse→Number.isFinitegate, returns the later valid timestamp, falls back to whichever is valid (orlive ?? persistedwhen both invalid). ThemergeSummarychange (updatedAt: laterActivityTimestamp(live.updatedAt, existing.updatedAt)) is exactly right — an async transcript write can never move a row backward.- The
emittedcursor field is the right fix for the pagination dedup hazard. A session whose watermark advances between pages can shift sort position and be emitted twice; carrying the already-emitted id set through the cursor (capped atMAX_EMITTED_CURSOR_SESSION_IDS = 64, validated as an array of non-empty strings) closes it.parseEmittedSessionIdsrejects malformed values (fail-closed) and the cursor is backward-compatible (emittedonly serialized when non-empty). - Additive field under
v: 1with no new route/gate — matches the compatibility requirement, old clients keep the unchanged path.
No blocking findings.
7 participants
What this PR does
This PR implements the workspace session live-state activity watermark contract designed earlier on this branch. The bridge now advances a strictly monotonic per-session watermark once when a prompt that reached
runningpublishes its formal terminal, projects it as the already-typed optionalBridgeSessionSummary.updatedAt, and returns it fromGET /workspaces/:workspace/sessions/live-state. Full workspace session lists now pick the later valid timestamp between the live watermark and the persisted transcript mtime so an async transcript write can never move a row backward. The TypeScript SDK type and the developer protocol docs describe the new optional field. The design document that opened the branch remains, followed by the implementation commit that realizes it. Web Shell consumption stays out of scope by design.Why it's needed
The workspace live-state protocol removed high-frequency full session-list polling, but the current Web Shell consumer still schedules a rate-limited catalog reconciliation after every completed turn because live-state does not carry session recency. Populating the already-typed activity watermark closes that gap on the server side, keeps the catalog version stable across ordinary turns so the two-second live-state poll does not observe a mismatch, and gives the later consumer PR a deployed compatibility target while old clients continue exercising the unchanged path.
Reviewer Test Plan
How to verify
updatedAtis an optional additive field under responsev: 1and the existingworkspace_session_live_statecapability, with no new route, feature gate, or readiness feature.runningwins its formal terminal latch, is written before the terminal event is published, remains strictly increasing under same-millisecond terminals, clock rollback, or a corrected forward jump, and is never driven by admission, streaming, queue-only cancellation, heartbeat, or interaction waits.generation + revision, persisted-list cache invalidation, selected-runtime ownership, trust checks, status codes, and the route's memory-only behavior unchanged.GET /session/:id/statusreturns the bridge-local watermark directly without merge, and the compatibility, downstream consumer, test, and rollout coverage described in the design document remain accurate for the shipped implementation.Evidence (Before & After)
N/A — server protocol change with no user-visible TUI or Web Shell surface. Behavior is exercised through the bridge, route, list-merge, and SDK unit tests added in this PR.
Tested on
Environment (optional)
Node 22, local
npm run typecheck,npm run lint,npx prettier --check, and targetedvitest runagainst the bridge, CLI serve, and SDK test files touched by this change.Risk & Scope
BridgeSessionSummary.updatedAtfield could depend on the current live-first merge policy or on the field remaining unpopulated from the live path. The design's downstream consumer audit maps every read site: workspace live-state, full session lists, live-only lists and cursors,GET /session/:id/status, Live Task cursors, goals, owner resolution, and admission all read compatibly, and the merge helper now takes the later valid timestamp so a stale mtime cannot displace a fresh terminal and a delayed mtime cannot mask a live regression.v: 1and the existingworkspace_session_live_statecapability are unchanged, no configuration or stored-data migration is required, and old clients decode the response by ignoring the extra property.Linked Issues
Follow-up to #9261 and #9366. Web Shell consumption ships in a separate PR.
中文说明
本 PR 做了什么
在本分支之前提交的设计文档基础上，实现 workspace session live-state 活动水位契约。Bridge 在到达
running的 prompt 发布正式终态时严格单调地推进每会话水位，投影到已经声明但未填充的BridgeSessionSummary.updatedAt，并由GET /workspaces/:workspace/sessions/live-state返回。整包 workspace session list 合并时改为取 live 水位与持久化 mtime 中较晚的那个有效值，避免异步 transcript 写入让活动排序回退。TypeScript SDK 类型和开发者协议文档同步更新。设计 PR 打开时的文档保留，后跟本次实现 commit。Web Shell 消费端仍是后续独立改动。为什么需要
Workspace live-state 协议取消了高频 session-list 轮询，但当前 Web Shell 在每次 turn 完成后仍安排一次限频 catalog reconciliation，因为 live-state 不携带会话最近活动时间。填充这个已声明的活动水位在服务端关闭该缺口；catalog 版本在普通 turn 下不变，所以两秒轮询不会因此触发全量扫描；后续消费端 PR 也能对着一个已经部署的兼容目标写，而老客户端继续走原路径。
Reviewer 测试方案
如何验证
updatedAt是 responsev: 1与既有workspace_session_live_statecapability 下的可选增量字段，不新增路由、feature gate 或 readiness feature。running的 prompt 赢下正式终态 latch 时水位才恰好推进一次；水位写入发生在终态事件发布之前；同毫秒多个终态、时钟回拨或修正后的向前跳跃下水位仍严格递增；admission、streaming、仅排队取消、heartbeat 和交互等待均不推进它。generation + revision、persisted-list cache 失效、selected-runtime 所有权、trust 检查、状态码以及路由纯内存行为。GET /session/:id/status直接返回 bridge-local 水位（无合并）；设计文档描述的兼容性、下游消费者、测试与发布覆盖在实现中依然成立。证据（改动前后）
N/A — 服务端协议改动，无用户可见 TUI 或 Web Shell 表面。行为通过本 PR 新增的 bridge、route、list-merge、SDK 单测覆盖。
测试平台
环境（可选）
Node 22，本地
npm run typecheck、npm run lint、npx prettier --check与针对本次改动文件的vitest run全部通过。风险与范围
BridgeSessionSummary.updatedAt是一个此前已声明但未由 live 路径填充的已公开字段。设计文档的下游消费者审计已覆盖所有读点（workspace live-state、全量 session lists、live-only 列表与游标、GET /session/:id/status、Live Task 游标、goals、owner resolution、admission），且合并规则改为取较晚有效值，既阻止已过时的 mtime 顶替新终态，也阻止延迟的 mtime 掩盖 live 回退。v: 1与既有 capability 均保持不变；不需要配置、存储数据迁移或 feature flag；老客户端忽略新增属性即可正确解码。关联 Issue
#9261 与 #9366 的后续工作。Web Shell 消费端在独立 PR 中提交。