From 4c335d5e426277d3a22936c5a867f5483230585e Mon Sep 17 00:00:00 2001 From: jimmyfraiture Date: Wed, 1 Oct 2025 14:44:46 +0100 Subject: [PATCH] Drop MD --- codex-rs/tools.md | 98 ----------------------------------------------- 1 file changed, 98 deletions(-) delete mode 100644 codex-rs/tools.md diff --git a/codex-rs/tools.md b/codex-rs/tools.md deleted file mode 100644 index cdc8eba32b..0000000000 --- a/codex-rs/tools.md +++ /dev/null @@ -1,98 +0,0 @@ -# Tool Handling Refactor Plan - -## Current Pain Points -- Tool specs and dispatch live in multiple files (`openai_tools.rs`, `codex.rs`, `shell.rs`, `apply_patch.rs`, `exec_command`, `plan_tool`), so every tool is stitched together by hand with duplicated string matches. -- `codex.rs` mixes concerns: argument parsing, apply-patch verification, approval policy enforcement, telemetry, and response construction for Function/Custom/LocalShell calls are all intertwined. -- Adding or adjusting a tool requires touching 5–7 sites and remembering subtle invariants (e.g., apply_patch needs extra verification, view_image requires path resolution), which increases review overhead and bug risk. -- Spec configuration (`ToolsConfig`, `get_openai_tools`) is disconnected from runtime dispatch, so there is no single registry that knows which tools are active or how to invoke them. - -## Goals -- Centralize tool definitions, specs, and dispatching inside a dedicated `tools` module with clearly defined traits and types. -- Unify handling for function calls, custom calls, and local shell calls, sharing approval checks, apply_patch guards, and telemetry. -- Slim down `codex.rs` so it only converts `ResponseItem` into a `ToolCall` and delegates the rest. -- Make new tool onboarding require (at most) one handler + registration entry + optional tests. - -## Core Traits & Types -- ```rust - pub trait ToolHandler: Send + Sync { - fn spec(&self) -> Option<&ToolSpec>; - fn kind(&self) -> ToolKind; - async fn handle(&self, invocation: ToolInvocation<'_>) -> Result; - } - ``` - - `spec`: supplies the OpenAI tool definition for function/custom tools (returns `None` for implicit tools like local shell). - - `kind`: declares whether the handler services `ToolCall::Function`, `ToolCall::Custom`, `ToolCall::LocalShell`, etc., for dispatch filtering. - - `handle`: runs the tool logic, returning structured output + optional streaming side effects. -- ```rust - pub struct ToolInvocation<'a> { - pub session: &'a Session, - pub turn: &'a TurnContext, - pub tracker: &'a mut TurnDiffTracker, - pub sub_id: &'a str, - pub call_id: &'a str, - pub payload: ToolPayload<'a>, - } - ``` - - `payload` captures parsed arguments (`serde_json::Value` for functions/custom, `ShellToolCallParams` for local shell, etc.). -- ```rust - pub enum ToolOutput { - Function(FunctionCallOutputPayload), - Custom(String), - } - ``` - - Helper constructors convert to `ResponseInputItem` in one place. -- ```rust - pub struct ToolRegistry { - handlers: HashMap>, - } - ``` - - Provides `register_handler`, `iter_specs()` for prompt assembly, and `dispatch(call: ToolCall) -> Result`. - -## Proposed Refactor -1. **Scaffold `core/src/tools` Module** - - `mod.rs` re-exports `context`, `registry`, `router`, `spec`, and `handlers::*`. - - `context.rs` defines `ToolInvocation`, `ToolPayload`, `ToolOutput`, helper constructors for `ResponseInputItem`. - - `registry.rs` implements `ToolRegistry` + `ToolKind` enum and houses the instrumentation wrapper (`otel.log_tool_result`, approval policy checks). - - `router.rs` provides `ToolCall` enum (variants: `Function`, `Custom`, `LocalShell`, `UnifiedExec`, `Mcp`), plus `Router::from_turn_context(&TurnContext)` to build the registry + spec list together. - -2. **Move Spec Logic Into `tools::spec`** - - Relocate `ToolsConfig`, `ToolsConfigParams`, `get_openai_tools`, `ConfigShellToolType`, and tool builders (shell/unified exec/apply_patch/etc.) into `spec.rs`. - - Add `ToolSpec` struct mirroring current `OpenAiTool`, keeping existing tests but updated paths. - - Expose `pub fn build_specs(config: &ToolsConfig, mcp: Option>) -> (Vec, ToolRegistryBuilder)` to keep spec generation and handler registration in sync. - -3. **Implement Concrete Handlers** - - Split existing logic into focused files: - 1. `handlers/shell.rs`: wraps `handle_container_exec_with_params`, approval policy enforcement, apply_patch verification, and streaming support. - 2. `handlers/apply_patch.rs`: retains apply_patch-specific parsing when invoked as a function/custom tool. - 3. `handlers/unified_exec.rs`: hosts `handle_unified_exec_tool_call` logic. - 4. `handlers/plan.rs`: wraps `handle_update_plan`. - 5. `handlers/view_image.rs`: handles local path resolution + `inject_input` call. - 6. `handlers/exec_stream.rs`: handles `exec_command` and `write_stdin` (for streamable exec shell variant). - 7. `handlers/mcp.rs`: optional adapter that calls through to `handle_mcp_tool_call` while emitting consistent telemetry. - - Each handler implements `ToolHandler` with explicit payload parsing (`serde_json::from_value` helpers) and reuses shared utilities (`create_env`, `maybe_parse_apply_patch_verified`). - -4. **Registry Wiring** - - Add `ToolRegistryBuilder` helper collecting `(ToolName, Arc)` pairs. - - Builder methods (e.g., `with_shell_handler`, `with_apply_patch_handler`) are invoked from `spec::build_specs` based on `ToolsConfig` decisions so that enabling a spec automatically registers the handler. - - Integrate MCP tools by registering a generic `McpHandler` per tool when MCP discovery runs. - -5. **Update `codex.rs`** - - Replace `handle_function_call`, `handle_custom_tool_call`, `to_exec_params`, and `handle_container_exec_with_params` with: - - `let router = tools::router::Router::new(sess, turn_context, turn_diff_tracker);` - - Convert `ResponseItem` into `ToolCall` and call `router.dispatch(...)`. - - Remove duplicate instrumentation; `ToolRegistry` handles `log_tool_result` and error wrapping. - - Keep only MCP discovery + fallback logic, delegating to `ToolRegistry` for execution. - -6. **Tests & Validation** - - Port existing unit tests in `openai_tools.rs` to `tools::spec` and add new tests for `ToolRegistry` covering success, error, and approval-policy rejection paths. - - Add smoke tests for converting `ResponseItem` to `ToolCall` in `tools::router`. - - Ensure apply_patch verification works both for function and local shell flows via dedicated tests (mock `apply_patch::apply_patch`). - -## Testing & Rollout -- Run `cargo test -p codex-core tools::spec` (once added) plus focused handler tests. -- Execute existing integration suites that cover exec/apply_patch/unified_exec flows. -- No behavior change expected, but snapshot suites (esp. TUI if any output differs) should be re-run as a sanity check. - -## Follow-ups -- With registry centralization, expose structured telemetry (counts, durations) and hook into metrics. -- Evaluate deletion of legacy local shell pathway once unified_exec proves stable.