From d076258d300ee46a834424335d11b8e24f1e2f87 Mon Sep 17 00:00:00 2001 From: Hare Date: Fri, 15 May 2026 05:33:33 +0900 Subject: [PATCH] =?UTF-8?q?update:=20Controller=E3=81=A7=E5=85=A5=E5=8A=9B?= =?UTF-8?q?=E3=81=AEValidation=E3=82=92=E8=A1=8C=E3=81=A3=E3=81=A6?= =?UTF-8?q?=E3=81=84=E3=81=9F=E9=83=A8=E5=88=86=E3=82=92Pod=E5=81=B4?= =?UTF-8?q?=E3=81=AB=E7=A7=BB=E3=81=99?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- KNOWN_ISSUES.md | 15 ++++++++ crates/pod/src/controller.rs | 23 ++++++------ crates/pod/src/interrupt_and_run.rs | 8 +++++ crates/pod/src/pod.rs | 13 +++++-- tickets/pod-input-validate-internalize.md | 5 +++ .../pod-input-validate-internalize.review.md | 36 +++++++++++++++++++ 6 files changed, 84 insertions(+), 16 deletions(-) create mode 100644 KNOWN_ISSUES.md create mode 100644 tickets/pod-input-validate-internalize.review.md diff --git a/KNOWN_ISSUES.md b/KNOWN_ISSUES.md new file mode 100644 index 00000000..8bfc005e --- /dev/null +++ b/KNOWN_ISSUES.md @@ -0,0 +1,15 @@ +# Known Issues + +Ticket を切るほどではないが、次に近所を触るときに合わせて拾いたい小粒な所見の置き場。 + +## 運用 + +- 1 項目 = 出典 (file:line) + 症状 (一文) + トリガー (いつ拾うか、一文) +- 関連 ticket があれば `→ [tickets/foo.md]` でリンク +- 修正したら同じコミットで該当エントリを削除する (履歴は git) +- ここに溜める基準: 「ticket は重い」「だが忘れたら次の触り手が踏む」もの。明確に作業すべきものは ticket 化する + +## エントリ + +- `crates/tui/src/app.rs:478-485` — bad workflow slug を含む `Method::Run` 送信時、`Event::UserMessage` の早期 broadcast で `turn_index += 1` されターンヘッダだけ残る ("ghost turn header")。次に TUI のターンヘッダ / エラー表示周りを触るときに整理。→ [tickets/pod-input-validate-internalize.md] の review 由来。 +- `crates/pod/src/controller.rs:944` — `worker_error_code` で `PodError::WorkflowResolve(_) => InvalidRequest` が post-commit な resolve エラー (`KnowledgeNotFound` 等) にも適用される。意味論的には妥当方向だが、resolve 系のエラー粒度を分けたくなったタイミングで再評価。 diff --git a/crates/pod/src/controller.rs b/crates/pod/src/controller.rs index 49f3b7a1..7f821986 100644 --- a/crates/pod/src/controller.rs +++ b/crates/pod/src/controller.rs @@ -610,19 +610,15 @@ async fn controller_loop( }); continue; } - if let Err(e) = pod.validate_workflow_invocations(&input) { - let _ = event_tx.send(Event::Error { - code: ErrorCode::InvalidRequest, - message: e.to_string(), - }); - continue; - } - // Broadcast the accepted user message so every - // subscriber (including the submitter) can render the - // turn header + user line from a single source of - // truth. shared_state's `user_segments` is re-synced - // from `pod` after the run completes, so we don't push - // here. + // Broadcast the user message so every subscriber + // (including the submitter) can render the turn header + // + user line from a single source of truth. + // shared_state's `user_segments` is re-synced from + // `pod` after the run completes, so we don't push + // here. Workflow-invocation validation happens inside + // `Pod::run` / `Pod::interrupt_and_run`; on failure the + // turn errors out via `Event::Error { InvalidRequest }` + // before any UserInput is committed. let _ = event_tx.send(Event::UserMessage { segments: input.clone(), }); @@ -945,6 +941,7 @@ fn worker_error_code(e: &PodError) -> ErrorCode { _ => ErrorCode::Internal, }, PodError::Provider(_) => ErrorCode::ProviderError, + PodError::WorkflowResolve(_) => ErrorCode::InvalidRequest, _ => ErrorCode::Internal, } } diff --git a/crates/pod/src/interrupt_and_run.rs b/crates/pod/src/interrupt_and_run.rs index 117cbd07..886f553c 100644 --- a/crates/pod/src/interrupt_and_run.rs +++ b/crates/pod/src/interrupt_and_run.rs @@ -28,6 +28,14 @@ impl Pod { &mut self, input: Vec, ) -> Result { + // Validate before any side effects so a bad workflow slug does + // not leave half-applied interrupt prep (orphan closure + + // system note) in worker history. `Pod::run` validates again at + // its own entry; the duplicate call is cheap (read-only) and + // collapses naturally once `interrupt_and_run` folds into + // `Pod::run` (see ticket pod-interrupt-prep-internalize). + self.validate_workflow_invocations(&input)?; + let tool_result_summary = self .prompts() .interrupt_tool_result_summary() diff --git a/crates/pod/src/pod.rs b/crates/pod/src/pod.rs index f6c7f2ee..83c93f16 100644 --- a/crates/pod/src/pod.rs +++ b/crates/pod/src/pod.rs @@ -1136,6 +1136,12 @@ impl Pod { /// the Worker is aborted, history is compacted, and execution resumes /// automatically. pub async fn run(&mut self, input: Vec) -> Result { + // Validate workflow invocations up front so an invalid slug + // never commits a UserInput entry, never triggers pre-run + // compaction, and never half-applies interrupt prep when run + // from `interrupt_and_run`. Read-only against `workflow_registry`. + self.validate_workflow_invocations(&input)?; + self.prepare_for_run().await?; // Persist the user input as typed segments before the worker @@ -1394,9 +1400,10 @@ impl Pod { } /// Validate explicit workflow invocations without reading dependency - /// bodies. Used by the controller before broadcasting `UserMessage` so - /// user-invocation errors are returned immediately and never reach the - /// Worker or client history. + /// bodies. Called from `Pod::run` / `Pod::interrupt_and_run` entry so + /// an invalid slug aborts the turn before any session-log commit or + /// interrupt-prep side effects; `pub` so completion / preview paths + /// can also dry-check inputs. pub fn validate_workflow_invocations( &self, segments: &[Segment], diff --git a/tickets/pod-input-validate-internalize.md b/tickets/pod-input-validate-internalize.md index 2e806607..ccf9b59f 100644 --- a/tickets/pod-input-validate-internalize.md +++ b/tickets/pod-input-validate-internalize.md @@ -48,3 +48,8 @@ attachments.extend(self.resolve_workflow_invocations(&input)?); - `resolve_workflow_invocations` 側のロジック変更。 - `validate_workflow_invocations` の判定基準変更（user_invocable / slug parse 等）。 - `Method::Run` 以外の経路（Resume / RunForNotification）からの入力検証。Resume は入力を取らず、RunForNotification の入力は notify buffer drain で system message として入るため workflow invocation の経路に乗らない。 + +## Review +- 状態: Approve with follow-up +- レビュー詳細: [./pod-input-validate-internalize.review.md](./pod-input-validate-internalize.review.md) +- 日付: 2026-05-15 diff --git a/tickets/pod-input-validate-internalize.review.md b/tickets/pod-input-validate-internalize.review.md new file mode 100644 index 00000000..e01aa2ee --- /dev/null +++ b/tickets/pod-input-validate-internalize.review.md @@ -0,0 +1,36 @@ +# Review: Run 入力の事前 validate を Pod 内部に閉じる + +## 前提・要件の確認 + +- 要件1「`Pod::run` の冒頭、`commit_entry(LogEntry::UserInput { .. })` よりも前に `validate_workflow_invocations` を呼び、エラー時は session log に何も commit せず `PodError` で early return」: 満たされている。`crates/pod/src/pod.rs:1138-1145` で `self.validate_workflow_invocations(&input)?;` が `prepare_for_run` よりも前、すなわち `ensure_session_head` / pre-run compact / `commit_entry(UserInput)` のいずれよりも手前にある。`?` で `WorkflowResolveError` が `PodError::WorkflowResolve` に lift され early return される。 +- 要件2「`controller_loop` の `Method::Run` ハンドリングから事前 validate 呼び出しを削除」: 満たされている。`crates/pod/src/controller.rs:600-630` の `Method::Run` ブランチに validate 呼び出しは存在しない。 +- 要件3「`PodError` を `worker_error_code` で `ErrorCode::InvalidRequest` にマップ」: 満たされている。`crates/pod/src/controller.rs:944` に `PodError::WorkflowResolve(_) => ErrorCode::InvalidRequest` 分岐が追加され、ticket が示した「既存バリアントの再利用」方針通り `WorkflowResolveError` を新規ラップせずに既存 `PodError::WorkflowResolve(#[from] WorkflowResolveError)` (`pod.rs:3177-3178`) を流用している。 +- 要件4「`Pod::validate_workflow_invocations` の可視性は `pub` のまま」: 満たされている (`pod.rs:1407`)。docstring は controller 経路ではなく `Pod::run` / `Pod::interrupt_and_run` 入口で呼ぶ旨に更新済 (`pod.rs:1402-1406`)。 + +## 完了条件の確認 + +- 「`controller.rs` の `Method::Run` ブランチに `pod.validate_workflow_invocations` の呼び出しが残っていない」: OK (`grep` で残存呼び出しなし、Pod::run/interrupt_and_run のみ参照)。 +- 「不正な workflow slug を含む `Method::Run` を投げると、UserInput が session log に commit されないまま `Event::Error { code: InvalidRequest, .. }` が flow する」: OK。`Pod::run` 入口の `validate_workflow_invocations?` が `prepare_for_run` / `commit_entry(UserInput)` よりも手前で抜けるため session log には何も書かれない。`drive_turn` の `Err(e)` ブランチが `worker_error_code` 経由で `InvalidRequest` を載せた `Event::Error` を broadcast する。 +- 「既存テスト通過」: `cargo test -p pod --lib` 173/173 通過 (再現確認済)。`workflow::tests::{resolves_requires_before_workflow_body, user_invocable_false_errors, missing_required_knowledge_errors}` は `resolve_workflow_invocation` の関数単体テストで本 refactor の影響を受けない。 + +## アーキテクチャ・スコープ + +- 責務漏れの是正方向は妥当: controller が Pod 内部の `workflow_registry` を覗いて条件分岐していたのを Pod の入口に集約。read-only 関数を本来の所有者側で呼ぶだけの最小変更。 +- 範囲外項目 (resolve ロジック / 判定基準 / Resume / RunForNotification) には踏み込んでいない。`run_for_notification` / `resume` は user 入力 segments を受け取らないので validate 経路に乗らない点もコードと整合。 +- `interrupt_and_run` 側にも validate を入れた判断は妥当。チケットには明示されていないが、ticket 背景にある同じ責務漏れパターン (`pod-interrupt-prep-internalize`) と整合させるため必要。validate 抜きで `interrupt_and_run` を呼ぶと orphan tool_result closure / interrupt system note が worker history に書き込まれた後で `Pod::run` 入口の validate に弾かれ、half-applied 状態が残る。`interrupt_and_run.rs:31-37` のコメントが `pod-interrupt-prep-internalize` 統合時に自然に重複解消する旨を明示しており、将来の畳み込みに対する負債もない。 +- `worker_error_code` の新規 `WorkflowResolve` 分岐は副作用として post-commit な resolve エラー (`KnowledgeNotFound` / `KnowledgeRead` / `KnowledgeFrontmatter`) も `InvalidRequest` にマップする。これらは以前 `_ => Internal` に落ちていたため挙動変化だが、意味論的にはより適切方向であり、validate / resolve でバリアントを分けるのは本チケットの範囲を超える。許容範囲。 + +## 指摘事項 + +### Non-blocking / Follow-up + +- `Event::UserMessage` 早期 broadcast 起因の UX 差分: 旧実装では validate 失敗時、`Event::UserMessage` も `Event::Status { Running }` も飛ばなかった。新実装では bad slug の場合でも UserMessage → Status(Running) → Error → Status(Idle) が並ぶ。TUI (`crates/tui/src/app.rs:478-485`) は `UserMessage` 受信で `turn_index += 1` し TurnHeader + UserMessage block を push するため、エラーが返るだけのリクエストでも turn 番号が前進し、ターンヘッダと user line がエラーブロックの上に残る。チケットの完了条件 (session log への UserInput commit 抑止) には抵触しないが、`bad slug → ghost turn header が残る` 表示挙動は旧実装と異なる。フォローアップで controller 側の `Method::Run` ハンドリングを「validate を controller でも呼ばずに Pod 側に丸投げするのは維持しつつ、UserMessage broadcast は drive_turn の中で投げる」「Pod::run 内で resolve 成功直後に Event::UserMessage 相当を内側から飛ばす」等の動線整理を検討する余地がある。本チケットの責務漏れ修正としては完了範囲外と判断。 +- `worker_error_code` の `WorkflowResolve` 分岐コメント: validate-stage 失敗 (InvalidSlug / NotFound / NotUserInvocable) と resolve-stage 失敗 (KnowledgeNotFound / KnowledgeRead / KnowledgeFrontmatter) が同じ `InvalidRequest` に丸まる旨を一行コメントで明示しておくと将来の差分判断が楽になる。Nit に近い。 + +### Nits + +- なし。 + +## 判断 + +Approve with follow-up — チケットの前提・要件・完了条件は全て満たされ、責務漏れの修正方向も妥当。`Event::UserMessage` 早期 broadcast による turn 表示差分は許容範囲のトレードオフ (実装者ノート通り) で、本チケットの完了範囲外として別途整理する余地のみ残る。