docs: document the Day Summary webhook and standardize created_at on UTC - #7110
Conversation
Greptile SummaryThis PR fixes the Confidence Score: 4/5Safe to merge; the one-line fix is correct and well-tested, with only minor test and doc polish remaining. All findings are P2 (style/best-practices): thinner test fixture schema than documented, MagicMock instead of AsyncMock for an async patch, and a missing UTC offset note in the docs example. No logic bugs or security issues introduced. backend/tests/unit/test_async_webhooks.py — AsyncMock vs MagicMock for the wiring test and _SAMPLE_SUMMARY completeness. Important Files Changed
Sequence DiagramsequenceDiagram
participant Cron as Hourly Cron Job
participant N as notifications.py<br/>_send_summary_notification
participant LLM as generate_comprehensive<br/>_daily_summary
participant DB as daily_summaries_db
participant Exec as storage_executor
participant W as webhooks.py<br/>day_summary_webhook
participant Recv as Your Webhook Endpoint
Cron->>N: _send_summary_notification(user_data)
N->>LLM: generate_comprehensive_daily_summary(...)
LLM-->>N: summary_data (dict)
N->>DB: create_daily_summary(uid, summary_data)
DB-->>N: summary_id
N->>Exec: submit(asyncio.run, day_summary_webhook(uid, summary_data))
Note over N,Exec: Previously str(summary_data) — now passes dict directly
Exec->>W: asyncio.run(coroutine)
W->>W: Check webhook enabled and circuit breaker
W->>Recv: POST /your-endpoint?uid=... with JSON summary object
Recv-->>W: 200 OK
Reviews (1): Last reviewed commit: "docs: list summary as a top-level field ..." | Re-trigger Greptile |
|
@beastoin @mdmohsin7 Could you review this pull request? The main question is whether the breaking change to the wire format is OK to land together with the docs. If you'd rather keep the docs and the fix separate, I'm happy to split this into:
That said, I'd personally lean toward landing them together. From a user's perspective, the current But either way is fine with me — just let me know which one you prefer and I'll do the work. Thanks! |
9b14340 to
0f91ffc
Compare
|
Friendly bump on this — I just had to merge No urgency on the breaking-change decision itself — happy to split into a docs-only PR + a separate JSON-object fix at a later breaking-change window if that's easier for you. I'd just like to avoid the patch surface drifting further before it lands. cc @beastoin @mdmohsin7 — would either of you have a few minutes to take a look? |
|
@syou6162 my insight: The date format should be updated to make it consistent. The You can also add a new field that returns the JSON object for the summary (which should be done in a breaking change PR), and then use that field from now on (update docs too ofc) Rebase your PR to ensure it is clean and does not include other changes. |
`day_summary_webhook` was the only place in `backend/` calling `datetime.now().isoformat()`, which produces a naive timestamp with no timezone suffix (`"2024-01-15T22:00:00.123456"`). Receivers had no way to know whether that was UTC, server-local, or something else. Switch to `datetime.now(timezone.utc).isoformat()`, matching the convention used everywhere else in the codebase (`utils/social.py`, `utils/chat.py`, `utils/agent.py`, `routers/users.py`, etc.). The webhook now sends `"2024-01-15T22:00:00.123456+00:00"` so receivers can parse it unambiguously with `datetime.fromisoformat`, `Date.parse`, etc. The `summary` field shape itself is intentionally left alone in this PR — see the accompanying Integrations docs change for the current wire format and the planned JSON-object follow-up.
The Day Summary webhook (`WebhookType.day_summary`) ships in the app
(Developer Mode → "Day Summary Webhook") and in the backend (cron at
`utils/other/notifications.py` calling `day_summary_webhook`), but the
public Integrations page never described it — developers seeing the
toggle in the app had no way to find the payload spec, the trigger
schedule, or the cases where no webhook is sent.
This adds a "Day Summary" H2 section covering:
- Endpoint shape (`POST /your-endpoint?uid=user123`) and a JSON payload
example reflecting what the backend actually sends today
- A `<Warning>` block that's blunt about the current wire format: the
`summary` field is a Python `repr` string (single-quoted, not JSON)
rather than a JSON object, so receivers cannot use `JSON.parse` and
currently need a Python-specific parser such as `ast.literal_eval`
to extract structured fields. A follow-up PR will add a dedicated
JSON-object field for the summary so receivers don't have to deal
with this; the warning sets that expectation
- A schema snapshot of the underlying summary `dict` (the same shape
the future JSON-object field will use) so receivers can plan against
it
- Delivery conditions where the webhook does *not* fire (no
conversations, all conversations locked / no transcribed speech, no
FCM token registered, atomic per-`(uid, date)` Redis lock already
held)
- Timezone requirement scoped to the scheduled cron path (the manual
"Generate Summary" trigger falls back to UTC day boundaries)
- A `<Note>` explaining that the in-app "Generate Summary" action
regenerates the summary but does not currently POST to the developer
webhook, so the recommended way to validate a receiver is still to
wait for the next scheduled cron tick
Page-level wiring is brought in line with the new section:
- The intro CardGroup is recomposed to `cols={2}` 2×2 so the four
trigger types fit cleanly, and a Day Summary card is added
- The mermaid diagram gets a Day Summary source node and edge
- "Choose Your Trigger Type" and "Set Webhook URL" steps now cover
all four trigger types (also fills the previous Audio Bytes gap)
- The page frontmatter description now mentions daily summaries
No backend behaviour is changed by this commit — `summary` remains a
Python `repr` string on the wire. Aligning the payload to a real JSON
object is deferred to a follow-up breaking-change PR per maintainer
direction.
0f91ffc to
1a363da
Compare
|
Thanks for the review @beastoin — fully agree with the direction. I've reworked the PR along the lines you suggested: What changed
Planned follow-up (separate PR) Per your suggestion, I'll open a separate breaking-change PR that adds a new field returning the summary as a real JSON object (likely Could you take another look when you have a moment? Thanks again for the steer. |
kodjima33
left a comment
There was a problem hiding this comment.
backend+docs (Day Summary webhook docs) — approve only
Follow-up to #7110, which documented the Day Summary webhook but left the `summary` wire format intact (a Python `repr` string that JSON parsers can't read). Per @beastoin's review there, this introduces a new JSON-object field — `summary_json` — alongside the legacy `summary` string so receivers can migrate to a properly-parseable payload without breaking anyone still consuming the old format. Changes: - `day_summary_webhook` gains an optional `summary_json: Optional[dict]` parameter and includes it in every outgoing payload. When the caller doesn't supply one, the field is sent as `null`, which keeps the JSON shape stable for receivers regardless of source. - The only in-tree caller (`_send_summary_notification` in `utils/other/notifications.py`) now passes the raw `summary_data` dict as `summary_json`, while continuing to wrap it in `str(...)` for the legacy `summary` field. Both fields therefore carry the same payload — one as a JSON object, one as the historical Python repr. - The legacy `summary` string is intentionally preserved; deprecation will be handled in a separate PR once receivers have had time to migrate.
Adds two test classes for the new `summary_json` field introduced in the previous commit (follow-up to #7110): - `TestDaySummaryWebhookJsonField` runs `day_summary_webhook` end to end with `get_webhook_client` mocked and asserts on the JSON body going to httpx: * when a dict is supplied, `summary_json` is sent as a dict, the legacy `summary` string is preserved verbatim, and `created_at` keeps the `+00:00` UTC offset shipped in #7110 * when no dict is supplied, `summary_json` is sent as `null` so receivers see a stable JSON shape regardless of source - `TestSendSummaryNotificationWiresSummaryJson` is a static guard matching the in-tree pattern (grep the source instead of importing the heavy `notifications.py`) that pins the call site to `day_summary_webhook(uid, str(summary_data), summary_data)` so a regression there would surface immediately.
Follow-up to #7110, which landed a `<Warning>` block calling out that the Day Summary webhook's `summary` field is a Python `repr` string rather than JSON. With the new `summary_json` payload field shipped in this PR, the docs can now point receivers at a properly-parseable alternative instead of just describing the pitfall. Changes in `Integrations.mdx`: - The Webhook Payload example now shows both fields side by side, with `summary_json` as a real nested JSON object and `summary` truncated to the legacy `repr` string. This makes the wire shape obvious at a glance. - The field reference table flags `summary_json` as **Recommended** and marks `summary` as **Legacy**, replacing the previous single row that pointed at the warning. - The `<Warning>` becomes a `<Note>` that explicitly recommends migrating to `summary_json`, explains why the legacy `summary` field still exists (backward compatibility), and signals that deprecation will follow once receivers have had time to migrate. Receivers no longer need `ast.literal_eval` for the new path. - The clarification about the two `created_at` timestamps is updated to reference `summary_json` as the canonical inner-timestamp source, with the legacy `summary` string noted as carrying the same value.
Summary
Documents the previously-undocumented Day Summary webhook on the public Integrations page, and aligns the top-level
created_atit sends with the rest of the backend (UTC ISO 8601 with+00:00offset).This branch was originally larger and proposed flipping the
summaryfield from a Pythonreprstring to a real JSON object as well. Per @beastoin's review feedback we've split that work off — this PR is now intentionally docs-leaning, ships the smallcreated_atconsistency fix, and leaves the wire-format question for a separate follow-up.What's in this PR
backend/utils/webhooks.py—day_summary_webhookcreated_atis nowdatetime.now(timezone.utc).isoformat(). That was the only place inbackend/still calling naivedatetime.now()for a wire-format timestamp; every other timestamp the backend emits (utils/social.py,utils/chat.py,routers/users.py, …) already usesdatetime.now(timezone.utc). The payload now sends"2024-01-15T22:00:00.123456+00:00"instead of the naive"2024-01-15T22:00:00.123456"so receivers can parse it unambiguously.docs/doc/developer/apps/Integrations.mdx— new "Day Summary" section covering:POST /your-endpoint?uid=...) and a JSON example reflecting the current payload<Warning>block that's blunt about the current wire format:summaryis a Pythonreprstring (single-quoted, not JSON) rather than a JSON object, so receivers cannot useJSON.parseand currently need a Python-specific parser such asast.literal_evalto extract structured fields. A follow-up PR will add a dedicated JSON-object field for the summarydict(the shape the future JSON-object field will use) so receivers can plan against it(uid, date)Redis lock already held)<Note>clarifying that the in-app "Generate Summary" action regenerates the summary but does not currently POST to the developer webhook, so the recommended validation path is still to wait for the next scheduled cron tickCardGroupis recomposed tocols={2}2×2 so all four trigger types fit, a Day Summary card is added, the mermaid diagram gets a Day Summary node, "Choose Your Trigger Type" and "Set Webhook URL" steps now cover all four trigger types (also fills the previous Audio Bytes gap), and the frontmatter description mentions daily summaries.What's deliberately not in this PR
summaryfield on the wire. It is still a Pythonreprstring; the docs describe that as the current state and warn receivers accordingly.summary_json(or similarly named) field. That belongs to the follow-up breaking-change PR per @beastoin's note ("you can also add a new field that returns the JSON object for the summary — that should be done in a breaking change PR"). Tracked separately so this docs-leaning PR can land without coupling to that decision.Why the
created_atchange is in scope@beastoin's review asked that "the date format should be updated to make it consistent". The only date format this PR touches is the top-level webhook
created_at— moving it from naive local-server time to a+00:00UTC ISO 8601 string. Pinging if a different field was meant: happy to revert this hunk and ship a docs-only PR instead.Migration impact
None for the
summaryfield — its shape and serialization are unchanged.The top-level
created_atswitches from a naive ISO 8601 string ("2024-01-15T22:00:00.123456") to an offset-suffixed one ("2024-01-15T22:00:00.123456+00:00"). Standard parsers (Date.parse,datetime.fromisoformat, Dart'sDateTime.parse, …) accept both forms, and the new form removes the ambiguity around which timezone the naive string was in (it was always UTC in practice — only the explicit suffix changes).Test plan
black --check --line-length 120 --skip-string-normalization backend/utils/webhooks.py— clean