📋 Review Summary

This PR implements a daemon-assisted follow-up suggestion feature that pushes server-generated "ghost-text" suggestions to attached clients after each clean assistant turn. The implementation spans the SDK schema layer, ACP bridge translation, CLI session logic, and webui hook integration. The code demonstrates strong architectural consistency with existing patterns (PR 14b MCP budget events, auth device-flow events) and includes comprehensive test coverage.

🔍 General Feedback

• Excellent commit breakdown: Each of the three commits is independently shippable and follows a clear schema → bridge → activation progression.
• Strong pattern reuse: The implementation mirrors established precedents (MCP budget events, auth device-flow events) for event schema evolution, validation predicates, and reducer handling.
• Comprehensive test coverage: Tests cover happy paths, malformed payloads, feature disabled states, approval mode exclusions, and abort semantics.
• Forward-compat design: Unknown event kinds, malformed payloads, and future error kinds are handled gracefully via the existing unrecognizedKnownEventCount diagnostic path.
• Well-documented design decisions: The PR body explains why new event unions were created, why client-side invalidation was chosen, and filtering semantics.

🎯 Specific Feedback

🟡 High

• File: packages/cli/src/acp-integration/session/Session.ts:#maybeEmitFollowupSuggestion - The fire-and-forget IIFE swallows all errors silently. While the comment explains this is intentional ("A failed suggestion is invisible to the user"), consider adding a rate-limit or circuit-breaker if suggestion generation fails repeatedly. A counter like consecutiveSuggestionFailures that disables the feature after N failures would prevent wasting resources on a broken suggestion pipeline.

• File: packages/acp-bridge/src/bridgeClient.ts:extNotification - The followup_suggestion handler returns early without publishing if !entry. This is correct for the "session already closed" case, but there's no diagnostic signal. Consider incrementing a droppedSuggestionCount on the bridge entry (similar to unrecognizedKnownEventCount) so operators can detect if suggestions are being dropped due to race conditions.

🟢 Medium

• File: packages/sdk-typescript/src/daemon/events.ts:isFollowupSuggestionData - The predicate comment says "Empty suggestion is protocol garbage" but the validation only checks isNonEmptyString. Consider adding a maxLength guard (e.g., 200 characters) to prevent pathological suggestions from breaking UI layouts. The daemon should filter these, but defense-in-depth at the wire boundary is valuable.

• File: packages/webui/src/daemon/useDaemonFollowupSuggestion.ts:useEffect - The effect dependency array includes lastFollowupSuggestion which is a reference from useSyncExternalStore. The comment explains reference stability via cloneTranscriptState, but this relies on the reducer never mutating in-place. Consider adding a useMemo wrapper around the suggestion object comparison to make the intent more explicit:

  const suggestionKey = useMemo(
    () => `${lastFollowupSuggestion?.promptId}:${lastFollowupSuggestion?.suggestion}`,
    [lastFollowupSuggestion?.promptId, lastFollowupSuggestion?.suggestion],
  );

• File: packages/cli/src/acp-integration/session/Session.ts:#maybeEmitFollowupSuggestion - The history slice (fullHistory.slice(-40)) is hardcoded. Consider extracting this to a constant MAX_SUGGESTION_HISTORY_LENGTH at the module level for consistency with other history-slicing code and easier tuning.

🔵 Low

• File: packages/sdk-typescript/src/daemon/events.ts:DAEMON_KNOWN_EVENT_TYPE_VALUES - The comment for followup_suggestion says "Old SDK consumers silently drop this event via asKnownDaemonEvent returning undefined (no protocol bump required)." Consider adding a // Added: <date> <PR#> comment style here (like some other entries have) for future maintainers tracing when this event was introduced.

• File: packages/sdk-typescript/src/daemon/ui/terminal.ts:daemonUiEventToTerminalText - The terminal renderer uses ANSI color '2' (green) for suggestions. This is a minor stylistic choice, but consider whether a different color (e.g., yellow '33' like cancelled prompts) might better convey the "ghost-text hint" semantics vs. a first-class terminal output.

