Skip to content

fix(cache): preserve tools prefix in side-query for Anthropic prompt-cache hits - #6225

Merged
wenshao merged 7 commits into
QwenLM:mainfrom
kagura-agent:fix/side-query-cache-prefix
Jul 4, 2026
Merged

fix(cache): preserve tools prefix in side-query for Anthropic prompt-cache hits#6225
wenshao merged 7 commits into
QwenLM:mainfrom
kagura-agent:fix/side-query-cache-prefix

Conversation

@kagura-agent

@kagura-agent kagura-agent commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

What this PR does

Preserves the parent conversation's tools array in side-queries (suggestion mode, pipelined suggestions) so the Anthropic prompt-cache key (system + tools) stays identical to the main conversation, eliminating guaranteed cache misses and preventing cache eviction cascading.

Adds a preserveTools?: boolean option to CachePathParams and enables it for suggestion generation and pipelined speculation. Other forked-query callers (e.g., /btw, memory extract) keep the default tool-stripping behavior for backward compatibility.

Why it's needed

As reported in #5942 (Defect 1), runForkedAgent applies NO_TOOLS = { tools: [] } to every per-request config. For Anthropic providers, the cache key includes both system and tools — so even though the forked query shares the parent's systemInstruction, the different tools value (empty vs. full) causes a cache prefix mismatch. Each side-query becomes a guaranteed full-price request (~17% of requests in measured sessions) and can evict the main conversation's cached prefix, forcing the next main-thread request to also fully miss.

Claude Code, on the same Anthropic backend, runs side-queries with the same system + tools prefix (appending the suggestion instruction as a trailing message) and achieves ~100% cache hit rates — confirming the fix direction.

Reviewer Test Plan

How to verify

  1. Unit tests — run the test suite to confirm cache-path behavior:
    npx vitest run packages/core/src/utils/forkedAgent.cache.test.ts
    npx vitest run packages/core/src/followup/suggestionGenerator.test.ts
    npx vitest run packages/core/src/followup/speculation.test.ts
  2. Cache behavior verification — to observe the actual cache improvement, run qwen-code with an Anthropic provider and a proxy that logs request bodies (e.g., mitmproxy). Compare side-query requests before and after:
    • Before: side-query tools field is [], different from main conversation → cache miss
    • After: side-query tools field matches main conversation → cache hit (look for cache_read_input_tokens > 0 in the API response)
  3. Backward compatibility — forked-query callers that don't pass preserveTools: true (e.g., /btw command, memory extract) should continue to strip tools as before.

Evidence (Before & After)

Non-UI change (internal cache optimization). All test results:

  • forkedAgent.cache.test.ts: 16 tests ✅
  • forkedAgent.agent.test.ts: 13 tests ✅
  • suggestionGenerator.test.ts: 20 tests ✅
  • speculation.test.ts: 8 tests ✅
  • Full followup/ directory: 112 tests ✅

Tested on

OS Status
🍏 macOS N/A
🪟 Windows N/A
🐧 Linux

Environment (optional)

Local dev environment with npm run dev. Unit tests only — cache behavior is backend-specific and verified via test assertions on the config object passed to the provider.

Risk & Scope

Linked Issues

Fixes #5942 (Defect 1)

中文说明

本 PR 做了什么

在 side-query(建议模式、流水线建议)中保留父对话的 tools 数组,使 Anthropic 的 prompt-cache 键(system + tools)与主对话保持一致,消除必然的缓存未命中并防止缓存驱逐级联。

CachePathParams 中添加 preserveTools?: boolean 选项,并在建议生成和流水线推测中启用。其他 forked-query 调用方(如 /btw、记忆提取)保持默认的工具剥离行为以保证向后兼容。

为什么需要

#5942(缺陷 1)所报告,runForkedAgent 对每个请求配置应用 NO_TOOLS = { tools: [] }。对于 Anthropic 提供商,缓存键包含 systemtools —— 即使 forked query 共享父级的 systemInstruction,不同的 tools 值(空 vs. 完整)也会导致缓存前缀不匹配。每次 side-query 都会成为全价请求(测量会话中约 17% 的请求),并可能驱逐主对话的缓存前缀,迫使下一次主线程请求也完全未命中。

Claude Code 在同一 Anthropic 后端上,side-query 使用相同的 system + tools 前缀(将建议指令作为尾部消息追加),实现了约 100% 的缓存命中率 —— 这验证了修复方向。

审阅测试计划

如何验证

  1. 单元测试 — 运行测试套件确认缓存路径行为:
    npx vitest run packages/core/src/utils/forkedAgent.cache.test.ts
    npx vitest run packages/core/src/followup/suggestionGenerator.test.ts
    npx vitest run packages/core/src/followup/speculation.test.ts
  2. 缓存行为验证 — 使用 Anthropic 提供商和记录请求体的代理(如 mitmproxy)运行 qwen-code。对比 side-query 请求的前后变化:
    • 之前:side-query 的 tools 字段为 [],与主对话不同 → 缓存未命中
    • 之后:side-query 的 tools 字段与主对话匹配 → 缓存命中(查看 API 响应中 cache_read_input_tokens > 0)
  3. 向后兼容 — 未传 preserveTools: true 的 forked-query 调用方(如 /btw 命令、记忆提取)应继续像之前一样剥离工具。

