feat: add chat summary tab in the right sidebar and per-chat cost endpoint - #26649
Conversation
Docs previewCheck off each page once it's been reviewed. If a page changes in a later push, its checkbox clears automatically so it gets a fresh look. Pages not yet wired into the docs navigation aren't listed here. |
Add a persisted chats.summary populated by a background generator after successful root-chat turns, delivered live via a new chat_summary_change watch event, with per-feature cost attribution and a deployment-wide summary-generation model override.
The UnknownContextReturns400 subtest hardcodes the valid override context list in its expected error. Adding the summary_generation context changed the message, so update both assertions.
28cda13 to
aa80827
Compare
Background summary and manual title generation write hidden, soft-deleted chat_messages rows tagged with cost_source to attribute their spend. The AFTER STATEMENT history triggers advanced chats.history_version for those rows too, so the history_version optimistic guard on UpdateChatSummary was invalidated by the very usage row recorded for that same summary. The summary write (and the last_turn_summary write racing behind it) was then rejected as stale even when no new turn had occurred, so chats.summary was never persisted. Add migration 000531 recreating the history triggers so only rows with cost_source IS NULL (ordinary turn history) advance history_version. snapshot_version still advances for every change, so history_version correctly trails after an accounting-only write and catches up on the next real message.
Add a reusable ChatSummary component surfaced via an info button placed to
the left of the chat sharing button in the chat header. The button opens a
"Summary" popover (400px on desktop, full width on mobile) showing the
chat's summary plus its Created/Updated dates and cumulative cost.
The summary text is the persisted whole-chat summary (chat.summary) from the
backend summary feature this branch is stacked on. It is generated
asynchronously and may be null until the first summary is produced, in which
case the popover renders a muted empty state.
Cost is served by a new per-chat endpoint, GET
/api/experimental/chats/{chat}/cost, that rolls up assistant-message cost
across a chat's root and child (subagent) chats and is authorized like the
other {chat} routes.
aa80827 to
ca032fd
Compare
|
/coder-agents-review |
|
Chat: Review posted | View chat Review historydeep-review v0.9.0 | Round 3 | Last posted: Round 3, 15 findings (6 P3, 2 P4, 7 Nit), APPROVE. Review Finding inventoryFindings
Contested and acknowledged(none) Round logRound 1Panel. Netero + 21 reviewers. 5 P3, 2 P4, 6 Nit, 0 dropped. Reviewed against 9ad21cb..b4e09f9. Round 2Panel. Netero + 9 reviewers. All 13 R1 findings addressed. 1 P3, 1 Nit new. Reviewed against 7a57ac0..bcbd65e. Round 3Panel. Netero + 3 reviewers. All 15 findings addressed. 0 new. Reviewed against 7a57ac0..d786b24. About deep-reviewCRF = Coder Review Finding (P0-P4, Nit, Note)
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b4e09f9cf0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Clean vertical slice: SQL CTE with root rollup, dbauthz wrapper, handler with swagger, Go SDK, TS API + query factory, presentational component, data-fetching wrapper, ChatTopBar integration, and 9 story variants. Test density at 51.2% with genuine assertions throughout. The decomposition into a pure ChatSummary and a data-fetching ChatSummaryPopoverContent is well done.
Severity breakdown: 5 P3, 2 P4, 6 Nit.
The strongest convergent finding (5 reviewers independently): the backend deliberately tracks priced vs unpriced messages to let callers flag partial totals, but the frontend drops both counts and passes only total_cost_micros. A chat with no pricing configured shows "$0.00" instead of indicating cost tracking is unavailable. The data to fix this is already in the API response.
As Pariston put it: "The backend author understood the problem deeply enough to track partial pricing. The frontend didn't carry that understanding into the display."
The other convergent finding (4 reviewers + 4 notes): the getChatCost handler has a dead IsNotAuthorizedError -> Forbidden branch. ExtractChatParam middleware already handles auth with 404; the handler's 403 would leak resource existence if the middleware were removed. The sibling getChatMessages behind the same middleware does not have this check, confirming it was incorrectly copied from getChatCostSummary (which legitimately needs it because it runs outside ExtractChatParam).
🤖 This review was automatically generated with Coder Agents.
# Conflicts: # docs/admin/security/audit-logs.md
Resolve the actionable findings from the deep review of #26657: - P0/CRF-2: renumber the chat summary migration from 000530 to 000534 to avoid colliding with 000530_relay_host_nats_port on main. - CRF-3: scope summary_change/chat_summary_change merges to their own field so an equal-timestamp event cannot clobber the other summary; add the equal-timestamp regression tests. - CRF-4/CRF-13: bail out of background summary generation when shutdown has begun so Close() is not blocked, and fix the stale comment. - CRF-5: re-read the chat before the cadence gate so rapid turns do not both pass against a stale snapshot. - CRF-7: add a CHECK constraint on chat_messages.cost_source. - CRF-8: use a dedicated chatSummaryWriteTimeout for the summary write. - CRF-17: only count sentence terminators at a word boundary so dotted identifiers do not inflate the count. - CRF-1/CRF-6: deduplicate the usage-recording and model-override resolution paths shared by title and summary generation. - CRF-15/CRF-16: cover ChatMessageVisibilityUser and model-only user messages in the summary tests. - CRF-9/10/11/12/19/22: comment cleanups, errors.AsType, and drop the popover reference from the settings description.
CRF-23: add TestCountSentenceTerminators so the CRF-17 fix is covered. Without it, removing the word-boundary guard in countSentenceTerminators would not fail any test. The test asserts periods inside dotted identifiers are not counted and that a dotted-identifier-dense summary stays under summaryMaxSentences.
Resolve conflicts in coderd/x/chatd/chatd.go by routing both recordManualTitleUsage and recordChatSummaryUsage through the base branch's recordHiddenUsageMessageTx helper, and reimplementing that helper with this PR's atomic InsertChatAccountingMessage insert. This drops the removed UpdateChatMessageCostSource two-step tag and the last_model_config_id restore, which the dedicated accounting insert makes unnecessary. Renumber the chat_history_ignore_accounting migration from 000531 to 000535 so it runs after 000534_chat_summary, which adds the cost_source column the recreated history triggers depend on.
…exclusion Address review feedback on the chat history accounting fix: - InsertChatAccountingMessage stores cost_source verbatim instead of NULLIF(@cost_source, ''), so an empty value fails the chat_messages CHECK instead of silently becoming NULL and re-enabling the history_version advance the trigger guard prevents. - Document in coderd/x/chatd/ARCHITECTURE.md that hidden accounting rows tagged with cost_source do not advance history_version. - Add a mock test for the recordChatSummaryUsage summary path and a DB assertion that an empty cost_source is rejected.
Move summary generation model overrides out of the persisted summary PR so the base feature always uses the chat model. Co-authored-by: Cursor <cursoragent@cursor.com>
…000530 collision Merging main into this stack produced two migrations numbered 000530: 000530_relay_host_nats_port (from main) and 000530_chat_summary (this stack). The duplicate broke coderd/database/dump.sql generation, which cascaded into the gen, lint, sqlc-vet, offlinedocs, and test-go CI jobs. Renumber the stack's migrations above main's latest so every number is unique: - 000530_chat_summary -> 000534_chat_summary - 000531_chat_history_ignore_accounting -> 000535_chat_history_ignore_accounting The schema is unchanged (dump.sql regenerates identically). This resolves an inherited base-stack conflict and may need reconciliation if the base PR is renumbered separately.
…feedback Move the chat summary out of the top-bar info button + popover and into a new left-most "Summary" tab in the right panel: - Add ChatSummaryPanel, a data wrapper that renders the existing presentational ChatSummary inside the panel and fetches the chat + cost only while the tab is visible. - Remove the info button, ChatSummaryPopover, and the ChatTopBar render-prop wiring. Summary is the default right-panel tab when no tab selection is persisted. Address review feedback on the per-chat cost endpoint and UI: - GetChatCostByChatID matches the chat family by indexed columns instead of COALESCE(root_chat_id, id) so Postgres can use the chat indexes. - Drop the dead IsNotAuthorizedError branch (the middleware already returns 404) and return a specific "Failed to get chat cost." 500, matching the sibling chat handlers. - Surface chat and cost query errors and partial (unpriced) totals in the UI, with matching stories. - Add ZeroMessages and ExcludesNonAssistantMessages query tests. - Name the cost staleTime constant and clarify the SQL comments.
Resolve conflict in coderd/x/chatd/chatd.go: adopt the renamed summary threshold constants (summaryInitialTurnThreshold, summaryStaleTurnThreshold) from the base branch while keeping the subagentReportSummary* constants added on this branch. Generated by Coder Agents.
Resolve merge conflicts in three files: - coderd/x/chatd/quickgen.go: keep both "net/http" and "regexp" imports (both are used). - coderd/exp_chats_internal_test.go: union the imports and keep all tests from both sides (TestGetChatCostSurfacesReadAuthzRace and TestGetChatCostQueriesRequestedChat from this branch, plus TestEnrichMissingChatAgentIDs from the base branch). - coderd/x/chatd/turn_summary_internal_test.go: keep this branch's subagent-report summary persistence test (TestSuccessfulChildChatOutcomeStoresReportSummaryWithoutPush). Drop TestPendingChatPersistsSummaryButSkipsWebPush because the base branch removed the unused "pending"/"paused"/"completed" chat statuses (migration 000543, PR #27064); that status no longer exists.
…6657) Adds a persisted whole-chat summary that backs the chat summary popover. A new nullable `chats.summary` column is populated in the background after a successful root-chat turn and pushed to clients via a new `chat_summary_change` watch event (distinct from `summary_change`, which is bound to `last_turn_summary`), so the popover reads `chat.summary` straight off the loaded `Chat` with no extra query. This is the data source for the popover and per-chat cost UI built in #26649; the popover can consume `chat.summary` once this lands (the field is nullable, so merge order does not matter). ## How it works - **Generation** runs in the existing successful-turn finalize hook, detached from the request so the user's turn is never blocked. A cadence gate generates the first summary after one completed turn, then regenerates every three turns, using the `chats.summary_generated_at` freshness marker. Generation reads compaction-aware history, renders it to a bounded plain-text transcript (short transcripts are skipped), and asks for a 1-3 sentence summary via structured output. Failures never clear an existing summary. - **Staleness** is guarded by `history_version` (mirroring `last_turn_summary`), so a background write racing a newer turn loses while worker lifecycle transitions cannot reject a fresh write. - **Model selection** uses the chat's configured model. ## Deferred to follow-ups - **Cost accounting**: the `chat_messages.cost_source` discriminator and summary/title usage recording were removed from this PR so summary persistence is not blocked by hidden accounting rows advancing `history_version`. Title usage recording stays on main's `InsertChatMessages` path. - **Model override**: deployment-wide summary generation model selection is split into #26803; the base feature always uses the chat model. ## Notes - Migration `000540` adds `chats.summary` and `chats.summary_generated_at`, and recreates `chats_expanded` to expose the new columns. - Root chats only; shared viewers pick up the summary on their next refetch (live watch events are owner-only). Refs #26649 --------- Co-authored-by: Cursor <cursoragent@cursor.com>
Resolve conflicts from #26657 (whole-chat summary landed on main) and the AgentsPage -> AgentsPageLayout refactor. - Drop the branch's duplicate 000549_chat_summary migration; main's identical 000551_chat_summary supersedes it. - Adopt main's reordered summary helper signatures (logger before chat) while keeping the net-new subagent report summary feature. - Re-integrate chatCostIdsToInvalidate and the per-chat cost endpoint into the renamed AgentsPageLayout. - Regenerate database code (adds GetChatModelUsageCostByChatID) and API docs.
TestSuccessfulChildChatOutcomeStoresReportSummaryWithoutPush now inserts an assistant message, so deriveFinalTurnRunResult runs past the empty final-text guard and reaches ensureSyntheticAPIKeyID. That path calls p.clock.Now(), which segfaulted because the hand-built Server literal left clock nil. The panic killed the whole coderd/x/chatd test binary, reporting every test in the package as failed. Generated by Coder Agents on behalf of @jaaydenh.
Stacked on #26657 (the persisted whole-chat summary backend). Base branch is
chat-summary-62j9; review/merge that first.Adds a reusable
ChatSummarycomponent.The summary text is the persisted whole-chat summary (
chat.summary) introduced by #26657. It is generated asynchronously and may benulluntil the first summary is produced, in which case the popover renders a muted empty state. Live updates arrive via that PR'schat_summary_changewatch event, which is already merged into the chat caches.Cost is served by a new per-chat endpoint,
GET /api/experimental/chats/{chat}/cost, which rolls up assistant-message cost across a chat's root and child (subagent) chats and is authorized like the other{chat}routes (read on the chat, 404 otherwise).Visual and interaction coverage lives in
ChatSummary.stories.tsxandChatSummaryPopover.stories.tsx(including populated-summary, empty-state, and cost-loading cases).