Skip to content

feat(serve): query a single session's status by id - #5857

Merged
samuelhsin merged 7 commits into
QwenLM:mainfrom
samuelhsin:feat/serve-single-session-status
Jun 25, 2026
Merged

feat(serve): query a single session's status by id#5857
samuelhsin merged 7 commits into
QwenLM:mainfrom
samuelhsin:feat/serve-single-session-status

Conversation

@samuelhsin

@samuelhsin samuelhsin commented Jun 25, 2026

Copy link
Copy Markdown
Collaborator

What this PR does

Adds a daemon HTTP endpoint that returns the live status of a single session looked up by its id — session id, workspace cwd, creation time, optional display name, attached client count, and whether a prompt is currently active. The route answers 200 with that summary when the daemon is hosting a live session with the given id, and 404 (body { error, sessionId }) when the id is unknown to the daemon. This is the raw live-session view from the bridge: the two fields the use case depends on — clientCount and hasActivePrompt — match the corresponding entry in the workspace session list, but the routes are not byte-identical, because the list endpoint enriches each item with persisted session-store data (its createdAt is the persisted first-prompt time, plus an updatedAt and a derived displayName) while /status reports the live session's own createdAt, omits updatedAt, and returns displayName only when one is set on the live session.

Why it's needed

Until now the only way to read a session's live state was the full, paginated workspace session list. A caller that already holds a session id and only wants to know "is this one session still running, and how many clients are attached?" had to fetch every page and filter on the client side — wasteful and racy as the session list grows. A direct by-id lookup is the natural primitive for that question. The motivating case is a client UI that drives a session and wants to reflect its run state in the surrounding controls — for example disabling buttons that must not be clicked mid-run, or showing a "task in progress" hint — by polling that one session's active-prompt flag until its task completes.

The status data already lives on the bridge, so the change is small: a single by-id accessor that shares the same summary builder as the list path, the new route, unit tests on both layers, and a documentation note.

Reviewer Test Plan

How to verify

Start a daemon (qwen serve), open a session, and request its status by id:

curl http://127.0.0.1:4170/session/$SESSION_ID/status
# → 200 {"sessionId":"…","workspaceCwd":"…","createdAt":"…","clientCount":1,"hasActivePrompt":false}

Expected: 200 with the summary object for a live session id; while a prompt is running on that session, hasActivePrompt reads true; clientCount tracks attached clients. Requesting an id the daemon does not host returns 404 with the session id echoed in the error body.

Unit coverage: the bridge accessor returns the summary for a known id and throws for an unknown one; the route returns 200 with the summary and 404 when the session is unknown. Run cd packages/acp-bridge && npx vitest run src/bridge.test.ts and cd packages/cli && npx vitest run src/serve/server.test.ts.

Evidence (Before & After)

N/A — daemon HTTP API addition, no user-visible TUI change.

Tested on

OS Status
🍏 macOS
🪟 Windows ⚠️
🐧 Linux ⚠️

Environment (optional)

Unit tests via vitest; both affected packages typecheck and lint clean.

Risk & Scope

  • Main risk or tradeoff: low — additive read-only route over data the bridge already exposes; reuses the existing summary builder so the single-session and list shapes can't drift.
  • Not validated / out of scope: no SDK client or MCP wrapper for the new route in this PR (the daemon route is the primitive); not exercised on Windows/Linux locally (CI covers those).
  • Breaking changes / migration notes: none.

Linked Issues

Closes #5855

中文说明

这个 PR 做了什么

新增一个 daemon HTTP 接口,按 session id 查询单个 session 的实时状态——session id、工作区 cwd、创建时间、可选的显示名、已连接客户端数,以及当前是否有正在执行的 prompt。当 daemon 持有该 id 对应的存活 session 时返回 200 及该 summary,id 未知时返回 404(响应体 { error, sessionId })。这是来自 bridge 的原始实时视图:用例真正依赖的两个字段 clientCounthasActivePrompt 与工作区 session 列表中对应项一致,但两个接口并非逐字节相同——列表接口会用持久化 session store 数据对每项做富化(其 createdAt 是持久化的首个 prompt 时间,另含 updatedAt 和派生的 displayName),而 /status 返回存活 session 自身的 createdAt、不含 updatedAt,且仅在存活 session 设置了显示名时才返回 displayName

为什么需要

此前读取 session 实时状态的唯一方式是拉取完整的、分页的工作区 session 列表。一个已经握着 session id、只想知道"这个 session 还在跑吗、连了几个客户端"的调用方,必须翻完所有分页再在客户端自己过滤——列表一大就低效且有竞态。按 id 直接单查才是这个问题的自然原语。典型场景是一个驱动 session 的客户端 UI,希望据此在周边控件上反映运行状态——比如禁用运行期间不可点击的按钮、或显示"任务进行中"提示——通过轮询该 session 的 active-prompt 标志,直到其任务完成。

状态数据本来就在 bridge 上,所以改动很小:一个共用列表路径同一 summary 构建逻辑的按 id 访问器、新路由、两层的单测,以及一段文档说明。

验证方式

启动 daemon(qwen serve),开一个 session,按 id 请求其状态:对存活的 session id 返回 200 及 summary;prompt 运行期间 hasActivePrompttrueclientCount 跟踪已连接客户端;请求 daemon 未持有的 id 返回 404 并在错误体中回显该 session id。单测覆盖 bridge 访问器与路由的命中/未命中两条路径。

风险与范围

  • 主要风险/取舍:低——只读、附加的路由,数据是 bridge 已暴露的;复用既有 summary 构建逻辑,单查与列表的结构不会漂移。
  • 未验证/范围外:本 PR 不含新路由的 SDK 客户端或 MCP 封装(daemon 路由是原语);本地未在 Windows/Linux 上跑(由 CI 覆盖)。
  • 破坏性变更:无。

关联 issue:Closes #5855

Add a daemon HTTP endpoint, GET /session/:id/status, that returns the
live status summary for one session by its id — the same per-item shape
that the workspace session list produces (sessionId, workspaceCwd,
createdAt, displayName, clientCount, hasActivePrompt). It answers 200
with the summary when the daemon holds a live session with that id, and
404 when the id is unknown.

Previously the only way to read a session's live state was the full
paginated workspace session list, forcing a caller that already holds a
session id to fetch every page and filter client-side just to answer
"is this session still running?". A by-id lookup is the natural
primitive for polling a single known session's hasActivePrompt /
clientCount — for example, a client UI that disables controls or shows a
"task in progress" hint while a specific session is running.

