מעקב: יתרת ממצאי הסקירה על PR #3428 (אחת עשרה הצעות)

[amirbiron]

למה אישו אחד ולא אחד-עשר

אלה יתרת ההצעות מסקירת הקוד על PR #3428. ששת הממצאים שנבחרו לטיפול מיידי כבר ב-PR עצמו; אלה נשארו מרוכזים כאן בכוונה, כי פיצול של אחת-עשרה הצעות ל-אחת-עשרה אישוז יוצר תור שאיש לא עובר עליו.

אף אחת מהן אינה משנה את מה שמשתמש רואה היום. הדוח המלא, עם ההסבר וההוכחה לכל ממצא: code-review-3428-docs-path-policy.md ב-CodeKeeper.

טסטים שמקבעים משהו אחר ממה שהם מצהירים

• SUGG-004 · tests/test_mcp_repo_policy.py:236 — assert returned == allowed דורש שהקריאה והחיפוש יחסמו בדיוק את אותה קבוצה, בזמן ש-mcp_server/repo_policy.py מצהיר את ההפך ככוונה: "is_denied נשאר השכבה האחרונה — תבנית שהמנוע אינו יכול לבטא עדיין חוסמת כאן". תבנית שגיט אינו יכול לבטא תפיל את הטסט אף שהמערכת תתנהג כמתועד. האמירה הראשונה באותו טסט — שהחיפוש לעולם אינו מחזיר נתיב שהקריאה חוסמת — היא זו שנושאת את תכונת הבטיחות, והיא נכונה בכל מקרה.
• SUGG-005 · tests/test_mcp_repo_policy.py:160 — שני טסטים תלויי-git מתנהגים אחרת כשאין git: אחד קורא pytest.skip, והשני משתמש ב-check=True ויקרוס. ה-importorskip בודק את מודול הפייתון ולא את הבינארי.

פערי כיסוי

• SUGG-009 · mcp_server/docs_handlers.py — שתי בדיקות הנרמול ב-_validate_policy_tables (סיומת בלי נקודה מובילה או לא-casefolded, שורש מוחלט או לא מנורמל) עדיין בלי טסט. (הבדיקה השלישית, "רשימה ריקה", נעלמה ב-PR עם צמצום השדות לסקלרים.)
• SUGG-010 · mcp_server/docs_handlers.py — שורת ה-logger.warning על repo_not_configured בלי טסט. מחיקתה תשאיר את כל הטסטים ירוקים.

אבחון ותצפית

• SUGG-011 · mcp_server/docs_handlers.py — מראה שאינה קיימת על המארח מוחזרת כ-not_found, בדיוק כמו קובץ שבאמת חסר. סוכן שמבקש עמוד אמיתי מקבל "לא נמצא" ומנסה שם אחר, בזמן שהבעיה היא שאין למארח עותק של הריפו. קוד נפרד — repo_not_mirrored — מפריד ביניהם.
• SUGG-012 · services/config_inspector_service.py — משטח בדיקת התצורה אינו מייבא את docs_handlers ואינו קורא את DOCS_PATH_POLICY, ולכן אינו יכול להציג את התקלה היחידה שה-PR הזה מאפשר: ריפו שמותר ב-MCP_DOCS_REPO ואין לו מדיניות נתיבים.
• SUGG-022 · mcp_server/docs_handlers.py — תשעה מעשרה מסלולי הסירוב אינם כותבים שורת לוג, והפרוטוקול סופר כל סירוב כקריאה מוצלחת. מודגש שזו מוסכמת הריפו ולא פער של ה-PR הזה: ב-mcp_server/handlers.py יש 61 מסלולי סירוב ואפס לוגים, ב-repo_handlers.py שישה ואפס, והקובץ הזה הוא היחיד שיש בו לוג בכלל. אם משנים — משנים את השכבה, לא קובץ אחד.

בהירות ומבנה

• SUGG-013 · mcp_server/docs_handlers.py — missing_path עדיין מכסה שתי דחיות שונות (קלט ריק או NUL, ונתיב מחוץ לשורש) בטיפוס שנבנה כדי להפריד ביניהן. (דחייה שלישית, path_too_long, כבר קיבלה קוד משלה ב-PR.)
• SUGG-018 · mcp_server/repo_policy.py + services/git_mirror_service.py — מדיניות אחת כתובה פעמיים בשתי שפות: fnmatch על רכיבים בפייתון, וארבע צורות pathspec מול git. רק רשימת תבניות משותפת וטסט אינטגרציה אחד מחזיקים אותן יחד, והן כבר נפרדו פעם אחת. (מאז ה-PR יש גם טסט ישיר שמקבע את ארבע הצורות.)
• SUGG-020 · mcp_server/docs_handlers.py — docs_get_section גדל ל-175 שורות ומשלב חמישה תפקידים: פתרון ריפו, פתרון נתיב, קריאת קובץ, בחירת פארסר, ועיצוב ארבעה מצבי תשובה. הזנב שמעצב את התשובה הוא החלק שנפרד הכי נקי.
• SUGG-021 · mcp_server/docs_handlers.py — _PARSERS ו-DOCS_PATH_POLICY הם מילונים ניתנים לשינוי שנבדקים פעם אחת בייבוא. שינוי אחרי הייבוא עוקף את הוולידטור, והטסטים נשענים על זה. MappingProxyType שומר על ההבטחה ומשאיר ל-monkeypatch.setitem את האובייקט שמתחת.

מה שכבר נסגר ב-PR

SUGG-001, 002, 003, 006, 007, 008, 014, 015, 016, 017, ו-YAGNI-001 — ולצדם WARN-001 ו-WARN-002. היעדר תקרת גוף בקשה והגבלת קצב בשרת הופרד לאישו #3431, כי הוא נוגע לכל הכלים ולא לזה.