• File: packages/webui/src/daemon/useDaemonFollowupSuggestion.ts:UseDaemonFollowupSuggestionReturn - The interface JSDoc says "Wire to <InputForm onAcceptFollowup={...} />" but doesn't document what the skipOnAccept option does. A one-liner explaining when a consumer would use { skipOnAccept: true } would help future maintainers.

✅ Highlights

• Excellent schema validation: The isFollowupSuggestionData predicate correctly guards against NaN/Infinity via isNonEmptyString and isRecord helpers, showing attention to edge cases.
• Clean abort semantics: The followupAbort controller is properly reset in both prompt() and cancelPendingPrompt(), ensuring no stale suggestions land after the user moves on.
• Reference stability optimization: The cloneTranscriptState sharing of lastFollowupSuggestion reference is a nice touch for useSyncExternalStore subscribers to skip re-renders on unrelated events.
• Comprehensive test scenarios: The test suite covers malformed payloads, early-event buffering absence, feature disabled states, PLAN approval mode exclusion, filter-reason logging, and abort signal propagation.
• Strong documentation: JSDoc comments throughout explain not just what the code does but why design decisions were made (e.g., why client-side invalidation, why no early buffering).

Thanks — declining all eight, brief reasoning per:

High

• Session.ts circuit breaker for suggestion failures: defer. generatePromptSuggestion is single-shot (maxAttempts: 1), no retry storm to break. A counter for a fire-and-forget that already swallows is YAGNI without a repro of repeated failures.
• bridgeClient.ts droppedSuggestionCount: defer. The sibling mcp-budget-event early-drop path has no such counter either; adding it just for this event is inconsistent. If we want bridge-side drop telemetry, do it uniformly across all extNotification branches in a separate change.

Medium

• events.ts maxLength guard: defer. Length filtering lives in generatePromptSuggestion's getFilterReason() (the daemon's source of truth). Stamping an arbitrary SDK-side cap (200?) would diverge from server-side semantics.
• useDaemonFollowupSuggestion.ts useMemo on promptId:suggestion: defer. lastPushedPromptIdRef already gates re-runs on promptId, which is the unique-per-turn key — the suggestion text alone doesn't disambiguate.
• Extract slice(-40) to a constant: defer for symmetry. The CLI's AppContainer.tsx also hardcodes 40 inline; extracting only here introduces asymmetry. Should be one shared constant in core if/when extracted.

Low

• Add // Added: <date> <PR#> comment: defer. Existing entries are inconsistent on this (some carry "PR 14b" / "proposal(serve): Mode B feature-priority roadmap toward v0.16 production-ready #4175 F4 prereq", many don't). Maintainers can backfill post-merge if desired.
• Terminal color choice ('2' dim vs '33' yellow): keeping dim. Yellow is used for warnings/cancellations in this file; assist hints aren't warnings.
• Document skipOnAccept in the wrapper hook: defer. Semantics live on the underlying useFollowupSuggestions controller; duplicating docs at the wrapper invites drift.

⚠️ Cross-PR Conflict Detected — Large Conflict Group (5 PRs)

This PR is part of a conflict group with PR #4476, PR #4520, PR #4545, and PR #4546. These PRs share modifications to core daemon and event handling code.

Key shared functions:

• publish / push — packages/acp-bridge/src/eventBus.ts
• push — packages/channels/base/src/BlockStreamer.ts
• prompt — packages/cli/src/acp-integration/session/Session.ts
• cancelPendingPrompt — packages/cli/src/acp-integration/session/Session.ts
• asKnownDaemonEvent, reduceDaemonSessionEvent — packages/sdk-typescript/src/daemon/events.ts
• shutdown — multiple files (DualOutputBridge.ts, ControlDispatcher.ts, etc.)
• subscribeEvents — packages/sdk-typescript/src/daemon/DaemonClient.ts, DaemonSessionClient.ts

Recommendation: This PR modifies daemon event handling which overlaps with 4 other PRs. Consider merging after the group has been reviewed together.

This conflict was detected automatically by CodeScope skills — structural analysis of call graphs and file modifications.

⚠️ Cross-PR Conflict Detected — Large Conflict Group (5 PRs)

This PR is part of a conflict group with PR #4476, PR #4520, PR #4545, and PR #4546. These PRs share modifications to core daemon and event handling code.

