45 lines
6.8 KiB
Markdown
45 lines
6.8 KiB
Markdown
# Review: Bash ツール
|
||
|
||
## 前提・要件の確認
|
||
|
||
- **コマンド実行 (`tokio::process::Command`)**: 満たされている。`crates/tools/src/bash.rs:94-103` で `bash -c <wrapped>` を起動。`stdin(null)` で stdin ブロックを防止、`kill_on_drop(true)` でタイムアウト時のリーク防止。
|
||
- **timeout (default 120s / max 600s)**: 満たされている。`bash.rs:38-39, 64-67` の `clamp(1, 600)`、`bash.rs:130-144` の `tokio::time::timeout`。`timeout_kills_long_command` で動作確認済み。
|
||
- **作業ディレクトリの永続**: 満たされている。`cd` のパースに頼らず wrapper script + tempfile で post-command の `pwd` を取得（`bash.rs:74-90`）。`cd_persists_across_calls` テストで `subdir` 移動後の `pwd` が反映されることを確認。`canonicalize` 同士で比較しており macOS の `/private/tmp` ずれにも耐性あり。
|
||
- **stdout/stderr 結合**: 満たされている。wrapper 内 `exec 2>&1` で実装、`merges_stdout_and_stderr` テストで両方含まれることを確認。子プロセス側の `stderr(Stdio::null())` も整合。
|
||
- **`ToolOutput` summary（コマンド + exit code）+ content（出力）**: 満たされている。`bash.rs:164-175` で exit 0 / 非 0 / シグナルを区別。content が空のときは `None` を返しており、`SUMMARY_THRESHOLD` を意識した良い実装。
|
||
|
||
## アーキテクチャ・スコープ
|
||
|
||
- **層分離**: `tools` クレート内に閉じており、`llm-worker` を低レベル基盤に保つ方針と整合（`bash.rs:20` で `Tool` trait のみ依存）。`builtin_tools()` のファクトリ列に追加するだけで、層を跨ぐ侵入はない。
|
||
- **クレート命名/構造**: `bash.rs` を独立モジュールに切り出し、`lib.rs` で `pub use bash::bash_tool` のみ公開。`read.rs/write.rs/...` と一貫。
|
||
- **依存追加**: `Cargo.toml` の tokio features に `process`/`time`/`io-util`/`sync` を追加（`Cargo.toml:22`）。`tempfile` は既存。`cargo add` 経由前提のフィールド追加で違和感なし。
|
||
- **Permission 層との関係**: ticket の前提通り、ScopedFs では保護せず Permission 層に委譲。`lib.rs:18-19` のドキュメントコメントで明示しており、設計意図は読み手に伝わる。
|
||
- **設計判断 1（wrapper による pwd 取得）**: `cd` パースの脆さ（サブシェル、変数展開、関数定義内 `cd` 等）を回避できるので妥当。`exec` で bash 自体が置換されると wrapper が走らないが、`bash.rs:149-155` が「ファイル読めなければ pwd 据え置き」とフェイルソフトしておりロバスト。
|
||
- **設計判断 2（wrapper の `wait`）**: `(sleep 0.05; echo bg) &` のようなジョブで stdout が EOF せずハングする問題に対する実装上必須の対処。`background_job_does_not_hang` で回帰防止済み。
|
||
- **設計判断 3（`tokio::sync::Mutex` で逐次化）**: pwd の共有可変状態と「順序のある shell セッション」の意味論を考えると正解。長時間コマンドの間 lock を握り続けるのは仕様上自然（同一セッションの bash は元々直列）。
|
||
- **設計判断 4（256KB cap）**: worker 側 `ToolOutputLimits` の手前で OOM を抑える二重防壁。truncated marker の追記後に `String::from_utf8_lossy` で UTF-8 化しており、マルチバイト切断もロスレスではないが panic はしない。妥当。
|
||
- **設計判断 5（summary/content）**: 既存ツールと API 形状が一致。`SUMMARY_THRESHOLD` の境界も意識されている。
|
||
- **設計判断 6（description のプロンプト誘導）**: Read/Write/Edit/Glob/Grep を優先させる文言は、Claude Code リファレンスとも整合し、ローカルモデルでも効きやすい簡潔さ。
|
||
|
||
## 指摘事項
|
||
|
||
### Non-blocking / Follow-up
|
||
|
||
- **TUI 側の `render_default` 修正の同梱について** (`crates/tui/src/tool.rs:590-619`)
|
||
- 内容としては正しいバグ修正。Bash のような汎用ツールが Detail モードでも summary しか出ない状態を解消している。
|
||
- ただし、厳密には Bash チケットの範囲外（既存の任意の "default 経路の" ツールに同じ問題があったはず）。同梱の妥当性: Bash 投入によりバグが顕在化したこと、5 行程度の置き換えで完結すること、Bash 単体だと UX として未完であることを踏まえれば現実的な判断と言える。次回同種の状況では、TUI 表示仕様の修正として別チケットを切るほうがレビュー単位がきれいになる、というレベル。
|
||
- フォローアップ提案: `crates/tui/` 配下に `output` を含むレンダリングが Detail/Normal で正しく出ることを確認するスナップショット/ユニットテストを 1 本追加すると、将来の `summary` フォールバック方向への意図しない退行を防げる（現状はロジックレビューのみで担保）。
|
||
|
||
- **`docs/ref/claude-code-deferred-tools.md` への追記**: Bash 実装と直接関係しない文献参照の追加（Anthropic vs OpenAI 比較への言及）。1 段落で軽微とはいえ、チケットスコープからは外れている。次回はドキュメント更新も別コミット/別チケット推奨。
|
||
|
||
- **pwd 更新の堅牢性についての観察 (`bash.rs:149-155`)**: ユーザーコマンドが `exec some-program` で bash を置換した場合や、wrapper の `pwd > tempfile` がディスクフル等で失敗した場合に pwd が据え置かれる挙動になっている。仕様上は妥当だが、ユーザー視点では「`cd foo && exec bar` 後に `cd` が消えた」ように見える可能性がある。コメントで現挙動の合理性は説明されているので blocking ではないが、将来 Permission 層導入時にエッジケースとして再考の余地あり。
|
||
|
||
### Nits
|
||
|
||
- `BashParams` の `timeout` フィールドが `Option<u64>` で `#[serde(default)]` だが、`Option` は serde が自動的に欠落を `None` にするため `#[serde(default)]` は冗長（害はない）。
|
||
- `bash.rs:111-112` の `let mut child = child; let mut stdout = stdout;` は `async move` ブロックで mutable に再束縛しているだけ。慣用的だが `let mut` を引数側で書いてもよい。スタイル差。
|
||
|
||
## 判断
|
||
|
||
**Approve with follow-up** — チケット要件は完全に満たされており、設計判断もすべて合理的に説明されている。テストカバレッジ (8 unit + 1 integration) も妥当。同梱されている TUI 修正は実害のあるバグ修正で内容は正しいが、本来は別チケット相当のスコープ越えがあり、回帰テストの追加は次回までのフォローアップとして残しておくとよい。
|