fix(ui): resolve interactive selections by index, not display text - #68
Open
aaearon wants to merge 4 commits into
Open
fix(ui): resolve interactive selections by index, not display text#68aaearon wants to merge 4 commits into
aaearon wants to merge 4 commits into
Conversation
Closes the eight actionable internal/ui mutation-ledger rows (UI-01..UI-08) and records UI-09/UI-10 as closed wont-fix. - IsInteractive() now has a test asserting WHICH fd is probed; every previous stub ignored its argument, so swapping os.Stdin.Fd() for os.Stdout.Fd() survived — the swap that would make `grant revoke < /dev/null` hang. - Extract sortGroupsForDisplay from SelectGroup (behaviour-preserving) so the display-collision ordering fix is testable without a TTY. - Add the exactly-zero remaining-time boundary, an RFC3339Nano timestamp case, a mixed-case role sort fixture, and the missing empty-list guards for SelectTarget, SelectSessions and SelectGroup.
SelectGroup bound survey.Select to a string and recovered the group with
FindGroupByDisplay. Two groups with the same name in different directories
render identically, so the lookup returned the first match regardless of which
row the user highlighted - highlighting the second elevated into the first.
Sorting a copy never fixed that; it only made the wrong answer deterministic.
SelectGroup now binds an int and resolves through resolveGroupSelection,
matching SelectRole and SelectRequest. The sort becomes SliceStable so
colliding rows keep their input order.
Tests:
- TestResolveGroupSelection_DuplicateDisplayStrings pins the index path; it
fails when the resolver is reverted to a display lookup.
- TestSortGroupsForDisplay_Ordering replaces the collision test, dropping its
tautological FindGroupByDisplay round-trip and claiming only what it pins:
display ordering plus a full-snapshot caller-slice immutability check.
- TestSelect{Target,Sessions,Group,Role,Request}_Non{TTY,Interactive}EmptyList
pin the non-interactive guard ahead of the empty-list guard; all five fail
when the guards are swapped.
- Role option/role parallelism and a length assertion; exact-match assertion
in the request timestamp test.
Docs: CHANGELOG Fixed entry; mutation-ledger UI-02/06/08 line references
repointed and UI-02's narrative corrected to list ordering only.
SelectTarget rendered BuildOptions(targets) - a sorted string slice - and then recovered the answer with FindTargetByDisplay against the caller's unsorted slice. FormatTargetOption carries no ID, so two eligible targets with the same workspace name and role in different subscriptions or accounts render identically; the lookup returned the first match whatever the user highlighted, and because the rendered and searched slices were in different orders the two could disagree even without a collision. Wrong-target elevation either way. SelectTarget now renders from sortTargetsForDisplay and resolves through resolveTargetSelection by index, matching SelectGroup/SelectRole/SelectRequest. The sort is stable, so colliding rows keep their input order. Guard order is unchanged: non-interactive still fires before the empty-list check. SelectSessions keeps its display-text lookup and gains a comment saying why: every option string embeds the session ID, so collisions are unreachable. Tests: - TestResolveTargetSelection_DuplicateDisplayStrings pins the index path; it fails when the resolver is reverted to a display lookup. - TestResolveTargetSelection_OutOfRange mirrors the group equivalent. - TestSortTargetsForDisplay_Ordering pins display ordering, stable ordering of colliding rows, and caller-slice immutability. Docs: CHANGELOG Fixed entry; mutation-ledger production-changes row (not an audit row - found by the PR7 review), UI-06 line reference repointed, and both FindTargetByDisplay and FindGroupByDisplay noted as deletion candidates.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part 7 of 8. Base:
test/isolation-harness(#63) — must merge first.Both
SelectGroupandSelectTargetboundsurvey.Selectto a string and resolved the user's choice by display text.FindGroupByDisplayreturned the first match regardless of which row was highlighted. Pick the second, elevate into the first — whileDirectoryID, the field that exists to disambiguate them, was never consulted.SelectTargetrendered a sorted option list but resolved against the caller's unsorted slice, andFormatTargetOptioncontains no ID — just workspace type, workspace name, role name, CSP. Two targets with the same workspace name and role in different subscriptions collide.Both now bind to
selectedIdxand resolve through a bounds-checked helper, matchingSelectRoleandSelectRequest, which already did this correctly and carried comments saying duplicate display strings are safe.How this was missed
A previous fix sorted a copy and left a comment claiming it "avoids wrong-group selection on display collisions". It never did — sorting only makes the wrong answer deterministic. The audit read that comment, classified the missing sort test as a wrong-group regression guard, and the plan repeated it. The test written to pin it turned out to be tautological: for any slice, a linear exact-match scan is guaranteed to find an element rendering to the string it was given, so the assertion held identically for unsorted input.
Only mutating the actual call site exposed it.
Also
IsInteractive()never asserted it inspects stdin. Swapping toos.Stdout.Fd()survived — that swap is what would makegrant revoke < /dev/nullhang forever in a terminal._EmptyListtests.sort.Slice→sort.SliceStableso colliding rows keep input order.Notes
FindGroupByDisplayandFindTargetByDisplayare now production-dead — only tests call them. Kept with accurate doc comments (no production caller; returns the first match on a collision) and flagged as follow-up deletion candidates, rather than removed inside a behaviour-fix commit.SelectSessionsstill resolves by text and is left alone: its display string embedsSessionID, so collisions are unreachable. That safety now rests onBuildSessionOptionscontinuing to embed the ID — asserted by comment, not by test.SelectRole/SelectRequestwere confirmed unreachable by readingsurvey/v2source:Indexalways derives from the original options slice.survey.Selectcannot be driven from a test, so "before" means the new helper with the old display-text resolution.Adversarial review performed (Codex credits exhausted; review by a Claude agent) — it found the group bug; the
SelectTargetinstance was found while fixing it.