docs: הרחבת שורת הטריגר — כל guard שמונע הערכה, לא רק isEnabledFor - #3246
Conversation
אירוע אמיתי מפרודקשן (CODEKEEPER-2Z) הראה את הטוקן המלא בתוך breadcrumbs של httplib — הערוץ שה-before_send העמוק מכסה, אבל רק באירועי שגיאה. אירועי ביצועים (transactions, נדגמים ב-10%/5%) נושאים את אותם URL-ים ב-spans ועוקפים את before_send לחלוטין. נוסף before_send_transaction עם אותו ניקוי fail-closed בשני השירותים, וטסט על אירוע בצורת transaction (spans + breadcrumbs + request.url). Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019YULCppaQRPYN1RgeY6NBu
שלושה ממצאי ריוויו על שכפולים שנוצרו תוך כדי הסבבים: - נוסף scrub_sentry_event ב-telegram_api — הניקוי העמוק + fail-closed במקום אחד, ושני ה-before_send (בוט ווובאפ) מאצילים אליו במקום להחזיק עותקים שכבר התחילו להתפצל. - הדפוס נחשף כ-BOT_TOKEN_RE ציבורי, ו-SensitiveDataFilter מייבא אותו ישירות בלי עותק fallback מקומי — עותק כזה מתפצל בשקט מהמקור, וזה גרוע יותר מכשל ייבוא קולני (telegram_api תלוי רק בספריה הסטנדרטית). - שלושת טסטי ה-Formatter חלקו תשתית של 13 שורות — אוחדה ל- _filtered_record_for, וכל טסט מספק רק את החריגה ואת ה-asserts. נוסף טסט ל-scrub_sentry_event: ניקוי תקין + החזרת None בכשל. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019YULCppaQRPYN1RgeY6NBu
…ר טוקן repr(None) לעולם לא מכיל טוקן, כך שהבדיקה הקודמת הייתה עוברת גם אם הפונקציה הייתה מפילה כל אירוע. עכשיו מאומת שהאירוע חזר ושההודעה נוקתה לערך המדויק. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019YULCppaQRPYN1RgeY6NBu
המונים ("1529 files" בכותרת ובתחתית הסיידבר) רונדרו בשרת פעם אחת בזמן
טעינת הדף, מתוך `metadata` שנטען ב-`repo_index`. `updateRepoDisplay` —
הפונקציה שמרעננת את ה-UI בהחלפת ריפו — עדכנה שם ריפו, dropdown ופריט
פעיל, ולא נגעה בהם. לכן בכל ריפו שעברו אליו בלי לרענן את הדף הוצג מספר
הקבצים של הריפו הראשון.
זה דפוס U5 / `linked-field-atomicity`: פעולה לוגית אחת דורשת עדכון של
קבוצת שדות מקושרים, והקוד מעדכן חלק מהם ושוכח את השאר.
**התיקון**
הוספת שתי שורות ל-`updateRepoDisplay` הייתה מתקנת את התסמין ומשאירה את
השורש — המונים ממולאים משני מסלולים נפרדים, ואף אחד לא מחייב שיישארו
מסונכרנים. במקום זה כל אלמנט מונה מסומן ב-`data-repo-stat`, ופונקציה
אחת (`renderRepoStats`) היא נקודת העדכון היחידה אחרי טעינת הדף. מונה
חדש שיסומן בתבנית יתעדכן מאליו.
הערכים נלקחים מ-`repoMetadataByName` שכבר יושב בזיכרון מ-`/api/repos`,
ולכן העדכון סינכרוני. בדקתי לפני שכתבתי: אין `await` במסלול, ולכן דפוס
U2§5 (תגובה מאוחרת שדורסת חדשה) לא חל וההגנה שתכננתי מיותרת.
כשאין מטא-דאטה לריפו הפונקציה לא נוגעת בכלום, כדי שכשל רגעי של
`/api/repos` לא ימחק ערך תקין שהשרת רינדר.
**הסרת מוני השפות**
"979 48" בתחתית היו מספר קבצי Python ו-JavaScript, עם `title` בלבד —
ובטאבלט אין hover, אז הם נראו כמו מספרים אקראיים. הוסרו לבקשת המשתמש.
בעקבות זה האגרגציה ב-`repo_index` שחישבה אותם הפכה לקוד מת והוסרה,
מה שחוסך שאילתת aggregation ב-MongoDB בכל טעינת דף. `/api/file-types`
נשאר — הוא משרת את סרגל הסינון ולא קשור.
**בדיקות**
`tests/test_repo_browser_stats_sync.py`. הסימונים נגזרים מה-HTML שנוצר
ולא מרשימה קשיחה, לפי כלל 1 ב-`claude-md-snippets/testing.md`.
כל ארבעת השומרים אומתו מול הקוד שלפני התיקון ונפלו: הסרת הקריאה
מ-`updateRepoDisplay`, מונה מסומן שאיש לא מטפל בו, חזרת מוני השפות,
והסרת היציאה המוקדמת. הבדיקות סטטיות ולא מריצות דפדפן — המגבלה מתועדת
בקובץ עצמו.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019YULCppaQRPYN1RgeY6NBuהקומיטים שבענף המרוחק מוזגו ל-main ב-#3237 (squash), ולכן ה-SHA שלהם שונה אף שהתוכן כבר שם. אימתתי: before_send_transaction ו-scrub_sentry_event קיימים ב-main. ה-merge הזה מאחד את ההיסטוריות בלי להחזיר את התוכן הישן, כדי לא לדרוס את הענף המרוחק ב-force push.
שלושה ממצאים מ-cubic, כולם אומתו מול הקוד ותוקנו.
**1. `Number(null)` הוא 0**
`getRepoTotalFiles` השתמשה ב-`Number()` לפני `Number.isFinite`. אימתתי:
`Number(null)`, `Number(false)` ו-`Number('')` כולם מחזירים 0, ו-`isFinite`
מאשר אותם. ריפו שסונכרן חלקית היה מציג "0 files" — מספר שנראה אמיתי ואינו.
עכשיו נבדק `typeof === 'number'` לפני הכל.
לא אימצתי את ההצעה לתמוך גם במחרוזות: הערך נכתב ב-`repo_sync_service` וב-
`git_mirror_service` כ-`len(...)`, כלומר `int` בלבד, וטיפול במחרוזות היה
קוד ספקולטיבי לתרחיש שלא קיים.
**2. הערה שהבטיחה יותר ממה שהקוד עושה**
ההערה טענה שמונה חדש שיסומן בתבנית "יתעדכן מכאן מאליו", אבל `values` ממפה
`total_files` בלבד — מונה בלי מיפוי יוצג כ-`—`. ההערה תוקנה לתאר את שתי
הפעולות הנדרשות בפועל.
**3. הטסטים בדקו צורה ולא התנהגות**
זה הממצא המשמעותי. `assert "renderRepoStats" in body` היה עובר גם אם הקריאה
מופיעה רק בהערה, וכל הסוויטה הייתה ירוקה מול רגרסיה ששומרת על הצורה ושוברת
את הפלט.
`tests/test_repo_browser_stats_behavior.py` מריץ את `renderRepoStats` ואת
`getRepoTotalFiles` עצמן, כפי שהן כתובות בקובץ, מול DOM מינימלי ב-Node.
שתיהן נוגעות רק ב-`querySelectorAll`, `dataset` ו-`textContent`, ולכן כפיל
קטן מספיק. לא נעשה שימוש ב-Playwright: הוא לא מותקן וה-CI לא מריץ אותו,
כך שטסט כזה לא היה מגן על כלום.
מדדתי את ההפרש על שלוש רגרסיות אמיתיות — כמה טסטים נופלים בכל שכבה:
| רגרסיה | סטטי | התנהגותי |
|---|---|---|
| חזרה ל-`Number()` | 0 | 4 |
| סלקטור שגוי | 0 | 10 |
| קריאת שדה שגוי | 0 | 3 |
השכבה הסטטית נשארת — היא תופסת מונה שנוסף לתבנית בלי מיפוי, וזה דבר
שהתנהגותי לא רואה. המגבלה שנותרה מתועדת בקובץ: פריסה בדפדפן, CSS וסדר
טעינה דורשים דפדפן אמיתי.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019YULCppaQRPYN1RgeY6NBuהצד השני של סגירת הלולאה: הדפוס תועד ב-amir-bug-patterns PR #12, וכאן נוספת שורת הטריגר שתגרום לו להיקרא בזמן המימוש. דפוס בלי טריגר הוא דפוס שלא ייקרא. הטריגר שלו הוא פעולת עריכה ולא נושא, ולכן לא היה מכוסה באף שורה קיימת: מי שמתקן ממצא PII בלוג לא מחפש בטבלה "לוגים" — הוא פשוט מוחק שורה. הדפוס השני מאותו PR (אובייקט SDK עצל) כבר מכוסה בשורת "PyGithub / קריאות SDK חיצוני", ולכן לא נוספה עבורו שורה. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019YULCppaQRPYN1RgeY6NBu
הניסוח הקודם ("וירידה ל-debug") היה לא מדויק: ב-Python הארגומנטים של
קריאת הלוג מוערכים לפני הקריאה, גם כשהרמה מנוטרלת — ולכן ירידה ל-debug
לבדה אינה מסירה את תופעת הלוואי. ה-guard היחיד שכן עוצר את ההערכה הוא
`if logger.isEnabledFor(...)`, והוא הטריגר הנכון.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019YULCppaQRPYN1RgeY6NBu`isEnabledFor` הוא ה-guard הנפוץ אבל לא היחיד: `if verbose:`, דגל פיצ'ר, או כל תנאי חוסם מונעים את הערכת הארגומנטים באותה מידה. הניסוח הקודם היה מפספס אותם. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019YULCppaQRPYN1RgeY6NBu
# Conflicts: # CLAUDE.md
There was a problem hiding this comment.
Sorry @amirbiron, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing |
Warning Review limit reached
Next review available in:35 minutes Limit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. 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. Comment |
🧯 Dangerous deletes guard reportPolicy: see .cursorrules — dangerous deletions are blocked unless wrapped safely. Summary:
Flagged findings (file:line:snippet): Excluded matches (by path pattern) |
Reviewer's guide (collapsed on small PRs)Reviewer's GuideUpdates documentation wording in CLAUDE.md to generalize the description of the logging guard from a specific isEnabledFor call to any guard that prevents argument evaluation, aligning docs with actual behavior and related bug pattern rules. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
⏱️ Performance report(No performance test durations collected. Mark tests with |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
No issues found across 1 file
Tip: cubic could auto-approve low-risk PRs like this, if it thinks it's safe to merge. Learn more
Re-trigger cubic
Uh oh!
There was an error while loading. Please reload this page.
תבנית Pull Request
✨ תיאור קצר
תיקון ניסוח לשורת הטריגר שנוספה ב-#3245. ריוויו על
amir-bug-patternsPR #12 תפס שהניסוח מציג אתisEnabledForכ-guard היחיד שמונע הערכת ארגומנטים. זו הכללה:if verbose:, דגל פיצ'ר, או כל תנאי חוסם אחר עושים בדיוק אותו דבר. ריוויוור שקורא את השורה כפי שהיא עלול להחיל את הכלל רק עלisEnabledForולפספס את השאר.📦 שינויים עיקריים
פירוט נקודות:
CLAUDE.mdשורה 54 —עוטף אותה ב-isEnabledForהוחלף ב-עוטף אותה ב-guard שמונע הערכת ארגומנטים.למה לא נגעתי ב-
integrations.py: באותו סבב ריוויו התגלה שהתיעוד ב-amir-bug-patterns טען דבר הפוך מהמציאות לגביlazy=False. אימתתי מול המקור המותקן (PyGithub 2.9.1) ובהרצה שסופרת בקשות בפועל, והתמונה היא:get_user()get_user(lazy=False)get_user(lazy=True)get_user("octocat")כלומר
get_user(lazy=False)היה מנסח את הכוונה טוב יותר מהשורה_ = user.loginשיש היום בבנאי. אבל הקוד הקיים נכון — נמדד:get_user()ואז גישה ל-loginשולחים בדיוק בקשה אחת, ול-AuthenticatedUserאין את מלכודת גזירת השדה מה-URL שקיימת ב-NamedUser. מעבר ל-lazy=Falseהיה מחייב לשכתב שלושה כפילים ב-tests/test_gist_per_user.pyולשנות את המשמעות שלtest_token_is_actually_validated_not_just_constructed, וזה שינוי בקוד ממוזג ועובד שלא נכלל בבקשה. מציע אותו כהמשך נפרד.🧪 בדיקות
שינוי ניסוח בקובץ מדיניות; אין קוד להריץ. הטענה שהניסוח נשען עליה אומתה בהרצה: עם רמת
debugמנוטרלת הביטוי בתוך ה-f-string עדיין הוערך, ורק עטיפה בתנאי חוסם אפסה אותו.🧪 בדיקות נדרשות ב‑PR
📝 סוג שינוי
✅ צ'קליסט
🧩 השפעות/סיכונים
אין. שורה בקובץ מדיניות, ללא נגיעה בקוד רץ.
🔗 קישורים
amirbiron/amir-bug-patternsPR Fix render runtime error by removing file logging #12🧯 סיכון / החזרה לאחור (Rollback)
git revertלקומיט הבודד.Generated by Claude Code
Summary by Sourcery
Clarify the trigger guidance for preventing side effects on logging lines by referring to argument-evaluation guards generally.
Enhancements:
isEnabledFor.Documentation:
CLAUDE.mdto clarify that log lines may be wrapped in any argument-evaluation guard when addressing side effects.