_Disclaimer: implemented by a Coder Agent using Claude Opus 4.6._
Marks the injected MCP approach in AI Bridge as deprecated across the
codebase.
## Changes
- **`codersdk/deployment.go`**: Deprecated `ExternalAuthConfig.MCPURL`,
`.MCPToolAllowRegex`, `.MCPToolDenyRegex` fields; deprecated and hid the
`--aibridge-inject-coder-mcp-tools` server flag; deprecated
`AIBridgeConfig.InjectCoderMCPTools`.
- **`coderd/externalauth/externalauth.go`**: Deprecated `Config.MCPURL`,
`.MCPToolAllowRegex`, `.MCPToolDenyRegex`.
- **`enterprise/aibridgedserver/aibridgedserver.go`**: Added runtime
deprecation warning when `CODER_AIBRIDGE_INJECT_CODER_MCP_TOOLS` is
enabled; deprecated `getCoderMCPServerConfig`.
- **`enterprise/aibridged/mcp.go`**: Deprecated `MCPProxyBuilder`
interface and `MCPProxyFactory` struct.
- **`docs/ai-coder/ai-bridge/mcp.md`**: Added deprecation warning
banner.
_Disclaimer: implemented with Opus 4.6 and Coder Agents._
Follow-up to #22879.
## Problem
The `CODER_SESSION_TOKEN` guard added in #22879 blocks `coder login`
unconditionally when the env var is set. This conflicts with
`--use-token-as-session`, which intentionally uses the provided token
(including from the env var) directly as the session token.
## Fix
Add `&& !useTokenForSession` to the check so that `coder login
--use-token-as-session` still works when `CODER_SESSION_TOKEN` is set.
## Testing
Added `TestLogin/SessionTokenEnvVarWithUseTokenAsSession` — sets the env
var with a valid token and passes `--use-token-as-session`, verifying
login succeeds.
---------
Signed-off-by: Danny Kopping <danny@coder.com>
_Disclaimer: created with Opus 4.6 and Coder Agents._
## Problem
When `CODER_SESSION_TOKEN` is set as an environment variable with an
invalid value, `coder login` fails with a confusing error:
```
error: Trace=[create api key: ]
You are signed out or your session has expired. Please sign in again to continue.
Suggestion: Try logging in using 'coder login'.
```
The suggestion to run `coder login` is what the user just did, making it
circular and unhelpful.
## Root cause
The `--token` flag is mapped to `CODER_SESSION_TOKEN` via serpent. When
the env var is set, `coder login` picks it up as the session token and
tries to use it to create a new API key, which fails because the token
is invalid. Even if login were to succeed and write a new token to disk,
subsequent commands would still use the env var (which takes precedence
over the on-disk token), so the user would remain stuck.
## Fix
Before attempting login, check if `CODER_SESSION_TOKEN` is set in the
environment. If so, return a clear error telling the user to unset it:
```
the environment variable CODER_SESSION_TOKEN is set, which takes precedence
over the session token stored on disk. Please unset it and try again.
unset CODER_SESSION_TOKEN
```
## Testing
Added `TestLogin/SessionTokenEnvVar` that verifies the error is returned
when the env var is set.
Adds database columns and server-side logic to track interception lineage via tool call IDs. When an interception ends, the server resolves the correlating tool call ID to find the parent interception and links them via `parent_id`.
New `provider_tool_call_id` column on `aibridge_tool_usages` and `parent_id` column on `aibridge_interceptions`, with indexes for lookup. `findParentInterceptionID` queries by tool call ID and filters out the current interception to find the parent.
Adapted from the [coder/coder `dk/prompt_provenance_poc`](https://github.com/coder/coder/compare/main...dk/prompt_provenance_poc) branch.
Depends on [coder/aibridge#188](https://github.com/coder/aibridge/pull/188).
Closes https://github.com/coder/internal/issues/1334
Adds database columns and server-side logic to track interception lineage via tool call IDs. When an interception ends, the server resolves the correlating tool call ID to find the parent interception and links them via `parent_id`.
New `provider_tool_call_id` column on `aibridge_tool_usages` and `parent_id` column on `aibridge_interceptions`, with indexes for lookup. `findParentInterceptionID` queries by tool call ID and filters out the current interception to find the parent.
Adapted from the [coder/coder `dk/prompt_provenance_poc`](https://github.com/coder/coder/compare/main...dk/prompt_provenance_poc) branch.
Depends on [coder/aibridge#188](https://github.com/coder/aibridge/pull/188).
Closes https://github.com/coder/internal/issues/1334
The comments generated are too noisy and not of sufficiently high signal
that we should automatically opt every PR in.
This PR moves the trigger to the `code-review` label _only_.
Signed-off-by: Danny Kopping <danny@coder.com>
On Windows, `pty.New()` was creating a `ConPTY` (`PseudoConsole`) even
when no process would be attached. `ConPTY` requires a real process to
function correctly - without one, the pipe handles become invalid
intermittently, causing flaky test failures like `read |0: The handle is
invalid.`
This affected tests using the `ptytest.New()` + `Attach()` pattern for
in-process CLI testing.
The fix splits Windows PTY creation into two paths:
- `newPty()` now returns a simple pipe-based PTY for the `Attach()` use
case
- `newConPty()` creates a real `ConPTY`, called by `Start()` when a
process will be attached
AFAICT this will result in no change in behaviour outside of tests.
Fixescoder/internal#1277
_Disclaimer: investigated and implemented by Claude Opus 4.5, reviewed
by me._
---------
Signed-off-by: Danny Kopping <danny@coder.com>
Liveness checks are currently causing pods to be killed during
long-running migrations.
They are generally not advisable for our workloads; if a pod becomes
unresponsive we _need_ to know about it (due to a deadlock, etc) and not
paper over the issue by killing the pod.
I've also made all probe settings configurable.
---------
Signed-off-by: Danny Kopping <danny@coder.com>
Operators need to know which API key was used in HTTP requests.
For example, if a key is leaking and a DDOS is underway using that key, operators need a way to identify the key in use and take steps to expire the key (see https://github.com/coder/coder/issues/21782).
_Disclaimer: created using Claude Opus 4.5_
Closes https://github.com/coder/internal/issues/1167
Previously we were checking that start != end time; this was flaking on
Windows.
On Windows, `time.Now()` has limited resolution (~1ms with Go runtime's
`timeBeginPeriod`, or ~15.6ms in default system resolution). When two
`time.Now()` calls execute within the same clock tick, they return
identical timestamps, causing `StartedAt.Before(EndedAt)` to return
`false`.
**References:**
- [Go issue #8687](https://github.com/golang/go/issues/8687) - Windows
system clock resolution issue
- [Go issue #67066](https://github.com/golang/go/issues/67066) -
time.Now precision on Windows (still open)
Instead, we're changing the assertion to (the more semantically correct)
"end not before start".
A possible future enhancement could be to plumb coder/quartz through the
recording mechanism, but it's unnecessary for now.
Signed-off-by: Danny Kopping <danny@coder.com>
Usage example:
```bash
$ make test TEST_CPUPROFILE=cpu.prof TEST_MEMPROFILE=mem.prof TEST_PACKAGES=./coderd
```
Note that `TEST_PACKAGES` has to be specified, otherwise you get `cannot
use -{cpu,memory}profile flag with multiple packages`.
Signed-off-by: Danny Kopping <danny@coder.com>
Closes https://github.com/coder/coder/issues/21360
A few considerations/notes:
- I've kept the number of conns to 10 in all other places, except coderd
- which uses the config value
- I opted to also make idle conns configurable; the greater the delta
between max open and max idle, the more connection churn
- Postgres maintains a [_process_ per
connection](https://www.postgresql.org/docs/current/connect-estab.html),
contrary to what the comment said previously
- Operators should be able to tune this, since process churn can
negatively affect OS scheduling
- I've set the value to `"auto"` by default so it's not another knob one
_has to_ twiddle, and sets max idle = max conns / 3
---------
Signed-off-by: Danny Kopping <danny@coder.com>
With this low upper bound, the cache thrashes under load (i.e. cache
entries are replaced too quickly), leading to audit records not
persisting in time before the context is canceled (see `OnEvict`
behaviour).
The TTL remains 15m because we need to keep MCP connections relatively
fresh, but this TTL is irrelevant if injected tools are not used.
This was an oversight; the limit should never have been set so low. 5000
is likely so large that the cache will never fill up; in future we
should make this configurable if customers run into issues. It's a bit
difficult right now to determine how much real memory each element
_actually_ uses, but even if it's a crazy number like 100KiB per
instance then it'll only use 500MiB.
Signed-off-by: Danny Kopping <danny@coder.com>
_Disclaimer: investigation done by Claude Opus 4.5_
Closes https://github.com/coder/internal/issues/1173
Closes https://github.com/coder/internal/issues/1174
The agent containers API is only marked "ready" under this condition in
`agent/agentcontainers/api.go`:
```go
// For now, all endpoints require the initial update to be done.
// If we want to allow some endpoints to be available before
// the initial update, we can enable this per-route.
```
However, what was actually being checked for was that the _init_ was
done, not the _initial update_.
In agent/agentcontainers/api.go, the `Start()` method:
1. Called `Init()` which closed `initDone` <--- API marked ready here
2. Then launched `go api.updaterLoop()` asynchronously
3. `updaterLoop()` performs the initial container update <--- should
have marked it ready after this
This PR fixes these semantics to avoid the race which was causing the
above two flakes.
Signed-off-by: Danny Kopping <danny@coder.com>
Closes https://github.com/coder/internal/issues/1173,
https://github.com/coder/internal/issues/1174
Currently these two tests are flaky because the contexts were created
before a potentially long-running process. By the time the context was
actually used, it may have timed out - leading to confusion.
Additionally, the `ExpectMatch` calls were not using the test context -
but rather a background context. I've marked that func as deprecated
because we should always tie these to the test context.
Special thanks to @mafredri for the brain probe 🧠
---------
Signed-off-by: Danny Kopping <danny@coder.com>
`prometheus.provisionerd_server_metrics: unsupported workspace timing
flags` appears in the logs, but without knowledge of the available flags
it's not possible to troubleshoot this.
Signed-off-by: Danny Kopping <danny@coder.com>
Currently, when AI Bridge is enabled AND the `oauth2` and
`mcp-server-http` experiments are enabled we inject Coder's MCP tools
into all intercepted AI Bridge requests.
This PR introduces a config to control this behaviour.
**NOTE:** this is a backwards-incompatible change; previously these
tools would be injected automatically, now this setting will need to be
explicitly enabled.
---------
Signed-off-by: Danny Kopping <danny@coder.com>
Allow for conditionally disabling AI Bridge to investigate occasional
issues.
When AI Bridge is disabled, a static credential will be configured for
Claude.
AI Bridge is still enabled by default.
Signed-off-by: Danny Kopping <danny@coder.com>
Audit Log entries can be deleted safely (with appropriate caveats), but
we don't specifically call this out in the docs.
---------
Signed-off-by: Danny Kopping <danny@coder.com>
v0.41.0 seems to have a bug where it passes nil for tool definitions
instead of an empty map like it used to.
Signed-off-by: Danny Kopping <danny@coder.com>
The input schemas of `coder_workspace_edit_file` and
`coder_workspace_edit_files` were violating the JSON Schemas of both
Anthropic and OpenAI.
Follow-up: we should add a test to ensure future compatibility with
these specs.
Signed-off-by: Danny Kopping <danny@coder.com>
Adds telemetry for a _global_ account of prebuilt workspaces created,
failed to build, and claimed.
Partitioning this data by template/preset tuple is not currently in
scope.
---------
Signed-off-by: Danny Kopping <dannykopping@gmail.com>
We probably shouldn't be suggesting `ignore_changes = all`. Only the
attributes which cause drift in prebuilds should be ignored; everything
else can behave as normal.
---------
Signed-off-by: Danny Kopping <dannykopping@gmail.com>
Co-authored-by: Edward Angert <EdwardAngert@users.noreply.github.com>
Publishing inside a db transaction can lead to database connection
starvation/contention since it requires its own connection.
This ruleguard rule (one-shotted by Claude Sonnet 3.7 and finalized by
@Emyrk) will detect two of the following 3 instances:
```go
type Nested struct {
ps pubsub.Pubsub
}
func TestFail(t *testing.T) {
t.Parallel()
db, ps := dbtestutil.NewDB(t)
nested := &Nested{
ps: ps,
}
// will catch this
_ = db.InTx(func(_ database.Store) error {
_, _ = fmt.Printf("")
_ = ps.Publish("", []byte{})
return nil
}, nil)
// will catch this
_ = db.InTx(func(_ database.Store) error {
_ = nested.ps.Publish("", []byte{})
return nil
}, nil)
// will NOT catch this
_ = db.InTx(func(_ database.Store) error {
blah(ps)
return nil
}, nil)
}
func blah(ps pubsub.Pubsub) {
ps.Publish("", []byte{})
}
```
The ruleguard doesn't recursively introspect function calls so only the
first two cases will be guarded against, but it's better than nothing.
<img width="1444" alt="image"
src="https://github.com/user-attachments/assets/8ffa0d88-16a0-41a9-9521-21211910dec9"
/>
---------
Signed-off-by: Danny Kopping <dannykopping@gmail.com>
Co-authored-by: Steven Masley <stevenmasley@gmail.com>
These items came up in an internal "bug bash" session yesterday.
@EdwardAngert note: I've reverted to the "transparent" phrasing; the
current docs confused a couple folks yesterday, and I feel that
"transparent" is clearly understood in this context.
---------
Signed-off-by: Danny Kopping <dannykopping@gmail.com>
Co-authored-by: Edward Angert <EdwardAngert@users.noreply.github.com>
`v1.5` is going out with release `v2.22`
I had to reorder `module_files` and `resource_replacements` because of
this.
---------
Signed-off-by: Danny Kopping <dannykopping@gmail.com>
Closes https://github.com/coder/internal/issues/369
We can't know whether a replacement (i.e. drift of terraform state
leading to a resource needing to be deleted/recreated) will take place
apriori; we can only detect it at `plan` time, because the provider
decides whether a resource must be replaced and it cannot be inferred
through static analysis of the template.
**This is likely to be the most common gotcha with using prebuilds,
since it requires a slight template modification to use prebuilds
effectively**, so let's head this off before it's an issue for
customers.
Drift details will now be logged in the workspace build logs:
Plus a notification will be sent to template admins when this situation
arises:
A new metric - `coderd_prebuilt_workspaces_resource_replacements_total`
- will also increment each time a workspace encounters replacements.
We only track _that_ a resource replacement occurred, not how many. Just
one is enough to ruin a prebuild, but we can't know apriori which
replacement would cause this.
For example, say we have 2 replacements: a `docker_container` and a
`null_resource`; we don't know which one might
cause an issue (or indeed if either would), so we just track the
replacement.
---------
Signed-off-by: Danny Kopping <dannykopping@gmail.com>
`Collect()` is called whenever the `/metrics` endpoint is hit to
retrieve metrics.
The queries used in prebuilds metrics collection are quite heavy, and we
want to avoid having them running concurrently / too often to keep db
load down.
Here I'm moving towards a background retrieval of the state required to
set the metrics, which gets invalidated every interval.
Also introduces `coderd_prebuilt_workspaces_metrics_last_updated` which
operators can use to determine when these metrics go stale.
See https://github.com/coder/coder/pull/17789 as well.
---------
Signed-off-by: Danny Kopping <dannykopping@gmail.com>
The changes in `coder/preview` necessitated the changes in
`codersdk/richparameters.go` & `provisioner/terraform/resources.go`.
---------
Signed-off-by: Danny Kopping <dannykopping@gmail.com>
Co-authored-by: Steven Masley <stevenmasley@gmail.com>
Database transactions hold onto connections, and `pubsub.Publish` tries
to acquire a connection of its own. If the latter is called within a
transaction, this can lead to connection exhaustion.
I plan two follow-ups to this PR:
1. Make connection counts tuneable
https://github.com/coder/coder/blob/main/cli/server.go#L2360-L2376
We will then be able to write tests showing how connection exhaustion
occurs.
2. Write a linter/ruleguard to prevent `pubsub.Publish` from being
called within a transaction.
---------
Signed-off-by: Danny Kopping <dannykopping@gmail.com>
Currently we don't have a way to get insight into Postgres connections
being exhausted.
By using the prometheus' [`DBStats`
collector](https://github.com/prometheus/client_golang/blob/main/prometheus/collectors/dbstats_collector.go),
we get some insight out-of-the-box.
```
# HELP go_sql_idle_connections The number of idle connections.
# TYPE go_sql_idle_connections gauge
go_sql_idle_connections{db_name="coder"} 1
# HELP go_sql_in_use_connections The number of connections currently in use.
# TYPE go_sql_in_use_connections gauge
go_sql_in_use_connections{db_name="coder"} 2
# HELP go_sql_max_idle_closed_total The total number of connections closed due to SetMaxIdleConns.
# TYPE go_sql_max_idle_closed_total counter
go_sql_max_idle_closed_total{db_name="coder"} 112
# HELP go_sql_max_idle_time_closed_total The total number of connections closed due to SetConnMaxIdleTime.
# TYPE go_sql_max_idle_time_closed_total counter
go_sql_max_idle_time_closed_total{db_name="coder"} 0
# HELP go_sql_max_lifetime_closed_total The total number of connections closed due to SetConnMaxLifetime.
# TYPE go_sql_max_lifetime_closed_total counter
go_sql_max_lifetime_closed_total{db_name="coder"} 0
# HELP go_sql_max_open_connections Maximum number of open connections to the database.
# TYPE go_sql_max_open_connections gauge
go_sql_max_open_connections{db_name="coder"} 10
# HELP go_sql_open_connections The number of established connections both in use and idle.
# TYPE go_sql_open_connections gauge
go_sql_open_connections{db_name="coder"} 3
# HELP go_sql_wait_count_total The total number of connections waited for.
# TYPE go_sql_wait_count_total counter
go_sql_wait_count_total{db_name="coder"} 28
# HELP go_sql_wait_duration_seconds_total The total time blocked waiting for a new connection.
# TYPE go_sql_wait_duration_seconds_total counter
go_sql_wait_duration_seconds_total{db_name="coder"} 0.086936235
```
`go_sql_wait_count_total` is the metric I'm most interested in gaining,
but the others are also very useful.
Changing the prefix is easy (`prometheus.WrapRegistererWithPrefix`), but
getting rid of the `go_` segment is not quite so easy. I've kept the
changeset small for now.
**NOTE:** I imported a library to determine the database name from the
given conn string. It's [not as
simple](https://www.postgresql.org/docs/current/libpq-connect.html#LIBPQ-CONNSTRING)
as one might hope. The database name is used for the `db_name` label.
---------
Signed-off-by: Danny Kopping <dannykopping@gmail.com>
We replace timestamps in our golden files to keep the values constant.
However, if a non-UTC timezone is used then the timestamp will still be
replaced but the whitespace will be messed up (since it was aligned to
the original value).
Therefore we must force a timezone when generating golden files.
Signed-off-by: Danny Kopping <dannykopping@gmail.com>
Pre-requisite for https://github.com/coder/coder/pull/16891
Closes https://github.com/coder/internal/issues/515
This PR introduces a new concept of a "system" user.
Our data model requires that all workspaces have an owner (a `users`
relation), and prebuilds is a feature that will spin up workspaces to be
claimed later by actual users - and thus needs to own the workspaces in
the interim.
Naturally, introducing a change like this touches a few aspects around
the codebase and we've taken the approach _default hidden_ here; in
other words, queries for users will by default _exclude_ all system
users, but there is a flag to ensure they can be displayed. This keeps
the changeset relatively small.
This user has minimal permissions (it's equivalent to a `member` since
it has no roles). It will be associated with the default org in the
initial migration, and thereafter we'll need to somehow ensure its
membership aligns with templates (which are org-scoped) for which it'll
need to provision prebuilds; that's a solution we'll have in a
subsequent PR.
---------
Signed-off-by: Danny Kopping <dannykopping@gmail.com>
Co-authored-by: Sas Swart <sas.swart.cdk@gmail.com>
Danny Kopping