perf: eliminate response.clone() memory doubling and cache lazy tool getters by skyhighbg22-jpg · Pull Request #1478 · Gitlawb/openclaude

skyhighbg22-jpg · 2026-06-01T16:47:06Z

Summary

Performance optimizations — eliminates memory doubling from response cloning and caches lazy tool getters.

Optimizations

`src/services/api/openaiShim.ts`

Line	Fix
2733	Replace `response.clone()` with `response.text()` → recreate `Response` immediately → then `JSON.parse()` for usage. Response body is preserved even if JSON.parse fails.

`src/tools.ts`

Line	Fix
214, 217, 230	Cache lazy `require()` getter results in local IIFEs — avoids double-invocation of `getSendMessageTool`, `getTeamCreateTool/DeleteTool`, `getPowerShellTool`

Testing

Build passes: bun run build ✓

Summary by CodeRabbit

Bug Fixes
- Prevented “Body already used” errors in non-streaming API calls by defensively recreating the response body before parsing.
- Improved resilience when parsing fails and preserved routing-related response metadata during body handling.
Tests
- Added regression coverage for usage-parse failure handling.
- Added regression coverage to ensure response routing metadata (e.g., response.url) is preserved.
Refactor
- Reduced redundant lazy-loading invocations when assembling the full tool list, keeping behavior unchanged.

jatmn

I found issues that need to be addressed before this is ready.

Findings

[P2] Preserve the response body when usage parsing fails
src/services/api/openaiShim.ts:2733
This now reads the live Response body before JSON.parse succeeds, but the catch leaves the original consumed response in place. If an OpenAI-compatible provider returns a successful non-streaming response with invalid JSON or an unexpected non-JSON body, the later caller path at response.json()/response.text() now fails with Body already used or loses the provider's original body, whereas the old response.clone().json() failure left the original response readable for normal conversion/error handling. Please recreate the Response immediately after text() succeeds, or otherwise only consume a clone/fallback buffer so downstream parsing can still read the body when usage extraction fails.
[P3] De-duplicate this scope with the older open PR
src/services/api/openaiShim.ts:2733
This PR repeats the same openaiShim.ts response-body optimization and the same src/tools.ts lazy getter caching that are already present in the older open #1473 from the same branch family, while #1473 also carries adjacent fixes in the same files. Keeping both PRs open makes reviewers resolve the same bug and tool-registry changes twice and risks merging one copy without the other PR's follow-up corrections. Please close/supersede one PR or remove the overlapping hunks so there is a single source of truth for these changes.

jatmn

Thanks for the update. I rechecked the changed paths and found an issue that still needs to be addressed.

Findings

[P2] Preserve the response body when usage parsing fails
src/services/api/openaiShim.ts:2733
The live Response body is still consumed before usage parsing has safely completed, and the replacement Response is only created after JSON.parse(bodyText) succeeds. If a provider returns a successful non-streaming response with invalid JSON or a 2xx non-JSON body, the catch swallows the parse failure and returns the original consumed response, so the downstream conversion/error path later hits Body already used instead of reading the provider body. Please recreate the response immediately after await response.text() succeeds, before parsing usage, or keep the usage probe on a clone/fallback buffer so parse failures leave the response readable.

…getters openaiShim.ts: - Replace response.clone() with response.text() + JSON.parse() + new Response() for non-streaming usage extraction — avoids doubling memory for large responses tools.ts: - Cache getSendMessageTool(), getTeamCreateTool/DeleteTool(), getPowerShellTool() results in local IIFEs — avoids double-invocation of lazy require() getters

jatmn

Thanks for the update. I rechecked the changed paths and found one issue that still needs to be addressed.

Findings

[P3] Align the PR description with the current scope
src/main.tsx:1280
The PR description still says this change replaces the synchronous readFileSync path for --system-prompt-file / --append-system-prompt-file, but the current diff only changes src/services/api/openaiShim.ts and src/tools.ts; src/main.tsx still imports readFileSync and uses it for both prompt-file flags. That makes the review target and claimed behavior drift from the actual implementation. Please either include the async file-read change, or remove that row and summary claim so the PR description matches the code being merged.

coderabbitai · 2026-06-03T15:00:18Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 437a4609-3635-48e0-9f63-a791b15b20ad

📥 Commits

Reviewing files that changed from the base of the PR and between f210599 and ecfc5f8.

📒 Files selected for processing (1)

src/services/api/openaiShim.test.ts

📜 Recent review details

🧰 Additional context used

📓 Path-based instructions (13)

src/**/*.{ts,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

Use TypeScript with strict mode and ESM imports

Files:

src/services/api/openaiShim.test.ts

{src/commands/**/*.ts,src/services/**/*.ts,src/entrypoints/**/*.ts}

📄 CodeRabbit inference engine (AGENTS.md)

Use chalk for terminal color in CLI code

Files:

src/services/api/openaiShim.test.ts

{src/services/**/*.ts,src/utils/**/*.ts}

📄 CodeRabbit inference engine (AGENTS.md)

Use execa for child processes

Files:

src/services/api/openaiShim.test.ts

{src/integrations/**/*.ts,src/services/**/*.ts}

📄 CodeRabbit inference engine (AGENTS.md)

Test the exact provider/model path you changed when possible for provider modifications

Files:

src/services/api/openaiShim.test.ts

**/*.{ts,tsx,js,jsx,py,json,md}