From 2c32144df06de499eea7db9eee97fc4d1529c47b Mon Sep 17 00:00:00 2001 From: Facundo Farias Date: Fri, 29 May 2026 08:27:28 +0200 Subject: [PATCH 1/2] fix(mcp): inject keyring credentials into MCP server child process MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The deployhq-mcp-server reads credentials from DEPLOYHQ_ACCOUNT, DEPLOYHQ_EMAIL, and DEPLOYHQ_API_KEY env vars and calls process.exit(1) if any is missing. But `dhq mcp` was just spawning the binary with c.Env = os.Environ() — the user's keyring-stored credentials never reached the child process. Telemetry showed 3 of 4 mcp invocations (75%) failing with "MCP server exited: exit status 1", all from users with no agent env wrapper (no codex/claude-code/warp shell) on darwin. These are people who ran `dhq auth login` (creds → keyring), then `dhq mcp`, and got a silent crash because the child couldn't see the keyring. Changes: - Extract credential resolution into a Context.Credentials helper that returns the resolved (account, email, apiKey) triplet using the same precedence as Client(): flags → env → file → keyring. - Refactor Client() to use it (no behavior change for that path). - In `dhq mcp`, call Credentials() before spawning the server and merge the three DEPLOYHQ_* vars into the child env if they aren't already set by the user. - mergeEnv preserves any explicit env override the user set (e.g. pointing the server at a different account for one session) and skips empty values. Co-Authored-By: Claude Opus 4.7 (1M context) --- internal/cli/context.go | 44 ++++++++++++++++++---------- internal/commands/mcp.go | 39 ++++++++++++++++++++++++- internal/commands/mcp_test.go | 55 +++++++++++++++++++++++++++++++++++ 3 files changed, 122 insertions(+), 16 deletions(-) create mode 100644 internal/commands/mcp_test.go diff --git a/internal/cli/context.go b/internal/cli/context.go index d8f9558..1bccb4a 100644 --- a/internal/cli/context.go +++ b/internal/cli/context.go @@ -34,22 +34,22 @@ func NewContext(cfg *config.Config, env *output.Envelope, logger *output.Logger) } } -// Client returns the SDK client, creating it on first use. -// It merges credentials from config and auth store. -func (c *Context) Client() (*sdk.Client, error) { - if c.client != nil { - return c.client, nil - } - - account := c.Config.Account - email := c.Config.Email - apiKey := c.Config.APIKey +// Credentials returns the resolved (account, email, apiKey) triplet, +// merging values from config (flags/env/files) with the auth store. It +// returns the same AuthError/UserError shapes that Client() would. +// +// This is useful for code paths that need credentials but don't need +// the full SDK client — e.g. shelling out to a child process that +// reads them from environment variables. +func (c *Context) Credentials() (account, email, apiKey string, err error) { + account = c.Config.Account + email = c.Config.Email + apiKey = c.Config.APIKey - // Fill gaps from auth store (look up by account name if known) if account == "" || email == "" || apiKey == "" { - creds, err := auth.LoadByAccount(account) - if err != nil { - return nil, &output.AuthError{ + creds, loadErr := auth.LoadByAccount(account) + if loadErr != nil { + return "", "", "", &output.AuthError{ Message: "Not logged in", Hint: "Authenticate first:\n" + " dhq auth login (interactive)\n" + @@ -68,11 +68,25 @@ func (c *Context) Client() (*sdk.Client, error) { } if account == "" { - return nil, &output.UserError{ + return "", "", "", &output.UserError{ Message: "Account not configured", Hint: "Set via --account flag, DEPLOYHQ_ACCOUNT env var, or 'dhq config set account '", } } + return account, email, apiKey, nil +} + +// Client returns the SDK client, creating it on first use. +// It merges credentials from config and auth store. +func (c *Context) Client() (*sdk.Client, error) { + if c.client != nil { + return c.client, nil + } + + account, email, apiKey, err := c.Credentials() + if err != nil { + return nil, err + } opts := []sdk.Option{} agent := harness.AgentInfo{} diff --git a/internal/commands/mcp.go b/internal/commands/mcp.go index 8a53234..17e5ac0 100644 --- a/internal/commands/mcp.go +++ b/internal/commands/mcp.go @@ -4,6 +4,7 @@ import ( "os" "os/exec" "path/filepath" + "strings" "github.com/deployhq/deployhq-cli/internal/output" "github.com/manifoldco/promptui" @@ -69,6 +70,15 @@ The MCP server binary is searched in: } } + // The MCP server reads credentials from DEPLOYHQ_ACCOUNT / DEPLOYHQ_EMAIL / + // DEPLOYHQ_API_KEY and exits with status 1 if any is missing. Inject the + // resolved CLI credentials so users who logged in via keyring (the common + // case) don't have to re-export them just to start the server. + account, email, apiKey, err := cliCtx.Credentials() + if err != nil { + return err + } + cmdArgs := append(bin.args, args...) cliCtx.Logger.Write("Starting MCP server: %s %v", bin.path, cmdArgs) @@ -76,7 +86,11 @@ The MCP server binary is searched in: c.Stdin = os.Stdin c.Stdout = os.Stdout c.Stderr = os.Stderr - c.Env = os.Environ() + c.Env = mergeEnv(os.Environ(), map[string]string{ + "DEPLOYHQ_ACCOUNT": account, + "DEPLOYHQ_EMAIL": email, + "DEPLOYHQ_API_KEY": apiKey, + }) if err := c.Run(); err != nil { return &output.InternalError{Message: "MCP server exited", Cause: err} @@ -86,6 +100,29 @@ The MCP server binary is searched in: } } +// mergeEnv returns env with each key in overrides set to its value, only when +// the key is not already present. This preserves the user's explicit env-var +// overrides (e.g. pointing the MCP server at a different account) while still +// providing sensible defaults from the keyring. +func mergeEnv(env []string, overrides map[string]string) []string { + present := make(map[string]struct{}, len(env)) + for _, kv := range env { + if i := strings.Index(kv, "="); i > 0 { + present[kv[:i]] = struct{}{} + } + } + for k, v := range overrides { + if _, ok := present[k]; ok { + continue + } + if v == "" { + continue + } + env = append(env, k+"="+v) + } + return env +} + func findMCPBinary() *mcpBinary { // 1. In PATH if p, err := exec.LookPath("deployhq-mcp-server"); err == nil { diff --git a/internal/commands/mcp_test.go b/internal/commands/mcp_test.go new file mode 100644 index 0000000..45b312a --- /dev/null +++ b/internal/commands/mcp_test.go @@ -0,0 +1,55 @@ +package commands + +import ( + "sort" + "strings" + "testing" +) + +func TestMergeEnv(t *testing.T) { + tests := []struct { + name string + env []string + overrides map[string]string + wantHas []string + wantAbsent []string + }{ + { + name: "adds missing keys", + env: []string{"PATH=/usr/bin", "HOME=/root"}, + overrides: map[string]string{"DEPLOYHQ_ACCOUNT": "acme", "DEPLOYHQ_EMAIL": "a@b"}, + wantHas: []string{"PATH=/usr/bin", "HOME=/root", "DEPLOYHQ_ACCOUNT=acme", "DEPLOYHQ_EMAIL=a@b"}, + }, + { + name: "preserves user-set keys", + env: []string{"DEPLOYHQ_ACCOUNT=other", "PATH=/usr/bin"}, + overrides: map[string]string{"DEPLOYHQ_ACCOUNT": "acme", "DEPLOYHQ_EMAIL": "a@b"}, + wantHas: []string{"DEPLOYHQ_ACCOUNT=other", "DEPLOYHQ_EMAIL=a@b", "PATH=/usr/bin"}, + wantAbsent: []string{"DEPLOYHQ_ACCOUNT=acme"}, + }, + { + name: "skips empty override values", + env: []string{"PATH=/usr/bin"}, + overrides: map[string]string{"DEPLOYHQ_API_KEY": ""}, + wantHas: []string{"PATH=/usr/bin"}, + wantAbsent: []string{"DEPLOYHQ_API_KEY="}, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := mergeEnv(tt.env, tt.overrides) + sort.Strings(got) + joined := strings.Join(got, "\n") + for _, want := range tt.wantHas { + if !strings.Contains(joined, want) { + t.Errorf("expected env to contain %q, got %v", want, got) + } + } + for _, absent := range tt.wantAbsent { + if strings.Contains(joined, absent+"\n") || strings.HasSuffix(joined, absent) { + t.Errorf("expected env NOT to contain %q, got %v", absent, got) + } + } + }) + } +} From 4a6e37041b9d30ecd55cfc48889cf11a38f4595e Mon Sep 17 00:00:00 2001 From: Facundo Farias Date: Fri, 29 May 2026 08:40:16 +0200 Subject: [PATCH 2/2] fix(mcp): treat empty exported DEPLOYHQ_* vars as missing Codex review on PR #17 caught a residual failure mode: when a user has exported a credential as empty (e.g. DEPLOYHQ_API_KEY= with no value) alongside valid keyring credentials, the previous mergeEnv saw the key as present and skipped injecting the resolved value, so the MCP server still received a blank required env var and exited at startup. mergeEnv now drops empty exported entries for keys we're about to override, and the override is appended in their place. Non-empty user exports are still preserved (power-user override path), and unrelated empty exports (e.g. OTHER_THING=) are left alone. Two new test cases cover the empty-export drop and the unrelated-key preservation. Co-Authored-By: Claude Opus 4.7 (1M context) --- internal/commands/mcp.go | 35 ++++++++++++++++++++++++++--------- internal/commands/mcp_test.go | 12 ++++++++++++ 2 files changed, 38 insertions(+), 9 deletions(-) diff --git a/internal/commands/mcp.go b/internal/commands/mcp.go index 17e5ac0..f457f9e 100644 --- a/internal/commands/mcp.go +++ b/internal/commands/mcp.go @@ -101,26 +101,43 @@ The MCP server binary is searched in: } // mergeEnv returns env with each key in overrides set to its value, only when -// the key is not already present. This preserves the user's explicit env-var -// overrides (e.g. pointing the MCP server at a different account) while still -// providing sensible defaults from the keyring. +// the key is not already present with a non-empty value. An exported-but-empty +// variable (e.g. `DEPLOYHQ_API_KEY=`) is treated as missing and dropped from +// the result — otherwise the MCP server would still crash at startup because +// empty required vars fail its validation just like absent ones, and a stale +// empty entry could shadow the override on platforms that take the first +// occurrence. +// +// User-set non-empty values are preserved, so power users can still point the +// server at a different account for one session. func mergeEnv(env []string, overrides map[string]string) []string { - present := make(map[string]struct{}, len(env)) + out := make([]string, 0, len(env)+len(overrides)) + occupied := make(map[string]struct{}, len(env)) for _, kv := range env { - if i := strings.Index(kv, "="); i > 0 { - present[kv[:i]] = struct{}{} + i := strings.Index(kv, "=") + if i <= 0 { + out = append(out, kv) + continue + } + key, val := kv[:i], kv[i+1:] + if val == "" { + if _, willOverride := overrides[key]; willOverride { + continue + } } + out = append(out, kv) + occupied[key] = struct{}{} } for k, v := range overrides { - if _, ok := present[k]; ok { + if _, ok := occupied[k]; ok { continue } if v == "" { continue } - env = append(env, k+"="+v) + out = append(out, k+"="+v) } - return env + return out } func findMCPBinary() *mcpBinary { diff --git a/internal/commands/mcp_test.go b/internal/commands/mcp_test.go index 45b312a..f3c0a13 100644 --- a/internal/commands/mcp_test.go +++ b/internal/commands/mcp_test.go @@ -34,6 +34,18 @@ func TestMergeEnv(t *testing.T) { wantHas: []string{"PATH=/usr/bin"}, wantAbsent: []string{"DEPLOYHQ_API_KEY="}, }, + { + name: "empty exported key is treated as missing and dropped", + env: []string{"DEPLOYHQ_API_KEY=", "PATH=/usr/bin"}, + overrides: map[string]string{"DEPLOYHQ_API_KEY": "realkey"}, + wantHas: []string{"DEPLOYHQ_API_KEY=realkey", "PATH=/usr/bin"}, + }, + { + name: "empty exported key for unrelated var is preserved", + env: []string{"OTHER_THING=", "PATH=/usr/bin"}, + overrides: map[string]string{"DEPLOYHQ_API_KEY": "realkey"}, + wantHas: []string{"OTHER_THING=", "DEPLOYHQ_API_KEY=realkey", "PATH=/usr/bin"}, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) {