Skip to content

fix(claude): strip empty Read pages tool input - #2937

Merged
diegosouzapw merged 3 commits into
diegosouzapw:mainfrom
makcimbx:fix/claude-read-empty-pages
May 31, 2026
Merged

diegosouzapw merged 3 commits into
diegosouzapw:mainfrom
makcimbx:fix/claude-read-empty-pages

Conversation

@makcimbx

Copy link
Copy Markdown
Contributor

Summary

  • Adds a Claude Code Read tool-call shim that removes invalid empty pages placeholders before streaming tool input back to Claude Code.
  • Preserves valid non-empty PDF page ranges.
  • Extends Responses → Chat tool argument cleanup to handle JSON-string item.arguments, not only object arguments.
  • Adds unit coverage for helper-level cleanup, streaming input_json_delta behavior, and JSON-string Responses arguments.

Closes #2935
Addresses #2889

Test Plan

npm exec -- cross-env DISABLE_SQLITE_AUTO_BACKUP=true node --max-old-space-size=4096 --import tsx --import ./open-sse/utils/setupPolyfill.ts --test --test-force-exit tests/unit/translator-tool-call-shim.test.ts tests/unit/translator-resp-openai-responses.test.ts

Result: 32 passing, 0 failing.

Buffer Claude Code Read tool calls through the existing shim layer so empty pages placeholders are removed before streaming input_json_delta to the client. Also clean JSON-string Responses tool arguments, not only object arguments.

Closes diegosouzapw#2935
Addresses diegosouzapw#2889
@makcimbx
makcimbx requested a review from diegosouzapw as a code owner May 30, 2026 08:50

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a Read tool shim to filter out empty pages arguments from non-Anthropic models and refactors the argument cleaning logic in openai-responses.ts into a reusable stripEmptyOptionalToolArgs helper, supported by new unit tests. Feedback on these changes highlights a potential bug in stripEmptyOptionalToolArgs where using the logical OR operator (||) for a fallback value could inadvertently replace valid falsy JSON values (such as false or 0) with {}. It is recommended to use the nullish coalescing operator (??) instead to preserve these values.

Comment on lines +15 to +23
if (typeof value === "string") {
try {
const parsed = JSON.parse(value);
const cleaned = stripEmptyOptionalToolArgs(parsed);
return JSON.stringify(cleaned || {});
} catch {
return value;
}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Using the logical OR operator (||) to provide a fallback value of {} will cause any valid falsy JSON values (such as false, 0, or "") to be incorrectly replaced with {}. For example, if a tool argument is a stringified boolean "false", it will be parsed as false, cleaned as false, and then false || {} will evaluate to {}, resulting in "{}" being returned instead of "false".

Using the nullish coalescing operator (??) instead ensures that only null or undefined values are coalesced to {}, preserving other valid falsy values.

Suggested change
if (typeof value === "string") {
try {
const parsed = JSON.parse(value);
const cleaned = stripEmptyOptionalToolArgs(parsed);
return JSON.stringify(cleaned || {});
} catch {
return value;
}
}
if (typeof value === "string") {
try {
const parsed = JSON.parse(value);
const cleaned = stripEmptyOptionalToolArgs(parsed);
return JSON.stringify(cleaned ?? {});
} catch {
return value;
}
}

makcimbx added 2 commits May 30, 2026 08:58
Keep existing object-argument cleanup behavior, but avoid parsing and stripping arbitrary JSON-string arguments for unrelated tools where empty strings or arrays may be valid payloads. Add regression coverage for non-Read and non-object Read arguments.
@makcimbx

Copy link
Copy Markdown
Contributor Author

Follow-up after the bot review: addressed in later commits.

  • 3767e13 replaced the cleaned || {} fallback with cleaned ?? {} so valid falsy JSON values are preserved.
  • d25e323 further narrowed JSON-string argument cleanup to the Claude Code Read tool only. Non-Read JSON-string tool arguments are now preserved as-is, because empty strings/arrays can be valid payloads for arbitrary tools.
  • Added regression coverage for falsy JSON-string arguments, non-Read JSON-string arguments, non-object Read JSON strings, and the original Read.pages cleanup path.

Local verification after the follow-up changes:

tests/unit/translator-tool-call-shim.test.ts
tests/unit/translator-resp-openai-responses.test.ts
35 passing, 0 failing

@diegosouzapw

Copy link
Copy Markdown
Owner

Thank you for your contribution! This PR has been reviewed and integrated into the upcoming v3.8.8 release. 🎉

@diegosouzapw
diegosouzapw merged commit ec72330 into diegosouzapw:main May 31, 2026
3 checks passed
HouMinXi pushed a commit to HouMinXi/OmniRoute that referenced this pull request Aug 2, 2026
Poid-ZA pushed a commit to Poid-ZA/OmniRoute that referenced this pull request Aug 5, 2026
muhamadgalihsaputra pushed a commit to niyatna/NiyatnaRoute that referenced this pull request Sep 27, 2026
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.

Claude Code Read should strip empty pages from streamed tool input

2 participants