diff --git a/docs/report/test-validity-20260612/README.md b/docs/report/test-validity-20260612/README.md new file mode 100644 index 00000000..774e73c8 --- /dev/null +++ b/docs/report/test-validity-20260612/README.md @@ -0,0 +1,32 @@ +# クレート別テスト妥当性レビュー + +2026-06-12 にクレート別 reviewer Pod で作成したテスト妥当性レビューです。 + +実行時の event log: + +`/run/user/1000/yoi/yoi-report-ja-restore-20260612183253` + +レポート一覧: + +- [client](client.md) — # テスト妥当性レビュー: client - 評価: 概ね良い +- [daemon](daemon.md) — # テスト妥当性レビュー: daemon - 評価: 混在 +- [lint-common](lint-common.md) — # テスト妥当性レビュー: lint-common +- [llm-worker-macros](llm-worker-macros.md) — # テスト妥当性レビュー: llm-worker-macros - 評価: 混在 +- [llm-worker](llm-worker.md) — # テスト妥当性レビュー: llm-worker - 評価: 概ね良い +- [manifest](manifest.md) — # テスト妥当性レビュー: manifest - 評価: 概ね良い +- [memory](memory.md) — # テスト妥当性レビュー: memory +- [pod](pod.md) — # テスト妥当性レビュー: pod - 評価: 混在 +- [pod-registry](pod-registry.md) — # テスト妥当性レビュー: pod-registry +- [pod-store](pod-store.md) — # テスト妥当性レビュー: pod-store - 評価: 混在 +- [project-record](project-record.md) — # テスト妥当性レビュー: project-record - 評価: 概ね良い +- [protocol](protocol.md) — # テスト妥当性レビュー: protocol - 評価: 概ね良い +- [provider](provider.md) — # テスト妥当性レビュー: provider - 評価: 概ね良い +- [secrets](secrets.md) — # テスト妥当性レビュー: secrets +- [session-analytics](session-analytics.md) — # テスト妥当性レビュー: session-analytics - 評価: 概ね良い +- [session-metrics](session-metrics.md) — # テスト妥当性レビュー: session-metrics - 評価: 概ね良い +- [session-store](session-store.md) — # テスト妥当性レビュー: session-store - 評価: 概ね良い +- [ticket](ticket.md) — # テスト妥当性レビュー: ticket - 評価: 概ね良い +- [tools](tools.md) — # テスト妥当性レビュー: tools - 評価: 混在 +- [tui](tui.md) — # テスト妥当性レビュー: tui - 評価: 混在 +- [workflow](workflow.md) — # テスト妥当性レビュー: workflow +- [yoi](yoi.md) — # テスト妥当性レビュー: yoi - 評価: 混在 diff --git a/docs/report/test-validity-20260612/client.md b/docs/report/test-validity-20260612/client.md new file mode 100644 index 00000000..a5babc8b --- /dev/null +++ b/docs/report/test-validity-20260612/client.md @@ -0,0 +1,94 @@ +# テスト妥当性レビュー: client + +- 評価: 概ね良い + +## 確認範囲 + +- `crates/client/Cargo.toml` と crate の全ソースファイルを確認した: + - `src/lib.rs` + - `src/runtime_command.rs` + - `src/spawn.rs` + - `src/pod_client.rs` + - `src/ticket_role.rs` +- `runtime_command`、`spawn`、`pod_client`、`ticket_role` の既存 unit tests を確認した。 +- `crates/client/tests/` 配下に integration tests がないことを確認した。 +- ソースファイルや Tickets は変更していない。 + +## 現在のテストがよくカバーしている点 + +- `runtime_command` のカバレッジは焦点が絞られていて適切: + - runtime executable には常に `pod` prefix が付与される; + - `YOI_POD_RUNTIME_COMMAND` は executable path だけを置き換える; + - 空の override は `current_exe` にフォールバックする。 +- `spawn::runtime_args` tests は重要な引数 invariants をカバーしている: + - workspace、pod identity、profile が分離されたままである; + - session restore mode では profile が省略される; + - child `cwd` は CLI argument として渡されない; + - `ticket_role` が存在する場合は明示的に渡される。 +- `pod_client` tests は mock だけでなく実際の Unix sockets を使っている: + - client が生存している間に events を受信する; + - `Method` JSON lines を送信する; + - drop された clients は connections を速やかに閉じる; + - blocked reader tasks は drop 時に abort される。 +- `ticket_role` は planning について広いカバレッジがある: + - role config 欠落と backend-only config が拒否される; + - `inherit` profile は top-level launches で拒否される; + - 解決不能な profile selectors は spawn 前に失敗する; + - concrete role config と scaffold config が launch plans を生成する; + - 設定された workflow/profile/launch prompt refs が表面化される; + - spawn config が pod name、profile、role、workspace root、`cwd` 境界を正しく保持する; + - prompt override precedence がテストされている; + - role-specific launch prompts が intake、orchestrator、coder、reviewer 向けの重要な運用ガイダンスを含む。 +- pre-run peer registration tests は、実際の `PodClient` framing path を通し、最初の `Run` 前の ordering を検証しているため有用。 + +## ギャップ / 疑問のあるテスト + +- 最大のギャップは `spawn_pod` の readiness behavior。実際の subprocess launch path、stderr file polling、`YOI-READY` parsing、socket wait loop、early-exit handling、stderr tail collection、malformed ready line handling、timeout behavior はほとんど未テスト。現在の `spawn` tests は純粋な argument construction だけをカバーしている。 +- `launch_ticket_role_pod_with_options` は end-to-end の host-side launch flow としてテストされていない。planning と pre-run send tests に分解されているが、`YOI-READY` を出力し、socket を bind し、最初の `Run` を受け取り、acceptance evidence を返す fake runtime process がない。 +- `wait_for_run_acceptance` は重要な分岐が直接カバーされていない: + - 一致する `Event::UserMessage` を受理する; + - `InvokeStart(UserSend)` / `TurnStart` を受理する; + - `Event::Error` で拒否する; + - closed / timeout errors を返す; + - unrelated events を安全に無視する。 +- Pre-run peer registration は success と explicit error のカバレッジがあるが、timeout、connection-closed、empty peer name、multiple peer registrations、response 前の unrelated-event cases が不足している。 +- Prompt tests は prompt resources から多数の exact substrings を assert している。prompt text は product contract の一部なのでこの一部は有益だが、大量の substring lists は脆く、無害な prompt wording edits が behavioral regressions に見える可能性がある。 +- Default pod-name generation と sanitization は、planning cases と caller-provided name 経由の間接テストを除き、直接テストされていない。whitespace ticket ids、punctuation-heavy ids、`MAX_POD_NAME_CHARS` への truncation、空の explicit pod names のような edge cases はカバーする価値がある。 +- `PodClient` tests は malformed JSON、EOF behavior、`try_next_event`、reader-channel backpressure をカバーしていない。これらは spawn/launch gaps より優先度は低いが、それでも crate の protocol boundary の一部である。 +- doc tests や integration tests はない。小さな client crate としては許容できるが、この crate の責務には process/socket orchestration が含まれるため、少なくとも 1 つの lightweight fake-runtime integration-style unit test があると信頼性が実質的に向上する。 + +## 追加提案 + +- 小さな test executable または shell script を使って、`spawn_pod` 用の fake-runtime tests を追加する: + - 通常の stderr progress lines を書く; + - `YOI-READY\t\t` を出力する; + - 短い delay の後に socket を bind する; + - failure cases では早期終了する。 +- readiness parsing について焦点を絞った unit tests を追加する: + - malformed ready line; + - socket なしの ready line; + - ready line 後、socket が connectable になる前に child が終了する; + - stderr tail が bounded tail だけを保持し、最後に flush された lines を含む。 +- in-process Unix socket server を使って `wait_for_run_acceptance` 周辺の tests を追加する: + - `UserMessage` exact segment match; + - `InvokeStart(UserSend)` acceptance; + - `TurnStart` acceptance; + - `Event::Error` rejection; + - connection close; + - unrelated events がある timeout。 +- pre-run peer registration の edge-case tests を追加する: + - empty peer name は warning を出し、それでも `Run` を送信する; + - timeout warning でも `Run` を送信する; + - closed connection warning または send error path; + - multiple peers が ordering を保持する。 +- Pod identity stability は user-visible で runtime state paths に影響するため、`default_pod_name` / sanitization / truncation について小さな pure tests を追加する。 +- exact wording が意図的に stable contract として扱われている場合を除き、prompt resource ごとに小さめの semantic sentinel assertions を維持することで、prompt の脆さを減らすことを検討する。 + +## 実行したコマンド + +```sh +cd /home/hare/Projects/yoi && cargo test -p client +``` + +結果: passed。`29` unit tests が passed; doc-tests は `0` tests。このコマンドは 2 回実行され、どちらも passed。 + diff --git a/docs/report/test-validity-20260612/daemon.md b/docs/report/test-validity-20260612/daemon.md new file mode 100644 index 00000000..7b8f0bf2 --- /dev/null +++ b/docs/report/test-validity-20260612/daemon.md @@ -0,0 +1,41 @@ +# テスト妥当性レビュー: daemon +- 評価: 混在 + +## 確認範囲 +- `crates/daemon/Cargo.toml` +- `crates/daemon/src/lib.rs` +- `crates/daemon/README.md` +- リポジトリ内の `daemon` 参照 +- `cargo test -p daemon` + +`daemon` は現時点では意図的な placeholder crate です。`README.md` では、将来の long-lived Pod lifecycle management のために予約されており、現在の Pod socket serving、CLI/TUI startup、registry mechanics をまだ所有してはいけないと説明されています。`src/lib.rs` は空です。 + +## 現在のテストがよくカバーしていること +- `cargo test -p daemon` は成功します。 +- crate が package target としてビルドできることは確認されています。 +- unit test と doc test は正常に実行されますが、どちらも `0` tests です。 +- 現在の空実装では、この crate 内で直接テストできる振る舞いはありません。 + +## 不足・疑わしいテスト +- テストは存在しないため、現在の test suite が証明しているのは placeholder crate がコンパイルできることだけです。 +- `README.md` に書かれた placeholder boundary はテストで強制されていません。たとえば、具体的な設計なしに runtime lifecycle authority が誤ってここへ追加されることを防ぐ自動 guard はありません。 +- `Cargo.toml` は `manifest`、`protocol`、`tokio` への依存を宣言していますが、crate にはそれらを使うコードがありません。テストからは、これらの依存が意図的な scaffolding なのか、古い placeholder baggage なのかは分かりません。 +- 実装が存在しないため、テスト妥当性は「空 crate がコンパイルできる」以上にはほとんど評価できません。 + +## 追加を提案するテスト +- `daemon` に具体的な責務が生まれるまでは、behavioral test は追加しないでください。 +- crate をしばらく placeholder のままにするなら、予期しない public API や非空の runtime 実装を検出する軽量な policy/lint-style check を crate 外に置くことは考えられます。ただし任意であり、過剰かもしれません。 +- daemon 機能を導入するときは、実装詳細ではなく実際の invariant に対するテストを追加してください。 + - long-lived Pod management の lifecycle state transition + - persistence / recovery behavior + - shutdown、cancellation、restart semantics + - `daemon`、`pod`、`pod-registry`、`yoi`、`tui` の authority boundary + - socket/path handling と failure case + - 該当する場合は manifest/profile-derived configuration との interaction + +## 実行したコマンド +```sh +cd /home/hare/Projects/yoi && cargo test -p daemon +``` + +結果: 成功。`daemon` をビルドし、`0` unit tests と `0` doc tests を実行しました。 diff --git a/docs/report/test-validity-20260612/lint-common.md b/docs/report/test-validity-20260612/lint-common.md new file mode 100644 index 00000000..5ddc72af --- /dev/null +++ b/docs/report/test-validity-20260612/lint-common.md @@ -0,0 +1,75 @@ +# テスト妥当性レビュー: lint-common +- 判定: 混在 + +## 確認範囲 +- 対象 crate: `crates/lint-common` +- 読んだ主なファイル: + - `crates/lint-common/src/lib.rs` + - `crates/lint-common/src/slug.rs` + - `crates/lint-common/src/frontmatter.rs` + - `crates/lint-common/README.md` +- 軽く確認した利用側: + - `crates/memory` + - `crates/workflow` + +## 現在のテストがよくカバーしていること +- `Slug` の基本的な受理/拒否ケースは押さえている。 + - 1文字、複数文字、数字、ハイフンを含む正常系。 + - 空文字、先頭/末尾ハイフン、大文字、underscore、空白、非 ASCII、連続ハイフンなどの異常系。 + - 最大長 64 文字と 65 文字拒否の境界。 +- `Slug` の serde deserialize 経路も最低限確認されている。 + - `Slug::parse` だけでなく、frontmatter/schema 側で効く deserialize validation の基礎確認として妥当。 +- `split_frontmatter` は最小限の正常系/異常系を持っている。 + - 通常の `---\n...\n---\nbody`。 + - opening delimiter 不在。 + - closing delimiter 不在。 + - 空 body。 +- テストは小さい crate の public behavior に近く、過度に内部実装へ寄ってはいない。 + +## 不足 / 疑問のあるテスト +- `split_frontmatter` の opening delimiter 境界が弱い。コメント上は document が `---\n` で始まる前提だが、現在のテストは「`---` が独立行であること」を検証していない。 + - 例: `---foo\n---\nbody` のような入力を拒否すべきかがテストで固定されていない。 + - これは frontmatter parser の中核 invariant なので、テスト不足として重要。 +- closing delimiter の EOF ケースが未確認。 + - コメントでは closing `---` at EOF を許すように読めるが、`---\nfoo: 1\n---` のようなケースがテストされていない。 +- CRLF の扱いが曖昧なままテストされていない。 + - closing delimiter 側は `\r` を落としているが、opening delimiter 側は `---\r\n` を明示的に扱っていない。 + - Windows 改行をサポート対象にするなら不足。サポート外なら拒否テストがあるとよい。 +- `Slug` の仕様表示とテストが少し噛み合っていない可能性がある。 + - `lib.rs` の error message と `slug.rs` のコメントにある regex `^[a-z0-9](?:[a-z0-9-]{0,62}[a-z0-9])?$` は `foo--bar` を許す形に見える。 + - 一方、実装とテストは `foo--bar` を拒否している。 + - 連続ハイフン禁止が正なら、テスト自体は妥当だが、テストが依拠する authoritative spec がコメント/エラー文とずれている。 +- `Slug` の public API 周辺の確認がやや薄い。 + - `Display`, `AsRef`, `FromStr`, `into_string`, serde serialize は未テスト。 + - 重大ではないが、この crate が共通 primitive を提供する責務なら薄め。 +- property/fuzz 的なテストはない。 + - 現状の実装サイズでは必須ではないが、slug validator は文字種/長さ/ハイフン位置の組み合わせが多いため、簡単な table-driven 境界テストを増やす価値はある。 + +## 追加候補 +- `split_frontmatter` の delimiter 厳密性テストを追加する。 + - `---foo\n---\nbody` は拒否。 + - `------\nbody` は拒否。 + - opening delimiter はファイル先頭の独立行でなければならない、という仕様を固定する。 +- EOF closing delimiter の正常系を追加する。 + - `---\nfoo: 1\n---` が成功し、body が空になること。 +- CRLF の方針をテストで固定する。 + - サポートするなら `---\r\nfoo: 1\r\n---\r\nbody`。 + - サポートしないなら明示的に error になること。 +- slug の仕様とテストを同期させる。 + - 連続 `--` 禁止が正なら、error message / regex コメントもそれに合わせる。 + - そのうえで `a--b`, `a--`, `--a` など、連続ハイフンと端点ハイフンの境界を分けた test case を置く。 +- `Slug` の変換 API の薄い smoke test を追加する。 + - `FromStr` + - `Display` + - `AsRef` + - `into_string` + - serde serialize roundtrip +- 可能なら `is_valid_slug` と `Slug::parse` の結果が常に一致する table test を、正常/異常すべてのケースで明示する。 + - 現在も一部では確認しているが、全 case を共通 helper にすると漏れが減る。 + +## 実行したコマンド +- `cargo test -p lint-common` + - 結果: pass + - 8 unit tests passed. + - 0 doc tests. + diff --git a/docs/report/test-validity-20260612/llm-worker-macros.md b/docs/report/test-validity-20260612/llm-worker-macros.md new file mode 100644 index 00000000..60776608 --- /dev/null +++ b/docs/report/test-validity-20260612/llm-worker-macros.md @@ -0,0 +1,109 @@ +# テスト妥当性レビュー: llm-worker-macros +- 評価: 混在 + +## 確認範囲 + +- 対象 crate: `crates/llm-worker-macros` +- 読んだ主なファイル: + - `crates/llm-worker-macros/src/lib.rs` + - `crates/llm-worker-macros/Cargo.toml` + - `crates/llm-worker-macros/README.md` + - `crates/llm-worker/tests/tool_macro_test.rs` +- 変更はしていない。 + +`llm-worker-macros` 自体にはローカル unit/integration test がなく、`cargo test -p llm-worker-macros` では実質的に挙動を検証していない。一方で、下流 crate の `llm-worker` 側に `#[tool_registry]` / `#[tool]` の実利用 integration test があり、主要な happy path と一部 error path は確認されている。 + +## 現在のテストがよくカバーしていること + +`crates/llm-worker/tests/tool_macro_test.rs` は、この proc macro が実際に `llm_worker::tool::Tool` と組み合わさって動くことを確認しており、単なる helper unit test より有用な面がある。 + +特に以下はよく押さえられている。 + +- `#[tool_registry]` が `*_definition()` を生成し、`ToolMeta` と `Tool` instance を返せること。 +- tool 名が method 名由来になること。 +- doc comment が `ToolMeta.description` に入ること。 +- JSON 引数が deserialize され、method に渡されること。 +- 複数引数の tool が動くこと。 +- 引数なし tool が `{}` で実行できること。 +- 不正な引数、少なくとも必須 field 欠落が error になること。 +- `Result` 返却の success/error mapping が動くこと。 +- async method と sync method の両方が wrapper 経由で実行できること。 +- `ToolExecutionContext` typed parameter が JSON schema/input からではなく実行時 context から渡されること。 +- definition factory を複数回呼んでも `ToolMeta` が安定していること。 + +この crate の中心責務は「compile-time macro expansion による runtime tool definition 生成」なので、下流 crate で実 macro expansion を通している点は妥当。 + +## 不足・疑問のあるテスト + +- `llm-worker-macros` package 自身の test は 0 件。`cargo test -p llm-worker-macros` 単体では回帰検知力がほぼない。 +- proc macro crate として重要な compile-pass / compile-fail matrix がない。現在は runtime integration test 中で使われる形だけが通っている。 +- `#[description = "..."]` parameter attribute の挙動が未検証。実装は `#[schemars(description = ...)]` に変換するが、schema に description が出るか、parameter attribute として期待通り扱えるかをテストしていない。 +- JSON Schema の検証が弱い。`properties` の存在だけを見ており、以下を確認していない。 + - field 名 + - `required` + - field type + - `ToolExecutionContext` が schema から除外されること + - `#[description]` が schema に反映されること +- 不正引数 test が error variant や message を確認していない。`ToolError::InvalidArgument` であることまで見るべき。 +- 引数なし tool は invalid JSON も `unwrap_or` で通す実装になっているが、この仕様が意図通りかを固定する test がない。現在の test は `{}` のみ。 +- macro expansion が参照する外部 path/import の契約が未検証。生成コードは `serde`, `schemars`, `serde_json`, `async_trait`, `::llm_worker` に依存しているため、利用側 crate で何を明示 import/dependency すべきかを compile test で固定した方がよい。 +- unsupported input の扱いが未検証。 + - destructuring pattern 引数 + - `self` 以外の receiver 形 + - generic method + - generic impl / lifetime 付き impl + - `Result` alias や path-qualified result + - `ToolExecutionContext` の path-qualified type / alias +- negative compile tests がないため、「サポート外 syntax が分かりやすく失敗するか」は不明。 +- assert が `summary.contains(...)` に寄りがちで、`Debug` formatting 前提の出力仕様を明確に固定していない。脆すぎるわけではないが、macro の期待仕様としてはやや曖昧。 +- doctest はすべて `ignore` なので、README/API example の鮮度は保証していない。 + +## 追加の提案 + +- `trybuild` による compile-pass tests を追加する。 + - 最小構成の consuming crate で `#[tool_registry]` が展開できること。 + - async/sync/Result/no-arg/multi-arg/context-injected/description-attribute の pass cases。 + - 必要な dependency/import 契約が明確になる fixture。 +- `trybuild` による compile-fail tests を追加する。 + - destructuring pattern 引数。 + - unsupported receiver/method form。 + - `Clone` できない context。 + - 期待していない attribute placement。 + - サポート外 generic form を意図的に禁止するなら、その diagnostic。 +- schema の exact-ish assertions を追加する。 + - `properties.message` 等の field 名。 + - `required`。 + - numeric/string type。 + - `ToolExecutionContext` が schema に出ないこと。 + - `#[description = "..."]` が field schema に反映されること。 +- error path の assertion を強くする。 + - invalid JSON syntax。 + - missing required field。 + - wrong type。 + - それぞれ `ToolError::InvalidArgument` になること。 +- 引数なし tool の invalid input 仕様を明文化して test する。現在の「parse 失敗でも空 args として続行」はかなり permissive なので、意図した仕様なら固定し、意図しないなら変更前提の regression test を作る。 +- `llm-worker-macros` crate 自身にも、少なくとも compile test harness を置くか、下流 `llm-worker` 側の macro tests がこの crate の実質テストであることを明示する。 + +## 実行したコマンド + +```sh +cd /home/hare/Projects/yoi && cargo test -p llm-worker-macros +``` + +結果: + +- 成功。 +- Unit tests: `0 passed`。 +- Doctests: `0 passed`, `2 ignored`。 +- この crate 単体の test 妥当性評価としては、挙動検証はほぼない。 + +```sh +cd /home/hare/Projects/yoi && cargo test -p llm-worker --test tool_macro_test +``` + +結果: + +- 成功。 +- `9 passed`。 +- `llm-worker` 側の integration test としては、macro の主要 runtime behavior をある程度検証できている。 + diff --git a/docs/report/test-validity-20260612/llm-worker.md b/docs/report/test-validity-20260612/llm-worker.md new file mode 100644 index 00000000..f54cfba0 --- /dev/null +++ b/docs/report/test-validity-20260612/llm-worker.md @@ -0,0 +1,80 @@ +# テスト妥当性レビュー: llm-worker +- 評価: 概ね良い + +## 確認範囲 +- Workspace root: `/home/hare/Projects/yoi` +- Crate/package: `crates/llm-worker` / `llm-worker` +- 読んだ範囲: + - `crates/llm-worker/Cargo.toml` + - `crates/llm-worker/src/**` + - `crates/llm-worker/tests/**` + - `crates/llm-worker/docs/{requirements,architecture}.md` +- ソースファイル、Ticket、設定ファイルは変更していない。 + +## 現在のテストがよくカバーしていること +- Provider adapter 周りの単体テストはかなり厚い。 + - Anthropic / Gemini / OpenAI Chat / OpenAI Responses の request projection、SSE event parsing、tool call/result、usage、reasoning metadata、prompt cache 関連が細かく検証されている。 + - OpenAI Responses は未知 event/error の保持、reasoning item の round-trip、encrypted content、stateless ZDR default など、最近壊れやすい仕様面もテストされている。 +- Worker の主要な in-process 挙動は integration test で押さえられている。 + - text response、tool call、tool call collector、history update、locked/mutable state transition、system prompt preservation、turn count、tool registration など。 + - `trybuild` による compile-fail test があり、typestate 制約の一部を型レベルで検証している点は良い。 +- Callback / hook / parallel tool execution のテストがある。 + - text/tool/usage/turn/retry callback、pre/post tool hook、synthetic/skip path、execution context の order / batch id が検証されている。 +- Provider fixture smoke test がある。 + - Anthropic / Gemini / Ollama / OpenAI fixture を deserialize し、timeline と usage token の最低限の整合を見る構成になっている。 +- Transport/retry 境界にもテストがある。 + - retryable/non-retryable status、`Retry-After` preservation、mid-stream SSE error、response timeout retryability、Codex backend header/compression が検証されている。 +- prune / token counter / tool server / truncation など、Worker 周辺の重要な小さい invariant も単体テスト化されている。 + +## 不足 / 疑問のあるテスト +- Core requirement に見える Pause/Resume 系の実行テストが不足している。 + - `PreToolAction::Pause` / `TurnEndAction::Pause`、pending tool call の保持、`resume()` 後の再開、resume 時に余計な user message を積まないこと、複数 tool call batch の途中停止などは、少なくとも現行テスト名からは直接確認できなかった。 + - typestate や history accumulation は見ているが、「pause を跨いだ Worker の安全性」は別の重要 invariant。 +- Cancel / interruption / continuation の安全性テストが薄い。 + - `cancel()` は doc example と API として存在するが、stream 中 cancel、tool 実行前後 cancel、partial assistant content の扱い、未完成 tool JSON/reasoning fragment を history に残さないことのような Worker として重要な保証がテストで明確に固定されていない。 + - stream-started failure 後の continuation / retry path も、単体の timeout/probe test はあるが、history-aware continuation の最終状態を integration で確認するテストが欲しい。 +- 一部 integration test が `worker.run(...).await` の戻り値を捨てており、後段で限定的な副作用だけを見る形になっている。 + - 意図的に error path を見ている箇所以外では、`expect` または error 内容の明示 assert にした方が、run 後半の失敗を取り逃がしにくい。 +- Fixture tests は smoke としては有用だが、契約テストとしては緩い箇所がある。 + - `common` 側で usage や timeline が不足した場合に warning 寄りの扱いになっている部分があり、provider-specific contract を固定するには弱い。 + - provider ごとの差異を許す helper は便利だが、重要な event sequence は fixture ごとにもう少し strict にしてよい。 +- Parallel execution test に wall-clock threshold 依存がある。 + - 現状は短時間 sleep ベースで並列性を確認しているため、遅い/混雑した CI では flaky になり得る。barrier と atomic max-concurrency などで「同時に走った」ことを直接見る方が堅い。 +- Doc tests は 24 件 list されるが、すべて `ignore` 扱い。 + - API 使用例の維持には役立ちにくい。外部 API surface が重要な crate なので、可能なものは `no_run` または compile-only doctest に寄せる価値がある。 +- 実 provider との E2E はない。 + - crate 単体の妥当性評価としては必須ではないが、provider schema drift を検知する意味では fixture 更新だけでは限界がある。 + +## 追加を提案するテスト +- Pause/Resume integration tests を追加する。 + - pre-tool pause → pending tool call 確認 → resume → tool result commit → assistant continuation。 + - turn-end pause → history に assistant output が残る → resume で次 request が正しく構築される。 + - 複数 tool calls のうち一部/全部を pause した場合の order、batch id、history item の不変条件。 +- Cancel/interruption tests を追加する。 + - stream 中 cancel で partial text/reasoning/tool call fragment が安全に扱われること。 + - tool 実行中 cancel の結果が history/callback/result にどう反映されるか。 + - incomplete tool JSON と reasoning fragment が次 turn の request に混入しないこと。 +- Continuation/retry の Worker-level integration を追加する。 + - stream-started 後の transport error → continuation notice callback → safe partial history commit → retry/continue request の形。 + - continuation limit 到達時の error と history post-condition。 +- `worker.run` の戻り値を捨てている既存テストを見直す。 + - 正常系は `expect("worker run succeeds")`。 + - 異常系は error kind/message と、その時点までに許される副作用を明示 assert。 +- Fixture tests を provider-specific に少し強める。 + - usage が必須の fixture では warning ではなく assert。 + - event sequence の必須 block / tool / usage / reasoning metadata を fixture ごとに固定。 +- Parallel execution test の時間依存を下げる。 + - sleep duration で速さを見るより、barrier + concurrent counter で最大同時実行数を assert する。 +- 主要 public examples を `ignore` から compile-only doctest に移す。 + - 実行不能な外部 call は `no_run`、擬似コードは明確に `text` fence などに分ける。 + +## 実行したコマンド +- `cargo test -p llm-worker` + - Result: passed. +- `cargo test -p llm-worker -- --list` + - Result: passed/listed. + - Executable tests listed: unit/integration/trybuild 全体で 237 件。 + - Doc tests listed: 24 件、すべて ignored。 +- `cargo test -p llm-worker --quiet` + - Result: passed. + diff --git a/docs/report/test-validity-20260612/manifest.md b/docs/report/test-validity-20260612/manifest.md new file mode 100644 index 00000000..2c150fb4 --- /dev/null +++ b/docs/report/test-validity-20260612/manifest.md @@ -0,0 +1,79 @@ +# テスト妥当性レビュー: manifest +- 評価: 概ね良い + +## 確認範囲 +- Workspace root: `/home/hare/Projects/yoi` +- Crate/package: `manifest` +- 読み取り専用のレビューのみを行い、ソースファイルと Ticket は変更していない。 +- 主な責務を以下で確認した: + - `crates/manifest/src/lib.rs` + - `crates/manifest/src/config.rs` + - `crates/manifest/src/scope.rs` + - `crates/manifest/src/profile.rs` + - `crates/manifest/src/paths.rs` + - `crates/manifest/src/model.rs` + - `crates/manifest/src/defaults.rs` +- `cargo test -p manifest` を実行: passed、`129 passed; 0 failed`。 + +## 現在のテストがよくカバーしていること +- この crate には、中核責務である manifest/profile 設定の parse、merge、validate、resolve に対する強い unit-test suite がある。 +- `config.rs` のテストは、多くの重要な manifest invariant をカバーしている: + - 最小限の TOML parse と defaulting; + - model/tool/web/memory/compaction/reasoning fields; + - TOML type mismatch を hard error として扱うこと; + - partial config cascade と upper-layer override behavior; + - deprecated/removed fields が reject されること; + - authority-bearing fields における invalid relative paths; + - feature-flag behavior、特に shell/file/web/memory/tool areas の無効化; + - prompt/resources/workflow directories と generated memory settings。 +- `scope.rs` のテストは、セキュリティ上もっとも重要な path permission behavior をカバーしている: + - recursive grants と non-recursive grants; + - read/write permission matching; + - deny entries が grants を override すること; + - path traversal rejection; + - canonicalization/relative-path normalization; + - delegation subset checks; + - shared-scope add/remove/clear operations。 +- `profile.rs` は、Lua profile behavior について特に広いカバレッジを持つ: + - 意図された `yoi.profile` API 経由の profile registration; + - legacy/global APIs の明示的な rejection; + - builtin role profile defaults と tool-policy boundaries; + - workspace/profile override precedence; + - `extend`、helper APIs、TOML loading、model catalog access、Lua module loading; + - multiple-profile discovery、selector handling、ambiguity、exact/unique match behavior。 +- `paths.rs` のテストは、pure resolver functions によって XDG/Yoi directory precedence rules をカバーしており、global environment races を避けている。 +- いくつかのテストは、単なる実装機構ではなく、重要な product decisions を regression checks として符号化している。manifest/profile semantics は durable contract なので、この crate ではそれが適切である。 + +## 不足 / 疑問のあるテスト +- カバレッジはほぼすべて in-module unit tests である。これは概ね適切だが、downstream crates や CLI startup の観点からの integration-style coverage は少ない。変更によってこれらの local unit tests は維持されても、`yoi`、`pod`、または profile loading が manifest API を compose する方法が壊れる可能性がある。 +- Path-scope security tests は良いが、adversarial filesystem boundaries に対してはまだ網羅的ではない。workspace 内の symlink が外を指すケース、workspace 外の symlink が中を指すケース、または canonicalized symlink paths を通じた mixed deny/grant interactions に対する専用テストは見当たらなかった。この crate の authority role を考えると、これが主な残リスクである。 +- Delegation subset tests は基本的な matrix をカバーしているが、より明示的な negative cases があるとよい: + - read-only grant は write delegation を満たしてはならない; + - non-recursive parent grant は child delegation を満たしてはならない; + - deny entries は direct access を制約するのと同じように delegation を制約するべきである; + - 同一 path 上の mixed read/write grants は order-independent のままであるべきである。 +- 一部の default/builtin profile tests は snapshot-like で、詳細な role policy を assert している。これらは価値があるが、builtin role contracts が意図的に進化すると脆くなりうる。この脆さは、これらを policy-contract tests として扱うなら許容できる。そうでなければ、“must never regress” invariants と “current default snapshot” fixtures に分けるべきである。 +- Model/auth schema coverage は manifest/profile coverage より薄い。`ModelManifest::merge` と `AuthRef` variants は単純だが、`codex_oauth`、`secret_ref`、context-window/max-context-window precedence の round-trip tests を追加すると regression risk を下げられる。 +- Compact-ratio behavior には known model context lookup を含む positive coverage があるが、以下についてより明確な negative tests があるべきである: + - model context がなく、unknown model ref である場合の ratio; + - `context_window` が `max_context_window` より明示的に大きい場合; + - それらを reject する意図があるなら、invalid/edge ratios。 +- Public path helpers は、exported environment-reading functions ではなく pure resolver helpers を通じてテストされている。これは determinism のためには良いが、ごく小さな serial/env-scoped smoke test があれば public functions の配線ミスを検出できる。 +- README/doctest coverage は実質的に存在しない（doctests は `running 0 tests`）。これは config crate にとって大きな問題ではないが、README examples が public contract の一部になるなら、実行可能にするか、テストでミラーするべきである。 + +## 追加を提案するもの +- private helpers だけをテストするのではなく、public API 経由で代表的な manifest/profile fixtures を load する小さな integration test を `crates/manifest/tests/` 配下に追加する。 +- `Scope` に symlink boundary tests を追加する: + - allowed root 内の symlink が外を指す場合は deny されるべきである; + - 外側の symlink が内側を指す場合は canonical target policy に従って振る舞うべきである; + - symlink を含む deny paths は、normalization/canonicalization 後も grants を override するべきである。 +- permission、recursion、deny、path ancestry の組み合わせをカバーする delegation matrix test table を追加する。 +- サポートされているすべての auth variants と context-window precedence について model/auth round-trip tests を追加する。 +- missing/unknown context と invalid edge values 周辺の negative compact-ratio tests を追加する。 +- snapshot-like builtin profile tests を policy-contract tests としてラベル付けするか、期待値を compact fixtures に移して、意図的な role-policy changes をレビューしやすくすることを検討する。 + +## 実行したコマンド +- `cargo test -p manifest` + - Result: passed + - Summary: `129 passed; 0 failed; 0 ignored; finished in 0.00s` + diff --git a/docs/report/test-validity-20260612/memory.md b/docs/report/test-validity-20260612/memory.md new file mode 100644 index 00000000..9db2eec9 --- /dev/null +++ b/docs/report/test-validity-20260612/memory.md @@ -0,0 +1,85 @@ +# テスト妥当性レビュー: memory +- 判定: 概ね良い + +## 確認範囲 +- 対象 crate: `crates/memory` / パッケージ `memory` +- 読んだ主な範囲: + - `crates/memory/README.md` + - `src/lib.rs` + - `src/workspace.rs` + - `src/linter/*` + - `src/schema/*` + - `src/tool/{read,write,edit,delete,query}.rs` + - `src/extract/*` + - `src/consolidate/*` + - `src/resident.rs` + - `src/usage.rs` + - `src/audit.rs` +- 変更は行っていない。 + +## 現在のテストがよくカバーしていること +- crate の責務に沿った主要な pure / filesystem-local invariants はかなり広く押さえられている。 + - `.yoi/memory` / `.yoi/knowledge` の path classification、opaque subtree (`_staging`, `_usage`, `_logs`) の除外、invalid slug / nested path reject。 + - workspace root resolution で `.yoi` project records だけを memory marker と見なさない挙動。 + - linter の frontmatter 必須 field、`replaced_by` の存在確認・self reference、body size、Knowledge `model_invokation` description cap、same slug create reject、similar slug warning。 + - `MemoryRead` / `MemoryWrite` / `MemoryEdit` / `MemoryDelete` / `MemoryQuery` / `KnowledgeQuery` の基本成功・基本失敗・slug rule・workflow kind 非公開。 + - query の list/search、case-insensitive search、excerpt context、result limit、Knowledge kind filter、frontmatter search、query が usage event を増やさないこと。 + - resident summary / resident knowledge collection の missing/malformed/empty/並び順/`model_invokation` filter。 + - extract staging, extract pointer fold, extract input rendering で tool result content や reasoning を落とすこと。 + - consolidation staging list, invalid staging count, lock acquire/release/stale handling, tidy hints, consolidate prompt sections。 + - usage event aggregation で explicit use と resident exposure を分けること。 + - audit JSONL append、record snapshot diff count、hash format。 +- テスト数も十分あり、`cargo test -p memory` は 108 tests + doc-tests 0 で成功している。 +- 大半が temporary directory を使う unit/integration-light test なので、crate 単体の filesystem behavior を検証する形として妥当。 + +## 不足 / 疑問のあるテスト +- full flow の結合テストは薄い。 + - extract payload → staging → consolidation input → memory tools write/edit/delete → audit/usage までの一連の流れは、個別部品ごとにはあるが、run-level の不変条件としては検証されていない。 + - Pod / Worker 経由の real tool registry や permission scope との接続はこの crate 単体ではほぼ未検証。 +- linter の網羅性に穴がある。 + - `InvalidStatus`、unknown/extra frontmatter fields、malformed date、`sources` / `last_sources` の shape、request / knowledge / summary の必須 field failure が体系的には確認されていない。 + - `replaced_by` cycle detection の実シナリオは弱い。`references.rs` の test は unknown reference で 1 error になる smoke に近く、既存 A→B / B→A のような cycle report を明確には固定していない。 + - `LowImportanceLargeRecord` と `SourcesOverflow` warning は tidy 側では一部見ているが、linter warning と tool output/audit への伝播としては薄い。 +- tool の edge case が不足している。 + - `MemoryRead` の `offset` / `limit` / truncation summary、limit=0 の `.max(1)` 挙動、空ファイル・末尾改行なしの line numbering が未検証。 + - `MemoryEdit` の `replace_all`, duplicate old_string reject, old_string empty, identical replacement, non-UTF-8 file, summary/knowledge edit path が未検証。 + - `MemoryWrite` の invalid slug、summary with slug、Knowledge/Request 作成、write success audit contents、warning summary/audit reason が未検証。 + - `MemoryDelete` は成功 path のみで、missing file、summary slug forbidden、invalid slug、workflow kind reject、audit failure record が未検証。 +- 誤解を招く / 弱い test がある。 + - `write_aggregates_multiple_errors` は名前に反して、実際の assertion は `status` / `missing` の substring だけで、body too long など複数 error aggregation を確認していない。現実の linter は frontmatter parse failure で早期 return するため、この test は「複数 error が集約される」保証になっていない。 + - 一部の assertion は `summary.contains("Created")` や error message substring など、契約の中核ではなく表示文言寄り。大きな脆さではないが、重要な audit/hash/file-content invariant を直接見る方が強い箇所がある。 +- filesystem failure / concurrency はほぼ未検証。 + - write/edit/delete の permission error、directory/file collision、partial write、外部同時変更は現状未カバー。 + - consolidation lock は live pid / stale pid / cleanup は見ているが、corrupt lock overwrite や `release_only` は明示テストがない。 +- query / resident の malformed handling はある程度あるが、KnowledgeQuery の kind filter 時に malformed frontmatter を skip する仕様、malformed でも query だけなら body hit できる仕様は直接の regression test があるとよい。 +- schema strictness が仕様なら危険。 + - `frontmatter::deserialize_strict` という名前だが、schema structs 側に `deny_unknown_fields` が見当たらず、unknown field reject の test もない。extra field を許す設計なら問題ないが、「strict」を期待するならテスト・実装とも不足。 + +## 追加を提案するテスト +- 高優先度: + - `write_aggregates_multiple_errors` を実際に複数 lint error を確認する test に直すか、名前を現実に合わせる。 + - `replaced_by` cycle の具体例を追加する: existing `a -> b`, `b -> c` に対して `c -> a`、または existing cycle を含む場合の `ReplacedByCycle`。 + - `MemoryEdit` の `replace_all=false` duplicate reject / `replace_all=true` multi replace / rollback after lint failure / audit failure record を追加。 + - `MemoryRead` の `offset` / `limit` / truncation / limit=0 を追加。 + - `MemoryDelete` の missing file・summary slug forbidden・invalid slug・workflow kind reject を追加。 +- 中優先度: + - linter の invalid status、malformed timestamps、request/knowledge/summary の必須 field、Knowledge `last_sources` malformed、warning propagation を追加。 + - KnowledgeQuery の malformed frontmatter behavior: kind filter では skip、query-only では body match 可能、という仕様を固定。 + - write/edit/delete/read の audit log JSON を success/failure それぞれで軽く確認する。 + - consolidation lock の corrupt lock overwrite と `release_only` の staging preservation を追加。 +- 低〜中優先度: + - extract/consolidate の miniature end-to-end test を 1 本追加する。実 Worker までは不要でも、`write_staging` → `list_staging_entries` → `build_consolidate_input` → tool write/edit の組み合わせで主要データ形状を固定できる。 + - unknown frontmatter fields を許す/拒否する方針を決め、方針に応じた test を追加する。 + - non-UTF-8 / directory collision / permission failure は OS 依存を避けつつ、可能な範囲で error path regression を足す。 + +## 実行したコマンド +```text +cargo test -p memory +``` +結果: 成功。108 passed, 0 failed, doc-tests 0。 + +```text +cargo test -p memory -- --list +``` +結果: 成功。108 tests listed, doc-tests 0。 + diff --git a/docs/report/test-validity-20260612/pod-registry.md b/docs/report/test-validity-20260612/pod-registry.md new file mode 100644 index 00000000..65dac24f --- /dev/null +++ b/docs/report/test-validity-20260612/pod-registry.md @@ -0,0 +1,95 @@ +# テスト妥当性レビュー: pod-registry +- 判定: 概ね良い + +## 確認した範囲 + +- `crates/pod-registry/README.md` +- `crates/pod-registry/Cargo.toml` +- `crates/pod-registry/src/lib.rs` +- `crates/pod-registry/src/table.rs` +- `crates/pod-registry/src/conflict.rs` +- `crates/pod-registry/src/mutate.rs` +- `crates/pod-registry/src/lifecycle.rs` +- `crates/pod-registry/src/error.rs` +- `crates/pod-registry/src/test_util.rs` + +`pod-registry` の責務は、machine-local な live Pod allocation、Pod 名衝突、write-scope / delegation-scope の衝突検出、stale allocation の回収、segment writer の重複防止を `pods.json` と file lock で調整すること。現状のテストは crate 内 unit test のみで、integration test / cross-process test はありません。 + +## 現在のテストがよくカバーしていること + +- 中核的な mutation invariants はよくカバーされています: + - top-level registration が write-scope conflicts を拒否すること + - duplicate Pod name の拒否 + - read-only scope が write scope と衝突しないこと + - release が allocation を削除し、scope を再度開放すること + - child release / stale reclaim が descendants を reparent すること + - delegated scope が release 後に parent へ戻ること + - `reclaim_delegated_scope` が存在する/存在しない child allocation を扱い、一致する deny layer を 1 つだけ削除すること +- Delegation behavior は意味のある形でテストされています: + - delegated scope が明示的な `DelegationScope` の subset でなければならないこと + - delegation が parent の直接の current effective write set ではなく `DelegationScope` を使うこと + - sibling overlap が拒否されること + - effective parent write scope が delegated child regions を除外すること +- Conflict-owner behavior は有用な semantic boundaries でテストされています: + - recursive prefix overlap + - non-recursive direct-child behavior + - 最も深い delegated owner が competitor として報告されること + - restored-parent deny rules が fully denied regions は抑制できるが、partial-deny parent conflicts は抑制しないこと +- Segment-lock behavior は基本的にカバーされています: + - `register_pod` が重複する `segment_id` を拒否すること + - `update_segment` が segment ownership を書き換えること + - `update_segment` が別の Pod にすでに保持されている target segment を拒否すること + - `find_by_segment` が `segment_id = None` の pre-adoption placeholder allocations を無視すること + - `lookup_segment` が live writer info を返し、guard release 後には `None` を返すこと +- File-backed table behavior には有用な smoke coverage があります: + - lock file creation + - `0600` file / `0700` directory permissions + - save/reopen roundtrip + - RAII `ScopeAllocationGuard` が drop 時に release すること +- Test helpers のスコープは妥当です。`reclaim_stale_with` は liveness の決定論的な seam を提供し、`RuntimeDirSandbox` はこの crate 内で env-var mutation を直列化しています。 + +## ギャップ / 疑問のあるテスト + +- 最大のギャップは cross-process/file-lock tests がないことです。この crate の中心的な safety claim は、`flock` が無関係な Pod spawn sequences を直列化することですが、現状のテストは一度に 1 つの `LockFileGuard` を使う単一プロセスしか exercised していません。lock blocking、concurrent registration 下の atomicity、または 2 つのプロセスが同じ `pods.json` で race したときの behavior を検証していません。 +- `adopt_allocation` には segment-conflict test がありません。`register_pod` と `update_segment` はどちらも別の Pod にすでに保持されている `segment_id` を拒否しますが、`adopt_allocation` は現状、そのようなテストなしに `segment_id` を書き換えます。“one live writer per segment” がすべての allocation entry points に対する invariant であるなら、これは意味のある coverage hole です。 +- Error-path coverage は薄いです: + - 壊れた `pods.json` の parse errors がテストされていない + - unknown-pod errors は `adopt_allocation` ではカバーされているが、`release_pod` / `update_segment` ではカバーされていない + - save failure / permission failure behavior がカバーされていない + - `reclaim_stale` は意図的に save errors を握りつぶしますが、その best-effort behavior は直接テストされていない +- Path-overlap logic は少数の例でしかテストされていません。この behavior は重要かつ微妙ですが、すべての recursive/non-recursive combinations、equal paths、parent/child/grandchild、siblings、read-vs-write、deny coverage cases に対する table-driven matrix がありません。 +- `register_pod_with_deny` には full deny と partial deny behavior のテストがありますが、複数の conflicting owners が同時に存在し、その一部だけが denied されている場合のテストはありません。これはまさに `find_conflict_owners` と `all_denied` semantics が重要になる branch です。 +- `manifest::DelegationScope` および `ScopeRule` normalization との integration boundary は軽くしか exercised されていません。upstream code が canonical absolute paths を保証しているなら問題ありませんが、そうでない場合、この crate には relative paths、`..`、symlinks、trailing separators、または同等だが同一ではない paths に対するテストがありません。 +- Stale reclaim は real `pid_alive` ではなく、注入された liveness predicate 経由でテストされています。これは決定論のためには良いことですが、Unix の `kill(pid, 0)` semantics は、他のテストで current process id を使う間接的な形を除いて未テストのままです。 +- 実際の Pod spawn/restore flows 中にこの registry に依存する caller crates との integration tests がありません。小さな coordination crate としては理解できますが、これはテストが end-to-end runtime safety よりも local invariants を証明していることを意味します。 + +## 追加を推奨するテスト + +- 2 つの child processes または helper binaries から同じ `pods.json` を開く小さな cross-process test を追加し、以下を検証する: + - 2 つ目の writer が最初の lock が release されるまで block すること、または少なくとも stale unlocked state を observe/write できないこと + - 同じ Pod name / scope / segment を claim する concurrent attempts の結果が成功 1 件だけになること +- `adopt_allocation_rejects_segment_id_collision` test を追加する、または adoption がすでに保持されている segment へ overwrite してよい理由を明示的に document し、test する。 +- `rules_overlap` と `covers_fully` の table-driven tests を追加し、以下をカバーする: + - recursive/recursive、recursive/non-recursive、non-recursive/recursive、non-recursive/non-recursive + - equal path、direct child、grandchild、sibling、unrelated path + - `covers_fully` における read/write permission ordering +- 複数 conflict に対する `register_pod_with_deny` tests を追加する: + - all conflicts denied: allowed + - 複数 conflicts のうち 1 つだけ denied: meaningful competitor とともに rejected +- persistence/error tests を追加する: + - invalid JSON が `InvalidData` を返すこと + - save されていない `data_mut` changes が永続化されないこと + - unknown `release_pod` と unknown `update_segment` が `UnknownPod` を返すこと +- 可能であれば Pod spawn/restore caller path の近くに integration-level test を 1 つ追加し、“no two live Pods write the same session segment” という durable invariant に焦点を当てる。 + +## 実行したコマンド + +- `cargo test -p pod-registry` + - 結果: passed + - 29 unit tests passed + - 0 doc tests +- `cargo test -p pod-registry; echo EXIT:$?` + - 結果: passed, `EXIT:0` + - 29 unit tests passed + - 0 doc tests + diff --git a/docs/report/test-validity-20260612/pod-store.md b/docs/report/test-validity-20260612/pod-store.md new file mode 100644 index 00000000..c4c89e7b --- /dev/null +++ b/docs/report/test-validity-20260612/pod-store.md @@ -0,0 +1,116 @@ +# テスト妥当性レビュー: pod-store + +- 評価: 混在 + +## 確認範囲 + +- 対象 crate: `crates/pod-store` +- 主な読解対象: + - `crates/pod-store/src/lib.rs` + - `crates/pod/tests/restore_test.rs` + - `crates/pod/src/discovery.rs` + - `crates/pod/src/spawn/registry.rs` +- 実行した検証: + - `cargo test -p pod-store -- --nocapture` + +`pod-store` は、Pod 名をキーにした永続 metadata を `{pod_state_root}/{pod_name}/metadata.json` に保存する薄い永続化 crate。session log の replay は `session-store` に任せ、ここでは active session/segment pointer、spawned child state、reclaimed child history、peer visibility、resolved manifest snapshot を保持する責務を持つ。 + +## 現在のテストがよくカバーしていること + +既存の 6 unit tests はすべて成功しており、crate の中心的な「metadata の各領域を更新しても他領域を壊さない」という invariant はある程度押さえられている。 + +特に良い点: + +- `resolved_manifest_snapshot` の JSON roundtrip を確認している。 +- `FsPodStore` が session root ではなく pod state root 配下に書くことを確認している。 +- `set_active` が `spawned_children` を保持しつつ active pointer / manifest snapshot を更新することを確認している。 +- `set_spawned_children` が active pointer と manifest snapshot を保持することを確認している。 +- peer 追加が重複を避け、名前順に安定化され、削除できることを確認している。 +- child reclaim が outstanding child から削除し、reclaimed history に記録することを確認している。 + +また、上位 crate 側には `FsPodStore` を使った restore/discovery/communication 系の integration tests があり、`pod-store` 単体ではなく利用経路側でも一部の実運用 invariant は間接的に検証されている。 + +## 不足 / 疑問のあるテスト + +重要な filesystem / schema boundary のテストが不足しているため、durable persistence crate としてはまだ弱い部分がある。 + +- `validate_pod_name` の境界値が直接テストされていない。 + - empty / `.` / `..` / slash / NUL が拒否されること。 + - 通常名が通ること。 + - invalid name で `write`, `read_by_name`, `delete_by_name` が root 外へ出ないこと。 +- `list_names` の仕様が未検証。 + - 名前順に sort されること。 + - `metadata.json` がない directory を無視すること。 + - file entry を無視すること。 + - invalid pod name directory を無視すること。 +- `delete_by_name` が未検証。 + - missing は no-op。 + - metadata file を消す。 + - 空になった pod directory を削除する。 + - 余計なファイルがある場合の挙動を確認するか、仕様として割り切る必要がある。 +- malformed JSON / unknown or partial old metadata の read 挙動が未検証。 + - `serde(default)` による後方互換読み込みはこの crate の重要な性質に見えるが、missing `spawned_children`, `reclaimed_children`, `peers`, `active`, `resolved_manifest_snapshot` の読み込みテストがない。 + - 壊れた JSON が `PodStoreError::Serde` になることも未確認。 +- `CombinedStore` の delegation が未検証。 + - session-store trait methods を inner session store に委譲すること。 + - `PodMetadataStore` methods を inner pod store に委譲すること。 + - `root_dir` が pod store 側 root を返すこと。 +- `update_by_name` の「missing metadata なら作成する」「closure が `pod_name` を変えても requested name に戻す」という contract が直接テストされていない。 +- concurrency / atomicity は未検証。 + - 現実には read-modify-write の `update_by_name` が複数 writer で lost update し得る。crate が単一 writer 前提なら仕様化すべきで、複数 Pod/process から同じ metadata を触る前提ならテスト以前に設計上の保護が必要。 +- `fs_store_writes_under_pod_state_root_only` は historical regression test としては妥当だが、実際の path escape 防止を保証するには invalid name tests が必要。現状では「正しい root に書く」ことは見ているが、「不正名で root 外へ書けない」ことは見ていない。 + +## 追加を提案するテスト + +優先度順に追加するとよいテスト: + +1. pod name validation / path traversal 防止 + - `validate_pod_name` が通常名を受け入れる。 + - `""`, `"."`, `".."`, `"a/b"`, `"a\0b"` を拒否する。 + - invalid name を渡した `FsPodStore::write/read_by_name/delete_by_name` が `InvalidPodName` を返す。 + - root 外にファイルが作られていないことも確認する。 + +2. list behavior + - 複数 metadata を書いて `list_names()` が sorted names を返す。 + - metadata-less directory / normal file / invalid-name directory を無視する。 + +3. delete behavior + - existing metadata を削除できる。 + - missing metadata delete が成功する。 + - delete 後 `read_by_name` が `None`、`list_names` から消える。 + +4. schema compatibility / error behavior + - minimal JSON `{ "pod_name": "agent" }` を手で置いて default fields が空/None になること。 + - malformed JSON が serde error になること。 + - active pointer の `segment_id` omitted が pending segment として読めること。 + +5. update merge contract + - `update_by_name` が missing metadata を作成する。 + - update closure が unrelated fields を壊さない。 + - closure が `metadata.pod_name` を変更しても requested name に正規化される。 + +6. CombinedStore thin-wrapper tests + - pod-store crate 内で軽量に `CombinedStore` を作り、pod metadata と session log operations がそれぞれ正しい backend に流れることを確認する。 + +## 実行したコマンド + +```sh +cd /home/hare/Projects/yoi && cargo test -p pod-store -- --nocapture +``` + +結果: + +```text +running 6 tests +test tests::pod_metadata_manifest_snapshot_roundtrips ... ok +test tests::fs_store_writes_under_pod_state_root_only ... ok +test tests::active_updates_preserve_children_and_manifest_snapshot ... ok +test tests::child_updates_preserve_active_and_manifest_snapshot ... ok +test tests::reclaim_children_removes_outstanding_and_records_history ... ok +test tests::peer_updates_preserve_active_children_and_manifest_snapshot ... ok + +test result: ok. 6 passed; 0 failed; 0 ignored + +Doc-tests pod_store +test result: ok. 0 passed; 0 failed + diff --git a/docs/report/test-validity-20260612/pod.md b/docs/report/test-validity-20260612/pod.md new file mode 100644 index 00000000..09a0e7f9 --- /dev/null +++ b/docs/report/test-validity-20260612/pod.md @@ -0,0 +1,63 @@ +# テスト妥当性レビュー: pod + +- 評価: 混在 + +## 確認した範囲 + +- `crates/pod/Cargo.toml`、`crates/pod/README.md`、`crates/pod/src/lib.rs`、`crates/pod/tests/*.rs` の integration suite、テスト一覧、および選択した失敗中の prompt-resource assertion を確認した。 +- `pod` crate の責務: `llm-worker` を中心とした Pod lifecycle/runtime authority、socket protocol serving、manifest/profile startup、スコープ付き built-in tools、session/pod-store persistence、spawned child Pod orchestration、prompt/resource assembly、compaction/extraction/consolidation、および runtime metadata integration。 +- `cargo test -p pod -- --list` によるテスト一覧: + - 310 件の lib/unit tests。 + - 9 件の integration test target、合計 106 件の integration tests。 + - 0 件の doctests。 + - 一覧上の合計: 416 tests。 + +## 現在のテストがよくカバーしている点 + +- suite は広範で、crate の実際の責務によく対応している。 +- Controller/runtime behavior は意味のある state-machine tests でカバーされている: idle/running transitions、double-run rejection、pause/resume/cancel behavior、empty-turn rollback、provider stream error recording、snapshots、event broadcast、socket method errors、notification handling。 +- Spawned Pod behavior は重要な authority invariants 周辺を強くカバーしている: workspace root と process cwd の区別、explicit delegation scope、parent authority 外の scope rejection、non-recursive scope rejection、omitted cwd behavior、`Run` の送信、予約済み child socket が一度も現れない場合の rollback。 +- Pod communication tools は純粋な mock だけではなく mock Unix sockets に対してテストされている: send/read/stop behavior、initial alert/snapshot draining、already-running errors、unreachable child stop behavior、durable registry restore、scope reclamation が表現されている。 +- Persistence-adjacent behavior は `FsStore`/`FsPodStore` tempdirs でカバーされている: active/pending segment restore、session-log drift auto-forking、metrics のない old sessions、durable spawned-child state、restore rejection cases。 +- Compaction、pruning、extraction、memory consolidation は通常以上に良い invariant coverage を受けている: token split boundaries、tool-call/result pair balancing、threshold behavior、circuit breakers、warning/error events、audit-only skips、lock behavior、staging cleanup、extract pointer reset。 +- Prompt/resource handling は単純な文字列読み込みを超えてテストされている: loader prefix resolution、include semantics、override order、system prompt trailing sections、AGENTS.md injection、memory guidance gating、compaction を跨ぐ runtime prompt materialisation。 +- Feature/plugin-adjacent boundaries はよくカバーされている: declared contributions と host authorities の区別、ticket feature registration と fail-closed cases、task tool replay/reminder behavior、hooks/interceptors、permission matching、fs-view scope filtering、runtime directory/shared-state basics。 + +## 不足 / 疑問のあるテスト + +- suite は現在 red。`cargo test -p pod --no-fail-fast` は 414 passed、2 failed で終了する。どちらの failure も古くなった prompt-prose substring assertions: + - `prompt::catalog::tests::pod_orchestration_guidance_section_renders_resource_body` + - `prompt::system::tests::pod_orchestration_guidance_is_included_for_pod_management_tools` + - どちらも `rendered.contains("worktree status, diff, and test results")` を assert している一方、`resources/prompts/common/pod-orchestration.md` は現在 `worktree state, diff, and test results` と書いている。 +- この 2 件の failing tests は、Rust tests 内で model-facing prose を正確に重複しているため疑問がある。prompt-resource drift は検出できるが、失敗している phrase は stable API invariant ではない。そのため semantic guidance が保たれていても、通常の prompt wording edits で crate が壊れる。 +- 真の end-to-end Pod process smoke test がない。integration tests は mock `LlmClient`、temp stores、mock Unix listeners、injected runtime commands を使っており、速度面では適切だが、実際の `yoi pod`/entrypoint/runtime-dir/socket/session path は全体として exercise されていない。 +- いくつかの async/socket tests はまだ fixed sleeps と process-wide environment mutation (`YOI_RUNTIME_DIR`, `YOI_HOME`, `XDG_RUNTIME_DIR`) に依存している。一部ファイルでは env-changing tests を guards で serialize しているが、fixed timing と global env は高負荷 CI や parallel execution 下で flakiness risk のまま。 +- Real provider wire behavior は意図的に `pod` の外側だが、crate と streaming edge cases の interaction はまだ大部分が mock されている。tests は重要な `Worker` outcomes をカバーしているが、malformed/partial provider streams を real provider adapter 経由では exercise していない。 +- Prompt tests は数が多く有用だが、一部は behavior-coupled というより prose-coupled である。critical safety wording に対して、その文字列を意図的に stable contract として扱う場合だけこれは許容できる。そうでなければ maintenance noise になる。 +- startup profile resolution、socket server、`Method::Run`、session persistence、shutdown、restore を跨ぐ full lifecycle integration は slice ごとにしかカバーされておらず、1 つの scenario としてはカバーされていない。 + +## 追加を提案するもの + +- 古くなった phrase ではなく durable semantic invariant を assert する形で、2 件の failing prompt tests を修正する。たとえば、section が含まれること、child notifications が hints であること、required evidence に `diff` と `test results` が含まれることの anchors は保ちつつ、2 つの Rust tests で文全体を重複しない。 +- まずは ignored または feature-gated でもよいので、最小限の end-to-end smoke test を 1 件追加する。temp `YOI_HOME`/runtime dir で実際の Pod runtime を起動し、socket に接続し、単純な method を送信し、status/events を観測し、session/pod metadata を検証し、clean に shutdown するもの。 +- profile selection と prompt-resource loading 周辺に real startup/restore smoke を追加する。`pod` は manifest/profile/resource resolution が running Pod になる境界を所有しているため。 +- controller/socket tests の fixed sleeps を、bounded `timeout` で包んだ condition-based waits に置き換える。特に status、socket accept、event propagation を待っている箇所。 +- `catalog` と `system` tests が脆い prose anchors を重複しないように、prompt-orchestration guidance assertions を 1 つの helper または snapshot-style semantic check に集約する。 +- simultaneous child reservation や duplicate child name/scope acquisition に対する focused race test を追加する。その invariant が `pod-registry` だけでなく `pod` によって enforce されることが期待される場合。 +- production が current executable path を使う境界で runtime-command resolution failure/override のテストを 1 件検討する。child Pod spawning はその path に運用上敏感なため。 + +## 実行したコマンド + +- `cargo test -p pod -- --list` + - 成功。 + - 310 件の lib tests、106 件の integration tests、0 件の doctests を列挙した。 +- `cargo test -p pod` + - integration targets に到達する前に lib test target で失敗。 + - 失敗時点の結果: 308 passed、2 failed。 +- `cargo test -p pod --no-fail-fast` + - lib target が失敗したため全体として失敗。 + - Lib target: 308 passed、2 failed。 + - Integration targets: 106 passed、0 failed。 + - Doctests: 0。 + - Failing assertions はどちらも `worktree status, diff, and test results` に対する古い exact prompt-prose checks。 + diff --git a/docs/report/test-validity-20260612/project-record.md b/docs/report/test-validity-20260612/project-record.md new file mode 100644 index 00000000..6db1b959 --- /dev/null +++ b/docs/report/test-validity-20260612/project-record.md @@ -0,0 +1,85 @@ +# テスト妥当性レビュー: project-record +- 評価: 概ね良い + +## 確認範囲 + +- Workspace root: `/home/hare/Projects/yoi` +- Crate: `crates/project-record` +- Main source/test file: `crates/project-record/src/lib.rs` +- Crate の責務: + - Unix epoch milliseconds を、path-safe な固定幅13文字の Crockford base32 風 ID に encode/decode する。 + - ID の validation を提供する。 + - collision 時に suffix を付けず、millisecond 値を bounded に increment して未使用 ID を割り当てる。 + - Ticket / Objective などの project record ID 生成・検証の共通基盤になっている。 + +## 現在のテストがよくカバーしていること + +- `encode_unix_epoch_millis` の基本境界を押さえている。 + - `0 -> 0000000000000` + - `31 -> 000000000000Z` + - `32 -> 0000000000010` +- 固定幅 ID の lexicographic order が numeric / chronological order と一致する、というこの crate の重要 invariant を明示的にテストしている。 +- ambiguous / path-unsafe な文字を reject するテストがある。 + - `I`, `L`, `O`, `/` を拒否しており、path-safe ID としての意図は確認できる。 +- decode overflow 相当のケースも一応含まれている。 + - `ZZZZZZZZZZZZZ` は文字種としては valid だが `u64` を超えるため invalid になる。 +- collision allocation の基本動作を押さえている。 + - 既存 ID と衝突したら `base_millis + 1` を使う。 + - 全候補が衝突する場合は `MAX_COLLISION_PROBES` で bounded に失敗する。 + +## 不足 / 疑問のあるテスト + +- encode/decode の round-trip coverage が薄い。 + - `decode_unix_epoch_millis("0000000000010") == 32` 以外は decode の成功系がほぼ確認されていない。 + - `u64::MAX`、現在時刻に近い値、桁境界、複数の代表値で `decode(encode(x)) == x` を見ると、crate の中心 invariant がより強くなる。 +- `decode_digit` の valid alphabet coverage が不足している。 + - 現在の成功系 decode は主に `0`, `1` 周辺に偏っている。 + - `A-H`, `J-K`, `M-N`, `P-T`, `V-Z` の range 分岐が壊れても一部は検出しにくい。 +- overflow 境界が明確にテストされていない。 + - `decode("FZZZZZZZZZZZZ") == u64::MAX` + - `decode("G000000000000")` や `decode("ZZZZZZZZZZZZZ")` が `TimestampOverflow` + のように、13桁 base32 が 65bit 空間であることに由来する境界を分けて確認したい。 +- `rejects_ambiguous_or_path_unsafe_characters` は少し名前と中身が混ざっている。 + - `ZZZZZZZZZZZZZ` は ambiguous/path-unsafe character ではなく numeric overflow なので、invalid character 系と overflow 系は別テストにした方が失敗時の意味が明確になる。 +- `allocate_record_id` の overflow 経路が未テスト。 + - `base_millis == u64::MAX` かつ最初の候補が衝突した場合、次の probe で `TimestampOverflow` になるはず。 + - collision boundedness だけでなく、上限付近の arithmetic safety を確認したい。 +- lowercase policy がテストされていない。 + - Crockford base32 という名前からは lowercase 許容を期待する読者もいるため、この実装が uppercase-only なら `a`, `z` などを reject するテストか、仕様コメントでの明示があると良い。 +- `unix_epoch_millis_now` は実質未テスト。 + - `SystemTime::now()` wrapper なので優先度は低いが、少なくとも「現在の正常環境で `Ok` を返す」程度の smoke test は可能。 + - ただし時刻依存なので、無理に強い assertion を置く必要はない。 + +## 追加を提案するテスト + +- 代表値での round-trip test: + - `0` + - `1` + - `31` + - `32` + - `33` + - `1024` + - 現在時刻近辺の固定値 + - `u64::MAX` +- decode boundary tests: + - `FZZZZZZZZZZZZ` が `u64::MAX` に decode されること。 + - `G000000000000` や `ZZZZZZZZZZZZZ` が `TimestampOverflow` になること。 +- alphabet mapping test: + - `RECORD_ID_ALPHABET` の各文字を下位桁に置いた ID を decode し、index と一致すること。 +- invalid character test の分割: + - ambiguous chars: `I`, `L`, `O` + - lowercase chars: `a`, `z` など、uppercase-only policy を明示する場合 + - path separators / unsafe chars: `/`, `\`, `.`, maybe whitespace + - overflow: valid alphabet だが `u64` 範囲外 +- allocation overflow test: + - `allocate_record_id(u64::MAX, |_| true)` が `TimestampOverflow` になること。 +- allocation probe behavior test: + - `exists` が必要以上に呼ばれないこと、また最初の available candidate で止まることを確認すると、bounded allocation の仕様がより明確になる。 + +## 実行したコマンド + +- `cargo test -p project-record` + - 結果: pass + - 概要: 5 unit tests passed, 0 failed; doc-tests 0 passed / 0 failed. + - 注記: artifact directory の file lock 待ちがあったが、テスト妥当性評価に影響する失敗ではない。 + diff --git a/docs/report/test-validity-20260612/protocol.md b/docs/report/test-validity-20260612/protocol.md new file mode 100644 index 00000000..17f4b4da --- /dev/null +++ b/docs/report/test-validity-20260612/protocol.md @@ -0,0 +1,71 @@ +# テスト妥当性レビュー: protocol + +- 評価: 概ね良い + +## 確認範囲 + +- 対象は `crates/protocol` と解釈した。 +- 読んだ範囲: + - `crates/protocol/README.md` + - `crates/protocol/Cargo.toml` + - `crates/protocol/src/lib.rs` + - `crates/protocol/src/stream.rs` +- 実行した検証: + - `cargo test -p protocol` — pass。unit tests 38 件、doc tests 0 件。 + +## 現在のテストがよくカバーしていること + +`protocol` crate の主責務である「Pod client/server 間の wire protocol 型」をかなり広く検証できている。特に以下は妥当。 + +- `Method` / `Event` の serde tag 形式を、`serde_json::Value` または固定 JSON で確認している。 +- `Method::Run` と `Segment::{Text, Paste}` の roundtrip がある。 +- 未知の `Segment` kind が `Segment::Unknown` に落ちる forward compatibility 要件を直接テストしている。 +- `Notify`, `PodEvent`, completion, snapshot, status, compaction, tool result, LLM retry/continuation など、最近の運用上重要そうな wire shape が個別に押さえられている。 +- `Snapshot` の legacy default (`status`, `context_window`, `context_tokens`) を確認しており、互換性テストとして価値がある。 +- `PodEvent::should_notify_agent()` のような単なる serde ではない分類 invariant もテストされている。 + +この crate はロジックが薄く、ほぼ protocol schema の authority なので、現在のテストの中心が serde shape / roundtrip になっていること自体は適切。 + +## 不足 / 疑問のあるテスト + +- `src/stream.rs` の `JsonLineReader` / `JsonLineWriter` にテストがない。JSONL reader/writer はこの crate の README 上の「JSONL message boundary」に近い責務なので、空行 skip、EOF、invalid JSON の `InvalidData`、writer の newline/flush 相当は最低限押さえたい。 +- `Method` / `Event` の variant coverage は完全ではない。 + - 未確認の例: `Method::Cancel`, `ListRewindTargets`, `RewindTo`, `Shutdown`; `Event::TextDone`, `ToolCallStart`, `ToolCallArgsDelta`, `ToolCallDone`, `Usage`, `Shutdown`, `MemoryWorker`, `RewindTargets`, `RewindApplied` など。 + - protocol crate は variant 追加・rename の影響が大きいため、未テスト variant が増えると wire contract の退行を見逃しやすい。 +- `Segment::flatten_to_text()` と `Method::run_text()` が未テスト。特に `flatten_to_text()` は sigil 復元・貼り付け content・unknown placeholder という明確な仕様を持つので、serde よりも壊れやすい実装ロジックとしてテスト価値が高い。 +- `ScopeRule` / `Permission` の wire shape と `recursive` default は `ScopeSubDelegated` 経由で一部触れているが、`recursive` 省略時の default や `Permission` の lowercase serde は直接確認されていない。 +- 一部テストは roundtrip のみで、期待 JSON 名や payload 値の確認が弱い。 + - 例: `pod_discovery_methods_roundtrip()` は `RestorePod { name }` / `RegisterPeer { name }` の値を assert していない。 + - roundtrip だけだと、同一型内で serialize/deserialize が揃って壊れた場合に検出できないことがある。 +- 固定 JSON 文字列との完全一致テストが一部ある。wire format の固定には有効だが、serde の field order に依存するため、意図しない脆さもある。重要な canonical shape だけに絞り、他は `serde_json::Value` で意味を assert する現在の混合方針は概ね妥当。 + +## 追加提案 + +- `stream.rs` に async unit tests を追加する。 + - writer が JSON + `\n` を出す。 + - reader が空行を skip して次の JSON を読む。 + - EOF で `Ok(None)`。 + - invalid JSON が `io::ErrorKind::InvalidData` になる。 + - writer → reader の小さな end-to-end roundtrip。 +- `Segment::flatten_to_text()` の仕様テストを追加する。 + - `Text`, `Paste`, `FileRef`, `KnowledgeRef`, `WorkflowInvoke`, `Unknown` を混ぜた順序保持。 + - `@`, `#`, `/` の sigil 復元。 +- 未カバーの control/event variant に、少なくとも代表的な pinned wire-shape テストを足す。 + - `Cancel`, `Shutdown`, `RewindTo`, `ListRewindTargets` + - `ToolCall*`, `Usage`, `TextDone`, `RewindTargets`, `RewindApplied`, `MemoryWorker` +- roundtrip だけのテストは、重要 payload を assert する。 + - `RestorePod.name` + - `RegisterPeer.name` + - `PodsListed` / `PodRestored` / `PeerRegistered` は現在 payload equality があるので良い。 +- `ScopeRule` の default と permission rename を単体で確認する。 + - `{"target": "...", "permission": "read"}` で `recursive == true` + - `Permission::Write` が `"write"` になる。 + +## 実行したコマンド + +```sh +cd /home/hare/Projects/yoi && cargo test -p protocol +``` + +結果: pass。38 unit tests passed、0 failed。doc tests は 0 件。 + diff --git a/docs/report/test-validity-20260612/provider.md b/docs/report/test-validity-20260612/provider.md new file mode 100644 index 00000000..e285a6cd --- /dev/null +++ b/docs/report/test-validity-20260612/provider.md @@ -0,0 +1,130 @@ +# テスト妥当性レビュー: provider +- 評価: 概ね良い + +## 確認した範囲 + +- Crate: `crates/provider` +- 確認した主な責務: + - provider/model catalog の読み込みと `ModelManifest` 解決 + - auth 参照から `ResolvedAuth` への解決 + - `LlmClient` 構築エントリポイント + - Codex OAuth の auth.json 解析、token refresh、header 生成、error mapping +- 読んだファイル: + - `crates/provider/Cargo.toml` + - `crates/provider/README.md` + - `crates/provider/src/lib.rs` + - `crates/provider/src/catalog.rs` + - `crates/provider/src/codex_oauth/{mod.rs,auth_json.rs,jwt.rs,refresh.rs,error.rs}` + - `resources/providers/builtin.toml` + - `resources/models/builtin.toml` + +## 現在のテストがよくカバーしていること + +- Catalog resolution は有用な振る舞いレベルでカバーされている。 + - builtin provider/model catalog が parse できる。 + - `ref` 解決が provider catalog と model catalog のフィールドを merge する。 + - inline manifest form が `scheme`、`model_id`、`auth` を要求する。 + - manifest override が catalog default より優先される。 + - unknown provider は hard error になる一方、unknown model は provider default に fallback する。 + - `openrouter/anthropic/claude-sonnet-4.6` のような nested model id がカバーされている。 + - context-window override と backend max clamping がカバーされている。 + +- User catalog override の振る舞いには意味のあるカバレッジがある。 + - `YOI_CONFIG_DIR` は serial tests で保護されている。 + - provider override は存在する場合 builtin より優先される。 + - override が存在しない場合は builtin に fallback する。 + - malformed provider TOML は silent fallback ではなく parse error を返す。 + +- Auth resolution tests は重要な local safety property をカバーしている。 + - `SecretRef` resolution は injectable resolver を使う。 + - missing secret は logical id を露出するが、key に見える material は露出しない。 + - API key file loading は whitespace を trim する。 + - relative auth file path は reject される。 + - missing API key は期待される `ApiKeyMissing` を返す。 + - auth-less Ollama-style construction は成功する。 + +- Codex OAuth tests は local integration として十分に強い。 + - `auth.json` loading は unknown field を保持する。 + - `id_token` JWT からの account id fallback がテストされている。 + - missing auth file は login-oriented error を報告する。 + - persistence は unknown field を保持し、Unix で `0600` を強制する。 + - JWT parsing は `exp`、ChatGPT claims、missing `exp`、malformed JWT をカバーしている。 + - refresh tests は timeout classification と一般的な permanent 401 classifications をカバーしている。 + - provider-level tests は fresh-token headers、FedRAMP header、persistence 付き expired-token refresh、permanent refresh login hint、missing auth file を検証している。 + +## 不足 / 疑問のあるテスト + +- `build_client` / `build_client_from_config` の construction tests は、ほとんどが `is_ok()` または 1 つの error shape を assert している。構築された transport の effective base URL、選択された scheme、model id、capability fallback、Codex-OAuth-specific OpenAI Responses behavior を検証していない。このため、重要な provider-factory invariant のテストが弱い。 + +- Codex OAuth default base URL path は直接テストされていない。`effective_base_url` には `https://chatgpt.com/backend-api/codex` 用の特別な `AuthRef::CodexOAuth` branch があるが、誤って OpenAI Responses scheme default に fallback しても検知するテストがない。 + +- Codex OAuth 用の OpenAI Responses special case は、`max_output_tokens`、`temperature`、`top_p` の forwarding を無効化する。これは重要な compatibility invariant だが、provider crate にはそれに対する focused assertion がない。transport internals が opaque なままであれば、狭い test hook か、所有 layer でのより高レベルな request-building test が必要かもしれない。 + +- Builtin catalog tests は、正確な provider list と count を assert している箇所がやや脆い: + - `builtin_has_four_providers` + - `load_falls_back_to_builtin_when_override_absent` + + これは偶発的な catalog drift を捕捉できるため価値があるが、正当な provider を追加すると、invariant-oriented というより一部 snapshot-like なテスト更新が必要になることも意味する。意図的であれば許容できるが、pure behavior testing ではなく catalog contract testing として扱うべき。 + +- User model override behavior は provider override behavior よりカバーが薄い。 + - `models.toml` override precedence に相当するテストがない。 + - malformed `models.toml` parse-error test がない。 + - providers と models は設計上独立して override されるため、対称的なカバレッジが必要。 + +- Codex OAuth guarded-reload behavior は文書化されているが、直接テストされていない。 + - “file becomes fresh between stale read and refresh” をカバーするテストがない。 + - refresh 前に account id が変わり、そのため refresh を skip するケースをカバーするテストがない。 + - これらは concurrency/race-safety invariants であり、現在のテストは通常の refresh path は検証しているが、race guards は検証していない。 + +- `last_refresh + 8 days` による Codex OAuth staleness fallback は直接テストされていない。現在の provider tests は access-token `exp` を使って fresh/expired cases を強制している。実際の auth file で malformed/non-JWT access token が想定されるなら、この fallback はテストするだけの重要性がある。 + +- Refresh HTTP tests は outbound request shape を assert していない。 + - `client_id` + - `grant_type = refresh_token` + - `refresh_token` + - JSON content type + + 現在の mock は期待される path へ POST が発生したことは証明しているが、Codex-compatible body が正しいことは証明していない。 + +- Error conversion のカバレッジは間接的である。`CodexAuthError::to_client_error` の permanent-path behavior は provider-level login-message test を通して部分的にカバーされているが、transient refresh と config-classified errors は `ClientError` variants として直接 assert されていない。 + +- `CodexAuthProvider::from_default_home` の environment precedence（`HOME` より `CODEX_HOME`）や missing `HOME` に対するテストがない。これは小さいが public construction path である。 + +## 追加を提案するテスト + +- Codex OAuth construction invariants に対する provider-factory tests を追加する: + - `ref = "codex-oauth/gpt-5.5"` が `AuthRef::CodexOAuth` として resolve/build される。 + - `base_url` がない場合、official OpenAI ではなく ChatGPT backend base URL になる。 + - explicit `base_url` は引き続き優先される。 + - 広い internals を露出せずに assert できるなら、Codex OAuth OpenAI Responses が unsupported output/sampling parameters を無効化する。 + +- 対称的な model catalog override tests を追加する: + - `load_models()` は `/models.toml` を優先する。 + - malformed model override は `CatalogError::Parse` を返す。 + - provider override と model override は片方だけが存在する場合も独立している。 + +- Codex OAuth race-guard tests を追加する。できれば `auth_json::load` 周辺の小さな test seam を使うか、呼び出し間で temp-file changes を構成する: + - stale initial snapshot、fresh pre-refresh snapshot: HTTP refresh しない。 + - pre-refresh account id が異なる: HTTP refresh せず、新しい account を採用する。 + +- staleness fallback unit tests を追加する: + - invalid/non-JWT access token + recent `last_refresh` は stale ではない。 + - invalid/non-JWT access token + old `last_refresh` は stale。 + - invalid/non-JWT access token + missing `last_refresh` は stale。 + +- `wiremock` で request body matching を使って refresh HTTP tests を強化し、偶発的な Codex OAuth request-shape 変更を捕捉できるようにする。 + +- catalog churn が想定されるなら、正確な builtin provider-count tests をより contract-focused な checks に置き換えることを検討する: + - required provider ids が存在する + - 各 provider が必要な scheme/auth/default context/capability を持つ + - 各 builtin model が known provider を参照する + + 正確なリストが意図的に product contract の一部であるなら、現在の snapshot-style assertions で問題ない。 + +## 実行したコマンド + +- `cargo test -p provider` + - Result: passed + - Summary: `47 passed; 0 failed; 0 ignored` + - Doc tests: `0 passed; 0 failed` + diff --git a/docs/report/test-validity-20260612/secrets.md b/docs/report/test-validity-20260612/secrets.md new file mode 100644 index 00000000..49330324 --- /dev/null +++ b/docs/report/test-validity-20260612/secrets.md @@ -0,0 +1,96 @@ +# テスト妥当性レビュー: secrets +- 判定: 概ね良い + +## 確認範囲 +- 対象 crate: `crates/secrets` +- 読んだファイル: + - `crates/secrets/Cargo.toml` + - `crates/secrets/README.md` + - `crates/secrets/src/lib.rs` +- 既存テスト: `crates/secrets/src/lib.rs` 内の unit tests 5件 +- 変更: なし + +`secrets` crate の責務は、provider-independent な secret id/value のローカル保存、ID 検証、軽量な plaintext-at-rest 低減、改ざん検出、store file format の読み書きに絞られている。 + +## 現在のテストがよくカバーしていること +- 基本 lifecycle は押さえられている。 + - `set` / `get` / `list_ids` / `delete` / 削除後 `NotFound` を `roundtrip_list_delete` が確認している。 +- ID validation の主要な危険入力は確認されている。 + - empty、absolute path、`..` traversal、double slash、空白、改行、`~`、長すぎる ID、代表的な valid ID を検証している。 +- fail-closed 系の重要性は理解されたテストになっている。 + - ciphertext 改ざん時に `Decode` になる。 + - wrong key で復号できない。 +- secret value の露出防止に関する最低限の invariant はある。 + - `SecretValue` の `Debug` が plaintext を含まない。 + - store file に plaintext が直接書かれない。 +- テストは tempdir と固定 key を使っており、外部環境や実 user secret store に依存しない点は良い。 + +## 不足 / 疑問のあるテスト +- file format error path の coverage が薄い。 + - invalid JSON は `Error::Parse` になるか。 + - unsupported `version` は `Error::UnsupportedVersion` になるか。 + - entry の `nonce` / `ciphertext` / `tag` が invalid hex、odd length、欠落、型不一致の場合に fail-closed するか。 +- ID validation の境界値が少し不足している。 + - `MAX_ID_LEN` ちょうどの ID が通ること。 + - `.`、`x/.`、`x/`、`a//b`、non-ASCII、colon/backslash などの representative invalid cases。 + - 可能なら error variant まで見ると、将来の validation 退行を発見しやすい。 +- encryption/encoding の境界値が不足している。 + - 空文字 secret。 + - 32 bytes を超える multi-block secret。 + - non-ASCII UTF-8 secret。 + - 同じ id/value を overwrite したときも復号できること。 + - nonce/ciphertext が毎回同一にならないことは、この実装の情報漏洩低減 invariant として確認する価値がある。 +- store 操作の細かい API invariant が未確認。 + - empty store の `list_ids()` が empty を返す。 + - missing id の `delete()` が `Ok(false)` を返し、既存 store を壊さない。 + - existing id の overwrite が正しく最新値を返す。 + - `SecretStore::new(data_dir)` が `data_dir/secrets/store.json` を使い、同じ `data_dir` で reopen 可能であること。 +- atomic write 周辺のテストは実質ない。 + - parent directory が自動作成されることは roundtrip で間接的に通っているが、明示されていない。 + - successful save 後に tmp file が残らないこと。 + - write/rename failure の portability ある検証は難しいが、この crate が file persistence を所有するなら少なくとも successful path の file-state invariant は追加できる。 +- `SecretStore` の `Debug` は derived で `key` を出す可能性がある。 + - plaintext secret ではないが、local obfuscation key material を diagnostics に出す設計でよいかは確認対象。 + - 少なくとも「SecretValue だけが redacted 対象」という意図なら明文化、そうでないなら redaction 実装とテストが必要。 +- 現在の corruption tests は private `StoreFile` を直接使う unit test で、file format owned crate としては妥当。 + - ただし JSON wire format の互換性を守りたいなら、private struct 経由だけでなく literal JSON fixture を使うテストもあると version/schema 退行に強くなる。 + +## 追加を提案するテスト +- `loads_empty_missing_store`: + - new temp path で `list_ids()` が `[]`。 + - missing id `get` は `NotFound`。 + - missing id `delete` は `false`。 +- `id_validation_boundaries`: + - `MAX_ID_LEN` exactly accepted。 + - `MAX_ID_LEN + 1` rejected。 + - `.` / `x/.` / `x/` / `x\\y` / `x:y` / `日本語` rejected。 +- `store_file_errors_fail_closed`: + - invalid JSON -> `Parse`。 + - `version: 999` -> `UnsupportedVersion`。 + - invalid hex in each entry field -> `Decode`。 +- `roundtrip_value_boundaries`: + - empty string。 + - long value over multiple SHA blocks。 + - UTF-8 value such as `日本語-secret-🔑`。 +- `overwrite_preserves_latest_value`: + - same id を異なる value で set し、latest のみ返る。 + - `list_ids` に duplicate が出ない。 +- `same_plaintext_reencrypts_with_new_nonce`: + - same id/same value を二度 set し、保存 JSON の nonce または ciphertext が変わることを確認する。 +- `new_uses_data_dir_store_path_and_key`: + - `SecretStore::new(dir)` で保存し、同じ `dir` の store で読める。 + - 別 `dir` の derived key では decode できない。 +- `successful_save_file_state`: + - parent directory 作成。 + - `store.json` が valid JSON。 + - success 後に sibling tmp file が残らない。 + +## 実行したコマンド +- `cargo test -p secrets` + +結果: + +- unit tests: 5 passed +- doc-tests: 0 passed +- failures: none + diff --git a/docs/report/test-validity-20260612/session-analytics.md b/docs/report/test-validity-20260612/session-analytics.md new file mode 100644 index 00000000..3a3c4e6e --- /dev/null +++ b/docs/report/test-validity-20260612/session-analytics.md @@ -0,0 +1,79 @@ +# テスト妥当性レビュー: session-analytics +- 評価: 概ね良い + +## 確認した範囲 +- 対象 crate: `crates/session-analytics` +- 実装: `crates/session-analytics/src/lib.rs` +- crate 内のテストは `src/lib.rs` の inline unit tests のみ。 +- `crates/yoi/src/session_cli.rs` も参照し、`session-analytics` が `yoi session analyze ... --json` から呼ばれる境界を確認した。 +- ソースファイル・Ticket は変更していない。 + +## 現在のテストがよくカバーしていること +- read-only な session JSONL analyzer としての主要な集計軸はかなり押さえられている。 + - repeated `Read` と intervening `Edit` の相関。 + - large `Edit` argument、`replace_all`、path 別 edit stats。 + - failed tool result count と turn ごとの tool call count。 + - large/truncated tool result の検出と、raw tool output を report JSON に混ぜないことの基本確認。 + - compaction/context lifecycle 後の repeated read を「correlation only」として扱う方針。 + - malformed JSONL / unknown entry の tolerant parsing。 + - Bash による file inspection の observation。 + - assistant response 単位の tool batching metrics。 + - same-file multi-edit、edit-only streak、Read/Bash/test interleave を含む edit round-trip metrics。 + - edits がない session の empty metrics。 +- fixture は小さく、各テストの意図が読みやすい。crate の責務である「実 session log を直接型依存せず、`serde_json::Value` で寛容に読む」性質にも合っている。 +- privacy/safety 重要点である「raw content を出さない」方向は、少なくとも large result について回帰テストがある。 +- `cargo test -p session-analytics` は全 12 unit tests と doc-tests が成功した。 + +## 不足 / 疑問のあるテスト +- 実 session JSONL に近い golden/contract fixture がない。現在の tests はすべて手作り最小 fixture なので、実ログ形式の変化、複数 kind の混在、長い session の代表ケースに対する report schema の安定性は十分には保証していない。 +- JSON report が CLI-facing な機械可読出力である割に、serialize 後の schema/field contract をまとめて確認する snapshot/golden test がない。 +- privacy invariant は一部のみ。raw user input、tool arguments の `old_string` / `new_string` / `content`、tool result `content` に sentinel を入れて、serialized report 全体に出ないことを横断的に確認するテストが欲しい。 +- tolerant parsing の edge cases が薄い。 + - invalid `arguments` JSON。 + - missing `tool_call.name`。 + - `Read` / `Edit` / `Write` の path 欠落。 + - `segment_start.history` に seeded tool calls が入る場合。 + - top-level/synthetic response boundary diagnostic。 + - empty file / nonexistent file error。 +- response batching の境界条件が不足している。 + - message/reasoning-only `assistant_item`。 + - assistant response が non-`assistant_item` で閉じられるケース。 + - 複数 response にまたがる percentile/distribution。 + - `top_tool_call_responses` の sort/truncate。 +- tool classification / observation 系の coverage が偏っている。 + - `counts_by_kind` は total の間接確認程度で、pod/memory/ticket/task/web/other 分類は未確認。 + - `Write` stats、large `Write.content`、`path` alias、large `Read.limit`、large `Grep.head_limit` は未確認。 +- diagnostic bounding はテスト名に “bounded” とあるが、実際には 3 件の diagnostic を見るだけで、`MAX_DIAGNOSTICS` で打ち止めになることは検証していない。 +- 一部の assertion は observation 文言の substring に依存している。policy 表現を守る意図なら妥当だが、単なる説明文変更で壊れやすい。可能なら stable な `kind` や boolean/metric を主に検証し、文言は最小限にした方がよい。 + +## 追加を提案するテスト +- 実ログに近い小さな golden fixture を `tests/fixtures` 相当で持ち、serialized JSON report の主要 schema と代表値を固定する。 +- sentinel privacy test を追加する。 + - `user_input` の本文。 + - `Edit.old_string` / `Edit.new_string`。 + - `Write.content`。 + - `tool_result.content`。 + - これらが serialized `SessionReport` に含まれないことを一括確認する。 +- tolerant parsing/error-path tests を追加する。 + - invalid tool arguments JSON。 + - missing fields。 + - nonexistent input path。 + - empty JSONL file。 + - `MAX_DIAGNOSTICS + n` 件の malformed entries。 +- response boundary tests を追加する。 + - `segment_start.history` seeded calls が tool usage には数えられるが response batching からは除外され、diagnostic/observation が出ること。 + - assistant message-only response と mixed message/tool calls。 + - multiple responses distribution と top-response ordering/truncation。 +- less-covered metric tests を追加する。 + - `Write` stats と large content。 + - `path` vs `file_path`。 + - large `Read` と large `Grep`。 + - pod/memory/ticket/task/web/other に対する `counts_by_kind`。 + - threshold boundary: `>= LARGE_*` と just-below cases。 + +## 実行したコマンド +- `cd /home/hare/Projects/yoi && cargo test -p session-analytics` + - 結果: 成功。12 unit tests passed, doc-tests 0 passed. +- `cd /home/hare/Projects/yoi && cargo test -p session-analytics -- --nocapture` + - 結果: 成功。12 unit tests passed, doc-tests 0 passed. + diff --git a/docs/report/test-validity-20260612/session-metrics.md b/docs/report/test-validity-20260612/session-metrics.md new file mode 100644 index 00000000..3b5d580f --- /dev/null +++ b/docs/report/test-validity-20260612/session-metrics.md @@ -0,0 +1,123 @@ +# テスト妥当性レビュー: session-metrics +- 評価: 概ね良い + +## 確認範囲 + +- 対象 crate: `crates/session-metrics` +- 読んだ主なファイル: + - `crates/session-metrics/src/lib.rs` + - `crates/session-metrics/README.md` + - 関連する実利用側テスト: `crates/pod/tests/session_metrics_test.rs` +- crate の責務: + - `LogEntry::Extension { domain: "metrics" }` 上に載せる軽量な metrics 型を定義する + - `Metric` の serde 表現と builder helper を提供する + - `record_metric` で `session-store::save_extension` に metrics domain として append する + - `RestoredState.extensions` 相当の `(domain, payload)` 列から metrics domain の `Metric` だけを抽出する + +## 現在のテストがよくカバーしていること + +`session-metrics` 自体の unit test は 4 件で、薄い crate の主要な純粋ロジックはおおむね押さえています。 + +- `metric_round_trip_via_json` + - `Metric::now(...)` + - `with_dimension` + - `with_value` + - `with_correlation_id` + - JSON round-trip 後の等価性 + を確認している。 +- `metric_serializes_minimal_form_compactly` + - 空の `dimensions`、`None` の `value` / `correlation_id` が JSON に出ないことを確認している。 + - session log に載せる payload を不要に膨らませない invariant の確認として妥当。 +- `fold_skips_other_domains` + - `metrics_from_extensions` が `DOMAIN == "metrics"` の entry だけを拾うことを確認している。 + - extension domain の分離という crate 境界に合っている。 +- `fold_skips_undeserializable_payloads` + - metrics domain でも deserialize できない payload を落として継続することを確認している。 + - コメントにある「schema 変更で読めない payload は無視する」方針に対応している。 + +加えて、`pod` crate 側の `crates/pod/tests/session_metrics_test.rs` が実利用パスをかなりよく補っています。 + +- prune metrics の `skip` / `fire` / `post_request` が session log に残り、`metrics_from_extensions` で復元できる。 +- `correlation_id` による `prune.fire` と `prune.post_request` の join 前提を確認している。 +- `record_metric` 経由の metrics write failure が本体 run を abort しないことを確認している。 +- metrics 以前の old session が clean に replay できることを確認している。 + +このため、crate 単体テストは小さいものの、利用側の integration coverage まで含めると現在の責務に対する妥当性は高めです。 + +## 不足 / 疑問のあるテスト + +- `record_metric` 自体の直接テストが `session-metrics` crate 内にありません。 + - 現状は `pod` 側 integration test で間接的に成功・失敗パスを踏んでいます。 + - ただし、この crate の public API としては `record_metric` が `DOMAIN` を正しく使い、`StoreError` をそのまま返し、restore 後に `Metric` として読めることを直接確認するテストがある方がよいです。 +- serde default の後方互換テストが片方向です。 + - 現在は「空/None の field を serialization で省略する」ことは確認しています。 + - しかし「省略された JSON を deserialize して `dimensions = {}`、`value = None`、`correlation_id = None` になる」ことは直接確認していません。 +- `metrics_from_extensions` の順序保持は実質的に確認されていますが、意図としてはもう少し明示してもよいです。 + - `fold_skips_other_domains` で `a`, `b` の順序は見ています。 + - これは十分とも言えますが、session-log の時系列を壊さないことが重要ならテスト名か assertion message で invariant として明記すると読みやすいです。 +- `value: Option` の非有限値の扱いは未定義に見えます。 + - `serde_json` は `NaN` / infinity の扱いが直感的でないため、metrics に非有限値が入る可能性を許すなら仕様化が必要です。 + - 現時点で producer が通常の token count / duration / savings だけなら優先度は低いです。 +- 実装詳細に寄りすぎた脆いテストは少ないです。 + - JSON の「field 名文字列を含まない」確認はやや表層的ですが、この crate では serde wire format 自体が API なので許容範囲です。 + - ただし厳密な JSON 文字列全体比較ではなく `contains` 否定に留めているため、過度には脆くありません。 + +## 追加の提案 + +優先度順に追加するなら以下です。 + +1. `record_metric_appends_metrics_extension_and_restores` + - `FsStore` か軽い fake `Store` を使う。 + - `record_metric(...)` を呼ぶ。 + - `session_store::restore(...)` 後に `state.extensions` から `metrics_from_extensions(...)` して、元の `Metric` が 1 件取れることを確認する。 + - これで `DOMAIN`、payload serde、session-store への append path を crate 側で直接守れる。 + +2. `record_metric_propagates_store_error` + - append が必ず失敗する minimal fake `Store` を置く。 + - `record_metric` が `Err(StoreError)` を返すことを確認する。 + - `pod` 側の「失敗しても run 継続」は pod の責務なので、ここでは error propagation だけで十分。 + +3. `metric_deserializes_minimal_legacy_payload` + - `{"name":"x","ts":1}` から `Metric` に deserialize する。 + - `dimensions.is_empty()`、`value.is_none()`、`correlation_id.is_none()` を確認する。 + - 現在の `#[serde(default)]` / optional field の後方互換意図を直接固定できる。 + +4. 必要なら `metric_non_finite_value_behavior_is_documented` + - 非有限値を禁止するなら constructor/helper 側で弾く設計にしてからテスト。 + - 許すなら JSON payload 上でどう表現されるかを明示する。 + - 現時点では必須ではなく、producer が非有限値を作る可能性が出た時点でよいです。 + +## 実行したコマンド + +```sh +cd /home/hare/Projects/yoi && cargo test -p session-metrics +``` + +結果: + +```text +running 4 tests +test tests::fold_skips_other_domains ... ok +test tests::fold_skips_undeserializable_payloads ... ok +test tests::metric_round_trip_via_json ... ok +test tests::metric_serializes_minimal_form_compactly ... ok + +test result: ok. 4 passed; 0 failed +``` + +```sh +cd /home/hare/Projects/yoi && cargo test -p pod --test session_metrics_test +``` + +結果: + +```text +running 5 tests +test old_sessions_without_metrics_replay_cleanly ... ok +test metric_write_failure_emits_warn_alert_and_does_not_abort_run ... ok +test prune_metrics_record_below_min_savings_skip ... ok +test prune_metrics_emit_skip_then_fire_with_post_request_join ... ok +test prune_metrics_fire_during_single_long_task_without_multiple_user_turns ... ok + +test result: ok. 5 passed; 0 failed + diff --git a/docs/report/test-validity-20260612/session-store.md b/docs/report/test-validity-20260612/session-store.md new file mode 100644 index 00000000..a6a347be --- /dev/null +++ b/docs/report/test-validity-20260612/session-store.md @@ -0,0 +1,171 @@ +# テスト妥当性レビュー: session-store +- 評価: 概ね良い + +## 確認範囲 + +- `crates/session-store/README.md` +- `crates/session-store/Cargo.toml` +- `crates/session-store/src/` + - `lib.rs` + - `store.rs` + - `fs_store.rs` + - `segment.rs` + - `segment_log.rs` + - `logged_item.rs` + - `system_item.rs` + - `event_trace.rs` +- `crates/session-store/tests/` + - `fs_store_test.rs` + - `session_test.rs` + - `common/mod.rs` +- 実行確認: `cargo test -p session-store` + +## 現在のテストがよくカバーしていること + +`session-store` の主要責務である「append-only JSONL session log を読み書きし、Worker 復元用 state に replay する」部分は、短時間レビューとしては十分に押さえられています。 + +良い点: + +- `collect_state` の基本 replay が広くテストされている。 + - 空の log + - `SegmentStart` + - user / assistant / tool call / tool result + - `TurnEnd` + - `ConfigChanged` + - `LlmUsage` + - `Invoke` marker が state を変えないこと + - `Extension` + - typed `protocol::Segment` の保存と flattened user message への復元 +- `LoggedItem` の stable mirror が比較的よく検証されている。 + - message + - tool call + - tool result content / error flag + - reasoning encrypted content + - reasoning signature + - legacy signature 欠落 JSON + - serde tag shape +- `SystemItem` は、LLM に入る `history_text` が stored body 由来であること、reminder wrapping、legacy default source、JSON round-trip が確認されている。 +- `FsStore` は tempfile を使った実 FS の読み書きで確認されている。 + - append / read round-trip + - `create_segment` + - `list_sessions` + - `list_segments` + - `exists` + - missing segment + - trace file が segment log と分離されること + - `read_entry_count` + - `lookup_session_of` +- `session_test.rs` は mock LLM + actual `llm_worker::Worker` を通して、単純応答・tool call・pause・restore・fork 系を実行している。単なる pure unit だけでなく、crate 境界上の使われ方もある程度守れている。 +- fork lineage について、fresh-session fork / in-session `fork_at` / live conflict `ensure_head_or_fork` / nested past fork まで触れているのは良いです。特に「source segment を mutate しない」方針をテストで固定できています。 + +## 不足・疑問のあるテスト + +現状は「基本機能の回帰検出」としては強い一方で、session-store の重要 invariant に対して未検証の穴がいくつか残っています。 + +- `create_compacted_segment` が未テスト。 + - この crate の説明上、compaction は fork と並ぶ segment lineage 操作です。 + - `compacted_from`、同一 `session_id` 継承、seed history/config/system prompt の保存が直接検証されていません。 + +- `Store::truncate` が未テスト。 + - trait default 実装で `read_all -> truncate -> create_segment` するため、empty-turn rollback などの重要用途で壊れてもこの crate 単体では検出しにくいです。 + - `entries_len > len` の `StoreError::Corrupt` も未検証です。 + +- corrupt JSONL の扱いが未テスト。 + - `FsStore::parse_jsonl` は blank line を無視し、serde failure を `StoreError::Corrupt { line, message }` に変換します。 + - line number、blank line skip、部分的に壊れた file で read が fail closed することは永続ログ store として重要ですが未確認です。 + +- `FsStore` の filtering / layout invariant が一部弱い。 + - top-level old flat files を `list_sessions` が無視すること。 + - invalid UUID directory を無視すること。 + - `.trace.jsonl` を `list_segments` が segment として拾わないこと。 + - invalid segment filename を無視すること。 + - `list_segments` の newest-first order は `contains` 中心で、順序を強く確認していません。 + +- `save_delta` / `classify_history_item` の直接テストが薄い。 + - user message を skip する contract は integration helper 経由では使われていますが、直接の単体テストがありません。 + - reasoning が `AssistantItem` になること、tool result error/content が `ToolResult` として保存されること、unknown/future-ish branch の defensive behavior は十分固定されていません。 + +- `RunErrored` path が未テスト。 + - `run_and_persist` helper には error branch がありますが、最後に `result.unwrap()` するため、実際には error persistence の検証に使えません。 + - `save_run_errored` と replay 時の `last_run_interrupted` 反映は、失敗 turn の session 復元に関わるため追加価値が高いです。 + +- `SystemItem` の replay path が直接弱い。 + - `SystemItem::history_text` 自体はテストされていますが、`LogEntry::SystemItem` を `collect_state` したときに `Item::system_message` へ入ること、`append_system_item` 経由で store に保存されることは明示的にテストされていません。 + +- fork 系の一部 assertion が弱い。 + - `session_fork_at_truncates_within_session` は fork state と source-at-fork の `history.len()` だけを比べています。content / item kind / config / system prompt / lineage まで比較した方が妥当です。 + - `at_turn_index == 0` を、実際に turn 後の source segment に対して使うケースが薄いです。 + - 存在しない `at_turn_index` の扱いは未固定です。現実装は `unwrap_or(entries.len())` で末尾 fork になりますが、それが意図かどうかをテストで明確にした方がよいです。 + - `ensure_head_or_fork` の no-op branch（count equal）と、store count が writer tally より小さい場合の policy が未検証です。 + +- integration tests の一部は smoke test 寄りで、壊れ方を見逃しやすい。 + - `session_restore_round_trip` は history の長さだけで、復元 item の content / role / tool call id までは比較していません。 + - `session_run_with_tool_call` は `ToolResult` / `AssistantItem` の存在だけを確認しており、順序・call id・tool result content・final assistant text を見ていません。 + - `session_run_logs_entries` はコメントでは最低 5 entry と書きつつ `>= 4` で、`UserInput` の存在も明示確認していません。 + +- real concurrency / append atomicity は未テスト。 + - この crate は append-mode JSONL を concurrency 境界として扱っていますが、複数 clone / thread からの append が line corruption しないことは未確認です。 + - ただし OS/filesystem 依存が強いので、軽い regression test に留めるのが妥当です。 + +## 追加を提案するテスト + +優先度順に追加するなら以下です。 + +1. `create_compacted_segment` の lineage test + - source session を作る + - compacted segment を作る + - first entry が `SegmentStart { session_id: source_sid, compacted_from: Some(...) }` + - `forked_from` は `None` + - history/config/system prompt が seed state から復元される + +2. `Store::truncate` default behavior test + - 3〜4 entries を作る + - 2 entries に truncate + - `read_all` が prefix のみ返す + - `read_entry_count` が一致する + - len 超過 truncate が `StoreError::Corrupt` になる + +3. corrupt JSONL test + - tempfile 上に直接 invalid JSON line を置く + - `read_all` が `StoreError::Corrupt { line, .. }` を返す + - blank line が count / parse に影響しないことも併せて確認する + +4. `FsStore` layout filtering test + - root 直下の `.jsonl` を無視 + - invalid directory を無視 + - session dir 内の `.trace.jsonl` と invalid filename を `list_segments` が無視 + - segment order を newest-first で assertion + +5. `save_delta` direct unit/integration test + - input に user message / assistant message / reasoning / tool call / tool result を混ぜる + - user message が保存されないこと + - classification と order が期待通りなこと + +6. `RunErrored` persistence test + - `save_run_errored` を直接使うだけでもよい + - 可能なら mock client error を発生させ、Pod 相当の persistence sequence で `RunErrored` が書かれることを確認する + - `collect_state` / `restore` で `last_run_interrupted` が反映されることを確認する + +7. `SystemItem` replay/store test + - `append_system_item` で notification / task reminder などを保存 + - `restore` 後の `history` に `role: system` の message と stored body が入ることを確認する + +8. fork tests の assertion 強化 + - `history.len()` だけでなく、復元 content / role / item kind を比較する + - `forked_from` の `segment_id` と `at_turn_index` を `fork_at` test でも確認する + - `at_turn_index == 0` on non-empty source と nonexistent turn index の policy を固定する + +## 実行したコマンド + +```sh +cd /home/hare/Projects/yoi && cargo test -p session-store +``` + +結果: + +- unit tests: 29 passed +- `tests/fs_store_test.rs`: 8 passed +- `tests/session_test.rs`: 9 passed +- doctest: 1 ignored +- overall: passed + diff --git a/docs/report/test-validity-20260612/ticket.md b/docs/report/test-validity-20260612/ticket.md new file mode 100644 index 00000000..fa1b5154 --- /dev/null +++ b/docs/report/test-validity-20260612/ticket.md @@ -0,0 +1,117 @@ +# テスト妥当性レビュー: ticket +- 評価: 概ね良い + +## 確認範囲 + +- 対象: `crates/ticket` / package `ticket` +- 主要責務: + - typed Ticket domain API と `.yoi/tickets//` local file backend + - Ticket lifecycle state, thread event, relation, orchestration-plan artifact の読み書き + - `.yoi/ticket.config.toml` の role/backend 設定解決 + - LLM-facing Ticket tools の schema / bounded output / backend adapter +- 既存テスト: + - `src/lib.rs` unit tests + - `src/config.rs` unit tests + - `src/tool.rs` async unit tests +- 変更: なし。読解とテスト実行のみ。 + +## 現在のテストがよくカバーしている点 + +- `cargo test -p ticket` は成功しており、66 tests が通過している。 +- backend の主要 happy path はかなり広く押さえられている: + - create / show / list 相当の local layout + - opaque canonical id + - `item.md`, `thread.md`, `artifacts/.gitkeep`, `resolution.md` + - obsolete frontmatter field を出さないこと + - Japanese record language の generated defaults + - numeric-looking YAML scalar の quoted round-trip +- lifecycle invariant のカバーは良い: + - `planning -> ready -> queued -> inprogress -> done` + - `ready/queued -> planning` + - disallowed transition rejection + - stale transition rejection + - generic `set_state_field` で `state` / `workflow_state` / `status` を変更できないこと + - `queue_ready` が non-ready Ticket を mutate しないこと +- thread event / doctor の基本検証は妥当: + - review status + - state_changed / intake_summary attributes + - invalid thread event attributes が append されないこと + - legacy bucket / obsolete fields / invalid state / malformed typed event の doctor diagnostics +- relation / orchestration-plan の重要点も押さえられている: + - forward relation storage + - inverse derived view + - dependency / incoming blocker が queue を止めること + - relation cycles / dangling target / self-target artifact を doctor が拾うこと + - orchestration plan JSONL append / query / invalid artifact detection +- config tests は fixed role config の方針に合っている: + - missing config defaults + - scaffold の全 role coverage + - backend-only config が role launch ready ではないこと + - unknown role / stale investigator role / unknown field rejection + - backend provider と legacy kind の扱い + - relative root resolution + - path-like profile selector rejection +- tool tests は LLM-facing surface として重要な bounded output を一部確認できている: + - tool name partitions + - generated schemas + - create/list/show/doctor flow + - list limit cap / title and hint truncation + - list が body/thread/artifact/resolution content を漏らさないこと + - workflow and relation tools の basic flow + +## 不足 / 疑問のあるテスト + +- `TicketCreate` tool の optional `state` 入力に対する output 整合性が未検証。実装上、backend には `params.state` を渡している一方、tool output は常に `"state": "planning"` を返しているため、`state: ready|queued|done|closed` で作成した場合の不一致をテストが捕まえられない。 +- `TicketShow` の bounded output が十分に直接検証されていない。`event_limit`, `artifact_limit`, `body_max_bytes` の truncation と UTF-8 boundary safety は tool の安全性に直結するが、現状は list 側の bounded behavior の方が厚い。 +- compound mutation の atomicity が一部未検証。特に `mark_intake_ready` は intake summary append と state transition が組み合わさるため、後段の state change validation 失敗時に partial event が残らないかをテストした方がよい。 +- Markdown thread parser の body delimiter edge case が弱い。thread entry は `---` 行を区切りとしているため、event body に Markdown horizontal rule として `---` が含まれる場合の仕様がテストで固定されていない。 +- relation API の negative cases は doctor artifact 手書き検査に寄っており、public backend API 経由の拒否が薄い: + - duplicate relation + - self-target relation + - missing target + - empty / multiline / oversized target, note, author + - dependency target が `done` / `closed` になった後は blocker ではなくなること +- `queued -> inprogress` 側の blocker enforcement は実装されているが、直接テストは薄い。`queue_ready` の blocker rejection はあるため大枠は安心だが、acceptance step としての `set_workflow_state(queued, inprogress)` でも同じ relation blocker が効くことを明示するとよい。 +- config parser は代表的な rejection を持つが、top-level unknown field、unsupported profile source、`builtin:` のような empty registry name、absolute backend root の扱いは未固定。 +- tool input validation の negative tests が少ない。invalid JSON、invalid enum、empty required text、same-state transition、invalid author/reason の error shape と no-mutation を追加すると tool boundary の信頼性が上がる。 +- tests は private helpers / exact diagnostic substring にかなり依存している。これは file-backend crate では許容範囲だが、diagnostic wording 変更で壊れる brittle tests が一部ある。仕様として固定したい文言だけ exact assertion にするのが望ましい。 + +## 追加候補 + +- `TicketCreate` tool: + - `{"title": "...", "state": "ready"}` で作成し、tool output / `TicketShow` / `TicketList state=ready` が同じ state を返すこと。 +- `TicketShow`: + - large body / many events / many artifacts を作り、`body_max_bytes`, `event_limit`, `artifact_limit` が count / returned / truncated を正しく返すこと。 + - multibyte UTF-8 の truncation が invalid UTF-8 や panic にならないこと。 +- Lifecycle atomicity: + - invalid `TicketStateChange` を渡した `mark_intake_ready` が intake_summary だけを残さないこと。 + - `TicketWorkflowState` tool の same-state transition rejection と no-mutation。 +- Thread parser: + - event body に quote, backslash, whitespace を含む attributes/body の round-trip。 + - body 中の `---` 行を許すのか禁止するのかを仕様化し、期待動作をテスト。 +- Relation: + - duplicate/self/missing-target を `add_ticket_relation` 経由で拒否するテスト。 + - dependency target を `done` または `closed` にした後、queue / queued->inprogress が通ること。 + - `supersedes` / `duplicate_of` notices の outgoing/incoming view。 +- Orchestration plan: + - `accepted_plan` missing / accepted_plan supplied to non-accepted kind / `waiting_capacity_note` without note / oversized or multiline single-line fields。 +- Config: + - unknown top-level field rejection。 + - unsupported profile source and empty source-qualified profile name。 + - absolute backend root が意図通り保持されること。 +- Tool boundary: + - malformed JSON and invalid enum values return `ToolError::InvalidArgument`。 + - mutating tools の invalid input が backend files を変更しないこと。 + +## 実行したコマンド + +```sh +cargo test -p ticket +``` + +結果: + +```text +66 passed; 0 failed; 0 ignored +doc-tests: 0 passed + diff --git a/docs/report/test-validity-20260612/tools.md b/docs/report/test-validity-20260612/tools.md new file mode 100644 index 00000000..0234612e --- /dev/null +++ b/docs/report/test-validity-20260612/tools.md @@ -0,0 +1,139 @@ +# テスト妥当性レビュー: tools + +- 評価: 混在 + +## 確認範囲 + +- Workspace: `/home/hare/Projects/yoi` +- Crate: `tools` +- 確認対象: + - `crates/tools/Cargo.toml` + - `crates/tools/src/{lib,scoped_fs,tracker,read,write,edit,glob,grep,bash,web,error}.rs` + - `crates/tools/tests/{integration,edge_cases}.rs` +- この crate は以下の built-in tool の挙動を担っている: + - filesystem tools: `Read`, `Write`, `Edit`, `Glob`, `Grep` + - process tool: `Bash` + - network tools: `WebSearch`, `WebFetch` + - 共有 safety primitives: `ScopedFs`, `Tracker`, `ToolsError` + +## 現在のテストがよくカバーしていること + +主要な機能テストのカバレッジは広く、概ね適切な対象に向いている。 + +- `ScopedFs` tests は中核的な read/write scope enforcement をカバーしている: + - absolute-path requirement + - missing files と directory targets + - out-of-scope と read-only paths + - write 時の parent creation + - symlink-to-file behavior + - symlink escape diagnostics + - clone 間での動的な `SharedScope` updates +- `Tracker` tests は重要な read-before-edit invariant をカバーしている: + - record 後の clean verify + - `NotRead` + - externally modified hash mismatch + - clone-shared state + - symlink variants の canonical key collapse + - recent-file LRU behavior + - per-file mutation guard serialization と different-file parallelism +- `Read` / `Write` / `Edit` tests は期待される user-facing contract をカバーしている: + - Read が history を記録する + - Write は read なしで new files を作成できる + - 既存ファイルへの Write/Edit は prior Read を要求する + - external modification が検出される + - unique replacement、`replace_all`、missing string、non-unique string +- `Glob` と `Grep` は user-visible options を多くカバーしている: + - glob matching、hidden files、mtime ordering、invalid pattern + - grep output modes、line numbers、context、multiline、glob/type filters、pagination、binary skip、invalid regex/type + - scope-denied results が filter される +- `Bash` tests は重要な runtime behavior をカバーしている: + - stdout/stderr merge + - nonzero exit summary + - stateless cwd + - timeout + - long output spill と tail rendering + - drop 時の spill cleanup + - background job non-hang regression +- `WebFetch` / `WebSearch` には有用な local-server tests がある: + - disabled tools が network requests を行わない + - private address blocking + - redirect handling + - Brave request shaping と secret ref use + - oversized Brave response rejection + - HTML fallback、local reader extraction、navigation inclusion/omission、output truncation + +2つの integration test files は価値がある。`core_builtin_tools()` factory wiring と、cross-tool sharing of `ScopedFs` / `Tracker` を exercise しており、これはこの crate の中心的 invariant の1つである。 + +## 不足 / 疑問のあるテスト + +default の crate test command は現在 green ではない。`cargo test -p tools` は doctests のみで失敗する。理由は、`tracker.rs` の documentation example がまだ `core_builtin_tools(fs, tracker, bash_outputs, None)` を呼んでいる一方で、実際の関数は現在3引数を取るためである。これは test-suite validity issue である。機能テストは通っているが、開発者が実行する default command は failure を報告する。 + +実質的に最も大きな gap は `web_builtin_tools()` 周辺である。core tool factory は integration-tested だが、web factory については registration names、metadata/schema presence、実際の `Tool` trait object 経由の disabled behavior、public tool surface との parity が確認されていない。 + +いくつかの領域は happy-path や string-fragment checks でテストされているが、boundary behavior はテストされていない: + +- `Read` + - invalid UTF-8 / binary-ish content rendering behavior の直接的な coverage がない + - offset-beyond-EOF や `limit = 0` behavior test がない + - trailing newline / CRLF line-count semantics が固定されていない +- `Write` / `Edit` + - mutation serialization はテストされているが、`write_then_edit_same_file_same_batch_uses_call_order` は脆く見える。coordinator は path ごとに serialize するが、test name は call-index ordering を示唆している。この test は ordering invariant を証明するというより、polling order によって通っている可能性が高い。 + - `old_string == new_string`、empty `old_string`、non-UTF-8 edit target の明示的な test がない + - write/edit の broken symlink handling は read/symlink escape より直接的な coverage が少ない +- `Glob` + - result cap at 1000 がテストされていない + - relative explicit `path`、non-directory `path`、out-of-scope explicit `path` が直接カバーされていない + - symlink directory behavior は `Glob` でカバーされているが、direct path としてのみである +- `Grep` + - description では `.gitignore` を honor するとされているが、直接的な `.gitignore` regression test は見当たらなかった + - `-n: false`、個別の `-A` / `-B`、zero `head_limit`、context lines 付き content-mode pagination がカバーされていない + - `Glob` test と同様の symlink-directory rejection がカバーされていない +- `Bash` + - timeout behavior は確認されているが、process-tree cleanup や partial output を伴う timed-out command は確認されていない + - tail read budget を超える very large output と partial UTF-8 tail boundaries がカバーされていない + - unusual shell syntax を含む multiline commands などの command wrapping edge cases は、間接的にしか軽くカバーされていない +- `WebFetch` + - URL parser constraints の coverage が不足している: non-http schemes、embedded credentials、missing host + - content-type rejection、binary body rejection、invalid UTF-8、JSON/XML/text rendering、`Content-Length` なしの response-size truncation、redirect limit exceeded、redirect-to-private blocking が直接カバーされていない + - private-address tests は有用だが必然的に local であり、DNS/rebinding-style behavior はここでは深くテストできない + +一部の tests は exact または near-exact な diagnostic strings を assert している。remediation-focused user diagnostics に対しては正当化できるが、invariant-focused checks よりもいくつかの tests を脆くしてもいる。 + +## 追加提案 + +1. `tracker.rs` の stale doctest を修正するか、意図的に mark する。`cargo test -p tools` が今日失敗するため、これは最初の test-suite maintenance item として扱うべきである。 + +2. 小さな `web_builtin_tools_registers_full_set` integration test を追加する: + - `WebSearch` と `WebFetch` を期待する + - non-empty descriptions と object schemas を verify する + - trait object 経由で disabled tools を execute し、no-network disabled error を assert する + +3. focused boundary tests を追加する: + - `Read` offset beyond EOF、`limit = 0`、CRLF/trailing newline behavior + - `Edit` empty `old_string`、identical strings、non-UTF-8 target + - `Glob` explicit relative/out-of-scope/non-directory path と 1000-result cap + - `Grep` `.gitignore`、`-n: false`、個別の `-A`/`-B`、symlink directory rejection、zero/low `head_limit` + - `Bash` partial output 付き timeout と `TAIL_READ_BUDGET` を超える huge output + - `WebFetch` unsupported scheme、credentials in URL、unsupported content-type、binary body、invalid UTF-8、redirect limit、redirect-to-private + +4. `write_then_edit_same_file_same_batch_uses_call_order` を作り直すか rename する。call-index ordering が intended invariant なら、implementation と test がそれを deterministic にすべきである。same-file serialization のみが intended なら、test name と assertion は ordering guarantees を示唆しないようにすべきである。 + +## 実行したコマンド + +- `cargo test -p tools` + - Result: failed. + - Unit/integration portions passed: + - lib tests: `99 passed` + - `tests/edge_cases.rs`: `10 passed` + - `tests/integration.rs`: `14 passed` + - Doctest failed: + - `crates/tools/src/tracker.rs - tracker (line 29)` + - error `E0061`: `core_builtin_tools` は現在3引数を取るが、doctest は4引数を渡している。 + +- `cargo test -p tools --lib --tests` + - Result: passed. + - `99` lib tests、`10` edge-case tests、`14` integration tests が passed。 + +- `cargo test -p tools --doc` + - Result: 同じ stale `tracker.rs` doctest error で failed。 + diff --git a/docs/report/test-validity-20260612/tui.md b/docs/report/test-validity-20260612/tui.md new file mode 100644 index 00000000..55bc57a4 --- /dev/null +++ b/docs/report/test-validity-20260612/tui.md @@ -0,0 +1,131 @@ +# テスト妥当性レビュー: tui + +- 評価: 混在 + +## 確認範囲 + +- 対象 crate: `crates/tui` +- 確認した主な範囲: + - `crates/tui/Cargo.toml` + - `crates/tui/README.md` + - `crates/tui/src/lib.rs` + - 既存 unit tests を持つ主要 module: + - `app.rs` + - `input.rs` + - `multi_pod.rs` + - `single_pod.rs` + - `workspace_panel.rs` + - `pod_list.rs` + - `task.rs` + - `command.rs` + - `markdown.rs` + - `ui.rs` + - `spawn.rs` + - `setup_model.rs` + - `composer_history.rs` + - `composer_keys.rs` + - `role_session_registry.rs` + - `picker.rs` + - `keys.rs` +- crate の責務として確認した内容: + - `ratatui` / `crossterm` ベースの TUI 表示・入力処理 + - single-pod chat UI、multi-pod / workspace panel、pod picker、spawn/setup/key management UI + - composer/input model、Markdown/tool/task/system event rendering + - Pod list / Ticket panel / local role-session claim などの表示用 view model glue + +## 現在のテストがよくカバーしていること + +- Unit test の量は多く、`cargo test -p tui` 実行時に 299 tests が列挙される。TUI crate としては model-level の回帰テストがかなり厚い。 +- `app.rs` / `input.rs` は重要な composer invariants をよく押さえている。 + - running 中の submit queueing + - rollback 時の入力復元 + - input history persistence + - file / knowledge completion + - typed `Segment` の保持 + - context usage 表示 + - live system item / task snapshot の反映 +- `multi_pod.rs` / `workspace_panel.rs` は dogfooding 上重要な panel semantics を広く検証している。 + - Ticket row ordering / state display + - ready ticket queue action + - done ticket close action + - Review action が黙って approve しないこと + - Companion / Orchestrator lifecycle decision + - composer が selected pod ではなく workspace companion に向くこと + - local role-session claim の扱い +- `pod_list.rs` は live/stored pod の分類・sort・socket reachability・corrupt metadata diagnostic を押さえており、TUI が誤って restore/send 可能扱いしないための invariant として有用。 +- `task.rs` には `snapshot_format_contract` があり、Pod 側 snapshot 形式と TUI 側 deserialization の境界を守るテストとして妥当。 +- `command.rs` / `single_pod.rs` は `:compact` / `:rewind` / `:peer` などの command が通常 user message として送られないこと、local diagnostic になることを確認していて良い。 +- `markdown.rs` は Markdown renderer の基本構文、list、code block、table を文字列化して確認しており、LLM 出力表示の最低限の regressions には効く。 +- `composer_history.rs` / `role_session_registry.rs` は workspace-scoped path、duplicate suppression、claim conflict など、永続化先を誤らないための安全側テストがある。 + +## 不足 / 疑問のあるテスト + +- 現在の test suite は red。`cargo test -p tui` は 296 passed / 3 failed。 + - `multi_pod::tests::orchestrator_launch_context_uses_orchestration_root_for_runtime_workspace` + - 失敗内容: expected `"/repo/yoi/.worktree/orchestration/yoi-orchestrator"` but actual `"/repo/yoi"` + - 現在の設計では dedicated Orchestrator が process cwd と runtime workspace を分ける可能性があり、この test 名・期待値は仕様の変化に追随していない疑いが強い。 + - `spawn::tests::profile_choices_use_project_registry_default` + - `spawn::tests::profile_choices_include_builtin_and_project_default_marker` + - 失敗内容: selected index expected `1`, actual `6` + - builtin profiles の数・順序に依存した brittle な assertion になっている。profile selector / default marker / selected profile identity を見るべきで、index 固定は妥当性が低い。 +- UI renderer の実画面に対する coverage は限定的。 + - `ui.rs`, `multi_pod.rs`, `tool.rs` には多数の render helper があるが、terminal buffer snapshot 的な検証は少ない。 + - 文字列 helper の unit test はある一方、layout collapse、small terminal width/height、wide Unicode、style priority、scroll offset と描画結果の統合的な確認は薄い。 +- `tool.rs` の test が見当たらないのは大きな穴。 + - Read/Write/Edit/Search/Bash などの tool call rendering は日常的に目に入る UI であり、diff wrapping、capped output、error suffix、aggregate Read rendering などの regression が入りやすい。 +- 実 terminal / event loop / crossterm raw mode まわりはほぼ unit-test 対象外。 + - project instruction 上 E2E は未設計なのでやむを得ないが、`run_loop`, polling, resize, alternate screen cleanup, Ctrl-C/quit guard の実端末統合は unit tests だけでは保証できない。 +- async / process interaction は一部 mock・TempDir・git repo でよく検証されているが、実 Pod process / socket / session log replay との E2E はない。 + - 特に Panel から child Pod launch、attach、completion notification、reload の end-to-end 整合は crate 単体の test だけでは判断できない。 +- 一部の panel/render tests は text alignment や label の exact string に寄っており、仕様化された文言なら有用だが、見た目調整で壊れやすい。重要 invariant と cosmetic expectation を分けると保守性が上がる。 + +## 追加を提案するテスト + +- まず red tests を仕様に合わせて直す。 + - Orchestrator launch context は `workspace_root`, `role_workspace_root`, `original_workspace_root`, `implementation_worktree_root`, `merge_target_workspace_root`, process cwd のどれを守る test なのかを明確にして、名前と assertion を更新する。 + - Profile choice tests は index ではなく、selected choice の `selector` / `source` / `is_default` 相当の意味で assert する。 +- `tool.rs` に focused unit tests を追加する。 + - Edit diff の old/new wrapping + - multiline result の cap + - failed tool call の error line + - Bash output truncation metadata + - Read aggregate rendering + - narrow width と wide Unicode +- `ratatui::backend::TestBackend` を使った small snapshot tests を少数追加する。 + - single-pod main screen の最小 smoke + - multi-pod panel の ticket row + companion composer + - task side pane / mini view + - very small terminal size で panic しないこと +- scroll / viewport invariant の test を増やす。 + - history scroll と rewind picker scroll が干渉しないことは既にあるが、tool blocks、Markdown long output、task pane open/close、terminal resize 時の offset clamp も対象にする。 +- 実 Pod process まで含む E2E が未設計なら、まずは crate 境界で replay-based tests を足すと効果が高い。 + - saved JSONL fragments / protocol events を `App` に流して expected blocks/actionbar/task state を確認する。 + - 実 socket は使わず、TUI state transition の regression を固定する。 + +## 実行したコマンド + +```text +cargo test -p tui +``` + +結果: + +```text +running 299 tests +test result: FAILED. 296 passed; 3 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.21s +``` + +失敗: + +```text +multi_pod::tests::orchestrator_launch_context_uses_orchestration_root_for_runtime_workspace +spawn::tests::profile_choices_include_builtin_and_project_default_marker +spawn::tests::profile_choices_use_project_registry_default +``` + +```text +cargo test -p tui -- --list +``` + +crate の unit test inventory を確認するために使用した。 + diff --git a/docs/report/test-validity-20260612/workflow.md b/docs/report/test-validity-20260612/workflow.md new file mode 100644 index 00000000..d98b0f7c --- /dev/null +++ b/docs/report/test-validity-20260612/workflow.md @@ -0,0 +1,108 @@ +# テスト妥当性レビュー: workflow +- 判定: 概ね良い + +## 確認範囲 +- Crate: `crates/workflow` +- 確認した責務: + - Workflow フロントマターの分割と schema defaults + - 人間編集向け Workflow linter + - builtin resources と `.yoi/workflow` からの Workflow discovery/loading + - `SKILL.md` parsing と Skill-to-Workflow projection + - Workflow registry の collision/shadowing behavior + - Workflow directory write-deny scope helper +- 主に読んだファイル: + - `crates/workflow/README.md` + - `crates/workflow/Cargo.toml` + - `crates/workflow/src/{lib.rs,schema.rs,linter.rs,workflow.rs,skill.rs,scope.rs,error.rs}` + - 狭い integration 参照: Workflow invocation resolution 周辺の `crates/pod/src/workflow/mod.rs` tests + +## 現在のテストがよくカバーしていること +- この crate には、純粋な parsing/loading 責務に対する堅実な unit-test set がある: `cargo test -p workflow` は 34 unit tests を実行し、すべて成功している。 +- Workflow loading は妥当にカバーされている: + - valid workspace Workflow loading と default flags + - `model_invokation` / `user_invocable` behavior + - workspace Workflow が slug で builtin Workflow を override すること + - 少なくとも 1 つの builtin Workflow の builtin provenance + - invalid filename が hard error になること + - required `description` の欠落が hard error になること + - legacy `.yoi/memory/workflow` が無視されること + - resident description cap enforcement +- Registry/Skill collision behavior がカバーされている: + - collision がない場合の insertion + - workspace Workflow が Skill を shadow すること + - first-fed Skill が later-fed Skill に勝つこと + - human-readable shadow message の smoke check +- `SKILL.md` parsing は主要な invariants をよくカバーしている: + - minimal valid skill + - directory/name mismatch + - invalid slug names + - empty description + - description が cap ちょうど、および cap 超過の場合 + - missing frontmatter + - `allowed-tools` を含む optional spec fields が受け入れられること + - scan behavior: missing root、deterministic ordering、broken sibling を skip しつつ good sibling を保持すること + - Skill-to-Workflow defaults +- Linter tests は現在の重要な linter checks をカバーしている: + - required Knowledge が存在する valid file + - missing required Knowledge + - resident description cap + - Workflow body size limit +- テストは temporary directories を使い、private implementation details よりも observable API behavior を主に assert しており、この crate には適切である。 + +## ギャップ / 疑問のあるテスト +- Builtin Workflow coverage はやや弱い。`missing_directory_loads_builtin_registry` は unrelated slug が存在しないことだけを確認しており、`builtin_workflow_records_have_visible_provenance` は `multi-agent-workflow` だけを確認している。別のテストがたまたま触れない限り、1 つの builtin slug が削除または misconfigure される regression を見逃す可能性がある。 +- Loader と linter の invariants がテスト上で完全には揃っていない。linter は `WORKFLOW_BODY_LIMIT` を enforce するが、`load_workflows` は現在 enforce していない。body size が runtime/load invariant の意図なら loader test がない。意図的に lint-only なら、その境界を明示したままにすべきである。 +- `requires` validation は linter と後段の `pod` invocation resolution でのみテストされており、`load_workflows` ではテストされていない。これは意図的かもしれないが、crate tests だけでは責務分担が明確ではない。 +- linter の Knowledge existence check は filename-stem ベースである。テストは malformed Knowledge files、invalid Knowledge filenames、または「一致する `.md` file が存在する」と「valid Knowledge record が存在する」の違いをカバーしていない。意図する invariant が「valid Knowledge record」なら、現在のテストは許容しすぎている。 +- 複数の hard-error paths が十分にテストされていない: + - Workflow files の malformed YAML frontmatter + - Workflow files の missing frontmatter + - `SKILL.md` の missing `name` / missing `description` + - `.yoi/workflow` 配下の non-`.md` files と subdirectories + - Workflow description cap と body cap の exact boundary +- `skill::tests::invalid_slug_name_is_error` は必要以上に緩い: fixture は uppercase の directory/name が一致しているため、期待される error は `InvalidName` であるべき。`NameDirMismatch` も許容すると、validation ordering や fixture の regression を隠してしまう。 +- `skill::tests::extra_frontmatter_fields_are_kept` は名前がやや不自然である: parsed `SkillRecord` はそれらの optional fields を保持しない。このテストが実際に assert しているのは「optional/spec-compatible fields are accepted and ignored」であり、それ自体は妥当だが、より直接的に表現すべきである。 +- crate-level tests は、「workspace Workflows を load し、configured directories から Skills を load し、registry に merge し、shadowed Skills を report する」という full pipeline を exercise していない。隣接する一部 behavior は `pod` でカバーされているが、`workflow` crate 自体では pieces を独立にテストしているだけである。 + +## 追加するとよいもの +- builtin assertions を強化する: + - 期待されるすべての builtin slugs が存在することを assert する + - それらの `source`, `path`, `model_invokation`, `user_invocable`, および選択した `requires` values を assert する + - builtin resource changes が明確に fail するよう、小さな contract test として維持する +- Workflow loader の negative/edge tests を追加する: + - missing frontmatter + - malformed YAML + - `model_invokation: true` のとき description cap ちょうどが accepted になること + - non-`.md` files が ignored されること + - subdirectories が ignored されること + - workspace Workflow が `requires` values を preserve すること + - body limit が lint-only なのか load-time でもあるのかをテストで明確にする +- linter edge tests を追加する: + - body が `WORKFLOW_BODY_LIMIT` ちょうどなら accepted + - resident description が cap ちょうどなら accepted + - 複数の `requires` がすべての missing references を report すること + - invalid `requires` slug が malformed frontmatter または slug parse error になること + - valid Knowledge parsing が意図されている場合、malformed Knowledge record behavior +- Skill tests を引き締める: + - matching invalid directory/name に対して invalid slug test が `SkillParseError::InvalidName` を assert するようにする + - missing `name` と missing `description` を追加する + - optional-field test を、それらの fields が accepted/ignored されることを表す名前または内容にする +- parsed Skills からの registry assembly に対する crate-level integration-style unit test を 1 つ追加する: + - builtin/workspace registry を load する + - 2 つの Skill directories を parse する + - priority order で merge する + - accepted Skill、shadowed Skill、および結果の user-invocable/resident entries を assert する + +## 実行したコマンド +- `cargo test -p workflow` + - 結果: passed + - 34 unit tests passed, 0 failed; doc-tests: 0 tests +- `cargo test -p workflow -- --list` + - 結果: passed + - 34 unit tests, 0 benchmarks を確認; doc-tests: 0 tests +- `git status --short` + - Read-only check; この review の変更外に既存の modified paths があることを示した: + - `.yoi/tickets/00001KTVPS6K3/item.md` + - `.yoi/tickets/00001KTVPS6K3/thread.md` + - `crates/tui/src/multi_pod.rs` + diff --git a/docs/report/test-validity-20260612/yoi.md b/docs/report/test-validity-20260612/yoi.md new file mode 100644 index 00000000..c43b969f --- /dev/null +++ b/docs/report/test-validity-20260612/yoi.md @@ -0,0 +1,149 @@ +# テスト妥当性レビュー: yoi +- 評価: 混在 + +## 確認範囲 + +`/home/hare/Projects/yoi/crates/yoi` 配下の `yoi` crate を確認した: + +- `crates/yoi/Cargo.toml` +- `crates/yoi/src/main.rs` +- `crates/yoi/src/ticket_cli.rs` +- `crates/yoi/src/objective_cli.rs` +- `crates/yoi/src/memory_lint.rs` +- `crates/yoi/src/session_cli.rs` + +この crate はプロダクトのバイナリ境界である。責務はトップレベル CLI の parse/dispatch、TUI/panel 起動選択、`yoi pod` 委譲、`yoi ticket ...`、`yoi memory lint`、`yoi session analyze`、および objective-record CLI の処理である。深いドメイン挙動の大半は他 crate（`ticket`、`memory`、`session-analytics`、`tui`、`pod`）に委譲されている。 + +この crate には 48 個の unit test があり、独立した integration test ファイルはない。 + +## 現在のテストがよくカバーしていること + +- `main.rs` のトップレベル引数 parsing は、重要な routing 判断をカバーしている: + - positional Pod name vs `--pod` + - `pod` runtime subcommand の pass-through + - `ticket`、`session`、`memory lint`、`panel`、`keys`、`setup-model` + - `--profile` の互換性制約 + - `--resume` / Pod-name の衝突 + - Pod identity を伴う明示的な `--session` + - `--multi` が launch alias として受理されないこと + +- `ticket_cli.rs` には `TempDir` を使った比較的強い in-process CLI test がある: + - `init` scaffold 作成と非上書き挙動 + - `create`、`list`、`show`、`comment`、`review`、`state`、`close`、`doctor` + - configured backend root + - relation add/list/show rendering + - list bounding と title truncation + - 曖昧な body source と review result の拒否 + - `state closed` が専用の `close` command を要求すること + +- `memory_lint.rs` は主要な CLI 固有 invariants をカバーしている: + - option parsing と usage error + - 意図した memory/Knowledge record path だけが lint されること + - invalid record が lint failure になること + - `--warnings-as-errors` が status を変えること + - JSON output が parse 可能で、期待される counts を含むこと + +- `objective_cli.rs` は基本的な record lifecycle をカバーしている: + - create/list/show + - 生成される opaque record id の形 + - linked Ticket validation + - linked-ticket state に対する doctor success/failure + +- `session_cli.rs` は以下をカバーしている: + - `analyze --json` parsing + - 最小 JSONL fixture からの output が valid JSON であること + - 初期 non-JSON output mode の拒否 + +## 不足 / 疑問のあるテスト + +- 現在の `yoi` crate test suite は pass しない。`cargo test -p yoi` は `ticket_cli::tests::ticket_cli_init_writes_explicit_ticket_config_scaffold` で決定的に失敗する。 + - 失敗している assertion は、scaffold role profiles に `profile = "builtin:default"` が含まれることを期待している。 + - 現在の `ticket::config::ticket_config_scaffold()` は、`role.default_profile()` 経由で `builtin:intake`、`builtin:orchestrator`、`builtin:coder`、`builtin:reviewer` のような role-specific builtin profiles を出力する。 + - 対応する `ticket` crate の scaffold test は pass しているため、これは scaffold generation 自体が壊れている証拠というより、`yoi` boundary test 内の stale duplicate assertion に見える。 + - 影響: この suite は現在、この crate に対する有効な green regression signal になっていない。 + +- `ticket_cli_init_writes_explicit_ticket_config_scaffold` は copied scaffold text に密結合しすぎている。`yoi` crate は、おそらく `yoi ticket init` がファイルを書き、そのファイルが期待される semantic config として parse できることを assert すべきであり、正確な role default は主に `ticket` crate tests の責務である。 + +- installed binary behavior に対する subprocess-level test がない。in-process test は有用で速いが、以下を検証していない: + - `main` からの実際の exit code + - parse/runtime error における stderr formatting + - real binary 経由の stdout behavior + - `std::env::current_dir()` を呼ぶ command の current-directory behavior + - packaging/runtime resource の影響 + +- トップレベル parser coverage は良いが、hand-written parser としては完全ではない。欠けている、または薄い case には以下がある: + - 引数なしの default spawn mode + - `--workspace=...`、`--pod=...`、`--socket=...`、`--profile=...`、`--session=...` + - 重複した positional Pod names + - Pod name なしの `--socket` + - `--session` と併用した `--socket` + - `--session` と併用した `--resume` + - `panel` の malformed argument cases + +- `ticket_cli.rs` は良い happy-path lifecycle をカバーしているが、多くの CLI error と file-boundary case がまだない: + - ほとんどの flag に対する required value 欠落 + - invalid `--state`、`--limit`、`--role`、relation kind + - `--message`/`--file` 欠落 + - 読めない `--file` + - comment/review/close に対する `--file` bodies + - invalid configured backend provider/root config + - malformed Ticket records に対する doctor failure rendering + +- `objective_cli.rs` は自身の Markdown records を書き、validate するため、test surface は現在より強くあるべきである。欠けている case には以下がある: + - missing/invalid frontmatter + - invalid state + - required section headings の欠落 + - duplicate linked tickets warning + - `show` における path traversal / invalid id rejection + - paused/done/archived/all に対する list filtering + +- `memory_lint.rs` には妥当な focused tests があるが、decisions と Knowledge records を明示的にはカバーしておらず、symlink/directory の特殊性、読めない file、summary fragments を超えた stdout human-output details もカバーしていない。 + +- `session_cli.rs` は意図的に薄いが、ほとんどは最小 fixture を通じて delegated analytics output shape を検証している。より広い analytics correctness は `session-analytics` に属する。`yoi` crate には、unknown subcommands/options と duplicate path handling の test がまだない。 + +## 追加提案 + +- まず失敗している scaffold test を修正する。以下のどちらかを推奨する: + - `builtin:default` ではなく `role.default_profile()` を assert するように更新する、または + - `yoi` test を semantic にする: `ticket init` を実行し、生成された config を `TicketConfig::from_toml` / `load_workspace` で parse し、各 fixed role が launch config を持つことを assert する。正確な scaffold-string coverage は `crates/ticket` に残す。 + +- 完全な E2E design の前でも、binary boundary に対するごく小さい subprocess smoke suite を追加する: + - `yoi --help` が 0 で exit し、core commands を出力する + - unknown top-level arg が non-zero で exit し、`try yoi --help` hint を書く + - temp cwd で `yoi ticket init` が 0 で exit し、`.yoi/ticket.config.toml` を作る + - `yoi memory lint --workspace --json` が適切に 0/1 で exit する + - `yoi session analyze --json` が 0 で exit し、JSON を出力する + +- トップレベル edge cases に対して、特に `--flag=value` forms と invalid combinations の table-driven parser tests を追加する。これは安価で、既存の unit-test style に合っている。 + +- `ticket_cli` parsing と body-file handling に対する CLI 固有の negative tests を追加する: + - missing `--title` + - invalid list state / limit + - missing body source + - unreadable `--file` + - invalid relation kind + - invalid comment role + +- malformed persisted records 周辺の objective CLI tests を強化する。この code は validation を別 crate にすべて委譲するのではなく、自身で storage format を所有しているためである。 + +- domain-deep tests は owning crates に置き続ける。`yoi` では、CLI mapping、filesystem boundary behavior、output/status contract、delegated crates との integration に focus する。 + +## 実行したコマンド + +- `cargo test -p yoi` + - Result: failed. + - Summary: 47 passed, 1 failed. + - Failure: `ticket_cli::tests::ticket_cli_init_writes_explicit_ticket_config_scaffold` + - Failure reason: assertion が generated role config に `profile = "builtin:default"` が含まれることを期待していた。 + +- `cargo test -p yoi ticket_cli::tests::ticket_cli_init_writes_explicit_ticket_config_scaffold -- --exact` + - Result: failed. + - 失敗する test が deterministic かつ isolated であることを確認した。 + +- `cargo test -p ticket config::tests::scaffold_config_includes_backend_and_all_fixed_roles -- --exact` + - Result: passed. + - これは、owning `ticket` crate の scaffold test は現行であり、`yoi` test に stale duplicate expectation があるという解釈を支持する。 + +- `cargo test -p yoi -- --list` + - Result: 48 tests, 0 benchmarks を listed。 +