Skip to content

fix(desktop): carry typed OAuth failure reasons from main-process producers - #4887

Merged
Astro-Han merged 1 commit into
apache:mainfrom
orangeCatDeveloper:fix/oauth-failure-reasons
Sep 12, 2026
Merged

Astro-Han merged 1 commit into
apache:mainfrom
orangeCatDeveloper:fix/oauth-failure-reasons

Conversation

@orangeCatDeveloper

@orangeCatDeveloper orangeCatDeveloper commented Sep 6, 2026 •

Copy link
Copy Markdown
Contributor

Summary

subscriptionResultMessage localized 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 in runtime-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, and get-auth-url maps it alongside the Host's operation codes onto the failure envelope:

  • operation_conflict → login_in_progress
  • OAuthPresentationError → presentation_failed
  • operation_unavailable → experimental_disabled (unchanged)

The renderer catalog maps the two new reasons per locale and the presentation regex is deleted. The enrollment is disabled and already in progress prose fallbacks stay: the first covers Hosts older than these codes, and the second still has a live producer in presentation.expect, which throws a plain error. The ad-hoc loginConflict / browserPresentFailed keys fold into resultCodes with the same wording.

Refs #2672

Verification

apps/desktop full dist suite        2489 pass / 0 fail
apps/desktop typecheck (4)          0 errors
renderer architecture ratchet       passes against the base
npm run format:check                clean

Not run: Electron e2e against a live Host login flow.

AI use

  • Generative tooling made a substantive contribution

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

  • Tests cover the change and fail without it
  • Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

  • Yes — login-conflict and browser-presentation failures render catalog copy through typed reasons instead of English prose matching.

@github-actions github-actions Bot added the effort/XL Under 2500 readable lines label Sep 6, 2026
@orangeCatDeveloper
orangeCatDeveloper force-pushed the fix/oauth-failure-reasons branch 6 times, most recently from bf801eb to c9118be Compare September 8, 2026 20:03
@github-actions github-actions Bot added effort/M Under 500 readable lines and removed effort/XL Under 2500 readable lines labels Sep 9, 2026
@orangeCatDeveloper
orangeCatDeveloper force-pushed the fix/oauth-failure-reasons branch 2 times, most recently from 494da50 to c27e996 Compare September 11, 2026 04:18
@orangeCatDeveloper
orangeCatDeveloper marked this pull request as ready for review September 11, 2026 04:21

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread apps/desktop/src/main/runtime-host-oauth-ipc-main.ts Outdated

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
@Astro-Han
Astro-Han merged commit ca4136a into apache:main Sep 12, 2026
1 check passed
@orangeCatDeveloper
orangeCatDeveloper deleted the fix/oauth-failure-reasons branch September 12, 2026 19:00
abhinav-phi added a commit to abhinav-phi/maka that referenced this pull request Sep 12, 2026
…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.
Shouly pushed a commit to Shouly/maka that referenced this pull request Sep 13, 2026
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>
me2seeks pushed a commit that referenced this pull request Oct 13, 2026
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/M Under 500 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants