chore: backport post-2.37 merged PRs to release/2.37 - #28683
Conversation
…s retryable errors (#28470) ## Problem When Gemini's OpenAI-compatible endpoint rejects a model-generated function call server-side, it ends the SSE stream cleanly with the nonstandard finish reason `function_call_filter: MALFORMED_FUNCTION_CALL` after streaming only thought summaries; the rejected call never reaches the wire. chatd treated this as a normal completion: fantasy maps the unrecognized finish reason to `unknown`, the reasoning block never closes (the only non-thought delta is the `</thought>` marker, which the transport seam strips to an empty string, and the openaicompat hook only ends reasoning on a non-empty content delta), so the step accumulates no content and the generation loop finishes the turn as complete. The user sees the model think for ~40 seconds and then nothing: no assistant message, no `last_error`, chat status `waiting`. Observed twice in a row in production on gemini-3.7-flash, with "Resume" reproducing it identically. ## Fix Two independent layers: - `coderd/x/googleopenai`: the stream rewrite now converts any chunk whose `finish_reason` starts with `function_call_filter` into an OpenAI-style SSE `{"error": ...}` event embedding the raw reason. openai-go turns error-bearing events into stream errors, so the failure rides the existing stream-error path instead of ending the stream cleanly. - `coderd/x/chatd/chaterror`: classifies that injected error as retryable (kind `generic`, provider `google`) with a clear user-facing message, so the existing generation retry machinery re-runs the step and persists a `last_error` if retries exhaust. - `coderd/x/chatd/chatloop`: provider-agnostic guard: a step that produced no user-visible content and no tool calls under a finish reason of `unknown`, `error`, `other`, or `tool-calls` (a tool-calls finish that delivered zero calls) now returns a retryable error instead of silently completing the turn. `stop` and `length` finishes keep their existing semantics. Tests cover the seam rewrite (live-capture SSE shape plus standard finish reason passthrough), the new classification, and the chatloop guard (error cases plus preserved stop, length, text, and tool-call behavior). Each layer was red-green verified independently. Remote dogfood UAT ran against this exact commit: normal reasoning and tool-call chats on a real model complete cleanly with no spurious guard errors and no retry loops. The Google-side failure itself is not deterministically triggerable against live Gemini and is owned by the unit tests. > Xum acted on Mike's (@ibetitsmike) behalf. <!-- xum-attribution: model=claude-opus, thinking=enabled --> (cherry picked from commit a48aedd)
## Problem
Editing a user message in an Agent chat silently dropped the MCP server
selection. The composer renders the MCP picker in edit mode and toggles
update local client state, so the picker displayed the new selection,
but the edit request omitted `mcp_server_ids` at every layer (frontend
request builder, `codersdk.EditChatMessageRequest`, the `PATCH
/chats/{chat}/messages/{message}` handler, and `chatd.EditMessage`). The
chat's persisted selection never changed and the regenerated turn ran
without the newly enabled MCP tools. Two dogfood users hit this within
hours; there is no error anywhere and the UI shows the opposite of the
server state.
## Changes
- `codersdk`: add `MCPServerIDs *[]uuid.UUID` to
`EditChatMessageRequest`, mirroring `CreateChatMessageRequest` (nil
preserves the current selection).
- `chatd`: extract the send path's MCP update block into one shared
`applyRequestedMCPServerIDs` helper (explore-subagent snapshot
immutability guard plus Force On enforcement, Cure53 CDM-02-010) and
call it from both `SendMessage` and `EditMessage`, so enforcement cannot
drift between the two paths.
- `coderd`: extract the send handler's request validation (dedupe,
enabled-in-organization check, persisted-ID exemption) into
`normalizeRequestedChatMCPServerIDs` and wire it into the edit handler,
which now threads the selection into `EditMessageOptions`.
- Frontend: the edit request now includes `mcp_server_ids:
[...effectiveMCPServerIds]`, exactly like the send path, making the
picker's displayed state real.
- `make gen` artifacts (swagger, API docs, `typesGenerated.ts`).
## Tests
Each layer is covered and was proven with independent red toggles
(removing one layer's wiring fails only that layer's tests):
- chatd (`TestEditMessage_MCPServerIDs`): edit applies a provided
selection, nil preserves it, an emptied list cannot remove a `force_on`
server, and explore subagent chats keep the spawn-time snapshot.
- API (`TestPatchChatMessage/MCPServerIDsApplied`,
`MCPServerIDsInvalidRejected`): persistence via the endpoint, omission
preserves, unknown IDs get the same 400 as the send path.
- Storybook (`EditAppliesMCPServerSelection`): toggling a server on
during an edit puts it in the edit request payload.
Note: the `AgentChatPage.stories.tsx` story "Queued For Capacity After
Polling" fails locally on current main as well (verified against the
main baseline with this branch's changes reverted); it is unrelated to
this diff.
Remote dogfood UAT ran on the exact head and passed, including proof
that the regenerated turn actually gains the newly enabled MCP server's
tools.
> 🤖 Xum acted on Mike's (@ibetitsmike) behalf. • Model:
`anthropic:claude-fable-5`
<!-- xum-attribution: model=anthropic:claude-fable-5 -->
(cherry picked from commit b6d7653)
…edia stub (#28460) Follow-up to review feedback on #28387. That PR added three hooks for one feature (`useIsBelowLgViewport`, `useIsBelowMdViewport`, and the local single-use `useRightPanelNarrowSuppression`) and stubbed `window.matchMedia` in tests with `Object.defineProperty`. - Replace the two single-purpose viewport hooks with one generic `useMediaQuery(query)`. Callers pass the shared Tailwind-aligned query constants from `utils/mobile.ts`. The one new hook replaces the two deleted ones (net -1). - Inline `useRightPanelNarrowSuppression` into `AgentChatPage`, its only consumer, and drop its `renderHook` unit suite. The behavior stays covered by the narrow-viewport stories; the widening-restore case moved into the `NarrowingSuppressesExpandedPanel` play function. - Rework `testHelpers/matchMedia.ts` to install the stub with `spyOn` from `storybook/test` instead of `Object.defineProperty`. The helper is story-only now (stories run in real Chromium; jsdom has no `matchMedia`), so any future unit test needing a stub should use `vi.stubGlobal` directly. - Encode the feedback in the canonical FE contract so it gets caught during development: `.claude/docs/FRONTEND_PATTERNS.md` now bans new React hooks when an existing hook, a plain function, or component state suffices (FE3) and bans replacing browser globals with `Object.defineProperty` in tests or stories (FE9: `vi.stubGlobal` / `spyOn`), and notes that `renderHook` suites for stateful UI hooks belong in the consuming component's story (FE1). The `frontend-review` skill checklist flags all three. `site/AGENTS.md` is unchanged since it already defers to the patterns doc. Validation: `pnpm check`, `pnpm format:check`, `pnpm lint` (biome, types, knip, circular deps, compiler check), `AgentChatPage.test.ts` (70 passed), story runs for `AgentChatPage.stories.tsx` and `WorkspacePill.stories.tsx` (49 passed in Chromium). > Xum acted on Mike's behalf (@ibetitsmike). <!-- xum-attribution: model=claude-opus-4-6 thinking=high --> (cherry picked from commit 3c32408)
… guidance (#28476) Broad `find_tools` queries such as "linear issues" saturated the hard 20-match cap on every call: a large server's name token-matches every one of its tools (+8 name, +1 server), so ranking was decided by a term carrying no discriminating information and the model got 20 activations per search regardless of intent. Three changes, mirroring what makes xum's `tool_catalog_search` behave well: - **Lower default with a `limit` argument.** Keyword matches default to 10 per call; the new optional `limit` raises that up to the existing hard cap of 20 (non-positive values fall back to the default). Exact `names` are explicit activation requests and bypass the limit up to the hard cap, so naming 15 tools still activates all 15. - **Coverage-first ranking.** Keyword matches sort by distinct query terms matched before raw score, so a query like "linear issues" ranks tools matching both terms above the dozens matching only the server name. - **Model guidance.** `queries` and `names` now carry schema descriptions (capability keywords and what they are matched against; exact-name activation), and the tool description opens by explaining what deferral means, that matches become callable on the next step, and that a `"server: terms"` prefix scopes a query to one server. An earlier revision also inferred a server scope from unprefixed query words ("linear issues" behaving like "linear: issues"). Review kept surfacing edge cases in that inference, and coverage-first ranking already resolves the original saturation complaint, so it was dropped in favor of the explicit prefix. The hard cap stays at 20 so the persisted result keeps fitting under the generic tool-result truncation budget that protects activation-recovery JSON. Budget and reservation accounting are unchanged; the frontend renderer ignores unknown argument fields, so no `site/` change is needed. > [!NOTE] > Xum acted on @ibetitsmike's behalf in this pull request. <!-- xum-attribution: model=claude-opus-4-6 thinking=high --> (cherry picked from commit e244cac)
… session cleanup (#28400) ## Problem During the Aug 19 dev.coder.com incident, chats stalled for up to ~15 minutes per turn. chatd reconnects to every configured MCP server on every generation step, and one configured server (`registry.coder.com/mcp`) was black-holing requests from the deployment's egress IPs: TCP/requests were silently dropped, and each connect attempt hung until the kernel gave up (~125s), far past the nominal 10s connect budget. The budget does not hold because go-sdk v1.7.0 detaches the context inside `StreamableClientTransport.Connect`, and its error paths block on HTTP work bound to that detached context. Reproduced in a test: a bare `Connect` with a 2s deadline against a black-holed server returns after **12s**. Step-end session cleanup (`session.Close()`) runs synchronously in the generation loop and blocks on the same detached requests, so a server that wedges mid-turn stalls every step boundary too. ## Changes - **Enforce the connect budget externally** (`connectOne`): run `Connect` + `ListTools` in a goroutine and select on the budget context. On timeout, abandon the goroutine and leave a reaper that drains its late result and closes any session that still materialized, so nothing leaks and the caller returns within the budget. - **Stop using bare `http.DefaultTransport`**: MCP traffic now rides a cloned transport with a 5s dial timeout (converts SYN black-holes into fast errors) and a 60s `ResponseHeaderTimeout`. `http.Client.Timeout` stays unset so long-lived SSE streams are unaffected. - **Close sessions in detached goroutines during cleanup** (`ConnectAll`'s cleanup func): the sessions are discarded regardless, and a wedged `Close` (DELETE bound to the SDK's detached context) must not stall the generation loop at step boundaries. One deliberate deviation from the incident plan: `ResponseHeaderTimeout` is 60s (matching `toolCallTimeout`) instead of ~10s. The same HTTP client serves tool-call POSTs, and a JSON-response MCP server sends no headers until the tool finishes, so a 10s bound would falsely kill legitimate 10-60s tools that fit today's tool-call budget. The real 10s connect budget is enforced by the external select, not the transport. ## Tests - Acceptance: with a black-holed server plus a healthy one configured, `ConnectAll` returns within the budget and the healthy server's tools are present; closing the black-holed connections makes the reaper exit (red without the fix: 1s budget took 11s). - A slow-but-alive server (300ms/request) still connects. - A server that only responds after the budget: `ConnectAll` returns promptly, the late result is reaped. - Cleanup returns promptly while the session-teardown DELETE is wedged server-side, and the teardown still happens in the background (red without the fix: cleanup blocked 60s). - Transport guard test pinning the dial/response-header bounds. `go test ./coderd/x/chatd/...` passes (including goleak in `chatd`). Part of the MCP connect-stall incident follow-up; observability (connect durations in logs/debug runs) comes in a stacked follow-up PR. > 🤖 Mux authored this PR on Mike's behalf. <!-- mux-attribution: model=anthropic:claude-opus-4-6 thinking=high --> (cherry picked from commit cc958b7)
…of rejecting them (#28589) Chat title generation asks the model for a title in 2-8 words and then hard-rejects any response longer than 8 words (`validateGeneratedTitle`). Quickgen pins temperature for repeatable output, so a model that overshoots the budget for a given conversation overshoots on every retry: the rename dialog's Generate button returns a deterministic 500 (`generate manual title: generated title exceeded 8 words`) no matter how often it is clicked, and the automatic first-message path silently leaves the chat on its fallback title. On dev.coder.com this rejection fires 1-3 times a day; one chat took 13 consecutive manual failures on 2026-08-25 (14:22-14:27 UTC). Truncate the normalized title to the 8-word budget in `normalizeTitleOutput` instead of rejecting it, and keep the empty-title validation. Both the automatic and manual title paths share this normalization, so both are fixed. > Xum (AI agent) authored this change and PR on Mike's behalf. (cherry picked from commit 60b0313)
## Stack context This is the base of a 3-PR stack promoting the chat API from `/api/experimental` to `/api/v2`: server compatibility mounts (this PR), codersdk promotion (#28497), and frontend path updates (#28498). ## Summary Double-mount the stable chat and MCP handlers under `/api/v2` while retaining the existing experimental routes for the one-release compatibility window decided in CODAGT-921. CODAGT-922 tracks removing the compatibility mounts. The shared route builders preserve existing authentication and middleware behavior. Experiment-gated, debug, tombstone, and legacy default-organization model routes remain experimental-only. Signed file URLs, external OAuth callback URLs, and mixed-version replica relays also remain on the experimental prefix during the transition. Update Swagger and the generated API reference for the promoted routes, including the workspace lookup and a runnable raw-body chat file upload example. Retain internal endpoints outside the published reference, share chat-file rate limits across both prefixes, enable CORS for the v2 MCP routes, and cover dual mounts plus exclusions with compatibility tests. Remote dogfood UAT passed for the promoted chat, model, MCP, and file flows. > [!NOTE] > Xum acted on Mike's behalf in this pull request. <!-- xum-attribution: model=claude-fable-5 thinking=high --> (cherry picked from commit 351bb14)
## Stack context This is the second PR in the 3-PR chat API promotion stack: server compatibility mounts (#28496), codersdk promotion (this PR), and frontend path updates (#28498). ## Summary Move the promoted chat and MCP SDK methods from `ExperimentalClient` to `Client` and update them to use `/api/v2`. Methods for routes that remain experimental stay on `ExperimentalClient`. Update in-repository callers and generated types. The multi-replica chat stream relay dials `/api/v2` directly: mixed-version replica sets are not a supported upgrade path, so no experimental-path fallback is kept (per review). Remote dogfood UAT passed for the composed stack. > [!NOTE] > Xum acted on Mike's behalf in this pull request. <!-- xum-attribution: model=claude-fable-5 thinking=high --> (cherry picked from commit 973d5a4)
## Stack context This is the final PR in the 3-PR chat API promotion stack: server compatibility mounts (#28496), codersdk promotion (#28497), and frontend path updates (this PR). ## Summary Switch promoted frontend chat and MCP REST and WebSocket calls to `/api/v2`. Debug runs, virtual desktop streaming, advisor, and computer-use provider routes remain on `/api/experimental` because those surfaces were not promoted. Update the matching tests, stories, helpers, and end-to-end route expectations. Remote dogfood UAT passed with chat traffic verified on the v2 routes. > [!NOTE] > Xum acted on Mike's behalf in this pull request. <!-- xum-attribution: model=claude-fable-5 thinking=high --> (cherry picked from commit 7394d2c)
|
👋 Hey @ibetitsmike! This PR is targeting the Only bug fixes should be cherry-picked to release branches. If this is a bug fix, please update the PR title to match the conventional commit format: If this is not a bug fix, it likely should not target a release branch. |
Docs previewCheck off each page once it's been reviewed. If a page changes in a later push, its checkbox clears automatically so it gets a fresh look. Pages not yet wired into the docs navigation aren't listed here. |
Documentation CheckThis PR promotes the chat/MCP-config API surface from Updates Needed
Note
Automated review via Coder Agents |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 29dd39ad51
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| We make use of a relay mechanism when there are multiple coderd replicas. If a client connects to the stream endpoint on replica A, but the chat worker that owns the chat is on replica B, the endpoint will connect to replica B and relay streaming message parts. | ||
|
|
||
| There exists a `GET /api/experimental/chats/{chat}/stream/parts` endpoint that is responsible exclusively for streaming message parts. That endpoint talks to the chat worker on the same replica to obtain the message parts and relay them to the client. | ||
| There exists a `GET /api/v2/chats/{chat}/stream/parts` endpoint that is responsible exclusively for streaming message parts. That endpoint talks to the chat worker on the same replica to obtain the message parts and relay them to the client. |
There was a problem hiding this comment.
Leave a TODO instead of editing chatd architecture
This directly rewrites the chatd architecture document, but the repository guardrail requires changes affecting documented chatd architecture to leave TODOs in the affected sections and defer the actual documentation update to the human PR author. Revert these prose substitutions and add the required TODOs instead.
AGENTS.md reference: AGENTS.md:L65-L65
Useful? React with 👍 / 👎.
| go func() { | ||
| if late := <-resCh; late.session != nil { | ||
| _ = late.session.Close() |
There was a problem hiding this comment.
Bound abandoned MCP response bodies
When an MCP endpoint sends response headers but never completes its initialization or ListTools response body, ResponseHeaderTimeout no longer applies and the SDK's detached request does not observe connectCtx. This timeout branch returns while both the connect goroutine and this reaper remain blocked forever on resCh; because ConnectAll runs again on later generation steps, one misbehaving server can accumulate goroutines and open connections. Add an operation-specific body deadline or cancellation mechanism before abandoning the request.
Useful? React with 👍 / 👎.
Batch backport of ibetitsmike's PRs that merged into
mainafter therelease/2.37branch was cut (merge-basec275327fb72, 2026-08-24).Included (9 cherry-picks, in order)
All picks applied cleanly with
git cherry-pick -xontorelease/2.37.Excluded and follow-up backports
The remaining post-cut PRs are cherry-picked onto this PR as a GitHub stack (#28692) that merges bottom-up, so the dependency order is enforced by the PR bases:
OrganizationModelsContextValuehas noorganizationsfieldIntentionally skipped: #28587 (restore agents-access cleanup migration). Release branches cannot take a migration unless its sequence number matches main with no gap.
Validation
go build ./...and test-binary compilation for./coderd/... ./codersdk/... ./enterprise/... ./sitepasspnpm checkandpnpm lint:typespassAgentChatPage.stories.tsx> Queued For Capacity After Polling) reproduces identically on the untouchedrelease/2.37baseline, so it is pre-existing and unrelated.