Uns 611 clubbed notification dispatch#1959
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
b8bf719
into
UN-3056-Notify-on-API-deployment-failures
|
|
| Filename | Overview |
|---|---|
| backend/notification_v2/clubbed_renderer.py | Refactored to serve as the canonical envelope builder for both IMMEDIATE and BATCHED dispatch; %-d strftime is Linux-only and the STOPPED-status file-count display is misleading. |
| workers/notification/providers/_clubbed_format.py | New worker-side mirror of the backend renderer; shares the same %-d strftime portability issue. |
| backend/notification_v2/provider/webhook/api_webhook.py | Now wraps IMMEDIATE payloads in the canonical envelope; missing the double-wrap guard present in the worker counterpart. |
| workers/notification/providers/slack_webhook.py | Simplified by replacing Block Kit assembly with the shared _clubbed_format renderer; pass-through check is fragile if backend Slack payload shape evolves. |
| backend/notification_v2/provider/webhook/slack_webhook.py | Replaced custom Block Kit construction with the canonical render_clubbed_message call; clean simplification with no standalone issues. |
| workers/shared/patterns/notification/helper.py | Adds type, timestamp, and additional_data fields to the buffer enqueue payload; NotificationPayload always has additional_data via its default factory, so attribute access is safe. |
| backend/notification_v2/helper.py | Stamps a buffered-at timestamp when one is absent; straightforward and safe. |
| backend/notification_v2/internal_api_views.py | Passes optional type, timestamp, and additional_data fields from the request body into the buffer payload; backward-compatible. |
| workers/notification/providers/api_webhook.py | Wraps flat IMMEDIATE payloads in the canonical envelope with a correct guard against double-wrapping already-enveloped payloads. |
| backend/notification_v2/provider/webhook/webhook.py | Minor: adds return type annotations to send() and get_headers(); no logic changes. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Execution event] --> B{Delivery mode?}
B -->|IMMEDIATE backend| C[Backend SlackWebhook / APIWebhook]
B -->|IMMEDIATE worker callback| D[Worker SlackWebhook / APIWebhook]
B -->|BATCHED| E[NotificationBuffer row via enqueue endpoint]
C --> F[clubbed_renderer.py]
D --> G{Already rendered?}
G -->|No - flat dict| H[_clubbed_format.py]
G -->|Yes| I[Pass-through]
E --> J[Flush job render_clubbed_message]
F --> K[Canonical envelope]
H --> K
I --> K
J --> K
K -->|Slack| L[text mrkdwn]
K -->|API| M[summary plus events JSON]
Prompt To Fix All With AI
Fix the following 5 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 5
backend/notification_v2/clubbed_renderer.py:82
The `%-d` strftime directive (strip leading zero from day) is a GNU/Linux extension and is not available on macOS, BSD, or Windows — it raises `ValueError` there. Both this file and its worker mirror `_clubbed_format.py` share the same issue. On Linux containers this works, but CI runners on macOS or developer machines running tests locally will crash. A portable alternative is to format the day component separately.
```suggestion
return f"{dt.strftime('%Y %b')} {dt.day} {dt.strftime('%I:%M:%S %p')}"
```
### Issue 2 of 5
workers/notification/providers/_clubbed_format.py:41
Same `%-d` portability issue as in `backend/notification_v2/clubbed_renderer.py` — this is the worker-side mirror and has an identical strftime call that will raise on non-Linux platforms.
```suggestion
return f"{dt.strftime('%Y %b')} {dt.day} {dt.strftime('%I:%M:%S %p')}"
```
### Issue 3 of 5
workers/notification/providers/slack_webhook.py:59-60
**Fragile backend-payload pass-through detection**
The pass-through check `"text" in payload and len(payload) == 1` will silently re-render any backend-rendered Slack payload that ever gains a second key (e.g., `blocks`, `unfurl_links`, or `username`). If that happens, `build_envelope` will receive the already-rendered `{"text": "…"}` dict and `_event_from_payload` will produce a line with all fields set to `_MISSING`, discarding the original message. Keying on an explicit sentinel — for example checking for the presence of `"events"` in the raw payload before rendering — would be more robust.
### Issue 4 of 5
backend/notification_v2/provider/webhook/api_webhook.py:24-30
**Missing envelope guard on re-entrant calls**
`format_payload` always wraps `self.payload` unconditionally. The worker-side counterpart (`workers/notification/providers/api_webhook.py`) guards against double-wrapping with `"events" not in payload`, but the backend provider has no equivalent check. If `send()` is ever called on the same instance twice — which can happen if a caller catches an exception and retries the provider object — the second call passes an already-wrapped envelope into `build_envelope`, and `_event_from_payload` projects the whole envelope structure as a single event entry with all meaningful fields empty.
### Issue 5 of 5
backend/notification_v2/clubbed_renderer.py:91-95
`_format_file_count` shows the failure emoji and `failed_files` count for any non-success status, including `STOPPED`. For a stopped execution, showing `❌ 0/10 files` is misleading — no files actually failed; the run was just stopped. Consider treating `STOPPED` as a neutral case and returning an empty string, or using a distinct emoji.
```suggestion
status = (event.get("status") or "").upper()
if _is_success(event.get("status")):
successful = counts.get("successful_files", 0)
return f"{_EMOJI_SUCCESS} {successful}/{total} files"
if status == "STOPPED":
return f":octagonal_sign: {total} files (stopped)"
failed = counts.get("failed_files", 0)
return f"{_EMOJI_FAILURE} {failed}/{total} files"
```
Reviews (1): Last reviewed commit: "notification slack" | Re-trigger Greptile
| dt = datetime.datetime.fromisoformat(iso) | ||
| except (TypeError, ValueError): | ||
| return _MISSING | ||
| return dt.strftime("%Y %b %-d %I:%M:%S %p") |
There was a problem hiding this comment.
%-d strftime directive (strip leading zero from day) is a GNU/Linux extension and is not available on macOS, BSD, or Windows — it raises ValueError there. Both this file and its worker mirror _clubbed_format.py share the same issue. On Linux containers this works, but CI runners on macOS or developer machines running tests locally will crash. A portable alternative is to format the day component separately.
| return dt.strftime("%Y %b %-d %I:%M:%S %p") | |
| return f"{dt.strftime('%Y %b')} {dt.day} {dt.strftime('%I:%M:%S %p')}" |
Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/notification_v2/clubbed_renderer.py
Line: 82
Comment:
The `%-d` strftime directive (strip leading zero from day) is a GNU/Linux extension and is not available on macOS, BSD, or Windows — it raises `ValueError` there. Both this file and its worker mirror `_clubbed_format.py` share the same issue. On Linux containers this works, but CI runners on macOS or developer machines running tests locally will crash. A portable alternative is to format the day component separately.
```suggestion
return f"{dt.strftime('%Y %b')} {dt.day} {dt.strftime('%I:%M:%S %p')}"
```
How can I resolve this? If you propose a fix, please make it concise.
| dt = datetime.datetime.fromisoformat(iso) | ||
| except (TypeError, ValueError): | ||
| return _MISSING | ||
| return dt.strftime("%Y %b %-d %I:%M:%S %p") |
There was a problem hiding this comment.
%-d portability issue as in backend/notification_v2/clubbed_renderer.py — this is the worker-side mirror and has an identical strftime call that will raise on non-Linux platforms.
| return dt.strftime("%Y %b %-d %I:%M:%S %p") | |
| return f"{dt.strftime('%Y %b')} {dt.day} {dt.strftime('%I:%M:%S %p')}" |
Prompt To Fix With AI
This is a comment left during a code review.
Path: workers/notification/providers/_clubbed_format.py
Line: 41
Comment:
Same `%-d` portability issue as in `backend/notification_v2/clubbed_renderer.py` — this is the worker-side mirror and has an identical strftime call that will raise on non-Linux platforms.
```suggestion
return f"{dt.strftime('%Y %b')} {dt.day} {dt.strftime('%I:%M:%S %p')}"
```
How can I resolve this? If you propose a fix, please make it concise.| if "text" in payload and len(payload) == 1: | ||
| return {"text": payload["text"]} |
There was a problem hiding this comment.
The pass-through check "text" in payload and len(payload) == 1 will silently re-render any backend-rendered Slack payload that ever gains a second key (e.g., blocks, unfurl_links, or username). If that happens, build_envelope will receive the already-rendered {"text": "…"} dict and _event_from_payload will produce a line with all fields set to _MISSING, discarding the original message. Keying on an explicit sentinel — for example checking for the presence of "events" in the raw payload before rendering — would be more robust.
Prompt To Fix With AI
This is a comment left during a code review.
Path: workers/notification/providers/slack_webhook.py
Line: 59-60
Comment:
**Fragile backend-payload pass-through detection**
The pass-through check `"text" in payload and len(payload) == 1` will silently re-render any backend-rendered Slack payload that ever gains a second key (e.g., `blocks`, `unfurl_links`, or `username`). If that happens, `build_envelope` will receive the already-rendered `{"text": "…"}` dict and `_event_from_payload` will produce a line with all fields set to `_MISSING`, discarding the original message. Keying on an explicit sentinel — for example checking for the presence of `"events"` in the raw payload before rendering — would be more robust.
How can I resolve this? If you propose a fix, please make it concise.| def format_payload(self) -> dict[str, Any]: | ||
| """Wrap a single IMMEDIATE event in the canonical envelope. | ||
|
|
||
| `interval_seconds=None` -> `summary.interval_minutes` is null; | ||
| receivers can use that to distinguish IMMEDIATE from BATCHED. | ||
| """ | ||
| return build_envelope(payloads=[self.payload], interval_seconds=None) |
There was a problem hiding this comment.
format_payload always wraps self.payload unconditionally. The worker-side counterpart (workers/notification/providers/api_webhook.py) guards against double-wrapping with "events" not in payload, but the backend provider has no equivalent check. If send() is ever called on the same instance twice — which can happen if a caller catches an exception and retries the provider object — the second call passes an already-wrapped envelope into build_envelope, and _event_from_payload projects the whole envelope structure as a single event entry with all meaningful fields empty.
Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/notification_v2/provider/webhook/api_webhook.py
Line: 24-30
Comment:
**Missing envelope guard on re-entrant calls**
`format_payload` always wraps `self.payload` unconditionally. The worker-side counterpart (`workers/notification/providers/api_webhook.py`) guards against double-wrapping with `"events" not in payload`, but the backend provider has no equivalent check. If `send()` is ever called on the same instance twice — which can happen if a caller catches an exception and retries the provider object — the second call passes an already-wrapped envelope into `build_envelope`, and `_event_from_payload` projects the whole envelope structure as a single event entry with all meaningful fields empty.
How can I resolve this? If you propose a fix, please make it concise.| if _is_success(event.get("status")): | ||
| successful = counts.get("successful_files", 0) | ||
| return f"{_EMOJI_SUCCESS} {successful}/{total} files" | ||
| failed = counts.get("failed_files", 0) | ||
| return f"{_EMOJI_FAILURE} {failed}/{total} files" |
There was a problem hiding this comment.
_format_file_count shows the failure emoji and failed_files count for any non-success status, including STOPPED. For a stopped execution, showing ❌ 0/10 files is misleading — no files actually failed; the run was just stopped. Consider treating STOPPED as a neutral case and returning an empty string, or using a distinct emoji.
| if _is_success(event.get("status")): | |
| successful = counts.get("successful_files", 0) | |
| return f"{_EMOJI_SUCCESS} {successful}/{total} files" | |
| failed = counts.get("failed_files", 0) | |
| return f"{_EMOJI_FAILURE} {failed}/{total} files" | |
| status = (event.get("status") or "").upper() | |
| if _is_success(event.get("status")): | |
| successful = counts.get("successful_files", 0) | |
| return f"{_EMOJI_SUCCESS} {successful}/{total} files" | |
| if status == "STOPPED": | |
| return f":octagonal_sign: {total} files (stopped)" | |
| failed = counts.get("failed_files", 0) | |
| return f"{_EMOJI_FAILURE} {failed}/{total} files" |
Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/notification_v2/clubbed_renderer.py
Line: 91-95
Comment:
`_format_file_count` shows the failure emoji and `failed_files` count for any non-success status, including `STOPPED`. For a stopped execution, showing `❌ 0/10 files` is misleading — no files actually failed; the run was just stopped. Consider treating `STOPPED` as a neutral case and returning an empty string, or using a distinct emoji.
```suggestion
status = (event.get("status") or "").upper()
if _is_success(event.get("status")):
successful = counts.get("successful_files", 0)
return f"{_EMOJI_SUCCESS} {successful}/{total} files"
if status == "STOPPED":
return f":octagonal_sign: {total} files (stopped)"
failed = counts.get("failed_files", 0)
return f"{_EMOJI_FAILURE} {failed}/{total} files"
```
How can I resolve this? If you propose a fix, please make it concise.
What
Why
How
Can this PR break any existing features. If yes, please list possible items. If no, please explain why. (PS: Admins do not merge the PR without this section filled)
Database Migrations
Env Config
Relevant Docs
Related Issues or PRs
Dependencies Versions
Notes on Testing
Screenshots
Checklist
I have read and understood the Contribution Guidelines.