You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@maka.apache.org by "sosyz (via GitHub)" <gi...@apache.org> on 2026/08/20 18:41:53 UTC
[PR] fix(desktop): show the Codex device sign-in code on connection-detail re-login [maka]
sosyz opened a new pull request, #3358:
URL: https://github.com/apache/maka/pull/3358
## Summary
The connection detail sheet's re-login notice (`OAuthReloginNotice`) drives the same browser-assisted OAuth flow as the provider catalog panel, but never rendered the flow's `stateHint`. For Codex that hint is the 9-digit device user code the authorization page requires — the verification URL is the static device page and does not embed the code — so a re-login started from the notice could never be completed: the browser asked for a code the app never showed.
- The re-login service mapping (`oauthLoginServiceFor` / `OAuthLoginService`) moves out of `use-connection-detail.ts` into a leaf module, `oauth-relogin-service.ts`, and gains a `showsDeviceCode` flag: `true` for Codex, `false` for xAI, whose authorization page needs no manual code (mirroring the catalog panel's existing `!isXai` guard). The leaf shape (no React, type-only imports at runtime) keeps the mapping loadable by the node:test suite.
- `OAuthReloginNotice` appends the sign-in code to the banner description while authorization is pending, reusing the catalog panel's `deviceCode` copy so both entry points speak the same words.
Fixes #3357
## Verification
- `npm run lint` — clean
- `npm run format:check` — clean
- `npm run typecheck` (desktop: preload + main + renderer + storybook) — clean
- `npm run build` — clean
- `npx knip --workspace apps/desktop` — clean
- `npm --workspace @maka/desktop test` — 982 passed, 0 failed (978 before this PR + 4 new)
The new `oauth-relogin-device-code.test.ts` pins the per-service device-code contract (Codex `showsDeviceCode: true`, xAI `false`, non-browser-assisted providers `null`, and host-scoped bridge routing). The banner render itself has no existing component-test harness (the repo's fake DOM is deliberately hooks-only), so the JSX change is covered by typecheck and the contract pin rather than a render assertion.
## AI use
Select exactly one:
- [ ] No generative tool made a substantive contribution
- [x] Generative tooling made a substantive contribution
Tool(s) and scope: Claude Code (Fable 5) performed the diagnosis, implementation, tests, and verification. The human contributor reviewed the result and authorized submission. The commit carries a Generated-by trailer.
## Checklist
- [x] Tests cover the change and fail without it
- [x] Lint, format, typecheck and the affected suites pass locally
Does this PR entail a change in behavior?
- [x] Yes — described under Summary above
- [ ] No
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] fix(desktop): show the Codex device sign-in code on connection-detail re-login [maka]
Posted by "YayoiNanoka (via GitHub)" <gi...@apache.org>.
YayoiNanoka merged PR #3358:
URL: https://github.com/apache/maka/pull/3358
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] fix(desktop): show the Codex device sign-in code on connection-detail re-login [maka]
Posted by "sosyz (via GitHub)" <gi...@apache.org>.
sosyz commented on PR #3358:
URL: https://github.com/apache/maka/pull/3358#issuecomment-5468557848
@YayoiNanoka thanks for the review. Added before/after screenshots to the description: **before (en)** shows the notice with no code while authorization is pending; **after** shows the code in both locales — `Sign-in code: …` (en) and `登录码：…` (zh). Each was taken on a real Codex device-authorization attempt from the dev app; the codes shown are short-lived and already expired.
On the lockfile commit: agreed it is independent. I kept it because `main` currently fails `npm ci` under `allow-remote=none` (#3381 was reverted by #3148's lockfile regeneration), so the branch would not install cleanly without it. Happy to drop `881be43` if a maintainer prefers to restore #3381 separately — just say the word.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] fix(desktop): show the Codex device sign-in code on connection-detail re-login [maka]
Posted by "YayoiNanoka (via GitHub)" <gi...@apache.org>.
YayoiNanoka commented on PR #3358:
URL: https://github.com/apache/maka/pull/3358#issuecomment-5468493593
## PR Assessment
### What problem does this PR solve?
OpenAI Codex re-login from the connection-detail sheet starts device authorization but does not display the `stateHint` containing the required 9-digit user code. The browser therefore asks for information that this entry point never exposes, making the flow impossible to complete.
### How does this PR solve the problem?
The patch keeps `useOAuthLoginFlow` as the single authorization lifecycle owner. It:
- marks Codex as requiring its device code to be displayed while keeping xAI excluded;
- renders the existing `flow.stateHint` in `OAuthReloginNotice`;
- reuses the catalog panel's localized device-code copy;
- separately restores six canonical npm registry resolutions without changing package versions.
No new authorization state machine, persistence, or parallel source of truth is introduced.
### Is the problem correctly defined?
Correct.
The linked issue, current base implementation, and existing catalog flow agree on the root cause: the re-login path receives the required code but drops it at the rendering boundary.
### Principle-based assessment
- First principles: while Codex device authorization is pending, the exact `stateHint` must remain visible until completion; providers that do not require manual entry must not expose it. The patch preserves both invariants.
- Occam's razor: the product fix is small and stays in the existing service mapping and notice renderer. The previous test-only production seam is no longer present.
- Minimal sufficient solution: the authentication change is minimal and sufficient. The lockfile restoration is correct but independent of the UI defect and slightly broadens the PR's scope.
### Required conclusions
1. Is the solution optimal for the actual problem? The authentication fix is; the PR as a unit is slightly broader because of the independent lockfile restoration.
2. Production code that can be deleted: none identified.
3. Low-quality tests that can be deleted or replaced: none remain in the current diff. No automated test protects the rendered regression.
4. Deeper refactor required: no. The final structure should keep the OAuth lifecycle in `useOAuthLoginFlow`, provider-specific presentation metadata in the existing service mapping, and rendering in the notice.
5. Ready to merge: conditionally, after the UI evidence requirement below is satisfied and an independent human committer approves it.
6. Residual risks or gaps: the actual Desktop/device-authorization flow and visual presentation were not independently reproduced during this review.
## Review Findings
### P1 Blocker
None.
### P2 Should Fix
#### [P2 Should Fix] Add the required visual evidence for the untested UI change
- Location: `CONTRIBUTING.md:87`; `apps/desktop/src/renderer/settings/provider-connection-detail.tsx:1136`
- Trigger: a reviewer evaluates the new Banner content without component/E2E coverage and without a screenshot or recording → the only evidence that the device code is visible and usable in the real Desktop layout is the author's manual assertion.
- Root cause: the PR changes user-visible UI and explicitly states that the Banner render has no automated coverage, but its description contains no before/after screenshots or recording as required by the repository's contribution guide.
- Evidence: the current diff conditionally inserts the localized label and `<code>{deviceCode}</code>`; the PR checklist leaves regression coverage unchecked and reports only manual verification.
- Required direction: attach before/after screenshots or a short recording of the Codex connection-detail re-login flow showing the code while authorization is pending. If practical, include both supported locales or explain which locale was exercised.
### Suggestion
#### Keep the lockfile restoration independently reviewable
- Location: `package-lock.json`
- The restoration matches npm's published tarball URLs and SHA-512 integrity values and changes no package versions, so no correctness defect was found.
- It is nevertheless independent of the Codex UI bug. If maintainers restore #3381 through a dedicated change, drop commit `881be43746f726c25e3e9c2de165fd635fa719e6` here; otherwise ensure the human review explicitly covers this dependency-provenance change.
## Verification
- Base: `9ff267510d381aba89f255b0d77bf6d277601de2`
- Head: `881be43746f726c25e3e9c2de165fd635fa719e6`
- Executed: fetched `refs/pull/3358/head` into a push-disabled isolated checkout; verified base/head and clean merge; inspected the linked issue, complete diff, OAuth controller, catalog rendering, localization, contribution rules, and commit provenance; ran renderer TypeScript checking, changed-source lint, renderer architecture tests/check, ASF header audit, and `git diff --check`; compared all six changed lockfile records with npm registry metadata.
- Key results: clean merge; renderer typecheck passed; 62 architecture tests passed; architecture check passed; lint and ASF header audit passed; all six registry URLs and integrity values matched; the current hosted `test` and other surfaced checks are successful.
- Temporary tests: not created; isolated checkout removed, never committed or pushed.
- Original workspace: confirmed unchanged.
- Not verified: a real Desktop Codex authorization attempt, visual layout in either locale, a fresh local dependency installation, or the full Desktop suite. Existing dependencies were reused read-only for targeted local checks.
## Conclusion
Merge conditionally.
Add the required UI evidence, then obtain the independent human approving review required by `CONTRIBUTING.md`. If the lockfile commit remains, that review should explicitly include it. No reachable authentication correctness, security, or public-contract blocker was identified in the reviewed revision.
<details>
<summary>点击展开中文</summary>
## PR 判断
### 这个 PR 解决了什么问题？
从连接详情页重新登录 OpenAI Codex 时，应用会启动设备授权，却不显示包含必需 9 位用户码的 `stateHint`。因此浏览器要求用户输入一个该入口从未展示的值，导致流程无法完成。
### 这个 PR 如何解决这个问题？
该补丁继续让 `useOAuthLoginFlow` 作为唯一的授权生命周期所有者。它：
- 标记 Codex 需要展示设备码，同时继续排除无需手动输入的 xAI；
- 在 `OAuthReloginNotice` 中渲染现有的 `flow.stateHint`；
- 复用 provider 目录面板已有的本地化设备码文案；
- 另外恢复六个 canonical npm registry resolution，不改变任何包版本。
它没有新增授权状态机、持久化状态或并行事实源。
### 这个问题定义得对吗？
Correct（正确）。
关联 Issue、当前 base 实现以及已有目录登录流程对根因的描述一致：重新登录路径已经拿到必需的设备码，却在渲染边界丢弃了它。
### 原则性判断
- 第一性原理：Codex 设备授权等待期间，准确的 `stateHint` 必须持续可见；不需要手动输入的 provider 不应展示它。补丁同时维持了这两个不变量。
- 奥卡姆剃刀：产品修复很小，仍位于既有 service mapping 和 notice renderer 中。旧版本中为测试新增的生产代码接缝已经不存在。
- 最小充分解：认证改动是最小且充分的。lockfile 恢复是正确的，但与 UI 缺陷相互独立，略微扩大了 PR 范围。
### 必答结论
1. 当前方案对实际问题是否最优？认证修复是；但整个 PR 因包含独立 lockfile 恢复而略宽。
2. 可删除的生产代码：未发现。
3. 可删除或替换的低质量测试：当前 diff 中已不存在。现在没有自动化测试保护实际渲染回归。
4. 是否需要更深层重构：不需要。最终结构应继续由 `useOAuthLoginFlow` 管理 OAuth 生命周期，由现有 service mapping 保存 provider 特定展示信息，并由 notice 负责渲染。
5. 当前 revision 是否可以合入：有条件可以；需要先满足下面的 UI 证据要求，并由独立的人类 committer 批准。
6. 剩余风险或验证缺口：本次审查没有独立复现真实 Desktop 设备授权流程及视觉呈现。
## Review Findings
### P1 阻塞
无。
### P2 应该改
#### [P2 应该改] 为没有自动化覆盖的 UI 改动补充仓库要求的视觉证据
- 位置：`CONTRIBUTING.md:87`；`apps/desktop/src/renderer/settings/provider-connection-detail.tsx:1136`
- 触发条件：reviewer 在没有组件/E2E 覆盖、也没有截图或录像的情况下审查新增 Banner 内容 → 设备码在真实 Desktop 布局中是否可见、可用，只剩作者的手动验证陈述作为证据。
- 根因：该 PR 修改了用户可见 UI，并明确说明 Banner 渲染没有自动化覆盖，但 PR 描述没有包含仓库贡献指南要求的前后截图或录像。
- 证据：当前 diff 条件性插入本地化标签和 `<code>{deviceCode}</code>`；PR checklist 没有勾选回归测试覆盖，验证部分只报告了手动验证。
- 必需方向：附上 Codex 连接详情重新登录流程的前后截图或短录像，展示授权等待期间出现设备码。如果实际可行，请覆盖两种受支持语言；否则说明验证了哪种语言。
### 建议
#### 保持 lockfile 恢复可被独立审查
- 位置：`package-lock.json`
- 恢复内容与 npm 发布的 tarball URL 和 SHA-512 integrity 一致，也没有改变包版本，因此没有发现正确性缺陷。
- 但它与 Codex UI 缺陷相互独立。如果维护者通过单独改动恢复 #3381，应从本 PR 删除提交 `881be43746f726c25e3e9c2de165fd635fa719e6`；否则应确保人类 review 明确覆盖这项依赖来源变更。
## 验证
- Base：`9ff267510d381aba89f255b0d77bf6d277601de2`
- Head：`881be43746f726c25e3e9c2de165fd635fa719e6`
- 已执行：在禁止 push 的隔离 checkout 中获取 `refs/pull/3358/head`；验证 base/head 和干净合并；检查关联 Issue、完整 diff、OAuth controller、目录面板渲染、本地化、贡献规则和提交来源；运行 renderer TypeScript 检查、变更源码 lint、renderer architecture 测试及检查、ASF header audit 和 `git diff --check`；将六条 lockfile 变更与 npm registry 元数据逐项对比。
- 关键结果：干净合并；renderer typecheck 通过；62 个 architecture 测试通过；architecture check、lint 和 ASF header audit 通过；六条 registry URL 与 integrity 全部匹配；当前托管 `test` 及其他展示出的 checks 均成功。
- 临时测试：未创建；隔离 checkout 已删除，从未 commit 或 push。
- 原工作区：确认未改变。
- 未验证：真实 Desktop Codex 授权、两种语言下的视觉布局、全新本地依赖安装或完整 Desktop 测试套件。定向本地检查只读复用了已有依赖。
## 结论
有条件合入。
补充要求的 UI 证据，然后获得 `CONTRIBUTING.md` 要求的独立人类 approving review。如果保留 lockfile 提交，该 review 应明确覆盖它。本次审查没有在当前 revision 中发现可达的认证正确性、安全或公共契约阻塞问题。
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org