protocol-tool-result-shape完了
This commit is contained in:
parent
ce59c5320e
commit
388079759c
2
TODO.md
2
TODO.md
|
|
@ -4,10 +4,8 @@
|
|||
- [ ] Bash ツール (Permission 層と統合) → [tickets/bash-tool.md](tickets/bash-tool.md)
|
||||
- [ ] パーミッション: パターンベースのツール実行制御 → [tickets/permission-extension-point.md](tickets/permission-extension-point.md)
|
||||
- [ ] LLM プロバイダ/モデルカタログ → [tickets/llm-provider-catalog.md](tickets/llm-provider-catalog.md)
|
||||
- [ ] モデル capability の責務を llm-worker 外へ → [tickets/llm-capability-ownership.md](tickets/llm-capability-ownership.md)
|
||||
- [ ] Pod オーケストレーション
|
||||
- [ ] 動的 Scope 変更 → [tickets/dynamic-scope.md](tickets/dynamic-scope.md)
|
||||
- [ ] protocol: ToolResult に summary を分離 → [tickets/protocol-tool-result-shape.md](tickets/protocol-tool-result-shape.md)
|
||||
- [ ] ネイティブ GUI クライアント MVP → [tickets/native-gui-mvp.md](tickets/native-gui-mvp.md)
|
||||
- [ ] TUI 拡充
|
||||
- [ ] フルスクリーン化によるオーバーホール → [tickets/tui-fullscreen-overhaul.md](tickets/tui-fullscreen-overhaul.md)
|
||||
|
|
|
|||
|
|
@ -1,79 +0,0 @@
|
|||
# モデル capability の責務を llm-worker 外へ移す
|
||||
|
||||
## 背景
|
||||
|
||||
`llm-worker` は「wire scheme の送受信」に専念する低レベル基盤に留める方針。にもかかわらず、現状は各 scheme に `capability.rs` が同居していて、`claude-*` / `gpt-5` prefix / `gemini-2.5` prefix / `codex-` prefix 等の個別モデル ID 知識が `llm-worker` に閉じ込められている。
|
||||
|
||||
該当箇所:
|
||||
|
||||
- `crates/llm-worker/src/llm_client/scheme/anthropic/capability.rs::lookup`
|
||||
- `crates/llm-worker/src/llm_client/scheme/openai_chat/capability.rs::classify` / `lookup`
|
||||
- `crates/llm-worker/src/llm_client/scheme/openai_responses/capability.rs::lookup`
|
||||
- `crates/llm-worker/src/llm_client/scheme/gemini/capability.rs::lookup`
|
||||
- `Scheme::capability_for(model_id) -> Option<ModelCapability>` trait メソッド
|
||||
|
||||
「モデル ID から能力を決める」は wire 実装の責務ではない。カタログ/プロバイダ構築層(高レベル側)の責務で、ここに居るのは層の漏出。新モデル(`gpt-6` 等)が出るたびに `llm-worker` を触る構造は抽象が壊れている。
|
||||
|
||||
## 要件
|
||||
|
||||
1. **`Scheme::capability_for` の廃止**: trait から削除。`llm-worker` は wire-level の安全側フォールバックである `default_capability()` のみ持つ。
|
||||
|
||||
2. **モデル知識を `crates/provider` へ移す**:
|
||||
- `crates/provider/src/capability.rs`(または `capability/` モジュール)を新設
|
||||
- 現 `scheme/*/capability.rs` の `lookup`(および `classify`)を移植
|
||||
- 公開 API: `pub fn lookup(scheme: SchemeKind, model_id: &str) -> Option<ModelCapability>`
|
||||
|
||||
3. **`build_client` の capability 解決順は維持**:
|
||||
1. `ModelConfig.capability` 明示指定
|
||||
2. `provider::capability::lookup(scheme, model_id)` (移設先)
|
||||
3. `scheme.default_capability()` (`llm-worker` に残る)
|
||||
|
||||
4. **`llm-worker/examples/*` の追従**: `scheme.capability_for` を使っている箇所(`worker_cli` / `worker_cancel_demo` / `record_test_fixtures`)は `scheme.default_capability()` か明示 `ModelCapability` に置換する。examples を provider に依存させない。
|
||||
|
||||
5. **テストの移設**: scheme 側 capability テストは provider 側に移す(テーブル本体と一緒に移動)。
|
||||
|
||||
6. **完了時の動作**: `cargo build --workspace` が通り、`test_pod.codex.local.toml` 経由の疎通テストが引き続き成功する。
|
||||
|
||||
## 設計判断
|
||||
|
||||
### scheme 側に `default_capability()` を残す理由
|
||||
|
||||
wire format 固有の保守的 default (例: OpenAI 互換は Parallel tool call, JsonSchema structured output) は wire 層の知識で、モデル固有ではない。「この scheme では何が最低限送れるか」を示す安全側の宣言なので `llm-worker` に残す。
|
||||
|
||||
### examples を provider に依存させない
|
||||
|
||||
worker_cli 等は scheme の使い方を示す最小例。provider クレートに依存させると低レベル例として壊れるし、循環依存の気配も出る。examples では `scheme.default_capability()` を使い、モデル固有最適化が必要なら `ModelCapability` 手組みで済ませる。
|
||||
|
||||
### `llm-provider-catalog` との関係
|
||||
|
||||
カタログチケットは「プロバイダと代表モデル ID の提示」が主題で、capability は意図的に載せない判断だった。本チケットはその決定を変えない。`provider::capability::lookup` は provider 層のうちカタログとは別の関数として置く(将来的にカタログが capability ヒントを提供する余地は残すが、本チケットでは扱わない)。
|
||||
|
||||
### 新モデル追加の運用
|
||||
|
||||
`provider::capability::lookup` が「知ってるモデル ID」のテーブルを持つ設計は維持する(本チケットの焦点は場所の正しさ)。新モデル対応はテーブル追記で済むが、新スキーマ/新プロバイダ経路が出たときに llm-worker を触らずに済むのが改善点。
|
||||
|
||||
## Scope 外
|
||||
|
||||
- ランタイム capability 検出 (OpenAI `/v1/models` や ChatGPT backend の動的モデル列挙)
|
||||
- `providers.toml` カタログ連携 (`llm-provider-catalog` チケット側)
|
||||
- prefix match から厳密マッチへの切替(別論点、今回は挙動保存)
|
||||
|
||||
## 影響範囲
|
||||
|
||||
- `crates/llm-worker/src/llm_client/scheme/mod.rs`: `capability_for` trait メソッド削除
|
||||
- `crates/llm-worker/src/llm_client/scheme/*/scheme_impl.rs`: `capability_for` impl 削除
|
||||
- `crates/llm-worker/src/llm_client/scheme/*/capability.rs`: `lookup`/`classify` 削除、`default_capability` のみ残す
|
||||
- `crates/provider/src/capability.rs`: 新設(テーブル統合)
|
||||
- `crates/provider/src/lib.rs`: `build_client` の capability 解決経路を更新
|
||||
- `crates/llm-worker/examples/*`: `capability_for` 呼び出し除去
|
||||
|
||||
## 依存
|
||||
|
||||
- `llm-model-config` 完了済
|
||||
- `llm-scheme-openai-responses` 完了済
|
||||
- `llm-auth-codex-oauth` 完了済
|
||||
|
||||
## Review
|
||||
- 状態: Approve
|
||||
- レビュー詳細: [./llm-capability-ownership.review.md](./llm-capability-ownership.review.md)
|
||||
- 日付: 2026-04-21
|
||||
|
|
@ -1,95 +0,0 @@
|
|||
# Review: モデル capability の責務を llm-worker 外へ移す
|
||||
|
||||
## 前提・要件の確認
|
||||
|
||||
1. **`Scheme::capability_for` の廃止**: 満たされている。
|
||||
- `crates/llm-worker/src/llm_client/scheme/mod.rs:79-83` では trait メソッドが削除され、doc コメントも「`provider::capability::lookup` 側（高レベル構築層）の責務」と明記。
|
||||
- 各 `scheme_impl.rs`（anthropic / openai_chat / openai_responses / gemini）から `capability_for` impl が消えていることを diff で確認。
|
||||
- 念のため grep で `capability_for` 残骸を全 crate 検索 → チケット本文以外の参照なし。
|
||||
|
||||
2. **モデル知識を `crates/provider` へ移す**: 満たされている。
|
||||
- `crates/provider/src/capability.rs:1-153` に Anthropic / OpenAI Chat / OpenAI Responses / Gemini の 4 テーブルと、openai_chat と openai_responses で共有していた `OpenAiFamily` / `openai_classify` を統合。
|
||||
- 公開 API は要件通り `pub fn lookup(scheme: SchemeKind, model_id: &str) -> Option<ModelCapability>`（`crates/provider/src/capability.rs:20-27`）。
|
||||
- `crates/provider/src/lib.rs:13` で `pub mod capability;` としてモジュール公開。
|
||||
|
||||
3. **`build_client` の capability 解決順**: 満たされている。
|
||||
- `crates/provider/src/lib.rs:119-123` で `config.capability.clone().or_else(|| capability::lookup(...)).unwrap_or_else(|| scheme.default_capability())` の 3 段階フォールバックが明示されており、コメントも要件の 3 階層とそれぞれの責務を正しく対応付けている。
|
||||
|
||||
4. **examples の追従（provider に依存させない）**: 満たされている。
|
||||
- `crates/llm-worker/examples/worker_cli.rs:341-349`: `scheme.default_capability()` 直接利用。Ollama 分岐（`worker_cli.rs:378`）だけは従前通りローカル `default_capability()` 関数を使用 → これは「明示 `ModelCapability` に置換」の要件に合致。
|
||||
- `crates/llm-worker/examples/worker_cancel_demo.rs:26-28`: `scheme.default_capability()` のみ。ローカル fallback 定義は削除済。
|
||||
- `crates/llm-worker/examples/record_test_fixtures/main.rs:22-36`: `scheme.default_capability()` + Ollama 分岐で `AnthropicScheme::new().default_capability()`。provider クレート import なし。
|
||||
- examples の `Cargo.toml` に provider 依存が追加されていないことも間接的に確認（worker_cli / worker_cancel_demo は `llm_worker::` のみ import）。
|
||||
|
||||
5. **テストの移設**: 満たされている。
|
||||
- 旧 `scheme/openai_responses/capability.rs` の `#[cfg(test)] mod tests` は削除。
|
||||
- `crates/provider/src/capability.rs:155-211` に 8 件の unit test（Anthropic known/unknown, OpenAI chat gpt5, OpenAI responses codex / gpt-5-codex, Gemini 2.5 / 1.5, unknown across schemes）を配置。旧来 `gpt_5_codex_is_reasoning` / `codex_mini_latest_is_reasoning` / `unknown_model_returns_none` の振る舞いは保存されている。
|
||||
- scheme 側の capability.rs には `#[cfg(test)]` 残留なしを grep で確認。
|
||||
|
||||
6. **完了時の動作**: 満たされている。
|
||||
- `cargo check --workspace --examples` 成功（warning は既存の `end_scope` 未使用のみ、今回の変更と無関係）。
|
||||
- `cargo test -p provider --lib` 33/33 pass（capability テスト 8 件込み）。
|
||||
- `cargo test -p llm-worker --lib` 100/100 pass。
|
||||
- 疎通テストの実行ログは diff に含まれていないが、静的テーブルは 1:1 で移植されており挙動差は出ない（下記「挙動保存の確認」参照）。
|
||||
|
||||
## アーキテクチャ・スコープ
|
||||
|
||||
### レイヤ境界
|
||||
`llm-worker` から `claude-*` / `gpt-5` / `codex-` / `gemini-2.5` 等のモデル ID 固有知識が完全に撤退し、`provider` に集約された。CLAUDE.md の「llm-worker は wire scheme の低レベル基盤に留める」方針と feedback memory `llm_worker_scope` に厳密に合致する、このチケットの趣旨通りの成果。
|
||||
|
||||
### モジュール粒度
|
||||
`provider/src/capability.rs` を 1 ファイルに統合した判断は本件では妥当。理由:
|
||||
- 統合前の 4 ファイル合計 ~180 行で、既に 1 ファイルに収まるサイズ。
|
||||
- `OpenAiFamily` / `openai_classify` が chat / responses 両方から参照される「一次情報」だったので、同一ファイルに閉じ込めた方が `pub(crate)` スコープだけで済み、サブモジュール間の `pub(super)` 公開戦略を考えなくて良い。
|
||||
- 将来テーブルが肥大化した場合の `capability/{anthropic,openai,gemini}.rs` サブモジュール分割は容易（今は不要）。
|
||||
|
||||
判断として Approve。将来新プロバイダを追加したときに 1 ファイル 400-500 行を超えるようなら改めて分割検討で十分。
|
||||
|
||||
### scheme 側 `default_capability()` の存置
|
||||
ticket 設計判断の通り、「この wire で安全に送れる最小共通項」は wire 知識であり scheme 層に残す意味がある。Ollama（Anthropic scheme + 認証なし + `default_capability`）経路でも wire 既定が有効に機能する（`crates/llm-worker/examples/record_test_fixtures/main.rs:126` など）。Approve。
|
||||
|
||||
### examples の provider 非依存
|
||||
worker_cli の Ollama 分岐が、provider 層に依存せずローカル `default_capability()` 関数で手組み `ModelCapability` を作っている実装は、チケット設計判断「examples を provider に依存させない」に一致。循環依存の予防としても正しい。Approve。
|
||||
|
||||
## 挙動保存の確認（旧 → 新のテーブル）
|
||||
|
||||
旧 4 ファイルの `lookup` / `classify` / `OpenAiFamily` と `provider/src/capability.rs` を突き合わせた結果、全フィールド一致:
|
||||
|
||||
| モデル family | 旧位置 | 新位置 | 差分 |
|
||||
| --- | --- | --- | --- |
|
||||
| Anthropic `claude-*` | `scheme/anthropic/capability.rs` | `provider::capability::anthropic_lookup` | 無し（`Explicit { max_breakpoints: 4 }`, `BudgetTokens`, `vision: true`） |
|
||||
| OpenAI `gpt-5` / `o1` / `o3` / `o4` | `scheme/openai_chat/capability.rs` | `openai_classify` + `openai_chat_lookup` | 無し（Reasoning: `Effort`） |
|
||||
| OpenAI `gpt-4` | 同上 | 同上 | 無し |
|
||||
| OpenAI `gpt-3.5` | 同上 | 同上 | 無し（`JsonObject`, `vision: false`） |
|
||||
| OpenAI Responses `codex-*` prefix | `scheme/openai_responses/capability.rs` | `openai_responses_lookup` の `or_else` | 無し（Reasoning にフォールバック） |
|
||||
| Gemini `gemini-*` (一般) | `scheme/gemini/capability.rs` | `gemini_lookup` | 無し |
|
||||
| Gemini `gemini-2.5` / `gemini-3` | 同上 | 同上 | 無し（`BudgetTokens`） |
|
||||
|
||||
`default_capability()` 値も全 scheme で保存（anthropic: vision=false, openai_chat: vision=false, openai_responses: vision=false, gemini: vision=true）。**特に gemini の `vision: true` が保たれているか**というレビュー観点は OK。
|
||||
|
||||
## 指摘事項
|
||||
|
||||
### Blocking
|
||||
なし。
|
||||
|
||||
### Non-blocking / Follow-up
|
||||
|
||||
- **[major][スコープ外変更の混入]** チケット diff にスコープ外の Codex OAuth 疎通修正が同居している（`crates/llm-worker/src/llm_client/scheme/openai_responses/scheme_impl.rs` の `default_base_url` / `path` 変更、`crates/provider/src/lib.rs::effective_base_url`、`crates/provider/src/codex_oauth/mod.rs::build_headers`、`crates/manifest/src/model.rs` の `#[serde(rename = "codex_oauth")]`）。ユーザー自身も「別コミット候補」と認識しているので後続コミット分割で対処できるが、レビュー観点としては:
|
||||
- 本チケットの review が「capability 移設」単体の妥当性判定を越えて Codex 疎通修正の可否まで巻き込むことは避けるべき。
|
||||
- 少なくともコミット段階で `llm-capability-ownership` コミットと「Codex OAuth 疎通修正」コミットを分けた方が後から git log で挙動回帰を追える。
|
||||
- `path` を `/v1/responses` → `/responses` にしつつ `default_base_url` に `/v1` を含める変更は OpenAI 公式経路と ChatGPT backend 経路の URL 組立てを統合する話で、本チケットで動作させた疎通確認が「capability 移設後の回帰」なのか「URL 変更の検証」なのかを混同しやすい。次回以降、本性質の「ついで修正」は別チケット/別コミットに切ることを推奨。
|
||||
|
||||
- **[minor][公開 API の可視性]** `provider::capability::lookup` は Pub だが、`OpenAiFamily` / `openai_classify` / `*_lookup` 系 helper が `pub(crate)` ですら宣言されておらず、module-private 関数として閉じている。現状 `build_client` は `lookup` 1 点経由で十分だが、将来 `llm-provider-catalog` から family を参照したいユースケースが出た時点で公開化を検討すればよい。今は Approve。
|
||||
|
||||
- **[minor][テスト粒度]** 旧 `openai_responses` の `gpt_5_codex_is_reasoning` / `codex_mini_latest_is_reasoning` / `unknown_model_returns_none` が `provider/src/capability.rs` tests に移植されたのは確認済だが、もとの `openai_chat::lookup` 側には個別 test が無かったので、新テーブルでも gpt-4 / gpt-3.5 の classify 成否テストは追加されていない。挙動保存の観点では現状で十分（ticket 要件は「移設」なので）。新モデル追加時に境界テストを足す運用で Approve。
|
||||
|
||||
### Nits
|
||||
|
||||
- `provider/src/capability.rs` の section コメント `// --- Anthropic ---` 等、既存 codebase 他所ではあまり見かけないスタイル。統一上は `mod anthropic { ... }` サブモジュール化でも良かったが、短い関数群の可読性補助としては問題なし。
|
||||
- `lib.rs:119-123` の capability 解決コメントが丁寧で、将来の保守者にとって助かる。good keep.
|
||||
|
||||
## 判断
|
||||
|
||||
**Approve** — 要件 1-6 すべてを満たし、テーブルの移設は旧挙動を完全保存、レイヤ境界を `llm-worker / provider` 間で正しく引き直した。scheme 側 `default_capability()` の存置判断、examples を provider 非依存に保った判断とも妥当。
|
||||
|
||||
ただし「スコープ外 Codex 疎通修正」の同居は、このチケットのコミット外に切り出すのが望ましい（コミットは user の責務なので、分割方針を user 判断で）。
|
||||
|
|
@ -1,46 +0,0 @@
|
|||
# protocol: ToolResult に summary を分離する
|
||||
|
||||
## 背景
|
||||
|
||||
`protocol::Event::ToolResult` は現状 `{ id, output: String, is_error: bool }` で、ツール実装側の `llm_worker::tool::ToolOutput` が持っている **`summary: String`** を flatten して捨てている。
|
||||
|
||||
各 builtin tool は既に意味のある summary を返している:
|
||||
|
||||
- Write (`crates/tools/src/write.rs`): `Created /path (N bytes)` / `Overwrote /path (N bytes)`
|
||||
- Read / Edit / Glob / Grep も同様に構造化された要約を返している
|
||||
|
||||
TUI 側で「俯瞰ビュー」の 1 行サマリや status line の短縮表示に使いたいが、protocol に summary が無いため直接は取れない。`output` の先頭行を切るようなヒューリスティックは脆く、各ツールが意図した要約と一致しない。
|
||||
|
||||
本チケットは protocol 層の小さな拡張に閉じる。TUI 側での利用は後続のオーバーホールチケットで行う。
|
||||
|
||||
## 要件
|
||||
|
||||
- `Event::ToolResult` に `summary: String` を追加する
|
||||
- `output` と `is_error` は残す（用途が異なる: summary は 1 行、output は詳細本文）
|
||||
- ツール実装から流れてくる `ToolOutput.summary` がそのまま protocol に乗るように worker / pod 層を通す
|
||||
- `Event::History` の replay 経路でも summary が欠落しないこと（セッション再接続時に TUI が過去の ToolResult 表示を組み直せるため）
|
||||
|
||||
## 完了条件
|
||||
|
||||
- `Event::ToolResult` に `summary: String` が存在し、worker 側から流れてくる
|
||||
- 各 builtin tool (Read / Write / Edit / Glob / Grep) の `ToolOutput.summary` が TUI まで届く
|
||||
- `Event::History` replay でも summary が取得できる
|
||||
- 既存 TUI は summary を無視するだけで従前通り動く（UI 側での利用は別チケット）
|
||||
|
||||
## 範囲外
|
||||
|
||||
- `ToolOutput.content` の protocol 化。現時点では不要。必要になったら別チケット
|
||||
- summary 文言の規格化や整形。各ツール側の責務
|
||||
- TUI の summary 利用 (`tickets/tui-fullscreen-overhaul.md` で扱う)
|
||||
|
||||
## Review
|
||||
|
||||
- 状態: Request changes
|
||||
- レビュー詳細: [./protocol-tool-result-shape.review.md](./protocol-tool-result-shape.review.md)
|
||||
- 日付: 2026-04-21
|
||||
|
||||
### 指摘反映 (2026-04-21)
|
||||
|
||||
- **Blocking**: `Event::ToolResult` の詳細本文フィールドをチケット本文どおり `output` 名で保持する形に修正。Option 化だけは残し `output: Option<String>` に落ち着けた（空文字列を wire に乗せない / worker の `ToolOutput { summary, content }` と意味論的に整合）。`content` という名前および意味（`ToolOutput.content` の protocol 化）は取り込まない、という範囲外条項は遵守している。
|
||||
- **Non-blocking**: `on_tool_result` の error-path カバレッジを `callback_test.rs::test_callback_tool_result_error_path` として追加。`ToolResult::error` 経由で `summary=エラーメッセージ / content=None / is_error=true` が emit されることを確認。
|
||||
- 再レビュー可。
|
||||
|
|
@ -1,42 +0,0 @@
|
|||
# Review: protocol: ToolResult に summary を分離する
|
||||
|
||||
## 前提・要件の確認
|
||||
|
||||
- **`Event::ToolResult` に `summary: String` を追加**: 満たしている（`crates/protocol/src/lib.rs:112-125`）。
|
||||
- **`output` と `is_error` は残す（用途: summary は 1 行、output は詳細本文）**: **満たしていない**。実装は `output: String` を `content: Option<String>` にリネーム + オプショナル化している（`crates/protocol/src/lib.rs:117-124`）。詳細本文フィールド自体は残っているが、名前と型が要件と食い違う。後述 Blocking 参照。
|
||||
- **`ToolOutput.summary` がそのまま protocol に乗るように worker / pod 層を通す**: 満たしている。worker に `on_tool_result` コールバックを追加（`crates/llm-worker/src/worker.rs:302-323, 777-780`）、pod コントローラーで `Event::ToolResult` を発火（`crates/pod/src/controller.rs:189-197`）。従来この variant を produce するコードは存在しなかったため、この経路新設は必須。
|
||||
- **`Event::History` replay でも summary が欠落しないこと**: 満たしている。`Item::ToolResult` の serde 形は既に `{type: "tool_result", summary, content}` で（`crates/llm-worker/src/llm_client/types.rs:71-82`）、TUI の `restore_history` がそれに合わせて `item["summary"]` を読むように修正された（`crates/tui/src/app.rs:342-348`）。
|
||||
- **既存 TUI は summary を無視するだけで従前通り動く**: 満たしている。live handler / restore_history ともにコンパイル通過、意味的にも summary を表示するようになっている（`crates/tui/src/app.rs:140-155, 342-348`）。
|
||||
|
||||
## アーキテクチャ・スコープ
|
||||
|
||||
- worker 既存の callback 流儀（`on_tool_use_block` / `on_turn_end` / `on_usage` / `on_warning`）と整合した形で `on_tool_result` を追加しており、拡張方法として proportionate。
|
||||
- callback が `post_tool_call` interceptor と `tool_output_limits` truncation の後に fire されるという不変条件が明示され（`worker.rs:310-316, 742-780`）、履歴に入る payload と一致する。この設計選択は妥当で、documentation も揃っている。
|
||||
- `Mutable` / `Locked` いずれの遷移経路でも `tool_result_cbs` が運搬されており、state 型安全性を崩していない（`worker.rs:1300, 1375`）。
|
||||
- TUI 側修正は protocol 変更に追従する最小限で、`tui-fullscreen-overhaul.md` の範疇（ブロックモデル、折りたたみ、レンダラ dispatch 等）に踏み込んでいない。スコープは守られている。
|
||||
- `restore_history` の `item["output"]` → `item["summary"]` は一見 drive-by だが、`Item::ToolResult` の serde 形は元から `{summary, content}` であり、変更前は History 経路で **常に空文字** を表示していた既存バグ。「`Event::History` replay でも summary が欠落しない」「既存 TUI は summary を無視するだけで従前通り動く」という要件を満たすために必要な修正で、スコープ内と判断できる。
|
||||
|
||||
## 指摘事項
|
||||
|
||||
### Blocking
|
||||
|
||||
- **`output` → `content: Option<String>` の改名と Option 化は要件違反**（`crates/protocol/src/lib.rs:117-124`, `crates/pod/src/controller.rs:191-196`, `crates/tui/src/app.rs:140-154`）。
|
||||
- 要件は「`output` と `is_error` は残す」。実装は `output: String` を `content: Option<String>` に差し替えている。
|
||||
- かつチケットの「範囲外」セクションが明示的に "`ToolOutput.content` の protocol 化。現時点では不要。必要になったら別チケット" と記している。`ToolOutput` 側のフィールド名 (`content`) と意味（prune で消せる詳細本文） を protocol に持ち込んでいるため、これは「範囲外」に真正面から抵触する。
|
||||
- ツール内部モデルと名前を揃えたい動機は理解できるが、意思決定は別チケットで扱うべきで、この PR で先取りすべきではない。
|
||||
- **対処方針**: 以下のいずれか。
|
||||
1. チケットの指示通り `output: String` に戻す（意味的に truncation 後の詳細本文を入れる）。pod 側は `result.content.unwrap_or_default()` を詰めるか、`summary` をフォールバックにするかを worker 層の semantics に合わせて選ぶ。
|
||||
2. 仕様変更として妥当だと判断するなら、チケット本文の要件行と「範囲外」の該当項目を **先に** 書き換えてから実装を通す（CLAUDE.md "b. 詳細化や前提の変化" の手順）。
|
||||
|
||||
### Non-blocking / Follow-up
|
||||
|
||||
- `Event::ToolResult` の wire 互換性に破壊変更が入る点はチケット範囲内なのでそれ自体は問題ないが、Blocking を (1) で解消する場合と (2) で解消する場合で最終形が変わる。方針確定後にテストの assertion (`lib.rs:520-542`) と `omits_absent_content` テストの取り扱いを合わせて決めること。
|
||||
- `tool_result_cbs` の error-path カバレッジはない。`ToolResult::error(...)` が `summary: error_message, content: None, is_error: true` で emit されることを示すテストを callback_test に追記するのは follow-up として価値がある（既存テストは success ケースのみ）。
|
||||
|
||||
### Nits
|
||||
|
||||
- なし。コード本体の命名・module 配置・docstring はプロジェクト慣習と整合。
|
||||
|
||||
## 判断
|
||||
|
||||
**Request changes** — 要件の文面と「範囲外」に明示的に反する protocol フィールド名変更（`output` → `content: Option<String>`）が含まれており、これは判断を伴う仕様変更なのでチケット更新を先行させるか、チケット通りに戻す必要がある。それ以外の worker / pod / TUI / テストの取り扱いはすべて proportionate で問題なし。
|
||||
Loading…
Reference in New Issue
Block a user