The data already exists on the bridge, so this adds one accessor
(getSessionSummary) sharing the same summary builder as the list path,
one route, unit tests, and a docs note.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@wenshao

wenshao commented Jun 25, 2026

Copy link
Copy Markdown
Collaborator

@qwen-code /triage

@qwen-code-ci-bot

qwen-code-ci-bot commented Jun 25, 2026

Copy link
Copy Markdown
Collaborator

Thanks for the PR!

Template looks good ✓

On direction: this directly addresses issue #5855 — a real pain point for client UIs that need to poll a single session's hasActivePrompt / clientCount without paginating through the full workspace session list. The daemon HTTP API layer is a core part of qwen-code's serve story, and adding a by-id lookup is the natural primitive. CHANGELOG has a pattern of serve/daemon session features landing over recent releases (session reaper, displayName unification, web-shell branching), confirming this is an active development area. Aligned.

On approach: the scope is tight and focused — a shared toSessionSummary helper to DRY the existing inline object literal, a getSessionSummary bridge method, the new GET /session/:id/status route, unit tests on both layers, and a docs paragraph. Every line in the diff serves the stated goal; no drive-by refactors or scope creep. Moving on to code review. 🔍

中文说明

感谢贡献!

模板完整 ✓

方向:直接解决 issue #5855——客户端 UI 需要轮询单个 session 的 hasActivePrompt / clientCount,而不必翻页拉取整个工作区 session 列表。daemon HTTP API 是 qwen-code serve 能力的核心,按 id 单查是自然的原语。CHANGELOG 中近期有一系列 serve/daemon session 相关功能(session reaper、displayName 统一、web-shell 分支),说明这是活跃的开发方向。完全对齐。

方案:范围紧凑聚焦——共享 toSessionSummary 帮助函数消除已有内联对象的重复、bridge 层 getSessionSummary 方法、新的 GET /session/:id/status 路由、两层单测、以及一段文档说明。diff 中每一行都服务于声明的目标;没有顺手重构或范围蔓延。进入代码审查 🔍

Qwen Code · qwen3.7-max

@qwen-code-ci-bot

qwen-code-ci-bot commented Jun 25, 2026

Copy link
Copy Markdown
Collaborator

Code Review

Independent proposal matched the PR's approach exactly: extract a toSessionSummary helper to DRY the existing inline object in listWorkspaceSessions, add a getSessionSummary accessor on the bridge interface that throws SessionNotFoundError for unknown IDs, and wire a new GET /session/:id/status route that follows the same requireSessionId → try/catch → sendBridgeError pattern as the adjacent session routes. The PR lands all three cleanly.

No correctness bugs, security holes, or regressions found. The toSessionSummary helper sits inside the createAcpSessionBridge closure near byId, which is the right scope — it closes over the same SessionEntry type and can't be called from outside. The route handler is a synchronous try/catch (matching getContext/getSupportedCommands), which is correct since getSessionSummary is synchronous. Unit coverage is solid: bridge tests verify both the hit path (including shape parity with the list builder) and the SessionNotFoundError path; server tests verify 200 with summary body and 404 with sessionId echoed back.

No AGENTS.md violations. No over-abstraction, no duplication, no code in the wrong package.

Real-Scenario Testing

Built the bundle from the PR branch and started qwen serve on port 4199. Verified both paths against the live daemon:

$ curl -s -w '\nHTTP %{http_code}\n' http://127.0.0.1:4199/session/ghost-id/status
{"error":"No session with id \"ghost-id\"","sessionId":"ghost-id"}
HTTP 404

$ curl -s -X POST http://127.0.0.1:4199/session -H 'Content-Type: application/json' \
    -d '{"message":"hello","clientId":"triage-test"}'
{"sessionId":"b0f84459-63f4-415a-b92b-ef8c232cbb0c","workspaceCwd":"/home/github-runner/actions-runner-14/_work/qwen-code/qwen-code/.qwen/worktrees/triage","attached":false,"clientId":"client_1ec45ea9-b947-447b-b587-cab96e0ac5ac","createdAt":"2026-06-25T09:31:44.903Z"}

$ curl -s -w '\nHTTP %{http_code}\n' http://127.0.0.1:4199/session/b0f84459-63f4-415a-b92b-ef8c232cbb0c/status
{"sessionId":"b0f84459-63f4-415a-b92b-ef8c232cbb0c","workspaceCwd":"/home/github-runner/actions-runner-14/_work/qwen-code/qwen-code/.qwen/worktrees/triage","createdAt":"2026-06-25T09:31:44.903Z","clientCount":1,"hasActivePrompt":false}
HTTP 200

Daemon log confirming both routes:

2026-06-25T09:31:19.787Z [WARN] [DAEMON] route=GET /session/nonexistent-id/status sessionId=nonexistent-id durationMs=4 status=404 request completed
2026-06-25T09:31:44.904Z [INFO] [DAEMON] sessionId=b0f84459-63f4-415a-b92b-ef8c232cbb0c clientId=client_1ec45ea9-b947-447b-b587-cab96e0ac5ac session spawned
2026-06-25T09:32:03.846Z [INFO] [DAEMON] route=GET /session/b0f84459-63f4-415a-b92b-ef8c232cbb0c/status sessionId=b0f84459-63f4-415a-b92b-ef8c232cbb0c durationMs=1 status=200 request completed

Both paths behave exactly as described: 404 with sessionId echoed back for unknown IDs, 200 with the live summary for a hosted session. The clientCount correctly reflects the attached client and hasActivePrompt is false (no prompt in flight). No displayName in the response (correct — only set when the live session has one).

Unit Tests

✓ packages/acp-bridge/src/bridge.test.ts     298 passed
✓ packages/cli/src/serve/server.test.ts      528 passed
中文说明

代码审查

独立提案与 PR 方案完全一致:提取 toSessionSummary 帮助函数消除 listWorkspaceSessions 中的内联对象重复,在 bridge 接口上添加 getSessionSummary 访问器(对未知 id 抛出 SessionNotFoundError),新增 GET /session/:id/status 路由遵循与其他 session 路由相同的 requireSessionId → try/catch → sendBridgeError 模式。PR 三项落地都很干净。

未发现正确性 bug、安全漏洞或回归。toSessionSummary 放在 createAcpSessionBridge 闭包内 byId 附近,作用域正确。路由处理器是同步 try/catch(与 getContext/getSupportedCommands 一致),因为 getSessionSummary 是同步的。单测覆盖完善:bridge 层命中(含与列表构建器的结构一致性校验)和 SessionNotFoundError 路径;server 层 200 带 summary body 和 404 带回 sessionId

