feat: ADR-0011 PR-E1 — OAuth schema + token store の土台 (issue #28)#29
Merged
Conversation
ADR-0011 (Tally 側で OAuth 2.1 フローを管理する) の実装段階 1。 issue #28 への対応として、外部 MCP server の access token を Tally で 保持して SDK の \`headers\` に inject するための型定義 / 永続化層を作る。 PR-E2 (OAuthClient + LoopbackCallbackServer) 以降の前提となる schema と storage を切り出した最小単位の PR。本 PR だけでは OAuth フローは動かない。 ## docs/adr/0011-tally-managed-oauth-flow.md 設計判断と PR 分割計画 (E1 〜 E5) を記録。後方互換は不要 (PoC 段階)、 oauth フィールドは PR-E1 では optional・PR-E4 で旧 auth_request 経路の 削除と同時に required 化、token は MVP では平文 YAML + 0o600 で保持 (暗号化は ADR-0012 で別途検討) と明記。 ## packages/core - McpOAuthConfigSchema (新規): McpServerConfig に紐付く client 設定。 clientId 必須、scopes optional (kind ごとの default 適用)。 - McpServerConfigSchema.oauth: optional で追加。PR-E4 で required 化予定。 - McpOAuthTokenSchema (新規): token store の YAML スキーマ。accessToken 必須、refreshToken / expiresAt / scopes optional、tokenType は default 'Bearer'。mcpServerId は McpServerIdRegex で charset 制限。 - src/oauth/atlassian.ts (新規): Atlassian Cloud OAuth 2.1 endpoint (authorize / token / audience / default scopes)。kind ごとの registry として OAUTH_REGISTRY も export。 - index.ts: 上記の公開 export を追加。 ## packages/storage - ProjectPaths.oauthDir (project-dir.ts): \`<projectDir>/oauth/\` を追加。 - FileSystemOAuthStore (oauth-store.ts, 新規): read/write/delete/list の 4 操作。1 server 1 ファイルで個別 IO が O(1)。write は mode 0o600 を 強制 (Windows では chmod no-op で許容)。read は YamlValidationError のみ warn + null、FS 系エラーは再 throw (silent fail で原因隠蔽を防ぐ)。 - index.ts: OAuthStore type と FileSystemOAuthStore class を export。 ## テスト - core/oauth/atlassian.test.ts: endpoint と registry の存在確認、 default scopes に offline_access (refresh_token に必須) が含まれる確認。 - core/schema.test.ts: McpServerConfigSchema.oauth の round-trip / 空 clientId reject、McpOAuthTokenSchema の最小フィールド + default tokenType / mcpServerId charset / accessToken 空チェック。 - storage/oauth-store.test.ts: read/write/list/delete の round-trip、 ファイル mode 0o600 確認、破損 YAML の warn + null、複数 server の list がソート順、未存在 delete が no-op。 pnpm typecheck 4/4 PASS / pnpm test core 100 + storage 95 全 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 (6)
📝 WalkthroughWalkthroughこのプルリクエストは、外部 MCP サーバー向けの OAuth 2.1 トークン管理インフラストラクチャを導入します。OAuth プロバイダー設定、トークンストレージシステム、スキーマ拡張、および関連テストを追加し、ADR-0011 で定義された Tally 管理型 OAuth フローの実装基盤を構築します。 ChangesOAuth プロバイダー設定とスキーマ基盤
OAuth トークンストレージシステム
プロジェクトディレクトリとクリアプロセスの統合
ドキュメント
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related issues
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)
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 48 minutes and 11 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: 5
🧹 Nitpick comments (3)
packages/core/src/schema.test.ts (1)
789-835: ⚡ Quick win
acquiredAt/expiresAtの不正 datetime 文字列を弾くテストが欠けている
schema.tsのコメント（line 315）には「z.string()のままだと'not-a-date'等が通り、PR-E2 の expiry 判定が境界バグを起こす」という Codex P2 指摘への対応として.datetime()を採用したと明記されています。ところがその検証テストがないため、将来.datetime()をz.string()に戻すリグレッションを防げません。✅ 追加案
+ it('acquiredAt が ISO 8601 形式でないなら fail (PR-E2 expiry 判定の境界バグ防止)', () => { + expect(() => + McpOAuthTokenSchema.parse({ + mcpServerId: 'atlassian', + accessToken: 'a-tok', + acquiredAt: 'not-a-date', + }), + ).toThrow(); + }); + + it('expiresAt が ISO 8601 形式でないなら fail', () => { + expect(() => + McpOAuthTokenSchema.parse({ + mcpServerId: 'atlassian', + accessToken: 'a-tok', + acquiredAt: '2026-05-02T10:00:00Z', + expiresAt: '2026/05/02 11:00', + }), + ).toThrow(); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/schema.test.ts` around lines 789 - 835, Add negative tests for McpOAuthTokenSchema to ensure acquiredAt and expiresAt reject invalid datetime strings: call McpOAuthTokenSchema.parse with acquiredAt: 'not-a-date' (and separately with a valid acquiredAt but expiresAt: 'not-a-date') and assert that parse throws; also include a positive check that a valid ISO datetime (e.g., '2026-05-02T10:00:00Z') still parses to guard behavior. These tests will prevent regressing the .datetime() validation described in schema.ts and should reference the existing McpOAuthTokenSchema test suite.packages/core/src/oauth/index.ts (1)
1-1: ⚡ Quick win
OAuthProviderConfigが public API として再エクスポートされていないPR-E2 で実装される
OAuthClient/LoopbackCallbackServerはこのインターフェースを引数型として使う想定ですが、oauth/index.tsおよびpackages/core/src/index.tsに再エクスポートが無いため、下流パッケージが内部パスを直接 import する必要が生じます。♻️ 修正案
-export { ATLASSIAN_CLOUD_OAUTH, OAUTH_REGISTRY, type OAuthKind } from './atlassian'; +export { ATLASSIAN_CLOUD_OAUTH, OAUTH_REGISTRY, type OAuthKind, type OAuthProviderConfig } from './atlassian';さらに
packages/core/src/index.tsの型エクスポート行にも追加:-export { ATLASSIAN_CLOUD_OAUTH, OAUTH_REGISTRY, type OAuthKind } from './oauth'; +export { ATLASSIAN_CLOUD_OAUTH, OAUTH_REGISTRY, type OAuthKind, type OAuthProviderConfig } from './oauth';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/oauth/index.ts` at line 1, OAuthProviderConfig が public API として再エクスポートされていないため下流が内部パスを直接参照してしまう問題があります。oauth のエントリで OAuthProviderConfig をエクスポートするように (現在エクスポートしている ATLASSIAN_CLOUD_OAUTH / OAUTH_REGISTRY / OAuthKind と同じ場所で) 追記し、さらにコアのインデックスの型エクスポート行にも OAuthProviderConfig を追加して public API に含めてください。対象となる識別子: OAuthProviderConfig, ATLASSIAN_CLOUD_OAUTH, OAUTH_REGISTRY, OAuthKind, およびコアの型エクスポートセクション（packages core の index 型エクスポート行）を更新してください。packages/core/src/oauth/atlassian.ts (1)
38-42: ⚡ Quick win
OAuthKindが'atlassian'ではなくstringに解決される
OAUTH_REGISTRYにReadonly<Record<string, OAuthProviderConfig>>という明示的な型アノテーションが付いているため、typeof OAUTH_REGISTRYの key 型はstringに widening されます。その結果OAuthKind = stringとなり、PR-E2 でOAuthClientがkind: OAuthKindを引数に受けても、コンパイル時に無効な kind を検出できなくなります。
Record<string, ...>を使うとkeyof typeofがstringになり型が広がることは既知の挙動です。satisfies演算子はアノテーションとして式の型を変えずに検証するため、以下のように修正するとOAuthKind = 'atlassian'が得られます。♻️ 修正案
-export const OAUTH_REGISTRY: Readonly<Record<string, OAuthProviderConfig>> = { - atlassian: ATLASSIAN_CLOUD_OAUTH, -}; +export const OAUTH_REGISTRY = { + atlassian: ATLASSIAN_CLOUD_OAUTH, +} satisfies Record<string, OAuthProviderConfig>;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/oauth/atlassian.ts` around lines 38 - 42, The OAUTH_REGISTRY declaration is annotated as Readonly<Record<string, OAuthProviderConfig>>, which widens its keys to string so OAuthKind becomes string; remove the explicit broad type annotation and instead keep the object literal and use the TypeScript "satisfies" operator (e.g. have OAUTH_REGISTRY satisfy Readonly<Record<string, OAuthProviderConfig>>) so the property keys stay literal and OAuthKind = keyof typeof OAUTH_REGISTRY resolves to the exact union ('atlassian'); ensure ATLASSIAN_CLOUD_OAUTH still conforms to OAuthProviderConfig so the satisfies check passes.
🤖 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/core/src/schema.ts`:
- Line 312: refreshToken is defined as z.string().optional() allowing empty
strings; update its schema to require non-empty when present by changing the
validator to z.string().min(1).optional() so it mirrors accessToken's
z.string().min(1) behavior; locate the refreshToken symbol in the schema
(packages/core/src/schema.ts) and replace its definition accordingly.
In `@packages/storage/src/clear-project.ts`:
- Around line 23-27: clearProject() currently calls clearDir() which only
removes *.yaml|*.yml and so leaves tmp token files (written as .tmp.<pid> in
oauth-store.ts) behind; update the logic handling paths.oauthDir (in
clear-project.ts) to fully remove and recreate the oauth directory or otherwise
delete all files inside it regardless of extension before writing edges, e.g.
remove paths.oauthDir recursively (fs.rm(..., { recursive:true, force:true }))
and then fs.mkdir(paths.oauthDir, { recursive: true }) so any .tmp.<pid> or
other leftover files are removed; ensure references to clearDir() behavior are
preserved for other dirs or add an option to clearDir() to delete all files for
oauth specifically.
In `@packages/storage/src/oauth-store.test.ts`:
- Around line 115-123: The test currently writes an explicit token with
tokenType: 'Bearer' (via makeToken) so it cannot catch regressions in the schema
default; change the test to create a stored token that omits tokenType (e.g.,
write a YAML token file into the test project directory without a tokenType
field rather than calling makeToken({ tokenType: 'Bearer' })), then instantiate
FileSystemOAuthStore and call read('atlassian') and assert that the returned
token's tokenType equals 'Bearer' (verifying McpOAuthTokenSchema default
behavior).
In `@packages/storage/src/oauth-store.ts`:
- Around line 67-68: The tmp filename `${filePath}.tmp.${process.pid}` can
collide for concurrent writes; modify the write routine (the tmpPath/fd creation
in the write function in oauth-store.ts) to append a per-write unique suffix
(for example use crypto.randomUUID() or Date.now()+a per-process incrementing
counter or crypto.randomBytes) so each tmp file is unique per write; replace the
current tmpPath construction with
`${filePath}.tmp.${process.pid}.${uniqueSuffix}` (and keep the subsequent
fs.open/rename logic unchanged) so concurrent write() calls cannot clobber each
other.
- Around line 41-42: Validate and constrain the mcpServerId inside filePath
before using it to build a filesystem path: reject or sanitize any input
containing path separators or traversal patterns (e.g. '..' or '/'/backslash) by
applying a shared regex/schema (e.g. allow only [A-Za-z0-9._-]) or by
canonicalizing with path.basename, then use the validated value when
constructing the final path (return path.join(this.oauthDir,
`${validatedMcpServerId}.yaml`)); ensure this check lives in this class
(filePath or a small helper) and throw a clear error if validation fails so
callers cannot escape oauthDir or overwrite files outside it.
---
Nitpick comments:
In `@packages/core/src/oauth/atlassian.ts`:
- Around line 38-42: The OAUTH_REGISTRY declaration is annotated as
Readonly<Record<string, OAuthProviderConfig>>, which widens its keys to string
so OAuthKind becomes string; remove the explicit broad type annotation and
instead keep the object literal and use the TypeScript "satisfies" operator
(e.g. have OAUTH_REGISTRY satisfy Readonly<Record<string, OAuthProviderConfig>>)
so the property keys stay literal and OAuthKind = keyof typeof OAUTH_REGISTRY
resolves to the exact union ('atlassian'); ensure ATLASSIAN_CLOUD_OAUTH still
conforms to OAuthProviderConfig so the satisfies check passes.
In `@packages/core/src/oauth/index.ts`:
- Line 1: OAuthProviderConfig が public API
として再エクスポートされていないため下流が内部パスを直接参照してしまう問題があります。oauth のエントリで OAuthProviderConfig
をエクスポートするように (現在エクスポートしている ATLASSIAN_CLOUD_OAUTH / OAUTH_REGISTRY / OAuthKind
と同じ場所で) 追記し、さらにコアのインデックスの型エクスポート行にも OAuthProviderConfig を追加して public API
に含めてください。対象となる識別子: OAuthProviderConfig, ATLASSIAN_CLOUD_OAUTH, OAUTH_REGISTRY,
OAuthKind, およびコアの型エクスポートセクション（packages core の index 型エクスポート行）を更新してください。
In `@packages/core/src/schema.test.ts`:
- Around line 789-835: Add negative tests for McpOAuthTokenSchema to ensure
acquiredAt and expiresAt reject invalid datetime strings: call
McpOAuthTokenSchema.parse with acquiredAt: 'not-a-date' (and separately with a
valid acquiredAt but expiresAt: 'not-a-date') and assert that parse throws; also
include a positive check that a valid ISO datetime (e.g.,
'2026-05-02T10:00:00Z') still parses to guard behavior. These tests will prevent
regressing the .datetime() validation described in schema.ts and should
reference the existing McpOAuthTokenSchema test suite.
🪄 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: 768f0694-86a5-4715-92fa-ac9ecc24a082
⛔ Files ignored due to path filters (1)
.claude/scheduled_tasks.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
docs/adr/0011-tally-managed-oauth-flow.mdpackages/core/src/index.tspackages/core/src/oauth/atlassian.test.tspackages/core/src/oauth/atlassian.tspackages/core/src/oauth/index.tspackages/core/src/schema.test.tspackages/core/src/schema.tspackages/storage/src/clear-project.test.tspackages/storage/src/clear-project.tspackages/storage/src/index.tspackages/storage/src/oauth-store.test.tspackages/storage/src/oauth-store.tspackages/storage/src/project-dir.ts
CodeRabbit (PR #29) 1 周目への対応。すべて Quick win。 [Minor / schema.ts:312] refreshToken に min(1) 制約を追加 provider が誤って空文字を返した場合のサイレント障害を schema で検出する。 [Major / clear-project.ts:27] oauth/ の tmp 残骸を含めて全削除 oauth-store の `*.tmp.<pid>.<uuid>` 中間ファイル (write 中断時) が clearDir の YAML フィルタに引っかからず残留する問題を修正。clearOAuthDir helper を 新設し、ディレクトリ配下を ext 不問で全削除する。removedOAuthTokens は YAML の数だけカウント (tmp は noise なので集計外)。 [Minor / oauth-store.test.ts:123] tokenType default 経路の test 改修 makeToken({ tokenType: 'Bearer' }) を直接 write していたため schema default が壊れても通る test だった。tokenType を欠いた YAML を直接書き、read 時に McpOAuthTokenSchema の default が適用されることを確認する形に変更。 [Major / oauth-store.ts:42] mcpServerId のストレージ境界検証 `path.join(oauthDir, ${mcpServerId}.yaml)` で `../etc/passwd` のような値が 渡ると oauth/ の外を読み書きできる問題があった。filePath() 内で McpServerIdRegex (core から export) を通して validate し、違反なら明示的に throw する自己防衛ロジックを追加。read/write/delete 全経路で path traversal を弾くテストも追加。 [Major / oauth-store.ts:68] tmp ファイル名を per-write で unique 化 `${filePath}.tmp.${process.pid}` だと同 mcpServerId への並行 write で衝突し、 内容が混ざる + 後続 rename が ENOENT で落ちる race があった。crypto.randomUUID() を suffix に付与して 1 書き込みごとに固有名にする。 ## 副次変更 - core: McpServerIdRegex を schema.ts から export し、core/index.ts でも 公開。storage の oauth-store がこれを使って path traversal を validate。 - clear-project.test.ts: tmp 残骸も削除されることを確認する test 追加。 pnpm typecheck 4/4 PASS / pnpm test core 99 + storage 97 + ai-engine 213 + frontend 273 = 682 PASS / pnpm lint exit 0。
This was referenced May 2, 2026
This was referenced May 2, 2026
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 (Tally 側で OAuth 2.1 フローを管理する) の実装段階 1。issue #28 への対応として、外部 MCP server の access token を Tally で保持して SDK の `headers` に inject する設計の土台を作る。
本 PR だけでは OAuth フローは動かない。PR-E2 (OAuthClient + LoopbackCallbackServer) 以降の前提となる schema と storage を切り出した最小単位の PR。
ADR-0011
docs/adr/0011-tally-managed-oauth-flow.mdで設計判断と PR 分割計画 (E1 〜 E5) を記録:oauthフィールドは PR-E1 では optional・PR-E4 で旧auth_request経路の削除と同時に required 化変更内容
packages/core
McpOAuthConfigSchema(新規):clientId必須、scopesoptionalMcpServerConfigSchema.oauth: optional で追加McpOAuthTokenSchema(新規): token store の YAML スキーマ。acquiredAt/expiresAtは ISO 8601 datetime、tokenTypedefault 'Bearer'src/oauth/atlassian.ts(新規): Atlassian Cloud OAuth 2.1 endpoint registry +OAuthProviderConfiginterfacepackages/storage
ProjectPaths.oauthDir:<projectDir>/oauth/を追加FileSystemOAuthStore(新規): read/write/delete/list、TOCTOU フリー write (tmp を 0o600 で open → rename)clearProject:oauth/も削除対象に追加 (removedOAuthTokens追跡)codex セカンドオピニオン対応 (push 直前 review)
clearProjectでoauth/削除acquiredAt/expiresAtをz.iso.datetime()/fs.mkdir二重を解消 / ADR の Confluence scope 例を Jira 系に修正mcpServerIdregex のエラーメッセージ統一 /it.skipIfで Windows skip /OAuthProviderConfig型注釈Test plan
OAuthProviderConfiginterface が使えるか確認 (本 PR 範囲外)Related
Summary by CodeRabbit
リリースノート
New Features
Infrastructure