mirror of
https://github.com/coder/coder.git
synced 2026-09-21 20:51:01 +08:00
995d7fe31bb30713c765a302a1a8a71b86ae5f15
4374
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Cian Johnston
|
6079c514ee |
fix: follow-up fixes for conditional VCS requests (#27711)
Follow-ups from #27627 - Memoizes `Config.Git()` with a mutex so the provider's ETag response cache survives across calls. Only successful construction is cached; errors are retried. - Moves the HTTP client onto `Config.HTTPClient`, wired through `ConvertConfig`, so `Git()` no longer takes a per-call argument that would be silently ignored after memoization. - `newGitHub` and `newGitLab` now return `(Provider, error)`, eliminating the typed-nil-interface class in `gitprovider.New` rather than the single instance. - Gates the 304 branch on a `haveCached` flag instead of a nil body check. - Only caches bodies that decode successfully, preventing poisoned entries. - Keys the response cache on the full token digest rather than a truncated prefix. - Tests added: `TestConfigGitMemoizesProvider`, `TestConfigGitRetriesOnConstructorError`, `TestGitLabConstructorErrorReturnsNilInterface`, `TestResponseCacheStore`, `TestConditionalRequestReuse/MalformedResponseNotCached`; `TestConvertYAML/CustomScopesAndEndpoint` now asserts `Config.HTTPClient` wiring. Follow-ups tracked in #28139, #28140, #28141, #28142. > 🤖 Generated by Coder Agents on behalf of @johnstcn. |
||
|
Jaayden Halko
|
fa8ffe4eda |
feat: report agent runtime hours usage in entitlements (#27985)
Populate `FeatureAgentRuntimeHours.Actual` on every entitlements refresh for licenses that grant the feature. A new `GetTotalUsageHBAgentRuntimeV1` query sums `runtime_ms` over the license's usage period, reading `usage_events` directly: `hb_agent_runtime_v1` is exactly one row per hourly bucket deployment-wide with `created_at` at the bucket start, enforced by the unique partial index introduced in #27983. The measurement reuses the shared `measureUsage` policy from #27984 through a new `AgentRuntimeMsFn` closure (usage publisher subject): failures publish the stable `LicenseAgentRuntimeUsageUnavailableErrorText` and log the cause. Usage is floored to whole hours, matching the unit of the `agent_runtime_hours_*` claims, and at most one warning is emitted per refresh: reaching the allocation supersedes the advisory soft limit. The dashboard renders the soft-limit advisory muted without a sales link and treats the runtime usage-unavailable text as a diagnostic. **Precise usage.** `Feature.ActualMs` (JSON `actual_ms`), set only for `agent_runtime_hours`, carries the exact stored milliseconds backing the floored `Actual` so clients can render fractional hours (e.g. `10.3`). It has the same freshness as `Actual`; the whole-hour warning thresholds are unchanged. **Unlimited licenses.** A license minted with the unlimited (`-1`) allocation decodes to an enabled feature with a nil `Limit` (#27984), so the warning write-back now guards the allocation dereference: no thresholds can exist for an unlimited license, so no runtime hours warning is ever emitted, while `Actual` is still measured and published. `Feature.Compare` is unchanged; for usage-period features the issued-at/end dates decide first, so a metered feature outranks an unlimited one only on an exact timestamp tie, an edge pinned by a `TestFeatureComparison` case and documented on `decodeAgentRuntimeHours`. **Grandfathered premium licenses.** Premium licenses without `agent_runtime_hours_*` claims are now granted the feature disabled with a zero limit over the license term, identical to an explicit `allocation: 0`: usage is measured and published for every Premium deployment, and chatd's pooled admission (#27902) caps concurrent agentic chats until a license with a positive allocation is added. The default carries a fixed early `UsagePeriod.IssuedAt` (2026-08-01, the same mechanism as the managed-agents default) so any license actually carrying the claims outranks it in the `AddFeature` merge regardless of the licenses' relative issue dates; the constant must stay earlier than the earliest legitimately issued claim-bearing license. Zero allocations (explicit or grandfathered) emit no deployment-wide warning banner: those deployments are steered by the in-page upgrade CTA and the concurrency cap. Enterprise licenses are unchanged. Part 3 of a 3-PR stack splitting up #27796 (see there for review history). Stack: #27983 → #27984 → this PR. Closes CODAGT-852. |
||
|
Asher
|
b5d18bb9c9 | feat: add redirect URL override for external auth (#28082) | ||
|
Cian Johnston
|
aa80fa3550 | fix(coderd/externalauth): also retry on 503 (#28218) | ||
|
Susana Ferreira
|
95328f1ead |
fix: label unpriced token usage metric by provider name and type (#28210)
## Problem The `provider` label was inconsistent between AI Gateway metrics. Every metric emitted by the gateway labels `provider` with the provider instance name, for example `anthropic-eu`, while `coder_ai_gateway_cost_control_unpriced_token_usage_records_total` used the provider type, for example `anthropic`. The two could not be correlated on `provider`. The metric was also inconsistent with itself: the path where a provider fails to resolve labelled by instance name, and the path where a model has no price labelled by type. The type is still worth exposing, since prices are keyed on `(provider_type, model)` and that is what an operator needs to add a price. ## Changes - Label the metric with `provider` (the instance name, consistent with the other gateway metrics) and add `provider_type` (the configured type the price is keyed on). - Use `unknown` for `provider_type` when the provider does not resolve to a configured type. - Log the unresolved-provider case at `warn` instead of `info`. A missing price is an expected steady state, but a provider that cannot be resolved is not. - Update the metrics docs and the `metricsdocgen` fixture. Closes [AIGOV-574](https://linear.app/codercom/issue/AIGOV-574) > [!NOTE] > Initially generated by Claude Opus 5, modified and reviewed by @ssncferreira |
||
|
Jaayden Halko
|
20c376a575 |
fix: enforce uniqueness and hour alignment for agent runtime usage events (#27983)
The usage generator writes `hb_agent_runtime_v1` rows with `created_at` at the UTC hourly bucket start and exactly one row per bucket, but nothing in the schema enforced either invariant. A duplicate bucket row under a different id would be double-counted by any consumer summing `runtime_ms`, and a misaligned `created_at` would skew which usage period a bucket is attributed to. This replaces the non-unique partial index `idx_usage_events_agent_runtime` (from migration 000561) with a unique index of the same shape and adds an hour-alignment `CHECK` constraint. Both statements validate existing rows: every supported writer has always produced conforming data, so a pre-existing violator is anomalous and failing the migration loudly beats silently rewriting usage rows. `generateBucket` treats a unique violation on the bucket index as another replica having won the race, mirroring the existing `ON CONFLICT (id)` no-op for committed rows. The `coderd/notifications` sync commit and its revert cancel out (the drift they addressed was fixed on main by #27979); the PR's net diff is only the usage-event changes. Part 1 of a 3-PR stack splitting up #27796 (see there for review history). Stack: this PR → #27984 → #27985. |
||
|
Michael Suchacz
|
521c383f6b |
fix: repair stale chat agent bindings after workspace rebuild (#28152)
## Problem When a chat is bound to a workspace, chatd persists `chats.agent_id` pointing at a specific workspace agent, and it only rebinds on the next chat turn. A workspace stop/start creates a new agent with a new ID in the latest build, so the chat page resolves the stale agent ID to `undefined` and the right sidebar silently drops Terminal, Desktop, Browser, apps, and ports even though the workspace is running. The existing read-time enrichment only filled nil agent IDs and skipped stale non-nil ones, so refreshing did not help until the user sent another message. ## Fix - `coderd/exp_chats.go`: single-chat reads now repair agent IDs that no longer resolve in the workspace's latest build, using the same `agentselect.FindChatAgent` selection chatd uses. A repaired binding also carries the latest build's ID so the response never pairs the new agent with the previous build. Bindings that still resolve are preserved, and repair stays best-effort and response-only (no write-on-read). List reads keep the previous nil-fill-only behavior because validating existing bindings would cost a per-workspace authorization lookup per listed chat. - `site/src/pages/AgentsPage/AgentChatPage.tsx`: the workspace watch update handler detects when a running workspace's latest build no longer contains the chat's bound agent and invalidates the chat query once per chat/build/binding key for immediate recovery, and the chat query polls every 30 seconds while the binding remains unresolved so a transiently failed repair retries even when an idle workspace publishes no further watch events. The watch stream replays the current workspace on every (re)connect, so this covers rebuilds that happen while the page is open or disconnected; page loads are covered by the server-side repair. The workspace-watcher bailout now also keys on `latest_build.id` so a rebuild propagates while the page is open. - `site/src/api/queries/chats.ts`: chat watch events replay the persisted (pre-repair) binding, so the summary merge adopts a snapshot's `build_id` only when the snapshot agrees on `agent_id`, keeping the repaired agent/build pair atomic in the caches. ## Testing - `go test ./coderd -run TestEnrichChatAgentIDs` covering repair, keep-valid, selection-error, list-mode-skips-bound, and no-workspaces cases. - Storybook interaction story `RecoversSidebarAfterWorkspaceRebuild` exercising the watch-event to chat-refetch to sidebar-recovery flow (verified red without the invalidation, green with it). - `pnpm test AgentChatPage.test.ts` covering the binding-resolution predicate. > Mux created this PR on Mike's behalf. |
||
|
Wyatt FryandEthan Dickson
|
a005e5cd22 |
feat: add username and email user search filters (#27922)
## Summary User search can now resolve exact `email:` and `username:` terms through `GET /api/v2/users` instead of only supporting fuzzy free-text matches. The database query already had exact email and username filters; this wires the public search parser and API handler to those filters so clients can ask for a single user by email without fetching every user or depending on substring matching. This is the API half of coder/terraform-provider-coderd#403: that provider PR adds `data.coderd_user.email`, and this PR gives it an efficient exact lookup path. ## Testing - `go test ./coderd/searchquery -run '^TestSearchUsers$' -count=1` - `go test ./coderd -run '^TestGetUsersFilter$' -count=1` - Live API test: - Built local enterprise Coder from this branch. - Started Coder on `http://127.0.0.1:39991` against a clean Postgres database. - Created `lookup-target@example.com`. - Verified `GET /api/v2/users?q=email:LOOKUP-TARGET@EXAMPLE.COM&limit=2` returned exactly one user: ```json { "count": 1, "users": [ { "id": "efc6f909-ce0a-4731-bd2f-6e4df417aaa7", "username": "lookup-target", "email": "lookup-target@example.com" } ] } ``` ---   --------- Co-authored-by: Ethan Dickson <ethanndickson@gmail.com> |
||
|
Bobby Ho
|
1aa3553b52 |
fix(coderd): set Cache-Control: no-store on OAuth2 responses (#28143)
No response from the `/oauth2` route tree set `Cache-Control` at all, so
an intermediary cache or customer-operated reverse proxy was free to
apply a heuristic freshness lifetime to a response carrying a live
credential. RFC 6749 §5.1 and OAuth 2.1 §3.2.3 both make an affirmative
`no-store` directive a MUST for the authorization server.
Adds `httpmw.NoStore`, mounted on the `/oauth2` and
`/api/v2/oauth2-provider` trees, setting `Cache-Control: no-store` and
`Pragma: no-cache` on every response from them. OAuth 2.1 drops `Pragma`
because RFC 9111 §5.4 deprecates it as a request-only field, so sending
both is conformant under either reading. Not operator-configurable,
since both specs say MUST.
## Scope
- **Both trees, not just `POST /oauth2/tokens`.** The mount is one line
either way, and the wider scope also covers DCR registration, client
configuration read and update, the authorize 302 whose `Location` query
carries the code, and `POST /oauth2-provider/apps/{app}/secrets`, which
returns a plaintext client secret. A route added later inherits the
headers, which matters for PLAT-449.
- **A middleware, not a hook in `httpapi.Write`.** Three write paths
never call it: `POST /oauth2/revoke` and `DELETE
/oauth2/clients/{client_id}` write a bare status, and
`writeOAuth2RegistrationError` encodes its own JSON.
- **`/.well-known/*` deliberately excluded.** Public discovery metadata,
and RFC 9728 §5 asks for the opposite treatment. Assertions pin the
exclusion so a later hoist onto a higher router fails CI.
- **Session-credential routes left alone.** `/users/login`,
`/users/otp/change-password`, and `/users/{user}/keys/*` have the same
gap, but PLAT-448 is scoped to OAuth2 and reaching into session auth
changes the risk profile.
Every credential-returning route here is a `POST`, and RFC 9111 §3 bars
heuristic caching of `POST` responses, so this is defense-in-depth
against a non-conformant intermediary rather than a live caching bug.
Both specs say MUST regardless of what caches would actually do.
## Note for PLAT-498
`DELETE /oauth2/tokens` now carries `no-store` and is wrapped in
`apiKeyMiddleware`, which is mounted inside the `/oauth2` tree and
therefore runs after this middleware. It is the one route where both can
write `Cache-Control`, and PLAT-498's write must not replace `no-store`
with something weaker such as `private`. `POST /oauth2/tokens` cannot
overlap, since it deliberately has no `apiKeyMiddleware`.
## Two assumptions testing corrected
- `GET /oauth2/does-not-exist` returns **200**, not 404. Chi runs the
subrouter's middleware chain for unmatched paths, so both headers are
present, but the request falls through to the root router's SPA handler.
The test asserts the headers and deliberately not the status.
- The experiment-disabled case is unreachable from a test binary, since
`RequireExperimentWithDevBypass` short-circuits on `buildinfo.IsDev()`.
A unit test covers the consequence against the `RequireExperiment` it
delegates to.
No schema, `codersdk`, or serpent option changes, so `make gen` produces
no diff. Rollback is a revert.
Refs PLAT-448
|
||
|
Michael Suchacz
|
93c6faf1de |
fix(coderd): send assigned chat model IDs verbatim (#28144)
Fixes #27361 (CODAGT-832). ## Problem When an Agents model was configured under a non-gateway provider type (e.g. Anthropic or OpenAI) with a model ID whose first `/`- or `:`-segment matched a built-in provider name (`anthropic`, `azure`, `bedrock`, `google`, `openai`, `openai-compat`, `openrouter`, `vercel`), `chatprovider.ResolveModelWithProviderHint` parsed it as a canonical `provider/model` reference: the prefix was stripped and the request rerouted to the embedded provider type, overriding the provider the admin explicitly assigned. LLM gateways (e.g. LiteLLM) that namespace their catalogs as `bedrock/...` or `anthropic/...` behind an Anthropic- or OpenAI-type provider failed with an opaque upstream "Model not found", and escaping was impossible (`bedrock/bedrock/...` still rerouted). ## Fix A valid provider hint is now authoritative: `ResolveModelWithProviderHint` returns the assigned provider and the verbatim model ID whenever a hint is present. Canonical `provider/model` and `provider:model` parsing applies only to hint-less resolution paths. Every production call site derives the hint from the model config's explicitly assigned AI provider, so the assignment always wins. The save-time guard rejecting slash-namespaced models on OpenRouter-like providers typed as `openai` (provider named `openrouter` or hosted at `openrouter.ai`) is kept: that combination remains a misconfiguration whose correct fix is the `openrouter` provider type, and rejecting it early beats a confusing upstream error. Its wording no longer claims prefix stripping happens. ## Back-compat note A pre-existing config that relied on stripping (e.g. model `anthropic/claude-x` assigned to an Anthropic-type provider pointing at the real Anthropic API) now sends the prefixed ID verbatim and will get a clear upstream model-not-found error; the admin fixes it by editing the model ID. Nothing in the product ever suggested the canonical form for assigned models. ## Validation - Unit: `TestResolveModelWithProviderHint` updated (hints preserve `bedrock/...`, `anthropic/...`, `provider:...` verbatim; hint-less canonical parsing unchanged), red-green verified against the old ordering. Gateway and openai-type provider routing tests assert verbatim pass-through end to end. - Full `./coderd/x/chatd/...` suites plus `TestCreateChatModelConfig`, `TestUpdateChatModelConfig`, and `TestValidateChatModelConfigProviderModel` pass. - Remote dogfood UAT on real models (PASS): an openai-type provider pointed at a Vercel AI Gateway mount returned a real completion for `anthropic/claude-haiku-4.5`, with trace logs confirming `provider=openai model=anthropic/claude-haiku-4.5` (verbatim, not rerouted); gateway-type (`openai-compat`) routing with `deepseek/deepseek-v4-pro-0813` and the model catalog/picker regressions pass. > Mux acted on Mike's behalf to create this PR. |
||
|
Michael Suchacz
|
d5bb35a49a |
fix(coderd): deflake TestChatMessageWithFiles/FileCapExceeded (#28091)
Fixes the flake tracked in [CODAGT-926](https://linear.app/codercom/issue/CODAGT-926/flake-testchatmessagewithfilesfilecapexceeded). ## Problem `TestChatMessageWithFiles/FileCapExceeded` asserted the rollback of a rejected over-cap send by comparing message counts taken before and after the send. `CreateChat` starts assistant generation asynchronously, so the assistant reply can be persisted between the two reads, making the count check fail even though the rejected message was correctly rolled back ("should have 1 item(s), but has 2"). ## Fix Replace the count comparison with a semantic assertion that the rejected `one too many` message was not persisted, hardened through Codex review rounds: - Scan message history for the rejected marker instead of comparing counts. - Also scan `QueuedMessages`: a busy chat queues the send before file-link validation, so a rollback regression could leave the rejected message queued rather than in history. - Close the queue-promotion race: `getChatMessages` reads history and the queue in two separate database reads, so the assertion first waits for the queue to observe empty; a promoted message must then appear in a fresh history read. ## Verification - Deterministic repro of the exact CI failure signature: waiting for the async assistant reply before the old count assertion reproduced `should have 1 item(s), but has 2` every run. - The new assertion passes under that same forced condition. - Assertion liveness (all temporary red checks reverted): persisting the marker in history fails the history scan; queuing the marker fails the queued scan; queuing the marker and letting it promote fails the post-drain history scan 3/3. - `go test ./coderd -run 'TestChatMessageWithFiles/FileCapExceeded' -count=100` and the full `TestChatMessageWithFiles` parent both pass. > Mux acted on Mike's behalf to create this PR. |
||
|
Michael Suchacz
|
48e1e28638 |
fix(coderd/x/chatd/chattool): make edit_files schema and errors actionable for models (#28121)
## Problem Chat `45b87e40-ffe7-49e5-8932-5fd0bdb9e542` on dev.coder.com failed 57 of 75 `edit_files` tool calls. Every failure was the same: the model omitted `files[].path` (it batched edits per file but only filled in `edits`), and the error relayed back to the model was: ``` POST http://[fd7a:115c:...]:4/api/v0/edit-files: unexpected status code 400: "path" is required ``` The model retried the identical malformed call dozens of times. Two gaps made this sticky: 1. The `edit_files` input schema had no field descriptions, so `path` was only a bare required property. 2. The agent API error reached the model wrapped in HTTP transport noise (method, internal tailnet URL, status code) with no indication of which `files` entry was broken. ## Changes - Add `description` tags to every `edit_files` schema field and state the path requirement in the tool description. - Validate `files` entries in the tool before plan-turn checks and the workspace connection lookup, returning entry-indexed errors such as `files[1].path is required; provide the absolute path of the file to edit; no files in this batch were applied`. - Relay agent API failures with `Message`, `Helper`, `Detail`, and `Validations` from `codersdk.Error` instead of the raw transport-prefixed string. ## Validation - `go test ./coderd/x/chatd/chattool` passes; new tests cover the schema description, entry-indexed validation errors, and transport-noise stripping (each verified red-green by toggling the fix off). - `go build ./...`, `go vet`, and pre-commit (fmt + lint) pass. > Mux created this PR on Mike's behalf. |
||
|
Bobby HoandClaude Opus 5
|
990d24dc42 |
feat: add oauth2 scope columns and single-use delete queries (#28007)
OAuth2 tokens issued by Coder ignore scope entirely. The authorize endpoint parses the `scope` parameter and then discards it, and both grant paths mint API keys with full API access regardless of what the client requested or what the app's allowlist permits. There is also nowhere to put a negotiated scope: nothing carries one from the authorize step to the token it produces. Schema and query groundwork for that pipeline. No behavior change on its own. - Migration `000569` adds a `scope` column to `oauth2_provider_app_codes` and `oauth2_provider_app_tokens`, so a negotiated scope can travel from a code to the token it is exchanged for, and from a token to its refreshed successor. - Existing rows are backfilled to `coder:all`, then both columns become NOT NULL with a non-empty CHECK. Every OAuth2 key is unrestricted in fact today, so the backfill only writes that down, and a caller that omits the column now fails instead of silently issuing full access. - Adds `DeleteOAuth2ProviderAppCodeByIDReturningRow` and `DeleteAPIKeyByIDReturningRow`, which return `sql.ErrNoRows` when the row is already gone. Postgres serializes concurrent deletes on the row lock, so exactly one caller gets a row back, which is what will let the grant paths enforce single use of a code or refresh token without a read-then-write race. - No callers yet. The existing blind deletes and all of their call sites are untouched, and codes and tokens record `coder:all` until a later phase negotiates a real value. Phase 1 of [PLAT-470](https://linear.app/codercom/issue/PLAT-470), tracked as [PLAT-478](https://linear.app/codercom/issue/PLAT-478/phase-1-schema-and-queries). Scope validation at authorize, applying the negotiated scope in the code grant, and refresh narrowing follow as separate PRs. Verified locally: `make gen` and `make lint` clean, the migrations suite passes both up and down, and dbauthz's `TestMethodTestSuite` passes. <details> <summary>End-to-end scope enforcement flow (green marks what this PR touches)</summary> ```mermaid flowchart TD subgraph authorize["/oauth2/authorize"] AZ1["ShowAuthorizePage (GET)<br/>renders consent page"] AZ2["ProcessAuthorize (POST)<br/>scope parsed, then discarded"] Q1["InsertOAuth2ProviderAppCode<br/>gains a Scope param"] AZ1 --> AZ2 --> Q1 end Q1 --> CODES[("oauth2_provider_app_codes<br/>new column: scope text NOT NULL")] subgraph codegrant["POST /oauth2/token, grant_type=authorization_code"] G1["authorizationCodeGrant"] Q2["GetOAuth2ProviderAppCodeByPrefix<br/>now returns Scope"] Q4["DeleteOAuth2ProviderAppCodeByIDReturningRow<br/>added, no caller yet"] G2["apikey.Generate + UserRBACSubject<br/>hardcoded to full access"] G1 --> Q2 --> G2 G1 -.-> Q4 end CODES --> G1 G2 --> Q3 Q3["InsertOAuth2ProviderAppToken<br/>gains a Scope param"] Q3 --> TOKENS[("oauth2_provider_app_tokens<br/>new column: scope text NOT NULL")] subgraph refresh["POST /oauth2/token, grant_type=refresh_token"] G3["refreshTokenGrant"] Q5["GetOAuth2ProviderAppTokenByPrefix<br/>now returns Scope"] Q6["DeleteAPIKeyByIDReturningRow<br/>added, no caller yet"] G3 --> Q5 G3 -.-> Q6 end TOKENS --> G3 Q5 --> Q3 subgraph enforce["Every authenticated API request"] E1["httpmw ExtractAPIKey"] --> E2["APIKey.ScopeSet()"] --> E3["UserRBACSubject"] --> E4["dbauthz authorize"] end TOKENS --> E1 classDef changed fill:#c8e6c9,stroke:#2e7d32,stroke-width:2px,color:#1b3c1e classDef dormant fill:#c8e6c9,stroke:#2e7d32,stroke-width:2px,stroke-dasharray:5 3,color:#1b3c1e class Q1,Q2,Q3,Q5,CODES,TOKENS changed class Q4,Q6 dormant ``` Solid green is added or changed here. Dashed green exists but has no caller yet. Everything else is unchanged, including the enforcement engine at the bottom, which already reads a key's scopes correctly and only needs real data fed into it. </details> <details> <summary>Suggested reading order</summary> Most of the diff is generated. `dump.sql`, `models.go`, `querier.go`, `queries.sql.go`, `check_constraint.go`, and the dbmock and dbmetrics packages all come from `make gen`. 1. `migrations/000569_oauth2_scope_columns.{up,down}.sql`: additive column, backfill, NOT NULL, CHECK, and a `COMMENT ON COLUMN` on each. 2. `queries/oauth2.sql` and `queries/apikeys.sql`: `scope` added to both insert column lists, plus the two new returning-row deletes alongside the untouched originals. The `Get...ByPrefix` selects needed no edit, since they are `SELECT *`. 3. `dbauthz/dbauthz.go`: hand-written wrappers for the two new queries, each fetching by ID, authorizing delete against the fetched object, then delegating. The generic `deleteQ` helper does not fit, since it requires the delete to return only `error`. 4. `oauth2provider/authorize.go` and `oauth2provider/tokens.go`: the only production changes, all behavior-neutral. 5. `dbgen/dbgen.go` and `dbauthz/dbauthz_test.go`: seed threading, plus a case per new query. `MethodTestSuite` fails with "Method never called" for anything untested. Neither type needs to become auditable, which `make lint` confirms by not erroring on `enterprise/audit/table.go`. </details> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Michael Suchacz
|
8d4d0b35dd |
feat: add Coder Agents chat tools to the MCP toolsdk (#28025)
Exposes the experimental Coder Agents chats API through the MCP tool registry, so MCP clients (the hosted `/api/experimental/mcp/http` server and `coder exp mcp server`) can start and drive server-side coding agents. New tools in `codersdk/toolsdk`, all thin wrappers over existing `codersdk.ExperimentalClient` methods: | Tool | Wraps | |---|---| | `coder_create_chat` | `CreateChat` (prompt, optional org, model config, labels) | | `coder_get_chat` | `GetChat` (status, last error, last turn summary, workspace, files) | | `coder_get_chat_messages` | `GetChatMessages` (user-facing parts, chronological, cursor pagination, queued prompts) | | `coder_send_chat_message` | `CreateChatMessage` (queue or interrupt busy behavior) | | `coder_interrupt_chat` | `InterruptChat` | | `coder_archive_chat` | `UpdateChat` with `archived: true` | | `coder_list_chat_model_configs` | `ListChatModelConfigs` (enabled configs with default flag) | Both MCP servers register tools from `toolsdk.All`, so no additional wiring is needed. Responses are trimmed to what an MCP caller needs (IDs as strings, user-facing transcripts) rather than full SDK payloads. No new endpoints and no database changes. Also adds MCP [prompts](https://modelcontextprotocol.io/specification/2026-07-28/server/prompts) for the chat workflows, defined once in `codersdk/toolsdk` and registered by both servers: | Prompt | Purpose | |---|---| | `coder_agents_delegate` | delegate a task to a Coder Agents chat and monitor it to completion | | `coder_agents_check` | check the status and recent activity of an existing chat | Each prompt declares the tools its workflow needs; the stdio server skips prompts whose tools are excluded by `--allowed-tools`. Tests run the tools against a chat-enabled coderdtest instance (fake OpenAI-compatible provider plus in-process AI bridge), covering the full lifecycle, an interrupt against a blocked turn, pagination cursors, permission-dependent model config filtering, and argument validation. Prompt coverage spans SDK rendering, the hosted `prompts/list`/`prompts/get` round trip, and the stdio server including allowlist gating. > Mux created this PR on Mike's behalf. |
||
|
Steven Masley
|
0d0f5b4392 |
test: skip racey tasks test (#28033)
tasks is being removed, so fixing tests is not worth it Closes: https://github.com/coder/internal/issues/1635 |
||
|
Susana Ferreira
|
2d9b6eda8f |
feat: add experimental CLI to price unpriced AI models (#27926)
## Description AI Gateway computes the cost of an interception from `ai_model_prices`, which is seeded on every server start from a price book embedded in the binary. A model the price book does not cover records a NULL cost, so its spend is invisible to cost reporting and is not enforced against budgets. The only fix was to wait for a Coder release that added the model. This adds an experimental CLI, backed by an experimental HTTP endpoint, for pricing those models. Models the price book already covers are rejected, because the seeder re-applies the book on every start and would overwrite an operator price. Support for custom pricing will be handled in https://linear.app/codercom/issue/AIGOV-589/extend-experimental-cli-command-to-set-custom-ai-model-prices. ## Commands ``` coder exp ai-model-prices list [--provider] [--model] coder exp ai-model-prices update [file|-] [--provider] [--model] [--input-price] [--output-price] [--cache-read-price] [--cache-write-price] [--yes] ``` ## Changes - Add `GET` and `POST /api/experimental/ai/model-prices`, gated behind the AI Bridge entitlement and the existing `ai_model_price` RBAC resource. - Add a `GetAIModelPrices` query with optional `provider` and `model` filters applied in SQL. - Validate the whole request before writing anything, so one bad entry cannot leave the table half updated, and report every problem at once. - Reject prices for models the embedded price book already covers, through a new `prices.IsDefaultPriced`. - Add the `coder exp ai-model-prices` command with `list` and `update`. `update` accepts a JSON document or the single-model flags and prints a plan, asking to confirm unless the document is piped in or `--yes` is passed. - Consolidate the supported provider list into `coderd/aibridge/prices/providers` so the price generator and the server share one definition. - Add `codersdk` types and client methods for both endpoints, and bound the request body at 1 MiB. - Document the command in the AI Gateway cost controls page. Closes https://linear.app/codercom/issue/AIGOV-567/experimental-cli-command-to-set-prices-for-unpriced-ai-models > [!NOTE] > Initially generated by Claude Opus 5, modified and reviewed by @ssncferreira |
||
|
Michael Suchacz
|
e92fd8e96f |
chore: retire mark3labs/mcp-go dependency (#28061)
## Stack Context PR 6 of 6 in a stack that migrates every Coder MCP surface from the archived `github.com/mark3labs/mcp-go` library to the official `github.com/modelcontextprotocol/go-sdk` v1.7.0. Stack: #28056 -> #28057 -> #28058 -> #28059 -> #28060 -> #28061 ## Why With every production surface migrated, this PR removes the mark3labs dependency entirely and converts the remaining test fixtures. - Migrates the remaining mark3labs test fixtures (coderd MCP e2e tests, chatd fixtures, mcpclient fixtures, and the Force On MCP policy tests) to official stateless SDK servers. - Removes `github.com/mark3labs/mcp-go` from `go.mod` and drops the corresponding dependabot ignore entry. Zero references remain repo-wide. - Updates the MCP docs for the 2026-07-28 protocol: stateless Streamable HTTP behavior, the supported 2024-11-05 through 2026-07-28 protocol range, and explicit non-features (resources, prompts, structured output, elicitation, MCP Tasks). - The e2e ping assertion is removed because MCP 2026-07-28 removed the ping method. > Mux created this PR on Mike's behalf. |
||
|
Michael Suchacz
|
c8e8b21a88 |
feat: migrate aibridge injected-MCP proxy to official MCP Go SDK (#28060)
## Stack Context PR 5 of 6 in a stack that migrates every Coder MCP surface from the archived `github.com/mark3labs/mcp-go` library to the official `github.com/modelcontextprotocol/go-sdk` v1.7.0. Stack: #28056 -> #28057 -> #28058 -> #28059 -> #28060 -> #28061 ## Why The aibridge injected-MCP proxy now owns an official `*mcp.Client`, `*mcp.StreamableClientTransport`, and `*mcp.ClientSession`. - The proxy constructor accepts an optional `*http.Client` instead of mark3labs options; the header-injecting wrapper shallow-copies a supplied client so its Timeout, Jar, and redirect policy survive. - Manual protocol version negotiation and the mark3labs five-second close workaround are removed; the SDK negotiates during `Connect` and fails when no mutually supported version exists. - Repeated `Init` closes the previous session, and a failed tool fetch closes the just-created session so transports do not leak. - Tool and intercept types use the official pointer content types; embedded resource blobs are re-encoded to base64 for model-facing text because the SDK decodes them into raw bytes. - `aibridge/mcpmock` is regenerated, and its stale `go:generate` source path is corrected. > Mux created this PR on Mike's behalf. |
||
|
Michael Suchacz
|
1e546ea8a3 |
feat(coderd/x/chatd/mcpclient): migrate external MCP client to official Go SDK (#28058)
## Stack Context PR 3 of 6 in a stack that migrates every Coder MCP surface from the archived `github.com/mark3labs/mcp-go` library to the official `github.com/modelcontextprotocol/go-sdk` v1.7.0. Stack: #28056 -> #28057 -> #28058 -> #28059 -> #28060 -> #28061 ## Why The chatd external MCP client (admin-configured MCP servers used by Agent chat) now holds `*mcp.ClientSession` connections created via `mcp.NewClient` and `Client.Connect`, with `StreamableClientTransport` or `SSEClientTransport` per server config. - Auth and identity headers are injected through a custom `http.RoundTripper` because the official SDK has no per-header transport options. - Tool input schemas are extracted from the SDK's `map[string]any` decoding. - Content conversion handles the official pointer content types; the SDK decodes blob resources into raw bytes, so binary content is handled without an extra base64 round trip. - Test fixtures are official stateless Streamable HTTP servers. > Mux created this PR on Mike's behalf. |
||
|
Michael Suchacz
|
08a1525f78 |
feat: migrate coderd MCP server to official MCP Go SDK (#28056)
## Stack Context PR 1 of 6 in a stack that migrates every Coder MCP surface from the archived `github.com/mark3labs/mcp-go` library to the official `github.com/modelcontextprotocol/go-sdk` v1.7.0, adding MCP 2026-07-28 support while keeping compatibility with clients speaking 2024-11-05 through 2025-06-18. Stack: #28056 -> #28057 -> #28058 -> #28059 -> #28060 -> #28061 ## Why The coderd Streamable HTTP MCP server (`/api/experimental/mcp/http`) is the foundation layer: it introduces the official SDK dependency and the shared `RegisterSDKTool` helper the CLI server reuses. - The server runs the SDK handler in stateless mode with `JSONResponse: true`, preserving the previous `application/json` POST wire format. GET and DELETE return 405, and no `Mcp-Session-Id` is issued, both permitted by the Streamable HTTP spec. - `DisableLocalhostProtection` is set because coderd commonly listens on loopback behind a reverse proxy with a public Host header; the endpoint's bearer authentication is the relevant access control. - Tool registration builds raw JSON object schemas and omits empty `required`, keeping `tools/list` output byte-identical to the previous server (verified with a golden comparison). - SDK logs are adapted to `cdr.dev/slog/v3`; only warnings and errors are forwarded because the SDK logs several INFO lines per stateless request. - Tests cover the modern 2026-07-28 flow, legacy 2025-06-18 initialize, unsupported protocol version rejection (`-32022`), and non-POST method behavior. ## Known behavior deltas vs the old endpoint Both deltas come from the SDK enforcing the Streamable HTTP spec where mark3labs was lenient, on an experimental endpoint: - POST requests whose `Accept` header lists `application/json` without `text/event-stream` are now rejected with 400 (the spec requires clients to list both; a missing `Accept` header is still tolerated). mark3labs did not validate `Accept` at all. - The old server generated an unvalidated `Mcp-Session-Id` response header; the stateless SDK handler issues none. Clients that merely echo the header back are unaffected. ## Validation Beyond unit/integration tests, a remote dogfood UAT ran protocol conformance against a live dev server built from the stack tip: version negotiation matrix (2024-11-05 through bogus/omitted values), auth, session/method semantics, tool schema sanity, tools/call happy and error paths (unknown tool, schema-violating args, malformed JSON, jsonrpc "1.0"), and a concurrency smoke test. No 500s or connection drops; error shapes are clean JSON-RPC/HTTP errors. > Mux created this PR on Mike's behalf. |
||
|
Cian JohnstonandCopilot Autofix powered by AI
|
3f9e8cca2a |
chore: add test coverage for chatd compaction (#28053)
## Summary Adds test coverage for the three compaction-decision functions in chatd that had zero tests: `latestPromptUsage`, `shouldCompactPromptUsage`, and `contextTokensFromUsage`. AIGOV-585 hypothesized that chatd's token counting logic was incorrect — that it compared a cumulative sum of prompt tokens across all agentic-loop steps against the context window. The tests disprove this: `latestPromptUsage` returns the last persisted assistant message's usage, not a sum. The actual bug was in the aibridge streaming interceptor, which summed usage across SSE chunks and persisted inflated values (fixed in `ad100452d4`). ## What's tested - `TestLatestPromptUsage` — pins that the compaction path reads the last step's usage (5,400), not a cumulative sum across steps (15,600). If someone wires `TotalUsage` into the compaction path as the issue suggested, this fails. - `TestShouldCompactPromptUsage` — covers the threshold decision with the inflated value from the issue (417,012 → compacts), the correct value (6,000 → doesn't compact), cache token counting, and both disable guards (threshold=100, contextLimit=0). <details> <summary>Plan / investigation notes</summary> - Traced the full flow: `chatloop.go:993` sets `result.usage = part.Usage` from the per-step `StreamPartTypeFinish` event, not the accumulated `TotalUsage` from `agent.go:544`. chatd never calls fantasy's `Agent` interface. - The `TotalUsage` accumulation in `agent.go:544` is only used for cost attribution, not context occupancy. - Commit `ad100452d4` fixed the real bug in `aibridge/intercept/chatcompletions/streaming.go` (cross-chunk usage summation for vLLM-style backends). - Tests reuse existing `dbMessage` and `withUsage` helpers from `message_conversion_test.go` (same package). </details> Generated by [Coder Agents](https://coder.com) --------- Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> |
||
|
Bobby Ho
|
209d1ca498 |
fix: reject PKCE code_verifier below RFC 7636 length floor (#28003)
The token endpoint accepted any non-empty `code_verifier`, so a one-character verifier was enough to authenticate. RFC 7636 §4.1 requires 43 to 128 characters from the unreserved set. That fix plus the related gaps review surfaced in the same path: - Enforce the length and charset floor on the verifier before the S256 comparison runs. - Validate the challenge at the authorize endpoint too. It was only checked for non-emptiness, so a malformed challenge was stored and then failed late at token exchange, blaming the wrong parameter. - A malformed verifier now returns `invalid_request` (RFC 6749 §5.2); a well-formed but wrong one still returns `invalid_grant` (RFC 7636 §4.6). Both looked identical before, so a client had no way to tell a syntax error from a hash mismatch and would retry the same bad verifier forever. - Revoke the authorization code when a PKCE check fails. Without that, a leaked code could be replayed with unlimited verifier guesses for its remaining lifetime, and RFC 6749 §10.5 requires codes to be single use. - Fix verifier generation in `scripts/oauth2/*.sh` and the docs example. They deleted reserved base64 characters instead of translating them to the URL-safe alphabet, so most runs produced verifiers under the new floor. Also carries #28041, which merged into this branch: public clients may register bare custom schemes such as `vscode://` again, with `mailto`, `tel`, and `sms` rejected. Split out of #27873 (public OAuth2 client support). PKCE is already mandatory for every client, so this stands on its own. <details> <summary>Manual verification</summary> Ran against a local dev server on this branch, using a session token and a throwaway app from `scripts/oauth2/setup-test-app.sh`. 1. Happy path unchanged: HTTP 200, verifier length 43. 2. `code_verifier=short`, and a 43-character verifier ending in `!`: both HTTP 400 `invalid_request`, so charset is enforced and not just length. 3. `code_challenge=tooshort` at authorize: HTTP 400 `invalid_request`, no code issued. An empty challenge still hits the older "required and cannot be empty" message. 4. Well-formed but wrong verifier: HTTP 400 `invalid_grant`, distinct from the cases above. 5. Retrying that same code with the correct verifier: HTTP 400, code already revoked by the failed check. 6. `generate-pkce.sh` produces a 43-character verifier (20 out of 20 runs); the docs example produces 128. 7. `scripts/oauth2/test-mcp-oauth2.sh` passes end to end. The two bearer-token failures in its output are a pre-existing script bug (`09c50559f3`, July 2025) that reuses a resource-scoped token against the real API, not a regression here. </details> |
||
|
Michael Suchacz
|
0acd9785fa | fix(coderd/x/agenthooks/dispatch): deflake TestDispatcherTimeoutNoRetry (#28050) | ||
|
Michael Suchacz
|
1458d27d78 |
fix: allow manual chat compaction from the error state (#28022)
A chat that fails generation with a context overflow (for example `Input
length 262625 exceeds the maximum allowed input length of 262112
tokens`) is stuck in a catch-22: `POST /chats/{id}/compact` returns 409
because the `RequestCompaction` transition is only allowed from the
waiting state, and the only other way out of the error state is sending
or editing a message, which re-runs generation with the same oversized
prompt and fails again. Compaction is exactly the recovery a
context-overflowed chat needs, and it is unreachable exactly when it is
needed.
Three semantic changes:
- Allow `RequestCompaction` from the error states: `E0 -> R0` and `E1 ->
R1` (queued messages are preserved and processed after the compaction
turn).
- Clear `last_error` in `Tx.RequestCompaction`, matching the
architecture rule that transitions leaving `E0`/`E1` clear the stored
error. Without this a successful compaction would land in waiting with a
stale persisted error.
- Grant the compaction turn a fresh history epoch: a
`grant_history_epoch` flag on `UpdateChatExecutionState` sets
`history_version = snapshot_version`, resets `generation_attempt`, and
clears `retry_state` in the same atomic update that clears `last_error`
(mirroring the `chat_messages` trigger postcondition). The transition
inserts no history, so without this the turn inherits the failed turn's
spent retry budget, and resetting the counter alone could collide with
message part episode keys still retained on the erroring replica.
No frontend change is required: the chat input is already enabled in the
error state and `/compact` submission already handles both the success
and 409 paths. Also updates ARCHITECTURE.md (transition matrix,
endpoint, and manual compaction sections), the endpoint's swagger
description, and SDK comments.
> Mux created this PR on Mike's behalf.
<!-- mux-attribution: model=claude-sonnet-4-6 thinking=high -->
|
||
|
Danielle Maywood
|
5a33b669b4 | feat: redesign the advisor tool row (#28069) | ||
|
Danielle Maywood
|
c424a76a12 | feat: wire chat search box to full-text search (#27973) | ||
|
Michael Suchacz
|
88e113554a | fix: report per-request Anthropic usage in chat token accounting (#27966) | ||
|
Michael Suchacz
|
bde38e9d10 |
fix(coderd/x/chatd): synchronize aibridgeTestFactory recorded fields (#28031)
Fixes the data race in the chatd test helper `aibridgeTestFactory` reported in CODAGT-917 (`test-go-race-pg` flake in `TestAwaitSubagentCompletion/Timeout`). `TransportFor` recorded `providerName` and `source` with plain field writes. Tests that start the chat worker share one factory between concurrently running chat runners (parent chat and spawned subagent), so two runners resolving models at the same time raced on those writes. The fix guards the recorded fields with a mutex and reads them through a locked `recorded()` accessor at the three asserting call sites. Verified with a red-green repro: concurrent `TransportFor` calls on one factory failed `go test -race` with the exact CI signature (lines 37-38) before the fix and pass after it. Also ran `go test -race ./coderd/x/chatd -run TestAwaitSubagentCompletion -count=10` and the full `go test -race ./coderd/x/chatd` package, both clean. Audited every other `aibridge.TransportFactory` implementation and `aibridgeTestFactory` use site for the same defect: `chattest.MockAIBridgeTransport` is already mutex-guarded, `stubTransportFactory` (coderd/aibridge_test.go) records via a channel, `providerRoutedTransportFactory` (chatd_test.go) is a stateless lookup, and the production factories keep no recorded state. No other occurrence exists. Closes CODAGT-917. > Mux acted on Mike's behalf to create this PR. |
||
|
J. Scott Miller
|
866e676320 |
feat: invalidate provisioner daemon sessions on key deletion (#26532)
## Summary Closes PLAT-305. When a provisioner key is deleted, the associated daemon kept operating on its existing WebSocket connection, because authentication was only checked at connection establishment and deletion was a bare `DELETE` with no session invalidation. This adds four layers of defense so a deleted key promptly stops doing work: 1. **Publish on delete.** `deleteProvisionerKey` publishes to a new per-key pubsub channel (`coderd/pubsub.ProvisionerKeyDeletedChannel`) after a successful delete. Publish errors are logged but still return `204`, since layer 3 is the durable backstop. 2. **Subscribe and tear down.** The daemon serve handler subscribes to its key's channel and terminates the DRPC session on a deletion event. Termination is deferred while a job claimed by the session is active: the daemon may finish and report the in-flight job (`UpdateJob`/`CompleteJob` have no key check), and the last active job's completion performs the cancellation. Because Postgres `LISTEN`/`NOTIFY` does not buffer for non-listeners, the handler also performs a synchronous key-existence re-check immediately after subscribing to close the race between auth and subscription. The subscription uses `SubscribeWithErr` so that an `ErrDroppedMessages` signal (emitted when the pubsub listener reconnects) triggers the same key re-check, closing the listener-outage window in which a deletion notification could be missed. 3. **Backstop on acquire.** `AcquireJob` and `AcquireJobWithCancel` verify the key still exists before waiting for a job, and the `Acquirer` claims jobs in a transaction that first locks the worker's deletable key (`LockProvisionerKeyByIDForShare`, a `FOR KEY SHARE` row lock held until commit) before running the `AcquireProvisionerJob` claim, so a claim cannot commit after the key's deletion. This guards against a missed pubsub message. A missing key row surfaces as its own result rather than overloading the claim query's no-rows response: the acquire terminates with `ErrProvisionerKeyDeleted` (terminating the session, with the same active-job deferral) and hands the consumed wakeup to another waiting daemon in the same domain, rather than silently re-parking and starving peers of job postings. 4. **Heartbeat watchdog.** The per-session heartbeat loop (1m interval) also re-checks the key, so even a session whose deletion notification was silently lost terminates within one heartbeat interval instead of living until the connection breaks (same active-job deferral as layer 2). Reserved keys skip the check. A job that is claimed but never delivered (the session or connection dies between the database claim and the stream send) is marked failed immediately on a fresh context, instead of staying assigned to the worker until the job reaper. Reserved keys (built-in, user-auth, PSK) are exempt throughout, since they are not deletable rows. The acquire-time lookup runs as `dbauthz.AsSystemReadProvisionerDaemons`, because the provisionerd role cannot read provisioner keys and a provisioner key's RBAC object is a provisioner daemon. A single key can back many daemons (and span HA replicas), so the per-key channel fans out to invalidate all of them at once. Per-key channels keep the `LISTEN` count proportional to distinct keys rather than waking every daemon on unrelated deletions. ### Known limitations - **`UpdateJob`/`CompleteJob` intentionally have no key check.** By the time those RPCs arrive the work has already run; rejecting completion would strand a build in "running" (until the job reaper fails it) with real infrastructure left unreconciled. Session termination is deferred while a job is active so the completion can be reported; the daemon may not receive the final RPC response when the deferred termination fires, but the job's outcome is already persisted. - **After termination, the daemon process redials and receives 401s until restarted.** The dial-time exit logic only triggers on 403, and the auth middleware returns 401 for an invalid key; this dial behavior predates this PR and is tracked as a follow-up in [PLAT-452](https://linear.app/codercom/issue/PLAT-452) (return 403 for invalid provisioner keys). ## Tests - `coderd/provisionerdserver`: `TestAcquireJob_ProvisionerKeyDeleted` (both RPC variants), `TestAcquireJob_ReservedProvisionerKey`, `TestHeartbeat_ProvisionerKeyDeleted` (heartbeat watchdog cancels the session after key deletion), `TestAcquirer_ProvisionerKeyDeleted` (a dead-key acquiree exits terminally and its clearance is promoted to a peer in the same domain), and `TestTerminateSession_Deferral` (termination is immediate when idle and deferred until the last active job finishes). - `coderd/database`: `TestAcquireProvisionerJob/ProvisionerKeyLock` covers the lock query against real Postgres: it returns the key ID while the row exists and no rows once it is deleted. The lock-then-claim composition is pinned by `TestAcquirer_ProvisionerKeyDeleted`. - `enterprise/coderd`: `TestProvisionerDaemonServe/KeyDeletionClosesSession` asserts an active session closes after its key is deleted. `KeyDeletedDuringSetupClosesSession` covers the post-subscribe re-check when a key is deleted between auth and subscription, and `DroppedMessageClosesSession` covers the `ErrDroppedMessages` re-check when a deletion is missed during a listener outage. ## Validation - `make` pre-commit (gen/fmt/lint/build) passed via git hooks. - Targeted tests pass; existing acquire tests pass with no regression. - Manual: brought up a dev deployment (coder-in-coder) with a Premium license, created a deletable provisioner key, and started an external daemon with `coder provisionerd start`. Confirmed it authenticated via the key and connected, appearing as `idle` in both `coder provisioner list` (with the key name) and the organization Provisioners UI. - Manual, idle teardown: deleted the key while the daemon was idle. The server logged `provisioner key deleted, terminating session`, the daemon's session closed immediately, and it dropped from `coder provisioner list` (then entered the known 401 redial loop, PLAT-452). - Manual, deferred termination: ran a workspace build (tagged template, `sleep 45` in `local-exec`) pinned to the external daemon and deleted the key mid-build. The server logged `deferring session cancellation until active jobs finish`; the heartbeat watchdog re-checked mid-build and re-deferred rather than force-killing. The build ran to completion (`Apply complete`, workspace `Started`) and only then did `canceling session after job completion` fire. The documented caveat reproduced: the daemon lost the final `CompleteJob` ack, and the build outcome was still persisted correctly. <details> <summary>Implementation plan and design decisions</summary> ### Design - **Per-key vs global channel:** chose per-key (`provisioner_key_deleted:<keyID>`) so daemons do not wake on unrelated deletions. The cost is one `LISTEN` per distinct key per replica on the shared listener connection, which is negligible against Coder's existing channels. - **Missing-key behavior on acquire:** returns an error that tears down the acquire rather than silently returning an empty job. - **Subscribe-startup race:** ordering is `authorize -> UpsertProvisionerDaemon -> Subscribe -> GetProvisionerKeyByID`. The post-subscribe re-check handles a deletion that committed before the `LISTEN` registered (Postgres does not buffer notifications for non-listeners; the in-process buffer only smooths bursts and drops on overflow). - **`NewServer` change:** `KeyID` was added to `provisionerdserver.Options` to avoid a positional signature change across call sites. The in-memory (built-in) daemon leaves it unset and is therefore exempt. ### Files - `coderd/pubsub/provisionerkeydeleted.go` (new) — channel helper. - `enterprise/coderd/provisionerkeys.go` — publish on delete. - `enterprise/coderd/provisionerdaemons.go` — subscribe, re-check, cancel session; pass `KeyID`. - `coderd/provisionerdserver/provisionerdserver.go` — `KeyID` option and acquire-time existence check. </details> --- This pull request was created by Coder Agents on behalf of @jscottmiller. |
||
|
Atif Ali
|
c6e3be5090 | chore(coderd/x/chatd): bump computer-use models to current frontier releases (#28026) | ||
|
Atif Ali
|
d7953bd046 | fix(coderd): use service account wording in account notifications (#27536) | ||
|
Hugo Dutka
|
7f75e625cc |
fix(coderd/x/chatd): deflake TestRunner_StartsRealInterruptTask (#28024)
Closes [ENG-2869](https://linear.app/codercom/issue/ENG-2869/flake-testpostchatmessagesbusyinterrupt). The test used to assert a transient chat state, so I got rid of that assertion. There was also a related race in `interruptChat` where the test pubsub message buffer could be cleared after a runner posted the pubsub message that tests look for. |
||
|
Michael Suchacz
|
57f38b5c24 |
fix: keep chat attachments while a linking chat exists
Fixes https://linear.app/codercom/issue/CODAGT-616/keep-chat-attachments-while-chats-remain-unarchived Chat attachments could disappear even though the chat was still available. This happened when a message was saved without recording which attachments it used, or when cleanup deleted attachments before an archived chat itself was removed. Creating a chat, sending or queuing a message, and editing a message now record both the message and which attachments it uses as one operation. If the chat is already at the 50-attachment limit, the chat change fails without being partially saved. Concurrent attachment writes serialize the 50-file cap per chat. Cleanup locks candidates and checks again for new links before deleting. If a file becomes unavailable after input validation, create, send, and edit return a clear client error and roll back the chat change. An attachment stays available while any chat that uses it still exists. After an archived chat reaches the end of its retention period and is deleted, an old attachment that no remaining chat uses can be cleaned up. The retention guide and unavailable-attachment UI text document this lifecycle. This change cannot restore attachments that were already deleted. The database migration adds two indexes so attachment cleanup stays fast as attachments accumulate. > This PR was authored by Mux (AI) on Mike's behalf. |
||
|
Michael Suchacz
|
2e5353bde7 |
feat: add built-in Browser tab for agent-browser (#27910)
Adds a built-in Browser tab to the Agents page right panel, alongside the built-in Terminal and Desktop tabs, when the chat's bound agent has an app with the well-known slug `agent-browser`. The tab shows only while the app is embeddable and its health is `healthy` (or `disabled`, for templates without a healthcheck), so it appears and disappears live as the daemon comes up or goes down. The iframe stays mounted across tab switches to preserve session state. To avoid duplicates, the generic Add Tab menu and persisted workspace-app tabs now exclude the `agent-browser` app. Detection uses the existing `coder_app` slug and healthcheck signals already present in the workspace data model. The workspace watch handler compares the agent app fields the chat UI consumes, so health transitions propagate without re-render churn on every heartbeat. On the backend, the chat `execute` tool now exports `AGENT_BROWSER_SESSION=<chat id>` on every process it starts. agent-browser resolves its default session from that variable, so browser automation from each chat lands in its own isolated session (named by the chat id in the embedded dashboard) instead of a shared default browser. > Mux created this PR on Mike's behalf. |
||
|
Thomas Kosiewski
|
37b3f11243 | fix(coderd): block SSRF in MCP OAuth2 discovery and client registration (#27989) | ||
|
Thomas Kosiewski
|
91d3027498 | fix(coderd): enforce Force On MCP server policy on the backend (#27990) | ||
|
Michael Suchacz
|
c97f4da3ac |
chore: sync fantasy fork with upstream v0.40.0 and openai-go with v3.50.0 (#27981)
Our fantasy fork had drifted far behind upstream charmbracelet/fantasy (base v0.31.0 vs current v0.40.0). This PR updates the pinned forks after reconciling which fork hacks upstream has fixed and which we still need, and adapts this repo to the new APIs. ## Fork updates - `charm.land/fantasy` -> [coder/fantasy#51](https://github.com/coder/fantasy/pull/51) (merged): `coder_2_33` synced with upstream v0.40.0, pinned at the merge commit `bb10946892ef`. - `github.com/openai/openai-go/v3` -> [coder/openai-go#10](https://github.com/coder/openai-go/pull/10) (merged): `coder/pinned` rebased from v3.16.0 onto upstream v3.50.0 (required by upstream fantasy), pinned at the merge commit `92b5addb22d2`. - `coder/anthropic-sdk-go` pin unchanged; the fantasy fork now tracks the same revision this repo ships. ## Hack reconciliation summary Dropped from our fantasy diff (upstream now has equivalents, often stricter): truncated-stream fail-closed detection, Anthropic EffortXHigh / computer use / thinking effort / thinking display, replay fidelity for signed reasoning and web_search errors, PDF and text documents with sanitized filename titles, refusal finish-reason mapping (upstream also maps Bedrock `content_filtered`/`guardrail_intervened`), gpt-5.5/5.6 Responses routing, the Go 1.25 downgrade, and the openai-go SSE decoder and appendCompact patches. Still fork-only and preserved: OpenAI computer use, OpenAI Responses replay continuity validation, Anthropic pre-4.6 budget-thinking conversion plus explicit thinking disable for effort none, Anthropic RefusalMetadata parsing, Bedrock cross-region inference profile region mirroring, and openai-go deferred body serialization with the WithJSONSet fix. Picked up new upstream features: stream transport retry with in-band SSE error classification, Bedrock expired-credential refresh, per-message cache markers for OpenAI-compatible models, tool panic recovery, extra usage fields in provider metadata, and ClientMetadata on tool results. ## Changes in this repo - `aibridge/intercept/responses`: `ResponseOutputItemUnion.Arguments` became a union type in openai-go v3.50; read function-call arguments via `.OfString` (plus test literal updates). - `coderd/x/chatd/chatdebug`: register the new fantasy `Call.Headers`, `ObjectCall.Headers`, and `ToolResultPart.ClientMetadata` fields in the normalization coverage map (all skipped). - `aibridge/internal/integrationtest`: make the RST test listener drain the request before resetting the connection. The new SDK's write path exposed the previous 1-byte-read race as sporadic `use of closed network connection` failures; the fix holds over 40 consecutive runs. - `go.mod`: rewrite the fork provenance comments to describe the post-sync state. ## Validation - `go build ./...` and `go vet ./...` clean (vet findings identical to base). - Fresh (`-count=1`) runs of `./coderd/x/chatd/...`, `./aibridge/...`, `./coderd/aibridged/...`, `./coderd/database/db2sdk/`: 37 packages pass. - `TestClientAndConnectionError` stress-tested 40x clean. - Both fork PRs have green CI. > Mux acted on Mike's behalf to create this PR. |
||
|
Andrew Aquino
|
6e07e2610f |
feat: add paginated API endpoint for groups (#27603)
backend-only changes from #27271; see that PR for summary of changes + implementation details |
||
|
Steven Masley
|
053b38944d |
fix(coderd): render collected_at as UTC RFC3339 in the agent metadata aggregate (#27991)
Follow-up to #27934; this fix was pushed to the branch after the squash-merge and missed it. `jsonb_build_object` renders timestamptz in the session `TimeZone`, which Coder never pins, and `collected_at` defaults to year 1 until the agent's first report. On a non-UTC Postgres session a registered-but-never-collected item renders with an LMT second-offset (even `BC`, e.g. `0001-12-31T19:03:58-04:56:02 BC`), which Go's RFC3339 parsing rejects - a 500 for the entire list page whenever `include_agent_metadata` is used. - `to_char(... AT TIME ZONE 'UTC', ...)` pins the rendering; never-collected items round-trip as Go's zero time. - The test now runs against a named-zone database (`dbtestutil.WithTimezone("America/Caracas")`) and requests a registered but never-collected key; it reproduces the 500 without the fix. Also contains the failure mode Go-side: an unparsable aggregate now degrades to missing metadata for that workspace (with a warning log) instead of failing the entire page. The SQL fix prevents the known cause; the containment covers any future one. The test still catches regressions because it asserts the metadata values, not just a 200. --- Authored by Coder Agents on behalf of @Emyrk. |
||
|
J. Scott Miller
|
66b065323b |
feat: log rate-limited external auth token validation (#26754)
When `ValidateToken` keeps a token because the external auth validation
endpoint was rate-limited (a `403` with rate-limit headers or a `429`),
it returns `valid=true` without provider confirmation. Previously this
happened silently, so operators couldn't tell a provider-confirmed token
from one kept optimistically during a rate limit.
This adds a `Logger` to `externalauth.Config` and emits a `Warn` (with
`provider_id`, `provider_type`, `status_code`, and `reason`) on those
rate-limit branches. It also adds a
`coderd_oauth2_external_requests_rate_limited_total{name, source,
status_code}` counter, incremented in the instrumented round tripper
whenever a provider returns a rate-limited response. The rate-limit
detection is the shared `xhttp.IsRateLimited` (in `coderd/util/xhttp`),
used by both the tripper and `ValidateToken` so the metric and the
validation decision share one definition; no extra wiring is needed
since `ValidateToken` already routes through the instrumented client
with `source="ValidateToken"`.
One deliberate behavioral change rides along: rate-limit detection now
also recognizes the unprefixed `RateLimit-Remaining` header (GitLab, and
the IETF draft rate-limit headers), so a `403` with
`RateLimit-Remaining: 0` is treated as optimistically valid where it was
previously treated as revoked. All other valid/invalid decisions are
unchanged. `TestValidateToken` asserts the warning's fields on the
rate-limited cases and no warning for revocations, `401`, and confirmed
responses; `promoauth` and `xhttp` tests cover the detector and the new
counter.
<details>
<summary>Manual testing</summary>
The signals fire on the external-auth status check (`GET
/api/v2/external-auth/{id}`), which calls `ValidateToken`. To force a
rate-limited response, point a provider's `validate_url` at a mock that
returns the rate-limit shape:
1. Run a mock returning `429` on one path and `403` +
`X-RateLimit-Remaining: 0` on another.
2. Start `coder server` with `--prometheus-enable` and external auth
providers whose `validate_url` point at those mock paths (e.g.
`CODER_EXTERNAL_AUTH_0_VALIDATE_URL=http://127.0.0.1:5599/429`).
3. Create a stored link, either complete the OAuth flow, or insert a row
into `external_auth_links` with a future `oauth_expiry` (token contents
are irrelevant; the mock rejects regardless).
4. `curl` the status endpoint with a session token, then check:
- coderd logs for the `Warn` (`reason=status_code` for `429`,
`reason=rate_limit_headers` for `403`),
- the metrics endpoint for
`coderd_oauth2_external_requests_rate_limited_total{...,status_code="429"|"403"}`.
Notes: `scripts/testidp -429` only rate-limits `/oauth2/userinfo`, not
the `/external-auth-validate/...` path, so it does not exercise this;
use a mock `validate_url`. The default Prometheus port `2112` may
already be taken on dogfood workspaces, set `CODER_PROMETHEUS_ADDRESS`
to a free port.
</details>
🤖 Generated with the help of Coder Agents on behalf of @jscottmiller.
|
||
|
Bobby HoandClaude Opus 5
|
16c58770f8 |
feat: constrain the OAuth2 client type column (#27931)
Extracted from #27873 so the schema change can be reviewed for migration safety on its own. #27873 will rebase onto this. `client_type` decides whether the token endpoint validates a client secret at all, and the column accepts any text: nullable, no `CHECK`, no enum. No Go path can write a bad value today, and `IsPublic` fails closed on anything unrecognized, so the read side is safe. What the schema still permits is the problem: a future migration writing `'public'` onto a row that holds a secret turns off client authentication for that app with nothing to catch it, no constraint, no log, no audit entry, no test. `000565` adds `CHECK (client_type IN ('confidential', 'public'))` and `NOT NULL`. The `UPDATE` ahead of it should touch zero rows, since migration `000344` added the column with a default of `'confidential'` and backfilled with `COALESCE`; it is there so `SET NOT NULL` cannot fail on an unexpected row. Both `ALTER`s take `ACCESS EXCLUSIVE` and scan a table holding one row per registered OAuth2 client, so the lock is brief. ## The second migration, and why it aligns the way it does Two columns describe the same fact and can currently contradict each other. `token_endpoint_auth_method` is the client's own declaration: registered client metadata under RFC 7591 §2, where `"none"` is defined to mean the client is public and has no secret. `client_type` is Coder's derived copy, and it is what the token endpoint enforces on. RFC 7591 defines no `client_type` metadata field; the column exists only as a denormalization. Registration used to persist the declaration verbatim while hardcoding `client_type` to `'confidential'`, so rows exist declaring `"none"` on a client stored confidential that was issued, and still requires, a real secret. A client that reads its own metadata and believes it is public will drop that secret and stop being able to exchange codes. `000566` aligns the declaration to what is enforced, not the reverse. Deriving enforcement from the declaration would reclassify every such client as public and stop requiring the secret it holds, which is a silent authentication downgrade. The down migration is deliberately empty: the previous values are not recorded, and restoring them would only reinstate metadata that tells a client to authenticate in a way the server rejects. ## Application changes `SET NOT NULL` changes the generated field from `sql.NullString` to `string`, so the three write sites are updated to match. That is the entire application diff and no behavior depends on it. Refs https://linear.app/codercom/issue/ENG-3029/oauth2-support-public-client --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Susana Ferreira
|
5efa7abe7d |
fix: only write AI model prices that changed (#27923)
Previously, the AI Gateway price seeder rewrote every row of `ai_model_prices` on each server start, because `ON CONFLICT` fires on a key conflict rather than on a value difference. `updated_at` therefore recorded when the server last restarted rather than when a price last changed. Guard the `DO UPDATE` branch so a conflicting row is only rewritten when one of its four prices differs. The comparison uses `IS DISTINCT FROM` rather than `<>` because the price columns are nullable, and `<>` yields NULL when either side is NULL, which would skip the update and leave a stale price in place. Related to https://linear.app/codercom/issue/AIGOV-567/experimental-cli-command-to-set-prices-for-unpriced-ai-models > [!NOTE] > Initially generated by Claude Opus 5, modified and reviewed by @ssncferreira |
||
|
Steven Masley
|
9a57dfa642 |
feat: include agent metadata in workspace list responses (#27934)
Closes #27933. Related: #27897 (single-agent GET). Agent metadata is only readable via a per-agent watch stream, so reading it across N workspaces costs N+1 requests. This adds a batch read to the list endpoint: ```text GET /api/v2/workspaces?q=param:"pool=demo" include_agent_metadata:task_status ``` - New `include_agent_metadata` search key, repeatable and key-scoped. It expands the response, it does not filter workspaces. - `GetWorkspaces` aggregates the requested keys as JSON behind a `CASE`: without opt-in the response is unchanged and the subquery never runs. Runs only for the returned page, inside the same authorized query. - Agents in the response gain `metadata` (`[]codersdk.WorkspaceAgentMetadata`, `omitempty`), mapped by the `workspace_agent_id` each element carries. The collection script is omitted; it can be long. - `codersdk.WorkspaceFilter` gains `IncludeAgentMetadata []string`. - No wildcard, no schema change, no migration. --- Authored by Coder Agents on behalf of @Emyrk. |
||
|
Jakub Domeracki
|
8c2f7adeb1 |
revert: "fix: markdown rendering improvements" (#27979)
This reverts commit
|
||
|
Michael Suchacz
|
95e8b71d03 |
fix(coderd): start test AI bridge after config in automatic title tests (#27971)
Fixes the reopened flake tracked in CODAGT-876 / coder/internal#1629. ## Problem The Aug 10 recurrence was not the previously fixed subtests failing again: the nightly-gauntlet macos run ([job log](https://github.com/coder/coder/actions/runs/31355808516/job/93355150162)) flaked in the sibling test `TestPostChats_AutomaticTitleGenerationPasteOnly`, which the flake investigator matched to the existing issue. Both `TestPostChats_AutomaticTitleGeneration*` tests still used `newChatClientWithAPI`, which starts the in-process AI Gateway daemon before the test creates its provider/model config. The daemon's synchronous initial provider load therefore sees zero providers, and route availability depends on the asynchronous pubsub-driven reload racing the one-shot automatic title generation that `CreateChat` kicks off. When the reload loses (initial load at `.575` with `provider_count=0`, title request at `.907` hitting `route not supported`, reload landing at `1.029`), the title candidate fails without retry and the test times out waiting for the `propose_title` request. ## Fix Convert both tests to the pattern #27564 established for the `TestRegenerateChatTitle`/`TestProposeChatTitle` subtests: `newChatClientWithoutAIBridge`, create the model config, then `aibridgedtest.StartTestAIBridgeDaemon`, so the daemon's synchronous initial load already contains the route. The existing `NoPubsubDelivery` subtest guards that initial-load invariant, so no new guard test is added. ## Validation - Deterministic red-green via the isolated-pubsub technique: old ordering with the pubsub leg removed reproduces the exact CI signature (`provider_count=0`, `route not supported`, timeout at the `titleRequested` wait); the new ordering passes with the pubsub leg still removed, proving the synchronous initial load alone provides the route. - `go test ./coderd -run 'TestPostChats_AutomaticTitleGeneration' -count=10` - `go test ./coderd -run 'TestRegenerateChatTitle|TestProposeChatTitle' -count=1` - `make lint` and `make pre-commit` via hooks. > Opened by Mux on Mike's behalf. |
||
|
Jakub Domeracki
|
07f79af65b |
fix: markdown rendering improvements
Improvements to markdown rendering in notification emails: - More consistent escaping of values interpolated into notification templates - Stricter link handling in the notification email renderer, scoped to the notification rendering path - HTML escaping of values interpolated into the outer email template - Expanded unit and end-to-end coverage of the notification rendering pipeline - `make gen` run to regenerate golden files for SMTP and webhook notification templates |
||
|
Cian Johnston
|
4e2620d64f |
fix(coderd/x/chatd): classify bedrock credential errors as non-retryable (#27913)
When a Bedrock provider is misconfigured without authentication methods, AWS credential resolution fails and AIBridge writes the error as a plain-text HTTP 500. The fantasy adapter captures the body text in `ProviderError.ResponseBody`, but `Error()` returns only the SDK transport wrapper, not the body. Signal patterns in `chaterror.Classify` checked only `err.Error()` (the wrapper), missing the useful text in `structured.detail` (the body). This caused permanent configuration errors to fall through to the generic 500 rule with `retryable=true`, making the chat worker retry up to 25 times. Introduce `combinedText` (merging the wrapper with `structured.detail`) and widen signal checks that have no dedicated status code to use it: overloaded, auth, config, usage limit, and timeout patterns. The deadline signal stays on `err.Error()` to avoid treating ambiguous body text as a local context deadline. Add a "resolve aws credentials" config pattern so credential resolution failures classify as config, not generic. > Generated by Coder Agents |
||
|
Jeremy Ruppel
|
d9d6ce9ddf |
perf(coderd/rbac): build span role attributes only when recording (#27310)
`rbacTraceAttributes` materialized the subject's role names (one string allocation per role) and was passed into every `Filter`, `Authorize`, and `Prepare` span at creation time, so the O(roles) work ran even when no tracer was recording. It also called `SafeRoleNames()` twice. Replace it with `setRBACAttributes`, which attaches the same attributes *after* the span is created and only when `span.IsRecording()` is true, reading `SafeRoleNames()` once. Recorded spans are unchanged; untraced and unsampled calls skip the per-role work. This originated from #27309: once `/authcheck` checks are batched through `rbac.Filter`, each below-threshold group paid the role-attribute build for the `Filter` span *and* for every per-object `Authorize` span, so the redundant per-call work showed up as extra allocations per request. ## Benchmarks `AMD EPYC 9575F`, `benchstat`, no tracer configured (exercises the `IsRecording()==false` path). **`BenchmarkRBACManyOrgs`** (general RBAC eval), before vs after: wall time flat (geomean −0.04%), allocations strictly lower everywhere (geomean B/op −0.52%; `Authorize` −1.0 to −1.3% B/op), no regressions. **Authcheck path** (`BenchmarkAuthcheckGrouping`, #27309 vs #27310, back-to-back): this change is an **allocation reduction and is time-neutral**. On the endpoint (`Grouped`) path, per-request allocations drop ~4-5% B/op at common org counts (1-10); on the pure per-object path the reduction grows with org count (B/op −2.6% → −7.3% at 100 orgs). Wall time is flat within noise: low-org deltas sit inside this host's ±10-23% run-to-run variance, so no wall-time claim is made. Net: same speed, less garbage per request, which also lowers GC pressure under real concurrent load. <details> <summary>Decision log</summary> - The `Filter` span wraps the whole filtering routine (total latency + `num_objects`); it is the valuable span and is kept. The costly part was `rbacTraceAttributes`, not the span itself. - `rbacTraceAttributes` was O(roles): it allocated a string per role for the `subject_roles` attribute and called `SafeRoleNames()` twice. On `Filter`'s below-threshold fallback it ran once for the `Filter` span and again for each per-object `Authorize` span, so a group of N objects paid N+1 builds vs the old loop's N. Benchmarks confirm this as real per-call allocation; its wall-time cost is below the authcheck benchmark's noise floor. - Deferring attribute construction behind `IsRecording()` requires the span object, so the three callsites moved from `StartSpan(ctx, rbacTraceAttributes(...))` to `StartSpan(ctx)` then `setRBACAttributes(span, ...)`. No spans were removed or renamed; recorded output is identical. - Tradeoff: when a span is not recording, `subject_roles`/`num_subject_roles`/etc. are not computed. Unsampled spans emit nothing anyway, so there is no observable output change. </details> --- Authored with Coder Agents. |
||
|
Jeremy RuppelandSteven Masley
|
51a9aa1bfc |
perf(coderd): batch authcheck permissions via rbac.Filter (#27309)
`POST /api/v2/authcheck` evaluated every check with a full policy evaluation in a serial loop. A subject in many organizations (100+) produced hundreds of full evaluations, taking seconds on a cold cache (DEVEX-608). Group the checks by `(action, resource type)` and authorize each group with the existing `rbac.Filter`, which amortizes a single partial evaluation across the group once it is large enough. Each check is wrapped in a small value struct that carries its response key, so `Filter`'s returned subset maps back to keys by reading a field rather than relying on element identity. `Filter` now takes an explicit `prepareThreshold`; existing callers pass the new `rbac.DefaultFilterThreshold` (10), and `checkAuthorization` passes 50, above the ~35-group crossover measured for this workload, so subjects with few objects of a given type keep the per-object path and cannot regress. ## Stacking This is stacked on top of #27244. `Filter` runs `Prepare` (partial evaluation), and those residuals are only compact once #27244's set-membership residuals land. On plain `main` the existing O(N) residual fanout means batching can regress at high org counts, so this change should land with or after #27244. <details> <summary>Decision log</summary> ### Bottleneck - `site/src/modules/permissions/organizations.ts` defines ~14 permission checks per org; `organizationsPermissions()` flattens them across all orgs into one `POST /api/v2/authcheck`. A 100-org request is ~1400 checks. - `checkAuthorization` looped serially, calling `Authorizer.Authorize` (full eval) once per check. - The endpoint's `maxFetch = 10` only caps checks that carry a `resource_id`, not total checks, so it does not bound this workload. ### Approach - Group checks by `(action, resource type)` and run each group through `rbac.Filter`, which does one partial evaluation (`Prepare`) and reuses it across the group. - Carry the response key as data in a small value struct implementing `RBACObject()`, so allowed results map back to keys without pointer identity: ```go type authorizeCheck struct { key string object rbac.Object } func (c authorizeCheck) RBACObject() rbac.Object { return c.object } ``` - `Filter` takes a required `prepareThreshold int` (no functional options). Generic callers pass `rbac.DefaultFilterThreshold = 10`; `/authcheck` passes 50 because the measured crossover for this workload is ~35 groups. ### Alternatives rejected - **Bounded `errgroup` parallelism**: reduced wall time at high org counts but not aggregate work (allocations flat). Discarded in favor of reducing work via partial evaluation. - **Symmetric-deny Rego simplification** (on the #27244 branch): replacing the known-org deny-fold with symmetric `org := -1` / `scope_org := -1` rules failed existing SQL-compile tests. A `-1` known-org vote gated by `not org = -1` produces a negated membership test over the unknown org id, which OPA emits as an unconvertible support rule. #27244's fold (`member_allow - org_deny`, a positive set-difference membership test) is therefore load-bearing, not incidental. </details> --- Authored with Coder Agents. --------- Co-authored-by: Steven Masley <Emyrk@users.noreply.github.com> |
||
|
Jeremy Ruppel
|
7e708b24ce |
perf(coderd/rbac): collapse org authorization to a set-membership test (#27244)
## Problem Authorization for users who belong to many organizations is slow. On the list <br>endpoints (`/api/v2/organizations`, `/users`, `/groups`) a user in hundreds of <br>orgs saw multi-second page loads <br>([DEVEX-608](https://linear.app/codercom/issue/DEVEX-608/performance-degrades-for-users-in-many-organizations-across-multiple) <br>/ coder/coder#21890 / Pylon [#2758](<https://github.com/coder/coder/issues/2758>)). This is partial-evaluation bound: `rbac.Prepare` <br>scales with the number of org-scoped roles the subject carries. ## Root cause The known-org path in `check_org_permissions` indexed an N-entry vote map by the <br>object's org id: ```rego vote := allow_map[input.object.org_owner] ``` `input.object.org_owner` is unknown during partial evaluation. Indexing a map by <br>an unknown key cannot reduce to a single expression, so OPA emits one residual <br>query per org membership, and `newPartialAuthorizer` then calls `PrepareForEval` <br>once per residual, making `Prepare` O(N) in org count. The list endpoints <br>intentionally use partial eval; the fan-out is in partial eval itself. ## Change Test the object's org id for membership in a set that is fully known at <br>partial-evaluation time, so the query collapses to a single <br>`organization_id = ANY(ARRAY[...])` residual instead of N residuals: * The known-org clause only ever votes to allow, tested via <br>`org_owner in org_ids_with_vote(role_org_votes, 1)`. * Org-level denies are folded into the org-member level as a ground set <br>difference (`member_allow - org_deny`), so the unknown org id appears in only <br>one positive membership test and the decision never branches on it. * The per-org vote maps are computed once as memoized zero-arg rules <br>(`role_org_votes`, `role_member_votes`, `scope_org_votes`, <br>`scope_member_votes`) instead of through parametrized functions that OPA <br>re-evaluates at every call site. * `role_allow`/`scope_allow`, the `any_org` path, and full evaluation are <br>unchanged in behavior. Semantics are unchanged (see the equivalence argument below). The only <br>representational change is that a denied known org's intermediate `org` vote is <br>now `0` instead of `-1`, compensated by the set difference and not observable in <br>the final `allow` decision. ## Results Measured with `BenchmarkRBACManyOrgs` (added on `main` in coder/coder#27270). Full tables: <br>[B/op and allocs/op](<https://github.com/coder/coder/pull/27244#issuecomment-4984523720>). * Residual queries: O(N) -> O(1). * `Prepare` / `PrepareAndCompile` memory changes from < />quadratic growth on `main` <br>(176 MiB, 7.08M allocs per op at 100 orgs) to near-linear (6.5 MiB, 258k <br>allocs), a < />96% reduction at 100 orgs, with similar wins in time. * Memoizing the vote maps removed an early single-org regression: at 1 org <br>`Prepare` now allocates < />7% fewer bytes and < />9% fewer objects than `main`. * `Authorize` (full evaluation) memory is marginally higher (+1-8%, largest at <br>1 org) and time-neutral. This is the inherent cost of the set-membership form <br>that keeps partial evaluation from fanning out; full evaluation builds an <br>allow set it would not otherwise need. * `go test ./coderd/rbac/...` passes, including `TestAuthorizeDomain` (full- vs <br>partial-eval equivalence) and the regosql suite. A second, independent bottleneck remains (out of scope here): the vote map is <br>still built in O(N^2) in `check_all_org_permissions` <br>(`roles[_].by_org_id[org_id]` scans all roles per org). Fixing it means <br>pre-merging roles' `by_org_id` into one org->perms map in the OPA input, and is <br>tracked as a follow-up. ## Testing * `OrgDenyBlocksMember` (`TestAuthorizeLevels`): an org-level deny blocks a <br>member-allowed action on an owned in-org object, while a clean org is allowed, <br>including an action-scoped deny. * `ScopeOrgDenyBlocksMember` (`TestAuthorizeScope`): the same fold at the scope <br>level. * The shared harness covers full and partial evaluation and asserts the partial <br>result compiles to SQL with zero support rules. <details><summary>Decision log and equivalence argument</summary> ### Why not deny-via-set-membership The first attempt expressed deny as a second set-membership clause (`:= -1 if org_owner in deny_set`). That makes `org`/`scope_org` multi-valued, and the `not org = -1` checks in `role_allow`/`scope_allow` then cause OPA to emit a `data.partial.__not__` support rule that regosql cannot compile (`TestAuthorizeDomain/UserACLList` failed). It failed even when the deny set was empty, purely because the `-1` clause exists. ### Why not deny-via-enumeration A follow-up enumerated only the (usually empty) deny set. It compiled and passed, but it branches on the unknown org id (one ground residual per denied org), which violates the "do not branch on the unknown" rule in `coderd/rbac/POLICY.md`. ### Final approach: allow-only + ground set difference The known-org clause votes only to allow, and the org-level deny gate is moved into the org-member level as `member_allow - org_deny`, a set difference over fully-known sets. The unknown org id is used only in positive `in` tests, so there is no enumeration, no negated membership, and no branching on the unknown. ### Empty-set residual pruning A naive set-membership left unsatisfiable residuals (`org_owner in set()`) for levels with no matching permissions (e.g. the org level for an org-member role, or scope-org for `ScopeAll`), each still costing a `PrepareForEval`. Guarding each membership with a ground `count(...) > 0` lets OPA drop those branches, flattening the residual count across org sizes. ### Memoized vote maps Profiling the single-org path showed the cost was repeated function evaluation: the parametrized helpers rebuilt the same vote map for the org, member, and scope paths on every check. Hoisting the maps into memoized zero-arg complete rules (which OPA evaluates once per query) removed that overhead and eliminated the single-org `Prepare` regression, while composition keeps the policy readable. ### Equivalence (known-org path, `site != -1`) * A: original `org == 1` <=> `org_owner in org_allow` (unchanged). * B: original `org != -1 and member == 1` <=> `org_owner not in org_deny and org_owner in member_allow` <=> `org_owner in (member_allow - org_deny)` = new `org_member == 1`. The critical case (`org` denies, member allows): old blocks it via `not org = -1`; new blocks it because `org_owner` is removed from `member_allow - org_deny`. Same outcome. Deny-wins aggregation is intact because `check_all_org_permissions` still nets an org to `-1` via `to_vote`, landing it in `org_deny`. </details> --- This PR was generated by Coder Agents on behalf of @jeremyruppel. |