fix(ui): resolve interactive selections by index, not display text

CHANGELOG.md

@@ -7,6 +7,8 @@ All notable changes to this project will be documented in this file.
 ### Fixed
 
 - `grant favorites add` now fails immediately without a terminal instead of authenticating first
+- Picking one of two identically named Entra ID groups in the interactive selector no longer elevates into the other
+- The interactive target selector now elevates the row you picked, not another target that renders the same way
 
 ## [0.9.0] - 2026-08-14
 

docs/mutation-ledger.md

@@ -187,16 +187,16 @@ premise does not hold).
 | CFG-07 | internal/config | `internal/config/config.go:103` | `filepath.Join(home, ".grant")` → `".grantx"` | CONFIRMED | test | `TestConfigDir_EndsInDotGrant` | PR6 | todo |
 | CFG-08 | internal/config | `internal/config/config.go:84` | `os.WriteFile(path, data, 0o600)` → `0o644` | CONFIRMED | test | `TestSave_FileMode` with the `runtime.GOOS == "windows"` skip | PR6 | todo |
 | CFG-09 | internal/config | `internal/config/config.go:75` | `if err := os.MkdirAll(dir, 0o700); err != nil { ... }` → ignore the error | CONFIRMED | test | `TestSave_MkdirAllFailure` — force it portably by pointing at a path whose parent component is an existing **regular file** (ENOTDIR / ERROR_DIRECTORY), never a hardcoded `/dev/null/...` | PR6 | todo |
