fix: mimo-parallel-tool-call-policy (2/2) - #1130
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds provider-aware tool-call policies, strict schema settings, typed parser failures, streamed ghost-call quarantine, enforcement telemetry, MiMo fallback handling, related tests, and branch-cleanup documentation. ChangesTool-call policy and telemetry
Repository reports
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (10)
scripts/find-dup-json-keys.js-156-160 (1)
156-160: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winReplace
process.exitwithprocess.exitCodeto avoid truncated output.
process.exitterminates the process before pending asynchronous stdout writes flush. When a caller pipes the output (for examplenode scripts/find-dup-json-keys.js src | tee report.txt),console.logwrites to a pipe are asynchronous, so the last lines can be lost. Setprocess.exitCodeinstead and let Node exit after the stream drains.♻️ Proposed fix
console.log(found === 0 ? "OK: no duplicate keys found" : `TOTAL: ${found} duplicate key occurrence(s)`) -process.exit(found === 0 ? 0 : 1) +process.exitCode = found === 0 ? 0 : 1🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/find-dup-json-keys.js` around lines 156 - 160, Update the final status handling in the duplicate-key script to assign the computed success or failure code to process.exitCode instead of calling process.exit, while preserving the existing console.log message and exit-code values so pending output can flush before Node terminates.scripts/find-dup-json-keys.js-141-159 (1)
141-159: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCount parse errors separately from duplicate keys.
Line 150 increments
foundfor a parse error. Line 159 then reports the same counter asduplicate key occurrence(s). A file that fails to parse is reported as a duplicate key, which misstates the result. Track the two conditions in separate counters, and keep the non-zero exit status when either counter is non-zero.♻️ Proposed fix
let found = 0 +let parseErrors = 0 for (const target of process.argv.slice(2)) { for (const file of walk(target)) { const text = fs.readFileSync(file, "utf8") let dups try { dups = findDuplicates(text) } catch (e) { console.log(`${file}: PARSE ERROR ${e.message}`) - found++ + parseErrors++ continue } for (const d of dups) { console.log(`${file}: duplicate key "${d.key}" at line ${d.line}`) found++ } } } -console.log(found === 0 ? "OK: no duplicate keys found" : `TOTAL: ${found} duplicate key occurrence(s)`) +console.log( + found === 0 && parseErrors === 0 + ? "OK: no duplicate keys found" + : `TOTAL: ${found} duplicate key occurrence(s), ${parseErrors} parse error(s)`, +)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/find-dup-json-keys.js` around lines 141 - 159, The code uses a single counter `found` for both parse errors and duplicate key occurrences, causing the final message to misreport parse errors as duplicate key occurrences. Create a separate counter variable for parse errors and keep `found` for duplicate keys only. In the catch block where the parse error is handled, increment the parse error counter instead of `found`. Update the final console.log message to report parse error count and duplicate key count separately, and ensure the non-zero exit status is triggered if either counter is non-zero.docs/260730_0001_session_branch-cleanup/173230_execution-plan.md-73-75 (1)
73-75: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winAllow for an empty commit during verification.
The execution report records 17 output commits because one of the 18 input commits became empty. This gate still requires exactly 18 commits. Expect feature-only commits and document that the count may be 17 when Git drops an empty commit.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/260730_0001_session_branch-cleanup/173230_execution-plan.md` around lines 73 - 75, Update the verification expectation for the git log so it allows 17 or 18 feature-only commits, documenting that Git may drop one empty commit during replay. Keep the exclusions for unrelated commits unchanged.webview-ui/src/i18n/locales/hi/settings.json-968-969 (1)
968-969: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTranslate the new locale strings.
These locale files display English text to users of their selected language. Replace both new values with translations for the target locale.
webview-ui/src/i18n/locales/hi/settings.json#L968-L969: add Hindi translations.webview-ui/src/i18n/locales/id/settings.json#L968-L969: add Indonesian translations.webview-ui/src/i18n/locales/it/settings.json#L968-L969: add Italian translations.webview-ui/src/i18n/locales/ja/settings.json#L968-L969: add Japanese translations.webview-ui/src/i18n/locales/ko/settings.json#L968-L969: add Korean translations.webview-ui/src/i18n/locales/nl/settings.json#L968-L969: add Dutch translations.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@webview-ui/src/i18n/locales/hi/settings.json` around lines 968 - 969, The new strictToolSchemas and strictToolSchemasDescription keys contain English text but need to be translated into the target languages for users. In webview-ui/src/i18n/locales/hi/settings.json (lines 968-969), replace the English values with Hindi translations. In webview-ui/src/i18n/locales/id/settings.json (lines 968-969), add Indonesian translations. In webview-ui/src/i18n/locales/it/settings.json (lines 968-969), add Italian translations. In webview-ui/src/i18n/locales/ja/settings.json (lines 968-969), add Japanese translations. In webview-ui/src/i18n/locales/ko/settings.json (lines 968-969), add Korean translations. In webview-ui/src/i18n/locales/nl/settings.json (lines 968-969), add Dutch translations. Each file should preserve the JSON structure with the same keys but localized string values appropriate for its target language audience.webview-ui/src/i18n/locales/zh-TW/settings.json-995-996 (1)
995-996: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winTranslate the new
strictToolSchemassettings into Traditional Chinese.
strictToolSchemasandstrictToolSchemasDescriptionstill show English text inwebview-ui/src/i18n/locales/zh-TW/settings.json, so zh-TW users will see untranslated labels. Translate these entries or route them through the project translation workflow before merge.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@webview-ui/src/i18n/locales/zh-TW/settings.json` around lines 995 - 996, Translate the `strictToolSchemas` and `strictToolSchemasDescription` entries in the zh-TW settings locale into Traditional Chinese, preserving the original labels’ meaning and the description’s strict-mode and MCP-tool behavior.src/core/assistant-message/NativeToolCallParser.ts-1191-1219 (1)
1191-1219: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winStructural failures now log and store
"[object Object]"as the legacy diagnostic string.The three throw sites throw plain object literals, not
Errorinstances. The catch block at Line 1249 computeserror instanceof Error ? error.message : String(error). For these tagged objectsString(error)returns"[object Object]". That value is written toparseErrorsat Line 1257 and printed byconsole.errorat Line 1251. Every missing-argument and invalid-shape failure therefore loses its human-readable diagnostic, which is the only purpose the legacy string channel still serves.Add a
messagefield to the tagged throws and prefer it when buildingerrorMessage. This also removes the throw-literal pattern for the message path.The test at
src/core/assistant-message/__tests__/NativeToolCallParser.spec.ts:409only asserts the string is defined, so it does not detect this.🐛 Proposed fix to keep a readable diagnostic string
if (!isPlainObject) { throw { __parserFailureKind: "invalid_argument_shape" as const, + message: `Tool '${resolvedName}' received arguments that are not a JSON object`, toolName: resolvedName as string, missingParameters: [], emptyArguments: false, } } const required = NativeToolCallParser.REQUIRED_PARAMETERS[resolvedName as string] ?? [] const missing = required.filter((p) => args[p] === undefined) const isEmpty = Object.keys(args).length === 0 if (missing.length > 0) { throw { __parserFailureKind: "missing_required_arguments" as const, + message: `Tool '${resolvedName}' is missing required parameter(s): ${missing.join(", ")}`, toolName: resolvedName as string, missingParameters: missing, emptyArguments: isEmpty, } } // Required fields are present but the structural shape didn't match // any known pattern in the switch above. throw { __parserFailureKind: "invalid_argument_shape" as const, + message: `Tool '${resolvedName}' received arguments with an unexpected shape`, toolName: resolvedName as string, missingParameters: [], emptyArguments: isEmpty, }Then read that field in the catch block:
- const errorMessage = error instanceof Error ? error.message : String(error) + const errorMessage = + error instanceof Error + ? error.message + : typeof error === "object" && error !== null && typeof (error as { message?: unknown }).message === "string" + ? (error as { message: string }).message + : String(error)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/assistant-message/NativeToolCallParser.ts` around lines 1191 - 1219, Add a readable message field to all tagged structural-failure throws in NativeToolCallParser, including invalid_argument_shape and missing_required_arguments cases, describing the tool and failure. Update the catch block’s errorMessage construction to prefer the tagged message before falling back to Error.message or String(error), so parseErrors and console.error retain useful diagnostics.webview-ui/src/i18n/locales/tr/settings.json-968-969 (1)
968-969: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTranslate the new strict-schema strings.
The new values are English in three non-English locale files.
webview-ui/src/i18n/locales/tr/settings.json#L968-L969: Add Turkish translations.webview-ui/src/i18n/locales/vi/settings.json#L968-L969: Add Vietnamese translations.webview-ui/src/i18n/locales/zh-CN/settings.json#L968-L969: Add Simplified Chinese translations.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@webview-ui/src/i18n/locales/tr/settings.json` around lines 968 - 969, Translate the strictToolSchemas and strictToolSchemasDescription values from English into the target languages in webview-ui/src/i18n/locales/tr/settings.json lines 968-969, webview-ui/src/i18n/locales/vi/settings.json lines 968-969, and webview-ui/src/i18n/locales/zh-CN/settings.json lines 968-969. Preserve the existing keys and meaning, including strict schema behavior, provider support limitations, and the non-strict handling of MCP tools.webview-ui/src/i18n/locales/ca/settings.json-968-969 (1)
968-969: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTranslate the new locale values.
These locale-specific catalogs add English labels and descriptions. Users of these locales will see English text in the strict-schema setting.
webview-ui/src/i18n/locales/ca/settings.json#L968-L969: add Catalan translations.webview-ui/src/i18n/locales/pl/settings.json#L968-L969: add Polish translations.webview-ui/src/i18n/locales/pt-BR/settings.json#L968-L969: add Brazilian Portuguese translations.webview-ui/src/i18n/locales/ru/settings.json#L968-L969: add Russian translations.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@webview-ui/src/i18n/locales/ca/settings.json` around lines 968 - 969, Translate the strictToolSchemas and strictToolSchemasDescription values in webview-ui/src/i18n/locales/ca/settings.json lines 968-969 into Catalan, webview-ui/src/i18n/locales/pl/settings.json lines 968-969 into Polish, webview-ui/src/i18n/locales/pt-BR/settings.json lines 968-969 into Brazilian Portuguese, and webview-ui/src/i18n/locales/ru/settings.json lines 968-969 into Russian, preserving the existing keys and meaning.webview-ui/src/i18n/locales/de/settings.json-967-969 (1)
967-969: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTranslate the new locale values.
The new labels remain in English in non-English locale files.
webview-ui/src/i18n/locales/de/settings.json#L967-L969: Translate both values to German.webview-ui/src/i18n/locales/es/settings.json#L967-L969: Translate both values to Spanish.webview-ui/src/i18n/locales/fr/settings.json#L967-L969: Translate both values to French.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@webview-ui/src/i18n/locales/de/settings.json` around lines 967 - 969, Translate both strictToolSchemas and strictToolSchemasDescription in webview-ui/src/i18n/locales/de/settings.json (lines 967-969) into German, webview-ui/src/i18n/locales/es/settings.json (lines 967-969) into Spanish, and webview-ui/src/i18n/locales/fr/settings.json (lines 967-969) into French, preserving the existing JSON keys and meaning.src/api/providers/__tests__/mimo.spec.ts-693-695 (1)
693-695: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPass valid metadata without a double assertion.
ApiHandlerCreateMessageMetadatarequirestaskId. This cast hides that contract from the test. Pass a test task ID directly.Proposed fix
- const stream = handler.createMessage("System prompt", messages, { - tools, - } as unknown as ApiHandlerCreateMessageMetadata) + const stream = handler.createMessage("System prompt", messages, { + taskId: "test-task", + tools, + })Run
pnpm --dir src exec eslint --prune-suppressions --max-warnings=0 api/providers/__tests__/mimo.spec.tsafter the change.As per coding guidelines, "Use double assertions only as a last resort and explain them with a comment."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api/providers/__tests__/mimo.spec.ts` around lines 693 - 695, Remove the double assertion (as unknown as ApiHandlerCreateMessageMetadata) from the metadata object passed to handler.createMessage. Instead, provide a properly typed metadata object that includes the required taskId property along with the existing tools property. This will ensure the test respects the ApiHandlerCreateMessageMetadata contract rather than bypassing type safety.Source: Coding guidelines
🧹 Nitpick comments (5)
scripts/find-dup-json-keys.js (3)
21-21: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unused
isArrayframe field.
parseArraynever pushes a stack frame, because arrays have no keys to track. Every frame is therefore an object frame, andisArrayis alwaysfalseand never read. Drop the field and the comment reference to keep the frame shape honest.♻️ Proposed fix
- const stack = [] // each frame: { keys: Set<string>, isArray: bool } + const stack = [] // each frame: { keys: Set<string> } for the enclosing objectAlso update the push at line 78:
stack.push({ keys: new Set() })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/find-dup-json-keys.js` at line 21, Remove the unused isArray field from the stack frame comment and update the stack.push call to create frames with only the keys Set. Keep the existing object-key tracking behavior unchanged.
95-98: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winValidate the
:separator instead of advancing blindly.Line 97 advances past one character without checking that the character is
:. If the separator is absent, the scanner silently continues at the wrong offset and can report incorrect duplicates or incorrect line numbers. The surrounding code throwsunexpected charfor other malformed input, so an explicit check keeps the error behavior consistent.♻️ Proposed fix
skipWs() - // expect ':' - i++ + if (text[i] !== ":") { + throw new Error(`expected ':' after key "${key}" at line ${line}`) + } + i++ skipValue()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/find-dup-json-keys.js` around lines 95 - 98, Update the object-key scanning logic around skipWs and skipValue to validate that the current character is ':' before advancing. If it is not, throw the same unexpected-char error used for other malformed input; otherwise advance and continue skipping the value.
6-15: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winSkip generated and VCS directories, and guard against symlink cycles.
walkdescends into every directory. If a caller passes the repository root, the scan traversesnode_modules,.git,dist, andout, which adds large amounts of work and reports duplicate keys in third-party files.fs.statSyncalso follows symlinks, so a symlink that points to an ancestor directory produces unbounded recursion.Use
withFileTypesto inspect entries without following symlinks, and skip known generated directories.♻️ Proposed fix
+const SKIP_DIRS = new Set(["node_modules", ".git", "dist", "out", "build", ".turbo"]) + function* walk(target) { const stat = fs.statSync(target) if (stat.isDirectory()) { - for (const entry of fs.readdirSync(target)) { - yield* walk(path.join(target, entry)) + for (const entry of fs.readdirSync(target, { withFileTypes: true })) { + if (entry.isSymbolicLink()) continue + if (entry.isDirectory() && SKIP_DIRS.has(entry.name)) continue + yield* walk(path.join(target, entry.name)) } } else if (target.endsWith(".json")) { yield target } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/find-dup-json-keys.js` around lines 6 - 15, Update walk to use directory-entry metadata via readdirSync with withFileTypes enabled, avoid descending through symlinked entries, and skip node_modules, .git, dist, and out directories before recursion. Preserve yielding only .json files while preventing ancestor-link cycles and unnecessary traversal.src/core/task/Task.ts (1)
3024-3041: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the repeated ghost-drop telemetry block.
The same ten-line block appears three times: Lines 3024-3041, Lines 3115-3132, and Lines 3516-3533. The only difference is the local variable name (
ghostPolicy1,ghostPolicy2,ghostPolicy3), which exists only to avoid shadowing. Each copy re-resolves the policy and callsthis.api.getModel()twice.Extract one private method and call it from all three sites. All three copies also omit
parallelToolCallsSent, while the policy-resolution event at Line 4561 sends it; decide the field once in the helper.♻️ Proposed helper
private emitGhostDrop(): void { const policy = resolveToolCallPolicy(this.api.getModel().info, this.apiConfiguration.apiProvider) emitGhostDropTelemetry({ taskId: this.taskId, provider: this.apiConfiguration.apiProvider ?? "unknown", model: this.api.getModel().id, policySource: policy.source, maxCallsPerTurn: policy.maxCallsPerTurn, enforcement: policy.enforcement, callCount: this.assistantMessageContent.filter( (b: AssistantMessageContent): b is ToolUse => b.type === "tool_use", ).length, ghostDroppedCount: 1, errorResultCount: 0, parallelToolCallsRequested: policy.generation === "parallel", }) }Then replace each site:
- const ghostPolicy1 = resolveToolCallPolicy( - this.api.getModel().info, - this.apiConfiguration.apiProvider, - ) - emitGhostDropTelemetry({ - taskId: this.taskId, - provider: this.apiConfiguration.apiProvider ?? "unknown", - model: this.api.getModel().id, - policySource: ghostPolicy1.source, - maxCallsPerTurn: ghostPolicy1.maxCallsPerTurn, - enforcement: ghostPolicy1.enforcement, - callCount: this.assistantMessageContent.filter( - (b: AssistantMessageContent): b is ToolUse => b.type === "tool_use", - ).length, - ghostDroppedCount: 1, - errorResultCount: 0, - parallelToolCallsRequested: ghostPolicy1.generation === "parallel", - }) + this.emitGhostDrop()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/task/Task.ts` around lines 3024 - 3041, Extract the duplicated ghost-drop telemetry logic into one private emitGhostDrop method in Task, resolving the policy and model once within the helper and setting parallelToolCallsSent consistently with the existing policy-resolution telemetry. Replace all three inline blocks using ghostPolicy1, ghostPolicy2, and ghostPolicy3 with calls to this helper, preserving the current telemetry values.src/core/assistant-message/ToolCallRetentionPolicy.ts (1)
203-310: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCollapse the two identical telemetry emitters.
GhostDropTelemetryInputandMaxOneEnforcementTelemetryInputdeclare the same fields.emitGhostDropTelemetryandemitMaxOneEnforcementTelemetryhave byte-identical bodies: the samehasInstanceguard and the same property mapping intocaptureToolCallEnforcement. The two copies can drift when the event contract changes.Define one input type and one emitter, then keep the two named exports as thin aliases so the call sites and tests stay unchanged.
♻️ Proposed consolidation
+export interface ToolCallEnforcementTelemetryInput { + /** The task identifier. */ + taskId: string + /** The provider name (e.g. "mimo", "openai"). */ + provider: string + /** The model ID. */ + model: string + /** The resolved policy source. */ + policySource: string + /** The resolved max-calls-per-turn limit. */ + maxCallsPerTurn: 1 | "unbounded" + /** The resolved enforcement mode. */ + enforcement: string + /** Total tool calls in the turn. */ + callCount: number + /** How many ghosts were dropped in this turn. */ + ghostDroppedCount: number + /** How many error results were emitted in this turn. */ + errorResultCount: number + /** What the metadata requested for parallel tool calls. */ + parallelToolCallsRequested: boolean + /** What was sent to the provider (if known). */ + parallelToolCallsSent?: boolean +} + +/** + * Emit a tool-call enforcement telemetry event. + * + * **Privacy:** This function emits ONLY counts and metadata. It does NOT emit + * the call ID, tool name, argument bytes, command strings, file paths, or any + * raw user data. + */ +function emitToolCallEnforcementTelemetry(input: ToolCallEnforcementTelemetryInput): void { + if (!TelemetryService.hasInstance()) { + return + } + + const { taskId, ...properties } = input + TelemetryService.instance.captureToolCallEnforcement(taskId, properties) +} + +export type GhostDropTelemetryInput = ToolCallEnforcementTelemetryInput +export type MaxOneEnforcementTelemetryInput = ToolCallEnforcementTelemetryInput + +export const emitGhostDropTelemetry = emitToolCallEnforcementTelemetry +export const emitMaxOneEnforcementTelemetry = emitToolCallEnforcementTelemetry🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/assistant-message/ToolCallRetentionPolicy.ts` around lines 203 - 310, Create a single consolidated input type and emitter function to replace the duplicate GhostDropTelemetryInput, MaxOneEnforcementTelemetryInput, emitGhostDropTelemetry, and emitMaxOneEnforcementTelemetry implementations. Define one input interface with all the shared fields and one emitter function with the single hasInstance check and captureToolCallEnforcement call, then re-export the original type names and function names as type aliases and thin wrapper functions pointing to the consolidated implementation to preserve existing call sites.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/260730_0001_session_branch-cleanup/173200_debug-report.md`:
- Around line 3-10: The task summary in the report contradicts itself by
claiming Git mutations were VP-only while also describing a throwaway rebase.
Update the summary to identify who authorized and performed the temporary
branch/rebase operations, or remove/revise the rebase statement so it accurately
reflects the audit trail; keep the diagnostic and planning scope clear.
In `@docs/260730_0001_session_branch-cleanup/173230_execution-plan.md`:
- Around line 33-42: Remove the obsolete rebase command targeting
feat/error-interception-middleware-clean from Step 3, leaving only the corrected
sequence that checks out feat/error-interception-remote-src and rebases that
branch onto main.
In `@docs/260730_0001_session_branch-cleanup/181500_debug-dnd-ux-runbook.md`:
- Around line 319-325: Update the rollback command in the “7. Rollback” section
to check out feature/task-dnd-ux, or another explicitly documented recovery
branch, instead of feat/error-interception-middleware; keep the cleanup and
backup-preservation commands unchanged.
- Around line 248-251: Update the git cherry-pick instructions to use --ours for
src/core/webview/ClineProvider.ts, preserving the clean squash file rather than
the incoming contaminated commit. Instruct the reader to stage the file
afterward and verify that only the intended model and spec changes remain
staged.
In `@docs/260730_0001_session_branch-cleanup/184700_debug-report.md`:
- Line 90: Update the documented conflict resolution for StructuralValidator.ts
to remove only src/core/tools/error-interception/StructuralValidator.ts,
avoiding whole-directory restore or broad git rm commands, then verify the
unmerged-path list is clear before continuing the cherry-pick.
- Around line 103-104: Update the documented working-tree precondition around
the git status check to require an actually empty git status, removing the
exception for untracked docs/ files. Ensure the instructions also account for
untracked files that could block git switch -C, either by requiring a clean tree
before switching or by moving documents aside after checking for path
collisions.
In `@src/api/providers/lite-llm.ts`:
- Line 222: The LiteLLM and OpenAI-compatible request paths must forward the
resolved parallel tool-call policy instead of defaulting upstream behavior. In
src/api/providers/lite-llm.ts:222, add parallel_tool_calls to requestOptions
using metadata?.parallelToolCalls ?? true; in
src/api/providers/openai-compatible.ts:165, pass the same value through the
provider-namespaced providerOptions for streamText (or explicitly mark that
route as local-only enforcement). Add request-capture tests covering both true
and false values.
In `@src/api/providers/mimo.ts`:
- Around line 243-261: The completion fallback logic around the
chat.completions.create call must apply compatibility removals cumulatively:
replace the single nested retry with a bounded retry flow that removes each
rejected option at most once, allowing a parallel_tool_calls rejection to be
followed by strict-schema stripping. Ensure all non-retryable or exhausted
errors go through handleProviderError(error, "MiMo"), and add a regression test
covering parallel rejection followed by strict-schema rejection.
In `@src/core/assistant-message/ToolCallRetentionPolicy.ts`:
- Around line 159-198: Integrate selectExecutableCall into the production
tool-call execution path before any calls execute, using the current turn’s
calls and maxCallsPerTurn policy. Execute only executableCallId, prevent all
rejectedCallIds from running, and convert each rejected ID into an error result
so enforcement telemetry is triggered; preserve unbounded and single-valid-call
behavior.
In `@src/core/task/Task.ts`:
- Around line 2998-3045: When removing a ghost block from
assistantMessageContent in both ghost-drop paths at src/core/task/Task.ts lines
2998-3045 and 3499-3535, also decrement currentStreamingContentIndex when
ghostIndex is less than it; apply this reconciliation alongside the existing
array splice, or centralize both paths through a shared helper.
In `@webview-ui/src/i18n/locales/en/settings.json`:
- Around line 1043-1051: Remove the duplicate strictToolSchemas and
strictToolSchemasDescription entries from the modelInfo locale object, retaining
exactly one pair with the existing translations.
---
Minor comments:
In `@docs/260730_0001_session_branch-cleanup/173230_execution-plan.md`:
- Around line 73-75: Update the verification expectation for the git log so it
allows 17 or 18 feature-only commits, documenting that Git may drop one empty
commit during replay. Keep the exclusions for unrelated commits unchanged.
In `@scripts/find-dup-json-keys.js`:
- Around line 156-160: Update the final status handling in the duplicate-key
script to assign the computed success or failure code to process.exitCode
instead of calling process.exit, while preserving the existing console.log
message and exit-code values so pending output can flush before Node terminates.
- Around line 141-159: The code uses a single counter `found` for both parse
errors and duplicate key occurrences, causing the final message to misreport
parse errors as duplicate key occurrences. Create a separate counter variable
for parse errors and keep `found` for duplicate keys only. In the catch block
where the parse error is handled, increment the parse error counter instead of
`found`. Update the final console.log message to report parse error count and
duplicate key count separately, and ensure the non-zero exit status is triggered
if either counter is non-zero.
In `@src/api/providers/__tests__/mimo.spec.ts`:
- Around line 693-695: Remove the double assertion (as unknown as
ApiHandlerCreateMessageMetadata) from the metadata object passed to
handler.createMessage. Instead, provide a properly typed metadata object that
includes the required taskId property along with the existing tools property.
This will ensure the test respects the ApiHandlerCreateMessageMetadata contract
rather than bypassing type safety.
In `@src/core/assistant-message/NativeToolCallParser.ts`:
- Around line 1191-1219: Add a readable message field to all tagged
structural-failure throws in NativeToolCallParser, including
invalid_argument_shape and missing_required_arguments cases, describing the tool
and failure. Update the catch block’s errorMessage construction to prefer the
tagged message before falling back to Error.message or String(error), so
parseErrors and console.error retain useful diagnostics.
In `@webview-ui/src/i18n/locales/ca/settings.json`:
- Around line 968-969: Translate the strictToolSchemas and
strictToolSchemasDescription values in
webview-ui/src/i18n/locales/ca/settings.json lines 968-969 into Catalan,
webview-ui/src/i18n/locales/pl/settings.json lines 968-969 into Polish,
webview-ui/src/i18n/locales/pt-BR/settings.json lines 968-969 into Brazilian
Portuguese, and webview-ui/src/i18n/locales/ru/settings.json lines 968-969 into
Russian, preserving the existing keys and meaning.
In `@webview-ui/src/i18n/locales/de/settings.json`:
- Around line 967-969: Translate both strictToolSchemas and
strictToolSchemasDescription in webview-ui/src/i18n/locales/de/settings.json
(lines 967-969) into German, webview-ui/src/i18n/locales/es/settings.json (lines
967-969) into Spanish, and webview-ui/src/i18n/locales/fr/settings.json (lines
967-969) into French, preserving the existing JSON keys and meaning.
In `@webview-ui/src/i18n/locales/hi/settings.json`:
- Around line 968-969: The new strictToolSchemas and
strictToolSchemasDescription keys contain English text but need to be translated
into the target languages for users. In
webview-ui/src/i18n/locales/hi/settings.json (lines 968-969), replace the
English values with Hindi translations. In
webview-ui/src/i18n/locales/id/settings.json (lines 968-969), add Indonesian
translations. In webview-ui/src/i18n/locales/it/settings.json (lines 968-969),
add Italian translations. In webview-ui/src/i18n/locales/ja/settings.json (lines
968-969), add Japanese translations. In
webview-ui/src/i18n/locales/ko/settings.json (lines 968-969), add Korean
translations. In webview-ui/src/i18n/locales/nl/settings.json (lines 968-969),
add Dutch translations. Each file should preserve the JSON structure with the
same keys but localized string values appropriate for its target language
audience.
In `@webview-ui/src/i18n/locales/tr/settings.json`:
- Around line 968-969: Translate the strictToolSchemas and
strictToolSchemasDescription values from English into the target languages in
webview-ui/src/i18n/locales/tr/settings.json lines 968-969,
webview-ui/src/i18n/locales/vi/settings.json lines 968-969, and
webview-ui/src/i18n/locales/zh-CN/settings.json lines 968-969. Preserve the
existing keys and meaning, including strict schema behavior, provider support
limitations, and the non-strict handling of MCP tools.
In `@webview-ui/src/i18n/locales/zh-TW/settings.json`:
- Around line 995-996: Translate the `strictToolSchemas` and
`strictToolSchemasDescription` entries in the zh-TW settings locale into
Traditional Chinese, preserving the original labels’ meaning and the
description’s strict-mode and MCP-tool behavior.
---
Nitpick comments:
In `@scripts/find-dup-json-keys.js`:
- Line 21: Remove the unused isArray field from the stack frame comment and
update the stack.push call to create frames with only the keys Set. Keep the
existing object-key tracking behavior unchanged.
- Around line 95-98: Update the object-key scanning logic around skipWs and
skipValue to validate that the current character is ':' before advancing. If it
is not, throw the same unexpected-char error used for other malformed input;
otherwise advance and continue skipping the value.
- Around line 6-15: Update walk to use directory-entry metadata via readdirSync
with withFileTypes enabled, avoid descending through symlinked entries, and skip
node_modules, .git, dist, and out directories before recursion. Preserve
yielding only .json files while preventing ancestor-link cycles and unnecessary
traversal.
In `@src/core/assistant-message/ToolCallRetentionPolicy.ts`:
- Around line 203-310: Create a single consolidated input type and emitter
function to replace the duplicate GhostDropTelemetryInput,
MaxOneEnforcementTelemetryInput, emitGhostDropTelemetry, and
emitMaxOneEnforcementTelemetry implementations. Define one input interface with
all the shared fields and one emitter function with the single hasInstance check
and captureToolCallEnforcement call, then re-export the original type names and
function names as type aliases and thin wrapper functions pointing to the
consolidated implementation to preserve existing call sites.
In `@src/core/task/Task.ts`:
- Around line 3024-3041: Extract the duplicated ghost-drop telemetry logic into
one private emitGhostDrop method in Task, resolving the policy and model once
within the helper and setting parallelToolCallsSent consistently with the
existing policy-resolution telemetry. Replace all three inline blocks using
ghostPolicy1, ghostPolicy2, and ghostPolicy3 with calls to this helper,
preserving the current telemetry values.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 90bdef99-32b2-43cd-9266-8de44a444825
📒 Files selected for processing (66)
docs/260730_0001_session_branch-cleanup/170000_debug-report.mddocs/260730_0001_session_branch-cleanup/173200_debug-report.mddocs/260730_0001_session_branch-cleanup/173230_execution-plan.mddocs/260730_0001_session_branch-cleanup/175300_code-report.mddocs/260730_0001_session_branch-cleanup/181500_debug-dnd-ux-runbook.mddocs/260730_0001_session_branch-cleanup/182225_code-report.mddocs/260730_0001_session_branch-cleanup/184700_debug-report.mddocs/260803_0002_session_6-branch-bug-fix-verification/173927_code-environment-feedback.mddocs/260803_0002_session_6-branch-bug-fix-verification/174043_code-causal-chain-environment-feedback.mddocs/260803_0002_session_6-branch-bug-fix-verification/174704_code-search-environment-feedback.mddocs/260803_0002_session_6-branch-bug-fix-verification/175017_code-vitest-environment-feedback.mddocs/260803_0002_session_6-branch-bug-fix-verification/175046_code-pnpm-environment-feedback.mddocs/260803_0002_session_6-branch-bug-fix-verification/175057_code-report.mdpackages/telemetry/src/TelemetryService.tspackages/types/src/__tests__/provider-settings.test.tspackages/types/src/model.tspackages/types/src/provider-settings.tspackages/types/src/providers/mimo.tspackages/types/src/telemetry.tsscripts/find-dup-json-keys.jssrc/api/index.tssrc/api/providers/__tests__/base-provider.spec.tssrc/api/providers/__tests__/mimo.spec.tssrc/api/providers/__tests__/openai.spec.tssrc/api/providers/base-openai-compatible-provider.tssrc/api/providers/base-provider.tssrc/api/providers/deepseek.tssrc/api/providers/friendli.tssrc/api/providers/kenari.tssrc/api/providers/lite-llm.tssrc/api/providers/lm-studio.tssrc/api/providers/mimo.tssrc/api/providers/openai-compatible.tssrc/api/providers/openai.tssrc/api/providers/opencode-go.tssrc/api/providers/openrouter.tssrc/core/assistant-message/NativeToolCallParser.tssrc/core/assistant-message/ToolCallRetentionPolicy.tssrc/core/assistant-message/__tests__/NativeToolCallParser.spec.tssrc/core/assistant-message/__tests__/ToolCallRetentionPolicy-telemetry.spec.tssrc/core/assistant-message/__tests__/ToolCallRetentionPolicy.spec.tssrc/core/prompts/tools/native-tools/execute_command.tssrc/core/task/Task.tssrc/core/task/__tests__/tool-call-policy.spec.tssrc/core/tools/ExecuteCommandTool.tssrc/eslint-suppressions.jsonsrc/shared/tools.tswebview-ui/src/components/settings/providers/OpenAICompatible.tsxwebview-ui/src/i18n/locales/ca/settings.jsonwebview-ui/src/i18n/locales/de/settings.jsonwebview-ui/src/i18n/locales/en/settings.jsonwebview-ui/src/i18n/locales/es/settings.jsonwebview-ui/src/i18n/locales/fr/settings.jsonwebview-ui/src/i18n/locales/hi/settings.jsonwebview-ui/src/i18n/locales/id/settings.jsonwebview-ui/src/i18n/locales/it/settings.jsonwebview-ui/src/i18n/locales/ja/settings.jsonwebview-ui/src/i18n/locales/ko/settings.jsonwebview-ui/src/i18n/locales/nl/settings.jsonwebview-ui/src/i18n/locales/pl/settings.jsonwebview-ui/src/i18n/locales/pt-BR/settings.jsonwebview-ui/src/i18n/locales/ru/settings.jsonwebview-ui/src/i18n/locales/tr/settings.jsonwebview-ui/src/i18n/locales/vi/settings.jsonwebview-ui/src/i18n/locales/zh-CN/settings.jsonwebview-ui/src/i18n/locales/zh-TW/settings.json
| ## Step 3 — Rebase the feature series onto main | ||
| ```powershell | ||
| git rebase --onto main d27153a25 feat/error-interception-middleware-clean | ||
| # (clean branch is at main; instead rebase the remote source series) | ||
| ``` | ||
| **Corrected command** (rebase the source series, landing on the clean branch name): | ||
| ```powershell | ||
| git checkout feat/error-interception-remote-src | ||
| git rebase --onto main d27153a25 feat/error-interception-remote-src | ||
| ``` |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Remove the obsolete rebase command.
Line 35 rebases feat/error-interception-middleware-clean, which was created at main and does not contain the remote feature series. It does not apply d27153a25..5c8c495e0. Keep only the corrected sequence that checks out feat/error-interception-remote-src and rebases that branch onto main.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/260730_0001_session_branch-cleanup/173230_execution-plan.md` around
lines 33 - 42, Remove the obsolete rebase command targeting
feat/error-interception-middleware-clean from Step 3, leaving only the corrected
sequence that checks out feat/error-interception-remote-src and rebases that
branch onto main.
| During `git cherry-pick` the conflicted file is the *new* commit applying onto remote HEAD, so: | ||
| ```powershell | ||
| git checkout --theirs src/core/webview/ClineProvider.ts # keep remote 0453c3a70 version | ||
| git add src/core/webview/ClineProvider.ts |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
Use --ours to preserve the clean squash file.
During git cherry-pick 78ba8218e, --theirs selects the incoming commit. It does not select myk1yt/feature/task-dnd-ux. The incoming ClineProvider.ts comes from contaminated history and can reintroduce unrelated TaskRegistry, shell, or stats changes. Use git checkout --ours src/core/webview/ClineProvider.ts, then verify that only the model and spec changes remain staged.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/260730_0001_session_branch-cleanup/181500_debug-dnd-ux-runbook.md`
around lines 248 - 251, Update the git cherry-pick instructions to use --ours
for src/core/webview/ClineProvider.ts, preserving the clean squash file rather
than the incoming contaminated commit. Instruct the reader to stage the file
afterward and verify that only the intended model and spec changes remain
staged.
| ## 7. Rollback | ||
| If verification fails before Step 6: | ||
| ```powershell | ||
| git cherry-pick --abort # if mid-cherry-pick | ||
| git checkout feat/error-interception-middleware # or any other working branch | ||
| git branch -D feature/task-dnd-ux-clean | ||
| # original feature/task-dnd-ux + contaminated-backup remain untouched |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Correct the rollback branch.
Line 323 checks out feat/error-interception-middleware, which is unrelated to this DND cleanup. If rollback is required, this command places the operator on the wrong feature branch. Use feature/task-dnd-ux or another explicitly documented recovery branch.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/260730_0001_session_branch-cleanup/181500_debug-dnd-ux-runbook.md`
around lines 319 - 325, Update the rollback command in the “7. Rollback” section
to check out feature/task-dnd-ux, or another explicitly documented recovery
branch, instead of feat/error-interception-middleware; keep the cleanup and
backup-preservation commands unchanged.
| | `src/__tests__/single-open-invariant.spec.ts` | deleted/modified on both sides (main's test suite changes vs stacked-branch deletion) | Not touched by §2.1 commits — no conflict expected in practice | | ||
| | `src/eslint-suppressions.json` | BOM churn on the contaminated branch vs main baseline | Avoided entirely by not picking the 4 cleanup commits | | ||
| | `webview-ui/playwright-ct.config.ts`, `zoo-hero-dark.png` | binary/config conflicts from stacked ancestors only | Not touched by §2.1 — no conflict expected | | ||
| | `615dfbacc` → `src/core/tools/error-interception/StructuralValidator.ts` | file absent on cleaned branch | Cherry-pick will conflict (modify/delete). **Resolution: skip this hunk** (`git restore --source=HEAD -- src/core/tools/error-interception` or just don't stage that path); the StructuralValidator normalization hunk belongs to the error-interception PR, not this one | |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Resolve only the conflicted file.
The documented conflict is limited to src/core/tools/error-interception/StructuralValidator.ts. Restoring the whole directory may fail when the directory is absent from upstream/main, and leaving the path unstaged leaves the cherry-pick unresolved. The git rm -r command is also broader than necessary. Remove only src/core/tools/error-interception/StructuralValidator.ts, then verify the unmerged-path list before continuing.
Also applies to: 114-116
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/260730_0001_session_branch-cleanup/184700_debug-report.md` at line 90,
Update the documented conflict resolution for StructuralValidator.ts to remove
only src/core/tools/error-interception/StructuralValidator.ts, avoiding
whole-directory restore or broad git rm commands, then verify the unmerged-path
list is clear before continuing the cherry-pick.
| include_usage: true, | ||
| }, | ||
| tools: this.convertToolsForOpenAI(metadata?.tools), | ||
| tools: this.convertToolsForOpenAI(metadata?.tools, this.options.openAiToolStrictMode ?? false), |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
For AI SDK version 6.0.218, what streamTextrequest option orproviderOptionsentry forwardsparallel_tool_callswhen usingcreateOpenAICompatible?
💡 Result:
When using the createOpenAICompatible provider in AI SDK version 6.0.218, the parallel_tool_calls option is passed via the providerOptions field using the provider's name as the key [1][2]. If you configured your provider with a name (e.g., name: 'my-provider'), you should set the option under providerOptions.my-provider [1]. Because the OpenAI Compatible provider forwards these options directly to the underlying API request body, you can include parallel_tool_calls as a property within that namespaced object [1]: const { text } = await streamText({ model: myProvider('model-id'), providerOptions: { 'my-provider': { parallel_tool_calls: false, // or true }, }, }); Note that the key used within providerOptions must match the name defined in your createOpenAICompatible configuration [1]. If you encounter issues where the option is not being forwarded, ensure that the provider name used in the configuration matches the key in your providerOptions exactly [1]. While the official @ai-sdk/openai provider uses a specific TypeScript interface (OpenAILanguageModelResponsesOptions) to type these options, the @ai-sdk/openai-compatible provider treats these entries as generic configuration to be passed to the API [1][3].
Citations:
- 1: https://ai-sdk.dev/v7/providers/openai-compatible-providers
- 2: https://ai-sdk.dev/docs/foundations/provider-options
- 3: https://ai-sdk.dev/providers/ai-sdk-providers/openai
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- files ---\n'
git ls-files | rg '(^|/)lite-llm\.ts$|(^|/)openai-compatible\.ts$|resolveToolCallPolicy|parallelToolCalls|parallel_tool_calls' || true
printf '\n--- lite-llm relevant section ---\n'
FILE=$(git ls-files | rg '(^|/)lite-llm\.ts$' | head -n1 || true)
if [ -n "$FILE" ]; then
wc -l "$FILE"
rg -n "parallelToolCalls|parallel_tool_calls|convertToolsForOpenAI|requestOptions|streamText|options" "$FILE" -C 3
fi
printf '\n--- openai compatible relevant section ---\n'
FILE2=$(git ls-files | rg '(^|/)openai-compatible\.ts$' | head -n1 || true)
if [ -n "$FILE2" ]; then
wc -l "$FILE2"
rg -n "parallelToolCalls|parallel_tool_calls|convertToolsForOpenAI|streamText|providerOptions|options" "$FILE2" -C 4
fi
printf '\n--- policy definition/usages ---\n'
rg -n "resolveToolCallPolicy|parallelToolCalls|parallel_tool_calls" . --glob '!node_modules' --glob '!dist' --glob '!build' -C 2 || trueRepository: Zoo-Code-Org/Zoo-Code
Length of output: 250
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- files ---'
git ls-files | rg '(^|/)lite-llm\.ts$|(^|/)openai-compatible\.ts$|resolveToolCallPolicy|parallelToolCalls|parallel_tool_calls' || true
printf '%s\n'
printf '%s\n' '--- lite-llm relevant section ---'
FILE=$(git ls-files | rg '(^|/)lite-llm\.ts$' | head -n1 || true)
if [ -n "$FILE" ]; then
wc -l "$FILE"
rg -n "parallelToolCalls|parallel_tool_calls|convertToolsForOpenAI|requestOptions|streamText|options" "$FILE" -C 3
fi
printf '%s\n'
printf '%s\n' '--- openai compatible relevant section ---'
FILE2=$(git ls-files | rg '(^|/)openai-compatible\.ts$' | head -n1 || true)
if [ -n "$FILE2" ]; then
wc -l "$FILE2"
rg -n "parallelToolCalls|parallel_tool_calls|convertToolsForOpenAI|streamText|providerOptions|options" "$FILE2" -C 4
fi
printf '%s\n'
printf '%s\n' '--- policy definition/usages ---'
rg -n "resolveToolCallPolicy|parallelToolCalls|parallel_tool_calls" . --glob '!node_modules' --glob '!dist' --glob '!build' -C 2 || trueRepository: Zoo-Code-Org/Zoo-Code
Length of output: 417
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- lite-llm.ts outline ---'
ast-grep outline src/api/providers/lite-llm.ts --view compact || true
printf '%s\n'
printf '%s\n' '--- lite-llm.ts relevant lines 180-245 ---'
sed -n '180,245p' src/api/providers/lite-llm.ts | nl -ba -v180
printf '%s\n'
if command -v wc >/dev/null 2>&1; then
wc -l src/api/providers/openai-compatible.ts
fi
ast-grep outline src/api/providers/openai-compatible.ts --view compact || true
printf '%s\n'
printf '%s\n' '--- openai-compatible.ts relevant lines 120-185 ---'
sed -n '120,185p' src/api/providers/openai-compatible.ts | nl -ba -v120
printf '%s\n'
printf '%s\n' '--- policy definitions/usages ---'
rg -n "resolveToolCallPolicy|parallelToolCalls|parallel_tool_calls" . --glob '!node_modules' --glob '!dist' --glob '!build' -C 2 || true
printf '%s\n'
printf '%s\n' '--- AI SDK openai compatible imports/options in target files ---'
rg -n "createOpenAICompatible|openai-compatible|streamText|providerOptions|requestOptions" src/api/providers/lite-llm.ts src/api/providers/openai-compatible.ts -C 3Repository: Zoo-Code-Org/Zoo-Code
Length of output: 422
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- lite-llm.ts relevant lines 180-245 ---'
sed -n '180,245p' src/api/providers/lite-llm.ts | awk '{printf "%6d\t%s\n", NR+179, $0}'
printf '%s\n'
printf '%s\n' '--- openai-compatible.ts file length ---'
wc -l src/api/providers/openai-compatible.ts
printf '%s\n'
printf '%s\n' '--- openai-compatible.ts relevant lines 120-185 ---'
sed -n '120,185p' src/api/providers/openai-compatible.ts | awk '{printf "%6d\t%s\n", NR+119, $0}'
printf '%s\n'
printf '%s\n' '--- policy definitions/usages ---'
rg -n "resolveToolCallPolicy|parallelToolCalls|parallel_tool_calls" . --glob '!node_modules' --glob '!dist' --glob '!build' -C 2 || true
printf '%s\n'
printf '%s\n' '--- AI SDK provider options in target files ---'
rg -n "createOpenAICompatible|openai-compatible|streamText|providerOptions|requestOptions" src/api/providers/lite-llm.ts src/api/providers/openai-compatible.ts -C 3 || true
printf '%s\n'
printf '%s\n' '--- lock/package references for AI SDK versions ---'
rg -n '"`@ai-sdk/openai-compatible`"|`@ai-sdk/openai-compatible`|ai-sdk.v7|ai-sdk|version.*6.0|6\.0\.218' package.json pnpm-lock.yaml package-lock.json yarn.lock 2>/dev/null || trueRepository: Zoo-Code-Org/Zoo-Code
Length of output: 50377
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- openai-compatible.ts relevant creation section ---'
sed -n '1,130p' src/api/providers/openai-compatible.ts | awk '{printf "%6d\t%s\n", NR, $0}'
printf '%s\n'
sed -n '153,181p' src/api/providers/openai-compatible.ts | awk '{printf "%6d\t%s\n", NR+152, $0}'
printf '%s\n'
printf '%s\n' '--- LiteLLM tests mentioning parallel_tool_calls or policy ---'
rg -n "parallel_tool_calls|parallelToolCalls|parallelToolCalls" src/api/providers/__tests__/lite-llm.spec.ts || true
printf '%s\n'
printf '%s\n' '--- OpenAI-compatible handler tests mentioning parallel_tool_calls or policy ---'
rg -n "parallel_tool_calls|parallelToolCalls" src/api/providers/__tests__/openai-compatible.spec.ts || true
printf '%s\n'
printf '%s\n' '--- package lock refs for AI SDK compatible package ---'
rg -n '"`@ai-sdk/openai-compatible`"|`@ai-sdk/openai-compatible`|ai-sdk.v7|ai-sdk|6\.0\.218' package.json pnpm-lock.yaml package-lock.json 2>/dev/null | head -n 80 || trueRepository: Zoo-Code-Org/Zoo-Code
Length of output: 9362
Forward metadata.parallelToolCalls on both request paths.
metadata.parallelToolCalls is available from resolveToolCallPolicy, but both LiteLLM and OpenAI-compatible providers omit it. This lets the upstream request use parallel tool calls even when the chosen policy is single-call.
src/api/providers/lite-llm.ts#L222-L222: Addparallel_tool_calls: metadata?.parallelToolCalls ?? truetorequestOptions.src/api/providers/openai-compatible.ts#L165-L165: Add a provider-namespaced option understreamText({ model: languageModel, providerOptions: { [config.providerName]: { parallel_tool_calls: metadata?.parallelToolCalls ?? true } } }), or mark this route as local-only enforcement.- Add request-capture tests covering
parallel_tool_calls: trueandparallel_tool_calls: false.
📍 Affects 2 files
src/api/providers/lite-llm.ts#L222-L222(this comment)src/api/providers/openai-compatible.ts#L165-L165
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/api/providers/lite-llm.ts` at line 222, The LiteLLM and OpenAI-compatible
request paths must forward the resolved parallel tool-call policy instead of
defaulting upstream behavior. In src/api/providers/lite-llm.ts:222, add
parallel_tool_calls to requestOptions using metadata?.parallelToolCalls ?? true;
in src/api/providers/openai-compatible.ts:165, pass the same value through the
provider-namespaced providerOptions for streamText (or explicitly mark that
route as local-only enforcement). Add request-capture tests covering both true
and false values.
| export function selectExecutableCall(input: SelectExecutableCallInput): SelectExecutableCallResult { | ||
| const { calls, maxCallsPerTurn } = input | ||
|
|
||
| if (maxCallsPerTurn === "unbounded") { | ||
| // Parallel-capable providers: no local enforcement needed. | ||
| const firstValid = calls.find((c) => c.hasNativeArgs && !c.isPartial) | ||
| return { | ||
| executableCallId: firstValid?.callId, | ||
| rejectedCallIds: [], | ||
| reason: "unbounded-policy", | ||
| } | ||
| } | ||
|
|
||
| // Single-call policy: collect all structurally valid, non-partial calls. | ||
| const validCandidates = calls.filter((c) => c.hasNativeArgs && !c.isPartial) | ||
|
|
||
| if (validCandidates.length === 0) { | ||
| return { | ||
| executableCallId: undefined, | ||
| rejectedCallIds: [], | ||
| reason: "no-valid-candidates", | ||
| } | ||
| } | ||
|
|
||
| if (validCandidates.length === 1) { | ||
| return { | ||
| executableCallId: validCandidates[0].callId, | ||
| rejectedCallIds: [], | ||
| reason: "single-valid-candidate", | ||
| } | ||
| } | ||
|
|
||
| // Two or more valid candidates under single-call policy: | ||
| // execute NEITHER automatically. All receive error results. | ||
| return { | ||
| executableCallId: undefined, | ||
| rejectedCallIds: validCandidates.map((c) => c.callId), | ||
| reason: "multiple-valid-calls-under-single-policy", | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find production callers of selectExecutableCall and emitMaxOneEnforcementTelemetry.
set -euo pipefail
rg -nP --type=ts -C4 '\b(selectExecutableCall|emitMaxOneEnforcementTelemetry)\s*\(' -g '!**/__tests__/**'Repository: Zoo-Code-Org/Zoo-Code
Length of output: 159
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate files =="
fd -a 'ToolCallRetentionPolicy\.ts|Task\.ts' . | sed 's#^\./##'
echo
echo "== all references to symbols =="
rg -n 'selectExecutableCall\b|emitMaxOneEnforcementTelemetry\b|maxCallsPerTurn|multiple-valid-calls-under-single-policy|single-valid-candidate|unbounded-policy|no-valid-candidates' . --glob '!**/node_modules/**' --glob '!**/dist/**' --glob '!**/build/**' || true
echo
echo "== ToolCall retention policy outline =="
if [ -f src/core/assistant-message/ToolCallRetentionPolicy.ts ]; then
ast-grep outline src/core/assistant-message/ToolCallRetentionPolicy.ts --view compact || true
echo
wc -l src/core/assistant-message/ToolCallRetentionPolicy.ts
sed -n '1,260p' src/core/assistant-message/ToolCallRetentionPolicy.ts
fiRepository: Zoo-Code-Org/Zoo-Code
Length of output: 19328
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate files =="
fd -a 'ToolCallRetentionPolicy\.ts|Task\.ts' . | sed 's#^\./##'
echo
echo "== all references to symbols =="
rg -n 'selectExecutableCall\b|emitMaxOneEnforcementTelemetry\b|maxCallsPerTurn|multiple-valid-calls-under-single-policy|single-valid-candidate|unbounded-policy|no-valid-candidates' . --glob '!**/node_modules/**' --glob '!**/dist/**' --glob '!**/build/**' || true
echo
echo "== ToolCall retention policy outline =="
if [ -f src/core/assistant-message/ToolCallRetentionPolicy.ts ]; then
ast-grep outline src/core/assistant-message/ToolCallRetentionPolicy.ts --view compact || true
echo
wc -л src/core/assistant-message/ToolCallRetentionPolicy.ts
sed -n '1,260p' src/core/assistant-message/ToolCallRetentionPolicy.ts
fiRepository: Zoo-Code-Org/Zoo-Code
Length of output: 10030
Wire selectExecutableCall into the execution path.
selectExecutableCall is only covered by unit tests; no non-test production site calls it. Under maxCallsPerTurn === 1, add the selection step before executing tool calls so two valid side-effecting calls do not both run automatically, and route rejectedCallIds to error results to trigger enforcement telemetry.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/core/assistant-message/ToolCallRetentionPolicy.ts` around lines 159 -
198, Integrate selectExecutableCall into the production tool-call execution path
before any calls execute, using the current turn’s calls and maxCallsPerTurn
policy. Execute only executableCallId, prevent all rejectedCallIds from running,
and convert each rejected ID into an error result so enforcement telemetry is
triggered; preserve unbounded and single-valid-call behavior.
| "strictToolSchemas": "Strict tool schemas", | ||
| "strictToolSchemasDescription": "Enables strict mode for function tool schemas, ensuring tool outputs match the schema exactly. Some providers may not support strict mode. MCP tools are always kept non-strict regardless of this setting.", | ||
| "gemini": { | ||
| "freeRequests": "* Free up to {{count}} requests per minute. After that, billing depends on prompt size.", | ||
| "pricingDetails": "For more info, see pricing details.", | ||
| "billingEstimate": "* Billing is an estimate - exact cost depends on prompt size." | ||
| } | ||
| }, | ||
| "strictToolSchemas": "Strict tool schemas", | ||
| "strictToolSchemasDescription": "Enables strict mode for function tool schemas, ensuring tool outputs match the schema exactly. Some providers may not support strict mode. MCP tools are always kept non-strict regardless of this setting." |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Remove one duplicate key pair.
modelInfo declares strictToolSchemas and strictToolSchemasDescription twice. Biome reports noDuplicateObjectKeys. Retain one pair only.
🧰 Tools
🪛 Biome (2.5.5)
[error] 1043-1043: The key strictToolSchemas was already declared.
(lint/suspicious/noDuplicateObjectKeys)
[error] 1044-1044: The key strictToolSchemasDescription was already declared.
(lint/suspicious/noDuplicateObjectKeys)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@webview-ui/src/i18n/locales/en/settings.json` around lines 1043 - 1051,
Remove the duplicate strictToolSchemas and strictToolSchemasDescription entries
from the modelInfo locale object, retaining exactly one pair with the existing
translations.
Source: Linters/SAST tools
github-actions
Bot
added
the
awaiting-review
132fb08 to
ccf2331
Compare
github-actions
Bot
added
has-conflicts
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/core/task/Task.ts (1)
3024-3041: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract one ghost-drop helper instead of three copies.
The ghost-drop policy resolution and telemetry payload are duplicated at Lines 3024-3041, Lines 3115-3132, and Lines 3516-3533. Each copy re-resolves the policy into a numbered local (
ghostPolicy1,ghostPolicy2,ghostPolicy3) and callsthis.api.getModel()twice. The block-removal and re-index logic at Lines 3002-3021 is also duplicated at Lines 3503-3513. One private method keeps the three sites in step and removes the numbered names.♻️ Proposed helper
private emitGhostDrop(): void { const policy = resolveToolCallPolicy(this.api.getModel().info, this.apiConfiguration.apiProvider) emitGhostDropTelemetry({ taskId: this.taskId, provider: this.apiConfiguration.apiProvider ?? "unknown", model: this.api.getModel().id, policySource: policy.source, maxCallsPerTurn: policy.maxCallsPerTurn, enforcement: policy.enforcement, callCount: this.assistantMessageContent.filter( (b: AssistantMessageContent): b is ToolUse => b.type === "tool_use", ).length, ghostDroppedCount: 1, errorResultCount: 0, parallelToolCallsRequested: policy.generation === "parallel", }) } private removeGhostBlock(callId: string): void { const ghostIndex = this.streamingToolCallIndices.get(callId) if (ghostIndex === undefined) { return } this.assistantMessageContent.splice(ghostIndex, 1) for (const [cid, idx] of this.streamingToolCallIndices.entries()) { if (idx > ghostIndex) { this.streamingToolCallIndices.set(cid, idx - 1) } } this.streamingToolCallIndices.delete(callId) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/task/Task.ts` around lines 3024 - 3041, In the Task class, extract the repeated ghost-drop telemetry into a private emitGhostDrop method using one resolved policy and model reference, then replace the three duplicated telemetry blocks with calls to it. Also extract the repeated block removal and streamingToolCallIndices re-indexing into a private removeGhostBlock(callId) method, and update both removal sites to use it while preserving their existing behavior.src/api/providers/__tests__/mimo.spec.ts (1)
127-133: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExplain each double assertion with a comment.
These casts build a
reasoningcontent block that the AnthropicMessageParamunion does not contain. The cast is a reasonable last resort, but the coding guidelines require a comment that states the reason. The same applies at Line 215, Line 228, Line 346-357, and Line 693-695, wheretaskIdis omitted fromApiHandlerCreateMessageMetadata.📝 Proposed comment for this site
+ // Double assertion: `reasoning` is a provider-specific block + // that the Anthropic MessageParam union does not model, and + // convertToR1Format must still handle it. { type: "reasoning" as const, text: "Let me think...", } as unknown as Anthropic.Messages.MessageParam["content"][number],As per coding guidelines: "Use double assertions only as a last resort and explain them with a comment."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api/providers/__tests__/mimo.spec.ts` around lines 127 - 133, Explain every double assertion in mimo.spec.ts with an adjacent comment stating why it is necessary: document that the reasoning content block is intentionally used despite not existing in Anthropic’s MessageParam union, and document the omitted taskId casts on the referenced metadata objects. Apply this consistently at the shown assertion sites without changing the test behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@codecov.yml`:
- Line 1: Convert the line endings in codecov.yml from CRLF to LF throughout the
file, without changing its YAML content.
In `@src/api/providers/openai.ts`:
- Around line 375-377: Update the O3-family request construction in the relevant
non-streaming and streaming paths around the tool conversion and
parallel_tool_calls assignments. Only include parallel_tool_calls when
metadata.tools is supplied and resolves to tools; omit the field when tools is
undefined, matching the existing guard used in the other provider paths and
preserving the configured boolean when tools are present.
In `@src/eslint-suppressions.json`:
- Around line 237-255: The no-explicit-any suppression counts must not increase
in src/eslint-suppressions.json. In opencode-go.spec.ts and
qwen-code-native-tools.spec.ts, replace the newly introduced any usages with
appropriate types or unknown plus type guards, then restore both entries to
their previous suppression counts.
---
Nitpick comments:
In `@src/api/providers/__tests__/mimo.spec.ts`:
- Around line 127-133: Explain every double assertion in mimo.spec.ts with an
adjacent comment stating why it is necessary: document that the reasoning
content block is intentionally used despite not existing in Anthropic’s
MessageParam union, and document the omitted taskId casts on the referenced
metadata objects. Apply this consistently at the shown assertion sites without
changing the test behavior.
In `@src/core/task/Task.ts`:
- Around line 3024-3041: In the Task class, extract the repeated ghost-drop
telemetry into a private emitGhostDrop method using one resolved policy and
model reference, then replace the three duplicated telemetry blocks with calls
to it. Also extract the repeated block removal and streamingToolCallIndices
re-indexing into a private removeGhostBlock(callId) method, and update both
removal sites to use it while preserving their existing behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c65700bd-2679-4183-a842-ed38158ddb9d
📒 Files selected for processing (45)
codecov.ymlpackages/telemetry/src/TelemetryService.tspackages/types/src/__tests__/provider-settings.test.tspackages/types/src/model.tspackages/types/src/provider-settings.tspackages/types/src/providers/mimo.tspackages/types/src/telemetry.tssrc/api/index.tssrc/api/providers/__tests__/base-provider.spec.tssrc/api/providers/__tests__/mimo.spec.tssrc/api/providers/__tests__/openai.spec.tssrc/api/providers/base-openai-compatible-provider.tssrc/api/providers/base-provider.tssrc/api/providers/mimo.tssrc/api/providers/openai.tssrc/core/assistant-message/NativeToolCallParser.tssrc/core/assistant-message/ToolCallRetentionPolicy.tssrc/core/assistant-message/__tests__/NativeToolCallParser.spec.tssrc/core/assistant-message/__tests__/ToolCallRetentionPolicy-telemetry.spec.tssrc/core/assistant-message/__tests__/ToolCallRetentionPolicy.spec.tssrc/core/prompts/tools/native-tools/execute_command.tssrc/core/task/Task.tssrc/core/task/__tests__/tool-call-policy.spec.tssrc/core/tools/ExecuteCommandTool.tssrc/eslint-suppressions.jsonsrc/shared/tools.tswebview-ui/src/components/settings/providers/OpenAICompatible.tsxwebview-ui/src/i18n/locales/ca/settings.jsonwebview-ui/src/i18n/locales/de/settings.jsonwebview-ui/src/i18n/locales/en/settings.jsonwebview-ui/src/i18n/locales/es/settings.jsonwebview-ui/src/i18n/locales/fr/settings.jsonwebview-ui/src/i18n/locales/hi/settings.jsonwebview-ui/src/i18n/locales/id/settings.jsonwebview-ui/src/i18n/locales/it/settings.jsonwebview-ui/src/i18n/locales/ja/settings.jsonwebview-ui/src/i18n/locales/ko/settings.jsonwebview-ui/src/i18n/locales/nl/settings.jsonwebview-ui/src/i18n/locales/pl/settings.jsonwebview-ui/src/i18n/locales/pt-BR/settings.jsonwebview-ui/src/i18n/locales/ru/settings.jsonwebview-ui/src/i18n/locales/tr/settings.jsonwebview-ui/src/i18n/locales/vi/settings.jsonwebview-ui/src/i18n/locales/zh-CN/settings.jsonwebview-ui/src/i18n/locales/zh-TW/settings.json
🚧 Files skipped from review as they are similar to previous changes (36)
- src/shared/tools.ts
- webview-ui/src/i18n/locales/id/settings.json
- webview-ui/src/i18n/locales/zh-TW/settings.json
- webview-ui/src/i18n/locales/zh-CN/settings.json
- webview-ui/src/i18n/locales/fr/settings.json
- packages/types/src/providers/mimo.ts
- webview-ui/src/i18n/locales/tr/settings.json
- webview-ui/src/i18n/locales/ca/settings.json
- webview-ui/src/i18n/locales/ja/settings.json
- src/core/tools/ExecuteCommandTool.ts
- webview-ui/src/i18n/locales/de/settings.json
- webview-ui/src/i18n/locales/es/settings.json
- webview-ui/src/i18n/locales/nl/settings.json
- webview-ui/src/i18n/locales/vi/settings.json
- src/core/assistant-message/tests/ToolCallRetentionPolicy.spec.ts
- src/core/assistant-message/tests/NativeToolCallParser.spec.ts
- packages/types/src/model.ts
- packages/types/src/provider-settings.ts
- packages/telemetry/src/TelemetryService.ts
- packages/types/src/tests/provider-settings.test.ts
- src/core/assistant-message/tests/ToolCallRetentionPolicy-telemetry.spec.ts
- packages/types/src/telemetry.ts
- webview-ui/src/i18n/locales/it/settings.json
- webview-ui/src/i18n/locales/hi/settings.json
- src/api/providers/tests/base-provider.spec.ts
- webview-ui/src/i18n/locales/ru/settings.json
- src/core/prompts/tools/native-tools/execute_command.ts
- src/api/index.ts
- webview-ui/src/i18n/locales/pt-BR/settings.json
- webview-ui/src/components/settings/providers/OpenAICompatible.tsx
- webview-ui/src/i18n/locales/ko/settings.json
- src/core/task/tests/tool-call-policy.spec.ts
- webview-ui/src/i18n/locales/pl/settings.json
- src/api/providers/mimo.ts
- src/core/assistant-message/NativeToolCallParser.ts
- src/core/assistant-message/ToolCallRetentionPolicy.ts
| comment: | ||
| layout: "diff, flags, components" | ||
| behavior: default | ||
| coverage: |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use LF line endings.
YAMLlint reports wrong new line character: expected \n at Line 1. Convert codecov.yml from CRLF to LF so the lint check passes.
🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 1-1: wrong new line character: expected \n
(new-lines)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@codecov.yml` at line 1, Convert the line endings in codecov.yml from CRLF to
LF throughout the file, without changing its YAML content.
Source: Linters/SAST tools
| "api/providers/__tests__/opencode-go.spec.ts": { | ||
| "@typescript-eslint/no-explicit-any": { | ||
| "count": 11 | ||
| } | ||
| }, | ||
| "api/providers/__tests__/openrouter.spec.ts": { | ||
| "@typescript-eslint/no-explicit-any": { | ||
| "count": 34 | ||
| } | ||
| }, | ||
| "api/providers/__tests__/poe.spec.ts": { | ||
| "@typescript-eslint/no-explicit-any": { | ||
| "count": 1 | ||
| } | ||
| }, | ||
| "api/providers/__tests__/qwen-code-native-tools.spec.ts": { | ||
| "@typescript-eslint/no-explicit-any": { | ||
| "count": 5 | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Git status and diff stat:"
git status --short
git diff --stat src/eslint-suppressions.json || true
echo
echo "Relevant suppression entries (old vs new):"
git diff --no-color src/eslint-suppressions.json | sed -n '200,280p' || true
echo
echo "File sizes and target lines:"
wc -l src/eslint-suppressions.json
sed -n '220,265p' src/eslint-suppressions.json | nl -ba -v220
echo
echo "Search for ESLint suppression manifest usage:"
rg -n '"src/eslint-suppressions.json"|eslint-suppressions|eslint: ' -S .Repository: Zoo-Code-Org/Zoo-Code
Length of output: 334
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import json, pathlib
p=pathlib.Path('src/esprit-suppressions.json')
PYRepository: Zoo-Code-Org/Zoo-Code
Length of output: 159
Keep the no-explicit-any suppression counts from increasing.
src/eslint-suppressions.json suppression counts must not increase. The entries for opencode-go.spec.ts and qwen-code-native-tools.spec.ts raise baseline @typescript-eslint/no-explicit-any counts, so remove the new any usages with typed replacements or unknown plus type guards, then restore the previous suppression counts.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/eslint-suppressions.json` around lines 237 - 255, The no-explicit-any
suppression counts must not increase in src/eslint-suppressions.json. In
opencode-go.spec.ts and qwen-code-native-tools.spec.ts, replace the newly
introduced any usages with appropriate types or unknown plus type guards, then
restore both entries to their previous suppression counts.
Source: Coding guidelines
github-actions
Bot
removed
the
has-conflicts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/vscode-e2e/src/fixtures/subtasks.ts`:
- Around line 574-579: Restrict the fixture registered in mock.addFixture to
match only the initial abandon-parent turn, preventing subsequent parent
requests without SUBTASK_RESULT_INJECTION from serving new_task again. Update
the predicate using an explicit turn-count or tool-call exclusion, or ensure the
fixture is removed after its first response while preserving the existing marker
and injection checks.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5cbf511f-c71e-40a9-8ddc-e100b27bbef1
📒 Files selected for processing (2)
apps/vscode-e2e/src/fixtures/subtasks.tssrc/eslint-suppressions.json
| mock.addFixture({ | ||
| match: { | ||
| userMessage: new RegExp(SUBTASK_ABANDON_PARENT_MARKER), | ||
| sequenceIndex: 0, | ||
| predicate: (req: ChatCompletionRequest) => | ||
| lastUserMessageContains(req, SUBTASK_ABANDON_PARENT_MARKER) && | ||
| !requestContains(req, [SUBTASK_RESULT_INJECTION]), | ||
| }, |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the abandon markers and prompts.
rg -n 'SUBTASK_ABANDON_' apps/vscode-e2e/src --glob '*.ts'
# Inspect the abandon e2e test to see how many parent turns it expects.
fd -i 'subtask' apps/vscode-e2e/src/suite --exec rg -n -C 5 'abandon' {}Repository: Zoo-Code-Org/Zoo-Code
Length of output: 7685
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== fixtures outline =="
wc -l apps/vscode-e2e/src/fixtures/subtasks.ts apps/vscode-e2e/src/suite/subtasks.test.ts
echo "== relevant fixture sections =="
sed -n '40,70p;540,645p' apps/vscode-e2e/src/fixtures/subtasks.ts
echo "== relevant test section =="
sed -n '844,1010p' apps/vscode-e2e/src/suite/subtasks.test.ts
echo "== addFixture signature/usages in fixture util =="
rg -n "addFixture|use|remove|sequenceIndex|predicate|ChatCompletionRequest|SubtaskFixture" apps/vscode-e2e/src -g '*.ts' | sed -n '1,220p'Repository: Zoo-Code-Org/Zoo-Code
Length of output: 37517
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== package/config snippets =="
for f in package.json apps/vscode-e2e/package.json apps/vscode-e2e/vitest.config.* apps/vscode-e2e/vite.config.*; do
if [ -f "$f" ]; then
echo "--- $f"
sed -n '1,140p' "$f"
fi
done
echo "== dependency versions mentioning aimock/copilotkit =="
rg -n '"`@copilotkit/aimock`"|`@copilotkit/aimock`|LLMock|addFixture|FixtureMatcher|fixture' package.json apps/vscode-e2e/package.json pnpm-lock.yaml --glob '*lock.yaml' --glob '!**/node_modules/**' | sed -n '1,220p'
echo "== deterministic text check for parent action flow =="
python3 - <<'PY'
from pathlib import Path
p=Path('apps/vscode-e2e/src/suite/subtasks.test.ts')
text=p.read_text()
start=text.index('test("abandoning an interrupted subtask severs the parent-child link"')
idx=text.index('})\n\t})', start)+5
block=text[start:idx]
for stmt in ['await api.abandonSubtask(childTaskId!)', 'await api.resumeTask(childTaskId!)', 'api.sendMessage(SUBTASK_ABANDON_CHILD_FOLLOWUP_ANSWER)']:
lines=[i+1 for i,l in enumerate(text[:text.index(block)+start].splitlines() if stmt in l)]
print(f"{stmt!r}: {len(lines)} occurrences in test file, first at {lines[0]}")
print('contains assert parent not completed:', 'Parent must never complete/reopen after its abandoned child resumes and completes' in block)
print('contains current stack empty finally:', 'while (api.getCurrentTaskStack().length > 0)' in text)
PYRepository: Zoo-Code-Org/Zoo-Code
Length of output: 4344
🌐 Web query:
@copilotkit/aimock 1.35.0 addFixture removeFixture fixture match sequenceIndex
💡 Result:
In @copilotkit/aimock (version 1.35.0), these terms refer to the mechanisms for controlling mock responses during AI application testing [1][2]. Fixtures are JSON objects used to simulate LLM or service responses [3][4]. You can manage them either by loading them from directories/files via the CLI or configuration [5][4], or by programmatically adding and removing them using the LLMock API [6]. Key concepts: 1. fixture: A recorded or manually defined exchange that simulates an API interaction [3][7]. 2. addFixture / removeFixture: These methods (or their equivalents like onMessage) are used to programmatically register or clear mock responses during test execution [3][6]. For instance, using the test framework plugins, you can register custom fixtures within test suites [6]. 3. match: Refers to the criteria used to route an incoming request to the correct fixture [2]. Matching can be based on properties like the user message, tool call IDs, or system messages [7][2]. 4. sequenceIndex: An optional field added to fixture definitions to differentiate between otherwise identical requests [7]. In scenarios where a test involves "byte-identical repeats" (such as loops or retries that send the same prompt multiple times), adding a sequenceIndex allows the router to distinguish and serve the intended fixture in the correct order [7]. For detailed usage, the official documentation covers the Record & Replay workflow, which automatically handles disambiguators like turnIndex and toolCallId, recommending sequenceIndex specifically for genuine repeats that lack other distinguishing features [7].
Citations:
- 1: https://github.com/CopilotKit/aimock
- 2: https://www.npmjs.com/package/@copilotkit/aimock
- 3: https://aimock.copilotkit.dev/docs/
- 4: https://aimock.copilotkit.dev/aimock-cli/index.html
- 5: https://aimock.copilotkit.dev/
- 6: http://aimock.copilotkit.dev/test-plugins/
- 7: https://aimock.copilotkit.dev/record-replay/index.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== lockfile entry for `@copilotkit/aimock` =="
sed -n '1330,1355p;9190,9208p' pnpm-lock.yaml
echo "== imports/types for LLMock in vscode e2e =="
rg -n "LLMock|createFixture|addFixture|removeFixture|Fixture" apps/vscode-e2e/src --glob '*.ts' | sed -n '1,220p'
echo "== deterministic flow check =="
python3 - <<'PY'
from pathlib import Path
p=Path('apps/vscode-e2e/src/suite/subtasks.test.ts')
text=p.read_text()
start=text.index('test("abandoning an interrupted subtask severs the parent-child link"')
end=text.index('\n\t})\n\t})', text.index('} catch (', start))
block=text[start:end]
events = [
('await api.abandonSubtask(childTaskId!)', block.count('await api.abandonSubtask(childTaskId!)')),
('await api.resumeTask(childTaskId!)', block.count('await api.resumeTask(childTaskId!)')),
('api.sendMessage(SUBTASK_ABANDON_CHILD_FOLLOWUP_ANSWER)', block.count('api.sendMessage(SUBTASK_ABANDON_CHILD_FOLLOWUP_ANSWER)')),
]
for ev, count in events:
print(f'{ev!r}: {count}')
print('contains parent completion assertion:', 'Parent must never complete/reopen after its abandoned child resumes and completes' in block)
print('contains parent awaitingChildId assertion after child completion:', 'Parent must not start awaiting the abandoned child again' in block)
PYRepository: Zoo-Code-Org/Zoo-Code
Length of output: 16538
🌐 Web query:
aimock 1.35.0 match predicate fixture "fixture" "addFixture" removeFixture docs
💡 Result:
AIMock (v1.35.0) is a deterministic mock server suite designed for testing AI agentic stacks [1][2]. It serves as a unified replacement for the older @copilotkit/llmock package [1][2]. Match Predicates and Fixtures AIMock uses a fixture-based routing system to provide deterministic responses for LLM and agent interactions [3]. When a request is received, the server attempts to match it against loaded fixtures [4]. Matching can be performed via several criteria, including: - User message (string or RegExp) [5][6] - Tool name [5][6] - Tool call ID [6] - Custom predicates: You can define a predicate function (e.g., (input: RunAgentInput) => boolean) to determine if a fixture matches a specific request [6]. Fixture Management You can programmatically manage the fixtures loaded in an AIMock instance using the following methods: - addFixture(fixture): Adds a single fixture to the server's collection [3]. - addFixtures(fixtures): Adds an array of fixtures [3][7]. - clearFixtures: Clears all currently loaded fixtures, which is often used to ensure isolation between test scenarios [4]. - removeFixture(fixtureId) / remove operations: While add/clear are standard, fixture isolation is typically handled by clearing and reloading or by leveraging reset mechanisms in test harnesses to ensure that fixtures from previous scenarios do not shadow current tests [4]. For detailed documentation, refer to the official AIMock site at aimock.copilotkit.dev [1][2]. Current features as of v1.35.0 include support for block ordering (allowing tool-first or interleaved interactions) [8] and native mocks for various AI services, including LLM providers, MCP tools, and agent-to-agent (A2A) protocols [1][2].
Citations:
- 1: https://github.com/CopilotKit/aimock
- 2: https://dev.to/copilotkit/aimock-one-mock-server-for-your-entire-ai-stack-1jhp
- 3: https://explore.market.dev/ecosystems/typescript/projects/llmock
- 4: cacheplane/dawnai@306380e
- 5: https://testomat.ai/docs/explorbot/contributing/ai-integration-tests/
- 6: [Feature]: Support recording and replaying HITL continuation requests CopilotKit/aimock#232
- 7: CopilotKit/CopilotKit@1bf8c44
- 8: https://github.com/CopilotKit/llmock/blob/main/CHANGELOG.md
🌐 Web query:
dev.to "AIMock" predicates match "last user message" "sequenceIndex"
💡 Result:
In AIMock, sequenceIndex is a matching criterion used to distinguish between identical request shapes (such as the same user message) that occur multiple times in a sequence [1][2][3]. When a test or conversation generates repeated requests that are otherwise byte-identical, AIMock can use sequenceIndex (e.g., 0, 1, 2,...) to ensure the correct response is served based on the call order [1]. Key points regarding sequenceIndex and message matching: - Usage: It is commonly used to test retry logic or multi-turn workflows where the same message should elicit different responses on successive attempts [2][3]. - Behavior: Fixtures are matched based on the request shape (often the last user message) [1][2]. If multiple fixtures share the same user message, sequenceIndex acts as a counter-based discriminator [1][4]. - Limitations: Because sequenceIndex relies on an absolute call counter, it may be brittle in concurrent environments [4]. For multi-turn conversational flows where you want to match specific turns regardless of absolute call order, documentation recommends using stateless discriminators like turnIndex (based on assistant message counts) or toolCallId [5][4][6]. - Shadowing: If fixtures have identical match keys, the router typically picks the first one defined in the load order, which may result in later entries being "shadowed" unless differentiated by sequenceIndex [1]. When defining fixtures, you can mix and match these criteria [5][7]. For instance, you might define a specific index for an initial call and omit it for a fallback response [2][3].
Citations:
- 1: https://aimock.copilotkit.dev/record-replay/index.html
- 2: https://dev.to/copilotkit/aimock-one-mock-server-for-your-entire-ai-stack-1jhp
- 3: https://www.copilotkit.ai/blog/aimock-one-tool-to-mock-your-entire-ai-stack
- 4: https://github.com/CopilotKit/aimock/blob/main/CHANGELOG.md
- 5: https://github.com/CopilotKit/aimock
- 6: https://github.com/ag-ui-protocol/ag-ui/blob/677dfca1/sdks/dotnet/docs/cross-language-testing.md
- 7: https://p.rst.im/q/github.com/CopilotKit/aimock
🌐 Web query:
site:github.com/CopilotKit/aimock "removeFixture" "addFixture" "sequenceIndex"
💡 Result:
In the context of CopilotKit's aimock library, addFixture and removeFixture are mechanisms used to manage request-response fixtures for testing and development, while sequenceIndex is a configuration option used for matching and disambiguating those fixtures [1][2]. addFixture and removeFixture (often associated with programmatic API control or server-side fixture management) handle the lifecycle of mock responses [2][3]. addFixture is used to register a new fixture (often with associated options like latency, chunkSize, and sequenceIndex), and issues may arise if these options are not correctly passed or normalized when added programmatically [1][3]. sequenceIndex serves as a match criterion used by the aimock router to identify the correct fixture for a given request [2]. It is particularly relevant for multi-turn or sequential interaction flows where multiple similar requests might occur [1][2]. In standard usage, sequenceIndex helps distinguish between these steps [2]. The system also utilizes turnIndex as a related, stateless counter based on assistant-message counts to assist in multi-turn conversations [1][2]. In some configurations, sequenceIndex or turnIndex state gates can lead to fixtures being skipped if they do not match the expected sequence or turn state [1]. Diagnostics in aimock will report when candidate fixtures are skipped due to these state indices [1][4].
Citations:
- 1: https://github.com/CopilotKit/aimock/blob/main/CHANGELOG.md
- 2: https://github.com/CopilotKit/aimock
- 3: CopilotKit/aimock@2cdbea3
- 4: CopilotKit/aimock@f21e718
🌐 Web query:
site:dev.to/copilotkit.ai CopilotKit/AIMock "predicate" "LLMock" "addFixture"
💡 Result:
The package formerly known as @copilotkit/llmock has been renamed to @copilotkit/aimock [1]. While the class LLMock remains available for backward compatibility, new projects should use the aimock package [1]. There is no publicly documented method named addFixture within the current CopilotKit/aimock codebase or its associated testing infrastructure based on available information. The aimock library provides specialized mocking tools for various services, including LLMs, MCP, A2A, AG-UI, and vector databases [1]. For detailed usage and documentation on how to configure and use these mocks, you should refer to the official documentation provided by the CopilotKit/aimock project [1].
Citations:
🌐 Web query:
aimock 1.35.0 fixtures router match "userMessage" "sequenceIndex"
💡 Result:
In aimock v1.35.0, the sequenceIndex field in a fixture match object is a property used to return different responses for the same query across successive requests [1][2]. Key details regarding its usage and behavior include: Matching Logic: - The router tracks how many times each unique match pattern has been hit [1]. - sequenceIndex: 0 matches the first request, sequenceIndex: 1 the second, and so on [1]. - Different match patterns have independent counters; if a fixture with a sequenceIndex does not match the current count, the router falls through to the next available fixture [1]. - Fixtures without a sequenceIndex match any occurrence, providing backward compatibility [1]. Operational Context: - sequenceIndex uses a server-side counter that resets on mock.reset() [1]. - Because it is a stateful, global counter, it can become non-deterministic in concurrent environments where multiple integrations share the same aimock instance [3]. - Due to these limitations, developers are encouraged to prefer stateless matching alternatives such as toolCallId, turnIndex, or hasToolResult for multi-turn or multi-step agent workflows [1][4][3]. Configuration Example: json { "fixtures": [ { "match": { "userMessage": "retry", "sequenceIndex": 0 }, "response": { "content": "First attempt" } }, { "match": { "userMessage": "retry", "sequenceIndex": 1 }, "response": { "content": "Second attempt" } }, { "match": { "userMessage": "retry" }, "response": { "content": "Fallback" } } ] } [2]
Citations:
- 1: https://aimock.copilotkit.dev/sequential-responses/index.html
- 2: https://dev.to/copilotkit/aimock-one-mock-server-for-your-entire-ai-stack-1jhp
- 3: CopilotKit/CopilotKit@54afbca
- 4: https://aimock.copilotkit.dev/record-replay/index.html
Bound the abandon parent fixture to one parent turn.
The abandoned parent is still active after abandoning the child, so a second parent request without SUBTASK_RESULT_INJECTION can satisfy this predicate and serve new_task again. Use an explicit one-turn marker such as a specific turn count/tool-call exclusion, or remove a bindable parent fixture after the initial response.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/vscode-e2e/src/fixtures/subtasks.ts` around lines 574 - 579, Restrict
the fixture registered in mock.addFixture to match only the initial
abandon-parent turn, preventing subsequent parent requests without
SUBTASK_RESULT_INJECTION from serving new_task again. Update the predicate using
an explicit turn-count or tool-call exclusion, or ensure the fixture is removed
after its first response while preserving the existing marker and injection
checks.
cd1ae7a to
7565dad
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (4)
src/core/task/__tests__/ghost-quarantine.spec.ts (1)
175-210: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared quarantine simulation.
Lines 175-210 duplicate lines 71-106 almost exactly. Two copies of the same simulated logic can drift, and then path 1 and path 3 would assert different behavior while claiming to model the same handler. Extract one helper, for example
quarantineGhost(callId, ...), and call it from bothhandleStreamingToolCallEndandhandleFinalizeRawChunks.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/task/__tests__/ghost-quarantine.spec.ts` around lines 175 - 210, Extract the duplicated ghost-removal and telemetry logic from the existing paths into one shared quarantine helper, such as quarantineGhost, preserving index updates, state cleanup, and telemetry values. Replace the inline implementations in handleStreamingToolCallEnd and handleFinalizeRawChunks with calls to that helper so both paths use identical behavior.src/api/providers/__tests__/mimo.spec.ts (3)
77-85: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer bracket notation over the double assertion.
optionsis a protected member. Bracket access reads it without a double assertion. If you keep the double assertion, add a comment that explains it, as the guidelines require.♻️ Proposed change
- expect((h as unknown as { options: { openAiBaseUrl: string } }).options.openAiBaseUrl).toBe( - "https://token-plan-sgp.xiaomimimo.com/v1", - ) + expect(h["options"].openAiBaseUrl).toBe("https://token-plan-sgp.xiaomimimo.com/v1")As per coding guidelines: "Avoid
as any; use typed APIs, bracket notation for private members where appropriate... Use double assertions only as a last resort and explain them with a comment."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api/providers/__tests__/mimo.spec.ts` around lines 77 - 85, Update the MimoHandler test assertions to access the protected options member via bracket notation instead of the current double assertion, preserving the existing openAiBaseUrl expectations for both default and custom URLs.Source: Coding guidelines
2111-2112: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReplace hard-coded production line numbers in comments.
These comments cite
mimo.tsline numbers such as "line 103-105", "lines 136-139", and "line 108". Any edit tomimo.tsmakes them wrong. Describe the branch instead, for example "the early return whendelta.tool_callsis absent or empty".Also applies to: 2142-2143, 2173-2174, 2218-2219
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api/providers/__tests__/mimo.spec.ts` around lines 2111 - 2112, Replace the hard-coded mimo.ts line-number references in the comments near the tests around filterToFirstToolCall with descriptions of the relevant branches, such as the early return when delta.tool_calls is absent or empty. Update all noted comment occurrences and preserve their explanatory intent without citing production line numbers.
693-695: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDrop the double assertion by supplying
taskId.
ApiHandlerCreateMessageMetadatarequirestaskId. Adding it removes the need for the cast.♻️ Proposed change
- const stream = handler.createMessage("System prompt", messages, { - tools, - } as unknown as ApiHandlerCreateMessageMetadata) + const stream = handler.createMessage("System prompt", messages, { taskId: "test-task", tools })As per coding guidelines: "Use double assertions only as a last resort and explain them with a comment."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api/providers/__tests__/mimo.spec.ts` around lines 693 - 695, Update the createMessage metadata in the test around handler.createMessage to include the required taskId property, then remove the as unknown as ApiHandlerCreateMessageMetadata double assertion while preserving the existing tools value.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/260730_0001_session_branch-cleanup/184700_debug-report.md`:
- Around line 61-65: Reconcile the “2.3 Contamination (drop)” classification by
recomputing commits with git log --not upstream/main, removing nonexistent
hashes such as 4e52024d1, correcting category memberships and totals, and
ensuring the cleanup plan uses only verified commits.
- Line 141: Update the documented rollback path before the branch switch to
abort an active cherry-pick first, using `git cherry-pick --abort` conditionally
when one is in progress. Preserve the existing `git switch -C` rollback command
and backup branch references.
In
`@docs/260805_0001_session_ci-all-green/150700_code-b17-mistral-coverage-report.md`:
- Line 5: Remove the unrelated Mistral coverage report from the current PR
report set, or relabel it as historical evidence and relocate it accordingly; if
retaining it as a current report, regenerate its metadata and contents for PR
`#1130` and MiMo enforcement instead of PR `#1132` and the Mistral cost-calculation
changes.
In `@docs/260805_0001_session_ci-all-green/151400_debug-coverage-b12.md`:
- Around line 170-186: Make the documented test command block independent of the
current working directory by isolating each directory change in a subshell or
explicitly returning to the repository root before subsequent commands. Preserve
the existing test and analysis commands while ensuring packages/types,
packages/telemetry, and scripts/coverage-diff-analysis.py resolve from the
repository root.
- Around line 69-70: Update the coverage note for resolveToolCallPolicy() and
captureToolCallPolicyResolution() to make the reported count auditable: either
change 12 to the full 24-line range count, or explicitly identify 12 as the
executable-line count and document the filtering rule used.
- Around line 157-162: Update the branch checkout sequence around git reset
--hard to protect tracked local changes: add a clean-working-tree validation
before the reset, or perform the checkout and reset in a temporary git worktree.
Preserve the existing fetch and branch target while ensuring the destructive
reset cannot silently discard uncommitted work.
In `@scripts/coverage-diff-analysis.py`:
- Around line 64-65: Rename the ambiguous loop variable l in the hunk lines
enumeration to a descriptive name such as added_line, and update its use in the
print statement so the script passes Ruff E741 without suppression.
- Line 8: Update the git diff subprocess handling in the script’s diff-analysis
flow to fail closed when the command returns a nonzero status, rather than
treating empty output as “NO CHANGES.” Check result.returncode (or enable
check=True), and include result.stderr in the raised or logged failure while
preserving normal processing for successful diffs.
- Around line 40-53: Update the diff parser around the hunk-processing loop to
maintain a new-file line cursor: initialize it from each hunk’s `hunk_start`,
advance it for context and added lines, and account for deletions without
advancing it. Store each added line together with its exact new-file line number
instead of inferring positions from addition order, and recognize only lines
matching the `+++ <path>` file-header format as metadata so valid `++`-prefixed
additions are retained. Add fixtures covering separated additions, deletions,
and additions whose content begins with `++`.
In `@src/api/providers/__tests__/mimo.spec.ts`:
- Around line 1915-1921: Update the rejectionError fixture in the “should retry
when error message contains 'parallel_tool_calls' without status 400” test by
removing its status property, so the test exercises the message-only retry
branch while preserving the existing error message and retry assertions.
In `@src/core/task/__tests__/ghost-quarantine.spec.ts`:
- Line 94: The shared telemetryContext fixture in
src/core/task/__tests__/ghost-quarantine.spec.ts#L223-L228 should type its
provider field using the parameter type declared by resolveToolCallPolicy; then
remove the as any casts at
src/core/task/__tests__/ghost-quarantine.spec.ts#L94-L94, `#L135-L135`, and
`#L198-L198` so each call passes telemetryContext.provider directly.
---
Nitpick comments:
In `@src/api/providers/__tests__/mimo.spec.ts`:
- Around line 77-85: Update the MimoHandler test assertions to access the
protected options member via bracket notation instead of the current double
assertion, preserving the existing openAiBaseUrl expectations for both default
and custom URLs.
- Around line 2111-2112: Replace the hard-coded mimo.ts line-number references
in the comments near the tests around filterToFirstToolCall with descriptions of
the relevant branches, such as the early return when delta.tool_calls is absent
or empty. Update all noted comment occurrences and preserve their explanatory
intent without citing production line numbers.
- Around line 693-695: Update the createMessage metadata in the test around
handler.createMessage to include the required taskId property, then remove the
as unknown as ApiHandlerCreateMessageMetadata double assertion while preserving
the existing tools value.
In `@src/core/task/__tests__/ghost-quarantine.spec.ts`:
- Around line 175-210: Extract the duplicated ghost-removal and telemetry logic
from the existing paths into one shared quarantine helper, such as
quarantineGhost, preserving index updates, state cleanup, and telemetry values.
Replace the inline implementations in handleStreamingToolCallEnd and
handleFinalizeRawChunks with calls to that helper so both paths use identical
behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 72cb97d3-0f67-47af-b88c-340da52a3245
📒 Files selected for processing (55)
docs/260730_0001_session_branch-cleanup/170000_debug-report.mddocs/260730_0001_session_branch-cleanup/173200_debug-report.mddocs/260730_0001_session_branch-cleanup/173230_execution-plan.mddocs/260730_0001_session_branch-cleanup/175300_code-report.mddocs/260730_0001_session_branch-cleanup/181500_debug-dnd-ux-runbook.mddocs/260730_0001_session_branch-cleanup/182225_code-report.mddocs/260730_0001_session_branch-cleanup/184700_debug-report.mddocs/260805_0001_session_ci-all-green/150700_code-b17-mistral-coverage-report.mddocs/260805_0001_session_ci-all-green/151400_debug-coverage-b12.mdpackages/telemetry/src/TelemetryService.tspackages/types/src/__tests__/provider-settings.test.tspackages/types/src/model.tspackages/types/src/provider-settings.tspackages/types/src/providers/mimo.tspackages/types/src/telemetry.tsscripts/coverage-diff-analysis.pysrc/api/index.tssrc/api/providers/__tests__/base-provider.spec.tssrc/api/providers/__tests__/mimo.spec.tssrc/api/providers/__tests__/openai.spec.tssrc/api/providers/base-openai-compatible-provider.tssrc/api/providers/base-provider.tssrc/api/providers/mimo.tssrc/api/providers/openai.tssrc/core/assistant-message/NativeToolCallParser.tssrc/core/assistant-message/ToolCallRetentionPolicy.tssrc/core/assistant-message/__tests__/NativeToolCallParser.spec.tssrc/core/assistant-message/__tests__/ToolCallRetentionPolicy-telemetry.spec.tssrc/core/assistant-message/__tests__/ToolCallRetentionPolicy.spec.tssrc/core/prompts/tools/native-tools/execute_command.tssrc/core/task/Task.tssrc/core/task/__tests__/ghost-quarantine.spec.tssrc/core/task/__tests__/tool-call-policy.spec.tssrc/core/tools/ExecuteCommandTool.tssrc/eslint-suppressions.jsonsrc/shared/tools.tswebview-ui/src/components/settings/providers/OpenAICompatible.tsxwebview-ui/src/i18n/locales/ca/settings.jsonwebview-ui/src/i18n/locales/de/settings.jsonwebview-ui/src/i18n/locales/en/settings.jsonwebview-ui/src/i18n/locales/es/settings.jsonwebview-ui/src/i18n/locales/fr/settings.jsonwebview-ui/src/i18n/locales/hi/settings.jsonwebview-ui/src/i18n/locales/id/settings.jsonwebview-ui/src/i18n/locales/it/settings.jsonwebview-ui/src/i18n/locales/ja/settings.jsonwebview-ui/src/i18n/locales/ko/settings.jsonwebview-ui/src/i18n/locales/nl/settings.jsonwebview-ui/src/i18n/locales/pl/settings.jsonwebview-ui/src/i18n/locales/pt-BR/settings.jsonwebview-ui/src/i18n/locales/ru/settings.jsonwebview-ui/src/i18n/locales/tr/settings.jsonwebview-ui/src/i18n/locales/vi/settings.jsonwebview-ui/src/i18n/locales/zh-CN/settings.jsonwebview-ui/src/i18n/locales/zh-TW/settings.json
💤 Files with no reviewable changes (1)
- src/eslint-suppressions.json
🚧 Files skipped from review as they are similar to previous changes (41)
- webview-ui/src/i18n/locales/id/settings.json
- src/shared/tools.ts
- src/api/providers/tests/openai.spec.ts
- webview-ui/src/i18n/locales/ko/settings.json
- packages/types/src/model.ts
- src/core/prompts/tools/native-tools/execute_command.ts
- webview-ui/src/components/settings/providers/OpenAICompatible.tsx
- webview-ui/src/i18n/locales/pl/settings.json
- webview-ui/src/i18n/locales/ca/settings.json
- webview-ui/src/i18n/locales/zh-CN/settings.json
- webview-ui/src/i18n/locales/ja/settings.json
- webview-ui/src/i18n/locales/hi/settings.json
- webview-ui/src/i18n/locales/de/settings.json
- src/core/assistant-message/tests/NativeToolCallParser.spec.ts
- packages/types/src/providers/mimo.ts
- packages/types/src/tests/provider-settings.test.ts
- src/api/providers/base-provider.ts
- packages/types/src/provider-settings.ts
- packages/types/src/telemetry.ts
- webview-ui/src/i18n/locales/zh-TW/settings.json
- webview-ui/src/i18n/locales/ru/settings.json
- src/core/tools/ExecuteCommandTool.ts
- src/api/providers/base-openai-compatible-provider.ts
- src/api/providers/openai.ts
- webview-ui/src/i18n/locales/fr/settings.json
- src/core/assistant-message/tests/ToolCallRetentionPolicy.spec.ts
- webview-ui/src/i18n/locales/tr/settings.json
- src/api/index.ts
- src/core/task/tests/tool-call-policy.spec.ts
- src/core/assistant-message/tests/ToolCallRetentionPolicy-telemetry.spec.ts
- webview-ui/src/i18n/locales/es/settings.json
- webview-ui/src/i18n/locales/pt-BR/settings.json
- webview-ui/src/i18n/locales/it/settings.json
- packages/telemetry/src/TelemetryService.ts
- src/api/providers/tests/base-provider.spec.ts
- src/api/providers/mimo.ts
- src/core/assistant-message/ToolCallRetentionPolicy.ts
- src/core/assistant-message/NativeToolCallParser.ts
- webview-ui/src/i18n/locales/nl/settings.json
- webview-ui/src/i18n/locales/vi/settings.json
- src/core/task/Task.ts
| # git branch -D fix/mimo-parallel-tool-call-policy-backup-260730 (keep until PR merges — recommended) | ||
| ``` | ||
|
|
||
| Rollback path at any point before step 7: `git switch -C fix/mimo-parallel-tool-call-policy fix/mimo-parallel-tool-call-policy-backup-260730`. |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Abort an active cherry-pick before switching branches.
If rollback runs during a conflict, git switch -C can fail because the index contains unmerged paths. Add git cherry-pick --abort before the switch when a cherry-pick is in progress.
Suggested rollback sequence
git cherry-pick --abort # only when a cherry-pick is active
git switch -C fix/mimo-parallel-tool-call-policy fix/mimo-parallel-tool-call-backup-260730🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/260730_0001_session_branch-cleanup/184700_debug-report.md` at line 141,
Update the documented rollback path before the branch switch to abort an active
cherry-pick first, using `git cherry-pick --abort` conditionally when one is in
progress. Preserve the existing `git switch -C` rollback command and backup
branch references.
|
|
||
| ## Task Summary | ||
|
|
||
| Added 3 test cases to `src/api/providers/__tests__/mistral.spec.ts` covering the uncovered cost-calculation block (lines 158-174) in `src/api/providers/mistral.ts` to resolve the `codecov/patch` failure on PR #1132. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove or relabel this unrelated coverage report.
The current PR is #1130 for MiMo enforcement, but this file reports PR #1132, commit 225ebeb41, branch myk1yt/pr/b17-provider-cost-v2, and mistral.spec.ts. Regenerate the report for PR #1130, or mark it as historical evidence and move it out of the current PR report set.
Also applies to: 25-26, 38-41
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@docs/260805_0001_session_ci-all-green/150700_code-b17-mistral-coverage-report.md`
at line 5, Remove the unrelated Mistral coverage report from the current PR
report set, or relabel it as historical evidence and relocate it accordingly; if
retaining it as a current report, regenerate its metadata and contents for PR
`#1130` and MiMo enforcement instead of PR `#1132` and the Mistral cost-calculation
changes.
| - **Lines 4480-4503** (12 lines) — `resolveToolCallPolicy()` and `captureToolCallPolicyResolution()` telemetry in `createMessage` stream setup. Not exercised. | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Correct the reported line count.
Lines 4480-4503 span 24 lines, but Line 69 labels the range as 12 lines. If 12 is the executable-line count, state that explicitly and show the filtering rule. Otherwise correct the count so the coverage prioritization is auditable.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/260805_0001_session_ci-all-green/151400_debug-coverage-b12.md` around
lines 69 - 70, Update the coverage note for resolveToolCallPolicy() and
captureToolCallPolicyResolution() to make the reported count auditable: either
change 12 to the full 24-line range count, or explicitly identify 12 as the
executable-line count and document the filtering rule used.
| ```bash | ||
| # Checkout branch | ||
| git fetch myk1yt | ||
| git checkout pr/b12-mimo-enforcement-v2 | ||
| git reset --hard myk1yt/pr/b12-mimo-enforcement-v2 | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Protect the working tree before git reset --hard.
Lines 160-161 can discard tracked local changes without warning. Add a clean-worktree check or use a temporary git worktree before running this sequence.
Example guard
git fetch myk1yt
git checkout pr/b12-mimo-enforcement-v2
+if ! git diff --quiet || ! git diff --cached --quiet; then
+ echo "Abort: working tree is not clean." >&2
+ exit 1
+fi
git reset --hard myk1yt/pr/b12-mimo-enforcement-v2📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ```bash | |
| # Checkout branch | |
| git fetch myk1yt | |
| git checkout pr/b12-mimo-enforcement-v2 | |
| git reset --hard myk1yt/pr/b12-mimo-enforcement-v2 |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/260805_0001_session_ci-all-green/151400_debug-coverage-b12.md` around
lines 157 - 162, Update the branch checkout sequence around git reset --hard to
protect tracked local changes: add a clean-working-tree validation before the
reset, or perform the checkout and reset in a temporary git worktree. Preserve
the existing fetch and branch target while ensuring the destructive reset cannot
silently discard uncommitted work.
| import os | ||
|
|
||
| REPO = os.path.dirname(os.path.dirname(os.path.abspath(__file__))) | ||
| BASE = "992585ff8b7bdc750ecf2b79372f5be4d2e5ff71" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Fail closed when git diff fails.
Line 8 selects a fixed commit, but Lines 29-35 ignore the subprocess exit status. If the commit is unavailable, such as in a shallow clone, diff can be empty and the script reports NO CHANGES for every file. Check result.returncode or use check=True, and include result.stderr in the failure.
Also applies to: 29-35
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/coverage-diff-analysis.py` at line 8, Update the git diff subprocess
handling in the script’s diff-analysis flow to fail closed when the command
returns a nonzero status, rather than treating empty output as “NO CHANGES.”
Check result.returncode (or enable check=True), and include result.stderr in the
raised or logged failure while preserving normal processing for successful
diffs.
| for line in diff.split("\n"): | ||
| if line.startswith("diff --git"): | ||
| m = re.search(r"diff --git a/(.+?) b/", line) | ||
| if m: | ||
| current_file = m.group(1) | ||
| added_lines[current_file] = [] | ||
| elif line.startswith("@@"): | ||
| m = re.search(r"\+(\d+)(?:,(\d+))?", line) | ||
| if m and current_file: | ||
| new_start = int(m.group(1)) | ||
| added_lines[current_file].append({"hunk_start": new_start, "lines": []}) | ||
| elif line.startswith("+") and not line.startswith("+++"): | ||
| if current_file and added_lines[current_file]: | ||
| added_lines[current_file][-1]["lines"].append(line[1:]) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve exact added-line positions from each diff hunk.
hunk_start is the first new-file line in a hunk. It is not the first added line. The parser stores only additions, so start + i becomes incorrect when context or deletion lines occur between additions. The not line.startswith("+++") check also drops valid added lines whose content begins with ++. Track a new-file cursor, store each added line with its exact number, and recognize only the +++ <path> file header as metadata.
Add fixtures for separated additions, deletions, and ++-prefixed additions.
Also applies to: 59-65
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/coverage-diff-analysis.py` around lines 40 - 53, Update the diff
parser around the hunk-processing loop to maintain a new-file line cursor:
initialize it from each hunk’s `hunk_start`, advance it for context and added
lines, and account for deletions without advancing it. Store each added line
together with its exact new-file line number instead of inferring positions from
addition order, and recognize only lines matching the `+++ <path>` file-header
format as metadata so valid `++`-prefixed additions are retained. Add fixtures
covering separated additions, deletions, and additions whose content begins with
`++`.
| for i, l in enumerate(hunk["lines"]): | ||
| print(f" {start + i}: {l.rstrip()}") |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Rename the ambiguous loop variable.
Ruff reports E741 at Line 64. Rename l to added_line or another descriptive name so this new script passes lint without a suppression.
🧰 Tools
🪛 Ruff (0.16.1)
[error] 64-64: Ambiguous variable name: l
(E741)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/coverage-diff-analysis.py` around lines 64 - 65, Rename the ambiguous
loop variable l in the hunk lines enumeration to a descriptive name such as
added_line, and update its use in the print statement so the script passes Ruff
E741 without suppression.
Source: Linters/SAST tools
| it("should retry when error message contains 'parallel_tool_calls' without status 400", async () => { | ||
| // isParallelToolCallsRejected also returns true when the message | ||
| // contains "parallel_tool_calls" regardless of status code. | ||
| const rejectionError = Object.assign(new Error("400 - Unrecognized parameter: parallel_tool_calls"), { | ||
| status: 400, | ||
| }) | ||
| mockCreate.mockRejectedValueOnce(rejectionError) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
The test name and the fixture disagree.
The title states "without status 400", but line 1919 sets status: 400. The test therefore repeats the status-400 path already covered at lines 469-519 and never exercises the message-only match. Remove status to cover the intended branch.
🐛 Proposed fix
- const rejectionError = Object.assign(new Error("400 - Unrecognized parameter: parallel_tool_calls"), {
- status: 400,
- })
+ const rejectionError = new Error("Unrecognized parameter: parallel_tool_calls")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it("should retry when error message contains 'parallel_tool_calls' without status 400", async () => { | |
| // isParallelToolCallsRejected also returns true when the message | |
| // contains "parallel_tool_calls" regardless of status code. | |
| const rejectionError = Object.assign(new Error("400 - Unrecognized parameter: parallel_tool_calls"), { | |
| status: 400, | |
| }) | |
| mockCreate.mockRejectedValueOnce(rejectionError) | |
| it("should retry when error message contains 'parallel_tool_calls' without status 400", async () => { | |
| // isParallelToolCallsRejected also returns true when the message | |
| // contains "parallel_tool_calls" regardless of status code. | |
| const rejectionError = new Error("Unrecognized parameter: parallel_tool_calls") | |
| mockCreate.mockRejectedValueOnce(rejectionError) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/api/providers/__tests__/mimo.spec.ts` around lines 1915 - 1921, Update
the rejectionError fixture in the “should retry when error message contains
'parallel_tool_calls' without status 400” test by removing its status property,
so the test exercises the message-only retry branch while preserving the
existing error message and retry assertions.
| } | ||
| streamingToolCallState.delete(event.id) | ||
|
|
||
| const ghostPolicy1 = resolveToolCallPolicy(telemetryContext.modelInfo, telemetryContext.provider as any) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Three as any casts share one root cause: the untyped provider field. The shared telemetryContext fixture at lines 223-228 infers provider as a plain string, so each resolveToolCallPolicy call site widens it with as any. Type the fixture field once and delete all three casts.
src/core/task/__tests__/ghost-quarantine.spec.ts#L94-L94: passtelemetryContext.providerwithout the cast, and annotate the fixtureproviderfield with the parameter type thatresolveToolCallPolicydeclares.src/core/task/__tests__/ghost-quarantine.spec.ts#L135-L135: passtelemetryContext.providerwithout the cast.src/core/task/__tests__/ghost-quarantine.spec.ts#L198-L198: passtelemetryContext.providerwithout the cast.
As per coding guidelines: "Avoid as any; use typed APIs, bracket notation for private members where appropriate, or precise test doubles and unknown with type guards."
📍 Affects 1 file
src/core/task/__tests__/ghost-quarantine.spec.ts#L94-L94(this comment)src/core/task/__tests__/ghost-quarantine.spec.ts#L135-L135src/core/task/__tests__/ghost-quarantine.spec.ts#L198-L198
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/core/task/__tests__/ghost-quarantine.spec.ts` at line 94, The shared
telemetryContext fixture in
src/core/task/__tests__/ghost-quarantine.spec.ts#L223-L228 should type its
provider field using the parameter type declared by resolveToolCallPolicy; then
remove the as any casts at
src/core/task/__tests__/ghost-quarantine.spec.ts#L94-L94, `#L135-L135`, and
`#L198-L198` so each call passes telemetryContext.provider directly.
Source: Coding guidelines
github-actions
Bot
added
has-conflicts
…penAI Compatible provider - Add openAiToolStrictMode boolean to provider settings (profile-scoped, default false) - Add strict toggle checkbox in OpenAICompatible settings UI - BaseProvider.convertToolsForOpenAI now accepts strictMode parameter - strictMode=true: strict:true + hardened schema - strictMode=false: strict:false + best-effort original schema - MCP tools: always strict:false regardless of setting - Wire setting into all 4 openai.ts request paths - Fix reasoning effort unsafe cast, add xhigh and max values - Make parallel_tool_calls conditional on tools being present
# Conflicts: # src/core/tools/error-interception/StructuralValidator.ts
# Conflicts: # src/core/assistant-message/__tests__/NativeToolCallParser.spec.ts # src/core/assistant-message/__tests__/presentAssistantMessage-parser-dedup.integration.spec.ts # src/core/assistant-message/presentAssistantMessage.ts
# Conflicts: # src/core/assistant-message/__tests__/presentAssistantMessage-parser-dedup.integration.spec.ts
…ception refs from backup
MimoHandler was passing raw tool schemas to the API without the strict mode conversion that all other OpenAI-compatible providers use. This caused tool call errors due to missing required/strict fields. - Call this.convertToolsForOpenAI(tools) instead of raw assignment - Adds strict: true, required properties, additionalProperties: false
An id-less argument-continuation chunk belongs to the most recent id chunk seen at its index. When a provider reuses index 0 with a NEW id (a disguised second parallel call), the new call's id chunk was dropped but its id-less argument fragments were still kept and concatenated into the FIRST call's accumulator, corrupting its JSON. Track dropped indexes in filterToFirstToolCall state and drop subsequent id-less fragments for those indexes. Also rewrite the function docblock, which referenced a non-existent error-interception retry loop.
The parseErrors/parseFailures docblocks claimed presentAssistantMessage routes recorded failures to an INVALID_JSON_ARGUMENTS error-interception pattern. No such routing exists on this codebase; describe the actual lifecycle (consumed via the consume* APIs, cleared on new API request). Comment-only change, no behavior difference.
parseErrors/parseFailures static maps accumulated an entry per malformed tool call and were never cleared in production (the consume* APIs have no production callers), slowly leaking for the extension-host lifetime. Add NativeToolCallParser.clearParseFailures() and call it in Task.recursivelyMakeClineRequests alongside clearAllStreamingToolCalls()/ clearRawChunkState(), where other per-stream state is reset. The consume* APIs keep working for tests.
MiMo sends tools through convertToolsForOpenAI(), which attaches a strict flag to every function tool. An OpenAI-compatible endpoint that doesn't support structured outputs rejects the request with a 400 and the turn fails outright. Mirror the existing parallel_tool_calls fallback: detect schema-rejection errors narrowly (400 status plus a mention of strict/additionalProperties in a tools context, so unrelated 400s like MiMo's missing-reasoning_content rejection are not retried) and retry once with the original schemas and no strict flag.
7565dad to
365f042
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
src/core/task/__tests__/tool-call-policy.spec.ts (2)
35-40: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the premise stated in the test name.
The test name states that the request control is
"none", but the test only assertspolicy.enforcement. IfmimoModels["mimo-v2.5-pro"].toolCallCapabilities.parallelToolCallsRequestControlchanges, the test still passes with a misleading name. Add an assertion on the fixture's request control.♻️ Proposed assertion
it("uses local enforcement when request control is 'none'", () => { const modelInfo = mimoModels["mimo-v2.5-pro"] as ModelInfo + expect(modelInfo.toolCallCapabilities?.parallelToolCallsRequestControl).toBe("none") const policy = resolveToolCallPolicy(modelInfo, "mimo") expect(policy.enforcement).toBe("local") })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/task/__tests__/tool-call-policy.spec.ts` around lines 35 - 40, Add an assertion in the test using mimoModels["mimo-v2.5-pro"] to verify its parallelToolCallsRequestControl is "none" before asserting policy.enforcement remains "local".
118-126: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename the test to match the actual condition.
mimois a declared provider inpackages/types/src/providers/mimo.ts. The test name calls it an unknown provider. The real condition is thatmimois not in the parallel-capable provider set. Rename the test to state that condition, for example "provider outside the parallel-capable set resolves to conservative single".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/task/__tests__/tool-call-policy.spec.ts` around lines 118 - 126, Rename the test case around resolveToolCallPolicy from describing “mimo” as an unknown provider to describing a provider outside the parallel-capable provider set, while preserving its existing assertions and behavior.src/api/providers/__tests__/mimo.spec.ts (3)
127-139: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExplain the double assertions on the reasoning content block.
reasoningis not a member ofAnthropic.Messages.MessageParam["content"], so the cast is required here. The coding guidelines require a comment that states the reason. The same pattern appears at lines 215, 228, and 346-357.♻️ Proposed change
+ // Double assertion: `reasoning` is a provider-specific block that + // the Anthropic content union does not declare. { type: "reasoning" as const, text: "Let me think...", } as unknown as Anthropic.Messages.MessageParam["content"][number],As per coding guidelines: "Use double assertions only as a last resort and explain them with a comment."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api/providers/__tests__/mimo.spec.ts` around lines 127 - 139, Add explanatory comments at each double assertion for the reasoning content blocks in the Mimo conversion tests, including the instances near the existing test cases and the later occurrences. State that the reasoning block is not part of Anthropic.Messages.MessageParam["content"] and therefore requires the double assertion, while leaving the test behavior unchanged.Source: Coding guidelines
77-85: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse bracket notation instead of a double assertion.
The coding guidelines allow bracket notation for private members and treat double assertions as a last resort. Here the double assertion is avoidable. Replace it with
handler["options"], which keeps the property typed.♻️ Proposed change
- expect((h as unknown as { options: { openAiBaseUrl: string } }).options.openAiBaseUrl).toBe( - "https://token-plan-sgp.xiaomimimo.com/v1", - ) + expect(h["options"].openAiBaseUrl).toBe("https://token-plan-sgp.xiaomimimo.com/v1") }) it("should use custom base URL when provided", () => { const customUrl = "https://api.xiaomimimo.com/v1" const h = new MimoHandler({ ...mockOptions, mimoBaseUrl: customUrl }) - expect((h as unknown as { options: { openAiBaseUrl: string } }).options.openAiBaseUrl).toBe(customUrl) + expect(h["options"].openAiBaseUrl).toBe(customUrl)As per coding guidelines: "Avoid
as any; use typed APIs, bracket notation for private members where appropriate… Use double assertions only as a last resort and explain them with a comment."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api/providers/__tests__/mimo.spec.ts` around lines 77 - 85, Replace the double type assertion used to access the private options member in the MimoHandler tests with bracket notation on the handler instance, using the existing typed property access pattern for both assertions. Keep the expected base URL checks unchanged.Source: Coding guidelines
693-698: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the double assertion by supplying
taskId.
ApiHandlerCreateMessageMetadatarequirestaskId. The other tests in this file pass{ taskId: "test-task", tools }and need no cast. Use the same shape here.♻️ Proposed change
- const stream = handler.createMessage("System prompt", messages, { - tools, - } as unknown as ApiHandlerCreateMessageMetadata) + const stream = handler.createMessage("System prompt", messages, { taskId: "test-task", tools })As per coding guidelines: "Use double assertions only as a last resort and explain them with a comment."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api/providers/__tests__/mimo.spec.ts` around lines 693 - 698, Update the metadata argument in the handler.createMessage call within the stream test to include taskId: "test-task" alongside tools, then remove the unknown as ApiHandlerCreateMessageMetadata double assertion. Match the metadata shape used by the other tests in mimo.spec.ts.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/260730_0001_session_branch-cleanup/170000_debug-report.md`:
- Around line 49-59: Add blank lines immediately before and after the
verification table in the “Verification evidence” section of the debug report,
separating it from both headings to satisfy markdownlint MD058.
In `@docs/260730_0001_session_branch-cleanup/181500_debug-dnd-ux-runbook.md`:
- Around line 330-334: Update the Section 8 environment-status summary to state
that no inspection command failed while explicitly noting the pnpm PATH
limitation in the verification environment; retain the existing full-path pnpm
guidance and reference to the prior cleanup report.
- Around line 37-42: Update the fenced code blocks in the runbook, including the
commit-list block near the shown hashes and the additional referenced sections,
with explicit language identifiers: use text for commit listings and powershell
for command examples. Ensure every reported untagged fence is labeled without
changing its contents.
In `@docs/260730_0001_session_branch-cleanup/182225_code-report.md`:
- Around line 29-32: Resolve the inconsistent pre-existing failure count in the
report by rerunning the timestamp off-by-1ms test against the base squash commit
and confirming whether it reproduces. Update the failure count consistently
across the verification, issues, and recommendation sections, including the
statements near the reported counts and investigation scope.
In `@src/eslint-suppressions.json`:
- Around line 847-850: Remove the new `@typescript-eslint/no-explicit-any`
baseline entry for core/task/__tests__/Task.spec.ts from
src/eslint-suppressions.json, and replace the test’s unsafe any values with
precise test doubles or unknown values narrowed by type guards. Update the
relevant tests in Task.spec.ts, then run ESLint with suppression pruning and
ensure the file’s suppression count does not increase.
---
Nitpick comments:
In `@src/api/providers/__tests__/mimo.spec.ts`:
- Around line 127-139: Add explanatory comments at each double assertion for the
reasoning content blocks in the Mimo conversion tests, including the instances
near the existing test cases and the later occurrences. State that the reasoning
block is not part of Anthropic.Messages.MessageParam["content"] and therefore
requires the double assertion, while leaving the test behavior unchanged.
- Around line 77-85: Replace the double type assertion used to access the
private options member in the MimoHandler tests with bracket notation on the
handler instance, using the existing typed property access pattern for both
assertions. Keep the expected base URL checks unchanged.
- Around line 693-698: Update the metadata argument in the handler.createMessage
call within the stream test to include taskId: "test-task" alongside tools, then
remove the unknown as ApiHandlerCreateMessageMetadata double assertion. Match
the metadata shape used by the other tests in mimo.spec.ts.
In `@src/core/task/__tests__/tool-call-policy.spec.ts`:
- Around line 35-40: Add an assertion in the test using
mimoModels["mimo-v2.5-pro"] to verify its parallelToolCallsRequestControl is
"none" before asserting policy.enforcement remains "local".
- Around line 118-126: Rename the test case around resolveToolCallPolicy from
describing “mimo” as an unknown provider to describing a provider outside the
parallel-capable provider set, while preserving its existing assertions and
behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b2283e5b-13fd-4eaa-9e0f-aa0d0059dbb4
📒 Files selected for processing (51)
docs/260730_0001_session_branch-cleanup/170000_debug-report.mddocs/260730_0001_session_branch-cleanup/173200_debug-report.mddocs/260730_0001_session_branch-cleanup/173230_execution-plan.mddocs/260730_0001_session_branch-cleanup/175300_code-report.mddocs/260730_0001_session_branch-cleanup/181500_debug-dnd-ux-runbook.mddocs/260730_0001_session_branch-cleanup/182225_code-report.mddocs/260730_0001_session_branch-cleanup/184700_debug-report.mdpackages/telemetry/src/TelemetryService.tspackages/types/src/__tests__/provider-settings.test.tspackages/types/src/model.tspackages/types/src/provider-settings.tspackages/types/src/providers/mimo.tspackages/types/src/telemetry.tssrc/api/index.tssrc/api/providers/__tests__/base-provider.spec.tssrc/api/providers/__tests__/mimo.spec.tssrc/api/providers/__tests__/openai.spec.tssrc/api/providers/base-openai-compatible-provider.tssrc/api/providers/base-provider.tssrc/api/providers/mimo.tssrc/api/providers/openai.tssrc/core/assistant-message/NativeToolCallParser.tssrc/core/assistant-message/ToolCallRetentionPolicy.tssrc/core/assistant-message/__tests__/NativeToolCallParser.spec.tssrc/core/assistant-message/__tests__/ToolCallRetentionPolicy-telemetry.spec.tssrc/core/assistant-message/__tests__/ToolCallRetentionPolicy.spec.tssrc/core/prompts/tools/native-tools/execute_command.tssrc/core/task/Task.tssrc/core/task/__tests__/tool-call-policy.spec.tssrc/core/tools/ExecuteCommandTool.tssrc/eslint-suppressions.jsonsrc/shared/tools.tswebview-ui/src/components/settings/providers/OpenAICompatible.tsxwebview-ui/src/i18n/locales/ca/settings.jsonwebview-ui/src/i18n/locales/de/settings.jsonwebview-ui/src/i18n/locales/en/settings.jsonwebview-ui/src/i18n/locales/es/settings.jsonwebview-ui/src/i18n/locales/fr/settings.jsonwebview-ui/src/i18n/locales/hi/settings.jsonwebview-ui/src/i18n/locales/id/settings.jsonwebview-ui/src/i18n/locales/it/settings.jsonwebview-ui/src/i18n/locales/ja/settings.jsonwebview-ui/src/i18n/locales/ko/settings.jsonwebview-ui/src/i18n/locales/nl/settings.jsonwebview-ui/src/i18n/locales/pl/settings.jsonwebview-ui/src/i18n/locales/pt-BR/settings.jsonwebview-ui/src/i18n/locales/ru/settings.jsonwebview-ui/src/i18n/locales/tr/settings.jsonwebview-ui/src/i18n/locales/vi/settings.jsonwebview-ui/src/i18n/locales/zh-CN/settings.jsonwebview-ui/src/i18n/locales/zh-TW/settings.json
🚧 Files skipped from review as they are similar to previous changes (43)
- src/api/providers/tests/openai.spec.ts
- webview-ui/src/i18n/locales/vi/settings.json
- webview-ui/src/i18n/locales/de/settings.json
- webview-ui/src/i18n/locales/pl/settings.json
- src/api/providers/openai.ts
- packages/types/src/provider-settings.ts
- webview-ui/src/i18n/locales/pt-BR/settings.json
- webview-ui/src/i18n/locales/nl/settings.json
- packages/types/src/tests/provider-settings.test.ts
- webview-ui/src/components/settings/providers/OpenAICompatible.tsx
- packages/types/src/model.ts
- src/core/prompts/tools/native-tools/execute_command.ts
- src/core/assistant-message/tests/ToolCallRetentionPolicy.spec.ts
- webview-ui/src/i18n/locales/tr/settings.json
- src/core/tools/ExecuteCommandTool.ts
- src/api/providers/base-openai-compatible-provider.ts
- webview-ui/src/i18n/locales/ko/settings.json
- webview-ui/src/i18n/locales/id/settings.json
- src/core/assistant-message/tests/NativeToolCallParser.spec.ts
- src/api/providers/base-provider.ts
- docs/260730_0001_session_branch-cleanup/173200_debug-report.md
- src/shared/tools.ts
- webview-ui/src/i18n/locales/hi/settings.json
- src/api/index.ts
- src/core/assistant-message/tests/ToolCallRetentionPolicy-telemetry.spec.ts
- docs/260730_0001_session_branch-cleanup/173230_execution-plan.md
- packages/types/src/providers/mimo.ts
- packages/types/src/telemetry.ts
- docs/260730_0001_session_branch-cleanup/184700_debug-report.md
- webview-ui/src/i18n/locales/ca/settings.json
- webview-ui/src/i18n/locales/ja/settings.json
- webview-ui/src/i18n/locales/ru/settings.json
- webview-ui/src/i18n/locales/it/settings.json
- webview-ui/src/i18n/locales/es/settings.json
- src/api/providers/tests/base-provider.spec.ts
- src/core/assistant-message/ToolCallRetentionPolicy.ts
- packages/telemetry/src/TelemetryService.ts
- src/core/task/Task.ts
- src/core/assistant-message/NativeToolCallParser.ts
- webview-ui/src/i18n/locales/fr/settings.json
- src/api/providers/mimo.ts
- webview-ui/src/i18n/locales/zh-CN/settings.json
- webview-ui/src/i18n/locales/zh-TW/settings.json
| ### Verification evidence | ||
| | Check | Result | | ||
| |---|---| | ||
| | `git log feature/local-usage-stats --not main` contamination scan | No terminal/shell/error-interception/mimo-feature/strict/task-dnd commits | | ||
| | Symbol grep for mimo/shell markers | 0 matches | | ||
| | `pnpm check-types` (turbo, 14 packages) | **11 successful, exit 0** | | ||
| | Backend stats: `UsageAggregator.spec` + `UsageStatsStreamCoordinator.spec` | **114 passed** | | ||
| | Backend wiring: `usageStatsMessageHandler.spec` + `usageStatsMessageRouting.spec` | **72 passed** | | ||
| | Webview: `src/components/dashboard/` | **120 passed (7 files)** | | ||
|
|
||
| ## Test Environment Issues (fixed / worked around) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add blank lines around the verification table.
markdownlint reports MD058 because the table starts directly after the heading and ends directly before the next heading. Add one blank line before and after the table.
🧰 Tools
🪛 markdownlint-cli2 (0.23.2)
[warning] 50-50: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/260730_0001_session_branch-cleanup/170000_debug-report.md` around lines
49 - 59, Add blank lines immediately before and after the verification table in
the “Verification evidence” section of the debug report, separating it from both
headings to satisfy markdownlint MD058.
Source: Linters/SAST tools
| ``` | ||
| 0ead76de7 feat(terminal): add unified shell resolution system | ||
| 71a85444f fix(terminal): add logging to silent error paths in shell resolution | ||
| 8e6799525 feat(terminal): port CommandScheduler and Shell abstraction from Zoo-Code/ | ||
| 3947666f0 chore(unified-shell-resolution): remove non-feature report files for PR readiness | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add language identifiers to fenced code blocks.
MD040 reports untagged fences at Lines 37, 46, 74, 101, 115, 123, 172, and 239. Add text to commit-list fences and powershell to command fences.
Also applies to: 45-63, 74-95, 101-112, 115-119, 123-167, 172-176, 239-246
🧰 Tools
🪛 markdownlint-cli2 (0.23.2)
[warning] 37-37: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/260730_0001_session_branch-cleanup/181500_debug-dnd-ux-runbook.md`
around lines 37 - 42, Update the fenced code blocks in the runbook, including
the commit-list block near the shown hashes and the additional referenced
sections, with explicit language identifiers: use text for commit listings and
powershell for command examples. Ensure every reported untagged fence is labeled
without changing its contents.
Source: Linters/SAST tools
| "core/task/__tests__/Task.spec.ts": { | ||
| "@typescript-eslint/no-explicit-any": { | ||
| "count": 31 | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Remove the new ESLint suppression baseline.
Lines 847-850 add 31 @typescript-eslint/no-explicit-any suppressions for core/task/__tests__/Task.spec.ts. This increases the manifest baseline and hides violations instead of fixing them. Replace unsafe test values with precise test doubles or unknown plus type guards, then remove this entry.
After the change, run pnpm --dir src exec eslint --prune-suppressions --max-warnings=0 core/task/__tests__/Task.spec.ts and confirm that the suppression count does not increase.
As per coding guidelines, “Suppression counts in src/eslint-suppressions.json must never increase; when touching a file, reduce its count when the fix is local and low-risk.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/eslint-suppressions.json` around lines 847 - 850, Remove the new
`@typescript-eslint/no-explicit-any` baseline entry for
core/task/__tests__/Task.spec.ts from src/eslint-suppressions.json, and replace
the test’s unsafe any values with precise test doubles or unknown values
narrowed by type guards. Update the relevant tests in Task.spec.ts, then run
ESLint with suppression pruning and ensure the file’s suppression count does not
increase.
Source: Coding guidelines
github-actions
Bot
removed
the
has-conflicts
github-actions
Bot
added
the
awaiting-review
1 participant
Stack Position
fix/mimo-parallel-tool-call-policyDescription
Full Feature Description
fix/mimo-parallel-tool-call-policymimo.ts,mimo.ts,NativeToolCallParser.ts,ToolCallRetentionPolicy.ts, task policy wiring, andTelemetryService.ts. B05a is shared between this chain and theopenai-compatible-strict-reasoningchain.Why Split Into 17 PRs
Instead of submitting this feature as a single unified PR, it was split into individual PRs because as code size grows, safely reviewing a PR becomes very difficult. The feature was broken into mutually exclusive individual PRs so that each can be reviewed independently.
What This PR Specifically Changes
Adds MiMo capability/request conversion, stream parser, ghost quarantine, malformed call retention, max-one execution guard, and payload-free telemetry. Does not change other provider behavior or cost calculation.
Included Files
packages/types/src/providers/mimo.tspackages/telemetry/src/TelemetryService.tssrc/api/providers/mimo.tssrc/core/assistant-message/NativeToolCallParser.tssrc/core/assistant-message/ToolCallRetentionPolicy.tsExclusion Scope
Summary by CodeRabbit
New Features
Bug Fixes
Privacy