diff --git a/AGENTS.md b/AGENTS.md index bc48bcef..c052a2b5 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -228,12 +228,21 @@ pkg/hookdeck/ ## 5. Development Workflow +### Agent command execution (Cursor / AI) + +**Strong preference:** When a command needs more than the default sandbox (network, full filesystem, real module cache, TLS to external services, `gpg-agent`, etc.), **prompt the user for permission to run that command outside the sandbox** (e.g. approve **`required_permissions: ["all"]`**, **network**, or the product’s “run outside sandbox” control)—and **execute it yourself** after approval. + +- **Do not** default to “please run this in your terminal” or long copy-paste-only instructions when the environment can elevate the **same** command. +- **Do** use copy-paste or “run locally” guidance only when elevation is **refused**, **unavailable**, or the user explicitly prefers it. + +This applies to **`go test`**, **`go mod download`**, **`git`**, **`gh`**, builds, and acceptance tests—not only network calls. + ### Building and Testing ```bash # Build the CLI go build -o hookdeck cmd/hookdeck/main.go -# Run tests +# Run all unit tests (default packages; run from repo root) go test ./... # Run specific package tests @@ -243,17 +252,55 @@ go test ./pkg/cmd/ go test -race ./... ``` +### Running `go test` after code changes (required for agents) + +**Whenever you change Go code in this repo, run tests from the repository root** and treat the run as failed if compilation or any test fails. + +**Recommended command in Cursor (module cache + full permissions):** + +```bash +cd /path/to/hookdeck-cli # repo root containing go.mod +GOMODCACHE="$(go env GOPATH)/pkg/mod" go test ./... +``` + +**Prompt the user** to approve running that command **outside the sandbox** with **`required_permissions: ["all"]`** and **`["network"]`** when the module cache may need to fetch (first run or new dependencies). Plain `go test ./...` is fine on a normal developer machine where `GOMODCACHE` is unset or already populated. + +**Cursor / agent environments — prompt for out-of-sandbox, fix module cache, do not skip tests** + +1. **Prompt the user** for permission, then run `go test ./...` with **`required_permissions: ["all"]`** (disable sandbox for that command). Do **not** use the default sandbox for `go test` here. + +2. **If you still see `no required module provides package github.com/...`**, the shell often has **`GOMODCACHE`** set to an **empty** Cursor temp directory (even with `all`). That is **not** a broken `go.mod`. **Re-run tests with an explicit cache path** (pick one): + +```bash +GOMODCACHE="$(go env GOPATH)/pkg/mod" go mod download +GOMODCACHE="$(go env GOPATH)/pkg/mod" go test ./... +``` + +Or unset the variable so Go uses its default (`$GOPATH/pkg/mod`): + +```bash +env -u GOMODCACHE go mod download +env -u GOMODCACHE go test ./... +``` + +Use **`go mod download`** with **`required_permissions: ["network"]` or `["all"]`** the first time the cache is cold (after **prompting** for that permission). + +- **`["network"]` alone is often not enough** for `go test` because compilation still needs a **readable** module cache. +- **Prompt for out-of-sandbox / full permissions**, then re-run with **`["all"]`** and the **`GOMODCACHE`** / **`env -u`** fix above—**do not** skip tests or push the user to run commands manually unless they decline elevation. + +**Acceptance tests** live under `test/acceptance/` with **build tags** (they are **not** included in plain `go test ./...`). See **`test/acceptance/README.md`** for tags, env vars, and CI parity. When you change CLI-facing behavior, **prompt for out-of-sandbox permissions** and run them the same way, with **`required_permissions: ["all"]`** (and network) when using an agent shell. + ### Sandbox and permissions Commands that need network (e.g. `git push`, `gh pr create`, `npm install`) or full system access often fail in the sandbox with **TLS/certificate errors** (e.g. `x509: OSStatus -26276`) or auth prompts. These are sandbox restrictions, not broken tooling. -**Prefer requesting elevated permissions** (e.g. `required_permissions: ["all"]` or `["network"]`) and asking the user to approve so the agent can retry the command. Do not default to prompting the user to run commands themselves when elevation is available. Only fall back to copy-pasteable commands when elevated permissions are not an option. +**Prompt the user for permission to run outside the sandbox** (e.g. **`required_permissions: ["all"]`** or **`["network"]`**) and **re-run the same command** after they approve. See **Agent command execution** above. Only fall back to copy-pasteable “run this locally” instructions when elevation is refused or unavailable. ### Git commits (GPG) **Always use GPG-signed commits** (`git commit -S`, or `commit.gpgsign=true` in git config). **Do not** use `--no-gpg-sign` to bypass signing. -In restricted environments, signing may fail with errors like “No agent running” or “Operation not permitted” on `~/.gnupg`. **Re-run the commit with full permissions** so `gpg-agent` is reachable, or sign from a normal local terminal. Unsigned commits should not be pushed as a shortcut. +In restricted environments, signing may fail with errors like “No agent running” or “Operation not permitted” on `~/.gnupg`. **Prompt for permission to run the commit outside the sandbox** (full permissions) so `gpg-agent` is reachable; only if that still fails, suggest the user sign from a normal local terminal. Unsigned commits should not be pushed as a shortcut. ### Linting and Formatting ```bash @@ -277,12 +324,13 @@ go run cmd/hookdeck/main.go login --help ``` ### Sandbox and command execution -- **Always run tests** when changing code: unit tests (`go test ./pkg/...`) and, for CLI-facing changes, acceptance tests (`go test ./test/acceptance/...`). Do not skip tests to avoid failures. -- When running commands (build, test, acceptance tests), if you see **TLS/certificate errors** (e.g. `x509: certificate verify failed`, `tls: failed to verify certificate`), **permission errors** (e.g. `operation not permitted` when writing to the Go module cache), or similar failures that look environment-related, the command is likely running inside a **sandbox**. **Prompt the user** and **re-run the command with elevated permissions** (e.g. `required_permissions: ["network"]` for tests that need API access, or `["all"]` to disable the sandbox) so the operation can succeed. Do not treat a build or test as passed if stderr shows these errors, even when the process exit code is 0. +- **Always run tests** when changing code: **`go test ./...`** from the repo root (see **Running `go test` after code changes** above), and for CLI-facing changes, acceptance tests with the appropriate **`-tags=...`** (see `test/acceptance/README.md`). Do not skip tests to avoid failures. +- For **`go test`** in this repo, **prompt for out-of-sandbox execution** and use **`required_permissions: ["all"]`** (and **`["network"]`** when needed) so the Go module cache works (see **Running `go test` after code changes**). Same for acceptance tests that call the real API. +- When you see **TLS/certificate errors**, **`no required module provides package`** during `go test`, **permission errors** on the module cache, or similar environment-related failures, **prompt for full/out-of-sandbox permissions** and **re-run** (with **`GOMODCACHE=...`** if needed)—**do not** treat the run as passed, and **do not** default to asking the user to run the command manually unless they declined elevation. ### GitHub CLI (`gh`) - Use the **[GitHub CLI](https://cli.github.com/) (`gh`)** to read GitHub data and perform actions from the shell: **workflow runs and job logs** (e.g. `gh run list`, `gh run view --log-failed`, `gh run view --job --log`), **PRs and checks** (`gh pr view`, `gh pr checks`, `gh pr diff`), **API access** (`gh api`), and creating or updating PRs, issues, and releases. -- Install and authenticate `gh` where needed (e.g. `gh auth login`). If `gh` fails with TLS, network, or permission errors, re-run with **network** or **all** permissions when the agent sandbox may be blocking access. +- Install and authenticate `gh` where needed (e.g. `gh auth login`). If `gh` fails with TLS, network, or permission errors, **prompt for permission** to re-run with **network** or **all** permissions when the agent sandbox may be blocking access. ## 6. Documentation Standards @@ -358,7 +406,7 @@ if apiErr, ok := err.(*hookdeck.APIError); ok { ## 9. Testing Guidelines -- **Always run tests** when changing code. Run unit tests (`go test ./pkg/...`) and, for CLI-facing changes, acceptance tests (`go test ./test/acceptance/...`). If tests fail due to TLS/network/sandbox (e.g. `x509`, `operation not permitted`), prompt the user and re-run with elevated permissions (e.g. `required_permissions: ["all"]`) so tests can pass. +- **Always run tests** when changing code. Run **`go test ./...`** from the repo root (see **§5 Running `go test` after code changes**). For CLI-facing changes, run acceptance tests with the correct **`-tags=...`** per `test/acceptance/README.md`. **Agents:** **prompt for out-of-sandbox / full permissions**, then run `go test` with **`required_permissions: ["all"]`** (and network if needed) so the module cache works—**do not** push the user to run tests manually unless elevation is refused. - **Create tests for new functionality.** Add unit tests for validation and business logic; add acceptance tests for flows that use the CLI as a user or agent would (success and failure paths). Acceptance tests must pass or fail—no skipping to avoid failures. ### Acceptance Test Setup @@ -384,7 +432,8 @@ Acceptance tests in `test/acceptance/` are partitioned by **feature build tags** |---------|---------| | `go run cmd/hookdeck/main.go --help` | View CLI help | | `go build -o hookdeck cmd/hookdeck/main.go` | Build CLI binary | -| `go test ./pkg/cmd/` | Test command implementations | +| `go test ./...` | All unit tests (repo root; agents: run with `required_permissions: ["all"]`) | +| `go test ./pkg/cmd/` | Test command implementations only | | `go generate ./...` | Run code generation (if used) | | `golangci-lint run` | Run comprehensive linting | diff --git a/CLAUDE.md b/CLAUDE.md index 1c4684ad..5f949eb4 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -1,5 +1,3 @@ # Hookdeck CLI — Claude / agent context -Read **[AGENTS.md](AGENTS.md)** first for project structure, testing, and CLI conventions. - -Repo-specific agent workflows (e.g. releases) live under **[skills/](skills/)** — see `skills/hookdeck-cli-release/` for cutting GitHub releases and drafting release notes. +Read **[AGENTS.md](AGENTS.md)** for all agent instructions (project structure, testing, sandbox permissions, CLI conventions, **`skills/`** workflows, and releases). diff --git a/pkg/cmd/gateway.go b/pkg/cmd/gateway.go index 36198904..1269e1c1 100644 --- a/pkg/cmd/gateway.go +++ b/pkg/cmd/gateway.go @@ -13,6 +13,26 @@ type gatewayCmd struct { cmd *cobra.Command } +// isGatewayMCPLeafCommand reports whether cmd is the gateway mcp subcommand. +// MCP uses JSON-RPC on stdout; when there is no API key yet, requireGatewayProject +// must not run so the server can start and expose hookdeck_login. +func isGatewayMCPLeafCommand(cmd *cobra.Command) bool { + return cmd != nil && cmd.Name() == "mcp" && cmd.Parent() != nil && cmd.Parent().Name() == "gateway" +} + +// gatewayPersistentPreRunE runs before gateway subcommands. MCP may start without credentials +// (JSON-RPC on stdout; hookdeck_login supplies auth). Once logged in, gateway MCP uses the +// same Gateway project rules as other gateway commands. +func gatewayPersistentPreRunE(cmd *cobra.Command, args []string) error { + initTelemetry(cmd) + if isGatewayMCPLeafCommand(cmd) { + if err := Config.Profile.ValidateAPIKey(); err != nil { + return nil + } + } + return requireGatewayProject(nil) +} + // requireGatewayProject ensures the current project is a Gateway project (inbound or console). // It runs API key validation, resolves project type from config or API, and returns an error if not Gateway. // cfg is optional; when nil, the global Config is used (for production). @@ -31,14 +51,14 @@ func requireGatewayProject(cfg *config.Config) error { projectType = config.ModeToProjectType(cfg.Profile.ProjectMode) } if projectType == "" { - // Resolve from API + // Resolve team/project/mode/type from API (authoritative for the key). Do not clear + // guest_url here — gateway PreRun may run for users who still have a guest upgrade link. response, err := cfg.GetAPIClient().ValidateAPIKey() if err != nil { return err } - projectType = config.ModeToProjectType(response.ProjectMode) - cfg.Profile.ProjectType = projectType - cfg.Profile.ProjectMode = response.ProjectMode + cfg.Profile.ApplyValidateAPIKeyResponse(response, false) + projectType = cfg.Profile.ProjectType _ = cfg.Profile.SaveProfile() } if !config.IsGatewayProject(projectType) { @@ -69,10 +89,7 @@ The gateway command group provides full access to all Event Gateway resources.`, # Start the MCP server for AI agent access hookdeck gateway mcp`, - PersistentPreRunE: func(cmd *cobra.Command, args []string) error { - initTelemetry(cmd) - return requireGatewayProject(nil) - }, + PersistentPreRunE: gatewayPersistentPreRunE, } // Register resource subcommands (same factory as root backward-compat registration) diff --git a/pkg/cmd/gateway_test.go b/pkg/cmd/gateway_test.go index 9359dc82..56643c00 100644 --- a/pkg/cmd/gateway_test.go +++ b/pkg/cmd/gateway_test.go @@ -1,10 +1,16 @@ package cmd import ( + "encoding/json" + "net/http" + "net/http/httptest" + "os" + "path/filepath" "strings" "testing" "github.com/hookdeck/hookdeck-cli/pkg/config" + "github.com/hookdeck/hookdeck-cli/pkg/hookdeck" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -86,3 +92,69 @@ func TestRequireGatewayProject(t *testing.T) { assert.True(t, strings.Contains(err.Error(), "requires a Gateway project") || strings.Contains(err.Error(), "Outpost")) }) } + +func TestRequireGatewayProject_resolveFromValidate(t *testing.T) { + config.ResetAPIClientForTesting() + t.Cleanup(config.ResetAPIClientForTesting) + + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path != hookdeck.APIPathPrefix+"/cli-auth/validate" { + http.NotFound(w, r) + return + } + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(hookdeck.ValidateAPIKeyResponse{ + ProjectID: "team_from_validate", + ProjectMode: "inbound", + }) + })) + t.Cleanup(server.Close) + + dir := t.TempDir() + path := filepath.Join(dir, "config.toml") + toml := `profile = "default" + +[default] +api_key = "sk_test_123456789012" +project_id = "stale_team_should_be_replaced" +guest_url = "https://guest.example/keep-me" +` + require.NoError(t, os.WriteFile(path, []byte(toml), 0600)) + + cfg, err := config.LoadConfigFromFile(path) + require.NoError(t, err) + cfg.APIBaseURL = server.URL + + err = requireGatewayProject(cfg) + require.NoError(t, err) + require.Equal(t, "team_from_validate", cfg.Profile.ProjectId) + require.Equal(t, config.ProjectTypeGateway, cfg.Profile.ProjectType) + require.Equal(t, "inbound", cfg.Profile.ProjectMode) + require.Equal(t, "https://guest.example/keep-me", cfg.Profile.GuestURL, "gateway validate path must not clear guest_url") +} + +func TestGatewayPersistentPreRunE_MCP(t *testing.T) { + old := Config + t.Cleanup(func() { Config = old }) + + gw := newGatewayCmd().cmd + mcpLeaf, _, err := gw.Find([]string{"mcp"}) + require.NoError(t, err) + require.True(t, isGatewayMCPLeafCommand(mcpLeaf)) + + t.Run("no API key skips requireGatewayProject", func(t *testing.T) { + Config = config.Config{} + Config.Profile.ProjectId = "" + err := gatewayPersistentPreRunE(mcpLeaf, nil) + assert.NoError(t, err) + }) + + t.Run("with API key and no project fails", func(t *testing.T) { + Config = config.Config{} + Config.Profile.APIKey = "sk_test_123456789012" + Config.Profile.ProjectId = "" + err := gatewayPersistentPreRunE(mcpLeaf, nil) + require.Error(t, err) + assert.Contains(t, err.Error(), "no project selected") + }) +} diff --git a/pkg/cmd/mcp.go b/pkg/cmd/mcp.go index 679ed84f..7e9d12d5 100644 --- a/pkg/cmd/mcp.go +++ b/pkg/cmd/mcp.go @@ -25,8 +25,13 @@ destinations, events, requests, and more — as MCP tools that AI agents and LLM-based clients can invoke. If the CLI is already authenticated, all tools are available immediately. -If not, a hookdeck_login tool is provided that initiates browser-based -authentication so the user can log in without leaving the MCP session.`), +If not, gateway MCP still starts: project selection is skipped until you +authenticate, and hookdeck_login initiates browser-based sign-in. Protocol +traffic uses stdout only (JSON-RPC); status and errors from the CLI before +the server runs go to stderr. + +hookdeck_login stays registered after sign-in so you can call it with reauth: true +to replace credentials (e.g. when project listing fails with a narrow API key).`), Example: ` # Start the MCP server (stdio transport) hookdeck gateway mcp diff --git a/pkg/cmd/root.go b/pkg/cmd/root.go index fe420675..a15ad2e2 100644 --- a/pkg/cmd/root.go +++ b/pkg/cmd/root.go @@ -50,6 +50,7 @@ func initTelemetry(cmd *cobra.Command) { tel.SetSource("cli") tel.SetEnvironment(hookdeck.DetectEnvironment()) tel.SetCommandContext(cmd) + tel.SetCommandFlagsFromCobra(cmd) tel.SetDeviceName(Config.DeviceName) if tel.InvocationID == "" { tel.SetInvocationID(hookdeck.NewInvocationID()) @@ -72,17 +73,24 @@ func addConnectionCmdTo(parent *cobra.Command) { // Execute adds all child commands to the root command and sets flags appropriately. // This is called by main.main(). It only needs to happen once to the rootCmd. func Execute() { + gatewayMCP := argvContainsGatewayMCP(os.Args) if err := rootCmd.Execute(); err != nil { errString := err.Error() isLoginRequiredError := errString == validators.ErrAPIKeyNotConfigured.Error() || errString == validators.ErrDeviceNameNotConfigured.Error() switch { case isLoginRequiredError: - // capitalize first letter of error because linter errRunes := []rune(errString) errRunes[0] = unicode.ToUpper(errRunes[0]) + capitalized := string(errRunes) - fmt.Printf("%s. Running `hookdeck login`...\n", string(errRunes)) + if gatewayMCP { + // MCP uses JSON-RPC on stdout; do not run interactive login or print recovery text there. + fmt.Fprintf(os.Stderr, "%s. Use hookdeck_login in the MCP session (or run `hookdeck login` in a terminal).\n", capitalized) + os.Exit(1) + } + + fmt.Printf("%s. Running `hookdeck login`...\n", capitalized) loginCommand, _, err := rootCmd.Find([]string{"login"}) if err != nil { @@ -103,18 +111,101 @@ func Execute() { suggStr = fmt.Sprintf(" Did you mean \"%s\"?\nIf not, s", suggestions[0]) } - fmt.Println(fmt.Sprintf("Unknown command \"%s\" for \"%s\".%s"+ + msg := fmt.Sprintf("Unknown command \"%s\" for \"%s\".%s"+ "ee \"hookdeck --help\" for a list of available commands.", - os.Args[1], rootCmd.CommandPath(), suggStr)) + os.Args[1], rootCmd.CommandPath(), suggStr) + if gatewayMCP { + fmt.Fprintln(os.Stderr, msg) + } else { + fmt.Println(msg) + } default: - fmt.Println(err) + if gatewayMCP { + fmt.Fprintln(os.Stderr, err) + } else { + fmt.Println(err) + } } os.Exit(1) } } +// argvContainsGatewayMCP reports whether argv invokes `hookdeck gateway mcp`, ignoring +// global flags and flag values (e.g. --profile name, -p name) so detection stays accurate. +func argvContainsGatewayMCP(argv []string) bool { + if len(argv) < 3 { + return false + } + pos := globalPositionalArgs(argv[1:]) + for i := 0; i < len(pos)-1; i++ { + if pos[i] == "gateway" && pos[i+1] == "mcp" { + return true + } + } + return false +} + +// flagNeedsNextArg lists global flags that consume the next argv token as their value. +// Keep in sync with the PersistentFlags registered in init() below. +var flagNeedsNextArg = map[string]bool{ + "profile": true, + "p": true, + "cli-key": true, + "api-key": true, + "hookdeck-config": true, + "device-name": true, + "log-level": true, + "color": true, + "api-base": true, + "dashboard-base": true, + "console-base": true, + "ws-base": true, +} + +// globalPositionalArgs returns argv arguments that are not global flags or flag values, +// stopping at `--` (which ends flag parsing; remaining tokens are positional). +func globalPositionalArgs(args []string) []string { + var out []string + i := 0 + for i < len(args) { + a := args[i] + if a == "--" { + return append(out, args[i+1:]...) + } + if !strings.HasPrefix(a, "-") { + return append(out, args[i:]...) + } + if strings.HasPrefix(a, "--") { + body := strings.TrimPrefix(a, "--") + name := body + hasEq := false + if j := strings.IndexByte(body, '='); j >= 0 { + name = body[:j] + hasEq = true + } + i++ + if flagNeedsNextArg[name] && !hasEq { + if i < len(args) && !strings.HasPrefix(args[i], "-") { + i++ + } + } + continue + } + // Short flags: support -p only; other shorts consume one token. + if a == "-p" { + i++ + if i < len(args) && !strings.HasPrefix(args[i], "-") { + i++ + } + continue + } + i++ + } + return out +} + func init() { cobra.OnInitialize(Config.InitConfig) diff --git a/pkg/cmd/root_argv_test.go b/pkg/cmd/root_argv_test.go new file mode 100644 index 00000000..57524b87 --- /dev/null +++ b/pkg/cmd/root_argv_test.go @@ -0,0 +1,34 @@ +package cmd + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestArgvContainsGatewayMCP(t *testing.T) { + tests := []struct { + name string + argv []string + want bool + }{ + {"minimal", []string{"hookdeck", "gateway", "mcp"}, true}, + {"with profile long", []string{"hookdeck", "--profile", "p1", "gateway", "mcp"}, true}, + {"profile equals", []string{"hookdeck", "--profile=p1", "gateway", "mcp"}, true}, + {"short p", []string{"hookdeck", "-p", "p1", "gateway", "mcp"}, true}, + {"double dash positional", []string{"hookdeck", "--", "gateway", "mcp"}, true}, + {"not mcp", []string{"hookdeck", "gateway", "connection", "list"}, false}, + {"wrong order", []string{"hookdeck", "mcp", "gateway"}, false}, + {"too short", []string{"hookdeck", "gateway"}, false}, + {"api key flag before", []string{"hookdeck", "--api-key", "k", "gateway", "mcp"}, true}, + // Boolean flags (--insecure, --version) are not in flagNeedsNextArg, so + // globalPositionalArgs treats them as single-token flags and skips them. + {"bool flag before gateway", []string{"hookdeck", "--insecure", "gateway", "mcp"}, true}, + {"bool flag between gateway and mcp", []string{"hookdeck", "gateway", "--insecure", "mcp"}, false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, argvContainsGatewayMCP(tt.argv)) + }) + } +} diff --git a/pkg/config/clear_active_profile_credentials_test.go b/pkg/config/clear_active_profile_credentials_test.go new file mode 100644 index 00000000..7138905c --- /dev/null +++ b/pkg/config/clear_active_profile_credentials_test.go @@ -0,0 +1,19 @@ +package config + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestClearActiveProfileCredentials_MemoryOnly(t *testing.T) { + c := &Config{} + c.Profile.APIKey = "sk_test_123456789012" + c.Profile.ProjectId = "proj_1" + c.Profile.ProjectMode = "inbound" + c.Profile.ProjectType = ProjectTypeGateway + require.NoError(t, c.ClearActiveProfileCredentials()) + assert.Empty(t, c.Profile.APIKey) + assert.Empty(t, c.Profile.ProjectId) +} diff --git a/pkg/config/config.go b/pkg/config/config.go index 6f267de6..f57d26aa 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -293,7 +293,13 @@ func (c *Config) RemoveAllProfiles() error { runtimeViper.SetConfigType("toml") runtimeViper.SetConfigFile(c.viper.ConfigFileUsed()) c.viper = runtimeViper - return c.writeConfig() + if err := c.writeConfig(); err != nil { + return err + } + // Match single-profile logout: clear credential fields in memory so a long-lived + // process does not keep using stale keys after the file was wiped. + zeroProfileCredentialFields(&c.Profile) + return nil } func (c *Config) writeConfig() error { @@ -359,6 +365,57 @@ func (c *Config) SetTelemetryDisabled(disabled bool) error { return c.writeConfig() } +// ClearActiveProfileCredentials removes stored credentials for the active profile without +// printing. +// +// Two paths: +// - Persisted config (viper set): removes the active profile section from the config file +// (same as RemoveProfile) and clears api_key / project_* / guest_url on the in-memory +// Profile. This is what the real CLI and MCP server use after InitConfig. +// - No viper (e.g. some tests): only clears those Profile fields in memory; nothing is +// written to disk. +// +// MCP reauth uses the persisted path in production. The shared *hookdeck.Client is also +// cleared separately in the MCP handler so API calls stop using the old key immediately. +func (c *Config) ClearActiveProfileCredentials() error { + if c == nil || c.Profile.APIKey == "" { + return nil + } + if c.viper == nil { + zeroProfileCredentialFields(&c.Profile) + return nil + } + if err := c.Profile.RemoveProfile(); err != nil { + return err + } + zeroProfileCredentialFields(&c.Profile) + return nil +} + +func zeroProfileCredentialFields(p *Profile) { + p.APIKey = "" + p.ProjectId = "" + p.ProjectMode = "" + p.ProjectType = "" + p.GuestURL = "" +} + +// SaveActiveProfileAfterLogin persists cfg.Profile credential fields and the active profile +// name when viper is configured. It is a no-op when c is nil or viper is nil (e.g. MCP unit +// tests with a minimal Config). The shared hookdeck client is updated separately by the caller. +func (c *Config) SaveActiveProfileAfterLogin() { + if c == nil || c.viper == nil { + return + } + c.Profile.Config = c + if err := c.Profile.SaveProfile(); err != nil { + log.WithError(err).Error("Login succeeded but failed to save profile") + } + if err := c.Profile.UseProfile(); err != nil { + log.WithError(err).Error("Login succeeded but failed to activate profile") + } +} + // getConfigPath returns the path for the config file. // Precedence: // - path (if path is provided, e.g. from --hookdeck-config flag) diff --git a/pkg/config/load_config_file.go b/pkg/config/load_config_file.go new file mode 100644 index 00000000..4c826a46 --- /dev/null +++ b/pkg/config/load_config_file.go @@ -0,0 +1,21 @@ +package config + +import ( + "github.com/spf13/viper" +) + +// LoadConfigFromFile loads an existing Hookdeck CLI TOML config into Config with viper and +// filesystem helpers wired so SaveProfile / writeConfig work. Intended for integration tests; +// production startup should use InitConfig. +func LoadConfigFromFile(configPath string) (*Config, error) { + v := viper.New() + v.SetConfigFile(configPath) + v.SetConfigType("toml") + if err := v.ReadInConfig(); err != nil { + return nil, err + } + c := &Config{viper: v, fs: newConfigFS()} + c.Profile.Config = c + c.constructConfig() + return c, nil +} diff --git a/pkg/config/load_config_file_test.go b/pkg/config/load_config_file_test.go new file mode 100644 index 00000000..25a6f184 --- /dev/null +++ b/pkg/config/load_config_file_test.go @@ -0,0 +1,31 @@ +package config + +import ( + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/require" +) + +func TestLoadConfigFromFile(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "config.toml") + content := `profile = "default" + +[default] +api_key = "sk_test_123456789012" +project_id = "proj_a" +project_mode = "inbound" +` + require.NoError(t, os.WriteFile(path, []byte(content), 0600)) + + c, err := LoadConfigFromFile(path) + require.NoError(t, err) + require.NotNil(t, c.viper) + require.Equal(t, "default", c.Profile.Name) + require.Equal(t, "sk_test_123456789012", c.Profile.APIKey) + require.Equal(t, "proj_a", c.Profile.ProjectId) + require.Equal(t, "inbound", c.Profile.ProjectMode) + require.Equal(t, ProjectTypeGateway, c.Profile.ProjectType) +} diff --git a/pkg/config/profile_credentials.go b/pkg/config/profile_credentials.go new file mode 100644 index 00000000..fc12d300 --- /dev/null +++ b/pkg/config/profile_credentials.go @@ -0,0 +1,40 @@ +package config + +import "github.com/hookdeck/hookdeck-cli/pkg/hookdeck" + +// ApplyValidateAPIKeyResponse updates project fields from GET /cli-auth/validate. +// When clearGuestURL is true, GuestURL is cleared (e.g. hookdeck login re-verify). +// When false, GuestURL is left unchanged (e.g. gateway PreRun resolving type only). +func (p *Profile) ApplyValidateAPIKeyResponse(resp *hookdeck.ValidateAPIKeyResponse, clearGuestURL bool) { + if resp == nil { + return + } + p.ProjectId = resp.ProjectID + p.ProjectMode = resp.ProjectMode + p.ProjectType = ModeToProjectType(resp.ProjectMode) + if clearGuestURL { + p.GuestURL = "" + } +} + +// ApplyPollAPIKeyResponse applies credentials from a completed CLI auth poll (browser or interactive login). +// guestURL is the guest upgrade URL when applicable; use "" for a normal account login. +func (p *Profile) ApplyPollAPIKeyResponse(resp *hookdeck.PollAPIKeyResponse, guestURL string) { + if resp == nil { + return + } + p.APIKey = resp.APIKey + p.ProjectId = resp.ProjectID + p.ProjectMode = resp.ProjectMode + p.ProjectType = ModeToProjectType(resp.ProjectMode) + p.GuestURL = guestURL +} + +// ApplyCIClient applies credentials from hookdeck login --ci. +func (p *Profile) ApplyCIClient(ci hookdeck.CIClient) { + p.APIKey = ci.APIKey + p.ProjectId = ci.ProjectID + p.ProjectMode = ci.ProjectMode + p.ProjectType = ModeToProjectType(ci.ProjectMode) + p.GuestURL = "" +} diff --git a/pkg/config/profile_credentials_test.go b/pkg/config/profile_credentials_test.go new file mode 100644 index 00000000..b55e9510 --- /dev/null +++ b/pkg/config/profile_credentials_test.go @@ -0,0 +1,85 @@ +package config + +import ( + "testing" + + "github.com/hookdeck/hookdeck-cli/pkg/hookdeck" + "github.com/stretchr/testify/require" +) + +func TestProfile_ApplyValidateAPIKeyResponse(t *testing.T) { + t.Run("nil response is no-op", func(t *testing.T) { + p := &Profile{ProjectId: "keep", GuestURL: "https://guest"} + p.ApplyValidateAPIKeyResponse(nil, true) + require.Equal(t, "keep", p.ProjectId) + require.Equal(t, "https://guest", p.GuestURL) + }) + + t.Run("sets project fields and clears guest when requested", func(t *testing.T) { + p := &Profile{GuestURL: "https://guest"} + p.ApplyValidateAPIKeyResponse(&hookdeck.ValidateAPIKeyResponse{ + ProjectID: "team_1", + ProjectMode: "inbound", + }, true) + require.Equal(t, "team_1", p.ProjectId) + require.Equal(t, "inbound", p.ProjectMode) + require.Equal(t, ProjectTypeGateway, p.ProjectType) + require.Empty(t, p.GuestURL) + }) + + t.Run("preserves guest URL when clearGuestURL is false", func(t *testing.T) { + p := &Profile{GuestURL: "https://guest.example/x"} + p.ApplyValidateAPIKeyResponse(&hookdeck.ValidateAPIKeyResponse{ + ProjectID: "team_2", + ProjectMode: "console", + }, false) + require.Equal(t, "team_2", p.ProjectId) + require.Equal(t, ProjectTypeConsole, p.ProjectType) + require.Equal(t, "https://guest.example/x", p.GuestURL) + }) +} + +func TestProfile_ApplyPollAPIKeyResponse(t *testing.T) { + t.Run("nil response is no-op", func(t *testing.T) { + p := &Profile{APIKey: "k", ProjectId: "p"} + p.ApplyPollAPIKeyResponse(nil, "") + require.Equal(t, "k", p.APIKey) + require.Equal(t, "p", p.ProjectId) + }) + + t.Run("sets credentials and guest URL", func(t *testing.T) { + p := &Profile{} + p.ApplyPollAPIKeyResponse(&hookdeck.PollAPIKeyResponse{ + APIKey: "key_from_poll", + ProjectID: "team_p", + ProjectMode: "inbound", + }, "https://guest") + require.Equal(t, "key_from_poll", p.APIKey) + require.Equal(t, "team_p", p.ProjectId) + require.Equal(t, ProjectTypeGateway, p.ProjectType) + require.Equal(t, "https://guest", p.GuestURL) + }) + + t.Run("clears-style guest with empty string", func(t *testing.T) { + p := &Profile{GuestURL: "old"} + p.ApplyPollAPIKeyResponse(&hookdeck.PollAPIKeyResponse{ + APIKey: "k123456789012", + ProjectID: "t", + ProjectMode: "inbound", + }, "") + require.Empty(t, p.GuestURL) + }) +} + +func TestProfile_ApplyCIClient(t *testing.T) { + p := &Profile{} + p.ApplyCIClient(hookdeck.CIClient{ + APIKey: "ci_key_123456", + ProjectID: "team_ci", + ProjectMode: "inbound", + }) + require.Equal(t, "ci_key_123456", p.APIKey) + require.Equal(t, "team_ci", p.ProjectId) + require.Equal(t, ProjectTypeGateway, p.ProjectType) + require.Empty(t, p.GuestURL) +} diff --git a/pkg/gateway/mcp/server.go b/pkg/gateway/mcp/server.go index e1ba7d88..99a99914 100644 --- a/pkg/gateway/mcp/server.go +++ b/pkg/gateway/mcp/server.go @@ -18,6 +18,12 @@ type Server struct { client *hookdeck.Client cfg *config.Config mcpServer *mcpsdk.Server + + // sessionCtx is the context passed to RunStdio. It is cancelled when the + // MCP transport closes (stdin EOF). Background goroutines (e.g. login + // polling) should select on this — NOT on the per-request ctx passed to + // tool handlers, which is cancelled when the handler returns. + sessionCtx context.Context } // NewServer creates an MCP server with all Hookdeck tools registered. @@ -25,9 +31,8 @@ type Server struct { // ProjectID (e.g. via the projects.use action) affects subsequent calls // within the same session. // -// When the client has no API key (unauthenticated), the server additionally -// registers a hookdeck_login tool that initiates browser-based device auth. -// Resource tool handlers will return an auth error until login completes. +// hookdeck_login is always registered: it signs in when unauthenticated, or +// with reauth: true clears stored credentials and starts a fresh browser login. func NewServer(client *hookdeck.Client, cfg *config.Config) *Server { s := &Server{client: client, cfg: cfg} @@ -44,23 +49,21 @@ func NewServer(client *hookdeck.Client, cfg *config.Config) *Server { } // registerTools adds all tool definitions to the MCP server. -// If the client is not yet authenticated, the hookdeck_login tool is also -// registered so that AI agents can initiate authentication in-band. func (s *Server) registerTools() { for _, td := range toolDefs(s.client) { s.mcpServer.AddTool(td.tool, s.wrapWithTelemetry(td.tool.Name, td.handler)) } - if s.client.APIKey == "" { - s.mcpServer.AddTool( - &mcpsdk.Tool{ - Name: "hookdeck_login", - Description: "Authenticate the Hookdeck CLI. Returns a URL that the user must open in their browser to complete login. The tool will wait for the user to complete authentication before returning.", - InputSchema: json.RawMessage(`{"type":"object","properties":{},"additionalProperties":false}`), - }, - s.wrapWithTelemetry("hookdeck_login", handleLogin(s.client, s.cfg, s.mcpServer)), - ) - } + s.mcpServer.AddTool( + &mcpsdk.Tool{ + Name: "hookdeck_login", + Description: "Authenticate the Hookdeck CLI or sign in again. Without arguments, returns a URL for browser login when not yet authenticated, or confirms if already signed in. Set reauth: true to clear the current session and start a new browser login (use when hookdeck_projects list fails and the stored key may be a single-project or dashboard API key).", + InputSchema: schema(map[string]prop{ + "reauth": {Type: "boolean", Desc: "If true, clear stored credentials and start a new browser login. Use when project listing fails — complete login in the browser, then retry hookdeck_projects."}, + }), + }, + s.wrapWithTelemetry("hookdeck_login", handleLogin(s)), + ) } // mcpClientInfo extracts the MCP client name/version string from the @@ -129,5 +132,13 @@ func extractAction(req *mcpsdk.CallToolRequest) string { // RunStdio starts the MCP server on stdin/stdout and blocks until the // connection is closed (i.e. stdin reaches EOF). func (s *Server) RunStdio(ctx context.Context) error { - return s.mcpServer.Run(ctx, &mcpsdk.StdioTransport{}) + return s.Run(ctx, &mcpsdk.StdioTransport{}) +} + +// Run starts the MCP server on the given transport. It stores ctx as the +// session-level context so background goroutines (e.g. login polling) can +// detect when the session ends. +func (s *Server) Run(ctx context.Context, transport mcpsdk.Transport) error { + s.sessionCtx = ctx + return s.mcpServer.Run(ctx, transport) } diff --git a/pkg/gateway/mcp/server_test.go b/pkg/gateway/mcp/server_test.go index a2023c48..80fcc344 100644 --- a/pkg/gateway/mcp/server_test.go +++ b/pkg/gateway/mcp/server_test.go @@ -9,6 +9,7 @@ import ( "net/url" "strings" "testing" + "time" mcpsdk "github.com/modelcontextprotocol/go-sdk/mcp" "github.com/stretchr/testify/assert" @@ -45,7 +46,7 @@ func connectInMemory(t *testing.T, client *hookdeck.Client) *mcpsdk.ClientSessio ctx, cancel := context.WithCancel(context.Background()) t.Cleanup(cancel) go func() { - _ = srv.mcpServer.Run(ctx, serverTransport) + _ = srv.Run(ctx, serverTransport) }() mcpClient := mcpsdk.NewClient(&mcpsdk.Implementation{ @@ -130,7 +131,7 @@ func TestListTools_Authenticated(t *testing.T) { toolNames[i] = tool.Name } - assert.NotContains(t, toolNames, "hookdeck_login") + assert.Contains(t, toolNames, "hookdeck_login") expectedTools := []string{ "hookdeck_projects", "hookdeck_connections", "hookdeck_sources", @@ -895,6 +896,21 @@ func TestProjectsList_Success(t *testing.T) { assert.Contains(t, text, `"active_project_id"`) } +func TestProjectsList_ForbiddenIncludesReauthHint(t *testing.T) { + session := mockAPIWithClient(t, map[string]http.HandlerFunc{ + "/2025-07-01/teams": func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusForbidden) + json.NewEncoder(w).Encode(map[string]any{"message": "not allowed"}) + }, + }) + + result := callTool(t, session, "hookdeck_projects", map[string]any{"action": "list"}) + assert.True(t, result.IsError) + text := textContent(t, result) + assert.Contains(t, strings.ToLower(text), "reauth") + assert.Contains(t, text, "hookdeck_login") +} + func TestProjectsUse_Success(t *testing.T) { session := mockAPIWithClient(t, map[string]http.HandlerFunc{ "/2025-07-01/teams": func(w http.ResponseWriter, r *http.Request) { @@ -1100,34 +1116,46 @@ func TestMetricsTool_UnknownAction(t *testing.T) { // --------------------------------------------------------------------------- func TestLoginTool_AlreadyAuthenticated(t *testing.T) { - client := newTestClient("https://api.hookdeck.com", "test-key") // already has API key - // Login tool is only registered for unauthenticated, so we test via unauthenticated - // then manually set key to simulate already-authenticated scenario. - // Actually, login tool is only present when unauthenticated, so we need to - // create an unauthenticated server first, then set the key before calling. - unauthClient := newTestClient("https://api.hookdeck.com", "") - cfg := &config.Config{} - srv := NewServer(unauthClient, cfg) + client := newTestClient("https://api.hookdeck.com", "test-key") + session := connectInMemory(t, client) + + result := callTool(t, session, "hookdeck_login", map[string]any{}) + assert.False(t, result.IsError) + assert.Contains(t, textContent(t, result), "Already authenticated") +} + +func TestLoginTool_ReauthStartsFreshLogin(t *testing.T) { + api := mockAPI(t, map[string]http.HandlerFunc{ + "/2025-07-01/cli-auth": func(w http.ResponseWriter, r *http.Request) { + json.NewEncoder(w).Encode(map[string]any{ + "browser_url": "https://hookdeck.com/auth?code=reauth", + "poll_url": "http://" + r.Host + "/2025-07-01/cli-auth/poll?key=reauth", + }) + }, + "/2025-07-01/cli-auth/poll": func(w http.ResponseWriter, r *http.Request) { + json.NewEncoder(w).Encode(map[string]any{"claimed": false}) + }, + }) + + client := newTestClient(api.URL, "sk_test_123456789012") + cfg := &config.Config{APIBaseURL: api.URL} + srv := NewServer(client, cfg) serverTransport, clientTransport := mcpsdk.NewInMemoryTransports() ctx, cancel := context.WithCancel(context.Background()) t.Cleanup(cancel) - go func() { - _ = srv.mcpServer.Run(ctx, serverTransport) - }() + go func() { _ = srv.Run(ctx, serverTransport) }() mcpClient := mcpsdk.NewClient(&mcpsdk.Implementation{Name: "test", Version: "0.0.1"}, nil) session, err := mcpClient.Connect(ctx, clientTransport, nil) require.NoError(t, err) t.Cleanup(func() { _ = session.Close() }) - // Now set the API key before calling login — simulates already-auth scenario. - unauthClient.APIKey = "test-key" - - result := callTool(t, session, "hookdeck_login", map[string]any{}) + result := callTool(t, session, "hookdeck_login", map[string]any{"reauth": true}) assert.False(t, result.IsError) - assert.Contains(t, textContent(t, result), "Already authenticated") - _ = client // suppress unused warning + text := textContent(t, result) + assert.Contains(t, text, "https://hookdeck.com/auth?code=reauth") + assert.Empty(t, client.APIKey) } func TestLoginTool_ReturnsURLImmediately(t *testing.T) { @@ -1155,7 +1183,7 @@ func TestLoginTool_ReturnsURLImmediately(t *testing.T) { serverTransport, clientTransport := mcpsdk.NewInMemoryTransports() ctx, cancel := context.WithCancel(context.Background()) t.Cleanup(cancel) - go func() { _ = srv.mcpServer.Run(ctx, serverTransport) }() + go func() { _ = srv.Run(ctx, serverTransport) }() mcpClient := mcpsdk.NewClient(&mcpsdk.Implementation{Name: "test", Version: "0.0.1"}, nil) session, err := mcpClient.Connect(ctx, clientTransport, nil) @@ -1191,7 +1219,7 @@ func TestLoginTool_InProgressShowsURL(t *testing.T) { serverTransport, clientTransport := mcpsdk.NewInMemoryTransports() ctx, cancel := context.WithCancel(context.Background()) t.Cleanup(cancel) - go func() { _ = srv.mcpServer.Run(ctx, serverTransport) }() + go func() { _ = srv.Run(ctx, serverTransport) }() mcpClient := mcpsdk.NewClient(&mcpsdk.Implementation{Name: "test", Version: "0.0.1"}, nil) session, err := mcpClient.Connect(ctx, clientTransport, nil) @@ -1209,6 +1237,70 @@ func TestLoginTool_InProgressShowsURL(t *testing.T) { assert.Contains(t, text, "https://hookdeck.com/auth?code=xyz") } +func TestLoginTool_PollSurvivesAcrossToolCalls(t *testing.T) { + // Regression: the login polling goroutine must use the session-level + // context, not the per-request ctx (which is cancelled when the handler + // returns). If the goroutine selected on per-request ctx, it would be + // cancelled immediately and the second hookdeck_login call would see a + // "login cancelled" error instead of "Already authenticated". + pollCount := 0 + api := mockAPI(t, map[string]http.HandlerFunc{ + "/2025-07-01/cli-auth": func(w http.ResponseWriter, r *http.Request) { + json.NewEncoder(w).Encode(map[string]any{ + "browser_url": "https://hookdeck.com/auth?code=survive", + "poll_url": "http://" + r.Host + "/2025-07-01/cli-auth/poll?key=survive", + }) + }, + "/2025-07-01/cli-auth/poll": func(w http.ResponseWriter, r *http.Request) { + pollCount++ + if pollCount >= 2 { + // Simulate user completing browser auth on 2nd poll. + json.NewEncoder(w).Encode(map[string]any{ + "claimed": true, + "key": "sk_test_survive12345", + "team_id": "proj_survive", + "team_name": "Survive Project", + "team_mode": "console", + "user_name": "test-user", + "organization_name": "test-org", + }) + return + } + json.NewEncoder(w).Encode(map[string]any{"claimed": false}) + }, + }) + + unauthClient := newTestClient(api.URL, "") + cfg := &config.Config{APIBaseURL: api.URL} + srv := NewServer(unauthClient, cfg) + + serverTransport, clientTransport := mcpsdk.NewInMemoryTransports() + ctx, cancel := context.WithCancel(context.Background()) + t.Cleanup(cancel) + go func() { _ = srv.Run(ctx, serverTransport) }() + + mcpClient := mcpsdk.NewClient(&mcpsdk.Implementation{Name: "test", Version: "0.0.1"}, nil) + session, err := mcpClient.Connect(ctx, clientTransport, nil) + require.NoError(t, err) + t.Cleanup(func() { _ = session.Close() }) + + // First call initiates the flow — handler returns immediately. + result := callTool(t, session, "hookdeck_login", map[string]any{}) + assert.False(t, result.IsError) + assert.Contains(t, textContent(t, result), "https://hookdeck.com/auth?code=survive") + + // Wait for the background poll loop: first unclaimed response is followed by + // loginPollInterval sleep inside pollForAPIKey before the second poll succeeds. + time.Sleep(loginPollInterval + 300*time.Millisecond) + + // Second call — if the goroutine survived, the client is now authenticated. + result2 := callTool(t, session, "hookdeck_login", map[string]any{}) + assert.False(t, result2.IsError) + text := textContent(t, result2) + assert.Contains(t, text, "Already authenticated") + assert.Equal(t, "sk_test_survive12345", unauthClient.APIKey) +} + // --------------------------------------------------------------------------- // API error scenarios (shared across tools) // --------------------------------------------------------------------------- diff --git a/pkg/gateway/mcp/tool_help.go b/pkg/gateway/mcp/tool_help.go index 485f39f5..87271c15 100644 --- a/pkg/gateway/mcp/tool_help.go +++ b/pkg/gateway/mcp/tool_help.go @@ -77,6 +77,7 @@ All tools operate on the active project. Call hookdeck_projects first when the u references a project by name, or when unsure which project is active. hookdeck_projects — List or switch projects (actions: list, use) +hookdeck_login — Sign in or reauth: true for a fresh browser session when listing projects fails hookdeck_connections — Inspect connections and control delivery flow (actions: list, get, pause, unpause) hookdeck_sources — Inspect inbound sources (actions: list, get) hookdeck_destinations — Inspect delivery destinations: HTTP, CLI, MOCK (actions: list, get) @@ -107,10 +108,20 @@ Actions: list — List all projects. data.projects is the array (id, org, project, type gateway/outpost/console, current). meta includes active_project_id, active_project_name (short), and active_project_org when known. Outbound projects are excluded. use — Switch the active project for this session (in-memory only). +If list or use fails with 401/403 (or similar), the error may mention hookdeck_login with reauth: true — the stored key may be a narrow dashboard API key. + Parameters: action (string, required) — "list" or "use" project_id (string) — Required for "use" action`, + "hookdeck_login": `hookdeck_login — Browser sign-in for the Hookdeck CLI inside MCP + +Without arguments when already authenticated: confirms the session is active. +When not authenticated: returns a URL the user opens in a browser; poll by calling this tool again. + +Parameters: + reauth (boolean, optional) — If true, clears stored credentials and starts a new browser login. Use when hookdeck_projects list fails and the key may be a single-project or dashboard API key that cannot list teams.`, + "hookdeck_connections": `hookdeck_connections — Inspect connections and control delivery flow Results are scoped to the active project — call hookdeck_projects first if the user has specified a project. diff --git a/pkg/gateway/mcp/tool_login.go b/pkg/gateway/mcp/tool_login.go index 502e34c8..99415d2a 100644 --- a/pkg/gateway/mcp/tool_login.go +++ b/pkg/gateway/mcp/tool_login.go @@ -13,7 +13,6 @@ import ( mcpsdk "github.com/modelcontextprotocol/go-sdk/mcp" - "github.com/hookdeck/hookdeck-cli/pkg/config" "github.com/hookdeck/hookdeck-cli/pkg/hookdeck" "github.com/hookdeck/hookdeck-cli/pkg/project" "github.com/hookdeck/hookdeck-cli/pkg/validators" @@ -26,17 +25,52 @@ const ( // loginState tracks a background login poll so that repeated calls to // hookdeck_login don't start duplicate auth flows. +// +// Synchronization: err is written by the goroutine before close(done). +// The handler only reads err after receiving from done, so the channel +// close provides the happens-before guarantee — no separate mutex needed. type loginState struct { - mu sync.Mutex browserURL string // URL the user must open done chan struct{} // closed when polling finishes err error // non-nil if polling failed } -func handleLogin(client *hookdeck.Client, cfg *config.Config, mcpServer *mcpsdk.Server) mcpsdk.ToolHandler { +func handleLogin(srv *Server) mcpsdk.ToolHandler { + client := srv.client + cfg := srv.cfg + var stateMu sync.Mutex var state *loginState return func(ctx context.Context, req *mcpsdk.CallToolRequest) (*mcpsdk.CallToolResult, error) { + in, err := parseInput(req.Params.Arguments) + if err != nil { + return ErrorResult(err.Error()), nil + } + reauth := in.Bool("reauth") + + stateMu.Lock() + defer stateMu.Unlock() + + if reauth && client.APIKey != "" { + if state != nil { + select { + case <-state.done: + state = nil + default: + return ErrorResult( + "A login flow is already in progress. Call hookdeck_login again after it completes, then use reauth: true if you still need to sign in again.", + ), nil + } + } + if err := cfg.ClearActiveProfileCredentials(); err != nil { + return ErrorResult(fmt.Sprintf("reauth: could not clear stored credentials: %v", err)), nil + } + client.APIKey = "" + client.ProjectID = "" + client.ProjectOrg = "" + client.ProjectName = "" + } + // Already authenticated — nothing to do. if client.APIKey != "" { return TextResult("Already authenticated. All Hookdeck tools are available."), nil @@ -88,38 +122,47 @@ func handleLogin(client *hookdeck.Client, cfg *config.Config, mcpServer *mcpsdk. } // Poll in the background so we return the URL to the agent immediately. + // WaitForAPIKey blocks with time.Sleep internally, so we run it in an + // inner goroutine and select on the session-level context (not the + // per-request ctx, which is cancelled when this handler returns). + sessionCtx := srv.sessionCtx go func(s *loginState) { defer close(s.done) - response, err := session.WaitForAPIKey(loginPollInterval, loginMaxAttempts) - if err != nil { - s.mu.Lock() - s.err = err - s.mu.Unlock() - log.WithError(err).Debug("Login polling failed") + type pollResult struct { + resp *hookdeck.PollAPIKeyResponse + err error + } + ch := make(chan pollResult, 1) + go func() { + resp, err := session.WaitForAPIKey(loginPollInterval, loginMaxAttempts) + ch <- pollResult{resp, err} + }() + + var response *hookdeck.PollAPIKeyResponse + select { + case <-sessionCtx.Done(): + s.err = fmt.Errorf("login cancelled: MCP session closed") + log.Debug("Login polling cancelled — MCP session closed") return + case r := <-ch: + if r.err != nil { + s.err = r.err + log.WithError(r.err).Debug("Login polling failed") + return + } + response = r.resp } if err := validators.APIKey(response.APIKey); err != nil { - s.mu.Lock() s.err = fmt.Errorf("received invalid API key: %s", err) - s.mu.Unlock() return } // Persist credentials so future MCP sessions start authenticated. - cfg.Profile.APIKey = response.APIKey - cfg.Profile.ProjectId = response.ProjectID - cfg.Profile.ProjectMode = response.ProjectMode - cfg.Profile.ProjectType = config.ModeToProjectType(response.ProjectMode) - cfg.Profile.GuestURL = "" - - if err := cfg.Profile.SaveProfile(); err != nil { - log.WithError(err).Error("Login succeeded but failed to save profile") - } - if err := cfg.Profile.UseProfile(); err != nil { - log.WithError(err).Error("Login succeeded but failed to activate profile") - } + cfg.Profile.ApplyPollAPIKeyResponse(response, "") + + cfg.SaveActiveProfileAfterLogin() // Update the shared client so all resource tools start working. client.APIKey = response.APIKey @@ -134,9 +177,6 @@ func handleLogin(client *hookdeck.Client, cfg *config.Config, mcpServer *mcpsdk. client.ProjectOrg = org client.ProjectName = proj - // Remove the login tool now that auth is complete. - mcpServer.RemoveTools("hookdeck_login") - log.WithFields(log.Fields{ "user": response.UserName, "project": response.ProjectName, diff --git a/pkg/gateway/mcp/tool_projects.go b/pkg/gateway/mcp/tool_projects.go index b3303394..57dd8826 100644 --- a/pkg/gateway/mcp/tool_projects.go +++ b/pkg/gateway/mcp/tool_projects.go @@ -45,7 +45,7 @@ type projectEntry struct { func projectsList(client *hookdeck.Client) (*mcpsdk.CallToolResult, error) { projects, err := client.ListProjects() if err != nil { - return ErrorResult(TranslateAPIError(err)), nil + return ErrorResult(listProjectsFailureMessage(err)), nil } items := project.NormalizeProjects(projects, client.ProjectID) @@ -73,7 +73,7 @@ func projectsUse(client *hookdeck.Client, in input) (*mcpsdk.CallToolResult, err projects, err := client.ListProjects() if err != nil { - return ErrorResult(TranslateAPIError(err)), nil + return ErrorResult(listProjectsFailureMessage(err)), nil } items := project.NormalizeProjects(projects, client.ProjectID) diff --git a/pkg/gateway/mcp/tool_projects_errors.go b/pkg/gateway/mcp/tool_projects_errors.go new file mode 100644 index 00000000..921fd8a2 --- /dev/null +++ b/pkg/gateway/mcp/tool_projects_errors.go @@ -0,0 +1,37 @@ +package mcp + +import ( + "errors" + "net/http" + "strings" + + "github.com/hookdeck/hookdeck-cli/pkg/hookdeck" +) + +const listProjectsReauthHint = `This may happen if the stored key is a dashboard or single-project API key that cannot list all teams/projects. Try hookdeck_login with reauth: true so the user can sign in via the browser and replace the credential with a full CLI session, then retry hookdeck_projects.` + +func listProjectsFailureMessage(err error) string { + base := TranslateAPIError(err) + if shouldSuggestReauthAfterListProjectsFailure(err) { + return base + "\n\n" + listProjectsReauthHint + } + return base +} + +func shouldSuggestReauthAfterListProjectsFailure(err error) bool { + var apiErr *hookdeck.APIError + if errors.As(err, &apiErr) { + if apiErr.StatusCode == http.StatusForbidden || apiErr.StatusCode == http.StatusUnauthorized { + return true + } + return strings.Contains(strings.ToLower(apiErr.Message), "fatal") + } + // ListProjects wraps some failures as plain fmt.Errorf with the status code + // in the text (e.g. "unexpected http status code: 403 "). + // Match "status code: 4xx" to avoid false positives on IDs containing "401"/"403". + msg := strings.ToLower(err.Error()) + if strings.Contains(msg, "status code: 403") || strings.Contains(msg, "status code: 401") { + return true + } + return strings.Contains(msg, "fatal") +} diff --git a/pkg/gateway/mcp/tool_projects_errors_test.go b/pkg/gateway/mcp/tool_projects_errors_test.go new file mode 100644 index 00000000..836bc6bb --- /dev/null +++ b/pkg/gateway/mcp/tool_projects_errors_test.go @@ -0,0 +1,63 @@ +package mcp + +import ( + "fmt" + "testing" + + "github.com/hookdeck/hookdeck-cli/pkg/hookdeck" + "github.com/stretchr/testify/assert" +) + +func TestShouldSuggestReauthAfterListProjectsFailure(t *testing.T) { + tests := []struct { + name string + err error + want bool + }{ + { + name: "APIError 403", + err: &hookdeck.APIError{StatusCode: 403, Message: "not allowed"}, + want: true, + }, + { + name: "APIError 401", + err: &hookdeck.APIError{StatusCode: 401, Message: "unauthorized"}, + want: true, + }, + { + name: "APIError 500 no match", + err: &hookdeck.APIError{StatusCode: 500, Message: "internal error"}, + want: false, + }, + { + name: "APIError with fatal message", + err: &hookdeck.APIError{StatusCode: 500, Message: "fatal: cannot proceed"}, + want: true, + }, + { + name: "plain error with status code 403", + err: fmt.Errorf("unexpected http status code: 403 "), + want: true, + }, + { + name: "plain error with status code 401", + err: fmt.Errorf("unexpected http status code: 401 "), + want: true, + }, + { + name: "plain error without status code", + err: fmt.Errorf("network timeout"), + want: false, + }, + { + name: "plain error with 403 in ID should not match", + err: fmt.Errorf("project proj_403_abc not found"), + want: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, shouldSuggestReauthAfterListProjectsFailure(tt.err)) + }) + } +} diff --git a/pkg/gateway/mcp/tools.go b/pkg/gateway/mcp/tools.go index 1f90b7f1..501d8539 100644 --- a/pkg/gateway/mcp/tools.go +++ b/pkg/gateway/mcp/tools.go @@ -22,7 +22,7 @@ func toolDefs(client *hookdeck.Client) []struct { { tool: &mcpsdk.Tool{ Name: "hookdeck_projects", - Description: "Always call this first when the user references a specific project by name. List available projects to find the matching project ID, then use the `use` action to switch to it before calling any other tools. All queries (events, issues, connections, metrics, requests) are scoped to the active project — if the wrong project is active, all results will be wrong. Also use this when unsure which project is currently active. JSON successes use a standard data/meta envelope; see hookdeck_help (overview or any tool topic).", + Description: "Always call this first when the user references a specific project by name. List available projects to find the matching project ID, then use the `use` action to switch to it before calling any other tools. All queries (events, issues, connections, metrics, requests) are scoped to the active project — if the wrong project is active, all results will be wrong. Also use this when unsure which project is currently active. If list or use fails (especially 401/403), the error may suggest hookdeck_login with reauth: true. JSON successes use a standard data/meta envelope; see hookdeck_help (overview or any tool topic).", InputSchema: schema(map[string]prop{ "action": {Type: "string", Desc: "Action to perform: list or use", Enum: []string{"list", "use"}}, "project_id": {Type: "string", Desc: "Project ID (required for use action)"}, diff --git a/pkg/hookdeck/auth.go b/pkg/hookdeck/auth.go index 55cba8f3..ed59d7da 100644 --- a/pkg/hookdeck/auth.go +++ b/pkg/hookdeck/auth.go @@ -133,9 +133,28 @@ func (c *Client) PollForAPIKeyWithKey(apiKey string, interval time.Duration, max return pollForAPIKey(pollURL, interval, maxAttempts) } +// clientForCLIAuthValidate returns a shallow copy of the client that omits +// X-Team-ID / X-Project-ID on requests. Stale project_id in config must not +// be sent to /cli-auth/validate — the server may prefer headers over the key's +// bound team and reject a valid key with 401. +func (c *Client) clientForCLIAuthValidate() *Client { + return &Client{ + BaseURL: c.BaseURL, + APIKey: c.APIKey, + ProjectID: "", + ProjectOrg: c.ProjectOrg, + ProjectName: c.ProjectName, + Verbose: c.Verbose, + SuppressRateLimitErrors: c.SuppressRateLimitErrors, + Telemetry: c.Telemetry, + TelemetryDisabled: c.TelemetryDisabled, + httpClient: c.httpClient, + } +} + // ValidateAPIKey validates an API key and returns user/project information func (c *Client) ValidateAPIKey() (*ValidateAPIKeyResponse, error) { - res, err := c.Get(context.Background(), APIPathPrefix+"/cli-auth/validate", "", nil) + res, err := c.clientForCLIAuthValidate().Get(context.Background(), APIPathPrefix+"/cli-auth/validate", "", nil) if err != nil { return nil, err } diff --git a/pkg/hookdeck/auth_test.go b/pkg/hookdeck/auth_test.go new file mode 100644 index 00000000..e8e202e0 --- /dev/null +++ b/pkg/hookdeck/auth_test.go @@ -0,0 +1,52 @@ +package hookdeck + +import ( + "encoding/json" + "net/http" + "net/http/httptest" + "net/url" + "testing" + + "github.com/stretchr/testify/require" +) + +func TestValidateAPIKey_omitsTeamAndProjectHeadersWhenConfigHasProjectID(t *testing.T) { + var sawTeamHeader bool + var sawProjectHeader bool + + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + sawTeamHeader = r.Header.Get("X-Team-ID") != "" + sawProjectHeader = r.Header.Get("X-Project-ID") != "" + if r.URL.Path != APIPathPrefix+"/cli-auth/validate" { + http.NotFound(w, r) + return + } + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(ValidateAPIKeyResponse{ + UserID: "u1", + UserName: "n", + UserEmail: "e@e", + OrganizationName: "o", + OrganizationID: "o1", + ProjectID: "t1", + ProjectName: "p", + ProjectMode: "gateway", + }) + })) + t.Cleanup(server.Close) + + baseURL, err := url.Parse(server.URL) + require.NoError(t, err) + + client := &Client{ + BaseURL: baseURL, + APIKey: "test_key", + ProjectID: "stale_team_should_not_be_sent", + } + + resp, err := client.ValidateAPIKey() + require.NoError(t, err) + require.False(t, sawTeamHeader, "validate must not send X-Team-ID") + require.False(t, sawProjectHeader, "validate must not send X-Project-ID") + require.Equal(t, "t1", resp.ProjectID) +} diff --git a/pkg/hookdeck/projects.go b/pkg/hookdeck/projects.go index 3fd9ce28..5a46b4d7 100644 --- a/pkg/hookdeck/projects.go +++ b/pkg/hookdeck/projects.go @@ -2,8 +2,6 @@ package hookdeck import ( "context" - "fmt" - "net/http" ) type Project struct { @@ -17,8 +15,8 @@ func (c *Client) ListProjects() ([]Project, error) { if err != nil { return []Project{}, err } - if res.StatusCode != http.StatusOK { - return []Project{}, fmt.Errorf("unexpected http status code: %d %s", res.StatusCode, err) + if err := checkAndPrintError(res); err != nil { + return []Project{}, err } projects := []Project{} postprocessJsonResponse(res, &projects) diff --git a/pkg/hookdeck/telemetry.go b/pkg/hookdeck/telemetry.go index bdefa306..fc0e738f 100644 --- a/pkg/hookdeck/telemetry.go +++ b/pkg/hookdeck/telemetry.go @@ -5,10 +5,12 @@ import ( "encoding/hex" "encoding/json" "os" + "sort" "strings" "sync" "github.com/spf13/cobra" + "github.com/spf13/pflag" ) // @@ -32,6 +34,8 @@ type CLITelemetry struct { DeviceName string `json:"device_name"` GeneratedResource bool `json:"generated_resource,omitempty"` MCPClient string `json:"mcp_client,omitempty"` + // CommandFlags lists flag names (not values) explicitly set on the CLI for this invocation. + CommandFlags []string `json:"command_flags,omitempty"` // Disabled is set from the user's config (telemetry_disabled). // It is checked by PerformRequest as a fallback so that clients @@ -58,6 +62,11 @@ func (t *CLITelemetry) SetDeviceName(deviceName string) { t.DeviceName = deviceName } +// SetCommandFlagsFromCobra records which flags the user explicitly set (names only). +func (t *CLITelemetry) SetCommandFlagsFromCobra(cmd *cobra.Command) { + t.CommandFlags = CollectChangedFlagNames(cmd) +} + // SetSource sets the telemetry source (e.g. "cli" or "mcp"). func (t *CLITelemetry) SetSource(source string) { t.Source = source @@ -100,6 +109,38 @@ func NewInvocationID() string { return "inv_" + hex.EncodeToString(b) } +// CollectChangedFlagNames returns sorted unique names of flags the user explicitly +// set on the command line (pflag Changed), walking from cmd up to the root. Values +// are not included. +func CollectChangedFlagNames(cmd *cobra.Command) []string { + if cmd == nil { + return nil + } + seen := make(map[string]struct{}) + var names []string + add := func(fs *pflag.FlagSet) { + if fs == nil { + return + } + fs.VisitAll(func(f *pflag.Flag) { + if !f.Changed { + return + } + if _, ok := seen[f.Name]; ok { + return + } + seen[f.Name] = struct{}{} + names = append(names, f.Name) + }) + } + for c := cmd; c != nil; c = c.Parent() { + add(c.Flags()) + add(c.PersistentFlags()) + } + sort.Strings(names) + return names +} + // DetectEnvironment returns "ci" if a CI environment is detected, // "interactive" otherwise. func DetectEnvironment() string { diff --git a/pkg/hookdeck/telemetry_flags_test.go b/pkg/hookdeck/telemetry_flags_test.go new file mode 100644 index 00000000..a9f2a8c7 --- /dev/null +++ b/pkg/hookdeck/telemetry_flags_test.go @@ -0,0 +1,42 @@ +package hookdeck + +import ( + "testing" + + "github.com/spf13/cobra" + "github.com/stretchr/testify/require" +) + +func TestCollectChangedFlagNames(t *testing.T) { + root := &cobra.Command{Use: "hookdeck"} + root.PersistentFlags().String("cli-key", "", "") + root.PersistentFlags().String("api-key", "", "") + login := &cobra.Command{ + Use: "login", + Run: func(cmd *cobra.Command, args []string) {}, + } + login.Flags().Bool("local", false, "") + login.Flags().BoolP("interactive", "i", false, "") + root.AddCommand(login) + + root.SetArgs([]string{"login", "--cli-key", "secret", "--local"}) + cmd, err := root.ExecuteC() + require.NoError(t, err) + names := CollectChangedFlagNames(cmd) + require.ElementsMatch(t, []string{"cli-key", "local"}, names) +} + +func TestCollectChangedFlagNames_apiKeyAlias(t *testing.T) { + root := &cobra.Command{Use: "hookdeck"} + root.PersistentFlags().String("api-key", "", "") + login := &cobra.Command{Use: "login", Run: func(*cobra.Command, []string) {}} + root.AddCommand(login) + root.SetArgs([]string{"login", "--api-key", "k"}) + cmd, err := root.ExecuteC() + require.NoError(t, err) + require.Equal(t, []string{"api-key"}, CollectChangedFlagNames(cmd)) +} + +func TestCollectChangedFlagNames_nilCommand(t *testing.T) { + require.Nil(t, CollectChangedFlagNames(nil)) +} diff --git a/pkg/login/client_login.go b/pkg/login/client_login.go index d79b5fe4..678cb0b5 100644 --- a/pkg/login/client_login.go +++ b/pkg/login/client_login.go @@ -39,6 +39,8 @@ func Login(config *configpkg.Config, input io.Reader) error { message := SuccessMessage(response.UserName, response.UserEmail, response.OrganizationName, response.ProjectName, response.ProjectMode == "console") ansi.StopSpinner(s, message, os.Stdout) + config.Profile.ApplyValidateAPIKeyResponse(response, true) + if err = config.Profile.SaveProfile(); err != nil { return err } @@ -92,11 +94,7 @@ func Login(config *configpkg.Config, input io.Reader) error { return err } - config.Profile.APIKey = response.APIKey - config.Profile.ProjectId = response.ProjectID - config.Profile.ProjectMode = response.ProjectMode - config.Profile.ProjectType = configpkg.ModeToProjectType(response.ProjectMode) - config.Profile.GuestURL = "" // Clear guest URL when logging in with permanent account + config.Profile.ApplyPollAPIKeyResponse(response, "") if err = config.Profile.SaveProfile(); err != nil { return err @@ -138,11 +136,7 @@ func GuestLogin(config *configpkg.Config) (string, error) { return "", err } - config.Profile.APIKey = response.APIKey - config.Profile.ProjectId = response.ProjectID - config.Profile.ProjectMode = response.ProjectMode - config.Profile.ProjectType = configpkg.ModeToProjectType(response.ProjectMode) - config.Profile.GuestURL = session.GuestURL + config.Profile.ApplyPollAPIKeyResponse(response, session.GuestURL) if err = config.Profile.SaveProfile(); err != nil { return "", err @@ -181,10 +175,7 @@ func CILogin(config *configpkg.Config, apiKey string, name string) error { return err } - config.Profile.APIKey = response.APIKey - config.Profile.ProjectId = response.ProjectID - config.Profile.ProjectMode = response.ProjectMode - config.Profile.ProjectType = configpkg.ModeToProjectType(response.ProjectMode) + config.Profile.ApplyCIClient(response) if err = config.Profile.SaveProfile(); err != nil { return err diff --git a/pkg/login/interactive_login.go b/pkg/login/interactive_login.go index 1cd49def..aeaf0b2d 100644 --- a/pkg/login/interactive_login.go +++ b/pkg/login/interactive_login.go @@ -55,11 +55,7 @@ func InteractiveLogin(config *configpkg.Config) error { return err } - config.Profile.APIKey = response.APIKey - config.Profile.ProjectMode = response.ProjectMode - config.Profile.ProjectId = response.ProjectID - config.Profile.ProjectType = configpkg.ModeToProjectType(response.ProjectMode) - config.Profile.GuestURL = "" // Clear guest URL when logging in with permanent account + config.Profile.ApplyPollAPIKeyResponse(response, "") if err = config.Profile.SaveProfile(); err != nil { ansi.StopSpinner(s, "", os.Stdout) diff --git a/pkg/logout/logout.go b/pkg/logout/logout.go index 77674ea9..426d99fa 100644 --- a/pkg/logout/logout.go +++ b/pkg/logout/logout.go @@ -18,7 +18,7 @@ func Logout(config *config.Config) error { fmt.Println("Logging out...") profileName := config.Profile.Name - if err := config.Profile.RemoveProfile(); err != nil { + if err := config.ClearActiveProfileCredentials(); err != nil { return err } diff --git a/test/acceptance/README.md b/test/acceptance/README.md index cd0e6d68..12f7c5e4 100644 --- a/test/acceptance/README.md +++ b/test/acceptance/README.md @@ -18,15 +18,15 @@ These tests require browser-based authentication via `hookdeck login` and must b **Why Manual?** These tests access endpoints (like `/teams`) that require CLI authentication keys obtained through interactive browser login, which aren't available to CI service accounts. -### Transient HTTP 502 from the API +### Transient HTTP 502 / 500 from the API -`CLIRunner.Run`, `RunWithEnv`, and `RunFromCwd` retry the same command up to **4** times when combined stdout/stderr looks like a Hookdeck API **HTTP 502** (matching CLI error text such as `unexpected http status code: 502`). **503** and **504** are not treated specially. Each retry is logged with `t.Logf` (attempt number, command summary, output excerpts); if all attempts fail, a final log line notes that the run is giving up. +`CLIRunner.Run`, `RunWithEnv`, and `RunFromCwd` retry the same command up to **4** times when combined stdout/stderr looks like a transient Hookdeck API **HTTP 502** or **HTTP 500** (matching CLI log text such as `status=502` / `status=500`). **503** and **504** are not treated specially. Each retry is logged with `t.Logf` (attempt number, command summary, output excerpts); if all attempts fail, a final log line notes that the run is giving up. ### Recording proxy (telemetry tests) Some tests (e.g. `TestTelemetryGatewayConnectionListProxy` in `telemetry_test.go`, `TestTelemetryListenProxy` in `telemetry_listen_test.go`) use a **recording proxy**: the CLI is run with `--api-base` pointing at a local HTTP server that forwards every request to the real Hookdeck API and records method, path, and the `X-Hookdeck-CLI-Telemetry` header. The same `CLIRunner` and `go run main.go` flow are used as in other acceptance tests; only the API base URL is overridden so traffic goes through the proxy. This verifies that a single CLI run sends consistent telemetry (same `invocation_id` and `command_path`) on all API calls. Helpers: `StartRecordingProxy`, `AssertTelemetryConsistent`. -**Login telemetry test (TestTelemetryLoginProxy)** uses the same proxy approach: it runs `hookdeck login --api-key KEY` with `--api-base` set to the proxy, and asserts exactly one recorded request (GET `/2025-07-01/cli-auth/validate`) with consistent telemetry. It uses **HOOKDECK_CLI_TESTING_CLI_KEY** (not the API/CI key), because the validate endpoint accepts CLI keys from interactive login; if unset, the test is skipped. Other telemetry tests still use the normal API key via `NewCLIRunner`. +**Login telemetry tests** use the same proxy approach with **HOOKDECK_CLI_TESTING_CLI_KEY** (not the API/CI key), because the validate endpoint accepts CLI keys from interactive login; if unset, those tests are skipped. **TestTelemetryLoginProxy** runs `hookdeck login --api-key KEY` with `--api-base` set to the proxy and asserts exactly one recorded request (GET `/2025-07-01/cli-auth/validate`) with consistent telemetry. **TestTelemetryLoginCommandFlagsProxy** additionally asserts the telemetry JSON includes **`command_flags`** containing **`api-key`** or **`cli-key`** on the wire when that flag is passed. Other telemetry tests still use the normal API key via `NewCLIRunner`. ## Setup diff --git a/test/acceptance/helpers.go b/test/acceptance/helpers.go index d58553db..aa572b2b 100644 --- a/test/acceptance/helpers.go +++ b/test/acceptance/helpers.go @@ -25,22 +25,26 @@ import ( const defaultAPIUpstream = "https://api.hookdeck.com" // acceptance502MaxAttempts is how many times CLIRunner runs a command when the -// combined output looks like a Hookdeck API HTTP 502 (transient gateway errors). +// combined output looks like a transient Hookdeck API error (HTTP 502 gateway or +// occasional HTTP 500 from upstream). const acceptance502MaxAttempts = 4 -// acceptance502RetryDelay is the pause between 502 retries. +// acceptance502RetryDelay is the pause between transient-error retries. const acceptance502RetryDelay = 2 * time.Second // acceptance502LogExcerpt is the max chars of stdout/stderr to include in retry logs. const acceptance502LogExcerpt = 800 // combinedOutputLooksLikeHTTP502 returns true when stdout+stderr match patterns -// emitted by the CLI/API client for HTTP 502 (only; 503/504 are not retried here). +// emitted by the CLI/API client for transient HTTP 502/500 (503/504 are not retried here). func combinedOutputLooksLikeHTTP502(stdout, stderr string) bool { combined := stdout + "\n" + stderr return strings.Contains(combined, "status code: 502") || strings.Contains(combined, "status=502") || - strings.Contains(combined, "error code: 502") + strings.Contains(combined, "error code: 502") || + strings.Contains(combined, "status code: 500") || + strings.Contains(combined, "status=500") || + strings.Contains(combined, "error code: 500") } func excerptFor502Log(s string, max int) string { @@ -64,7 +68,7 @@ func commandSummaryFor502Log(args []string) string { } // runWithHTTP502Retry re-runs run() when the process exits with an error and -// output looks like HTTP 502 from the Hookdeck API. Logs each retry clearly via t.Logf. +// output looks like a transient Hookdeck API HTTP 502/500. Logs each retry clearly via t.Logf. func (r *CLIRunner) runWithHTTP502Retry(commandSummary string, run func() (stdout, stderr string, err error)) (stdout, stderr string, err error) { r.t.Helper() var lastStdout, lastStderr string @@ -75,7 +79,7 @@ func (r *CLIRunner) runWithHTTP502Retry(commandSummary string, run func() (stdou return lastStdout, lastStderr, lastErr } if attempt < acceptance502MaxAttempts { - r.t.Logf("acceptance: Hookdeck API HTTP 502 (transient); retrying CLI command [%s] (attempt %d/%d, next retry after %v)\nstderr excerpt:\n%s\nstdout excerpt:\n%s", + r.t.Logf("acceptance: Hookdeck API transient HTTP 502/500; retrying CLI command [%s] (attempt %d/%d, next retry after %v)\nstderr excerpt:\n%s\nstdout excerpt:\n%s", commandSummary, attempt, acceptance502MaxAttempts, acceptance502RetryDelay, excerptFor502Log(lastStderr, acceptance502LogExcerpt), excerptFor502Log(lastStdout, acceptance502LogExcerpt)) @@ -83,7 +87,7 @@ func (r *CLIRunner) runWithHTTP502Retry(commandSummary string, run func() (stdou } } if lastErr != nil && combinedOutputLooksLikeHTTP502(lastStdout, lastStderr) { - r.t.Logf("acceptance: Hookdeck API HTTP 502 still failing after %d attempts (command [%s]); giving up. stderr excerpt:\n%s\nstdout excerpt:\n%s", + r.t.Logf("acceptance: Hookdeck API transient HTTP 502/500 still failing after %d attempts (command [%s]); giving up. stderr excerpt:\n%s\nstdout excerpt:\n%s", acceptance502MaxAttempts, commandSummary, excerptFor502Log(lastStderr, acceptance502LogExcerpt), excerptFor502Log(lastStdout, acceptance502LogExcerpt)) @@ -521,6 +525,72 @@ func (r *CLIRunner) RunListenWithTimeout(args []string, runDuration time.Duratio return stdoutBuf.String(), stderrBuf.String(), waitErr } +// RunGatewayMCPSubprocess builds the CLI binary, runs `gateway mcp` with optional stdin, +// lets it run for runDuration, then kills the process. Returns stdout, stderr, and the +// error from Wait (often non-nil because the process was killed). configPath, when non-empty, +// is passed as HOOKDECK_CONFIG_FILE. extraEnv entries override the process environment. +func RunGatewayMCPSubprocess(t *testing.T, projectRoot, configPath string, extraEnv map[string]string, stdin string, runDuration time.Duration) (stdout, stderr string, err error) { + t.Helper() + tmpBinary := filepath.Join(projectRoot, "hookdeck-mcp-test-"+generateTimestamp()) + defer os.Remove(tmpBinary) + + buildCmd := exec.Command("go", "build", "-o", tmpBinary, ".") + buildCmd.Dir = projectRoot + if buildErr := buildCmd.Run(); buildErr != nil { + return "", "", fmt.Errorf("build CLI for gateway mcp test: %w", buildErr) + } + + cmd := exec.Command(tmpBinary, "gateway", "mcp") + cmd.Dir = projectRoot + env := os.Environ() + if configPath != "" { + env = appendEnvOverride(env, "HOOKDECK_CONFIG_FILE", configPath) + } + for k, v := range extraEnv { + env = appendEnvOverride(env, k, v) + } + cmd.Env = env + + var stdoutBuf, stderrBuf bytes.Buffer + cmd.Stdout = &stdoutBuf + cmd.Stderr = &stderrBuf + + closeStdin := func() {} + if stdin != "" { + pr, pw, pipeErr := os.Pipe() + if pipeErr != nil { + return "", "", pipeErr + } + cmd.Stdin = pr + if _, werr := io.WriteString(pw, stdin); werr != nil { + _ = pr.Close() + _ = pw.Close() + return "", "", werr + } + if !strings.HasSuffix(stdin, "\n") { + _, _ = pw.Write([]byte("\n")) + } + closeStdin = func() { + _ = pw.Close() + _ = pr.Close() + } + } + + if err := cmd.Start(); err != nil { + closeStdin() + return "", "", err + } + timer := time.AfterFunc(runDuration, func() { + if cmd.Process != nil { + _ = cmd.Process.Kill() + } + }) + defer timer.Stop() + waitErr := cmd.Wait() + closeStdin() + return stdoutBuf.String(), stderrBuf.String(), waitErr +} + // RunFromCwd executes the CLI from the current working directory. // This is useful for tests that need to test --local flag behavior, // which creates config in the current directory. diff --git a/test/acceptance/mcp_test.go b/test/acceptance/mcp_test.go index 61abc7c1..351e6c51 100644 --- a/test/acceptance/mcp_test.go +++ b/test/acceptance/mcp_test.go @@ -3,11 +3,46 @@ package acceptance import ( + "encoding/json" + "fmt" + "os" + "path/filepath" + "strings" "testing" + "time" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) +const mcpInitializeJSON = `{"jsonrpc":"2.0","id":1,"method":"initialize","params":{"protocolVersion":"2025-03-26","clientInfo":{"name":"test","version":"1.0"},"capabilities":{}}}` + +func firstJSONRPCMessageLine(t *testing.T, stdout string) map[string]any { + t.Helper() + for _, line := range strings.Split(stdout, "\n") { + line = strings.TrimSpace(line) + if line == "" { + continue + } + var msg map[string]any + if err := json.Unmarshal([]byte(line), &msg); err != nil { + continue + } + if _, ok := msg["jsonrpc"]; ok { + return msg + } + } + t.Fatalf("no JSON-RPC line in stdout: %q", stdout) + return nil +} + +func assertGatewayMCPStdioHygiene(t *testing.T, stdout, stderr string) { + t.Helper() + assert.NotContains(t, stdout, "Running `hookdeck login`") + assert.NotContains(t, stdout, "You aren't") + assert.NotContains(t, stderr, "Running `hookdeck login`") +} + // --- Help --- func TestMCPHelp(t *testing.T) { @@ -29,3 +64,77 @@ func TestGatewayHelpListsMCP(t *testing.T) { stdout := cli.RunExpectSuccess("gateway", "--help") assert.Contains(t, stdout, "mcp", "gateway --help should list 'mcp' subcommand") } + +// --- Stdio / auth-aware gateway MCP (subprocess) --- + +func TestGatewayMCPStdio_UnauthenticatedInitialize(t *testing.T) { + if testing.Short() { + t.Skip("Skipping acceptance test in short mode") + } + projectRoot, err := filepath.Abs("../..") + require.NoError(t, err) + cfgPath := filepath.Join(t.TempDir(), "config.toml") + require.NoError(t, os.WriteFile(cfgPath, []byte("profile = \"default\"\n\n[default]\n"), 0644)) + + extra := map[string]string{ + "HOOKDECK_CLI_TESTING_API_KEY": "", + "HOOKDECK_CLI_TESTING_API_KEY_2": "", + "HOOKDECK_CLI_TESTING_API_KEY_3": "", + } + stdout, stderr, _ := RunGatewayMCPSubprocess(t, projectRoot, cfgPath, extra, mcpInitializeJSON+"\n", 4*time.Second) + msg := firstJSONRPCMessageLine(t, stdout) + assert.Equal(t, "2.0", msg["jsonrpc"]) + assertGatewayMCPStdioHygiene(t, stdout, stderr) +} + +func TestGatewayMCPStdio_AuthenticatedInitialize(t *testing.T) { + if testing.Short() { + t.Skip("Skipping acceptance test in short mode") + } + cli := NewCLIRunner(t) + stdout, stderr, _ := RunGatewayMCPSubprocess(t, cli.projectRoot, cli.configPath, nil, mcpInitializeJSON+"\n", 4*time.Second) + msg := firstJSONRPCMessageLine(t, stdout) + assert.Equal(t, "2.0", msg["jsonrpc"]) + assertGatewayMCPStdioHygiene(t, stdout, stderr) +} + +func TestGatewayMCPStdio_NoProjectExitsWithStderr(t *testing.T) { + if testing.Short() { + t.Skip("Skipping acceptance test in short mode") + } + projectRoot, err := filepath.Abs("../..") + require.NoError(t, err) + apiKey := os.Getenv("HOOKDECK_CLI_TESTING_API_KEY") + require.NotEmpty(t, apiKey, "HOOKDECK_CLI_TESTING_API_KEY required") + cfgPath := filepath.Join(t.TempDir(), "config.toml") + toml := fmt.Sprintf("profile = \"default\"\n\n[default]\napi_key = %q\n", apiKey) + require.NoError(t, os.WriteFile(cfgPath, []byte(toml), 0644)) + + stdout, stderr, waitErr := RunGatewayMCPSubprocess(t, projectRoot, cfgPath, nil, "", 4*time.Second) + assert.Error(t, waitErr) + lower := strings.ToLower(stderr) + assert.True(t, strings.Contains(lower, "project"), "stderr=%q", stderr) + if strings.TrimSpace(stdout) != "" { + assertGatewayMCPStdioHygiene(t, stdout, stderr) + } +} + +func TestGatewayMCPStdio_OutpostProjectRejected(t *testing.T) { + if testing.Short() { + t.Skip("Skipping acceptance test in short mode") + } + projectRoot, err := filepath.Abs("../..") + require.NoError(t, err) + apiKey := os.Getenv("HOOKDECK_CLI_TESTING_API_KEY") + require.NotEmpty(t, apiKey) + cfgPath := filepath.Join(t.TempDir(), "config.toml") + toml := fmt.Sprintf("profile = \"default\"\n\n[default]\napi_key = %q\nproject_id = \"proj_outpost_fake\"\nproject_type = \"Outpost\"\n", apiKey) + require.NoError(t, os.WriteFile(cfgPath, []byte(toml), 0644)) + + stdout, stderr, waitErr := RunGatewayMCPSubprocess(t, projectRoot, cfgPath, nil, "", 4*time.Second) + assert.Error(t, waitErr) + assert.Contains(t, strings.ToLower(stderr), "gateway") + if strings.TrimSpace(stdout) != "" { + assertGatewayMCPStdioHygiene(t, stdout, stderr) + } +} diff --git a/test/acceptance/telemetry_test.go b/test/acceptance/telemetry_test.go index 9e15e4c5..ed1a562d 100644 --- a/test/acceptance/telemetry_test.go +++ b/test/acceptance/telemetry_test.go @@ -74,6 +74,51 @@ func TestTelemetryLoginProxy(t *testing.T) { AssertTelemetryConsistent(t, recorded, "hookdeck login") } +// TestTelemetryLoginCommandFlagsProxy asserts X-Hookdeck-CLI-Telemetry JSON includes +// command_flags (flag names only) on the wire for hookdeck login when --api-key or +// --cli-key is passed. Same env and proxy pattern as TestTelemetryLoginProxy. +func TestTelemetryLoginCommandFlagsProxy(t *testing.T) { + if testing.Short() { + t.Skip("Skipping acceptance test in short mode") + } + cliKey := os.Getenv("HOOKDECK_CLI_TESTING_CLI_KEY") + if cliKey == "" { + t.Skip("Skipping: HOOKDECK_CLI_TESTING_CLI_KEY must be set (same as login telemetry test)") + } + cli := NewCLIRunner(t) + + t.Run("api-key", func(t *testing.T) { + proxy := StartRecordingProxy(t, defaultAPIUpstream) + defer proxy.Close() + stdout, stderr, err := cli.Run("--api-base", proxy.URL(), "login", "--api-key", cliKey) + require.NoError(t, err, "stdout=%q stderr=%q", stdout, stderr) + recorded := proxy.Recorded() + require.Len(t, recorded, 1) + assertTelemetryCommandFlagsContain(t, recorded[0].Telemetry, "api-key") + }) + + t.Run("cli-key", func(t *testing.T) { + proxy := StartRecordingProxy(t, defaultAPIUpstream) + defer proxy.Close() + stdout, stderr, err := cli.Run("--api-base", proxy.URL(), "login", "--cli-key", cliKey) + require.NoError(t, err, "stdout=%q stderr=%q", stdout, stderr) + recorded := proxy.Recorded() + require.Len(t, recorded, 1) + assertTelemetryCommandFlagsContain(t, recorded[0].Telemetry, "cli-key") + }) +} + +func assertTelemetryCommandFlagsContain(t *testing.T, telemetryJSON, wantFlag string) { + t.Helper() + require.NotEmpty(t, telemetryJSON, "X-Hookdeck-CLI-Telemetry should be present when telemetry is enabled") + var p struct { + CommandFlags []string `json:"command_flags"` + } + require.NoError(t, json.Unmarshal([]byte(telemetryJSON), &p), "telemetry JSON: %s", telemetryJSON) + require.Contains(t, p.CommandFlags, wantFlag, + "command_flags on the wire should include %q (got %v)", wantFlag, p.CommandFlags) +} + func TestTelemetryGatewayConnectionListProxy(t *testing.T) { if testing.Short() { t.Skip("Skipping acceptance test in short mode")