אישור שליחת הודעת אדמין - #3071
Conversation
Co-authored-by: amir haim <amirbiron@users.noreply.github.com>
Co-authored-by: amir haim <amirbiron@users.noreply.github.com>
|
Cursor Agent can help with this pull request. Just |
🧯 Dangerous deletes guard reportPolicy: see .cursorrules — dangerous deletions are blocked unless wrapped safely. Summary:
Flagged findings (file:line:snippet): Excluded matches (by path pattern) |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
⏱️ Performance report(No performance test durations collected. Mark tests with |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Co-authored-by: amir haim <amirbiron@users.noreply.github.com>
📖 Documentation PreviewThe documentation has been built successfully!
To view locally:
|
Co-authored-by: amir haim <amirbiron@users.noreply.github.com>
Co-authored-by: amir haim <amirbiron@users.noreply.github.com>
Co-authored-by: amir haim <amirbiron@users.noreply.github.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
|
|
||
| sent_direct = await _send_direct_admins(context, report) | ||
| if sent_direct: | ||
| await message.reply_text("כרגע Alertmanager לא זמין, אבל שלחתי את הדיווח ישירות לאדמין.") |
There was a problem hiding this comment.
Fallback bypasses Rule Engine and internal alerts
High Severity
The new /admin command flow bypasses notify_admins() entirely. When Alertmanager fails and _send_direct_admins() is used as fallback, messages are sent directly via bot.send_message, which bypasses both emit_internal_alert() and the Rule Engine's suppress/routing logic. The existing notify_admins() function (line 734) explicitly documents that admin messages should not be sent directly to avoid bypassing the Rule Engine. This breaks alert consolidation, suppression rules, and observability for fallback cases.
| if low.endswith(":443"): | ||
| scheme = "https" | ||
| elif low.endswith(":9093") or low.endswith(":80") or low.startswith("localhost") or low.startswith("127.") or low.startswith("alertmanager"): | ||
| scheme = "http" |
There was a problem hiding this comment.
Scheme guessing fails for non-standard HTTPS ports
Low Severity
The scheme guessing logic checks if the hostname starts with alertmanager and defaults to http, which incorrectly handles URLs like alertmanager.prod.example.com:8443 or alertmanager-api.example.com:9443 where HTTPS is running on non-standard ports. The check for startswith("alertmanager") on line 778 takes precedence over port-based detection for any port other than 443, causing HTTPS requests to fail. Users can work around this by using ALERTMANAGER_API_URL with the full scheme.


✨ תיאור קצר
פקודת
/adminבבוט הטלגרם תאשר למשתמש שהדיווח נשלח רק לאחר קבלת אישור הצלחה (HTTP 2xx) מ-Alertmanager. במקרה של כשל ב-Alertmanager, הבוט ינסה לשלוח את הדיווח ישירות לאדמינים ויעדכן את המשתמש בהתאם, או יודיע על כשל מוחלט.📦 שינויים עיקריים
פירוט נקודות:
/adminב-main.pyשונתה כך שהיא ממתינה לתשובת הצלחה מ-Alertmanager._send_admin_report_via_alertmanagerשמבצעת קריאת HTTP POST ל-Alertmanager ומחזירהTrueרק על קבלת סטטוס 2xx._get_alertmanager_api_alerts_urlלבניית URL של Alertmanager מתוך משתני סביבה._send_direct_admins.ALERTMANAGER_API_URL,ALERTMANAGER_API_TOKEN,ALERTMANAGER_TARGETותועדו ב-docs/environment-variables.rstוב-services/config_inspector_service.py.tests/test_admin_report_command_alertmanager_ack.pyהמכסות את תרחישי ההצלחה והכשל.🧪 בדיקות
השינויים נבדקו באמצעות בדיקות יחידה חדשות המדמות תרחישי הצלחה של Alertmanager, כשל ב-Alertmanager עם הצלחת Fallback, וכשל מוחלט בשני המסלולים. כל הבדיקות עברו בהצלחה.
🧪 בדיקות נדרשות ב‑PR
📝 סוג שינוי
✅ צ'קליסט
services/register_jobs.py(כולל Callback/Trigger להפעלה ידנית — למשלcallback_name/trigger_funcלפי המבנה) כדי שיופיעו בדשבורדdocs/environment-variables.rstוגםservices/config_inspector_service.pydocs/webapp/theming_and_css.rst+FEATURE_SUGGESTIONS/theme_matrix.md🧩 השפעות/סיכונים
/admin. אם Alertmanager אינו זמין, המשתמש יקבל על כך הודעה מפורשת./adminעקב המתנה לתשובת HTTP מ-Alertmanager (עם Timeout של 6 שניות).🔗 קישורים
🧯 סיכון / החזרה לאחור (Rollback)