From 63d9e98751a21d3f5766a6bd83ad5aa1b491e6b6 Mon Sep 17 00:00:00 2001 From: borjaperfra Date: Mon, 14 Sep 2026 17:21:48 +0200 Subject: [PATCH] fix(setup): one tool failing is not the whole step failing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reported from a machine where Hermes will not configure: the last step of the setup stops there and the only way on is escape. Two things were wrong and they compounded. configureTools kept only the last error and threw away everything else it had just done, so the message became "error: " and the four configs that HAD been written went unmentioned - a partial success read as a total failure. And the wizard refused to leave its last step while that message was showing, so the member was held on a screen that said their setup had failed when most of it had not. Failures are per tool now, with the name on them, and the summary carries both halves: "4 added · Hermes failed". Only the first line of what a tool printed goes in it, because a tool configured by running it hands back whatever it felt like printing. The step finishes either way. What failed is shown under the list, with the one sentence that was missing: the rest went in, fix that one and press c again, or carry on without it and its page has the manual steps. Co-Authored-By: Claude Opus 5 (1M context) --- internal/tui/config_test.go | 90 ++++++++++++++++++++++++++++++++++++- internal/tui/tui.go | 85 ++++++++++++++++++++++++++++------- scripts/install.ps1 | 2 +- scripts/install.sh | 2 +- 4 files changed, 160 insertions(+), 19 deletions(-) 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