无 AGENTS.md 违规。无过度抽象、无重复、无错放位置的代码。

真实场景测试

从 PR 分支构建 bundle,在 4199 端口启动 qwen serve。对真实 daemon 验证了两条路径:

  • 404:未知 id 返回 {"error":"...","sessionId":"ghost-id"} + HTTP 404 ✓
  • 200:POST /session 创建会话后,GET /session/:id/status 返回完整 summary,clientCount 正确反映已连接客户端,hasActivePromptfalse

daemon 日志确认两条路由均正常处理。

单测全部通过:bridge 298 个、server 528 个。

Qwen Code · qwen3.7-max

@qwen-code-ci-bot

qwen-code-ci-bot commented Jun 25, 2026

Copy link
Copy Markdown
Collaborator

This is one of those PRs where everything lines up. The motivation is clear (issue #5855 — client UIs shouldn't have to paginate through every session just to check one), the implementation is minimal (shared helper + bridge accessor + route + tests + docs), and the live-daemon test confirms both paths work exactly as described.

My independent proposal was identical to the PR's approach — there's really only one sensible way to add this: DRY the summary builder, expose it by ID on the bridge, add a GET route. The PR doesn't over-engineer, doesn't carry unrelated changes, and follows every existing pattern in the codebase. The toSessionSummary extraction is a nice side benefit — it guarantees the single-item and list shapes can never drift.

Live testing confirmed: 404 returns the session ID in the error body (useful for debugging), 200 returns the correct live summary with clientCount: 1 for an attached client and hasActivePrompt: false. All 826 unit tests pass.

Approving. ✅

中文说明

这是一个各方面都对齐的 PR。动机清晰(issue #5855——客户端 UI 不该为了查一个 session 而翻页拉取全部),实现最小化(共享帮助函数 + bridge 访问器 + 路由 + 测试 + 文档),真实 daemon 测试确认两条路径都按描述工作。

我的独立提案与 PR 方案完全一致——这个问题本质上只有一种合理的实现方式:去重 summary 构建器、按 id 暴露到 bridge、添加 GET 路由。PR 没有过度工程化,没有夹带无关改动,遵循代码库中所有既有模式。toSessionSummary 的提取是一个额外好处——保证单查和列表的结构永远不会漂移。

真实测试确认:404 在错误体中返回 session ID(便于调试),200 返回正确的实时 summary,clientCount: 1 对应已连接客户端,hasActivePrompt: false。全部 826 个单测通过。

批准 ✅

Qwen Code · qwen3.7-max

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, looks ready to ship. ✅

@wenshao

wenshao commented Jun 25, 2026

Copy link
Copy Markdown
Collaborator

Local runtime verification — PR #5857

Verdict: functionally verified. I drove the new GET /session/:id/status route against a real qwen serve daemon (built bundle, real bridge over a socket) — 200/404 behave as specified, and the two live fields the use case depends on (hasActivePrompt, clientCount) are accurate and update in real time. Unit tests pass and are load-bearing; both changed packages typecheck clean. One non-blocking accuracy note about the "same shape as the session list" claim is below — worth a doc tweak before merge.

Method & environment
  • Base (BEFORE): origin/main @ 07beac1dd
  • Merged (AFTER): clean auto-merge of origin/main + PR head 44e1f71c26a5435690. The PR is 16 commits behind main and main has since touched 4 of the 6 PR files (bridge.ts, bridge.test.ts, bridgeTypes.ts, server.test.ts) — so I verified the merge result, not the PR branch in isolation. Merge is clean and the merged diff is exactly the PR's +145 / −9; the merge preserved getSessionSummary/toSessionSummary/the route intact.
  • Two worktrees, full real build chain → dist/cli.js serve. Bundle freshness: the session/:id/status + getSessionSummary strings appear in the merged bundle (4×) and in base.
  • Daemon driven over HTTP with a mock OpenAI provider (a held-prompt mode keeps promptActive=true for the hasActivePrompt check). node v22.22.2, Linux, tmux 3.5a.

Note: the route's unit tests use a fake bridge, so only a live daemon proves the real bridge.getSessionSummary ↔ server wiring end-to-end.

1. Live daemon E2E (the headline)

Real daemon, real bridge, real socket — GET /session/:id/status:

[1] AFTER  GET /session/<known-sid>/status        → 200
    {"sessionId":"02387ccd-…","workspaceCwd":"/root/wt/e2e5857/ws",
     "createdAt":"…T08:57:19.908Z","clientCount":1,"hasActivePrompt":false}

[2] AFTER  GET /session/ghost-xyz/status           → 404   (id echoed)
    {"error":"No session with id \"ghost-xyz\"","sessionId":"ghost-xyz"}

[3] BEFORE GET /session/<sid>/status               → 404 HTML "Cannot GET …"  (route absent)

Dynamic fields track real state:

Probe Result
clientCount 1 → second POST /session (same cwd, attached:true, new clientId) → 2
hasActivePrompt false → held prompt (202 async) → true (stable across polls) → POST /session/:id/cancel (204) → false

So the motivating use case — "poll one known session's run-state without scanning the whole list" — works exactly as described: hasActivePrompt flips with the prompt lifecycle and clientCount reflects attached clients. ✅

2. Unit tests, typecheck, revert-proof (merged tree)

Check Result
acp-bridge/src/bridge.test.ts 299/299 (incl. 2 new getSessionSummary: known-id summary + toEqual parity with the bridge list builder; unknown-id throws SessionNotFoundError)
cli/src/serve/server.test.ts 528/528 (incl. 2 new route: 200-with-summary, 404-unknown-with-id-echoed)
tsc --noEmit acp-bridge / cli 0 / 0 errors
Revert-proof revert bridge.ts→base ⇒ 2 tests fail (getSessionSummary is not a function); revert server.ts→base ⇒ 2 tests fail (404≠200, body sessionId undefined); restore ⇒ all green

The shared-helper refactor (toSessionSummary now used by both the list path and the new accessor) caused no regression in the full 299- and 528-test suites.

3. Non-blocking note — "same shape as the list" is only true at the bridge layer

The PR description and the docs say the response is "the same item shape that GET /workspace/:id/sessions lists (sessionId, workspaceCwd, createdAt, displayName?, clientCount, hasActivePrompt)". At the bridge layer that's exactly right — getSessionSummary and listWorkspaceSessions both call toSessionSummary, and the new unit test pins summary).toEqual(fromList).

But the HTTP /workspace/:id/sessions endpoint does not return raw bridge summarieslistWorkspaceSessionsForResponse merges in persisted SessionService data. For the same live session the two HTTP routes diverge:

GET /session/<sid>/status                    GET /workspace/<ws>/sessions  (same session)
  sessionId        ✔ equal                     sessionId
  workspaceCwd     ✔ equal                     workspaceCwd
  createdAt  08:57:19.908Z   ◀── DIFFERS ──▶   createdAt  08:58:16.397Z   (persisted startTime)
  (no updatedAt)             ◀── list adds ──  updatedAt  08:58:29.848Z
  (displayName omitted)      ◀── list adds ──  displayName "HOLD please keep running"
  clientCount      ✔ 2  ==  2                  clientCount
  hasActivePrompt  ✔ false == false            hasActivePrompt
  • createdAt: by-id returns the bridge spawn time; the list returns the persisted startTime (first-prompt time) → different values for the same session.
  • updatedAt: the list HTTP response includes it; /status does not.
  • displayName: the list derives it from customTitle || prompt; /status uses entry.displayName, which is usually unset → the field is omitted.

The two fields that matter for the stated use case (hasActivePrompt, clientCount) are identical in both, so this doesn't block the feature. But a client that switches from list-scan to by-id (the PR's own motivation) will see a different createdAt, no updatedAt, and a missing displayName. Suggest the doc say the route returns the live bridge summary (and note createdAt/displayName are the live-session values, not the enriched/persisted ones) rather than implying byte-parity with the list endpoint. (Minor, related: the 404 body is {error, sessionId} with no code field — consistent with every other session route, but the docs' "404 session_not_found" reads like a code that isn't actually returned.)

Verified on the real esbuild bundle via a live qwen serve daemon + mock provider. Happy to share the harness or re-run anything.

🇨🇳 中文说明(点击展开)

本地运行验证 — PR #5857

结论:功能验证通过。真实的 qwen serve daemon(构建产物、真实 bridge、走 socket)上验证了新增的 GET /session/:id/status:200/404 行为符合规格;用例真正依赖的两个实时字段(hasActivePromptclientCount)准确且实时更新。单测全绿且可证伪;两个改动的包 tsc 干净。下面有一个非阻塞的准确性说明——关于"与 session 列表结构一致"的措辞,建议合并前微调文档。

方法与环境

  • BEFORE:origin/main @ 07beac1dd;AFTER:origin/main + PR 头 44e1f71c2 的干净自动合并(6a5435690)。PR 落后 main 16 个提交,且 main 已改动了 6 个 PR 文件中的 4 个,所以我验证的是合并结果而非 PR 分支本身。合并干净,合并 diff 恰为 PR 的 +145 / −9getSessionSummary/toSessionSummary/路由均完整保留。
  • 注意:路由的单测用的是假 bridge,所以只有真实 daemon 才能端到端证明真实 bridge.getSessionSummary ↔ server 的接线。

1. 真实 daemon E2E(核心证据)

  • 已知 sid → 200 + summary(clientCount:1, hasActivePrompt:false)。
  • 未知 id → 404 且回显 id:{"error":"No session with id \"ghost-xyz\"","sessionId":"ghost-xyz"}
  • BEFORE(base)→ 404 HTML "Cannot GET …"(路由不存在)——干净的 A/B 信号。
  • clientCount1 →(同 cwd 第二次 POST /sessionattached:true)→ 2
  • hasActivePromptfalse →(挂起的 prompt)→ true(多次轮询稳定)→(POST /cancel 204)→ false
  • 即 PR 的目标用例"按 id 轮询单个 session 运行态、无需翻列表"完全可用。✅

2. 单测 / 类型检查 / 可证伪(合并树)

  • bridge.test.ts 299/299(含 2 个新 getSessionSummary:命中返回 summary 且 toEqual bridge 列表项、未命中抛 SessionNotFoundError)。
  • server.test.ts 528/528(含 2 个新路由:200 命中、404 未命中且回显 id)。
  • tsc --noEmit acp-bridge / cli:0 / 0 错误。
  • 可证伪:bridge.ts 还原到 base → 2 个测试失败(getSessionSummary is not a function);server.ts 还原到 base → 2 个测试失败(404≠200、body sessionId 为 undefined);恢复后全绿。
  • 共享 toSessionSummary(列表与新访问器共用)的重构在 299/528 全量套件中无回归

3. 非阻塞说明 ——"与列表结构一致"只在 bridge 层成立
PR 描述与文档称返回"与 GET /workspace/:id/sessions 列表项相同的结构"。在 bridge 层确实如此(两者都用 toSessionSummary,新单测也 toEqual 钉住了)。但 HTTP 列表接口并不返回原始 bridge summary——listWorkspaceSessionsForResponse 会并入持久化的 SessionService 数据。对同一个存活 session,两个 HTTP 接口存在差异:

  • createdAt:by-id 返回 bridge 创建时间;列表返回持久化的 startTime(首个 prompt 时间)→ 同一 session 两值不同(实测 08:57:19 vs 08:58:16)。
  • updatedAt:列表 HTTP 响应有;/status 没有。
  • displayName:列表由 customTitle || prompt 派生;/statusentry.displayName,通常未设置 → 该字段被省略。

用例真正关心的 hasActivePromptclientCount 两者完全一致,所以不影响功能。但一个从"翻列表"切到"按 id"(正是本 PR 的动机)的客户端,会看到不同的 createdAt、没有 updatedAt、缺失 displayName。建议文档改为"返回实时 bridge summary"(并说明 createdAt/displayName 是存活 session 的实时值,而非列表的持久化/富化值),而不要暗示与列表接口逐字节一致。(次要:404 体为 {error, sessionId},没有 code 字段——与其他所有 session 路由一致,但文档里"404 session_not_found"读起来像一个实际并不返回的 code。)

(以上均在真实 esbuild 产物 + 真实 qwen serve daemon + mock provider 上验证;需要 harness 或复跑可随时告知。)

Comment thread docs/users/qwen-serve.md Outdated
`GET /session/:id/tasks`, and `GET /session/:id/lsp`.

`GET /session/:id/status` returns the live status summary for a single
session — the same item shape that `GET /workspace/:id/sessions` lists

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] The docs state this returns "the same item shape that GET /workspace/:id/sessions lists", but the list endpoint enriches updatedAt from SessionService persisted data (file mtime) via listWorkspaceSessionsForResponse(). The new route calls bridge.getSessionSummary() directly, which never populates updatedAtSessionEntry has no such field.

