feat: notification ack fields (cherry-pick, isolated from unrelated stacked commits) - #10
Conversation
… consumers Cherry-pick of 1bdc0d8 (the real notification-ack commit), rebased onto current main's alembic head (d8e9f0a1b2c3 instead of the stale l2g3h4i5j6k7 parent, avoiding a second alembic head). Also fixes two downstream consumers broken by webhook_notifier.send() now returning NotificationSendResult instead of bool: - worker/tasks.py: success was a non-JSON-serializable dataclass instead of bool in the send_notification celery task's return value. - workflow/webhook_delivery.py: `if not delivered` never fired since a dataclass instance is always truthy, silently swallowing genuine webhook delivery failures. Both now reuse the existing _normalize_send_result() helper instead of new logic. Also fixes a stale assertion in test_notifiers.py (result is True -> result.success is True) and drops the stray backend/test_notification_ack.py scratch file with no references elsewhere.
|
✅ Health: 9.1 📋 At a glance Files & modules (2)
🚨 Change risk: 8.5/10 (high)
🔎 More signals (3)🔥 Hotspots touched (3)
🔗 Hidden coupling (1 file)
💀 Dead code (2 findings)
👀 Suggested reviewers @xujinghua 📊 Full report · ⭐ Star Repowise · 📥 Install bot · Last updated 2026-07-12 10:30 UTC |
📝 WalkthroughSummary by CodeRabbit
WalkthroughNotification delivery now creates identifiable logs, returns structured webhook results, persists response data, and tracks acknowledgement state. A new HMAC-authenticated endpoint records acknowledgements and updates linked collected records. ChangesNotification acknowledgement flow
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Dispatch
participant Webhook
participant NotificationLog
participant AckAPI
participant CollectedRecord
Dispatch->>NotificationLog: create pending log and obtain delivery_id
Dispatch->>Webhook: send payload with delivery_id
Webhook-->>Dispatch: return success and response data
Dispatch->>NotificationLog: persist delivery and ack state
Webhook->>AckAPI: POST signed acknowledgement
AckAPI->>NotificationLog: update ack_status, ack_data, and acked_at
AckAPI->>CollectedRecord: update linked record status
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a notification acknowledgement (ACK) mechanism. It adds ACK fields to the notification logs, updates the notifier interface to return a detailed NotificationSendResult (including response data), and implements a new /notifications/logs/{log_id}/ack endpoint to handle signed ACK webhooks. The review feedback highlights three key areas for improvement: resolving a type mismatch in WebhookNotifier.send where an exception handler still returns a boolean, optimizing database queries in the ACK endpoint by eagerly loading the notification rule using joinedload, and addressing a transactional consistency issue where notification logs are flushed but not committed immediately, potentially leading to missing logs if the pipeline fails after a webhook is dispatched.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| async def send( | ||
| self, config: dict[str, Any], payload: NotificationPayload | ||
| ) -> NotificationSendResult: |
There was a problem hiding this comment.
The send method is annotated to return NotificationSendResult, but the error handling block for SSRFValidationError (on line 33, which is outside this diff) still returns False:
except SSRFValidationError:
return FalseThis violates the new type signature and can cause issues if downstream consumers expect a NotificationSendResult object. Please update line 33 to return a NotificationSendResult instead:
except SSRFValidationError:
return NotificationSendResult(
success=False,
response_data={"error": "SSRF validation failed"},
)| result = await db.execute(select(NotificationLog).where(NotificationLog.id == log_id)) | ||
| log = result.scalar_one_or_none() | ||
| if not log: | ||
| raise HTTPException(status_code=404, detail="Notification log not found") | ||
|
|
||
| rule = await db.get(NotificationRule, log.rule_id) | ||
| if not rule: | ||
| raise HTTPException(status_code=404, detail="Notification rule not found") |
There was a problem hiding this comment.
We can optimize this by eagerly loading the associated NotificationRule using joinedload. This reduces the number of database queries from 2 to 1. Additionally, since rule_id is non-nullable and has a foreign key constraint, the rule is guaranteed to exist if the log exists, allowing us to safely remove the redundant second 404 check.
from sqlalchemy.orm import joinedload
result = await db.execute(
select(NotificationLog)
.options(joinedload(NotificationLog.rule))
.where(NotificationLog.id == log_id)
)
log = result.scalar_one_or_none()
if not log:
raise HTTPException(status_code=404, detail="Notification log not found")
rule = log.rule| log.status = status | ||
| log.response_data = response_data | ||
| log.error_message = error_msg | ||
| if status == "sent" and _ack_secret(rule.notifier_config): | ||
| log.ack_status = "pending" |
There was a problem hiding this comment.
Since NotificationLog entries are flushed but not committed within this loop, any unhandled exception later in the pipeline or during the transaction commit will roll back these log entries. However, the external notifications (e.g., webhooks) have already been sent and cannot be rolled back.
This leads to a state mismatch where notifications are delivered but no logs exist in the database, which can also cause duplicate notifications if the pipeline is retried.
Consider committing the session or using a separate transaction/session for creating and updating NotificationLog entries immediately after each dispatch to ensure they are persisted regardless of the overall pipeline transaction outcome.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/notifiers/webhook_notifier.py (1)
33-33: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
return Falseviolates the declaredNotificationSendResultreturn type.The method signature on line 20 declares
-> NotificationSendResult, but the SSRF validation failure path on line 33 still returns a barebool. This is a leftover from before the return type change. While_normalize_send_resulthandles both types, any direct consumer ofWebhookNotifier.sendexpectingNotificationSendResultwill receive aboolon this path.🐛 Proposed fix
client, url = await guarded_async_client(url, timeout=timeout) except SSRFValidationError: - return False + return NotificationSendResult(success=False)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/notifiers/webhook_notifier.py` at line 33, Update the SSRF validation failure path in WebhookNotifier.send to return the appropriate NotificationSendResult value instead of the bare False boolean. Keep the existing failure semantics while ensuring every return path matches the declared return type for direct callers.
🧹 Nitpick comments (2)
backend/pipeline/notifier_dispatch.py (1)
12-13: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate
_ack_secretlogic with a different signature.
backend/api/v1/notifications.pydefines its own_ack_secret(rule: NotificationRule)that re-implements the samenotifier_config.get("ack_secret")lookup this function performs on the raw dict. Two independent implementations of the same lookup risk drifting if the config key ever changes. Consider havingnotifications.pycall this one withrule.notifier_config, or extracting a single shared helper.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/pipeline/notifier_dispatch.py` around lines 12 - 13, Consolidate the duplicate acknowledgement-secret lookup by updating notifications.py’s _ack_secret(NotificationRule) to reuse backend/pipeline/notifier_dispatch.py’s _ack_secret with rule.notifier_config, or move the lookup into one shared helper used by both paths. Preserve the existing empty-string fallback and avoid maintaining separate config-key logic.backend/api/v1/notifications.py (1)
29-30: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate
_ack_secretimplementation.This re-implements the same
notifier_config.get("ack_secret")lookup already defined inbackend/pipeline/notifier_dispatch.py::_ack_secret, just with a different (rule-object) signature. Consolidate into one helper to avoid the two drifting.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/api/v1/notifications.py` around lines 29 - 30, Remove the duplicate _ack_secret helper in the notifications flow and reuse the existing backend/pipeline/notifier_dispatch.py::_ack_secret implementation. Adapt the call site or shared helper interface as needed so it still resolves ack_secret from the notification rule’s notifier_config without maintaining two lookup implementations.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/api/v1/notifications.py`:
- Around line 143-155: Add a guard in ack_log before assigning log.ack_status,
log.ack_data, or log.acked_at and before updating the linked CollectedRecord:
continue only when log.ack_status is "pending". Reject or ignore
acknowledgements for any other state, preserving the existing mutation behavior
for eligible pending logs.
- Around line 29-30: Redact the shared ack_secret from
NotificationRuleRead.notifier_config in every /rules list, get, create, and
update response. Update the response serialization path using
NotificationRuleRead (and reuse _ack_secret where needed) so the secret is
removed or masked without altering stored rule configuration or unrelated
notifier fields.
In `@backend/models/notification.py`:
- Around line 42-43: Update the status comment adjacent to the status field in
the notification model to include the pending state alongside sent and failed,
matching the values assigned by notifier_dispatch.py.
In `@backend/pipeline/notifier_dispatch.py`:
- Around line 16-19: Move _normalize_send_result alongside
NotificationSendResult in backend/notifiers/base.py, rename it to
normalize_send_result, and expose it as the shared public utility. Update
notifier_dispatch.py, backend/worker/tasks.py, and
backend/workflow/webhook_delivery.py to import and call the renamed helper while
preserving its current return behavior.
---
Outside diff comments:
In `@backend/notifiers/webhook_notifier.py`:
- Line 33: Update the SSRF validation failure path in WebhookNotifier.send to
return the appropriate NotificationSendResult value instead of the bare False
boolean. Keep the existing failure semantics while ensuring every return path
matches the declared return type for direct callers.
---
Nitpick comments:
In `@backend/api/v1/notifications.py`:
- Around line 29-30: Remove the duplicate _ack_secret helper in the
notifications flow and reuse the existing
backend/pipeline/notifier_dispatch.py::_ack_secret implementation. Adapt the
call site or shared helper interface as needed so it still resolves ack_secret
from the notification rule’s notifier_config without maintaining two lookup
implementations.
In `@backend/pipeline/notifier_dispatch.py`:
- Around line 12-13: Consolidate the duplicate acknowledgement-secret lookup by
updating notifications.py’s _ack_secret(NotificationRule) to reuse
backend/pipeline/notifier_dispatch.py’s _ack_secret with rule.notifier_config,
or move the lookup into one shared helper used by both paths. Preserve the
existing empty-string fallback and avoid maintaining separate config-key logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b6ba0930-3706-4299-b9c4-668ad9ec5297
📒 Files selected for processing (10)
backend/api/v1/notifications.pybackend/migrations/versions/m2n3o4p5q6r7_add_notification_ack_fields.pybackend/models/notification.pybackend/notifiers/base.pybackend/notifiers/webhook_notifier.pybackend/pipeline/notifier_dispatch.pybackend/schemas/notification.pybackend/worker/tasks.pybackend/workflow/webhook_delivery.pytests/unit/test_notifiers.py
| def _ack_secret(rule: NotificationRule) -> str: | ||
| return str(rule.notifier_config.get("ack_secret") or "") |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n "notifier_config" backend/api/v1/notifications.py backend/schemas/notification.pyRepository: 2233admin/opencli-admin
Length of output: 514
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' backend/api/v1/notifications.py
printf '\n---\n'
sed -n '1,220p' backend/schemas/notification.py
printf '\n---\n'
rg -n "notifier_config|ack_secret|NotificationRule" backend/api/v1 backend/schemas backend -g '!**/__pycache__/**'Repository: 2233admin/opencli-admin
Length of output: 17906
Redact ack_secret from rule responses. The /rules list/get/create/update endpoints serialize NotificationRuleRead.notifier_config directly, so the shared HMAC secret is exposed to any client that can read a notification rule.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/api/v1/notifications.py` around lines 29 - 30, Redact the shared
ack_secret from NotificationRuleRead.notifier_config in every /rules list, get,
create, and update response. Update the response serialization path using
NotificationRuleRead (and reuse _ack_secret where needed) so the secret is
removed or masked without altering stored rule configuration or unrelated
notifier fields.
| log.ack_status = body.status | ||
| log.ack_data = body.ack_data | ||
| log.acked_at = datetime.now(UTC) | ||
|
|
||
| if log.record_id: | ||
| record = await db.get(CollectedRecord, log.record_id) | ||
| if record: | ||
| if body.status == "acked": | ||
| record.status = "notified" | ||
| record.error_message = None | ||
| else: | ||
| record.status = "error" | ||
| record.error_message = str(body.ack_data.get("error") or "Downstream ACK failed") |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
ack_log accepts acknowledgements for logs that were never eligible.
ack_log updates log.ack_status/acked_at and flips the linked CollectedRecord.status purely based on a valid HMAC signature, without checking that log.ack_status == "pending" first. Per notifier_dispatch.py, a log only becomes "pending" when the send actually succeeded (status == "sent") and an ack_secret is configured; logs whose send failed stay "not_required". Since the endpoint doesn't gate on this, a caller who knows the rule's shared ack_secret can "ack" a log_id whose delivery actually failed, incorrectly flipping the associated CollectedRecord.status to "notified"/"error".
Add a state check before mutating the log/record, e.g.:
Proposed fix
secret = _ack_secret(rule)
if not secret:
raise HTTPException(
status_code=400, detail="Notification rule has no ack_secret configured"
)
+ if log.ack_status != "pending":
+ raise HTTPException(
+ status_code=409, detail=f"Notification log is not awaiting acknowledgement (ack_status={log.ack_status!r})"
+ )
+
signature = request.headers.get("X-Signature-256", "")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/api/v1/notifications.py` around lines 143 - 155, Add a guard in
ack_log before assigning log.ack_status, log.ack_data, or log.acked_at and
before updating the linked CollectedRecord: continue only when log.ack_status is
"pending". Reject or ignore acknowledgements for any other state, preserving the
existing mutation behavior for eligible pending logs.
| # sent | failed | ||
| status: Mapped[str] = mapped_column(String(50), nullable=False) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Stale comment on status.
The # sent | failed comment no longer reflects reality — notifier_dispatch.py now also sets status="pending" before the send attempt completes.
Proposed fix
- # sent | failed
+ # pending | sent | failed
status: Mapped[str] = mapped_column(String(50), nullable=False)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # sent | failed | |
| status: Mapped[str] = mapped_column(String(50), nullable=False) | |
| # pending | sent | failed | |
| status: Mapped[str] = mapped_column(String(50), nullable=False) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/models/notification.py` around lines 42 - 43, Update the status
comment adjacent to the status field in the notification model to include the
pending state alongside sent and failed, matching the values assigned by
notifier_dispatch.py.
| def _normalize_send_result(result: bool | NotificationSendResult) -> tuple[bool, dict | None]: | ||
| if isinstance(result, NotificationSendResult): | ||
| return result.success, result.response_data | ||
| return bool(result), None |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Promote _normalize_send_result to a shared, public utility.
This helper is private (leading underscore) yet is now imported across module boundaries by backend/worker/tasks.py and backend/workflow/webhook_delivery.py. Importing a module-private symbol from other modules blurs the API boundary and makes future refactors of notifier_dispatch.py risky. Consider moving it (and dropping the underscore) to backend/notifiers/base.py, next to NotificationSendResult, and updating the three call sites.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/pipeline/notifier_dispatch.py` around lines 16 - 19, Move
_normalize_send_result alongside NotificationSendResult in
backend/notifiers/base.py, rename it to normalize_send_result, and expose it as
the shared public utility. Update notifier_dispatch.py, backend/worker/tasks.py,
and backend/workflow/webhook_delivery.py to import and call the renamed helper
while preserving its current return behavior.
1 participant
Summary
1bdc0d8— the single real notification-ack commit — isolated from thecodex/notification-ackupstream branch, which had 4 unrelated large feature commits (workspace RBAC/auth, IDE convergence, system pulse, visualization layer) stacked on top. None of those are included here.down_revisionrepointed from the stalel2g3h4i5j6k7to main's actual current headd8e9f0a1b2c3, avoiding a second alembic head.NotificationSendResultinterface change (webhook notifier now returns a dataclass, not bool), both via the existing_normalize_send_result()helper:backend/worker/tasks.py:send_notificationcelery task was returning a non-JSON-serializable object instead of bool.backend/workflow/webhook_delivery.py:if not deliverednever fired against a dataclass instance (always truthy), silently swallowing real webhook delivery failures.tests/unit/test_notifiers.py(result is True->result.success is True).backend/test_notification_ack.py(zero references elsewhere).Test plan
uv run pytest -m "not live"— 1629 passed, 0 failed, 90.08% coverageuv run alembic heads— single head confirmed