feat: integrate agent context snapshots into chats#26389
Merged
Conversation
Docs preview📖 View docs preview for |
kylecarbs
force-pushed
the
chatd-context-integration
branch
2 times, most recently
from
9c176dc to
dd9a1ee
Compare
Member
Author
|
/coder-agents-review |
Contributor
|
Chat: Review posted | View chat Review historydeep-review v0.7.1 | Round 3 | Last posted: Round 3, 23 findings (1 P2, 10 P3, 1 P4, 7 Nit, 4 Note), APPROVE. Review Finding inventoryFinding inventoryFindings
Contested and acknowledgedCRF-3 (Note, coderd/x/chatd/context_hydration.go:116) - RefreshChatContext does not emit watch event on dirty clear
CRF-16 (Note, coderd/database/queries/chats.sql:1494) - HydrateAgentChatsContext stamps terminal chats
CRF-17 (Note, coderd/x/chatd/chatd.go:1464) - Created event published before hydration
Round logRound 1Panel. 19 reviewers (Bisky, Hisoka, Mafu-san, Mafuuu, Pariston, Gon, Leorio, Ging-Go, Ryosuke, Chopper, Robin, Knov, Kurapika, Kite, Meruem, Knuckle, Zoro, Razor, Killua). 1 P2, 7 P3, 1 P4, 4 Nit, 4 Note. Reviewed against 210261b..dd9a1ee04b7a. Round 2Churn guard: PROCEED. 14 addressed, 3 acknowledged. Panel: 11 reviewers (Bisky, Hisoka, Mafu-san, Mafuuu, Pariston, Gon, Leorio, Meruem, Kite, Chopper, Knov). 3 P3, 3 Nit new. Reviewed against 210261b..bedf763fcd0a. Round 3Churn guard: PROCEED. 6 addressed, 0 silent. Panel: 5 reviewers (Bisky, Hisoka, Mafuuu, Pariston, Meruem). 0 new findings. All R1+R2 fixes verified. APPROVE. About deep-reviewCRF = Coder Review Finding (P0-P4, Nit, Note)
|
![coder-agents-review[bot]](https://avatars.githubusercontent.com/in/3454270?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
The transactional design of HydrateAndMarkChatsDirty is strong: running hydration and dirty marking inside the push's ReadModifyUpdate transaction prevents phantom events from rolled-back retries, and the post-commit callback pattern ensures watch events only fire for committed state. The SDK type design (nullable ChatContext sub-struct, omitempty) is clean.
1 P2, 7 P3, 1 P4, 4 Nit, 4 Note across 19 reviewers.
"I tried to build a case against this change and could not." (Pariston)
🤖 This review was automatically generated with Coder Agents.
Member
Author
|
/coder-agents-review |
Contributor
There was a problem hiding this comment.
All 14 fixed R1 findings verified against bedf763. The R1 P2 (TOCTOU in RefreshChatContext) is properly closed: snapshot read and re-pin now share a single ReadModifyUpdate transaction with serialization-error retry. Create-time hydration correctly uses the NULL-guarded HydrateAgentChatsContext. The shared latestAgentSnapshot helper eliminated the duplication. Test coverage is substantially improved.
3 P3, 2 Nit across 11 reviewers. R1 acknowledged Notes (CRF-3, CRF-16, CRF-17) remain deferred to the UI phase.
"This is a gem." (Bisky, on the integration test)
Process note: the fix commit subject (fix(chatd): address deep-review feedback on chat context) names the process, not the fix. If not squash-merged, consider fix(chatd): close TOCTOU in context refresh, tighten create-time hydration. (Leorio)
🤖 This review was automatically generated with Coder Agents.
Member
Author
|
/coder-agents-review |
sreya
approved these changes
Jun 16, 2026
kylecarbs
pushed a commit
that referenced
this pull request
Jun 16, 2026
## Problem `test-go-pg (windows-2022)` has been red on `main` since the Vercel CLI was added to `mise.toml`. Every PR that runs Windows CI (including #26389) fails at `Restore Go cache` -> setup-mise -> `mise install`, and the `:x: CI Failure` Slack notifications have been firing ~hourly because of it. The Vercel CLI lives in the top-level `[tools]` block, so mise tries to install it on every runner. It is installed via the `npm:vercel` backend, which runs: ``` C:\Windows\system32\cmd.exe /d /s /c node install.js ``` as an npm post-install script. mise installs node into its own shim dir; `node` is not on cmd.exe's PATH at the moment npm spawns the post-install script, so it fails with: ``` npm error command C:\Windows\system32\cmd.exe /d /s /c node install.js npm error 'node' is not recognized as an internal or external command, npm error operable program or batch file. mise ERROR Failed to install npm:vercel@54.14.0: npm.cmd exited with non-zero status: exit code 1 ``` The whole setup-mise step aborts, so the rest of the job is skipped. Tracked in coder/internal#1596. ## Fix The Vercel CLI is only used to deploy / preview from dogfood workspaces (Linux). Restrict it to linux/macos via the documented [`os`](https://mise.jdx.dev/dev-tools/#os-specific-tools) filter: ```toml vercel = { version = "54.14.0", os = ["linux", "macos"] } ``` On Windows mise will now skip the install entirely. On Linux/macOS behavior is unchanged, the binary still resolves through the same `npm:vercel` backend. ## Lockfile `os` is an install-time filter and has no representation in `mise.lock`. `mise lock` against the pinned `min_version = "2026.5.12"` produces no diff, so the lockfile is intentionally left as-is. ## Verification - `mise trust && mise ls` on Linux still resolves `vercel 54.14.0` from `mise.toml`. - `mise lock` against pinned v2026.5.12 is a no-op on `mise.lock`. - Windows runners will report `(skipped)` for `vercel` and continue past setup-mise. Fixes coder/internal#1596. Created on behalf of @kylecarbs. Co-authored-by: blink-so[bot] <211532188+blink-so[bot]@users.noreply.github.com>
Makes the chat context foundation (#26385) live. Agent context pushes now hydrate bound chats and mark them dirty on drift, chat creation pins the agent's latest snapshot, the experimental chat API reports the pinned context state, and a new refresh endpoint re-pins the latest snapshot and clears the dirty marker. - coderd/x/chatd/context_hydration.go: HydrateAndMarkChatsDirty (push-time hydrate + dirty fan-out, publishes context_dirty after commit), hydrateChatContextOnCreate (create-time pin, best-effort), and RefreshChatContext (re-pin latest snapshot + clear dirty). - coderd/workspaceagentsrpc.go: wire the chat daemon in as the agentapi ContextDirtyMarker, nil-guarded so a disabled daemon stays a pure write path. - codersdk + db2sdk: expose Chat.Context (*ChatContext) carrying dirty state and snapshot error; re-add the context_dirty watch event. - coderd: PUT /api/experimental/chats/{chat}/context refresh endpoint. context_dirty_resources stays NULL and the live per-turn context pull is unchanged. Adds an end-to-end test where an echo-provisioned agent pushes an initial snapshot (hydrate), re-pushes a different hash (drift -> dirty), the API reports dirty, and refresh clears it.
- RefreshChatContext now reads the snapshot and re-pins in one repeatable-read transaction, closing a TOCTOU window where a concurrent push could leave the chat pinned to a stale hash with the dirty marker cleared (CRF-4). - hydrateChatContextOnCreate pins via the NULL-guarded HydrateAgentChatsContext so a concurrent push that already hydrated the chat is not clobbered (CRF-6). - The dirty fan-out resolves only the dirtied chats after commit instead of scanning every active chat for the agent (CRF-10), and reuses publishChatPubsubEvents (CRF-15). - Extract a shared latestAgentSnapshot helper (CRF-11), rename flipped->dirtied (CRF-14), align the no-op callback contract with its doc (CRF-13), and trim restated comments (CRF-12). - Add @produce json to the refresh endpoint (CRF-1). - Tests: exercise the snapshot-error path end-to-end, assert an agent-less chat is untouched by the fan-out, and re-push the pinned hash to prove refresh advanced the pin (CRF-7/8/9); add an internal test for create-time hydration (CRF-2).
Read the dirtied chats inside the push transaction and publish the captured rows after commit, so the post-commit callback needs no database access. This closes two issues introduced when the read was moved post-commit: - the callback used the request-scoped RPC context for the read, so a dying agent stream could silently drop every dirty notification; - the post-commit re-read could observe a concurrent refresh clearing the marker, publishing a context_dirty event whose payload was clean. The read still touches only the transitioned chats, not every active chat for the agent. Also assert the pinned error is unchanged at the dirty step (the dirty fan-out must not overwrite it) and trim two doc comments that restated the signature / leaked a SQL detail.
Contributor
There was a problem hiding this comment.
All 23 findings across 3 rounds resolved. R1 P2 (TOCTOU in RefreshChatContext) closed with ReadModifyUpdate. R2 P3s (post-commit context lifetime and TOCTOU) closed by capturing dirtied rows in-transaction. Test coverage is thorough: integration test exercises the full state machine (nil -> hydrated -> dirty -> refreshed -> re-push stays clean) with negative guards and error propagation; internal unit tests cover the three create-time hydration branches.
"The threads I pulled held." (Hisoka, R3)
🤖 This review was automatically generated with Coder Agents.
kylecarbs
force-pushed
the
chatd-context-integration
branch
from
30b54eb to
ae3c50b
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Makes the chat context foundation from #26385 live. That PR added the storage columns, writer queries, and a dormant
agentapi.ContextDirtyMarkertrigger with no production callers; this PR wires them together end to end.When a workspace agent pushes a context snapshot, bound chats now hydrate to that snapshot's hash, and a later push with a different hash flips already-pinned chats to dirty (emitting a
context_dirtywatch event after the transaction commits). Chat creation pins the agent's latest snapshot when one already exists. The experimental chat API exposes this asChat.Context(*ChatContextwithdirty,dirty_since,error), and a newPUT /api/experimental/chats/{chat}/contextendpoint re-pins the agent's latest snapshot and clears the dirty marker.context_dirty_resourcesstays NULL (the resource-level diff is deferred to the UI phase) and the live per-turn context pull is unchanged.The end-to-end test provisions a workspace agent via the echo provisioner, connects it over the Agent API v2.10, and exercises the full path: an initial push hydrates a bound chat (clean), a second push with a different hash marks it dirty, the API reports the dirty state, and the refresh endpoint clears it.
Decision log
codersdk.Chat.Context *ChatContext { Dirty bool; DirtySince *time.Time; Error string }rather than flat fields, matching the RFC's namedChatContexttype and leaving room for future fields (resource diff, sources).db2sdk.Chatpopulates it when the chat is context-tracked (len(ContextAggregateHash) > 0), dirty, or carries a snapshot error, and leaves it nil (omitempty) otherwise.Dirtymirrorscontext_dirty_sincebeing set.agentapi.ContextDirtyMarker. It is unconditionally constructed (only its background worker is gated), so the marker is always non-nil and the wiring matches every otherapi.chatDaemoncall site.agentapistill treats a nil marker as "chatd absent", soPushContextStatestays a pure write path for any future caller that does not wire chatd in.RefreshChatContextreads the agent's latest snapshot and re-pins the chat in one repeatable-read transaction, so a concurrent push cannot land between the read and the write and leave the chat pinned to a stale hash with the dirty marker cleared.context_dirtywatch events publish only after commit. The pinned hash on dirtied chats is intentionally left unchanged — the refresh endpoint re-pins it.chats.agent_iddirectly. In production the binding is set lazily during a chat turn (chatd.persistBuildAgentBinding); the test sets it viadbgenso it exercises the context flow rather than turn resolution.Plan:
coderd/x/chatdcontext integration + E2E (sub-struct API, create-time + push-time hydration, refresh endpoint;context_dirty_resourcesand the per-turn pull untouched).🤖 Generated by Coder Agents on behalf of @kylecarbs