Clients relying on this "same shape" contract will see updatedAt missing from the single-item response while it is always present in the list response.

Either update the docs to describe the subset (e.g., "a live-only summary with the same core fields"), or enrich the response in the route handler to match the list endpoint:

app.get('/session/:id/status', async (req, res) => {
  const sessionId = requireSessionId(req, res);
  if (sessionId === null) return;
  try {
    const live = bridge.getSessionSummary(sessionId);
    const existing = await sessionService.getSession(sessionId);
    res.status(200).json({
      ...live,
      updatedAt: existing?.mtime
        ? new Date(existing.mtime).toISOString()
        : undefined,
    });
  } catch (err) {
    sendBridgeError(res, err, { route: 'GET /session/:id/status', sessionId });
  }
});

— qwen3.7-max via Qwen Code /review

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in e029794 — reworded to describe the live bridge summary and spelled out that the list endpoint enriches createdAt (persisted first-prompt time), adds updatedAt, and derives displayName, whereas /status returns the live createdAt, omits updatedAt, and only returns displayName when set. No longer claims byte-parity.

chiga0
chiga0 previously approved these changes Jun 25, 2026

@chiga0 chiga0 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overview

Final Verdict: Approve — Small, clean additive change. Adds a single by-id session status lookup that reuses the existing summary builder, follows established route patterns, and has good test coverage including a cross-check against the list path.

