feat: upstream backports — MCP reconnect, compress fixes, hooks cleanup, follow-up suggestions#19
Merged
Merged
Conversation
…ions When conversation history is near the context window limit and dominated by tool call/response cycles, findCompressSplitPoint would return a near-zero split point because it only considered non-functionResponse user messages as valid split points. This caused /compress to send almost no history to the compression API (e.g. 29 tokens), producing a useless summary that inflated token count instead of reducing it. Changes: - Track tool completion boundaries (positions after functionResponse) as fallback split points in findCompressSplitPoint - Add user-with-functionResponse to the compress-everything safety check - Use Math.max of primary and fallback split points for better coverage - Add minimum content guard (5% threshold) to prevent futile API calls - Add 4 new test cases covering tool-heavy conversation scenarios Fixes QwenLM#2647
…heavy conversations - Strip trailing orphaned funcCall (force=true) before split point calculation, so normal compression logic runs cleanly on the remaining history instead of requiring ad-hoc special-casing - Remove redundant lastToolCompletionSplitPoint machinery: after fixing the i+2 index bug, lastSplitPoint already subsumes it, making Math.max redundant - Add MIN_COMPRESSION_FRACTION constant (0.05) to guard against futile API calls when historyToCompress is too small relative to total history - Add tests for orphaned funcCall handling (force=true compresses, force=false NOOP) - Add test for MIN_COMPRESSION_FRACTION guard Fixes QwenLM#2647
The previous version (1.1.0) has a native-level bug on macOS where each PTY spawn leaks one /dev/ptmx file descriptor that is never closed. Over a long session with hundreds of shell commands, this exhausts the system-wide PTY pool (kern.tty.ptmx_max = 511), breaking other programs like tmux and new terminal windows. Root cause: microsoft/node-pty#882 Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
…mprovements in KeypressProvider
The error handler in hookRunner cleared the timeout but did not remove the abort signal listener, unlike the close handler. When spawn fails (e.g. executable not found), only the error event fires — the close event is not guaranteed — so the abort listener leaked on the signal.
- Add new npm extension installation channel via scoped packages (@scope/name) - Implement npm.ts module with registry resolution, authentication, and download logic - Support version pinning, dist-tags (latest, beta), and custom registries - Handle private registry auth via NPM_TOKEN env var and .npmrc _authToken entries - Update CLI install command with --registry flag for npm extensions - Add comprehensive tests for npm package parsing and registry operations - Update documentation for releasing and installing from npm registries - Integrate npm updates into extension manager and update checking flow This enables teams using npm for package distribution to publish Qwen Code extensions through their existing infrastructure, with full support for private registries and access control. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
- Restore lsp field to CliArgs interface (removed by hooks cleanup cherry-pick) - Add lsp: undefined to auth handler and gemini test fixtures - Resolve client.ts conflict: keep both endTurnSpan and cache-safe param saving - Fix speculationToolGate: ToolNames.TODO_WRITE → ToolNames.TASK_LIST - mcp-tool: replace getStringifiedResultForDisplay with getDisplayFromParts, remove unused allowlist field and truncateTextParts method Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a follow-up suggestion and speculative execution subsystem (generation, cache-aware forked queries, filtering, overlay FS, tool-gating, speculation lifecycle), CLI/Web UI integration (ghost-text UX, fast-model), npm extension support, hooks opt-out semantics, MCP reconnect command and retries, telemetry events, many tests, and documentation. Changes
Sequence Diagram(s)mermaid User->>UI: Model turn completes Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
There was a problem hiding this comment.
Actionable comments posted: 18
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
packages/core/src/services/chatCompressionService.ts (1)
366-430:⚠️ Potential issue | 🟠 MajorStripping the orphaned tail is lost on NOOP/failure paths.
historyForSplitonly changes the summary input. Ifforce=trueand the stripped history later hitssplitPoint === 0, the new 5% guard, or any failed-summary path, this method still returnsnewHistory: null, so the caller keeps the originalcuratedHistorywith the danglingfunctionCallintact. A minimal repro is[user, model(functionCall)]: the tail is "stripped" locally, then the early NOOP puts it right back. The sanitized history needs to survive those early returns too—either by persisting the cleanup before deciding whether to summarize, or by returning a distinct sanitized result.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/services/chatCompressionService.ts` around lines 366 - 430, The current logic only strips the orphaned model functionCall into historyForSplit but then returns newHistory: null on NOOP/failure paths, which restores the dangling tail; update the early-return branches (the historyToCompress.length === 0 branch and the MIN_COMPRESSION_FRACTION guard branch) to return a sanitized newHistory that reflects the stripped history (i.e., use historyForSplit or reconstructed curatedHistory without the last orphaned message when force && hasOrphanedFuncCall) instead of null so the caller receives the cleaned-up history; locate references to curatedHistory, lastMessage, hasOrphanedFuncCall, historyForSplit, historyToCompress and the findCompressSplitPoint call to implement the change.packages/core/src/telemetry/types.ts (1)
898-932:⚠️ Potential issue | 🟠 MajorNew telemetry events are not included in the
TelemetryEventunion type.
PromptSuggestionEventandSpeculationEvent(lines 1066-1137) are missing from theTelemetryEventunion. This will cause type mismatches when these events are passed to telemetry logging functions that expectTelemetryEvent.🐛 Proposed fix
export type TelemetryEvent = | StartSessionEvent | EndSessionEvent | UserPromptEvent | ToolCallEvent | ApiRequestEvent | ApiErrorEvent | ApiCancelEvent | ApiResponseEvent | FlashFallbackEvent | LoopDetectedEvent | LoopDetectionDisabledEvent | NextSpeakerCheckEvent | KittySequenceOverflowEvent | MalformedJsonResponseEvent | IdeConnectionEvent | ConversationFinishedEvent | SlashCommandEvent | FileOperationEvent | InvalidChunkEvent | ContentRetryEvent | ContentRetryFailureEvent | SubagentExecutionEvent | ExtensionEnableEvent | ExtensionInstallEvent | ExtensionUninstallEvent | ToolOutputTruncatedEvent | ModelSlashCommandEvent | AuthEvent | HookCallEvent | SkillLaunchEvent | UserFeedbackEvent | ArenaSessionStartedEvent | ArenaAgentCompletedEvent - | ArenaSessionEndedEvent; + | ArenaSessionEndedEvent + | PromptSuggestionEvent + | SpeculationEvent;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/telemetry/types.ts` around lines 898 - 932, The TelemetryEvent union type is missing the new event types, causing type errors when logging PromptSuggestionEvent and SpeculationEvent; update the TelemetryEvent union (the exported type TelemetryEvent) to include PromptSuggestionEvent and SpeculationEvent so they are recognized as valid telemetry events for functions that accept TelemetryEvent.packages/core/src/extension/marketplace.ts (1)
207-258:⚠️ Potential issue | 🟠 Major
npm:sources still miss the new npm branch.At Line 211,
parseSourceAndPluginName()splits a raw source likenpm:package-nameintorepo === 'npm'andpluginName === 'package-name'. That means the new check at Lines 252-258 never runs, so rawnpm:installs still fall through toInstall source not found. Please special-case thenpm:scheme before plugin-name parsing, and use the npm package parser for unscoped specs too.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/extension/marketplace.ts` around lines 207 - 258, In parseInstallSource, special-case the "npm:" scheme before calling parseSourceAndPluginName: if source startsWith "npm:" (or matches the npm scheme), treat it as an npm install, parse the rest of the spec with the npm package parser (not parseSourceAndPluginName) to obtain package name and optional version, set installMetadata.type = 'npm' and pluginName appropriately, and skip the isScopedNpmPackage branch; keep parseSourceAndPluginName for non-scheme inputs and retain isScopedNpmPackage for scoped-package detection. Ensure you update repo/repoSource/pluginName assignments so raw "npm:package-name" no longer yields repo === 'npm' and falls through.packages/cli/src/ui/components/ModelDialog.tsx (1)
148-206:⚠️ Potential issue | 🟠 MajorFast-model mode can't round-trip every selectable option.
Fast mode still renders runtime models and models from every auth provider, but it only persists
fastModel = modelIdand later reconstructs the selected key with the currentauthTypewhileactiveRuntimeSnapshotis disabled. Picking a runtime model, or a model from a different provider, therefore won't restore reliably and can resolve to the wrong target on reopen/use. Either hide unsupported entries in fast mode, or persist enough metadata (authType/ snapshot id) alongsidefastModel.Also applies to: 250-265, 301-323
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/ui/components/ModelDialog.tsx` around lines 148 - 206, The current fast-model flow can't reliably restore selections because only modelId is persisted; update the code so fast-mode selections either are limited to reconstructable entries or persist full metadata: when building availableModelEntries (used by the selector) detect fast mode (fastModel flag) and filter out entries that cannot be round-tripped (e.g., skip runtime models if activeRuntimeSnapshot is false and skip registry models whose authType cannot be reselected), and/or change the fastModel storage to an object { modelId, authType, snapshotId } and update the selection serialization/deserialization logic to read/write these fields so reconstruction uses authType and snapshotId (adjust any code that reads fastModel to prefer the expanded shape); make changes to the selector build (availableModelEntries) and the fastModel read/write code paths so they stay consistent.
🟡 Minor comments (18)
docs/users/extension/extension-releasing.md-138-146 (1)
138-146:⚠️ Potential issue | 🟡 MinorAdd language identifiers to fenced code blocks.
Several changed fenced blocks are missing a language tag (markdownlint MD040). Please use
```textfor directory trees and```bashfor commands.Suggested doc fix
-``` +```text my-extension/ ├── package.json ├── qwen-extension.json ├── QWEN.md # optional context file ├── commands/ # optional custom commands ├── skills/ # optional custom skills └── agents/ # optional custom subagents-
+bashInstall latest version
qwen extensions install
@your-org/my-extension
...-``` +```bash # Publish a beta release npm publish --tag beta ...Also applies to: 166-175, 196-202
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/users/extension/extension-releasing.md` around lines 138 - 146, Update the fenced code blocks in docs/users/extension/extension-releasing.md to include language identifiers: change the directory tree block (the block showing "my-extension/ ├── package.json ...") to use ```text, and change the bash/command blocks (the install and publish examples shown in the diff) to use ```bash; apply the same fixes to the other similar blocks referenced around lines 166-175 and 196-202 so all directory trees use text and all shell examples use bash.docs/users/extension/extension-releasing.md-9-9 (1)
9-9:⚠️ Potential issue | 🟡 MinorUse hyphenated compound adjective on Line 9.
platform specificshould beplatform-specificin both occurrences for correct grammar and consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/users/extension/extension-releasing.md` at line 9, Replace the two instances of the unhyphenated phrase "platform specific" in the sentence starting "GitHub releases may also contain platform specific archives..." with the hyphenated compound adjective "platform-specific" for correct grammar and consistency; search for the exact phrase "platform specific archives" in the doc and update both occurrences to "platform-specific archives".docs/users/extension/introduction.md-19-19 (1)
19-19:⚠️ Potential issue | 🟡 MinorFix command description text on Line 19.
There’s a small user-facing wording issue:
page(Gemini or ClaudeCode)is missing a space afterpage, andClaudeCodeshould be consistently written asClaude Codeif that’s the product name used elsewhere.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/users/extension/introduction.md` at line 19, Update the table cell description for the `/extensions explore [source]` command so it reads "Open extensions source page (Gemini or Claude Code) in your browser" — add the missing space after "page" and change "ClaudeCode" to "Claude Code" to match the project's product naming; update the cell string that currently contains "| `/extensions explore [source]` | Open extensions source page(Gemini or ClaudeCode) in your browser |".docs/users/overview.md-59-59 (1)
59-59:⚠️ Potential issue | 🟡 MinorConsider hyphenating "Follow-up" for consistency with standard English conventions.
"Follow-up" is typically hyphenated when used as a compound adjective. However, if "Followup" is the established product/feature naming convention across the codebase, this can be ignored.
📝 Proposed fix
-- **[Followup suggestions](./features/followup-suggestions)**: Qwen Code predicts what you want to type next and shows it as ghost text. Press Tab to accept, or just keep typing to dismiss. +- **[Follow-up suggestions](./features/followup-suggestions)**: Qwen Code predicts what you want to type next and shows it as ghost text. Press Tab to accept, or just keep typing to dismiss.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/users/overview.md` at line 59, Change the link text "**[Followup suggestions](./features/followup-suggestions)**" to use the hyphenated form "**Follow-up suggestions**" (i.e., update the markdown link label to "[Follow-up suggestions](./features/followup-suggestions)"), unless the project-wide product/feature naming convention intentionally uses "Followup"—in that case leave it as-is after confirming the established naming.packages/cli/src/acp-integration/session/Session.ts-926-926 (1)
926-926:⚠️ Potential issue | 🟡 MinorRemove unnecessary optional chaining on Config method calls.
Line 926 uses optional chaining on
getDisableAllHooks?.()andgetMessageBus?.(), butconfigis a required non-optional property and both methods are required on theConfigtype (always defined and non-optional). This pattern diverges from all other callsites in the codebase, which call these methods directly. Remove the?.operators to align with the rest of the codebase and clarify that these methods always exist.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/acp-integration/session/Session.ts` at line 926, Remove the unnecessary optional chaining when calling config methods: replace uses of this.config.getDisableAllHooks?.() and this.config.getMessageBus?.() with direct calls this.config.getDisableAllHooks() and this.config.getMessageBus() (these methods are required on Config); update those callsites in Session.ts (and any other occurrences in the file) so they match the rest of the codebase and no longer use the ?. operator.integration-tests/hook-integration/hooks.test.ts-47-47 (1)
47-47:⚠️ Potential issue | 🟡 MinorRemove the non-functional
enabled: truepattern from hook configs or ensure tests verify actual hook execution.Line 47 correctly uses
disableAllHooks: false, but the file also contains many instances ofhooks: { enabled: true, ... }(e.g., line 1687+). Theenabledfield at the hooks-object level is explicitly filtered out and ignored by HookRegistry during processing—it's listed inHOOKS_CONFIG_FIELDSand skipped viaif (HOOKS_CONFIG_FIELDS.includes(eventName)) { continue; }.This means tests using
hooks: { enabled: true, SessionStart: [...] }are relying on a non-functional flag. While the hook definitions (SessionStart, etc.) are still processed and the tests likely pass, theenabled: truehas no effect and provides false assurance of control. Standardize on the working pattern: either removeenabled: trueor migrate all instances to usedisableAllHooks: falseat the settings level for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@integration-tests/hook-integration/hooks.test.ts` at line 47, Tests are using a non-functional "enabled: true" key inside hooks configs which HookRegistry ignores (see HOOKS_CONFIG_FIELDS and the filtering logic in HookRegistry), so remove all occurrences of the top-level "enabled" property from hook config objects or replace them with the functional pattern (set disableAllHooks: false in the settings used by the tests); search for "hooks: { enabled:" and update those instances, ensuring HookRegistry still receives actual hook event keys like "SessionStart" so the tests verify real hook execution.packages/cli/src/i18n/locales/de.js-627-628 (1)
627-628:⚠️ Potential issue | 🟡 MinorFix German grammar in disabled-hooks sentence.
Line 627–628 is missing a noun after
{{count}}, so the sentence reads incomplete in UI.Suggested wording
- 'All hooks are currently disabled. You have {{count}} that are not running.': - 'Alle Hooks sind derzeit deaktiviert. Sie haben {{count}} die nicht ausgeführt werden.', + 'All hooks are currently disabled. You have {{count}} that are not running.': + 'Alle Hooks sind derzeit deaktiviert. Sie haben {{count}} Hooks, die nicht ausgeführt werden.',🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/i18n/locales/de.js` around lines 627 - 628, The German translation for the key 'All hooks are currently disabled. You have {{count}} that are not running.' is missing a noun after {{count}}; update the translation string (currently 'Alle Hooks sind derzeit deaktiviert. Sie haben {{count}} die nicht ausgeführt werden.') to include the noun and correct grammar, e.g. change it to 'Alle Hooks sind derzeit deaktiviert. Sie haben {{count}} Hooks, die nicht ausgeführt werden.' so the sentence reads complete and grammatically correct.packages/cli/src/ui/components/hooks/HooksDisabledStep.test.tsx-13-29 (1)
13-29:⚠️ Potential issue | 🟡 MinorZero-count handling in the mock is still falsy-sensitive.
The guards here use
options?.count, so a numeric0skips the interpolation branches. IfHooksDisabledStepforwardsconfiguredHooksCountas a number, the zero-hooks case stops exercising the translated path and falls back to the raw key instead.🛠️ Suggested fix
- t: vi.fn((key: string, options?: { count?: string }) => { + t: vi.fn((key: string, options?: { count?: number | string }) => { // Handle pluralization - if (key === '{{count}} configured hook' && options?.count) { + if ( + key === '{{count}} configured hook' && + options?.count !== undefined + ) { return `${options.count} configured hook`; } - if (key === '{{count}} configured hooks' && options?.count) { + if ( + key === '{{count}} configured hooks' && + options?.count !== undefined + ) { return `${options.count} configured hooks`; } // Handle interpolation for main message if ( key === 'All hooks are currently disabled. You have {{count}} that are not running.' && - options?.count + options?.count !== undefined ) { return `All hooks are currently disabled. You have ${options.count} that are not running.`; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/ui/components/hooks/HooksDisabledStep.test.tsx` around lines 13 - 29, The mock translator function t in HooksDisabledStep.test.tsx currently checks options?.count which treats 0 as falsy and skips interpolation; update the guards to explicitly detect numeric zero (e.g., check options?.count !== undefined and options?.count !== null or use typeof options?.count === 'number' or Number.isFinite(options.count')) so the branches for '{{count}} configured hook(s)' and the long message string interpolate when count is 0 and the test exercises the translated path.packages/core/src/core/client.ts-994-1013 (1)
994-1013:⚠️ Potential issue | 🟡 MinorPersist the cache-safe snapshot on every successful return.
This block only runs on the final fallthrough. When
model.skipNextSpeakerCheckis enabled, Line 945 returns before it, so the follow-up cache never refreshes and suggestions stay stale/empty for that mode. Pull this into a helper and call it from each non-aborted success exit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/core/client.ts` around lines 994 - 1013, The cache-save logic that calls saveCacheSafeParams using this.getChat and chat.getHistory only runs on the final fallthrough, so when model.skipNextSpeakerCheck causes an early return the cache snapshot isn't refreshed; extract that block into a helper method (e.g., persistCacheSafeSnapshot or similar) which accepts the signal check and uses this.getChat(), chat.getGenerationConfig(), and this.config.getModel() to compute cachedHistory and call saveCacheSafeParams, ensure it swallows errors like the original, and invoke this helper from every non-aborted success exit path (including the branch where model.skipNextSpeakerCheck triggers an early return) so the cache is persisted on every successful return.packages/cli/src/ui/components/hooks/HooksManagementDialog.tsx-361-364 (1)
361-364:⚠️ Potential issue | 🟡 MinorDon't bypass load/error handling for the disabled step.
Because this branch returns before the
isLoadingandloadErrorchecks, a settings parse/load failure while hooks are globally disabled renders a normal disabled view with a misleading count instead of surfacing the failure. Either move the disabled branch below the error handling, or letHooksDisabledSteprender explicit loading/error states.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/ui/components/hooks/HooksManagementDialog.tsx` around lines 361 - 364, The early return for the disabled state in HooksManagementDialog (checking currentStep === HOOKS_MANAGEMENT_STEPS.HOOKS_DISABLED and returning HooksDisabledStep) bypasses isLoading/loadError handling; move that disabled-branch check below the existing isLoading and loadError checks so the component first handles loading and error states before rendering HooksDisabledStep (passing configuredHooksCount as before), or alternatively update HooksDisabledStep to accept isLoading and loadError and render explicit loading/error UIs instead—choose one approach and apply consistently so failures are surfaced when hooks are disabled.docs/users/configuration/settings.md-191-195 (1)
191-195:⚠️ Potential issue | 🟡 MinorClarify the JSON path for
fastModel.This section introduces
fastModelas a bare setting, but Line 76 says settings live inside top-level category objects. Right now it's unclear whether users should set{ "fastModel": ... }or nest it undermodel. Please make the path explicit and mirror it in the example config.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/users/configuration/settings.md` around lines 191 - 195, The documentation currently treats fastModel as a top-level key but settings are stored under category objects; update the wording and example to explicitly state that fastModel lives under the model category (i.e., model.fastModel) and mirror that in the example configuration, update the table/description to show the JSON path as model.fastModel, and note that the CLI command /model --fast maps to model.fastModel so users know where to set it.docs/users/features/followup-suggestions.md-105-105 (1)
105-105:⚠️ Potential issue | 🟡 MinorCapitalize "Markdown" as a proper noun.
"Markdown" is a proper noun (named after its creator John Gruber) and should be capitalized.
Proposed fix
-- Cannot be multiple sentences or contain formatting (markdown, newlines) +- Cannot be multiple sentences or contain formatting (Markdown, newlines)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/users/features/followup-suggestions.md` at line 105, Update the text fragment "Cannot be multiple sentences or contain formatting (markdown, newlines)" so that the word "markdown" is capitalized as "Markdown" (i.e., change the string to "Cannot be multiple sentences or contain formatting (Markdown, newlines)"); ensure any other occurrences of the lowercase "markdown" in this document are similarly capitalized to "Markdown".docs/users/features/followup-suggestions.md-11-13 (1)
11-13:⚠️ Potential issue | 🟡 MinorAdd language specifier to fenced code block.
The code block lacks a language specifier. For user input examples, consider using
textorplaintext.Proposed fix
-``` +```text > run the tests</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@docs/users/features/followup-suggestions.mdaround lines 11 - 13, The fenced
code block containing the example line "> run the tests" is missing a language
specifier; update that fenced block in
docs/users/features/followup-suggestions.md to add a language token (e.g.,
text orplaintext) immediately after the opening backticks so the block
becomes a plaintext example and renders correctly.</details> </blockquote></details> <details> <summary>docs/users/features/followup-suggestions.md-51-53 (1)</summary><blockquote> `51-53`: _⚠️ Potential issue_ | _🟡 Minor_ **Add language specifier to fenced code block.** Same issue as above - the code block should have a language specifier for consistent markdown linting. <details> <summary>Proposed fix</summary> ```diff -``` +```text /model --fast qwen3.5-flash ``` ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@docs/users/features/followup-suggestions.mdaround lines 51 - 53, The fenced
code block containing "/model --fast qwen3.5-flash" is missing a language
specifier; update the triple-backtick fence to include a language (e.g., change
totext) so the block follows markdown lint rules and renders
consistently—locate the fenced block around the literal "/model --fast
qwen3.5-flash" and add the language token to the opening fence.</details> </blockquote></details> <details> <summary>packages/cli/src/ui/components/hooks/HooksDisabledStep.tsx-7-17 (1)</summary><blockquote> `7-17`: _⚠️ Potential issue_ | _🟡 Minor_ **Missing `React` import for type annotation at line 17.** The return type `React.JSX.Element` references the `React` namespace, but `React` is not imported. While React 19's JSX transform allows JSX syntax without importing React, type annotations using `React.JSX.Element` still require the import. This pattern is inconsistent across the codebase—22 other components that use `React.JSX.Element` do import React. <details> <summary>Proposed fix</summary> ```diff +import React from 'react'; import { Box, Text } from 'ink'; import { theme } from '../../semantic-colors.js'; import { t } from '../../../i18n/index.js'; ``` Alternatively, use `JSX.Element` directly without the `React.` prefix. </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/ui/components/hooks/HooksDisabledStep.tsx` around lines 7 - 17, The component HooksDisabledStep declares its return type as React.JSX.Element but React is not imported; either add an import for React (so the React namespace is available) or change the return type to JSX.Element to avoid needing the React import; update the HooksDisabledStep function signature accordingly and ensure consistency with other components using React.JSX.Element or JSX.Element. ``` </details> </blockquote></details> <details> <summary>docs/design/prompt-suggestion/prompt-suggestion-design.md-9-10 (1)</summary><blockquote> `9-10`: _⚠️ Potential issue_ | _🟡 Minor_ **The word-count rule conflicts with the filter table.** The overview and prompt both say suggestions are `2-12 words`, but `too_few_words` explicitly allows one-word outputs like `commit` and `push`. That ambiguity will leak into implementation and tests; please pick one rule and use it consistently throughout the spec. Also applies to: 75-95 <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@docs/design/prompt-suggestion/prompt-suggestion-design.md` around lines 9 - 10, The doc currently conflicts: the overview/spec for "prompt suggestion" states suggestions must be 2–12 words while the filter `too_few_words` allows one-word outputs; pick and apply a single rule consistently by either (A) changing the overview and example prompts to state the allowed range as 1–12 words, or (B) tightening the `too_few_words` filter to reject single-word suggestions and require a minimum of 2 words; update every occurrence of the phrase/rule (including the "prompt suggestion" overview and the `too_few_words` filter and the repeated text around the sections referenced) so the spec, examples, and filter table all use the same word-count rule. ``` </details> </blockquote></details> <details> <summary>packages/core/src/followup/overlayFs.test.ts-107-119 (1)</summary><blockquote> `107-119`: _⚠️ Potential issue_ | _🟡 Minor_ **This case never exercises a relative path.** The value passed to `resolveReadPath()` is still the absolute file path under `testDir`, so this rechecks the written-file branch instead of covering relative resolution. Pass a path relative to `realCwd` here. <details> <summary>🧪 Minimal fix</summary> ```diff - const resolved = overlay.resolveReadPath(join(testDir, 'src', 'app.ts')); + const resolved = overlay.resolveReadPath(join('src', 'app.ts')); ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@packages/core/src/followup/overlayFs.test.ts` around lines 107 - 119, The test is passing an absolute path to overlay.resolveReadPath so it never exercises the relative-path branch; change the argument to a path relative to overlay.realCwd (or realCwd used in the test) so resolveReadPath receives a relative path — e.g. compute the relative path from realCwd to realFile (or directly pass join('src','app.ts') if realCwd is testDir) after calling overlay.redirectWrite(realFile) so the test asserts the relative-resolution behavior of overlay.resolveReadPath. ``` </details> </blockquote></details> <details> <summary>packages/cli/src/ui/components/hooks/HooksManagementDialog.test.tsx-126-133 (1)</summary><blockquote> `126-133`: _⚠️ Potential issue_ | _🟡 Minor_ **Reset the `useConfig` implementation between tests.** Tests override `useConfig()` to return `getDisableAllHooks() === true` (at lines 241–246 and 261–265), but `beforeEach` only calls `vi.clearAllMocks()`. In Vitest, this clears call history only—custom mock implementations set via `mockReturnValue()` persist across tests. If test execution order changes (e.g., due to randomization), later tests could inherit the overridden state. Either restore the default `useConfig` mock in `beforeEach`, or switch to `vi.resetAllMocks()` and re-register the shared mocks. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/ui/components/hooks/HooksManagementDialog.test.tsx` around lines 126 - 133, The tests override useConfig() in individual test cases but beforeEach only calls vi.clearAllMocks(), so mock implementations persist across tests; update the beforeEach in HooksManagementDialog.test.tsx to reset the useConfig mock between tests — either call vi.resetAllMocks() and then re-register shared mocks (e.g., mockedUseKeypress mockImplementation) or explicitly restore the default mockedUseConfig implementation (undoing test-specific mockReturnValue for getDisableAllHooks) so functions like useConfig and getDisableAllHooks do not leak state across tests. ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🧹 Nitpick comments (10)</summary><blockquote> <details> <summary>docs/superpowers/plans/2026-04-02-task-update-diff-display.md (2)</summary><blockquote> `13-13`: **Fix heading level increment.** The task headings (lines 13, 122, 393, 509) use h3 (`###`) directly after an h2 (`---` on line 11), skipping the proper hierarchy. These should be h2 (`##`) headings to maintain consistent document structure. <details> <summary>📝 Proposed fix</summary> ```diff --- -### Task 1: Add `TaskUpdateDiffDisplay` type to core +## Task 1: Add `TaskUpdateDiffDisplay` type to core **Files:** ``` Apply the same change to Task 2 (line 122), Task 3 (line 393), and Task 4 (line 509). </details> As per coding guidelines: Format code with Prettier for consistent styling applies to `**/*.md` files. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@docs/superpowers/plans/2026-04-02-task-update-diff-display.md` at line 13, Headings for the task sections use h3 (###) immediately after the h2 document divider (---), breaking hierarchy; update the task section headings "Task 1: Add `TaskUpdateDiffDisplay` type to core", "Task 2", "Task 3", and "Task 4" to h2 (##) instead of h3, and reformat the markdown file with Prettier (apply to **/*.md) to ensure consistent styling. ``` </details> --- `608-612`: **Add language specifier to example output block.** The example output block is missing a language specifier. Adding `text` or `plaintext` improves markdown rendering consistency. <details> <summary>📝 Proposed fix</summary> ```diff Expected output should show a diff block: -``` +```text ✓ Update task <id>: status=in_progress test task status pending → in_progress ``` ``` </details> As per coding guidelines: Format code with Prettier for consistent styling applies to `**/*.md` files. <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@docs/superpowers/plans/2026-04-02-task-update-diff-display.mdaround lines
608 - 612, The fenced code block showing the task update example currently has
no language tag; update the opening fence fromtotext (orplaintext) so the snippet becomes "text" and then run Prettier (or format markdown) to
ensure consistent styling for **/*.md files; target the example block containing
"✓ Update task : status=in_progress" and ensure the closing fence remains
unchanged.</details> </blockquote></details> <details> <summary>docs/superpowers/specs/2026-04-02-task-update-diff-display.md (1)</summary><blockquote> `21-41`: **Add language specifiers to fenced code blocks.** The example output blocks on lines 21, 27, and 37 are missing language specifiers. Adding `text` or `plaintext` to these code fences improves markdown rendering and readability. <details> <summary>📝 Proposed fix</summary> ```diff -``` +```text ✓ Update task 5e24f34e: status=in_progress Explore project context for brainstorming status pending → in_progress ``` -``` +```text ✓ Update task 5e24f34e: status=completed, title=... Research auth patterns title "Research auth" → "Research auth patterns" status in_progress → completed priority medium → high ``` -``` +```text ✓ Update task 5e24f34e Explore project context no changes ``` ``` </details> As per coding guidelines: Format code with Prettier for consistent styling applies to `**/*.md` files. <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@docs/superpowers/specs/2026-04-02-task-update-diff-display.mdaround lines
21 - 41, The three example fenced code blocks showing "✓ Update task 5e24f34e:
status=in_progress", "✓ Update task 5e24f34e: status=completed, title=...", and
the "✓ Update task 5e24f34e" no-op block are missing language specifiers; update
each opening triple-backtick to include a language (e.g.,text) so the blocks readtext, and then reformat the markdown with Prettier to ensure consistent
styling across the file.</details> </blockquote></details> <details> <summary>packages/core/src/followup/forkedQuery.ts (2)</summary><blockquote> `71-83`: **`JSON.stringify` comparison is fragile for object equality.** Using `JSON.stringify` to compare `systemInstruction` and `tools` objects can produce false positives (treating equal objects as different) when key ordering differs, or false negatives when semantically equivalent values serialize identically despite differences. Consider using a deep equality utility for more reliable comparison. <details> <summary>♻️ Alternative approaches</summary> Option 1: Use a deep-equal library like `fast-deep-equal`: ```diff +import equal from 'fast-deep-equal'; const sysChanged = !prevConfig || - JSON.stringify(prevConfig.systemInstruction) !== - JSON.stringify(generationConfig.systemInstruction); + !equal(prevConfig.systemInstruction, generationConfig.systemInstruction); const toolsChanged = !prevConfig || - JSON.stringify(prevConfig.tools) !== JSON.stringify(generationConfig.tools); + !equal(prevConfig.tools, generationConfig.tools); ``` Option 2: If the current approach works in practice, add a comment explaining the trade-off. </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@packages/core/src/followup/forkedQuery.ts` around lines 71 - 83, The JSON.stringify comparisons for prevConfig.systemInstruction and prevConfig.tools are fragile; replace them with a proper deep-equality check (e.g., import deepEqual from "fast-deep-equal" or use lodash's isEqual) and change the sysChanged/toolsChanged checks to use deepEqual(prevConfig.systemInstruction, generationConfig.systemInstruction) and deepEqual(prevConfig.tools, generationConfig.tools); update imports accordingly and ensure currentVersion is incremented only when deepEqual reports inequality. If you intentionally accept stringify behavior, add a short comment on why JSON.stringify is sufficient and note the trade-offs instead of changing logic. ``` </details> --- `55-60`: **Global mutable state may cause issues in multi-session scenarios.** The module-level `currentCacheSafeParams` and `currentVersion` variables assume a single active session. If the codebase ever supports multiple concurrent sessions, this design would need refactoring to per-session state management. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@packages/core/src/followup/forkedQuery.ts` around lines 55 - 60, The module-level mutable globals currentCacheSafeParams and currentVersion create shared state across all sessions which will break in multi-session/concurrent use; refactor to move these into a per-session or per-instance store (e.g., attach them to a Session/Context object or create a ForkedQueryManager class) and update all usages in this module to read/write from that per-session object instead of the globals (replace direct references to currentCacheSafeParams/currentVersion with getters/setters on the session/context or instance fields on the new ForkedQueryManager). ``` </details> </blockquote></details> <details> <summary>packages/cli/src/utils/systemInfoFields.test.ts (1)</summary><blockquote> `34-46`: **LGTM!** The test correctly verifies that "Fast Model" appears in the expected position. Since the test input lacks an explicit `fastModel` property, this validates the fallback behavior where `modelVersion` is displayed. Consider adding a test case with an explicit `fastModel` value to verify both code paths. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/utils/systemInfoFields.test.ts` around lines 34 - 46, Add a new unit test in packages/cli/src/utils/systemInfoFields.test.ts that covers the explicit fastModel code path by supplying an input object with a fastModel property (e.g., alongside modelVersion) and asserting that the systemInfoFields (or the test's labels/values) include "Fast Model" and display the provided fastModel value instead of falling back to modelVersion; locate the existing test that checks labels (the one asserting the array with 'Fast Model') and duplicate/extend it to verify both the fallback behavior and the explicit fastModel behavior for functions/variables used in that file (e.g., systemInfoFields generator or the test helpers). ``` </details> </blockquote></details> <details> <summary>packages/webui/src/styles/components.css (1)</summary><blockquote> `452-458`: **Add keyboard-focus affordance to match hover behavior.** Line 452 currently improves discoverability only on hover. Consider mirroring this style on focus/focus-visible for keyboard users. <details> <summary>Suggested tweak</summary> ```diff .composer-input[data-has-suggestion='true']:hover:empty::before, .composer-input[data-has-suggestion='true']:hover[data-empty='true']::before { opacity: 0.9; text-decoration: underline; text-decoration-style: dotted; text-underline-offset: 2px; } + +.composer-input[data-has-suggestion='true']:focus:empty::before, +.composer-input[data-has-suggestion='true']:focus[data-empty='true']::before, +.composer-input[data-has-suggestion='true']:focus-visible:empty::before, +.composer-input[data-has-suggestion='true']:focus-visible[data-empty='true']::before { + opacity: 0.9; + text-decoration: underline; + text-decoration-style: dotted; + text-underline-offset: 2px; +} ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@packages/webui/src/styles/components.css` around lines 452 - 458, The hover-only affordance for suggestion discoverability should also apply on keyboard focus; update the selector(s) targeting .composer-input[data-has-suggestion='true']:hover:empty::before and .composer-input[data-has-suggestion='true']:hover[data-empty='true']::before to include :focus and/or :focus-visible (e.g., add :focus:empty::before and :focus-visible[data-empty='true']::before) so the same opacity, dotted underline, and text-underline-offset styles are applied for keyboard users using the .composer-input element. ``` </details> </blockquote></details> <details> <summary>packages/cli/src/ui/components/InputPrompt.test.tsx (1)</summary><blockquote> `239-302`: **Prefer fake timers over fixed sleeps here.** These tests add four 350ms wall-clock waits and couple the assertions to the current suggestion-delay implementation, which makes the suite slower and more brittle in CI. `vi.useFakeTimers()` plus `vi.advanceTimersByTimeAsync()` would make this path deterministic. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/ui/components/InputPrompt.test.tsx` around lines 239 - 302, Replace the real-time sleeps in the InputPrompt prompt-suggestions tests with fake timers: call vi.useFakeTimers() in the describe or beforeEach and vi.useRealTimers() in afterEach, then replace each await wait(350) with await vi.advanceTimersByTimeAsync(350) (and any subsequent await wait() with advanceTimersByTimeAsync(0) or a small advance to let microtasks run) so the suggestion-delay logic in the InputPrompt tests (the `describe('prompt suggestions')` block and the tests that call renderWithProviders, stdin.write, mockBuffer.insert, props.onSubmit, mockCommandCompletion.handleAutocomplete) becomes deterministic and fast. Ensure timers are restored after the tests. ``` </details> </blockquote></details> <details> <summary>packages/cli/src/ui/components/hooks/HooksDisabledStep.tsx (1)</summary><blockquote> `19-24`: **Consider using i18n pluralization rules instead of manual branching.** The manual `configuredHooksCount === 1` check works but doesn't scale well for languages with complex pluralization rules (e.g., Russian, Arabic). Most i18n libraries support built-in pluralization. <details> <summary>Suggestion using i18n pluralization</summary> If the i18n library supports ICU plural format or count-based keys: ```diff - const hooksText = - configuredHooksCount === 1 - ? t('{{count}} configured hook', { count: String(configuredHooksCount) }) - : t('{{count}} configured hooks', { - count: String(configuredHooksCount), - }); + const hooksText = t('configured_hooks', { count: configuredHooksCount }); ``` With translation key: ```json { "configured_hooks_one": "{{count}} configured hook", "configured_hooks_other": "{{count}} configured hooks" } ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/ui/components/hooks/HooksDisabledStep.tsx` around lines 19 - 24, The current manual branching on configuredHooksCount (in the hooksText constant) bypasses the i18n library's pluralization support and won't handle complex plural forms; replace the conditional logic with a single t(...) call that leverages the i18n pluralization/count feature (pass configuredHooksCount as the count/value and use pluralized translation keys or ICU plural syntax in your translation files) so hooksText is produced by the i18n layer rather than by configuredHooksCount === 1 checks. ``` </details> </blockquote></details> <details> <summary>packages/cli/src/ui/commands/modelCommand.ts (1)</summary><blockquote> `48-77`: **Consider more robust argument parsing for `--fast` flag.** The current parsing with `.replace('--fast', '').trim()` works for typical usage but doesn't handle edge cases like `/model --fastmodel` (no space after flag). Using a more explicit parsing approach would be safer. <details> <summary>More robust parsing suggestion</summary> ```diff // Handle --fast flag: /model --fast <modelName> const args = context.invocation?.args?.trim() ?? ''; - if (args.startsWith('--fast')) { - const modelName = args.replace('--fast', '').trim(); + const fastMatch = args.match(/^--fast(?:\s+(.+))?$/); + if (fastMatch) { + const modelName = fastMatch[1]?.trim() ?? ''; if (!modelName) { // Open model dialog in fast-model mode ``` This ensures `--fast` is either standalone or followed by whitespace and a model name. </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/ui/commands/modelCommand.ts` around lines 48 - 77, The current args parsing around context.invocation?.args in the model command incorrectly treats any occurrence of "--fast" (e.g., "--fastmodel") as the flag; update the parsing so "--fast" is matched as a discrete token (either alone or followed by whitespace + model name), extract the optional modelName safely, and only treat the remainder as modelName when separated by whitespace; then continue to call getPersistScopeForModelSelection(...) and settings.setValue(...) as before and return the same dialog/message flows. ``` </details> </blockquote></details> </blockquote></details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: Path: .coderabbit.yaml **Review profile**: CHILL **Plan**: Pro **Run ID**: `c100091a-655f-4ba7-8ce5-e821e5f9d93f` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 63cef9d3b22c82a7c38fa5643140c4cf8effbee6 and db370e385d2b6a61726c0c693e10c7f5ad2aa2b8. </details> <details> <summary>⛔ Files ignored due to path filters (1)</summary> * `package-lock.json` is excluded by `!**/package-lock.json` </details> <details> <summary>📒 Files selected for processing (106)</summary> * `docs/design/prompt-suggestion/prompt-suggestion-design.md` * `docs/design/prompt-suggestion/prompt-suggestion-implementation.md` * `docs/design/prompt-suggestion/speculation-design.md` * `docs/superpowers/plans/2026-04-02-task-update-diff-display.md` * `docs/superpowers/specs/2026-04-02-task-update-diff-display.md` * `docs/users/configuration/settings.md` * `docs/users/extension/extension-releasing.md` * `docs/users/extension/introduction.md` * `docs/users/features/_meta.ts` * `docs/users/features/commands.md` * `docs/users/features/followup-suggestions.md` * `docs/users/features/hooks.md` * `docs/users/overview.md` * `integration-tests/hook-integration/hooks.test.ts` * `packages/cli/src/acp-integration/session/Session.test.ts` * `packages/cli/src/acp-integration/session/Session.ts` * `packages/cli/src/commands/auth/handler.ts` * `packages/cli/src/commands/extensions/install.ts` * `packages/cli/src/commands/mcp.test.ts` * `packages/cli/src/commands/mcp.ts` * `packages/cli/src/commands/mcp/reconnect.test.ts` * `packages/cli/src/commands/mcp/reconnect.ts` * `packages/cli/src/config/config.ts` * `packages/cli/src/config/settingsSchema.ts` * `packages/cli/src/gemini.test.tsx` * `packages/cli/src/i18n/locales/de.js` * `packages/cli/src/i18n/locales/en.js` * `packages/cli/src/i18n/locales/ja.js` * `packages/cli/src/i18n/locales/pt.js` * `packages/cli/src/i18n/locales/ru.js` * `packages/cli/src/i18n/locales/zh.js` * `packages/cli/src/services/BuiltinCommandLoader.test.ts` * `packages/cli/src/services/BuiltinCommandLoader.ts` * `packages/cli/src/ui/AppContainer.tsx` * `packages/cli/src/ui/commands/bugCommand.test.ts` * `packages/cli/src/ui/commands/modelCommand.ts` * `packages/cli/src/ui/commands/types.ts` * `packages/cli/src/ui/components/Composer.tsx` * `packages/cli/src/ui/components/DialogManager.tsx` * `packages/cli/src/ui/components/InputPrompt.test.tsx` * `packages/cli/src/ui/components/InputPrompt.tsx` * `packages/cli/src/ui/components/ModelDialog.tsx` * `packages/cli/src/ui/components/hooks/HooksDisabledStep.test.tsx` * `packages/cli/src/ui/components/hooks/HooksDisabledStep.tsx` * `packages/cli/src/ui/components/hooks/HooksManagementDialog.test.tsx` * `packages/cli/src/ui/components/hooks/HooksManagementDialog.tsx` * `packages/cli/src/ui/components/hooks/constants.ts` * `packages/cli/src/ui/components/hooks/index.ts` * `packages/cli/src/ui/components/hooks/types.ts` * `packages/cli/src/ui/contexts/KeypressContext.tsx` * `packages/cli/src/ui/contexts/UIStateContext.tsx` * `packages/cli/src/ui/hooks/slashCommandProcessor.ts` * `packages/cli/src/ui/hooks/useAttentionNotifications.ts` * `packages/cli/src/ui/hooks/useFollowupSuggestions.tsx` * `packages/cli/src/ui/hooks/useModelCommand.ts` * `packages/cli/src/ui/hooks/useToolScheduler.test.ts` * `packages/cli/src/utils/systemInfo.ts` * `packages/cli/src/utils/systemInfoFields.test.ts` * `packages/cli/src/utils/systemInfoFields.ts` * `packages/cli/tsconfig.json` * `packages/core/src/config/config.test.ts` * `packages/core/src/config/config.ts` * `packages/core/src/core/client.test.ts` * `packages/core/src/core/client.ts` * `packages/core/src/core/coreToolScheduler.ts` * `packages/core/src/core/geminiChat.ts` * `packages/core/src/extension/extensionManager.ts` * `packages/core/src/extension/github.ts` * `packages/core/src/extension/index.ts` * `packages/core/src/extension/marketplace.test.ts` * `packages/core/src/extension/marketplace.ts` * `packages/core/src/extension/npm.test.ts` * `packages/core/src/extension/npm.ts` * `packages/core/src/followup/followupState.test.ts` * `packages/core/src/followup/followupState.ts` * `packages/core/src/followup/forkedQuery.test.ts` * `packages/core/src/followup/forkedQuery.ts` * `packages/core/src/followup/index.ts` * `packages/core/src/followup/overlayFs.test.ts` * `packages/core/src/followup/overlayFs.ts` * `packages/core/src/followup/smoke.test.ts` * `packages/core/src/followup/speculation.test.ts` * `packages/core/src/followup/speculation.ts` * `packages/core/src/followup/speculationToolGate.test.ts` * `packages/core/src/followup/speculationToolGate.ts` * `packages/core/src/followup/suggestionGenerator.test.ts` * `packages/core/src/followup/suggestionGenerator.ts` * `packages/core/src/hooks/hookRegistry.test.ts` * `packages/core/src/hooks/hookRegistry.ts` * `packages/core/src/hooks/hookRunner.ts` * `packages/core/src/index.ts` * `packages/core/src/services/chatCompressionService.test.ts` * `packages/core/src/services/chatCompressionService.ts` * `packages/core/src/telemetry/constants.ts` * `packages/core/src/telemetry/loggers.ts` * `packages/core/src/telemetry/types.ts` * `packages/core/src/tools/mcp-tool.test.ts` * `packages/core/src/tools/mcp-tool.ts` * `packages/webui/package.json` * `packages/webui/src/components/layout/InputForm.tsx` * `packages/webui/src/followup.ts` * `packages/webui/src/hooks/useFollowupSuggestions.ts` * `packages/webui/src/index.ts` * `packages/webui/src/styles/components.css` * `packages/webui/vite.config.followup.ts` * `packages/webui/vite.config.ts` </details> <details> <summary>💤 Files with no reviewable changes (2)</summary> * packages/cli/src/commands/auth/handler.ts * packages/cli/src/gemini.test.tsx </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
Comment on lines
+96
to
+107
| try { | ||
| const config = await createMinimalConfig(); | ||
| const toolRegistry = config.getToolRegistry(); | ||
| await toolRegistry.discoverToolsForServer(serverName); | ||
| writeStdoutLine(`Successfully reconnected to server "${serverName}".`); | ||
| await config.shutdown(); | ||
| } catch (error) { | ||
| const message = error instanceof Error ? error.message : String(error); | ||
| throw createReconnectError( | ||
| `Failed to reconnect to server "${serverName}": ${message}`, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Always shut Config down on the error path.
If discoverToolsForServer() throws, this function rethrows a ReconnectError without ever calling config.shutdown(). That leaks whatever state/resources initialize() created for the reconnect session.
Suggested fix
async function reconnectMcpServer(serverName: string): Promise<void> {
const mcpServers = await getMcpServersFromConfig();
@@
- try {
- const config = await createMinimalConfig();
+ let config: Config | undefined;
+ try {
+ config = await createMinimalConfig();
const toolRegistry = config.getToolRegistry();
await toolRegistry.discoverToolsForServer(serverName);
writeStdoutLine(`Successfully reconnected to server "${serverName}".`);
- await config.shutdown();
} catch (error) {
const message = error instanceof Error ? error.message : String(error);
throw createReconnectError(
`Failed to reconnect to server "${serverName}": ${message}`,
);
+ } finally {
+ if (config) {
+ await config.shutdown();
+ }
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| try { | |
| const config = await createMinimalConfig(); | |
| const toolRegistry = config.getToolRegistry(); | |
| await toolRegistry.discoverToolsForServer(serverName); | |
| writeStdoutLine(`Successfully reconnected to server "${serverName}".`); | |
| await config.shutdown(); | |
| } catch (error) { | |
| const message = error instanceof Error ? error.message : String(error); | |
| throw createReconnectError( | |
| `Failed to reconnect to server "${serverName}": ${message}`, | |
| ); | |
| } | |
| let config: Config | undefined; | |
| try { | |
| config = await createMinimalConfig(); | |
| const toolRegistry = config.getToolRegistry(); | |
| await toolRegistry.discoverToolsForServer(serverName); | |
| writeStdoutLine(`Successfully reconnected to server "${serverName}".`); | |
| } catch (error) { | |
| const message = error instanceof Error ? error.message : String(error); | |
| throw createReconnectError( | |
| `Failed to reconnect to server "${serverName}": ${message}`, | |
| ); | |
| } finally { | |
| if (config) { | |
| await config.shutdown(); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/commands/mcp/reconnect.ts` around lines 96 - 107, The
try/catch path can leak resources because createMinimalConfig()/config may never
be shut down if discoverToolsForServer() throws; ensure the Config returned by
createMinimalConfig() is always shut down by moving config declaration outside
the try and calling await config.shutdown() in a finally block (or add a finally
that checks config truthiness and awaits config.shutdown()); keep the existing
createReconnectError throw in the catch but still perform shutdown before
rethrowing, referencing createMinimalConfig, config.shutdown,
discoverToolsForServer, and createReconnectError to locate the fix.
Comment on lines
+133
to
+146
| for (const serverName of serverNames) { | ||
| try { | ||
| await toolRegistry.discoverToolsForServer(serverName); | ||
| writeStdoutLine(`✓ ${serverName}: Reconnected successfully`); | ||
| } catch (error) { | ||
| const message = error instanceof Error ? error.message : String(error); | ||
| writeStdoutLine(`✗ ${serverName}: Failed - ${message}`); | ||
| } | ||
| } | ||
| } finally { | ||
| if (config) { | ||
| await config.shutdown(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Return a non-zero status when any --all reconnect fails.
The loop logs ✗ ... Failed lines but then falls through as success, so mcp reconnect --all exits 0 even if one or more servers could not reconnect. That makes the new command unreliable for automation.
Suggested fix
let config: Config | undefined;
+ let hadFailures = false;
try {
config = await createMinimalConfig();
const toolRegistry = config.getToolRegistry();
for (const serverName of serverNames) {
try {
await toolRegistry.discoverToolsForServer(serverName);
writeStdoutLine(`✓ ${serverName}: Reconnected successfully`);
} catch (error) {
+ hadFailures = true;
const message = error instanceof Error ? error.message : String(error);
writeStdoutLine(`✗ ${serverName}: Failed - ${message}`);
}
}
+
+ if (hadFailures) {
+ throw createReconnectError(
+ 'One or more MCP servers failed to reconnect.',
+ );
+ }
} finally {
if (config) {
await config.shutdown();
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/commands/mcp/reconnect.ts` around lines 133 - 146, The
reconnect loop currently logs failures but always exits 0; modify the code
around the serverNames loop (the try/catch using
toolRegistry.discoverToolsForServer and writeStdoutLine) to record failures
(e.g., a boolean like hadFailure or an integer failureCount set in the catch
block) and, after the loop completes (but before/after the finally that calls
config.shutdown), set process.exitCode = 1 if any failures occurred (or throw a
new Error if the command framework expects thrown errors) so `mcp reconnect
--all` returns a non-zero status when one or more servers failed to reconnect.
Comment on lines
+953
to
+955
| if (result.nextSuggestion) { | ||
| setPromptSuggestion(result.nextSuggestion); | ||
| } |
There was a problem hiding this comment.
Clear the accepted suggestion when there's no pipelined replacement.
This path never transitions back through Responding, so leaving promptSuggestion untouched here keeps the just-executed ghost prompt visible and lets the user submit it again on the next Enter.
Suggested fix
- if (result.nextSuggestion) {
- setPromptSuggestion(result.nextSuggestion);
- }
+ setPromptSuggestion(result.nextSuggestion ?? null);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (result.nextSuggestion) { | |
| setPromptSuggestion(result.nextSuggestion); | |
| } | |
| setPromptSuggestion(result.nextSuggestion ?? null); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/ui/AppContainer.tsx` around lines 953 - 955, When handling
the result where you currently only setPromptSuggestion(result.nextSuggestion)
if truthy, add an explicit else branch to clear the accepted suggestion (e.g.,
call setPromptSuggestion(undefined) or setPromptSuggestion('')) so the previous
ghost prompt is removed when there's no pipelined replacement; update the block
that checks result.nextSuggestion in AppContainer.tsx (the setPromptSuggestion
call) to clear the state in the else path because this code path never
transitions back through the Responding state.
Comment on lines
+1175
to
+1192
| if ( | ||
| followupSuggestionsEnabled && | ||
| config.isInteractive() && | ||
| !config.getSdkMode() && | ||
| prevStreamingStateRef.current === StreamingState.Responding && | ||
| streamingState === StreamingState.Idle && | ||
| // Check both committed history and pending items for errors | ||
| // (API errors go to pendingGeminiHistoryItems, not historyManager.history) | ||
| historyManager.history[historyManager.history.length - 1]?.type !== | ||
| 'error' && | ||
| !pendingGeminiHistoryItems.some((item) => item.type === 'error') && | ||
| !shellConfirmationRequest && | ||
| !confirmationRequest && | ||
| !loopDetectionConfirmationRequest && | ||
| !isPermissionsDialogOpen && | ||
| settingInputRequests.length === 0 && | ||
| config.getApprovalMode() !== ApprovalMode.PLAN | ||
| ) { |
There was a problem hiding this comment.
Guard follow-up generation behind the full dialog/modal state.
This condition only blocks a subset of interactive overlays. Theme/model/auth/update/feedback/plugin dialogs can still satisfy it, which means we'll launch another LLM request—and possibly speculation—behind an open modal. Reuse the existing aggregate dialog visibility guard here instead of hand-picking a few flags.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/ui/AppContainer.tsx` around lines 1175 - 1192, The long
conditional that gates followup suggestion generation (the if starting with
followupSuggestionsEnabled && config.isInteractive() ...) manually re-checks
many individual flags (shellConfirmationRequest, confirmationRequest,
loopDetectionConfirmationRequest, isPermissionsDialogOpen, settingInputRequests,
and others) and can still allow followups while other modal/dialog overlays are
open; replace those hand-picked checks with the existing aggregate dialog/modal
visibility guard used elsewhere in the app (the single boolean/function that
indicates any dialog/modal is open — e.g., isAnyDialogOpen/isAnyModalOpen) so
the if condition uses that guard (negated) instead of the individual flags,
keeping the other checks (followupSuggestionsEnabled, config.isInteractive(),
!config.getSdkMode(), prevStreamingStateRef/current StreamingState checks,
history/pending error checks, and config.getApprovalMode()) unchanged.
Comment on lines
+243
to
+249
| const followup = useFollowupSuggestionsCLI({ | ||
| onAccept: (suggestion) => { | ||
| buffer.insert(suggestion); | ||
| }, | ||
| config, | ||
| isFocused: isShellFocused, | ||
| }); |
There was a problem hiding this comment.
Don't reinsert the accepted text after Enter submits it.
followup.accept('enter') queues onAccept, and onAccept always does buffer.insert(suggestion). On the Enter path you then call handleSubmitAndClear(text) immediately, so the queued insert runs after the submit/clear and leaves the just-submitted suggestion sitting back in the prompt. The submit path needs to bypass that insert or move the submit into the accept callback.
Also applies to: 977-986
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/ui/components/InputPrompt.tsx` around lines 243 - 249, The
enter-submit workflow reinserts accepted followups because
useFollowupSuggestionsCLI's onAccept always calls buffer.insert(suggestion) and
followup.accept('enter') queues that before the submit; fix by making the Enter
submission bypass or coordinate with the accept handler: either (a) detect the
Enter path in followup.accept('enter') and call handleSubmitAndClear(text) from
the accept callback instead of queueing buffer.insert, or (b) change onAccept in
the useFollowupSuggestionsCLI call to conditionally insert only for non-Enter
acceptances (e.g., expose an accept reason or flag) so that the Enter case does
not call buffer.insert(suggestion) before handleSubmitAndClear; modify the
followup.accept('enter') usage and the onAccept callback in InputPrompt to
implement one of these options (references: followup.accept('enter'),
useFollowupSuggestionsCLI, onAccept, buffer.insert, handleSubmitAndClear).
Comment on lines
+210
to
+212
| const newTool = toolRegistry.getTool( | ||
| `mcp__${this.serverName}__${this.serverToolName}`, | ||
| ); |
There was a problem hiding this comment.
Look the rediscovered tool up by its normalized name.
DiscoveredMCPTool registers under generateValidName(...), but this lookup uses the raw mcp__${server}__${tool} string. Any server/tool name that needed sanitizing or truncation will never be found after rediscovery, so auto-reconnect silently falls back to the original failure.
Suggested fix
- const newTool = toolRegistry.getTool(
- `mcp__${this.serverName}__${this.serverToolName}`,
- );
+ const newTool = toolRegistry.getTool(
+ generateValidName(`mcp__${this.serverName}__${this.serverToolName}`),
+ );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/tools/mcp-tool.ts` around lines 210 - 212, The lookup uses
the raw `mcp__${this.serverName}__${this.serverToolName}` string but
`DiscoveredMCPTool` registers under the normalized name from
`generateValidName(...)`, so replace the raw key with the normalized name when
calling `toolRegistry.getTool(...)`: compute the generated name via
`generateValidName(this.serverName, this.serverToolName)` (or call the same
normalization function used by `DiscoveredMCPTool`) and use that value to get
the tool (the `newTool` lookup) so rediscovered tools with sanitized/truncated
names are found.
Comment on lines
+228
to
+265
| private async handleReconnectOnError( | ||
| error: unknown, | ||
| signal: AbortSignal, | ||
| updateOutput?: (output: ToolResultDisplay) => void, | ||
| ): Promise<ToolResult> { | ||
| debugLogger.error(`MCP server error '${this.serverName}': ${error}`); | ||
|
|
||
| if (this.retryCount < DiscoveredMCPToolInvocation.MAX_RECONNECT_RETRIES) { | ||
| debugLogger.info( | ||
| `Reconnection attempt ${this.retryCount + 1}/${DiscoveredMCPToolInvocation.MAX_RECONNECT_RETRIES} for MCP server '${this.serverName}'`, | ||
| ); | ||
| const newTool = await this.attemptReconnect(); | ||
| if (newTool) { | ||
| const newInvocation = new DiscoveredMCPToolInvocation( | ||
| newTool['mcpTool'], | ||
| this.serverName, | ||
| this.serverToolName, | ||
| this.displayName, | ||
| this.trust, | ||
| this.params, | ||
| this.cliConfig, | ||
| newTool['mcpClient'], | ||
| this.mcpTimeout, | ||
| this.annotations, | ||
| this.retryCount + 1, | ||
| ); | ||
| return newInvocation.execute(signal, updateOutput); | ||
| } | ||
| } else if ( | ||
| this.retryCount >= DiscoveredMCPToolInvocation.MAX_RECONNECT_RETRIES | ||
| ) { | ||
| debugLogger.error( | ||
| `Max reconnection attempts (${DiscoveredMCPToolInvocation.MAX_RECONNECT_RETRIES}) reached for MCP server '${this.serverName}'`, | ||
| ); | ||
| } | ||
|
|
||
| throw error; | ||
| } |
There was a problem hiding this comment.
Don't auto-retry every failed MCP invocation.
This path reconnects and re-executes the tool for any thrown error, including aborts and ordinary tool/runtime failures, and it does so without checking whether the tool is safe to replay. If the first call reached the server and only the transport failed on the way back, retrying a non-idempotent MCP tool can duplicate side effects. Restrict reconnect/retry to verified connection-loss cases and only auto-reexecute tools that are explicitly read-only or idempotent.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/tools/mcp-tool.ts` around lines 228 - 265, The handler
handleReconnectOnError currently retries for any thrown error; change it to only
attempt reconnect/re-execution when the error is a verified connection-loss
(e.g., network/transport errors, timeouts, ECONNRESET) and the tool is known
safe to replay (check a read-only/idempotent flag on the MCP tool or
annotations, e.g., this.mcpTool.isReadOnly or this.annotations?.idempotent).
Also immediately rethrow when signal.aborted or when the error is not a
connection-loss, or when the tool is not marked safe to replay, so
non-idempotent or abort scenarios do not get retried; keep attemptReconnect and
the DiscoveredMCPToolInvocation re-execution only under the verified conditions.
Comment on lines
47
to
+55
| "peerDependencies": { | ||
| "@qwen-code/qwen-code-core": ">=0.13.1", | ||
| "react": "^18.0.0 || ^19.0.0", | ||
| "react-dom": "^18.0.0 || ^19.0.0" | ||
| }, | ||
| "peerDependenciesMeta": { | ||
| "@qwen-code/qwen-code-core": { | ||
| "optional": true | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- package versions ---'
find packages -name package.json -print0 | xargs -0 -n1 jq -r '.name + "@" + .version'
echo
echo '--- core imports used by the followup entrypoint ---'
rg -n '@qwen-code/qwen-code-core' \
packages/webui/src/followup.ts \
packages/webui/src/hooks/useFollowupSuggestions.ts \
packages/webui/vite.config.followup.tsRepository: protoLabsAI/protoCLI
Length of output: 997
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== useFollowupSuggestions.ts imports ==="
sed -n '1,30p' packages/webui/src/hooks/useFollowupSuggestions.ts
echo
echo "=== followup.ts imports ==="
sed -n '1,30p' packages/webui/src/followup.ts
echo
echo "=== Checking if followup module has guards/optional logic ==="
rg -A 5 -B 5 'try.*@qwen-code/qwen-code-core|optional.*@qwen-code/qwen-code-core|catch.*@qwen-code/qwen-code-core' packages/webui/src/Repository: protoLabsAI/protoCLI
Length of output: 1619
Remove optional: true from the core peer dependency or refactor the hard imports to be conditional.
The /followup entrypoint has hard runtime imports from @qwen-code/qwen-code-core (specifically INITIAL_FOLLOWUP_STATE and createFollowupController in useFollowupSuggestions.ts). Setting optional: true suppresses npm install warnings despite these dependencies being required at runtime, contradicting the file's stated intent to avoid forcing installation. Additionally, the version constraint >=0.13.1 is loose for coordinated 0.x packages; with both @qwen-code/webui and @qwen-code/qwen-code-core at 0.14.0, use a tighter constraint like ^0.14.0 or >=0.14.0.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/webui/package.json` around lines 47 - 55, Remove "optional": true
for the `@qwen-code/qwen-code-core` peer dependency in package.json or make its
runtime imports conditional: either delete the peerDependenciesMeta entry so the
core peer dep is required and tighten the version constraint from ">=0.13.1" to
a coordinated range such as "^0.14.0" (or ">=0.14.0"), or if you truly want it
optional, refactor the hard imports in useFollowupSuggestions.ts (the symbols
INITIAL_FOLLOWUP_STATE and createFollowupController) to be dynamically
imported/guarded so the code only requires the module when present.
Comment on lines
+247
to
+285
| // Tab to accept prompt suggestion (only when callback is wired) | ||
| if ( | ||
| e.key === 'Tab' && | ||
| hasFollowup && | ||
| onAcceptFollowup && | ||
| !inputText && | ||
| !completionActive | ||
| ) { | ||
| e.preventDefault(); | ||
| e.stopPropagation(); | ||
| onAcceptFollowup('tab'); | ||
| return; | ||
| } | ||
| // Right arrow to accept prompt suggestion (only when callback is wired) | ||
| if ( | ||
| e.key === 'ArrowRight' && | ||
| hasFollowup && | ||
| onAcceptFollowup && | ||
| !inputText && | ||
| !completionActive | ||
| ) { | ||
| e.preventDefault(); | ||
| onAcceptFollowup?.('right'); | ||
| return; | ||
| } | ||
| // If composing (Chinese IME input), don't process Enter key | ||
| if (e.key === 'Enter' && !e.shiftKey && !isComposing) { | ||
| // If CompletionMenu is open, let it handle Enter key | ||
| if (completionActive) { | ||
| return; | ||
| } | ||
| // Accept and submit prompt suggestion on Enter when input is empty | ||
| if (hasFollowup && !inputText && followupSuggestion) { | ||
| e.preventDefault(); | ||
| onAcceptFollowup?.('enter'); | ||
| // Pass suggestion text explicitly — onInputChange is async (React setState) | ||
| // so onSubmit cannot rely on reading inputText from the closure. | ||
| onSubmit(e, followupSuggestion); | ||
| return; |
There was a problem hiding this comment.
Make follow-up acceptance available to non-keyboard submit paths.
This only wires Tab/ArrowRight/Enter. A visible follow-up still isn't submitted through the normal form path, and the send button remains tied to real draft text, so mouse/touch users can't accept the suggestion at all. Treat hasFollowup + empty input as submittable content and pass followupSuggestion through the regular submit handler too.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/webui/src/components/layout/InputForm.tsx` around lines 247 - 285,
The current code only accepts follow-up suggestions via keyboard handlers;
update the normal submit path so mouse/touch submissions accept a follow-up when
inputText is empty and hasFollowup is true: in the form submit handler
(onSubmit) and the send-button click handler, detect if hasFollowup &&
!inputText && followupSuggestion, then call onAcceptFollowup('submit' or
similar) and pass followupSuggestion into the existing onSubmit flow (i.e.,
invoke the same submit logic with followupSuggestion instead of relying on
inputText). Also ensure the send button enabled/disabled logic treats
hasFollowup+empty-as-submittable so the button is clickable for accepting
suggestions. Reference symbols: hasFollowup, followupSuggestion, onSubmit,
onAcceptFollowup, inputText, completionActive.
- ideCommand.test: remove install subcommand tests (no longer in ideCommand) - constants.test: update DISPLAY_HOOK_EVENTS length from 12→15 (3 new hook events) - Footer.test: mock VoiceMicButton to avoid UIActionsProvider dep, add voice state fields to mock UIState, update snapshots - client.test: add getDisableAllHooks mock for hooks cleanup change Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/cli/src/ui/commands/ideCommand.test.ts (1)
13-22: Remove unusedgetIdeInstallermock.The
getIdeInstallermock at line 17 is no longer used after theinstallsubcommand tests were removed. This is dead code that should be cleaned up for clarity.♻️ Proposed cleanup
vi.mock('@qwen-code/qwen-code-core', async (importOriginal) => { const original = await importOriginal<typeof core>(); return { ...original, - getIdeInstaller: vi.fn(original.getIdeInstaller), IdeClient: { getInstance: vi.fn(), }, }; });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/ui/commands/ideCommand.test.ts` around lines 13 - 22, The getIdeInstaller mock is unused and should be removed from the vi.mock block: in the mocked module return object remove the getIdeInstaller property (leaving the spread of original and the mocked IdeClient.getInstance) so the mock only defines what the tests actually use (e.g., IdeClient.getInstance) and avoid introducing dead code; update the vi.mock callback to no longer reference getIdeInstaller.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/cli/src/ui/components/hooks/constants.test.ts`:
- Around line 179-180: Update the test to remove the stale hardcoded count and
assert the DISPLAY_HOOK_EVENTS array is in sync with the HookEventName enum:
change the case description from "should have 12 events" to something like
"should match HookEventName count" and replace
expect(DISPLAY_HOOK_EVENTS).toHaveLength(15) with
expect(DISPLAY_HOOK_EVENTS).toHaveLength(Object.values(HookEventName).length),
referencing DISPLAY_HOOK_EVENTS and HookEventName to ensure the test stays
correct when the enum changes.
---
Nitpick comments:
In `@packages/cli/src/ui/commands/ideCommand.test.ts`:
- Around line 13-22: The getIdeInstaller mock is unused and should be removed
from the vi.mock block: in the mocked module return object remove the
getIdeInstaller property (leaving the spread of original and the mocked
IdeClient.getInstance) so the mock only defines what the tests actually use
(e.g., IdeClient.getInstance) and avoid introducing dead code; update the
vi.mock callback to no longer reference getIdeInstaller.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d67dedc5-1952-43c5-bed9-a7b3e03a0f0d
⛔ Files ignored due to path filters (1)
packages/cli/src/ui/components/__snapshots__/Footer.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (4)
packages/cli/src/ui/commands/ideCommand.test.tspackages/cli/src/ui/components/Footer.test.tsxpackages/cli/src/ui/components/hooks/constants.test.tspackages/core/src/core/client.test.ts
✅ Files skipped from review due to trivial changes (1)
- packages/core/src/core/client.test.ts
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. |
- constants.test: replace hardcoded count with Object.values(HookEventName).length so the test stays correct when the enum grows - ideCommand.test: remove unused getIdeInstaller mock from vi.mock block Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- coreToolScheduler.test + nonInteractiveToolExecutor.test: add getDisableAllHooks mock to all mock configs (hooks cleanup change) - mcp-tool.ts: restore output truncation via truncateParts helper using existing truncateToolOutput utility - mcp-tool.ts: gate auto-reconnect on isConnectionError check so generic errors (Invalid parameters) don't trigger reconnect - extension/npm.ts: fix CodeQL alerts — use replaceAll for '/' encoding in scoped package names Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/core/src/extension/npm.ts (2)
285-286:⚠️ Potential issue | 🟡 MinorInconsistent encoding: use
replaceAllhere to match line 374.This line uses
replace('/', '%2f')which only replaces the first occurrence, whilecheckNpmUpdateat line 374 correctly usesreplaceAll. The commit message indicates this was intended to be fixed. While scoped packages typically have only one/, usingreplaceAllis safer and maintains consistency.🔧 Proposed fix
- const encodedName = name.replace('/', '%2f'); + const encodedName = name.replaceAll('/', '%2f');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/extension/npm.ts` around lines 285 - 286, The encoding of package names is inconsistent: change the creation of encodedName (used to build metadataUrl) to use name.replaceAll('/', '%2f') instead of name.replace('/', '%2f') so all slashes are encoded (matching the checkNpmUpdate implementation) and ensuring scoped or multi-slash names are handled consistently.
195-201:⚠️ Potential issue | 🔴 CriticalAuth token leakage on cross-origin redirects remains unaddressed.
The redirect handling still passes
authTokento any redirect target verbatim. A malicious or misconfigured registry could redirect to a different host (or downgrade tohttp://), leaking private registry credentials. RelativeLocationheaders are also not resolved against the current URL.🔒️ Recommended fix for secure redirect handling
if (res.statusCode === 301 || res.statusCode === 302) { if (res.headers.location) { - fetchNpmJson<T>(res.headers.location, authToken) + const nextUrl = new URL(res.headers.location, url).toString(); + const next = new URL(nextUrl); + const current = new URL(url); + const forwardedToken = + next.protocol === 'https:' && next.origin === current.origin + ? authToken + : undefined; + fetchNpmJson<T>(nextUrl, forwardedToken) .then(resolve) .catch(reject); return; } }
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/extension/npm.ts`:
- Around line 257-262: The file write stream created as file =
fs.createWriteStream(dest) lacks an error handler, so if the stream errors the
surrounding Promise (the resolve/reject used with res.pipe) never settles; add a
file.on('error', ...) handler that closes/destroys the stream and calls
reject(err) to propagate the failure (mirror the existing file.on('finish', ...)
behavior) so writes failing during res.pipe(...) properly reject the Promise.
---
Duplicate comments:
In `@packages/core/src/extension/npm.ts`:
- Around line 285-286: The encoding of package names is inconsistent: change the
creation of encodedName (used to build metadataUrl) to use name.replaceAll('/',
'%2f') instead of name.replace('/', '%2f') so all slashes are encoded (matching
the checkNpmUpdate implementation) and ensuring scoped or multi-slash names are
handled consistently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3e48ba9d-3487-487a-adf0-93c3cbb730a8
📒 Files selected for processing (4)
packages/core/src/core/coreToolScheduler.test.tspackages/core/src/core/nonInteractiveToolExecutor.test.tspackages/core/src/extension/npm.tspackages/core/src/tools/mcp-tool.ts
✅ Files skipped from review due to trivial changes (2)
- packages/core/src/core/nonInteractiveToolExecutor.test.ts
- packages/core/src/core/coreToolScheduler.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/core/src/tools/mcp-tool.ts
- Add file.on('error') handler to write stream in downloadNpmTarball so
stream errors during res.pipe() properly reject the Promise instead of
hanging forever
- Fix name.replace('/', '%2f') → replaceAll in downloadFromNpmRegistry
(line 285 was missed in previous commit; line 374 already correct)
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/core/src/extension/npm.ts (2)
42-62:⚠️ Potential issue | 🟠 MajorUnscoped npm packages are still rejected.
parseNpmPackageSource()still hard-requires@scope/name[...], and the helper below is scoped-only too. That means the unscoped npm install flow described in this PR still gets rejected before any registry metadata is fetched. When you relax this parser, make the later scope extraction conditional as follow-up.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/extension/npm.ts` around lines 42 - 62, The parser currently rejects unscoped packages; update parseNpmPackageSource to accept both scoped (`@scope/name`[`@version`]) and unscoped (name[`@version`]) forms by using a regex that captures either full package name (with optional scope) and an optional version, return {name, version} for both, and replace the current strict scoped error with a generic "Invalid npm package source" message; keep isScopedNpmPackage focused on detecting scoped packages only (e.g., test for leading @) so any later scope extraction logic can be made conditional based on that helper.
195-199:⚠️ Potential issue | 🔴 CriticalRedirects are still followed unsafely.
These recursive calls reuse
Locationverbatim and forward the bearer token unchanged. Relative redirects break, and cross-origin or protocol-downgraded redirects can leak private registry credentials.Safer redirect handling
- fetchNpmJson<T>(res.headers.location, authToken) + const nextUrl = new URL(res.headers.location, url).toString(); + const current = new URL(url); + const next = new URL(nextUrl); + const forwardedToken = + next.protocol === 'https:' && next.origin === current.origin + ? authToken + : undefined; + res.resume(); + fetchNpmJson<T>(nextUrl, forwardedToken) .then(resolve) .catch(reject);- downloadNpmFile(res.headers.location, dest, authToken) + const nextUrl = new URL(res.headers.location, url).toString(); + const current = new URL(url); + const next = new URL(nextUrl); + const forwardedToken = + next.protocol === 'https:' && next.origin === current.origin + ? authToken + : undefined; + res.resume(); + downloadNpmFile(nextUrl, dest, forwardedToken) .then(resolve) .catch(reject);Also applies to: 242-246
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/extension/npm.ts` around lines 195 - 199, The redirect handling in fetchNpmJson is unsafe: it forwards the Location header verbatim and always re-sends the bearer token, which breaks relative redirects and can leak credentials on cross-origin or protocol-downgrade redirects. Fix fetchNpmJson by resolving relative Location headers against the original request URL (use a URL constructor with the previous request as base), introduce a redirect limit parameter to avoid infinite recursion, and strip the Authorization header when the redirect target has a different origin or a downgraded protocol (e.g., https -> http); only forward the token for same-origin, same-protocol redirects. Update the recursive call sites in fetchNpmJson to pass the resolved absolute URL, the adjusted authToken (or null), and the remaining redirect count.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/extension/npm.ts`:
- Around line 145-168: The code currently returns the first .npmrc token match
while iterating lines, which lets a host-wide token mask a more specific
path-scoped token; instead, when scanning npmrcPaths (and inside the loop over
lines) collect all matching tokens (capture entryPrefix and token from match)
into a list, and after reading all files select the token whose entryPrefix
appears earliest in registryPrefixes (registryPrefixes is built from
most-specific to least-specific) — i.e., choose the match with the lowest index
in registryPrefixes — then return that token; update the loop that uses
match/entryPrefix to push candidates rather than immediately return.
---
Duplicate comments:
In `@packages/core/src/extension/npm.ts`:
- Around line 42-62: The parser currently rejects unscoped packages; update
parseNpmPackageSource to accept both scoped (`@scope/name`[`@version`]) and unscoped
(name[`@version`]) forms by using a regex that captures either full package name
(with optional scope) and an optional version, return {name, version} for both,
and replace the current strict scoped error with a generic "Invalid npm package
source" message; keep isScopedNpmPackage focused on detecting scoped packages
only (e.g., test for leading @) so any later scope extraction logic can be made
conditional based on that helper.
- Around line 195-199: The redirect handling in fetchNpmJson is unsafe: it
forwards the Location header verbatim and always re-sends the bearer token,
which breaks relative redirects and can leak credentials on cross-origin or
protocol-downgrade redirects. Fix fetchNpmJson by resolving relative Location
headers against the original request URL (use a URL constructor with the
previous request as base), introduce a redirect limit parameter to avoid
infinite recursion, and strip the Authorization header when the redirect target
has a different origin or a downgraded protocol (e.g., https -> http); only
forward the token for same-origin, same-protocol redirects. Update the recursive
call sites in fetchNpmJson to pass the resolved absolute URL, the adjusted
authToken (or null), and the remaining redirect count.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0ca40341-3e6a-4213-8451-5a0f545734ee
📒 Files selected for processing (1)
packages/core/src/extension/npm.ts
Comment on lines
+145
to
+168
| const parsed = new URL(registryUrl); | ||
| const registryPrefixes: string[] = []; | ||
| const pathSegments = parsed.pathname | ||
| .replace(/\/$/, '') | ||
| .split('/') | ||
| .filter(Boolean); | ||
| for (let i = pathSegments.length; i >= 0; i--) { | ||
| const prefix = pathSegments.slice(0, i).join('/'); | ||
| registryPrefixes.push(`${parsed.host}${prefix ? `/${prefix}` : ''}`); | ||
| } | ||
|
|
||
| for (const npmrcPath of npmrcPaths) { | ||
| try { | ||
| const content = fs.readFileSync(npmrcPath, 'utf-8'); | ||
| const lines = content.split('\n'); | ||
| for (const line of lines) { | ||
| const trimmed = line.trim(); | ||
| // Format: //host[/path]/:_authToken=TOKEN | ||
| const match = trimmed.match(/^\/\/(.+?)\/:_authToken\s*=\s*(.+)/); | ||
| if (match) { | ||
| const entryPrefix = match[1].replace(/\/$/, ''); | ||
| if (registryPrefixes.includes(entryPrefix)) { | ||
| return match[2].trim(); | ||
| } |
There was a problem hiding this comment.
Choose the most specific matching _authToken.
registryPrefixes is built from most-specific to least-specific, but this loop returns the first matching .npmrc line. A host-wide token can therefore mask a path-scoped token on the same registry host, which breaks feeds like Azure Artifacts or Artifactory when both entries exist.
Possible fix
for (const npmrcPath of npmrcPaths) {
try {
const content = fs.readFileSync(npmrcPath, 'utf-8');
const lines = content.split('\n');
+ const tokens = new Map<string, string>();
for (const line of lines) {
const trimmed = line.trim();
// Format: //host[/path]/:_authToken=TOKEN
const match = trimmed.match(/^\/\/(.+?)\/:_authToken\s*=\s*(.+)/);
if (match) {
const entryPrefix = match[1].replace(/\/$/, '');
- if (registryPrefixes.includes(entryPrefix)) {
- return match[2].trim();
- }
+ tokens.set(entryPrefix, match[2].trim());
}
}
+ for (const prefix of registryPrefixes) {
+ const token = tokens.get(prefix);
+ if (token) return token;
+ }
} catch {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/extension/npm.ts` around lines 145 - 168, The code
currently returns the first .npmrc token match while iterating lines, which lets
a host-wide token mask a more specific path-scoped token; instead, when scanning
npmrcPaths (and inside the loop over lines) collect all matching tokens (capture
entryPrefix and token from match) into a list, and after reading all files
select the token whose entryPrefix appears earliest in registryPrefixes
(registryPrefixes is built from most-specific to least-specific) — i.e., choose
the match with the lowest index in registryPrefixes — then return that token;
update the loop that uses match/entryPrefix to push candidates rather than
immediately return.
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
7 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
Backports 10 high-value upstream items from QwenLM/qwen-code into protoCLI.
What's included
Skipped
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
mcp reconnectcommand; npm-based extension install with--registry.Improvements
Documentation