diff --git a/internal/tui/config_test.go b/internal/tui/config_test.go index a71ee95..76d40df 100644 --- a/internal/tui/config_test.go +++ b/internal/tui/config_test.go @@ -3,6 +3,7 @@ package tui import ( "encoding/json" "errors" + "fmt" "os" "os/exec" "path/filepath" @@ -422,7 +423,7 @@ func TestConfigureToolsWritesEveryEnabledTool(t *testing.T) { } } - msg, written := configureTools(testKey, nil) + msg, written, failed := configureTools(testKey, nil) if !strings.Contains(msg, "5 added") { t.Fatalf("configureTools said %q, want the five tools written", msg) } @@ -431,6 +432,9 @@ func TestConfigureToolsWritesEveryEnabledTool(t *testing.T) { } // The names come back so the tab can say how to use each one; a tool // written but not named leaves a member with a config and no next step. + if len(failed) != 0 { + t.Errorf("tools failed on a clean run: %v", failed) + } if len(written) != 5 { t.Errorf("configureTools named %v, want all five it wrote", written) } @@ -458,7 +462,7 @@ func TestConfigureToolsWritesEveryEnabledTool(t *testing.T) { } // And unticking a tool takes only that one out. - msg, _ = configureTools(testKey, map[string]bool{"Pi": false}) + msg, _, _ = configureTools(testKey, map[string]bool{"Pi": false}) if !strings.Contains(msg, "1 removed") { t.Errorf("configureTools said %q, want Pi removed", msg) } @@ -1684,3 +1688,85 @@ func TestSigningOutIsAdvertisedWhereItIsLookedFor(t *testing.T) { t.Error("Home offers to sign out of a session that is not there") } } + +// One tool failing used to read as the whole step failing: the message became +// "error: " and the configs that HAD been written +// went unmentioned. Reported from a machine where Hermes would not configure, +// which left the setup on its last screen with nothing to do but escape. +func TestOneToolFailingDoesNotLoseTheOthers(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + t.Setenv("USERPROFILE", home) + hermesHomeDir := filepath.Join(home, "hermes") + t.Setenv("HERMES_HOME", hermesHomeDir) + if err := os.MkdirAll(hermesHomeDir, 0o700); err != nil { + t.Fatal(err) + } + // Hermes refuses; everything else is written as usual. + original := runHermesConfig + runHermesConfig = func(string, ...string) error { + return fmt.Errorf("hermes config set: exit status 1\nsomething it printed\nand more of it") + } + t.Cleanup(func() { runHermesConfig = original }) + + for _, p := range []string{ + filepath.Join(home, ".factory", "settings.json"), + filepath.Join(home, ".config", "opencode", "opencode.json"), + filepath.Join(home, ".pi", "agent", "models.json"), + filepath.Join(home, ".codex", "config.toml"), + } { + if err := os.MkdirAll(filepath.Dir(p), 0o700); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(p, nil, 0o600); err != nil { + t.Fatal(err) + } + } + + msg, written, failed := configureTools(testKey, nil) + + if !strings.Contains(msg, "4 added") { + t.Errorf("the message is %q, and does not say what worked", msg) + } + if !strings.Contains(msg, "Hermes failed") { + t.Errorf("the message is %q, and does not name what did not", msg) + } + if len(written) != 4 { + t.Errorf("wrote %v, want the four that did not fail", written) + } + if len(failed) != 1 || failed[0].name != "Hermes" { + t.Fatalf("failures are %v, want just Hermes", failed) + } + // The summary takes one line of what a tool printed, not the paragraph. + if strings.ContainsAny(failed[0].firstLine(), "\r\n") { + } +} + +// And the step finishes anyway. Holding somebody on the last screen of a setup +// with no way on but escape is the thing that was reported. +func TestAFailedToolDoesNotStrandTheSetup(t *testing.T) { + m := setupModel(t, &session.Session{Token: "t", APIKey: testKey}) + m.lay = newLayout(96, 44) + m.wizard = wizardTools + + updated, _ := m.Update(configuredMsg{ + msg: "4 added · Hermes failed", + written: []string{"OpenCode"}, + failed: []toolFailure{{"Hermes", "hermes config set: exit status 1"}}, + }) + after := updated.(model) + + if after.wizard != wizardDone { + t.Error("a failed tool holds the setup on its last step") + } + out := after.renderWizard(after.lay) + if !strings.Contains(out, "Hermes") { + t.Error("the screen does not say which tool failed") + } + if !strings.Contains(out, "The rest went in") { + t.Error("the screen does not say the others worked") + } + if !strings.Contains(out, "press c again") { + t.Error("the screen does not say what can be done about it") + } +} diff --git a/internal/tui/tui.go b/internal/tui/tui.go index b6d157c..679207c 100644 --- a/internal/tui/tui.go +++ b/internal/tui/tui.go @@ -131,6 +131,7 @@ var errNotSignedIn = errors.New("not signed in — press s") type configuredMsg struct { msg string written []string + failed []toolFailure } // Sent once, by Init, so the panel can pick up wherever the setup was left. @@ -163,6 +164,7 @@ type model struct { // Signing in, without leaving the panel. See startLogin. configuring bool configured []string + failures []toolFailure // Set by the first press of `o`, cleared by anything else: signing out is // not something to do to somebody on a stray keystroke. @@ -312,11 +314,16 @@ func (m model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { case configuredMsg: m.configuring = false - if m.wizard == wizardTools && !strings.HasPrefix(msg.msg, "error") { - m.wizard = wizardDone - } m.setupMsg = msg.msg m.configured = msg.written + m.failures = msg.failed + // The step is finished either way. One tool refusing to be configured + // is not a reason to hold somebody on the last screen of a setup with + // no way on but escape - which is exactly what it did, and what was + // reported. What failed is shown, and they can carry on. + if m.wizard == wizardTools { + m.wizard = wizardDone + } case firstRunMsg: return m, m.resumeSetup() @@ -605,8 +612,8 @@ func (m model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { m.setupMsg = "" key, tools := m.sess.APIKey, m.sess.EnabledTools return m, tea.Batch(m.spin.Tick, func() tea.Msg { - msg, written := configureTools(key, tools) - return configuredMsg{msg, written} + msg, written, failed := configureTools(key, tools) + return configuredMsg{msg, written, failed} }) } } @@ -1779,7 +1786,23 @@ func installedTools() int { return n } -func configureTools(apiKey string, enabledTools map[string]bool) (string, []string) { +// What went wrong for one tool, kept apart from the others so a failure in one +// does not read as a failure in all of them. +type toolFailure struct { + name string + reason string +} + +// firstLine of a reason, for a summary. A tool that is configured by running +// it - Hermes - hands back whatever it printed, which can be a paragraph. +func (f toolFailure) firstLine() string { + if i := strings.IndexAny(f.reason, "\r\n"); i >= 0 { + return strings.TrimSpace(f.reason[:i]) + } + return strings.TrimSpace(f.reason) +} + +func configureTools(apiKey string, enabledTools map[string]bool) (string, []string, []toolFailure) { isEnabled := func(name string) bool { if enabledTools == nil { return true @@ -1790,8 +1813,8 @@ func configureTools(apiKey string, enabledTools map[string]bool) (string, []stri return true } tools := detectTools() - var lastErr error var written []string + var failed []toolFailure added, removed := 0, 0 for _, t := range tools { if !t.installed { @@ -1812,7 +1835,7 @@ func configureTools(apiKey string, enabledTools map[string]bool) (string, []stri err = writeHermesConfig(filepath.Dir(t.configPath), apiKey) } if err != nil { - lastErr = err + failed = append(failed, toolFailure{t.name, err.Error()}) } else { added++ written = append(written, t.name) @@ -1832,15 +1855,17 @@ func configureTools(apiKey string, enabledTools map[string]bool) (string, []stri err = removeHermesConfig(filepath.Dir(t.configPath)) } if err != nil { - lastErr = err + failed = append(failed, toolFailure{t.name, err.Error()}) } else { removed++ } } } - if lastErr != nil { - return "error: " + lastErr.Error(), written - } + // One tool failing used to throw away everything that worked: the message + // became "error: " and the four configs that + // had just been written went unmentioned. A member with a broken Hermes + // was told the whole step had failed, and left on it with nothing to do + // but escape. parts := []string{} if added > 0 { parts = append(parts, fmt.Sprintf("%d added", added)) @@ -1848,10 +1873,13 @@ func configureTools(apiKey string, enabledTools map[string]bool) (string, []stri if removed > 0 { parts = append(parts, fmt.Sprintf("%d removed", removed)) } + for _, f := range failed { + parts = append(parts, f.name+" failed") + } if len(parts) == 0 { - return "nothing to sync", written + return "nothing to sync", written, nil } - return strings.Join(parts, " · "), written + return strings.Join(parts, " · "), written, failed } func factoryCustomID(displayName string, index int) string { @@ -2505,10 +2533,35 @@ func (m model) renderWizard(l layout) string { } else if m.setupMsg != "" { b.WriteString("\n" + m.wrapped(l, m.setupMsg) + "\n") } + b.WriteString(m.renderFailures(l)) } return b.String() } +// What would not configure, and what to do about it. +// +// One tool failing used to read as the whole step failing, and left a member +// on the last screen of a setup with nothing to do but press escape. The rest +// of the tools were configured and nothing said so. +func (m model) renderFailures(l layout) string { + if len(m.failures) == 0 { + return "" + } + bad := lipgloss.NewStyle().Foreground(cRed) + dim := lipgloss.NewStyle().Foreground(cDimGray) + + var b strings.Builder + b.WriteString("\n") + for _, f := range m.failures { + b.WriteString(m.wrapped(l, "error: "+f.name+" — "+f.firstLine()) + "\n") + } + _ = bad + b.WriteString("\n" + l.indent + dim.Render( + "The rest went in. Fix that one and press c again, or carry on without it - "+ + "its page on nan.builders has the manual steps.") + "\n") + return b.String() +} + // A message wrapped to the panel, coloured by whether it is one. A sign-in // link is longer than any terminal. func (m model) wrapped(l layout, msg string) string { @@ -2701,6 +2754,8 @@ func (m model) renderSetup(l layout) string { b.WriteString(l.indent + style.Render(m.setupMsg) + "\n") } + b.WriteString(m.renderFailures(l)) + // And now what. The answer used to be nowhere in the panel. if len(m.configured) > 0 { b.WriteString("\n" + l.indent + titleStyle.Render("Now open them") + "\n\n") @@ -2745,7 +2800,7 @@ func (m model) renderSetup(l layout) string { // ── about renderer ─────────────────────────────────────────────────────────── -const Version = "0.1.17" +const Version = "0.1.18" func renderAbout(l layout) string { var b strings.Builder diff --git a/scripts/install.ps1 b/scripts/install.ps1 index c61c46b..29c269e 100644 --- a/scripts/install.ps1 +++ b/scripts/install.ps1 @@ -118,7 +118,7 @@ function Get-LatestVersion { could not work out the latest version from the GitHub API it rate limits unauthenticated requests, so this is usually temporary wait a few minutes, or pick a version yourself: - & ([scriptblock]::Create((irm https://nan.builders/install.ps1))) -Version v0.1.17 + & ([scriptblock]::Create((irm https://nan.builders/install.ps1))) -Version v0.1.18 the releases are at https://github.com/$Repo/releases "@ } diff --git a/scripts/install.sh b/scripts/install.sh index ad35bb6..5d7e16a 100755 --- a/scripts/install.sh +++ b/scripts/install.sh @@ -76,7 +76,7 @@ require_version() { err "could not work out the latest version from the GitHub API" err "it rate limits unauthenticated requests, so this is usually temporary" err "wait a few minutes, or pick a version yourself:" - printf " VERSION=v0.1.17 curl -fsSL https://nan.builders/install | bash + printf " VERSION=v0.1.18 curl -fsSL https://nan.builders/install | bash " >&2 err "the releases are at https://github.com/$REPO/releases" exit 1