fix: resolve tool getter double-invocation, dedup getMergedTools, fix openaiShim errors#1473
Conversation
… fix openaiShim errors Bug fixes: - Cache lazy require() getter results in tools.ts to avoid double-invocation - getMergedTools() now uses uniqBy to deduplicate like assembleToolPool() - openaiShim error status defaults to 500 instead of 200 - Redaction fallback returns [redacted] instead of unredacted URL - Provider detection uses hostname parsing instead of fragile URL substring matching - Debugger detection now writes error message before exit Optimizations: - Replace O(N*M*K) tool result lookup with Map-based O(N+M) in query.ts - Eliminate response.clone() memory doubling in openaiShim non-streaming path
|
@jatmn I have created multiple PRs as I was asked to. |
thank you! |
jatmn
left a comment
There was a problem hiding this comment.
I found an issue that should be addressed before this is ready.
Findings
-
[P3] Preserve the response body when usage parsing fails
src/services/api/openaiShim.ts:2739
This now consumes every successful non-streaming response withawait response.text()before the normal response conversion path gets to read it, but the response is only recreated afterJSON.parse(bodyText)succeeds. If a provider returns a 2xx non-JSON body, or malformed JSON, the parse throws, the catch swallows it, and callers later see an already-consumed body. For example, the existing non-JSON fallback atsrc/services/api/openaiShim.ts:1923can no longer include the provider's response text in the "unexpected response" error, whereas the oldresponse.clone().json()path left the original body readable. Please recreate theResponseafter readingbodyTexteven when usage parsing fails, or keep using a clone for this diagnostic-only parse. -
[P3] Split or reconcile the overlapping bundled fixes
src/services/api/openaiShim.ts:2739
This PR bundles several unrelated fixes and optimizations, but at least some of the same changes are also present in sibling PRs from this branch set:#1478covers the response-clone optimization and lazy tool getter caching,#1476covers thegetMergedTools()dedupe, and#1479covers the debugger stderr message. That makes it hard to tell which review path should be authoritative, and it raises the risk that a partial merge will either duplicate work or bring in an older version of a change that has been revised elsewhere. Please either narrow this PR to the changes that are not covered by the sibling PRs, or explicitly close/supersede the overlapping PRs so the implementation, PR description, and review target all line up.
Summary
Fixes 5 bugs and adds 2 performance optimizations to existing code. No new features or tools.
Bug Fixes
src/tools.ts:214,217,230require()getter results to avoid double-invocation (getSendMessageTool,getPowerShellTool)src/tools.ts:380-386getMergedTools()now usesuniqByto deduplicate, matchingassembleToolPool()behaviorsrc/services/api/openaiShim.ts:1471src/services/api/openaiShim.ts:236,238[redacted]instead of unredacted URL whenredactSecretValueForDisplayfailssrc/main.tsx:270process.exit(1)Optimizations
src/query.ts:1734-1751Map<tool_use_id, result>for O(N+M) instead of O(N×M×K) nestedfind()src/services/api/openaiShim.ts:2733Responseinstead ofresponse.clone()which doubles memorysrc/services/api/openaiShim.ts:2682new URL().hostnameparsing instead of fragilebaseUrl.includes()substring matchingTesting
bun run build✓node dist/cli.mjs --version→0.15.0✓