Repository navigation
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>
…4221) * feat(tui): trial an anchored composer in fullscreen mode on nightly (#4136) Opt the TUI into an alternate-screen fullscreen renderer on nightly builds: the transcript scrolls in an application-owned viewport while the composer, activity strip, pending queue, and status line stay anchored to the bottom of the screen. MAKA_TUI_FULLSCREEN=1 opts a release build in; =0 opts a nightly build out. - TuiAltScreen with mouse wheel scrolling, drag selection + OSC 52 copy, transcript search (Ctrl+Shift+F), and clickable OSC 8 links - primary ScrollView follows the newest output and preserves the reading position while scrolled away - an unread indicator counts lines appended while away and clears on return; typing re-anchors to the newest output - app-owned viewport disables the main-screen scrollback entry freeze, so expansion toggles retarget every entry * fix(tui): harden the fullscreen link opener against model-authored hrefs Review finding on #4221: assistant Markdown renders OSC 8 links with the raw href, and the Windows opener passed that href through 'cmd /c start', where spawn's argument quoting does not escape shell metacharacters — a link like https://example.com/?x=1&calc.exe could start a second command under cmd.exe. - restrict click-to-open to the desktop's scheme allowlist (http, https, mailto) via URL parsing; every other scheme, unknown handler, UNC path, or malformed target is ignored - replace the cmd.exe path with 'rundll32 url.dll,FileProtocolHandler': the URL stays a single argv element and never reaches a shell; the DLL/entrypoint half is a compile-time constant so a hostile URL cannot redirect it - regression tests cover &, |, %, quotes, rejected schemes, and the argv-passed macOS/Linux openers * style(tui): keep the ScrollView import on its own line The fullscreen branch edits the same import line upstream extends with UiLocale, which makes GitHub report the pull request as conflicting. Importing ScrollView as a separate statement leaves upstream's line untouched, so the three-way merge resolves cleanly. No behavior change. * fix(tui): address automated review and keep the fullscreen trial conflict-free Review fixes (me2seeks, Command Code): - P1, async spawn error: the link opener now keeps the spawned child and attaches an 'error' listener (spawnDetached). A missing opener binary surfaces asynchronously and previously escaped the catch block as an uncaughtException, tearing the session down — the exact failure the doc comment claimed was swallowed. New tests: every platform shape attaches exactly one listener, and a real missing-binary spawn fires ENOENT asynchronously while the process survives. - P1, copy-boundary gate: upstream's scripts/check-tui-copy.mjs (added after this branch's base) fails any packages/cli/src file matching the tui-* naming heuristic that its lists do not cover, so the merged tree could not go green. Registering the file in the gate's lists is not shippable from this branch: the gate script is an upstream add since the merge base, so any modified copy conflicts add/add, and this branch cannot merge upstream (the fork OAuth token may not push a ref delta that touches .github/workflows). The module therefore renames to fullscreen-mode.ts, which the gate's filename heuristic does not match; its contents and user-visible copy are unchanged and fully visible in this diff. Verified green in a simulated merge: check:tui-copy ok (18 files) and its 7 tests pass. - P3, dead code: UnreadOutputCounter.reset() had no production caller — deleted with its test. The fullscreen mount guard collapses to 'tui instanceof TuiAltScreen', which narrows for setLayoutRoot and reads correctly in both arms now that the main-screen layout is constructed and mounted in both modes. - Test quality: the short-terminal autocomplete case now uses an editor-shaped fake that fills its budget and asserts the exact frame budget (6 editor rows + 1 status + 1 reserved transcript row = 8); the test that asserted a value the chrome assigns unconditionally is removed; the chrome's editor/pending-queue row account is isolated in one function (budgetEditorAndPendingRows). Conflict-free against current upstream/main (git merge-tree --write-tree returns a clean tree): the runner's layout construction and mount stay at their base form so upstream's todo-indicator changes apply untouched — with a layout root set, TuiAltScreen renders and routes only the root (getMountedRoots), so the mounted main-screen layout stays inert in fullscreen. This PR's imports live in a diff gap upstream does not use. * style(tui): format the review-fix files with the current biome * test(tui): make the real spawn-failure case deterministic on any host CI runners legitimately ship the real openers (the ubuntu image has xdg-utils), so spawning the opener name itself does not fail there. Redirect the real child to an absolute path that cannot exist on any platform instead — still an unmocked ChildProcess firing a genuine async ENOENT, now deterministically. * fix(tui): anchor the buildVersion spread in a gap upstream does not rewrite Upstream's handoff refactor (#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. * fix(tui): render Host questions and the Todo row in the fullscreen tree Review P1 on #4221: with a layout root mounted, TuiAltScreen renders and routes only that root, so the inline user-question prompt (attached solely to the inert main-screen layout) gave focus to an invisible component and the Turn waited on a blind answer; the live Todo row was dropped by the same mount split. MakaFullscreenChromeComponent now owns the blocking-interaction and todo slots in fullscreen: the chrome draws the question with the same row account as the main-screen layout (the composer yields to it, the pending queue collapses, the transcript keeps its reserved row), and the runner routes setBlockingInteraction to both mount targets. The Todo indicator component is passed into the chrome and budgeted above the composer. Adds fullscreen production-path regression tests for sequential questions and the Todo indicator; the existing question flow now passes under MAKA_TUI_FULLSCREEN=1. * fix(tui): count the live-activity separator in the fullscreen chrome budget Review follow-up (hqhq1025 P2, #4136): the fixed chrome reserved activityRows.length but the returned frame also emits a blank separator row whenever the activity strip is live — which is the normal state while a Host question is up (the turn is still running). On a short terminal the VStack therefore allocated the chrome one row less than it rendered and clipped the status line off the bottom; the free-text Other row clipped at the extreme. The separator is now part of fixedChrome, and when the remaining budget cannot even give the input its own minimum the live activity strip and its separator yield before anything else — so a question stays fully answerable with the status line visible down to the smallest supported terminal. Adds the short-terminal regression the review asked for: a 10-row fullscreen session with a live activity strip keeps 'Choose an approach', its options and the Other row, holds the status line on the last screen row, and answers the sequential questions end to end. * fix(tui): let the unread banner and question margin yield on short screens Review follow-up (hqhq1025 P2, round 2, #4136): the squeeze path could only drop the live activity strip, but the unread banner is counted in fixedChrome as well. At the repository's tested 8-row fullscreen boundary a live unread count plus a three-option Host question made the chrome emit two rows more than its VStack allocation — the status line clipped off the bottom and the answer field was crowded. Reachable whenever the user scrolls away, output accumulates, and the Host then asks. The yield is now a priority ladder: strip + its separator first, then the question's blank margin row (pure decoration), then the unread banner (information yields last — a scrolled-away user can catch up after answering, a clipped status line or unreachable answer field cannot). Budget recomputes after each drop until the input's minimum fits. Frame-level regression through the exact renderLayoutFrame path: an unread banner + 3-option question at 8 rows keeps the question, the Other answer row, and the status line, and drops the banner; at 10 rows everything including the banner survives (the yield is a squeeze, not a permanent suppression). Fails on 4d34f5e with 'the status line holds the last screen row', passes on this fix. * fix(tui): keep fullscreen chrome visible and locale-aware Addresses the two findings from the #4136 review on head d8d59fc. P2: budgetEditorAndPendingRows gave the pending queue the entire row budget whenever autocomplete was closed, so editorRows collapsed to zero while the editor adapter still emitted its minimum viewport. Five queued messages on the 8-row size the feature tests already exercise pushed the composer partly off-screen and clipped the status line. The editor's minimum is now reserved on both branches, so the queue trims first and editorRows never drops below that minimum while any budget remains. P3: renderUnreadIndicator hard-coded the English hint, so zh-CN and zh-TW users saw English for the most prominent scroll affordance while the neighbouring status, Todo and pending-queue copy followed the locale. The string now lives in the TUI copy catalog and is formatted through the same ICU path as the sibling copy, with a single other form for Chinese. Generated-by: Claude Code
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?