fix(core): refresh systemInstruction in setTools() so progressive MCP tools reach the model#4166
Merged
Merged
Conversation
… tools reach the model Under PR #3994's progressive MCP path, Config.initialize() runs startChat() BEFORE MCP discovery starts, then kicks discovery off in the background and re-runs setTools() once it settles. But setTools() only updated chat.generationConfig.tools — not systemInstruction — and MCP tools are shouldDefer=true, so they were filtered out of declarations anyway. The prompt's "Deferred Tools" listing was frozen at the built-in-only snapshot from the initial startChat(), and the model had no signal that any MCP tool existed. Headless --prompt runs silently regressed to built-ins (issue #4163); interactive mode had the same gap but was masked by retries. setTools() now rebuilds the system instruction with the up-to-date deferred summary and re-binds it to the live chat. The eager-reveal guard for "ToolSearch unavailable + deferred tools present" moves with it so a freshly-arrived MCP tool in `--exclude-tools tool_search` sessions still lands in declarations instead of disappearing silently. Shared with startChat() / refreshSystemInstruction() via a new private resolveDeferredToolsForSystemPrompt() helper so the three paths cannot drift apart again. The legacy synchronous path (QWEN_CODE_LEGACY_MCP_BLOCKING=1) was incidentally correct because discovery happened before startChat(); it remains correct. Test plan: - packages/core/src/core/client.test.ts — three new cases covering newly-arrived MCP tools, already-revealed filtering, and the no-ToolSearch eager-reveal path. - Full client.test.ts (107 tests) green. - tool-search / skill-manager / agent / mcp-client-manager / AppContainer test suites green (callers of setTools()). - CI integration: integration-tests/cli/simple-mcp-server.test.ts is expected to pass on first try without QWEN_CODE_LEGACY_MCP_BLOCKING. Fixes #4163 Generated with AI Co-authored-by: Qwen-Coder <[email protected]>
Contributor
📋 Review SummaryThis PR fixes a critical regression where MCP tools registered after 🔍 General Feedback
🎯 Specific Feedback🟡 High
// Current (duplicated logic):
await toolRegistry.warmAll();
const deferredSummary = toolRegistry.getDeferredToolSummary();
const toolSearchAvailable = !!toolRegistry.getTool(ToolNames.TOOL_SEARCH);
const deferredTools = toolSearchAvailable
? deferredSummary.filter(...)
: undefined;
// Should be:
await toolRegistry.warmAll();
const deferredTools = this.resolveDeferredToolsForSystemPrompt();This duplication was the root cause of the original bug and should be fully eliminated. 🟢 Medium
// Rebuild system instruction so deferred tools reach the model.
// See PR #4166 for why this refresh is required after progressive MCP discovery.
this.getChat().setSystemInstruction(
this.getMainSessionSystemInstruction(deferredTools),
);
🔵 Low
recordStartupEvent('gemini_tools_updated', {
toolCount: toolDeclarations.length,
deferredCount: deferredTools?.length ?? 0,
toolSearchAvailable, // helpful for debugging silent-disappearance issues
});
if (history.length === 0) {
const deferredTools = this.resolveDeferredToolsForSystemPrompt();
// ...
}✅ Highlights
|
Contributor
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. |
chiga0
commented
May 15, 2026
| // built-in-only snapshot taken inside startChat(). The model then has | ||
| // no signal that an MCP tool exists and never invokes ToolSearch to | ||
| // reveal it — silently regressing non-interactive `--prompt` runs. | ||
| this.getChat().setSystemInstruction( |
Collaborator
Author
There was a problem hiding this comment.
[P1] 这个 setSystemInstruction() 在当前 main 上会和 #4115 的 SessionStart hook 改动冲突：startChat() 现在会先把 SessionStart additionalContext 通过 chat.applySessionStartContext(...) 追加进去，然后立即调用 setTools()。如果 rebase 时保留这里的直接重设 system instruction，它会把刚追加的 hook context 覆盖掉；后续 MCP/skill refresh 触发 setTools() 也会再次清掉它。建议像 refreshSystemInstruction() 一样在重设后重新 apply lastSessionStartContext，或者把 setTools() 调整到追加 SessionStart context 之前，并补一个 hook context 不被 setTools 刷掉的回归测试。
Collaborator
Author
There was a problem hiding this comment.
已在合并 main（#4115 SessionStart hook）时按你的建议处理：
修复（commit ebe27b8）：setTools() 在 setSystemInstruction 之后会重新 apply lastSessionStartContext，镜像 refreshSystemInstruction() 的模式。这样 startChat 末尾的 setTools、AppContainer 16ms 批量 flush、以及 waitForMcpReady 之后的 trailing setTools 都不会再把 hook context 擦掉。
回归测试（commit 6ff1c48）：新增 preserves SessionStart additionalContext when refreshing via setTools，组合调用 startChat（带 SessionStart hook）+ 显式 setTools，断言最终 systemInstruction 同时包含 refreshed base prompt 和 <qwen:session-start-context> 块。
Generated by claude-opus-4-7
Collaborator
Author
Multi-Round Code Review — PR #41661) OverviewFixes a silent regression introduced by PR #3994 (progressive MCP discovery). In non-interactive The fix:
2) Round 1 — CorrectnessOrder of operations in Resume scan ordering in Idempotence under concurrent Behavior change in 3) Round 2 — Tests & coverage
Full test runs pass locally: 4) Round 3 — Side-effects / blast radius
Potential concern — startup latency: No concern about subagents mutating parent state: 5) Round 4 — Code quality / styleStrengths:
Minor nits (non-blocking):
6) Round 5 — Security / perf
7) Issues foundNone blocking. This is a tight, well-scoped fix:
SummaryPR #4166 is safe to merge. The fix is surgical, addresses both the headline #4163 regression and a related disappearance window, and the shared helper makes future drift between the three call sites mechanically impossible. No correctness, performance, or side-effect concerns surfaced after 5 review rounds. Existing test suites (548 tests across the touched packages) all pass. Generated by Claude Opus 4.7 (claude-opus-4-7) |
Conflict in packages/core/src/core/client.ts: the SessionStart hook infrastructure from main and this branch's resolveDeferredToolsForSystemPrompt helper both landed in the same region. Both are kept side by side. The order in startChat now interleaves SessionStart context with setTools's new system-instruction rebuild — setTools's rebuild used to clobber the just-applied SessionStart context, so setTools now re-applies lastSessionStartContext after rewriting the system instruction, mirroring refreshSystemInstruction's pattern. The refresh-after-SessionStart test gets a third mockReturnValueOnce because startChat now invokes getCoreSystemPrompt twice (once for initial chat, once via the trailing setTools). Generated by claude-opus-4-7 Co-authored-by: Claude <[email protected]>
Adds the regression test chiga0 asked for in the PR #4166 review: proves that setTools()'s setSystemInstruction-then-reapply pattern keeps the SessionStart hook's additionalContext intact, so progressive-MCP refreshes (AppContainer batch flush + the trailing setTools after waitForMcpReady) don't silently strip hook context from the system instruction. Generated by claude-opus-4-7 Co-authored-by: Claude <[email protected]>
Collaborator
Author
Review triage针对最新两轮评论的处理（严格区分 real vs false positive，不偏离 PR 主线）： ✅ Real issue — handled
❌ False positive — skipped
⏭️ Out of mainline — intentionally skipped为了不偏离 PR 主线（修复 progressive MCP 下 systemInstruction 不刷新的 silent regression），以下风格/范围扩展建议本 PR 不处理：
Generated by claude-opus-4-7 |
tanzhenxin
approved these changes
May 15, 2026
tanzhenxin
left a comment
tanzhenxin
left a comment
Collaborator
There was a problem hiding this comment.
Review
Diagnosis is exact. After PR #3994 made MCP discovery progressive, setTools() was the only post-init refresh the non-interactive path waited on, but it only updated the tool declarations — and MCP tools are deferred, so they reach the model exclusively through the system prompt's deferred-tools listing, which stayed frozen at the built-in-only snapshot taken at chat construction. The fix rebuilds the system instruction from the same builder used at chat construction and re-applies the cached SessionStart context block. The shared helper that drives startChat, setTools, and refreshSystemInstruction also centralises the eager-reveal-when-ToolSearch-unavailable branch, which closes a parallel disappearance window for --exclude-tools tool_search sessions.
I traced all three claims end-to-end: the non-interactive path picks up the fix via the setTools() continuation that waitForMcpReady already awaits; interactive mode picks it up via the AppContainer batched mcp-client-update flush; and the legacy synchronous path is a correct no-op because MCP discovery runs before chat construction. The follow-up SessionStart-preservation test is the right regression guard.
Verdict
APPROVE — fix is correct, minimal, well-tested, and the three refresh sites now share one helper so they cannot drift again.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #4163. Headless
--promptruns silently lost every MCP tool after PR #3994 (progressive MCP availability) becausesetTools()only refreshedchat.tools— not the system prompt — and MCP tools areshouldDefer=trueso they never appear in thetoolsdeclaration list anyway. The model had no signal that any MCP tool existed and never invokedToolSearchto reveal one.setTools()now rebuilds the system instruction with the up-to-date deferred-tools summary and re-binds it to the live chat. Same call site, additive contract.resolveDeferredToolsForSystemPrompt()sostartChat()/setTools()/refreshSystemInstruction()cannot drift apart again. The!toolSearchAvailable → eager-revealguard now applies on every refresh path, not just startup — closing a second silent-disappearance window for sessions running--exclude-tools tool_search.Config.startMcpDiscoveryInBackground's trailinggeminiClient.setTools()already runs afterawait waitForMcpReady(), so the non-interactive path picks up the fix without any change togemini.tsx. Interactive mode is fixed at the same time viaAppContainer'smcp-client-updatebatch flush.Why this didn't break the legacy path
QWEN_CODE_LEGACY_MCP_BLOCKING=1runs MCP discovery synchronously insidecreateToolRegistry, so by the timestartChat()runs,getDeferredToolSummary()already lists MCP tools. The legacy path was correct by accident of ordering — and remains correct after this fix.Why this doesn't undo PR #3994's startup win
startChat()still runs without waiting for MCP. The only extra work is onegetMainSessionSystemInstruction()string composition insidesetTools()after discovery settles — sub-millisecond, well after first-screen.Test plan
packages/core/src/core/client.test.ts— three new cases insetTools — system instruction refresh: newly-arrived MCP tool reaches the deferred-tools arg; already-revealed tools filtered out;!toolSearchAvailabletriggers eager-reveal and renders without the deferred-tools section.client.test.ts(107 tests) green.tool-search.test.ts,skill-manager.test.ts,agent.test.ts,agent-override.test.ts,mcp-client-manager.test.ts,AppContainer.test.tsx— all callers ofsetTools()— green (267 tests total).tsc --noEmitclean.integration-tests/cli/simple-mcp-server.test.tsexpected to pass on first try withoutQWEN_CODE_LEGACY_MCP_BLOCKING=1. (Local re-run not performed — requires real model credentials.)🤖 Generated with Qwen Code