[brandonstensby-dotcom]

Summary

Hardens backend/database/feedback.py and backend/routers/mobile_feedback.py with rigorous input validation, whitespace normalization, and boundary guards:

1. Deterministic Coordinate Normalization:
   • Strips uid and target_id before database write operations.
   • Refuses empty, non-string, or blank whitespace inputs.
   • record_feedback_event: logs a warning and returns None by default (best-effort, preserving caller flow), and raises descriptive ValueError when raise_on_error=True.
2. Hash Normalization Parity:
   • _idempotent_event_id and record_feedback_event_idempotent normalize and strip uid and feedback_id prior to computing the SHA-256 digest, ensuring identical idempotent keys regardless of client whitespace variance.
3. Safe Fetch & Parsing Boundary Guards:
   • get_feedback_event: safely validates event_id string and returns None immediately if blank/invalid without firing an invalid document path query.
   • get_report: validates date string and returns None immediately if blank/invalid.
   • save_report: validates that report is a valid FeedbackReport instance with a non-empty date string.
   • list_negative_events: rejects non-datetime arguments or inverted datetime ranges (start_at > end_at), and returns an empty list for limit <= 0.
   • list_report_dates: returns empty list for limit <= 0 and safely handles client transport exceptions.
4. Router Error Mapping:
   • In backend/routers/mobile_feedback.py, catches database-level ValueError and converts to HTTPException(status_code=400, detail=str(exc)).

Invariants & Failure-Class

• Product invariants affected: none
• Failure-Class: none

How it was verified

Hermetic unit tests executed via pytest:

$env:PYTHONPATH="backend"
python -m pytest tests/unit/test_database_feedback_resilience.py tests/unit/test_feedback_report.py

Results:

• tests/unit/test_database_feedback_resilience.py: 40/40 passed (0.08s)
• tests/unit/test_feedback_report.py: 20/20 passed (0.16s)
• Total: 60 passed in 2.29s (100% passing, hermetic mocks, 0 external network dependencies).

[brandonstensby-dotcom]

@Git-on-my-level Small resilience hardening for database feedback (#19891): normalizes coordinates, enforces hash parity on idempotent retries, and adds safe fetch/parse guards with 60 hermetic tests. Could you take a look when you have a moment?

[Git-on-my-level]

Thanks @brandonstensby-dotcom — this is a solid, well-scoped hardening pass, and the hermetic test coverage is appreciated.

Reviewed each changed file:

• backend/database/feedback.py — Moving _idempotent_event_id above record_feedback_event and stripping uid/feedback_id before the SHA-256 keeps idempotency keys stable under client whitespace variance. I confirmed the digest change only affects whitespace-padded uids, which the auth-derived uids on this path are not, so there is no stored-key migration concern. The new uid/target_id refusal guards in record_feedback_event preserve the best-effort contract (warn + return None, or ValueError with raise_on_error=True), and the immutable-field comparison in record_feedback_event_idempotent now uses the normalized values consistently. The blank-input guards in get_feedback_event/get_report correctly avoid issuing Firestore document-path lookups with empty strings.
• backend/routers/mobile_feedback.py — The new except ValueError -> 400 is correctly ordered: FeedbackIdempotencyConflict (a ValueError subclass) is still caught first, so the 409 idempotency-conflict semantics are preserved, and FeedbackPersistenceError (RuntimeError) still maps to 503. One small note: this handler also converts any unexpected internal ValueError into a 400 client error rather than a 500; today every ValueError raised on this path is an input-validation message, so that is fine, but worth keeping in mind if non-validation ValueErrors are added later.
• backend/tests/unit/test_database_feedback_resilience.py — Good hermetic design (get_firestore_client patched everywhere, no network). I ran this suite against your head locally: 40/40 pass.

Two minor observations, neither blocking:

1. In save_report, the document id uses str(report.date).strip() while the persisted payload keeps the unstripped date, so a whitespace-padded date would produce an id/payload mismatch. It is unreachable today (dates come from date.isoformat() in the daily job), but stripping the value once up front would make the two consistent.
2. list_negative_events raising ValueError (rather than TypeError) for non-datetime arguments is slightly unconventional, but with the daily report job being the only caller (it passes real datetimes) it is harmless and matches the router mapping.

Since this touches validation and error-mapping on a user-data persistence path, leaving the final merge call for a human maintainer — the change itself looks correct and defensive-only.

— automated maintainer feedback, AI-generated on David's behalf

---

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 @Git-on-my-level! Addressed the observation in commit 04e47f7: in save_report, report.date is now stripped up front and assigned to payload['date'], ensuring the Firestore document ID and persisted payload date are 100% consistent under any whitespace variance, verified by test_save_report_strips_date_consistently_in_doc_and_payload. All 41 unit tests passing cleanly.

[Git-on-my-level]

Thanks @brandonstensby-dotcom — this is a genuinely useful hardening pass on the feedback persistence path. Reviewed all three changed files:

backend/database/feedback.py

• Moving _idempotent_event_id up and hashing the stripped uid/feedback_id is the right call — previously a whitespace-padded uid produced a different idempotency key than the coordinates stored in the immutable-record comparison, so the key derivation and the 409 conflict check could disagree. For any client that actually sends padded coordinates this changes the derived event id once, but uid is auth-derived and feedback_id comes from the client SDK, so in practice both are clean strings and the parity fix is the correct trade.
• The new uid/target_id refusals in record_feedback_event correctly honor the existing raise_on_error contract (warn + None on the best-effort path, ValueError on the idempotent path).
• list_negative_events raising on non-datetime or inverted ranges is safe for the only production caller (backend/jobs/feedback_daily_report.py passes computed datetimes), and the limit <= 0 early return doesn't touch the registered query path, so query coverage is unaffected.
• save_report keeping the document id and the persisted payload['date'] consistent after stripping is a good catch — they could previously diverge.

backend/routers/mobile_feedback.py

• The new except ValueError → 400 is correctly ordered: FeedbackIdempotencyConflict is a ValueError subclass and is still caught first, so the 409/503 semantics are preserved.

backend/tests/unit/test_database_feedback_resilience.py

• Hermetic (all Firestore access mocked via patch.object(feedback_db, "get_firestore_client", ...)), and it covers both the default best-effort refusals and the raise_on_error variants, the whitespace-normalization parity of the idempotency key, and the report date/document-id consistency. This is the level of test hygiene we like to see.

One non-blocking observation: str(uid or '') means a truthy non-string uid (e.g. an int) is silently coerced to its string form rather than refused. Since uids are auth-derived strings this is theoretical, but an isinstance(uid, str) check would make the refusal boundary exact if you want the strictness.

No CI results are visible on this head yet. Since this touches the feedback data-write path, leaving for human maintainer review before merge.

---

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

[aryanorastar]

On 04e47f7c2cf41dfcea8cb0b9922183a77b9b1c77, backend/routers/mobile_feedback.py still returns detail=str(exc) from the new ValueError handler.

[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!