Repository navigation
fix(sdk): tool call repair, CLI abort handling, fallback provider (BZ-665, BZ-667, BZ-1341) - #946
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Caution Review failedPull request was closed or merged during review Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a CLI SIGINT-driven stream abort handler, implements a schema-driven tool-call repair utility, wires the repair hook into BaseProvider and multiple providers, and extends StreamOptions with per-request fallback and disableToolCallRepair flags. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant Iterator
participant Provider
User->>CLI: start streaming request / or press Ctrl+C
CLI->>CLI: createStreamAbortHandler() -> provides AbortSignal
CLI->>Provider: start streamText(..., signal)
Provider->>Iterator: return async iterator of chunks
CLI->>Iterator: await next() (raced with abortPromise)
User->>CLI: SIGINT (Ctrl+C)
CLI->>CLI: abortController.abort() -> abortPromise rejects
CLI->>Iterator: pending next() unblocks (AbortError)
CLI->>Iterator: call streamIterator.return() (if present)
Iterator->>Provider: close stream/connection
CLI->>CLI: abortHandler.cleanup(), print newline, return partial content
sequenceDiagram
participant Provider
participant ToolCall
participant Repair
participant Schema
Provider->>ToolCall: detect tool-call error (NoSuchTool/InvalidToolInput)
ToolCall->>Repair: invoke experimental_repairToolCall(error, toolCall, tools, inputSchema)
Repair->>Repair: attempt name repair (exact, substring, Levenshtein)
Repair->>Schema: request inputSchema({ toolName })
Schema-->>Repair: schema or null
Repair->>Repair: reconcile keys, coerce types per schema
Repair-->>Provider: return repaired toolCall or null
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
There was a problem hiding this comment.
Pull request overview
This PR enhances the SDK/CLI robustness by adding schema-driven tool-call repair across providers, graceful CLI stream cancellation on SIGINT, and configurable fallback provider/model overrides for streaming.
Changes:
- Add schema-driven
experimental_repairToolCallimplementation and wire it into all providers (with an opt-out stream option). - Add CLI SIGINT→AbortController bridge to cancel streaming cleanly (and update streaming output handling).
- Add per-call/env overrides for fallback provider/model selection during retries.
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/lib/utils/toolCallRepair.ts | New schema-driven tool name/arg repair + type coercion for AI SDK tool calls |
| src/lib/core/baseProvider.ts | Adds getToolCallRepairFn() and lazy-import wiring for providers |
| src/lib/providers/openRouter.ts | Wires experimental_repairToolCall into OpenRouter calls |
| src/lib/providers/openaiCompatible.ts | Wires experimental_repairToolCall into OpenAI-compatible calls |
| src/lib/providers/openAI.ts | Wires experimental_repairToolCall into OpenAI calls |
| src/lib/providers/mistral.ts | Wires experimental_repairToolCall into Mistral calls |
| src/lib/providers/litellm.ts | Wires experimental_repairToolCall into LiteLLM calls |
| src/lib/providers/huggingFace.ts | Wires experimental_repairToolCall into HuggingFace calls |
| src/lib/providers/googleVertex.ts | Wires experimental_repairToolCall into Google Vertex calls |
| src/lib/providers/googleAiStudio.ts | Wires experimental_repairToolCall into Google AI Studio calls |
| src/lib/providers/azureOpenai.ts | Wires experimental_repairToolCall into Azure OpenAI calls |
| src/lib/providers/anthropicBaseProvider.ts | Wires experimental_repairToolCall into Anthropic V2 base provider |
| src/lib/providers/anthropic.ts | Wires experimental_repairToolCall into Anthropic calls |
| src/lib/types/streamTypes.ts | Adds disableToolCallRepair, fallbackProvider, fallbackModel to stream options |
| src/lib/types/configTypes.ts | Updates MCP cache documentation to reflect new default behavior |
| src/lib/neurolink.ts | Enables tool-result cache by default; adds fallback provider/model override logic; caches external MCP tool results |
| src/lib/core/modules/ToolsManager.ts | Adds tool output truncation guard and wraps tool execution paths |
| src/cli/utils/typewriter.ts | Adds animated stdout writing helper used by streaming output |
| src/cli/utils/abortHandler.ts | New SIGINT abort handler for CLI streaming |
| src/cli/factories/commandFactory.ts | Integrates abort handler + animatedWrite into streaming processing |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const called = toolCall.toolName; | ||
|
|
||
| // 1. Case-insensitive exact match | ||
| const ciMatch = availableTools.find( | ||
| (t) => t.toLowerCase() === called.toLowerCase(), | ||
| ); | ||
| if (ciMatch) { | ||
| logger.debug( | ||
| `[ToolCallRepair] Name repair (case): "${called}" → "${ciMatch}"`, | ||
| ); | ||
| return { ...toolCall, toolName: ciMatch }; | ||
| } | ||
|
|
||
| // 2. Substring match: "search_file" is substring of "search_files" or vice versa | ||
| const calledLower = called.toLowerCase(); | ||
| const subMatch = availableTools.find((t) => { | ||
| const tLower = t.toLowerCase(); | ||
| return tLower.includes(calledLower) || calledLower.includes(tLower); | ||
| }); |
There was a problem hiding this comment.
repairToolName() can incorrectly "repair" an empty tool name because substring matching treats "" as a substring of every tool name (so the first available tool would be chosen). Add an early guard (e.g., if toolName.trim().length === 0 return null) before attempting case/substring/Levenshtein matching.
| const mapped = findMatchingKey(inputKey, expectedKeys); | ||
| if (mapped) { | ||
| logger.debug( | ||
| `[ToolCallRepair] Param repair: "${inputKey}" → "${mapped}" (tool: ${toolCall.toolName})`, | ||
| ); | ||
| repaired[mapped] = inputObj[inputKey]; | ||
| didRepair = true; | ||
| } else { | ||
| // Unknown key — pass through (schema may allow additionalProperties) | ||
| repaired[inputKey] = inputObj[inputKey]; | ||
| } |
There was a problem hiding this comment.
repairToolInput() always passes through unknown keys when it can't map them. For schemas with additionalProperties: false (used for OpenAI strict mode elsewhere in the codebase), those extra keys will keep failing validation, so the repair can't succeed. Consider dropping unmapped keys when schema.additionalProperties === false (and marking didRepair = true), while still preserving them when additional properties are allowed.
| /** | ||
| * Coerce a value to match the expected schema type. | ||
| * Handles: string→number, JSON string→object, JSON string→array, value→[value]. | ||
| */ | ||
| function coerceType( | ||
| value: unknown, | ||
| propSchema: Record<string, unknown>, | ||
| ): unknown { | ||
| const expectedType = propSchema.type as string | undefined; | ||
| if (!expectedType || value === null || value === undefined) { | ||
| return value; | ||
| } | ||
|
|
||
| // String → Number | ||
| if (expectedType === "number" && typeof value === "string") { | ||
| const num = Number(value); | ||
| if (!isNaN(num)) { | ||
| return num; | ||
| } | ||
| } | ||
|
|
||
| // String → Integer | ||
| if (expectedType === "integer" && typeof value === "string") { | ||
| const num = parseInt(value, 10); | ||
| if (!isNaN(num)) { | ||
| return num; | ||
| } | ||
| } | ||
|
|
||
| // String → Boolean | ||
| if (expectedType === "boolean" && typeof value === "string") { | ||
| if (value.toLowerCase() === "true") { | ||
| return true; | ||
| } | ||
| if (value.toLowerCase() === "false") { | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| // JSON string → Object | ||
| if (expectedType === "object" && typeof value === "string") { | ||
| try { | ||
| const parsed = JSON.parse(value); | ||
| if (parsed && typeof parsed === "object" && !Array.isArray(parsed)) { | ||
| return parsed; | ||
| } | ||
| } catch { | ||
| // Not valid JSON — return as-is | ||
| } | ||
| } | ||
|
|
||
| // JSON string → Array | ||
| if (expectedType === "array" && typeof value === "string") { | ||
| try { | ||
| const parsed = JSON.parse(value); | ||
| if (Array.isArray(parsed)) { | ||
| return parsed; | ||
| } | ||
| } catch { | ||
| // Not valid JSON — return as-is | ||
| } | ||
| } | ||
|
|
||
| // Single value → Array (wrap) | ||
| if ( | ||
| expectedType === "array" && | ||
| !Array.isArray(value) && | ||
| typeof value !== "string" | ||
| ) { | ||
| return [value]; | ||
| } |
There was a problem hiding this comment.
The comment says "Single value → Array (wrap)" but the implementation explicitly excludes string values (typeof value !== "string"). Either include strings in the wrap logic or update the comment/behavior to avoid this mismatch.
| const DEFAULT_CHAR_DELAY_MS = 8; | ||
|
|
||
| /** | ||
| * Write text to stdout with a per-character typewriter animation. | ||
| * Falls back to raw write when delay is 0 or negative. | ||
| */ | ||
| export async function typewriterWrite( | ||
| text: string, | ||
| delayMs: number = DEFAULT_CHAR_DELAY_MS, | ||
| ): Promise<void> { | ||
| if (!text) { | ||
| return; | ||
| } | ||
| if (delayMs <= 0) { | ||
| process.stdout.write(text); | ||
| return; | ||
| } | ||
| // Use Array.from to handle surrogate pairs / emoji correctly | ||
| for (const ch of Array.from(text)) { | ||
| process.stdout.write(ch); | ||
| await new Promise((r) => setTimeout(r, delayMs)); | ||
| } |
There was a problem hiding this comment.
DEFAULT_CHAR_DELAY_MS = 8 combined with animatedWrite() awaiting typewriterWrite() will add ~8ms per character whenever stdout is a TTY. For typical streaming outputs this can slow the CLI by orders of magnitude and can cause upstream stream buffering/backpressure. Consider making animation opt-in (CLI flag/env), defaulting delay to 0, or animating per-chunk without awaiting per-character delays.
| // BZ-667: Wire SIGINT to abort stream gracefully | ||
| const abortHandler = createStreamAbortHandler(); | ||
| abortHandler.signal.addEventListener("abort", () => { | ||
| abortController.abort(); | ||
| }); |
There was a problem hiding this comment.
abortHandler.signal.addEventListener("abort", () => abortController.abort()) uses an anonymous listener that can't be removed in abortHandler.cleanup(). If processStreamWithTimeout() is called multiple times in one process, this can accumulate listeners. Consider using { once: true } and/or storing the handler function and removing it during cleanup.
| // BZ-664: ToolCache — enabled by default to prevent duplicate tool calls. | ||
| // Callers can explicitly opt out via mcp.cache.enabled = false. | ||
| if (mcpConfig?.cache?.enabled !== false) { | ||
| this.mcpToolResultCache = new ToolResultCache({ | ||
| ttl: mcpConfig.cache.ttl ?? 300_000, | ||
| maxSize: mcpConfig.cache.maxSize ?? 500, | ||
| strategy: mcpConfig.cache.strategy ?? "lru", | ||
| ttl: mcpConfig?.cache?.ttl ?? 300_000, | ||
| maxSize: mcpConfig?.cache?.maxSize ?? 500, | ||
| strategy: mcpConfig?.cache?.strategy ?? "lru", |
There was a problem hiding this comment.
The PR description/title focuses on BZ-665/BZ-667/BZ-1341, but this diff also changes MCP tool-result caching defaults (BZ-664: now enabled by default unless explicitly disabled). Since this is a behavior change that can affect callers (performance/side effects), it should be called out explicitly in the PR description/release notes and ideally behind a clearly documented config flag.
| export function createToolCallRepair(): ToolCallRepairFunction<ToolSet> { | ||
| return async ({ toolCall, tools, inputSchema, error }) => { | ||
| // Import error classes lazily to avoid circular deps at module level | ||
| const { NoSuchToolError: NoSuchTool, InvalidToolInputError: InvalidInput } = | ||
| await import("ai"); | ||
|
|
||
| if (NoSuchTool.isInstance(error)) { | ||
| return repairToolName(toolCall, Object.keys(tools)); | ||
| } | ||
|
|
||
| if (InvalidInput.isInstance(error)) { | ||
| try { | ||
| const schema = await inputSchema({ toolName: toolCall.toolName }); | ||
| return repairToolInput(toolCall, schema); | ||
| } catch { | ||
| // inputSchema() failed — can't repair without schema | ||
| return null; | ||
| } | ||
| } | ||
|
|
||
| return null; | ||
| }; |
There was a problem hiding this comment.
New schema-driven tool-call repair logic is non-trivial (name matching + param remapping + type coercion) and currently appears untested in the repo’s automated test suites. Adding focused unit tests for the repair behavior (e.g., additionalProperties:false handling, key mapping, coercions) would help prevent regressions across providers.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/lib/providers/googleAiStudio.ts (1)
647-662:⚠️ Potential issue | 🟠 MajorGemini 3's native tool path bypasses tool call repair logic.
The
experimental_repairToolCallhandler is only wired into the AI SDKstreamText()path (line 661). Gemini 3 + tools route throughexecuteNativeGemini3Stream()/executeNativeGemini3Generate()instead, which callbuildNativeToolDeclarations()and thenexecuteNativeToolCalls(). Neither of these functions apply any repair logic for wrong tool names or parameter schemas (BZ-665). Additionally, thedisableToolCallRepairoption is only checked ingetToolCallRepairFn(), which is never invoked in the native path, so that option cannot control repair behavior there.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/providers/googleAiStudio.ts` around lines 647 - 662, The native Gemini 3 tool path never applies the tool-call repair logic because getToolCallRepairFn() (and its disableToolCallRepair option) is only used with streamText(), so executeNativeGemini3Stream(), executeNativeGemini3Generate(), buildNativeToolDeclarations(), and executeNativeToolCalls() bypass repair; fix by wiring the same repair handler into the native flow—either accept and pass the repair function (this.getToolCallRepairFn(options)) into buildNativeToolDeclarations()/executeNativeToolCalls() or call that repair function inside executeNativeToolCalls() before executing a tool call, and ensure the disableToolCallRepair option is respected when choosing whether to apply repairs.src/lib/neurolink.ts (1)
6797-6832:⚠️ Potential issue | 🟠 MajorCentralize fallback-route resolution; this only fixes one stream fallback path.
The override merge lives only here.
handleStreamError()(Lines 7440-7454) still builds its fallback fromgetBestProvider(options.provider)and ignoresfallbackProvider/fallbackModel, so the new stream-level overrides will not apply when the primary provider throws. Also, if only the provider is overridden, Lines 6818-6820 keepmodelConfigRoute.model, which can hand the new provider an incompatible model. Please move this into a shared resolver and drop or recompute the model whenever the provider changes without an explicit model.Based on learnings: In neurolink stream fallback logging (src/lib/neurolink.ts:6191-6195), typical usage is to override both provider and model together, but logs should still accurately reflect one-sided overrides when only model or only provider is set.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/neurolink.ts` around lines 6797 - 6832, The fallback override merge is duplicated and incomplete: centralize the logic into a shared resolver (e.g., a new function like resolveFallbackRoute or reuse ModelRouter.getFallbackRoute) and replace the in-place merge at the current block and inside handleStreamError to call that resolver; ensure the resolver accepts (originalPrompt, enhancedOptions, providerName, strategy) and returns {provider, model, source} by applying optFallbackProvider/optFallbackModel, env FALLBACK_PROVIDER/FALLBACK_MODEL, then fall back to ModelRouter.getFallbackRoute; importantly, if the returned provider differs from modelConfigRoute.provider and no explicit model override was provided, drop or recompute the model from ModelRouter.getFallbackRoute (do not keep modelConfigRoute.model blindly) so provider-model incompatibilities are avoided; finally update the stream error logging to use the resolver’s source and accurately reflect one-sided overrides (only provider or only model) in fallbackSource.
🧹 Nitpick comments (2)
src/lib/core/baseProvider.ts (1)
1255-1268: Cache the repair factory inside the returned closure.On Line 1264,
createToolCallRepair()is rebuilt on every repair invocation. Cache it once pergetToolCallRepairFn()call to reduce repeated setup work.♻️ Proposed refactor
protected getToolCallRepairFn( options?: StreamOptions | TextGenerationOptions, ): ToolCallRepairFunction<ToolSet> | undefined { if ( (options as Record<string, unknown> | undefined)?.disableToolCallRepair ) { return undefined; } // Lazy import to avoid circular dependency at module load time - return (async (...args: Parameters<ToolCallRepairFunction<ToolSet>>) => { - const { createToolCallRepair } = - await import("../utils/toolCallRepair.js"); - return createToolCallRepair()(...args); - }) as ToolCallRepairFunction<ToolSet>; + let repairFnPromise: Promise<ToolCallRepairFunction<ToolSet>> | undefined; + return (async (...args: Parameters<ToolCallRepairFunction<ToolSet>>) => { + const repairFn = await (repairFnPromise ??= + import("../utils/toolCallRepair.js").then( + ({ createToolCallRepair }) => + createToolCallRepair() as ToolCallRepairFunction<ToolSet>, + )); + return repairFn(...args); + }) as ToolCallRepairFunction<ToolSet>; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/core/baseProvider.ts` around lines 1255 - 1268, getToolCallRepairFn currently re-imports and calls createToolCallRepair on every invocation of the returned async closure; change it to cache the produced repair factory once per getToolCallRepairFn call: inside getToolCallRepairFn, declare a local variable (e.g., repairFactory?: ReturnType<typeof createToolCallRepair>), then return an async closure that, on first call, does the lazy import of "../utils/toolCallRepair.js", sets repairFactory = createToolCallRepair(), and thereafter calls repairFactory(...args) for subsequent invocations; keep the same return type ToolCallRepairFunction<ToolSet> and preserve the disableToolCallRepair check and lazy import behavior.src/lib/utils/toolCallRepair.ts (1)
104-106: Guard debug-only serialization before constructing log payloads.At Line 105,
availableTools.join(", ")is computed even when debug logs are disabled.Suggested fix
- logger.debug( - `[ToolCallRepair] Could not repair tool name "${called}". Available: [${availableTools.join(", ")}]`, - ); + if (logger.shouldLog("debug")) { + logger.debug( + `[ToolCallRepair] Could not repair tool name "${called}". Available: [${availableTools.join(", ")}]`, + ); + }As per coding guidelines: "Always wrap expensive serialization operations with
logger.shouldLog("debug")guard before calling".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/utils/toolCallRepair.ts` around lines 104 - 106, The debug log currently computes availableTools.join(", ") unconditionally; wrap the expensive serialization with a debug guard: check logger.shouldLog("debug") before calling availableTools.join and only build the full message when true, then call logger.debug with the constructed string (use the same symbols: logger.debug, logger.shouldLog("debug"), availableTools.join, and called) so the join is not executed when debug logging is disabled.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cli/factories/commandFactory.ts`:
- Around line 2996-3000: The Ctrl+C handler currently only aborts a local
timeout (abortController) and is created after sdk.stream() resolves, so pending
streamIterator.next() calls never see the signal; fix by creating the abort
signal before calling sdk.stream() (use createStreamAbortHandler() early) and
pass that signal into sdk.stream(...) if the SDK supports it, otherwise wrap
each streamIterator.next() in a race with an abort promise and on abort call
streamIterator.return?.() to settle the pending read; update the abortHandler
listener to both abort the timeout controller and trigger
streamIterator.return?.() (or the SDK-provided cancel) so streaming stops
immediately.
In `@src/cli/utils/abortHandler.ts`:
- Around line 25-45: This file currently registers its own process.on("SIGINT")
and calls process.exit(0) inside sigintHandler, which conflicts with the
top-level signal coordinator; remove the internal process.on("SIGINT")
registration and the direct process.exit call and instead expose sigintHandler
(or provide attach/detach functions) so the top-level coordinator can call
sigintHandler to cancel the controller (using controller.abort()), manage
aborted/forceExitTimer state, and handle the "Stream cancelled." message; ensure
any existing callers (serve/loop/session/index) attach the handler centrally and
that forceExitTimer and aborted cleanup still occur when the coordinator invokes
the handler.
In `@src/cli/utils/typewriter.ts`:
- Around line 7-29: Change the typewriter path to be opt-in and abort-aware by
making the default behavior raw write: set DEFAULT_CHAR_DELAY_MS to 0 (or make
delayMs default to 0) so typewriterWrite only animates when a positive delay is
explicitly provided; add an optional AbortSignal parameter (e.g., signal?:
AbortSignal) to typewriterWrite and any caller like animatedWrite, check
signal?.aborted before starting and inside the character loop, and break/return
(or throw an AbortError) immediately if aborted so Ctrl+C can cancel
mid-animation; retain the existing Array.from(text) iteration and per-char delay
logic but replace await new Promise(r => setTimeout(r, delayMs)) with an
abort-aware wait (check signal before/after sleep) or short-circuit to avoid
hanging.
In `@src/lib/core/modules/ToolsManager.ts`:
- Around line 103-137: The issue is that returning nextObj immediately after
truncating content/data skips the subsequent whole-object JSON size check;
instead of returning from ToolsManager after setting nextObj, assign the
truncated object back into the variable used for the final size check (e.g.,
replace or set result = nextObj) and let execution fall through to the existing
JSON serialization/Buffer.byteLength logic so the 50KB budget check (and any
further trimming logic) runs on the full object; keep using
generateToolOutputPreview for per-field trimming but ensure the final
JSON.stringify uses the updated object (nextObj/result) before comparing to
51_200 bytes.
- Around line 133-150: The fallback currently replaces the tool result with only
{_truncated, _originalSize, _preview}, breaking callers expecting the tool's
original fields; change the return from ToolsManager's JSON-size branch to
preserve the original structured fields by merging them with the metadata (e.g.,
return { ...result, _truncated: true, _originalSize: originalSize, _preview:
preview }) so callers can still read fields like those produced by the tool; use
the same symbols generateToolOutputPreview, toolName, logger, and result to
locate the logic and ensure the preview and metadata are added under sentinel
names to avoid name collisions, or alternatively move this truncation logic out
of ToolsManager into the model-facing serialization layer if you prefer not to
ship the preview with the full object.
In `@src/lib/neurolink.ts`:
- Around line 10950-10954: getToolAnnotationsForExecution currently reads from
this.toolCache and then falls back to inferAnnotations, which can miss a
server-supplied destructiveHint; update it to first consult
externalServerManager.getServerTools(serverId) for the concrete tool metadata
(match by toolName and serverId) and use any server-provided destructiveHint
before consulting this.toolCache or calling inferAnnotations; ensure the cache
check that computes cacheEnabled (uses mcpToolResultCache and
_disableToolCacheForCurrentRequest) uses the annotations returned by the new
precedence so a server-marked destructive tool will disable caching immediately.
- Around line 10982-10985: The current cache branch unconditionally calls
this.mcpToolResultCache.cacheResult(toolName, cacheKeyArgs, result) which will
persist failed external-tool responses; change it to only cache successful
executions by checking the result flags used by handleSuccessfulToolExecution
(e.g. require result.success === true and result.isError !== true, or the
equivalent success predicate used later) before calling cacheResult, so only
truly successful tool responses are stored.
In `@src/lib/utils/toolCallRepair.ts`:
- Around line 145-163: A fuzzy-mapped key can overwrite a value that already
matched exactly earlier; in the loop inside the tool args repair logic (see
inputKeys, expectedKeys, findMatchingKey, repaired, inputObj, toolCall,
didRepair) add a guard before assigning repaired[mapped] so you do not overwrite
an existing repaired value: check if repaired already hasOwnProperty(mapped) (or
inputObj already provided the exact mapped key) and if so skip the fuzzy
assignment (optionally log a debug that the mapping was skipped); only assign
repaired[mapped] = inputObj[inputKey] and set didRepair = true when the target
key is not already present.
- Around line 241-247: The current coercion in the integer branch (inside the
function handling expectedType === "integer", using the local variable value and
num) uses parseInt which silently truncates inputs like "12abc" or "3.7";
replace it with a strict integer validation: first check that value is a string
and matches a whole-integer pattern (e.g. optional leading - and one or more
digits) before converting, then convert to a Number/parseInt and ensure
Number.isInteger on the result; only return the integer when the regex check and
integer check both pass, otherwise fall through (do not coerce).
---
Outside diff comments:
In `@src/lib/neurolink.ts`:
- Around line 6797-6832: The fallback override merge is duplicated and
incomplete: centralize the logic into a shared resolver (e.g., a new function
like resolveFallbackRoute or reuse ModelRouter.getFallbackRoute) and replace the
in-place merge at the current block and inside handleStreamError to call that
resolver; ensure the resolver accepts (originalPrompt, enhancedOptions,
providerName, strategy) and returns {provider, model, source} by applying
optFallbackProvider/optFallbackModel, env FALLBACK_PROVIDER/FALLBACK_MODEL, then
fall back to ModelRouter.getFallbackRoute; importantly, if the returned provider
differs from modelConfigRoute.provider and no explicit model override was
provided, drop or recompute the model from ModelRouter.getFallbackRoute (do not
keep modelConfigRoute.model blindly) so provider-model incompatibilities are
avoided; finally update the stream error logging to use the resolver’s source
and accurately reflect one-sided overrides (only provider or only model) in
fallbackSource.
In `@src/lib/providers/googleAiStudio.ts`:
- Around line 647-662: The native Gemini 3 tool path never applies the tool-call
repair logic because getToolCallRepairFn() (and its disableToolCallRepair
option) is only used with streamText(), so executeNativeGemini3Stream(),
executeNativeGemini3Generate(), buildNativeToolDeclarations(), and
executeNativeToolCalls() bypass repair; fix by wiring the same repair handler
into the native flow—either accept and pass the repair function
(this.getToolCallRepairFn(options)) into
buildNativeToolDeclarations()/executeNativeToolCalls() or call that repair
function inside executeNativeToolCalls() before executing a tool call, and
ensure the disableToolCallRepair option is respected when choosing whether to
apply repairs.
---
Nitpick comments:
In `@src/lib/core/baseProvider.ts`:
- Around line 1255-1268: getToolCallRepairFn currently re-imports and calls
createToolCallRepair on every invocation of the returned async closure; change
it to cache the produced repair factory once per getToolCallRepairFn call:
inside getToolCallRepairFn, declare a local variable (e.g., repairFactory?:
ReturnType<typeof createToolCallRepair>), then return an async closure that, on
first call, does the lazy import of "../utils/toolCallRepair.js", sets
repairFactory = createToolCallRepair(), and thereafter calls
repairFactory(...args) for subsequent invocations; keep the same return type
ToolCallRepairFunction<ToolSet> and preserve the disableToolCallRepair check and
lazy import behavior.
In `@src/lib/utils/toolCallRepair.ts`:
- Around line 104-106: The debug log currently computes availableTools.join(",
") unconditionally; wrap the expensive serialization with a debug guard: check
logger.shouldLog("debug") before calling availableTools.join and only build the
full message when true, then call logger.debug with the constructed string (use
the same symbols: logger.debug, logger.shouldLog("debug"), availableTools.join,
and called) so the join is not executed when debug logging is disabled.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 341eaed1-e1d9-4958-a58f-28ef0366ce0f
📒 Files selected for processing (20)
src/cli/factories/commandFactory.tssrc/cli/utils/abortHandler.tssrc/cli/utils/typewriter.tssrc/lib/core/baseProvider.tssrc/lib/core/modules/ToolsManager.tssrc/lib/neurolink.tssrc/lib/providers/anthropic.tssrc/lib/providers/anthropicBaseProvider.tssrc/lib/providers/azureOpenai.tssrc/lib/providers/googleAiStudio.tssrc/lib/providers/googleVertex.tssrc/lib/providers/huggingFace.tssrc/lib/providers/litellm.tssrc/lib/providers/mistral.tssrc/lib/providers/openAI.tssrc/lib/providers/openRouter.tssrc/lib/providers/openaiCompatible.tssrc/lib/types/configTypes.tssrc/lib/types/streamTypes.tssrc/lib/utils/toolCallRepair.ts
| const sigintHandler = () => { | ||
| if (aborted) { | ||
| // Second Ctrl+C — force exit | ||
| if (forceExitTimer) { | ||
| clearTimeout(forceExitTimer); | ||
| } | ||
| process.exit(0); | ||
| } | ||
|
|
||
| aborted = true; | ||
| controller.abort(); | ||
| process.stderr.write(chalk.yellow("\nStream cancelled.\n")); | ||
|
|
||
| // Allow force exit on second Ctrl+C within 1 second | ||
| forceExitTimer = setTimeout(() => { | ||
| forceExitTimer = null; | ||
| }, 1000); | ||
| }; | ||
|
|
||
| process.on("SIGINT", sigintHandler); | ||
|
|
There was a problem hiding this comment.
Avoid owning another process-level SIGINT handler here.
src/cli/index.ts Lines 55-68 already exits on SIGINT, and src/cli/commands/serve.ts / src/cli/loop/session.ts install their own handlers too. Registering another process.on("SIGINT") here means the first Ctrl+C still races the existing exit paths instead of only canceling the active stream, while Line 31's process.exit(0) can bypass the async cleanup in src/cli/index.ts Lines 70-90. This needs one top-level signal coordinator instead of a second process-level owner inside the helper.
Also applies to: 46-52
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cli/utils/abortHandler.ts` around lines 25 - 45, This file currently
registers its own process.on("SIGINT") and calls process.exit(0) inside
sigintHandler, which conflicts with the top-level signal coordinator; remove the
internal process.on("SIGINT") registration and the direct process.exit call and
instead expose sigintHandler (or provide attach/detach functions) so the
top-level coordinator can call sigintHandler to cancel the controller (using
controller.abort()), manage aborted/forceExitTimer state, and handle the "Stream
cancelled." message; ensure any existing callers (serve/loop/session/index)
attach the handler centrally and that forceExitTimer and aborted cleanup still
occur when the coordinator invokes the handler.
| const DEFAULT_CHAR_DELAY_MS = 8; | ||
|
|
||
| /** | ||
| * Write text to stdout with a per-character typewriter animation. | ||
| * Falls back to raw write when delay is 0 or negative. | ||
| */ | ||
| export async function typewriterWrite( | ||
| text: string, | ||
| delayMs: number = DEFAULT_CHAR_DELAY_MS, | ||
| ): Promise<void> { | ||
| if (!text) { | ||
| return; | ||
| } | ||
| if (delayMs <= 0) { | ||
| process.stdout.write(text); | ||
| return; | ||
| } | ||
| // Use Array.from to handle surrogate pairs / emoji correctly | ||
| for (const ch of Array.from(text)) { | ||
| process.stdout.write(ch); | ||
| await new Promise((r) => setTimeout(r, delayMs)); | ||
| } | ||
| } |
There was a problem hiding this comment.
Make the typewriter path opt-in and abort-aware.
At 8ms per character this caps streamed output at about 125 chars/sec, so a 4k-char response adds ~32s of artificial latency. Because commandFactory awaits animatedWrite(...), Ctrl+C also cannot stop the current chunk until this sleep loop finishes. Default to raw writes unless a delay is explicitly requested, and thread an AbortSignal through the loop so cancellation can cut animation short.
Possible direction
-const DEFAULT_CHAR_DELAY_MS = 8;
+const DEFAULT_CHAR_DELAY_MS = 0;
export async function typewriterWrite(
text: string,
delayMs: number = DEFAULT_CHAR_DELAY_MS,
+ signal?: AbortSignal,
): Promise<void> {
if (!text) {
return;
}
if (delayMs <= 0) {
process.stdout.write(text);
return;
}
// Use Array.from to handle surrogate pairs / emoji correctly
for (const ch of Array.from(text)) {
+ if (signal?.aborted) {
+ return;
+ }
process.stdout.write(ch);
- await new Promise((r) => setTimeout(r, delayMs));
+ await new Promise((resolve) => {
+ const timer = setTimeout(resolve, delayMs);
+ signal?.addEventListener(
+ "abort",
+ () => {
+ clearTimeout(timer);
+ resolve(undefined);
+ },
+ { once: true },
+ );
+ });
}
}
export async function animatedWrite(
text: string,
+ signal?: AbortSignal,
): Promise<void> {
if (shouldAnimate()) {
- await typewriterWrite(text);
+ await typewriterWrite(text, DEFAULT_CHAR_DELAY_MS, signal);
} else {
process.stdout.write(text);
}
}Also applies to: 41-46
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cli/utils/typewriter.ts` around lines 7 - 29, Change the typewriter path
to be opt-in and abort-aware by making the default behavior raw write: set
DEFAULT_CHAR_DELAY_MS to 0 (or make delayMs default to 0) so typewriterWrite
only animates when a positive delay is explicitly provided; add an optional
AbortSignal parameter (e.g., signal?: AbortSignal) to typewriterWrite and any
caller like animatedWrite, check signal?.aborted before starting and inside the
character loop, and break/return (or throw an AbortError) immediately if aborted
so Ctrl+C can cancel mid-animation; retain the existing Array.from(text)
iteration and per-char delay logic but replace await new Promise(r =>
setTimeout(r, delayMs)) with an abort-aware wait (check signal before/after
sleep) or short-circuit to avoid hanging.
| // Truncate "content" if present and oversized | ||
| if (typeof obj.content === "string") { | ||
| const { preview, truncated, originalSize } = generateToolOutputPreview( | ||
| obj.content, | ||
| ); | ||
| if (truncated) { | ||
| logger.debug( | ||
| `[ToolsManager] Truncated '${toolName}' content field: ${originalSize} bytes → ${Buffer.byteLength(preview, "utf-8")} bytes`, | ||
| ); | ||
| nextObj = { ...(nextObj ?? obj), content: preview }; | ||
| } | ||
| } | ||
|
|
||
| // Truncate "data" if present and oversized — both fields can coexist | ||
| if (typeof obj.data === "string") { | ||
| const { preview, truncated, originalSize } = generateToolOutputPreview( | ||
| obj.data, | ||
| ); | ||
| if (truncated) { | ||
| logger.debug( | ||
| `[ToolsManager] Truncated '${toolName}' data field: ${originalSize} bytes → ${Buffer.byteLength(preview, "utf-8")} bytes`, | ||
| ); | ||
| nextObj = { ...(nextObj ?? obj), data: preview }; | ||
| } | ||
| } | ||
|
|
||
| if (nextObj) { | ||
| return nextObj; | ||
| } | ||
|
|
||
| // For other objects, check if their JSON serialization is too large. | ||
| // Use UTF-8 byte length, not string length, to match the 50KB budget. | ||
| try { | ||
| const jsonStr = JSON.stringify(result); | ||
| if (Buffer.byteLength(jsonStr, "utf-8") > 51_200) { |
There was a problem hiding this comment.
Field-level trimming skips the final size budget check.
nextObj is returned immediately after content / data truncation, so the whole-object check below never runs. A payload with both large content and data fields can still end up well over the 50 KB budget after each field was individually shortened.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/core/modules/ToolsManager.ts` around lines 103 - 137, The issue is
that returning nextObj immediately after truncating content/data skips the
subsequent whole-object JSON size check; instead of returning from ToolsManager
after setting nextObj, assign the truncated object back into the variable used
for the final size check (e.g., replace or set result = nextObj) and let
execution fall through to the existing JSON serialization/Buffer.byteLength
logic so the 50KB budget check (and any further trimming logic) runs on the full
object; keep using generateToolOutputPreview for per-field trimming but ensure
the final JSON.stringify uses the updated object (nextObj/result) before
comparing to 51_200 bytes.
| // For other objects, check if their JSON serialization is too large. | ||
| // Use UTF-8 byte length, not string length, to match the 50KB budget. | ||
| try { | ||
| const jsonStr = JSON.stringify(result); | ||
| if (Buffer.byteLength(jsonStr, "utf-8") > 51_200) { | ||
| const { preview, truncated, originalSize } = | ||
| generateToolOutputPreview(jsonStr); | ||
| if (truncated) { | ||
| logger.debug( | ||
| `[ToolsManager] Truncated '${toolName}' JSON output: ${originalSize} bytes → ${Buffer.byteLength(preview, "utf-8")} bytes`, | ||
| ); | ||
| // Preserve object shape so callers reading structured fields don't | ||
| // get a type surprise. Attach the preview under a sentinel field. | ||
| return { | ||
| _truncated: true, | ||
| _originalSize: originalSize, | ||
| _preview: preview, | ||
| }; |
There was a problem hiding this comment.
The JSON fallback still breaks structured tool outputs.
This branch replaces the original payload with { _truncated, _originalSize, _preview }, so any caller that reads fields from the real tool result gets a different object once the JSON-size path is hit. If the preview must travel downstream, keep the original fields alongside the metadata or move truncation to the model-facing serialization layer instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/core/modules/ToolsManager.ts` around lines 133 - 150, The fallback
currently replaces the tool result with only {_truncated, _originalSize,
_preview}, breaking callers expecting the tool's original fields; change the
return from ToolsManager's JSON-size branch to preserve the original structured
fields by merging them with the metadata (e.g., return { ...result, _truncated:
true, _originalSize: originalSize, _preview: preview }) so callers can still
read fields like those produced by the tool; use the same symbols
generateToolOutputPreview, toolName, logger, and result to locate the logic and
ensure the preview and metadata are added under sentinel names to avoid name
collisions, or alternatively move this truncation logic out of ToolsManager into
the model-facing serialization layer if you prefer not to ship the preview with
the full object.
| const toolAnnotations = this.getToolAnnotationsForExecution(toolName); | ||
| const cacheEnabled = | ||
| !!this.mcpToolResultCache && | ||
| !this._disableToolCacheForCurrentRequest && | ||
| !toolAnnotations?.destructiveHint; |
There was a problem hiding this comment.
Resolve destructive annotations from the concrete external tool, not the cache helper.
getToolAnnotationsForExecution(toolName) only checks this.toolCache and then falls back to inferAnnotations({ name, description: "" }). On a cold cache, a server-supplied destructiveHint: true can be missed, and src/lib/mcp/toolAnnotations.ts:198-220 only infers destructive tools from keywords. That means the first call can still be cached even when the external server explicitly marked the tool unsafe to cache. Prefer the { serverId, toolName } metadata from externalServerManager.getServerTools(serverId) before falling back to inference.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/neurolink.ts` around lines 10950 - 10954,
getToolAnnotationsForExecution currently reads from this.toolCache and then
falls back to inferAnnotations, which can miss a server-supplied
destructiveHint; update it to first consult
externalServerManager.getServerTools(serverId) for the concrete tool metadata
(match by toolName and serverId) and use any server-provided destructiveHint
before consulting this.toolCache or calling inferAnnotations; ensure the cache
check that computes cacheEnabled (uses mcpToolResultCache and
_disableToolCacheForCurrentRequest) uses the annotations returned by the new
precedence so a server-marked destructive tool will disable caching immediately.
| // BZ-664: Store result in cache after successful execution | ||
| if (cacheEnabled && this.mcpToolResultCache) { | ||
| this.mcpToolResultCache.cacheResult(toolName, cacheKeyArgs, result); | ||
| } |
There was a problem hiding this comment.
Only cache successful external-tool responses.
This branch caches every resolved payload, but handleSuccessfulToolExecution() later in this file (Lines 8872-8944) treats success: false / isError: true results as failures. Caching those here will pin transient MCP errors and short-circuit later retries.
Suggested fix
- // BZ-664: Store result in cache after successful execution
- if (cacheEnabled && this.mcpToolResultCache) {
- this.mcpToolResultCache.cacheResult(toolName, cacheKeyArgs, result);
- }
+ // BZ-664: Store only successful results in cache
+ const resultRecord =
+ result && typeof result === "object"
+ ? (result as Record<string, unknown>)
+ : undefined;
+ const isSuccessfulResult =
+ !resultRecord ||
+ (resultRecord.isError !== true && resultRecord.success !== false);
+ if (cacheEnabled && this.mcpToolResultCache && isSuccessfulResult) {
+ this.mcpToolResultCache.cacheResult(toolName, cacheKeyArgs, result);
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/neurolink.ts` around lines 10982 - 10985, The current cache branch
unconditionally calls this.mcpToolResultCache.cacheResult(toolName,
cacheKeyArgs, result) which will persist failed external-tool responses; change
it to only cache successful executions by checking the result flags used by
handleSuccessfulToolExecution (e.g. require result.success === true and
result.isError !== true, or the equivalent success predicate used later) before
calling cacheResult, so only truly successful tool responses are stored.
d34903a to
af3f940
Compare
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
af3f940 to
71e3394
Compare
✅ Single Commit Policy - COMPLIANTStatus: Policy requirements met • 1 commit • Valid format • Ready for merge 📊 View validation details📝 Commit Details
✅ Validation Results
🤖 Automated validation by NeuroLink Single Commit Enforcement |
Review Feedback AddressedChanges Made
Comments NOT in our scope (pre-existing code, not part of BZ-665/667/1341):
Validation
@coderabbitai review |
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
|
The out-of-scope justifications are well-reasoned — BZ-664 items belong to their own ticket, and the E2E test coverage in Triggering a fresh review now. 🧠 Learnings used✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/cli/factories/commandFactory.ts (1)
2996-3004:⚠️ Potential issue | 🟠 MajorCreate the SIGINT bridge before
sdk.stream()starts.This handler is still created inside
processStreamWithTimeout(), so the firstCtrl+Ccannot cancel stalls whileexecuteRealStream()is still awaitingsdk.stream(...)itself. That leaves auth/setup/connect hangs outside the new graceful-abort path; movecreateStreamAbortHandler()up intoexecuteRealStream()and thread its signal through thesdk.stream(...)call path as well.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/factories/commandFactory.ts` around lines 2996 - 3004, Move creation of the SIGINT bridge out of processStreamWithTimeout() into executeRealStream(): call createStreamAbortHandler() at the start of executeRealStream(), keep its AbortSignal and add it to sdk.stream(...) call chain (pass it into executeRealStream -> processStreamWithTimeout -> sdk.stream or into any intermediate request options) so the first Ctrl+C triggers the same abort signal used by abortController.abort(); remove the duplicate handler in processStreamWithTimeout and ensure the created signal is wired through all places that call or await sdk.stream(...) so auth/setup/connect stalls can be cancelled immediately.
🧹 Nitpick comments (1)
src/lib/neurolink.ts (1)
6922-6927: Make fallback source logging precise for mixed-source overrides.
fallbackSourcecurrently collapses mixed cases into"options"or"env"even when provider/model come from different sources. Logging provider/model source separately (or emitting"mixed") will make diagnostics more accurate.♻️ Suggested logging refinement
- fallbackSource: - optFallbackProvider || optFallbackModel - ? "options" - : envFallbackProvider || envFallbackModel - ? "env" - : "model_config", + fallbackProviderSource: optFallbackProvider + ? "options" + : envFallbackProvider + ? "env" + : "model_config", + fallbackModelSource: optFallbackModel + ? "options" + : envFallbackModel + ? "env" + : "model_config", + fallbackSource: + (optFallbackProvider + ? "options" + : envFallbackProvider + ? "env" + : "model_config") === + (optFallbackModel + ? "options" + : envFallbackModel + ? "env" + : "model_config") + ? (optFallbackProvider + ? "options" + : envFallbackProvider + ? "env" + : "model_config") + : "mixed",Based on learnings: “In neurolink stream fallback logging (src/lib/neurolink.ts handleStreamFallback), typical usage is to override both provider and model together, but logs should still accurately reflect one-sided overrides when only model or only provider is set.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/neurolink.ts` around lines 6922 - 6927, The current fallbackSource calculation in handleStreamFallback collapses provider/model origins into one value; instead determine provider and model origins separately using optFallbackProvider, envFallbackProvider and optFallbackModel, envFallbackModel (e.g., compute providerSource and modelSource) and then set fallbackSource to a precise combined value (either "mixed" when providerSource !== modelSource or a combined string like `${providerSource}/${modelSource}`) so logs accurately reflect one-sided overrides; update any consumers of fallbackSource accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cli/factories/commandFactory.ts`:
- Around line 3131-3134: The current cleanup calls abortController.abort(), then
awaits streamIterator.return?.(), which can itself throw and mask the original
stream error; change the sequence in the stream/error handling to preserve the
original error by: capture any original error/abort reason before cleanup, run
await streamIterator.return?.() inside its own try/catch so any exception from
streamIterator.return is caught/logged but does not replace the original error,
then call abortHandler.cleanup() and finally rethrow or return the original
error/result; follow the same pattern used in
src/lib/server/routes/claudeProxyRoutes.ts and update the code paths around
abortController.abort(), streamIterator.return?.(), and abortHandler.cleanup()
to ensure cleanup failures do not overwrite the original failure.
In `@src/lib/utils/toolCallRepair.ts`:
- Around line 250-256: In the branch that handles numeric coercion (where
expectedType === "number" and typeof value === "string"), trim the input string
first and reject empty/whitespace-only strings, then coerce using Number on the
trimmed string and only accept the result if it is finite (use isFinite on the
numeric result); i.e., use value.trim() to produce the input to Number(), check
trimmed !== "" and isFinite(num) before returning num (referencing expectedType,
value, and num in this block).
---
Duplicate comments:
In `@src/cli/factories/commandFactory.ts`:
- Around line 2996-3004: Move creation of the SIGINT bridge out of
processStreamWithTimeout() into executeRealStream(): call
createStreamAbortHandler() at the start of executeRealStream(), keep its
AbortSignal and add it to sdk.stream(...) call chain (pass it into
executeRealStream -> processStreamWithTimeout -> sdk.stream or into any
intermediate request options) so the first Ctrl+C triggers the same abort signal
used by abortController.abort(); remove the duplicate handler in
processStreamWithTimeout and ensure the created signal is wired through all
places that call or await sdk.stream(...) so auth/setup/connect stalls can be
cancelled immediately.
---
Nitpick comments:
In `@src/lib/neurolink.ts`:
- Around line 6922-6927: The current fallbackSource calculation in
handleStreamFallback collapses provider/model origins into one value; instead
determine provider and model origins separately using optFallbackProvider,
envFallbackProvider and optFallbackModel, envFallbackModel (e.g., compute
providerSource and modelSource) and then set fallbackSource to a precise
combined value (either "mixed" when providerSource !== modelSource or a combined
string like `${providerSource}/${modelSource}`) so logs accurately reflect
one-sided overrides; update any consumers of fallbackSource accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8df07219-25b9-4a5a-af44-a7a22cad1f74
📒 Files selected for processing (17)
src/cli/factories/commandFactory.tssrc/cli/utils/abortHandler.tssrc/lib/core/baseProvider.tssrc/lib/neurolink.tssrc/lib/providers/anthropic.tssrc/lib/providers/anthropicBaseProvider.tssrc/lib/providers/azureOpenai.tssrc/lib/providers/googleAiStudio.tssrc/lib/providers/googleVertex.tssrc/lib/providers/huggingFace.tssrc/lib/providers/litellm.tssrc/lib/providers/mistral.tssrc/lib/providers/openAI.tssrc/lib/providers/openRouter.tssrc/lib/providers/openaiCompatible.tssrc/lib/types/streamTypes.tssrc/lib/utils/toolCallRepair.ts
✅ Files skipped from review due to trivial changes (1)
- src/lib/providers/googleVertex.ts
🚧 Files skipped from review as they are similar to previous changes (10)
- src/lib/providers/huggingFace.ts
- src/lib/providers/openaiCompatible.ts
- src/lib/providers/litellm.ts
- src/lib/providers/mistral.ts
- src/lib/providers/openAI.ts
- src/lib/providers/googleAiStudio.ts
- src/lib/providers/anthropic.ts
- src/lib/providers/azureOpenai.ts
- src/lib/types/streamTypes.ts
- src/cli/utils/abortHandler.ts
71e3394 to
c016550
Compare
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
c016550 to
4fa41ed
Compare
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
Review Feedback Addressed — Round 2Fixes Applied (2 inline comments)
Dismissed with rationale (2 comments)
Previously addressed (Round 1 — 8 fixes)
Out of scope (pre-existing code, not part of BZ-665/667/1341)
Validation
@coderabbitai review |
|
Fixes applied — LGTM:
Dismissals — rationale accepted:
Triggering a fresh review now. ✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cli/factories/commandFactory.ts`:
- Around line 3054-3068: Declare nextResult with an explicit iterator result
type instead of implicit any: replace "let nextResult;" with a typed declaration
using Awaited<ReturnType<typeof streamIterator.next>> (e.g. let nextResult:
Awaited<ReturnType<typeof streamIterator.next>>>;) so TypeScript knows the shape
(.done and .value) returned by streamIterator.next() even when used inside
Promise.race with timeoutPromise and abortPromise; reference symbol: nextResult,
streamIterator.next, timeoutPromise, abortPromise.
In `@src/lib/utils/toolCallRepair.ts`:
- Around line 77-88: The substring-match branch currently uses
availableTools.find(...) which picks the first match and makes repairs
order-dependent; change it to collect all substring candidates (e.g., filter
into a candidates array using the same tLower.includes(calledLower) ||
calledLower.includes(tLower) logic) and only apply the repair (return updated
toolCall and logger.debug) if candidates.length === 1; if there are 0 or >1
candidates, do not modify toolCall and let the stricter match logic below handle
it. Ensure you reference calledLower, availableTools, subMatch (rename to
candidates or similar), toolCall, and logger.debug in the updated
implementation.
- Around line 147-180: The current repaired object is created with a plain {}
which allows keys like "__proto__" or "constructor" to be treated as prototype
properties and thus lost; change the initialization of repaired (the const
repaired: Record<string, unknown> = {} in toolCallRepair.ts) to use
Object.create(null) so it has no prototype (e.g., const repaired =
Object.create(null) as Record<string, unknown>), leaving the rest of the logic
(checks using Object.prototype.hasOwnProperty.call(repaired, mapped),
findMatchingKey(inputKey), expectedKeys, inputObj, toolCall.toolName) unchanged
so unmapped keys are forwarded safely when additionalProperties is allowed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ea936847-ce0f-4f89-b861-7160ed767932
📒 Files selected for processing (17)
src/cli/factories/commandFactory.tssrc/cli/utils/abortHandler.tssrc/lib/core/baseProvider.tssrc/lib/neurolink.tssrc/lib/providers/anthropic.tssrc/lib/providers/anthropicBaseProvider.tssrc/lib/providers/azureOpenai.tssrc/lib/providers/googleAiStudio.tssrc/lib/providers/googleVertex.tssrc/lib/providers/huggingFace.tssrc/lib/providers/litellm.tssrc/lib/providers/mistral.tssrc/lib/providers/openAI.tssrc/lib/providers/openRouter.tssrc/lib/providers/openaiCompatible.tssrc/lib/types/streamTypes.tssrc/lib/utils/toolCallRepair.ts
✅ Files skipped from review due to trivial changes (4)
- src/lib/providers/huggingFace.ts
- src/lib/providers/mistral.ts
- src/lib/providers/anthropicBaseProvider.ts
- src/lib/providers/googleAiStudio.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- src/lib/providers/openRouter.ts
- src/lib/providers/azureOpenai.ts
- src/lib/providers/openAI.ts
- src/lib/providers/openaiCompatible.ts
- src/lib/providers/anthropic.ts
- src/lib/providers/litellm.ts
- src/cli/utils/abortHandler.ts
- src/lib/neurolink.ts
| let nextResult; | ||
|
|
||
| if (timeoutActive && !contentReceived) { | ||
| // Race between next chunk and timeout for first chunk only | ||
| // Race between next chunk, timeout, and abort signal | ||
| nextResult = await Promise.race([ | ||
| streamIterator.next(), | ||
| timeoutPromise, | ||
| abortPromise, | ||
| ]); | ||
| } else { | ||
| // No timeout for subsequent chunks | ||
| nextResult = await streamIterator.next(); | ||
| // Race between next chunk and abort signal | ||
| nextResult = await Promise.race([ | ||
| streamIterator.next(), | ||
| abortPromise, | ||
| ]); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Confirm the implicit-any declaration site
rg -n 'let nextResult;' src/cli/factories/commandFactory.ts
# Re-run Biome on the file to verify the lint finding
pnpm biome check src/cli/factories/commandFactory.tsRepository: juspay/neurolink
Length of output: 16099
🏁 Script executed:
# Get context around line 3054 to understand the function and streamIterator
sed -n '3000,3100p' src/cli/factories/commandFactory.ts | head -100Repository: juspay/neurolink
Length of output: 3535
🏁 Script executed:
# Find streamIterator declaration/usage to understand its type
rg -n 'streamIterator' src/cli/factories/commandFactory.ts | head -20Repository: juspay/neurolink
Length of output: 345
🏁 Script executed:
# Get the function signature containing line 3054
sed -n '2980,3060p' src/cli/factories/commandFactory.tsRepository: juspay/neurolink
Length of output: 3016
🏁 Script executed:
# Show how nextResult is used after line 3054
sed -n '3054,3120p' src/cli/factories/commandFactory.tsRepository: juspay/neurolink
Length of output: 2201
Type nextResult to avoid implicit any at Line 3054.
let nextResult; is implicitly any and triggers Biome noImplicitAnyLet. The variable is assigned the result of Promise.race([streamIterator.next(), ...]) and used with .done and .value properties, so it must have an explicit IteratorResult type.
💡 Recommended fix
- let nextResult;
+ let nextResult: IteratorResult<
+ | { content: string }
+ | { type: "audio" }
+ | { type: "image"; imageOutput: { base64: string } }
+ >;📝 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.
| let nextResult; | |
| if (timeoutActive && !contentReceived) { | |
| // Race between next chunk and timeout for first chunk only | |
| // Race between next chunk, timeout, and abort signal | |
| nextResult = await Promise.race([ | |
| streamIterator.next(), | |
| timeoutPromise, | |
| abortPromise, | |
| ]); | |
| } else { | |
| // No timeout for subsequent chunks | |
| nextResult = await streamIterator.next(); | |
| // Race between next chunk and abort signal | |
| nextResult = await Promise.race([ | |
| streamIterator.next(), | |
| abortPromise, | |
| ]); | |
| let nextResult: IteratorResult< | |
| | { content: string } | |
| | { type: "audio" } | |
| | { type: "image"; imageOutput: { base64: string } } | |
| >; | |
| if (timeoutActive && !contentReceived) { | |
| // Race between next chunk, timeout, and abort signal | |
| nextResult = await Promise.race([ | |
| streamIterator.next(), | |
| timeoutPromise, | |
| abortPromise, | |
| ]); | |
| } else { | |
| // Race between next chunk and abort signal | |
| nextResult = await Promise.race([ | |
| streamIterator.next(), | |
| abortPromise, | |
| ]); |
🧰 Tools
🪛 Biome (2.4.10)
[error] 3054-3054: This variable implicitly has the any type.
(lint/suspicious/noImplicitAnyLet)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cli/factories/commandFactory.ts` around lines 3054 - 3068, Declare
nextResult with an explicit iterator result type instead of implicit any:
replace "let nextResult;" with a typed declaration using
Awaited<ReturnType<typeof streamIterator.next>> (e.g. let nextResult:
Awaited<ReturnType<typeof streamIterator.next>>>;) so TypeScript knows the shape
(.done and .value) returned by streamIterator.next() even when used inside
Promise.race with timeoutPromise and abortPromise; reference symbol: nextResult,
streamIterator.next, timeoutPromise, abortPromise.
4fa41ed to
b7157a9
Compare
Review Feedback Addressed — Round 3Fixes Applied (2 inline comments)
Dismissed with rationale (1 inline comment)
RebasedBranch rebased on latest Cumulative fixes across all rounds
Validation
@coderabbitai review |
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
|
Fixes applied — LGTM:
Dismissal — rationale accepted:
The cumulative 12 fixes across 3 rounds represent thorough coverage of all raised concerns. Triggering a fresh review now. ✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/cli/factories/commandFactory.ts (1)
3054-3054:⚠️ Potential issue | 🟠 MajorType
nextResultexplicitly to avoidimplicit anylint failure.
let nextResult;at Line 3054 is still implicitanyand can fail Biome (noImplicitAnyLet) in this path.💡 Minimal fix
- let nextResult; + let nextResult: Awaited<ReturnType<typeof streamIterator.next>>;#!/bin/bash set -euo pipefail # Verify declaration site rg -n 'let nextResult;' src/cli/factories/commandFactory.ts # Verify lint status for this file pnpm biome check src/cli/factories/commandFactory.tsExpected result: no
lint/suspicious/noImplicitAnyLeterror at Line 3054.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/factories/commandFactory.ts` at line 3054, The local variable nextResult is declared without a type (let nextResult;) causing an implicit any lint failure; update its declaration to an explicit type that matches how it's assigned/used (e.g., the IteratorResult/Promise return type or a domain-specific type) — if unsure, use a safe typed placeholder like unknown or the appropriate union (e.g., IteratorResult<YourItemType, void> | undefined) and narrow it where used; modify the single declaration of nextResult in commandFactory.ts to include that explicit type and re-run the Biome check to ensure the noImplicitAnyLet error is resolved.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cli/utils/abortHandler.ts`:
- Around line 8-10: Update the doc comment to match the actual API usage:
replace the claim that the code uses `prependOnceListener` with accurate wording
that it uses `process.prependListener` (a persistent listener) and clarify that
the SIGINT handler remains registered until `cleanup()` removes it—so the first
Ctrl+C triggers graceful stream cancellation and only a subsequent rapid Ctrl+C
will exit; also update the similar text around the second occurrence (lines
~49-52) to the same wording.
In `@src/lib/utils/toolCallRepair.ts`:
- Around line 160-166: When findMatchingKey(inputKey, expectedKeys) returns
mapped but the canonical key is already present in repaired, don't silently
continue — mark that a repair occurred so didRepair becomes true; update the
logic in the loop inside toolCallRepair (the block that checks
Object.prototype.hasOwnProperty.call(repaired, mapped)) to set didRepair = true
(and optionally record the discarded fuzzy duplicate in a diagnostics/repairs
collection if one exists) before skipping the duplicate, so the function will
return the repaired version instead of null.
- Around line 91-108: The loop that selects a replacement tool uses raw edit
distance (bestScore = dist) to pick the winner but filters by normalized
distance, causing wrong picks when lengths differ; change the selection to track
and compare the normalized score instead (e.g., replace bestScore to represent
the lowest normalized value and compare normalized < bestScore) while still
ensuring candidates pass the normalized < 0.3 filter; update references to
bestScore/bestMatch in the block (variables: bestMatch, bestScore, dist,
normalized, calledLower, availableTools, toolCall) so the returned toolName is
the candidate with the smallest normalized distance rather than the smallest raw
dist.
---
Duplicate comments:
In `@src/cli/factories/commandFactory.ts`:
- Line 3054: The local variable nextResult is declared without a type (let
nextResult;) causing an implicit any lint failure; update its declaration to an
explicit type that matches how it's assigned/used (e.g., the
IteratorResult/Promise return type or a domain-specific type) — if unsure, use a
safe typed placeholder like unknown or the appropriate union (e.g.,
IteratorResult<YourItemType, void> | undefined) and narrow it where used; modify
the single declaration of nextResult in commandFactory.ts to include that
explicit type and re-run the Biome check to ensure the noImplicitAnyLet error is
resolved.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e6671b8c-3e35-4d1d-bb2e-f1b2f1257109
📒 Files selected for processing (17)
src/cli/factories/commandFactory.tssrc/cli/utils/abortHandler.tssrc/lib/core/baseProvider.tssrc/lib/neurolink.tssrc/lib/providers/anthropic.tssrc/lib/providers/anthropicBaseProvider.tssrc/lib/providers/azureOpenai.tssrc/lib/providers/googleAiStudio.tssrc/lib/providers/googleVertex.tssrc/lib/providers/huggingFace.tssrc/lib/providers/litellm.tssrc/lib/providers/mistral.tssrc/lib/providers/openAI.tssrc/lib/providers/openRouter.tssrc/lib/providers/openaiCompatible.tssrc/lib/types/streamTypes.tssrc/lib/utils/toolCallRepair.ts
✅ Files skipped from review due to trivial changes (4)
- src/lib/providers/openRouter.ts
- src/lib/providers/anthropicBaseProvider.ts
- src/lib/providers/openAI.ts
- src/lib/providers/googleAiStudio.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/lib/providers/azureOpenai.ts
- src/lib/providers/openaiCompatible.ts
- src/lib/providers/anthropic.ts
…ovider (BZ-665, BZ-667, BZ-1341)
BZ-665 — Schema-driven tool call repair via experimental_repairToolCall:
- Dynamic repair function reads JSON schema at repair time (zero static maps)
- Tool name repair: case-insensitive → substring → Levenshtein (< 30% normalized)
- Param name repair: case-insensitive → Levenshtein (≤ 2 edits) against schema properties
- Type coercion: string→number, string→boolean, JSON string→object/array per schema type
- Empty tool name guard to prevent false substring matches
- Strict integer validation (regex + isSafeInteger) to reject partial parses like "12abc"
- Drop unmapped keys when schema has additionalProperties: false
- Prevent remapped keys from overwriting already-valid canonical keys
- Wired into all 11 providers via BaseProvider.getToolCallRepairFn()
- Opt-out via disableToolCallRepair option on StreamOptions
BZ-667 — Graceful abort signal handling for CLI stream commands:
- createStreamAbortHandler() bridges SIGINT → AbortController
- Uses prependListener to fire before top-level SIGINT handler in cli/index.ts
- Abort promise races against streamIterator.next() to unblock pending reads
- streamIterator.return() called on abort to release provider connection
- First Ctrl+C cancels stream + returns partial content; second Ctrl+C force-exits
- Listener cleanup via { once: true } to prevent accumulation across calls
BZ-1341 — Fallback provider override via options and env vars:
- fallbackProvider/fallbackModel on StreamOptions for per-call override
- FALLBACK_PROVIDER/FALLBACK_MODEL env vars as second-priority fallback
- Empty string guard (trim || undefined) to treat "" as unset
- fallbackSource tracking: "options" > "env" > "model_config"
b7157a9 to
3510944
Compare
Review Feedback Addressed — Round 4Fixes Applied (3 inline comments)
Dismissed with rationale (1 duplicate comment)
Validation
@coderabbitai review |
|
Fixes applied — LGTM:
Dismissal — rationale accepted:
The cumulative 15 fixes across 4 rounds are comprehensive. Triggering a fresh review now. ✅ Actions performedReview triggered.
|
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/lib/utils/toolCallRepair.ts (1)
93-109:⚠️ Potential issue | 🟠 MajorTreat equal Levenshtein winners as ambiguous.
The normalized-score change fixed the raw-distance bias, but Line 99 still keeps the first tool when two candidates tie on the best score. That makes repair order-dependent again in a path that can execute the wrong tool.
Suggested fix
let bestMatch: string | null = null; let bestNormalized = Infinity; + let bestDistance = Infinity; + let ambiguous = false; for (const t of availableTools) { const dist = levenshtein(calledLower, t.toLowerCase()); const maxLen = Math.max(called.length, t.length); const normalized = maxLen === 0 ? 0 : dist / maxLen; - if (normalized < 0.3 && normalized < bestNormalized) { + if (normalized >= 0.3) { + continue; + } + if ( + normalized < bestNormalized || + (normalized === bestNormalized && dist < bestDistance) + ) { bestNormalized = normalized; + bestDistance = dist; bestMatch = t; + ambiguous = false; + } else if ( + normalized === bestNormalized && + dist === bestDistance + ) { + ambiguous = true; } } - if (bestMatch) { + if (bestMatch && !ambiguous) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/utils/toolCallRepair.ts` around lines 93 - 109, The current loop over availableTools can pick the first tool on equal normalized Levenshtein scores, making repair order-dependent; change the comparison logic in the loop that computes dist/normalized (using levenshtein, calledLower, availableTools) to detect ties using a small epsilon (e.g. 1e-6): only accept a candidate when normalized < bestNormalized - eps, and if Math.abs(normalized - bestNormalized) <= eps mark the result as ambiguous (e.g. set bestMatch to null or set an ambiguous flag). After the loop, only perform the repair and logger.debug (use bestNormalized and toolCall/toolName) when a unique bestMatch exists (not ambiguous); otherwise skip repair to avoid order-dependent selection.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/utils/toolCallRepair.ts`:
- Around line 226-236: The fuzzy-match loop in toolCallRepair.ts (variables
schemaKeys, best, bestDist, levenshtein, inputLower) silently keeps the first
key when a later key has the same edit distance; change the logic to detect ties
and return null in that case: introduce a tie flag (or set best = null) and when
you find dist < bestDist set best = k, bestDist = dist and clear the tie; when
you find dist === bestDist set best = null (or set tie = true) so tied distances
do not select a winner; at the end return best only if no tie (otherwise null),
preserving the ≤2 threshold.
---
Duplicate comments:
In `@src/lib/utils/toolCallRepair.ts`:
- Around line 93-109: The current loop over availableTools can pick the first
tool on equal normalized Levenshtein scores, making repair order-dependent;
change the comparison logic in the loop that computes dist/normalized (using
levenshtein, calledLower, availableTools) to detect ties using a small epsilon
(e.g. 1e-6): only accept a candidate when normalized < bestNormalized - eps, and
if Math.abs(normalized - bestNormalized) <= eps mark the result as ambiguous
(e.g. set bestMatch to null or set an ambiguous flag). After the loop, only
perform the repair and logger.debug (use bestNormalized and toolCall/toolName)
when a unique bestMatch exists (not ambiguous); otherwise skip repair to avoid
order-dependent selection.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f122fb96-0aba-46e8-afb8-1b0f182f19ce
📒 Files selected for processing (17)
src/cli/factories/commandFactory.tssrc/cli/utils/abortHandler.tssrc/lib/core/baseProvider.tssrc/lib/neurolink.tssrc/lib/providers/anthropic.tssrc/lib/providers/anthropicBaseProvider.tssrc/lib/providers/azureOpenai.tssrc/lib/providers/googleAiStudio.tssrc/lib/providers/googleVertex.tssrc/lib/providers/huggingFace.tssrc/lib/providers/litellm.tssrc/lib/providers/mistral.tssrc/lib/providers/openAI.tssrc/lib/providers/openRouter.tssrc/lib/providers/openaiCompatible.tssrc/lib/types/streamTypes.tssrc/lib/utils/toolCallRepair.ts
✅ Files skipped from review due to trivial changes (4)
- src/lib/providers/openAI.ts
- src/lib/providers/googleAiStudio.ts
- src/lib/providers/anthropicBaseProvider.ts
- src/lib/providers/googleVertex.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- src/lib/providers/openRouter.ts
- src/lib/providers/anthropic.ts
- src/lib/providers/azureOpenai.ts
- src/lib/providers/openaiCompatible.ts
- src/lib/types/streamTypes.ts
- src/lib/core/baseProvider.ts
- src/cli/utils/abortHandler.ts
| // Levenshtein — threshold ≤2 edits | ||
| let best: string | null = null; | ||
| let bestDist = Infinity; | ||
| for (const k of schemaKeys) { | ||
| const dist = levenshtein(inputLower, k.toLowerCase()); | ||
| if (dist <= 2 && dist < bestDist) { | ||
| bestDist = dist; | ||
| best = k; | ||
| } | ||
| } | ||
| return best; |
There was a problem hiding this comment.
Skip fuzzy param repair on tied distances.
Line 231 keeps the first schema key when two params are equally close. For short or similar names, that can silently rewrite the payload to the wrong argument instead of letting validation fail.
Suggested fix
let best: string | null = null;
let bestDist = Infinity;
+ let ambiguous = false;
for (const k of schemaKeys) {
const dist = levenshtein(inputLower, k.toLowerCase());
- if (dist <= 2 && dist < bestDist) {
+ if (dist > 2) {
+ continue;
+ }
+ if (dist < bestDist) {
bestDist = dist;
best = k;
+ ambiguous = false;
+ } else if (dist === bestDist) {
+ ambiguous = true;
}
}
- return best;
+ return ambiguous ? null : best;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/utils/toolCallRepair.ts` around lines 226 - 236, The fuzzy-match loop
in toolCallRepair.ts (variables schemaKeys, best, bestDist, levenshtein,
inputLower) silently keeps the first key when a later key has the same edit
distance; change the logic to detect ties and return null in that case:
introduce a tie flag (or set best = null) and when you find dist < bestDist set
best = k, bestDist = dist and clear the tie; when you find dist === bestDist set
best = null (or set tie = true) so tied distances do not select a winner; at the
end return best only if no tie (otherwise null), preserving the ≤2 threshold.
Review Feedback — Round 5Dismissed (1 comment, already resolved in prior rounds)
This comment describes the state of the code before Round 1 fixes. The current code already addresses both concerns:
No changes made this round. @coderabbitai review |
|
Triggering a fresh review now. ✅ Actions performedReview triggered.
|
|
🎉 This PR is included in version 9.54.3 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Summary
experimental_repairToolCall— dynamically fixes wrong tool names and parameter names from LLMs using the tool's JSON schema (no static alias maps). Wired into all 11 providers.fallbackProvider/fallbackModelstream options orFALLBACK_PROVIDER/FALLBACK_MODELenv vars (cherry-picked from PR feat(provider): Add support to pass fallback provider through stream … #880 with fixes).Changes
BZ-665: Tool Call Repair
src/lib/utils/toolCallRepair.ts: Levenshtein distance, type coercion (string→number, JSON string→object/array), schema-driven param remappingbaseProvider.ts:getToolCallRepairFn()protected method with lazy importexperimental_repairToolCallstreamTypes.ts:disableToolCallRepairopt-out optionBZ-667: CLI Abort Handling
src/cli/utils/abortHandler.ts: SIGINT→AbortController bridge (first Ctrl+C cancels, second force-exits)commandFactory.ts: Wire abort handler intoprocessStreamWithTimeout()BZ-1341: Fallback Provider
neurolink.ts: Override fallback provider/model via options or env vars with empty-string guard (.trim() || undefined)streamTypes.ts:fallbackProviderandfallbackModelfieldsTest plan
pnpm run check— 0 errors, 0 warningspnpm run lint— 0 errors (1 pre-existing warning)pnpm run build— successgenerate()andstream()with Vertex/Gemini 2.5 Flash — tools execute correctly, repair wired in 13 source filesneurolink stream+ SIGINT → "Stream cancelled." message confirmed/tmp/bz665-repro/and/tmp/bz667-repro/Summary by CodeRabbit
New Features
Improvements