Findings Summary

  • Critical/Major: 0 items
  • Minor: 0 items
  • Nit: 0 items

Key Observations

The design is sound — extracting toSessionSummary as a shared builder ensures the single-item and list shapes can't drift. The route implementation (requireSessionIdbridge.getSessionSummarysendBridgeError on SessionNotFoundError) mirrors the existing session-scoped GET routes exactly.

The bridge test that cross-checks getSessionSummary(id) against listWorkspaceSessions().find(s => s.sessionId === id) is a nice guard against future divergence between the two paths.

Additional Audit Coverage

  • Synchronous bridge method: getSessionSummary is synchronous (map lookup), unlike async methods (spawnOrAttach, load). This is correct — no I/O involved, just a Map.get() call.
  • Error mapping: SessionNotFoundError is correctly mapped to 404 via sendBridgeError. The 404 response body echoes the session id, which is useful for debugging.
  • displayName optionality: toSessionSummary passes entry.displayName (possibly undefined) directly — matches BridgeSessionSummary's displayName?: string type. Consistent with list path.
  • Route ordering: The new route is placed before GET /session/:id/context, which is fine — Express matches routes in order and /status doesn't conflict with any other session-scoped path segment.
  • No SDK/ACP wiring: PR description correctly scopes this out — the daemon route is the primitive. SDK helper can be added in a follow-up if needed.

This review was generated by QoderWork AI

@chiga0
chiga0 dismissed their stale review June 25, 2026 09:07

cannot pass

The session-status docs said the response is "the same item shape that
GET /workspace/:id/sessions lists". That parity only holds at the bridge
layer; the HTTP list endpoint enriches each item with persisted
session-store data, so for the same live session the two routes diverge
on createdAt (persisted first-prompt time vs live spawn time), updatedAt
(present only on the list), and displayName (derived from the stored
title/prompt vs the live session's own, usually unset). Reword to
describe /status as the raw live-session view, spell out those
differences, and fix the 404 note to match the actual { error, sessionId }
body (no code field).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@samuelhsin

Copy link
Copy Markdown
Collaborator Author

@wenshao Thanks for the thorough live-daemon verification — and you're right about the shape claim. The bridge-layer parity (getSessionSummary and listWorkspaceSessions both go through toSessionSummary) is exact, but the HTTP GET /workspace/:id/sessions route enriches each item with persisted session-store data, so for a session that's both live and persisted the two routes diverge on createdAt (persisted first-prompt time vs live spawn time), updatedAt (list-only), and displayName (derived from stored title/prompt vs the live session's own, usually unset). The two fields the use case depends on — hasActivePrompt and clientCount — are identical, as you confirmed.

I kept the behavior as-is (the route intentionally returns the raw live view) and fixed the docs + PR description instead, per your suggestion:

  • docs/users/qwen-serve.md now describes /status as the live bridge summary, spells out the createdAt / updatedAt / displayName differences from the enriched list endpoint, and no longer implies byte-parity.
  • Also corrected the 404 session_not_found wording → the body is { error, sessionId } with no code field, matching the other session routes.

Pushed in e029794.

中文说明

感谢细致的真实 daemon 验证——结构一致性的说法确实需要修正。bridge 层是完全一致的(getSessionSummarylistWorkspaceSessions 都走 toSessionSummary),但 HTTP 列表接口 GET /workspace/:id/sessions 会用持久化 session store 数据对每项做富化,所以对一个同时存活且已持久化的 session,两个接口在 createdAt(持久化首个 prompt 时间 vs 实时创建时间)、updatedAt(仅列表有)、displayName(由存储标题/prompt 派生 vs 存活 session 自身、通常未设置)上会有差异。用例真正依赖的 hasActivePromptclientCount 两者完全一致(如你所验证)。

行为保持不变(该路由有意返回原始实时视图),按你的建议改的是文档与 PR 描述:

  • docs/users/qwen-serve.md 现在把 /status 描述为实时 bridge summary,明确写出与富化列表接口在 createdAt / updatedAt / displayName 上的差异,不再暗示逐字节一致。
  • 同时修正了 404 session_not_found 的措辞 → 响应体是 { error, sessionId }没有 code 字段,与其他 session 路由一致。

已在 e029794 推送。

}
});

app.get('/session/:id/status', (req, res) => {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] This adds a new public route, but the route side tables are not updated with it. /capabilities.features still has no tag for this endpoint, so clients cannot feature-detect it the way they do for neighboring session routes such as session_context, session_tasks, session_stats, and session_lsp. resolveDaemonTelemetryRoute also has no GET /session/:id/status matcher, so polling traffic and failures for this endpoint will not get normalized telemetry attribution.

Please add an always-on capability tag such as session_status, update the expected feature/protocol docs, and add a telemetry matcher for GET /session/:id/status that carries the session id.

— GPT-5 Codex via Qwen Code /review

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added the always-on session_status capability tag in e4104ef — registry, registered-features test, and the protocol feature list / capability→route map / capability-versioning reference. On the telemetry matcher: I left resolveDaemonTelemetryRoute alone for sibling-consistency — none of the GET status routes (context/stats/tasks/lsp/supported-commands) are in it; adding only /status would make it the lone read-only GET route with telemetry attribution. Happy to add matchers for all of them as a separate change if you'd prefer.

Comment thread docs/users/qwen-serve.md Outdated
session — the same item shape that `GET /workspace/:id/sessions` lists
(`sessionId`, `workspaceCwd`, `createdAt`, `displayName?`, `clientCount`,
`hasActivePrompt`). It answers `200` with the summary when the daemon holds a
live session with that id, and `404 session_not_found` otherwise. Use it to

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] This says unknown sessions return 404 session_not_found, but the shared SessionNotFoundError mapper currently responds with only { error, sessionId }; it does not include a code field. A client following this doc could key off a non-existent error code and mishandle the main negative response for this route.

