diff --git a/cmd/unified_selector_guard_test.go b/cmd/unified_selector_guard_test.go new file mode 100644 index 0000000..2152aa6 --- /dev/null +++ b/cmd/unified_selector_guard_test.go @@ -0,0 +1,77 @@ +package cmd + +import ( + "errors" + "testing" + + "github.com/aaearon/grant-cli/internal/ui" +) + +// TestUIUnifiedSelector_EmptyItemsGuard pins the two entry guards on +// uiUnifiedSelector.SelectItem, including the order they run in. +// +// The empty-items guard used to have an equivalent in ui.SelectGroup; PR #68 +// deleted that function as dead code and its test went with it, leaving this +// guard unpinned. Note the message is "available", which is what distinguishes +// it from the earlier "...found, check your SCA policies" guard in +// resolveUnifiedSelection — asserting the exact string keeps the two apart. +// +// Not parallel: mutates the package-global ui.IsTerminalFunc via +// withInteractiveTTY. +func TestUIUnifiedSelector_EmptyItemsGuard(t *testing.T) { + const wantEmptyMsg = "no eligible targets or groups available" + + t.Run("interactive with no items returns the empty-items error", func(t *testing.T) { + withInteractiveTTY(t, true) + withDiscardedStdout(t) + + selector := &uiUnifiedSelector{} + got, err := selector.SelectItem(nil) + if err == nil { + t.Fatalf("SelectItem(nil) returned no error (got item %+v); the len(items)==0 guard is missing", got) + } + if got != nil { + t.Errorf("SelectItem(nil) returned item %+v, want nil", got) + } + if err.Error() != wantEmptyMsg { + t.Errorf("SelectItem(nil) error = %q, want exactly %q", err.Error(), wantEmptyMsg) + } + if errors.Is(err, ui.ErrNotInteractive) { + t.Errorf("SelectItem(nil) in a TTY wrapped ui.ErrNotInteractive: %v", err) + } + }) + + t.Run("interactive with empty slice returns the empty-items error", func(t *testing.T) { + withInteractiveTTY(t, true) + withDiscardedStdout(t) + + selector := &uiUnifiedSelector{} + _, err := selector.SelectItem([]selectionItem{}) + if err == nil { + t.Fatal("SelectItem([]) returned no error; the len(items)==0 guard is missing") + } + if err.Error() != wantEmptyMsg { + t.Errorf("SelectItem([]) error = %q, want exactly %q", err.Error(), wantEmptyMsg) + } + }) + + // Guard order: the interactivity check must run first. With the guards + // swapped, a non-interactive caller with nothing eligible would be told + // "no eligible targets" instead of being pointed at the non-interactive + // flags, which is the wrong remedy. + t.Run("non-interactive with no items reports non-interactive, not empty", func(t *testing.T) { + withInteractiveTTY(t, false) + + selector := &uiUnifiedSelector{} + _, err := selector.SelectItem(nil) + if err == nil { + t.Fatal("SelectItem(nil) when non-interactive returned no error") + } + if !errors.Is(err, ui.ErrNotInteractive) { + t.Errorf("SelectItem(nil) when non-interactive = %v, want ui.ErrNotInteractive (guards may be in the wrong order)", err) + } + if err.Error() == wantEmptyMsg { + t.Errorf("SelectItem(nil) when non-interactive returned the empty-items error %q; the interactivity guard must run first", err.Error()) + } + }) +} diff --git a/docs/mutation-ledger.md b/docs/mutation-ledger.md index a249fdd..3d95afd 100644 --- a/docs/mutation-ledger.md +++ b/docs/mutation-ledger.md @@ -235,7 +235,7 @@ premise does not hold). | UI-05 | internal/ui | `internal/ui/role_selector.go:36` | Make the role sort case-**sensitive** (drop the `strings.ToLower` normalization in the `sort.SliceStable` less-func). Note: the raw mutation orphans the `strings` import — remove it too | CONFIRMED | test | `TestBuildRoleOptions_MixedCaseSort` — the sort lives in `BuildRoleOptions`; no `sortRolesForDisplay` helper exists or was needed | PR7 | done | | UI-06 | internal/ui | `internal/ui/selector.go:95` | Delete the `if len(targets) == 0` guard in `SelectTarget` (`internal/ui/selector.go:49` is the identically-worded guard inside `BuildOptions`, and mutating *that* one is an **equivalent mutant** — `make([]string, 0)` + `sort.Strings` yields the same empty non-nil slice — so it is not an escape). (`SelectRole`/`SelectRequest` equivalents are **killed**; these three are not) | CONFIRMED | test | `TestSelectTarget_EmptyList`; guard **order** (non-interactive first) is pinned separately by `TestSelectTarget_NonTTYEmptyList` (mutation also orphans the `errors` import — removed so the package compiles; survey then fails with "please provide options to select from") | PR7 | done | | UI-07 | internal/ui | `internal/ui/session_selector.go:122` | Delete the `if len(sessions) == 0` guard in `SelectSessions` | CONFIRMED | test | `TestSelectSessions_EmptyList`; guard **order** pinned by `TestSelectSessions_NonTTYEmptyList` (the `errors` import stays live via "no sessions selected"; survey fails with "please provide options to select from") | PR7 | done | -| UI-08 | cmd/root (unified selector) | `cmd/root.go:1005` (`uiUnifiedSelector.SelectItem`). **The original site is gone:** this was recorded against the `if len(groups) == 0` guard in `ui.SelectGroup` (`internal/ui/group_selector.go`), which PR7 (#68) deleted along with `SelectGroup` itself | Delete the `if len(items) == 0 { return nil, errors.New("no eligible targets or groups available") }` guard in `uiUnifiedSelector.SelectItem`. Note: the raw mutation orphans the `errors` import — remove it too | CONFIRMED | test | **Not pinned on `main`.** `TestSelectGroup_EmptyList` / `TestSelectGroup_NonTTYEmptyList` went with `SelectGroup` and have no successor: `TestUnifiedSelector_NonTTY` (`cmd/root_test.go:348`) covers only the non-interactive guard that precedes it, and nothing exercises `SelectItem` with an empty slice. Needs a `TestUnifiedSelector_EmptyList` plus a non-TTY sibling to pin guard **order**, mirroring UI-06/UI-07 | PR7 (#68) | todo | +| UI-08 | cmd/root (unified selector) | `cmd/root.go:1005` (`uiUnifiedSelector.SelectItem`). **The original site is gone:** this was recorded against the `if len(groups) == 0` guard in `ui.SelectGroup` (`internal/ui/group_selector.go`), which PR7 (#68) deleted along with `SelectGroup` itself | Delete the `if len(items) == 0 { return nil, errors.New("no eligible targets or groups available") }` guard in `uiUnifiedSelector.SelectItem`. Note: the raw mutation orphans the `errors` import — remove it too | CONFIRMED | test | `TestUIUnifiedSelector_EmptyItemsGuard` (`cmd/unified_selector_guard_test.go`). Three subtests: `nil` and empty-slice inputs in a TTY assert the **exact** message `no eligible targets or groups available` — a loose substring would also match the earlier `...found, check your SCA policies` guard in `resolveUnifiedSelection` — and a non-TTY subtest asserts `errors.Is(err, ui.ErrNotInteractive)`, pinning guard **order** the way UI-06/UI-07 do. Verified by three mutations, all killed: deleting the guard, swapping the two guards, and rewording `available` | PR7 (#68) | done | | UI-09 | internal/ui | `internal/ui/role_selector.go` (post-`survey` index bounds check) | Disable the returned-index bounds check in `SelectRole` | CONFIRMED | wont-fix | none — defensive-only and unreachable through `survey`, which can only return a string it was given. Closed as **wont-fix in PR7**: the guard stays in place, deliberately uncovered; do not chase it | PR7 | wont-fix (closed) | | UI-10 | internal/ui | `internal/ui/request_selector.go` (post-`survey` index bounds check) | Disable the returned-index bounds check in `SelectRequest` | CONFIRMED | wont-fix | none — same rationale as UI-09. Closed as **wont-fix in PR7**: the guard stays in place, deliberately uncovered; do not chase it | PR7 | wont-fix (closed) | | COV-01 | cmd/root Execute | `cmd/root.go` (the `if shouldShowVerboseHint(...) { Fprintln(...) }` block in `Execute()`) | Delete the whole block. **Killed only on the integration leg.** `go test -count=1 ./cmd/` stays green — the package-level `Execute()` calls `os.Exit`, so only the compiled binary exercises the wiring. It fails under `-tags=integration` via `TestIntegration_ElevateWithoutLogin` and `TestIntegration_VerboseHint`. CI runs that leg unguarded on both OSes, so the call site *is* covered; the default local `make test` loop is not. Do not "simplify" CI by guarding the integration step | CONFIRMED | test | `TestIntegration_VerboseHint` + `TestIntegration_ElevateWithoutLogin` (`-tags=integration` only) | PR4 | done | @@ -367,11 +367,12 @@ closed by review alone (disposition `refuted`: OUT-28, OUT-29, SCA-17 — OUT-27 also REFUTED but a test was added anyway), so **179 rows require work**, of which 7 carry a production change (OUT-26, SFU-02, SFU-07, SFU-23, CACHE-07, CFG-02, UI-02). -**Status on merged `main`:** 184 rows are `done`, 2 are `wont-fix (closed)` (UI-09, -UI-10) and 2 remain `todo` — WF-25 (form-metadata wire tags, deliberately deferred -out of PR8; `Validator.Regex` is still unasserted anywhere in `internal/workflows`) -and UI-08 (its guard moved to `uiUnifiedSelector.SelectItem` when PR7 deleted -`ui.SelectGroup`, and no test succeeded `TestSelectGroup_EmptyList`). +**Status on merged `main`:** 185 rows are `done`, 2 are `wont-fix (closed)` (UI-09, +UI-10) and 1 remains `todo` — WF-25 (form-metadata wire tags, deliberately deferred +out of PR8; `Validator.Regex` is still unasserted anywhere in `internal/workflows`). +UI-08 is now `done`: its guard moved to `uiUnifiedSelector.SelectItem` when PR7 +deleted `ui.SelectGroup`, and `TestUIUnifiedSelector_EmptyItemsGuard` succeeds +`TestSelectGroup_EmptyList`. ### Reconciliation against "145"