feat(perf): progressive MCP availability — MCP no longer blocks first input - #3994
Conversation
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. |
…ntion sections Restructures the PR rollout based on baseline-informed re-prioritization: - PR-A = original PR4 (progressive MCP). Ship first — biggest user-facing improvement (7-10s → 340ms for MCP users) and the only PR with behavioral semantic change, so isolating it from other refactors makes the audit trail cleaner. - PR-B = original PR2 (loadSettingsAsync) + PR3 (parallel init + lazy entry). Merged because PR2's measured win shrank to ~3ms (baseline showed after_load_settings at 9ms warm-cache, not the 100-200ms estimated in the original design). Folding it into PR3's "startup main path optimization" narrative keeps review cost low and avoids a near-empty standalone PR. - PR0+1 (#3994) remains the hard prerequisite for both. New sections in design.md § 4: - § 4.0 Label mapping: old PR2/3/4 → new PR-A/PR-B-α/PR-B-β so existing decision-log references stay traceable without mechanical rewrites. - § 4.1 Restructured rollout with the "why this order" rationale. - § 4.2 PR inter-dependency matrix. PR-A and PR-B have no logical dependencies on each other; they share AppContainer.tsx / gemini.tsx edits so serial merge is recommended to avoid rebase churn. - § 4.3 UX / breaking-change matrix per PR. Explicit audit checklist for PR-A (grep all `MCPDiscoveryState.COMPLETED` and `config.initialize` call sites + integration tests + release notes). - § 4.4 Bundle size impact. PR-A negligible; PR-B headless chunk predicted -30~50% (Ink + AppContainer + themes + hooks剥离), with scripts/check-bundle-leakage.mjs as the CI gate. - § 4.5 PR0+1 retention strategy. Documents that production code, scripts, fixtures, summary.json baselines, Heisenberg report, node-pty devDep, and design docs all stay on main; only raw.jsonl + report.md are gitignored. Avoids splitting "infrastructure" from "feature" PRs. §§ 6-8 headers updated to PR-B Phase α / PR-B Phase β / PR-A labels with cross-references to § 4 for context. Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
3fe5351 to
322aaa9
Compare
322aaa9 to
68486b3
Compare
chiga0
changed the title
97eff9c to
c8eef71
Compare
… input
Today `Config.initialize()` runs MCP discovery synchronously and the cli
can't accept input until every configured MCP server finishes its
discover handshake. One slow or hung server bottlenecks every user with
MCP configured. Validated by the profiler instrumentation added in this
PR (set `QWEN_CODE_PROFILE_STARTUP=1` to reproduce):
| User scenario | Time to first prompt input |
| ------------------------- | -------------------------- |
| No MCP | ~480 ms |
| 1 fast MCP | ~875 ms |
| 2 fast + 1 slow MCP | **~7.1 s** |
| 1 hung MCP server | **~10.5 s** |
(Measured on macOS arm64 / Node 24.15, n=30/fixture, p50.)
`Config.initialize()` now passes `{ skipDiscovery: true }` to
`createToolRegistry` by default and kicks off MCP discovery in a
fire-and-forget background path. As each server completes discover,
the cli's `AppContainer` debounces `setTools()` calls into one-frame
(16 ms) batches so the model sees the consolidated tool list shortly
after each server settles. Rollback: `QWEN_CODE_LEGACY_MCP_BLOCKING=1`.
- `packages/core/src/config/config.ts` — `Config.initialize` switches
to `skipDiscovery: true` + new `startMcpDiscoveryInBackground()`
(defensive against partially-stubbed `ToolRegistry` in tests). Adds
`MCPServerConfig.discoveryTimeoutMs` (last positional ctor param —
doesn't shift existing call sites). Tool-call timeout is untouched.
- `packages/core/src/tools/tool-registry.ts` — new
`getMcpClientManager()` getter so the background path can call the
incremental discover directly without going through `discoverMcpTools`
(which would wipe already-registered tools).
- `packages/core/src/tools/mcp-client-manager.ts` —
`discoverAllMcpToolsIncremental` now: emits `mcp-client-update`
after IN_PROGRESS transition, wraps each per-server discover in a
discovery-only timeout (stdio 30s, remote 5s), emits trailing
`mcp-client-update` after COMPLETED so UI subscribers see the
terminal state.
- `packages/cli/src/ui/AppContainer.tsx` — new `useEffect` (gated on
`isConfigInitialized`) subscribes to `mcp-client-update` and
16ms-batches `setTools()` calls. Same effect also defers
`finalizeStartupProfile` until MCP settles (or 35s hard cap), so
startup-perf profiles capture the full MCP timeline.
Activated only by `QWEN_CODE_PROFILE_STARTUP=1`; when unset every
profiler entry point short-circuits in a single null/flag check and
returns. Heisenberg overhead measured at -1.12% Δp50 between
profile-on vs profile-off (Welch p=0.092, n=30/config × 3 configs) —
within statistical noise.
- `packages/cli/src/utils/startupProfiler.ts` — extended with
`events` array (multi-fire), `recordStartupEvent`,
`setInteractiveMode`, `derivedPhases`, per-checkpoint heap snapshots,
`MAX_EVENTS` cap, and `QWEN_CODE_PROFILE_STARTUP_OUTER` / NO_HEAP
env opt-ins. + 7 new tests.
- `packages/core/src/utils/startupEventSink.ts` (new) — minimal
cross-package sink so `core` can emit profiler events without
reverse-depending on `cli`. No-op when no sink registered. + 4 tests.
- `packages/core/src/index.ts` — export `setStartupEventSink` /
`recordStartupEvent` / type aliases.
- `packages/cli/src/gemini.tsx` — registers the sink at `main()`
entry, adds `first_paint` checkpoint after Ink render, calls
`setInteractiveMode(true)` in the interactive branch.
- `packages/core/src/config/config.ts` — emits
`tool_registry_created`.
- `packages/core/src/core/client.ts` — emits `gemini_tools_updated`
at the end of `setTools()`.
- `packages/core/src/tools/mcp-client-manager.ts` — emits
`mcp_discovery_start`, `mcp_server_ready:<name>`,
`mcp_first_tool_registered`, `mcp_all_servers_settled`.
- `packages/cli/src/ui/AppContainer.tsx` — emits
`config_initialize_start`, `config_initialize_end`, `input_enabled`.
`Config.initialize()` now returns BEFORE MCP discovery completes.
Things to check:
- Any code path that assumed "after `config.initialize()`, all MCP
tools exist in the registry" — these will see only built-in tools
initially; new tools appear via `mcp-client-update` events.
- `MCPDiscoveryState.COMPLETED` is now set asynchronously instead of
synchronously after `initialize()` resolves.
- Model requests issued before MCP settles see only built-in tools;
subsequent requests see the full set as servers come online.
- Tests that assert MCP tool count immediately after
`config.initialize()` should wait for the `mcp-client-update` with
COMPLETED discoveryState instead.
- 313 impacted-area tests green (config / mcp-client-manager / client
/ startupProfiler 18 / startupEventSink 4).
- `tsc --noEmit` clean for `packages/core` and `packages/cli`.
- `eslint` clean on touched files.
- Manual: `QWEN_CODE_PROFILE_STARTUP=1 SANDBOX=1` interactive run
produces a JSON profile in `~/.qwen/startup-perf/` containing
`first_paint`, `config_initialize_start/end`, `input_enabled`,
MCP per-server events, and `gemini_tools_updated`. See PR
description's "How to validate" section.
Generated with AI
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
c8eef71 to
8aaec50
Compare
| startupWarnings: string[], | ||
| workspaceRoot: string = process.cwd(), | ||
| initializationResult: InitializationResult, | ||
| ) { |
There was a problem hiding this comment.
[Critical] runtime.json sidecar deleted from interactive mode
The writeRuntimeStatus + markRuntimeStatusEnabled block and the writeRuntimeStatus import were removed. runtimeStatusEnabled is now never set to true, making the session-swap sidecar logic in Config.refreshSessionId() dead code.
External integrations (terminal multiplexers, IDE integrations, status daemons) that map PID → session ID via runtime.json silently break. This appears accidental — the removal was in the same diff hunk as profiler additions.
| ) { | |
| const version = await getCliVersion(); | |
| setWindowTitle(basename(workspaceRoot), settings); | |
| // Write a small runtime.json sidecar next to the chat log so external | |
| // tools (terminal multiplexers, IDE integrations, status daemons) can | |
| // map the running PID back to its session id and work directory. | |
| try { | |
| const sessionId = config.getSessionId(); | |
| const runtimeStatusPath = | |
| config.storage.getRuntimeStatusPath(sessionId); | |
| await writeRuntimeStatus(runtimeStatusPath, { | |
| sessionId, | |
| workDir: config.getTargetDir(), | |
| qwenVersion: version, | |
| }); | |
| config.markRuntimeStatusEnabled(); | |
| } catch { | |
| // ignored: best-effort, never block UI startup. | |
| } |
— glm-5.1 via Qwen Code /review
There was a problem hiding this comment.
Confirmed real regression — fixed in 6dcea68 → restored via commit a49b3e2 (also restores markRuntimeStatusEnabled(), which is what arms the Config.refreshSessionId() session-swap refresh path you cited).
Verified the diff against main:
writeRuntimeStatusre-imported from@qwen-code/qwen-code-core- block re-added in
startInteractiveUIbetweensetWindowTitleandinstallTerminalRedrawOptimizer - still wrapped in try/catch so a read-only fs does not block UI startup (matches the original behavior)
Non-interactive paths still never call markRuntimeStatusEnabled, so they won't trample a sibling shell's sidecar on the same session id, which preserves the original design.
| new Error( | ||
| `MCP server '${serverName}' discovery timed out after ${timeoutMs}ms`, | ||
| ), | ||
| ); |
There was a problem hiding this comment.
[Critical] Timed-out MCP server silently registers tools
runWithDiscoveryTimeout rejects on timeout but does NOT cancel the underlying discoverMcpToolsForServer. The background promise continues — if the server eventually responds, its tools are registered into the live toolRegistry and mcp-client-update is emitted. The user sees the server as "failed" but its tools are silently active.
A slow/attacker-controlled MCP server can inject tools into the model's surface after the discovery timeout, including tools that shadow built-in ones.
| ); | |
| private runWithDiscoveryTimeout<T>( | |
| serverName: string, | |
| serverConfig: MCPServerConfig | undefined, | |
| fn: () => Promise<T>, | |
| ): Promise<T> { | |
| const timeoutMs = this.discoveryTimeoutFor(serverConfig); | |
| let timedOut = false; | |
| return new Promise<T>((resolve, reject) => { | |
| const timer = setTimeout(() => { | |
| timedOut = true; | |
| // Disconnect the client to prevent silent background tool registration | |
| const client = this.clients.get(serverName); | |
| if (client) { | |
| void client.disconnect().catch(() => {}); | |
| } | |
| reject( | |
| new Error( | |
| `MCP server '${serverName}' discovery timed out after ${timeoutMs}ms`, | |
| ), | |
| ); | |
| }, timeoutMs); | |
| fn().then( | |
| (value) => { | |
| clearTimeout(timer); | |
| if (!timedOut) resolve(value); | |
| }, | |
| (err) => { | |
| clearTimeout(timer); | |
| if (!timedOut) reject(err instanceof Error ? err : new Error(String(err))); | |
| }, | |
| ); | |
| }); | |
| } |
— glm-5.1 via Qwen Code /review
There was a problem hiding this comment.
Real and exploitable as you flagged — fixed in 6b07a85 with the same disconnect-on-timeout strategy you suggested.
Implementation differs slightly: I keep a local timedOut flag and silently swallow a late success path so the inner promise can't accidentally re-fire mcp_server_ready:ready / mcp_first_tool_registered after the outer rejection. New test runWithDiscoveryTimeout disconnects the client on timeout to abort silent tool registration pins the disconnect.
Combined with the new discoveryTimeoutMs clamping (next thread) and the websocket transport fix, the worst case is now a 5s remote / 30s stdio bounded window with the client torn down at the boundary — client.connect() throws and the registry never receives any tools.
| if (serverConfig?.discoveryTimeoutMs !== undefined) { | ||
| return serverConfig.discoveryTimeoutMs; | ||
| } | ||
| // Remote transports (HTTP/SSE) carry network risk and get a shorter |
There was a problem hiding this comment.
[Suggestion] discoveryTimeoutMs accepts 0/negative/Infinity without validation
Values pass through to setTimeout unvalidated. 0 causes immediate timeout for every server; Infinity hangs waitForMcpReady() forever in non-interactive mode. Combined with the silent background registration bug above, discoveryTimeoutMs: 0 makes it reliably exploitable.
| // Remote transports (HTTP/SSE) carry network risk and get a shorter | |
| private discoveryTimeoutFor(serverConfig?: MCPServerConfig): number { | |
| if (serverConfig?.discoveryTimeoutMs !== undefined) { | |
| return Math.max(100, Math.min(serverConfig.discoveryTimeoutMs, 300_000)); | |
| } | |
| const isRemote = !!(serverConfig?.httpUrl || serverConfig?.url); | |
| return isRemote ? 5_000 : 30_000; | |
| } |
— glm-5.1 via Qwen Code /review
There was a problem hiding this comment.
Fixed in 6b07a85 with the clamp you suggested (100ms floor, 300_000ms ceiling). Added Number.isFinite so Infinity falls back to the per-transport default rather than passing through.
New test discoveryTimeoutMs is clamped to a minimum and maximum asserts setTimeout is called with the clamped values and never with 0, -5, or 10_000_000.
| */ | ||
| private startMcpDiscoveryInBackground(): void { | ||
| const registry = this.toolRegistry as ToolRegistry & { | ||
| getMcpClientManager?: () => { |
There was a problem hiding this comment.
[Suggestion] Type-unsafe cast on getMcpClientManager
Uses (this.toolRegistry as ToolRegistry & { getMcpClientManager?: () => ... }) with optional chaining, even though getMcpClientManager() is now a public method on ToolRegistry (added in this PR). If the method is ever removed or renamed, TypeScript won't flag this call site. AppContainer.tsx calls it directly without the cast, showing inconsistency.
| getMcpClientManager?: () => { | |
| const manager = this.toolRegistry.getMcpClientManager(); |
— glm-5.1 via Qwen Code /review
There was a problem hiding this comment.
Tightened in a49b3e2 — kept a minimal optional-chain guard but the cast now preserves the typed ReturnType<ToolRegistry['getMcpClientManager']>, so a future rename WILL be flagged at this call site (vs. the previous structurally-typed cast which would have silently accepted any function returning anything).
I considered going all-in on the direct call as you suggested, but several core tests stub ToolRegistry as a plain object via createMockToolRegistry-style fixtures; calling a method that doesn't exist on the stub would crash the init path in tests that don't exercise MCP at all. The defensive guard preserves that ergonomics while still being type-safe to renames.
AppContainer.tsx continues to call it without the cast because it has a real ToolRegistry in scope.
| // Terminal and layout hooks | ||
| const { columns: terminalWidth, rows: terminalHeight } = useTerminalSize(); | ||
| const previousTerminalWidthRef = useRef(terminalWidth); | ||
| const { stdin, setRawMode } = useStdin(); |
There was a problem hiding this comment.
[Suggestion] Terminal resize handler deleted
previousTerminalWidthRef, repaintStaticViewport, and the useEffect that called it on terminalWidth changes were removed. No replacement repaint mechanism exists. Users who resize terminals (tmux splits, fullscreen toggle) will see stale header content.
If this removal was intentional, please add a comment explaining why. Otherwise, restore the resize handler.
— glm-5.1 via Qwen Code /review
There was a problem hiding this comment.
Confirmed accidental — fixed in 6dcea68. Restored previousTerminalWidthRef, repaintStaticViewport, and the terminalWidth-watching useEffect at their original positions. Verified the diff against main so the restored block matches the pre-PR shape (cursor-to + erase-down, not full clearTerminal — keeping the no-flash optimization).
The new effect runs alongside the existing PTY-resize useEffect at the same terminalWidth dep, so resize ordering is preserved.
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [config]); | ||
|
|
||
| /** |
There was a problem hiding this comment.
[Suggestion] AppContainer MCP batch-flush useEffect untested
This ~85-line useEffect implements 16ms batched setTools() + deferred finalizeStartupProfile and is the core interactive-mode wiring for progressive MCP availability. It has zero test coverage — no AppContainer.test.tsx exists.
Consider adding tests covering: (a) setTools() called after mcp-client-update + flush window; (b) finalizeStartupProfile on COMPLETED; (c) cap timeout fires even when discovery never completes; (d) cleanup unsubscribes from mcp-client-update.
— glm-5.1 via Qwen Code /review
There was a problem hiding this comment.
Fair point. I won't add full coverage in this PR pass (a ~85-line effect with timers + event emitter + manager state needs dedicated harness work to avoid flakes), but I added several focused tests at lower layers that pin the contracts this useEffect depends on:
discoverAllMcpToolsIncremental enforces a per-server discoveryTimeoutMs— caps server discovery, which keeps the COMPLETED state finite.discoverAllMcpToolsIncremental emits the trailing mcp-client-update after COMPLETED— guarantees the subscriber wakes up at the terminal state (your (b) case).runWithDiscoveryTimeout disconnects the client on timeout— prevents the cap-timeout-fires case from racing setTools().discoverAllMcpToolsIncremental skips servers flagged as disabled— keeps the event volume bounded.
Will track a follow-up issue to add the dedicated useEffect tests for (a) flush coalescing, (b) COMPLETED → finalize ordering (now post-flush, see r3223777822), (c) cap timeout, (d) unsubscribe on unmount.
| nonInteractiveConfig, | ||
| trimmedInput.length > 0 ? trimmedInput : '', | ||
| settings, | ||
| ); |
There was a problem hiding this comment.
[Suggestion] Stream-json path dropped settings argument
The settings parameter was removed from the runNonInteractiveStreamJson call, causing it to fall back to createMinimalSettings() (empty settings). The parallel runNonInteractive path still passes settings. This appears accidental — the removal is in the same hunk as the waitForMcpReady() addition.
| ); | |
| await runNonInteractiveStreamJson( | |
| nonInteractiveConfig, | |
| trimmedInput.length > 0 ? trimmedInput : '', | |
| settings, | |
| ); |
— glm-5.1 via Qwen Code /review
There was a problem hiding this comment.
Confirmed accidental — fixed in a49b3e2. The settings argument is now passed through. Also added a regression guard in gemini.test.tsx:
const [configArg, inputArg, settingsArg] = runStreamJsonSpy.mock.calls[0];
...
expect(settingsArg).toBeDefined();Without this, stream-json sessions silently fell back to createMinimalSettings() and lost any user-configured permission / approval / hook setup.
17d00eb to
8aaec50
Compare
| // user-perceived metric ("first MCP server is ready"). | ||
| let firstToolEventFired = false; | ||
|
|
||
| // Find servers that are new or have changed configuration |
There was a problem hiding this comment.
[Critical] discoverAllMcpToolsIncremental doesn't skip disabled MCP servers
The new incremental method populates serversToUpdate from Object.entries(servers) without calling cliConfig.isMcpServerDisabled(name). The existing discoverAllMcpTools correctly skips disabled servers (line ~96: if (cliConfig.isMcpServerDisabled(name)) { ... return; }), but the new method connects and registers tools for servers the user explicitly disabled.
Add if (cliConfig.isMcpServerDisabled(name)) continue; inside the for (const [name] of Object.entries(servers)) loop that populates serversToUpdate.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
There was a problem hiding this comment.
Real bug, confirmed by inspection — fixed in 6b07a85. The exact check you suggested:
if (cliConfig.isMcpServerDisabled(name)) {
debugLogger.debug(`Skipping disabled MCP server: ${name}`);
continue;
}is now in the for (const [name] of Object.entries(servers)) loop that populates serversToUpdate. Mirror of the existing protection in discoverAllMcpTools (line ~102). New test discoverAllMcpToolsIncremental skips servers flagged as disabled regression-pins it.
| } catch (error) { | ||
| recordStartupEvent(`mcp_server_ready:${name}`, { | ||
| outcome: 'failed', | ||
| reason: getErrorMessage(error), |
There was a problem hiding this comment.
[Critical] serverDiscoveryPromises stale entry permanently blocks reconnection after timeout
When runWithDiscoveryTimeout fires, the per-server catch block (line 513) does not call this.serverDiscoveryPromises.delete(name). The underlying discoverMcpToolsForServer is still stuck at connect(), so its finally hasn't executed and the Map entry remains. Any subsequent discoverMcpToolsForServer call hits the dedup guard (if (inProgressDiscovery) { await inProgressDiscovery; return; }) and waits on the hung promise forever.
| reason: getErrorMessage(error), | |
| } catch (error) { | |
| this.serverDiscoveryPromises.delete(name); | |
| recordStartupEvent(`mcp_server_ready:${name}`, { | |
| outcome: 'failed', | |
| reason: getErrorMessage(error), | |
| }); |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
There was a problem hiding this comment.
Partially real. Let me walk through the trace, because the fix is now load-bearing on the disconnect-on-timeout fix (your Critical thread above):
runWithDiscoveryTimeoutrejects on timeout. BEFORE my fix, the innerdiscoverMcpToolsForServerkept running — itsconnect()was still hung. The dedup Map entry stayed because the inner promise'sfinally(line 168-172) hadn't run.- With the disconnect-on-timeout fix in 6b07a85,
runWithDiscoveryTimeoutnow forcesclient.disconnect()when it times out.client.connect()then throws (transport closed mid-handshake), the inner promise rejects, and itsfinallyclears the Map entry.
So the stale-entry symptom you described is no longer reachable in the steady state.
That said, I still added your suggested defensive this.serverDiscoveryPromises.delete(name) in the per-server catch (line 514) as belt-and-suspenders — if any future code path bypasses the disconnect (e.g. a custom client that ignores disconnect()), the Map entry won't leak.
| // Remote transports (HTTP/SSE) carry network risk and get a shorter | ||
| // default; stdio servers we trust the user already runs locally. | ||
| const isRemote = !!(serverConfig?.httpUrl || serverConfig?.url); | ||
| return isRemote ? 5_000 : 30_000; |
There was a problem hiding this comment.
[Suggestion] discoveryTimeoutFor misses TCP/websocket transport
The remote transport detection !!(serverConfig?.httpUrl || serverConfig?.url) misses the tcp field (position 8 in MCPServerConfig, used for WebSocket transport). A TCP MCP server gets the 30s stdio default instead of the documented 5s remote default.
| return isRemote ? 5_000 : 30_000; | |
| const isRemote = !!(serverConfig?.httpUrl || serverConfig?.url || serverConfig?.tcp); |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
There was a problem hiding this comment.
Right — fixed in 6b07a85. The remote check now includes tcp:
const isRemote = !!(
serverConfig?.httpUrl ||
serverConfig?.url ||
serverConfig?.tcp
);New test discoveryTimeoutFor treats websocket (tcp) transport as remote pins it by spying on setTimeout and asserting 5_000 is used (not 30_000).
| // setTools() emits `gemini_tools_updated` internally; we don't | ||
| // need to record anything from here. Errors are logged inside | ||
| // GeminiClient — never throw into the React tree. | ||
| void geminiClient.setTools().catch(() => {}); |
There was a problem hiding this comment.
[Suggestion] setTools() silent error swallow with misleading comment
The comment says "Errors are logged inside GeminiClient" but GeminiClient.setTools() (client.ts:356-369) has no try/catch. If setTools() throws (e.g. from warmAll(), getFunctionDeclarations(), or getChat().setTools()), the error is silently discarded with no logging, making production debugging difficult.
| void geminiClient.setTools().catch(() => {}); | |
| void geminiClient.setTools().catch((err) => { | |
| debugLogger.error( | |
| `setTools() batch-flush failed: ${err instanceof Error ? err.message : String(err)}`, | |
| ); | |
| }); |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
There was a problem hiding this comment.
Fixed in 6dcea68. The .catch(() => {}) is gone — setTools() errors are now routed through debugLogger.error:
return geminiClient.setTools().catch((err) => {
debugLogger.error(
`setTools() batch-flush failed: ${err instanceof Error ? err.message : String(err)}`,
);
});Same handler is reused by the new flushNow() path for the COMPLETED → finalize ordering fix (see r3223777822). Verified via direct read of client.ts:356-369 that GeminiClient.setTools() does not wrap warmAll() / getFunctionDeclarations() / getChat().setTools() — so this is the only debugging surface for those failures in production.
| // during initialize() / waitForMcpReady() are captured. Subsequent stdin | ||
| // reads / auth checks / prompt execution are not part of the | ||
| // "first-screen" budget. | ||
| finalizeStartupProfile(config.getSessionId()); |
There was a problem hiding this comment.
[Suggestion] Stream-json path finalizes startup profile before config.initialize()
For stream-json, config.initialize() is skipped (line 760-764) but await config.waitForMcpReady() (line 772, no-op) and finalizeStartupProfile() (line 777) still execute. The profile file is written without config_initialize_* checkpoints or any MCP events. When Session.initialize() later calls config.initialize() on the same config, the module-level finalized guard in startupProfiler.ts prevents event capture. Result: stream-json startup profiles are empty/misleading.
Suggested fix: move finalizeStartupProfile inside the if (inputFormat !== InputFormat.STREAM_JSON) block, or defer it to after Session.initialize() completes.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
There was a problem hiding this comment.
Confirmed and fixed in a49b3e2. Moved finalizeStartupProfile(config.getSessionId()) inside the if (inputFormat !== InputFormat.STREAM_JSON) block (your first suggestion).
For stream-json, the profile is now finalized inside Session.ensureConfigInitialized AFTER config.initialize() + waitForMcpReady() complete:
// session.ts
await this.config.initialize(options);
await this.config.waitForMcpReady();
// ... mcp warning emit ...
finalizeStartupProfile(this.config.getSessionId());This way the profile captures config_initialize_start, config_initialize_end, mcp_first_tool_registered, mcp_all_servers_settled, and gemini_tools_updated. The previous behavior produced empty stream-json profiles because the module-level finalized guard suppressed every event after the early finalize.
| // GeminiClient.setTools) into the cli's startup profiler. No-op when | ||
| // QWEN_CODE_PROFILE_STARTUP is unset because `recordStartupEvent` returns | ||
| // early in that case. | ||
| setStartupEventSink((name, attrs) => recordStartupEvent(name, attrs)); |
There was a problem hiding this comment.
[Suggestion] Profiler event sink always registered even when profiling is disabled
setStartupEventSink is called unconditionally. isStartupProfilerEnabled() exists in startupProfiler.ts:366 but is never imported or called. With QWEN_CODE_PROFILE_STARTUP unset, every recordStartupEvent() from core code traverses: sink null-check → arrow function → recordStartupEvent() → enabled check → return — unnecessary overhead per event.
| setStartupEventSink((name, attrs) => recordStartupEvent(name, attrs)); | |
| if (isStartupProfilerEnabled()) { | |
| setStartupEventSink((name, attrs) => recordStartupEvent(name, attrs)); | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
There was a problem hiding this comment.
Applied in a49b3e2 with the exact gate you suggested:
if (isStartupProfilerEnabled()) {
setStartupEventSink((name, attrs) => recordStartupEvent(name, attrs));
}isStartupProfilerEnabled() is now imported alongside the other profiler helpers. Safe because initStartupProfiler() is called at the very top of packages/cli/index.ts (before this module is even imported), so the enabled flag is already set by the time main() reaches this guard.
For the QWEN_CODE_PROFILE_STARTUP=unset case the cost drops to a single null-check on the core side (if (sink) in startupEventSink.ts:45), no arrow allocation per event.
| } | ||
| }) | ||
| .catch((err: unknown) => { | ||
| this.debugLogger.error( |
There was a problem hiding this comment.
Test comment - please ignore
| } | ||
| }) | ||
| .catch((err: unknown) => { | ||
| this.debugLogger.error( |
There was a problem hiding this comment.
[Critical] Silent MCP discovery failure — no user-visible warning in non-interactive paths
startMcpDiscoveryInBackground()'s .catch() only calls debugLogger.error(). When ALL configured MCP servers fail (network partition, binary not on PATH, auth expiry), non-interactive paths (--prompt, stream-json, ACP) proceed with only built-in tools and zero user-visible output. The legacy synchronous path surfaced MCP failures visibly during config.initialize().
Impact: Silent regression — CI/script users get wrong behavior with no error message. Debug requires enabling debug logging.
| this.debugLogger.error( | |
| .catch((err: unknown) => { | |
| const msg = `Background MCP discovery failed: ${err instanceof Error ? err.message : String(err)}`; | |
| this.debugLogger.error(msg); | |
| console.error(`Warning: ${msg}`); | |
| }); |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
There was a problem hiding this comment.
Real silent regression — fixed in a49b3e2, but I took a slightly different approach than the in-line .catch you suggested. Here's why:
The outer .catch() on discoverAllMcpToolsIncremental(this) is unreachable in the all-servers-fail case. Per-server connect() / discover() failures are caught inside discoverAllMcpToolsIncremental's discoveryPromises.map(...) per-server try/catch (mcp-client-manager.ts ~line 511), which records mcp_server_ready:<name>:failed and continues. The outer promise resolves successfully even when every server failed.
Adding console.error to the outer .catch would never fire — confirmed by inspection of the per-server catch block.
Instead I added Config.getFailedMcpServerNames() which inspects getMCPServerStatus(name) !== CONNECTED for every non-disabled configured server, and the non-interactive entry points (gemini.tsx --prompt path, Session.ensureConfigInitialized for stream-json, acpAgent.runAcpAgent) emit a single user-visible stderr warning after waitForMcpReady() returns. Per-config-test coverage in config.test.ts.
Example output:
Warning: MCP server(s) failed to start: github, jira. Continuing with built-in tools and any servers that did connect. Re-run with QWEN_CODE_DEBUG=1 to see per-server reasons.
|
|
||
| const onMcpUpdate = () => { | ||
| scheduleFlush(); | ||
| if (manager.getDiscoveryState() === MCPDiscoveryState.COMPLETED) { |
There was a problem hiding this comment.
[Suggestion] gemini_tools_lag metric non-deterministically missing
When mcp-client-update fires with COMPLETED, onMcpUpdate calls finalizeOnce() synchronously (line 585), setting finalized = true BEFORE the 16ms batch flush timer fires setTools(). recordStartupEvent('gemini_tools_updated') is dropped because it returns early when finalized is true. The gemini_tools_lag derived phase becomes undefined when no post-MCP event is captured. Whether it IS captured depends on per-server timing vs COMPLETED arrival.
Impact: The PR's advertised performance metric is unreliable in interactive mode.
Suggested fix: Defer finalizeOnce() until after the batch flush completes, or call setTools() + finalize atomically.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
There was a problem hiding this comment.
Confirmed — fixed in 6dcea68. You're right that the previous onMcpUpdate had a finalize-before-flush ordering bug. New code:
const onMcpUpdate = () => {
if (manager.getDiscoveryState() === MCPDiscoveryState.COMPLETED) {
// Flush setTools() NOW (rather than the 16ms timer) and only
// finalize after it runs — setTools() emits gemini_tools_updated,
// and finalizing before it fires would drop that event because
// the module-level `finalized` guard suppresses every subsequent
// record. That dropped event is what gemini_tools_lag is derived
// from.
void flushNow().finally(finalizeOnce);
} else {
scheduleFlush();
}
};flushNow() returns a Promise so we can chain finalize via .finally. This guarantees gemini_tools_updated is recorded BEFORE finalized = true, so gemini_tools_lag will reliably appear in the derived phases.
| }, | ||
| ); | ||
| // Records the moment Ink has produced its first frame. AppContainer's mount | ||
| // effect runs after this — it carries the `config_initialize_*` and |
There was a problem hiding this comment.
[Suggestion] first_paint checkpoint fires before actual Ink paint
profileCheckpoint('first_paint') is placed immediately after render() (line 332). Ink's render() returns synchronously but terminal output happens asynchronously through React reconciliation. The checkpoint fires before any pixels reach the terminal, making the to_first_paint derived phase artificially low.
Suggested fix: Rename to render_call_returned and update the comment, or hook into Ink's actual first-frame callback if available.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
There was a problem hiding this comment.
Agreed it's a semantic mismatch. Kept the checkpoint name first_paint for backward compatibility with previously-collected profile files (analysis tooling references the name), but the comment is now explicit:
// Records the moment Ink's `render()` call has returned, which is
// synchronous and happens before React reconciliation actually pushes
// bytes to the terminal. We intentionally keep the legacy name
// `first_paint` for backward compatibility with previously-collected
// profile files; the value is best read as "render call returned"
// rather than literal pixel paint.If we want a true first-frame hook, Ink's useApp().rerender callback or stdout drain detection would work, but neither is exposed today and the value of first_paint is already useful as an upper bound for "time to render call returned". Open to revisiting in a follow-up if the gap matters in practice.
Addresses review feedback on PR #3994: - Skip user-disabled servers in discoverAllMcpToolsIncremental. The new incremental path used to iterate Object.entries(servers) without consulting isMcpServerDisabled, so a server the user had explicitly turned off would still get connected and its tools registered. Mirrors the existing protection in discoverAllMcpTools. - Disconnect the underlying client when runWithDiscoveryTimeout fires. Without this, the inner discoverMcpToolsForServer kept running after the timeout rejected the outer promise — if discover() eventually succeeded it would register the late server's tools into the live toolRegistry (a silent registration vector, especially exploitable with a 0/negative discoveryTimeoutMs override). - Clamp discoveryTimeoutMs to [100ms, 300_000ms]. 0/negative/Infinity values previously passed through to setTimeout unvalidated and made the silent-registration bug above trivially reachable. - Classify the `tcp` (WebSocket) transport field as remote so hung WS handshakes use the 5s default instead of the 30s stdio default. - Defensive delete of serverDiscoveryPromises[name] in the per-server catch so a doomed/orphan entry can't briefly short-circuit a subsequent discoverMcpToolsForServer call. Adds focused tests for each fix. Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
… visibility Addresses review feedback on PR #3994: - Restore writeRuntimeStatus + markRuntimeStatusEnabled in startInteractiveUI. The progressive-MCP diff inadvertently dropped the runtime.json sidecar write from the interactive entry point, leaving Config.refreshSessionId()'s session-swap refresh as dead code and silently breaking external integrations (terminal multiplexers, IDE integrations, status daemons) that map PID → sessionId via runtime.json. - Add Config.getFailedMcpServerNames() and surface a stderr warning in --prompt / stream-json / ACP entry points when one or more MCP servers failed during background discovery. Per-server errors are caught inside discoverAllMcpToolsIncremental and never reached a TTY otherwise, so a script using non-interactive mode with broken MCP config would silently run with only built-in tools — a regression vs the legacy synchronous path. - Pass the parsed `settings` object through to runNonInteractiveStreamJson. The new call site dropped the argument, falling back to createMinimalSettings() and losing any user-configured permission / approval / hook setup for stream-json sessions. Added regression assertion to gemini.test.tsx. - Move finalizeStartupProfile out of gemini.tsx's stream-json branch and into Session.ensureConfigInitialized so it runs AFTER config.initialize() / waitForMcpReady() in stream-json. Previously the profile was finalized before any MCP / config_initialize_* events were emitted, producing empty stream-json profiles. - Gate setStartupEventSink registration on isStartupProfilerEnabled() so core-side recordStartupEvent calls short-circuit at the first null-check when profiling is disabled, instead of going through an arrow wrapper and the profiler's own enabled gate. - Tighten the type-unsafe ToolRegistry cast in startMcpDiscoveryInBackground to preserve the typed return signature so a rename of getMcpClientManager would be flagged at this call site (kept the optional-chain guard for tests that stub ToolRegistry as a plain object). - Re-document first_paint as "render call returned" so consumers don't confuse Ink's synchronous render() return with literal pixel paint. Kept the checkpoint name for backward compatibility with collected profiles. Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
…AppContainer Addresses review feedback on PR #3994: - Restore the terminal-resize useEffect that calls repaintStaticViewport() when terminalWidth changes. The progressive- MCP diff removed previousTerminalWidthRef + the repaint useCallback + the resize useEffect, so tmux pane resizes and fullscreen toggles leave the static region rendered at the old width — header content visibly tears until something else triggers refreshStatic. - Pin the gemini_tools_lag startup metric. The previous onMcpUpdate handler called finalizeOnce() synchronously when discovery reached COMPLETED, but the pending setTools() batch was still 16ms away. setTools() emits `gemini_tools_updated` — when finalize ran first the profile's `finalized` guard suppressed that event, so gemini_tools_lag came out undefined in interactive mode. New onMcpUpdate flushes setTools() NOW on COMPLETED and only finalizes after the flush resolves, guaranteeing the event lands. - Log setTools() batch-flush errors via debugLogger instead of silently swallowing them. GeminiClient.setTools() has no try/catch around warmAll() / getFunctionDeclarations() / getChat().setTools(); the previous `.catch(() => {})` would have hidden production tool-registration regressions completely. Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
@wenshao — addressed all four rounds of review feedback. Summary of dispositions across the 16 substantive inline comments (skipped the explicit Fixes landed[Critical] All 5 accepted, fixed in 3 commits. All confirmed by direct code-path trace.
Deferred with reasoning
CI
Verified locally: typecheck clean on both packages; affected vitest suites (mcp-client-manager 16, config 140, gemini 14, session 27, acpAgent 34, AppContainer 61, startupProfiler 19, nonInteractiveCli 42) all pass. Ready for re-review when you have a moment. |
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Additional finding not mappable to a single diff line:
[Critical] McpClient.discover() in packages/core/src/tools/mcp-client.ts:169 lacks error status update — McpClient.connect() calls this.updateStatus(MCPServerStatus.DISCONNECTED) on error, but McpClient.discover() never calls updateStatus() on success or failure. If discoverTools or discoverPrompts throw (e.g., server crashes mid-discovery), the status stays CONNECTED. Since Config.getFailedMcpServerNames() (new in this PR) checks status !== CONNECTED, such servers won't appear in the failure list — silent data loss. Fix: wrap the body of discover() in try/catch and call this.updateStatus(MCPServerStatus.DISCONNECTED) on error.
Also see the 3 inline comments below for the other findings.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| serverName: name, | ||
| }); | ||
| } | ||
| recordStartupEvent(`mcp_server_ready:${name}`, { outcome: 'ready' }); |
There was a problem hiding this comment.
[Critical] mcp_server_ready incorrectly reports outcome: 'ready' for failed connections
discoverMcpToolsForServerInternal catches all errors from connect()/discover() without re-throwing, so the try block in discoverAllMcpToolsIncremental always succeeds — unconditionally recording mcp_first_tool_registered and outcome: 'ready' events. Only runWithDiscoveryTimeout rejections reach the catch block. Any non-timeout failure (auth error, server crash, missing tools) is incorrectly recorded as success in the startup profile.
| recordStartupEvent(`mcp_server_ready:${name}`, { outcome: 'ready' }); | |
| const client = this.clients.get(name); | |
| const actuallyReady = | |
| client && | |
| getMCPServerStatus(name) === MCPServerStatus.CONNECTED; | |
| if (actuallyReady) { | |
| if (!firstToolEventFired) { | |
| firstToolEventFired = true; | |
| recordStartupEvent('mcp_first_tool_registered', { | |
| serverName: name, | |
| }); | |
| } | |
| recordStartupEvent(`mcp_server_ready:${name}`, { | |
| outcome: 'ready', | |
| }); | |
| } else { | |
| recordStartupEvent(`mcp_server_ready:${name}`, { | |
| outcome: 'failed', | |
| reason: 'connection or discovery error', | |
| }); | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
There was a problem hiding this comment.
Fixed in 0996ee8. You're right — discoverMcpToolsForServerInternal swallows connect/discover errors without re-throwing (best-effort discovery), so the try block here resolved for every failure except the timeout path. The fix consults the actual server status (now correctly DISCONNECTED after the McpClient.discover fix above) before recording outcome: 'ready', and emits outcome: 'failed' otherwise. The mcp_first_tool_registered event is gated on the same check so a failed server can't pollute that user-facing metric either. Test added: discoverAllMcpToolsIncremental records 'failed' outcome for swallowed connect errors.
| // see it reconnected by the incremental path. Without this, the | ||
| // PR-A background path silently re-registers tools the user has | ||
| // told us to ignore. | ||
| if (cliConfig.isMcpServerDisabled(name)) { |
There was a problem hiding this comment.
[Suggestion] discoverAllMcpToolsIncremental fails to clean up enabled→disabled server transitions
When a server is disabled, the loop skips it with continue but does not remove its existing client (this.clients), registered tools (ToolRegistry), or health checks. The removeServer() method should be called for newly-disabled servers to ensure state consistency. Same issue at line 102 in connectAndDiscover. The synchronous discoverAllMcpTools handles this correctly by calling this.stop() first.
| if (cliConfig.isMcpServerDisabled(name)) { | |
| if (cliConfig.isMcpServerDisabled(name)) { | |
| debugLogger.debug(`Skipping disabled MCP server: ${name}`); | |
| if (this.clients.has(name)) { | |
| this.removeServer(name); | |
| } | |
| continue; | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
There was a problem hiding this comment.
Fixed in 0996ee8. Agreed — the previous code just continued past disabled servers, so a mid-session enabled→disabled transition (e.g. via /mcp disable foo or by editing settings) left the existing client, registered tools, health check, and global status entry in place. The disabled branch now calls removeServer(name) for any already-known client, which disconnects, drops the client entry, removes the server's tools from the registry, stops the health check, and removes the global status. Test added: discoverAllMcpToolsIncremental tears down enabled→disabled transitions. Note: I did NOT mirror this into connectAndDiscover at line ~102 — that's the legacy synchronous discoverAllMcpTools path, which already calls this.stop() at the top (line 89) and so already starts from a clean slate. The bug was specific to the incremental path.
| */ | ||
| private startMcpDiscoveryInBackground(): void { | ||
| // `getMcpClientManager` is a public method on `ToolRegistry` (added | ||
| // alongside this PR), so we call it directly — no defensive cast. |
There was a problem hiding this comment.
[Suggestion] Comment claims "no defensive cast" but code still uses a type assertion with optional chaining
The comment says getMcpClientManager is called directly without a defensive cast, but the code still uses as ToolRegistry & { getMcpClientManager?: ... } with ?.(). Since getMcpClientManager is a public method on ToolRegistry, the cast is technically unnecessary — it exists only for test compatibility where ToolRegistry is stubbed as a plain object. Update the comment to explain the real reason.
| // alongside this PR), so we call it directly — no defensive cast. | |
| // Tests stub `toolRegistry` as a plain object, so we guard with | |
| // optional chaining to avoid crashing the production path. | |
| const manager = ( | |
| this.toolRegistry as ToolRegistry & { | |
| getMcpClientManager?: () => ReturnType<ToolRegistry['getMcpClientManager']>; | |
| } | |
| ).getMcpClientManager?.(); |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
There was a problem hiding this comment.
Fixed in d20bbf9 (comment-only). You're right that the existing wording contradicted the code — the cast remains, just for a different reason than the cast it replaced in a49b3e2. The new comment makes the test-stub motivation explicit and also notes that the inner ReturnType<ToolRegistry['getMcpClientManager']> shape means a future rename of getMcpClientManager on ToolRegistry still surfaces here as a type error (rather than silently falling through to if (!manager) return like the previous hand-rolled { discoverAllMcpToolsIncremental: ... } shape would have). I went slightly more verbose than your suggested wording to retain that point — happy to trim if you prefer the shorter version.
Addresses three review findings on PR #3994: - McpClient.discover() now flips the client status to DISCONNECTED before re-throwing. Previously, a server that connected successfully but whose discoverPrompts / discoverTools then rejected (or that returned no prompts and no tools) would remain CONNECTED in the global status registry. Config.getFailedMcpServerNames() filters by `status !== CONNECTED`, so such servers were silently omitted from the non-interactive failure banner and the Footer's MCP health pill kept counting them as healthy. - discoverAllMcpToolsIncremental no longer records `outcome: 'ready'` for servers whose connect/discover threw. The inner discoverMcpToolsForServerInternal catches errors without re-throwing (best-effort discovery semantics), so the try block resolved even for failures — only the runWithDiscoveryTimeout path reached the catch. Auth errors, server crashes, and missing-tools responses were therefore recorded as success in the startup profile. We now consult the actual server status (now correctly DISCONNECTED after the first fix) before emitting `ready`, and emit `outcome: 'failed'` otherwise. `mcp_first_tool_registered` is gated on the same check so a failed server can't pollute that user-facing metric. - discoverAllMcpToolsIncremental tears down enabled→disabled mid-session transitions. When a previously-connected server is disabled (e.g. via `/mcp disable foo` or by editing settings), the incremental path used to just `continue` past it, leaving its client, tools, health check, and global status entry in place. Now calls removeServer() for any already-known client we encounter in the disabled branch. Adds focused tests for each fix. Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
…ackground Addresses review feedback on PR #3994. The previous comment claimed the call site uses "no defensive cast" but the code still casts via `as ToolRegistry & { getMcpClientManager?: ... }`. Reword to explain the cast's actual purpose: it exists only because some tests stub ToolRegistry as a plain object, so we use optional chaining to avoid crashing the init path when those tests run. Also note that the inner shape now uses `ReturnType<ToolRegistry['getMcpClientManager']>` — a future rename of the production method still surfaces as a type error at this call site rather than silently falling through to the `if (!manager)` branch. Comment-only change; no behavior diff. Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
Re: top-level [Critical] on The fix wraps the body of Test added in mcp-client.test.ts: |
|
All round-5 feedback addressed:
These 4 items are NEW findings against the post-round-4 codebase (no overlap with prior fixes in 6b07a85 / a49b3e2 / 6dcea68). mcp-client / mcp-client-manager / config tests all green (35+18+140). Ready for re-review. |
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
[Suggestion] Duplicate MCP failure warning logic across 3 call sites — packages/cli/src/acp-integration/acpAgent.ts, packages/cli/src/gemini.tsx, packages/cli/src/nonInteractive/session.ts. The same getFailedMcpServerNames() + stderr.write pattern is repeated with near-identical logic. Extract into a shared helper on Config or as an importable utility to avoid future drift in message format, throttling, or log-target changes.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| // ever reach the registry. | ||
| const client = this.clients.get(serverName); | ||
| if (client) { | ||
| void client.disconnect().catch((err) => { |
There was a problem hiding this comment.
[Critical] TOCTOU race: fire-and-forget disconnect() allows tools to register after timeout fires
The timeout handler calls void client.disconnect().catch(...) without await, then immediately reject(). Since disconnect() is async (awaits transport.close()), there is a window where connect() has already completed and discover() is mid-flight registering tools — those tools land in the registry after timeout. The code comment already labels this as a "remote-exploitable silent-tool-registration vector" but the fix is incomplete.
| void client.disconnect().catch((err) => { | |
| await client.disconnect(); | |
| reject(new TimeoutError(`MCP server '${serverName}' discovery timed out after ${timeoutMs}ms`)); |
or, after disconnect completes, remove the server's tools from toolRegistry.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
There was a problem hiding this comment.
Fixed in b6e97d9. Switched the timeout handler to await client.disconnect() so transport.close() lands before reject, and added this.toolRegistry.removeMcpToolsByServer(serverName) afterwards to drop any tools that did slip into the registry during the disconnect window (your second suggestion). Together they close the silent-tool-registration race: even if discover() synchronously pumps tools/list into the registry just before the transport closes, the cleanup call removes them.
| this.updateStatus(MCPServerStatus.DISCONNECTED); | ||
| throw error; | ||
| } | ||
| } |
There was a problem hiding this comment.
[Critical] isDisconnecting flag prevents updateStatus(DISCONNECTED) from reaching the global registry for timeout-ed servers
When the timeout callback in runWithDiscoveryTimeout calls client.disconnect(), McpClient.disconnect() sets this.isDisconnecting = true (line 204) before calling this.updateStatus(DISCONNECTED) (line 209). However, updateStatus() has a guard at line 246: if (this.isDisconnecting) { return; } that skips updateMCPServerStatus() — so the global status registry never receives the DISCONNECTED update.
This affects the discover() catch block at this line too: after disconnect() tears down the transport, the in-flight connect()/discover() throws, and this.updateStatus(DISCONNECTED) fires here — but the global update is silently swallowed. The server stays CONNECTED in the global registry forever. Config.getFailedMcpServerNames() — which filters on status !== CONNECTED — never reports it.
| } | |
| // In McpClient.disconnect(), move updateStatus before isDisconnecting: | |
| this.updateStatus(MCPServerStatus.DISCONNECTED); | |
| this.isDisconnecting = true; |
or, in the timeout callback, call updateMCPServerStatus(name, DISCONNECTED) directly on the global registry before client.disconnect().
— DeepSeek/deepseek-v4-pro via Qwen Code /review
There was a problem hiding this comment.
Fixed in b6e97d9. McpClient.disconnect() now writes MCPServerStatus.DISCONNECTED to the global registry directly (via updateMCPServerStatus) BEFORE setting isDisconnecting = true, so the intentional disconnect notification can't be swallowed by the guard whose only purpose is to block stale connect() catch updates. The local this.status is also flipped first so a concurrent discover() reading it doesn't proceed to register tools. The round-5 getMCPServerStatus === CONNECTED gate now correctly distinguishes ready vs failed because timeout-disconnected servers actually reach DISCONNECTED in the registry. Added a regression test in mcp-client.test.ts.
| if (prompts.length === 0 && tools.length === 0) { | ||
| throw new Error('No prompts or tools found on the server.'); | ||
| } | ||
|
|
There was a problem hiding this comment.
[Suggestion] Healthy MCP servers with 0 tools/prompts are reported as "failed"
McpClient.discover() throws "No prompts or tools found on the server." when both prompts and tools arrays are empty. The surrounding try/catch at this line calls updateStatus(DISCONNECTED), causing getFailedMcpServerNames() to include the server in the "Warning: MCP server(s) failed to start" banner. A server that connects successfully but exposes no tools (e.g., a pure resource server) is healthy — consider keeping its status as CONNECTED with 0 tools.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
There was a problem hiding this comment.
Declining for this PR. The if (prompts.length === 0 && tools.length === 0) throw check predates this PR — git log on packages/core/src/tools/mcp-client.ts shows it was introduced as part of an earlier refactor and we only re-arranged the surrounding try/catch in round 5. Loosening it to treat resource-only servers as healthy would change the getFailedMcpServerNames() semantics that the Footer health pill and non-interactive failure banner depend on; that belongs in a focused follow-up rather than this progressive-discovery PR. Happy to take that on as a separate change if you file it.
| `Continuing with built-in tools and any servers that did connect.\n`, | ||
| ); | ||
| } | ||
| // Finalize the startup profile here so `config_initialize_*` and the |
There was a problem hiding this comment.
[Suggestion] Stream-json path is missing config_initialize_start / config_initialize_end profiler checkpoints
Session.ensureConfigInitialized() calls config.initialize() directly without wrapping it in profileCheckpoint('config_initialize_start') / profileCheckpoint('config_initialize_end'), unlike the interactive and non-stream-json paths in gemini.tsx. The comment here acknowledges the timing difference but doesn't address the missing config_initialize_dur derived phase in stream-json session profiles. Consider adding the checkpoints (gated on isStartupProfilerEnabled()) for consistent profiling across all modes.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
There was a problem hiding this comment.
Done in b6e97d9. Added profileCheckpoint('config_initialize_start') / _end around this.config.initialize(options) in Session.ensureConfigInitialized(), mirroring the non-stream-json branch in gemini.tsx so the config_initialize_dur derived phase shows up consistently across all modes. profileCheckpoint is a no-op when QWEN_CODE_PROFILE_STARTUP is unset so this adds zero overhead off the profiling path.
Code ReviewOverviewThis PR replaces synchronous MCP discovery during Strengths
Issues / suggestionsBlocking-ish
Non-blocking
Test coverageSolid. Highlights:
Worth adding:
Security
No new attack surface I can see. The Risk assessment
RecommendationApprove with the import ordering and |
Addresses two critical findings on PR #3994 round 6: - runWithDiscoveryTimeout no longer uses fire-and-forget disconnect. The prior `void client.disconnect()` returned before `transport.close()` landed, leaving a window where an in-flight `discover()` could pump `tools/list` through the transport and synchronously register tools into the live registry BEFORE the close took effect. The earlier fix comment described this as a "remote-exploitable silent-tool-registration vector"; the await closes the timing window but doesn't help if tools already landed, so we also drop them with `removeMcpToolsByServer()` after the disconnect resolves. No-op when discover hadn't reached registration yet. - McpClient.disconnect() now writes DISCONNECTED to the global registry directly. Previously, `isDisconnecting = true` was set BEFORE the internal `updateStatus(DISCONNECTED)` call, and `updateStatus`'s guard (designed to suppress LATE writes from a stale `connect()` catch) silently swallowed the write. The global stayed CONNECTED forever for timeout-disconnected servers, so `Config.getFailedMcpServerNames()` (which filters `status !== CONNECTED`) omitted them from the non-interactive failure banner and the Footer's MCP health pill kept counting them as healthy. This invalidated the round-5 `getMCPServerStatus === CONNECTED` gate, which would always pass the "ready" check for timed-out servers. The guard stays in place for its original purpose; the legitimate disconnect→DISCONNECTED notification now bypasses it by writing the registry directly. Also adds the `config_initialize_start` / `_end` profiler checkpoints to `Session.ensureConfigInitialized()` so stream-json startup profiles include the same derived `config_initialize_dur` phase as the non-stream-json branch in gemini.tsx (round 6 [Suggestion]). Tests cover (a) the disconnect-and-cleanup path on timeout and (b) the intentional-disconnect global registry propagation regression. Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
Round 6 follow-ups in b6e97d9:
Tests cover both critical paths ( Windows CI failure: the failing tests ( |
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
[Critical] AppContainer MCP batch-flush useEffect untested (packages/cli/src/ui/AppContainer.tsx:526-628)
The ~100-line useEffect that implements 16ms batch-flushed setTools() + deferred profile finalize + mcp-client-update subscription — the core wiring for interactive progressive MCP availability — has no component-level tests. AppContainer.test.tsx has zero matches for MCP_BATCH, mcp-client-update, onMcpUpdate, or flushNow.
Suggested fix: Add AppContainer.test.tsx cases covering: batch debouncing, COMPLETED→immediate flush+finalize, 35s cap fallback, cleanup removes listeners/timers.
Test coverage gaps (Suggestion):
config.ts:1297-1344—startMcpDiscoveryInBackgroundsetTools()call and error propagationsession.ts:146-180—ensureConfigInitializedwaitForMcpReady,getFailedMcpServerNames, profile checkpointsgemini.tsx:396-401—setStartupEventSinkbridge gating logicconfig.ts:1400-1414—getFailedMcpServerNamesDISCONNECTED/CONNECTING/mixed statesmcp-client-manager.ts:658-674—discoveryTimeoutForhttpUrl/url/non-finite edge casesmcp-client-manager.ts:585-637—runWithDiscoveryTimeoutnon-timeout success/failure pathsclient.ts:366—gemini_tools_updatedevent emission insetTools()
| }; | ||
|
|
||
| const onMcpUpdate = () => { | ||
| if (manager.getDiscoveryState() === MCPDiscoveryState.COMPLETED) { |
There was a problem hiding this comment.
[Critical] Interactive mode silently ignores MCP startup failures
Non-interactive paths (gemini.tsx:822, session.ts:167, acpAgent.ts:100) all call getFailedMcpServerNames() and emit Warning: MCP server(s) failed to start to stderr. The interactive AppContainer's onMcpUpdate callback has no equivalent logic. MCP failures are routed only to debugLogger (debug-mode only) and profile events (profiling-mode only) — regular interactive users see a normal UI with zero indication that their configured MCP servers failed.
Impact: Users with broken MCP servers get no visible error. The only clue is unexpected model behavior (missing tools). Extremely hard to debug without any error output.
| if (manager.getDiscoveryState() === MCPDiscoveryState.COMPLETED) { | |
| // In onMcpUpdate, when state transitions to COMPLETED: | |
| if (manager.getDiscoveryState() === MCPDiscoveryState.COMPLETED) { | |
| const failedNames = config.getFailedMcpServerNames(); | |
| if (failedNames.length > 0) { | |
| debugLogger.warn( | |
| `Warning: MCP server(s) failed to start: ${failedNames.join(', ')}. ` + | |
| `Continuing with built-in tools and any servers that did connect.`, | |
| ); | |
| } | |
| // ... existing flushNow + finalizeOnce logic | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
There was a problem hiding this comment.
Fixed in aaea392. The interactive now mirrors the non-interactive stderr warning — adjusted to debugLogger.warn instead of stderr because Ink owns stdout/stderr for the interactive UI and direct writes would collide with the rendered frame. Visible under QWEN_CODE_DEBUG=1 and in the debug log file, matching the channel setTools() errors already use. The surfaceFailuresOnce guard ensures the warning fires exactly once per discovery cycle (both the live onMcpUpdate COMPLETED branch and the legacy synchronous-COMPLETED branch). The MCP status footer pill continues to surface failures continuously in the UI; this log is the actionable-on-debug record.
There was a problem hiding this comment.
(Correcting the previous reply — shell ate one of the backticks.)
Fixed in aaea392. The interactive AppContainer now mirrors the non-interactive stderr warning — adjusted to debugLogger.warn instead of stderr because Ink owns stdout/stderr for the interactive UI and direct writes would collide with the rendered frame. Visible under QWEN_CODE_DEBUG=1 and in the debug log file, matching the channel setTools() errors already use. The surfaceFailuresOnce guard ensures the warning fires exactly once per discovery cycle (both the live onMcpUpdate COMPLETED branch and the legacy synchronous-COMPLETED branch). The MCP status footer pill continues to surface failures continuously in the UI; this log is the actionable-on-debug record.
| await config.initialize(); | ||
| // Same reasoning as the top-level runAcpAgent path: ACP feeds session | ||
| // messages to the model immediately, so we cannot return a Config whose | ||
| // MCP discovery is still in flight. |
There was a problem hiding this comment.
[Critical] ACP per-session MCP failures silently fall back to built-in-tools-only
QwenAgent.initializeConfig() (per-session ACP path) calls await config.waitForMcpReady() but does NOT call config.getFailedMcpServerNames() or emit any warning — despite the comment claiming "Same reasoning as the top-level runAcpAgent path." The top-level runAcpAgent path (lines 95-107) properly emits Warning: MCP server(s) failed to start to stderr.
Impact: ACP integrations using per-session Config instances silently lose MCP tools with zero user-visible indication. This is inconsistent with the top-level path and makes ACP MCP issues undebuggable without QWEN_CODE_DEBUG=1.
| // MCP discovery is still in flight. | |
| await config.waitForMcpReady(); | |
| const failedMcpServers = | |
| typeof config.getFailedMcpServerNames === 'function' | |
| ? config.getFailedMcpServerNames() | |
| : []; | |
| if (failedMcpServers.length > 0) { | |
| process.stderr.write( | |
| `Warning: MCP server(s) failed to start: ${failedMcpServers.join(', ')}. ` + | |
| `Continuing with built-in tools and any servers that did connect.\n`, | |
| ); | |
| } | |
| return config; |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
There was a problem hiding this comment.
Fixed in aaea392 — applied the suggestion almost verbatim (kept the existing defensive typeof config.getFailedMcpServerNames === 'function' guard so tests that stub Config don't break). Per-session QwenAgent.initializeConfig() now emits the same Warning: MCP server(s) failed to start stderr line as the top-level runAcpAgent path. Added two regression tests in acpAgent.test.ts: (1) failures surface to stderr listing every failed server name, (2) stubbed Configs without getFailedMcpServerNames don't throw and don't emit a warning.
| // Drop any tools that registered during the disconnect window. No-op | ||
| // if the server hadn't reached `discover()` yet, so it's safe to | ||
| // always call. | ||
| this.toolRegistry.removeMcpToolsByServer(serverName); |
There was a problem hiding this comment.
[Suggestion] Health check silently reconnects MCP servers that were intentionally timed out
discoverMcpToolsForServerInternal's finally block (line 235) unconditionally calls this.startHealthCheck(serverName), even after the timeout handler has disconnected the client. The health check detects DISCONNECTED status and after ~90 seconds triggers reconnectServer(), which calls discoverMcpToolsForServer() directly — with no per-server discovery timeout guard. A server intentionally timed out for being slow will silently reconnect later, re-introducing the same pathology this PR was designed to fix.
The timeout handler here correctly calls removeMcpToolsByServer() but does not remove the client from this.clients Map or stop its health check, allowing the finally block's startHealthCheck to create a timer that will later reconnect the server.
| this.toolRegistry.removeMcpToolsByServer(serverName); | |
| this.toolRegistry.removeMcpToolsByServer(serverName); | |
| // Prevent the finally block's startHealthCheck from resurrecting | |
| // this server: remove from clients so performHealthCheck is a no-op. | |
| this.clients.delete(serverName); | |
| reject( |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
There was a problem hiding this comment.
Fixed in aaea392. Good catch — this was indeed the round-6 fix's missing other half. The timeout handler now does three things instead of two: (a) await disconnect, (b) removeMcpToolsByServer, (c) stopHealthCheck + this.clients.delete(serverName). The (c) step makes performHealthCheck early-return (no client) AND ensures the finally block's startHealthCheck is now also guarded — I tightened startHealthCheck to no-op when !this.clients.has(serverName) so the trailing call is harmless even if future code adds another path to it. Regression test in mcp-client-manager.test.ts: runWithDiscoveryTimeout drops the client + stops health-check so the auto-reconnect loop cannot resurrect an intentionally timed-out server — verifies both the clients-map and healthCheckTimers-map are purged after timeout.
| */ | ||
| export function recordStartupEvent( | ||
| name: string, | ||
| attrs?: StartupEventAttrs, |
There was a problem hiding this comment.
[Suggestion] Silent catch {} swallows all profiler sink exceptions with no logging
The empty catch {} block silently drops all exceptions from the registered sink. If the sink throws (e.g., file write failure, corrupted internal state), profiling data is silently lost with zero debugging visibility — the worst kind of silent failure mode for an observability tool.
| attrs?: StartupEventAttrs, | |
| } catch (err) { | |
| // Profiler sinks must never throw into hot paths, but we log failures. | |
| if (typeof process !== 'undefined' && process.stderr) { | |
| process.stderr.write( | |
| `[startup-profiler] event sink error: ${String(err)}\n`, | |
| ); | |
| } | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
There was a problem hiding this comment.
Fixed in aaea392. Routed through debugLogger.error instead of process.stderr.write — same reasoning as the AppContainer interactive surfacing fix in the same commit: a direct stderr write here would leak to interactive users' terminals (the sink is registered when QWEN_CODE_PROFILE_STARTUP=1 is set, which is a profile/debug mode but doesn't necessarily imply the user wants stderr noise during normal runs). debugLogger is quiet by default, visible under QWEN_CODE_DEBUG=1, and written to the debug log file — matches how other "must never throw" sites in this PR (AppContainer's setTools flush, runWithDiscoveryTimeout disconnect errors) surface failures. The existing "does not bubble sink exceptions into hot paths" test still pins the contract.
…ed-out servers
Round-7 review follow-ups:
- AppContainer (interactive): MCP startup failures now route through
debugLogger.warn on COMPLETED. Was silent — only debug logs / profile
events surfaced failures, so regular interactive users got no
indication their MCP servers failed. Mirrors the non-interactive
stderr warning, adjusted to debugLogger so it doesn't collide with
Ink's rendered output.
- acpAgent per-session: `QwenAgent.initializeConfig()` now emits the
same `Warning: MCP server(s) failed to start` stderr line as the
top-level `runAcpAgent` path. Previously per-session ACP configs
with failed MCP servers silently fell back to built-in tools.
- mcp-client-manager timeout handler: after disconnecting an
intentionally timed-out server, also drop it from `this.clients` and
stop any pending health-check timer. Without this the discovery
`finally` block would arm a health-check that detected DISCONNECTED
status and called `reconnectServer()` → `discoverMcpToolsForServer()`
directly — bypassing `runWithDiscoveryTimeout` entirely and silently
resurrecting the slow server. `startHealthCheck` also early-returns
for unknown servers so the trailing finally-block call is a no-op.
- startupEventSink: silent `catch {}` now logs via `debugLogger.error`
so a corrupted sink doesn't silently drop every subsequent event.
Quiet by default; visible under `QWEN_CODE_DEBUG=1`.
Tests:
- mcp-client-manager.test.ts: regression for the timeout → no-reconnect
invariant (clients map purged + health-check timer absent).
- acpAgent.test.ts: per-session newSession surfaces failures to stderr,
and stays safe when Config lacks `getFailedMcpServerNames`.
Declines (with reasoning in PR reply):
- [Critical] AppContainer batch-flush useEffect untested → re-flag of
the round-5 deferral that wenshao acknowledged at the time. Lower-
layer invariants (this PR's mcp-client-manager + mcp-client tests)
pin the dependent contracts. The component-test harness for timers +
event emitters in this file is non-trivial and out of scope; tracked
for a follow-up.
Generated with AI
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
Round 7 dispositions (commit aaea392): Fixed (3)
Declined (1)
Test-coverage suggestionsAcknowledged. Most map to lower-layer files that already have coverage ( typecheck clean; mcp-client-manager (19), mcp-client (36), startupEventSink (4), AppContainer (61), acpAgent (36, +2 new), config (140), gemini (14), session (27) all green. |
| // eventually responds, `discover()` registers its tools into | ||
| // the live `toolRegistry` and re-emits `mcp-client-update`. | ||
| // From the user's perspective the server "failed" but its tools | ||
| // are silently active, including any that shadow built-ins. |
There was a problem hiding this comment.
[Suggestion] runWithDiscoveryTimeout timeout handler can hang if disconnect() blocks
The timeout callback executes await client.disconnect(), which internally calls await this.transport.close(). If the transport is unresponsive (which is precisely why the timeout fired — e.g., a hung stdio child process), close() may never return, causing reject() to never be called. This cascades: Promise.all(discoveryPromises) hangs → waitForMcpReady() hangs → non-interactive CLI silently blocks forever.
| // are silently active, including any that shadow built-ins. | |
| // Move reject() BEFORE await disconnect(), and make the cleanup fire-and-forget: | |
| const client = this.clients.get(serverName); | |
| reject(new Error(`MCP server '${serverName}' discovery timed out after ${timeoutMs}ms`)); | |
| if (client) { | |
| client.disconnect().catch(() => {}); // fire-and-forget cleanup | |
| } | |
| this.toolRegistry.removeMcpToolsByServer(serverName); | |
| this.stopHealthCheck(serverName); | |
| this.clients.delete(serverName); |
Alternatively, wrap disconnect() in a Promise.race with a short (e.g., 2s) safety timeout.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
|
|
||
| this.discoveryState = MCPDiscoveryState.IN_PROGRESS; | ||
| recordStartupEvent('mcp_discovery_start', { | ||
| serverCount: Object.keys(servers).length, |
There was a problem hiding this comment.
[Suggestion] serverCount in profiler events includes disabled servers
recordStartupEvent('mcp_discovery_start', { serverCount: Object.keys(servers).length }) counts all configured servers including those with disabled: true. Disabled servers are filtered out later in the per-server loop, so the reported count inflates metrics and skews per-server averages in downstream dashboards.
| serverCount: Object.keys(servers).length, | |
| const enabledServerNames = Object.entries(servers).filter( | |
| ([name]) => !cliConfig.isMcpServerDisabled(name), | |
| ); | |
| recordStartupEvent('mcp_discovery_start', { | |
| serverCount: enabledServerNames.length, | |
| incremental: true, | |
| }); |
The same fix applies to the mcp_all_servers_settled event (~line 584).
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| // server response time. Users can opt back into the legacy synchronous | ||
| // behavior with `QWEN_CODE_LEGACY_MCP_BLOCKING=1` — kept ≥ 1 release as | ||
| // an escape hatch. | ||
| const legacyBlockingMcp = |
There was a problem hiding this comment.
[Suggestion] QWEN_CODE_LEGACY_MCP_BLOCKING only matches === '1'
Users who set =true, =yes, or =TRUE will silently fall through to progressive mode with no warning. This is a classic env-var footgun — easy to miss during debugging.
| const legacyBlockingMcp = | |
| const legacyBlockingMcp = | |
| ['1', 'true', 'yes'].includes( | |
| (process.env['QWEN_CODE_LEGACY_MCP_BLOCKING'] || '').toLowerCase(), | |
| ); |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
… input (QwenLM#3994) Cherry-picked from upstream QwenLM/qwen-code commit d343e2c. Resolved branding conflicts (QWEN_CODE_ → HOPCODE_) and skipped writeRuntimeStatus block not exported by @hoptrendy/hopcode-core. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- client.ts: fix 'requestToSent' typo to 'requestToSend' (cherry-pick artifact) - historyUtils.ts: add 'history_context_note' to exhaustive switch (new type from QwenLM#3994) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- fix(cli): restore settings arg to runNonInteractiveStreamJson (QwenLM#3994 regression) - feat(skills): add bash-defensive-patterns, javascript-testing-patterns, modern-javascript-patterns from wshobson/agents; add zod, react-best-practices, composition-patterns, use-ai-sdk via autoskills - fix(cli): correct sandboxImageUri in CLI package to use taimoorSiddiquiofficial - chore: bump all packages to 0.27.8 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…#4164) The progressive-MCP rollout (#3994) regressed non-interactive MCP tool visibility on the first `--prompt` request — the model never sees the configured MCP tool and answers from its own knowledge, so the test's `waitForToolCall('mcp__addition-server__add')` assertion times out on all three retries. Reproduced locally: 167s 3/3-fail without the rollback flag, 22s pass with it. Set `QWEN_CODE_LEGACY_MCP_BLOCKING=1` in the test's `beforeAll` so the spawned CLI uses the pre-#3994 synchronous discovery path. Scoped to this single test rather than the workflow env so other integration tests keep exercising the new progressive-MCP code path. Temporary workaround. Remove once #4163 is fixed.
…QwenLM#4164) The progressive-MCP rollout (QwenLM#3994) regressed non-interactive MCP tool visibility on the first `--prompt` request — the model never sees the configured MCP tool and answers from its own knowledge, so the test's `waitForToolCall('mcp__addition-server__add')` assertion times out on all three retries. Reproduced locally: 167s 3/3-fail without the rollback flag, 22s pass with it. Set `QWEN_CODE_LEGACY_MCP_BLOCKING=1` in the test's `beforeAll` so the spawned CLI uses the pre-QwenLM#3994 synchronous discovery path. Scoped to this single test rather than the workflow env so other integration tests keep exercising the new progressive-MCP code path. Temporary workaround. Remove once QwenLM#4163 is fixed. (cherry picked from commit fa6f664)
… tools reach the model (#4166) * fix(core): refresh systemInstruction in setTools() so progressive MCP 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 <qwen-coder@alibabacloud.com> * test(core): lock in SessionStart preservation across setTools refresh 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 <claude-opus-4-7@anthropic.com> --------- Co-authored-by: 秦奇 <gary.gq@alibaba-inc.com> Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Co-authored-by: Claude <claude-opus-4-7@anthropic.com>
Addresses review feedback on PR QwenLM#3994: - Skip user-disabled servers in discoverAllMcpToolsIncremental. The new incremental path used to iterate Object.entries(servers) without consulting isMcpServerDisabled, so a server the user had explicitly turned off would still get connected and its tools registered. Mirrors the existing protection in discoverAllMcpTools. - Disconnect the underlying client when runWithDiscoveryTimeout fires. Without this, the inner discoverMcpToolsForServer kept running after the timeout rejected the outer promise — if discover() eventually succeeded it would register the late server's tools into the live toolRegistry (a silent registration vector, especially exploitable with a 0/negative discoveryTimeoutMs override). - Clamp discoveryTimeoutMs to [100ms, 300_000ms]. 0/negative/Infinity values previously passed through to setTimeout unvalidated and made the silent-registration bug above trivially reachable. - Classify the `tcp` (WebSocket) transport field as remote so hung WS handshakes use the 5s default instead of the 30s stdio default. - Defensive delete of serverDiscoveryPromises[name] in the per-server catch so a doomed/orphan entry can't briefly short-circuit a subsequent discoverMcpToolsForServer call. Adds focused tests for each fix. Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> # Conflicts: # packages/core/src/tools/mcp-client-manager.test.ts # packages/core/src/tools/mcp-client-manager.ts
3 participants
Why
Config.initialize()currently runs MCP discovery synchronously, so the cli can't accept user input until every configured MCP server finishes its discover handshake. One slow or hung server bottlenecks every user who has MCP configured.Measured TTI (time to first prompt input) before this PR:
(macOS arm64 / Node 24.15, n=30/fixture, p50, profiler enabled.)
What changes
Progressive MCP availability:
Config.initialize()returns as soon as built-in tools are ready, and MCP discovery runs in a fire-and-forget background path. Each server's tools land in the registry as it becomes ready, and the cli debouncessetTools()calls into one-frame (16 ms) batches so the model sees the consolidated tool list shortly after each server settles.Interactive vs non-interactive split (important — see the "behavioral audit" section below):
qwen-codewith a TTY):Config.initialize()returns fast, UI appears immediately, MCP tools come online progressively.setTools()fires at most once per ~16 ms frame.--prompt, stream-json, ACP):Config.initialize()returns fast, then the path explicitly awaitsConfig.waitForMcpReady()BEFORE the first model send. Same tool surface as the legacy synchronous behavior — no silent regression for CI / scripts / IDE integrations.17 files, +1089 / -68:
packages/core/src/config/config.tsConfig.initialize()skipDiscovery + newstartMcpDiscoveryInBackground()+ newwaitForMcpReady()(resolves when background discovery settles, no-op when nothing was started).MCPServerConfig.discoveryTimeoutMs(last positional ctor param).packages/core/src/tools/tool-registry.tsgetMcpClientManager()getter.packages/core/src/tools/mcp-client-manager.tsdiscoverAllMcpToolsIncrementalemitsmcp-client-updateafter IN_PROGRESS / COMPLETED. Per-server discover wrapped in timeout (stdio 30 s, remote 5 s).packages/cli/src/gemini.tsxconfig.waitForMcpReady()beforerunNonInteractive. Profiler sink registration +first_paintcheckpoint +setInteractiveMode(true).packages/cli/src/nonInteractive/session.tsSession.initialize()awaitswaitForMcpReady()before resolving.packages/cli/src/acp-integration/acpAgent.tswaitForMcpReady().packages/cli/src/ui/AppContainer.tsxuseEffect(gated onisConfigInitialized) for 16 ms batch-flush ofsetTools()+ deferred startup-profile finalize (waits for MCP settle or 35 s cap).packages/core/src/utils/startupEventSink.ts(new)corecan emit profiler events without reverse-depending oncli. No-op when no sink registered.packages/cli/src/utils/startupProfiler.tsevents,recordStartupEvent,setInteractiveMode,derivedPhases, heap snapshots, MAX_EVENTS cap,QWEN_CODE_PROFILE_STARTUP_OUTER/_NO_HEAPenv opt-ins.packages/core/src/{config/config.ts, core/client.ts, tools/mcp-client-manager.ts}tool_registry_created,gemini_tools_updated,mcp_discovery_start,mcp_server_ready:<name>,mcp_first_tool_registered,mcp_all_servers_settled).packages/core/src/index.tsdocs/users/configuration/settings.mdQWEN_CODE_LEGACY_MCP_BLOCKING,QWEN_CODE_PROFILE_STARTUP_OUTER,QWEN_CODE_PROFILE_STARTUP_NO_HEAP+ clarifyQWEN_CODE_PROFILE_STARTUP.docs/users/features/mcp.mddiscoveryTimeoutMsoverride syntax, and the rollback escape hatch.Rollback:
QWEN_CODE_LEGACY_MCP_BLOCKING=1restores the previous synchronous semantics. Kept ≥ 1 release as an escape hatch. Single-commit revert otherwise.Profiler zero-cost when off: every profiler entry point short-circuits in a single null/flag check when
QWEN_CODE_PROFILE_STARTUPis unset. Heisenberg overhead measured at -1.12 % Δp50 vs profile-off (Welch p = 0.092, n=30 × 3 configs) — within statistical noise.Measured results (after this PR, same fixtures)
Lag from "first MCP server ready" → "model sees updated tool list" (2 fast + 1 slow): 6235 ms → 17.1 ms — confirms the 16 ms batch window is the operative cap.
mcp_all_servers_settledis unchanged — slow servers still take their time, but the work is now invisible to interactive users (and non-interactive paths still wait for them, by design).first_paintis unchanged (±3 % noise) across all fixtures, confirming this PR doesn't touch the pre-mount path.How to validate
The profiler instrumentation in this PR is the verification layer: anyone can reproduce the numbers above on their own machine.
One-off run (no MCP setup required)
Expected
derivedPhaseskeys when run with the changes in this PR:module_load— Node process start →main_entrycheckpointto_first_paint— Ink first frameconfig_initialize_dur—config.initialize()wall timeto_input_enabled— TTI, what users feelmcp_first_tool/mcp_all_settled— only present when MCP configuredgemini_tools_lag—mcp_first_tool→ firstsetTools()after it (one frame under PR)Before / after comparison (with MCP)
Run twice — once on
main, once on this branch — same fixture, comparederivedPhases.to_input_enabled:The structured benchmark harness I used to generate the numbers above (Welch's t-test + 4 fixtures × 30 runs + node-pty interactive driver) isn't in this PR. Happy to share it as a separate followup tooling PR if reviewers want a reproducible CI gate.
Behavioral audit
Config.initialize()now returns BEFORE MCP discovery completes. Mitigations in this PR:AppContainersubscribes tomcp-client-updateand refreshessetTools()as servers come online. Model sees new tools within ~16 ms of each server settling.--prompt(gemini.tsx:746-752): awaitsconfig.waitForMcpReady()before the first model send.nonInteractive/session.ts:138-148): awaitswaitForMcpReady()insideSession.initialize()before the first prompt can be dispatched.acp-integration/acpAgent.ts:86, 680): awaitswaitForMcpReady()after bothConfig.initialize()call sites.MCPDiscoveryState.COMPLETEDstill transitions exactly once per discovery cycle (verified by new regression test that checksmcp-client-updateemit ordering).Things reviewers should double-check:
config.getToolRegistry()immediately afterconfig.initialize()resolves and assumes MCP tools are present.Config.initialize()and expect specificdiscoveryAllToolscall counts (this PR'sConfig.initializeno longer callsdiscoverAllToolssynchronously by default — see newconfig.test.tscases).Test plan
packages/core/src/config/config.test.ts— 136 tests (132 existing + 4 new forskipInlineMcpDiscoverydefault,QWEN_CODE_LEGACY_MCP_BLOCKING=1escape hatch,waitForMcpReadyno-op when no discovery started)packages/core/src/tools/mcp-client-manager.test.ts— 12 tests (10 existing + 2 new for per-serverdiscoveryTimeoutMsenforcement and IN_PROGRESS → COMPLETEDmcp-client-updateemit ordering)packages/core/src/core/client.test.ts— 92 testspackages/cli/src/utils/startupProfiler.test.ts— 18 tests (11 existing + 7 new for events / OUTER / heap / derivedPhases)packages/core/src/utils/startupEventSink.test.ts— 4 tests (no-op / forward / exception isolation / null reset)tsc --noEmitclean forpackages/coreandpackages/clieslintclean on touched filesOut of scope (future PRs)
loadSettingsAsync+initializeAppparallelization + dynamicimportof the interactive UI (drop ~200-500 KB Ink + AppContainer from non-interactive / headless / ACP / subcommand bundles) + module-eval-time prefetch. Estimated additional TTI improvement: ~100-150 ms across all users + significant bundle reduction for headless paths./reload-plugins,ExtensionManager.refreshTools(), list-changed events).MCPServerConfig.discoveryTimeoutMsexposed via JSON schema + settings dialog so users can tune per-server.🤖 Generated with Qwen Code