Repository navigation
Conversation
- New tool to make HTTP requests (GET, POST, PUT, PATCH, DELETE, HEAD, OPTIONS) - Supports custom headers, query params, request body - Includes timeout handling with AbortController - Follows redirect handling P2 fix: add setCleanupTimeout helper to cleanupRegistry for cleanup-safe timers
jatmn
left a comment
There was a problem hiding this comment.
Findings
-
[P1] Register the HTTP tool so users can actually call it
src/tools.ts:182
The PR addsHttpRequestTool, but it never imports it intosrc/tools.tsor adds it togetAllBaseTools(). That registry is whatgetTools()uses for the CLI, SDK, default preset, permission filtering, and ToolSearch exposure, so this ships new files and tests but no user-visibleHttpRequesttool. Please wire the tool into the base tool list, and decide whether it should be immediately available or deferred likeWebFetchTool. -
[P2] Re-save the new TypeScript files as UTF-8 text
src/tools/HttpRequestTool/HttpRequestTool.ts:1
The newHttpRequestToolsource, test, and prompt files are committed as UTF-16 LE, so Git treats them as binary (git diff --numstatreports- -for all three files andgh pr diffcannot show their contents). That makes the code hard to review in GitHub and can break normal text tooling. Please re-save these files as UTF-8, matching the rest of the repo, before merging. -
[P2] Enforce the response size limit while reading, not after
resp.text()
src/tools/HttpRequestTool/HttpRequestTool.ts:83
The prompt says the response size is limited to prevent memory issues, but the implementation first reads the entire response withawait resp.text()and only then truncates toMAX_BODY_CHARS. A large or streaming response can still consume unbounded memory before truncation happens. Please read fromresp.bodywith a cap, abort once the limit is reached, and add a focused test that proves an oversized response is stopped before the full body is buffered.
|
Thanks for putting this together — a first-class HTTP tool is a genuinely useful addition, and the API surface you've designed reads cleanly. I went through it independently alongside @jatmn's review, and his points all hold: the tool isn't yet wired into One more item worth raising before the next pass, since it's the most important one: No rush — once the registration, encoding, buffering, and the SSRF guard are addressed, I'd be glad to take another look. Appreciate the work here. |
Closing as Bulk Tool AdditionThis PR is being closed because it follows the same pattern as your previous bulk tool PRs (#1139, #1131, #1130, #1129, #1128) which were also closed. Why we're closing:
How to contribute tools properly:
If you believe these tools are valuable, please open a discussion issue first. |
|
Closed — bulk tool addition without prior maintainer discussion. |
#1201) * fix(api): tighten reasoning_content heuristic to prevent false-positive provider configuration leaks (issue #859) * fix(api): segment-boundary-aware reasoning_content heuristic (issue #859) * test: add regression tests for segment-boundary heuristic Adds 10 tests covering: - Custom alias (my-deepseek-rag) should NOT infer preserveReasoningContent - Provider paths (openrouter/deepseek/..., accounts/fireworks/...) MUST infer it - Standard model names (deepseek-chat) should infer it - Kimi/Moonshot models should infer correctly - Non-matching models (gpt-4o, claude) should not infer Addresses jatmn's review feedback: 'add regression coverage for the segment-boundary heuristic' * fix: remove unsupported provider property from test calls (PR #1195 review)
Summary
Impact
Testing
Notes