-| UI-01 | internal/ui | `internal/ui/tty.go:18` | `return IsTerminalFunc(os.Stdin.Fd())` → `IsTerminalFunc(os.Stdout.Fd())`. This swap is what makes `grant revoke < /dev/null` hang in a terminal. All twelve prompt-level guards are well covered (8 spot-checked, all killed with `errors.Is` + flag hints); `IsInteractive()` itself is not, because every stub ignores `fd` | CONFIRMED | test | `TestIsInteractive_ChecksStdinFd` — a stub that records the fd and asserts `os.Stdin.Fd()` | PR7 | todo |
-| UI-02 | internal/ui | `internal/ui/group_selector.go:60` | Delete the `sort.Slice(sorted, ...)` call in the group selector | CONFIRMED | test + prod-fix | Extract `sortGroupsForDisplay` so ordering is testable without a TTY; `TestSortGroupsForDisplay_CollisionOrdering` | PR7 | todo |
-| UI-03 | internal/ui | `internal/ui/session_selector.go:57` | `if remaining <= 0 {` → `if remaining < 0 {`. The fixture only supplies `-5m` (`session_selector_test.go:154`), so exactly-zero is unpinned | CONFIRMED | test | `TestFormatSessionOption_ExactlyZeroRemaining` | PR7 | todo |
-| UI-04 | internal/ui | `internal/ui/request_selector.go:18` | Delete the `time.Parse(time.RFC3339Nano, ts)` branch in the timestamp formatter | CONFIRMED | test | `TestFormatRequestOption_RFC3339Nano` | PR7 | todo |
-| 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 | `TestSortRolesForDisplay_MixedCase` | PR7 | todo |
-| UI-06 | internal/ui | `internal/ui/selector.go:49` | Delete the `if len(targets) == 0` guard in `SelectTarget`. (`SelectRole`/`SelectRequest` equivalents are **killed**; these three are not) | CONFIRMED | test | `TestSelectTarget_EmptyList` | PR7 | todo |
-| UI-07 | internal/ui | `internal/ui/session_selector.go:112` | Delete the `if len(sessions) == 0` guard in `SelectSessions` | CONFIRMED | test | `TestSelectSessions_EmptyList` | PR7 | todo |
-| UI-08 | internal/ui | `internal/ui/group_selector.go:54` | Delete the `if len(groups) == 0` guard in `SelectGroup`. Note: the raw mutation orphans the `errors` import — remove it too | CONFIRMED | test | `TestSelectGroup_EmptyList` | PR7 | todo |
-| 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 | PR7 | todo |
-| 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 | PR7 | todo |
+| UI-01 | internal/ui | `internal/ui/tty.go:18` | `return IsTerminalFunc(os.Stdin.Fd())` → `IsTerminalFunc(os.Stdout.Fd())`. This swap is what makes `grant revoke < /dev/null` hang in a terminal. All twelve prompt-level guards are well covered (8 spot-checked, all killed with `errors.Is` + flag hints); `IsInteractive()` itself is not, because every stub ignores `fd` | CONFIRMED | test | `TestIsInteractive_ChecksStdinFd` — a stub that records the fd and asserts `os.Stdin.Fd()` | PR7 | done |
+| UI-02 | internal/ui | `internal/ui/group_selector.go:55` | Delete the `sort.SliceStable(sorted, ...)` call in `sortGroupsForDisplay` (orphans nothing — `sort` stays live via `BuildGroupOptions`) | CONFIRMED | test + prod-fix | Extract `sortGroupsForDisplay` so **list ordering** is testable without a TTY; `TestSortGroupsForDisplay_Ordering`. This row guards the order options are rendered in, and nothing else: sorting a copy never prevented wrong-group selection, it only made a display collision resolve consistently to the *first* matching row. Which group a selection denotes is `resolveGroupSelection`'s job (see the `SelectGroup` index fix in the production-changes table below) | PR7 | done |
+| UI-03 | internal/ui | `internal/ui/session_selector.go:57` | `if remaining <= 0 {` → `if remaining < 0 {`. The fixture only supplies `-5m` (`session_selector_test.go:154`), so exactly-zero is unpinned | CONFIRMED | test | `TestFormatSessionOption_ExactlyZeroRemaining` | PR7 | done |
+| UI-04 | internal/ui | `internal/ui/request_selector.go:18` | Delete the `time.Parse(time.RFC3339Nano, ts)` branch in the timestamp formatter | CONFIRMED | test | `TestFormatRequestOption_RFC3339Nano` | PR7 | done |
+| 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:107` | Delete the `if len(targets) == 0` guard in `SelectTarget` (`: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:112` | 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 | internal/ui | `internal/ui/group_selector.go:79` | Delete the `if len(groups) == 0` guard in `SelectGroup`. Note: the raw mutation orphans the `errors` import — remove it too | CONFIRMED | test | `TestSelectGroup_EmptyList`; guard **order** pinned by `TestSelectGroup_NonTTYEmptyList` | PR7 | 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) |
 
 ---
 
@@ -236,7 +236,7 @@ premise does not hold).
 | `refuted` | 5 |
 | **Total** | **165** |
 
-The seven production changes, matching the plan's table:
+The seven production changes from the plan's table, plus two added by the PR7 review:
 
 | Row | Change | PR | CHANGELOG |
 |---|---|---|---|
@@ -247,6 +247,12 @@ The seven production changes, matching the plan's table:
 | CACHE-07 | `maxSessionAge` → `sessionTimestampRetention` | PR6 | no (internal) |
 | UI-02 | `sortGroupsForDisplay` extraction | PR7 | no (refactor) |
 | SFU-10 | `syncStagedFileFn` seam | PR3 | no (test seam; **not** a production gap) |
+| — | `SelectGroup` resolves the answer by **index** (`resolveGroupSelection`) instead of display text, matching `SelectRole`/`SelectRequest`. Two groups with the same name in different directories render identically, so the old text lookup returned the first match whatever the user highlighted. Not an audit row — found by the PR7 review. Pinned by `TestResolveGroupSelection_DuplicateDisplayStrings` | PR7 | `### Fixed` |
+| — | `SelectTarget` resolves the answer by **index** (`resolveTargetSelection`) against the same sorted copy it renders (`sortTargetsForDisplay`), instead of looking the display text up in the caller's **unsorted** slice. `FormatTargetOption` carries no ID, so two targets with the same workspace name and role in different subscriptions/accounts render identically; worse than the group case, because the rendered and searched slices were in different orders. Not an audit row — found by the PR7 review. Pinned by `TestResolveTargetSelection_DuplicateDisplayStrings` and `TestSortTargetsForDisplay_Ordering`. `SelectSessions` still resolves by text and is deliberately left alone — its option strings embed the session ID, so collisions are unreachable | PR7 | `### Fixed` |
+
+`FindGroupByDisplay` and `FindTargetByDisplay` are exported and still covered by tests,
+but neither has a production caller after the two index fixes. Both are follow-up
+deletion candidates; they were kept here rather than removed in a fix commit.
 
 ### Total CONFIRMED
 

