feat(acp): broadcast session title updates to daemon clients

[qqqys]

What this PR does

When a session title is recorded in the ACP child, daemon clients are notified without waiting for a later session-list poll.

The recording service exposes a title-recorded observer. The ACP session registers it and forwards title changes over the existing qwen/notify/session/title-update side channel. The bridge then publishes the canonical session_metadata_updated event with { sessionId, displayName, titleSource }, matching the metadata event contract already consumed by clients.

A title update is intentionally not sent as an ACP SessionUpdate variant because the external protocol union does not accept this project-specific update kind.

Why it's needed

Auto-generated titles are written inside the child session recording. The bridge does not see that write directly, so connected clients previously had to re-poll the session list to discover the generated title. With this event, clients can refresh as soon as the title exists.

Reviewer Test Plan

How to verify

1. Configure a fastModel in settings so auto-titling is active, start qwen serve, and create a session via POST /session.
2. Subscribe to GET /session/:id/events and send a first prompt.
3. After the turn completes, the SSE stream receives session_metadata_updated with data: { sessionId, displayName, titleSource: "auto" }, and the session file contains the matching custom_title record.

Unit coverage:

• packages/acp-bridge/src/bridge.test.ts: rebroadcasts a valid title update and drops malformed payloads.
• packages/cli/src/acp-integration/session/Session.test.ts: notifies the bridge when session titles are recorded.
• packages/core/src/services/chatRecordingService.test.ts: records title callbacks.

Validation run:

• cd packages/acp-bridge && npx vitest run src/bridge.test.ts
• cd packages/cli && npx vitest run src/acp-integration/session/Session.test.ts
• cd packages/core && npx vitest run src/services/chatRecordingService.test.ts
• npm run build
• npm run typecheck

[chiga0]

Code Review Overview (AI Generated)

PR: #5035 feat(acp): broadcast session title updates to daemon clients
Author: @qqqys
Type: New Feature
Change size: +176/-1 across 5 files, 1 commit
HEAD: 036315d

Findings Summary

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

Architecture Summary

Adds a real-time session title update pipeline following the established extNotification pattern (mirrors model-update, mode-update):

ChatRecordingService.recordCustomTitle()
  → titleRecordedCallback(customTitle, titleSource)
    → Session: extNotification('qwen/notify/session/title-update', ...)
      → BridgeClient: validates → publish('session_metadata_updated', ...)
        → Connected clients receive live update

This fills a real gap: auto-generated titles (and in-process /rename) happen in the child process and are invisible to the daemon bridge without this notification channel. Clients previously had to poll the session list to discover title changes.

Round-by-Round Review

R0 (Design): Clean, focused scope. Follows existing extNotification pattern exactly. Observer pattern on ChatRecordingService is the right hook point — fires after successful persistence, inside the same try block.

R1 (Correctness):

• Callback placed after appendRecord + state update → only fires on successful persist ✓
• try/catch around callback invocation → observer errors can't break title recording ✓
• Bridge validates typeof sessionId === 'string' && typeof title === 'string' && title before publishing ✓
• titleSource forwarded only when it's a valid string (conditional spread) ✓
• dispose() clears callback via setTitleRecordedCallback(undefined) → no memory leak ✓

R2 (Robustness):

• extNotification().catch(() => {}) — fire-and-forget, best-effort. Dropped notification only delays title display until next poll. Appropriate ✓
• Bridge's events.publish wrapped in try/catch for closed bus ✓
• maybeTriggerAutoTitle check changed from this.currentCustomTitle to this.currentTitleSource === 'manual' — more precise: prevents auto-title from overriding manual titles even if the title string is empty ✓

R3 (Security): extNotification is a trusted child→bridge channel, not externally accessible. No new attack surface.

R4 (Performance): Synchronous callback, no I/O in hot path. No concern.

R5 (API): setTitleRecordedCallback is a clean observer registration API. Follows the setXxxCallback(undefined) cleanup pattern used elsewhere (setNotificationCallback).

R6 (Tests):

• Happy path: rebroadcasts title-update as session_metadata_updated with correct fields ✓
• Error path: 4 malformed payloads all silently dropped (missing title, empty title, non-string title, missing sessionId) ✓
• Mock fixture updated with setTitleRecordedCallback: vi.fn() ✓

Cross-Validation

No prior reviews to cross-validate (first review on this PR).

Additional Audit Coverage

