Skip to content

feat(runtime-host): bind OAuth login to Connection entities - #3924

Open
me2seeks wants to merge 3 commits into
apache:mainfrom
me2seeks:feat/3852-oauth-connection-entity
Open

feat(runtime-host): bind OAuth login to Connection entities#3924
me2seeks wants to merge 3 commits into
apache:mainfrom
me2seeks:feat/3852-oauth-connection-entity

Conversation

@me2seeks

@me2seeksme2seeks commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Summary

  • bind every interactive OAuth login to one explicit Connection target: create(providerType) or existing(connectionId)
  • allocate and recover OAuth Connection identity, credential publication, and authenticated receipts in Runtime Host/Storage
  • migrate Desktop add/re-login/status/refresh/logout paths to exact Connection identity and fail closed on mismatch or ambiguity
  • replace silent attempt supersede with operation_conflict and advance the Runtime Host compatibility epoch from 53 to 54

Stack

This is the OAuth entity-binding workstream of #3852 and is stacked after #3882. Review the final commit c88dd63b1 for this PR's isolated change.

Closes#3888.

Validation

  • npm --workspace @maka/core run typecheck
  • npm --workspace @maka/storage run typecheck
  • npm --workspace @maka/runtime-host run typecheck
  • npx tsc -p apps/desktop/tsconfig.main.json --noEmit
  • Storage full suite: 995 tests, 977 passed, 18 skipped
  • Runtime Host OAuth/protocol focused suite: 73 passed
  • Desktop OAuth/Connections focused suite: 17 passed
  • node scripts/protocol-epoch-check.mjs --base f1fa3f9c7 --head HEAD (53 -> 54)
  • git diff --check f1fa3f9c7..HEAD

@me2seeks

Copy link
Copy Markdown
ContributorAuthor

Addressed the current hosted format:check failure on head 04dd9c1b7 by applying Biome formatting to the two reported Storage files. The follow-up commit changes only line wrapping; two independent reviews confirmed no identity, authority, argument, or control-flow change.

The OAuth entity-binding commit is patch-equivalent across the final restack onto #3882. Validation: Storage 995 tests passed with 18 platform skips, Storage typecheck, Biome check, protocol epoch guard (54), and diff-check.

Generated-by: Codex

@Astro-HanAstro-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.

I reviewed this head and found no blocking issues.

Binds each interactive OAuth login to an explicit target (create(providerType) or existing(connectionId)) with persistent oauth-login-receipts (monotonic order, attemptId dedup, target↔identity checks), turning silent supersede into operation_conflict; correct identity matching, fail-closed conflict, legacy adoption CAS retry, and onboarding ticket fixing TOCTOU; hosted test+windows_recovery SUCCESS.

No P0-P3.

简体中文该头无阻断。

Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.

@github-actionsgithub-actionsBot added the effort/XL Over 1000 readable lines label Aug 27, 2026
@me2seeks
me2seeksforce-pushed the feat/3852-oauth-connection-entity branch from 04dd9c1 to 01a325eCompareAugust 29, 2026 16:25

@M4n5terM4n5ter left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I found no P0-P3 source issues on exact head 01a325eff179de48ac0b4e371f5c64fe1f7f6138, but I cannot approve this head while its required test check is red.

The first run failed three WorkHub E2E cases; I reran the failed job on the unchanged head and it again failed the two reconstruction cases: WorkHub rebuilds delegated execution feedback after navigating away and back and WorkHub defers destructive correction until linked delegation exists. The PR does not modify WorkHub and the frozen base's hosted test is green, so I am not attributing these failures to this patch without a causal base/head comparison. The gate can be lifted by making the exact-head check green or by demonstrating and resolving an inherited failure with repeatable base/head evidence.

On the reviewed change itself, the flow keeps one immutable Connection identity from the closed create(providerType) / existing(connectionId) target through Host admission, Storage allocation, credential/catalog commit, the durable receipt, and Desktop completion. A new Connection remains invisible until authentication commits; exact re-login and account actions preserve the selected ID; ambiguous aggregate mutations fail closed. The durable intent recovers vault/catalog/receipt boundaries before orphan cleanup.

Validation outside the failing WorkHub E2E gate included the complete Core, Storage, and Runtime Host suites; focused Desktop OAuth tests; affected builds; protocol epoch guard 68 -> 69; diff check; and a byte-identical current-main merge tree.


Automated review notice: This comment was posted by an automated review agent operated by M4n5ter. It is not an independent human review and does not replace one.

@Astro-HanAstro-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 restacking this onto the current Connection foundation and narrowing the PR to the OAuth entity-binding slice. The underlying problem is real: OAuth login must target an explicit create(providerType) or existing(connectionId) intent rather than selecting a Connection by provider.

