feat(mcp): Atlassian MCP integration foundation (PR-A: 純 MCP 統合)#19
Merged
Conversation
…set / envVar 名 regex)
…lt []) Atlassian MCP 連携の Project レベル設定を保持するため、 ProjectSchema と ProjectMetaSchema に mcpServers: McpServerConfig[] を追加。 default は [] とし、mcpServers フィールドを持たない既存 YAML も後方互換で読み込める。 宣言順の都合で MCP セクションを ProjectMetaSchema より前に移動。 ProjectMeta / Project 型は z.input 由来に切り替え、書き込み側で mcpServers 省略可とした (output は parse 時に default [] が解決される)。
3987780 で導入した z.input 由来の Project / ProjectMeta / ProjectMetaPatch を z.infer (output 型) に戻す。foundation schema は parse 後の値を表すべきで、 input 緩和は downstream で ?? [] ガードを書く debt を生むため。 これに伴い必要となる既存 fixture 17 ファイルに mcpServers: [] を機械的に追加: - packages/storage/src/init-project.ts (1) - packages/storage/src/project-store.test.ts (5) - packages/ai-engine の test 4 ファイル (9) - packages/frontend の test 11 ファイル (32) 新規 test の追加は無し。既存 546 tests は同数のまま全 pass。
…owedTools wildcard 合成)
… (Task 12) - ChatEvent に chat_tool_external_use / chat_tool_external_result を追加 - extractAssistantBlocks を拡張: text + 外部 MCP tool_use + tool_result を返す - mcp__tally__* は intercept 経路で処理されるため拾わない (重複回避) - chatStore.appendBlockToMessage で source: 'external' で永続化、approval 省略 - console.log は redactMcpSecrets でラップ済み (Task 11 で導入済)
大規模 epic 取り込みで 1 ターン 500KB+ になり得る tool_result の YAML 永続化を 4KB + "(truncated, N chars total)" マーカーに切り詰める。event はフルを流すので UI セッション内では全文展開可、リロード後は truncated 版のみ。
…k 14, T4 fix) multi-turn 対話で AI が前ターンの外部 MCP 出力を覚えるように buildChatPrompt を拡張。 旧版は text block のみ replay していたため、AI が 1 ターン目で @jira EPIC-1 を読んで proposal を生成しても、2 ターン目「続けて子チケット STORY-2 も見て」では Jira の内容を 覚えていない (= AI が summarize した assistant text しか残らない) 問題があった。 各 block を順に replay: - text → そのまま (assistant / user の自然言語) - tool_use → <tool_use id="..." name="..." source="external"?>...input...</tool_use> - tool_result → <tool_result id="..." ok="...">...output...</tool_result> ChatRunner.runUserTurn の prompt 構築タイミングは現状維持 (user message append → contextNodes load → buildChatPrompt → 空 assistant append)。
agent-runner.ts:114 の hardcoded mcpServers / allowedTools を chat-runner と 共通の buildMcpServers utility 経由に変更。プロジェクト設定 mcpServers[] が agent 側でも有効化され、Task 16+ で実装する ingest-jira-epic agent がそのまま 動く土台が整う。 - 既存 5 agent (decompose-to-stories / extract-questions / find-related-code / analyze-impact / ingest-document) の動作不変を 193 既存 test で regression 担保 - 新規 2 test: 外部 MCP 合成 (Bearer auth) / env 未設定で error event - agent 固有 allowedTools (mcp__tally__find_related 等) と外部 MCP wildcard (mcp__<id>__*) を合流、tally の wildcard は dedup - env 未設定 throw は既存の catch (err) で agent_failed error event に流れる
…k 16) - core: ProjectMetaPatchSchema に mcpServers: McpServerConfig[] optional を追加 (.strict() なので明示追加が必要) - frontend: PATCH /api/projects/:id で parsed.data.mcpServers を next に流す - 新規 test 3 件: round-trip / hardening (http://example.com で 400) / 空配列で全消去
- mcpServers[] の追加 / 削除 / 編集 (id / name / url / scheme / envVar) - Bearer (Server/DC) と Basic (Cloud) 切替で emailEnvVar 入力欄が出現 - secret 値はフォームに無し → caption で .env への誘導 - store の patchProjectMeta type に mcpServers? を追加 - 新規 test 4 件: 追加/削除/Basic 切替/secret 欄が無いこと
- store.ts: chat_tool_external_use / chat_tool_external_result の event handler を追加 - 内部 tool_use とは別 handler で source='external' の block を append - external は承認概念なし (approval は付与しない) - tool-approval-card.tsx: source 分岐 - source='external' は <details> 折り畳み + 「外部ソース」ラベル + 承認 / 却下なし - source='internal' は既存通り承認 / 却下 button (退行なし) - 新規 test 2 件: external で承認 button 非表示 + details 折り畳み
|
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 (3)
📝 WalkthroughWalkthroughプロジェクトごとの MCP サーバ設定を実行時に構築して SDK に注入する機能、外部 MCP 用のツール使用追跡（イベント・永続化・切り詰め）、及びノード作成時のプラグイン可能な重複検知ガード群を追加・統合しました。フロントエンドに MCP 設定 UI と表示差分も導入しています。 Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Agent as Agent Runner
participant Store as Project Store
participant Builder as buildMcpServers
participant SDK as SDK Query
participant MCP as MCP Servers
User->>Agent: runAgent()
Agent->>Store: getProjectMeta()
Store-->>Agent: { mcpServers: [...] }
Agent->>Builder: buildMcpServers(tally, configs)
Builder->>Builder: validate env vars / build Authorization
Builder-->>Agent: { mcpServers, allowedTools }
Agent->>SDK: query(prompt, options:{ mcpServers, allowedTools })
SDK->>MCP: execute tool_use
MCP-->>SDK: tool_result
SDK-->>Agent: response + tool events
Agent-->>User: final response / events
sequenceDiagram
actor User
participant Chat as Chat Runner
participant Store as Project Store
participant Builder as buildMcpServers
participant SDK as SDK Stream
participant History as Chat Store
User->>Chat: send message
Chat->>Store: getProjectMeta()
Store-->>Chat: { mcpServers }
Chat->>Builder: buildMcpServers(...)
Builder-->>Chat: { mcpServers, allowedTools }
Chat->>SDK: stream(messages, options)
SDK->>SDK: emit tool_use/tool_result events
alt external tool (non-tally)
SDK->>Chat: chat_tool_external_use / result
Chat->>History: persist blocks with source='external' (truncate large outputs)
Chat-->>User: emit external events
else internal tool (tally)
Chat->>History: persist with source='internal'
end
SDK-->>Chat: final assistant message
Chat-->>User: message + persisted history
sequenceDiagram
actor Client
participant Handler as Create Node Handler
participant GuardMgr as dispatchDuplicateGuard
participant Guard as Specific Guard
participant Store as Project Store
Client->>Handler: createNode(adoptAs, input)
Handler->>GuardMgr: dispatchDuplicateGuard(adoptAs, input, ctx)
GuardMgr->>Guard: check(input, ctx)
alt duplicate found
Guard-->>Handler: { reason }
Handler-->>Client: { ok:false, reason }
else no duplicate
Guard-->>Handler: null
Handler->>Store: addNode(...)
Handler->>GuardMgr: notifyCreated(adoptAs, input, ctx)
Handler-->>Client: { ok:true, nodeId }
end
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)
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 49 minutes.Comment |
旧実装は \`mcpServers.length + 1\` で id を採番していたため、 \`atlassian-1\` / \`atlassian-2\` の状態で 1 件削除して追加すると、 再び \`atlassian-2\` が生成されて key 衝突 → 1 件を黙って上書きしていた。 未使用 suffix を探索する方式に変更。 副次効果として React key も id だけで一意になるため、配列 index を key に使う必要が無くなり biome の \`noArrayIndexKey\` lint error を解消。 - addMcpServer: usedIds set を作り、最小未使用 suffix を採番 - map の key を \`s.id\` のみに変更 (index 削除) - regression test 追加: 削除→追加で id 衝突しない
CodeRabbit 指摘 (PR #18 2nd review): このテストは sourceUrlGuard を 直接呼んでおり、dispatchDuplicateGuard 経由のグローバル registry は 触らないため、beforeEach の reset 呼び出しは不要 + テスト順依存を 作るリスクがあった。
This was referenced Apr 28, 2026
Merged
buildMcpServers が Record<id, ...> にマップするため、重複 id を 登録すると後勝ちで silent override されつつ allowedTools には 両方残るため整合性が崩れる。codebases[] と対称に checkUniqueMcpServerIds を ProjectMetaSchema / ProjectSchema / ProjectMetaPatchSchema の superRefine に組み込む。 codex セカンドオピニオン (PR #19) Major 1 指摘対応。
env 未設定で buildMcpServers が throw した場合、これまでは空 blocks の assistant message が YAML に永続化された後に error event を出して return していた。リロード時には buildChatPrompt が空 message を スキップするため AI への影響はないが、chat 履歴 UI には空のアシスタント バブルが蓄積する問題があった。 assistantMsgId の生成は handler が参照するため先に行い、永続化のみを MCP 構築成功後に遅延させる。 codex セカンドオピニオン (PR #19) Major 2 指摘対応。
tool_result の output は外部 MCP の生出力 (Jira 本文・epic 説明等) で </tool_result> や < を含む可能性があり、prompt の XML 構造を 壊して AI が前ターンの context を誤読する恐れがあった。 escapeXmlText で & < > の最低限のみエスケープ。tool_use.input 側は JSON.stringify が < を \\u003c にエスケープするため対象外。 codex セカンドオピニオン (PR #19) Major 3 指摘対応。
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/frontend/src/lib/store.test.ts (1)
436-489:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
patchProjectMetaでもmcpServersの往復を確認してください。hydrate fixture を更新しただけだと、PATCH 応答や store の map で新フィールドが落ちてもテストが通るままです。
projectMetaの期待値にmcpServersを 1 回入れて、この変更が frontend 側にも届くことを固定したいです。下のcodebases全置換テストにも同じ確認を足すとより安全です。♻️ 例: PATCH 応答と store 状態の両方で確認
okJson({ id: 'proj-1', name: 'P', codebases: [{ id: 'backend', label: 'Backend', path: '../backend' }], + mcpServers: [], createdAt: '2026-04-18T00:00:00Z', updatedAt: '2026-04-19T00:00:00Z', }); @@ expect(useCanvasStore.getState().projectMeta?.codebases).toEqual([ { id: 'backend', label: 'Backend', path: '../backend' }, ]); + expect(useCanvasStore.getState().projectMeta?.mcpServers).toEqual([]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/frontend/src/lib/store.test.ts` around lines 436 - 489, Update the two tests for patchProjectMeta to also assert mcpServers round-trip: in each test where you call useCanvasStore.getState().hydrate(...) and okJson(...) include a non-empty mcpServers field in both the initial hydrate fixture and the PATCH response, and after awaiting useCanvasStore.getState().patchProjectMeta(...) add an expectation that useCanvasStore.getState().projectMeta?.mcpServers equals the returned mcpServers; reference the test helpers and methods patchProjectMeta, hydrate, okJson, and projectMeta to locate where to add the new fixture data and assertions.packages/storage/src/project-store.test.ts (1)
33-47:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
mcpServersも保存・復元できることを明示してください。今の assertions は
nameとcodebasesしか見ていないので、保存処理が新フィールドを落としても気づけません。meta?.mcpServersも 1 回は期待値に入れて、少なくともこの round-trip だけは固定したいです。下の保存ケースにも同じ確認を入れると安心です。♻️ 例: 追加したフィールドを検証
const meta = await store.getProjectMeta(); expect(meta?.name).toBe('テストプロジェクト'); expect(meta?.codebases).toEqual([]); + expect(meta?.mcpServers).toEqual([]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/storage/src/project-store.test.ts` around lines 33 - 47, テストが saveProjectMeta → getProjectMeta の往復で mcpServers を検証しておらず、新フィールドが落ちても気づけないので、FileSystemProjectStore の saveProjectMeta に渡しているオブジェクトの mcpServers を getProjectMeta の戻り値でも検証するように追加してください（テスト内の saveProjectMeta 呼び出しと取得した meta に対して meta?.mcpServers の期待値を assert する）。同様の保存ケース（ファイル内の別の "保存ケース" テスト）にも同じ mcpServers の検証を追加して保護してください。packages/frontend/src/components/dialog/project-settings-dialog.tsx (1)
57-58:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMCP 設定の入力検証が保存前に効いていません
saveDisabledが codebase だけを見ており、MCP のid重複/形式不正、url、env var 名の不正がある状態でも保存できます。結果的に PATCH 後のサーバーエラー待ちになり、修正箇所が分かりづらいです。MCP も事前検証してsaveDisabledに含めるのが安全です。Also applies to: 171-299
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/frontend/src/components/dialog/project-settings-dialog.tsx` around lines 57 - 58, The current saveDisabled only checks codebase-level duplicateIds/invalidIds and busy; extend it to include MCP validation results so saves are blocked when any MCP has duplicate/invalid id, invalid url, or invalid env var names. Run the same validation logic you use for codebase entries against the MCP list (produce e.g. mcpDuplicateIds, mcpInvalidIds, mcpInvalidUrls, mcpInvalidEnvNames) and include those booleans in the saveDisabled expression (alongside busy, duplicateIds, invalidIds). Also mirror this change in the other save block that appears around the 171-299 region so both save controls consistently prevent PATCH when MCP validation errors exist.
🧹 Nitpick comments (4)
packages/storage/src/chat-store.test.ts (1)
63-79: ⚡ Quick win
sourceの省略ケースも 1 つ残してください。
tool_useを常にsource: 'internal'付きで固定すると、既存 YAML の後方互換として重要な「source無し →internalに復元される」経路をこのテストで検証できません。Also applies to: 155-172, 233-246
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/storage/src/chat-store.test.ts` around lines 63 - 79, The test is overwriting all tool_use fixtures to include source: 'internal', which removes the case that verifies restoring a missing source to 'internal'; update the test data in chat-store.test.ts so at least one tool_use object (the one passed to store.updateMessageBlock and the original message block fixture) omits the source field to exercise the "no source → restored to 'internal'" migration path, and make the same change in the other two test locations referenced (the blocks around the ranges you noted) so each set contains one tool_use without a source while leaving other tool_use entries as-is.packages/ai-engine/src/chat-runner.ts (1)
69-755: 🏗️ Heavy lift
chat-runner.tsは 500 行制約を超えているため分割を推奨しますMCP 構築、イベント抽出、承認フロー、prompt 組み立てを別モジュールに切り出すと、今後の不具合混入とレビュー負荷を下げられます。
As per coding guidelines "Keep individual files under 500 lines; split larger files into multiple modules".
🤖 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 69 - 755, The file exceeds the 500-line guideline; split large responsibilities into modules: extract the MCP server builder (buildMcpServer and its inner makeHandler/find helper) into a new mcp-server module that exports a function createTallyMcpServer(tools, emit, assistantMsgId) and import it in ChatRunner; move prompt/format helpers (buildChatPrompt, buildChatSystemPrompt, formatNodeForContext, loadContextNodes, escapeXmlText) into a prompt-utils module and export them for use in runUserTurn; move assistant block parsing (extractAssistantBlocks) into a sdk-parser module; and move tool invocation/approval logic (invokeInterceptedTool and buildToolRegistry) into a tool-runner module that exports the functions or a class and keep ChatRunner wiring minimal by importing these; ensure exported symbols keep the same names used by ChatRunner (or update references), preserve access needed by tests (export as public), and update imports/exports and unit tests accordingly so behavior is unchanged.packages/ai-engine/src/chat-runner.test.ts (1)
479-1187: 🏗️ Heavy liftテストファイルが肥大化しているため、シナリオ単位で分割してください
この追加で
packages/ai-engine/src/chat-runner.test.tsが 500 行を大きく超えています。buildMcpServers 統合/外部MCP永続化/buildChatPrompt replayなどの describe 単位でファイル分割する方が保守しやすいです。As per coding guidelines "Keep individual files under 500 lines; split larger files into multiple modules".
🤖 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 479 - 1187, The test file is too large; split the big describe suites into separate test files by moving each top-level describe block into its own file (e.g., the "ChatRunner — buildMcpServers 統合 (Task 11)" suite, the "ChatRunner — 外部 MCP tool_use/tool_result 永続化 (Task 12)" suite, and the "buildChatPrompt — tool_use/tool_result replay (Task 14, T4 fix)" suite), keeping any shared setup/teardown helpers (like creation of FileSystemProjectStore/FileSystemChatStore, mkdtempSync/ rmSync cleanup, and the buildChatPrompt references) in a new test-utils module that each new test imports; ensure each new file preserves the same test names and imports SdkLike, ChatRunner, buildChatPrompt, and types so tests run unchanged.packages/ai-engine/src/duplicate-guards/index.ts (1)
1-3: import をファイル先頭に集約し、グループ順を規約どおりに揃えてください相対 import が実行コードの後ろ（Line 75-77）にあり、import order 規約から外れています。先頭で
internal package (@tally/*) → relative (./)の順に統一し、グループ間に空行を配置してください。修正案（import を先頭へ移動）
import type { AdoptableType } from '@tally/core'; import type { ProjectStore } from '@tally/storage'; +import { coderefGuard } from './coderef'; +import { questionGuard } from './question'; +import { sourceUrlGuard } from './source-url'; + // create-node 入力のうち guard に必要な最小 shape。 export interface GuardInput { title: string; body: string; additional: Record<string, unknown> | undefined; } // guard が共有するランタイム文脈。 export interface DuplicateGuardContext { store: ProjectStore; // anchor 無し (chat) のときは空文字。anchor 依存 guard は空文字を skip せよ。 anchorId: string; // セッション内で生成済みノードの重複記録。キーは guard 実装が決める。 sessionMemo: Set<string>; // マルチコードベース対応のために流すコードベース ID (optional)。 codebaseId?: string; } export interface DuplicateFound { reason: string; // ユーザー向けメッセージ (既存 node id などを含む) } export interface DuplicateGuard { // 対象 adoptAs。複数対応は同 guard を複数 adoptAs で登録する。 adoptAs: AdoptableType; // 重複があれば DuplicateFound、無ければ null。 check(input: GuardInput, ctx: DuplicateGuardContext): Promise<DuplicateFound | null>; // 生成成功後に呼ばれる (sessionMemo 更新など)。任意。 onCreated?(input: GuardInput, ctx: DuplicateGuardContext): void; } // adoptAs → Guard[] のレジストリ。Task 7-9 で個別 guard を追加する。 const REGISTRY = new Map<AdoptableType, DuplicateGuard[]>(); export function registerGuard(guard: DuplicateGuard): void { const list = REGISTRY.get(guard.adoptAs) ?? []; list.push(guard); REGISTRY.set(guard.adoptAs, list); } // dispatcher: 登録 guard を順に check し、最初に重複を見つけたら返す。 // 全部 null なら null。Promise を一つずつ await する (並列にしない: 副作用順序を保つ)。 export async function dispatchDuplicateGuard( adoptAs: AdoptableType, input: GuardInput, ctx: DuplicateGuardContext, ): Promise<DuplicateFound | null> { const guards = REGISTRY.get(adoptAs) ?? []; for (const g of guards) { const found = await g.check(input, ctx); if (found) return found; } return null; } // 生成成功通知: 登録 guard の onCreated を全部呼ぶ。 export function notifyCreated( adoptAs: AdoptableType, input: GuardInput, ctx: DuplicateGuardContext, ): void { const guards = REGISTRY.get(adoptAs) ?? []; for (const g of guards) g.onCreated?.(input, ctx); } // テスト用: REGISTRY をクリア。プロダクションコードからは呼ばないこと。 // 命名 prefix で「test-only」を明示し、accidental 使用を防ぐ。 export function __resetGuardsForTest(): void { REGISTRY.clear(); } - -import { coderefGuard } from './coderef'; -import { questionGuard } from './question'; -import { sourceUrlGuard } from './source-url'; // 個別 guard を register する (module load 時の副作用)。 // テストは __resetGuardsForTest でクリアした後、必要な guard を再登録すること。 registerGuard(coderefGuard); registerGuard(questionGuard); registerGuard(sourceUrlGuard);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ai-engine/src/duplicate-guards/index.ts` around lines 1 - 3, ファイル先頭に import を集約し、グループ順を「内部パッケージ(`@tally/`*) → 相対(./)」の規約どおりに揃えてください：現在先頭にある型 import (AdoptableType, ProjectStore) の直後に空行を入れ、その後で相対 import（現在ファイル内にある ./... の import）を先頭に移動して配置し、実行コード（関数定義や変数代入）が import の前に来ないようにしてください。グループ間は空行で区切り、既存の import 文は重複や未使用がないか確認して整理してください。
🤖 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/chat-runner.ts`:
- Around line 659-665: The tool_use XML injection risk: wrap the input payload
with the XML text-escaping helper and ensure attributes are escaped; replace the
unescaped JSON.stringify(b.input) inside the element body with
escapeXmlText(JSON.stringify(b.input)) and use an escapeXmlAttr utility on
attribute values (e.g., b.name and any sourceAttr construction) to prevent `<`,
`&`, `>` or `</tool_use>` from breaking the XML; add or reuse escapeXmlText and
implement escapeXmlAttr functions and apply them where attributes are
interpolated and where element text content is rendered.
In `@packages/core/src/schema.ts`:
- Around line 267-290: The URL validator (the z.string().url().refine(...) for
the url field) currently allows userinfo like https://user:pass@host; update the
refine predicate to reject any URL with embedded credentials by parsing new
URL(u) and returning false if parsed.username or parsed.password are non-empty
(i.e., ensure parsed.username === '' && parsed.password === ''); keep existing
protocol and localhost checks and preserve the error message (or adjust to
mention credentials) so MCP URLs with userinfo are disallowed.
In `@packages/frontend/src/app/api/projects/`[id]/route.test.ts:
- Around line 73-96: The test "mcpServers[] を全置換 (Task 16)" assumes existing
mcpServers but currently runs against an empty project so a merge-append
implementation would still pass; before calling PATCH (the PATCH/Request call
using projectId) insert an initial mcpServers entry for that projectId (via the
existing setup/fixture or API used elsewhere in route.test.ts) containing a
different id (e.g., "old"), then run the PATCH and assert the resulting
body.mcpServers has length 1, its only id is "atlassian", and that the original
"old" id is not present to ensure true replacement rather than merge.
In `@packages/frontend/src/components/chat/tool-approval-card.test.tsx`:
- Around line 88-104: The test currently only asserts a <details> element exists
for ToolApprovalCard when source is 'external' (in the test block using
useCanvasStore and ToolApprovalCard), but must also verify the input preview is
rendered inside that <details>; update the test to locate the <pre> (input
preview) and assert it is a descendant of the found details element (e.g., get
the details element from container.querySelector and confirm
details.contains(pre) or details.querySelector('pre') is not null) so the input
isn't rendered outside the collapsed details.
In `@packages/frontend/src/components/dialog/project-settings-dialog.test.tsx`:
- Around line 181-190: The test for ProjectSettingsDialog currently checks for
absence of secret inputs without adding an MCP server row, so it doesn't
validate the real input rendering; update the test to first interact with the
dialog to add an MCP server row (simulate clicking the "+ MCP サーバーを追加" control),
then assert that the env var input (tokenEnvVar) exists while any raw secret
input fields (labels matching PAT, シークレット, api_token, password) are absent, and
still assert the ".env" guidance text; use the existing
render(<ProjectSettingsDialog ... />) and screen utilities (e.g.
queryByLabelText/getByText) to locate elements and the add-server button before
making the assertions.
In `@packages/frontend/src/components/dialog/project-settings-dialog.tsx`:
- Around line 184-187: The list currently uses the editable s.id as React key
(mcpServers.map(... key={s.id})), which causes remounts and breakage on edits or
duplicates; instead add and persist a stable, non-editable unique identifier
(e.g., uid or uuid) when creating entries in addMcpServer and use that uid as
the React key in project-settings-dialog.tsx (replace key={s.id} with
key={s.uid}); ensure the uid is generated once on creation, not exposed to the
editable form, and update any types/constructors that build mcpServers so the
uid is always present.
---
Outside diff comments:
In `@packages/frontend/src/components/dialog/project-settings-dialog.tsx`:
- Around line 57-58: The current saveDisabled only checks codebase-level
duplicateIds/invalidIds and busy; extend it to include MCP validation results so
saves are blocked when any MCP has duplicate/invalid id, invalid url, or invalid
env var names. Run the same validation logic you use for codebase entries
against the MCP list (produce e.g. mcpDuplicateIds, mcpInvalidIds,
mcpInvalidUrls, mcpInvalidEnvNames) and include those booleans in the
saveDisabled expression (alongside busy, duplicateIds, invalidIds). Also mirror
this change in the other save block that appears around the 171-299 region so
both save controls consistently prevent PATCH when MCP validation errors exist.
In `@packages/frontend/src/lib/store.test.ts`:
- Around line 436-489: Update the two tests for patchProjectMeta to also assert
mcpServers round-trip: in each test where you call
useCanvasStore.getState().hydrate(...) and okJson(...) include a non-empty
mcpServers field in both the initial hydrate fixture and the PATCH response, and
after awaiting useCanvasStore.getState().patchProjectMeta(...) add an
expectation that useCanvasStore.getState().projectMeta?.mcpServers equals the
returned mcpServers; reference the test helpers and methods patchProjectMeta,
hydrate, okJson, and projectMeta to locate where to add the new fixture data and
assertions.
In `@packages/storage/src/project-store.test.ts`:
- Around line 33-47: テストが saveProjectMeta → getProjectMeta の往復で mcpServers
を検証しておらず、新フィールドが落ちても気づけないので、FileSystemProjectStore の saveProjectMeta
に渡しているオブジェクトの mcpServers を getProjectMeta の戻り値でも検証するように追加してください（テスト内の
saveProjectMeta 呼び出しと取得した meta に対して meta?.mcpServers の期待値を assert
する）。同様の保存ケース（ファイル内の別の "保存ケース" テスト）にも同じ mcpServers の検証を追加して保護してください。
---
Nitpick comments:
In `@packages/ai-engine/src/chat-runner.test.ts`:
- Around line 479-1187: The test file is too large; split the big describe
suites into separate test files by moving each top-level describe block into its
own file (e.g., the "ChatRunner — buildMcpServers 統合 (Task 11)" suite, the
"ChatRunner — 外部 MCP tool_use/tool_result 永続化 (Task 12)" suite, and the
"buildChatPrompt — tool_use/tool_result replay (Task 14, T4 fix)" suite),
keeping any shared setup/teardown helpers (like creation of
FileSystemProjectStore/FileSystemChatStore, mkdtempSync/ rmSync cleanup, and the
buildChatPrompt references) in a new test-utils module that each new test
imports; ensure each new file preserves the same test names and imports SdkLike,
ChatRunner, buildChatPrompt, and types so tests run unchanged.
In `@packages/ai-engine/src/chat-runner.ts`:
- Around line 69-755: The file exceeds the 500-line guideline; split large
responsibilities into modules: extract the MCP server builder (buildMcpServer
and its inner makeHandler/find helper) into a new mcp-server module that exports
a function createTallyMcpServer(tools, emit, assistantMsgId) and import it in
ChatRunner; move prompt/format helpers (buildChatPrompt, buildChatSystemPrompt,
formatNodeForContext, loadContextNodes, escapeXmlText) into a prompt-utils
module and export them for use in runUserTurn; move assistant block parsing
(extractAssistantBlocks) into a sdk-parser module; and move tool
invocation/approval logic (invokeInterceptedTool and buildToolRegistry) into a
tool-runner module that exports the functions or a class and keep ChatRunner
wiring minimal by importing these; ensure exported symbols keep the same names
used by ChatRunner (or update references), preserve access needed by tests
(export as public), and update imports/exports and unit tests accordingly so
behavior is unchanged.
In `@packages/ai-engine/src/duplicate-guards/index.ts`:
- Around line 1-3: ファイル先頭に import を集約し、グループ順を「内部パッケージ(`@tally/`*) →
相対(./)」の規約どおりに揃えてください：現在先頭にある型 import (AdoptableType, ProjectStore)
の直後に空行を入れ、その後で相対 import（現在ファイル内にある ./... の import）を先頭に移動して配置し、実行コード（関数定義や変数代入）が
import の前に来ないようにしてください。グループ間は空行で区切り、既存の import 文は重複や未使用がないか確認して整理してください。
In `@packages/storage/src/chat-store.test.ts`:
- Around line 63-79: The test is overwriting all tool_use fixtures to include
source: 'internal', which removes the case that verifies restoring a missing
source to 'internal'; update the test data in chat-store.test.ts so at least one
tool_use object (the one passed to store.updateMessageBlock and the original
message block fixture) omits the source field to exercise the "no source →
restored to 'internal'" migration path, and make the same change in the other
two test locations referenced (the blocks around the ranges you noted) so each
set contains one tool_use without a source while leaving other tool_use entries
as-is.
🪄 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: d5a64237-9d30-4772-82ef-cca8319a55e5
📒 Files selected for processing (44)
packages/ai-engine/src/agent-runner.test.tspackages/ai-engine/src/agent-runner.tspackages/ai-engine/src/agents/find-related-code.test.tspackages/ai-engine/src/chat-runner.test.tspackages/ai-engine/src/chat-runner.tspackages/ai-engine/src/duplicate-guards/coderef.test.tspackages/ai-engine/src/duplicate-guards/coderef.tspackages/ai-engine/src/duplicate-guards/index.test.tspackages/ai-engine/src/duplicate-guards/index.tspackages/ai-engine/src/duplicate-guards/question.test.tspackages/ai-engine/src/duplicate-guards/question.tspackages/ai-engine/src/duplicate-guards/source-url.test.tspackages/ai-engine/src/duplicate-guards/source-url.tspackages/ai-engine/src/mcp/build-mcp-servers.test.tspackages/ai-engine/src/mcp/build-mcp-servers.tspackages/ai-engine/src/mcp/redact.test.tspackages/ai-engine/src/mcp/redact.tspackages/ai-engine/src/server.test.tspackages/ai-engine/src/stream.tspackages/ai-engine/src/tools/create-node.test.tspackages/ai-engine/src/tools/create-node.tspackages/core/src/schema.test.tspackages/core/src/schema.tspackages/core/src/types.tspackages/frontend/src/app/api/projects/[id]/chats/chats-route.test.tspackages/frontend/src/app/api/projects/[id]/edges/edges-route.test.tspackages/frontend/src/app/api/projects/[id]/nodes/[nodeId]/adopt/adopt-route.test.tspackages/frontend/src/app/api/projects/[id]/nodes/nodes-route.test.tspackages/frontend/src/app/api/projects/[id]/route.test.tspackages/frontend/src/app/api/projects/[id]/route.tspackages/frontend/src/components/ai-actions/analyze-impact-button.test.tsxpackages/frontend/src/components/ai-actions/codebase-agent-button.test.tsxpackages/frontend/src/components/ai-actions/extract-questions-button.test.tsxpackages/frontend/src/components/ai-actions/find-related-code-button.test.tsxpackages/frontend/src/components/chat/tool-approval-card.test.tsxpackages/frontend/src/components/chat/tool-approval-card.tsxpackages/frontend/src/components/details/usecase-detail.test.tsxpackages/frontend/src/components/dialog/project-settings-dialog.test.tsxpackages/frontend/src/components/dialog/project-settings-dialog.tsxpackages/frontend/src/lib/store.test.tspackages/frontend/src/lib/store.tspackages/storage/src/chat-store.test.tspackages/storage/src/init-project.tspackages/storage/src/project-store.test.ts
前 commit で追加した escapeXmlText は tool_result.output のみを対象として おり「JSON.stringify が < を \\u003c にエスケープするので tool_use 側は 安全」とコメントしていたが、これは事実誤認だった。JSON.stringify が エスケープするのは " \\ と control chars のみで、< > & は素通しする。 このため input オブジェクトに </tool_use> や < が含まれると prompt の XML 構造が壊れて AI が前ターンの context を誤読する恐れがあった。 - escapeXmlAttr (& " < > エスケープ) を追加 - tool_use の input は escapeXmlText(JSON.stringify(...)) でエスケープ - 属性値 (toolUseId / name / role) は escapeXmlAttr でラップ - コメントの誤りを訂正 CodeRabbit (PR #19) Major 1 指摘対応。
これまでの url validator は \`https://user:pass@host/mcp\` を許容していた。 URL 内資格情報はログ・プロキシ・ブラウザ履歴に漏洩しやすく、本実装が Authorization header 経由で PAT を送る設計とも不整合。 parsed.username / parsed.password が非空なら reject する。 CodeRabbit (PR #19) Major 2 指摘対応 (Quick win)。
React key として s.id を使っていたが、id はフォームから編集可能で あり、編集中に key が変わると React が要素を再マウントしてしまい 入力フォーカスや state がリセットされるリスクがあった。重複入力時 には reconciliation の混乱で間違った DOM ノードが再利用される 可能性もある。 UI ローカルの不変 ID として _uid をマウント時に 1 度だけ割り当て、 React key には _uid を使う。永続化前 (onSave) に _uid を剥がす。 CodeRabbit (PR #19) Major 3 指摘対応。
CodeRabbit (PR #19) Minor 3 件 (Quick win) 対応: - route.test.ts: mcpServers 全置換テストに事前データを投入し、 「マージ追記」では通らないようにする。 - tool-approval-card.test.tsx: <details> の存在だけでなく、その内側に input preview の <pre> が含まれていることまで検証する。 - project-settings-dialog.test.tsx: secret UI 検証を実 MCP 行に対する ものに変更 (空 list だと退行検知が効かないため、+ MCP サーバーを追加 をクリックしてから tokenEnvVar 存在 + secret 入力欄非存在を確認)。
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
packages/frontend/src/components/dialog/project-settings-dialog.test.tsx (2)
111-125: ⚡ Quick win
_uid除去の退行検知をテストで固定してください。
patchProjectMetaの呼び出し検証が shape のみなので、UI ローカルキー_uidが混入しても通ってしまいます。保存時クレンジングの回帰を防ぐため、_uid不在も明示的に確認したほうが安全です。差分案
await waitFor(() => expect(patchProjectMeta).toHaveBeenCalledWith( expect.objectContaining({ mcpServers: [ expect.objectContaining({ id: 'atlassian-1', kind: 'atlassian', url: 'https://x.test/mcp', auth: expect.objectContaining({ type: 'pat', scheme: 'bearer', tokenEnvVar: 'JIRA_PAT', }), }), ], }), ), ); + const lastArg = patchProjectMeta.mock.calls.at(-1)?.[0] as { + mcpServers: Array<Record<string, unknown>>; + }; + expect(lastArg.mcpServers[0]).not.toHaveProperty('_uid');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/frontend/src/components/dialog/project-settings-dialog.test.tsx` around lines 111 - 125, The test currently only verifies shape via patchProjectMeta and can miss regressions where a UI-local key `_uid` is preserved; update the assertion for patchProjectMeta so the mcpServers entry explicitly does not include `_uid` (e.g., include expect.not.objectContaining({ _uid: expect.anything() }) for the server object or assert the called arg's mcpServers array items equal an object that omits `_uid`), targeting the existing patchProjectMeta and mcpServers checks to ensure saved data is cleansed of `_uid`.
147-151: ⚡ Quick win削除ボタンの取得が位置依存で壊れやすいです。
getAllByRole('button', { name: /削除/ })の末尾/末尾-1 をクリックする方式は、将来ボタンが増えると誤クリックになります。MCP 行に限定したセレクタ（行スコープ or 専用 aria-label）へ寄せるのがおすすめです。Also applies to: 166-168
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/frontend/src/components/dialog/project-settings-dialog.test.tsx` around lines 147 - 151, The test is brittle because it selects all "削除" buttons and clicks by index (removeButtons + getAllByRole + userEvent.click), so change the selector to target the MCP row specifically: locate the MCP section container (e.g., the MCP row element or an element with an MCP-specific aria-label), then use within(container).getByRole('button', { name: /削除/ }) or getByRole with a more specific accessible name to click only the MCP delete button; update both occurrences that use getAllByRole and index-based clicks.packages/ai-engine/src/chat-runner.ts (1)
69-770: 🏗️ Heavy lift
chat-runner.tsが 500 行を大きく超えており、責務分割した方が保守しやすいです
runUserTurn、prompt エンコード、SDK block 抽出、キュー制御をモジュール分割することを推奨します。As per coding guidelines, "Keep individual files under 500 lines; split larger files into multiple modules".
🤖 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 69 - 770, The file is too large and should be split by responsibility: extract helper logic (buildChatPrompt, formatNodeForContext, escapeXmlText/Attr, loadContextNodes, extractAssistantBlocks, truncateForPersistence if present) into a prompt/util module; move EventQueue into a small async-queue module; move MCP/server-related builders (buildMcpServer, buildToolRegistry, and makeHandler logic) into an mcp module; keep ChatRunner class (and public methods runUserTurn, invokeInterceptedTool) as the orchestration layer that imports these helpers. Update references inside ChatRunner to import the new modules, preserve the existing function/class names and signatures (ChatRunner, runUserTurn, invokeInterceptedTool, buildChatPrompt, extractAssistantBlocks, EventQueue, buildMcpServer, buildToolRegistry) so callers/tests remain unchanged, and ensure exports/typing are adjusted so compilation and tests pass.
🤖 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/chat-runner.ts`:
- Around line 666-667: The raw message bodies are being inserted into prompts
unescaped (e.g., pushing b.text into lines), which breaks XML-style prompt
structure when texts contain '<' or '&'; update the code paths that build
<message> and <current_user_message> (where lines.push(b.text) is used) to
XML-escape the text before pushing, reusing the same escape helper used for
tool_use/tool_result (or implement an escapeXml helper) so all inserted
user/tool text is safely escaped.
- Around line 212-243: The tool_result branch currently persists and emits every
result as external without verifying origin; add a guard that only treats
tool_result as external if its toolUseId was previously recorded as external
when handling the tool_use event. Concretely, introduce a scoped collection
(e.g., externalToolUseIds) updated in the tool_use branch (where
chatStore.appendBlockToMessage and queue.push for 'chat_tool_external_use' are
invoked) to add b.toolUseId, then in the tool_result branch check
externalToolUseIds.has(b.toolUseId) before appending/pushing the external
payload (and otherwise handle it as internal or skip external persistence); also
remove/cleanup the id from the set when the tool lifecycle completes to avoid
growth. Ensure references to chatStore.appendBlockToMessage,
queue.push('chat_tool_external_result'), truncateForPersistence, and b.toolUseId
are used to locate the changes.
In `@packages/frontend/src/app/api/projects/`[id]/route.test.ts:
- Around line 145-162: このテストは事前登録の seed PATCH
呼び出しの結果を検証しておらず前提が崩れる可能性があるため、packages/frontend/src/app/api/projects/[id]/route.test.ts
内の先頭で行っている await PATCH(...)（seed PATCH、params に Promise.resolve({ id: projectId
}) を渡している呼び出し）のレスポンスの status または返却 JSON を必ずアサートするように追加してください。具体的には PATCH
呼び出し後にレスポンスの status が期待値（例: 200/204）であること、または返却 body の status フィールドや mcpServers
の状態を検証する assertion を入れ、以降の「登録済みを空配列で消す」シナリオの前提が満たされていることを保証してください。
---
Nitpick comments:
In `@packages/ai-engine/src/chat-runner.ts`:
- Around line 69-770: The file is too large and should be split by
responsibility: extract helper logic (buildChatPrompt, formatNodeForContext,
escapeXmlText/Attr, loadContextNodes, extractAssistantBlocks,
truncateForPersistence if present) into a prompt/util module; move EventQueue
into a small async-queue module; move MCP/server-related builders
(buildMcpServer, buildToolRegistry, and makeHandler logic) into an mcp module;
keep ChatRunner class (and public methods runUserTurn, invokeInterceptedTool) as
the orchestration layer that imports these helpers. Update references inside
ChatRunner to import the new modules, preserve the existing function/class names
and signatures (ChatRunner, runUserTurn, invokeInterceptedTool, buildChatPrompt,
extractAssistantBlocks, EventQueue, buildMcpServer, buildToolRegistry) so
callers/tests remain unchanged, and ensure exports/typing are adjusted so
compilation and tests pass.
In `@packages/frontend/src/components/dialog/project-settings-dialog.test.tsx`:
- Around line 111-125: The test currently only verifies shape via
patchProjectMeta and can miss regressions where a UI-local key `_uid` is
preserved; update the assertion for patchProjectMeta so the mcpServers entry
explicitly does not include `_uid` (e.g., include expect.not.objectContaining({
_uid: expect.anything() }) for the server object or assert the called arg's
mcpServers array items equal an object that omits `_uid`), targeting the
existing patchProjectMeta and mcpServers checks to ensure saved data is cleansed
of `_uid`.
- Around line 147-151: The test is brittle because it selects all "削除" buttons
and clicks by index (removeButtons + getAllByRole + userEvent.click), so change
the selector to target the MCP row specifically: locate the MCP section
container (e.g., the MCP row element or an element with an MCP-specific
aria-label), then use within(container).getByRole('button', { name: /削除/ }) or
getByRole with a more specific accessible name to click only the MCP delete
button; update both occurrences that use getAllByRole and index-based clicks.
🪄 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: b835bae9-b38c-42ef-9d3c-ad77c0de27cb
📒 Files selected for processing (6)
packages/ai-engine/src/chat-runner.tspackages/core/src/schema.tspackages/frontend/src/app/api/projects/[id]/route.test.tspackages/frontend/src/components/chat/tool-approval-card.test.tsxpackages/frontend/src/components/dialog/project-settings-dialog.test.tsxpackages/frontend/src/components/dialog/project-settings-dialog.tsx
✅ Files skipped from review due to trivial changes (1)
- packages/core/src/schema.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/frontend/src/components/dialog/project-settings-dialog.tsx
CodeRabbit (PR #19) 2 周目 Major 2 件対応: [Major 1] tool_result を無条件に external として永続化していたため、SDK 仕様 変更や edge case で内部 (mcp__tally__*) の tool_result が SDK ストリームに 流れた場合に誤分類されるリスクがあった。同 turn 内で観測した外部 tool_use の toolUseId を Set に保持し、対応する tool_result のみ external として扱う。 集合に無い toolUseId (内部 / intercept 経路 / 想定外) は無視する。 [Major 2] buildChatPrompt で <message> 内の text 本文と <current_user_message> の本文が未エスケープだった。前 commit で tool_use/tool_result はエスケープ したが、自由入力の text block 自体は <message>/</message> や < & が混入し うる。escapeXmlText でラップ。 test: 既存の Task 13 テスト 2 件 (4KB 超 truncate / 4KB 以下 not truncate) は tool_result のみ yield していたが、Major 1 ガード後は tool_use 先行が 必須なので mock を整備 (tool_use → tool_result の順)。
CodeRabbit (PR #19) 2 周目 Minor 1 件対応 (Quick win)。 事前登録ステップが silent fail すると後段の「空配列で消す」前提が崩れて 偽の成功で通る恐れがあったため、seed PATCH の status を 200 で assert する。
shomatan
added a commit
that referenced
this pull request
May 1, 2026
) * refactor: MCP 認証を OAuth 2.1 / SDK 任せに切替、Tally から PAT/API key を完全排除 Premise 9 撤回: PAT only → MCP プロトコルの OAuth 2.1 採用 (user sovereignty)。 理由: - Atlassian 公式 Rovo MCP は OAuth 2.1 ネイティブ、Claude Agent SDK は MCP の 'needs-auth' status を扱う built-in support を持つ → SDK 任せで auto auth flow が回る - sooperset/mcp-atlassian を使う場合も PAT は MCP server 自身の env で持つ前提に - Tally プロセスから PAT/API key の概念が完全消滅 (project.yaml / メモリ / ログのいずれにも残らない) 実装変更: - core: McpServerConfigSchema から auth フィールド削除、関連 hardening (envVar regex / Basic+Bearer 分岐) も削除。schema test を auth-less に整理 - ai-engine: buildMcpServers の auth header 構築ロジック削除、url のみで HTTP config (req は SDK が必要に応じて WWW-Authenticate を解釈)。chat-runner / agent-runner も auth に依存しない。env 未設定 throw test は仕様変更のため削除 - frontend: 設定 dialog から auth scheme dropdown / envVar 入力欄を削除、URL のみのシンプル UI、caption も「OAuth 2.1 / API token は MCP 任せ」に書き換え * feat(chat): OAuth 2.1 認証フローを auth_request ブロックで 1 等地化 外部 MCP の OAuth 認証フロー (authenticate / complete_authentication tool_use) を assistant 文中の生 tool_use ではなく、専用カードで扱う。「Atlassian で認証」 ボタン (新規タブ) と callback URL paste 入力欄を統合。 - packages/core/src/schema.ts: ChatBlock に \`auth_request\` variant を追加 (mcpServerId / mcpServerLabel / authUrl / status / failureMessage)。 status と failureMessage の整合を superRefine で固定: failed なのに message 無し / pending・completed に message が付いているケースを reject。 - packages/ai-engine/src/auth-detector.ts (新規): \`mcp__<id>__authenticate\` / \`complete_authentication\` パターンの分解、tool_result.output からの auth URL 抽出。文末の句読点・括弧を URL に含めない処理 (`...state=xyz.` のような 自然文出力で認可リンクが壊れるのを防ぐ)。 - packages/ai-engine/src/stream.ts: \`chat_auth_request\` event を ChatEvent に追加。 - packages/ai-engine/src/chat-runner.ts: tool_use を stash → tool_result とペアに なった瞬間に auth_request ブロックへ変換 (raw な tool_use/tool_result は出さない)。 \`handleAuthToolResult\` / \`findLatestPendingAuthRequest\` を新設。 complete_authentication 受領時は同 mcpServerId の最新 pending を更新。 - packages/frontend/src/components/chat/auth-request-card.tsx (新規): 認証ボタン (新規タブ) と callback URL paste 入力欄を 1 カードに集約。 callback URL は localhost / 127.0.0.1 のみ受け付ける軽い形式チェック付き。 - packages/frontend/src/components/chat/chat-message.tsx: auth_request → AuthRequestCard dispatch。 - packages/frontend/src/lib/store.ts: \`chat_auth_request\` event 反映 (pending append / completed・failed in-place 更新)。 - core/schema.test.ts: auth_request バリエーション 6 件 (基本 + 状態遷移 reject) - ai-engine/auth-detector.test.ts: parseAuthToolName / extractAuthUrl 計 13 件 (末尾句読点 strip 含む) - ai-engine/chat-runner.test.ts: auth_request 変換フロー 3 件 (authenticate / complete success / complete failure) - frontend/auth-request-card.test.tsx: pending/completed/failed 描画と paste 検証 7 件 注: ChatRunner は per-turn sdk.query() のままで、long-lived Query 化は別 PR (PR-C) で扱う。本 PR の auth_request UX 単体では「auth 1 回 → 同 thread の 次 turn で再 auth が必要」(SDK subprocess が turn ごとに再生成されるため OAuth state が引き継がれない) という挙動を残す。 * test(frontend): mcpServers PATCH の事前登録を assert 「mcpServers を空配列で全消去できる」テストの事前 PATCH の status を assert していなかった。失敗していると後続の「空配列で削除」ケース が偽の成功 (空 → 空) で通る恐れがあったため修正。 * fix(frontend): chat_auth_request reducer に pending 不在 failed の append 分岐 これまで failed/completed イベントは「同 mcpServerId の pending ブロック を更新」する経路しか持たず、URL 抽出失敗等で pending を append する 前に failed が確定するケースで in-memory state に反映されなかった (YAML には書かれるがリロードまで UI に出ない → 「何も起きない」と見える)。 evt.status==='failed' で同 mcpServerId の pending が見つからない場合、 pending と同様に該当 messageId に新規 append する。completed で pending 不在は異常系のため無視 (現状維持)。 codex セカンドオピニオン (PR #21) Major M1 指摘対応。 * fix(ai-engine): complete_authentication 専用 turn の空 assistant をプレースホルダで埋める callback URL paste 後の turn では SDK 応答が complete_authentication tool_use/tool_result のみで、handleAuthToolResult は過去 message の pending ブロックを更新するだけ。結果として現 turn の assistantMsgId には何も append されず、空のアシスタント bubble が thread に蓄積し、 再生時の prompt にもノイズとして入り込む問題があった。 textBuffer が空 + 対象 message のブロックが 0 件なら、プレースホルダ 「(認証処理を完了しました)」テキストを replaceMessageBlocks で書く。 codex セカンドオピニオン (PR #21) Major M2 指摘対応。 * fix(frontend): auth-request-card で IPv6 loopback (::1) も callback URL として受付 McpServerConfigSchema.url は loopback として localhost / 127.0.0.1 / ::1 / [::1] を許容しているが、AuthRequestCard 側の isLikelyCallbackUrl は localhost / 127.0.0.1 のみ。IPv6 優先環境で SDK がリダイレクト先を http://[::1]:XXXXX/callback?... で通知すると「認証完了」ボタンが永久 に無効になる可能性があった。schema.ts と loopback 判定を揃える。 codex セカンドオピニオン (PR #21) Minor m1 指摘対応。 * fix(chat): PR-B CR 3 周目 5 件対応 (Quick win 4 + Heavy lift 1) CodeRabbit (PR #21) Major 4 件 + Minor 1 件への対応。 [Heavy lift / Major #4] OAuth callback を構造化 WS メッセージで送る これまで callback URL paste 後に「[OAuth callback for ${mcpServerId}] ...」 という自然文を sendChatMessage で送り、AI に mcpServerId を解釈させて mcp__<id>__complete_authentication を呼ばせていた。複数 MCP server を 同時運用する状況で AI が別 server の complete_authentication を選び、 元の auth_request カードが pending のまま残るリスクがあった。 UI から WS の構造化メッセージ {type:'oauth_callback', mcpServerId, callbackUrl} を送り、サーバ側で確定的に「指定 server の complete_authentication を呼べ」とプロンプトを生成する設計に変更。 - ai-engine/server.ts: ChatOAuthCallbackSchema 追加、handler 分岐 - ai-engine/chat-runner.ts: runOAuthCallback メソッド追加 (runUserTurn ラッパー) - frontend/lib/ws.ts: sendOAuthCallback 追加 - frontend/lib/store.ts: sendOAuthCallback action 追加 - frontend/auth-request-card.tsx: sendChatMessage → sendOAuthCallback - 関連 test 7 件を新 API に追従 [Quick win / Major #1] chat_auth_request を discriminated union 化 ai-engine/stream.ts: status='failed' のときのみ failureMessage を必須に する型分割。schema.ts (AuthRequestBlockSchema) の superRefine と型レベル で整合させ、永続化時の検証エラーを型チェックで早期検出。chat-runner の emit 側と test の type narrowing も追従。 [Quick win / Major #3] callback URL の username/password 拒否 auth-request-card.tsx: isLikelyCallbackUrl で URL 内資格情報 (user:pass@) を弾く。schema.ts (McpServerConfigSchema.url) と整合。 [Quick win / Major #5] 最新の pending auth_request を更新 frontend/lib/store.ts: chat_auth_request reducer の走査を末尾起点に変更。 再認証で複数 pending が並ぶケースで、古いカードを更新して新しいカードが pending のまま残る問題を解消。 [Quick win / Minor #2] window.open spy の cleanup を afterEach に auth-request-card.test.tsx: vi.restoreAllMocks() を afterEach に集約し、 テスト失敗時に spy が漏れて後続に影響する経路を塞ぐ。 [見送り / Minor (codex)] auth-detector.ts の id 命名規則 ADR codex セカンドオピニオン Minor m2 (regex / schema 二重メンテの ADR メモ) は 本 PR スコープ外として見送り。 * fix(chat): runOAuthCallback を ephemeral 実装 + サーバ側 callback URL 検証強化 CodeRabbit (PR #21) 4 周目 Major 2 件対応。 [Major #1 / Heavy lift] runOAuthCallback の ephemeral 化 これまで runOAuthCallback は \`yield* this.runUserTurn(text, [])\` を 呼んでおり、callback URL (code/state 含む) が user message として永続化 され、後続の prompt 再構築でも再送される問題があった。さらに通常 turn の ままなので、AI が他のツール (create_node 等) を実行する余地が残っていた。 専用の ephemeral 実行経路に切替: - chatStore.appendMessage を呼ばない (callback URL を chat 履歴に残さない) - assistantMsgId は ephemeral (handleAuthToolResult の orphan 失敗 fallback 以外では参照されない) - 対象 server の config のみ buildMcpServers に渡す - allowedTools = [\`mcp__<id>__complete_authentication\`] の 1 件のみ - tool_use は kind === 'complete_authentication' && id 一致のみ stash - text block と stash 不一致の tool_result は emit せず黙って捨てる - handleAuthToolResult が過去 message の最新 pending を更新する経路は維持 [Major #2] サーバ側 callbackUrl 検証強化 ChatOAuthCallbackSchema.callbackUrl が \`z.string().url()\` のみで、 loopback / no-credentials / code+state 必須を確認していなかった。 WS は UI 経由でない外部クライアントからも到達しうるので、isValidCallbackUrl ヘルパで isLikelyCallbackUrl (UI 側) と同じ条件をサーバ側でも検証する。
4 tasks
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
Atlassian (および任意の HTTP MCP サーバー) を Tally の Chat から呼べるようにする土台。プロジェクト設定で `mcpServers[]` を登録し、ChatRunner が外部 MCP の tool を SDK に動的に渡す。外部 MCP の tool_use / tool_result は `source: 'external'` で承認なしに永続化、UI でも折り畳みカードで表示。
これは PR スタックの 1 本目 (PR #18 を 4 PR に割り直したうちの基盤側)。後続:
変更内容
core
ai-engine
frontend
テスト
```
pnpm typecheck # 4/4 PASS
pnpm test # core 86 / storage 88 / ai-engine 196 / frontend 265 = 635 PASS
```
注
PAT (api_token) 認証前提の実装。後続 PR-B で OAuth 2.1 にピボット (Basic/Bearer header 生成は PR-B で削除)。
Summary by CodeRabbit
新機能
リファクタ
テスト
セキュリティ