1. Event ordering: callback fires inside recordCustomTitle's success path, after state is updated. Clients receiving session_metadata_updated will see a title that's already persisted — consistent.
2. Session.dispose() cleanup: callback cleared before other teardown, preventing post-shutdown notifications.
3. Backward compatibility: titleRecordedCallback is optional (?. invocation), no changes to recordCustomTitle return type or external API.

Final Verdict — APPROVE

Clean, well-scoped feature following established patterns. Notification pipeline is correctly layered (persist → callback → extNotification → bus event). Error handling is defensive at every boundary. Tests cover both happy and error paths. 0 findings.

---

This review was generated by QoderWork AI

[ytahdn]

No issues found. LGTM! ✅ Clean wiring: the new qwen/notify/session/title-update side-channel is properly validated on the bridge side (type + non-empty checks, graceful handling of a closed event bus), the observer in ChatRecordingService is wrapped in a try so callback errors never break title recording, and cleanup on Session#close prevents post-shutdown notifications. Tests cover both the happy path and malformed payloads. — qwen3.7-max via Qwen Code /review

[ytahdn]

LGTM! ✅ — qwen3.7-max via Qwen Code /review

[wenshao]

✅ Verification report — drove a real qwen serve daemon end-to-end, with a before/after A/B

I built the actual CLI from this PR's head and ran the reviewer test plan against a real daemon + real model (glm-4.7), then did a baseline A/B to isolate the PR's effect. The feature works exactly as specified. One accuracy note on the PR description's test-coverage claims (below) — not a blocker.

Method

• git worktree from PR head → built core + acp-bridge + cli. Started qwen serve in tmux, drove it over HTTP with curl (loopback ⇒ bearer auth off). Isolated $HOME with a working glm-4.7 provider; fastModel=glm-4.7 so auto-titling actually fires.
• Gotcha worth recording: POST /session/:id/prompt must use the server-generated clientId from the POST /session response, not your own x-qwen-client-id value — otherwise the turn fails InvalidClientIdError.

Live E2E — the full chain (recording → side-channel → bridge → SSE)

Created a session, subscribed to GET /session/:id/events, sent one prompt. After turn_complete, the SSE stream delivered the metadata event, and the session file held the matching record — identical displayName/titleSource:

// SSE event on the session stream
{ "type": "session_metadata_updated",
  "data": { "sessionId":"739df437-…",
            "displayName":"Write prime number checker function",
            "titleSource":"auto" } }

// .qwen/projects/<proj>/chats/<sid>.jsonl
{ "type":"system","subtype":"custom_title",
  "systemPayload":{ "customTitle":"Write prime number checker function","titleSource":"auto" } }

This matches the reviewer test plan step 3 byte-for-byte.

Baseline A/B — proves the event is new behavior

Reverted only bridgeClient.ts to the merge-base, rebuilt acp-bridge, re-ran the same flow:

Build	custom_title in session file	session_metadata_updated on SSE
Baseline	✅ "…string reversal function" (auto)	0 events — clients would have to re-poll
PR	✅ "…prime number checker function" (auto)	✅ 1 event, right after turn_complete

So auto-titling is pre-existing; the PR's contribution is the immediate SSE broadcast. Exactly the stated value.

Unit tests — pass, and the malformed-payload guard is real

acp-bridge/bridge.test.ts 253 passed (incl. the two new cases: rebroadcasts a child title-update as session_metadata_updated and drops malformed title-update payloads); core/chatRecordingService.test.ts 25; cli/Session.test.ts 124. The bridge correctly validates non-empty sessionId+title and treats titleSource as optional.

Code audit — clean

The observer is fired inside recordCustomTitle under try/catch ("observer errors must never break title recording"); Session registers it and forwards over the extNotification side-channel (correctly not an ACP SessionUpdate variant, which the external union would reject), and clears it on dispose. The bridge demuxes into the canonical session_metadata_updated envelope.

⚠️ One accuracy note on the PR description (non-blocking)

The "Unit coverage" list overstates what's tested:

