diff --git a/internal/agents/claudecode/adapter.go b/internal/agents/claudecode/adapter.go index 2bf70a33..3b52cba9 100644 --- a/internal/agents/claudecode/adapter.go +++ b/internal/agents/claudecode/adapter.go @@ -1,6 +1,7 @@ package claudecode import ( + "encoding/json" "fmt" "io" "os" @@ -111,11 +112,28 @@ func (a *Adapter) Apply(env agents.Env, opts agents.ApplyOpts) (*agents.Result, } // 1. Project .mcp.json — create if absent, skip otherwise. - mcpAction, err := agents.WriteIfNotExists(w, filepath.Join(env.Root, ".mcp.json"), ProjectMCPJSON, opts) - if err != nil { - return res, fmt.Errorf(".mcp.json: %w", err) + // + // If gortex is already registered at user scope (~/.claude.json), a + // project .mcp.json adds a second registration under the same name. + // Claude Code keys OAuth tokens per endpoint and flags this as a + // "conflicting scopes" diagnostic. The user-scope entry already + // serves this repo machine-wide, so skip the project file unless + // --force. (A pre-existing .mcp.json is left in place — we never + // delete it — but we warn so the user can resolve the duplication.) + mcpPath := filepath.Join(env.Root, ".mcp.json") + if !opts.Force && env.Home != "" && userScopeGortexRegistered(env.Home) && !pathExists(mcpPath) { + logWarn(w, "gortex is already registered at user scope (%s); skipping project .mcp.json to avoid a Claude Code \"conflicting scopes\" warning. Re-run with --force to write it anyway (e.g. for teammates without a global install).", userClaudeJSONPath(env.Home)) + res.Files = append(res.Files, agents.FileAction{Path: mcpPath, Action: agents.ActionSkip, Reason: "gortex already registered at user scope"}) + } else { + if !opts.Force && env.Home != "" && userScopeGortexRegistered(env.Home) && pathExists(mcpPath) { + logWarn(w, "gortex is registered at both user scope (%s) and project scope (%s); Claude Code may warn about conflicting scopes — keep one with `claude mcp remove gortex -s user` or `-s project`.", userClaudeJSONPath(env.Home), mcpPath) + } + mcpAction, err := agents.WriteIfNotExists(w, mcpPath, ProjectMCPJSON, opts) + if err != nil { + return res, fmt.Errorf(".mcp.json: %w", err) + } + res.Files = append(res.Files, mcpAction) } - res.Files = append(res.Files, mcpAction) // 2. MCP permissions in .claude/settings.json — merge, not create. permAction, err := installPermissions(w, filepath.Join(env.Root, ".claude", "settings.json"), opts) @@ -602,23 +620,26 @@ func installGlobalSubAgents(w io.Writer, home string, opts agents.ApplyOpts) ([] // existing file is malformed JSON, it's backed up before we // overwrite. func upsertGlobalMCPConfig(w io.Writer, path string, opts agents.ApplyOpts) (agents.FileAction, error) { - exe, err := os.Executable() - if err != nil { - // Fall back to bare "gortex" on PATH. Reasonable for - // homebrew / go install deployments. - exe = "gortex" - } + // Prefer the bare "gortex" command when it resolves on PATH to the + // binary we're running, so this user-scope entry matches the + // portable project .mcp.json template byte-for-byte. Claude Code + // keys OAuth tokens per endpoint, so a user-scope stanza that + // disagrees with a project-scope one trips its "conflicting scopes" + // diagnostic. Falls back to the absolute path only when gortex is + // not on PATH (e.g. a Windows install whose dir isn't on PATH). entry := map[string]any{ - "command": exe, + "command": agents.ResolveGortexCommand(), "args": []string{"mcp"}, "env": map[string]any{}, } - // Try a direct merge first. MergeJSON handles malformed JSON - // with a timestamped backup already. + // Try a direct merge first. MergeJSON handles malformed JSON with a + // timestamped backup already. UpsertMCPServerWithMigration rewrites + // a stale Gortex-authored stanza (including the older absolute-path + // form) in place without clobbering a user's hand-rolled wrapper. action, err := agents.MergeJSON(w, path, func(root map[string]any, existed bool) (bool, error) { _ = existed - return agents.UpsertMCPServer(root, "gortex", entry, agents.ApplyOpts{Force: opts.Force}), nil + return agents.UpsertMCPServerWithMigration(root, "gortex", entry, agents.ApplyOpts{Force: opts.Force}), nil }, opts) if err != nil { return agents.FileAction{}, err @@ -637,6 +658,30 @@ func upsertGlobalMCPConfig(w io.Writer, path string, opts agents.ApplyOpts) (age return action, nil } +// userScopeGortexRegistered reports whether ~/.claude.json already +// registers a "gortex" MCP server at user scope. A project .mcp.json +// written on top of that produces a second registration under the same +// name, which Claude Code flags as a "conflicting scopes" warning +// because it stores OAuth tokens per endpoint. A missing or malformed +// file is treated as "not registered" — we only suppress the project +// write on positive evidence of a user-scope entry. +func userScopeGortexRegistered(home string) bool { + data, err := os.ReadFile(userClaudeJSONPath(home)) + if err != nil { + return false + } + var root map[string]any + if err := json.Unmarshal(data, &root); err != nil { + return false + } + servers, ok := root["mcpServers"].(map[string]any) + if !ok { + return false + } + _, ok = servers["gortex"] + return ok +} + // Paths — user-level files. func userClaudeJSONPath(home string) string { diff --git a/internal/agents/claudecode/adapter_test.go b/internal/agents/claudecode/adapter_test.go index a39610a0..fdb012dd 100644 --- a/internal/agents/claudecode/adapter_test.go +++ b/internal/agents/claudecode/adapter_test.go @@ -440,3 +440,65 @@ func TestClaudeCodeDryRunWritesNothing(t *testing.T) { } } } + +// TestProjectModeSkipsMCPWhenUserScopeRegistered is the regression +// guard for issue #201: a project `gortex init` must NOT create a +// project .mcp.json when gortex is already registered at user scope — +// that double-registration is what trips Claude Code's "conflicting +// scopes" diagnostic. --force overrides the skip (so a maintainer can +// still commit a .mcp.json for teammates without a global install). +func TestProjectModeSkipsMCPWhenUserScopeRegistered(t *testing.T) { + SetConfigDirOverride("") + + env, buf := agentstest.NewEnv(t) + a := New() + + // Seed a user-scope gortex registration in ~/.claude.json. + claudeJSON := userClaudeJSONPath(env.Home) + if err := os.MkdirAll(filepath.Dir(claudeJSON), 0o755); err != nil { + t.Fatalf("mkdir: %v", err) + } + seed := `{"mcpServers":{"gortex":{"command":"gortex","args":["mcp"],"env":{}}}}` + if err := os.WriteFile(claudeJSON, []byte(seed), 0o644); err != nil { + t.Fatalf("seed user config: %v", err) + } + + mcpPath := filepath.Join(env.Root, ".mcp.json") + + // Without --force: the project .mcp.json must be skipped and the + // user warned. + if _, err := a.Apply(env, agents.ApplyOpts{}); err != nil { + t.Fatalf("apply: %v", err) + } + if _, err := os.Stat(mcpPath); err == nil { + t.Errorf("project .mcp.json was written despite a user-scope gortex registration") + } + if !strings.Contains(buf.String(), "already registered at user scope") { + t.Errorf("expected a conflicting-scopes warning, got: %q", buf.String()) + } + + // With --force: the project .mcp.json is written anyway. + if _, err := a.Apply(env, agents.ApplyOpts{Force: true}); err != nil { + t.Fatalf("apply --force: %v", err) + } + if _, err := os.Stat(mcpPath); err != nil { + t.Errorf("--force should still write project .mcp.json: %v", err) + } +} + +// TestProjectModeWritesMCPWhenNoUserScope confirms the common path is +// unchanged: with no user-scope gortex registration, the project +// .mcp.json is created as before. +func TestProjectModeWritesMCPWhenNoUserScope(t *testing.T) { + SetConfigDirOverride("") + + env, _ := agentstest.NewEnv(t) + a := New() + + if _, err := a.Apply(env, agents.ApplyOpts{}); err != nil { + t.Fatalf("apply: %v", err) + } + if _, err := os.Stat(filepath.Join(env.Root, ".mcp.json")); err != nil { + t.Errorf("project .mcp.json should be written when no user-scope entry exists: %v", err) + } +} diff --git a/internal/agents/mcp.go b/internal/agents/mcp.go index 2ecabcb2..1c0b83d7 100644 --- a/internal/agents/mcp.go +++ b/internal/agents/mcp.go @@ -2,7 +2,11 @@ package agents import ( "encoding/json" + "os" + "os/exec" + "path/filepath" "reflect" + "strings" ) // UpsertMCPServer merges a gortex-flavored MCP server stanza into a @@ -79,17 +83,18 @@ func UpsertMCPServerWithMigration(root map[string]any, serverName string, entry } // IsGortexAuthoredMCPEntry returns true for MCP server stanzas that -// look like Gortex wrote them — `command == "gortex"` and the args -// list starts with the `mcp` subcommand. Used by global-mode -// installers to migrate their own legacy stanzas without clobbering -// user-customized servers. +// look like Gortex wrote them — a command naming the gortex binary +// (bare "gortex" or an absolute path whose basename is gortex) and an +// args list starting with the `mcp` subcommand. Used by global-mode +// installers to migrate their own legacy stanzas — including the older +// absolute-path form — without clobbering user-customized servers. func IsGortexAuthoredMCPEntry(entry any) bool { m, ok := entry.(map[string]any) if !ok { return false } cmd, _ := m["command"].(string) - if cmd != "gortex" { + if !commandIsGortex(cmd) { return false } args, ok := m["args"].([]any) @@ -100,6 +105,98 @@ func IsGortexAuthoredMCPEntry(entry any) bool { return first == "mcp" } +// commandIsGortex reports whether an MCP stanza's command string names +// the gortex binary — either the bare "gortex"/"gortex.exe" name or an +// absolute path whose basename is gortex (the legacy os.Executable() +// form a pre-fix `gortex install` baked into ~/.claude.json). Lets the +// global installer recognize and migrate its own older stanza in place +// instead of leaving a stale absolute path that disagrees with the +// bare-`gortex` project .mcp.json template. +func commandIsGortex(cmd string) bool { + return gortexCommandBase(cmd) == "gortex" +} + +// gortexCommandBase extracts the binary base name from a command +// string, tolerating both / and \ separators so a path written on one +// OS is still recognized when parsed on another (matters for +// cross-platform tests; in production the path is always native). The +// trailing extension (.exe on Windows) is stripped. +func gortexCommandBase(cmd string) string { + if cmd == "" { + return "" + } + base := cmd + if i := strings.LastIndexAny(base, `/\`); i >= 0 { + base = base[i+1:] + } + return strings.TrimSuffix(base, filepath.Ext(base)) +} + +// ResolveGortexCommand returns the command string an installer should +// bake into a gortex MCP server stanza. It prefers the bare "gortex" +// name — portable across machines and byte-identical to the project +// .mcp.json template — but only when "gortex" on PATH resolves to the +// same binary that is currently running. Matching the project template +// matters for Claude Code specifically: it stores OAuth tokens per +// endpoint (command + args), so a user-scope entry that disagrees with +// a project-scope entry trips its "conflicting scopes" diagnostic. +// +// When the running binary is not reachable on PATH (e.g. a Windows +// install whose program directory is not on PATH) it falls back to the +// absolute os.Executable() path so the entry still launches. Under +// `go run` (a transient temp build) or any other ambiguity it falls +// back to the bare name rather than bake a path that won't exist later. +func ResolveGortexCommand() string { + exe, exeErr := os.Executable() + lp, lpErr := exec.LookPath("gortex") + return resolveGortexCommandFrom(exe, exeErr, lp, lpErr, sameFile) +} + +// resolveGortexCommandFrom is the pure decision core of +// ResolveGortexCommand, split out so the PATH/executable inputs can be +// injected in tests. +func resolveGortexCommandFrom(exe string, exeErr error, lookPath string, lookErr error, same func(a, b string) bool) string { + exeUsable := exeErr == nil && exe != "" && gortexCommandBase(exe) == "gortex" && !isUnderTempDir(exe) + if lookErr == nil && lookPath != "" { + // On PATH: collapse to the bare name when it points at the + // binary we are running (or we cannot trust os.Executable), + // so the entry matches the portable project template. + if !exeUsable || same(lookPath, exe) { + return "gortex" + } + } + if exeUsable { + // Not on PATH, but we know exactly where we live: pin the + // absolute path so the entry launches. + return exe + } + return "gortex" +} + +// isUnderTempDir reports whether p lives under the OS temp directory — +// the tell-tale of a `go run` / `go test` transient build that must not +// be baked into a long-lived config. +func isUnderTempDir(p string) bool { + return strings.HasPrefix(p, filepath.Clean(os.TempDir())+string(os.PathSeparator)) +} + +// sameFile reports whether two paths reference the same on-disk binary, +// resolving symlinks and falling back to os.SameFile. A pure string +// match short-circuits the stat calls. +func sameFile(a, b string) bool { + if a == b { + return true + } + if ra, err := filepath.EvalSymlinks(a); err == nil { + if rb, err := filepath.EvalSymlinks(b); err == nil && ra == rb { + return true + } + } + fa, e1 := os.Stat(a) + fb, e2 := os.Stat(b) + return e1 == nil && e2 == nil && os.SameFile(fa, fb) +} + // MCPEntriesEqual compares two MCP stanzas by their JSON-marshaled // form. Round-tripping is the simplest way to handle the []string vs // []any drift between freshly-built entries and entries decoded from diff --git a/internal/agents/mcp_test.go b/internal/agents/mcp_test.go index 2105d557..9fdd159e 100644 --- a/internal/agents/mcp_test.go +++ b/internal/agents/mcp_test.go @@ -2,6 +2,9 @@ package agents import ( "encoding/json" + "errors" + "os" + "path/filepath" "testing" ) @@ -159,3 +162,116 @@ func TestUpsertMCPServerWithMigrationPreservesUserCustomization(t *testing.T) { t.Errorf("opts.Force must overwrite user-customized entry") } } + +// TestIsGortexAuthoredMCPEntryAbsolutePath verifies the broadened +// detection: the legacy absolute-path command form a pre-fix `gortex +// install` baked into ~/.claude.json (via os.Executable()) is now +// recognized as Gortex-authored, so the next install can migrate it to +// the canonical bare-`gortex` shape. A user's own wrapper script is +// still left alone. Both / and \ separators are handled regardless of +// the host OS. +func TestIsGortexAuthoredMCPEntryAbsolutePath(t *testing.T) { + cases := []struct { + name string + raw string + want bool + }{ + { + name: "unix absolute path to gortex (legacy os.Executable form)", + raw: `{"command":"/usr/local/bin/gortex","args":["mcp"]}`, + want: true, + }, + { + name: "windows absolute path to gortex.exe (the issue #201 form)", + raw: `{"command":"C:\\Users\\daoti\\AppData\\Local\\Programs\\gortex\\gortex.exe","args":["mcp"]}`, + want: true, + }, + { + name: "absolute path to a non-gortex binary", + raw: `{"command":"/usr/local/bin/something-else","args":["mcp"]}`, + want: false, + }, + { + name: "user wrapper whose basename merely contains gortex", + raw: `{"command":"/opt/scripts/my-gortex.sh","args":["mcp"]}`, + want: false, + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + var v any + if err := json.Unmarshal([]byte(tc.raw), &v); err != nil { + t.Fatalf("unmarshal: %v", err) + } + if got := IsGortexAuthoredMCPEntry(v); got != tc.want { + t.Errorf("IsGortexAuthoredMCPEntry(%s) = %v, want %v", tc.raw, got, tc.want) + } + }) + } +} + +// TestResolveGortexCommandFrom pins the command-resolution policy that +// keeps the user-scope ~/.claude.json entry in agreement with the +// project .mcp.json template (and so avoids Claude Code's "conflicting +// scopes" diagnostic) while staying robust when gortex isn't on PATH. +func TestResolveGortexCommandFrom(t *testing.T) { + sameTrue := func(a, b string) bool { return true } + sameFalse := func(a, b string) bool { return false } + notFound := errors.New("executable file not found in $PATH") + + cases := []struct { + name string + exe string + exeErr error + lookPath string + lookErr error + same func(a, b string) bool + want string + }{ + { + name: "on PATH and same binary -> bare gortex (matches project template)", + exe: "/usr/local/bin/gortex", + lookPath: "/usr/local/bin/gortex", + same: sameTrue, + want: "gortex", + }, + { + name: "installed but not on PATH -> pin absolute path", + exe: `C:\Users\daoti\AppData\Local\Programs\gortex\gortex.exe`, + lookPath: "", + lookErr: notFound, + same: sameFalse, + want: `C:\Users\daoti\AppData\Local\Programs\gortex\gortex.exe`, + }, + { + name: "on PATH but a different gortex -> pin the running binary, not the PATH one", + exe: "/opt/build/gortex", + lookPath: "/usr/local/bin/gortex", + same: sameFalse, + want: "/opt/build/gortex", + }, + { + name: "go run transient temp build + gortex on PATH -> bare gortex", + exe: filepath.Join(os.TempDir(), "go-build1234", "agents.test"), + lookPath: "/usr/local/bin/gortex", + same: sameFalse, + want: "gortex", + }, + { + name: "nothing resolvable -> bare gortex last resort", + exe: "", + exeErr: errors.New("os.Executable failed"), + lookErr: notFound, + same: sameFalse, + want: "gortex", + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got := resolveGortexCommandFrom(tc.exe, tc.exeErr, tc.lookPath, tc.lookErr, tc.same) + if got != tc.want { + t.Errorf("resolveGortexCommandFrom() = %q, want %q", got, tc.want) + } + }) + } +}