internal/ui/group_selector.go

@@ -33,7 +33,9 @@ func BuildGroupOptions(groups []models.GroupsEligibleTarget) []string {
 	return options
 }
 
-// FindGroupByDisplay finds a group by its formatted display string.
+// FindGroupByDisplay finds a group by its formatted display string. SelectGroup no
+// longer uses it — it resolves by index — so this has no production caller today.
+// On a display collision it returns the first match in the slice it is given.
 func FindGroupByDisplay(groups []models.GroupsEligibleTarget, display string) (*models.GroupsEligibleTarget, error) {
 	for i := range groups {
 		if FormatGroupOption(groups[i]) == display {
@@ -43,9 +45,32 @@ func FindGroupByDisplay(groups []models.GroupsEligibleTarget, display string) (*
 	return nil, fmt.Errorf("group not found: %s", display)
 }
 
-// SelectGroup presents an interactive selector for choosing a group.
-// It sorts a copy of the groups so that FindGroupByDisplay searches the same
-// ordered slice the user saw, avoiding wrong-group selection on display collisions.
+// sortGroupsForDisplay returns a copy of groups ordered by display string, leaving
+// the caller's slice untouched. It only fixes the order the options are rendered in;
+// which group a selection denotes is decided by index in resolveGroupSelection.
+// The sort is stable, so groups that render identically keep their input order.
+func sortGroupsForDisplay(groups []models.GroupsEligibleTarget) []models.GroupsEligibleTarget {
+	sorted := make([]models.GroupsEligibleTarget, len(groups))
+	copy(sorted, groups)
+	sort.SliceStable(sorted, func(i, j int) bool {
+		return FormatGroupOption(sorted[i]) < FormatGroupOption(sorted[j])
+	})
+	return sorted
+}
+
+// resolveGroupSelection recovers the group at the index survey returned. Resolving by
+// index rather than by display text is what makes duplicate display strings safe: the
+// same group name in two directories renders identically, and a text lookup would
+// return the first match no matter which row the user highlighted.
+func resolveGroupSelection(sorted []models.GroupsEligibleTarget, idx int) (*models.GroupsEligibleTarget, error) {
+	if idx < 0 || idx >= len(sorted) {
+		return nil, fmt.Errorf("invalid group selection index %d", idx)
+	}
+	return &sorted[idx], nil
+}
+
+// SelectGroup presents an interactive selector for choosing a group. Uses the selected
+// index (not display text) to recover the group, so duplicate display strings are safe.
 func SelectGroup(groups []models.GroupsEligibleTarget) (*models.GroupsEligibleTarget, error) {
 	if !IsInteractive() {
 		return nil, fmt.Errorf("%w; use --group flag for non-interactive mode", ErrNotInteractive)
@@ -55,27 +80,23 @@ func SelectGroup(groups []models.GroupsEligibleTarget) (*models.GroupsEligibleTa
 		return nil, errors.New("no eligible groups available")
 	}
 
-	sorted := make([]models.GroupsEligibleTarget, len(groups))
-	copy(sorted, groups)
-	sort.Slice(sorted, func(i, j int) bool {
-		return FormatGroupOption(sorted[i]) < FormatGroupOption(sorted[j])
-	})
+	sorted := sortGroupsForDisplay(groups)
 
 	options := make([]string, len(sorted))
 	for i := range sorted {
 		options[i] = FormatGroupOption(sorted[i])
 	}
 
-	var selected string
+	var selectedIdx int
 	prompt := &survey.Select{
 		Message: "Select a group:",
 		Options: options,
 		Filter:  nil,
 	}
 
-	if err := survey.AskOne(prompt, &selected, survey.WithStdio(os.Stdin, os.Stderr, os.Stderr)); err != nil {
+	if err := survey.AskOne(prompt, &selectedIdx, survey.WithStdio(os.Stdin, os.Stderr, os.Stderr)); err != nil {
 		return nil, fmt.Errorf("group selection failed: %w", err)
 	}
 
-	return FindGroupByDisplay(sorted, selected)
+	return resolveGroupSelection(sorted, selectedIdx)
 }

internal/ui/group_selector_test.go

@@ -2,6 +2,8 @@ package ui
 
 import (
 	"errors"
+	"reflect"
+	"sort"
 	"strings"
 	"testing"
 
@@ -199,6 +201,114 @@ func TestFindGroupByDisplay(t *testing.T) {
 	}
 }
 
+// TestSortGroupsForDisplay_Ordering pins exactly two properties of the helper, and no
+// more: the options rendered from its result are in display order, and the caller's
+// slice is not reordered underneath it. It says nothing about which group a selection
+// denotes — that is resolveGroupSelection's job and is pinned by
+// TestResolveGroupSelection_DuplicateDisplayStrings below.
+func TestSortGroupsForDisplay_Ordering(t *testing.T) {
+	t.Parallel()
+	// Deliberately unsorted input containing a display collision: the two
+	// "Engineering" groups have no DirectoryName, so both render identically.
+	groups := []models.GroupsEligibleTarget{
+		{DirectoryID: "dir-z", GroupID: "grp-zebra", GroupName: "Zebra Team"},
+		{DirectoryID: "dir-1", GroupID: "grp-eng-1", GroupName: "Engineering"},
+		{DirectoryID: "dir-2", GroupID: "grp-eng-2", GroupName: "Engineering"},
+		{DirectoryID: "dir-a", GroupID: "grp-alpha", GroupName: "Alpha Team"},
+	}
+	before := append([]models.GroupsEligibleTarget(nil), groups...)
+
+	sorted := sortGroupsForDisplay(groups)
+
+	if len(sorted) != len(groups) {
+		t.Fatalf("sortGroupsForDisplay() length = %d, want %d", len(sorted), len(groups))
+	}
+
+	options := make([]string, len(sorted))
+	for i := range sorted {
+		options[i] = FormatGroupOption(sorted[i])
+	}
+	if !sort.StringsAreSorted(options) {
+		t.Errorf("rendered options are not in display order: %q", options)
+	}
+
+	// The caller's slice must not be reordered underneath it — full snapshot, not
+	// just the endpoints, so an in-place sort cannot hide in the middle.
+	if !reflect.DeepEqual(groups, before) {
+		t.Errorf("sortGroupsForDisplay() mutated the caller's slice:\n got %+v\nwant %+v", groups, before)
+	}
+}
+
+// TestResolveGroupSelection_DuplicateDisplayStrings pins the wrong-group fix. Two
+// groups with the same name in different directories render to the same display
+// string, so recovering the answer by text returns the first match regardless of
+// which row the user highlighted — highlight the second, elevate into the first.
+// survey.Select cannot be driven from a test, so the index path is asserted on the
+// extracted resolver, mirroring SelectRole/SelectRequest.
+func TestResolveGroupSelection_DuplicateDisplayStrings(t *testing.T) {
+	t.Parallel()
+	groups := []models.GroupsEligibleTarget{
+		{DirectoryID: "dir-1", GroupID: "grp-eng-1", GroupName: "Engineering"},
+		{DirectoryID: "dir-2", GroupID: "grp-eng-2", GroupName: "Engineering"},
+	}
+	sorted := sortGroupsForDisplay(groups)
+	if len(sorted) != 2 {
+		t.Fatalf("sortGroupsForDisplay() length = %d, want 2", len(sorted))
+	}
+	if FormatGroupOption(sorted[0]) != FormatGroupOption(sorted[1]) {
+		t.Fatalf("fixture no longer collides: %q vs %q", FormatGroupOption(sorted[0]), FormatGroupOption(sorted[1]))
+	}
+	if sorted[0].GroupID == sorted[1].GroupID {
+		t.Fatalf("fixture groups are indistinguishable: %+v", sorted)
+	}
+
+	// The sort is stable and the two entries compare equal, so the rendered order is
+	// the input order: row 0 is grp-eng-1, row 1 is grp-eng-2.
+	tests := []struct {
+		idx         int
+		wantGroupID string
+		wantDirID   string
+	}{
+		{idx: 0, wantGroupID: "grp-eng-1", wantDirID: "dir-1"},
+		{idx: 1, wantGroupID: "grp-eng-2", wantDirID: "dir-2"},
+	}
+	for _, tt := range tests {
+		got, err := resolveGroupSelection(sorted, tt.idx)
+		if err != nil {
+			t.Fatalf("resolveGroupSelection(_, %d) error = %v", tt.idx, err)
+		}
+		if got.GroupID != tt.wantGroupID || got.DirectoryID != tt.wantDirID {
+			t.Errorf("selecting row %d returned GroupID=%q DirectoryID=%q, want %q/%q",
+				tt.idx, got.GroupID, got.DirectoryID, tt.wantGroupID, tt.wantDirID)
+		}
+	}
+}
+
+func TestResolveGroupSelection_OutOfRange(t *testing.T) {
+	t.Parallel()
+	groups := []models.GroupsEligibleTarget{{DirectoryID: "dir-1", GroupID: "grp1", GroupName: "Engineering"}}
+	for _, idx := range []int{-1, 1} {
+		if _, err := resolveGroupSelection(groups, idx); err == nil {
+			t.Errorf("resolveGroupSelection(_, %d) = nil error, want out-of-range error", idx)
+		}
+	}
+}
+
+// Not parallel: mutates the package-global IsTerminalFunc.
+func TestSelectGroup_EmptyList(t *testing.T) {
+	original := IsTerminalFunc
+	defer func() { IsTerminalFunc = original }()
+	IsTerminalFunc = func(fd uintptr) bool { return true }
+
+	_, err := SelectGroup(nil)
+	if err == nil {
+		t.Fatal("expected error for empty list")
+	}
+	if !strings.Contains(err.Error(), "no eligible groups available") {
+		t.Errorf("unexpected error: %v", err)
+	}
+}
+
 // Not parallel: mutates the package-global IsTerminalFunc.
 func TestSelectGroup_NonTTY(t *testing.T) {
 	original := IsTerminalFunc
@@ -220,3 +330,24 @@ func TestSelectGroup_NonTTY(t *testing.T) {
 		t.Errorf("error should mention --group, got: %v", err)
 	}
 }
+
+// TestSelectGroup_NonTTYEmptyList pins the order of the two guards. _NonTTY passes a
+// non-empty list and _EmptyList forces a TTY, so their inputs never intersect and
+// swapping the guards survives both. This case satisfies both conditions at once and
+// demands the non-interactive error.
+// Not parallel: mutates the package-global IsTerminalFunc.
+// The package restores globals with defer rather than t.Cleanup — deliberate, it is
+// the convention every other test in internal/ui already follows.
+func TestSelectGroup_NonTTYEmptyList(t *testing.T) {
+	original := IsTerminalFunc
+	defer func() { IsTerminalFunc = original }()
+	IsTerminalFunc = func(fd uintptr) bool { return false }
+
+	_, err := SelectGroup(nil)
+	if err == nil {
+		t.Fatal("expected error for non-TTY with an empty list")
+	}
+	if !errors.Is(err, ErrNotInteractive) {
+		t.Errorf("expected ErrNotInteractive to win over the empty-list guard, got: %v", err)
+	}
+}