The overall architecture is sound. Runtime Host owns the active OAuth lifecycle, Storage remains the single durable Connection and credential authority, and Desktop only carries the user’s exact target. The receipt and journal mechanisms have distinct recovery responsibilities and do not create a second Connection authority.

I found one P2 concurrency issue:

When OAuth request A has been admitted by the Host but is still waiting for browser presentation, request B calls RuntimeHostOAuthPresentation.expect(). The current implementation rejects and replaces A’s pending presentation before B reaches Host admission. The Host correctly rejects B with operation_conflict, but A then observes its rejected presentation while startedOnHost=true and calls cancelOAuthLogin(), cancelling the original active login.

This is a reachable concurrent Desktop path and contradicts the intended “a conflicting second start must not cancel the active attempt” behavior. The impact is a recoverable interrupted login rather than identity corruption, so I am classifying it as P2 rather than P1.

The smallest fix is to make presentation admission non-destructive: if a presentation is already pending, reject the new expectation without replacing the existing one. Please add a regression where request B arrives after A is active on the Host but before A’s browser presentation completes, and assert that A is never cancelled.

The exact-head test check is currently red on two WorkHub reconstruction E2E assertions. They do not appear related to OAuth, but the required exact-head check still needs to be green before merge.

Review analysis was assisted by Codex and independent @reviewer agents. Astro-Han verified the exact head, the concurrent production path, the severity and recoverability, and the current CI evidence, and owns this review.

中文对照

谢谢你把这个 PR 重新整理到当前 Connection 基础上,并收敛为 OAuth entity binding 这一条完整切片。底层问题是真实的:OAuth 登录必须明确指向 create(providerType)existing(connectionId),不能继续按 provider 猜测某条 Connection。

整体架构是正确的。Runtime Host 持有活跃 OAuth 生命周期,Storage 仍然是 Connection 和 credential 的唯一持久化 authority,Desktop 只传递用户选择的精确目标。receipt 与 journal 分别承担恢复职责,没有形成第二套 Connection authority。

当前有一个 P2 并发问题:

当 OAuth 请求 A 已被 Host 接受、但仍在等待浏览器展示时,请求 B 会调用 RuntimeHostOAuthPresentation.expect()。当前实现会先拒绝并替换 A 的 pending presentation,然后 B 才进入 Host admission。Host 会正确地以 operation_conflict 拒绝 B,但 A 随后发现 presentation 被拒绝,并且 startedOnHost=true,于是调用 cancelOAuthLogin(),反而取消了原本活跃的登录。

这是可达的 Desktop 并发路径,也违背了“第二个冲突请求不能取消活跃 attempt”的目标。后果是一次可恢复的登录中断,而不是身份损坏,因此定为 P2,不升 P1。

最小修复是让 presentation admission 不具破坏性:已有 pending presentation 时,只拒绝新 expectation,不替换旧 expectation。请补一个回归测试:B 在 A 已被 Host 接受、但 A 的浏览器展示尚未完成时到达,并确认 A 从未被取消。

当前 exact-head test 还因为两条 WorkHub reconstruction E2E 断言失败而红。它们看起来与 OAuth 无关,但合并前 required exact-head check 仍必须恢复绿色。

本次审查分析由 Codex 和独立的 @reviewer 子代理协助;Astro-Han 核验了精确 head、并发生产路径、问题的分级与可恢复性以及当前 CI 证据,并对本次 Review 负责。

@me2seeks
me2seeksforce-pushed the feat/3852-oauth-connection-entity branch from 01a325e to 23cae6dCompareAugust 30, 2026 04:20
Make interactive OAuth enrollment create or reauthenticate one exact Connection, recover credential and catalog publication durably, and keep Desktop account actions entity-scoped.
Generated-by: Codex
@me2seeks
me2seeksforce-pushed the feat/3852-oauth-connection-entity branch from 23cae6d to 4fbdfe5CompareAugust 30, 2026 14:13

@Astro-HanAstro-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.

I found one concurrency issue in the Desktop OAuth presentation bridge. The Host/storage ownership and connection-identity design otherwise look coherent on this exact head. The required test check is still running, and this PR still needs an independent human committer review.

中文摘要

发现 1 个并发路径 P2:第二次登录请求会先替换 Desktop 侧已有 presentation expectation,随后即使 Host 正确拒绝第二次请求,也会反过来取消第一条已经被 Host 接受的登录。其余 Host/storage/Connection identity 边界在当前 exact head 上未发现新的 P0/P1。

AI-assisted review disclosure: Codex coordinated independent reviewer lanes; Astro-Han independently checked the exact head, production path, and severity, and owns this review.

Comment threadapps/desktop/src/main/runtime-host-oauth-ipc-main.ts
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/XLOver 1000 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[OAuth] Bind interactive login to explicit Connection entities

3 participants

@me2seeks@M4n5ter@Astro-Han