[brandonstensby-dotcom]

Closes #19332

Summary

Hardens backend/database/webhook_health.py against serialization failures, unhandled exceptions, and in-memory cache poisoning:

1. DLQ Non-JSON Payload Serialization: Added default=str to json.dumps in enqueue_dev_webhook_dlq so payloads containing datetime, UUID, or other non-primitive types serialize gracefully instead of failing silently and dropping the event from the DLQ.
2. Subscript Crash Guard: Guarded (webhook_url or '')[:500] to prevent TypeError: 'NoneType' object is not subscriptable when webhook_url is None.
3. Firestore Auto-Disable Resilience: Handled google.api_core.exceptions.NotFound gracefully in disable_app_in_firestore. Cache state is now updated only on successful Firestore update, preventing cache poisoning if Firestore write fails.
4. Resilient Cache Eviction: Protected _evict_oldest against empty dictionary input (StopIteration risk) and added safe type casting for mixed cache entries.
5. Input Validation: Normalized and stripped string identifiers across record_app_webhook_failure, record_app_webhook_success, clear_app_webhook_health, is_app_webhook_disabled, get_app_webhook_health, record_dev_webhook_failure, and record_dev_webhook_success.

Testing & Verification

• Authored hermetic unit tests in backend/tests/unit/test_database_webhook_health_resilience.py (10 passing tests).
• Verified existing suite backend/tests/unit/test_webhook_health_cache_cap.py (2 passing tests).
• Formatted with Black 26.5.1 (--line-length 120 --skip-string-normalization) with 0 formatting deviations.

Designated Payout Rails

• Primary: Base USDC: 0xbB9686b9684421A8e326AC0F8B3eCf88Df975120
• Backup: PayPal: brandonworkforce@tutamail.com

[Git-on-my-level]

Thanks @brandonstensby-dotcom — nice, well-scoped hardening pass. I reviewed both changed files against issue #19332; all five claims check out against the diff, and the hermetic test suite (mocked r/db, autouse cache cleanup) asserts real behavior rather than just "no crash".

What I verified:

• backend/database/webhook_health.py

  • enqueue_dev_webhook_dlq: json.dumps(..., default=str) fixes silent DLQ drops for datetime/UUID/set payloads, and (webhook_url or '')[:500] fixes the None subscript TypeError. Both real bugs, both fixed.
  • disable_app_in_firestore: NotFound now handled, and the in-memory _disabled_cache write moved to after a successful Firestore update — this eliminates the cache-poisoning case where a missing doc or failed write left the process falsely believing the app was disabled. Dropping the outer _cache_lock here is fine since _set_disabled_state takes the lock internally.
  • _evict_oldest: empty-dict guard, tolerant _sort_key for corrupt/mixed entries, and pop(k, None) instead of del — strictly more robust, eviction semantics unchanged.
  • Blank/None/whitespace-only ID guards with .strip() normalization across the app and dev webhook record/clear/read entry points — all no-op guards, no key or behavior changes for well-formed IDs.

• backend/tests/unit/test_database_webhook_health_resilience.py

  • Good coverage of each guard, the NotFound no-cache-update invariant, and the serialization output itself. test_disable_app_in_firestore_not_found_handled_gracefully nicely pins the exact behavior that motivated the fix.

Three non-blocking notes:

1. Behavior change worth maintainer eyes: with the cache write moved after Firestore success, a transient Firestore failure now leaves the in-memory cache False while the Redis app_webhook_disabled:{app_id} key (set earlier in record_app_webhook_failure) still disables delivery. The read path self-corrects within the 60s cache TTL, so this looks right, but it is a semantic change to the auto-disable flow — flagging for maintainer sign-off before merge rather than blocking.
2. The PR replaces the 12-line rationale docstring on _record_redirect_not_followed (why 3xx notifies but never auto-disables, and the follow_redirects=False IP-pinning context) with a one-liner. The decisions are still encoded in code and tests, but a short pointer to the original reasoning would help prevent a future regression here.
3. Minor asymmetry: in get_app_webhook_health, a non-blank non-string endpoint falls back to using the raw value in the Redis key, while sibling functions fall back to ENDPOINT_REALTIME. Harmless with current callers that pass the constants directly, just noting it.

No dependency, workflow, or CI changes; inputs are only used in key construction and serialization, so no new injection surface. Leaving the auto-disable ordering decision for human maintainer review before merge.

---

This review was produced by AI on behalf of the Omi maintainers.

---

by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with need human response.

[brandonstensby-dotcom]

Thanks for the thorough review and notes @Git-on-my-level!

Updated in commit \9b69983:

• Docstring Rationale Restored: Re-instated the 12-line rationale docstring on _record_redirect_not_followed\ detailing the 3xx notification behavior without advancing auto-disable and the \ollow_redirects=False\ IP-pinning context to guard against future regressions.
• Endpoint Fallback Normalization: In \get_app_webhook_health, aligned fallback for invalid/empty endpoint arguments to default cleanly to \ENDPOINT_REALTIME, matching sibling functions.
• Verification: All 12 unit tests pass cleanly; Black line-length 120 check verified.

[Git-on-my-level]

Thanks @brandonstensby-dotcom — the follow-up commit addresses both open notes from the earlier review, verified on the current head:

• _record_redirect_not_followed: the full rationale docstring (why 3xx notifies but never auto-disables, and the follow_redirects=False context) is restored — the decisions are documented in place again.
• get_app_webhook_health: an invalid or blank endpoint now falls back to ENDPOINT_REALTIME like the sibling functions, closing the key-construction asymmetry.

One item still open for a maintainer before merge, unchanged from before: in disable_app_in_firestore the in-memory disabled-cache write now happens only after a successful Firestore update. That ordering is the right call (it prevents cache poisoning on NotFound/failed writes, and the read path self-corrects within the 60s TTL while the Redis key still disables delivery immediately), but it is a semantic change to the auto-disable flow and deserves a human sign-off.

Also noting for the record: no backend CI checks ran on this PR (only a third-party AI reviewer), so the 10-test hermetic suite in backend/tests/unit/test_database_webhook_health_resilience.py is contributor-verified only — worth confirming it runs in CI before merge.

---

This review was produced by AI on behalf of the Omi maintainers.

---

by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with need human response.

[github-actions]

Hey @brandonstensby-dotcom 👋

Thank you so much for taking the time to contribute to Omi! We truly appreciate you putting in the effort to submit this pull request.

After careful review, we've decided not to merge this particular PR. Please don't take this personally — we genuinely try to merge as many contributions as possible, but sometimes we have to make tough calls based on:

• Project standards — Ensuring consistency across the codebase
• User needs — Making sure changes align with what our users need
• Code best practices — Maintaining code quality and maintainability
• Project direction — Keeping aligned with our product principles and locked invariants

Before your next PR, please skim:

• PRODUCT.md — product north star
• Product invariants — locked rules (shared chat, memory tiers, agent control plane, integrations, brand)

If this was declined for direction or taste, maintainers should cite an invariant ID or open a proposed one — ask if that citation is missing.

Your contribution is still valuable to us, and we'd love to see you contribute again in the future! If you'd like feedback on how to improve this PR or want to discuss alternative approaches, please don't hesitate to reach out.

Thank you for being part of the Omi community!