mirror of
https://github.com/coder/coder.git
synced 2026-10-28 02:42:54 +08:00
9afd3d0bea603d6d7fa188dae01fcff2ac48fa10
100
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Kyle Carberry
|
58f6b9c4d0 |
fix(coderd/externalauth): retry transient refresh failures with backoff (#25686)
## Summary Wraps external auth token refresh in an exponential-backoff retry so a brief upstream hiccup (5xx, network timeout, rate-limited 429) no longer surfaces as an `InvalidTokenError` and forces users to re-authenticate. GitHub in particular has been flaky enough lately that this is hitting real users. ## Behavior - `(*Config).RefreshToken` now calls a helper that retries the `TokenSource.Token()` exchange with exponential backoff (250ms → 2s), bounded by a 10s total budget. - Errors classified as permanent by `isFailedRefresh` (e.g. `bad_refresh_token`, `invalid_grant`, `unauthorized_client`, ...) skip the retry loop. Retrying a permanent failure wastes the refresh quota and, on providers with single-use refresh tokens, can mask a legitimate concurrent winner with repeated `bad_refresh_token` responses. - Refreshes with an empty refresh token still short-circuit without making an API call. - The existing concurrent-refresh-race detection and optimistic-lock paths are unchanged. ## Tunables Three new `time.Duration` fields on `externalauth.Config` (`RefreshRetryInitialBackoff`, `RefreshRetryMaxBackoff`, `RefreshRetryTimeout`) let callers override the defaults. They default to zero, which falls back to the package defaults, so existing call sites are unaffected. The fields exist primarily so tests can dial the timing way down without touching package globals (and therefore without serializing parallel tests). ## Tests - `TestRefreshToken/RefreshRetries` now disables internal retries via `RefreshRetryTimeout = time.Nanosecond` so its existing "1 IDP call per `RefreshToken` invocation" assertion still holds. Otherwise its assertions are unchanged. - New `TestRefreshToken/RefreshTokenWithBackoff` simulates 3 transient 5xx failures followed by success and verifies the refresh ultimately succeeds with 4 total IDP attempts. - New `TestRefreshToken/RefreshTokenBackoffPermanentError` returns `bad_refresh_token` and verifies the refresh is **not** retried even with a generous 1s budget. <details> <summary>Why the explicit <code>retryCtx.Err()</code> guard?</summary> `retry.Retrier.Wait` `select`s between `time.After(delay)` and `ctx.Done()`. The first call has `delay == 0`, so `time.After(0)` and an already-cancelled context both fire immediately and Go picks the case nondeterministically. Without the guard, a near-zero retry budget would still trigger an unwanted extra refresh attempt roughly half the time, which would have made the `RefreshRetries` test flaky. </details> This PR was opened by a Coder agent on behalf of @kylecarbs. |
||
|
Kyle CarberryandCoder Agent
|
385146000b |
feat: record created_at/completed_at on reasoning ChatMessageParts (#24789)
Records reasoning start and end times on persisted reasoning `ChatMessagePart`s so reasoning duration can be computed for stored chats. Backend-only: no SSE changes and no frontend rendering ship in this PR. The `created_at` field on `ChatMessagePart` is extended to also be present on `reasoning` parts (it previously appeared only on `tool-call` and `tool-result`), and a new `completed_at` field is added for `reasoning` parts. ### How timestamps are recorded - `StreamPartTypeReasoningStart`: stamp `startedAt = dbtime.Now()` on the active reasoning state. - `StreamPartTypeReasoningEnd`: stamp `completedAt = dbtime.Now()` and append both into parallel `[]time.Time` slices on `stepResult`. - Persistence reads the slices in occurrence order (reasoning has no provider-side ID) and applies them to the matching `ChatMessagePart` via `buildAssistantPartsForPersist`. The first reasoning block's stamps go onto the first reasoning part, and so on. - `flushActiveState` flushes partial reasoning interrupted before `StreamPartTypeReasoningEnd` with `startedAt` from the active state and `completedAt = dbtime.Now()` at the interruption. ### Why two fields, not one? Tool calls and results are point events. The frontend computes their duration by subtracting the call's `created_at` from the result's `created_at`. Reasoning is one assistant part that brackets a span, so we record both endpoints on the part itself. ### Why not stamp in `PartFromContent`? Same rationale as #24101: `PartFromContent` is called during both SSE publishing and persistence. Stamping there would yield incorrect persistence-time timestamps for reasoning blocks that finished much earlier in the step. Instead we capture in the chatloop and apply during persistence. <details><summary>Implementation plan</summary> - `codersdk/chats.go`: extend `CreatedAt`'s `variants` to include `reasoning?`; add `CompletedAt *time.Time` with `variants:"reasoning?"`. - `coderd/x/chatd/chatloop/chatloop.go`: extend `reasoningState` with `startedAt`; extend `stepResult` and `PersistedStep` with parallel `[]time.Time` reasoning slices; stamp on `ReasoningStart`/`ReasoningEnd`; thread the slices through all `PersistStep` call sites including the interrupt-safe path; record partial reasoning in `flushActiveState`. - `coderd/x/chatd/attachments.go`: walk reasoning parts in occurrence order and apply `step.ReasoningStartedAt[i]` to `part.CreatedAt` and `step.ReasoningCompletedAt[i]` to `part.CompletedAt`. ### Tests - `codersdk/chats_test.go` round-trips `created_at` + `completed_at` on reasoning parts and verifies omission when absent and partial interrupted parts. - `coderd/x/chatd/chatprompt/chatprompt_test.go` asserts `PartFromContent(ReasoningContent{})` does NOT stamp timestamps. - `coderd/x/chatd/chatloop/chatloop_test.go` `TestRun_ReasoningTimestamps` drives a stream with two reasoning blocks and verifies parallel slices, monotonicity, ordering, non-zero values, and content-block ordering. `TestRun_InterruptedReasoningFlushesTimestamps` cancels mid-reasoning and verifies `flushActiveState` records a non-zero pair. - `coderd/x/chatd/attachments_test.go` covers `buildAssistantPartsForPersist` for normal interleaved reasoning, partial (zero `completed_at`), and missing slices. </details> > Generated by Coder Agents. Co-authored-by: Coder Agent <agent@coder.com> |
||
|
Kyle Carberry
|
159089686a |
fix(coderd/x/chatd): prime workspace MCP cache after create/start (#25298)
## Problem Mid-turn workspace MCP discovery was broken when an agent was still cold-starting. `PrepareTools` in `chatd.go` flipped `workspaceMCPDiscovered = true` *before* calling `discoverWorkspaceMCPTools`, so a failed discovery attempt permanently blocked retries within the turn. Customer-reported repro: - New chat with no pre-selected workspace. - LLM calls `create_workspace` mid-turn at `23:35:05`. - `PrepareTools` fires, dials the agent with a 30s timeout, dial times out at `23:38:15`, `discoverWorkspaceMCPTools` returns empty. - Agent connects at `23:38:29`, 14 seconds later. - `workspaceMCPDiscovered` was already true, so `PrepareTools` never retried for the rest of the turn. MCP tools only appeared on the next user message. A naive retry loop in `PrepareTools` would also miss the bigger picture: a workspace boot can take several minutes (EC2 cold start, 10 min startup scripts), and the chatloop only gets a chance to call `PrepareTools` between LLM steps. ## Fix Do the workspace MCP discovery from inside the tool that already waits for the agent. `chattool.CreateWorkspace` and `chattool.StartWorkspace` call `waitForAgentReady`, which has a 2 min agent-online budget plus a 10 min startup-script budget. By the time they fire `OnChatUpdated`, the agent is `Ready`. The chatd `onChatUpdated` callback now launches an async `primeWorkspaceMCPCache` goroutine on every bind that has a valid workspace ID: - The primer calls `discoverWorkspaceMCPTools` until it returns a non-empty list or `workspaceMCPPrimeMaxWait` (30s) elapses, with a 2s backoff between attempts. The bounded wait handles the short race between agent-online and the agent's MCP `Connect` settling. - The primer runs asynchronously so the tool itself never blocks. Some templates simply do not advertise MCP tools, in which case the primer would otherwise spend its full budget for nothing. - The primer shares the chat `ctx` (not a detached one) so it is canceled together with the chat. A dangling primer would re-dial the workspace conn after `runChat`'s deferred `workspaceCtx.close()` and leak that conn. - `inflight.Add(1)` ensures server shutdown still waits for any in-progress primer. - `PrepareTools` is simplified back to a single discovery call. It now only sets `workspaceMCPDiscovered = true` on success, so an empty result no longer permanently blocks discovery within the turn. The cache hit warmed by the primer makes that call cheap in the common case; the dial fallback handles the rare cache miss. ## Tests All in `coderd/x/chatd/chatd_internal_test.go`: - `TestPrimeWorkspaceMCPCache_SuccessOnFirstAttempt` — single `ListMCPTools` call returning tools populates the cache. - `TestPrimeWorkspaceMCPCache_RetriesUntilToolsAppear` — first call empty, second returns tools; primer retries past the backoff and writes the cache. Uses `quartz.Mock.Trap` on `NewTimer`. - `TestPrimeWorkspaceMCPCache_GivesUpAfterDeadline` — `ListMCPTools` always empty; primer stops at `workspaceMCPPrimeMaxWait` and refuses to cache the empty result so PrepareTools can retry on the next step. The existing integration test `TestRunChat_WorkspaceMCPDiscoveryAfterMidTurnCreateWorkspace` continues to pass and now also exercises the async-primer path end-to-end via the create_workspace tool. ``` go test ./coderd/x/chatd/... -count=1 go test ./coderd/x/chatd/ -race -count=1 make pre-commit ``` <details> <summary>Design notes</summary> - The first iteration of this PR added retry+cooldown+failure-cap logic inside `PrepareTools`. It worked for the customer's ~30s race window but did not help workspaces that take several minutes to boot, because `PrepareTools` only fires between LLM steps. Reviewer pointed out the right place to handle this is the tool itself; the current implementation does that. - Why async: a primer that ran synchronously inside the `OnChatUpdated` callback blocked the create_workspace tool from returning for up to `workspaceMCPPrimeMaxWait`, which broke `TestCreateWorkspaceTool_EndToEnd` and would hurt any template that does not expose MCP tools. Decoupling lets the tool return immediately and lets the primer warm the cache concurrently with the next LLM step. - Why share the chat `ctx` rather than `context.WithoutCancel(ctx)` (the title-generation pattern): the primer touches `workspaceCtx.getWorkspaceConn`, which `runChat`'s deferred `workspaceCtx.close()` invalidates. A detached primer outliving the chat would dial a fresh conn and leak it. - The constant naming distinguishes `workspaceMCPDiscoveryTimeout` (35s per-call dial budget, unchanged from #25169) from `workspaceMCPPrimeMaxWait` (30s total budget for the post-ready primer loop) and `workspaceMCPPrimeRetryInterval` (2s between empty-result retries). </details> Follow-up to #25169. --- _This pull request was generated by Coder Agents._ |
||
|
Kyle Carberry
|
9f99a7bc0b |
fix(coderd): stabilize TestPatchChatMessage/ChangesModel flaky test (#25306)
Fixes coder/internal#1535 ## Problem `TestPatchChatMessage/ChangesModel` is flaky because it races with the chat daemon's background processing. `CreateChat` sets the chat to `pending` and the daemon picks it up asynchronously. The test immediately calls `EditChatMessage` (which changes the model to an override) while the first processing round is still running. The `InsertChatMessages` SQL CTE unconditionally updates `chats.last_model_config_id` to the model of the last inserted message. When the daemon's in-flight message insertions commit after the edit transaction, they overwrite `last_model_config_id` back to the default model. Similarly, after the edit sets the chat back to `pending`, the daemon re-processes it. The test's `GetChat` call could race with this second round. ## Fix Poll for the chat to reach `waiting` (or `error`) status: 1. **Before editing**: wait for the initial processing round to complete 2. **After editing**: wait for the second processing round (triggered by the edit) to complete Then assert `last_model_config_id`, which is now stable. > Generated with [Coder Agents](https://coder.com/agents) by @kylecarbs |
||
|
Kyle Carberry
|
5040ab6fca |
feat: filter chats by diff URL via the q search parameter (#24970)
Adds a `diff_url:` term to the `q` search parameter on `GET /api/experimental/chats` so callers can look up the chat associated with a particular pull request, merge request, or any other URL persisted on the chat's diff status. ``` q=diff_url:"https://github.com/coder/coder/pull/123" ``` Match is case-insensitive. When the URL lives on a delegated sub-agent's diff status, the parent chat is returned so the relationship surfaces from a single lookup. <details> <summary>Design notes</summary> - **Forge-agnostic.** Reuses the existing `chat_diff_statuses.url` column rather than introducing a `pr:` vocabulary, since the SDK already documents the URL as "may point to a pull request or a branch page depending on whether a PR has been opened." Works for GitHub PRs, GitLab MRs, branch pages, etc. - **Composes with `archived:`.** The two terms can be combined: `q=archived:true diff_url:"..."`. - **Case handling.** The parser used to lowercase the entire `q` string up front, which would mangle URL path segments. Switched to lowercasing only the field key inside `searchTerms` (already happens there) and keeping the value as the caller typed it. The SQL comparison lowercases on both sides. - **Validation.** `diff_url` must be a syntactically valid HTTP(S) URL with a non-empty host. No forge-specific validation. - **Index.** Adds `idx_chat_diff_statuses_url_lower` on `LOWER(url)` so the lookup is cheap even on large datasets. - **Sub-agent fan-in.** `EXISTS` clause matches when the URL lives on the chat itself or any chat with `root_chat_id` equal to the chat's id, so a delegated sub-agent's PR pulls in its parent. - **Deferred.** Sentinels like `pr:any` / `pr:none` and a forge-agnostic state filter (`diff_state:open|merged|closed`) were intentionally left out of this change. They couple cleanly to a second forge or a clearer product call, and shipping them now would lock in vocabulary we may want to revisit. </details> ## Tests - `coderd/searchquery`: parser tests for valid URLs, case handling (key insensitive, value preserved), composition with `archived:`, and validation errors (non-HTTP scheme, missing host, malformed URL). - `coderd/exp_chats_test.go`: end-to-end coverage hitting `ListChats`. Verifies a root chat matches its own URL, a parent chat surfaces when only a sub-agent has the URL, lookups are case-insensitive, non-matching URLs return empty, and invalid URLs return `400`. --- _This PR was authored by a Coder Agent on behalf of @kylecarbs._ |
||
|
Kyle Carberry
|
147f50c5e8 |
fix(agent/x/agentmcp): watch MCP config files for late-appearing or rewritten config (#25172)
## Bug `agent/x/agentmcp/Manager` resolves its config paths once at boot, calls `Reload`, and then only re-stats them lazily when a `GET /api/v0/mcp/tools` request arrives (PR #24700). If any of the manager's MCP config files (`~/.mcp.json` by default, or whatever paths `agentcontextconfig.MCPConfigFiles()` resolves from `CODER_AGENT_EXP_MCP_CONFIG_FILES`) is created, atomically rewritten, or removed _after_ that initial `Reload` and _before_ the next tools HTTP request, the manager keeps serving the stale (often empty) snapshot. `parseAndDedup` silently swallows `fs.ErrNotExist`, so a late-appearing file looks indistinguishable from "no config" until something pokes the manager again. This affects any workspace where the file lands after `MarkStartupSettled` fires, including: - a startup script that writes `~/.mcp.json` after MCP init - a user creating or editing the file mid-session - an installer, dotfiles step, or sync tool writing the file later in startup - another agent process (Claude Code, etc.) producing the file out-of-band - editor rewrites that land as `Write + Chmod + Rename` bursts ### Concrete repro (dual-agent workspace) The race is easiest to reproduce on dual-agent workspaces (inner sandbox + outer host), where the inner agent's `scriptRunner` has `script_count=0` and `mcpManager.Reload` fires at ~t+0.3 s while the host agent writes `~/.mcp.json` ~21 s later. Timeline from `workspace-otto-aa16`: - agent up `20:23:38.918` - lifecycle Ready `20:23:39.200` - MCP config file Birth `20:24:00.460` (~21 s gap) - no MCP log lines for 8 minutes - first `GET /api/v0/mcp/tools` at `20:32:11.812` logs `[warn] mcp: mcp reload canceled by caller`, takes 4946 ms; subsequent turns are cached at 2 ms. The single-agent case has the same race; it's just usually narrow enough that the next HTTP request masks it, at the cost of a multi-second stall on the first call that has to do the lazy reload itself. PR #25034's `MarkStartupSettled` does not help: "settled" fires before the file is necessarily on disk. ## Fix Add an fsnotify-backed `configWatcher` to `agent/x/agentmcp/Manager`. The watcher consumes whatever paths the manager is told to reload, which is the same `[]string` returned by `agentcontextconfig.MCPConfigFiles()`. For each path the watcher: - Watches the **parent directory** of the path, not the file itself. This handles late creation, atomic rewrite (rename + create), and deletion uniformly because inotify watches on individual non-existent files return `ENOENT` and are lost across renames. The pattern matches `agent/agentcontainers/watcher`. - Walks up to the first existing **ancestor directory** when the parent does not yet exist, and re-arms deeper on `Create` events that promote an unrealized path. - Refcounts directory watches so multiple configured paths sharing a parent dir only register one inotify watch. - Resolves symlinks **once at arming time** via `filepath.EvalSymlinks`; never chases arbitrary symlink targets on events. - Debounces multi-event editor writes through a single `quartz.AfterFunc` timer so a `Write + Chmod + Rename` burst produces one reload. - Fires a debounced callback that calls `Manager.Reload`, which routes through the existing singleflight so concurrent triggers coalesce. - Re-syncs on every `Reload` call so a future path-list change is picked up. Lifecycle: the watcher is armed lazily on the first `Reload` (no goroutine cost for unit tests that never reload). `Manager.Close` marks the manager closed and closes its `closedCh` before tearing down the watcher, so any in-flight watcher-driven reload observes the close via `waitReload` and returns `ErrManagerClosed` instead of blocking `firesWG.Wait()` on a stuck connect. The watcher then waits for its goroutine and any in-flight debounced `fire` callback before returning. `parseAndDedup` behavior is unchanged: `fs.ErrNotExist` still records an empty snapshot. With the watcher armed before that snapshot is committed, any `Create` event that races `parseAndDedup` is still delivered. This is the agent-side complement to PR #25169, which fixes chatd's mid-turn workspace MCP discovery. `MarkStartupSettled` semantics are not changed. ## Tests (`agent/x/agentmcp/configwatcher_internal_test.go`) All new tests pass with the fix and fail without it. No `time.Sleep`; synchronization uses `testutil.Eventually` for fsnotify-driven assertions and the quartz mock clock for debounce assertions. - `TestWatcher_LateFileTriggersReload` - the late-file regression: empty dir, settle startup, `Reload` sees no file, write the file later, watcher reloads, tools appear. - `TestWatcher_RewriteTriggersReload` - existing file overwritten with a new server list, watcher reloads, cache reflects new server. - `TestWatcher_RemovalTransitionsToEmpty` - delete the file, watcher reloads, manager transitions to empty cleanly. - `TestWatcher_DebouncesBurst` - quartz mock clock; three back-to-back `scheduleFire` calls produce exactly one `onChange` after `AdvanceNext`. - `TestWatcher_CloseStopsGoroutine` - construct/Reload/Close five times to surface goroutine or fd leaks under `-race`. - `TestWatcher_DualAgentHTTPNoStall` - integration: write file after `Reload`, wait for watcher reload, then `GET /tools` returns the MCP tools in less than `testutil.WaitShort` instead of the multi-second "reload canceled" stall. - `TestWatcher_LateParentDirTriggersReload` - parent dir doesn't exist at `Reload` time; create the dir then the file; watcher re-arms deeper and reloads. - `TestWatcher_SharedParentRefcount` - two configured paths share a parent dir; only one inotify watch is registered and both reload on changes. - `TestWatcher_CloseDoesNotStallOnInFlightReload` - installs a `connectStartedHook` to block a watcher-driven reload mid-`connectAll`, then asserts `Close()` returns within `WaitMedium`. Regression-verified: reverting the close-ordering causes the test to time out. ## Acceptance checklist - [x] `go test ./agent/x/agentmcp/... -race -count=1` passes (also `-count=5`). - [x] All `./agent/...` tests pass under `-race -short`. - [x] No emdash, endash, or ` -- `; `scripts/check_emdash.sh` clean. - [x] No `time.Sleep` in tests. - [x] New tests fail without the fix and pass with it (verified by temporarily disabling `m.armWatcher(paths)` and by reverting `Close()` ordering). ## Out of scope No changes to chatd's mid-turn workspace MCP discovery (PR #25169) or `MarkStartupSettled` semantics. --- <sub>This pull request was prepared by a [Coder Agents](https://coder.com/docs/admin/ai-coder) run.</sub> |
||
|
Kyle Carberry
|
b0b07536fc | feat: add opt-in Coder identity headers for MCP servers (#25153) | ||
|
Kyle Carberry
|
376fc80451 |
fix(coderd/x/chatd): discover workspace MCP tools mid-turn after create_workspace (#25169)
## Problem In `coderd/x/chatd/chatd.go` `runChat`, workspace MCP discovery is gated on `chat.WorkspaceID.Valid` at the start of each turn. New chats that bind their workspace mid-turn (via `create_workspace` or `start_workspace`) get an empty workspace tool list on the first step, and the model falls back to `execute` (bash) because no workspace MCP tools are advertised. **Repro:** new chat → "create a workspace and use MCP tools". No `/api/v0/mcp/tools` request hits the agent on turn 1; turn 2 in the same chat works fine. ## Fix - Add a `PrepareTools` callback to `chatloop.RunOptions`, analogous to `PrepareMessages`. It is invoked once before each LLM step with the current tool list. When it returns non-nil, the chatloop replaces `opts.Tools`, rebuilds the per-step tool definitions, and appends new tool names to `opts.ActiveTools` so newly injected tools are callable immediately. - Wire `PrepareTools` in `runChat` to trigger workspace MCP discovery the first time the chat snapshot reports a valid `WorkspaceID`. The previous top-of-turn discovery path is unchanged for chats that start with a workspace. - Extract the discovery logic into `Server.discoverWorkspaceMCPTools` so the top-of-turn and mid-turn paths share identical behavior (cache, agent resolution, `ListMCPTools` timeout, invalidation). Mid-turn discovery stays disabled in plan-mode turns and Explore subagents, matching the existing top-of-turn gate. The `workspaceMCPDiscovered` flag prevents redundant dials after the first successful discovery. ## Tests - `coderd/x/chatd/chatloop/chatloop_test.go`: two new `TestRun_PrepareTools*` cases covering injection on the next step and active-set merging when `ActiveTools` is non-empty. - `coderd/x/chatd/chatd_test.go`: `TestRunChat_WorkspaceMCPDiscoveryAfterMidTurnCreateWorkspace` drives `runChat` through a `create_workspace` tool call against a real Postgres + mocked agent conn and asserts the second streamed LLM request advertises the workspace MCP tool. Verified that the test fails (and pinpoints the missing tool) when the `PrepareTools` wiring is disabled. ## Validation ``` go test ./coderd/x/chatd/chatloop/... -count=1 go test ./coderd/x/chatd/... -count=1 make lint/emdash ``` <details> <summary>Decision log</summary> - Chose a per-step `PrepareTools` callback over mutating `opts.Tools` in place because `chatloop.Run` builds the `fantasy.Tool` definitions once at start; a hook is required to let the LLM see new tools on the next step. - Returned `[]fantasy.AgentTool` (not also active-tool-names) and let the chatloop derive name merges via `mergeNewToolNames`. This avoids leaking plan-mode gating decisions into the callback contract. - Kept the existing top-of-turn discovery path so chats that already have a workspace at turn start pay no extra latency. - Skipped reusing `ReloadMessages` (history reload) since this is purely a tool-availability concern; coupling it to a history reload would defeat the chatloop cache prefix optimizations. </details> --- _This pull request was generated by Coder Agents._ |
||
|
Kyle Carberry
|
5a5cd79c4c | fix: drop buffered chat parts after their durable message commits (#25164) | ||
|
Kyle Carberry
|
07ff3b3f90 | fix(coderd/exp_chats_test.go): stabilize TestListChats/Pagination by inserting chats directly (#25137) | ||
|
Kyle Carberry
|
0ed57ee343 | fix(coderd/x/chatd): checkpoint buffered message_parts to avoid stale replay (#25145) | ||
|
Kyle Carberry
|
e3db203011 |
fix(coderd/azureidentity): set explicit roots to avoid macOS system verifier (#25136)
Fixes [CODAGT-372](https://linear.app/codercom/issue/CODAGT-372/coderdazureidentity-testvalidateregular-fails-on-macos). Closes coder/internal#101. ## Problem `coderd/azureidentity TestValidate/regular` fails on macOS with: ``` verify signature: github.com/coder/coder/v2/coderd/azureidentity.Validate /Users/runner/work/coder/coder/coderd/azureidentity/azureidentity.go:75 - x509: “metadata.azure.com” certificate is not standards compliant ``` When `crypto/x509.VerifyOptions.Roots` is `nil`, Go's verifier on macOS/iOS falls back to the system verifier (`systemVerify` in `crypto/x509/root_darwin.go`), which delegates to Apple's `SecTrustEvaluateWithError`. Apple's framework enforces stricter standards-compliance checks than Go's pure-Go verifier and rejects some otherwise valid Azure instance-identity leaf certificates with `errSecCertificateIsNotStandardsCompliant`, surfaced as the `not standards compliant` error. The test had been skipped on darwin since #12979 (April 2024) as a workaround. ## Fix - Embed the three root CAs that Azure instance-identity certificates ultimately chain to: - DigiCert Global Root G2 - DigiCert Global Root G3 - Baltimore CyberTrust Root (kept for historical chains via `Microsoft RSA TLS CA 01/02`) - In `Validate`, populate `options.Roots` from those embedded roots when the caller does not supply its own pool. Because `Roots != nil`, Go no longer takes the `systemVerify` path on darwin and uses the pure-Go verifier on all platforms. - Remove the `runtime.GOOS == "darwin"` skip from `TestValidate`. - Add `TestEmbeddedRoots` to guard against future regressions in the embedded root list (parses each PEM, asserts self-signed, requires all three named roots). The caller's existing `Intermediates` handling is unchanged. Tests that pass their own `Roots` (e.g. `coderdtest.NewAzureInstanceIdentity`) are unaffected. ## Verification On Linux: ``` $ go test ./coderd/azureidentity/ -race -count=1 -v === RUN TestValidate === RUN TestValidate/regular === RUN TestValidate/govcloud === RUN TestValidate/rsa --- PASS: TestValidate (0.00s) --- PASS: TestValidate/regular (0.00s) --- PASS: TestValidate/rsa (0.00s) --- PASS: TestValidate/govcloud (0.00s) === RUN TestEmbeddedRoots --- PASS: TestEmbeddedRoots (0.00s) === RUN TestExpiresSoon --- SKIP: TestExpiresSoon (0.00s) PASS ok github.com/coder/coder/v2/coderd/azureidentity 1.020s ``` The `test-go-pg` job on `macos-latest` in CI is the authoritative confirmation of the fix on macOS; previously it would have failed `TestValidate/regular` had the skip been removed. <details> <summary>Why this is the correct fix</summary> From `/usr/local/go/src/crypto/x509/verify.go`: ```go // Use platform verifiers, where available, if Roots is from SystemCertPool. if runtime.GOOS == "windows" || runtime.GOOS == "darwin" || runtime.GOOS == "ios" { systemPool := systemRootsPool() if opts.Roots == nil && (systemPool == nil || systemPool.systemPool) { return c.systemVerify(&opts) } ... } ``` Setting `opts.Roots` to any non-nil, non-system pool deterministically routes verification through Go's pure-Go verifier, bypassing Apple's stricter compliance checks. The embedded roots are sufficient to validate every chain we currently care about, since every intermediate in `Certificates` ultimately issues to one of the three embedded roots. </details> > Generated by Coder Agents. Reviewed manually. |
||
|
Kyle Carberry
|
aaa0dacdb3 |
fix: infer workspace claim time from build history for /agents delete dialog (#25057)
Closes [CODAGT-317](https://linear.app/codercom/issue/CODAGT-317/pr-workspaces-sometimes-require-name-confirmation-to-delete). ## Problem The `/agents` archive-and-delete molly-guard (typing the workspace name) was firing for chats that had clearly created their own workspace. The heuristic in `resolveArchiveAndDeleteAction` decides whether confirmation is needed by comparing the workspace's `created_at` against the chat's `created_at`: ```ts return new Date(workspaceCreatedAt) >= new Date(chatCreatedAt); ``` That assumption breaks for **prebuilt workspaces**. `ClaimPrebuiltWorkspace` rewrites `owner_id`, `name`, `updated_at`, `last_used_at`, etc., but **never touches `created_at`**, which still reflects when the prebuild was provisioned by the reconciler, often hours before the chat exists. Result: every prebuild-claimed workspace looks pre-existing, so the molly-guard fires. Concrete example from a real chat: | Field | Value | |---|---| | `chat.created_at` | `2026-05-07T15:12:23Z` | | `workspace.created_at` (provision) | `2026-05-07T14:22:24Z` | | `latest_build.created_at` (claim) | `2026-05-07T15:19:09Z` | `14:22:24 < 15:12:23` so `isWorkspaceAutoCreated` returned false even though the chat issued the claim. ## Fix (frontend-only) Derive the moment a workspace was acquired from existing build history rather than relying on `workspace.created_at`: - Build #1 initiator = prebuilds system user → workspace was a prebuild → use `build_2.created_at` (the claim build) as the acquisition time. - Build #1 initiator = real user → workspace was created from scratch → use `workspace.created_at` (unchanged behavior). - Unclaimed prebuild or no build history → return `null` (force confirmation; safe degradation for a destructive flow). The resolver fetches the build list via the existing `getWorkspaceBuilds` endpoint when the dialog might fire. No new column, no migration, no schema change. Works retroactively for all existing claimed prebuilds; no backfill needed. The prebuilds system user UUID is exposed via `codersdk.PrebuildsSystemUserID` and typegen'd to `typesGenerated.ts`. `coderd/database.PrebuildsSystemUserID` parses that constant via `uuid.MustParse` so the two cannot drift; if the codersdk literal ever changes, package init fails fast. ## History The first draft of this PR added a `workspaces.claimed_at` column populated by `ClaimPrebuiltWorkspace`. After review feedback from @johnstcn pointing out that the same fact is already implicit in build history, I pivoted to the frontend-only approach. Subsequent review notes consolidated the prebuilds system user UUID into a single typegen'd constant. ## Why not the other open PRs - **#25055** (`chatKey` cache fallback) only fixes a different cache-miss path; it explicitly notes it does not address `created_at < chat.created_at`. - **#25053** (`chats.workspace_auto_created` boolean) puts the truth on the wrong side of the schema: "this workspace was claimed at time T" is a property of the workspace, not the chat. The MCP plumbing it adds is also unnecessary now that the same answer is available from build history. ## Test plan - `pnpm vitest run --project=unit src/pages/AgentsPage/utils/agentWorkspaceUtils.test.ts` — 40/40 pass; new cases cover prebuild claim before/after chat, unclaimed prebuild, missing-build-history fallback, and the fetch-skip when the chat is not in cache. - `pnpm lint:types`, `pnpm check`, `make pre-commit`. <details> <summary>Disclosure</summary> Opened on behalf of @kylecarbs by [Coder Agents](https://coder.com/coder-agents). </details> |
||
|
Kyle Carberry
|
f0fd2111fd |
feat(site/src/pages/AgentsPage): render markdown attachments in preview popup (#24936)
Markdown attachments on `/agents` now render through the same `Response` component used for chat messages instead of falling back to a monospaced `<pre>` block. The popup detects markdown via an explicit `text/markdown` media type and falls back to the `.md`/`.markdown` filename extension when no media type is available. `PreviewTextAttachment` and `TextPreviewDialog` gain an optional `mediaType` so that callers (`AttachmentBlock` for already-sent messages and `AttachmentPreview` for live drafts) can plumb the upload metadata through. Plain `.txt` and unrecognized text attachments keep the existing monospaced rendering. ## Demo  ## Screenshots | Markdown rendering | Plain text rendering | | --- | --- | |  |  | Light theme also verified:  <details> <summary>Coverage details</summary> New stories in `TextPreviewDialog.stories.tsx` cover: - `MarkdownByExtension` — `.md` filename, headings/lists/tables/fenced code render natively. - `MarkdownByMediaType` — explicit `text/markdown` mediaType wins even without a `.md` suffix. - `MarkdownProseOnly` — inline `**bold**`, `_italic_`, and `` `code` `` render via streamdown. - `PlainTextStaysMonospaced` — `.txt` content stays inside `<pre>` so existing previews don't regress. Manual verification (desktop, Chromium, dark + light): all four stories above plus the existing `Default`, `LongContent`, and `NoFileName` stories pass. </details> _Coder Agents generated PR._ |
||
|
Kyle Carberry
|
1ecdad689b |
fix(site/src/pages/AgentsPage): restore sticky user message pinning after react-infinite-scroll-component refactor (#24937)
Restores the sticky user message pinning behavior in the Agents chat that regressed after #24687 swapped the chat scroll container for `react-infinite-scroll-component`. ## Root cause `react-infinite-scroll-component` renders two wrapper divs between the `.overflow-y-auto` scroller and the rendered messages, and its inner wrapper hard-codes `overflow: auto` in its inline style. With the new layout, `position: sticky` on a user message resolved against that inner wrapper rather than the real scroller, so the message scrolled out with its sentinel and the existing fade/clip overlay never engaged. ## Fix Force both InfiniteScroll wrappers to `display: contents` so they no longer participate in layout. The user message's nearest scrolling ancestor is once again the `.overflow-y-auto` element, and `position: sticky` anchors to the scroll container as it did before #24687. The outer wrapper is reached via the Tailwind arbitrary selector `[&>[class$=outerdiv]]:contents` because the library only exposes `style` for the inner wrapper. The inverse infinite-scroll behavior is preserved: the scroller itself stays `flex-col-reverse`, so it remains bottom-anchored and the library's load-more sentinel still lands at the visual top of the content stack. Also drops the dead `overflow-y-auto` class on the floating scroll-to-bottom button wrapper noted in the bug report. ## Test coverage Adds `StickyUserMessagePinsOnScroll` to `AgentChatPageView.stories.tsx`. With a 40-message conversation it walks the user-message sentinels in reverse DOM order to find the one currently pinned (the latest sentinel above the scroller's top edge) and asserts the matching sticky container is anchored within a few pixels of that edge. Without the fix the container ends up hundreds of pixels above the scroller because `position: sticky` silently no-ops. The existing structural `StickyUserMessageStructure` story in `ConversationTimeline.stories.tsx` continues to pass unchanged. <details> <summary>Verification</summary> ```sh pnpm exec tsc -p . # 0 errors pnpm run lint:check # passes pnpm exec vitest run --project=unit # 2303 passed pnpm exec vitest run --project=storybook \ src/pages/AgentsPage/AgentChatPage.stories.tsx \ src/pages/AgentsPage/AgentChatPageView.stories.tsx \ src/pages/AgentsPage/components/ChatConversation/ConversationTimeline.stories.tsx # 90 passed ``` Confirmed the new story fails on `main` (sticky container at the sentinel's position instead of the scroller top) and passes with the fix applied. </details> --- Generated by Coder Agents. |
||
|
Kyle Carberry
|
c16034d127 |
fix(site/src/pages/AgentsPage/components): match chat input font size to message bubbles (#24928)
The `/agents` chat composer rendered user input at 15px, but once a message was sent the user message bubble displays content at 13px (via `MessageContent` in `ChatElements/Message.tsx`). The size jump made what you typed not match what got displayed. Sets the `ChatMessageInput` className inside `AgentChatInput` to `text-[13px] leading-relaxed` so the composer matches the rendered user message. <sub>This PR was opened by a Coder agent on behalf of @kylecarbs.</sub> |
||
|
Kyle CarberryandCoder Agents
|
d889ba1842 |
feat: add user_oidc auth type for MCP servers (#24793)
Adds a 5th MCP server authentication mode, `user_oidc` ("User OIDC
Identity"), that forwards the calling user's OIDC access token from
`user_links.oauth_access_token` to the upstream MCP server as
`Authorization: Bearer <token>`.
The token is read from `user_links` and refreshed transparently via
`oauth2.TokenSource` before each MCP request. No new per-MCP-server
secret storage and no per-user connect/disconnect step.
**Limitation**: only users who logged in via OIDC have a forwardable
token. Users authenticated via password or GitHub will see requests sent
without an `Authorization` header, and the upstream MCP server is
expected to respond with 401. A pluggable token source (e.g. CLI-minted
E2E tokens) is left as future work.
<details>
<summary>Implementation notes</summary>
- Schema: new
`coderd/database/migrations/000481_mcp_user_oidc_auth.{up,down}.sql`
relaxes the `mcp_server_configs.auth_type` CHECK constraint to include
`user_oidc`. Down migration deletes affected rows before restoring the
old constraint.
- SDK validation: `codersdk/mcp.go` extends `oneof` for
`CreateMCPServerConfigRequest` and `UpdateMCPServerConfigRequest`.
- Handler: `coderd/mcp.go` adds `case "user_oidc":` to the
field-clearing switch on update. The existing list and detail handlers
already report `auth_connected = true` for any non-`oauth2` auth type.
- Header construction: `coderd/x/chatd/mcpclient/mcpclient.go`
introduces a `UserOIDCTokenSource` interface and adds the `user_oidc`
case to `buildAuthHeaders`. `ConnectAll` / `connectOne` /
`buildAuthHeaders` gain `userID uuid.UUID, oidcSrc UserOIDCTokenSource`
parameters.
- Wiring: `coderd/x/chatd/chatd.go` adds `OIDCTokenSource` to `Config` /
`Server` and passes `chat.OwnerID` plus the source through `ConnectAll`.
`coderd/coderd.go` constructs the source next to the `chatd.New` call
when `options.OIDCConfig` is non-nil.
- Token source: `oidcMCPTokenSource` lives in `coderd/mcp.go`. It reads
the user's OIDC link, refreshes via `oauth2.TokenSource`, and writes the
refreshed token back to `user_links`. Logic is duplicated from
`provisionerdserver.ObtainOIDCAccessToken` to avoid an MCP ->
provisionerdserver dependency. The two copies must be kept in sync; a
comment on `oidcMCPTokenSource` records this.
- Frontend: `MCPServerAdminPanel.tsx` adds the new dropdown option, an
explanatory helper block (no admin-configurable fields), and a Storybook
story (`CreateServerUserOIDC`).
- Tests:
- `mcpclient_test.go`: `TestConnectAll_UserOIDCAuth`,
`TestConnectAll_UserOIDCAuth_NoLink`,
`TestConnectAll_UserOIDCAuth_NilSource`. All existing tests updated for
the new signature.
- `mcp_test.go`: extends `TestMCPServerConfigsAuthConnected` to assert
`auth_connected=true` for `user_oidc`; adds
`TestMCPServerConfigsUserOIDCClearsFields` and
`TestMCPServerConfigsUserOIDCDirect`.
- Docs: `docs/ai-coder/agents/platform-controls/mcp-servers.md`
describes the new mode and its OIDC-only limitation.
</details>
This PR was created by Coder Agents.
---------
Co-authored-by: Coder Agents <agents@coder.com>
|
||
|
Kyle Carberry
|
4a91656fe5 |
refactor(site/src/pages/AgentsPage): align tool-call and message styling (#24790)
Tighten visual rhythm and typography in the agent chat page so tool calls, reasoning, and assistant text share the same baseline. ## Highlights - Unify font size to **13px** across user messages, assistant `Response`, reasoning, and every tool-call label. - Reuse the `text-content-secondary → hover:text-content-primary` transition on tool-call rows so labels, chevrons, and lucide icons brighten together. Icons inside hover-aware headers switch to `text-current` so they inherit the parent transition; static icons in non-collapsible cards (`ExecuteTool`, `ProcessOutputTool`) keep the constant secondary color. - Collapse padding between adjacent tool/thinking blocks via a shared `data-tool-call` attribute and adjacent-sibling selectors (`[&:has(+[data-tool-call])]:pb-0` + `[[data-tool-call]+&]:pt-0`). First/last items keep their padding against text and reasoning siblings. - `read_file` now mirrors `write_file`: `Reading <name>…` while running, `Read <name>` once complete. - `ask_user_question` flips the inline label from `Asking:` to `Asked:` once answered. - Subagent row layout: status icon + label + chevron sit together at the start of the row, while the `Worked for <duration>` text uses `ml-auto` to anchor the right edge. - New Storybook story `WithEveryTool` (under `Pages / AgentsPage / AgentChatPage`) exercises every tool renderer plus subagent variants and the generic MCP fallback in a single completed-then-streaming turn. --- _Authored with help from a Coder Agent._ |
||
|
Kyle Carberry
|
ad3095106d |
fix(site/src/pages/AgentsPage): align thinking disclosure (#24743)
The agent chat thinking disclosure used a smaller label with the caret on the left, which made collapsed and in-progress thinking look different from collapsible tool calls. Align the thinking disclosure with the shared tool-call row treatment by using the same label size, trailing caret placement, and hover color while preserving the streaming shimmer. Adds a Storybook story that renders collapsed thinking next to a tool call. <details> <summary>Storybook screenshots</summary> Captured from Storybook: - `pages/AgentsPage/ChatConversation/ConversationTimeline/ThinkingBlockWithToolCall` - `pages/AgentsPage/ChatConversation/ConversationTimeline/ThinkingBlockWithToolCall` hovered - `pages/AgentsPage/ChatConversation/StreamingOutput/ThinkingDuringStreamingWithToolCalls` Screenshots are attached in the Coder task. </details> Generated by Coder Agents. |
||
|
Kyle Carberry
|
069223ae26 | fix: recover web push subscriptions after PWA reinstall (#24720) | ||
|
Kyle Carberry
|
8bc91d982f | fix(site): truncate long workspace name in chat input toolbar (#24412) | ||
|
Kyle Carberry
|
9c74c8c674 | fix: move OnChatUpdated call after agent is ready in create/start workspace (#24410) | ||
|
Kyle Carberry
|
d11849d94a |
fix: re-fetch context files and skills from workspace on each turn (#24360)
Context files (AGENTS.md) and skills were only fetched from the
workspace on the first turn or when the agent changed. On subsequent
turns, stale content from persisted messages was used. This meant that
if AGENTS.md or skills were modified on the workspace between turns, the
agent wouldn't see the changes until the user created a new chat.
## Changes
- Extract `fetchWorkspaceContext` from `persistInstructionFiles` to
allow fetching workspace context without persisting
- On subsequent turns, re-fetch fresh context from the workspace instead
of reading stale persisted content; falls back to persisted messages if
the workspace dial fails
- Update `ReloadMessages` callback to re-derive instruction and skills
from reloaded database messages after compaction, instead of using
captured closure variables
- Add `formatSystemInstructionsFromParts` helper to build system
instructions directly from agent parts without requiring separate
OS/directory params
- Add tests for the new helper
<details><summary>Implementation Notes</summary>
### Root cause
In `runChat`, the `else if hasContextFiles` branch (subsequent turns)
called `instructionFromContextFiles(messages)` which read stale content
from persisted DB messages. The `ReloadMessages` callback
(post-compaction) also used captured `instruction`/`skills` closure
variables from the start of the turn, never re-deriving them.
### Approach
1. **Extract `fetchWorkspaceContext`** — Pure refactor of the fetch-only
part of `persistInstructionFiles` (agent connection, context config
retrieval, content sanitization, metadata stamping). Returns parts +
skills without persisting.
2. **Subsequent turns**: Instead of reading from persisted messages,
launch a `g2` goroutine that calls `fetchWorkspaceContext` to get fresh
context from the workspace. Falls back gracefully to persisted messages
if the workspace is unreachable.
3. **ReloadMessages**: Re-derive `instruction` from
`instructionFromContextFiles(reloadedMsgs)` and `skills` from
`skillsFromParts(reloadedMsgs)` using the freshly loaded messages, with
fallback to captured values if the reloaded messages don't contain
context (e.g. compacted away).
</details>
> 🤖 Generated by Coder Agents
|
||
|
Kyle Carberry
|
a414d37165 |
feat(site/src/pages/AgentsPage): add full-width chat layout toggle (#24307)
Adds a user preference to remove the `max-w-3xl` constraint on agent chat messages, letting the content fill the available viewport width. The toggle lives in **Agents > Settings > Behavior** under "Chat Layout" and persists via `localStorage` (`agents.chat-full-width`). A `useSyncExternalStore` hook (`useChatFullWidth`) provides same-tab reactivity so flipping the toggle updates all mounted consumers immediately — the chat timeline, chat input, the Suspense skeleton, and the in-page loading view. This was requested by a customer and is an individual user setting, so it seems fine to add. |
||
|
Kyle Carberry
|
69917b4516 |
fix: resolve flaky TestWatchChats/DiffStatusChangeIncludesDiffStatus (#24298)
## Problem `TestWatchChats/DiffStatusChangeIncludesDiffStatus` flakes with: ``` failed to read JSON message: failed to get reader: context deadline exceeded ``` Tracker: https://github.com/coder/internal/issues/1467 ## Root Cause The test published a pubsub event **once**, immediately after `client.Dial()` returned. However, `Dial` completes as soon as the HTTP upgrade finishes — before the server-side `watchChats` handler has called `SubscribeWithErr` on the pubsub channel. When the publish races ahead of the subscription, the message is silently dropped and the `wsjson.Read` loop blocks until the context deadline. ## Fix Publish the event on a short ticker (`testutil.IntervalFast` = 25ms) in a background goroutine instead of publishing once. This guarantees that at least one publish lands **after** the subscription is active, regardless of goroutine scheduling. The goroutine is cleanly stopped via a `publishDone` channel once the expected event is received. ## Verification - `go test -race -count=50 -run TestWatchChats/DiffStatusChangeIncludesDiffStatus` → **50/50 PASS, 0 races** - `go test -race -count=5 -run TestWatchChats` → **all 5 subtests pass 5/5 times** |
||
|
Kyle Carberry
|
8dff1cbc57 |
fix: resolve idle timeout recording test flake on macOS (#24240)
Fixes https://github.com/coder/internal/issues/1461 Two synchronization issues caused `TestPortableDesktop_IdleTimeout_StopsRecordings` (and the `MultipleRecordings` variant) to flake on macOS CI: 1. **`clk.Advance(idleTimeout)` was not awaited.** In `MultipleRecordings`, both idle timers fire simultaneously but their `fire()` goroutines race to remove themselves from the mock clock's event list. Without `MustWait`, the second timer may still be in `m.all` when the next `Advance` is called, causing `"cannot advance ... beyond next timer/ticker event in 0s"`. 2. **The test depended on SIGINT being handled promptly.** After the `stop_timeout` timer was released, the test relied entirely on the shell process handling SIGINT (via `rec.done`). On macOS, `/bin/sh` may not interrupt `wait` reliably, leaving `lockedStopRecordingProcess` blocked in its `select` while holding `p.mu` — deadlocking the `require.Eventually` callback. ### Fix Wait for each `Advance` to complete and advance past the 15s stop timeout so the process is forcibly killed via the timer path, independent of signal handling. Verified with 1000 iterations (500 per test) with zero failures. > Generated with [Coder Agents](https://coder.com/agents) |
||
|
Kyle CarberryandMichael Suchacz
|
391b22aef7 |
feat: add CLI commands for managing chat context from workspaces (#24105)
Adds `coder exp chat context add` and `coder exp chat context clear` commands that run inside a workspace to manage chat context files via the agent token. `add` reads instruction and skill files from a directory (defaulting to cwd) and inserts them as context-file messages into an active chat. Multiple calls are additive — `instructionFromContextFiles` already accumulates all context-file parts across messages. `clear` soft-deletes all context-file messages, causing `contextFileAgentID()` to return `!found` on the next turn, which triggers `needsInstructionPersist=true` and re-fetches defaults from the agent. Both commands auto-detect the target chat via `CODER_CHAT_ID` (already set by `agentproc` on chat-spawned processes), or fall back to single-active-chat resolution for the agent. The `--chat` flag overrides both. Also adds sub-agent context inheritance: `createChildSubagentChat` now copies parent context-file messages to child chats at spawn time, so delegated sub-agents share the same instruction context without independently re-fetching from the workspace agent. <details><summary>Implementation details</summary> **New files:** - `cli/exp_chat.go` — CLI command tree under `coder exp chat context` **Modified files:** - `agent/agentcontextconfig/api.go` — `ConfigFromDir()` reads context from an arbitrary directory without env vars - `codersdk/agentsdk/agentsdk.go` — `AddChatContext`/`ClearChatContext` SDK methods - `coderd/workspaceagents.go` — POST/DELETE handlers on `/workspaceagents/me/chat-context` - `coderd/coderd.go` — Route registration - `coderd/database/queries/chats.sql` — `GetActiveChatsByAgentID`, `SoftDeleteContextFileMessages` - `coderd/database/dbauthz/dbauthz.go` — RBAC implementations for new queries - `coderd/x/chatd/subagent.go` — `copyParentContextFiles` for sub-agent inheritance - `cli/root.go` — Register `chatCommand()` in `AGPLExperimental()` **Auth pattern:** Uses `AgentAuth` (same as `coder external-auth`) — agent token via `CODER_AGENT_TOKEN` + `CODER_AGENT_URL` env vars. </details> > 🤖 Generated by Coder Agents --------- Co-authored-by: Michael Suchacz <203725896+ibetitsmike@users.noreply.github.com> |
||
|
Kyle CarberryandEthan
|
35c26ce22a |
feat: add CreatedAt to tool-call and tool-result ChatMessageParts (#24101)
Adds an optional `CreatedAt` timestamp to `tool-call` and `tool-result` `ChatMessagePart` variants so the frontend can compute tool execution duration (`result.created_at - call.created_at`). Timestamps are recorded at the correct moments in the chatloop: - **Tool-call**: when the model stream emits the tool call - **Tool-result**: when tool execution completes (or is interrupted) These are passed through `PersistedStep.PartCreatedAt` so the persistence layer can apply accurate timestamps to stored parts. SSE-published parts also carry `CreatedAt` for real-time display. Old persisted messages without `created_at` deserialize to `nil` — fully backward compatible. <details><summary>Implementation notes (Coder Agents generated)</summary> ### Why not stamp in `PartFromContent`? `PartFromContent` is called both for SSE publishing (correct timing) and during persistence (wrong timing — both tool-call and tool-result would get the same "persistence time" timestamp, yielding ~0 duration). Instead, timestamps are captured in the chatloop at the right moments and carried through `PersistedStep.PartCreatedAt` as a `map[string]time.Time` keyed by `"call:<id>"` / `"result:<id>"`. ### Interrupted tool calls `persistInterruptedStep` also stamps `CreatedAt` on synthetic error results for cancelled/interrupted tool calls, so partial duration is available. ### Files changed | File | Change | |------|--------| | `codersdk/chats.go` | Add `CreatedAt *time.Time` field | | `codersdk/chats_test.go` | JSON round-trip test | | `coderd/database/dbtime/dbtime.go` | Add `TimePtr` helper | | `coderd/x/chatd/chatloop/chatloop.go` | Track timestamps, pass through `PersistedStep` | | `coderd/x/chatd/chatd.go` | Apply timestamps during persistence | | `coderd/x/chatd/chatprompt/chatprompt_test.go` | Verify `PartFromContent` does NOT stamp | | `site/src/api/typesGenerated.ts` | Auto-generated | </details> --------- Co-authored-by: Ethan <39577870+ethanndickson@users.noreply.github.com> |
||
|
Kyle Carberry
|
b969d66978 |
feat: add dynamic tools support for chat API (#24036)
Adds client-executed dynamic tools to the chat API. Dynamic tools are
declared by the client at chat creation time, presented to the LLM
alongside built-in tools, but executed by the client rather than chatd.
This enables external systems (Slack bots, IDE extensions, Discord bots,
CI/CD integrations) to plug custom tools into the LLM chat loop without
modifying chatd's built-in tool set.
Modeled after OpenAI's Assistants API: the chat pauses with
`requires_action` status when the LLM calls a dynamic tool, the client
POSTs results back via `POST /chats/{id}/tool-results`, and the chat
resumes.
See [this example](https://github.com/coder/coder-slackbot-poc) as a
reference for how this is used. It's highly-configurable, which would
enable creating chats from webhooks, periodically polling, or running as
a Slackbot.
<details>
<summary>Design context</summary>
### Architecture
The chatloop **exits** when it encounters dynamic tools and
**re-enters** when results arrive. No blocking channels, no pubsub for
tool results, no in-memory registry. The DB is the only coordination
mechanism.
```
Phase 1 (chatloop):
LLM response → execute built-in tools only →
Persist(assistant + built-in results) →
status = requires_action → chatloop exits
Phase 2 (POST /tool-results):
Persist(dynamic tool results) →
status = pending → wakeCh → chatloop re-enters
```
### Validation (POST /tool-results)
1. Chat status must be `requires_action` (409 if not)
2. Read chat's `dynamic_tools` → set of dynamic tool names
3. Read last assistant message → extract tool-call parts matching
dynamic tool names
4. Submitted tool_call_ids must match exactly (400 for missing/extra)
5. Persist tool-result message parts, set status to `pending`, signal
wake
### Idempotency
Tool call IDs scoped per LLM step. State machine (`requires_action` →
`pending`) is the guard. First POST wins, subsequent get 409.
### Mixed tool calls
When the LLM calls both built-in and dynamic tools in one step, built-in
tools execute immediately. Their results are persisted in phase 1.
Dynamic tool results arrive via POST in phase 2. The LLM sees all
results when the chatloop resumes.
</details>
> 🤖 Generated by Coder Agents
|
||
|
Kyle Carberry
|
c5d720f73d |
feat(coderd): add telemetry for agents chats and messages (#24068)
Adds telemetry collection for the agents chat system (`/agents`) to the
existing telemetry snapshot pipeline.
Three new snapshot fields:
- **`Chats`** — per-chat metadata (id, owner, status, mode,
workspace_id, root_chat_id, has_parent, archived, model config)
collected time-windowed via `createdAfter`
- **`ChatMessageSummaries`** — per-chat aggregated message metrics
(counts by role, token sums by type, cost, runtime, model count,
compression count) collected time-windowed
- **`ChatModelConfigs`** — model configuration metadata (provider,
model, context limit, enabled, default) collected as full dump
No PII is included — titles, message content, and URLs are excluded at
the SQL level. Only structural metadata flows through telemetry.
<details><summary>Implementation plan</summary>
### SQL Queries (`coderd/database/queries/chats.sql`)
- `GetChatsCreatedAfter` — time-windowed chat metadata
- `GetChatMessageSummariesPerChat` — per-chat message aggregates via
`GROUP BY`
- `GetChatModelConfigsForTelemetry` — full dump of model configs
### Telemetry (`coderd/telemetry/telemetry.go`)
- `Chat`, `ChatMessageSummary`, `ChatModelConfig` structs
- `ConvertChat`, `ConvertChatMessageSummary`, `ConvertChatModelConfig`
conversion functions
- Three `eg.Go()` blocks in `createSnapshot()` following the existing
collection pattern
### Authorization (`coderd/database/dbauthz/dbauthz.go`)
- System-only access for all three queries via `rbac.ResourceSystem`
### Tests
- `TestChatsTelemetry` in `coderd/telemetry/telemetry_test.go` — creates
chats (root + child), messages with token/cost data, model configs;
verifies all snapshot fields
- dbauthz test entries for all three queries in
`coderd/database/dbauthz/dbauthz_test.go`
</details>
> 🤖 Generated by Coder Agents
|
||
|
Kyle Carberry
|
f3f0a2c553 |
fix(enterprise/coderd/x/chatd): harden TestSubscribeRelayEstablishedMidStream against CI flakes (#24108)
Fixes coder/internal#1455 Three changes to eliminate the timing-sensitive flake in `TestSubscribeRelayEstablishedMidStream`: 1. **Reduce `PendingChatAcquireInterval` from `time.Hour` to `time.Second`.** The primary trigger is still `signalWake()` from `SendMessage`, but a short fallback poll ensures the worker picks up the pending chat even under heavy CI goroutine scheduling contention. 2. **Increase context timeout from `WaitLong` (25s) to `WaitSuperLong` (60s).** The worker pipeline (model resolution, message loading, LLM call) involves multiple DB round-trips that can be slow when PostgreSQL is shared with many parallel test packages. 3. **Add a status-polling loop while waiting for the streaming request.** If the worker errors out during chat processing, the test now fails immediately with the error status and message instead of silently timing out. > Generated by Coder Agents |
||
|
Kyle Carberry
|
cffc68df58 | feat(site): render read_skill body as markdown (#24069) | ||
|
Kyle Carberry
|
16265e834e |
chore: update fantasy fork to use github.com/coder/fantasy (#24100)
Moves the `charm.land/fantasy` replace directive from `github.com/kylecarbs/fantasy` to `github.com/coder/fantasy`, pointing at the same `cj/go1.25` branch and commit (`112927d9b6d8`). > Generated by Coder Agents |
||
|
Kyle Carberry
|
684f21740d |
perf(coderd): batch chat heartbeat queries into single UPDATE per interval (#24037)
## Summary
Replaces N per-chat heartbeat goroutines with a single centralized
heartbeat loop that issues one `UPDATE` per 30s interval for all running
chats on a worker.
## Problem
Each running chat spawned a dedicated goroutine that issued an
individual `UPDATE chats SET heartbeat_at = NOW() WHERE id = $1 AND
worker_id = $2 AND status = 'running'` query every 30 seconds. At 10,000
concurrent chats this produces **~333 DB queries/second** just for
heartbeats, plus ~333 `ActivityBumpWorkspace` CTE queries/second from
`trackWorkspaceUsage`.
## Solution
New `UpdateChatHeartbeats` (plural) SQL query replaces the old singular
`UpdateChatHeartbeat`:
```sql
UPDATE chats
SET heartbeat_at = @now::timestamptz
WHERE worker_id = @worker_id::uuid
AND status = 'running'::chat_status
RETURNING id;
```
A single `heartbeatLoop` goroutine on the `Server`:
1. Ticks every `chatHeartbeatInterval` (30s)
2. Issues one batch UPDATE for all registered chats
3. Detects stolen/completed chats via set-difference (equivalent of old
`rows == 0`)
4. Calls `trackWorkspaceUsage` for surviving chats
`processChat` registers an entry in the heartbeat registry instead of
spawning a goroutine.
## Impact
| Metric | Before (10K chats) | After (10K chats) |
|---|---|---|
| Heartbeat queries/sec | ~333 | ~0.03 (1 per 30s per replica) |
| Heartbeat goroutines | 10,000 | 1 |
| Self-interrupt detection | Per-chat `rows==0` | Batch set-difference |
---
> 🤖 Generated by Coder Agents
<details><summary>Implementation notes</summary>
- Uses `@now` parameter instead of `NOW()` so tests with `quartz.Mock`
can control timestamps.
- `heartbeatEntry` stores `context.CancelCauseFunc` + workspace state
for the centralized loop.
- `recoverStaleChats` is unaffected — it reads `heartbeat_at` which is
still updated.
- The old singular `UpdateChatHeartbeat` is removed entirely.
- `dbauthz` wrapper uses system-level `rbac.ResourceChat` authorization
(same pattern as `AcquireChats`).
</details>
|
||
|
Kyle Carberry
|
acd5f01b4b |
fix: use GreaterOrEqual for step runtime assertion in chatloop test (#24067)
Fixes https://github.com/coder/internal/issues/1418 The `TestRun_ActiveToolsPrepareBehavior` test asserts `persistedStep.Runtime > 0`, but on Windows the timer resolution (~15ms) means the in-memory mock model can complete within the same clock tick, producing a measured duration of `0s`. Change the assertion from `require.Greater` to `require.GreaterOrEqual` so that a legitimately measured zero duration on low-resolution clocks does not cause a flake. > Generated by Coder Agents |
||
|
Kyle Carberry
|
6c62d8f5e6 |
fix(coderd/x/chatd): fix flaky TestAwaitSubagentCompletion/CompletesViaPubsub (#24066)
## Fix flaky TestAwaitSubagentCompletion/CompletesViaPubsub Fixes coder/internal#1435 ### Root Cause During `createParentChildChats`, the processor publishes notifications on `ChatStreamNotifyChannel(child.ID)` via PostgreSQL `LISTEN/NOTIFY`. After `drainInflight()` returns, these stale notifications can still be buffered in the pgListener's `NotifyChan()`. When `awaitSubagentCompletion` subscribes and a stale notification is dispatched between `setChatStatus(Waiting)` and `insertAssistantMessage`, `checkSubagentCompletion` sees `done=true` (status is `Waiting`) but returns an empty report because the message hasn't been committed yet. ### Fix Swap the order: insert the assistant message **before** transitioning the status to `Waiting`. This guarantees the report is committed before the status makes the chat appear complete to `checkSubagentCompletion`. ### Verification - 50 consecutive runs of the specific test: all pass - 10 runs of the full `TestAwaitSubagentCompletion` suite: all pass - 20 runs with `-race`: all pass > Generated by Coder Agents |
||
|
Kyle Carberry
|
648787e739 |
feat: expose busy_behavior on chat message API (#24054)
The backend (`chatd.go`) already fully implements both `"queue"` and `"interrupt"` busy behaviors for `SendMessage`, and the `message_agent` subagent tool already leverages both internally. However the HTTP API hardcoded `"queue"` and the SDK had no way for callers to request interrupt-on-send. This adds a `ChatBusyBehavior` enum type to the SDK and an optional `busy_behavior` field on `CreateChatMessageRequest`. The HTTP handler validates the field and passes it through to `chatd.SendMessage`. Default remains `"queue"` for full backward compatibility. <details><summary>Implementation notes</summary> - `codersdk/chats.go`: New `ChatBusyBehavior` type with `ChatBusyBehaviorQueue` and `ChatBusyBehaviorInterrupt` constants. Added `BusyBehavior` field to `CreateChatMessageRequest` with `enums` tag for codegen. - `coderd/exp_chats.go`: `postChatMessages` now reads `req.BusyBehavior`, maps SDK constants to `chatd.SendMessageBusyBehavior*`, returns 400 on invalid values. - `site/src/api/typesGenerated.ts`: Auto-generated via `make gen`. - No frontend behavior changes — the field is available but unused by the UI. </details> > [!NOTE] > Generated by Coder Agents |
||
|
Kyle Carberry
|
4cfbf544a0 |
feat: add per-chat system prompt option (#24053)
Adds a `system_prompt` field to `CreateChatRequest` that allows API consumers to provide custom instructions when creating a chat. The per-chat prompt is stored as a separate system message (`role=system`, `visibility=model`) in the `chat_messages` table, inserted between the deployment system prompt and the workspace awareness message. Also moves deployment system prompt resolution from the HTTP handler (`resolvedChatSystemPrompt`) into `chatd.CreateChat` where it belongs. The handler no longer assembles system prompts — `CreateOptions.SystemPrompt` is now purely the per-chat user prompt, and the deployment prompt is resolved internally by chatd. No database schema changes required. **Message insertion order:** 1. Deployment system prompt (resolved by chatd, existing) 2. Per-chat user system prompt (new, from `CreateOptions.SystemPrompt`) 3. Workspace awareness (existing) 4. Initial user message (existing) 🤖 Generated with [Coder Agents](https://coder.com/agents) |
||
|
Kyle Carberry
|
a2ce74f398 |
feat: add total_runtime_ms to chat cost analytics endpoints (#24050)
Surface the aggregated `runtime_ms` from `chat_messages` through all
four cost analytics queries (summary, per-model, per-chat, per-user).
This is the key billing metric for agent compute time.
The per-chat breakdown already groups by `root_chat_id`, so subagent
runtime is automatically rolled up under the parent chat — no additional
query changes needed.
<details>
<summary>Implementation details</summary>
**SQL** (`coderd/database/queries/chats.sql`): Added
`COALESCE(SUM(cm.runtime_ms), 0)::bigint AS total_runtime_ms` to
`GetChatCostSummary`, `GetChatCostPerModel`, `GetChatCostPerChat`, and
`GetChatCostPerUser`.
**Go SDK** (`codersdk/chats.go`): Added `TotalRuntimeMs int64` to
`ChatCostSummary`, `ChatCostModelBreakdown`, `ChatCostChatBreakdown`,
and `ChatCostUserRollup`.
**Handler** (`coderd/exp_chats.go`): Wired the new field through all
converter functions and the response assembly.
**Tests** (`coderd/exp_chats_test.go`): Updated fixture to seed non-zero
`runtime_ms` values and added assertions for the new field at summary,
per-model, and per-chat levels.
</details>
> 🤖 Generated by Coder Agents
|
||
|
Kyle Carberry
|
500fc5e2a4 |
feat: polish model config form UI (#24047)
Polishes the AI model configuration form (add/edit model) with tighter
layout and better input affordances.
**Frontend changes:**
- Replace "Unset" with "Default" in select dropdowns to communicate
system fallback
- Show pricing fields inline instead of behind a collapsible toggle
- Use flat section dividers (`border-t`) instead of bordered fieldsets
- Move field descriptions into info-icon tooltips to fix input
misalignment
- Add InputGroup adornments: `$` prefix + `/1M` suffix on pricing,
`tokens` suffix on token fields, `%` suffix on compression threshold,
range placeholders on temperature/penalty fields
- Shorter pricing labels (Input, Output, Cache Read, Cache Write)
- Compact JSON textareas (1-row height, resizable)
- Smart grid layouts by field type (3-col provider, 4-col pricing, 3-col
advanced)
- Boolean fields render as a segmented control (Default · On · Off)
instead of a dropdown
**Backend changes:**
- Add `enum` tags to OpenAI `service_tier`
(`auto,default,flex,scale,priority`) and `reasoning_summary`
(`auto,concise,detailed`) so they render as select dropdowns instead of
free-text inputs
> 🤖 Generated by Coder Agents
|
||
|
Kyle Carberry
|
937f50f0ae |
fix: show message action tooltips at bottom on agents page (#24041)
The CopyButton tooltip on `/agents` defaulted to top (Radix default),
while the Edit button already used `side="bottom"`. This adds an
optional `tooltipSide` prop to `CopyButton` and passes `"bottom"` in the
agents `ConversationTimeline` so both tooltips appear below the buttons
consistently.
## Changes
- `CopyButton`: added optional `tooltipSide` prop, forwarded to
`<TooltipContent side={tooltipSide}>`
- `ConversationTimeline`: passed `tooltipSide="bottom"` to the
copy-message `CopyButton`
> Generated by Coder Agents
|
||
|
Kyle Carberry
|
a16755dd66 |
fix: prevent stale REST status from dropping streamed parts (#24040)
The `useEffect` that syncs `chatRecord.status` from React Query unconditionally overwrites the store's `chatStatus`. The `chat(chatId)` query has no `staleTime` (defaults to 0), so it refetches on window focus, remount, etc. If the REST response catches a transient `"pending"` status (e.g. between multi-step tool-call cycles), it regresses `chatStatus` from `"running"` to `"pending"`. Since `shouldApplyMessagePart()` drops ALL parts when status is `"pending"` or `"waiting"`, every incoming `message_part` event is silently discarded — not even buffered. Parts are visible on the WebSocket but nothing renders, and the UI shows "Response is taking longer than expected". A page reload fixes it because a fresh REST fetch returns the current status. **Fix:** Add `wsStatusReceivedRef` — once the WebSocket delivers a status event, it becomes the authoritative source and REST refetches can no longer overwrite it. This mirrors the existing `wsQueueUpdateReceivedRef` pattern already used for queued messages. The ref resets on chat change. > Generated with [Coder Agents](https://coder.com/agents) |
||
|
Kyle Carberry
|
8bdc35f91f |
refactor(site): unify message copy/edit UX across user and assistant messages (#24039)
Aligns the copy/edit action bar so both user and assistant messages use
the same hover-to-reveal pattern.
## Changes
- Replace bifurcated copy UX (inline `afterResponseSlot` for assistant,
floating toolbar for user) with a single unified action bar using
`CopyButton` + optional edit `Button`
- Remove `BlockList` `afterResponseSlot` prop and related machinery
- Remove per-message `copyHovered`/`useClipboard` state and left-border
highlight effect
- Remove `lastAssistantPerTurnIds`/`isTurnActive` computation — all
messages with content get actions on hover
- Hide actions on mid-chain assistant messages (only last in consecutive
chain shows buttons)
- Reduce inter-message gap from `gap-3` to `gap-2`
- Shrink action buttons to `size-6` for tighter vertical spacing
- Add 8px sticky top offset for user messages
> 🤖 Generated by Coder Agents
|
||
|
Kyle Carberry
|
5b32c4d79d |
fix: prevent stdio MCP server subprocess from dying after connect (#24035)
## Problem
MCP servers configured in `.mcp.json` with stdio transport are
discovered successfully (tools appear) but die immediately after
connection, making all tool calls fail.
## Root Cause
In `connectServer`, the subprocess is spawned with `connectCtx` — a
30-second timeout context whose `cancel()` is deferred:
```go
connectCtx, cancel := context.WithTimeout(ctx, connectTimeout)
defer cancel()
if err := c.Start(connectCtx); err != nil { ... }
```
The mcp-go stdio transport calls `exec.CommandContext(connectCtx, ...)`.
When `connectServer` returns, `cancel()` fires, and
`exec.CommandContext` sends SIGKILL to the subprocess. The process
immediately becomes a zombie.
Confirmed by checking `/proc/<pid>/status` after context cancellation:
```
State: Z (zombie)
```
## Fix
Pass the parent `ctx` (which is `a.gracefulCtx` — the agent's long-lived
context) to `c.Start()`. `connectCtx` continues to bound only the
`Initialize()` handshake. The subprocess is cleaned up when the Manager
is closed or the parent context is canceled.
## Regression Test
Added `TestConnectServer_StdioProcessSurvivesConnect` which:
- Spawns a real subprocess (re-execs the test binary as a fake MCP
server)
- Calls `connectServer` and lets it return (internal `connectCtx` gets
canceled)
- Verifies the subprocess is still alive by calling `ListTools`
The test **fails** on the old code with `transport error: context
deadline exceeded` and **passes** with the fix.
> Generated with [Coder Agents](https://coder.com/agents)
|
||
|
Kyle Carberry
|
8625543413 |
feat(coderd/x/chatd): parallelize ConvertMessagesWithFiles with g2 errgroup (#24034)
## Summary Move `ConvertMessagesWithFiles` into the `g2` errgroup so prompt conversion runs concurrently with instruction persistence, user prompt resolution, MCP server connections, and workspace MCP tool discovery. ## Problem In `runChat`, the setup before the first LLM `Stream()` call is sequential across two errgroups: ``` g.Wait() // model + messages + MCP configs ConvertMessagesWithFiles() // sequential — blocked on g2 starting g2.Wait() // instructions + user prompt + MCP connect + workspace MCP ``` `ConvertMessagesWithFiles` can take non-trivial time on conversations with file attachments (batch DB resolution), and it was blocking g2 from starting. ## Fix `ConvertMessagesWithFiles` only reads the `messages` slice (available after `g.Wait()`) and resolves file references via the database. No g2 task reads or writes the `prompt` variable. This makes it safe to overlap with g2: ``` g.Wait() g2.Wait() // now includes ConvertMessagesWithFiles in parallel ``` The `InsertSystem` call for parent chats and the `promptErr` check are deferred to after `g2.Wait()`, preserving correctness. <details><summary>Decision log</summary> - `ConvertMessagesWithFiles` is read-only on `messages` — no mutation, safe for concurrent access - `prompt` and `promptErr` are written only by the conversion goroutine, read only after `g2.Wait()` — no data race - Error from prompt conversion is checked immediately after `g2.Wait()`, before any code that uses `prompt` - `chatloop.Run` now uses `:=` instead of `=` since the prior `err` declaration from `prompt, err :=` was removed </details> > Generated by Coder Agents |
||
|
Kyle Carberry
|
e18094825a | fix: retain message_part buffer for cross-replica relay (#24031) | ||
|
Kyle Carberry
|
919dc299fc |
feat: agent reads context files and discovers skills locally (#23935)
Piggybacks on #23878. Moves instruction file reading and skill discovery from `chatd` (server-side, via multiple `LS`/`ReadFile` round-trips through the agent connection) to the agent itself (local filesystem access). This intentionally drops backward compatibility with older agents that don't support the context-config endpoint. Agents and server are deployed together; there is no rolling-update contract to maintain here. ## What changed The agent's `GET /api/v0/context-config` response now returns `[]ChatMessagePart` directly — the same types chatd persists. This eliminates intermediate type conversions and makes the protocol extensible. | Field | Type | Description | |---|---|---| | `parts` | `[]ChatMessagePart` | Context-file and skill parts, ready to persist | | `working_dir` | `string` | Agent's resolved working directory | Removed from the response: `instructions_dirs`, `instructions_file`, `skills_dirs`, `skill_meta_file`, `mcp_config_files` — the agent reads files locally and returns their content as parts. Removed from chatd: all legacy `LS`/`ReadFile` fallback code (`readHomeInstructionFile`, `readInstructionDirFile`, `DiscoverSkills` via LS, etc). ## Why The previous architecture had the agent resolve paths, serve them over HTTP, then `chatd` make N+1 round-trips back through the agent connection to read files. The agent has direct filesystem access and should just read the files. ## Key design decisions - **Agent returns `ChatMessagePart` directly** — same types chatd persists. No intermediate `InstructionFileEntry`/`SkillEntry` types needed. - **`SkillMeta.MetaFile`** — persisted via `ContextFileSkillMetaFile` on the skill part, so custom meta file names (`CODER_AGENT_EXP_SKILL_META_FILE`) survive across chat turns. - **No pre-read body** — `read_skill` always dials the workspace to fetch the skill body on demand. Simpler than caching the body in the response. - **MCP config paths kept agent-internal** — `MCPConfigFiles()` getter, not sent over the wire. - **No backward compat fallback** — old agents that don't support context-config get no instruction files. This is acceptable since agent and server deploy together. |
||
|
Kyle Carberry
|
7c048d8eb4 |
fix(site): fix "Thinking..." indicator disappearing prematurely (#23933)
The "Thinking..." indicator flickered or failed to appear when the user
sent a message.
## Problem
The server sends `status:pending` before `status:running` when
processing a new message. `selectIsAwaitingFirstStreamChunk` only
accepted `"running"`, so during the pending window the indicator was
hidden. When the optimistic `setChatStatus("running")` from `handleSend`
was overridden by the WS `status:pending` event, the indicator would
flash and disappear.
Secondarily, `StreamingOutput` hid the indicator as soon as
`streamState` became non-null, even when no text/reasoning blocks
existed yet (e.g. only tool-call parts or whitespace-only deltas had
arrived).
## Fix
1. **`chatStore.ts`** — `selectIsAwaitingFirstStreamChunk` now also
accepts `chatStatus === "pending"` when the latest durable message is a
user message (fresh send). Tool-call cycles (where latest =
assistant/tool) remain unaffected.
2. **`StreamingOutput.tsx`** — During streaming, the component keeps
showing "Thinking..." until a text or reasoning block appears, bridging
the visual gap between the startup placeholder and the first visible
content.
3. **`streamState.ts`** — Changed the early-return guard for
text/reasoning parts from `!part.text` to `!part.text?.trim()` so
whitespace-only deltas don't create a non-null `StreamState` with empty
blocks.
<details><summary>Decision log</summary>
- Including `"pending"` in `isAwaitingFirstStreamChunk` was previously
rejected because it caused the 15-second "startup taking longer" warning
during tool-call cycles. The `latestMessage?.role === "user"` guard now
prevents that — during tool cycles the latest durable message is
assistant/tool, not user.
- The `StreamingOutput` streaming-thinking check uses a synthetic
`"starting"` status for `ChatStatusCallout` rather than adding a new
phase to `LiveStatusModel`, keeping the status model clean.
- The whitespace trim fix in `streamState.ts` is defense-in-depth — the
`StreamingOutput` fix handles the rendering gap, but preventing
empty-block `StreamState` creation is the correct behavior at the
source.
</details>
|
||
|
Kyle Carberry
|
7dc81bdef1 |
fix(site): fix sticky user message clipping and fade-in behavior (#23928)
The sticky user message in the chat timeline had two visual issues: 1. **Dead space during scroll** — the clipping calculation subtracted 48px prematurely (`fullHeight - scrolledPast - 48`), causing the message to shrink before its content had actually left the viewport. Removed the offset so clipping begins exactly when content scrolls out of view. 2. **Blur/gradient popping in abruptly** — the `--fade-opacity` variable was a binary 0/1 toggle. Now it ramps 0→1 over the last 40px before `MIN_HEIGHT`, so the blur and bottom gradient only appear when the message is fully compressed. Also added a longer (~25 line) user message to the `WithMessageHistory` story to make the sticky behavior easier to test visually. |
||
|
Kyle Carberry
|
ee855f9618 |
feat: make agent context paths configurable via env vars (#23878)
Replace hardcoded paths for instruction files, skills, and MCP config
with
values read from `CODER_AGENT_EXP_*` environment variables. Template
authors
configure paths via the existing `coder_agent` `env` block. The agent
resolves `~`, relative, and absolute paths locally, then serves the
resolved config over `GET /api/v0/context-config`. `chatd` fetches this
once per workspace attach and falls back to today's defaults for older
agents.
All path env vars are comma-separated, allowing multiple directories:
| Env Var | Default | Controls |
|---|---|---|
| `CODER_AGENT_EXP_INSTRUCTIONS_DIRS` | `~/.coder` | Dirs containing the
instruction file |
| `CODER_AGENT_EXP_INSTRUCTIONS_FILE` | `AGENTS.md` | Instruction file
name |
| `CODER_AGENT_EXP_SKILLS_DIRS` | `.agents/skills` | Skills directories
|
| `CODER_AGENT_EXP_SKILL_META_FILE` | `SKILL.md` | Skill metadata file
name |
| `CODER_AGENT_EXP_MCP_CONFIG_FILES` | `.mcp.json` | MCP config files |
### Example
```hcl
resource "coder_agent" "main" {
os = "linux"
arch = "amd64"
env = {
CODER_AGENT_EXP_INSTRUCTIONS_DIRS = "/opt/company/agent-config,~/.coder"
CODER_AGENT_EXP_INSTRUCTIONS_FILE = "CLAUDE.md"
CODER_AGENT_EXP_SKILLS_DIRS = "/opt/company/ai-skills,.agents/skills"
CODER_AGENT_EXP_MCP_CONFIG_FILES = "/opt/company/mcp.json,.mcp.json"
}
}
```
<details>
<summary>Implementation Details</summary>
### Architecture
Follows the same pattern as MCP tool discovery:
agent resolves locally → exposes via HTTP → chatd consumes.
**Agent-side** (`agent/agentcontextconfig/`):
- `ResolvePath` / `ResolvePaths` handle `~`, relative, and absolute path
forms; returns `""` for relative paths when baseDir is empty
- `Config` reads env vars, falls back to defaults, resolves all paths
- `GET /api/v0/context-config` serves the resolved config as JSON
**chatd-side** (`coderd/x/chatd/`):
- Calls `conn.ContextConfig()` once on first workspace attach
- Falls back to hardcoded defaults on 404 (older agents)
- Iterates instruction dirs, skills dirs using resolved absolute paths
- `LSRelativityRoot` everywhere — no more home/root juggling
### Key design decisions
- **`EXP_` prefix**: env vars use `CODER_AGENT_EXP_*` to indicate
experimental status
- **Plural names**: comma-separated vars use plural names (`DIRS`,
`FILES`); single-value vars use singular (`FILE`)
- **Defaults in `workspacesdk`**: default constants live in
`codersdk/workspacesdk/` so both agent and server reference them without
cross-layer imports
- **`skillMetaFile` persistence**: stored on context-file parts via
`ContextFileSkillMetaFile` and restored on subsequent chat turns so
custom values survive across turns
- **Working dir dedup**: `slices.Contains` guard prevents reading the
same instruction file from both `InstructionsDirs` and the working
directory
- **MCP server dedup**: first-occurrence-wins dedup prevents leaking
duplicate connections from overlapping config files
- **ResolvePath safety**: returns `""` for relative paths when `baseDir`
is empty, so `ResolvePaths` filters them out
### Files changed
| File | Change |
|---|---|
| `agent/agentcontextconfig/` | New package — path resolution + HTTP
endpoint |
| `codersdk/workspacesdk/agentconn.go` | `ContextConfigResponse` type,
default constants, client method |
| `agent/agent.go` + `agent/api.go` | Wire up endpoint, pass config to
MCP |
| `agent/x/agentmcp/manager.go` | Accept `[]string` MCP config paths,
dedup by name |
| `coderd/x/chatd/chatd.go` | Fetch config, thread through, named
returns |
| `coderd/x/chatd/instruction.go` | Accept configurable dir + file name,
`skillMetaFileFromParts` |
| `coderd/x/chatd/chattool/skill.go` | Accept configurable dirs + meta
file |
| `codersdk/chats.go` | `ContextFileSkillMetaFile` field for persistence
|
### Test coverage
- `TestConfig` (4 cases): defaults, custom env vars, whitespace
trimming, comma-separated dirs
- `TestResolvePath` / `TestResolvePaths`: including empty baseDir edge
case
- `TestPersistInstructionFilesFallbackOnOlderAgent`: backward-compat
path when `ContextConfig` returns 404
- `TestChatMessagePartVariantTags`: updated exclusion list for new
internal field
### Backward compatibility
Older agents return 404 for the new endpoint. `chatd` catches this and
falls back to today's defaults via `readHomeInstructionFile` (using
`LSRelativityHome`). Existing workspaces work with no changes.
</details>
|
||
|
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. |
