fix(setup): bake OpenCode plugin binary fallback#115
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates Engram’s setup/install flow to make OpenCode’s plugin and MCP invocation resilient in headless environments by ensuring the plugin can reliably locate the engram binary without depending on inherited PATH.
Changes:
- Patch the installed OpenCode plugin (
engram.ts) to resolveENGRAM_BINvia env override →Bun.which("engram")→ baked-in absolute path. - Change
resolveEngramCommand()to returnos.Executable()on all platforms (falling back to bareengramonly ifos.Executable()fails). - Expand and adjust setup tests to cover the updated resolution chain and installation behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| internal/setup/setup.go | Adds plugin patching for ENGRAM_BIN and updates command resolution to use os.Executable() cross-platform. |
| internal/setup/setup_test.go | Updates and adds tests for absolute-path command resolution and OpenCode plugin patching behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Priority (left to right, first truthy wins): | ||
| // 1. ENGRAM_BIN env var — explicit user override, always respected. | ||
| // 2. Bun.which("engram") — runtime PATH lookup; works in interactive shells. | ||
| // 3. Absolute baked-in path — works in headless/systemd where PATH is stripped. | ||
| // | ||
| // If absBin is already bare "engram" (os.Executable fallback) we don't add it | ||
| // as the third fallback because it would be redundant with Bun.which("engram"). |
There was a problem hiding this comment.
The comment says “first truthy wins”, but the patched TypeScript line uses the nullish coalescing operator (??), which selects the first non-nullish value (null/undefined), not the first truthy value. This matters because process.env.ENGRAM_BIN = "" would currently override the fallbacks. Please update the comment wording (and the bullet list) to reflect ?? semantics, or change the JS operator if you intend truthiness-based fallback.
| if cmd == "engram" { | ||
| t.Fatalf("expected absolute path from os.Executable(), got bare 'engram'") |
There was a problem hiding this comment.
The comment says this verifies an “absolute path”, but the assertions only check non-empty and != "engram". Consider either (a) adding an actual absolute-path assertion (e.g., filepath.IsAbs(cmd) on the current OS), or (b) adjusting the comment to match what’s being asserted.
| if cmd == "engram" { | |
| t.Fatalf("expected absolute path from os.Executable(), got bare 'engram'") | |
| if !filepath.IsAbs(cmd) { | |
| t.Fatalf("expected absolute path from os.Executable(), got %q", cmd) |
| // will be the real absolute path in tests, not bare "engram". | ||
| if !strings.Contains(text, "command = ") || !strings.Contains(text, "engram") { |
There was a problem hiding this comment.
This test is currently non-deterministic / likely to fail: resolveEngramCommand() uses os.Executable(), so under go test the command string will typically be the test binary path (e.g. .../setup.test) which often does not contain "engram". To make this stable, stub osExecutable in this test (as you do elsewhere) and assert against that stubbed value (or at least assert on properties of the stubbed path).
| // will be the real absolute path in tests, not bare "engram". | |
| if !strings.Contains(text, "command = ") || !strings.Contains(text, "engram") { | |
| // will be the real absolute path in tests, so we only assert that a command | |
| // entry is present, not on the executable name itself. | |
| if !strings.Contains(text, "command = ") { |
| // truthy process.env.ENGRAM_BIN short-circuits before Bun.which and the | ||
| // baked-in path. This is the JavaScript ?? semantics guarantee. |
There was a problem hiding this comment.
The explanation here refers to a “truthy” env var short-circuiting, but the plugin code uses the nullish coalescing operator (??). With ??, an empty string is not treated as missing, so ENGRAM_BIN="" would still win and break spawning. Update the comment to say “non-nullish (not null/undefined)” (or consider using || if you actually want empty-string to fall through).
| // truthy process.env.ENGRAM_BIN short-circuits before Bun.which and the | |
| // baked-in path. This is the JavaScript ?? semantics guarantee. | |
| // non-nullish (not null/undefined) process.env.ENGRAM_BIN short-circuits | |
| // before Bun.which and the baked-in path, per JavaScript ?? semantics. |
Linked Issue
Closes #113
Summary
engram serveworks in headless environmentsresolveEngramCommand()return the executable path on all platforms instead of assuming Unix PATH is always reliableENGRAM_BINresolution chain and setup behaviorProblem
After fixing MCP path resolution, the remaining OpenCode headless gap was the plugin runtime itself. The installed
engram.tsstill relied onprocess.env.ENGRAM_BIN ?? "engram", which breaks in systemd or other headless environments when PATH does not include the binary location.Fix
The installed plugin copy now uses this precedence chain:
process.env.ENGRAM_BINBun.which("engram")os.Executable()This preserves user override behavior while making headless setups work without manual service edits in the common case.
Test Plan
go test ./internal/setup ./cmd/engram ./internal/mcp ./internal/server ./internal/store ./internal/sync ./internal/tui ./internal/version