From f8cdf9cf53c9fd0c238a1ab770b223f2e099668f Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 20 Sep 2026 23:38:17 +0000 Subject: [PATCH] =?UTF-8?q?fix(mirror):=20get=5Ffile=5Fat=5Fcommit=20?= =?UTF-8?q?=D7=91=D7=95=D7=93=D7=A7=20=D7=90=D7=AA=20=D7=92=D7=95=D7=93?= =?UTF-8?q?=D7=9C=20=D7=94=D7=90=D7=95=D7=91=D7=99=D7=99=D7=A7=D7=98=20?= =?UTF-8?q?=D7=9C=D7=A4=D7=A0=D7=99=20git=20show,=20=D7=95=D7=A7=D7=95?= =?UTF-8?q?=D7=91=D7=A5=20=D7=9E=D7=A2=D7=9C=20=D7=94=D7=AA=D7=A7=D7=A8?= =?UTF-8?q?=D7=94=20=D7=90=D7=99=D7=A0=D7=95=20=D7=A0=D7=98=D7=A2=D7=9F=20?= =?UTF-8?q?=D7=9B=D7=9C=D7=9C=20(#3433,=20=D7=A4=D7=A8=D7=99=D7=98=209)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit עד היום git show נטען כולו לזיכרון (capture_output=True) ורק אז max_size נבדק, כלומר התקרה הייתה בדיקה בדיעבד ולא חסם על הזיכרון: קובץ של 12MB שנדחה עלה 20MiB שיא בחוט הקריאה לפני שהתשובה הייתה file_too_large. זה השורש של WARN-003 בסקירת #3429, ומה שהניח את "קריאה אחת עולה לכל היותר X" כהערה ולא כתכונה של הקוד. - _object_size: git cat-file -s : מחזיר את גודל האובייקט מהמאגר בלי לקרוא אותו. שתי הפקודות פונות לאותו sha שנפתר פעם אחת ב-_validate_ref_with_git, ולכן אין חלון בין הבדיקה לקריאה שסנכרון של המראה יכול להיכנס בו (TOCTOU נבחן ונדחה: אובייקט בקומיט נתון אינו משתנה). - כשל בבדיקת הגודל הוא סירוב באותה מפה של git show, לא נפילה לקריאה בלי תקרה; פלט שאינו מספר הוא כשל ולא אפס (U3). מיפוי ה-stderr אוחד ל-_object_read_error, כי git 2.43 מדפיס את אותן הודעות לשתי הפקודות על נתיב חסר ועל קומיט שאינו מכיל אותו (נמדד). - החסם על מה שמוחזר נשאר: לנתיב של תיקייה cat-file -s מודד את אובייקט העץ ואילו git show מדפיס רשימה, ומה שחוזר לעולם אינו גדול מ-max_size. - אותה תשובה ואותו size בסירוב (גודל הבלוב), ואותה תשובה מתחת לתקרה. נמדד (VmHWM, תהליך נקי, מראה bare עם קבצי טקסט): 12MB מול תקרה של 500KB ושל 10MiB — 20.2 ו-20.4MiB שיא לפני, 0.0 אחרי; 7MB מתחת לתקרה — 14.2 לפני ו-14.5 אחרי, כי אותו כן קוראים. טסטים על מראה git אמיתית ב-tmp_path עם מרגל על subprocess.run: ארבעה נופלים על origin/main (הסירוב לפני git show, אותו sha לשתי הפקודות והסדר ביניהן, כשל בבדיקה שאינו נופל לקריאה, גודל שאינו מספר), ושלושה עוברים שם בכוונה כבקרות (מתחת לתקרה, נתיב חסר, החסם על מה שמוחזר); מוטציה שמוחקת את החסם השני מפילה את הפין שלו. התיעוד וההערות שתיארו את "טוענת את ה-blob כולו לפני בדיקת הגודל" עודכנו. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01SfJTSpDAhDr2yhtmpFkwTx --- docs/mcp-server.rst | 10 +- docs/whats-new.rst | 1 + mcp_server/repo_backend.py | 6 +- mcp_server/server.py | 5 +- services/git_mirror_service.py | 83 +++++++++++++---- tests/test_git_mirror_service.py | 154 +++++++++++++++++++++++++++++++ 6 files changed, 237 insertions(+), 22 deletions(-) diff --git a/docs/mcp-server.rst b/docs/mcp-server.rst index e8948f355..288684444 100644 --- a/docs/mcp-server.rst +++ b/docs/mcp-server.rst @@ -115,8 +115,9 @@ usable=8)`` מול ``cpu quota cgroup v2: 0.50 cpu``, כלומר 12 חוטים המספרים מחדש **לשני הפרסרים** — מריצים אותו כשאחד מהם משתנה. - **ההחזקה הגדולה ביותר של חוט קריאה אינה הפרסור, וזו החלטה מתועדת.** כלי האדמין ``codekeeper_get_repo_file`` עם ``outline=true`` או ``lines=`` קורא עד - ``RANGE_READ_MAX_BYTES`` (‏10MiB), והמראה טוענת את ה-blob כולו לזיכרון - **לפני** בדיקת הגודל. נמדד (2026-09-20): outline של קובץ RST בגודל 10MB + ``RANGE_READ_MAX_BYTES`` (‏10MiB), והמראה טוענת blob **שמתחת לתקרה** כולו + לזיכרון לפני הפענוח והחיתוך (blob שמעליה נדחה מ-``git cat-file -s`` בלי + להיקרא — ראו #3433 למטה). נמדד (2026-09-20): outline של קובץ RST בגודל 10MB מחזיק 61–77MiB לאורך המסלול כולו — הטקסט של 10MB בזיכרון ועליו הפרסור; שורת ``outline_densest_real`` של הסקריפט מראה את הפרסור לבדו מעל השיא שלפניו, 51MiB — וקריאת ``lines=`` של ``webapp/static/js/md_preview.bundle.js`` @@ -128,7 +129,10 @@ usable=8)`` מול ``cpu quota cgroup v2: 0.50 cpu``, כלומר 12 חוטים (הגדול ביותר כאן 169KB). תמחור לפי ``RANGE_READ_MAX_BYTES`` היה מצמצם את המסלול הציבורי ל-4 חוטים (לפי 77MiB) או ל-9 (לפי 37MiB) בגלל צורה שאין לה מקור. מה שכן השתנה בגלל זה הוא התקרה (למטה); והשורש — לחסום את הקריאה - במקור, בבדיקת גודל לפני ``git show`` — נמצא ב-#3433. + במקור — נסגר ב-#3433: ``get_file_at_commit`` שואל את מאגר האובייקטים לגודל + (``git cat-file -s :``) לפני ``git show``, ולכן ``max_size`` הוא + חסם על מה שנקרא ולא בדיקה בדיעבד. נמדד: קובץ של 12MB שנדחה עלה 20MiB שיא + לפני, ואפס אחרי; קובץ של 7MB שמתחת לתקרה נשאר 14MiB, כי אותו כן קוראים. - **רצפה** (``_READ_POOL_FLOOR``, ‏2): כדי שקריאת מונגו תקועה אחת לא תשבית את השאר. שניים הוא הגודל הקטן ביותר עם התכונה הזאת, וכל יחידה מעליו היא זיכרון שהחשבון אמר שאין. **תקרה** (``_READ_POOL_CAP``, ‏12): הרוחב שהייצור diff --git a/docs/whats-new.rst b/docs/whats-new.rst index ef65c8cd0..86a14edff 100644 --- a/docs/whats-new.rst +++ b/docs/whats-new.rst @@ -4,6 +4,7 @@ What's New 2026-09-20 ---------- +- **fix (mirror): ‏``get_file_at_commit`` בודק את גודל הקובץ במאגר האובייקטים לפני ``git show``, ולכן קובץ מעל התקרה אינו נטען לזיכרון כלל (#3433, פריט 9).** עד היום ``git show`` נטען כולו לזיכרון ורק אז ``max_size`` נבדק — כלומר התקרה הייתה בדיקה בדיעבד: קובץ של 12MB שנדחה עלה 20MiB שיא בחוט הקריאה לפני שהתשובה הייתה ``file_too_large``. מעכשיו ``git cat-file -s :`` מחזיר את גודל האובייקט בלי לקרוא אותו, שתי הפקודות פונות לאותו קומיט שנפתר פעם אחת (אין חלון לסנכרון להיכנס בו), וקובץ מעל התקרה נדחה לפני שנקרא בית אחד — אותה תשובה, אותו ``size``, אפס זיכרון. כשל בבדיקת הגודל אינו נופל לקריאה בלי תקרה, ומה שמוחזר עדיין נבדק מול התקרה בנפרד, לכל סוג אובייקט. נמדד: 12MB מעל תקרה של 500KB ושל 10MiB — 20.2 ו-20.4MiB לפני, אפס אחרי; 7MB מתחת לתקרה — 14MiB לפני ואחרי, כי אותו כן קוראים. ראו "מודל הריצה של הכלים" ב-:doc:`mcp-server`. - **feat: ``codekeeper_docs_get_section`` קורא גם קובצי Markdown, ולכל ריפו מדיניות נתיבים משלו.** עד היום הכלי ידע לקרוא ``.rst`` בלבד, ורק מתחת ל-``docs/``. הפארסר של Markdown נכתב ונמדד כבר קודם — ואף אחד לא קרא לו; זה השינוי שמחווט אותו. מעכשיו הסיומת בוחרת את הפארסר, ולכל ריפו מוצהר **איפה התיעוד שלו יושב ובאיזה פורמט**: ``CodeBot`` נשאר ``docs/`` עם ``.rst`` ללא שינוי, ו-``amir-bug-patterns`` מגיש ``.md`` משורש הריפו. **וזו הרחבה מכוונת של גבול הקריאה** — הכלי ציבורי, כלומר כל משתמש מאומת מפעיל אותו בלי אדמין, והריפו שנוסף ציבורי ב-GitHub. מה ש**לא** השתנה: מדיניות סינון הסודות חלה על המסלול הזה במלואה, ולכן ``path="secrets"`` מחזיר ``path_denied`` ולא ``not_found``, וגם ``secrets.md`` ו-``credentials.md`` חסומים; ותקרת 500KB של שירות המראה נשארת ההגנה על הפרסור, כי הכלי קורא את הקובץ כולו בלי ``lines``. **שני שערים ולא אחד:** ``MCP_DOCS_REPO`` מתיר בזמן ריצה, טבלת המדיניות בקוד יודעת לקרוא, וריפו שנמצא במשתנה הסביבה ואין לו מדיניות נדחה ב-``repo_not_configured`` במקום ליפול לברירת מחדל מתירנית. **ובקשה לפורמט שהריפו אינו מגיש נדחית בשמה** — ``suffix_not_allowed`` עם רשימת הסיומות שכן מוגשות, במקום להשלים סיומת בשקט מעל הקיימת ולחזור כ"לא נמצא" על קובץ שקיים. שני סירובים חדשים מגיעים מהפרסור עצמו: ``inconsistent_line_endings`` על ``\r`` בודד (CRLF עובד במלואו), ו-``too_many_sections`` על קובץ שחוצה את תקרת 50,000 הכותרות — Markdown דרך ברירת המחדל של הפרסר, ו-RST דרך התקרה שהכלי מעביר מאז #3429; סירוב שהפארסר מספק לו מספר שורה נושא גם ``line`` — תמיד ב-Markdown, ולא ב-``too_many_sections`` של RST, כי ``rst_parser`` מרים אותה בלי מספר. ``includes`` הוא שדה של RST ולכן בעמוד ``.md`` הוא תמיד ריק. **ומסלול ה-RST לא זז**: אפס דיף מדוד על 5,920 רשומות מ-208 קובצי RST, לפני ואחרי. ראו :ref:`mcp-docs-path-policy`. - **fix: מאגר הקריאות של ה-MCP נגזר ממכסת הזיכרון של הקונטיינר, ולא ממספר המעבדים של המארח (#3391).** כלי הקריאה רצים ב-``asyncio.to_thread``, כלומר ב-executor ברירת המחדל של הלולאה, שגודלו ``min(32, os.cpu_count() + 4)`` — ו-``os.cpu_count()`` בקונטיינר הוא מספר הליבות של **המארח**, כפי ש-CPython מתעד במפורש. שורת הקיבולת מהייצור אמרה את זה במספרים: ``read pool 12 threads (os.cpu_count=8, usable=8)`` על מכסה של ``0.50 cpu``. לקריאת מונגו זה לא מזיק; לפרסור בפייתון טהור אין מקביליות ב-CPU בגלל ה-GIL, ולכן 12 חוטים היו 12 עצי פרסור בזיכרון בו-זמנית — יותר ממה שנכנס ב-512MiB — ורגע לפני שהפרסור הופך לכלי ציבורי. מעכשיו ``attach_read_pool`` מתקין בעליית ה-lifespan של אפליקציית ה-ASGI מאגר משלו, בגודל ``(מגבלת הזיכרון − קו בסיס − שוליים) ÷ עלות פרסור אחד``: המגבלה נקראת מ-cgroup בזמן ריצה (``memory.max`` ב-v2, ‏``memory.limit_in_bytes`` ב-v1) ולא מקובעת בקוד, קו הבסיס והשוליים הם מדידות מהשירות בייצור עם תאריך, ועלות הפרסור נגזרת מ-``MAX_FILE_SIZE_FOR_DISPLAY`` כי הכלי דוחה ``too_large`` לפני שהוא מפרסר. על תוכנית הייצור זה **10** חוטים במקום 12 — עלות הפרסור נמדדה מחדש על הפרסר הנעוץ (``scripts/measure_md_parse_cost.py``; הקבוע נמדד על ``services.md_parser`` כהכנה לכלי ה-Markdown של PR 5, בעוד הכלי הציבורי היום מפרסר RST ב-``services.rst_parser``, שעולה כ-6 בתים לבית גם על העמוד הצפוף ביותר — והסקריפט מודד את שניהם), והיא תלויה בצפיפות הטוקנים של המסמך ולא בגודלו: המסמך הצפוף ביותר בקורפוס הוא הקבוע, וצורה עוינת של שורות-תבליט בודדות עולה פי ארבעה ממנו — מה ששום רוחב מאגר אינו סופג, ומחכה לתקרת טוקנים בפרסר עצמו. **ובעקבות הסקירה:** ``codekeeper_docs_get_section`` מעביר לפרסר את תקרת הסקשנים של האאוטליין (50,000) ומחזיר ``too_many_sections`` במקום לפרסר את הקובץ כולו — צורה עוינת של RST (כותרת בת תו אחד בכל שורה) עלתה 44.0MiB לפרסור, יותר מ-35.2MiB שהמאגר מקצה לחוט, ועכשיו נעצרת ב-20.1MiB; אף עמוד אמיתי אינו מתקרב לתקרה. רצפה של 2 (קריאה תקועה אחת לא משביתה את השאר), תקרה של 12, הרוחב שהייצור כבר הריץ (מעבר לתוכנית גדולה לא מרחיב את המאגר בלי החלטה; הגרסה הראשונה נקבה ב-32, והסקירה של #3429 הורידה אותה כי המחלק מתאר את הפרסור ולא את קריאות הטווח של האדמין; חסימת הקריאה במקור — #3433), וכשאין מגבלה קריאה — הרצפה, **לא** ``cpu_count + 4``, עם WARNING שאומר שזה קרה. **ושורת הקיבולת מפסיקה לחשב בעצמה:** היא נרשמת מתוך ה-lifespan, קוראת את ``_max_workers`` של המאגר שהותקן בפועל, ומדווחת גם את מגבלת הזיכרון לצד מכסת ה-CPU — כי רשומה שמתארת מצב ומתעדכנת בנפרד ממנו היא בדיוק מה שמשקר בשקט. ה-seam הוא עטיפת ``router.lifespan_context``, אותו seam שניקוז ה-PostHog כבר משתמש בו, כי ``FastMCP.streamable_http_app()`` בונה את Starlette עם ``lifespan=`` משלה ו-``on_startup`` אינו רץ. הטסט המרכזי נכנס ל-lifespan של האפליקציה האמיתית ומוכיח שקריאת ``to_thread`` נוחתת על חוט של המאגר החדש — לא שפונקציה נקראה; שורת הקיבולת נבדקת בתת-תהליך נקי דרך הפלט האמיתי ולא דרך ``caplog``. ראו "מודל הריצה של הכלים" ב-:doc:`mcp-server`. - **feat: ``codekeeper_docs_get_section`` מוצא סעיף גם לפי המזהה שלו, ולא רק לפי השם המלא.** הסוכנים מפנים זה לזה לפי מזהה — "קרא K11", "ראו U3" — ועד היום ``section="K11"`` החזיר ``section_not_found`` עם רשימת הצעות **ריקה**, כי ההתאמה הייתה שוויון מלא ו-``difflib`` עם ``cutoff=0.5`` אינו מוצא קרבה בין שלושה תווים לכותרת עברית ארוכה. כלומר הזרימה המרכזית של הכלי לא עבדה. מעכשיו, **ורק אחרי שהשוויון המלא לא מצא כלום**, שאילתה שבנויה כמזהה (עד שלוש אותיות ואחריהן עד שלוש ספרות, עם נקודה אופציונלית) מותאמת לכותרת שנפתחת באותו מזהה. **והמזהה נקרא כיחידה ולא כקידומת של מחרוזת**, ולכן ``K1`` מחזיר את ``K1`` בלבד ולעולם לא את ``K10`` עד ``K15`` — הצורה השבורה של הבדיקה הזאת היא אותה מחלקה בדיוק שבה גבול נתיב נבדק כרצף תווים. שאילתה שאינה בנויה כמזהה אינה עוברת במסלול הזה כלל, ולכן ``section="איך"`` ממשיך להחזיר אפס התאמות גם בעמוד שיש בו חמש-עשרה כותרות שמתחילות במילה הזאת. מזהה שחוזר פעמיים הוא ``ambiguous_section`` עם מועמדים, כמו כל כותרת כפולה, ושאילתת מזהה שלא נמצאה מקבלת ב-``suggestions`` קודם כול כותרת קרובה, אם יש כזו בעמוד, **ורק כשאין אף כותרת קרובה** את המזהים שכן קיימים — עד 50, ועם ``suggestions_truncated`` כשהיו עוד. **והתקרה גבוהה בכוונה**, כי רשימת מזהים אינה מדורגת: לשאלה "אלה שכן קיימים" אין תשובה "החמש הטובות", וחיתוך שלה מסתיר פריטים בלי קריטריון ובלי דרך לבקש את השאר. **והדגל הזה אומר את אותו דבר בשני המסלולים**: גם רשימת מזהים שנחתכה וגם רשימת כותרות קרובות שנחתכה מדליקות אותו. שדה בשם ``truncated`` שמשמעותו "נחתך" במסלול אחד ו"שקט" בשני הוא בדיוק ההפתעה שאנחנו מנסים למנוע. **ונקודה עוד יותר קלה לפספס: הנקודה שפותחת תת-מספור אינה גבול.** בכותרת ``K11. טקסט`` הנקודה **מסיימת** את המזהה, ובכותרת ``K11.1 טקסט`` היא **מפרידה** בתוך מזהה ארוך יותר — ולכן ``K11`` לעולם אינו מחזיר את ``K11.1``, וכותרת בתת-מספור נגישה בשמה המלא בלבד. זו אותה מחלקה בדיוק כמו ``K1`` שתופס את ``K10``: גבול שנבדק כתו בודד, בלי לשאול מה בא אחריו. **ההתנהגות על התיעוד כאן לא זזה, וזה נמדד ולא הונח:** הפלט של הכלי על כל קובצי ה-RST — כל כותרת, בכל צורותיה — זהה בית-בית לפני ואחרי. וההנחה שהכול נשען עליה, שאף כותרת בעמודים כאן אינה נפתחת במזהה, **אינה ספירה שנרשמה פעם אחת אלא טסט שרץ בכל CI**: ``tests/test_docs_headings_carry_no_identifier.py`` סורק את כל העמודים ונופל על כותרת שנפתחת במזהה, עם דוגמת autodoc אמיתית שמוכיחה שהוא מסוגל ליפול. **ומה שלא נבדק בקורפוס הזה נבדק בבקרה סינתטית**, כי סוללה שכל תשובותיה "לא נמצא" אינה מבדילה בין ענף כבוי לבין פרובים עיוורים. שני הדברים שמפתיעים קורא — קיצור המזהה, וכותרת שנושאת בקטיקים ולכן דורשת אותם בשאילתה — עברו לתיאור הפרמטר ``section``, שאינו נחתך אצל הלקוח כמו תיאור הכלי. ראו :ref:`mcp-docs-section`. diff --git a/mcp_server/repo_backend.py b/mcp_server/repo_backend.py index 4eb348f4c..c9e82cd66 100644 --- a/mcp_server/repo_backend.py +++ b/mcp_server/repo_backend.py @@ -38,11 +38,13 @@ #: ``too_large`` עם הפרמטר בדיוק כמו בלעדיו (#3317). ``webapp/app.py`` בן #: 805,594 הבייטים, הקובץ שהכי הרבה עובדים עליו, היה בלתי קריא דרך הכלי. #: -#: **למה תקרה אחרת ולא ביטול.** הבלוב עדיין נקרא ומפוענח במלואו לפני +#: **למה תקרה אחרת ולא ביטול.** בלוב שמתחת לתקרה נקרא ומפוענח במלואו לפני #: החיתוך, אז "בלי תקרה" פירושו שקובץ פתולוגי בריפו יגיע ל-RAM כמו שהוא. #: נמדד: קריאה ופענוח צורכים כפי שלושה מגודל הקובץ — 6.6MB הגיעו ל-35.7MB #: שיא. תקרה של 10MB חוסמת את זה בערך ב-30MB, ונותנת פי 12 מרווח מעל -#: הקובץ הגדול ביותר שבאמת קוראים. +#: הקובץ הגדול ביותר שבאמת קוראים. ומאז #3433 התקרה נבדקת מול גודל +#: האובייקט במאגר **לפני** ``git show``, ולכן קובץ שמעליה אינו נטען כלל — +#: החסם הוא על מה שנקרא, לא בדיקה בדיעבד. #: #: זו החלטת מדיניות של שכבת ה-MCP, לא של שירות המראה — ולכן היא כאן ולא #: שם, ואינה מייתרת את 500KB שממשיכה לחול על קריאה מלאה ועל הוובאפ. diff --git a/mcp_server/server.py b/mcp_server/server.py index ee9094438..3c383025f 100644 --- a/mcp_server/server.py +++ b/mcp_server/server.py @@ -750,7 +750,10 @@ def _memory_limit() -> tuple[int | None, str]: #: Pricing by ``RANGE_READ_MAX_BYTES`` would have cut the public path to 4 #: threads (at 77MiB) or 9 (at 37MiB) for a shape without a source. What did #: change because of this is :data:`_READ_POOL_CAP`; the root fix — bounding -#: the read at its source with a size probe before ``git show`` — is #3433. +#: the read at its source with a size probe before ``git show`` (#3433) — has +#: landed in ``get_file_at_commit``: a blob over ``max_size`` is refused from +#: ``git cat-file -s`` without being read (12MB: 20MiB peak before, 0 after, +#: measured), so the hold above is now only what a file *under* the cap costs. _PARSE_COST_BYTES = _PARSE_RSS_PER_INPUT_BYTE * MAX_FILE_SIZE_FOR_DISPLAY diff --git a/services/git_mirror_service.py b/services/git_mirror_service.py index 12654aa1b..99856ee65 100644 --- a/services/git_mirror_service.py +++ b/services/git_mirror_service.py @@ -1573,6 +1573,41 @@ def get_file_history( self.logger.error(f"Error getting file history: {e}") return {"error": "internal_error", "message": "שגיאה פנימית"} + def _object_size( + self, mirror_path: Path, resolved_commit: str, safe_file_path: str, timeout: float = 10 + ) -> Dict[str, Any]: + """גודל האובייקט ``:`` מהמאגר, בלי לקרוא את תוכנו — ``git cat-file -s``. + + מחזיר ``{"size": int}`` או ``{"error": ..., "message": ...}`` באותה מפה + של ``git show`` (:meth:`_object_read_error`), כי git 2.43 מדפיס את + **אותן** הודעות לשתי הפקודות על נתיב חסר (``path 'x' does not exist + in ''``) ועל קומיט שאינו מכיל אותו (``exists on disk, but not + in``) — נמדד, לא הונח. הפלט הוא קלט חיצוני (U3): מספר שאינו מתפענח + הוא כשל ולא אפס, כדי שלא ייקרא כ"קטן מהתקרה". ``TimeoutExpired`` עולה + לקורא, שכבר ממפה אותה ל-``timeout``. + """ + result = subprocess.run( + ["git", "-C", str(mirror_path), "cat-file", "-s", f"{resolved_commit}:{safe_file_path}"], + capture_output=True, + timeout=timeout, + ) + if result.returncode != 0: + return self._object_read_error(result.stderr) + try: + return {"size": int(result.stdout.decode("ascii", errors="replace").strip())} + except ValueError: + self.logger.warning("git cat-file -s returned a non-numeric size for %s", safe_file_path) + return {"error": "git_error", "message": "git לא החזיר גודל אובייקט תקין"} + + def _object_read_error(self, stderr_bytes: bytes) -> Dict[str, Any]: + """ממפה stderr של ``git show`` ושל ``git cat-file`` לתשובת שגיאה — הגדרה אחת לשתיהן.""" + stderr = self._sanitize_output(stderr_bytes.decode('utf-8', errors='replace')) + if "does not exist" in stderr or "exists on disk" in stderr: + return {"error": "file_not_in_commit", "message": "הקובץ לא קיים ב-commit זה"} + if "unknown revision" in stderr or "bad revision" in stderr: + return {"error": "invalid_commit", "message": "Commit לא נמצא"} + return {"error": "git_error", "message": stderr} + def get_file_at_commit( self, repo_name: str, @@ -1587,7 +1622,10 @@ def get_file_at_commit( repo_name: שם הריפו file_path: נתיב הקובץ commit: Hash, tag, branch או expression - max_size: גודל מקסימלי להחזרה (bytes) + max_size: תקרה בבתים. **נבדקת מול גודל האובייקט במאגר לפני + ``git show``** (#3433), ולכן קובץ גדול ממנה אינו נטען לזיכרון + כלל — ``max_size`` הוא חסם על מה שנקרא, לא בדיקה בדיעבד. ונבדקת + שוב על מה שמוחזר, כדי שהחסם יחול על כל סוג אובייקט. Returns: Dict עם תוכן הקובץ או שגיאה @@ -1615,6 +1653,29 @@ def get_file_at_commit( resolved_commit = ref_validation["resolved_sha"] try: + # **הגודל נבדק מול מאגר האובייקטים לפני שנקרא בית אחד של תוכן (#3433).** + # עד כאן ``git show`` נטען כולו לזיכרון (``capture_output=True``) ורק + # אז ``max_size`` נבדק — כלומר התקרה הייתה בדיקה בדיעבד ולא חסם על + # הזיכרון: קובץ של 12MB נטען במלואו (20MiB שיא, נמדד) לפני שנדחה. + # ``git cat-file -s`` מחזיר את גודל האובייקט מהמאגר בלי לקרוא אותו, + # ושתי הפקודות פונות לאותו ``:`` שנפתר פעם אחת למעלה — + # אובייקט בקומיט נתון אינו משתנה, ולכן אין חלון בין הבדיקה לקריאה + # שסנכרון של המראה יכול להיכנס בו (TOCTOU נבחן ונדחה). + probe = self._object_size(mirror_path, resolved_commit, safe_file_path) + if "error" in probe: + # כשל בבדיקה אינו נופל לקריאה בלי תקרה — זו בדיוק הנפילה-לאחור + # השקטה למסלול הגרוע (``silent-fallback-to-worse-path``). הסירוב + # עולה כמות שהוא, באותה מפה שהייתה ל-``git show``. + return probe + file_size = probe["size"] + if file_size > max_size: + return { + "error": "file_too_large", + "message": f"הקובץ גדול מדי ({file_size:,} bytes)", + "size": file_size, + "max_size": max_size + } + # שליפת תוכן כ-bytes (לא text) לזיהוי בינארי נכון cmd = [ "git", "-C", str(mirror_path), @@ -1629,25 +1690,15 @@ def get_file_at_commit( ) if result.returncode != 0: - stderr = result.stderr.decode('utf-8', errors='replace') - stderr = self._sanitize_output(stderr) - - if "does not exist" in stderr or "exists on disk" in stderr: - return { - "error": "file_not_in_commit", - "message": "הקובץ לא קיים ב-commit זה" - } - if "unknown revision" in stderr or "bad revision" in stderr: - return { - "error": "invalid_commit", - "message": "Commit לא נמצא" - } - return {"error": "git_error", "message": stderr} + return self._object_read_error(result.stderr) raw_content = result.stdout file_size = len(raw_content) - # בדיקת גודל + # **החסם על מה שמוחזר, בנוסף לחסם על מה שנקרא.** לבלוב שני + # המספרים זהים; לנתיב של תיקייה ``cat-file -s`` מודד את אובייקט + # העץ ואילו ``git show`` מדפיס רשימה מעוצבת — ומה שחוזר מכאן + # לעולם אינו גדול מ-``max_size``, יהיה סוג האובייקט אשר יהיה. if file_size > max_size: return { "error": "file_too_large", diff --git a/tests/test_git_mirror_service.py b/tests/test_git_mirror_service.py index 5c1e48000..faf8884f2 100644 --- a/tests/test_git_mirror_service.py +++ b/tests/test_git_mirror_service.py @@ -249,3 +249,157 @@ def test_constructor_token_overrides_map(tmp_path, monkeypatch): svc = GitMirrorService(base_path=str(tmp_path), github_token="ghp_EXPLICIT") # טוקן שהוזרק במפורש ל-constructor גובר על הכל assert svc._token_for_url("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/Campaign-AI4U/campaign-ai.git") == "ghp_EXPLICIT" + + +# --------------------------------------------------------------------------- +# get_file_at_commit: התקרה נבדקת מול מאגר האובייקטים לפני git show (#3433, פריט 9) +# --------------------------------------------------------------------------- +# +# עד #3433 ``git show`` נטען כולו לזיכרון ורק אז ``max_size`` נבדק — קובץ של +# 12MB שנדחה עלה 20MiB שיא. הטסטים כאן רצים על מראה git אמיתית שנבנית +# ב-``tmp_path`` (bare clone, כמו בייצור), ומרגלים על ``subprocess.run`` כדי +# לראות **אילו** פקודות git רצו — כי התכונה הנבדקת היא מה לא נקרא, ואת זה +# רואים רק ברשימת הפקודות. + +import subprocess # noqa: E402 + +from services import git_mirror_service as _gms # noqa: E402 + + +def _bare_mirror(tmp_path, files): + """מראה bare בשם ``probe`` תחת ``tmp_path``, עם הקבצים הנתונים בקומיט אחד; מחזיר את השירות ואת ה-sha.""" + src = tmp_path / "src" + src.mkdir() + for name, data in files.items(): + target = src / name + target.parent.mkdir(parents=True, exist_ok=True) + target.write_bytes(data) + env = {"GIT_AUTHOR_NAME": "t", "GIT_AUTHOR_EMAIL": "t@t", "GIT_COMMITTER_NAME": "t", + "GIT_COMMITTER_EMAIL": "t@t", "PATH": __import__("os").environ["PATH"]} + for cmd in (["git", "init", "-q"], ["git", "add", "-A"], ["git", "commit", "-qm", "init"]): + subprocess.run(cmd, cwd=src, check=True, env=env, capture_output=True) + sha = subprocess.run(["git", "rev-parse", "HEAD"], cwd=src, check=True, env=env, + capture_output=True, text=True).stdout.strip() + subprocess.run(["git", "clone", "-q", "--bare", str(src), str(tmp_path / "probe.git")], + check=True, env=env, capture_output=True) + return GitMirrorService(base_path=str(tmp_path)), sha + + +def _spy_on_git(monkeypatch, override=None): + """מרגל שמעביר כל קריאה ל-``subprocess.run`` האמיתי ורושם את ה-argv; ``override(argv)`` יכול להחזיר תוצאה מזויפת.""" + calls = [] + real = subprocess.run + + def spy(cmd, *args, **kwargs): + calls.append(list(cmd)) + if override is not None: + faked = override(list(cmd)) + if faked is not None: + return faked + return real(cmd, *args, **kwargs) + + monkeypatch.setattr(_gms.subprocess, "run", spy) + return calls + + +def _subcommands(calls): + return [cmd[cmd.index("-C") + 2] if "-C" in cmd else cmd[1] for cmd in calls] + + +def test_a_file_over_the_cap_is_refused_before_git_show_reads_it(tmp_path, monkeypatch): + """הסירוב מגיע מ-``git cat-file -s``, ו-``git show`` אינו רץ כלל — אפס בתים נקראים.""" + svc, _ = _bare_mirror(tmp_path, {"big.bin": b"x" * 3000}) + calls = _spy_on_git(monkeypatch) + + out = svc.get_file_at_commit("probe", "big.bin", "HEAD", max_size=1000) + + assert out["error"] == "file_too_large" and out["size"] == 3000 and out["max_size"] == 1000 + assert "cat-file" in _subcommands(calls) + assert "show" not in _subcommands(calls), f"git show רץ על קובץ שמעל התקרה: {calls}" + + +def test_a_file_under_the_cap_is_read_exactly_as_before(tmp_path, monkeypatch): + """הבקרה: מתחת לתקרה התשובה זהה לזו של קודם — תוכן, קידוד, גודל, שורות, sha.""" + svc, sha = _bare_mirror(tmp_path, {"a.txt": "שלום\nעולם\n".encode("utf-8")}) + calls = _spy_on_git(monkeypatch) + + out = svc.get_file_at_commit("probe", "a.txt", "HEAD", max_size=1000) + + assert out["success"] and out["content"] == "שלום\nעולם\n" and out["is_binary"] is False + assert out["encoding"] == "utf-8" and out["size"] == len("שלום\nעולם\n".encode("utf-8")) + assert out["lines"] == 3 and out["resolved_commit"] == sha + assert "show" in _subcommands(calls), "מתחת לתקרה הקובץ כן נקרא" + + +def test_a_missing_path_is_still_file_not_in_commit_through_the_probe(tmp_path): + """git מדפיס את אותה הודעה ל-``cat-file -s`` ול-``show`` על נתיב חסר, והמפה אחת לשתיהן.""" + svc, _ = _bare_mirror(tmp_path, {"a.txt": b"x"}) + out = svc.get_file_at_commit("probe", "nope.txt", "HEAD") + assert out == {"error": "file_not_in_commit", "message": "הקובץ לא קיים ב-commit זה"} + + +def test_both_commands_name_the_same_resolved_commit(tmp_path, monkeypatch): + """בלי חלון בין הבדיקה לקריאה: שתי הפקודות פונות ל-``:`` שנפתר פעם אחת, לא ל-``HEAD``.""" + svc, sha = _bare_mirror(tmp_path, {"a.txt": b"x"}) + calls = _spy_on_git(monkeypatch) + + svc.get_file_at_commit("probe", "a.txt", "HEAD") + + # ``rev-parse`` של אימות ה-ref רץ גם הוא עם ``-C``; כאן מעניינות רק שתי פקודות האובייקט. + targets = {sub: cmd[-1] for cmd in calls if "-C" in cmd + for sub in [cmd[cmd.index("-C") + 2]] if sub in ("cat-file", "show")} + assert targets == {"cat-file": f"{sha}:a.txt", "show": f"{sha}:a.txt"} + assert [s for s in _subcommands(calls) if s in ("cat-file", "show")] == ["cat-file", "show"], ( + "הבדיקה קודמת לקריאה — זה הסדר שהופך את max_size לחסם על מה שנקרא") + + +def test_a_failed_size_probe_never_falls_back_to_an_unbounded_read(tmp_path, monkeypatch): + """כשל בבדיקת הגודל הוא סירוב, לא קריאה בלי תקרה — ``silent-fallback-to-worse-path``.""" + svc, _ = _bare_mirror(tmp_path, {"a.txt": b"x" * 3000}) + + def fail_the_probe(cmd): + if "cat-file" in cmd: + return subprocess.CompletedProcess(cmd, 128, stdout=b"", stderr=b"fatal: object store unreadable") + return None + + calls = _spy_on_git(monkeypatch, override=fail_the_probe) + out = svc.get_file_at_commit("probe", "a.txt", "HEAD", max_size=10) + + assert out["error"] == "git_error" and "unreadable" in out["message"] + assert "show" not in _subcommands(calls) + + +def test_a_non_numeric_size_is_a_failure_and_not_a_small_file(tmp_path, monkeypatch): + """הפלט של git הוא קלט חיצוני (U3): "lots" אינו אפס, והוא אינו עובר את התקרה בשקט.""" + svc, _ = _bare_mirror(tmp_path, {"a.txt": b"x" * 3000}) + + def garble_the_probe(cmd): + if "cat-file" in cmd: + return subprocess.CompletedProcess(cmd, 0, stdout=b"lots\n", stderr=b"") + return None + + calls = _spy_on_git(monkeypatch, override=garble_the_probe) + out = svc.get_file_at_commit("probe", "a.txt", "HEAD", max_size=10) + + assert out["error"] == "git_error" + assert "show" not in _subcommands(calls) + + +def test_what_comes_back_never_exceeds_the_cap_whatever_the_probe_said(tmp_path, monkeypatch): + """החסם השני, על מה שמוחזר: גם אם המאגר דיווח גודל קטן, מה ש-``git show`` הדפיס נבדק שוב. + + פין של תכונה שהייתה קיימת — הבדיקה בדיעבד — ונשארת בכוונה, כי לנתיב + של תיקייה ``cat-file -s`` מודד את אובייקט העץ ואילו ``git show`` מדפיס + רשימה מעוצבת. הדרך היחידה לבודד אותה היא מאגר שמשקר. + """ + svc, _ = _bare_mirror(tmp_path, {"a.txt": b"x" * 3000}) + + def understate_the_probe(cmd): + if "cat-file" in cmd: + return subprocess.CompletedProcess(cmd, 0, stdout=b"10\n", stderr=b"") + return None + + _spy_on_git(monkeypatch, override=understate_the_probe) + out = svc.get_file_at_commit("probe", "a.txt", "HEAD", max_size=1000) + + assert out["error"] == "file_too_large" and out["size"] == 3000