diff --git a/.gemini_security/graphiti.db b/.gemini_security/graphiti.db new file mode 100644 index 00000000000..77f479106bf Binary files /dev/null and b/.gemini_security/graphiti.db differ diff --git a/.gemini_security/pulse.db b/.gemini_security/pulse.db new file mode 100644 index 00000000000..744a8be3f49 Binary files /dev/null and b/.gemini_security/pulse.db differ diff --git a/.gemini_security/second_brain.db b/.gemini_security/second_brain.db new file mode 100644 index 00000000000..3cf722893b4 Binary files /dev/null and b/.gemini_security/second_brain.db differ diff --git a/.serena/.gitignore b/.serena/.gitignore new file mode 100644 index 00000000000..2e510aff585 --- /dev/null +++ b/.serena/.gitignore @@ -0,0 +1,2 @@ +/cache +/project.local.yml diff --git a/.serena/project.yml b/.serena/project.yml new file mode 100644 index 00000000000..30820061022 --- /dev/null +++ b/.serena/project.yml @@ -0,0 +1,119 @@ +# the name by which the project can be referenced within Serena +project_name: "qwen-code" + + +# list of languages for which language servers are started; choose from: +# al ansible bash clojure cpp +# cpp_ccls crystal csharp csharp_omnisharp dart +# elixir elm erlang fortran fsharp +# go groovy haskell haxe hlsl +# java json julia kotlin lean4 +# lua luau markdown matlab msl +# nix ocaml pascal perl php +# php_phpactor powershell python python_jedi python_ty +# r rego ruby ruby_solargraph rust +# scala solidity swift systemverilog terraform +# toml typescript typescript_vts vue yaml +# zig +# (This list may be outdated. For the current list, see values of Language enum here: +# https://github.com/oraios/serena/blob/main/src/solidlsp/ls_config.py +# For some languages, there are alternative language servers, e.g. csharp_omnisharp, ruby_solargraph.) +# Note: +# - For C, use cpp +# - For JavaScript, use typescript +# - For Free Pascal/Lazarus, use pascal +# Special requirements: +# Some languages require additional setup/installations. +# See here for details: https://oraios.github.io/serena/01-about/020_programming-languages.html#language-servers +# When using multiple languages, the first language server that supports a given file will be used for that file. +# The first language is the default language and the respective language server will be used as a fallback. +# Note that when using the JetBrains backend, language servers are not used and this list is correspondingly ignored. +languages: +- typescript + +# the encoding used by text files in the project +# For a list of possible encodings, see https://docs.python.org/3.11/library/codecs.html#standard-encodings +encoding: "utf-8" + +# line ending convention to use when writing source files. +# Possible values: unset (use global setting), "lf", "crlf", or "native" (platform default) +# This does not affect Serena's own files (e.g. memories and configuration files), which always use native line endings. +line_ending: + +# The language backend to use for this project. +# If not set, the global setting from serena_config.yml is used. +# Valid values: LSP, JetBrains +# Note: the backend is fixed at startup. If a project with a different backend +# is activated post-init, an error will be returned. +language_backend: + +# whether to use project's .gitignore files to ignore files +ignore_all_files_in_gitignore: true + +# advanced configuration option allowing to configure language server-specific options. +# Maps the language key to the options. +# Have a look at the docstring of the constructors of the LS implementations within solidlsp (e.g., for C# or PHP) to see which options are available. +# No documentation on options means no options are available. +ls_specific_settings: {} + +# list of additional paths to ignore in this project. +# Same syntax as gitignore, so you can use * and **. +# Note: global ignored_paths from serena_config.yml are also applied additively. +ignored_paths: [] + +# whether the project is in read-only mode +# If set to true, all editing tools will be disabled and attempts to use them will result in an error +# Added on 2025-04-18 +read_only: false + +# list of tool names to exclude. +# This extends the existing exclusions (e.g. from the global configuration) +# Find the list of tools here: https://oraios.github.io/serena/01-about/035_tools.html +excluded_tools: [] + +# list of tools to include that would otherwise be disabled (particularly optional tools that are disabled by default). +# This extends the existing inclusions (e.g. from the global configuration). +# Find the list of tools here: https://oraios.github.io/serena/01-about/035_tools.html +included_optional_tools: [] + +# fixed set of tools to use as the base tool set (if non-empty), replacing Serena's default set of tools. +# This cannot be combined with non-empty excluded_tools or included_optional_tools. +# Find the list of tools here: https://oraios.github.io/serena/01-about/035_tools.html +fixed_tools: [] + +# list of mode names that are to be activated by default, overriding the setting in the global configuration. +# The full set of modes to be activated is base_modes (from global config) + default_modes + added_modes. +# If the setting is undefined/empty, the default_modes from the global configuration (serena_config.yml) apply. +# Otherwise, this overrides the setting from the global configuration (serena_config.yml). +# Therefore, you can set this to [] if you do not want the default modes defined in the global config to apply +# for this project. +# This setting can, in turn, be overridden by CLI parameters (--mode). +# See https://oraios.github.io/serena/02-usage/050_configuration.html#modes +default_modes: + +# list of mode names to be activated additionally for this project, e.g. ["query-projects"] +# The full set of modes to be activated is base_modes (from global config) + default_modes + added_modes. +# See https://oraios.github.io/serena/02-usage/050_configuration.html#modes +added_modes: + +# initial prompt for the project. It will always be given to the LLM upon activating the project +# (contrary to the memories, which are loaded on demand). +initial_prompt: "" + +# time budget (seconds) per tool call for the retrieval of additional symbol information +# such as docstrings or parameter information. +# This overrides the corresponding setting in the global configuration; see the documentation there. +# If null or missing, use the setting from the global configuration. +symbol_info_budget: + +# list of regex patterns which, when matched, mark a memory entry as read‑only. +# Extends the list from the global configuration, merging the two lists. +read_only_memory_patterns: [] + +# list of regex patterns for memories to completely ignore. +# Matching memories will not appear in list_memories or activate_project output +# and cannot be accessed via read_memory or write_memory. +# To access ignored memory files, use the read_file tool on the raw file path. +# Extends the list from the global configuration, merging the two lists. +# Example: ["_archive/.*", "_episodes/.*"] +ignored_memory_patterns: [] diff --git a/RUN2.md b/RUN2.md new file mode 100644 index 00000000000..b5d79a26f5a --- /dev/null +++ b/RUN2.md @@ -0,0 +1,599 @@ +╭─── Claude Code v2.1.109 ─────────────────────────────────────────────────────╮ +│ │ Tips for getting started │ +│ Welcome back! │ Run /init to create a CL… │ +│ │ ───────────────────────── │ +│ ▐▛███▜▌ │ Recent activity │ +│ ▝▜█████▛▘ │ No recent activity │ +│ ▘▘ ▝▝ │ │ +│ │ │ +│ tencent/hy3-preview:free · API Usage Billing │ │ +│ ~/qwen-code │ │ +╰──────────────────────────────────────────────────────────────────────────────╯ +⎿  SessionStart:clear hook error ⎿  Failed with non-blocking status code: bash: /home/bamn/.claude/hooks/gsd-session-state.sh: No such file or directory +⎿  SessionStart:clear hook error ⎿  Failed with non-blocking status code: +⎿  SessionStart:clear hook error ⎿  Failed with non-blocking status code: ⚠️ Semgrep not found. Please + +❯ /clear + ⎿  (no content) + +❯ /prforge:pr +https://github.com/QwenLM/qwen-code/pull/3797#pullrequestreview-4215940090 + ⎿  UserPromptSubmit hook error ⎿  Failed with non-blocking status code: /bin/sh: 1: semgrep: not found + +● I'll start by reading the PRForge skill file and checking for existing state, +then fetch the PR and review details. + +Searched for 5 patterns, read 4 files, listed 1 directory (ctrl+o to expand) +⎿  PreToolUse:Bash hook error ⎿  Failed with non-blocking status code: bash: +⎿  PreToolUse:Bash hook error ⎿  Failed with non-blocking status code: bash: +⎿  PreToolUse:Bash hook error ⎿  Failed with non-blocking status code: bash: + +● Now let me find the skill root and load the required files: + +Searched for 1 pattern (ctrl+o to expand) + +❯ /reload-plugins + ⎿ Reloaded: 28 plugins · 44 skills · 16 agents · 15 hooks · 2 plugin MCP servers · 6 plugin LSP servers +2 errors during load. Run /doctor for details. + +❯ continue + ⎿  UserPromptSubmit hook error ⎿  Failed with non-blocking status code: /bin/sh: 1: semgrep: not found + +● Good, plugins reloaded. Now let me find the skill root and load all required +files. + +Searched for 1 pattern (ctrl+o to expand) +⎿  PreToolUse:Bash hook error ⎿  Failed with non-blocking status code: bash: + +● Now let me load all the required files in parallel: + +Read 4 files (ctrl+o to expand) + +● Now let me load the mode file and the INTAKE phase file, and check for +existing state: + +Read 2 files (ctrl+o to expand) + +● Now let me check for existing state and fetch the PR/review data: + +Read 1 file (ctrl+o to expand) +⎿  PreToolUse:Bash hook error ⎿  Failed with non-blocking status code: bash: + +● Existing state found — phase is VALIDATE. Let me also check what other +artifacts exist and understand the current situation better. The user provided +a different review link (4215940090) than what's in state (4215939438). Let +me check the current state of the branch and fetch the new review. + +Listed 1 directory (ctrl+o to expand) +⎿  PreToolUse:Bash hook error ⎿  Failed with non-blocking status code: bash: + +● Bash(git status --short) +⎿  PreToolUse:Bash hook error ⎿  Failed with non-blocking status code: bash: +⎿   M scripts/unused-keys-only-in-locales.json +?? .gemini_security/ +?? .prforge/ +?? .serena/ + +● Bash(git branch -vv && echo "---" && git remote -v && echo "---" && git log +--oneline --decorate -8) +⎿  PreToolUse:Bash hook error ⎿  Failed with non-blocking status code: bash: +⎿   backup-feat-stats-model-cost-estimation 554dee2c5 test(core): add ti +meout env override tests for OAuth path +feat/model-list-command ab93d73b3 fix(cli): fix i18n +… +40 lines (ctrl+o to expand) + +● Now let me fetch the new review data. The user provided review 4215940090 +which is different from the one in state (4215939438). +● Bash(gh pr view 3799 --json author,title,state,reviewDecision,reviews,comments +,reviewThreads,statusCheckRollup 2>&1) +⎿  Error: Exit code 1 +Unknown JSON field: "reviewThreads" +Available fields: +additions +assignees +author +baseRefName +body +changedFiles +closed +… +32 lines (ctrl+o to see all) + +● Bash(gh pr view 3799 --json +author,title,state,reviewDecision,reviews,comments,statusCheckRollup 2>&1) +⎿  PreToolUse:Bash hook error ⎿  Failed with non-blocking status code: bash: /home/bamn/.claude/hooks/gsd-validate-commit.sh: No such file or directory +⎿  { +"author": { +"login": "B-A-M-N" +… +103 lines (ctrl+o to expand) + +● Now let me also fetch the specific review that was linked (4215940090): + +● Bash(gh api repos/QwenLM/qwen-code/pulls/3799/reviews 2>&1) +⎿  PreToolUse:Bash hook error ⎿  Failed with non-blocking status code: bash: /home/bamn/.claude/hooks/gsd-validate-commit.sh: No such file or directory +⎿  PreToolUse:Bash hook error ⎿  Failed with non-blocking status code: No stderr output +⎿  [ +{ +"id": 4215773687, +… +66 lines (ctrl+o to expand) + +● Now let me fetch the inline review comments (the specific lines being +reviewed): + +● Bash(gh api repos/QwenLM/qwen-code/pulls/3799/comments 2>&1) +⎿  PreToolUse:Bash hook error ⎿  Failed with non-blocking status code: bash: /home/bamn/.claude/hooks/gsd-validate-commit.sh: No such file or directory +⎿  PreToolUse:Bash hook error ⎿  Failed with non-blocking status code: No stderr output +⎿  [{"url":"https://api.github.com/repos/QwenLM/qwen-code/pulls/comments/ +3177513898","pull*request_review_id":4215773687,"id":3177513898,"node* +id":"PRRC_kwDOPB-92c69ZQOq","diff_hunk":"@@ -148,4 +148,137 @@ export +… +426 lines (ctrl+o to expand) + +Read 1 file (ctrl+o to expand) + +● Excellent. Now I have a complete picture. Let me analyze the situation: + +Current state: The existing .prforge/state.json shows phase VALIDATE with all +previous issues marked as "fixed". The user provided a new review link +(4215940090) — but looking at the data, the latest review from wenshao is +4215939438 (the second review). The ID 4215940090 doesn't appear in the API +results — it may be a review that was superseded or the user may have meant +the latest review. + +Looking at the second review (4215939438, submitted at 05:51:06Z), there are +new critical issues that the current code on HEAD does NOT address: + +1. CRIT: AbortError from timeout/user cancel is indistinguishable from real + network errors (line 243/212) +2. CRIT: Empty array from endpoint produces blank output — silent failure + (line 216/200) +3. SUGG: clearTimeout redundancy in try+finally (line 291/260) +4. SUGG: response.text() in error path — the try/catch was added but + console.error suggestion not applied (line 296/265) +5. SUGG: response.json() on success path still has no try-catch (line + null/278) + +Let me read the current source to understand what's actually on HEAD: + +Read 1 file (ctrl+o to expand) + +● Now I have a clear picture. The current code on HEAD already addresses many +issues from the first review, but the second review (4215939438) raised new +issues. Let me check which ones are still open: + +1. ✅ AbortError handling (line 232-241) — Already fixed in current code +2. ✅ Empty array check (line 218-224) — Already fixed in current code +3. ⚠️ clearTimeout redundancy (line 297 + 358) — clearTimeout is called in + both try (line 297) and finally (line 358). The suggestion is to remove the + one in the try block. +4. ✅ response.text() try/catch (line 300-305) — Already fixed +5. ✅ response.json() try/catch on success path (line 309-316) — Already fixed + +So the only remaining item from the second review is the redundant +clearTimeout. Let me also check the test file: + +Read 1 file (ctrl+o to expand) + +● Good. Now I have a complete understanding. Let me set up the .prforge/ +directory and update the state, then proceed with the remaining fix. + +Analysis of remaining issues from the second review (4215939438): + +1. ✅ AbortError handling — already done +2. ✅ Empty array check — already done +3. 🔧 SUGG: Redundant clearTimeout — Line 297 has clearTimeout(timeoutId) in + the try block, and line 358 has it in finally. The one in the try block is + redundant. +4. ✅ response.text() try/catch — already done +5. ✅ response.json() try/catch — already done + +Now let me set up the artifacts and make the fix: + +● Bash(mkdir -p .prforge/snapshots) +⎿  PreToolUse:Bash hook error ⎿  Failed with non-blocking status code: bash: /home/bamn/.claude/hooks/gsd-validate-commit.sh: No such file or directory +⎿  Done + +Searched for 1 pattern (ctrl+o to expand) +⎿  PreToolUse:Bash hook error ⎿  Failed with non-blocking status code: bash: /home/bamn/.claude/hooks/gsd-validate-commit.sh: No such file or directory + +● Bash(echo ".prforge/" >> .gitignore 2>/dev/null; echo "DONE") +⎿  PreToolUse:Bash hook error ⎿  Failed with non-blocking status code: bash: /home/bamn/.claude/hooks/gsd-validate-commit.sh: No such file or directory +⎿  DONE + +● Cannot read properties of undefined (reading 'input_tokens') + +✻ Worked for 11m 27s + +❯ . + ⎿  UserPromptSubmit hook error ⎿  Failed with non-blocking status code: /bin/sh: 1: semgrep: not found + +● Cannot read properties of undefined (reading 'input_tokens') + +✻ Worked for 4m 2s + +❯ . + ⎿  UserPromptSubmit hook error ⎿  Failed with non-blocking status code: /bin/sh: 1: semgrep: not found + +● Cannot read properties of undefined (reading 'input_tokens') + +✻ Sautéed for 3m 15s + +※ recap: Pushed review fixes to PR #3799 (commit `2f7e4c666`) — QWEN_OAUTH authType guard, AbortError timeout/cancel distinction, response.json() and response.text() error handling, empty models message, and i18n translations. All 34 tests pass, i18n and lint are clean. The PR is ready for re-review. (disable recaps in /config) +❯ . + ⎿  UserPromptSubmit hook error ⎿  Failed with non-blocking status code: /bin/sh: 1: semgrep: not found + +● Got it — you want me to just proceed. Let me get to work on the remaining fix +and run the tests. + +The only unaddressed item from the second review is the redundant +clearTimeout(timeoutId) on line 297 (the finally block on line 358 already +handles it). Let me fix that, then validate. + +● Update(packages/cli/src/ui/commands/modelCommand.ts) +⎿  PreToolUse:Edit hook error ⎿  Failed with non-blocking status code: +⎿  PreToolUse:Edit hook error ⎿  Failed with non-blocking status code: +⎿  PreToolUse:Edit hook error ⎿  Failed with non-blocking status code: +⎿  Removed 2 lines +294 headers, +295 signal, +296 }); +297 - clearTimeout(timeoutId); + 298 - + 297 if (!response.ok) { +298 let errorText: string; +299 try { +⎿  PostToolUse:Edit hook error ⎿  Failed with non-blocking status code: bash: +⎿  PostToolUse:Edit hook error ⎿  Failed with non-blocking status code: /bin/sh: 1: semgrep: not found + +● Now let me update the state file and run the tests: + +● Write(.prforge/state.json) +⎿  Error writing file + +Read 1 file (ctrl+o to expand) + +● Write(.prforge/state.json) +⎿  PreToolUse:Write hook error ⎿  Failed with non-blocking status code: +⎿  PreToolUse:Write hook error ⎿  Failed with non-blocking status code: +⎿  PreToolUse:Write hook error ⎿  Failed with non-blocking status code: +⎿  Added 12 lines, removed 6 lines +4 "branch": "feat/model-list-response-normalization", +5 "task": "review_response", +6 "reviewer": "wenshao", +7 - "phase": "VALIDATE", + 7 + "phase": "IMPLEMENT", + 8 "reviewUrl": "https://github.com/QwenLM/qwen-code/pull/3799#p +ullrequestreview-4215939438", +9 "issues": [ +10 { +11 "id": "CRIT-1", +12 "severity": "critical", +13 "status": "fixed", +14 - "description": "QWEN_OAUTH users get 401 because apiKey i +-s sentinel string." + 14 + "description": "QWEN_OAUTH users get 401 because apiKey i ++s sentinel string. Fixed: authType guard added to list subcomma ++nd." + 15 }, +16 { +17 "id": "CRIT-2", +18 "severity": "critical", +19 "status": "fixed", +20 - "description": "AbortError from timeout/user cancel now d +-istinguished." + 20 + "description": "AbortError from timeout/user cancel now d ++istinguished. Fixed: DOMException check in catch block." + 21 }, +22 { +23 + "id": "CRIT-3", + 24 + "severity": "critical", + 25 + "status": "fixed", + 26 + "description": "Empty array from endpoint produces blank ++output. Fixed: models.length === 0 check returns 'No models fou ++nd' message." + 27 + }, + 28 + { + 29 "id": "SUGG-1", +30 "severity": "suggestion", +31 "status": "fixed", +... +38 "description": "response.json() on success path wrapped i +n try/catch." +39 }, +40 { +35 - "id": "CI-1", + 36 - "severity": "ci", + 41 + "id": "SUGG-3", + 42 + "severity": "suggestion", + 43 "status": "fixed", +38 - "description": "i18n translations added for zh.js and zh- +-TW.js." + 44 + "description": "clearTimeout(timeoutId) removed from try ++block — finally already handles it." + 45 } +46 ] +47 } +⎿  PostToolUse:Write hook error ⎿  Failed with non-blocking status code: /bin/sh: 1: semgrep: not found +⎿  PostToolUse:Write hook error ⎿  Failed with non-blocking status code: bash: /home/bamn/.claude/hooks/gsd-phase-boundary.sh: No such file or directory + +● Now let me run the tests: + +● Bash(npx vitest run packages/cli/src/ui/commands/modelCommand.test.ts 2>&1) +⎿  PreToolUse:Bash hook error ⎿  Failed with non-blocking status code: bash: +⎿   RUN v3.2.4 /home/bamn/qwen-code + + ✓ |@qwen-code/qwen-code| src/ui/commands/modelCommand.test.ts (34 tes + … +7 lines (ctrl+o to expand) + +⎿  (timeout 2m) + +● All 34 tests pass. Let me also run the full test suite to make sure nothing +else broke: + +● Bash(npx vitest run --reporter=verbose 2>&1 | tail -40) +⎿  PreToolUse:Bash hook error ⎿  Failed with non-blocking status code: bash: +⎿  Running in the background (↓ to manage) +⎿  (timeout 5m) + +● Task Output bb64xy5ub +⎿  (No output) + +● Task Output bb64xy5ub +⎿  (No output) + +● Full suite is taking too long. Let me check what's happening and just verify +the relevant package tests pass: + +● Stop Task +⎿  npx vitest run --reporter=verbose 2>&1 | tail -40 · stopped + +● The full suite is too broad. The modelCommand tests all pass (34/34). Let me +now commit the change and prepare the review response. + +● Bash(git add packages/cli/src/ui/commands/modelCommand.ts && git commit -m +"$(cat <<'EOF' +fix(cli): remove redundant clearTimeout in fetchModels try block…) +⎿  PreToolUse:Bash hook error ⎿  Failed with non-blocking status code: bash: +⎿  > @qwen-code/qwen-code@0.15.3 pre-commit > node scripts/pre-commit.js +… +19 lines (ctrl+o to expand) +● Now let me verify the commit and prepare the review response: + +● Bash(git log --oneline -3 && echo "---" && git diff upstream/main...HEAD +--stat) +⎿  PreToolUse:Bash hook error ⎿  Failed with non-blocking status code: bash: /home/bamn/.claude/hooks/gsd-validate-commit.sh: No such file +⎿  94ef253ab fix(cli): remove redundant clearTimeout in fetchModels try b +lock +2f7e4c666 fix(cli): address PR #3799 review comments +… +10 lines (ctrl+o to expand) + +● Now let me write the review response and update state to PACKAGE phase: + +● Write(.prforge/review_response.md) +⎿  PreToolUse:Write hook error ⎿  Failed with non-blocking status code: node:internal/modules/cjs/loader:1386 +⎿  PreToolUse:Write hook error ⎿  Failed with non-blocking status code: node:internal/modules/cjs/loader:1386 +⎿  PreToolUse:Write hook error ⎿  Failed with non-blocking status code: node:internal/modules/cjs/loader:1386 +⎿  Wrote 20 lines to .prforge/review_response.md +2 # Maintainer Response Draft +und have been addressed.follow-up review. All items from this ro +5 Items already implemented in the prior push: +6 +distinguishes `AbortError` vs network errors. Timeout produces " +user cancel produces "Request cancelled."w or unreachable." and +8 - **Empty array from endpoint** — `models.length === 0` now retu +stead of a blank string.rom the configured endpoint." message in +rows a clear error if the 200 body isn't valid JSON.ry/catch, th +10 - \*\*response.text() +… +23 lines (ctrl+o to expand) +⎿  PostToolUse:Write hook error ⎿  Failed with non-blocking status code: bash: /home/bamn/.claude/hooks/gsd-phase-boundary.sh: No such file or +⎿  PostToolUse:Write hook error ⎿  Failed with non-blocking status code: /bin/sh: 1: semgrep: not found + +● Write(.prforge/state.json) +⎿  PreToolUse:Write hook error ⎿  Failed with non-blocking status code: node:internal/modules/cjs/loader:1386 +⎿  PreToolUse:Write hook error ⎿  Failed with non-blocking status code: node:internal/modules/cjs/loader:1386 +⎿  PreToolUse:Write hook error ⎿  Failed with non-blocking status code: node:internal/modules/cjs/loader:1386 +⎿  Added 12 lines, removed 6 lines +4 "branch": "feat/model-list-response-normalization", +5 "task": "review_response", +6 "reviewer": "wenshao", +7 - "phase": "IMPLEMENT", + 7 + "phase": "APPROVAL", + 8 "reviewUrl": "https://github.com/QwenLM/qwen-code/pull/3799#p +ullrequestreview-4215939438", +9 + "approval_status": "READY", + 10 "issues": [ +11 { +12 "id": "CRIT-1", +13 "severity": "critical", +14 "status": "fixed", +14 - "description": "QWEN_OAUTH users get 401 because apiKey i +-s sentinel string. Fixed: authType guard added to list subcomma +-nd." + 15 + "description": "QWEN_OAUTH users get 401 — authType guard ++ added to list subcommand." + 16 }, +17 { +18 "id": "CRIT-2", +19 "severity": "critical", +20 "status": "fixed", +20 - "description": "AbortError from timeout/user cancel now d +-istinguished. Fixed: DOMException check in catch block." + 21 + "description": "AbortError distinguished from network err ++ors in catch block." + 22 }, +23 { +24 "id": "CRIT-3", +25 "severity": "critical", +26 "status": "fixed", +26 - "description": "Empty array from endpoint produces blank +-output. Fixed: models.length === 0 check returns 'No models fou +-nd' message." + 27 + "description": "Empty array returns 'No models found' mes ++sage instead of blank." + 28 }, +29 { +30 "id": "SUGG-1", +... +42 "id": "SUGG-3", +43 "severity": "suggestion", +44 "status": "fixed", +44 - "description": "clearTimeout(timeoutId) removed from try +-block — finally already handles it." + 45 + "description": "Redundant clearTimeout removed from try b ++lock." + 46 } +46 - ] + 47 + ], + 48 + "validation": { + 49 + "command": "npx vitest run packages/cli/src/ui/commands/mod ++elCommand.test.ts", + 50 + "result": "34 passed", + 51 + "passed": true + 52 + } + 53 } +⎿  PostToolUse:Write hook error ⎿  Failed with non-blocking status code: /bin/sh: 1: semgrep: not found +⎿  PostToolUse:Write hook error ⎿  Failed with non-blocking status code: bash: /home/bamn/.claude/hooks/gsd-phase-boundary.sh: No such file or + +● Now let me write the approval artifact: + +● Write(.prforge/approval.md) +⎿  PreToolUse:Write hook error ⎿  Failed with non-blocking status code: node:internal/modules/cjs/loader:1386 +⎿  PreToolUse:Write hook error ⎿  Failed with non-blocking status code: node:internal/modules/cjs/loader:1386 +⎿  PreToolUse:Write hook error ⎿  Failed with non-blocking status code: node:internal/modules/cjs/loader:1386 +⎿  Wrote 47 lines to .prforge/approval.md +2 # PRForge Approval — PR #3799 Review Response +3 ## Summary +4 +5 Addressed all remaining items from wenshao's second review (42159 +39438). One code change committed; all other items were already i +6 mplemented in the prior push. +7 ## Change Made +8 +9 **Commit:** `94ef253ab` — `fix(cli): remove redundant clearTimeou + t in fetchModels try block` +… +50 lines (ctrl+o to expand) +⎿  PostToolUse:Write hook error ⎿  Failed with non-blocking status code: /bin/sh: 1: semgrep: not found +⎿  PostToolUse:Write hook error ⎿  Failed with non-blocking status code: bash: /home/bamn/.claude/hooks/gsd-phase-boundary.sh: No such file or + +● All review items from wenshao's second review are now addressed. Here's the +summary: + +One new commit (94ef253ab): Removed the redundant clearTimeout(timeoutId) from +the try block — the finally block already handles it. + +All other items from the second review were already implemented in the prior +push: + +- AbortError vs network error distinction ✅ +- Empty array → "No models found" message ✅ +- response.json() try/catch on success path ✅ +- response.text() try/catch in error path ✅ + +Tests: 34/34 passing. CI was already green on the prior push. + +Ready to push feat/model-list-response-normalization to origin and post the +review response. Want me to proceed? + +✻ Worked for 24m 13s + +※ recap: PRForge is handling review response for PR #3799. All 6 reviewer concerns are now addressed, tests pass (34/34), and a new commit is ready. Next: push to origin and post the review response — awaiting your approval. + +❯ proceed + ⎿  UserPromptSubmit hook error ⎿  Failed with non-blocking status code: /bin/sh: 1: semgrep: not found + +● Bash(git push origin feat/model-list-response-normalization 2>&1) +⎿  PreToolUse:Bash hook error ⎿  Failed with non-blocking status code: bash: /home/bamn/.claude/hooks/gsd-validate-commit.sh: No such file +⎿  PreToolUse:Bash hook error ⎿  Failed with non-blocking status code: No stderr output +⎿  To https://github.com/B-A-M-N/qwen-code.git +2f7e4c666..94ef253ab feat/model-list-response-normalization -> fea +t/model-list-response-normalization + +● Pushed. Now posting the review response: + +● Bash(gh pr comment 3799 --body "$(cat <<'RESPONSE' +Thanks for the follow-up review. All items from this round have been +addressed:…) +⎿  PreToolUse:Bash hook error ⎿  Failed with non-blocking status code: bash: /home/bamn/.claude/hooks/gsd-validate-commit.sh: No such file ⎿  https://github.com/QwenLM/qwen-code/pull/3799#issuecomment-4366260076 + +● Write(.prforge/state.json) +⎿  PreToolUse:Write hook error ⎿  Failed with non-blocking status code: node:internal/modules/cjs/loader:1386 +⎿  PreToolUse:Write hook error ⎿  Failed with non-blocking status code: node:internal/modules/cjs/loader:1386 +⎿  PreToolUse:Write hook error ⎿  Failed with non-blocking status code: node:internal/modules/cjs/loader:1386 +⎿  Added 7 lines, removed 45 lines +4 "branch": "feat/model-list-response-normalization", +5 "task": "review_response", +6 "reviewer": "wenshao", +7 - "phase": "APPROVAL", + 7 + "phase": "SHIPPED", + 8 "reviewUrl": "https://github.com/QwenLM/qwen-code/pull/3799#p +ullrequestreview-4215939438", +9 - "approval_status": "READY", + 10 - "issues": [ + 11 - { + 12 - "id": "CRIT-1", + 13 - "severity": "critical", + 14 - "status": "fixed", + 15 - "description": "QWEN_OAUTH users get 401 — authType guard +- added to list subcommand." + 16 - }, + 17 - { + 18 - "id": "CRIT-2", + 19 - "severity": "critical", + 20 - "status": "fixed", + 21 - "description": "AbortError distinguished from network err +-ors in catch block." + 22 - }, + 23 - { + 24 - "id": "CRIT-3", + 25 - "severity": "critical", + 26 - "status": "fixed", + 27 - "description": "Empty array returns 'No models found' mes +-sage instead of blank." + 28 - }, + 29 - { + 30 - "id": "SUGG-1", + 31 - "severity": "suggestion", + 32 - "status": "fixed", + 33 - "description": "response.text() in error path wrapped in +-try/catch." + 34 - }, + 35 - { + 36 - "id": "SUGG-2", + 37 - "severity": "suggestion", + 38 - "status": "fixed", + 39 - "description": "response.json() on success path wrapped i +-n try/catch." + 40 - }, + 41 - { + 42 - "id": "SUGG-3", + 43 - "severity": "suggestion", + 44 - "status": "fixed", + 45 - "description": "Redundant clearTimeout removed from try b +-lock." + 46 - } + 47 - ], + 48 - "validation": { + 49 - "command": "npx vitest run packages/cli/src/ui/commands/mod +-elCommand.test.ts", + 50 - "result": "34 passed", + 51 - "passed": true + 52 - } + 9 + "approval_status": "SHIPPED", + 10 + "shipped_at": "2026-05-03T07:50:00Z", + 11 + "actions": [ + 12 + "Pushed commit 94ef253ab to origin/feat/model-list-response ++-normalization", + 13 + "Posted review response comment on PR #3799" + 14 + ] + 15 } +⎿  PostToolUse:Write hook error ⎿  Failed with non-blocking status code: /bin/sh: 1: semgrep: not found +⎿  PostToolUse:Write hook error ⎿  Failed with non-blocking status code: bash: /home/bamn/.claude/hooks/gsd-phase-boundary.sh: No such file + +● Done. Here's what was shipped: + +1. Pushed 94ef253ab to origin/feat/model-list-response-normalization +2. Posted review response on PR #3799 + +All 6 review items from wenshao's second review are now addressed. Waiting for +the next round of feedback (if any) and CI to confirm. + +✻ Churned for 1m 51s + +※ recap: PRForge review response for PR #3799: pushed the clearTimeout fix to origin and posted the review response comment. All 6 items from wenshao's second review are now addressed. Waiting on CI and next review round. diff --git a/packages/cli/src/commands/auth/anthropic.ts b/packages/cli/src/commands/auth/anthropic.ts new file mode 100644 index 00000000000..d17723dcd66 --- /dev/null +++ b/packages/cli/src/commands/auth/anthropic.ts @@ -0,0 +1,104 @@ +/** + * @license + * Copyright 2025 Qwen + * SPDX-License-Identifier: Apache-2.0 + */ + +import type { ProviderModelConfig as ModelConfig } from '@qwen-code/qwen-code-core'; + +const ANTHROPIC_MODELS_URL = 'https://api.anthropic.com/v1/models'; +const ANTHROPIC_DEFAULT_MODELS: ModelConfig[] = [ + { + id: 'claude-3-5-sonnet-20241022', + name: 'Anthropic · Claude 3.5 Sonnet', + baseUrl: 'https://api.anthropic.com/v1', + envKey: 'ANTHROPIC_API_KEY', + }, + { + id: 'claude-3-5-haiku-20241022', + name: 'Anthropic · Claude 3.5 Haiku', + baseUrl: 'https://api.anthropic.com/v1', + envKey: 'ANTHROPIC_API_KEY', + }, + { + id: 'claude-3-opus-20240229', + name: 'Anthropic · Claude 3 Opus', + baseUrl: 'https://api.anthropic.com/v1', + envKey: 'ANTHROPIC_API_KEY', + }, +]; + +interface AnthropicModelApiRecord { + id?: string; + type?: string; + display_name?: string; + created_at?: string; +} + +/** + * Fetch models from Anthropic API. + * Uses x-api-key and anthropic-version headers as documented. + */ +export async function fetchAnthropicModels( + apiKey: string, + baseUrl?: string, +): Promise { + const url = baseUrl + ? `${baseUrl.replace(/\/+$/, '')}/v1/models` + : ANTHROPIC_MODELS_URL; + + const response = await fetch(url, { + method: 'GET', + headers: { + 'x-api-key': apiKey, + 'anthropic-version': '2023-06-01', + 'Content-Type': 'application/json', + }, + }); + + if (!response.ok) { + const errorText = await response.text(); + throw new Error( + `Anthropic models request failed (${response.status}): ${errorText}`, + ); + } + + const data = (await response.json()) as { + data?: AnthropicModelApiRecord[]; + }; + + const records = Array.isArray(data.data) ? data.data : []; + + const models: ModelConfig[] = records + .filter((record) => record.id && record.type === 'model') + .map((record) => ({ + id: record.id!, + name: record.display_name + ? `Anthropic · ${record.display_name}` + : `Anthropic · ${record.id!}`, + baseUrl: baseUrl || 'https://api.anthropic.com/v1', + envKey: 'ANTHROPIC_API_KEY', + })); + + if (models.length === 0) { + throw new Error('Anthropic models request returned no usable models.'); + } + + return models; +} + +/** + * Fetch Anthropic models with fallback to defaults on error. + */ +export async function getAnthropicModelsWithFallback( + apiKey: string, + baseUrl?: string, +): Promise { + try { + return await fetchAnthropicModels(apiKey, baseUrl); + } catch { + return ANTHROPIC_DEFAULT_MODELS; + } +} + +export { ANTHROPIC_DEFAULT_MODELS }; diff --git a/packages/cli/src/ui/components/ManageModelsDialog.tsx b/packages/cli/src/ui/components/ManageModelsDialog.tsx index 98998d54fc4..4d4d6c65968 100644 --- a/packages/cli/src/ui/components/ManageModelsDialog.tsx +++ b/packages/cli/src/ui/components/ManageModelsDialog.tsx @@ -37,11 +37,9 @@ export type FilterMode = 'all' | 'enabled' | 'free' | 'vision'; const MAX_VISIBLE_MODELS = 12; const MANAGE_MODELS_TABS = [ { source: 'openrouter', label: 'OpenRouter', enabled: true }, - { source: 'modelstudio', label: 'ModelStudio', enabled: false }, + { source: 'anthropic', label: 'Anthropic', enabled: true }, ] as const; -type ManageModelsTabSource = (typeof MANAGE_MODELS_TABS)[number]['source']; - export function buildModelLabel(entry: ManageModelsCatalogEntry): string { return entry.label; } @@ -135,9 +133,9 @@ export function getNextFocusMode( } export function getNextEnabledTabSource( - current: ManageModelsTabSource, + current: ManageModelsSource, direction: 'left' | 'right', -): ManageModelsTabSource { +): ManageModelsSource { const currentIndex = MANAGE_MODELS_TABS.findIndex( (tab) => tab.source === current, ); @@ -164,8 +162,8 @@ export function ManageModelsDialog({ }: ManageModelsDialogProps): React.JSX.Element { const settings = useSettings(); const [activeTabSource, setActiveTabSource] = - useState('openrouter'); - const source: ManageModelsSource = 'openrouter'; + useState('openrouter'); + const source = activeTabSource; const [status, setStatus] = useState('loading'); const [error, setError] = useState(null); @@ -183,7 +181,16 @@ export function ManageModelsDialog({ setStatusMessage(null); try { - const nextCatalog = await fetchManageModelsCatalog(source); + let apiKey: string | undefined; + let baseUrl: string | undefined; + + if (source === 'anthropic') { + const contentGenConfig = config.getContentGeneratorConfig(); + apiKey = contentGenConfig?.apiKey || process.env['ANTHROPIC_API_KEY']; + baseUrl = contentGenConfig?.baseUrl; + } + + const nextCatalog = await fetchManageModelsCatalog(source, apiKey, baseUrl); const enabledIds = getEnabledModelIdsForSource(source, settings); setCatalog(nextCatalog); setSelectedIds(enabledIds); @@ -195,7 +202,7 @@ export function ManageModelsDialog({ ); setStatus('error'); } - }, [settings, source]); + }, [settings, source, config]); useEffect(() => { void loadCatalog(); @@ -497,7 +504,7 @@ export function ManageModelsDialog({ {status === 'loading' - ? 'Loading OpenRouter catalog…' + ? `Loading ${source === 'anthropic' ? 'Anthropic' : 'OpenRouter'} catalog…` : 'Saving enabled models…'} diff --git a/packages/cli/src/ui/manageModels/manageModels.ts b/packages/cli/src/ui/manageModels/manageModels.ts index c2d4bfe1244..c691071e1e3 100644 --- a/packages/cli/src/ui/manageModels/manageModels.ts +++ b/packages/cli/src/ui/manageModels/manageModels.ts @@ -18,8 +18,11 @@ import { isOpenRouterConfig, mergeOpenRouterConfigs, } from '../../commands/auth/openrouterOAuth.js'; +import { + fetchAnthropicModels, +} from '../../commands/auth/anthropic.js'; -export const MANAGE_MODELS_SOURCES = ['openrouter'] as const; +export const MANAGE_MODELS_SOURCES = ['openrouter', 'anthropic'] as const; export type ManageModelsSource = (typeof MANAGE_MODELS_SOURCES)[number]; @@ -101,6 +104,8 @@ function createEntry( export async function fetchManageModelsCatalog( source: ManageModelsSource, + apiKey?: string, + baseUrl?: string, ): Promise { switch (source) { case 'openrouter': { @@ -114,6 +119,20 @@ export async function fetchManageModelsCatalog( entries: models.map((model) => createEntry(source, model)), }; } + case 'anthropic': { + if (!apiKey) { + throw new Error('API key is required for Anthropic model listing.'); + } + const models = await fetchAnthropicModels(apiKey, baseUrl); + return { + source, + title: 'Anthropic', + description: + 'Browse available Anthropic models and choose which models are enabled locally.', + authType: AuthType.USE_ANTHROPIC, + entries: models.map((model) => createEntry(source, model)), + }; + } default: throw new Error(`Unsupported manage models source: ${source}`); } @@ -126,13 +145,18 @@ export function getEnabledModelIdsForSource( const modelProviders = settings.merged.modelProviders as | ModelProvidersConfig | undefined; - const openaiConfigs = modelProviders?.[AuthType.USE_OPENAI] || []; switch (source) { - case 'openrouter': + case 'openrouter': { + const openaiConfigs = modelProviders?.[AuthType.USE_OPENAI] || []; return openaiConfigs .filter((config) => isOpenRouterConfig(config)) .map((config) => config.id); + } + case 'anthropic': { + const anthropicConfigs = modelProviders?.[AuthType.USE_ANTHROPIC] || []; + return anthropicConfigs.map((config) => config.id); + } default: return []; } @@ -149,11 +173,11 @@ export async function saveManageModelsSelection(params: { const mergedModelProviders = settings.merged.modelProviders as | ModelProvidersConfig | undefined; - const existingOpenAIConfigs = - mergedModelProviders?.[AuthType.USE_OPENAI] || []; switch (source) { case 'openrouter': { + const existingOpenAIConfigs = + mergedModelProviders?.[AuthType.USE_OPENAI] || []; const updatedConfigs = mergeOpenRouterConfigs( existingOpenAIConfigs, selectedModels, @@ -205,6 +229,50 @@ export async function saveManageModelsSelection(params: { activeModelId, }; } + case 'anthropic': { + if (selectedModels.length === 0) { + throw new Error( + 'At least one Anthropic model must remain enabled.', + ); + } + + settings.setValue( + persistScope, + `modelProviders.${AuthType.USE_ANTHROPIC}`, + selectedModels, + ); + + const selectedIds = selectedModels.map((model) => model.id); + const currentAuthType = config.getContentGeneratorConfig()?.authType; + const currentModelId = config.getModel(); + const currentModelStillAvailable = currentModelId + ? selectedModels.some((model) => model.id === currentModelId) + : false; + + let activeModelId = currentModelId; + if (!currentModelStillAvailable) { + activeModelId = selectedModels[0]?.id; + if (activeModelId) { + settings.setValue(persistScope, 'model.name', activeModelId); + } + } + + const updatedModelProviders: ModelProvidersConfig = { + ...(mergedModelProviders || {}), + [AuthType.USE_ANTHROPIC]: selectedModels, + }; + config.reloadModelProvidersConfig(updatedModelProviders); + + if (currentAuthType === AuthType.USE_ANTHROPIC) { + await config.refreshAuth(AuthType.USE_ANTHROPIC); + } + + return { + updatedConfigs: selectedModels, + selectedIds, + activeModelId, + }; + } default: throw new Error(`Unsupported manage models source: ${source}`); } diff --git a/packages/core/src/core/geminiChat.test.ts b/packages/core/src/core/geminiChat.test.ts index 39cfed3d5de..03ee416855c 100644 --- a/packages/core/src/core/geminiChat.test.ts +++ b/packages/core/src/core/geminiChat.test.ts @@ -1700,6 +1700,216 @@ describe('GeminiChat', async () => { ).toHaveBeenCalledTimes(2); }); + it('should retry on 408 Request Timeout errors', async () => { + const error408 = new ApiError({ + message: 'Request Timeout', + status: 408, + }); + + vi.mocked(mockContentGenerator.generateContentStream) + .mockRejectedValueOnce(error408) + .mockResolvedValueOnce( + (async function* () { + yield { + candidates: [ + { + content: { parts: [{ text: 'Recovered from 408' }] }, + finishReason: 'STOP', + }, + ], + } as unknown as GenerateContentResponse; + })(), + ); + + const stream = await chat.sendMessageStream( + 'test-model', + { message: 'test' }, + 'prompt-id-408-retry', + ); + + const events: StreamEvent[] = []; + for await (const event of stream) { + events.push(event); + } + + // Should be called twice (initial + retry) + expect( + mockContentGenerator.generateContentStream, + ).toHaveBeenCalledTimes(2); + + // Should have successful content + expect( + events.some( + (e) => + e.type === StreamEventType.CHUNK && + e.value.candidates?.[0]?.content?.parts?.[0]?.text === + 'Recovered from 408', + ), + ).toBe(true); + }); + + it('should retry on 409 transient conflict errors', async () => { + const error409 = new ApiError({ + message: 'Lock contention detected', + status: 409, + }); + + vi.mocked(mockContentGenerator.generateContentStream) + .mockRejectedValueOnce(error409) + .mockResolvedValueOnce( + (async function* () { + yield { + candidates: [ + { + content: { parts: [{ text: 'Recovered from 409' }] }, + finishReason: 'STOP', + }, + ], + } as unknown as GenerateContentResponse; + })(), + ); + + const stream = await chat.sendMessageStream( + 'test-model', + { message: 'test' }, + 'prompt-id-409-retry', + ); + + const events: StreamEvent[] = []; + for await (const event of stream) { + events.push(event); + } + + // Should be called twice (initial + retry) + expect( + mockContentGenerator.generateContentStream, + ).toHaveBeenCalledTimes(2); + + expect( + events.some( + (e) => + e.type === StreamEventType.CHUNK && + e.value.candidates?.[0]?.content?.parts?.[0]?.text === + 'Recovered from 409', + ), + ).toBe(true); + }); + + it('should NOT retry on 409 deterministic conflict errors', async () => { + const error409 = new ApiError({ + message: 'Resource already exists', + status: 409, + }); + + vi.mocked(mockContentGenerator.generateContentStream).mockRejectedValue( + error409, + ); + + const stream = await chat.sendMessageStream( + 'test-model', + { message: 'test' }, + 'prompt-id-409-noretry', + ); + + await expect( + (async () => { + for await (const _ of stream) { + /* consume stream */ + } + })(), + ).rejects.toThrow(error409); + + // Should only be called once (no retry) + expect( + mockContentGenerator.generateContentStream, + ).toHaveBeenCalledTimes(1); + }); + + it('should retry on network errors (ECONNRESET)', async () => { + const networkError = new Error( + 'Connection reset', + ) as NodeJS.ErrnoException; + networkError.code = 'ECONNRESET'; + + vi.mocked(mockContentGenerator.generateContentStream) + .mockRejectedValueOnce(networkError) + .mockResolvedValueOnce( + (async function* () { + yield { + candidates: [ + { + content: { + parts: [{ text: 'Recovered from network error' }], + }, + finishReason: 'STOP', + }, + ], + } as unknown as GenerateContentResponse; + })(), + ); + + const stream = await chat.sendMessageStream( + 'test-model', + { message: 'test' }, + 'prompt-id-network-retry', + ); + + const events: StreamEvent[] = []; + for await (const event of stream) { + events.push(event); + } + + // Should be called twice (initial + retry) + expect( + mockContentGenerator.generateContentStream, + ).toHaveBeenCalledTimes(2); + + expect( + events.some( + (e) => + e.type === StreamEventType.CHUNK && + e.value.candidates?.[0]?.content?.parts?.[0]?.text === + 'Recovered from network error', + ), + ).toBe(true); + }); + + it('should NOT retry on invalid argument errors (isInvalidArgumentError guard)', async () => { + vi.useFakeTimers(); + try { + const invalidArgError = new ApiError({ + message: + 'Request contains an invalid argument: schema validation failed', + status: 400, + }); + + vi.mocked( + mockContentGenerator.generateContentStream, + ).mockRejectedValue(invalidArgError); + + const stream = await chat.sendMessageStream( + 'test-model', + { message: 'test' }, + 'prompt-id-invalid-arg', + ); + + await expect( + (async () => { + for await (const _ of stream) { + void 0; + } + })(), + ).rejects.toThrow(invalidArgError); + + // Should only be called once (no retry) + expect( + mockContentGenerator.generateContentStream, + ).toHaveBeenCalledTimes(1); + } finally { + vi.useRealTimers(); + } + }); + afterEach(() => { // Reset to default behavior mockRetryWithBackoff.mockImplementation(async (apiCall) => apiCall()); diff --git a/packages/core/src/core/geminiChat.ts b/packages/core/src/core/geminiChat.ts index cf6e37761d5..d287131370d 100644 --- a/packages/core/src/core/geminiChat.ts +++ b/packages/core/src/core/geminiChat.ts @@ -17,8 +17,11 @@ import type { GenerateContentResponseUsageMetadata, } from '@google/genai'; import { createUserContent, FinishReason } from '@google/genai'; -import { retryWithBackoff, isUnattendedMode } from '../utils/retry.js'; -import { getErrorStatus } from '../utils/errors.js'; +import { + retryWithBackoff, + isUnattendedMode, + classifyError, +} from '../utils/retry.js'; import { createDebugLogger } from '../utils/debugLogger.js'; import { parseAndFormatApiError } from '../utils/errorParsing.js'; import { isRateLimitError, type RetryInfo } from '../utils/rateLimit.js'; @@ -713,17 +716,20 @@ export class GeminiChat { ); const streamResponse = await retryWithBackoff(apiCall, { shouldRetryOnError: (error: unknown) => { + // Independent safety-net guards not covered by classifyError: + // never retry schema-depth-limit or invalid-argument errors regardless + // of what classifyError returns. if (error instanceof Error) { if (isSchemaDepthError(error.message)) return false; if (isInvalidArgumentError(error.message)) return false; } - const status = getErrorStatus(error); - if (status === 400) return false; - if (status === 429) return true; - if (status && status >= 500 && status < 600) return true; - - return false; + // Delegate to classifyError for all remaining cases. Explicitly accepted + // retryable categories for Gemini streaming: 408 (timeout), 409 (transient + // lock/contention only), 429 (rate limit), 5xx (server errors), network + // transport errors. Deterministic errors (400, 401, 403, 404, 422) are + // handled by classifyError and return retryable=false. + return classifyError(error).retryable; }, authType: this.config.getContentGeneratorConfig()?.authType, persistentMode: isUnattendedMode(), diff --git a/packages/core/src/tools/mcp-client-manager.test.ts b/packages/core/src/tools/mcp-client-manager.test.ts index 140b78324ed..9bb4dc4b4f0 100644 --- a/packages/core/src/tools/mcp-client-manager.test.ts +++ b/packages/core/src/tools/mcp-client-manager.test.ts @@ -227,6 +227,102 @@ describe('McpClientManager', () => { expect(secondClient.disconnect).toHaveBeenCalledOnce(); }); + it('should not spawn duplicate clients when discoverMcpToolsForServer is called concurrently for the same server', async () => { + // Simulate a slow connect to create a window where a second call could race. + let connectCallCount = 0; + const connectDelays: Array<(value: void) => void> = []; + + const mockedClient = { + connect: vi.fn().mockImplementation(() => { + connectCallCount++; + return new Promise((resolve) => { + connectDelays.push(resolve); + }); + }), + discover: vi.fn().mockResolvedValue(undefined), + disconnect: vi.fn().mockResolvedValue(undefined), + getStatus: vi.fn(), + }; + + vi.mocked(McpClient).mockReturnValue(mockedClient as unknown as McpClient); + + const mockConfig = { + isTrustedFolder: () => true, + getMcpServers: () => ({ 'test-server': {} }), + getMcpServerCommand: () => undefined, + getPromptRegistry: () => ({}) as PromptRegistry, + getWorkspaceContext: () => ({}) as WorkspaceContext, + getDebugMode: () => false, + } as unknown as Config; + const manager = new McpClientManager(mockConfig, {} as ToolRegistry); + + // Fire two concurrent discoveries for the same server. + const p1 = manager.discoverMcpToolsForServer( + 'test-server', + {} as unknown as Config, + ); + const p2 = manager.discoverMcpToolsForServer( + 'test-server', + {} as unknown as Config, + ); + + // Let both settle (the second should have been a no-op). + // Resolve the single in-flight connect. + connectDelays[0]?.(); + await p1; + await p2; + + // Only one connect call should have been made — the concurrent second + // call must have bailed out early. + expect(connectCallCount).toBe(1); + expect(mockedClient.connect).toHaveBeenCalledOnce(); + expect(mockedClient.discover).toHaveBeenCalledOnce(); + }); + + it('should clean up in-flight tracking after discovery completes', async () => { + const mockedClient = { + connect: vi.fn().mockResolvedValue(undefined), + discover: vi.fn().mockResolvedValue(undefined), + disconnect: vi.fn().mockResolvedValue(undefined), + getStatus: vi.fn(), + }; + + vi.mocked(McpClient).mockReturnValue(mockedClient as unknown as McpClient); + + const mockConfig = { + isTrustedFolder: () => true, + getMcpServers: () => ({ 'test-server': {} }), + getMcpServerCommand: () => undefined, + getPromptRegistry: () => ({}) as PromptRegistry, + getWorkspaceContext: () => ({}) as WorkspaceContext, + getDebugMode: () => false, + } as unknown as Config; + const manager = new McpClientManager(mockConfig, {} as ToolRegistry); + + await manager.discoverMcpToolsForServer( + 'test-server', + {} as unknown as Config, + ); + + // After completion, a second call should proceed (not be skipped). + const secondClient = { + connect: vi.fn().mockResolvedValue(undefined), + discover: vi.fn().mockResolvedValue(undefined), + disconnect: vi.fn().mockResolvedValue(undefined), + getStatus: vi.fn(), + }; + vi.mocked(McpClient).mockReturnValue(secondClient as unknown as McpClient); + + await manager.discoverMcpToolsForServer( + 'test-server', + {} as unknown as Config, + ); + + // The second call should have created a new client and connected. + expect(secondClient.connect).toHaveBeenCalledOnce(); + expect(secondClient.discover).toHaveBeenCalledOnce(); + }); + it('should no-op when discovering an unknown server', async () => { const mockedMcpClient = { connect: vi.fn(), @@ -254,4 +350,47 @@ describe('McpClientManager', () => { expect(vi.mocked(McpClient)).not.toHaveBeenCalled(); }); + + it('should clean up in-flight state when removing a server', async () => { + const mockedClient = { + connect: vi.fn().mockResolvedValue(undefined), + discover: vi.fn().mockResolvedValue(undefined), + disconnect: vi.fn().mockResolvedValue(undefined), + getStatus: vi.fn(), + }; + + vi.mocked(McpClient).mockReturnValue(mockedClient as unknown as McpClient); + + const mockConfig = { + isTrustedFolder: () => true, + getMcpServers: () => ({ 'test-server': {} }), + getMcpServerCommand: () => undefined, + getPromptRegistry: () => ({}) as PromptRegistry, + getWorkspaceContext: () => ({}) as WorkspaceContext, + getDebugMode: () => false, + } as unknown as Config; + const manager = new McpClientManager(mockConfig, {} as ToolRegistry); + + // Discover the server first + await manager.discoverMcpToolsForServer('test-server', mockConfig); + + // Simulate an in-flight discovery by directly adding to the internal set + // (we can't easily access private fields, so we'll test indirectly) + // Instead, remove the server and verify re-discovery works + await manager.stop(); + + // After stop, a new discovery should proceed normally + const secondClient = { + connect: vi.fn().mockResolvedValue(undefined), + discover: vi.fn().mockResolvedValue(undefined), + disconnect: vi.fn().mockResolvedValue(undefined), + getStatus: vi.fn(), + }; + vi.mocked(McpClient).mockReturnValue(secondClient as unknown as McpClient); + + await manager.discoverMcpToolsForServer('test-server', mockConfig); + + expect(secondClient.connect).toHaveBeenCalledOnce(); + expect(secondClient.discover).toHaveBeenCalledOnce(); + }); }); diff --git a/packages/core/src/tools/mcp-client-manager.ts b/packages/core/src/tools/mcp-client-manager.ts index ecc700739fe..f30190874b3 100644 --- a/packages/core/src/tools/mcp-client-manager.ts +++ b/packages/core/src/tools/mcp-client-manager.ts @@ -58,6 +58,7 @@ export class McpClientManager { private healthCheckTimers: Map = new Map(); private consecutiveFailures: Map = new Map(); private isReconnecting: Map = new Map(); + private inFlightDiscoveries: Set = new Set(); constructor( config: Config, @@ -157,6 +158,20 @@ export class McpClientManager { return; } + // Prevent concurrent re-discovery of the same server to avoid spawning + // duplicate MCP child processes. If a discovery/reconnection is already + // in progress for this server, bail out early. + if (this.inFlightDiscoveries.has(serverName)) { + debugLogger.debug( + `Discovery already in flight for server '${serverName}', skipping.`, + ); + return; + } + + // Track this discovery in-flight immediately — before any await — to + // prevent a TOCTOU race where a concurrent call slips in during disconnect. + this.inFlightDiscoveries.add(serverName); + // Ensure we don't leak an existing connection for this server. const existingClient = this.clients.get(serverName); if (existingClient) { @@ -168,6 +183,7 @@ export class McpClientManager { ); } finally { this.clients.delete(serverName); + this.stopHealthCheck(serverName); this.eventEmitter?.emit('mcp-client-update', this.clients); } } @@ -177,20 +193,20 @@ export class McpClientManager { ? this.sendSdkMcpMessage : undefined; - const client = new McpClient( - serverName, - serverConfig, - this.toolRegistry, - this.cliConfig.getPromptRegistry(), - this.cliConfig.getWorkspaceContext(), - this.cliConfig.getDebugMode(), - sdkCallback, - ); + try { + const client = new McpClient( + serverName, + serverConfig, + this.toolRegistry, + this.cliConfig.getPromptRegistry(), + this.cliConfig.getWorkspaceContext(), + this.cliConfig.getDebugMode(), + sdkCallback, + ); - this.clients.set(serverName, client); - this.eventEmitter?.emit('mcp-client-update', this.clients); + this.clients.set(serverName, client); + this.eventEmitter?.emit('mcp-client-update', this.clients); - try { await client.connect(); await client.discover(cliConfig); // Start health check for this server after successful discovery @@ -202,7 +218,21 @@ export class McpClientManager { error, )}`, ); + // Disconnect the failed client (may have spawned a child process) + // before removing it from the map, to avoid orphaned processes. + const failedClient = this.clients.get(serverName); + if (failedClient) { + try { + await failedClient.disconnect(); + } catch { + // Ignore disconnect errors — the client may never have connected + } + } + // Remove the failed client so a subsequent discovery can retry cleanly. + this.clients.delete(serverName); + this.stopHealthCheck(serverName); } finally { + this.inFlightDiscoveries.delete(serverName); this.eventEmitter?.emit('mcp-client-update', this.clients); } } @@ -231,6 +261,7 @@ export class McpClientManager { this.clients.clear(); this.consecutiveFailures.clear(); this.isReconnecting.clear(); + this.inFlightDiscoveries.clear(); } /** @@ -253,6 +284,7 @@ export class McpClientManager { this.clients.delete(serverName); this.consecutiveFailures.delete(serverName); this.isReconnecting.delete(serverName); + this.inFlightDiscoveries.delete(serverName); this.eventEmitter?.emit('mcp-client-update', this.clients); } } @@ -388,11 +420,25 @@ export class McpClientManager { setTimeout(resolve, this.healthConfig.reconnectDelayMs), ); + // Guard: another discovery may have completed during the delay window + // and cleared inFlightDiscoveries. Check before proceeding. + if (this.inFlightDiscoveries.has(serverName)) { + debugLogger.debug( + `Reconnect skipped: discovery already in flight for '${serverName}'`, + ); + return; + } + await this.discoverMcpToolsForServer(serverName, this.cliConfig); - // Reset failure count on successful reconnection - this.consecutiveFailures.set(serverName, 0); - debugLogger.info(`Successfully reconnected to server '${serverName}'`); + // Only report success if the server is actually connected. + // discoverMcpToolsForServer() may return early (in-flight guard), + // in which case the server won't be connected. + const client = this.clients.get(serverName); + if (client && client.getStatus() === MCPServerStatus.CONNECTED) { + this.consecutiveFailures.set(serverName, 0); + debugLogger.info(`Successfully reconnected to server '${serverName}'`); + } } catch (error) { debugLogger.error( `Failed to reconnect to server '${serverName}': ${getErrorMessage(error)}`, @@ -482,6 +528,8 @@ export class McpClientManager { this.clients.delete(serverName); this.stopHealthCheck(serverName); this.consecutiveFailures.delete(serverName); + this.inFlightDiscoveries.delete(serverName); + this.isReconnecting.delete(serverName); } // Remove tools for this server from registry diff --git a/packages/core/src/utils/retry.test.ts b/packages/core/src/utils/retry.test.ts index 0fd2478602a..dd1ce00754e 100644 --- a/packages/core/src/utils/retry.test.ts +++ b/packages/core/src/utils/retry.test.ts @@ -19,6 +19,8 @@ import { retryWithBackoff, isTransientCapacityError, isUnattendedMode, + isRetryableNetworkError, + classifyError, } from './retry.js'; import { getErrorStatus } from './errors.js'; import { setSimulate429 } from './testUtils.js'; @@ -97,6 +99,7 @@ describe('retryWithBackoff', () => { // 2. IMPORTANT: Attach the rejection expectation to the promise *immediately*. // This ensures a 'catch' handler is present before the promise can reject. // The result is a new promise that resolves when the assertion is met. + // eslint-disable-next-line vitest/valid-expect const assertionPromise = expect(promise).rejects.toThrow( 'Simulated error attempt 3', @@ -117,9 +120,10 @@ describe('retryWithBackoff', () => { // This function will fail more than 7 times to ensure all retries are used. const mockFn = createFailingFunction(10); - const promise = retryWithBackoff(mockFn); + const promise = retryWithBackoff(mockFn, { initialDelayMs: 10 }); // Expect it to fail with the error from the 7th attempt. + // eslint-disable-next-line vitest/valid-expect const assertionPromise = expect(promise).rejects.toThrow( 'Simulated error attempt 7', @@ -134,9 +138,13 @@ describe('retryWithBackoff', () => { // This function will fail more than 7 times to ensure all retries are used. const mockFn = createFailingFunction(10); - const promise = retryWithBackoff(mockFn, { maxAttempts: undefined }); + const promise = retryWithBackoff(mockFn, { + maxAttempts: undefined, + initialDelayMs: 10, + }); // Expect it to fail with the error from the 7th attempt. + // eslint-disable-next-line vitest/valid-expect const assertionPromise = expect(promise).rejects.toThrow( 'Simulated error attempt 7', @@ -189,7 +197,8 @@ describe('retryWithBackoff', () => { // Attach the rejection expectation *before* running timers const assertionPromise = - expect(promise).rejects.toThrow('Too Many Requests'); // eslint-disable-line vitest/valid-expect + // eslint-disable-next-line vitest/valid-expect + expect(promise).rejects.toThrow('Too Many Requests'); // Run timers to trigger retries and eventual rejection await vi.runAllTimersAsync(); @@ -215,6 +224,51 @@ describe('retryWithBackoff', () => { expect(mockFn).toHaveBeenCalledTimes(1); }); + it('should NOT retry on 408 Request Timeout with default shouldRetry', async () => { + const mockFn = vi.fn(async () => { + const error = new Error('Request Timeout') as any; + error.status = 408; + throw error; + }); + + const promise = retryWithBackoff(mockFn, { + maxAttempts: 2, + initialDelayMs: 10, + }); + await expect(promise).rejects.toThrow('Request Timeout'); + expect(mockFn).toHaveBeenCalledTimes(1); + }); + + it('should NOT retry on 409 Conflict with default shouldRetry', async () => { + const mockFn = vi.fn(async () => { + const error = new Error('Conflict') as any; + error.status = 409; + throw error; + }); + + const promise = retryWithBackoff(mockFn, { + maxAttempts: 2, + initialDelayMs: 10, + }); + await expect(promise).rejects.toThrow('Conflict'); + expect(mockFn).toHaveBeenCalledTimes(1); + }); + + it('should NOT retry on ECONNRESET with default shouldRetry', async () => { + const mockFn = vi.fn(async () => { + const error = new Error('Connection reset') as NodeJS.ErrnoException; + error.code = 'ECONNRESET'; + throw error; + }); + + const promise = retryWithBackoff(mockFn, { + maxAttempts: 2, + initialDelayMs: 10, + }); + await expect(promise).rejects.toThrow('Connection reset'); + expect(mockFn).toHaveBeenCalledTimes(1); + }); + it('should respect maxDelayMs', async () => { const mockFn = createFailingFunction(3); const setTimeoutSpy = vi.spyOn(global, 'setTimeout'); @@ -250,15 +304,17 @@ describe('retryWithBackoff', () => { const runRetry = () => retryWithBackoff(mockFn, { maxAttempts: 2, // Only one retry, so one delay - initialDelayMs: 100, - maxDelayMs: 1000, + initialDelayMs: 10, + maxDelayMs: 100, }); // We expect rejections as mockFn fails 5 times const promise1 = runRetry(); // Attach the rejection expectation *before* running timers - // eslint-disable-next-line vitest/valid-expect - const assertionPromise1 = expect(promise1).rejects.toThrow(); + + const assertionPromise1 = + // eslint-disable-next-line vitest/valid-expect + expect(promise1).rejects.toThrow(); await vi.runAllTimersAsync(); // Advance for the delay in the first runRetry await assertionPromise1; @@ -272,8 +328,10 @@ describe('retryWithBackoff', () => { const promise2 = runRetry(); // Attach the rejection expectation *before* running timers - // eslint-disable-next-line vitest/valid-expect - const assertionPromise2 = expect(promise2).rejects.toThrow(); + + const assertionPromise2 = + // eslint-disable-next-line vitest/valid-expect + expect(promise2).rejects.toThrow(); await vi.runAllTimersAsync(); // Advance for the delay in the second runRetry await assertionPromise2; @@ -291,10 +349,10 @@ describe('retryWithBackoff', () => { throw new Error('Delays were not captured for jitter test'); } - // Ensure delays are within the expected jitter range [70, 130] for initialDelayMs = 100 + // Ensure delays are within the expected jitter range [7, 13] for initialDelayMs = 10 [...firstDelaySet, ...secondDelaySet].forEach((d) => { - expect(d).toBeGreaterThanOrEqual(100 * 0.7); - expect(d).toBeLessThanOrEqual(100 * 1.3); + expect(d).toBeGreaterThanOrEqual(10 * 0.7); + expect(d).toBeLessThanOrEqual(10 * 1.3); }); }); @@ -499,6 +557,31 @@ describe('isTransientCapacityError', () => { expect(isTransientCapacityError(new Error('generic'))).toBe(false); expect(isTransientCapacityError(null)).toBe(false); }); + + // 408 and network errors are NOT transient capacity errors — they can indicate + // permanent config issues and should not trigger indefinite persistent retries. + // They remain retryable in standard mode via classifyError. + it('should return false for 408 errors', () => { + const error = { status: 408 }; + expect(isTransientCapacityError(error)).toBe(false); + }); + + it('should return false for ECONNRESET network errors', () => { + const error = new Error('Connection reset') as NodeJS.ErrnoException; + error.code = 'ECONNRESET'; + expect(isTransientCapacityError(error)).toBe(false); + }); + + it('should return false for ETIMEDOUT network errors', () => { + const error = new Error('Timed out') as NodeJS.ErrnoException; + error.code = 'ETIMEDOUT'; + expect(isTransientCapacityError(error)).toBe(false); + }); + + it('should return false for "socket closed" message', () => { + const error = new Error('The socket closed unexpectedly'); + expect(isTransientCapacityError(error)).toBe(false); + }); }); describe('isUnattendedMode', () => { @@ -713,7 +796,7 @@ describe('retryWithBackoff - persistent mode', () => { const promise = retryWithBackoff(fn, { maxAttempts: 3, - initialDelayMs: 10000, // Long delay so abort happens during sleep + initialDelayMs: 100, // Short delay; abort via setTimeout below persistentMode: true, heartbeatIntervalMs: 50, signal: controller.signal, @@ -1024,3 +1107,491 @@ describe('getErrorStatus', () => { expect(getErrorStatus(new Error('HTTP_STATUS/4291'))).toBeUndefined(); }); }); + +describe('isRetryableNetworkError', () => { + it('should return true for ECONNRESET', () => { + const error = new Error('Connection reset'); + (error as NodeJS.ErrnoException).code = 'ECONNRESET'; + expect(isRetryableNetworkError(error)).toBe(true); + }); + + it('should return true for ETIMEDOUT', () => { + const error = new Error('Timed out'); + (error as NodeJS.ErrnoException).code = 'ETIMEDOUT'; + expect(isRetryableNetworkError(error)).toBe(true); + }); + + it('should return true for ESOCKETTIMEDOUT', () => { + const error = new Error('Socket timed out'); + (error as NodeJS.ErrnoException).code = 'ESOCKETTIMEDOUT'; + expect(isRetryableNetworkError(error)).toBe(true); + }); + + it('should return true for ECONNREFUSED', () => { + const error = new Error('Connection refused'); + (error as NodeJS.ErrnoException).code = 'ECONNREFUSED'; + expect(isRetryableNetworkError(error)).toBe(true); + }); + + it('should return false for ENOTFOUND (removed from retryable codes)', () => { + const error = new Error('Not found'); + (error as NodeJS.ErrnoException).code = 'ENOTFOUND'; + expect(isRetryableNetworkError(error)).toBe(false); + }); + + it('should return false for EHOSTUNREACH (removed from retryable codes)', () => { + const error = new Error('Host unreachable'); + (error as NodeJS.ErrnoException).code = 'EHOSTUNREACH'; + expect(isRetryableNetworkError(error)).toBe(false); + }); + + it('should return true for EAI_AGAIN', () => { + const error = new Error('Temporary failure'); + (error as NodeJS.ErrnoException).code = 'EAI_AGAIN'; + expect(isRetryableNetworkError(error)).toBe(true); + }); + + it('should return true for "socket closed" message', () => { + const error = new Error('The socket closed unexpectedly'); + expect(isRetryableNetworkError(error)).toBe(true); + }); + + it('should return true for "stream ended" message', () => { + const error = new Error('The stream ended before completion'); + expect(isRetryableNetworkError(error)).toBe(true); + }); + + it('should return false for "network error" message (removed overly-broad substring match)', () => { + const error = new Error('A network error occurred'); + expect(isRetryableNetworkError(error)).toBe(false); + }); + + it('should return true for "connection reset" message', () => { + const error = new Error('connection reset by peer'); + expect(isRetryableNetworkError(error)).toBe(true); + }); + + it('should return true for "econnreset" message (case-insensitive)', () => { + const error = new Error('ECONNRESET: econnreset'); + expect(isRetryableNetworkError(error)).toBe(true); + }); + + it('should return true for "etimedout" message (case-insensitive)', () => { + const error = new Error('etimedout waiting for response'); + expect(isRetryableNetworkError(error)).toBe(true); + }); + + it('should return false for non-retryable errors', () => { + const error = new Error('Bad request'); + expect(isRetryableNetworkError(error)).toBe(false); + }); + + it('should return false for errors with non-retryable codes', () => { + const error = new Error('Permission denied'); + (error as NodeJS.ErrnoException).code = 'EACCES'; + expect(isRetryableNetworkError(error)).toBe(false); + }); + + it('should return false for null/undefined', () => { + expect(isRetryableNetworkError(null)).toBe(false); + expect(isRetryableNetworkError(undefined)).toBe(false); + }); +}); + +describe('classifyError', () => { + it('should classify 400 as non-retryable', () => { + const error = { status: 400 }; + const result = classifyError(error); + expect(result.retryable).toBe(false); + expect(result.reason).toContain('Deterministic request error'); + expect(result.status).toBe(400); + }); + + it('should classify 401 as non-retryable', () => { + const error = { status: 401 }; + const result = classifyError(error); + expect(result.retryable).toBe(false); + expect(result.reason).toContain('Deterministic request error'); + }); + + it('should classify 403 as non-retryable', () => { + const error = { status: 403 }; + const result = classifyError(error); + expect(result.retryable).toBe(false); + expect(result.reason).toContain('Deterministic request error'); + }); + + it('should classify 404 as non-retryable', () => { + const error = { status: 404 }; + const result = classifyError(error); + expect(result.retryable).toBe(false); + expect(result.reason).toContain('Deterministic request error'); + }); + + it('should classify 422 as non-retryable', () => { + const error = { status: 422 }; + const result = classifyError(error); + expect(result.retryable).toBe(false); + expect(result.reason).toContain('Deterministic request error'); + }); + + it('should classify 429 as retryable', () => { + const error = { status: 429 }; + const result = classifyError(error); + expect(result.retryable).toBe(true); + expect(result.reason).toContain('Rate limited'); + expect(result.status).toBe(429); + }); + + it('should classify 408 as retryable', () => { + const error = { status: 408 }; + const result = classifyError(error); + expect(result.retryable).toBe(true); + expect(result.reason).toContain('Request timeout'); + }); + + it('should classify 409 with transient message as retryable', () => { + const error: HttpError = new Error('Lock contention detected'); + error.status = 409; + const result = classifyError(error); + expect(result.retryable).toBe(true); + expect(result.reason).toContain('Transient conflict'); + }); + + it('should classify 409 with contention message as retryable', () => { + const error: HttpError = new Error('Resource contention'); + error.status = 409; + const result = classifyError(error); + expect(result.retryable).toBe(true); + }); + + // 'conflict' is NOT a transient keyword — it appears in the standard HTTP 409 + // reason phrase "Conflict", so matching it would make all 409s transient. + it('should classify 409 with conflict-only message as non-retryable', () => { + const error: HttpError = Object.assign(new Error('Duplicate resource'), { + status: 409, + }); + const result = classifyError(error); + expect(result.retryable).toBe(false); + expect(result.reason).toContain('Deterministic conflict'); + }); + + it('should classify 409 without transient message as non-retryable', () => { + const error: HttpError = Object.assign(new Error('Validation failed'), { + status: 409, + }); + const result = classifyError(error); + expect(result.retryable).toBe(false); + expect(result.reason).toContain('Deterministic conflict'); + }); + + it('should classify 500 as retryable', () => { + const error = { status: 500 }; + const result = classifyError(error); + expect(result.retryable).toBe(true); + expect(result.reason).toContain('Server error'); + }); + + it('should classify 503 as retryable', () => { + const error = { status: 503 }; + const result = classifyError(error); + expect(result.retryable).toBe(true); + expect(result.reason).toContain('Server error'); + }); + + it('should classify 599 as retryable', () => { + const error = { status: 599 }; + const result = classifyError(error); + expect(result.retryable).toBe(true); + expect(result.reason).toContain('Server error'); + }); + + it('should classify ECONNRESET as retryable network error', () => { + const error = new Error('Connection reset'); + (error as NodeJS.ErrnoException).code = 'ECONNRESET'; + const result = classifyError(error); + expect(result.retryable).toBe(true); + expect(result.reason).toContain('network error'); + }); + + it('should classify "socket closed" as retryable network error', () => { + const error = new Error('The socket closed unexpectedly'); + const result = classifyError(error); + expect(result.retryable).toBe(true); + expect(result.reason).toContain('network error'); + }); + + it('should classify unknown errors as non-retryable', () => { + const error = new Error('Something weird happened'); + const result = classifyError(error); + expect(result.retryable).toBe(false); + expect(result.reason).toContain('Non-retryable'); + }); + + it('should classify null as non-retryable', () => { + const result = classifyError(null); + expect(result.retryable).toBe(false); + expect(result.reason).toContain('Non-retryable'); + }); + + it('should classify undefined as non-retryable', () => { + const result = classifyError(undefined); + expect(result.retryable).toBe(false); + expect(result.reason).toContain('Non-retryable'); + }); +}); + +describe('retryWithBackoff integration — defaultShouldRetry new error paths', () => { + beforeEach(() => { + vi.useFakeTimers(); + setSimulate429(false); + console.warn = vi.fn(); + }); + + afterEach(() => { + vi.restoreAllMocks(); + vi.useRealTimers(); + }); + + // --- 408 Request Timeout --- + // Note: defaultShouldRetry only retries 429/5xx. 408 requires a custom + // shouldRetryOnError (e.g. classifyError) — these tests verify that + // callers using classifyError-based retry DO retry on 408. + + it('should retry on 408 when shouldRetryOnError uses classifyError', async () => { + let attempts = 0; + const mockFn = vi.fn(async () => { + attempts++; + if (attempts === 1) { + const error = new Error('Request Timeout') as any; + error.status = 408; + throw error; + } + return 'ok'; + }); + + const promise = retryWithBackoff(mockFn, { + maxAttempts: 3, + initialDelayMs: 10, + shouldRetryOnError: (e) => classifyError(e).retryable, + }); + + await vi.runAllTimersAsync(); + const result = await promise; + + expect(result).toBe('ok'); + expect(mockFn).toHaveBeenCalledTimes(2); + }); + + it('should exhaust retries on persistent 408 with classifyError', async () => { + const mockFn = vi.fn(async () => { + const error = new Error('Request Timeout') as any; + error.status = 408; + throw error; + }); + + const promise = retryWithBackoff(mockFn, { + maxAttempts: 2, + initialDelayMs: 10, + shouldRetryOnError: (e) => classifyError(e).retryable, + }); + + // eslint-disable-next-line vitest/valid-expect + const assertionPromise = expect(promise).rejects.toThrow('Request Timeout'); + await vi.runAllTimersAsync(); + await assertionPromise; + + expect(mockFn).toHaveBeenCalledTimes(2); + }); + + // --- 409 Conflict (transient vs deterministic) --- + + it('should retry on 409 with lock contention message when using classifyError', async () => { + let attempts = 0; + const mockFn = vi.fn(async () => { + attempts++; + if (attempts === 1) { + const error: HttpError = new Error('Lock contention on resource'); + error.status = 409; + throw error; + } + return 'ok'; + }); + + const promise = retryWithBackoff(mockFn, { + maxAttempts: 3, + initialDelayMs: 10, + shouldRetryOnError: (e) => classifyError(e).retryable, + }); + + await vi.runAllTimersAsync(); + const result = await promise; + + expect(result).toBe('ok'); + expect(mockFn).toHaveBeenCalledTimes(2); + }); + + it('should NOT retry on 409 without transient message', async () => { + const mockFn = vi.fn(async () => { + const error: HttpError = new Error('Resource already exists'); + error.status = 409; + throw error; + }); + + const promise = retryWithBackoff(mockFn, { + maxAttempts: 3, + initialDelayMs: 10, + shouldRetryOnError: (e) => classifyError(e).retryable, + }); + + // Attach rejection handler before running timers to avoid unhandled rejection + // eslint-disable-next-line vitest/valid-expect + const assertionPromise = expect(promise).rejects.toThrow( + 'Resource already exists', + ); + await vi.runAllTimersAsync(); + await assertionPromise; + + expect(mockFn).toHaveBeenCalledTimes(1); + }); + + // --- Network errors --- + + it('should retry on ECONNRESET when shouldRetryOnError uses classifyError', async () => { + let attempts = 0; + const mockFn = vi.fn(async () => { + attempts++; + if (attempts === 1) { + const error = new Error('Connection reset') as NodeJS.ErrnoException; + error.code = 'ECONNRESET'; + throw error; + } + return 'ok'; + }); + + const promise = retryWithBackoff(mockFn, { + maxAttempts: 3, + initialDelayMs: 10, + shouldRetryOnError: (e) => classifyError(e).retryable, + }); + + await vi.runAllTimersAsync(); + const result = await promise; + + expect(result).toBe('ok'); + expect(mockFn).toHaveBeenCalledTimes(2); + }); + + it('should retry on ETIMEDOUT when shouldRetryOnError uses classifyError', async () => { + let attempts = 0; + const mockFn = vi.fn(async () => { + attempts++; + if (attempts === 1) { + const error = new Error('Operation timed out') as NodeJS.ErrnoException; + error.code = 'ETIMEDOUT'; + throw error; + } + return 'ok'; + }); + + const promise = retryWithBackoff(mockFn, { + maxAttempts: 3, + initialDelayMs: 10, + shouldRetryOnError: (e) => classifyError(e).retryable, + }); + + await vi.runAllTimersAsync(); + const result = await promise; + + expect(result).toBe('ok'); + expect(mockFn).toHaveBeenCalledTimes(2); + }); + + it('should retry on "socket closed" message when using classifyError', async () => { + let attempts = 0; + const mockFn = vi.fn(async () => { + attempts++; + if (attempts === 1) { + const error = new Error('The socket closed unexpectedly'); + throw error; + } + return 'ok'; + }); + + const promise = retryWithBackoff(mockFn, { + maxAttempts: 3, + initialDelayMs: 10, + shouldRetryOnError: (e) => classifyError(e).retryable, + }); + + await vi.runAllTimersAsync(); + const result = await promise; + + expect(result).toBe('ok'); + expect(mockFn).toHaveBeenCalledTimes(2); + }); + + it('should exhaust retries on persistent network error with classifyError', async () => { + const mockFn = vi.fn(async () => { + const error = new Error('Connection reset') as NodeJS.ErrnoException; + error.code = 'ECONNRESET'; + throw error; + }); + + const promise = retryWithBackoff(mockFn, { + maxAttempts: 2, + initialDelayMs: 10, + shouldRetryOnError: (e) => classifyError(e).retryable, + }); + + const assertionPromise = + // eslint-disable-next-line vitest/valid-expect + expect(promise).rejects.toThrow('Connection reset'); + await vi.runAllTimersAsync(); + await assertionPromise; + + expect(mockFn).toHaveBeenCalledTimes(2); + }); + + // --- Non-retryable status codes should NOT retry --- + + it('should NOT retry on 401 via defaultShouldRetry', async () => { + const mockFn = vi.fn(async () => { + const error = new Error('Unauthorized') as any; + error.status = 401; + throw error; + }); + + const promise = retryWithBackoff(mockFn, { + maxAttempts: 3, + initialDelayMs: 10, + }); + + // eslint-disable-next-line vitest/valid-expect + const assertionPromise = expect(promise).rejects.toThrow('Unauthorized'); + await vi.runAllTimersAsync(); + await assertionPromise; + + expect(mockFn).toHaveBeenCalledTimes(1); + }); + + it('should NOT retry on 404 via defaultShouldRetry', async () => { + const mockFn = vi.fn(async () => { + const error = new Error('Not Found') as any; + error.status = 404; + throw error; + }); + + const promise = retryWithBackoff(mockFn, { + maxAttempts: 3, + initialDelayMs: 10, + }); + + // eslint-disable-next-line vitest/valid-expect + const assertionPromise = expect(promise).rejects.toThrow('Not Found'); + await vi.runAllTimersAsync(); + await assertionPromise; + + expect(mockFn).toHaveBeenCalledTimes(1); + }); +}); diff --git a/packages/core/src/utils/retry.ts b/packages/core/src/utils/retry.ts index 54b0b6db9e0..0d71eb20847 100644 --- a/packages/core/src/utils/retry.ts +++ b/packages/core/src/utils/retry.ts @@ -8,7 +8,12 @@ import type { GenerateContentResponse } from '@google/genai'; import { AuthType } from '../core/contentGenerator.js'; import { isQwenQuotaExceededError } from './quotaErrorDetection.js'; import { createDebugLogger } from './debugLogger.js'; -import { getErrorStatus } from './errors.js'; +import { + getErrorStatus, + getErrorMessage, + getErrorType, + isNodeError, +} from './errors.js'; const debugLogger = createDebugLogger('RETRY'); @@ -52,25 +57,39 @@ const DEFAULT_RETRY_OPTIONS: RetryOptions = { /** * Default predicate function to determine if a retry should be attempted. - * Retries on 429 (Too Many Requests) and 5xx server errors. + * Retries on unambiguous transient capacity errors: 429 (Rate Limit) and 5xx + * (Server Errors). Does NOT retry on 408, 409, or network transport errors — + * those require caller-specific judgment (e.g. geminiChat uses classifyError + * directly for broader retry coverage including 408/409/network errors). + * + * Never retries deterministic request errors (400, 401, 403, 404, 422). + * * @param error The error object. * @returns True if the error is a transient error, false otherwise. */ function defaultShouldRetry(error: Error | unknown): boolean { const status = getErrorStatus(error); - return ( - status === 429 || (status !== undefined && status >= 500 && status < 600) - ); + if (status === 429) return true; + if (typeof status === 'number' && status >= 500 && status < 600) return true; + return false; } /** - * Determines if an error is a transient capacity error eligible for persistent retry. - * Only 429 (Rate Limit) and 529 (Overloaded) qualify — HTTP 500 is excluded - * because it may indicate a permanent server bug. + * Determines if an error is a transient capacity error eligible for persistent + * retry. Only 429 (Rate Limit) and 529 (Overloaded) qualify — these are + * unambiguous capacity signals safe to retry indefinitely in unattended mode. + * + * 408 and network errors are intentionally excluded: they can indicate permanent + * configuration issues (wrong endpoint, firewall block, proxy timeout) that would + * cause an unattended job to hang for hours instead of failing fast. + * These remain retryable in standard (non-persistent) retry mode via classifyError. */ export function isTransientCapacityError(error: unknown): boolean { const status = getErrorStatus(error); - return status === 429 || status === 529; + if (status === 429 || status === 529) { + return true; + } + return false; } /** @@ -357,3 +376,146 @@ function logRetryAttempt( debugLogger.warn(message, error); } } + +/** + * Network error codes that indicate a transient transport failure. + * These are retryable because they indicate temporary network issues, + * not deterministic request errors. + */ +const RETRYABLE_NETWORK_CODES = new Set([ + 'ECONNRESET', + 'ETIMEDOUT', + 'ESOCKETTIMEDOUT', + 'ECONNREFUSED', + 'EAI_AGAIN', +]); + +/** + * Determine if a 409 Conflict error is likely transient. + * Some providers use 409 for lock contention that may resolve. + * Only checks message content — does NOT fall back to status code matching + * (the caller already knows status === 409 to invoke this function). + */ +function isTransientConflict(error: Error | unknown): boolean { + const message = getErrorMessage(error).toLowerCase(); + // Only 'lock'/'locked' (as whole words) and 'contention' are reliable transient signals. + // \b on both sides prevents false positives on "blocked", "clock", "flock". + // 'conflict' is excluded because it appears in the standard HTTP 409 reason + // phrase "Conflict", which would make all standards-compliant 409s transient. + return ( + new RegExp('\\bLOCKS?\\b', 'i').test(message) || + message.includes('contention') + ); +} + +/** + * Check if an error is a retryable network transport failure. + * These are distinct from deterministic request errors (400, 401, 403, 404, 422). + */ +export function isRetryableNetworkError(error: Error | unknown): boolean { + // Check Node.js error codes (ECONNRESET, ETIMEDOUT, etc.) + if (isNodeError(error)) { + const nodeError = error as NodeJS.ErrnoException; + if (nodeError.code && RETRYABLE_NETWORK_CODES.has(nodeError.code)) { + return true; + } + } + + // Check error message patterns for network/socket issues + const message = getErrorMessage(error).toLowerCase(); + if ( + message.includes('socket closed') || + message.includes('stream ended') || + message.includes('connection reset') || + message.includes('econnreset') || + message.includes('etimedout') + ) { + return true; + } + + return false; +} + +/** + * Classification result for an error. + */ +export interface ErrorClassification { + retryable: boolean; + reason: string; + status?: number; +} + +/** + * Classify an error as retryable or not. + * Returns an object with the classification and reason. + */ +export function classifyError(error: Error | unknown): ErrorClassification { + const status = getErrorStatus(error); + + // Deterministic request errors — never retry + if ( + status === 400 || + status === 401 || + status === 403 || + status === 404 || + status === 422 + ) { + const statusText = status ? `HTTP ${status}` : 'unknown status'; + return { + retryable: false, + reason: `Deterministic request error: ${statusText}`, + status, + }; + } + + // Retryable status codes + if (status === 429) { + return { + retryable: true, + reason: `Rate limited (HTTP 429)`, + status, + }; + } + + if (status === 408) { + return { + retryable: true, + reason: `Request timeout (HTTP 408)`, + status, + }; + } + + if (status === 409) { + const transient = isTransientConflict(error); + return { + retryable: transient, + reason: transient + ? 'Transient conflict/lock (HTTP 409)' + : 'Deterministic conflict (HTTP 409)', + status, + }; + } + + if (status !== undefined && status >= 500 && status < 600) { + return { + retryable: true, + reason: `Server error (HTTP ${status})`, + status, + }; + } + + // Network errors + if (isRetryableNetworkError(error)) { + const errorType = getErrorType(error); + return { + retryable: true, + reason: `Retryable network error: ${errorType}`, + }; + } + + // Unknown — not retryable + return { + retryable: false, + reason: `Non-retryable error: ${getErrorType(error)}`, + }; +}