diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index 9a838304a..43e8248d7 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -44,7 +44,7 @@ version = "0.9.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "c2ffe7d502c1e451aafc5aff655000f84d09c9af681354ac0012527009b1af13" dependencies = [ - "agent-client-protocol-schema", + "agent-client-protocol-schema 0.10.1", "anyhow", "async-broadcast", "async-trait", @@ -55,6 +55,19 @@ dependencies = [ "serde_json", ] +[[package]] +name = "agent-client-protocol-schema" +version = "0.6.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "16d08d095e8069115774caa50392e9c818e3fb1c482ef4f3153d26b4595482f2" +dependencies = [ + "anyhow", + "derive_more 2.0.1", + "schemars 1.0.4", + "serde", + "serde_json", +] + [[package]] name = "agent-client-protocol-schema" version = "0.10.1" @@ -661,6 +674,12 @@ dependencies = [ "piper", ] +[[package]] +name = "boxfnonce" +version = "0.1.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "5988cb1d626264ac94100be357308f29ff7cbdd3b36bda27f450a4ee3f713426" + [[package]] name = "bstr" version = "1.12.0" @@ -1320,7 +1339,7 @@ dependencies = [ "libc", "path-absolutize", "pretty_assertions", - "rmcp", + "rmcp 0.10.0", "serde", "serde_json", "shlex", @@ -1596,7 +1615,7 @@ dependencies = [ "oauth2", "pretty_assertions", "reqwest", - "rmcp", + "rmcp 0.10.0", "serde", "serde_json", "serial_test", @@ -3483,17 +3502,6 @@ dependencies = [ "rustversion", ] -[[package]] -name = "io-uring" -version = "0.7.9" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "d93587f37623a1a17d94ef2bc9ada592f5465fe7732084ab7beefabe5c77c0c4" -dependencies = [ - "bitflags 2.10.0", - "cfg-if", - "libc", -] - [[package]] name = "ioctl-rs" version = "0.1.6" @@ -3631,6 +3639,16 @@ dependencies = [ "wasm-bindgen", ] +[[package]] +name = "jsonrpcmsg" +version = "0.1.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "6d833a15225c779251e13929203518c2ff26e2fe0f322d584b213f4f4dad37bd" +dependencies = [ + "serde", + "serde_json", +] + [[package]] name = "keyring" version = "3.6.3" @@ -5308,6 +5326,27 @@ dependencies = [ "windows-sys 0.52.0", ] +[[package]] +name = "rmcp" +version = "0.8.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e5947688160b56fb6c827e3c20a72c90392a1d7e9dec74749197aa1780ac42ca" +dependencies = [ + "base64", + "chrono", + "futures", + "paste", + "pin-project-lite", + "rmcp-macros 0.8.5", + "schemars 1.0.4", + "serde", + "serde_json", + "thiserror 2.0.17", + "tokio", + "tokio-util", + "tracing", +] + [[package]] name = "rmcp" version = "0.10.0" @@ -5328,7 +5367,7 @@ dependencies = [ "process-wrap", "rand 0.9.2", "reqwest", - "rmcp-macros", + "rmcp-macros 0.10.0", "schemars 1.0.4", "serde", "serde_json", @@ -5343,6 +5382,19 @@ dependencies = [ "uuid", ] +[[package]] +name = "rmcp-macros" +version = "0.8.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "01263441d3f8635c628e33856c468b96ebbce1af2d3699ea712ca71432d4ee7a" +dependencies = [ + "darling 0.21.3", + "proc-macro2", + "quote", + "serde_json", + "syn 2.0.104", +] + [[package]] name = "rmcp-macros" version = "0.10.0" @@ -5485,6 +5537,78 @@ version = "1.0.20" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "28d3b2b1366ec20994f1fd18c3c594f05c5dd4bc44d8bb0c1c632c8d6829481f" +[[package]] +name = "sacp" +version = "2.0.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "db6db3649202a5381e4b80e4cd33165e1d3416df501f78361e650dfe9e848226" +dependencies = [ + "agent-client-protocol-schema 0.6.3", + "anyhow", + "boxfnonce", + "futures", + "jsonrpcmsg", + "serde", + "serde_json", + "thiserror 2.0.17", + "tracing", + "uuid", +] + +[[package]] +name = "sacp-proxy" +version = "3.0.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "0d6540b3f66a016a710b4c55016e3c42213e6ddac23e79a7387ae2a49297f9eb" +dependencies = [ + "agent-client-protocol-schema 0.6.3", + "futures", + "fxhash", + "rmcp 0.8.5", + "sacp", + "schemars 1.0.4", + "serde", + "serde_json", + "tokio", + "tokio-util", + "tracing", + "uuid", +] + +[[package]] +name = "sacp-tee" +version = "0.1.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "9bb5c45af7b157bc53c5d31b09c7241653781fba23941e9fa1d1651920b0fe03" +dependencies = [ + "anyhow", + "chrono", + "clap", + "sacp", + "sacp-proxy", + "sacp-tokio", + "serde", + "serde_json", + "tokio", + "tracing", + "tracing-subscriber", +] + +[[package]] +name = "sacp-tokio" +version = "2.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ce9d41464fadd415f7b7c040aaafdaceacdc785cbe23954aa5011687a91bf479" +dependencies = [ + "futures", + "sacp", + "serde", + "serde_json", + "shell-words", + "tokio", + "tokio-util", +] + [[package]] name = "same-file" version = "1.0.6" @@ -6716,29 +6840,26 @@ checksum = "1f3ccbac311fea05f86f61904b462b55fb3df8837a366dfc601a0161d0532f20" [[package]] name = "tokio" -version = "1.47.1" +version = "1.49.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "89e49afdadebb872d3145a5638b59eb0691ea23e46ca484037cfab3b76b95038" +checksum = "72a2903cd7736441aac9df9d7688bd0ce48edccaadf181c3b90be801e81d3d86" dependencies = [ - "backtrace", "bytes", - "io-uring", "libc", "mio", "parking_lot", "pin-project-lite", "signal-hook-registry", - "slab", "socket2 0.6.0", "tokio-macros", - "windows-sys 0.59.0", + "windows-sys 0.61.1", ] [[package]] name = "tokio-macros" -version = "2.5.0" +version = "2.6.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "6e06d43f1345a3bcd39f6a56dbb7dcab2ba47e68e8ac134855e7e2bdbaf8cab8" +checksum = "af407857209536a95c8e56f8231ef2c2e2aff839b22e07a1ffcbc617e9db9fa5" dependencies = [ "proc-macro2", "quote", @@ -7002,6 +7123,16 @@ dependencies = [ "tracing-core", ] +[[package]] +name = "tracing-serde" +version = "0.2.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "704b1aeb7be0d0a84fc9828cae51dab5970fee5088f83d1dd7ee6f6246fc6ff1" +dependencies = [ + "serde", + "tracing-core", +] + [[package]] name = "tracing-subscriber" version = "0.3.20" @@ -7012,12 +7143,15 @@ dependencies = [ "nu-ansi-term", "once_cell", "regex-automata", + "serde", + "serde_json", "sharded-slab", "smallvec", "thread_local", "tracing", "tracing-core", "tracing-log", + "tracing-serde", ] [[package]] @@ -7135,6 +7269,7 @@ dependencies = [ "owo-colors", "portable-pty 0.8.1", "regex", + "sacp-tee", "tempfile", "vt100 0.15.2", ] diff --git a/codex-rs/acp/docs.md b/codex-rs/acp/docs.md index e10356c14..f23370763 100644 --- a/codex-rs/acp/docs.md +++ b/codex-rs/acp/docs.md @@ -221,6 +221,396 @@ The `init_rolling_file_tracing()` function in `@/codex-rs/acp/src/tracing_setup. - ANSI colors disabled for clean file output - Automatically initialized by the CLI (`@/codex-rs/cli/src/main.rs`) at startup, writing to `$NORI_HOME/log/nori-acp.YYYY-MM-DD` +### ACP Traffic Tracing + +**Note: This feature is only available in debug builds and is automatically disabled in release builds.** + +When enabled via `--acp-trace` CLI flag (debug builds only) or `acp_trace_enabled = true` in config, the ACP module captures all JSON-RPC 2.0 traffic between the client and agent subprocess using the `sacp-tee` proxy from the symposium-acp library. + +**Configuration Flow:** + +``` +┌─────────────────────────┐ acp_trace_enabled ┌─────────────────────────┐ +│ Config / CLI Flag │────────────────────────────►│ TUI spawn_acp_agent() │ +│ │ │ │ +│ --acp-trace flag │ │ Builds AcpTraceConfig │ +│ acp.trace_enabled │ │ with session log path │ +└─────────────────────────┘ └───────────┬─────────────┘ + │ + ▼ +┌─────────────────────────┐ trace_config: Option ┌─────────────────────────┐ +│ AcpBackendConfig │◄────────────────────────────│ AcpBackend::spawn() │ +│ - trace_config field │ │ │ +└───────────┬─────────────┘ └─────────────────────────┘ + │ + ▼ +┌─────────────────────────┐ build_agent_command() ┌─────────────────────────┐ +│ AcpConnection::spawn()│────────────────────────────►│ Wrapped Command │ +│ │ │ │ +│ Passes trace_config │ │ sacp-tee --log-file │ +│ to internal spawn │ │ -- │ +└─────────────────────────┘ └─────────────────────────┘ +``` + +**Key Types:** + +- `AcpTraceConfig`: Holds the log file path for a session (exported from `connection.rs`) +- `AcpBackendConfig.trace_config`: Optional field passed from TUI to backend +- `build_agent_command()`: Wraps the agent command with `sacp-tee` when tracing is enabled + +**Log File Naming:** + +Session-specific log files are created at `{codex_home}/log/acp-trace-{timestamp}-{pid}.log`: +- `timestamp`: Unix timestamp in milliseconds at session creation +- `pid`: Process ID of the Nori process +- Log directory is created automatically if it doesn't exist + +**Command Wrapping:** + +When `trace_config` is `Some`, `build_agent_command()` transforms: +``` +Original: gemini-cli --experimental-acp +Wrapped: sacp-tee --log-file /path/to/log.log -- gemini-cli --experimental-acp +``` + +The `sacp-tee` proxy intercepts all stdio traffic and logs JSON-RPC messages while passing them through unchanged. Assumes `sacp-tee` is installed and available in PATH when enabled. + +**Debug-Only Enforcement:** + +The tracing feature is enforced at compile time using conditional compilation: +- The `--acp-trace` CLI flag only exists in debug builds (`#[cfg(debug_assertions)]`) +- The `build_agent_command()` function ignores trace config in release builds +- Config fields for `acp_trace_enabled` flow through all code unchanged, but are silently ignored at the command spawn boundary +- This ensures zero runtime overhead in production builds while keeping the codebase simple + +### Core Implementation + +**Thread-Safe Connection Wrapper (`connection.rs`):** + +The ACP library uses `LocalBoxFuture` which is `!Send`, preventing direct use in codex-core's multi-threaded tokio runtime. The solution is a thread-safe wrapper pattern: + +``` +┌─────────────────────────┐ mpsc channels ┌─────────────────────────┐ +│ Main Tokio Runtime │◄───────────────────►│ ACP Worker Thread │ +│ │ AcpCommand enum │ (single-threaded RT) │ +│ AcpConnection │ │ │ +│ - spawn() │ ────────────────► │ AcpConnectionInner │ +│ - create_session() │ CreateSession │ - ClientDelegate │ +│ - prompt() │ Prompt │ - run_command_loop() │ +│ - cancel() │ Cancel │ - model_state Arc │ +│ - set_model() [unst] │ SetModel [unstable]│ │ +│ │ ◄──────────────── │ │ +│ │ oneshot responses │ │ +└─────────────────────────┘ └─────────────────────────┘ +``` + +Model state is stored in `Arc>` shared between the main thread and worker thread for thread-safe access. + +- `AcpConnection::spawn()` creates dedicated thread with `LocalSet` for `!Send` futures +- Commands sent via `mpsc::Sender` to worker thread +- Responses returned via `oneshot` channels embedded in commands +- Worker thread spawns subprocess, handles JSON-RPC handshake, runs command loop + +**Subprocess Lifecycle Management:** + +The `run_command_loop()` function manages agent subprocess cleanup: +- Runs until the command channel is closed (when `AcpConnection` is dropped) +- On exit, calls `child.kill()` to terminate the subprocess +- This prevents orphaned/zombie processes when sessions are switched (e.g., via `/new` command) +- Logs subprocess PID at spawn via `debug!("ACP agent spawned (pid: {:?})")` for E2E test verification + +**ClientDelegate (`connection.rs`):** + +- Implements `acp::Client` trait to handle agent requests +- Routes session updates to registered `mpsc::Sender` channels +- Bridges permission requests to Codex approval system via `ApprovalRequest` channel +- Implements file read (synchronous `std::fs::read_to_string`) +- Terminal operations return `method_not_found` (not yet supported) +- Implements file write (`write_text_file`) with relative path resolution and security boundaries + +**File Write Implementation:** + +The `write_text_file` method implements file creation and modification for ACP agents with security boundaries: + +1. **Relative Path Resolution**: Paths like `file.txt` are resolved against the workspace directory (`cwd`) before validation, so agents can use simple relative paths for workspace files + +2. **Security Boundaries**: Application-level path restrictions (temporary until OS-level sandboxing is deployed): + - Workspace writes: Any path within or under the workspace directory + - Temporary writes: Any path under `/tmp` directory + - System paths: All other paths are rejected with an error + +3. **Auto-create Directories**: Parent directories are created automatically using `std::fs::create_dir_all` when needed + +4. **Atomicity**: Not currently atomic - partial writes can occur if interrupted + +The path validation canonicalizes paths to prevent symlink-based directory traversal attacks. + +**Session Transcript Parsing (`session_parser.rs`):** + +The `session_parser` module provides parsers to extract token usage and metadata from agent session transcript files. Each agent (Claude, Codex, Gemini) runs as an opaque subprocess and stores session data in different formats: + +- **Codex**: `~/.codex/sessions///
/rollout-T-.jsonl` +- **Gemini**: `~/.gemini/tmp//chats/session-T-.json` +- **Claude**: `~/.claude/projects//.jsonl` + +Key types: +- `TokenUsageReport`: Unified report wrapping `TokenUsage` (from codex-protocol) with agent type, session ID, and transcript path +- `AgentKind`: Enum identifying the agent (Claude, Codex, Gemini) +- `ParseError`: Error cases (EmptyFile, MissingSessionId, TokenOverflow, IoError, JsonError) + +Parser functions: +- `parse_codex_session()`: Parses Codex JSONL with cumulative `token_count` events. Session ID derived from filename since not embedded in content. Extracts `model_context_window` when available. +- `parse_gemini_session()`: Parses Gemini JSON with messages array. Aggregates tokens from each message. Maps `tokens.thoughts` to `reasoning_output_tokens`, `tokens.cached` to `cached_input_tokens`. +- `parse_claude_session()`: Parses Claude JSONL with per-message usage objects (nested in `.message.usage`). Maps `cache_read_input_tokens` to `cached_input_tokens`. No separate reasoning tokens. + +Implementation details: +- **Line-by-line JSONL parsing**: Resilient error handling logs warnings and continues on malformed lines +- **Checked arithmetic**: Uses `.checked_add()` for token aggregation to prevent overflow +- **Agent-specific token mapping**: Each parser maps agent-specific token fields to the unified `TokenUsage` struct +- **Codex as external agent**: Treats Codex sessions as external data (like Gemini/Claude), not relying on internal Codex state + +Session discovery logic (finding files in ~/.codex, ~/.gemini, ~/.claude) is deferred for future TUI integration. + +**Approval Bridging:** + +The ACP module bridges permission requests to Codex's approval UI. Approval requests are handled **immediately** (not deferred) to avoid deadlocks: + +``` +┌─────────────────────────┐ ApprovalRequest ┌─────────────────────────┐ +│ ACP Worker Thread │──────────────────────►│ Main Thread (TUI) │ +│ │ │ │ +│ ClientDelegate │ │ - Display approval UI │ +│ - request_permission()│◄──────────────────────│ - Get user decision │ +│ │ ReviewDecision │ - Send via oneshot │ +└─────────────────────────┘ (via oneshot) └─────────────────────────┘ +``` + +- `ApprovalRequest` bundles the `ApprovalEventType`, original ACP options, and response channel +- `ApprovalEventType` enum selects the appropriate approval UI: + - `Exec(ExecApprovalRequestEvent)` - for shell commands and generic operations + - `Patch(ApplyPatchApprovalRequestEvent)` - for file Edit/Write/Delete with diff rendering +- `AcpConnection::take_approval_receiver()` exposes the receiver for TUI consumption +- Falls back to auto-approve if approval channel is closed (no UI listening) +- Falls back to deny if response channel is dropped (UI didn't respond) +- **Critical timing**: The agent subprocess blocks waiting for approval. Deferring approval display would deadlock (agent waits for approval, but TaskComplete never arrives until agent finishes) + +**Patch Event Translation:** + +For Edit/Write/Delete operations, the ACP backend emits native patch events for better TUI rendering: + +| Operation | Approval Event | Result Event | +|-----------|----------------|--------------| +| Edit (old_string + new_string) | `ApplyPatchApprovalRequest` | `PatchApplyBegin` with `FileChange::Update` | +| Write (content only) | `ApplyPatchApprovalRequest` | `PatchApplyBegin` with `FileChange::Add` | +| Delete | `ApplyPatchApprovalRequest` | `PatchApplyBegin` with `FileChange::Delete` | +| Execute, Read, etc. | `ExecApprovalRequest` | `ExecCommandBegin/End` | + +The patch event flow requires state tracking since ToolCallUpdate may not have the same fields as ToolCall: + +``` +┌───────────────────┐ ┌───────────────────────────────┐ ┌───────────────────┐ +│ ToolCall │ │ RequestPermission │ │ ToolCallUpdate │ +│ (Edit detected) │ │ │ │ (Completed) │ +│ │ │ │ │ │ +│ Store FileChange │─────►│ ApplyPatchApprovalRequest │─────►│ Retrieve stored │ +│ in pending map │ │ (approval overlay shown) │ │ FileChange, emit │ +│ (no event) │ │ │ │ PatchApplyBegin │ +└───────────────────┘ └───────────────────────────────┘ └───────────────────┘ +``` + +Key translator functions: +- `is_patch_operation()` - detects Edit/Write/Delete based on ToolKind or raw_input fields +- `tool_call_to_file_change()` - converts raw_input to `FileChange` using `diffy` for unified diffs +- `permission_request_to_patch_approval_event()` - creates `ApplyPatchApprovalRequestEvent` for patch ops + +**TUI Backend Adapter (`backend.rs`):** + +The `AcpBackend` provides a TUI-compatible interface that wraps `AcpConnection`: + +``` +┌─────────────────────────┐ ┌─────────────────────────┐ +│ TUI Event Loop │ Event channel │ AcpBackend │ +│ │◄─────────────────────│ │ +│ - spawn_acp_agent() │ codex_protocol:: │ - spawn() │ +│ - forwards events │ Event │ - submit(Op) │ +│ │ │ - approval handling │ +│ │ ─────────────────► │ │ +│ │ Op channel │ │ +└─────────────────────────┘ └─────────────────────────┘ +``` + +- `AcpBackendConfig`: Configuration for spawning (model, cwd, approval_policy, sandbox_policy) +- `AcpBackend::spawn()`: Creates AcpConnection, session, and starts approval handler task +- `AcpBackend::submit(Op)`: Translates Codex Ops to ACP actions: + - `Op::UserInput` → ACP `prompt()` + - `Op::Interrupt` → ACP `cancel()` + - `Op::ExecApproval`/`PatchApproval` → Resolves pending approval + - Unsupported ops → Error event sent to TUI +- `AcpBackend::model_state()`: Returns current model state (available models and current selection) +- `AcpBackend::set_model()` [unstable]: Delegates to `AcpConnection::set_model()` for model switching +- `translate_session_update_to_events()`: Converts ACP `SessionUpdate` to `codex_protocol::EventMsg`: + - `AgentMessageChunk` → `AgentMessageDelta` + - `AgentThoughtChunk` → `AgentReasoningDelta` + - `ToolCall` → `ExecCommandBegin` (with filtering, see below) + - `ToolCallUpdate(Completed)` → `ExecCommandEnd` + +### ACP Tool Call Event Filtering + +The ACP protocol emits **multiple ToolCall events** for the same `call_id` as details become available during LLM streaming: + +``` +Event 1 (early): ToolCall { call_id="toolu_123", title="Read File", raw_input={} } +Event 2 (later): ToolCall { call_id="toolu_123", title="Read /home/.../file.rs", raw_input={path: "..."} } +``` + +Without filtering, duplicate events would cause ExecCells to disappear briefly and reappear at the end of agent turns. The fix uses two layers of filtering: + +**Layer 1 - Skip Generic Events (`translate_session_update_to_events`):** +- Skip ToolCall events that lack useful display information +- Check both `raw_input` (for path/command/pattern fields) and title (for embedded paths or commands) +- `title_contains_useful_info()` detects paths in titles (`" /"`, backticks, long non-generic titles) +- `extract_display_args()` extracts display-friendly arguments based on tool type + +**Layer 2 - Dispatch-Loop Deduplication:** +- The update handler tracks `emitted_begin_call_ids: HashSet` +- Skips any `ExecCommandBegin` with a call_id that was already emitted +- Safety net for edge cases that slip through Layer 1 + +### Tool Classification System + +The `classify_tool_to_parsed_command()` function maps ACP `ToolKind` to TUI rendering modes: + +| ACP ToolKind | ParsedCommand | TUI Rendering | +|--------------|---------------|---------------| +| `Read` | `ParsedCommand::Read` | Exploring (compact, grouped) | +| `Search` | `ParsedCommand::Search` | Exploring (compact, grouped) | +| `Other` + title heuristics | `ListFiles`, `Search`, `Read` | Exploring (title-based fallback) | +| `Execute`, `Edit`, `Delete`, `Move`, `Fetch`, `Think` | `ParsedCommand::Unknown` | Command (full display) | + +Title-based fallback uses `classify_tool_by_title()` when `ToolKind::Other` or `None`: +- Titles containing "list", "glob", "ls", "find files" → `ListFiles` +- Titles containing "search", "grep" → `Search` +- Titles containing "read" or exactly "file" → `Read` +- Everything else → `Unknown` (command mode) + +This enables the TUI to group and collapse read-only operations ("Explored 3 files") while showing mutating operations prominently + +**Event Translation (`translator.rs`):** + +Bridges between ACP types and codex-protocol types: + +| Function | Purpose | +|----------|---------| +| `response_items_to_content_blocks()` | Converts codex `ResponseItem` to ACP `ContentBlock` for prompts | +| `text_to_content_block()` | Simple text-to-ContentBlock conversion | +| `translate_session_update()` | Translates ACP `SessionUpdate` to `TranslatedEvent` enum | +| `permission_request_to_approval_event()` | Converts ACP `RequestPermissionRequest` to Codex `ExecApprovalRequestEvent` | +| `review_decision_to_permission_outcome()` | Converts Codex `ReviewDecision` back to ACP `RequestPermissionOutcome` | + +`TranslatedEvent` variants: +- `TextDelta(String)` - Text content from `AgentMessageChunk` or `AgentThoughtChunk` +- `Completed(StopReason)` - Session completion signal + +Non-text content (images, audio, resources) and tool calls are currently dropped with empty vec. + +**Approval Request Formatting (`translator.rs`):** + +When ACP agents request permission for tool operations, the translator converts raw JSON tool call data into human-readable approval requests using git-style formatting: + +| Tool Kind | Format Example | +|-----------|----------------| +| Edit | `Edit main.rs (+6 -5)` | +| Write (new file) | `Write config.toml (23 lines)` | +| Execute | `Execute: cargo build --release` | +| Delete | `Delete temp.txt` | +| Move | `Move old.rs → new.rs` | +| Generic | `ToolName(argument)` | + +The formatting pipeline: +1. `extract_command_from_tool_call()` dispatches to format functions based on `ToolKind` +2. `extract_reason_from_tool_call()` generates the descriptive reason shown in the approval prompt +3. Helper functions extract and format data from the `raw_input` JSON: + - `extract_file_path()` - finds path from `file_path`, `path`, or `file` fields + - `shorten_path()` - extracts just the filename for compact display + - `calculate_diff_stats()` - computes +added/-removed using set difference on line splits + - `truncate_str()` - truncates long strings with "..." + +Write vs Edit detection uses field presence since ACP lacks a distinct Write variant: +- `old_string` field present → Edit operation (string replacement) +- `content` field present → Write operation (new file creation) + +**Approval Translation Details:** + +The approval translation maps between Codex's binary approve/deny model and ACP's option-based model: + +- `Approved`/`ApprovedForSession` → Finds option with `AllowOnce` or `AllowAlways` kind +- `Denied`/`Abort` → Finds option with `RejectOnce` or `RejectAlways` kind +- Falls back to text matching ("allow", "approve", "yes" vs "deny", "reject", "no") if kind-based matching fails +- Last resort: first option for approve, last option for deny + +### Things to Know + +**Event Flow Tracing:** + +The ACP backend provides detailed tracing for debugging tool event flow issues: + +```bash +RUST_LOG=acp_event_flow=debug cargo run +``` + +The `acp_event_flow` target logs: +- Streaming text and reasoning deltas with content previews +- ToolCall events (skipped generic events, emitted events with parsed_cmd info) +- ToolCallUpdate completion events with output extraction +- Dispatch loop event counts and duplicate detection + +This pairs with TUI-side tracing targets (`tui_event_flow`, `cell_flushing`, `pending_exec_cells`) for full event lifecycle debugging. + +**Protocol Version Check:** + +- Minimum supported version is `acp::V1` +- Checked during initialization handshake +- Connection fails if agent reports older version + +**Stderr Handling:** + +- Agent stderr is captured via `spawn_local` task in `spawn_connection_internal()` +- Lines are logged via `tracing::warn!` with "ACP agent stderr:" prefix +- Task runs until EOF or error + +**Session Update Routing:** + +- `ClientDelegate` maintains `HashMap>` +- Updates for unregistered sessions are silently dropped +- Uses `try_send()` (non-blocking) - full/closed channels cause update loss + +**Agent Initialization:** + +Client advertises these capabilities to agents: +- `fs.read_text_file: true` +- `fs.write_text_file: true` +- `terminal: false` + +### Future Work + +The following features are marked with TODO comments in the codebase: + +**Resume/Fork Integration (connection.rs:343-350):** +- Accept optional session_id parameter to resume existing sessions +- Load persisted history from Codex rollout format +- Send history to agent via session initialization + +**Codex-format History Persistence (connection.rs:385-394):** +- Collect all SessionUpdates during prompts +- Convert to Codex ResponseItem format using translator functions +- Write to rollout storage for session resume and history browsing + +**History Export for Handoff (connection.rs:220-234):** +- Export session history in Codex format +- Enable switching from ACP mode to HTTP mode mid-session +- Support replaying history through different backends + +Created and maintained by Nori. ### Core Implementation **Thread-Safe Connection Wrapper (`connection.rs`):** diff --git a/codex-rs/acp/src/backend.rs b/codex-rs/acp/src/backend.rs index 7307f4888..2f6594f1a 100644 --- a/codex-rs/acp/src/backend.rs +++ b/codex-rs/acp/src/backend.rs @@ -31,6 +31,7 @@ use tracing::warn; use crate::connection::AcpConnection; use crate::connection::AcpModelState; +use crate::connection::AcpTraceConfig; use crate::connection::ApprovalEventType; use crate::connection::ApprovalRequest; use crate::registry::get_agent_config; @@ -143,6 +144,8 @@ pub struct AcpBackendConfig { pub approval_policy: AskForApproval, /// Sandbox policy for command execution pub sandbox_policy: SandboxPolicy, + /// Optional trace configuration for ACP traffic debugging + pub trace_config: Option, /// Optional external notifier command for OS-level notifications pub notify: Option>, /// Nori home directory for history storage @@ -199,7 +202,8 @@ impl AcpBackend { debug!("Spawning ACP backend for model: {}", config.model); // Spawn the ACP connection with enhanced error handling - let connection_result = AcpConnection::spawn(&agent_config, &cwd).await; + let connection_result = + AcpConnection::spawn(&agent_config, &cwd, config.trace_config.clone()).await; let mut connection = match connection_result { Ok(conn) => conn, @@ -2266,6 +2270,7 @@ mod tests { cwd: temp_dir.path().to_path_buf(), approval_policy: AskForApproval::Never, sandbox_policy: SandboxPolicy::new_read_only_policy(), + trace_config: None, notify: None, nori_home: temp_dir.path().to_path_buf(), history_persistence: crate::config::HistoryPersistence::SaveAll, diff --git a/codex-rs/acp/src/config/loader.rs b/codex-rs/acp/src/config/loader.rs index 91e6f3fb0..dd10b94ca 100644 --- a/codex-rs/acp/src/config/loader.rs +++ b/codex-rs/acp/src/config/loader.rs @@ -57,6 +57,14 @@ impl NoriConfig { /// Load configuration from a specific path (for testing) pub fn load_from_path(config_path: &PathBuf) -> Result { + Self::load_from_path_with_overrides(config_path, NoriConfigOverrides::default()) + } + + /// Load configuration from a specific path with CLI overrides (for testing) + pub fn load_from_path_with_overrides( + config_path: &PathBuf, + overrides: NoriConfigOverrides, + ) -> Result { let nori_home = config_path .parent() .map(PathBuf::from) @@ -71,7 +79,7 @@ impl NoriConfig { NoriConfigToml::default() }; - Self::from_toml(toml_config, nori_home, NoriConfigOverrides::default()) + Self::from_toml(toml_config, nori_home, overrides) } /// Build resolved config from TOML + overrides @@ -109,6 +117,10 @@ impl NoriConfig { .approval_policy .or(toml.approval_policy) .unwrap_or(ApprovalPolicy::OnRequest), + acp_trace_enabled: overrides + .acp_trace_enabled + .or(toml.acp_trace_enabled) + .unwrap_or(false), history_persistence: toml .history_persistence .unwrap_or(super::types::HistoryPersistence::SaveAll), diff --git a/codex-rs/acp/src/config/types.rs b/codex-rs/acp/src/config/types.rs index 3e2000ac0..ba187ea6f 100644 --- a/codex-rs/acp/src/config/types.rs +++ b/codex-rs/acp/src/config/types.rs @@ -38,6 +38,9 @@ pub struct NoriConfigToml { /// Approval policy for commands pub approval_policy: Option, + /// Enable ACP traffic tracing via sacp-tee proxy + pub acp_trace_enabled: Option, + /// History persistence policy pub history_persistence: Option, @@ -112,6 +115,9 @@ pub struct NoriConfigOverrides { /// Override current working directory pub cwd: Option, + + /// Override ACP trace enabled setting + pub acp_trace_enabled: Option, } /// Resolved configuration with defaults applied @@ -130,6 +136,9 @@ pub struct NoriConfig { /// Approval policy for commands pub approval_policy: ApprovalPolicy, + /// Enable ACP traffic tracing via sacp-tee proxy + pub acp_trace_enabled: bool, + /// History persistence policy pub history_persistence: HistoryPersistence, @@ -149,6 +158,17 @@ pub struct NoriConfig { pub mcp_servers: HashMap, } +impl NoriConfig { + /// Get the path for the ACP trace log file for a given session. + /// + /// Returns a path like `~/.nori/cli/log/acp-trace-.log` + pub fn acp_trace_log_path(&self, session_id: &str) -> PathBuf { + self.nori_home + .join("log") + .join(format!("acp-trace-{session_id}.log")) + } +} + impl Default for NoriConfig { fn default() -> Self { Self { @@ -156,6 +176,7 @@ impl Default for NoriConfig { model: DEFAULT_MODEL.to_string(), sandbox_mode: SandboxMode::WorkspaceWrite, approval_policy: ApprovalPolicy::OnRequest, + acp_trace_enabled: false, history_persistence: HistoryPersistence::default(), animations: true, notifications: true, diff --git a/codex-rs/acp/src/connection.rs b/codex-rs/acp/src/connection.rs index b3e7bc897..0ba832151 100644 --- a/codex-rs/acp/src/connection.rs +++ b/codex-rs/acp/src/connection.rs @@ -38,6 +38,61 @@ use tracing::warn; use crate::registry::AcpAgentConfig; use crate::translator; +/// Configuration for ACP traffic tracing via sacp-tee proxy. +/// +/// When enabled, the agent subprocess command is wrapped with `sacp-tee` +/// to capture all JSON-RPC 2.0 traffic for debugging purposes. +#[derive(Debug, Clone)] +pub struct AcpTraceConfig { + /// Path to the log file where traffic will be recorded + pub log_file_path: PathBuf, +} + +/// Build the command and arguments for spawning an ACP agent. +/// +/// When `trace_config` is `Some` and running in debug builds, the command is wrapped +/// with `sacp-tee` to enable traffic logging. In release builds, tracing is disabled +/// and the original command is always returned unchanged. +/// +/// # Arguments +/// * `config` - The ACP agent configuration +/// * `trace_config` - Optional tracing configuration (ignored in release builds) +/// +/// # Returns +/// A tuple of (command, args) to use when spawning the process +pub fn build_agent_command( + config: &AcpAgentConfig, + trace_config: Option, +) -> (String, Vec) { + #[cfg(debug_assertions)] + { + match trace_config { + Some(trace) => { + // Wrap with sacp-tee: sacp-tee --log-file -- + let mut args = vec![ + "--log-file".to_string(), + trace.log_file_path.to_string_lossy().to_string(), + "--".to_string(), + config.command.clone(), + ]; + args.extend(config.args.clone()); + ("sacp-tee".to_string(), args) + } + None => { + // Pass through unchanged + (config.command.clone(), config.args.clone()) + } + } + } + + #[cfg(not(debug_assertions))] + { + // In release builds, always pass through unchanged (ignore trace_config) + let _ = trace_config; // Suppress unused variable warning + (config.command.clone(), config.args.clone()) + } +} + /// The type of approval event to send to the UI. /// /// This enum allows us to use the more appropriate approval UI for different @@ -171,10 +226,15 @@ impl AcpConnection { /// # Arguments /// * `config` - Agent configuration (command, args, provider info) /// * `cwd` - Working directory for the agent subprocess + /// * `trace_config` - Optional tracing configuration for sacp-tee proxy /// /// # Returns /// A connected `AcpConnection` ready for creating sessions. - pub async fn spawn(config: &AcpAgentConfig, cwd: &Path) -> Result { + pub async fn spawn( + config: &AcpAgentConfig, + cwd: &Path, + trace_config: Option, + ) -> Result { let config = config.clone(); let cwd = cwd.to_path_buf(); @@ -208,7 +268,9 @@ impl AcpConnection { let local = tokio::task::LocalSet::new(); local .run_until(async move { - match spawn_connection_internal(&config, &cwd, approval_tx).await { + match spawn_connection_internal(&config, &cwd, approval_tx, trace_config) + .await + { Ok((inner, capabilities)) => { let _ = init_tx.send(Ok(capabilities)); run_command_loop( @@ -460,16 +522,20 @@ async fn spawn_connection_internal( config: &AcpAgentConfig, cwd: &Path, approval_tx: mpsc::Sender, + trace_config: Option, ) -> Result<(AcpConnectionInner, acp::AgentCapabilities)> { + // Build the command, potentially wrapping with sacp-tee for tracing + let (command, args) = build_agent_command(config, trace_config); + debug!( "Spawning ACP agent: {} {:?} in {}", - config.command, - config.args, + command, + args, cwd.display() ); - let mut cmd = Command::new(&config.command); - cmd.args(&config.args) + let mut cmd = Command::new(&command); + cmd.args(&args) .envs(&config.env) .current_dir(cwd) .stdin(Stdio::piped()) @@ -515,7 +581,7 @@ async fn spawn_connection_internal( let mut child = cmd .spawn() - .with_context(|| format!("Failed to spawn ACP agent: {}", config.command))?; + .with_context(|| format!("Failed to spawn ACP agent: {command}"))?; let stdout = child.stdout.take().context("Failed to take stdout")?; let stdin = child.stdin.take().context("Failed to take stdin")?; @@ -1148,8 +1214,8 @@ mod tests { let temp_dir = tempdir().expect("Failed to create temp dir"); - // Spawn connection - let conn = AcpConnection::spawn(&config, temp_dir.path()) + // Spawn connection (without tracing) + let conn = AcpConnection::spawn(&config, temp_dir.path(), None) .await .expect("Failed to spawn ACP connection"); diff --git a/codex-rs/acp/src/lib.rs b/codex-rs/acp/src/lib.rs index 8eca948eb..52b10cf1d 100644 --- a/codex-rs/acp/src/lib.rs +++ b/codex-rs/acp/src/lib.rs @@ -34,6 +34,7 @@ pub use backend::AcpBackend; pub use backend::AcpBackendConfig; pub use connection::AcpConnection; pub use connection::AcpModelState; +pub use connection::AcpTraceConfig; pub use connection::ApprovalRequest; pub use registry::AcpAgentConfig; pub use registry::AcpAgentInfo; diff --git a/codex-rs/acp/tests/acp_trace_test.rs b/codex-rs/acp/tests/acp_trace_test.rs new file mode 100644 index 000000000..ba374f41c --- /dev/null +++ b/codex-rs/acp/tests/acp_trace_test.rs @@ -0,0 +1,194 @@ +//! Tests for ACP trace configuration and command wrapping +//! +//! These tests verify the behavior of: +//! - `acp_trace_enabled` config field +//! - `acp_trace_log_path()` helper with session ID +//! - Command wrapping with sacp-tee proxy + +use codex_acp::NoriConfig; +use std::path::PathBuf; +use tempfile::TempDir; + +// ============================================================================= +// Config Field Tests +// ============================================================================= + +#[test] +fn test_acp_trace_enabled_defaults_to_false() { + let temp_dir = TempDir::new().unwrap(); + let config_path = temp_dir.path().join("config.toml"); + + // Write an empty config file + std::fs::write(&config_path, "").unwrap(); + + let config = NoriConfig::load_from_path(&config_path).unwrap(); + + assert!( + !config.acp_trace_enabled, + "acp_trace_enabled should default to false" + ); +} + +#[test] +fn test_acp_trace_enabled_can_be_loaded_from_toml() { + let temp_dir = TempDir::new().unwrap(); + let config_path = temp_dir.path().join("config.toml"); + + // Write a config file with acp_trace_enabled = true + std::fs::write(&config_path, "acp_trace_enabled = true").unwrap(); + + let config = NoriConfig::load_from_path(&config_path).unwrap(); + + assert!( + config.acp_trace_enabled, + "acp_trace_enabled should be loaded as true from config" + ); +} + +#[test] +fn test_acp_trace_enabled_cli_override() { + use codex_acp::NoriConfigOverrides; + + let temp_dir = TempDir::new().unwrap(); + let config_path = temp_dir.path().join("config.toml"); + + // Write a config file with acp_trace_enabled = false + std::fs::write(&config_path, "acp_trace_enabled = false").unwrap(); + + // Create overrides with acp_trace_enabled = true + let overrides = NoriConfigOverrides { + acp_trace_enabled: Some(true), + ..Default::default() + }; + + let config = NoriConfig::load_from_path_with_overrides(&config_path, overrides).unwrap(); + + assert!( + config.acp_trace_enabled, + "CLI override should take precedence over config file" + ); +} + +// ============================================================================= +// Trace Log Path Tests +// ============================================================================= + +#[test] +fn test_acp_trace_log_path_uses_nori_home() { + let temp_dir = TempDir::new().unwrap(); + let config_path = temp_dir.path().join("config.toml"); + + std::fs::write(&config_path, "").unwrap(); + + let config = NoriConfig::load_from_path(&config_path).unwrap(); + let session_id = "session-456"; + let log_path = config.acp_trace_log_path(session_id); + + // The log path should be under nori_home + let nori_home_str = config.nori_home.to_string_lossy(); + let log_path_str = log_path.to_string_lossy(); + + assert!( + log_path_str.starts_with(nori_home_str.as_ref()), + "Log path {:?} should be under nori_home {:?}", + log_path, + config.nori_home + ); +} + +// ============================================================================= +// Command Wrapping Tests +// ============================================================================= + +#[test] +fn test_build_wrapped_command_without_trace() { + use codex_acp::connection::build_agent_command; + use codex_acp::get_agent_config; + + let config = get_agent_config("mock-model").unwrap(); + let trace_config = None; + + let (command, args) = build_agent_command(&config, trace_config); + + // Without tracing, command should be unchanged + assert!( + command.contains("mock_acp_agent"), + "Command should be the original agent command: {command}" + ); + assert!( + args.is_empty(), + "Args should be unchanged (empty for mock): {args:?}" + ); +} + +#[test] +fn test_build_wrapped_command_with_trace() { + use codex_acp::connection::AcpTraceConfig; + use codex_acp::connection::build_agent_command; + use codex_acp::get_agent_config; + + let config = get_agent_config("mock-model").unwrap(); + let log_path = PathBuf::from("/tmp/test-trace.log"); + let trace_config = Some(AcpTraceConfig { + log_file_path: log_path.clone(), + }); + + let (command, args) = build_agent_command(&config, trace_config); + + // With tracing, command should be sacp-tee + assert_eq!(command, "sacp-tee", "Command should be sacp-tee"); + + // Args should include --log-file, the path, --, and the original command + assert!( + args.contains(&"--log-file".to_string()), + "Args should contain --log-file: {args:?}" + ); + assert!( + args.contains(&log_path.to_string_lossy().to_string()), + "Args should contain log path: {args:?}" + ); + assert!( + args.contains(&"--".to_string()), + "Args should contain -- separator: {args:?}" + ); + // The original command should be after -- + let separator_pos = args.iter().position(|a| a == "--").unwrap(); + assert!( + args[separator_pos + 1..] + .iter() + .any(|a| a.contains("mock_acp_agent")), + "Original command should be after --: {args:?}" + ); +} + +#[test] +fn test_build_wrapped_command_preserves_original_args() { + use codex_acp::connection::AcpTraceConfig; + use codex_acp::connection::build_agent_command; + use codex_acp::get_agent_config; + + // Use gemini which has args (--experimental-acp) + let config = get_agent_config("gemini").unwrap(); + let log_path = PathBuf::from("/tmp/test-trace.log"); + let trace_config = Some(AcpTraceConfig { + log_file_path: log_path, + }); + + let (command, args) = build_agent_command(&config, trace_config); + + assert_eq!(command, "sacp-tee"); + + // Original args should be preserved after -- + let separator_pos = args.iter().position(|a| a == "--").unwrap(); + let downstream_args = &args[separator_pos + 1..]; + + // Should have the original command and its args + assert!( + downstream_args.iter().any(|a| a.contains("gemini-cli")), + "Should contain original gemini-cli: {downstream_args:?}" + ); + assert!( + downstream_args.iter().any(|a| a == "--experimental-acp"), + "Should preserve original --experimental-acp arg: {downstream_args:?}" + ); +} diff --git a/codex-rs/core/docs.md b/codex-rs/core/docs.md index 3dccd52ab..fc78bc2a8 100644 --- a/codex-rs/core/docs.md +++ b/codex-rs/core/docs.md @@ -70,6 +70,17 @@ All config mutations should go through `ConfigEditsBuilder` which provides atomi This pattern ensures config changes merge with existing content rather than overwriting the entire file. +**ACP Configuration:** + +The `Config` struct includes fields for ACP (Agent Context Protocol) integration, which are passed through to `@/codex-rs/acp` when spawning ACP agents: + +| Field | TOML Path | Description | +|-------|-----------|-------------| +| `acp_allow_http_fallback` | `acp.allow_http_fallback` | Allow fallback to HTTP providers for unregistered models | +| `acp_trace_enabled` | `acp.trace_enabled` | Enable traffic tracing via `sacp-tee` proxy | + +These fields follow the standard override priority: CLI flag > TOML config > default (false). The `ConfigOverrides` struct includes `acp_trace_enabled` for CLI override support. + ### Things to Know **Test Suite Configuration:** diff --git a/codex-rs/core/src/config/mod.rs b/codex-rs/core/src/config/mod.rs index 81b6ab7dd..508fbef27 100644 --- a/codex-rs/core/src/config/mod.rs +++ b/codex-rs/core/src/config/mod.rs @@ -283,6 +283,10 @@ pub struct Config { /// registered in the ACP registry. When `false` (the default), Codex runs /// in ACP-only mode and produces an error for unregistered models. pub acp_allow_http_fallback: bool, + + /// When `true`, enables ACP traffic tracing using `sacp-tee` proxy. + /// Traffic is logged to `codex_home/log/acp-trace-{session_id}.log`. + pub acp_trace_enabled: bool, } impl Config { @@ -737,6 +741,11 @@ pub struct AcpConfigToml { /// in ACP-only mode and produces an error for unregistered models. #[serde(default)] pub allow_http_fallback: bool, + + /// When `true`, enables ACP traffic tracing using `sacp-tee` proxy. + /// Traffic is logged to `codex_home/log/acp-trace-{session_id}.log`. + #[serde(default)] + pub trace_enabled: bool, } impl From for UserSavedConfig { @@ -923,6 +932,8 @@ pub struct ConfigOverrides { pub experimental_sandbox_command_assessment: Option, /// Additional directories that should be treated as writable roots for this session. pub additional_writable_roots: Vec, + /// Enable ACP traffic tracing using `sacp-tee` proxy. + pub acp_trace_enabled: Option, } /// Resolves the OSS provider from CLI override, profile config, or global config. @@ -981,6 +992,7 @@ impl Config { tools_web_search_request: override_tools_web_search_request, experimental_sandbox_command_assessment: sandbox_command_assessment_override, additional_writable_roots, + acp_trace_enabled: acp_trace_enabled_override, } = overrides; let active_profile_name = config_profile_key @@ -1289,7 +1301,14 @@ impl Config { exporter, } }, - acp_allow_http_fallback: cfg.acp.map(|a| a.allow_http_fallback).unwrap_or(false), + acp_allow_http_fallback: cfg + .acp + .as_ref() + .map(|a| a.allow_http_fallback) + .unwrap_or(false), + acp_trace_enabled: acp_trace_enabled_override + .or(cfg.acp.as_ref().map(|a| a.trace_enabled)) + .unwrap_or(false), }; Ok(config) } @@ -3030,6 +3049,7 @@ model_verbosity = "high" animations: true, otel: OtelConfig::default(), acp_allow_http_fallback: false, + acp_trace_enabled: false, }, o3_profile_config ); @@ -3104,6 +3124,7 @@ model_verbosity = "high" animations: true, otel: OtelConfig::default(), acp_allow_http_fallback: false, + acp_trace_enabled: false, }; assert_eq!(expected_gpt3_profile_config, gpt3_profile_config); @@ -3193,6 +3214,7 @@ model_verbosity = "high" animations: true, otel: OtelConfig::default(), acp_allow_http_fallback: false, + acp_trace_enabled: false, }; assert_eq!(expected_zdr_profile_config, zdr_profile_config); @@ -3268,6 +3290,7 @@ model_verbosity = "high" animations: true, otel: OtelConfig::default(), acp_allow_http_fallback: false, + acp_trace_enabled: false, }; assert_eq!(expected_gpt5_profile_config, gpt5_profile_config); diff --git a/codex-rs/exec/src/lib.rs b/codex-rs/exec/src/lib.rs index 3fdb8b09a..27a643951 100644 --- a/codex-rs/exec/src/lib.rs +++ b/codex-rs/exec/src/lib.rs @@ -247,6 +247,7 @@ pub async fn run_main(cli: Cli, codex_linux_sandbox_exe: Option) -> any tools_web_search_request: None, experimental_sandbox_command_assessment: None, additional_writable_roots: add_dir, + acp_trace_enabled: None, }; let config = Config::load_with_cli_overrides(cli_kv_overrides, overrides).await?; diff --git a/codex-rs/mcp-server/src/codex_tool_config.rs b/codex-rs/mcp-server/src/codex_tool_config.rs index 4e61bde02..6f598a710 100644 --- a/codex-rs/mcp-server/src/codex_tool_config.rs +++ b/codex-rs/mcp-server/src/codex_tool_config.rs @@ -171,6 +171,7 @@ impl CodexToolCallParam { tools_web_search_request: None, experimental_sandbox_command_assessment: None, additional_writable_roots: Vec::new(), + acp_trace_enabled: None, }; let cli_overrides = cli_overrides diff --git a/codex-rs/tui-pty-e2e/Cargo.toml b/codex-rs/tui-pty-e2e/Cargo.toml index 8fd060c6a..a18fd158c 100644 --- a/codex-rs/tui-pty-e2e/Cargo.toml +++ b/codex-rs/tui-pty-e2e/Cargo.toml @@ -18,3 +18,4 @@ libc = "0.2" [dev-dependencies] tempfile = "3" regex = "1" +sacp-tee = "0.1" diff --git a/codex-rs/tui-pty-e2e/src/lib.rs b/codex-rs/tui-pty-e2e/src/lib.rs index 8e7f93ba6..6d620a4a6 100644 --- a/codex-rs/tui-pty-e2e/src/lib.rs +++ b/codex-rs/tui-pty-e2e/src/lib.rs @@ -270,11 +270,21 @@ impl TuiSession { .as_ref() .map(|p| p.to_string_lossy().into_owned()) .unwrap_or_else(|| codex_home.to_string_lossy().into_owned()); - let acp_section = if config.allow_http_fallback { - "\n[acp]\nallow_http_fallback = true\n" + + // Build ACP section with both allow_http_fallback and trace_enabled + let mut acp_settings = Vec::new(); + if config.allow_http_fallback { + acp_settings.push("allow_http_fallback = true"); + } + if config.acp_trace_enabled { + acp_settings.push("trace_enabled = true"); + } + let acp_section = if !acp_settings.is_empty() { + format!("\n[acp]\n{}\n", acp_settings.join("\n")) } else { - "" + String::new() }; + format!( r#"model = "{model}" model_provider = "mock_provider" @@ -622,6 +632,16 @@ name = "Mock ACP provider for tests" pub fn nori_home_path(&self) -> Option { self._temp_dir.as_ref().map(|d| d.path().to_path_buf()) } + + /// Get the path to the ACP trace log file (if temp directory exists and tracing is enabled) + /// + /// This is useful for E2E tests that need to verify sacp-tee wire protocol logging. + /// Logs are stored in `$NORI_HOME/log/` with the pattern `acp-trace-*.log`. + pub fn acp_trace_log_path(&self) -> Option { + self._temp_dir + .as_ref() + .and_then(|d| find_acp_trace_log_file(d.path())) + } } /// Find the ACP log file in the given NORI_HOME directory. @@ -647,6 +667,29 @@ fn find_acp_log_file(nori_home: &std::path::Path) -> Option .map(|entry| entry.path()) } +/// Find the ACP trace log file in the given NORI_HOME directory. +/// +/// Searches for files matching `acp-trace-*` in the `log/` subdirectory, +/// returning the most recently modified one. +fn find_acp_trace_log_file(nori_home: &std::path::Path) -> Option { + let log_dir = nori_home.join("log"); + if !log_dir.exists() { + return None; + } + + std::fs::read_dir(&log_dir) + .ok()? + .filter_map(|entry| entry.ok()) + .filter(|entry| { + entry + .file_name() + .to_str() + .is_some_and(|name| name.starts_with("acp-trace-") && name.ends_with(".log")) + }) + .max_by_key(|entry| entry.metadata().ok().and_then(|m| m.modified().ok())) + .map(|entry| entry.path()) +} + /// Configuration for spawning a test session pub struct SessionConfig { pub model: String, @@ -671,6 +714,9 @@ pub struct SessionConfig { /// Binary names to exclude from PATH (filters out directories containing these binaries). /// Useful for testing behavior when certain commands are "not installed". pub exclude_binaries: Vec, + /// Enable ACP wire protocol tracing via sacp-tee proxy. + /// When true, writes `acp_trace_enabled = true` to config.toml. + pub acp_trace_enabled: bool, } impl Default for SessionConfig { @@ -694,6 +740,7 @@ impl SessionConfig { // Exclude nori-ai by default since it won't be in PATH on CI runners. // Tests that need nori-ai should explicitly add it via with_extra_path(). exclude_binaries: vec!["nori-ai".to_string()], + acp_trace_enabled: false, // Tracing disabled by default } } @@ -765,6 +812,13 @@ impl SessionConfig { self.exclude_binaries.push(binary_name.into()); self } + + /// Enable ACP wire protocol tracing via sacp-tee proxy. + /// When enabled, sets `acp_trace_enabled = true` in config.toml. + pub fn with_acp_trace_enabled(mut self, enabled: bool) -> Self { + self.acp_trace_enabled = enabled; + self + } } /// Get path to codex binary diff --git a/codex-rs/tui-pty-e2e/tests/acp_tracing.rs b/codex-rs/tui-pty-e2e/tests/acp_tracing.rs new file mode 100644 index 000000000..cd182b711 --- /dev/null +++ b/codex-rs/tui-pty-e2e/tests/acp_tracing.rs @@ -0,0 +1,131 @@ +//! E2E tests for ACP wire protocol tracing via sacp-tee +//! +//! These tests verify that: +//! 1. ACP tracing can be enabled via SessionConfig +//! 2. sacp-tee logs all JSON-RPC messages between client and agent +//! 3. The wire protocol log structure is consistent and correct + +use std::time::Duration; +use tui_pty_e2e::SessionConfig; +use tui_pty_e2e::TIMEOUT; +use tui_pty_e2e::TIMEOUT_INPUT; +use tui_pty_e2e::TuiSession; + +#[test] +fn test_acp_tracing_enabled_creates_log_file() { + let config = SessionConfig::new() + .with_model("mock-model".to_owned()) + .with_acp_trace_enabled(true); + + let mut session = TuiSession::spawn_with_config(24, 80, config) + .expect("Failed to spawn session with ACP tracing"); + + // Wait for TUI to start + session + .wait_for_text("›", TIMEOUT) + .expect("TUI should start"); + + std::thread::sleep(TIMEOUT_INPUT); + + // Verify log file was created + let log_path = session + .acp_trace_log_path() + .expect("ACP trace log path should be available"); + + assert!( + log_path.exists(), + "ACP trace log file should exist at {}", + log_path.display() + ); +} + +#[test] +fn test_acp_tracing_logs_wire_protocol_messages() { + let config = SessionConfig::new() + .with_model("mock-model".to_owned()) + .with_acp_trace_enabled(true) + .with_mock_response("Test response from mock agent"); + + let mut session = TuiSession::spawn_with_config(24, 80, config) + .expect("Failed to spawn session with ACP tracing"); + + // Wait for startup + session + .wait_for_text("›", TIMEOUT) + .expect("TUI should start"); + + std::thread::sleep(TIMEOUT_INPUT); + + // Send a prompt to trigger wire protocol communication + session.send_str("Test prompt").unwrap(); + std::thread::sleep(TIMEOUT_INPUT); + session.send_key(tui_pty_e2e::Key::Enter).unwrap(); + + // Wait for response from mock agent + session + .wait_for_text("Test response from mock agent", TIMEOUT) + .expect("Should receive response from mock agent"); + + // Read the log file + let log_path = session + .acp_trace_log_path() + .expect("ACP trace log path should be available"); + + let log_content = std::fs::read_to_string(&log_path).expect("Should be able to read log file"); + + // Verify the log contains expected JSON-RPC messages with direction markers + assert!( + log_content.contains(r#"→ {"jsonrpc":"2.0","id":0,"method":"initialize""#), + "Log should contain initialize request, got:\n{}", + log_content + ); + + assert!( + log_content.contains(r#"← {"jsonrpc":"2.0","id":0,"result":"#), + "Log should contain initialize response, got:\n{}", + log_content + ); + + assert!( + log_content.contains(r#"→ {"jsonrpc":"2.0","id":1,"method":"session/new""#), + "Log should contain session/new request, got:\n{}", + log_content + ); + + assert!( + log_content.contains(r#"→ {"jsonrpc":"2.0","id":2,"method":"session/prompt""#) + || log_content.contains(r#"→ {"jsonrpc":"2.0","method":"session/prompt""#), + "Log should contain session/prompt request, got:\n{}", + log_content + ); +} + +#[test] +fn test_acp_tracing_disabled_by_default() { + let config = SessionConfig::new().with_model("mock-model".to_owned()); + // Note: NOT calling .with_acp_trace_enabled(true) + + let mut session = + TuiSession::spawn_with_config(24, 80, config).expect("Failed to spawn session"); + + // Wait for startup + session + .wait_for_text("›", TIMEOUT) + .expect("TUI should start"); + + std::thread::sleep(TIMEOUT_INPUT); + + // Send a prompt + session.send_str("Test prompt").unwrap(); + std::thread::sleep(TIMEOUT_INPUT); + session.send_key(tui_pty_e2e::Key::Enter).unwrap(); + + // Wait a bit for any potential log file creation + std::thread::sleep(Duration::from_millis(500)); + + // Verify NO log file was created + assert!( + session.acp_trace_log_path().is_none(), + "ACP trace log should not exist when tracing is disabled" + ); +} diff --git a/codex-rs/tui-pty-e2e/tests/snapshots/acp_tracing__acp_tracing_wire_protocol.snap b/codex-rs/tui-pty-e2e/tests/snapshots/acp_tracing__acp_tracing_wire_protocol.snap new file mode 100644 index 000000000..23abca54d --- /dev/null +++ b/codex-rs/tui-pty-e2e/tests/snapshots/acp_tracing__acp_tracing_wire_protocol.snap @@ -0,0 +1,11 @@ +--- +source: tui-pty-e2e/tests/acp_tracing.rs +expression: normalized +--- +→ {"jsonrpc":"2.0","id":0,"method":"initialize","params":{"protocolVersion":1,"clientCapabilities":{"fs":{"readTextFile":true,"writeTextFile":true},"terminal":false},"clientInfo":{"name":"codex","title":"Codex CLI","version":"0.0.0"}}} +← {"jsonrpc":"2.0","id":0,"result":{"protocolVersion":1,"agentCapabilities":{"loadSession":false,"promptCapabilities":{"image":false,"audio":false,"embeddedContext":false},"mcpCapabilities":{"http":false,"sse":false},"sessionCapabilities":{}},"authMethods":[],"agentInfo":{"name":"mock-agent","title":"Mock Agent","version":"0.1.0"}}} +→ {"jsonrpc":"2.0","id":1,"method":"session/new","params":{"cwd":"[TMP_DIR] +← {"jsonrpc":"2.0","id":1,"result":{"sessionId":"0","models":{"currentModelId":"mock-model-default","availableModels":[{"modelId":"mock-model-default","name":"Mock Default Model","description":"The default mock model"},{"modelId":"mock-model-fast","name":"Mock Fast Model","description":"A faster mock model variant"},{"modelId":"mock-model-powerful","name":"Mock Powerful Model","description":"A more powerful mock model variant"}]}}} +→ {"jsonrpc":"2.0","id":2,"method":"session/prompt","params":{"sessionId":"0","prompt":[{"type":"text","text":"Snapshot test prompt"}]}} +← {"jsonrpc":"2.0","method":"session/update","params":{"sessionId":"0","update":{"sessionUpdate":"agent_message_chunk","content":{"type":"text","text":"Snapshot test response"}}}} +← {"jsonrpc":"2.0","id":2,"result":{"stopReason":"end_turn"}} diff --git a/codex-rs/tui/docs.md b/codex-rs/tui/docs.md index b539cfc64..d039f47ab 100644 --- a/codex-rs/tui/docs.md +++ b/codex-rs/tui/docs.md @@ -46,6 +46,10 @@ The TUI supports two backend modes, selected automatically at startup based on m Both backends produce `codex_protocol::Event` for the TUI event loop, enabling unified event handling. +**ACP Traffic Tracing:** + +When `config.acp_trace_enabled` is true (via `--acp-trace` CLI flag in debug builds or `acp_trace_enabled` in config.toml), `spawn_acp_agent()` constructs an `AcpTraceConfig` with a session-specific log path (`{codex_home}/log/acp-trace-{timestamp}-{pid}.log`) and passes it to `AcpBackendConfig`. **Note: The `--acp-trace` flag is only available in debug builds; in release builds, trace config is silently ignored.** See `@/codex-rs/acp/docs.md` for details on traffic tracing. + **Agent Connecting Status:** When an ACP agent subprocess is being spawned, the TUI shows a "Connecting to [Agent]" status indicator with shimmer animation: @@ -586,7 +590,7 @@ For E2E testing, `NORI_SYNC_SYSTEM_INFO=1` env var enables synchronous collectio **Configuration Flow:** TUI respects config overrides from: -1. CLI flags (`--model` always available; `--sandbox`, `--oss`, `-a`, `--full-auto`, etc. require `codex-features`) +1. CLI flags (`--model` always available; `--acp-trace` in debug builds only; `--sandbox`, `--oss`, `-a`, `--full-auto`, etc. require `codex-features`) 2. `-c key=value` overrides 3. Config profiles (`-p profile-name`) 4. `~/.nori/cli/config.toml` diff --git a/codex-rs/tui/src/chatwidget/agent.rs b/codex-rs/tui/src/chatwidget/agent.rs index f34ea2761..02b175ee6 100644 --- a/codex-rs/tui/src/chatwidget/agent.rs +++ b/codex-rs/tui/src/chatwidget/agent.rs @@ -4,6 +4,7 @@ use codex_acp::AcpBackend; use codex_acp::AcpBackendConfig; #[cfg(feature = "unstable")] use codex_acp::AcpModelState; +use codex_acp::AcpTraceConfig; use codex_acp::HistoryPersistence; use codex_acp::find_nori_home; use codex_acp::get_agent_config; @@ -174,6 +175,30 @@ fn spawn_acp_agent(config: Config, app_event_tx: AppEventSender) -> SpawnAgentRe // Create event channel for backend → TUI let (event_tx, mut event_rx) = mpsc::channel(32); + // Build trace config if tracing is enabled + let trace_config = if config.acp_trace_enabled { + // Generate a session ID for the trace file using timestamp and process ID + let timestamp = std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .map(|d| d.as_millis()) + .unwrap_or(0); + let session_id = format!("{}-{}", timestamp, std::process::id()); + let log_path = config + .codex_home + .join("log") + .join(format!("acp-trace-{session_id}.log")); + // Ensure the log directory exists + if let Err(e) = std::fs::create_dir_all(config.codex_home.join("log")) { + tracing::warn!("Failed to create log directory for ACP trace: {e}"); + } + tracing::info!("ACP tracing enabled, logging to: {}", log_path.display()); + Some(AcpTraceConfig { + log_file_path: log_path, + }) + } else { + None + }; + // Create ACP backend config from codex config let nori_home = find_nori_home().unwrap_or_else(|_| config.cwd.clone()); let acp_config = AcpBackendConfig { @@ -181,6 +206,7 @@ fn spawn_acp_agent(config: Config, app_event_tx: AppEventSender) -> SpawnAgentRe cwd: config.cwd.clone(), approval_policy: config.approval_policy, sandbox_policy: config.sandbox_policy.clone(), + trace_config, notify: config.notify.clone(), nori_home, history_persistence: HistoryPersistence::SaveAll, diff --git a/codex-rs/tui/src/cli.rs b/codex-rs/tui/src/cli.rs index ff880a14c..9481a6bc7 100644 --- a/codex-rs/tui/src/cli.rs +++ b/codex-rs/tui/src/cli.rs @@ -106,4 +106,13 @@ pub struct Cli { /// Intended for testing and automation scenarios. #[arg(long = "skip-trust-directory", default_value_t = false)] pub skip_trust_directory: bool, + + /// Enable ACP traffic tracing using sacp-tee proxy (debug builds only). + /// When enabled, all JSON-RPC 2.0 traffic between the client and agent + /// is logged to a session-specific file in the log directory. + /// Requires sacp-tee to be installed and available in PATH. + /// This feature is only available in debug builds. + #[cfg(debug_assertions)] + #[arg(long = "acp-trace")] + pub acp_trace: bool, } diff --git a/codex-rs/tui/src/lib.rs b/codex-rs/tui/src/lib.rs index f3c02ce3f..e61b6c5b9 100644 --- a/codex-rs/tui/src/lib.rs +++ b/codex-rs/tui/src/lib.rs @@ -334,6 +334,10 @@ pub async fn run_main( tools_web_search_request: None, experimental_sandbox_command_assessment: None, additional_writable_roots: additional_dirs, + #[cfg(debug_assertions)] + acp_trace_enabled: if cli.acp_trace { Some(true) } else { None }, + #[cfg(not(debug_assertions))] + acp_trace_enabled: None, }; let config = load_config_or_exit(cli_kv_overrides.clone(), overrides.clone()).await;