Please either document the actual response shape or intentionally add and test a code: 'session_not_found' mapping for this route contract.

— GPT-5 Codex via Qwen Code /review

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in e029794 — the docs now describe the actual { error, sessionId } body (no code field), matching the shared SessionNotFoundError mapper used by every other session route.

@chiga0 chiga0 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review Overview (AI Generated)

PR: #5857 feat(serve): query a single session's status by id
Type: New Feature
Change size: +145/-9 across 6 files

Findings Summary

  • Critical/Major: 0 items
  • Minor: 1 item (enrichment suggestion)
  • Nit: 0 items

Key Observations

This is a clean, well-scoped PR. The getSessionSummary accessor reuses the same toSessionSummary builder as listWorkspaceSessions, preventing shape drift. The route follows the existing pattern (requireSessionId → bridge call → sendBridgeError). Tests cover both happy path and 404. Good work.

However, I believe this endpoint has significant potential to provide richer status information that would be far more useful for remote clients (web-shell, IDE extensions, etc.). See the enrichment suggestion below.


Cross-Validation

Finding Source My Assessment
PR template format qwen-code-ci-bot Non-blocking format nit, not a code issue

No other automated findings. My independent review found no bugs, security issues, or correctness problems.


Minor: Consider enriching the status response with turn-phase information

Currently GET /session/:id/status returns the same BridgeSessionSummary shape as the list endpoint — essentially { sessionId, workspaceCwd, createdAt, displayName, clientCount, hasActivePrompt }. While this is correct and DRY, a dedicated /status endpoint has the opportunity to provide much richer per-session state that remote clients (web-shell, IDE plugins) desperately need but cannot cheaply obtain.

The core gap: hasActivePrompt is a boolean, but clients need to know what phase the session is in — is the model thinking? Generating text? Executing a tool? Waiting for permission approval? This information is already available on the daemon side:

  1. EventBus ring buffer — the per-session EventBus already holds the most recent events. Scanning the last few events can derive the current phase:

    • Last event is session_update with agent_thought_chunkthinking
    • Last event is session_update with agent_message_chunkgenerating
    • Last event is session_update with tool_call/tool_call_update with non-terminal status → tool_execution (with tool name available)
    • Last event is permission_requestawaiting_permission
    • No active prompt → idle
  2. SessionEntry fields already available: activePromptOriginatorClientId, pendingPermissionIds.size, modelRoundtripInFlight, events.lastEventId, events.subscriberCount.

Suggested enriched response shape (additive, backward-compatible):

interface SessionStatusResponse extends BridgeSessionSummary {
  // Current turn phase (derived from recent EventBus events)
  phase?: 'idle' | 'thinking' | 'generating' | 'tool_execution' | 'awaiting_permission' | 'streaming';
  // Currently executing tool (when phase === 'tool_execution')
  activeTool?: {
    toolCallId: string;
    toolName: string;
    status: string;
  };
  // Aggregate counts from EventBus ring
  pendingPermissionCount?: number;
  subscriberCount?: number;
  lastEventId?: number;
}

Implementation sketch: Add a peekRecentPhase(eventBus: EventBus) helper that scans the last ~20 events in the ring backwards, finds the most recent session_update event, checks its sessionUpdate sub-type, and returns the phase. This is O(k) where k is the scan depth (small constant), and reads from an already-in-memory ring buffer — negligible cost.

Why this matters: Without phase information, a polling client can only show "busy" vs "idle". With it, the client can show "Thinking…", "Writing response…", "Running Bash: npm test…", "Waiting for permission…" — dramatically better UX for remote/headless clients.

This could be a follow-up PR, but since the /status endpoint is being introduced here, it's worth considering whether to include richer data from the start to avoid versioning the response shape later.


This review was generated by QoderWork AI

@chiga0 chiga0 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes on the current /session/:id/status shape.

The route name says “status”, but the implementation currently returns only the same sparse live-session list summary shape: sessionId, workspaceCwd, createdAt, optional displayName, clientCount, and hasActivePrompt. That is not enough for the main status use case. A caller still cannot tell whether the session is waiting for the model, streaming thought, streaming assistant text, running a tool, waiting on permission, cancelling, or recovering from an error. In practice every UI/operator would still need to subscribe to SSE and reconstruct activity from events, which defeats the purpose of a lightweight status endpoint.

This looks feasible to support from the existing code rather than speculative. The bridge already has the relevant live signals:

  • sendPrompt() owns prompt start/end and currently flips entry.promptActive.
  • BridgeClient.sessionUpdate() already sees agent_thought_chunk, agent_message_chunk, tool_call, tool_call_update, plan, and other update kinds before publishing to the session event bus.
  • requestPermission() publishes permission_request and tracks pendingPermissionIds.
  • turn_complete, turn_error, and prompt_cancelled are already emitted as explicit terminal/cancel frames.
  • SessionEntry already caches model/mode state via currentModelId / currentApprovalMode.

So I think /status should either be renamed to something narrower like /summary / /live-summary, or preferably be expanded now into a real live activity status. A minimal additive shape could be something like:

{
  sessionId: string;
  workspaceCwd: string;
  createdAt: string;
  clientCount: number;
  hasActivePrompt: boolean;
  phase: 'idle' | 'queued' | 'thinking' | 'responding' | 'tool_calling' | 'awaiting_permission' | 'cancelling' | 'errored';
  activePrompt?: {
    promptId?: string;
    originatorClientId?: string;
    startedAt: string;
    lastActivityAt: string;
  };
  activeTools?: Array<{
    toolCallId: string;
    name?: string;
    title?: string;
    status?: string;
    startedAt?: string;
    lastUpdatedAt?: string;
  }>;
  pendingPermissions?: Array<{
    requestId: string;
    toolName?: string;
    issuedAt: string;
  }>;
  lastEventId?: number;
  currentModelId?: string;
  currentApprovalMode?: string;
}

