fix(cli): honour the declared enableCacheSharing default in both suggestion gates - #9233
Conversation
…estion gates enableCacheSharing declares default: true in the settings schema, but mergeSettings never applies schema defaults and both runtime gates compared with === true (AppContainer and the ACP Session path), so the cache-aware forked suggestion query was dead code unless the user explicitly set the flag — on prefix-caching servers the default side query then defeats the main session's prefix cache every turn (#9230). Flip both gates to !== false, the exact treatment the adjacent enableFollowupSuggestions gate already uses for the same quirk. Pin the unset=>true and explicit-false=>false behavior in Session.test.ts.
|
✅ Qwen Triage finished — CI landed green on ✅ Qwen Triage 已完成 —— |
|
Thanks for the PR! Template looks good ✓ (all required headings present; the body omits the template's Chinese Problem: observed bug, and I verified it exists in the base code. The schema declares Direction: aligned. This restores the behavior the schema always advertised, and prefix-cache reuse is an area with visible ongoing investment upstream (recent claude-code CHANGELOG entries on reusing cached prefixes for permission checks, staggered sibling agents, and forked subagents). No direct CHANGELOG reference to this flag, but the area is clearly relevant. Size: not applicable — no core-module paths touched (cli Approach: the scope is exactly right. Flipping both gates to Risk: Moving on to code review. 🔍 中文说明感谢贡献! 模板完整 ✓(所有必需标题齐全;正文缺少模板中的中文 问题:已观测到的 bug,且已在基线代码中核实。schema 为 方向:对齐。此改动恢复了 schema 一直声明的行为;前缀缓存复用在上下游也有持续投入的迹象(claude-code 近期 CHANGELOG 中关于权限检查复用缓存前缀、错峰调度兄弟代理、fork 子代理继承 prompt cache 等多条记录)。CHANGELOG 中没有对该标志的直接引用,但该领域明显相关。 规模:不适用——未触及核心模块路径(仅 cli 的 方案:范围恰到好处。把两个 gate 改为 风险: 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewI formed my independent proposal before reading the diff: flip both gates to What I verified in the code:
No critical blockers, no convention violations. One caveat: PR code is never executed during triage, so the author's note that two unrelated tests ( CI evidence (snapshot at review time)The PR's own CI, fetched via API at the reviewed commit; nothing re-run locally. No red checks in this snapshot: the Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行(取最新一次运行),省略 skipped,失败项排在最前。 Sandboxed verification would settle this: 中文说明代码审查:我在看 diff 之前先给出了独立方案——把两个 gate 改为 已在代码中核实:这两处是 加强的测试是真正的钉子:describe 块的 CI 证据:评审时点通过 API 抓取的快照,未在本地重跑。快照中没有红色检查:Linux 单测套件与 SDK Java 真实 daemon E2E 仍在运行;其余完成项为绿色或按设计跳过(macOS/Windows 单测仅存在于合并队列,tmux/verify 通道由评论触发)。表格由 finalize 任务在 CI 落定后更新。 沙盒验证通道: — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 5/5 — clean across every stage; this is exactly the fix I would have written, and approval is only waiting on the in-flight unit suite and E2E. Stepping back: the problem is real — I verified it independently in the base code before reading the diff. The schema declares The one outstanding item is procedural, not substantive: the Linux unit suite and the SDK Java daemon E2E are still running on the reviewed commit, and 中文说明信心度:5/5——每个阶段都干净,这正是我自己会写出的修复;批准只在等待运行中的单测与 E2E。 回顾:问题是真实的——我在看 diff 之前已在基线代码中独立核实。schema 声明 唯一待办是流程性的,而非实质性的:Linux 单测套件与 SDK Java daemon E2E 仍在评审提交上运行,而 — Qwen Code · qwen3.8-max Reviewed at |
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. |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship — CI landed green after the review. ✅
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 5": executing the changed Session.test.ts tests to confirm they pass (worktree has no node_modules; verified by static mutation analysis of the assertions and mock ….
Test Plan (not a blocker): src/acp-integration/session/Session.test.ts — no such file or directory.
— qwen3.8-max via Qwen Code /review (v0.21.12)
|
Closeout from resolve-pr-comments: Changed: passed the prompt-suggestion AbortSignal through the cache-aware forked request path, and switched the daemon follow-up suggestion path to the existing history-tail API instead of cloning full history. Verified: ESLint, Prettier, and diff check passed; focused core/CLI vitest collection is still blocked locally by the existing missing |
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/acp-integration/session/Session.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/acp-integration/session/Session.test.ts — no such file or directory。
— qwen3.8-max via Qwen Code /review (v0.21.12)
R2-1 (Critical): the cache-aware follow-up-suggestion fork read the process-global, session-unbound currentCacheSafeParams slot, so in a multi-session daemon one session's suggestion could be built from another session's transcript + systemInstruction — a cross-session content leak this PR's default-on gate newly enabled. Record the owning session id in saveCacheSafeParams and, in generatePromptSuggestion, fall back to the session-safe base-LLM path when the slot's sessionId does not match config.getSessionId(). generateViaForkedQuery now receives the session-checked params instead of re-reading the slot (closes the check-then-read race). R2-2: corrected the #maybeEmitFollowupSuggestion JSDoc to reference getHistoryTail(40, true) instead of the removed getHistory(true).slice(-40). R2-3: pinned getHistoryTail(40, true) in the daemon followup test so a revert to the full structuredClone shape (#4624) or a dropped curated argument cannot pass silently. Tests: 4 cache-mode tests updated to carry a matching sessionId; new cross-session regression test asserts the fork is NOT used and the base-LLM fallback runs when the slot belongs to another session.
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline. 1 Suggestion-level finding(s) could not be anchored to a changed line and were dropped; nothing further to act on here.
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/acp-integration/session/Session.test.ts — no such file or directory.
中文说明
仅完成部分审查,审查缺口已披露。 建议见行内评论。 1 条建议级发现无法锚定到改动行,已丢弃;此处无需进一步处理。
未审查:build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
Test Plan(非阻断):src/acp-integration/session/Session.test.ts — no such file or directory。
— qwen3.8-max via Qwen Code /review (v0.21.12)
|
Closeout summary: latest head skips cloning foreign cache slots and logs session mismatches; I added the missing save-side sessionId round-trip pin. Verification: Prettier and git diff check passed; focused core vitest is locally blocked before collection by the existing missing ajv/dist/2020.js dependency. Not changed: cross-reader auto-memory/speculation ownership checks and retry-policy plumbing remain maintainer decisions. |
|
Closeout for this patrol: fixed the cache-sharing foreign-slot clone path, save-side session pin, and debug state; pushed 783f1d8. Verified focused suggestion/client tests, eslint on touched files, and diff-check. The remaining retry-ladder thread needs a maintainer scope decision; CI is still pending on the latest head. |
|
Closeout from resolve-pr-comments: no code change. Resolved the latest non-actionable thread and left the retry-cap idea as follow-up because it requires cross-layer retry plumbing outside this cache-sharing default fix. Verified: active review threads now 0. Pending: automatic review-pr is still running. |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
Not explored to full depth (tool budget reached): "agent 6c": none — no check was cut short..
Test Plan (not a blocker): src/acp-integration/session/Session.test.ts — no such file or directory.
中文说明
已审查。 建议见行内评论。
未探索到全部深度(达到工具调用预算):"agent 6c":none — no check was cut short.。
Test Plan(非阻断):src/acp-integration/session/Session.test.ts — no such file or directory。
— qwen3.8-max via Qwen Code /review (v0.21.12)
|
Closeout from resolve-pr-comments automation: Changed: added focused coverage for cache-sharing opt-out and cache-safe session ID access. Verified: core vitest for suggestionGenerator and forkedAgent cache passed. Intentionally not changed: naming-only tail-count cleanup. Pending: CI/review are running. |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline. 1 Suggestion-level finding(s) could not be anchored to a changed line and were dropped; nothing further to act on here.
Test Plan (not a blocker): src/acp-integration/session/Session.test.ts — no such file or directory.
中文说明
已审查——无阻断问题。 建议见行内评论。 1 条建议级发现无法锚定到改动行,已丢弃;此处无需进一步处理。
Test Plan(非阻断):src/acp-integration/session/Session.test.ts — no such file or directory。
— qwen3.8-max via Qwen Code /review (v0.21.12)
|
Resolved the remaining cache-session suggestion as follow-up: the production save path already tags the slot, and making the exported helper require sessionId would expand the type contract beyond this PR. No code changed. |
|
@qwen-code /review --timeout=360 |
|
Qwen Code review request accepted. Review is queued in workflow run. |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No blocking issues. LGTM! ✅
Test Plan (not a blocker): src/acp-integration/session/Session.test.ts — no such file or directory.
Deferred under the convergence posture (round 6, not a blocker) — recorded, not requested in this round:
packages/core/src/followup/suggestionGenerator.ts:119 — [review] 第二次会话归属检查恒为真——死分支伪装成 TOCTOU 防护,注释挂在不承重的检查上,诱导未来维护者删错检查
中文说明
无阻断问题。LGTM!✅
Test Plan(非阻断):src/acp-integration/session/Session.test.ts — no such file or directory。
收敛姿态下延后(第 6 轮,非阻断)——已记录,本轮不要求修改:共 1 条(原文未翻译,列表见上方英文部分)。
— qwen3.8-max via Qwen Code /review (v0.21.13)
|
Released in v0.21.14. |
What this PR does
Fixes the default-value bug from #9230:
enableCacheSharingdeclaresdefault: truein the settings schema (settingsSchema.ts), butmergeSettingsnever applies schema defaults, and both runtime gates compared with=== true— so an unset value read asundefined === true→ false, and the cache-aware forked suggestion query was dead code unless the user explicitly set the flag. Both gates now use!== false, the exact treatment the adjacentenableFollowupSuggestionsgate already applies for the same documented quirk:packages/cli/src/ui/AppContainer.tsx(TUI suggestion generation)packages/cli/src/acp-integration/session/Session.ts(daemon/ACP suggestion push)Why it's needed
With the fork dead by default, every follow-up suggestion goes through the prefix-hostile base-LLM side query, which on prefix-caching servers (e.g. llama.cpp
--parallel 1) evicts the main session's cached prefix between turns — the ~0% prompt-cache reuse measured in #9200/#9230. Honoring the declared default turns the cache-aware path back on for everyone instead of only users who found and set the hidden flag.Reviewer Test Plan
How to verify
New/updated pins in
Session.test.ts(follow-up suggestion describe):enableCacheSharing→ generator receivesenableCacheSharing: true(the strengthened assertion fails on the old=== truegate — red before, green after);enableCacheSharing: false→ generator receivesfalse(opt-out preserved).Full-file Session.test.ts run: 622 pass; the 2 failures (
runs prompt inside runtime output dir context,pins durable cron startup…) reproduce on clean main — pre-existing, unrelated.Evidence (Before & After)
Before: with
merged.uileaving the flag unset, the daemon path forwardedenableCacheSharing: false(assertion evidence above), sogeneratePromptSuggestionalways tookgenerateViaBaseLlm. After: the same state forwardstrue, enablinggetCacheSafeParams()/generateViaForkedQueryper the schema's declared default. Runtime cache-hit-rate improvement itself is observable only against a prefix-caching server (the #9200 setup).Tested on
Environment (optional)
Unit tests +
tsc --noEmit(clean) + eslint/prettier (clean).Risk & Scope
falserestores the old behavior.enableCacheSharingoff by default #9230): making the side query share the main session's system instruction/tools so it extends the same prefix, the forked path's 40-entry history truncation andNO_TOOLSdefault (which cap how much prefix the fork can reuse), and pre-send microcompaction destabilizing mid-history prefixes. This PR is the gate fix only.Linked Issues
Part of #9230
中文说明
修复 #9230 中的默认值 bug:
enableCacheSharing在 settings schema 声明default: true,但mergeSettings不应用 schema 默认值,两处运行时门控(交互式 CLI 的 AppContainer、ACP session 的 daemon 推送路径)又都用=== true判断,未设置时undefined === true为 false,cache-aware 分叉建议查询沦为死代码。两处改为!== false,与相邻enableFollowupSuggestions门控对同一已知问题的处理一致。修复前每个 follow-up suggestion 都走不带 system instruction、不带 tools 的 base-LLM 侧查询,在 llama.cpp 等前缀缓存服务端(
--parallel 1)上会挤掉主会话的缓存前缀,即 #9200/#9230 测得的约 0% prompt-cache 复用。本 PR 让 cache-aware 路径按 schema 声明对所有用户默认生效,而非只有手动配置了该隐藏 flag 的用户。测试:收紧
Session.test.ts中既有的宽松断言为enableCacheSharing: true(改前红、改后绿),另加显式 false opt-out 用例;follow-up suggestion 相关 8 个测试通过,tsc --noEmit与 eslint/prettier 干净。范围之外(均在 #9230 追踪):侧查询共享主会话 system instruction/tools、forked 路径 40 条历史截断与NO_TOOLS默认值、微压缩破坏历史中段前缀。无破坏性变更,配置 schema 未动。