דפוסי באגים חדשים מ-PR #3306 — לתיעוד ב-amir-bug-patterns

[amirbiron]

PR #3306 הפיק שלושה ממצאי ריוויוור אמיתיים שאינם מכוסים ב-amir-bug-patterns. לפי כלל "סגירת הלולאה" ב-README.md שם, ריוויוור שתפס ממצא אמיתי שאינו מכוסה מחייב תיעוד באותו יום, בזמן שהראיות טריות. האישיו הזה מרכז את שלושתם, מנוסחים בפורמט של הריפו ומוכנים להעתקה.

בדקתי שאף אחד מהם אינו מכוסה: חיפשתי fallback, כפילות ו-מסונכרן בכל הריפו, וקראתי את widened-exception-scope.md, migration-model-drift.md, linked-field-atomicity.md, input-field-not-persisted.md, external-input-isinstance.md, TESTING-PATTERNS.md ו-test-mirrors-spec-not-client.md. הקרובים ביותר נוגעים בקצוות ולא במרכז — הפירוט בכל סעיף.

---

דפוס 1 — עותק fallback של כלל קנוני (העיקרי)

שם מוצע: bugbot-rules/fallback-copy-of-canonical-rule.md

איך זה נראה

כלל שחייב להיות זהה בין שכבות מוגדר במודול כבד. צרכן בשכבה אחרת אינו יכול לייבא ממנו בבטחה, ולכן נכתב:

try:
    from heavy.module import the_rule
except Exception:            # pragma: no cover
    def the_rule(...):       # עותק מילה במילה
        ...

שני כשלים נפרדים שמצטברים:

1. העותק חייב להישאר מסונכרן ידנית. כל שינוי עתידי בכלל הקנוני — הוספת שדה, שינוי טיפול ב-falsy, קבלת טיפוס נוסף — מחזיר בשקט את הבאג שזה עתה תוקן, בסביבה אחת בלבד. זה הסוג הכי קשה לשחזור.
2. ה-except הרחב מסתיר למה הפולבק נכנס לפעולה. הוא תופס לא רק מודול חסר אלא כל חריגה שנזרקת בזמן ייבוא המודול הכבד. תקלה אמיתית מתדרדרת בשקט לעותק ישן, בלי שום אות.

שורש

הכלל יושב בשכבה הלא נכונה. התיקון אינו הצרת ה-except ל-ImportError, ואינו בדיקה שמוודאת שהעותקים זהים — שניהם טלאים שמקבעים את הכפילות. השורש הוא חילוץ הכלל למודול דל-תלויות ששתי השכבות מייבאות ישירות, ואז הפולבק מיותר לגמרי.

כלל לזיהוי

1. try/except סביב ייבוא, שגוף ה-except שלו מגדיר פונקציה או קבוע באותו שם שהייבוא מספק. זו החתימה המובהקת.
2. אותו גוף פונקציה מופיע פעמיים או יותר בריפו, כשאחד המופעים בתוך except.
3. הצהרת "מקור אמת אחד" בהערה, ב-docstring או בגוף PR, בזמן שיש יותר מהגדרה אחת.
4. ייבוא בפולבק שתופס רחב מ-ImportError.

False positives

• תלות אופציונלית אמיתית עם התנהגות מדורגת מוצהרת — try: import ujson / except ImportError: import json. זה תחליף עם אותו חוזה, לא עותק של הלוגיקה שלך.
• פולבק שנכשל בקול: מלוגג ברמת error עם החריגה שנתפסה, ומכבה את הפיצ'ר — במקום לשכפל אותו בשקט.
• ייבוא תחת TYPE_CHECKING.

מצב מומלץ

מודול דל-תלויות שכל הצרכנים מייבאים ישירות, בלי try/except. אם פולבק באמת בלתי נמנע — הוא חייב להיכשל בקול ולא להיות עותק של הלוגיקה.

חומרה

MEDIUM-HIGH — סטייה שקטה. הבאג שתוקן חוזר בסביבה אחת בלבד, בלי שגיאה ובלי לוג.

ראיות — CodeBot PR #3306

