mirror of
https://github.com/coder/coder.git
synced 2026-09-21 20:51:01 +08:00
2e5353bde753be744e070e80a6704bab4602349c
4341
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 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. |
||
|
|
4b7494be72 |
feat: harden chat generation runtime instrumentation for billing (#27451)
Closes CODAGT-835 ## Summary `chat_messages.runtime_ms` becomes the billing source of truth for Coder Agents runtime (summed hourly by #27312), but it was built for debugging: the June refactor (#26270) silently stopped recording tool-step runtime, compaction was never measured, and interrupted turns lost their partial runtime entirely. This PR defines the billable metric, closes the paths that dropped it, and documents the definition where the data lives. ## The billable definition **`runtime_ms` is the wall-clock duration of the model invocation that produced the persisted message content**, measured from just before the provider stream opens until it is fully consumed. What counts: - Assistant generation steps, in top-level and sub-agent chats (sub-agents are ordinary chats on the same generation path). - Compaction summarization calls, persisted on the compaction assistant message (**new**). - Interrupted attempts: the message-part episode's lifetime is persisted on the partial assistant message committed by `FinishInterruption`, so partial generation time survives interruption (**new**; measured via a new `Buffer.EpisodeDuration`, which works even though the generation goroutine and the interrupt task are different tasks). What deliberately does not count (each is documented in code and docs): - **Local tool execution.** Tool wall time includes idle waits, most importantly `wait_agent` polling a sub-agent chat that already bills its own model invocations; billing the batch would double count, and excluding one tool from a concurrent batch's wall time is ill-defined. Pre-refactor instrumentation did include tool time; this makes the exclusion an explicit product definition instead of a silent regression. - **Failed model calls whose output is discarded** (retried attempts, terminal errors, content-filter refusals). They persist no content, so they bill nothing; billing errs toward undercounting. Notably a stream-silence timeout can burn 10 idle minutes before a retry, which should not be billable "active generation". If product later wants failed attempts billed, that needs a place to persist runtime on error turns (`FinishError` inserts no rows today) and is a deliberate follow-up, not instrumentation drift. - **Ancillary calls that produce no chat messages** (title generation, advisor, turn summaries) and all idle/parked time (`requires_action`, queueing). The definition is documented as `COMMENT ON COLUMN chat_messages.runtime_ms` (migration 000551, surfacing as a Go doc comment on `ChatMessage.RuntimeMs`), on `chatloop.PersistedStep.Runtime`, in the chatd architecture doc, and in the Spend Management docs page. ## Index for the hourly scan None needed: `GetTotalChatMessageRuntimeMsInRange` (#27312) filters an hour-wide `created_at` range, which the existing `idx_chat_messages_created_at` b-tree already serves; the residual `runtime_ms IS NOT NULL` filter applies to one hour of rows. A partial index would add permanent write amplification for a query that runs once an hour. > [!NOTE] > Migration 000551 is also claimed by #27312; whichever merges second renumbers via `fix_migration_numbers.sh`. ## Tests - End-to-end: the existing full-server generation test now asserts `RuntimeMs.Valid` on the committed assistant row (it previously read `.Int64` without checking `.Valid`, so it passed on NULL). - Interrupted turn: full task-level test (real DB, mock clock) asserting the partial assistant message persists the attempt's runtime. - Errored stream: asserts a failed invocation yields no step and no runtime. - Tool-using turn: asserts runtime lands on the assistant row only and tool rows stay NULL. - Compaction: asserts the summarization call duration is recorded and lands on the compaction assistant message only. - `messagepartbuffer.EpisodeDuration` unit coverage. Blocks: CODAGT-843 (B3), CODAGT-838 (D8). --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: Hugo Dutka <hugo@coder.com> |
||
|
Ethan
|
d2f9280138 |
chore: remove legacy chat template allowlist (#27515)
Relates to CODAGT-713 Depends on #27514 Removes the legacy deployment-wide allowlist now that the API and frontend use per-template `agents_allowed`: the experimental `/template-allowlist` routes, SDK methods and generated types, site config queries, frontend bindings, and the now-unused `xjson` utility. Migration `000563` deletes the obsolete `agents_template_allowlist` value. It's irreversible for deployments that configured an allowlist, which I think is fine, since `000562` already drops `agents_allowed` on the way down, and this release ships `000548` and `000555` with the same property. Two side effects of the model change worth writing down, both from #27514 rather than here. The value used to need `ActionRead` on `ResourceDeploymentConfig` to read and deployment config update to write. `AgentsAllowed` is now a plain field on the template response, readable by anyone who can read the template, and it's set with a template update, so org admins manage it themselves. That's the delegation we wanted, and it's tracked in the audit log. The rest of the stack adds `--agents-allowed` to the CLI and updates the platform controls docs. |
||
|
Ethan
|
0ac23e3ee1 |
feat: add per-template Coder Agents access control (#27285)
Relates to CODAGT-713 Depends on #27284 This makes the per-template `agents_allowed` field authoritative in the API and chatd. It adds optional create and metadata update fields with the intended default and omission semantics, supports `agents-allowed:` template search, includes the value in telemetry, and makes `list_templates`, `read_template`, and `create_workspace` read the template row directly. Existing-workspace retries remain idempotent, and blocked same-organisation templates return an actionable message. The experimental `/template-allowlist` routes remain temporarily because the shipped AI Settings page still calls them, but they no longer control chatd enforcement. #27514 moves that page to per-template metadata, #27515 removes the legacy storage, routes, SDK types, and utility, #27517 adds the CLI flags, and #27518 updates the platform controls documentation for the per-template model, directly addressing CRF-5 and CRF-6. The stack is intended to merge as a unit. |
||
|
Ethan
|
b3485d9b3a |
chore: add agents_allowed to templates (#27284)
Relates to CODAGT-713 This adds `templates.agents_allowed` as a default-true, auditable template attribute, along with nullable database filtering. Migration `000562` translates the effective legacy `agents_template_allowlist` state for existing templates: a valid nonempty list allows matching templates and blocks the rest, missing or empty values leave templates allowed, whilst corrupt values fail closed by blocking all existing templates. As per the linear issue, new templates deliberately default to allowed under the per-template model. This is the database-only first PR in the stack. #27285 makes the field authoritative in the API and chatd whilst temporarily retaining the compatibility routes needed by the shipped frontend. Later PRs migrate the UI, remove the legacy storage, routes, SDK types, and utility, then add CLI flags. |
||
|
david-fraley
|
0d0c6e53ba | fix(coderd): document HTTP 201 for workspace and build creation (#27903) | ||
|
Michael Suchacz
|
4b9880afa6 |
feat: add --chat-hook-allow-insecure to allow plain HTTP chat hook URLs (#27896)
Adds a hidden `--chat-hook-allow-insecure` / `CODER_CHAT_HOOK_ALLOW_INSECURE` deployment option (default `false`) that allows the chat lifecycle hook URL to use plain HTTP for any host. The HTTPS requirement is enforced at two points, and the flag relaxes both: `DeploymentValues.Validate()` rejects `http` hook URLs at startup, and the hook dispatcher's `validateHookURL` allows `http` only for loopback hosts. With the flag set, any-host `http` is accepted; the host, fragment/userinfo, secret, and timeout checks are unchanged, and non-http(s) schemes still fail. This removes the need for an HTTPS reverse proxy when testing a hook consumer on a trusted network. Following security review feedback, the flag description and docs state that plain HTTP lets an on-path attacker forge hook responses (which control agent execution), and `coder server` logs a startup warning (with a redacted hook URL) when hooks run over plain HTTP. Docs, generated API types, and the server config golden are updated accordingly. > Mux acted on Mike's behalf to create this PR. |
||
|
Bobby Ho
|
97c4031526 |
feat!: resolve agent external auth by template, not config order (#27854)
## TL;DR
**Problem.** A template can declare which external auth provider it
wants via `data "coder_external_auth" { id = "..." }`, and that
declaration is honored at every stage of the build. It was ignored at
runtime. Any git operation going through `GIT_ASKPASS` supplies only a
hostname, never a provider ID, and the handler scanned *every* provider
configured on the deployment and returned whichever matched the hostname
**last in config order**, with no reference to what the requesting
workspace's own template declared. Reordering
`CODER_EXTERNAL_AUTH_<N>_*` silently redirected a plain `git clone` from
one OAuth client's token to a completely different one.
**Fix.** For hostname-only requests, resolve the calling agent's
workspace and build *before* selecting a provider, then narrow
candidates to the providers declared by that build's template version.
Exactly one match wins regardless of config order. No matching declared
provider falls back to today's deployment-wide scan, so a template that
declares only a GitHub provider can still clone an unrelated host. Two
or more matching declared providers return `409` naming them, rather
than picking one arbitrarily: `external_auth_providers` is stored sorted
by ID, so HCL declaration order is already unavailable and no principled
tie-break exists.
Requests supplying an explicit provider ID are untouched. Server-side
only: no wire protocol, proto, manifest, or database schema change, so
already-running agents get the corrected behavior on their next askpass
call with no restart.
Refs #23718
<details>
<summary><b>Call flow</b></summary>
```mermaid
flowchart TD
subgraph Push["1. Template import: coder templates push"]
A1["Terraform extracts coder_external_auth id/optional attrs"]
A2["CompleteJob(TemplateImport) validates each id<br/>against deployment config"]
A4["template_versions.external_auth_providers persisted"]
A1 --> A2 --> A4
end
subgraph PreBuild["2. Pre-build and workspace build (unaffected)"]
B1["User authenticates declared provider(s), exact-ID lookup"]
B2["Build resolves token by exact ID<br/>(provisionerdserver.go)"]
A4 --> B1 --> B2
end
subgraph Runtime["3. Workspace running: a credential is needed"]
B2 --> C0{"Caller supplies id or match?"}
C0 -->|"id (explicit)"| D1["Exact-ID match<br/>UNCHANGED, already deterministic<br/>(coder external-auth access-token)"]
C0 -->|"match only (GIT_ASKPASS)"| C1["git needs credentials for a hostname<br/>GIT_ASKPASS invoked, unchanged"]
C1 --> C2["coder gitaskpass sends ExternalAuthRequest{Match: host}<br/>unchanged (cli/gitaskpass.go)"]
C2 --> C3["workspaceAgentsExternalAuth<br/>(coderd/workspaceagents.go)"]
C3 --> C4["CHANGED:<br/>1. resolve workspace/build BEFORE matching<br/>2. read that build's declared provider IDs<br/>3. filter: declared AND regex matches host"]
C4 --> C5{"how many candidates?"}
C5 -->|"exactly 1"| C6["use it, regardless of config order"]
C5 -->|"0"| C7["fall back to deployment-wide scan<br/>(unchanged legacy behavior)"]
C5 -->|"2 or more"| C8["409 naming every matching ID"]
end
D1 --> E1["Token returned"]
C6 --> E1
C7 --> E1
style C4 fill:#1f4d2e,stroke:#4caf50,color:#fff
style C6 fill:#1f4d2e,stroke:#4caf50,color:#fff
style C8 fill:#1f4d2e,stroke:#4caf50,color:#fff
style D1 fill:#333,stroke:#888,color:#fff
```
</details>
## Verification
Two test functions were added in `coderd/workspaceagents_test.go`, and
the behavior no unit test can reach was verified against a local dev
cluster with two real GitHub OAuth Apps whose regexes both match
`github.com`.
| Behavior | Unit | Manual |
|---|---|---|
| Declared provider wins over a colliding one | yes | yes |
| Outcome independent of deployment config order | yes | yes |
| No declared match falls back to the full scan | yes | yes |
| Host the template never declared still resolves | yes | via fallback |
| Two declared providers matching one host return `409` | yes | not run
|
| Declared but unauthenticated provider returns its auth URL | yes | not
run |
| Two templates resolve independently and concurrently | yes | no |
| Explicit-ID path unaffected | no | yes |
| Running agent corrected with no restart | **no** | **yes** |
| Declared ID since removed from config falls back | **no** | **yes** |
| Recomputed per build after a template update | **no** | **yes** |
The last three are properties a unit test cannot express: they involve
swapping the server binary underneath a live agent, removing deployment
configuration, and rebuilding a workspace against a new template
version.
<details>
<summary><b>Unit test detail</b></summary>
`TestWorkspaceAgentsExternalAuthTemplateScoped` builds a deployment with
two providers sharing a regex, a template declaring one of them, and a
seeded token for **every** provider, so a mis-selection returns a valid
token with the wrong identity rather than an error. Subtests:
- `DeclaredProviderLast` / `DeclaredProviderFirst`: the declared
provider wins in both config orders. Only the `First` arm is
discriminating, since the pre-change loop had no `break` and returned
the last regex match, which the `Last` arm happens to agree with.
- `NoDeclaredProvidersFallsBackToFullScan`: a template declaring nothing
keeps today's behavior exactly, pinning the legacy last-match rule.
- `UnrelatedHostStillResolvesViaFallback`: a template declaring only a
GitHub provider still resolves a GitLab host.
- `AmbiguousDeclaredSetReturnsError`: `409` whose message names both
colliding provider IDs.
- `OptionalUnauthenticatedDeclaredProviderReturnsAuthURL`: returns the
auth URL for the *declared* provider, not for an unrelated one the user
happens to hold a token for.
`TestWorkspaceAgentsExternalAuthMultipleTemplates` runs two workspaces
from two templates, each declaring a different provider, issuing
requests concurrently. Each resolves to its own template's provider.
</details>
<details>
<summary><b>Manual verification detail</b></summary>
Local dev cluster, two GitHub OAuth Apps both defaulting to
`^(https?://)?github\.com(/.*)?$`, both authorized by the workspace
owner so a wrong selection yields a usable token rather than an error.
Workspace built from a template declaring only `github-dotfiles`. Tokens
redacted.
**Order independence.** Same workspace, never rebuilt, config order
reversed between runs:
| Deployment config order | Token returned |
|---|---|
| `[github-broad, github-dotfiles]` | `gho_<dotfiles>` |
| `[github-dotfiles, github-broad]` | `gho_<dotfiles>` |
**A/B against the pre-fix binary.** Everything held constant except the
coderd build, with `/api/v2/buildinfo` checked on both sides so the
comparison rests on verified binary identity. The workspace was never
stopped, rebuilt, or re-authorized:
| coderd | buildinfo | Token | Honors declaration |
|---|---|---|---|
| pre-fix | `v2.35.3-devel+11e03cfb3a` | `gho_<broad>` | no |
| this branch | `v2.35.3-devel+e8b87d0333` | `gho_<dotfiles>` | yes |
This doubles as the demonstration that a coderd-only upgrade corrects
behavior on a live agent's next askpass call.
**Declared provider removed from config.** `github-dotfiles` deleted
from deployment configuration while the workspace's template still
declared it. Result: `HTTP/2 200` with `gho_<broad>` via the fallback.
No `500`, no fail-closed `404`. The orphaned `external_auth_link` row
remained in the database throughout and correctly had no effect.
**Recomputation after a template update.**
| Workspace state | Build's declared provider | Token returned |
|---|---|---|
| new version pushed, workspace not updated | `github-dotfiles` |
`gho_<dotfiles>` |
| after `coder update` | `github-broad` | `gho_<broad>` |
The pair is what makes it conclusive: the first rules out following the
template's newest version, the second rules out a cached value.
**Explicit-ID path.** `coder external-auth access-token github-broad`
returned that provider's result even though the template declared only
`github-dotfiles`, and did not substitute the declared provider's
already-valid token.
Raw traces were captured with `GIT_CURL_VERBOSE=1 git -c
credential.helper="" ls-remote <private repo>`, reading the unredacted
`== Info: Server auth using Basic with user '<token>'` line. A private
repo is required, since a public one never triggers a `401` and
therefore never invokes `GIT_ASKPASS`.
</details>
|
||
|
david-fraley
|
d458fe4941 | fix(coderd/database): match group name case-insensitively in search (#27894) | ||
|
Yevhenii Shcherbina
|
dae41eb711 |
fix: detect out-of-range AI Gateway costs instead of wrapping silently (#27602)
Implements: https://linear.app/codercom/issue/AIGOV-448/use-decimal-for-cost-computation Follow-up to https://github.com/coder/coder/pull/26229 Follow-up to the AI Gateway cost-control work. Cost is computed per token category as `tokens × price / 1_000_000` in `int64`, then summed. This change makes an unrepresentable result a defined outcome instead of an accident of integer wrap-around. ## Motivation The intermediate `tokens × price` can exceed `int64`. Real usage cannot get there: at a $75/M model the product overflows at roughly 123 billion tokens in a single response, about six orders of magnitude above a maxed-out Opus request, so this is not a live incident. The problem is what happens if it ever does, because the sign of the wrapped value silently selects between two different failure modes, neither of which was chosen: 1. **Wraps positive.** A plausible-looking cost is stored, incremented into the user's daily spend, and enforced against their AI budget. No error, no signal, wrong number. 2. **Wraps negative.** The value violates `CHECK (cost_micros >= 0)`, the insert fails, the surrounding transaction rolls back, and `RecordTokenUsage` returns a Postgres constraint error that says nothing about overflow. The token usage record is lost entirely, along with its token counts. So the same class of bad input either corrupts budget accounting or discards an audit record, depending on arithmetic that nobody reasoned about. That is the undefined behaviour. ## Decision **An unrepresentable cost is treated as bad input, not a large bill.** Since real usage cannot produce one, it can only mean a wrong price row or implausible provider-reported token counts. In both cases the true cost is unknowable, so no number is stored. **Detect rather than avoid.** `computeCost` now evaluates in `decimal`, so nothing wraps, and range-checks the total against `[0, MaxInt64]` before converting back. Out of range returns `errCostOutOfRange`. Rejecting negatives in the same check also keeps them away from the non-negative column constraint, which would otherwise discard the record. **Log, do not block.** The error is swallowed at the call site: the record is written with token counts intact and `cost_micros` NULL, the spend update is skipped, and the condition is logged at ERROR. **Per-category truncation is unchanged.** Each category is still truncated independently rather than the total being rounded once, so a per-category breakdown recomputed from the snapshotted price columns sums exactly to the stored total. Every existing `computeCost` test case passes unmodified. |
||
|
Nick Vigilante
|
9dcb75cd56 |
chore: add docs inline-HTML linter and backtick generated placeholders (#27399)
## What Adds CI enforcement that fails when docs Markdown contains invalid inline HTML the docs site silently drops or mangles, and fixes the remaining generated-doc placeholders at their source. This is the tooling half of the docs-HTML audit. The hand-written fixes it guards landed in #27298 (kept small and separate so it reviewed fast); this PR carries everything that touches code, CI, or generated output. ## Changes **Linter (`scripts/docshtmlcheck`), wired into `make lint` via `lint/docs-html`.** Markdown-aware: parses each file with goldmark and inspects only raw-HTML nodes, so angle brackets in fenced code blocks, inline code, HTML comments, and `<https://…>` / `<user@host>` autolinks are ignored. Flags swallowed placeholders (`<region>`), void-element end tags (`</br>`), unregistered or incorrectly capitalized component tags (`<Image>`), and unclosed container tags (a `<div class="tabs">` that leaks its wrapper). The one intentional renderer component, `<children>`, is allowed but still balance-checked. **Generator-source placeholder fixes (regenerated via `make gen`).** - `codersdk/chats.go`: backtick `<server>__` in the `ChatContextTool.Name` doc comment (it becomes the Swagger description, so it was swallowed in `reference/api/{chats,schemas}.md`). - `codersdk/deployment.go`: backtick `<region>` in the AWS Bedrock region flag help (swallowed in `reference/cli/server.md`); also updates `coder server --help` output and the golden files. **Temporary allowlist.** `docs/reference/cli/agent-firewall.md`'s `<host>` / `<glob>` come from the external `github.com/coder/boundary` CLI help (still `v0.10.0` on `main`), so they are suppressed on that one file. The suppression is self-clearing: if an allowlisted tag stops appearing on a scanned file, the linter reports `stale-allowlist-entry` and fails until the dead entry is removed, so a dead entry cannot silently mask a later regression of that tag on that page. (An entry whose file is deleted outright is never rescanned, but a missing file yields no findings, so nothing hides behind it either.) ## Review feedback addressed This tool + generator work was reviewed by Coder Agents Review while it was bundled into #27298. Addressed here: - **P1:** tokenize each raw-HTML node as a whole instead of per source line, so a tag whose attributes wrap across lines is no longer torn in half. This fixes both the missed multi-line unclosed `<div>` (a leaked wrapper that passed with exit 0) and the spurious `stray-end-tag` on valid multi-line tags. Each token maps back to its own source line. - Normalize allowlist lookup/report paths to a canonical repo-relative form, so the escape hatch no longer silently misses under absolute / `./` paths. - Route generated-page findings to the generator source. - Add `<search>` to the allowed set; reword the unknown-element message to note that a real element can be added to `allowedElements`. - Self-clearing allowlist guard (above); rename `optionalEndTag(s)` and `kindUnclosed(Tag)`; adopt `slices`/`maps` idioms; move the lint banner to the Makefile recipe; stop aliasing the input slice in `filterAllowed`. - New tests: multi-line tokenization (both classes), interleaved nesting, a pinned line number, `collectMarkdown`, and the stale-allowlist guard. ### Round 2 (Coder Agents Review on this PR) A second `/coder-agents-review` pass on this PR raised 16 findings; addressed in `fix(docshtmlcheck): catch self-closing containers and capitalized tags`: - **P2:** self-closing container tags (`<div class="tabs"/>`) were ignored by the HTML5 parser and leaked their wrapper like the open spelling; the balance check now tracks self-closing tokens too (CRF-1). - **P2:** a capitalized component tag whose lowercase name is a real element (`<Table>`, `<Section>`) slipped through on the `allowedElements` lookup. The tokenizer lowercases tag names, so the check now reads the raw token and reports any capitalized name as a component reference (CRF-2). - Narrowed the `:` / `@` autolink skip to a real URI scheme or a dotted `local@domain`, so `<region:id>` and `<user@host>` stay checked (CRF-3). - Stale-allowlist findings now report against the linter source with no line, and count separately from invalid-HTML issues in the footer (CRF-7, CRF-11). - Comment / README / Makefile wording synced to the honest capitalized-tag behavior; added the deleted-file allowlist caveat and a note that `allowedElements` is hand-maintained against the renderer (CRF-14, CRF-17, CRF-9). - Internal cleanups (`pop` -> `matchEndTag`, extracted `unclosedFinding`) and new tests: self-closing, capitalized open/close, colon/at placeholders, a non-first-token line assertion, `isGeneratedDoc`, and the stale message (CRF-12, CRF-13, CRF-1/2/3/4/5/16). Two findings resolved without a code change: - **CRF-8** (also wire `lint/docs-html` into `lint-light`): declined. `lint-light` is the Go-free fast path; `lint/docs-html` needs the Go toolchain, so it stays in the full `make lint`, which CI runs. Adding it would pull Go into the light path for no coverage gain. - **CRF-9** (`allowedElements` <-> renderer coupling): documented with a maintenance note in the `allowedElements` comment and tracked in DOCS-597 for a cross-repo sync/check decision. Deferred (note, no current trigger): raw-text element interiors (`<script>` / `<style>`) are not scanned for nested tags. No docs page relies on this today; noted for follow-up. ## Merge order #27298 (the hand-written fixes this PR guards) has merged, and this branch is rebased on `main`, so `make lint/docs-html` now reports 0 findings and the `lint` check passes. The two PRs are independent (disjoint files, no stacking). ## Verification - `go test ./scripts/docshtmlcheck/`, `go vet`, `gofmt -l`, `golangci-lint run`: clean. - `make lint/docs-html` (branch rebased on `main`): 0 findings. ## Linear - DOCS-584: https://linear.app/codercom/issue/DOCS-584/add-ci-check-that-fails-on-invalid-inline-html-in-docs - DOCS-551: https://linear.app/codercom/issue/DOCS-551/backtick-placeholder-syntax-in-generated-reference-docs-cli-help - DOCS-597 (follow-up, from CRF-9): https://linear.app/codercom/issue/DOCS-597/track-docshtmlcheck-allowedelements-drift-vs-docs-renderer-component > This PR was created with AI assistance (Coder Agents). |
||
|
Bobby HoandClaude Sonnet 5
|
d814dfad88 |
feat(coderd): support public OAuth2 client tokens at the schema layer (#27712)
Layer 1 of a multi-PR split of #27195 (public/secretless PKCE-only OAuth2 clients), broken up for easier review: **database schema (this PR)** → oauth2provider handler logic → API/e2e integration tests. ## Goal Coder's OAuth2 provider only works correctly for confidential clients today. Public clients — native apps that can't safely hold a shared secret, such as the CLI's browser-based login flow, IDE plugins (VS Code, JetBrains), desktop apps, and MCP clients — cannot complete a real OAuth2 flow against Coder, even though OAuth 2.1 §2.1 explicitly defines this client type and RFC 8252 §8.5 requires PKCE alone to be sufficient authentication for it. Every MCP client, CLI login flow, and IDE plugin is a public client by construction, and none of them can complete a secretless flow against Coder today: dynamic registration always classifies a client as confidential regardless of what it asks for, the token endpoint unconditionally requires a `client_secret`, and discovery metadata never advertises `"none"` as a supported auth method. Full write-up: [ENG-3029](https://linear.app/codercom/issue/ENG-3029/oauth2-support-public-client) ### Overall design (end state across the full PR stack) `[PR2]` marks handler-layer changes landing in the next PR in this stack. The green box is what this PR implements. ```mermaid sequenceDiagram autonumber participant C as Public Client (CLI/MCP/IDE plugin) participant S as coderd (chi router) participant H as oauth2provider handlers participant DB as PostgreSQL Note over C,S: Discovery C->>S: GET /.well-known/oauth-authorization-server S->>H: GetAuthorizationServerMetadata() Note over H: [PR2] add "none" to<br/>the returned auth methods list H-->>C: [PR2] 200 { token_endpoint_auth_methods_supported:<br/>[..., "none"] } Note over C,S: Dynamic Client Registration C->>S: POST /oauth2/register<br/>{redirect_uris, token_endpoint_auth_method: "none"} S->>H: CreateDynamicClientRegistration() Note over H: [PR2] client type now reads<br/>the request -> "public" Note over H: [PR2] skip secret generation<br/>for public clients H->>DB: [PR2] INSERT app row<br/>(client_type = 'public') DB-->>H: app row Note over H: [PR2] skip secret insert entirely H-->>C: [PR2] 201 { client_id }<br/>(no client_secret field) Note over C,S: Authorization Code + PKCE flow C->>S: GET /oauth2/authorize?client_id=...&code_challenge=... C->>S: POST /oauth2/tokens (grant_type=authorization_code)<br/>no client_secret S->>H: extractTokenRequest() Note over H: [PR2] client_secret no longer required<br/>for public clients H->>H: authorizationCodeGrant() Note over H: [PR2] skip secret lookup for public clients Note over H: PKCE verification — already mandatory, unchanged rect rgb(198, 239, 206) Note over H,DB: [THIS PR] oauth2_provider_app_tokens.app_id<br/>column added (NOT NULL, populated at insert<br/>time from app.ID) and app_secret_id loosened<br/>to nullable. Revocation now checks app_id<br/>directly. Confidential-client behavior is<br/>unchanged — no public client can be created yet. H->>DB: [PR2] INSERT refresh token row<br/>(no secret reference, for public clients) end DB-->>H: token row H-->>C: 200 { access_token, refresh_token } ``` ## This PR: database schema A public client has no `client_secret`, so it has nothing to put in `oauth2_provider_app_tokens.app_secret_id`, which was `NOT NULL`. This PR makes that column nullable and instead attributes a token to its owning app through a new, always-populated `app_id` column — so ownership checks (e.g. revocation) work identically for public and confidential clients without joining through a secret that may not exist. | Column | Before | After (this PR) | |---|---|---| | `app_secret_id` | `uuid NOT NULL` | **nullable** | | `app_id` | — | **new**: `uuid NOT NULL`, `FOREIGN KEY → oauth2_provider_apps(id) ON DELETE CASCADE`, backfilled for every existing row and populated on every new insert from that point on | This is a single, complete migration — not staged across multiple PRs. An earlier version of this branch deferred `app_secret_id`'s nullability and the insert-time population of `app_id` to a later PR, keeping this PR's diff limited to `coderd/database`. [Automated review](https://github.com/coder/coder/pull/27712#discussion_r3686851911) correctly flagged that as unsafe: the migration would backfill existing rows once, but nothing would populate `app_id` for rows written afterward, so the moment this PR merged, new tokens would start accumulating a permanently `NULL` app_id — and if a release happened to be cut before the follow-up PR landed, that gap could ship to customers and would need a second, later backfill to close. Doing the full migration now avoids that: `app_id` is correct from the first row written, and the promised `NOT NULL` constraint requires no data repair because it's already enforced. Closing that gap requires a few mechanical, non-branching touches outside `coderd/database`: - `revoke.go`'s two ownership checks now compare `dbToken.AppID` directly instead of looking up the app through `app_secret_id` — a genuine simplification (and slightly less code), not a temporary shim. - `tokens.go`'s two `InsertOAuth2ProviderAppToken` call sites supply the new `app_id` column and wrap `app_secret_id` as a `NullUUID`. - `oauth2_test.go`'s one direct-insert test fixture does the same. None of these introduce client-type branching or new capability — every client today is still confidential-only, still always presents a secret, and behavior is unchanged. The full repo builds, vets, and all existing tests pass unmodified in behavior. ## Coming next - **PR2 (handler layer)**: `codersdk`'s `DetermineClientType()` reading the requested `token_endpoint_auth_method`; `registration.go` skipping secret generation for public clients (and wrapping the app+secret insert in a single transaction, fixing a pre-existing orphan-row/visibility-race gap); `tokens.go` making the secret check conditional so PKCE alone authenticates a public client; `metadata.go` advertising `"none"` in discovery. No further migration is needed — the schema this PR ships is already final. - **PR3 (API/e2e layer)**: integration tests through the real HTTP API (`coderd/oauth2_test.go`), the MCP OAuth2 e2e flow (`coderd/mcp/mcp_e2e_test.go`), and the manual test script (`scripts/oauth2/test-mcp-oauth2.sh`). Depends on: #27195 (original combined PR, being superseded by this stack) --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> |
||
|
Cian Johnston
|
c62079c053 |
refactor(coderd): optimize chatdebug (#27129)
Adds a bund of optimizations to chatdebug:
In `coderd/x/chatd/chatdebug`:
- Adds a benchmark (excluding LLM and database stuff)
- Replaces string concatenation with strings.Builder when accumulating
stream parts (~105,000ns -> ~50,00ns)
- Removes double JSON encode in RecordingTransport (114,000ns ->
64,000ns)
In `coderd/util/strings`:
- Adds a benchmark for Truncate
- Removes unnecessary allocations in Truncate (~110,000ns -> 1,550ns in
truncation case, 1 alloc -> 0 allocs in no truncation case)
> 🤖 Claude helped with this.
|
||
|
Ethan
|
1702bbb816 |
test(coderd/x/chatd): accept query cancellation in subagent wait (#27818)
Closes CODAGT-877 Closes https://github.com/coder/internal/issues/1437 The linked flake happens because the context deadline can be observed as `context.DeadlineExceeded`, or as a PostgreSQL query cancellation when a database call is in flight. The fix is just to assert that the deadline expired and the returned error is a recognised query cancellation, rather than depending on which layer notices it first. |
||
|
Yevhenii Shcherbina
|
11427066a1 |
fix: require bedrock model fields for the invoke-model protocol (#27846)
Implements: https://linear.app/codercom/issue/AIGOV-564/aibridge-bedrock-provider-skipped-404-on-all-routes-when-settings-omit Improves validation when creating and updating AI providers: a Bedrock provider using the `invoke-model` protocol now requires `model` and `small_fast_model`. This brings API validation in sync with the UI, which already required both fields. |
||
|
Paweł Banaszewski
|
8f5f15a92f |
fix: remove unbound Client() method from aibridged.Server (#27845)
Adds client context to `Client()` method in `aibridged.Server`, effectivly renaming `ClientContext()` method as `Client()`. Similarly `aibridged.ClientFuncWithContext` became `aibridged.ClientFunc`. `aibridged.Server.Client()` acquired a DRPC client with `context.Background()`, callers in theory could wait indefinitely for the daemon to connect to coderd. Every call site already had a context except the recorder callback. `aibridge.NewRecorder` takes a `func(context.Context) (Recorder, error)` and acquires against the record call's context. |
||
|
Susana Ferreira
|
db88ec3f6a |
fix: price AI usage by configured provider type (#27836)
## Problem AI Gateway records the aibridge provider on each interception, which is the upstream wire format and only ever `anthropic`, `openai`, or `copilot`. Prices are matched on exact provider and model equality, so a provider configured as Azure, Bedrock, Google, OpenRouter, or Vercel is priced as if it were native OpenAI or Anthropic, matching either the wrong price or no price at all. ## Changes - Resolve the configured provider type from `ai_providers` by provider name, which is unique among live providers, and key the price lookup on it instead of the aibridge provider. No schema change is needed. - Label `unpriced_token_usage_records_total` with the same provider value used for the lookup, so it names a provider an operator actually configured. - Treat a provider that cannot be resolved as unpriced, consistent with how a missing price is handled today. Closes https://linear.app/codercom/issue/AIGOV-570/resolve-ai-model-prices-using-the-configured-provider-type Depends on the follow-up that extends the shipped price book to the remaining provider types: https://linear.app/codercom/issue/AIGOV-571/ship-prices-for-all-ai-governance-provider-types > [!NOTE] > Initially generated by Claude Opus 5, modified and reviewed by @ssncferreira |
||
|
Cian JohnstonandSusana Ferreira
|
60161fd375 |
chore: update model prices to include more providers (#27837)
Adds the full set of supported provider types to `scripts/aibridgepricesgen` and updates stored model prices. Notes: * We need to rename a few keys from models.dev JSON to match our internal provider types. * `prices.json` is now marked as generated. --------- Co-authored-by: Susana Ferreira <susana@coder.com> |
||
|
Michael Suchacz
|
6b8f820493 |
feat: remove native chat cost tracking in favor of AI Gateway cost data (#27330)
## Stack Context This stack makes AI Gateway data and budgets the source of truth for AI spend controls. 1. Re-back the per-chat cost endpoint with AI Gateway data (#27328, merged). 2. Remove native chat usage limits (#27329, merged). 3. **This PR, now based on `main`:** remove native chat cost tracking and its dedicated admin UI. ## Summary Removes native per-message price calculation, model pricing fields, cost persistence, aggregate cost queries, and admin cost API types. It also deletes the Analytics and Spend pages plus their legacy redirects. The AI Gateway-backed per-chat cost row and compact budget indicators remain. The spend documentation is renamed to `spend-management.md` and updated for the remaining surfaces, group budget APIs, CSV export, upgrade handling for native pricing and cost history, and the absence of a deployment-wide spend dashboard. The per-chat cost API documents that data follows AI Gateway retention and reports zero after all matching requests are purged. No schema is dropped in this release. `chat_messages.total_cost_micros` remains nullable and unwritten so replicas from the previous release can continue inserting messages during rolling upgrades. #27600 tracks removal after the compatibility window. > Mux prepared this PR on Mike's behalf. |
||
|
Michael Suchacz
|
f0e6ac64b3 |
feat: remove native chat usage limits in favor of AI Gateway budgets (#27329)
## Stack Context This stack makes AI Gateway data and budgets the source of truth for AI spend controls. 1. Re-back the per-chat cost endpoint with AI Gateway data (#27328, merged). 2. **This PR:** remove native chat usage limits. 3. Remove native chat cost tracking and its dedicated admin UI (#27330). ## Summary Removes the native usage-limit API, SDK types, SQL, and chat enforcement for deployment, user, and group chat limits. Compact AI Gateway budget indicators remain in the Agents sidebar, user menu, and group settings. Gateway budget rejections and provider quota failures continue to classify as usage-limit errors, including a 409 response for synchronous title generation. Budget-period labels now use the API's UTC boundaries, so users see the same dates in every browser timezone. The documentation explains the AI Gateway replacement, its licensing requirements, and the differences from native limits. No schema is dropped in this release. The usage-limit table, index, user and group columns, constraints, audit mappings, and generated scan fields remain for mixed-version rolling upgrades. #27600 tracks their removal after the compatibility window. ## Breaking change Native day, week, and month chat spend limits are removed and are not migrated. AI Gateway budgets are month-based, group-scoped with per-user overrides, and require the AI Gateway entitlement. Deployments without that entitlement no longer have chat spend enforcement. > Mux prepared this PR on Mike's behalf. |
||
|
Michael Suchacz
|
404bb2f663 |
refactor(coderd/x/chatd): own provider option construction in one function (#27705)
Stacked on #27704. Provider option conversion and reasoning effort injection both create OpenAI option structs, so each of the four call sites had to pair them in the right order and pick the same transport for each. `chatprovider.ProviderOptionsForCall` now owns both steps, and the two helpers it wraps are unexported. The advisor, main generation, compaction override, and quickgen paths each collapse to one call. The ARCHITECTURE section on transport selection is updated to match: `ProviderOptionsForCall` is described as the only entry point in `chatprovider` that builds provider options for a call, delegating OpenAI conversion to `chatopenai.ProviderOptionsFromChatConfig`, and it now records that the quickgen turn status label and chat summary paths deliberately send no provider options. > Mux prepared this PR on Mike's behalf. |
||
|
Michael Suchacz
|
0e16e356b0 |
refactor(coderd/x/chatd): read the OpenAI transport from the model (#27704)
Stacked on #27703. Provider option conversion, reasoning effort injection, and file part acceptance each recomputed the OpenAI wire format from `(provider, modelID, override)`. They now read it from `chatprovider.Model`, so a decision cannot drift from the client it was built for. `ProviderOptionsFromChatConfig` takes a `Transport`, `ApplyReasoningEffort` takes a `Model`, and `AcceptsFilePartMediaType` becomes a `Model` method. `UsesResponsesAPI` and `UsesResponsesOptions` are deleted. The override extraction is unexported and reachable only from `ModelFromConfig`, which now takes the model's `ChatModelOpenAIConfig` directly, removing the six scattered extractions at call sites. That also resolves the computer-use mismatch. The computer-use model is a hardcoded default with no config row of its own: its client was built without an override while request preparation applied the chat model's. Preparation now reads the computer-use model's own transport, so the two agree without one model's client settings following a different model. Passing the chat model's `openai_config` into the computer-use client would have made them agree on the wrong value. `TestModelTransportConsumersAgree` pins the invariant in one test: the HTTP path the client actually hits, the concrete provider option struct type, the type created by reasoning effort, and text/image file acceptance. > Mux prepared this PR on Mike's behalf. |
||
|
Michael Suchacz
|
b5c9e8e471 |
refactor(coderd/x/chatd): carry the resolved OpenAI transport on a model wrapper (#27703)
Stacked on #27683. The OpenAI wire format is decided when the client is built, then thrown away, so downstream sites recompute it from `(provider, modelID, override)`. Any disagreement fails silently: the SDK type-asserts the concrete provider options struct and discards every OpenAI option, and text attachments are dropped because Responses natively accepts only images and PDFs. This adds `chatprovider.Model`, which pairs a fantasy client with the transport resolved from that client's own identity. Its fields are unexported and only the constructor sets the transport, deriving it from the client, so no caller can pick a transport that disagrees with the client it wraps. `chatopenai.Transport`'s zero value is invalid and panics when read rather than defaulting to a wire format, following the existing precedent for construction invariants. `Model` is threaded through construction, the resolve paths, and the four struct fields that store a model for later request preparation. Terminal call sites keep taking `fantasy.LanguageModel` and receive `LanguageModel()`, which avoids a new package edge from `chatloop` and `chatadvisor` into `chatprovider`. No decisions move yet. The consumers still recompute the transport, and `UsesResponsesAPI` now delegates to `TransportFor` so the two agree by construction. #27704 makes the consumers read it from the model. > Mux prepared this PR on Mike's behalf. |
||
|
Michael Suchacz
|
c6cee10e8b |
feat: add per-model OpenAI Responses API toggle (#27683)
chatd hardcoded `WithUseResponsesAPI()`, so the provider SDK's static known-model list decided whether an OpenAI model spoke the Responses API or Chat Completions. A model absent from that list silently fell back to Chat Completions until the fantasy fork was patched. This exposes the SDK's `WithResponsesAPIFunc` hook as a per-model setting, `openai_config.use_responses_api`, stored in the existing `chat_model_configs.options` JSONB. Unset keeps the known-model list, `true` forces Responses, `false` forces Chat Completions. There is no migration. It sits in a new construction-time `openai_config` section rather than in `provider_options.openai` because it selects the API when the client is built, while `provider_options` holds per-request parameters. That placement is also load-bearing: a config setting only this field would otherwise materialize an OpenAI request-options struct and turn on provider-side response storage, since `Store` defaults to true there. Three places independently decided the transport and would silently disagree with the client actually built: | Site | Effect when it disagrees | | --- | --- | | `ModelFromConfig` | the transport being overridden | | `AcceptsFilePartMediaType` | text attachments dropped, since Responses natively accepts only images and PDFs | | `UsesResponsesOptions` | the SDK type-asserts the concrete options struct, so every OpenAI option is discarded | They share one predicate here, `chatopenai.UsesResponsesAPI`, with the override threaded to each. The rest of the stack removes that threading by resolving the transport once and carrying it. Compaction overrides and the quickgen debug model built clients without `ConfigOptions`, so they now pass it and pick up both this setting and the existing Anthropic beta headers. The toggle also makes transport-conditional option handling admin-switchable, so two hardening changes ride along. `ServiceTierFromChat` now maps every tier the codersdk enum advertises (`auto`, `default`, `flex`, `scale`, `priority`); it previously returned nil for `default` and `scale`, so flipping a model to Responses silently dropped a configured `service_tier` that the API accepts (fantasy forwards the value unchanged). And a new `TestProviderOptionsTransportParity` pins, per `provider_options.openai` field, which transport honors it, against a table in ARCHITECTURE.md, so a field honored on one transport and silently ignored on the other fails the test unless recorded as intentional. Review rounds also caught two lifecycle gaps around the new field. `isZeroChatModelCallConfig` now inspects `OpenAIConfig`, so a stored options blob whose only setting is this toggle survives into GET/list responses instead of reading as `model_config: null`; `TestIsZeroChatModelCallConfigCoversEveryField` sets each config field in isolation and fails if any field is invisible to the zero check. And the model editor's update path sends an explicit empty `model_config` when an edit clears the last field, since an omitted property preserves the stored options server-side; covered by the `EditClearingLastOptionSendsEmptyConfig` story. Azure keeps following the known-model list, because the Azure provider exposes no equivalent hook. The model editor renders Azure with the OpenAI option schema, so instead of shipping a visible but inert control, the option schema generator gains a `providers` struct tag that it emits as `visible_for_providers`. Gating uses the raw provider type rather than the alias table, so the control appears only for openai-typed providers. No hand-written frontend field: the editor renders it from the generated schema. Closes https://linear.app/codercom/issue/CODAGT-874/add-completionsresponses-api-toggle-in-model-editor > Mux prepared this PR on Mike's behalf. |
||
|
Michael Suchacz
|
9bab7a8e06 | fix(coderd): preseed AI providers for title tests (#27564) | ||
|
Steven Masley
|
52423eb87b |
feat: promote MinimumImplicitMember experiment to GA (#27472)
Promotes the `minimum-implicit-member` experiment to GA and removes it.
## What changes
- The `minimum-implicit-member` experiment constant, its
`RoleOptions.MinimumImplicitMember` toggle, and the global
`rbac.MinimumImplicitMember()` accessor are deleted. The minimal-member
behavior is now the only behavior: `organization-member` and
`organization-service-account` carry only the floor (read-self records,
notifications, and similar) and grant **no workspace permissions**.
Workspace access lives exclusively on the
`organization-workspace-access` role.
- The experiment gate on customizing `default_org_member_roles` (`PATCH
/organizations/{org}`) is removed; the built-in-roles-only validation
remains.
- The dashboard's Default Roles section and the implied-roles display on
the members page are no longer experiment-gated.
- Admin docs: new "Default member roles" section in
`docs/admin/users/organizations.md`, cross-linked from
`groups-roles.md`.
## Why this is safe for existing deployments
Migration `000516` (shipped earlier) backfilled
`default_org_member_roles` with `['organization-workspace-access']` on
every organization. Members therefore keep exactly the effective
permissions they had with the experiment off; the workspace elevation
flows through the default role instead of being baked into
`organization-member`.
**Rollback caveat:** rolling back past this release restores the bundled
elevation, silently re-granting workspace access to members of
organizations that cleared their default roles.
## Review
Deep-review R1 findings are addressed in `chore: address deep-review
findings` (copy fixes, read-only Default Roles for viewers, removable
overlapping explicit grants, RBAC prose restoration, test
de-tautologizing, docs). Point-by-point disposition is in the PR
comments.
---
Generated by Coder Agents on behalf of @Emyrk.
|
||
|
Steven Masley
|
76eb43a78e |
test(coderd): drain dormancy audit log before reset in StartWakesUpDormantWorkspace (#27541)
Closes PLAT-219 / [coder/internal#1532](https://github.com/coder/internal/issues/1532). The reported failure (`should have 2 item(s), but has 3`) was the stop build's outcome audit landing after `auditor.ResetLogs()`; that path was already deflaked by #26232 and the pre-reset drain added in #25037. This closes the remaining hole in the same class: the dormancy update's audit log is exported in a middleware `defer` after the API response, so it can also land after `ResetLogs` and break the exact `len == 2` assertion. Extend the pre-reset poll to drain the dormancy write entry alongside the stop build entry. Verified with `go test ./coderd -run TestWorkspaceDormant -count=5` and `-race`. > Generated by Coder Agents on behalf of @Emyrk. |
||
|
Michael Suchacz
|
fc24c27dfd |
fix: reserve chat hook dispatch capacity for running turns (#27656)
## Context Follow-up fix from live UAT of the merged chat lifecycle hooks stack (#27430). Its companion UAT fix (#27655) has merged, so this targets `main` directly. ## Why? UAT measured a burst of 1,500 concurrent chat creations against a consumer with 1.2s latency. 255 were admitted and 1,245 got `502 hook_dispatch_failed (over_capacity)`, which is correct fail-closed behavior. The collateral wasn't: the same burst failed 24 `stop` dispatches, parking chats that had already been admitted and had already executed tools. One 256-slot semaphore served every event, so new-work admission could take every slot and kill turns in flight. Callers now classify each dispatch as admission or generation, and admission draws from a 192-slot gate held *before* the shared pool. At least 64 shared slots stay reachable only by dispatches for work a chat already admitted. The dispatcher is per `coderd` replica, so these limits are per replica, not deployment-wide, and the docs say so. **The caller classifies, not the event type.** Event type isn't a reliable proxy in either direction: a subagent spawn dispatches `user_prompt_submit` from inside a running turn, and the edit path dispatches `session_start` at admission time. `CapacityClassUnset` is rejected in `Dispatch`, so a new call site fails closed rather than silently inheriting a share. **Acquisition order is load-bearing.** Admission takes its own gate first. Taking a shared slot first would let admissions queued on the gate occupy the very capacity the reserve protects. `acquireCapacity` is the only path that takes either pool, so the order can't be bypassed. ## What this does not guarantee Nothing bounds how many turns generate concurrently, so the 192/64 split is a judgement call, not a derived ceiling. This stops an *admission* burst from consuming every slot; it does not make the remainder sufficient. A large enough generation load can still exhaust the reserve and error a running chat. The docs say so explicitly rather than promising a guarantee the code doesn't deliver. Generation can now take all 256 slots, so generation traffic starves admission harder than before. That's the intended priority: rejecting a new prompt is recoverable, ending a turn that already ran tools is not. ## Testing Red-green proved both new tests. Removing the release-on-failure path fails `RefusedSharedAcquireReleasesAdmission` deterministically; removing the expired-deadline check fails `ExpiredDeadlineRefusesFreeSlot` in 18/30 runs. That deadline check fixes a real race found in review. `acquire` previously shared one `time.Timer` across both acquires. Because `select` picks a ready case at random, an admission dispatch could take a slot after its capacity deadline had passed. Measured over 300 trials: 135 late acquisitions, worst overshoot 2.1ms. `acquire` now takes an absolute deadline and refuses an expired one before selecting, which measures 0/300. Go: `coderd/x/agenthooks/...` and `coderd/x/chatd/...`, plus `-race -count=3` on the dispatcher. > Mux opened this PR on Mike's behalf. |
||
|
Fabien Penso
|
7bd9f5ec93 |
fix: correct authorization header spelling in api docs (#27721)
Corrects the misspelled `Authorizaiton` Swagger header name to `Authorization` in the source annotation and checked-in generated API documentation. This prevents generated API specs and SCIM examples from documenting the wrong HTTP header name. |
||
|
Michael Suchacz
|
df1c0f9710 |
feat: show what a chat lifecycle hook changed (#27655)
## Stack Context Follow-up fixes from live UAT of the merged chat lifecycle hooks stack (#27430). Two PRs: 1. **This PR**: make hook effects visible and correctly attributed in the transcript. 2. [`mike/chat-hooks-uat/dispatch-capacity`]: reserve dispatch capacity so an admission burst can't fail running turns. ## Why? UAT found three ways the transcript misrepresented what a lifecycle hook did. All three are user-visible and share the same surface (`chathooks/effects.go`, `codersdk.ChatMessagePart`, the conversation timeline), so they're reviewed together. **A prompt `input_override` silently discarded attachments.** `ComposeUserPromptContent` replaced the entire submitted part list with one text part, dropping `file` and `file-reference` parts along with their `chat_file_links`. The user saw their attachments vanish with no explanation. The override now replaces submitted *text* parts only and preserves non-text parts in order. A consumer that wants to block attachments uses `deny`, which is the documented mechanism for refusing a submission. **Every user-visible `system` row was labelled "Lifecycle hook".** The timeline keyed the notice off `role === "system"`. That was correct only by accident, because the hook `user_message` was the sole client-visible system row. The backend now emits the notice as a typed `hook-notice` part and the timeline renders on that, so a future system row can't be mislabelled as a policy notice. **Nothing marked a tool call the hook had rewritten.** A consumer could replace tool input via `input_override` and the transcript showed the rewritten input as if the model had produced it. `ChatMessagePart` gains `hook_rewritten`, set from `preflight.Overrides` on the same path that already carries `ToolCallCreatedAt`, and the tool row renders a "Modified by policy" badge. `ToolCall.PolicyProvider` renders the badge itself, at four wrap sites: the `Tool` dispatch wrapper, the `ReadFilesTool` aggregate and its per-file rows, and `ReadFileTimelineBlock` (grouped and single `read_file` rows bypass `Tool`). Renderer props do not include the flag; descendants consume it through the provider context. The badge is emitted by the provider rather than by the shared header because several renderer branches return early without one, including the auth-required `execute` card, a completed `ask_user_question`, and an empty question payload. Those branches would drop the attribution with no type or runtime error, and the gap is not greppable: every renderer file contains a header somewhere, only individual branches do not. Emitting at the provider removes the possibility instead of enumerating the cases. A rewritten call is wrapped in a group labelled by its badge, so one rewritten file inside a merged read is attributed on its own rather than inheriting the group's badge. `HeaderButton` still appends the policy wording to an explicit `ariaLabel`, since an explicit `aria-label` replaces the name computed from descendants. Provider-executed calls are excluded from attribution. Hooks never see them, and duplicate tool-call ID rejection deliberately skips them, so a reused ID would otherwise mark a provider-executed call as policy-rewritten. ## Testing Go: `coderd/x/chatd/...`, `coderd/x/agenthooks/...`, `codersdk/...`, and `coderd -run 'Hook|Chat'`. Frontend: `tsc` plus every `AgentsPage` story; the only failures are `MCP Tool Completed` and `Scroll To Bottom Button Works With Inverse Scroll`, both of which fail on trunk. A registry-wide story asserts every registered renderer shows the badge, verified against three inverted toggles: removing the badge, hiding it with `display:none`, and skipping the provider for one renderer (which names that renderer). Storybook also covers the rewritten subagent spawn, a completed empty question payload, a non-hook system message, and a failed `read_file` guarding the accessible name. > Mux opened this PR on Mike's behalf. |
||
|
Sas SwartandClaude Opus 4.8
|
8886a5749a |
feat: add network calls list to AI session threads API (#27425)
The AI session threads API returned only a network call *summary* (total/blocked counts + top domains). This adds the per-call list so the session detail can render individual Agent Firewall network calls. `ListAIBridgeSessionNetworkCalls` reuses the same sequence-number windowing as the existing summary and includes all protocols. The list is exposed as `network_call_logs` on the threads response and is capped server-side at 100 rows. The summary (`network_calls.total`/`blocked`) remains authoritative for whole-session totals: the list length and its blocked count equal the summary only when a session has at most 100 calls, and are truncated beyond that. ### PR map (merge strictly bottom-up) This change is a 4-PR stack. Each PR depends on all the ones below it, so merge in this exact order: 1. #27417 — backend network summary 2. #27418 — frontend summary rows 3. #27425 — backend per-call list `network_call_logs` 4. #27426 — frontend network-calls panel Refs AIGOV-464 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
Josh FreeandCopilot
|
2acfe7e829 |
feat(coderd/externalauth/gitprovider): use conditional requests for GitHub JSON reads (#27628)
Fixes #27627. ## What The chat diff-status gitsync worker polls open pull requests on a fixed 10s interval and re-downloads the full JSON body every tick, even when nothing changed, because the GitHub client never sends `If-None-Match` / ETag. This adds a small, concurrency-safe, bounded in-memory ETag+body cache (`coderd/externalauth/gitprovider/conditional.go`) and wires it into `githubProvider.decodeJSON`. When an ETag is cached for a request, we send `If-None-Match`; on `304 Not Modified` we decode the cached body; on `200` we cache `{etag, body}` when an ETag is present and the body is under a size cap. ## Why `304` responses do not count against GitHub's primary rate limit, but full `200`s do. Today every unchanged poll burns quota that the same token also needs for interactive Git and API operations, so busy instances can hit rate-limit errors and stalls elsewhere. Unchanged PRs now revalidate for free with no behavior change; only genuine changes transfer a body. ## Details - Cache key = request URL + a hash of the token, so one token's response is never served under another; raw tokens are not retained. - Bounded by entry count (LRU eviction, default 2048) and per-body size (1 MiB) to cap memory. - Scope limited to the JSON reads through `decodeJSON`; the raw-diff path (`fetchDiff`, up to `MaxDiffSize`) is intentionally left out to avoid caching large bodies. ## Tests `TestConditionalRequestReuse` in `github_test.go` covers: - `NotModifiedReusesCachedBody` — a warm poll sends `If-None-Match` with the prior ETag and reuses the cached body on `304`, yielding the same result with exactly two upstream requests. - `DifferentTokenDoesNotShareCache` — a different token never sends another token's cached ETag. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4959e1f9-f8e6-4e97-a487-f395a0123c79 |
||
|
Susana Ferreira
|
7b104b6a98 |
fix: add info log level to unpriced models message (#27693)
Change the unpriced models log to info level. Refs https://github.com/coder/coder/pull/27678 |
||
|
|
3f3fd1c4d7 |
feat: show network request summary on AI session detail card (#27418)
Frontend for the AI session network summary. Adds Network calls, Blocked network requests, and Top domains rows to the Session summary card on the individual AI session detail page, driven by the network fields on the session threads response. Renders "Disabled" when network monitoring was not active and "No activity" when there were no calls. Covered by Storybook stories for each state. ### PR map (merge strictly bottom-up) This change is a 4-PR stack. Each PR depends on all the ones below it, so merge in this exact order: 1. #27417 — backend network summary 2. #27418 — frontend summary rows 3. #27425 — backend per-call list `network_call_logs` 4. #27426 — frontend network-calls panel Refs AIGOV-463 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: Cian Johnston <cian@coder.com> |