feat: ADR-0011 PR-E4 — chat-runner OAuth 削除 + token 注入#33
Merged
Conversation
…+ token 注入
ADR-0011 PR-E4: chat-runner から OAuth フロー関連を全削除し、buildMcpServers が
FileSystemOAuthStore から token を読んで MCP HTTP transport の Authorization header
に注入する形に切替える。AuthRequestCard を chat 文脈から切り離して project settings
の MCP server 行に配置し、UI 起点の Connect ボタンとして再構成する。
core (schema):
- McpServerConfig.oauth を required 化 (PR-E1 で optional 導入、本 PR で必須化)
- ChatBlockSchema から auth_request 型を削除
ai-engine:
- chat-runner.ts: ~250 行削除 (runOAuthCallback / handleAuthToolResult /
findLatestPendingAuthRequest / parseAuthToolName 経路 / TurnState.stashedAuthUses)
- auth-detector.ts: ファイル削除
- server.ts: oauth_callback WS message 経路と ChatOAuthCallbackSchema を削除
- stream.ts: chat_auth_request event 型を削除
- mcp/build-mcp-servers.ts: oauthStore deps 追加 + async 化、token を Authorization
header に注入。expiresAt が過去なら無視 (codex Major 対応)
- agent-runner.ts: deps に oauthStore 追加
frontend:
- lib/store.ts: chat_auth_request handler / sendOAuthCallback action を削除
- lib/ws.ts: sendOAuthCallback を削除
- components/chat/chat-message.tsx: auth_request branch / AuthRequestCard import を削除
- components/{chat → mcp}/auth-request-card.tsx: prop signature を
`block: AuthRequestBlock` から `{ mcpServerId, mcpServerLabel }` に refactor、
ディレクトリも mcp/ に移動 (chat 文脈非依存)
- components/dialog/project-settings-dialog.tsx: oauth.clientId 入力欄を追加し、
保存済み + 編集なし + clientId 入力済 のとき AuthRequestCard を埋め込み表示
codex セカンドオピニオン Major 対応:
1. 期限切れトークンの扱い: buildMcpServers で expiresAt を確認し過去なら null 扱い
2. isOAuthConnectable: 全フィールド比較 (JSON.stringify) で options 編集も検知
3. retrograde guard: 外部 MCP tool_use テストに `chat_auth_request` event が emit
されないことを assert (旧経路の誤再導入を CI で検知)
テスト:
- core: 94 / storage: 97 / ai-engine: 235 / frontend: 280 すべて pass
- 削除: chat-runner.test.ts の auth_request 変換 describe (~270 行)、auth-detector.test.ts
- 追加: build-mcp-servers expiry / token-type 注入 / クロス config テスト
|
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)
📝 WalkthroughWalkthroughOAuth authorization flow transitions from in-chat request/callback handling to route handler–driven orchestration with pre-computed Authorization headers injected into MCP server configurations. Token storage, schema requirements, and component integrations are updated accordingly. ChangesOAuth Authorization Flow Refactor
Sequence Diagram(s)sequenceDiagram
participant Client as Frontend Client
participant Settings as ProjectSettingsDialog
participant AuthCard as AuthRequestCard
participant Route as OAuth Route Handler<br/>/api/.../oauth
participant OAuthRegistry as OAUTH_REGISTRY<br/>(External Provider)
participant Tally as Tally Core<br/>(Token Storage)
Client->>Settings: Enter OAuth Client ID
Settings->>AuthCard: Render (when clientId saved)
AuthCard->>Route: POST /oauth (start flow)
Route->>OAuthRegistry: Redirect to provider
OAuthRegistry->>Route: Authorization code
Route->>Tally: Store access token
AuthCard->>Route: Poll GET /oauth
Route->>AuthCard: Return completed status
AuthCard->>Client: Show "認証済"
sequenceDiagram
participant Chat as ChatRunner
participant Store as FileSystemOAuthStore
participant Builder as buildMcpServers
participant MCPServer as MCP HTTP Server
Chat->>Builder: buildMcpServers(configs, oauthStore)
Builder->>Store: read(mcpServerId) for each config
Store-->>Builder: { accessToken, tokenType, expiresAt }
Builder->>Builder: Check expiration vs Date.now()
alt Token valid & not expired
Builder->>Builder: Inject Authorization header
else Token missing or expired
Builder->>Builder: Skip Authorization header
end
Builder-->>Chat: MCP configs with headers
Chat->>MCPServer: Connect with Authorization
MCPServer-->>Chat: Authenticated response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: Turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 👉 Get your free trial and get 200 agent minutes per Slack user (a $50 value). 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 51 minutes and 43 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/core/src/schema.ts (1)
385-410:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
auth_requestの即時削除で既存チャット履歴が読めなくなります。
ChatMessageSchema/ChatThreadSchemaはこの union をそのまま読むので、以前に永続化したauth_requestblock を含む chat は parse 失敗になります。project.yaml側と違って read-time の互換処理が無いので、少なくとも legacy parser を残すか、読み込み時 migration を先に入れたいです。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/schema.ts` around lines 385 - 410, The ChatBlockSchema removal of the old auth_request variant will break parsing of persisted chats (ChatMessageSchema / ChatThreadSchema) that contain legacy auth_request blocks; restore backward compatibility by adding either (a) a legacy union member for auth_request in ChatBlockSchema (or a z.union fallback) or (b) implement a read-time migration that transforms legacy auth_request blocks to the new representation before parsing; update the discriminated union logic in ChatBlockSchema and the code paths that call ChatMessageSchema/ChatThreadSchema to apply the migration/conversion so existing persisted chat data can be parsed without errors.packages/frontend/src/app/api/projects/[id]/route.test.ts (1)
121-139:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
httpURL hardening の検証が偽陽性になっています。この payload は
oauthを入れていないので、今は URL 制約ではなくMcpServerConfigSchema.oauth必須化で 400 になります。これだとhttp://example.com拒否の回帰を拾えません。修正例
{ id: 'a', name: 'A', kind: 'atlassian', url: 'http://example.com/mcp', + oauth: { clientId: 'cid' }, options: { maxChildIssues: 30, maxCommentsPerIssue: 5 }, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/frontend/src/app/api/projects/`[id]/route.test.ts around lines 121 - 139, The test currently fails due to missing McpServerConfigSchema.oauth causing a 400 before URL validation; update the test payload in the PATCH call to include a valid oauth object for the mcpServers entry (so schema validation passes) while keeping url: 'http://example.com/mcp' to trigger the HTTP non-loopback URL rejection. Locate the test case using the mcpServers array in this file (referenced by the PATCH helper, projectId and McpServerConfigSchema.oauth) and add a minimal valid oauth field appropriate for the atlassian kind so the response check (expect(res.status).toBe(400)) reflects the URL hardening check instead of missing oauth validation.packages/ai-engine/src/chat-runner.ts (1)
281-312:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftOAuth トークンの更新が既存の
ChatRunnerに反映されません。
buildMcpServers({ oauthStore })はここで一度しか実行されませんが、ensureQuery()は生きている subprocess を turn 間で再利用します。つまり project settings で接続/再接続しても、この runner は古いAuthorizationヘッダのまま次ターンを処理し続けます。expiresAtの無視も起動時にしか効かないので、設定画面で「認証完了」になっても同じセッションでは外部 MCP が未認証/期限切れのままになるはずです。OAuth state の変更時に query を破棄して再生成するか、turn ごとに MCP 設定を再構築する仕組みが必要です。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ai-engine/src/chat-runner.ts` around lines 281 - 312, ensureQuery/startQueryInternal builds MCP servers once (via buildMcpServer and buildMcpServers) and caches external configs (cachedExternalConfigById), so OAuth token updates in oauthStore are not picked up for long‑lived ChatRunner sessions; change the logic to detect OAuth state changes and recreate the query or rebuild MCPs: either invalidate/tearDown the existing query when oauthStore or externalConfigs (including expiresAt/token) change before reusing ensureQuery, or move the MCP construction (buildMcpServer/buildMcpServers) out of startQueryInternal into a per-turn refresh step so each turn re-reads oauthStore and rebuilds Authorization headers; implement detection using the same keys stored in cachedExternalConfigById or a version/hash of oauthStore and clear ensureQueryInflight/query via tearDownQuery when a change is detected.
🧹 Nitpick comments (1)
packages/ai-engine/src/chat-runner.test.ts (1)
619-677: 🏗️ Heavy liftMCP/OAuth 系のケースは別 test module に分割したいです。
このファイルはすでに 1,200 行超で、今回の PR-E4 / Task 11-13 の追加で
ChatRunner本体、prompt 組み立て、外部 MCP 統合の責務がさらに混ざっています。ここはchat-runner.mcp.test.tsのような専用ファイルへ切り出した方が保守しやすいです。As per coding guidelines, "Keep individual files under 500 lines; split larger files into multiple modules".
Also applies to: 680-1081
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ai-engine/src/chat-runner.test.ts` around lines 619 - 677, This test file contains MCP/OAuth-specific tests that should be extracted into a dedicated module to keep files under ~500 lines; create a new test file (e.g., chat-runner.mcp.test.ts) and move the PR-E4 test and the block noted (the MCP/OAuth-related tests spanning the comment's referenced region) into it. Ensure the new file imports the same helpers and classes used by the moved tests (mkdtempSync, tmpdir, rmSync, path, FileSystemProjectStore, FileSystemOAuthStore, FileSystemChatStore, ChatRunner, SdkLike, vi, etc.), preserve the test titles and setup/teardown, and remove the moved tests from packages/ai-engine/src/chat-runner.test.ts so the original file remains below the size guideline. Run tests to confirm imports and mocks (e.g., querySpy) still work after relocation.
🤖 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/frontend/src/components/dialog/project-settings-dialog.tsx`:
- Around line 44-55: isOAuthConnectable currently does a JSON.stringify equality
between saved and entry, but entry is a McpServerEntry that includes
runtime-only _uid so the comparison always fails; change the comparison to omit
ephemeral fields (at least _uid) from entry before stringifying or compare only
the relevant persisted fields (e.g. id, oauth, etc.). Update the function
isOAuthConnectable to derive a cleanedEntry (or a shallow pick of persisted
keys) from entry, then compare JSON.stringify(saved) ===
JSON.stringify(cleanedEntry) so saved matches unedited rows.
---
Outside diff comments:
In `@packages/ai-engine/src/chat-runner.ts`:
- Around line 281-312: ensureQuery/startQueryInternal builds MCP servers once
(via buildMcpServer and buildMcpServers) and caches external configs
(cachedExternalConfigById), so OAuth token updates in oauthStore are not picked
up for long‑lived ChatRunner sessions; change the logic to detect OAuth state
changes and recreate the query or rebuild MCPs: either invalidate/tearDown the
existing query when oauthStore or externalConfigs (including expiresAt/token)
change before reusing ensureQuery, or move the MCP construction
(buildMcpServer/buildMcpServers) out of startQueryInternal into a per-turn
refresh step so each turn re-reads oauthStore and rebuilds Authorization
headers; implement detection using the same keys stored in
cachedExternalConfigById or a version/hash of oauthStore and clear
ensureQueryInflight/query via tearDownQuery when a change is detected.
In `@packages/core/src/schema.ts`:
- Around line 385-410: The ChatBlockSchema removal of the old auth_request
variant will break parsing of persisted chats (ChatMessageSchema /
ChatThreadSchema) that contain legacy auth_request blocks; restore backward
compatibility by adding either (a) a legacy union member for auth_request in
ChatBlockSchema (or a z.union fallback) or (b) implement a read-time migration
that transforms legacy auth_request blocks to the new representation before
parsing; update the discriminated union logic in ChatBlockSchema and the code
paths that call ChatMessageSchema/ChatThreadSchema to apply the
migration/conversion so existing persisted chat data can be parsed without
errors.
In `@packages/frontend/src/app/api/projects/`[id]/route.test.ts:
- Around line 121-139: The test currently fails due to missing
McpServerConfigSchema.oauth causing a 400 before URL validation; update the test
payload in the PATCH call to include a valid oauth object for the mcpServers
entry (so schema validation passes) while keeping url: 'http://example.com/mcp'
to trigger the HTTP non-loopback URL rejection. Locate the test case using the
mcpServers array in this file (referenced by the PATCH helper, projectId and
McpServerConfigSchema.oauth) and add a minimal valid oauth field appropriate for
the atlassian kind so the response check (expect(res.status).toBe(400)) reflects
the URL hardening check instead of missing oauth validation.
---
Nitpick comments:
In `@packages/ai-engine/src/chat-runner.test.ts`:
- Around line 619-677: This test file contains MCP/OAuth-specific tests that
should be extracted into a dedicated module to keep files under ~500 lines;
create a new test file (e.g., chat-runner.mcp.test.ts) and move the PR-E4 test
and the block noted (the MCP/OAuth-related tests spanning the comment's
referenced region) into it. Ensure the new file imports the same helpers and
classes used by the moved tests (mkdtempSync, tmpdir, rmSync, path,
FileSystemProjectStore, FileSystemOAuthStore, FileSystemChatStore, ChatRunner,
SdkLike, vi, etc.), preserve the test titles and setup/teardown, and remove the
moved tests from packages/ai-engine/src/chat-runner.test.ts so the original file
remains below the size guideline. Run tests to confirm imports and mocks (e.g.,
querySpy) still work after relocation.
🪄 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: 25026917-c314-4bbb-8f3a-09225141e22e
📒 Files selected for processing (22)
packages/ai-engine/src/agent-runner.test.tspackages/ai-engine/src/agent-runner.tspackages/ai-engine/src/auth-detector.test.tspackages/ai-engine/src/auth-detector.tspackages/ai-engine/src/chat-runner.test.tspackages/ai-engine/src/chat-runner.tspackages/ai-engine/src/mcp/build-mcp-servers.test.tspackages/ai-engine/src/mcp/build-mcp-servers.tspackages/ai-engine/src/server.tspackages/ai-engine/src/stream.tspackages/core/src/schema.test.tspackages/core/src/schema.tspackages/frontend/src/app/api/projects/[id]/mcp/[mcpServerId]/oauth/route.test.tspackages/frontend/src/app/api/projects/[id]/mcp/[mcpServerId]/oauth/route.tspackages/frontend/src/app/api/projects/[id]/route.test.tspackages/frontend/src/components/chat/chat-message.tsxpackages/frontend/src/components/dialog/project-settings-dialog.test.tsxpackages/frontend/src/components/dialog/project-settings-dialog.tsxpackages/frontend/src/components/mcp/auth-request-card.test.tsxpackages/frontend/src/components/mcp/auth-request-card.tsxpackages/frontend/src/lib/store.tspackages/frontend/src/lib/ws.ts
💤 Files with no reviewable changes (4)
- packages/frontend/src/lib/ws.ts
- packages/ai-engine/src/auth-detector.ts
- packages/frontend/src/components/chat/chat-message.tsx
- packages/ai-engine/src/auth-detector.test.ts
CR Major 対応: McpServerEntry は UI ローカルの `_uid` を持つが、saved 側 (永続化済み McpServerConfig[]) には _uid が無い。JSON.stringify による全フィールド比較が _uid の 差分で常に false になり、Connect ボタンが一度も出ないバグだった。 entry から _uid を destructure で剥がしてから比較するよう修正。 回帰テスト 2 件追加: - 保存済み + clientId 入力済 → Connect ボタンが描画される - name を編集すると Connect ボタンが消えてヒント文に切り替わる
This was referenced May 3, 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 PR-E4: chat-runner から OAuth フロー関連を全削除し、buildMcpServers が FileSystemOAuthStore から token を読んで MCP HTTP transport の Authorization header に注入する形に切替える。AuthRequestCard を chat 文脈から切り離して project settings の MCP server 行に配置し、UI 起点の Connect ボタンとして再構成する。
McpServerConfig.oauthを required 化、auth_requestChatBlock 型を削除oauthStore引数追加 + Authorization header 注入 + expiresAt チェックADR-0011 ロードマップ進捗
主な変更点
core/schema
McpServerConfig.oauthを required (PR-E1 で optional 導入、本 PR で必須化)ChatBlockSchemaからauth_request型を削除ai-engine
auth-detector.tsファイル削除 + テスト削除chat-runner.ts:runOAuthCallback,handleAuthToolResult,findLatestPendingAuthRequest,parseAuthToolName経路、TurnState.stashedAuthUsesをすべて削除server.ts:oauth_callbackWS message 経路とChatOAuthCallbackSchemaを削除、ChatRunner/runAgentにoauthStoreを注入stream.ts:chat_auth_requestevent 型を削除mcp/build-mcp-servers.ts:oauthStoredeps 追加 + async 化、token をAuthorization: <tokenType> <accessToken>で header 注入。expiresAt が過去なら無視 (codex Major 対応)frontend
lib/store.ts:chat_auth_requesthandler /sendOAuthCallbackaction を削除lib/ws.ts:sendOAuthCallbackを削除components/chat/chat-message.tsx:auth_requestbranch /AuthRequestCardimport を削除components/{chat → mcp}/auth-request-card.tsx: prop signature をblock: AuthRequestBlock→{ mcpServerId, mcpServerLabel }に refactorcomponents/dialog/project-settings-dialog.tsx:oauth.clientId入力欄を追加、保存済み + 編集なし + clientId 入力済 のとき AuthRequestCard を埋め込み表示codex セカンドオピニオン Major 対応
buildMcpServersで expiresAt < now ならトークンを無視 (header 無し)。期限切れを盲目的に注入すると 401 が AI tool 失敗として埋もれて UI に通知できない問題を回避chat_auth_requestevent が emit されないことを assert (旧経路の誤再導入を CI で検知)Test plan
既知の限界 (PR-E5 で扱う)
Summary by CodeRabbit
リリースノート
新機能
リファクタリング