Skip to content

fix(mcp): codekeeper_note_str_replace מודד את התוצאה לפני שהוא בונה אותה — max_size חובה ב-_apply_edit, ו-call-arg חוסם ב-mypy של CI (#3495) - #3517

Merged
amirbiron merged 3 commits into
mainfrom
claude/beautiful-carson-n4ikhm
Oct 4, 2026

Conversation

@amirbiron

@amirbiron amirbiron commented Oct 4, 2026 •

Copy link
Copy Markdown
Owner

תבנית Pull Request

✨ תיאור קצר

  • מה השתנה למשתמש ולשירות: codekeeper_note_str_replace בודק עכשיו את אורך התוצאה לפני שהוא בונה אותה. עד היום הוא בנה את גוף הפתק החדש ורק אז מדד אותו, ולכן replace_all על מחרוזת קצרה עם new_string ארוך בנה בזיכרון מחרוזת באורך "מספר המופעים כפול אורך ההחלפה" לפני הסירוב. על שרת חי, בקשה של כ-10KB הקפיצה את שיא הזיכרון של התהליך בכ-19MiB, ועל פתק מלא בכ-172MiB. בתקרות של הייצור זה כ-40GB — בקשה אחת שמפילה את שירות ה-MCP לכל המשתמשים. התשובה לסוכן על תוצאה ארוכה מדי שאינה רווחים בלבד לא השתנתה: אותו content_too_long עם max, רק לפני ההקצאה. הקצה היחיד שהשתנה: תוצאה של רווחים בלבד מעל התקרה, שנענתה עד היום empty_content.
  • ומה מונע את הפעם הבאה: max_size של _apply_edit הפך לחובה, בלי ברירת מחדל, ו-call-arg נוסף לשער החוסם של mypy ב-CI. כך כלי עתידי שיחליף טקסט דרך _apply_edit וישכח את התקרה ייעצר ב-CI, ולא בייצור.

📦 שינויים עיקריים

  • קוד (Backend)
  • בוט טלגרם — רק שינוי שם של מחלקה מקומית ב-main.py, בלי שינוי התנהגות (פירוט למטה)
  • מסד נתונים/מיגרציות
  • תיעוד (docs/)
  • DevOps/CI/CD

פירוט נקודות:

  • mcp_server/handlers.py — _apply_edit: max_size: int חובה (במקום int | None = None), והבדיקה המוקדמת רצה תמיד. ה-docstring מסביר למה, ומוסיף מקור לחשבון: זה בדיוק החשבון שלפיו str.replace עצמו מקצה את התוצאה (new_size = slen + n * (len2 - len1) ב-replace, Objects/unicodeobject.c, נקרא ב-CPython 3.11.15; הייצור רץ על 3.11 — Dockerfile, .python-version).
  • mcp_server/handlers.py — note_str_replace: מעביר max_size=MAX_NOTE_CONTENT, ו-code_too_large שחוזר משם נענה {"ok": false, "error": "content_too_long", "max": MAX_NOTE_CONTENT} — בדיוק התשובה של היום על תוצאה ארוכה מדי שאינה רווחים בלבד (הקצה של רווחים בלבד — ב"השפעות/סיכונים"). הבדיקה שאחרי ההחלפה נשארה כרשת על מה שנכתב (ProductionBackend.update_note אינו בודק אורך, נבדק), והיא אינה ההגנה: היא לא נדלקת כל עוד החשבון ב-_apply_edit מדויק.
  • .github/workflows/ci.yml: השער של המעבר הרחב של mypy חוסם עכשיו גם call-arg (attr-defined|return-value|call-arg). עד היום call-arg הודפס ביומן ולא עצר דבר.
  • main.py: המחלקה המקומית השנייה בשם _Ctx ב-setup_bot_data נקראת עכשיו _CtxPredictive (כמו _CtxMaint ו-_CtxBkp שלידה). זה היה המופע היחיד של call-arg בריפו — false positive של mypy, כי שתי מחלקות באותו שם באותה פונקציה — ובלי התיקון השער החדש היה אדום מהיום הראשון. שינוי שם בלבד.
  • תיעוד: docs/mcp-server.rst — בשורת codekeeper_note_str_replace בטבלת הכלים (הגודל נבדק לפני הבנייה, כמו ב-codekeeper_edit_file), ובפסקה "גודל, לפני שהתוצאה נבנית". docs/whats-new.rst — שתי שורות: התיקון (כולל שינוי הקצה של empty_content), ושער ה-CI.

אתרי הקריאה של _apply_edit (grep -rn "_apply_edit(")

איפה התקרה
mcp_server/handlers.py — edit_file max_size=max_code_size() (כבר מ-#3494)
mcp_server/handlers.py — multi_edit_file max_size=max_code_size() (כבר מ-#3494)
mcp_server/handlers.py — note_str_replace max_size=MAX_NOTE_CONTENT — חדש
tests/test_mcp_edit_append.py שמונה קריאות עם max_size, ואחת בכוונה בלעדיו — מוודאת TypeError
tests/test_mcp_server_build.py אחת, עם max_size=MAX_NOTE_CONTENT

האם mypy באמת תופס קורא ששכח? נמדד, לא הונח: אחרי שהפכתי את max_size לחובה ולפני שתיקנתי את note_str_replace, הפקודה של CI (mypy . --ignore-missing-imports --show-error-codes) החזירה mcp_server/handlers.py:2095: error: Missing named argument "max_size" for "_apply_edit" [call-arg] — גם ב-mypy 2.4.0 (מה שה-CI מתקין, בלי נעיצה) וגם ב-1.18.2 (הנעוץ ב-requirements/development.txt). אבל השער של CI לא חסם call-arg, כלומר החובה הייתה נבדקת רק בזמן ריצה. לכן השער הורחב. הטסטים מוחרגים מ-mypy (mypy.ini) — שם הקריאות נבדקות בהרצה.

תקרה אחת

MAX_NOTE_CONTENT ב-mcp_server/handlers.py הוא from sticky_notes_target import MAX_NOTE_CHARS as MAX_NOTE_CONTENT — אותו אובייקט, לא עותק — ו-tests/test_note_boards_api.py::test_the_limit_has_a_single_source כבר מצמיד אותו לוובאפ. לא נוסף עותק שלישי: הטסטים והתיעוד נוקבים בשם, לא במספר.

החשבון באישו, מתוקן

באישו כתוב "גוף בקשה של 1MiB — כ-20GB". תקרת הגוף נגזרת מ-MAX_CODE_SIZE (request_bytes_for ב-mcp_server/limits.py): על ברירת המחדל שבקוד (100,000) היא 1MiB, ועל 300,000 — הערך בייצור כפי שמסרת (את הסביבה ב-Render לא קראתי) — היא 2MiB (נמדד: request_bytes_for(300_000) == 2_097_152). כלומר פתק של MAX_NOTE_CHARS תווים כפול new_string של כ-2MiB — כ-40GB. המסקנה זהה. תיקנתי את אותו מספר גם במסמך ההצעה ב-Ck (הצעת-דפוס-תקרה-אחרי-הגברה.md); את גוף האישו לא ערכתי.

סריקה לאותו דפוס (mcp_server/handlers.py, mcp_server/backend.py)

סריקת AST של כל *, .replace, .join, .sub ודומיהם בשני הקבצים:

  • המקום היחיד שבונה תוכן של משתמש כפול מונה הוא _apply_edit (code.replace(old, new)), ועכשיו כל שלושת הקוראים שלו מוגנים.
  • _sanitize_note_text — .replace ו-re.sub רק מקצרים.
  • apply_line_range — "\n".join על פרוסה של הקובץ, לא יותר ממנו.
  • append_file — code + sep + content נבנה לפני ש-_resave_edited מודד אותו, אבל זה סכום ולא מכפלה: חסום בתקרת הגוף. לא הדפוס.
  • backend.py — אין בו join, replace או sub בכלל; ה-+= שם מספריים.
  • סרקתי גם את שאר mcp_server/*.py: אין מופע נוסף (redaction.py מחליף התאמה במסמן קבוע, ובמקרה הגרוע גדל פי שניים בערך — לא בשליטת הבקשה).
  • אין מה לתקן מעבר ל-PR הזה, ולא נפתח אישו.

🧪 בדיקות

  • Unit
  • Integration
  • Manual

טסטים חדשים:

  • tests/test_mcp_multi_edit.py::test_a_note_replace_all_is_checked_before_its_result_is_built — דרך call_tool על build_mcp אמיתי, מעל ProductionBackend אמיתי (הפתק באוסף sticky_notes של הדמה המשותפת), בדגם של test_a_single_replace_all_is_checked_before_its_result_is_built. בודק את התשובה, שהגבול על שיא ההקצאה אינו תלוי במספר המופעים — peak < 20 × request_bytes — ושבמסד שום דבר לא נכתב (הפתק נקרא מהאוסף, ואין צילום ב-sticky_note_versions). פרמטרי: 2,000 מופעים (האישו) ופתק מלא עד התקרה.
  • tests/test_mcp_new_note_tools.py — על תוצאה שאינה רווחים בלבד צורת הסירוב זהה להיום (שוויון מלא, בלי שדות נוספים; עובר גם על הקוד הישן, וזו הכוונה); הגבול בתו (MAX_NOTE_CONTENT נכתב, תו אחד יותר נדחה); ושינוי הקצה של רווחים בלבד.
  • tests/test_mcp_edit_append.py — החשבון מדויק בתו (6 עובר, 5 נדחה), ו-_apply_edit בלי max_size הוא TypeError.

לפני ואחרי — tracemalloc דרך call_tool (בקשה של 10,097 בתים, גבול 201,940):

מקרה הקוד שלפני (d2f9e3b) אחרי
2,000 מופעים × 10,000 תווים 20,019,845 בתים — נופל 23,303 בתים
פתק מלא (MAX_NOTE_CHARS מופעים) 200,015,625 בתים — נופל 18,287–23,367 בתים

וטסט הקצה של רווחים בלבד נופל על הקוד הישן (empty_content), כמצופה.

שרת חי — uvicorn mcp_server.app:app במצב PAT, מעל mongod 8.0.15 אמיתי, ולקוח MCP אמיתי (streamable_http_client של mcp 1.28.1) על HTTP. אותו תסריט על שני עצים; הזיכרון הוא VmHWM של תהליך השרת (/proc/<pid>/status) לפני ואחרי כל קריאה, ומה שבמסד נקרא אחרי כל קריאה ב-pymongo:

מקרה לפני אחרי
2,000 מופעים content_too_long, שיא ה-RSS +19,184KB content_too_long, +92KB
פתק מלא content_too_long, +175,900KB content_too_long, +56KB
רווחים בלבד מעל התקרה empty_content content_too_long
תוצאה בדיוק בתקרה ok, במסד 20,000 תווים, גרסה אחת ok, במסד 20,000 תווים, גרסה אחת

בכל הסירובים הפתק במסד נשאר כמו שהיה ולא נכתבה גרסה. שיא ה-RSS של השרת אחרי כל הקריאות: 285,276KB לפני, 90,748KB אחרי.

מוטציות — כולן ב-git worktree נפרד, וכולן נתפסו:

מוטציה מה נפל
M0: note_str_replace בלי max_size שער ה-mypy החדש (exit 1). ועם השער הקודם אותה מוטציה עברה — זו הראיה ששינוי השער הוא מה שתופס
M1: בלי הבדיקה החזויה ב-_apply_edit שלושת טסטי הזיכרון, החשבון בתו, רווחים בלבד
M2: note_str_replace עם תקרה ענקית טסט הזיכרון של הפתק, רווחים בלבד
M3: >= במקום > הגבול בתו (פתק ויחידה)
M4: MAX_NOTE_CONTENT - 1 הגבול בתו
M5: ברירת מחדל ל-max_size חזרה test_apply_edit_has_no_default_ceiling
M6: בלי התרגום ל-content_too_long צורת הסירוב, טסט הזיכרון, הגבול, רווחים בלבד
M7: התרגום בלי max צורת הסירוב, טסט הזיכרון, הגבול, רווחים בלבד

(M1 ו-M2 אינם מפילים את טסט הצורה ואת טסט הגבול — הרשת שאחרי ההחלפה עונה שם נכון. הם נתפסים בשיא ההקצאה, וזה התפקיד של טסט הזיכרון.)

שאר הבדיקות:

  • כל החבילה (-n auto --dist=loadscope, כמו ב-CI), על הקוד הסופי: 7,203 עברו, 313 דולגו, 0 נכשלו (Python 3.11.15; הדילוגים הם מה שמדלג גם ב-CI — מונגו אינו נגיש ובדיקות הדפדפן). ובקבצים שנגעתי בהם גם ב-Python 3.12.3, השנייה במטריצה: 379 עברו, והשיא בטסט הפתק 17,916 ו-16,652 בתים.
  • השוואת תשובות בין הבסיס ל-PR, בהרצה (נוסף אחרי הריוויו): אותה מטריצה של 14 סוגי קלט ל-note_str_replace רצה על d2f9e3b ועל 6ba68c0, כל צד ב-git worktree נפרד. השתנו רק תוצאות של רווחים בלבד מעל התקרה (רווחים, ירידות שורה, ותו אחד מעל התקרה): עד היום empty_content, עכשיו content_too_long, ובשני המקרים לא נכתב דבר. כל השאר זהה במילון המלא, כולל ambiguous_match על תוצאה שהייתה חורגת, ורווחים בלבד בדיוק בתקרה (empty_content בשניהם).
  • שער ה-mypy של CI משוחזר מקומית על העץ הסופי: עובר (mypy 2.4.0), ואין שגיאת call-arg / attr-defined / return-value גם ב-1.18.2.
  • flake8 בקונפיג של CI על הקבצים שהשתנו — נקי. scripts/generate_ai_map.py --check — עדכני.
  • לא בניתי את Sphinx: רק פרוזה בעמודים קיימים, בלי עמוד חדש ובלי toctree — ה-check של RTD יתפוס אזהרות.

🧪 בדיקות נדרשות ב‑PR

  • 🔍 Code Quality & Security
  • Unit Tests (3.11)
  • Unit Tests (3.12)

📝 סוג שינוי

  • feat: פיצ'ר חדש
  • fix: תיקון באג (התקרה של הפתק נבדקת לפני שהתוצאה נבנית)
  • docs: שינוי תיעוד בלבד
  • refactor: שינוי קוד ללא שינוי התנהגות
  • perf: שיפור ביצועים
  • chore/ci: תשתית/CI (call-arg בשער של mypy)
  • breaking change: שינוי שובר תאימות — רק מקרה קצה אחד משנה קוד שגיאה (למטה), ושני הקודים אינם כותבים דבר

✅ צ'קליסט

  • הקוד עוקב אחרי הסגנון — flake8 בקונפיג של CI נקי; mypy — השער החוסם עובר
  • בדיקות רצות ועוברות
  • תיעוד עודכן (README/Docs)
  • ג'ובים חדשים — לא רלוונטי
  • משתני סביבה — לא נוספו
  • טוקנים של עיצוב — לא רלוונטי
  • אין סודות/מפתחות בקוד
  • אין מחיקות מסוכנות/פעולות על root
  • הודעת הקומיט תואמת Conventional Commits
  • CHANGELOG עודכן (docs/whats-new.rst)
  • כל ה‑Required Checks ירוקים — ממתין ל-CI
  • צילום/וידאו UI — לא רלוונטי
  • עיינתי במסמכי אתר התיעוד — נתיב: docs/mcp-server.rst | המשפט: "אותה בדיקה חלה גם על codekeeper_edit_file: replace_all עם new_string ארוך נדחה לפני שהתוצאה נבנית."

עיון ב-CodeBot – Project Docs: כן. AI-MAP.md; docs/mcp-server.rst (טבלת הכלים, "כמה עריכות בגרסה אחת" ו"גודל, לפני שהתוצאה נבנית", "קריאת פתק בודד", "היסטוריית פתקים", פתרון תקלות); docs/whats-new.rst; docs/doc-authoring.rst; docs/versioning-stable-anchors.rst; docs/style-glossary.rst; docs/testing.rst; docs/dev/sticky_notes_extending.rst ("התיעוד למשתמש הוא חלק מהפיצ'ר"); docs/user/sticky_notes.rst — נבדק ש"מעבר לתקרה השמירה נדחית ומוחזר content_too_long" נשאר נכון (ועכשיו גם בקצה של רווחים בלבד), ולכן העמוד לא שונה.

דפוסי amir-bug-patterns שנדלקו ונקראו: U3 ו-external-input-isinstance (תקרה היא סירוב עם קוד משלו, ואינה תחליף לתקרת הגוף ולהגבלת הקצב); H6 (התקרה בתווים, החשבון בתווים, max בתווים — אין ערבוב יחידות); silent-truncation-at-sink; prose-restates-code-fact (grep על max_size, _apply_edit, note_str_replace, empty_content, content_too_long בכל הפרוזה; שמות ולא ערכים); claude-md-snippets/testing.md ו-TESTING-PATTERNS.md (דרך הממשק של הצרכן, בתצורת הייצור, ונופל על הקוד הישן); widened-exception-scope; line-number-coupling; host-metric-in-container (העלות נמדדת בשיא — tracemalloc ו-VmHWM — ולא אחרי); K15 ו-lazy-init-guard-publish-order ("לפני שמסירים התנהגות מנוונת — מודדים מי נשען עליה": None כ"בלי תקרה" — קורא ייצור אחד ושבע קריאות בטסטים, כולם עודכנו); R6; write-from-cached-read (הקריאה של הפתק היא find_one ישיר, לא קאש); K11. ובנוסף ההצעה ב-Ck: הצעת-דפוס-תקרה-אחרי-הגברה.md — אין לה פתקים.

🧩 השפעות/סיכונים

  • שינוי בקצה (הכרעה מהאישו): תוצאה של רווחים בלבד שארוכה מהתקרה מחזירה עכשיו content_too_long ולא empty_content. שני הסירובים אינם כותבים דבר.
  • ה-CI: PR עתידי עם שגיאת call-arg ייכשל ב-"🔍 Code Quality & Security". זו הכוונה. היום אין אף אחת בריפו (mypy 2.4.0). ה-CI מתקין mypy בלי נעיצה, ולכן גרסה חדשה שתזהה מקרים חדשים יכולה להאדים את השער — אותו סיכון שכבר קיים ל-attr-defined ו-return-value.
  • ביצועים: הבדיקה המוקדמת היא str.count אחד שכבר רץ קודם — אין עבודה נוספת.

🔗 קישורים

🧯 סיכון / החזרה לאחור (Rollback)

  • Revert של ה-PR מחזיר את הבנייה לפני המדידה בפתקים ואת השער הקודם. אין מיגרציה, אין אינדקס ואין שינוי בנתונים.

סטיות מהתוכנית

  • נוסף, ולא היה בתוכנית: הרחבת שער ה-mypy ל-call-arg, ושינוי השם ב-main.py. ביקשת לוודא ש-mypy ב-CI תופס max_size חסר — הוא סימן אותו, אבל השער לא חסם עליו. בלי זה "חובה" הייתה נבדקת רק בזמן ריצה.
  • הבדיקה שאחרי ההחלפה נשארה כרשת על מה שנכתב, עם הערה שאומרת שהיא אינה ההגנה. התוכנית לא אמרה להסיר אותה; backend.update_note אינו בודק אורך, וכל שאר כותבי הפתקים בודקים בגבול הכתיבה.
  • טסטים מעבר לתוכנית: הגבול בתו, הקצה של רווחים בלבד, החשבון ביחידה, וברירת המחדל שאינה קיימת. וטסט הזיכרון רץ גם על פתק מלא, כדי להראות שהגבול אינו תלוי במספר המופעים.

🤖 Generated with Claude Code

https://claude.ai/code/session_01DhRqpoqm8WFN2KP7cJSKXJ


Generated by Claude Code

&lt;source media="(prefers-color-scheme: dark)" srcset="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"&gt;&lt;source media="(prefers-color-scheme: light)" srcset="https://www.cubic.dev/buttons/review-in-cubic-light.svg"&gt;&lt;img alt="Review in cubic" src="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"&gt;

Summary by Sourcery

Reject oversized note replacements before constructing their results and enforce explicit edit size limits through mypy and CI.

Bug Fixes:

  • Prevent oversized note replacements from building the full replacement string before rejecting it, avoiding excessive MCP server memory usage while preserving the existing error response.
  • Define the note replacement boundary consistently, including the exact-limit and whitespace-only edge cases.

Enhancements:

  • Make the edit size limit mandatory for all _apply_edit callers so every edit path is protected by an explicit ceiling.

CI:

  • Make mypy call-arg errors fail the CI type-check gate and resolve the existing duplicate context-class naming conflict.

Documentation:

  • Document that note replacement size is checked before constructing the result and record the updated behavior in the changelog.

Tests:

  • Add coverage for exact size limits, missing required ceilings, unchanged rejection responses, whitespace-only content, and memory usage across large replacement workloads.

…ותה — max_size חובה ב-_apply_edit, ו-call-arg חוסם ב-mypy של CI (#3495)

עד היום הכלי בנה את גוף הפתק החדש ורק אז השווה אותו ל-MAX_NOTE_CHARS, ולכן
replace_all על מחרוזת קצרה עם new_string ארוך בנה בזיכרון מחרוזת באורך מספר
המופעים כפול אורך ההחלפה לפני הסירוב: בקשה של כ-10KB — שיא של 20,019,845 בתים
(tracemalloc דרך call_tool), ופתק מלא — 200,015,625. בתקרות הייצור (גוף של 2MiB)
כ-40GB לפני הסירוב.

- _apply_edit: max_size חובה, בלי ברירת מחדל; הבדיקה המוקדמת רצה תמיד.
- note_str_replace: max_size=MAX_NOTE_CONTENT, ו-code_too_large נענה
  content_too_long עם max — אותה תשובה, רק לפני ההקצאה. הבדיקה שאחרי ההחלפה
  נשארת כרשת על מה שנכתב. קצה: רווחים בלבד מעל התקרה הם content_too_long.
- ci.yml: השער של mypy חוסם גם call-arg, כדי שקורא ששכח את max_size ייתפס
  סטטית. main.py: _Ctx השני ב-setup_bot_data הוא _CtxPredictive — המופע
  היחיד של call-arg בריפו (שתי מחלקות באותו שם באותה פונקציה).
- טסטים: שיא ההקצאה מתחת לגבול שאינו תלוי במספר המופעים, צורת הסירוב זהה
  להיום, הגבול בתו, הקצה של רווחים בלבד, והחשבון ביחידה.
- תיעוד: docs/mcp-server.rst (שורת הכלי והפסקה על הגודל), docs/whats-new.rst.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DhRqpoqm8WFN2KP7cJSKXJ
@qodo-code-review

Copy link
Copy Markdown

ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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 8 hours by commenting @sourcery-ai review. Upgrade to get a review now.

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@greptile-apps greptile-apps Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Your trial has ended. Reactivate Greptile to resume code reviews.

@sourcery-ai

sourcery-ai Bot commented Oct 4, 2026

Copy link
Copy Markdown
Contributor

Reviewer's Guide

The PR closes a memory-amplification vulnerability in note-wide string replacement by enforcing an exact pre-allocation size check, makes the ceiling mandatory for all _apply_edit callers, and updates CI, tests, and documentation to prevent regressions.

Sequence diagram for bounded note string replacement

sequenceDiagram
    participant Client
    participant MCP as note_str_replace
    participant Edit as _apply_edit
    participant Store as update_note

    Client->>MCP: note_str_replace
    MCP->>Edit: _apply_edit(body, old_clean, new_clean, replace_all, max_size=MAX_NOTE_CONTENT)
    Edit->>Edit: str.count(old_string)
    alt computed result exceeds MAX_NOTE_CONTENT
        Edit-->>MCP: code_too_large
        MCP-->>Client: content_too_long with max
    else result fits ceiling
        Edit->>Edit: str.replace(old_string, new_string)
        Edit-->>MCP: new_body
        MCP->>Store: update_note
        Store-->>MCP: saved note
        MCP-->>Client: ok
    end
Loading

Flow diagram for mandatory edit size ceilings

flowchart LR
    A[_apply_edit requires max_size] --> B{All callers provide a ceiling}
    B -->|yes| C[Pre-allocation size check]
    B -->|no| D[mypy call-arg error]
    C --> E[CI gate blocks error]
    C --> F[Oversized result rejected before str.replace]
Loading

File-Level Changes

Change Details Files
Prevent oversized note replacements from allocating the full result before rejection.
  • Make _apply_edit require max_size and compute the exact result length before calling str.replace.
  • Pass MAX_NOTE_CONTENT from note_str_replace and translate code_too_large to the existing content_too_long response.
  • Retain the post-replacement size check as a defensive write-side guard and preserve the exact-size boundary behavior.
  • Add unit and integration coverage for allocation limits, exact boundaries, unchanged refusal responses, whitespace-only results, and no database writes.
mcp_server/handlers.py
tests/test_mcp_edit_append.py
tests/test_mcp_multi_edit.py
tests/test_mcp_new_note_tools.py
tests/test_mcp_server_build.py
Make missing required edit ceilings fail CI instead of remaining a runtime-only risk.
  • Extend the CI mypy error gate to block call-arg diagnostics.
  • Rename the colliding local context class so the new mypy gate passes without behavioral change.
  • Update all direct _apply_edit test calls to provide an explicit ceiling and verify omission raises TypeError.
.github/workflows/ci.yml
main.py
tests/test_mcp_edit_append.py
tests/test_mcp_server_build.py
Document the pre-allocation size check and its behavioral edge cases.
  • Explain the exact result-size calculation and required ceiling in the MCP server documentation.
  • Record the fix, CI gate change, and whitespace-only refusal ordering in the release notes.
docs/mcp-server.rst
docs/whats-new.rst

Assessment against linked issues

Issue Objective Addressed Explanation
#3495 למנוע בנייה של תוצאת החלפה ענקית בזיכרון לפני בדיקת תקרת התוכן ב-codekeeper_note_str_replace, תוך החזרת content_too_long עם max וללא כתיבה למסד. ✅
#3495 להפוך את max_size של _apply_edit לחובה, כך שכל קורא יעביר תקרה מפורשת ושכחת התקרה תזוהה בבדיקות הסוגים וב-CI. ✅
#3495 להוסיף בדיקות ותיעוד המכסים את בדיקת הגודל המוקדמת, גבולות התקרה, אי-כתיבה במקרה של סירוב ושינוי סדר השגיאות עבור תוכן ריק וארוך. ✅

Possibly linked issues


Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@github-actions

github-actions Bot commented Oct 4, 2026

Copy link
Copy Markdown
Contributor

🧯 Dangerous deletes guard report

Policy: see .cursorrules — dangerous deletions are blocked unless wrapped safely.

Summary:

  • Flagged findings (blocking): 0
    0
  • Excluded matches (not blocking): 15
  • Total matches (all files): 128

Flagged findings (file:line:snippet):
(none)

Excluded matches (by path pattern)
./node_modules/katex/src/fonts/Makefile:139:	rm -rf pfa ff otf ttf woff woff2
./node_modules/katex/package.json:153:    "build": "rimraf dist/ && mkdirp dist && cp README.md dist && rollup -c --failAfterWarnings && webpack && node update-sri.js package dist/README.md",
./node_modules/mermaid/dist/mermaid.js.map:4:  "sourcesContent": ["/**\n* Default values for dimensions\n*/\nconst defaultIconDimensions = Object.freeze({\n\tleft: 0,\n\ttop: 0,\n\twidth: 16,\n\theight: 16\n});\n/**\n* Default values for tr … [truncated]
./node_modules/mermaid/dist/chunks/mermaid.esm/chunk-2M32CCKP.mjs.map:4:  "sourcesContent": ["{\n  \"name\": \"mermaid\",\n  \"version\": \"11.12.0\",\n  \"description\": \"Markdown-ish syntax for generating flowcharts, mindmaps, sequence d … [truncated]
./node_modules/mermaid/dist/chunks/mermaid.core/chunk-KS23V3DP.mjs.map:4:  "sourcesContent": ["{\n  \"name\": \"mermaid\",\n  \"version\": \"11.12.0\",\n  \"description\": \"Markdown-ish syntax for generating flowcharts, mindmaps, sequence  … [truncated]
./node_modules/mermaid/dist/chunks/mermaid.esm.min/chunk-4HFYJGYH.mjs.map:4:  "sourcesContent": ["{\n  \"name\": \"mermaid\",\n  \"version\": \"11.12.0\",\n  \"description\": \"Markdown-ish syntax for generating flowcharts, mindmaps, sequen … [truncated]
./node_modules/mermaid/dist/chunks/mermaid.esm.min/chunk-4HFYJGYH.mjs:1:var r={name:"mermaid",version:"11.12.0",description:"Markdown-ish syntax for generating flowcharts, mindmaps, sequence diagrams, class diagrams, gantt charts, git graph … [truncated]
./node_modules/mermaid/dist/mermaid.min.js:1524:`,"getStyles"),c1e=RQe});var h1e={};dr(h1e,{diagram:()=>NQe});var NQe,f1e=N(()=>{"use strict";$ge();a1e();l1e();u1e();NQe={parser:Fge,db:n1e,renderer:o1e,styles:c1e}});var m1e,g1e=N(()=>{"use  … [truncated]
./node_modules/mermaid/dist/mermaid.min.js.map:4:  "sourcesContent": ["/**\n* Default values for dimensions\n*/\nconst defaultIconDimensions = Object.freeze({\n\tleft: 0,\n\ttop: 0,\n\twidth: 16,\n\theight: 16\n});\n/**\n* Default values fo … [truncated]
./README.md:829:find . -name "__pycache__" -exec rm -rf {} +
./webapp/static/js/md_preview.bundle.js.map:4:  "sourcesContent": ["// Markdown-it plugin to render GitHub-style task lists; see\n//\n// https://github.com/blog/1375-task-lists-in-gfm-issues-pulls-comments\n// https://github.com/blog/1825-t … [truncated]
./Dockerfile:42:    rm -rf /var/lib/apt/lists/*
./Dockerfile:121:    rm -rf /var/lib/apt/lists/*
./docs/DOCUMENTATION_GUIDE.md:453:rm -rf _build
./docs/Makefile:24:	rm -rf $(BUILDDIR)

@coderabbitai

coderabbitai Bot commented Oct 4, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

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 configuration
  • Configuration used: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: c1994209-6089-444f-9d65-9449f39c6b5e
📥 Commits

Reviewing files that changed from the base of the PR and between d2f9e3b and 59f3a25.

📒 Files selected for processing (9)
  • .github/workflows/ci.yml
  • docs/mcp-server.rst
  • docs/whats-new.rst
  • main.py
  • mcp_server/handlers.py
  • tests/test_mcp_edit_append.py
  • tests/test_mcp_multi_edit.py
  • tests/test_mcp_new_note_tools.py
  • tests/test_mcp_server_build.py

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

העדכון מחשב את גודל תוצאת החלפת תוכן בפתק לפני יצירתה, ומחזיר content_too_long כשהתוצאה חורגת מהתקרה. נוספו בדיקות לספי הגודל ולסירובים. שער MyPy ב-CI בודק כעת גם שגיאות call-arg.

Changes

החלפת תוכן בפתק

Layer / File(s) Summary
בדיקת גודל מוקדמת והעברת התקרה
mcp_server/handlers.py, docs/mcp-server.rst, docs/whats-new.rst
‏_apply_edit דורשת כעת max_size ובודקת את גודל התוצאה לפני בנייתה. note_str_replace מעבירה את תקרת תוכן הפתק וממירה code_too_large ל-content_too_long עם max.
בדיקות תקרה, סירוב ואי-כתיבה
tests/test_mcp_edit_append.py, tests/test_mcp_multi_edit.py, tests/test_mcp_new_note_tools.py, tests/test_mcp_server_build.py
הבדיקות מכסות תוצאה בגודל התקרה, תוצאה שחורגת ממנה, תוצאת רווחים בלבד, והיעדר כתיבה או יצירת גרסה במסלולי סירוב. בדיקות _apply_edit מעבירות תקרה מפורשת.
בדיקות CI והקשר מקומי
.github/workflows/ci.yml, main.py, docs/whats-new.rst
שער MyPy נכשל כעת גם עבור שגיאות call-arg. שם מחלקת הקונטקסט המקומית ב-fallback של _predictive_sampler_job שונה ל-_CtxPredictive. יומן השינויים מתעד גם את העדכונים האלה.

Priority: ⬆️ High

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix · Severity of issue fixed: High

Merge Risk: ⚪ Minimal · up to 59f3a

The size-limit change has no confirmed merge-blocking issue; the production edit callers pass the required limit.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 59f3a

The change strengthens protection against oversized replacements without an observed expansion of write access. No new material security concern was established. Deployed size limits, memory capacity, and service-wide exposure remain unverified.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — A write-authorized caller editing an owned note previously could cause replacement-output allocation far larger than the request. Exhaustion would affect the hosting MCP process, potentially disrupting other work sharing it. The head removes this over-ceiling output construction path; the number of users, tenants, or services sharing a process is not established.

Security Findings and Attack Paths

  • observed — The changed control interrupts the request-to-allocation path before constructing an oversized replacement and before reaching persistence. This strengthens an existing refusal mechanism; the pre-allocation weakness predates this PR rather than being introduced by it.

Trust Boundaries and Controls

  • observed — The traced MCP entrypoint requires write permission and derives user identity from request context. Production note reads and conditional writes filter by note identity and user ownership. The new ceiling operates within these controls rather than replacing authorization or accepting an independently supplied owner identity.

Resilience and Maintainability Implications

  • observed — The existing backend snapshots changed content before overwriting it, refuses mutation when snapshot creation fails, and conservatively retains recovery history when a write outcome is uncertain. Snapshot and note writes are separate operations, so interruption can leave an orphan snapshot; this compensating-recovery limitation was not shown to worsen.
  • observed — The content-based conditional write rejects ordinary stale edits but is not ABA-resistant because its normal predicate lacks a revision token. Replacement is documented as non-idempotent, and conflict requires re-reading before retry. These existing transition characteristics are not active PR concerns because no introduced or worsened exposure was established.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 65.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 20 functions across 6 files. (3 skipped: … Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed #3495: _apply_edit דורש כעת max_size ובודק את אורך התוצאה לפני str.replace. note_str_replace מעביר MAX_NOTE_CONTENT ומתרגם code_too_large ל־content_too_long עם max. בדיקות חדשות מכסות …
Out of Scope Changes check ✅ Passed לא נמצאו שינויים שאינם קשורים למטרה. הרחבת שער mypy ל־call-arg אוכפת את max_size החובה; שינוי שם _Ctx ל־_CtxPredictive מונע כשל קיים בשער החדש. התיעוד, יומן השינויים והבדיקות תומכים בתיקון של …
Title check ✅ Passed הכותרת מתארת במדויק את בדיקת הגודל המוקדמת ב־codekeeper_note_str_replace ואת אכיפת שגיאות call-arg ב־CI. היא ארוכה, אך ברורה וספציפית.
Description check ✅ Passed התיאור מפורט ותואם בעיקרו לתבנית: הוא מסביר מה השתנה ולמה, מפרט את הקוד והסיכונים, ומתעד בדיקות, CI, תיעוד ותוכנית rollback. הוא מציין ש־CI עדיין ממתין, ושהבדיקה של Sphinx לא הורצה.
Full details: Docstring Coverage

Explanation

Docstring coverage is 65.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 20 functions across 6 files. (3 skipped: 3 unsupported.)

  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

לפני שהמחרוזת נבנית, נמדד לה האורך
אם עברה את התקרה, נעצרת בלי לכתוב
תוצאה בגבול מתקבלת, וגבול נשמר
גם MyPy מחפש כעת עוד סוג שגיאה
Claude Code מקבל כוכב קטן בשמי הקוד
CodeKeeper forever 💫

Comment @coderabbitai help to get the list of available commands.

@github-actions

github-actions Bot commented Oct 4, 2026 •

Copy link
Copy Markdown
Contributor

⏱️ Performance report

(No performance test durations collected. Mark tests with @pytest.mark.performance.)

@kilo-code-bot

kilo-code-bot Bot commented Oct 4, 2026 •

Copy link
Copy Markdown

Code Review Summary

Status: No Issues Found | Recommendation: Merge

Files Reviewed (3 files)
  • docs/whats-new.rst - Clarified release notes: pre-check catches content_too_long for non-whitespace-only results
  • mcp_server/handlers.py - Updated docstring with same clarification
  • tests/test_mcp_new_note_tools.py - Renamed test to clarify it tests non-whitespace-only results
Previous Review Summaries (2 snapshots, latest commit 6ba68c0)

Current summary above is authoritative. Previous snapshots are kept for context only.

Previous review (commit 6ba68c0)

Status: No Issues Found | Recommendation: Merge

Files Reviewed (4 files)
  • docs/mcp-server.rst - Fixed documentation to accurately describe the whitespace-only edge case behavior change
  • docs/whats-new.rst - Fixed release notes to accurately describe the behavior change
  • mcp_server/handlers.py - Fixed docstring to accurately describe the behavior change
  • tests/test_mcp_new_note_tools.py - Fixed test comment to accurately describe the behavior

Previous review (commit 59f3a25)

Status: No Issues Found | Recommendation: Merge

Files Reviewed (9 files)
  • .github/workflows/ci.yml - Extended mypy gate to block call-arg errors
  • docs/mcp-server.rst - Documented pre-allocation size check for note_str_replace
  • docs/whats-new.rst - Release notes for the fix and CI gate change
  • main.py - Renamed _Ctx to _CtxPredictive to avoid mypy false positive
  • mcp_server/handlers.py - Core fix: max_size required in _apply_edit, pre-replacement size check, note_str_replace passes ceiling
  • tests/test_mcp_edit_append.py - Unit tests for _apply_edit semantics, ceiling boundary, missing ceiling TypeError
  • tests/test_mcp_multi_edit.py - Integration test for note replacement memory allocation bounds
  • tests/test_mcp_new_note_tools.py - Tests for refusal format, exact ceiling boundary, whitespace-only edge case
  • tests/test_mcp_server_build.py - Updated test to pass required max_size

Reviewed by nemotron-3-ultra-550b-a55b:free · Input: 155.9K · Output: 1.6K · Cached: 177.1K

@github-actions

github-actions Bot commented Oct 4, 2026 •

Copy link
Copy Markdown
Contributor

📖 Documentation Preview

The documentation has been built successfully!

To view locally:

  1. Download the artifacts
  2. Extract the zip file
  3. Open index.html in your browser

@codecov

codecov Bot commented Oct 4, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

All reported issues were addressed across 9 files

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

Comment thread docs/mcp-server.rst Outdated
…ים בלבד כן השתנה (#3495)

ריוויו (cubic, P3) על docs/mcp-server.rst: "אותה תשובה שהכלי החזיר תמיד" אינו
נכון. בקוד שלפני #3495 בדיקת הריקות רצה לפני בדיקת האורך, ולכן תוצאה של רווחים
בלבד שארוכה מהתקרה נענתה empty_content. אותה הכללה הופיעה בעוד מקומות שה-PR כתב,
ותוקנה בכולם:

- docs/mcp-server.rst: מתאר את ההתנהגות של היום במלואה — תוצאה ארוכה מהתקרה,
  גם של רווחים בלבד, נענית content_too_long — בלי טענה על העבר (ההיסטוריה ב-whats-new).
- handlers.py (docstring של note_str_replace): "התשובה שנתן גם לפני #3495 על
  תוצאה ארוכה מדי", והקצה היחיד שהשתנה נקוב בשמו.
- docs/whats-new.rst: "התשובה על תוצאה ארוכה מדי היא אותה תשובה".
- tests/test_mcp_new_note_tools.py: הטענה מוגבלת למקרה שהטסט בודק, ומפנה לטסט של הקצה.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DhRqpoqm8WFN2KP7cJSKXJ

@greptile-apps greptile-apps Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Your trial has ended. Reactivate Greptile to resume code reviews.

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sourcery assessment

Approved.

…שם הטסט (#3495)

המשך לריוויו של cubic (P3). בתשובה בשרשור דחיתי את הנוסח "אותה תשובה שהכלי
החזיר על תוצאה שחרגה מהתקרה", כי תוצאה של רווחים בלבד שחרגה נענתה עד #3495
empty_content. אבל באותו היקף נשארה הטענה ב-docstring של note_str_replace
וב-whats-new, והחריג הופיע שם רק במשפט שאחריה. ושם הטסט
test_a_result_over_the_ceiling_is_the_same_refusal_as_before טען אותו דבר על
כל תוצאה מעל התקרה, בלי חריג בכלל (והסיכום של Sourcery העתיק ממנו
"unchanged rejection responses").

- handlers.py ו-whats-new.rst: "על תוצאה ארוכה מדי שאינה רווחים בלבד".
- הטסט נקרא עכשיו
  test_a_result_over_the_ceiling_that_is_not_only_whitespace_is_the_same_refusal_as_before.

נבדק בהרצה ולא רק בקריאה: אותה מטריצה של 14 סוגי קלט רצה על הבסיס (d2f9e3b)
ועל 6ba68c0, כל אחד ב-worktree נפרד. השתנו רק תוצאות של רווחים בלבד מעל התקרה
(רווחים, ירידות שורה, ותו אחד מעל התקרה): עד #3495 empty_content, ועכשיו
content_too_long, ובשני המקרים לא נכתב דבר. כל השאר זהה במילון המלא, כולל
ambiguous_match על תוצאה שהייתה חורגת, ורווחים בלבד בדיוק בתקרה.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DhRqpoqm8WFN2KP7cJSKXJ

@greptile-apps greptile-apps Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Your trial has ended. Reactivate Greptile to resume code reviews.

@gitar-bot

gitar-bot Bot commented Oct 4, 2026

Copy link
Copy Markdown
Code Review ✅ Approved

🟡 Medium risk · Note replacements reject oversized output before allocation; whitespace-only errors also change.

Fixes note replacement memory exhaustion by checking content size before building the result, preventing massive intermediate allocations on production. Makes max_size mandatory in _apply_edit and extends mypy's CI gate to call-arg errors, blocking future unbounded edits at build time rather than runtime. No issues found.

Review coverage

📋 Rules No rules evaluated

🧪 Functional validation Not enabled · Set up

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Counting what did not apply, without listing it.

Comment with these commands to change the behavior for this request:

Auto-apply Compact
gitar auto-apply:on         
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | Gitar

@amirbiron
amirbiron merged commit accf14a into main Oct 4, 2026
32 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

codekeeper_note_str_replace בונה את התוצאה לפני שהוא מודד אותה — בקשה קטנה מכלה זיכרון (OOM)

2 participants