inherited_created_at הוגדר ב-database/repository.py ושוכפל מילה במילה בתוך except Exception ב-webapp/app.py וב-webapp/collections_api.py. cubic וסוכן תחזוקתיות סימנו את שניהם — חמש הערות נפרדות שהן שני באגים.

למה ה-try/except נולד מלכתחילה, וזה החלק המעניין: database/__init__.py:11 מריץ DatabaseManager() ברמת המודול, וה-__init__ שלו מסתיים ב-connect(). כלומר ייבוא של כל תת-מודול תחת database/ פותח חיבור למסד. בנוסף, כ-25 טסטים מחליפים את sys.modules["database"] ב-types.SimpleNamespace שאינו חבילה, ואז from database.X import ... נופל ב-ModuleNotFoundError: database is not a package. הפולבק היה תגובה נכונה לאילוץ אמיתי — אבל לתסמין, לא לשורש.

התיקון: חילוץ ל-file_dates.py בשורש הריפו, datetime ו-typing בלבד. שלושה ייבואים ישירים, אפס פולבקים. תקדים קיים בריפו: user_roles.py (שה-docstring שלו מתאר בדיוק את אותו סיפור — פונקציה שהייתה משוכפלת מילה במילה בשני מקומות), sticky_notes_scope.py, sticky_notes_target.py.

ראה גם

widened-exception-scope.md — תופס את חצי ה-except, לא את חצי הכפילות. migration-model-drift.md — עוסק בסטייה בין שתי הצהרות, אבל ספציפי ל-migrations של SQL.

---

דפוס 2 — פורמט תצוגה כמכריע החלטה סמנטית

שם מוצע: bugbot-rules/display-format-as-semantic-decision.md

איך זה נראה

קוד מקבל החלטה לוגית מתוך השוואה בין ערכים שכבר עברו פורמט לתצוגה, במקום מתוך הערכים הגולמיים. הפורמט תמיד מאבד מידע, ולכן ההשוואה עונה על שאלה אחרת מזו שהתכוונו לה.

ב-CodeBot: התבנית הסתירה שורת "עודכן" באמצעות {% if file.updated_at != file.created_at %}, כששני הערכים כבר מפורמטים כ-%d/%m/%Y %H:%M. עריכה שקרתה באותה דקה שבה הקובץ נוצר נעלמה מהמסך לגמרי.

שני נזקים

1. אובדן דיוק. הפורמט מעגל, קוטם או ממיר אזור זמן, וההשוואה יורשת את זה.
2. צימוד. החלטה סמנטית נקשרת לפורמט התצוגה. שינוי עתידי של הפורמט — למשל לתאריך בלי שעה — משנה בשקט איזה מידע מוצג, בלי שאף אחד קישר בין השניים.

כלל לזיהוי

1. השוואה (==, !=, >, sorted, set, in) בין ערכים שעברו strftime, format_*, humanize, round, f"{x:.2f}", או str() על מספר או תאריך.
2. תנאי בתבנית (Jinja/Django/JSX) בין שני שדות שהראוט כבר פירמט.
3. שדה שנשלח לתצוגה וגם משמש כמפתח החלטה — למשל dedup או קיבוץ.

מצב מומלץ

ההחלטה נקבעת מהערכים הגולמיים בשכבה שיש לה גישה אליהם, ונשלחת לתצוגה כדגל בוליאני מפורש. התבנית מציגה, לא מכריעה.

False positives

• השוואה שכל מטרתה היא הטקסט עצמו — למשל "האם המחרוזת שהצגתי השתנתה מאז הרינדור הקודם".
• נרמול מכוון לצורך דה-דופ, כשהעיגול הוא הכוונה המוצהרת.

חומרה

MEDIUM — אובדן מידע שקט בתצוגה, וצימוד שהופך שינוי עיצובי לשינוי התנהגותי.

---

דפוס 3 — פייק מתירני שמתאים לכל מסמך (שייך ל-TESTING-PATTERNS.md)

