diff --git a/internal/agents/kimi/adapter.go b/internal/agents/kimi/adapter.go index 9c4b6f034..ab257612a 100644 --- a/internal/agents/kimi/adapter.go +++ b/internal/agents/kimi/adapter.go @@ -1,8 +1,8 @@ // Package kimi provides Kimi Code CLI agent integration. // // Integration Note: -// This adapter natively relies on Astral's `uv` package manager -// (`uv tool install kimi-cli`) to securely download and run Kimi CLI, +// This adapter natively relies on Astral's `uv` package manager +// (`uv tool install kimi-cli`) to securely download and run Kimi CLI, // avoiding upstream's pipe-to-shell bootstrap scripts. package kimi @@ -243,7 +243,6 @@ Skills root: "%s"`, gentlemanYaml, skillsRoot) } - // --- Helpers --- func defaultStat(path string) statResult { @@ -272,7 +271,7 @@ func binaryName() string { } // BootstrapTemplate ensures the base KIMI.md template exists in the agent's config directory. -// It is used by the installation pipeline to guarantee that modular components +// It is used by the installation pipeline to guarantee that modular components // (SDD, Engram) can be included even if the Persona component is not installed. func (a *Adapter) BootstrapTemplate(homeDir string) error { kimiDir := a.GlobalConfigDir(homeDir) @@ -281,9 +280,9 @@ func (a *Adapter) BootstrapTemplate(homeDir string) error { } skeletonPath := a.SystemPromptFile(homeDir) - + // We always write the skeleton to ensure any missing includes are restored. - // Since KIMI.md is the 'router' for modular Jinja components, it should + // Since KIMI.md is the 'router' for modular Jinja components, it should // remain managed by the framework. content := assets.MustRead("kimi/KIMI.md") if _, err := filemerge.WriteFileAtomic(skeletonPath, []byte(content), 0o644); err != nil { @@ -301,5 +300,3 @@ func (a *Adapter) BootstrapTemplate(homeDir string) error { return nil } - - diff --git a/internal/agents/kimi/adapter_test.go b/internal/agents/kimi/adapter_test.go index 28664a5eb..1b1e396b4 100644 --- a/internal/agents/kimi/adapter_test.go +++ b/internal/agents/kimi/adapter_test.go @@ -199,8 +199,6 @@ func TestAdapter_Detect_FallbackPaths(t *testing.T) { t.Fatal(err) } - - a := &Adapter{ lookPath: func(string) (string, error) { return "", os.ErrNotExist // Not in PATH @@ -266,27 +264,27 @@ func TestAdapter_PostInstallMessage(t *testing.T) { if tt.os == "windows" { homeDir = `C:\Users\test` } - + msg := a.PostInstallMessage(homeDir) // Construct expected path to verify against quoted output gentlemanYaml := filepath.Join(homeDir, ".kimi", "agents", "gentleman.yaml") - + // Normalize the expected string to the current host's separator. // Since the code uses filepath.Join, it will use \ on Windows and / on Linux. // The test should expect the host's actual separator if we want it to PASS // while running on that host. normalizedExpected := filepath.FromSlash(tt.expected) - + // On Windows, if we are simulating we want backslashes. - // If we are on Windows and testing 'Unix paths' case, it will fail because + // If we are on Windows and testing 'Unix paths' case, it will fail because // the code (running on Windows) used \. This is expected. - // We skip the cross-platform check if it contradicts the host's logic, + // We skip the cross-platform check if it contradicts the host's logic, // or we only check the one matching the current host. // On Windows, if we are simulating we want backslashes. - // If we are on Windows and testing 'Unix paths' case, it will fail because + // If we are on Windows and testing 'Unix paths' case, it will fail because // the code (running on Windows) used \. This is expected. - // We skip the cross-platform check if it contradicts the host's logic, + // We skip the cross-platform check if it contradicts the host's logic, // or we only check the one matching the current host. if (runtime.GOOS == "windows" && tt.os == "windows") || (runtime.GOOS != "windows" && tt.os == "linux") { // Verify path is present @@ -302,5 +300,3 @@ func TestAdapter_PostInstallMessage(t *testing.T) { }) } } - - diff --git a/internal/cli/run_engram_download_test.go b/internal/cli/run_engram_download_test.go index bb285bd20..0d455a518 100644 --- a/internal/cli/run_engram_download_test.go +++ b/internal/cli/run_engram_download_test.go @@ -4,6 +4,7 @@ import ( "context" "os" "path/filepath" + "runtime" "strings" "testing" @@ -600,6 +601,9 @@ func TestRunInstallBetaEngramUsesMainGoInstallAndInstalledBinary(t *testing.T) { home := t.TempDir() gobin := filepath.Join(home, "go-bin") betaEngram := filepath.Join(gobin, "engram") + if runtime.GOOS == "windows" { + betaEngram += ".exe" + } restoreCommand := runCommand restoreLookPath := cmdLookPath diff --git a/internal/components/engram/download_test.go b/internal/components/engram/download_test.go index 32fb08de0..3e94b60fc 100644 --- a/internal/components/engram/download_test.go +++ b/internal/components/engram/download_test.go @@ -1267,7 +1267,7 @@ func TestEngramGoInstallFromMain_UsesGoEnvForBinDir(t *testing.T) { t.Fatalf("engramGoInstallFromMain: unexpected error: %v", err) } - wantDir := fakeInstallDir + wantDir := filepath.FromSlash(fakeInstallDir) gotDir := filepath.Dir(binaryPath) if gotDir != wantDir { t.Errorf("binary dir = %q, want %q (from go env GOBIN)", gotDir, wantDir) @@ -1278,9 +1278,22 @@ func TestEngramGoInstallFromMain_BypassesPublicGoProxy(t *testing.T) { binDir := t.TempDir() goPath := filepath.Join(binDir, "go") recordPath := filepath.Join(t.TempDir(), "go-env.txt") - fakeGo := filepath.Join(binDir, "go") - script := "#!/usr/bin/env bash\n" + - "printf 'GONOSUMDB=%s\\nGOPRIVATE=%s\\nGONOPROXY=%s\\n' \"${GONOSUMDB:-}\" \"${GOPRIVATE:-}\" \"${GONOPROXY:-}\" > \"$GO_ENV_RECORD\"\n" + + var fakeGo string + var script string + if runtime.GOOS == "windows" { + fakeGo = filepath.Join(binDir, "go.bat") + script = "@echo off\n" + + "(\n" + + "echo GONOSUMDB=%GONOSUMDB%\n" + + "echo GOPRIVATE=%GOPRIVATE%\n" + + "echo GONOPROXY=%GONOPROXY%\n" + + ") > \"%GO_ENV_RECORD%\"\n" + } else { + fakeGo = filepath.Join(binDir, "go") + script = "#!/usr/bin/env bash\n" + + "printf 'GONOSUMDB=%s\\nGOPRIVATE=%s\\nGONOPROXY=%s\\n' \"${GONOSUMDB:-}\" \"${GOPRIVATE:-}\" \"${GONOPROXY:-}\" > \"$GO_ENV_RECORD\"\n" + } if err := os.WriteFile(fakeGo, []byte(script), 0o755); err != nil { t.Fatal(err) } diff --git a/internal/components/gga/config_test.go b/internal/components/gga/config_test.go index ec9021c5b..fbe9b5992 100644 --- a/internal/components/gga/config_test.go +++ b/internal/components/gga/config_test.go @@ -3,6 +3,7 @@ package gga import ( "os" "path/filepath" + "runtime" "strings" "testing" @@ -119,6 +120,10 @@ func TestBuildConfigDifferentProviders(t *testing.T) { func TestInjectWritesConfigAndAgents(t *testing.T) { home := t.TempDir() + if runtime.GOOS == "windows" { + t.Setenv("APPDATA", filepath.Join(home, "AppData", "Roaming")) + } + result, err := Inject(home, []model.AgentID{model.AgentClaudeCode}) if err != nil { t.Fatalf("Inject() error = %v", err) @@ -163,6 +168,10 @@ func TestInjectWritesConfigAndAgents(t *testing.T) { func TestInjectIsIdempotent(t *testing.T) { home := t.TempDir() + if runtime.GOOS == "windows" { + t.Setenv("APPDATA", filepath.Join(home, "AppData", "Roaming")) + } + first, err := Inject(home, []model.AgentID{model.AgentOpenCode}) if err != nil { t.Fatalf("Inject() first error = %v", err) @@ -238,10 +247,10 @@ func TestGGAConfigDirDarwin(t *testing.T) { func TestGGAConfigDirWindows(t *testing.T) { tests := []struct { - name string - homeDir string - appDataEnv string - wantSuffix string + name string + homeDir string + appDataEnv string + wantSuffix string }{ { name: "APPDATA set to standard roaming path", diff --git a/internal/components/permissions/inject_test.go b/internal/components/permissions/inject_test.go index 72e8cef87..6644c37e5 100644 --- a/internal/components/permissions/inject_test.go +++ b/internal/components/permissions/inject_test.go @@ -377,7 +377,6 @@ func TestInjectCodexWritesGentleDevPermissionsProfile(t *testing.T) { `"~/.gitconfig" = "read"`, `"~/.local/state/nix/profiles/home-manager/home-path" = "read"`, `"~/.nix-profile" = "read"`, - `"/nix/store" = "read"`, `":tmpdir" = "write"`, `":slash_tmp" = "write"`, `glob_scan_max_depth = 6`, @@ -396,6 +395,9 @@ func TestInjectCodexWritesGentleDevPermissionsProfile(t *testing.T) { `[permissions.gentle-dev.workspace_roots]`, `"~" = true`, } + if codexPermissionsGOOS != "windows" { + wantSubstrings = append(wantSubstrings, `"/nix/store" = "read"`) + } for _, want := range wantSubstrings { if !strings.Contains(text, want) { t.Fatalf("config.toml missing %q; got:\n%s", want, text) diff --git a/internal/components/uninstall/cleaners_test.go b/internal/components/uninstall/cleaners_test.go index 033be7b82..cd9bff79e 100644 --- a/internal/components/uninstall/cleaners_test.go +++ b/internal/components/uninstall/cleaners_test.go @@ -278,7 +278,7 @@ func TestReadManagedFile_RejectsSymlink(t *testing.T) { } link := filepath.Join(dir, "link.json") if err := os.Symlink(target, link); err != nil { - t.Fatalf("Symlink() error = %v", err) + t.Skipf("skipping symlink test; symlink creation failed (likely due to missing privileges on Windows): %v", err) } _, err := readManagedFile(link) diff --git a/internal/model/capability.go b/internal/model/capability.go index bae22b0e7..d27e96a00 100644 --- a/internal/model/capability.go +++ b/internal/model/capability.go @@ -19,4 +19,4 @@ func ModelCapability(modelID string) string { } } return "capable" -} \ No newline at end of file +} diff --git a/internal/model/capability_test.go b/internal/model/capability_test.go index 608735ba1..65a08a2c4 100644 --- a/internal/model/capability_test.go +++ b/internal/model/capability_test.go @@ -4,7 +4,7 @@ import "testing" func TestModelCapability(t *testing.T) { tests := []struct { - modelID string + modelID string wantCapability string }{ {"gemini-3-flash", "small"}, @@ -13,9 +13,9 @@ func TestModelCapability(t *testing.T) { {"claude-sonnet-4", "capable"}, {"gpt-4o", "capable"}, {"", "capable"}, - {"GEMINI-3-FLASH", "small"}, // case-insensitive - {"GPT-4O-MINI", "small"}, // case-insensitive - {"Claude-Haiku", "small"}, // case-insensitive + {"GEMINI-3-FLASH", "small"}, // case-insensitive + {"GPT-4O-MINI", "small"}, // case-insensitive + {"Claude-Haiku", "small"}, // case-insensitive {"anthropic/claude-haiku-3-5", "small"}, {"openai/gpt-4o-mini-2024-07-18", "small"}, {"google/gemini-2.0-flash", "small"}, @@ -34,4 +34,4 @@ func TestModelCapability(t *testing.T) { } }) } -} \ No newline at end of file +} diff --git a/internal/model/codex_model.go b/internal/model/codex_model.go index 698c32642..bab997451 100644 --- a/internal/model/codex_model.go +++ b/internal/model/codex_model.go @@ -98,7 +98,6 @@ var codexPresetMatrix = map[CodexPresetKey]map[string]CodexCarrilDefault{ }, } - // CodexOrchestratorAssignment is the explicit top-level Codex session model // selected by a Gentle AI preset. It is separate from delegated SDD carriles. type CodexOrchestratorAssignment struct { @@ -197,6 +196,7 @@ type CodexTierGroup struct { // resolveProfileAssignments agree on the same canonical tier values: // // These efforts are Gentle AI workload policy, not Codex defaults. +// // Carril LowCost Recommended Powerful // sdd-strong medium medium high // sdd-mid medium medium high diff --git a/internal/model/selection.go b/internal/model/selection.go index 8648dbbba..ad10cbc63 100644 --- a/internal/model/selection.go +++ b/internal/model/selection.go @@ -1,27 +1,27 @@ package model type Selection struct { - Agents []AgentID - Components []ComponentID - Skills []SkillID - Persona PersonaID - Preset PresetID - SDDMode SDDModeID - SDDProfileStrategy SDDProfileStrategyID - StrictTDD bool - CodexMultiAgent bool // deprecated: Codex now always writes features.multi_agent = true; retained for state/back-compat - ModelAssignments map[string]ModelAssignment // key = sub-agent name (e.g., "sdd-init") - ClaudeModelAssignments map[string]ClaudeModelAlias // key = phase name; value = fable|opus|sonnet|haiku - ClaudePhaseAssignments map[string]ClaudePhaseAssignment // key = phase name; value = Claude model+effort - KiroModelAssignments map[string]KiroModelAlias // key = phase name; value = Kiro-native model alias - CodexModelAssignments map[string]CodexEffort // key = phase name; value = low|medium|high|xhigh - CodexOrchestratorAssignment *CodexOrchestratorAssignment // non-nil = apply curated top-level Codex model/effort - ClearCodexOrchestratorAssignment bool // true = clear persisted curated assignment while preserving config.toml - CodexCarrilModelAssignments map[string]string // key = carril profile (sdd-strong|sdd-mid|sdd-cheap); value = model id - CodexPhaseModelAssignments map[string]string // key = phase name; value = model id (Custom per-phase picker only) - Profiles []Profile // named SDD profiles to generate/update during sync - OpenCodePlugins []OpenCodeCommunityPluginID // optional community OpenCode TUI plugins - CommunityTools []CommunityToolID // optional cross-agent community tools/plugins + Agents []AgentID + Components []ComponentID + Skills []SkillID + Persona PersonaID + Preset PresetID + SDDMode SDDModeID + SDDProfileStrategy SDDProfileStrategyID + StrictTDD bool + CodexMultiAgent bool // deprecated: Codex now always writes features.multi_agent = true; retained for state/back-compat + ModelAssignments map[string]ModelAssignment // key = sub-agent name (e.g., "sdd-init") + ClaudeModelAssignments map[string]ClaudeModelAlias // key = phase name; value = fable|opus|sonnet|haiku + ClaudePhaseAssignments map[string]ClaudePhaseAssignment // key = phase name; value = Claude model+effort + KiroModelAssignments map[string]KiroModelAlias // key = phase name; value = Kiro-native model alias + CodexModelAssignments map[string]CodexEffort // key = phase name; value = low|medium|high|xhigh + CodexOrchestratorAssignment *CodexOrchestratorAssignment // non-nil = apply curated top-level Codex model/effort + ClearCodexOrchestratorAssignment bool // true = clear persisted curated assignment while preserving config.toml + CodexCarrilModelAssignments map[string]string // key = carril profile (sdd-strong|sdd-mid|sdd-cheap); value = model id + CodexPhaseModelAssignments map[string]string // key = phase name; value = model id (Custom per-phase picker only) + Profiles []Profile // named SDD profiles to generate/update during sync + OpenCodePlugins []OpenCodeCommunityPluginID // optional community OpenCode TUI plugins + CommunityTools []CommunityToolID // optional cross-agent community tools/plugins } func (s Selection) HasCommunityTool(tool CommunityToolID) bool { @@ -63,18 +63,18 @@ type SyncOverrides struct { // TargetAgents forces TUI sync to run the adapter(s) affected by the // override, even when persisted install state omits them. This is used by // model/profile configurators, where the user picked a concrete target agent. - TargetAgents []AgentID - ModelAssignments map[string]ModelAssignment // nil = no override; empty map = reset to defaults - ClaudeModelAssignments map[string]ClaudeModelAlias // nil = no override; empty map = reset to defaults - ClaudePhaseAssignments map[string]ClaudePhaseAssignment // nil = no override; empty map = reset to defaults - KiroModelAssignments map[string]KiroModelAlias // nil = no override; empty map = reset to defaults - CodexModelAssignments map[string]CodexEffort // nil = no override; empty map = reset to defaults - CodexOrchestratorAssignment *CodexOrchestratorAssignment // non-nil = apply curated top-level Codex model/effort - ClearCodexOrchestratorAssignment bool // true = clear persisted curated assignment while preserving config.toml - CodexCarrilModelAssignments map[string]string // nil = no override; empty map = reset to defaults - CodexPhaseModelAssignments map[string]string // nil = no override (partial sync); non-nil empty = clear (preset selected); non-nil non-empty = custom per-phase assignments - SDDMode SDDModeID // "" = no override; when non-empty, overrides the sync's default SDD mode - SDDProfileStrategy SDDProfileStrategyID // "" = auto; otherwise explicit sync profile strategy - StrictTDD *bool // nil = no override; non-nil = override strict TDD mode - Profiles []Profile // NEW: profile creation/updates during sync + TargetAgents []AgentID + ModelAssignments map[string]ModelAssignment // nil = no override; empty map = reset to defaults + ClaudeModelAssignments map[string]ClaudeModelAlias // nil = no override; empty map = reset to defaults + ClaudePhaseAssignments map[string]ClaudePhaseAssignment // nil = no override; empty map = reset to defaults + KiroModelAssignments map[string]KiroModelAlias // nil = no override; empty map = reset to defaults + CodexModelAssignments map[string]CodexEffort // nil = no override; empty map = reset to defaults + CodexOrchestratorAssignment *CodexOrchestratorAssignment // non-nil = apply curated top-level Codex model/effort + ClearCodexOrchestratorAssignment bool // true = clear persisted curated assignment while preserving config.toml + CodexCarrilModelAssignments map[string]string // nil = no override; empty map = reset to defaults + CodexPhaseModelAssignments map[string]string // nil = no override (partial sync); non-nil empty = clear (preset selected); non-nil non-empty = custom per-phase assignments + SDDMode SDDModeID // "" = no override; when non-empty, overrides the sync's default SDD mode + SDDProfileStrategy SDDProfileStrategyID // "" = auto; otherwise explicit sync profile strategy + StrictTDD *bool // nil = no override; non-nil = override strict TDD mode + Profiles []Profile // NEW: profile creation/updates during sync } diff --git a/internal/model/types_test.go b/internal/model/types_test.go index bf7139da4..e3e5ce630 100644 --- a/internal/model/types_test.go +++ b/internal/model/types_test.go @@ -13,9 +13,9 @@ func TestAgentAntigravity(t *testing.T) { // 1.1 — all six TriggerEvent constants exist with correct string values. func TestTriggerEvent_ClosedSet(t *testing.T) { events := []struct { - name string - got TriggerEvent - want string + name string + got TriggerEvent + want string }{ {"EventPreCommit", EventPreCommit, "pre-commit"}, {"EventPrePush", EventPrePush, "pre-push"}, diff --git a/internal/system/detect_test.go b/internal/system/detect_test.go index 7b9df5ed9..1ab030d82 100644 --- a/internal/system/detect_test.go +++ b/internal/system/detect_test.go @@ -311,9 +311,9 @@ func TestResolvePlatformProfileMatrix(t *testing.T) { // used by effectiveMethod to implement the brew → go-install → binary auto-detect order. func TestGoAvailableInPlatformProfile(t *testing.T) { tests := []struct { - name string - tools map[string]ToolStatus - wantGoAvail bool + name string + tools map[string]ToolStatus + wantGoAvail bool }{ { name: "go in tools and installed → GoAvailable true", diff --git a/internal/tui/model.go b/internal/tui/model.go index 210f0bb72..eede8f890 100644 --- a/internal/tui/model.go +++ b/internal/tui/model.go @@ -1261,12 +1261,12 @@ func (m Model) handleKeyPress(key tea.KeyMsg) (tea.Model, tea.Cmd) { phaseOverride = map[string]string{} // explicit clear signal for the preset path } m.PendingSyncOverrides = &model.SyncOverrides{ - TargetAgents: []model.AgentID{model.AgentCodex}, - CodexModelAssignments: assignments, - CodexOrchestratorAssignment: m.Selection.CodexOrchestratorAssignment, + TargetAgents: []model.AgentID{model.AgentCodex}, + CodexModelAssignments: assignments, + CodexOrchestratorAssignment: m.Selection.CodexOrchestratorAssignment, ClearCodexOrchestratorAssignment: m.Selection.ClearCodexOrchestratorAssignment, - CodexCarrilModelAssignments: presetCarrilModels, - CodexPhaseModelAssignments: phaseOverride, + CodexCarrilModelAssignments: presetCarrilModels, + CodexPhaseModelAssignments: phaseOverride, } m = m.withResetSyncState() m.setScreen(ScreenSync) diff --git a/internal/tui/model_test.go b/internal/tui/model_test.go index d499034bd..d29790f44 100644 --- a/internal/tui/model_test.go +++ b/internal/tui/model_test.go @@ -923,7 +923,9 @@ func sddMultiCursor(t *testing.T) int { // opencode.json and otherwise shows its explicit empty state instead of silently // skipping model assignment. func TestSDDModeMultiShowsModelPickerWhenCacheMissing(t *testing.T) { - t.Setenv("HOME", t.TempDir()) + tmp := t.TempDir() + t.Setenv("HOME", tmp) + t.Setenv("USERPROFILE", tmp) m := NewModel(system.DetectionResult{}, "dev") m.Screen = ScreenSDDMode @@ -943,7 +945,9 @@ func TestSDDModeMultiShowsModelPickerWhenCacheMissing(t *testing.T) { } func TestSDDModeMultiEmptyModelPickerCanContinueWithDefaults(t *testing.T) { - t.Setenv("HOME", t.TempDir()) + tmp := t.TempDir() + t.Setenv("HOME", tmp) + t.Setenv("USERPROFILE", tmp) m := NewModel(system.DetectionResult{}, "dev") m.Screen = ScreenSDDMode @@ -6357,7 +6361,9 @@ func TestPickerFlowSlice(t *testing.T) { { name: "non-custom all agents SDDMode Multi cache absent excludes ModelPicker", setup: func(t *testing.T) Model { - t.Setenv("HOME", t.TempDir()) // guarantees cache path resolves to missing file + tmp := t.TempDir() + t.Setenv("HOME", tmp) + t.Setenv("USERPROFILE", tmp) m := NewModel(system.DetectionResult{}, "dev") m.Selection.Preset = model.PresetFullGentleman m.Selection.Agents = allPickerAgents diff --git a/internal/tui/preset_flow_test.go b/internal/tui/preset_flow_test.go index 6c02c9a9f..a0e3030ce 100644 --- a/internal/tui/preset_flow_test.go +++ b/internal/tui/preset_flow_test.go @@ -398,13 +398,13 @@ func TestInstallNavigationRoundTrips(t *testing.T) { return m }, forwardActions: []flowAction{ - {key: tea.KeyMsg{Type: tea.KeyEnter}}, // Preset → Claude picker - {key: tea.KeyMsg{Type: tea.KeyEnter}}, // Claude preset → Kiro picker - {key: tea.KeyMsg{Type: tea.KeyEnter}}, // Kiro preset → Codex picker - {key: tea.KeyMsg{Type: tea.KeyEnter}}, // Codex preset → SDDMode - {key: tea.KeyMsg{Type: tea.KeyEnter}}, // SDDMode single → StrictTDD - {key: tea.KeyMsg{Type: tea.KeyEnter}}, // StrictTDD → OpenCodePlugins - {key: tea.KeyMsg{Type: tea.KeyEnter}, cursor: continuePluginsCursor, setCursor: true}, // OpenCodePlugins → DependencyTree + {key: tea.KeyMsg{Type: tea.KeyEnter}}, // Preset → Claude picker + {key: tea.KeyMsg{Type: tea.KeyEnter}}, // Claude preset → Kiro picker + {key: tea.KeyMsg{Type: tea.KeyEnter}}, // Kiro preset → Codex picker + {key: tea.KeyMsg{Type: tea.KeyEnter}}, // Codex preset → SDDMode + {key: tea.KeyMsg{Type: tea.KeyEnter}}, // SDDMode single → StrictTDD + {key: tea.KeyMsg{Type: tea.KeyEnter}}, // StrictTDD → OpenCodePlugins + {key: tea.KeyMsg{Type: tea.KeyEnter}, cursor: continuePluginsCursor, setCursor: true}, // OpenCodePlugins → DependencyTree }, forwardScreens: []Screen{ ScreenClaudeModelPicker, @@ -445,11 +445,11 @@ func TestInstallNavigationRoundTrips(t *testing.T) { return m }, forwardActions: []flowAction{ - {key: tea.KeyMsg{Type: tea.KeyEnter}}, // Preset → Claude picker - {key: tea.KeyMsg{Type: tea.KeyEnter}}, // Claude preset → Kiro picker - {key: tea.KeyMsg{Type: tea.KeyEnter}}, // Kiro preset → Codex picker - {key: tea.KeyMsg{Type: tea.KeyEnter}}, // Codex preset → SDDMode - {key: tea.KeyMsg{Type: tea.KeyEnter}, cursor: sddMultiCursor(t), setCursor: true}, // SDDMode multi → ModelPicker + {key: tea.KeyMsg{Type: tea.KeyEnter}}, // Preset → Claude picker + {key: tea.KeyMsg{Type: tea.KeyEnter}}, // Claude preset → Kiro picker + {key: tea.KeyMsg{Type: tea.KeyEnter}}, // Kiro preset → Codex picker + {key: tea.KeyMsg{Type: tea.KeyEnter}}, // Codex preset → SDDMode + {key: tea.KeyMsg{Type: tea.KeyEnter}, cursor: sddMultiCursor(t), setCursor: true}, // SDDMode multi → ModelPicker { key: tea.KeyMsg{Type: tea.KeyEnter}, cursor: len(screens.ModelPickerRows()), diff --git a/internal/update/check_test.go b/internal/update/check_test.go index 5540768db..a4cba261f 100644 --- a/internal/update/check_test.go +++ b/internal/update/check_test.go @@ -878,9 +878,9 @@ func TestCheckSingleTool_EngramUsesBinaryReleaseChannel(t *testing.T) { } execCommand = func(name string, args ...string) *exec.Cmd { if name == "engram" { - return exec.Command("echo", "engram 1.15.13") + return mockCmd("echo", "engram 1.15.13") } - return exec.Command("false") + return mockCmd("false") } result := checkSingleTool(context.Background(), Tools[1], "dev", system.PlatformProfile{OS: "darwin", PackageManager: "brew", Supported: true}) diff --git a/internal/update/install_script_test.go b/internal/update/install_script_test.go index 72a28095f..bc93fc8e0 100644 --- a/internal/update/install_script_test.go +++ b/internal/update/install_script_test.go @@ -70,7 +70,7 @@ func TestInstallScriptBetaGoInstallBypassesPublicGoProxy(t *testing.T) { t.Fatalf("ReadFile(%q) error = %v", path, err) } - script := string(content) + script := strings.ReplaceAll(string(content), "\r\n", "\n") for _, want := range []string{ "prepend_go_env_pattern GONOSUMDB github.com/gentleman-programming/gentle-ai", "prepend_go_env_pattern GOPRIVATE github.com/gentleman-programming/gentle-ai", @@ -103,7 +103,7 @@ func TestInstallScriptBetaGoInstallBypassesPublicGoProxy(t *testing.T) { } function := script[start : start+end+3] - cmd := exec.Command("bash", "-c", function+` + cmdStr := function + ` GONOSUMDB=example.com/private GOPRIVATE=github.com/acme/* GONOPROXY=github.com/gentleman-programming/gentle-ai @@ -111,13 +111,32 @@ prepend_go_env_pattern GONOSUMDB github.com/gentleman-programming/gentle-ai prepend_go_env_pattern GOPRIVATE github.com/gentleman-programming/gentle-ai prepend_go_env_pattern GONOPROXY github.com/gentleman-programming/gentle-ai printf '%s\n%s\n%s\n' "$GONOSUMDB" "$GOPRIVATE" "$GONOPROXY" -`) +` + cmdStr = strings.ReplaceAll(cmdStr, "\r", "") + tmpFile := "script_test_" + t.Name() + ".sh" + if err := os.WriteFile(tmpFile, []byte(cmdStr), 0o755); err != nil { + t.Fatal(err) + } + defer os.Remove(tmpFile) + cmd := exec.Command("bash", tmpFile) out, err := cmd.CombinedOutput() if err != nil { t.Fatalf("run prepend_go_env_pattern fixture: %v\noutput: %s", err, out) } - got := strings.TrimSpace(string(out)) + lines := strings.Split(strings.TrimSpace(string(out)), "\n") + var cleanLines []string + for _, l := range lines { + l = strings.TrimSpace(l) + if strings.HasPrefix(l, "wsl:") { + continue + } + if l == "" { + continue + } + cleanLines = append(cleanLines, l) + } + got := strings.Join(cleanLines, "\n") want := strings.Join([]string{ "github.com/gentleman-programming/gentle-ai,example.com/private", "github.com/gentleman-programming/gentle-ai,github.com/acme/*", diff --git a/internal/update/upgrade/download.go b/internal/update/upgrade/download.go index 30ec1288d..8e1472d7d 100644 --- a/internal/update/upgrade/download.go +++ b/internal/update/upgrade/download.go @@ -27,6 +27,12 @@ var httpClient = &http.Client{Timeout: 5 * time.Minute} // lookPathFn resolves the binary path. Package-level var for testability. var lookPathFn = exec.LookPath +// renameFn performs a filesystem rename. Package-level var for testability — +// it lets tests simulate rename outcomes (including a rename that reports an +// error despite actually completing on disk) without touching the real +// filesystem semantics of os.Rename. +var renameFn = os.Rename + // resolveAssetURLFn and resolveChecksumURLFn build download URLs. // Package-level vars for testability. var resolveAssetURLFn = resolveAssetURL @@ -100,8 +106,16 @@ func Download(ctx context.Context, r update.UpdateResult, profile system.Platfor return fmt.Errorf("extract %s: %w", r.Tool.Name, err) } - // Atomic replace. - if err := atomicReplace(tmpBinaryPath, binaryPath); err != nil { + // Hash the extracted binary BEFORE replacing so we know exactly what + // "correct" means once the replace step runs (see replaceAndVerify). + wantHash, err := hashFile(tmpBinaryPath) + if err != nil { + _ = os.Remove(tmpBinaryPath) + return fmt.Errorf("hash extracted %s binary: %w", r.Tool.Name, err) + } + + // Replace the installed binary and positively verify the result. + if err := replaceAndVerify(tmpBinaryPath, binaryPath, wantHash); err != nil { _ = os.Remove(tmpBinaryPath) return fmt.Errorf("replace %q: %w", binaryPath, err) } @@ -278,12 +292,74 @@ func writeExecutable(r io.Reader, outPath string) error { return nil } -// atomicReplace moves src to dst atomically using os.Rename. -// This is safe on Unix (same-filesystem rename) but NOT safe on Windows -// when the binary is running. The caller must guard against Windows before calling. +// atomicReplace moves src to dst via a single atomic rename. +// On Unix this is atomic: on success dst is the new binary; on a failure dst +// is left untouched (the previous binary, or absent on first install). The +// caller guards against Windows before calling (a running binary cannot be +// renamed over on Windows). func atomicReplace(src, dst string) error { - if err := os.Rename(src, dst); err != nil { + if err := renameFn(src, dst); err != nil { return fmt.Errorf("rename %s -> %s: %w", src, dst, err) } return nil } + +// hashFile returns the SHA256 hex digest of the file at path. +func hashFile(path string) (string, error) { + f, err := os.Open(path) + if err != nil { + return "", fmt.Errorf("open %s: %w", path, err) + } + defer f.Close() + + h := sha256.New() + if _, err := io.Copy(h, f); err != nil { + return "", fmt.Errorf("hash %s: %w", path, err) + } + return hex.EncodeToString(h.Sum(nil)), nil +} + +// replaceAndVerify replaces dst with src, then guards against issue #230. +// In some hardened or restricted environments (potentially involving overlayfs, +// immutable distros, or SELinux/AppArmor profiles), a rename error might be +// reported during a running binary's self-replace even if the replacement actually +// completed on disk. When the rename reports success we trust it (the rename +// is atomic). When it reports an error we positively verify dst's on-disk content +// against wantHash — the SHA256 of the extracted binary — and only treat it +// as success if the content provably matches. +// +// Analysis of self-update race: +// A concurrent self-update race occurs if two processes attempt to upgrade the +// same binary to wantHash concurrently. Because renameFn (os.Rename) is atomic, +// if one process succeeds in renaming, the binary on disk is updated fully. +// The other process's rename will fail (e.g., due to file locks or busy text file). +// By verifying the hash, the second process confirms that the correct binary +// is indeed present on disk, ensuring a consistent final state without reporting +// a false failure to the user. Since the check only succeeds if the hash matches +// wantHash exactly, it is impossible to mask a failure to write a different version, +// nor can it result in a half-written or corrupt binary. +// +// Non-masking guarantee: a reported error is converted to success ONLY when +// dst's bytes equal wantHash exactly. On a genuine failure the atomic rename +// leaves dst as the previous binary (never missing/half-written), and the real +// error is returned — a real failure is never reported as success. +// +// Invariant: the success path (nil error from atomicReplace) is trusted +// without re-hashing, because renameFn is os.Rename — a metadata-only +// operation that cannot report nil unless the rename actually completed, so +// dst then holds src's bytes (== wantHash, computed just before). Do NOT wire +// a retrying/best-effort rename into renameFn that could report nil on a +// rename that did not complete; that would reopen a masking hole here. +func replaceAndVerify(src, dst, wantHash string) error { + if err := atomicReplace(src, dst); err != nil { + // The rename reported an error, but it may still have landed. Verify. + gotHash, hashErr := hashFile(dst) + if hashErr == nil && gotHash == wantHash { + return nil + } + return err + } + // Rename reported success: os.Rename is atomic, so dst now holds the + // extracted binary (== wantHash by construction) — no re-hash needed. + return nil +} diff --git a/internal/update/upgrade/download_test.go b/internal/update/upgrade/download_test.go index dff0d05ee..a8244c282 100644 --- a/internal/update/upgrade/download_test.go +++ b/internal/update/upgrade/download_test.go @@ -235,6 +235,187 @@ func TestAtomicReplace(t *testing.T) { } } +// --- TestReplaceAndVerify --- +// +// TestReplaceAndVerify covers issue #230: the TUI showed a false ✗ failure +// for the gentle-ai self-binary upgrade even though the binary was correctly +// downloaded and replaced. replaceAndVerify performs a single atomic rename +// and, only when that rename reports an error, positively verifies dst's +// on-disk content against the extracted binary's SHA256. A benign/environment- +// specific rename error (overlayfs, immutable distro, SELinux/AppArmor, etc.) +// therefore does not surface as a failure when the correct binary provably +// landed on disk — while a genuine failure (content mismatch, old binary +// remains) must still return an error and leave the previous binary intact. +// +// The renameFn seam lets these run cross-platform (including Windows). +func TestReplaceAndVerify(t *testing.T) { + sha256Hex := func(b []byte) string { + h := sha256.New() + h.Write(b) + return hex.EncodeToString(h.Sum(nil)) + } + + t.Run("rename succeeds → success, dst is the new binary", func(t *testing.T) { + dir := t.TempDir() + src := filepath.Join(dir, "new-binary") + dst := filepath.Join(dir, "existing-binary") + + newContent := []byte("new content") + if err := os.WriteFile(src, newContent, 0o755); err != nil { + t.Fatalf("write src: %v", err) + } + if err := os.WriteFile(dst, []byte("old content"), 0o755); err != nil { + t.Fatalf("write dst: %v", err) + } + + orig := renameFn + t.Cleanup(func() { renameFn = orig }) + renameFn = os.Rename + + if err := replaceAndVerify(src, dst, sha256Hex(newContent)); err != nil { + t.Fatalf("replaceAndVerify: expected nil, got %v", err) + } + + got, err := os.ReadFile(dst) + if err != nil { + t.Fatalf("read dst: %v", err) + } + if string(got) != string(newContent) { + t.Errorf("dst content = %q, want %q", got, newContent) + } + }) + + t.Run("benign rename error but the correct binary lands → success", func(t *testing.T) { + dir := t.TempDir() + src := filepath.Join(dir, "new-binary") + dst := filepath.Join(dir, "existing-binary") + + newContent := []byte("new content") + if err := os.WriteFile(src, newContent, 0o755); err != nil { + t.Fatalf("write src: %v", err) + } + if err := os.WriteFile(dst, []byte("old content"), 0o755); err != nil { + t.Fatalf("write dst: %v", err) + } + + orig := renameFn + t.Cleanup(func() { renameFn = orig }) + renameFn = func(oldname, newname string) error { + // Perform the real rename, then report a benign error AFTER it + // actually completed on disk — the issue #230 case. + if err := os.Rename(oldname, newname); err != nil { + return err + } + return fmt.Errorf("simulated benign rename error") + } + + if err := replaceAndVerify(src, dst, sha256Hex(newContent)); err != nil { + t.Fatalf("replaceAndVerify: expected nil (content verified correct despite reported error), got %v", err) + } + + got, err := os.ReadFile(dst) + if err != nil { + t.Fatalf("read dst: %v", err) + } + if string(got) != string(newContent) { + t.Errorf("dst content = %q, want %q", got, newContent) + } + }) + + t.Run("content mismatch on the error path → error (non-masking)", func(t *testing.T) { + dir := t.TempDir() + src := filepath.Join(dir, "new-binary") + dst := filepath.Join(dir, "existing-binary") + + newContent := []byte("new content") + oldContent := []byte("old content") + if err := os.WriteFile(src, newContent, 0o755); err != nil { + t.Fatalf("write src: %v", err) + } + if err := os.WriteFile(dst, oldContent, 0o755); err != nil { + t.Fatalf("write dst: %v", err) + } + + orig := renameFn + t.Cleanup(func() { renameFn = orig }) + renameFn = func(oldname, newname string) error { + // Genuine failure: report an error WITHOUT moving src, so dst keeps + // its old content. + return fmt.Errorf("simulated real rename failure") + } + + err := replaceAndVerify(src, dst, sha256Hex(newContent)) + if err == nil { + t.Fatal("expected error when dst does not match wantHash, got nil (mismatch was masked as success)") + } + + // The previous binary must be preserved — never a false success. + got, readErr := os.ReadFile(dst) + if readErr != nil { + t.Fatalf("read dst after failed replace: %v", readErr) + } + if string(got) != string(oldContent) { + t.Errorf("dst content after failed replace = %q, want original %q (previous binary preserved)", got, oldContent) + } + }) + + t.Run("first install: dst absent, rename succeeds → success", func(t *testing.T) { + dir := t.TempDir() + src := filepath.Join(dir, "new-binary") + dst := filepath.Join(dir, "existing-binary") + + newContent := []byte("new content") + if err := os.WriteFile(src, newContent, 0o755); err != nil { + t.Fatalf("write src: %v", err) + } + // No dst file created (first install). + + orig := renameFn + t.Cleanup(func() { renameFn = orig }) + renameFn = os.Rename + + if err := replaceAndVerify(src, dst, sha256Hex(newContent)); err != nil { + t.Fatalf("replaceAndVerify: expected nil, got %v", err) + } + + got, err := os.ReadFile(dst) + if err != nil { + t.Fatalf("read dst: %v", err) + } + if string(got) != string(newContent) { + t.Errorf("dst content = %q, want %q", got, newContent) + } + }) + + t.Run("concurrent self-update race: another process updated dst first → success", func(t *testing.T) { + dir := t.TempDir() + src := filepath.Join(dir, "new-binary") + dst := filepath.Join(dir, "existing-binary") + + newContent := []byte("new content") + if err := os.WriteFile(src, newContent, 0o755); err != nil { + t.Fatalf("write src: %v", err) + } + + // Simulate another process winning the race and writing the new content to dst + if err := os.WriteFile(dst, newContent, 0o755); err != nil { + t.Fatalf("write dst: %v", err) + } + + orig := renameFn + t.Cleanup(func() { renameFn = orig }) + renameFn = func(oldname, newname string) error { + // Simulate rename failing because dst is locked/busy by the other process + return fmt.Errorf("text file busy") + } + + // This should succeed because dst already has the correct content (wantHash matches) + if err := replaceAndVerify(src, dst, sha256Hex(newContent)); err != nil { + t.Fatalf("replaceAndVerify: expected nil (race absorbed safely because dst content is correct), got %v", err) + } + }) +} + // --- TestDownload_WindowsSkipped --- // TestDownload_WindowsSkipped is a build-constraint smoke test: @@ -321,11 +502,11 @@ func TestExpectedChecksumFor(t *testing.T) { content := "abc123 gentle-ai_1.0.0_darwin_arm64.tar.gz\ndef456 gentle-ai_1.0.0_linux_amd64.tar.gz\n" tests := []struct { - name string - content string - filename string - want string - wantErr bool + name string + content string + filename string + want string + wantErr bool }{ { name: "found first entry", diff --git a/internal/update/upgrade/executor_test.go b/internal/update/upgrade/executor_test.go index 7d366dc37..e95192065 100644 --- a/internal/update/upgrade/executor_test.go +++ b/internal/update/upgrade/executor_test.go @@ -13,6 +13,7 @@ import ( "testing" "github.com/gentleman-programming/gentle-ai/internal/backup" + "github.com/gentleman-programming/gentle-ai/internal/components/gga" "github.com/gentleman-programming/gentle-ai/internal/model" "github.com/gentleman-programming/gentle-ai/internal/state" "github.com/gentleman-programming/gentle-ai/internal/system" @@ -598,11 +599,11 @@ func TestConfigPathsForBackup_CoversManagedAgentPaths(t *testing.T) { homeDir := t.TempDir() managedFiles := map[string]string{ - ".claude/CLAUDE.md": "# Claude", - ".config/opencode/AGENTS.md": "# OpenCode", + ".claude/CLAUDE.md": "# Claude", + ".config/opencode/AGENTS.md": "# OpenCode", ".config/opencode/opencode.json": `{"model":"claude"}`, - ".gemini/GEMINI.md": "# Gemini", - ".cursor/rules/gentle-ai.mdc": "# Cursor rules", + ".gemini/GEMINI.md": "# Gemini", + ".cursor/rules/gentle-ai.mdc": "# Cursor rules", } unmanagedFile := filepath.Join(homeDir, ".claude", "conversation-transcript.md") @@ -856,8 +857,12 @@ func TestConfigPathsForBackup_CoversRegistryAgentsNotInOldList(t *testing.T) { func TestConfigPathsForBackup_GGAExtrasAreIncluded(t *testing.T) { homeDir := t.TempDir() - // Create GGA config file at ~/.config/gga/config - ggaConfigFile := filepath.Join(homeDir, ".config", "gga", "config") + if runtime.GOOS == "windows" { + t.Setenv("APPDATA", filepath.Join(homeDir, "AppData", "Roaming")) + } + + // Create GGA config file at the platform-appropriate path + ggaConfigFile := gga.ConfigPath(homeDir) if err := os.MkdirAll(filepath.Dir(ggaConfigFile), 0o755); err != nil { t.Fatalf("MkdirAll gga config: %v", err) } @@ -865,8 +870,8 @@ func TestConfigPathsForBackup_GGAExtrasAreIncluded(t *testing.T) { t.Fatalf("WriteFile gga config: %v", err) } - // Create GGA runtime lib file at ~/.local/share/gga/lib/pr_mode.sh - ggaLibFile := filepath.Join(homeDir, ".local", "share", "gga", "lib", "pr_mode.sh") + // Create GGA runtime lib file at the platform-appropriate path + ggaLibFile := gga.RuntimePRModePath(homeDir) if err := os.MkdirAll(filepath.Dir(ggaLibFile), 0o755); err != nil { t.Fatalf("MkdirAll gga lib: %v", err) } @@ -1432,7 +1437,7 @@ func TestConfigPathsForBackup_EmptyStateAgentsFallsBackToFilesystem(t *testing.T func mockCmd(name string, args ...string) *exec.Cmd { if runtime.GOOS == "windows" { - if name == "echo" { + if name == "echo" || name == "printf" { return exec.Command("cmd", "/c", "echo "+strings.Join(args, " ")) } if name == "true" { @@ -1444,4 +1449,3 @@ func mockCmd(name string, args ...string) *exec.Cmd { } return exec.Command(name, args...) } - diff --git a/scripts/install.sh b/scripts/install.sh index 777b06eef..40588fec6 100755 --- a/scripts/install.sh +++ b/scripts/install.sh @@ -311,7 +311,7 @@ install_go() { prepend_go_env_pattern() { local name="$1" local pattern="$2" - local current="${!name:-}" + eval "current=\${$name:-}" if [ -z "$current" ]; then printf -v "$name" '%s' "$pattern"