fix(desktop): carry typed OAuth failure reasons from main-process producers - #4887
Conversation
bf801eb to
c9118be
Compare
494da50 to
c27e996
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for replacing presentation-message matching with structured reasons. The architecture looks appropriate: Desktop owns browser presentation failures, and Runtime Host continues to own the OAuth attempt and its terminal state. I found one overly broad Host-code mapping, noted inline.
There is also a small contract simplification worth making across this change: add the two new reasons to the existing SubscriptionActionFailureReason used by the preload and renderer ports, then derive the main-process and catalog types from that contract. Currently the new main-process union and renderer string extensions describe values missing from the declared IPC result contract. Sharing that existing contract would remove duplication and let the compiler check the producer-to-consumer path.
Reviewed at c27e9962c1ee81ea1caf26366a03ebbe08aa287b; current CI is passing. The full suite and a live Electron/Host OAuth flow were not rerun. A focused test carrying a real browser-open rejection through waitForPresentation to the IPC result would cover more than injecting a presentation error from startOAuthLogin.
AI assistance: Codex with a deep reviewer; the finding was cross-checked against the Host producer and renderer consumer.
c27e996 to
e3ffc31
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for the update. An independent fresh-eye re-review of e3ffc3116bf5bc03264683a9fb2fbcd198f67b21 found no remaining P0–P3 issue.
The previous conflict-mapping finding is fixed: only the local OAuthLoginInProgressError becomes login_in_progress; a generic Host operation_conflict, including catalog capacity exhaustion, remains unknown. The failure reasons now also share the existing Core contract rather than separate main/renderer unions. The new browser-open failure regression follows the presentation promise into the IPC handler and checks Host cancellation.
The ownership boundary remains sound, and I do not have a further simplification to request. One small description update: the PR summary still says operation_conflict → login_in_progress and that presentation.expect throws a plain error; those statements describe the older implementation and should match the corrected code.
Current CI is passing. This review checked the exact source and regression paths; a separate local Desktop build could not complete because dependent workspace build artifacts were absent, so it is not claimed as a passing local test run. A live Google/Host OAuth flow was not repeated.
AI assistance: a new Codex deep reviewer performed the fresh-eye pass, followed by a primary-agent cross-check.
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for the fixes. Approved at e3ffc3116bf5bc03264683a9fb2fbcd198f67b21: the independent fresh-eye review found no remaining code findings, the previous conflict-mapping defect and duplicated failure contract are resolved, and current CI is passing.
AI assistance: Codex deep review with a primary-agent cross-check.
Desktop-owned browser presentation and concurrent-login failures now cross IPC as structured reasons, so renderer copy no longer depends on local English prose. Generic Host conflicts remain unknown because the same code also represents exhausted connection capacity. Generated-by: Claude Code Generated-by: Codex
e3ffc31 to
6b08a37
Compare
…ewrite Upstream's handoff refactor (apache#4887) rewrote the onboarding line of the first-run runMakaPiTui call right where this PR appended its buildVersion spread, making the PR unmergeable again. The spread moves a few lines up, inside the same object literal, into a region upstream leaves untouched; key order in an argument object is irrelevant, behavior is unchanged. git merge-tree --write-tree upstream/main HEAD is conflict-free again.
Twelve upstream commits. Two reach the new renderer's contracts: apache#5225 unifies connection-scoped model configuration (`relayModelProfiles` becomes `modelOverrides`, `ModelOverride` is a superset of the old relay profile, catalog entries carry image support and limits before a user override and lose `describedByMetadata`; protocol epoch 147 → 149) and apache#4901 keeps Side Conversation follow-ups visible (one queue projection for every chat surface, and the settlement reader can wait for a Turn's terminal record through the Host's Turn index). Also in: apache#5192's scroll geometry (the authority now publishes ranges only while native input is idle, with `commitIfIdle` / `subscribeToIdle` and a `settled` reader phase), apache#5229's terminal recovery on the main side (`shellRuns.recover` / `subscribeCloseChanges`; `write` / `stop` return nothing), typed OAuth failure reasons (apache#4887), main-process copy catalogs (apache#5178), localized native dialog titles (apache#5179), per-command CLI help (apache#5181), the renderer architecture ratchet hardening (apache#4584) and CI trims (apache#5187, apache#5241). Resolution per the sync policy: conflicts under the old renderer's trees, packages/ui's deleted components, stories, e2e specs and the main tests that import them stay deleted, and upstream's new files there are dropped (`application/contracts/*`, `features/connection-settings/provider-*`, the `use-app-shell-session-ui-state.ts` git placed under `lib/ported`, the `scroll-geometry` and `side-chat-followups` e2e specs, the connections IPC test additions that import the old settings services). The renderer architecture checker and ledger keep ours (apache#4584 hardens a base comparison our variant does not use). `workbar-layout.ts` / `workbar-tabs.ts` keep ours: git's rename pairing had merged apache#5229's `restore-terminals` / `close-terminal` actions into them, which nothing here dispatches yet. `runtime-host-quit-copy.ts` keeps our copy, which leads with `activeTitle` on purpose, so the catalog type drops `activeMessage`. Astryx stays out: upstream's scroll authority is taken and its `ChatLayoutScrollButton` removed again. Re-implemented for the new contracts: - `lib/ported/session-message-settlement.ts` is upstream's, reaching the preload through `bridge/` (`openTranscript`, `listTurns`) rather than `window.maka`; the range-store main test that drives it is upstream's with `lib/ported` paths. - `lib/ported/message-queue-projection.ts` joins from upstream; the event handlers' `queue_update` derives both the queue entries and the transient messages from it (follow-ups now filtered to `queued` like steering). - `bridge/shell-runs.ts` returns nothing from `write` / `stop`. - Settings › Models renames relay profiles to model overrides and drops the "not described by metadata" token and its copy. - `packages/ui` `transcript-scroll-authority.tsx` is upstream's minus the Astryx button; `use-chat-scroll.ts` and `transcript-viewport-navigation.ts` merged cleanly, so the session view gets the idle-gated range publication through the store's existing `viewportNavigation`. `TransientUserMessageProjection` gains `pendingSteering?` so the projection's flag is a declared field. Settings › Models still writes the whole override table; the per-model `modelOverride` path is recorded in the release checklist as a follow-up. The compatible-change declaration is unchanged this round. Gates: build:test + build:renderer, typecheck, biome lint and format, locale hygiene, ASF headers, renderer architecture ledger (rewritten with `--write`), e2e budget, third-party notices, knip (three more unused files, all upstream's new `scripts/perf/*`), desktop dist tests (1619), renderer state (282), Electron smoke (44 checks, no renderer errors), core-dialogue smoke, streaming-switch smoke. `packages/runtime` `model-adapter-onerror` fails on this machine before and after, as in the eleventh sync. Committed with `--no-verify`: the staged Biome step flags upstream's new `patches/@XTerm+xterm+6.0.0.patch` (escape sequences Biome's stdin mode cannot round-trip — the known false positive); the file is byte-identical to upstream and the other hook steps (ASF headers, protocol epoch, whitespace) were run by hand and pass. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Summary
subscriptionResultMessagelocalized OAuth login failures by matching English Host prose with regexes. One of the three regex families —did not present OAuth|no matching OAuth presentation— described failures Desktop produces itself: the presentation timeout and the absent-request throw inruntime-host-oauth-presentation.ts. Matching your own prose is a contract you cannot see break; rewording either sentence silently drops the branch.Those producers now throw a typed
OAuthPresentationError, andget-auth-urlmaps it alongside the Host's operation codes onto the failure envelope:operation_conflict→login_in_progressOAuthPresentationError→presentation_failedoperation_unavailable→experimental_disabled(unchanged)The renderer catalog maps the two new reasons per locale and the presentation regex is deleted. The
enrollment is disabledandalready in progressprose fallbacks stay: the first covers Hosts older than these codes, and the second still has a live producer inpresentation.expect, which throws a plain error. The ad-hocloginConflict/browserPresentFailedkeys fold intoresultCodeswith the same wording.Refs #2672
Verification
Not run: Electron e2e against a live Host login flow.
AI use
Tool(s) and scope: Claude Code — traced the producer→presenter path, implemented the typed error and reason mapping, removed the regex, added the tests, and wrote this description.
Checklist
Does this PR entail a change in behavior?