mirror of
https://github.com/coder/coder.git
synced 2026-10-28 02:42:54 +08:00
c048a4093e37bb0a27fb5ffbb3c3592c5aec2dcc
100
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Kyle Carberry
|
8c8b307b97 | fix: persist session cookie to disk to prevent PWA logout (#23746) | ||
|
Kyle Carberry
|
19e44f4136 |
fix: target specific chat in MarkStale instead of broadcasting to all workspace chats (#23883)
## Problem Subagent chats were receiving git context (branch, remote origin, PR status) from their parent or sibling chats' git operations. When a git operation triggers external auth, the workspace agent sends `chat_id` identifying which chat initiated it — but this was broken at two levels: 1. **Agent side:** `CODER_CHAT_ID` was never injected into process environments. `chatd` sets `Coder-Chat-Id` HTTP headers and the agent extracts them for process isolation, but never propagated `CODER_CHAT_ID` to `cmd.Env`. So `gitaskpass` always sent an empty `chat_id`. 2. **Server side:** `workspaceAgentsExternalAuth` ignored the `chat_id` query param. `MarkStale` broadcast git context to **all** chats on the workspace via `filterChatsByWorkspaceID`. ## Fix - Inject `CODER_CHAT_ID` into `cmd.Env` in `agentproc` when the chat ID is known, so `gitaskpass` can read and forward it. - Read `chat_id` from query params in `workspaceAgentsExternalAuth` and thread it through `chatGitRef`. - Refactor `MarkStale` to accept a `MarkStaleParams` struct. When `ChatID` is provided, target only that specific chat. When empty (legacy agents, non-chat git operations), fall back to the existing workspace-wide broadcast. - Extract `markStaleSingle` helper to deduplicate the upsert+publish logic. <details><summary>Investigation notes</summary> ### Data flow before fix ``` chatd → sets Coder-Chat-Id header on agent conn agent → extracts chatID, stores on process struct agent → does NOT set CODER_CHAT_ID in cmd.Env ← gap 1 gitaskpass → reads CODER_CHAT_ID (always empty), sends chat_id="" server handler → ignores chat_id query param ← gap 2 MarkStale → broadcasts to ALL workspace chats ``` ### Data flow after fix ``` chatd → sets Coder-Chat-Id header on agent conn agent → extracts chatID, stores on process struct agent → sets CODER_CHAT_ID in cmd.Env gitaskpass → reads CODER_CHAT_ID, sends chat_id=<uuid> server handler → reads chat_id, passes to MarkStale MarkStale → targets only that specific chat ``` </details> |
||
|
Kyle Carberry
|
2ea89e1f1b |
fix(site/src/pages/AgentsPage): show Thinking indicator immediately after sending a message (#23904)
After sending a message, `handleSend` clears stream state and inserts the user message but did not set `chatStatus` to `"running"`. Combined with #23805 narrowing `selectIsAwaitingFirstStreamChunk` to only match `chatStatus === "running"` (instead of `isActiveChatStatus` which included `"pending"`), the "Thinking..." indicator could not appear until the WebSocket delivered `status:running` — a 50–500ms+ gap. Optimistically set `chatStatus` to `"running"` in both the send and edit paths after the POST returns (non-queued). The WebSocket `status:running` event no-ops via the `setChatStatus` guard; error/pending events override the optimistic value. <details><summary>Investigation & decision log</summary> ### Root cause chain 1. **PR #23805** (`953c3bdc0`) changed `selectIsAwaitingFirstStreamChunk` from `isActiveChatStatus(state.chatStatus)` → `state.chatStatus === "running"`. Valid fix: during `"pending"`, `shouldApplyMessagePart()` drops stream parts, so `streamState` stays null and the 15s "startup taking too long" warning fired spuriously during multi-turn tool-call cycles. 2. **PR #23884** (`4b5265695`) fixed event ordering within a WebSocket batch so both `[message_part, status:running]` and `[status:running, message_part]` orderings show "Thinking...". Correct fix, but only operates **after** `chatStatus` reaches `"running"`. 3. `handleSend` never set `chatStatus` optimistically — it relied entirely on the WebSocket `status:running` event. After #23805 narrowed the selector, the gap between POST completion and WebSocket event became visible. ### Why this fix is safe - Non-queued POST = server accepted the message → `"running"` is the correct next state. - `setChatStatus("running")` guard: `if (state.chatStatus === status) return` makes the subsequent WebSocket confirmation a no-op. - If the server transitions to error/pending instead, the WebSocket event overrides the optimistic value. - `shouldApplyMessagePart()` returns `true` for `"running"`, so early stream parts arriving before the WebSocket `status:running` will not be silently dropped. ### What was NOT regressed by PR #23884 PR #23884's `setTimeout(0)` deferred flush is correct. Both event orderings now produce a render cycle where `chatStatus === "running"` and `streamState === null`, allowing "Thinking..." to appear. The `setTimeout(0)` fires in a separate macrotask, giving the browser a paint opportunity. </details> |
||
|
Kyle Carberry
|
7861fcf1f6 |
perf(coderd): stop inline-resolving diff status on every GetChat call (#23901)
## Problem
Every `GET /api/experimental/chats/{chatID}` call was blocking for
200-800ms because the `getChat` handler called `resolveChatDiffStatus`,
which unconditionally hit the git provider API (e.g. GitHub's `GET
/repos/{owner}/{repo}/pulls?head=...`) via `ResolveBranchPullRequest` —
even when the cached diff status was fresh.
This made every chat page load at `/agents/{id}` noticeably slow.
## Root cause
The call chain was:
1. `getChat` → `resolveChatDiffStatus`
2. `resolveChatDiffStatus` → `resolveChatDiffReference` →
`gp.ResolveBranchPullRequest(...)` **(external HTTP call)**
3. Only **after** the external call: `chatDiffStatusIsStale(status,
now)` check
The staleness check happened after the expensive work, so every request
paid the cost regardless of cache freshness.
## Fix
`getChat` now returns the cached `chat_diff_statuses` row directly from
the database. The background `gitsync` worker already keeps these rows
fresh (every `DiffStatusTTL = 120s`), so inline resolution was
redundant.
The `resolveChatDiffContents` endpoint (which fetches actual diff
content) still uses the full resolution path since it needs to make
provider API calls by design.
## Changes
- `getChat` reads cached diff status from DB instead of calling
`resolveChatDiffStatus`
- Remove `resolveChatDiffStatus` (dead code — no production callers)
- Remove `chatDiffStatusIsStale` and `chatDiffStatusTTL` (dead code)
- Remove `RefreshesStaleStatusWithExternalAuth` test (tested the removed
inline refresh path)
<details><summary>Decision log</summary>
- **Why not just add a staleness gate?** The background worker already
handles refreshes on the same schedule. Adding an early-return-if-fresh
would work but leaves dead code for the stale path that's never
exercised in production (the worker gets there first). Removing the
inline path entirely is simpler and eliminates the external API
dependency from the read path.
- **Why keep `resolveChatDiffContents` unchanged?** That endpoint's job
is to fetch the actual diff content from the provider, so external API
calls are inherent to its purpose.
</details>
|
||
|
Kyle Carberry
|
4b52656958 |
fix(site): ensure Thinking indicator appears regardless of WebSocket event ordering (#23884)
The "Thinking..." indicator intermittently failed to render after submitting a message. The behavior depended on the order of events within a single WebSocket frame. ## Root Cause `flushMessageParts()` was called before **all** non-`message_part` events in the batch loop. When the server sent `[message_part, status:"running"]` in the same SSE chunk: 1. `message_part` → pushed to `partsBuf` 2. `status:"running"` → `flushMessageParts()` applied parts **first** → `streamState` became non-null → then `chatStatus` set to `"running"` 3. Subscriber saw `streamState != null && chatStatus == "running"` → `selectIsAwaitingFirstStreamChunk` returned `false` → no "Thinking..." When events arrived in the reverse order (`[status:"running", message_part]`), the indicator worked because the status was set before parts were applied. ## Fix - Move `flushMessageParts()` to only fire before `message` and `error` events (which need prior parts visible) - Add `discardBufferedParts()` for events that clear stream state (`status:"pending"/"waiting"`, `retry`) so the deferred `setTimeout(0)` flush doesn't re-populate cleared state - Status changes are now always applied before parts within a batch, and the deferred flush gives React one render cycle to show "Thinking..." | Event | Flush? | Rationale | |---|---|---| | `message` | YES | Durable commit must include all stream parts | | `error` | YES | Partial output should be visible alongside error | | `status` | NO | Status must be set before parts so "starting" phase renders | | `retry` | DISCARD | Retry clears stream state; flushing would re-populate it | | `queue_update` | NO | Doesn't interact with stream state | ## Tests (written first, failing before fix) 1. **"shows starting phase when message_part arrives before status:running in same batch"** — the exact bug scenario 2. **"shows starting phase when status:running arrives before message_part in same batch"** — verifies the "good" order still works 3. **"discards buffered parts when status transitions to pending"** — verifies parts don't leak through pending transitions All tests are deterministic (fake timers, no race conditions). <details><summary>Implementation plan & decision log</summary> ### Why not reorder events within the batch? Reordering would change the semantic ordering of events from the server, which could have subtle side effects. The simpler approach is to be selective about when parts are flushed. ### Why discard (not flush) before pending/waiting/retry? These events clear `streamState`. If parts were flushed before the clear, they'd be visible for one frame then disappear. If the deferred flush ran after the clear, it would re-populate the state. Discarding is the only correct behavior. ### Why keep flush before error? Errors should surface partial output so the user can see what the agent was doing when it failed. </details> |
||
|
Kyle Carberry
|
2c5e003c91 |
refactor(site): use hover popover for context indicator with nested skill tooltips (#23870)
Replaces the tooltip-inside-tooltip approach for the context usage
indicator with a hover-based Popover. Skill descriptions now appear as
nested tooltips to the right, matching the ModelSelector pattern.
**Before**: Tooltip with inline skill descriptions (truncated, janky
nested tooltips)
**After**: Popover opens on hover, skill names listed cleanly,
descriptions appear to the right on hover
- Popover opens on `mouseEnter`, closes after 150ms delay on
`mouseLeave`
- `onOpenAutoFocus` prevented to avoid stealing chat input focus
- Mobile keeps tap-to-toggle Popover behavior
- Skill rows get subtle `hover:bg-surface-tertiary` highlight
- `TooltipProvider` with `delayDuration={300}` wraps skill items (same
as ModelSelector)
|
||
|
Kyle Carberry
|
2953245862 |
feat(site): display loaded context files and skills in context indicator tooltip (#23853)
Renders the `last_injected_context` data (AGENTS.md files and skills) from the Chat API in the `ContextUsageIndicator` hover tooltip. On hover, users now see: - **Context files**: basename with full path on title hover, truncation indicator - **Skills**: name and optional description Separated from the existing token usage info by a border divider when both sections are present. Added `max-w-72` to prevent the tooltip from getting too wide. <img width="970" height="598" alt="image" src="https://github.com/user-attachments/assets/5bc25cb2-1d92-41d2-ab1a-63e5e49f667a" /> <details> <summary>Data flow</summary> ``` chatQuery.data.last_injected_context → AgentChatPage (AgentChatPageView prop) → AgentChatPageView (ChatPageInput prop) → ChatPageInput (spread into latestContextUsage) → AgentChatInput (contextUsage prop) → ContextUsageIndicator (usage.lastInjectedContext) ``` </details> <details> <summary>Files changed</summary> | File | Change | |---|---| | `ContextUsageIndicator.tsx` | Add `lastInjectedContext` to interface, render context files and skills sections in tooltip | | `ChatPageContent.tsx` | Thread `lastInjectedContext` prop, spread into context usage object | | `AgentChatPageView.tsx` | Thread `lastInjectedContext` prop to `ChatPageInput` | | `AgentChatPage.tsx` | Pass `chatQuery.data?.last_injected_context` down | </details> |
||
|
Kyle Carberry
|
b3d5b8d13c |
fix: stabilize flaky chatd subscribe/promote queued tests (#23816)
## Summary Fixes three flaky chatd tests that intermittently fail due to timing races with the background run loop. Closes coder/internal#1428 ## Root Cause `CreateChat` and `PromoteQueued` call `signalWake()` which writes to `wakeCh`, triggering `processOnce` immediately. Even though `newTestServer` sets `PendingChatAcquireInterval: testutil.WaitLong` to prevent ticker-based polling, the wake channel bypasses this. This causes `processOnce` to acquire and process the chat concurrently with the test's manual DB updates and assertions. ### Failing tests | Test | Failure | Cause | |------|---------|-------| | `TestPromoteQueuedAllowsAlreadyQueuedMessageWhenUsageLimitReached` | `expected: "pending", actual: "running"` | Wake from `CreateChat` races with manual `UpdateChatStatus`; wake from `PromoteQueued` acquires the chat before the status assertion | | `TestSendMessageInterruptBehaviorQueuesAndInterruptsWhenBusy` | `should have 1 item(s), but has 2` | Wake from `CreateChat` triggers `processChat` which auto-promotes a queued message, adding an extra row to `chat_messages` | | `TestSubscribeNoPubsubNoDuplicateMessageParts` | `Condition satisfied` (duplicate events) | Pre-existing `WaitGroup.Add/Wait` race in the `Eventually` + `WaitUntilIdleForTest` pattern | ## Fix Introduces a `waitForChatProcessed` helper that: 1. Polls until the chat reaches a **terminal state** (not pending AND not running) 2. Then calls `WaitUntilIdleForTest` to wait for the inflight `WaitGroup` Waiting for a terminal state (not just "not pending") avoids a `sync.WaitGroup` `Add/Wait` race: `AcquireChats` updates the DB status to `running` **before** `processOnce` calls `inflight.Add(1)`. Checking only `status != pending` could return while `Add(1)` hasn't happened yet, causing `Wait()` to return prematurely. ### Per-test changes - **`TestSendMessageInterruptBehaviorQueuesAndInterruptsWhenBusy`**: Call `waitForChatProcessed` after `CreateChat` before manually setting running status - **`TestPromoteQueuedAllowsAlreadyQueuedMessageWhenUsageLimitReached`**: Call `waitForChatProcessed` after `CreateChat`; remove the inherently racy `status == pending` assertion after `PromoteQueued` (the wake immediately acquires the chat). Key assertions on promoted message, queue state, and message count remain. - **`TestSubscribeNoPubsubNoDuplicateMessageParts`**: Replace inline `Eventually` with the safer `waitForChatProcessed` helper ## Verification All three tests pass 150 consecutive executions with `-race -count=10` across 15 runs (0 failures). |
||
|
Kyle Carberry
|
a5cc579453 |
feat: add last_injected_context column to chats table (#23798)
Adds a nullable JSONB column `last_injected_context` to the `chats` table that stores the most recently persisted injected context parts (AGENTS.md context-file and skill message parts). The column is updated only when `persistInstructionFiles()` runs — on first workspace attach or when the agent changes — so there are no redundant writes on subsequent turns. Internal fields (`ContextFileContent`, `ContextFileOS`, `ContextFileDirectory`, `SkillDir`) are stripped at write time so the column only holds small metadata. No stripping needed on the read path. <details> <summary>Implementation notes</summary> - New migration `000456` adds nullable `last_injected_context JSONB` column. - New SQL query `UpdateChatLastInjectedContext` writes the column without touching `updated_at`. - `persistInstructionFiles()` strips internal fields from parts via `StripInternal()` before persisting. - Sentinel path (no AGENTS.md) persists skill-only parts when skills exist. - `codersdk.Chat` exposes `LastInjectedContext []ChatMessagePart` (omitempty). - `db2sdk.Chat()` passes through the already-clean data. </details> |
||
|
Kyle Carberry
|
953c3bdc0f |
fix(site): prevent spurious startup warning during pending status (#23805)
## Problem The `/agents` page frequently shows "Response startup is taking longer than expected" even while the agent is actively working and messages are appearing in the transcript. ## Root Cause There's an inconsistency between `isActiveChatStatus` and `shouldApplyMessagePart` during `"pending"` status (the state between agent tool-call turns): | Component | Treats `"pending"` as... | |---|---| | `isActiveChatStatus` | **active** — includes both `"running"` and `"pending"` | | `shouldApplyMessagePart` | **inactive** — drops all `message_part` events during `"pending"` | | Status handler | clears `streamState` to `null` on `"pending"` | This creates a dead state during multi-turn tool-call cycles: 1. Agent finishes a turn → status = `"pending"` → `streamState` cleared to `null` 2. `selectIsAwaitingFirstStreamChunk` returns `true` (status is "active", stream is null, latest message isn't assistant) 3. Phase = `"starting"` → 15s timer starts 4. Stream parts from the server are **silently dropped** (`shouldApplyMessagePart()` returns `false` for `"pending"`) 5. `streamState` stays `null` — phase is stuck at `"starting"` 6. Meanwhile, durable messages (tool calls, tool results) appear normally in the transcript 7. After 15s → "Response startup is taking longer than expected" fires ## Fix Narrow `selectIsAwaitingFirstStreamChunk` to only check `chatStatus === "running"` instead of `isActiveChatStatus(chatStatus)`. `"running"` is the only status where the transport actually accepts stream parts, so it's the only status where we should be showing the "starting" indicator. `isActiveChatStatus` is left unchanged since its other caller (`shouldSurfaceReconnectState`) correctly needs to include `"pending"`. |
||
|
Kyle Carberry
|
4d2b0a2f82 |
feat: persist skills as message parts like AGENTS.md (#23748)
## Summary Skills are now discovered once on the first turn (or when the workspace agent changes) and persisted as `skill` message parts alongside `context-file` parts. On subsequent turns, the skill index is reconstructed from persisted parts instead of re-dialing the workspace agent. This makes skills consistent with the AGENTS.md pattern and is groundwork for a future `/context` endpoint that surfaces loaded workspace context to the frontend. ## Changes - Add `skill` `ChatMessagePartType` with `SkillName` and `SkillDescription` fields - Extend `persistInstructionFiles` to also discover and persist skills as parts - Add `skillsFromParts()` to reconstruct skill index from persisted parts on subsequent turns - Update `runChat()` to use `skillsFromParts` instead of re-dialing workspace for skills - Frontend: handle new `skill` part type (skip rendering, hide metadata-only messages) ## Before / After | | AGENTS.md | Skills | |---|---|---| | **Before** | Persist as `context-file` parts, reconstruct from parts | In-memory `skillsCache` only, re-dial workspace on cache miss | | **After** | Persist as `context-file` parts, reconstruct from parts | Persist as `skill` parts, reconstruct from parts | The in-memory `skillsCache` remains for `read_skill`/`read_skill_file` tool calls that need full skill bodies on demand. <details><summary>Design context</summary> This is the first step toward a unified workspace context representation. Currently: - Context files are persisted as message parts (works) - Skills were only in-memory (inconsistent) - Workspace MCP servers are cached in-memory (future work) Persisting skills as parts means a future `/context` endpoint can query both context files and skills from the same message parts in the DB, without depending on ephemeral server-side caches. </details> |
||
|
Kyle Carberry
|
be99b3cb74 |
fix: prioritize context cancellation in WebSocket sendEvent (#23756)
## Problem
Commit
|
||
|
Kyle Carberry
|
386b449273 |
perf(coderd): reduce chat streaming latency with event-driven acquisition (#23745)
Previously, when a user sent a message, there was a 0–1000ms (avg ~500ms) polling delay before processing began. `SendMessage`/`CreateChat`/`EditMessage` set `status='pending'` in the DB and returned, but nothing woke the processing loop — it was a blind 1-second ticker. ## Changes **Event-driven acquisition (main change):** Adds a `wakeCh` channel to the chatd `Server`. `CreateChat`, `SendMessage`, `EditMessage`, and `PromoteQueued` call `signalWake()` after committing their transactions, which wakes the run loop to call `processOnce` immediately. The 1-second ticker remains as a fallback safety net for edge cases (stale recovery, missed signals). **Buffer WebSocket write channel:** Changes the `OneWayWebSocketEventSender` event channel from unbuffered to buffered (64), decoupling the event producer from WebSocket write speed. The existing 10s write timeout guards against stuck connections. <details><summary>Implementation plan & analysis</summary> The full latency analysis identified these sources of delay in the streaming pipeline: 1. **Chat acquisition polling** — 0–1000ms (avg 500ms) dead time per message. Fixed by wake channel. 2. **Unbuffered WebSocket write channel** — each token blocked on the previous WS write completing. Fixed by buffering. 3. **PersistStep DB transaction per step** — `FOR UPDATE` lock + batch insert. Not addressed in this PR (medium risk, would overlap DB write with next provider TTFB). 4. **Multi-hop channel pipeline** — 4 channel hops per token. Not addressed (medium complexity). </details> <details><summary>Test stabilization notes</summary> `signalWake()` causes the chatd daemon to process chats immediately after creation/send/edit, which exposed timing assumptions in several tests that expected chats to remain in `pending` status long enough to assert on. These tests were updated with `require.Eventually` + `WaitUntilIdleForTest` patterns to wait for processing to settle before asserting. The race detector (`test-go-race-pg`) shows failures in `TestCreateWorkspaceTool_EndToEnd` and `TestAwaitSubagentCompletion` — these appear to be pre-existing races in the end-to-end chat flow that are now exercised more aggressively because processing starts immediately instead of after a 1s delay. Main branch CI (race detector) passes without these changes. </details> |
||
|
Kyle Carberry
|
a2799560eb |
fix: use Popover for context indicator on mobile viewports (#23747)
## Problem The context-usage indicator ring in the agents chat uses a Radix UI `Tooltip`, which only opens on hover. On mobile/touch devices there is no hover event, so tapping the indicator does nothing. ## Fix On mobile viewports (`< 640 px`, matching the existing `isMobileViewport()` helper), render a `Popover` instead of a `Tooltip` so that tapping the ring toggles the context-usage info. Desktop behavior (hover tooltip) is unchanged. - Extract the trigger button and content into shared variables to avoid duplication - Conditionally render `Popover` (mobile) or `Tooltip` (desktop) based on viewport width - Both `Popover` and `PopoverContent` were already imported in the file |
||
|
Kyle Carberry
|
a708e9d869 |
feat: add tool rendering for read_skill, read_skill_file, start_workspace (#23744)
These tools previously fell through to the `GenericToolRenderer` which
showed a wrench icon and the raw tool name. Now they get dedicated icons
and contextual labels.
## Changes
**ToolIcon.tsx** — new icon mappings:
- `read_skill` / `read_skill_file` → `BookOpenIcon`
- `start_workspace` → `PlayIcon`
- `web_search` → `SearchIcon`
**ToolLabel.tsx** — contextual labels:
- `read_skill`: "Reading skill {name}…" → "Read skill {name}"
- `read_skill_file`: "Reading {skill}/{path}…" → "Read {skill}/{path}"
- `start_workspace`: "Starting workspace…" → "Started {name}"
- `web_search`: "Searching \"{query}\"…" → "Searched \"{query}\""
**tool.stories.tsx** — 12 new stories covering running, completed, and
error states for all four tools.
<details>
<summary>Visually verified in Storybook</summary>
All 10 story variants verified:
- ReadSkillRunning / ReadSkillCompleted / ReadSkillError
- ReadSkillFileRunning / ReadSkillFileCompleted / ReadSkillFileError
- StartWorkspaceRunning / StartWorkspaceCompleted / StartWorkspaceError
- WebSearchRunning / WebSearchCompleted / WebSearchNoQuery
</details>
|
||
|
Kyle Carberry
|
839165818b |
feat(coderd/x/chatd): add skills discovery and tools for chatd (#23715)
Adds skill discovery and tools to chatd so the agent can discover and load `.agents/skills/` from workspaces, following the same pattern as AGENTS.md instruction loading and MCP tool discovery. ## What changed ### `chattool/skill.go` — discovery, loading, and tools - **DiscoverSkills** — walks `.agents/skills/` via `conn.LS()` + `conn.ReadFile()`, parses SKILL.md frontmatter (name + description), validates kebab-case names match directory names, silently skips broken/missing entries. - **FormatSkillIndex** — renders a compact `<available-skills>` XML block for system prompt injection (~60 tokens for 3 skills). Progressive disclosure: only names + descriptions in context, full body loaded on demand. - **LoadSkillBody** / **LoadSkillFile** — on-demand loading with path traversal protection and size caps (64KB for SKILL.md, 512KB for supporting files). - **read_skill** / **read_skill_file** tools — `fantasy.AgentTool` implementations following the same pattern as ReadFile and WorkspaceMCPTool. Receive pre-discovered `[]SkillMeta` via closure to avoid re-scanning on every call. ### `chatd.go` — integration into runChat - Skills discovered in the `g2` errgroup parallel with instructions and MCP tools. - `skillsCache` (sync.Map) per chat+agent, same invalidation pattern as MCP tools cache. - Skill index injected via `InsertSystem` after workspace instructions. - Re-injected in `ReloadMessages` callback so it survives compaction. - `read_skill` + `read_skill_file` tools registered when skills are present (for both root and subagent chats). - Cache cleaned up in `cleanupStreamIfIdle` alongside MCP tools cache. ## Format compatibility Uses the same `.agents/skills/<name>/SKILL.md` format as [coder/mux](https://github.com/coder/mux) and [openai/codex](https://github.com/openai/codex). |
||
|
Kyle Carberry
|
6b77fa74a1 |
fix: remove bold font-weight from unread chats in agents sidebar (#23725)
Removes the `font-semibold` class applied to unread chat titles in the `/agents` sidebar. The unread indicator dot already provides sufficient visual distinction for unread chats. |
||
|
Kyle Carberry
|
bcdc35ee3e |
feat: add chat read/unread indicator to sidebar (#23129)
## Summary Adds read/unread tracking for chats so users can see which agent conversations have new assistant messages they haven't viewed. ## Backend Changes - Adds `last_read_message_id` column to the `chats` table (migration 000439). - Computes `has_unread` as a virtual column in `GetChatsByOwnerID` using an `EXISTS` subquery checking for assistant messages beyond the read cursor. - Exposes `has_unread` on the `codersdk.Chat` struct and auto-generated TypeScript types. - Updates `last_read_message_id` on stream connect/disconnect in `streamChat`, avoiding per-message API calls during active streaming. - Uses `context.WithoutCancel` for the deferred disconnect write so the DB update succeeds even after the client disconnects. ## Frontend Changes - Bold title (`font-semibold`) for unread chats in the sidebar. - Small blue dot indicator next to the relative timestamp. - Suppresses unread indicator for the currently active chat via `isActive` from NavLink. ## Design Decisions - Only `assistant` messages count as unread — the user's own messages don't trigger the indicator. - No foreign key on `last_read_message_id` since messages can be deleted (via rollback/truncation) and the column is just a high-water mark. - Zero API calls during streaming: exactly 2 DB writes per stream session (connect + disconnect). - Unread state refreshes on chat list load and window focus. The `watchChats` WebSocket optimistically marks non-active chats as unread on `status_change` events, but does not carry a server-computed `has_unread` field. Navigating to a chat optimistically clears its unread indicator in the cache. |
||
|
Kyle Carberry
|
4ed9094305 |
perf(site): memoize chat rendering hot path (#23720)
Addresses chat page rendering performance. Profiling with React Profiler showed `AgentChat` actual render times of 20–31ms (exceeding the 16ms/60fps budget), with `StickyUserMessage` as the #1 component bottleneck at 35.7% of self time. ## Changes **Hoist `createComponents` to module scope** (`response.tsx`): Previously every `<Response>` instance called `createComponents()` per render, creating a fresh components map that forced Streamdown to discard its cached render tree. Now both light/dark variants are precomputed once at module scope. **Wrap `StickyUserMessage` in `memo()`** (`ConversationTimeline.tsx`): Profile-confirmed #1 bottleneck. Each instance carries IntersectionObserver + ResizeObserver + scroll handlers; skipping re-render avoids all that setup. **Wrap `ConversationTimeline` in `memo()`** (`ConversationTimeline.tsx`): Prevents cascade re-renders from the parent when props haven't changed. **Remove duplicate `buildSubagentTitles`** (`ConversationTimeline.tsx` → `AgentDetailContent.tsx`): Was computed in both `AgentDetailTimeline` and `ConversationTimeline`. Now computed once and passed as a prop. <details> <summary>Profiling data & analysis</summary> ### Profiler Metrics | Metric | Value | |--------|-------| | INP (Interaction to Next Paint) | 82ms | | Processing duration (event handlers) | 52ms | | AgentChat actual render | 20–31ms (budget: 16ms) | | AgentChat base render (no memo) | ~100ms | ### Top Bottleneck Components (self-time %) | Component | Self Time | % | |-----------|-----------|---| | StickyUserMessage | 11.0ms | 35.7% | | ForwardRef (radix-ui) | 7.4ms | 24.0% | | Presence (radix-ui) | 2.0ms | 6.5% | | AgentChatInput | 1.4ms | 4.5% | ### Decision log - Chose module-scope precomputation over `useMemo` for `createComponents` because there are only two possible theme variants and they're static. - Did not add virtualization — sticky user messages + scroll anchoring make it complex. The memoization fixes should be measured first. - Did not wrap `BlockList` in `memo()` — the React Compiler (enabled for `pages/AgentsPage/`) already auto-memoizes JSX elements inside it. - Phase 2 (verify React Compiler effectiveness on `parseMessagesWithMergedTools`) and Phase 3 (radix-ui Tooltip lazy-mounting) deferred to follow-up PRs. </details> |
||
|
Kyle Carberry
|
d973a709df |
feat: add model_intent option to MCP server configs (#23717)
Add a per-MCP-server `model_intent` toggle that wraps tool schemas with
a
`model_intent` field, requiring the LLM to provide a human-readable
description of each tool call's purpose. The intent string is shown as a
status label in the UI instead of opaque tool names, and is
transparently
stripped before the call reaches the remote MCP server.
Built-in tools have rich specialized renderers (terminal blocks, file
diffs,
etc.) and don't need this. MCP tools hit `GenericToolRenderer` which
only
shows raw tool names and JSON — that's where model_intent adds value.
The model learns what to provide via the JSON Schema `description` on
the
`model_intent` property itself — no system prompt changes needed.
<details>
<summary>Implementation details</summary>
### Architecture
Inspired by the `withModelIntent()` pattern from `coder/blink`, adapted
for
Go + React. The wrapping is entirely in the `mcpclient` layer — tool
implementations never see `model_intent`.
**Schema wrapping** (`mcpToolWrapper.Info()`): When enabled, wraps the
original tool parameters under a `properties` key and adds a
`model_intent`
string field with a rich description that teaches the model inline.
**Input unwrapping** (`mcpToolWrapper.Run()`): Strips `model_intent` and
unwraps `properties` before forwarding to the remote MCP server. Handles
three input shapes models may produce:
1. `{ model_intent, properties: {...} }` — correct format
2. `{ model_intent, key: val, ... }` — flat, no wrapper
3. Malformed — falls through gracefully
**Frontend extraction**: `streamState.ts` extracts `model_intent` from
incrementally parsed streaming JSON. `messageParsing.ts` extracts it
from
persisted tool call args.
**UI rendering**: `GenericToolRenderer` shows the capitalized intent
string
as the primary label when available, falling back to the raw tool name.
### Changes
- Database: `model_intent` boolean column on `mcp_server_configs`
- SDK: `ModelIntent` field on config/create/update types
- API: pass-through in create/update handlers + converter
- mcpclient: schema wrapping in `Info()`, input unwrapping in `Run()`
- Frontend: extraction from streaming + persisted args
- UI: intent label in `GenericToolRenderer`, toggle in admin panel
- Tests: 6 new tests (schema wrapping, unwrapping, passthrough,
fallback)
### Decision log
- **Option lives on MCPServerConfig, not model config**: Built-in tools
already have rich renderers; only MCP tools benefit from model_intent.
- **No system prompt changes**: The JSON Schema `description` on the
`model_intent` property teaches the model inline.
- **Pointer bool on update request**: Follows existing pattern (`*bool`)
so PATCH requests don't reset the value when omitted.
</details>
|
||
|
Kyle Carberry
|
50c0c89503 |
fix(coderd): refresh expired MCP OAuth2 tokens everywhere (#23713)
Fixes expired MCP OAuth2 tokens causing 401 errors and stale `auth_connected` status in the UI. When users authenticate MCP servers (e.g. GitHub) via OAuth2, the access token and refresh token are stored in the database. However, when the access token expired, nothing refreshed it anywhere: - **chatd**: sent the expired token as-is, getting a 401 and skipping the MCP server - **list/get endpoints**: reported `auth_connected: true` just because a token record existed, regardless of expiry ## Changes ### Shared utility: `mcpclient.RefreshOAuth2Token` Pure function that uses `golang.org/x/oauth2` `TokenSource` to check if a token is expired (or within 10s of expiry) and refresh it. No DB dependency — callers handle persistence. ### chatd (`coderd/x/chatd/chatd.go`) Before calling `mcpclient.ConnectAll`, refreshes expired tokens. Persists new credentials to the database. Falls back to the old token if refresh fails. ### List/get MCP server endpoints (`coderd/mcp.go`) Both `listMCPServerConfigs` and `getMCPServerConfig` now attempt refresh when checking `auth_connected`. If the token is expired: - **Has refresh token**: attempt refresh, persist result, report `auth_connected` based on success - **No refresh token**: report `auth_connected: false` if expired This means the UI accurately reflects whether the user's token is actually usable, rather than just whether a record exists. <details> <summary>Design notes</summary> - `RefreshOAuth2Token` lives in `mcpclient` to avoid circular imports (`coderd` → `chatd` → `mcpclient` is fine; `chatd` → `coderd` would be circular). - DB persistence is handled by each caller with their own authz context (`AsSystemRestricted` in both cases). - The `buildAuthHeaders` warning in mcpclient about expired tokens is kept as defense-in-depth logging. </details> |
||
|
Kyle Carberry
|
0f86c4237e |
feat: add workspace MCP tool discovery and proxying for chat (#23680)
Coder's chat (chatd) can now discover and use MCP servers configured in a workspace's `.mcp.json` file. This brings project-specific tooling (GitHub, databases, docs servers, etc.) into the chat without any manual configuration. ## How it works The workspace agent reads `.mcp.json` from the workspace directory (same format Claude Code uses), connects to the declared MCP servers — spawning child processes for stdio servers and connecting over the network for HTTP/SSE — and caches their tool lists. Two new agent HTTP endpoints expose this: - `GET /api/v0/mcp/tools` returns the cached tool list (supports `?refresh=true`) - `POST /api/v0/mcp/call-tool` proxies calls to the correct server On each chat turn, chatd calls `ListMCPTools` through the existing `AgentConn` tailnet connection, wraps each tool as a `fantasy.AgentTool`, and adds them to the LLM's tool set alongside built-in and admin-configured MCP tools. Tool names are prefixed with the server name (`github__create_issue`) to avoid collisions. Failed server connections are logged and skipped — they never block the agent or break the chat. Child stdio processes are terminated on agent shutdown. |
||
|
Kyle Carberry
|
61d2a4a9b8 |
fix(site): preserve streaming output when queued message is sent (#23595)
## Problem
When the user sends a message while the agent is actively streaming a
response, `handleSend` called `store.clearStreamState()`
**unconditionally before** the POST request. If the server queues the
message (`response.queued = true` because the agent is busy), the
in-progress stream output is immediately wiped from the UI. The full
text only reappears once the agent finishes and the durable message
arrives via WebSocket — causing a visible cutoff mid-stream.
## Fix
Move `clearStreamState()` from before the POST to **after** the
response, gated behind `!response.queued`:
- **Queued sends** (`response.queued === true`): `clearStreamState()` is
never called. The stream continues uninterrupted. The WebSocket `status`
handler already clears stream state when the chat transitions to
`"pending"` / `"waiting"` after the queued message is dequeued.
- **Non-queued sends** (`response.queued === false`):
`clearStreamState()` + `upsertDurableMessage()` fire immediately after
the POST, same net behavior as before.
- **Edit and promote paths**: Unchanged — those are intentional
interruptions where eager clearing is correct.
### Additional behavior changes (both improvements)
1. **Failed sends no longer wipe stream state.** Previously
`clearStreamState()` ran before the `try` block, so a network error
still wiped the agent's in-progress output. Now the `catch` re-throws
before reaching `clearStreamState()`, preserving the stream on failure.
2. **`clearStreamState()` fires for all non-queued responses**, not just
those with a `message` body. The original guard was `!response.queued &&
response.message`; now `clearStreamState()` is under `!response.queued`
while `upsertDurableMessage` retains the `response.message` check. The
server always sets `message` for non-queued responses, so this is a
no-op in practice but is semantically correct.
## Testing
**AgentDetail.stories.tsx**: New `StreamingSurvivesQueuedSend` story
exercises the full flow — mocks `createChatMessage` to return `{ queued:
true }`, delivers streaming text via WebSocket, sends a message through
the UI, and asserts the streaming text remains visible.
|
||
|
Kyle Carberry
|
09bcd0b260 |
fix: revert "refactor(site/src/pages/AgentsPage): normalize transcript scrolling" (#23638)
Reverts coder/coder#23576 |
||
|
Kyle Carberry
|
1f13324075 |
fix(coderd): use path-aware discovery for MCP OAuth2 metadata (RFC 9728, RFC 8414) (#23520)
## Problem
MCP OAuth2 auto-discovery stripped the path component from the MCP
server URL
before looking up Protected Resource Metadata. Per RFC 9728 §3.1, the
well-known
URL should be path-aware:
```
{origin}/.well-known/oauth-protected-resource{path}
```
For `https://api.githubcopilot.com/mcp/`, the correct metadata URL is
`https://api.githubcopilot.com/.well-known/oauth-protected-resource/mcp/`,
not
`https://api.githubcopilot.com/.well-known/oauth-protected-resource`
(which
returns 404).
The same issue applied to RFC 8414 Authorization Server Metadata for
issuers
with path components (e.g. `https://github.com/login/oauth` →
`/.well-known/oauth-authorization-server/login/oauth`).
## Fix
Replace the `mcp-go` `OAuthHandler`-based discovery with a
self-contained
implementation that correctly follows path-aware well-known URI
construction for
both RFC 9728 and RFC 8414, falling back to root-level URLs when the
path-aware
form returns an error. Also implements RFC 7591 registration directly,
removing
the `mcp-go/client/transport` dependency from the discovery path.
Note: this fix resolves the discovery half of the problem for servers
like
GitHub Copilot. Full OAuth2 support for GitHub's MCP server also
requires
dynamic client registration (RFC 7591), which GitHub's authorization
server
does not currently support — that will be addressed separately.
|
||
|
Kyle Carberry
|
c0f93583e4 |
fix(site): soften tool failure display and improve subagent timeout UX (#23617)
## Summary
Tool call failures in `/agents` previously displayed alarming red
styling (red icons, red text, red alert icons) that made it look like
the user did something wrong. This PR replaces the scary error
presentation with a calm, unified style and adds a dedicated timeout
display for subagent tools.
## Changes
### Unified failure style (all tools)
- Replace red `CircleAlertIcon` + `text-content-destructive` with a
muted `TriangleAlertIcon` in `text-content-secondary` across **all 11
tool renderers**.
- Remove red icon/label recoloring on error from `ToolIcon` and all
specialized tool components.
- Error details remain accessible via tooltip on hover.
### Subagent timeout display
- `ClockIcon` with "Timed out waiting for [Title]" instead of a generic
error display.
- `CircleXIcon` for non-timeout subagent errors with proper error verbs
("Failed to spawn", "Failed waiting for", etc.) instead of the
misleading running verb ("Waiting for").
- Timeout detection from result string/error field containing "timed
out".
### Title resolution for historical messages
- `ConversationTimeline` now computes `subagentTitles` via
`useMemo(buildSubagentTitles(...))` and passes it to historical
`ChatMessageItem` rendering, so `wait_agent` can resolve the actual
agent title from a prior `spawn_agent` result even outside streaming
mode.
### Stories
8 new stories: `GenericToolFailed`, `GenericToolFailedNoResult`,
`SubagentWaitTimedOut`, `SubagentWaitTimedOutWithTitle`,
`SubagentWaitTimedOutTitleFromMap`, `SubagentSpawnError`,
`SubagentWaitError`, `MCPToolFailedUnifiedStyle`.
## Files changed (15)
- `tool/Tool.tsx` — GenericToolRenderer + SubagentRenderer
- `tool/SubagentTool.tsx` — timeout/error verbs, icon changes
- `tool/ToolIcon.tsx` — remove destructive recoloring
- `tool/*.tsx` (10 specialized tools) — unified warning icon
- `ConversationTimeline.tsx` — pass subagentTitles to historical
rendering
- `tool.stories.tsx` — 8 new stories, updated existing assertions
|
||
|
Kyle Carberry
|
d4660d8a69 |
feat: add labels to chats (#23594)
## Summary
Adds a general-purpose `map[string]string` label system to chats, stored
as jsonb with a GIN index for efficient containment queries.
This is a standalone foundational feature that will be used by the
upcoming Automations feature for session identity (matching webhook
events to existing chats), replacing the need for bespoke session-key
tables.
## Changes
### Database
- **Migration 000451**: Adds `labels jsonb NOT NULL DEFAULT '{}'` column
to `chats` table with a GIN index (`idx_chats_labels`)
- **`InsertChat`**: Accepts labels on creation via `COALESCE(@labels,
'{}')`
- **`UpdateChatByID`**: Supports partial update —
`COALESCE(sqlc.narg('labels'), labels)` preserves existing labels when
NULL is passed
- **`GetChats`**: New `has_labels` filter using PostgreSQL `@>`
containment operator
- **`GetAuthorizedChats`**: Synced with generated `GetChats` (new column
scan + query param)
### API
- **Create chat** (`POST /chats`): Accepts optional `labels` field,
validated before creation
- **Update chat** (`PATCH /chats/{chat}`): Supports `labels` field for
atomic label replacement
- **List chats** (`GET /chats`): Supports `?label=key:value` query
parameters (multiple are AND-ed)
### SDK
- `Chat`, `CreateChatRequest`, `UpdateChatRequest`, `ListChatsOptions`
all gain `Labels` fields
- `UpdateChatRequest.Labels` is a pointer (`*map[string]string`) so
`nil` means "don't change" vs empty map means "clear all"
### Validation (`coderd/httpapi/labels.go`)
- Max 50 labels per chat
- Key: 1–64 chars, must match `[a-zA-Z0-9][a-zA-Z0-9._/-]*` (supports
namespaced keys like `github.repo`, `automation/pr-number`)
- Value: 1–256 chars
- 13 test cases covering all edge cases
### Chat runtime
- `chatd.CreateOptions` gains `Labels` field, threaded through to
`InsertChat`
- Existing `UpdateChatByID` callers (e.g., quickgen title updates) are
unaffected — NULL labels preserve existing values via COALESCE
|
||
|
Kyle Carberry
|
d9fc5a5be1 |
feat: persist chat instruction files as context-file message parts (#23592)
## Summary Introduces a new `context-file` ChatMessagePart type for persisting workspace instruction files (AGENTS.md) as durable, frontend-visible message parts. This is the foundation for showing loaded context files in the chat input's context indicator tooltip. ### Problem Previously, instruction files were resolved transiently on every turn via `resolveInstructions()` → `InsertSystem()` and injected into the in-memory prompt without persistence. The frontend had no knowledge that instruction files were loaded into context, and there was no way to surface this information to users. ### Solution Instruction files are now read **once** when a workspace is first attached to a chat (matching how [openai/codex handles it](https://developers.openai.com/codex/guides/agents-md)) and persisted as `user`-role, `both`-visibility message parts with a new `context-file` type. This ensures: - **Durability**: survives page refresh (data is in the DB, returned by `getChatMessages`) - **Cache-friendly**: `user`-role avoids the system-message hoisting that providers do, keeping the instruction content in a stable position for prompt caching - **Frontend-visible**: the frontend receives paths and truncation status for future context indicator rendering - **Extensible**: the same pattern works for Skills (future) ### Key changes | Layer | Change | |---|---| | **SDK** (`codersdk/chats.go`) | Add `ChatMessagePartTypeContextFile` with `context_file_path`, `context_file_content` (internal, stripped from API), `context_file_truncated` fields | | **Prompt expansion** (`chatprompt`) | Expand `context-file` parts to `<workspace-context>` text blocks in `partsToMessageParts()` | | **Chat engine** (`chatd.go`) | Add `persistInstructionFiles()`, called on first turn with a workspace. Remove per-turn `resolveInstructions()` + `InsertSystem()` from `processChat()` and `ReloadMessages` | | **Frontend** | Ignore `context-file` parts in `messageParsing.ts` and `streamState.ts` (no rendering yet — follow-up will add tooltip display) | ### How it works 1. On each turn, `processChat` checks if any loaded message contains `context-file` parts 2. If not (first turn with a workspace), reads AGENTS.md files via the workspace agent connection and persists them 3. For this first turn, also injects the instruction text into the prompt (since messages were loaded before persistence) 4. On all subsequent turns, `ConvertMessagesWithFiles()` encounters the persisted `context-file` parts and expands them into text automatically — no extra resolution needed |
||
|
Kyle Carberry
|
fdc9b3a7e4 |
fix: match text and image attachment heights in conversation timeline (#23593)
## Problem Text attachments (`InlineTextAttachmentButton`) and image thumbnails (`ImageThumbnail`) rendered at different heights when displayed side by side in user messages. Text cards had no explicit height (content-driven), while images used `h-16` (64px). ## Changes **`ConversationTimeline.tsx`** - Added `h-16` to `InlineTextAttachmentButton` to match `ImageThumbnail` - Added `isPlaceholder` prop: when the content hasn't been fetched yet (file_id path), renders "Pasted text" in sans-serif `text-sm` with `items-center` alignment instead of monospace `text-xs` - Once real content loads, it still renders in `font-mono text-xs` with `formatTextAttachmentPreview()` **`ConversationTimeline.stories.tsx`** - Added `UserMessageWithMixedAttachments` story showing a text attachment and image side by side as a visual regression guard |
||
|
Kyle Carberry
|
40395c6e32 |
fix(coderd): fast-retry PR discovery after git push (#23579)
## Problem
When chatd pushes a branch and then creates a PR (e.g. `git push`
followed by `gh pr create`), the gitsync background worker often picks
up the stale `chat_diff_statuses` row between the two operations. At
that point no PR exists yet, so the worker skips the row. However, the
acquisition SQL locks the row for **5 minutes** (crash-recovery
interval), creating a dead zone where the PR diff is invisible in the UI
until the user manually navigates to the chat.
### Root cause
1. `git push` triggers `GIT_ASKPASS` → coderd external-auth handler →
`MarkStale()` sets `stale_at = now - 1s`
2. Background worker acquires the row within ~10s, atomically bumps
`stale_at = NOW() + 5 min` (crash-recovery lock)
3. Worker calls `ResolveBranchPullRequest` → no PR exists yet → returns
`nil` → worker skips with `continue`
4. `gh pr create` completes moments later, but uses its own auth (not
`GIT_ASKPASS`), so no second `MarkStale` fires
5. Row is locked for 5 minutes before the worker can retry
Loading the chat works immediately because `GET /chats/{chat}` calls
`resolveChatDiffStatus` synchronously, which discovers the PR inline.
## Fix
When `ResolveBranchPullRequest` returns nil (no PR yet) **and** the row
was recently marked stale (within 2 minutes), apply a short 15-second
backoff via `BackoffChatDiffStatus` instead of letting the 5-minute
acquisition lock stand. Outside the retry window, the worker skips the
row as before — no indefinite fast-polling for branches that never
receive a PR.
To make the "recently marked stale" check work, `updated_at` is no
longer overwritten by the acquisition and backoff SQL queries. This
preserves it as a reliable "last externally changed" timestamp (set by
`MarkStale` or a successful refresh).
### Behavior summary
| Scenario | `updated_at` age | Backoff | Effective retry |
|---|---|---|---|
| Fresh push, no PR yet | < 2 min | 15s (`NoPRBackoff`) | ~15s |
| Old row, no PR | ≥ 2 min | None (skip) | ~5 min (acquisition lock) |
| Error (any age) | Any | 120s (`DiffStatusTTL`) | ~120s |
| Success (any age) | Any | 120s (`DiffStatusTTL`) | ~120s |
## Changes
- **`coderd/database/queries/chats.sql`** — Remove `updated_at = NOW()`
from `AcquireStaleChatDiffStatuses` and `BackoffChatDiffStatus`
- **`coderd/database/queries.sql.go`** — Regenerated
- **`coderd/x/gitsync/worker.go`** — Add `NoPRBackoff` (15s) and
`NoPRRetryWindow` (2 min) constants; apply short backoff only within the
retry window
- **`coderd/x/gitsync/worker_test.go`** — Add
`TestWorker_NoPR_RecentMarkStale_BacksOffShort` and
`TestWorker_NoPR_OldRow_Skips`
|
||
|
Kyle Carberry
|
44baac018a |
fix(site): replace model catalog loading text with skeleton (#23583)
## Changes Replaces the "Loading model catalog..." / "Loading models..." text flash on `/agents` with a clean skeleton loading state, and removes the admin-nag status messages entirely. ### Removed - `getModelCatalogStatusMessage()` function and `modelCatalogStatusMessage` prop chain — "Loading model catalog..." / "No chat models are configured. Ask an admin to configure one." text below the input - `inputStatusText` prop chain — "No models configured. Ask an admin." / "Models are configured but unavailable. Ask an admin." inline text - `modelCatalogError` prop from `AgentCreateForm` ### Changed - `AgentChatInput`: when `isModelCatalogLoading` is true, renders a `Skeleton` in place of the `ModelSelector` - `getModelSelectorPlaceholder()`: "No Models Configured" / "No Models Available" (title case) ### Added - `LoadingModelCatalog` story — skeleton where model selector sits - `NoModelsConfigured` story — selector shows "No Models Configured" Net -69 lines. |
||
|
Kyle Carberry
|
4ba9986301 |
fix(site): update sticky messages during streaming (#23577)
## Problem The sticky user message visual state (`--clip-h`, fade gradient, push-up positioning) is driven by an `update()` function that only ran on `scroll` events. The chat scroll container uses `flex-col-reverse`, where `scrollTop = 0` means "at bottom." When streaming content grows the transcript while the user is auto-scrolled to the bottom, `scrollTop` stays at `0` — no `scroll` event fires — so `update()` never runs and the sticky messages become visually stale until the user manually scrolls. ## Fix Add a `ResizeObserver` on the scroller's content wrapper inside the existing `useLayoutEffect` that sets up the scroll/resize listeners. When the content wrapper resizes (streaming growth), it fires the observer which calls `update()` through the same RAF-throttle pattern used by the scroll handler. Single observer per sticky message instance. Zero cost when nothing is resizing. Cleanup handled in the same effect teardown. |
||
|
Kyle Carberry
|
07dbee69df |
feat: collapse MCP tool results by default (#23568)
Wraps the `GenericToolRenderer` (used for MCP and unrecognized tools) in `ToolCollapsible` so the result content is hidden behind a click-to-expand chevron, matching the pattern used by `read_file`, `write_file`, and other built-in tool renderers. ### Changes - Move `ToolIcon` + `ToolLabel` into the `ToolCollapsible` `header` prop - Compute `hasContent` from `writeFileDiff` / `fileContent` / `resultOutput` — when there's no content, the header renders as a plain div with no chevron - Remove `ml-6` from `ScrollArea` classNames (the `ToolCollapsible` button handles its own layout) - `defaultExpanded` is `false` by default in `ToolCollapsible`, so results start collapsed ### Before MCP tool results were always fully visible inline. ### After MCP tool results are collapsed by default with a chevron toggle, consistent with `read_file`, `edit_files`, `list_templates`, etc. |
||
|
Kyle Carberry
|
f784b230ba |
fix(coderd/x/chatd/mcpclient): handle EmbeddedResource and ResourceLink in MCP tool results (#23569)
## Problem When an MCP tool returns an `EmbeddedResource` content item (e.g. GitHub MCP server returning file contents via `get_file_contents`), the `convertCallResult` function falls through to the `default` case, producing: ``` [unsupported content type: mcp.EmbeddedResource] ``` This loses the actual resource content and shows an unhelpful message in the chat UI. ## Root Cause The type switch in `convertCallResult` handles `TextContent`, `ImageContent`, and `AudioContent`, but not the other two `mcp.Content` implementations from `mcp-go`: - `mcp.EmbeddedResource` — wraps a `ResourceContents` (either `TextResourceContents` or `BlobResourceContents`) - `mcp.ResourceLink` — contains a URI, name, and description ## Fix Add two new cases to the type switch: 1. **`mcp.EmbeddedResource`**: nested type switch on `.Resource`: - `TextResourceContents` → append `.Text` to `textParts` - `BlobResourceContents` → base64-decode `.Blob` as binary (type `"image"` or `"media"` based on MIME) - Unknown → fallback `[unsupported embedded resource type: ...]` 2. **`mcp.ResourceLink`**: render as `[resource: Name (URI)]` text ## Testing Added 3 new test cases (all passing, full suite 23/23 PASS): - `TestConnectAll_EmbeddedResourceText` — text resource extraction - `TestConnectAll_EmbeddedResourceBlob` — binary blob decoding - `TestConnectAll_ResourceLink` — resource link rendering |
||
|
Kyle Carberry
|
6b105994c8 |
feat(site): persist MCP server selection in localStorage (#23572)
## Summary Previously the user's MCP server toggles were ephemeral — every page reload or navigation to a new chat reset them to the admin-configured defaults (`force_on` + `default_on`). This was frustrating for users who routinely disabled a default-on server or enabled a default-off one. This PR persists the MCP server picker selection to `localStorage` under the key `agents.selected-mcp-server-ids`. ## Changes ### `MCPServerPicker.tsx` - **`mcpSelectionStorageKey`** — exported constant for the localStorage key. - **`getSavedMCPSelection(servers)`** — reads from localStorage, filters out stale/disabled IDs, always includes `force_on` servers. - **`saveMCPSelection(ids)`** — writes the current selection to localStorage. ### `AgentCreateForm.tsx` - Initialises `userMCPServerIds` from `getSavedMCPSelection` instead of `null`. - Calls `saveMCPSelection` on every toggle. ### `AgentDetail.tsx` - Adds localStorage as a fallback tier in `effectiveMCPServerIds`: user override → chat record → **saved selection** → defaults. - Calls `saveMCPSelection` on every toggle. ### `MCPServerPicker.test.ts` (new) - 13 unit tests covering save, restore, stale-ID filtering, force_on merging, invalid JSON handling, and disabled server filtering. ## Fallback priority | Priority | Source | When | |----------|--------|------| | 1 | In-memory state | User toggled during this session | | 2 | Chat record | Existing conversation with `mcp_server_ids` | | 3 | localStorage | User has a saved selection from a prior session | | 4 | Server defaults | `force_on` + `default_on` servers | |
||
|
Kyle Carberry
|
894fcecfdc |
fix: inherit MCP server IDs from parent chat when spawning subagents (#23571)
Child chats created via `spawn_agent` and `spawn_computer_use_agent` were not inheriting the parent's `MCPServerIDs`, meaning subagents lost access to the parent's MCP server tools. ## Changes - Pass `parent.MCPServerIDs` in the `CreateOptions` for both `createChildSubagentChat()` and the `spawn_computer_use_agent` tool handler in `coderd/x/chatd/subagent.go`. ## Tests Added 3 tests in `subagent_internal_test.go`: - `TestCreateChildSubagentChat_InheritsMCPServerIDs` — verifies child chat gets parent's MCP server IDs (multiple servers) - `TestSpawnComputerUseAgent_InheritsMCPServerIDs` — verifies computer use subagent gets parent's MCP server IDs via the tool - `TestCreateChildSubagentChat_NoMCPServersStaysEmpty` — verifies no regression when parent has no MCP servers |
||
|
Kyle Carberry
|
3812b504fc | fix(coderd/x/chatd): prevent nil required field in MCP tool schemas for OpenAI (#23538) | ||
|
Kyle Carberry
|
dda985150d | feat: add MCP server config ID to tool-call message parts (#23522) | ||
|
Kyle Carberry
|
3495cad133 |
fix: resolve localhost URLs in markdown with correct port and protocol (#23513)
## Summary Fixes several bugs in the markdown URL transform that replaces `localhost` URLs with workspace port-forward URLs in the AI agent chat. ## Bugs Fixed ### 1. URLs without an explicit port produce `NaN` in the subdomain When an LLM outputs a URL like `http://localhost/path` (no port), `parsed.port` is the empty string `""`. `parseInt("", 10)` returns `NaN`, producing a broken URL like: ``` http://NaN--agent--workspace--user.proxy.example.com/path ``` Now defaults to port 80 for HTTP and 443 for HTTPS via the new `resolveLocalhostPort()` helper. ### 2. Protocol always hardcoded to `"http"` The `urlTransform` in `AgentDetail.tsx` always passed `"http"` as the protocol argument, silently discarding the original URL's scheme. This meant `https://localhost:8443/...` would not get the `s` suffix in the subdomain. Now extracts the protocol from the parsed URL, matching the existing behavior of `openMaybePortForwardedURL`. ### 3. `urlTransform` not memoized The closure was re-created on every render. Wrapped in `useCallback` with the four primitive dependencies (`proxyHost`, `agentName`, `wsName`, `wsOwner`). ### 4. Duplicated `localHosts` definition The localhost detection set was defined separately in both `AgentDetail.tsx` and `portForward.ts`. Consolidated into a single shared export from `portForward.ts`. ## Changes - **`site/src/utils/portForward.ts`**: Export shared `localHosts` set and new `resolveLocalhostPort()` helper. Update `openMaybePortForwardedURL` to use both. - **`site/src/pages/AgentsPage/AgentDetail.tsx`**: Import shared `localHosts` and `resolveLocalhostPort`. Fix protocol extraction. Memoize `urlTransform`. - **`site/src/utils/portForward.jest.ts`**: Add tests for `resolveLocalhostPort` and `localHosts`. Renamed from `.test.ts` to `.jest.ts` to match project convention. |
||
|
Kyle Carberry
|
e34162945a |
fix(coderd/x/chatd): normalize OAuth2 token type to canonical Bearer case (#23516)
Linear's MCP server (`mcp.linear.app`) returns `token_type="bearer"` (lowercase) in its OAuth2 token response but rejects requests that use the lowercase form in the `Authorization` header. RFC 6750 says the scheme is case-insensitive, but Linear enforces capital-B `Bearer`. Confirmed by running the actual Linear MCP OAuth flow end-to-end: - `Authorization: Bearer <token>` → **42 tools, works** - `Authorization: bearer <token>` → **401 invalid_token** This is a one-line fix: normalize any case variant of `bearer` to `Bearer` before building the `Authorization` header, matching the behavior of the mcp-go library's own OAuth handler. |
||
|
Kyle Carberry
|
e87ea1e0f5 |
fix(coderd): add PKCE support to MCP server OAuth2 flow (#23503)
## Problem
MCP servers like Linear (`mcp.linear.app`) require PKCE (RFC 7636) for
their OAuth2 flow. Without it, the token exchange may succeed but the
resulting access token is immediately rejected with a 401
`invalid_token` error when the chat daemon tries to connect to the MCP
server.
This means users can authenticate successfully in the UI (the OAuth
popup completes, `auth_connected` shows `true`), but the model never
receives the MCP tools — they silently fail to load.
### Root cause
The `mcpServerOAuth2Connect` handler was calling
`oauth2Config.AuthCodeURL(state)` without any PKCE parameters
(`code_challenge`, `code_challenge_method`). The callback was calling
`oauth2Config.Exchange(ctx, code)` without a `code_verifier`. Linear's
MCP OAuth endpoint decoded state confirms it expected PKCE with
`codeChallengeMethod: "plain"`.
### Investigation
- The chat (`c2c04fc5-5622-4b71-a5a9-80508e86f78e`) had the Linear MCP
server ID in `mcp_server_ids`
- `auth_connected: true` (token row exists in DB)
- No "expired" or "empty token" warnings in logs
- Server log showed: `skipping MCP server due to connection failure ...
error="initialize: transport error: request failed with status 401:
{"error":"invalid_token","error_description":"Missing or invalid access
token"}"`
- Decoding Linear's OAuth state revealed PKCE was expected
## Changes
- Generate a PKCE `code_verifier` during the OAuth2 connect step using
`oauth2.GenerateVerifier()` and store it in a cookie scoped to the
callback path
- Include `code_challenge` (S256) in the authorization redirect URL via
`oauth2.S256ChallengeOption()`
- Pass the `code_verifier` during the token exchange in the callback via
`oauth2.VerifierOption()`
- Fix a nil-pointer guard on `api.HTTPClient` in the callback
- Add tests verifying PKCE parameters are sent correctly and backwards
compatibility when no verifier cookie is present
|
||
|
Kyle Carberry
|
13241a58ba |
fix(coderd/x/chatd/mcpclient): use dedicated HTTP transport per MCP connection (#23494)
## Problem `TestConnectAll_MultipleServers` flakes with: ``` net/http: HTTP/1.x transport connection broken: http: CloseIdleConnections called ``` Each MCP client connection implicitly uses `http.DefaultTransport`. When `httptest.Server.Close()` runs during parallel test cleanup, it calls `CloseIdleConnections` on `http.DefaultTransport`, breaking in-flight connections from other goroutines or parallel tests sharing that transport. ## Fix Clone the default transport for each MCP connection via `http.DefaultTransport.(*http.Transport).Clone()`, passed through `WithHTTPBasicClient` (StreamableHTTP) and `WithHTTPClient` (SSE). This scopes idle connection cleanup to a single MCP server so it cannot disrupt unrelated connections. Fixes coder/internal#1420 |
||
|
Kyle Carberry
|
631e4449bb |
fix: use actual config ID in MCP OAuth2 redirect URI during auto-discovery (#23491)
## Problem
During OAuth2 auto-discovery for MCP servers, the callback URL
registered with the remote authorization server via Dynamic Client
Registration (RFC 7591) contained the literal string `{id}` instead of
the actual config UUID:
```
https://coder.example.com/api/experimental/mcp/servers/{id}/oauth2/callback
```
This happened because the discovery and registration occurred **before**
the database insert that generates the ID. When the user later initiated
the OAuth2 connect flow, the redirect URL used the real UUID, causing
the authorization server to reject it with:
> The provided redirect URIs are not approved for use by this
authorization server
## Fix
Restructure the auto-discovery flow in `createMCPServerConfig` to:
1. **Insert** the MCP server config first (with empty OAuth2 fields) to
get the database-generated UUID
2. **Build** the callback URL with the actual UUID
3. **Perform** OAuth2 discovery and dynamic client registration with the
correct URL
4. **Update** the record with the discovered OAuth2 credentials
5. **Clean up** the record if discovery fails
## Testing
Added regression test
`TestMCPServerConfigsOAuth2AutoDiscovery/RedirectURIContainsRealConfigID`
that:
- Stands up mock auth + MCP servers
- Captures the `redirect_uris` sent during dynamic client registration
- Asserts the URI contains the real config UUID, not `{id}`
- Verifies the full callback path structure
All existing MCP server config tests continue to pass.
|
||
|
Kyle Carberry
|
f62f2ffe6a |
feat(site): add MCP server picker to agent chat UI (#23470)
## Summary
Adds a user-facing MCP server configuration panel to the chat input
toolbar. Users can toggle which MCP servers provide tools for their chat
sessions, and authenticate with OAuth2 servers via popup windows.
## Changes
### New Components
- **`MCPServerPicker`** (`MCPServerPicker.tsx`): Popover-based picker
that appears in the chat input toolbar next to the model selector. Shows
all enabled MCP servers with toggles.
- **`MCPServerPicker.stories.tsx`**: 13 Storybook stories covering all
states.
### Availability Policies
Respects the admin-configured availability for each server:
- **`force_on`**: Always active, toggle disabled, lock icon shown. User
cannot disable.
- **`default_on`**: Pre-selected by default, user can opt out via
toggle.
- **`default_off`**: Not selected by default, user must opt in via
toggle.
### OAuth2 Authentication
For servers with `auth_type: "oauth2"`:
- Shows auth status (connected/not connected)
- "Connect to authenticate" link opens a popup window to
`/api/experimental/mcp/servers/{id}/oauth2/connect`
- Listens for `postMessage` with `{type: "mcp-oauth2-complete"}` from
the callback page
- Same UX pattern as external auth on the Create Workspace screen
### Integration Points
- `AgentChatInput`: MCP picker appears in the toolbar after the model
selector
- `AgentDetail`: Manages MCP selection state, initializes from
`chat.mcp_server_ids` or defaults
- `AgentDetailView` / `AgentDetailContent`: Props plumbed through to
input
- `AgentCreatePage` / `AgentCreateForm`: MCP selection for new chats
- `mcp_server_ids` now sent with `CreateChatMessageRequest` and
`CreateChatRequest`
### Helper
- `getDefaultMCPSelection()`: Computes default selection from
availability policies (`force_on` + `default_on`)
## Storybook Stories
| Story | Description |
|-------|-------------|
| NoServers | No servers - picker hidden |
| AllDisabled | All disabled servers - picker hidden |
| SingleForceOn | Force-on server with locked toggle |
| SingleDefaultOnNoAuth | Default-on with no auth required |
| SingleDefaultOff | Optional server not selected |
| OAuthNeedsAuth | OAuth2 server needing authentication |
| OAuthConnected | OAuth2 server already connected |
| MixedServers | Multiple servers with mixed availability/auth |
| AllConnected | All OAuth2 servers authenticated |
| Disabled | Picker in disabled state |
| WithDisabledServer | Disabled servers filtered out |
| AllOptedOut | All toggled off except force_on |
| OptionalOAuthNeedsAuth | Optional OAuth2 needing auth |
|
||
|
Kyle Carberry
|
acbfb90c30 |
feat: auto-discover OAuth2 config for MCP servers via RFC 7591 DCR (#23406)
## Problem When adding an external MCP server with `auth_type=oauth2`, admins currently must manually provide: - `oauth2_client_id` - `oauth2_client_secret` - `oauth2_auth_url` - `oauth2_token_url` This requires the admin to manually register an OAuth2 client with the external MCP server's authorization server first — a friction-heavy process that contradicts the MCP spec's vision of plug-and-play discovery. ## Solution When an admin creates an MCP server config with `auth_type=oauth2` and omits the OAuth2 fields, Coder now automatically discovers and registers credentials following the MCP authorization spec: 1. **Protected Resource Metadata (RFC 9728)** — Fetches `/.well-known/oauth-protected-resource` from the MCP server to discover its authorization server. Falls back to probing the server URL for a `WWW-Authenticate` header with a `resource_metadata` parameter. 2. **Authorization Server Metadata (RFC 8414)** — Fetches `/.well-known/oauth-authorization-server` from the discovered auth server to find all endpoints. 3. **Dynamic Client Registration (RFC 7591)** — Registers Coder as an OAuth2 client at the auth server's registration endpoint, obtaining a `client_id` and `client_secret` automatically. The discovered/generated credentials are stored in the MCP server config, and the existing per-user OAuth2 connect flow works unchanged. ### Backward compatibility - **Manual config still works**: If all three fields (`oauth2_client_id`, `oauth2_auth_url`, `oauth2_token_url`) are provided, the existing behavior is unchanged. - **Partial config is rejected**: Providing some but not all fields returns a clear error explaining the two options. - **Discovery failure is clear**: If auto-discovery fails, the error message explains what went wrong and suggests manual configuration. ## Changes - **New package `coderd/mcpauth`** — Self-contained discovery and DCR logic with no `codersdk` dependency - **Modified `coderd/mcp.go`** — `createMCPServerConfig` handler now attempts auto-discovery when OAuth2 fields are omitted - **Tests** — Unit tests for discovery (happy path, WWW-Authenticate fallback, no registration endpoint, registration failure) and `parseResourceMetadataParam` helper |
||
|
Kyle Carberry
|
e388a88592 |
feat(coderd/chatd): connect to external MCP servers for chat tool invocation (#23333)
## Summary
Adds a new `coderd/chatd/mcpclient` package that connects to
admin-configured MCP servers and wraps their tools as
`fantasy.AgentTool` values that the chat loop can invoke.
## What changed
### New: `coderd/chatd/mcpclient/mcpclient.go`
The core package with a single entry point:
```go
func ConnectAll(
ctx context.Context,
logger slog.Logger,
configs []database.MCPServerConfig,
tokens []database.MCPServerUserToken,
) (tools []fantasy.AgentTool, cleanup func(), err error)
```
This:
1. Connects to each enabled MCP server using `mark3labs/mcp-go`
(streamable HTTP or SSE transport)
2. Discovers tools via the MCP `tools/list` method
3. Wraps each tool as a `fantasy.AgentTool` with namespaced name
(`serverslug__toolname`)
4. Applies tool allow/deny list filtering from the server config
5. Handles auth: OAuth2 bearer tokens, API keys, and custom headers
6. Skips broken servers with a warning (10s connect timeout per server)
7. Returns a cleanup function to close all MCP connections
### Modified: `coderd/chatd/chatd.go`
In `runChat()`, after loading the model/messages but before assembling
the tool list:
- Reads `chat.MCPServerIDs` from the chat record
- Loads the MCP server configs from the database
- Resolves the user's auth tokens
- Calls `mcpclient.ConnectAll()` to connect and discover tools
- Appends the MCP tools to the chat's tool set
- Defers cleanup to close connections when the chat turn ends
The chat loop (`chatloop.Run`) already handles tools generically —
MCP-backed tools are invoked identically to built-in workspace tools. No
changes needed in `chatloop/`.
### New: `coderd/chatd/mcpclient/mcpclient_test.go`
10 tests covering:
- Tool discovery and namespacing
- Tool call forwarding and result conversion
- Allow/deny list filtering
- Connection failure handling (graceful skip)
- Multi-server support with correct prefixes
- OAuth2 auth header injection
- Disabled server skipping
- Invalid input handling
- Tool info parameter propagation
## Design decisions
- **Tool namespacing**: `slug__toolname` with double underscore
separator. Avoids collisions with tools containing single underscores.
Stripped when forwarding to `tools/call`.
- **Connection lifecycle**: Fresh connections per chat turn, closed via
`defer`. Matches the `turnWorkspaceContext` pattern.
- **Failure isolation**: Each server connects independently. A broken
server doesn't fail the chat — its tools are simply unavailable.
- **No chatloop changes**: The existing `[]fantasy.AgentTool` interface
is already fully generic.
## What's NOT in this PR (follow-ups)
- Frontend MCP server picker UI (selecting servers for a chat)
- System prompt additions describing available MCP tools
- Token refresh on expiry mid-chat
- The deprecated `aibridged` MCP proxy cleanup
|
||
|
Kyle Carberry
|
4da273ba3c | feat(site): add usage indicator to agents sidebar (#23307) | ||
|
Kyle Carberry
|
7db77bbefa |
feat(site): add MCP server admin UI (#23301)
This adds the UI but does not add it to the Settings sidebar. Until it's actually functional and usable (which will come in future PRs) it will remain hidden. Next step is wiring this up to chats and actually testing the full flow end-to-end, but we aren't there yet. |
||
|
Kyle Carberry
|
742694eb20 |
fix: filter empty text/reasoning parts before sending to LLM (#23284)
## Problem Anthropic rejects requests containing empty text content blocks with: ``` messages: text content blocks must be non-empty ``` Empty text parts (`""` or whitespace-only like `" "`) get persisted in the database when a stream sends `TextStart`/`TextEnd` with no `TextDelta` in between. On the next turn, these parts are loaded from the DB and sent to Anthropic, which rejects them. ## Fix Filter empty/whitespace-only text and reasoning parts at the two LLM dispatch boundaries, without modifying persistence (the raw record is preserved): - **`partsToMessageParts()`** in `chatprompt.go` — filters when converting persisted DB messages to fantasy message parts for LLM calls. This is the last gateway before the Anthropic provider creates `TextBlockParam` objects. - **`toResponseMessages()`** in `chatloop.go` — filters when building in-flight conversation messages between steps within a single turn. Note: `flushActiveState()` (the interruption path) already had this guard — the normal `TextEnd` streaming path did not, but since we're not changing persistence, the fix is applied at the dispatch layer. |
||
|
Kyle Carberry
|
86cb313765 |
fix: update fantasy to fix OpenAI reasoning replay with Store enabled (#23297)
## Problem When `Store: true` is set for OpenAI Responses API calls (the new default), multi-turn conversations with reasoning models fail on the second message: ``` stream response: bad request: Item 'rs_xxx' of type 'reasoning' was provided without its required following item. ``` The fantasy library was reconstructing full `OfReasoning` input items (with encrypted content and summary) when replaying assistant messages. The API cannot pair these reconstructed reasoning items with the output items that originally followed them because the output items are sent as plain `OfMessage` without server-side IDs. ## Fix Updates the fantasy dependency (`kylecarbs/fantasy@cj/go1.25`) to skip reasoning parts during conversation replay in `toResponsesPrompt`. With `Store` enabled, the API already has the reasoning persisted server-side — it doesn't need to be replayed in the input. Fantasy PR: https://github.com/charmbracelet/fantasy/pull/181 ## Testing Adds `TestOpenAIReasoningRoundTrip` integration test that: 1. Sends a query to `o4-mini` (reasoning model with `Store: true`) 2. Verifies reasoning content is persisted 3. Sends a follow-up message — this was the failing step 4. Verifies the follow-up completes successfully Requires `OPENAI_API_KEY` env var to run. |
||
|
Kyle Carberry
|
d8ff67fb68 |
feat: add MCP server configuration backend for chats (#23227)
## Summary
Adds the database schema, API endpoints, SDK types, and encryption
wrappers for admin-managed MCP (Model Context Protocol) server
configurations that chatd can consume. This is the backend foundation
for allowing external MCP tools (Sentry, Linear, GitHub, etc.) to be
used during AI chat sessions.
## Database
Two new tables:
- **`mcp_server_configs`**: Admin-managed server definitions with URL,
transport (Streamable HTTP / SSE), auth config (none / OAuth2 / API key
/ custom headers), tool allow/deny lists, and an availability policy
(`force_on` / `default_on` / `default_off`). Includes CHECK constraints
on transport, auth_type, and availability values.
- **`mcp_server_user_tokens`**: Per-user OAuth2 tokens for servers
requiring individual authentication. Cascades on user/config deletion.
New column on `chats` table:
- **`mcp_server_ids UUID[]`**: Per-chat MCP server selection, following
the same pattern as `model_config_id` — passed at chat creation,
changeable per-message with nil-means-no-change semantics.
## API Endpoints
All routes are under `/api/experimental/mcp/servers/` and gated behind
the `agents` experiment.
**Admin endpoints** (`ResourceDeploymentConfig` auth):
- `POST /` — Create MCP server config
- `PATCH /{id}` — Update MCP server config (full-replace)
- `DELETE /{id}` — Delete MCP server config
**Authenticated endpoints** (all users, enabled servers only for
non-admins):
- `GET /` — List configs (admins see all, members see enabled-only with
admin fields redacted)
- `GET /{id}` — Get config by ID (with `auth_connected` populated
per-user)
**OAuth2 per-user auth flow:**
- `GET /{id}/oauth2/connect` — Initiate OAuth2 flow (state cookie CSRF
protection)
- `GET /{id}/oauth2/callback` — Handle OAuth2 callback, store tokens
- `DELETE /{id}/oauth2/disconnect` — Remove stored OAuth2 tokens
## Security
- **Secrets never returned**: `OAuth2ClientSecret`, `APIKeyValue`, and
`CustomHeaders` are never in API responses — only boolean indicators
(`has_oauth2_secret`, `has_api_key`, `has_custom_headers`).
- **Field redaction for non-admins**: `convertMCPServerConfigRedacted`
strips `OAuth2ClientID`, auth URLs, scopes, and `APIKeyHeader` from
non-admin responses.
- **dbcrypt encryption at rest**: All 5 secret fields use `dbcrypt_keys`
encryption with full encrypt-on-write / decrypt-on-read wrappers (11
dbcrypt method overrides + 2 helpers), following the same pattern as
`chat_providers.api_key`.
- **OAuth2 CSRF protection**: State parameter stored in `HttpOnly`
cookie with `HTTPCookies.Apply()` for correct `Secure`/`SameSite` behind
TLS-terminating proxies.
- **dbauthz authorization**: All 18 querier methods have authorization
wrappers. Read operations use `ActionRead`, write operations use
`ActionUpdate` on `ResourceDeploymentConfig`.
## Governance Model
| Control | Implementation |
|---------|---------------|
| **Global kill switch** | `enabled` defaults to `false` |
| **Availability policy** | `force_on` (always injected), `default_on`
(pre-selected), `default_off` (opt-in) |
| **Per-chat selection** | `mcp_server_ids` on `CreateChatRequest` /
`CreateChatMessageRequest` |
| **Auth gate** | OAuth2 servers require per-user auth before tools are
injected |
| **Tool-level allow/deny** | Arrays on `mcp_server_configs` for
granular tool filtering |
| **Secrets encrypted at rest** | Uses `dbcrypt_keys` (same pattern as
`chat_providers.api_key`) |
## Tests
8 test functions covering:
- Full CRUD lifecycle (create, list, update, delete)
- Non-admin visibility filtering (enabled-only, field redaction)
- `auth_connected` population for OAuth2 vs non-OAuth2 servers
- Availability policy validation (valid values + invalid rejection)
- Unique slug enforcement (409 Conflict)
- OAuth2 disconnect idempotency
- Chat creation with `mcp_server_ids` persistence
## Known Limitations (Deferred)
These are documented and intentional for an experimental feature:
- **Audit logging** not yet wired — will add when feature stabilizes
- **Cross-field validation** (e.g., OAuth2 fields required when
`auth_type=oauth2`) — admin-only endpoint, will add when stabilizing
- **`force_on` auto-injection** — query exists but not yet wired into
chatd tool injection (follow-up)
- **Additional test coverage** — 403 auth tests, GET-by-ID tests,
callback CSRF tests planned for follow-up
## What's NOT in this PR
- Frontend UI (admin panel + chat picker)
- Actual MCP client connections (`chatd/chatmcp/` manager)
- Tool injection into `chatloop/`
|
||
|
Kyle Carberry
|
fdc2366227 |
chore: update fantasy dep to rebased cj/go1.25 branch (#23242)
Updates the `charm.land/fantasy` replace to the rebased `cj/go1.25` branch on `kylecarbs/fantasy`, which now includes: - **chore: downgrade to Go 1.25** - **feat: anthropic computer use** - **chore: use kylecarbs/openai-go fork for coder/coder compat** Switches the `openai-go/v3` replace from `SasSwart/openai-go` → `kylecarbs/openai-go`, which is the same SasSwart perf fork plus a fix for `WithJSONSet` being clobbered by deferred body serialization. Without the fix, `NewStreaming` silently drops `stream: true` from requests. See https://github.com/kylecarbs/openai-go/pull/2 for details. |
||
|
Kyle Carberry
|
2245612ece | fix(site): fix browser back navigation between agents settings pages (#23254) | ||
|
Kyle Carberry
|
147d627505 |
fix: deduplicate PR insights, fix cost computation, simplify UI (#23251)
## Problem The `/agents/settings/insights` page had several issues: 1. **Duplicate PRs** in "Recent Pull Requests" — multiple chats referencing the same PR URL each produced a row 2. **Wildly wrong costs** — the cost subquery summed ALL messages across the entire chat *tree* (`GROUP BY root_chat_id`), so every chat in a tree got the same inflated total. When aggregated, the same tree cost was counted N× per PR in that tree 3. **UI clutter** — too many stat cards, too many table columns, mixed naming conventions ## Fix ### Backend (SQL) - **Deduplicate by PR URL** using `DISTINCT ON (COALESCE(cds.url, c.id::text))` across all 4 queries - **Fix cost computation**: use two CTEs — `pr_costs` sums cost from ALL chats that reference a PR (so review chats contribute), `deduped` picks one row per PR for state/additions/deletions via DISTINCT ON - **Tests**: 3 subtests covering multi-chat cost summing, different PRs no duplication, and duplicate URL counted once ### Frontend - **3 stat cards** (down from 5): Merged, Merge rate, Cost / merge - **2-line chart** (down from 3): created (dashed) + merged (solid) - **4-column model table** (down from 7): Model, Merged, Merge rate, Cost/merge - **4-column recent table** (down from 7): Title, Status, Cost, Created — with `table-fixed` to prevent overflow - **Consistent naming**: no mixed PR/PRs abbreviation, contextual labels since page title establishes context |
||
|
Kyle Carberry
|
1f0d896fc9 |
feat: add deleted flag to chat messages for soft-delete (#23223)
Adds a `deleted` boolean column to the `chat_messages` table. Messages are never physically deleted from the database — instead they are marked as deleted so that usage and cost data is preserved. ## Changes ### Migration - New migration (000444) adds `deleted boolean NOT NULL DEFAULT false` to `chat_messages` ### SQL queries - `DeleteChatMessagesAfterID` → `SoftDeleteChatMessagesAfterID` (UPDATE SET deleted=true instead of DELETE) - New `SoftDeleteChatMessageByID` query for single-message soft-delete - All read queries now filter `deleted = false`: - `GetChatMessageByID` - `GetChatMessagesByChatID` - `GetChatMessagesByChatIDDescPaginated` - `GetChatMessagesForPromptByChatID` (both CTE and main query) - `GetLastChatMessageByRole` - Cost/usage queries (`GetChatCostSummary`, `GetChatCostPerModel`, etc.) intentionally still include deleted messages to preserve accurate spend tracking ### EditMessage behavior - Previously: updated the message content in-place + hard-deleted subsequent messages - Now: soft-deletes the original message + soft-deletes subsequent messages + inserts a new message with the updated content - This preserves the original message data (tokens, cost, content) in the database |
||
|
Kyle Carberry
|
cbe29e4e25 |
fix: encode non-ASCII filenames in chat file upload header (#23241)
## Problem Uploading a file on the `/agents` chat page fails with: ``` Failed to execute 'setRequestHeader' on 'XMLHttpRequest': String contains non ISO-8859-1 code point. ``` This happens when the image filename contains non-ASCII characters (e.g. CJK characters from macOS screenshots like `スクリーンショット.png`, accented characters, emoji, etc.). HTTP headers only support ISO-8859-1 code points, and the filename was being interpolated directly into the `Content-Disposition` header. ## Fix Use [RFC 5987](https://datatracker.ietf.org/doc/html/rfc5987) `filename*=UTF-8''` encoding so the percent-encoded name is always valid in the header. A static ASCII `filename="file"` fallback is included for older clients. The server already uses Go's `mime.ParseMediaType` which decodes `filename*` automatically, so no backend changes are needed. ### Before ```ts "Content-Disposition": `attachment; filename="${file.name}"` ``` ### After ```ts "Content-Disposition": `attachment; filename="file"; filename*=UTF-8''${encodeURIComponent(file.name)}` ``` ## Testing Added a server-side test (`TestGetChatFile/UnicodeFilename`) that uploads with a Japanese filename and verifies it round-trips correctly through the `Content-Disposition` header. |
||
|
Kyle Carberry
|
90cf4f0a91 |
refactor: consolidate chat streaming endpoints under /stream (#23248)
Moves per-chat streaming/watch endpoints under a `/stream` sub-route for
better API consistency:
| Before | After |
|--------|-------|
| `GET /{chat}/stream` | `GET /{chat}/stream/` |
| `GET /{chat}/desktop` | `GET /{chat}/stream/desktop` |
| `GET /{chat}/git/watch` | `GET /{chat}/stream/git` |
### Changes
- **`coderd/coderd.go`** — Route definitions: replaced flat routes with
`r.Route("/stream", ...)` sub-router
- **`site/src/api/api.ts`** — Updated WebSocket URLs for `watchChatGit`
and `watchChatDesktop`
- **`coderd/chats_test.go`** — Updated desktop test URL
- **`coderd/workspaceagents_internal_test.go`** — Updated git watcher
test URLs (route mounts + dial URLs)
- **`site/src/pages/AgentsPage/AgentDetail.stories.tsx`** — Updated
storybook WebSocket mock paths
|
||
|
Kyle CarberryandCian Johnston
|
d4a072b61e |
fix: address review comments on InsertChatMessages (#23239)
Follow-up to #23220, addressing Cian's review comments: - **SQL casing**: Uppercase `UNNEST` to match `NULLIF`/`COALESCE` convention in the query. - **Builder pattern**: `chatMessage` struct now uses unexported fields with a `newChatMessage` constructor for required fields (role, content, visibility, modelConfigID, contentVersion) and chainable builder methods (`withCreatedBy`, `withCompressed`, `withUsage`, `withContextLimit`, `withTotalCostMicros`, `withRuntimeMs`) for optional/nullable fields. - **Batch test in chats_test**: Replaced the `for i := 0; i < 2` loop with a single batch insert of 2 messages to actually exercise the batch logic. - **Multi-message querier test**: Added `BatchInsertMultipleMessages` test verifying 3-message batch insert with role ordering, sequential IDs, nullable field semantics (NULL for zero UUIDs and zero ints), and token/cost assertions. --------- Co-authored-by: Cian Johnston <cian@coder.com> |
||
|
Kyle Carberry
|
2577d16af2 |
fix(site): use correct /api/experimental endpoint for PR insights (#23235)
## Problem
The `/agents/settings/insights` page was broken because
`InsightsContent` was calling `/api/v2/chats/insights/pull-requests`,
but the backend route is registered under
`/api/experimental/chats/insights/pull-requests` (the entire `/chats`
route block lives under `r.Route("/api/experimental", ...)` in
`coderd.go`).
Every other chat endpoint in the frontend correctly uses
`/api/experimental/chats/...`, but this one was missed.
## Fix
- Added `getPRInsights` method to the API client (`api.ts`) pointing to
`/api/experimental/chats/insights/pull-requests`
- Added a `prInsights` react-query helper in `api/queries/chats.ts`
(matching the pattern of `chatCostUsers`, etc.)
- Updated `InsightsContent.tsx` to use the query helper instead of a raw
`fetch()` with the wrong URL
|
||
|
Kyle Carberry
|
483adc59fe |
feat: replace InsertChatMessage with batch InsertChatMessages (#23220)
Replaces the singular `InsertChatMessage` query with `InsertChatMessages` that uses PostgreSQL's `unnest()` for batch inserts. This reduces the number of database round-trips when inserting multiple messages in a single transaction. ## Changes - **SQL**: New `InsertChatMessages :many` query using `unnest()` arrays following the existing codebase pattern (e.g., `InsertWorkspaceAgentStats`). Preserves the CTE that updates `chats.last_model_config_id` using the last non-null model config from the batch. Uses `NULLIF` for UUID columns to handle NULL foreign keys. - **Go layers**: Updated `querier.go`, `dbauthz.go`, `dbmetrics/querymetrics.go`, `dbmock/dbmock.go`, and `queries.sql.go` to use the new batch signature (`[]ChatMessage` return type, array params). - **chatd.go**: All call sites converted to batch inserts: - **CreateChat**: System prompt + user message batched into one call - **persistStep**: Assistant message + tool messages batched into one call - **persistSummary**: Hidden summary + assistant + tool messages batched into one call - Single-message sites use the same API with single-element arrays - **Helper**: New `appendChatMessage` function simplifies building batch params at each call site. - **Tests**: All test files updated to use the new API. Builds on top of #23213. |
||
|
Kyle Carberry
|
a130a7dc97 |
fix: renumber duplicate migration 000444 to 000445 (#23229)
Two migrations were merged with the same number 000444: - `000444_usage_events_ai_seats` (#22689, merged first at 09:30) — keeps 000444 - `000444_chat_message_runtime_ms` (#23219, merged second at 10:57) — renumbered to **000445** This collision causes `golang-migrate` to fail at runtime since it reads both files as the same version. **Fix:** Rename `000444_chat_message_runtime_ms.{up,down}.sql` → `000445_chat_message_runtime_ms.{up,down}.sql`. Closes https://github.com/coder/internal/issues/1411 |
||
|
Kyle Carberry
|
d6fef96d72 |
feat: add PR insights analytics dashboard (#23215)
## What Adds a new admin-only **PR Insights** page for the `/agents` analytics view — a dashboard for engineering leaders to understand code shipped by AI agents. ### Backend - `GET /api/v2/chats/insights/pull-requests` — admin-only endpoint - 4 SQL queries in `chatinsights.sql` aggregating `chat_diff_statuses` joined with chat cost data (via root chat tree rollup) - Runs 5 parallel DB queries: current summary, previous summary (for trends), time series, per-model breakdown, recent PRs - SDK types auto-generate to TypeScript ### Frontend (`PRInsightsView`) - **Stat cards**: PRs created, Merged, Merge rate, Lines shipped, Cost/merged PR — with trend badges comparing to previous period - **Activity chart**: Stacked area chart (created/merged/closed) using git color tokens (`git-added-bright`, `git-merged-bright`, `git-deleted-bright`) - **Model performance table**: Per-model PR counts, inline merge rate bars, diff stats, cost breakdown - **Recent PRs table**: Status badges, review state icons, author info, external links - **Time range filter**: 7d/14d/30d/90d button group - **4 Storybook stories**: Default, HighPerformance, LowVolume, NoPRs ### Data source All PR data comes from the existing `chat_diff_statuses` table (populated by the `gitsync.Worker` background job that polls GitHub every 120s). No new data collection required. ### Screenshot View in Storybook: `pages/AgentsPage/PRInsightsView` |
||
|
Kyle Carberry
|
4dd8531f37 |
feat: track step runtime_ms on chat messages (#23219)
## Summary Adds a `runtime_ms` column to `chat_messages` that records the wall-clock duration (in milliseconds) of each LLM step. This covers LLM streaming, tool execution, and retries — the full time the agent is "alive" for a step. This is the foundation for billing by agent alive time. The column follows the same pattern as `total_cost_micros`: stored per assistant message, aggregatable with `SUM()` over time periods by user. ## Changes - **Migration**: adds nullable `runtime_ms bigint` to `chat_messages`. - **chatloop**: adds `Runtime time.Duration` field to `PersistedStep`, measures `time.Since(stepStart)` at the beginning of each step (covering stream + tool execution + retries). - **chatd**: passes `step.Runtime.Milliseconds()` to the assistant message `InsertChatMessage` call; all other message types (system, user, tool) get `NULL`. - **Tests**: adds `runtime > 0` assertion in chatloop tests. ## Billing query pattern Once ready, aggregation mirrors the existing cost queries: ```sql SELECT COALESCE(SUM(cm.runtime_ms), 0)::bigint AS total_runtime_ms FROM chat_messages cm JOIN chats c ON c.id = cm.chat_id WHERE c.owner_id = @user_id AND cm.created_at >= @start_time AND cm.created_at < @end_time AND cm.runtime_ms IS NOT NULL; ``` |
||
|
Kyle Carberry
|
b83b93ea5c |
feat: add workspace awareness system message on chat creation (#23213)
When a chat is created via `chatd`, a system message is now inserted informing the model whether the chat was created with or without a workspace. **With workspace:** > This chat is attached to a workspace. You can use workspace tools like execute, read_file, write_file, etc. **Without workspace:** > There is no workspace associated with this chat yet. Create one using the create_workspace tool before using workspace tools like execute, read_file, write_file, etc. This is a model-only visibility system message (not shown to users) that helps the model understand its available capabilities upfront — particularly important for subagents spawned without a workspace, which previously would attempt to use workspace tools and fail. **Changes:** - `coderd/chatd/chatd.go`: Added workspace awareness constants and inserted the system message in `CreateChat` after the system prompt, before the initial user message. - `coderd/chatd/chatd_test.go`: Added `TestCreateChatInsertsWorkspaceAwarenessMessage` with sub-tests for both with-workspace and without-workspace cases. |
||
|
Kyle Carberry
|
d42008e93d |
fix: persist partial assistant response when chat is interrupted mid-stream (#23193)
## Problem
When a user cancels a streaming chat response mid-stream, the partial
content disappears entirely — both from the UI and the database. The
streamed text vanishes as if the response never happened.
## Root Causes
Three issues combine to prevent partial message persistence on
interrupt:
### 1. StreamPartTypeError only matched `context.Canceled`
(`chatloop.go`)
The interrupt detection in `processStepStream` checked:
```go
errors.Is(part.Error, context.Canceled) && errors.Is(context.Cause(ctx), ErrInterrupted)
```
But some providers propagate `ErrInterrupted` directly as the stream
error rather than wrapping it in `context.Canceled`. This caused the
condition to fail, so `flushActiveState` was never called and partial
text accumulated in `activeTextContent` was lost.
### 2. No post-loop interrupt check (`chatloop.go`)
If the stream iterator stops yielding parts without producing a
`StreamPartTypeError` (e.g., a provider that silently closes the
response body on cancel), there was no check after the `for part :=
range stream` loop to detect the interrupt and flush active state.
### 3. Worker ownership check blocked interrupted persists (`chatd.go`)
`InterruptChat` → `setChatWaiting` clears `worker_id` in the DB
**before** the chatloop detects the interrupt. When
`persistInterruptedStep` (using `context.WithoutCancel`) tried to write
the partial message, the ownership check:
```go
if !lockedChat.WorkerID.Valid || lockedChat.WorkerID.UUID != p.workerID {
return chatloop.ErrInterrupted // always blocks!
}
```
unconditionally rejected the write. The error was silently logged as a
warning.
## Fix
- **Broaden the `StreamPartTypeError` interrupt detection** to match
both `context.Canceled` and `ErrInterrupted` as the stream error.
- **Add a post-loop interrupt check** in `processStepStream` that
flushes active state when the context was canceled with
`ErrInterrupted`.
- **Allow `persistStep` to write when the chat is in `waiting` status**
(interrupt) even if `worker_id` was cleared. The `pending` status (from
`EditMessage`, where history is truncated) still correctly blocks stale
writes.
## Testing
Added `TestInterruptChatPersistsPartialResponse` — an end-to-end
integration test that:
1. Streams partial text chunks from a mock LLM
2. Waits for the chatloop to publish `message_part` events (confirming
chunks were processed)
3. Interrupts the chat mid-stream
4. Verifies the partial assistant message is persisted in the database
with the expected text content
|
||
|
Kyle Carberry
|
6fc9f195f1 |
fix: resolve chat message pagination scroll issues (#23169)
## Summary Fixes four interrelated issues that caused scroll position jumps and phantom scroll growth when paginating older chat messages. ## Changes ### 1. Removed client-side message windowing (`useMessageWindow`) There were two competing sentinel systems: server-side pagination and client-side windowing. The client windowing sentinel was nested deep inside the timeline with no explicit IntersectionObserver `root`, causing scroll position jumps when messages were prepended. Blink (coder/blink) has no client-side windowing. Removed it entirely; server pagination + `contentVisibility` handled performance. ### 2. Removed `contentVisibility: "auto"` from message sections Each section had `contentVisibility: "auto"` with `containIntrinsicSize: "1px 600px"`, causing the scroll region to grow/shrink as the browser swapped 600px placeholders for actual heights while scrolling. This created phantom scroll growth with no fetch involved. ### 3. Gated WebSocket on initial REST data The WebSocket `Subscribe` snapshot calls `GetChatMessagesByChatID` (no LIMIT) which returns every message when `afterMessageID` is 0. The WebSocket effect opened before the REST page resolved, so `lastMessageIdRef` was undefined, causing the server to replay the entire history and defeating pagination. Added `initialDataLoaded` guard so the socket waits for the first REST page. ### 4. Manual scroll position restoration Replaced unreliable CSS scroll anchoring in `flex-col-reverse` with a `ScrollAnchoredContainer` that snapshots `scrollHeight` before fetch and restores `scrollTop` via `useLayoutEffect` after render. Disabled browser scroll anchoring (`overflow-anchor: none`) to prevent conflicts. |
||
|
Kyle Carberry
|
497e1e6589 |
feat: render file references inline in user messages (#23174)
File references in user messages now render as inline chips (matching the chat input style) instead of in a separate bordered section at the bottom of the message bubble. This reimplements #23131 which was accidentally reverted during the merge of #23072 (the spend-limit UI PR resolved a merge conflict by dropping the inline chip logic). ## Changes - **FileReferenceNode.tsx**: Export `FileReferenceChip` so it can be imported for read-only use (no remove button when `onRemove` is omitted). - **ConversationTimeline.tsx**: Iterate through `parsed.blocks` in document order, rendering `response` blocks as text and `file-reference` blocks as inline `FileReferenceChip` components. Removes the old separated file-reference section with `border-t` divider. - **ConversationTimeline.stories.tsx**: Added `UserMessageWithInlineFileRef` and `UserMessageWithMultipleInlineFileRefs` stories. |
||
|
Kyle Carberry
|
b779c9ee33 |
fix: use SQL-level auth filtering for chat listing (#23159)
## Problem The chat listing endpoint (`GetChatsByOwnerID`) was using `fetchWithPostFilter`, which fetches N rows from the database and then filters them in Go memory using RBAC checks. This causes a pagination bug: if the user requests `limit=25` but some rows fail the auth check, fewer than 25 rows are returned even though more authorized rows exist in the database. The client may incorrectly assume it has reached the end of the list. ## Solution Switch to the same pattern used by `GetWorkspaces`, `GetTemplates`, and `GetUsers`: `prepareSQLFilter` + `GetAuthorized*` variant. The RBAC filter is compiled to a SQL WHERE clause and injected into the query before `ORDER BY`/`LIMIT`, so the database returns exactly the requested number of authorized rows. Additionally, `GetChatsByOwnerID` is renamed to `GetChats` with `OwnerID` as an optional (nullable) filter parameter, matching the `GetWorkspaces` naming convention. ## Changes | File | Change | |------|--------| | `queries/chats.sql` | Renamed to `GetChats`, `owner_id` now optional via CASE/NULL, added `-- @authorize_filter` | | `queries.sql.go` | Renamed constant, params struct (`GetChatsParams`), and method | | `querier.go` | Interface method renamed | | `modelqueries.go` | Added `chatQuerier` interface + `GetAuthorizedChats` impl | | `dbauthz/dbauthz.go` | `GetChats` now uses `prepareSQLFilter` instead of `fetchWithPostFilter` | | `dbauthz/dbauthz_test.go` | Updated tests for SQL filter pattern | | `dbmock/dbmock.go` | Renamed + added mock for `GetAuthorizedChats` | | `dbmetrics/querymetrics.go` | Renamed + added metrics wrapper | | `rbac/regosql/configs.go` | Added `ChatConverter` (maps `org_owner` to empty string literal since `chats` has no `organization_id` column) | | `rbac/authz.go` | Added `ConfigChats()` | | `chats.go` | Handler uses renamed method with `uuid.NullUUID` | | `searchquery/search.go` | Updated return type | | `gitsync/worker.go` | Updated interface and call site | | Various test files | Updated for renamed types | |
||
|
Kyle Carberry
|
a40716b6fe |
fix(site): stop spamming chats list endpoint on diff_status_change events (#23167)
## Problem
The WebSocket handler for `diff_status_change` events in
`AgentsPage.tsx` was triggering a burst of redundant HTTP requests on
every event:
1. **`invalidateChatListQueries(queryClient)`** — Full refetch of the
chats list endpoint. Unnecessary because `updateInfiniteChatsCache`
already writes `diff_status` into the sidebar cache optimistically on
every event.
2. **`invalidateQueries({ queryKey: chatKey(id) })`** — Refetch of the
individual chat. Also unnecessary — the SSE event carries `diff_status`
in its payload and the optimistic updater writes it into the `chatKey`
cache directly. Worse, this call was missing `exact: true`, so TanStack
Query's prefix matching cascaded the invalidation to `chatMessagesKey`,
`chatDiffContentsKey`, and every other query under `["chats", id]`.
Since diff status changes fire frequently during active agent work, this
spammed the chats list endpoint and caused redundant refetches of
messages and diff contents on every single event.
## Fix
Strip the handler down to the one invalidation that's actually needed —
`chatDiffContentsKey` (the file-level diff contents aren't in the SSE
payload):
```typescript
if (chatEvent.kind === "diff_status_change") {
void queryClient.invalidateQueries({
queryKey: chatDiffContentsKey(updatedChat.id),
exact: true,
});
}
```
## Why tests didn't catch this
The existing tests in `chats.test.ts` cover query utilities in isolation
(e.g. `invalidateChatListQueries` scoping, mutation invalidation). The
WebSocket event handler lives in the `AgentsPage` component — there was
no test covering what the `diff_status_change` code path actually
invalidates.
Added regression tests verifying that `exact: true` prevents
prefix-match cascade vs the old behavior.
|
||
|
Kyle Carberry
|
075dfecd12 |
refactor: consolidate experimental chats API types (#23143)
## Summary
Consolidates three areas of type duplication in the experimental chats
API:
### 1. Merge archive/unarchive into `PATCH /{chat}`
- **Before:** `POST /{chat}/archive` + `POST /{chat}/unarchive` (two
endpoints, two handlers with mirrored logic)
- **After:** `PATCH /{chat}` accepting `{ "archived": true/false }` via
`UpdateChatRequest`
- Removes one endpoint and ~30 lines of duplicated handler code
### 2. Collapse identical request/response prompt types
- `ChatSystemPromptResponse` + `UpdateChatSystemPromptRequest` →
`ChatSystemPrompt`
- `UserChatCustomPromptResponse` + `UpdateUserChatCustomPromptRequest` →
`UserChatCustomPrompt`
- These pairs were field-for-field identical (single string field)
### 3. Merge duplicate reasoning options types
- `ChatModelOpenRouterReasoningOptions` +
`ChatModelVercelReasoningOptions` → `ChatModelReasoningOptions`
- Same 4 fields, same types — only field ordering and enum value sets
differed
- Unified type uses the superset of enum values
### Files changed
- `codersdk/chats.go` — SDK types and client methods
- `coderd/chats.go` — Handler consolidation
- `coderd/coderd.go` — Route change
- `coderd/chats_test.go` — Test updates
- `site/src/api/api.ts` — Frontend API client
- `site/src/api/queries/chats.ts` — Query mutations
- `site/src/api/queries/chats.test.ts` — Test mocks
- `site/src/pages/AgentsPage/AgentsPage.tsx` — Call site
- Generated files (`typesGenerated.ts`,
`chatModelOptionsGenerated.json`)
### Testing
- All Go tests pass (`TestArchiveChat`, `TestUnarchiveChat`,
`TestChatSystemPrompt`)
- All frontend tests pass (31/31 in `chats.test.ts`)
|
||
|
Kyle Carberry
|
ca5158f94a |
fix: unify sidebar Git/Desktop tab styles with GitPanel tabs (#23164)
The active Git tab looked different than the Desktop tab, and they didn't match the actual tabs in the Git section. |
||
|
Kyle Carberry
|
eb828a6a86 | fix: skip input refocus after send on mobile viewports (#23141) | ||
|
Kyle Carberry
|
b69631cb35 | chore(site): improve mobile layout for agent chat (#23139) | ||
|
Kyle Carberry
|
7b0aa31b55 | feat: render file references inline in user messages (#23131) | ||
|
Kyle Carberry
|
6972d073a2 |
fix: improve background process handling for agent tools (#23132)
## Problem Models frequently use shell `&` instead of `run_in_background=true` when starting long-running processes through `/agents`, causing them to die shortly after starting. This happens because: 1. **No guidance in tool schema** — The `ExecuteArgs` struct had zero `description` tags. The model saw `run_in_background: boolean (optional)` with no explanation of when/why to use it. 2. **Shell `&` is silently broken** — `sh -c "command &"` forks the process, the shell exits immediately, and the forked child becomes an orphan not tracked by the process manager. 3. **No process group isolation** — The SSH subsystem sets `Setsid: true` on spawned processes, but the agent process manager set no `SysProcAttr` at all. Signals only hit the top-level `sh`, not child processes. ## Investigation Compared our implementation against **openai/codex** and **coder/mux**: | Aspect | codex | mux | coder/coder (before) | |--------|-------|-----|---------------------| | Background flag | Yield/resume with `session_id` | `run_in_background` with rich description | `run_in_background` with **no description** | | `&` handling | `setsid()` + `killpg()` | `detached: true` + `killProcessTree()` | **Nothing** — orphaned children escape | | Process isolation | `setsid()` on every spawn | `set -m; nohup ... setsid` for background | **No `SysProcAttr` at all** | | Signal delivery | `killpg(pgid, sig)` — entire group | `kill -15 -\$pid` — negative PID | `proc.cmd.Process.Signal()` — **PID only** | ## Changes ### Fix 1: Add descriptions to `ExecuteArgs` (highest impact) The model now sees explicit guidance: *"Use for long-running processes like dev servers, file watchers, or builds. Do NOT use shell & — it will not work correctly."* ### Fix 2: Update tool description The top-level execute tool description now reinforces: *"Use run_in_background=true for long-running processes. Never use shell '&' for backgrounding."* ### Fix 3: Detect trailing `&` and auto-promote to background Defense-in-depth: if the model still uses `command &`, we strip the `&` and promote to `run_in_background=true` automatically. Correctly distinguishes `&` from `&&`. ### Fix 4: Process group isolation (`Setpgid`) New platform-specific files (`proc_other.go` / `proc_windows.go`) following the same pattern as `agentssh/exec_other.go`. Every spawned process gets its own process group. ### Fix 5: Process group signaling `signal()` now uses `syscall.Kill(-pid, sig)` on Unix to signal the entire process group, ensuring child processes from shell pipelines are also cleaned up. ## Testing All existing `agent/agentproc` tests pass. Both packages compile cleanly. |
||
|
Kyle Carberry
|
89bb5bb945 |
ci: fix build job disk exhaustion on Depot runners (#23136)
## Problem The `build` job on `main` has been failing intermittently (and now consistently) with `no space left on device` on the `depot-ubuntu-22.04-8` runner. The runner's disk fills up during Docker image builds or SBOM generation, depending on how close to the limit a given run lands. The build was already at the boundary — the Go build cache alone is ~1.3 GB, build artifacts are ~2 GB, and Docker image builds + SBOM scans need several hundred MB of headroom in `/tmp`. No single commit caused this; cumulative growth in dependencies and the scheduled `coder-base:latest` rebuild on Monday morning nudged it past the limit. ## Fix Three changes to reclaim ~2 GB of disk before Docker runs: 1. **Build all platform archives and packages in the Build step** — moves arm64/armv7 `.tar.gz` and `.deb` from the Docker step to the Build step so we can clean caches in between. 2. **Clean up Go caches between Build and Docker** — once binaries are compiled, the Go build cache and module cache aren't needed. Also removes `.apk`/`.rpm` packages that are never uploaded. 3. **Set `DOCKER_IMAGE_NO_PREREQUISITES`** — tells make to skip redundantly building `.deb`/`.rpm`/`.apk`/`.tar.gz` as prerequisites of Docker image targets. The Makefile already supports this flag for exactly this purpose. |
||
|
Kyle Carberry
|
b7eab35734 |
fix(site): scope chat cache helpers to chat-list queries only (#23134)
## Problem
`updateInfiniteChatsCache`, `prependToInfiniteChatsCache`, and
`readInfiniteChatsCache` use `setQueriesData({ queryKey: ["chats"] })`
which prefix-matches **all** queries starting with `"chats"`, including
`["chats", chatId, "messages"]`.
After #23083 converted chat messages to `useInfiniteQuery`, the cached
messages data gained a `.pages` property containing
`ChatMessagesResponse` objects (not `Chat[]` arrays). The `if
(!prev.pages)` guard no longer bailed out, and the updater called
`.map()` on these objects — `TypeError: Z.map is not a function`.
## Fix
Extract the `isChatListQuery` predicate that already existed inline in
`invalidateChatListQueries` and apply it to all four cache helpers. This
scopes them to sidebar queries (`["chats"]` or `["chats",
<filterOpts>]`) and skips per-chat queries (`["chats", <id>, ...]`).
|
||
|
Kyle Carberry
|
741af057dc |
feat: paginate chat messages endpoint with cursor-based infinite scroll (#23083)
Adds cursor-based pagination to the chat messages endpoint. ## Backend - New `GetChatMessagesByChatIDPaginated` SQL query: returns messages in `id DESC` order with a `before_id` keyset cursor and configurable `limit` - Handler parses `?before_id=N&limit=N` query params, uses the `LIMIT N+1` trick to set `has_more` without a separate COUNT query - Queued messages only returned on the first page (no cursor) since they're always the most recent - SDK client updated with `ChatMessagesPaginationOptions` - Fully backward compatible: omitting params returns the 50 newest messages ## Frontend - Switches `getChatMessages` from `useQuery` to `useInfiniteQuery` with cursor chaining via `getNextPageParam` - Pages flattened and sorted by `id` ascending for chronological display - `MessagesPaginationSentinel` component uses `IntersectionObserver` (200px rootMargin prefetch) inside the existing `flex-col-reverse` scroll container - `flex-col-reverse` handles scroll anchoring natively when older messages are prepended — no manual `scrollTop` adjustment needed (same pattern as coder/blink) ## Why cursor-based instead of offset/limit Offset-based pagination breaks when new messages arrive while paginating backward (offsets shift, causing duplicates or missed messages). The `before_id` cursor is stable regardless of inserts — each page is deterministic. |
||
|
Kyle Carberry
|
32a894d4a7 |
fix: error on ambiguous matches in edit_files tool (#23125)
## Problem The `edit_files` tool used `strings.ReplaceAll` for exact substring matches, silently replacing **every** occurrence. When an LLM's search string wasn't unique in the file, this caused unintended edits. Fuzzy matches (passes 2 and 3) only replaced the first occurrence, creating inconsistent behavior. Zero matches were also silently ignored. ## Investigation Investigated how **coder/mux** and **openai/codex** handle this: | Tool | Multiple matches | No match | Flag | |---|---|---|---| | **coder/mux** `file_edit_replace_string` | Error (default `replace_count=1`) | Error | `replace_count` (int, default 1, -1=all) | | **openai/codex** `apply_patch` | Uses first match after cursor (structural disambiguation via context lines + `@@` markers) | Error | None (different paradigm) | | **coder/coder** `edit_files` (before) | Exact: replaces all. Fuzzy: replaces first. | Silent success | None | ## Solution Adopted the mux approach (error on ambiguity) with a simpler `replace_all: bool` instead of `replace_count: int`: - **Default (`replace_all: false`)**: search string must match exactly once. Multiple matches → error with guidance: *"search string matches N occurrences. Include more surrounding context to make the match unique, or set replace_all to true"* - **`replace_all: true`**: replaces all occurrences (opt-in for intentional bulk operations like variable renames) - **Zero matches**: now returns an error instead of silently succeeding Chose `bool` over `int` count because: 1. LLMs are bad at counting occurrences 2. The real intent is binary (one specific spot vs. all occurrences) 3. Simpler error recovery loop for the LLM ## Changes | File | Change | |---|---| | `codersdk/workspacesdk/agentconn.go` | Add `ReplaceAll bool` to `FileEdit` struct | | `agent/agentfiles/files.go` | Count matches before replacing; error if >1 and not opted in; error on zero matches; add `countLineMatches` helper | | `codersdk/toolsdk/toolsdk.go` | Expose `replace_all` in tool schema with description | | `agent/agentfiles/files_test.go` | Update existing tests, add `EditEditAmbiguous`, `EditEditReplaceAll`, `NoMatchErrors`, `AmbiguousExactMatch`, `ReplaceAllExact` | |
||
|
Kyle Carberry
|
af1be592cf |
fix: disable agent notification chime by default (#23124)
The completion chime on `/agents` was enabled by default for new users (or when no localStorage preference existed). This changes the default to disabled, so users must explicitly opt in via the sound toggle button. ## Changes - `getChimeEnabled()` now returns `false` when no preference is stored (was `true`) - `catch` fallback also returns `false` (was `true`) - Updated tests to reflect the new default and explicitly enable the chime in `maybePlayChime` tests |
||
|
Kyle Carberry
|
6f97539122 |
fix: update sidebar diff status on WebSocket events (#23116)
## Problem The sidebar diff status (PR icon, +additions/-deletions, file count) was not updating in real-time. Users had to reload the page to see changes. Two root causes: 1. **Frontend**: The `diff_status_change` WebSocket handler in `AgentsPage.tsx` had an early `return` (line 398) that skipped `updateInfiniteChatsCache`, so the sidebar's cache was never updated. Even for other event types, the cache merge only spread `status` and `title` — never `diff_status`. 2. **Server**: `publishChatPubsubEvent` in `chatd.go` constructed a minimal `Chat` payload without `DiffStatus`, so even if the frontend consumed the event, `updatedChat.diff_status` would be `undefined`. ## Fix ### Server (`coderd/chatd/chatd.go`) - `publishChatPubsubEvent` now accepts an optional `*codersdk.ChatDiffStatus` parameter; when non-nil it's set on the outgoing `Chat` payload. - `PublishDiffStatusChange` fetches the diff status from the DB, converts it, and passes it through. - Added `convertDBChatDiffStatus` (mirrors `coderd/chats.go`'s converter to avoid circular import). - All other callers pass `nil`. ### Frontend (`site/src/pages/AgentsPage/AgentsPage.tsx`) - Removed the early `return` so `diff_status_change` events fall through to the cache update logic. - Added `isDiffStatusEvent` flag and spread `diff_status` into both the infinite chats cache (sidebar) and the individual chat cache. |
||
|
Kyle Carberry
|
530872873e |
chore: remove swagger annotations from experimental chat endpoints (#23120)
The `/archive` and `/desktop` chat endpoints had swagger route comments (`@Summary`, `@ID`, `@Router`, etc.) that would cause them to appear in generated API docs. Since these live under `/experimental/chats`, they should not be documented. This removes the swagger annotations and adds the standard `// EXPERIMENTAL: this endpoint is experimental and is subject to change.` comment to `archiveChat` (the `watchChatDesktop` handler already had it, just needed the swagger block removed). |
||
|
Kyle Carberry
|
27cbf5474b |
refactor: remove /diff-status endpoint, include diff_status in chat payload (#23082)
The `/chats/{chat}/diff-status` endpoint was redundant because:
- The `Chat` type already has a `DiffStatus` field
- Listing chats already resolves and returns `diff_status`
- The `getChat` endpoint was the only one not resolving it (passing
`nil`)
## Changes
**Backend:**
- `getChat` now calls `resolveChatDiffStatus` and includes the result in
the response
- Removed `getChatDiffStatus` handler, route (`GET /diff-status`), and
SDK method
- Tests updated to use `GetChat` instead of `GetChatDiffStatus`
**Frontend:**
- `AgentDetail.tsx`: uses `chatQuery.data?.diff_status` instead of
separate query
- `RemoteDiffPanel.tsx`: accepts `diffStatus` as a prop instead of
fetching internally
- `AgentsPage.tsx`: `diff_status_change` events now invalidate the chat
query
- Removed `chatDiffStatus` query, `chatDiffStatusKey`, and
`getChatDiffStatus` API method
|
||
|
Kyle Carberry
|
3d9628c27e |
ci: split build artifacts into per-platform uploads (#23081)
Splits the single `coder` artifact (containing all platforms in a 1.3GB zip) into individual artifacts per OS/arch/format. ## Problem All CI build artifacts are uploaded as a single artifact named `coder`, producing a 1.3GB zip containing every platform's binary. This makes it impossible to download a single platform's binary without pulling the entire bundle. ## Solution Upload each platform/format combination as a separate artifact: | Artifact Name | Contents | |---|---| | `coder-linux-amd64.tar.gz` | Linux amd64 tarball | | `coder-linux-amd64.deb` | Linux amd64 deb package | | `coder-linux-arm64.tar.gz` | Linux arm64 tarball | | `coder-linux-arm64.deb` | Linux arm64 deb package | | `coder-linux-armv7.tar.gz` | Linux armv7 tarball | | `coder-linux-armv7.deb` | Linux armv7 deb package | | `coder-windows-amd64.zip` | Windows amd64 zip | ## Plan This is the first step toward letting customers install directly from `main` via: ```bash curl -L https://coder.com/install.sh | sh -s -- --unsafe-unstable ``` GitHub Actions artifact downloads require authentication even for public repos, so the next steps are to add a small Cloudflare Worker (similar to the one we already have for `install.sh`) that: 1. Lists artifacts via the GitHub API (unauthenticated) to find the latest artifact ID for the requested platform 2. Calls the download endpoint with a GitHub token (CF Worker secret) to get a 302 redirect to a time-limited Azure Blob URL 3. Redirects the caller to that URL (which requires no auth) This gives us publicly accessible per-platform URLs that the `--unsafe-unstable` flag would point at. The worker doesn't proxy the binary itself — it only proxies the metadata API call (~1KB) and redirects for the actual download. This PR splits the artifacts so the worker can serve individual platform downloads (~200MB each) instead of forcing a 1.3GB bundle. |
||
|
Kyle Carberry
|
266c611716 |
refactor(site): consolidate Git panel diff viewers and polish UI (#23080)
## Summary Refactors the Git panel in the Agents page to consolidate duplicated diff viewer code and significantly improve the UI. ### Deduplication - **RemoteDiffPanel** now uses the shared `DiffViewer` component instead of duplicating file tree, lazy loading, scroll tracking, and layout (~500 lines removed). - Renamed `RepoChangesPanel` → `LocalDiffPanel`, `FilesChangedPanel` → `RemoteDiffPanel` to reflect actual scope. - Removed `headerLeft`/`headerRight` abstraction from `DiffViewer` — each consumer owns its own header. - Replaced hand-rolled `ChatDiffStatusResponse` with auto-generated `ChatDiffStatus` from `typesGenerated.ts`. ### Tab Redesign - Per-repo tabs: each local repo gets its own tab (`Working <repo>`) instead of a single stacked view. - PR tab shows state icon + PR title; branch-only tab shows branch icon. - Tabs use `Button variant="outline"` matching the Git/Desktop tab style. - Radix `ScrollArea` with thin horizontal scrollbar for tab overflow. - Diff style toggle and refresh button lifted to shared toolbar, always visible. ### PR Header - Compact sub-header: `base_branch ←`, state badge (`Open`/`Draft`/`Merged`/`Closed`), diff stats, and `View PR` button. - GitHub-style state-aware icons (green open, gray draft, purple merged, red closed). - New API fields synced: `base_branch`, `author_login`, `pr_number`, `commits`, `approved`, `reviewer_count`. ### Local Changes Header - Compact sub-header: branch name, repo root path, diff stats, and `Commit` button (styled to match `View PR`). - `CircleDotIcon` (amber) for working changes tabs — universal "modified" indicator. ### Visual Polish - All text in sub-headers and buttons at 13px matching chat font size. - All badges (`DiffStatBadge`, PR state, `View PR`, `Commit`) use consistent `border-border-default`, `rounded-sm`, `leading-5`. - No background color on diff viewer header bars. - Tabs hidden when their view has no content; auto-switch when active tab disappears. ### Stories - New `GitPanel.stories.tsx` covering: open PR + working changes, draft PR, merged PR, closed PR, branch only, working changes only, multiple repos, empty state. - Removed old `LocalDiffPanel.stories.tsx` and `RemoteDiffPanel.stories.tsx`. |
||
|
Kyle Carberry
|
83e4f9f93e |
fix(agents): narrow chat mutation query invalidation (#23078)
## Problem
Sending a message on the `/agents` page triggers a burst of redundant
HTTP requests. The root cause is that chat mutations call
`invalidateQueries({ queryKey: ["chats"] })` which, due to React Query's
default **prefix matching**, cascades to every query whose key starts
with `["chats"]`:
- `["chats", {archived: false}]` — infinite sidebar list
- `["chats", chatId]` — individual chat detail
- `["chats", chatId, "messages"]` — all messages
- `["chats", chatId, "diff-status"]` — diff status
- `["chats", chatId, "diff-contents"]` — diff contents
- `["chats", "costSummary", ...]` — cost summaries
All of these have active subscribers on the page, so each one fires a
network request. The WebSocket stream already delivers these updates in
real-time, making the HTTP refetches completely redundant.
## Fix
| Mutation | Before | After |
|---|---|---|
| `createChatMessage` | `invalidateQueries({ queryKey: chatsKey })` —
prefix cascade | **Removed** — WebSocket delivers messages + sidebar
updates |
| `interruptChat` | `invalidateQueries({ queryKey: chatsKey })` — prefix
cascade | **Removed** — WebSocket delivers status changes |
| `editChatMessage` | 3 broad invalidations including `chatsKey` prefix
| 2 targeted with `exact: true`: `chatKey(id)` + `chatMessagesKey(id)` |
| `promoteChatQueuedMessage` | 3 broad invalidations including
`chatsKey` prefix | 2 targeted with `exact: true`: `chatKey(id)` +
`chatMessagesKey(id)` |
`editChatMessage` keeps `chatMessagesKey` invalidation because editing
truncates messages server-side and the WebSocket can only insert/update,
never remove stale entries.
## Net effect
Sending a message previously triggered **5–7 HTTP requests**. Now it
triggers **zero** — the WebSocket handles everything.
## Tests
Added `describe("mutation invalidation scope")` with 8 test cases
asserting that each mutation only invalidates the queries it genuinely
needs.
|
||
|
Kyle Carberry
|
ff9d061ae9 |
fix(site): prevent duplicate chat in agents sidebar on creation (#23077)
## Problem
When creating a new chat in the agents page (`/agents`), the chat could
appear multiple times in the sidebar. This was a race condition
triggered by the WebSocket `created` event handler.
## Root Cause
`updateInfiniteChatsCache` applies its updater function **independently
on each page** of the infinite query:
```ts
const nextPages = prev.pages.map((page) => updater(page));
```
When the `watchChats` WebSocket received a `"created"` event, the
handler checked `exists` only within the *current page*, then prepended
the new chat if not found:
```ts
updateInfiniteChatsCache(queryClient, (chats) => {
const exists = chats.some((c) => c.id === updatedChat.id);
// ...
if (chatEvent.kind === "created") {
return [updatedChat, ...chats]; // runs per page!
}
});
```
Since a brand-new chat doesn't exist in any page, **every loaded page**
prepends it. After `pages.flat()`, the chat appears once per loaded page
in the sidebar.
## Fix
- Added `prependToInfiniteChatsCache` in `chats.ts` that checks across
**all pages** before prepending, and only adds to page 0.
- Split the WebSocket handler so `"created"` events use the new safe
prepend, while update events (`title_change`, `status_change`) continue
using `updateInfiniteChatsCache` (which is safe for `.map()` operations
that don't add entries).
|
||
|
Kyle Carberry
|
0d3e39a24e |
feat: add head_branch to pull request diff status (#23076)
Adds the `head_branch` field (the source/feature branch name of a PR) to the diff status pipeline. Previously only `base_branch` (target branch) and the head commit SHA were captured from the GitHub API, but not the head branch name itself. ## Changes - **Migration 438**: Add `head_branch` nullable TEXT column to `chat_diff_statuses` - **gitprovider**: Parse `head.ref` from the GitHub API response (alongside `head.sha`) and add `HeadBranch` to `PRStatus` - **gitsync**: Wire `HeadBranch` through `refreshOne()` into the DB upsert params - **worker**: Map `HeadBranch` in `chatDiffStatusFromRow()` - **coderd**: Convert `HeadBranch` in `convertChatDiffStatus()` - **codersdk**: Expose as `head_branch` (`*string`, omitempty) in `ChatDiffStatus` API response - **Tests**: Updated `github_test.go` pull JSON fixtures and assertions |
||
|
Kyle Carberry
|
ddd1e86a90 |
fix(site): prevent infinite scroll from spamming duplicate chat list requests (#23075)
## Problem The agents sidebar infinite scroll was spamming the `/api/v2/chats` endpoint with duplicate requests at the same offset, caused by the `LoadMoreSentinel` component. ### Root cause `onLoadMore` is an inline arrow function (`() => void chatsQuery.fetchNextPage()`), creating a **new function reference on every render**. The `useEffect` in `LoadMoreSentinel` depended on `[onLoadMore]`, so it tore down and re-created the `IntersectionObserver` on every render. Each new observer immediately fired its callback when the sentinel was already visible, triggering duplicate fetches. ## Fix - Store `onLoadMore` and `isFetchingNextPage` in **refs** so the observer callback always reads the latest values without needing to tear down/re-create. - Create the `IntersectionObserver` **once on mount** (empty deps array). - **Guard** against calling `onLoadMore` while `isFetchingNextPage` is true. ## Tests - **LoadMoreSentinel behavior tests** (6 tests): verifies no duplicate calls across re-renders, proper `isFetchingNextPage` gating, ref-based observer stability, and correct resume after fetch completes. - **`infiniteChats` query factory tests** (6 tests): covers `getNextPageParam` and `queryFn` offset computation to prevent pagination regressions. |
||
|
Kyle Carberry
|
b492c42624 |
chore(dogfood): add Google Chrome to dogfood image (#23063)
Install Google Chrome stable directly from `dl.google.com`. Ubuntu 22.04 ships `chromium-browser` as a snap-only package, which does not work in Docker containers. ```dockerfile RUN wget -q https://dl.google.com/linux/direct/google-chrome-stable_current_amd64.deb && \\ apt-get install --yes ./google-chrome-stable_current_amd64.deb && \\ rm google-chrome-stable_current_amd64.deb ``` Verified in a running dogfood workspace: ``` $ google-chrome --version Google Chrome 146.0.7680.75 ``` |
||
|
Kyle Carberry
|
c5b8611c5a |
feat(gitsync): enrich PR status with author, base branch, review info (#23038)
## Summary Adds 7 new fields to the PR status stored by gitsync, all sourced from the existing GitHub API calls (**zero additional HTTP requests**): | Field | Source | Purpose | |---|---|---| | `author_login` | `pull.user.login` | PR author username | | `author_avatar_url` | `pull.user.avatar_url` | PR author avatar for UI | | `base_branch` | `pull.base.ref` | Target branch (e.g. `main`) | | `pr_number` | `pull.number` | Explicit PR number | | `commits` | `pull.commits` | Number of commits in PR | | `approved` | Derived from reviews | True when ≥1 approved, no outstanding changes requested | | `reviewer_count` | Derived from reviews | Distinct reviewers with a decisive state | ## Changes - **`gitprovider/gitprovider.go`**: Added 7 fields to `PRStatus` struct. - **`gitprovider/github.go`**: Expanded the anonymous struct in `FetchPullRequestStatus` to decode new JSON fields. Replaced `hasOutstandingChangesRequested()` with `summarizeReviews()` returning a `reviewStats` struct with `changesRequested`, `approved`, and `reviewerCount`. - **Migration 000434**: Adds 7 columns to `chat_diff_statuses`. - **`queries/chats.sql`**: Updated `UpsertChatDiffStatus` INSERT/VALUES/ON CONFLICT. - **`gitsync/gitsync.go`**: Maps new `PRStatus` fields into upsert params. - **`gitsync/worker.go`**: Maps new columns in row-to-model converter. - **`codersdk/chats.go`**: Added fields to SDK `ChatDiffStatus` type. - **`coderd/chats.go`**: Maps new DB fields in `convertChatDiffStatus()`. - Auto-generated: `models.go`, `queries.sql.go`, `dump.sql`, `typesGenerated.ts`. |
||
|
Kyle Carberry
|
1152b61ebb |
fix(site): fix speech recognition race condition and silent errors (#23043)
## Problem
Clicking the microphone button on `/agents` briefly activates recording
then immediately stops, focusing the text input.
## Root causes
**1. Race condition in `start()`** — When aborting a previous
recognition instance, the old instance's `onend` fires
**asynchronously** in real browsers (unlike our mock which fires it
synchronously). The stale `onend` callback then sets `isRecording=false`
and nullifies `recognitionRef.current`, killing the new recording
session. The unit tests didn't catch this because the mock fires `onend`
synchronously inside `abort()`.
**2. Silent `onerror` handler** — The `onerror` callback completely
discarded the error event. If the browser denied mic permission or the
speech service was unreachable (Chrome sends audio to Google servers),
recording silently died with no feedback.
**3. No cleanup on unmount** — The hook leaked a running recognition
instance if the component unmounted while recording.
## Fixes
- Guard `onend`/`onerror` callbacks with `recognitionRef.current !==
recognition` so stale instances are ignored
- Expose `error: string | null` state from the hook; surface it in the
UI ("Mic access denied" / "Voice input failed")
- Add a cleanup `useEffect` that aborts recognition on unmount
- Added 4 new tests covering the race condition, error exposure, and
error clearing
|
||
|
Kyle Carberry
|
690e3a87d8 |
feat: move chat messages to dedicated /chats/{id}/messages endpoint (#23021)
## Summary
Moves the messages response out of `GET /chats/{id}` and into a
dedicated `GET /chats/{id}/messages` endpoint.
### Backend
- `GET /chats/{id}` now returns just the `Chat` object (no messages)
- `GET /chats/{id}/messages` is a new endpoint returning
`ChatMessagesResponse` with `messages` and `queued_messages`
- Added `ChatMessagesResponse` SDK type and `GetChatMessages` client
method
### Frontend
- `getChat()` API method returns `Chat` instead of `ChatWithMessages`
- Added `getChatMessages()` API method for the new endpoint
- Split `chatQuery` into two: `chatQuery` (metadata) and
`chatMessagesQuery` (messages)
- Updated all cache mutations, optimistic updates, and websocket
handlers
- Updated tests and stories
### Files changed
| File | Change |
|---|---|
| `coderd/coderd.go` | Register `GET /messages` route |
| `coderd/chats.go` | Simplify `getChat`, add `getChatMessages` handler
|
| `codersdk/chats.go` | New type + method, update `GetChat` return |
| `site/src/api/api.ts` | New method, update `getChat` |
| `site/src/api/queries/chats.ts` | New query, update cache mutations |
| `site/src/pages/AgentsPage/AgentDetail.tsx` | Use separate queries |
| `site/src/pages/AgentsPage/AgentDetail/ChatContext.ts` | Update types
and cache writes |
| `site/src/pages/AgentsPage/AgentsPage.tsx` | Update websocket cache
handler |
|
||
|
Kyle Carberry
|
0e7e0a959e |
feat(site): add voice-to-text input to agent chat (#23022)
## Summary Adds a microphone button to the agent chat input for browser-native voice-to-text transcription using the Web Speech API. ## Changes ### New: `site/src/hooks/useSpeechRecognition.ts` - Custom React hook wrapping the Web Speech API (`SpeechRecognition` / `webkitSpeechRecognition`) - Feature-detects browser support via `isSupported` - Provides `start()`, `stop()`, and `cancel()` controls - Accumulates real-time transcript from interim and final recognition results - Inline TypeScript declarations for the Web Speech API types ### Modified: `site/src/pages/AgentsPage/AgentChatInput.tsx` - **Mic button**: Appears to the right of the image attach button when the browser supports the Speech Recognition API. Shows a microphone icon when idle, X icon when recording. - **Send button**: Transforms into a checkmark during recording to accept the transcription. Always enabled during recording. - **Editor sync**: Live-updates the Lexical editor with the transcription as the user speaks. Preserves any pre-existing text. - **Cancel**: Restores the editor to its pre-recording content. ## How it works 1. User clicks the mic button → recording starts, real-time transcript appears in the editor 2. User clicks the checkmark (send button) → recording stops, transcribed text stays 3. User clicks X (mic button) → recording stops, editor reverts to original content |
||
|
Kyle Carberry
|
84dc1a3482 | fix: hide web_search tool — preserve ProviderExecuted on DB-loaded tool results (#23014) | ||
|
Kyle Carberry
|
0e1846fe2a | fix(agent): reap exited processes and scope process list by chat ID (#22944) | ||
|
Kyle Carberry
|
42c12176a0 |
fix(chatd): persist interrupted tool call steps instead of losing them (#23011)
## Problem When a chat is interrupted while tools are executing, the step content (text, reasoning, tool calls, and partial tool results) was being lost. Two gaps existed: 1. **During tool execution**: `executeTools` returns with error results for interrupted tools, but the subsequent `PersistStep(ctx, ...)` fails on the canceled context and returns `ErrInterrupted` without persisting anything. 2. **PersistStep race**: If the context is canceled between the post-tool interrupt check and the `PersistStep` call, the same loss occurs. This is inconsistent with how we handle stream interruptions (which properly flush and persist partial content via `persistInterruptedStep`) and how [coder/blink](https://github.com/coder/blink) handles interruptions (always inserting the response message regardless of execution phase). ## Fix Two changes in `chatloop.go`: - **Post-tool-execution interrupt check**: After `executeTools` returns, check if the context was interrupted and route through `persistInterruptedStep` (which uses `context.WithoutCancel` internally) to save the accumulated content. - **PersistStep fallback**: If `PersistStep` returns `ErrInterrupted`, retry via `persistInterruptedStep` so partial content is not lost. ## Tests - `TestRun_InterruptedDuringToolExecutionPersistsStep`: Verifies that when a tool is blocked and the chat is interrupted, the step (text + reasoning + tool call + tool error result) is persisted via the interrupt-safe path. - `TestRun_PersistStepInterruptedFallback`: Verifies that when `PersistStep` itself returns `ErrInterrupted`, the step is retried via the fallback path and content is saved. |
||
|
Kyle Carberry
|
072e9a212f |
fix(chatloop): keep provider-executed tool results in assistant message (#23012)
## Problem When a step contains both provider-executed tool calls (e.g. Anthropic web search) and local tool calls in parallel, the next loop iteration fails with the Anthropic API claiming the regular tool call has no result. However, sending a new user message (which reloads messages from the DB) works fine. ## Root cause `toResponseMessages` was placing **all** tool results into the tool-role message, regardless of `ProviderExecuted`. When Fantasy's Anthropic provider later converted these messages for the API, it moved the provider tool result from the tool message to the **end** of the previous assistant message (`prevMsg.Content = append(...)`). This placed `web_search_tool_result` **after** the regular `tool_use` block: ``` assistant: [server_tool_use(A), tool_use(B), web_search_tool_result(A)] ← wrong order user: [tool_result(B)] ``` The persistence layer in `chatd.go` already handles this correctly — provider-executed tool results stay in the assistant message, producing the expected ordering: ``` assistant: [server_tool_use(A), web_search_tool_result(A), tool_use(B)] ← correct order user: [tool_result(B)] ``` This is why reloading from the DB fixed it. ## Fix In the `ContentTypeToolResult` case of `toResponseMessages`, route provider-executed results to `assistantParts` instead of `toolParts`, matching the persistence layer's behavior. ## Testing Added `TestToResponseMessages_ProviderExecutedToolResultInAssistantMessage` which verifies that mixed provider+local tool results are split correctly between the assistant and tool messages. |
||
|
Kyle Carberry
|
3407fa80a4 |
fix: only show subagent chevron on icon hover, not full row hover (#23010)
## Summary When hovering on an agent row that is **not running**, the expand/collapse chevron (`>`) for subagents was appearing by replacing the status icon. This was visually distracting when scanning the sidebar. ## Change The chevron now only appears when hovering directly over the **icon area** (`group-hover/icon`), not the entire row (`group-hover`). This was already the behavior for running agents — this PR makes it consistent for all states. - Unified both the status-icon-hide and chevron-show hover triggers to use `group-hover/icon` - Removed the now-unused `isExecuting` variable (net -15 lines) |
||
|
Kyle Carberry
|
1ac5418fc4 |
fix(agent-chat): use direct concatenation for completed message text blocks (#23009)
## Problem
LLMs stream list markers and item content as separate text blocks:
```json
{ "type": "text", "text": "Intro\n\n- " }
{ "type": "text", "text": "First item" }
{ "type": "text", "text": "\n- " }
{ "type": "text", "text": "Second item" }
```
The **streaming path** concatenated these directly → `"- First item"` ✅
The **completed path** used `appendText` which inserted `\n` between
chunks → `"- \nFirst item"` ❌
Every CommonMark parser treats `"- \nText"` (marker and content on
different lines, content not indented) as an empty `<li>` followed by a
sibling `<p>`, producing broken list rendering once a message finished
streaming.
## Fix
Make `appendText` use direct concatenation — the same as the streaming
path. The API text blocks already contain all necessary whitespace and
newlines; inserting extra `\n` between them was the bug.
## Changes
- **`messageParsing.ts`** — `appendText` simplified to direct concat
(skip whitespace-only chunks). `appendParsedTextBlock` no longer passes
a custom joiner, so it uses the same default as the streaming path.
- **`messageParsing.test.ts`** — Updated existing merge test
expectation; added regression test with the exact LLM list-marker
payload.
|