From de991d96a659c8eb61f180146ed3d5106e943865 Mon Sep 17 00:00:00 2001 From: Ahmed Ibrahim Date: Tue, 10 Feb 2026 09:26:40 -0800 Subject: [PATCH 1/7] Always expose view_image and return unsupported-model error --- .../core/src/tools/handlers/view_image.rs | 44 ++++++++++++++++++- codex-rs/core/src/tools/spec.rs | 14 +++--- 2 files changed, 49 insertions(+), 9 deletions(-) diff --git a/codex-rs/core/src/tools/handlers/view_image.rs b/codex-rs/core/src/tools/handlers/view_image.rs index fe62522180c..dc278fabe1e 100644 --- a/codex-rs/core/src/tools/handlers/view_image.rs +++ b/codex-rs/core/src/tools/handlers/view_image.rs @@ -16,7 +16,30 @@ use codex_protocol::models::ContentItem; use codex_protocol::models::ResponseInputItem; use codex_protocol::models::local_image_content_items_with_label_number; -pub struct ViewImageHandler; +pub struct ViewImageHandler { + supports_image_input: bool, +} + +const VIEW_IMAGE_UNSUPPORTED_MESSAGE: &str = + "view_image is not allowed because the current model does not support image inputs"; + +impl ViewImageHandler { + pub fn new(supports_image_input: bool) -> Self { + Self { + supports_image_input, + } + } + + fn ensure_supported(&self) -> Result<(), FunctionCallError> { + if self.supports_image_input { + Ok(()) + } else { + Err(FunctionCallError::RespondToModel( + VIEW_IMAGE_UNSUPPORTED_MESSAGE.to_string(), + )) + } + } +} #[derive(Deserialize)] struct ViewImageArgs { @@ -30,6 +53,8 @@ impl ToolHandler for ViewImageHandler { } async fn handle(&self, invocation: ToolInvocation) -> Result { + self.ensure_supported()?; + let ToolInvocation { session, turn, @@ -98,3 +123,20 @@ impl ToolHandler for ViewImageHandler { }) } } + +#[cfg(test)] +mod tests { + use super::*; + use pretty_assertions::assert_eq; + + #[test] + fn unsupported_models_return_clear_message() { + let handler = ViewImageHandler::new(false); + + let err = handler.ensure_supported().expect_err("must reject"); + assert_eq!( + err, + FunctionCallError::RespondToModel(VIEW_IMAGE_UNSUPPORTED_MESSAGE.to_string()) + ); + } +} diff --git a/codex-rs/core/src/tools/spec.rs b/codex-rs/core/src/tools/spec.rs index d0c5a3c07dd..9b7eb41ccf9 100644 --- a/codex-rs/core/src/tools/spec.rs +++ b/codex-rs/core/src/tools/spec.rs @@ -1370,7 +1370,7 @@ pub(crate) fn build_specs( let plan_handler = Arc::new(PlanHandler); let apply_patch_handler = Arc::new(ApplyPatchHandler); let dynamic_tool_handler = Arc::new(DynamicToolHandler); - let view_image_handler = Arc::new(ViewImageHandler); + let view_image_handler = Arc::new(ViewImageHandler::new(config.supports_image_input)); let mcp_handler = Arc::new(McpHandler); let mcp_resource_handler = Arc::new(McpResourceHandler); let shell_command_handler = Arc::new(ShellCommandHandler); @@ -1498,10 +1498,8 @@ pub(crate) fn build_specs( Some(WebSearchMode::Disabled) | None => {} } - if config.supports_image_input { - builder.push_spec_with_parallel_support(create_view_image_tool(), true); - builder.register_handler("view_image", view_image_handler); - } + builder.push_spec_with_parallel_support(create_view_image_tool(), true); + builder.register_handler("view_image", view_image_handler); if config.collab_tools { let collab_handler = Arc::new(CollabHandler); @@ -2077,7 +2075,7 @@ mod tests { } #[test] - fn test_non_multimodal_models_exclude_view_image() { + fn test_non_multimodal_models_include_view_image() { let config = test_config(); let mut model_info = ModelsManager::construct_model_info_offline("gpt-5.1", &config); model_info.input_modalities = vec![InputModality::Text]; @@ -2091,11 +2089,11 @@ mod tests { let (tools, _) = build_specs(&tools_config, Some(HashMap::new()), &[]).build(); assert!( - !tools + tools .iter() .map(|t| t.spec.name()) .any(|name| name == VIEW_IMAGE_TOOL_NAME), - "view_image should be excluded for non-multimodal models" + "view_image should be present for non-multimodal models" ); } From 15a976856ef84327776a82167320055d9e042702 Mon Sep 17 00:00:00 2001 From: Ahmed Ibrahim Date: Tue, 10 Feb 2026 09:31:08 -0800 Subject: [PATCH 2/7] tests --- AGENTS.md | 160 ------------------ .../core/src/tools/handlers/view_image.rs | 19 +-- codex-rs/core/tests/suite/view_image.rs | 118 +++++++++++++ 3 files changed, 119 insertions(+), 178 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 292e4c92d97..e69de29bb2d 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -1,160 +0,0 @@ -# Rust/codex-rs - -In the codex-rs folder where the rust code lives: - -- Crate names are prefixed with `codex-`. For example, the `core` folder's crate is named `codex-core` -- When using format! and you can inline variables into {}, always do that. -- Install any commands the repo relies on (for example `just`, `rg`, or `cargo-insta`) if they aren't already available before running instructions here. -- Never add or modify any code related to `CODEX_SANDBOX_NETWORK_DISABLED_ENV_VAR` or `CODEX_SANDBOX_ENV_VAR`. - - You operate in a sandbox where `CODEX_SANDBOX_NETWORK_DISABLED=1` will be set whenever you use the `shell` tool. Any existing code that uses `CODEX_SANDBOX_NETWORK_DISABLED_ENV_VAR` was authored with this fact in mind. It is often used to early exit out of tests that the author knew you would not be able to run given your sandbox limitations. - - Similarly, when you spawn a process using Seatbelt (`/usr/bin/sandbox-exec`), `CODEX_SANDBOX=seatbelt` will be set on the child process. Integration tests that want to run Seatbelt themselves cannot be run under Seatbelt, so checks for `CODEX_SANDBOX=seatbelt` are also often used to early exit out of tests, as appropriate. -- Always collapse if statements per https://rust-lang.github.io/rust-clippy/master/index.html#collapsible_if -- Always inline format! args when possible per https://rust-lang.github.io/rust-clippy/master/index.html#uninlined_format_args -- Use method references over closures when possible per https://rust-lang.github.io/rust-clippy/master/index.html#redundant_closure_for_method_calls -- When possible, make `match` statements exhaustive and avoid wildcard arms. -- When writing tests, prefer comparing the equality of entire objects over fields one by one. -- When making a change that adds or changes an API, ensure that the documentation in the `docs/` folder is up to date if applicable. -- If you change `ConfigToml` or nested config types, run `just write-config-schema` to update `codex-rs/core/config.schema.json`. -- Do not create small helper methods that are referenced only once. - -Run `just fmt` (in `codex-rs` directory) automatically after you have finished making Rust code changes; do not ask for approval to run it. Additionally, run the tests: - -1. Run the test for the specific project that was changed. For example, if changes were made in `codex-rs/tui`, run `cargo test -p codex-tui`. -2. Once those pass, if any changes were made in common, core, or protocol, run the complete test suite with `cargo test --all-features`. project-specific or individual tests can be run without asking the user, but do ask the user before running the complete test suite. - -Before finalizing a large change to `codex-rs`, run `just fix -p ` (in `codex-rs` directory) to fix any linter issues in the code. Prefer scoping with `-p` to avoid slow workspace‑wide Clippy builds; only run `just fix` without `-p` if you changed shared crates. Do not re-run tests after running `fix` or `fmt`. - -## TUI style conventions - -See `codex-rs/tui/styles.md`. - -## TUI code conventions - -- Use concise styling helpers from ratatui’s Stylize trait. - - Basic spans: use "text".into() - - Styled spans: use "text".red(), "text".green(), "text".magenta(), "text".dim(), etc. - - Prefer these over constructing styles with `Span::styled` and `Style` directly. - - Example: patch summary file lines - - Desired: vec![" └ ".into(), "M".red(), " ".dim(), "tui/src/app.rs".dim()] - -### TUI Styling (ratatui) - -- Prefer Stylize helpers: use "text".dim(), .bold(), .cyan(), .italic(), .underlined() instead of manual Style where possible. -- Prefer simple conversions: use "text".into() for spans and vec![…].into() for lines; when inference is ambiguous (e.g., Paragraph::new/Cell::from), use Line::from(spans) or Span::from(text). -- Computed styles: if the Style is computed at runtime, using `Span::styled` is OK (`Span::from(text).set_style(style)` is also acceptable). -- Avoid hardcoded white: do not use `.white()`; prefer the default foreground (no color). -- Chaining: combine helpers by chaining for readability (e.g., url.cyan().underlined()). -- Single items: prefer "text".into(); use Line::from(text) or Span::from(text) only when the target type isn’t obvious from context, or when using .into() would require extra type annotations. -- Building lines: use vec![…].into() to construct a Line when the target type is obvious and no extra type annotations are needed; otherwise use Line::from(vec![…]). -- Avoid churn: don’t refactor between equivalent forms (Span::styled ↔ set_style, Line::from ↔ .into()) without a clear readability or functional gain; follow file‑local conventions and do not introduce type annotations solely to satisfy .into(). -- Compactness: prefer the form that stays on one line after rustfmt; if only one of Line::from(vec![…]) or vec![…].into() avoids wrapping, choose that. If both wrap, pick the one with fewer wrapped lines. - -### Text wrapping - -- Always use textwrap::wrap to wrap plain strings. -- If you have a ratatui Line and you want to wrap it, use the helpers in tui/src/wrapping.rs, e.g. word_wrap_lines / word_wrap_line. -- If you need to indent wrapped lines, use the initial_indent / subsequent_indent options from RtOptions if you can, rather than writing custom logic. -- If you have a list of lines and you need to prefix them all with some prefix (optionally different on the first vs subsequent lines), use the `prefix_lines` helper from line_utils. - -## Tests - -### Snapshot tests - -This repo uses snapshot tests (via `insta`), especially in `codex-rs/tui`, to validate rendered output. When UI or text output changes intentionally, update the snapshots as follows: - -- Run tests to generate any updated snapshots: - - `cargo test -p codex-tui` -- Check what’s pending: - - `cargo insta pending-snapshots -p codex-tui` -- Review changes by reading the generated `*.snap.new` files directly in the repo, or preview a specific file: - - `cargo insta show -p codex-tui path/to/file.snap.new` -- Only if you intend to accept all new snapshots in this crate, run: - - `cargo insta accept -p codex-tui` - -If you don’t have the tool: - -- `cargo install cargo-insta` - -### Test assertions - -- Tests should use pretty_assertions::assert_eq for clearer diffs. Import this at the top of the test module if it isn't already. -- Prefer deep equals comparisons whenever possible. Perform `assert_eq!()` on entire objects, rather than individual fields. -- Avoid mutating process environment in tests; prefer passing environment-derived flags or dependencies from above. - -### Spawning workspace binaries in tests (Cargo vs Bazel) - -- Prefer `codex_utils_cargo_bin::cargo_bin("...")` over `assert_cmd::Command::cargo_bin(...)` or `escargot` when tests need to spawn first-party binaries. - - Under Bazel, binaries and resources may live under runfiles; use `codex_utils_cargo_bin::cargo_bin` to resolve absolute paths that remain stable after `chdir`. -- When locating fixture files or test resources under Bazel, avoid `env!("CARGO_MANIFEST_DIR")`. Prefer `codex_utils_cargo_bin::find_resource!` so paths resolve correctly under both Cargo and Bazel runfiles. - -### Integration tests (core) - -- Prefer the utilities in `core_test_support::responses` when writing end-to-end Codex tests. - -- All `mount_sse*` helpers return a `ResponseMock`; hold onto it so you can assert against outbound `/responses` POST bodies. -- Use `ResponseMock::single_request()` when a test should only issue one POST, or `ResponseMock::requests()` to inspect every captured `ResponsesRequest`. -- `ResponsesRequest` exposes helpers (`body_json`, `input`, `function_call_output`, `custom_tool_call_output`, `call_output`, `header`, `path`, `query_param`) so assertions can target structured payloads instead of manual JSON digging. -- Build SSE payloads with the provided `ev_*` constructors and the `sse(...)`. -- Prefer `wait_for_event` over `wait_for_event_with_timeout`. -- Prefer `mount_sse_once` over `mount_sse_once_match` or `mount_sse_sequence` - -- Typical pattern: - - ```rust - let mock = responses::mount_sse_once(&server, responses::sse(vec![ - responses::ev_response_created("resp-1"), - responses::ev_function_call(call_id, "shell", &serde_json::to_string(&args)?), - responses::ev_completed("resp-1"), - ])).await; - - codex.submit(Op::UserTurn { ... }).await?; - - // Assert request body if needed. - let request = mock.single_request(); - // assert using request.function_call_output(call_id) or request.json_body() or other helpers. - ``` - -## App-server API Development Best Practices - -These guidelines apply to app-server protocol work in `codex-rs`, especially: - -- `app-server-protocol/src/protocol/common.rs` -- `app-server-protocol/src/protocol/v2.rs` -- `app-server/README.md` - -### Core Rules - -- All active API development should happen in app-server v2. Do not add new API surface area to v1. -- Follow payload naming consistently: - `*Params` for request payloads, `*Response` for responses, and `*Notification` for notifications. -- Expose RPC methods as `/` and keep `` singular (for example, `thread/read`, `app/list`). -- Always expose fields as camelCase on the wire with `#[serde(rename_all = "camelCase")]` unless a tagged union or explicit compatibility requirement needs a targeted rename. -- Exception: config RPC payloads are expected to use snake_case to mirror config.toml keys (see the config read/write/list APIs in `app-server-protocol/src/protocol/v2.rs`). -- Always set `#[ts(export_to = "v2/")]` on v2 request/response/notification types so generated TypeScript lands in the correct namespace. -- Never use `#[serde(skip_serializing_if = "Option::is_none")]` for v2 API payload fields. - Exception: client->server requests that intentionally have no params may use: - `params: #[ts(type = "undefined")] #[serde(skip_serializing_if = "Option::is_none")] Option<()>`. -- Keep Rust and TS wire renames aligned. If a field or variant uses `#[serde(rename = "...")]`, add matching `#[ts(rename = "...")]`. -- For discriminated unions, use explicit tagging in both serializers: - `#[serde(tag = "type", ...)]` and `#[ts(tag = "type", ...)]`. -- Prefer plain `String` IDs at the API boundary (do UUID parsing/conversion internally if needed). -- Timestamps should be integer Unix seconds (`i64`) and named `*_at` (for example, `created_at`, `updated_at`, `resets_at`). -- For experimental API surface area: - use `#[experimental("method/or/field")]`, derive `ExperimentalApi` when field-level gating is needed, and use `inspect_params: true` in `common.rs` when only some fields of a method are experimental. - -### Client->server request payloads (`*Params`) - -- Every optional field must be annotated with `#[ts(optional = nullable)]`. Do not use `#[ts(optional = nullable)]` outside client->server request payloads (`*Params`). -- Optional collection fields (for example `Vec`, `HashMap`) must use `Option<...>` + `#[ts(optional = nullable)]`. Do not use `#[serde(default)]` to model optional collections, and do not use `skip_serializing_if` on v2 payload fields. -- When you want omission to mean `false` for boolean fields, use `#[serde(default, skip_serializing_if = "std::ops::Not::not")] pub field: bool` over `Option`. -- For new list methods, implement cursor pagination by default: - request fields `pub cursor: Option` and `pub limit: Option`, - response fields `pub data: Vec<...>` and `pub next_cursor: Option`. - -### Development Workflow - -- Update docs/examples when API behavior changes (at minimum `app-server/README.md`). -- Regenerate schema fixtures when API shapes change: - `just write-app-server-schema` - (and `just write-app-server-schema --experimental` when experimental API fixtures are affected). -- Validate with `cargo test -p codex-app-server-protocol`. diff --git a/codex-rs/core/src/tools/handlers/view_image.rs b/codex-rs/core/src/tools/handlers/view_image.rs index dc278fabe1e..e7896dffc58 100644 --- a/codex-rs/core/src/tools/handlers/view_image.rs +++ b/codex-rs/core/src/tools/handlers/view_image.rs @@ -21,7 +21,7 @@ pub struct ViewImageHandler { } const VIEW_IMAGE_UNSUPPORTED_MESSAGE: &str = - "view_image is not allowed because the current model does not support image inputs"; + "view_image is not allowed because you do not support image inputs"; impl ViewImageHandler { pub fn new(supports_image_input: bool) -> Self { @@ -123,20 +123,3 @@ impl ToolHandler for ViewImageHandler { }) } } - -#[cfg(test)] -mod tests { - use super::*; - use pretty_assertions::assert_eq; - - #[test] - fn unsupported_models_return_clear_message() { - let handler = ViewImageHandler::new(false); - - let err = handler.ensure_supported().expect_err("must reject"); - assert_eq!( - err, - FunctionCallError::RespondToModel(VIEW_IMAGE_UNSUPPORTED_MESSAGE.to_string()) - ); - } -} diff --git a/codex-rs/core/tests/suite/view_image.rs b/codex-rs/core/tests/suite/view_image.rs index b72f66fce84..7c9b6d21773 100644 --- a/codex-rs/core/tests/suite/view_image.rs +++ b/codex-rs/core/tests/suite/view_image.rs @@ -2,17 +2,28 @@ use base64::Engine; use base64::engine::general_purpose::STANDARD as BASE64_STANDARD; +use codex_core::CodexAuth; +use codex_core::features::Feature; use codex_core::protocol::AskForApproval; use codex_core::protocol::EventMsg; use codex_core::protocol::Op; use codex_core::protocol::SandboxPolicy; use codex_protocol::config_types::ReasoningSummary; +use codex_protocol::openai_models::ConfigShellToolType; +use codex_protocol::openai_models::InputModality; +use codex_protocol::openai_models::ModelInfo; +use codex_protocol::openai_models::ModelVisibility; +use codex_protocol::openai_models::ModelsResponse; +use codex_protocol::openai_models::ReasoningEffort; +use codex_protocol::openai_models::ReasoningEffortPreset; +use codex_protocol::openai_models::TruncationPolicyConfig; use codex_protocol::user_input::UserInput; use core_test_support::responses; use core_test_support::responses::ev_assistant_message; use core_test_support::responses::ev_completed; use core_test_support::responses::ev_function_call; use core_test_support::responses::ev_response_created; +use core_test_support::responses::mount_models_once; use core_test_support::responses::sse; use core_test_support::responses::start_mock_server; use core_test_support::skip_if_no_network; @@ -521,6 +532,113 @@ async fn view_image_tool_errors_when_file_missing() -> anyhow::Result<()> { Ok(()) } +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn view_image_tool_returns_unsupported_message_for_text_only_model() -> anyhow::Result<()> { + skip_if_no_network!(Ok(())); + + let server = start_mock_server().await; + let model_slug = "text-only-view-image-test-model"; + let text_only_model = ModelInfo { + slug: model_slug.to_string(), + display_name: "Text-only view_image test model".to_string(), + description: Some("Remote model for view_image unsupported-path coverage".to_string()), + default_reasoning_level: Some(ReasoningEffort::Medium), + supported_reasoning_levels: vec![ReasoningEffortPreset { + effort: ReasoningEffort::Medium, + description: ReasoningEffort::Medium.to_string(), + }], + shell_type: ConfigShellToolType::ShellCommand, + visibility: ModelVisibility::List, + supported_in_api: true, + input_modalities: vec![InputModality::Text], + priority: 1, + upgrade: None, + base_instructions: "base instructions".to_string(), + model_messages: None, + supports_reasoning_summaries: false, + support_verbosity: false, + default_verbosity: None, + apply_patch_tool_type: None, + truncation_policy: TruncationPolicyConfig::bytes(10_000), + supports_parallel_tool_calls: false, + context_window: Some(272_000), + auto_compact_token_limit: None, + effective_context_window_percent: 95, + experimental_supported_tools: Vec::new(), + }; + mount_models_once( + &server, + ModelsResponse { + models: vec![text_only_model], + }, + ) + .await; + + let TestCodex { codex, cwd, .. } = test_codex() + .with_auth(CodexAuth::create_dummy_chatgpt_auth_for_testing()) + .with_config(|config| { + config.features.enable(Feature::RemoteModels); + config.model = Some(model_slug.to_string()); + }) + .build(&server) + .await?; + + let rel_path = "assets/example.png"; + let abs_path = cwd.path().join(rel_path); + if let Some(parent) = abs_path.parent() { + std::fs::create_dir_all(parent)?; + } + let image = ImageBuffer::from_pixel(20, 20, Rgba([255u8, 0, 0, 255])); + image.save(&abs_path)?; + + let call_id = "view-image-unsupported-model"; + let arguments = serde_json::json!({ "path": rel_path }).to_string(); + let first_response = sse(vec![ + ev_response_created("resp-1"), + ev_function_call(call_id, "view_image", &arguments), + ev_completed("resp-1"), + ]); + responses::mount_sse_once(&server, first_response).await; + + let second_response = sse(vec![ + ev_assistant_message("msg-1", "done"), + ev_completed("resp-2"), + ]); + let mock = responses::mount_sse_once(&server, second_response).await; + + codex + .submit(Op::UserTurn { + items: vec![UserInput::Text { + text: "please attach the image".into(), + text_elements: Vec::new(), + }], + final_output_json_schema: None, + cwd: cwd.path().to_path_buf(), + approval_policy: AskForApproval::Never, + sandbox_policy: SandboxPolicy::DangerFullAccess, + model: model_slug.to_string(), + effort: None, + summary: ReasoningSummary::Auto, + collaboration_mode: None, + personality: None, + }) + .await?; + + wait_for_event(&codex, |event| matches!(event, EventMsg::TurnComplete(_))).await; + + let output_text = mock + .single_request() + .function_call_output_content_and_success(call_id) + .and_then(|(content, _)| content) + .expect("output text present"); + assert_eq!( + output_text, + "view_image is not allowed because you do not support image inputs" + ); + + Ok(()) +} + #[cfg(not(debug_assertions))] #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn replaces_invalid_local_image_after_bad_request() -> anyhow::Result<()> { From 5eb631282279820c7fd347740d2dd1030d978ec0 Mon Sep 17 00:00:00 2001 From: Ahmed Ibrahim Date: Tue, 10 Feb 2026 09:34:01 -0800 Subject: [PATCH 3/7] tests --- codex-rs/core/src/tools/handlers/view_image.rs | 18 ++++++------------ codex-rs/core/src/tools/spec.rs | 5 +---- 2 files changed, 7 insertions(+), 16 deletions(-) diff --git a/codex-rs/core/src/tools/handlers/view_image.rs b/codex-rs/core/src/tools/handlers/view_image.rs index e7896dffc58..8adebc71cb5 100644 --- a/codex-rs/core/src/tools/handlers/view_image.rs +++ b/codex-rs/core/src/tools/handlers/view_image.rs @@ -1,5 +1,6 @@ use async_trait::async_trait; use codex_protocol::models::FunctionCallOutputBody; +use codex_protocol::openai_models::InputModality; use serde::Deserialize; use tokio::fs; @@ -16,22 +17,14 @@ use codex_protocol::models::ContentItem; use codex_protocol::models::ResponseInputItem; use codex_protocol::models::local_image_content_items_with_label_number; -pub struct ViewImageHandler { - supports_image_input: bool, -} +pub struct ViewImageHandler; const VIEW_IMAGE_UNSUPPORTED_MESSAGE: &str = "view_image is not allowed because you do not support image inputs"; impl ViewImageHandler { - pub fn new(supports_image_input: bool) -> Self { - Self { - supports_image_input, - } - } - - fn ensure_supported(&self) -> Result<(), FunctionCallError> { - if self.supports_image_input { + fn ensure_supported(&self, model_supports_image_input: bool) -> Result<(), FunctionCallError> { + if model_supports_image_input { Ok(()) } else { Err(FunctionCallError::RespondToModel( @@ -53,7 +46,8 @@ impl ToolHandler for ViewImageHandler { } async fn handle(&self, invocation: ToolInvocation) -> Result { - self.ensure_supported()?; + let model_supports_image_input = invocation.turn.model_info.input_modalities.contains(&InputModality::Image); + self.ensure_supported(model_supports_image_input)?; let ToolInvocation { session, diff --git a/codex-rs/core/src/tools/spec.rs b/codex-rs/core/src/tools/spec.rs index 9b7eb41ccf9..b3e76ebeaa4 100644 --- a/codex-rs/core/src/tools/spec.rs +++ b/codex-rs/core/src/tools/spec.rs @@ -17,7 +17,6 @@ use codex_protocol::dynamic_tools::DynamicToolSpec; use codex_protocol::models::VIEW_IMAGE_TOOL_NAME; use codex_protocol::openai_models::ApplyPatchToolType; use codex_protocol::openai_models::ConfigShellToolType; -use codex_protocol::openai_models::InputModality; use codex_protocol::openai_models::ModelInfo; use serde::Deserialize; use serde::Serialize; @@ -31,7 +30,6 @@ pub(crate) struct ToolsConfig { pub shell_type: ConfigShellToolType, pub apply_patch_tool_type: Option, pub web_search_mode: Option, - pub supports_image_input: bool, pub search_tool: bool, pub collab_tools: bool, pub collaboration_modes_tools: bool, @@ -87,7 +85,6 @@ impl ToolsConfig { shell_type, apply_patch_tool_type, web_search_mode: *web_search_mode, - supports_image_input: model_info.input_modalities.contains(&InputModality::Image), search_tool: include_search_tool, collab_tools: include_collab_tools, collaboration_modes_tools: include_collaboration_modes_tools, @@ -1370,7 +1367,7 @@ pub(crate) fn build_specs( let plan_handler = Arc::new(PlanHandler); let apply_patch_handler = Arc::new(ApplyPatchHandler); let dynamic_tool_handler = Arc::new(DynamicToolHandler); - let view_image_handler = Arc::new(ViewImageHandler::new(config.supports_image_input)); + let view_image_handler = Arc::new(ViewImageHandler); let mcp_handler = Arc::new(McpHandler); let mcp_resource_handler = Arc::new(McpResourceHandler); let shell_command_handler = Arc::new(ShellCommandHandler); From 7e1cfa640826964b95c9adfc84e3b90adbe4a87e Mon Sep 17 00:00:00 2001 From: Ahmed Ibrahim Date: Tue, 10 Feb 2026 09:35:12 -0800 Subject: [PATCH 4/7] tests --- codex-rs/core/src/tools/spec.rs | 23 ----------------------- 1 file changed, 23 deletions(-) diff --git a/codex-rs/core/src/tools/spec.rs b/codex-rs/core/src/tools/spec.rs index b3e76ebeaa4..7254252eb40 100644 --- a/codex-rs/core/src/tools/spec.rs +++ b/codex-rs/core/src/tools/spec.rs @@ -2071,29 +2071,6 @@ mod tests { ); } - #[test] - fn test_non_multimodal_models_include_view_image() { - let config = test_config(); - let mut model_info = ModelsManager::construct_model_info_offline("gpt-5.1", &config); - model_info.input_modalities = vec![InputModality::Text]; - let mut features = Features::with_defaults(); - features.enable(Feature::CollaborationModes); - let tools_config = ToolsConfig::new(&ToolsConfigParams { - model_info: &model_info, - features: &features, - web_search_mode: Some(WebSearchMode::Cached), - }); - let (tools, _) = build_specs(&tools_config, Some(HashMap::new()), &[]).build(); - - assert!( - tools - .iter() - .map(|t| t.spec.name()) - .any(|name| name == VIEW_IMAGE_TOOL_NAME), - "view_image should be present for non-multimodal models" - ); - } - #[test] fn test_gpt_5_1_codex_max_unified_exec_web_search() { let mut features = Features::with_defaults(); From 99113cc811d2804612e3163754357253973ec489 Mon Sep 17 00:00:00 2001 From: Ahmed Ibrahim Date: Tue, 10 Feb 2026 09:36:55 -0800 Subject: [PATCH 5/7] fix --- .../core/src/tools/handlers/view_image.rs | 24 ++++++++----------- 1 file changed, 10 insertions(+), 14 deletions(-) diff --git a/codex-rs/core/src/tools/handlers/view_image.rs b/codex-rs/core/src/tools/handlers/view_image.rs index 8adebc71cb5..cfcd5ec2abc 100644 --- a/codex-rs/core/src/tools/handlers/view_image.rs +++ b/codex-rs/core/src/tools/handlers/view_image.rs @@ -22,18 +22,6 @@ pub struct ViewImageHandler; const VIEW_IMAGE_UNSUPPORTED_MESSAGE: &str = "view_image is not allowed because you do not support image inputs"; -impl ViewImageHandler { - fn ensure_supported(&self, model_supports_image_input: bool) -> Result<(), FunctionCallError> { - if model_supports_image_input { - Ok(()) - } else { - Err(FunctionCallError::RespondToModel( - VIEW_IMAGE_UNSUPPORTED_MESSAGE.to_string(), - )) - } - } -} - #[derive(Deserialize)] struct ViewImageArgs { path: String, @@ -46,8 +34,16 @@ impl ToolHandler for ViewImageHandler { } async fn handle(&self, invocation: ToolInvocation) -> Result { - let model_supports_image_input = invocation.turn.model_info.input_modalities.contains(&InputModality::Image); - self.ensure_supported(model_supports_image_input)?; + if !invocation + .turn + .model_info + .input_modalities + .contains(&InputModality::Image) + { + return Err(FunctionCallError::RespondToModel( + VIEW_IMAGE_UNSUPPORTED_MESSAGE.to_string(), + )); + } let ToolInvocation { session, From f098d7d309c7a9f34b2360626d0c69910ec77923 Mon Sep 17 00:00:00 2001 From: Ahmed Ibrahim Date: Tue, 10 Feb 2026 09:37:16 -0800 Subject: [PATCH 6/7] fix --- AGENTS.md | 160 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 160 insertions(+) diff --git a/AGENTS.md b/AGENTS.md index e69de29bb2d..292e4c92d97 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -0,0 +1,160 @@ +# Rust/codex-rs + +In the codex-rs folder where the rust code lives: + +- Crate names are prefixed with `codex-`. For example, the `core` folder's crate is named `codex-core` +- When using format! and you can inline variables into {}, always do that. +- Install any commands the repo relies on (for example `just`, `rg`, or `cargo-insta`) if they aren't already available before running instructions here. +- Never add or modify any code related to `CODEX_SANDBOX_NETWORK_DISABLED_ENV_VAR` or `CODEX_SANDBOX_ENV_VAR`. + - You operate in a sandbox where `CODEX_SANDBOX_NETWORK_DISABLED=1` will be set whenever you use the `shell` tool. Any existing code that uses `CODEX_SANDBOX_NETWORK_DISABLED_ENV_VAR` was authored with this fact in mind. It is often used to early exit out of tests that the author knew you would not be able to run given your sandbox limitations. + - Similarly, when you spawn a process using Seatbelt (`/usr/bin/sandbox-exec`), `CODEX_SANDBOX=seatbelt` will be set on the child process. Integration tests that want to run Seatbelt themselves cannot be run under Seatbelt, so checks for `CODEX_SANDBOX=seatbelt` are also often used to early exit out of tests, as appropriate. +- Always collapse if statements per https://rust-lang.github.io/rust-clippy/master/index.html#collapsible_if +- Always inline format! args when possible per https://rust-lang.github.io/rust-clippy/master/index.html#uninlined_format_args +- Use method references over closures when possible per https://rust-lang.github.io/rust-clippy/master/index.html#redundant_closure_for_method_calls +- When possible, make `match` statements exhaustive and avoid wildcard arms. +- When writing tests, prefer comparing the equality of entire objects over fields one by one. +- When making a change that adds or changes an API, ensure that the documentation in the `docs/` folder is up to date if applicable. +- If you change `ConfigToml` or nested config types, run `just write-config-schema` to update `codex-rs/core/config.schema.json`. +- Do not create small helper methods that are referenced only once. + +Run `just fmt` (in `codex-rs` directory) automatically after you have finished making Rust code changes; do not ask for approval to run it. Additionally, run the tests: + +1. Run the test for the specific project that was changed. For example, if changes were made in `codex-rs/tui`, run `cargo test -p codex-tui`. +2. Once those pass, if any changes were made in common, core, or protocol, run the complete test suite with `cargo test --all-features`. project-specific or individual tests can be run without asking the user, but do ask the user before running the complete test suite. + +Before finalizing a large change to `codex-rs`, run `just fix -p ` (in `codex-rs` directory) to fix any linter issues in the code. Prefer scoping with `-p` to avoid slow workspace‑wide Clippy builds; only run `just fix` without `-p` if you changed shared crates. Do not re-run tests after running `fix` or `fmt`. + +## TUI style conventions + +See `codex-rs/tui/styles.md`. + +## TUI code conventions + +- Use concise styling helpers from ratatui’s Stylize trait. + - Basic spans: use "text".into() + - Styled spans: use "text".red(), "text".green(), "text".magenta(), "text".dim(), etc. + - Prefer these over constructing styles with `Span::styled` and `Style` directly. + - Example: patch summary file lines + - Desired: vec![" └ ".into(), "M".red(), " ".dim(), "tui/src/app.rs".dim()] + +### TUI Styling (ratatui) + +- Prefer Stylize helpers: use "text".dim(), .bold(), .cyan(), .italic(), .underlined() instead of manual Style where possible. +- Prefer simple conversions: use "text".into() for spans and vec![…].into() for lines; when inference is ambiguous (e.g., Paragraph::new/Cell::from), use Line::from(spans) or Span::from(text). +- Computed styles: if the Style is computed at runtime, using `Span::styled` is OK (`Span::from(text).set_style(style)` is also acceptable). +- Avoid hardcoded white: do not use `.white()`; prefer the default foreground (no color). +- Chaining: combine helpers by chaining for readability (e.g., url.cyan().underlined()). +- Single items: prefer "text".into(); use Line::from(text) or Span::from(text) only when the target type isn’t obvious from context, or when using .into() would require extra type annotations. +- Building lines: use vec![…].into() to construct a Line when the target type is obvious and no extra type annotations are needed; otherwise use Line::from(vec![…]). +- Avoid churn: don’t refactor between equivalent forms (Span::styled ↔ set_style, Line::from ↔ .into()) without a clear readability or functional gain; follow file‑local conventions and do not introduce type annotations solely to satisfy .into(). +- Compactness: prefer the form that stays on one line after rustfmt; if only one of Line::from(vec![…]) or vec![…].into() avoids wrapping, choose that. If both wrap, pick the one with fewer wrapped lines. + +### Text wrapping + +- Always use textwrap::wrap to wrap plain strings. +- If you have a ratatui Line and you want to wrap it, use the helpers in tui/src/wrapping.rs, e.g. word_wrap_lines / word_wrap_line. +- If you need to indent wrapped lines, use the initial_indent / subsequent_indent options from RtOptions if you can, rather than writing custom logic. +- If you have a list of lines and you need to prefix them all with some prefix (optionally different on the first vs subsequent lines), use the `prefix_lines` helper from line_utils. + +## Tests + +### Snapshot tests + +This repo uses snapshot tests (via `insta`), especially in `codex-rs/tui`, to validate rendered output. When UI or text output changes intentionally, update the snapshots as follows: + +- Run tests to generate any updated snapshots: + - `cargo test -p codex-tui` +- Check what’s pending: + - `cargo insta pending-snapshots -p codex-tui` +- Review changes by reading the generated `*.snap.new` files directly in the repo, or preview a specific file: + - `cargo insta show -p codex-tui path/to/file.snap.new` +- Only if you intend to accept all new snapshots in this crate, run: + - `cargo insta accept -p codex-tui` + +If you don’t have the tool: + +- `cargo install cargo-insta` + +### Test assertions + +- Tests should use pretty_assertions::assert_eq for clearer diffs. Import this at the top of the test module if it isn't already. +- Prefer deep equals comparisons whenever possible. Perform `assert_eq!()` on entire objects, rather than individual fields. +- Avoid mutating process environment in tests; prefer passing environment-derived flags or dependencies from above. + +### Spawning workspace binaries in tests (Cargo vs Bazel) + +- Prefer `codex_utils_cargo_bin::cargo_bin("...")` over `assert_cmd::Command::cargo_bin(...)` or `escargot` when tests need to spawn first-party binaries. + - Under Bazel, binaries and resources may live under runfiles; use `codex_utils_cargo_bin::cargo_bin` to resolve absolute paths that remain stable after `chdir`. +- When locating fixture files or test resources under Bazel, avoid `env!("CARGO_MANIFEST_DIR")`. Prefer `codex_utils_cargo_bin::find_resource!` so paths resolve correctly under both Cargo and Bazel runfiles. + +### Integration tests (core) + +- Prefer the utilities in `core_test_support::responses` when writing end-to-end Codex tests. + +- All `mount_sse*` helpers return a `ResponseMock`; hold onto it so you can assert against outbound `/responses` POST bodies. +- Use `ResponseMock::single_request()` when a test should only issue one POST, or `ResponseMock::requests()` to inspect every captured `ResponsesRequest`. +- `ResponsesRequest` exposes helpers (`body_json`, `input`, `function_call_output`, `custom_tool_call_output`, `call_output`, `header`, `path`, `query_param`) so assertions can target structured payloads instead of manual JSON digging. +- Build SSE payloads with the provided `ev_*` constructors and the `sse(...)`. +- Prefer `wait_for_event` over `wait_for_event_with_timeout`. +- Prefer `mount_sse_once` over `mount_sse_once_match` or `mount_sse_sequence` + +- Typical pattern: + + ```rust + let mock = responses::mount_sse_once(&server, responses::sse(vec![ + responses::ev_response_created("resp-1"), + responses::ev_function_call(call_id, "shell", &serde_json::to_string(&args)?), + responses::ev_completed("resp-1"), + ])).await; + + codex.submit(Op::UserTurn { ... }).await?; + + // Assert request body if needed. + let request = mock.single_request(); + // assert using request.function_call_output(call_id) or request.json_body() or other helpers. + ``` + +## App-server API Development Best Practices + +These guidelines apply to app-server protocol work in `codex-rs`, especially: + +- `app-server-protocol/src/protocol/common.rs` +- `app-server-protocol/src/protocol/v2.rs` +- `app-server/README.md` + +### Core Rules + +- All active API development should happen in app-server v2. Do not add new API surface area to v1. +- Follow payload naming consistently: + `*Params` for request payloads, `*Response` for responses, and `*Notification` for notifications. +- Expose RPC methods as `/` and keep `` singular (for example, `thread/read`, `app/list`). +- Always expose fields as camelCase on the wire with `#[serde(rename_all = "camelCase")]` unless a tagged union or explicit compatibility requirement needs a targeted rename. +- Exception: config RPC payloads are expected to use snake_case to mirror config.toml keys (see the config read/write/list APIs in `app-server-protocol/src/protocol/v2.rs`). +- Always set `#[ts(export_to = "v2/")]` on v2 request/response/notification types so generated TypeScript lands in the correct namespace. +- Never use `#[serde(skip_serializing_if = "Option::is_none")]` for v2 API payload fields. + Exception: client->server requests that intentionally have no params may use: + `params: #[ts(type = "undefined")] #[serde(skip_serializing_if = "Option::is_none")] Option<()>`. +- Keep Rust and TS wire renames aligned. If a field or variant uses `#[serde(rename = "...")]`, add matching `#[ts(rename = "...")]`. +- For discriminated unions, use explicit tagging in both serializers: + `#[serde(tag = "type", ...)]` and `#[ts(tag = "type", ...)]`. +- Prefer plain `String` IDs at the API boundary (do UUID parsing/conversion internally if needed). +- Timestamps should be integer Unix seconds (`i64`) and named `*_at` (for example, `created_at`, `updated_at`, `resets_at`). +- For experimental API surface area: + use `#[experimental("method/or/field")]`, derive `ExperimentalApi` when field-level gating is needed, and use `inspect_params: true` in `common.rs` when only some fields of a method are experimental. + +### Client->server request payloads (`*Params`) + +- Every optional field must be annotated with `#[ts(optional = nullable)]`. Do not use `#[ts(optional = nullable)]` outside client->server request payloads (`*Params`). +- Optional collection fields (for example `Vec`, `HashMap`) must use `Option<...>` + `#[ts(optional = nullable)]`. Do not use `#[serde(default)]` to model optional collections, and do not use `skip_serializing_if` on v2 payload fields. +- When you want omission to mean `false` for boolean fields, use `#[serde(default, skip_serializing_if = "std::ops::Not::not")] pub field: bool` over `Option`. +- For new list methods, implement cursor pagination by default: + request fields `pub cursor: Option` and `pub limit: Option`, + response fields `pub data: Vec<...>` and `pub next_cursor: Option`. + +### Development Workflow + +- Update docs/examples when API behavior changes (at minimum `app-server/README.md`). +- Regenerate schema fixtures when API shapes change: + `just write-app-server-schema` + (and `just write-app-server-schema --experimental` when experimental API fixtures are affected). +- Validate with `cargo test -p codex-app-server-protocol`. From f66e20aaa638c01f378f70a6f649175b1ec8f4a2 Mon Sep 17 00:00:00 2001 From: Ahmed Ibrahim Date: Tue, 10 Feb 2026 10:58:57 -0800 Subject: [PATCH 7/7] tests --- codex-rs/core/tests/suite/view_image.rs | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/codex-rs/core/tests/suite/view_image.rs b/codex-rs/core/tests/suite/view_image.rs index 7c9b6d21773..cabee944db1 100644 --- a/codex-rs/core/tests/suite/view_image.rs +++ b/codex-rs/core/tests/suite/view_image.rs @@ -37,6 +37,8 @@ use image::Rgba; use image::load_from_memory; use serde_json::Value; use tokio::time::Duration; +use wiremock::BodyPrintLimit; +use wiremock::MockServer; fn find_image_message(body: &Value) -> Option<&Value> { body.get("input") @@ -536,7 +538,14 @@ async fn view_image_tool_errors_when_file_missing() -> anyhow::Result<()> { async fn view_image_tool_returns_unsupported_message_for_text_only_model() -> anyhow::Result<()> { skip_if_no_network!(Ok(())); - let server = start_mock_server().await; + // Use MockServer directly (not start_mock_server) so the first /models request returns our + // text-only model. start_mock_server mounts empty models first, causing get_model_info to + // fall back to model_info_from_slug with default_input_modalities (Text+Image), which would + // incorrectly allow view_image. + let server = MockServer::builder() + .body_print_limit(BodyPrintLimit::Limited(80_000)) + .start() + .await; let model_slug = "text-only-view-image-test-model"; let text_only_model = ModelInfo { slug: model_slug.to_string(),