feat(serve): Add workspace session live-state endpoint and catalog version - #9261
Conversation
Design review: direction approved, with suggestions before implementationI checked the design's key claims against What verifies cleanly against the code
Suggestions1. Unversioned-but-visible changes freeze after adoption (please address explicitly). Session ordering / 2. Clarify the cache-consistency scope. The doc's Cache Consistency section reads as if the catalog were generally cached, but the default path is uncached — the WeakMap exposure-time invalidation only protects organized/metadata reads. One sentence would both strengthen the motivation (the hot path has no cache at all) and prevent reviewers from misreading the handshake's protection scope. 3. Eliminate the missed-writer risk structurally rather than by test coverage. The doc's stated top risk is a false-stable version from a missed daemon writer. Two choke points reduce it substantially: (a) fuse "invalidate persisted cache scopes + advance revision" into the existing 4. Reconsider excluding 5. Group CRUD version semantics are incomplete for clients. Groups come from a separate 6. Add reload throttling to the client handshake. During bulk restore or batch operations the version can change on every poll; "coalesce one more reload, no tight retry loop" is not enough to prevent a full reload per poll — i.e. back to the expensive regime. Please specify a minimum background full-reload interval or backoff. Minor
中文摘要方案方向正确，协议面与仓库既有契约高度一致，关键事实核查（无缓存的默认 catalog 路径、严格/宽松双信任门、persisted-only branch、restore 失败保留 fork、BridgeClient 尾参惯例、EventBus epoch 先例）全部通过。落地前建议补三条：① 未版本化变更（排序/updatedAt、TUI 外部写入）在采用后会"冻住"，需正面论证并考虑低频兜底全量刷新；② 把"失效缓存 + 推进 revision"融合进现有失效 helper、bridge 注册/移除挂在 emitSessionLifecycle 单一收口点，结构性消除漏 writer 风险；③ 快照建议加 hasTurnError/pendingInteractionCount（bridge 已免费算出），否则跨 tab 的 turn-error 状态只在版本变化时更新而 turn error 又不推进版本，可能形成可见回退。另建议客户端契约补：版本变化时同时重载 groups/organization、后台全量重载最小间隔或退避。缓存一致性一节建议注明 default catalog 路径不走缓存。 |
|
Thanks for the detailed review. I rechecked each point against the current Disposition
Minor points
I will incorporate the accepted protocol clarifications before treating the design as implementation-ready; no production behavior changes are part of this Draft PR. 中文说明感谢详细评审。逐项对照当前
其他：实现 PR 应将路由注册到 telemetry route classifier 并验证稳定的低基数 label。2 秒轮询对单一 poller 约为 30 次/分钟，低于默认 read-tier 120 次/分钟，但该 bucket 与其他读请求共享，所以只能作为容量背景，不能作为保证。 会在将方案视为可实施之前补充上述已接受的协议说明；当前 Draft PR 仍然不包含任何生产行为变更。 |
|
Confirmed both corrections after independently re-checking against On the polling path: you are right. On The remaining dispositions all look sound to me: documenting the unversioned-change boundary while deferring any periodic fallback to the Web Shell PR; choke-point marking at With the accepted documentation updates incorporated, the design looks implementation-ready to me. 中文两处纠正经我独立复核均成立：sidebar 默认携带 |
|
Updated the design in
Validation: 中文已在 文档格式检查和 |
|
v2 checked against the review thread — all accepted items are faithfully incorporated, and the per-path motivation (uncached numeric path with the 100 page cap, cached metadata/organized full scans, TTL ≈ cadence, Two additions deserve explicit credit: requiring The lifecycle choke-point marking with failure-isolated host callbacks, the preserved no-op/pre-commit-failure semantics, the dual-resource bundle handshake, and the bounded-reload client contract all read correctly. From my side this design is implementation-ready. LGTM. 中文v2 已核对：评审线程中的全部接受项均忠实落地，按路径精确化的动机描述与代码事实一致。两处额外加固值得肯定：版本快照不可变性（否则 WeakMap diff 静默失效）、pre-A 请求资格排除（堵住客户端去重机制与握手间的竞态）。收口打标的失败隔离、no-op 语义保留、双资源 bundle 握手、有界重载契约均正确。方案已可进入实施阶段。LGTM。 |
Review: design is sound and implementation-ready; one argument gap worth closingOverall I think this design is solid. It targets a real problem (high-frequency volatile state coupled to the most expensive catalog path), and it nails the things that usually bite later: cache ordering, trust boundaries, failure semantics, and the restart-safety of the version clock. A few specific notes. What works well
Main concern: the A → full → B handshake is overkill relative to what it buysThe handshake guarantees a catalog bundle is never accepted with a mismatched data/version pairing. But a mismatch is self-healing: the next live-state poll sees the revision change and reloads. So the real question is whether the product can tolerate at most one poll cycle (~2s) of a transiently mismatched bundle. If yes, a single-request shape (full-list response carries its own There is also an argument gap in the Rejected Alternatives section for "add the version to the existing session-list response":
That explains why it cannot serve high-frequency polling — but as part of a full reload response, that path is executed anyway. It does not explain why a full-list response cannot carry its own version. Those are two different claims. I'd suggest a short paragraph stating which product constraint makes the handshake necessary, or explicitly allowing clients to choose the single-request shape. Risks to watch at implementation time
Bottom lineI support the design and think it is ready to move into the server implementation PR. The one thing I'd ask the author to strengthen is the handshake-vs-single-request argument, because if the handshake is not required by a product constraint, the client state machine is complexity paid for a guarantee that self-heals anyway. 中文摘要:总体认可该设计,可进入服务端实现阶段。主要异议是 A→全量→B 握手相对其收益过度设计——错配本身会自愈,若产品能接受最多一个轮询周期的短暂错配,单请求方案(全量响应自带版本)可砍掉一半客户端状态机;且 Rejected Alternatives 中对"全量响应带版本"的否定理由存在论证缺口。另提醒实现期风险:版本递增完整性(最大静默失败风险)、外部 writer 可见性边界需产品确认、cooldown 值需实测、capability 预检时序。 |
doudouOUC
changed the title
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
…rsion Add GET /workspaces/:workspace/sessions/live-state: a memory-only volatile snapshot (clientCount, hasActivePrompt, waiting flags) plus an in-memory catalog version (generation+revision equality token), so clients stop polling the persisted catalog for volatile state. The bridge owns the clock: registration/removal marks flow through the emitSessionLifecycle choke point; rename, automatic title, worktree, and persisted branch commits mark at exact points; serve-layer REST/ACP mutations share an invalidate-then-mark helper with exact no-op semantics (deleted:false group deletes, removeSession:false cleanups, no-op renames). The route exposes a new version only after invalidating both persisted catalog scopes, enabling the client live-A -> full catalog -> live-B reconciliation handshake. Wire-additive: new unconditional capability workspace_session_live_state, TypeScript SDK types and DaemonClient/WorkspaceDaemonClient methods (native REST, no per-poll capability preflight), telemetry label, and protocol/capability/SDK docs. Required clock methods on AcpSessionBridge are a source-level contract change for external structural implementations; in-repo fakes updated. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
4259524 to
eb1899c
Compare
|
Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. 中文请勿对活跃的 PR 执行 rebase 或 force-push，因为这会使已有的评审评论失效。另外，供日后参考：作为集成流程的一部分，机器人始终会自动将所有改动压缩（squash）为单个提交。 |
|
@qwen-code /triage |
…ion baseline The capabilities envelope E2E asserts the exact advertised feature list; the new unconditional live-state capability must appear after the archived-export tag, matching registry declaration order. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Separate the two claims the earlier paragraph conflated: the catalog path cost is irrelevant to carrying a version (it runs anyway on a full reload), but stamp placement decides consistency. Stamp-after-scan can silently accept a bundle missing a mid-scan mutation; stamp-first is safe and self-heals within one poll cycle, which a client may legitimately choose. The A/B handshake buys provable consistency for one extra cheap live-state read; the server supports both and the Web Shell PR picks per product tolerance.
|
Thanks for the careful review — especially for pressing on the handshake justification. Addressing the main concern first. On A→full→B vs. a single-request shapeYou are right that the earlier Rejected Alternatives paragraph conflated two different claims: "the catalog path is expensive" (true, but it runs anyway on a full reload, so it does not bear on carrying a version) and "a full-list response cannot safely carry its own version" (the real question). I have rewritten that section in the design doc to separate them explicitly. The correctness hinge is where the stamp is read, not whether it rides along:
So the honest answer to "which product constraint makes the handshake necessary" is: none, for most consumers. What A/B buys is a stronger statement for clients that must never render a bundle that is not provably consistent with the version they accepted — e.g. a UI exposing destructive actions against catalog rows, where acting on a row the server has already mutated is worse than a 2 s lag — at the cost of exactly one extra cheap live-state read per reload. The design doc now says this, and explicitly allows the bounded single-request reconciliation as a legitimate client choice. Two reasons it does not change this PR: the server contract is identical under both options (the choice lives entirely in the client, so it belongs to the Web Shell follow-up), and a version baked into the catalog response still would not provide the volatile snapshot this route exists to serve. On the implementation-time risks
Bottom line: your read is adopted in the design doc — the bounded single-request shape is now an explicitly allowed client choice, with the stamp-placement analysis that explains why naive (stamp-after-scan) single-request cannot work.
|
|
Follow-up actions from this poll:
|
|
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: 50 passed · 0 failed · 50 total 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：50 通过 · 0 失败 · 50 总计 Verification reportPR 9261 — Deep Verification ReportVerdict:
中文摘要
Central claim + A/BCentral claim: the new trusted-only, memory-only Secondary claims: (1) the version-exposure ordering (invalidate both persisted catalog cache scopes before answering a newly observed version) and exact no-op semantics ( A/B table (same harness
The control cell is the load-bearing proof: identical daemon, identical boot, identical capability advertisement — the only delta is the reverted route registration hunk, and the endpoint disappears ( Mutation matrix on the new guards. Positive controls green first (bridge clock suite 12/12, CLI live-state route suite 9/9, dispatch ACP matrix 1/1). Each mutant is a single-point revert applied to source, run, then restored (
No mutant regressed from killed to killed-differently; every kill names the intended expected-vs-actual (e.g. M7 → M2 survivor adjudication (not a finding). The stated invariant is "a throwing Perf measurement (secondary claim 2)Real daemon, 1500 seeded transcripts (~26 JSONL records each), isolated runtime,
The live-state endpoint cost is independent of the persisted store (it stays ~ms with 1500 files on disk, Targeted gates (exact counts, all green)
Witness: CorrectionsNone to prior reviews. One factual note for the author: the PR body cites a "full bridge suites are green (783/783)" figure; the acp-bridge package on the verified head actually runs 1483 tests across 29 files (all green), and FindingsNo blocking findings. The single notable observation (M2 surviving mutant) is adjudicated above as an equivalent mutant with a killed finer discriminator (M2b), so it is reported as completeness evidence rather than a defect. Not covered
MethodologyEnvironment: Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention). — Qwen Code · sandboxed verification |
🩺 serve daemon A/BBuilt the PR base vs this PR head
|
| field | PR base (before) | this PR (after) |
|---|---|---|
features[] |
— | "workspace_session_live_state" |
— Qwen Code · serve A/B
|
Thanks for the PR — this one is unusually well prepared.
Moving on to code review. 🔍 中文说明
进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewI reviewed the diff statically against the current
No critical blockers found. One design fact reviewers should have on the record (already in Risk & Scope): the exported Files changed (20 of 36 shown)
Testing evidence — the PR's own CI (this is an unattended run; no PR code executed)Evidence carried here is the PR's own CI on the reviewed commit, fetched via the checks API — not a re-run, and not the author's self-reported numbers. Every Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 Two honest caveats, so the maintainer knows exactly what CI did and did not prove:
Sandboxed verification would settle that remaining claim: 中文说明代码审查本次为无人值守运行，未执行任何 PR 代码，仅对照当前
未发现致命阻塞项。一个需要记录在案的设计事实（Risk & Scope 已写明）：导出的 测试证据（无人值守运行，未执行 PR 代码）此处证据为被审 commit 上 PR 自己的 CI（经 checks API 抓取），不是重新运行，也不是作者自报数字。 两点如实说明，便于 maintainer 清楚 CI 证明了什么、没证明什么：
沙箱验证可以了结这最后一项： — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 3/5 — clean review, no correctness concerns found; deferring only because this is a cross-package core-infrastructure change that adds public wire-protocol + SDK surface, and that class needs a maintainer's sign-off rather than a bot auto-approve. Stepping back: this is what the repo's design-doc workflow is supposed to produce. The problem was verified against the code by reviewers before a line was written (and found understated), the two review rounds left a visible trail of accepted suggestions — structural choke points, exact no-op semantics, the stamp-placement argument — and the implementation lands each one where the thread said it would. My independent read of the problem would have proposed essentially this shape: a memory-only trusted route plus a bridge-owned equality token, with cache invalidation ordered before version exposure. The PR matches that, and goes one step further on the backstop — the exposure-time invalidation in the route closes the bridge-internal writers (auto-title, persisted-only branches) without coupling The tests are the strongest signal here. They don't just assert "it increments" — they assert exact revision equality on every no-op path, a settle window that catches wrongful extra marks, and a throwing host listener at the choke point. A suite that still passed with the mark logic removed would be worthless; this one would fail loudly. CI on the reviewed commit is fully green for every Why defer rather than approve. Correctness is not the concern — I verified the mechanism against base code and found no blockers. The reason is scope class: this change spans ⏸️ Deferring to @ytahdn — this is an otherwise-clean cross-package protocol + SDK addition that needs a maintainer's sign-off on the public-contract surface (and the exported-interface change) before merge. Needs a human call on this one, not a correctness fix. 中文说明置信度：3/5 —— 审查干净、未发现正确性问题；之所以 defer，仅因为这是一次跨包的核心基础设施改动，新增了公开的 wire 协议 + SDK 面，这一类改动需要 maintainer 拍板，而非机器人自动批准。 退一步看：这正是本仓库设计文档工作流应当产出的样子。问题在动笔前就被评审者对照代码核实（且被发现轻描淡写），两轮评审留下了可见的建议轨迹——结构性收口点、精确 no-op 语义、stamp 位置论证——实现把每一条都落在了讨论指定的位置。我对问题的独立判断也会提出同样的形态：纯内存 trusted 路由 + bridge 持有的等值令牌，缓存失效先于版本曝光。PR 与此一致，并在兜底上更进一步：路由曝光时刻的失效收掉了 bridge 内部 writer（自动标题、persisted-only branch），而不必把 测试是最强的信号：不是断言"会递增"了事，而是在每条 no-op 路径断言精确的 revision 相等、用沉降窗口捕捉错误的多余打标、并在收口点放一个会抛错的 host 监听器。被审 commit 上的 CI 对所有 为什么 defer 而非 approve。 正确性不是顾虑——我已对照基础代码核实机制、未发现阻塞项。原因是改动的"类别"：它横跨 ⏸️ 转交 @ytahdn —— 这是一次本来很干净的跨包协议 + SDK 新增，合并前需要 maintainer 对公开契约面（以及导出接口变更）拍板。需要人来定夺，而非正确性修复。 — Qwen Code · qwen3.8-max Reviewed at |
Resolve capability registry collisions: keep both workspace_session_live_state (this PR) and workspace_session_metadata (main) in the registry and its mirrors in serve tests.
ytahdn
left a comment
ytahdn
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
Not explored to full depth (tool budget reached): chunk 5: SDK vitest run ( packages/sdk-typescript DaemonClient.test.ts / daemon-public-surface.test.ts) hangs in this environment — no output even for the smallest test….
Not reviewed: "agent reverse-audit (round 1)" — the agent made no tool call: it read nothing.
Not reviewed: "agent reverse-audit (round 2)" — pointed at diff lines it never opened: it made tool calls, but none of them read the diff.
Not reviewed: the whole-diff test-coverage check, the build-and-test check — its prompt was built, but no agent on record was launched with it.
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
未探索到全部深度（达到工具调用预算）：chunk 5：SDK vitest run ( packages/sdk-typescript DaemonClient.test.ts / daemon-public-surface.test.ts) hangs in this environment — no output even for the smallest test…。
未审查："agent reverse-audit (round 1)"——该 agent 未发起任何工具调用：它什么都没读。
未审查："agent reverse-audit (round 2)"——启动 prompt 为它指定了 diff 中的行，但它从未打开：有工具调用，却没有一次读取 diff。
未审查：全 diff 测试覆盖检查、构建与测试验证——它的 prompt 已构建，但没有任何 agent 有记录用它启动过。
— DeepSeek/deepseek-v4-flash via Qwen Code /review (v0.21.11)
The metadata route's SessionNotFoundError fallback renamed persisted sessions without advancing the catalog revision, so version-watching clients kept the stale display name. Mark after a successful persisted rename (parity with the live path, which marks on an actual change). Also reconcile the design doc summary with its Implementation Boundaries (the implementation ships in this PR, not a follow-up) and spell out the child-recording persistence mechanism behind the auto-title catalog mark.
|
Addressed the @ytahdn /review round (3 inline suggestions) in 1c3f3ea, plus resolved the merge conflict against
Verification: |
Resolve the acp-session-bridge type-import collision: keep both BridgePromptContentBlock (session media references, QwenLM#9310) and BridgeSessionCatalogVersion (this PR). Media references pushed the daemon browser bundle to 156 B under the 196 KiB budget; adding this PR's live-state daemon surface tips it over, so bump the guard to 197 KiB with the house precedent comment.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
中文说明
已审查——无阻断问题。 建议见行内评论。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| ids((await organized('&archiveState=archived')).body).sort(), | ||
| ).toEqual([archivedOne, archivedTwo]); | ||
|
|
||
| // An unchanged high-frequency poll must not invalidate again: a third |
There was a problem hiding this comment.
[Suggestion] Both directions of this cache-invalidation test hinge on the real-time 2 s PERSISTED_SESSION_LIST_CACHE_TTL_MS window (no fake timers; assertions only check response ids) — Failure scenario: (a) flake: after the post-exposure refill, the remaining sequence must complete within 2 s; on a stalled/loaded CI runner the refilled entry expires, the rescan surfaces the hidden write, and the final toEqual fails intermittently; (b) vacuous pass: if the gap between the initial cache fill and the post-exposure organized queries exceeds 2 s, natural TTL expiry is indistinguishable from route invalidation — deleting invalidateSessionLists from the live-state route keeps the test green, losing the only end-to-end pin of invalidate-before-expose. Fix: make the assertions timing-independent — surface and assert the cache lookup status (cache_hit vs scan) on the organized queries, or inject a controllable TTL/clock for this test.
Witness (probe): baseline 3 runs pass in 126/146/138 ms; invalidateSessionLists removed + injected 2.2 s stall → mutant passes (2326 ms); route intact + 2.2 s stall before the final query → fails (received […201, …203, +…205]). Both directions flipped; tree restored.
中文说明
[建议] 该缓存失效测试的两个方向都依赖实时的 2 秒 PERSISTED_SESSION_LIST_CACHE_TTL_MS 窗口（无假时钟；断言只检查响应 id）— 失败场景：(a) 不稳定：曝光后重新填充缓存之后，剩余步骤必须在 2 秒内完成；在停顿/高负载的 CI runner 上，重填条目过期、重扫会把被隐藏的写入暴露出来，最终 toEqual 间歇性失败；(b) 空过：若首次缓存填充与曝光后 organized 查询之间的间隔超过 2 秒，自然 TTL 过期与路由失效不可区分 — 删除 live-state 路由中的 invalidateSessionLists 测试仍绿，失去对"先失效后曝光"的唯一端到端约束。修复：让断言与时间无关——在 organized 查询上暴露并断言缓存查找状态（cache_hit vs scan），或为该测试注入可控 TTL/时钟。
证据（探针）：基线 3 次运行分别 126/146/138 ms 通过；删除 invalidateSessionLists 并注入 2.2 秒停顿 → 变异通过（2326 ms）；路由保持完整但在最终查询前注入 2.2 秒停顿 → 失败（收到 […201, …203, +…205]）。两个方向均翻转验证；工作树已还原。
— qwen3.8-max via Qwen Code /review (v0.21.13)
There was a problem hiding this comment.
Declining for this round, reasoning recorded here. The timing-independence fix needs either a cache-lookup-status surface in responses or a controllable TTL/clock — both are test-infra changes wider than this review-round scope, and the same TTL constraint is shared by every cache test in this file, so a rewrite belongs in a dedicated change. Net today: the mechanism is pinned twice deterministically — the version-comparison test this thread anchors on, plus the new first-exposure test in 852b7ea that fills both scopes before any live-state request and asserts refills with the revision unchanged. The flake direction your probe measured is real but needs a >2 s stall mid-test (~13x the observed 130-150 ms budget); I would rather address it holistically than re-plumb TTL inside a review round. Happy to file a follow-up issue if you want it tracked.
Maintainer verification: built and ran this PR locally against a real daemon — no blocking issuesI built both the PR head ( Result: 49/49 end-to-end checks pass on the PR build, the same probe returns 404 / no capability on the base build, and 4 single-point mutations of the shipped bundle each kill exactly the checks they should. Two behavioural nuances worth a reviewer's eye are noted at the end; neither blocks merge. Environment
1. Discovery and wire contract (PR build vs base build)
Contract checks that passed on the PR build: exactly the three documented top-level keys; Trust and routing — verified against a daemon with
2. Catalog-version semantics against real REST mutationsEvery mutation below ran against seeded persisted sessions that were confirmed present in the catalog first (
Cache coherence also holds end to end: after a mutation, live A ≠ live B, the full catalog read issued after live B reflects the mutation inside the 2 s cache TTL, and live C equals live B so the handshake closes and the bundle is acceptable. The TypeScript SDK works against the live daemon: 3. The central claim: volatile state moves, the version does notThis is the part that only a real session can prove. A real live session (real
4. Cost, on a store built to be hostile1500 persisted sessions, 168 MiB of JSONL, measured at the sidebar's own 2 s cadence (rapid polling would sit inside the 2 s TTL and flatter the catalog path):
164–166× cheaper across two independent runs. Live-state latency is the same whether polled rapidly or at 2.1 s spacing (memory-only, no TTL to miss), its response size is independent of the persisted session count, and polling it spawned no additional daemon child process. 5. Falsification: are these checks actually load-bearing?I applied four single-point mutations to the built bundle, re-ran the same suites, and restored the bundle (sha256-verified back to the original):
Each mutation kills exactly the assertions it should and nothing else — the green run is not green by accident. 6. Repository test suites, run locally on this branch
(The PR description quotes 783 and 355 for the bridge and SDK suites; locally I measure 711 / 1541 and 358. Counts only — everything passes.) Notes for reviewers (non-blocking)
Verdict: the implementation behaves exactly as the protocol document describes, the security boundary is real and differentially demonstrated, the performance argument holds on a 168 MiB store, and the version-exactness claims survive targeted falsification. LGTM from my side as merge evidence. 中文版维护者验证：本地构建并在真实 daemon 上跑通本 PR —— 未发现阻塞问题我从源码分别构建了 PR head（ 结果：PR 构建上 49/49 端到端检查全绿；同一探针在 base 构建上返回 404 且不广播 capability；对已构建产物做的 4 个单点变异，各自精确杀死其应当杀死的检查。 文末有两条值得评审者留意的行为细节，均不阻塞合并。 环境
1. 能力发现与 wire 契约（PR 构建 vs base 构建）
PR 构建上通过的契约检查：顶层恰好三个文档化字段；每次成功响应均带 信任与路由 —— 在开启
2. 针对真实 REST mutation 的 catalog 版本语义下列每一项都作用在"先确认已出现在 catalog 中"的持久化会话上（
缓存一致性也端到端成立：mutation 后 live A ≠ live B；在 live B 之后发起的全量 catalog 读取在 2 秒缓存 TTL 之内即反映该 mutation；live C 等于 live B，握手闭合，bundle 可接受。 TypeScript SDK 对真实 daemon 可用： 3. 核心主张：volatile 状态会动，版本不动这一点只有真实会话能证明。真实 live session（真实
4. 在刻意做大的 store 上的开销1500 个持久化会话、168 MiB JSONL，按 sidebar 自身的 2 秒节奏测量（快速轮询会落在 2 秒 TTL 内，反而美化 catalog 路径）：
两次独立运行均为 164–166 倍更便宜。live-state 的时延在快速轮询与 2.1 秒间隔下相同（纯内存，没有 TTL 可错过），响应体大小与持久化会话数无关，且轮询它没有派生任何额外的 daemon 子进程。 5. 反证：这些检查真的有承载力吗我对已构建的产物施加了四个单点变异，重跑同样的套件，然后还原产物（sha256 校验与原始一致）：
每个变异精确杀死其应当杀死的断言，且不多杀 —— 绿色结果不是碰巧绿的。 6. 本地在本分支上跑的仓库测试套件
（PR 描述中 bridge 与 SDK 分别写的是 783 与 355；我本地测得 711 / 1541 与 358。仅计数差异，全部通过。） 给评审者的说明（非阻塞）
结论： 实现的行为与协议文档描述完全一致，安全边界真实存在并有差分证据，性能论证在 168 MiB store 上成立，版本精确性主张也经受住了定向反证。从我这边看，作为合并证据 LGTM。 |
…w round - Assert markSessionCatalogChanged in the scheduled-task rollback (including the no-op-removal negative case), the sub-session and Live coordinator rollback paths, and the never-live orphan deletion; previously each mark could regress with suites green. - Cover the live-state route's ?? false projection for both wait flags, and its first-exposure invalidation arm (revision unchanged, both organized scopes refilled). - Cover the side-task generation-closed rollback arm (kill, remove, catalog mark). - Compile the SDK live-state type fence via tsconfig.test-fence.json so shape assertions really pin the wire contract; the default tsconfig excludes test/. - Align the design doc's cache-consistency goal with the cache mechanics (waiters joined before an invalidation may resolve, but cannot install).
|
Addressed the qwen-code-ci-bot /review round (12 inline suggestions from 11:26 UTC) in 852b7ea (local HEAD == PR head). 10 accepted and pinned, 2 declined with reasoning in-thread.
Local verification (env-stripped, sequential): create-sub-session 35/35 · live-session-coordinator 24/24 · multi-workspace-sessions 117/117 · scheduled-tasks 86/86 · server 1002/1003 (sole failure is the pre-existing The 10 accepted threads are being batch-resolved; the 2 declined threads stay open for reviewer disposal. |
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
LGTM — approving after a full pass on head 852b7ea.
Scope & linkage. All 40 files match the stated goal: design doc, trusted-only GET /workspaces/:workspace/sessions/live-state route, in-memory catalog clock in the bridge, unconditional workspace_session_live_state capability, SDK wire types + accessors, telemetry tag, and docs. No unrelated changes.
Auth gate verified against siblings. The new route uses requireTrustedRuntimeForWorkspaceRoute (routes/session.ts) — strictly stronger than the catalog GET sibling (which permits untrusted-secondaries) and consistent with the workspace-qualified mutation routes; no primary fallback for unknown selectors; per-request trust check. As a read-only GET it correctly skips the client-id header, same as the sibling catalog list route.
Correctness verified from head sources:
- Catalog clock: per-bridge
generation(randomUUID) + monotonerevision; getters return fresh snapshots (bridge.ts). - Lifecycle choke point: all 7
byIdmap mutations (set/delete/clear) are paired withemitSessionLifecycle, and the mark fires before the host callback, so a throwingsessionLifecyclelistener cannot suppress a revision advance. - Invalidate-before-expose: the route synchronously invalidates both persisted cache scopes before answering a never-exposed version and records
lastExposedin a WeakMap keyed on the bridge (runtime replacements drop cleanly); double generation-guard assertions bracket the critical section. - No-op semantics preserved:
deleted: falsegroup deletes and unchanged renames do not advance the revision; the persisted-rename fallback now marks (round-1 fix). - Response surface is a strict subset of what the catalog route already exposes (sessionId + 4 volatile fields),
Cache-Control: no-store,v: 1envelope — no new data exposure. - SDK types match the route contract exactly;
mode: 'rest'pins native transport; the documented no-per-poll-capability-pre-flight tradeoff is sound; bundle budget bump is justified; the type-fence now has a real tsc compile pass via tsconfig.test-fence.json.
Review history & CI. Both prior rounds (3 + 12 suggestions) are fully dispositioned — accepted fixes are in this head, declines carry in-thread reasoning. The head commit beyond wenshao's real-daemon verification is tests/docs only, and all non-skipped checks on head are green (Test, Desktop Shell ubuntu/windows, Live Host macOS, Real daemon E2E matrix, Serve A/B, TruffleHog, CVE audit, precheck). The serve A/B diff confirms the only wire-visible change is the new capability tag. mergeable: true; the blocked state is only the pending approval.
ytahdn
left a comment
ytahdn
left a comment
There was a problem hiding this comment.
Verdict: Approve
No Criticals found across all review dimensions (correctness, security, performance, tests, reverse audit).
The implementation faithfully follows the design doc's protocol contract — the bridge clock (generation + revision), the no-await live-state route ordering, the WeakMap cache-exposure tracking, the trust gate before bridge access, and the invalidate-then-mark persisted mutation integration are all correct.
Additional suggestions (non-blocking, body-only)
4. session-archive.ts — The orphan mark test covers the positive case (persisted removal succeeds → mark fires), but no test covers the deleteDaemonSessionIfOrphan early-return path where the session is not an orphan (returns false before the mark). Consider adding a test that asserts the mark does NOT fire when the function returns false.
5. session.ts generation re-assertion — The assertRuntimeOpen?.() re-assertion after bridge reads guards against runtime replacement, but no test exercises the actual 503 path. Consider adding a test that closes the runtime generation between listWorkspaceSessions and the re-assertion.
6. deleteDaemonSessionIfOrphan unconditional mark (low confidence) — markSessionCatalogChanged fires unconditionally after persisted removal succeeds, even if killSession reported notFound. The design explicitly permits conservative extra increments, so this is likely by design. Flagged only for the author's consideration.
| }), | ||
| ); | ||
| invalidateSessionListsAndMarkCatalog(runtime, ['active', 'archived']); | ||
| res.status(201).json({ group }); |
There was a problem hiding this comment.
[Suggestion] Mid-request generation close untested
The second assertRuntimeOpen?.() guards against a runtime replacement between the bridge reads and the response — if the generation closed, this should throw and map to a 503. However, no test exercises this mid-request path.
Consider adding a test that closes the runtime generation between listWorkspaceSessions() and this re-assertion, verifying the response is 503.
| }); | ||
| }); | ||
|
|
||
| it('invalidates both organized cache scopes on the first live-state exposure, revision unchanged', async () => { |
There was a problem hiding this comment.
[Suggestion] Live→catalog→live handshake race untested
The client consistency handshake (live-state A → full catalog load → live-state B, comparing catalogVersion) is the core protocol guarantee, but no test covers the case where a mutation occurs between A and B. The first-exposure test here covers the case where revision is unchanged, but not where it changes between two live-state calls.
Consider adding a test that mutates between two live-state calls and asserts B's catalogVersion differs from A's, and that the pre-exposure invalidation fires before B answers.
| const transcriptRemoved = await new SessionService( | ||
| boundWorkspace, | ||
| ).removeSession(spawnedSession.sessionId); | ||
| if (transcriptRemoved) bridge.markSessionCatalogChanged(); |
There was a problem hiding this comment.
[Suggestion] Negative rollback path untested
The positive case (removeSession returns true → mark fires) is well-tested, but the negative path where removeSession returns false (transcript not found) is not covered. Consider adding a test where removeSession resolves to false and asserting markSessionCatalogChanged was NOT called.
Main moved past our base (last: QwenLM#9261 workspace session live-state), which conflicted with this branch on the sdk package.json typecheck script. Resolved by keeping both tsc projects (tsconfig.typetest.json from QwenLM#8978 and tsconfig.test-fence.json from QwenLM#9261). All other overlap (channel/serve/sdk files touched on both sides) auto-merged cleanly.
|
Released in v0.21.14. |
5 participants
What this PR does
This PR adds the workspace session live-state protocol — the reviewed design at
docs/design/2026-08-16-workspace-session-live-state.md— together with its implementation and documentation. A new trusted-only, memory-onlyGET /workspaces/:workspace/sessions/live-stateroute returns the complete volatile snapshot of the selected runtime's live sessions (client count, active-prompt and waiting flags) plus an in-memory catalog version: ageneration + revisionequality token over daemon-observed catalog membership and static-metadata changes. Clients therefore stop polling the persisted session catalog for volatile state and reload it only when the version says it may be stale, using the documentedlive A -> full catalog -> live Breconciliation handshake.The bridge owns the clock: live registration and removal advance through the single
emitSessionLifecyclechoke point; manual rename, child automatic titles, worktree updates, and persisted branch commits (including persisted-only forks and forks committed before a failed restore) mark at exact points; REST and ACP catalog mutations share an invalidate-then-mark helper that preserves exact no-op semantics (deleted: falsegroup deletes,removeSession: falsecleanups, no-op renames). The route exposes a newly observed version only after invalidating both persisted catalog cache scopes, so a client can never accept a catalog snapshot that predates the version it observed. A new unconditionalworkspace_session_live_statecapability advertises the route, the TypeScript SDK gains the three wire types andDaemonClient.getWorkspaceSessionLiveState/WorkspaceDaemonClient.getSessionLiveState(native REST, no per-poll capability pre-flight), and telemetry, the protocol reference, capability versioning docs, and SDK docs are updated.Why it's needed
GET /workspaces/:workspace/sessionsis a persisted catalog query, not a live-status probe: the default numeric-cursor path re-reads storage for every request (page size capped at 100), and the organized and metadata-filtered paths use a two-second cache whose TTL matches the sidebar's two-second active cadence, so steady polling repeatedly triggers full-workspace scans. Coupling small volatile updates (active, waiting, client count) to that path turns a routine sidebar refresh into a request that can time out on large session stores even while the daemon and its ACP child are healthy. E2E on this branch with a 1500-file (~150 MB) seeded store: one organized full-catalog scan ~1.0 s vs live-state ~1.1 ms across ten polls, with no ACP child spawned.The protocol is intentionally two independent signals: a cheap complete snapshot for volatile state, and an equality token that says when the persisted catalog may be stale. Ordinary turn activity, prompt lifecycle, attach/detach, and waiting-state transitions deliberately stay out of the version — the snapshot already carries those values.
Reviewer Test Plan
How to verify
cd packages/acp-bridge && npx vitest run src/bridge.test.ts -t "session catalog version clock"— 12/12 covers immutable version snapshots, the lifecycle choke point with a throwing host listener, exact rename/auto-title/worktree gates, persisted-only branch and committed-branch-restore-failure marks, no mark on failed mutations, and no marks for prompt/attach/heartbeat/permission-wait transitions; the full bridge suites are green (783/783).cd packages/cli && npx vitest run src/serve/multi-workspace-sessions.test.ts src/serve/acp-http/transport.test.ts src/serve/server/telemetry.test.ts src/serve/server/session-archive.test.ts src/serve/scheduled-task-keepalive.test.ts src/serve/live/live-task-service.test.ts src/serve/create-sub-session.test.ts src/serve/server.test.ts— route shape with the exact five-field projection andCache-Control: no-store, trusted-primary/secondary isolation, untrusted 403 before any bridge read, unknown-selector 400 with no primary fallback, transitioning-generation 503 +Retry-After, exposure-time invalidation of both catalog cache scopes with the version-comparison arm explicitly pinned, REST + ACP mutation matrices with exactdeleted: false/ no-op semantics, capability feature lists, telemetry label, and cleanup-path marks.cd packages/sdk-typescript && npx vitest run test/unit/DaemonClient.test.ts test/unit/daemon-public-surface.test.ts— encoded workspace selector, exactly one HTTP request with no capability pre-flight, native-REST bypass of a configured ACP transport, and public-surface type pins (355/355).node dist/cli.js serve(isolated runtime dir):workspace_session_live_stateadvertised; unknown selector returns 400; seeded persisted sessions advance the version exactly once per REST mutation (organization pin, group create/update/delete withdeleted: falseunchanged, archive/unarchive/delete), with the archived catalog reflecting each mutation immediately; a held streaming turn from a mock OpenAI server flipshasActivePromptwhile the version is unchanged; a daemon restart changesgenerationand resetsrevisionto 0; on the 1500-file seeded store, live-state stays ~1.1 ms while the organized catalog scan costs ~1 s. NoteQWEN_RUNTIME_DIRmust be unset or isolated when running repo vitest suites locally, since a leaked parent-session value produces phantom Storage failures.Evidence (Before & After)
N/A — new endpoint, no existing UI surface changes.
Tested on
Environment (optional)
npm run build && npm run bundle,node dist/cli.js servewith an isolatedQWEN_RUNTIME_DIR, and a slowed mock OpenAI server for the held-turn case.Risk & Scope
getSessionCatalogVersion/markSessionCatalogChangedmethods on the exportedAcpSessionBridgeinterface are a source-level contract change for external structural bridge implementations and complete typed test fakes (all in-repo fakes are updated). The wire protocol is additive; older daemons simply omit the capability tag, and clients discover support from it.updatedAt/ordering staleness between version changes (documented). Windows/Linux coverage is left to CI.AcpSessionBridgeimplementers must add the two in-memory clock methods on upgrade. Conservative extra revision increments are protocol-permitted and expected on partial mutations (e.g. batch archive/delete fromfinally).Linked Issues
N/A — this PR first landed its own reviewed design document (
docs/design/2026-08-16-workspace-session-live-state.md) and now lands the implementation plus documentation of the same protocol.中文说明
本 PR 做什么
本 PR 在既有评审过的设计文档（
docs/design/2026-08-16-workspace-session-live-state.md）基础上，交付 workspace session live-state 协议的完整实现与文档。新增 trusted-only、纯内存的GET /workspaces/:workspace/sessions/live-state路由：返回所选 runtime 全部 live session 的 volatile 快照（clientCount、hasActivePrompt、waiting 标志），以及一个内存 catalog 版本——generation + revision等值令牌，覆盖 daemon 可观测的 catalog 成员与静态元数据变化。客户端由此不再用持久化 catalog 轮询 volatile 状态，只在版本提示过期时按文档的live A -> full catalog -> live B握手重新加载。时钟由 bridge 持有：live 注册/移除经唯一的
emitSessionLifecycle收口点打标；手动重命名、子进程自动标题、worktree 更新、持久化 branch 提交（含 persisted-only fork 与 restore 失败但已提交的 fork）在精确点位打标；REST/ACP catalog mutation 共享"先失效后打标"的 helper，并保留精确 no-op 语义（deleted: false的 group 删除、removeSession: false的清理、no-op 重命名均不递增）。路由只有在失效 active+archived 两个 catalog 缓存作用域后才曝光新版本，保证客户端不会接受到早于所观测版本的 catalog 快照。新增无条件 capabilityworkspace_session_live_state；TypeScript SDK 增加三个 wire 类型与DaemonClient.getWorkspaceSessionLiveState/WorkspaceDaemonClient.getSessionLiveState（原生 REST，不做逐轮 capability 探测）；telemetry、协议参考、capability 版本化与 SDK 文档同步更新。为什么需要
GET /workspaces/:workspace/sessions是持久化 catalog 查询而不是 live 状态探针：默认数字游标路径每次都现扫存储（页面上限 100），organized/metadata 路径的缓存 TTL 为 2 秒、恰与 sidebar 活跃轮询同周期，稳态轮询会反复触发全量扫描。把 active/waiting/clientCount 等小状态更新耦合到这条最贵的路径，会让大 session store 上的常规刷新演变成超时。本分支实测：1500 个文件（约 150 MB）的 store 上 organized 全量扫描约 1.0 秒，live-state 十次轮询约 1.1 毫秒，且不启动 ACP 子进程。协议刻意拆成两个信号：便宜的完整 volatile 快照 + 提示 catalog 可能过期的等值令牌；普通 turn 活动、prompt 生命周期、attach/detach、等待状态均不推进版本。Reviewer 验证
packages/acp-bridge的"session catalog version clock"套件 12/12：不可变快照、host 回调抛错仍打标的收口点、精确 rename/auto-title/worktree 门控、persisted-only branch 与提交后 restore 失败仍打标、失败 mutation 不打标、prompt/attach/心跳/等待不推进；桥全套件 783/783 绿。packages/cli相关套件：路由精确五字段投影 + no-store、主/次工作区隔离、untrusted 403 先于任何 bridge 调用、未知 selector 400 不回退 primary、transitioning 503+Retry-After、钉住首次曝光后验证版本比较分支的双作用域失效、REST+ACP mutation 矩阵（含deleted: false）、capability 清单、telemetry 标签、清理路径打标。packages/sdk-typescript：selector 编码、恰好一次请求且无 capability 探测、配置 ACP transport 时仍走原生 REST、公开面类型 pin（355/355）。deleted:false）/archive/unarchive/delete 各精确递增一次且归档视图即时生效；慢速 mock 模型的长流式 turn 中hasActivePrompt翻转而版本不变；重启后 generation 变化、revision 归零；1500 文件 store 上 live-state ~1.1ms vs catalog ~1s。注意本地跑测试需解除或隔离QWEN_RUNTIME_DIR，否则父会话泄漏会产生幻影 Storage 失败。风险与范围
AcpSessionBridge接口新增两个必需方法，对外部结构化实现与完整 typed fake 构成源码级契约变化（仓内 fake 已全部更新）；wire 协议为增量兼容，旧 daemon 不广播该 capability 即可。关联 Issue
N/A——本 PR 先合入了经过评审的自有设计文档，现合入同一协议的实现与文档。