Repository navigation
feat: קריאת טווח שורות מקבלת תקרה משלה (10MB) במקום ה-500KB - #3320
Merged
Merged
Conversation
… cap codekeeper_get_repo_file opened with "(max 500KB; binary files return metadata only)" and, two sentences later, "read only that range instead of the whole file". Read together those say the range is how you handle a file that is too big. It is not: the size check runs in get_file_at_commit before the content is decoded and sliced, so a file over the cap returns too_large with lines just as without it. That mattered more here than in prose, because this string is what the agent reads to decide how to call the tool. An agent that got too_large had the lines sentence sitting in the same paragraph and would plausibly retry with a range, spending a second call on the identical answer. The cap now closes the description instead of opening it, so it reads as a constraint on the tool rather than as context for the range. Reordering alone still leaves the inference available, so the limit now says out loud that it applies to the file and not to the range. docs/mcp-server.rst already carried this caveat in the mcp-line-range section. It was missing only where the agent looks, which is the half that acts on it. Verified by mutation: restoring the old order fails, and reordering without the explicit sentence fails too. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0154RpRM1aCgLXCwUqDgywYp
|
ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Sorry @amirbiron, you've used your own review budget of 250,000 diff characters for the last 7 days.
You can request another review in 1 day and 15 hours by commenting @sourcery-ai review. Upgrade to get a review now.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Contributor
🧯 Dangerous deletes guard reportPolicy: see .cursorrules — dangerous deletions are blocked unless wrapped safely. Summary:
Flagged findings (file:line:snippet): Excluded matches (by path pattern) |
Contributor
Reviewer's GuideUpdates the runtime MCP tool description to explicitly distinguish file-size enforcement from line-range reads, and adds a regression test covering both ordering and wording without changing tool behavior. File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughהתיאור של Changesתיאור codekeeper_get_repo_file
Estimated code review effort: 1 (Trivial) | ~5 דקות Merge Risk: ⚪ Minimal · up to This localized documentation change clarifies that ranged reads do not bypass the 500KB file limit and does not alter tool behavior, data, or deployment. No actionable merge-blocking risk remains beyond normal checks and review. Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Contributor
⏱️ Performance report(No performance test durations collected. Mark tests with |
Contributor
📖 Documentation PreviewThe documentation has been built successfully!
To view locally:
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 issue found across 2 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="tests/test_mcp_server_build.py">
<violation number="1" location="tests/test_mcp_server_build.py:373">
P3: This test reads the description through the private `mcp._tool_manager` attribute, unlike every other test in the file which uses the public `await mcp.list_tools()`. The admin repo tool is filtered out of `list_tools()` in the fail-closed non-admin context, which is why the private API was used. Prefer the public path used elsewhere (e.g. set `mcp._request_is_admin = lambda: True` then `await mcp.list_tools()` as `test_repo_tools_visible_to_admin` does) to avoid coupling the test to FastMCP's internal `_tool_manager`.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| ``mcp-line-range``). הוא חסר היה בדיוק במקום שהסוכן קורא. | ||
| """ | ||
| mcp = build_mcp(_FakeBackend(), repo_backend=_FakeRepoBackend()) | ||
| description = mcp._tool_manager.get_tool("codekeeper_get_repo_file").description |
Contributor
There was a problem hiding this comment.
P3: This test reads the description through the private mcp._tool_manager attribute, unlike every other test in the file which uses the public await mcp.list_tools(). The admin repo tool is filtered out of list_tools() in the fail-closed non-admin context, which is why the private API was used. Prefer the public path used elsewhere (e.g. set mcp._request_is_admin = lambda: True then await mcp.list_tools() as test_repo_tools_visible_to_admin does) to avoid coupling the test to FastMCP's internal _tool_manager.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/test_mcp_server_build.py, line 373:
<comment>This test reads the description through the private `mcp._tool_manager` attribute, unlike every other test in the file which uses the public `await mcp.list_tools()`. The admin repo tool is filtered out of `list_tools()` in the fail-closed non-admin context, which is why the private API was used. Prefer the public path used elsewhere (e.g. set `mcp._request_is_admin = lambda: True` then `await mcp.list_tools()` as `test_repo_tools_visible_to_admin` does) to avoid coupling the test to FastMCP's internal `_tool_manager`.</comment>
<file context>
@@ -352,3 +352,26 @@ async def test_save_file_description_matches_what_the_tool_actually_does():
+ ``mcp-line-range``). הוא חסר היה בדיוק במקום שהסוכן קורא.
+ """
+ mcp = build_mcp(_FakeBackend(), repo_backend=_FakeRepoBackend())
+ description = mcp._tool_manager.get_tool("codekeeper_get_repo_file").description
+
+ assert description.index("500KB") > description.index("lines=[start, end]")
</file context>
… one codekeeper_get_repo_file refused any file over 500KB, lines or not, because the size check runs in get_file_at_commit before the content is decoded and sliced. webapp/app.py at 805,594 bytes — the file most worked on in this repo — was simply unreadable through the tool (#3317). A range read is judged against its own ceiling now. The whole-file path is untouched: max_size is passed only when lines is given, so the mirror service default stays the single source of truth for 500KB and the webapp caller keeps the behaviour it has today. The ceiling is replaced, not removed. The blob is still read and decoded whole before slicing, so "no limit" would put a pathological file in RAM as-is; measured, read plus decode costs about three times the file size, so 10MB bounds the peak near 30MB while leaving twelve times the headroom over the largest file anyone actually reads. That number is MCP-layer policy, which is why it lives in repo_backend rather than in the mirror service. Raising the ceiling also made range validation worth moving. It used to run after the read, which cost nothing when a large file was refused anyway; now a malformed range like [9, 2] would read and decode up to 10MB before being rejected. The check is pure and cheap, so it runs first. The description and the docs both said the opposite of this and are corrected. They now name both ceilings rather than one, because an agent that gets too_large needs to know whether lines will help it or whether the file is past every limit. Verified against a real mirror: webapp/app.py returns the requested lines and still returns too_large without them; the 11.7MB source map is refused with max reporting the ceiling that judged it. Five mutations fail, including passing the ceiling always, removing it entirely, and moving validation back after the read. Closes #3317 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0154RpRM1aCgLXCwUqDgywYp
amirbiron
changed the title
17 of 20 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
✨ תיאור קצר
codekeeper_get_repo_fileסירב לכל קובץ מעל 500KB, עםlinesובלעדיו, כי בדיקת הגודל רצה ב-get_file_at_commitלפני שהתוכן מפוענח ונחתך.webapp/app.pyבן 805,594 הבייטים — הקובץ שהכי הרבה עובדים עליו כאן — היה פשוט בלתי קריא דרך הכלי.מעכשיו קריאת טווח נשפטת מול תקרה משלה. סוגר את #3317.
📦 שינויים עיקריים
איך זה מומש — הדרך הפשוטה מהאישו
max_sizeכבר היה פרמטר שלget_file_at_commitעם ברירת מחדל. אז השינוי כולו יושב ב-mcp_server/repo_backend.get_file:בלי
linesהפרמטר לא מועבר כלל, כך שברירת המחדל של שירות המראה נשארת מקור האמת היחיד ל-500KB ולא נוצר מספר שני שצריך לסנכרן.services/git_mirror_service.pyלא נגע,webapp/routes/repo_browser.pyלא נגע, והעוזר המשותף שחותך את הטווח נשאר במקומו — כלומר האינווריאנטה מ-#3316, ששני הכלים חולקים סמנטיקה מדויקת, מוחזקת.למה תקרה אחרת ולא ביטול
הבלוב עדיין נקרא ומפוענח במלואו לפני החיתוך — זה בדיוק מה שמדדתי באישו. "בלי תקרה" פירושו שקובץ פתולוגי בריפו יגיע ל-RAM כמו שהוא.
webapp/app.pymd_preview.bundle.jsקריאה ופענוח עולים בערך פי שלושה מגודל הקובץ. 10MB חוסמים את השיא סביב 30MB, ונותנים פי 12 מרווח מעל הקובץ הגדול ביותר שבאמת קוראים (
app.py). המספר הזה הוא החלטת מדיניות של שכבת ה-MCP — לכן הוא ב-repo_backendולא בשירות המראה, והוא שינוי של שורה אחת אם תרצה אחרת.ולידציית הטווח עברה לפני הקריאה
הרמת התקרה הפכה את זה לעלות אמיתית: טווח פגום כמו
[9, 2]היה גורם לקריאה ולפענוח של עד 10MB רק כדי להיפסל בסוף. כשהתקרה הייתה 500KB זה לא היה משנה, כי קובץ גדול נפסל ממילא. הבדיקה טהורה וזולה, אז היא רצה ראשונה — וזה חלק מהשינוי, לא שיפור נפרד.🧪 בדיקות
git clone --mirrorשל הריפו הזה), ולא מול דמה:webapp/app.pyעםlines=[5431, 5436]←status: ok,total_lines: 20036, התוכן נכוןlines←status: too_large,max: 512000md_preview.bundle.js.map(11.7MB) עםlines←too_large,max: 10485760lines←status: binary, מטא-דאטה בלבדapp.py: 5.4MBtests/test_mcp_additive_params.py, עם דמה שרושמת אתmax_sizeשהועבר בפועלmax_sizeכלל (חזרה למצב שב-קבצים מעל 500KB אינם נקראים דרך ה-MCP כלל — גם לא בקריאת טווח שורות #3317)tests/test_mcp_*.py— 528 עוברים, אפס נופליםpy_compileואורך שורות מולmax-line-length=120ובדרך תפסתי טסט שלי ששיקר: קראתי לו
..._before_the_cap_is_liftedבזמן שהקובץ כן נקרא ורק אחר כך הטווח נפסל.mirror.calls == []הוא מה שהפך את ההבטחה לבדיקה.🧪 בדיקות נדרשות ב‑PR
📝 סוג שינוי
✅ צ'קליסט
docs/mcp-server.rst(הסייג בסעיףmcp-line-rangeאמר את ההפך ותוקן; טבלת הכלים),mcp_server/README.md, ומחרוזת התיאור של הכליdocs/mcp-server.rst, סעיפיםmcp-line-rangeוטבלת הכלים🧩 השפעות/סיכונים
קריאה קיימת אינה משתנה. בלי
linesההתנהגות זהה בבייט, ולוובאפ אין שום שינוי — יש על שניהם טסט.קריאת טווח מקובץ גדול צורכת עכשיו עד ~30MB שיא רגעי במקום להיכשל מיד. בשירות שמשרת משתמש אחד זה זניח, וזו בדיוק ההחלטה שהתקבלה באישו אחרי שהמספרים נמדדו.
לקוח MCP שהיה מחובר לפני הדיפלוי לא יראה את התיאור החדש עד שיתחבר מחדש — אבל ההתנהגות תעבוד לו בכל מקרה.
🧯 סיכון / החזרה לאחור (Rollback)
revert של הקומיטים מחזיר את המצב במלואו. אין מיגרציה ואין שינוי מצב מתמשך. ולחלופין,
RANGE_READ_MAX_BYTES = 512_000מחזיר את ההתנהגות הישנה בשורה אחת.🔗 קישורים
lines🤖 Generated with Claude Code
https://claude.ai/code/session_0154RpRM1aCgLXCwUqDgywYp