Key shared functions:

• publish / push — packages/acp-bridge/src/eventBus.ts
• push — packages/channels/base/src/BlockStreamer.ts
• prompt — packages/cli/src/acp-integration/session/Session.ts
• cancelPendingPrompt — packages/cli/src/acp-integration/session/Session.ts
• asKnownDaemonEvent, reduceDaemonSessionEvent — packages/sdk-typescript/src/daemon/events.ts
• shutdown — multiple files (DualOutputBridge.ts, ControlDispatcher.ts, etc.)
• subscribeEvents — packages/sdk-typescript/src/daemon/DaemonClient.ts, DaemonSessionClient.ts

Recommendation: This PR modifies daemon event handling which overlaps with 4 other PRs. Consider merging after the group has been reviewed together.

This conflict was detected automatically by CodeScope skills — structural analysis of call graphs and file modifications.

Review fixes pushed (f059338)

Ready for re-review.

本地真实测试报告（追加，针对最新 commit 0166d432c — merge 参考）

接续我前一份 本地真实测试报告（commit a4b95be83）。这一轮覆盖在那之后追加的两个 R5 review fix commit：

• 579347762 — [demux] 日志 key 顺序对齐到 session=/type=/action=/reason=（与 current_model_update 一致），并把 bridge.test.ts 里 "silently" 字样从用例名删掉（drops 现在已经会打日志）
• 0166d432c — 删除 followup_suggestion 上 dead 的 originatorClientId 条件 spread 和对应的 false-confidence 测试（理由：activePromptOriginatorClientId 在 prompt 完成时的 .finally() 里被清空，suggestion 是在 prompt 完成后才 fire 的——这个字段在生产里永远 undefined）

测试环境

1. 静态检查

2. 单测

3. tmux 真实 daemon end-to-end

启动方式（PR body 第 1-3 步）：

node /root/git/qwen-code-x1/.qwen/tmp/pr-4507/packages/cli/dist/index.js \
  serve --port 18507 --workspace /tmp/pr4507-ws

.qwen/settings.json 仅切换 ui.enableFollowupSuggestions 一个开关，其它走我自己已有的 user-level settings。

Positive：enableFollowupSuggestions: true

发两条 prompt，订 GET /session/<id>/events：

=== Prompt 1: 用一句话告诉我什么是CAP定理 ===
{"stopReason":"end_turn"}, 9.3s

=== Prompt 2: 再用一句话告诉我什么是Paxos协议 ===
{"stopReason":"end_turn"}, 7.3s

SSE 总计 504 行、125 个事件，分类：

关键证据：Wire 帧（实测原文）

{
  "id": 125,
  "v": 1,
  "type": "followup_suggestion",
  "data": {
    "sessionId": "62411896-0812-4967-8480-bb7c4c621cb4",
    "suggestion": "what about Raft protocol",
    "promptId": "62411896-0812-4967-8480-bb7c4c621cb4########2"
  },
  "_meta": {
    "serverTimestamp": 1779862761764
  }
}

关键验证：originatorClientId 字段不再出现（0166d432c 验证点）

$ grep '"type":"followup_suggestion"' /tmp/pr4507-sse.log | sed 's/^data: //' | jq 'keys'
[
  "_meta",
  "data",
  "id",
  "type",
  "v"
]

$ grep '"type":"followup_suggestion"' /tmp/pr4507-sse.log | grep -c originatorClientId
0

✅ Frame 的 root 层只有 _meta, data, id, type, v 5 个 key，没有 originatorClientId——这与 0166d432c 删除的 dead 条件 spread 行为完全一致：activePromptOriginatorClientId 在 prompt 的 .finally() 里被清空，suggestion 在 prompt 完成后 fire，spread 永远拿到 undefined → 删除后 wire 上更干净，少传 1 个 undefined 字段。

Per-turn 触发模式：MIN_ASSISTANT_TURNS=2 守卫生效

只有 prompt 2 之后才有 followup_suggestion——这与 packages/core/src/followup/suggestionGenerator.ts:103 的 if (modelTurns < MIN_ASSISTANT_TURNS) return { suggestion: null, filterReason: 'early_conversation' } 一致：prompt 1 后 model turn = 1，被 early_conversation 抑制（走 PromptSuggestionEvent 上 telemetry，不上 wire）；prompt 2 后 model turn = 2，开始放行。