证据(前后对比)

非 UI 变更(内部缓存优化)。所有测试结果:

  • forkedAgent.cache.test.ts: 16 tests ✅
  • forkedAgent.agent.test.ts: 13 tests ✅
  • suggestionGenerator.test.ts: 20 tests ✅
  • speculation.test.ts: 8 tests ✅
  • 完整 followup/ 目录: 112 tests ✅

测试环境

OS 状态
🍏 macOS N/A
🪟 Windows N/A
🐧 Linux

风险与范围

关联 Issue

修复 #5942(缺陷 1)

…cache hits

Side-queries (suggestion mode, pipelined suggestions) strip tools from
the per-request config via NO_TOOLS, which changes the Anthropic
prompt-cache key (system + tools). This causes guaranteed cache misses
(~17% of requests in measured sessions) and can evict the main
conversation's cached prefix.

Add a `preserveTools` option to CachePathParams that, when true, skips
the NO_TOOLS override so the forked query shares the exact same
system + tools prefix as the main agent — matching Claude Code's
behavior where side-queries achieve ~100% cache hit rates.

Enable `preserveTools: true` for:
- Prompt suggestion generation (suggestionGenerator.ts)
- Pipelined suggestion generation (speculation.ts)

Other forked query callers (e.g. /btw, memory extract) retain the
default tool-stripping behavior for backward compatibility.

Fixes QwenLM#5942 (Defect 1)

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR, @kagura-agent! The analysis of the cache-prefix problem is thorough and the fix looks minimal and well-targeted.

However, the PR body doesn't follow our PR template. Several required sections are missing:

  • ## What this PR does / ## Why it's needed — your Summary/Problem/Fix sections cover this content, but please use the template headings so reviewers can find things quickly.
  • ## Reviewer Test Plan — this is the most important missing piece. How should a reviewer verify the cache-hit improvement? Since prompt-cache behavior is backend-specific and hard to observe in CI, a clear "How to verify" section (e.g., proxy-captured request/response showing the tools array preserved in side-queries, or before/after cache_read token counts) is essential. The unit test results you included are useful but don't substitute for a reviewer-verifiable test plan.
  • ## Risk & Scope — what's the main risk of preserving tools in side-queries? (e.g., the model now sees tool definitions and could attempt function calls in a single-turn context) What's explicitly out of scope? (you already note Defect 2 — good, but put it in this section)
  • Tested on table — which OS did you test on?
  • <details>中文说明</details> — bilingual body is required for accessibility across our maintainer team.

Please restructure the PR body to match the template. The technical content is solid — it just needs to be organized so reviewers can work through it efficiently.

中文说明

感谢 @kagura-agent 的 PR!对缓存前缀问题的分析很深入,修复方案也很精简。

但 PR 正文没有遵循 PR 模板,缺少几个必要章节:

  • ## What this PR does / ## Why it's needed — Summary/Problem/Fix 覆盖了这些内容,但请使用模板标题方便审阅者快速定位。
  • ## Reviewer Test Plan — 最重要的缺失。审阅者该如何验证缓存命中率的改善?由于 prompt-cache 行为依赖于后端且 CI 中难以观测,清晰的验证步骤(如代理抓包对比、before/after 的 cache_read token 数)至关重要。单元测试结果是好的补充,但不能替代审阅者可执行的测试计划。
  • ## Risk & Scope — 在 side-query 中保留工具定义的主要风险是什么?明确不在范围内的内容(已提到 Defect 2,但请放在这个章节里)。
  • Tested on 表格 — 在哪些操作系统上测试过?
  • <details>中文说明</details> — 双语正文是必需的。

请按照模板重新组织 PR 正文。技术内容很好,只需要调整结构以便审阅。

Qwen Code · qwen3.7-max


const requestConfig: GenerateContentConfig = { ...NO_TOOLS };
const requestConfig: GenerateContentConfig = preserveTools
? {}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] When preserveTools: true, the model receives the parent's full tool definitions and could produce functionCall response parts instead of text. The response extraction loop at ~line 500 (.map((p) => p.text ?? '')) silently drops non-text parts — no defensive check, log, or warning.

Both current callers pair preserveTools with jsonSchema (constrained decoding), making this unlikely today. But a future caller without jsonSchema — or a provider behavior change — would silently produce { text: null, jsonResult: undefined } with no diagnostic trail.

Consider adding a guard in the stream loop:

const parts = response.candidates?.[0]?.content?.parts ?? [];
if (parts.some((p) => (p as Record<string, unknown>).functionCall)) {
  debugLogger.warn(
    'Cache-path forked query received functionCall with preserveTools; discarding.',
  );
}

— qwen3.7-max via Qwen Code /review

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! Added in fa04bea:

  • Defensive guard in the stream loop: detects functionCall parts when preserveTools is true, logs a warning via debugLogger.warn(), and filters them out before text extraction.
  • Also filters functionCall parts unconditionally in the .filter() chain so they never contribute empty strings to the text output.
  • Added createDebugLogger('FORKED_AGENT') for the warning.

