fix: handle malformed JSON in OpenAI API responses #1
Conversation
- Add robust JSON parsing with extractPartialJson method for streaming responses - Handle incomplete JSON chunks from OpenAI function calls gracefully - Prevent 'invalid character' errors during API parsing - Add comprehensive error logging for troubleshooting - Fix missing mapGeminiFinishReasonToOpenAI method - Resolve TypeScript compilation issues
WalkthroughAdds resilient JSON parsing and streaming-edge handling across OpenAI/Gemini clients and format conversion; introduces CLI user-scoped MCP config management, migration script and guide, example settings, a JSON verification script, and two local package dependencies. Minor visibility changes: two methods had explicit Changes
Sequence Diagram(s)sequenceDiagram
rect rgb(240,248,255)
participant U as User/Caller
participant G as OpenAIContentGenerator
participant C as GeminiClient / OpenAIFormatConverter
participant P as PartialJsonExtractor
participant L as Logger
end
U->>G: send messages or streaming chunks
alt streaming chunk
G->>P: per-chunk parse attempt
alt chunk parse ok
P-->>G: parsed fragment
else Ollama chunk error
P-->>G: treat as recoverable -> continue
G-->>L: warn about chunk parse issue
else non-recoverable error
P-->>G: throw -> surface error
end
else non-streaming request
G->>C: request JSON generation
C->>P: safeJsonParse / extractPartialJson
alt full parse succeeds
P-->>C: parsed object
else partial extracted
P-->>C: partial object returned
C-->>L: warn and fallback where needed
else parse fails
C-->>G: error -> thrown
end
end
G->>G: cleanOrphanedToolCalls / merge messages
G-->>U: assembled response with safe tool-call args (or minimal fallback)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/core/src/core/openaiContentGenerator.ts(6 hunks)packages/core/src/utils/openaiFormatConverter.ts(6 hunks)
| args = JSON.parse(toolCall.function.arguments); | ||
| } catch (error) { | ||
| console.error('Failed to parse function arguments:', error); | ||
| args = {}; | ||
| console.error('Problematic arguments:', toolCall.function.arguments); | ||
| // Try to extract partial JSON or provide a fallback | ||
| args = this.extractPartialJson(toolCall.function.arguments) || {}; | ||
| } |
There was a problem hiding this comment.
Redact the tool-call payloads in error logs.
Both error paths now print the entire arguments string when JSON parsing fails. Those blobs routinely contain user data, so this change dumps PII straight into logs. Please log only metadata (e.g., message id/length) and apply the same redaction in the helper.
- console.error('Problematic arguments:', toolCall.function.arguments);
+ console.error(
+ 'Problematic tool arguments (length=%d)',
+ toolCall.function.arguments.length
+ );
…
- console.error('Problematic accumulated arguments:', accumulatedCall.arguments);
+ console.error(
+ 'Problematic accumulated tool arguments (length=%d)',
+ accumulatedCall.arguments.length
+ );Also redact the payload in extractPartialJson before returning the warning so we never emit the raw JSON.
Also applies to: 1274-1281
🤖 Prompt for AI Agents
In packages/core/src/core/openaiContentGenerator.ts around lines 1154-1160 (and
similarly 1274-1281), the code logs the full tool-call arguments when JSON.parse
fails; change those console.error calls to emit only safe metadata (e.g.,
message id, arguments length, and a short hash or truncated prefix) rather than
the raw payload, and replace the two raw-argument logs with a single metadata
log. Update the extractPartialJson helper so it never logs or returns the raw
JSON: when it cannot fully recover JSON, return a sanitized fallback (empty
object or redacted placeholder) and emit a warning that contains only metadata
(message id/length/placeholder) — do not include the original string. Ensure
both places use the same redaction utility function to produce consistent
metadata-only logs.
| // Last resort: return an empty object rather than throwing | ||
| console.warn('Could not extract valid JSON from:', input); |
There was a problem hiding this comment.
Apply the same redaction inside extractPartialJson.
This helper still logs the full input string on failure, undoing the redaction above. Please switch to a metadata-only message (length, hash, etc.) so we don’t leak request content.
- console.warn('Could not extract valid JSON from:', input);
+ console.warn(
+ 'Could not extract valid JSON from tool arguments (length=%d)',
+ input.length
+ );📝 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.
| // Last resort: return an empty object rather than throwing | |
| console.warn('Could not extract valid JSON from:', input); | |
| // Last resort: return an empty object rather than throwing | |
| console.warn( | |
| 'Could not extract valid JSON from tool arguments (length=%d)', | |
| input.length | |
| ); |
🤖 Prompt for AI Agents
In packages/core/src/core/openaiContentGenerator.ts around lines 1924-1925, the
extractPartialJson helper currently logs the full input on failure; change that
to avoid leaking request content by replacing the console.warn call with a
metadata-only log: compute and log the input length and a short cryptographic
hash (e.g., first 8-12 chars of a SHA-256 hex) plus a clear message and any
parse error details, but do NOT include the raw input string itself; ensure
dependencies used for hashing are already imported or use Node's crypto module.
| // Last resort: return an empty object rather than throwing | ||
| console.warn('Could not extract valid JSON from:', input); | ||
| return {}; |
There was a problem hiding this comment.
Stop logging raw tool-call payloads.
console.warn('…', input) dumps the entire argument blob (often user/PII) to stdout whenever parsing fails. That’s a privacy/compliance risk and will end up in server logs. Emit only a generic message (or length/ID), not the payload.
- console.warn('Could not extract valid JSON from:', input);
+ console.warn('Could not extract valid JSON from tool call arguments (payload redacted)');📝 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.
| // Last resort: return an empty object rather than throwing | |
| console.warn('Could not extract valid JSON from:', input); | |
| return {}; | |
| // Last resort: return an empty object rather than throwing | |
| console.warn('Could not extract valid JSON from tool call arguments (payload redacted)'); | |
| return {}; |
🤖 Prompt for AI Agents
In packages/core/src/utils/openaiFormatConverter.ts around lines 458 to 460, the
code currently calls console.warn('Could not extract valid JSON from:', input)
which prints the full input (possibly containing user/PII) to stdout; replace
this with a non-sensitive log that omits the raw payload — e.g., log a generic
message like "Could not extract valid JSON from tool call" and optionally
include a safe identifier such as the input length or a truncated/hashed ID (not
the full content), then return the empty object as before.
…tion - Fix TypeScript compilation errors in openaiFormatConverter.ts - Add missing type definitions and imports for OpenAI API compatibility - Resolve type predicate compatibility issues with FunctionCall/FunctionResponse - Add comprehensive JSDoc documentation to address CodeRabbit feedback: * Detailed descriptions for extractPartialJson() method * Complete documentation for safeJsonParse() with examples * Class-level documentation explaining utility purpose and usage * Method-level documentation with @example blocks and @see references - Improve docstring coverage from 0% to address CodeRabbit requirements - Ensure all JSON parsing improvements are now functional and deployable This resolves the original 'unmarshal: invalid character I' error by making the robust JSON parsing code actually build and deploy successfully.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
packages/core/src/core/openaiContentGenerator.ts (3)
1205-1230: Redact tool-call payloads from error logs.Line 1215 logs the full
toolCall.function.argumentsstring when JSON parsing fails. This data often contains user information or PII, creating a privacy and compliance risk as flagged in the previous review.Apply this diff to log only safe metadata:
- console.error('Problematic arguments:', toolCall.function.arguments); + console.error( + 'Problematic tool arguments (length=%d)', + toolCall.function.arguments.length + );
1321-1353: Redact accumulated tool arguments from error logs.Line 1336 logs the full
accumulatedCall.argumentswhen JSON parsing fails in the streaming path. This creates the same privacy risk as the non-streaming path and was flagged in the previous review.Apply this diff to log only metadata:
- console.error('Problematic accumulated arguments:', accumulatedCall.arguments); + console.error( + 'Problematic accumulated tool arguments (length=%d)', + accumulatedCall.arguments.length + );
2005-2007: Redact sensitive data from warning logs.Line 2006 logs both the raw
inputandfixedInputstrings, potentially exposing user data or PII twice. This was flagged in the previous review. Log only metadata such as input length and error details.Apply this diff to redact the sensitive data:
- console.warn('Could not extract valid JSON from:', input, 'Fixed version:', fixedInput); + console.warn( + 'Could not extract valid JSON from tool arguments (length=%d)', + input.length + );packages/core/src/utils/openaiFormatConverter.ts (1)
607-609: Redact sensitive data from error logs.This warning logs the raw input string which may contain user data or PII, creating a privacy and compliance risk. Per the previous review feedback, log only metadata such as input length instead of the full payload.
Apply this diff to redact the sensitive data:
- console.warn('Could not extract valid JSON from:', input); + console.warn('Could not extract valid JSON from tool call arguments (length=%d)', input.length);
🧹 Nitpick comments (2)
packages/core/src/core/openaiContentGenerator.ts (2)
143-153: Visibility modifiers: no functional change.Removing the explicit
publickeyword doesn't change access—these methods remain public in TypeScript (default visibility). If the intent was to restrict access, consider marking themprivateor documenting the intended scope.
1885-2008: Consider consolidating duplicate JSON parsing logic.This
extractPartialJsonimplementation is nearly identical to the one inopenaiFormatConverter.ts, but with subtle differences (returnsnullvs{}). Consolidating into a shared utility would reduce maintenance burden and ensure consistent behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/core/src/core/openaiContentGenerator.ts(8 hunks)packages/core/src/utils/openaiFormatConverter.ts(7 hunks)
🔇 Additional comments (7)
packages/core/src/utils/openaiFormatConverter.ts (4)
28-52: Type definitions restored successfully.The local type aliases (OpenAIToolCall, OpenAIMessage, OpenAIRequestFormat) are now properly defined and exported, which should resolve the TS2304 compilation errors flagged in the previous review.
86-124: LGTM!The tool conversion logic correctly transforms Gemini function declarations to OpenAI's expected format, with appropriate type guards.
144-172: LGTM!The type predicates and optional chaining provide robust handling of potentially undefined properties, preventing runtime errors during function call conversion.
426-476: LGTM!The safe JSON parsing method provides robust error handling with graceful fallback, ensuring the code never throws on malformed input. The comprehensive documentation clearly explains the use cases and behavior.
packages/core/src/core/openaiContentGenerator.ts (3)
926-1100: LGTM!The orphaned tool call cleanup logic is thorough and well-documented. The multi-pass validation ensures message consistency and prevents API errors from mismatched tool calls and responses.
1105-1186: LGTM!The message merging logic correctly consolidates consecutive assistant messages while preserving content order and tool calls. The comprehensive documentation clearly explains the use case and behavior.
1483-1508: LGTM!The finish reason mapping correctly handles both string and numeric Gemini finish reason values, with appropriate fallback to 'stop' for unknown cases.
- Replace direct JSON.parse() calls with robust extractPartialJson() method
- Handle malformed JSON from Ollama API streaming responses gracefully
- Fix streaming tool call argument parsing in convertToGeminiFormat()
- Fix accumulated streaming tool call parsing in streamGenerator
- Add comprehensive error logging for debugging JSON parsing issues
- Ensure API continues to work even when JSON parsing partially fails
- Fix TypeScript compilation errors with proper error type handling
Resolves: 'unmarshal: invalid character { after top-level value' errors
during Ollama API streaming responses
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
packages/core/src/core/openaiContentGenerator.ts (3)
1210-1216: Redact tool-call arguments from error logs to prevent PII leakage.The error logging at line 1215 prints the entire
toolCall.function.argumentsstring, which may contain sensitive user data (API keys, personal information, etc.). This should be replaced with metadata-only logging.Apply this diff:
const parsedArgs = this.extractPartialJson(toolCall.function.arguments); args = parsedArgs || {}; if (!parsedArgs) { - console.warn('Could not parse function arguments, using empty object:', toolCall.function.arguments); + console.warn( + 'Could not parse function arguments (length=%d), using empty object', + toolCall.function.arguments?.length || 0 + ); }
1327-1332: Redact accumulated tool-call arguments from error logs.The error logging at line 1331 prints the entire
accumulatedCall.argumentsstring, which may contain sensitive user data. Replace with metadata-only logging.Apply this diff:
const parsedArgs = this.extractPartialJson(accumulatedCall.arguments); args = parsedArgs || {}; if (!parsedArgs) { - console.warn('Could not parse accumulated tool call arguments, using empty object:', accumulatedCall.arguments); + console.warn( + 'Could not parse accumulated tool call arguments (length=%d), using empty object', + accumulatedCall.arguments?.length || 0 + ); }
1914-2006: Redact input strings from extractPartialJson error logs.The helper logs the full input string on parsing failures at lines 1956, 2000, and 2004. This could leak sensitive request content. Replace with metadata-only logging (length, short hash, etc.).
Apply this diff:
} catch (fixedError) { // If still fails, try to extract key-value pairs const errorMessage = fixedError instanceof Error ? fixedError.message : String(fixedError); - console.warn('Failed to parse fixed JSON input:', fixedInput, 'Error:', errorMessage); + console.warn('Failed to parse fixed JSON input (length=%d):', fixedInput.length, 'Error:', errorMessage); } // ... [middle section unchanged] ... } catch (singleQuoteError) { const errorMessage = singleQuoteError instanceof Error ? singleQuoteError.message : String(singleQuoteError); - console.warn('Failed to parse single-quote-fixed JSON:', singleQuoteFixed, 'Error:', errorMessage); + console.warn('Failed to parse single-quote-fixed JSON (length=%d):', singleQuoteFixed.length, 'Error:', errorMessage); } // Final fallback: return empty object rather than throwing to ensure API continues to work - console.warn('All JSON parsing methods failed. Could not extract valid JSON from:', input); + console.warn('All JSON parsing methods failed. Could not extract valid JSON (input length=%d)', input.length); return null;
🧹 Nitpick comments (1)
packages/cli/src/config/mcp-config.ts (1)
182-182: Minor formatting: missing newline before closing brace.There's a missing newline between the closing parenthesis and the closing brace of the function.
Apply this diff:
definition.includeTools, definition.excludeTools - );} + ); +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (8)
example-migrated-settings.json(1 hunks)json-fix-verification.js(1 hunks)mcp-migration-guide.md(1 hunks)migrate-mcp-config.sh(1 hunks)packages/cli/package.json(1 hunks)packages/cli/src/config/mcp-config.ts(1 hunks)packages/core/package.json(1 hunks)packages/core/src/core/openaiContentGenerator.ts(8 hunks)
✅ Files skipped from review due to trivial changes (1)
- example-migrated-settings.json
🧰 Additional context used
🧬 Code graph analysis (3)
json-fix-verification.js (1)
packages/core/src/core/openaiContentGenerator.ts (1)
extractPartialJson(1914-2006)
packages/cli/src/config/mcp-config.ts (1)
packages/core/src/config/config.ts (1)
MCPServerConfig(74-96)
packages/core/src/core/openaiContentGenerator.ts (1)
json-fix-verification.js (16)
trimmed(58-58)fixedInput(68-68)openBraces(75-75)closeBraces(76-76)openBrackets(77-77)closeBrackets(78-78)i(80-80)i(83-83)keyValuePattern(95-95)matches(96-96)result(99-99)result(162-162)key(102-102)value(103-103)rawValue(110-110)singleQuoteFixed(129-129)
🪛 LanguageTool
mcp-migration-guide.md
[grammar] ~22-~22: Ensure spelling is correct
Context: ...t ~/.config/claude/mcp.json ``` ### 2. Backup Your Ollama Code Settings Before making...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 Shellcheck (0.11.0)
migrate-mcp-config.sh
[warning] 112-112: Use single quotes, otherwise this expands now rather than when signalled.
(SC2064)
[warning] 112-112: Use single quotes, otherwise this expands now rather than when signalled.
(SC2064)
[warning] 112-112: Use single quotes, otherwise this expands now rather than when signalled.
(SC2064)
🔇 Additional comments (5)
json-fix-verification.js (1)
1-203: LGTM! Useful verification tool for JSON parsing improvements.This verification script effectively demonstrates and tests the JSON parsing robustness fixes. The test cases cover common malformed JSON scenarios (trailing commas, missing braces, incomplete streaming chunks) and the implementation mirrors the production code in
openaiContentGenerator.ts.mcp-migration-guide.md (1)
1-147: LGTM! Comprehensive migration guide.The MCP configuration migration guide is well-structured and provides clear step-by-step instructions for migrating from Claude Code to Ollama Code. It covers all necessary aspects including backup procedures, verification steps, and troubleshooting.
packages/core/src/core/openaiContentGenerator.ts (2)
1914-2006: Excellent JSON parsing resilience implementation.The
extractPartialJsonmethod provides robust handling of malformed JSON with multiple fallback strategies including trailing comma removal, brace/bracket balancing, manual key-value extraction, and quote normalization. This significantly improves the resilience of tool-call argument parsing in streaming scenarios.
954-1100: Well-implemented message cleanup helpers.The
cleanOrphanedToolCallsandmergeConsecutiveAssistantMessagesmethods are well-documented and properly handle edge cases in tool-call message sequences. The two-pass validation approach ensures no orphaned tool calls remain.Also applies to: 1135-1186
packages/cli/package.json (1)
32-32: Remove the self-referential dependency"@tcsenpai/ollama-code-cli": "file:"unless there's a specific reason for it.The dependency is confirmed at line 32 of packages/cli/package.json. This is an unusual pattern in monorepos where a package lists itself as a dependency with
"file:"(no path), referring to its own directory. While the npm install attempt failed due to a separate missing esbuild dependency issue, the self-reference itself is questionable and should be removed or explicitly documented if intentional. Self-referential dependencies can cause confusion during installation and build processes.
| TEMP_MERGED=$(mktemp) | ||
|
|
||
| # Clean up temp files on exit | ||
| trap "rm -f $TEMP_CLAUDE $TEMP_OLLAMA $TEMP_MERGED" EXIT |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Quote the trap command arguments to defer variable expansion.
The trap command arguments should be in single quotes to ensure the temp file variables are expanded when the trap fires, not when it's set. This is a common shell scripting pitfall.
Apply this diff:
- trap "rm -f $TEMP_CLAUDE $TEMP_OLLAMA $TEMP_MERGED" EXIT
+ trap 'rm -f "$TEMP_CLAUDE" "$TEMP_OLLAMA" "$TEMP_MERGED"' EXIT📝 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.
| trap "rm -f $TEMP_CLAUDE $TEMP_OLLAMA $TEMP_MERGED" EXIT | |
| trap 'rm -f "$TEMP_CLAUDE" "$TEMP_OLLAMA" "$TEMP_MERGED"' EXIT |
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 112-112: Use single quotes, otherwise this expands now rather than when signalled.
(SC2064)
[warning] 112-112: Use single quotes, otherwise this expands now rather than when signalled.
(SC2064)
[warning] 112-112: Use single quotes, otherwise this expands now rather than when signalled.
(SC2064)
🤖 Prompt for AI Agents
In migrate-mcp-config.sh around line 112, the trap command currently expands the
TEMP_* variables when the trap is set; to defer expansion until the trap runs,
wrap the entire trap command string in single quotes and put each temp-file
variable inside double quotes within that single-quoted string so the variables
are expanded at trap execution (e.g., change the trap line to use a
single-quoted command with "$TEMP_CLAUDE", "$TEMP_OLLAMA", and "$TEMP_MERGED"
double-quoted inside).
| "@opentelemetry/exporter-trace-otlp-grpc": "^0.52.0", | ||
| "@opentelemetry/instrumentation-http": "^0.52.0", | ||
| "@opentelemetry/sdk-node": "^0.52.0", | ||
| "@tcsenpai/ollama-code": "file:", |
There was a problem hiding this comment.
🧩 Analysis chain
Verify the self-referential dependency.
The package @tcsenpai/ollama-code is adding itself as a dependency with "file:". This creates a self-referential dependency which could cause circular dependency issues. Please verify this is intentional and necessary.
Run the following script to check if this causes installation issues:
🏁 Script executed:
#!/bin/bash
# Check if the package.json has circular dependency issues
cd packages/core
npm install --dry-run 2>&1 | grep -i "circular\|warn\|error" | head -20Length of output: 342
🏁 Script executed:
#!/bin/bash
# Inspect the packages/core/package.json to see actual package name and dependencies
echo "=== packages/core/package.json ==="
cat packages/core/package.json | head -50
echo -e "\n=== Search for all package.json in monorepo ==="
find . -name "package.json" -type f | grep -v node_modules | sort
echo -e "\n=== Look for @tcsenpai/ollama-code references ==="
rg -n "@tcsenpai/ollama-code" --type jsonLength of output: 2961
🏁 Script executed:
#!/bin/bash
# Check the root package.json and understand the monorepo intent
echo "=== Root package.json ==="
cat ./package.json | head -60
echo -e "\n=== Check for workspaces configuration ==="
cat ./package.json | grep -A 10 "workspaces"
echo -e "\n=== Look for any documentation or comments about 'file:' pattern ==="
find . -name "*.md" -type f | grep -v node_modules | xargs grep -l "file:" 2>/dev/null | head -5Length of output: 4947
Remove self-referential dependency.
The package @tcsenpai/ollama-code in packages/core/package.json adds itself as a dependency with "file:", creating a self-referential dependency. The "file:" syntax without a path resolves to the current directory, causing the package to depend on itself.
In a monorepo with workspaces, packages do not need to declare themselves as dependencies. Remove line 31 from packages/core/package.json:
- "@tcsenpai/ollama-code": "file:",
Note: The same pattern exists in packages/cli/package.json at line 32 ("@tcsenpai/ollama-code-cli": "file:"), which should also be removed.
🤖 Prompt for AI Agents
In packages/core/package.json around line 31 remove the self-referential
dependency entry that lists "@tcsenpai/ollama-code": "file:" since a package
must not declare itself as a dependency; delete that line and save the file,
then run the workspace install to ensure lockfiles update; also check
packages/cli/package.json and remove the analogous "@tcsenpai/ollama-code-cli":
"file:" line if present.
- Add extractPartialJson method to client.ts generateJson() with fallback parsing
- Enhance openaiFormatConverter.ts extractPartialJson to handle JSON + extra text
- Implement progressive JSON parsing: direct parse → fix trailing commas/missing braces
- Add manual key-value extraction for severely malformed JSON cases
- Ensure API continues working even when JSON parsing partially fails
- Fix license header formatting in openaiFormatConverter.ts
- Configure ESLint for root-level script files (json-fix-verification.js)
- Add comprehensive error logging for debugging JSON parsing issues
Resolves: 'unmarshal: invalid character { after top-level value' and
'Unexpected non-whitespace character after JSON' errors during
Ollama API responses
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/core/src/utils/openaiFormatConverter.ts (1)
671-673: Redact sensitive input from logs.This is a duplicate of the privacy concern raised in past reviews. Logging the full input could expose PII or sensitive data in server logs, creating a compliance risk. Log only metadata (e.g., input length) or a truncated/sanitized version.
Based on learnings
Apply this diff to redact the input:
- console.warn('Could not extract valid JSON from:', input); + console.warn(`Could not extract valid JSON (input length: ${input.length})`);
🧹 Nitpick comments (1)
packages/core/src/utils/openaiFormatConverter.ts (1)
481-674: Consider consolidating duplicate JSON parsing implementations.Similar
extractPartialJsonmethods exist in bothclient.ts(line 349) and this file. While the implementations differ slightly (this version is more sophisticated with char-by-char parsing, and returns{}vsnull), consolidating them into a shared utility module would improve maintainability and reduce code duplication.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
eslint.config.js(1 hunks)json-fix-verification.js(1 hunks)packages/core/src/core/client.ts(2 hunks)packages/core/src/utils/openaiFormatConverter.ts(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- json-fix-verification.js
🧰 Additional context used
🧬 Code graph analysis (1)
packages/core/src/utils/openaiFormatConverter.ts (1)
json-fix-verification.js (10)
trimmed(65-65)i(87-87)i(90-90)keyValuePattern(102-102)matches(103-103)result(106-106)result(169-169)key(109-109)value(110-110)rawValue(117-117)
🔇 Additional comments (9)
packages/core/src/core/client.ts (3)
345-363: Good defensive parsing with proper error handling.The initial JSON.parse attempt with error logging is appropriate. The null return type provides a clear failure signal.
365-392: Reasonable best-effort approach for fixing malformed JSON.The trailing comma removal and brace/bracket balancing provide useful fallbacks for streaming chunks. While this won't catch every edge case of nested incomplete objects, it's appropriate for the intended use case of recovering from partial API responses.
507-522: Well-structured integration of robust JSON parsing.The fallback logic properly checks for null results and throws a descriptive error when all parsing attempts fail. The control flow is clear and correct.
packages/core/src/utils/openaiFormatConverter.ts (6)
7-84: Excellent documentation improvements.The module-level and class-level documentation provides clear context and helpful examples for developers. This significantly improves maintainability.
148-150: Improved type guards with clearer narrowing.The refactored type guards using inline object types provide better type narrowing and are more maintainable than the previous implementation.
Also applies to: 161-162, 165-166
169-176: Good use of optional chaining for defensive handling.The optional chaining with fallback values ensures robust handling of potentially missing properties in external API data.
429-479: Well-designed safe parsing interface.The method provides a clean, documented entry point for robust JSON parsing with clear fallback behavior. The separation between this method and
extractPartialJsonmaintains good code organization.
481-605: More sophisticated parsing strategy than the client.ts version.This implementation includes character-by-character parsing with escape sequence handling and brace balance tracking, which is more robust than the simpler approach in
client.ts. The extraction of valid JSON between balanced braces is particularly valuable for handling streaming chunks.
240-240: Proper integration of safe JSON parsing.The use of
safeJsonParsefor tool call arguments directly addresses the PR objective of handling malformed JSON in streaming responses. The fallback to empty objects prevents crashes during partial data transmission.Also applies to: 295-295
| // extra settings for scripts that we run directly with node | ||
| { | ||
| files: ['./scripts/**/*.js', 'esbuild.config.js'], | ||
| files: ['./scripts/**/*.js', 'esbuild.config.js', 'json-fix-verification.js', 'migrate-mcp-config.sh'], |
There was a problem hiding this comment.
Remove shell script from ESLint configuration.
ESLint cannot parse shell scripts. Including migrate-mcp-config.sh (a .sh file) in this JavaScript/TypeScript linting configuration will cause ESLint to either fail or skip the file with warnings. Shell scripts should not be included in ESLint configurations.
Apply this diff to remove the shell script:
- files: ['./scripts/**/*.js', 'esbuild.config.js', 'json-fix-verification.js', 'migrate-mcp-config.sh'],
+ files: ['./scripts/**/*.js', 'esbuild.config.js', 'json-fix-verification.js'],📝 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.
| files: ['./scripts/**/*.js', 'esbuild.config.js', 'json-fix-verification.js', 'migrate-mcp-config.sh'], | |
| files: ['./scripts/**/*.js', 'esbuild.config.js', 'json-fix-verification.js'], |
🤖 Prompt for AI Agents
In eslint.config.js around line 160, the files array incorrectly includes
'migrate-mcp-config.sh' (a shell script); remove 'migrate-mcp-config.sh' from
the files array so only JavaScript/TypeScript files remain (e.g., change files:
['./scripts/**/*.js', 'esbuild.config.js', 'json-fix-verification.js']).
| } | ||
|
|
||
| // Final fallback: return empty object rather than throwing to ensure API continues to work | ||
| console.warn('All JSON parsing methods failed. Could not extract valid JSON from:', input); |
There was a problem hiding this comment.
Redact sensitive input from logs.
Logging the full input could expose PII or sensitive data in server logs, creating a compliance risk. Log only metadata (e.g., input length) or a truncated/sanitized version.
Apply this diff to redact the input:
- console.warn('All JSON parsing methods failed. Could not extract valid JSON from:', input);
+ console.warn(`All JSON parsing methods failed. Could not extract valid JSON (input length: ${input.length})`);📝 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.
| console.warn('All JSON parsing methods failed. Could not extract valid JSON from:', input); | |
| console.warn(`All JSON parsing methods failed. Could not extract valid JSON (input length: ${input.length})`); |
🤖 Prompt for AI Agents
In packages/core/src/core/client.ts around line 439, the console.warn currently
prints the full input which may contain PII; replace that with a redacted log
that does not include the raw input — log safe metadata such as input length and
type, and if you need a sample log a truncated and sanitized preview (e.g.,
first N characters with sensitive characters masked or replaced by ellipses).
Ensure the warning message uses the sanitized preview and length only, and avoid
printing the full input to stdout or persistent logs.
- Enhanced streamGenerator method to detect and handle Ollama-specific streaming chunk errors - Improved convertStreamChunkToGeminiFormat method with graceful error handling for malformed chunks - Enhanced extractPartialJson method with multiple fallback strategies for malformed JSON from Ollama - Added fixOllamaSpecificJson helper to handle Ollama-specific JSON formatting issues - Prevents complete streaming failures from isolated chunk parsing errors - Addresses 'unmarshal: invalid character' errors that caused retry loops Resolves streaming compatibility issues with Ollama API responses that don't match OpenAI client expectations.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (3)
packages/core/src/core/openaiContentGenerator.ts (3)
1288-1295: Redact tool call arguments from logs.Line 1293 logs the full
toolCall.function.argumentsstring, which routinely contains user data (PII). This creates privacy/compliance risks.Apply this diff to log only safe metadata:
- console.warn('Could not parse function arguments, using empty object:', toolCall.function.arguments); + console.warn( + 'Could not parse function arguments (length=%d), using empty object', + toolCall.function.arguments.length + );Based on learnings
1411-1416: Redact accumulated arguments from logs.Line 1415 logs the full
accumulatedCall.argumentsstring, which contains user data (PII).Apply this diff:
- console.warn('Could not parse accumulated tool call arguments, using empty object:', accumulatedCall.arguments); + console.warn( + 'Could not parse accumulated tool call arguments (length=%d), using empty object', + accumulatedCall.arguments.length + );Based on learnings
2092-2103: Redact input from final fallback log.Line 2102 logs the full input string when all parsing methods fail. This directly exposes PII/user data in logs.
Apply this diff:
- // Final fallback: return empty object rather than throwing to ensure API continues to work - console.warn('All JSON parsing methods failed. Could not extract valid JSON from:', input); + // Final fallback: return null rather than throwing to ensure API continues to work + console.warn( + 'All JSON parsing methods failed. Could not extract valid JSON (length=%d, starts with: %s)', + input.length, + input.substring(0, 20) + );Based on learnings
🧹 Nitpick comments (4)
packages/core/src/core/openaiContentGenerator.ts (4)
544-579: Review error swallowing in streaming.The nested error handling continues processing on Ollama chunk errors and yields minimal responses on setup errors. While this improves resilience, it risks:
- Silently processing incomplete data when chunks are malformed
- Masking genuine errors that should fail fast
- Delivering partial/incorrect responses to users without clear indication
Consider:
- Tracking and reporting cumulative error counts to the caller
- Adding metadata to responses indicating partial success
- Logging at error level (not just warn) for visibility
1392-1399: Remove unnecessary try-catch.String concatenation (
accumulatedCall.arguments += toolCall.function.arguments) cannot throw an exception. This try-catch is unnecessary and the comment "Handle Ollama-specific malformed JSON" is misleading—no JSON parsing occurs here.Apply this diff:
- if (toolCall.function?.arguments) { - // Handle Ollama-specific malformed JSON in streaming chunks - try { - accumulatedCall.arguments += toolCall.function.arguments; - } catch (argsError) { - console.warn('Error accumulating tool call arguments from Ollama chunk:', argsError); - // Continue with existing accumulated arguments if any - } - } + if (toolCall.function?.arguments) { + accumulatedCall.arguments += toolCall.function.arguments; + }
1447-1459: Make empty choices handling provider-agnostic.While this defensive code is valuable, the comment and warning assume this only happens with Ollama. Missing choices could occur with any provider due to network issues, API changes, or edge cases.
Apply this diff:
- // Handle Ollama-specific case where no choices are returned - console.warn('Ollama streaming chunk has no choices:', chunk); + // Handle edge case where no choices are returned + console.warn('Streaming chunk has no choices:', chunk);
2012-2027: Good defensive structure with logging improvements needed.The multi-strategy parsing approach is sound. However, line 2024 logs the error message which could indirectly contain input fragments.
Apply this diff to reduce exposure:
- console.warn('Initial JSON.parse failed:', errorMessage); + console.warn('Initial JSON.parse failed (input length=%d)', trimmed.length);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/core/src/core/openaiContentGenerator.ts(11 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/core/src/core/openaiContentGenerator.ts (1)
json-fix-verification.js (16)
trimmed(65-65)fixedInput(75-75)openBraces(82-82)closeBraces(83-83)openBrackets(84-84)closeBrackets(85-85)i(87-87)i(90-90)keyValuePattern(102-102)matches(103-103)result(106-106)result(169-169)key(109-109)value(110-110)rawValue(117-117)singleQuoteFixed(136-136)
🔇 Additional comments (2)
packages/core/src/core/openaiContentGenerator.ts (2)
1576-1598: LGTM: Finish reason mapping is complete.The implementation correctly handles both string and numeric finish reason values with appropriate fallbacks. This addresses the missing functionality mentioned in the PR objectives.
1004-1031: Excellent documentation.The detailed JSDoc comments with examples and rationale significantly improve code maintainability. The explanations of why these methods exist (handling streaming/tool interaction edge cases) are particularly valuable.
Also applies to: 1183-1212
| updateClient(): void { | ||
| this.initializeClient(); | ||
| } | ||
|
|
||
| /** | ||
| * Update the model being used | ||
| */ | ||
| public updateModel(model: string): void { | ||
| updateModel(model: string): void { | ||
| this.model = model; | ||
| console.log('[DEBUG] Updated model to:', this.model); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Verify impact of visibility change.
Removing the explicit public modifiers changes these methods to package-private visibility. If external code calls updateClient() or updateModel(), this is a breaking change.
Run the following script to check for external usage:
🏁 Script executed:
#!/bin/bash
# Search for calls to updateClient and updateModel outside this file
rg -n --type=ts -C3 '\b(updateClient|updateModel)\s*\(' \
-g '!packages/core/src/core/openaiContentGenerator.ts'Length of output: 1562
Revert visibility changes or document breaking change.
External code calls updateModel() at packages/cli/src/ui/hooks/slashCommandProcessor.ts:839 and updateClient() at packages/cli/src/ui/hooks/slashCommandProcessor.ts:896. Removing the public modifiers breaks these call sites. Either restore the public modifiers or update the external callers.
🤖 Prompt for AI Agents
In packages/core/src/core/openaiContentGenerator.ts around lines 143 to 153, the
methods' visibility was changed causing external callers in
packages/cli/src/ui/hooks/slashCommandProcessor.ts (lines ~839 and ~896) to
break; restore the original public visibility on updateClient() and
updateModel() (i.e., re-add the public modifiers) so existing external call
sites continue to work, or if you prefer to keep them non-public, update the two
external callers to use a supported public API instead and run the typecheck to
confirm no other call sites are affected.
| private isOllamaStreamingChunkError(error: unknown): boolean { | ||
| if (!error) return false; | ||
|
|
||
| const errorMessage = error instanceof Error ? error.message.toLowerCase() : String(error).toLowerCase(); | ||
|
|
||
| // Check for common Ollama streaming chunk error patterns | ||
| const ollamaChunkErrorPatterns = [ | ||
| 'unmarshal', | ||
| 'invalid character', | ||
| 'json:', | ||
| 'unexpected token', | ||
| 'invalid json', | ||
| 'malformed json', | ||
| 'syntax error', | ||
| 'parsing', | ||
| ]; | ||
|
|
||
| return ollamaChunkErrorPatterns.some(pattern => errorMessage.includes(pattern)); | ||
| } | ||
|
|
||
| /** | ||
| * Check if an error is specifically related to Ollama streaming setup | ||
| */ | ||
| private isOllamaStreamingSetupError(error: unknown): boolean { | ||
| if (!error) return false; | ||
|
|
||
| const errorMessage = error instanceof Error ? error.message.toLowerCase() : String(error).toLowerCase(); | ||
|
|
||
| // Check for Ollama-specific streaming setup error patterns | ||
| const ollamaStreamErrorPatterns = [ | ||
| '500', | ||
| 'unmarshal: invalid character', | ||
| 'invalid character', | ||
| 'ollama', | ||
| 'json', | ||
| 'parse', | ||
| 'syntax error', | ||
| 'malformed', | ||
| ]; | ||
|
|
||
| return ollamaStreamErrorPatterns.some(pattern => errorMessage.includes(pattern)); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Error detection patterns are overly broad.
These methods claim to detect "Ollama-specific" errors but use generic patterns like 'json', 'parse', 'syntax error', and 'invalid character' that match errors from any provider. This could:
- Misclassify genuine OpenAI/Gemini errors as Ollama errors
- Apply incorrect error handling (continue/yield minimal response) to non-Ollama failures
- Mask real bugs in the format conversion code
Consider either:
- Check the provider explicitly by examining
baseURLor a provider flag before applying special handling - Rename methods to reflect generic JSON error detection (e.g.,
isJsonParsingError) if this resilience should apply to all providers
Apply this diff to check the provider:
private isOllamaStreamingChunkError(error: unknown): boolean {
if (!error) return false;
+
+ // Only apply Ollama-specific handling if we're actually using Ollama
+ const baseURL = process.env.OLLAMA_BASE_URL || process.env.OPENAI_BASE_URL || 'http://localhost:11434/v1';
+ if (!baseURL.includes('11434') && !baseURL.toLowerCase().includes('ollama')) {
+ return false;
+ }
const errorMessage = error instanceof Error ? error.message.toLowerCase() : String(error).toLowerCase();🤖 Prompt for AI Agents
In packages/core/src/core/openaiContentGenerator.ts around lines 584 to 625, the
two helper methods use overly broad JSON/parsing patterns and can misclassify
non-Ollama errors; update them to first validate the provider (e.g., check a
field like baseURL, provider flag, or model metadata that identifies Ollama) and
only run Ollama-specific pattern checks when the provider is Ollama;
alternatively, if you intend to detect generic JSON parsing issues across
providers, rename the methods to reflect that (e.g., isJsonParsingError) and
remove Ollama-specific naming and messages so downstream handling is applied
correctly.
| let fixedInput = this.fixOllamaSpecificJson(trimmed); | ||
|
|
||
| // Fix common issues: | ||
| // 1. Remove trailing commas | ||
| fixedInput = fixedInput.replace(/,\s*([}\]])/g, '$1'); | ||
|
|
||
| // 2. Add missing closing braces/brackets if possible | ||
| const openBraces = (fixedInput.match(/\{/g) || []).length; | ||
| const closeBraces = (fixedInput.match(/\}/g) || []).length; | ||
| const openBrackets = (fixedInput.match(/\[/g) || []).length; | ||
| const closeBrackets = (fixedInput.match(/\]/g) || []).length; | ||
|
|
||
| for (let i = 0; i < openBraces - closeBraces; i++) { | ||
| fixedInput += '}'; | ||
| } | ||
| for (let i = 0; i < openBrackets - closeBrackets; i++) { | ||
| fixedInput += ']'; | ||
| } | ||
|
|
||
| // Try to parse the fixed input with robust error handling | ||
| try { | ||
| return JSON.parse(fixedInput); | ||
| } catch (fixedError) { | ||
| // If still fails, try to extract key-value pairs | ||
| const errorMessage = fixedError instanceof Error ? fixedError.message : String(fixedError); | ||
| console.warn('Failed to parse fixed JSON input:', fixedInput, 'Error:', errorMessage); | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add input validation to prevent abuse.
The method has no length or complexity limits. A malicious or corrupted stream could send megabytes of malformed JSON, causing:
- Excessive memory usage during regex operations
- CPU exhaustion from multiple parsing attempts
- Log flooding from warnings
Apply this diff:
private extractPartialJson(input: string): Record<string, unknown> | null {
if (!input || typeof input !== 'string') {
return null;
}
+
+ // Limit input size to prevent abuse (100KB should be more than enough for tool args)
+ const MAX_INPUT_SIZE = 100000;
+ if (input.length > MAX_INPUT_SIZE) {
+ console.warn('Input exceeds maximum size for JSON extraction (length=%d)', input.length);
+ return null;
+ }
const trimmed = input.trim();Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/core/src/core/openaiContentGenerator.ts around lines 2029 to 2056,
the JSON-fixing logic lacks input validation and can be abused by extremely
large or complex inputs; add a safe-guard that rejects or truncates inputs above
a configured max length (e.g., 50KB) before any regex/parse work, refuse
processing (throw or return null) for inputs exceeding a max
nesting/brace-difference threshold (e.g., >1000 unmatched braces/brackets),
limit the number of characters affected by regex (operate on a slice or run at
most N replacements) and cap loop iterations to a reasonable max, and replace
the full console.warn with a truncated/templated log (e.g., include length,
first/last 200 chars and the error message) to avoid log flooding and excessive
memory/CPU usage.
| // Try to extract key-value pairs manually for simple cases | ||
| // This handles: key1="value1",key2="value2" | ||
| const keyValuePattern = /"([^"]+)"\s*:\s*("([^"]*)"|([^,}\]]+))/g; | ||
| const matches = [...fixedInput.matchAll(keyValuePattern)]; | ||
|
|
||
| if (matches.length > 0) { | ||
| const result: Record<string, unknown> = {}; | ||
|
|
||
| for (const match of matches) { | ||
| const key = match[1]; | ||
| let value: unknown; | ||
|
|
||
| if (match[3] !== undefined) { | ||
| // String value | ||
| value = match[3]; | ||
| } else if (match[4] !== undefined) { | ||
| // Try to parse as number, boolean, or leave as string | ||
| const rawValue = match[4].trim(); | ||
| if (rawValue === 'true' || rawValue === 'false') { | ||
| value = rawValue === 'true'; | ||
| } else if (!isNaN(Number(rawValue)) && rawValue !== '') { | ||
| value = Number(rawValue); | ||
| } else { | ||
| value = rawValue; | ||
| } | ||
| } | ||
|
|
||
| if (key && value !== undefined) { | ||
| result[key] = value; | ||
| } | ||
| } | ||
|
|
||
| return Object.keys(result).length > 0 ? result : null; | ||
| } |
There was a problem hiding this comment.
ReDoS vulnerability in key-value extraction regex.
Line 2059's regex /([^"]+)"\s*:\s*("([^"]*)"|([^,}\]]+))/g with [^"]+ can cause catastrophic backtracking on input like "key":"value followed by many non-quote characters and no closing quote.
Consider using a more specific pattern or setting a maximum number of match attempts:
// Try to extract key-value pairs manually for simple cases
// This handles: key1="value1",key2="value2"
- const keyValuePattern = /"([^"]+)"\s*:\s*("([^"]*)"|([^,}\]]+))/g;
- const matches = [...fixedInput.matchAll(keyValuePattern)];
+ const keyValuePattern = /"([^"]{1,200})"\s*:\s*("([^"]*)"|([^,}\]]{1,200}))/g;
+ const matches = [];
+ let match;
+ let matchCount = 0;
+ const MAX_MATCHES = 50; // Prevent infinite loop on malformed input
+ while ((match = keyValuePattern.exec(fixedInput)) !== null && matchCount++ < MAX_MATCHES) {
+ matches.push(match);
+ }| private fixOllamaSpecificJson(input: string): string { | ||
| let fixed = input; | ||
|
|
||
| // Handle Ollama-specific issue: unescaped quotes or special characters | ||
| // Pattern: text with unescaped quotes that break JSON parsing | ||
| fixed = fixed.replace(/([^\\])"/g, '$1"'); | ||
|
|
||
| // Handle Ollama-specific case: incomplete objects or arrays | ||
| // If we see an opening brace/bracket but no closing, and content looks incomplete | ||
| if ((fixed.includes('{') && !fixed.includes('}')) || (fixed.includes('[') && !fixed.includes(']'))) { | ||
| // Check if the content looks like it was cut off mid-stream | ||
| const lastPart = fixed.split(/[,}\]]/).pop()?.trim(); | ||
| if (lastPart && lastPart.length > 0 && !lastPart.includes(':')) { | ||
| // This looks like incomplete content, add closing brace | ||
| if (fixed.includes('{')) { | ||
| fixed = fixed.replace(/\{[^}]*$/, ''); // Remove incomplete object | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Handle Ollama-specific case: extra characters after valid JSON | ||
| // Check if we have a complete JSON object followed by additional content | ||
| const jsonMatch = fixed.match(/^\s*\{[\s\S]*\}\s*/); | ||
| if (jsonMatch) { | ||
| // If we found a complete JSON object, return just that part | ||
| fixed = jsonMatch[0]; | ||
| } | ||
|
|
||
| return fixed; | ||
| } |
There was a problem hiding this comment.
Fix or remove no-op regex replacement.
Line 2114's regex fixed.replace(/([^\\])"/g, '$1"') replaces each quote with itself (no actual change). This appears to be a bug.
If this was intended to escape unescaped quotes:
- fixed = fixed.replace(/([^\\])"/g, '$1"');
+ fixed = fixed.replace(/([^\\])"/g, '$1\\"');If this serves no purpose, remove it:
- // Handle Ollama-specific issue: unescaped quotes or special characters
- // Pattern: text with unescaped quotes that break JSON parsing
- fixed = fixed.replace(/([^\\])"/g, '$1"');
- 📝 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.
| private fixOllamaSpecificJson(input: string): string { | |
| let fixed = input; | |
| // Handle Ollama-specific issue: unescaped quotes or special characters | |
| // Pattern: text with unescaped quotes that break JSON parsing | |
| fixed = fixed.replace(/([^\\])"/g, '$1"'); | |
| // Handle Ollama-specific case: incomplete objects or arrays | |
| // If we see an opening brace/bracket but no closing, and content looks incomplete | |
| if ((fixed.includes('{') && !fixed.includes('}')) || (fixed.includes('[') && !fixed.includes(']'))) { | |
| // Check if the content looks like it was cut off mid-stream | |
| const lastPart = fixed.split(/[,}\]]/).pop()?.trim(); | |
| if (lastPart && lastPart.length > 0 && !lastPart.includes(':')) { | |
| // This looks like incomplete content, add closing brace | |
| if (fixed.includes('{')) { | |
| fixed = fixed.replace(/\{[^}]*$/, ''); // Remove incomplete object | |
| } | |
| } | |
| } | |
| // Handle Ollama-specific case: extra characters after valid JSON | |
| // Check if we have a complete JSON object followed by additional content | |
| const jsonMatch = fixed.match(/^\s*\{[\s\S]*\}\s*/); | |
| if (jsonMatch) { | |
| // If we found a complete JSON object, return just that part | |
| fixed = jsonMatch[0]; | |
| } | |
| return fixed; | |
| } | |
| private fixOllamaSpecificJson(input: string): string { | |
| let fixed = input; | |
| // Handle Ollama-specific issue: unescaped quotes or special characters | |
| // Pattern: text with unescaped quotes that break JSON parsing | |
| fixed = fixed.replace(/([^\\])"/g, '$1\\"'); | |
| // Handle Ollama-specific case: incomplete objects or arrays | |
| // If we see an opening brace/bracket but no closing, and content looks incomplete | |
| if ((fixed.includes('{') && !fixed.includes('}')) || (fixed.includes('[') && !fixed.includes(']'))) { | |
| // Check if the content looks like it was cut off mid-stream | |
| const lastPart = fixed.split(/[,}\]]/).pop()?.trim(); | |
| if (lastPart && lastPart.length > 0 && !lastPart.includes(':')) { | |
| // This looks like incomplete content, add closing brace | |
| if (fixed.includes('{')) { | |
| fixed = fixed.replace(/\{[^}]*$/, ''); // Remove incomplete object | |
| } | |
| } | |
| } | |
| // Handle Ollama-specific case: extra characters after valid JSON | |
| // Check if we have a complete JSON object followed by additional content | |
| const jsonMatch = fixed.match(/^\s*\{[\s\S]*\}\s*/); | |
| if (jsonMatch) { | |
| // If we found a complete JSON object, return just that part | |
| fixed = jsonMatch[0]; | |
| } | |
| return fixed; | |
| } |
| private fixOllamaSpecificJson(input: string): string { | |
| let fixed = input; | |
| // Handle Ollama-specific issue: unescaped quotes or special characters | |
| // Pattern: text with unescaped quotes that break JSON parsing | |
| fixed = fixed.replace(/([^\\])"/g, '$1"'); | |
| // Handle Ollama-specific case: incomplete objects or arrays | |
| // If we see an opening brace/bracket but no closing, and content looks incomplete | |
| if ((fixed.includes('{') && !fixed.includes('}')) || (fixed.includes('[') && !fixed.includes(']'))) { | |
| // Check if the content looks like it was cut off mid-stream | |
| const lastPart = fixed.split(/[,}\]]/).pop()?.trim(); | |
| if (lastPart && lastPart.length > 0 && !lastPart.includes(':')) { | |
| // This looks like incomplete content, add closing brace | |
| if (fixed.includes('{')) { | |
| fixed = fixed.replace(/\{[^}]*$/, ''); // Remove incomplete object | |
| } | |
| } | |
| } | |
| // Handle Ollama-specific case: extra characters after valid JSON | |
| // Check if we have a complete JSON object followed by additional content | |
| const jsonMatch = fixed.match(/^\s*\{[\s\S]*\}\s*/); | |
| if (jsonMatch) { | |
| // If we found a complete JSON object, return just that part | |
| fixed = jsonMatch[0]; | |
| } | |
| return fixed; | |
| } | |
| private fixOllamaSpecificJson(input: string): string { | |
| let fixed = input; | |
| // Handle Ollama-specific case: incomplete objects or arrays | |
| // If we see an opening brace/bracket but no closing, and content looks incomplete | |
| if ((fixed.includes('{') && !fixed.includes('}')) || (fixed.includes('[') && !fixed.includes(']'))) { | |
| // Check if the content looks like it was cut off mid-stream | |
| const lastPart = fixed.split(/[,}\]]/).pop()?.trim(); | |
| if (lastPart && lastPart.length > 0 && !lastPart.includes(':')) { | |
| // This looks like incomplete content, add closing brace | |
| if (fixed.includes('{')) { | |
| fixed = fixed.replace(/\{[^}]*$/, ''); // Remove incomplete object | |
| } | |
| } | |
| } | |
| // Handle Ollama-specific case: extra characters after valid JSON | |
| // Check if we have a complete JSON object followed by additional content | |
| const jsonMatch = fixed.match(/^\s*\{[\s\S]*\}\s*/); | |
| if (jsonMatch) { | |
| // If we found a complete JSON object, return just that part | |
| fixed = jsonMatch[0]; | |
| } | |
| return fixed; | |
| } |
🤖 Prompt for AI Agents
In packages/core/src/core/openaiContentGenerator.ts around lines 2109 to 2138,
the regex replacement at line ~2114 is a no-op and should either be removed or
corrected; replace the current fixed.replace(/([^\\])"/g, '$1"') with a regex
that actually escapes unescaped double quotes (e.g. fixed =
fixed.replace(/(^|[^\\])"/g, '$1\\"')) so quotes that are not already
backslash-escaped become escaped, ensuring you handle quotes at the start of the
string and avoid double-escaping already-escaped quotes.
1 participant
This PR fixes JSON parsing errors in OpenAI API responses that occur during streaming.
Problem
The application fails with 'invalid character' errors when OpenAI API returns incomplete JSON chunks during streaming, particularly for function call arguments.
Solution
Testing
Impact
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Refactor
Breaking Changes