Negative：enableFollowupSuggestions: false

通过 jq 把 .ui.enableFollowupSuggestions 改成 false 后重启 daemon（设置在 boot 时读），发同样两条 prompt：

=== SSE size ===
556 lines
=== followup count (must be 0) ===
0
=== types seen ===
    138 session_update

✅ 0 个 followup_suggestion 帧——Session.ts:609 的 if (this.settings.merged.ui?.enableFollowupSuggestions !== true) return; 守卫工作正常。

4. R5 改动逐项对账

0166d432c：删除 dead originatorClientId spread

579347762：[demux] 日志格式对齐

5. 与 PR body Design notes / Test plan 对账

6. 结论

• ✅ 静态检查（4 个 package tsc + 17 个文件 eslint）全 0
• ✅ 单测 SDK 274 / acp-bridge 196 / CLI Session 81 全绿
• ✅ 真 daemon + SSE 端到端：positive 1 个 followup_suggestion 帧、shape 完全对得上、originatorClientId 字段确实不再出现；negative 0 帧
• ✅ R5 改动两个点都验到：0166d432c 的 wire schema 清洁化在实测帧上看得到；579347762 的 [demux] 日志格式对齐在源码 + 单测都对得上
• ✅ MIN_ASSISTANT_TURNS 门控、enableFollowupSuggestions feature gate、stopReason guard 都在实测里复现

从本地真实测试角度，该 PR 可以合并。 R5 这两个 commit 是纯收敛性 fix——一个删 dead code 让 wire 更干净，一个对齐日志格式让 grep 更省心，都不改外部契约。Wire schema 在两轮 review 中保持 stable（v:1 没动、字段集只减不增、originatorClientId 的移除让 frame shape 反而更窄）。下游 webui 的 useDaemonFollowupSuggestion 拿到的载荷与单测 fixture 完全一致，可以安心合上。

chiga0 review response (255da80)

Ready for re-review.

chiga0 P2 fixes pushed (f64b82a)

Ready for re-review.

R7 review response (bb6e234)

Ready for re-review.

Verification Report — PR #4507

Reviewer: wenshao
Branch: feat/daemon-followup-suggestion → daemon_mode_b_main
Test environment: macOS Darwin 25.4.0, Node v22.17.0

Build Verification

Note: Full monorepo build has pre-existing TS errors in cli/src/acp-integration/acpAgent.ts (unrelated imports from ../serve/status.js). Not introduced by this PR — confirmed via git diff origin/daemon_mode_b_main...HEAD --name-only showing acpAgent.ts is not in the diff.

Test Results

Test Bug Analysis

1. daemonUi.test.ts — "clears lastFollowupSuggestion when a new user prompt starts"

The test dispatches a raw { type: 'user_message_chunk', data: { text: 'next question' } } event at line 5351, but the normalizer expects { type: 'session_update', data: { update: { sessionUpdate: 'user_message_chunk', content: { type: 'text', text: 'next question' } } } }. The incorrectly formatted event gets dropped by the normalizer, so the reducer never receives a user.text.delta and never clears lastFollowupSuggestion.

The implementation is correct (line 169-171 of transcript.ts clears on the first user delta). Only the test's event shape is wrong.

2. Session.test.ts — All 4 "follow-up suggestion" tests

All failures share the same root cause: mockChat.getHistory returns [] (empty array per the default beforeEach setup). In #maybeEmitFollowupSuggestion, line 624 checks if (!lastEntry || lastEntry.role !== 'model') return; — with an empty history, lastEntry is undefined and the method bails before calling generatePromptSuggestion.

Fix: add mockChat.getHistory.mockReturnValue([{ role: 'model', parts: [{ text: 'response' }] }]) in the followup test suite's beforeEach.

Implementation Review

Architecture: Clean 3-layer approach:

