From 83405677bf75a7ab69847e40dae1c33b410002d9 Mon Sep 17 00:00:00 2001 From: Jon Ayers Date: Mon, 31 Mar 2025 22:56:21 -0400 Subject: [PATCH 001/498] chore: pin goimports to 0.31.0 (#17177) --- .github/workflows/ci.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 2ff0978e5d807..1e23aa991bb1f 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -252,7 +252,7 @@ jobs: run: | go install google.golang.org/protobuf/cmd/protoc-gen-go@v1.30 go install storj.io/drpc/cmd/protoc-gen-go-drpc@v0.0.34 - go install golang.org/x/tools/cmd/goimports@latest + go install golang.org/x/tools/cmd/goimports@v0.31.0 go install github.com/mikefarah/yq/v4@v4.44.3 go install go.uber.org/mock/mockgen@v0.5.0 From 989c3ec62e7f5c3d55b04ac3dbb650cfb6de6278 Mon Sep 17 00:00:00 2001 From: Jon Ayers Date: Tue, 1 Apr 2025 00:15:15 -0400 Subject: [PATCH 002/498] chore: pin various dependencies in CI files (#17180) --- .github/workflows/ci.yaml | 2 +- dogfood/coder/Dockerfile | 17 +++++++++++------ 2 files changed, 12 insertions(+), 7 deletions(-) diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 1e23aa991bb1f..2b4f69fb2e72f 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -860,7 +860,7 @@ jobs: run: | go install google.golang.org/protobuf/cmd/protoc-gen-go@v1.30 go install storj.io/drpc/cmd/protoc-gen-go-drpc@v0.0.34 - go install golang.org/x/tools/cmd/goimports@latest + go install golang.org/x/tools/cmd/goimports@v0.31.0 go install github.com/mikefarah/yq/v4@v4.44.3 go install go.uber.org/mock/mockgen@v0.5.0 diff --git a/dogfood/coder/Dockerfile b/dogfood/coder/Dockerfile index d23156caf94f8..a5eb1c411883e 100644 --- a/dogfood/coder/Dockerfile +++ b/dogfood/coder/Dockerfile @@ -34,7 +34,7 @@ RUN apt-get update && \ # go-swagger tool to generate the go coder api client go install github.com/go-swagger/go-swagger/cmd/swagger@v0.28.0 && \ # goimports for updating imports - go install golang.org/x/tools/cmd/goimports@v0.1.7 && \ + go install golang.org/x/tools/cmd/goimports@v0.31.0 && \ # protoc-gen-go is needed to build sysbox from source go install google.golang.org/protobuf/cmd/protoc-gen-go@v1.30 && \ # drpc support for v2 @@ -45,7 +45,7 @@ RUN apt-get update && \ go install github.com/goreleaser/goreleaser@v1.6.1 && \ # Install the latest version of gopls for editors that support # the language server protocol - go install golang.org/x/tools/gopls@latest && \ + go install golang.org/x/tools/gopls@v0.18.1 && \ # gotestsum makes test output more readable go install gotest.tools/gotestsum@v1.9.0 && \ # goveralls collects code coverage metrics from tests @@ -84,7 +84,8 @@ RUN apt-get update && \ rm -rf /tmp/go/pkg && \ rm -rf /tmp/go/src -FROM gcr.io/coder-dev-1/alpine:3.18 as proto +# alpine:3.18 +FROM gcr.io/coder-dev-1/alpine@sha256:25fad2a32ad1f6f510e528448ae1ec69a28ef81916a004d3629874104f8a7f70 AS proto WORKDIR /tmp RUN apk add curl unzip RUN curl -L -o protoc.zip https://github.com/protocolbuffers/protobuf/releases/download/v23.4/protoc-23.4-linux-x86_64.zip && \ @@ -232,18 +233,22 @@ RUN DOCTL_VERSION=$(curl -s "https://api.github.com/repos/digitalocean/doctl/rel tar xf doctl.tar.gz -C /usr/local/bin doctl && \ rm doctl.tar.gz +ARG NVM_INSTALL_SHA=bdea8c52186c4dd12657e77e7515509cda5bf9fa5a2f0046bce749e62645076d # Install frontend utilities ENV NVM_DIR=/usr/local/nvm ENV NODE_VERSION=20.16.0 RUN mkdir -p $NVM_DIR -RUN curl https://raw.githubusercontent.com/nvm-sh/nvm/v0.40.0/install.sh | bash +RUN curl -o nvm_install.sh https://raw.githubusercontent.com/nvm-sh/nvm/v0.40.0/install.sh && \ + echo "${NVM_INSTALL_SHA} nvm_install.sh" | sha256sum -c && \ + bash nvm_install.sh && \ + rm nvm_install.sh RUN source $NVM_DIR/nvm.sh && \ nvm install $NODE_VERSION && \ nvm use $NODE_VERSION ENV PATH=$NVM_DIR/versions/node/v$NODE_VERSION/bin:$PATH # Allow patch updates for npm and pnpm -RUN npm install -g npm@^10.8 -RUN npm install -g pnpm@^9.6 +RUN npm install -g npm@10.8.1 +RUN npm install -g pnpm@9.15.1 RUN pnpx playwright@1.47.0 install --with-deps chromium From cc733aba71e3c6be7146ea837bf3f84b502fcfd2 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Tue, 1 Apr 2025 09:03:54 +0100 Subject: [PATCH 003/498] ci: check go versions are consistent (#17149) Fixes https://github.com/coder/coder/issues/17063 I'm ignoring flake.nix for now. ``` $ IGNORE_NIX=true ./scripts/check_go_versions.sh INFO : go.mod : 1.24.1 INFO : dogfood/coder/Dockerfile : 1.24.1 INFO : setup-go/action.yaml : 1.24.1 INFO : flake.nix : 1.22 INFO : Ignoring flake.nix, as IGNORE_NIX=true Go version check passed, all versions are 1.24.1 $ ./scripts/check_go_versions.sh INFO : go.mod : 1.24.1 INFO : dogfood/coder/Dockerfile : 1.24.1 INFO : setup-go/action.yaml : 1.24.1 INFO : flake.nix : 1.22 ERROR: Go version mismatch between go.mod and flake.nix ``` --- .github/workflows/ci.yaml | 3 +++ scripts/check_go_versions.sh | 50 ++++++++++++++++++++++++++++++++++++ 2 files changed, 53 insertions(+) create mode 100755 scripts/check_go_versions.sh diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 2b4f69fb2e72f..64d274d1b46d5 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -299,6 +299,9 @@ jobs: - name: Setup Node uses: ./.github/actions/setup-node + - name: Check Go version + run: IGNORE_NIX=true ./scripts/check_go_versions.sh + # Use default Go version - name: Setup Go uses: ./.github/actions/setup-go diff --git a/scripts/check_go_versions.sh b/scripts/check_go_versions.sh new file mode 100755 index 0000000000000..8349960bd580a --- /dev/null +++ b/scripts/check_go_versions.sh @@ -0,0 +1,50 @@ +#!/usr/bin/env bash + +# This script ensures that the same version of Go is referenced in all of the +# following files: +# - go.mod +# - dogfood/coder/Dockerfile +# - flake.nix +# - .github/actions/setup-go/action.yml +# The version of Go in go.mod is considered the source of truth. + +set -euo pipefail +# shellcheck source=scripts/lib.sh +source "$(dirname "${BASH_SOURCE[0]}")/lib.sh" +cdroot + +# At the time of writing, Nix only has go 1.22.x. +# We don't want to fail the build for this reason. +IGNORE_NIX=${IGNORE_NIX:-false} + +GO_VERSION_GO_MOD=$(grep -Eo 'go [0-9]+\.[0-9]+\.[0-9]+' ./go.mod | cut -d' ' -f2) +GO_VERSION_DOCKERFILE=$(grep -Eo 'ARG GO_VERSION=[0-9]+\.[0-9]+\.[0-9]+' ./dogfood/coder/Dockerfile | cut -d'=' -f2) +GO_VERSION_SETUP_GO=$(yq '.inputs.version.default' .github/actions/setup-go/action.yaml) +GO_VERSION_FLAKE_NIX=$(grep -Eo '\bgo_[0-9]+_[0-9]+\b' ./flake.nix) +# Convert to major.minor format. +GO_VERSION_FLAKE_NIX_MAJOR_MINOR=$(echo "$GO_VERSION_FLAKE_NIX" | cut -d '_' -f 2-3 | tr '_' '.') +log "INFO : go.mod : $GO_VERSION_GO_MOD" +log "INFO : dogfood/coder/Dockerfile : $GO_VERSION_DOCKERFILE" +log "INFO : setup-go/action.yaml : $GO_VERSION_SETUP_GO" +log "INFO : flake.nix : $GO_VERSION_FLAKE_NIX_MAJOR_MINOR" + +if [ "$GO_VERSION_GO_MOD" != "$GO_VERSION_DOCKERFILE" ]; then + error "Go version mismatch between go.mod and dogfood/coder/Dockerfile:" +fi + +if [ "$GO_VERSION_GO_MOD" != "$GO_VERSION_SETUP_GO" ]; then + error "Go version mismatch between go.mod and .github/actions/setup-go/action.yaml" +fi + +# At the time of writing, Nix only constrains the major.minor version. +# We need to check that specifically. +if [ "$IGNORE_NIX" = "false" ]; then + GO_VERSION_GO_MOD_MAJOR_MINOR=$(echo "$GO_VERSION_GO_MOD" | cut -d '.' -f 1-2) + if [ "$GO_VERSION_FLAKE_NIX_MAJOR_MINOR" != "$GO_VERSION_GO_MOD_MAJOR_MINOR" ]; then + error "Go version mismatch between go.mod and flake.nix" + fi +else + log "INFO : Ignoring flake.nix, as IGNORE_NIX=${IGNORE_NIX}" +fi + +log "Go version check passed, all versions are $GO_VERSION_GO_MOD" From e4cf18989c0db6560e785c042f64617af4d88b66 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Tue, 1 Apr 2025 11:28:47 +0100 Subject: [PATCH 004/498] chore(mcp): fix test flakes (#17183) Closes https://github.com/coder/internal/issues/547 --- mcp/mcp_test.go | 64 ++++++++++++++++++++++++++----------------------- 1 file changed, 34 insertions(+), 30 deletions(-) diff --git a/mcp/mcp_test.go b/mcp/mcp_test.go index f2573f44a1be6..1144d9265aa15 100644 --- a/mcp/mcp_test.go +++ b/mcp/mcp_test.go @@ -77,15 +77,12 @@ func TestCoderTools(t *testing.T) { pty.WriteLine(ctr) _ = pty.ReadLine(ctx) // skip the echo - templates, err := memberClient.Templates(ctx, codersdk.TemplateFilter{}) + // Then: the response is a list of expected visible to the user. + expected, err := memberClient.Templates(ctx, codersdk.TemplateFilter{}) require.NoError(t, err) - templatesJSON, err := json.Marshal(templates) - require.NoError(t, err) - - // Then: the response is a list of templates visible to the user. - expected := makeJSONRPCTextResponse(t, string(templatesJSON)) - actual := pty.ReadLine(ctx) - testutil.RequireJSONEq(t, expected, actual) + actual := unmarshalFromCallToolResult[[]codersdk.Template](t, pty.ReadLine(ctx)) + require.Len(t, actual, 1) + require.Equal(t, expected[0].ID, actual[0].ID) }) t.Run("coder_report_task", func(t *testing.T) { @@ -111,20 +108,16 @@ func TestCoderTools(t *testing.T) { t.Run("coder_whoami", func(t *testing.T) { // When: the coder_whoami tool is called - me, err := memberClient.User(ctx, codersdk.Me) - require.NoError(t, err) - meJSON, err := json.Marshal(me) - require.NoError(t, err) - ctr := makeJSONRPCRequest(t, "tools/call", "coder_whoami", map[string]any{}) pty.WriteLine(ctr) _ = pty.ReadLine(ctx) // skip the echo // Then: the response is a valid JSON respresentation of the calling user. - expected := makeJSONRPCTextResponse(t, string(meJSON)) - actual := pty.ReadLine(ctx) - testutil.RequireJSONEq(t, expected, actual) + expected, err := memberClient.User(ctx, codersdk.Me) + require.NoError(t, err) + actual := unmarshalFromCallToolResult[codersdk.User](t, pty.ReadLine(ctx)) + require.Equal(t, expected.ID, actual.ID) }) t.Run("coder_list_workspaces", func(t *testing.T) { @@ -138,15 +131,10 @@ func TestCoderTools(t *testing.T) { pty.WriteLine(ctr) _ = pty.ReadLine(ctx) // skip the echo - ws, err := memberClient.Workspaces(ctx, codersdk.WorkspaceFilter{}) - require.NoError(t, err) - wsJSON, err := json.Marshal(ws) - require.NoError(t, err) - // Then: the response is a valid JSON respresentation of the calling user's workspaces. - expected := makeJSONRPCTextResponse(t, string(wsJSON)) - actual := pty.ReadLine(ctx) - testutil.RequireJSONEq(t, expected, actual) + actual := unmarshalFromCallToolResult[codersdk.WorkspacesResponse](t, pty.ReadLine(ctx)) + require.Len(t, actual.Workspaces, 1, "expected 1 workspace") + require.Equal(t, r.Workspace.ID, actual.Workspaces[0].ID, "expected the workspace to be the one we created in setup") }) t.Run("coder_get_workspace", func(t *testing.T) { @@ -161,15 +149,12 @@ func TestCoderTools(t *testing.T) { pty.WriteLine(ctr) _ = pty.ReadLine(ctx) // skip the echo - ws, err := memberClient.Workspace(ctx, r.Workspace.ID) - require.NoError(t, err) - wsJSON, err := json.Marshal(ws) + expected, err := memberClient.Workspace(ctx, r.Workspace.ID) require.NoError(t, err) // Then: the response is a valid JSON respresentation of the workspace. - expected := makeJSONRPCTextResponse(t, string(wsJSON)) - actual := pty.ReadLine(ctx) - testutil.RequireJSONEq(t, expected, actual) + actual := unmarshalFromCallToolResult[codersdk.Workspace](t, pty.ReadLine(ctx)) + require.Equal(t, expected.ID, actual.ID) }) // NOTE: this test runs after the list_workspaces tool is called. @@ -322,6 +307,25 @@ func makeJSONRPCTextResponse(t *testing.T, text string) string { return string(bs) } +func unmarshalFromCallToolResult[T any](t *testing.T, raw string) T { + t.Helper() + + var resp map[string]any + require.NoError(t, json.Unmarshal([]byte(raw), &resp), "failed to unmarshal JSON RPC response") + res, ok := resp["result"].(map[string]any) + require.True(t, ok, "expected a result field in the response") + ct, ok := res["content"].([]any) + require.True(t, ok, "expected a content field in the result") + require.Len(t, ct, 1, "expected a single content item in the result") + ct0, ok := ct[0].(map[string]any) + require.True(t, ok, "expected a content item in the result") + txt, ok := ct0["text"].(string) + require.True(t, ok, "expected a text field in the content item") + var actual T + require.NoError(t, json.Unmarshal([]byte(txt), &actual), "failed to unmarshal content") + return actual +} + // startTestMCPServer is a helper function that starts a MCP server listening on // a pty. It is the responsibility of the caller to close the server. func startTestMCPServer(ctx context.Context, t testing.TB, stdin io.Reader, stdout io.Writer) (*server.MCPServer, func() error) { From 7d08bf0afe79e75961aeda9407dae05ead3f8942 Mon Sep 17 00:00:00 2001 From: Hugo Dutka Date: Tue, 1 Apr 2025 13:23:06 +0200 Subject: [PATCH 005/498] chore: improve error logging in TestServer/EphemeralDeployment (#17184) There's a flake reported in https://github.com/coder/internal/issues/549 that was caused by the built-in Postgres failing to start. However, the test was written in a way that didn't log the actual error which caused Postgres to fail. This PR improves error logging in the affected test so that the next time the error happens, we know what it is. --- cli/server_test.go | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) diff --git a/cli/server_test.go b/cli/server_test.go index f224fcb43fe63..715cbe5c7584c 100644 --- a/cli/server_test.go +++ b/cli/server_test.go @@ -201,7 +201,16 @@ func TestServer(t *testing.T) { go func() { errCh <- inv.WithContext(ctx).Run() }() - pty.ExpectMatch("Using an ephemeral deployment directory") + matchCh1 := make(chan string, 1) + go func() { + matchCh1 <- pty.ExpectMatchContext(ctx, "Using an ephemeral deployment directory") + }() + select { + case err := <-errCh: + require.NoError(t, err) + case <-matchCh1: + // OK! + } rootDirLine := pty.ReadLine(ctx) rootDir := strings.TrimPrefix(rootDirLine, "Using an ephemeral deployment directory") rootDir = strings.TrimSpace(rootDir) @@ -210,7 +219,17 @@ func TestServer(t *testing.T) { require.NotEmpty(t, rootDir) require.DirExists(t, rootDir) - pty.ExpectMatchContext(ctx, "View the Web UI") + matchCh2 := make(chan string, 1) + go func() { + // The "View the Web UI" log is a decent indicator that the server was successfully started. + matchCh2 <- pty.ExpectMatchContext(ctx, "View the Web UI") + }() + select { + case err := <-errCh: + require.NoError(t, err) + case <-matchCh2: + // OK! + } cancelFunc() <-errCh From 3a243c111b9abb5c38328169ff70064025bbe2fe Mon Sep 17 00:00:00 2001 From: Ethan <39577870+ethanndickson@users.noreply.github.com> Date: Tue, 1 Apr 2025 22:28:05 +1100 Subject: [PATCH 006/498] fix: remove shared mutable state between oidc tests (#17179) Spotted on main: https://github.com/coder/coder/actions/runs/14179449567/job/39721999486 ``` === FAIL: coderd TestOIDCDomainErrorMessage/MalformedEmailErrorOmitsDomains (0.01s) ================== WARNING: DATA RACE Read at 0x00c060b54e68 by goroutine 296485: golang.org/x/oauth2.(*Config).Exchange() /home/runner/go/pkg/mod/golang.org/x/oauth2@v0.28.0/oauth2.go:228 +0x1d8 github.com/coder/coder/v2/coderd.(*OIDCConfig).Exchange() :1 +0xb7 github.com/coder/coder/v2/coderd.New.func11.12.1.2.ExtractOAuth2.1.1() /home/runner/work/coder/coder/coderd/httpmw/oauth2.go:168 +0x7b5 net/http.HandlerFunc.ServeHTTP() /opt/hostedtoolcache/go/1.24.1/x64/src/net/http/server.go:2294 +0x47 [...] Previous write at 0x00c060b54e68 by goroutine 55730: github.com/coder/coder/v2/coderd/coderdtest/oidctest.(*FakeIDP).SetRedirect() /home/runner/work/coder/coder/coderd/coderdtest/oidctest/idp.go:1280 +0x1e6 github.com/coder/coder/v2/coderd/coderdtest/oidctest.(*FakeIDP).LoginWithClient() /home/runner/work/coder/coder/coderd/coderdtest/oidctest/idp.go:494 +0x170 github.com/coder/coder/v2/coderd/coderdtest/oidctest.(*FakeIDP).AttemptLogin() /home/runner/work/coder/coder/coderd/coderdtest/oidctest/idp.go:479 +0x624 github.com/coder/coder/v2/coderd_test.TestOIDCDomainErrorMessage.func3() /home/runner/work/coder/coder/coderd/userauth_test.go:2041 +0x1f2 ``` As seen, this race was caused by sharing a `*oidctest.FakeIDP` between test cases. The fix is to simply do the setup twice. ``` $ go test -race -run "TestOIDCDomainErrorMessage" github.com/coder/coder/v2/coderd -count=100 ok github.com/coder/coder/v2/coderd 7.551s ```` --- coderd/userauth_test.go | 30 +++++++++++++++++++----------- 1 file changed, 19 insertions(+), 11 deletions(-) diff --git a/coderd/userauth_test.go b/coderd/userauth_test.go index ad8e126706dd1..ddf3dceba236f 100644 --- a/coderd/userauth_test.go +++ b/coderd/userauth_test.go @@ -1988,22 +1988,28 @@ func TestUserLogout(t *testing.T) { func TestOIDCDomainErrorMessage(t *testing.T) { t.Parallel() - fake := oidctest.NewFakeIDP(t, oidctest.WithServing()) - allowedDomains := []string{"allowed1.com", "allowed2.org", "company.internal"} - cfg := fake.OIDCConfig(t, nil, func(cfg *coderd.OIDCConfig) { - cfg.EmailDomain = allowedDomains - cfg.AllowSignups = true - }) - server := coderdtest.New(t, &coderdtest.Options{ - OIDCConfig: cfg, - }) + setup := func() (*oidctest.FakeIDP, *codersdk.Client) { + fake := oidctest.NewFakeIDP(t, oidctest.WithServing()) + + cfg := fake.OIDCConfig(t, nil, func(cfg *coderd.OIDCConfig) { + cfg.EmailDomain = allowedDomains + cfg.AllowSignups = true + }) + + client := coderdtest.New(t, &coderdtest.Options{ + OIDCConfig: cfg, + }) + return fake, client + } // Test case 1: Email domain not in allowed list t.Run("ErrorMessageOmitsDomains", func(t *testing.T) { t.Parallel() + fake, client := setup() + // Prepare claims with email from unauthorized domain claims := jwt.MapClaims{ "email": "user@unauthorized.com", @@ -2011,7 +2017,7 @@ func TestOIDCDomainErrorMessage(t *testing.T) { "sub": uuid.NewString(), } - _, resp := fake.AttemptLogin(t, server, claims) + _, resp := fake.AttemptLogin(t, client, claims) defer resp.Body.Close() require.Equal(t, http.StatusForbidden, resp.StatusCode) @@ -2031,6 +2037,8 @@ func TestOIDCDomainErrorMessage(t *testing.T) { t.Run("MalformedEmailErrorOmitsDomains", func(t *testing.T) { t.Parallel() + fake, client := setup() + // Prepare claims with an invalid email format (no @ symbol) claims := jwt.MapClaims{ "email": "invalid-email-without-domain", @@ -2038,7 +2046,7 @@ func TestOIDCDomainErrorMessage(t *testing.T) { "sub": uuid.NewString(), } - _, resp := fake.AttemptLogin(t, server, claims) + _, resp := fake.AttemptLogin(t, client, claims) defer resp.Body.Close() require.Equal(t, http.StatusForbidden, resp.StatusCode) From 1e11e823c9420713991a7838ad94528969d97d56 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Tue, 1 Apr 2025 15:02:08 +0100 Subject: [PATCH 007/498] fix(mcp): report task status correctly (#17187) --- cli/exp_mcp.go | 72 ++++++++++++++++++++------ mcp/mcp.go | 135 +++++++++++++++++------------------------------- mcp/mcp_test.go | 50 ++++++++++++++---- 3 files changed, 144 insertions(+), 113 deletions(-) diff --git a/cli/exp_mcp.go b/cli/exp_mcp.go index a5af41d9103a6..b46a8b4d7f03a 100644 --- a/cli/exp_mcp.go +++ b/cli/exp_mcp.go @@ -4,14 +4,18 @@ import ( "context" "encoding/json" "errors" - "log" "os" "path/filepath" + "github.com/mark3labs/mcp-go/server" + "golang.org/x/xerrors" + "cdr.dev/slog" "cdr.dev/slog/sloggers/sloghuman" + "github.com/coder/coder/v2/buildinfo" "github.com/coder/coder/v2/cli/cliui" "github.com/coder/coder/v2/codersdk" + "github.com/coder/coder/v2/codersdk/agentsdk" codermcp "github.com/coder/coder/v2/mcp" "github.com/coder/serpent" ) @@ -191,14 +195,16 @@ func (*RootCmd) mcpConfigureCursor() *serpent.Command { func (r *RootCmd) mcpServer() *serpent.Command { var ( - client = new(codersdk.Client) - instructions string - allowedTools []string + client = new(codersdk.Client) + instructions string + allowedTools []string + appStatusSlug string + mcpServerAgent bool ) return &serpent.Command{ Use: "server", Handler: func(inv *serpent.Invocation) error { - return mcpServerHandler(inv, client, instructions, allowedTools) + return mcpServerHandler(inv, client, instructions, allowedTools, appStatusSlug, mcpServerAgent) }, Short: "Start the Coder MCP server.", Middleware: serpent.Chain( @@ -209,24 +215,39 @@ func (r *RootCmd) mcpServer() *serpent.Command { Name: "instructions", Description: "The instructions to pass to the MCP server.", Flag: "instructions", + Env: "CODER_MCP_INSTRUCTIONS", Value: serpent.StringOf(&instructions), }, { Name: "allowed-tools", Description: "Comma-separated list of allowed tools. If not specified, all tools are allowed.", Flag: "allowed-tools", + Env: "CODER_MCP_ALLOWED_TOOLS", Value: serpent.StringArrayOf(&allowedTools), }, + { + Name: "app-status-slug", + Description: "When reporting a task, the coder_app slug under which to report the task.", + Flag: "app-status-slug", + Env: "CODER_MCP_APP_STATUS_SLUG", + Value: serpent.StringOf(&appStatusSlug), + Default: "", + }, + { + Flag: "agent", + Env: "CODER_MCP_SERVER_AGENT", + Description: "Start the MCP server in agent mode, with a different set of tools.", + Value: serpent.BoolOf(&mcpServerAgent), + }, }, } } -func mcpServerHandler(inv *serpent.Invocation, client *codersdk.Client, instructions string, allowedTools []string) error { +//nolint:revive // control coupling +func mcpServerHandler(inv *serpent.Invocation, client *codersdk.Client, instructions string, allowedTools []string, appStatusSlug string, mcpServerAgent bool) error { ctx, cancel := context.WithCancel(inv.Context()) defer cancel() - logger := slog.Make(sloghuman.Sink(inv.Stdout)) - me, err := client.User(ctx, codersdk.Me) if err != nil { cliui.Errorf(inv.Stderr, "Failed to log in to the Coder deployment.") @@ -253,19 +274,40 @@ func mcpServerHandler(inv *serpent.Invocation, client *codersdk.Client, instruct inv.Stderr = invStderr }() - options := []codermcp.Option{ - codermcp.WithInstructions(instructions), - codermcp.WithLogger(&logger), + mcpSrv := server.NewMCPServer( + "Coder Agent", + buildinfo.Version(), + server.WithInstructions(instructions), + ) + + // Create a separate logger for the tools. + toolLogger := slog.Make(sloghuman.Sink(invStderr)) + + toolDeps := codermcp.ToolDeps{ + Client: client, + Logger: &toolLogger, + AppStatusSlug: appStatusSlug, + AgentClient: agentsdk.New(client.URL), + } + + if mcpServerAgent { + // Get the workspace agent token from the environment. + agentToken, ok := os.LookupEnv("CODER_AGENT_TOKEN") + if !ok || agentToken == "" { + return xerrors.New("CODER_AGENT_TOKEN is not set") + } + toolDeps.AgentClient.SetSessionToken(agentToken) } - // Add allowed tools option if specified + // Register tools based on the allowlist (if specified) + reg := codermcp.AllTools() if len(allowedTools) > 0 { - options = append(options, codermcp.WithAllowedTools(allowedTools)) + reg = reg.WithOnlyAllowed(allowedTools...) } - srv := codermcp.NewStdio(client, options...) - srv.SetErrorLogger(log.New(invStderr, "", log.LstdFlags)) + reg.Register(mcpSrv, toolDeps) + srv := server.NewStdioServer(mcpSrv) done := make(chan error) go func() { defer close(done) diff --git a/mcp/mcp.go b/mcp/mcp.go index 80e0f341e16e6..0dd01ccdc5fdd 100644 --- a/mcp/mcp.go +++ b/mcp/mcp.go @@ -6,7 +6,6 @@ import ( "encoding/json" "errors" "io" - "os" "slices" "strings" "time" @@ -17,76 +16,12 @@ import ( "golang.org/x/xerrors" "cdr.dev/slog" - "cdr.dev/slog/sloggers/sloghuman" - "github.com/coder/coder/v2/buildinfo" "github.com/coder/coder/v2/coderd/util/ptr" "github.com/coder/coder/v2/codersdk" + "github.com/coder/coder/v2/codersdk/agentsdk" "github.com/coder/coder/v2/codersdk/workspacesdk" ) -type mcpOptions struct { - instructions string - logger *slog.Logger - allowedTools []string -} - -// Option is a function that configures the MCP server. -type Option func(*mcpOptions) - -// WithInstructions sets the instructions for the MCP server. -func WithInstructions(instructions string) Option { - return func(o *mcpOptions) { - o.instructions = instructions - } -} - -// WithLogger sets the logger for the MCP server. -func WithLogger(logger *slog.Logger) Option { - return func(o *mcpOptions) { - o.logger = logger - } -} - -// WithAllowedTools sets the allowed tools for the MCP server. -func WithAllowedTools(tools []string) Option { - return func(o *mcpOptions) { - o.allowedTools = tools - } -} - -// NewStdio creates a new MCP stdio server with the given client and options. -// It is the responsibility of the caller to start and stop the server. -func NewStdio(client *codersdk.Client, opts ...Option) *server.StdioServer { - options := &mcpOptions{ - instructions: ``, - logger: ptr.Ref(slog.Make(sloghuman.Sink(os.Stdout))), - } - for _, opt := range opts { - opt(options) - } - - mcpSrv := server.NewMCPServer( - "Coder Agent", - buildinfo.Version(), - server.WithInstructions(options.instructions), - ) - - logger := slog.Make(sloghuman.Sink(os.Stdout)) - - // Register tools based on the allowed list (if specified) - reg := AllTools() - if len(options.allowedTools) > 0 { - reg = reg.WithOnlyAllowed(options.allowedTools...) - } - reg.Register(mcpSrv, ToolDeps{ - Client: client, - Logger: &logger, - }) - - srv := server.NewStdioServer(mcpSrv) - return srv -} - // allTools is the list of all available tools. When adding a new tool, // make sure to update this list. var allTools = ToolRegistry{ @@ -120,6 +55,8 @@ Choose an emoji that helps the user understand the current phase at a glance.`), mcp.WithBoolean("done", mcp.Description(`Whether the overall task the user requested is complete. Set to true only when the entire requested operation is finished successfully. For multi-step processes, use false until all steps are complete.`), mcp.Required()), + mcp.WithBoolean("need_user_attention", mcp.Description(`Whether the user needs to take action on the task. +Set to true if the task is in a failed state or if the user needs to take action to continue.`), mcp.Required()), ), MakeHandler: handleCoderReportTask, }, @@ -265,8 +202,10 @@ Can be either "start" or "stop".`)), // ToolDeps contains all dependencies needed by tool handlers type ToolDeps struct { - Client *codersdk.Client - Logger *slog.Logger + Client *codersdk.Client + AgentClient *agentsdk.Client + Logger *slog.Logger + AppStatusSlug string } // ToolHandler associates a tool with its handler creation function @@ -313,18 +252,23 @@ func AllTools() ToolRegistry { } type handleCoderReportTaskArgs struct { - Summary string `json:"summary"` - Link string `json:"link"` - Emoji string `json:"emoji"` - Done bool `json:"done"` + Summary string `json:"summary"` + Link string `json:"link"` + Emoji string `json:"emoji"` + Done bool `json:"done"` + NeedUserAttention bool `json:"need_user_attention"` } // Example payload: -// {"jsonrpc":"2.0","id":1,"method":"tools/call", "params": {"name": "coder_report_task", "arguments": {"summary": "I'm working on the login page.", "link": "https://github.com/coder/coder/pull/1234", "emoji": "🔍", "done": false}}} +// {"jsonrpc":"2.0","id":1,"method":"tools/call", "params": {"name": "coder_report_task", "arguments": {"summary": "I need help with the login page.", "link": "https://github.com/coder/coder/pull/1234", "emoji": "🔍", "done": false, "need_user_attention": true}}} func handleCoderReportTask(deps ToolDeps) server.ToolHandlerFunc { return func(ctx context.Context, request mcp.CallToolRequest) (*mcp.CallToolResult, error) { - if deps.Client == nil { - return nil, xerrors.New("developer error: client is required") + if deps.AgentClient == nil { + return nil, xerrors.New("developer error: agent client is required") + } + + if deps.AppStatusSlug == "" { + return nil, xerrors.New("No app status slug provided, set CODER_MCP_APP_STATUS_SLUG when running the MCP server to report tasks.") } // Convert the request parameters to a json.RawMessage so we can unmarshal @@ -334,20 +278,33 @@ func handleCoderReportTask(deps ToolDeps) server.ToolHandlerFunc { return nil, xerrors.Errorf("failed to unmarshal arguments: %w", err) } - // TODO: Waiting on support for tasks. - deps.Logger.Info(ctx, "report task tool called", slog.F("summary", args.Summary), slog.F("link", args.Link), slog.F("done", args.Done), slog.F("emoji", args.Emoji)) - /* - err := sdk.PostTask(ctx, agentsdk.PostTaskRequest{ - Reporter: "claude", - Summary: summary, - URL: link, - Completion: done, - Icon: emoji, - }) - if err != nil { - return nil, err - } - */ + deps.Logger.Info(ctx, "report task tool called", + slog.F("summary", args.Summary), + slog.F("link", args.Link), + slog.F("emoji", args.Emoji), + slog.F("done", args.Done), + slog.F("need_user_attention", args.NeedUserAttention), + ) + + newStatus := agentsdk.PatchAppStatus{ + AppSlug: deps.AppStatusSlug, + Message: args.Summary, + URI: args.Link, + Icon: args.Emoji, + NeedsUserAttention: args.NeedUserAttention, + State: codersdk.WorkspaceAppStatusStateWorking, + } + + if args.Done { + newStatus.State = codersdk.WorkspaceAppStatusStateComplete + } + if args.NeedUserAttention { + newStatus.State = codersdk.WorkspaceAppStatusStateFailure + } + + if err := deps.AgentClient.PatchAppStatus(ctx, newStatus); err != nil { + return nil, xerrors.Errorf("failed to patch app status: %w", err) + } return &mcp.CallToolResult{ Content: []mcp.Content{ diff --git a/mcp/mcp_test.go b/mcp/mcp_test.go index 1144d9265aa15..c5cf000efcfa3 100644 --- a/mcp/mcp_test.go +++ b/mcp/mcp_test.go @@ -17,7 +17,9 @@ import ( "github.com/coder/coder/v2/coderd/database" "github.com/coder/coder/v2/coderd/database/dbfake" "github.com/coder/coder/v2/codersdk" + "github.com/coder/coder/v2/codersdk/agentsdk" codermcp "github.com/coder/coder/v2/mcp" + "github.com/coder/coder/v2/provisionersdk/proto" "github.com/coder/coder/v2/pty/ptytest" "github.com/coder/coder/v2/testutil" ) @@ -39,7 +41,14 @@ func TestCoderTools(t *testing.T) { r := dbfake.WorkspaceBuild(t, store, database.WorkspaceTable{ OrganizationID: owner.OrganizationID, OwnerID: member.ID, - }).WithAgent().Do() + }).WithAgent(func(agents []*proto.Agent) []*proto.Agent { + agents[0].Apps = []*proto.App{ + { + Slug: "some-agent-app", + }, + } + return agents + }).Do() // Note: we want to test the list_workspaces tool before starting the // workspace agent. Starting the workspace agent will modify the workspace @@ -65,9 +74,12 @@ func TestCoderTools(t *testing.T) { // Register tools using our registry logger := slogtest.Make(t, nil) + agentClient := agentsdk.New(memberClient.URL) codermcp.AllTools().Register(mcpSrv, codermcp.ToolDeps{ - Client: memberClient, - Logger: &logger, + Client: memberClient, + Logger: &logger, + AppStatusSlug: "some-agent-app", + AgentClient: agentClient, }) t.Run("coder_list_templates", func(t *testing.T) { @@ -86,24 +98,44 @@ func TestCoderTools(t *testing.T) { }) t.Run("coder_report_task", func(t *testing.T) { + // Given: the MCP server has an agent token. + oldAgentToken := agentClient.SDK.SessionToken() + agentClient.SetSessionToken(r.AgentToken) + t.Cleanup(func() { + agentClient.SDK.SetSessionToken(oldAgentToken) + }) // When: the coder_report_task tool is called ctr := makeJSONRPCRequest(t, "tools/call", "coder_report_task", map[string]any{ "summary": "Test summary", "link": "https://example.com", "emoji": "🔍", "done": false, - "coder_url": client.URL.String(), - "coder_session_token": client.SessionToken(), + "need_user_attention": true, }) pty.WriteLine(ctr) _ = pty.ReadLine(ctx) // skip the echo - // Then: the response is a success message. - // TODO: check the task was created. This functionality is not yet implemented. - expected := makeJSONRPCTextResponse(t, "Thanks for reporting!") + // Then: positive feedback is given to the reporting agent. actual := pty.ReadLine(ctx) - testutil.RequireJSONEq(t, expected, actual) + require.Contains(t, actual, "Thanks for reporting!") + + // Then: the response is a success message. + ws, err := memberClient.Workspace(ctx, r.Workspace.ID) + require.NoError(t, err, "failed to get workspace") + agt, err := memberClient.WorkspaceAgent(ctx, ws.LatestBuild.Resources[0].Agents[0].ID) + require.NoError(t, err, "failed to get workspace agent") + require.NotEmpty(t, agt.Apps, "workspace agent should have an app") + require.NotEmpty(t, agt.Apps[0].Statuses, "workspace agent app should have a status") + st := agt.Apps[0].Statuses[0] + // require.Equal(t, ws.ID, st.WorkspaceID, "workspace app status should have the correct workspace id") + require.Equal(t, agt.ID, st.AgentID, "workspace app status should have the correct agent id") + require.Equal(t, agt.Apps[0].ID, st.AppID, "workspace app status should have the correct app id") + require.Equal(t, codersdk.WorkspaceAppStatusStateFailure, st.State, "workspace app status should be in the failure state") + require.Equal(t, "Test summary", st.Message, "workspace app status should have the correct message") + require.Equal(t, "https://example.com", st.URI, "workspace app status should have the correct uri") + require.Equal(t, "🔍", st.Icon, "workspace app status should have the correct icon") + require.True(t, st.NeedsUserAttention, "workspace app status should need user attention") }) t.Run("coder_whoami", func(t *testing.T) { From fcac4abcca51d27df3fcd7379e6333da16690c51 Mon Sep 17 00:00:00 2001 From: Michael Smith Date: Tue, 1 Apr 2025 10:13:56 -0400 Subject: [PATCH 008/498] fix(site): standardize headers for Admin Settings page (#16911) ## Changes made - Switched almost all headers to use the `SettingHeader` component - Redesigned component to be more composition-based, to stay in line with the patterns we're starting to use more throughout the codebase - Refactored `SettingHeader` to be based on Radix and Tailwind, rather than Emotion/MUI - Added additional props to `SettingHeader` to help resolve issues with the component creating invalid HTML - Beefed up `SettingHeader` to have better out-of-the-box accessibility - Addressed some typographic problems in `SettingHeader` - Addressed some responsive layout problems for `SettingsHeader` - Added first-ever stories for `SettingsHeader` ## Notes - There are still a few headers that aren't using `SettingHeader` yet. There were some UI edge cases that meant I couldn't reliably bring it in without consulting the Design team first. I'm a little less worried about them, because they at least *look* like the other headers, but it'd be nice if we could centralize everything in a followup PR --- .../SettingsHeader/SettingsHeader.stories.tsx | 83 +++++++++ .../SettingsHeader/SettingsHeader.tsx | 161 +++++++++++------- .../AppearanceSettingsPageView.tsx | 16 +- .../ExternalAuthSettingsPageView.tsx | 19 ++- .../AddNewLicensePageView.tsx | 17 +- .../LicensesSettingsPageView.tsx | 16 +- .../NetworkSettingsPageView.tsx | 38 +++-- .../NotificationsPage/NotificationsPage.tsx | 37 ++-- .../CreateOAuth2AppPageView.tsx | 17 +- .../EditOAuth2AppPageView.tsx | 17 +- .../OAuth2AppsSettingsPageView.tsx | 16 +- .../ObservabilitySettingsPageView.tsx | 43 +++-- .../OverviewPage/OverviewPageView.tsx | 19 ++- .../SecuritySettingsPageView.tsx | 49 ++++-- .../UserAuthSettingsPageView.tsx | 45 +++-- .../pages/GroupsPage/CreateGroupPageView.tsx | 16 +- site/src/pages/GroupsPage/GroupPage.tsx | 19 ++- site/src/pages/GroupsPage/GroupsPage.tsx | 18 +- .../pages/HealthPage/WorkspaceProxyPage.tsx | 4 +- .../CreateEditRolePageView.tsx | 19 ++- .../CustomRolesPage/CustomRolesPage.tsx | 16 +- .../OrganizationMembersPageView.tsx | 10 +- .../OrganizationProvisionersPageView.tsx | 10 +- .../OrganizationSettingsPageView.tsx | 10 +- .../WorkspaceProxyPage/WorkspaceProxyPage.tsx | 35 +--- .../WorkspaceProxyPage/WorkspaceProxyView.tsx | 13 ++ site/src/pages/UsersPage/UsersPageView.tsx | 39 +++-- 27 files changed, 556 insertions(+), 246 deletions(-) create mode 100644 site/src/components/SettingsHeader/SettingsHeader.stories.tsx diff --git a/site/src/components/SettingsHeader/SettingsHeader.stories.tsx b/site/src/components/SettingsHeader/SettingsHeader.stories.tsx new file mode 100644 index 0000000000000..75381d419c4dc --- /dev/null +++ b/site/src/components/SettingsHeader/SettingsHeader.stories.tsx @@ -0,0 +1,83 @@ +import type { Meta, StoryObj } from "@storybook/react"; +import { docs } from "utils/docs"; +import { + SettingsHeader, + SettingsHeaderDescription, + SettingsHeaderDocsLink, + SettingsHeaderTitle, +} from "./SettingsHeader"; + +const meta: Meta = { + title: "components/SettingsHeader", + component: SettingsHeader, +}; + +export default meta; +type Story = StoryObj; + +export const PrimaryHeaderOnly: Story = { + args: { + children: This is a header, + }, +}; + +export const PrimaryHeaderWithDescription: Story = { + args: { + children: ( + <> + Another primary header + + This description can be any ReactNode. This provides more options for + composition. + + + ), + }, +}; + +export const PrimaryHeaderWithDescriptionAndDocsLink: Story = { + args: { + children: ( + <> + Another primary header + + This description can be any ReactNode. This provides more options for + composition. + + + ), + actions: , + }, +}; + +export const SecondaryHeaderWithDescription: Story = { + args: { + children: ( + <> + + This is a secondary header. + + + The header's styling is completely independent of its semantics. Both + can be adjusted independently to help avoid invalid HTML. + + + ), + }, +}; + +export const SecondaryHeaderWithDescriptionAndDocsLink: Story = { + args: { + children: ( + <> + + Another secondary header + + + Nothing to add, really. + + + ), + actions: , + }, +}; diff --git a/site/src/components/SettingsHeader/SettingsHeader.tsx b/site/src/components/SettingsHeader/SettingsHeader.tsx index edd06a6957815..b5128bcc28224 100644 --- a/site/src/components/SettingsHeader/SettingsHeader.tsx +++ b/site/src/components/SettingsHeader/SettingsHeader.tsx @@ -1,74 +1,107 @@ -import { useTheme } from "@emotion/react"; +import { type VariantProps, cva } from "class-variance-authority"; import { Button } from "components/Button/Button"; -import { Stack } from "components/Stack/Stack"; import { SquareArrowOutUpRightIcon } from "lucide-react"; -import type { FC, ReactNode } from "react"; +import type { FC, PropsWithChildren, ReactNode } from "react"; +import { cn } from "utils/cn"; -interface HeaderProps { - title: ReactNode; - description?: ReactNode; - secondary?: boolean; - docsHref?: string; - tooltip?: ReactNode; -} - -export const SettingsHeader: FC = ({ - title, - description, - docsHref, - secondary, - tooltip, +type SettingsHeaderProps = Readonly< + PropsWithChildren<{ + actions?: ReactNode; + className?: string; + }> +>; +export const SettingsHeader: FC = ({ + children, + actions, + className, }) => { - const theme = useTheme(); + return ( +
+ {/* + * The text-sm class is only meant to adjust the font size of + * SettingsDescription, but we need to apply it here. That way, + * text-sm combines with the max-w-prose class and makes sure + * we have a predictable max width for the header + description by + * default. + */} +
{children}
+ {actions} +
+ ); +}; +type SettingsHeaderDocsLinkProps = Readonly< + PropsWithChildren<{ href: string }> +>; +export const SettingsHeaderDocsLink: FC = ({ + href, + children = "Read the docs", +}) => { return ( - -
- -

- {title} -

- {tooltip} - + + ); +}; - {description && ( - - {description} - - )} -
+const titleVariants = cva("m-0 pb-1 flex items-center gap-2 leading-tight", { + variants: { + hierarchy: { + primary: "text-3xl font-bold", + secondary: "text-2xl font-medium", + }, + }, + defaultVariants: { + hierarchy: "primary", + }, +}); +type SettingsHeaderTitleProps = Readonly< + PropsWithChildren< + VariantProps & { + level?: `h${1 | 2 | 3 | 4 | 5 | 6}`; + tooltip?: ReactNode; + className?: string; + } + > +>; +export const SettingsHeaderTitle: FC = ({ + children, + tooltip, + className, + level = "h1", + hierarchy = "primary", +}) => { + // Explicitly not using Radix's Slot component, because we don't want to + // allow any arbitrary element to be composed into this. We specifically + // only want to allow the six HTML headers. Anything else will likely result + // in invalid markup + const Title = level; + return ( +
+ + {children} + + {tooltip} +
+ ); +}; - {docsHref && ( - - )} - +type SettingsHeaderDescriptionProps = Readonly< + PropsWithChildren<{ + className?: string; + }> +>; +export const SettingsHeaderDescription: FC = ({ + children, + className, +}) => { + return ( +

+ {children} +

); }; diff --git a/site/src/pages/DeploymentSettingsPage/AppearanceSettingsPage/AppearanceSettingsPageView.tsx b/site/src/pages/DeploymentSettingsPage/AppearanceSettingsPage/AppearanceSettingsPageView.tsx index 6509153694f6d..4f72c67d02fb3 100644 --- a/site/src/pages/DeploymentSettingsPage/AppearanceSettingsPage/AppearanceSettingsPageView.tsx +++ b/site/src/pages/DeploymentSettingsPage/AppearanceSettingsPage/AppearanceSettingsPageView.tsx @@ -8,7 +8,11 @@ import { } from "components/Badges/Badges"; import { Button } from "components/Button/Button"; import { PopoverPaywall } from "components/Paywall/PopoverPaywall"; -import { SettingsHeader } from "components/SettingsHeader/SettingsHeader"; +import { + SettingsHeader, + SettingsHeaderDescription, + SettingsHeaderTitle, +} from "components/SettingsHeader/SettingsHeader"; import { Popover, PopoverContent, @@ -54,10 +58,12 @@ export const AppearanceSettingsPageView: FC< return ( <> - + + Appearance + + Customize the look and feel of your Coder deployment. + + diff --git a/site/src/pages/DeploymentSettingsPage/ExternalAuthSettingsPage/ExternalAuthSettingsPageView.tsx b/site/src/pages/DeploymentSettingsPage/ExternalAuthSettingsPage/ExternalAuthSettingsPageView.tsx index e64986c5788fc..d2a916823f56e 100644 --- a/site/src/pages/DeploymentSettingsPage/ExternalAuthSettingsPage/ExternalAuthSettingsPageView.tsx +++ b/site/src/pages/DeploymentSettingsPage/ExternalAuthSettingsPage/ExternalAuthSettingsPageView.tsx @@ -8,7 +8,12 @@ import TableRow from "@mui/material/TableRow"; import type { DeploymentValues, ExternalAuthConfig } from "api/typesGenerated"; import { Alert } from "components/Alert/Alert"; import { PremiumBadge } from "components/Badges/Badges"; -import { SettingsHeader } from "components/SettingsHeader/SettingsHeader"; +import { + SettingsHeader, + SettingsHeaderDescription, + SettingsHeaderDocsLink, + SettingsHeaderTitle, +} from "components/SettingsHeader/SettingsHeader"; import type { FC } from "react"; import { docs } from "utils/docs"; @@ -22,10 +27,14 @@ export const ExternalAuthSettingsPageView: FC< return ( <> + actions={} + > + External Authentication + + Coder integrates with GitHub, GitLab, BitBucket, Azure Repos, and + OpenID Connect to authenticate developers with external services. + +