Say when the team list could not be loaded during sign-in - #477
Say when the team list could not be loaded during sign-in#477beardthelion wants to merge 1 commit into
Conversation
Both sign-in paths turned every team-fetch failure into an empty team list, which is the same value an account with no teams produces. A 500, a rejected credential, a malformed body, or a response whose entries do not parse therefore left the user with a session scoped to their personal account, no team picker, and output byte-identical to a healthy login. The session persists that way, so gateway requests that need a team are rejected until the next login. Keep signing in, since the token pair is valid and the team fetch is auxiliary, but track whether the list actually arrived and say so when it did not. An account that genuinely has no teams stays silent. parseTeams now counts the entries it cannot read. A non-empty array that yields nothing usable is a response we could not read rather than an account with no teams, so it returns InvalidTeamsResponse; a partly readable list keeps the teams it got and still reports the shortfall, because the team the user wants may be one of the entries that dropped. This also reaches the fx teams command, which fails loudly on a list it cannot read instead of reporting no teams. The CLI prints one line after the existing gateway-access caveat. The TUI emits a warning notice beside the existing sign-in notice, reading the flag before the picker branch because openTeamPicker takes the selection. The two surfaces share a prefix and append their own remedy, so the TUI does not tell you to run a shell command. TestAuth.openTeamPicker was a no-op that did not take the selection the way the real picker does, and TestApp discarded the notice tone. Making both faithful is what lets the unit tests cover a team-carrying selection whose fetch failed, and assert the warning tone, neither of which the end-to-end tests can reach.
|
This needs a On CI, so it isn't a surprise: upstream checks are green. On my fork's Full CI, everything passes except I don't think that shard is related to this change, and the check that convinced me is that shard 3's file list is identical on |
|
Correcting my earlier note on that macOS shard. I said it looked unrelated because its tests never touch the login path, which was the wrong reason: every e2e test runs the same binary this change is compiled into, so the test file's contents prove nothing either way. I settled it properly by pushing Full CI is green on this commit now. The PR body has the details. |
1 participant
Fixes #476.
Needs
type: bug. I cannot apply labels, so this stays in draft until a maintainer adds it. Full CI is green on this commit across all four native runners.A failed team lookup during sign-in was indistinguishable from an account that has no teams, so
fx loginreported success and quietly left you in personal scope. Both sign-in paths did:Every failure became an empty list, which is exactly what a zero-team account produces. No picker, no message,
auth.jsonwritten withoutteam_slugorteam_id, and stdout byte-identical to a healthy login.The sign-in itself is fine, so this keeps it. The token pair is valid and the team fetch is auxiliary; failing the login over it would be worse, and re-running the device flow is expensive. What was missing was any signal, so that is what this adds.
What changed
Both call sites now track whether the list actually arrived. On a caught error the CLI prints one line after the existing gateway-access caveat, and the in-app flow emits a warning notice beside the existing sign-in notice. An account with genuinely no teams stays silent, which is the case that decides whether this is a useful signal or a false alarm.
parseTeamsnow counts entries it cannot read. A non-empty array that yields nothing usable is a response we could not read rather than an account with no teams, so it returnsInvalidTeamsResponse; a partly readable list keeps what it got and still reports the shortfall, since the team you want may be the entry that dropped. Without this a200carrying{"teams":[{"id":123,"slug":"acme"}]}reproduced the original bug with no error anywhere.That last part also reaches
fx teams, which now fails loudly on a list it cannot read instead of reporting no teams. It already usedtryon the same call, so this is the direction it was already going, but it is a behavior change in a second command and worth your attention.The two surfaces share a prefix constant and append their own remedy, following
remote_revocation_warning, so the in-app notice does not tell you to run a shell command.On the test harness
Two changes there were load-bearing rather than incidental.
TestAuth.openTeamPickerwas a no-op that did not take the selection the way the real picker does. The consumer reads the flag before the picker branch precisely becauseopenTeamPickertakes ownership, and with the old stub a unit test covering that could pass whether or not the read was in the right place. Making the stub faithful is what lets the test actually pin it: removing the hoist now turns it red.TestAppdiscardednotice.tone, so nothing could distinguish a warning notice from a neutral one. It records it now, and the test asserts the tone.Verification
Nine new end-to-end cases and six new unit tests. The three failure cases were each observed failing before the implementation, and both silence controls were proven load-bearing by forcing the flag true and watching them go red.
The full
tui-auth-source-selection.test.tsrun was compared against the same file onmain: identical failing-test sets, 14 either way, none of them these. The Zig suite was compared the same way, by failing test name rather than count.zig fmt --check src/passes.Full CI on my fork is green on this commit. It took a few runs to get there, and the reason is worth recording rather than hiding:
E2E (ReleaseSafe, macos-aarch64, shard 3/4)failed twice before passing, on subagent-manager tests.That shard is flaky on
mainindependently of this change. I pushed261a7c3funmodified to my fork and ran the same workflow: it passed once and then failed the same shard, onconfigure rejects a concurrent winner then retries the preserved draft once,persistent child pointer drag replaces the selected composer range, andpersistent child preserves its reading position across both reopen paths. Across runs the failing set keeps changing, and the pointer-drag test failed on bothmainand this branch.Tally on
macos-aarch64 shard 3/4: unmodifiedmain1 pass / 1 fail, this branch 1 pass / 2 fail.I mention it because my first read was that the shard could not be related since its tests never touch the login path, and that reasoning was wrong: every e2e test drives the same binary this change is compiled into. The A/B against unmodified
mainis what actually settles it, not the argument from test names.Not covered
A socket-level transport failure is not directly exercised. It reaches the same catch as the HTTP cases, so the flag is covered, but the socket path itself is not.
The wasm
fetchTeamsFromJsHostbranch has no test here.Re-authenticating over an existing session still clears a previously selected team when the fetch fails. That predates this change and fixing it means deciding a team should survive re-login, which felt like a separate call to make.