All 57 tests pass (cache: 16, agent: 13, suggestion: 20, speculation: 8).

/**
* When true, keep the parent's tools in the per-request config so the
* Anthropic prompt-cache key (system + tools) matches the main agent's.
* Default (false/omitted): strip tools via NO_TOOLS to prevent function

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Nice to have] The ForkedQueryResult JSDoc at ~line 280 still reads "Single-turn, text-only — tools are denied." With preserveTools: true, tools are no longer denied. The module-level doc was updated correctly but the type-level doc was missed.

Suggest updating to: "Single-turn, text-only. Tools stripped by default; pass preserveTools to keep the parent's tools for cache-prefix matching."

— qwen3.7-max via Qwen Code /review

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in fa04bea! Updated the JSDoc to:

Single-turn, text-only. Tools stripped by default; pass preserveTools to keep the parent's tools for cache-prefix matching.

Thanks for catching the inconsistency.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in fa04bea! Updated the JSDoc to:

Single-turn, text-only. Tools stripped by default; pass preserveTools to keep the parent's tools for cache-prefix matching.

Thanks for catching the inconsistency.

@kagura-agent

Copy link
Copy Markdown
Contributor Author

Thanks for the template feedback! I've restructured the PR body to match the required format:

  • What this PR does / Why it's needed — reorganized with template headings
  • Reviewer Test Plan — added step-by-step verification (unit tests + proxy-based cache observation + backward compat check)
  • Risk & Scope — documented tool-visibility risk in side-queries and explicit out-of-scope note for Defect 2
  • Tested on — Linux ✅, macOS/Windows N/A
  • 中文说明 — full bilingual translation added

Ready for re-review!

…sult JSDoc

Address review feedback:
- Add defensive check in response extraction: when preserveTools is true
  and the model returns functionCall parts, log a warning and filter them
  out instead of silently dropping to empty text.
- Update ForkedQueryResult JSDoc to reflect that tools are stripped by
  default but can be preserved via preserveTools option.

Co-authored-by: qwen-code-ci-bot <qwen-code-ci-bot@users.noreply.github.com>
Comment thread packages/core/src/utils/forkedAgent.ts Outdated
// functionCall parts instead of text. Log and discard them.
if (
preserveTools &&
parts.some((p) => (p as Record<string, unknown>).functionCall)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Critical] TS4111: .functionCall uses dot notation on Record<string, unknown>, which violates TypeScript's noPropertyAccessFromIndexSignature. This causes the build to fail (tsc --noEmit exits with code 2), blocking CI.

Line 517 above already uses bracket notation for ['thought'] on the same type — this is inconsistent.

Suggested change
parts.some((p) => (p as Record<string, unknown>).functionCall)
parts.some((p) => (p as Record<string, unknown>)['functionCall'])

The same fix is needed on line 518:

Suggested change
parts.some((p) => (p as Record<string, unknown>).functionCall)
.filter((p) => !(p as Record<string, unknown>)['functionCall'])

— qwen3.7-max via Qwen Code /review

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 2ab6f80 — changed both .functionCall occurrences to bracket notation ['functionCall'], matching the existing ['thought'] pattern. No new TS errors in forkedAgent.ts (confirmed via tsc --noEmit).

expect(createRuntimeContentGeneratorView).not.toHaveBeenCalled();
});

it('does not strip tools when preserveTools is true', async () => {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] The three new tests verify tool-stripping behavior at the per-request config level, but none covers the defensive functionCall filter added in the same commit (forkedAgent.ts:505–520). Consider adding a test where preserveTools: true and the mock stream yields a response containing both functionCall and text parts, asserting:

  1. result.text contains only the text content
  2. functionCall parts are discarded
  3. The debugLogger.warn path is exercised

This is the highest-risk new logic — a regression in the filter would silently produce garbled output.

— qwen3.7-max via Qwen Code /review

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added in 2ab6f80 — new test 'filters out functionCall parts when preserveTools is true' in forkedAgent.cache.test.ts. The test mocks a stream chunk containing both a text part and a functionCall part, calls runForkedAgent with preserveTools: true, and asserts only the text content is returned. All 17 tests in the file pass.

jsonSchema: PIPELINED_SCHEMA,
...(model !== undefined ? { model } : {}),
abortSignal,
preserveTools: true,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] speculation.ts now passes preserveTools: true but has no corresponding test asserting this argument is passed. The suggestionGenerator has a parallel test, but speculation is a separate caller with its own code path (pipelined suggestions with a different prompt and schema). If this flag is accidentally removed in a future refactor, no test will catch it.

Consider adding a test in speculation.test.ts that mocks runForkedAgent and asserts it is called with preserveTools: true.

— qwen3.7-max via Qwen Code /review

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added in 2ab6f80 — new test 'passes preserveTools: true to runForkedAgent' in speculation.test.ts. The test runs a text-only speculation to completion (triggering generatePipelinedSuggestion), then asserts runForkedAgent was called with preserveTools: true. All 9 tests in the file pass.

TypeScript noPropertyAccessFromIndexSignature requires bracket notation
for Record<string, unknown> index signature properties. Changed
.functionCall to ['functionCall'] on lines that filter/check for
function call parts.
@kagura-agent

Copy link
Copy Markdown
Contributor Author

Fixed the TS build failure — noPropertyAccessFromIndexSignature requires bracket notation for Record<string, unknown> index signatures. Changed .functionCall['functionCall'] on the two flagged lines (509, 518).

Local build + tests pass (16/16 forkedAgent cache tests, 20/20 suggestionGenerator tests).

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No review findings. Downgraded from Approve to Comment: CI still running.

— qwen3.7-max via Qwen Code /review

Address PR QwenLM#6225 round-2 review feedback:

- TS4111: change dot-notation `.functionCall` to bracket-notation
  `['functionCall']` on Record<string, unknown> casts, matching the
  existing `['thought']` pattern (noPropertyAccessFromIndexSignature).
- Add test: speculation passes preserveTools: true to runForkedAgent
  when generating pipelined suggestions.
- Add test: forkedAgent cache path filters out functionCall parts from
  model stream when preserveTools is true, returning only text content.
expect(sendParams.config!.tools).toBeUndefined();
});

it('strips tools by default when preserveTools is omitted', async () => {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] This test ("strips tools by default when preserveTools is omitted") is functionally redundant with the pre-existing test at line 199 ("passes tools: [] in per-request config so the model cannot produce function calls"). Both omit preserveTools, call runForkedAgent with the same shape, and assert sendParams.config!.tools equals []. The duplicate increases maintenance burden — if the behavior changes, two tests must be updated.

Consider removing this test; the existing test at line 199 already covers the default path more thoroughly (it also verifies GeminiChat constructor args, prompt_id, and result text).

— qwen3.7-max via Qwen Code /review

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 7405fed — removed this test. The first test at line 199 already covers the default preserveTools omission path more thoroughly.

expect(sendParams.config!.tools).toEqual([]);
});

it('filters out functionCall parts when preserveTools is true', async () => {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] This test covers mixed parts (one text + one functionCall), but there's no test for the more critical scenario: a response where all parts are functionCall (zero text parts). That path — where fullText stays empty and result.text becomes null — is the exact defensive scenario preserveTools was designed for, yet it's untested.

Consider adding:

it('returns null text when response contains only functionCall parts', async () => {
  // mock yields: parts: [{ functionCall: { name: 'edit', args: {} } }]
  const result = await runForkedAgent({ ..., preserveTools: true });
  expect(result.text).toBeNull();
});

— qwen3.7-max via Qwen Code /review

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added in 7405fed — new test 'returns null text when response contains only functionCall parts'. Mocks a response with a single functionCall part and zero text parts, asserts result.text is null (the defensive filter removes all functionCall parts → fullText stays empty → fullText.trim() || nullnull). All 17 tests pass.

- Add 'returns null text when response contains only functionCall parts':
  tests the defensive filter edge case where the model produces zero text
  parts, verifying fullText stays empty and result.text becomes null.
- Remove 'strips tools by default when preserveTools is omitted': redundant
  with the existing test at line 199 which already covers the default path
  more thoroughly (CI bot review suggestion).
expect(createRuntimeContentGeneratorView).not.toHaveBeenCalled();
});

it('does not strip tools when preserveTools is true', async () => {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] preserveTools: true combined with jsonSchema is never tested at the runForkedAgent level. Both production callers (suggestionGenerator.ts:159 and speculation.ts:564) always pass both together, but no test verifies the combined request config has tools absent (preserveTools honored) while also having responseMimeType: 'application/json' and responseJsonSchema set. If the requestConfig assembly had a bug where one option clobbered the other, no test would catch it.

Consider adding a test that passes both preserveTools: true and jsonSchema, then asserts all three properties on the captured config (tools is undefined, responseMimeType is 'application/json', responseJsonSchema is set) and that result.jsonResult is parsed correctly.

— qwen3.7-max via Qwen Code /review

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added in 8d53a0e — new test preserves tools and includes jsonSchema fields when both preserveTools and jsonSchema are set in forkedAgent.cache.test.ts.

The test:

  1. Saves cacheSafeParams with tools
  2. Calls runForkedAgent with both preserveTools: true and a jsonSchema
  3. Captures the params passed to sendMessageStream
  4. Asserts config.tools is undefined (tools not stripped — preserveTools working)
  5. Asserts config.responseMimeType === "application/json"
  6. Asserts config.responseJsonSchema matches the passed schema

All 49 tests pass (18 forkedAgent.cache + 21 suggestionGenerator + 10 speculation).

cacheSafeParams,
jsonSchema: SUGGESTION_SCHEMA,
model,
preserveTools: true,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] preserveTools: true is passed unconditionally, even when the resolved model differs from cacheSafeParams.model. When the fast model override is active and differs from the parent model, cache keys cannot match (prompt caches are model-specific on every provider), so tools are forwarded for zero cache benefit. The model also receives the parent's full tool definitions and could produce functionCall parts that get silently discarded by the defensive filter.

Consider making preserveTools conditional on whether the model actually matches:

Suggested change
preserveTools: true,
preserveTools: model === cacheSafeParams.model,

This also applies to the parallel caller in speculation.ts:564.

— qwen3.7-max via Qwen Code /review

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in 8d53a0e:

  • suggestionGenerator.ts: changed to preserveTools: model === cacheSafeParams.model
  • speculation.ts: introduced resolvedModel = model ?? cacheSafeParams.model, then preserveTools: resolvedModel === cacheSafeParams.model
  • Added tests in both suggestionGenerator.test.ts and speculation.test.ts verifying preserveTools: false when getFastModel() returns a model different from cacheSafeParams.model

All 49 tests pass.

preserveTools: true is only beneficial when the side-query's resolved
model matches cacheSafeParams.model (the main agent's model). Prompt
cache is model-specific, so a different model cannot hit the tools
prefix cache — passing preserveTools in that case just adds token
overhead without cache benefit.

- suggestionGenerator: preserveTools = (model === cacheSafeParams.model)
- speculation: introduce resolvedModel, same condition
- Add test: preserveTools + jsonSchema coexist without conflict
- Add tests: preserveTools is false when fast model differs
it('passes preserveTools: false when fast model differs from cache-safe model', async () => {
const config = {
getApprovalMode: vi.fn().mockReturnValue(ApprovalMode.DEFAULT),
getCwd: vi.fn().mockReturnValue(process.cwd()),

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] The two describe blocks ('generatePipelinedSuggestion preserveTools' and 'generatePipelinedSuggestion preserveTools conditional') share ~45 lines of identical setup — only getFastModel return value and the expected preserveTools boolean differ. This codebase uses it.each extensively (125+ matches in packages/core/src) for exactly this pattern:

describe('generatePipelinedSuggestion preserveTools', () => {
  it.each([
    { fastModel: undefined, expected: true, label: 'matches cache-safe model' },
    { fastModel: 'different-fast-model', expected: false, label: 'differs from cache-safe model' },
  ])('passes preserveTools: $expected when $label', async ({ fastModel, expected }) => {
    // single shared config/mock setup
  });
});

— qwen3.7-max via Qwen Code /review

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done — merged both blocks into a single describe.each with [{ fastModel, expectedPreserveTools }] parameterization. Went from 90 → 53 lines. All 10 tests in the file pass.

wenshao
wenshao previously approved these changes Jul 3, 2026

@wenshao wenshao left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found. LGTM! ✅

— qwen3.7-max via Qwen Code /review

Merge two near-identical describe blocks into a single parameterized
describe.each block. The shared setup (~45 lines) is now written once,
with only the getFastModel return value and expected preserveTools
boolean varying between cases.

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No review findings. Downgraded from Approve to Comment: CI still running.

— qwen3.7-max via Qwen Code /review

@wenshao

wenshao commented Jul 4, 2026

Copy link
Copy Markdown
Collaborator

✅ Maintainer local real-build verification — PASS

I built the PR head (4f2e89f3) and verified it end-to-end with the real qwen binary driven against a fake Anthropic /v1/messages endpoint that records every request body. The fix does exactly what the description claims: the suggestion side-query now ships the same system + tools prefix as the main conversation — the precise condition Anthropic prompt-caching needs to reuse a cached prefix.

1 · Unit tests + mutation A/B — the fix is load-bearing

npx vitest run on the three changed suites → 49/49 pass (forkedAgent.cache 18 · suggestionGenerator 21 · speculation 10).

To prove the new tests actually guard the behavior (not vacuous), I ran two mutations and confirmed each fails only the tests that assert the true path:

Mutation (revert) What it neutralizes Result
requestConfig = preserveTools ? {} : NO_TOOLSalways NO_TOOLS tool-preservation in forkedAgent.ts 2 forkedAgent.cache tests FAILdoes not strip tools when preserveTools is true, preserves tools … when both preserveTools and jsonSchema
both callers' preserveTools: model === cacheSafeParams.modelfalse the opt-in in suggestionGenerator.ts + speculation.ts 2 caller tests FAILpasses preserveTools: true for Anthropic prompt-cache sharing, generatePipelinedSuggestion … same model … preserveTools: true

Both mutations leave the preserveTools: false tests green → precise, non-vacuous coverage of both branches.

2 · Real end-to-end capture (over the wire)

Real qwen binary → anthropic provider (ANTHROPIC_BASE_URL → local fake /v1/messages logging each request). Interactive TUI in tmux, two prompts, ui.enableCacheSharing: true, no fastModel so the side-query runs on the main model → preserveTools = true. The follow-up-suggestion side-query fires after the 2nd turn (needs ≥2 assistant turns).

I compared the cache prefix sha256(system + tools) of the suggestion side-query against the main-conversation turn, on the fixed binary vs a pre-fix mutant (always-strip):

request A/B

main turn suggestion side-query prefix
AFTER (PR head) 15 tools · 49b49fa75934 15 tools · 49b49fa75934 ✅ identical → cache-hit eligible
BEFORE (pre-fix) 15 tools · 49b49fa75934 0 tools · 0ad833df6f6b ❌ divergent → guaranteed miss

After the fix the side-query carries the full parent tool set (agent, read_file, grep_search, run_shell_command, todo_write, glob, edit, write_file, skill, tool_search, … — 15 tools); before, it sent tools: []. Real TUI with the suggestion firing:

real TUI

Notes for the merge decision

  • Honest scope of the proof. The local fake can't reproduce Anthropic's server-side cache accounting, so I did not measure cache_read_input_tokens. What I proved is the necessary & sufficient client-side condition: the side-query's system + tools prefix is now byte-identical to the main conversation's — that is the Anthropic prompt-cache key.
  • UI-invisible / no regression. The suggestion renders identically in both A/B; the change is purely the request shape. The preserveTools:false branch (a distinct fastModel) is covered by the unit tests above.
  • Tight blast radius. The unrelated background managed-memory forked query (the bg memory / 7-tool rows) is unchanged across A/B — the PR touches only the suggestion & pipelined-speculation side-queries, exactly as intended.
  • typecheck ✅ · eslint (changed files) ✅ · full packages/core suite (14,156 tests): the only failures were Test timed out in 5000ms flakes in git/ripgrep-subprocess suites unrelated to this PR (filesearch/crawler, gitDiff, team-memory-sync) under local machine load — the count varied 5→23 across two runs, and re-running those three files in isolation passes 116/116. Zero failures in the PR's touched code.

Verdict: LGTM. Correct, minimal, load-bearing, and does what the description claims. Verified against head 4f2e89f3.

中文版(完整对应)

✅ 维护者本地真实构建验证 —— 通过

我构建了 PR head(4f2e89f3),并用真实 qwen 二进制对接一个会记录每个请求体的假 Anthropic /v1/messages 端点做了端到端验证。修复完全符合描述所声称的效果:建议 side-query 现在发送与主对话相同的 system + tools 前缀——这正是 Anthropic prompt-caching 复用缓存前缀所需的条件。

1 · 单元测试 + 变异 A/B —— 修复是"承重"的

对三个改动套件跑 npx vitest run49/49 通过forkedAgent.cache 18 · suggestionGenerator 21 · speculation 10)。

为证明新测试真正守护行为(而非空过),我做了两处变异,确认每处只让断言 true 路径的测试失败:

变异(回退) 中和了什么 结果
requestConfig = preserveTools ? {} : NO_TOOLS永远 NO_TOOLS forkedAgent.ts 里的 tools 保留 2 个 forkedAgent.cache 测试 FAIL —— does not strip tools when preserveTools is truepreserves tools … when both preserveTools and jsonSchema
两个调用方的 preserveTools: model === cacheSafeParams.modelfalse suggestionGenerator.ts + speculation.ts 的启用 2 个调用方测试 FAIL —— passes preserveTools: true for Anthropic prompt-cache sharinggeneratePipelinedSuggestion … same model … preserveTools: true

两处变异都让 preserveTools: false 的测试保持绿色 → 对两个分支都是精准、非空过的覆盖。

2 · 真实端到端抓包(over the wire)

真实 qwen 二进制 → anthropic provider(ANTHROPIC_BASE_URL 指向本地假 /v1/messages,记录每个请求)。tmux 里交互式 TUI,两条 prompt,ui.enableCacheSharing: true不配 fastModel,使 side-query 走主模型 → preserveTools = true。后续建议 side-query 在第 2 轮后触发(需 ≥2 个 assistant turn)。

我对比了建议 side-query主对话 turn 的缓存前缀 sha256(system + tools),分别在修复版二进制与 pre-fix 变异版(永远剥离)上:

(见上方第一张图)

主对话 turn 建议 side-query 前缀
AFTER(PR head) 15 tools · 49b49fa75934 15 tools · 49b49fa75934 ✅ 一致 → 可命中缓存
BEFORE(pre-fix) 15 tools · 49b49fa75934 0 tools · 0ad833df6f6b ❌ 分叉 → 必然未命中

修复后 side-query 携带完整父 tool 集(agent, read_file, grep_search, run_shell_command, todo_write, glob, edit, write_file, skill, tool_search, … 共 15 个);修复前发送 tools: []。真实 TUI 中建议触发见上方第二张图。

合并决策注记

  • 证据的诚实边界。 本地假服务器无法复现 Anthropic 服务端的缓存计费,因此我没有测 cache_read_input_tokens。我证明的是客户端的充要条件:side-query 的 system + tools 前缀现在与主对话逐字节一致——这本身就是 Anthropic prompt-cache 的键。
  • UI 不可见 / 无回归。 建议在两版 A/B 中渲染完全一致;改动纯粹是请求形状。preserveTools:false 分支(配置了不同的 fastModel)由上述单测覆盖。
  • 影响面收敛。 无关的后台"托管记忆"forked query(bg memory / 7-tool 行)在 A/B 中保持不变——PR 仅改动建议与流水线推测的 side-query,完全符合设计意图。
  • typecheck ✅ · eslint(改动文件)✅ · 完整 packages/core 套件(14,156 个测试):仅有的失败是与本 PR 无关的 git/ripgrep 子进程套件(filesearch/crawlergitDiffteam-memory-sync)在本地负载下的 Test timed out in 5000ms flake——两次运行失败数在 5→23 间浮动,且将这三个文件隔离重跑为 116/116 全过。PR 改动的代码中零失败。

结论:LGTM。 正确、最小、承重,且符合描述所声称的效果。基于 head 4f2e89f3 验证。

Verification harness: real binary + fake Anthropic /v1/messages recorder + tmux + unit/mutation A/B. Screenshots are from the actual run.

@wenshao

wenshao commented Jul 4, 2026

Copy link
Copy Markdown
Collaborator

@qwen-code /triage

@qwen-code-ci-bot

qwen-code-ci-bot commented Jul 4, 2026

Copy link
Copy Markdown
Collaborator

Stage 0: Core Module Protection — noted, proceeding.

This PR touches 6 files in packages/core/src/**, with +513/-12 lines reported by GitHub. The 500-line threshold is crossed numerically, but the actual source code changes are ~89 lines — the rest is test coverage. Per AGENTS.md: "Breadth ≠ size... a low-risk sweep... can touch 10+ files while changing only a line or two each. Don't auto-reject on file count alone."

Source-only breakdown: forkedAgent.ts (+54/-25), speculation.ts (+3/-2), suggestionGenerator.ts (+2/-3). Focused optimization with thorough tests, not a large-scale core refactor. Flagged for maintainer awareness; full triage proceeds in Stages 1–3 below.

中文说明

Stage 0: 核心模块保护 — 已注意,继续。

此 PR 涉及 packages/core/src/** 中的 6 个文件,GitHub 报告 +513/-12 行。500 行阈值在数字上被超过,但实际源代码更改约 89 行——其余为测试覆盖。按 AGENTS.md:"广度 ≠ 规模……低风险扫描……可以触及 10+ 个文件而每个只改一两行。不要仅因文件数量自动拒绝。"

源代码明细:forkedAgent.ts(+54/-25)、speculation.ts(+3/-2)、suggestionGenerator.ts(+2/-3)。聚焦的优化配合完善的测试,非大规模核心重构。已标记给维护者注意;完整分流在下方 Stages 1–3 中进行。

Qwen Code · qwen3.7-max

@wenshao

wenshao commented Jul 4, 2026

Copy link
Copy Markdown
Collaborator

@qwen-code /triage

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Thanks for the PR!

Template looks good ✓ — all required headings present, bilingual body complete.

Problem: observed, well-documented. Issue #5942 provides proxy-level measurements showing 22/129 requests (17%) with cache_read = 0, clustering around side-query misses that cascade into main-thread misses. Claude Code on the same Anthropic backend achieves ~100% cache hits for identical side-queries by keeping the same system + tools prefix. This is a real cost problem, not theoretical hardening.

Direction: aligned. Cache optimization for Anthropic providers matches what Claude Code already does on the same backend. The PR correctly scopes to Defect 1 only (Defect 2 — conversation breakpoint placement — is explicitly out of scope). Non-breaking: default behavior unchanged.

Approach: minimal and focused. One new optional parameter (preserveTools), one conditional branch (preserveTools ? {} : { ...NO_TOOLS }), defensive functionCall filtering for the edge case, and a debug logger. The condition preserveTools: model === cacheSafeParams.model correctly ensures tools are only preserved when the side-query uses the same model as the main conversation — different fast models still get tools stripped. Source changes are ~89 lines; the rest is thorough test coverage on both branches.

Moving on to code review. 🔍

中文说明

感谢贡献!

模板完整 ✓ — 所有必需标题齐全,双语正文完整。

问题:已观测,有据可查。 Issue #5942 提供了代理级测量数据,显示 22/129 个请求(17%)的 cache_read = 0,集中在 side-query 未命中导致主线程级联未命中。Claude Code 在相同 Anthropic 后端上通过保持相同的 system + tools 前缀,对相同的 side-query 实现了约 100% 的缓存命中。这是一个真实的成本问题,而非理论性加固。

方向:对齐。 针对 Anthropic 提供商的缓存优化与 Claude Code 在相同后端上的做法一致。PR 正确地仅涵盖缺陷 1(缺陷 2——对话断点放置——明确不在范围内)。非破坏性:默认行为不变。

方案:最小且聚焦。 一个新可选参数(preserveTools),一个条件分支(preserveTools ? {} : { ...NO_TOOLS }),针对边缘情况的防御性 functionCall 过滤,以及调试日志。条件 preserveTools: model === cacheSafeParams.model 正确确保仅当 side-query 使用与主对话相同的模型时才保留工具——不同的快速模型仍会剥离工具。源代码更改约 89 行,其余为两个分支的完善测试覆盖。

进入代码审查 🔍

Qwen Code · qwen3.7-max

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Code Review

No blockers. The implementation is clean and minimal.

The core logic change in forkedAgent.ts is a single conditional: preserveTools ? {} : { ...NO_TOOLS }. The defensive functionCall part filter handles the one edge case that preserveTools: true introduces (model could theoretically emit tool calls in a side-query), and the debug logger uses the existing createDebugLogger pattern. Both callers (suggestionGenerator.ts, speculation.ts) correctly gate preserveTools on model identity — only preserving tools when the side-query uses the same model as the main conversation.

No correctness bugs, no security holes, no AGENTS.md violations. The functionCall filter is justified defensive code, not speculative hardening — the PR documents the risk and the filter handles it.

Testing

All 49 tests across the three affected suites pass on the PR head (4f2e89f3):

$ cd packages/core && npx vitest run src/utils/forkedAgent.cache.test.ts src/followup/suggestionGenerator.test.ts src/followup/speculation.test.ts

 RUN  v3.2.4 packages/core
      Coverage enabled with v8

 ✓ src/utils/forkedAgent.cache.test.ts (18 tests) 15ms
 ✓ src/followup/suggestionGenerator.test.ts (21 tests) 10ms
 ✓ src/followup/speculation.test.ts (10 tests) 167ms

 Test Files  3 passed (3)
      Tests  49 passed (49)
   Duration  6.48s

New tests cover both branches precisely: preserveTools: true (tools not stripped, functionCall parts filtered) and preserveTools: false (tools stripped as before), plus caller-level tests verifying the model-identity condition.

npm run typecheck ✅ · eslint on changed source files ✅

Note: this is a non-UI change (internal cache optimization). The before/after behavior difference is in the HTTP request shape (tools field in the API request body), not in terminal output. The maintainer @wenshao independently verified the wire-level behavior with a real binary against a fake Anthropic endpoint in this comment, confirming the side-query's cache prefix hash matches the main conversation after the fix.

中文说明

代码审查

无阻塞问题。实现干净且最小。

forkedAgent.ts 中的核心逻辑更改是一个条件:preserveTools ? {} : { ...NO_TOOLS }。防御性 functionCall part 过滤处理了 preserveTools: true 引入的一个边缘情况(模型理论上可能在 side-query 中发出工具调用),调试日志使用现有的 createDebugLogger 模式。两个调用方(suggestionGenerator.tsspeculation.ts)都正确地将 preserveTools 基于模型一致性——仅当 side-query 使用与主对话相同的模型时才保留工具。

无正确性 bug、无安全漏洞、无 AGENTS.md 违规。functionCall 过滤器是有依据的防御代码,非投机性加固。

测试

PR head(4f2e89f3)上三个受影响套件的 49 个测试全部通过(见上方终端输出)。

新测试精准覆盖两个分支:preserveTools: true(工具未被剥离,functionCall parts 被过滤)和 preserveTools: false(工具如前被剥离),以及调用方级别的模型一致性条件测试。

npm run typecheck ✅ · eslint(改动的源文件) ✅

注意:这是非 UI 更改(内部缓存优化)。before/after 的行为差异体现在 HTTP 请求形状(API 请求体中的 tools 字段),而非终端输出。维护者 @wenshao 已在此评论中用真实二进制对接假 Anthropic 端点独立验证了线上行为,确认修复后 side-query 的缓存前缀哈希与主对话匹配。

Qwen Code · qwen3.7-max

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

This PR fixes a real, measured cost problem — #5942's proxy data shows 17% of requests getting zero cache hits due to side-query prefix divergence, and Claude Code on the same backend demonstrates the fix is purely client-side. The implementation does exactly what it claims in the most minimal way possible: one new optional parameter, one conditional branch, defensive filtering for the edge case it introduces.

My independent proposal would have been identical: add a flag to control tool-stripping in runForkedAgent, thread it from the two callers that need cache-identity, and filter out any functionCall parts defensively. The PR matches this exactly, with the added refinement of gating on model identity (model === cacheSafeParams.model) — correctly ensuring tools are only preserved when the cache prefix would actually match.

All 49 tests pass, typecheck clean, lint clean. The maintainer's independent wire-level verification confirms the fix achieves its goal at the HTTP level. No scope creep, no drive-by refactors, no unnecessary abstraction.

LGTM — approving. ✅

中文说明

此 PR 修复了一个真实的、已测量的成本问题——#5942 的代理数据显示 17% 的请求因 side-query 前缀分叉而缓存命中为零,而 Claude Code 在相同后端上证明修复完全是客户端的。实现以最小化的方式完成了所声称的功能:一个新可选参数、一个条件分支、针对引入的边缘情况的防御性过滤。

我的独立方案会完全一致:添加一个标志来控制 runForkedAgent 中的工具剥离,从需要缓存一致性的两个调用方传递,并防御性地过滤任何 functionCall parts。PR 完全匹配这一方案,并增加了基于模型一致性的条件(model === cacheSafeParams.model)——正确确保仅当缓存前缀实际匹配时才保留工具。

49 个测试全部通过,typecheck 干净,lint 干净。维护者的独立线上验证确认修复在 HTTP 级别达到了目标。无范围蔓延、无顺手重构、无不必要的抽象。

LGTM — 批准。✅

Qwen Code · qwen3.7-max

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, looks ready to ship. ✅

@qwen-code-ci-bot qwen-code-ci-bot added category/performance Performance and optimization scope/caching Caching mechanisms labels Jul 4, 2026
@qwen-code-ci-bot qwen-code-ci-bot added the type/bug Something isn't working as expected label Jul 4, 2026
@wenshao
wenshao added this pull request to the merge queue Jul 4, 2026
Merged via the queue into QwenLM:main with commit 1a227f0 Jul 4, 2026
49 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

category/performance Performance and optimization scope/caching Caching mechanisms type/bug Something isn't working as expected

Projects

None yet

3 participants