mirror of
https://github.com/coder/coder.git
synced 2026-10-28 02:42:54 +08:00
011914bb142a4d67fa7d7618c145ffd3ffceb701
100
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Thomas Kosiewski
|
9448624d2d | feat(site): add Opus 4.8 known model (#25839) | ||
|
Thomas Kosiewski
|
e32be68687 | fix(dogfood/coder): verify Homebrew installer (#25721) | ||
|
Thomas Kosiewski
|
bfa17c315e | fix(dogfood/coder): persist mise user installs (#25720) | ||
|
Thomas KosiewskiandClaude Opus 4.7
|
e9f0f81d76 |
fix(dogfood/coder): override mise oci build MISE_CONFIG_DIR bake (#25684)
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Thomas Kosiewski <tk@coder.com> |
||
|
Thomas KosiewskiandClaude Opus 4.7
|
8a2f28fa6a |
fix(dogfood/coder): fix install-deps heredoc and /opt/mise ownership (#25678)
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Thomas Kosiewski <tk@coder.com> |
||
|
Thomas KosiewskiandClaude Opus 4.7
|
51836e681e |
refactor: build dogfood image as base + mise oci layers (#25448)
Splits the dogfood image into two artifacts: - `ghcr.io/coder/oss-dogfood-base:<distro>-<base-sha>`: Ubuntu base with apt packages, chrome, rustup, brew, gh, and the mise binary. The base-sha is a cache key over `Dockerfile.base` and `files/`, so commits that don't touch those inputs reuse the previous build. - `codercom/oss-dogfood:<final-sha>-<distro>` and rolling tags (`:22.04`, `:26.04`, `:latest`, `:<branch>`): produced by `mise oci build` on top of the base, with one content-addressed OCI layer per mise tool. The rolling tag scheme is unchanged, so the workspace template doesn't need updating. Single-tool version bumps now invalidate only that tool's OCI layer, so workspaces re-pull just what changed instead of the entire 5-6 GB image on every recreate. Also: - Drops the build-time `pnpm dlx playwright@1.47.0 install --with-deps chromium` step (~400 MB) and the equivalent `playwright-driver.browsers` install from `flake.nix`. `@playwright/mcp` (used by the claude-code and codex MCP servers in `dogfood/coder/main.tf`) does NOT auto-install browsers, so the existing `install-deps` `coder_script` now runs two installs on workspace start: `pnpm exec playwright install chromium` for the site's pinned `@playwright/test`, and `npx --package=@playwright/mcp@latest playwright-core install --no-shell chromium` so the MCP servers find their matching browser revision. Browser revisions coexist under `~/.cache/ms-playwright/chromium-<rev>/`, which lives on the home volume so both downloads happen once per workspace recreate and persist across restarts. Net effect: same MCP behavior as before, +~1-2 min on first workspace start. Nix devshell users running site e2e tests locally now need `pnpm exec playwright install` once (instead of getting browsers via nixpkgs). - Bumps the pinned mise binary to v2026.5.12 (matching main after #25521) and adds top-level `min_version = "2026.5.12"` to `mise.toml` so every consumer (devs, CI, the embedded mise inside the dogfood image, mise oci builds) fails fast on an older mise. - Adds bison, flex, libicu-dev, libreadline-dev, uuid-dev, and zlib1g-dev to both Ubuntu base images for source-build use cases (e.g., building Postgres from source). - Replaces skopeo with crane as the registry client `mise oci push` shells out to: crane is added to `mise.toml`, the workflow drops its `apt-get install skopeo` and forces `--tool crane`, and the local wrapper image stops bundling skopeo. One source of truth for tool versions, no apt drift, smaller wrapper image, and workspace users get a registry client on PATH for free via mise oci's tool layers. - Removes `nix.hash`/`mise.hash` and their Makefile rules. The registry digest already captures every effective change since CI rebuilds when any baked-in input moves; the per-file `filesha1()` entries in `pull_triggers` are redundant. Supersedes #25400 (the `mise.hash` pull trigger landed there in `2b612abe7b`; this PR removes it as part of the broader simplification). > [!NOTE] > `mise oci build` is experimental and requires `MISE_EXPERIMENTAL=1` (set at job level in the workflow). The local-only `scripts/dogfood/mise-oci-wrapper.sh` builds a tiny `coderdev/mise-oci-wrapper:<version>` Debian image with curl-installed mise on first invocation (cached by version tag thereafter); we don't reuse `jdxcode/mise:latest` because that tag lags upstream GitHub releases by days and would defeat the `min_version` enforcement above. > [!NOTE] > `compute-base-sha.sh` and `compute-final-sha.sh` are cache keys, not strict content addresses: the base Dockerfile still pulls dynamic resources at build time (gh/buildx `releases/latest`, chrome `stable_current_amd64.deb`, apt mirror state). Two runs with identical checked-in files can produce slightly different bytes, which is acceptable here because the cache-hit savings on irrelevant commits outweigh that drift. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Signed-off-by: Thomas Kosiewski <tk@coder.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> |
||
|
Thomas Kosiewski
|
26a0805dcd |
fix(cli): isolate root HTTP transports (#25430)
The CLI root client shared `http.DefaultTransport` for normal API requests and for the version-check build-info request. In parallel tests, other clients can close idle connections on that process-global transport, which can fail the Boundary license check before the AGPL 404 handling runs. `TestBoundaryLicenseVerification/AGPLDeployment` configures a proxy that returns `404` from `/api/v2/entitlements`, which `verifyLicense()` maps to the expected AGPL deployment error. However, `clitest.SetupConfig()` only writes the URL and session token to disk. It does not pass the test's isolated `proxyClient.HTTPClient` into the CLI invocation, so `coder boundary` builds a fresh client through `RootCmd.InitClient()`. Before this change, that fresh client used `http.DefaultTransport`; if another parallel test closed idle connections on the shared transport while the entitlement request was in flight, Go returned `http: CloseIdleConnections called` instead of the proxy's `404`. The command then failed with `failed to get entitlements`, and the test never reached the expected AGPL error path. Clone the default transport for each CLI root HTTP client and for the unwrapped build-info client, preserving the configured TLS settings when present. Each CLI invocation now gets its own transport instance, so cleanup from unrelated parallel tests cannot interrupt its entitlement or build-info requests. Closes https://github.com/coder/internal/issues/1538 <details> <summary>Coder Agents notes</summary> Generated by Coder Agents for Linear ENG-2705. Local validation: - `go test ./cli -run 'TestNewHTTPTransport|Test_ensureTLSConfig|Test_wrapTransportWithVersionCheck' -count=1` - `go test ./enterprise/cli -run TestBoundaryLicenseVerification/AGPLDeployment -count=20 -parallel=16` - `go test ./cli ./enterprise/cli` - `make lint` - `go test ./enterprise/cli -run '^TestBoundaryLicenseVerification$' -count=50 -parallel=16` - pre-commit hook during `git commit` </details> |
||
|
Thomas KosiewskiandClaude Opus 4.7
|
70ab2b9940 |
chore: switch helm to aqua backend and bump dogfood mise (#25521)
mise's aqua plugin now templates `{{.Version}}` with the leading `v`, so
the official `get.helm.sh` URLs resolve without the hand-rolled `http:`
workaround that was added when the version-prefix templating was broken.
Drops the templated URL block (and its explanatory comment) in favor of
plain `helm = "3.21.0"`. The regenerated lockfile picks up windows-amd64
(which the old workaround intentionally omitted) and replaces the
locally-computed blake3 checksum with the upstream sha256 sums.
Follow-up to #25520, which bumped the pinned version to 3.21.
---------
Signed-off-by: Thomas Kosiewski <tk@coder.com>
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
||
|
Thomas KosiewskiandClaude Opus 4.7
|
7a985f8830 |
fix(dogfood): chown /etc/mise to coder so mise can update lockfile (#25431)
Mise writes a temp file like `/etc/mise/.mise.lock.XXXXXX` and renames it onto `mise.lock` for atomic updates, which requires write access to the parent directory. `/etc/mise` was previously root-owned (`install --directory --mode=0755` and `COPY` without `--chown` default to root), so any mise command that updated the lockfile failed for the coder user: ``` mise ERROR failed to update lockfiles mise ERROR Permission denied (os error 13) at path "/etc/mise/.mise.lock.HbuLAN" ``` Chown `/etc/mise` and the baked `config.toml` / `mise.lock` to `coder:coder`, matching how `/opt/mise` is already set up. The dogfood image is single-user, and mise is expected to update its own lockfile when the coder user installs new tools. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Signed-off-by: Thomas Kosiewski <tk@coder.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> |
||
|
Thomas KosiewskiandClaude Opus 4.7
|
2b612abe7b |
feat: trigger image pull on mise.toml or mise.lock changes (#25400)
The dogfood Dockerfiles consume the repo-root `mise.toml` and `mise.lock` at build time (see `.dockerignore` allowlist), but the template's `pull_triggers` list ignored them, so mise-only changes (tool bumps, new tools) didn't roll out to existing workspaces. Mirror the `nix.hash` pattern: a Makefile rule writes the sha256 of both files into `dogfood/coder/mise.hash`, and `main.tf` hashes that in-module file via `filesha1`. Run `make dogfood/coder/mise.hash` after editing `mise.toml`/`mise.lock`. Signed-off-by: Thomas Kosiewski <tk@coder.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> |
||
|
Thomas KosiewskiandClaude Opus 4.7
|
130ab40b97 |
fix(mise.toml): scope CGO_ENABLED to sqlc install (#25397)
Follow-up to #25387. The top-level `[env] CGO_ENABLED = "1"` was re-exported through every mise shim at runtime, forcing cgo on for `go build` calls and breaking cross-compilation of coderd (slim builds, `./scripts/develop.sh`) on the dogfood image. `scripts/build_go.sh` sets `CGO_ENABLED=0` explicitly for non-boringcrypto builds, but the mise shim overwrites it back to `1` before `go` runs. Scope the variable to sqlc's `install_env` so it only applies during the one `go install` that needs it. Ref: https://mise.en.dev/configuration.html#tools-dev-tools Signed-off-by: Thomas Kosiewski <tk@coder.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> |
||
|
Thomas KosiewskiandClaude Opus 4.7
|
6d7fb07f4c |
feat: bake mise tools into a shared dir on dogfood image (#25387)
Three changes to make mise-managed tooling reach every dogfood workspace cleanly, with the upstream `devcontainers-cli` module fix as the original trigger. ## Why the module breaks The upstream [`devcontainers-cli` coder module](https://github.com/coder/registry/blob/main/registry/coder/modules/devcontainers-cli/run.sh) does `npm install -g @devcontainers/cli` and then verifies the binary is on `PATH`. With mise-managed Node (introduced in #25282), `npm install -g` lands the binary at `$MISE_DATA_DIR/installs/node/<ver>/bin/`, which is *not* on `PATH` and which `mise reshim` does not surface as a shim. The post-install check fails: ``` Installing @devcontainers/cli using npm... changed 1 package in 661ms Reshimming mise 26... Installation completed but 'devcontainer' command not found in PATH ``` Even though nothing the user does is actually broken. ## What this PR does 1. **`mise.toml`** — pre-install `@devcontainers/cli` via mise's `npm:` backend (`npm:@devcontainers/cli = "0.87.0"`). The mise shim lands at `$MISE_DATA_DIR/shims/devcontainer`, on `PATH`. The upstream module's `run.sh` short-circuits on its `command -v devcontainer` check and exits 0 without ever running the broken npm-install path. Strictly redundant after fix the second point makes `npm i -g` work natively, but kept for build-time pre-install and pinned-version reasons matching the other mise-pinned CLIs. 2. **`dogfood/coder/ubuntu-*.04/Dockerfile`** — set `NPM_CONFIG_PREFIX=/home/coder/.npm-global` and prepend `/home/coder/.npm-global/bin` to `PATH`. With this, generic `npm install -g <pkg>` (prettier, biome, anything frontend folks reach for) lands in a stable home-volume dir that is already on `PATH`, survives node version bumps, and needs no `mise reshim`. The mise `npm:` backend keeps using its own `--prefix` internally so the `npm:@devcontainers/cli` pin still installs under `$MISE_DATA_DIR` as before. 3. **`dogfood/coder/ubuntu-*.04/Dockerfile`** — install image tools into `/opt/mise/data` at build time (owned by `coder`) and expose them at runtime via `MISE_SHARED_INSTALL_DIRS=/opt/mise/data/installs`, keeping `MISE_DATA_DIR=/home/coder/.local/share/mise` for the user's own installs. This decouples baked tool versions from the home volume's copy-on-first-mount: fresh and existing workspaces both immediately see the image's tool set without a `mise install` step, and the user's own `mise install <tool>` / `mise use --global` still lands on the home volume. The `/opt/mise/data/shims` dir trails the user shim dir on `PATH` so a user-installed version wins when both exist. Pinned to `0.87.0` (current latest) so Renovate/Dependabot can bump deliberately, matching the policy applied to the other floating tools during the mise migration (`lazygit`, `doctl`, `jj`, `typos`, `watchexec`). --------- Signed-off-by: Thomas Kosiewski <tk@coder.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> |
||
|
Thomas KosiewskiandClaude Opus 4.7
|
5f9b3220b5 |
chore: install dogfood image tooling via mise.toml (#25282)
This PR replaces the hand-rolled `curl | tar | go install | cargo install` chains in the dogfood Ubuntu 22.04 and 26.04 Dockerfiles with a single `mise install` driven by a new repo-root `mise.toml`. The previous Dockerfiles installed ~25 CLIs across three multi-stage builds with versions hardcoded inline. Version bumps were scattered across the Dockerfiles, the root `mise.toml` (added in #24618 but otherwise unused at runtime), and CI's setup actions; build-time network failures came from a dozen distinct endpoints; and `mise` itself sat in the image with no manifest to install from. The new flow: - The repo's `mise.toml` is the single source of truth for image tool versions. The Dockerfiles `COPY` it to `/etc/mise/config.toml` and run a single `mise install` as the `coder` user. - Tools are installed into `/opt/mise/data` rather than the default `/home/coder/.local/share/mise`, so they live in the image (not on the persistent home volume) and reach every workspace on recreate. - Build context moves to the repo root so the Dockerfile can `COPY mise.toml`; an allowlist `.dockerignore` keeps the transferred context to ~24 kB. - Optional `--secret id=github_token` plumbing through the Makefile and `.github/workflows/dogfood.yaml` lifts aqua's GitHub API quota from 60/hr unauthenticated to 1000/hr with `secrets.GITHUB_TOKEN`. - `MISE_TRUSTED_CONFIG_PATHS=/home/coder:/etc/mise` is set as an ENV so users who clone the coder repo into their workspace home aren't prompted to `mise trust`. Net diff for the two Ubuntu Dockerfiles: -399 / +244 lines (~200 lines shorter each). The `FROM rust-utils`, `FROM go`, and `FROM proto` multi-stage builds are gone; so are the NVM/Node block, the bulk binary-install block (golangci-lint, helm, kubectx, syft, cosign, bun), the gh `.deb`/lazygit/doctl tarball installs, the gofmt `update-alternatives` line, and the `yq`→`yq4` rename (`scripts/lib.sh:267-275` already auto-detects either name). Both images were built and smoke-tested with Apple's `container` CLI on macOS — every migrated tool resolves to the expected pinned version including outside the cloned coder repo (e.g. `gh` from `/home/coder`, matching the workspace startup script in `dogfood/coder/main.tf`), `sqlc` runs (proving `CGO_ENABLED=1` was honoured at install), `yq --version` reports v4 for `scripts/lib.sh`'s detection, and `gofmt` resolves via the mise shim. Follow-ups (out of scope here): - Commit a multi-platform `mise.lock` so `gh = "latest"` and the other floating versions resolve deterministically across rebuilds and dev machines. - Migrate CI's `setup-go` / `setup-node` actions to consume `mise.toml` so image and CI versions stop being able to drift. --------- Signed-off-by: Thomas Kosiewski <tk@coder.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> |
||
|
Thomas KosiewskiandClaude Opus 4.7
|
96ea2465b7 |
build(coderd/database/gen/dump): fall back to embedded postgres without docker (#25332)
Generating `coderd/database/dump.sql` previously required a Docker-compatible socket via `ory/dockertest`. Contributors using runtimes that don't expose one (e.g. Apple's `container` CLI) hit a panic during `make gen`: ``` build: panic: open containerized database failed: open container: could not start resource: dial unix /var/run/docker.sock: connect: no such file or directory ``` Fall back to `fergusstrange/embedded-postgres` (already a direct module dep, used by `scripts/develop/dbrecovery.go`) when `dbtestutil.OpenContainerized` fails. The server's timezone is forced to UTC so `timestamptz` DEFAULT expressions canonicalize identically to the Docker-based path; otherwise the host's local TZ leaks into the dump as values like `'0001-12-31 23:06:32+00 BC'`. `PGDumpSchemaOnly` still needs `pg_dump` v13.x on PATH (the embedded-postgres archive ships only `initdb`/`postgres`/`pg_ctl`). When neither `pg_dump` nor `docker` is available, the existing error is supplemented with install hints for `mise`, `brew`, and `apt`. CI keeps using the Docker path unchanged; the fallback is local-dev-only and produces a byte-identical `dump.sql`. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Signed-off-by: Thomas Kosiewski <tk@coder.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> |
||
|
Thomas Kosiewski
|
f71bccf53f |
ci(.github/actions/setup-node): verify active Node version (#25143)
Updates the shared setup-node composite action to current Node 24 based releases of `pnpm/action-setup` and `actions/setup-node`. This avoids the deprecated Node 20 action runtime seen in CODAGT-178 while keeping the third-party actions pinned by SHA. Adds an explicit post-setup check that fails inside Setup Node when `node --version` is not `v22.19.0`, so self-hosted runner/toolcache mismatches are surfaced before `pnpm install` reports a dependency engine error. Closes https://github.com/coder/internal/issues/1457 Generated by Coder Agents. |
||
|
Thomas KosiewskiandClaude Opus 4.7
|
b9b8d763e3 |
refactor(site/src/pages/AgentsPage): break AgentChatPage circular dep (#25287)
`AgentChatPageView.tsx` imported `getPersistedSidebarTabId` / `savePersistedSidebarTabId` from `AgentChatPage.tsx`, which already imports `AgentChatPageView`, closing a cycle that `pnpm run lint:circular-deps` reports but doesn't fail on (dpdm defaults to exit code 0; the script is missing `--exit-code circular:1`). Move the three sidebar-tab localStorage helpers and the key prefix into `utils/sidebarTabStorage.ts` alongside `draftStorage.ts` and the other per-chat storage modules. Pure code move, no behavior change. After this change, `pnpm run lint:circular-deps` reports zero cycles. --------- Signed-off-by: Thomas Kosiewski <tk@coder.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> |
||
|
Thomas Kosiewski
|
969da320ec |
feat: export Coder Agents debug logs (#25039)
Adds JSON export actions to the Coder Agents Debug panel so users can download either the current chat's recent debug runs or one expanded run for support sharing. The export reuses the existing chat debug endpoints and react-query cache, adds Storybook and unit coverage for the JSON envelope, and updates the chat debug logging docs with UI and cURL instructions. Refs CODAGT-280. Generated by Coder Agents. <details> <summary>Implementation notes</summary> - Chat-level export fetches full detail for each listed debug run with `queryClient.fetchQuery(chatDebugRun(chatId, run.id))` and writes a single JSON file. - Run-level export uses the already-loaded detail query data from an expanded run card. - The JSON envelope includes `version`, `scope`, `exported_at`, `chat_id`, and either `runs` or `run`. - The chat-level export reflects the current backend list endpoint behavior, up to the 100 newest debug runs. - Agent-browser dogfooding verified files were downloaded and that `jq` validated the chat-level and run-level JSON contents. </details> |
||
|
Thomas Kosiewski
|
38091f1d82 |
fix(site/src/pages/AgentsPage/components/ChatConversation): remove attachment copy actions (#25119)
Messages with chat file attachments showed a `Copy message` action even though the action only copied `parsed.markdown` and omitted the attachment content. Hide the message copy action whenever a parsed message contains file attachments, and add regression coverage for both user and assistant attachment messages. Refs https://linear.app/codercom/issue/CODAGT-344/remove-or-fix-copy-buttons-on-file-attachments Generated by Coder Agents. |
||
|
Thomas Kosiewski
|
5c3b59151e |
feat: add Cmd/Ctrl+Enter send setting (#25062)
Adds an Agents General setting to require Cmd/Ctrl+Enter before sending
chat messages. When enabled, plain Enter inserts a newline in agent chat
inputs while the send button remains available.
The preference is now persisted server-side through
`/api/v2/users/{user}/preferences`, alongside the existing user
preference settings, and is applied to both the create-agent input and
existing chat composer. Storybook and API coverage verify the setting,
keyboard behavior, validation, and persistence.
<details>
<summary>Coder Agents notes</summary>
Generated by Coder Agents from a Slack request. Dogfooded with
agent-browser against the Storybook settings and chat input stories.
</details>
|
||
|
Thomas Kosiewski
|
e56381eb61 |
feat: stream advisor tool output (#25032)
Stream advisor output into the advisor tool card while the nested advisor call is still running. This keeps the advisor implementation intentionally advisor-specific: the parent model still receives the same final structured tool result, while the frontend receives transient `tool-result.result_delta` parts to render partial advisor text in the expanded card. The final persisted chat history remains unchanged. Refs CODAGT-322. Generated by Coder Agents. <details> <summary>Implementation plan</summary> - Publish advisor text deltas from the nested `chatloop.Run` via `RunAdvisorOptions.OnAdviceDelta`. - Forward those deltas through `chatadvisor.Tool` with the parent advisor tool call ID. - Emit transient `ChatMessagePartTypeToolResult` websocket parts with `ResultDelta` from `chatd`. - Add `result_delta` to the generated tool-result TypeScript variant. - Accumulate tool result deltas in frontend stream state and keep the tool running until the final result arrives. - Render streamed advisor advice in the existing advisor card using streaming markdown mode, while retaining the updated advisor UI. </details> |
||
|
Thomas Kosiewski
|
4a6756a3e8 | fix: isolate test HTTP clients (#25038) | ||
|
Thomas Kosiewski
|
273e828442 | fix: remove advisor reasoning configuration (#25030) | ||
|
Thomas Kosiewski
|
10a22a4753 |
fix(site/src/pages/AgentsPage): polish advisor UI (#25002)
Polishes the advisor tool card so the header uses a clearer lightbulb icon, inline pill metadata, wrapped/clamped question text, and a separate advice pill before the rendered response. Adds Storybook coverage for a long advice plus 1.8k-character question peak state, including collapse/expand behavior. Refs https://linear.app/codercom/issue/CODAGT-322/improve-advisor-icon-and-duplicate-loading-ui <details> <summary>Coder Agents disclosure</summary> This PR was generated by Coder Agents. </details> |
||
|
Thomas Kosiewski
|
f605d6bcb4 |
feat(dogfood/coder): add brew and mise to ubuntu images (#24618)
This adds Homebrew and mise to the Ubuntu dogfood images and makes mise shims win PATH resolution for the `coder` user. It installs Homebrew in `/home/linuxbrew/.linuxbrew`, installs the latest mise release (`v2026.4.19`) via its verified GitHub release artifact, exposes mise at `/usr/local/bin/mise`, wires `HOMEBREW_*` and `MISE_DATA_DIR`, and adds build-time checks for both tools. The mise executable target lives in writable `/opt/mise/bin` so `mise self-update` can replace it as the `coder` user. This also adds `libc6-dev` to the Go utility stages so the existing CGO-backed tool installs keep building on newer Ubuntu bases. The dogfood template now mounts a dedicated `/home/linuxbrew/` Docker volume in addition to `/home/coder/`. Fresh volumes are seeded from the image-baked Homebrew tree on first mount, while user-installed formulae persist across workspace container recreation. I revalidated the bootstrap on jammy and resolute base images with fresh mounted `/home/coder` and `/home/linuxbrew` volumes. In those runs, `brew install hello` succeeded, `mise doctor` reported no PATH or activation problems, `mise self-update --force --yes --no-plugins 2026.4.19` succeeded as `coder`, and `mise use --global github:BurntSushi/ripgrep@14.1.1` moved `rg` resolution to the mise shim after container recreation. --- <details> <summary>📋 Implementation Plan</summary> # Plan: add `mise` and Homebrew to the dogfood Ubuntu images with mise-first PATH ## Goal - Make both dogfood Ubuntu images ship `brew` and `mise`. - Ensure `mise doctor` does **not** complain about activation/PATH ordering in the shell entrypoints we support. - Keep the implementation robust against the persistent `/home/coder` volume used by the dogfood template. ## Verified context - The relevant image definitions are: - `dogfood/coder/ubuntu-22.04/Dockerfile` - `dogfood/coder/ubuntu-26.04/Dockerfile` - The dogfood template mounts a persistent home volume at `/home/coder/` in `dogfood/coder/main.tf:840-843`, so required image-baked state should not live only under `/home/coder`. - Both Dockerfiles already manipulate PATH in multiple places: - Go appended early (`:26`) - Cargo prepended (`ubuntu-26.04/Dockerfile:202-206`; mirrored in 22.04) - Node via nvm prepended (`ubuntu-26.04/Dockerfile:245-255`; mirrored in 22.04) - Final `coder` PATH prepends `/home/coder/go/bin` (`ubuntu-26.04/Dockerfile:348-358`; mirrored in 22.04) - `COPY files /` is already present in both Dockerfiles, so adding new global shell-init files is possible without Terraform changes. - `scripts/lib.sh:94-124` uses `command -v` for dependency detection, so PATH order is the practical repo-level behavior we need to control. - `.github/workflows/dogfood.yaml:99-126` builds both Ubuntu variants, and the 22.04 image is still tagged `latest`, so both Dockerfiles must be updated in the same change. ## Recommended implementation ### Phase 1 — Bootstrap Homebrew and `mise` in both Ubuntu Dockerfiles 1. Update both Dockerfiles in parallel: - `dogfood/coder/ubuntu-22.04/Dockerfile` - `dogfood/coder/ubuntu-26.04/Dockerfile` 2. Add the minimum explicit Homebrew prerequisites that are missing from the current apt package set. - The images already install `build-essential`, `curl`, `file`, and `git`. - Audit whether `procps` must be added explicitly for Homebrew’s Linux requirements. 3. Install Homebrew in the supported Linux prefix: - Prefix: `/home/linuxbrew/.linuxbrew` - Keep the install/build logic in the Dockerfile, before `USER coder`. - Make the resulting prefix writable by `coder` before switching users. Prefer the smallest-diff approach that leaves `brew install ...` usable as `coder`. 4. Install `mise` to a stable image-owned path instead of relying on `~/.local/bin`: - Preferred binary path: `/usr/local/bin/mise` - Use a pinned installation method that fits the current Dockerfile style (versioned release asset or otherwise explicitly pinned installer path). 5. Add defensive build-time sanity checks near the install steps so the image fails early if assumptions are wrong: - `test -x /usr/local/bin/mise` - `test -x /home/linuxbrew/.linuxbrew/bin/brew` - `brew --version` - `mise --version` **Quality gate:** both Dockerfiles build locally, and the resulting container can run `brew --version` and `mise --version` as `coder`. ### Phase 2 — Make `mise` win PATH resolution by default 1. After `USER coder` in both Dockerfiles, define stable environment variables for the final shell/runtime behavior: - `HOMEBREW_PREFIX=/home/linuxbrew/.linuxbrew` - `MISE_DATA_DIR=/home/coder/.local/share/mise` - `MISE_ACTIVATE_AGGRESSIVE=1` only if later shell activation proves necessary 2. Replace the final PATH composition so it resolves in this order: 1. `mise` shims 2. Homebrew `bin`/`sbin` 3. Existing `/home/coder/go/bin` 4. Existing image/system PATH 3. Keep the current Go/Rust/Node setup intact aside from the final PATH ordering. Add a short Dockerfile comment explaining that `mise` shims must be first so `mise doctor` and `command -v` resolve `mise`-managed tools ahead of Homebrew/system binaries. 4. Do **not** rely on image-baked `mise` state under `/home/coder` for the initial implementation. The goal here is binary availability and path precedence, not preinstalling shared `mise` toolchains. **Quality gate:** in a fresh container as `coder`, `echo "$PATH"` shows `mise` shims before Homebrew, and `mise doctor`/`mise doctor path` show no PATH or activation problem in the tested shell entrypoints. ### Phase 3 — Add shell-init hardening only if smoke tests prove it is needed 1. Start with the Dockerfile `ENV PATH` solution as the default behavior. 2. If dogfooding shows that supported login shells still need shell integration beyond the final `ENV PATH`, add minimal global shell-init files under: - `dogfood/coder/ubuntu-22.04/files/etc/profile.d/` - `dogfood/coder/ubuntu-26.04/files/etc/profile.d/` 3. If these files are needed, keep them narrowly scoped: - a Homebrew file that exports/evals `brew shellenv` - a `mise` file that only reinforces the intended shims-first behavior 4. Avoid touching per-user dotfiles in `/home/coder`; they are the wrong place for required image behavior because of the persistent home volume. **Quality gate:** if profile.d files are added, login-shell smoke tests pass and we do not introduce new PATH-order regressions versus the Dockerfile-only path. ## Acceptance criteria - Both Ubuntu dogfood Dockerfiles are updated in one change and still build. - `brew` is installed in `/home/linuxbrew/.linuxbrew` and is usable as `coder`. - `mise` is installed at `/usr/local/bin/mise` and is usable as `coder`. - `mise doctor` does not report an activation/PATH-ordering problem in the shell entrypoints we verify. - Final PATH precedence is: 1. `mise` shims 2. Homebrew `bin`/`sbin` 3. existing user/tool paths 4. system paths - Existing dogfood workflows still work for Go/Rust/Node tooling after the PATH change. - The change passes the dogfood image CI path in `.github/workflows/dogfood.yaml`. ## Dogfooding and verification 1. Build both images locally: - `dogfood/coder/ubuntu-22.04` - `dogfood/coder/ubuntu-26.04` 2. Run each image with an empty mounted home volume at `/home/coder` to mimic the actual dogfood runtime constraint instead of only testing the image’s baked filesystem. 3. Capture a short terminal recording and screenshots for each variant showing: - `brew --prefix` - `brew --version` - `mise --version` - `echo "$PATH"` - `mise doctor` - `mise doctor path` 4. Verify at least one login-shell path and one non-login-shell path, so we can tell whether Dockerfile `ENV PATH` is sufficient or whether `/etc/profile.d` hardening is required. 5. Add one tool-resolution smoke test that proves `mise` wins when configured: - install/use a small `mise`-managed runtime as `coder` - run `which -a <tool>` - run `<tool> --version` 6. Verify existing image behavior did not regress: - `go version` - `node --version` - any other must-have image tools that were already on PATH 7. Preserve the artifacts from dogfooding for review: - screenshots attached to the change summary - a short screen recording (or terminal recording) covering the smoke test ## Risks and decision points - **Homebrew ownership model:** installing Homebrew during `docker build` is not enough by itself; the prefix must end up writable for `coder`. - **Scope control:** the initial change should solve `mise doctor` by fixing PATH precedence, not by introducing a larger `mise`-managed tool bootstrap. - **Shell-init uncertainty:** if the dogfood terminal entrypoints do not source `/etc/profile`, a Dockerfile `ENV PATH` fix may be sufficient and profile.d may be unnecessary. This should be decided by smoke tests, not by assumption. - **Persistent home behavior:** avoid any required implementation detail that only works if fresh volumes copy image-baked `/home/coder` contents. <details> <summary>Why this is the lowest-risk path</summary> This plan keeps the initial implementation focused on the user’s stated goal: install Homebrew and `mise`, then guarantee that `mise`-controlled paths win so `mise doctor` stays quiet. The main repo-specific constraint is the persistent `/home/coder` volume. That pushes required binaries and ownership-sensitive state out of `/home/coder` where possible, and it argues against relying on user dotfiles for required image behavior. Starting with Dockerfile-level install steps plus a final PATH reorder keeps the diff small, makes behavior consistent across shells, and gives us a clean place to add shell-init hardening only if the smoke tests prove it is necessary. </details> </details> --- _Generated with [`mux`](https://github.com/coder/mux) • Model: `openai:gpt-5.5` • Thinking: `xhigh`_ |
||
|
Thomas Kosiewski
|
69610cca75 |
feat(site/src): add Known Model autocomplete and frontend defaults (#24842)
Replaces the blank Model Identifier free-text input on the **Add Model** page with provider-scoped Known Model autocomplete and frontend-only metadata defaults for native OpenAI and Anthropic providers. Selecting a Known Model, or typing an exact canonical identifier and blurring the field, prefills `contextLimit`, the appropriate max-output-tokens field, and flat base pricing in the existing form. Edit mode, duplicate mode, and unsupported providers preserve the existing plain `Input` behavior and submit payload byte-for-byte. The catalog is curated TypeScript records sourced from `models.dev`, scoped initially to 6 OpenAI and 5 Anthropic models in declared display order. The pure `applyKnownModelDefaults` helper only writes a field when its current value still equals the form's initial value (or was last applied by Known Model defaulting in this form session, tracked cumulatively across selections). It never sets `compressionThreshold` or any reasoning/thinking fields, ignores tiered pricing, and never writes to the `model` field (canonicalization is the caller's responsibility). This PR also makes two narrow, additive changes outside the panel directory: - `site/src/components/Autocomplete/Autocomplete.tsx` gains optional `triggerAriaInvalid`, `triggerAriaDescribedBy`, and `onEscapeKeyDown` props so the new catalog branch can preserve `aria-invalid` / `aria-describedby` parity with the plain input and observe Escape close intent reliably across the Radix portal. Existing `Autocomplete` consumers are unaffected; `stopPropagation` is gated on `onEscapeKeyDown` being provided. - `site/src/pages/AgentsPage/components/ChatModelAdminPanel/modelConfigFormLogic.ts` exports `deepGet` / `deepSet` so the defaulting helper can reuse them instead of re-implementing the same path traversal. No backend, API, SDK, or DB changes. No edits to `ModelsSection.tsx`, `ModelConfigFields.tsx`, `pricingFields.ts`, or `providerPolicyDefaults.ts`. ## Validation - 37 colocated unit tests across `knownModels/` (catalog, search, exact-canonical lookup, exact-alias lookup, badge, defaulting helper). - 134 unit tests across the full ChatModelAdminPanel directory pass. - 50 Storybook play tests on `ChatModelAdminPanel.stories.tsx` pass, including 17 DEREM-traceable interaction tests covering each plan-listed and review-driven scenario (open-no-error, Escape cancellation, sequential selection, double-apply guard, blur-canonical, alias cancellation, provider-change reset, ARIA parity, no-options copy, off-catalog substring commit, stale-cost-field, off-catalog interleaving, chain tracking, keyboard selection, clearable-disabled, off-catalog punctuation variant). - `tsc -p .` passes. ## Dogfooding Storybook was run locally and the user-facing flows were exercised end-to-end via `agent-browser`, capturing screenshots for: 1. OpenAI happy path (selection → defaults applied note → populated fields). 2. Anthropic happy path (selection → populated fields, reasoning/thinking blank). 3. Unsupported provider fallback (Google plain input, no popover). 4. OpenAI suggestion popover at empty focus (declared catalog order, context badges). 5. OpenAI search filter (typing `5.4` filters to GPT-5.4 / 5.4 mini / 5.4 nano). 6. Edit mode plain input (autocomplete correctly gated to add mode only). 7. DEREM-3: empty popover open on Add Model — no premature `Model ID is required.` error. 8. DEREM-1: autocomplete trigger `aria-invalid="true"` and `aria-describedby` matching the rendered error element. 9. DEREM-6: exact `No matching known models. You can still use this identifier.` copy. --- <details> <summary>📋 Implementation Plan</summary> # Plan: Known Model autocomplete and frontend-only defaults for Chat Model Admin ## Goal Improve the admin Add Model onboarding flow by replacing the blank Model Identifier experience with provider-scoped Known Model discovery suggestions for native OpenAI and Anthropic providers. Selecting a Known Model, or typing an exact canonical Known Model identifier and blurring the field, should prefill safe objective model metadata in the existing form without changing backend APIs, database schema, or runtime behavior. The primary UX goal is discovery for admins who do not know exact provider model identifiers or metadata. Typing convenience is a secondary benefit. ## Evidence and current code facts - The current Model Identifier field is a plain free-text `Input` in `site/src/pages/AgentsPage/components/ChatModelAdminPanel/ModelForm.tsx`. It submits as `model` and is only validated as a non-empty string. - The provider selector is disabled in edit and duplicate modes. In add mode, `ModelsSection.tsx` keys the form by provider, so provider changes remount `ModelForm`. - The shared `site/src/components/Autocomplete/Autocomplete.tsx` primitive already supports free-text input with suggestions and is the right UI primitive for this feature. - `modelConfigFormLogic.ts` owns form initialization via `buildInitialModelFormValues(...)`, and `modelConfigFormLogic.test.ts` already covers this pure logic area. - No frontend or backend Known Model catalog exists today. - The database has a non-unique `(provider, model)` index, not a uniqueness constraint. Multiple Model Configs can share the same Provider and Model Identifier, so suggestions must not hide already-configured models. - `models.dev/api.json` has provider-keyed model metadata with canonical IDs, names, limits, pricing, release dates, and `last_updated` values. The Phase 1 catalog should copy a curated subset into TypeScript records, not fetch at runtime. ## Domain language Use these terms consistently in code, tests, docs, and review discussion: - **Provider**: configured external AI service such as native `openai` or `anthropic`. - **Model Config**: persisted admin-defined config row used by Coder chat runtime. - **Model Identifier**: exact provider API string submitted as `model`, such as `gpt-5.5`. - **Known Model**: curated frontend catalog entry with advisory metadata for one canonical Model Identifier. - **Model Catalog**: checked-in frontend-only list of Known Models. - **Off-catalog Model Identifier**: user-entered Model Identifier that does not match any Known Model and remains valid. - **Default application**: copying advisory Known Model metadata into a draft add-mode Model Config form. ## Resolved design decisions ### UX scope - Implement this on the Add Model page/form only. - Do not add provider success popups, provider-side calls to action, or new deep-link behavior in this pass. - Use `Autocomplete` only when all are true: - form mode is add; - selected Provider is native `openai` or native `anthropic`; - that Provider has Known Models. - Edit mode, duplicate mode, and unsupported providers keep the existing free-text input behavior. ### Suggestion behavior - Suggestions open on focus only when the Model Identifier field is empty. - Once the field has text, suggestions open while typing or interacting with the autocomplete. - Empty unsupported-provider catalogs degrade silently to the existing plain input behavior. - When a supported provider has zero matches for a non-empty query, show a non-blocking empty state such as: `No matching known models. You can still use this identifier.` - Suggestion rows show: - display name; - canonical Model Identifier; - context-window badge, for example `1.05M context`. - Format context badges with a deterministic helper covered by tests, for example `200K context`, `400K context`, and `1.05M context`. - Do not show pricing, recommendations, capability tags, or large-context caveats in suggestion rows. - Keep catalog display order as product ordering. Do not show a visible `Recommended` badge. ### Canonical IDs and aliases - Selecting a Known Model always writes its canonical Model Identifier into the form. - Use non-date latest aliases as canonical onboarding IDs when the provider exposes them, such as `gpt-5.5` or `claude-sonnet-4-6`. - Date-pinned IDs may be aliases for search, but selecting a Known Model writes the non-date canonical ID. - Typing aliases filters suggestions but does not rewrite the field and does not apply defaults by itself. - Search over canonical ID, display name, and explicit aliases. - Search is case-insensitive and normalizes spaces, hyphens, underscores, and dots before substring matching. - Aliases are objective name or identifier variants only. Do not include editorial intent tags such as `best`, `cheap`, `fast`, `coding`, or `reasoning`. - Do not implement typo-tolerant fuzzy search in Phase 1. ### Default application rules - Default application only runs in add mode. - Explicit Known Model selection applies defaults immediately. - Exact typed or pasted canonical Model Identifier applies defaults on blur, not on every keystroke. This avoids prematurely applying `gpt-5.5` while the admin is typing `gpt-5.5-pro`. - Defaults fill only target fields whose current values still equal this form session's initial values. - Do not use Formik touched state as the source of truth for safety. - Do not implement field-level provenance tracking in Phase 1. - Capture an immutable `initialValuesRef` at `ModelForm` mount/remount and compare against that snapshot for safe default application. Do not compare against a live Formik reference that can drift. - Do not reapply repeatedly for the same provider/model pair in a single form session. - The defaulting helper must return both the next values and the list of applied form paths: ```ts interface ApplyKnownModelDefaultsResult { values: ModelFormValues; appliedFields: readonly string[]; } ``` - Treat Model Identifier canonicalization separately from metadata default application. `appliedFields` tracks populated metadata/form paths only, not the `model` field change caused by selecting a Known Model. - Show an inline note near Model Identifier only when `appliedFields.length > 0`, such as: `Defaults applied from GPT-5.5. Review and adjust before saving.` - Do not show a note for off-catalog identifiers, no-op Known Model selections, or selections that only canonicalize the Model Identifier. ### Initial Model Catalog Use curated TypeScript records with source metadata copied from models.dev. Do not check in the full `models.dev/api.json` snapshot and do not add a generator in Phase 1. Add a file-level comment that array order controls suggestion order so future cleanup does not accidentally change onboarding UX. Initial native OpenAI entries, in display order: 1. `gpt-5.5` 2. `gpt-5.5-pro` 3. `gpt-5.4` 4. `gpt-5.4-mini` 5. `gpt-5.4-nano` 6. `gpt-5.3-codex` Initial native Anthropic entries, in display order: 1. `claude-opus-4-7` 2. `claude-opus-4-6` 3. `claude-sonnet-4-6` 4. `claude-haiku-4-5` 5. `claude-sonnet-4-5` Do not include GPT-4.x, pre-5.3 GPT models, or Claude models older than 4.5 in this onboarding catalog unless product intentionally expands scope. Each Known Model record should include: - provider; - canonical Model Identifier; - display name; - aliases; - source metadata, including `sourceName: "models.dev"`, `sourceRetrievedAt`, and the model record's `last_updated` value; - `contextLimit` from `limit.context`; - `maxOutputTokens` from `limit.output`; - flat base pricing from supported `cost.*` fields. ### Field mapping - `models.dev.limit.context` maps to `contextLimit`. - `models.dev.limit.output` maps to the selected provider's exact max-output-tokens field when one exists, otherwise to generic `config.maxOutputTokens`. - Never fill both generic and provider-specific output-token fields for the same Known Model. - Ignore `models.dev.limit.input` unless the current form schema already exposes an exact matching field. - Map only flat base pricing fields that the existing form can persist: - `cost.input`; - `cost.output`; - `cost.cache_read`; - `cost.cache_write`. - Reuse `pricingFields.ts` or the existing pricing field descriptors instead of hard-coding cost form paths. - If `cache_read` or `cache_write` is absent from a models.dev entry, leave the corresponding field at its initial value and do not include it in `appliedFields`. - Ignore tiered pricing such as `context_over_200k` in Phase 1. Add a code comment in the adapter explaining that Coder currently persists flat pricing only. - Do not show a UI caveat for tiered pricing in Phase 1. - Do not set `compressionThreshold` from Known Models. - Do not prefill provider-specific reasoning or thinking fields in Phase 1, including: - OpenAI `reasoningEffort` and `reasoningSummary`; - Anthropic `sendReasoning`, `effort`, and `thinking.budgetTokens`. ## Proposed file structure Use `knownModels/` rather than `modelDefaults/` because the data powers both discovery and default application. New files: - `site/src/pages/AgentsPage/components/ChatModelAdminPanel/knownModels/types.ts` - `site/src/pages/AgentsPage/components/ChatModelAdminPanel/knownModels/openai.ts` - `site/src/pages/AgentsPage/components/ChatModelAdminPanel/knownModels/anthropic.ts` - `site/src/pages/AgentsPage/components/ChatModelAdminPanel/knownModels/index.ts` - `site/src/pages/AgentsPage/components/ChatModelAdminPanel/knownModels/applyKnownModelDefaults.ts` - `site/src/pages/AgentsPage/components/ChatModelAdminPanel/ModelIdentifierField.tsx` Existing files to modify: - `site/src/pages/AgentsPage/components/ChatModelAdminPanel/ModelForm.tsx` - `site/src/pages/AgentsPage/components/ChatModelAdminPanel/modelConfigFormLogic.ts` - `site/src/pages/AgentsPage/components/ChatModelAdminPanel/modelConfigFormLogic.test.ts` - `site/src/pages/AgentsPage/components/ChatModelAdminPanel/ChatModelAdminPanel.stories.tsx` Documentation artifacts to keep in sync if implementing from a clean workspace: - `site/src/pages/AgentsPage/components/ChatModelAdminPanel/CONTEXT.md` - `site/src/pages/AgentsPage/components/ChatModelAdminPanel/docs/adr/0001-frontend-known-model-catalog.md` ## Implementation plan ### Phase 1: Red, define pure behavior first 1. Add tests in `modelConfigFormLogic.test.ts` or a colocated `knownModels` test file for: - provider-scoped lookup; - normalized alias search; - canonicalization on selection; - unknown model leaves values unchanged; - exact canonical ID lookup; - safe initial-value patching; - `appliedFields` output that excludes Model Identifier canonicalization; - tiered pricing ignored; - missing cache pricing fields left at initial values; - compression threshold not populated; - reasoning/thinking fields not populated; - output-token mapping prefers provider-specific exact field and never fills both; - context badge formatting. 2. Add lifecycle tests where feasible: - provider change in add mode remounts the form and resets `initialValuesRef`, `lastAppliedProviderModelRef`, and inline default-feedback state. 3. Add edge-case tests for event and reapplication semantics: - selecting `gpt-5.5` then blurring does not apply defaults a second time; - typing `gpt-5.5-pro` then blurring applies only pro defaults, never prefix `gpt-5.5` defaults; - selecting one Known Model, then another, does not overwrite fields already populated by the first selection because they no longer match initial values; - typing an alias then blurring does not canonicalize or apply defaults; - an Off-catalog value for a supported provider remains valid and preserves existing required-field validation behavior. 4. Add tests for the initial OpenAI and Anthropic catalog entries to ensure IDs, source metadata, and display order remain intentional. Quality gate: targeted unit tests fail for missing implementation. ### Phase 2: Green, add Known Model catalog and pure helpers 1. Add `knownModels/types.ts` with readonly types for catalog records and source metadata. 2. Add `knownModels/openai.ts` and `knownModels/anthropic.ts` with the initial catalog entries and file-level refresh comments. 3. Add lookup and search helpers in `knownModels/index.ts`. 4. Add `applyKnownModelDefaults(...)` as a pure helper that accepts: - current form values; - initial form values; - selected provider; - Known Model; - provider field mapping helpers if needed. 5. Ensure assertions or explicit guards make impossible cases fail fast during tests, for example missing provider, missing canonical ID, or invalid source metadata. Quality gate: targeted unit tests pass. ### Phase 3: Wire Model Identifier autocomplete UX Autocomplete integration constraints: - Control the shared `Autocomplete` with `inputValue` for the free-text Model Identifier string and `value: KnownModel | null` for selected suggestions. - Pass pre-filtered Known Model options to `Autocomplete`; do not rely on `cmdk` internal filtering once `inputValue` is controlled. - Clear the selected `KnownModel | null` value whenever the admin types arbitrary text that no longer corresponds to the selected Known Model. - Guard selection and blur event ordering so selecting a row does not cause the input blur handler to apply defaults a second time. - Run exact-match blur behavior only when focus leaves the whole field/combobox, not when focus moves into the suggestion list. - Store the last-applied provider/model pair in form-local state or a ref so add-mode provider remounts reset it naturally. - Preserve the existing field contract: label, tooltip/help text, `name`, validation error rendering, `aria-invalid`, `aria-describedby`, disabled state, Formik blur/touched behavior, and submitted request shape. 1. Add `ModelIdentifierField.tsx`. 2. Preserve existing plain `Input` markup for edit mode, duplicate mode, and unsupported providers. 3. For add-mode supported providers, render `Autocomplete` with: - controlled free-text value tied to Formik's `model` field; - custom row rendering with display name, canonical ID, and context badge; - open-on-empty-focus behavior; - non-blocking no-match copy for non-empty supported-provider queries; - keyboard support inherited from `Autocomplete`. 4. On Known Model selection: - set the form's `model` field to the canonical ID; - apply defaults immediately; - show inline feedback only if fields changed. 5. On blur: - if the final field value exactly equals a Known Model canonical ID, apply defaults safely; - do not auto-apply aliases on blur. 6. Track the last applied provider/model pair in the form session to avoid repeated reapplication. Quality gate: Storybook stories compile and the main interaction paths work locally. ### Phase 4: Storybook and UX coverage Add or extend `ChatModelAdminPanel.stories.tsx` with three user-visible flows: 1. OpenAI happy path: - open Add Model for OpenAI; - focus empty Model Identifier; - suggestions appear; - select `GPT-5.5`; - assert `gpt-5.5` is in the input; - assert inline defaults note appears; - assert visible context limit and max output fields populate; - expand the pricing section before asserting pricing fields, or keep detailed pricing assertions in unit tests if the Storybook UI would become brittle. 2. Anthropic happy path: - open Add Model for Anthropic; - select `Claude Opus 4.7`; - assert canonical ID, visible context limit, and output field populate; - expand the pricing section before asserting pricing fields, or keep detailed pricing assertions in unit tests if the Storybook UI would become brittle; - assert Anthropic reasoning/thinking fields remain blank. 3. Unsupported provider fallback: - open Add Model for Azure or openai-compat; - assert Model Identifier behaves as plain free text and no suggestion popover appears. If practical, include one keyboard selection path in Storybook or manual dogfooding: - tab/focus Model Identifier; - arrow to a suggestion; - press Enter; - verify canonicalization and defaults. Quality gate: Storybook interaction tests pass for touched stories. ### Phase 5: Refactor and documentation pass 1. Keep catalog data isolated from UI rendering code. 2. Keep provider field mapping in one helper so future Google, Bedrock, OpenRouter, or Azure support does not require editing defaulting logic everywhere. 3. Ensure comments explain why tiered pricing and reasoning defaults are excluded. 4. Update `CONTEXT.md` and ADR if implementation changes any design decision captured there. 5. Run formatting and linting for touched frontend files. Quality gate: no broad refactors beyond this feature's files. ## Validation commands Use the repo's existing frontend validation commands, scoped where possible: - `pnpm -C site test <targeted ChatModelAdminPanel pattern>` - `pnpm -C site test <targeted modelConfigFormLogic pattern>` - `pnpm -C site test:storybook` - `pnpm -C site lint:types` - `pnpm -C site check` If command names differ in this workspace, inspect `site/package.json` and use the closest existing targeted commands. Do not claim success until the actual commands run and pass. ## Dogfooding plan Primary dogfood path is Storybook because this is a form-level UI improvement using mocked admin data. 1. Run Storybook for the Chat Model Admin Panel. 2. Record a short video showing: - OpenAI Add Model, focus empty Model Identifier, suggestions appear, select `GPT-5.5`, defaults note appears, fields populate; - Anthropic Add Model, select `Claude Opus 4.7`, fields populate, reasoning/thinking fields remain blank; - unsupported provider Add Model, Model Identifier stays free text with no suggestions. 3. Capture screenshots for the final state of each flow and attach them for review. 4. If implementation touches routing, `ModelsSection` URL state, or provider pages, also run the local UI and record `/agents/settings/models?newModel=openai` exercising the same OpenAI flow. ## Acceptance criteria - Add-mode native OpenAI and Anthropic Model Identifier fields provide discovery suggestions from the curated Known Model catalog. - Suggestions appear on empty focus and filter as the admin types. - Unsupported providers, edit mode, and duplicate mode preserve the current plain input behavior. - Selecting a Known Model canonicalizes the field and safely applies objective defaults. - Exact typed/pasted canonical IDs apply defaults on blur. - Off-catalog Model Identifiers remain valid and non-blocking. - Display name, context limit, output-token field, and flat pricing fill only when target fields still match initial values. - Compression threshold, tiered pricing, and provider-specific reasoning/thinking fields are not populated by Phase 1 defaults. - Inline feedback appears only when default application changed at least one field. - Unit tests, Storybook coverage, typecheck, formatting, and lint/check commands pass. - Dogfooding includes screenshots and video recordings. ## Risks and mitigations - **Catalog staleness**: models change frequently. Mitigate with source metadata and clear file-level refresh comments. - **Provider namespace mistakes**: Azure, Bedrock, OpenRouter, and openai-compat use different identifier semantics. Mitigate by supporting only native OpenAI and Anthropic in Phase 1. - **Auto-fill surprise**: defaults can feel magical. Mitigate with selection-first UX, blur-only exact-match behavior, initial-value safety checks, and inline feedback. - **Pricing inaccuracy for tiered models**: current form persists flat prices only. Mitigate by mapping base flat prices only and documenting tiered pricing as out of scope. - **Reasoning option overreach**: generic source metadata does not map cleanly to provider-specific controls. Mitigate by leaving reasoning/thinking fields blank in Phase 1. - **Overbroad UI changes**: replacing an input can affect accessibility and keyboard users. Mitigate by using the shared Autocomplete primitive, preserving plain Input fallback, and dogfooding keyboard selection. </details> --- _Generated with [`mux`](https://github.com/coder/mux) • Model: `anthropic:claude-opus-4-7` • Thinking: `max`_ |
||
|
Thomas Kosiewski
|
c3794d54ac | fix: avoid PTY for ssh command mode (#24862) | ||
|
Thomas Kosiewski
|
0b9b94ae4e |
feat(site/src): add advisor admin settings UI (#24624)
## Summary Add the admin-only Advisor settings form on the Agents Behavior page, wired to the PR 3 HTTP API via TanStack Query. Admins can enable/disable the advisor, tune per-run caps and max output tokens, pick a reasoning effort, and optionally select a dedicated advisor model. ## Motivation PR 3 introduced the configuration storage and API but no surface to change it. This PR puts the knobs in the existing admin Behavior UI so operators can turn the feature on/off and tune it without touching the database. ## Changes - `site/src/pages/AgentsPage/components/AdvisorSettings.tsx`: the admin-only settings panel. - TanStack Query hooks (`chatAdvisorConfig`, `updateChatAdvisorConfig`) with optimistic cache invalidation on save. - Inputs for `Enabled`, `MaxUsesPerRun`, `MaxOutputTokens`, `ReasoningEffort`, `ModelConfigID`. - Model-override dropdown renders "Use chat model" when unset. - `ZERO_UUID` / `isUnsetModelConfigId` helpers normalize the model-override payload in both directions: the backend serializes the unset sentinel as `"00000000-0000-0000-0000-000000000000"`, and the frontend must both recognize that sentinel on read and never send `""` on write (which would trigger a backend `invalid UUID length: 0`). - Behavior page wiring so the panel appears in the existing settings flow alongside the rest of the agent behavior toggles. ## Stack context This is **PR 6 of 6** in the advisor feature stack. It is stacked directly on **PR 3** (the config API) and is a sibling of PRs 4/5, so it can be reviewed independently of the chat-rendering work. ## Scope / non-goals - Does not change advisor runtime behavior; that is PR 4's responsibility. - No new audit entries for this settings change; it rides on the same storage mechanism as the existing site-configs pattern. ## Validation - `pnpm --filter site lint:types` - Manual dogfood against `./scripts/develop.sh`: toggle Enabled on/off, save, confirm `GET /api/experimental/chats/config/advisor` reflects changes, confirm non-admin users cannot reach the form. --- <details> <summary>📋 Implementation Plan (shared across the advisor stack)</summary> # Plan: Add a Mux-style advisor tool to coder agents/chatd ## Outcome Add a first-class `advisor` tool to agent chats in `coderd/x/chatd` that feels native to Coder: - it is a built-in server-side tool, not an MCP/dynamic-tool workaround; - it performs a nested **tool-less** model call for strategic advice; - it is exposed only when eligible, and the prompt mentions it only when it is actually available; - it is treated as a **planning-only** tool so it does not run alongside action tools in the same batch; - it tracks usage/cost separately enough for operators to reason about it; - it has a minimally polished UI in the Agents page; - and it ships with explicit dogfooding evidence, including screenshots and repro videos. ## Design decisions to lock before coding 1. **Primary architecture:** native built-in tool in `chattool/`, backed by a small `chatadvisor` package. 2. **Nested model execution:** reuse chatd's existing model/provider stack for a one-step, tool-less advisor call rather than inventing a new provider pathway. 3. **Execution policy:** treat `advisor` as an exclusive/planning-only tool; mixed batches must return structured policy errors and force the model to retry cleanly. 4. **Availability:** initial rollout is for root agent chats only; disable for child/sub-agent chats until recursion/cost policy is proven. 5. **Prompt sync:** use one eligibility boolean to drive both tool registration and advisor guidance injection. 6. **Persistence/cost split:** MVP should keep advisor usage visible in result metadata and server metrics; only add DB schema if product/billing explicitly needs queryable advisor-specific cost. 7. **UI scope:** generic tool rendering is an acceptable temporary milestone during backend bring-up, but the release candidate should include a dedicated lightweight advisor renderer. ## Delivery model The work should be executed as coordinated workstreams with one integration owner and parallel contributors for low-conflict areas. The integration owner should own `coderd/x/chatd/chatd.go` because prompt assembly, tool registration, and model resolution all converge there. ## Detailed workstreams ### Repo evidence used for this plan <details> <summary>Mux reference and current chatd seams</summary> **Mux reference implementation** - `src/node/services/tools/advisor.ts` — native advisor tool implementation. - `src/common/constants/advisor.ts` — advisor prompt/constants and truncation policy. - `src/common/utils/tools/tools.ts` — conditional tool registration. - `src/node/services/streamContextBuilder.ts` — injects advisor guidance only when the tool is available. **Current chatd seams** - `coderd/x/chatd/chatd.go` - `processChat()` — tool assembly, prompt assembly, and chatloop invocation. - `resolveChatModel()` — current model/provider/key resolution seam. - `type Config struct` — server-level chatd configuration surface. - `coderd/x/chatd/chatloop/chatloop.go` - `Run()` — main streaming/model loop. - `executeTools()` — built-in tool execution/batching seam. - `coderd/x/chatd/chattool/` — built-in tool implementations. - `site/src/pages/AgentsPage/components/ChatElements/tools/Tool.tsx` — tool renderer dispatch. - `site/src/pages/AgentsPage/components/ChatConversation/messageParsing.ts` and `ConversationTimeline.tsx` — tool/result merge and rendering flow. </details> ### Workstream map and ownership | Workstream | Primary owner | Main files | Can run in parallel? | Done when | |---|---|---|---|---| | 0. Integration + gating | Integration lead | `coderd/x/chatd/chatd.go` | No; central merge lane | Tool registration, prompt sync, and model selection are wired together | | 1. Advisor runtime + tool | Backend agent | new `coderd/x/chatd/chatadvisor/`, new `coderd/x/chatd/chattool/advisor.go` | Yes | Tool can perform a tool-less advisor call in memory and return structured results | | 2. Planning-only execution policy | Chatloop agent | `coderd/x/chatd/chatloop/chatloop.go`, related tests | Yes | Mixed `advisor` + action-tool batches are rejected cleanly and deterministically | | 3. Metrics/usage/config | Backend/telemetry agent | `chatd.go`, `chatloop/metrics.go`, optional config plumbing | Partially; coordinate with integration lead | Advisor usage is separately visible in metadata/metrics and limits are enforced | | 4. Frontend rendering | Frontend agent | `site/.../tools/Tool.tsx`, new `AdvisorTool.tsx`, stories | Yes after result schema stabilizes | Advisor renders as a readable card and story tests pass | | 5. Dogfood + QA evidence | QA agent | dev server, Storybook, dogfood output | After backend + UI are usable | Repro videos, screenshots, and a concise QA report exist | ### Parallelization rules - **Do not split `coderd/x/chatd/chatd.go` across multiple execution agents without an integration lead.** That file owns prompt building, tool registration, model resolution, and cost persistence. - Workstreams 1 and 2 can be developed in parallel and then stacked onto the integration branch. - Workstream 4 should begin once the backend result schema is agreed on, even if the backend is still behind a feature flag. - Any agent that needs to re-check Mux behavior should clone `coder/mux` into a temporary directory (for example, `$(mktemp -d)/mux`) and inspect it read-only; do not vendor or copy code from Mux directly. ## Phase 0 — Preflight and guardrails ### Goals - Align the team on the smallest shippable architecture. - Prevent scope creep into MCP/dynamic-tool/sub-agent variants. - Decide upfront what is MVP vs. follow-up. ### Tasks 1. **Confirm the MVP boundary.** - Ship a built-in advisor tool first. - Do **not** make MCP, dynamic tools, or sub-agents the primary implementation. - Do **not** add transient streaming phases in the first backend PR unless they fall out almost for free. 2. **Confirm local workflow hygiene before coding.** - Ensure the repo is using the project git hooks from `scripts/githooks`. - Do not bypass hooks with `--no-verify`. - Use `./scripts/develop.sh` for the full dev server rather than manual build/run commands. 3. **Lock the model-selection policy.** - **Recommended MVP:** advisor uses the same resolved provider/model/cost config as the current chat, with advisor-specific max-output and usage caps. - **Follow-up only if required:** add a separate `AdvisorModelConfigID`-style override that resolves through the existing `configCache`/model-config path. Do not invent a new free-form `provider:model` parser if chatd already stores provider/model separately. 4. **Lock the persistence policy.** - **Recommended MVP:** no DB migration. Persist advisor-visible metadata in the tool result and record separate metrics in memory/Prometheus. - **Only if product/billing explicitly asks for queryable advisor cost:** add a later DB migration or usage table, following the normal `queries/*.sql` + `make gen` workflow. 5. **Create an execution ADR note in the work item or tracking doc.** - Capture: built-in tool, tool-less nested call, root-chat-only rollout, exclusive execution policy, MVP no-DB-migration default. ### Quality gate - Everyone on the team can state the same answers to these questions: - Is advisor a built-in tool? **Yes.** - Can advisor run with action tools in the same batch? **No.** - Does advisor get tools of its own? **No.** - Is a DB migration required for MVP? **No, unless billing insists.** ## Phase 1 — Build the advisor runtime and tool wrapper ### Goals Create the core advisor implementation in a way that is easy to test and keeps `chattool/` thin. ### Files to add - `coderd/x/chatd/chatadvisor/types.go` - `coderd/x/chatd/chatadvisor/guidance.go` - `coderd/x/chatd/chatadvisor/handoff.go` - `coderd/x/chatd/chatadvisor/runtime.go` - `coderd/x/chatd/chatadvisor/runner.go` - `coderd/x/chatd/chattool/advisor.go` ### Responsibilities by file 1. **`types.go`** - Define the input/result schema used by the tool and UI. - Keep the result shape close to Mux so the UI and model both have predictable cases. - Recommended result variants: - `advice` - `limit_reached` - `error` Recommended shape: ```go type AdvisorArgs struct { Question string `json:"question"` } type AdvisorResult struct { Type string `json:"type"` Advice string `json:"advice,omitempty"` Error string `json:"error,omitempty"` AdvisorModel string `json:"advisor_model,omitempty"` RemainingUses int `json:"remaining_uses,omitempty"` Usage *AdvisorUsageResult `json:"usage,omitempty"` } ``` 2. **`guidance.go`** - Hold two strings: - the nested advisor system prompt; - the parent-agent guidance block to inject into the outer system prompt. - The nested advisor prompt must say, in plain language: - you are advising the parent agent; - you do not address the end user directly; - you do not claim actions happened; - you return concise strategic guidance and tradeoffs. 3. **`runtime.go`** - Define the per-run runtime state. - Recommended fields: - resolved model + model config; - provider keys/options reused from the outer chat; - `MaxUsesPerRun`; - `MaxOutputTokens`; - atomic/current call counter; - callback(s) to obtain the current prompt snapshot and current-step snapshot; - optional metrics/usage hook. - Add fail-fast validation for impossible config: nil model, non-positive limits, empty prompt builders, etc. 4. **`handoff.go`** - Build the advisor handoff message from: - the explicit question; - the exact prompt/messages the parent model just used; - the current step's text/reasoning snapshot, if available; - the most recent relevant tool outputs, if they are already in the prompt snapshot. - **Important:** use the already-prepared outer prompt tail, not a fresh DB reload. That keeps the advisor aligned with compaction and the exact context the outer model saw. - Apply hard truncation budgets with recent-context bias. 5. **`runner.go`** - Execute the nested advisor call. - **Recommended implementation:** call `chatloop.Run()` in an in-memory, one-step mode: - `Tools: nil` - `ProviderTools: nil` - `MaxSteps: 1` - `PersistStep`: capture the assistant output in memory instead of writing DB rows - Reuse the existing provider/model/cost path instead of building a second provider runner. - Assert that no tool definitions are passed to the nested call. 6. **`chattool/advisor.go`** - Keep this file thin and consistent with other built-ins. - Responsibilities: - decode `AdvisorArgs`; - validate `Question` is non-empty and bounded; - call the `chatadvisor` runner; - return a structured tool response. ### Defensive programming requirements - Assert `Question` is non-empty after trimming. - Assert runtime limits are positive. - Assert the nested advisor call runs with zero tools/provider tools. - Assert `AdvisorResult.Type` is one of the known variants before returning. - Assert remaining uses never goes negative. ### Acceptance criteria - A unit test can call the advisor tool with a fake model and receive a stable `advice` result. - The nested advisor call is impossible to run with tools accidentally attached. - The core logic lives in `chatadvisor/`, not embedded inside `chatd.go`. ## Phase 2 — Wire advisor into chatd and keep prompt/tool availability in sync ### Goals Register the tool in the right place, expose it only when eligible, and inject system guidance only when the tool is present. ### Files to modify - `coderd/x/chatd/chatd.go` - optionally a small helper file if `chatd.go` becomes too crowded ### Tasks 1. **Compute one eligibility boolean in `processChat()`.** Recommended inputs: - server-level advisor enabled flag; - root chat only (`chat.ParentChatID == uuid.Nil` or equivalent existing root/child check); - a usable resolved model/provider exists; - optional experiment/workspace/org gate if product wants staged rollout. 2. **Create the runtime once per outer chat run.** - Use the model/config/keys resolved by `resolveChatModel()`. - Reuse provider options from the current chat's `ChatModelCallConfig`. - Set `MaxUsesPerRun` and `MaxOutputTokens` from advisor config defaults. 3. **Register the tool in the built-in tool block.** - Insert after the skill tools and before MCP tools in `processChat()`. - Record `builtinToolNames["advisor"] = true` so metrics stay bounded. 4. **Inject advisor guidance into the outer system prompt using the same boolean.** - Use `chatprompt.InsertSystem()` in the same prompt assembly path that already injects user/system instructions. - Place the block near the existing instruction insertion, before plan-path/skill context blocks. - Wrap the guidance in an explicit tag like `<advisor-guidance>` so it is easy to spot in tests and future refactors. 5. **Keep advisor out of child chats for the first release.** - That avoids recursion/cost blowups with `spawn_agent` / `wait_agent` flows. - Document this explicitly in the rollout notes and tests. ### Acceptance criteria - If advisor is disabled, neither the tool nor the prompt guidance appears. - If advisor is enabled, both the tool and the prompt guidance appear. - Root chats can use advisor; child chats cannot. - Built-in tool names include `advisor` so metrics do not collapse it into the generic `mcp` label. ## Phase 3 — Enforce planning-only execution policy in `chatloop` ### Goals Prevent the model from calling `advisor` and action tools in the same execution batch. ### Files to modify - `coderd/x/chatd/chatloop/chatloop.go` - related chatloop tests ### Recommended implementation Keep the MVP small; do **not** build a general policy engine yet. 1. Add a minimal field to `chatloop.RunOptions`, for example: ```go ExclusiveToolName *string ``` 2. In `Run()` / `executeTools()`, detect the case where the exclusive tool appears in the same local-tool batch as any other locally executed tool. 3. When that happens, synthesize structured tool-result errors for the affected calls instead of executing anything in the batch. - `advisor` should receive a clear error like: _advisor must be called by itself before action tools_. - The sibling action tools should receive a paired policy error like: _this tool was skipped because advisor must run alone_. 4. Let the outer model see those tool errors and retry cleanly. - This is simpler and safer than partial execution or hidden deferral. - It preserves deterministic transcript history for debugging. 5. Pass the just-finished step snapshot into the tool execution context. - The advisor runtime should be able to see the current step's text/reasoning content, because that is often the best hint about what the outer model is trying to decide. ### Why this is the right fit - It matches the intended semantics: advisor is consulted **before** taking action. - It avoids subtle race conditions caused by concurrent built-in tool execution. - It keeps the behavior easy to test with fake models. ### Acceptance criteria - A model-emitted batch containing only `advisor` succeeds. - A model-emitted batch containing `advisor` plus any other locally executed tool returns deterministic policy errors and executes nothing. - Non-advisor tool execution stays unchanged for normal chats. ## Phase 4 — Usage limits, metrics, and configuration ### Goals Make advisor safe to operate without over-designing billing/storage in the first release. ### Files to modify - `coderd/x/chatd/chatd.go` - `coderd/x/chatd/chatloop/metrics.go` as needed - `coderd/x/chatd/chatd.go` `Config` struct and constructor path - optional follow-up config/db files only if a separate advisor model or persistent billing is required ### Tasks 1. **Add explicit server config knobs for MVP.** Recommended fields on `chatd.Config` or a nested advisor config struct: - `AdvisorEnabled bool` - `AdvisorMaxUsesPerRun int` - `AdvisorMaxOutputTokens int64` 2. **Track usage per outer run.** - Reset the counter for each `processChat()` invocation. - Return `remaining_uses` in the tool result. - Return `limit_reached` when the cap is exhausted. 3. **Expose advisor usage metadata in the tool result.** - Include model name and token/cost summary if available. - Use the same `callConfig.Cost` calculation path as the outer chat for MVP if advisor reuses the same model. 4. **Record server-side metrics.** - Count advisor invocations, failures, and latency. - Ensure they show up under the built-in tool label `advisor`. 5. **Optional decision gate: separate advisor model.** - If product insists on a stronger/different advisor model, add a follow-up config hook that resolves another existing chat model config through the same `configCache` path. - Keep that out of the first landing PR unless it is required for acceptance. 6. **Optional decision gate: queryable advisor cost.** - If this becomes required, spin a follow-up DB task: - update `coderd/database/queries/*.sql`; - add migration files; - run `make gen`; - update audit mappings if a new auditable type/field is introduced. ### Acceptance criteria - Advisor calls are capped per outer run. - Limit exhaustion is user-visible in the tool result. - Metrics distinguish advisor calls from other built-in tools. - MVP does not require a schema migration unless explicitly approved. ## Phase 5 — Frontend rendering and Storybook coverage ### Goals Make advisor feel intentional in the Agents UI without blocking the backend on fancy streaming UI. ### Files to modify - `site/src/pages/AgentsPage/components/ChatElements/tools/Tool.tsx` - new `site/src/pages/AgentsPage/components/ChatElements/tools/AdvisorTool.tsx` - Storybook story file(s) in the same tools directory ### Delivery strategy 1. **Intermediate milestone during backend bring-up:** rely on the existing generic tool renderer if needed. - This is acceptable only as a short-lived integration checkpoint. 2. **Release milestone:** add a dedicated lightweight `AdvisorTool` renderer. - Reuse existing primitives: - `ToolCollapsible` - `ToolIcon` - `Response` for markdown/prose rendering - `ScrollArea` if the advice can be long - Keep styling light and consistent with the Agents page. - Do not add unnecessary React memoization in `site/src/pages/AgentsPage/`; that area is already React-Compiler aware. 3. **Render the structured result states cleanly.** - `advice` — readable prose/markdown with optional metadata footer. - `limit_reached` — warning-style message. - `error` — error state with visible fallback text. - `running` — existing tool loading state/spinner is enough for MVP. 4. **Add Storybook coverage instead of ad-hoc component tests.** Recommended stories: - successful advice; - running/loading; - limit reached; - error. 5. **Keep the UI contract narrow.** - Prefer one text field like `advice` plus small metadata rather than a deeply nested schema. - That keeps the UI resilient to prompt iteration. ### Acceptance criteria - The advisor tool card renders readable content rather than raw quoted JSON in the final release branch. - Running, limit, and error states are visibly distinct. - Storybook stories and play assertions cover the new states. - Existing tool rendering flows remain unchanged. ## Phase 6 — Automated tests and validation gates ### Backend tests to add 1. **Advisor runtime/tool tests** - question validation; - tool-less nested execution assertion; - success result shaping; - limit-reached result shaping; - error result shaping. 2. **Prompt/gating tests in chatd** - advisor disabled ⇒ no tool, no guidance; - advisor enabled/root chat ⇒ tool + guidance; - child chat ⇒ advisor absent. 3. **Chatloop policy tests** - advisor alone runs; - advisor + action tool mixed batch returns deterministic policy errors; - non-advisor tools still execute normally. 4. **Usage/metrics tests** - per-run cap resets correctly; - builtin tool labeling includes `advisor`; - returned metadata includes model/usage summary when available. ### Frontend tests to add - Storybook `play()` assertions for the advisor renderer states. - Verify expand/collapse behavior and visible fallback text. - Verify the message timeline still renders adjacent tools correctly. ### Recommended command sequence Run these as the implementation matures, not only at the end: 1. Backend-focused gate after phases 1–4: - `make test RUN=TestAdvisor` - `make test RUN=TestChatloopAdvisor` - `make lint` 2. Frontend-focused gate after phase 5: - `pnpm test:storybook src/pages/AgentsPage/components/ChatElements/tools/AdvisorTool.stories.tsx` - `pnpm lint` - `pnpm format` 3. Final repo gate before handoff: - `make pre-commit` - run any additional targeted `make test RUN=...` selections covering touched chatd paths > Use the exact new test names the implementing agents create; the names above are recommended anchors, not existing tests. ## Dogfooding plan ### Principle Dogfood the change as a real agent feature, not just a unit-tested backend. Per the dogfood and `agent-browser` skills, the reviewer should get **watchable repro videos** plus screenshots that make the behavior obvious without reading logs. ### Required setup 1. Start the full dev environment with: - `./scripts/develop.sh` 2. If the frontend renderer changes, also start Storybook from `site/` with: - `pnpm storybook --no-open` 3. Use `agent-browser` directly — **never `npx agent-browser`**. 4. Use named browser sessions and an output folder such as: - `./dogfood-output/advisor/` - with subfolders `screenshots/` and `videos/` ### Evidence protocol For every interactive scenario below: 1. Start video recording **before** the action. 2. Capture step-by-step screenshots at human pace. 3. Capture one annotated screenshot of the final state. 4. Stop the recording. 5. Note the exact pass/fail observation in the QA report. For static UI states (for example Storybook error/limit cards), an annotated screenshot is sufficient; video is optional but still encouraged by this project’s review preference. ### Dogfood scenarios #### Scenario A — Happy path in the real Agents UI **Goal:** prove that a root agent chat can invoke advisor and produce a readable recommendation before taking further action. Steps: 1. Open the Agents page with an advisor-enabled root chat. 2. Start a repro video. 3. Send a prompt that should reasonably trigger strategic planning, such as an architecture or multi-tradeoff question. 4. Capture screenshots of: - the prompt before send; - the running advisor state; - the completed advisor card and the assistant’s follow-up response. 5. Stop recording. Pass criteria: - advisor appears in the timeline; - the rendered result is readable; - the assistant can continue after consuming the advisor output. #### Scenario B — Advisor unavailable path **Goal:** prove the feature is truly gated. Suggested variants (at least one is required, both are better): - feature flag/config off; - child/sub-agent chat. Evidence: - annotated screenshot of the chat/tool state showing advisor is absent; - short video if toggling the gate live is part of the repro. Pass criteria: - no advisor tool is available; - no advisor-specific prompt behavior leaks through. #### Scenario C — UI states in Storybook **Goal:** prove the renderer handles non-happy states cleanly. Required story states: - success/advice; - running; - limit reached; - error. Evidence: - one screenshot per state; - at least one short video showing collapse/expand behavior. Pass criteria: - success renders readable advice; - limit/error have visible fallback text; - the component behaves like the other tool cards. #### Scenario D — Regression sweep of nearby tools **Goal:** ensure advisor does not break the surrounding chat timeline. Check at minimum: - another existing built-in tool still renders correctly near advisor; - sub-agent/tool cards still expand/collapse normally; - no obvious console errors appear in the Agents page during the advisor flow. Evidence: - screenshots of adjacent tool cards; - console/error capture if anything suspicious appears. ### `agent-browser` usage notes for the QA agent - Prefer `agent-browser batch` for 2+ sequential commands when no intermediate parsing is needed. - Use `snapshot -i` to discover interactive refs. - Re-snapshot after navigation or major DOM changes. - Avoid `wait --load networkidle` unless the page is known to go idle; prefer explicit element/text waits or short fixed waits. - Record videos at human pace and include pauses that a reviewer can follow. ## Rollout plan ### Initial rollout - Gate behind a server-side advisor-enabled flag. - Enable only for selected internal/root agent chats first. - Watch metrics for: - invocation count; - failure rate; - latency; - obvious retry loops. ### Expansion conditions Expand beyond the initial rollout only after the following are true: - mixed-batch policy behavior is stable; - cost impact is understood; - frontend UX is readable in production-like dogfood; - no recursion surprises have appeared with sub-agent flows. ### Explicit non-goals for the first release - advisor inside child/sub-agent chats; - provider-agnostic streaming phase UI; - MCP-based external advisor implementation; - mandatory DB-backed advisor cost reporting. ## Final acceptance checklist - [ ] `advisor` is a built-in chatd tool, not an MCP/dynamic-tool substitute. - [ ] The nested advisor call is tool-less and bounded to one in-memory step. - [ ] One eligibility boolean controls both tool registration and prompt guidance injection. - [ ] Root chats can use advisor; child chats cannot in the initial rollout. - [ ] Mixed advisor/action batches produce deterministic policy errors instead of partial execution. - [ ] Per-run usage caps and limit-reached behavior work. - [ ] Advisor usage is visible in metadata/metrics without forcing a DB migration for MVP. - [ ] The Agents UI has a readable advisor card and Storybook coverage. - [ ] Dogfooding produced screenshots and repro videos for the required scenarios. - [ ] Validation commands (`make lint`, targeted `make test`, Storybook tests, `make pre-commit`) passed before handoff. ## Suggested PR split 1. **PR 1 — Backend foundation** - `chatadvisor/` package - `chattool/advisor.go` - `chatloop` exclusive policy - chatd gating/prompt sync - backend tests 2. **PR 2 — Frontend + QA** - advisor renderer - stories/play assertions - dogfood artifacts and QA notes 3. **PR 3 — Optional follow-ups only if demanded by stakeholders** - separate advisor model override - persistent advisor billing/queryability - transient phase-stream UX </details> --- _Generated with [`mux`](https://github.com/coder/mux) • Model: `anthropic:claude-opus-4-7` • Thinking: `max`_ |
||
|
Thomas Kosiewski
|
07e86ae565 |
feat(site/src/pages/AgentsPage): add advisor chat tool renderer (#24623)
## Summary Add a dedicated lightweight advisor chat card to the Agents page so advisor results render as readable prose instead of raw JSON. Includes Storybook coverage for the renderer's states. ## Motivation Once PR 4 lands, the backend can emit advisor tool results, but the generic tool renderer shows them as quoted JSON. A small dedicated renderer turns the recommendation into something an operator or reviewer can actually read, without blocking the backend on full streaming UI. ## Changes - `site/src/pages/AgentsPage/components/ChatElements/tools/AdvisorTool.tsx`: renders the `AdvisorResult` states: - `advice` as markdown prose via the existing `Response` primitive, - `limit_reached` as a warning card, - `error` as an error card with fallback text, - `running` via the existing tool loading spinner. Reuses `ToolCollapsible`, `ToolIcon`, and `Response`. Question is shown only in the card subtitle (no duplicate label in the body). - Metadata footer surfaces `advisor_model` and `remaining_uses` when present. - `Tool.tsx` dispatcher routes the `advisor` tool to the new renderer. - Storybook stories for each state (success/running/limit/error) with `play()` assertions for expand/collapse behavior. - No new React memoization; the Agents page is already React-Compiler-aware. ## Stack context This is **PR 5 of 6** in the advisor feature stack, stacked on top of PR 4. ## Scope / non-goals - No changes to the backend schema. - No changes to adjacent tool renderers. ## Validation - `pnpm --filter site lint:types` - `pnpm --filter site test:storybook src/pages/AgentsPage/components/ChatElements/tools/AdvisorTool.stories.tsx` --- <details> <summary>📋 Implementation Plan (shared across the advisor stack)</summary> # Plan: Add a Mux-style advisor tool to coder agents/chatd ## Outcome Add a first-class `advisor` tool to agent chats in `coderd/x/chatd` that feels native to Coder: - it is a built-in server-side tool, not an MCP/dynamic-tool workaround; - it performs a nested **tool-less** model call for strategic advice; - it is exposed only when eligible, and the prompt mentions it only when it is actually available; - it is treated as a **planning-only** tool so it does not run alongside action tools in the same batch; - it tracks usage/cost separately enough for operators to reason about it; - it has a minimally polished UI in the Agents page; - and it ships with explicit dogfooding evidence, including screenshots and repro videos. ## Design decisions to lock before coding 1. **Primary architecture:** native built-in tool in `chattool/`, backed by a small `chatadvisor` package. 2. **Nested model execution:** reuse chatd's existing model/provider stack for a one-step, tool-less advisor call rather than inventing a new provider pathway. 3. **Execution policy:** treat `advisor` as an exclusive/planning-only tool; mixed batches must return structured policy errors and force the model to retry cleanly. 4. **Availability:** initial rollout is for root agent chats only; disable for child/sub-agent chats until recursion/cost policy is proven. 5. **Prompt sync:** use one eligibility boolean to drive both tool registration and advisor guidance injection. 6. **Persistence/cost split:** MVP should keep advisor usage visible in result metadata and server metrics; only add DB schema if product/billing explicitly needs queryable advisor-specific cost. 7. **UI scope:** generic tool rendering is an acceptable temporary milestone during backend bring-up, but the release candidate should include a dedicated lightweight advisor renderer. ## Delivery model The work should be executed as coordinated workstreams with one integration owner and parallel contributors for low-conflict areas. The integration owner should own `coderd/x/chatd/chatd.go` because prompt assembly, tool registration, and model resolution all converge there. ## Detailed workstreams ### Repo evidence used for this plan <details> <summary>Mux reference and current chatd seams</summary> **Mux reference implementation** - `src/node/services/tools/advisor.ts` — native advisor tool implementation. - `src/common/constants/advisor.ts` — advisor prompt/constants and truncation policy. - `src/common/utils/tools/tools.ts` — conditional tool registration. - `src/node/services/streamContextBuilder.ts` — injects advisor guidance only when the tool is available. **Current chatd seams** - `coderd/x/chatd/chatd.go` - `processChat()` — tool assembly, prompt assembly, and chatloop invocation. - `resolveChatModel()` — current model/provider/key resolution seam. - `type Config struct` — server-level chatd configuration surface. - `coderd/x/chatd/chatloop/chatloop.go` - `Run()` — main streaming/model loop. - `executeTools()` — built-in tool execution/batching seam. - `coderd/x/chatd/chattool/` — built-in tool implementations. - `site/src/pages/AgentsPage/components/ChatElements/tools/Tool.tsx` — tool renderer dispatch. - `site/src/pages/AgentsPage/components/ChatConversation/messageParsing.ts` and `ConversationTimeline.tsx` — tool/result merge and rendering flow. </details> ### Workstream map and ownership | Workstream | Primary owner | Main files | Can run in parallel? | Done when | |---|---|---|---|---| | 0. Integration + gating | Integration lead | `coderd/x/chatd/chatd.go` | No; central merge lane | Tool registration, prompt sync, and model selection are wired together | | 1. Advisor runtime + tool | Backend agent | new `coderd/x/chatd/chatadvisor/`, new `coderd/x/chatd/chattool/advisor.go` | Yes | Tool can perform a tool-less advisor call in memory and return structured results | | 2. Planning-only execution policy | Chatloop agent | `coderd/x/chatd/chatloop/chatloop.go`, related tests | Yes | Mixed `advisor` + action-tool batches are rejected cleanly and deterministically | | 3. Metrics/usage/config | Backend/telemetry agent | `chatd.go`, `chatloop/metrics.go`, optional config plumbing | Partially; coordinate with integration lead | Advisor usage is separately visible in metadata/metrics and limits are enforced | | 4. Frontend rendering | Frontend agent | `site/.../tools/Tool.tsx`, new `AdvisorTool.tsx`, stories | Yes after result schema stabilizes | Advisor renders as a readable card and story tests pass | | 5. Dogfood + QA evidence | QA agent | dev server, Storybook, dogfood output | After backend + UI are usable | Repro videos, screenshots, and a concise QA report exist | ### Parallelization rules - **Do not split `coderd/x/chatd/chatd.go` across multiple execution agents without an integration lead.** That file owns prompt building, tool registration, model resolution, and cost persistence. - Workstreams 1 and 2 can be developed in parallel and then stacked onto the integration branch. - Workstream 4 should begin once the backend result schema is agreed on, even if the backend is still behind a feature flag. - Any agent that needs to re-check Mux behavior should clone `coder/mux` into a temporary directory (for example, `$(mktemp -d)/mux`) and inspect it read-only; do not vendor or copy code from Mux directly. ## Phase 0 — Preflight and guardrails ### Goals - Align the team on the smallest shippable architecture. - Prevent scope creep into MCP/dynamic-tool/sub-agent variants. - Decide upfront what is MVP vs. follow-up. ### Tasks 1. **Confirm the MVP boundary.** - Ship a built-in advisor tool first. - Do **not** make MCP, dynamic tools, or sub-agents the primary implementation. - Do **not** add transient streaming phases in the first backend PR unless they fall out almost for free. 2. **Confirm local workflow hygiene before coding.** - Ensure the repo is using the project git hooks from `scripts/githooks`. - Do not bypass hooks with `--no-verify`. - Use `./scripts/develop.sh` for the full dev server rather than manual build/run commands. 3. **Lock the model-selection policy.** - **Recommended MVP:** advisor uses the same resolved provider/model/cost config as the current chat, with advisor-specific max-output and usage caps. - **Follow-up only if required:** add a separate `AdvisorModelConfigID`-style override that resolves through the existing `configCache`/model-config path. Do not invent a new free-form `provider:model` parser if chatd already stores provider/model separately. 4. **Lock the persistence policy.** - **Recommended MVP:** no DB migration. Persist advisor-visible metadata in the tool result and record separate metrics in memory/Prometheus. - **Only if product/billing explicitly asks for queryable advisor cost:** add a later DB migration or usage table, following the normal `queries/*.sql` + `make gen` workflow. 5. **Create an execution ADR note in the work item or tracking doc.** - Capture: built-in tool, tool-less nested call, root-chat-only rollout, exclusive execution policy, MVP no-DB-migration default. ### Quality gate - Everyone on the team can state the same answers to these questions: - Is advisor a built-in tool? **Yes.** - Can advisor run with action tools in the same batch? **No.** - Does advisor get tools of its own? **No.** - Is a DB migration required for MVP? **No, unless billing insists.** ## Phase 1 — Build the advisor runtime and tool wrapper ### Goals Create the core advisor implementation in a way that is easy to test and keeps `chattool/` thin. ### Files to add - `coderd/x/chatd/chatadvisor/types.go` - `coderd/x/chatd/chatadvisor/guidance.go` - `coderd/x/chatd/chatadvisor/handoff.go` - `coderd/x/chatd/chatadvisor/runtime.go` - `coderd/x/chatd/chatadvisor/runner.go` - `coderd/x/chatd/chattool/advisor.go` ### Responsibilities by file 1. **`types.go`** - Define the input/result schema used by the tool and UI. - Keep the result shape close to Mux so the UI and model both have predictable cases. - Recommended result variants: - `advice` - `limit_reached` - `error` Recommended shape: ```go type AdvisorArgs struct { Question string `json:"question"` } type AdvisorResult struct { Type string `json:"type"` Advice string `json:"advice,omitempty"` Error string `json:"error,omitempty"` AdvisorModel string `json:"advisor_model,omitempty"` RemainingUses int `json:"remaining_uses,omitempty"` Usage *AdvisorUsageResult `json:"usage,omitempty"` } ``` 2. **`guidance.go`** - Hold two strings: - the nested advisor system prompt; - the parent-agent guidance block to inject into the outer system prompt. - The nested advisor prompt must say, in plain language: - you are advising the parent agent; - you do not address the end user directly; - you do not claim actions happened; - you return concise strategic guidance and tradeoffs. 3. **`runtime.go`** - Define the per-run runtime state. - Recommended fields: - resolved model + model config; - provider keys/options reused from the outer chat; - `MaxUsesPerRun`; - `MaxOutputTokens`; - atomic/current call counter; - callback(s) to obtain the current prompt snapshot and current-step snapshot; - optional metrics/usage hook. - Add fail-fast validation for impossible config: nil model, non-positive limits, empty prompt builders, etc. 4. **`handoff.go`** - Build the advisor handoff message from: - the explicit question; - the exact prompt/messages the parent model just used; - the current step's text/reasoning snapshot, if available; - the most recent relevant tool outputs, if they are already in the prompt snapshot. - **Important:** use the already-prepared outer prompt tail, not a fresh DB reload. That keeps the advisor aligned with compaction and the exact context the outer model saw. - Apply hard truncation budgets with recent-context bias. 5. **`runner.go`** - Execute the nested advisor call. - **Recommended implementation:** call `chatloop.Run()` in an in-memory, one-step mode: - `Tools: nil` - `ProviderTools: nil` - `MaxSteps: 1` - `PersistStep`: capture the assistant output in memory instead of writing DB rows - Reuse the existing provider/model/cost path instead of building a second provider runner. - Assert that no tool definitions are passed to the nested call. 6. **`chattool/advisor.go`** - Keep this file thin and consistent with other built-ins. - Responsibilities: - decode `AdvisorArgs`; - validate `Question` is non-empty and bounded; - call the `chatadvisor` runner; - return a structured tool response. ### Defensive programming requirements - Assert `Question` is non-empty after trimming. - Assert runtime limits are positive. - Assert the nested advisor call runs with zero tools/provider tools. - Assert `AdvisorResult.Type` is one of the known variants before returning. - Assert remaining uses never goes negative. ### Acceptance criteria - A unit test can call the advisor tool with a fake model and receive a stable `advice` result. - The nested advisor call is impossible to run with tools accidentally attached. - The core logic lives in `chatadvisor/`, not embedded inside `chatd.go`. ## Phase 2 — Wire advisor into chatd and keep prompt/tool availability in sync ### Goals Register the tool in the right place, expose it only when eligible, and inject system guidance only when the tool is present. ### Files to modify - `coderd/x/chatd/chatd.go` - optionally a small helper file if `chatd.go` becomes too crowded ### Tasks 1. **Compute one eligibility boolean in `processChat()`.** Recommended inputs: - server-level advisor enabled flag; - root chat only (`chat.ParentChatID == uuid.Nil` or equivalent existing root/child check); - a usable resolved model/provider exists; - optional experiment/workspace/org gate if product wants staged rollout. 2. **Create the runtime once per outer chat run.** - Use the model/config/keys resolved by `resolveChatModel()`. - Reuse provider options from the current chat's `ChatModelCallConfig`. - Set `MaxUsesPerRun` and `MaxOutputTokens` from advisor config defaults. 3. **Register the tool in the built-in tool block.** - Insert after the skill tools and before MCP tools in `processChat()`. - Record `builtinToolNames["advisor"] = true` so metrics stay bounded. 4. **Inject advisor guidance into the outer system prompt using the same boolean.** - Use `chatprompt.InsertSystem()` in the same prompt assembly path that already injects user/system instructions. - Place the block near the existing instruction insertion, before plan-path/skill context blocks. - Wrap the guidance in an explicit tag like `<advisor-guidance>` so it is easy to spot in tests and future refactors. 5. **Keep advisor out of child chats for the first release.** - That avoids recursion/cost blowups with `spawn_agent` / `wait_agent` flows. - Document this explicitly in the rollout notes and tests. ### Acceptance criteria - If advisor is disabled, neither the tool nor the prompt guidance appears. - If advisor is enabled, both the tool and the prompt guidance appear. - Root chats can use advisor; child chats cannot. - Built-in tool names include `advisor` so metrics do not collapse it into the generic `mcp` label. ## Phase 3 — Enforce planning-only execution policy in `chatloop` ### Goals Prevent the model from calling `advisor` and action tools in the same execution batch. ### Files to modify - `coderd/x/chatd/chatloop/chatloop.go` - related chatloop tests ### Recommended implementation Keep the MVP small; do **not** build a general policy engine yet. 1. Add a minimal field to `chatloop.RunOptions`, for example: ```go ExclusiveToolName *string ``` 2. In `Run()` / `executeTools()`, detect the case where the exclusive tool appears in the same local-tool batch as any other locally executed tool. 3. When that happens, synthesize structured tool-result errors for the affected calls instead of executing anything in the batch. - `advisor` should receive a clear error like: _advisor must be called by itself before action tools_. - The sibling action tools should receive a paired policy error like: _this tool was skipped because advisor must run alone_. 4. Let the outer model see those tool errors and retry cleanly. - This is simpler and safer than partial execution or hidden deferral. - It preserves deterministic transcript history for debugging. 5. Pass the just-finished step snapshot into the tool execution context. - The advisor runtime should be able to see the current step's text/reasoning content, because that is often the best hint about what the outer model is trying to decide. ### Why this is the right fit - It matches the intended semantics: advisor is consulted **before** taking action. - It avoids subtle race conditions caused by concurrent built-in tool execution. - It keeps the behavior easy to test with fake models. ### Acceptance criteria - A model-emitted batch containing only `advisor` succeeds. - A model-emitted batch containing `advisor` plus any other locally executed tool returns deterministic policy errors and executes nothing. - Non-advisor tool execution stays unchanged for normal chats. ## Phase 4 — Usage limits, metrics, and configuration ### Goals Make advisor safe to operate without over-designing billing/storage in the first release. ### Files to modify - `coderd/x/chatd/chatd.go` - `coderd/x/chatd/chatloop/metrics.go` as needed - `coderd/x/chatd/chatd.go` `Config` struct and constructor path - optional follow-up config/db files only if a separate advisor model or persistent billing is required ### Tasks 1. **Add explicit server config knobs for MVP.** Recommended fields on `chatd.Config` or a nested advisor config struct: - `AdvisorEnabled bool` - `AdvisorMaxUsesPerRun int` - `AdvisorMaxOutputTokens int64` 2. **Track usage per outer run.** - Reset the counter for each `processChat()` invocation. - Return `remaining_uses` in the tool result. - Return `limit_reached` when the cap is exhausted. 3. **Expose advisor usage metadata in the tool result.** - Include model name and token/cost summary if available. - Use the same `callConfig.Cost` calculation path as the outer chat for MVP if advisor reuses the same model. 4. **Record server-side metrics.** - Count advisor invocations, failures, and latency. - Ensure they show up under the built-in tool label `advisor`. 5. **Optional decision gate: separate advisor model.** - If product insists on a stronger/different advisor model, add a follow-up config hook that resolves another existing chat model config through the same `configCache` path. - Keep that out of the first landing PR unless it is required for acceptance. 6. **Optional decision gate: queryable advisor cost.** - If this becomes required, spin a follow-up DB task: - update `coderd/database/queries/*.sql`; - add migration files; - run `make gen`; - update audit mappings if a new auditable type/field is introduced. ### Acceptance criteria - Advisor calls are capped per outer run. - Limit exhaustion is user-visible in the tool result. - Metrics distinguish advisor calls from other built-in tools. - MVP does not require a schema migration unless explicitly approved. ## Phase 5 — Frontend rendering and Storybook coverage ### Goals Make advisor feel intentional in the Agents UI without blocking the backend on fancy streaming UI. ### Files to modify - `site/src/pages/AgentsPage/components/ChatElements/tools/Tool.tsx` - new `site/src/pages/AgentsPage/components/ChatElements/tools/AdvisorTool.tsx` - Storybook story file(s) in the same tools directory ### Delivery strategy 1. **Intermediate milestone during backend bring-up:** rely on the existing generic tool renderer if needed. - This is acceptable only as a short-lived integration checkpoint. 2. **Release milestone:** add a dedicated lightweight `AdvisorTool` renderer. - Reuse existing primitives: - `ToolCollapsible` - `ToolIcon` - `Response` for markdown/prose rendering - `ScrollArea` if the advice can be long - Keep styling light and consistent with the Agents page. - Do not add unnecessary React memoization in `site/src/pages/AgentsPage/`; that area is already React-Compiler aware. 3. **Render the structured result states cleanly.** - `advice` — readable prose/markdown with optional metadata footer. - `limit_reached` — warning-style message. - `error` — error state with visible fallback text. - `running` — existing tool loading state/spinner is enough for MVP. 4. **Add Storybook coverage instead of ad-hoc component tests.** Recommended stories: - successful advice; - running/loading; - limit reached; - error. 5. **Keep the UI contract narrow.** - Prefer one text field like `advice` plus small metadata rather than a deeply nested schema. - That keeps the UI resilient to prompt iteration. ### Acceptance criteria - The advisor tool card renders readable content rather than raw quoted JSON in the final release branch. - Running, limit, and error states are visibly distinct. - Storybook stories and play assertions cover the new states. - Existing tool rendering flows remain unchanged. ## Phase 6 — Automated tests and validation gates ### Backend tests to add 1. **Advisor runtime/tool tests** - question validation; - tool-less nested execution assertion; - success result shaping; - limit-reached result shaping; - error result shaping. 2. **Prompt/gating tests in chatd** - advisor disabled ⇒ no tool, no guidance; - advisor enabled/root chat ⇒ tool + guidance; - child chat ⇒ advisor absent. 3. **Chatloop policy tests** - advisor alone runs; - advisor + action tool mixed batch returns deterministic policy errors; - non-advisor tools still execute normally. 4. **Usage/metrics tests** - per-run cap resets correctly; - builtin tool labeling includes `advisor`; - returned metadata includes model/usage summary when available. ### Frontend tests to add - Storybook `play()` assertions for the advisor renderer states. - Verify expand/collapse behavior and visible fallback text. - Verify the message timeline still renders adjacent tools correctly. ### Recommended command sequence Run these as the implementation matures, not only at the end: 1. Backend-focused gate after phases 1–4: - `make test RUN=TestAdvisor` - `make test RUN=TestChatloopAdvisor` - `make lint` 2. Frontend-focused gate after phase 5: - `pnpm test:storybook src/pages/AgentsPage/components/ChatElements/tools/AdvisorTool.stories.tsx` - `pnpm lint` - `pnpm format` 3. Final repo gate before handoff: - `make pre-commit` - run any additional targeted `make test RUN=...` selections covering touched chatd paths > Use the exact new test names the implementing agents create; the names above are recommended anchors, not existing tests. ## Dogfooding plan ### Principle Dogfood the change as a real agent feature, not just a unit-tested backend. Per the dogfood and `agent-browser` skills, the reviewer should get **watchable repro videos** plus screenshots that make the behavior obvious without reading logs. ### Required setup 1. Start the full dev environment with: - `./scripts/develop.sh` 2. If the frontend renderer changes, also start Storybook from `site/` with: - `pnpm storybook --no-open` 3. Use `agent-browser` directly — **never `npx agent-browser`**. 4. Use named browser sessions and an output folder such as: - `./dogfood-output/advisor/` - with subfolders `screenshots/` and `videos/` ### Evidence protocol For every interactive scenario below: 1. Start video recording **before** the action. 2. Capture step-by-step screenshots at human pace. 3. Capture one annotated screenshot of the final state. 4. Stop the recording. 5. Note the exact pass/fail observation in the QA report. For static UI states (for example Storybook error/limit cards), an annotated screenshot is sufficient; video is optional but still encouraged by this project’s review preference. ### Dogfood scenarios #### Scenario A — Happy path in the real Agents UI **Goal:** prove that a root agent chat can invoke advisor and produce a readable recommendation before taking further action. Steps: 1. Open the Agents page with an advisor-enabled root chat. 2. Start a repro video. 3. Send a prompt that should reasonably trigger strategic planning, such as an architecture or multi-tradeoff question. 4. Capture screenshots of: - the prompt before send; - the running advisor state; - the completed advisor card and the assistant’s follow-up response. 5. Stop recording. Pass criteria: - advisor appears in the timeline; - the rendered result is readable; - the assistant can continue after consuming the advisor output. #### Scenario B — Advisor unavailable path **Goal:** prove the feature is truly gated. Suggested variants (at least one is required, both are better): - feature flag/config off; - child/sub-agent chat. Evidence: - annotated screenshot of the chat/tool state showing advisor is absent; - short video if toggling the gate live is part of the repro. Pass criteria: - no advisor tool is available; - no advisor-specific prompt behavior leaks through. #### Scenario C — UI states in Storybook **Goal:** prove the renderer handles non-happy states cleanly. Required story states: - success/advice; - running; - limit reached; - error. Evidence: - one screenshot per state; - at least one short video showing collapse/expand behavior. Pass criteria: - success renders readable advice; - limit/error have visible fallback text; - the component behaves like the other tool cards. #### Scenario D — Regression sweep of nearby tools **Goal:** ensure advisor does not break the surrounding chat timeline. Check at minimum: - another existing built-in tool still renders correctly near advisor; - sub-agent/tool cards still expand/collapse normally; - no obvious console errors appear in the Agents page during the advisor flow. Evidence: - screenshots of adjacent tool cards; - console/error capture if anything suspicious appears. ### `agent-browser` usage notes for the QA agent - Prefer `agent-browser batch` for 2+ sequential commands when no intermediate parsing is needed. - Use `snapshot -i` to discover interactive refs. - Re-snapshot after navigation or major DOM changes. - Avoid `wait --load networkidle` unless the page is known to go idle; prefer explicit element/text waits or short fixed waits. - Record videos at human pace and include pauses that a reviewer can follow. ## Rollout plan ### Initial rollout - Gate behind a server-side advisor-enabled flag. - Enable only for selected internal/root agent chats first. - Watch metrics for: - invocation count; - failure rate; - latency; - obvious retry loops. ### Expansion conditions Expand beyond the initial rollout only after the following are true: - mixed-batch policy behavior is stable; - cost impact is understood; - frontend UX is readable in production-like dogfood; - no recursion surprises have appeared with sub-agent flows. ### Explicit non-goals for the first release - advisor inside child/sub-agent chats; - provider-agnostic streaming phase UI; - MCP-based external advisor implementation; - mandatory DB-backed advisor cost reporting. ## Final acceptance checklist - [ ] `advisor` is a built-in chatd tool, not an MCP/dynamic-tool substitute. - [ ] The nested advisor call is tool-less and bounded to one in-memory step. - [ ] One eligibility boolean controls both tool registration and prompt guidance injection. - [ ] Root chats can use advisor; child chats cannot in the initial rollout. - [ ] Mixed advisor/action batches produce deterministic policy errors instead of partial execution. - [ ] Per-run usage caps and limit-reached behavior work. - [ ] Advisor usage is visible in metadata/metrics without forcing a DB migration for MVP. - [ ] The Agents UI has a readable advisor card and Storybook coverage. - [ ] Dogfooding produced screenshots and repro videos for the required scenarios. - [ ] Validation commands (`make lint`, targeted `make test`, Storybook tests, `make pre-commit`) passed before handoff. ## Suggested PR split 1. **PR 1 — Backend foundation** - `chatadvisor/` package - `chattool/advisor.go` - `chatloop` exclusive policy - chatd gating/prompt sync - backend tests 2. **PR 2 — Frontend + QA** - advisor renderer - stories/play assertions - dogfood artifacts and QA notes 3. **PR 3 — Optional follow-ups only if demanded by stakeholders** - separate advisor model override - persistent advisor billing/queryability - transient phase-stream UX </details> --- _Generated with [`mux`](https://github.com/coder/mux) • Model: `anthropic:claude-opus-4-7` • Thinking: `max`_ |
||
|
Thomas Kosiewski
|
17409a515c |
feat(coderd): wire advisor runtime to admin config (#24622)
## Summary Wire the advisor runtime into `chatd`: read the admin config on every `runChat`, gate tool registration and system-prompt guidance on a **single eligibility boolean**, register the `advisor` built-in tool, and apply the exclusive-tool policy from PR 1. ## Motivation This is the integration seam where PRs 1–3 come together into an actual user-visible feature. Gating is deliberately root-chat-only for the initial rollout; child/sub-agent chats still do not see the tool or the guidance block. ## Changes ### `coderd/x/chatd/chatd.go` - `loadAdvisorConfig(ctx, logger)` reads the admin config (from PR 3) on each run. If `ModelConfigID` is set, it resolves the override model via `configCache.ModelConfigByID`; otherwise it falls back to the outer chat's model and provider options. Reasoning effort is plumbed into provider options via `applyAdvisorReasoningEffort`. - One computed `advisorEligible` boolean drives **both** tool registration (after skill tools, before MCP tools) and guidance injection via `chatprompt.InsertSystem(prompt, chatadvisor.ParentGuidanceBlock)`. - `setAdvisorPromptSnapshot` closures capture the outer prompt state at the right points in the lifecycle (`renderPlanPathPrompt`, `ReloadMessages`, `PrepareMessages`) so the advisor handoff uses the same context the outer model saw. - `ExclusiveToolNames["advisor"] = true` is passed to `chatloop.Run()` so mixed batches are rejected cleanly (PR 1 machinery). - `builtinToolNames["advisor"] = true` so metrics keep advisor distinct from the generic `mcp` label. ### Child-chat guard - Child/sub-agent chats deliberately do not see the advisor tool or guidance block, to avoid recursion/cost blowups until the pattern is proven. This is covered by `TestAdvisorGating_ChildChat` (currently skipped pending a rewrite against the new `plan`/`explore` subagent infrastructure; core gating logic is still exercised by `TestAdvisorGating_Disabled` and `TestAdvisorGating_RootChat`). ## Stack context This is **PR 4 of 6** in the advisor feature stack. It depends on PRs 1–3. ## Scope / non-goals - No frontend changes. The feature is invocable via the backend but renders generically until PR 5. - No separate provider runner; the nested advisor call reuses the existing model/provider path. - No DB migration. ## Validation - `go test ./coderd/x/chatd/... -run TestAdvisor` - `go build ./...` - `make lint` --- <details> <summary>📋 Implementation Plan (shared across the advisor stack)</summary> # Plan: Add a Mux-style advisor tool to coder agents/chatd ## Outcome Add a first-class `advisor` tool to agent chats in `coderd/x/chatd` that feels native to Coder: - it is a built-in server-side tool, not an MCP/dynamic-tool workaround; - it performs a nested **tool-less** model call for strategic advice; - it is exposed only when eligible, and the prompt mentions it only when it is actually available; - it is treated as a **planning-only** tool so it does not run alongside action tools in the same batch; - it tracks usage/cost separately enough for operators to reason about it; - it has a minimally polished UI in the Agents page; - and it ships with explicit dogfooding evidence, including screenshots and repro videos. ## Design decisions to lock before coding 1. **Primary architecture:** native built-in tool in `chattool/`, backed by a small `chatadvisor` package. 2. **Nested model execution:** reuse chatd's existing model/provider stack for a one-step, tool-less advisor call rather than inventing a new provider pathway. 3. **Execution policy:** treat `advisor` as an exclusive/planning-only tool; mixed batches must return structured policy errors and force the model to retry cleanly. 4. **Availability:** initial rollout is for root agent chats only; disable for child/sub-agent chats until recursion/cost policy is proven. 5. **Prompt sync:** use one eligibility boolean to drive both tool registration and advisor guidance injection. 6. **Persistence/cost split:** MVP should keep advisor usage visible in result metadata and server metrics; only add DB schema if product/billing explicitly needs queryable advisor-specific cost. 7. **UI scope:** generic tool rendering is an acceptable temporary milestone during backend bring-up, but the release candidate should include a dedicated lightweight advisor renderer. ## Delivery model The work should be executed as coordinated workstreams with one integration owner and parallel contributors for low-conflict areas. The integration owner should own `coderd/x/chatd/chatd.go` because prompt assembly, tool registration, and model resolution all converge there. ## Detailed workstreams ### Repo evidence used for this plan <details> <summary>Mux reference and current chatd seams</summary> **Mux reference implementation** - `src/node/services/tools/advisor.ts` — native advisor tool implementation. - `src/common/constants/advisor.ts` — advisor prompt/constants and truncation policy. - `src/common/utils/tools/tools.ts` — conditional tool registration. - `src/node/services/streamContextBuilder.ts` — injects advisor guidance only when the tool is available. **Current chatd seams** - `coderd/x/chatd/chatd.go` - `processChat()` — tool assembly, prompt assembly, and chatloop invocation. - `resolveChatModel()` — current model/provider/key resolution seam. - `type Config struct` — server-level chatd configuration surface. - `coderd/x/chatd/chatloop/chatloop.go` - `Run()` — main streaming/model loop. - `executeTools()` — built-in tool execution/batching seam. - `coderd/x/chatd/chattool/` — built-in tool implementations. - `site/src/pages/AgentsPage/components/ChatElements/tools/Tool.tsx` — tool renderer dispatch. - `site/src/pages/AgentsPage/components/ChatConversation/messageParsing.ts` and `ConversationTimeline.tsx` — tool/result merge and rendering flow. </details> ### Workstream map and ownership | Workstream | Primary owner | Main files | Can run in parallel? | Done when | |---|---|---|---|---| | 0. Integration + gating | Integration lead | `coderd/x/chatd/chatd.go` | No; central merge lane | Tool registration, prompt sync, and model selection are wired together | | 1. Advisor runtime + tool | Backend agent | new `coderd/x/chatd/chatadvisor/`, new `coderd/x/chatd/chattool/advisor.go` | Yes | Tool can perform a tool-less advisor call in memory and return structured results | | 2. Planning-only execution policy | Chatloop agent | `coderd/x/chatd/chatloop/chatloop.go`, related tests | Yes | Mixed `advisor` + action-tool batches are rejected cleanly and deterministically | | 3. Metrics/usage/config | Backend/telemetry agent | `chatd.go`, `chatloop/metrics.go`, optional config plumbing | Partially; coordinate with integration lead | Advisor usage is separately visible in metadata/metrics and limits are enforced | | 4. Frontend rendering | Frontend agent | `site/.../tools/Tool.tsx`, new `AdvisorTool.tsx`, stories | Yes after result schema stabilizes | Advisor renders as a readable card and story tests pass | | 5. Dogfood + QA evidence | QA agent | dev server, Storybook, dogfood output | After backend + UI are usable | Repro videos, screenshots, and a concise QA report exist | ### Parallelization rules - **Do not split `coderd/x/chatd/chatd.go` across multiple execution agents without an integration lead.** That file owns prompt building, tool registration, model resolution, and cost persistence. - Workstreams 1 and 2 can be developed in parallel and then stacked onto the integration branch. - Workstream 4 should begin once the backend result schema is agreed on, even if the backend is still behind a feature flag. - Any agent that needs to re-check Mux behavior should clone `coder/mux` into a temporary directory (for example, `$(mktemp -d)/mux`) and inspect it read-only; do not vendor or copy code from Mux directly. ## Phase 0 — Preflight and guardrails ### Goals - Align the team on the smallest shippable architecture. - Prevent scope creep into MCP/dynamic-tool/sub-agent variants. - Decide upfront what is MVP vs. follow-up. ### Tasks 1. **Confirm the MVP boundary.** - Ship a built-in advisor tool first. - Do **not** make MCP, dynamic tools, or sub-agents the primary implementation. - Do **not** add transient streaming phases in the first backend PR unless they fall out almost for free. 2. **Confirm local workflow hygiene before coding.** - Ensure the repo is using the project git hooks from `scripts/githooks`. - Do not bypass hooks with `--no-verify`. - Use `./scripts/develop.sh` for the full dev server rather than manual build/run commands. 3. **Lock the model-selection policy.** - **Recommended MVP:** advisor uses the same resolved provider/model/cost config as the current chat, with advisor-specific max-output and usage caps. - **Follow-up only if required:** add a separate `AdvisorModelConfigID`-style override that resolves through the existing `configCache`/model-config path. Do not invent a new free-form `provider:model` parser if chatd already stores provider/model separately. 4. **Lock the persistence policy.** - **Recommended MVP:** no DB migration. Persist advisor-visible metadata in the tool result and record separate metrics in memory/Prometheus. - **Only if product/billing explicitly asks for queryable advisor cost:** add a later DB migration or usage table, following the normal `queries/*.sql` + `make gen` workflow. 5. **Create an execution ADR note in the work item or tracking doc.** - Capture: built-in tool, tool-less nested call, root-chat-only rollout, exclusive execution policy, MVP no-DB-migration default. ### Quality gate - Everyone on the team can state the same answers to these questions: - Is advisor a built-in tool? **Yes.** - Can advisor run with action tools in the same batch? **No.** - Does advisor get tools of its own? **No.** - Is a DB migration required for MVP? **No, unless billing insists.** ## Phase 1 — Build the advisor runtime and tool wrapper ### Goals Create the core advisor implementation in a way that is easy to test and keeps `chattool/` thin. ### Files to add - `coderd/x/chatd/chatadvisor/types.go` - `coderd/x/chatd/chatadvisor/guidance.go` - `coderd/x/chatd/chatadvisor/handoff.go` - `coderd/x/chatd/chatadvisor/runtime.go` - `coderd/x/chatd/chatadvisor/runner.go` - `coderd/x/chatd/chattool/advisor.go` ### Responsibilities by file 1. **`types.go`** - Define the input/result schema used by the tool and UI. - Keep the result shape close to Mux so the UI and model both have predictable cases. - Recommended result variants: - `advice` - `limit_reached` - `error` Recommended shape: ```go type AdvisorArgs struct { Question string `json:"question"` } type AdvisorResult struct { Type string `json:"type"` Advice string `json:"advice,omitempty"` Error string `json:"error,omitempty"` AdvisorModel string `json:"advisor_model,omitempty"` RemainingUses int `json:"remaining_uses,omitempty"` Usage *AdvisorUsageResult `json:"usage,omitempty"` } ``` 2. **`guidance.go`** - Hold two strings: - the nested advisor system prompt; - the parent-agent guidance block to inject into the outer system prompt. - The nested advisor prompt must say, in plain language: - you are advising the parent agent; - you do not address the end user directly; - you do not claim actions happened; - you return concise strategic guidance and tradeoffs. 3. **`runtime.go`** - Define the per-run runtime state. - Recommended fields: - resolved model + model config; - provider keys/options reused from the outer chat; - `MaxUsesPerRun`; - `MaxOutputTokens`; - atomic/current call counter; - callback(s) to obtain the current prompt snapshot and current-step snapshot; - optional metrics/usage hook. - Add fail-fast validation for impossible config: nil model, non-positive limits, empty prompt builders, etc. 4. **`handoff.go`** - Build the advisor handoff message from: - the explicit question; - the exact prompt/messages the parent model just used; - the current step's text/reasoning snapshot, if available; - the most recent relevant tool outputs, if they are already in the prompt snapshot. - **Important:** use the already-prepared outer prompt tail, not a fresh DB reload. That keeps the advisor aligned with compaction and the exact context the outer model saw. - Apply hard truncation budgets with recent-context bias. 5. **`runner.go`** - Execute the nested advisor call. - **Recommended implementation:** call `chatloop.Run()` in an in-memory, one-step mode: - `Tools: nil` - `ProviderTools: nil` - `MaxSteps: 1` - `PersistStep`: capture the assistant output in memory instead of writing DB rows - Reuse the existing provider/model/cost path instead of building a second provider runner. - Assert that no tool definitions are passed to the nested call. 6. **`chattool/advisor.go`** - Keep this file thin and consistent with other built-ins. - Responsibilities: - decode `AdvisorArgs`; - validate `Question` is non-empty and bounded; - call the `chatadvisor` runner; - return a structured tool response. ### Defensive programming requirements - Assert `Question` is non-empty after trimming. - Assert runtime limits are positive. - Assert the nested advisor call runs with zero tools/provider tools. - Assert `AdvisorResult.Type` is one of the known variants before returning. - Assert remaining uses never goes negative. ### Acceptance criteria - A unit test can call the advisor tool with a fake model and receive a stable `advice` result. - The nested advisor call is impossible to run with tools accidentally attached. - The core logic lives in `chatadvisor/`, not embedded inside `chatd.go`. ## Phase 2 — Wire advisor into chatd and keep prompt/tool availability in sync ### Goals Register the tool in the right place, expose it only when eligible, and inject system guidance only when the tool is present. ### Files to modify - `coderd/x/chatd/chatd.go` - optionally a small helper file if `chatd.go` becomes too crowded ### Tasks 1. **Compute one eligibility boolean in `processChat()`.** Recommended inputs: - server-level advisor enabled flag; - root chat only (`chat.ParentChatID == uuid.Nil` or equivalent existing root/child check); - a usable resolved model/provider exists; - optional experiment/workspace/org gate if product wants staged rollout. 2. **Create the runtime once per outer chat run.** - Use the model/config/keys resolved by `resolveChatModel()`. - Reuse provider options from the current chat's `ChatModelCallConfig`. - Set `MaxUsesPerRun` and `MaxOutputTokens` from advisor config defaults. 3. **Register the tool in the built-in tool block.** - Insert after the skill tools and before MCP tools in `processChat()`. - Record `builtinToolNames["advisor"] = true` so metrics stay bounded. 4. **Inject advisor guidance into the outer system prompt using the same boolean.** - Use `chatprompt.InsertSystem()` in the same prompt assembly path that already injects user/system instructions. - Place the block near the existing instruction insertion, before plan-path/skill context blocks. - Wrap the guidance in an explicit tag like `<advisor-guidance>` so it is easy to spot in tests and future refactors. 5. **Keep advisor out of child chats for the first release.** - That avoids recursion/cost blowups with `spawn_agent` / `wait_agent` flows. - Document this explicitly in the rollout notes and tests. ### Acceptance criteria - If advisor is disabled, neither the tool nor the prompt guidance appears. - If advisor is enabled, both the tool and the prompt guidance appear. - Root chats can use advisor; child chats cannot. - Built-in tool names include `advisor` so metrics do not collapse it into the generic `mcp` label. ## Phase 3 — Enforce planning-only execution policy in `chatloop` ### Goals Prevent the model from calling `advisor` and action tools in the same execution batch. ### Files to modify - `coderd/x/chatd/chatloop/chatloop.go` - related chatloop tests ### Recommended implementation Keep the MVP small; do **not** build a general policy engine yet. 1. Add a minimal field to `chatloop.RunOptions`, for example: ```go ExclusiveToolName *string ``` 2. In `Run()` / `executeTools()`, detect the case where the exclusive tool appears in the same local-tool batch as any other locally executed tool. 3. When that happens, synthesize structured tool-result errors for the affected calls instead of executing anything in the batch. - `advisor` should receive a clear error like: _advisor must be called by itself before action tools_. - The sibling action tools should receive a paired policy error like: _this tool was skipped because advisor must run alone_. 4. Let the outer model see those tool errors and retry cleanly. - This is simpler and safer than partial execution or hidden deferral. - It preserves deterministic transcript history for debugging. 5. Pass the just-finished step snapshot into the tool execution context. - The advisor runtime should be able to see the current step's text/reasoning content, because that is often the best hint about what the outer model is trying to decide. ### Why this is the right fit - It matches the intended semantics: advisor is consulted **before** taking action. - It avoids subtle race conditions caused by concurrent built-in tool execution. - It keeps the behavior easy to test with fake models. ### Acceptance criteria - A model-emitted batch containing only `advisor` succeeds. - A model-emitted batch containing `advisor` plus any other locally executed tool returns deterministic policy errors and executes nothing. - Non-advisor tool execution stays unchanged for normal chats. ## Phase 4 — Usage limits, metrics, and configuration ### Goals Make advisor safe to operate without over-designing billing/storage in the first release. ### Files to modify - `coderd/x/chatd/chatd.go` - `coderd/x/chatd/chatloop/metrics.go` as needed - `coderd/x/chatd/chatd.go` `Config` struct and constructor path - optional follow-up config/db files only if a separate advisor model or persistent billing is required ### Tasks 1. **Add explicit server config knobs for MVP.** Recommended fields on `chatd.Config` or a nested advisor config struct: - `AdvisorEnabled bool` - `AdvisorMaxUsesPerRun int` - `AdvisorMaxOutputTokens int64` 2. **Track usage per outer run.** - Reset the counter for each `processChat()` invocation. - Return `remaining_uses` in the tool result. - Return `limit_reached` when the cap is exhausted. 3. **Expose advisor usage metadata in the tool result.** - Include model name and token/cost summary if available. - Use the same `callConfig.Cost` calculation path as the outer chat for MVP if advisor reuses the same model. 4. **Record server-side metrics.** - Count advisor invocations, failures, and latency. - Ensure they show up under the built-in tool label `advisor`. 5. **Optional decision gate: separate advisor model.** - If product insists on a stronger/different advisor model, add a follow-up config hook that resolves another existing chat model config through the same `configCache` path. - Keep that out of the first landing PR unless it is required for acceptance. 6. **Optional decision gate: queryable advisor cost.** - If this becomes required, spin a follow-up DB task: - update `coderd/database/queries/*.sql`; - add migration files; - run `make gen`; - update audit mappings if a new auditable type/field is introduced. ### Acceptance criteria - Advisor calls are capped per outer run. - Limit exhaustion is user-visible in the tool result. - Metrics distinguish advisor calls from other built-in tools. - MVP does not require a schema migration unless explicitly approved. ## Phase 5 — Frontend rendering and Storybook coverage ### Goals Make advisor feel intentional in the Agents UI without blocking the backend on fancy streaming UI. ### Files to modify - `site/src/pages/AgentsPage/components/ChatElements/tools/Tool.tsx` - new `site/src/pages/AgentsPage/components/ChatElements/tools/AdvisorTool.tsx` - Storybook story file(s) in the same tools directory ### Delivery strategy 1. **Intermediate milestone during backend bring-up:** rely on the existing generic tool renderer if needed. - This is acceptable only as a short-lived integration checkpoint. 2. **Release milestone:** add a dedicated lightweight `AdvisorTool` renderer. - Reuse existing primitives: - `ToolCollapsible` - `ToolIcon` - `Response` for markdown/prose rendering - `ScrollArea` if the advice can be long - Keep styling light and consistent with the Agents page. - Do not add unnecessary React memoization in `site/src/pages/AgentsPage/`; that area is already React-Compiler aware. 3. **Render the structured result states cleanly.** - `advice` — readable prose/markdown with optional metadata footer. - `limit_reached` — warning-style message. - `error` — error state with visible fallback text. - `running` — existing tool loading state/spinner is enough for MVP. 4. **Add Storybook coverage instead of ad-hoc component tests.** Recommended stories: - successful advice; - running/loading; - limit reached; - error. 5. **Keep the UI contract narrow.** - Prefer one text field like `advice` plus small metadata rather than a deeply nested schema. - That keeps the UI resilient to prompt iteration. ### Acceptance criteria - The advisor tool card renders readable content rather than raw quoted JSON in the final release branch. - Running, limit, and error states are visibly distinct. - Storybook stories and play assertions cover the new states. - Existing tool rendering flows remain unchanged. ## Phase 6 — Automated tests and validation gates ### Backend tests to add 1. **Advisor runtime/tool tests** - question validation; - tool-less nested execution assertion; - success result shaping; - limit-reached result shaping; - error result shaping. 2. **Prompt/gating tests in chatd** - advisor disabled ⇒ no tool, no guidance; - advisor enabled/root chat ⇒ tool + guidance; - child chat ⇒ advisor absent. 3. **Chatloop policy tests** - advisor alone runs; - advisor + action tool mixed batch returns deterministic policy errors; - non-advisor tools still execute normally. 4. **Usage/metrics tests** - per-run cap resets correctly; - builtin tool labeling includes `advisor`; - returned metadata includes model/usage summary when available. ### Frontend tests to add - Storybook `play()` assertions for the advisor renderer states. - Verify expand/collapse behavior and visible fallback text. - Verify the message timeline still renders adjacent tools correctly. ### Recommended command sequence Run these as the implementation matures, not only at the end: 1. Backend-focused gate after phases 1–4: - `make test RUN=TestAdvisor` - `make test RUN=TestChatloopAdvisor` - `make lint` 2. Frontend-focused gate after phase 5: - `pnpm test:storybook src/pages/AgentsPage/components/ChatElements/tools/AdvisorTool.stories.tsx` - `pnpm lint` - `pnpm format` 3. Final repo gate before handoff: - `make pre-commit` - run any additional targeted `make test RUN=...` selections covering touched chatd paths > Use the exact new test names the implementing agents create; the names above are recommended anchors, not existing tests. ## Dogfooding plan ### Principle Dogfood the change as a real agent feature, not just a unit-tested backend. Per the dogfood and `agent-browser` skills, the reviewer should get **watchable repro videos** plus screenshots that make the behavior obvious without reading logs. ### Required setup 1. Start the full dev environment with: - `./scripts/develop.sh` 2. If the frontend renderer changes, also start Storybook from `site/` with: - `pnpm storybook --no-open` 3. Use `agent-browser` directly — **never `npx agent-browser`**. 4. Use named browser sessions and an output folder such as: - `./dogfood-output/advisor/` - with subfolders `screenshots/` and `videos/` ### Evidence protocol For every interactive scenario below: 1. Start video recording **before** the action. 2. Capture step-by-step screenshots at human pace. 3. Capture one annotated screenshot of the final state. 4. Stop the recording. 5. Note the exact pass/fail observation in the QA report. For static UI states (for example Storybook error/limit cards), an annotated screenshot is sufficient; video is optional but still encouraged by this project’s review preference. ### Dogfood scenarios #### Scenario A — Happy path in the real Agents UI **Goal:** prove that a root agent chat can invoke advisor and produce a readable recommendation before taking further action. Steps: 1. Open the Agents page with an advisor-enabled root chat. 2. Start a repro video. 3. Send a prompt that should reasonably trigger strategic planning, such as an architecture or multi-tradeoff question. 4. Capture screenshots of: - the prompt before send; - the running advisor state; - the completed advisor card and the assistant’s follow-up response. 5. Stop recording. Pass criteria: - advisor appears in the timeline; - the rendered result is readable; - the assistant can continue after consuming the advisor output. #### Scenario B — Advisor unavailable path **Goal:** prove the feature is truly gated. Suggested variants (at least one is required, both are better): - feature flag/config off; - child/sub-agent chat. Evidence: - annotated screenshot of the chat/tool state showing advisor is absent; - short video if toggling the gate live is part of the repro. Pass criteria: - no advisor tool is available; - no advisor-specific prompt behavior leaks through. #### Scenario C — UI states in Storybook **Goal:** prove the renderer handles non-happy states cleanly. Required story states: - success/advice; - running; - limit reached; - error. Evidence: - one screenshot per state; - at least one short video showing collapse/expand behavior. Pass criteria: - success renders readable advice; - limit/error have visible fallback text; - the component behaves like the other tool cards. #### Scenario D — Regression sweep of nearby tools **Goal:** ensure advisor does not break the surrounding chat timeline. Check at minimum: - another existing built-in tool still renders correctly near advisor; - sub-agent/tool cards still expand/collapse normally; - no obvious console errors appear in the Agents page during the advisor flow. Evidence: - screenshots of adjacent tool cards; - console/error capture if anything suspicious appears. ### `agent-browser` usage notes for the QA agent - Prefer `agent-browser batch` for 2+ sequential commands when no intermediate parsing is needed. - Use `snapshot -i` to discover interactive refs. - Re-snapshot after navigation or major DOM changes. - Avoid `wait --load networkidle` unless the page is known to go idle; prefer explicit element/text waits or short fixed waits. - Record videos at human pace and include pauses that a reviewer can follow. ## Rollout plan ### Initial rollout - Gate behind a server-side advisor-enabled flag. - Enable only for selected internal/root agent chats first. - Watch metrics for: - invocation count; - failure rate; - latency; - obvious retry loops. ### Expansion conditions Expand beyond the initial rollout only after the following are true: - mixed-batch policy behavior is stable; - cost impact is understood; - frontend UX is readable in production-like dogfood; - no recursion surprises have appeared with sub-agent flows. ### Explicit non-goals for the first release - advisor inside child/sub-agent chats; - provider-agnostic streaming phase UI; - MCP-based external advisor implementation; - mandatory DB-backed advisor cost reporting. ## Final acceptance checklist - [ ] `advisor` is a built-in chatd tool, not an MCP/dynamic-tool substitute. - [ ] The nested advisor call is tool-less and bounded to one in-memory step. - [ ] One eligibility boolean controls both tool registration and prompt guidance injection. - [ ] Root chats can use advisor; child chats cannot in the initial rollout. - [ ] Mixed advisor/action batches produce deterministic policy errors instead of partial execution. - [ ] Per-run usage caps and limit-reached behavior work. - [ ] Advisor usage is visible in metadata/metrics without forcing a DB migration for MVP. - [ ] The Agents UI has a readable advisor card and Storybook coverage. - [ ] Dogfooding produced screenshots and repro videos for the required scenarios. - [ ] Validation commands (`make lint`, targeted `make test`, Storybook tests, `make pre-commit`) passed before handoff. ## Suggested PR split 1. **PR 1 — Backend foundation** - `chatadvisor/` package - `chattool/advisor.go` - `chatloop` exclusive policy - chatd gating/prompt sync - backend tests 2. **PR 2 — Frontend + QA** - advisor renderer - stories/play assertions - dogfood artifacts and QA notes 3. **PR 3 — Optional follow-ups only if demanded by stakeholders** - separate advisor model override - persistent advisor billing/queryability - transient phase-stream UX </details> --- _Generated with [`mux`](https://github.com/coder/mux) • Model: `anthropic:claude-opus-4-7` • Thinking: `max`_ |
||
|
Thomas Kosiewski
|
06bad73df4 |
feat: add admin-configurable advisor API, SDK, and queries (#24621)
## Summary
Add the **admin-configurable advisor configuration**: database-backed storage, SDK types, and the experimental HTTP handlers that back the admin settings UI (later PRs). Follows the same "site-configs" pattern as Virtual Desktop.
## Motivation
The advisor needs runtime-tunable knobs (enable/disable, per-run cap, max output tokens, reasoning effort, optional model override) without a service restart or redeploy. Using the existing `site_configs` K/V table keeps this pattern consistent with other admin features and avoids a bespoke schema.
## Changes
### Database (`coderd/database/queries/siteconfig.sql`)
- `GetChatAdvisorConfig` returns the stored JSON blob (default `'{}'`) under key `agents_advisor_config`.
- `UpsertChatAdvisorConfig` uses the standard `INSERT ... ON CONFLICT` pattern.
- Regenerated via `make gen` (queries.sql.go + mocks).
### SDK (`codersdk/chats.go`)
- `AdvisorConfig` type with `Enabled`, `MaxUsesPerRun`, `MaxOutputTokens`, `ReasoningEffort` (`""` / `low` / `medium` / `high`), `ModelConfigID uuid.UUID`.
- Client methods: `ChatAdvisorConfig(ctx)` / `UpdateChatAdvisorConfig(ctx, cfg)`.
### API (`coderd/exp_chats.go`)
- `GET /api/experimental/chats/config/advisor`: reads current config; relies on `ActorFromContext` validation.
- `PUT /api/experimental/chats/config/advisor`: requires `policy.ActionUpdate` on `rbac.ResourceDeploymentConfig`.
- Handlers unmarshal `{}` to a typed zero value and re-marshal on upsert for schema stability.
- Tests in `exp_chats_test.go` cover empty defaults, round-trip update, unauthorized update, and invalid body.
## Stack context
This is **PR 3 of 6** in the advisor feature stack. Consumed by:
- PR 4 (`feat/advisor-04-chatd-runtime`), which reads this config on every `runChat`.
- PR 6 (`feat/advisor-06-admin-settings-ui`), which renders the admin form.
## Scope / non-goals
- No `chatd` read path (lands in PR 4).
- No UI (lands in PR 6).
- `agents_advisor_config` remains a single-row JSON blob; we intentionally do not shard per-org/per-template yet.
## Validation
- `make gen`
- `go test ./coderd/database/... -run TestChatAdvisor`
- `go test ./coderd/... -run TestChatAdvisorConfig`
- `make lint`
---
<details>
<summary>📋 Implementation Plan (shared across the advisor stack)</summary>
# Plan: Add a Mux-style advisor tool to coder agents/chatd
## Outcome
Add a first-class `advisor` tool to agent chats in `coderd/x/chatd` that feels native to Coder:
- it is a built-in server-side tool, not an MCP/dynamic-tool workaround;
- it performs a nested **tool-less** model call for strategic advice;
- it is exposed only when eligible, and the prompt mentions it only when it is actually available;
- it is treated as a **planning-only** tool so it does not run alongside action tools in the same batch;
- it tracks usage/cost separately enough for operators to reason about it;
- it has a minimally polished UI in the Agents page;
- and it ships with explicit dogfooding evidence, including screenshots and repro videos.
## Design decisions to lock before coding
1. **Primary architecture:** native built-in tool in `chattool/`, backed by a small `chatadvisor` package.
2. **Nested model execution:** reuse chatd's existing model/provider stack for a one-step, tool-less advisor call rather than inventing a new provider pathway.
3. **Execution policy:** treat `advisor` as an exclusive/planning-only tool; mixed batches must return structured policy errors and force the model to retry cleanly.
4. **Availability:** initial rollout is for root agent chats only; disable for child/sub-agent chats until recursion/cost policy is proven.
5. **Prompt sync:** use one eligibility boolean to drive both tool registration and advisor guidance injection.
6. **Persistence/cost split:** MVP should keep advisor usage visible in result metadata and server metrics; only add DB schema if product/billing explicitly needs queryable advisor-specific cost.
7. **UI scope:** generic tool rendering is an acceptable temporary milestone during backend bring-up, but the release candidate should include a dedicated lightweight advisor renderer.
## Delivery model
The work should be executed as coordinated workstreams with one integration owner and parallel contributors for low-conflict areas. The integration owner should own `coderd/x/chatd/chatd.go` because prompt assembly, tool registration, and model resolution all converge there.
## Detailed workstreams
### Repo evidence used for this plan
<details>
<summary>Mux reference and current chatd seams</summary>
**Mux reference implementation**
- `src/node/services/tools/advisor.ts` — native advisor tool implementation.
- `src/common/constants/advisor.ts` — advisor prompt/constants and truncation policy.
- `src/common/utils/tools/tools.ts` — conditional tool registration.
- `src/node/services/streamContextBuilder.ts` — injects advisor guidance only when the tool is available.
**Current chatd seams**
- `coderd/x/chatd/chatd.go`
- `processChat()` — tool assembly, prompt assembly, and chatloop invocation.
- `resolveChatModel()` — current model/provider/key resolution seam.
- `type Config struct` — server-level chatd configuration surface.
- `coderd/x/chatd/chatloop/chatloop.go`
- `Run()` — main streaming/model loop.
- `executeTools()` — built-in tool execution/batching seam.
- `coderd/x/chatd/chattool/` — built-in tool implementations.
- `site/src/pages/AgentsPage/components/ChatElements/tools/Tool.tsx` — tool renderer dispatch.
- `site/src/pages/AgentsPage/components/ChatConversation/messageParsing.ts` and `ConversationTimeline.tsx` — tool/result merge and rendering flow.
</details>
### Workstream map and ownership
| Workstream | Primary owner | Main files | Can run in parallel? | Done when |
|---|---|---|---|---|
| 0. Integration + gating | Integration lead | `coderd/x/chatd/chatd.go` | No; central merge lane | Tool registration, prompt sync, and model selection are wired together |
| 1. Advisor runtime + tool | Backend agent | new `coderd/x/chatd/chatadvisor/`, new `coderd/x/chatd/chattool/advisor.go` | Yes | Tool can perform a tool-less advisor call in memory and return structured results |
| 2. Planning-only execution policy | Chatloop agent | `coderd/x/chatd/chatloop/chatloop.go`, related tests | Yes | Mixed `advisor` + action-tool batches are rejected cleanly and deterministically |
| 3. Metrics/usage/config | Backend/telemetry agent | `chatd.go`, `chatloop/metrics.go`, optional config plumbing | Partially; coordinate with integration lead | Advisor usage is separately visible in metadata/metrics and limits are enforced |
| 4. Frontend rendering | Frontend agent | `site/.../tools/Tool.tsx`, new `AdvisorTool.tsx`, stories | Yes after result schema stabilizes | Advisor renders as a readable card and story tests pass |
| 5. Dogfood + QA evidence | QA agent | dev server, Storybook, dogfood output | After backend + UI are usable | Repro videos, screenshots, and a concise QA report exist |
### Parallelization rules
- **Do not split `coderd/x/chatd/chatd.go` across multiple execution agents without an integration lead.** That file owns prompt building, tool registration, model resolution, and cost persistence.
- Workstreams 1 and 2 can be developed in parallel and then stacked onto the integration branch.
- Workstream 4 should begin once the backend result schema is agreed on, even if the backend is still behind a feature flag.
- Any agent that needs to re-check Mux behavior should clone `coder/mux` into a temporary directory (for example, `$(mktemp -d)/mux`) and inspect it read-only; do not vendor or copy code from Mux directly.
## Phase 0 — Preflight and guardrails
### Goals
- Align the team on the smallest shippable architecture.
- Prevent scope creep into MCP/dynamic-tool/sub-agent variants.
- Decide upfront what is MVP vs. follow-up.
### Tasks
1. **Confirm the MVP boundary.**
- Ship a built-in advisor tool first.
- Do **not** make MCP, dynamic tools, or sub-agents the primary implementation.
- Do **not** add transient streaming phases in the first backend PR unless they fall out almost for free.
2. **Confirm local workflow hygiene before coding.**
- Ensure the repo is using the project git hooks from `scripts/githooks`.
- Do not bypass hooks with `--no-verify`.
- Use `./scripts/develop.sh` for the full dev server rather than manual build/run commands.
3. **Lock the model-selection policy.**
- **Recommended MVP:** advisor uses the same resolved provider/model/cost config as the current chat, with advisor-specific max-output and usage caps.
- **Follow-up only if required:** add a separate `AdvisorModelConfigID`-style override that resolves through the existing `configCache`/model-config path. Do not invent a new free-form `provider:model` parser if chatd already stores provider/model separately.
4. **Lock the persistence policy.**
- **Recommended MVP:** no DB migration. Persist advisor-visible metadata in the tool result and record separate metrics in memory/Prometheus.
- **Only if product/billing explicitly asks for queryable advisor cost:** add a later DB migration or usage table, following the normal `queries/*.sql` + `make gen` workflow.
5. **Create an execution ADR note in the work item or tracking doc.**
- Capture: built-in tool, tool-less nested call, root-chat-only rollout, exclusive execution policy, MVP no-DB-migration default.
### Quality gate
- Everyone on the team can state the same answers to these questions:
- Is advisor a built-in tool? **Yes.**
- Can advisor run with action tools in the same batch? **No.**
- Does advisor get tools of its own? **No.**
- Is a DB migration required for MVP? **No, unless billing insists.**
## Phase 1 — Build the advisor runtime and tool wrapper
### Goals
Create the core advisor implementation in a way that is easy to test and keeps `chattool/` thin.
### Files to add
- `coderd/x/chatd/chatadvisor/types.go`
- `coderd/x/chatd/chatadvisor/guidance.go`
- `coderd/x/chatd/chatadvisor/handoff.go`
- `coderd/x/chatd/chatadvisor/runtime.go`
- `coderd/x/chatd/chatadvisor/runner.go`
- `coderd/x/chatd/chattool/advisor.go`
### Responsibilities by file
1. **`types.go`**
- Define the input/result schema used by the tool and UI.
- Keep the result shape close to Mux so the UI and model both have predictable cases.
- Recommended result variants:
- `advice`
- `limit_reached`
- `error`
Recommended shape:
```go
type AdvisorArgs struct {
Question string `json:"question"`
}
type AdvisorResult struct {
Type string `json:"type"`
Advice string `json:"advice,omitempty"`
Error string `json:"error,omitempty"`
AdvisorModel string `json:"advisor_model,omitempty"`
RemainingUses int `json:"remaining_uses,omitempty"`
Usage *AdvisorUsageResult `json:"usage,omitempty"`
}
```
2. **`guidance.go`**
- Hold two strings:
- the nested advisor system prompt;
- the parent-agent guidance block to inject into the outer system prompt.
- The nested advisor prompt must say, in plain language:
- you are advising the parent agent;
- you do not address the end user directly;
- you do not claim actions happened;
- you return concise strategic guidance and tradeoffs.
3. **`runtime.go`**
- Define the per-run runtime state.
- Recommended fields:
- resolved model + model config;
- provider keys/options reused from the outer chat;
- `MaxUsesPerRun`;
- `MaxOutputTokens`;
- atomic/current call counter;
- callback(s) to obtain the current prompt snapshot and current-step snapshot;
- optional metrics/usage hook.
- Add fail-fast validation for impossible config: nil model, non-positive limits, empty prompt builders, etc.
4. **`handoff.go`**
- Build the advisor handoff message from:
- the explicit question;
- the exact prompt/messages the parent model just used;
- the current step's text/reasoning snapshot, if available;
- the most recent relevant tool outputs, if they are already in the prompt snapshot.
- **Important:** use the already-prepared outer prompt tail, not a fresh DB reload. That keeps the advisor aligned with compaction and the exact context the outer model saw.
- Apply hard truncation budgets with recent-context bias.
5. **`runner.go`**
- Execute the nested advisor call.
- **Recommended implementation:** call `chatloop.Run()` in an in-memory, one-step mode:
- `Tools: nil`
- `ProviderTools: nil`
- `MaxSteps: 1`
- `PersistStep`: capture the assistant output in memory instead of writing DB rows
- Reuse the existing provider/model/cost path instead of building a second provider runner.
- Assert that no tool definitions are passed to the nested call.
6. **`chattool/advisor.go`**
- Keep this file thin and consistent with other built-ins.
- Responsibilities:
- decode `AdvisorArgs`;
- validate `Question` is non-empty and bounded;
- call the `chatadvisor` runner;
- return a structured tool response.
### Defensive programming requirements
- Assert `Question` is non-empty after trimming.
- Assert runtime limits are positive.
- Assert the nested advisor call runs with zero tools/provider tools.
- Assert `AdvisorResult.Type` is one of the known variants before returning.
- Assert remaining uses never goes negative.
### Acceptance criteria
- A unit test can call the advisor tool with a fake model and receive a stable `advice` result.
- The nested advisor call is impossible to run with tools accidentally attached.
- The core logic lives in `chatadvisor/`, not embedded inside `chatd.go`.
## Phase 2 — Wire advisor into chatd and keep prompt/tool availability in sync
### Goals
Register the tool in the right place, expose it only when eligible, and inject system guidance only when the tool is present.
### Files to modify
- `coderd/x/chatd/chatd.go`
- optionally a small helper file if `chatd.go` becomes too crowded
### Tasks
1. **Compute one eligibility boolean in `processChat()`.**
Recommended inputs:
- server-level advisor enabled flag;
- root chat only (`chat.ParentChatID == uuid.Nil` or equivalent existing root/child check);
- a usable resolved model/provider exists;
- optional experiment/workspace/org gate if product wants staged rollout.
2. **Create the runtime once per outer chat run.**
- Use the model/config/keys resolved by `resolveChatModel()`.
- Reuse provider options from the current chat's `ChatModelCallConfig`.
- Set `MaxUsesPerRun` and `MaxOutputTokens` from advisor config defaults.
3. **Register the tool in the built-in tool block.**
- Insert after the skill tools and before MCP tools in `processChat()`.
- Record `builtinToolNames["advisor"] = true` so metrics stay bounded.
4. **Inject advisor guidance into the outer system prompt using the same boolean.**
- Use `chatprompt.InsertSystem()` in the same prompt assembly path that already injects user/system instructions.
- Place the block near the existing instruction insertion, before plan-path/skill context blocks.
- Wrap the guidance in an explicit tag like `<advisor-guidance>` so it is easy to spot in tests and future refactors.
5. **Keep advisor out of child chats for the first release.**
- That avoids recursion/cost blowups with `spawn_agent` / `wait_agent` flows.
- Document this explicitly in the rollout notes and tests.
### Acceptance criteria
- If advisor is disabled, neither the tool nor the prompt guidance appears.
- If advisor is enabled, both the tool and the prompt guidance appear.
- Root chats can use advisor; child chats cannot.
- Built-in tool names include `advisor` so metrics do not collapse it into the generic `mcp` label.
## Phase 3 — Enforce planning-only execution policy in `chatloop`
### Goals
Prevent the model from calling `advisor` and action tools in the same execution batch.
### Files to modify
- `coderd/x/chatd/chatloop/chatloop.go`
- related chatloop tests
### Recommended implementation
Keep the MVP small; do **not** build a general policy engine yet.
1. Add a minimal field to `chatloop.RunOptions`, for example:
```go
ExclusiveToolName *string
```
2. In `Run()` / `executeTools()`, detect the case where the exclusive tool appears in the same local-tool batch as any other locally executed tool.
3. When that happens, synthesize structured tool-result errors for the affected calls instead of executing anything in the batch.
- `advisor` should receive a clear error like: _advisor must be called by itself before action tools_.
- The sibling action tools should receive a paired policy error like: _this tool was skipped because advisor must run alone_.
4. Let the outer model see those tool errors and retry cleanly.
- This is simpler and safer than partial execution or hidden deferral.
- It preserves deterministic transcript history for debugging.
5. Pass the just-finished step snapshot into the tool execution context.
- The advisor runtime should be able to see the current step's text/reasoning content, because that is often the best hint about what the outer model is trying to decide.
### Why this is the right fit
- It matches the intended semantics: advisor is consulted **before** taking action.
- It avoids subtle race conditions caused by concurrent built-in tool execution.
- It keeps the behavior easy to test with fake models.
### Acceptance criteria
- A model-emitted batch containing only `advisor` succeeds.
- A model-emitted batch containing `advisor` plus any other locally executed tool returns deterministic policy errors and executes nothing.
- Non-advisor tool execution stays unchanged for normal chats.
## Phase 4 — Usage limits, metrics, and configuration
### Goals
Make advisor safe to operate without over-designing billing/storage in the first release.
### Files to modify
- `coderd/x/chatd/chatd.go`
- `coderd/x/chatd/chatloop/metrics.go` as needed
- `coderd/x/chatd/chatd.go` `Config` struct and constructor path
- optional follow-up config/db files only if a separate advisor model or persistent billing is required
### Tasks
1. **Add explicit server config knobs for MVP.**
Recommended fields on `chatd.Config` or a nested advisor config struct:
- `AdvisorEnabled bool`
- `AdvisorMaxUsesPerRun int`
- `AdvisorMaxOutputTokens int64`
2. **Track usage per outer run.**
- Reset the counter for each `processChat()` invocation.
- Return `remaining_uses` in the tool result.
- Return `limit_reached` when the cap is exhausted.
3. **Expose advisor usage metadata in the tool result.**
- Include model name and token/cost summary if available.
- Use the same `callConfig.Cost` calculation path as the outer chat for MVP if advisor reuses the same model.
4. **Record server-side metrics.**
- Count advisor invocations, failures, and latency.
- Ensure they show up under the built-in tool label `advisor`.
5. **Optional decision gate: separate advisor model.**
- If product insists on a stronger/different advisor model, add a follow-up config hook that resolves another existing chat model config through the same `configCache` path.
- Keep that out of the first landing PR unless it is required for acceptance.
6. **Optional decision gate: queryable advisor cost.**
- If this becomes required, spin a follow-up DB task:
- update `coderd/database/queries/*.sql`;
- add migration files;
- run `make gen`;
- update audit mappings if a new auditable type/field is introduced.
### Acceptance criteria
- Advisor calls are capped per outer run.
- Limit exhaustion is user-visible in the tool result.
- Metrics distinguish advisor calls from other built-in tools.
- MVP does not require a schema migration unless explicitly approved.
## Phase 5 — Frontend rendering and Storybook coverage
### Goals
Make advisor feel intentional in the Agents UI without blocking the backend on fancy streaming UI.
### Files to modify
- `site/src/pages/AgentsPage/components/ChatElements/tools/Tool.tsx`
- new `site/src/pages/AgentsPage/components/ChatElements/tools/AdvisorTool.tsx`
- Storybook story file(s) in the same tools directory
### Delivery strategy
1. **Intermediate milestone during backend bring-up:** rely on the existing generic tool renderer if needed.
- This is acceptable only as a short-lived integration checkpoint.
2. **Release milestone:** add a dedicated lightweight `AdvisorTool` renderer.
- Reuse existing primitives:
- `ToolCollapsible`
- `ToolIcon`
- `Response` for markdown/prose rendering
- `ScrollArea` if the advice can be long
- Keep styling light and consistent with the Agents page.
- Do not add unnecessary React memoization in `site/src/pages/AgentsPage/`; that area is already React-Compiler aware.
3. **Render the structured result states cleanly.**
- `advice` — readable prose/markdown with optional metadata footer.
- `limit_reached` — warning-style message.
- `error` — error state with visible fallback text.
- `running` — existing tool loading state/spinner is enough for MVP.
4. **Add Storybook coverage instead of ad-hoc component tests.**
Recommended stories:
- successful advice;
- running/loading;
- limit reached;
- error.
5. **Keep the UI contract narrow.**
- Prefer one text field like `advice` plus small metadata rather than a deeply nested schema.
- That keeps the UI resilient to prompt iteration.
### Acceptance criteria
- The advisor tool card renders readable content rather than raw quoted JSON in the final release branch.
- Running, limit, and error states are visibly distinct.
- Storybook stories and play assertions cover the new states.
- Existing tool rendering flows remain unchanged.
## Phase 6 — Automated tests and validation gates
### Backend tests to add
1. **Advisor runtime/tool tests**
- question validation;
- tool-less nested execution assertion;
- success result shaping;
- limit-reached result shaping;
- error result shaping.
2. **Prompt/gating tests in chatd**
- advisor disabled ⇒ no tool, no guidance;
- advisor enabled/root chat ⇒ tool + guidance;
- child chat ⇒ advisor absent.
3. **Chatloop policy tests**
- advisor alone runs;
- advisor + action tool mixed batch returns deterministic policy errors;
- non-advisor tools still execute normally.
4. **Usage/metrics tests**
- per-run cap resets correctly;
- builtin tool labeling includes `advisor`;
- returned metadata includes model/usage summary when available.
### Frontend tests to add
- Storybook `play()` assertions for the advisor renderer states.
- Verify expand/collapse behavior and visible fallback text.
- Verify the message timeline still renders adjacent tools correctly.
### Recommended command sequence
Run these as the implementation matures, not only at the end:
1. Backend-focused gate after phases 1–4:
- `make test RUN=TestAdvisor`
- `make test RUN=TestChatloopAdvisor`
- `make lint`
2. Frontend-focused gate after phase 5:
- `pnpm test:storybook src/pages/AgentsPage/components/ChatElements/tools/AdvisorTool.stories.tsx`
- `pnpm lint`
- `pnpm format`
3. Final repo gate before handoff:
- `make pre-commit`
- run any additional targeted `make test RUN=...` selections covering touched chatd paths
> Use the exact new test names the implementing agents create; the names above are recommended anchors, not existing tests.
## Dogfooding plan
### Principle
Dogfood the change as a real agent feature, not just a unit-tested backend. Per the dogfood and `agent-browser` skills, the reviewer should get **watchable repro videos** plus screenshots that make the behavior obvious without reading logs.
### Required setup
1. Start the full dev environment with:
- `./scripts/develop.sh`
2. If the frontend renderer changes, also start Storybook from `site/` with:
- `pnpm storybook --no-open`
3. Use `agent-browser` directly — **never `npx agent-browser`**.
4. Use named browser sessions and an output folder such as:
- `./dogfood-output/advisor/`
- with subfolders `screenshots/` and `videos/`
### Evidence protocol
For every interactive scenario below:
1. Start video recording **before** the action.
2. Capture step-by-step screenshots at human pace.
3. Capture one annotated screenshot of the final state.
4. Stop the recording.
5. Note the exact pass/fail observation in the QA report.
For static UI states (for example Storybook error/limit cards), an annotated screenshot is sufficient; video is optional but still encouraged by this project’s review preference.
### Dogfood scenarios
#### Scenario A — Happy path in the real Agents UI
**Goal:** prove that a root agent chat can invoke advisor and produce a readable recommendation before taking further action.
Steps:
1. Open the Agents page with an advisor-enabled root chat.
2. Start a repro video.
3. Send a prompt that should reasonably trigger strategic planning, such as an architecture or multi-tradeoff question.
4. Capture screenshots of:
- the prompt before send;
- the running advisor state;
- the completed advisor card and the assistant’s follow-up response.
5. Stop recording.
Pass criteria:
- advisor appears in the timeline;
- the rendered result is readable;
- the assistant can continue after consuming the advisor output.
#### Scenario B — Advisor unavailable path
**Goal:** prove the feature is truly gated.
Suggested variants (at least one is required, both are better):
- feature flag/config off;
- child/sub-agent chat.
Evidence:
- annotated screenshot of the chat/tool state showing advisor is absent;
- short video if toggling the gate live is part of the repro.
Pass criteria:
- no advisor tool is available;
- no advisor-specific prompt behavior leaks through.
#### Scenario C — UI states in Storybook
**Goal:** prove the renderer handles non-happy states cleanly.
Required story states:
- success/advice;
- running;
- limit reached;
- error.
Evidence:
- one screenshot per state;
- at least one short video showing collapse/expand behavior.
Pass criteria:
- success renders readable advice;
- limit/error have visible fallback text;
- the component behaves like the other tool cards.
#### Scenario D — Regression sweep of nearby tools
**Goal:** ensure advisor does not break the surrounding chat timeline.
Check at minimum:
- another existing built-in tool still renders correctly near advisor;
- sub-agent/tool cards still expand/collapse normally;
- no obvious console errors appear in the Agents page during the advisor flow.
Evidence:
- screenshots of adjacent tool cards;
- console/error capture if anything suspicious appears.
### `agent-browser` usage notes for the QA agent
- Prefer `agent-browser batch` for 2+ sequential commands when no intermediate parsing is needed.
- Use `snapshot -i` to discover interactive refs.
- Re-snapshot after navigation or major DOM changes.
- Avoid `wait --load networkidle` unless the page is known to go idle; prefer explicit element/text waits or short fixed waits.
- Record videos at human pace and include pauses that a reviewer can follow.
## Rollout plan
### Initial rollout
- Gate behind a server-side advisor-enabled flag.
- Enable only for selected internal/root agent chats first.
- Watch metrics for:
- invocation count;
- failure rate;
- latency;
- obvious retry loops.
### Expansion conditions
Expand beyond the initial rollout only after the following are true:
- mixed-batch policy behavior is stable;
- cost impact is understood;
- frontend UX is readable in production-like dogfood;
- no recursion surprises have appeared with sub-agent flows.
### Explicit non-goals for the first release
- advisor inside child/sub-agent chats;
- provider-agnostic streaming phase UI;
- MCP-based external advisor implementation;
- mandatory DB-backed advisor cost reporting.
## Final acceptance checklist
- [ ] `advisor` is a built-in chatd tool, not an MCP/dynamic-tool substitute.
- [ ] The nested advisor call is tool-less and bounded to one in-memory step.
- [ ] One eligibility boolean controls both tool registration and prompt guidance injection.
- [ ] Root chats can use advisor; child chats cannot in the initial rollout.
- [ ] Mixed advisor/action batches produce deterministic policy errors instead of partial execution.
- [ ] Per-run usage caps and limit-reached behavior work.
- [ ] Advisor usage is visible in metadata/metrics without forcing a DB migration for MVP.
- [ ] The Agents UI has a readable advisor card and Storybook coverage.
- [ ] Dogfooding produced screenshots and repro videos for the required scenarios.
- [ ] Validation commands (`make lint`, targeted `make test`, Storybook tests, `make pre-commit`) passed before handoff.
## Suggested PR split
1. **PR 1 — Backend foundation**
- `chatadvisor/` package
- `chattool/advisor.go`
- `chatloop` exclusive policy
- chatd gating/prompt sync
- backend tests
2. **PR 2 — Frontend + QA**
- advisor renderer
- stories/play assertions
- dogfood artifacts and QA notes
3. **PR 3 — Optional follow-ups only if demanded by stakeholders**
- separate advisor model override
- persistent advisor billing/queryability
- transient phase-stream UX
</details>
---
_Generated with [`mux`](https://github.com/coder/mux) • Model: `anthropic:claude-opus-4-7` • Thinking: `max`_
|
||
|
Thomas Kosiewski
|
eaf2609bb8 |
feat(coderd/x/chatd/chatadvisor): add advisor runtime and tool wrapper (#24620)
## Summary Introduce the `coderd/x/chatd/chatadvisor` package: the self-contained runtime that performs a nested, **tool-less, single-step** model call to return strategic guidance to the parent agent. Also ships the thin `AgentTool` wrapper that the agent exposes as the `advisor` built-in. ## Motivation The advisor is a "consult before you act" planning step. Keeping its runtime in its own package (rather than inside `chatd.go` or `chattool/`) makes the behavior easy to unit-test, keeps `chattool/` thin, and avoids import cycles with `chatprompt`. ## Changes - `chatadvisor/types.go`: `AdvisorArgs` and `AdvisorResult` (`advice` / `limit_reached` / `error` variants) plus usage metadata. Stable JSON shape for the UI. - `chatadvisor/guidance.go`: the nested advisor system prompt and `ParentGuidanceBlock` injected into the outer agent (tagged `<advisor-guidance>`). The nested prompt explicitly tells the advisor it is advising the parent agent, must not address the user, and must not claim actions happened. - `chatadvisor/handoff.go`: builds the advisor input from the already-prepared outer prompt tail (not a fresh DB reload) with hard truncation budgets and a recent-context bias so the advisor sees the exact context the outer model saw. - `chatadvisor/runner.go`: wraps `chatloop.Run()` in strict one-step mode (`Tools: nil`, `ProviderTools: nil`, `MaxSteps: 1`) so the nested call is structurally incapable of calling tools. - `chatadvisor/tool.go`: the `AgentTool` wrapper. Validates the question (non-empty, bounded to 2000 runes), invokes the runtime, and maps the result to a JSON tool response. - Defensive assertions throughout (question non-empty after trim, positive limits, zero tools on nested call, known result variants, non-negative remaining uses). - Unit tests cover question validation, tool-less execution, and each result variant. ## Stack context This is **PR 2 of 6** in the advisor feature stack. It depends on PR 1's `ExclusiveToolNames` hook; no consumer of this package exists yet (that lands in PR 4). ## Scope / non-goals - No wiring into `chatd`. - No HTTP/API surface. - No UI. - No separate provider; the runtime reuses the outer chat's resolved model/provider/keys. ## Validation - `go test ./coderd/x/chatd/chatadvisor/...` - `make lint` --- <details> <summary>📋 Implementation Plan (shared across the advisor stack)</summary> # Plan: Add a Mux-style advisor tool to coder agents/chatd ## Outcome Add a first-class `advisor` tool to agent chats in `coderd/x/chatd` that feels native to Coder: - it is a built-in server-side tool, not an MCP/dynamic-tool workaround; - it performs a nested **tool-less** model call for strategic advice; - it is exposed only when eligible, and the prompt mentions it only when it is actually available; - it is treated as a **planning-only** tool so it does not run alongside action tools in the same batch; - it tracks usage/cost separately enough for operators to reason about it; - it has a minimally polished UI in the Agents page; - and it ships with explicit dogfooding evidence, including screenshots and repro videos. ## Design decisions to lock before coding 1. **Primary architecture:** native built-in tool in `chattool/`, backed by a small `chatadvisor` package. 2. **Nested model execution:** reuse chatd's existing model/provider stack for a one-step, tool-less advisor call rather than inventing a new provider pathway. 3. **Execution policy:** treat `advisor` as an exclusive/planning-only tool; mixed batches must return structured policy errors and force the model to retry cleanly. 4. **Availability:** initial rollout is for root agent chats only; disable for child/sub-agent chats until recursion/cost policy is proven. 5. **Prompt sync:** use one eligibility boolean to drive both tool registration and advisor guidance injection. 6. **Persistence/cost split:** MVP should keep advisor usage visible in result metadata and server metrics; only add DB schema if product/billing explicitly needs queryable advisor-specific cost. 7. **UI scope:** generic tool rendering is an acceptable temporary milestone during backend bring-up, but the release candidate should include a dedicated lightweight advisor renderer. ## Delivery model The work should be executed as coordinated workstreams with one integration owner and parallel contributors for low-conflict areas. The integration owner should own `coderd/x/chatd/chatd.go` because prompt assembly, tool registration, and model resolution all converge there. ## Detailed workstreams ### Repo evidence used for this plan <details> <summary>Mux reference and current chatd seams</summary> **Mux reference implementation** - `src/node/services/tools/advisor.ts` — native advisor tool implementation. - `src/common/constants/advisor.ts` — advisor prompt/constants and truncation policy. - `src/common/utils/tools/tools.ts` — conditional tool registration. - `src/node/services/streamContextBuilder.ts` — injects advisor guidance only when the tool is available. **Current chatd seams** - `coderd/x/chatd/chatd.go` - `processChat()` — tool assembly, prompt assembly, and chatloop invocation. - `resolveChatModel()` — current model/provider/key resolution seam. - `type Config struct` — server-level chatd configuration surface. - `coderd/x/chatd/chatloop/chatloop.go` - `Run()` — main streaming/model loop. - `executeTools()` — built-in tool execution/batching seam. - `coderd/x/chatd/chattool/` — built-in tool implementations. - `site/src/pages/AgentsPage/components/ChatElements/tools/Tool.tsx` — tool renderer dispatch. - `site/src/pages/AgentsPage/components/ChatConversation/messageParsing.ts` and `ConversationTimeline.tsx` — tool/result merge and rendering flow. </details> ### Workstream map and ownership | Workstream | Primary owner | Main files | Can run in parallel? | Done when | |---|---|---|---|---| | 0. Integration + gating | Integration lead | `coderd/x/chatd/chatd.go` | No; central merge lane | Tool registration, prompt sync, and model selection are wired together | | 1. Advisor runtime + tool | Backend agent | new `coderd/x/chatd/chatadvisor/`, new `coderd/x/chatd/chattool/advisor.go` | Yes | Tool can perform a tool-less advisor call in memory and return structured results | | 2. Planning-only execution policy | Chatloop agent | `coderd/x/chatd/chatloop/chatloop.go`, related tests | Yes | Mixed `advisor` + action-tool batches are rejected cleanly and deterministically | | 3. Metrics/usage/config | Backend/telemetry agent | `chatd.go`, `chatloop/metrics.go`, optional config plumbing | Partially; coordinate with integration lead | Advisor usage is separately visible in metadata/metrics and limits are enforced | | 4. Frontend rendering | Frontend agent | `site/.../tools/Tool.tsx`, new `AdvisorTool.tsx`, stories | Yes after result schema stabilizes | Advisor renders as a readable card and story tests pass | | 5. Dogfood + QA evidence | QA agent | dev server, Storybook, dogfood output | After backend + UI are usable | Repro videos, screenshots, and a concise QA report exist | ### Parallelization rules - **Do not split `coderd/x/chatd/chatd.go` across multiple execution agents without an integration lead.** That file owns prompt building, tool registration, model resolution, and cost persistence. - Workstreams 1 and 2 can be developed in parallel and then stacked onto the integration branch. - Workstream 4 should begin once the backend result schema is agreed on, even if the backend is still behind a feature flag. - Any agent that needs to re-check Mux behavior should clone `coder/mux` into a temporary directory (for example, `$(mktemp -d)/mux`) and inspect it read-only; do not vendor or copy code from Mux directly. ## Phase 0 — Preflight and guardrails ### Goals - Align the team on the smallest shippable architecture. - Prevent scope creep into MCP/dynamic-tool/sub-agent variants. - Decide upfront what is MVP vs. follow-up. ### Tasks 1. **Confirm the MVP boundary.** - Ship a built-in advisor tool first. - Do **not** make MCP, dynamic tools, or sub-agents the primary implementation. - Do **not** add transient streaming phases in the first backend PR unless they fall out almost for free. 2. **Confirm local workflow hygiene before coding.** - Ensure the repo is using the project git hooks from `scripts/githooks`. - Do not bypass hooks with `--no-verify`. - Use `./scripts/develop.sh` for the full dev server rather than manual build/run commands. 3. **Lock the model-selection policy.** - **Recommended MVP:** advisor uses the same resolved provider/model/cost config as the current chat, with advisor-specific max-output and usage caps. - **Follow-up only if required:** add a separate `AdvisorModelConfigID`-style override that resolves through the existing `configCache`/model-config path. Do not invent a new free-form `provider:model` parser if chatd already stores provider/model separately. 4. **Lock the persistence policy.** - **Recommended MVP:** no DB migration. Persist advisor-visible metadata in the tool result and record separate metrics in memory/Prometheus. - **Only if product/billing explicitly asks for queryable advisor cost:** add a later DB migration or usage table, following the normal `queries/*.sql` + `make gen` workflow. 5. **Create an execution ADR note in the work item or tracking doc.** - Capture: built-in tool, tool-less nested call, root-chat-only rollout, exclusive execution policy, MVP no-DB-migration default. ### Quality gate - Everyone on the team can state the same answers to these questions: - Is advisor a built-in tool? **Yes.** - Can advisor run with action tools in the same batch? **No.** - Does advisor get tools of its own? **No.** - Is a DB migration required for MVP? **No, unless billing insists.** ## Phase 1 — Build the advisor runtime and tool wrapper ### Goals Create the core advisor implementation in a way that is easy to test and keeps `chattool/` thin. ### Files to add - `coderd/x/chatd/chatadvisor/types.go` - `coderd/x/chatd/chatadvisor/guidance.go` - `coderd/x/chatd/chatadvisor/handoff.go` - `coderd/x/chatd/chatadvisor/runtime.go` - `coderd/x/chatd/chatadvisor/runner.go` - `coderd/x/chatd/chattool/advisor.go` ### Responsibilities by file 1. **`types.go`** - Define the input/result schema used by the tool and UI. - Keep the result shape close to Mux so the UI and model both have predictable cases. - Recommended result variants: - `advice` - `limit_reached` - `error` Recommended shape: ```go type AdvisorArgs struct { Question string `json:"question"` } type AdvisorResult struct { Type string `json:"type"` Advice string `json:"advice,omitempty"` Error string `json:"error,omitempty"` AdvisorModel string `json:"advisor_model,omitempty"` RemainingUses int `json:"remaining_uses,omitempty"` Usage *AdvisorUsageResult `json:"usage,omitempty"` } ``` 2. **`guidance.go`** - Hold two strings: - the nested advisor system prompt; - the parent-agent guidance block to inject into the outer system prompt. - The nested advisor prompt must say, in plain language: - you are advising the parent agent; - you do not address the end user directly; - you do not claim actions happened; - you return concise strategic guidance and tradeoffs. 3. **`runtime.go`** - Define the per-run runtime state. - Recommended fields: - resolved model + model config; - provider keys/options reused from the outer chat; - `MaxUsesPerRun`; - `MaxOutputTokens`; - atomic/current call counter; - callback(s) to obtain the current prompt snapshot and current-step snapshot; - optional metrics/usage hook. - Add fail-fast validation for impossible config: nil model, non-positive limits, empty prompt builders, etc. 4. **`handoff.go`** - Build the advisor handoff message from: - the explicit question; - the exact prompt/messages the parent model just used; - the current step's text/reasoning snapshot, if available; - the most recent relevant tool outputs, if they are already in the prompt snapshot. - **Important:** use the already-prepared outer prompt tail, not a fresh DB reload. That keeps the advisor aligned with compaction and the exact context the outer model saw. - Apply hard truncation budgets with recent-context bias. 5. **`runner.go`** - Execute the nested advisor call. - **Recommended implementation:** call `chatloop.Run()` in an in-memory, one-step mode: - `Tools: nil` - `ProviderTools: nil` - `MaxSteps: 1` - `PersistStep`: capture the assistant output in memory instead of writing DB rows - Reuse the existing provider/model/cost path instead of building a second provider runner. - Assert that no tool definitions are passed to the nested call. 6. **`chattool/advisor.go`** - Keep this file thin and consistent with other built-ins. - Responsibilities: - decode `AdvisorArgs`; - validate `Question` is non-empty and bounded; - call the `chatadvisor` runner; - return a structured tool response. ### Defensive programming requirements - Assert `Question` is non-empty after trimming. - Assert runtime limits are positive. - Assert the nested advisor call runs with zero tools/provider tools. - Assert `AdvisorResult.Type` is one of the known variants before returning. - Assert remaining uses never goes negative. ### Acceptance criteria - A unit test can call the advisor tool with a fake model and receive a stable `advice` result. - The nested advisor call is impossible to run with tools accidentally attached. - The core logic lives in `chatadvisor/`, not embedded inside `chatd.go`. ## Phase 2 — Wire advisor into chatd and keep prompt/tool availability in sync ### Goals Register the tool in the right place, expose it only when eligible, and inject system guidance only when the tool is present. ### Files to modify - `coderd/x/chatd/chatd.go` - optionally a small helper file if `chatd.go` becomes too crowded ### Tasks 1. **Compute one eligibility boolean in `processChat()`.** Recommended inputs: - server-level advisor enabled flag; - root chat only (`chat.ParentChatID == uuid.Nil` or equivalent existing root/child check); - a usable resolved model/provider exists; - optional experiment/workspace/org gate if product wants staged rollout. 2. **Create the runtime once per outer chat run.** - Use the model/config/keys resolved by `resolveChatModel()`. - Reuse provider options from the current chat's `ChatModelCallConfig`. - Set `MaxUsesPerRun` and `MaxOutputTokens` from advisor config defaults. 3. **Register the tool in the built-in tool block.** - Insert after the skill tools and before MCP tools in `processChat()`. - Record `builtinToolNames["advisor"] = true` so metrics stay bounded. 4. **Inject advisor guidance into the outer system prompt using the same boolean.** - Use `chatprompt.InsertSystem()` in the same prompt assembly path that already injects user/system instructions. - Place the block near the existing instruction insertion, before plan-path/skill context blocks. - Wrap the guidance in an explicit tag like `<advisor-guidance>` so it is easy to spot in tests and future refactors. 5. **Keep advisor out of child chats for the first release.** - That avoids recursion/cost blowups with `spawn_agent` / `wait_agent` flows. - Document this explicitly in the rollout notes and tests. ### Acceptance criteria - If advisor is disabled, neither the tool nor the prompt guidance appears. - If advisor is enabled, both the tool and the prompt guidance appear. - Root chats can use advisor; child chats cannot. - Built-in tool names include `advisor` so metrics do not collapse it into the generic `mcp` label. ## Phase 3 — Enforce planning-only execution policy in `chatloop` ### Goals Prevent the model from calling `advisor` and action tools in the same execution batch. ### Files to modify - `coderd/x/chatd/chatloop/chatloop.go` - related chatloop tests ### Recommended implementation Keep the MVP small; do **not** build a general policy engine yet. 1. Add a minimal field to `chatloop.RunOptions`, for example: ```go ExclusiveToolName *string ``` 2. In `Run()` / `executeTools()`, detect the case where the exclusive tool appears in the same local-tool batch as any other locally executed tool. 3. When that happens, synthesize structured tool-result errors for the affected calls instead of executing anything in the batch. - `advisor` should receive a clear error like: _advisor must be called by itself before action tools_. - The sibling action tools should receive a paired policy error like: _this tool was skipped because advisor must run alone_. 4. Let the outer model see those tool errors and retry cleanly. - This is simpler and safer than partial execution or hidden deferral. - It preserves deterministic transcript history for debugging. 5. Pass the just-finished step snapshot into the tool execution context. - The advisor runtime should be able to see the current step's text/reasoning content, because that is often the best hint about what the outer model is trying to decide. ### Why this is the right fit - It matches the intended semantics: advisor is consulted **before** taking action. - It avoids subtle race conditions caused by concurrent built-in tool execution. - It keeps the behavior easy to test with fake models. ### Acceptance criteria - A model-emitted batch containing only `advisor` succeeds. - A model-emitted batch containing `advisor` plus any other locally executed tool returns deterministic policy errors and executes nothing. - Non-advisor tool execution stays unchanged for normal chats. ## Phase 4 — Usage limits, metrics, and configuration ### Goals Make advisor safe to operate without over-designing billing/storage in the first release. ### Files to modify - `coderd/x/chatd/chatd.go` - `coderd/x/chatd/chatloop/metrics.go` as needed - `coderd/x/chatd/chatd.go` `Config` struct and constructor path - optional follow-up config/db files only if a separate advisor model or persistent billing is required ### Tasks 1. **Add explicit server config knobs for MVP.** Recommended fields on `chatd.Config` or a nested advisor config struct: - `AdvisorEnabled bool` - `AdvisorMaxUsesPerRun int` - `AdvisorMaxOutputTokens int64` 2. **Track usage per outer run.** - Reset the counter for each `processChat()` invocation. - Return `remaining_uses` in the tool result. - Return `limit_reached` when the cap is exhausted. 3. **Expose advisor usage metadata in the tool result.** - Include model name and token/cost summary if available. - Use the same `callConfig.Cost` calculation path as the outer chat for MVP if advisor reuses the same model. 4. **Record server-side metrics.** - Count advisor invocations, failures, and latency. - Ensure they show up under the built-in tool label `advisor`. 5. **Optional decision gate: separate advisor model.** - If product insists on a stronger/different advisor model, add a follow-up config hook that resolves another existing chat model config through the same `configCache` path. - Keep that out of the first landing PR unless it is required for acceptance. 6. **Optional decision gate: queryable advisor cost.** - If this becomes required, spin a follow-up DB task: - update `coderd/database/queries/*.sql`; - add migration files; - run `make gen`; - update audit mappings if a new auditable type/field is introduced. ### Acceptance criteria - Advisor calls are capped per outer run. - Limit exhaustion is user-visible in the tool result. - Metrics distinguish advisor calls from other built-in tools. - MVP does not require a schema migration unless explicitly approved. ## Phase 5 — Frontend rendering and Storybook coverage ### Goals Make advisor feel intentional in the Agents UI without blocking the backend on fancy streaming UI. ### Files to modify - `site/src/pages/AgentsPage/components/ChatElements/tools/Tool.tsx` - new `site/src/pages/AgentsPage/components/ChatElements/tools/AdvisorTool.tsx` - Storybook story file(s) in the same tools directory ### Delivery strategy 1. **Intermediate milestone during backend bring-up:** rely on the existing generic tool renderer if needed. - This is acceptable only as a short-lived integration checkpoint. 2. **Release milestone:** add a dedicated lightweight `AdvisorTool` renderer. - Reuse existing primitives: - `ToolCollapsible` - `ToolIcon` - `Response` for markdown/prose rendering - `ScrollArea` if the advice can be long - Keep styling light and consistent with the Agents page. - Do not add unnecessary React memoization in `site/src/pages/AgentsPage/`; that area is already React-Compiler aware. 3. **Render the structured result states cleanly.** - `advice` — readable prose/markdown with optional metadata footer. - `limit_reached` — warning-style message. - `error` — error state with visible fallback text. - `running` — existing tool loading state/spinner is enough for MVP. 4. **Add Storybook coverage instead of ad-hoc component tests.** Recommended stories: - successful advice; - running/loading; - limit reached; - error. 5. **Keep the UI contract narrow.** - Prefer one text field like `advice` plus small metadata rather than a deeply nested schema. - That keeps the UI resilient to prompt iteration. ### Acceptance criteria - The advisor tool card renders readable content rather than raw quoted JSON in the final release branch. - Running, limit, and error states are visibly distinct. - Storybook stories and play assertions cover the new states. - Existing tool rendering flows remain unchanged. ## Phase 6 — Automated tests and validation gates ### Backend tests to add 1. **Advisor runtime/tool tests** - question validation; - tool-less nested execution assertion; - success result shaping; - limit-reached result shaping; - error result shaping. 2. **Prompt/gating tests in chatd** - advisor disabled ⇒ no tool, no guidance; - advisor enabled/root chat ⇒ tool + guidance; - child chat ⇒ advisor absent. 3. **Chatloop policy tests** - advisor alone runs; - advisor + action tool mixed batch returns deterministic policy errors; - non-advisor tools still execute normally. 4. **Usage/metrics tests** - per-run cap resets correctly; - builtin tool labeling includes `advisor`; - returned metadata includes model/usage summary when available. ### Frontend tests to add - Storybook `play()` assertions for the advisor renderer states. - Verify expand/collapse behavior and visible fallback text. - Verify the message timeline still renders adjacent tools correctly. ### Recommended command sequence Run these as the implementation matures, not only at the end: 1. Backend-focused gate after phases 1–4: - `make test RUN=TestAdvisor` - `make test RUN=TestChatloopAdvisor` - `make lint` 2. Frontend-focused gate after phase 5: - `pnpm test:storybook src/pages/AgentsPage/components/ChatElements/tools/AdvisorTool.stories.tsx` - `pnpm lint` - `pnpm format` 3. Final repo gate before handoff: - `make pre-commit` - run any additional targeted `make test RUN=...` selections covering touched chatd paths > Use the exact new test names the implementing agents create; the names above are recommended anchors, not existing tests. ## Dogfooding plan ### Principle Dogfood the change as a real agent feature, not just a unit-tested backend. Per the dogfood and `agent-browser` skills, the reviewer should get **watchable repro videos** plus screenshots that make the behavior obvious without reading logs. ### Required setup 1. Start the full dev environment with: - `./scripts/develop.sh` 2. If the frontend renderer changes, also start Storybook from `site/` with: - `pnpm storybook --no-open` 3. Use `agent-browser` directly — **never `npx agent-browser`**. 4. Use named browser sessions and an output folder such as: - `./dogfood-output/advisor/` - with subfolders `screenshots/` and `videos/` ### Evidence protocol For every interactive scenario below: 1. Start video recording **before** the action. 2. Capture step-by-step screenshots at human pace. 3. Capture one annotated screenshot of the final state. 4. Stop the recording. 5. Note the exact pass/fail observation in the QA report. For static UI states (for example Storybook error/limit cards), an annotated screenshot is sufficient; video is optional but still encouraged by this project’s review preference. ### Dogfood scenarios #### Scenario A — Happy path in the real Agents UI **Goal:** prove that a root agent chat can invoke advisor and produce a readable recommendation before taking further action. Steps: 1. Open the Agents page with an advisor-enabled root chat. 2. Start a repro video. 3. Send a prompt that should reasonably trigger strategic planning, such as an architecture or multi-tradeoff question. 4. Capture screenshots of: - the prompt before send; - the running advisor state; - the completed advisor card and the assistant’s follow-up response. 5. Stop recording. Pass criteria: - advisor appears in the timeline; - the rendered result is readable; - the assistant can continue after consuming the advisor output. #### Scenario B — Advisor unavailable path **Goal:** prove the feature is truly gated. Suggested variants (at least one is required, both are better): - feature flag/config off; - child/sub-agent chat. Evidence: - annotated screenshot of the chat/tool state showing advisor is absent; - short video if toggling the gate live is part of the repro. Pass criteria: - no advisor tool is available; - no advisor-specific prompt behavior leaks through. #### Scenario C — UI states in Storybook **Goal:** prove the renderer handles non-happy states cleanly. Required story states: - success/advice; - running; - limit reached; - error. Evidence: - one screenshot per state; - at least one short video showing collapse/expand behavior. Pass criteria: - success renders readable advice; - limit/error have visible fallback text; - the component behaves like the other tool cards. #### Scenario D — Regression sweep of nearby tools **Goal:** ensure advisor does not break the surrounding chat timeline. Check at minimum: - another existing built-in tool still renders correctly near advisor; - sub-agent/tool cards still expand/collapse normally; - no obvious console errors appear in the Agents page during the advisor flow. Evidence: - screenshots of adjacent tool cards; - console/error capture if anything suspicious appears. ### `agent-browser` usage notes for the QA agent - Prefer `agent-browser batch` for 2+ sequential commands when no intermediate parsing is needed. - Use `snapshot -i` to discover interactive refs. - Re-snapshot after navigation or major DOM changes. - Avoid `wait --load networkidle` unless the page is known to go idle; prefer explicit element/text waits or short fixed waits. - Record videos at human pace and include pauses that a reviewer can follow. ## Rollout plan ### Initial rollout - Gate behind a server-side advisor-enabled flag. - Enable only for selected internal/root agent chats first. - Watch metrics for: - invocation count; - failure rate; - latency; - obvious retry loops. ### Expansion conditions Expand beyond the initial rollout only after the following are true: - mixed-batch policy behavior is stable; - cost impact is understood; - frontend UX is readable in production-like dogfood; - no recursion surprises have appeared with sub-agent flows. ### Explicit non-goals for the first release - advisor inside child/sub-agent chats; - provider-agnostic streaming phase UI; - MCP-based external advisor implementation; - mandatory DB-backed advisor cost reporting. ## Final acceptance checklist - [ ] `advisor` is a built-in chatd tool, not an MCP/dynamic-tool substitute. - [ ] The nested advisor call is tool-less and bounded to one in-memory step. - [ ] One eligibility boolean controls both tool registration and prompt guidance injection. - [ ] Root chats can use advisor; child chats cannot in the initial rollout. - [ ] Mixed advisor/action batches produce deterministic policy errors instead of partial execution. - [ ] Per-run usage caps and limit-reached behavior work. - [ ] Advisor usage is visible in metadata/metrics without forcing a DB migration for MVP. - [ ] The Agents UI has a readable advisor card and Storybook coverage. - [ ] Dogfooding produced screenshots and repro videos for the required scenarios. - [ ] Validation commands (`make lint`, targeted `make test`, Storybook tests, `make pre-commit`) passed before handoff. ## Suggested PR split 1. **PR 1 — Backend foundation** - `chatadvisor/` package - `chattool/advisor.go` - `chatloop` exclusive policy - chatd gating/prompt sync - backend tests 2. **PR 2 — Frontend + QA** - advisor renderer - stories/play assertions - dogfood artifacts and QA notes 3. **PR 3 — Optional follow-ups only if demanded by stakeholders** - separate advisor model override - persistent advisor billing/queryability - transient phase-stream UX </details> --- _Generated with [`mux`](https://github.com/coder/mux) • Model: `anthropic:claude-opus-4-7` • Thinking: `max`_ |
||
|
Thomas Kosiewski
|
a84534315c |
feat(coderd/x/chatd/chatloop): add exclusive tool execution policy (#24619)
## Summary
Introduce an **exclusive-tool execution policy** in `chatloop.Run()` so that a designated "planning-only" tool can never execute in the same batch as any other locally executed tool. This is the generic foundation for the advisor tool landed in the stacked PRs above.
## Motivation
The advisor tool (stacked on top) is intentionally a pre-action planning step. It must be consulted *alone*, not interleaved with side-effecting tools, so the model has to reason strategically before committing to actions. Rather than building advisor-specific plumbing into `chatloop`, this PR adds a small, general-purpose policy hook that any future tool can opt into.
## Changes
- `RunOptions.ExclusiveToolNames map[string]bool` declares which tool names must run exclusively within a single tool-execution batch.
- `executeTools()` detects mixed batches: if any exclusive tool name appears next to any other locally executed tool, no tool runs. Instead, structured `ToolResultOutputContentError` entries are synthesized for every tool in the batch so the model can cleanly retry.
- Deterministic, model-facing error copy:
- Exclusive tool gets: "must be called by itself before action tools".
- Sibling tools get: "skipped because `<exclusive>` must run alone".
- New tests in `chatloop_test.go` cover: single exclusive batch runs normally, mixed batch returns policy errors and executes nothing, non-exclusive batches are untouched.
## Stack context
This is **PR 1 of 6** in the advisor feature stack.
```
main
└─ feat/advisor-01-chatloop-exclusive-policy ← this PR
└─ feat/advisor-02-chatadvisor-pkg
└─ feat/advisor-03-config-api
├─ feat/advisor-04-chatd-runtime
│ └─ feat/advisor-05-chat-tool-renderer
└─ feat/advisor-06-admin-settings-ui
```
## Scope / non-goals
- No advisor-specific code lives in this PR.
- Zero behavior change when `ExclusiveToolNames` is empty (current default for every existing caller).
## Validation
- `go test ./coderd/x/chatd/chatloop/... -run TestExclusive`
- `make lint`
---
<details>
<summary>📋 Implementation Plan (shared across the advisor stack)</summary>
# Plan: Add a Mux-style advisor tool to coder agents/chatd
## Outcome
Add a first-class `advisor` tool to agent chats in `coderd/x/chatd` that feels native to Coder:
- it is a built-in server-side tool, not an MCP/dynamic-tool workaround;
- it performs a nested **tool-less** model call for strategic advice;
- it is exposed only when eligible, and the prompt mentions it only when it is actually available;
- it is treated as a **planning-only** tool so it does not run alongside action tools in the same batch;
- it tracks usage/cost separately enough for operators to reason about it;
- it has a minimally polished UI in the Agents page;
- and it ships with explicit dogfooding evidence, including screenshots and repro videos.
## Design decisions to lock before coding
1. **Primary architecture:** native built-in tool in `chattool/`, backed by a small `chatadvisor` package.
2. **Nested model execution:** reuse chatd's existing model/provider stack for a one-step, tool-less advisor call rather than inventing a new provider pathway.
3. **Execution policy:** treat `advisor` as an exclusive/planning-only tool; mixed batches must return structured policy errors and force the model to retry cleanly.
4. **Availability:** initial rollout is for root agent chats only; disable for child/sub-agent chats until recursion/cost policy is proven.
5. **Prompt sync:** use one eligibility boolean to drive both tool registration and advisor guidance injection.
6. **Persistence/cost split:** MVP should keep advisor usage visible in result metadata and server metrics; only add DB schema if product/billing explicitly needs queryable advisor-specific cost.
7. **UI scope:** generic tool rendering is an acceptable temporary milestone during backend bring-up, but the release candidate should include a dedicated lightweight advisor renderer.
## Delivery model
The work should be executed as coordinated workstreams with one integration owner and parallel contributors for low-conflict areas. The integration owner should own `coderd/x/chatd/chatd.go` because prompt assembly, tool registration, and model resolution all converge there.
## Detailed workstreams
### Repo evidence used for this plan
<details>
<summary>Mux reference and current chatd seams</summary>
**Mux reference implementation**
- `src/node/services/tools/advisor.ts` — native advisor tool implementation.
- `src/common/constants/advisor.ts` — advisor prompt/constants and truncation policy.
- `src/common/utils/tools/tools.ts` — conditional tool registration.
- `src/node/services/streamContextBuilder.ts` — injects advisor guidance only when the tool is available.
**Current chatd seams**
- `coderd/x/chatd/chatd.go`
- `processChat()` — tool assembly, prompt assembly, and chatloop invocation.
- `resolveChatModel()` — current model/provider/key resolution seam.
- `type Config struct` — server-level chatd configuration surface.
- `coderd/x/chatd/chatloop/chatloop.go`
- `Run()` — main streaming/model loop.
- `executeTools()` — built-in tool execution/batching seam.
- `coderd/x/chatd/chattool/` — built-in tool implementations.
- `site/src/pages/AgentsPage/components/ChatElements/tools/Tool.tsx` — tool renderer dispatch.
- `site/src/pages/AgentsPage/components/ChatConversation/messageParsing.ts` and `ConversationTimeline.tsx` — tool/result merge and rendering flow.
</details>
### Workstream map and ownership
| Workstream | Primary owner | Main files | Can run in parallel? | Done when |
|---|---|---|---|---|
| 0. Integration + gating | Integration lead | `coderd/x/chatd/chatd.go` | No; central merge lane | Tool registration, prompt sync, and model selection are wired together |
| 1. Advisor runtime + tool | Backend agent | new `coderd/x/chatd/chatadvisor/`, new `coderd/x/chatd/chattool/advisor.go` | Yes | Tool can perform a tool-less advisor call in memory and return structured results |
| 2. Planning-only execution policy | Chatloop agent | `coderd/x/chatd/chatloop/chatloop.go`, related tests | Yes | Mixed `advisor` + action-tool batches are rejected cleanly and deterministically |
| 3. Metrics/usage/config | Backend/telemetry agent | `chatd.go`, `chatloop/metrics.go`, optional config plumbing | Partially; coordinate with integration lead | Advisor usage is separately visible in metadata/metrics and limits are enforced |
| 4. Frontend rendering | Frontend agent | `site/.../tools/Tool.tsx`, new `AdvisorTool.tsx`, stories | Yes after result schema stabilizes | Advisor renders as a readable card and story tests pass |
| 5. Dogfood + QA evidence | QA agent | dev server, Storybook, dogfood output | After backend + UI are usable | Repro videos, screenshots, and a concise QA report exist |
### Parallelization rules
- **Do not split `coderd/x/chatd/chatd.go` across multiple execution agents without an integration lead.** That file owns prompt building, tool registration, model resolution, and cost persistence.
- Workstreams 1 and 2 can be developed in parallel and then stacked onto the integration branch.
- Workstream 4 should begin once the backend result schema is agreed on, even if the backend is still behind a feature flag.
- Any agent that needs to re-check Mux behavior should clone `coder/mux` into a temporary directory (for example, `$(mktemp -d)/mux`) and inspect it read-only; do not vendor or copy code from Mux directly.
## Phase 0 — Preflight and guardrails
### Goals
- Align the team on the smallest shippable architecture.
- Prevent scope creep into MCP/dynamic-tool/sub-agent variants.
- Decide upfront what is MVP vs. follow-up.
### Tasks
1. **Confirm the MVP boundary.**
- Ship a built-in advisor tool first.
- Do **not** make MCP, dynamic tools, or sub-agents the primary implementation.
- Do **not** add transient streaming phases in the first backend PR unless they fall out almost for free.
2. **Confirm local workflow hygiene before coding.**
- Ensure the repo is using the project git hooks from `scripts/githooks`.
- Do not bypass hooks with `--no-verify`.
- Use `./scripts/develop.sh` for the full dev server rather than manual build/run commands.
3. **Lock the model-selection policy.**
- **Recommended MVP:** advisor uses the same resolved provider/model/cost config as the current chat, with advisor-specific max-output and usage caps.
- **Follow-up only if required:** add a separate `AdvisorModelConfigID`-style override that resolves through the existing `configCache`/model-config path. Do not invent a new free-form `provider:model` parser if chatd already stores provider/model separately.
4. **Lock the persistence policy.**
- **Recommended MVP:** no DB migration. Persist advisor-visible metadata in the tool result and record separate metrics in memory/Prometheus.
- **Only if product/billing explicitly asks for queryable advisor cost:** add a later DB migration or usage table, following the normal `queries/*.sql` + `make gen` workflow.
5. **Create an execution ADR note in the work item or tracking doc.**
- Capture: built-in tool, tool-less nested call, root-chat-only rollout, exclusive execution policy, MVP no-DB-migration default.
### Quality gate
- Everyone on the team can state the same answers to these questions:
- Is advisor a built-in tool? **Yes.**
- Can advisor run with action tools in the same batch? **No.**
- Does advisor get tools of its own? **No.**
- Is a DB migration required for MVP? **No, unless billing insists.**
## Phase 1 — Build the advisor runtime and tool wrapper
### Goals
Create the core advisor implementation in a way that is easy to test and keeps `chattool/` thin.
### Files to add
- `coderd/x/chatd/chatadvisor/types.go`
- `coderd/x/chatd/chatadvisor/guidance.go`
- `coderd/x/chatd/chatadvisor/handoff.go`
- `coderd/x/chatd/chatadvisor/runtime.go`
- `coderd/x/chatd/chatadvisor/runner.go`
- `coderd/x/chatd/chattool/advisor.go`
### Responsibilities by file
1. **`types.go`**
- Define the input/result schema used by the tool and UI.
- Keep the result shape close to Mux so the UI and model both have predictable cases.
- Recommended result variants:
- `advice`
- `limit_reached`
- `error`
Recommended shape:
```go
type AdvisorArgs struct {
Question string `json:"question"`
}
type AdvisorResult struct {
Type string `json:"type"`
Advice string `json:"advice,omitempty"`
Error string `json:"error,omitempty"`
AdvisorModel string `json:"advisor_model,omitempty"`
RemainingUses int `json:"remaining_uses,omitempty"`
Usage *AdvisorUsageResult `json:"usage,omitempty"`
}
```
2. **`guidance.go`**
- Hold two strings:
- the nested advisor system prompt;
- the parent-agent guidance block to inject into the outer system prompt.
- The nested advisor prompt must say, in plain language:
- you are advising the parent agent;
- you do not address the end user directly;
- you do not claim actions happened;
- you return concise strategic guidance and tradeoffs.
3. **`runtime.go`**
- Define the per-run runtime state.
- Recommended fields:
- resolved model + model config;
- provider keys/options reused from the outer chat;
- `MaxUsesPerRun`;
- `MaxOutputTokens`;
- atomic/current call counter;
- callback(s) to obtain the current prompt snapshot and current-step snapshot;
- optional metrics/usage hook.
- Add fail-fast validation for impossible config: nil model, non-positive limits, empty prompt builders, etc.
4. **`handoff.go`**
- Build the advisor handoff message from:
- the explicit question;
- the exact prompt/messages the parent model just used;
- the current step's text/reasoning snapshot, if available;
- the most recent relevant tool outputs, if they are already in the prompt snapshot.
- **Important:** use the already-prepared outer prompt tail, not a fresh DB reload. That keeps the advisor aligned with compaction and the exact context the outer model saw.
- Apply hard truncation budgets with recent-context bias.
5. **`runner.go`**
- Execute the nested advisor call.
- **Recommended implementation:** call `chatloop.Run()` in an in-memory, one-step mode:
- `Tools: nil`
- `ProviderTools: nil`
- `MaxSteps: 1`
- `PersistStep`: capture the assistant output in memory instead of writing DB rows
- Reuse the existing provider/model/cost path instead of building a second provider runner.
- Assert that no tool definitions are passed to the nested call.
6. **`chattool/advisor.go`**
- Keep this file thin and consistent with other built-ins.
- Responsibilities:
- decode `AdvisorArgs`;
- validate `Question` is non-empty and bounded;
- call the `chatadvisor` runner;
- return a structured tool response.
### Defensive programming requirements
- Assert `Question` is non-empty after trimming.
- Assert runtime limits are positive.
- Assert the nested advisor call runs with zero tools/provider tools.
- Assert `AdvisorResult.Type` is one of the known variants before returning.
- Assert remaining uses never goes negative.
### Acceptance criteria
- A unit test can call the advisor tool with a fake model and receive a stable `advice` result.
- The nested advisor call is impossible to run with tools accidentally attached.
- The core logic lives in `chatadvisor/`, not embedded inside `chatd.go`.
## Phase 2 — Wire advisor into chatd and keep prompt/tool availability in sync
### Goals
Register the tool in the right place, expose it only when eligible, and inject system guidance only when the tool is present.
### Files to modify
- `coderd/x/chatd/chatd.go`
- optionally a small helper file if `chatd.go` becomes too crowded
### Tasks
1. **Compute one eligibility boolean in `processChat()`.**
Recommended inputs:
- server-level advisor enabled flag;
- root chat only (`chat.ParentChatID == uuid.Nil` or equivalent existing root/child check);
- a usable resolved model/provider exists;
- optional experiment/workspace/org gate if product wants staged rollout.
2. **Create the runtime once per outer chat run.**
- Use the model/config/keys resolved by `resolveChatModel()`.
- Reuse provider options from the current chat's `ChatModelCallConfig`.
- Set `MaxUsesPerRun` and `MaxOutputTokens` from advisor config defaults.
3. **Register the tool in the built-in tool block.**
- Insert after the skill tools and before MCP tools in `processChat()`.
- Record `builtinToolNames["advisor"] = true` so metrics stay bounded.
4. **Inject advisor guidance into the outer system prompt using the same boolean.**
- Use `chatprompt.InsertSystem()` in the same prompt assembly path that already injects user/system instructions.
- Place the block near the existing instruction insertion, before plan-path/skill context blocks.
- Wrap the guidance in an explicit tag like `<advisor-guidance>` so it is easy to spot in tests and future refactors.
5. **Keep advisor out of child chats for the first release.**
- That avoids recursion/cost blowups with `spawn_agent` / `wait_agent` flows.
- Document this explicitly in the rollout notes and tests.
### Acceptance criteria
- If advisor is disabled, neither the tool nor the prompt guidance appears.
- If advisor is enabled, both the tool and the prompt guidance appear.
- Root chats can use advisor; child chats cannot.
- Built-in tool names include `advisor` so metrics do not collapse it into the generic `mcp` label.
## Phase 3 — Enforce planning-only execution policy in `chatloop`
### Goals
Prevent the model from calling `advisor` and action tools in the same execution batch.
### Files to modify
- `coderd/x/chatd/chatloop/chatloop.go`
- related chatloop tests
### Recommended implementation
Keep the MVP small; do **not** build a general policy engine yet.
1. Add a minimal field to `chatloop.RunOptions`, for example:
```go
ExclusiveToolName *string
```
2. In `Run()` / `executeTools()`, detect the case where the exclusive tool appears in the same local-tool batch as any other locally executed tool.
3. When that happens, synthesize structured tool-result errors for the affected calls instead of executing anything in the batch.
- `advisor` should receive a clear error like: _advisor must be called by itself before action tools_.
- The sibling action tools should receive a paired policy error like: _this tool was skipped because advisor must run alone_.
4. Let the outer model see those tool errors and retry cleanly.
- This is simpler and safer than partial execution or hidden deferral.
- It preserves deterministic transcript history for debugging.
5. Pass the just-finished step snapshot into the tool execution context.
- The advisor runtime should be able to see the current step's text/reasoning content, because that is often the best hint about what the outer model is trying to decide.
### Why this is the right fit
- It matches the intended semantics: advisor is consulted **before** taking action.
- It avoids subtle race conditions caused by concurrent built-in tool execution.
- It keeps the behavior easy to test with fake models.
### Acceptance criteria
- A model-emitted batch containing only `advisor` succeeds.
- A model-emitted batch containing `advisor` plus any other locally executed tool returns deterministic policy errors and executes nothing.
- Non-advisor tool execution stays unchanged for normal chats.
## Phase 4 — Usage limits, metrics, and configuration
### Goals
Make advisor safe to operate without over-designing billing/storage in the first release.
### Files to modify
- `coderd/x/chatd/chatd.go`
- `coderd/x/chatd/chatloop/metrics.go` as needed
- `coderd/x/chatd/chatd.go` `Config` struct and constructor path
- optional follow-up config/db files only if a separate advisor model or persistent billing is required
### Tasks
1. **Add explicit server config knobs for MVP.**
Recommended fields on `chatd.Config` or a nested advisor config struct:
- `AdvisorEnabled bool`
- `AdvisorMaxUsesPerRun int`
- `AdvisorMaxOutputTokens int64`
2. **Track usage per outer run.**
- Reset the counter for each `processChat()` invocation.
- Return `remaining_uses` in the tool result.
- Return `limit_reached` when the cap is exhausted.
3. **Expose advisor usage metadata in the tool result.**
- Include model name and token/cost summary if available.
- Use the same `callConfig.Cost` calculation path as the outer chat for MVP if advisor reuses the same model.
4. **Record server-side metrics.**
- Count advisor invocations, failures, and latency.
- Ensure they show up under the built-in tool label `advisor`.
5. **Optional decision gate: separate advisor model.**
- If product insists on a stronger/different advisor model, add a follow-up config hook that resolves another existing chat model config through the same `configCache` path.
- Keep that out of the first landing PR unless it is required for acceptance.
6. **Optional decision gate: queryable advisor cost.**
- If this becomes required, spin a follow-up DB task:
- update `coderd/database/queries/*.sql`;
- add migration files;
- run `make gen`;
- update audit mappings if a new auditable type/field is introduced.
### Acceptance criteria
- Advisor calls are capped per outer run.
- Limit exhaustion is user-visible in the tool result.
- Metrics distinguish advisor calls from other built-in tools.
- MVP does not require a schema migration unless explicitly approved.
## Phase 5 — Frontend rendering and Storybook coverage
### Goals
Make advisor feel intentional in the Agents UI without blocking the backend on fancy streaming UI.
### Files to modify
- `site/src/pages/AgentsPage/components/ChatElements/tools/Tool.tsx`
- new `site/src/pages/AgentsPage/components/ChatElements/tools/AdvisorTool.tsx`
- Storybook story file(s) in the same tools directory
### Delivery strategy
1. **Intermediate milestone during backend bring-up:** rely on the existing generic tool renderer if needed.
- This is acceptable only as a short-lived integration checkpoint.
2. **Release milestone:** add a dedicated lightweight `AdvisorTool` renderer.
- Reuse existing primitives:
- `ToolCollapsible`
- `ToolIcon`
- `Response` for markdown/prose rendering
- `ScrollArea` if the advice can be long
- Keep styling light and consistent with the Agents page.
- Do not add unnecessary React memoization in `site/src/pages/AgentsPage/`; that area is already React-Compiler aware.
3. **Render the structured result states cleanly.**
- `advice` — readable prose/markdown with optional metadata footer.
- `limit_reached` — warning-style message.
- `error` — error state with visible fallback text.
- `running` — existing tool loading state/spinner is enough for MVP.
4. **Add Storybook coverage instead of ad-hoc component tests.**
Recommended stories:
- successful advice;
- running/loading;
- limit reached;
- error.
5. **Keep the UI contract narrow.**
- Prefer one text field like `advice` plus small metadata rather than a deeply nested schema.
- That keeps the UI resilient to prompt iteration.
### Acceptance criteria
- The advisor tool card renders readable content rather than raw quoted JSON in the final release branch.
- Running, limit, and error states are visibly distinct.
- Storybook stories and play assertions cover the new states.
- Existing tool rendering flows remain unchanged.
## Phase 6 — Automated tests and validation gates
### Backend tests to add
1. **Advisor runtime/tool tests**
- question validation;
- tool-less nested execution assertion;
- success result shaping;
- limit-reached result shaping;
- error result shaping.
2. **Prompt/gating tests in chatd**
- advisor disabled ⇒ no tool, no guidance;
- advisor enabled/root chat ⇒ tool + guidance;
- child chat ⇒ advisor absent.
3. **Chatloop policy tests**
- advisor alone runs;
- advisor + action tool mixed batch returns deterministic policy errors;
- non-advisor tools still execute normally.
4. **Usage/metrics tests**
- per-run cap resets correctly;
- builtin tool labeling includes `advisor`;
- returned metadata includes model/usage summary when available.
### Frontend tests to add
- Storybook `play()` assertions for the advisor renderer states.
- Verify expand/collapse behavior and visible fallback text.
- Verify the message timeline still renders adjacent tools correctly.
### Recommended command sequence
Run these as the implementation matures, not only at the end:
1. Backend-focused gate after phases 1–4:
- `make test RUN=TestAdvisor`
- `make test RUN=TestChatloopAdvisor`
- `make lint`
2. Frontend-focused gate after phase 5:
- `pnpm test:storybook src/pages/AgentsPage/components/ChatElements/tools/AdvisorTool.stories.tsx`
- `pnpm lint`
- `pnpm format`
3. Final repo gate before handoff:
- `make pre-commit`
- run any additional targeted `make test RUN=...` selections covering touched chatd paths
> Use the exact new test names the implementing agents create; the names above are recommended anchors, not existing tests.
## Dogfooding plan
### Principle
Dogfood the change as a real agent feature, not just a unit-tested backend. Per the dogfood and `agent-browser` skills, the reviewer should get **watchable repro videos** plus screenshots that make the behavior obvious without reading logs.
### Required setup
1. Start the full dev environment with:
- `./scripts/develop.sh`
2. If the frontend renderer changes, also start Storybook from `site/` with:
- `pnpm storybook --no-open`
3. Use `agent-browser` directly — **never `npx agent-browser`**.
4. Use named browser sessions and an output folder such as:
- `./dogfood-output/advisor/`
- with subfolders `screenshots/` and `videos/`
### Evidence protocol
For every interactive scenario below:
1. Start video recording **before** the action.
2. Capture step-by-step screenshots at human pace.
3. Capture one annotated screenshot of the final state.
4. Stop the recording.
5. Note the exact pass/fail observation in the QA report.
For static UI states (for example Storybook error/limit cards), an annotated screenshot is sufficient; video is optional but still encouraged by this project’s review preference.
### Dogfood scenarios
#### Scenario A — Happy path in the real Agents UI
**Goal:** prove that a root agent chat can invoke advisor and produce a readable recommendation before taking further action.
Steps:
1. Open the Agents page with an advisor-enabled root chat.
2. Start a repro video.
3. Send a prompt that should reasonably trigger strategic planning, such as an architecture or multi-tradeoff question.
4. Capture screenshots of:
- the prompt before send;
- the running advisor state;
- the completed advisor card and the assistant’s follow-up response.
5. Stop recording.
Pass criteria:
- advisor appears in the timeline;
- the rendered result is readable;
- the assistant can continue after consuming the advisor output.
#### Scenario B — Advisor unavailable path
**Goal:** prove the feature is truly gated.
Suggested variants (at least one is required, both are better):
- feature flag/config off;
- child/sub-agent chat.
Evidence:
- annotated screenshot of the chat/tool state showing advisor is absent;
- short video if toggling the gate live is part of the repro.
Pass criteria:
- no advisor tool is available;
- no advisor-specific prompt behavior leaks through.
#### Scenario C — UI states in Storybook
**Goal:** prove the renderer handles non-happy states cleanly.
Required story states:
- success/advice;
- running;
- limit reached;
- error.
Evidence:
- one screenshot per state;
- at least one short video showing collapse/expand behavior.
Pass criteria:
- success renders readable advice;
- limit/error have visible fallback text;
- the component behaves like the other tool cards.
#### Scenario D — Regression sweep of nearby tools
**Goal:** ensure advisor does not break the surrounding chat timeline.
Check at minimum:
- another existing built-in tool still renders correctly near advisor;
- sub-agent/tool cards still expand/collapse normally;
- no obvious console errors appear in the Agents page during the advisor flow.
Evidence:
- screenshots of adjacent tool cards;
- console/error capture if anything suspicious appears.
### `agent-browser` usage notes for the QA agent
- Prefer `agent-browser batch` for 2+ sequential commands when no intermediate parsing is needed.
- Use `snapshot -i` to discover interactive refs.
- Re-snapshot after navigation or major DOM changes.
- Avoid `wait --load networkidle` unless the page is known to go idle; prefer explicit element/text waits or short fixed waits.
- Record videos at human pace and include pauses that a reviewer can follow.
## Rollout plan
### Initial rollout
- Gate behind a server-side advisor-enabled flag.
- Enable only for selected internal/root agent chats first.
- Watch metrics for:
- invocation count;
- failure rate;
- latency;
- obvious retry loops.
### Expansion conditions
Expand beyond the initial rollout only after the following are true:
- mixed-batch policy behavior is stable;
- cost impact is understood;
- frontend UX is readable in production-like dogfood;
- no recursion surprises have appeared with sub-agent flows.
### Explicit non-goals for the first release
- advisor inside child/sub-agent chats;
- provider-agnostic streaming phase UI;
- MCP-based external advisor implementation;
- mandatory DB-backed advisor cost reporting.
## Final acceptance checklist
- [ ] `advisor` is a built-in chatd tool, not an MCP/dynamic-tool substitute.
- [ ] The nested advisor call is tool-less and bounded to one in-memory step.
- [ ] One eligibility boolean controls both tool registration and prompt guidance injection.
- [ ] Root chats can use advisor; child chats cannot in the initial rollout.
- [ ] Mixed advisor/action batches produce deterministic policy errors instead of partial execution.
- [ ] Per-run usage caps and limit-reached behavior work.
- [ ] Advisor usage is visible in metadata/metrics without forcing a DB migration for MVP.
- [ ] The Agents UI has a readable advisor card and Storybook coverage.
- [ ] Dogfooding produced screenshots and repro videos for the required scenarios.
- [ ] Validation commands (`make lint`, targeted `make test`, Storybook tests, `make pre-commit`) passed before handoff.
## Suggested PR split
1. **PR 1 — Backend foundation**
- `chatadvisor/` package
- `chattool/advisor.go`
- `chatloop` exclusive policy
- chatd gating/prompt sync
- backend tests
2. **PR 2 — Frontend + QA**
- advisor renderer
- stories/play assertions
- dogfood artifacts and QA notes
3. **PR 3 — Optional follow-ups only if demanded by stakeholders**
- separate advisor model override
- persistent advisor billing/queryability
- transient phase-stream UX
</details>
---
_Generated with [`mux`](https://github.com/coder/mux) • Model: `anthropic:claude-opus-4-7` • Thinking: `max`_
|
||
|
Thomas Kosiewski
|
ab75e46f1d | fix: record debug runs for proposed chat titles (#24820) | ||
|
Thomas Kosiewski
|
88b62a3359 | fix(site/src/pages/AgentsPage): add copy buttons to raw attempts (#24705) | ||
|
Thomas Kosiewski
|
5c316d4252 |
test(site/src/pages/AgentsPage): add Debug panel Storybook stories (#23921)
## Summary Add comprehensive Storybook stories covering the Debug panel in various states: loading, empty, single/multiple runs, expanded steps, tool calls, error states, and streaming indicators. This is PR 9/9 in the chat debug logging stack. ### Changes - **DebugPanel stories** (`site/src/pages/AgentsPage/components/RightPanel/DebugPanel/DebugPanel.stories.tsx`): Storybook stories with `play` functions for interaction testing, covering: - Loading state - Empty / disabled state - Single-step successful run - Multi-step run with tool calls - Error state with redacted headers - Compaction and title generation run kind badges - Long raw request/response payload rendering - Streaming / in-progress indicators ### Stack overview 1. Database schema & SDK types 2. Types, context, and model normalization 3. Recorder, transport, and redaction 4. Service and summary aggregation 5. Chat lifecycle wiring 6. HTTP handlers and API docs 7. Frontend API layer and panel utilities 8. Debug panel components and settings 9. **→ Storybook stories** (this PR) --- _Generated with [`mux`](https://github.com/coder/mux) • Model: `anthropic:claude-opus-4-6` • Thinking: `xhigh`_ |
||
|
Thomas Kosiewski
|
249b71b96a |
feat(site): add Debug panel components and settings (#23920)
## Summary Add the Debug panel UI components: run list, run cards, step cards with transcript and tool-call rendering, attempt accordions, and shared primitives. Wire the panel into the AgentChatPage sidebar and add the per-chat debug logging toggle in the behavior settings page. This is PR 8/9 in the chat debug logging stack. ### Screenshots Settings Page <img width="4608" height="2348" alt="CleanShot 2026-04-21 at 21 19 36@2x" src="https://github.com/user-attachments/assets/69391465-4c56-468a-9923-59576d326963" /> Conditional Debug tab <img width="4608" height="2348" alt="CleanShot 2026-04-21 at 21 19 58@2x" src="https://github.com/user-attachments/assets/bc1e07cb-21d9-40e7-8928-6fd9a7ec7f57" /> Last request's tools and schema <img width="4608" height="2348" alt="CleanShot 2026-04-21 at 21 20 03@2x" src="https://github.com/user-attachments/assets/401f26af-98ce-443f-a586-424d3636d98b" /> "Raw" JSON request bodies <img width="4608" height="2348" alt="CleanShot 2026-04-21 at 21 20 35@2x" src="https://github.com/user-attachments/assets/3605a373-9e29-4183-89e7-8b2704ff9333" /> ### Changes - **DebugPanel** (`site/src/pages/AgentsPage/components/RightPanel/DebugPanel/DebugPanel.tsx`): top-level panel component owning data fetching and subscription lifecycle. - **DebugRunCard**: compact single-row header with capitalized provider name, status badge, compact duration (`1.3s`), and token summary (`3→5 tok`). Expandable to show child step cards. - **DebugStepCard**: step inspector with normalized transcript rendering — system prompts, assistant text with 160-char clamping + independent "see more/less" toggle, tool calls with fully-expanded JSON payloads in `CopyableCodeBlock`. - **DebugAttemptAccordion**: nested accordion for HTTP-level attempt details showing request/response headers and bodies. - **Shared primitives**: `CopyableCodeBlock`, `MessageRow`, `ToolPayloadDisclosure`, `StatusBadge`. - **Sidebar wiring** (`AgentChatPageView.tsx`): adds the Debug tab to the right panel when debug logging is enabled. - **Behavior settings**: deployment-wide and per-user debug logging toggles on the settings page. ### Stack overview 1. Database schema & SDK types 2. Types, context, and model normalization 3. Recorder, transport, and redaction 4. Service and summary aggregation 5. Chat lifecycle wiring 6. HTTP handlers and API docs 7. Frontend API layer and panel utilities 8. **→ Debug panel components and settings** (this PR) 9. Storybook stories --- _Generated with [`mux`](https://github.com/coder/mux) • Model: `anthropic:claude-opus-4-6` • Thinking: `xhigh`_ |
||
|
Thomas Kosiewski
|
8c0fe6d5f2 |
feat(site): add chat debug API layer and panel utilities (#23919)
## Summary Add API client methods, React Query builders, and unit tests for the chat debug endpoints. Add `debugPanelUtils` with coercion helpers that transform raw debug step data into structured display models for the Debug panel, and wire debug run streaming into the chat store. This is PR 7/9 in the chat debug logging stack. ### Changes - **API client** (`site/src/api/api.ts`): typed methods for all debug endpoints — list runs, list steps, get/set deployment logging, get/set user logging, set per-chat override. - **React Query builders** (`site/src/api/queries/chats.ts`): `chatDebugRuns`, `chatDebugSteps`, `chatDebugLoggingConfig`, `userDebugLoggingConfig` query/mutation factories with `refetchInterval: 5000` for live polling. - **Debug panel utilities** (`site/src/pages/AgentsPage/components/RightPanel/DebugPanel/debugPanelUtils.ts`): `coerceStepRequest` / `coerceStepResponse` that recursively parse nested JSONB into `StepRequestViewModel` / `StepResponseViewModel` with `MessagePart`, `ToolDef`, and `ToolCallPart` types. Includes `formatTokenSummary` (compact `3→5 tok` notation) and `compactDuration` helpers. - **Unit tests**: coverage for coercion edge cases and formatting utilities. ### Stack overview 1. Database schema & SDK types 2. Types, context, and model normalization 3. Recorder, transport, and redaction 4. Service and summary aggregation 5. Chat lifecycle wiring 6. HTTP handlers and API docs 7. **→ Frontend API layer and panel utilities** (this PR) 8. Debug panel components and settings 9. Storybook stories --- _Generated with [`mux`](https://github.com/coder/mux) • Model: `anthropic:claude-opus-4-6` • Thinking: `xhigh`_ |
||
|
Thomas Kosiewski
|
b7c2c59931 |
fix(coderd/x/chatd/chatdebug): allow Anthropic per-modality ratelimit headers (#24592)
Previously, Anthropic's per-modality, Priority Tier, and fast-mode rate-limit headers (`Anthropic-Ratelimit-Input-Tokens-*`, `Anthropic-Ratelimit-Output-Tokens-*`, `Anthropic-Priority-Input-Tokens-*`, `Anthropic-Priority-Output-Tokens-*`, `Anthropic-Fast-Input-Tokens-*`, and `Anthropic-Fast-Output-Tokens-*`) were shown as `[REDACTED]` in the Debug panel because they contain `"token"` in the name and fell through the generic credential filter. Add them to the allowlist in `coderd/x/chatd/chatdebug/redaction.go` alongside the existing `Anthropic-Ratelimit-Tokens-*` entries so the limits/remaining/reset values surface in the raw response view. |
||
|
Thomas Kosiewski
|
26b64fa523 |
fix(coderd/x/chatd/chatdebug): record SSE attempts on EOF (#24565)
`chat_turn` debug steps persist with `attempts: []` even when the streaming call to Anthropic completes successfully. Fantasy's Anthropic SSE adapter iterates the response to EOF via `for stream.Next()` and abandons the body without calling `Close()`, so `RecordingTransport`'s Close-only recording path never fires and the attempt is lost. Non-streaming runs (`quickgen`, `title_generation`) go through `model.Generate(...)` and are unaffected. Record on `io.EOF` for `text/event-stream` bodies specifically. Non-SSE responses stay on the Close-only path so JSON integrity, content-length validation, and inner-`Close()` error semantics are preserved. `record()` is already `sync.Once`-guarded, so a later `Close()` is a no-op for recording. |
||
|
Thomas Kosiewski
|
18a30a7a10 | feat: add chat debug HTTP handlers and API docs (#23918) | ||
|
Thomas Kosiewski
|
df7e838c21 | feat(coderd): wire debug logging into chat lifecycle (#23917) | ||
|
Thomas Kosiewski
|
91f9de27a1 | feat(coderd): add chat debug service and summary aggregation (#23916) | ||
|
Thomas Kosiewski
|
4651ca5a9a | feat(coderd/x/chatd/chatdebug): add recorder, transport, and redaction (#23915) | ||
|
Thomas KosiewskiandCian Johnston
|
5812f84e1c |
fix(coderd): validate webpush subscription endpoints (#24347)
Co-authored-by: Cian Johnston <cian@coder.com> |
||
|
Thomas Kosiewski
|
8382e96a81 | feat: add types, context, and model normalization (#23914) | ||
|
Thomas Kosiewski
|
6ab30123bf | feat: add chat debug log tables, queries, and SDK types (#23913) | ||
|
Thomas Kosiewski
|
12f87acad6 |
feat(site): add terminal panel to chat sidebar (#23231)
## Add terminal panel to chat sidebar Extract the reusable terminal runtime from `TerminalPage` into `modules/terminal/` and wire it into the agents chat right sidebar as a new **Terminal** tab. ### Changes - **`modules/terminal/WorkspaceTerminal.tsx`** — Shared xterm + websocket terminal component (container-sized, no route dependency) - **`modules/terminal/WorkspaceTerminalAlerts.tsx`** — Moved from `TerminalPage/` to shared module - **`pages/AgentsPage/TerminalPanel.tsx`** — Sidebar wrapper around `WorkspaceTerminal` - **`pages/AgentsPage/AgentDetailView.tsx`** — Terminal tab added (gated on `hasWorkspace`) - **`pages/TerminalPage/TerminalPage.tsx`** — Slimmed to page-shell using shared component ### Demo [dogfood-terminal-demo.webm](https://github.com/user-attachments/assets/359200dc-f8e4-4a9a-b00b-923f142dc228) ### Behavior - Terminal tab appears only when the chat has a workspace with a connected agent - Connects via the existing workspace agent PTY websocket - Resizes correctly on panel width changes, expand/collapse, and viewport resize - `fitAddon.fit()` guarded against pre-renderer crashes (fixes proxy access) - Tab switching unmounts/remounts cleanly (reconnects via session token) - No changes to Git or Desktop panel behavior --- _Generated with [`mux`](https://github.com/coder/mux) • Model: `anthropic:claude-opus-4-6` • Thinking: `xhigh`_ |
||
|
Thomas Kosiewski
|
ea37f1ff86 |
feat: pass session token as query param on agent chat WebSockets (#23405)
## Problem When the Coder chat UI is embedded in a VS Code webview, the session token is set via the Coder-Session-Token header for HTTP requests. However, browsers cannot attach custom headers to WebSocket connections, and VS Code Electron webview environment does not support cookies set via Set-Cookie from iframe origins. This causes all chat WebSocket connections to fail with authorization errors. ## Solution Pass the session token as a coder_session_token query parameter on all chat-related WebSocket connections. The backend already accepts this parameter (see APITokenFromRequest in coderd/httpmw/apikey.go). The token is only included when API.getSessionToken() returns a value, which only happens in the embed bootstrap flow. Normal browser sessions use cookies and are unaffected. > Built with [Coder Agents](https://coder.com/agents) |
||
|
Thomas Kosiewski
|
83809bb380 |
feat: add token-to-cookie endpoint for embedded chat WebSocket auth (#23280)
## Problem The VS Code extension embeds the Coder agent chat UI in an iframe, passing the session token via `postMessage`. HTTP requests use the `Coder-Session-Token` header, but browser WebSocket connections **cannot carry custom headers** — they rely on cookies. This causes all WebSocket requests (e.g. streaming chat messages) to fail with authorization errors in the embedded iframe. ## Solution Add `POST /api/v2/users/me/session/token-to-cookie` — a lightweight endpoint that converts the current (already-validated) session token into a `Set-Cookie` response. The frontend embed bootstrap flow calls this immediately after `API.setSessionToken(token)`, before any WebSocket connections are opened. ### Backend (`coderd/userauth.go`, `coderd/coderd.go`) - New handler `postSessionTokenCookie` behind `apiKeyMiddleware`. - Reads the validated token via `httpmw.APITokenFromRequest(r)`. - Sets an `HttpOnly` cookie with the API key's expiry, applying site-wide cookie config (Secure, SameSite, host prefix) via `HTTPCookies.Apply`. - Returns `204 No Content`. ### Frontend (`site/src/pages/AgentsPage/EmbedContext.tsx`) - `bootstrapChatEmbedSessionFn` now calls the new endpoint after setting the header token and before fetching user/permissions. - The cookie is in place before any WebSocket connections are opened. ## Security - **No privilege escalation**: The token is already valid — this just moves it from a header credential to a cookie credential. - **POST only**: Avoids CSRF-via-navigation. - **Same origin**: The iframe loads from the Coder server, so the cookie applies to the correct domain. - **HttpOnly**: The cookie is not accessible to JavaScript. > Built with [Coder Agents](https://coder.com/agents) 🤖 |
||
|
Thomas Kosiewski
|
20ac96e68d |
feat(site): include chatId in editor deep links (#23214)
## Summary - include the current agent chat ID in VS Code and Cursor deep links opened from the agent detail page - extend `getVSCodeHref` so `chatId` is added only when provided - add focused tests for deep-link generation with and without `chatId` ## Testing - `pnpm -C site run format -- src/modules/apps/apps.ts src/modules/apps/apps.test.ts src/pages/AgentsPage/AgentDetail.tsx` - `pnpm -C site run check -- src/modules/apps/apps.ts src/modules/apps/apps.test.ts src/pages/AgentsPage/AgentDetail.tsx` - `pnpm -C site exec vitest run src/modules/apps/apps.test.ts` - `pnpm -C site run lint:types` --- _Generated with [`mux`](https://github.com/coder/mux) • Model: `openai:gpt-5.4` • Thinking: `high`_ |
||
|
Thomas Kosiewski
|
c1884148f0 |
feat: add VS Code iframe embed auth bootstrap (#23060)
## VS Code iframe embed auth via postMessage + setSessionToken Adds embed auth for VS Code iframe integration, allowing the Coder agent chat UI to be embedded in VS Code webviews without manual login — using direct header auth instead of cookies. ### How it works 1. **Parent frame** (VS Code webview) loads an iframe pointing to `/agents/:agentId/embed` 2. **Embed page** detects the user is signed out and posts `coder:vscode-ready` to the parent 3. **Parent** responds with `coder:vscode-auth-bootstrap` containing the user's Coder API token 4. **Embed page** calls `API.setSessionToken(token)` to set the `Coder-Session-Token` header on all subsequent axios requests 5. **Embed page** fetches user + permissions, sets them in the React Query cache atomically, and renders the authenticated agent chat UI No cookies, no CSRF, no backend endpoint needed. The token is passed via postMessage and used as a header on every API request. ### What changed **Frontend** (`site/src/pages/AgentsPage/`): - `AgentEmbedPage.tsx` — added postMessage bootstrap directly in the embed page: listens for `coder:vscode-auth-bootstrap`, calls `API.setSessionToken(token)`, fetches user/permissions atomically to avoid race conditions - `EmbedContext.tsx` — React context signaling embed mode (from previous commit, unchanged) - `AgentDetail/TopBar.tsx` — conditionally hides navigation elements in embed mode (from previous commit, unchanged) - Both `/agents/:agentId/embed` and `/agents/:agentId/embed/session` routes live outside `RequireAuth` **Auth bootstrap** (`site/src/api/queries/users.ts`): - `bootstrapChatEmbedSessionFn` now calls `API.setSessionToken(token)` instead of posting to a backend endpoint - Fetches user and permissions directly via `API.getAuthenticatedUser()` and `API.checkAuthorization()`, then sets both in the query cache atomically — this avoids a race where `isSignedIn` flips before permissions are loaded **Removed** (no longer needed): - `coderd/embedauth.go` — the `POST /api/experimental/chats/embed-session` handler - `coderd/embedauth_test.go` — backend tests for the endpoint - `codersdk/embedauth.go` — `EmbedSessionTokenRequest` SDK type - `site/src/api/api.ts` — `postChatEmbedSession` method - `docs/user-guides/workspace-access/vscode-embed-auth.md` — doc page for the old cookie flow - Swagger/API doc entries for the endpoint ### Why not cookies? The initial implementation used a backend endpoint to set an HttpOnly session cookie. This required `SameSite=None; Secure` for cross-origin iframes, which doesn't work over HTTP in development (Chrome requires HTTPS for `Secure` cookies). The `setSessionToken` approach bypasses cookies entirely — the token is set as an axios default header, and header-based auth also naturally bypasses CSRF protection. ### Dogfooding Tested end-to-end with a VS Code extension that: 1. Registers a `/openChat` deep link handler (`vscode://coder.coder-remote/openChat?url=...&token=...&agentId=...`) 2. Starts a local HTTP reverse proxy (to work around VS Code webview iframe sandboxing) 3. Loads `/agents/:agentId/embed` in an iframe through the proxy 4. Relays the postMessage handshake between the iframe and the extension host 5. The embed page receives the token, calls `setSessionToken`, and renders the chat Verified: chat title, messages, and input field all display correctly in VS Code's secondary sidebar panel. |
||
|
Thomas Kosiewski
|
d6306461bb |
feat(site): render computer tool screenshots as images in chat UI (#23074)
Instead of showing raw base64 JSON for Anthropic's computer use tool, render the screenshot as an inline image. The image is clickable to open at full resolution in a new tab. ## Changes - **ComputerTool.tsx** — New component that renders base64 image data as an `<img>` tag - **Tool.tsx** — Added `ComputerRenderer` handling both single-object and array-of-blocks result shapes - **ToolIcon.tsx** — Added `MonitorIcon` for the `computer` tool - **ToolLabel.tsx** — Added \Screenshot\ label for the `computer` tool |
||
|
Thomas Kosiewski
|
069d3e2beb |
fix(coderd): require ssh access for workspace chats (#23094)
### Motivation - The chat creation flow associated a workspace agent for a chat if the requester could read the workspace, enabling privilege escalation where users without SSH/app-connect permissions could cause the daemon to open privileged agent connections and execute commands. - The intent is to ensure that attaching a workspace agent to a chat only happens when the requester has the workspace SSH permission so the chat daemon cannot be abused to bypass RBAC. ### Description - Require request-scoped authorization for workspace agent usage by changing `validateCreateChatWorkspaceSelection` to accept the `*http.Request` and calling `api.Authorize(r, policy.ActionSSH, workspace)` before selecting the workspace for a chat. - Pass the HTTP request into the validator from `postChats` so authorization is evaluated in the request context (`postChats` now calls `validateCreateChatWorkspaceSelection(ctx, r, req)`). - Add a regression test `WorkspaceAccessibleButNoSSH` in `coderd/chats_test.go` which creates an org-admin-scoped user (read access but no `ActionSSH`) and asserts that creating a chat with `WorkspaceID` is denied. ### Testing - Ran `gofmt -w coderd/chats.go coderd/chats_test.go` which succeeded. - Attempted to run repository pre-commit checks (`make pre-commit`) and targeted `go test` invocations; these checks could not be completed in this environment due to missing local tooling and environment constraints (protobuf include resolution, containerized DB access via Docker socket, and long-running golden generation tasks), so full CI/pre-commit verification and end-to-end test runs did not complete here. - Added a focused regression unit test (`WorkspaceAccessibleButNoSSH`) to prevent reintroduction of the authorization bypass; this test is included in the change and should be executed in CI where the full toolchain and test environment are available. ------ [Codex Task](https://chatgpt.com/codex/tasks/task_b_69b432502670832e91d14e937745de46) |
||
|
Thomas Kosiewski
|
3f7f25b3ee |
fix(chats): enforce desktop connect authorization (#23073)
### Motivation - The desktop watch handler opened a VNC stream using the chat's workspace ID while only relying on workspace read permissions, allowing read-only users to escalate to interactive desktop access. - Enforce connect-level authorization so only actors with `ActionApplicationConnect` or `ActionSSH` can open the desktop stream. ### Description - Added an explicit workspace lookup in `watchChatDesktop` using `GetWorkspaceByID` to obtain a workspace object for authorization. - Require the requester to be authorized for either `policy.ActionApplicationConnect` or `policy.ActionSSH` on the workspace before proceeding to locate agents or connect to the VNC stream, and return `403 Forbidden` when neither permission is present. - The change is minimal and localized to `coderd/chats.go` and does not alter other code paths or behavior when the requester has the necessary connect permissions. ### Testing - Ran `gofmt -w coderd/chats.go` to format the modified file, which succeeded. - Attempted to run the unit test `TestWatchChatDesktop/NoWorkspace` via `go test` in this environment but the test run did not complete within the environment constraints and did not produce a full pass result. - Attempted to run the repository pre-commit/gen steps but they could not complete due to missing developer tooling and services in this environment (e.g. `sqlc`, `mockgen`, `protoc` plugins and test services like Docker/Postgres), so full pre-commit validation did not finish here. - Code review and static validation confirm the added authorization check properly prevents read-only access from opening the desktop VNC stream. ------ [Codex Task](https://chatgpt.com/codex/tasks/task_b_69b46a4ac5c4832ea9d330aeba43c32d) |
||
|
Thomas Kosiewski
|
e96cd5cbb2 |
chore(githooks): remove pre-push hook (#22956)
## Summary - remove the `pre-push` git hook script from the repository - remove the `make pre-push` target and related Makefile documentation - update contributor and agent docs so they only describe the remaining `pre-commit` hook ## Validation - `make pre-commit` - `git diff --check` --- _Generated with [`mux`](https://github.com/coder/mux) • Model: `openai:gpt-5.4` • Thinking: `high`_ |
||
|
Thomas Kosiewski
|
b776a14b46 |
fix(coderd): harden OAuth2 provider security (#22194)
## Summary Harden the OAuth2 provider with multiple security fixes addressing `coder/security#121` (CSRF session takeover) and converge on OAuth 2.1 compliance. ### Security Fixes | Fix | Description | Commits | |-----|-------------|---------| | **CSRF on `/oauth2/authorize`** | Enforce CSRF protection on the authorize endpoint POST (consent form submission) | `ba7d646`, `b94a64e` | | **Clickjacking: `frame-ancestors` CSP** | Prevent consent page from being iframed (`Content-Security-Policy: frame-ancestors 'none'` + `X-Frame-Options: DENY`) | `597aeb2` | | **Exact redirect URI matching** | Changed from prefix matching to full string exact matching per OAuth 2.1 §4.1.2.1 | `73d64b1`, `93897f1` | | **Store & verify `redirect_uri`** | Store redirect_uri with auth code in DB, verify at token exchange matches exactly (RFC 6749 §4.1.3) | `50569b9`, `d7ca315` | | **Mandatory PKCE** | Require `code_challenge` at authorization (for `response_type=code`) + unconditional `code_verifier` verification at token exchange | `d7ca315`, `1cda1a9` | | **Reject implicit grant** | `response_type=token` now returns `unsupported_response_type` error page (OAuth 2.1 removes implicit flow) | `d7ca315`, `91b8863` | ### Changes by File **`coderd/httpmw/csrf.go`** — Extended the CSRF `ExemptFunc` to enforce CSRF on `/oauth2/authorize` in addition to `/api` routes. The consent form POST is now CSRF-protected to prevent cross-site authorization code theft. **`site/site.go`** — Added `Content-Security-Policy: frame-ancestors 'none'` and `X-Frame-Options: DENY` headers to `RenderOAuthAllowPage` (consent page only — does not affect the SPA/global CSP used by AI tasks). **`coderd/httpapi/queryparams.go`** — Changed `RedirectURL` from prefix matching (`strings.HasPrefix(v.Path, base.Path)`) to full URI exact matching (`v.String() != base.String()`), comparing scheme, host, path, and query. **`coderd/oauth2provider/authorize.go`** — Added PKCE enforcement: `code_challenge` is required when `response_type=code` (via a conditional check, not `RequiredNotEmpty`, so `response_type=token` can reach the explicit rejection path). `ShowAuthorizePage` (GET) validates `response_type` before rendering and returns a 400 error page for unsupported types. `ProcessAuthorize` (POST) stores the `redirect_uri` with the auth code when explicitly provided. **`coderd/oauth2provider/tokens.go`** — PKCE verification is now unconditional (not gated on `code_challenge` being present in DB). If the stored code has a `redirect_uri`, the token endpoint verifies it matches exactly — mismatch returns `errBadCode` → `invalid_grant`. Missing `code_verifier` returns `invalid_grant`. **`codersdk/oauth2.go`** — `OAuth2ProviderResponseTypeToken` constant and `Valid()` acceptance are **kept** so the authorize handler can parse `response_type=token` and return the proper `unsupported_response_type` error rather than failing at parameter validation. **`coderd/database/migrations/000421_*`** — Added `redirect_uri text` column to `oauth2_provider_app_codes`. ### Design Decisions **`state` parameter remains optional** — The plan initially required `state` via `RequiredNotEmpty`, but this was reverted in `376a753` to avoid breaking existing clients. The `state` is still hashed and stored when provided (via `state_hash` column), securing clients that opt in. **`response_type=token` kept in `Valid()`** — Removing it from `Valid()` would cause the parameter parser to reject the request before the authorize handler can return the proper `unsupported_response_type` error. The constant is kept for correct error handling flow. **CSP scoped to consent page only** — `frame-ancestors 'none'` is set only on the OAuth consent page renderer, not globally. The SPA/global CSP was previously changed to allow framing for AI tasks ([#18102](https://github.com/coder/coder/pull/18102)); this change does not regress that. ### Out of Scope (follow-up PRs) - Bearer tokens in query strings (needs internal caller audit) - Scope enforcement on OAuth2 tokens - Rate limiting on dynamic client registration --- <details> <summary>📋 Implementation Plan</summary> # Plan: Harden OAuth2 Provider — Security Fixes + OAuth 2.1 Compliance ## Context & Why Security issue `coder/security#121` reports a critical session takeover via CSRF on the OAuth2 provider. This plan covers all remaining security fixes from that issue **plus** convergence on OAuth 2.1 requirements. The goal is a single PR that closes all actionable gaps. ## Current State (already committed on branch `csrf-sjx1`) | Fix | Status | Commits | |-----|--------|---------| | Fix 1: CSRF on `/oauth2/authorize` | ✅ Done | `ba7d646`, `b94a64e` | | CSRF token in consent form HTML | ✅ Done | `b94a64e` | | `state_hash` column + storage | ✅ Done (hash stored, but state still optional) | `9167d83`, `b94a64e` | | Tests for CSRF + state hash | ✅ Done | `e4119b5` | ## Remaining Work ### ~~Fix 2 — Require `state` parameter~~ (DROPPED) > **Decision:** Do not enforce `state` as required. The `state` parameter is still hashed and stored when provided (via `hashOAuth2State` / `state_hash` column from prior commits), but clients are not forced to supply it. This avoids breaking existing integrations that omit state. **Rollback:** Remove `"state"` from the `RequiredNotEmpty` call in `coderd/oauth2provider/authorize.go:42`: ```go // BEFORE (current on branch) p.RequiredNotEmpty("response_type", "client_id", "state", "code_challenge") // AFTER p.RequiredNotEmpty("response_type", "client_id", "code_challenge") ``` No test changes needed — tests already pass `state` voluntarily. ### Fix 4 — Exact redirect URI matching Currently `coderd/httpapi/queryparams.go:233` uses prefix matching: ```go // CURRENT — prefix match if v.Host != base.Host || !strings.HasPrefix(v.Path, base.Path) { ``` OAuth 2.1 requires **exact string matching**. Change to: ```go // AFTER — exact match (OAuth 2.1 §4.1.2.1) if v.Host != base.Host || v.Path != base.Path { ``` **File: `coderd/httpapi/queryparams.go` — `RedirectURL` method** Also update the error message from "must be a subset of" to "must exactly match". **Additionally**, store `redirect_uri` with the auth code and verify at the token endpoint (RFC 6749 §4.1.3): 1. **New migration** (same migration file or a new `000421`): Add `redirect_uri text` column to `oauth2_provider_app_codes` 2. **Update INSERT query** in `coderd/database/queries/oauth2.sql` to include `redirect_uri` 3. **`coderd/oauth2provider/authorize.go`**: Store `params.redirectURL.String()` when inserting the code 4. **`coderd/oauth2provider/tokens.go`**: After retrieving the code from DB, verify that `redirect_uri` from the token request matches the stored value exactly. Currently `tokens.go:103` calls `p.RedirectURL(vals, callbackURL, "redirect_uri")` for prefix validation only — it must compare against the stored redirect_uri from the code, not just the app's callback URL. <details> <summary>Why both exact match AND store+verify?</summary> Exact matching at the authorize endpoint prevents open redirectors (attacker can't use a sub-path). Storing and verifying at the token endpoint prevents code injection — an attacker who steals a code can't exchange it with a different redirect_uri than was originally authorized. This is required by RFC 6749 §4.1.3 and OAuth 2.1. </details> ### Fix 7 — `frame-ancestors` CSP on consent page The consent page can be iframed by a workspace app (same-site), which is the attack vector. Add a `Content-Security-Policy` header to prevent framing. **File: `site/site.go` — `RenderOAuthAllowPage` function (~line 731)** Before writing the response, add: ```go func RenderOAuthAllowPage(rw http.ResponseWriter, r *http.Request, data RenderOAuthAllowData) { rw.Header().Set("Content-Type", "text/html; charset=utf-8") // Prevent the consent page from being framed to mitigate // clickjacking attacks (coder/security#121). rw.Header().Set("Content-Security-Policy", "frame-ancestors 'none'") rw.Header().Set("X-Frame-Options", "DENY") ... ``` Both headers for defense-in-depth (CSP for modern browsers, X-Frame-Options for legacy). ### OAuth 2.1 — Mandatory PKCE Currently PKCE is checked only when `code_challenge` was provided during authorization (`tokens.go:258`): ```go // CURRENT — conditional check if dbCode.CodeChallenge.Valid && dbCode.CodeChallenge.String != "" { // verify PKCE } ``` OAuth 2.1 requires PKCE for ALL authorization code flows. Change to: **File: `coderd/oauth2provider/authorize.go`** — Add `"code_challenge"` to required params: ```go p.RequiredNotEmpty("response_type", "client_id", "code_challenge") ``` **File: `coderd/oauth2provider/tokens.go:257-265`** — Make PKCE verification unconditional: ```go // AFTER — PKCE always required (OAuth 2.1) if req.CodeVerifier == "" { return codersdk.OAuth2TokenResponse{}, errInvalidPKCE } if !dbCode.CodeChallenge.Valid || dbCode.CodeChallenge.String == "" { // Code was issued without a challenge — should not happen // with the authorize endpoint enforcement, but defend in // depth. return codersdk.OAuth2TokenResponse{}, errInvalidPKCE } if !VerifyPKCE(dbCode.CodeChallenge.String, req.CodeVerifier) { return codersdk.OAuth2TokenResponse{}, errInvalidPKCE } ``` **File: `codersdk/oauth2.go`** — Remove `OAuth2ProviderResponseTypeToken` from the enum or reject it explicitly in the authorize handler. Currently it's defined at line 216 but the handler ignores `response_type` and always issues a code. We should either: - (a) Remove the `"token"` variant from the enum and reject it with `unsupported_response_type`, OR - (b) Add an explicit check in `ProcessAuthorize` that rejects `response_type=token` Option (b) is simpler and more backwards-compatible: ```go // In ProcessAuthorize, after extracting params: if params.responseType != codersdk.OAuth2ProviderResponseTypeCode { httpapi.WriteOAuth2Error(ctx, rw, http.StatusBadRequest, codersdk.OAuth2ErrorCodeUnsupportedResponseType, "Only response_type=code is supported") return } ``` ### OAuth 2.1 — Bearer tokens in query strings `coderd/httpmw/apikey.go:743` accepts `access_token` from URL query parameters. OAuth 2.1 prohibits this. However, this may be used internally (e.g., workspace apps, DERP). Need to audit callers before removing. **Approach:** This is a larger change with potential breakage. Mark as a **separate follow-up issue** rather than including in this PR. Document the finding. ### OAuth 2.1 — Removed flows ✅ **Already compliant.** `tokens.go` only supports `authorization_code` and `refresh_token` grant types. The implicit grant (`response_type=token`) will be explicitly rejected per the PKCE section above. ### OAuth 2.1 — Refresh token rotation ✅ **Already compliant.** `tokens.go:442` deletes the old API key when a refresh token is used. ## Migration Plan All DB changes can go in a single new migration (or extend 000420 if the branch is rebased before merge). Columns to add: - `redirect_uri text` on `oauth2_provider_app_codes` The `state_hash` column is already added by migration 000420. ## Implementation Order 1. **Fix 7** — CSP headers on consent page (isolated, no deps) 2. ~~**Fix 2** — Require `state` parameter~~ (DROPPED — state stays optional) 3. **Fix 4** — Exact redirect URI matching + store/verify redirect_uri 4. **PKCE mandatory** — Require `code_challenge` + reject `response_type=token` 5. **Rollback** — Remove `"state"` from `RequiredNotEmpty` in `authorize.go` 6. **Tests** — Update/add tests for all changes 7. **`make gen`** after DB changes ## Out of Scope (separate PRs) - Bearer tokens in query strings (needs internal caller audit) - Scope enforcement on OAuth2 tokens - Rate limiting / quota on dynamic client registration </details> --- _Generated with [`mux`](https://github.com/coder/mux) • Model: `anthropic:claude-opus-4-6` • Thinking: `xhigh`_ |
||
|
Thomas Kosiewski
|
dd6aec04d7 | fix(coderd/oauth2provider): support client_secret_basic client auth (#21793) | ||
|
Thomas Kosiewski
|
cadf1352b4 |
feat: add scoped token support to CLI (#19985)
<!-- If you have used AI to produce some or all of this PR, please ensure you have read our [AI Contribution guidelines](https://coder.com/docs/about/contributing/AI_CONTRIBUTING) before submitting. --> Add support for scoped API tokens in CLI This PR adds CLI support for creating and viewing API tokens with scopes and allow lists. It includes: - New `--scope` and `--allow` flags for the `tokens create` command - A new `tokens view` command to display detailed information about a token - Updated table columns in `tokens list` to show scopes and allow list entries - Updated help text and examples These changes enable users to create tokens with limited permissions through the CLI, similar to the existing functionality in the web UI. |
||
|
Thomas Kosiewski
|
c6e551f538 | fix: renumber api key allow list migration (#20457) | ||
|
Thomas Kosiewski
|
f684831f56 |
feat: add allow list to API keys (#19972)
Add API key allow list to the SDK This PR adds an allow list to API keys in the SDK. The allow list is a list of targets that the API key is allowed to access. If the allow list is empty, a default allow list with a single entry that allows access to all resources is created. The changes include: - Adding a default allow list when generating an API key if none is provided - Adding allow list to the API key response in the SDK - Converting database allow list entries to SDK format in the API response - Adding tests to verify the default allow list behavior Fixes #19854 |
||
|
Thomas Kosiewski
|
251f787743 | fix: normalize oauth2 scope parsing (#20359) | ||
|
Thomas Kosiewski
|
ed90ecf00e |
feat: add allow_list to resource-scoped API tokens (#19964)
# Add API key allow_list for resource-scoped tokens This PR adds support for API key allow lists, enabling tokens to be scoped to specific resources. The implementation: 1. Adds a new `allow_list` field to the `CreateTokenRequest` struct, allowing clients to specify resource-specific scopes when creating API tokens 2. Implements `APIAllowListTarget` type to represent resource targets in the format `<type>:<id>` with support for wildcards 3. Adds validation and normalization logic for allow lists to handle wildcards and deduplication 4. Integrates with RBAC by creating an `APIKeyEffectiveScope` that merges API key scopes with allow list restrictions 5. Updates API documentation and TypeScript types to reflect the new functionality This feature enables creating tokens that are limited to specific resources (like workspaces or templates) by ID, making it possible to create more granular API tokens with limited access. |
||
|
Thomas Kosiewski
|
b60ae0a0c4 |
refactor: add wildcard scope entries for API key scopes (#20032)
# Add API Key Scope Wildcards This PR adds wildcard API key scopes (`resource:*`) for all RBAC resources to ensure every resource has a matching wildcard value. It also adds all individual `resource:action` scopes to the API documentation and TypeScript definitions. The changes include: - Adding a new database migration (000377) that adds wildcard API key scopes - Updating the API documentation to include all available scopes - Enhancing the scope generation scripts to include all resource wildcards - Updating the TypeScript definitions to match the expanded scope list These changes make creating API keys with comprehensive permissions for specific resource types easier. |
||
|
Thomas Kosiewski
|
79126ab6c7 |
feat: implement composite API key scopes for workspaces and templates (#19945)
# Add Composite API Key Scopes This PR adds high-level composite API key scopes to simplify token creation with common permission sets: - `coder:workspaces.create` - Create and update workspaces - `coder:workspaces.operate` - Read and update workspaces - `coder:workspaces.delete` - Read and delete workspaces - `coder:workspaces.access` - Read, SSH, and connect to workspace applications - `coder:templates.build` - Read templates and create/read files - `coder:templates.author` - Full template management with insights - `coder:apikeys.manage_self` - Manage your own API keys These composite scopes are persisted in the database and expanded during authorization, providing a more intuitive way to grant permissions compared to the granular resource:action scopes. |
||
|
Thomas Kosiewski
|
05537c1894 |
feat: publish RBAC scopes in OAuth2 metadata endpoints (#19942)
<!-- If you have used AI to produce some or all of this PR, please ensure you have read our [AI Contribution guidelines](https://coder.com/docs/about/contributing/AI_CONTRIBUTING) before submitting. --> Publish supported OAuth2 scopes from RBAC external scope names This PR updates the OAuth2 metadata endpoints to publish the supported scopes from the RBAC external scope names. Previously, the `ScopesSupported` field was empty with a TODO to implement a scope system. Now, both the authorization server metadata and protected resource metadata endpoints return the list of scopes from `rbac.ExternalScopeNames()`. The tests have been updated to verify that the correct scopes are being returned in the metadata responses. |
||
|
Thomas Kosiewski
|
d0db9ec88f |
feat: add multi-scope support to API keys (#19917)
# Canonicalize API Key Scopes This PR introduces canonical API key scopes with a `coder:` namespace prefix to avoid collisions with low-level resource:action names. It: 1. Renames special API key scopes in the database: - `all` → `coder:all` - `application_connect` → `coder:application_connect` 2. Adds support for a new `scopes` field in the API key creation request, allowing multiple scopes to be specified while maintaining backward compatibility with the singular `scope` field. 3. Updates the API documentation to reflect these changes, including the new endpoint for listing public API key scopes. 4. Ensures backward compatibility by mapping between legacy and canonical scope names in relevant code paths. |
||
|
Thomas Kosiewski
|
4bda39585d |
feat: add external API key scopes (#19916)
# Add support for low-level API key scopes This PR adds support for fine-grained API key scopes based on RBAC resource:action pairs. It includes: 1. A new endpoint `/api/v2/auth/scopes` to list all public low-level API key scopes 2. Generated constants in the SDK for all public scopes 3. Tests to verify scope validation during token creation 4. Updated API documentation to reflect the expanded scope options The implementation allows users to create API keys with specific permissions like `workspace:read` or `template:use` instead of only the legacy `all` or `application_connect` scopes. Fixes #19847 |
||
|
Thomas Kosiewski
|
47c92ad1d2 |
feat: add public RBAC scope catalog for user-requestable permissions (#19913)
# Add a curated catalog of public RBAC scopes This PR introduces a curated catalog of public RBAC scopes that are exposed to users. It adds: - A `publicLowLevel` map in `scopes_catalog.go` that defines which resource:action pairs are user-requestable - `IsPublicLowLevel()` function to check if a scope is in the public catalog - `PublicLowLevelScopeNames()` function that returns a sorted list of public scopes - Tests to verify the catalog entries are valid and properly sorted - Updated documentation in the check-scopes README to clarify that public scopes should be added to this catalog This change helps distinguish between internal-only scopes and those that should be exposed to users in the API. |
||
|
Thomas Kosiewski
|
adb7521066 |
feat: generate RBAC scope name constants (#19896)
# Generate RBAC scope name constants This PR adds a new generated file `coderd/rbac/scopes_constants_gen.go` that contains typed constants for all RBAC scope names in the format `Scope<Resource><Action>`. For example, `ScopeWorkspaceRead` for the scope "workspace:read". These constants make it easier to reference specific scopes in code without using string literals, improving type safety and making refactoring easier. The PR: - Adds a new template file `scripts/typegen/scopenames.gotmpl` - Updates the typegen script to support generating scope name constants - Updates the Makefile to include the new generated file in build targets |
||
|
Thomas Kosiewski
|
acc0890dce |
feat: add lint check for API key scope enum completeness (#19862)
Added a script/linter to ensure all `policy.RBACPermissions` entries are part of the `api_key_scope` enumerated in the `coderd/database/dump.sql` file. Fixes #19846 |
||
|
Thomas Kosiewski
|
77d19c99cd | ci: fix shellcheck lint error (#19933) | ||
|
Thomas Kosiewski
|
fb0ce389a6 |
feat: implement API key scopes database migration (#19861)
Added database migration for API key scopes. Fixes #19845 |
||
|
Thomas Kosiewski
|
6fb4cc6b82 |
fix: add retry logic to OAuth2 metadata tests to avoid race conditions (#19813)
This PR adds a readiness wait to OAuth2 metadata endpoint tests to avoid rare races with server startup. Instead of immediately making HTTP requests, the tests now use `testutil.Eventually` to retry the requests until they succeed, with a short interval between attempts. This helps prevent flaky tests that might fail due to timing issues during server initialization. Fixes: https://github.com/coder/internal/issues/996 |
||
|
Thomas Kosiewski
|
4d8dc221bf |
chore: upgrade Node.js from 20.19.4 to 22.19.0 and update dependencies (#19870)
# Update Node.js from 20.19.4 to 22.19.0 This PR updates Node.js from v20.19.4 to v22.19.0 across the codebase. The change includes: - Updated Node.js version in GitHub Actions setup-node workflow - Updated Node.js version in the dogfood Dockerfile - Changed from `pkgs.nodejs_20` to `unstablePkgs.nodejs_22` in the Nix flake - Updated the Node.js engine version constraints in package.json files to allow Node.js 22 - Updated Playwright from v1.47.0 to v1.50.1 - Updated tzdata dependency from v1.0.44 to v1.0.46 - Updated the flake.lock file with latest nixpkgs references The PR also improves the error message for Playwright version mismatches by showing the actual versions in the error. |
||
|
Thomas Kosiewski
|
d238480c7a | fix: trim whitespace from API tokens (#19814) | ||
|
Thomas Kosiewski
|
088d14933c | feat: ensure OAuth2 refresh tokens outlive access tokens (#19769) | ||
|
Thomas Kosiewski
|
2701d5588e |
fix: support path parameters in OAuth2 metadata endpoints (#19729)
Update OAuth2 metadata endpoint routes to support path suffixes This PR updates the OAuth2 metadata endpoint routes to include a wildcard character (*) at the end of the paths. This change allows the endpoints to match requests with path suffixes, making our OAuth2 discovery implementation more flexible and compliant with the relevant RFCs. The updated routes are: - `/.well-known/oauth-authorization-server*` for RFC 8414 discovery - `/.well-known/oauth-protected-resource*` for RFC 9728 discovery |
||
|
Thomas Kosiewski
|
b7e026682a |
fix: skip bash tests on Windows (#19277)
Adds Windows compatibility to toolsdk tests This PR adds Windows compatibility to the toolsdk tests by: 1. Adding build constraints to exclude bash_test.go from running on Windows 2. Skipping the WorkspaceSSHExec test on Windows platforms with a clear message These changes ensure tests run properly across all supported platforms. Related to https://github.com/coder/internal/issues/798 Signed-off-by: Thomas Kosiewski <tk@coder.com> |
||
|
Thomas Kosiewski
|
247efc0dcc |
docs: add OAuth2 provider experimental feature documentation (#19165)
# Add OAuth2 Provider Documentation This PR adds comprehensive documentation for the experimental OAuth2 Provider feature, which allows Coder to function as an OAuth2 authorization server. The documentation covers: - Feature overview and experimental status warning - Setup requirements and enabling the feature - Methods for creating OAuth2 applications (UI and API) - Integration patterns including standard OAuth2 and PKCE flows - Discovery endpoints and token management - Testing and development guidance - Troubleshooting common issues - Security considerations and current limitations The documentation is marked as experimental and includes appropriate warnings about production usage. Signed-off-by: Thomas Kosiewski <tk@coder.com> |
||
|
Thomas Kosiewski
|
398e80f003 |
feat: add timeout support to workspace bash tool (#19035)
# Add timeout support to workspace bash tool This PR adds a timeout feature to the workspace bash tool, allowing users to specify a maximum execution time for commands. Key changes include: - Added a `timeout_ms` parameter to control command execution time (defaults to 60 seconds, with a maximum of 5 minutes) - Implemented a new `executeCommandWithTimeout` function that properly handles command timeouts - Added proper output capturing during timeout scenarios, returning all output collected before the timeout - Updated documentation to explain the timeout feature and provide usage examples - Added comprehensive tests for the timeout functionality, including integration tests When a command times out, the tool now returns all captured output up to that point along with a cancellation message, making it clear to users what happened. Signed-off-by: Thomas Kosiewski <tk@coder.com> |
||
|
Thomas Kosiewski
|
d1595781e1 |
fix: fix nil pointer dereference in ReportTask (#19045)
This pull request addresses a bug related to a nil pointer dereference in the task reporting functionality. ### Bug Fixes and Error Handling: * Updated `RegisterTools` in `mcp.go` to skip registering the `ReportTask` tool in the remote MCP context when a task reporter is not configured, preventing potential nil pointer dereference panics. * Added a check in `toolsdk.go` to ensure task reporting dependencies are available before invoking the reporter, returning an appropriate error if not. ### Test Coverage: * Added `TestReportTaskNilPointerDeref` in `toolsdk_test.go` to verify that the system does not panic when task reporting dependencies are missing and instead returns a clear error message. * Added `TestReportTaskWithReporter` in `toolsdk_test.go` to validate correct behavior when a task reporter is configured, ensuring the handler processes the request as expected. Signed-off-by: Thomas Kosiewski <tk@coder.com> |
||
|
Thomas Kosiewski
|
c6efe64a65 |
fix: handle nil writer in bash MCP tool (#18978)
- Refactors the bash tool to use `io.Discard` instead of nil to avoid panics. - Enhances panic recovery in `codersdk/toolsdk/toolsdk.go` by adding stack trace information in development builds. When a panic occurs in a tool handler: - In development builds: The error includes the full stack trace for easier debugging - In production builds: A simpler error message is shown without the stack trace |
||
|
Thomas Kosiewski
|
326c02459f |
feat: add workspace SSH execution tool for AI SDK (#18924)
# Add SSH Command Execution Tool for Coder Workspaces This PR adds a new AI tool `coder_workspace_ssh_exec` that allows executing commands in Coder workspaces via SSH. The tool provides functionality similar to the `coder ssh <workspace> <command>` CLI command. Key features: - Executes commands in workspaces via SSH and returns the output and exit code - Automatically starts workspaces if they're stopped - Waits for the agent to be ready before executing commands - Trims leading and trailing whitespace from command output - Supports various workspace identifier formats: - `workspace` (uses current user) - `owner/workspace` - `owner--workspace` - `workspace.agent` (specific agent) - `owner/workspace.agent` The implementation includes: - A new tool definition with schema and handler - Helper functions for workspace and agent discovery - Workspace name normalization to handle different input formats - Comprehensive test coverage including integration tests This tool enables AI assistants to execute commands in user workspaces, making it possible to automate tasks and provide more interactive assistance. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Introduced the ability to execute bash commands inside a Coder workspace via SSH, supporting multiple workspace identification formats. * **Tests** * Added comprehensive unit and integration tests for executing bash commands in workspaces, including input validation, output handling, and error scenarios. * **Chores** * Registered the new bash execution tool in the global tools list. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Thomas Kosiewski
|
1db096d8f9 |
chore: fix CodeRabbit config to disable review status (#18973)
Disable review status in CodeRabbit configuration Change-Id: I0ee266e0b284832b65762a4f7a3f26d56af53e86 Signed-off-by: Thomas Kosiewski <tk@coder.com> |
||
|
Thomas Kosiewski
|
4ac6be6d83 | chore: add CodeRabbit config with disabled auto-reviews (#18949) | ||
|
Thomas Kosiewski
|
0d3b7703f7 |
docs: remove dbmem references from documentation files (#18861)
Change-Id: Ic33bc383d00d0e354c25a0dd6080a4307d9862b6 Signed-off-by: Thomas Kosiewski <tk@coder.com> |
||
|
Thomas Kosiewski
|
fcd361d374 |
feat: add logo SVG and replace inline SVG with image reference (#18930)
# Replace SVG with external logo file in OAuth2 authorization page This PR replaces the inline SVG logo in the OAuth2 authorization page with a reference to an external SVG file. The change: 1. Adds a new `logo.svg` file in the static directory with the Coder logo 2. Updates the OAuth2 authorization page to use this external file instead of embedding the SVG directly This approach improves maintainability by centralizing the logo in a single file and reduces duplication in the codebase. |
||
|
Thomas Kosiewski
|
7b06fc77ae |
refactor: simplify OAuth2 authorization flow and use 302 redirects (#18923)
# Refactor OAuth2 Provider Authorization Flow This PR refactors the OAuth2 provider authorization flow by: 1. Removing the `authorizeMW` middleware and directly implementing its functionality in the `ShowAuthorizePage` handler 2. Simplifying function signatures by removing unnecessary parameters: - Removed `db` parameter from `ShowAuthorizePage` - Removed `accessURL` parameter from `ProcessAuthorize` 3. Changing the redirect status code in `ProcessAuthorize` from 307 (Temporary Redirect) to 302 (Found) to improve compatibility with external OAuth2 apps and browsers. (Technical explanation: we replied with a 307 to a POST request, thus the browser performs a redirect to that URL as a POST request, but we need it to be a GET request to be compatible. Thus, we use the 302 redirect so that browsers turn it into a GET request when redirecting back to the redirect_uri.) The changes maintain the same functionality while simplifying the code and improving compatibility with external systems. |
||
|
Thomas Kosiewski
|
071383bbe8 |
feat: add RFC 9728 OAuth2 resource metadata support (#18920)
# Enhanced OAuth2 and MCP Compliance for API Authentication This PR improves OAuth2 and MCP (Microsoft Cloud for Sovereignty) compliance by: 1. Adding RFC 9728 compliant `WWW-Authenticate` headers with resource metadata URLs 2. Passing the configured `AccessURL` to API key middleware for proper audience validation 3. Creating specialized CORS handling for OAuth2 and MCP endpoints with appropriate headers 4. Making the `state` parameter optional in OAuth2 authorization requests These changes ensure proper OAuth2 token audience validation against the configured access URL and improve interoperability with OAuth2 clients by providing better error responses and metadata discovery. Signed-off-by: Thomas Kosiewski <tk@coder.com> |
||
|
Thomas Kosiewski
|
3dcd2acf1d |
fix: return 404 instead of 401 for missing OAuth2 apps (#18755)
## Problem Users were being automatically logged out when deleting OAuth2 applications. ## Root Cause 1. User deletes OAuth2 app successfully 2. React Query automatically refetches the app data 3. Management API incorrectly returned **401 Unauthorized** for the missing app 4. Frontend axios interceptor sees 401 and calls `signOut()` 5. User gets logged out unexpectedly ## Solution - Change management API to return **404 Not Found** for missing OAuth2 apps - OAuth2 protocol endpoints continue returning 401 per RFC 6749 - Rename `writeInvalidClient` to `writeClientNotFound` for clarity ## Additional Changes - Add conditional OAuth2 navigation when experiment is enabled or in dev builds - Add `isDevBuild()` utility and `buildInfo` to dashboard context - Minor improvements to format script and warning dialogs Signed-off-by: Thomas Kosiewski <tk@coder.com> |
||
|
Thomas Kosiewski
|
4607e5113b |
refactor: organize OAuth2 provider tests into dedicated packages (#18747)
# OAuth2 Provider Code Reorganization This PR reorganizes the OAuth2 provider code to improve separation of concerns and maintainability. The changes include: 1. Migrating OAuth2 provider app validation tests from `coderd/oauth2_test.go` to `oauth2provider/provider_test.go` 2. Moving OAuth2 client registration validation tests to `oauth2provider/validation_test.go` 3. Adding new comprehensive test files for metadata and validation edge cases 4. Renaming `OAuth2ProviderAppSecret` to `AppSecret` for better naming consistency 5. Simplifying the main integration test in `oauth2_test.go` to focus on core functionality The PR maintains all existing test coverage while organizing the code more logically, making it easier to understand and maintain the OAuth2 provider implementation. This reorganization will help with future enhancements to the OAuth2 provider functionality. |
||
|
Thomas Kosiewski
|
c65013384a |
refactor: move OAuth2 provider code to dedicated package (#18746)
# Refactor OAuth2 Provider Code into Dedicated Package This PR refactors the OAuth2 provider functionality by moving it from the main `coderd` package into a dedicated `oauth2provider` package. The change improves code organization and maintainability without changing functionality. Key changes: - Created a new `oauth2provider` package to house all OAuth2 provider-related code - Moved existing OAuth2 provider functionality from `coderd/identityprovider` to the new package - Refactored handler functions to follow a consistent pattern of returning `http.HandlerFunc` instead of being handlers directly - Split large files into smaller, more focused files organized by functionality: - `app_secrets.go` - Manages OAuth2 application secrets - `apps.go` - Handles OAuth2 application CRUD operations - `authorize.go` - Implements the authorization flow - `metadata.go` - Provides OAuth2 metadata endpoints - `registration.go` - Handles dynamic client registration - `revoke.go` - Implements token revocation - `secrets.go` - Manages secret generation and validation - `tokens.go` - Handles token issuance and validation This refactoring improves code organization and makes the OAuth2 provider functionality more maintainable while preserving all existing behavior. |
||
|
Thomas Kosiewski
|
7fbb3ced5b |
feat: add MCP HTTP server experiment and improve experiment middleware (#18712)
# Add MCP HTTP Server Experiment This PR adds a new experiment flag `mcp-server-http` to enable the MCP HTTP server functionality. The changes include: 1. Added a new experiment constant `ExperimentMCPServerHTTP` with the value "mcp-server-http" 2. Added display name and documentation for the new experiment 3. Improved the experiment middleware to: - Support requiring multiple experiments - Provide better error messages with experiment display names - Add a development mode bypass option 4. Applied the new experiment requirement to the MCP HTTP endpoint 5. Replaced the custom OAuth2 middleware with the standard experiment middleware The PR also improves the `Enabled()` method on the `Experiments` type by using `slices.Contains()` for better readability. |
||
|
Thomas Kosiewski
|
15551541e8 |
feat: add OAuth2 provider functionality as an experiment (#18692)
# Add OAuth2 Provider Functionality as an Experiment This PR adds a new experiment flag `oauth2` that enables OAuth2 provider functionality in Coder. When enabled, this experiment allows Coder to act as an OAuth2 provider. The changes include: - Added the new `ExperimentOAuth2` constant with appropriate documentation - Updated the OAuth2 provider middleware to check for the experiment flag - Modified the error message to indicate that the OAuth2 provider requires enabling the experiment - Added the new experiment to the known experiments list in the SDK Previously, OAuth2 provider functionality was only available in development mode. With this change, it can be enabled in production environments by activating the experiment. |
||
|
Thomas Kosiewski
|
494dccc510 |
feat: implement MCP HTTP server endpoint with authentication (#18670)
# Add MCP HTTP server with streamable transport support - Add MCP HTTP server with streamable transport support - Integrate with existing toolsdk for Coder workspace operations - Add comprehensive E2E tests with OAuth2 bearer token support - Register MCP endpoint at /api/experimental/mcp/http with authentication - Support RFC 6750 Bearer token authentication for MCP clients Change-Id: Ib9024569ae452729908797c42155006aa04330af Signed-off-by: Thomas Kosiewski <tk@coder.com> |
||
|
Thomas Kosiewski
|
60b08f0960 |
fix: remove unique constraint on OAuth2 provider app names (#18669)
# Remove unique constraint on OAuth2 provider app names This PR removes the unique constraint on the `name` field in the `oauth2_provider_apps` table to comply with RFC 7591, which only requires unique client IDs, not unique client names. Changes include: - Removing the unique constraint from the database schema - Adding migration files for both up and down migrations - Removing the name uniqueness check in the in-memory database implementation - Updating the unique constraint constants Change-Id: Iae7a1a06546fbc8de541a52e291f8a4510d57e8a Signed-off-by: Thomas Kosiewski <tk@coder.com> |
||
|
Thomas Kosiewski
|
4dcf0c3e7e |
docs: add comprehensive development documentation (#18646)
# Organize Development Documentation into Separate Files This PR reorganizes the development documentation by splitting the monolithic CLAUDE.md file into multiple focused documents. The main file now provides a concise overview with essential commands and critical patterns, while importing detailed content from specialized guides. Key improvements: - Created separate documentation files for specific domains: - Database development patterns - OAuth2 implementation guidelines - Testing best practices - Troubleshooting common issues - Development workflows and guidelines - Restructured the main CLAUDE.md to be more scannable with improved formatting - Added quick-reference tables for common commands - Maintained all existing content while making it more accessible - Highlighted critical patterns that must be followed This organization makes the documentation more maintainable and easier to navigate, allowing developers to quickly find relevant information for their specific tasks. |
||
|
Thomas Kosiewski
|
74e1d5c4b6 |
feat: implement OAuth2 dynamic client registration (RFC 7591/7592) (#18645)
# Implement OAuth2 Dynamic Client Registration (RFC 7591/7592)
This PR implements OAuth2 Dynamic Client Registration according to RFC 7591 and Client Configuration Management according to RFC 7592. These standards allow OAuth2 clients to register themselves programmatically with Coder as an authorization server.
Key changes include:
1. Added database schema extensions to support RFC 7591/7592 fields in the `oauth2_provider_apps` table
2. Implemented `/oauth2/register` endpoint for dynamic client registration (RFC 7591)
3. Added client configuration management endpoints (RFC 7592):
- GET/PUT/DELETE `/oauth2/clients/{client_id}`
- Registration access token validation middleware
4. Added comprehensive validation for OAuth2 client metadata:
- URI validation with support for custom schemes for native apps
- Grant type and response type validation
- Token endpoint authentication method validation
5. Enhanced developer documentation with:
- RFC compliance guidelines
- Testing best practices to avoid race conditions
- Systematic debugging approaches for OAuth2 implementations
The implementation follows security best practices from the RFCs, including proper token handling, secure defaults, and appropriate error responses. This enables third-party applications to integrate with Coder's OAuth2 provider capabilities programmatically.
|
||
|
Thomas Kosiewski
|
09c50559f3 |
feat: implement RFC 6750 Bearer token authentication (#18644)
# Add RFC 6750 Bearer Token Authentication Support This PR implements RFC 6750 Bearer Token authentication as an additional authentication method for Coder's API. This allows clients to authenticate using standard OAuth 2.0 Bearer tokens in two ways: 1. Using the `Authorization: Bearer <token>` header 2. Using the `access_token` query parameter Key changes: - Added support for extracting tokens from both Bearer headers and access_token query parameters - Implemented proper WWW-Authenticate headers for 401/403 responses with appropriate error descriptions - Added comprehensive test coverage for the new authentication methods - Updated the OAuth2 protected resource metadata endpoint to advertise Bearer token support - Enhanced the OAuth2 testing script to verify Bearer token functionality These authentication methods are added as fallback options, maintaining backward compatibility with Coder's existing authentication mechanisms. The existing authentication methods (cookies, session token header, etc.) still take precedence. This implementation follows the OAuth 2.0 Bearer Token specification (RFC 6750) and improves interoperability with standard OAuth 2.0 clients. |
||
|
Thomas Kosiewski
|
33bbf18a4b |
feat: add OAuth2 protected resource metadata endpoint for RFC 9728 (#18643)
# Add OAuth2 Protected Resource Metadata Endpoint This PR implements the OAuth2 Protected Resource Metadata endpoint according to RFC 9728. The endpoint is available at `/.well-known/oauth-protected-resource` and provides information about Coder as an OAuth2 protected resource. Key changes: - Added a new endpoint at `/.well-known/oauth-protected-resource` that returns metadata about Coder as an OAuth2 protected resource - Created a new `OAuth2ProtectedResourceMetadata` struct in the SDK - Added tests to verify the endpoint functionality - Updated API documentation to include the new endpoint The implementation currently returns basic metadata including the resource identifier and authorization server URL. The `scopes_supported` field is empty until a scope system based on RBAC permissions is implemented. The `bearer_methods_supported` field is omitted as Coder uses custom authentication methods rather than standard RFC 6750 bearer tokens. A TODO has been added to implement RFC 6750 bearer token support in the future. |