fix(setup): use absolute MCP paths on Windows#111
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes Windows MCP startup reliability by ensuring agent configurations use an absolute engram binary path (avoiding fragile PATH inheritance), and adds targeted tests to prevent regressions.
Changes:
- Add Windows-aware
resolveEngramCommand()and use it for Gemini CLI + Codex MCP registrations. - Write a durable Claude Code user-level MCP config at
~/.claude/mcp/engram.jsonduring setup. - Expand setup tests to cover Claude MCP config writing and Windows absolute-path behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| internal/setup/setup.go | Adds absolute-path resolution, durable Claude MCP config writing, and updates Gemini/Codex injection to use resolved command. |
| internal/setup/setup_test.go | Adds/updates tests for Claude user MCP config writing and Windows absolute-path expectations across agents. |
| cmd/engram/main.go | Updates Claude Code post-setup output messaging to mention the durable MCP config location. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fmt.Println(" 3. MCP config written to ~/.claude/mcp/engram.json using absolute binary path") | ||
| fmt.Println(" (survives plugin auto-updates; re-run 'engram setup claude-code' if you move the binary)") |
There was a problem hiding this comment.
The post-install message for Claude Code always claims the MCP config was written to ~/.claude/mcp/engram.json, but setup treats that write as non-fatal and can fail (it only warns to stderr). This can mislead users into thinking the durable config exists when it doesn’t. Consider passing the setup.Result into printPostInstall (or a boolean) and only printing this step when the user MCP config write actually succeeded (or wording it as “attempted/will be written if possible”).
| fmt.Println(" 3. MCP config written to ~/.claude/mcp/engram.json using absolute binary path") | |
| fmt.Println(" (survives plugin auto-updates; re-run 'engram setup claude-code' if you move the binary)") | |
| fmt.Println(" 3. Engram will attempt to write MCP config to ~/.claude/mcp/engram.json using the absolute binary path") | |
| fmt.Println(" (this config survives plugin auto-updates; re-run 'engram setup claude-code' if you move the binary, and verify the file exists)") |
| // claudeCodeMCPDir returns the directory for user-level Claude Code MCP configs. | ||
| // Files placed here are NOT managed by the plugin system and survive plugin updates. | ||
| func claudeCodeMCPDir() string { | ||
| home, _ := userHomeDir() | ||
| return filepath.Join(home, ".claude", "mcp") | ||
| } |
There was a problem hiding this comment.
claudeCodeMCPDir() ignores the error from os.UserHomeDir(). If home resolution fails, this will fall back to a relative path (".claude/mcp"), and writeClaudeCodeUserMCP() could end up creating/writing files in the current working directory. It would be safer to surface the error (e.g., return (string, error) and propagate) or at least guard against an empty home before using filepath.Join.
| // codexEngramBlock is the canonical Codex TOML MCP block. | ||
| // Command is always the bare "engram" name in this constant because | ||
| // upsertCodexEngramBlock generates the actual content via codexEngramBlockStr() | ||
| // which uses resolveEngramCommand() at runtime. This constant is kept for tests | ||
| // that verify idempotency against the already-written string. | ||
| const codexEngramBlock = "[mcp_servers.engram]\ncommand = \"engram\"\nargs = [\"mcp\", \"--tools=agent\"]" | ||
|
|
||
| // codexEngramBlockStr returns the Codex TOML block for the engram MCP server, | ||
| // using the resolved command (absolute path on Windows, bare name on Unix). | ||
| func codexEngramBlockStr() string { | ||
| cmd := resolveEngramCommand() | ||
| return "[mcp_servers.engram]\ncommand = " + fmt.Sprintf("%q", cmd) + "\nargs = [\"mcp\", \"--tools=agent\"]" | ||
| } |
There was a problem hiding this comment.
codexEngramBlockStr() makes the Codex MCP block OS-dependent (absolute path on Windows), but the file still keeps codexEngramBlock as a single canonical constant used by tests for exact string equality. This can make those tests fail on Windows (where upsertCodexEngramBlock() now returns a different block). Consider updating any exact-string expectations to use codexEngramBlockStr() (or resolveEngramCommand()) instead of codexEngramBlock so tests are portable across GOOS.
Linked Issue
Closes #100
Summary
engrambinary pathProblem
On Windows, MCP subprocesses do not reliably inherit the PATH that contains
engram.exe. The Claude Code plugin cache also overwrites manual fixes to.mcp.json, so the user-side workaround is fragile and recurring.Fix
This change resolves the command path from
os.Executable()on Windows and writes a user-owned Claude MCP config under~/.claude/mcp/engram.json, which survives plugin updates. Gemini CLI and Codex now also use absolute command paths on Windows.Test Plan
go test ./cmd/engram ./internal/setup ./internal/mcp ./internal/server ./internal/store ./internal/sync ./internal/tui ./internal/version