diff --git a/internal/detector/agent.go b/internal/detector/agent.go index 8940c01..ba68c72 100644 --- a/internal/detector/agent.go +++ b/internal/detector/agent.go @@ -13,18 +13,18 @@ import ( ) type agentSpec struct { - Name string - Vendor string - DetectionPaths []string // relative to home dir - Binaries []string + Name string + Vendor string + ConfigDirs []string // candidate config directories (relative to home or absolute, ~ expanded). Used as ConfigDir when populated alongside a discovered binary; never used as the sole proof of installation. + Binaries []string // binary names to search via LookPath, or home-relative paths (~/...). At least one must resolve before the agent is considered installed. } var agentDefinitions = []agentSpec{ - {"openclaw", "OpenSource", []string{".openclaw"}, []string{"openclaw"}}, - {"clawdbot", "OpenSource", []string{".clawdbot"}, []string{"clawdbot"}}, - {"moltbot", "OpenSource", []string{".moltbot"}, []string{"moltbot"}}, - {"moldbot", "OpenSource", []string{".moldbot"}, []string{"moldbot"}}, - {"gpt-engineer", "OpenSource", []string{".gpt-engineer"}, []string{"gpt-engineer"}}, + {"openclaw", "OpenSource", []string{"~/.openclaw"}, []string{"openclaw", "~/.local/bin/openclaw"}}, + {"clawdbot", "OpenSource", []string{"~/.clawdbot"}, []string{"clawdbot", "~/.local/bin/clawdbot"}}, + {"moltbot", "OpenSource", []string{"~/.moltbot"}, []string{"moltbot", "~/.local/bin/moltbot"}}, + {"moldbot", "OpenSource", []string{"~/.moldbot"}, []string{"moldbot", "~/.local/bin/moldbot"}}, + {"gpt-engineer", "OpenSource", []string{"~/.gpt-engineer"}, []string{"gpt-engineer", "~/.local/bin/gpt-engineer"}}, } // AgentDetector detects general-purpose AI agents. @@ -41,19 +41,27 @@ func (d *AgentDetector) Detect(ctx context.Context, searchDirs []string) []model var results []model.AITool for _, spec := range agentDefinitions { - installPath, found := d.findAgent(spec, homeDir) + binaryPath, found := d.findAgentBinary(spec, homeDir) if !found { continue } - version := d.getVersion(ctx, spec) + // Prefer the resolved real path (and, for npm-packaged agents, the + // package root) as install_path so investigators can find the actual + // package contents rather than the shim on PATH. + installPath := resolveInstallPath(d.exec, binaryPath) + + configDir := d.findConfigDir(spec, homeDir) + version := d.getVersion(ctx, binaryPath) results = append(results, model.AITool{ Name: spec.Name, Vendor: spec.Vendor, Type: "general_agent", Version: version, + BinaryPath: binaryPath, InstallPath: installPath, + ConfigDir: configDir, }) } @@ -65,41 +73,63 @@ func (d *AgentDetector) Detect(ctx context.Context, searchDirs []string) []model return results } -func (d *AgentDetector) findAgent(spec agentSpec, homeDir string) (string, bool) { - // Check detection paths - for _, relPath := range spec.DetectionPaths { - fullPath := filepath.Join(homeDir, relPath) - if d.exec.DirExists(fullPath) || d.exec.FileExists(fullPath) { - return fullPath, true - } - } - - // Check binaries in PATH +// findAgentBinary searches for the agent's binary, treating any "~/..." +// entry as a home-relative file path and any other entry as a PATH-resolvable +// binary name. Returns the discovered path and a found flag. +// +// Existence of a config dir alone is intentionally NOT treated as proof of +// installation: an empty ~/.openclaw/ directory doesn't mean the openclaw +// agent is installed, and treating it as such produces phantom entries on +// machines that have stale dotfiles. +func (d *AgentDetector) findAgentBinary(spec agentSpec, homeDir string) (string, bool) { for _, bin := range spec.Binaries { - if path, err := d.exec.LookPath(bin); err == nil { + expanded := expandTilde(bin, homeDir) + if expanded != bin { + // Home-relative: must exist on disk as a file. + if d.exec.FileExists(expanded) { + return expanded, true + } + if d.exec.GOOS() == model.PlatformWindows && !strings.HasSuffix(expanded, ".exe") { + if d.exec.FileExists(expanded + ".exe") { + return expanded + ".exe", true + } + } + continue + } + if path, err := d.exec.LookPath(expanded); err == nil { return path, true } } - return "", false } -func (d *AgentDetector) getVersion(ctx context.Context, spec agentSpec) string { - for _, bin := range spec.Binaries { - if _, err := d.exec.LookPath(bin); err == nil { - stdout, _, _, err := d.exec.RunWithTimeout(ctx, 10*time.Second, bin, "--version") - if err == nil { - lines := strings.SplitN(stdout, "\n", 2) - if len(lines) > 0 { - v := strings.TrimSpace(lines[0]) - if v != "" { - return v - } - } - } +// findConfigDir returns the first config directory candidate that exists and +// is non-empty. An entirely empty directory is treated as "no config dir": +// stale dotfiles shouldn't get surfaced as a real config location. We don't +// inspect file sizes here — the binary-on-PATH requirement in +// findAgentBinary is what defends against false positives like an empty +// config.json. This check is purely cosmetic (whether to populate the field). +func (d *AgentDetector) findConfigDir(spec agentSpec, homeDir string) string { + for _, dir := range spec.ConfigDirs { + expanded := expandTilde(dir, homeDir) + if !d.exec.DirExists(expanded) { + continue } + entries, err := d.exec.ReadDir(expanded) + if err != nil || len(entries) == 0 { + continue + } + return expanded + } + return "" +} + +func (d *AgentDetector) getVersion(ctx context.Context, binaryPath string) string { + stdout, _, _, err := d.exec.RunWithTimeout(ctx, 10*time.Second, binaryPath, "--version") + if err != nil { + return "unknown" } - return "unknown" + return extractVersionFromOutput(stdout) } // detectClaudeCowork checks for Claude Cowork (a mode within Claude Desktop 0.7+). diff --git a/internal/detector/agent_test.go b/internal/detector/agent_test.go index 949e368..25eac9d 100644 --- a/internal/detector/agent_test.go +++ b/internal/detector/agent_test.go @@ -2,37 +2,132 @@ package detector import ( "context" + "os" "testing" "github.com/step-security/dev-machine-guard/internal/executor" + "github.com/step-security/dev-machine-guard/internal/model" ) func TestAgentDetector_FindsOpenclaw(t *testing.T) { mock := executor.NewMock() + binaryPath := "/usr/local/bin/openclaw" + mock.SetPath("openclaw", binaryPath) + mock.SetCommand("0.5.2\n", "", 0, binaryPath, "--version") + // Non-empty config dir. mock.SetDir("/Users/testuser/.openclaw") - mock.SetPath("openclaw", "/usr/local/bin/openclaw") - mock.SetCommand("0.5.2\n", "", 0, "openclaw", "--version") + mock.SetDirEntries("/Users/testuser/.openclaw", []os.DirEntry{ + executor.MockDirEntry("config.toml", false), + }) + mock.SetFile("/Users/testuser/.openclaw/config.toml", []byte("[settings]\nfoo = 1")) + + det := NewAgentDetector(mock) + results := det.Detect(context.Background(), []string{"/Users/testuser"}) + + var got *agentResult + for i, r := range results { + if r.Name == "openclaw" { + got = &agentResult{r} + _ = i + } + } + if got == nil { + t.Fatal("openclaw not found") + } + if got.Type != "general_agent" { + t.Errorf("expected general_agent, got %s", got.Type) + } + if got.BinaryPath != binaryPath { + t.Errorf("expected binary_path %s, got %s", binaryPath, got.BinaryPath) + } + if got.InstallPath != binaryPath { + t.Errorf("expected install_path %s (resolved real path), got %s", binaryPath, got.InstallPath) + } + if got.ConfigDir != "/Users/testuser/.openclaw" { + t.Errorf("expected config_dir /Users/testuser/.openclaw, got %s", got.ConfigDir) + } + if got.Version != "0.5.2" { + t.Errorf("expected version 0.5.2, got %s", got.Version) + } +} + +// TestAgentDetector_NoFalsePositive_EmptyConfigDir asserts that an empty +// "~/.openclaw" left over from an uninstall (or a fixture) does NOT trick the +// detector into reporting openclaw as installed when the binary isn't on PATH. +// See bug 0001. +func TestAgentDetector_NoFalsePositive_EmptyConfigDir(t *testing.T) { + mock := executor.NewMock() + // Empty config dir present, but no binary anywhere. + mock.SetDir("/Users/testuser/.openclaw") + mock.SetDirEntries("/Users/testuser/.openclaw", []os.DirEntry{}) det := NewAgentDetector(mock) results := det.Detect(context.Background(), []string{"/Users/testuser"}) - found := false for _, r := range results { if r.Name == "openclaw" { - found = true - if r.Type != "general_agent" { - t.Errorf("expected general_agent, got %s", r.Type) - } - if r.InstallPath != "/Users/testuser/.openclaw" { - t.Errorf("expected /Users/testuser/.openclaw, got %s", r.InstallPath) - } + t.Errorf("openclaw should not be detected from an empty config dir alone; got %+v", r) } } - if !found { - t.Error("openclaw not found") +} + +// TestAgentDetector_NoFalsePositive_EmptyConfigFile asserts that a directory +// containing only a zero-byte config file (the actual fixture observed on the +// Linux test VM that triggered bug 0001) does not produce a phantom detection. +func TestAgentDetector_NoFalsePositive_EmptyConfigFile(t *testing.T) { + mock := executor.NewMock() + mock.SetDir("/Users/testuser/.openclaw") + mock.SetDirEntries("/Users/testuser/.openclaw", []os.DirEntry{ + executor.MockDirEntry("config.json", false), + }) + mock.SetFile("/Users/testuser/.openclaw/config.json", []byte{}) // size=0 + + det := NewAgentDetector(mock) + results := det.Detect(context.Background(), []string{"/Users/testuser"}) + + for _, r := range results { + if r.Name == "openclaw" { + t.Errorf("openclaw should not be detected from an empty config.json; got %+v", r) + } + } +} + +// TestAgentDetector_ResolvesNpmInstallPath asserts that an agent installed via +// npm (binary is a symlink into node_modules) reports the package root as +// install_path instead of the shim path. +func TestAgentDetector_ResolvesNpmInstallPath(t *testing.T) { + mock := executor.NewMock() + shim := "/usr/local/bin/openclaw" + target := "/usr/local/lib/node_modules/openclaw/bin/openclaw.js" + pkgRoot := "/usr/local/lib/node_modules/openclaw" + mock.SetPath("openclaw", shim) + mock.SetSymlink(shim, target) + mock.SetCommand("0.5.2\n", "", 0, shim, "--version") + + det := NewAgentDetector(mock) + results := det.Detect(context.Background(), []string{"/Users/testuser"}) + + var got *agentResult + for _, r := range results { + if r.Name == "openclaw" { + r := r + got = &agentResult{r} + } + } + if got == nil { + t.Fatal("openclaw not found") + } + if got.BinaryPath != shim { + t.Errorf("expected binary_path %s, got %s", shim, got.BinaryPath) + } + if got.InstallPath != pkgRoot { + t.Errorf("expected install_path %s (npm package root), got %s", pkgRoot, got.InstallPath) } } +// agentResult is a tiny helper alias to keep test reads clean. +type agentResult struct{ model.AITool } + func TestAgentDetector_ClaudeCowork(t *testing.T) { mock := executor.NewMock() mock.SetDir("/Applications/Claude.app") diff --git a/internal/detector/aicli.go b/internal/detector/aicli.go index a28fc4d..2cb698a 100644 --- a/internal/detector/aicli.go +++ b/internal/detector/aicli.go @@ -127,20 +127,143 @@ func (d *AICLIDetector) Detect(ctx context.Context) []model.AITool { version := d.getVersion(ctx, spec, binaryPath) configDir := d.findConfigDir(spec, homeDir) + installPath := resolveInstallPath(d.exec, binaryPath) results = append(results, model.AITool{ - Name: spec.Name, - Vendor: spec.Vendor, - Type: "cli_tool", - Version: version, - BinaryPath: binaryPath, - ConfigDir: configDir, + Name: spec.Name, + Vendor: spec.Vendor, + Type: "cli_tool", + Version: version, + BinaryPath: binaryPath, + InstallPath: installPath, + ConfigDir: configDir, }) } return results } +// resolveInstallPath returns the on-disk install root for a CLI tool, given +// the binary path that was found via PATH or a home-relative lookup. +// +// Many AI CLIs (claude-code, codex, opencode) ship as npm packages whose +// binary is exposed as a tiny shim under /usr/local/bin/. The shim's symlink +// target lives inside `node_modules///...` — that directory +// is what an investigator actually wants when they ask "where is claude +// installed?". When we detect that pattern, return the package root. +// +// If symlink resolution fails or the resolved path doesn't sit inside a +// node_modules tree, return the resolved real path (or the original path if +// resolution failed) so we still surface a meaningful install location +// instead of leaving the field empty. +func resolveInstallPath(exec executor.Executor, binaryPath string) string { + if binaryPath == "" { + return "" + } + resolved, err := exec.EvalSymlinks(binaryPath) + if err != nil || resolved == "" { + resolved = binaryPath + } + if pkgRoot := nodeModulesPackageRoot(resolved); pkgRoot != "" { + return pkgRoot + } + // Windows: npm publishes a `.cmd` (and `.ps1`) shim instead of a symlink, + // so the resolved path points at the shim itself, not the package. Parse + // the shim to recover the node_modules package root. + if pkgRoot := npmShimPackageRoot(exec, resolved); pkgRoot != "" { + return pkgRoot + } + return resolved +} + +// npmShimPackageRoot reads a Windows-style npm shim (`.cmd`, +// `.ps1`, `.bat`) and returns the install root of the package the +// shim invokes, by locating the first `node_modules\<...>` reference in the +// shim body. Returns "" when path isn't a shim, can't be read, or contains +// no node_modules reference. +// +// npm's Windows shim, generated by cmd-shim, looks like: +// +// "%_prog%" "%dp0%\node_modules\@anthropic-ai\claude-code\cli.js" %* +// +// We extract the node_modules path, resolve it relative to the shim's own +// directory (cmd-shim's `%dp0%`), and feed the absolute path through +// nodeModulesPackageRoot. +func npmShimPackageRoot(exec executor.Executor, path string) string { + lower := strings.ToLower(path) + if !strings.HasSuffix(lower, ".cmd") && !strings.HasSuffix(lower, ".ps1") && !strings.HasSuffix(lower, ".bat") { + return "" + } + data, err := exec.ReadFile(path) + if err != nil { + return "" + } + content := string(data) + idx := strings.Index(content, "node_modules") + if idx < 0 { + return "" + } + rest := content[idx:] + if end := strings.IndexAny(rest, "\"' \t\r\n"); end > 0 { + rest = rest[:end] + } + // Resolve relative to the shim's own directory, separator-agnostic so + // the test path-parser works the same on a Windows host and a Mac host. + sep := pathSeparator(path) + shimDir := path + for i := len(path) - 1; i >= 0; i-- { + if path[i] == '/' || path[i] == '\\' { + shimDir = path[:i] + break + } + } + rest = strings.TrimLeft(rest, "/\\") + return nodeModulesPackageRoot(shimDir + sep + rest) +} + +// nodeModulesPackageRoot walks up `path` looking for a `node_modules` +// directory; if found, returns the package root (one or two levels deeper +// depending on whether the package is scoped). Returns "" when the path +// isn't inside a node_modules tree. +// +// Separator-agnostic: handles both `/` and `\` paths regardless of the host +// OS. The returned path uses the same separator style as the input — Windows +// paths preserve backslashes, Unix paths preserve forward slashes. +// +// Examples: +// /usr/local/lib/node_modules/@anthropic-ai/claude-code/bin/claude.exe +// -> /usr/local/lib/node_modules/@anthropic-ai/claude-code +// C:\Users\u\AppData\Roaming\npm\node_modules\@scope\name\cli.js +// -> C:\Users\u\AppData\Roaming\npm\node_modules\@scope\name +func nodeModulesPackageRoot(path string) string { + sep := pathSeparator(path) + norm := strings.ReplaceAll(path, "\\", "/") + parts := strings.Split(norm, "/") + for i := len(parts) - 1; i >= 0; i-- { + if parts[i] != "node_modules" { + continue + } + if i+1 >= len(parts) { + return "" + } + // Scoped package (`@scope/name`): take two segments past node_modules. + if strings.HasPrefix(parts[i+1], "@") && i+2 < len(parts) { + return strings.Join(parts[:i+3], sep) + } + return strings.Join(parts[:i+2], sep) + } + return "" +} + +// pathSeparator picks the separator style of an input path. If both styles +// are present (rare), `/` wins because it's portable. +func pathSeparator(path string) string { + if strings.Contains(path, "\\") && !strings.Contains(path, "/") { + return "\\" + } + return "/" +} + func (d *AICLIDetector) findBinary(ctx context.Context, spec cliToolSpec, homeDir string) (string, bool) { for _, bin := range spec.Binaries { expanded := expandTilde(bin, homeDir) @@ -174,11 +297,25 @@ func (d *AICLIDetector) getVersion(ctx context.Context, spec cliToolSpec, binary if err != nil { return "unknown" } - lines := strings.SplitN(stdout, "\n", 2) - if len(lines) > 0 { - v := strings.TrimSpace(lines[0]) - if v != "" { - return cleanVersionString(v) + return extractVersionFromOutput(stdout) +} + +// extractVersionFromOutput finds the first line of `--version` output that +// contains a version-shaped token, then returns that token. +// +// Tools that talk to a daemon (ollama, lm-studio CLI) prepend warnings to +// their version output when the daemon isn't running, so we can't rely on the +// first line. Walking line-by-line and skipping lines without a version token +// keeps real version output ("codex-cli 0.118.0", "aider 0.86.2") working +// while making the detector robust against decorated output. +func extractVersionFromOutput(stdout string) string { + for _, line := range strings.Split(stdout, "\n") { + line = strings.TrimSpace(line) + if line == "" { + continue + } + if v := cleanVersionString(line); v != "unknown" { + return v } } return "unknown" diff --git a/internal/detector/aicli_test.go b/internal/detector/aicli_test.go index 91e1bc8..62be3e1 100644 --- a/internal/detector/aicli_test.go +++ b/internal/detector/aicli_test.go @@ -5,6 +5,7 @@ import ( "testing" "github.com/step-security/dev-machine-guard/internal/executor" + "github.com/step-security/dev-machine-guard/internal/model" ) func TestAICLIDetector_FindsClaude(t *testing.T) { @@ -164,6 +165,197 @@ func TestAICLIDetector_FindsCursorAgent(t *testing.T) { } } +// TestAICLIDetector_ResolvesNpmInstallPath asserts that when the binary on +// PATH is a symlink to a node_modules package (the standard layout for +// claude-code, codex, opencode, etc.), the detector surfaces both the shim +// (binary_path) and the package root (install_path). See bug 0001. +func TestAICLIDetector_ResolvesNpmInstallPath(t *testing.T) { + mock := executor.NewMock() + shim := "/usr/local/bin/claude" + target := "/usr/local/lib/node_modules/@anthropic-ai/claude-code/bin/claude.exe" + pkgRoot := "/usr/local/lib/node_modules/@anthropic-ai/claude-code" + mock.SetPath("claude", shim) + mock.SetSymlink(shim, target) + mock.SetCommand("2.1.117 (Claude Code)\n", "", 0, shim, "--version") + mock.SetDir("/Users/testuser/.claude") + + det := NewAICLIDetector(mock) + results := det.Detect(context.Background()) + + var got *model.AITool + for i, r := range results { + if r.Name == "claude-code" { + got = &results[i] + break + } + } + if got == nil { + t.Fatal("claude-code not found") + } + if got.BinaryPath != shim { + t.Errorf("expected binary_path %s, got %s", shim, got.BinaryPath) + } + if got.InstallPath != pkgRoot { + t.Errorf("expected install_path %s (npm package root), got %s", pkgRoot, got.InstallPath) + } + if got.Version != "2.1.117" { + t.Errorf("expected version 2.1.117 (extractVersionFromOutput should strip the suffix), got %s", got.Version) + } +} + +// TestAICLIDetector_NonSymlinkInstallPath asserts that when the PATH binary +// is not a symlink, install_path equals the binary path itself rather than +// being left empty. +func TestAICLIDetector_NonSymlinkInstallPath(t *testing.T) { + mock := executor.NewMock() + bin := "/usr/local/bin/aider" + mock.SetPath("aider", bin) + mock.SetCommand("aider 0.86.2\n", "", 0, bin, "--version") + // No SetSymlink: EvalSymlinks returns the path unchanged. + + det := NewAICLIDetector(mock) + results := det.Detect(context.Background()) + + var got *model.AITool + for i, r := range results { + if r.Name == "aider" { + got = &results[i] + break + } + } + if got == nil { + t.Fatal("aider not found") + } + if got.InstallPath != bin { + t.Errorf("expected install_path %s (resolved real path == binary), got %s", bin, got.InstallPath) + } +} + +// TestAICLIDetector_ResolvesNpmShimOnWindows asserts that on Windows, where +// npm installs `.cmd` shims rather than symlinks, the detector still surfaces +// the node_modules package root as install_path by parsing the shim. +func TestAICLIDetector_ResolvesNpmShimOnWindows(t *testing.T) { + mock := executor.NewMock() + mock.SetGOOS("windows") + shim := `C:\Users\Administrator\AppData\Roaming\npm\claude.cmd` + mock.SetPath("claude", shim) + // cmd-shim layout: the shim references node_modules\\\cli.js + // relative to its own directory (%dp0%). + shimBody := `@ECHO off +GOTO start +:find_dp0 +SET dp0=%~dp0 +EXIT /b +:start +SETLOCAL +CALL :find_dp0 + +IF EXIST "%dp0%\node.exe" ( + SET "_prog=%dp0%\node.exe" +) ELSE ( + SET "_prog=node" + SET PATHEXT=%PATHEXT:;.JS;=;% +) + +endLocal & goto #_undefined_# 2>NUL || title %COMSPEC% & "%_prog%" "%dp0%\node_modules\@anthropic-ai\claude-code\cli.js" %* +` + mock.SetFile(shim, []byte(shimBody)) + mock.SetCommand("2.1.98 (Claude Code)\n", "", 0, shim, "--version") + + det := NewAICLIDetector(mock) + results := det.Detect(context.Background()) + + var got *model.AITool + for i, r := range results { + if r.Name == "claude-code" { + got = &results[i] + break + } + } + if got == nil { + t.Fatal("claude-code not found") + } + wantInstall := `C:\Users\Administrator\AppData\Roaming\npm\node_modules\@anthropic-ai\claude-code` + if got.InstallPath != wantInstall { + t.Errorf("expected install_path %s (parsed from .cmd shim), got %s", wantInstall, got.InstallPath) + } + if got.BinaryPath != shim { + t.Errorf("expected binary_path %s, got %s", shim, got.BinaryPath) + } +} + +// TestNodeModulesPackageRoot exercises the npm package-root extractor +// directly. The resolveInstallPath wrapper depends on this for both the AI +// CLI detector and the general-agent detector. +func TestNodeModulesPackageRoot(t *testing.T) { + tests := []struct { + path string + want string + }{ + {"/usr/local/lib/node_modules/@anthropic-ai/claude-code/bin/claude.exe", "/usr/local/lib/node_modules/@anthropic-ai/claude-code"}, + {"/usr/local/lib/node_modules/@openai/codex/bin/codex.js", "/usr/local/lib/node_modules/@openai/codex"}, + {"/home/u/.npm-global/lib/node_modules/opencode/bin/opencode", "/home/u/.npm-global/lib/node_modules/opencode"}, + {"/usr/bin/ollama", ""}, // not a node_modules path + {"/Users/u/Library/foo/node_modules", ""}, // node_modules with no package after + {"", ""}, + } + for _, tt := range tests { + got := nodeModulesPackageRoot(tt.path) + if got != tt.want { + t.Errorf("nodeModulesPackageRoot(%q) = %q, want %q", tt.path, got, tt.want) + } + } +} + +// TestExtractVersionFromOutput asserts that decorated `--version` output +// (notably ollama warnings emitted before the version line) still yields the +// real version. See bug 0001 F3. +func TestExtractVersionFromOutput(t *testing.T) { + tests := []struct { + name string + stdout string + want string + }{ + { + name: "ollama warnings before version", + stdout: "Warning: could not connect to a running Ollama instance\nWarning: client version is 0.0.0\n", + want: "0.0.0", + }, + { + name: "single-line plain version", + stdout: "0.5.4\n", + want: "0.5.4", + }, + { + name: "tool-name prefix", + stdout: "codex-cli 0.118.0\n", + want: "0.118.0", + }, + { + name: "v-prefix preserved", + stdout: "v1.2.3\n", + want: "v1.2.3", + }, + { + name: "all-noise no version token", + stdout: "Hello world\nGoodbye\n", + want: "unknown", + }, + { + name: "empty", + stdout: "", + want: "unknown", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := extractVersionFromOutput(tt.stdout); got != tt.want { + t.Errorf("extractVersionFromOutput(%q) = %q, want %q", tt.stdout, got, tt.want) + } + }) + } +} + func TestAICLIDetector_FindsCursorAgentInLocalBin(t *testing.T) { // Binary is not on PATH, but the official installer's symlink at // ~/.local/bin/cursor-agent exists. The home-relative fallback should pick it up. diff --git a/internal/detector/framework.go b/internal/detector/framework.go index 998512a..b75c123 100644 --- a/internal/detector/framework.go +++ b/internal/detector/framework.go @@ -3,7 +3,6 @@ package detector import ( "context" "path/filepath" - "strings" "time" "github.com/step-security/dev-machine-guard/internal/executor" @@ -77,14 +76,7 @@ func (d *FrameworkDetector) getVersion(ctx context.Context, binaryPath string) s if err != nil { return "unknown" } - lines := strings.SplitN(stdout, "\n", 2) - if len(lines) > 0 { - v := strings.TrimSpace(lines[0]) - if v != "" { - return v - } - } - return "unknown" + return extractVersionFromOutput(stdout) } func (d *FrameworkDetector) detectLMStudioApp(ctx context.Context) (model.AITool, bool) { diff --git a/internal/detector/framework_test.go b/internal/detector/framework_test.go index 9ce4d91..dd0396e 100644 --- a/internal/detector/framework_test.go +++ b/internal/detector/framework_test.go @@ -33,6 +33,36 @@ func TestFrameworkDetector_FindsOllama(t *testing.T) { } } +// TestFrameworkDetector_OllamaVersionWithWarnings asserts that the framework +// detector strips warning lines that ollama prints when its daemon isn't +// running and still surfaces the real version. See bug 0001 F3. +func TestFrameworkDetector_OllamaVersionWithWarnings(t *testing.T) { + mock := executor.NewMock() + mock.SetPath("ollama", "/usr/bin/ollama") + mock.SetCommand( + "Warning: could not connect to a running Ollama instance\nWarning: client version is 0.0.0\n", + "", 0, + "/usr/bin/ollama", "--version", + ) + mock.SetCommand("", "", 1, "pgrep", "-x", "ollama") // not running + + det := NewFrameworkDetector(mock) + results := det.Detect(context.Background()) + + found := false + for _, r := range results { + if r.Name == "ollama" { + found = true + if r.Version != "0.0.0" { + t.Errorf("expected version 0.0.0 (extracted from second 'Warning:' line), got %q", r.Version) + } + } + } + if !found { + t.Error("ollama not found") + } +} + func TestFrameworkDetector_NotRunning(t *testing.T) { mock := executor.NewMock() mock.SetPath("ollama", "/usr/local/bin/ollama") diff --git a/internal/executor/executor.go b/internal/executor/executor.go index 70a20d4..0068625 100644 --- a/internal/executor/executor.go +++ b/internal/executor/executor.go @@ -49,6 +49,9 @@ type Executor interface { HomeDir(username string) (string, error) // Glob returns filenames matching a pattern. Glob(pattern string) ([]string, error) + // EvalSymlinks resolves symbolic links in a path. Returns the resolved + // canonical path. If the path is not a symlink, returns it unchanged. + EvalSymlinks(path string) (string, error) // LoggedInUser returns the actual logged-in console user. // When running as root on macOS (e.g., via LaunchDaemon), this detects the // real console user via /dev/console rather than returning root. @@ -163,6 +166,10 @@ func (r *Real) Glob(pattern string) ([]string, error) { return filepath.Glob(pattern) } +func (r *Real) EvalSymlinks(path string) (string, error) { + return filepath.EvalSymlinks(path) +} + func (r *Real) LoggedInUser() (*user.User, error) { if runtime.GOOS != "darwin" || !r.IsRoot() { return r.CurrentUser() diff --git a/internal/executor/mock.go b/internal/executor/mock.go index 06a490a..6d27d94 100644 --- a/internal/executor/mock.go +++ b/internal/executor/mock.go @@ -36,6 +36,9 @@ type Mock struct { // Glob stubs globs map[string][]string + + // Symlink stubs: path -> resolved target + symlinks map[string]string } type cmdResult struct { @@ -55,6 +58,7 @@ func NewMock() *Mock { paths: make(map[string]string), env: make(map[string]string), globs: make(map[string][]string), + symlinks: make(map[string]string), hostname: "test-host", username: "testuser", homeDir: "/Users/testuser", @@ -145,6 +149,15 @@ func (m *Mock) SetGlob(pattern string, matches []string) { m.globs[pattern] = matches } +// SetSymlink stubs a symlink resolution: EvalSymlinks(path) -> target. +// If a path is not registered, EvalSymlinks returns the path unchanged +// (matching the behavior of filepath.EvalSymlinks on a non-symlink). +func (m *Mock) SetSymlink(path, target string) { + m.mu.Lock() + defer m.mu.Unlock() + m.symlinks[path] = target +} + func (m *Mock) SetGOOS(goos string) { m.mu.Lock() defer m.mu.Unlock() @@ -275,6 +288,16 @@ func (m *Mock) LoggedInUser() (*user.User, error) { return m.CurrentUser() } +func (m *Mock) EvalSymlinks(path string) (string, error) { + m.mu.RLock() + defer m.mu.RUnlock() + if target, ok := m.symlinks[path]; ok { + return target, nil + } + // Default: behave like a non-symlink — return the path unchanged. + return path, nil +} + func (m *Mock) GOOS() string { m.mu.RLock() defer m.mu.RUnlock() diff --git a/internal/executor/user_aware.go b/internal/executor/user_aware.go index fbd0ac3..41cb2e4 100644 --- a/internal/executor/user_aware.go +++ b/internal/executor/user_aware.go @@ -103,5 +103,8 @@ func (e *UserAwareExecutor) HomeDir(username string) (string, error) { return e.inner.HomeDir(username) } func (e *UserAwareExecutor) Glob(pattern string) ([]string, error) { return e.inner.Glob(pattern) } -func (e *UserAwareExecutor) LoggedInUser() (*user.User, error) { return e.inner.LoggedInUser() } -func (e *UserAwareExecutor) GOOS() string { return e.inner.GOOS() } +func (e *UserAwareExecutor) EvalSymlinks(path string) (string, error) { + return e.inner.EvalSymlinks(path) +} +func (e *UserAwareExecutor) LoggedInUser() (*user.User, error) { return e.inner.LoggedInUser() } +func (e *UserAwareExecutor) GOOS() string { return e.inner.GOOS() }