Add file-size guard on artifact creation via MCP GitHub tools - #523
Add file-size guard on artifact creation via MCP GitHub tools#523totto wants to merge 3 commits into
Conversation
When the LLM hits its output token limit, create_or_update_file silently pushes a truncated stub to GitHub, destroying the original file (the pattern that caused PR Arvo-AI#513 on staging). This adds a pre-flight guard inside the MCP tool wrapper that: 1. Rejects content exceeding a 50 KB absolute cap for both create_or_update_file and push_files (per-entry). 2. For file updates (sha present), fetches the existing file size via get_file_contents and rejects writes smaller than 50% of the original when the original exceeds 10 KB. The guard runs after the HITL confirmation gate but before the MCP server call, so it catches both foreground and background writes. On guard-check failure the write is allowed through (fail-open). Closes Arvo-AI#521 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 16 minutes and 53 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughAdds a file-size safety guard in ChangesGitHub Write File-Size Guard
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/chat/backend/agent/tools/mcp_tools.py`:
- Around line 1433-1439: The _check_file_size_guard function is being called on
raw kwargs before nested arguments are normalized. The guard needs to operate on
the actual arguments that will be used, but currently the code normalizes nested
arguments (unwrapping kwargs["kwargs"]) later in the execution flow. Normalize
and unwrap any nested arguments before calling _check_file_size_guard to ensure
the guard properly validates the file size and content checks on the correct
argument structure, preventing the guard from being bypassed for nested-call
shapes.
- Around line 164-172: The exception handler wrapping the entire loop causes
premature exit when any content item fails JSON parsing, preventing later valid
items from being processed. Move the try-except block to wrap only the JSON
parsing and data extraction for each individual item (around the _json.loads and
subsequent dictionary checks within the for loop), so that a non-JSON item in
the content_items list will skip to the next item instead of breaking out of the
entire loop. This ensures all content items are evaluated before the function
returns.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: cba62495-b3d2-45d8-80be-1f4de9b78c43
📒 Files selected for processing (1)
server/chat/backend/agent/tools/mcp_tools.py
| if server_type == "github" and original_tool_name in ( | ||
| "create_or_update_file", "push_files", | ||
| ): | ||
| try: | ||
| guard_msg = _check_file_size_guard( | ||
| original_tool_name, kwargs, _mcp_manager, run_async_in_thread, | ||
| ) |
There was a problem hiding this comment.
Normalize nested arguments before invoking the guard.
At Line 1433, the guard runs on raw kwargs, but at Line 1456 the wrapper unwraps legacy nested args (kwargs["kwargs"]). That means the guard can be bypassed for nested-call shape, including content/sha checks.
Suggested fix
if server_type == "github" and original_tool_name in (
"create_or_update_file", "push_files",
):
try:
+ guard_kwargs = kwargs.get("kwargs") if isinstance(kwargs.get("kwargs"), dict) else kwargs
guard_msg = _check_file_size_guard(
- original_tool_name, kwargs, _mcp_manager, run_async_in_thread,
+ original_tool_name, guard_kwargs, _mcp_manager, run_async_in_thread,
)
if guard_msg:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/chat/backend/agent/tools/mcp_tools.py` around lines 1433 - 1439, The
_check_file_size_guard function is being called on raw kwargs before nested
arguments are normalized. The guard needs to operate on the actual arguments
that will be used, but currently the code normalizes nested arguments
(unwrapping kwargs["kwargs"]) later in the execution flow. Normalize and unwrap
any nested arguments before calling _check_file_size_guard to ensure the guard
properly validates the file size and content checks on the correct argument
structure, preventing the guard from being bypassed for nested-call shapes.
Move the try/except inside the loop so a JSON parse failure on one content item does not abort processing of subsequent items. Also move the `import json` out of the loop body. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Complexity was 33 (max allowed: 15). Extract the create_or_update_file logic into _guard_create_or_update_file() and the push_files loop into _guard_push_files(), leaving _check_file_size_guard as a dispatcher. Complexity after refactor: dispatcher ~2, each helper ≤13. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
|
hey @totto, can you answer the coderabbit comments please? Thanks (same thing for your other PRs) |
| ) -> str | None: | ||
| """Return an error string if the write should be blocked, else ``None``.""" | ||
| if original_tool_name == "create_or_update_file": | ||
| return _guard_create_or_update_file(kwargs, manager, run_async) |
There was a problem hiding this comment.
The guard reads kwargs.get("content") but at invocation time the LLM can pass args nested inside kwargs["kwargs"] (the wrapper unwraps this at line 1326 below). Move the guard after the actual_kwargs extraction, or unwrap before calling _check_file_size_guard. Otherwise the guard silently passes everything on the nested-args shape.
| # --------------------------------------------------------------------------- | ||
| # File-size guard for GitHub write MCP tools (issue #521) | ||
| # --------------------------------------------------------------------------- | ||
| # When the LLM's output is silently truncated, create_or_update_file pushes |
There was a problem hiding this comment.
50 KB cap is too low. Our codebase has legitimate files in the 50-100 KB range (large config YAMLs, generated schemas). A user doing create_or_update_file on those will get blocked every time. Consider 100 KB or 150 KB — the real danger (truncated LLM output) is more like 5-15 KB stubs overwriting 80 KB files, which the ratio check already catches.
| ) | ||
|
|
||
| owner = kwargs.get("owner") | ||
| repo = kwargs.get("repo") |
There was a problem hiding this comment.
get_file_contents MCP call here has no timeout. If the GitHub API is slow this blocks the entire tool execution indefinitely. Pass a timeout to run_async or wrap in asyncio.wait_for. Also — this fires on every update to a file >10 KB, adding latency to the hot path. Consider caching or making the ratio check opt-in via an env var.
|
|
||
|
|
||
| def _guard_push_files(kwargs: dict) -> str | None: | ||
| """Size guard for ``push_files``: absolute cap per file entry.""" |
There was a problem hiding this comment.
The import json as _json inside the loop body runs on every iteration. Move it to module-level or at least outside the loop. Also the outer try/except wraps the entire loop — if the first content item is a status text line (not JSON), the json.loads raises and the loop exits without checking remaining items that might have the metadata. Narrow the try/except to the parse line.
|
Hi! Thanks for your contribution. Before we can merge this, we need you to sign our Contributor License Agreement (CLA) for legal purposes. This is a one-time requirement for external contributors — it ensures that contributions are properly licensed and that both parties are protected. I'll send the document separately. Once signed, we're good to go on this and any future PRs. |
2 participants
Summary
When the LLM hits its output token limit,
create_or_update_filesilently pushes a truncated stub to GitHub, destroying the original file (the pattern that caused PR #513 on staging). This adds a pre-flight guard inside the MCP tool wrapper that:create_or_update_fileandpush_files(per-entry)shapresent), fetches the existing file size viaget_file_contentsand rejects writes smaller than 50% of the original when the original exceeds 10 KBThe guard runs after the HITL confirmation gate but before the MCP server call, so it catches both foreground and background writes.
Test plan
create_or_update_filewith content > 50 KB is rejectedcreate_or_update_fileupdating a 20 KB file with 5 KB content (shapresent) is rejected with ratio errorcreate_or_update_filecreating a new file (nosha) is not subject to ratio checkpush_fileswith a per-entry > 50 KB is rejectedget_file_contentstimeout) allows the write throughCloses #521
Summary by CodeRabbit