feat(tui): add daemon adapter spike#4202
Conversation
📋 Review SummaryThis PR introduces a draft design document for a flag-gated TUI daemon adapter that enables the TUI to communicate with 🔍 General Feedback
🎯 Specific Feedback🔵 LowFile: File: File:
File: This would make the failure mode explicit rather than relying only on File: ✅ Highlights
|
|
|
||
| export function reduceDaemonEventToTuiUpdates( | ||
| event: DaemonTuiEvent, | ||
| ): DaemonTuiUpdate[] { |
There was a problem hiding this comment.
[Critical] The daemon carries critical data in the _meta field: token usage (usage), turn duration (durationMs), and stop-hook-loop signals. The adapter never reads _meta. This means:
- Stop-hook-loop warnings are invisible — the safety mechanism that prevents infinite agent loops is silently dropped.
- Token usage stats are lost — the daemon TUI would never show usage data that the in-process TUI displays after every turn.
- The
agent_message_chunkbranch requires&& text(falsy empty strings are dropped), so_meta-only events like stop-hook-loop are silently discarded.
Consider extracting _meta and surfacing it, or at minimum passing it through on the DaemonTuiUpdate type for consumers to handle.
— glm-5.1 via Qwen Code /review
718bd20 to
ca12542
Compare
|
处理了这轮 review：
边界/后续项：
验证：
|
4f49825 to
1fe2b04
Compare
1fe2b04 to
da6a35c
Compare
0481f0e to
56ece34
Compare
|
Follow-up update after rebasing this PR to main:
Local validation passed:
I resolved the addressed threads. One real blocker remains open intentionally: daemon |
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
wenshao
left a comment
There was a problem hiding this comment.
Code Review: feat(tui): add daemon adapter spike
Verdict: REQUEST_CHANGES (7 Critical findings, default-off spike)
阻塞问题集中在 adapter 层的状态一致性和安全输入清理上。这些问题不影响 MCP 协议本身，但会导致 TUI 层出现不可恢复的状态偏差。
Critical Issues
| # | 行号 | 问题 | 影响 |
|---|---|---|---|
| 1 | 82 | turn_complete 是 dead code type variant |
编译器无法对未处理事件报错 |
| 2 | 164 | sanitizeReason 仅处理 ANSI ESC，漏掉 OSC/DCS/控制字符 |
注入风险（标题/链接操控） |
| 3 | 284 | toolCallsById 无限增长，无淘汰 |
长会话内存泄漏 |
| 4 | 313 | agent_message_chunk/thought_chunk 未经过滤直接进 React 标签 |
任意字符串注入 |
| 5 | 484 | sendPrompt 出错后无状态清理 |
busy=true 卡死，后续 prompt 永久阻塞 |
| 6 | 495 | cancel/setModel/approve/reject 无 try/catch |
未捕获异常 + 无事件上报 |
| 7 | 554 | pumpEvents catch 块中 emit() 可能抛出异常 |
捕获异常再抛 → 事件循环终止 |
Positive Notes
EventQueue设计正确：mutex + linked list，容量上限防止 OOMpumpEvents断连处理和 abort/timeout 机制可靠- 测试覆盖合理：4 个 test case 覆盖核心路径
- MCP
turn_complete移除方向正确（避免双重信号）
| daemonEventId?: number; | ||
| } | ||
| | { | ||
| type: 'turn_complete'; |
There was a problem hiding this comment.
[Critical] turn_complete 是 dead code type variant。DaemonEventType 定义了它，但事件分发循环（line 379）从未处理此类型——所有 turn_complete 事件都会落入 default 分支被静默丢弃。如果未来有人在 daemon 端发送 turn_complete，adapter 不会通知 TUI 层，导致 UI 无法知道 turn 已结束。
建议：移除类型定义或添加对应的处理分支（后者需要决定与 MCP response_finished 的优先级关系）。
| } | ||
| } | ||
|
|
||
| function sanitizeReason(reason: string): string { |
There was a problem hiding this comment.
[Critical] sanitizeReason 只剥离 ANSI escape sequences（ESC[...]m），但不处理 OSC（ESC]...BEL/ESC\\）和 DCS（ESC P...ST）序列。终端模拟器（iTerm2、xterm.js）会解释这些序列，可用于设置窗口标题、创建超链接、甚至执行 iTerm2 特定的 shell integration 命令。
此外，控制字符（\x00-\x1F 除了 \n/\r/\t）也未被过滤。
建议：改用更全面的 ANSI 剥离方案，或限制为 [\x20-\x7E\n\r\t] 白名单。
| state?.toolCallsById.set(toolCallId, tool); | ||
| return { | ||
| type: 'tool_group', | ||
| tools: Array.from(state?.toolCallsById.values() ?? [tool]), |
There was a problem hiding this comment.
[Critical] toolCallsById Map 没有容量上限或淘汰策略。每次 tool_call_started 都会插入新条目，只有 sendPrompt 开始时清空一次。如果 daemon 在一次 turn 内发送大量 tool_call_update（例如一个长 chain-of-thought 工具调用序列），或如果 tool_call_finished 事件丢失（网络分区、daemon bug），Map 会无限增长。
建议：添加容量上限（例如 256），超出时丢弃最旧条目并 emit warning event。
| return []; | ||
| } | ||
|
|
||
| if (sessionUpdate === 'agent_message_chunk' && text) { |
There was a problem hiding this comment.
[Critical] agent_message_chunk 和 thought_chunk 的 data.text 未经过任何过滤直接传入 AssistantMessage标签.content。与 sanitizeReason（至少处理了 ANSI）不同，这些内容完全没有清理。
如果 daemon 返回恶意或畸形的文本（例如极长字符串、控制字符、null bytes），会直接进入 React 渲染管线，可能导致：
- 渲染异常或崩溃
- 终端控制字符注入（如果 AssistantMessage标签 使用 dangerouslySetInnerHTML 或直接打印）
- OOM（如果 thought_chunk 是一个超长字符串）
建议：对所有用户可见的文本字段应用统一的清理管道。
| : prompt; | ||
| try { | ||
| return await this.session.prompt({ prompt: promptBlocks }); | ||
| } catch (error) { |
There was a problem hiding this comment.
[Critical] sendPrompt 出错时 emit disconnected + turn_complete，但不清理 busy 状态和 pending request。如果 session.prompt() 抛出异常（daemon 崩溃、网络超时），adapter 会：
- emit
disconnected（将 TUI 置为 disconnected 状态） - emit
turn_complete（但busy仍为true，因为busy=false只在事件循环的response_finished分支设置） currentPendingRequestId不会被清理
结果：即使 TUI 显示 disconnected，内部状态认为仍在处理中。后续 prompt() 调用会因 busy 检查被拒绝。
建议：在 catch 块中显式设置 busy = false 并清除 currentPendingRequestId。
| } | ||
| } | ||
|
|
||
| async cancel(): Promise<void> { |
There was a problem hiding this comment.
[Critical] cancel()、setModel()、approve()、reject() 四个方法都没有 try/catch。如果底层 session 方法抛出异常（连接已断、超时、协议错误），未捕获的异常会向上传播到 TUI 调用方，可能导致 UI 层未处理的 promise rejection 或进程崩溃。
建议：用 try/catch 包裹，捕获异常后 emit error event 并返回 false（而非让异常逃逸）。
| reason: 'event stream ended', | ||
| }); | ||
| } | ||
| } catch (error) { |
There was a problem hiding this comment.
[Critical] emit(disconnected, ...) 调用位于 pumpEvents 的 catch 块内。但 EventEmitter.emit() 在有 error listener 时可能抛出异常（如果没有 error listener，Node.js 会直接抛出未捕获异常）。如果 onUpdate 回调（注册为 disconnected listener）内部抛出异常，这个异常会逃逸到 catch 块，导致整个事件循环终止且无清理。
建议：在 catch 块内用 try/catch 包裹 emit 调用，或将 emit 移到 catch 块外部（先设置状态，再通知）。
|
Updated #4202 with Handled the latest valid review findings that are fixable inside this default-off TUI adapter spike:
Validation passed locally: cd packages/cli && npx vitest run src/ui/daemon/DaemonTuiAdapter.test.ts
cd packages/cli && npm run typecheck
cd packages/cli && npx eslint src/ui/daemon/DaemonTuiAdapter.ts src/ui/daemon/DaemonTuiAdapter.test.ts --max-warnings 0 --no-warn-ignored
cd packages/cli && npx prettier --check src/ui/daemon/DaemonTuiAdapter.ts src/ui/daemon/DaemonTuiAdapter.test.tsStill intentionally not claiming TUI parity/default migration here: daemon-side turn metadata / Generated by GPT-5 model |
| type: 'history', | ||
| item: { | ||
| type: 'info', | ||
| text: `Model switched to ${event.data['modelId']}`, |
There was a problem hiding this comment.
[Suggestion] model_switched 的 info 文本未经过清洗：Model switched to ${event.data['modelId']} 直接将 daemon 提供的 modelId 拼接到展示文本中。而 agent_message_chunk 和 agent_thought_chunk 路径都使用了 sanitizeDisplayText。daemon 注入的终端转义序列可能通过此路径在 TUI 中被解释执行。
| text: `Model switched to ${event.data['modelId']}`, | |
| text: `Model switched to ${sanitizeDisplayText(event.data['modelId'])}`, |
同理，formatPlan（~168-183）、formatToolContentText（~230-246）和 formatToolResultDisplay 的字符串分支也需统一清洗。
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| } | ||
| } | ||
|
|
||
| private reportDaemonFailure(error: unknown): void { |
There was a problem hiding this comment.
[Suggestion] reportDaemonFailure 同步调用 eventController?.abort() 并发出 disconnected，但不清理 eventPump / eventController。泵的 finally 块是异步的，在此窗口内 pumpEvents 可能仍在 for await 循环中运行，发出额外的事件。此外 start() 因 eventPump 仍非空而静默返回，直到泵的 finally 执行完毕。
| private reportDaemonFailure(error: unknown): void { | |
| private reportDaemonFailure(error: unknown): void { | |
| this.eventController?.abort(); | |
| this.eventController = null; | |
| this.eventPump = null; | |
| const message = sanitizeReason( | |
| error instanceof Error ? error.message : String(error), | |
| ); | |
| this.emit({ type: 'disconnected', reason: message }); | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| constructor(options: DaemonTuiAdapterOptions) { | ||
| this.session = options.session; | ||
| this.onUpdate = options.onUpdate; | ||
| this.lastSeenEventId = options.session.lastEventId; |
There was a problem hiding this comment.
[Suggestion] start() 通过 if (this.eventPump) { return; } 防止重复启动。但如果在 stop() 正在 await this.eventPump 时（泵的 finally 尚未运行）调用 start()，则 start() 静默返回。随后 finally 清除状态，adapter 进入僵尸状态——无泵运行、无法重启。
建议使用显式状态机（idle | running | stopping）替代对 eventPump 是否为 null 的检查，或在 start() 中等待当前泵完成后再重启。
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| async cancel(): Promise<void> { | ||
| try { | ||
| await this.session.cancel(); | ||
| } catch (error) { |
There was a problem hiding this comment.
[Suggestion] sendPrompt 从未清除 this.reducerState（toolCallsById/toolCallOrder）。上一轮的工具调用会在新轮次中累积并出现在 tool_group_update 中。MAX_TOOL_CALLS=128 的淘汰上限是跨轮次共享的，新工具调用可能被陈旧条目挤出。测试仅覆盖单轮次场景。
| } catch (error) { | |
| async sendPrompt( | |
| prompt: string | ContentBlock[], | |
| ): Promise<DaemonTuiPromptResult> { | |
| this.reducerState.toolCallsById.clear(); | |
| this.reducerState.toolCallOrder.length = 0; | |
| const promptBlocks = |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| } | ||
| } | ||
| } | ||
| return { |
There was a problem hiding this comment.
[Suggestion] toolUpdateToHistoryItem 每次 tool_call_update 都通过 Array.from(state?.toolCallsById.values() ?? [tool]) 返回完整工具列表（最多 128 条）。TUI 渲染器在每次状态转换时都收到完整数组，无法区分哪个工具发生了变化，导致不必要的数组分配和协调开销。
建议发送增量更新——仅包含变化的工具——或添加 changed 字段指示哪些 toolCallId 发生了变化，让 TUI 渲染器自行维护累积状态。
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| } | ||
|
|
||
| function stripControlSequences(value: string): string { | ||
| const esc = String.fromCharCode(27); |
There was a problem hiding this comment.
[Suggestion] stripControlSequences 在每次调用时创建 4 个 new RegExp()。由于此函数是每条 agent_message_chunk / agent_thought_chunk 流式块的必经之路，这些模式是静态的（ESC 字符不变），应提升为模块级常量。
| const esc = String.fromCharCode(27); | |
| const ESC = String.fromCharCode(27); | |
| const OSC_RE = new RegExp(`${ESC}\\][\\s\\S]*?(?:\\x07|${ESC}\\\\)`, 'g'); | |
| const DCS_RE = new RegExp(`${ESC}[P^_][\\s\\S]*?${ESC}\\\\`, 'g'); | |
| const CSI_RE = new RegExp(`${ESC}\\[[0-?]*[ -/]*[@-~]`, 'g'); | |
| const C1_RE = new RegExp(`${ESC}[@-Z\\\\-_]`, 'g'); | |
| function stripControlSequences(value: string): string { | |
| return value | |
| .replace(OSC_RE, '') | |
| .replace(DCS_RE, '') | |
| .replace(CSI_RE, '') | |
| .replace(C1_RE, ''); | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
e3b071d to
514654d
Compare
|
Updated #4202 with another TUI adapter hardening pass ( Handled the latest valid review findings:
Latest local validation passed: cd packages/cli && npx vitest run src/ui/daemon/DaemonTuiAdapter.test.ts
cd packages/cli && npm run typecheck
cd packages/cli && npx eslint src/ui/daemon/DaemonTuiAdapter.ts src/ui/daemon/DaemonTuiAdapter.test.ts --max-warnings 0 --no-warn-ignored
cd packages/cli && npm run buildOne suggestion remains intentionally not implemented in this PR: switching Generated by GPT-5 model |
|
Generated by GPT-5 model Status update after re-running the failed jobs:
Merge-safety check:
I marked the PR ready for review and requested review from @wenshao and @doudouOUC. |
| const withoutAnsi = stripControlSequences(reason); | ||
| let sanitized = ''; | ||
| for (const char of withoutAnsi) { | ||
| const code = char.charCodeAt(0); |
There was a problem hiding this comment.
[Suggestion] formatPlan 中的 content 和 status 来自 daemon 事件，直接拼接到 TUI 展示文本（${index + 1}. [${status}] ${content}），未经过 sanitizeDisplayText 清洗。其他路径（agent_message_chunk、agent_thought_chunk）使用了清洗，但 plan 路径遗漏了。
| const code = char.charCodeAt(0); | |
| const content = sanitizeDisplayText(getString(entry['content']) ?? ''); | |
| const status = sanitizeDisplayText(getString(entry['status']) ?? 'pending'); |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| if (typeof value === 'string') { | ||
| return value; | ||
| } | ||
| if ( |
There was a problem hiding this comment.
[Suggestion] formatToolResultDisplay 中 typeof value === 'string' 分支直接返回原始字符串作为 resultDisplay，未经过清洗。工具输出可能包含终端转义序列，会在 TUI 渲染时被解释。
| if ( | |
| if (typeof value === 'string') { | |
| return sanitizeDisplayText(value); | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| } | ||
| try { | ||
| return JSON.stringify(value); | ||
| } catch { |
There was a problem hiding this comment.
[Suggestion] formatToolContentText 从 daemon 事件的 tool call content block 中提取文本后用 \n 直接拼接，未经过 sanitizeDisplayText 清洗。这些文本来自工具 content 输出，可能携带终端转义序列。
| } catch { | |
| .filter((part): part is string => part !== undefined && part.length > 0) | |
| .map((part) => sanitizeDisplayText(part)); |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| } | ||
| } | ||
|
|
||
| function sanitizeReason(reason: string): string { |
There was a problem hiding this comment.
[Suggestion] sanitizeReason（line 164-178）和 sanitizeDisplayText（line 180-209）共享约 90% 逻辑：stripControlSequences + 逐字符过滤 + 长度截断。区别仅在于 max length（500 vs 20000）和保留的空白字符（仅 \n vs \t/\n/\r）。建议提取参数化函数以减少维护负担和分支发散风险。
| function sanitizeReason(reason: string): string { | |
| function sanitizeText( | |
| text: string, | |
| maxLength: number, | |
| preserveChars: Set<number>, | |
| ): string { | |
| const stripped = stripControlSequences(text); | |
| const chars: string[] = []; | |
| for (const char of stripped) { | |
| const code = char.charCodeAt(0); | |
| if ((code < 32 && !preserveChars.has(code)) || code === 127) continue; | |
| chars.push(char); | |
| if (chars.length >= maxLength) break; | |
| } | |
| return chars.join(''); | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| const kind = getString(update['kind']); | ||
| const rawOutput = formatToolResultDisplay(update['rawOutput']); | ||
| const contentOutput = formatToolContentText(update['content']); | ||
| const previous = state?.toolCallsById.get(toolCallId); |
There was a problem hiding this comment.
[Suggestion] toolUpdateToHistoryItem 中用 update['status'] === undefined（三等号）检查状态是否存在。如果 daemon 发送 status: null，会落入 mapToolStatus(null) → ToolCallStatus.Error，而实际上可能只是无状态字段的普通更新。建议用 == null 同时捕获 null 和 undefined。
| const previous = state?.toolCallsById.get(toolCallId); | |
| update['status'] == null | |
| ? (previous?.status ?? ToolCallStatus.Pending) | |
| : mapToolStatus(update['status']), |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
wenshao
left a comment
There was a problem hiding this comment.
Code Review: DaemonTuiAdapter
Verdict: REQUEST_CHANGES
4 Critical findings related to unsanitized daemon-sourced strings flowing into Ink/TUI rendering. The daemon is an untrusted source — any string it provides can contain ANSI escape sequences, control characters, or terminal manipulation payloads. The existing sanitizeDisplayText() helper is already defined in this file but only applied in a few code paths.
| # | Severity | Location | Issue |
|---|---|---|---|
| 1 | Critical | formatPlan L135 |
content and status from plan entries rendered without sanitization |
| 2 | Critical | formatToolResultDisplay L215 |
String rawOutput passed through without sanitizeDisplayText |
| 3 | Critical | toolUpdateToHistoryItem L292-293 |
kind, title, toolCallId flow into name/description unsanitized |
| 4 | Critical | formatToolContentText L246 |
Tool content text from daemon rendered without sanitization |
These findings complement @wenshao's existing review comments covering agent_message_chunk/thought_chunk (L364), model_switched, and formatUnknown — the same class of sanitization gap exists in several additional code paths.
— reviewed by qwen-code / mimo-v2.5-pro
| .map((entry, index) => { | ||
| const content = getString(entry['content']) ?? ''; | ||
| const status = getString(entry['status']) ?? 'pending'; | ||
| return `${index + 1}. [${status}] ${content}`; |
There was a problem hiding this comment.
[Critical] Unsanitized daemon string in formatPlan
content (L133) and status (L134) are extracted via getString() but never passed through sanitizeDisplayText() before being interpolated into rendered text. A malicious daemon can inject ANSI escape sequences or control characters through plan entry content.
| return `${index + 1}. [${status}] ${content}`; | |
| return `${index + 1}. [${sanitizeDisplayText(status)}] ${sanitizeDisplayText(content)}`; |
— qwen-code / mimo-v2.5-pro
| return undefined; | ||
| } | ||
| if (typeof value === 'string') { | ||
| return value; |
There was a problem hiding this comment.
[Critical] String rawOutput passes through unsanitized
When rawOutput is a string, it's returned directly without sanitizeDisplayText(). This is the most common path for tool output and the most dangerous — a daemon-controlled string can contain terminal escape sequences that manipulate the TUI.
| return value; | |
| return sanitizeDisplayText(value); |
— qwen-code / mimo-v2.5-pro
| const previous = state?.toolCallsById.get(toolCallId); | ||
| const tool: IndividualToolCallDisplay = { | ||
| callId: toolCallId, | ||
| name: kind ?? title ?? previous?.name ?? toolCallId, |
There was a problem hiding this comment.
[Critical] Unsanitized kind/title/toolCallId in tool name and description
kind, title, and toolCallId are daemon-sourced strings that flow directly into the name and description fields of IndividualToolCallDisplay, which are rendered in the TUI. None pass through sanitizeDisplayText().
| name: kind ?? title ?? previous?.name ?? toolCallId, | |
| name: sanitizeDisplayText(kind ?? title ?? previous?.name ?? toolCallId), | |
| description: sanitizeDisplayText(title ?? kind ?? previous?.description ?? toolCallId), |
— qwen-code / mimo-v2.5-pro
| } | ||
| const content = item['content']; | ||
| if (isRecord(content)) { | ||
| return getString(content['text']); |
There was a problem hiding this comment.
[Critical] Tool content text rendered without sanitization
getString(content['text']) and getString(item['text']) extract daemon-sourced strings that are joined and returned without sanitizeDisplayText(). This text flows directly into the TUI's tool result display. The sanitizeDisplayText helper is already available in this file.
| return getString(content['text']); | |
| const text = getString(content['text']); | |
| return text !== undefined ? sanitizeDisplayText(text) : undefined; | |
| } | |
| const text = getString(item['text']); | |
| return text !== undefined ? sanitizeDisplayText(text) : undefined; |
— qwen-code / mimo-v2.5-pro
| .map((entry, index) => { | ||
| const content = getString(entry['content']) ?? ''; | ||
| const status = getString(entry['status']) ?? 'pending'; | ||
| return `${index + 1}. [${status}] ${content}`; |
There was a problem hiding this comment.
[Critical] Unsanitized daemon string in formatPlan
content (L133) and status (L134) are extracted via getString() but never passed through sanitizeDisplayText() before being interpolated into rendered text. A malicious daemon can inject ANSI escape sequences or control characters through plan entry content.
| return `${index + 1}. [${status}] ${content}`; | |
| return `${index + 1}. [${sanitizeDisplayText(status)}] ${sanitizeDisplayText(content)}`; |
— qwen-code / mimo-v2.5-pro
| return undefined; | ||
| } | ||
| if (typeof value === 'string') { | ||
| return value; |
There was a problem hiding this comment.
[Critical] String rawOutput passes through unsanitized
When rawOutput is a string, it is returned directly without sanitizeDisplayText(). This is the most common path for tool output and the most dangerous — a daemon-controlled string can contain terminal escape sequences that manipulate the TUI.
| return value; | |
| return sanitizeDisplayText(value); |
— qwen-code / mimo-v2.5-pro
| const previous = state?.toolCallsById.get(toolCallId); | ||
| const tool: IndividualToolCallDisplay = { | ||
| callId: toolCallId, | ||
| name: kind ?? title ?? previous?.name ?? toolCallId, |
There was a problem hiding this comment.
[Critical] Unsanitized kind/title/toolCallId in tool name and description
kind, title, and toolCallId are daemon-sourced strings that flow directly into the name and description fields of IndividualToolCallDisplay, which are rendered in the TUI. None pass through sanitizeDisplayText().
| name: kind ?? title ?? previous?.name ?? toolCallId, | |
| name: sanitizeDisplayText(kind ?? title ?? previous?.name ?? toolCallId), | |
| description: sanitizeDisplayText(title ?? kind ?? previous?.description ?? toolCallId), |
— qwen-code / mimo-v2.5-pro
| } | ||
| const content = item['content']; | ||
| if (isRecord(content)) { | ||
| return getString(content['text']); |
There was a problem hiding this comment.
[Critical] Tool content text rendered without sanitization
getString(content["text"]) and getString(item["text"]) extract daemon-sourced strings that are joined and returned without sanitizeDisplayText(). This text flows directly into the TUI tool result display. The sanitizeDisplayText helper is already available in this file.
| return getString(content['text']); | |
| const text = getString(content["text"]); | |
| return text !== undefined ? sanitizeDisplayText(text) : undefined; | |
| } | |
| const text = getString(item["text"]); | |
| return text !== undefined ? sanitizeDisplayText(text) : undefined; |
— qwen-code / mimo-v2.5-pro
| this.start(); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
[Critical] emit() silently swallows all onUpdate callback exceptions. If the TUI renderer throws (e.g. after a data structure change), all daemon events are silently dropped forever — no log, no crash, no signal. The event pump keeps running normally but every onUpdate call is discarded, creating a frozen TUI with zero observability. This is the hardest bug to debug at 3 AM.
| } | |
| private emitFailureCount = 0; | |
| private emit(update: DaemonTuiUpdate): void { | |
| try { | |
| this.onUpdate(update); | |
| } catch (error) { | |
| if (++this.emitFailureCount === 1 || this.emitFailureCount % 100 === 0) { | |
| console.error('[DaemonTuiAdapter] onUpdate failed', error); | |
| } | |
| } | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| } | ||
| } | ||
|
|
||
| private reportDaemonFailure(error: unknown): void { |
There was a problem hiding this comment.
[Critical] reportDaemonFailure emits disconnected but the adapter has no setSession() — it's permanently broken after any RPC failure. The TUI sees disconnected, tries to recover by calling start(), which uses the same broken session, fails immediately, and triggers another disconnected. This creates an infinite loop: disconnect → reconnect → immediate disconnect → reconnect → …
Every daemon restart, network blip, or RPC timeout permanently bricks the adapter. The only recovery path is destroying and recreating the entire adapter instance, which the current architecture doesn't expose.
| private reportDaemonFailure(error: unknown): void { | |
| private sessionHealth: 'ok' | 'broken' = 'ok'; | |
| start(): void { | |
| if (this.sessionHealth === 'broken') { | |
| throw new Error('Session is broken; create a new DaemonTuiAdapter with a fresh session'); | |
| } | |
| // ... rest of start() | |
| } | |
| private reportDaemonFailure(error: unknown): void { | |
| this.sessionHealth = 'broken'; | |
| // ... rest | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
|
|
||
| async sendPrompt( | ||
| prompt: string | ContentBlock[], | ||
| ): Promise<DaemonTuiPromptResult> { |
There was a problem hiding this comment.
[Critical] All 5 RPC methods (sendPrompt, cancel, setModel, approvePermission, rejectPermission) re-throw the raw daemon Error after reportDaemonFailure. reportDaemonFailure sanitizes the message for the internal disconnected update, but the Error object propagated to the TUI caller retains the unsanitized message property. If the caller displays error.message, daemon-injected ANSI/OSC/DCS escape sequences execute in the terminal, bypassing the adapter's sanitization layer entirely.
| ): Promise<DaemonTuiPromptResult> { | |
| } catch (error) { | |
| this.reportDaemonFailure(error); | |
| const msg = sanitizeReason( | |
| error instanceof Error ? error.message : String(error), | |
| ); | |
| throw new Error(`Daemon RPC failed: ${msg}`); | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| .replace(C1_RE, ''); | ||
| } | ||
|
|
||
| function formatToolResultDisplay( |
There was a problem hiding this comment.
[Critical] formatToolResultDisplay passes structured objects (fileDiff, ansiOutput, todo_list, plan_summary, task_execution, mcp_tool_progress) through with an unchecked cast without sanitizing their internal string fields. A malicious daemon can embed ANSI/OSC/DCS sequences inside FileDiff.fileName, PlanResultDisplay.message, AnsiOutputDisplay.ansiOutput, or any nested string field, and they flow directly into the TUI rendering pipeline.
| function formatToolResultDisplay( | |
| if (typeof value['fileDiff'] === 'string') { | |
| return { | |
| ...value, | |
| fileDiff: sanitizeDisplayText(value['fileDiff']), | |
| fileName: typeof value['fileName'] === 'string' | |
| ? sanitizeDisplayText(value['fileName']) : value['fileName'], | |
| originalContent: typeof value['originalContent'] === 'string' | |
| ? sanitizeDisplayText(value['originalContent']) : value['originalContent'], | |
| newContent: typeof value['newContent'] === 'string' | |
| ? sanitizeDisplayText(value['newContent']) : value['newContent'], | |
| } as unknown as IndividualToolCallDisplay['resultDisplay']; | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| return []; | ||
| } | ||
| return [ | ||
| { |
There was a problem hiding this comment.
[Critical] The permission_request handler passes event.data through directly without sanitizing daemon-controlled display strings. toolCall.title, toolCall.kind, and options[].name flow unsanitized into the TUI permission confirmation dialog, creating an injection vector distinct from the tool call sanitization path.
| { | |
| case 'permission_request': { | |
| if (!isPermissionRequestData(event.data)) return []; | |
| const sanitized = { | |
| ...event.data, | |
| toolCall: { | |
| ...event.data.toolCall, | |
| title: typeof event.data.toolCall.title === 'string' | |
| ? sanitizeDisplayText(event.data.toolCall.title) : event.data.toolCall.title, | |
| kind: typeof event.data.toolCall.kind === 'string' | |
| ? sanitizeDisplayText(event.data.toolCall.kind) : event.data.toolCall.kind, | |
| }, | |
| options: event.data.options.map((opt) => ({ | |
| ...opt, | |
| name: typeof opt.name === 'string' ? sanitizeDisplayText(opt.name) : opt.name, | |
| })), | |
| }; | |
| return [{ type: 'permission_request', requestId: sanitized.requestId, request: sanitized, daemonEventId: event.id }]; | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
[Suggestion] sendPrompt returns session.prompt() result directly, including daemon-controlled stopReason, without sanitization. If the TUI renders stopReason (e.g. in footer or status display), it receives unsanitized daemon-controlled text.
| // Either sanitize stopReason before returning, or document that callers | |
| // must sanitize all string fields from the returned DaemonTuiPromptResult. | |
| const result = await this.session.prompt({ prompt: promptBlocks }); | |
| if (typeof result.stopReason === 'string') { | |
| result.stopReason = sanitizeReason(result.stopReason); | |
| } | |
| return result; |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| export interface DaemonTuiEvent { | ||
| id?: number; | ||
| v: 1; | ||
| type: string; |
There was a problem hiding this comment.
[Suggestion] DaemonTuiEvent.v: 1 is declared as a required field but never validated anywhere. When the daemon protocol upgrades from v1 to v2 and changes event data shapes, the adapter silently processes v2 events with v1 logic, producing corrupted TUI updates without any "protocol incompatible" error. This is the most subtle trap during protocol evolution.
| type: string; | |
| if (event.v !== 1) { | |
| this.emit({ | |
| type: 'history', | |
| item: { type: 'error', text: `Unsupported daemon protocol version: ${event.v}` }, | |
| }); | |
| continue; | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
|
|
||
| const title = getString(update['title']); | ||
| const kind = getString(update['kind']); | ||
| const safeToolCallId = sanitizeDisplayText(toolCallId); |
There was a problem hiding this comment.
[Suggestion] toolUpdateToHistoryItem never populates renderOutputAsMarkdown on the constructed IndividualToolCallDisplay. The in-process tool scheduler sets this field from tool metadata; without it, all daemon tool output renders as plain text even when markdown rendering is appropriate (e.g. web_fetch results).
| const safeToolCallId = sanitizeDisplayText(toolCallId); | |
| renderOutputAsMarkdown: kind === 'web_fetch' ? true : previous?.renderOutputAsMarkdown, |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| { | ||
| type: 'history', | ||
| item: { | ||
| type: 'gemini_thought_content', |
There was a problem hiding this comment.
[Suggestion] reduceDaemonEventToTuiUpdates is an exported function that directly mutates its state parameter (toolCallsById.set, toolCallOrder.push/shift). Its name and signature suggest a pure reducer, but it has side effects. Future maintainers writing unit tests may accidentally share state between test cases or be surprised by in-place mutation.
| type: 'gemini_thought_content', | |
| // Make it a private method of DaemonTuiAdapter instead of an exported function | |
| private reduceWithState(event: DaemonTuiEvent): DaemonTuiUpdate[] { | |
| // directly uses this.reducerState, avoiding reference-passing issues | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| item: { type: 'info', text }, | ||
| daemonEventId: event.id, | ||
| }, | ||
| ] |
There was a problem hiding this comment.
[Suggestion] tool_call and tool_call_update share the exact same code path, but semantically tool_call is the initial creation (carrying full kind/title/rawInput) while tool_call_update is an incremental delta (may only carry status or rawOutput). Tests only cover tool_call_update. If the daemon evolves to send richer initial data on tool_call, those fields will be silently ignored.
| ] | |
| // tool_call (initial) and tool_call_update (delta) share the same reducer | |
| // path because the daemon currently sends full snapshots for both. | |
| // If this changes, split the handling here. | |
| if (sessionUpdate === 'tool_call' || sessionUpdate === 'tool_call_update') { |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| for (const char of stripped) { | ||
| const code = char.charCodeAt(0); | ||
| if ( | ||
| (code < 32 && code !== 9 && code !== 10 && code !== 13) || |
There was a problem hiding this comment.
[Critical] sanitizeDisplayText and sanitizeReason do not strip C1 control characters (0x80-0x9F), including single-byte CSI (\x9B), DCS (\x90), and ST (\x9C). A malicious daemon can send single-byte CSI (\x9B31m) to inject terminal colors/controls, bypassing stripControlSequences (which only matches ESC[, not \x9B). The project's own packages/core/src/utils/terminalSafe.ts already handles this range with [\x00-\x1f\x7f-\x9f].
Additionally, neither sanitizer strips Unicode bidirectional control characters (U+200E/F, U+202A-U+202E, U+2066-U+2069) — the CVE-2021-42574 "Trojan Source" attack surface. A malicious daemon can inject \u202E to reverse-display model IDs, tool names, or error messages. The project's sanitizeForOsc already handles this.
| (code < 32 && code !== 9 && code !== 10 && code !== 13) || | |
| // In sanitizeDisplayText and sanitizeReason char loop, add: | |
| if (code >= 0x80 && code <= 0x9f) { | |
| continue; | |
| } | |
| // In stripControlSequences, add: | |
| .replace(/[\u200e\u200f\u202a-\u202e\u2066-\u2069]/g, '') |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| return terminalUpdates(event, reason); | ||
| } | ||
|
|
||
| default: |
There was a problem hiding this comment.
[Critical] reduceDaemonEventToTuiUpdates default: return [] silently drops unknown daemon event types with zero diagnostic output. If the daemon is upgraded and emits a new event type the adapter doesn't recognize, every such event is swallowed silently — no warning, no counter, nothing. The symptom is "TUI is missing some information" rather than a clear error, making this extremely hard to diagnose.
| default: | |
| // Track unknown event types and warn once per type: | |
| private static unknownEventTypes = new Set<string>(); | |
| // in default case: | |
| if (!DaemonTuiAdapter.unknownEventTypes.has(event.type)) { | |
| DaemonTuiAdapter.unknownEventTypes.add(event.type); | |
| console.warn(`[DaemonTuiAdapter] unknown event type: ${sanitizeDisplayText(event.type)}`); | |
| } | |
| return []; |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| this.eventController?.abort(); | ||
| if (this.eventPump) { | ||
| try { | ||
| await this.eventPump; |
There was a problem hiding this comment.
[Critical] stop() calls await this.eventPump with no timeout. If the daemon's events() async generator does not respect the AbortSignal and continues yielding events, the for await loop never exits and stop() blocks forever. At this point lifecycle === 'stopping', so start() only sets restartAfterStop = true and returns — the adapter is permanently bricked. The only recovery is killing the process.
| await this.eventPump; | |
| if (this.eventPump) { | |
| const timeout = new Promise<void>((resolve) => setTimeout(resolve, 5000)); | |
| try { | |
| await Promise.race([this.eventPump, timeout]); | |
| } catch {} | |
| if (this.lifecycle === 'stopping') { | |
| console.error('[DaemonTuiAdapter] pump did not drain within 5s, forcing idle'); | |
| this.lifecycle = 'idle'; | |
| } | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| } | ||
| } | ||
|
|
||
| async sendPrompt( |
There was a problem hiding this comment.
[Critical] sendPrompt has no lifecycle guard. When lifecycle === 'stopping', clearDaemonTuiReducerState() wipes reducer state while the pump may still be processing buffered events from the previous turn, and session.prompt() will fail because the controller is being aborted — triggering a spurious reportDaemonFailure → disconnected event. When lifecycle === 'idle', the prompt may succeed but there's no pump to consume the response events, causing silent prompt loss. cancel(), setModel(), approvePermission(), and rejectPermission() have the same issue.
| async sendPrompt( | |
| async sendPrompt(prompt: string | ContentBlock[]): Promise<DaemonTuiPromptResult> { | |
| if (this.lifecycle !== 'running') { | |
| throw new Error('Adapter is not running'); | |
| } | |
| clearDaemonTuiReducerState(this.reducerState); | |
| // ... | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| const MAX_DISPLAY_TEXT_LENGTH = 20_000; | ||
| const ESC = String.fromCharCode(27); | ||
| const OSC_RE = new RegExp(`${ESC}\\][\\s\\S]*?(?:\\x07|${ESC}\\\\)`, 'g'); | ||
| const DCS_RE = new RegExp(`${ESC}[P^_][\\s\\S]*?${ESC}\\\\`, 'g'); |
There was a problem hiding this comment.
[Suggestion] DCS_RE uses the character class [P^_] which matches only the literal characters P, ^, and _ — not the ASCII range P through _. This misses SOS (ESC X, Start of String) sequences. ECMA-48 requires all string sequences terminated by ST (ESC ) to be stripped: DCS (ESC P), SOS (ESC X), PM (ESC ^), and APC (ESC _). The missing X means ESC X ... ESC \ sequences bypass all four regexes.
| const DCS_RE = new RegExp(`${ESC}[P^_][\\s\\S]*?${ESC}\\\\`, 'g'); | |
| const DCS_RE = new RegExp(`${ESC}[PX^_][\\s\\S]*?${ESC}\\\\`, 'g'); |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
514654d to
4f9658f
Compare
|
Generated by GPT-5 model Handled the latest valid TUI daemon adapter review comments in Fixes included:
Triage/deferred items:
Local validation run after the change:
CI status at this check: latest Classify PR is green; Lint, CodeQL, and macOS/Ubuntu/Windows tests are queued for the new head. |
| } | ||
|
|
||
| private forceIdleAfterPumpTimeout(): void { | ||
| this.pumpGeneration += 1; |
There was a problem hiding this comment.
[Critical] forceIdleAfterPumpTimeout does not abort the old eventController — only the new one it just created. After the timeout fires, pumpEvents re-enters its loop with a fresh eventController while the stale generator from the previous iteration is still alive. This produces two concurrent pumps sharing the same session.events() stream, which double-processes events and corrupts TUI state.
Fix: capture this.eventController before the await, then abort it inside the timeout callback:
const stale = this.eventController;
const t = setTimeout(() => { stale?.abort(); … }, …);Also guard pumpEvents with a pumpGeneration counter so that after forceIdleAfterPumpTimeout creates a new controller, the old pump's finally block doesn't tear down the new one.
| ...request, | ||
| toolCall: { | ||
| ...sanitizedToolCall, | ||
| toolCallId: request.toolCall.toolCallId, |
There was a problem hiding this comment.
[Suggestion] sanitizePermissionRequest sanitizes toolCall via sanitizeDaemonValue (line 408) but then overwrites the sanitized toolCallId with the raw unsanitized value on line 410. The requestId is also passed through unsanitized from the caller (line 508). Both should be stripped through sanitizeDisplayText to match the defense-in-depth pattern used elsewhere.
| ]; | ||
| } | ||
|
|
||
| case 'permission_resolved': { |
There was a problem hiding this comment.
[Suggestion] The permission_resolved branch passes event.data['outcome'] directly into the update object without sanitization. The outcome value is daemon-controlled and should be validated against the expected PermissionOutcome shape ('selected' / 'cancelled') rather than forwarded opaquely, which could carry unexpected keys or crafted strings that reach the TUI permission UI.
| const C1_CSI_RE = /\x9b[0-?]*[ -/]*[@-~]/g; | ||
| const C1_STRING_RE = /[\x90\x98\x9e\x9f][\s\S]*?\x9c/g; | ||
| const BIDI_CONTROL_RE = /[\u200e\u200f\u202a-\u202e\u2066-\u2069]/g; | ||
| const UNKNOWN_EVENT_TYPES = new Set<string>(); |
There was a problem hiding this comment.
[Suggestion] UNKNOWN_EVENT_TYPES is a module-level Set that grows without bound. In a long-lived daemon session that emits many distinct unknown event types (or a malicious daemon that randomizes type names), this set accumulates indefinitely. Consider capping it (e.g. if (UNKNOWN_EVENT_TYPES.size < 100) UNKNOWN_EVENT_TYPES.add(…) or using a bounded cache with eviction).
wenshao
left a comment
There was a problem hiding this comment.
Review: DaemonTuiAdapter.ts (incremental from 514654d → 4f9658f)
Model: mimo-v2.5-pro
Verdict: REQUEST_CHANGES
Summary
The incremental diff adds ~250 lines of hardening (C1/bidi/DCS sanitization, pumpGeneration, busy flag, assertRunning, stop timeout, protocol version check, sanitizePermissionRequest, sanitizeDaemonValue, warnUnknownEventTypeOnce, MAX_PLAN_ENTRIES). The sanitization improvements are solid overall, but one critical concurrency bug and three sanitization gaps remain.
Findings
| # | Severity | Location | Summary |
|---|---|---|---|
| F1 | Critical | L837 | forceIdleAfterPumpTimeout does not abort the old eventController — creates dual pumps sharing session.events(), corrupting TUI state. Needs to capture the stale controller before the await and use pumpGeneration to guard cleanup. |
| F3 | Suggestion | L410 | sanitizePermissionRequest overwrites sanitized toolCallId with the raw unsanitized value. Same for requestId at L508. |
| F4 | Suggestion | L512 | permission_resolved passes event.data['outcome'] unsanitized. Should validate against expected PermissionOutcome shape. |
| F8 | Suggestion | L119 | UNKNOWN_EVENT_TYPES is an unbounded module-level Set. Should cap size (e.g. 100 entries). |
Deterministic checks
tsc: 0 errorseslint: 0 findingsvitest: 13/13 passed
See inline comments for details and suggested fixes.
wenshao
left a comment
There was a problem hiding this comment.
新一轮 review。上一轮的修复(stop drain、event resume、permission validation、tool reducer、sanitize)整体很扎实,control char / ANSI / OSC 防护尤其细致。这里再挑一些剩下的协议契约空白和工程细节,详见 inline。
结论:approve with minor nits。三条 Medium(timer 泄漏、reportDaemonFailure 太激进、v 不匹配每条都 emit)建议合并前补上 follow-up;其他在后续 wave 处理即可。
| setTimeout(() => { | ||
| timedOut = true; | ||
| resolve(); | ||
| }, STOP_TIMEOUT_MS); |
There was a problem hiding this comment.
🟠 Medium — setTimeout 没有 clearTimeout,5s 内泄漏一个 timer
new Promise<void>((resolve) => {
setTimeout(() => {
timedOut = true;
resolve();
}, STOP_TIMEOUT_MS);
}),pump 先完成时,这条 setTimeout 不会被取消,5s 后仍会 fire(只是没人 await 了)。长连续运行/频繁 start-stop 场景下每次 stop 都漏一个 timer 句柄,Node 的 active handles 会被它撑住延迟退出。
建议:
let timer: NodeJS.Timeout | undefined;
try {
await Promise.race([
pump.then(() => undefined, () => undefined),
new Promise<void>((resolve) => {
timer = setTimeout(() => { timedOut = true; resolve(); }, STOP_TIMEOUT_MS);
}),
]);
return !timedOut;
} finally {
if (timer) clearTimeout(timer);
}| private reportDaemonFailure(error: unknown): void { | ||
| if (this.lifecycle === 'running') { | ||
| this.lifecycle = 'stopping'; | ||
| this.eventController?.abort(); |
There was a problem hiding this comment.
🟠 Medium — 任何 RPC 失败都 abort 整条 SSE 流
reportDaemonFailure 把 lifecycle 切到 stopping 并 abort event controller。调用方包括 sendPrompt / cancel / setModel / approvePermission / rejectPermission。
也就是说,一次 transient 的 setModel 失败(比如模型名拼错、HTTP 429)就会把整个 SSE 订阅干掉,用户后续什么都收不到,必须重启 adapter。
建议区分:
- 致命错误(连接级、prompt 路径上的 abort)→ kill pump;
- 单次 RPC 业务失败 → 只把这次错误 surface 给调用方,SSE 继续。
至少为 cancel / setModel / approve|rejectPermission 留一条非致命路径。
| if (signal.aborted) { | ||
| break; | ||
| } | ||
| if ((event as { v?: unknown }).v !== 1) { |
There was a problem hiding this comment.
🟠 Medium — v !== 1 对每条事件都 emit 一条 error history
假设 daemon 升到 v2 后,SSE 里每条事件都会触发这条 history error,TUI 会被刷屏。
建议改成一次性 protocol mismatch 后 disconnect:
if ((event as { v?: unknown }).v !== 1) {
this.emit({ type: 'disconnected', reason: `unsupported protocol version: ${...}` });
break; // exit the for-await
}也可以在 start 之前的 capability 校验里就拒绝 v2 daemon。
| private emit(update: DaemonTuiUpdate): void { | ||
| try { | ||
| this.onUpdate(update); | ||
| } catch { |
There was a problem hiding this comment.
🟡 Low — emit() 完全静默吞掉 renderer 错误
private emit(update: DaemonTuiUpdate): void {
try {
this.onUpdate(update);
} catch {
/* isolate renderer callback failures from the daemon event pump */
}
}理由(隔离 renderer 抛错避免 pump 挂掉)成立,但完全没记录会让渲染器一直抛这种 bug 无法察觉,用户只看到 daemon 静默不响应。
至少加 debugLogger.warn 记录一下 error 和 update.type,运维和开发者能看到信号。
| throw new Error('A prompt is already in progress'); | ||
| } | ||
| this.busy = true; | ||
| clearDaemonTuiReducerState(this.reducerState); |
There was a problem hiding this comment.
🟡 Low — prompt 成功与否都先清掉 tool reducer state
现在的语义:每次 sendPrompt 一进来就 clearDaemonTuiReducerState,无论 prompt 是否成功。如果 prompt 立刻失败,上一轮的 tool 状态已经清空,用户失去了上一轮跑到一半发生了什么的上下文。
改成 prompt 成功提交后再清更稳妥:
try {
const result = await this.session.prompt(...);
clearDaemonTuiReducerState(this.reducerState);
return ...
}或者放到 turn-complete 事件之后清(daemon 协议补齐后)。
| ...request, | ||
| toolCall: { | ||
| ...sanitizedToolCall, | ||
| toolCallId: request.toolCall.toolCallId, |
There was a problem hiding this comment.
🔵 Nit — toolCallId / requestId / optionId 不 sanitize 的理由可以写在注释里
这里把 sanitize 后的 toolCallId 又覆盖回原始值,理由是这些 id 用于回查/对齐,sanitize 会破坏相关性。但没写注释,后续读者很容易觉得是 bug 漏 sanitize。
建议加一两行注释,顺便提醒 downstream log 输出 id 时要自己再 sanitize,避免日志注入。
| state.toolCallOrder.length = 0; | ||
| } | ||
|
|
||
| const MAX_TOOL_CALLS = 128; |
There was a problem hiding this comment.
🔵 Nit — MAX_TOOL_CALLS = 128 写死
对 TUI 一次会话来说 128 一般够,但深度依赖工具链的 turn 会爆。建议在 DaemonTuiAdapterOptions 里暴露 maxToolCalls?: number,默认 128。
| { | ||
| type: 'permission_resolved', | ||
| requestId: event.data['requestId'], | ||
| outcome: event.data['outcome'], |
There was a problem hiding this comment.
🔵 Nit — outcome 字段直接透传
和 channel bridge 那条评论同源:outcome: event.data['outcome'] 没做任何 shape 校验,消费方拿到的可能是 undefined 或任意结构。
建议要么 narrow 一下,要么在 DaemonTuiUpdate.permission_resolved.outcome 的类型上明确写 unknown + 消费方自己处理。
|
|
||
| ## Minimal Flow | ||
|
|
||
| 1. Create `DaemonClient` with daemon URL and token. |
There was a problem hiding this comment.
🟡 Low — capability 校验列表与 Minimal Flow 不对齐
这里只要求 session_create / session_prompt / session_events,但 Minimal Flow 第 6-8 步用了 session.cancel() / setModel() / respondToPermission()。
如果 daemon 不广告这些 capability,这些路径会在运行时静默崩。
要么补进必需 capability,要么在 doc 里说明它们可选(并解释为什么)。
| - Session-scoped permission route. | ||
| - Output sink refactor for JSONL / stream-json / dual-output parity. | ||
| - Session lifecycle close/delete semantics. | ||
| - Runtime diagnostics for MCP, skills, providers, and workspace env. |
|
Generated by GPT-5 Codex Processed the latest #4202 TUI daemon adapter comments with scoped changes only inside the adapter spike:
Triage notes:
Local validation:
|
wenshao
left a comment
There was a problem hiding this comment.
本地完整验证通过（worktree + tmux，HEAD de6cb66f4）：
npm ci✅npx vitest run src/ui/daemon/DaemonTuiAdapter.test.ts✅ 14/14 passednpm run typecheck✅npx eslint src/ui/daemon/DaemonTuiAdapter.{ts,test.ts} --max-warnings 0 --no-warn-ignored✅npx prettier --check(adapter + test + docs/developers/daemon-client-adapters/tui.md) ✅npm run build（cli 包 + 根 monorepo）✅git diff --check origin/main...HEAD✅- Remote CI: Lint / Test (mac+ubuntu+win) / CodeQL 全绿
R7 review 的 4 条均在最新 commit de6cb66f4 落实：
- F1 (Critical)
forceIdleAfterPumpTimeout— 先staleController?.abort()再pumpGeneration += 1，避免 stale pump 与新 pump 并存。 - F3
sanitizePermissionRequest—toolCallId保留原始值（opaque identifier 不应被 sanitize 改写，display 字段kind/title/options.name已过滤）。 - F4
permission_resolved.outcome— 通过sanitizePermissionOutcome严格校验cancelled/selected形状，非法 payload 返回undefined。 - F8
UNKNOWN_EVENT_TYPES—MAX_UNKNOWN_EVENT_TYPES = 100容量上限。 - 附带的 timer 泄漏 / cancel·setModel·permission RPC 失败不断连 SSE 也已修复。
改动范围 3 文件、+1970/-0，default-off TUI adapter spike，未接入默认交互 TUI 路径，符合 Stage 1.5 wave 工程原则（独立可合、向后兼容、默认关闭、可回滚、单测覆盖）。LGTM 👍
Picks up MCP guardrails (PR 14) + TUI daemon adapter spike (PR #4202). Two conflicts in status.ts / status.test.ts where PR 14's `'budget_exhausted'` error kind lands alongside our PR 16 `'stat_failed'` addition; both kept as union additions to the closed taxonomy, with the test comment updated to mention both PRs. Local validation: typecheck OK; serve+memory **513/513**; SDK **350/350**; eslint clean.
Summary
DaemonTuiAdapterspike that reduces daemon SSE frames into TUI-consumable updates and forwards prompt, cancel, model switch, and permission votes through a session-scoped daemon client.qwen serveHTTP + SSE, but the migration needs a default-off adapter layer before touching the existing Ink runtime.Validation
cd packages/cli && npx vitest run src/ui/daemon/DaemonTuiAdapter.test.ts.src/ui/daemon/DaemonTuiAdapter.test.ts (4 tests)andTest Files 1 passed.Scope / Risk
qwen servesmoke, no flag/env parsing, no default TUI wiring, no JSONL/stream-json/dual-output behavior changes, no output sink migration.Engineering principles checklist (Stage 1.5 wave PRs)
qwen serveStage 1 routes / SDK behavior preservedLinked Issues / Bugs
Related to #4175 Wave 5 adapter track and #4201.