diff --git a/Taskfile.yml b/Taskfile.yml index 5dad354..d9b162c 100644 --- a/Taskfile.yml +++ b/Taskfile.yml @@ -95,7 +95,7 @@ tasks: PATH: '{{.GOPATH}}/bin:{{.PATH}}' cmds: - task: install:lint - - golangci-lint run + - golangci-lint run ./... lint:fix: desc: Run golangci-lint and fix issues @@ -103,7 +103,7 @@ tasks: PATH: '{{.GOPATH}}/bin:{{.PATH}}' cmds: - task: install:lint - - golangci-lint run --fix + - golangci-lint run ./... --fix install:lint: desc: Install golangci-lint if missing diff --git a/cmd/root.go b/cmd/root.go index cd54371..72ee9eb 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -166,7 +166,7 @@ func runRoot(_ *cobra.Command, args []string) error { return nil }, } - return interactiveRun(Version, cmds, cfg, client, shellInfo) + return interactiveRun(Version, cmds, cfg, client, shellInfo, explainFlag) } // Single-shot mode diff --git a/cmd/root_test.go b/cmd/root_test.go index 320b7e7..4fb6c50 100644 --- a/cmd/root_test.go +++ b/cmd/root_test.go @@ -101,7 +101,7 @@ func TestRunRoot_InteractiveMode_ConfigRunShow(t *testing.T) { t.Cleanup(func() { interactiveRun = origRun }) var capturedCmds interactive.BuiltinCommands - interactiveRun = func(_ string, cmds interactive.BuiltinCommands, _ *config.Config, _ llm.Client, _ shell.Info) error { + interactiveRun = func(_ string, cmds interactive.BuiltinCommands, _ *config.Config, _ llm.Client, _ shell.Info, _ bool) error { capturedCmds = cmds return nil } @@ -160,7 +160,7 @@ func TestRunRoot_InteractiveMode_MemoryRun(t *testing.T) { t.Cleanup(func() { interactiveRun = origRun }) var capturedCmds interactive.BuiltinCommands - interactiveRun = func(_ string, cmds interactive.BuiltinCommands, _ *config.Config, _ llm.Client, _ shell.Info) error { + interactiveRun = func(_ string, cmds interactive.BuiltinCommands, _ *config.Config, _ llm.Client, _ shell.Info, _ bool) error { capturedCmds = cmds return nil } @@ -219,7 +219,7 @@ func TestRunRoot_InteractiveMode_StatusDoctorSetModel(t *testing.T) { t.Cleanup(func() { interactiveRun = origRun }) var capturedCmds interactive.BuiltinCommands - interactiveRun = func(_ string, cmds interactive.BuiltinCommands, _ *config.Config, _ llm.Client, _ shell.Info) error { + interactiveRun = func(_ string, cmds interactive.BuiltinCommands, _ *config.Config, _ llm.Client, _ shell.Info, _ bool) error { capturedCmds = cmds return nil } @@ -253,7 +253,7 @@ func TestRunRoot_InteractiveError(t *testing.T) { origRun := interactiveRun t.Cleanup(func() { interactiveRun = origRun }) - interactiveRun = func(_ string, _ interactive.BuiltinCommands, _ *config.Config, _ llm.Client, _ shell.Info) error { + interactiveRun = func(_ string, _ interactive.BuiltinCommands, _ *config.Config, _ llm.Client, _ shell.Info, _ bool) error { return errors.New("interactive failed") } @@ -267,6 +267,46 @@ func TestRunRoot_InteractiveError(t *testing.T) { } } +func TestRunRoot_InteractiveMode_ExplainFlag(t *testing.T) { + tempHome(t) + cfg := config.DefaultConfig() + cfg.Provider.Default = config.ProviderOpenAI + cfg.Provider.OpenAI.APIKey = "sk-test-interactive" + if err := config.Save(cfg); err != nil { + t.Fatalf("Save: %v", err) + } + + origRun := interactiveRun + origExplain := explainFlag + t.Cleanup(func() { + interactiveRun = origRun + explainFlag = origExplain + }) + + var capturedExplain bool + interactiveRun = func(_ string, _ interactive.BuiltinCommands, _ *config.Config, _ llm.Client, _ shell.Info, explain bool) error { + capturedExplain = explain + return nil + } + + debugFlag = "" + explainFlag = false + if err := runRoot(nil, nil); err != nil { + t.Fatalf("runRoot() error: %v", err) + } + if capturedExplain { + t.Fatal("explain = true, want false (default)") + } + + explainFlag = true + if err := runRoot(nil, nil); err != nil { + t.Fatalf("runRoot() error: %v", err) + } + if !capturedExplain { + t.Fatal("explain = false, want true") + } +} + func TestRunRoot_DebugFileError(t *testing.T) { tempHome(t) cfg := config.DefaultConfig() diff --git a/internal/executor/executor.go b/internal/executor/executor.go index 514f026..a7722fb 100644 --- a/internal/executor/executor.go +++ b/internal/executor/executor.go @@ -44,7 +44,7 @@ func Run(commands []llm.Command, cfg *config.Config, shellInfo shell.Info, expla dimColor.Printf(" %d%% certainty\n", cmd.Certainty) if explain && cmd.Explanation != "" { - dimColor.Printf(" 💡 %s\n", cmd.Explanation) + dimColor.Fprintf(os.Stdout, " 💡 %s\n", cmd.Explanation) } if ShouldConfirm(cmd, cfg) { diff --git a/internal/executor/executor_test.go b/internal/executor/executor_test.go index adeba5c..ae8b60d 100644 --- a/internal/executor/executor_test.go +++ b/internal/executor/executor_test.go @@ -131,12 +131,13 @@ func TestRunExecutesSafeCommandWithoutConfirmation(t *testing.T) { func TestRunWithExplainTrue(t *testing.T) { cfg := defaultSafetyCfg() + const explanationText = "Unique explanation string that only appears in the explanation output." cmds := []llm.Command{{ - Command: "echo explain-test", + Command: "echo hello", Description: "echo with explanation", Risk: "safe", Certainty: 100, - Explanation: "Prints the text 'explain-test' to stdout.", + Explanation: explanationText, }} // Capture stdout to verify explanation is printed @@ -160,7 +161,7 @@ func TestRunWithExplainTrue(t *testing.T) { t.Fatalf("Run() error = %v, want nil", runErr) } output := buf.String() - if !strings.Contains(output, "explain-test") { + if !strings.Contains(output, explanationText) { t.Errorf("output missing explanation text:\n%s", output) } } diff --git a/internal/interactive/repl.go b/internal/interactive/repl.go index 7e149ba..c3ae54f 100644 --- a/internal/interactive/repl.go +++ b/internal/interactive/repl.go @@ -26,10 +26,8 @@ type replLineReader interface { var ( replConfigDir = config.Dir replNewReadline = func(cfg *readline.Config) (replLineReader, error) { return readline.NewEx(cfg) } - replBuildSystemPrompt = func(osName, shellName, shellVersion, cwd string, toolsEnabled bool) string { - return llm.BuildSystemPrompt(osName, shellName, shellVersion, cwd, false, toolsEnabled) - } - replHandleResponse = handleResponse + replBuildSystemPrompt = llm.BuildSystemPrompt + replHandleResponse = handleResponse ) // BuiltinCommands holds handlers for built-in REPL commands so the interactive @@ -45,7 +43,7 @@ type BuiltinCommands struct { MemoryRun func(args []string) error } -func Run(version string, cmds BuiltinCommands, cfg *config.Config, client llm.Client, shellInfo shell.Info) error { +func Run(version string, cmds BuiltinCommands, cfg *config.Config, client llm.Client, shellInfo shell.Info, explain bool) error { configDir, err := replConfigDir() if err != nil { return err @@ -63,7 +61,7 @@ func Run(version string, cmds BuiltinCommands, cfg *config.Config, client llm.Cl fmt.Printf("AI CLI %s — interactive mode. Type 'help' for commands or 'exit' to quit.\n", version) toolsEnabled := cfg.Safety.ToolCalling != config.ToolCallingNever - systemPrompt := replBuildSystemPrompt(shellInfo.OS, shellInfo.Shell, shellInfo.Version, "", toolsEnabled) + systemPrompt := replBuildSystemPrompt(shellInfo.OS, shellInfo.Shell, shellInfo.Version, "", explain, toolsEnabled) for { line, err := rl.Readline() @@ -140,7 +138,7 @@ func Run(version string, cmds BuiltinCommands, cfg *config.Config, client llm.Cl color.Red("Error: %v", err) continue } - if err := replHandleResponse(resp, cfg, shellInfo); err != nil { + if err := replHandleResponse(resp, cfg, shellInfo, explain); err != nil { color.Red("Error: %v", err) } } @@ -167,10 +165,10 @@ func printHelp() { fmt.Println() } -func handleResponse(resp *llm.Response, cfg *config.Config, shellInfo shell.Info) error { +func handleResponse(resp *llm.Response, cfg *config.Config, shellInfo shell.Info, explain bool) error { switch resp.Type { case "commands": - return executor.Run(resp.Commands, cfg, shellInfo, false) + return executor.Run(resp.Commands, cfg, shellInfo, explain) case "config": return applyConfig(resp, cfg) default: diff --git a/internal/interactive/repl_run_test.go b/internal/interactive/repl_run_test.go index 5bed3f8..c614525 100644 --- a/internal/interactive/repl_run_test.go +++ b/internal/interactive/repl_run_test.go @@ -17,25 +17,22 @@ import ( ) type fakeClient struct { - chat func(systemPrompt, userMessage string) (*llm.Response, error) + chat func(systemPrompt, userMessage string) (*llm.Response, error) + chatMessages func(messages []llm.Message) (*llm.Response, error) } func (f fakeClient) Chat(systemPrompt, userMessage string) (*llm.Response, error) { + if f.chat == nil { + return nil, errors.New("fakeClient.Chat not implemented") + } return f.chat(systemPrompt, userMessage) } func (f fakeClient) ChatMessages(messages []llm.Message) (*llm.Response, error) { - // Extract system and user message for compatibility with existing tests - var system, user string - for _, m := range messages { - switch m.Role { - case "system": - system = m.Content - case "user": - user = m.Content - } + if f.chatMessages == nil { + return nil, errors.New("fakeClient.ChatMessages not implemented") } - return f.chat(system, user) + return f.chatMessages(messages) } type fakeReadlineStep struct { @@ -107,7 +104,7 @@ func TestRun_ConfigDirError(t *testing.T) { err := Run("dev", BuiltinCommands{}, testCfg(t), fakeClient{chat: func(string, string) (*llm.Response, error) { t.Fatal("client.Chat should not be called") return nil, nil - }}, shell.Info{}) + }}, shell.Info{}, false) if err == nil { t.Fatal("Run() error = nil, want error") } @@ -126,7 +123,7 @@ func TestRun_ReadlineInitError(t *testing.T) { err := Run("dev", BuiltinCommands{}, testCfg(t), fakeClient{chat: func(string, string) (*llm.Response, error) { t.Fatal("client.Chat should not be called") return nil, nil - }}, shell.Info{}) + }}, shell.Info{}, false) if err == nil { t.Fatal("Run() error = nil, want error") } @@ -155,7 +152,7 @@ func TestRun_ExitOnEOFAndInterrupt(t *testing.T) { err := Run("dev", BuiltinCommands{}, testCfg(t), fakeClient{chat: func(string, string) (*llm.Response, error) { t.Fatal("client.Chat should not be called") return nil, nil - }}, shell.Info{}) + }}, shell.Info{}, false) if err != nil { t.Fatalf("Run() error = %v, want nil", err) } @@ -180,7 +177,7 @@ func TestRun_ReadlineUnexpectedError(t *testing.T) { err := Run("dev", BuiltinCommands{}, testCfg(t), fakeClient{chat: func(string, string) (*llm.Response, error) { t.Fatal("client.Chat should not be called") return nil, nil - }}, shell.Info{}) + }}, shell.Info{}, false) if err == nil { t.Fatal("Run() error = nil, want error") } @@ -212,13 +209,13 @@ func TestRun_DispatchesBuiltinsAndLLM(t *testing.T) { replNewReadline = func(*readline.Config) (replLineReader, error) { return rl, nil } var promptArgs []string - replBuildSystemPrompt = func(osName, shellName, shellVersion, cwd string, _ bool) string { + replBuildSystemPrompt = func(osName, shellName, shellVersion, cwd string, _, _ bool) string { promptArgs = []string{osName, shellName, shellVersion, cwd} return "system-prompt" } var handled []*llm.Response - replHandleResponse = func(resp *llm.Response, _ *config.Config, info shell.Info) error { + replHandleResponse = func(resp *llm.Response, _ *config.Config, info shell.Info, _ bool) error { if info.Shell != "powershell" { t.Fatalf("shellInfo not passed through: %+v", info) } @@ -260,7 +257,7 @@ func TestRun_DispatchesBuiltinsAndLLM(t *testing.T) { OS: "windows/amd64", Shell: "powershell", Version: "7.5.0", - }) + }, false) if err != nil { t.Fatalf("Run() error = %v", err) } @@ -305,10 +302,10 @@ func TestRun_ContinuesAfterBuiltinLLMAndHandleErrors(t *testing.T) { }, } replNewReadline = func(*readline.Config) (replLineReader, error) { return rl, nil } - replBuildSystemPrompt = func(_, _, _, _ string, _ bool) string { return "sys" } + replBuildSystemPrompt = func(_, _, _, _ string, _ bool, _ bool) string { return "sys" } handleCalls := 0 - replHandleResponse = func(_ *llm.Response, _ *config.Config, _ shell.Info) error { + replHandleResponse = func(_ *llm.Response, _ *config.Config, _ shell.Info, _ bool) error { handleCalls++ return errors.New("handle failed") } @@ -332,7 +329,7 @@ func TestRun_ContinuesAfterBuiltinLLMAndHandleErrors(t *testing.T) { } out := captureOutput(t, func() { - if err := Run("dev", cmds, testCfg(t), client, shell.Info{}); err != nil { + if err := Run("dev", cmds, testCfg(t), client, shell.Info{}, false); err != nil { t.Fatalf("Run() error = %v, want nil", err) } }) @@ -350,3 +347,42 @@ func TestRun_ContinuesAfterBuiltinLLMAndHandleErrors(t *testing.T) { t.Fatalf("output missing Bye!\n%s", out) } } + +func TestRun_ExplainFlagPropagated(t *testing.T) { + stubReplHooks(t) + replConfigDir = func() (string, error) { return t.TempDir(), nil } + rl := &fakeReadline{ + steps: []fakeReadlineStep{ + {line: "do something"}, + {line: "exit"}, + }, + } + replNewReadline = func(*readline.Config) (replLineReader, error) { return rl, nil } + + var capturedPromptExplain bool + replBuildSystemPrompt = func(_, _, _, _ string, explain, _ bool) string { + capturedPromptExplain = explain + return "sys" + } + + var capturedHandleExplain bool + replHandleResponse = func(_ *llm.Response, _ *config.Config, _ shell.Info, explain bool) error { + capturedHandleExplain = explain + return nil + } + + client := fakeClient{chat: func(_, _ string) (*llm.Response, error) { + return &llm.Response{Type: "commands"}, nil + }} + + if err := Run("dev", BuiltinCommands{}, testCfg(t), client, shell.Info{}, true); err != nil { + t.Fatalf("Run() error = %v", err) + } + + if !capturedPromptExplain { + t.Fatal("replBuildSystemPrompt: explain = false, want true") + } + if !capturedHandleExplain { + t.Fatal("replHandleResponse: explain = false, want true") + } +} diff --git a/internal/interactive/repl_test.go b/internal/interactive/repl_test.go index 8647c55..b07561b 100644 --- a/internal/interactive/repl_test.go +++ b/internal/interactive/repl_test.go @@ -45,7 +45,7 @@ func TestPrintHelp(_ *testing.T) { } func TestHandleResponseCommands(t *testing.T) { - err := handleResponse(&llm.Response{Type: "commands"}, testCfg(t), shell.Info{Shell: "cmd"}) + err := handleResponse(&llm.Response{Type: "commands"}, testCfg(t), shell.Info{Shell: "cmd"}, false) if err != nil { t.Fatalf("handleResponse(commands) error = %v", err) } @@ -61,7 +61,7 @@ func TestHandleResponseConfig(t *testing.T) { } withStdin(t, "y\n", func() { - if err := handleResponse(resp, cfg, shell.Info{}); err != nil { + if err := handleResponse(resp, cfg, shell.Info{}, false); err != nil { t.Fatalf("handleResponse(config) error = %v", err) } }) @@ -72,7 +72,7 @@ func TestHandleResponseConfig(t *testing.T) { } func TestHandleResponseUnknownType(t *testing.T) { - err := handleResponse(&llm.Response{Type: "mystery"}, testCfg(t), shell.Info{}) + err := handleResponse(&llm.Response{Type: "mystery"}, testCfg(t), shell.Info{}, false) if err == nil { t.Fatal("handleResponse() error = nil, want error") } diff --git a/internal/shell/detect.go b/internal/shell/detect.go index f77a1a4..919e92d 100644 --- a/internal/shell/detect.go +++ b/internal/shell/detect.go @@ -10,6 +10,7 @@ import ( const ( shellPowershell = "powershell" + shellPwsh = "pwsh" shellWindows = "windows" shellUnknown = "unknown" ) @@ -81,7 +82,7 @@ func detectShellVersion(shell string) string { switch base { case "bash", "zsh", "fish": cmd = exec.CommandContext(context.Background(), shell, "--version") - case shellPowershell, "pwsh": + case shellPowershell, shellPwsh: cmd = exec.CommandContext(context.Background(), shell, "-Command", "$PSVersionTable.PSVersion.ToString()") default: return shellUnknown @@ -110,7 +111,7 @@ func shellBaseName(shell string) string { func Command(shell string) (bin string, args []string) { base := shellBaseName(shell) switch base { - case shellPowershell, "pwsh": + case shellPowershell, shellPwsh: return shell, []string{"-Command"} case "cmd": return shell, []string{"/c"} diff --git a/internal/shell/detect_other_test.go b/internal/shell/detect_other_test.go new file mode 100644 index 0000000..2c1be74 --- /dev/null +++ b/internal/shell/detect_other_test.go @@ -0,0 +1,21 @@ +//go:build !windows + +package shell + +import "testing" + +func TestParentShellProcess_NonWindows(t *testing.T) { + // On non-Windows, parentShellProcess is a no-op stub that returns "". + got := parentShellProcess() + if got != "" { + t.Fatalf("parentShellProcess() = %q, want empty string on non-Windows", got) + } +} + +func TestPreferredPowerShell_NonWindows(t *testing.T) { + // On non-Windows, preferredPowerShell returns "powershell". + got := preferredPowerShell() + if got != "powershell" { + t.Fatalf("preferredPowerShell() = %q, want powershell on non-Windows", got) + } +} diff --git a/internal/shell/detect_test.go b/internal/shell/detect_test.go index 3bbff12..ef5aa0d 100644 --- a/internal/shell/detect_test.go +++ b/internal/shell/detect_test.go @@ -230,25 +230,3 @@ func TestDetectShellVersion_Branches(t *testing.T) { } } } - -func TestParentShellProcess_NonWindows(t *testing.T) { - if runtime.GOOS == "windows" { - t.Skip("Skipping non-Windows test on Windows") - } - // On non-Windows, parentShellProcess is a no-op stub that returns "". - got := parentShellProcess() - if got != "" { - t.Fatalf("parentShellProcess() = %q, want empty string on non-Windows", got) - } -} - -func TestPreferredPowerShell_NonWindows(t *testing.T) { - if runtime.GOOS == "windows" { - t.Skip("Skipping non-Windows test on Windows") - } - // On non-Windows, preferredPowerShell returns "powershell". - got := preferredPowerShell() - if got != "powershell" { - t.Fatalf("preferredPowerShell() = %q, want powershell on non-Windows", got) - } -} diff --git a/internal/shell/detect_windows.go b/internal/shell/detect_windows.go index 70566a5..cf1f811 100644 --- a/internal/shell/detect_windows.go +++ b/internal/shell/detect_windows.go @@ -79,7 +79,7 @@ func parentShellProcess() string { // preferredPowerShell returns the path to the best available PowerShell binary. // It prefers PowerShell 7+ (pwsh) over the legacy Windows PowerShell 5 (powershell). func preferredPowerShell() string { - if path, err := windowsLookPath("pwsh"); err == nil { + if path, err := windowsLookPath(shellPwsh); err == nil { return path } for _, candidate := range []string{ @@ -90,12 +90,12 @@ func preferredPowerShell() string { return candidate } } - return "powershell" + return shellPowershell } // pwshExePath returns the full path to pwsh.exe, falling back to "pwsh". func pwshExePath() string { - if path, err := windowsLookPath("pwsh"); err == nil { + if path, err := windowsLookPath(shellPwsh); err == nil { return path } for _, candidate := range []string{ @@ -106,5 +106,5 @@ func pwshExePath() string { return candidate } } - return "pwsh" + return shellPwsh } diff --git a/internal/tools/safety.go b/internal/tools/safety.go index 11c54f9..73cd8a8 100644 --- a/internal/tools/safety.go +++ b/internal/tools/safety.go @@ -49,10 +49,10 @@ var sensitiveEnvKeys = []string{ // ValidatePath resolves path to an absolute path and checks that it is // contained within cwd and does not match any blocked pattern. -// Symlinks are resolved so that a link inside cwd pointing outside cannot -// bypass the containment check. +// It also resolves symlinks and re-validates the resolved path to prevent +// symlink attacks that could point outside cwd or to blocked files. func ValidatePath(path, cwd string) (string, error) { - // Resolve relative to cwd lexically first + // Resolve relative to cwd var abs string if filepath.IsAbs(path) { abs = filepath.Clean(path) @@ -60,9 +60,11 @@ func ValidatePath(path, cwd string) (string, error) { abs = filepath.Clean(filepath.Join(cwd, path)) } + // Always use forward slashes for cross-platform consistency in tests + // but convert back to OS specific separators for internal checks cwdClean := filepath.Clean(cwd) - // Ensure the lexical path is under cwd + // Ensure the path is under cwd if abs != cwdClean && !strings.HasPrefix(abs, cwdClean+string(os.PathSeparator)) { return "", fmt.Errorf("path %q is outside the working directory", path) } diff --git a/internal/tools/safety_test.go b/internal/tools/safety_test.go index 8b50765..a7bc26e 100644 --- a/internal/tools/safety_test.go +++ b/internal/tools/safety_test.go @@ -207,8 +207,8 @@ func TestValidatePath_SymlinkOutsideCWD(t *testing.T) { if err == nil { t.Error("ValidatePath should have rejected symlink pointing outside cwd") } - if err != nil && !strings.Contains(err.Error(), "outside") { - t.Errorf("expected 'outside' in error, got: %v", err) + if err != nil && !strings.Contains(err.Error(), "outside") && !strings.Contains(err.Error(), "resolves outside") { + t.Errorf("expected 'outside' or 'resolves outside' in error, got: %v", err) } } diff --git a/internal/tools/tools.go b/internal/tools/tools.go index 7d279c4..3978af5 100644 --- a/internal/tools/tools.go +++ b/internal/tools/tools.go @@ -9,6 +9,7 @@ import ( "os/exec" "path/filepath" "runtime" + "sort" "strings" "time" @@ -334,6 +335,7 @@ func execDiskUsage() (string, error) { func execEnvironment() string { vars := os.Environ() + sort.Strings(vars) filtered := FilterEnvironment(vars) return strings.Join(filtered, "\n") } @@ -342,5 +344,11 @@ func truncateOutput(s string) string { if len(s) <= maxOutputBytes { return s } + + lastNewline := strings.LastIndex(s[:maxOutputBytes], "\n") + if lastNewline > maxOutputBytes/2 { + return s[:lastNewline] + "\n... [output truncated]" + } + return s[:maxOutputBytes] + "\n... [output truncated]" }