From b619b7787dab0bade47ad9f1ade5cc9b8969e56a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jeffry=20W=C3=BCrmli?= Date: Wed, 7 Oct 2026 11:00:28 +0200 Subject: [PATCH] fix: keep package manager output out of read-only commands --- CHANGELOG.md | 7 ++ internal/cli/apply.go | 12 ++- internal/cli/context.go | 20 ++++- internal/cli/doctor.go | 8 +- internal/cli/list.go | 2 +- internal/cli/quiet_test.go | 147 +++++++++++++++++++++++++++++++++++++ 6 files changed, 192 insertions(+), 4 deletions(-) create mode 100644 internal/cli/quiet_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 712341c..27883e0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,13 @@ Format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). first runtime dependencies, Bubble Tea and Lip Gloss, so the binary grows by about 1.5 MiB. +### Fixed +- `list`, `diff` / `apply --dry-run` and `doctor` no longer print the package + manager's answers (for example `brew list --versions fzf`) into their own + output. `list --json` was not valid JSON on hosts where the package manager + writes to stdout. `apply`, `doctor --fix`, `remove` and `uninstall` still show + installation and hook output. + ## [0.6.0] - 2026-10-07 ### Added diff --git a/internal/cli/apply.go b/internal/cli/apply.go index b02778a..c0b5b90 100644 --- a/internal/cli/apply.go +++ b/internal/cli/apply.go @@ -46,7 +46,17 @@ func runApply(cmd *cobra.Command, forceDryRun bool) error { out := cmd.OutOrStdout() errOut := cmd.ErrOrStderr() - e, cfgPath, lockPath, err := buildEngine(out, errOut) + // A dry run only asks the package manager questions, so what it prints is + // dropped; a real apply streams installs and hooks to the user. + dryRun := forceDryRun + if !forceDryRun { + dryRun, _ = cmd.Flags().GetBool("dry-run") + } + build := buildEngine + if dryRun { + build = buildQueryEngine + } + e, cfgPath, lockPath, err := build(out, errOut) if err != nil { return err } diff --git a/internal/cli/context.go b/internal/cli/context.go index 685e439..8f1d035 100644 --- a/internal/cli/context.go +++ b/internal/cli/context.go @@ -173,7 +173,25 @@ func defaultPrompt(question string) bool { // buildEngine detects the host, loads the module registry (built-in + user), // detects the package manager, and assembles an engine.Engine. It does NOT load // config.toml — commands that need it call config.Load(cfgPath) themselves. +// +// The writers receive the engine's own messages and also whatever the commands +// it runs print (an install's progress, a hook's output). Use it for commands +// that install, remove or run hooks; the user is waiting for that output. func buildEngine(stdout, stderr io.Writer) (e engine.Engine, cfgPath, lockPath string, err error) { + return newEngine(stdout, stderr, stdout, stderr) +} + +// buildQueryEngine is buildEngine for commands that only look: the engine's own +// messages (warnings, plans) still go to stdout and stderr, but what the package +// manager prints while it is being asked about installed packages is dropped. +// Without this, `brew list --versions fzf` lands in the middle of `list --json`. +func buildQueryEngine(stdout, stderr io.Writer) (e engine.Engine, cfgPath, lockPath string, err error) { + return newEngine(stdout, stderr, io.Discard, io.Discard) +} + +// newEngine is the shared body: stdout and stderr are for the engine's own +// messages, runnerOut and runnerErr for the commands its runner executes. +func newEngine(stdout, stderr, runnerOut, runnerErr io.Writer) (e engine.Engine, cfgPath, lockPath string, err error) { env := platform.Env{ GOOS: runtime.GOOS, GOARCH: runtime.GOARCH, @@ -202,7 +220,7 @@ func buildEngine(stdout, stderr io.Writer) (e engine.Engine, cfgPath, lockPath s if runnerOverride != nil { runner = runnerOverride } else { - runner = runnerFactory(stdout, stderr) + runner = runnerFactory(runnerOut, runnerErr) } manager, ok := pkgmgr.DetectManager(runtime.GOOS, runner) diff --git a/internal/cli/doctor.go b/internal/cli/doctor.go index d4852b7..7f70468 100644 --- a/internal/cli/doctor.go +++ b/internal/cli/doctor.go @@ -37,7 +37,13 @@ func runDoctor(cmd *cobra.Command, fix, yes bool) error { out := cmd.OutOrStdout() errOut := cmd.ErrOrStderr() - e, cfgPath, lockPath, err := buildEngine(out, errOut) + // Checking only asks the package manager questions; --fix may re-apply, and + // then a hook's output should reach the user. + build := buildQueryEngine + if fix { + build = buildEngine + } + e, cfgPath, lockPath, err := build(out, errOut) if err != nil { return err } diff --git a/internal/cli/list.go b/internal/cli/list.go index 24185e6..1897ab4 100644 --- a/internal/cli/list.go +++ b/internal/cli/list.go @@ -37,7 +37,7 @@ PACKAGES is one of: Args: cobra.NoArgs, RunE: func(cmd *cobra.Command, _ []string) error { out := cmd.OutOrStdout() - e, cfgPath, _, err := buildEngine(out, cmd.ErrOrStderr()) + e, cfgPath, _, err := buildQueryEngine(out, cmd.ErrOrStderr()) if err != nil { return err } diff --git a/internal/cli/quiet_test.go b/internal/cli/quiet_test.go new file mode 100644 index 0000000..6e50863 --- /dev/null +++ b/internal/cli/quiet_test.go @@ -0,0 +1,147 @@ +package cli_test + +import ( + "bytes" + "encoding/json" + "io" + "strings" + "testing" + + "github.com/JtheGunner/omnishell/internal/cli" + "github.com/JtheGunner/omnishell/internal/config" + "github.com/JtheGunner/omnishell/internal/pkgmgr" +) + +const probeNoise = "NOISE-FROM-PACKAGE-MANAGER" + +// chattyRunner behaves like pkgmgr.ExecRunner as far as output goes: every +// command it runs writes a line to the writer it was built with, the way brew +// and dpkg-query print to the terminal. Every tool counts as installed and on +// PATH, so a package manager is detected and every package gets probed. +type chattyRunner struct { + out io.Writer + calls *int +} + +func (r chattyRunner) Run(name string, _ ...string) ([]byte, error) { + *r.calls++ + _, _ = io.WriteString(r.out, probeNoise+" from "+name+"\n") + return []byte("install ok installed"), nil +} + +func (r chattyRunner) Look(name string) (string, error) { return "/usr/bin/" + name, nil } + +// writers records what a command hands to the runner factory. +type writers struct{ out, err io.Writer } + +// chattyEngine installs a runner factory that records the writers it is given +// and builds a chattyRunner on the stdout writer. It returns the recorded +// writers and the number of commands the runner ran. +func chattyEngine(t *testing.T) (*writers, *int) { + t.Helper() + got, calls := &writers{}, new(int) + cli.SetRunnerForTest(nil) // setupModuleCLITest installed a mock; the factory below replaces it + cli.SetRunnerFactoryForTest(func(stdout, stderr io.Writer) pkgmgr.Runner { + got.out, got.err = stdout, stderr + return chattyRunner{out: stdout, calls: calls} + }) + t.Cleanup(func() { cli.SetRunnerFactoryForTest(nil) }) + return got, calls +} + +func TestListJSONStaysValidWhenThePackageManagerPrintsToStdout(t *testing.T) { + setTestInit(t) + _, calls := chattyEngine(t) + + var out, errb bytes.Buffer + if code := cli.Execute([]string{"list", "--json"}, &out, &errb); code != 0 { + t.Fatalf("exit = %d (stderr: %s)", code, errb.String()) + } + + if *calls == 0 { + t.Fatal("setup: the package manager was never asked, so this test proves nothing") + } + var rows []map[string]any + if err := json.Unmarshal(out.Bytes(), &rows); err != nil { + t.Fatalf("list --json is not valid JSON (%v); it starts with:\n%.200s", err, out.String()) + } + if len(rows) == 0 { + t.Fatal("expected module rows") + } + if strings.Contains(errb.String(), probeNoise) { + t.Fatalf("package manager output leaked to stderr:\n%.200s", errb.String()) + } +} + +func TestListTableHasNoPackageManagerLines(t *testing.T) { + setTestInit(t) + _, calls := chattyEngine(t) + + var out, errb bytes.Buffer + if code := cli.Execute([]string{"list"}, &out, &errb); code != 0 { + t.Fatalf("exit = %d (stderr: %s)", code, errb.String()) + } + + if *calls == 0 { + t.Fatal("setup: the package manager was never asked, so this test proves nothing") + } + if strings.Contains(out.String(), probeNoise) { + t.Fatalf("list output contains package manager lines:\n%.300s", out.String()) + } + if !strings.HasPrefix(out.String(), "MODULE") { + t.Fatalf("the table must be the first thing printed, got:\n%.200s", out.String()) + } +} + +// Read-only commands ask the package manager questions; the answers are none of +// the user's business. Commands that install, remove or run hooks keep +// streaming the output, which is what the user is waiting for. +func TestRunnerWritersPerCommand(t *testing.T) { + cases := []struct { + name string + args []string + quiet bool + }{ + {"list", []string{"list"}, true}, + {"list --json", []string{"list", "--json"}, true}, + {"diff", []string{"diff"}, true}, + {"apply --dry-run", []string{"apply", "--dry-run"}, true}, + {"doctor", []string{"doctor"}, true}, + {"apply", []string{"apply", "--yes"}, false}, + {"doctor --fix", []string{"doctor", "--fix", "--yes"}, false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + setTestInit(t) + got, _ := chattyEngine(t) + + var out, errb bytes.Buffer + cli.Execute(tc.args, &out, &errb) // the exit code does not matter here + + if got.out == nil { + t.Fatal("the command never built an engine") + } + quiet := got.out == io.Discard && got.err == io.Discard + if quiet != tc.quiet { + t.Fatalf("runner writers: stdout=%T stderr=%T, want quiet=%v", got.out, got.err, tc.quiet) + } + }) + } +} + +// Quieting the runner must not swallow the engine's own messages: they share +// the writers buildEngine is given. +func TestDiffStillWarnsAboutAnUnknownModule(t *testing.T) { + cfgPath := setTestInit(t) + chattyEngine(t) + if err := config.SetEnabled(cfgPath, "ghost", true); err != nil { + t.Fatalf("SetEnabled: %v", err) + } + + var out, errb bytes.Buffer + cli.Execute([]string{"diff"}, &out, &errb) + + if !strings.Contains(errb.String(), `unknown module "ghost"`) { + t.Fatalf("the engine's warning is missing from stderr:\n%s", errb.String()) + } +}