1. Session.ts — fire-and-forget IIFE after end_turn, proper abort semantics
2. bridgeClient.ts — extNotification demux with validation (string type, non-empty, ≤500 chars, promptId required) + malformed logging
3. SDK — new DaemonAssistEvent union, sidechannel reducer (no transcript block), clearFollowupSuggestion() store action

Wire contract: Additive only. Old SDKs ignore unknown events via normalizer fallback. Old ACP children never emit the new extNotification method. No protocol version bump required.

Key design decisions verified:

• followup_suggestion is correctly a sidechannel (stored in lastFollowupSuggestion, not as a transcript block)
• Self-invalidation semantics correct: reducer clears on first user.text.delta, store exposes clearFollowupSuggestion() for adapters
• Abort chain: followupAbort cancelled on both prompt() entry and cancelPendingPrompt()
• Suggestion length cap (500 chars) in bridge prevents oversized payloads

Lint: All modified source files pass ESLint cleanly.

Verdict

✅ Implementation is correct and merge-ready pending the 5 test fixture bugs (all are test-side issues, not implementation regressions). The implementation logic, type safety, wire contract, and abort semantics are all sound.

Recommend: fix the test event shapes in a follow-up commit, then merge.

R8 review response (2a9e982)

Ready for re-review.

Thread resolution + R9 nit fixes (c276fd9)

Resolved 27/34 review threads. 7 remain open — all are deferred/pushed-back items (not actionable in this PR):

New fixes in this commit:

• Session.ts:625: added debugLogger.debug for role guard skip (observability)
• bridgeClient.ts: extracted MAX_SUGGESTION_LENGTH constant (no magic numbers)

Maintainer Verification Report

Reviewer: wenshao
Branch: feat/daemon-followup-suggestion @ c276fd946
Base: daemon_mode_b_main
Date: 2026-05-27

Build

Test Results

Code Review Summary

Architecture: Clean 3-layer design (SDK schema → bridge handler → session generator + webui hook). Each layer is independently testable and the wire contract is additive (old SDKs ignore unknown events via asKnownDaemonEvent → undefined).

SDK layer (events.ts, ui/*):

• New DaemonAssistEvent union correctly separated from DaemonControlEvent (assist/UX hints vs mutations)
• isFollowupSuggestionData validates all three required fields as non-empty strings
• clearFollowupSuggestion() store action follows clearAwaitingResync pattern
• cloneTranscriptState shares reference for re-render perf — correct since reducer assigns, never mutates
• Exhaustive switch in terminal renderer updated

Bridge layer (bridgeClient.ts):

• extNotification handler follows existing mcp-budget-event pattern
• MAX_SUGGESTION_LENGTH = 500 cap — defense-in-depth at SSE trust boundary
• Malformed payloads logged via writeStderrLine and dropped (consistent with other handlers)
• No originatorClientId spread (correctly removed — field is cleared in .finally() before suggestion fires)

Session layer (Session.ts):

• Fire-and-forget IIFE with proper error swallowing (won't break primary response path)
• Guards: stopReason === 'end_turn', enableFollowupSuggestions === true, not PLAN mode, last history entry role === 'model'
• followupAbort correctly aborted in both prompt() (line 528) and cancelPendingPrompt() (line 482, before the hadPrompt/hadCron guard — fix from R1 review)
• getHistory + structuredClone moved inside IIFE try-catch (fix from P2)
• Debug/warn log split for abort vs real errors

Webui hook (useDaemonFollowupSuggestion.ts):

• Clean useSyncExternalStore subscription to store sidechannel
• lastPushedPromptIdRef prevents re-push after dismiss — correct dedup mechanism
• All three callbacks (accept/dismiss/clear) properly clear both controller state and store
• Well-typed return interface, correctly re-exported through daemon and package entry points

Export Chain Verification

events.ts → daemon/index.ts → sdk-typescript/src/index.ts  (types)
useDaemonFollowupSuggestion.ts → webui/src/daemon/index.ts → webui/src/index.ts  (hook + return type)

All public surface re-exports verified present.

Verdict

LGTM for merge, pending CI validation of Session.test.ts (6 new test cases for the generator — cannot validate locally due to pre-existing dev env issue on the base branch).

The changes are well-structured, additive-only, follow existing patterns, and introduce no regressions in the 977 tests that run locally.