fix(settings): surface actionable OAuth failures - #3319
Conversation
Generated-by: Codex
|
After:
Maka Desktop diagnostic report Error Environment Recent main-process logs (1) Runtime Host |
Scope and recovery-path reviewThe direction is valuable: preserving actionable OAuth failure reasons and giving users a direct path to Network Settings fixes a real UX gap. However, I think the current PR combines two different problems and expands well beyond the minimum change needed for OAuth recovery. 1. The problem statement should be corrected#1301 describes a configured Maka proxy being bypassed during Codex OAuth token exchange/refresh. That issue was already closed by #1302, which routed those requests through the active proxy. This PR addresses a different problem:
Proxy discovery is a separate feature request:
These should be defined and evaluated independently. The “Before” evidence currently proves only that the UI displayed a generic authentication failure; it does not by itself establish that proxy discovery is required to fix the failure. 2. Occam's razor: the minimum sufficient fix is much smallerFor the OAuth recovery issue, the minimum sufficient change appears to be:
Automatic environment/system proxy discovery, candidate adoption, restoring proxy defaults, credential deletion, a new Host operation, and a new remote-owner grant are not necessary to solve the error-reporting/recovery-path problem. I recommend keeping the failure projection + recovery navigation in this PR and moving proxy discovery/reset into separately designed changes. 3. Side effects and gaps to resolveThe current proxy test does not verify OAuth recovery. Proxy discovery exposes more than credentials. Only the first parseable candidate is returned. System/PAC discovery has no explicit timeout. Reset is a multi-step, non-atomic mutation. The compatibility impact is broader than the OAuth fix. RecommendationI recommend splitting the change:
This keeps the OAuth recovery fix focused while allowing the proxy-discovery security and product behavior to be reviewed on their own merits. 中文范围与恢复路径审视这个方向有价值:保留可操作的 OAuth 失败原因,并给用户一个直接进入网络设置的入口,确实补上了真实的 UX 缺口。但当前 PR 混合了两个不同的问题,而且明显超出了 OAuth 恢复所需的最小改动。 1. 建议修正问题定义#1301 描述的是:Maka 已配置代理,但 Codex OAuth token exchange/refresh 绕过了代理。这个问题已经由 #1302 修复并关闭。 当前 PR 实际解决的是:
而代理发现属于另一个独立需求:
这两个问题应当分别定义、分别验证。目前的 Before 证据只能证明 UI 显示了通用鉴权失败,不能单独证明“自动发现代理”是修复该问题的必要条件。 2. 从奥卡姆剃刀看,最小充分修复应该小得多对于 OAuth 恢复问题,最小充分变更应当是:
环境/系统代理自动发现、候选采用、恢复代理默认值、删除凭据、新增 Host operation 和 remote-owner grant,都不是解决“错误报告与恢复入口”问题的必要条件。建议本 PR 只保留失败投影与恢复导航,把代理发现和 reset 拆到独立变更中。 3. 需要处理的副作用和缺口当前代理测试不能证明 OAuth 已恢复。 代理发现暴露的不只是凭据。 只返回第一个可解析候选。 系统/PAC 检测没有显式超时。 恢复默认是非原子的多步变更。 兼容性影响大于 OAuth 修复本身。 建议建议拆分为:
这样既能尽快修复 OAuth 恢复体验,也能让代理发现相关的安全边界和产品行为得到独立、充分的审查。 |
Generated-by: Codex
|
Thanks for the detailed review. I agree that this PR conflated the follow-up OAuth recovery problem with proxy discovery and reset. I have narrowed the branch locally to:
I also agree that the existing generic proxy probe cannot establish OAuth recovery. The recovery copy now tells the user to update Network Settings and retry sign-in. That retry performs the destination-correct Epoch 29 remains necessary because the OAuth failure codes extend the closed projection schema; upstream already uses epoch 28. Proxy discovery should be a separately designed change with a local/privacy boundary, bounded user-initiated detection, and ordered fallback behavior. Reset should likewise be separate and Host-owned so policy and credential changes have honest atomic semantics. The reduced patch is 16 files / 331 additions instead of 32 files / 1,014 additions. Local lint, format, build, typecheck, Knip, Runtime (2,967 passed / 13 skipped), Runtime Host (1,033 passed), and Desktop (979 passed) are green. A focused recovery-navigation E2E was added, but its local run was blocked during Electron fixture startup before the assertion; I will not claim it as locally passing. The branch update is prepared locally and will be pushed after contributor sign-off. Prepared and posted with OpenAI Codex at the contributor request. |
Generated-by: Codex
| if (payload === null || typeof payload !== 'object' || Array.isArray(payload)) return false; | ||
| const error = (payload as Record<string, unknown>).error; | ||
| if (error === null || typeof error !== 'object' || Array.isArray(error)) return false; | ||
| return (error as Record<string, unknown>).code === 'unsupported_country_region_territory'; |
There was a problem hiding this comment.
[P3] The whole feature hangs on one external string literal, and its only guard uses the same literal.
return (error as Record<string, unknown>).code === 'unsupported_country_region_territory';codex-oauth-enrollment.test.ts pins the identical literal, so if the provider ever renames this code, the implementation and its test go wrong together and stay silent — the test shares the assumption it is supposed to be checking, so it is not independent verification of it.
What the user sees after a rename, which is the part that makes this worth raising: the classification falls back to provider_rejected, the renderer's reason no longer matches either special case, and subscriptionResultMessage re-classifies from the HTTP status embedded in the error text. The result is a generic auth/service message steering the user to retry or check their credentials — while the actual cause is a regional block and their credentials are perfectly fine.
That is exactly the asymmetric misdirection this PR exists to remove: telling someone with a configuration problem that they have a service problem makes them wait and retry, when the correct action is to configure a proxy. So the failure mode of this feature degrading is not "no improvement" — it is "back to actively misleading."
This is not a current defect. Today's behavior is correct, the matching works, and the code is compared as a structured field, not fished out of a message string — which is the right design and the reason this is P3 rather than higher.
Suggestion, non-blocking: mark this code at the provider-contract layer as an external contract whose change silently disables a feature, or emit one observable signal when a response lands in the status band where regional rejections normally arrive but matches no known code — rather than letting it disappear into provider_rejected without a trace.
Two things checked and found clean, recorded so they need not be re-derived:
- No provider text reaches the user. The chain carries a closed enum end to end (
OAuthTokenEndpointError.category→projection.failure→terminalFailureReason()→ localized copy). No provider payload string is surfaced. - The "actionable" action is not a URL. It dispatches
maka:jumpToSettingsSectionwithsectionandfocusIdas hard-coded constants — nowindow.open, nohref, nothing taken from the response. There is no open-redirect shape here; that was looked for specifically.
One hypothesis raised and withdrawn: outcome_unknown → network_unavailable looked like a semantic widening ("we don't know" reported as "can't reach the service"). It is not — the only throw site is inside a fetch catch, with aborted already split out, so the HTTP exchange demonstrably did not complete and "check your proxy and retry" is accurate advice.
Note on state, not a finding: this PR is currently CONFLICTING against main and has no check runs at all on this head. It will need a rebase before it can be merged, and CI has never actually run on it — "no runs" is not the same as "passing."
Independent untrusted-input & leakage line, bound to 272d4d6c55931ea00d5667fb331a3dbf28b841d6. Verified at the gate 2026-08-23 12:52 UTC. Only this surface was reviewed; no overall verdict is offered.
|
Hi — this PR conflicts with current I tested a rebase onto current
These are real source conflicts, so they need your judgement rather than a mechanical rebase — please rebase onto current Thanks for the contribution — happy to help if any conflict is unclear. AI-assisted maintenance note, not a review. It does not count as the required human review under |


Summary
Follow-up to #1301 and #1302. This does not change the proxy-routing fix from #1302.
Verification
npm run lintnpm run format:checknpm run buildnpm run typechecknpx knip --workspace apps/desktopnpx knip --workspace packages/uinpm --workspace @maka/runtime test— 2,967 passed, 13 skippednpm --workspace @maka/runtime-host test— 1,033 passednpm --workspace @maka/desktop test— 979 passedThe focused recovery-navigation E2E is included, but its local run timed out while setting up the Electron fixture before product assertions. The same local branch build/typecheck validation used the three-line fix now merged upstream as #3332.
Before: #3319 (comment)
After: #3319 (comment)
Compatibility
Runtime Host compatibility epoch 29 carries the two new closed OAuth failure codes. Epoch 28 is already used by the Fast service-tier profile change.
AI use
Select exactly one:
Tool(s) and scope: OpenAI Codex assisted with analysis, implementation, tests, verification, review response, and PR preparation. The human contributor reviewed the result and authorized submission.
Checklist
Does this PR entail a change in behavior?