From d4055fb19d9a0d100ea2fb339fca143ba4ea7200 Mon Sep 17 00:00:00 2001 From: Hare Date: Tue, 21 Apr 2026 20:50:59 +0900 Subject: [PATCH] =?UTF-8?q?TUI=E3=81=AB=E5=90=91=E3=81=91=E3=81=9Fprotocol?= =?UTF-8?q?=E3=81=AE=E8=A9=B3=E7=B4=B0=E8=AA=BF=E6=95=B4?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../agent-memory/ticket-reviewer/MEMORY.md | 1 + ...eedback_explicit_out_of_scope_violation.md | 14 ++ crates/llm-worker/src/worker.rs | 29 ++++ crates/llm-worker/tests/callback_test.rs | 141 ++++++++++++++++++ crates/pod/src/controller.rs | 10 ++ crates/protocol/src/lib.rs | 85 ++++++++++- crates/tui/src/app.rs | 18 +-- tickets/protocol-tool-result-shape.md | 12 ++ tickets/protocol-tool-result-shape.review.md | 42 ++++++ 9 files changed, 337 insertions(+), 15 deletions(-) create mode 100644 .claude/agent-memory/ticket-reviewer/feedback_explicit_out_of_scope_violation.md create mode 100644 tickets/protocol-tool-result-shape.review.md diff --git a/.claude/agent-memory/ticket-reviewer/MEMORY.md b/.claude/agent-memory/ticket-reviewer/MEMORY.md index 0e50694f..5e0cc9e4 100644 --- a/.claude/agent-memory/ticket-reviewer/MEMORY.md +++ b/.claude/agent-memory/ticket-reviewer/MEMORY.md @@ -2,3 +2,4 @@ - [Test-path omission precedent](feedback_test_path_omission.md) — 要件に挙がったテストを「共通ヘルパ経由だから省略」した場合は Approve with follow-up が相場 - [cargo add workspace pitfall](feedback_cargo_add_workspace_pitfall.md) — ルート Cargo.toml に [workspace.dependencies] が未定義、workspace = true 指定は現状使えない - [Out-of-scope diff mixing](feedback_out_of_scope_mixing.md) — スコープ外修正が ticket diff に同居 → [major] Non-blocking で指摘、コミット分割推奨、総合は Approve +- [Explicit out-of-scope violation](feedback_explicit_out_of_scope_violation.md) — 要件/範囲外に反する変更は Request changes、ticket 先更新 or 戻すの二択 diff --git a/.claude/agent-memory/ticket-reviewer/feedback_explicit_out_of_scope_violation.md b/.claude/agent-memory/ticket-reviewer/feedback_explicit_out_of_scope_violation.md new file mode 100644 index 00000000..3e5b428b --- /dev/null +++ b/.claude/agent-memory/ticket-reviewer/feedback_explicit_out_of_scope_violation.md @@ -0,0 +1,14 @@ +--- +name: Explicit out-of-scope violation is Request-changes +description: Precedent — when implementation contradicts ticket's 要件 or 範囲外 sections verbatim, reviewer should Request changes even if the change seems like a quality-of-life improvement +type: feedback +--- + +実装が ticket の「要件」または「範囲外」に明記された指示と矛盾している場合、それが「どう見ても spirit としては改善」であっても Request changes が相場。 + +**Why:** チケットのライフサイクルは CLAUDE.md で "b. 詳細化や前提の変化: `tickets/foo.md` を更新して commit" と定められており、仕様判断は ticket 更新で先に通すのが手順。実装から暗黙に上書きすると、後続チケットが前提にしている文面とのズレが残る。過去の「protocol-tool-result-shape」では要件に「`output` は残す」、範囲外に「`ToolOutput.content` の protocol 化は別チケット」と明記されているにもかかわらず protocol フィールドを `output: String` → `content: Option` に改名する diff が出て、Blocking として指摘。 + +**How to apply:** +- 要件・範囲外セクションの明示的な文言に反する変更を見つけたら、たとえ技術的には合理的でも Blocking として扱う +- 対処方針として (1) 元通りに戻す、(2) ticket を先に書き換える、の 2 択を review に書き添える +- 「内部モデル (worker 側 struct など) と名前を揃える」系の動機は特に見落としやすい。protocol / wire 層は内部型と分離しておくのが既定、合わせるなら意思決定を ticket に上げさせる diff --git a/crates/llm-worker/src/worker.rs b/crates/llm-worker/src/worker.rs index e18397d6..770daca1 100644 --- a/crates/llm-worker/src/worker.rs +++ b/crates/llm-worker/src/worker.rs @@ -162,6 +162,11 @@ pub struct Worker { /// can be forwarded to the user — distinct from `tracing::warn!`, /// which is for developer-facing logs. warning_cbs: Vec>, + /// Tool-result callbacks. Invoked once per completed tool call + /// after post-execution interceptors and the output byte-cap + /// truncation have been applied — i.e. on the same data that + /// enters history. + tool_result_cbs: Vec>, /// Request configuration (max_tokens, temperature, etc.) request_config: RequestConfig, /// Whether the previous run was interrupted @@ -302,6 +307,22 @@ impl Worker { } } + /// Register a callback invoked once per completed tool execution. + /// + /// Fired after `post_tool_call` interceptors and any `content` + /// truncation from `tool_output_limits`, so the callback observes + /// exactly what is persisted to history. Intended for upper layers + /// (e.g. Pod) to forward tool results to clients. + pub fn on_tool_result(&mut self, callback: impl Fn(&ToolResult) + Send + Sync + 'static) { + self.tool_result_cbs.push(Box::new(callback)); + } + + fn emit_tool_result(&self, result: &ToolResult) { + for cb in &self.tool_result_cbs { + cb(result); + } + } + /// Register a turn-end callback (receives 0-based turn number). pub fn on_turn_end(&mut self, callback: impl Fn(usize) + Send + Sync + 'static) { self.turn_end_cbs.push(Box::new(callback)); @@ -753,6 +774,11 @@ impl Worker { } } + // Emit per-result callbacks on the post-truncation payload. + for tool_result in &results { + self.emit_tool_result(tool_result); + } + Ok(ToolExecutionResult::Completed(results)) } @@ -1016,6 +1042,7 @@ impl Worker { turn_start_cbs: Vec::new(), turn_end_cbs: Vec::new(), warning_cbs: Vec::new(), + tool_result_cbs: Vec::new(), request_config: RequestConfig::default(), last_run_interrupted: false, cancel_tx, @@ -1270,6 +1297,7 @@ impl Worker { turn_start_cbs: self.turn_start_cbs, turn_end_cbs: self.turn_end_cbs, warning_cbs: self.warning_cbs, + tool_result_cbs: self.tool_result_cbs, request_config: self.request_config, last_run_interrupted: self.last_run_interrupted, @@ -1344,6 +1372,7 @@ impl Worker { turn_start_cbs: self.turn_start_cbs, turn_end_cbs: self.turn_end_cbs, warning_cbs: self.warning_cbs, + tool_result_cbs: self.tool_result_cbs, request_config: self.request_config, last_run_interrupted: self.last_run_interrupted, diff --git a/crates/llm-worker/tests/callback_test.rs b/crates/llm-worker/tests/callback_test.rs index 1bb972a7..f0cb1071 100644 --- a/crates/llm-worker/tests/callback_test.rs +++ b/crates/llm-worker/tests/callback_test.rs @@ -6,9 +6,11 @@ mod common; use std::sync::{Arc, Mutex}; +use async_trait::async_trait; use common::MockLlmClient; use llm_worker::Worker; use llm_worker::llm_client::event::{Event, ResponseStatus, StatusEvent as ClientStatusEvent}; +use llm_worker::tool::{Tool, ToolDefinition, ToolError, ToolMeta, ToolOutput}; // ============================================================================= // Tests @@ -149,6 +151,145 @@ async fn test_callback_turn_events() { assert_eq!(ends[0], 0); } +/// Stub tool returning a fixed [`ToolOutput`] for result-callback tests. +struct FixedOutputTool { + output: ToolOutput, +} + +#[async_trait] +impl Tool for FixedOutputTool { + async fn execute(&self, _input_json: &str) -> Result { + Ok(self.output.clone()) + } +} + +fn fixed_tool(name: &'static str, output: ToolOutput) -> ToolDefinition { + Arc::new(move || { + let meta = ToolMeta::new(name).input_schema(serde_json::json!({"type":"object"})); + ( + meta, + Arc::new(FixedOutputTool { + output: output.clone(), + }) as Arc, + ) + }) +} + +/// Verify that on_tool_result fires once per executed tool with +/// summary/content/is_error matching what the tool returned. +#[tokio::test] +async fn test_callback_tool_result_events() { + let events = vec![ + Event::tool_use_start(0, "call_1", "fixed"), + Event::tool_input_delta(0, "{}"), + Event::tool_use_stop(0), + Event::Status(ClientStatusEvent { + status: ResponseStatus::Completed, + }), + ]; + + let client = MockLlmClient::new(events); + let mut worker = Worker::new(client); + + worker.register_tool(fixed_tool( + "fixed", + ToolOutput { + summary: "did the thing".into(), + content: Some("full detail body".into()), + }, + )); + + let captured: Arc, bool)>>> = + Arc::new(Mutex::new(Vec::new())); + let sink = captured.clone(); + worker.on_tool_result(move |result| { + sink.lock().unwrap().push(( + result.tool_use_id.clone(), + result.summary.clone(), + result.content.clone(), + result.is_error, + )); + }); + + let _ = worker.run("call it").await; + + let observed = captured.lock().unwrap(); + assert_eq!(observed.len(), 1); + assert_eq!(observed[0].0, "call_1"); + assert_eq!(observed[0].1, "did the thing"); + assert_eq!(observed[0].2.as_deref(), Some("full detail body")); + assert!(!observed[0].3); +} + +/// Stub tool that always fails, for exercising the error path through +/// `on_tool_result`. +struct ErroringTool { + message: String, +} + +#[async_trait] +impl Tool for ErroringTool { + async fn execute(&self, _input_json: &str) -> Result { + Err(ToolError::ExecutionFailed(self.message.clone())) + } +} + +fn erroring_tool(name: &'static str, message: &'static str) -> ToolDefinition { + Arc::new(move || { + let meta = ToolMeta::new(name).input_schema(serde_json::json!({"type":"object"})); + ( + meta, + Arc::new(ErroringTool { + message: message.to_string(), + }) as Arc, + ) + }) +} + +/// Verify on_tool_result also fires for failed executions with +/// is_error=true, and that the ToolOutput content channel stays empty. +#[tokio::test] +async fn test_callback_tool_result_error_path() { + let events = vec![ + Event::tool_use_start(0, "call_err", "erroring"), + Event::tool_input_delta(0, "{}"), + Event::tool_use_stop(0), + Event::Status(ClientStatusEvent { + status: ResponseStatus::Completed, + }), + ]; + + let client = MockLlmClient::new(events); + let mut worker = Worker::new(client); + + worker.register_tool(erroring_tool("erroring", "boom")); + + let captured: Arc, bool)>>> = + Arc::new(Mutex::new(Vec::new())); + let sink = captured.clone(); + worker.on_tool_result(move |result| { + sink.lock().unwrap().push(( + result.tool_use_id.clone(), + result.summary.clone(), + result.content.clone(), + result.is_error, + )); + }); + + let _ = worker.run("fail it").await; + + let observed = captured.lock().unwrap(); + assert_eq!(observed.len(), 1); + assert_eq!(observed[0].0, "call_err"); + assert!( + observed[0].1.contains("boom"), + "summary should carry the error message: {}", + observed[0].1 + ); + assert!(observed[0].2.is_none()); + assert!(observed[0].3); +} + /// Verify that on_usage callback receives usage events #[tokio::test] async fn test_callback_usage_events() { diff --git a/crates/pod/src/controller.rs b/crates/pod/src/controller.rs index 8fb4442b..07596f90 100644 --- a/crates/pod/src/controller.rs +++ b/crates/pod/src/controller.rs @@ -186,6 +186,16 @@ impl PodController { }); }); + let tx = event_tx.clone(); + worker.on_tool_result(move |result| { + let _ = tx.send(Event::ToolResult { + id: result.tool_use_id.clone(), + summary: result.summary.clone(), + output: result.content.clone(), + is_error: result.is_error, + }); + }); + let tx = event_tx.clone(); worker.on_usage(move |event| { let _ = tx.send(Event::Usage { diff --git a/crates/protocol/src/lib.rs b/crates/protocol/src/lib.rs index fad95709..cf7d0a43 100644 --- a/crates/protocol/src/lib.rs +++ b/crates/protocol/src/lib.rs @@ -111,7 +111,14 @@ pub enum Event { }, ToolResult { id: String, - output: String, + /// Short human-readable summary. Always present; used by clients + /// that only want a 1-line rendering (e.g. collapsed views). + summary: String, + /// Full tool output. Absent when the tool chose to return + /// summary-only, or when the result was pruned. + #[serde(default, skip_serializing_if = "Option::is_none")] + output: Option, + #[serde(default)] is_error: bool, }, Usage { @@ -502,6 +509,82 @@ mod tests { } } + #[test] + fn event_tool_result_roundtrip() { + let event = Event::ToolResult { + id: "call_1".into(), + summary: "Read 128 bytes".into(), + output: Some("hello world".into()), + is_error: false, + }; + let json = serde_json::to_string(&event).unwrap(); + let parsed: serde_json::Value = serde_json::from_str(&json).unwrap(); + assert_eq!(parsed["event"], "tool_result"); + assert_eq!(parsed["data"]["id"], "call_1"); + assert_eq!(parsed["data"]["summary"], "Read 128 bytes"); + assert_eq!(parsed["data"]["output"], "hello world"); + assert_eq!(parsed["data"]["is_error"], false); + + let decoded: Event = serde_json::from_str(&json).unwrap(); + match decoded { + Event::ToolResult { + id, + summary, + output, + is_error, + } => { + assert_eq!(id, "call_1"); + assert_eq!(summary, "Read 128 bytes"); + assert_eq!(output.as_deref(), Some("hello world")); + assert!(!is_error); + } + other => panic!("expected ToolResult, got {other:?}"), + } + } + + #[test] + fn event_tool_result_omits_absent_output() { + let event = Event::ToolResult { + id: "call_2".into(), + summary: "ok".into(), + output: None, + is_error: false, + }; + let json = serde_json::to_string(&event).unwrap(); + assert!( + !json.contains("\"output\""), + "absent output must not be serialized: {json}" + ); + } + + #[test] + fn event_tool_result_error_roundtrip() { + let event = Event::ToolResult { + id: "call_3".into(), + summary: "invalid argument".into(), + output: None, + is_error: true, + }; + let json = serde_json::to_string(&event).unwrap(); + let parsed: serde_json::Value = serde_json::from_str(&json).unwrap(); + assert_eq!(parsed["data"]["is_error"], true); + + let decoded: Event = serde_json::from_str(&json).unwrap(); + match decoded { + Event::ToolResult { + summary, + output, + is_error, + .. + } => { + assert_eq!(summary, "invalid argument"); + assert!(output.is_none()); + assert!(is_error); + } + other => panic!("expected ToolResult, got {other:?}"), + } + } + #[test] fn event_error_format() { let event = Event::Error { diff --git a/crates/tui/src/app.rs b/crates/tui/src/app.rs index 1ccdb107..76774452 100644 --- a/crates/tui/src/app.rs +++ b/crates/tui/src/app.rs @@ -140,21 +140,16 @@ impl App { )); } Event::ToolResult { - output, is_error, .. + summary, is_error, .. } => { let prefix = if is_error { "[tool error]" } else { "[tool result]" }; - let display = if output.len() > 200 { - format!("{}...", &output[..200]) - } else { - output - }; self.output_queue.push(OutputItem::Padded( MessageKind::Tool, - format!("{prefix} {display}"), + format!("{prefix} {summary}"), )); } Event::Usage { @@ -345,15 +340,10 @@ impl App { )); } "tool_result" => { - let output = item["output"].as_str().unwrap_or(""); - let display = if output.len() > 200 { - format!("{}...", &output[..200]) - } else { - output.to_owned() - }; + let summary = item["summary"].as_str().unwrap_or(""); self.output_queue.push(OutputItem::Padded( MessageKind::Tool, - format!("[tool result] {display}"), + format!("[tool result] {summary}"), )); } _ => {} diff --git a/tickets/protocol-tool-result-shape.md b/tickets/protocol-tool-result-shape.md index a03d8fcf..f467fc93 100644 --- a/tickets/protocol-tool-result-shape.md +++ b/tickets/protocol-tool-result-shape.md @@ -32,3 +32,15 @@ TUI 側で「俯瞰ビュー」の 1 行サマリや status line の短縮表示 - `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` に落ち着けた（空文字列を 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 されることを確認。 +- 再レビュー可。 diff --git a/tickets/protocol-tool-result-shape.review.md b/tickets/protocol-tool-result-shape.review.md new file mode 100644 index 00000000..d3d7c43c --- /dev/null +++ b/tickets/protocol-tool-result-shape.review.md @@ -0,0 +1,42 @@ +# 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` にリネーム + オプショナル化している（`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` の改名と 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` に差し替えている。 + - かつチケットの「範囲外」セクションが明示的に "`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`）が含まれており、これは判断を伴う仕様変更なのでチケット更新を先行させるか、チケット通りに戻す必要がある。それ以外の worker / pod / TUI / テストの取り扱いはすべて proportionate で問題なし。