Skip to content

fix: bound read_file allocations — 32MB per-file cap + batch total limit#125

Merged
jamestexas merged 2 commits intomainfrom
fix/p1-bug-cluster
Mar 23, 2026
Merged

fix: bound read_file allocations — 32MB per-file cap + batch total limit#125
jamestexas merged 2 commits intomainfrom
fix/p1-bug-cluster

Conversation

@jamestexas
Copy link
Copy Markdown
Contributor

Summary

  • Reject single-file reads exceeding 32MB with clear error message
  • Cap batch read total content at 32MB — remaining files get per-file error, not OOM
  • Adversarial TDD tests prove both vectors

Test plan

  • TestReadFile_RejectsOversizedContent — 64MB file rejected
  • TestReadFile_BatchRejectsTotalContentOverflow — 10×4MB batch caps at 32MB
  • All existing ReadFile tests still pass
  • task build + task test clean

Closes mache-5c76b3

Adversarial tests prove both single-file and batch-read OOM vectors.
Per-file reads over 32MB are rejected. Batch reads stop accumulating
once total content exceeds 32MB, skipping remaining files.

[mache-5c76b3]
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR hardens the read_file MCP tool against unbounded memory usage by adding explicit per-file and batch-level size limits, with adversarial tests to prevent regressions.

Changes:

  • Enforce a 32MB maximum size for single-file reads in read_file.
  • Add a 32MB total-content cap for batch reads, returning per-file errors once the cap is hit.
  • Add new tests covering oversized single-file reads and oversized batch totals.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File Description
cmd/serve_handlers.go Adds per-file size check and batch total-content limiting logic for read_file.
cmd/serve_test.go Introduces adversarial tests validating oversized single-file rejection and batch total cap behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread cmd/serve_handlers.go
Comment thread cmd/serve_handlers.go Outdated
Comment thread cmd/serve_test.go Outdated
Comment thread cmd/serve_test.go Outdated
- Pre-check ContentSize before reading to avoid allocate-then-reject
- Clarify test comment re: handler allocation vs test setup allocation
- Unmarshal batch results and assert structural invariants (succeeded > 0,
  exactly 1 cap trigger, skipped > 0, all paths accounted for)

[mache-5c76b3]
@jamestexas jamestexas merged commit 701d0a7 into main Mar 23, 2026
14 checks passed
@jamestexas jamestexas deleted the fix/p1-bug-cluster branch March 23, 2026 22:00
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.

2 participants