ה-README.md של הריפו אומר במפורש: "לפני שמוסיפים דפוס — לשאול אם הוא בכלל באג בקוד. אם הבאג היה בפער בין מה שהבדיקות אימתו לבין מה שהמערכת עושה, מקומו ב-TESTING-PATTERNS.md". זה בדיוק המקרה — מוצע כ-T4.

איך זה נראה

פייק של מסד נתונים כתוב ביד. השאילתה מכילה אופרטור שהפייק לא מימש, והוא מדלג עליו במקום להיכשל:

if isinstance(cond, dict):
    if "$in" in cond and value not in cond["$in"]:
        return False
    ...
    return True     # ← $gt, $regex, $all, $elemMatch — כולם עוברים

התוצאה: השאילתה מחזירה את כל האוסף במקום את מה שביקשה. הטסט עובר, ובאג אמיתי בסינון של הראוט נשאר בלתי נראה.

כלל לזיהוי

1. continue או return True בענף "אופרטור לא מוכר" של פייק מסד נתונים.
2. מנוע התאמה שמדלג על מפתחות שאינם ברשימת שמות שדות ידועים.
3. פייק שמממש רק חלק מהאופרטורים שהקוד הנבדק באמת מריץ — הפער הוא בדיוק הסיכון.

מצב מומלץ — fail-closed, ולא זריקת חריגה

אופרטור שאינו ממומש מחזיר False. השאילתה מחזירה ריק, והאסרשן נופל בקול.

למה לא raise, וזו הנקודה הלא-אינטואיטיבית: בקוד פרודקשן טיפוסי כל קריאת DB עטופה ב-try/except Exception. חריגה מהפייק תיבלע שם, הראוט ימשיך עם None, והטסט יעבור מאותה סיבה שגויה בדיוק — רק עם יותר קוד. תוצאה ריקה, לעומת זאת, מתפשטת עד האסרשן. זה חיבור ישיר ל-widened-exception-scope.md: ה-except הרחב בפרודקשן הופך כל חוסר בפייק לשקט מוחלט.

חומרה

HIGH — לפי הניסוח ב-test-mirrors-spec-not-client.md: "ריבוי בדיקות ירוקות הוא עצמו הסיכון".

ראיות

הדפוס כבר קיים בריפו CodeBot פעמיים, מה שמקיים את כלל "אותו סוג טעות פעמיים = דפוס": tests/test_sticky_notes_repo_api.py:107-119 ו-tests/test_note_boards_api.py:110-122 — זהים שורה בשורה, שניהם עם return True. ו-tests/test_skill_manager.py:48-62 מדלג על כל מפתח שאינו אחד מארבעה שמות ידועים. מנגד, 13 מתוך 20 הפייקים בריפו כבר fail-closed — כלומר זה המיעוט, אבל מיעוט משוכפל.

---

מה נדרש בפועל ב-amir-bug-patterns

לפי README.md §"איך לתחזק":

1. docs/source-projects/codebot-patterns.md — שלושת הדפוסים כ-Pattern חדשים, עם ה-PR והקומיטים.
2. bugbot-rules/fallback-copy-of-canonical-rule.md ו-bugbot-rules/display-format-as-semantic-decision.md — קבצים חדשים.
3. TESTING-PATTERNS.md — T4 לדפוס 3.
4. הצלבה לפי ה-tier: כולם ממקור אחד וללא חומרת אבטחה, ולכן לא ל-CRITICAL-PATTERNS.md. אין קובץ BY-STACK/ שמתאים לדפוס 1 — הוא stack-agnostic. שווה לשקול העברה ל-CORE-PATTERNS.md רק אם יחזור בפרויקט נוסף.
5. claude-md-snippets/universal.md — דפוס 1 נכנס בשתי שורות. דפוס 3 נכנס ל-claude-md-snippets/testing.md.
6. שורות טריגר בטבלת CLAUDE.md של CodeBot: "עותק fallback מאחורי except סביב ייבוא" ← דפוס 1; "השוואה בין ערכים מפורמטים" ← דפוס 2; "פייק מסד נתונים כתוב ביד" ← דפוס 3. דפוס בלי שורת טריגר לא ייקרא בזמן המימוש — שני הצעדים הם צעד אחד.