mirror of
https://github.com/coder/coder.git
synced 2026-10-16 14:48:56 +08:00
mike/chatstate-outstanding-tool-calls
4802
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Michael Suchacz
|
65c8dc52cc |
test(coderd/x/chatd/chatstate): pin the dynamic-only cancellation filter
CancelRequiresAction from A1 now runs on a seed with one dynamic and one non-dynamic outstanding call, so dropping the dynamic filter fails the transition matrix. Also tighten the outstandingToolCalls doc and build the unparsable test row directly. |
||
|
Michael Suchacz
|
062f56941d |
refactor(coderd/x/chatd/chatstate): separate last-assistant loading from call policy
lastAssistantTurn loads the last assistant message and the tool calls already answered; outstandingToolCalls keeps the eligibility rules. Test fixtures share one chatMessage builder, and the waiting-state cancellation test now covers provider-executed, answered, and unparsable cases and part order. |
||
|
Michael Suchacz
|
7b3405fdd1 |
refactor(coderd/x/chatd/chatstate): share one outstanding tool-call scan
Cancellation synthesis and the pending-call checks each repeated the last-assistant lookup, tool-result tracking, and eligibility rules. outstandingToolCalls now owns that scan; its callers only build cancellation messages or the pending-call map. |
||
|
Atif Ali
|
c85ebc1346 |
fix: make workspace create denials on behalf of another user actionable (#30537)
Creating a workspace for another user as a caller without permission returned the same 403 as creating your own: "You are unable to create a workspace in this organization. It is possible to have access to the template, but not be able to create a workspace." For service accounts and integrations that always create for someone else, that doesn't say who the workspace was for, which organization, or what to do about it. When the caller is not the owner, the two 403s now say what is missing and for whom. Messages and details only; status codes and authorization are unchanged, and self-creates keep their existing text. Missing create permission: > You can't create workspaces for other users in this organization. > Creating a workspace for "alice" needs permission to create workspaces owned by other users in organization "coder", which your account doesn't have. Use an account with that permission (for example an organization admin), or create the workspace as "alice". Missing `use` on the template: > You can't use the template "demo" to create workspaces for other users. > Creating a workspace for "alice" from this template needs "use" access to template "demo", which your account doesn't have. Ask a template admin to grant it, or use an account that has it. `preflightWorkspaceCreate` now receives the caller's ID and the owner (name included) instead of just the owner ID. `coder create` prints the detail already, and the web UI shows it through `ErrorAlert`. **How it renders** Web UI (create page, `ErrorAlert`): <img width="2592" height="1490" alt="Web UI error for creating a workspace on behalf of another user" src="https://github.com/user-attachments/assets/9b1f1378-e4df-40df-8b6f-ca6cccdc4238" /> CLI (`coder create alice/ws1 --template demo` as a plain member): ``` error: Trace=[create workspace: ] You can't create workspaces for other users in this organization. Creating a workspace for "alice" needs permission to create workspaces owned by other users in organization "coder", which your account doesn't have. Use an account with that permission (for example an organization admin), or create the workspace as "alice". ``` Refs PLAT-726. Related owner-state inconsistencies are tracked in PLAT-749. |
||
|
cdrciandgithub-actions[bot]
|
7e69cd0af7 |
chore: refresh AI model price book (#30828)
## Price book changes 0 models added, 0 models removed, 24 models changed. <details> <summary>Changed</summary> - openrouter/deepseek/deepseek-v4-pro - openrouter/deepseek/deepseek-v4-pro-0813 - openrouter/deepseek/deepseek-v4.1-flash - openrouter/google/gemma-4-31b-it - openrouter/minimax/minimax-m2.5 - openrouter/mistralai/mistral-small-3.2-24b-instruct - openrouter/moonshotai/kimi-k3 - openrouter/openai/gpt-5.6-sol - openrouter/openai/gpt-5.6-sol-pro - openrouter/openai/gpt-oss-120b - openrouter/qwen/qwen3.6-35b-a3b - openrouter/qwen/qwen3.8-27b - openrouter/tencent/hy3 - openrouter/tencent/hy4-preview - openrouter/z-ai/glm-5 - openrouter/z-ai/glm-5.1 - openrouter/z-ai/glm-5.2 - openrouter/z-ai/glm-5.3 - openrouter/~deepseek/deepseek-flash-latest - openrouter/~deepseek/deepseek-pro-latest - openrouter/~google/gemini-pro-latest - openrouter/~moonshotai/kimi-latest - openrouter/~z-ai/glm-flash-latest - openrouter/~z-ai/glm-latest </details> ## Review notes Regenerated by `make gen/aibridge-prices` from the live [models.dev](https://models.dev) catalog. Both artifacts come from one snapshot, so they ship together: - `coderd/aibridge/prices/data/prices.json` - `site/src/modules/aiModels/knownModels/knownModelsGenerated.json` These are customer-visible cost numbers taken from upstream data, so this PR is never merged automatically. The summary above lists what moved; check the diff for exact figures before approving. Opened automatically by the [aigateway-prices-refresh workflow](https://github.com/coder/coder/actions/runs/38710433812). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> |
||
|
Ethan
|
aef7f15510 |
fix(coderd/x/chatd): only retry without drop_block when it's rejected (#30821)
In #29792, once a provider and model had accepted `drop_block`, any 400 on a request with it retried once without it, since I didn't know what error a provider returns for an unsupported beta. That turned out to hide real errors. On dogfood, a chat replaying some very tall screenshots hit Anthropic's many-image limit: ``` messages.9.content.43.image.source.base64.data: At least one of the image dimensions exceed max allowed size for many-image requests: 3000 pixels ``` The retry without `drop_block` then hit the stale thinking block, so the chat failed with the binding error instead, and the image error only showed up in a warn log. To fix, we'll only retry without `drop_block` when the 400 is a known rejection of the field or its beta, and otherwise return the error as is and keep the hint. The catch is that if a provider rejects the beta with an error we don't recognise, chats on that model will see it until a restart clears the hint, rather than us recovering. Claude Code (as of 2.1.300) does the same on Anthropic's own API, Claude Platform on AWS included: it only retries without the beta when the 400 names it or the `block_binding` field. On Bedrock, Vertex and Mantle it also retries on any 400 it doesn't otherwise recognise, which we could copy if that catch turns out to matter. The oversized screenshots are a separate problem, since chatd doesn't resize images at all. |
||
|
Bobby Ho
|
603dd420d1 |
feat: add ServerTailnet logs and metrics for unreachable agents (#30133)
Add default-level metrics and warn-level diagnostics to investigate ServerTailnet reachability failures causing agent-specific 502s, without restarting coderd or enabling debug logging. - Enrich request logs and PTY/chatd warnings with agent ID, failure reason, wait duration, coordinator state, WireGuard handshake and byte counts, and DERP regions. - Add metrics for unreachable attempts by reason, reachability latency, embedded DERP connections, and offline app/terminal requests. - Distinguish reachability failures from subsequent TCP dial failures, cap reverse proxy dials at one minute, and exclude expected chatd readiness polls and superseded-agent dials from unreachable counts. - Report missing handshakes as zero time instead of 1970, also fixing `coder ping`. Part of PLAT-680, a sub-issue of PLAT-360. |
||
|
Michael Suchacz
|
e7c2fd5d2e |
feat: add Gemini 4 Argon to Agents and AI Gateway (#30780)
Add Gemini 4 Argon to the Agents known models catalog, so it appears as a suggestion when an admin adds a chat model, and regenerate the AI Gateway price book from models.dev. | Provider | Model ID | Placed before | |---|---|---| | google | `gemini-4-argon` | `gemini-3.1-pro-preview` | | openrouter, vercel | `google/gemini-4-argon` | `google/gemini-3.8-flash` | **Pricing.** The price book carries Google's launch rates on google, openrouter, vercel, and copilot: $2 per 1M input tokens, $10 per 1M output tokens, and $0.10 per 1M cached input tokens. Google doubles them on 2026-12-01 ($4, $20, $0.20); the weekly refresh picks that up once models.dev changes. The google entry keeps models.dev's 32,768 max output tokens, the cap Google enforces on non-streaming `generateContent` calls (larger values return HTTP 400 there). **Thinking levels.** `googleSupportedThinkingLevels` did not recognize the `argon` name token and failed closed, so chatd sent Argon no `thinking_level` and no `include_thoughts`: the native Google path dropped the reasoning effort, and neither path showed thought summaries. Argon accepts LOW, MEDIUM, and HIGH like Gemini 3.1 Pro and rejects MINIMAL, so the map now treats it like the Pro family. **Upstream drift.** `prices.json` covers every model of its 8 providers, so the regeneration also brings the changes since the last weekly refresh, all on OpenRouter and Vercel: 14 other new entries, 26 entries models.dev delisted, and 51 price changes. The catalog picks up the same changes for 8 curated OpenRouter entries (DeepSeek, Kimi, Qwen, and GLM). ## Testing - Live requests to `gemini-4-argon` through the AI Gateway's Google provider (Google's OpenAI-compatible endpoint): `reasoning_effort` and `thinking_level` values `low`, `medium`, and `high` return 200 and `minimal` returns 400; `include_thoughts` returns thought text in streaming and non-streaming responses. - Red-green: removing the `argon` token fails `TestGoogleSupportedThinkingLevels/gemini-4-argon` (expected LOW, MEDIUM, HIGH; got nil). - Regenerating from the saved models.dev snapshot reproduces both generated files byte for byte. - `go test ./scripts/aibridgepricesgen/... ./coderd/aibridge/prices/... ./coderd/x/chatd/chatprovider/...` and `pnpm exec vitest run --project=unit src/modules/aiModels/` pass. The pre-commit hook passed; local pre-push checks were skipped, so CI is the first full run. ## Review record - Frontend review: the only `site/` change is the generated `knownModelsGenerated.json`, so the FE1 to FE10 checklist does not apply; no hand-written frontend code changed. > Xum opened this PR on behalf of @ibetitsmike. |
||
|
Bobby Ho
|
ee5e3a4029 |
feat: send script resource addresses and dependencies in the agent manifest (#30627)
feat: include script dependencies in agent manifests Send persisted script Terraform addresses and dependency lists on every manifest request, preserving them across agent restarts, reconnects, and shutdown without rebuilding the workspace. - Add `resource_address` and `dependencies` to the agent proto, with `SUCCESS` and `COMPLETION` requirements. - Reuse shared database-to-SDK validation and add bidirectional SDK/proto conversion. Reject empty prerequisite addresses and invalid requirements rather than accepting partial lists. - Bump the agent API to v2.13 and update the agent's RPC connection. Older agents ignore the new fields; older servers return empty fields. - Test manifest delivery, subagent isolation, round trips, invalid dependencies, and unchanged encoding when the new fields are unset. Script execution is unchanged; ordering enforcement follows in PLAT-549. Production dependency lists remain empty until #30556 merges. Stacked on #30625. Refs PLAT-548. |
||
|
Nick Vigilante
|
6afd1515e0 |
fix(coderd/templatebuilder): guard quickstart C/C++ install on every tool it installs (#30374)
The Quickstart base's C/C++ branch guarded on `command -v gcc`, but installs `gcc g++ make cmake`. `codercom/enterprise-base:ubuntu` ships gcc but not cmake, so cmake was never installed while the script reported success. The guard now checks every tool the install provides (`gcc`, `g++`, `make`, `cmake`). Adds `TestQuickstartCppInstallGuard`, which runs the rendered C/C++ branch against stubbed tools and asserts the install runs when any of them is missing. It fails without the fix. Fixes DOCS-736. The matching example template change is in the DOCS-613 PR. Generated by Coder Agents on behalf of @nickvigilante. |
||
|
Garrett Delfosse
|
2b9fad8bba |
feat: add chat project sharing database layer (#30479)
Database and authorization layer for chat project sharing (experiment `chat-projects`), part 1 of 2. #30439 adds the API on top of it. Nothing here changes behavior: projects start with empty ACLs, no endpoint writes them yet, and deleting a project still detaches its chats. Project deletion is in #30683. - **Schema:** migration `000621` adds `user_acl` and `group_acl` jsonb columns to `chat_projects`, typed as `ChatACL` like chats, with GIN indexes. It also adds the `chat_project:share` API key scope. - **Queries:** - `GetChatProjectsOwnedOrSharedWithUserID`: owned projects plus projects with an ACL entry for the user directly, through a group, or through the Everyone group (whose ID is the organization ID). Entries count only for members of the project's organization. Keys match regardless of actions, so dbauthz filters the rows. - `IsChatProjectAccessibleByUserID`: whether a user owns a project or holds a `read` or `*` grant on it, for code running under a system subject. With sharing disabled, dbauthz answers owner-only. - `GetChatProjectByIDForUpdate`: lets the ACL update read, modify, and write in one transaction. - `UpdateChatProjectACLByID`. - **RBAC:** - New `share` action on `chat_project`. dbauthz requires it to update the ACL. - `ChatProject.RBACObject` attaches the project ACL. Memory objects get a derived ACL: project `read` grants memory read, and `read` with `update` grants memory create and delete. Wildcard grants count. Read sharees change memories through their chats' agents, which #30439 gates. Project instructions authorize through the same object, so a `read` grant reads them and an `update` grant edits them. - `--disable-chat-sharing` also covers projects: ACLs are ignored, the ACL update is rejected, and roles deny `chat_project:share`. The help text and chat sharing docs say so. - **Audit:** `user_acl` and `group_acl` changes are tracked. - **Tests:** - Querier tests run the access queries both raw and through dbauthz (owner, direct user, group, Everyone, wildcard grants, a member of another org, grants without read). - RBAC object and memory ACL tests, with an authorizer check, in both sharing modes. - dbauthz cases including the disabled guards, and the `chat_project:share` negation in the roles test. The generated TypeScript changes only add the new scope value. > Generated by Coder Agents. |
||
|
coder[bot]
|
69fab7de06 |
chore!: remove the coder_report_task MCP tool (#30742)
Coder Tasks have been removed from coder/coder, so the
`coder_report_task` MCP tool and the status reporting pipeline behind it
no longer do anything useful. This PR removes the tool and the code that
only existed to support it.
### Removed
- `codersdk/toolsdk`: `ToolNameReportTask`, `ReportTaskArgs`,
`ReportTask`, `Deps.report`, `WithTaskReporter`, and the `All` entry,
plus their tests.
- `coderd/mcp`: the remote MCP exclusion for `coder_report_task`.
- `coder exp mcp server`: the task reporter, the AgentAPI screen
watcher, the agent socket connection, and the `coder_report_task` skip
logic. The server now requires an authenticated user client, because
every remaining tool needs one.
- `coder exp mcp configure claude-code`: the
`mcp__coder__coder_report_task` allowed tool, the injected
`coder_report_task` CLAUDE.md prompt, the `report tasks properly`
history line, the default system prompt asking the agent to send task
status updates, and the `CODER_MCP_APP_STATUS_SLUG` /
`CODER_MCP_AI_AGENTAPI_URL` env written to the Claude MCP config.
- `cli/cliutil/queue.go`: only used by the reporter.
- `github.com/coder/agentapi-sdk-go` (and transitive `tmaxmax/go-sse`)
via `go mod tidy`.
- The "task reporting" mention in `docs/ai-coder/mcp-server.md`.
### Breaking: removed `coder exp` flags and env
`coder exp` is experimental. Any scripts that still pass these flags
will now fail with an unknown flag error. Env vars that are no longer
read are ignored.
| Command | Flag | Env |
|---|---|---|
| `coder exp mcp server` | `--app-status-slug` |
`CODER_MCP_APP_STATUS_SLUG` |
| `coder exp mcp server` | `--ai-agentapi-url` |
`CODER_MCP_AI_AGENTAPI_URL` |
| `coder exp mcp server` | `--socket-path` | `CODER_AGENT_SOCKET_PATH`
(still used by `coder agent` and `coder exp sync`) |
| `coder exp mcp configure claude-code` | `--claude-app-status-slug` |
`CODER_MCP_APP_STATUS_SLUG` |
| `coder exp mcp configure claude-code` | `--ai-agentapi-url` |
`CODER_MCP_AI_AGENTAPI_URL` |
`--claude-system-prompt` no longer has a default.
<details>
<summary>Intentionally kept</summary>
- **Workspace app status reporting** (`UpdateAppStatus` on the agent API
and agent socket, `agentsdk.PatchAppStatus` / `ProtoFromPatchAppStatus`,
the deprecated `PATCH /workspaceagents/me/app-status` endpoint): still
served by coderd and the agent and part of versioned protocols. With
this change, nothing in the CLI calls the agent socket `UpdateAppStatus`
client anymore. Removing it would need a separate protocol change.
- **`report_tasks` in
`coderd/templatebuilder/modules/{aider,amazon-q}`**: these catalog
entries mirror the input variables of pinned coder/registry module
versions, and the behavior lives in those modules. Dropping the variable
here would not disable it, because `amazon-q` defaults it to `true`. The
registry modules need a follow-up instead.
- **`UserClientOptional` on toolsdk tools**: other tools still set it,
and remote MCP still uses it.
- No CLI golden files or generated CLI docs changed, because `coder exp`
commands are hidden.
</details>
### Verification
- `go build ./...`
- `go test ./codersdk/toolsdk/... ./coderd/mcp/... ./cli/cliutil/...`
- `go test ./cli -run 'TestExpMcp|TestCommandHelp'`
- `golangci-lint run ./cli/... ./codersdk/toolsdk/... ./coderd/mcp/...`
(0 issues)
---
_Generated by Coder Agents._
Co-authored-by: coder[bot] <137810501+coder[bot]@users.noreply.github.com>
|
||
|
Ehab Younes
|
f573ec3533 |
feat: report agent connection methods and app identities (#30467)
Agents now report how a connection was made and which app made it, so the connection log records the method and the client app separately. - Bump the Agent API to v2.13. `Connection` gains `connection_method` and `app_name`, and `type` is deprecated. - SSH and reconnecting PTY handlers set the method themselves, so a client-supplied app name can't change it. - SSH sessions report the app from `CODER_SSH_SESSION_TYPE`. Sessions without one count as SSH but report no app. - Disconnect reports repeat the connect report's method, app, and client session ID. - Coderd logs the reported method and normalized app name. For agents older than v2.13, it maps `type` as before. - Coderd rejects unknown methods instead of logging them as SSH. Closes CLIENT-1037 |
||
|
Ethan
|
60c6c0959e |
fix: run chat interrupt messages before earlier queued messages (#30729)
If an agent is working with a message already queued, and you send another message with `busy_behavior=interrupt`, the agent stops, but then answers the queued message first and yours second. The interrupt send appends to the end of the queue, and finishing the interruption promotes the head of the queue, which is the older message. To fix, we'll put the message that starts the interruption at the front of the queue, so it's the one promoted once the partial response is saved. Messages sent whilst an interruption is already in progress still go to the back, as they do today, so a burst of interrupts keeps the order it was sent in. On a chat waiting on dynamic tool results, or one that errored with messages still queued, an interrupt send now goes straight into history ahead of the queue (cancelling any pending tool calls), instead of waiting behind it. This is just a narrow fix to the current queue behaviour, and it doesn't change the API shape. It's compatible with the queued message delivery rework I've got in progress, which replaces these code paths. The one case this doesn't fix is a second interrupt sent during the first, which still waits behind earlier queued messages; that rework handles it properly. |
||
|
Bobby Ho
|
0ea3f16dd3 |
fix(coderd/database/migrations): renumber MCP OAuth2 resource migration to 000622 (#30722)
Main is red: #30625 and #30651 both added a migration numbered 000618,
and the
loader refuses to start with a duplicate number. See the failing run on
|
||
|
Michael Suchacz
|
fa7cd62c8b |
feat: send RFC 8707 resource parameter on MCP server OAuth2 flows (#30651)
Coder's MCP OAuth2 client never sent the RFC 8707 `resource` parameter, which the MCP authorization spec requires on authorization and token requests. Authorization servers that enforce it cannot be connected: with Dagster+, discovery and dynamic client registration succeed, then the authorize request fails with "MCP OAuth requests must include resource". MCP server configs get an `oauth2_resource` field (migration 000618, admin-only in API responses, audited). Coder sends it on the authorize redirect, the authorization-code exchange, and token refresh. Refresh now goes through `clientcredentials` with `grant_type=refresh_token`, because `oauth2.Config` cannot add parameters to refresh requests; it keeps the same auth-style detection and error types, and adds no dependency. Auto-discovery fills the field with the most specific Protected Resource Metadata `resource` that covers the server URL, stored as advertised. As in the MCP SDKs, a resource covers URLs on its origin at or under its path, so `/mcp` covers `/mcp/` but `/mcp/` does not cover `/mcp`. URLs compare in RFC 3986 normal form (default ports, percent-encodings, dot segments, and an empty path as `/`), ports ignore leading zeros, IPv6 literals compare by address, and `%2F` stays inside its segment. Unlike the SDKs, a resource with a query, even an empty one, only covers a server URL with the same query. When none covers it, Coder uses the server URL with the scheme and host lowercased and user information and fragment removed, or sends no resource when that is not a valid resource indicator. An explicit value in the create request wins. Manual configurations send no `resource` unless an admin sets one, because some providers reject it (Microsoft Entra ID v2 fails with AADSTS901002). Values must be absolute RFC 3986 URIs without a fragment, user information, or an IPv6 zone identifier. The resource binds grants like the other OAuth settings: changing it deletes stored user tokens, and a callback whose config resource changed after the user started connecting returns 409 before the code is redeemed. The connect step records the resource in a callback-scoped cookie for that check. Discovery only runs when a configuration is created, so existing auto-discovered configurations, including the Dagster+ one that prompted this, keep an empty resource until an admin sets the new "Resource URL" field or re-creates the server.  ## Testing - Go: discovery (most specific match, path and trailing-slash boundaries, query matching including empty queries, equivalent ports, IPv6 literals, escapes, and dot segments including escaped ones, the normalized fallback and an invalid one, explicit override), validation (URI characters, brackets, IP literals, user information, trimming), grant deletion on change, clearing on an auth type change, redaction for members, refresh with and without a resource, the 409 when the resource changes mid-flow (the authorization server never receives the code), and a connect and callback flow against an authorization server that rejects requests without the resource. Each new test was checked to fail when the behavior it covers is broken, except two malformed IP literal hosts that Go's URL parser already rejects. - Vitest: the form round-trips the stored value, and clearing the field sends `""` on update. - Remote UAT through Coder Agents ([chat](https://dogfood.cdr.dev/agents/58695d5b-2112-400b-88f7-ab64c678f756)) on a test deployment with loopback MCP and authorization server mocks: - Round 1 on |
||
|
Michael Suchacz
|
b7d63121b0 |
refactor(coderd/x/chatd): move chat mutations into chat_mutations.go (#28756)
Moves the chat mutation verbs out of `chatd.go` into `chat_mutations.go`. They share one helper, `Server.updateChat`, instead of each repeating the transition, reload and publish steps. ## Changes - `chat_mutations.go`: `SendMessage`, `EditMessage`, `PromoteQueued`, `DeleteQueued`, `ArchiveChat`, `UnarchiveChat`, `SubmitToolResults`, `InterruptChat`, `CompactChat`, `ClearChat` and `ReconcileInvalidStateChat`, plus their private helpers. They stay `*Server` methods; there is no wrapper type. - `updateChat` runs the transition in one chatstate transaction and reloads the chat inside it, so the result and the sidebar watch event carry the snapshot bump. Then it publishes only the `status_change` watch event. Compact and clear already hold the post-transition row, so they skip the reload. - Edit-target checks reuse `validateEditTarget`. The root-chat archive guard lives only in `setChatFamilyArchived`. `publishChatPubsubEvent(s)` stay in `chatd.go`. - Net diff: +1,125/-1,128 (production -83, tests +80). ## Behavior No intended behavior change. Transaction boundaries, locking, hook order, error strings, publish order and the debug cleanup cutoff match main. The moved code includes main's later changes: inline MCP server replacement on send (#29546), the first-turn fallback title (#27111), configurable limits (#28927), queue wait metrics (#29771) and inline interruption finish (#30259). `auto_archive.go` keeps its own archive path. Its precondition is checked under the lock, it uses the worker's store, and it returns rows for auditing. ## Testing Rebuilt on main `b2c8798e0e0`. A 42-mutant sweep ran on main and on this branch, and nothing main's tests caught goes uncaught here. New tests, each red-green checked by removing the code it guards: - `TestUpdateChat` covers the reload, skipping the reload, and rollback with nothing published. - `TestArchiveStateRejectsChildChat` uses the public methods on a real database, uses `ErrorIs`, and checks both archive flags stay unchanged. It replaces `TestUnarchiveChildChat` and `TestArchiveChat_RejectsChildChat`. - `TestArchivedChatRejectsMutations` covers the in-transaction archived check for six verbs; no test covered it before. - A `title_change` assertion is added to `TestSendMessageFirstUserTurnFallbackTitle`, and `TestArchiveChatDeletesDebugRuns` covers archive debug cleanup. Validation: - `go build ./coderd/... ./enterprise/...`, `go vet ./coderd/x/chatd/...`, `golangci-lint` and `make lint/go/new-code`: clean. - `go test ./coderd/x/chatd/... -count=1` passes on all packages, and a race run on the touched tests passes. - `./coderd` chat tests pass, including `TestInlineMCPServers`, `TestWatchChats` and `TestSubmitToolResults` (158 runs), as do the chat state and hook tests and `./enterprise/coderd` chat tests. - Local pre-push checks were skipped at the author's request; CI is the gate. ## Review record - [Codex OK](https://github.com/coder/coder/pull/28756#issuecomment-6151488087) on head `fc327b8b873`, no unresolved threads. [CI passed](https://github.com/coder/coder/actions/runs/38439818111) on the same head. The first CI run on `3d9b0e83369` caught a test bug: parallel subtests shared one parent context and timed out while waiting for a slot. `fc327b8b873` gives each subtest its own context. - Earlier Codex OKs and @ThomasK33's approval are on superseded heads (`fe038a5f13a` and earlier). This description replaces the old "single mutator" framing. - Independent agent review of the rebuilt branch: GO, with every verb's behavior equal to main. P3 fixes applied: moved code kept in main's formatting, and a retry rationale comment restored. Not added: a unit test of the tool-result validation detail strings, because it would make the PR net-positive. Main also only checks those details through coderd `TestSubmitToolResults`. - This PR and #28757 conflict in `chatd_test.go`. Whichever merges second needs main merged in. > Xum (an AI coding agent) rebuilt this PR on behalf of @ibetitsmike. |
||
|
Michael Suchacz
|
adcce66826 |
feat: load organization skills in chats (#30613)
## Stack Closes [CODAGT-1341](https://linear.app/codercom/issue/CODAGT-1341/support-organization-level-skills-in-coder-agents). This stack adds organization skills to Coder Agents: admins publish a skill once per organization, share it with groups or members, and chats in that organization load it next to personal and workspace skills. It replaces the 21-PR draft stack #30584 to #30604 with five PRs, bottom to top: 1. #30609 data model: one migration, queries, RBAC and dbauthz 2. #30610 organization skill API routes and the scope-neutral SDK 3. #30611 sharing ACL routes 4. #30612 settings UI: shared skills table, admin page, member section 5. **#30613** chats: chatd loading, `/` menu, docs (this PR) ## What changes This PR makes chats use organization skills: chatd adds the skills shared with the chat owner in the chat's organization to the skill index, `read_skill` loads them, the `/` menu offers them, and the docs describe the feature. **chatd** (`coderd/x/skills`, `coderd/x/chatd`): - New source `skills.SourceOrganization` (`"org"`). `MergeSkills(personal, organization, workspace)` sorts by name, then source. A name in one source keeps its bare alias; a name in more than one source gets a qualified alias in every source that has it (`personal/<name>`, `org/<name>`, `workspace/<name>`), so no source overrides another. `Lookup` returns `ErrSkillAmbiguous` for a bare name that matches several sources. - **Authorization (please review closely):** each turn, chatd reads organization skill metadata and bodies as the chat owner's real RBAC subject with their roles and groups (`chatOwnerSubject`), never as the chatd system actor, so per-skill ACLs apply. Disabled skills are skipped. A skill that is deleted, disabled, or no longer shared by the time `read_skill` runs is reported as not found. If the owner subject or the metadata can't be loaded, turn preparation fails with the error, as main now does for personal skills (#30492), instead of running the turn without organization skills. - `read_skill` loads personal and organization bodies through one `LoadStoredSkillBody` callback. An index description longer than 1,024 runes, the Agent Skills specification limit (`maxSkillIndexDescriptionRunes`), is cut to 1,023 runes plus an ellipsis so it stays within the limit; `read_skill` still returns the full skill. - Parse and lookup errors keep `%w` last on purpose: with `xerrors`, a `%w` that isn't last repeats the cause, so main's detail read "skill is too large: got N bytes, maximum is 65536 bytes: skill is too large". This stack returns the cause once (ThomasK33 asked about the order in review). **`/` menu** (`site/src/pages/AgentsPage`): the chat page fetches the chat organization's enabled skills (`organizationSkills(...)` from #30612's queries) next to personal ones. `utils/skillAliases.ts` (`resolveSkillTriggers`) follows `MergeSkills`: a personal or organization trigger is bare unless its name is in more than one source, then it becomes `/personal/<name>` or `/org/<name>`. While any list is unknown every trigger is qualified, because a qualified alias always resolves; workspace triggers are always qualified. Built-in `/clear` and `/compact` stay pending until every skill list is known, and a list with no data counts as unknown even after an error, so a failed fetch can't make a built-in run in place of a same-named skill. Disabled skills never shadow a built-in. **Docs:** `docs/ai-coder/agents/extending-agents.md` gains two sections, Organization skills (manage them, control who can use one, find the ones you can use) and How chats load stored skills (turning a skill on or off, skills with the same name). The admin page's organization picker shows only for a user who can view more than one organization's skills. The member "From your organizations" section shows one organization at a time with its own picker, starts on the default organization, and has Export all for the selected organization. The Enabled column is a switch for editors and a check or X otherwise. The settings table in `platform-controls/organizations.md` gains rows for organization and personal skills. ## Commits The first commit reproduces the reviewed chatd code from the old stack; the site and docs commits reapply the old layers on #30612's queries, and the rest are labeled trims or fixes. - `aa634eec94a` feat(coderd/x): load organization skills in chats. Reproduction of the old skill sources and chatd layers (old head `a1047f4986`). - `97945db4e86` test(coderd/x/skills): make `TestMergeSkills` (6 cases) and `TestLookup` (8 cases) table-driven; coverage is 100% before and after, with the same blocks. - `dde61cc5fe2` test(coderd/x/chatd): drop skill tests duplicated by the organization skills integration test `TestOrganizationSkillsFollowChatOwnerAccess`; the covered block set of the touched code is identical (348 of 506 blocks). - `aad14b2dd05` feat(site/src/pages/AgentsPage): show organization skills in the slash menu. The old alias helper and slash menu layers, applied cleanly on #30612's queries. - `712e272e140` test(site/src/pages/AgentsPage): trim duplicated slash menu tests and stories. Deletes `AgentChatPage.test.tsx` (177 lines; its assertions live in `submitChatTurn.test.ts` and `ChatMessageInput.test.tsx`), two duplicate `ChatMessageInput` cases, six of 17 `SkillsTriggerMenu` stories, and one disabled-skill fixture row from a `ChatMessageInput` story. - `232535e1011` docs(docs/ai-coder/agents): document organization skills. From the old docs layer, with the View line updated for #30612's read-only dialog and Download. - `62dbbd2dfd3` refactor(site/src/pages/AgentsPage): address frontend review on the slash menu. - `7349eac4389` Merge the updated settings-ui layer into chats. Brings in the #30610 and #30612 fixes; the merge has no changes of its own. - `0f84a30ec69` Merge the updated settings-ui layer into chats again, for #30612's read-only View fix; no changes of its own. - `fb37822f985` fix(coderd/x/chatd/chattool): keep truncated skill index descriptions within 1,024 runes. The cut now happens at 1,023 runes so the ellipsis fits; the existing `skill_test.go` assertion now expects 1,023 runes plus the ellipsis. - `1543666b584` docs(docs/ai-coder/agents): state the 1,023-character cut before the skill index ellipsis. - `96f1da47c9a` and `13f03d095ff` merge the updated settings-ui layer for #30612's cache and Skills link fixes; no changes of their own. - `b03c14c86de` and `41cc078b85a` merge the updated settings-ui layer; `b03c14c86de` moves the `AgentChatPage.tsx` and `ChatMessageInput.tsx` call sites, and their tests and stories, to #30612's split `organizationSkills(...)` query. - `1a32d05c9bc` test(site/src/pages/AgentsPage/components/ChatMessageInput): restore per-source loading stories for the slash menu. - `830b0466982` fix(coderd/x/chatd): fail turn preparation when organization skills cannot load. Main now returns database errors from turn preparation (#30492); organization skills still logged and dropped the failure, so a failed read silently hid them from the model. The new `TestFetchOrganizationSkillMetadataListFailure` covers the returned error. - `84c135f8801` and `a6f8f63ad13` merge the updated settings-ui layer, which brings in main; they resolve `generation_preparer.go` against main's turn preparation error handling. - `19b7880840f` merges #30612's member page test change; no changes of its own. - `c74d597f3c5` merges #30612's read-only status change; no changes of its own. - `f89c232c081` merges #30612's skills test changes; no changes of its own. - `17d5b9bfbc5` docs(docs/ai-coder/agents): describe the member skills organization picker and read-only Enabled status. - `f97b1710f71` merges the updated settings-ui layer (main at `e7b76872098`); no changes of its own. ## Size Production +558/-213 (docs +116/-6), tests +895/-262; total +1453/-475. This is above the 500-line reviewer-friendly limit on purpose: Mike asked for 4 to 5 PRs instead of the 21 small ones this stack replaces. ## Replaces #30593 (skill sources), #30594 (chatd), #30601 (alias helper), #30602 (slash menu), #30604 (docs). ## Validation - Go: all 21 packages under `coderd/x/skills` and `coderd/x/chatd` pass on `62dbbd2dfd3`, plus the coverage comparisons above; `coderd/x/chatd/chattool` passes again on `13f03d095ff` after the truncation fix. - Site: `pnpm run lint` and Biome clean; Vitest 102 tests; Storybook Vitest 49 + 4 tests. `pnpm run lint` reran clean on `13f03d095ff`. - On `a6f8f63ad13`, this PR's head before the test-only `19b7880840f` merge: `go test` passes for `coderd/x/skills`, `coderd/x/chatd/chattool`, and the `coderd/x/chatd` skill tests (`-run 'OrganizationSkill|Skill'`); 38 Vitest files covering skills and the slash menu pass, 542 tests with 2 skipped. - Docs: markdownlint 0 findings, Vale 0 findings. - On `17d5b9bfbc5`: Vale clean, markdownlint and `make pre-commit` pass. On `f97b1710f71`: `go build`, the `coderd/x/skills` and `chattool` tests, `pnpm check` and `tsc` pass. - Remote UAT round 7 through Coder Agents: PASS ([chat](https://dogfood.cdr.dev/agents/3426d4d3-2139-4ae1-8e0c-72ddeff413a8)). - Full run on `62dbbd2dfd3`, including an in-place upgrade from main `50c785cc6d7`: personal skills kept their IDs, names, content hashes and timestamps, came up enabled, and worked in chats (`read_skill`) and the `/` menu; create, edit, download and delete worked after the upgrade. The admin page, sharing, the Enabled toggle, the member section, the `/` menu, chats reading organization skills as the owner, limits, audit, authorization and scoped keys, and the docs all passed. - Delta run on `7349eac4389`: an update-only key gets 404 on PATCH with no content in the response; saving permissions refreshes Updated without a reload; smoke of the admin page, auditor View and Download, the `/` menu and `read_skill`. - Delta run on `0f84a30ec69` for `aaa356f0fd5`: read-only View, as an org auditor and as a member, shows no Name hint, focuses no field and starts on Close, and Escape or Close returns focus to the row menu button; Add skill and Edit still show the hint and focus the import field, and a saved edit persisted; smoke of the admin page, the `/` menu organization group and `read_skill`. Screen-reader behavior was not tested. - Delta run on `13f03d095ff` for `698dd83946d`, `f82573cd1c2`, `0e55137de7b` and `fb37822f985`: for an organization admin of org B who is a plain member of org A, the sidebar Skills link drops `?org=` for org A on seven AI settings pages and opens org B's skills, keeps `?org=` for org B, and a typed `/ai/settings/skills?org=` for org A still shows the permission error. A skill deleted and recreated with the same name loads a fresh sharing list showing only Everyone, with no stale grant. A 1,500-character description appears in the chat's skill index as 1,023 characters plus an ellipsis, while `read_skill` returns all of it. Smoke of the admin toggle, the member section, the `/` menu organization group and `read_skill`. - Low findings: the read-only View hint and field focus, fixed in `aaa356f0fd5` and verified by the delta run above. The member section's always-on disabled Enabled switch (also in the old stack) is replaced by a read-only status icon in #30612's `63bb42754e3`, which has no UAT run. Not fixed: `/` shows the `/personal/` prefix while organization skills fail to load (the unknown-list rule above); the shared Switch component's disabled styling (unchanged from main). A user who can read skills in only one organization sees no organization name on the Skills page, because the picker is hidden for a single organization, as on the MCP servers page on main. - Not tested: the workspace skills group (no running agent on the test deployment), the 100-per-user personal cap, and behavior without a Premium license. - Remote UAT round 8 through Coder Agents: PASS on `b03c14c86de` (same chat). The remote grader marked the unknown `read_skill` item FAIL only because the prompt asked for the literal phrase "skill not found"; the result names the skill once and says "not found" once, the same format as main, so the runner graded it PASS on intent. - Member page: starts on the default organization, shows one organization's table with the read-only note once, Export all downloads only the selected organization's skills, the picker is hidden with one organization, and an organization without enabled skills isn't listed. - Personal and admin tables: create, edit, toggle and delete each refetch the list; reopening the editor shows the new content even with the detail request delayed; a toggle changes state once, with no flicker. A skill deleted and recreated with the same name shows only Everyone in sharing. - The sidebar Skills link keeps `?org=` only for an organization whose skills the user can read. A create over 64 KiB returns 400 with the detail "got 70000 bytes, maximum is 65536 bytes: skill is too large", the cause once. The `/` menu shows Commands, Personal skills and Organization skills, and invoking an organization skill loads its body through `read_skill`. - New low findings, not fixed: the member page picker's listbox is labeled "Suggestions" (likely from the shared `OrganizationAutocomplete`); a typed body over 64 KiB shows only the size counter and a disabled Create until Import; `/ai/settings?org=` redirects to the models page without `?org=`. The first and third were not checked against main. - `9002dc60382`, `1a32d05c9bc`, `830b0466982`, `ac3e62c83b7`, `63bb42754e3`, `82b2487ad17`, `17d5b9bfbc5` and the main merges came after `b03c14c86de`; they have tests but no UAT run. - Every UAT round ran the migration as `000615_skills`. The heads ship the same up and down SQL as `000621_skills`, so the upgrade path from main's 000620 to 000621 has no UAT run. > Xum opened this PR on behalf of @ibetitsmike. |
||
|
Michael Suchacz
|
2c432a12a3 |
feat: add organization skill sharing ACL routes (#30611)
## Stack Part of [CODAGT-1341](https://linear.app/codercom/issue/CODAGT-1341/support-organization-level-skills-in-coder-agents). This stack adds organization skills to Coder Agents: admins publish a skill once per organization, share it with groups or members, and chats in that organization load it next to personal and workspace skills. It replaces the 21-PR draft stack #30584 to #30604 with five PRs, bottom to top: 1. #30609 data model: one migration, queries, RBAC and dbauthz 2. #30610 organization skill API routes and the scope-neutral SDK 3. **#30611** sharing ACL routes (this PR) 4. #30612 settings UI: shared skills table, admin page, member section 5. #30613 chats: chatd loading, `/` menu, docs ## What changes This PR adds the routes that let admins choose who can read an organization skill. It reuses the MCP server ACL machinery rather than adding a second implementation. Routes under `/api/experimental/organizations/{organization}/skills/{skillName}`, all behind `httpmw.ExtractOrganizationSkillParam` with `share`, so a caller without share gets 404: - `GET /acl` returns the resolved user and group grants. - `GET /acl/available` returns the organization's users and groups that can be granted, with the same `q`, `after_id`, `limit` and `offset` paging as the MCP endpoint. - `PATCH /acl` takes a sparse `UpdateOrganizationSkillACLRequest`: only the listed principals change, `read` grants and `""` removes. It returns 204 and writes a `write` audit entry with the old and new ACL. **Concurrency (please review closely):** the PATCH transaction first locks the granted users' `organization_members` rows `FOR KEY SHARE` (new query `LockOrganizationMembersByUserIDsForShare`, run as system), then locks the skill row `FOR UPDATE`, then validates the update on the transaction handle. Member removal takes the same locks in the same order (its membership row, then the skill rows through the #30609 trigger), so the two can't deadlock, and a removal can't miss a grant written by a concurrent PATCH. A skill deleted between the middleware read and the row lock stays a 404. Grants are users and groups, the same as MCP server configs (Mike's decision, for parity). Only the `read` role exists; any other role is a validation error, as are users or groups outside the organization. Supporting changes: - `coderd/mcp_acl.go`: the MCP ACL handlers' resolution, candidate listing and validation move into generic helpers (`writeOrganizationACLAvailable`, `validateOrganizationACLUpdate`, `resolveACLUsers`, `resolveACLGroups`) shared by both resources; `acl.Validate` accepts a named `acl.Role` constraint that now includes `codersdk.OrganizationSkillRole`. - `codersdk/skills.go`: `OrganizationSkillRole`, `OrganizationSkillACL` with its user and group entries, `UpdateOrganizationSkillACLRequest`, and client methods. - New sqlc query `UpdateOrganizationSkillACLByID` with its dbauthz wrapper (requires `share`). ## Commits The first commit reproduces the reviewed code from the old stack; each later commit is a labeled trim or fix. - `b1c85560b69` feat: add organization skill sharing ACL routes. Reproduction: every file this commit changes equals the reviewed old `acl-routes` head `80c1c3594e6`; the rest of the tree differs only by #30609's migration merge and main. - `b5d2ee265c8` test(coderd): merge duplicate organization skill ACL route tests (+49/-111). The removed cases repeated `TestMCPServerConfigACL`, `TestMCPServerConfigACLAvailable` or `TestRolePermissions`; with the MCP tests included, the covered block set of `organizationskills_acl.go`, `mcp_acl.go` and `rbac/acl/updatevalidator.go` is identical before and after. Kept: `TestOrganizationSkillACL` (`OrgAdminManagesACL`, `ReadersGetNotFound`, `SparseUpdateChangesReaders` with the audit diff, `RejectsUnsupportedRole`, `ScopedAPIKeys`), `TestOrganizationSkillACLConcurrentDelete`, and the lock order tests `TestOrganizationSkillACLMemberRemoval` (`RemovalBeforeGrant`, `RemovalAfterGrant`). `OrgAdminManagesACL` now also checks that users from another organization are not candidates (mutation checked). Sharing the param extractor with MCP was skipped: it would save about 10 lines of security-sensitive code. - `e09c4dba66b` Merge the updated org-api layer into sharing. Brings in #30610's `ad0a7a37504`; the merge has no changes of its own. - `1d3817dc639` and `c8c88f84d18` merge the updated #30610 layer, including main; no changes of their own. - `c4d22c47990` merges the updated #30610 layer (main at `e7b76872098`); no changes of its own. ## Size Production +431/-102, tests +377/-0, generated +918/-0; total +1726/-102. This is above the 500-line reviewer-friendly limit on purpose: Mike asked for 4 to 5 PRs instead of the 21 small ones this stack replaces. ## Replaces #30591 (ACL queries), #30592 (ACL routes). ## Validation - `go build` and `go vet` passed; `make gen` exited 0 with no diff. - `go test` passed for `coderd` (skill, MCP and ACL tests), `coderd/database`, `dbauthz`, `migrations`, `gentest`, `coderd/rbac/...`, `coderd/httpmw`, `coderd/x/skills/...`, `coderd/x/chatd/...`, and the audit and skill tests in `enterprise/audit` and `enterprise/coderd`. - The covered block comparison above, before and after the test merge. - On `c4d22c47990`: the same checks as #30610's head, plus `go test ./coderd/ -run 'Skill|MCP|ACL'`, pass. - Remote UAT round 7 through Coder Agents passed on the whole stack; see #30613's Validation for scope and findings. > Xum opened this PR on behalf of @ibetitsmike. |
||
|
Michael Suchacz
|
db7f5a7703 |
feat: add organization skill API routes (#30610)
## Stack Part of [CODAGT-1341](https://linear.app/codercom/issue/CODAGT-1341/support-organization-level-skills-in-coder-agents). This stack adds organization skills to Coder Agents: admins publish a skill once per organization, share it with groups or members, and chats in that organization load it next to personal and workspace skills. It replaces the 21-PR draft stack #30584 to #30604 with five PRs, bottom to top: 1. #30609 data model: one migration, queries, RBAC and dbauthz 2. **#30610** organization skill API routes and the scope-neutral SDK (this PR) 3. #30611 sharing ACL routes 4. #30612 settings UI: shared skills table, admin page, member section 5. #30613 chats: chatd loading, `/` menu, docs ## What changes This PR adds the organization skill CRUD routes and makes the skill SDK types owner-neutral so personal and organization skills share them. **API contract change (experimental API, personal and organization skills):** skill list and get responses gain `enabled`, and `PATCH .../skills/{skillName}` now takes `{content?, enabled?}`, so `content` is optional and a toggle doesn't resend it. A PATCH with neither field returns 400; `content`, when present, is validated as before. In the SDK, `UserSkill`, `UserSkillMetadata`, `CreateUserSkillRequest` and `UpdateUserSkillRequest` become `Skill`, `SkillMetadata`, `CreateSkillRequest` and `UpdateSkillRequest` (`codersdk/userskills.go` moves to `codersdk/skills.go`), and `UpdateSkillRequest.Content` is now `*string`. The personal client method names are unchanged. The site changes are the handwritten edits that rename forces (`api.ts`, `queries/userSkills.ts`, `testHelpers/skills.ts` and three call sites), with no UI change. Organization routes under `/api/experimental/organizations/{organization}/skills`, not gated by an experiment or license, like the personal ones: - `GET` lists with the caller's own permissions, so admins, the site owner and org auditors see every skill and members see only skills shared with them. `GET /{skillName}` goes through the new `httpmw.ExtractOrganizationSkillParam` and admits only read, so a missing skill and a denied one both return 404. - `POST` checks `create` on the organization before reading the body, then seeds `group_acl` with the Everyone group (whose ID is the organization ID) so a new skill reaches every member until an admin narrows it. The 100-skill cap and a duplicate name return 409. - `PATCH` admits only callers who can read the skill, because its response includes the content; `DELETE` admits callers who can read or delete it. A reader who can't change the skill gets a write-specific 403 from the database wrapper, and everyone else gets 404. PATCH locks the row as system, updates as the caller in the same transaction, and audits the locked row only after the transaction commits. - Audit: `database.AuditableOrganizationSkill` records resource type `organization_skill` with the organization ID; personal skills keep `user_skill`. Personal and organization handlers share one create and update path in `coderd/userskills.go` (`createSkill`, `updateSkill`, and a `skillWriteErrors` table per owner and action). ## Commits The first commit reproduces the reviewed code from the old stack; each later commit is a labeled trim or fix. - `49cb2c84276` feat: add organization skill API routes. Reproduction: every file this commit changes, including the SDK layer's site edits, equals the reviewed old `write-routes` head `aa597100f28`; the rest of the tree differs only by #30609's migration merge and main. - `dc893bc60d6` refactor(coderd): share the skill create and update path across owners (production +164/-188). It keeps the organization create check before the body read, the lock as system then update as caller order, and audit assignment after the transaction. No swagger, `codersdk` or `typesGenerated.ts` change; `make gen` is clean. - `f94925425ef` test(coderd): merge duplicate personal and organization skill tests. `TestSkillsCRUD`, `TestSkillValidationAndConflicts` and `TestSkillAudit` are table-driven over Personal, OrgAdmin and SiteOwner, alongside `TestOrganizationSkillAuditsDeniedWrites` and one `ScopedAPIKeys` subtest; `TestPatchUserSkill` is deleted because none of its blocks were unique. Coverage of `userskills.go`, `organizationskills.go` and `httpmw/organizationskillparam.go` is 101 of 117 blocks before and after, the same block set. - `ad0a7a37504` fix(coderd): require read access to patch an organization skill. The organization PATCH route now admits only `read`, because its response carries the skill's content: a key scoped to `organization_skill:update` without read gets 404, like GET, instead of 200 with the content. Readers without update still get 403, and DELETE is unchanged. `TestOrganizationSkillAccess/ScopedAPIKeys` asserts the 404, that the response has no content, and that the skill is unchanged; against the old route it fails with 200. - `69d00ded0b6` and `f67b79343ec` merge the updated #30609 layer, including main; `69d00ded0b6` resolves a conflict in the personal skills page with main. No other changes of their own. - `18373ffd59e` merges the updated #30609 layer (main at `e7b76872098`) and reapplies the `AuditableOrganizationSkill` entry in `enterprise/audit/table.go` alongside main's new `AuditableChatProjectInstructions` entries. ## Size Production +838/-308, tests +733/-322, generated +801/-274; total +2372/-904. This is above the 500-line reviewer-friendly limit on purpose: Mike asked for 4 to 5 PRs instead of the 21 small ones this stack replaces. ## Replaces #30587 (scope-neutral SDK and personal enabled state), #30588 (read routes), #30589 (write routes and audit). ## Validation - The coverage comparison above, before and after the test merge. - `make gen` leaves no diff after the shared create and update refactor. - On `18373ffd59e`: `make gen` is clean; build, vet, the `coderd` and `enterprise/coderd` skill tests, `rbac`, `httpmw`, `dbauthz` and `enterprise/audit` pass; `pnpm check`, `pnpm lint` and `make pre-commit` pass. - Remote UAT round 7 through Coder Agents passed on the whole stack; see #30613's Validation for scope and findings. > Xum opened this PR on behalf of @ibetitsmike. |
||
|
Michael Suchacz
|
b67587c875 |
feat: add the organization skills data model (#30609)
## Stack Part of [CODAGT-1341](https://linear.app/codercom/issue/CODAGT-1341/support-organization-level-skills-in-coder-agents). This stack adds organization skills to Coder Agents: admins publish a skill once per organization, share it with groups or members, and chats in that organization load it next to personal and workspace skills. It replaces the 21-PR draft stack #30584 to #30604 with five PRs, bottom to top: 1. **#30609** data model: one migration, queries, RBAC and dbauthz (this PR) 2. #30610 organization skill API routes and the scope-neutral SDK 3. #30611 sharing ACL routes 4. #30612 settings UI: shared skills table, admin page, member section 5. #30613 chats: chatd loading, `/` menu, docs ## What changes This PR adds the data model and authorization for organization skills. Personal skills keep working unchanged: `git diff origin/main -- codersdk site` shows only generated RBAC and API key scope additions (`apikey_scopes_gen.go`, `rbacresources_gen.go`, `rbacresourcesGenerated.ts`, the RBAC and scope unions in `typesGenerated.ts`) and `codersdk.ResourceTypeOrganizationSkill` with its friendly string. No personal route, SDK type or site component changes, and `coderd/userskills_test.go` changes only by mechanical constant renames. **One migration, `000621_skills`** (please review it closely): - renames `user_skills` to `skills` with its constraints, index and triggers, and recreates the cap function and `delete_deleted_user_resources`, whose PL/pgSQL bodies name the table; - adds `organization_id` and `project_id` (`ON DELETE CASCADE`), `enabled` (default true), and `group_acl` / `user_acl` (`jsonb`, default `{}`), with CHECKs for exactly one owner, ACLs that are JSON objects, and ACLs that stay empty unless an organization owns the row, plus partial unique name indexes per owner kind; - replaces the cap trigger with `enforce_skills_per_owner_limit()`: 100 rows per owner, disabled rows included, locking the owner row `FOR NO KEY UPDATE` so unrelated foreign key checks are not blocked; - adds a `BEFORE DELETE` trigger on `organization_members` that removes a departing member from `user_acl` on that organization's skills, so rejoining doesn't restore a direct grant; - adds the `organization_skill` `resource_type` and `api_key_scope` enum values last; nothing in the migration uses them. The down migration undoes the three old migrations in reverse order: it drops organization and project rows and restores `user_skills`, its names, the cap function and `user_id NOT NULL`. Authorization: a new RBAC resource `organization_skill` (create, read, update, delete, share). Owner and org admin get it; site and org auditors get read; members read only through the skill's ACL, which applies only to members of its organization. `Skill.RBACObject()` returns the organization object with both ACLs; project rows stay denied to everyone. New sqlc queries cover organization insert, get, list (SQL-filtered through `GetAuthorizedOrganizationSkillMetadata` and a `regosql` converter), update and delete, with dbauthz wrappers and the audit table entries. No routes yet. ## Commits The first commit reproduces the reviewed code from the old stack; each later commit is a labeled trim or fix. - `baa3150e3b7` feat: add the organization skills data model. Reproduction: every non-site file this commit changes equals the reviewed old `authz` head `1e16b3004c4`, except that the old stack's migrations 000615, 000616 and 000617 are merged into one migration (then `000615_skills`, now `000621_skills`) with one fixture (`testdata/fixtures/000621_skills.up.sql`) and one migration test (now `testMigration000621Skills`). At that commit, `dump.sql` was byte-identical to the old head's. - `1b84e83463a` test(coderd/database): keep only the SQL paths of the org skill matrix. `TestOrganizationSkillAuthorization` becomes `TestOrganizationSkillQueries` instead of being deleted, because coverage showed it is this PR's only run of the `GetAuthorizedOrganizationSkillMetadata` row scan (80% to 65% without it) and of the organization update and delete queries (100% to 0%). Swapping `group_acl` and `user_acl` in the converter makes `GrantedMember` fail. The removed cases are decided by `TestRolePermissions`, `TestSkillRBACObjectOrganizationACL` and the dbauthz method suite. - `e2931b412e9` test(coderd/database): fold the skill cap cases into existing coverage. The user case is covered by `TestUserSkillLimit` and `TestUserSkillLimitConcurrentCreates`; the organization case folds into `ConcurrentOrganizationInsertsHonorCap`, which now also checks that disabled rows count and another organization is unaffected (mutation checked); the project case stays. - `be62aea9021` test(coderd/rbac): drop `TestOrganizationSkillReadScope`, covered by `TestExternalScopeNames` and the generated scope file. - `0c275ba37de` merges main at `454a2fcc697`, which took 000615 to 000618: it renumbers the migration to 000619 and resolves `dump.sql`, `check_constraint.go` and the generated API docs. `8f2b4b87ee2` merges main at `ad85646164e`, which took 000619: it renumbers the migration to 000620. - `5248d56b64f` merges main at `e7b76872098`, which took 000620 (`000620_audit_chat_project_instructions_resource_type`): it renumbers the migration, fixture and migration test to `000621_skills` (step `Migration000621Skills`, version 621), and resolves `enterprise/audit/table.go` (keeping main's `AuditableChatProjectInstructions` entries and the `Skill` entry), `dump.sql` and `models.go` (the `resource_type` enum gets `chat_project_instructions`, then `organization_skill`) and the regenerated `schemas.md`. The up and down SQL are unchanged by every renumbering. ## Size Production +549/-67, tests +728/-33, generated +881/-314; total +2158/-414. This is above the 500-line reviewer-friendly limit on purpose: Mike asked for 4 to 5 PRs instead of the 21 small ones this stack replaces. ## Replaces #30584 (rename `user_skills` to `skills`), #30585 (skill owners), #30586 (organization skill authorization). ## Validation - Migration test: up keeps the existing personal row and its ID; down restores the `user_skills` names, cap function and `NOT NULL`, and drops organization rows; soft-deleting a user removes their skills after up, after down, and after re-up. - `go test` passed on `be62aea9021`, before the main merges, for `coderd` (skill tests), `coderd/database`, `dbauthz`, `migrations`, `gentest`, `coderd/rbac/...`, `coderd/x/chatd`, `coderd/x/skills`, `enterprise/audit` and `enterprise/coderd`; `make lint/check-scopes` passed; the four personal skills Vitest files pass unchanged. - On `5248d56b64f`: build, vet, `TestMigrate`, `TestMigrationChain`, the `migrations`, `database`, `dbauthz`, `gentest`, `rbac` and `enterprise/audit` packages pass; `make gen` run twice is clean, and `make pre-commit` passes. - Remote UAT round 7 through Coder Agents passed on the whole stack; see #30613's Validation for scope and findings. > Xum opened this PR on behalf of @ibetitsmike. |
||
|
|
97c5260adb |
feat: add chat project instructions API (#30368)
Adds the experimental API for chat project instructions: `GET`, `PUT`,
and `DELETE
/api/experimental/organizations/{organization}/chats/projects/{project}/instructions`,
behind the `chat-projects` experiment.
- `GET` returns 200 with empty instructions and null
`updated_by`/`updated_at` when nothing is set, so clients don't need a
404 path.
- `PUT` requires update access only (the read-back runs under a system
context, so a custom role with update but not read can save). It
sanitizes the text with `SanitizePromptText`. It rejects blank text with
400, pointing the caller to `DELETE`, and rejects text over the chat
prompt byte limit. It records the caller as `updated_by`. The upsert and
the read-back that returns the editor's profile run in one transaction.
- `DELETE` returns 204, including when nothing is set.
- Users without access to the project get 404, like the other project
routes.
- `PUT` and `DELETE` are audited as a new `chat_project_instructions`
resource (`write` and `delete`), with the project as the resource ID and
its name as the target. The diff tracks only the instruction text.
Saving unchanged text and deleting unset instructions are not audited. A
per-project advisory lock serializes reading the previous value with the
write, so concurrent edits produce accurate diffs. Migration `000615`
adds the enum value.
The codersdk client gains `ChatProjectInstructions`,
`UpdateChatProjectInstructions`, and `DeleteChatProjectInstructions`.
**What to scrutinize:** writes are last-writer-wins with no version
precondition, matching the deployment, user, and organization prompt
settings.
**Generated:** `coderd/apidoc/docs.go`, `coderd/apidoc/swagger.json`,
and `docs/reference/api/schemas.md` come from the Swagger annotations;
`site/src/api/typesGenerated.ts` comes from `codersdk`;
`docs/admin/security/audit-logs.md`, `coderd/database/dump.sql`, and
`coderd/database/models.go` come from the audit table and migration.
Together they are about 400 of this PR's lines.
**Validation:** `make gen` leaves no diff. `go test ./coderd -run
TestChatProject` and `go test ./enterprise/coderd -run
TestChatProjectInstructions` pass. `TestChatProjectInstructionsAudit`
checks the audit entries for create, update, and delete, and that no-op
saves and deletes are not audited. It covers unset GET, create, update,
blank and oversized PUT, delete, access denial, and cascade on project
delete.
---
Stack (merge top to bottom). #30377 through #29583 are f0ssel's split of
the original #29583, and this stack sits on #29583:
- #30377 data layer and project dialog (f0ssel)
- #30378 new-chat form locked to a project (f0ssel)
- #30379 project page route (f0ssel)
- #29583 sidebar folders (f0ssel), the base of this stack
- #30291 composer frame
- #30292 project chat list caches
- #30293 project page header
- #30294 project chats list
- #30367 instructions storage and prompt injection
- #30368 instructions API 👈
- #30369 instructions editor dialog
- #30370 instructions Details panel
---------
Co-authored-by: Garrett Delfosse <delfossegarrett@gmail.com>
Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
||
|
Michael Suchacz
|
b2c8798e0e |
fix: replay stateless OpenAI reasoning across chat turns (#30298)
OpenAI Responses chats default to `store=false`, and chatd only replayed reasoning as an `item_reference`, which needs provider-side storage. Reasoning was therefore dropped between tool steps and turns, so reasoning models lost their chain of thought mid-task. This pins coder/fantasy `coder_2_33` at 6ba8681, which includes fantasy #63 (and #64, #65, and #66). #63 captures each completed reasoning item (ID, `encrypted_content`, summary) and replays it in full regardless of `store`. In chatd: - Every Responses call requests `reasoning.encrypted_content`. The `store` default and configured includes are unchanged, and Chat Completions requests are untouched. - Prompt rebuilds keep reasoning parts without visible text when they carry finalized encrypted state, so reasoning survives durable history and coderd restarts. - The provider-switch sanitizer also strips OpenAI reasoning from history produced by a different or unknown provider instance, since reasoning IDs and encrypted content only resolve where they were issued. - A compaction model override on a different provider instance, or with a different model, gets a compaction prompt without the chat's OpenAI reasoning. - Each saved reasoning part records the provider instance, model, and credential (the deployment's or the user's personal API key) that produced it, and the sanitizer keeps it only when all three match the current turn, because another model, or the organization behind another credential, can reject the encrypted content. Switching models, adding or removing a personal key, or editing a config's model or provider drops the earlier reasoning; switching back restores it. The record is never returned by the API. Reasoning saved before this change has no record and follows its model config. - If OpenAI rejects replayed reasoning it cannot decrypt (`invalid_encrypted_content`), for example after the deployment's key is replaced with one from another organization, the request is retried once without reasoning, and the reasoning that step produces is saved with the provider's next rejection epoch. Epochs are kept separately for each provider, model, and credential. Later steps and turns leave out reasoning from earlier epochs, on any replica and after restarts, so only the first request after the change is rejected and newer reasoning keeps replaying. A retried step that returns no reasoning saves its epoch on its first part. The advisor's prompt never includes OpenAI reasoning, as on `main`. ## Testing Rebased onto main at |
||
|
|
ad85646164 |
feat(coderd): add chat project instructions storage and prompt injection (#30367)
Adds storage for project-level instructions in Coder Agents chat projects and injects them into every chat in the project. This is the first of four stacked PRs that replace #30349. A new `chat_project_instructions` table (migration 000613) holds at most one row per project, scoped to the project's organization. It records `updated_by` and cascades on project delete. The dbauthz wrappers authorize against the parent project: read access to get the instructions, update access to upsert or delete them. No new RBAC resource is added; project RBAC comes in a later PR. chatd loads the instructions on every turn, so edits reach existing chats on their next turn. They are added to the system prompt as a `<project-instructions>` block after the memory guidance and before personal instructions, so a user's personal instructions still win a conflict. Subagent chats carry no `project_id`, so they use their root chat's project. A load error other than no rows is logged, and the turn continues without instructions. **What to scrutinize:** the prompt ordering in `buildSystemPrompt`, and the choice to inject per turn rather than snapshot at chat creation. Per-turn injection costs one prompt-cache miss after each edit; the org instructions in #30272 use a creation-time snapshot instead. **Generated** (`make gen` from the migration and `queries/chatprojectinstructions.sql`): `dump.sql`, `models.go`, `querier.go`, `queries.sql.go`, `dbmetrics/querymetrics.go`, `dbmock/dbmock.go`, and the `*_constraint.go` files. `coderd/x/chatd/ARCHITECTURE.md` gets two TODOs for the PR author to replace with the real description, per AGENTS.md. **Validation:** `make gen` leaves no diff. `go test ./coderd/database/dbauthz -run TestMethodTestSuite/TestChats`, `go test ./coderd/x/chatd -run 'SystemPrompt|ProjectInstructions|Memory'`, and `go test ./coderd/database/migrations/...` pass. --- Stack (merge top to bottom). #30377 through #29583 are f0ssel's split of the original #29583, and this stack sits on #29583: - #30377 data layer and project dialog (f0ssel) - #30378 new-chat form locked to a project (f0ssel) - #30379 project page route (f0ssel) - #29583 sidebar folders (f0ssel), the base of this stack - #30291 composer frame - #30292 project chat list caches - #30293 project page header - #30294 project chats list - #30367 instructions storage and prompt injection 👈 - #30368 instructions API - #30369 instructions editor dialog - #30370 instructions Details panel --------- Co-authored-by: Garrett Delfosse <delfossegarrett@gmail.com> Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com> |
||
|
Michael Suchacz
|
7517301ffa |
chore: add golangci new-code quality gates (#30622)
Stack context: second layer of the Go lint gate stack, built on the exclusion fix below. Why: adds commented-out code detection and a `log/slog` ban to the main config, plus a new-code-only pass requiring reasons on lint suppressions, flagging ignored errors (including `_, _ = w.Write(...)`), and banning `time.Sleep` in tests. Fixing the hits surfaced a real bug: GitLab warnings went to the wrong logger. The new-code pass resolves symlinked checkout paths so it does not silently report nothing. Validation: both configs report 0 issues; each seeded violation is caught; `make pre-commit` passed. > Xum acted for Mike to open this PR. |
||
|
Michael Suchacz
|
e8dcf9a3f1 |
fix: anchor golangci .git exclusion and fix hidden findings (#30621)
Stack context: two-layer stack tightening Go lint gates. This bottom layer fixes the lint exclusion; the layer above adds new rules. Why: a `.git` path exclusion in the golangci config was hiding Go files, masking 70 findings. This fixes the exclusion, fixes the findings (`agent/agentgit` now runs git through `agentexec.Execer`), and adds a `make lint/go` check that fails when an exclusion hides Go files. `make lint/go` now waits for a concurrent golangci-lint run instead of failing. The exhaustruct exclusion rule for `scripts/*` had the same problem: as a regex it also matched `agent/agentscripts` and `coderd/agentapi/scripts.go`, so it is now `^scripts/`. Validation: full-tree lint reports 0 issues; the new check fails on the old entry and passes on the new one. > Xum acted for Mike to open and update this PR. |
||
|
Bobby Ho
|
dae7967bd1 |
feat: persist script resource addresses and dependencies (#30625)
feat(coderd): persist workspace agent script dependencies Persist Terraform resource addresses and script dependencies supplied by the provisioner (#30451, #30491), and expose them through the SDK and REST API. - Add migration 000618 with `resource_address` and `dependencies` columns, empty defaults, and a JSON-array constraint. API responses always include the address and omit empty dependency lists. - Preserve `success` and `completion` requirements, including scripts on devcontainer subagents. Legacy scripts and older provisioners remain compatible through empty defaults. - Reject empty prerequisite addresses and unsupported requirements, rolling back the entire job. Invalid stored dependencies return HTTP 500 rather than silently dropping ordering. - Add persistence, validation, rollback, devcontainer, API, and migration coverage; regenerate API docs and TypeScript types. Unordered templates retain their existing execution behavior. Dependency lists remain empty until #30556. Agent manifest wiring and the agent API version bump are deferred to a follow-up. Refs PLAT-548. |
||
|
Ehab Younes
|
2f2169ed19 |
fix: deduplicate template insights family metrics (#30497)
coderd_insights_applications_usage_seconds summed each family's per-app minutes, so a user with VS Code and Cursor open in the same minute was counted as two VS Code minutes. GetTemplateInsightsByTemplate now takes the registry's app-to-family mapping as jsonb, groups the raw agent stats by family, and counts each minute once per user and family. - Keep the metric's four built-in series (Visual Studio Code, JetBrains, Web Terminal, SSH) and their labels; only their values change. The HELP text now says built-in apps are grouped by family. - Add AppFamilyName.DisplayName() and remove the duplicate TemplateBuiltinAppDisplayName* constants from codersdk. The template insights handler returns the same display names. - Pin how the migration 000596 rollback approximates family minutes from per-app rows. Family values can drop wherever the old per-app sum double-counted overlapping apps. No migration or storage changes. |
||
|
Michael Suchacz
|
41f5caafec | fix: fold web searches, answer reasoning, and OpenAI narration into working blocks (#30529) | ||
|
Ehab Younes
|
5b060beeb1 |
feat: separate connection log methods from app names (#30466)
Connection logs now record how a connection was made (`connection_method`) separately from which app made it (`app_name`). Storage, the HTTP API, search filters, and the UI move to the new fields. Legacy agent reports and the deprecated `type` field keep working. - Add `connection_method` (`ssh`, `reconnecting_pty`, `workspace_app`, `port_forwarding`, `tunnel`) and optional `app_name` and `app_display_name` to `GET /api/v2/connectionlog`. Deprecate `type` and make it a plain string in the API and the Go SDK. - Add `method:` and repeatable `app:` search filters. `type:` keeps its old results but can't be combined with the new filters. - Replace the Type menu with a Method menu, show the app next to the method in each row, and warn when a filter uses `type:`. - Change the Active SSH connections preset to `method:ssh`, so it also includes SSH sessions from VS Code and JetBrains apps. - Generate `codersdk.ConnectionLogMethod` from the database enum with `typegen dbenums`. - Migration 000617 rewrites `connection_logs` under an ACCESS EXCLUSIVE lock, which takes about 50 seconds for 10 million rows. `vscode` and `jetbrains` rows become `ssh` rows with that app. Rolling back folds apps into the old types and loses per-app names. - Document the new filters and how to replace each `type:` value. Closes #27412 |
||
|
Danielle Maywood
|
cf6ab8739e | fix(coderd/x/gitsync): attach subagent git refs to the root chat (#30647) | ||
|
Ethan
|
4afd834ad2 |
fix(coderd/x/chatd): return database errors from turn preparation (#30492)
Relates to CODAGT-1330 When chatd prepares a turn, it reads a bunch of things from the database: the user's custom prompt, plan mode instructions, personal skills, the advisor config, the user's compaction threshold override, MCP server configs, inline MCP servers, MCP user tokens and the pinned workspace MCP tools. If any of those reads failed, we'd log a warning and run the turn without them, so the model would quietly ignore the user's instructions or be missing tools for that turn. To fix, we'll just return the error instead, so the prepare retry runs, and the chat shows an error if the reads keep failing. The provider switch history sanitizer and the cancel of deleted tool calls also return their database errors now. A model config that's unavailable, e.g. deleted, is still treated as foreign by the sanitizer like it is today. An inline MCP server with invalid stored headers still shows up as a failed connect in the chat debug panel, but a failed read of the inline MCP servers themselves now fails the turn. This PR is in a stack on top of #30449. |
||
|
Ethan
|
807878c1ab |
fix(coderd): return database errors at chat create and subagent spawn (#30449)
Relates to CODAGT-1330 chatd stores the deployment and organization system prompts in a chat when it's created. If either read failed, we'd log it and use the built-in default, or skip the organization prompt, so the chat ignored the admin's instructions for its whole life. The subagent model override had the same problem, and the compaction and advisor overrides quietly fell back to the chat model for that turn. To fix, we'll just return the database error instead. Overrides that can't be used, e.g. the model was deleted or the user has no API key for its provider, still fall back like they do today. |
||
|
Thomas Kosiewski
|
f923f4a525 |
chore!: remove the chat automations experiment (#30412)
Removes the experimental Chat Automations feature (`chat-automations` experiment) from main. This reverts the 16-PR stack that added it and adds two forward migrations that undo its schema and credential changes.
> [!WARNING]
> **Upgrade consequences**
>
> - **Automation data is deleted.** Migration 000615 drops the `chat_automations` table and deletes every queued message an automation delivered. Chats, their messages and ordinary queued messages stay. Messages lose their automation provenance (`automation_id`, `input_id`).
> - **Automation-only credentials are revoked.** Migration 000616 deletes API keys, OAuth2 codes and OAuth2 tokens whose only scopes (or allow-list entries) were `chat_automation:*`. Mixed credentials keep their other scopes. Custom roles lose their `chat_automation` permissions. OAuth2 apps that allowed only `chat_automation` scopes get the whitespace allowlist that grants nothing, as in 000595.
> - **A coordinated restart is required.** Stop all old replicas before the new version runs the migrations. Old replicas read the dropped table and columns, so a rolling deploy is not supported.
> - **Remove `--chat-max-automations-per-owner` before upgrading.** The flag, the YAML key `chat.maxAutomationsPerOwner` and the env var `CODER_CHAT_MAX_AUTOMATIONS_PER_OWNER` are removed. A deployment that still sets the flag or the YAML key will not start (`unknown flag` / `unknown option`). The env var is ignored. This includes dogfood. No release or tag contains the flag.
> - **Down migrations restore the schema only.** 000616 down and 000615 down recreate the 000611 schema with empty tables. They do not restore deleted automations, webhook secrets, provenance, queued deliveries, revoked credentials, stripped permissions or the deleted experiment rule.
## What is removed
- The `chat-automations` experiment and its stored runtime rule (`experiment_rule:chat-automations`).
- The `--chat-max-automations-per-owner` deployment option.
- The `chat_automation` RBAC resource, its member and chatd grants, and the `chat_automation:*` API key scopes.
- The automation sqlc queries, dbauthz wrappers and audit table entries.
- The management routes under `/api/experimental/organizations/{organization}/chat-automations`, and the webhook route `POST /api/experimental/chat-automations/{automation}/events` with its rate limiter.
- The chatd management service, webhook publish, schedule loop, the `manage_automations` tool, and the chatstate admission seam and queue promotion guard that only automations used.
- The Agents Automations page, editor, nav entry, chat labels, the Manage automations toggle and the API client.
- The `archived:any` chat search value that the automations "View chats" dialog added.
- `docs/ai-coder/agents/automations.md`, its manifest entry, the glossary entry and the related rows.
Reverted PRs: #30085, #30096, #30112, #30117, #30127, #30134, #30160, #30166, #30167, #30171, #30187, #30207, #30214, #30217, #30219, #30220.
## What stays, and why
- Migrations 000609, 000610 and 000611, their fixture, and the #30297 lock fix with `migration000609_test.go` stay unchanged. Deployments such as dogfood already ran them.
- New migration **000615** reverses the schema changes of 000609 and 000611 against the current schema. It uses the same short-wait lock-and-retry block as 000609, and it rebuilds `chats_expanded` from main's current definition minus the two automation columns. New migration **000616** follows the 000595 Tasks removal for credentials and recreates `api_key_scope` without the five `chat_automation:*` values. It keeps the `chat_project_memory:*` values from 000612.
- The `chat_automation` value stays in the `resource_type` enum, and `codersdk.ResourceTypeChatAutomation` stays. Old audit log rows stay readable, as with `task` after the Tasks removal.
- The 000615 delete fires the existing queue-version trigger, so `queue_version` moves to `snapshot_version` for each affected chat. `snapshot_version` is not bumped: the coordinated restart restarts every stream loop from an empty state, and the site does not read the version fields.
- `coderd/x/chatd/ARCHITECTURE.md` drops the sections the stack added and gets back its pre-stack wording, merged with the later main edits to the same file.
## Notes for reviewers
- #30379 (chat project page) merged while this PR was in progress. It passed `manageAutomationsEnabled` in `AgentCreatePage.test.tsx` and listed an `automations` route in `AgentsPageLayout.test.tsx`. Later main commits also added automation props in `ConversationTimeline.test.tsx` and `ConversationTimelineWorkingBlocks.stories.tsx`. This PR removes all of them.
- Remaining `automation` matches under `coderd`, `codersdk`, `enterprise`, `site/src` and `docs` are intended:
- migrations 000609 to 000611, their fixture and `migration000609_test.go`;
- the new 000615 and 000616 migrations and `migration000615_test.go`;
- the `chat_automation` resource type in `dump.sql`, `models.go`, `codersdk/audit.go`, the generated API docs, `swagger.json` and `typesGenerated.ts`;
- unrelated uses of the word "automation" (docs, `chatlabels_test.go`, `ratelimit.go`, `autostop.go`, `subagent_catalog.go`, `WorkspaceSharingSection.tsx`, `AgentChatPage.stories.tsx`).
Remote UAT on an isolated dev deployment passed. It upgraded a populated pre-removal database, then checked chats, queued messages, tokens, the 404 routes, the removed page and nav entry, and the experiments list.
---
_Generated with [`xum`](https://github.com/coder/xum) • Model: `anthropic:claude-opus-5-5` • Thinking: `high`_
|
||
|
Michael Suchacz
|
c28ae0dc9b |
feat: trigger compaction independently for the chat and compaction models (#28568)
> [!NOTE]
> Xum (an AI agent) authored this pull request on Mike's behalf.
## Summary
Automatic compaction now fires at whichever of two independent triggers
is reached first, and the UI shows when the organization override's
trigger applies.
- **Chat trigger**: the chat owner's personal threshold (or the chat
model's) against the chat model's window. A threshold of 100 turns off
only this trigger.
- **Organization trigger**: a fixed 80% of the organization compaction
override model's window. The override model's own compression threshold
is ignored. The other 20% is a target margin for the summary prompt and
the summary, not a guarantee, because the trigger reads the previous
step's prompt size.
On main, the chat threshold was applied to the smaller of the two
windows, and nothing in the UI showed it. Example: with a 20K chat model
at 70% and an 8K override, main compacts at 5,600 tokens; this PR
compacts at 6,400 (80% of 8K, shown as 32%).
## Changes
Backend (`coderd/x/chatd`):
- The trigger with the lower token point binds (ties go to the chat
trigger). It drives the automatic compaction gate and the
still-over-limit check, and every summary, automatic or manual, records
it.
- An override that is unset, deleted, disabled, on a disabled provider,
or without credentials for the chat owner is treated as absent: the chat
model summarizes and only the chat trigger applies.
- The summary's output cap is bounded by the summarizer's own window,
not the trigger's.
- The still-over-limit error names what can clear it: the compaction
threshold (chat trigger), an override model with a larger window
(organization trigger), or both when usage is over both points.
- The still-over-limit check compacts once more when the latest summary
was recorded under a different threshold or trigger window. The
`chat_summarized` result gains `trigger_context_limit_tokens`, and
`context_limit_tokens` now records the chat model's window (main
recorded the smaller window). On main, lowering a threshold after a
compaction ended every later turn with the error.
- The still-over-limit check skips a 0% threshold ("always compact"),
which on main ended the run with the error after every compaction.
- `ARCHITECTURE.md` documents the two triggers, the summary cap, and the
still-over-limit rules.
Frontend (`site`), through a shared `compactionTriggers` helper that
mirrors the backend rules (no new API):
- Coder Agents settings: the compaction override row warns when the
organization trigger undercuts chat models' default triggers ("the
organization override compacts chats at 80% of {model}'s {N}-token
window").
- Personal thresholds page: an Effective column shows the trigger that
applies, with a popover when the organization override binds. "Compacts
at" follows the same trigger, and a point the chat model's window cannot
reach shows as Off. The header's 100% sentence, and the row warning when
a member types 100, say an organization override may still compact chats
when one is set or its organization's overrides failed to load (the
header also while they load).
- Chat context gauge: the compact-at percent follows the binding trigger
and is annotated when the organization override binds. If the overrides
request fails, it shows the chat-only value with "(organization override
not loaded)", or "Compaction off (organization override not loaded)"
when the chat trigger is off.
- An override whose provider the user cannot use is ignored on the gauge
and the personal page, matching chatd.
- The shared `formatContextLimit` prints counts below 1,000 exactly (an
800-token compaction point showed "~1K") and 999,500 to 999,999 as
"1.0M" instead of "1000K". It also labels the model pickers; their other
values are unchanged.
- `HelpPopoverIconTrigger` lets callers set their own `aria-label`; only
the Effective column's popover does.
Known v1 limits: the Coder Agents settings alert is organization-scoped,
so it can warn about an override that some members fall back from for
lack of provider credentials. The gauge checks the viewer's provider
access, so on a shared chat its label can differ from the owner's
runtime trigger (Codex P2, declined). Compaction itself is unaffected.
## Screenshots
From remote UAT round 13 on
|
||
|
McKayla はな
|
86ebc72236 | chore: remove unused autofill-parameters endpoint and experiment (#30525) | ||
|
Michael Suchacz
|
d28a433b23 |
perf(coderd/database): index chat message pagination (#30562)
Loading a chat's history can take seconds when its messages sit far from the end of the `chat_messages` id space, because no index returns one chat's live messages in id order. Given the `LIMIT`, the planner chooses `chat_messages_pkey` instead and filters by `chat_id`, so it reads other chats' messages until it reaches this chat's. On dogfood, the newest page of one idle long chat took 21 s: `Index Scan Backward using chat_messages_pkey` discarded 614,776 rows and did 78,749 disk reads. Over the past 7 days, 108 of 33,745 `GetChatMessagesByChatIDDescPaginated` calls took more than 1 s, and 10 took more than 10 s. `GetChatMessagesByChatIDAscPaginated` has the same problem in the other direction: for `after_id = 0, limit 1`, which chatd uses as a first-turn probe, dogfood's plan scans forward from the table's oldest row. 305 of its 2,863 calls took more than 1 s. This PR adds migration 000614: `CREATE INDEX idx_chat_messages_chat_id ON chat_messages (chat_id, id DESC) WHERE deleted = false`. No queries change. **Migration cost:** a plain `CREATE INDEX` inside the migration transaction, which blocks writes to `chat_messages` until the upgrade commits. Reads continue. On a 1.01M-row fixture (641 MB heap) the build took 0.8 s and the index is 39 MB. Dogfood has 5.85M rows and a 5.7 GB heap, so expect seconds and about 230 MB. ## Testing Remote UAT through Coder Agents (round 1 PASS) on the 1.01M-row fixture, Postgres 13.21. It ran base |
||
|
Michael Suchacz
|
782fb30f09 |
feat: load whole chat turns per history page (#30524)
Closes [CODAGT-1372](https://linear.app/codercom/issue/CODAGT-1372/load-whole-turns-per-history-page-so-collapsed-working-blocks-fill-in). With collapsed working blocks, a long agent turn renders as one row, but chat history pages hold a fixed 50 messages. A 400-step turn took about eight sequential page loads to fill, and its block showed partial labels ("at least", "or more") until then. `GET /api/v2/chats/{chat}/messages` now returns `turn_start_id` on newest-first pages: the ID of the user prompt that starts the turn containing the page's oldest message. It comes from a new `GetChatTurnStartID` query on the existing `idx_chat_messages_user_prompts` index, so there is no migration. Soft-deleted prompts and model-only user rows never start a turn. The history query then fills each page back to that prompt with requests of up to 200 messages, capped at 1000 messages per page, so a page holds whole turns and a complete block renders without partial labels. Past the cap, or when a fill request fails or returns no messages, the page keeps what loaded and ends mid-turn, and the next page continues it, as before. Because the fill runs inside the page fetch, a chat now waits for its fill requests (one to five) before it first renders: with an injected 500 ms per-request delay, a chat whose newest turn hits the cap appeared after 3.8 s instead of 1.1 s. ## Screenshots From the dogfood UAT, the same 200-step turn with its next history request held, so each label shows only what loaded before it: **Before**, on the base commit `29e876df4da`, after two 50-message pages: "Worked for at least 1m 35s (48 steps or more)", partly hidden by the loading indicator.  **After**, on `186a9b053a0`, after the first page: "Worked for 6m 39s (200 steps)".  A 750-step turn, larger than one 1000-message page, is exact once the next page loads:  A malformed fill response keeps the loaded messages and the partial label instead of showing "Failed to load chat", as `ecdfb27ec29` did:  ## Testing - Go: `TestGetChatMessages_TurnStartID` (pages spanning turns, deleted prompts, model-only user rows, polling and empty pages) and the dbauthz suite entry. - Vitest: fill request parameters, the 1000-message cap, failed, malformed, and empty fill responses, and servers without `turn_start_id`. - Dogfood UAT through Coder Agents: PASS on `186a9b053a0` (round 4): whole-turn first pages, the 1000-message cap and its continuation, fill failures, a live turn, and API checks for deleted prompts and model-only user rows. Round 3 on `ecdfb27ec29` failed: a malformed fill failed the chat load, and an empty fill reporting `has_more` repeated forever. `7266f468290` fixes both, with regression tests. ## Review record - Test audit with mutation checks: the Go test now covers a page spanning two turns, which catches a lookup keyed on the newest message. A re-audit of later test changes changed nothing; the one surviving mutation drops the `!older.has_more` stop, costing one extra request when the prompt is deleted mid-fill. - Frontend review (8 roles, 5 rounds): fixed doc, naming, loose-type, and test findings. Its suggestions to catch only the fill request (DF5) and drop the empty-page stop (SD4) caused the round 3 failures and are reverted. Kept: SD1 (`MAX_MESSAGES_PER_REQUEST` is used once, named like `MESSAGES_PAGE_SIZE`) and round 5 SD4 (`!older.has_more` is the endpoint's pagination signal). Declined DF6: the `queued_messages` refetch race predates this change, and a failed refetch fill only unloads older history that reloads on scroll. - Codex: no findings on `186a9b053a0`. - Known: a hung fill delays the first render (no client timeout); fills are not cancelled when the user leaves; the top of history costs one extra empty request; injected malformed fill rows break the page as they do on the first page. > Xum opened this PR on behalf of @ibetitsmike. |
||
|
Mathias Fredriksson
|
befda9b29f |
docs: mention chat reasoning_tokens is part of output_tokens (#30500)
Document that reasoning_tokens is included in the output_tokens on ChatMssageUsage, which is the case for all known providers. Refs CODAGT-1350 |
||
|
Atif Ali
|
261269aff0 |
fix: support path-aware OAuth discovery for remote MCP (#30168)
## Problem VS Code discovers Coder's MCP endpoint as a path-qualified OAuth resource, but Coder advertises the deployment root. Accepting MCP-bound tokens also requires keeping REST-backed tool calls within the originating MCP resource rather than granting those tokens direct REST access. ## Fix - Publish path-aware protected resource metadata and authentication challenges; keep authorization-server metadata at the root. - Validate the MCP audience and delegate tool requests through a private in-process transport that retains identity, scopes, expiry, revocation, and streaming cancellation. - Reject unsupported MCP path aliases and validate SDK path components before URL resolution. - Add discovery, audience, authorization, endpoint-isolation, and transport lifecycle regression tests and update integration docs. ## Validation VS Code completed OAuth and returned the authenticated user through a real MCP tool. Official MCP SDK 1.30.1 completed discovery, registration, PKCE, tool execution, and refresh. These live checks preceded the final path hardening; the hardened paths are covered by automated regressions. Affected Go package tests, targeted race tests, backend/SDK lint, and targeted documentation checks passed during self-review. The exact PR commit was also checked separately from the uncommitted consent-scope fix. > [!NOTE] > Tokens are MCP-audience-bound, but grants still include `coder:all`; least-privilege scopes remain a separate requirement ([PLAT-639](https://linear.app/codercom/issue/PLAT-639)). Preview: https://coder.com/docs/@fix/mcp-internal-delegation/ai-coder/mcp-server <details> <summary>Implementation decisions and review context</summary> 1. Treat `/api/experimental/mcp/http` and its single-trailing-slash form as supported MCP resource paths; reject other aliases and descendants before route authentication. 2. Preserve deployment-root discovery and authorization-server metadata for existing REST integrations. 3. Bind private delegation to the authenticated API-key ID. Revalidate the original credential and enforce normal RBAC/scopes on every internal REST request; do not derive delegation from HTTP headers. 4. Keep internal HTTP, streaming, and WebSocket traffic in-process with no network fallback. Close the transport on outer-request cancellation and normal completion. 5. Validate workspace owner/name path segments before SDK URL construction, including tool-level tests that malformed arguments cannot select a different REST endpoint. 6. Keep compatibility and delegated tool execution together: discovery-only changes would allow authentication while leaving real MCP tool calls broken. </details> > 🤖 This PR was created with the help of Coder Agents, and needs a human review. 🧑💻 |
||
|
cdrciandgithub-actions[bot]
|
315789c6bf |
chore: refresh AI model price book (#30498)
## Price book changes 27 models added, 8 models removed, 46 models changed. <details> <summary>Added</summary> - anthropic/claude-haiku-5-5 - azure/claude-haiku-5-5 - azure/grok-4.6 - bedrock/anthropic.claude-haiku-5-5 - bedrock/au.anthropic.claude-haiku-5-5 - bedrock/eu.anthropic.claude-haiku-5-5 - bedrock/eu.anthropic.claude-sonnet-5-5 - bedrock/global.anthropic.claude-haiku-5-5 - bedrock/global.zai.glm-5.3 - bedrock/in.anthropic.claude-haiku-4-5-20251001-v1:0 - bedrock/in.anthropic.claude-opus-5 - bedrock/in.anthropic.claude-sonnet-5 - bedrock/jp.anthropic.claude-haiku-5-5 - bedrock/us.anthropic.claude-haiku-5-5 - bedrock/us.anthropic.claude-sonnet-5-5 - bedrock/us.zai.glm-5.3 - copilot/claude-haiku-5.5 - openrouter/anthropic/claude-haiku-5.5 - openrouter/apodex/apodex-1.1-mini:free - openrouter/google/gemini-nano-banana-2.1 - openrouter/inclusionai/ling-3.1-flash - openrouter/mistralai/mistral-large-4-0 - openrouter/unbiased/pareto-26.10-preview - vercel/anthropic/claude-haiku-5.5 - vercel/google/gemini-nano-banana-2.1 - vercel/mistral/mistral-large-4 - vercel/stealth/glyph-cluster </details> <details> <summary>Removed</summary> - openrouter/kwaipilot/kat-coder-pro-v2.5 - openrouter/qwen/qwen3.8-27b:free - openrouter/stealth/space-bunny-alpha - vercel/deepseek/deepseek-v3.1-terminus - vercel/inclusionai/ling-3.0-flash-sante-free - vercel/liquid/d1 - vercel/moonshotai/kimi-k2-thinking - vercel/stepfun/step-3.5-flash </details> <details> <summary>Changed</summary> - anthropic/claude-sonnet-5-5 - bedrock/anthropic.claude-sonnet-5-5 - bedrock/global.anthropic.claude-sonnet-5-5 - copilot/claude-sonnet-5.5 - openrouter/anthropic/claude-sonnet-5.5 - openrouter/deepseek/deepseek-v3.1-terminus - openrouter/deepseek/deepseek-v3.2 - openrouter/deepseek/deepseek-v4-flash - openrouter/deepseek/deepseek-v4-flash-0731 - openrouter/deepseek/deepseek-v4-pro - openrouter/deepseek/deepseek-v4.1-flash - openrouter/inclusionai/ling-3.0-flash-fin - openrouter/meta-llama/llama-4-maverick - openrouter/meta/muse-glimmer-30b - openrouter/moonshotai/kimi-k2.6 - openrouter/moonshotai/kimi-k3 - openrouter/nvidia/nemotron-3-nano-30b-a3b - openrouter/nvidia/nemotron-3-super-120b-a12b - openrouter/nvidia/nemotron-3-ultra-550b-a55b - openrouter/nvidia/nemotron-3.5-lightning - openrouter/openai/gpt-5.6-sol-pro - openrouter/qwen/qwen3-235b-a22b-2507 - openrouter/qwen/qwen3-next-80b-a3b-instruct - openrouter/qwen/qwen3.5-27b - openrouter/qwen/qwen3.5-35b-a3b - openrouter/qwen/qwen3.5-397b-a17b - openrouter/qwen/qwen3.6-27b - openrouter/qwen/qwen3.8-27b - openrouter/z-ai/glm-5.1 - openrouter/z-ai/glm-5.2 - openrouter/z-ai/glm-5.3 - openrouter/~anthropic/claude-haiku-latest - openrouter/~anthropic/claude-sonnet-latest - openrouter/~deepseek/deepseek-flash-latest - openrouter/~deepseek/deepseek-pro-latest - openrouter/~deepseek/deepseek-v4-flash-latest - openrouter/~moonshotai/kimi-latest - openrouter/~z-ai/glm-flash-latest - openrouter/~z-ai/glm-latest - vercel/anthropic/claude-sonnet-5.5 - vercel/arcee-ai/trinity-large-thinking - vercel/google/gemini-3.8-flash-lite-tts - vercel/google/gemini-3.8-flash-tts - vercel/inclusionai/ling-3.0-flash-sante - vercel/poolside/laguna-s-2.1 - vercel/xiaomi/mimo-v2.6-flash </details> ## Review notes Regenerated by `make gen/aibridge-prices` from the live [models.dev](https://models.dev) catalog. Both artifacts come from one snapshot, so they ship together: - `coderd/aibridge/prices/data/prices.json` - `site/src/modules/aiModels/knownModels/knownModelsGenerated.json` These are customer-visible cost numbers taken from upstream data, so this PR is never merged automatically. The summary above lists what moved; check the diff for exact figures before approving. Opened automatically by the [aigateway-prices-refresh workflow](https://github.com/coder/coder/actions/runs/37754651010). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> |
||
|
Mathias Fredriksson
|
788a8bc1ba |
test: skip agent tool call tests until netstack TIME_WAIT fix (#30465)
Skip `TestAgent_ToolCall` and `TestEditMessageCancelsToolCallsOnAgent` until the agent netstack fix in PLAT-717 lands. The agent's gVisor netstack accepts connections through `tcp.Forwarder` and has no listening endpoint, so it drops a SYN that reuses a source port it still holds in TIME_WAIT (google/gvisor#15013). Agent API requests open a new connection each and the agent closes first, so a client that reuses a port within 60s hangs for about 63s. Both tests make enough requests to one agent to hit it (CODAGT-1336, CODAGT-1331). The same stall was reproduced against a production agent on dogfood, so this is a product bug, tracked in PLAT-717. |
||
|
Sas Swart
|
ef590afda7 |
fix: coalesce AI session metadata in list query (#30351)
Update `ListAIBridgeSessions` to return `{}` when selected
session/interception metadata is SQL NULL, regenerate database code, and
add a regression test that inserts an interception with NULL metadata
and verifies the sessions list succeeds with empty metadata. No schema
migration is included; existing insert-path normalization remains
unchanged.
Fixes AIGOV-694
|
||
|
dylanhuff-at-coderandNick Vigilante
|
b4d51012da |
feat: allow template version selection for workspace builds (#30199)
Add named template version selection to `coder start` and `coder stop`, and independent stop/start selection to `coder update` with `--stop-template-version` and `--template-version`. Stop still defaults to the workspace's current version, and update starts with the active version by default. Explicit selections use the selected version for parameter preparation and builds, keep server permissions authoritative, and never silently fall back to the active version. Classic stop builds retain previous parameter values omitted by the selected stop version, so a later start can reuse them across separate commands or an interrupted update. Declared parameters still follow normal validation, and start builds still discard removed parameters. Update now returns an error and retry guidance when a start apply fails after stopping, including unflagged updates that previously returned success. Closes #29100 [PLAT-616](https://linear.app/codercom/issue/PLAT-616/cli-allow-specifying-a-template-version-for-stopstart-builds-and-both) --------- Co-authored-by: Nick Vigilante <nickvigilante@users.noreply.github.com> |
||
|
Jakub Domeracki
|
b4fb7901e6 |
docs: clarify that disable-password-auth also applies to owners (#30362)
The `--disable-password-auth` help text says owners can still sign in with a password "to avoid potential lock out", and a comment in `coderd/userauth.go` says the same. Neither is true. #6193 removed the owner bypass on purpose, so the flag blocks password login for everyone. The recovery advice was also wrong. It named a command that doesn't exist (`create-admin`), and any admin that `create-admin-user` creates while the flag is on still can't sign in. This PR changes docs and comments only: - Rewrites the flag description to say it applies to owners too and how to recover access. Regenerated the CLI and config reference docs and the golden files. - Fixes the misleading comment in `coderd/userauth.go`. - Adds the same warning and recovery steps to `docs/admin/users/password-auth.md`. > [!NOTE] > Generated by Coder Agents on behalf of @jdomeracki-coder. |
||
|
Paweł Banaszewski
|
f08bfddfa7 |
feat: validate AI Gateway actor and credentials in proxy mode (#30225)
Extends the AI Gateway `forwardingHandler` with actor check, provider credential resolution, and initial interception metadata. Missing provider credentials return 403 Credential resolution failures return 500 with details kept in logs. AWS-signed bridged credentials remain unsupported. Generated by Coder Agents on behalf of @pawbana. |
||
|
Michael Suchacz
|
28fe61ffa1 |
feat(coderd): evaluate read_template parameters for the chat owner (#29561)
The chat `read_template` tool returned the parameter defaults recorded at template import. Defaults that depend on the workspace owner, such as a region chosen from the owner's groups, were therefore wrong for many users, and an agent that omitted the parameter in `create_workspace` built a workspace with a different value than the tool had shown. `read_template` now evaluates the active version's parameters for the chat owner with no inputs, through `dynamicparameters.Prepare` and the same `Render` call `wsbuilder` makes for a first build, via a `RenderTemplateParametersFn` hook wired from coderd like the other chat hooks. The result carries a `parameters_note` naming the source: owner-evaluated build values, import rows for templates on the classic parameter flow (which `wsbuilder` also builds from), a retry note while the version is still importing, or import rows with a warning when evaluation fails. A parameter whose evaluated default fails its own validation is reported as `error` instead of a usable `default`. Only an evaluated default is checked: without one, preview and the static renderer validate an empty value, so required parameters and unevaluated defaults are never reported as broken, and their remaining errors surface at build time. Fields preview cannot evaluate before the build are shown as evaluated rather than filled from the import row, because preview's output does not distinguish a default, option set, or validation bound that is unknown (it depends on `data.coder_provisioner`) from one that is null for this owner: an unevaluated default carries a `default_note` saying so, an option with an unknown value is omitted, with preview's invalid-options error surfaced as `error` when the parameter has an evaluated default, and bounds appear exactly as rendered. The one import-row fill is for parameters a `module_not_loaded` render dropped, keyed on the same diagnostic code `wsbuilder` uses. Import rows carry no module provenance, so that fill also restores root-level rows the render omitted for this owner; every restored row carries a `note` marking it an import-time estimate. The build-values note is scoped to a request that omits every parameter and matches no preset, since a default that depends on another parameter is re-evaluated against the values `create_workspace` passes, a preset can match without a `preset_id`, and a claimed prebuilt workspace can keep its own values. When evaluation falls back, the warning logs only diagnostic codes, because diagnostic text can quote owner attributes. Measured on dogfood, evaluation adds about 4 ms for a two-parameter template and about 70 ms for the 6-parameter, 19-module `coder` template per `read_template` call, matching the workspace create form's evaluate endpoint, with no file cache growth across repeated calls. > Xum acted for Mike on this pull request. |
||
|
Michael Suchacz
|
9cfe27063d |
test(coderd): reduce chat test fixture and timer waits (#29523)
Reduce repeated database/server fixtures and fixed timer waits in chat tests without changing production code or assertions. `TestTransitionMatrix_AllCombinations` shares one database and seeded dependencies across 272 leaves, with a unique chat and recording publisher per leaf. `TestPostChatFile` shares one authenticated client across 20 upload leaves. `TestResolveFallbackModelConfigID` shares one database with independently seeded model configurations. The requires-action test closes its server after the expected state persists; the stopped-workspace test advances a trapped Quartz validation timer; two tool-filtering tests no longer start unused workspace agents. | Test | Before | After | |---|---|---| | `chatstate/TestTransitionMatrix_AllCombinations` (272 leaves) | 94.06s | 16.86s | | `coderd/TestPostChatFile` (20 leaves) | 31.23s | 0.95s | | `chatd/TestRequiresActionChatPersistsWaitingStatusLabel` | 11.21s | 1.25s | | `chatd/TestSubagentChatExcludesWorkspaceProvisioningTools` | 7.90s | 2.84s | | `chatd/TestPlanModeSubagentChatExcludesAskUserQuestion` | 8.25s | 3.00s | | `chatd/TestStoppedWorkspaceWithPersistedAgentBindingDoesNotBlockChat` | 6.91s | 1.91s | | `chatd/TestResolveFallbackModelConfigID` (12 leaves) | 7.58s | 0.34s | Independent review measurements on September 18, 2026: median summed leaf time from two runs per side in A/B/B/A order, comparing base `0ea777a341` with head `e378f9e936`. Every run used `-tags=testsmallbatch -count=1 -v -p 1 -parallel 8`, PostgreSQL 17, and the same test selection per package. Other agents shared the host, so absolute times varied. The speedups reproduced, but not the original ratios: the matrix improved 5.58x and uploads 32.87x. Summed leaf time excludes shared parent fixture setup and cleanup; it is not package wall time. Median package elapsed was 19.26s to 9.76s for the full `chatstate` package, 4.47s to 1.28s for `coderd` with only `TestPostChatFile`, and 11.43s to 3.36s for the five targeted `chatd` tests. Compilation time is excluded. <details> <summary>Investigation notes</summary> - Transition matrix: the shared user, organization, membership, provider, and model are not mutated by any leaf. Each leaf seeds its own chat, messages, and queue, and copies the fixture with a new recording publisher (`transitions_matrix_test.go:617,728,762`). `ChatMachine.Update` still opens real transactions on the shared database store; no outer transaction masks rollback assertions (`chatstate/machine.go:161`, `transitions_helpers_test.go:891`). Parent cleanup runs after every leaf. - Requires-action: the old idle hook reached its 10s timeout because the runner remains active while awaiting dynamic-tool results (`testhooks.go:14`, `worker.go:114`). The test now waits for the persisted status and label, closes and drains the server, and retains the no-web-push assertion (`chatd_test.go:3072`). - Stale binding: the test injects a new Quartz mock, traps the 5s validation timer, waits for `MustRelease` to finish timer registration and for the dial to start, then advances the clock (`chatd_test.go:4907`). The one-dial, model-call, and unavailable-workspace tool-result assertions remain intact. - Tool filtering: both tests create templates but no workspace, and their mocked model only invokes `spawn_agent` before returning text. The removed workspace agents could not authenticate and were never used by the tested flow; their shutdown added about 5s (`chatd_test.go:229,384`). - Fallback models: configurations and providers are independently inserted. Most leaves use unique organizations; the two default-organization cases insert distinct models, and only one inserts a default. No leaf mutates another leaf's model or provider (`chatd_internal_test.go:3512`). Validation: all changed tests passed with `-count=5` and `-race -count=2`; the full `chatstate` package passed both checks (2,350 and 940 test executions, including parents). All 272 matrix, 20 upload, and 12 fallback leaves remain present. `gofmt -l`, `git diff --check`, and `golangci-lint run` on the three changed packages were clean. The full `coderd` package was not run. No correctness changes were needed during review. `ARCHITECTURE.md` was read; this test-only PR does not affect the documented architecture, so no TODO or architecture edit is needed. </details> _Investigated, implemented, and validated by Coder Agents on behalf of @ibetitsmike._ |
||
|
Ehab Younes
|
fc01bee831 |
fix: keep new API keys within the caller's scopes and allow list (#30256)
A scoped key holding api_key:create could mint a broader key, e.g. a token scoped to [api_key:create user:read] could mint coder:all.
- New keys must be covered by the caller's scopes and allow list, and inherit them when none are requested
- Requests beyond the caller return 403
- New key expiry is capped at the caller's
- Covers POST /users/{user}/keys/tokens, POST /users/{user}/keys, app auth-redirect, and /oauth2/authorize (invalid_scope on excess grants or a restricted session allow list)
- coder:all sessions, including browser logins, behave as before
- Not addressed: workspace start still mints a coder:all key for the owner
|
||
|
Michael Suchacz
|
67a781ce1d |
feat: remove model_intent from the execute, process_output, and advisor tools (#30397)
The built-in `execute`, `process_output`, and `advisor` tools offered an
optional `model_intent` argument whose text replaced the tool row's
default label. Models fill it inconsistently, so most chats mixed intent
labels with plain command labels. On dogfood over three days, Claude
Opus 5.5 sent it on 17% of `execute` calls through Bedrock and 30%
through the Anthropic API. The rate follows reasoning effort: the few
chats at max or xhigh effort sent it on every call, while chats at high,
medium, or default effort sent it on about a third of calls or fewer.
This removes the argument from the three tool schemas and always renders
the command-based labels ("Ran ...", "Started ... in the background",
"Checked ...", "Consulted the advisor"). It also deletes the frontend
helpers, tests, and stories that existed only for the intent labels.
- Old chats keep working. chatd's decoders and its tool input check
ignore undeclared keys, so calls that still carry `model_intent` run as
before and their rows show the default label.
- The per-server `model_intent` option for MCP tools is unchanged. That
field is required in the schema, and Opus 5.5 fills it on 96 to 99.7% of
those calls.
- The MCP workspace `execute` and `process_output` tools now match the
Coder Agents contract apart from their `workspace` argument, so the
golden test drops its Agents-only exception.
- Pixel diffs from this change: the deleted intent stories and the new
default labels in `AllToolIconsTranscript`,
`PolicyBadgeCoversEveryRenderer`, and
`StreamedAdvisorResultOverlaysDurableCall`.
## Testing
- `go test` for `chattool`, `chatadvisor`, `toolschema`, `coderd/mcp`,
and `coderd/x/chatd -run Advisor`; `go vet ./coderd/x/chatd/...
./coderd/mcp/...`.
- Red check: re-adding the field to `ExecuteArgs` fails
`TestWorkspaceToolsGolden`.
- Vitest unit tests for the tool utils, the generic tool row, and chat
parsing (5 files) and the affected Storybook files (4 files); `biome`,
`oxlint`, and `tsc`.
- `make pre-commit` passed on the rebased head. The pre-push hook was
skipped per the maintainer's standing preference; CI covers it.
## Review record
- `frontend-review` (FE1 to FE10): all PASS or N/A.
- `coder-frontend-review` (8 parallel reviewer roles): no findings. Two
optional side notes were declined: a nested ternary for `ExecuteTool`'s
label (SD3 discourages them) and a story for an old call that still
carries `model_intent` (TS7: it shows the same state as the existing
default-label stories).
> Xum opened this PR on behalf of @ibetitsmike.
|