Skip to content

fix(github): handle non-JSON upstream tool responses#388

Open
tlgimenes wants to merge 2 commits intomainfrom
tlgimenes/fix-get-file-parse
Open

fix(github): handle non-JSON upstream tool responses#388
tlgimenes wants to merge 2 commits intomainfrom
tlgimenes/fix-get-file-parse

Conversation

@tlgimenes
Copy link
Copy Markdown
Contributor

@tlgimenes tlgimenes commented Apr 15, 2026

Summary

  • Fix get_file_contents (and other tools) failing when the upstream GitHub MCP server returns plain text instead of JSON
  • Previously, non-JSON responses like "successfully downloaded text file (SHA: ...)" caused a Failed to parse error
  • Now returns the raw text message when JSON parsing fails, instead of throwing

Test plan

  • Call get_file_contents on a text file and verify it returns content without error
  • Call tools that return JSON responses and verify they still work correctly

🤖 Generated with Claude Code


Summary by cubic

Handle non-JSON responses from the upstream GitHub MCP server so tools like get_file_contents don’t fail on plain-text messages. Added debug logging of upstream callTool results to help trace response formats.

  • Bug Fixes
    • Parse JSON when available; otherwise return the original text (or null) instead of throwing.
    • Log upstream callTool responses (tool name and payload) to diagnose non-JSON cases.

Written for commit 58eae94. Summary will update on new commits.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 issue found across 1 file

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="github/server/lib/mcp-proxy.ts">

<violation number="1" location="github/server/lib/mcp-proxy.ts:188">
P2: Valid falsy JSON values are misclassified as parse failures because of a truthiness check.</violation>
</file>

Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.

Comment thread github/server/lib/mcp-proxy.ts Outdated
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 issues found across 1 file (changes from recent commits).

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="github/server/lib/mcp-proxy.ts">

<violation number="1">
P1: Non-JSON tool responses still throw `Failed to parse` instead of falling back to raw text/null.</violation>

<violation number="2" location="github/server/lib/mcp-proxy.ts:171">
P2: Avoid logging full upstream tool results in production; this can expose sensitive payload data and create noisy/large logs.</violation>
</file>

Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.

@@ -168,6 +168,7 @@ export function buildUpstreamTools(
name: toolDef.name,
Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai bot Apr 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1: Non-JSON tool responses still throw Failed to parse instead of falling back to raw text/null.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At github/server/lib/mcp-proxy.ts, line 189:

<comment>Non-JSON tool responses still throw `Failed to parse` instead of falling back to raw text/null.</comment>

<file context>
@@ -185,11 +186,10 @@ export function buildUpstreamTools(
           const parsed = safeJsonParse(msg);
-          if (parsed) {
-            return parsed;
+          if (!parsed) {
+            throw new Error(`Failed to parse: ${msg}`);
           }
</file context>
Fix with Cubic

name: toolDef.name,
arguments: context as Record<string, unknown>,
});
console.log(`[mcp-proxy] callTool result for ${toolDef.name}:`, JSON.stringify(result, null, 2));
Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai bot Apr 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: Avoid logging full upstream tool results in production; this can expose sensitive payload data and create noisy/large logs.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At github/server/lib/mcp-proxy.ts, line 171:

<comment>Avoid logging full upstream tool results in production; this can expose sensitive payload data and create noisy/large logs.</comment>

<file context>
@@ -168,6 +168,7 @@ export function buildUpstreamTools(
             name: toolDef.name,
             arguments: context as Record<string, unknown>,
           });
+          console.log(`[mcp-proxy] callTool result for ${toolDef.name}:`, JSON.stringify(result, null, 2));
           const contents = result.content as
             | Array<{ type: string; text?: string }>
</file context>
Suggested change
console.log(`[mcp-proxy] callTool result for ${toolDef.name}:`, JSON.stringify(result, null, 2));
if ((ctx as AppContext<Env>).env.NODE_ENV !== "production") {
console.log(`[mcp-proxy] callTool result for ${toolDef.name}:`, JSON.stringify(result, null, 2));
}
Fix with Cubic

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant