fix(mcp): העלאה שהשתבשה באחסון הזמני אינה נשמרת (upload_corrupted), ותיאור upload_id בהוספה אומר מה ה-hash מוכיח - #3515
Conversation
…ספה אומר מה ה-hash מוכיח content_changed משווה את הטקסט שנשלף מ-mcp_uploads למה שנכתב לקובץ, ולכן טקסט שהשתבש בין ההעלאה לשמירה היה נשמר בשקט, עם content_changed: false. - _consume_upload (handlers.py), העוזר שכל כלי שמקבל upload_id צורך דרכו, משווה לפני המחיקה את ה-hash של הטקסט שנשלף ל-content_sha256 שנשמר עם ההעלאה כשהגיעה. שונים, או שמה שנשמר אינו 64 ספרות הקס: upload_corrupted, שום דבר לא נכתב, וההעלאה נמחקת (discard_upload). התשובה אינה תלויה בתוצאת המחיקה. - backend: find_upload מחזיר גם את ה-hash השמור; _delete_live_upload הוא המקום היחיד שמוחק העלאה, ו-discard_upload רושם שורת ERROR אחת בלי המזהה ובלי התוכן, ולא consumed. - תיאור upload_id: המשפט על ה-hash היה משותף לשני הכלים והבטיח גם בהוספה ש-file.content_sha256 יהיה שווה ל-hash של ההעלאה. בהוספה הוא של הקובץ כולו, ומה שמראה שהטקסט נכנס בשלמותו הוא content_changed: false. עכשיו לכל כלי המשפט שלו (_UPLOAD_HASH_AFTER_SAVE / _UPLOAD_HASH_AFTER_APPEND). - טסטים: המשפט בתיאור מול מה שהכלי מחזיר, העלאה משובשת בשני הכלים (חמש צורות שיבוש), מחיקה שלא מוצאת כלום, שני חוטים, אחסון שלא עונה למחיקה, שורת הלוג בתהליך נקי, שומר מבני על המקור, ומונגו אמיתי (הוספה ושיבוש). - תיעוד: mcp-server.rst, mcp_server/README.md, whats-new. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PLrwvW1zc6nhJfG2Mtr4wi
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
ⓘ 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}
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 4 days and 17 hours by commenting @sourcery-ai review. Upgrade to get a review now.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
🧯 Dangerous deletes guard reportPolicy: see .cursorrules — dangerous deletions are blocked unless wrapped safely. Summary:
Flagged findings (file:line:snippet): Excluded matches (by path pattern) |
Reviewer's GuideThe PR closes the temporary-upload integrity gap by validating the fetched text against the SHA-256 recorded at upload time before the atomic consume/delete gate, returning a retryable Sequence diagram for temporary upload integrity validationsequenceDiagram
participant Agent
participant Tool as MCP tool
participant Handler as _consume_upload
participant Backend
participant Storage as Upload storage
Agent->>Tool: call_tool(upload_id)
Tool->>Handler: _load_upload()
Handler->>Backend: find_upload()
Backend->>Storage: find_one()
Storage-->>Backend: text, bytes, content_sha256
Backend-->>Handler: _PendingUpload
Handler->>Handler: _upload_integrity_problem()
alt hash mismatch or invalid stored hash
Handler->>Backend: discard_upload()
Backend->>Storage: _delete_live_upload()
Backend-->>Handler: cleanup result
Handler-->>Tool: upload_corrupted
Tool-->>Agent: retryable error, nothing written
else hash matches
Handler->>Backend: consume_upload()
Backend->>Storage: _delete_live_upload()
Storage-->>Backend: atomic delete result
Backend-->>Handler: consumed
Handler-->>Tool: continue file operation
Tool-->>Agent: save or append result
end
Flow diagram for upload consumption integrity gateflowchart TD
A[Load upload with find_upload] --> B{_upload_integrity_problem}
B -->|stored_hash_invalid or hash_mismatch| C[discard_upload]
C --> D[_delete_live_upload]
D --> E[Return upload_corrupted]
E --> F[No file write; upload again]
B -->|None| G[consume_upload]
G --> H[_delete_live_upload atomic gate]
H --> I{Deleted by this request?}
I -->|yes| J[Perform save or append]
I -->|no| K[Return upload_not_found]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (3)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughהעלאות MCP כוללות כעת hash שמור, שנבדק מול הטקסט שנשלף לפני השימוש בו. אם הבדיקה נכשלת, המערכת מחזירה Changesשלמות העלאות MCP
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant MCPTool
participant LoadUpload as _load_upload
participant ProductionBackend
participant MongoDB
MCPTool->>LoadUpload: upload_id ושם הכלי
LoadUpload->>ProductionBackend: find_upload
ProductionBackend->>MongoDB: שליפת הטקסט וה-hash השמור
MongoDB-->>ProductionBackend: נתוני ההעלאה
ProductionBackend-->>LoadUpload: טקסט ו-hash
alt ה-hash אינו תקין או אינו תואם
LoadUpload->>ProductionBackend: discard_upload
ProductionBackend->>MongoDB: ניסיון למחיקת ההעלאה
LoadUpload-->>MCPTool: upload_corrupted, ללא כתיבה
else ה-hash תואם
LoadUpload-->>MCPTool: תוכן מאומת להמשך עיבוד
end
Merge Risk: ⚪ Minimal · up to Upload integrity validation and the documented save/append hash semantics are consistent. No actionable merge-blocking risk was identified; merge after normal checks pass. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The change strengthens upload integrity checks while preserving write permission, ownership, expiry, and single-use controls. No newly introduced security concern was established. Production interruption and deployment behavior remain unverified. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 65.38% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 52 functions across 5 files. (3 skipped: 3 unsupported.)
✨ Finishing Touches 💡 1📝 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. hash נשלף, נבדק מול התוכן Comment |
⏱️ Performance report(No performance test durations collected. Mark tests with |
📖 Documentation PreviewThe documentation has been built successfully!
To view locally:
|
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (7 files)
Previous Review Summary (commit 07a6743)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 07a6743)Status: No Issues Found | Recommendation: Merge Files Reviewed (8 files)
Reviewed by nemotron-3-ultra-550b-a55b:free · Input: 0 · Output: 0 · Cached: 0 |
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}
There was a problem hiding this comment.
All reported issues were addressed across 8 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
… הטקסט שני ממצאי ריוויו על #3515: - הבדיקה ישבה רק ב-_consume_upload, אחרי השערים של הכלי. העלאה משובשת שפגשה שער — התקרה, file_exists, existence_check_unavailable, conflict, not_found, תוכן ריק, expected_content_sha256 פגום — קיבלה את הסירוב של השער, החלטה על טקסט שלא נשלח, ונשארה חיה, בלי שורת ERROR, עד שפקעה. עכשיו _load_upload, שכל כלי עם upload_id שולף דרכו, משווה את ה-hash מיד אחרי השליפה: משובשת ← discard_upload ו-upload_corrupted, לפני כל שער. _PendingUpload נבנית רק שם, אחרי הבדיקה, ו-_consume_upload חזר להיות השער בלבד. זה לא TOCTOU: מה שנשמר הוא הטקסט שנבדק. - whats-new אמר "ההעלאה נמחקת" כעובדה. המחיקה של העלאה משובשת היא ניקיון: כשהאחסון לא עונה היא פוקעת ב-TTL, והתשובה אינה תלויה בזה. התיקון גם ב-mcp-server.rst וב-README. טסטים: כל שער, עם בקרה (העלאה שלמה מקבלת את סירוב השער ונשארת) ועם העלאה משובשת (upload_corrupted, השלכה אחת עם הכלי והסיבה, רק היא נמחקה, שום דבר לא נכתב). על הקוד הקודם (07a6743) כל המקרים ענו את סירוב השער. השומר המבני נקרא עכשיו test_every_upload_is_checked_as_it_is_read_and_consumed_at_one_gate: discard_upload רק מ-_load_upload, ו-_PendingUpload לא נבנית בשום מקום אחר. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PLrwvW1zc6nhJfG2Mtr4wi
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
Code Review ✅ Approved🔴 High risk · Hash validation now rejects corrupted uploads before they can be saved as files. Adds SHA-256 integrity validation before consuming MCP uploads to prevent silently saving corrupted content, and clarifies that upload hashes prove different outcomes for OptionsAuto-apply is off → Gitar will not commit updates to this branch. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Gitar |
2 participants
✨ תיאור קצר
content_changedמשווה את הטקסט שנשלף מ-mcp_uploadsלמה שנכתב לקובץ. לכן טקסט שהשתבש באחסון הזמני, בין ההעלאה לשמירה, היה נשמר בשקט, עםcontent_changed: false: מה שנכתב שווה למה שנשלף, ושניהם כבר משובשים._load_uploadמשווה את ה-hash של הטקסט שנשלף ל-content_sha256שנשמר עם ההעלאה כשהגיעה. לא תואם ←upload_corrupted, גם כשהתקרה,file_existsאוconflictהיו נכשלים: שום דבר לא נכתב, ההעלאה נמחקת (ואם האחסון לא ענה למחיקה, היא פוקעת ב-TTL), והתשובה אומרת להעלות שוב. זה מכסה את שני הכלים, בלי שהסוכן צריך להשוות כלום. (בסבב הראשון הבדיקה רצה רק לפני המחיקה. ראו "סבב 2" למטה.)upload_idבהוספה: התיאור היה משותף לשני הכלים, והבטיח גם ב-codekeeper_append_fileש-file.content_sha256יהיה שווה ל-hash של ההעלאה. בהוספה זה ה-hash של הקובץ כולו. סוכן שבודק לפי התיאור היה מסיק שההוספה נכשלה, מעלה שוב, ומוסיף את הטקסט פעמיים. עכשיו התיאור של ההוספה אומר שמה שמראה שהטקסט נכנס בשלמותו הואcontent_changed: false, ושל השמירה נשאר כמו שהיה.📦 שינויים עיקריים
find_uploadקורא שדה אחד נוסף שכבר נשמר מאז feat(mcp): העלאת תוכן ארוך בלי לעבור דרך המודל — PUT /api/agent/upload ו-upload_id #3502פירוט:
mcp_server/handlers.py:_upload_integrity_problem—"stored_hash_invalid"כשמה שנשמר אינו 64 ספרות הקס (_SHA256_HEXהקיים),"hash_mismatch"כשה-hash של הטקסט שנשלף שונה. הבדיקה רצה ב-_load_upload, מיד אחרי השליפה ולפני כל שער, ועל כישלון שם גםdiscard_upload. ההשוואה מדויקת, כי הערך נכתב רק מ-hexdigest._upload_corruptedהוא הסירוב, וה-hint לא טוען שההעלאה נמחקה._PendingUploadנבנית רק אחרי הבדיקה, ו-_consume_uploadהוא השער בלבד.mcp_server/backend.py:find_uploadמחזיר גם אתcontent_sha256השמור, בלי בדיקה. הבדיקה של ה-handler._delete_live_uploadהוא המקום היחיד שמוחק העלאה, ו-consume_uploadעובר דרכו.discard_uploadמשליך העלאה משובשת דרך אותה מחיקה. זה ניקיון ולא השער: שגיאת מסד נרשמת ולא עולה, ובאג עולה כמו שהוא.ERRORאחת בלי המזהה ובלי התוכן, עם הסיבה ועם מה שהמחיקה עשתה (deleted: yes / no / unknown).consumedלא נרשם.mcp_server/server.py:_UPLOAD_HASH_AFTER_SAVEו-_UPLOAD_HASH_AFTER_APPEND, המשפט על ה-hash לכל כלי בנפרד._upload_id_param_docמקבל אותו כפרמטר.תיעוד:
docs/mcp-server.rstעודכן בכמה מקומות:content_changedמכסה, ומה הבדיקה מכסה.upload_corruptedבטבלת הסירובים וב"פתרון תקלות".ERRORב"מה נרשם בלוג".בנוסף
mcp_server/README.md,docs/whats-new.rst, וה-docstring שלcreate_upload.🧪 בדיקות
המספרים בסעיף הזה הם של הסבב הראשון (
07a6743). המספרים של סבב 2 נמצאים בסעיף שלו, למטה.הטסטים החדשים על הקוד שלפני התיקון (ב-
git worktreeנפרד, על7440abe): 22 נכשלו, ו-146 עברו. הנכשלים: המשפט בתיאור מול מה שהכלי מחזיר (ההוספה), העלאה משובשת בשני הכלים ובחמש צורות שיבוש, מחיקה שלא מוצאת כלום, שני חוטים על העלאה משובשת, אחסון שלא עונה למחיקה, שורת הלוג, ההשלכה ב-backend, החוזה שלfind_upload, השומר המבני, ושיבוש באוסף האמיתי.טסטים שעוברים גם על הקוד הישן, ובכוונה, כי הם מקבעים התנהגות שכבר הייתה נכונה:
test_without_upload_id_the_upload_storage_is_not_touched, כלומר שבליupload_idשום דבר לא משתנה.content_changed: false.כל אחד מהם אומר את זה בגוף שלו.
שומר מבני (בסבב 2 שמו
test_every_upload_is_checked_as_it_is_read_and_consumed_at_one_gate): על הקוד הישן הוא נופל רק כי המחיקה עוד לא עברה ל-_delete_live_upload, כלומר שם ולא עקיפה אמיתית. הראיה שהוא מסוגל ליפול היא המוטציות.11 מוטציות על הקוד הסופי, כולן נתפסו (ב-worktree נפרד; הקוד בלי מוטציה — 168 עברו):
['upload_corrupted', 'upload_not_found'])save_fileצורך בלי העוזרdeleted: no)find_uploadבלי ה-hash השמורconsumedריצות:
tests/test_mcp_uploads.pyו-tests/test_mcp_content_sha256_real_mongo.py: 168 עברו מול mongod 8.0.15 מקומי. בלעדיו קובץ המונגו מדולג, וב-CI הוא מדולג.tests/test_mcp_*.py,tests/test_doc*.py,-n 4): 2,129 עברו, אפס דולגו. שני טסטי התיעוד שדורשיםdocutilsרצו אחרי שהותקן בסביבה המקומית.ERROR:mcp_server.backend:mcp upload: codekeeper_save_file refused a corrupted upload (hash_mismatch; 20 bytes, 11 chars) for user 4242 — deleted: yesdocs/mcp-server.rstו-docs/whats-new.rstנבדקו ב-docutils, עם התפקידים של Sphinx מדומים: אין שגיאות ברמה 3 ומעלה. בנייה מלאה לא הורצה. RTD תופס.מה לא אומת:
call_toolשל שרת ה-MCP האמיתי. השינוי לא נוגע בתעבורה או באימות.upload_idאמורות להתנהג בדיוק כמו קודם, והתיאור החדש אמור להופיע ב-tools/list.🧪 בדיקות נדרשות ב‑PR
📝 סוג שינוי
🔀 סטיות מהתוכנית
discard_uploadולא ב-consume_upload: כך הלוג לא אומר "consumed" על העלאה שלא נשמרה. שתיהן עוברות באותה מחיקה,_delete_live_upload, ולכן יש אתר מחיקה אחד.🔁 סבב 2 — שני ממצאים של cubic (
b643d80)מה השתנה למשתמש: העלאה שהשתבשה באחסון הזמני נענית עכשיו
upload_corruptedתמיד. זה נכון גם כשהיא הייתה נתקלת קודם בבדיקה אחרת של הכלי, כמו התקרה,file_exists,conflictאוnot_found. היא נמחקת ונרשמת בשורת ה-ERROR. עד עכשיו היא קיבלה במקרים האלה את הסירוב של הבדיקה האחרת, כלומר החלטה על טקסט שלא נשלח (למשלcode_too_largeעל אורך שאינו של ההעלאה), ונשארה חיה בלי לוג עד שפקעה.handlers.py) — תוקן. הבדיקה עברה ל-_load_upload, מיד אחרי השליפה._PendingUploadנבנית רק שם, אחרי הבדיקה, ו-_consume_uploadחזר להיות השער בלבד. סטייה מההצעה: לא השארתי בדיקה שנייה ב-_consume_upload"כחגורת ביטחון". אחרי הבדיקה בשליפה, אף טסט לא יכול להפיל שורה כזו, ובמקומה השומר המבני מקבע שאין מקום אחר שבונה_PendingUpload. זה לא TOCTOU: מה שנשמר הוא הטקסט שנבדק, והשער מכריע רק שההעלאה נצרכת פעם אחת.whats-new.rst) — תוקן. "ההעלאה נמחקת (ואם האחסון לא ענה למחיקה, היא פוקעת ב-TTL; התשובה אינה תלויה בזה)". אותו סייג נוסף גם לשורה בטבלת הסירובים, ל"פתרון תקלות" ול-mcp_server/README.md.בדיקות:
טסט חדש לכל שער: תוכן ריק, התקרה, בירור קיום שלא ענה ו-
file_existsבשמירה; תוכן ריק,expected_content_sha256פגום,not_found,conflictוהתקרה בהוספה.upload_corrupted,discard_uploadפעם אחת עם הכלי והסיבה, רק היא נמחקה, ושום דבר לא נכתב.על הקוד של הסבב הראשון (
07a6743, worktree נפרד): 10 נכשלו. אלה כל מקרי השערים, כל אחד עם סירוב השער שלו בדיוק, והשומר המבני. 108 עברו. טסט שורת הלוג עובר גם על07a6743, כי הוא מאפיין את השורה ולא את המיקום.7 מוטציות, כולן נתפסו:
save_fileששולף בלי_load_upload: 22._PendingUploadשנבנית ב-save_file: השומר המבני."הבדיקה במקומה הקודם" היא הריצה על
07a6743.ריצות:
✅ צ'קליסט
AI-MAP.md←docs/mcp-server.rst(mcp-uploads,mcp-content-sha256, "פתרון תקלות"),docs/doc-authoring.rst,docs/versioning-stable-anchors.rst.bugbot-rules/return-value-failure-unchecked.mdbugbot-rules/race-toctou.mdbugbot-rules/external-input-isinstance.mdbugbot-rules/prose-restates-code-fact.mdbugbot-rules/derived-field-added-to-one-writer.mdbugbot-rules/write-from-cached-read.mdbugbot-rules/silent-fallback-to-worse-path.mdTESTING-PATTERNS.md(T1–T4) ו-bugbot-rules/widened-exception-scope.mdclaude-md-snippets/testing.mdbugbot-rules/line-number-coupling.md🧩 השפעות/סיכונים
upload_id. הטקסט הוא עדmax_code_size()תווים, כי ראוט ההעלאה דוחה טקסט ארוך מזה. זה זניח מול השמירה עצמה.create_upload, שומר hash תקין מאז feat(mcp): העלאת תוכן ארוך בלי לעבור דרך המודל — PUT /api/agent/upload ו-upload_id #3502, וכל העלאה פוקעת אחריUPLOAD_TTL_SECONDS._upload_id_param_doc: אם כלי שלישי יקבל את התיאור, הוא חייב לבחור משפט. שכחה נכשלת בזמן הרישום ולא בשקט.🔗 קישורים
upload_content_sha256נפסל בתכנון: הוא היה הד לערך שה-curl כבר החזיר, והבדיקה בצד השרת סוגרת את הפער האמיתי.docs/mcp-server.rst, הסעיף "העלאת תוכן בלי לעבור דרך המודל".🧯 סיכון / החזרה לאחור (Rollback)
🤖 Generated with Claude Code
https://claude.ai/code/session_01PLrwvW1zc6nhJfG2Mtr4wi