fix: stdio MCP transport compatibility with Claude Desktop#5
Open
PaoloC68 wants to merge 7 commits intossdeanx:mainfrom
Open
fix: stdio MCP transport compatibility with Claude Desktop#5PaoloC68 wants to merge 7 commits intossdeanx:mainfrom
PaoloC68 wants to merge 7 commits intossdeanx:mainfrom
Conversation
…port Pino was writing JSON logs to stdout, corrupting the JSON-RPC stream. OutputManager.flushLogs() was writing ANSI escape codes to stdout unconditionally. - Configure pino.destination(2) to write all log output to stderr - Guard flushLogs() with TTY check to skip stdout writes in non-TTY (MCP) mode - Replace console.log in logCacheEviction with process.stderr.write
Claude Desktop rejects tool names containing dots. The pattern
'^[a-zA-Z0-9_-]{1,64}$' only allows alphanumeric, underscore, and hyphen.
- Rename 'deepResearch.run' to 'deep-research'
The non-greedy regex /\{[\s\S]*?\}/ matched the first '{' to the
first '}', truncating nested JSON like {"queries": [{...}, {...}]}.
- Implement brace-balanced extraction that tracks depth, string
literals, and escape characters
- Support both object ({}) and array ([]) extraction
- Try JSON.parse on raw text first as fast path
Summary of ChangesHello @PaoloC68, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances compatibility with stdio MCP clients, particularly Claude Desktop, by addressing critical issues related to log output, tool naming conventions, and JSON parsing. The changes ensure that the MCP server's communication stream remains clean, tool definitions adhere to client requirements, and complex JSON responses are reliably processed, leading to a more stable and functional integration. Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughLogger instantiation split into explicit pretty/non-pretty branches; redaction simplified; JSON extraction now tries full-parse then balanced-substring extraction; MCP server updated tool identifier, typings, and payload shapes; output manager timers/flush and stderr behavior tightened; assorted formatting and TypeScript refinements. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant MCPServer as MCP Server
participant Tool as Deep-Research Tool
participant Logger
participant Storage as Output/DB
Client->>MCPServer: submit research request
MCPServer->>Logger: log request (pretty or destination config)
MCPServer->>Tool: invoke 'deep-research' tool
Tool-->>MCPServer: stream progress updates / results
MCPServer->>MCPServer: compute progress pct (depth / breadth / queries)
MCPServer->>Logger: log progress (use redactIfNeeded if configured)
MCPServer->>Storage: flush/persist interim results (flushLogs clears timers/queue)
MCPServer->>Client: return final MCPResearchResult (typed content)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Reviewer's GuideUpdates MCP stdio server to be compatible with Claude Desktop by redirecting logging to stderr, renaming the deep-research tool to meet client naming constraints, and replacing fragile regex-based JSON extraction with a balanced parser while tightening OutputManager behavior in non-TTY environments. Sequence diagram for stdio MCP transport logging separationsequenceDiagram
actor ClaudeDesktop
participant StdioTransport
participant McpServer
participant DeepResearchTool
participant Logger
participant Stdout
participant Stderr
ClaudeDesktop->>StdioTransport: JSON-RPC request (call deep-research)
StdioTransport->>McpServer: deliver request
McpServer->>DeepResearchTool: invoke handler
DeepResearchTool->>Logger: logger.info Environment check
Logger-->>Stderr: write JSON log (pino.destination 2)
DeepResearchTool-->>McpServer: tool result
McpServer-->>StdioTransport: JSON-RPC response
StdioTransport-->>Stdout: write JSON-RPC response
Stdout-->>ClaudeDesktop: response over stdio
note over Logger,Stdout: Logs now go only to stderr, keeping stdout JSON-RPC stream valid
Sequence diagram for balanced JSON extraction from provider textsequenceDiagram
participant DeepResearchTool
participant JsonUtils
participant ProviderText as ProviderResponseText
DeepResearchTool->>ProviderText: get raw text from Gemini
DeepResearchTool->>JsonUtils: extractJsonFromText(text)
activate JsonUtils
JsonUtils->>JsonUtils: JSON.parse(text)
alt parse succeeds
JsonUtils-->>DeepResearchTool: parsed JSON
else parse fails
JsonUtils->>JsonUtils: extractBalancedJson(text,"{","}")
alt object found
JsonUtils->>JsonUtils: JSON.parse(objectSegment)
JsonUtils-->>DeepResearchTool: parsed JSON
else no object found
JsonUtils->>JsonUtils: extractBalancedJson(text,"[","]")
alt array found
JsonUtils->>JsonUtils: JSON.parse(arraySegment)
JsonUtils-->>DeepResearchTool: parsed JSON
else none found
JsonUtils-->>DeepResearchTool: null
end
end
end
deactivate JsonUtils
Class diagram for OutputManager and JSON utilitiesclassDiagram
class OutputManager {
-initialized : boolean
-progressArea : string[]
-lastLogMessage : string
-logQueue : string[]
-logTimer : NodeJS.Timeout?
+constructor()
-formatLogEntry(message : string, metadata : Record<string, unknown>?) string
-bufferLog(message : string)
-flushLogs()
+log(message : string, metadata : Record<string, unknown>?)
+updateProgress(progress : ResearchProgress)
-getProgressBar(value : number, total : number) string
+drawProgress()
+saveFinalReport(reportContent : string)
+static logCacheEviction(value : unknown)
}
class JsonUtils {
+extractJsonFromText(text : string) any
+isValidJSON(jsonString : string) boolean
+safeParseJSON~T~(jsonString : string, defaultValue : T) T
+stringifyJSON(jsonObject : any, prettyPrint : boolean) string
-extractBalancedJson(text : string, open : string, close : string) string
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- There are still
console.warncalls inOutputManagerthat will write to stdout in non-TTY environments, which can corrupt the stdio JSON-RPC stream; consider routing these through the logger or stderr instead. - The new
extractBalancedJsonhelper only looks at the first{/[occurrence viaindexOf, so if the first candidate is truncated or invalid JSON you never scan for later valid blocks; consider iterating over all occurrences to make extraction more robust. getProgressBardivides bytotalwithout guarding against0, which can produceNaNand break the bar rendering; even if current callers avoid zero, it would be safer to handletotal === 0explicitly.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- There are still `console.warn` calls in `OutputManager` that will write to stdout in non-TTY environments, which can corrupt the stdio JSON-RPC stream; consider routing these through the logger or stderr instead.
- The new `extractBalancedJson` helper only looks at the first `{`/`[` occurrence via `indexOf`, so if the first candidate is truncated or invalid JSON you never scan for later valid blocks; consider iterating over all occurrences to make extraction more robust.
- `getProgressBar` divides by `total` without guarding against `0`, which can produce `NaN` and break the bar rendering; even if current callers avoid zero, it would be safer to handle `total === 0` explicitly.
## Individual Comments
### Comment 1
<location> `src/utils/json.ts:21-14` </location>
<code_context>
+ return null;
+}
+
+function extractBalancedJson(
+ text: string,
+ open: string,
+ close: string,
+): string | null {
+ const start = text.indexOf(open);
+ if (start === -1) return null;
+
+ let depth = 0;
+ let inString = false;
+ let escape = false;
+
+ for (let i = start; i < text.length; i++) {
+ const ch = text[i];
+
+ if (escape) {
+ escape = false;
+ continue;
+ }
+ if (ch === '\\' && inString) {
+ escape = true;
+ continue;
+ }
+ if (ch === '"') {
+ inString = !inString;
+ continue;
+ }
+ if (inString) continue;
+
+ if (ch === open) depth++;
+ else if (ch === close) depth--;
+
+ if (depth === 0) {
+ return text.slice(start, i + 1);
}
- } catch (regexError) {
- console.error("Regex error during JSON extraction:", regexError);
- return null; // Return null if regex matching fails
}
+
+ return null;
}
</code_context>
<issue_to_address>
**issue (bug_risk):** `extractBalancedJson` only considers the first opening brace/bracket and gives up if it can't find a matching close, which can miss valid JSON later in the string.
This changes behavior vs the previous regex, which could still match a later `{...}` block even if an earlier one was malformed. If this is used on model output that may contain partial/bad JSON before valid JSON, consider looping: when a run from `start` can’t be balanced, advance to the next occurrence of `open` and try again instead of immediately returning `null`.
</issue_to_address>
### Comment 2
<location> `src/output-manager.ts:54-62` </location>
<code_context>
}
private flushLogs() {
- try {
- process.stdout.write(TERMINAL_CONTROLS.savePos);
- process.stdout.write(this.logQueue.join('\n') + '\n');
+ if (this.logTimer) {
+ clearTimeout(this.logTimer);
+ this.logTimer = undefined;
+ }
+ if (!this.initialized) {
this.logQueue = [];
- } finally {
- if (this.logTimer) {
- clearTimeout(this.logTimer);
- }
- process.stdout.write(TERMINAL_CONTROLS.restorePos);
+ return;
}
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Removing the try/finally around terminal cursor restores can leave the terminal in an inconsistent state if writes throw.
The previous `try/finally` guaranteed `restorePos` even if `process.stdout.write` threw. Now, an exception during `savePos` or the log writes can leave the cursor saved but never restored, leading to broken terminal behavior. Consider keeping the new timer/initialization logic but wrapping the terminal writes (`savePos` / log output / `restorePos`) in a `try/finally` to always restore the cursor on error.
```suggestion
private flushLogs() {
if (this.logTimer) {
clearTimeout(this.logTimer);
this.logTimer = undefined;
}
if (!this.initialized) {
this.logQueue = [];
return;
}
if (this.logQueue.length === 0) {
return;
}
try {
process.stdout.write(TERMINAL_CONTROLS.savePos);
process.stdout.write(this.logQueue.join('\n') + '\n');
this.logQueue = [];
} finally {
process.stdout.write(TERMINAL_CONTROLS.restorePos);
}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively addresses compatibility issues with stdio MCP clients by redirecting log output to stderr, renaming a tool to meet client constraints, and implementing a much more robust JSON extraction method. The changes are well-targeted and correct. I've identified a couple of areas for improvement: one for robustness in terminal UI handling and another for correctness in the new JSON extraction logic to ensure it extracts the first JSON structure without bias. Overall, this is a solid fix.
Comment on lines
+8
to
+9
| const result = | ||
| extractBalancedJson(text, '{', '}') ?? extractBalancedJson(text, '[', ']'); |
There was a problem hiding this comment.
The current implementation has a bias for extracting a JSON object over an array. It will always try to find a balanced {...} structure first. If the input text contains an array before an object (e.g., ... [...] ... {...}), it will incorrectly extract the object instead of the array that appears first. To ensure the first JSON structure is extracted, you should check which opening character ({ or [) appears earlier in the string and prioritize parsing based on that order, while still having a fallback to the other type if the first one is malformed.
For example:
const firstBrace = text.indexOf('{');
const firstBracket = text.indexOf('[');
let result;
if (firstBrace !== -1 && (firstBracket === -1 || firstBrace < firstBracket)) {
// Object appears first, try to parse it, fallback to array.
result = extractBalancedJson(text, '{', '}') ?? extractBalancedJson(text, '[', ']');
} else if (firstBracket !== -1) {
// Array appears first or is the only one, try to parse it, fallback to object.
result = extractBalancedJson(text, '[', ']') ?? extractBalancedJson(text, '{', '}');
} else {
result = null;
}
Comment on lines
+55
to
+66
| if (this.logTimer) { | ||
| clearTimeout(this.logTimer); | ||
| this.logTimer = undefined; | ||
| } | ||
| if (!this.initialized) { | ||
| this.logQueue = []; | ||
| } finally { | ||
| if (this.logTimer) { | ||
| clearTimeout(this.logTimer); | ||
| } | ||
| process.stdout.write(TERMINAL_CONTROLS.restorePos); | ||
| return; | ||
| } | ||
| process.stdout.write(TERMINAL_CONTROLS.savePos); | ||
| process.stdout.write(this.logQueue.join('\n') + '\n'); | ||
| this.logQueue = []; | ||
| process.stdout.write(TERMINAL_CONTROLS.restorePos); |
There was a problem hiding this comment.
The try...finally block was removed from this function. While the new logic correctly handles the non-TTY case, removing try...finally makes the terminal operations less robust. If any process.stdout.write() call throws an error (e.g., if the stream is closed unexpectedly), the cursor position won't be restored, which can corrupt the terminal display. It's best practice to wrap terminal escape sequence operations in try...finally to guarantee cleanup.
Suggested change
| if (this.logTimer) { | |
| clearTimeout(this.logTimer); | |
| this.logTimer = undefined; | |
| } | |
| if (!this.initialized) { | |
| this.logQueue = []; | |
| } finally { | |
| if (this.logTimer) { | |
| clearTimeout(this.logTimer); | |
| } | |
| process.stdout.write(TERMINAL_CONTROLS.restorePos); | |
| return; | |
| } | |
| process.stdout.write(TERMINAL_CONTROLS.savePos); | |
| process.stdout.write(this.logQueue.join('\n') + '\n'); | |
| this.logQueue = []; | |
| process.stdout.write(TERMINAL_CONTROLS.restorePos); | |
| if (this.logTimer) { | |
| clearTimeout(this.logTimer); | |
| this.logTimer = undefined; | |
| } | |
| if (!this.initialized) { | |
| this.logQueue = []; | |
| return; | |
| } | |
| try { | |
| process.stdout.write(TERMINAL_CONTROLS.savePos); | |
| process.stdout.write(this.logQueue.join('\n') + '\n'); | |
| this.logQueue = []; | |
| } finally { | |
| process.stdout.write(TERMINAL_CONTROLS.restorePos); | |
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/logger.ts (1)
27-35: 🧹 Nitpick | 🔵 TrivialDead code in
redactIfNeededtry/catchThe
tryblock contains onlyreturn '[REDACTED]', which can never throw. Thecatchbranch is unreachable.♻️ Proposed simplification
export function redactIfNeeded<T>(obj: T): T | string { const redacted = (process.env.PROGRESS_REDACT_BODIES || 'false').toLowerCase() === 'true'; if (!redacted) { return obj; } - try { - return '[REDACTED]'; - } catch { - return '[REDACTED]'; - } + return '[REDACTED]'; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/logger.ts` around lines 27 - 35, The function redactIfNeeded currently wraps a constant return in a try/catch, producing unreachable code; remove the try/catch and simplify the control flow in redactIfNeeded so that after computing redacted (the existing process.env.PROGRESS_REDACT_BODIES check) it directly returns obj when !redacted and returns the string '[REDACTED]' when redacted, eliminating the unnecessary catch block and any unreachable branches.
🤖 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/logger.ts`:
- Around line 6-24: The logger instantiation mixes transport and stream args:
refactor the export const logger initialization to split the pretty vs
non-pretty paths so when pretty is true you call pino(...) with the transport
configuration (including options.destination = 2) and do NOT pass the second
pino.destination(2) argument, and when pretty is false call pino({...},
pino.destination(2)) as before; update references to pino.destination usage
accordingly and keep the transport.target set to 'pino-pretty'. Also simplify
redactIfNeeded by removing the dead try/catch (the try only returns '[REDACTED]'
and cannot throw) so the function returns directly without a catch block.
In `@src/mcp-server.ts`:
- Around line 35-46: The local interface MCPResearchResult in this file shadows
the same-named type in src/types.ts and adds an index signature ([key: string]:
unknown), causing a silent type mismatch; either remove the duplicate and import
the canonical type from src/types.ts (and, if needed, extend it there to include
the index signature) or rename the local type to something like
LocalMCPResearchResult and make it explicitly compatible (e.g., with a
satisfies/compatibility assertion) so callers are not misled; update references
in this file from MCPResearchResult to the chosen solution and ensure the index
signature is defined only in the shared type if you intend it globally.
- Around line 193-195: The log call currently passes a string in logger.error ({
err: errorMessage }, 'Error during deep research') which loses the stack; update
the logger.error call to pass the original Error instance (or create one when
error is not an Error) as the err field (e.g., use error if error instanceof
Error else new Error(String(error))) while keeping the existing errorMessage
string for the returned content.text; change only the logger.error invocation
around the logger and errorMessage variables so Pino's err serializer can
include name/message/stack.
In `@src/output-manager.ts`:
- Around line 78-93: The progress percentage calculations in updateProgress
(building this.progressArea) and the filled computation in getProgressBar can
divide by zero; update updateProgress to guard each denominator
(progress.totalDepth, progress.totalBreadth, progress.totalQueries) by using a
conditional (e.g., total > 0 ? calculation : 0) when computing both the percent
and the value passed to getProgressBar, and modify getProgressBar to handle
total <= 0 by returning an empty/unfilled bar (or width spaces) and treating
filled as 0 so no NaN is produced; reference the updateProgress block that
builds this.progressArea and the private getProgressBar(value:number,
total:number) function when making the changes.
In `@src/utils/json.ts`:
- Around line 8-16: The current logic uses a single combined result variable
(via ??) so if extractBalancedJson(text, '{', '}') returns a non-null string
that fails JSON.parse the array extractor is never attempted; change the flow in
the function that calls extractBalancedJson to try each extractor independently:
first call extractBalancedJson(text, '{', '}'), attempt JSON.parse on that
string inside a try/catch and if parsing succeeds return it, otherwise proceed
to call extractBalancedJson(text, '[', ']') and attempt JSON.parse on that
result, returning it if successful or null if both fail; reference
extractBalancedJson and the JSON.parse attempts to locate the code to update.
---
Outside diff comments:
In `@src/logger.ts`:
- Around line 27-35: The function redactIfNeeded currently wraps a constant
return in a try/catch, producing unreachable code; remove the try/catch and
simplify the control flow in redactIfNeeded so that after computing redacted
(the existing process.env.PROGRESS_REDACT_BODIES check) it directly returns obj
when !redacted and returns the string '[REDACTED]' when redacted, eliminating
the unnecessary catch block and any unreachable branches.
Comment on lines
35
to
46
| interface MCPResearchResult { | ||
| content: { type: "text"; text: string; }[]; | ||
| metadata: { | ||
| learnings: string[]; | ||
| visitedUrls: string[]; | ||
| stats: { | ||
| totalLearnings: number; | ||
| totalSources: number; | ||
| }; | ||
| content: { type: 'text'; text: string }[]; | ||
| metadata: { | ||
| learnings: string[]; | ||
| visitedUrls: string[]; | ||
| stats: { | ||
| totalLearnings: number; | ||
| totalSources: number; | ||
| }; | ||
| [key: string]: unknown; | ||
| }; | ||
| [key: string]: unknown; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Local MCPResearchResult interface shadows the same-named type in src/types.ts
The comment on Line 34 says "to avoid conflict", but the local interface keeps the same identifier MCPResearchResult. The local definition also diverges from src/types.ts by adding [key: string]: unknown. If code outside this file imports from types.ts expecting the same shape, the mismatch will be a silent type discrepancy.
Consider either:
- Extending the definition in
src/types.tsto include the index signature (so it can be imported here), or - Using a locally-distinct name (e.g.
LocalMCPResearchResult) with asatisfies/compatibility assertion to keep the intent clear.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/mcp-server.ts` around lines 35 - 46, The local interface
MCPResearchResult in this file shadows the same-named type in src/types.ts and
adds an index signature ([key: string]: unknown), causing a silent type
mismatch; either remove the duplicate and import the canonical type from
src/types.ts (and, if needed, extend it there to include the index signature) or
rename the local type to something like LocalMCPResearchResult and make it
explicitly compatible (e.g., with a satisfies/compatibility assertion) so
callers are not misled; update references in this file from MCPResearchResult to
the chosen solution and ensure the index signature is defined only in the shared
type if you intend it globally.
Comment on lines
+193
to
195
| const errorMessage = | ||
| error instanceof Error ? error.message : String(error); | ||
| logger.error({ err: errorMessage }, 'Error during deep research'); |
There was a problem hiding this comment.
err field loses stack trace — pass the original Error object instead
errorMessage is a plain string extracted from the error. Pino's built-in err serializer (pino.stdSerializers.err) expects an Error instance and extracts name, message, and stack. Passing a string skips that serialization, losing the stack trace in logs.
🐛 Proposed fix
- const errorMessage =
- error instanceof Error ? error.message : String(error);
- logger.error({ err: errorMessage }, 'Error during deep research');
+ const errorMessage =
+ error instanceof Error ? error.message : String(error);
+ logger.error({ err: error }, 'Error during deep research');errorMessage is still used below in the returned content.text, so keep the extraction — just don't pass it as the pino err field.
📝 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.
Suggested change
| const errorMessage = | |
| error instanceof Error ? error.message : String(error); | |
| logger.error({ err: errorMessage }, 'Error during deep research'); | |
| const errorMessage = | |
| error instanceof Error ? error.message : String(error); | |
| logger.error({ err: error }, 'Error during deep research'); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/mcp-server.ts` around lines 193 - 195, The log call currently passes a
string in logger.error ({ err: errorMessage }, 'Error during deep research')
which loses the stack; update the logger.error call to pass the original Error
instance (or create one when error is not an Error) as the err field (e.g., use
error if error instanceof Error else new Error(String(error))) while keeping the
existing errorMessage string for the returned content.text; change only the
logger.error invocation around the logger and errorMessage variables so Pino's
err serializer can include name/message/stack.
- extractBalancedJson: iterate over occurrences if first candidate fails - extractJsonFromText: try object and array extraction independently - flushLogs: restore try/finally to guarantee cursor restore on error - console.warn in OutputManager constructor routed to stderr - getProgressBar and updateProgress: guard against divide-by-zero - logger: split pretty/non-pretty paths to avoid mixing transport and stream args - redactIfNeeded: remove dead try/catch
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/utils/json.ts`:
- Around line 60-63: The helper extractBalancedJson currently parses candidate
to validate but returns the raw string, causing extractJsonFromText to parse
again; change extractBalancedJson to return the parsed value (unknown) or null
on failure and update its signature accordingly, then modify extractJsonFromText
to accept the parsed value returned by extractBalancedJson instead of calling
JSON.parse again and remove the now-redundant try/catch; ensure any other
callers of extractBalancedJson are updated for the new return type and handle
null parse failures.
- Around line 59-71: The loop prematurely aborts when an opener is unclosed
because on unbalanced detection you set searchFrom = text.length; instead, when
the inner loop exits without reaching depth === 0 (balanced === false) advance
searchFrom to start + 1 so the outer while can continue searching for subsequent
openers; update the code surrounding the inner for loop and the searchFrom
assignment (variables: searchFrom, start, balanced, depth, and the inner for
that checks depth) to use searchFrom = start + 1 on unbalanced cases, preserving
monotonic advancement and relying on indexOf to eventually terminate.
- searchFrom always advances to start+1 (not text.length) on unclosed openers
- extractBalancedJson returns parsed value directly to avoid double JSON.parse
- position-based ordering: extract whichever delimiter ({/[) appears first in text
- Add chunkOverlap >= chunkSize validation in splitText() (not just constructor) so mutating properties after construction is also caught - Fix test expectation for large text: overlap produces 6 chunks, not 5 - Fix special char test: reset chunkSize to 9 so string actually gets split
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/utils/json.ts`:
- Around line 1-28: The function extractJsonFromText currently computes
firstBrace and firstBracket then builds pairs and loops, but if both are -1 we
can short-circuit earlier; modify extractJsonFromText to check if firstBrace ===
-1 && firstBracket === -1 and immediately return null before constructing pairs
and calling extractBalancedJson, referencing the existing variables firstBrace,
firstBracket, pairs and the helper extractBalancedJson to keep behavior
identical otherwise.
- Around line 30-34: Update the return type of function extractBalancedJson from
"unknown | null" to simply "unknown" in the function signature (function
extractBalancedJson(...): unknown) since null is assignable to unknown and the
union is redundant; adjust any call sites or tests if they explicitly expect the
narrower union type to ensure type compatibility, and run typechecks to confirm
no further changes are needed.
---
Duplicate comments:
In `@src/utils/json.ts`:
- Around line 65-70: Ensure extractBalancedJson returns a parsed JS value (not a
JSON string) and that extractJsonFromText accepts and returns that parsed value
directly; remove any remaining redundant JSON.parse in extractJsonFromText and
update types/signatures for extractBalancedJson and extractJsonFromText to
reflect returning a parsed object/value so callers use the value as-is (refer to
extractBalancedJson and extractJsonFromText to locate the code).
Comment on lines
+30
to
+34
| function extractBalancedJson( | ||
| text: string, | ||
| open: string, | ||
| close: string, | ||
| ): unknown | null { |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
unknown | null return type — | null is redundant.
null is already assignable to unknown, so the union simplifies to unknown.
🧹 Suggested cleanup
function extractBalancedJson(
text: string,
open: string,
close: string,
-): unknown | null {
+): unknown {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/utils/json.ts` around lines 30 - 34, Update the return type of function
extractBalancedJson from "unknown | null" to simply "unknown" in the function
signature (function extractBalancedJson(...): unknown) since null is assignable
to unknown and the union is redundant; adjust any call sites or tests if they
explicitly expect the narrower union type to ensure type compatibility, and run
typechecks to confirm no further changes are needed.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/ai/text-splitter.ts (1)
175-189:⚠️ Potential issue | 🟡 Minor
TiktokenTextSplitter.splitTextis missing thechunkOverlapguard added toRecursiveCharacterTextSplitter.splitText.
chunkSizeandchunkOverlapare public mutable fields onTiktokenTextSplitteras well. IfchunkOverlap >= chunkSizeafter construction,Math.max(1, this.chunkSize - this.chunkOverlap)silently clamps the step to1, producing an effective overlap ofchunkSize - 1instead ofchunkOverlap— wrong result with no error signal.🛡️ Proposed fix
splitText(text: string): string[] { + if (this.chunkOverlap >= this.chunkSize) { + throw new Error('Cannot have chunkOverlap >= chunkSize'); + } if (!text) { return []; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ai/text-splitter.ts` around lines 175 - 189, TiktokenTextSplitter.splitText currently allows chunkOverlap >= chunkSize which silently changes behavior; add the same guard used in RecursiveCharacterTextSplitter.splitText to validate chunkOverlap and fail-fast. Specifically, in the TiktokenTextSplitter.splitText method check whether this.chunkOverlap >= this.chunkSize (or a negative value) and throw a clear Error (or RangeError) stating that chunkOverlap must be smaller than chunkSize, so callers cannot mutate public fields to produce incorrect overlaps; locate the check near the start of splitText and mirror the validation logic from RecursiveCharacterTextSplitter.
🤖 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/ai/text-splitter.ts`:
- Around line 21-30: Remove the redundant empty-string entry from the default
separators array on the this.separators initialization (the trailing '' in the
array) because the code already falls back to '' via const sep =
this.separators[sepIdx] ?? '' and the existing sep === '' branch duplicates the
hard-cut behavior handled when sepIdx is out of bounds; update the default array
in the TextSplitter (this.separators) initializer to omit '' and leave the
existing sepIdx/sep === '' logic unchanged.
---
Outside diff comments:
In `@src/ai/text-splitter.ts`:
- Around line 175-189: TiktokenTextSplitter.splitText currently allows
chunkOverlap >= chunkSize which silently changes behavior; add the same guard
used in RecursiveCharacterTextSplitter.splitText to validate chunkOverlap and
fail-fast. Specifically, in the TiktokenTextSplitter.splitText method check
whether this.chunkOverlap >= this.chunkSize (or a negative value) and throw a
clear Error (or RangeError) stating that chunkOverlap must be smaller than
chunkSize, so callers cannot mutate public fields to produce incorrect overlaps;
locate the check near the start of splitText and mirror the validation logic
from RecursiveCharacterTextSplitter.
Comment on lines
+21
to
+30
| this.separators = params?.separators ?? [ | ||
| '\n\n', | ||
| '\n', | ||
| '. ', | ||
| '.', | ||
| ', ', | ||
| ',', | ||
| ' ', | ||
| '', | ||
| ]; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
The explicit '' separator at the end is redundant.
Line 71 reads const sep = this.separators[sepIdx] ?? ''; — when sepIdx reaches this.separators.length the out-of-bounds access already defaults to '', and the if (sep === '') branch at line 72 performs the identical hard-cut loop as the sepIdx >= this.separators.length branch at line 58. Adding '' explicitly only causes the code to reach that branch one step earlier with no difference in output.
♻️ Proposed simplification
this.separators = params?.separators ?? [
'\n\n',
'\n',
'. ',
'.',
', ',
',',
' ',
- '',
];If the intent is documentation ("fall back to hard cuts"), a comment on the sep === '' branch already says that, making the explicit entry unnecessary.
📝 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.
Suggested change
| this.separators = params?.separators ?? [ | |
| '\n\n', | |
| '\n', | |
| '. ', | |
| '.', | |
| ', ', | |
| ',', | |
| ' ', | |
| '', | |
| ]; | |
| this.separators = params?.separators ?? [ | |
| '\n\n', | |
| '\n', | |
| '. ', | |
| '.', | |
| ', ', | |
| ',', | |
| ' ', | |
| ]; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ai/text-splitter.ts` around lines 21 - 30, Remove the redundant
empty-string entry from the default separators array on the this.separators
initialization (the trailing '' in the array) because the code already falls
back to '' via const sep = this.separators[sepIdx] ?? '' and the existing sep
=== '' branch duplicates the hard-cut behavior handled when sepIdx is out of
bounds; update the default array in the TextSplitter (this.separators)
initializer to omit '' and leave the existing sepIdx/sep === '' logic unchanged.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes three issues preventing the MCP server from working correctly with Claude Desktop (and other stdio MCP clients):
{"level":30,...,"msg":"Environment check"}as a JSON-RPC message and failed validation.deepResearch.runcontains a.which violates Claude Desktop's tool name pattern^[a-zA-Z0-9_-]{1,64}$./\{[\s\S]*?\}/matched{to the first}, truncating nested JSON responses from Gemini (e.g.{"queries": [{...}, {...}]}).Changes
src/logger.ts+src/output-manager.tspino.destination(2)OutputManager.flushLogs()to skip stdout writes when not in a TTYconsole.loginlogCacheEvictionwithprocess.stderr.writesrc/mcp-server.tsdeepResearch.runtodeep-researchsrc/utils/json.tsJSON.parseon raw text before extraction{...}and[...]top-level JSONTesting
Verified the server connects and lists tools successfully via
mcpm runwith Claude Desktop. Research queries execute without JSON parse errors.Summary by Sourcery
Ensure the MCP deep research server is compatible with stdio-based clients like Claude Desktop by adjusting tool naming, logging output, and JSON extraction.
Bug Fixes:
Enhancements: