Conversation
nikolasdehor
left a comment
nikolasdehor
left a comment
There was a problem hiding this comment.
Clean, well-implemented fix. Using http.MaxBytesReader is the correct idiomatic approach in Go — it actively tears down the connection rather than silently truncating. The test is thorough and the PR description is excellent. One minor note: MaxFetchLimitBytes is exported. If no external packages need it, consider making it unexported (maxFetchLimitBytes). The test is in the same package so it would still have access. LGTM — good work preventing OOM from unbounded fetches.
nikolasdehor
left a comment
nikolasdehor
left a comment
There was a problem hiding this comment.
Good hardening. The key improvement is replacing io.LimitReader (which silently truncates and returns io.EOF) with http.MaxBytesReader (which returns a typed *http.MaxBytesError and actively closes the connection). This gives the tool a clear signal that the content was too large, and allows returning an explicit error message to the LLM.
The error handling correctly uses errors.As to check for *http.MaxBytesError, and the test generates a payload slightly over the limit to verify the error path.
One nit (non-blocking): MaxFetchLimitBytes is exported but only used within the tools package. Consider making it unexported (maxFetchLimitBytes) unless external consumers need to reference it. The test is in the same package so it can access unexported symbols.
LGTM.
pkg/tools/web.go
Outdated
| const ( | ||
| userAgent = "Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/120.0.0.0 Safari/537.36" | ||
| userAgent = "Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/120.0.0.0 Safari/537.36" | ||
| MaxFetchLimitBytes = int64(10 * 1024 * 1024) // 10MB limit |
There was a problem hiding this comment.
This should be in configuration with default 10MB limit
|
@mengzhuo make sense! fixed |
3 participants
📝 Description
Implemented a strict memory limit on the
WebFetchToolresponse body to prevent Out of Memory (OOM) crashes and bandwidth exhaustion when the agent attempts to fetch excessively large files (e.g., massive PDFs, ISOs, or infinite streams).Changes made:
io.LimitReader+io.ReadAllcombo withhttp.MaxBytesReader. This ensures that if the response exceeds the 10MB limit, the tool explicitly drops the TCP connection and returns an explicit*http.MaxBytesError, rather than silently returning a truncated response.TestWebFetchTool_PayloadTooLarge) to dynamically verify the OOM protection mechanism.🗣️ Type of Change
🤖 AI Code Generation
🔗 Related Issue
N/A
📚 Technical Context (Skip for Docs)
io.LimitReaderonly stops reading but allows the connection to remain open, masking the limit as a normalEOF.http.MaxBytesReaderis the idiomatic way to cap memory consumption in Go; it actively aborts the connection upon reaching the threshold, protecting the host's RAM and network resources.🧪 Test Environment
📸 Evidence (Optional)
Click to view Logs/Screenshots