fix(mcp): תקרת הגוף מפסיקה לקרוא גוף אנונימי לפני האימות, ויתרת ממצאי סקירת שבעת ה-PRים (#3434–#3441)

Merged
AM
1mo ago
+3273-568

✨ תיאור קצר

  • סגירת ממצאי סקירת Han (code-review) על שבעת ה-PRים #3434, #3435, #3436, #3437, #3439, #3440, #3441. הדוח המלא שמור ב-CodeKeeper בשם code-review-7prs-3434-3441.md (כולל הנספח של המעבר על amir-bug-patterns). מה נכנס כאן: הרגרסיה SEC-001 בתקרת הגוף (שלושה חלקים, test-first, נמדדו על שני העצים), ארבע האזהרות, ארבע-עשרה הצעות ושני פריטי YAGNI. עשר הצעות שנדחו — באישו #3442.
  • מבנה הענף, וזה חשוב למיזוג: claude/gracious-einstein-sevk8p-review = origin/main + שבעת ענפי ה-PRים ממוזגים בסדר (קונפליקטים היו רק ב-docs/whats-new.rst, נפתרו כ"שניהם") + קומיט התיקונים + מיזוג origin/main העדכני (#3438). אם שבעת ה-PRים ימוזגו קודם — הדיף כאן יתכווץ לתיקונים בלבד; אפשר גם למזג את ה-PR הזה לבדו, והוא נושא את כולם. (הענף הייעודי של הסשן הוא claude/gracious-einstein-sevk8p, שהוא הענף של #3434 עצמו; לכן, כמו בסבב הקודם, נפתח ענף-אח.)

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

  • קוד (Backend)
  • בוט טלגרם
  • מסד נתונים/מיגרציות
  • תיעוד (docs/)
  • DevOps/CI/CD

פירוט נקודות (רשימת תבליטים):

  • SEC-001 — mcp_server/limits.py, שלושה חלקים, כל אחד עם טסט שנפל על הקוד שלפני התיקון (הורץ על worktree של 5c6f30c5, 20 טסטים חדשים או משונים נפלו שם ועוברים כאן):
    1. מסלול מהיר ל-Content-Length תקין (ספרות ASCII בלבד, כמו ש-h11 מקבל; ובלי Transfer-Encoding, כי לצידו h11 תוחם לפי chunked ומעביר את שתי הכותרות — נמדד): הבקשה עוברת בלי שהמידלוור קורא בית, כי השרת תוחם את הגוף לאורך המוצהר. test_a_streamed_body_over_the_cap_is_refused_and_the_app_never_runs שינה טענה (בלי Content-Length), וה-docstring שלו אומר למה.
    2. דדליין על לולאת הניקוז — anyio.fail_after(30.0) על הלולאה כולה (לא לקריאה: בית כל 29 שניות היה עובר), ואז 408 body_read_timeout עם received_bytes. anyio הוא תלות ישירה של mcp (anyio>=4.5) ושל starlette ולא נעוץ בנפרד, כמו starlette עצמו.
    3. שער לפי method (WARN-002): רק POST/PUT/PATCH נבדקות (METHODS_WITH_BODY); GET /healthz עם Content-Length מזויף — 200 במקום 413.
    • שני הסירובים נושאים Connection: close (h11 מכבה keep-alive על תשובה שנושאת אותו, uvicorn סוגר את ה-transport — מצוטט מהמקור בקוד); בלעדיו השרת היה ממשיך לקרוא ולזרוק את שארית הגוף שסירבנו לו. ה-413 על אורך מוצהר גם הוא סוגר עכשיו (לפני: החיבור נשאר פתוח — נמדד).
    • test_in_oauth_mode_an_anonymous_post_is_a_401_before_a_byte_of_its_body_is_read — טסט סדר-ההתקנה למצב OAuth לצד זה של מצב PAT; test_a_spoofed_length_on_the_health_check_is_still_a_200_in_both_modes על האפליקציה האמיתית בשני המצבים.
    • המשפט "הפטור מבני" תוקן בשלושה מקומות (docstring המודול, ההערה ב-build_app, docs/mcp-server.rst): הפטור מהמגביל מבני; הפטור מתקרת הגוף הוא כלל (GET אינה מתודה שנושאת גוף), לא מבנה.
  • WARN-001 — tests/test_mcp_app_wiring.py: תת-תהליך שמייבא את mcp_server.app כמו uvicorn (עם database מזויף ב-sys.modules ו-MCP_REPO_AUTOSYNC=0), בשני מצבי האימות, ועם ריצת בקרה בלי המשתנים; קורא את max_bytes מה-BodySizeLimitMiddleware המותקן ואת per_minute דרך תת-מחלקה על mcp_server.server.ToolRateLimiter. ריצה שלישית מוכיחה ש-MAX_CODE_SIZE מהסביבה מעלה את התקרה (SUGG-022 בנקודת הכניסה).
  • WARN-003 — הוחלט: שתי הדרכים נדחו לעת עתה (העברה לתפר המנוטר הייתה סופרת קריאה אבל לא סירוב — ל-PostHog אין hook על התוצאה — ומעמידה את השער על מאפיין פרטי שגם PostHog מחליף; אירוע משלנו הוא עותק של צורת האירוע של ה-SDK). התיעוד אומר עכשיו במפורש שסירובי מכסה אינם נספרים ב-PostHog, ושגם סירוב מגוף כלי נרשם שם כהצלחה (is_tool_result_error מסמן רק isError/חריגה — מצוטט). האישו #3442 נושא את ההחלטה ואת מה שנשאר לעצב.
  • WARN-004 — scripts/measure_md_parse_cost.py: השורה העוינת מקבלת max_sections=None במפורש (docstring, הערה ו-note בפלט עודכנו); הטסט מקבע את ה-kwargs של כל שורה.
  • SUGG-022 — request_bytes_for(max_code_chars) ב-limits.py: MAX_CODE_SIZE × 6 + 64KiB, מעוגל ל-MiB שלם (1MiB על ברירת המחדל). DEFAULT_MAX_REQUEST_BYTES נגזר ממנו, ו-create_app גוזר מהתצורה בפועל (handlers.max_code_size(), שהפך ציבורי מ-_max_code_size). טסטים: הצמדה (handlers.DEFAULT_MAX_CODE_SIZE == BotConfig.model_fields["MAX_CODE_SIZE"].default, המספר המתועד, ותקרת קוד אחרת נותנת תקרה אחרת), בקשת codekeeper_save_file על קובץ עברי בגודל המקסימלי בקידוד ensure_ascii נכנסת מתחת לתקרה, ותצוגת התצורה מצמידה שם וברירת מחדל לקבועים (חלק מ-SUGG-014).
  • SUGG-009 — הסירוב נבנה כ-CallToolResult שלם (_refusal_result): מטפל ה-tools/call של השרת הנמוך מחזיר אותו כמות שהוא, בלי ולידציה מול outputSchema — האב-טיפוס של הסקירה ([TextContent]) היה נופל שם ל-isError לכלי עם טיפוס החזרה ("outputSchema defined but no structured output returned"), וזה נבדק דרך המטפל האמיתי בטסט נפרד. כלי -> list[str] ושם כלי לא מוכר מקבלים rate_limited.
  • SUGG-023 — is None במקום or; טסט עם תת-מחלקה שמגדירה __len__ שמחזיר 0 (עם or המגביל הכבוי הוחלף בברירת המחדל — הטסט נפל על הקוד הישן). SUGG-024 — asyncio.gather של 25 קריאות, תקציב 2: בדיוק 2 עוברות ו-23 נדחות. SUGG-011 — RateLimiter._live_entries אינו יוצר רשומה (dict רגיל, מפתח שהתרוקן נמחק, check_rate_limit שומר באישור), ו-_warned_at מנוקה במעבר שכותב חותמת חדשה; tests/test_rate_limiter_basic.py עובר כמות שהוא.
  • SUGG-019 — _entry_checks ב-services/md_parser.py: שער כניסה משותף (require_str, BOM, \r בודד) ל-parse_document, token_count ו-front_matter_end; ההערות על ה-BOM עברו אליו. scripts/measure_md_parse_cost.py::rank מדלג בקול על קובץ עם \r בודד, כמו על קובץ שאינו UTF-8. SUGG-013 — _body_start נקרא פעם אחת ב-_title, וה-docstring שלו אומר שקובץ עם \r בודד מרים InconsistentLineEndings במקום להזיז את הגבול בשקט.
  • קטנות: SUGG-002 (_CANDIDATES_MAX = doc_sections.MAX_IDENTIFIER_SUGGESTIONS), SUGG-020 (הרוחב נקרא מ-_read_pool_size במקום 10), SUGG-021 (התנאי המת ב-utils.normalize_code), SUGG-005/006/010 (הערות עם תאריכים: מספרי הכתיבה 7.5/4.3–4.9 שניות התיישנו אחרי המעבר לפרנקפורט ב-2026-09-16 — הלוגים מאז מראים 0.012–1.993 שניות ב-30 כתיבות; ההפניה ל-numInstances: 1 מצביעה על הגדרת השירות ב-Render; ה-docstring של _caller_identity נשען על שני מסלולי האימות ולא על "הגוף יסרב"), SUGG-001 (חצי משפט ב-whats-new שאומר איזו רשומה גוברת). YAGNI-001 — ToolRateLimiter.enabled נמחק; YAGNI-002 — MCP_MAX_REQUEST_BYTES נשאר ומתועד כהחלטה סגורה (לא kill switch; 0 אינו מכבה).
  • U3 בדרך: str.isdigit() על Content-Length היה מקבל ² ונופל על int() (מול uvicorn זה 400 עוד לפני ה-ASGI; מול TestClient — 500). עכשיו [0-9]+ כמו h11, וטסט.
  • תיעוד: docs/mcp-server.rst (טבלת הקבועים, סעיף "גבולות הבקשה" — הגזירה, מה נקרא ומה לא, שני חצאי הפטור, מה נרשם בלוג ו-PostHog, "כיוון" עם ההחלטה הסגורה, שורת פתרון התקלות), docs/environment-variables.rst (השורה של MCP_MAX_REQUEST_BYTES), docs/whats-new.rst (רשומה תחת 2026-09-21 + חצי המשפט של SUGG-001). בלי מספרי שורות, בלי שינוי עוגנים.

🧪 בדיקות

  • איך בדקתם? מה עבר? מה נשאר?
  • Unit
  • Integration
  • Manual

test-first, על שני העצים. הטסטים החדשים הורצו קודם על worktree של הקוד שלפני התיקון (5c6f30c5): 20 נפלו שם — שלושת חלקי SEC-001, טסט ה-OAuth, שני טסטי ה-/healthz המזויף, SUGG-009 (שלושה), SUGG-011 (שלושה), SUGG-022 (שניים), SUGG-023, ופנקס האזהרות — ו-test_a_declared_length_over_the_cap... שקיבל טענה על Connection: close. שלושה טסטים חדשים עוברים גם על הקוד הישן בכוונה: test_a_declared_length_beside_transfer_encoding_is_not_trusted (שומר מפני מסלול מהיר נאיבי — נמדד שהוא מפיל אותו), test_the_budget_holds_under_concurrent_calls... (מוכיח תכונה קיימת) ו-test_a_declared_length_that_is_not_ascii_digits... (הגרסה הראשונה שלו לא בדקה כלום כי הכותרת קודדה ב-UTF-8; תוקנה לבתים כפי ש-Headers מפענח — b"\xb2" — ואז הקוד הישן נופל ב-ValueError).

המדידות (בקשת הסקירה: "הרץ את שלושת החלקים ודווח מספרים, כולל chunked אנונימי"). לפני = 5c6f30c5, אחרי = HEAD:

מהלפניאחרי
OAuth, POST /mcp אנונימי, Content-Length: 5000 (ASGI, קריאות receive() לפני ה-401)50
OAuth, POST /mcp אנונימי, chunked בלי אורך (המקרה שנשאר)55 — חסום בתקרה ובדדליין
uvicorn אמיתי, chunked אנונימי, נתח אחד ובלי סיוםאין תשובה אחרי 35.0 שניות, החיבור פתוח408 אחרי 30.005 שניות, Connection: close, השרת סגר את החיבור
uvicorn אמיתי, Content-Length: 1000 ורק 500 בתים נשלחו, אנונימיאין תשובה אחרי 8.0 שניות401 אחרי 0.001 שניות (האימות של ה-SDK עונה בלי לקרוא)
uvicorn אמיתי, GET /healthz עם Content-Length: 99999999413200
uvicorn אמיתי, POST /mcp אנונימי עם Content-Length: 2000000413, החיבור נשאר פתוח413, Connection: close, נסגר
PAT, GET /, /healthz, /health עם אורך מזויף413 (0 קריאות)404 / 200 / 404 (0 קריאות)
PAT, POST עם Content-Length: 500 לנתיבים הפטורים5 קריאות לפני התשובה0
PAT, POST chunked לנתיבים הפטורים55 — המקרה שנשאר
h11: Content-Length: ²400400 (לפני ה-ASGI)
h11: 20 בתים מאחורי Content-Length: 10האפליקציה קיבלה 1010
h11: Content-Length: 5 + chunked, 200 בתים, תקרה 100413 (received_bytes: 200)413
25 קריאות מקבילות, תקציב 2, זהות אחת2 עוברות, 23 נדחות2 / 23 (עכשיו טסט)

סוויטה: tests/ במלואה על העץ המתוקן (לפני מיזוג #3438, שנוגע רק בקבצי sticky notes): 5,899 עברו, 190 דילוגים קיימים, 5 נפלו — כולן ב-tests/test_infrastructure.py::TestEnvironmentTools ו-TestServiceIntegration::test_service_detects_tools, כי isort ו-autopep8 אינם מותקנים ב-venv של הסשן; אותן חמש נופלות זהה על העץ שלפני התיקון, ו-CI מתקין את הכלים. בקבצים הנוגעים בדבר — 377 עברו. flake8 נקי על כל הקבצים שהשתנו (ב-webapp/push_api.py יש אזהרות W293/F841 קיימות מלפני, בשורות שלא נגעתי בהן). שלושת עמודי התיעוד עברו פרסור docutils בלי הודעות; RTD יתפוס אזהרות Sphinx ב-check של ה-PR (לא נבנה מקומית — פרוזה בעמודים קיימים בלבד, לפי CLAUDE.md).

מקורות שנקראו (source-driven): mcp/server/lowlevel/server.py (מטפל tools/call: CallToolResult מוחזר כמות שהוא, ולידציית outputSchema), mcp/server/fastmcp/utilities/func_metadata.py (_convert_to_content, convert_result), mcp/types.py (CallToolResult, TextContent), posthog/mcp/_instrumentation.py (is_tool_result_error, record_tool_call), h11/_headers.py ([0-9]+), h11/_connection.py (_keep_alive), uvicorn/protocols/http/h11_impl.py (send, receive), anyio/_core/_tasks.py (fail_after), httpx/_content.py (encode_content), מפרט ASGI (method uppercased), MDN על 408 ו-413 (ה-RFC עצמו לא נמשך בהצלחה — הציטוט על Connection: close הוא מ-MDN, וכתוב כך בקוד). לא אימתתי את התנהגות הייצור אחרי הדיפלוי (אין preview); הצפוי כתוב בקוד ובתיעוד.

עמודי תיעוד שנקראו לפני הכתיבה: AI-MAP.md, docs/doc-authoring.rst, docs/versioning-stable-anchors.rst, docs/testing.rst, docs/mcp-server.rst, docs/environment-variables.rst, docs/whats-new.rst. דפוסי באגים (amir-bug-patterns) שנקראו במלואם בסבב הסקירה והוחלו כאן: K5/network-exposed-without-auth, K11, K12 §3, K13, K15/lazy-init-guard-publish-order, K16, U1/race-toctou, U3/external-input-isinstance, R6, R7, R9/observability, TESTING-PATTERNS T1(d)/T2, claude-md-snippets/testing.md, external-sdk (11, 13), silent-fallback-to-worse-path, state-record-without-state-change, blanket-policy-silent-block §7, silent-truncation-at-sink, import-time-side-effects, widened-exception-scope, line-number-coupling.

סיכוני Rollback: אין מיגרציה. שינוי חוזה קטן ומכוון: סירוב מכסה מוחזר כ-CallToolResult (אותו בלוק טקסט ללקוח; ל-PostHog הוא ממילא אינו מגיע), שני הסירובים של תקרת הגוף סוגרים את החיבור, ו-GET אינו נבדק. השפעה על Deploy: אין משתנה חדש; MCP_MAX_REQUEST_BYTES וברירת המחדל (1MiB) לא זזו.

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

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

📝 סוג שינוי

  • fix: תיקון באג
  • docs: שינוי תיעוד בלבד
  • feat: פיצ'ר חדש
  • refactor: שינוי קוד ללא שינוי התנהגות
  • perf: שיפור ביצועים
  • chore/ci: תשתית/CI
  • breaking change: שינוי שובר תאימות

🤖 Generated with Claude Code

https://claude.ai/code/session_01SfJTSpDAhDr2yhtmpFkwTx