feat(ai-engine): ADR-0011 PR-E3a — OAuthFlowOrchestrator#31
Merged
Conversation
ADR-0011 の実装段階 3a。issue #28 への対応として、PR-E1 (schema + token store) と PR-E2 (OAuthClient + LoopbackCallbackServer) を組み合わせる単一エントリの オーケストレータを実装する。本 PR でも UI / API は接続されない (PR-E3b で Route Handler + UI 統合)。 ## 設計判断 - process scope の singleton state (flows Map) で in-progress 状態を保持。 Next.js Route Handler が module scope で共有 - 1 mcpServerId につき同時に 1 フロー (pending 中の二重 start は throw) - state verify (CSRF 対策) は orchestrator が持つ - completed / failed 状態は次 start まで残す (UI が status を pull) ## 新規ファイル - packages/ai-engine/src/oauth/oauth-flow-orchestrator.ts: - startOAuthFlow: PKCE / state 生成 → loopback server 起動 → authorization URL 構築 → bg で callback 受領 → token 交換 → token store 保存 - getOAuthFlowStatus: polling 用の status 取得 (pending / completed / failed) - awaitOAuthFlowSettled: test helper - clearOAuthFlow: pending 中なら callbackHandle.close で bg 中断 - __resetAllFlowsForTest: @internal、test isolation 用 - packages/ai-engine/src/oauth/oauth-flow-orchestrator.test.ts (7 件) ## codex セカンドオピニオン対応 (push 直前 review) [HIGH] スロット予約レース: await より前に flows.set で sentinel 予約 (同期で check-and-set 完結)、callback server 起動失敗時は sentinel を片付け [HIGH] clearOAuthFlow が bg を停止しない: flows entry に callbackHandle を 保持し、clear 時に close() で bg を中断 [MEDIUM] concurrent race の test 不足: Promise.all で並走 start を呼んで fulfilled 1 / rejected 1 を確認するテスト追加 [MEDIUM] scope split が空文字混入: result.scope.split(/\\s+/).filter(Boolean) と scopesParsed.length > 0 の conditional spread に変更 [MEDIUM] flows.get の no entry 時に silent: console.warn を追加 [MEDIUM] __resetAllFlowsForTest が public: @internal JSDoc 追加 [LOW] completed の authorizationUrl 保持: TODO コメントで PR-E3b の整理予告 [LOW] callbackHandle.close 失敗の swallow: console.warn を追加 ## テスト 7 件: start で pending / 成功で completed + token 保存 / state mismatch で failed / 二重 start で reject / 未開始 null / concurrent race / clearOAuthFlow で bg 中断 + 再 start 即可能。 pnpm typecheck 4/4 PASS / pnpm test core 99 + storage 97 + ai-engine 244 + frontend 273 = 713 PASS / pnpm lint exit 0。
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughウォークスルーOAuth フロー オーケストレーション機能の新規実装と統合により、プロセスローカルの OAuth 2.1 フロー制御機能が導入されました。 変更内容OAuth フロー オーケストレーション実装
シーケンス図sequenceDiagram
actor User
participant Client as Client Code
participant Orchestrator as OAuth Flow<br/>Orchestrator
participant LoopbackServer as Loopback<br/>Callback Server
participant Provider as OAuth Provider
participant Store as FileSystem<br/>OAuth Store
User->>Client: startOAuthFlow(config)
Client->>Orchestrator: startOAuthFlow(input)
Orchestrator->>Orchestrator: Reserve pending sentinel<br/>for mcpServerId
Orchestrator->>LoopbackServer: Start server
LoopbackServer-->>Orchestrator: callbackHandle + port
Orchestrator->>Orchestrator: Generate PKCE + state<br/>Build authorization URL
Orchestrator-->>Client: {authorizationUrl}
Note over Orchestrator: Background async process starts
User->>Provider: Click authorize link
Provider->>LoopbackServer: Redirect to localhost<br/>with code + state
LoopbackServer->>Orchestrator: Callback received
Orchestrator->>Orchestrator: Validate state (CSRF check)
Orchestrator->>Provider: Exchange code for tokens
Provider-->>Orchestrator: Access token + (optional refresh)
Orchestrator->>Store: Write normalized tokens
Store-->>Orchestrator: Stored
Orchestrator->>Orchestrator: Update status to completed
Orchestrator->>LoopbackServer: Close server
LoopbackServer-->>Orchestrator: Closed
Orchestrator-->>Client: Flow settled
見積もり コード レビュー工数🎯 4 (Complex) | ⏱️ ~45 分 関連する可能性のある PR
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 28 minutes and 41 seconds.Comment |
Contributor
Author
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/ai-engine/src/oauth/index.ts (1)
19-28: ⚡ Quick winテスト専用ヘルパーは public barrel から外した方がよいです。
__resetAllFlowsForTestとawaitOAuthFlowSettledをここで再 export すると、本番コードが内部同期手段や全消去 API に依存できてしまいます。テストは実装ファイルを直接 import できるので、公開面からは外して test-only な入口に閉じるのが無難です。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ai-engine/src/oauth/index.ts` around lines 19 - 28, Remove the test-only helpers from the public barrel: stop re-exporting __resetAllFlowsForTest and awaitOAuthFlowSettled from this file so production consumers cannot depend on test-only APIs; keep exports for the public APIs (e.g., startOAuthFlow, getOAuthFlowStatus, OAuthFlowStatus, StartOAuthFlowInput/Result) and update tests to import __resetAllFlowsForTest and awaitOAuthFlowSettled directly from './oauth-flow-orchestrator' (or a dedicated test-only entry) instead of from this barrel.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/ai-engine/src/oauth/oauth-flow-orchestrator.ts`:
- Around line 168-176: Replace the direct inclusion of raw exception text into
the public failureMessage stored in flows (the object set for input.mcpServerId)
with a generic, non-sensitive message or error code; keep the detailed
err.message/stack for server-side logging only (e.g., log via processLogger or a
dedicated logger inside the catch). Update all catch blocks that set
failureMessage (including the block around FileSystemOAuthStore.write and the
token exchange paths referenced) to populate failureMessage with a fixed string
like "OAuth flow failed" or a short error code, and ensure the original err is
logged via a secure logger so internal paths and filesystem errors are not
exposed to the UI/polling response.
- Around line 89-93: clearOAuthFlow can fail to cancel an in-progress
startOAuthFlow because flows.set(...) at start only places a sentinel and the
actual callbackHandle is stored later (callbackHandle, startOAuthFlow, flows,
mcpServerId). Fix by making the flow entry immutable for that run or by
validating ownership before resuming: when creating the initial entry in
startOAuthFlow, store a stable run id or the callbackHandle immediately in the
flows map (instead of a temporary empty object) and when the async continuation
(where callbackHandle is assigned) resumes, check that the current
flows.get(mcpServerId) still matches this run id/callbackHandle before
registering or continuing; alternatively update clearOAuthFlow to mark the entry
as cancelled and ensure startOAuthFlow aborts if it detects that cancelled
marker. Ensure all reads/writes use the same unique token so old start tasks do
not re-register stale callbackHandle.
- Around line 238-240: __resetAllFlowsForTest currently just calls flows.clear()
and leaves pending awaitCallback entries and loopback servers open; change it
into an async helper that iterates over the existing flow entries (the same
structure referenced by flows and the pending awaitCallback logic), call each
entry.close() (or the flow's cleanup/abort method) and await Promise.all of
those close promises before clearing flows, then export/rename it so tests can
await it (update oauth-flow-orchestrator.__resetAllFlowsForTest to return a
Promise and ensure tests call/await it to avoid leaked open handles and pending
loopback servers).
---
Nitpick comments:
In `@packages/ai-engine/src/oauth/index.ts`:
- Around line 19-28: Remove the test-only helpers from the public barrel: stop
re-exporting __resetAllFlowsForTest and awaitOAuthFlowSettled from this file so
production consumers cannot depend on test-only APIs; keep exports for the
public APIs (e.g., startOAuthFlow, getOAuthFlowStatus, OAuthFlowStatus,
StartOAuthFlowInput/Result) and update tests to import __resetAllFlowsForTest
and awaitOAuthFlowSettled directly from './oauth-flow-orchestrator' (or a
dedicated test-only entry) instead of from this barrel.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 59907c9b-1e80-4123-98f8-2f8b8f7af655
📒 Files selected for processing (3)
packages/ai-engine/src/oauth/index.tspackages/ai-engine/src/oauth/oauth-flow-orchestrator.test.tspackages/ai-engine/src/oauth/oauth-flow-orchestrator.ts
CR 1 周目 Major: - runId pattern 導入: 各 startOAuthFlow 呼び出しに UUID を発行し、bg IIFE が flows.set する前に「自分が現在の run か」を確認。clearOAuthFlow → 即 start race で旧 bg が新 pending を踏みつぶすバグを防ぐ。await 後の各分岐 (startListen 後 / completed / failed) で runId guard。 - failureMessage は固定文字列に正規化 (raw err.message を UI 経由で漏らさない)。 詳細は console.warn で server log に出す。 - __resetAllFlowsForTest を async 化、pending flow の callbackHandle.close() を await してから関数 return。テスト間の loopback fd リークを防ぐ。 codex セカンドオピニオン Major: - store.write 直前にも runId guard を追加。旧実装は write 後にしか guard が無く、 preempted な旧 run が storage にトークンを書き込むウィンドウがあった。 テスト追加: - 「store.write 直前に preempt されたら旧 run はトークンを書き込まない」 - 「clearOAuthFlow → 即 start で旧 bg が新 pending を踏まない (runId guard)」
Contributor
Author
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
This was referenced May 2, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ADR-0011 (Tally 側で OAuth 2.1 フローを管理する) の実装段階 3a。issue #28 への対応として、PR-E1 (schema + token store) と PR-E2 (OAuthClient + LoopbackCallbackServer) を組み合わせる OAuthFlowOrchestrator を実装する。
本 PR でも UI / API は接続されない (PR-E3b で Route Handler + UI 統合)。
設計判断
flowsMap) で in-progress 状態を保持。Next.js Route Handler が module scope で共有公開 API
startOAuthFlow(input)getOAuthFlowStatus(mcpServerId)awaitOAuthFlowSettled(mcpServerId)clearOAuthFlow(mcpServerId)callbackHandle.close()で bg 中断__resetAllFlowsForTest/* @internal */、test isolation 用codex セカンドオピニオン対応 (push 直前 review)
clearOAuthFlowで bg 中断@internalTest plan
orchestrator test 7 件: start で pending / 成功で completed + token 保存 / state mismatch で failed / 二重 start で reject / 未開始 null / concurrent race / clearOAuthFlow で bg 中断 + 再 start 即可能。
Related
Summary by CodeRabbit
リリースノート
New Features
Tests