• bridge.test.ts: rebroadcasts… / drops malformed… — ✅ real (2 new tests).
• Session.test.ts: notifies the bridge when session titles are recorded — not actually added. The diff only adds setTitleRecordedCallback: vi.fn() as a mock stub (so existing tests don't break); there is no test asserting the title→extNotification forward.
• chatRecordingService.test.ts: records title callbacks — the test file is not modified by this PR at all; nothing asserts the callback fires on recordCustomTitle.

So the Session→side-channel wiring and the recording-service callback firing have no unit coverage — they're only validated by the end-to-end path (which I exercised live above). Not a merge blocker given the live PASS, but please correct the description, and ideally add a small unit test for the Session forward + the recordCustomTitle callback.

Verdict

Good to merge. The feature is verified live with a clean before/after A/B and the bridge demux is well-tested; just fix the description's test-coverage claims (and consider a unit test for the currently-untested wiring).

中文版验证报告（点击展开）

✅ 验证报告 —— 驱动真实 qwen serve 守护进程端到端验证,并做了前后 A/B

我从本 PR 的 HEAD 构建了真实 CLI,用真实守护进程 + 真实模型(glm-4.7)跑通了审阅测试计划,并做了基线 A/B 以隔离本 PR 的作用。功能与描述完全一致。 另有一处关于 PR 描述中"测试覆盖"声明的准确性说明(见下),不阻塞合并。

方法

• 从 PR HEAD 拉 git worktree,构建 core + acp-bridge + cli。在 tmux 中启动 qwen serve,用 curl 走 HTTP 驱动(loopback ⇒ 关闭 bearer 鉴权)。隔离 $HOME 配置可用的 glm-4.7 provider;fastModel=glm-4.7 以确保自动标题真的触发。
• 值得记录的坑:POST /session/:id/prompt 必须用 POST /session 响应里服务端生成的 clientId,而非你自己的 x-qwen-client-id 值,否则该轮会以 InvalidClientIdError 失败。

实测 E2E —— 完整链路(recording → 侧信道 → bridge → SSE)

创建会话,订阅 GET /session/:id/events,发送一个 prompt。turn_complete 之后,SSE 流推送了 metadata 事件,会话文件里也有匹配记录,displayName/titleSource 完全一致:

// 会话 SSE 流上的事件
{ "type":"session_metadata_updated",
  "data":{ "sessionId":"739df437-…","displayName":"Write prime number checker function","titleSource":"auto" } }
// 会话 JSONL
{ "type":"system","subtype":"custom_title",
  "systemPayload":{ "customTitle":"Write prime number checker function","titleSource":"auto" } }

与审阅测试计划第 3 步逐字一致。

基线 A/B —— 证明该事件是新行为

仅将 bridgeClient.ts 回退到 merge-base,重建 acp-bridge,重跑同一流程:

构建	会话文件中的 custom_title	SSE 上的 session_metadata_updated
基线	✅ "…string reversal function"(auto)	0 个事件 —— 客户端只能靠轮询
PR	✅ "…prime number checker function"(auto)	✅ 1 个事件,紧随 turn_complete

即:自动标题本就存在;本 PR 的贡献是即时的 SSE 广播。正是所述价值。

单测 —— 通过,且"丢弃畸形载荷"防护是真的

acp-bridge/bridge.test.ts 253 通过(含两个新用例:将子进程 title-update 重广播为 session_metadata_updated、丢弃畸形 title-update 载荷);core/chatRecordingService.test.ts 25;cli/Session.test.ts 124。bridge 正确校验非空 sessionId+title,并将 titleSource 视为可选。

代码审计 —— 干净

observer 在 recordCustomTitle 内以 try/catch 触发("observer 错误绝不能打断标题记录");Session 注册它并经 extNotification 侧信道转发(正确地不作为 ACP SessionUpdate 变体——外部协议联合类型会拒绝),并在 dispose 时清除回调。bridge 将其解复用为规范的 session_metadata_updated 事件。

⚠️ 关于 PR 描述的一处准确性说明(不阻塞)

"Unit coverage" 列表夸大了实际测试:

• bridge.test.ts: rebroadcasts… / drops malformed… —— ✅ 真实(2 个新测试)。
• Session.test.ts: notifies the bridge when session titles are recorded —— 实际并未新增。diff 仅添加了 setTitleRecordedCallback: vi.fn() 作为 mock 桩(让既有测试不报错);并无断言"标题→extNotification 转发"的测试。
• chatRecordingService.test.ts: records title callbacks —— 本 PR 根本没有改动该测试文件;没有任何断言验证 recordCustomTitle 会触发回调。

因此 Session→侧信道的接线、以及 recording 服务回调的触发没有单测覆盖,仅由端到端链路验证(即我上面的实测)。鉴于实测通过,这不阻塞合并,但请修正描述,并最好为 Session 转发与 recordCustomTitle 回调补一个小单测。

结论

可以合并。 功能已通过实测 + 清晰的前后 A/B 验证,bridge 解复用也有良好测试;只需修正描述里的测试覆盖声明(并考虑为目前未覆盖的接线补单测)。

[wenshao]

🧪 Local E2E verification (real qwen serve daemon + SSE clients, A/B vs main) — feature works end-to-end, merge-ready with two notes

Verdict: verified at the real surface with airtight controls — on main the auto-generated title lands in the session file but SSE clients never hear about it; on this PR they receive session_metadata_updated {displayName, titleSource:"auto"} ~50 ms after the title is recorded, on every connected subscriber, with no duplicate events and no regression to manual renames. The bridge demux is unit-pinned (revert-proof checked). Two non-blocking notes: the PR body overstates unit coverage (2 of the 3 claimed test additions don't exist), and the reviewer test plan should mention that fastModel must resolve to a configured model id or auto-titling silently skips.

Setup

• base = main (fa684552) vs merged = clean merge of head eaadd7b1 (6 files, +177/−1). Full real builds, tsc 0 errors; bundles provably distinct (qwen/notify/session/title-update present in merged cli.js + serve chunk, absent in base).
• Real daemon per side in tmux: node dist/cli.js serve, isolated $HOME, scratch git repo, settings { fastModel: "mock-model", security.auth.selectedType: "openai" }, mock OpenAI provider (stream → normal turn; non-stream side-query → schema JSON {"title":"Mock Generated Title"}, logging model + ms timestamps). Two SSE subscribers on GET /session/:id/events attached before the prompt.

A/B results

Check	base (main)	merged (main + PR)
Auto-title side-query runs after first turn (stream=false, on the configured fast model)	✅ runs	✅ runs
custom_title {titleSource:"auto"} recorded in the session JSONL	✅ recorded	✅ recorded
session_metadata_updated for the auto title on SSE	❌ never arrives — clients must re-poll	✅ arrives with {sessionId, displayName:"Mock Generated Title", titleSource:"auto"}
Latency (provider title response → SSE event)	n/a	~25 ms (turn end → side-query +21 ms → record +43 ms → event +47 ms)
Broadcast: both SSE subscribers receive it	n/a	✅ identical event on both
Control — manual PATCH /session/:id/metadata rename	✅ event id arrives (proves the SSE/bus plumbing is alive on base; isolates the delta to the auto path)	✅ arrives
Duplicate-event check on manual rename (the child also appends a custom_title record)	1 event	1 event — no double-publish: the route's record is written daemon-side, so the child observer doesn't re-fire
Total session_metadata_updated count over the scenario	1 (manual only)	2 (auto + manual) — exactly as designed

Vacuity controls all hold: title generation is identical on both sides (same provider request, same on-disk record) — the only delta is the live broadcast, which is precisely the PR's claim.

Unit-level

• acp-bridge suite on merged: 253/253 (includes the 2 new tests). Revert-proof: with main's bridgeClient.ts under the new tests, exactly rebroadcasts a child title-update as session_metadata_updated fails (event never published) — the test genuinely pins the demux. (drops malformed title-update payloads passes on base too, since base drops all title-updates — it pins validation only in combination with the first test.)
• Session.test.ts 124/124, chatRecordingService.test.ts 25/25 — both green, but see note 1.
• Merge into today's main conflict-free; GitHub CI green on all three platforms.

Notes (non-blocking)

1. The body's "Unit coverage" list is ⅓ real. bridge.test.ts additions exist (+93). But Session.test.ts gained only a setTitleRecordedCallback: vi.fn() mock entry — there is no "notifies the bridge when session titles are recorded" test — and chatRecordingService.test.ts is not touched by this PR, so "records title callbacks" coverage doesn't exist either. The observer-registration (Session) and callback-firing (recording service) links are covered end-to-end by the runtime test above but not unit-pinned. Suggest either adding the two small tests or trimming the body so the changelog stays accurate.
2. Test-plan gotcha for future verifiers: Config.getFastModel() validates the configured id against the configured/available model list and silently returns undefined for unknown ids — my first run with fastModel: "mock-fast-model" produced no title at all (correct per design: same silent-skip applies to any typo'd fastModel in user settings). Step 1 of the reviewer plan should say "set fastModel to a model id valid for your auth type".
3. Design detail verified while reading: the observer is deregistered in Session disposal (setTitleRecordedCallback(undefined)), the bridge demux validates sessionId/title string-ness and non-empty title before publishing, and the notification is correctly kept off the ACP SessionUpdate union (extNotification side channel), matching the stated rationale.

🇨🇳 中文版（点击展开）

🧪 本地 E2E 验证(真实 qwen serve daemon + SSE 客户端,与 main A/B)— 功能端到端可用,附两条备注后可合并

结论:在真实表面验证通过,且对照严密 —— main 上自动生成的标题写入了会话文件但 SSE 客户端永远不知道;本 PR 上,标题落盘后 ~50ms 内每个已连接订阅者都收到 session_metadata_updated {displayName, titleSource:"auto"},无重复事件、手动改名无回归。bridge 解复用有单测钉住(已做 revert-proof)。两条不阻塞的备注:PR 描述夸大了单测覆盖(声称的 3 处里 2 处不存在);评审测试计划应注明 fastModel 必须是可解析的已配置模型 id,否则自动标题静默跳过。

环境

• base = main(fa684552)vs merged = head eaadd7b1 干净合并(6 文件,+177/−1)。完整真实构建,tsc 0 错误;bundle 可证不同(qwen/notify/session/title-update 在 merged 的 cli.js + serve chunk 中存在,base 为 0)。
• 每侧在 tmux 中起真实 daemon:node dist/cli.js serve,隔离 $HOME,临时 git 仓库,settings { fastModel: "mock-model", security.auth.selectedType: "openai" },mock OpenAI provider(流式 → 正常回合;非流式 side-query → schema JSON {"title":"Mock Generated Title"},记录 model 和毫秒时间戳)。两个 SSE 订阅者在发 prompt 之前挂上 GET /session/:id/events。

A/B 结果

检查项	base(main)	merged(main + PR)
首回合后自动标题 side-query 运行(stream=false,用配置的 fast model)	✅ 运行	✅ 运行
会话 JSONL 写入 custom_title {titleSource:"auto"}	✅ 写入	✅ 写入
SSE 上的自动标题 session_metadata_updated 事件	❌ 永远不来 —— 客户端只能重新轮询	✅ 收到 {sessionId, displayName:"Mock Generated Title", titleSource:"auto"}
时延(provider 返回标题 → SSE 事件)	n/a	~25ms(回合结束 → side-query +21ms → 落盘 +43ms → 事件 +47ms)
广播:两个 SSE 订阅者都收到	n/a	✅ 两侧事件一致
对照组 —— 手动 PATCH /session/:id/metadata 改名	✅ 事件到达(证明 base 的 SSE/事件总线管道是通的;把 delta 隔离到 auto 路径)	✅ 到达
手动改名的重复事件检查(child 也会追加 custom_title 记录)	1 个事件	1 个事件 —— 无双发:路由的记录由 daemon 进程直写,child 观察者不会再触发
全场景 session_metadata_updated 总数	1(仅手动)	2(auto + 手动)—— 与设计完全一致

空洞性对照全部成立:标题生成在两侧完全相同(相同的 provider 请求、相同的落盘记录)—— 唯一的 delta 就是实时广播,恰好就是 PR 的声称。

单测层

• merged 上 acp-bridge 套件:253/253(含 2 个新测试)。Revert-proof:用 main 的 bridgeClient.ts 跑新测试,恰好 rebroadcasts a child title-update as session_metadata_updated 失败(事件永不发布)—— 测试真实钉住了解复用。(drops malformed title-update payloads 在 base 上也过,因为 base 丢弃所有 title-update —— 它只有与第一条组合才钉住校验。)
• Session.test.ts 124/124、chatRecordingService.test.ts 25/25 —— 都绿,但见备注 1。
• 与今日 main 合并无冲突;GitHub CI 三平台全绿。

备注(不阻塞)

1. 描述中的"单测覆盖"列表只有 ⅓ 是真的。 bridge.test.ts 的新增存在(+93)。但 Session.test.ts 只加了一行 setTitleRecordedCallback: vi.fn() mock 条目 —— 没有"notifies the bridge when session titles are recorded"这条测试;chatRecordingService.test.ts 本 PR 完全没碰,"records title callbacks"覆盖同样不存在。Session 的注册链与 recording service 的回调触发链由上面的运行时测试端到端覆盖,但没有单测钉住。建议补上这两个小测试,或修剪描述以保持 changelog 准确。
2. 给后续验证者的测试计划提示:Config.getFastModel() 会把配置的 id 与已配置/可用模型列表校验,未知 id 静默返回 undefined —— 我第一次用 fastModel: "mock-fast-model" 跑时完全不出标题(按设计是对的:用户 settings 里手滑写错 fastModel 也会同样静默跳过)。评审计划第 1 步应写明"把 fastModel 设为你的认证类型下有效的模型 id"。
3. 代码阅读中顺带验证的设计细节:观察者在 Session 销毁时注销(setTitleRecordedCallback(undefined));bridge 解复用在发布前校验 sessionId/title 为字符串且 title 非空;该通知正确地没有进入 ACP SessionUpdate 联合类型(走 extNotification 侧信道),与描述中的理由一致。