diff --git a/AGENTS.md b/AGENTS.md index d4ef650..49acf06 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -34,6 +34,7 @@ Skipping these steps leads to pattern violations, broken dual-mode, and pricing - **Interactive hint bar** — every direct `prompter.Select(...)` outside the wizard engine must pass `tui.WithShowHints(true)` (and the equivalent option on `MultiSelect`) so the prompt renders its key hints below the choices. Wizard steps are exempt — the composite already renders the hint bar - **Ctrl+C exits immediately, no confirmation** — use `cmdutil.IsPromptCancel(err)` to detect either Esc or Ctrl+C and return cleanly. When a flow needs different behavior per key (e.g. a "Back to list / Exit" gate where Esc means back), split with `IsPromptInterrupt(err)` (Ctrl+C) and `IsPromptBack(err)` (Esc). Never show an "Exit?" confirmation dialog — Unix users expect Ctrl+C to be terminal - **`pkg/` is in-tree** — the TUI core (`pkg/tui*`), `pkg/log`, `pkg/version` are part of this repo; edit them directly +- **Never run the binary against the real config dir** — every manual, scripted, or pty-driven `./bin/verda` run sets `VERDA_HOME=$(mktemp -d)` (or uses `make run.sandbox`). `VERDA_SHARED_CREDENTIALS_FILE` is not enough; it leaves `config.yaml` and `EnsureVerdaDir` pointing at the real `~/.verda`. Driving `auth login` to completion once overwrote a developer's real credentials, and a clobbered client secret cannot be recovered from the API. See CLAUDE.md § "NEVER run the binary against the real config dir" - **Commit only when asked** — don't auto-commit ## Risky Areas — Slow Down @@ -44,6 +45,7 @@ Skipping these steps leads to pattern violations, broken dual-mode, and pricing | `options/credentials.go` | Break auth = break everything | Test all profiles, expired tokens | | Agent mode (`--agent`) | JSON contract change = break downstream | Check structured error format | | Wizard steps | Step ordering, cache invalidation | Map dependencies before coding | +| Running `auth login` / any binary run | Overwrites the real `~/.verda`; lost secrets are unrecoverable | Set `VERDA_HOME=$(mktemp -d)` first, always | ## Done Checklist @@ -54,5 +56,6 @@ Skipping these steps leads to pattern violations, broken dual-mode, and pricing - [ ] Interactive and non-interactive modes both work - [ ] Interactive Selects pass `tui.WithShowHints(true)` so the hint bar renders - [ ] No leftover debug code, TODOs, or commented-out blocks +- [ ] Every manual/pty run of the binary set `VERDA_HOME` to a temp dir — the real `~/.verda` is untouched If `make lint` reports issues, fix them *before* announcing completion. See `CLAUDE.md` § "Go House Style" for the patterns that prevent the common hits (http.NoBody, American spelling, reused constants, rangeValCopy, nilerr annotations, etc.). diff --git a/CLAUDE.md b/CLAUDE.md index 9d6b02d..c3c971e 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -174,6 +174,31 @@ If you modified a command, also verify: - `--agent -o json` mode works (structured output, no TUI) - `--debug` shows request/response payloads +### NEVER run the binary against the real config dir + +Any manual, scripted, or pty-driven run of `./bin/verda` MUST set `VERDA_HOME` to a +throwaway directory: + +```bash +VERDA_HOME=$(mktemp -d) ./bin/verda # or: make run.sandbox ARGS="" +``` + +`VERDA_HOME` (see `options.VerdaDir`) redirects the whole config dir — credentials *and* +`config.yaml`. `VERDA_SHARED_CREDENTIALS_FILE` covers only the credentials file, so +`auth use`, `settings`, and `EnsureVerdaDir` still hit the real `~/.verda`. Use +`VERDA_HOME`. + +This is not hypothetical: driving the `auth login` wizard to completion to verify a TUI +fix overwrote a developer's real `~/.verda/credentials` with test values. +`auth login` replaces an existing profile with no warning — the documented re-auth +behavior — and **a client secret cannot be read back from the API, so a clobber is +unrecoverable**. Assume any command may write to the config dir, not just the obviously +auth-shaped ones. + +The repo's own suites already do this — copy them, don't hand-roll a harness: +`tests/contract/main_test.go` (`cliEnv` strips every inherited `VERDA_*`, then sets +`VERDA_HOME=t.TempDir()`) and `options/registry_credentials_test.go:168`. + ## Other Agents This repo targets Claude Code and OpenAI Codex. Claude auto-loads this file; Codex auto-loads `AGENTS.md` (execution contract). A `.cursor/rules/main.mdc` pointer exists for Cursor users but is not a primary target — if Cursor drops out of the stack, delete it rather than letting it drift. diff --git a/Makefile b/Makefile index 2a1c5a1..6debe0e 100644 --- a/Makefile +++ b/Makefile @@ -1,6 +1,6 @@ OUTPUT_DIR ?= bin -.PHONY: all build clean lint lint.fix security test test.integration test-s3-integration fmt changelog changelog.unreleased hooks.install pre-commit help +.PHONY: all build clean run.sandbox lint lint.fix security test test.integration test-s3-integration fmt changelog changelog.unreleased hooks.install pre-commit help ## Build ------------------------------------------------------------------- @@ -14,6 +14,12 @@ build: ## Build the binary into bin/ clean: ## Remove build artifacts @rm -rf $(OUTPUT_DIR) +# Never drive the binary against the real ~/.verda: auth login replaces a profile +# with no warning, and a clobbered client secret cannot be read back from the API. +# VERDA_HOME redirects the whole config dir; VERDA_SHARED_CREDENTIALS_FILE does not. +run.sandbox: build ## Run the binary against a throwaway config dir, e.g. make run.sandbox ARGS="auth login" + @dir=$$(mktemp -d) && echo "VERDA_HOME=$$dir" && VERDA_HOME=$$dir $(OUTPUT_DIR)/verda $(ARGS) + ## Quality ----------------------------------------------------------------- lint: ## Run golangci-lint on all packages diff --git a/go.mod b/go.mod index 6c4caa6..972e45b 100644 --- a/go.mod +++ b/go.mod @@ -1,6 +1,6 @@ module github.com/verda-cloud/verda-cli -go 1.25.12 +go 1.25.13 require ( charm.land/lipgloss/v2 v2.0.2 diff --git a/internal/verda-cli/cmd/auth/CLAUDE.md b/internal/verda-cli/cmd/auth/CLAUDE.md index a0b263d..261f5cc 100644 --- a/internal/verda-cli/cmd/auth/CLAUDE.md +++ b/internal/verda-cli/cmd/auth/CLAUDE.md @@ -12,6 +12,7 @@ - `path.go` -- Helpers: `resolveCredentialsFile`, `defaultConfigFilePath` - `auth_test.go` -- Tests for `writeActiveProfile`, `resolveCredentialsFile` - `wizard_test.go` -- Wizard flow tests with mock prompter + - `login_test.go` -- Flag-driven write path: new/named profile, merge, re-auth overwrite, 0600, flag-over-env ## Domain-Specific Logic - Credentials file resolution order: explicit flag > `VERDA_SHARED_CREDENTIALS_FILE` env var > `options.DefaultCredentialsFilePath()` @@ -28,6 +29,18 @@ - The `selectThemeWizard` pattern of returning `nil` on wizard error (user cancel) is NOT used here -- login returns the wizard error directly. - `writeActiveProfile` in `use.go` merges into existing config YAML rather than overwriting the whole file. - `login` creates the `~/.verda/` directory via `options.EnsureVerdaDir()` before saving. +- **Never run `auth login` against the real config dir.** Set `VERDA_HOME` to a temp dir for + any manual or pty-driven run (`make run.sandbox ARGS="auth login"`). Re-running login + replaces an existing profile with no warning -- intentional, it is the re-auth path -- + and a client secret cannot be read back from the API, so a clobber is unrecoverable. + `VERDA_SHARED_CREDENTIALS_FILE` alone is insufficient: `EnsureVerdaDir()` resolves + through `VerdaDir()` and would still mkdir the real `~/.verda`. Tests must set + `VERDA_HOME` for the same reason. +- `login_test.go` covers only the flag-driven path. Supplying both `--client-id` and + `--client-secret` is what skips the wizard, so the post-wizard validation gate is + unreachable from a test -- the engine is constructed inline in `RunE`, and a wizard in a + test would start a real `tea.Program` against the developer's stdin. Covering that gate + means injecting the engine. ## Relationships - `cmdutil.Factory` / `cmdutil.IOStreams` -- standard dependency injection diff --git a/internal/verda-cli/cmd/auth/login_test.go b/internal/verda-cli/cmd/auth/login_test.go new file mode 100644 index 0000000..7b39d13 --- /dev/null +++ b/internal/verda-cli/cmd/auth/login_test.go @@ -0,0 +1,216 @@ +// Copyright 2026 Verda Cloud Oy +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package auth + +import ( + "bytes" + "os" + "path/filepath" + "runtime" + "strings" + "testing" + + cmdutil "github.com/verda-cloud/verda-cli/internal/verda-cli/cmd/util" + "github.com/verda-cloud/verda-cli/internal/verda-cli/options" +) + +// These tests cover the flag-driven write path only. Supplying both +// --client-id and --client-secret is what skips the wizard (login.go checks +// them with OR), and a wizard here would start a real tea.Program against the +// developer's stdin — a hang when `go test` runs from a terminal. The +// post-wizard "still empty" validation gate is therefore unreachable from a +// test; covering it needs the engine injected rather than constructed inline. + +// sandboxHome points the whole config dir at a temp tree. VERDA_HOME, not +// VERDA_SHARED_CREDENTIALS_FILE: login calls options.EnsureVerdaDir, which +// resolves through VerdaDir and would mkdir the developer's real ~/.verda even +// with the credentials path redirected elsewhere. +func sandboxHome(t *testing.T) string { + t.Helper() + dir := t.TempDir() + t.Setenv("VERDA_HOME", dir) + t.Setenv("VERDA_SHARED_CREDENTIALS_FILE", filepath.Join(dir, "credentials")) + return dir +} + +func runAuthLoginForTest(t *testing.T, args ...string) error { + t.Helper() + streams := cmdutil.IOStreams{Out: &bytes.Buffer{}, ErrOut: &bytes.Buffer{}} + cmd := NewCmdLogin(cmdutil.NewTestFactory(nil), streams) + cmd.SetArgs(args) + cmd.SetOut(streams.Out) + cmd.SetErr(streams.ErrOut) + cmd.SilenceUsage = true + cmd.SilenceErrors = true + return cmd.Execute() +} + +func loadProfile(t *testing.T, path, profile string) *options.SharedCredentials { + t.Helper() + creds, err := options.LoadSharedCredentialsForProfile(path, profile) + if err != nil { + t.Fatalf("LoadSharedCredentialsForProfile(%q, %q): %v", path, profile, err) + } + return creds +} + +func TestLoginWritesNewProfile(t *testing.T) { + dir := sandboxHome(t) + path := filepath.Join(dir, "credentials") + + if err := runAuthLoginForTest(t, "--client-id", "id-1", "--client-secret", "secret-1"); err != nil { + t.Fatalf("login: %v", err) + } + + got := loadProfile(t, path, "default") + if got.ClientID != "id-1" { + t.Errorf("ClientID = %q, want id-1", got.ClientID) + } + if got.ClientSecret != "secret-1" { + t.Errorf("ClientSecret = %q, want secret-1", got.ClientSecret) + } + if got.BaseURL != defaultBaseURL { + t.Errorf("BaseURL = %q, want %q", got.BaseURL, defaultBaseURL) + } +} + +// A leaked secret is not recoverable, so the 0600 is load-bearing rather than +// cosmetic. Windows has no mode bits to assert. +func TestLoginRestrictsFilePermissions(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("no Unix mode bits on Windows") + } + dir := sandboxHome(t) + path := filepath.Join(dir, "credentials") + + if err := runAuthLoginForTest(t, "--client-id", "id", "--client-secret", "secret"); err != nil { + t.Fatalf("login: %v", err) + } + + info, err := os.Stat(path) + if err != nil { + t.Fatalf("stat: %v", err) + } + if perm := info.Mode().Perm(); perm != 0o600 { + t.Errorf("mode = %#o, want 0600", perm) + } +} + +func TestLoginWritesNamedProfileAndBaseURL(t *testing.T) { + dir := sandboxHome(t) + path := filepath.Join(dir, "credentials") + + err := runAuthLoginForTest(t, + "--profile", "staging", + "--base-url", "https://staging-api.verda.com/v1", + "--client-id", "stg-id", + "--client-secret", "stg-secret", + ) + if err != nil { + t.Fatalf("login: %v", err) + } + + got := loadProfile(t, path, "staging") + if got.BaseURL != "https://staging-api.verda.com/v1" { + t.Errorf("BaseURL = %q", got.BaseURL) + } + if got.ClientID != "stg-id" { + t.Errorf("ClientID = %q, want stg-id", got.ClientID) + } + + if _, err := options.LoadSharedCredentialsForProfile(path, "default"); err == nil { + t.Error("a [default] section appeared; --profile must write only the named section") + } +} + +// The writer merges into the existing INI. Dropping unrelated profiles would +// destroy credentials the user cannot recover from the API. +func TestLoginPreservesOtherProfiles(t *testing.T) { + dir := sandboxHome(t) + path := filepath.Join(dir, "credentials") + + seed := "[other]\n" + + "verda_base_url = https://other.verda.com/v1\n" + + "verda_client_id = other-id\n" + + "verda_client_secret = other-secret\n" + if err := os.WriteFile(path, []byte(seed), 0o600); err != nil { + t.Fatalf("seed: %v", err) + } + + if err := runAuthLoginForTest(t, "--client-id", "new-id", "--client-secret", "new-secret"); err != nil { + t.Fatalf("login: %v", err) + } + + other := loadProfile(t, path, "other") + if other.ClientID != "other-id" || other.ClientSecret != "other-secret" { + t.Errorf("[other] was modified: %+v", other) + } + if added := loadProfile(t, path, "default"); added.ClientID != "new-id" { + t.Errorf("[default] ClientID = %q, want new-id", added.ClientID) + } +} + +// Re-running login against a profile is the documented re-auth path: rotating a +// secret must replace the stored one, not append or refuse. +func TestLoginOverwritesSameProfile(t *testing.T) { + dir := sandboxHome(t) + path := filepath.Join(dir, "credentials") + + if err := runAuthLoginForTest(t, "--client-id", "old", "--client-secret", "old-secret"); err != nil { + t.Fatalf("first login: %v", err) + } + if err := runAuthLoginForTest(t, "--client-id", "new", "--client-secret", "new-secret"); err != nil { + t.Fatalf("second login: %v", err) + } + + got := loadProfile(t, path, "default") + if got.ClientID != "new" || got.ClientSecret != "new-secret" { + t.Errorf("re-login did not replace credentials: %+v", got) + } + + data, err := os.ReadFile(path) //nolint:gosec // test-owned temp file + if err != nil { + t.Fatalf("read: %v", err) + } + if n := strings.Count(string(data), "[default]"); n != 1 { + t.Errorf("found %d [default] sections, want 1", n) + } + if strings.Contains(string(data), "old-secret") { + t.Error("the replaced secret is still present in the file") + } +} + +// --credentials-file outranks VERDA_SHARED_CREDENTIALS_FILE; sandboxHome sets +// the env var, so a write landing at the flag path proves the precedence. +func TestLoginCredentialsFileFlagWinsOverEnv(t *testing.T) { + dir := sandboxHome(t) + flagPath := filepath.Join(dir, "explicit-credentials") + + err := runAuthLoginForTest(t, + "--credentials-file", flagPath, + "--client-id", "flag-id", + "--client-secret", "flag-secret", + ) + if err != nil { + t.Fatalf("login: %v", err) + } + + if got := loadProfile(t, flagPath, "default"); got.ClientID != "flag-id" { + t.Errorf("ClientID = %q, want flag-id", got.ClientID) + } + if _, err := os.Stat(filepath.Join(dir, "credentials")); !os.IsNotExist(err) { + t.Error("the env-var path was written despite --credentials-file") + } +} diff --git a/internal/verda-cli/cmd/util/iostreams.go b/internal/verda-cli/cmd/util/iostreams.go index f84ff1a..1d7b633 100644 --- a/internal/verda-cli/cmd/util/iostreams.go +++ b/internal/verda-cli/cmd/util/iostreams.go @@ -30,6 +30,34 @@ type IOStreams struct { ErrOut io.Writer } +// terminalWriter is a colorprofile writer that still answers Fd(). +// +// The fd must stay reachable through the wrapper: bubbletea and this repo's own +// rendersToTerminal both identify a terminal by asserting the writer to +// term.File and asking for its descriptor. A bare colorprofile.Writer hides it, +// which costs bubbletea term.GetSize — leaving every prompt rendering into a +// 0x0 viewport (a blank screen that looks like a hang) and silencing every +// spinner, progress bar and pager. +// +// Write is promoted from the embedded colorprofile.Writer, so ANSI is still +// downsampled or stripped to suit the destination. +type terminalWriter struct { + *colorprofile.Writer + file *os.File +} + +func newTerminalWriter(f *os.File) *terminalWriter { + return &terminalWriter{Writer: colorprofile.NewWriter(f, os.Environ()), file: f} +} + +func (w *terminalWriter) Fd() uintptr { return w.file.Fd() } + +// Read and Close exist only to satisfy term.File; nothing in the stack calls +// either on an output stream. Close is a deliberate no-op — closing the +// process's own stdout or stderr is never what a caller wants. +func (w *terminalWriter) Read(p []byte) (int, error) { return w.file.Read(p) } +func (w *terminalWriter) Close() error { return nil } + // NewStdIOStreams returns an IOStreams wired to os.Stdin, os.Stdout, and os.Stderr. // // Both writers are wrapped in a colorprofile writer, which detects what the @@ -43,8 +71,8 @@ type IOStreams struct { func NewStdIOStreams() IOStreams { return IOStreams{ In: os.Stdin, - Out: colorprofile.NewWriter(os.Stdout, os.Environ()), - ErrOut: colorprofile.NewWriter(os.Stderr, os.Environ()), + Out: newTerminalWriter(os.Stdout), + ErrOut: newTerminalWriter(os.Stderr), } } diff --git a/internal/verda-cli/cmd/util/iostreams_test.go b/internal/verda-cli/cmd/util/iostreams_test.go index e26a90d..7f1605d 100644 --- a/internal/verda-cli/cmd/util/iostreams_test.go +++ b/internal/verda-cli/cmd/util/iostreams_test.go @@ -17,12 +17,14 @@ package util import ( "bytes" "fmt" + "io" "os" "strings" "testing" "charm.land/lipgloss/v2" "github.com/charmbracelet/colorprofile" + "github.com/charmbracelet/x/term" ) // styled is what every table and card in this CLI produces: lipgloss always @@ -84,27 +86,46 @@ func TestColorProfileWriterKeepsColorWhenSupported(t *testing.T) { } } -// Pins the wiring itself: unwrapping either stream silently reintroduces ANSI on -// every piped command, which no per-command test would notice. +// Pins both halves of the wiring, each invisible to a per-command test: +// unwrapping a stream reintroduces ANSI on every piped command, and hiding the +// fd behind the wrapper costs bubbletea term.GetSize — which blanks every +// prompt, spinner and pager on a real terminal. func TestNewStdIOStreamsWrapsBothWriters(t *testing.T) { t.Parallel() s := NewStdIOStreams() - out, ok := s.Out.(*colorprofile.Writer) - if !ok { - t.Fatalf("Out is %T, want *colorprofile.Writer", s.Out) + cases := []struct { + name string + w io.Writer + file *os.File + }{ + {"Out", s.Out, os.Stdout}, + {"ErrOut", s.ErrOut, os.Stderr}, } - if out.Forward != os.Stdout { - t.Errorf("Out forwards to %v, want os.Stdout", out.Forward) - } - - errOut, ok := s.ErrOut.(*colorprofile.Writer) - if !ok { - t.Fatalf("ErrOut is %T, want *colorprofile.Writer", s.ErrOut) - } - if errOut.Forward != os.Stderr { - t.Errorf("ErrOut forwards to %v, want os.Stderr", errOut.Forward) + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + tw, ok := tc.w.(*terminalWriter) + if !ok { + t.Fatalf("%s is %T, want *terminalWriter", tc.name, tc.w) + } + if tw.Writer == nil { + t.Fatalf("%s has no colorprofile writer; ANSI would survive a pipe", tc.name) + } + if tw.Forward != tc.file { + t.Errorf("%s forwards to %v, want %v", tc.name, tw.Forward, tc.file) + } + + f, ok := tc.w.(term.File) + if !ok { + t.Fatalf("%s does not satisfy term.File; bubbletea renders into a 0x0 viewport", tc.name) + } + if f.Fd() != tc.file.Fd() { + t.Errorf("%s Fd() = %d, want %d", tc.name, f.Fd(), tc.file.Fd()) + } + }) } if s.In != os.Stdin { @@ -112,6 +133,18 @@ func TestNewStdIOStreamsWrapsBothWriters(t *testing.T) { } } +// Close must not take the process's stdout with it. +func TestTerminalWriterCloseIsNoop(t *testing.T) { + t.Parallel() + + if err := newTerminalWriter(os.Stdout).Close(); err != nil { + t.Fatalf("Close() = %v, want nil", err) + } + if _, err := fmt.Fprint(io.Discard, "still usable"); err != nil { + t.Fatalf("stdout unusable after Close: %v", err) + } +} + // Under test the profile is derived from the destination, so a buffer resolves to // something that cannot show color. If this ever flips, the strip test above // would pass for the wrong reason. diff --git a/internal/verda-cli/cmd/volume/trash_test.go b/internal/verda-cli/cmd/volume/trash_test.go index 5c2775e..4058279 100644 --- a/internal/verda-cli/cmd/volume/trash_test.go +++ b/internal/verda-cli/cmd/volume/trash_test.go @@ -22,17 +22,29 @@ import ( "net/http/httptest" "strings" "testing" + "time" "github.com/verda-cloud/verdacloud-sdk-go/pkg/verda" cmdutil "github.com/verda-cloud/verda-cli/internal/verda-cli/cmd/util" ) -const trashBody = `[{"id":"vol-1","name":"box-a-os","size":50,"type":"NVMe_Shared",` + - `"location":"FIN-00","contract":"PAY_AS_YOU_GO","is_os_volume":true,` + - `"monthly_price":10,"currency":"usd","deleted_at":"2026-08-11T18:51:12Z"},` + - `{"id":"vol-2","name":"undated","size":20,"type":"NVMe_Shared",` + - `"location":"FIN-00","contract":"PAY_AS_YOU_GO","is_os_volume":false}]` +// trash.go counts down from deleted_at + 96h and prints "Expires:" only while +// that window is still open, so the fixture's timestamp must be relative to now. +// A pinned date silently stops rendering the countdown once it ages out — this +// fixture was written with a hardcoded 2026-08-11 and began failing on +// 2026-08-15, four days later, having passed in CI the whole time in between. +func recentDeletion() time.Time { + return time.Now().UTC().Add(-time.Hour).Truncate(time.Second) +} + +func trashBodyDeletedAt(deletedAt time.Time) string { + return `[{"id":"vol-1","name":"box-a-os","size":50,"type":"NVMe_Shared",` + + `"location":"FIN-00","contract":"PAY_AS_YOU_GO","is_os_volume":true,` + + `"monthly_price":10,"currency":"usd","deleted_at":"` + deletedAt.Format(time.RFC3339) + `"},` + + `{"id":"vol-2","name":"undated","size":20,"type":"NVMe_Shared",` + + `"location":"FIN-00","contract":"PAY_AS_YOU_GO","is_os_volume":false}]` +} func runTrashCmd(t *testing.T, body, format string, agent bool) string { t.Helper() @@ -81,7 +93,8 @@ func runTrashCmd(t *testing.T, body, format string, agent bool) string { func TestTrashHonorsJSONOutput(t *testing.T) { t.Parallel() - got := runTrashCmd(t, trashBody, "json", true) + deletedAt := recentDeletion() + got := runTrashCmd(t, trashBodyDeletedAt(deletedAt), "json", true) if strings.ContainsRune(got, '\033') { t.Errorf("JSON output carries ANSI escapes:\n%q", got) @@ -93,8 +106,8 @@ func TestTrashHonorsJSONOutput(t *testing.T) { if len(rows) != 2 { t.Fatalf("len = %d, want 2", len(rows)) } - if rows[0]["deleted_at"] != "2026-08-11T18:51:12Z" { - t.Errorf("deleted_at = %v, want it preserved", rows[0]["deleted_at"]) + if rows[0]["deleted_at"] != deletedAt.Format(time.RFC3339) { + t.Errorf("deleted_at = %v, want %v preserved", rows[0]["deleted_at"], deletedAt.Format(time.RFC3339)) } if _, ok := rows[1]["deleted_at"]; ok { t.Errorf("undated volume carries deleted_at: %v", rows[1]) @@ -116,7 +129,8 @@ func TestTrashHonorsJSONOutput(t *testing.T) { func TestTrashTableMarksAbsentDeletedAt(t *testing.T) { t.Parallel() - got := runTrashCmd(t, trashBody, "table", true) + deletedAt := recentDeletion() + got := runTrashCmd(t, trashBodyDeletedAt(deletedAt), "table", true) if strings.Contains(got, "0001") { t.Errorf("table emits a zero timestamp:\n%s", got) @@ -124,8 +138,8 @@ func TestTrashTableMarksAbsentDeletedAt(t *testing.T) { if !strings.Contains(got, "2 volume(s) in trash") { t.Errorf("missing the count line:\n%s", got) } - if !strings.Contains(got, "11 Aug 2026") { - t.Errorf("real deleted_at not rendered:\n%s", got) + if want := deletedAt.Format("2 Jan 2006, 15:04"); !strings.Contains(got, want) { + t.Errorf("real deleted_at %q not rendered:\n%s", want, got) } if !strings.Contains(got, "Deleted: -\n") { t.Errorf("absent timestamp not rendered as %q:\n%s", "-", got) @@ -141,7 +155,7 @@ func TestTrashTableMarksAbsentDeletedAt(t *testing.T) { func TestTrashTableHasNoANSIWhenNotATerminal(t *testing.T) { t.Parallel() - got := runTrashCmd(t, trashBody, "table", true) + got := runTrashCmd(t, trashBodyDeletedAt(recentDeletion()), "table", true) if strings.ContainsRune(got, '\033') { t.Errorf("table output carries ANSI escapes:\n%q", got) } @@ -163,7 +177,7 @@ func TestTrashEmpty(t *testing.T) { func TestTrashTableCarriesPriceDisclaimer(t *testing.T) { t.Parallel() - got := runTrashCmd(t, trashBody, "table", true) + got := runTrashCmd(t, trashBodyDeletedAt(recentDeletion()), "table", true) if !strings.Contains(got, cmdutil.PriceDisclaimer) { t.Errorf("missing the price disclaimer:\n%s", got) } diff --git a/pkg/tui/bubbletea/spinner.go b/pkg/tui/bubbletea/spinner.go index bb9be18..fcf5aa7 100644 --- a/pkg/tui/bubbletea/spinner.go +++ b/pkg/tui/bubbletea/spinner.go @@ -18,7 +18,6 @@ import ( "context" "fmt" "io" - "os" "sync" "charm.land/bubbles/v2/spinner" @@ -166,8 +165,12 @@ func (silentProgress) Interrupted() bool { return false } // rendersToTerminal reports whether w is a terminal — the precondition for // animated UI (spinner/progress) to be visible instead of polluting a pipe. +// Matches term.File rather than *os.File: the CLI hands us a writer that wraps +// the stream to strip ANSI, and a wrapper that forwards Fd() is still a +// terminal. Asserting the concrete type instead silently disables every +// spinner, progress bar and pager on a real tty. func rendersToTerminal(w io.Writer) bool { - f, ok := w.(*os.File) + f, ok := w.(term.File) return ok && term.IsTerminal(f.Fd()) } diff --git a/pkg/tui/bubbletea/terminal_detect_test.go b/pkg/tui/bubbletea/terminal_detect_test.go new file mode 100644 index 0000000..be339e8 --- /dev/null +++ b/pkg/tui/bubbletea/terminal_detect_test.go @@ -0,0 +1,62 @@ +package bubbletea + +import ( + "bytes" + "os" + "testing" +) + +// fdWriter is the shape the CLI hands the prompter: a writer that filters ANSI +// on the way out while still exposing the underlying descriptor. +type fdWriter struct { + bytes.Buffer + fd uintptr +} + +func (w *fdWriter) Fd() uintptr { return w.fd } +func (w *fdWriter) Read(_ []byte) (int, error) { return 0, nil } +func (w *fdWriter) Close() error { return nil } + +// rendersToTerminal must follow the fd, not the concrete type. Matching only +// *os.File made it answer false for every wrapped stream, which silently +// disabled every spinner, progress bar and pager on a real terminal. +func TestRendersToTerminalSeesThroughAWrapper(t *testing.T) { + t.Parallel() + + tty, err := os.OpenFile("/dev/tty", os.O_WRONLY, 0) + if err != nil { + t.Skipf("no controlling terminal available: %v", err) + } + defer func() { _ = tty.Close() }() + + if !rendersToTerminal(tty) { + t.Fatal("a raw *os.File tty was not detected as a terminal") + } + if !rendersToTerminal(&fdWriter{fd: tty.Fd()}) { + t.Error("a wrapper forwarding a tty fd was not detected as a terminal") + } +} + +// The non-terminal answer has to stay false, or piped runs start launching +// alt-screen programs against a pipe. +func TestRendersToTerminalRejectsNonTerminals(t *testing.T) { + t.Parallel() + + if rendersToTerminal(&bytes.Buffer{}) { + t.Error("a bytes.Buffer was treated as a terminal") + } + + r, w, err := os.Pipe() + if err != nil { + t.Fatalf("os.Pipe: %v", err) + } + defer func() { _ = r.Close() }() + defer func() { _ = w.Close() }() + + if rendersToTerminal(w) { + t.Error("a pipe was treated as a terminal") + } + if rendersToTerminal(&fdWriter{fd: w.Fd()}) { + t.Error("a wrapper forwarding a pipe fd was treated as a terminal") + } +} diff --git a/pkg/tui/wizard/engine.go b/pkg/tui/wizard/engine.go index 56c3cc4..29a1328 100644 --- a/pkg/tui/wizard/engine.go +++ b/pkg/tui/wizard/engine.go @@ -89,6 +89,10 @@ type Engine struct { resultOverride chan promptResult // test-only: bypasses composite model program *tea.Program // the running composite program (nil in test mode) resultCh chan promptResult // channel for receiving prompt results + // progErrCh carries the composite program's Run error. Nil in test mode. + // The engine's answer to a prompt only ever arrives from a live program, so + // its exit has to be a wait condition too — see awaitPromptResult. + progErrCh chan error // validationMsg is set when a step fails Validate; printed above the // re-drawn prompt so a rejected answer isn't a silent redraw. @@ -210,6 +214,7 @@ func (e *Engine) Run(ctx context.Context, flow *Flow) error { // In test mode (WithTestResults), bypass the composite program entirely. e.resultCh = e.resultOverride e.program = nil + e.progErrCh = nil // Turn SIGINT into a context cancellation so Ctrl+C works during // Loader execution, when the terminal is in cooked mode and no @@ -273,11 +278,7 @@ func (e *Engine) runPersistentProgram(ctx context.Context) error { tea.WithInput(e.reader), } e.program = tea.NewProgram(&composite, progOpts...) - progDone := make(chan struct{}) - go func() { - defer close(progDone) - _, _ = e.program.Run() - }() + progDone := e.runProgram() defer func() { e.program.Quit() <-progDone @@ -339,7 +340,7 @@ func (e *Engine) stepLoop(ctx context.Context) error { // In per-prompt mode (no persistent program), start a fresh program. perPrompt := e.program == nil && e.resultOverride == nil - var progDone chan struct{} + var progDone chan error if perPrompt { progDone = e.startProgram() } @@ -358,7 +359,13 @@ func (e *Engine) stepLoop(ctx context.Context) error { } // Wait for result from composite and process it. - result := <-e.resultCh + result, waitErr := e.awaitPromptResult() + if waitErr != nil { + if perPrompt { + e.stopProgram(progDone) + } + return waitErr + } done, err := e.handlePromptResult(result, step, choices, canGoBack) if perPrompt { @@ -383,7 +390,7 @@ func (e *Engine) stepLoop(ctx context.Context) error { // startProgram creates and starts a new composite tea.Program for one prompt. // Returns a channel that closes when the program exits. // In test mode (resultOverride set), this is a no-op. -func (e *Engine) startProgram() chan struct{} { +func (e *Engine) startProgram() chan error { if e.resultOverride != nil { return nil // test mode — no real program } @@ -398,17 +405,50 @@ func (e *Engine) startProgram() chan struct{} { progOpts = append(progOpts, tea.WithInput(e.reader)) } e.program = tea.NewProgram(&composite, progOpts...) - done := make(chan struct{}) + return e.runProgram() +} + +// runProgram runs e.program in the background and publishes its exit on a +// channel that is closed after the error is queued, so a second receiver (the +// stopProgram / defer wait) never blocks on an already-drained send. +func (e *Engine) runProgram() chan error { + done := make(chan error, 1) + e.progErrCh = done go func() { defer close(done) - _, _ = e.program.Run() + _, err := e.program.Run() + done <- err }() return done } +// awaitPromptResult blocks for the composite's answer, treating the program +// exiting first as fatal: only a live program writes resultCh, so a bare +// receive turns any startup failure into a permanent hang with a blank screen. +// Bubbletea fails this way when it cannot claim the terminal — a non-file +// output, an OpenTTY error, a panic inside Run. +func (e *Engine) awaitPromptResult() (promptResult, error) { + select { + case result := <-e.resultCh: + return result, nil + case runErr := <-e.progErrCh: + // The composite may have queued an answer just before exiting; a + // delivered result outranks the exit. + select { + case result := <-e.resultCh: + return result, nil + default: + } + if runErr != nil { + return promptResult{}, fmt.Errorf("wizard prompt program exited: %w", runErr) + } + return promptResult{}, errors.New("wizard prompt program exited without returning a result") + } +} + // stopProgram quits the composite program and waits for it to fully exit // so the terminal is restored before the next Loader or prompt. -func (e *Engine) stopProgram(done chan struct{}) { +func (e *Engine) stopProgram(done chan error) { if e.program == nil { return }