feat(sdk/daemon-ui): unified completeness follow-up to #4328#4353
Conversation
📋 Review SummaryThis PR delivers a comprehensive follow-up to #4328, implementing a unified daemon UI layer across 5 coordinated commits (PR-A through PR-E). The changes introduce typed event schemas, server-side timestamps, state machine tracking, tool preview taxonomy, and render contract helpers. Test coverage is strong (77/77 passing), and the implementation demonstrates solid architectural thinking around forward-compatibility and cross-client consistency. 🔍 General FeedbackPositive aspects:
Architectural decisions:
Potential concerns:
🎯 Specific Feedback🟡 High
🟢 Medium
🔵 Low
✅ Highlights
|
wenshao
left a comment
There was a problem hiding this comment.
A deterministic typecheck also reports TS4111 in packages/webui/src/components/toolcalls/ShellToolCall.tsx for existing Record<string, unknown> dot-property accesses (description / command). Those lines are not part of the PR diff, so I am not posting them as inline comments, but the changed-file typecheck will still need to be clean before merge.
— gpt-5.5 via Qwen Code /review
Self-review — PR #4353 多轮审计 (9 commits / +5080 LOC)Multi-round self-review covering correctness / coverage / side effects / bundle size + cold start impact + React dependency check. Round 1 — 依赖与 bundle 影响React dependency
Conclusion: SDK has zero React dependency. This PR adds no new external deps. Bundle size — actual measurementCritical caveat — tree-shaking measurement. Simulated webui's actual import surface ( Implication:
Cold-start analysis
Round 2 — Test regression0 regressions across the entire SDK test suite. Round 3 — Per-commit correctness auditPR-A (event coverage)
PR-B (time schema)
PR-E (state machine)
PR-C (preview taxonomy + content extraction)
PR-D (render contract)
PR-F (5 additional preview kinds)
PR-G (conformance framework)
PR-H (WebUI migration)
PR-I (docs)
Round 4 — Side effect scanPublic API breaking changes
Type-level changes (additive widening only)
Round 5 — Coverage completeness audit (against original review)Cross-check each gap in the PR #4328 unified renderer review:
Completion ~95%, matching the PR description. Remaining 5% all declared in the daemon dependency declaration — waiting on daemon/Core landing. Round 6 — Known minor issues⚠ Minor 1 — Empty user.text produces trailing newlines in markdown
⚠ Minor 2 — MCP heuristic parses
|
| Dimension | Assessment |
|---|---|
| React dependency | ❌ None added — SDK pure |
| Full bundle size | +26 KB minified (acceptable for the feature surface) |
| Webui actual increase | +2.2 KB gzip (tree-shaking removes 75%) |
| Cold start | <0.5% impact, negligible |
| Side effects | 0 critical; 3 documented minor issues |
| Test regression | 0 (521/521 SDK tests pass) |
| Type-level breaking | clientReceivedAt required + union widening — internally self-consistent; downstream exhaustive switches need new cases (healthy) |
| Coverage completeness | ~95% of original review (rest explicitly declared as daemon/Core deps) |
| Mergeable | ✅ — no rollback or rewrite required |
The three minor issues are not blockers and can be follow-up.
Generated with assistance from Claude Opus 4.7 (claude-opus-4-7) — bundle sizes measured via esbuild --minify --bundle against the actual PR branch worktree; tree-shaking simulated with the webui's verbatim import set; full SDK test suite (vitest run) executed against post-PR HEAD.
768eb4e to
ae72935
Compare
|
Updated this PR in Handled the latest review threads as follows:
Verification:
Note: Generated by GPT-5 model. |
Daemon-side dependencies landed — F4 prereq #4360 merged@chiga0 — PR #4360 (F4 prereq — daemon protocol completion) merged into What's now emitted on the wire:
Plus addressed the state-divergence hazard Ilya0527 raised in #15 — SDK reducer now has SDK PR #4353 unblocking: the forward-compat field slots you preserved are no longer dead code on Caveat: my PR #4360 implements daemon→SDK protocol fields, but I left the SDK-side type definitions in your domain — the Anything else from your comment #19 P1 (subagent nesting + 🤖 Generated with Qwen Code |
| promptBusyRef.current = false; | ||
| store.reset(); | ||
| } else if (previousSessionId !== undefined) { | ||
| store.dispatch({ type: 'assistant.done', reason: 'reconnected' }); |
There was a problem hiding this comment.
[Critical] awaitingResync latch is never cleared after same-session SSE reconnect
When the daemon emits session.state_resync_required, the reducer sets awaitingResync = true and applyDaemonTranscriptEvent drops every non-passthrough event (transcript.ts:148). This same-session reconnect branch dispatches assistant.done { reason: 'reconnected' } but never calls store.clearAwaitingResync().
After reconnect, SSE events flow and the connection shows 'connected', but the transcript stays permanently frozen — all text deltas, tool updates, permission requests, and shell output are silently dropped with only a console.warn. The only recovery is a full page reload.
The store exposes clearAwaitingResync() (store.ts:81) specifically for this recovery path, but it's never invoked here.
| store.dispatch({ type: 'assistant.done', reason: 'reconnected' }); | |
| } else if (previousSessionId !== undefined) { | |
| store.dispatch({ type: 'assistant.done', reason: 'reconnected' }); | |
| store.clearAwaitingResync(); | |
| } |
— qwen3.7-max via Qwen Code /review
| } | ||
| } | ||
| } | ||
| updateCurrentToolPointer(state, event.toolCallId, event.status); |
There was a problem hiding this comment.
[Critical] updateCurrentToolPointer receives raw event.status — undefined bypasses the pointer update while the block is created as 'pending'
At line 455, the block is created with status: event.status ?? 'pending'. But here, updateCurrentToolPointer is called with the raw event.status. When event.status is undefined, the block's status is 'pending' (which is in IN_FLIGHT_TOOL_STATUSES), but updateCurrentToolPointer hits its if (status === undefined) return; guard at line 511 and exits without setting state.currentToolCallId.
Result: selectCurrentTool(state) returns undefined even though an in-flight tool block exists. Spinners and "running X" indicators that depend on currentToolCallId silently fail.
The same issue affects the update path at line 436.
| updateCurrentToolPointer(state, event.toolCallId, event.status); | |
| updateCurrentToolPointer(state, event.toolCallId, event.status ?? 'pending'); |
— qwen3.7-max via Qwen Code /review
| // Without this API the latch could only be cleared by `reset()`, | ||
| // which forces session-id reset semantics — wrong shape for the | ||
| // same-session-with-replay recovery flow. | ||
| clearAwaitingResync() { |
There was a problem hiding this comment.
[Critical] clearAwaitingResync() recovery flow is broken — replay events are dropped during drain
The JSDoc documents a recovery flow: "Re-subscribe with Last-Event-ID: 0 to receive a full replay, then call clearAwaitingResync() once the replay stream has drained." But during the replay drain, awaitingResync is still true, so applyDaemonTranscriptEvent drops every non-passthrough event (transcript.ts:148). After calling clearAwaitingResync() post-drain, the latch clears but zero replay data was incorporated — the transcript remains frozen with a permanent gap.
Conversely, calling clearAwaitingResync() before re-subscribing causes the full replay to apply on top of the existing transcript, producing duplicate blocks.
The only correct recovery (reset() + full replay) is the one the doc says to avoid.
Consider either:
- Correcting the JSDoc to document that
clearAwaitingResync()is for "accept the gap and resume" recovery only, pointing toreset()for full replay - Adding a
clearAwaitingResync({ resetState: true })overload that also clears blocks/indexes for a clean replay
— qwen3.7-max via Qwen Code /review
…k + 10 more Walks 13 inline items from wenshao's 16:46-17:28 reviews. 11 fixed, 1 deduped (lint-no-console flagged in both reviews), 1 reverted/push-back (multi-part deny re-flags the same design-intent territory as R2 QwenLM#4). ## Critical fixes ### sanitizeUrl: OAuth #fragment leak `sanitizeUrl` cleared query params and Basic Auth userinfo, but `u.toString()` preserved `u.hash`. OAuth 2.0 implicit grant puts `access_token=...` directly in the fragment (e.g., `https://app/#access_token=gho_xxx&token_type=bearer`); some Azure SAS variants similarly. Now `u.hash = ''` before serialize. For rendered output (markdown / HTML / plaintext), the fragment is client- state-only and dropping it removes the entire fragment-side leak surface. ### ESLint no-console on awaitingResync diagnostic Project lint forbids bare `console.*`. Added `eslint-disable-next-line no-console -- intentional diagnostic` per wenshao's suggestion. Behavior unchanged. ### normalizeAuthDeviceFlowCancelled test coverage (still missing post-R4) R4 added tests for one of the five device-flow normalizers; the `cancelled` variant was still uncovered. Added happy + malformed-payload tests. ## Behavior fixes ### Plaintext sanitizeTerminalText parity `daemonBlockToPlainText` + `daemonToolPreviewToPlainText` previously returned ANSI/bidi-control text verbatim, while markdown and HTML paths sanitized via `sanitizeTerminalText`. A daemon emitting bidi overrides survived clean to plaintext output — contradicting the "copy-paste / logs" JSDoc intent. Now routes every text field through `clean()` = `cap(sanitizeTerminalText(raw))`. ### blockquote helper applied to image_generation + subagent_delegation R3 added the helper for thought/debug/error but missed two preview markdown sites (`> ${text(preview.prompt)}` for image_generation, `> ${text(preview.task)}` for subagent_delegation). Multi-line prompts / tasks now stay inside the blockquote. ### Default unrecognized-event branch: single debug block Was emitting `status + debug` (2 blocks) per unknown event type. In long sessions where the daemon adds new types an older SDK doesn't recognize, this doubled block-consumption rate and accelerated `maxBlocks` trimming of real content. Now emit a single `debug` block that prefixes the event-type for adapters that want to pattern-match. ### writeIntent regex underscore-boundary aware R4's `content` alias gate-check used `\b` word boundaries, but `\b` doesn't match between `write` and `_` in `write_file` (both `\w`). Fixed to `(?:^|[_-])verb(?:$|[_-])` which catches the canonical `write_file` naming AND still rejects `prewrite_check`. Verb list extended per wenshao's suggestion (`overwrite`/`modify`/`patch`/`generate`). ### useDaemonPendingPermissions over-subscription Hook used `useDaemonTranscriptState()` which fires on every daemon event (text deltas, tool updates, sidechannel). Switched to `useDaemonTranscriptBlocks()` which only invalidates when the blocks array reference changes — block-mutating dispatches only, thanks to lazy COW. Same selector semantics, ~10x fewer renders in chat-heavy sessions. ### Conformance suite: try/catch adapter JSDoc promised "does not throw" but the loop wrapped adapter calls without try/catch. Buggy adapters aborted the whole suite instead of producing a structured `ConformanceFailure`. Now wrap; on throw, capture the error message in `renderedExcerpt: "[adapter threw: ...]"` and continue. ## Type / Quality fixes ### DaemonTranscriptState.blocks typed readonly Runtime contract is frozen (lazy-COW poison defense), but the type was mutable — consumers got runtime `TypeError` for in-place mutation instead of compile errors. Now `readonly DaemonTranscriptBlock[]` so mutation is caught at the type level. ### formatMissedRange exported / deduplicated Helper was duplicated inline between transcript.ts (full phrasing) and terminal.ts (terser phrasing). Exported from transcript.ts and reused in terminal.ts to prevent future drift. ## Push-back (false-positive — see reply) ### classifySelectedPermissionOption multi-part deny (`selected:deny:access_violation`) Re-flags the same `selected:X` design intent rejected in R2 QwenLM#4. The caller comment explicitly states a selected option resolves the prompt even when the option id contains `deny`/`cancel`. The existing test `cancelled-substring-permission` (payload `selected:abort`, expected `completed`) codifies this. Daemon expresses true user-cancellation via the `cancelled` PRIMARY token, not `selected:cancel`. Not changing; reply directs to the same R2 QwenLM#4 reasoning. ## Tests added (+10) - normalizeAuthDeviceFlowCancelled happy + malformed - sanitizeUrl OAuth fragment access_token rejected - sanitizeUrl AWS/GCP/Azure SAS credential params stripped - formatMissedRange no-gap / single-event / multi-event - detectFileDiff content alias rejected for read-like tools - detectFileDiff content alias accepted for write-like tools - writeIntent word boundaries (prewrite_check NOT matched) - conformance captures adapter throw - unrecognized event → single debug block - store.clearAwaitingResync clears latch ## Validation | | | |---|---| | SDK tests | **172/172** (was 162, +10) | | WebUI tests | **9/9** | | SDK typecheck | clean | | WebUI typecheck | clean | Generated with AI Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
R5 review batch — `e394f4935` + 1 push-backThanks @wenshao — 3 reviews (16:46 / 16:58 / 17:28) + the cold-start ✅ PASS verification. Fixed (11)Critical
Behavior
Type / Quality
Pushed back (1) — false-positive per R2 #4 precedent`classifySelectedPermissionOption` multi-part deny (`selected:deny:access_violation`) This re-flags the same `selected:X` design territory rejected in R2 #4. The caller comment at `transcriptAdapter.ts:301-304` explicitly states a selected option resolves the prompt even when the option id contains `deny`/`cancel`/`abort`. The existing test `cancelled-substring-permission` (input `selected:abort`, expected `completed`) codifies this contract. Daemon expresses true user-cancellation via the `cancelled` PRIMARY token (handled at the caller layer in `classifyPermissionResolution`), not nested under `selected:`. A `selected:deny:access_violation` payload is a selected option whose ID happens to contain colon-separated tokens — still a successful selection per the contract. If the daemon ever emits `selected:cancel` to mean "user pressed Cancel button", the daemon side is malformed and should be fixed there. The SDK side should not silently change the resolved status based on label heuristics. Not changing; flagging the source comment to prevent future re-flag. Verification 🎉@wenshao your cold-start verification (2026-05-23 16:51) shows ✅ PASS across 16 sub-tests (conformance / lifecycle / redaction / sub-agent nesting / out-of-order / approval mirror / event ordering / Intl / forward-compat / browser-safe assertion / preview taxonomy / sensitive-key 15 variants / maxFieldLength / broken-adapter). Independent verification on a fresh consumer project that only uses the public `@qwen-code/sdk/daemon` export — huge confidence boost for downstream embedders. Validation
Head: `e394f4935`. |
…pointer
Three Criticals from R6 review (4351217188) all pointing at real bugs
introduced by R4/R5 work — not false positives. Fixes plus regression
tests.
## Critical 1 — same-session reconnect never clears the latch
When the daemon emitted `state_resync_required`, the reducer set
`awaitingResync = true`. The webui provider dispatched
`assistant.done { reason: 'reconnected' }` after re-attaching SSE but
never called `store.clearAwaitingResync()`. Result: events flowed in
on the fresh stream but every one got dropped by the
`applyDaemonTranscriptEvent` passthrough guard. Transcript appeared
permanently frozen with no diagnostic clue (the `console.warn` fired
on each drop, but the user wouldn't necessarily check DevTools).
Fix: in `DaemonSessionProvider.tsx`, after dispatching the synthetic
`reconnected` `assistant.done`, check `awaitingResync` and clear it
BEFORE the new SSE event loop starts.
## Critical 2 — updateCurrentToolPointer breaks on undefined status
In `upsertToolBlock`, a new tool block is created with
`status: event.status ?? 'pending'`. But `updateCurrentToolPointer`
was called with raw `event.status` — when undefined, the function's
own `if (status === undefined) return;` guard short-circuited without
ever pointing at the new (visually-pending) block.
Result: `selectCurrentTool` returned `undefined` for daemon events
that omitted the explicit `status` field, while the block sat at
"pending" in the UI — invisible to the current-tool selector.
Fix: pass the EFFECTIVE status (`event.status ?? 'pending'`) so the
pointer logic mirrors the actual stored status.
## Critical 3 — clearAwaitingResync flow chicken-and-egg
The earlier (R4) JSDoc documented the recovery flow as: "re-subscribe
with `Last-Event-ID: 0`, then call clearAwaitingResync after replay
drains." But while the latch is true, EVERY non-passthrough event is
dropped at `applyDaemonTranscriptEvent`. So during the replay drain,
zero events made it into state, and clearing the latch afterward did
nothing — transcript permanently empty.
Correct flow: clear FIRST, then stream events. Updated JSDoc on both
`types.ts` interface and `store.ts` impl to document this clearly.
Added a regression test (`clearAwaitingResync AFTER dispatching events:
events ARE dropped`) that pins the correct flow in code.
## Regression tests (+3)
- `undefined status` creates pending block AND sets currentToolCallId
- clear-then-dispatch ✓ events flow
- dispatch-then-clear ✗ events dropped (correct flow documentation)
## Validation
| | |
|---|---|
| SDK tests | **175/175** (was 172, +3) |
| WebUI tests | **9/9** |
| SDK typecheck | clean |
| WebUI typecheck | clean |
## Note on doudouOUC heads-up
QwenLM#4469 (main → daemon_mode_b_main sync, 45 commits since 2026-05-19)
will land soon. doudouOUC's note says rebase should be smooth (no
daemon-ui surface conflicts). Will rebase on the cron's next pass
after QwenLM#4469 merges.
Generated with AI
Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
R6 — 3 Criticals all fixed in `971a69d14` (real bugs, not false-positives)Thanks @wenshao — all three Criticals in review 4351217188 pointed at real bugs I introduced in R4/R5 work. Walked the gates and confirmed. C1 — same-session reconnect never clears the latchYou're right. After dispatching the synthetic `assistant.done { reason: 'reconnected' }`, the provider never called `clearAwaitingResync()` — every event from the fresh SSE stream got dropped by the latch guard. Fix in `DaemonSessionProvider.tsx`: after the reconnect-dispatch, check `awaitingResync` and clear BEFORE the new event loop starts. C2 — updateCurrentToolPointer undefined-status guard mismatchYou're right. `upsertToolBlock` writes `status: event.status ?? 'pending'` to the block, but called `updateCurrentToolPointer` with raw `event.status`. When undefined, the pointer logic short-circuited while the block sat at "pending" — invisible to `selectCurrentTool`. Fix: pass effective status (`event.status ?? 'pending'`) so pointer mirrors the stored status. C3 — clearAwaitingResync flow was chicken-and-eggYou're right. My R4 JSDoc said "clear AFTER replay drains" — but while the latch is true, every replay event gets dropped. Clearing afterward produced an empty transcript. Fix: corrected JSDoc on both `types.ts` interface and `store.ts` impl to document the correct flow:
Added a regression test that pins the correct flow (`dispatch-then-clear events ARE dropped`) so future revisions can't silently regress this. Regression tests (+3)
Validation
Head: `971a69d14`. cc @doudouOUC — saw the #4469 heads-up. Will rebase on the next monitor cron pass after it merges. |
| opts: DaemonRenderOptions = {}, | ||
| ): string { | ||
| const capped = capLength(opts)(sanitizeTerminalText(raw)); | ||
| return capped.replace(/([\\`*_{}[\]()#+!>-])/g, '\\$1'); |
There was a problem hiding this comment.
[Suggestion] escapeMarkdownText escapes > but omits < from the character class. When markdown output is rendered through an HTML-backed pipeline (e.g., markdown-it with html: true), tool names or titles containing <img src=x onerror=...> or <script> would pass through as raw HTML.
| return capped.replace(/([\\`*_{}[\]()#+!>-])/g, '\\$1'); | |
| return capped.replace(/([\\`*_{}[\\]()#+!><-])/g, '\\$1'); |
The HTML render path (daemonBlockToHtml) correctly escapes < via defaultEscapeHtml, so this only affects the markdown projection. Adding < to the escape set makes the markdown output safe for HTML-backed renderers without changing behavior for pure-markdown consumers.
— qwen3.7-max via Qwen Code /review
wenshao
left a comment
There was a problem hiding this comment.
No high-confidence review findings. All 204 tests pass, TypeScript typecheck clean, ESLint clean (1 non-blocking warning). 11 low-confidence suggestions identified for human review (awaitingResync recovery paths, credential exposure in render, permission classification consistency, test coverage gaps). — qwen3.7-max via Qwen Code /review
| // poisoning future snapshots. Internal reducer mutation goes through | ||
| // `takeBlocksOwnership` which copies BEFORE mutating, so the frozen | ||
| // shared reference is never touched in-place by the next dispatch. | ||
| Object.freeze(result.blocks); |
There was a problem hiding this comment.
[Suggestion] appendLocalUserTranscriptMessage (line 100) returns trimTranscriptState(next) without Object.freeze. After a user message, the blocks array is mutable — a consumer casting away readonly and calling .sort() would succeed silently, poisoning the lazy-COW WeakMap caches.
The TypeScript type provides compile-time safety, but the stated intent of this freeze (catching casts at runtime in strict mode) is inconsistent between the two public state-producing functions.
| Object.freeze(result.blocks); | |
| Object.freeze(result.blocks); | |
| return result; | |
| } | |
| // NOTE: also apply Object.freeze in appendLocalUserTranscriptMessage above | |
| // to maintain consistent runtime immutability defense. |
— claude-opus-4-7 via Claude Code /qreview
| // `Expires` is included because in signed-URL contexts it pairs with | ||
| // the credential; non-signed URLs typically don't include it as a | ||
| // top-level query param so the false-positive risk is bounded. | ||
| const AZURE_SAS_KEYS = new Set([ |
There was a problem hiding this comment.
[Suggestion] AZURE_SAS_KEYS is allocated per sanitizeUrl() call. In a render pass over many tool blocks with URLs, this creates unnecessary GC pressure from repeated 16-element Set construction.
Hoist to module scope — it's a static constant:
| const AZURE_SAS_KEYS = new Set([ | |
| const AZURE_SAS_KEYS = new Set([ |
→ move to module level (next to DEFAULT_MAX_FIELD_LENGTH):
const AZURE_SAS_KEYS: ReadonlySet<string> = new Set([
'sv', 'se', 'sr', 'sp', 'st', 'spr', 'sip', 'ss', 'srt', 'sig', 'skoid',
'sktid', 'skt', 'ske', 'sks', 'skv',
]);— claude-opus-4-7 via Claude Code /qreview
| // line 509 already did this; plainText was missed in the prior | ||
| // doudouOUC fix. | ||
| const preview = daemonToolPreviewToPlainText(block.preview, opts); | ||
| const status = `status: ${block.status}`; |
There was a problem hiding this comment.
[Suggestion] block.status is interpolated raw — missed by the R5 clean() pass that sanitized all other fields in this function.
The markdown path (line 81) correctly uses escapeMarkdownText(block.status, opts) and the HTML path (line 565) uses sanitizer(block.status). For consistency:
| const status = `status: ${block.status}`; | |
| const status = `status: ${clean(block.status)}`; |
— claude-opus-4-7 via Claude Code /qreview
Verification re-run — PR #4353 @
|
…URL sanitization Two items from wenshao R7 (one inline Suggestion + one Verification-PASS finding). Both gate-checked as real; fixed. ## escapeMarkdownText: add `<` to escape set Markdown rendered through markdown-it with `html: true` would previously pass through raw `<img onerror>` / `<script>` from reviewer-untrusted metadata fields (tool title / toolKind / status / permission label / preview labels). The HTML render path already escapes via `defaultEscapeHtml`; this brings markdown to the same safety baseline. Note: `escapeMarkdownText` is only applied to metadata fields, NOT to assistant/user/thought body text (those are intentionally markdown content; escaping `<` there would mangle legitimate markdown). ## markdown tool details: sanitize URL credentials when sanitizeUrls:true `daemonBlockToMarkdown`'s `case 'tool':` branch appended `block.details` (serialized `rawInput` JSON) through `text()` which only handled ANSI/bidi. When `rawInput.url` contained credentials (Basic Auth in userinfo / OAuth in `#fragment` / signed-URL query params), the preview path correctly sanitized via `sanitizeUrl`, but the details dump leaked the raw URL. HTML + plaintext branches exclude details entirely, so they didn't leak. The asymmetry meant a consumer rendering markdown + relying on the R5 fragment-leak protection would still leak via details. Fix: added `sanitizeUrlsInText(text)` helper that regex-replaces every `https?://` URL in a string with its `sanitizeUrl(url)` form. Applied to `block.details` in the markdown tool case when `opts.sanitizeUrls`. Default behavior unchanged (back-compat for consumers not opting in). ## Tests (+3) - escapeMarkdownText escapes `<` in metadata fields, but not assistant body - markdown tool details strips Basic Auth / query token / x-amz / OAuth fragment when sanitizeUrls:true - default (sanitizeUrls:false) preserves URLs in details verbatim ## Validation | | | |---|---| | SDK tests | **178/178** (was 175, +3) | | WebUI tests | **9/9** | | SDK typecheck | clean | | WebUI typecheck | clean | ## Verification re-run acknowledgment @wenshao your second cold-start verification (PR QwenLM#4353 @ 971a69d) caught the details-dump leak that the v1 verification didn't surface because v1's probe targeted preview URLs only. R7 fix closes that gap; markdown / HTML / plaintext now have symmetric URL-credential handling when sanitizeUrls is enabled. Generated with AI Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
R7 — 2 items addressed in `473614d02` (post-APPROVED hardening)Thanks @wenshao — APPROVED + cold-start verification ✅ PASS noted. Two substantive items also surfaced; both real, both fixed. Item 1 — `escapeMarkdownText` missing `<` (inline Suggestion, review 4351555418)Verified — title / toolKind / status / preview labels were all reviewer-untrusted metadata going through `escapeMarkdownText`, and `<` wasn't in the escape set. A markdown-it pipeline with `html: true` would pass ` Fix: added `<` to the escape character class. `escapeMarkdownText` callers (metadata fields only — NOT assistant/user/thought body text, which are markdown content and must stay un-escaped) now produce `\<` for Item 2 — markdown `block.details` bypassed `sanitizeUrl` (verification finding)This was the asymmetry your second cold-start verification caught — the v1 probe targeted preview URLs only and didn't surface the details-dump leak. `daemonBlockToMarkdown`'s `case 'tool':` appends `block.details` (serialized `rawInput` JSON) through `text()` which strips ANSI/bidi but doesn't touch URL credentials. When `rawInput.url` carries Basic Auth userinfo / OAuth `#fragment` / signed-URL query params, the preview path correctly sanitized but the details dump leaked. HTML + plaintext branches deliberately exclude details, which is why your probe found: ``` Fix: added `sanitizeUrlsInText(text)` helper that regex-replaces every `http(s)://` URL in a string with its `sanitizeUrl(url)` form. Applied to `block.details` in the markdown tool case when `opts.sanitizeUrls: true`. Default (no opt-in) preserves URLs verbatim per existing contract. Result: when `sanitizeUrls: true`, markdown / HTML / plaintext now have symmetric URL-credential handling. Validation
Re. the 11 low-confidence suggestions from the R5 reviewer summaryLooking through the cron monitor log — most of these have either been fixed (R4/R5/R6) or are documented forward-compat / design intent (the `selected:X` push-back, the lenient `isDeviceFlowErrorKind` per the public type's `(string & {})` arm). If any specific one is still live blocking, please flag and I'll walk it. Head: `473614d02`. Bundle delta: marginal (+~500B for the two helper additions; still well under the 100 KB browser cap). |
R7 verification — ✅ leak closedRe-ran the same The new No regressions:
Bundle 93,522 B (+112 B over R6, +895 B total since v1). Still well under the 100 KB browser cap. Additional escapeMarkdownText Verdict update from v2: PASS, no remaining merge-time findings. |
Summary
Unified follow-up to #4328 — closes every SDK-only gap from the unified-renderer-layer review so library-embedder consumers (web chat, web terminal, and any third-party host built on
@qwen-code/sdk/daemon+@qwen-code/webui) all render the same transcript the same way. Native TUI, channel adapters (DingTalk / Telegram / WeChat), and IDE companions stay on their existing direct ACP paths and are NOT in this PR's adoption scope (seedocs/developers/daemon-ui/README.md).#4328 shipped the v1 transcript-layer skeleton (~55%). This PR brings the daemon UI surface to ~95% completeness — the remaining 5% is daemon/Core work outside the SDK package, declared in TODO §B / §D below.
What this PR delivers
1. Full daemon event coverage (was 13 types → now 28+)
The normalizer used to fall back to
debugfor 16 of the daemon's emitted event types (session-meta, workspace Wave 3/4, auth device-flow, etc.). Adapters had no way to dispatch on them without greppingdebug.text. This PR types every one —session.metadata.changed,session.approval_mode.changed,workspace.mcp.budget_warning,auth.device_flow.failed, and so on — with closed-enum fields where the daemon protocol defines them (errorKind,provenance,serverId).Benefit: adapters get a typed discriminated union to switch on. Forward-compat for new daemon events still routes through
debugcleanly; no exhaustiveness failures.2. Cross-client time consistency
DaemonUiEventBase.serverTimestamp?+DaemonTranscriptBlockBase.clientReceivedAt, plusselectTranscriptBlocksOrderedByEventId(daemon-monotonic ordering) andformatBlockTimestamp(Intl-based locale-aware formatter).Benefit: when multiple clients attach to the same session, "X minutes ago" labels and block ordering stay consistent regardless of each client's local clock drift. Survives SSE replay-after-reconnect because the daemon's
eventIdis the primary sort key.3. Reducer state machine — currentTool / approvalMode / cancellation
DaemonTranscriptStatenow tracks sidechannel state alongside the block list:currentToolCallId— which tool is in-flight right now (auto-maintained on tool lifecycle transitions)approvalMode— mirrored fromsession.approval_mode.changedtoolProgress— ready for the (still-pending)tool.progresseventassistant.done.reason === 'cancelled', every in-flight tool's status flips to'cancelled'automatically — daemon doesn't guarantee a terminaltool_call_updatefor every in-flight tool when the parent prompt is cancelledBenefit: UIs stop showing "tool spinning forever" after cancel. Renderers can read
selectCurrentTool(state)instead of scanning blocks. NewselectSubagentChildBlocksexposes sub-agent delegation as a queryable tree (via the daemon's_meta.parentToolCallIdstamp).4. Render contract — markdown / HTML / plain text
daemonBlockToMarkdown/daemonBlockToHtml/daemonBlockToPlainText/daemonToolPreviewToMarkdown— four projection helpers that take a block and return a renderable string. Conservative HTML sanitizer (ANSI strip → HTML escape;role="alert"for errors).sanitizeUrlsstrips token-shaped query params from CDN/auth URLs.maxFieldLengthtruncation caps any single field at 8192 chars by default.Benefit: web chat, web terminal, IDE extension, and any future adapter all render identically by default. Adapters opt into custom rendering per
block.kind/preview.kindonly where they need it. No more per-adapter projection drift.5. Tool preview taxonomy — 4 → 13 kinds
file_diff·file_read·web_fetch·mcp_invocation·code_block·search·tabular·image_generation·subagent_delegation·ask_user_question·command·key_value·generic. Each detected from tool input shape; each with a markdown + plain-text projection.Benefit: tool calls render with appropriate per-kind affordances (unified diff for edits, MCP server badge for MCP calls, image thumbnail for generation tools, etc.) without each adapter writing its own switch.
6. Adapter conformance framework
runAdapterConformanceSuite(adapter)+ an embedded fixture corpus (11 fixtures including subagent nesting, redaction, cancellation, mcp-budget, auth-device-flow). Adapters run this in their own test suite and surface projection drift before users see it.Benefit: when a new daemon event or preview kind lands, every adapter that runs conformance sees a failing fixture instead of silently displaying nothing — projection drift is caught in CI, not in user reports.
7. WebUI migration
packages/webui'stranscriptAdapternow bridges through the SDK render contract. Opt-in flags (useMarkdown,enrichToolDetailsWithPreview) preserve legacy default behavior for incremental rollout.Benefit: web chat starts consuming the shared render layer immediately; rich previews (file diffs, MCP, tabular) surface without webui adding kind-specific components.
8. Sensitive-field redaction at the normalizer boundary
redactSensitiveFieldswalks tool input/output/content/locations and redacts values forapiKey/token/secret/password/authorization/cookie/bearertoken/accesstokenetc. (closed list, normalized for case/separators) before they reach transcript blocks.Benefit: a buggy debug panel or naive
JSON.stringify(block)can't leak credentials. Tests verify end-to-end (Bearer secret-do-not-leaknever appears in any serialized event).9. Sub-agent nesting
When the daemon stamps
_meta.parentToolCallId+_meta.subagentTypeon a tool call (theTask-equivalent delegation pattern), the reducer correlates child tool blocks under their parent (parentBlockId). Out-of-order arrival (child before parent) is handled — back-fill happens when the parent appears, or when a later child update arrives.Benefit: renderers can draw nested sub-agent activity (folder-header + indented children) without re-correlating on every render.
selectSubagentChildBlocks(state, parentId)returns direct children in O(1) after first build.10. Performance & correctness hardening
Lazy copy-on-write in the reducer (
state.blocksreference preserved across sidechannel-only dispatches → WeakMap caches for sort + children-index actually hit). Cancellation iterates only non-trimmed entries. Tool progress + permission block index pruned post-trim to bound memory in long sessions.Benefit:
useSyncExternalStoreconsumers don't pay an O(n log n) re-sort on every dispatch when only metadata changed.11. Adapter author documentation
docs/developers/daemon-ui/README.md— full API reference with cookbook (markdown / HTML / plain-text rendering, sub-agent nested rendering, sensitive-field handling, time formatting).docs/developers/daemon-ui/MIGRATION.md— 9-step before/after guide for adapter authors.Benefit: lowers cost of bringing a new adapter (channel plugin, IDE extension, dashboard) onto the shared contract from "read 600 LOC of normalizer source" to "run
runAdapterConformanceSuite+ read the cookbook".Daemon-side dependency status (verified against
daemon_mode_b_main@57d04786d)After landing #4360 (daemon protocol completion), 5 of 7 declared dependencies are now satisfied on the wire — meaning the forward-compat code paths in this PR activate automatically once merged:
_meta.serverTimestampenvelope stampingserver.ts:2670(cites issue #19 P0)formatBlockTimestampprovenance+serverIdon tool_callToolCallEmitter.emitStarterrorKindonstream_errorserver.ts:2046DaemonUiErrorEvent.errorKindtypederrorKindonsession_diedreasonfieldreason_meta.parentToolCallId)SubAgentTracker.getSubagentMeta()selectSubagentChildBlockstool.progresseventMessageEmitter.emitUserContent)extractContentPartreadyValidation
Reference adapter conformance:
Remaining (deferred to follow-up PRs, not blockers for this one)
tool.progress— new SSE event type (~50 LOC daemon). SDK state shape already ready.MessageEmitter.emitUserContent(parts)+HistoryReplayerinlineData/fileDatabranches (~80 LOC Core) + reducer wiring (~80 LOC SDK). SDK'sextractContentParthelper already shipped, awaiting Core.Both unblock specific UX features (long-task progress display + image/audio attachment echo); neither blocks this PR's render-contract delivery.
Scope / Risk
daemon_mode_b_main(21 files). All changes are additive to the public API; no existing export removed or renamed.createdAtpreserved as@deprecatedalias forclientReceivedAt.debug.@qwen-code/sdk/daemonsubpath has zero React / zero Node-only deps (asserted inassertBrowserSafeBundle). Web-terminal / web-chat bundles include only the helpers they import (tree-shake friendly).Dependencies
daemon_mode_b_main@57d04786d(post-feat(daemon): add shared UI transcript layer #4328 merge, post-feat(serve+sdk): F4 prereq — daemon protocol completion (serverTimestamp / provenance / errorKind / state_resync_required) #4360 F4 prereq, post-perf(core): F2 cleanup PR A — R9/W11/W12/R10 (post-merge follow-ups) #4411 F2 cleanup, post-refactor(acp-bridge): F1 test split — lift bridge.test.ts (6861 LOC) to acp-bridge #4445 F1 test split).Linked
feat(daemon): add shared UI transcript layer(base, merged)feat(serve+sdk): F4 prereq — daemon protocol completion(merged; satisfies §C1/§C2/§C3 dependencies)cc @wenshao @doudouOUC
Generated with assistance from Claude Opus 4.7. Full SDK + WebUI typecheck + 153 unit tests pass against the rebased branch HEAD.