feat(daemon): attach skill-toggle mutation metadata to settings_changed - #9051
Conversation
Hosts can apply Skill toggles incrementally without a full task reload or suppressing skills.* events. Co-authored-by: Cursor <cursoragent@cursor.com>
github-actions
Bot
added
the
review/self-reported
|
Thanks for the PR! Template looks good ✓ Problem: this is a real, observed integration constraint, not a theoretical one — issue #9024 documents that hosts (e.g. WebShell) receive Direction: aligned. The issue-stage triage already accepted this direction ("accept for exploration"), and there is precedent — the protocol already carries typed attribution on Size: ~128 production lines + 176 test lines + 10 doc lines, spanning Approach: the scope feels right — the toggle flow already computes Risk: no elevated risk signals — none of the changed files match the high-risk paths from the revert analysis. Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题：这是一个真实观测到的集成限制，不是假想问题——issue #9024 记录了宿主（如 WebShell）收到的 方向：一致。issue 阶段分诊已接受此方向（"接受探索"），协议中也有先例—— 规模：跨 方案：范围合理——切换流程内部已计算出 风险：无升级风险信号——改动文件均未命中 revert 分析中的高风险路径。 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewThe implementation is clean and matches the direction accepted on #9024. I verified the emit flows against the base code rather than just the diff:
No correctness or convention problems found in the code itself. The blocker is CI. Test evidence — the PR's own CI⛔ Per the review rules I do not run PR code; this is the PR's CI read through the API at the reviewed commit. One root cause explains most of the red: the SDK build enforces a browser bundle budget — The one failure that is not this PR's fault: Note on pinning: once the budget issue is resolved, this PR's own tests do pin the behavior — the updated facade assertions require the exact
To unblock: bring the daemon browser bundle back under the 190464-byte budget — the parser and type surface added ~512 bytes, so either tighten the normalizer implementation (e.g. a more compact parse path) or, if the maintainers agree the budget should grow for this feature, raise 中文说明代码审查实现干净，与 #9024 接受的方向一致。我对照基线代码核实了发出流程，而不只是看 diff：
代码本身没有正确性或约定问题。阻塞项是 CI。 测试证据 —— 来自 PR 自身的 CI⛔ 按审查规则不运行 PR 代码；以上是通过 API 读取的该提交自己的 CI 结果。 大部分红色来自同一个根因：SDK 构建有浏览器 bundle 体积预算—— 唯一不是本 PR 造成的失败： 关于测试钉住能力：预算问题解决后，本 PR 自己的测试确实能钉住行为——更新后的 facade 断言要求完整的 解锁方式：把 daemon 浏览器 bundle 拉回 190464 字节预算之内——新增解析器与类型面约 512 字节，要么精简 normalizer 实现（例如更紧凑的解析路径），要么在维护者同意为该功能放宽预算时、经其确认后上调 — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 2/5 — the design and the code are genuinely good, but the PR cannot merge as it stands: the SDK build fails its bundle-size budget in every lane that compiles it, and the new tests have never actually run. Stepping back: my independent proposal for #9024 was exactly what this PR does — compute one mutation descriptor per toggle request after the session refresh, attach it to every resulting The problem is the one thing the description's "unit tests only" framing can't cover: shipping ~512 bytes of new SDK surface puts the daemon browser bundle at 190976 bytes against a 190464-byte budget, so So: request changes, solely on the bundle-budget failure. Direction, scope, implementation, and test coverage are otherwise ready — once the budget is settled (trim the additions, or raise the cap with a maintainer's sign-off) and CI goes green, this should sail through on re-run. @samuelhsin — details and the exact error are in the Stage 2 comment above. 中文说明信心度：2/5 —— 设计与代码确实不错，但当前状态无法合并：SDK 构建在每个编译它的通道上都超出 bundle 体积预算，新测试实际上从未运行过。 退一步看：我对 #9024 的独立方案与本 PR 完全一致——每次切换请求在会话刷新后计算一个 mutation 描述符，附加到每条由此产生的 问题在于 PR 描述里"仅单元测试"的说法覆盖不到的一点：新增约 512 字节的 SDK 面使 daemon 浏览器 bundle 达到 190976 字节，而预算是 190464 字节，于是 因此：仅因 bundle 预算失败而请求修改。方向、范围、实现与测试覆盖在其他方面都已就绪——预算问题解决后（精简新增体积，或经维护者确认后上调上限）、CI 变绿，re-run 时应该能顺利通过。 @samuelhsin —— 细节与确切报错见上方 Stage 2 评论。 — Qwen Code · qwen3.8-max Reviewed at |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Requesting changes on one specific blocker: the SDK build's bundle-size gate fails on this commit — Browser daemon SDK bundle is 190976 bytes; expected <= 190464 — so the build dies before any test runs, in every lane that compiles the SDK. The new unit tests (which otherwise pin the payload shape well) have never executed.
Fix path: trim ~512 bytes of daemon-browser-bundle surface, or raise MAX_DAEMON_BROWSER_BUNDLE_BYTES with maintainer sign-off. Direction, implementation, and coverage look good — see the Stage 2/3 comments for the full review. 🙏
— Qwen Code · qwen3.8-max
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not explored to full depth (tool budget reached): This PR adds backward-compatible mutation metadata (kind:...: none — did not run the facade/SDK test suites locally (CI evidence assumed for the updated tests); all findings above rest on source reads, not test execution.; This PR adds backward-compatible mutation metadata (kind:...: none — all checks I started were completed (~16 tool calls)..
Test Plan (not a blocker): src/serve/workspace-service/__tests__/facade.test.ts — no such file or directory; test/unit/daemonUi.test.ts — no such file or directory.
中文说明
未探索到全部深度（达到工具调用预算）：This PR adds backward-compatible mutation metadata (kind:...：none — did not run the facade/SDK test suites locally (CI evidence assumed for the updated tests); all findings above rest on source reads, not test execution.；This PR adds backward-compatible mutation metadata (kind:...：none — all checks I started were completed (~16 tool calls).。
Test Plan（非阻断）：src/serve/workspace-service/__tests__/facade.test.ts — no such file or directory; test/unit/daemonUi.test.ts — no such file or directory。
— qwen3.8-max via Qwen Code /review (v0.21.10)
The new normalizer parser pushed the browser daemon bundle over the 186KB cap. Raise it to 187KB and pin the review gaps that were cheap to close. Co-authored-by: Cursor <cursoragent@cursor.com>
🩺 serve daemon A/BBuilt the PR base vs this PR head ✅ No response changes against the PR base across 4 scenario(s). — Qwen Code · serve A/B |
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. |
Co-authored-by: Cursor <cursoragent@cursor.com>
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Test Plan (not a blocker): src/serve/workspace-service/__tests__/facade.test.ts — no such file or directory; test/unit/daemonUi.test.ts — no such file or directory.
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
Test Plan（非阻断）：src/serve/workspace-service/__tests__/facade.test.ts — no such file or directory; test/unit/daemonUi.test.ts — no such file or directory。
— qwen3.8-max via Qwen Code /review (v0.21.11)
Co-authored-by: Cursor <cursoragent@cursor.com>
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not explored to full depth (tool budget reached): "PR #9051 adds backward-compatible mutation metadata to…": verifying the actual daemon browser bundle size against the raised 190KB cap — packages/sdk-typescript/scripts/build.js fails in this review worktree because …; "PR #9051 adds backward-compatible mutation metadata to…": none — all checks above completed within budget..
Test Plan (not a blocker): src/serve/workspace-service/__tests__/facade.test.ts — no such file or directory; test/unit/daemonUi.test.ts — no such file or directory.
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）："PR #9051 adds backward-compatible mutation metadata to…"：verifying the actual daemon browser bundle size against the raised 190KB cap — packages/sdk-typescript/scripts/build.js fails in this review worktree because …；"PR #9051 adds backward-compatible mutation metadata to…"：none — all checks above completed within budget.。
Test Plan（非阻断）：src/serve/workspace-service/__tests__/facade.test.ts — no such file or directory; test/unit/daemonUi.test.ts — no such file or directory。
— qwen3.8-max via Qwen Code /review (v0.21.11)
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
Test Plan (not a blocker): src/serve/workspace-service/__tests__/facade.test.ts — no such file or directory; test/unit/daemonUi.test.ts — no such file or directory.
中文说明
已审查。 建议见行内评论。
Test Plan（非阻断）：src/serve/workspace-service/__tests__/facade.test.ts — no such file or directory; test/unit/daemonUi.test.ts — no such file or directory。
— deepseek-v4-flash via Qwen Code /review (v0.21.10)
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not explored to full depth (tool budget reached): "PR #9051 adds backward-compatible mutation metadata to…": did not run the SDK browser build to confirm the actual bundle sits under the new 190KB cap (the build script enforces it in CI, so an overshoot fails the build…; "PR #9051 adds backward-compatible mutation metadata to…": SDK browser-bundle size gate ( scripts/build.js , 189→190KB cap) — could not execute locally; the worktree's @qwen-code/qwen-code-core has no built dist/ , s…; "You are review agent reverse-audit — Reverse audit agent…": none — all checks above were completed within budget.; "You are review agent reverse-audit — Reverse audit agent…": none — all checks I started were completed within budget..
Test Plan (not a blocker): src/serve/workspace-service/__tests__/facade.test.ts — no such file or directory; test/unit/daemonUi.test.ts — no such file or directory.
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）："PR #9051 adds backward-compatible mutation metadata to…"：did not run the SDK browser build to confirm the actual bundle sits under the new 190KB cap (the build script enforces it in CI, so an overshoot fails the build…；"PR #9051 adds backward-compatible mutation metadata to…"：SDK browser-bundle size gate ( scripts/build.js , 189→190KB cap) — could not execute locally; the worktree's @qwen-code/qwen-code-core has no built dist/ , s…；"You are review agent reverse-audit — Reverse audit agent…"：none — all checks above were completed within budget.；"You are review agent reverse-audit — Reverse audit agent…"：none — all checks I started were completed within budget.。
Test Plan（非阻断）：src/serve/workspace-service/__tests__/facade.test.ts — no such file or directory; test/unit/daemonUi.test.ts — no such file or directory。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| // Bumped from 189KB to 190KB for skill-toggle mutation metadata on | ||
| // settings_changed in the UI normalizer. | ||
| const MAX_DAEMON_BROWSER_BUNDLE_BYTES = 190 * 1024; |
There was a problem hiding this comment.
[Suggestion] The 189→190 KB MAX_DAEMON_BROWSER_BUNDLE_BYTES bump is not justified by the measured bundle: rebuilding the daemon browser bundle from this commit with the pinned esbuild config yields 193,129 bytes, which fits under the old 189 KiB cap (193,536) with 407 bytes to spare, so assertBrowserSafeBundle would pass without the bump. — Failure scenario: the size-budget ratchet gives away 1,024 bytes of headroom it did not need to, and this file's meticulously maintained bump-by-bump comment history records a necessity claim that does not reproduce, making future budget accounting harder to trust.
| // Bumped from 189KB to 190KB for skill-toggle mutation metadata on | |
| // settings_changed in the UI normalizer. | |
| const MAX_DAEMON_BROWSER_BUNDLE_BYTES = 190 * 1024; | |
| const MAX_DAEMON_BROWSER_BUNDLE_BYTES = 189 * 1024; |
中文说明
[建议] 189→190 KB 的 MAX_DAEMON_BROWSER_BUNDLE_BYTES 上调缺乏实测依据：用锁定的 esbuild 配置从本提交重新构建浏览器 daemon bundle，实测为 193,129 字节，仍低于旧的 189 KiB 上限（193,536），余量 407 字节，即使不上调 assertBrowserSafeBundle 也能通过。——失败场景：体积预算棘轮白白让出了本不需要的 1,024 字节余量，且本文件逐次精心维护的上调注释史会记录一条无法复现的必要性声明，使未来的预算核算更难采信。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| function isSettingsChangedData( | ||
| value: unknown, | ||
| ): value is DaemonSettingsChangedData { | ||
| return isRecord(value) && isNonEmptyString(value['key']); |
There was a problem hiding this comment.
[Suggestion] isSettingsChangedData narrows to DaemonSettingsChangedData — which declares scope?: string and mutation?: DaemonSkillToggleMutation — but validates neither optional field, so the type promise is stronger than the runtime check. Sibling guards in this file validate their declared optional fields (isAuthDeviceFlowFailedData checks hint, isTrustChangeRequestedData checks reason), and this same PR writes the strict 40-line parseSkillToggleMutation for the identical shape on the normalizer path. Probe-verified at this commit: { key: 'skills.disabled', scope: 42, mutation: 'corrupt' } passes the guard. — Failure scenario: an SDK consumer following the documented raw-event pattern (asKnownDaemonEvent narrowing) gets past the guard, then reads event.data.scope / event.data.mutation per the declared types and throws a TypeError far from the non-conforming emitter. No in-repo emitter can produce such a payload today, so this is defensive hardening, not a live bug.
| return isRecord(value) && isNonEmptyString(value['key']); | |
| return ( | |
| isRecord(value) && | |
| isNonEmptyString(value['key']) && | |
| (value['scope'] === undefined || typeof value['scope'] === 'string') && | |
| (value['mutation'] === undefined || isRecord(value['mutation'])) | |
| ); |
中文说明
[建议] isSettingsChangedData 收窄到 DaemonSettingsChangedData——该类型声明了 scope?: string 和 mutation?: DaemonSkillToggleMutation——但两个可选字段都未校验，类型承诺强于运行时检查。同文件中的兄弟守卫都会校验其声明的可选字段（isAuthDeviceFlowFailedData 检查 hint，isTrustChangeRequestedData 检查 reason），而且本 PR 自己在 normalizer 路径上为完全相同的形状写了 40 行的严格校验器 parseSkillToggleMutation。已在本提交上探针验证：{ key: 'skills.disabled', scope: 42, mutation: 'corrupt' } 能通过该守卫。——失败场景：遵循文档中原始事件模式（asKnownDaemonEvent 收窄）的 SDK 使用者会通过守卫，随后按声明类型读取 event.data.scope / event.data.mutation，在远离不合规范发出方的位置抛出 TypeError。当前仓库内没有任何发出方能产生这种载荷，因此这是防御性加固，不是现存 bug。
— qwen3.8-max via Qwen Code /review (v0.21.11)
Code review (static, verified against a PR-head worktree)Overall a tight, well-scoped change: additive metadata, careful backward compatibility, and solid test coverage of the parser edges. I verified the key assumptions against the codebase rather than taking the description at face value. No blockers from my side. CriticalNone. Warning
Suggestions
Verified in the worktree (not just assumed)
Nice work — the defensive normalizer design (drop the malformed |
ytahdn
left a comment
ytahdn
left a comment
There was a problem hiding this comment.
Approving after a static review against the PR head.
- Verified all nine
settings_changedemitters (current and since #4490) includekey, so the stricterasKnownDaemonEventguard is backward-safe. Noted in my comment as worth a description mention, not a blocker. - Emission paths are correctly gated: single toggle inside
persisted.changed, batch onchangedwithmutation.skillsfiltered byresult.changed— metadata is accurate and never empty at emit time. - Normalizer is properly defensive: malformed
mutationis dropped while the event is still delivered; parser assumptions matchnumberField/stringFieldsemantics and are covered by the malformed-input tests. - The earlier bundle-size blocker is resolved by the 190KB budget bump; CI is fully green on the latest head.
Remaining nits (stale commit message, shared mutation object reference, missing-scope test case) are tracked in my comment and can be addressed in a follow-up or ignored.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Test Plan (not a blocker): src/serve/workspace-service/__tests__/facade.test.ts — no such file or directory; test/unit/daemonUi.test.ts — no such file or directory.
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
Test Plan（非阻断）：src/serve/workspace-service/__tests__/facade.test.ts — no such file or directory; test/unit/daemonUi.test.ts — no such file or directory。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| function isSettingsChangedData( | ||
| value: unknown, | ||
| ): value is DaemonSettingsChangedData { | ||
| return isRecord(value) && isNonEmptyString(value['key']); | ||
| } |
There was a problem hiding this comment.
[Suggestion] R4-2: isSettingsChangedData narrows to DaemonSettingsChangedData — which declares scope?: string and mutation?: DaemonSkillToggleMutation — but validates neither optional field, so the type promise is stronger than the runtime check. Sibling guards validate their declared optional fields (isAuthDeviceFlowFailedData checks hint, isTrustChangeRequestedData checks reason), and this same PR writes the strict parseSkillToggleMutation for the identical shape on the normalizer path — recognition and normalization disagree about what the same payload guarantees. Still stands from round 4; probe re-verified at this commit: { key: 'skills.disabled', scope: 42, mutation: 'corrupt' } and a mutation missing skills both pass the guard, and the strengthening below rejects both while valid payloads stay accepted (110 daemonEvents tests green with it). — Failure scenario: a host consuming raw events through asKnownDaemonEvent (the documented schema layer this PR extends) trusts the declared type after recognition passes; a version-skewed or nonconforming emitter sending { key: 'skills.disabled', mutation: { kind: 'skill_toggle', id: 'x' } } or { key: 'x', scope: 42 } passes the guard, and event.data.mutation.skills.map(...) throws a TypeError far from the emitter. No in-repo emitter produces such a payload today — defensive hardening at a newly public schema boundary, not a live bug.
| function isSettingsChangedData( | |
| value: unknown, | |
| ): value is DaemonSettingsChangedData { | |
| return isRecord(value) && isNonEmptyString(value['key']); | |
| } | |
| function isSettingsChangedData( | |
| value: unknown, | |
| ): value is DaemonSettingsChangedData { | |
| return ( | |
| isRecord(value) && | |
| isNonEmptyString(value['key']) && | |
| (value['scope'] === undefined || typeof value['scope'] === 'string') && | |
| (value['mutation'] === undefined || isRecord(value['mutation'])) | |
| ); | |
| } |
Note: this minimal form closes the scope: 42 / mutation: 'corrupt' half; a mutation object missing skills still passes it. Closing that half too means reusing parseSkillToggleMutation's validation core here, or typing the unvalidated fields as unknown on DaemonSettingsChangedData.
中文说明
[建议] R4-2：isSettingsChangedData 收窄到 DaemonSettingsChangedData——该类型声明了 scope?: string 和 mutation?: DaemonSkillToggleMutation——但两个可选字段都未校验，类型承诺强于运行时检查。同文件中的兄弟守卫都会校验其声明的可选字段（isAuthDeviceFlowFailedData 检查 hint，isTrustChangeRequestedData 检查 reason），而且本 PR 自己在 normalizer 路径上为完全相同的形状写了严格校验器 parseSkillToggleMutation——识别层与归一化层对同一载荷的保证并不一致。该发现自第 4 轮起仍然存在；已在本提交上重新探针验证：{ key: 'skills.disabled', scope: 42, mutation: 'corrupt' } 以及缺少 skills 的 mutation 均能通过该守卫，而下面的加固能拒绝这两类载荷且合法载荷仍被接受（daemonEvents 110 个测试在其下保持绿色）。——失败场景：遵循文档中原始事件模式（asKnownDaemonEvent 收窄，即本 PR 扩展的公开 schema 层）的宿主在通过识别后会信任声明的类型；版本不一致或不合规范的发出方发送 { key: 'skills.disabled', mutation: { kind: 'skill_toggle', id: 'x' } } 或 { key: 'x', scope: 42 } 时能通过守卫，随后 event.data.mutation.skills.map(...) 会在远离发出方的位置抛出 TypeError。当前仓库内没有任何发出方能产生这种载荷——这是新公开 schema 边界上的防御性加固，不是现存 bug。
注：上面的最小形式关闭 scope: 42 / mutation: 'corrupt' 一类；缺少 skills 的 mutation 对象仍会通过。若也要关闭这一类，可在此复用 parseSkillToggleMutation 的校验核心，或将 DaemonSettingsChangedData 上未校验的字段类型改为 unknown。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| expect(firstMutation?.id).toEqual(expect.any(String)); | ||
| expect(firstMutation?.id).toBe(secondMutation?.id); |
There was a problem hiding this comment.
[Suggestion] The mutation-id correlation contract is only half-tested: same-id within one request is pinned here, but distinct ids across separate toggle requests are not — a one-line mutation id: randomUUID() → id: 'constant' in createSkillToggleMutation survives the entire suite. Probe-verified: the mutant passes 112/112, and a distinctness assertion catches it while staying green against the real code. — Failure scenario: the docs this PR adds state id correlates every settings event produced by one toggle request, which only has meaning if different requests get different ids. If a future refactor replaces randomUUID() with a deterministic id (constant, counter, workspace hash), every test stays green, and a host that groups buffered settings_changed events by mutation.id — the exact consumer this metadata was built for (#9024) — merges events from two unrelated toggles into one mutation and applies them as a single incremental change.
Suggested fix — perform a second, separate toggle call in a facade test and assert its mutation id differs from the first request's:
expect(secondRequestMutation?.id).not.toBe(firstMutation?.id);中文说明
[建议] mutation id 关联契约只被测试了一半：同一请求内共享 id 在此处被钉住，但不同请求之间 id 必须不同这一点没有任何测试覆盖——把 createSkillToggleMutation 中的 id: randomUUID() 一行突变为 id: 'constant' 后整个套件仍然全部通过。已探针验证：该突变体以 112/112 通过，而补上差异性断言即可捕获它，且该断言对真实代码保持绿色。——失败场景：本 PR 新增的文档写明 id 用于关联一次切换请求产生的所有 settings 事件，这只有在不同请求获得不同 id 时才有意义。如果未来某次重构把 randomUUID() 换成确定性 id（常量、计数器、workspace 哈希），所有测试仍为绿色，而按 mutation.id 归组缓冲的 settings_changed 事件的宿主——正是这份元数据为之而生的消费者（#9024）——会把两次互不相关的切换的事件合并进同一个 mutation，并作为一次增量变更应用。
建议修复——在 facade 测试中再发起一次独立的切换调用，并断言其 mutation id 与第一次请求的不同：
expect(secondRequestMutation?.id).not.toBe(firstMutation?.id);— qwen3.8-max via Qwen Code /review (v0.21.11)
| const mutation = createSkillToggleMutation({ | ||
| skills: [{ name: skill.name, enabled }], | ||
| activation, |
There was a problem hiding this comment.
[Suggestion] The single-toggle path's multi-event emission shape is untested: enabling a default-disabled skill that sits in skills.disabled makes the real persist fn (updateWorkspaceSkillSettingLists, config/skill-settings.ts:127-134) return TWO settingsChanges entries (skills.disabled AND skills.enabled), so this loop publishes two settings_changed events sharing one mutation id — but every single-toggle test mocks persistDisabledSkills with zero or one entry, so the intra-request shared-id contract is pinned only on the batch path. Probe-verified: moving this createSkillToggleMutation call inside the loop survives 112/112, while the identical refactor on the batch path fails the existing shared-id assertion in facade.test.ts. — Failure scenario: a refactor that regenerates the mutation id per change inside the single-toggle loop breaks the documented contract both doc pages this PR updates ('every skills.disabled / skills.enabled event from the same request shares one mutation id'), while the suite stays green.
Suggested fix — add a single-toggle test whose persistDisabledSkills mock returns both entries, assert two published events, and assert they share one id:
settingsChanges: [
{ key: 'skills.disabled', value: undefined },
{ key: 'skills.enabled', value: ['review'] },
],
// ...
expect(firstMutation?.id).toBe(secondMutation?.id);中文说明
[建议] 单个切换路径的多事件发出形状没有测试覆盖：启用一个位于 skills.disabled 中的默认禁用 Skill 时，真实持久化函数（updateWorkspaceSkillSettingLists，config/skill-settings.ts:127-134）会返回两条 settingsChanges（skills.disabled 和 skills.enabled），因此这个循环会发布两条共享同一个 mutation id 的 settings_changed 事件——但所有单个切换的测试都把 persistDisabledSkills mock 成零条或一条，请求内共享 id 的契约只在批量路径上被钉住。已探针验证：把这个 createSkillToggleMutation 调用移进循环内后套件仍以 112/112 通过，而同样的重构放在批量路径上会使 facade.test.ts 中现有的共享 id 断言失败。——失败场景：在单个切换循环内按变更为每个事件重新生成 mutation id 的重构，会破坏本 PR 更新的两份文档都写明的契约（同一次请求的每条 skills.disabled / skills.enabled 事件共享同一个 mutation id），而测试套件仍全部为绿色。
建议修复——新增一个单个切换测试，让 persistDisabledSkills mock 返回上述两条 settingsChanges，断言发布了两个事件，并断言它们共享同一个 id：
settingsChanges: [
{ key: 'skills.disabled', value: undefined },
{ key: 'skills.enabled', value: ['review'] },
],
// ...
expect(firstMutation?.id).toBe(secondMutation?.id);— qwen3.8-max via Qwen Code /review (v0.21.11)
# Conflicts: # packages/cli/src/serve/workspace-service/__tests__/facade.test.ts
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
Test Plan (not a blocker): src/serve/workspace-service/__tests__/facade.test.ts — no such file or directory; test/unit/daemonUi.test.ts — no such file or directory.
中文说明
已审查——无阻断问题。 建议见行内评论。
Test Plan（非阻断）：src/serve/workspace-service/__tests__/facade.test.ts — no such file or directory; test/unit/daemonUi.test.ts — no such file or directory。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| publishWorkspaceEvent({ | ||
| type: 'settings_changed', | ||
| data: { ...change, scope: 'workspace' }, | ||
| data: { ...change, scope: 'workspace', mutation }, |
There was a problem hiding this comment.
[Suggestion] The skill-toggle settings_changed publish loop exists in two near-duplicate copies — the single-toggle path (~L940 spells out key/value) and this batch path (spreads ...change) — both edited by this diff. The protocol docs this PR updates promise both routes emit the same wire object with one shared mutation.id per request. — Failure scenario: any future payload change (adding/renaming a field on the event data) must be applied in both loops; missing one makes single-toggle and batch events diverge, breaking hosts that correlate events by mutation.id across the two routes. This PR itself performed the identical edit twice.
Suggested fix: extract a small local publish helper inside createDaemonWorkspaceService (e.g. publishSkillToggleSettingsChanged(changes, mutation, ctx)) and call it from both paths with the same { ...change, scope: 'workspace', mutation } construction, so the payload shape has a single home.
中文说明
Skill 切换的 settings_changed 发布循环存在两份近乎重复的副本——单个切换路径（约 L940，显式写出 key/value）和本批量路径（展开 ...change），本 diff 同时修改了这两处。本 PR 更新的协议文档承诺两条路由发出相同的线上对象、且每次请求共享同一个 mutation.id。——失败场景：未来任何载荷变更（为事件数据新增/重命名字段）都必须在两个循环中各改一次；漏改其一会使单个切换与批量切换的事件产生分歧，破坏依赖 mutation.id 在两条路由间统一关联事件的宿主。本 PR 自己就把同一个编辑做了两遍。
建议修复：在 createDaemonWorkspaceService 内部提取一个小的本地发布辅助函数（例如 publishSkillToggleSettingsChanged(changes, mutation, ctx)），两条路径都以相同的 { ...change, scope: 'workspace', mutation } 构造调用它，让载荷形状只有一个出处。
— qwen3.8-max via Qwen Code /review (v0.21.12)
Follow-up: R9-1 daemon browser bundle cap
Remaining open items are Suggestions and stay deferred. |
The 190KB cap overflowed by 491 bytes after merging main, so the SDK build fails before tests run. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not explored to full depth (tool budget reached): "agent reverse-audit (round 4)": none — no Budget gap: lines, as nothing was truncated by the tool ceiling (~15 of ~52 calls used)..
Test Plan (not a blocker): src/serve/workspace-service/__tests__/facade.test.ts — no such file or directory; test/unit/daemonUi.test.ts — no such file or directory.
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）："agent reverse-audit (round 4)"：none — no Budget gap: lines, as nothing was truncated by the tool ceiling (~15 of ~52 calls used).。
Test Plan（非阻断）：src/serve/workspace-service/__tests__/facade.test.ts — no such file or directory; test/unit/daemonUi.test.ts — no such file or directory。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| (activation === 'applied' || | ||
| activation === 'deferred' || | ||
| activation === 'partial') && |
There was a problem hiding this comment.
[Suggestion] R11-1: isDaemonSkillToggleMutation's activation === 'deferred' / activation === 'partial' acceptance branches are exercised by no SDK test — only 'applied' (valid) and 'soon' (invalid) pass through asKnownDaemonEvent / the normalizer, even though the daemon provably emits all three values (facade.test.ts pins 'deferred' and 'partial' emissions on both toggle paths). — Failure scenario: a future edit dropping either disjunct leaves the entire sdk-typescript suite green, yet every legitimate partial/deferred skill-toggle event then fails asKnownDaemonEvent and is reclassified into unrecognizedKnownEventCount — hosts relying on the guard silently lose the refresh-failure metadata this PR adds.
Witness (mutation probe):
BASE (unmodified): probe 6/6 pass
MUTATION ('partial' disjunct dropped): probe flips — "expected undefined not to be undefined", "expected 1 to be +0"
existing suite with mutation in place: Tests 405 passed (405) ← regression invisible
Suggested fix — parametrize the valid-mutation case over all three activations (here or in daemonUi.test.ts):
for (const activation of ['applied', 'deferred', 'partial'] as const) {
expect(
asKnownDaemonEvent({
id: 1, v: 1, type: 'settings_changed',
data: { key: 'skills.disabled', mutation: { ...validMutation, activation } },
}),
).toBeDefined();
}中文说明
[Suggestion] R11-1：isDaemonSkillToggleMutation 中 activation === 'deferred' / activation === 'partial' 两个接受分支没有任何 SDK 测试覆盖——目前只有 'applied'（合法）与 'soon'（非法）会经过 asKnownDaemonEvent / normalizer，而 daemon 侧确实会发出全部三种取值（facade.test.ts 在单切与批量两条路径上都钉住了 'deferred' 和 'partial' 的发出）。——失败场景：未来若删除其中任一分支（例如去掉 activation === 'partial' ||），整个 sdk-typescript 测试套件仍然全绿，但所有合法的 partial/deferred skill-toggle 事件都会在 asKnownDaemonEvent 处被拒绝、被归入 unrecognizedKnownEventCount——依赖该 guard 的宿主会静默丢失本 PR 新增的刷新失败元数据。
证据（变异探针）：基线探针 6/6 通过；删除 'partial' 分支后探针翻转（"expected undefined not to be undefined"、"expected 1 to be +0"），而既有套件在变异下仍 405/405 全绿——回归不可见。
建议修复：将合法 mutation 用例参数化到全部三种 activation（在本文件或 daemonUi.test.ts 中）。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| type DaemonSkillToggleMutation, | ||
| type DaemonSkillToggleMutationSkill, |
There was a problem hiding this comment.
[Suggestion] R11-2: The four new public type exports (DaemonSettingsChangedData, DaemonSettingsChangedEvent, DaemonSkillToggleMutation, DaemonSkillToggleMutationSkill) are pinned by no compile fence on this published barrel — nothing in-repo imports them through it, and test/unit/daemon-public-surface.test.ts (the designated fence for exactly this drift) does not list them. Only the outer barrel is unpinned: inner-barrel (src/daemon/index.ts) drops are caught by tsc via these named re-exports. — Failure scenario: a future refactor dropping one of these re-export lines leaves tsc --noEmit and the whole vitest suite green, and external @qwen-code/sdk consumers typing against the new envelope/mutation types break at compile time only on their side.
Witness (mutation probe):
drop all four re-exports from src/index.ts:
tsc --noEmit → exit 0 (green)
vitest (full suite) → Test Files 34 passed (34) / Tests 1574 passed (1574)
inner-barrel drops → error TS2724 (that half IS gated)
fix-probe: adding the four names to the surface test alone flips no gate
(package tsconfig excludes test/; esbuild erases type-only imports)
Suggested fix — add the four names to the surface test's import type {...} from '../../src/index.js' list, AND make the fence bind: include test/ in a typecheck tsconfig pass or enable vitest typecheck — the import alone flips no gate today (probe-verified).
中文说明
[Suggestion] R11-2：新增的四个公共类型导出（DaemonSettingsChangedData、DaemonSettingsChangedEvent、DaemonSkillToggleMutation、DaemonSkillToggleMutationSkill）在这个发布 barrel 上没有任何编译围栏钉住——仓库内没有任何代码通过该 barrel 导入它们，专门防范此类漂移的 test/unit/daemon-public-surface.test.ts 也没有列出它们。未设防的只是外层 barrel：删除内层 barrel（src/daemon/index.ts）的导出会被 tsc 通过这些具名 re-export 捕获。——失败场景：未来重构若删除其中一行 re-export，tsc --noEmit 与整个 vitest 套件仍会全绿，只有依赖新 envelope/mutation 类型的外部 @qwen-code/sdk 使用者会在自己一侧编译失败。
证据（变异探针）：从 src/index.ts 删除全部四个 re-export 后，tsc --noEmit 退出码 0，vitest 全套件 34 文件 / 1574 用例全绿；仅把四个名字加入 surface test 不会触发任何门禁（包 tsconfig 排除了 test/，esbuild 会擦除纯类型导入）。
建议修复：将这四个名字加入 surface test 的 import type {...} from '../../src/index.js' 列表，并让围栏真正生效——将 test/ 纳入 typecheck，或启用 vitest typecheck（已用探针验证：仅加 import 在变异状态下仍然全绿）。
— qwen3.8-max via Qwen Code /review (v0.21.12)
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: build-and-test — packages/web-shell and packages/webui test suites did not run (Agent 7 whole-call budget exhausted before them).
Test Plan (not a blocker): src/serve/workspace-service/__tests__/facade.test.ts — no such file or directory; test/unit/daemonUi.test.ts — no such file or directory.
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查：build-and-test — packages/web-shell and packages/webui test suites did not run (Agent 7 whole-call budget exhausted before them)。
Test Plan（非阻断）：src/serve/workspace-service/__tests__/facade.test.ts — no such file or directory; test/unit/daemonUi.test.ts — no such file or directory。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| Array.isArray(skills) && | ||
| skills.length > 0 && | ||
| skills.every( |
There was a problem hiding this comment.
[Suggestion] The per-element skills.every(...) validation in isDaemonSkillToggleMutation is exercised only with single-element arrays in both SDK test files, so the one-token mutant skills.every( → skills.some( survives this diff's entire SDK suite — probe-verified: baseline Tests 405 passed (405); mutant at events.ts:3019 Tests 405 passed (405) (identical — mutant survives); a probe feeding skills: [{ name: 'web-search', enabled: true }, { name: '', enabled: 'yes' }] through asKnownDaemonEvent gives 1 failed | 1 passed against the mutant and 2 passed against the correct code (probe flips; mutant reverted, tree restored). — Failure scenario: a multi-element skills array with a later invalid entry — realistic, since the CLI genuinely emits multi-element arrays (the facade tests pin a 3-skill batch) — is accepted under the mutant, letting a mutation with entries violating DaemonSkillToggleMutationSkill flow through asKnownDaemonEvent and the normalizer into typed consumers; the regression ships green.
// add to the daemonUi.test.ts malformed list:
{
...validMutation,
skills: [
{ name: 'web-search', enabled: true },
{ name: '', enabled: true },
],
},
// plus one valid two-element `skills` accept case in daemonEvents.test.ts中文说明
isDaemonSkillToggleMutation 的逐元素校验 skills.every(...) 在两个 SDK 测试文件中只被单元素数组覆盖，因此单 token 变异 skills.every( → skills.some( 能在本 diff 的整个 SDK 测试套件中存活——已用探针实测：基线 Tests 405 passed (405)；events.ts:3019 处的变异体同样 Tests 405 passed (405)（结果完全一致，变异体存活）；将 skills: [{ name: 'web-search', enabled: true }, { name: '', enabled: 'yes' }] 喂给 asKnownDaemonEvent 的探针，对变异体 1 failed | 1 passed，对正确代码 2 passed（探针两侧翻转；变异体已还原，工作区已恢复）。——失败场景：一个后段元素非法的多元素 skills 数组——CLI 确实会发出多元素数组（facade 测试钉住了 3 个 Skill 的批量切换），因此该输入是真实的——在变异体下会被接受，使含有违反 DaemonSkillToggleMutationSkill 条目的 mutation 经 asKnownDaemonEvent 与 normalizer 流入类型化消费者；回归将在全绿中上线。
建议修复：在 daemonUi.test.ts 的 malformed 列表中补一条上面的畸形用例（期望 mutation 被丢弃），并在 daemonEvents.test.ts 补一条合法的双元素 skills 接受用例。
— qwen3.8-max via Qwen Code /review (v0.21.12)
| case 'settings_changed': | ||
| return event.data != null && typeof event.data === 'object' | ||
| ? (event as DaemonEventEnvelope< | ||
| 'settings_changed', | ||
| Record<string, unknown> | ||
| >) | ||
| return isSettingsChangedData(event.data) | ||
| ? (event as DaemonSettingsChangedEvent) |
There was a problem hiding this comment.
[Suggestion] No test crosses the new CLI-emit ↔ SDK-validate boundary: facade.test.ts asserts the emitted payload against its own local skillToggleSettingsChanged helper, and the SDK tests use hand-written payloads — nothing pipes a CLI-produced shape through isSettingsChangedData/normalizeDaemonEvent. Verified at this commit: zero settings_changed coverage in integration-tests/, zero @qwen-code/sdk imports in packages/cli/src/serve/workspace-service/__tests__/, zero CLI-package references in the SDK test tree. — Failure scenario: the wire shape drifts one-sidedly (e.g. sessionsRefreshed renamed on the emit side only) → every unit test in both packages stays green, but in production isSettingsChangedData rejects every skill-toggle payload, asKnownDaemonEvent returns undefined, and reducer-path SDK hosts silently drop every skill-toggle settings_changed event with no error anywhere.
Suggested fix: add a contract test that pipes the exact payload shape asserted in facade.test.ts through isSettingsChangedData/normalizeDaemonEvent, or an integration test driving the skill-toggle routes against a live daemon and asserting the subscribed event normalizes with mutation (shared mutation.id across both keys).
中文说明
没有任何测试穿越新的 CLI 发出 ↔ SDK 校验边界：facade.test.ts 用本地的 skillToggleSettingsChanged 助手断言发出的载荷，SDK 测试使用手写载荷——没有任何测试把 CLI 真实产生的形状喂给 isSettingsChangedData/normalizeDaemonEvent。已在本提交核实：integration-tests/ 中没有任何 settings_changed 覆盖，packages/cli/src/serve/workspace-service/__tests__/ 没有任何 @qwen-code/sdk 导入，SDK 测试树中没有任何对 CLI 包的引用。——失败场景：wire 形状发生单边漂移（例如只在发出侧重命名 sessionsRefreshed）→ 两个包的全部单元测试依旧为绿，但生产环境中 isSettingsChangedData 会拒绝所有 skill-toggle 载荷，asKnownDaemonEvent 返回 undefined，走 reducer 路径的 SDK 宿主会静默丢弃每一条 skill-toggle settings_changed 事件，且任何地方都没有错误。
建议修复：新增一个契约测试，把 facade.test.ts 断言的载荷原样喂给 isSettingsChangedData/normalizeDaemonEvent；或新增一个集成测试，对真实 daemon 调用 skill 切换路由，并断言订阅到的事件能归一化出 mutation（两个 key 共享同一 mutation.id）。
— qwen3.8-max via Qwen Code /review (v0.21.12)
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not explored to full depth (tool budget reached): "agent 4": none — no check was cut short..
Test Plan (not a blocker): src/serve/workspace-service/__tests__/facade.test.ts — no such file or directory; test/unit/daemonUi.test.ts — no such file or directory.
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）："agent 4"：none — no check was cut short.。
Test Plan（非阻断）：src/serve/workspace-service/__tests__/facade.test.ts — no such file or directory; test/unit/daemonUi.test.ts — no such file or directory。
— qwen3.8-max via Qwen Code /review (v0.21.12)
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Test Plan (not a blocker): src/serve/workspace-service/__tests__/facade.test.ts — no such file or directory; test/unit/daemonUi.test.ts — no such file or directory.
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
Test Plan（非阻断）：src/serve/workspace-service/__tests__/facade.test.ts — no such file or directory; test/unit/daemonUi.test.ts — no such file or directory。
— qwen3.8-max via Qwen Code /review (v0.21.12)
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not explored to full depth (tool budget reached): "agent reverse-audit (round 2)": none — no check was cut short..
Test Plan (not a blocker): src/serve/workspace-service/__tests__/facade.test.ts — no such file or directory; test/unit/daemonUi.test.ts — no such file or directory.
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）："agent reverse-audit (round 2)"：none — no check was cut short.。
Test Plan（非阻断）：src/serve/workspace-service/__tests__/facade.test.ts — no such file or directory; test/unit/daemonUi.test.ts — no such file or directory。
— qwen3.8-max via Qwen Code /review (v0.21.12)
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Test Plan (not a blocker): src/serve/workspace-service/__tests__/facade.test.ts — no such file or directory; test/unit/daemonUi.test.ts — no such file or directory.
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
Test Plan（非阻断）：src/serve/workspace-service/__tests__/facade.test.ts — no such file or directory; test/unit/daemonUi.test.ts — no such file or directory。
— qwen3.8-max via Qwen Code /review (v0.21.12)
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅
Test Plan (not a blocker): src/serve/workspace-service/__tests__/facade.test.ts — no such file or directory; test/unit/daemonUi.test.ts — no such file or directory.
中文说明
未发现问题。LGTM！✅
Test Plan（非阻断）：src/serve/workspace-service/__tests__/facade.test.ts — no such file or directory; test/unit/daemonUi.test.ts — no such file or directory。
— qwen3.8-max via Qwen Code /review (v0.21.12)
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅
Test Plan (not a blocker): src/serve/workspace-service/__tests__/facade.test.ts — no such file or directory; test/unit/daemonUi.test.ts — no such file or directory.
中文说明
未发现问题。LGTM！✅
Test Plan（非阻断）：src/serve/workspace-service/__tests__/facade.test.ts — no such file or directory; test/unit/daemonUi.test.ts — no such file or directory。
— qwen3.8-max via Qwen Code /review (v0.21.12)
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅
Test Plan (not a blocker): src/serve/workspace-service/__tests__/facade.test.ts — no such file or directory; test/unit/daemonUi.test.ts — no such file or directory.
中文说明
未发现问题。LGTM！✅
Test Plan（非阻断）：src/serve/workspace-service/__tests__/facade.test.ts — no such file or directory; test/unit/daemonUi.test.ts — no such file or directory。
— qwen3.8-max via Qwen Code /review (v0.21.12)
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
3 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- SDK guard 'deferred'/'partial' activation branches untested (events.ts ~3025) — already reported (comment 3790107913, R11-1)
- settings_changed recognition coupled to optional mutation validity (events.ts ~3040) — already reported (comments 3788996594 / 3789777547, R7-1)
- No CLI-emit ↔ SDK-validate cross-boundary conformance test — already reported (comment 3790224829)
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not explored to full depth (tool budget reached): "agent 6b": none — no check was cut short..
Test Plan (not a blocker): src/serve/workspace-service/__tests__/facade.test.ts — no such file or directory; test/unit/daemonUi.test.ts — no such file or directory.
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 3 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）："agent 6b"：none — no check was cut short.。
Test Plan（非阻断）：src/serve/workspace-service/__tests__/facade.test.ts — no such file or directory; test/unit/daemonUi.test.ts — no such file or directory。
— qwen3.8-max via Qwen Code /review (v0.21.13)
There was a problem hiding this comment.
Reviewed at c131b3f.
No blocking issues found.
I verified the complete workspace-service → daemon event → SDK schema/normalizer path. The mutation is created after the live-session refresh, carries the final activation/counts, is shared by every settings event from one request, and batch payloads include only changed Skills. The change remains additive for existing consumers, and the current normalized-event consumers are unaffected.
Validation:
- CLI facade: 114/114 passed
- SDK daemon events + UI normalizer: 405/405 passed
- SDK build and typecheck passed
- Current Ubuntu Node 22 CI and Web Shell E2E passed
- No unresolved Critical review threads remain
The remaining open threads are Suggestion-level forward-compatibility/test-fence improvements already recorded on the PR; they are non-blocking for this change.
|
Released in v0.21.14. |
5 participants
What this PR does
Adds backward-compatible mutation metadata to Skill-related
settings_changedevents. After a single or batch Skill toggle, each event names the Skills that actually changed, their resulting enabled states, and the live-session refresh outcome. When one request writes bothskills.disabledandskills.enabled, those events share one mutation id. Other settings writes stay as they were.Why it's needed
Skill toggle APIs already refresh active sessions and then emit a generic
settings_changedpayload with only the settings key, complete value, and scope. Hosts such as WebShell cannot tell which Skill changed without diffing cached arrays, so they either reload the whole task or suppressskills.*events. Fixes #9024.Reviewer Test Plan
How to verify
packages/cli:npx vitest run src/serve/workspace-service/__tests__/facade.test.tspackages/sdk-typescript:npx vitest run test/unit/daemonUi.test.tssettings_changedwithmutation.kind: 'skill_toggle'and that Skill's canonical name and enabled state.skills.disabledandskills.enableduses the samemutation.idon both events.mutationobject is omitted by the UI normalizer and the event is still delivered.Evidence (Before & After)
N/A — protocol and SDK typing change. No TUI or WebShell UI change in this PR.
Tested on
Environment (optional)
N/A — unit tests only.
Risk & Scope
mutationis additive and optional. Older consumers ignore it. The UI normalizer drops a malformedmutationinstead of rejecting the wholesettings_changedevent.skill_toggledevent.settings_changedfields are unchanged.Linked Issues
Fixes #9024
中文说明
这个 PR 做了什么
为 Skill 相关的
settings_changed事件追加向后兼容的 mutation 元数据。单次或批量切换 Skill 后，每条事件都会带上实际变更的 Skill 名称、结果启用状态，以及 live session 刷新结果。同一次请求如果同时写入skills.disabled和skills.enabled，这些事件共享同一个 mutation id。其他设置写入保持原样。为什么需要
Skill 切换接口已经会刷新活跃会话，随后发出的通用
settings_changed载荷却只有设置键、完整值和 scope。像 WebShell 这样的宿主无法在不 diff 本地缓存数组的情况下知道哪个 Skill 变了，于是只能整任务重载，或屏蔽skills.*事件。修复 #9024。评审测试计划
如何验证
packages/cli下跑：npx vitest run src/serve/workspace-service/__tests__/facade.test.tspackages/sdk-typescript下跑：npx vitest run test/unit/daemonUi.test.tsmutation.kind: 'skill_toggle'的settings_changed，并包含该 Skill 的规范名称和启用状态。skills.disabled和skills.enabled的批量切换，两条事件使用同一个mutation.id。mutation，但事件本身仍会送达。证据（Before & After）
N/A — 协议与 SDK 类型变更。本 PR 不改 TUI 或 WebShell UI。
测试环境
运行环境（可选）
N/A — 仅单元测试。
风险与范围
mutation是可选的附加字段。旧消费者会忽略它。UI normalizer 在mutation格式错误时会丢掉该字段，而不是整条settings_changed事件。skill_toggled事件。settings_changed字段未改。关联 Issue
Fixes #9024
Made with Cursor