The implementation does not need to expose raw tool args or full event payloads; a redacted summary is enough. The important part is that the daemon owns the runtime phase and active tool/permission summary, so clients do not each have to infer it differently.

Suggested implementation path:

  1. Add a dedicated BridgeSessionStatus type instead of reusing BridgeSessionSummary.
  2. Extend SessionEntry with a small runtimeStatus object updated at the same publish sites that already emit prompt/session/permission/turn events.
  3. Keep hasActivePrompt for backward compatibility, but derive phase and active summaries from the same state.
  4. Add bridge tests that drive sendPrompt, agent_thought_chunk, agent_message_chunk, tool_call / tool_call_update, permission_request, turn_complete, turn_error, and cancel, asserting the phase transitions.
  5. Update the serve route/docs to describe /status as the richer live activity endpoint.

With the current shape, this PR would likely bake in a misleading /status contract and require another immediate API expansion for real clients.

Generated by GPT-5

@wenshao

wenshao commented Jun 25, 2026

Copy link
Copy Markdown
Collaborator

@qwen-code /triage

});

describe('GET /session/:id/status', () => {
it('200 with the live session summary', async () => {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] This 200 test only covers the displayName-present case. The PR documents a specific /status contract — "returns displayName only when one is set on the live session" (docs/users/qwen-serve.md) — but nothing asserts the key is absent from the HTTP body when unset.

That omission relies entirely on res.json() dropping the undefined-valued key that toSessionSummary always sets (bridge.ts: displayName: entry.displayName). A future "consistency" change to that helper (e.g. entry.displayName ?? '') would silently break the documented shape with the whole suite still green. A sibling test pins the contract:

it('200 omits displayName when the live session has none', async () => {
  const summary: BridgeSessionSummary = {
    sessionId: 's-2',
    workspaceCwd: WS_BOUND,
    createdAt: '2026-05-17T12:00:00.000Z',
    clientCount: 0,
    hasActivePrompt: false,
  };
  const bridge = fakeBridge({ summaryImpl: () => summary });
  const app = createServeApp(
    { ...baseOpts, workspace: WS_BOUND },
    undefined,
    { bridge },
  );
  const res = await request(app)
    .get('/session/s-2/status')
    .set('Host', `127.0.0.1:${baseOpts.port}`);
  expect(res.status).toBe(200);
  expect('displayName' in res.body).toBe(false);
});

— claude-opus-4-8[1m] via Qwen Code /qreview

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added your suggested test in 842d722 — a 200 case with a no-displayName summary asserting 'displayName' in res.body === false, so a future change to the shared summary builder can't silently break the documented shape. Thanks!

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, looks ready to ship. ✅

…ability

The new single-session status route had no entry in the capability
registry, so clients couldn't feature-detect it the way they pre-flight
the sibling read-only session routes (session_context, session_tasks,
session_stats, session_lsp, …). Add an always-on `session_status` tag,
mirror it in the registered-features test, and document it in the
protocol feature list, the capability→route map, and the capability
versioning reference.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@samuelhsin

Copy link
Copy Markdown
Collaborator Author

Thanks all for the careful review. Pushed two follow-up commits (e02979491, e4104efe7) and want to respond to the open points.

Addressed

  • Docs — "same shape as the list" / missing updatedAt (@wenshao docs:94): reworded to describe /status as the live bridge summary and spelled out that the list endpoint enriches createdAt (persisted first-prompt time), adds updatedAt, and derives displayName/status returns the live createdAt, omits updatedAt, and only returns displayName when set. No longer implies byte-parity.
  • Docs — 404 session_not_found code (@wenshao docs:97): corrected; the body is { error, sessionId } with no code field, matching the shared SessionNotFoundError mapper used by every other session route.
  • Capability tag (@wenshao server.ts:3327): added an always-on session_status tag to the registry, mirrored it in the registered-features test, and documented it in the protocol feature list, the capability→route map, and the capability-versioning reference. Clients can now pre-flight caps.features.session_status like the sibling routes.

One push-back — telemetry matcher (@wenshao server.ts:3327): I left resolveDaemonTelemetryRoute alone on purpose. None of the sibling read-only GET status routes (/session/:id/context, /stats, /tasks, /lsp, /supported-commands) have a matcher there either — it only classifies mutations, DELETE /session/:id, and the workspace session list. Adding only /status would make it the lone GET status route with telemetry attribution, which is less consistent, not more. Happy to add it if maintainers would rather all the read-only status routes get matchers, but that feels like a separate, broader change.

On enriching /status with phase / active tools / pending permissions (@chiga0): I agree this is genuinely valuable for remote/headless clients, and you're right that the daemon already owns all the signals. But I'd like to keep this PR at the scope of its originating issue (#5855 asked for exactly the minimal by-id summary), in line with the repo's "Simplicity First" guideline. Importantly, the response is JSON and the enrichment is purely additive — adding phase, activeTools, etc. later does not break hasActivePrompt / clientCount consumers, so shipping the minimal shape now doesn't bake in a contract we'd have to version away from. I've opened #5863 to track the richer live-activity status as a focused follow-up, with your proposed shape and implementation path credited. Would you be open to taking the enrichment there so this minimal lookup can land first?

中文说明

感谢各位细致 review。已推送两个后续提交(e02979491e4104efe7),逐条回应。

已处理

  • 文档"与列表结构一致"/ 缺 updatedAt@wenshao docs:94):改为把 /status 描述为实时 bridge summary,并写明列表接口会富化 createdAt(持久化首个 prompt 时间)、加 updatedAt、派生 displayName,而 /status 返回实时 createdAt、不含 updatedAt、仅在设置时返回 displayName,不再暗示逐字节一致。
  • 文档 404 code(@wenshao docs:97):已修正,响应体是 { error, sessionId },没有 code 字段,与其他所有 session 路由共用的 SessionNotFoundError 映射一致。
  • capability tag(@wenshao server.ts:3327):在注册表加了 always-on 的 session_status 标签,同步到 registered-features 测试,并写入协议 feature 列表、capability→route 映射、capability 版本化参考文档。客户端现在可像 sibling 路由一样 pre-flight caps.features.session_status

一处不改 —— telemetry matcher(@wenshao server.ts:3327):我有意没动 resolveDaemonTelemetryRoute。所有 sibling 只读 GET 状态路由(context/stats/tasks/lsp/supported-commands)都不在其中——它只分类变更类、DELETE /session/:id 和工作区 session 列表。只给 /status 加 matcher 会让它成为唯一被 telemetry 归因的 GET 状态路由,反而更不一致。如果维护者希望所有只读状态路由都加 matcher,我很乐意做,但那是另一个更大范围的改动。

关于把 /status 扩充为 phase / active tools / pending permissions(@chiga0:我同意这对远程/无头客户端很有价值,daemon 确实已握有全部信号。但我希望本 PR 保持在其来源 issue 的范围(#5855 就是要最小的按 id summary),符合本仓库"Simplicity First"原则。关键是响应是 JSON、扩充是纯可加的——后续加 phaseactiveTools 等不会破坏 hasActivePrompt / clientCount 消费方,所以先合入最小形态不会锁定一个将来要弃用的契约。我已开 #5863 跟踪更丰富的实时活动状态,并把你提的 shape 与实现路径记入、致谢。是否可以把扩充放到那里,让这个最小查询先落地?

The docs state the route returns displayName only when the live session
has one, but no test asserted the key is absent from the HTTP body in
that case — it relied implicitly on res.json() dropping the
undefined-valued key. Add a sibling 200 test with a summary that has no
displayName and assert the key is not present, so a future change to the
shared summary builder can't silently break the documented shape.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
wenshao
wenshao previously approved these changes Jun 25, 2026
@chiga0

chiga0 commented Jun 25, 2026

Copy link
Copy Markdown
Collaborator

Follow-up on my earlier request-changes review after re-checking compatibility and the latest discussion.

I agree the richer live activity shape can be handled as an additive follow-up, especially now that #5863 tracks it explicitly. Since this endpoint is new and the response is JSON, adding fields later such as phase, activeTools, pendingPermissions, lastEventId, or model/mode state should not be a breaking change as long as the current fields remain stable and keep their meaning. So I no longer think the richer /status shape needs to block this minimal by-id lookup PR.

The remaining blocker I see is CI, not the API shape: Ubuntu currently fails in cli/qwen-serve-routes.test.ts > qwen serve — capabilities envelope > advertises all baseline capabilities because the actual capabilities include the new session_status feature but the expected baseline list does not. Please update that baseline/expectation (or otherwise align the advertised feature list with the test). Once CI is green, I am happy to approve this PR and let the richer status work proceed in #5863.

Generated by GPT-5

@doudouOUC doudouOUC left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found. LGTM! ✅ Downgraded from Approve to Comment: CI failing (Test (ubuntu-latest, Node 22.x)).

— qwen3.7-max via Qwen Code /review

The capabilities-envelope integration test pins the full caps.features
list returned by a live daemon, so adding the session_status capability
tag to the registry made the live list diverge from the test's hardcoded
baseline (CI: expected 65, received 66). Add session_status to that
baseline in the same position the registry emits it (after session_lsp),
and to the session-lifecycle capability-tag reference for completeness.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

@chiga0 chiga0 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-Review at HEAD 21cda4c

5 new commits since last review. All actionable feedback addressed.

Re-Review Status Table

Previous Finding Status Evidence
Docs accuracy (live vs list shape) ✓ Fixed e029794 — reworded to "live bridge summary", spelled out differences from list endpoint (createdAt source, updatedAt omission, displayName conditional)
404 response body docs ✓ Fixed e029794 — docs now describe actual { error, sessionId } body
Capability advertisement missing ✓ Fixed e4104efsession_status added to SERVE_CAPABILITY_REGISTRY, EXPECTED_STAGE1_FEATURES test, protocol docs, and capability→route map. Author correctly left resolveDaemonTelemetryRoute alone for sibling consistency (no GET status routes in it).
displayName omission untested ✓ Fixed 842d722 — new test asserts 'displayName' in res.body === false when summary has no displayName
Enrichment: phase information — Deferred Broader scope (requires EventBus scanning logic), appropriate as follow-up. Current shape is well-documented and the capability tag lets clients gate on availability.

Additional changes

  • 7eff417 — Integration capabilities baseline updated with session_status
  • 21cda4c — Merge from main

No new issues. The PR is clean and well-documented.

Verdict: Approve


This review was generated by QoderWork AI

@chiga0 chiga0 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-Review at HEAD 21cda4c

Re-Review Status Table

Previous Finding Status Evidence
Docs: live vs list shape 描述不准确 ✓ Fixed e029794 — 文档已重写,明确区分 live bridge summary 与 list endpoint 的差异
Docs: 404 响应体描述不准 ✓ Fixed e029794 — 已更新为 { error, sessionId }
缺少 session_status capability 广告 ✓ Fixed e4104ef — 已注册到 SERVE_CAPABILITY_REGISTRY、测试基线、协议文档
缺少 displayName 缺失场景测试 ✓ Fixed 842d722 — 新增 test 断言 'displayName' in res.body === false
Enrichment: phase 信息 (thinking/generating/tool_execution) — Deferred 合理的后续优化,不阻塞本 PR

All actionable feedback has been addressed. The code is clean, tests are comprehensive, and the implementation is well-scoped.


This review was generated by QoderWork AI

@samuelhsin
samuelhsin enabled auto-merge June 25, 2026 16:13
@samuelhsin
samuelhsin added this pull request to the merge queue Jun 25, 2026
Merged via the queue into QwenLM:main with commit 24edd45 Jun 25, 2026
12 checks passed

@doudouOUC doudouOUC left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review at 762c94b6 (merge commit only, no new author commits). All 828 tests pass, build/typecheck/lint clean.

[Suggestion] Batch 404 test missing /session/:id/statuspackages/cli/src/serve/server.test.ts:4671

The batch test exercising /session/missing/{context,supported-commands,stats,tasks,lsp} was not updated to include /session/missing/status. While the PR has its own dedicated 404 test, the batch test serves as a regression safety net for consistent error handling across all read-only session routes.

Suggested fix: Add { method: 'get', path: '/session/missing/status' } to the batch test alongside the existing routes.

}
});

app.get('/session/:id/status', (req, res) => {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] This returns raw bridge data while GET /workspace/:id/sessions enriches with persisted fields (createdAt from item.startTime, updatedAt from item.mtime, displayName from item.customTitle || item.prompt). This divergence is well-documented but no test pins it. A future refactor could silently harmonize or break the divergence with no test catching it.

Suggested fix: Add a server-level test that sets up a session with persisted metadata and asserts the two endpoints return different createdAt / displayName values for the same session.

— qwen3.7-max via Qwen Code /review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat(serve): query a single session's status by id

6 participants