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..73eed928b89 --- /dev/null +++ b/.serena/.gitignore @@ -0,0 +1,3 @@ +/cache +/project.local.yml +/project.yml 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/core/src/core/geminiChat.test.ts b/packages/core/src/core/geminiChat.test.ts index 39cfed3d5de..c595d3e4ee2 100644 --- a/packages/core/src/core/geminiChat.test.ts +++ b/packages/core/src/core/geminiChat.test.ts @@ -1700,6 +1700,180 @@ 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); + }); + 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..e33d802e749 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,28 @@ 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; + // isSchemaDepthError / isInvalidArgumentError above are independent + // safety nets not covered by classifyError — do not remove them. + // 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. + const classification = classifyError(error); + if (classification.retryable) { + debugLogger.debug( + `geminiChat retrying: status=${classification.status ?? 'none'}, reason=${classification.reason}`, + ); + } + return classification.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..84df566ddad 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,92 @@ 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(); + }); + + it('should disconnect failed client and clean up when discoverMcpToolsForServer fails', async () => { + const failedClient = { + connect: vi.fn().mockRejectedValue(new Error('connection refused')), + discover: vi.fn().mockResolvedValue(undefined), + disconnect: vi.fn().mockResolvedValue(undefined), + getStatus: vi.fn(), + }; + + vi.mocked(McpClient).mockReturnValue(failedClient 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', { + isTrustedFolder: () => true, + } as unknown as Config); + + // The catch block should disconnect the failed client before deleting it. + expect(failedClient.disconnect).toHaveBeenCalledOnce(); + + // After failure cleanup, a subsequent discovery should proceed (in-flight + // tracking was cleared and client was removed). + 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', { + isTrustedFolder: () => true, + } as unknown as Config); + + 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..06874a277c5 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,20 @@ export class McpClientManager { error, )}`, ); + // Remove the failed client gracefully: disconnect first to avoid + // orphaning any child process that connect() may have spawned. + const failedClient = this.clients.get(serverName); + if (failedClient) { + try { + await failedClient.disconnect(); + } catch { + // Best-effort cleanup; ignore disconnect errors. + } + } + this.clients.delete(serverName); + this.stopHealthCheck(serverName); } finally { + this.inFlightDiscoveries.delete(serverName); this.eventEmitter?.emit('mcp-client-update', this.clients); } } @@ -231,6 +260,7 @@ export class McpClientManager { this.clients.clear(); this.consecutiveFailures.clear(); this.isReconnecting.clear(); + this.inFlightDiscoveries.clear(); } /** @@ -253,6 +283,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); } } @@ -390,9 +421,14 @@ export class McpClientManager { 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 +518,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..7e5dfcbf7e4 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', @@ -188,8 +196,10 @@ describe('retryWithBackoff', () => { }); // Attach the rejection expectation *before* running timers + /* eslint-disable vitest/valid-expect */ const assertionPromise = - expect(promise).rejects.toThrow('Too Many Requests'); // eslint-disable-line vitest/valid-expect + expect(promise).rejects.toThrow('Too Many Requests'); + /* eslint-enable vitest/valid-expect */ // Run timers to trigger retries and eventual rejection await vi.runAllTimersAsync(); @@ -250,13 +260,14 @@ 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(); await vi.runAllTimersAsync(); // Advance for the delay in the first runRetry @@ -272,6 +283,7 @@ 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(); await vi.runAllTimersAsync(); // Advance for the delay in the second runRetry @@ -291,10 +303,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 +511,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 +750,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 +1061,547 @@ 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); + }); + + // DNS/route failures are treated as permanent misconfiguration, not + // transient provider instability. Retrying them just wastes time and + // hides the actual configuration problem (wrong hostname, firewall block). + it('does not retry permanent network misconfiguration errors', () => { + const enotfound = new Error('Not found'); + (enotfound as NodeJS.ErrnoException).code = 'ENOTFOUND'; + expect(isRetryableNetworkError(enotfound)).toBe(false); + + const ehostunreach = new Error('Host unreachable'); + (ehostunreach as NodeJS.ErrnoException).code = 'EHOSTUNREACH'; + expect(isRetryableNetworkError(ehostunreach)).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 true for "network error" message', () => { + const error = new Error('A network error occurred'); + expect(isRetryableNetworkError(error)).toBe(true); + }); + + 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); + }); + + // Word-boundary regression: substrings that contain the keyword but are + // not the actual phrase must NOT match. These guard against the previous + // `message.includes(...)` approach which had no boundary checks. + it('should return false for messages that merely contain retryable keywords as substrings', () => { + // "notanetwork error" — no word boundary before "network" + expect(isRetryableNetworkError(new Error('notanetwork error'))).toBe(false); + // "network erroring" — no word boundary after "error" + expect(isRetryableNetworkError(new Error('network erroring'))).toBe(false); + // "presocket closedapp" — no word boundaries around "socket closed" + expect(isRetryableNetworkError(new Error('presocket closedapp'))).toBe( + false, + ); + // "stream endedness" — no word boundary after "ended" + expect(isRetryableNetworkError(new Error('stream endedness'))).toBe(false); + // "xeconnresetting" — no word boundaries around "econnreset" + expect(isRetryableNetworkError(new Error('xeconnresetting'))).toBe(false); + // "gettimedoutnow" — no word boundaries around "etimedout" + expect(isRetryableNetworkError(new Error('gettimedoutnow'))).toBe(false); + }); + + 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); + }); + + // Word-boundary regression: "lock" as a substring of a non-transient word + // must NOT trigger retryable classification. + it('should classify 409 with lock-as-substring message as non-retryable', () => { + // "blocked", "clock", "flock" all contain "lock" but are not transient. + const blocked: HttpError = Object.assign(new Error('Resource is blocked'), { + status: 409, + }); + expect(classifyError(blocked).retryable).toBe(false); + + const clock: HttpError = Object.assign(new Error('Clock skew detected'), { + status: 409, + }); + expect(classifyError(clock).retryable).toBe(false); + + const flock: HttpError = Object.assign(new Error('Flock timeout'), { + status: 409, + }); + expect(classifyError(flock).retryable).toBe(false); + }); + + // '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'); + }); + + // A deterministic 409 whose .cause mentions "lock" must NOT be classified + // as transient. isTransientConflict reads .message directly, not + // getErrorMessage() which concatenates .cause. + it('should classify 409 with lock in cause but not message as non-retryable', () => { + const cause = new Error('Lock file stale'); + const error: HttpError = Object.assign(new Error('Duplicate resource'), { + status: 409, + cause, + }); + 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, + }); + + /* eslint-disable vitest/valid-expect */ + const assertionPromise = + expect(promise).rejects.toThrow('Connection reset'); + /* eslint-enable vitest/valid-expect */ + 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..7d02ff15439 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,152 @@ 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', + // ENOTFOUND and EHOSTUNREACH are intentionally excluded: DNS/routing + // failures are almost always permanent misconfigurations (wrong hostname, + // firewall block) and should fail fast rather than retry 7x (~76s). +]); + +/** + * 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 { + // Read .message directly instead of getErrorMessage(), which concatenates + // .cause and could inject a transient keyword from a wrapped deterministic error. + const rawMessage = error instanceof Error ? error.message : String(error); + const message = rawMessage.toLowerCase(); + // Use word boundaries to avoid matching substrings like "blocked", "clock", "flock". + // Only 'lock' and 'contention' are reliable transient signals. + // 'conflict' is excluded because it appears in the standard HTTP 409 reason + // phrase "Conflict", which would make all standards-compliant 409s transient. + return /\block\b/.test(message) || /\bcontention\b/.test(message); +} + +/** + * 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. + // Use word-boundary regexes to avoid false positives from substrings + // (e.g. "notanetwork error", "econnresetting", "stream endedness"). + const message = getErrorMessage(error).toLowerCase(); + if ( + /\bsocket closed\b/.test(message) || + /\bstream ended\b/.test(message) || + /\bnetwork error\b/.test(message) || + /\bconnection reset\b/.test(message) || + /\beconnreset\b/.test(message) || + /\betimedout\b/.test(message) + ) { + 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)}`, + }; +}