From 086d02fb147fa1aa056cc71e26afb10bc26849b6 Mon Sep 17 00:00:00 2001 From: pakrym-oai Date: Mon, 9 Feb 2026 12:01:30 -0800 Subject: [PATCH 01/23] Try to stop small helper methods (#11203) --- AGENTS.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/AGENTS.md b/AGENTS.md index 1506fd188c..292e4c92d9 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -15,13 +15,14 @@ In the codex-rs folder where the rust code lives: - 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. +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 From 86183847fdd99f19d7de0bae75ded15e78b08707 Mon Sep 17 00:00:00 2001 From: zbarsky-openai Date: Mon, 9 Feb 2026 15:08:10 -0500 Subject: [PATCH 02/23] [bazel] Upgrade some rulesets in preparation for enabling windows, part 2 (#11197) https://github.com/openai/codex/pull/11109 had automerge set, so I didn't get to address feedback before merging, oops! --- .github/workflows/ci.bazelrc | 2 +- MODULE.bazel | 12 +- MODULE.bazel.lock | 181 +++++++++--------- defs.bzl | 1 - .../toolchains_llvm_private_frameworks.patch | 12 -- 5 files changed, 97 insertions(+), 111 deletions(-) delete mode 100644 patches/toolchains_llvm_private_frameworks.patch diff --git a/.github/workflows/ci.bazelrc b/.github/workflows/ci.bazelrc index 34db2c8a07..997a774eb3 100644 --- a/.github/workflows/ci.bazelrc +++ b/.github/workflows/ci.bazelrc @@ -23,5 +23,5 @@ common:macos --config=remote common:macos --strategy=remote common:macos --strategy=TestRunner=darwin-sandbox,local -# On windows we can not cross-build the tests but run them locally due to what appears to be a Bazel bug +# On windows we cannot cross-build the tests but run them locally due to what appears to be a Bazel bug # (windows vs unix path confusion) diff --git a/MODULE.bazel b/MODULE.bazel index 689020d6ae..f12eeb8884 100644 --- a/MODULE.bazel +++ b/MODULE.bazel @@ -1,11 +1,10 @@ bazel_dep(name = "platforms", version = "1.0.0") -bazel_dep(name = "toolchains_llvm_bootstrapped", version = "0.5.2") +bazel_dep(name = "toolchains_llvm_bootstrapped", version = "0.5.3") single_version_override( module_name = "toolchains_llvm_bootstrapped", patch_strip = 1, patches = [ "//patches:toolchains_llvm_bootstrapped_resource_dir.patch", - "//patches:toolchains_llvm_private_frameworks.patch", ], ) @@ -50,16 +49,15 @@ use_repo(rules_rust, "rules_rust") toolchains = use_extension("@rules_rs//rs/experimental/toolchains:module_extension.bzl", "toolchains") toolchains.toolchain( edition = "2024", - # TODO(zbarsky): bump to 1.93 after fixing mingw - version = "1.92.0", + version = "1.93.0", ) use_repo( toolchains, - "experimental_rust_toolchains_1_92_0", - "rust_toolchain_artifacts_macos_aarch64_1_92_0", + "experimental_rust_toolchains_1_93_0", + "rust_toolchain_artifacts_macos_aarch64_1_93_0", ) -register_toolchains("@experimental_rust_toolchains_1_92_0//:all") +register_toolchains("@experimental_rust_toolchains_1_93_0//:all") crate = use_extension("@rules_rs//rs:extensions.bzl", "crate") crate.from_cargo( diff --git a/MODULE.bazel.lock b/MODULE.bazel.lock index 28499f59d7..7e43f74cbf 100644 --- a/MODULE.bazel.lock +++ b/MODULE.bazel.lock @@ -217,7 +217,8 @@ "https://bcr.bazel.build/modules/tar.bzl/0.6.0/MODULE.bazel": "a3584b4edcfafcabd9b0ef9819808f05b372957bbdff41601429d5fd0aac2e7c", "https://bcr.bazel.build/modules/tar.bzl/0.6.0/source.json": "4a620381df075a16cb3a7ed57bd1d05f7480222394c64a20fa51bdb636fda658", "https://bcr.bazel.build/modules/toolchains_llvm_bootstrapped/0.5.2/MODULE.bazel": "f7c822cea99caef928d7cbe695498096e53c4b2c0ea45997e9a64bf6b77b43b0", - "https://bcr.bazel.build/modules/toolchains_llvm_bootstrapped/0.5.2/source.json": "13d260b3a10804b3b2ab822c49e329c36ef5cd325fa01d0f9a1616c5364b7fff", + "https://bcr.bazel.build/modules/toolchains_llvm_bootstrapped/0.5.3/MODULE.bazel": "902ab5c2bc9eff04df45d17aac3c5c642afaad2d4569dcfdc86d0256f68f18e0", + "https://bcr.bazel.build/modules/toolchains_llvm_bootstrapped/0.5.3/source.json": "8c03ccd25188ecf8baf4a97a8b42c507ff592d917bcbc06333b9c82d18c7ef13", "https://bcr.bazel.build/modules/upb/0.0.0-20220923-a547704/MODULE.bazel": "7298990c00040a0e2f121f6c32544bab27d4452f80d9ce51349b1a28f3005c43", "https://bcr.bazel.build/modules/with_cfg.bzl/0.12.0/MODULE.bazel": "b573395fe63aef4299ba095173e2f62ccfee5ad9bbf7acaa95dba73af9fc2b38", "https://bcr.bazel.build/modules/with_cfg.bzl/0.12.0/source.json": "3f3fbaeafecaf629877ad152a2c9def21f8d330d91aa94c5dc75bbb98c10b8b8", @@ -1500,95 +1501,95 @@ "zvariant_utils_2.1.0": "{\"dependencies\":[{\"name\":\"proc-macro2\",\"req\":\"^1.0.81\"},{\"name\":\"quote\",\"req\":\"^1.0.36\"},{\"features\":[\"extra-traits\",\"full\"],\"name\":\"syn\",\"req\":\"^2.0.64\"}],\"features\":{}}" }, "@@rules_rs+//rs/experimental/toolchains:module_extension.bzl%toolchains": { - "cargo-1.92.0-aarch64-apple-darwin.tar.xz": "bce6e7def37240c5a63115828017a9fc0ebcb31e64115382f5943b62b71aa34a", - "cargo-1.92.0-aarch64-pc-windows-gnullvm.tar.xz": "56d38a6a89e2a38ec777938d74feb93a3498bc8788df96a94fcb4eac2e07338b", - "cargo-1.92.0-aarch64-unknown-linux-gnu.tar.xz": "cb2ce6be6411b986e25c71ad8a813f9dfbe3461738136fd684e3644f8dd75df4", - "cargo-1.92.0-x86_64-apple-darwin.tar.xz": "b033a7c33aba8af947c9d0ab2785f9696347cded228ffe731897f1c627466262", - "cargo-1.92.0-x86_64-pc-windows-gnullvm.tar.xz": "c27d5936e1c11feb33f3221b85741c33f783c8723fca84552e7c2a5a73959352", - "cargo-1.92.0-x86_64-unknown-linux-gnu.tar.xz": "e5e12be2c7126a7036c8adf573078a28b92611f5767cc9bd0a6f7c83081df103", - "clippy-1.92.0-aarch64-apple-darwin.tar.xz": "08c65b6cf8faae3861706f8c97acf2aa6b784ed9455354c3b13495a7cfe5cb84", - "clippy-1.92.0-aarch64-pc-windows-gnullvm.tar.xz": "ea3e63c684273f629f918e8f50d510225c48a35ec28eaf026fa68c27f273bbd6", - "clippy-1.92.0-aarch64-unknown-linux-gnu.tar.xz": "333ab38c673b589468b8293b525e5704fb52515d9d516ee28d3d34dd5a63d3c3", - "clippy-1.92.0-x86_64-apple-darwin.tar.xz": "39cce87aab3d8b71350edcb3f943fba7bc59581ce1e65e158ee01e64cf0f1cf5", - "clippy-1.92.0-x86_64-pc-windows-gnullvm.tar.xz": "b22f615210f328aabafa8d0eab3fed62784fa771956c51a92ae24a4dfd2073ed", - "clippy-1.92.0-x86_64-unknown-linux-gnu.tar.xz": "2c1bf6e7da8ec50feba03fe188fc9a744ba59e2c6ece7970c13e201d08defa9a", - "rust-std-1.92.0-aarch64-apple-darwin.tar.xz": "ea619984fcb8e24b05dbd568d599b8e10d904435ab458dfba6469e03e0fd69aa", - "rust-std-1.92.0-aarch64-apple-ios-macabi.tar.xz": "d0453906db0abe9efb595e1ed59feb131a94c0312c0bc72da6482936667ff8da", - "rust-std-1.92.0-aarch64-apple-ios-sim.tar.xz": "9362b66fbaf2503276ea34f34b41b171269db9ba4dce05dde3472ea9d1852601", - "rust-std-1.92.0-aarch64-apple-ios.tar.xz": "81fb496f94a3f52ec2818a76a7107905b13b37d490ca849d22c0a7a7d7b0125e", - "rust-std-1.92.0-aarch64-linux-android.tar.xz": "ce6350bd43856c630773c93e40310989c6cb98a1178233c44e512a31761943d0", - "rust-std-1.92.0-aarch64-pc-windows-gnullvm.tar.xz": "4e79f57b80040757a3425568b5978986f026daf771649a64021c74bcc138214b", - "rust-std-1.92.0-aarch64-pc-windows-msvc.tar.xz": "b20c5c696af4ecfb683370ca0ee3c76ab8726fe6470ce9f1368d41a5b06ea065", - "rust-std-1.92.0-aarch64-unknown-fuchsia.tar.xz": "6021246b6c0d9d6104c0b350f7cd48a31d5707edaa8063f77f5636fe07bdb79d", - "rust-std-1.92.0-aarch64-unknown-linux-gnu.tar.xz": "ce2ab42c09d633b0a8b4b65a297c700ae0fad47aae890f75894782f95be7e36d", - "rust-std-1.92.0-aarch64-unknown-linux-musl.tar.xz": "94b9f84f21d29825c55a27fbb6b4b9fb9296a4a841aa54d85b95c42445623363", - "rust-std-1.92.0-aarch64-unknown-none-softfloat.tar.xz": "0dc46fafaaa36f53eec49e14a69e1d6d9ac6f0b9624a01081ad311d8139a2be0", - "rust-std-1.92.0-aarch64-unknown-none.tar.xz": "ab6a2edab924739fc2c86e9f8fd8068b379e881a6261a177d66608d3ea4cacb1", - "rust-std-1.92.0-aarch64-unknown-uefi.tar.xz": "f98001222bf23743598382c232b08d3137035b53645a420a1425288d501808af", - "rust-std-1.92.0-arm-linux-androideabi.tar.xz": "d41ec7255556b605dda04201a23e4445b5b86bc6786c703f91eb985bddc9f4ca", - "rust-std-1.92.0-arm-unknown-linux-gnueabi.tar.xz": "35478e20f8cc13912b31f2905b313a2820ddae564b363a66ab7a5da39d12787f", - "rust-std-1.92.0-arm-unknown-linux-gnueabihf.tar.xz": "836ada282b65c57d71f9b7e6fb490832410c954aac905c5437fb0bf851b53c83", - "rust-std-1.92.0-arm-unknown-linux-musleabi.tar.xz": "91d3c5fdbda9ba2e778bb638e3a5d060f3021bbc7a60edf22b0035be4e611b30", - "rust-std-1.92.0-arm-unknown-linux-musleabihf.tar.xz": "20411e3858308add0dadde9ce2059d82cdd6c5339881fa93ac995e733df0f832", - "rust-std-1.92.0-armv7-linux-androideabi.tar.xz": "06ac2f08dcf5c480e7767c0541c9bd7460754ec3a369a4b7097b328eca28fab9", - "rust-std-1.92.0-armv7-unknown-linux-gnueabi.tar.xz": "3d5e3fb14441ea8e6fc6307cbd89efd69be42143ff7f2b8dfb19818ddca993c0", - "rust-std-1.92.0-armv7-unknown-linux-gnueabihf.tar.xz": "e3ac810db43067d8af9e17679d47534e871f1daad8cd0762e67da935681e9e19", - "rust-std-1.92.0-armv7-unknown-linux-musleabi.tar.xz": "89cbf7db934d543754b446a52398081ec40ee6b98ed5bca93ac1dbd5faf48c16", - "rust-std-1.92.0-armv7-unknown-linux-musleabihf.tar.xz": "c7c08a389bd351226fd52266bfe2d2c444597e1bbb5d0307da44bdfa4df62c99", - "rust-std-1.92.0-i686-linux-android.tar.xz": "e45832b005556f65c6d26f05f34bd4ab5ceea8d9b9fefc4175d52b0780ca89db", - "rust-std-1.92.0-i686-pc-windows-gnu.tar.xz": "b3eefe10d4aed3ccbeaff3ae9cd2e0e0a36894c0635d0e69af1c9a698679d75f", - "rust-std-1.92.0-i686-pc-windows-gnullvm.tar.xz": "e6d709f85dea51d81f2f1a030845b732b9f7761d95d93c57c7276b0a451c2993", - "rust-std-1.92.0-i686-pc-windows-msvc.tar.xz": "e5671b276047647e994a7cab99c90ee70c46787572fbe4e266a13c6edb84d5ce", - "rust-std-1.92.0-i686-unknown-freebsd.tar.xz": "e008a0506ec4d5eff30abdf376c7933e235670bd6c5e1131c52bcda097a21116", - "rust-std-1.92.0-i686-unknown-linux-gnu.tar.xz": "abc840631a4462a4c8ec61341110ff653ab2ef86ef3b10f84489d00cc8a9310d", - "rust-std-1.92.0-i686-unknown-linux-musl.tar.xz": "c796874b1343721f575203fa179dc258e09ac45cd95dd6c35c4d5979a3870494", - "rust-std-1.92.0-i686-unknown-uefi.tar.xz": "90da7759e28e62fb82454d4eb4e02ac14320b8da3372e02f9ca4f47f7afbfe40", - "rust-std-1.92.0-powerpc-unknown-linux-gnu.tar.xz": "c3e809a324b00eb53096c58df38645bb496c6560de334dfe04ed0b77c0605aaa", - "rust-std-1.92.0-powerpc64-unknown-linux-gnu.tar.xz": "2ce706afa4a46b6773340854de877fc63618a40e351298a4e3da8eb482619863", - "rust-std-1.92.0-powerpc64le-unknown-linux-gnu.tar.xz": "eba59766c2d9805c0a1fc82fd723acbb36569e1bec1088c037bba84d965f70ba", - "rust-std-1.92.0-powerpc64le-unknown-linux-musl.tar.xz": "8b515e18b6ac8f8d37ea3cabe644b7f571984333c3b4192b7f5877e79eae7893", - "rust-std-1.92.0-riscv32imc-unknown-none-elf.tar.xz": "e1c6968ce25ab78f2c5b5460691763a2d39cb71b01d6d54c208d4a8b735b0584", - "rust-std-1.92.0-riscv64gc-unknown-linux-gnu.tar.xz": "8ee20dcf3b1063fa6069b3ce85e1fcf42794dfa783263314865cb53fff42d9e4", - "rust-std-1.92.0-riscv64gc-unknown-linux-musl.tar.xz": "f20d822309900fd6c7230688694baf91e900154e44e6247feca49b7b7a203a57", - "rust-std-1.92.0-riscv64gc-unknown-none-elf.tar.xz": "7eacf6a98786d58ef912643fd5aedc35111712e96a47b7e5d3ccddcdf9fb166d", - "rust-std-1.92.0-s390x-unknown-linux-gnu.tar.xz": "ebf944dc95015498d322504a54e4f9cdb28590f7790aa3a9eb86d6cf4b6c93ff", - "rust-std-1.92.0-thumbv6m-none-eabi.tar.xz": "f55de77126b60e1da38f8a5cdd127db872155ce9fbb53d4459502dd47b1dd476", - "rust-std-1.92.0-thumbv7em-none-eabi.tar.xz": "fdee017dcebfa8675220c20ca65b945b6eaee744d5d19b92cb0ca5480dd19ba6", - "rust-std-1.92.0-thumbv7em-none-eabihf.tar.xz": "6bfd083ce4917440fb4b04ca39ab4dadc9f40d9dc2775056899857bfa8911fd0", - "rust-std-1.92.0-thumbv7m-none-eabi.tar.xz": "7dc4c92e97db5ce1ddd9fbb7fbb1ad2d367f1c2e20d296a6d6427480c900c315", - "rust-std-1.92.0-thumbv8m.main-none-eabi.tar.xz": "61a6a80d03ebdb80ab06a044d4ec60e3c2bd8dc4d6011e920cf41592df4f0646", - "rust-std-1.92.0-thumbv8m.main-none-eabihf.tar.xz": "24c2f65371a2a5c6a40b51ae0e276ea9afff0479255f180f5e1549a0a4d58fb3", - "rust-std-1.92.0-wasm32-unknown-emscripten.tar.xz": "59b7adf18c0cc416a005fad7f704203b965905eb1c0ed9c556daa3c14048b9f4", - "rust-std-1.92.0-wasm32-unknown-unknown.tar.xz": "6c73f053ccd6adc886f802270ba960fd854e5e1111e4b5cfb875f1fdcfc0eb60", - "rust-std-1.92.0-wasm32-wasip1-threads.tar.xz": "feea056dd657a26560dfddfe4b53daeef3ded83c140b453b6dbdebaabd2a664c", - "rust-std-1.92.0-wasm32-wasip1.tar.xz": "8107dc35f0b6b744d998e766351419b4e0d27cddd6456c461337753a25f29fcd", - "rust-std-1.92.0-wasm32-wasip2.tar.xz": "e69ce601b6b24eea08b0d9e1fb0d9bf2a4188b3109353b272be4d995f687c943", - "rust-std-1.92.0-x86_64-apple-darwin.tar.xz": "6ce143bf9e83c71e200f4180e8774ab22c8c8c2351c88484b13ff13be82c8d57", - "rust-std-1.92.0-x86_64-apple-ios-macabi.tar.xz": "6a292d774653f2deaac743060ec68bfd5d7ff01d9b364e1a7d1e57a679650b47", - "rust-std-1.92.0-x86_64-apple-ios.tar.xz": "b6e38e5f8c9e6fb294681a7951301856b8f9424af4589e14493c0c939338814c", - "rust-std-1.92.0-x86_64-linux-android.tar.xz": "ffd39429435ff2f0763b940dd6afb4b9ccb1ed443eeef4fff9f1e9b3c5730026", - "rust-std-1.92.0-x86_64-pc-windows-gnu.tar.xz": "d4043304ef0e4792fb79a1153cbeca41308aac37cb1af3aa6bc3f0bb6d2276e1", - "rust-std-1.92.0-x86_64-pc-windows-gnullvm.tar.xz": "6169605b3073a7c2d6960bc1c74cb9cd6b65f45ee34e7ede02368e07ce4564cf", - "rust-std-1.92.0-x86_64-pc-windows-msvc.tar.xz": "b4e53a9c9b96a1a0618364791b7728a1c8978101ec6d1ee78fe930e1ef061994", - "rust-std-1.92.0-x86_64-unknown-freebsd.tar.xz": "151929a4255175d14b2cbcb69ef46d9217add23be268b9cd1446f8eab16616f2", - "rust-std-1.92.0-x86_64-unknown-fuchsia.tar.xz": "51ec26391d166e2f190a329919e3c7bd793861f02284aba461a086268725d9f1", - "rust-std-1.92.0-x86_64-unknown-linux-gnu.tar.xz": "5f106805ed86ebf8df287039e53a45cf974391ef4d088c2760776b05b8e48b5d", - "rust-std-1.92.0-x86_64-unknown-linux-musl.tar.xz": "11f0b7efccdb5e7972e3c0fc23693a487abc28b624675c08161d055a016d527e", - "rust-std-1.92.0-x86_64-unknown-netbsd.tar.xz": "db6a8d3a091551701b12e40cc58d4a541adfb63f250074aae90d250329beb8de", - "rust-std-1.92.0-x86_64-unknown-none.tar.xz": "1d8420ab8eb241a35e38b76470277c722bd5a7aa4ac0c7a565ad6f30b37cb852", - "rust-std-1.92.0-x86_64-unknown-uefi.tar.xz": "1b849250cf095269f3a2c7bc2087a919386da7da28e80dc289e6268bc705142d", - "rustc-1.92.0-aarch64-apple-darwin.tar.xz": "15dee753c9217dff4cf45d734b29dc13ce6017d8a55fe34eed75022b39a63ff0", - "rustc-1.92.0-aarch64-pc-windows-gnullvm.tar.xz": "a7556b86bce94dd8c078a62f71a9a0a3f4b3e841bc5d4fae546d797b78186eb1", - "rustc-1.92.0-aarch64-unknown-linux-gnu.tar.xz": "7c8706fad4c038b5eacab0092e15db54d2b365d5f3323ca046fe987f814e7826", - "rustc-1.92.0-x86_64-apple-darwin.tar.xz": "0facbd5d2742c8e97c53d59c9b5b81db6088cfc285d9ecb99523a50d6765fc5c", - "rustc-1.92.0-x86_64-pc-windows-gnullvm.tar.xz": "e67ec11b4c6e04e95effa7b4063ae8327c2861c6d08a7c692d69a0f1adcd8ecb", - "rustc-1.92.0-x86_64-unknown-linux-gnu.tar.xz": "78b2dd9c6b1fcd2621fa81c611cf5e2d6950690775038b585c64f364422886e0", - "rustfmt-1.92.0-aarch64-apple-darwin.tar.xz": "5d8ea865a7999dc9141603be8a9352745bf8440da051eb1c43f0fcaaf6845441", - "rustfmt-1.92.0-aarch64-pc-windows-gnullvm.tar.xz": "272d3af11e41ebdcc605ff8b6163f20b55526ed87e812fd1e0d001141a0c9a8a", - "rustfmt-1.92.0-aarch64-unknown-linux-gnu.tar.xz": "1dce37aea2a7cb801f1756ffc531d7140428315a3d2c2f836272547eb7b9dacd", - "rustfmt-1.92.0-x86_64-apple-darwin.tar.xz": "e038bda323ed7f4d417efc5be44c4245d74b8394f9f8393b9d464d662c3a9499", - "rustfmt-1.92.0-x86_64-pc-windows-gnullvm.tar.xz": "c03f55deeaaffb9520426d573e42ece889e760788012bdc97d2d27146033b0be", - "rustfmt-1.92.0-x86_64-unknown-linux-gnu.tar.xz": "38951ee55f21e9170236fc98c8ba373ae4338d863087c6b0a5fa8c4e797d52c4" + "cargo-1.93.0-aarch64-apple-darwin.tar.xz": "6443909350322ad07f09bb5edfd9ff29268e6fe88c7d78bfba7a5e254248dc25", + "cargo-1.93.0-aarch64-pc-windows-gnullvm.tar.xz": "387832b989c8eb96c9ebd66402a87962167633bc6d91a49ffb8c7903c45f3476", + "cargo-1.93.0-aarch64-unknown-linux-gnu.tar.xz": "5998940b8b97286bb67facb1a85535eeb3d4d7a61e36a85e386e5c0c5cfe5266", + "cargo-1.93.0-x86_64-apple-darwin.tar.xz": "95a47c5ed797c35419908f04188d8b7de09946e71073c4b72632b16f5b10dfae", + "cargo-1.93.0-x86_64-pc-windows-gnullvm.tar.xz": "f19766837559f90476508140cb95cc708220012ec00a854fa9f99187b1f246b6", + "cargo-1.93.0-x86_64-unknown-linux-gnu.tar.xz": "c23de3ae709ff33eed5e4ae59d1f9bcd75fa4dbaa9fb92f7b06bfb534b8db880", + "clippy-1.93.0-aarch64-apple-darwin.tar.xz": "0b6e943a8d12be0e68575acf59c9ea102daf795055fcbbf862b0bfd35ec40039", + "clippy-1.93.0-aarch64-pc-windows-gnullvm.tar.xz": "296949f49be2de77ce9d1c5023a5f0a58e28e329eec03642a3da0e175a67beeb", + "clippy-1.93.0-aarch64-unknown-linux-gnu.tar.xz": "872ae6d68d625946d281b91d928332e6b74f6ab269b6af842338df4338805a60", + "clippy-1.93.0-x86_64-apple-darwin.tar.xz": "e6d0b1afb9607c14a1172d09ee194a032bbb3e48af913d55c5a473e0559eddde", + "clippy-1.93.0-x86_64-pc-windows-gnullvm.tar.xz": "b6f1f7264ed6943c59dedfb9531fbadcc3c0fcf273c940a63d58898b14a1060f", + "clippy-1.93.0-x86_64-unknown-linux-gnu.tar.xz": "793108977514b15c0f45ade28ae35c58b05370cb0f22e89bd98fdfa61eabf55d", + "rust-std-1.93.0-aarch64-apple-darwin.tar.xz": "8603c63715349636ed85b4fe716c4e827a727918c840e54aff5b243cedadf19b", + "rust-std-1.93.0-aarch64-apple-ios-macabi.tar.xz": "24d47e615ce101869ff452a572a6b77ed7cf70f2454d0b50892ac849e8c7ac4d", + "rust-std-1.93.0-aarch64-apple-ios-sim.tar.xz": "d1d5e2d1b79206f2cc9fb7f6a2958cfe0f4bbc9147fda8dbc3608aa4be5e6816", + "rust-std-1.93.0-aarch64-apple-ios.tar.xz": "49228e70387773a71cf144509baf39979ab2cdb604340fff64b483ab41f61617", + "rust-std-1.93.0-aarch64-linux-android.tar.xz": "59c16648d9a29c07f63a1749cae6b73078f20fef1206c5e0f81c784ae8796cdb", + "rust-std-1.93.0-aarch64-pc-windows-gnullvm.tar.xz": "9a270d50eaaacc7cb1925565a8b65ff831686aa1592b7034bb9848d7f2a9738d", + "rust-std-1.93.0-aarch64-pc-windows-msvc.tar.xz": "f7bd3d25baf3643c8769b8c4d2e6cde45bb25042fac698e0daf19fc9f58f8568", + "rust-std-1.93.0-aarch64-unknown-fuchsia.tar.xz": "d1e46c443a9607603c810942e99a95a1acfb105d1312426b468ff68febaabf77", + "rust-std-1.93.0-aarch64-unknown-linux-gnu.tar.xz": "84e82ff52c39c64dfd0e1c2d58fd3d5309d1d2502378131544c0d486b44af20a", + "rust-std-1.93.0-aarch64-unknown-linux-musl.tar.xz": "bab885a87da586040064064bd1c314d707164d8dc0fefee39d59be7f15ce6f7d", + "rust-std-1.93.0-aarch64-unknown-none-softfloat.tar.xz": "0f6305daf658a7d6c0efd075859cb60432c13b82e7ecee0d097074e4e1873391", + "rust-std-1.93.0-aarch64-unknown-none.tar.xz": "3cf1aa3309a8903e89bb20537113155ca4e88844c8cc9c34c43865d5ce5a6192", + "rust-std-1.93.0-aarch64-unknown-uefi.tar.xz": "317b0af124e0e124bd76b8e5a2fb0c600279177d0bed9c841a3202df2d0f7f8e", + "rust-std-1.93.0-arm-linux-androideabi.tar.xz": "d010b26fc88e28a93cc94ea6ca5d2c90efed7f9846fae1e40db7c183b50575e2", + "rust-std-1.93.0-arm-unknown-linux-gnueabi.tar.xz": "deedc54ffce099781986eed4aec0843866f1bf72904ab0f5cdb115b9c7af540e", + "rust-std-1.93.0-arm-unknown-linux-gnueabihf.tar.xz": "89e44e042bc1241b3999191c385fec8433d60a5a9fc373583cd3b2d9408d5074", + "rust-std-1.93.0-arm-unknown-linux-musleabi.tar.xz": "641a17acb5104637d4dc9c4be022a7927ae721eb08759fea96ecfaf5c60be4dc", + "rust-std-1.93.0-arm-unknown-linux-musleabihf.tar.xz": "94a92b454bf3b0aab046b257f555ccb08f16dc2dc281bea6a4ef17ea8f58cbdc", + "rust-std-1.93.0-armv7-linux-androideabi.tar.xz": "e295f26bb219a7a4ebb5c2e07fedfebb075be6830aaf910c742a57cd21018b6d", + "rust-std-1.93.0-armv7-unknown-linux-gnueabi.tar.xz": "8a7bd5227c78294864095edb07837ff32ff6c07cd1a4a418f9bcc3ebd7e79874", + "rust-std-1.93.0-armv7-unknown-linux-gnueabihf.tar.xz": "f015f9b2d588454a9dc62942ab2e800d82c602e4eab6f89f8213419491bcd203", + "rust-std-1.93.0-armv7-unknown-linux-musleabi.tar.xz": "d15d24c9fb7c15243e1341cea53590002df271060118914bd0efcda8ccbd0731", + "rust-std-1.93.0-armv7-unknown-linux-musleabihf.tar.xz": "a2e5ec22ed35fb51a503d1e10b37447b0fa7333f079585bc0b6a2eb599de43f3", + "rust-std-1.93.0-i686-linux-android.tar.xz": "68fd86f62dd63221717d1741210f0f5cf75da7a1e32eed5a46b1e67c9d9430e1", + "rust-std-1.93.0-i686-pc-windows-gnu.tar.xz": "cb613d5d1eb245e8a1f4c0b25f93c2997cd06c1cc3fc202155f2997aebf44d4d", + "rust-std-1.93.0-i686-pc-windows-gnullvm.tar.xz": "0f713dc252a6de706519fe6cdaab6d66aaf1b555133b536cc0ab28061aa4269c", + "rust-std-1.93.0-i686-pc-windows-msvc.tar.xz": "33dc1951e2dc21bd05361160d52f496eecf48e0b95df5083172698b1cd5b9a3f", + "rust-std-1.93.0-i686-unknown-freebsd.tar.xz": "67718aae1381879fdcca5699051eb87e0cda3d2fd0fe75e306ba0948b79df7db", + "rust-std-1.93.0-i686-unknown-linux-gnu.tar.xz": "b8b7020a61418b95c1ea26badaf8db6979778e28dbadddc81fb5010fe27c935b", + "rust-std-1.93.0-i686-unknown-linux-musl.tar.xz": "867e54b3e89dc0b6d2b7a538a80443add6c3990bb4bd2260dea2ed98a0dc9870", + "rust-std-1.93.0-i686-unknown-uefi.tar.xz": "929fd484b08d5b2077ff864f5f2d24b51a78f1b6e837b9eab9d7e8fb7f31adce", + "rust-std-1.93.0-powerpc-unknown-linux-gnu.tar.xz": "e851c0fa3e726ce3f7139c5803198a1aa9723594394734ac9e373c93d92e5ea3", + "rust-std-1.93.0-powerpc64-unknown-linux-gnu.tar.xz": "f729bb7d95705e12a92eb072e494b93d8822ca40aa4802ca780b0cf33b56d401", + "rust-std-1.93.0-powerpc64le-unknown-linux-gnu.tar.xz": "d209ac698a69ca9b9035adb97a0ed8e60a08db52960198c3e03b9ee714c1a46b", + "rust-std-1.93.0-powerpc64le-unknown-linux-musl.tar.xz": "34b98d5eca2fdbd6ba41b0faf14160ef1ebd038f6ecaa264d318ad33263e1cf1", + "rust-std-1.93.0-riscv32imc-unknown-none-elf.tar.xz": "71af84c81241cbc7811b267927990be025f30d7d3dc55df4b56da7ac250f7c78", + "rust-std-1.93.0-riscv64gc-unknown-linux-gnu.tar.xz": "b769fb6c9f3e0419a6bd0b7b79f9191bbd7a48a9f243b23eb7d135711aa6de1b", + "rust-std-1.93.0-riscv64gc-unknown-linux-musl.tar.xz": "a7ced602573d814d875d69022e026c1ccb520b4b2de9d430ddfd0966ec6c9643", + "rust-std-1.93.0-riscv64gc-unknown-none-elf.tar.xz": "842f72913f288a0c76601438e67ccd88c816dbf187587928e48bf8b9ce74cbf3", + "rust-std-1.93.0-s390x-unknown-linux-gnu.tar.xz": "41a65db45a288eb3eedb187b366f132d5b3615767de7ce994b123b342ac7a848", + "rust-std-1.93.0-thumbv6m-none-eabi.tar.xz": "be3f8aad5680dabb203300847dcbbabc15729170ba5c3a9c499efae4df410a9e", + "rust-std-1.93.0-thumbv7em-none-eabi.tar.xz": "8f93eefca39c0da417feddab64775f862c72bbe80da11914dcf47babef8f1644", + "rust-std-1.93.0-thumbv7em-none-eabihf.tar.xz": "a3b6914b966ac93dbe7531016d5f570b189445603c43614a60e0b9ea12674bd3", + "rust-std-1.93.0-thumbv7m-none-eabi.tar.xz": "cfa6227214f3ae58c06b36547c5bd6f0f6787764afa48cfa4ff3488264deab6c", + "rust-std-1.93.0-thumbv8m.main-none-eabi.tar.xz": "5f24df0aa8322561125575e365be7ad13a5bb26cf73c7fc9a3f4bcfa58e0febc", + "rust-std-1.93.0-thumbv8m.main-none-eabihf.tar.xz": "906b07580be2df277cced2b56bc03cb565b758c382bf3e82cbd8375b459815dd", + "rust-std-1.93.0-wasm32-unknown-emscripten.tar.xz": "63cdbb1ea7f353060539c00f7346f4f5fb0d6f09899cacddc1f172ef07c4af8b", + "rust-std-1.93.0-wasm32-unknown-unknown.tar.xz": "3100cb920ddac646943243f0eddd331128836b9161dd5f7b0a6c76375d39cc5e", + "rust-std-1.93.0-wasm32-wasip1-threads.tar.xz": "439c65dea31e855f0258632b6d19435ba8a80561297fa6dc6be48048c5cd1871", + "rust-std-1.93.0-wasm32-wasip1.tar.xz": "075de970ef865678dad258f1566d7cfe76a594698e9bf93dd69fa5cfdfcf1a6f", + "rust-std-1.93.0-wasm32-wasip2.tar.xz": "0ef01bb552036ab44456f5505015b13c88d3694629804d7af46452c8b0a48f8c", + "rust-std-1.93.0-x86_64-apple-darwin.tar.xz": "f112d41c8a31794f0f561d37fe77010ed0b405fa70284a2910891869d8c52418", + "rust-std-1.93.0-x86_64-apple-ios-macabi.tar.xz": "a543dd545747d372d973ace8b485a13603ce96c110c7ae734d605e45f6e162c5", + "rust-std-1.93.0-x86_64-apple-ios.tar.xz": "e151013b9bc5990e178285a33e62bae7700d8c48c06e97771abb1643aa907d75", + "rust-std-1.93.0-x86_64-linux-android.tar.xz": "dc05ca79d9fecc5ce3643adb9c6f89fd35c8e1d7146bf9b62e30bad41f9fb6a7", + "rust-std-1.93.0-x86_64-pc-windows-gnu.tar.xz": "a07c6ab596fad15ca7acd63ee7f2a5fea93fd421179252067e309c2aa0b2021b", + "rust-std-1.93.0-x86_64-pc-windows-gnullvm.tar.xz": "ef6cf0977bc5aa4bbd594afb9df4ba76fdd4f0fc5685cddbefff49ceed202a91", + "rust-std-1.93.0-x86_64-pc-windows-msvc.tar.xz": "2593e29af0b8def34ceb1185b8e85bd93a9e0d3b0c108d704c1b31370c50a48c", + "rust-std-1.93.0-x86_64-unknown-freebsd.tar.xz": "51b2feaff7c2d28633504ed92ab442a55d112e6a2bf09c91188f00dbaf03378a", + "rust-std-1.93.0-x86_64-unknown-fuchsia.tar.xz": "41f0f3eb96cedfc13ab5fd4f15065063f262d035c1f71d96c42587acdacdbabe", + "rust-std-1.93.0-x86_64-unknown-linux-gnu.tar.xz": "a849a418d0f27e69573e41763c395e924a0b98c16fcdc55599c1c79c27c1c777", + "rust-std-1.93.0-x86_64-unknown-linux-musl.tar.xz": "874658d2ced1ed2b9bf66c148b78a2e10cad475d0a4db32e68a08900905b89b8", + "rust-std-1.93.0-x86_64-unknown-netbsd.tar.xz": "aad63193af89772031f9a85f193afc0b15f8e6d4a9a4983c5f5d3802f69a89e8", + "rust-std-1.93.0-x86_64-unknown-none.tar.xz": "01dcca7ae4b7e82fbfa399adb5e160afaa13143e5a17e1e0737c38cf07365fb3", + "rust-std-1.93.0-x86_64-unknown-uefi.tar.xz": "ec4e439d9485ce752b56999e8e41ed82373fc833a005cf2531c6f7ef7e785392", + "rustc-1.93.0-aarch64-apple-darwin.tar.xz": "092be03c02b44c405dab1232541c84f32b2d9e8295747568c3d531dd137221dc", + "rustc-1.93.0-aarch64-pc-windows-gnullvm.tar.xz": "d3bc0cdaf157e20b1f23e510b5e3c4c6e9117d08f5284c04dee60aecff1bc851", + "rustc-1.93.0-aarch64-unknown-linux-gnu.tar.xz": "1a9045695892ec08d8e9751bf7cf7db71fe27a6202dd12ce13aca48d0602dbde", + "rustc-1.93.0-x86_64-apple-darwin.tar.xz": "594bb293f0a4f444656cf8dec2149fcb979c606260efee9e09bcf8c9c6ed6ae7", + "rustc-1.93.0-x86_64-pc-windows-gnullvm.tar.xz": "0cdaa8de66f5ce21d1ea73917efc5c64f408bda49f678ddde19465ced9d5ec63", + "rustc-1.93.0-x86_64-unknown-linux-gnu.tar.xz": "00c6e6740ea6a795e33568cd7514855d58408a1180cd820284a7bbf7c46af715", + "rustfmt-1.93.0-aarch64-apple-darwin.tar.xz": "0dd1faedf0768ef362f4aae4424b34e8266f2b9cf5e76ea4fcaf780220b363a0", + "rustfmt-1.93.0-aarch64-pc-windows-gnullvm.tar.xz": "5888827e7fbd7d59930870b4856fce8d6d8fca5e02f6535f8ae3d7ad0ccf2d4a", + "rustfmt-1.93.0-aarch64-unknown-linux-gnu.tar.xz": "92e1acb45ae642136258b4dabb39302af2d53c83e56ebd5858bc969f9e5c141a", + "rustfmt-1.93.0-x86_64-apple-darwin.tar.xz": "c8453b4c5758eb39423042ffa9c23ed6128cbed2b15b581e5e1192c9cc0b1d4e", + "rustfmt-1.93.0-x86_64-pc-windows-gnullvm.tar.xz": "47167e9e78db9be4503a060dee02f4df2cda252da32175dbf44331f965a747b9", + "rustfmt-1.93.0-x86_64-unknown-linux-gnu.tar.xz": "7f81f6c17d11a7fda5b4e1b111942fb3b23d30dcec767e13e340ebfb762a5e33" } } } diff --git a/defs.bzl b/defs.bzl index c569686990..5800d996aa 100644 --- a/defs.bzl +++ b/defs.bzl @@ -85,7 +85,6 @@ def codex_rust_crate( proc_macro_dev_deps = all_crate_deps(proc_macro_dev = True) test_env = { - "INSTA_REQUIRE_FULL_MATCH": "0", "INSTA_WORKSPACE_ROOT": ".", "INSTA_SNAPSHOT_PATH": "src", } diff --git a/patches/toolchains_llvm_private_frameworks.patch b/patches/toolchains_llvm_private_frameworks.patch deleted file mode 100644 index 86931b1f58..0000000000 --- a/patches/toolchains_llvm_private_frameworks.patch +++ /dev/null @@ -1,12 +0,0 @@ -diff --git a/extensions/osx.bzl b/extensions/osx.bzl -index 1e25dc0..8f3e2f9 100644 ---- a/extensions/osx.bzl -+++ b/extensions/osx.bzl -@@ -41,6 +41,7 @@ def _osx_extension_impl(mctx): - - for framework in frameworks: - includes.append("System/Library/Frameworks/%s.framework/*" % framework) -+ includes.append("System/Library/PrivateFrameworks/%s.framework/*" % framework) - - # The following directories are unused, deprecated, or private headers. - # These components: From c2c6bc90f8c033460f1bea8a7a55082b9649cbf6 Mon Sep 17 00:00:00 2001 From: viyatb-oai Date: Mon, 9 Feb 2026 12:13:55 -0800 Subject: [PATCH 03/23] chore: remove network-proxy-cli crate (#11158) ## Summary - remove `network-proxy-cli` from the Rust workspace members - delete the dedicated `codex-network-proxy-cli` crate files - remove the stale `codex-network-proxy-cli` package entry from `Cargo.lock` ## Testing - just fmt - cargo test -p codex-network-proxy --- codex-rs/Cargo.lock | 12 ------------ codex-rs/Cargo.toml | 1 - codex-rs/network-proxy-cli/Cargo.toml | 17 ----------------- codex-rs/network-proxy-cli/src/main.rs | 19 ------------------- 4 files changed, 49 deletions(-) delete mode 100644 codex-rs/network-proxy-cli/Cargo.toml delete mode 100644 codex-rs/network-proxy-cli/src/main.rs diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index e6e9cc22cd..1b7c3cbbed 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -1928,18 +1928,6 @@ dependencies = [ "url", ] -[[package]] -name = "codex-network-proxy-cli" -version = "0.0.0" -dependencies = [ - "anyhow", - "clap", - "codex-core", - "codex-network-proxy", - "tokio", - "tracing-subscriber", -] - [[package]] name = "codex-ollama" version = "0.0.0" diff --git a/codex-rs/Cargo.toml b/codex-rs/Cargo.toml index 6f9a2b5ddb..44a4c9a34e 100644 --- a/codex-rs/Cargo.toml +++ b/codex-rs/Cargo.toml @@ -29,7 +29,6 @@ members = [ "login", "mcp-server", "network-proxy", - "network-proxy-cli", "ollama", "process-hardening", "protocol", diff --git a/codex-rs/network-proxy-cli/Cargo.toml b/codex-rs/network-proxy-cli/Cargo.toml deleted file mode 100644 index 9cc05372d2..0000000000 --- a/codex-rs/network-proxy-cli/Cargo.toml +++ /dev/null @@ -1,17 +0,0 @@ -[package] -name = "codex-network-proxy-cli" -edition = "2024" -version = { workspace = true } -license.workspace = true - -[[bin]] -name = "codex-network-proxy" -path = "src/main.rs" - -[dependencies] -anyhow = { workspace = true } -clap = { workspace = true, features = ["derive"] } -codex-core = { workspace = true } -codex-network-proxy = { path = "../network-proxy" } -tokio = { workspace = true, features = ["full"] } -tracing-subscriber = { workspace = true, features = ["fmt"] } diff --git a/codex-rs/network-proxy-cli/src/main.rs b/codex-rs/network-proxy-cli/src/main.rs deleted file mode 100644 index c74e7a2268..0000000000 --- a/codex-rs/network-proxy-cli/src/main.rs +++ /dev/null @@ -1,19 +0,0 @@ -use anyhow::Result; -use clap::Parser; -use codex_core::network_proxy_loader; -use codex_network_proxy::Args; -use codex_network_proxy::NetworkProxy; -use codex_network_proxy::NetworkProxyState; -use std::sync::Arc; - -#[tokio::main] -async fn main() -> Result<()> { - tracing_subscriber::fmt::init(); - - let args = Args::parse(); - let (state, reloader) = network_proxy_loader::build_network_proxy_state_and_reloader().await?; - let state = Arc::new(NetworkProxyState::with_reloader(state, Arc::new(reloader))); - let _ = args; - let proxy = NetworkProxy::builder().state(state).build().await?; - proxy.run().await?.wait().await -} From c2bfd1e473377e158cbdcb234dd62d2fc3ecf7ce Mon Sep 17 00:00:00 2001 From: jif-oai Date: Mon, 9 Feb 2026 20:22:38 +0000 Subject: [PATCH 04/23] Revert "chore: enable sub agents" (#11230) Reverts openai/codex#11173 --- codex-rs/core/src/agent/role.rs | 1 + codex-rs/core/src/features.rs | 16 +++--- codex-rs/core/src/tools/spec.rs | 55 --------------------- codex-rs/core/tests/suite/model_tools.rs | 32 +----------- codex-rs/core/tests/suite/prompt_caching.rs | 5 -- 5 files changed, 12 insertions(+), 97 deletions(-) diff --git a/codex-rs/core/src/agent/role.rs b/codex-rs/core/src/agent/role.rs index 234d679948..74eca4179d 100644 --- a/codex-rs/core/src/agent/role.rs +++ b/codex-rs/core/src/agent/role.rs @@ -95,6 +95,7 @@ Rules: reasoning_effort: Some(ReasoningEffort::Medium), description: r#"Use `explorer` for all codebase questions. Explorers are fast and authoritative. +Always prefer them over manual search or file reading. Rules: - Ask explorers first and precisely. - Do not re-read or re-search code they cover. diff --git a/codex-rs/core/src/features.rs b/codex-rs/core/src/features.rs index 201430c9bd..7de37e78ad 100644 --- a/codex-rs/core/src/features.rs +++ b/codex-rs/core/src/features.rs @@ -432,12 +432,6 @@ pub const FEATURES: &[FeatureSpec] = &[ stage: Stage::Deprecated, default_enabled: false, }, - FeatureSpec { - id: Feature::Collab, - key: "collab", - stage: Stage::Stable, - default_enabled: true, - }, // Experimental program. Rendered in the `/experimental` menu for users. FeatureSpec { id: Feature::RuntimeMetrics, @@ -517,6 +511,16 @@ pub const FEATURES: &[FeatureSpec] = &[ stage: Stage::Stable, default_enabled: true, }, + FeatureSpec { + id: Feature::Collab, + key: "collab", + stage: Stage::Experimental { + name: "Sub-agents", + menu_description: "Ask Codex to spawn multiple agents to parallelize the work and win in efficiency.", + announcement: "NEW: Sub-agents can now be spawned by Codex. Enable in /experimental and restart Codex!", + }, + default_enabled: false, + }, FeatureSpec { id: Feature::Apps, key: "apps", diff --git a/codex-rs/core/src/tools/spec.rs b/codex-rs/core/src/tools/spec.rs index 4844828886..729b108888 100644 --- a/codex-rs/core/src/tools/spec.rs +++ b/codex-rs/core/src/tools/spec.rs @@ -1634,11 +1634,6 @@ mod tests { external_web_access: Some(true), }, create_view_image_tool(), - create_spawn_agent_tool(), - create_send_input_tool(), - create_resume_agent_tool(), - create_wait_tool(), - create_close_agent_tool(), ] { expected.insert(tool_name(&spec).to_string(), spec); } @@ -1806,11 +1801,6 @@ mod tests { "apply_patch", "web_search", "view_image", - "spawn_agent", - "send_input", - "resume_agent", - "wait", - "close_agent", ], ); } @@ -1833,11 +1823,6 @@ mod tests { "apply_patch", "web_search", "view_image", - "spawn_agent", - "send_input", - "resume_agent", - "wait", - "close_agent", ], ); } @@ -1862,11 +1847,6 @@ mod tests { "apply_patch", "web_search", "view_image", - "spawn_agent", - "send_input", - "resume_agent", - "wait", - "close_agent", ], ); } @@ -1891,11 +1871,6 @@ mod tests { "apply_patch", "web_search", "view_image", - "spawn_agent", - "send_input", - "resume_agent", - "wait", - "close_agent", ], ); } @@ -1917,11 +1892,6 @@ mod tests { "request_user_input", "web_search", "view_image", - "spawn_agent", - "send_input", - "resume_agent", - "wait", - "close_agent", ], ); } @@ -1944,11 +1914,6 @@ mod tests { "apply_patch", "web_search", "view_image", - "spawn_agent", - "send_input", - "resume_agent", - "wait", - "close_agent", ], ); } @@ -1970,11 +1935,6 @@ mod tests { "request_user_input", "web_search", "view_image", - "spawn_agent", - "send_input", - "resume_agent", - "wait", - "close_agent", ], ); } @@ -1997,11 +1957,6 @@ mod tests { "apply_patch", "web_search", "view_image", - "spawn_agent", - "send_input", - "resume_agent", - "wait", - "close_agent", ], ); } @@ -2048,11 +2003,6 @@ mod tests { "apply_patch", "web_search", "view_image", - "spawn_agent", - "send_input", - "resume_agent", - "wait", - "close_agent", ], ); } @@ -2076,11 +2026,6 @@ mod tests { "request_user_input", "web_search", "view_image", - "spawn_agent", - "send_input", - "resume_agent", - "wait", - "close_agent", ], ); } diff --git a/codex-rs/core/tests/suite/model_tools.rs b/codex-rs/core/tests/suite/model_tools.rs index 3143f5d496..ae59146135 100644 --- a/codex-rs/core/tests/suite/model_tools.rs +++ b/codex-rs/core/tests/suite/model_tools.rs @@ -81,11 +81,6 @@ async fn model_selects_expected_tools() { "request_user_input", "web_search", "view_image", - "spawn_agent", - "send_input", - "resume_agent", - "wait", - "close_agent", ], ), "codex-mini-latest should expose the local shell tool", @@ -105,11 +100,6 @@ async fn model_selects_expected_tools() { "apply_patch", "web_search", "view_image", - "spawn_agent", - "send_input", - "resume_agent", - "wait", - "close_agent", ], ), "gpt-5-codex should expose the apply_patch tool", @@ -129,11 +119,6 @@ async fn model_selects_expected_tools() { "apply_patch", "web_search", "view_image", - "spawn_agent", - "send_input", - "resume_agent", - "wait", - "close_agent", ], ), "gpt-5.1-codex should expose the apply_patch tool", @@ -152,11 +137,6 @@ async fn model_selects_expected_tools() { "request_user_input", "web_search", "view_image", - "spawn_agent", - "send_input", - "resume_agent", - "wait", - "close_agent", ], ), "gpt-5 should expose the apply_patch tool", @@ -176,11 +156,6 @@ async fn model_selects_expected_tools() { "apply_patch", "web_search", "view_image", - "spawn_agent", - "send_input", - "resume_agent", - "wait", - "close_agent", ], ), "gpt-5.1 should expose the apply_patch tool", @@ -198,12 +173,7 @@ async fn model_selects_expected_tools() { "request_user_input".to_string(), "apply_patch".to_string(), "web_search".to_string(), - "view_image".to_string(), - "spawn_agent".to_string(), - "send_input".to_string(), - "resume_agent".to_string(), - "wait".to_string(), - "close_agent".to_string() + "view_image".to_string() ], "exp-5.1 should expose the apply_patch tool", ); diff --git a/codex-rs/core/tests/suite/prompt_caching.rs b/codex-rs/core/tests/suite/prompt_caching.rs index d1001c8dfd..242f931406 100644 --- a/codex-rs/core/tests/suite/prompt_caching.rs +++ b/codex-rs/core/tests/suite/prompt_caching.rs @@ -153,11 +153,6 @@ async fn prompt_tools_are_consistent_across_requests() -> anyhow::Result<()> { "apply_patch", "web_search", "view_image", - "spawn_agent", - "send_input", - "resume_agent", - "wait", - "close_agent", ]); let body0 = req1.single_request().body_json(); From f88667042e0b9197bcdeb726feac58308e51f4ad Mon Sep 17 00:00:00 2001 From: Charley Cunningham Date: Mon, 9 Feb 2026 12:23:31 -0800 Subject: [PATCH 05/23] TUI: fix request_user_input wrapping for long option labels (#11123) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary This PR fixes long-text rendering in the `request_user_input` TUI overlay while preserving a clear two-column option layout. (Issue https://github.com/openai/codex/issues/11093) Before: - very long option labels could push description text into a narrow right-edge strip - option labels were effectively single-line when descriptions were present, causing truncation/poor readability - label and description wrapping interacted in one combined wrapped line Screenshot 2026-02-08 at 2 27 25 PM After: - option labels wrap inside the left column - descriptions wrap independently inside the right column - row measurement and row rendering use the same wrapping path, so layout stays stable Screenshot 2026-02-09 at 10 28 02 AM ## Problem `request_user_input` needs to handle verbose prompts/options. With oversized labels: - descriptions could collapse into a thin, hard-to-read column - important label context was lost ## Root Cause In shared row rendering (`selection_popup_common`): - rows were wrapped as a single combined line - auto column sizing could still place `desc_col` too far right for long labels - `request_user_input` rows did not provide wrap metadata to align continuation lines after the option prefix ## What Changed ### 1) `request_user_input` rows opt into wrapped labels File: `codex-rs/tui/src/bottom_pane/request_user_input/mod.rs` - In `option_rows()`, compute the rendered option prefix (`› 1. ` / ` 2. `) and set `wrap_indent` from its display width. - Apply the same behavior to the synthetic “None of the above” row. - Add long-text snapshot test coverage (`question_with_very_long_option_text` + `request_user_input_long_option_text_snapshot`). ### 2) Shared renderer now has an opt-in two-column wrapping path File: `codex-rs/tui/src/bottom_pane/selection_popup_common.rs` - Add focused helpers: - `should_wrap_name_in_column` - `wrap_two_column_row` - `wrap_standard_row` - `wrap_row_lines` - `apply_row_state_style` - For opted-in rows (plain option rows with `wrap_indent` + description), wrap label and description independently in their own columns. - Keep the legacy standard wrapping path for non-opted rows. - Use the same `wrap_row_lines` function in both rendering and height measurement to keep them in sync. ### 3) Keep column sizing simple and derived from existing fixed split constants File: `codex-rs/tui/src/bottom_pane/selection_popup_common.rs` - Keep fixed mode at `3/10` left column (`30/70` split). - In auto modes, cap label width using those same fixed constants (max 70% label, min 30% description), instead of extra special-case constants/branches. - Add/keep narrow-width safety guard in `wrap_two_column_row` so extremely small widths do not panic. ### 4) Snapshot coverage File: `codex-rs/tui/src/bottom_pane/request_user_input/snapshots/ codex_tui__bottom_pane__request_user_input__tests__request_user_input_long_option_text.snap` - Add snapshot for long-label/long-description two-column rendering behavior. --- .../src/bottom_pane/list_selection_view.rs | 2 +- .../src/bottom_pane/request_user_input/mod.rs | 104 ++++- .../bottom_pane/request_user_input/render.rs | 44 +- ...__request_user_input_long_option_text.snap | 17 + .../src/bottom_pane/selection_popup_common.rs | 379 ++++++++++++++---- 5 files changed, 473 insertions(+), 73 deletions(-) create mode 100644 codex-rs/tui/src/bottom_pane/request_user_input/snapshots/codex_tui__bottom_pane__request_user_input__tests__request_user_input_long_option_text.snap diff --git a/codex-rs/tui/src/bottom_pane/list_selection_view.rs b/codex-rs/tui/src/bottom_pane/list_selection_view.rs index 8f20f2bc64..00baf9c56c 100644 --- a/codex-rs/tui/src/bottom_pane/list_selection_view.rs +++ b/codex-rs/tui/src/bottom_pane/list_selection_view.rs @@ -631,7 +631,7 @@ impl Renderable for ListSelectionView { "no matches", ColumnWidthMode::Fixed, ), - } + }; } if footer_area.height > 0 { diff --git a/codex-rs/tui/src/bottom_pane/request_user_input/mod.rs b/codex-rs/tui/src/bottom_pane/request_user_input/mod.rs index a00aedb0f1..c2a4291cf1 100644 --- a/codex-rs/tui/src/bottom_pane/request_user_input/mod.rs +++ b/codex-rs/tui/src/bottom_pane/request_user_input/mod.rs @@ -280,9 +280,12 @@ impl RequestUserInputOverlay { let prefix = if selected { '›' } else { ' ' }; let label = opt.label.as_str(); let number = idx + 1; + let prefix_label = format!("{prefix} {number}. "); + let wrap_indent = UnicodeWidthStr::width(prefix_label.as_str()); GenericDisplayRow { - name: format!("{prefix} {number}. {label}"), + name: format!("{prefix_label}{label}"), description: Some(opt.description.clone()), + wrap_indent: Some(wrap_indent), ..Default::default() } }) @@ -293,9 +296,12 @@ impl RequestUserInputOverlay { let selected = selected_idx.is_some_and(|sel| sel == idx); let prefix = if selected { '›' } else { ' ' }; let number = idx + 1; + let prefix_label = format!("{prefix} {number}. "); + let wrap_indent = UnicodeWidthStr::width(prefix_label.as_str()); rows.push(GenericDisplayRow { - name: format!("{prefix} {number}. {OTHER_OPTION_LABEL}"), + name: format!("{prefix_label}{OTHER_OPTION_LABEL}"), description: Some(OTHER_OPTION_DESCRIPTION.to_string()), + wrap_indent: Some(wrap_indent), ..Default::default() }); } @@ -1382,6 +1388,57 @@ mod tests { } } + fn question_with_very_long_option_text(id: &str, header: &str) -> RequestUserInputQuestion { + RequestUserInputQuestion { + id: id.to_string(), + header: header.to_string(), + question: "Choose one option.".to_string(), + is_other: false, + is_secret: false, + options: Some(vec![ + RequestUserInputQuestionOption { + label: "Job: running/completed/failed/expired; Run/Experiment: succeeded/failed/unknown (Recommended when triaging long-running background work and status transitions)".to_string(), + description: "Keep async job statuses for progress tracking and include enough context for debugging retries, stale workers, and unexpected expiration paths.".to_string(), + }, + RequestUserInputQuestionOption { + label: "Add a short status model".to_string(), + description: "Simpler labels with less detail for quick rollouts.".to_string(), + }, + ]), + } + } + + fn question_with_long_scroll_options(id: &str, header: &str) -> RequestUserInputQuestion { + RequestUserInputQuestion { + id: id.to_string(), + header: header.to_string(), + question: + "Choose one option; each hint is intentionally very long to test wrapped scrolling." + .to_string(), + is_other: false, + is_secret: false, + options: Some(vec![ + RequestUserInputQuestionOption { + label: "Use Detailed Hint A (Recommended)".to_string(), + description: "Select this if you want a deliberately overextended explanatory hint that reads like a miniature specification, including context, rationale, expected behavior, and an explicit statement that this choice is mainly for testing how gracefully the interface wraps, truncates, and preserves readability under unusually verbose helper text conditions.".to_string(), + }, + RequestUserInputQuestionOption { + label: "Use Detailed Hint B".to_string(), + description: "Select this if you want an equally verbose but differently phrased guidance block that emphasizes user-facing clarity, spacing tolerance, multiline wrapping, visual hierarchy interactions, and whether long descriptive metadata remains understandable when scanned quickly in a constrained layout where cognitive load is already high.".to_string(), + }, + RequestUserInputQuestionOption { + label: "Use Detailed Hint C".to_string(), + description: "Select this when you specifically want to verify that navigating downward will keep the currently highlighted option visible, even when previous options consume many wrapped lines and would otherwise push the selection out of the viewport.".to_string(), + }, + RequestUserInputQuestionOption { + label: "None of the above".to_string(), + description: + "Use this only if the previous long-form options do not apply.".to_string(), + }, + ]), + } + } + fn question_without_options(id: &str, header: &str) -> RequestUserInputQuestion { RequestUserInputQuestion { id: id.to_string(), @@ -2575,6 +2632,49 @@ mod tests { ); } + #[test] + fn request_user_input_long_option_text_snapshot() { + let (tx, _rx) = test_sender(); + let overlay = RequestUserInputOverlay::new( + request_event( + "turn-1", + vec![question_with_very_long_option_text("q1", "Status")], + ), + tx, + true, + false, + false, + ); + let area = Rect::new(0, 0, 120, 18); + insta::assert_snapshot!( + "request_user_input_long_option_text", + render_snapshot(&overlay, area) + ); + } + + #[test] + fn selected_long_wrapped_option_stays_visible() { + let (tx, _rx) = test_sender(); + let mut overlay = RequestUserInputOverlay::new( + request_event( + "turn-1", + vec![question_with_long_scroll_options("q1", "Scroll")], + ), + tx, + true, + false, + false, + ); + let answer = overlay.current_answer_mut().expect("answer missing"); + answer.options_state.selected_idx = Some(2); + + let rendered = render_snapshot(&overlay, Rect::new(0, 0, 80, 20)); + assert!( + rendered.contains("› 3. Use Detailed Hint C"), + "expected selected option to be visible in viewport\n{rendered}" + ); + } + #[test] fn request_user_input_footer_wrap_snapshot() { let (tx, _rx) = test_sender(); diff --git a/codex-rs/tui/src/bottom_pane/request_user_input/render.rs b/codex-rs/tui/src/bottom_pane/request_user_input/render.rs index 2180ec0553..eeda763579 100644 --- a/codex-rs/tui/src/bottom_pane/request_user_input/render.rs +++ b/codex-rs/tui/src/bottom_pane/request_user_input/render.rs @@ -316,7 +316,7 @@ impl RequestUserInputOverlay { // Ensure the selected option is visible in the scroll window. options_state .ensure_visible(option_rows.len(), sections.options_area.height as usize); - render_rows( + render_rows_bottom_aligned( sections.options_area, buf, &option_rows, @@ -431,6 +431,48 @@ fn line_width(line: &Line<'_>) -> usize { .sum() } +/// Render rows into `area`, bottom-aligning the visible rows when fewer than +/// `area.height` lines are produced. +/// +/// This keeps footer spacing stable by anchoring the options block to the +/// bottom of its allocated region. +fn render_rows_bottom_aligned( + area: Rect, + buf: &mut Buffer, + rows: &[crate::bottom_pane::selection_popup_common::GenericDisplayRow], + state: &ScrollState, + max_results: usize, + empty_message: &str, +) { + if area.width == 0 || area.height == 0 { + return; + } + + let scratch_area = Rect::new(0, 0, area.width, area.height); + let mut scratch = Buffer::empty(scratch_area); + for y in 0..area.height { + for x in 0..area.width { + scratch[(x, y)] = buf[(area.x + x, area.y + y)].clone(); + } + } + let rendered_height = render_rows( + scratch_area, + &mut scratch, + rows, + state, + max_results, + empty_message, + ); + + let visible_height = rendered_height.min(area.height); + let y_offset = area.height.saturating_sub(visible_height); + for y in 0..visible_height { + for x in 0..area.width { + buf[(area.x + x, area.y + y_offset + y)] = scratch[(x, y)].clone(); + } + } +} + /// Truncate a styled line to `max_width`, preferring a word boundary, and append an ellipsis. /// /// This walks spans character-by-character, tracking the last width-safe position and the last diff --git a/codex-rs/tui/src/bottom_pane/request_user_input/snapshots/codex_tui__bottom_pane__request_user_input__tests__request_user_input_long_option_text.snap b/codex-rs/tui/src/bottom_pane/request_user_input/snapshots/codex_tui__bottom_pane__request_user_input__tests__request_user_input_long_option_text.snap new file mode 100644 index 0000000000..ae5e53b477 --- /dev/null +++ b/codex-rs/tui/src/bottom_pane/request_user_input/snapshots/codex_tui__bottom_pane__request_user_input__tests__request_user_input_long_option_text.snap @@ -0,0 +1,17 @@ +--- +source: tui/src/bottom_pane/request_user_input/mod.rs +expression: "render_snapshot(&overlay, area)" +--- + + Question 1/1 (1 unanswered) + Choose one option. + + › 1. Job: running/completed/failed/expired; Run/Experiment: succeeded/failed/ Keep async job statuses for + unknown (Recommended when triaging long-running background work and status progress tracking and include + transitions) enough context for debugging + retries, stale workers, and + unexpected expiration paths. + 2. Add a short status model Simpler labels with less detail for + quick rollouts. + + tab to add notes | enter to submit answer | esc to interrupt diff --git a/codex-rs/tui/src/bottom_pane/selection_popup_common.rs b/codex-rs/tui/src/bottom_pane/selection_popup_common.rs index 6377bedb95..6b55125c9f 100644 --- a/codex-rs/tui/src/bottom_pane/selection_popup_common.rs +++ b/codex-rs/tui/src/bottom_pane/selection_popup_common.rs @@ -9,6 +9,7 @@ use ratatui::text::Line; use ratatui::text::Span; use ratatui::widgets::Block; use ratatui::widgets::Widget; +use std::borrow::Cow; use unicode_width::UnicodeWidthChar; use unicode_width::UnicodeWidthStr; @@ -52,6 +53,8 @@ pub(crate) enum ColumnWidthMode { Fixed, } +// Fixed split used by explicitly fixed column mode: 30% label, 70% +// description. const FIXED_LEFT_COLUMN_NUMERATOR: usize = 3; const FIXED_LEFT_COLUMN_DENOMINATOR: usize = 10; @@ -107,6 +110,21 @@ fn line_width(line: &Line<'_>) -> usize { .sum() } +fn line_to_owned(line: Line<'_>) -> Line<'static> { + Line { + style: line.style, + alignment: line.alignment, + spans: line + .spans + .into_iter() + .map(|span| Span { + style: span.style, + content: Cow::Owned(span.content.into_owned()), + }) + .collect(), + } +} + pub(crate) fn truncate_line_to_width(line: Line<'static>, max_width: usize) -> Line<'static> { if max_width == 0 { return Line::from(Vec::>::new()); @@ -192,11 +210,6 @@ pub(crate) fn truncate_line_with_ellipsis_if_overflow( } } -/// Computes the shared start column used for descriptions in selection rows. -/// The column is derived from the widest row name plus two spaces of padding -/// while always leaving at least one terminal cell for description content. -/// [`ColumnWidthMode::AutoAllRows`] computes width across the full dataset so -/// the description column does not shift as the user scrolls. fn compute_desc_col( rows_all: &[GenericDisplayRow], start_idx: usize, @@ -209,6 +222,14 @@ fn compute_desc_col( } let max_desc_col = content_width.saturating_sub(1) as usize; + // Reuse the existing fixed split constants to derive the auto cap: + // if fixed mode is 30/70 (label/description), auto mode caps label width + // at 70% to keep at least 30% available for descriptions. + let max_auto_desc_col = max_desc_col.min( + ((content_width as usize * (FIXED_LEFT_COLUMN_DENOMINATOR - FIXED_LEFT_COLUMN_NUMERATOR)) + / FIXED_LEFT_COLUMN_DENOMINATOR) + .max(1), + ); match col_width_mode { ColumnWidthMode::Fixed => ((content_width as usize * FIXED_LEFT_COLUMN_NUMERATOR) / FIXED_LEFT_COLUMN_DENOMINATOR) @@ -243,7 +264,7 @@ fn compute_desc_col( ColumnWidthMode::Fixed => 0, }; - max_name_width.saturating_add(2).min(max_desc_col) + max_name_width.saturating_add(2).min(max_auto_desc_col) } } } @@ -261,6 +282,219 @@ fn wrap_indent(row: &GenericDisplayRow, desc_col: usize, max_width: u16) -> usiz indent.min(max_indent) } +fn should_wrap_name_in_column(row: &GenericDisplayRow) -> bool { + // This path intentionally targets plain option rows that opt into wrapped + // labels. Styled/fuzzy-matched rows keep the legacy combined-line path. + row.wrap_indent.is_some() + && row.description.is_some() + && row.disabled_reason.is_none() + && row.match_indices.is_none() + && row.display_shortcut.is_none() + && row.category_tag.is_none() +} + +fn wrap_two_column_row(row: &GenericDisplayRow, desc_col: usize, width: u16) -> Vec> { + let Some(description) = row.description.as_deref() else { + return Vec::new(); + }; + + let width = width.max(1); + let max_desc_col = width.saturating_sub(1) as usize; + if max_desc_col == 0 { + // No valid description column exists at this width; let callers fall + // back to single-line wrapping path. + return Vec::new(); + } + + let desc_col = desc_col.clamp(1, max_desc_col); + let left_width = desc_col.saturating_sub(2).max(1); + let right_width = width.saturating_sub(desc_col as u16).max(1) as usize; + let name_wrap_indent = row + .wrap_indent + .unwrap_or(0) + .min(left_width.saturating_sub(1)); + + let name_subsequent_indent = " ".repeat(name_wrap_indent); + let name_options = textwrap::Options::new(left_width) + .initial_indent("") + .subsequent_indent(name_subsequent_indent.as_str()); + let name_lines = textwrap::wrap(row.name.as_str(), name_options); + + let desc_options = textwrap::Options::new(right_width).initial_indent(""); + let desc_lines = textwrap::wrap(description, desc_options); + + let rows = name_lines.len().max(desc_lines.len()).max(1); + let mut out = Vec::with_capacity(rows); + for idx in 0..rows { + let mut spans: Vec> = Vec::new(); + if let Some(name) = name_lines.get(idx) { + spans.push(name.to_string().into()); + } + + if let Some(desc) = desc_lines.get(idx) { + let left_used = spans + .iter() + .map(|span| UnicodeWidthStr::width(span.content.as_ref())) + .sum::(); + let gap = if left_used == 0 { + desc_col + } else { + desc_col.saturating_sub(left_used).max(2) + }; + if gap > 0 { + spans.push(" ".repeat(gap).into()); + } + spans.push(desc.to_string().dim()); + } + + out.push(Line::from(spans)); + } + + out +} + +fn wrap_standard_row(row: &GenericDisplayRow, desc_col: usize, width: u16) -> Vec> { + use crate::wrapping::RtOptions; + use crate::wrapping::word_wrap_line; + + let full_line = build_full_line(row, desc_col); + let continuation_indent = wrap_indent(row, desc_col, width); + let options = RtOptions::new(width.max(1) as usize) + .initial_indent(Line::from("")) + .subsequent_indent(Line::from(" ".repeat(continuation_indent))); + word_wrap_line(&full_line, options) + .into_iter() + .map(line_to_owned) + .collect() +} + +fn wrap_row_lines(row: &GenericDisplayRow, desc_col: usize, width: u16) -> Vec> { + if should_wrap_name_in_column(row) { + let wrapped = wrap_two_column_row(row, desc_col, width); + if !wrapped.is_empty() { + return wrapped; + } + } + + wrap_standard_row(row, desc_col, width) +} + +fn apply_row_state_style(lines: &mut [Line<'static>], selected: bool, is_disabled: bool) { + if selected { + for line in lines.iter_mut() { + line.spans.iter_mut().for_each(|span| { + span.style = Style::default().fg(Color::Cyan).bold(); + }); + } + } + if is_disabled { + for line in lines.iter_mut() { + line.spans.iter_mut().for_each(|span| { + span.style = span.style.dim(); + }); + } + } +} + +fn compute_item_window_start( + rows_all: &[GenericDisplayRow], + state: &ScrollState, + max_items: usize, +) -> usize { + if rows_all.is_empty() || max_items == 0 { + return 0; + } + + let mut start_idx = state.scroll_top.min(rows_all.len().saturating_sub(1)); + if let Some(sel) = state.selected_idx { + if sel < start_idx { + start_idx = sel; + } else { + let bottom = start_idx.saturating_add(max_items.saturating_sub(1)); + if sel > bottom { + start_idx = sel + 1 - max_items; + } + } + } + start_idx +} + +fn is_selected_visible_in_wrapped_viewport( + rows_all: &[GenericDisplayRow], + start_idx: usize, + max_items: usize, + selected_idx: usize, + desc_col: usize, + width: u16, + viewport_height: u16, +) -> bool { + if viewport_height == 0 { + return false; + } + + let mut used_lines = 0usize; + let viewport_height = viewport_height as usize; + for (idx, row) in rows_all.iter().enumerate().skip(start_idx).take(max_items) { + let row_lines = wrap_row_lines(row, desc_col, width).len().max(1); + // Keep rendering semantics in sync: always show the first row, even if + // it overflows the viewport. + if used_lines > 0 && used_lines.saturating_add(row_lines) > viewport_height { + break; + } + if idx == selected_idx { + return true; + } + used_lines = used_lines.saturating_add(row_lines); + if used_lines >= viewport_height { + break; + } + } + false +} + +fn adjust_start_for_wrapped_selection_visibility( + rows_all: &[GenericDisplayRow], + state: &ScrollState, + max_items: usize, + desc_measure_items: usize, + width: u16, + viewport_height: u16, + col_width_mode: ColumnWidthMode, +) -> usize { + let mut start_idx = compute_item_window_start(rows_all, state, max_items); + let Some(sel) = state.selected_idx else { + return start_idx; + }; + if viewport_height == 0 { + return start_idx; + } + + // If wrapped row heights push the selected item out of view, advance the + // item window until the selected row is visible. + while start_idx < sel { + let desc_col = compute_desc_col( + rows_all, + start_idx, + desc_measure_items, + width, + col_width_mode, + ); + if is_selected_visible_in_wrapped_viewport( + rows_all, + start_idx, + max_items, + sel, + desc_col, + width, + viewport_height, + ) { + break; + } + start_idx = start_idx.saturating_add(1); + } + start_idx +} + /// Build the full display line for a row with the description padded to start /// at `desc_col`. Applies fuzzy-match bolding when indices are present and /// dims the description. @@ -347,6 +581,8 @@ fn build_full_line(row: &GenericDisplayRow, desc_col: usize) -> Line<'static> { /// Render a list of rows using the provided ScrollState, with shared styling /// and behavior for selection popups. +/// Returns the number of terminal lines actually rendered (including the +/// single-line empty placeholder when shown). fn render_rows_inner( area: Rect, buf: &mut Buffer, @@ -355,36 +591,37 @@ fn render_rows_inner( max_results: usize, empty_message: &str, col_width_mode: ColumnWidthMode, -) { +) -> u16 { if rows_all.is_empty() { if area.height > 0 { Line::from(empty_message.dim().italic()).render(area, buf); } - return; + // Count the placeholder line only when there is vertical space to draw it. + return u16::from(area.height > 0); } - // Determine which logical rows (items) are visible given the selection and - // the max_results clamp. Scrolling is still item-based for simplicity. - let visible_items = max_results - .min(rows_all.len()) - .min(area.height.max(1) as usize); - - let mut start_idx = state.scroll_top.min(rows_all.len().saturating_sub(1)); - if let Some(sel) = state.selected_idx { - if sel < start_idx { - start_idx = sel; - } else if visible_items > 0 { - let bottom = start_idx + visible_items - 1; - if sel > bottom { - start_idx = sel + 1 - visible_items; - } - } + let max_items = max_results.min(rows_all.len()); + if max_items == 0 { + return 0; } + let desc_measure_items = max_items.min(area.height.max(1) as usize); + + // Keep item-window semantics, then correct for wrapped row heights so the + // selected row remains visible in a line-based viewport. + let start_idx = adjust_start_for_wrapped_selection_visibility( + rows_all, + state, + max_items, + desc_measure_items, + area.width, + area.height, + col_width_mode, + ); let desc_col = compute_desc_col( rows_all, start_idx, - visible_items, + desc_measure_items, area.width, col_width_mode, ); @@ -392,38 +629,18 @@ fn render_rows_inner( // Render items, wrapping descriptions and aligning wrapped lines under the // shared description column. Stop when we run out of vertical space. let mut cur_y = area.y; - for (i, row) in rows_all - .iter() - .enumerate() - .skip(start_idx) - .take(visible_items) - { + let mut rendered_lines: u16 = 0; + for (i, row) in rows_all.iter().enumerate().skip(start_idx).take(max_items) { if cur_y >= area.y + area.height { break; } - let mut full_line = build_full_line(row, desc_col); - if Some(i) == state.selected_idx && !row.is_disabled { - // Match previous behavior: cyan + bold for the selected row. - // Reset the style first to avoid inheriting dim from keyboard shortcuts. - full_line.spans.iter_mut().for_each(|span| { - span.style = Style::default().fg(Color::Cyan).bold(); - }); - } - if row.is_disabled { - full_line.spans.iter_mut().for_each(|span| { - span.style = span.style.dim(); - }); - } - - // Wrap with subsequent indent aligned to the description column. - use crate::wrapping::RtOptions; - use crate::wrapping::word_wrap_line; - let continuation_indent = wrap_indent(row, desc_col, area.width); - let options = RtOptions::new(area.width as usize) - .initial_indent(Line::from("")) - .subsequent_indent(Line::from(" ".repeat(continuation_indent))); - let wrapped = word_wrap_line(&full_line, options); + let mut wrapped = wrap_row_lines(row, desc_col, area.width); + apply_row_state_style( + &mut wrapped, + Some(i) == state.selected_idx && !row.is_disabled, + row.is_disabled, + ); // Render the wrapped lines. for line in wrapped { @@ -440,8 +657,11 @@ fn render_rows_inner( buf, ); cur_y = cur_y.saturating_add(1); + rendered_lines = rendered_lines.saturating_add(1); } } + + rendered_lines } /// Render a list of rows using the provided ScrollState, with shared styling @@ -451,6 +671,7 @@ fn render_rows_inner( /// /// This function should be paired with [`measure_rows_height`] when reserving /// space; pairing it with a different measurement mode can cause clipping. +/// Returns the number of terminal lines actually rendered. pub(crate) fn render_rows( area: Rect, buf: &mut Buffer, @@ -458,7 +679,7 @@ pub(crate) fn render_rows( state: &ScrollState, max_results: usize, empty_message: &str, -) { +) -> u16 { render_rows_inner( area, buf, @@ -467,7 +688,7 @@ pub(crate) fn render_rows( max_results, empty_message, ColumnWidthMode::AutoVisible, - ); + ) } /// Render a list of rows using the provided ScrollState, with shared styling @@ -478,6 +699,7 @@ pub(crate) fn render_rows( /// This function should be paired with /// [`measure_rows_height_stable_col_widths`] so reserved and rendered heights /// stay in sync. +/// Returns the number of terminal lines actually rendered. pub(crate) fn render_rows_stable_col_widths( area: Rect, buf: &mut Buffer, @@ -485,7 +707,7 @@ pub(crate) fn render_rows_stable_col_widths( state: &ScrollState, max_results: usize, empty_message: &str, -) { +) -> u16 { render_rows_inner( area, buf, @@ -494,7 +716,7 @@ pub(crate) fn render_rows_stable_col_widths( max_results, empty_message, ColumnWidthMode::AutoAllRows, - ); + ) } /// Render a list of rows using the provided ScrollState and explicit @@ -502,6 +724,7 @@ pub(crate) fn render_rows_stable_col_widths( /// /// This is the low-level entry point for callers that need to thread a mode /// through higher-level configuration. +/// Returns the number of terminal lines actually rendered. pub(crate) fn render_rows_with_col_width_mode( area: Rect, buf: &mut Buffer, @@ -510,7 +733,7 @@ pub(crate) fn render_rows_with_col_width_mode( max_results: usize, empty_message: &str, col_width_mode: ColumnWidthMode, -) { +) -> u16 { render_rows_inner( area, buf, @@ -519,13 +742,14 @@ pub(crate) fn render_rows_with_col_width_mode( max_results, empty_message, col_width_mode, - ); + ) } /// Render rows as a single line each (no wrapping), truncating overflow with an ellipsis. /// /// This path always uses viewport-local width alignment and is best for dense /// list UIs where multi-line descriptions would add too much vertical churn. +/// Returns the number of terminal lines actually rendered. pub(crate) fn render_rows_single_line( area: Rect, buf: &mut Buffer, @@ -533,12 +757,13 @@ pub(crate) fn render_rows_single_line( state: &ScrollState, max_results: usize, empty_message: &str, -) { +) -> u16 { if rows_all.is_empty() { if area.height > 0 { Line::from(empty_message.dim().italic()).render(area, buf); } - return; + // Count the placeholder line only when there is vertical space to draw it. + return u16::from(area.height > 0); } let visible_items = max_results @@ -566,6 +791,7 @@ pub(crate) fn render_rows_single_line( ); let mut cur_y = area.y; + let mut rendered_lines: u16 = 0; for (i, row) in rows_all .iter() .enumerate() @@ -599,7 +825,10 @@ pub(crate) fn render_rows_single_line( buf, ); cur_y = cur_y.saturating_add(1); + rendered_lines = rendered_lines.saturating_add(1); } + + rendered_lines } /// Compute the number of terminal rows required to render up to `max_results` @@ -690,8 +919,6 @@ fn measure_rows_height_inner( col_width_mode, ); - use crate::wrapping::RtOptions; - use crate::wrapping::word_wrap_line; let mut total: u16 = 0; for row in rows_all .iter() @@ -700,13 +927,27 @@ fn measure_rows_height_inner( .take(visible_items) .map(|(_, r)| r) { - let full_line = build_full_line(row, desc_col); - let continuation_indent = wrap_indent(row, desc_col, content_width); - let opts = RtOptions::new(content_width as usize) - .initial_indent(Line::from("")) - .subsequent_indent(Line::from(" ".repeat(continuation_indent))); - let wrapped_lines = word_wrap_line(&full_line, opts).len(); + let wrapped_lines = wrap_row_lines(row, desc_col, content_width).len(); total = total.saturating_add(wrapped_lines as u16); } total.max(1) } + +#[cfg(test)] +mod tests { + use super::*; + use pretty_assertions::assert_eq; + + #[test] + fn one_cell_width_falls_back_without_panic_for_wrapped_two_column_rows() { + let row = GenericDisplayRow { + name: "1. Very long option label".to_string(), + description: Some("Very long description".to_string()), + wrap_indent: Some(4), + ..Default::default() + }; + + let two_col = wrap_two_column_row(&row, 0, 1); + assert_eq!(two_col.len(), 0); + } +} From 9450cd9ce510ee07fdb80a3bb4782cb303803f4b Mon Sep 17 00:00:00 2001 From: Charley Cunningham Date: Mon, 9 Feb 2026 12:42:20 -0800 Subject: [PATCH 06/23] core: add focused diagnostics for remote compaction overflows (#11133) ## Summary - add targeted remote-compaction failure diagnostics in compact_remote logging - log the specific values needed to explain overflow timing: - last_api_response_total_tokens - estimated_tokens_of_items_added_since_last_successful_api_response - estimated_bytes_of_items_added_since_last_successful_api_response - failing_compaction_request_body_bytes - simplify breakdown naming and remove last_api_response_total_bytes_estimate (it was an approximation and not useful for debugging) ## Why When compaction fails with context_length_exceeded, we need concrete, low-ambiguity numbers that map directly to: 1) what the API most recently reported, and 2) what local history added since then. This keeps the failure logs actionable without adding broad, noisy metrics. ## Testing - just fmt - cargo test -p codex-core --- codex-rs/core/src/codex.rs | 13 ++- codex-rs/core/src/compact_remote.rs | 58 +++++++++++++ codex-rs/core/src/context_manager/history.rs | 83 +++++++++++++------ .../core/src/context_manager/history_tests.rs | 15 +++- codex-rs/core/src/context_manager/mod.rs | 2 + codex-rs/core/src/truncate.rs | 8 ++ 6 files changed, 150 insertions(+), 29 deletions(-) diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index a6ce928b4a..ceb8fcf949 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -114,6 +114,7 @@ use crate::config::resolve_web_search_mode_for_turn; use crate::config::types::McpServerConfig; use crate::config::types::ShellEnvironmentPolicy; use crate::context_manager::ContextManager; +use crate::context_manager::TotalTokenUsageBreakdown; use crate::environment_context::EnvironmentContext; use crate::error::CodexErr; use crate::error::Result as CodexResult; @@ -1231,12 +1232,20 @@ impl Session { format!("auto-compact-{id}") } - async fn get_total_token_usage(&self) -> i64 { + pub(crate) async fn get_total_token_usage(&self) -> i64 { let state = self.state.lock().await; state.get_total_token_usage(state.server_reasoning_included()) } - async fn get_estimated_token_count(&self, turn_context: &TurnContext) -> Option { + pub(crate) async fn get_total_token_usage_breakdown(&self) -> TotalTokenUsageBreakdown { + let state = self.state.lock().await; + state.history.get_total_token_usage_breakdown() + } + + pub(crate) async fn get_estimated_token_count( + &self, + turn_context: &TurnContext, + ) -> Option { let state = self.state.lock().await; state.history.estimate_token_count(turn_context) } diff --git a/codex-rs/core/src/compact_remote.rs b/codex-rs/core/src/compact_remote.rs index eac3188cd5..8c7808e3fb 100644 --- a/codex-rs/core/src/compact_remote.rs +++ b/codex-rs/core/src/compact_remote.rs @@ -4,7 +4,10 @@ use crate::Prompt; use crate::codex::Session; use crate::codex::TurnContext; use crate::context_manager::ContextManager; +use crate::context_manager::TotalTokenUsageBreakdown; +use crate::context_manager::estimate_response_item_model_visible_bytes; use crate::context_manager::is_codex_generated_item; +use crate::error::CodexErr; use crate::error::Result as CodexResult; use crate::protocol::CompactedItem; use crate::protocol::EventMsg; @@ -14,6 +17,8 @@ use codex_protocol::items::ContextCompactionItem; use codex_protocol::items::TurnItem; use codex_protocol::models::BaseInstructions; use codex_protocol::models::ResponseItem; +use futures::TryFutureExt; +use tracing::error; use tracing::info; pub(crate) async fn run_inline_remote_auto_compact_task( @@ -98,6 +103,18 @@ async fn run_remote_compact_task_inner_impl( &turn_context.model_info, &turn_context.otel_manager, ) + .or_else(|err| async { + let total_usage_breakdown = sess.get_total_token_usage_breakdown().await; + let compact_request_log_data = + build_compact_request_log_data(&prompt.input, &prompt.base_instructions.text); + log_remote_compact_failure( + turn_context, + &compact_request_log_data, + total_usage_breakdown, + &err, + ); + Err(err) + }) .await?; new_history = sess .process_compacted_history(turn_context, new_history) @@ -121,6 +138,47 @@ async fn run_remote_compact_task_inner_impl( Ok(()) } +#[derive(Debug)] +struct CompactRequestLogData { + failing_compaction_request_model_visible_bytes: i64, +} + +fn build_compact_request_log_data( + input: &[ResponseItem], + instructions: &str, +) -> CompactRequestLogData { + let failing_compaction_request_model_visible_bytes = input + .iter() + .map(estimate_response_item_model_visible_bytes) + .fold( + i64::try_from(instructions.len()).unwrap_or(i64::MAX), + i64::saturating_add, + ); + + CompactRequestLogData { + failing_compaction_request_model_visible_bytes, + } +} + +fn log_remote_compact_failure( + turn_context: &TurnContext, + log_data: &CompactRequestLogData, + total_usage_breakdown: TotalTokenUsageBreakdown, + err: &CodexErr, +) { + error!( + turn_id = %turn_context.sub_id, + last_api_response_total_tokens = total_usage_breakdown.last_api_response_total_tokens, + all_history_items_model_visible_bytes = total_usage_breakdown.all_history_items_model_visible_bytes, + estimated_tokens_of_items_added_since_last_successful_api_response = total_usage_breakdown.estimated_tokens_of_items_added_since_last_successful_api_response, + estimated_bytes_of_items_added_since_last_successful_api_response = total_usage_breakdown.estimated_bytes_of_items_added_since_last_successful_api_response, + model_context_window_tokens = ?turn_context.model_context_window(), + failing_compaction_request_model_visible_bytes = log_data.failing_compaction_request_model_visible_bytes, + compact_error = %err, + "remote compaction failed" + ); +} + fn trim_function_call_history_to_fit_context_window( history: &mut ContextManager, turn_context: &TurnContext, diff --git a/codex-rs/core/src/context_manager/history.rs b/codex-rs/core/src/context_manager/history.rs index 4bf62d791a..a5b2238057 100644 --- a/codex-rs/core/src/context_manager/history.rs +++ b/codex-rs/core/src/context_manager/history.rs @@ -5,7 +5,7 @@ use crate::instructions::UserInstructions; use crate::session_prefix::is_session_prefix; use crate::truncate::TruncationPolicy; use crate::truncate::approx_token_count; -use crate::truncate::approx_tokens_from_byte_count; +use crate::truncate::approx_tokens_from_byte_count_i64; use crate::truncate::truncate_function_output_items_with_policy; use crate::truncate::truncate_text; use crate::user_shell_command::is_user_shell_command_text; @@ -27,6 +27,14 @@ pub(crate) struct ContextManager { token_info: Option, } +#[derive(Debug, Clone, Copy, Default)] +pub(crate) struct TotalTokenUsageBreakdown { + pub last_api_response_total_tokens: i64, + pub all_history_items_model_visible_bytes: i64, + pub estimated_tokens_of_items_added_since_last_successful_api_response: i64, + pub estimated_bytes_of_items_added_since_last_successful_api_response: i64, +} + impl ContextManager { pub(crate) fn new() -> Self { Self { @@ -102,9 +110,11 @@ impl ContextManager { let base_tokens = i64::try_from(approx_token_count(&base_instructions.text)).unwrap_or(i64::MAX); - let items_tokens = self.items.iter().fold(0i64, |acc, item| { - acc.saturating_add(estimate_item_token_count(item)) - }); + let items_tokens = self + .items + .iter() + .map(estimate_item_token_count) + .fold(0i64, i64::saturating_add); Some(base_tokens.saturating_add(items_tokens)) } @@ -231,9 +241,8 @@ impl ContextManager { } ) }) - .fold(0i64, |acc, item| { - acc.saturating_add(estimate_item_token_count(item)) - }) + .map(estimate_item_token_count) + .fold(0i64, i64::saturating_add) } // These are local items added after the most recent model-emitted item. @@ -247,14 +256,6 @@ impl ContextManager { &self.items[start..] } - fn get_items_after_last_model_generated_tokens(&self) -> i64 { - self.items_after_last_model_generated_item() - .iter() - .fold(0i64, |acc, item| { - acc.saturating_add(estimate_item_token_count(item)) - }) - } - /// When true, the server already accounted for past reasoning tokens and /// the client should not re-estimate them. pub(crate) fn get_total_token_usage(&self, server_reasoning_included: bool) -> i64 { @@ -263,8 +264,11 @@ impl ContextManager { .as_ref() .map(|info| info.last_token_usage.total_tokens) .unwrap_or(0); - let items_after_last_model_generated_tokens = - self.get_items_after_last_model_generated_tokens(); + let items_after_last_model_generated_tokens = self + .items_after_last_model_generated_item() + .iter() + .map(estimate_item_token_count) + .fold(0i64, i64::saturating_add); if server_reasoning_included { last_tokens.saturating_add(items_after_last_model_generated_tokens) } else { @@ -274,6 +278,34 @@ impl ContextManager { } } + pub(crate) fn get_total_token_usage_breakdown(&self) -> TotalTokenUsageBreakdown { + let last_usage = self + .token_info + .as_ref() + .map(|info| info.last_token_usage.clone()) + .unwrap_or_default(); + let items_after_last_model_generated = self.items_after_last_model_generated_item(); + + TotalTokenUsageBreakdown { + last_api_response_total_tokens: last_usage.total_tokens, + all_history_items_model_visible_bytes: self + .items + .iter() + .map(estimate_response_item_model_visible_bytes) + .fold(0i64, i64::saturating_add), + estimated_tokens_of_items_added_since_last_successful_api_response: + items_after_last_model_generated + .iter() + .map(estimate_item_token_count) + .fold(0i64, i64::saturating_add), + estimated_bytes_of_items_added_since_last_successful_api_response: + items_after_last_model_generated + .iter() + .map(estimate_response_item_model_visible_bytes) + .fold(0i64, i64::saturating_add), + } + } + /// This function enforces a couple of invariants on the in-memory history: /// 1. every call (function/custom) has a corresponding output entry /// 2. every output has a corresponding call entry @@ -357,6 +389,11 @@ fn estimate_reasoning_length(encoded_len: usize) -> usize { } fn estimate_item_token_count(item: &ResponseItem) -> i64 { + let model_visible_bytes = estimate_response_item_model_visible_bytes(item); + approx_tokens_from_byte_count_i64(model_visible_bytes) +} + +pub(crate) fn estimate_response_item_model_visible_bytes(item: &ResponseItem) -> i64 { match item { ResponseItem::GhostSnapshot { .. } => 0, ResponseItem::Reasoning { @@ -365,14 +402,10 @@ fn estimate_item_token_count(item: &ResponseItem) -> i64 { } | ResponseItem::Compaction { encrypted_content: content, - } => { - let reasoning_bytes = estimate_reasoning_length(content.len()); - i64::try_from(approx_tokens_from_byte_count(reasoning_bytes)).unwrap_or(i64::MAX) - } - item => { - let serialized = serde_json::to_string(item).unwrap_or_default(); - i64::try_from(approx_token_count(&serialized)).unwrap_or(i64::MAX) - } + } => i64::try_from(estimate_reasoning_length(content.len())).unwrap_or(i64::MAX), + item => serde_json::to_string(item) + .map(|serialized| i64::try_from(serialized.len()).unwrap_or(i64::MAX)) + .unwrap_or_default(), } } diff --git a/codex-rs/core/src/context_manager/history_tests.rs b/codex-rs/core/src/context_manager/history_tests.rs index 26bff9f4bd..99670c05e4 100644 --- a/codex-rs/core/src/context_manager/history_tests.rs +++ b/codex-rs/core/src/context_manager/history_tests.rs @@ -193,7 +193,11 @@ fn items_after_last_model_generated_tokens_include_user_and_tool_output() { ); assert_eq!( - history.get_items_after_last_model_generated_tokens(), + history + .items_after_last_model_generated_item() + .iter() + .map(estimate_item_token_count) + .fold(0i64, i64::saturating_add), expected_tokens ); } @@ -202,7 +206,14 @@ fn items_after_last_model_generated_tokens_include_user_and_tool_output() { fn items_after_last_model_generated_tokens_are_zero_without_model_generated_items() { let history = create_history_with_items(vec![user_msg("no model output yet")]); - assert_eq!(history.get_items_after_last_model_generated_tokens(), 0); + assert_eq!( + history + .items_after_last_model_generated_item() + .iter() + .map(estimate_item_token_count) + .fold(0i64, i64::saturating_add), + 0 + ); } #[test] diff --git a/codex-rs/core/src/context_manager/mod.rs b/codex-rs/core/src/context_manager/mod.rs index 22e9682fe3..dcaf794c49 100644 --- a/codex-rs/core/src/context_manager/mod.rs +++ b/codex-rs/core/src/context_manager/mod.rs @@ -2,5 +2,7 @@ mod history; mod normalize; pub(crate) use history::ContextManager; +pub(crate) use history::TotalTokenUsageBreakdown; +pub(crate) use history::estimate_response_item_model_visible_bytes; pub(crate) use history::is_codex_generated_item; pub(crate) use history::is_user_turn_boundary; diff --git a/codex-rs/core/src/truncate.rs b/codex-rs/core/src/truncate.rs index 441a157375..309ed91b6f 100644 --- a/codex-rs/core/src/truncate.rs +++ b/codex-rs/core/src/truncate.rs @@ -304,6 +304,14 @@ pub(crate) fn approx_tokens_from_byte_count(bytes: usize) -> u64 { / (APPROX_BYTES_PER_TOKEN as u64) } +pub(crate) fn approx_tokens_from_byte_count_i64(bytes: i64) -> i64 { + if bytes <= 0 { + return 0; + } + let bytes = usize::try_from(bytes).unwrap_or(usize::MAX); + i64::try_from(approx_tokens_from_byte_count(bytes)).unwrap_or(i64::MAX) +} + #[cfg(test)] mod tests { From becc3a0424b564a7db33eb371c73c7809d2b4728 Mon Sep 17 00:00:00 2001 From: Anton Panasenko Date: Mon, 9 Feb 2026 12:53:50 -0800 Subject: [PATCH 07/23] feat: search_tool (#10657) **Why We Did This** - The goal is to reduce MCP tool context pollution by not exposing the full MCP tool list up front - It forces an explicit discovery step (`search_tool_bm25`) so the model narrows tool scope before making MCP calls, which helps relevance and lowers prompt/tool clutter. **What It Changed** - Added a new experimental feature flag `search_tool` in `core/src/features.rs:90` and `core/src/features.rs:430`. - Added config/schema support for that flag in `core/config.schema.json:214` and `core/config.schema.json:1235`. - Added BM25 dependency (`bm25`) in `Cargo.toml:129` and `core/Cargo.toml:23`. - Added new tool handler `search_tool_bm25` in `core/src/tools/handlers/search_tool_bm25.rs:18`. - Registered the handler and tool spec in `core/src/tools/handlers/mod.rs:11` and `core/src/tools/spec.rs:780` and `core/src/tools/spec.rs:1344`. - Extended `ToolsConfig` to carry `search_tool` enablement in `core/src/tools/spec.rs:32` and `core/src/tools/spec.rs:56`. - Injected dedicated developer instructions for tool-discovery workflow in `core/src/codex.rs:483` and `core/src/codex.rs:1976`, using `core/templates/search_tool/developer_instructions.md:1`. - Added session state to store one-shot selected MCP tools in `core/src/state/session.rs:27` and `core/src/state/session.rs:131`. - Added filtering so when feature is enabled, only selected MCP tools are exposed on the next request (then consumed) in `core/src/codex.rs:3800` and `core/src/codex.rs:3843`. - Added E2E suite coverage for enablement/instructions/hide-until-search/one-turn-selection in `core/tests/suite/search_tool.rs:72`, `core/tests/suite/search_tool.rs:109`, `core/tests/suite/search_tool.rs:147`, and `core/tests/suite/search_tool.rs:218`. - Refactored test helper utilities to support config-driven tool collection in `core/tests/suite/tools.rs:281`. **Net Behavioral Effect** - With `search_tool` **off**: existing MCP behavior (tools exposed normally). - With `search_tool` **on**: MCP tools start hidden, model must call `search_tool_bm25`, and only returned `selected_tools` are available for the next model call. --- codex-rs/Cargo.lock | 108 ++++ codex-rs/Cargo.toml | 1 + codex-rs/core/Cargo.toml | 1 + codex-rs/core/config.schema.json | 6 + codex-rs/core/src/codex.rs | 200 ++++++- codex-rs/core/src/features.rs | 8 + codex-rs/core/src/mcp_connection_manager.rs | 31 ++ codex-rs/core/src/state/session.rs | 104 ++++ codex-rs/core/src/tasks/mod.rs | 1 + codex-rs/core/src/tools/handlers/mod.rs | 3 + .../src/tools/handlers/search_tool_bm25.rs | 217 ++++++++ codex-rs/core/src/tools/spec.rs | 41 ++ .../search_tool/developer_instructions.md | 29 ++ codex-rs/core/tests/suite/mod.rs | 1 + codex-rs/core/tests/suite/search_tool.rs | 488 ++++++++++++++++++ 15 files changed, 1238 insertions(+), 1 deletion(-) create mode 100644 codex-rs/core/src/tools/handlers/search_tool_bm25.rs create mode 100644 codex-rs/core/templates/search_tool/developer_instructions.md create mode 100644 codex-rs/core/tests/suite/search_tool.rs diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index 1b7c3cbbed..3a3a601724 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -922,6 +922,20 @@ dependencies = [ "piper", ] +[[package]] +name = "bm25" +version = "2.3.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "1cbd8ffdfb7b4c2ff038726178a780a94f90525ed0ad264c0afaa75dd8c18a64" +dependencies = [ + "cached", + "deunicode", + "fxhash", + "rust-stemmers", + "stop-words", + "unicode-segmentation", +] + [[package]] name = "borsh" version = "1.6.0" @@ -1000,6 +1014,39 @@ dependencies = [ "pkg-config", ] +[[package]] +name = "cached" +version = "0.56.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "801927ee168e17809ab8901d9f01f700cd7d8d6a6527997fee44e4b0327a253c" +dependencies = [ + "ahash", + "cached_proc_macro", + "cached_proc_macro_types", + "hashbrown 0.15.5", + "once_cell", + "thiserror 2.0.18", + "web-time", +] + +[[package]] +name = "cached_proc_macro" +version = "0.25.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "9225bdcf4e4a9a4c08bf16607908eb2fbf746828d5e0b5e019726dbf6571f201" +dependencies = [ + "darling 0.20.11", + "proc-macro2", + "quote", + "syn 2.0.114", +] + +[[package]] +name = "cached_proc_macro_types" +version = "0.1.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ade8366b8bd5ba243f0a58f036cc0ca8a2f069cff1a2351ef1cac6b083e16fc0" + [[package]] name = "cassowary" version = "0.3.0" @@ -1555,6 +1602,7 @@ dependencies = [ "async-channel", "async-trait", "base64 0.22.1", + "bm25", "chardetng", "chrono", "clap", @@ -2704,6 +2752,16 @@ dependencies = [ "syn 2.0.114", ] +[[package]] +name = "darling" +version = "0.20.11" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "fc7f46116c46ff9ab3eb1597a45688b6715c6e628b5c133e288e709a29bcb4ee" +dependencies = [ + "darling_core 0.20.11", + "darling_macro 0.20.11", +] + [[package]] name = "darling" version = "0.21.3" @@ -2724,6 +2782,20 @@ dependencies = [ "darling_macro 0.23.0", ] +[[package]] +name = "darling_core" +version = "0.20.11" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "0d00b9596d185e565c2207a0b01f8bd1a135483d02d9b7b0a54b11da8d53412e" +dependencies = [ + "fnv", + "ident_case", + "proc-macro2", + "quote", + "strsim 0.11.1", + "syn 2.0.114", +] + [[package]] name = "darling_core" version = "0.21.3" @@ -2751,6 +2823,17 @@ dependencies = [ "syn 2.0.114", ] +[[package]] +name = "darling_macro" +version = "0.20.11" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "fc34b93ccb385b40dc71c6fceac4b2ad23662c7eeb248cf10d529b7e055b6ead" +dependencies = [ + "darling_core 0.20.11", + "quote", + "syn 2.0.114", +] + [[package]] name = "darling_macro" version = "0.21.3" @@ -2955,6 +3038,12 @@ dependencies = [ "unicode-xid", ] +[[package]] +name = "deunicode" +version = "1.6.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "abd57806937c9cc163efc8ea3910e00a62e2aeb0b8119f1793a978088f8f6b04" + [[package]] name = "diff" version = "0.1.13" @@ -7223,6 +7312,16 @@ dependencies = [ "walkdir", ] +[[package]] +name = "rust-stemmers" +version = "1.2.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e46a2036019fdb888131db7a4c847a1063a7493f971ed94ea82c67eada63ca54" +dependencies = [ + "serde", + "serde_derive", +] + [[package]] name = "rustc-demangle" version = "0.1.27" @@ -8456,6 +8555,15 @@ version = "1.1.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "a2eb9349b6444b326872e140eb1cf5e7c522154d69e7a0ffb0fb81c06b37543f" +[[package]] +name = "stop-words" +version = "0.9.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "645a3d441ccf4bf47f2e4b7681461986681a6eeea9937d4c3bc9febd61d17c71" +dependencies = [ + "serde_json", +] + [[package]] name = "streaming-iterator" version = "0.1.9" diff --git a/codex-rs/Cargo.toml b/codex-rs/Cargo.toml index 44a4c9a34e..c2dee35e57 100644 --- a/codex-rs/Cargo.toml +++ b/codex-rs/Cargo.toml @@ -130,6 +130,7 @@ async-stream = "0.3.6" async-trait = "0.1.89" axum = { version = "0.8", default-features = false } base64 = "0.22.1" +bm25 = "2.3.2" bytes = "1.10.1" chardetng = "0.1.17" chrono = "0.4.43" diff --git a/codex-rs/core/Cargo.toml b/codex-rs/core/Cargo.toml index 5a8b07ce80..30339858c5 100644 --- a/codex-rs/core/Cargo.toml +++ b/codex-rs/core/Cargo.toml @@ -23,6 +23,7 @@ arc-swap = "1.8.0" async-channel = { workspace = true } async-trait = { workspace = true } base64 = { workspace = true } +bm25 = { workspace = true } chardetng = { workspace = true } chrono = { workspace = true, features = ["serde"] } clap = { workspace = true, features = ["derive"] } diff --git a/codex-rs/core/config.schema.json b/codex-rs/core/config.schema.json index 53ab772492..37e77a6b7d 100644 --- a/codex-rs/core/config.schema.json +++ b/codex-rs/core/config.schema.json @@ -245,6 +245,9 @@ "runtime_metrics": { "type": "boolean" }, + "search_tool": { + "type": "boolean" + }, "shell_snapshot": { "type": "boolean" }, @@ -1288,6 +1291,9 @@ "runtime_metrics": { "type": "boolean" }, + "search_tool": { + "type": "boolean" + }, "shell_snapshot": { "type": "boolean" }, diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index ceb8fcf949..80319fa8a0 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -139,6 +139,8 @@ use crate::mcp::effective_mcp_servers; use crate::mcp::maybe_prompt_and_install_mcp_dependencies; use crate::mcp::with_codex_apps_mcp; use crate::mcp_connection_manager::McpConnectionManager; +use crate::mcp_connection_manager::filter_codex_apps_mcp_tools_only; +use crate::mcp_connection_manager::filter_mcp_tools_by_name; use crate::mentions::build_connector_slug_counts; use crate::mentions::build_skill_name_counts; use crate::mentions::collect_explicit_app_paths; @@ -504,6 +506,9 @@ pub(crate) struct Session { next_internal_sub_id: AtomicU64, } +const SEARCH_TOOL_DEVELOPER_INSTRUCTIONS: &str = + include_str!("../templates/search_tool/developer_instructions.md"); + /// The context needed for a single turn of the thread. #[derive(Debug)] pub(crate) struct TurnContext { @@ -1257,6 +1262,21 @@ impl Session { } } + pub(crate) async fn merge_mcp_tool_selection(&self, tool_names: Vec) -> Vec { + let mut state = self.state.lock().await; + state.merge_mcp_tool_selection(tool_names) + } + + pub(crate) async fn get_mcp_tool_selection(&self) -> Option> { + let state = self.state.lock().await; + state.get_mcp_tool_selection() + } + + pub(crate) async fn clear_mcp_tool_selection(&self) { + let mut state = self.state.lock().await; + state.clear_mcp_tool_selection(); + } + async fn record_initial_history(&self, conversation_history: InitialHistory) { let turn_context = self.new_default_turn().await; match conversation_history { @@ -2182,6 +2202,11 @@ impl Session { if let Some(developer_instructions) = turn_context.developer_instructions.as_deref() { items.push(DeveloperInstructions::new(developer_instructions.to_string()).into()); } + if turn_context.tools_config.search_tool { + items.push( + DeveloperInstructions::new(SEARCH_TOOL_DEVELOPER_INSTRUCTIONS.to_string()).into(), + ); + } // Add developer instructions from collaboration_mode if they exist and are non-empty let (collaboration_mode, base_instructions) = { let state = self.state.lock().await; @@ -4119,6 +4144,7 @@ async fn run_sampling_request( .list_all_tools() .or_cancel(&cancellation_token) .await?; + let connectors_for_tools = if turn_context.config.features.enabled(Feature::Apps) { let connectors = connectors::accessible_connectors_from_mcp_tools(&mcp_tools); Some(filter_connectors_for_input( @@ -4130,9 +4156,25 @@ async fn run_sampling_request( } else { None }; - if let Some(connectors) = connectors_for_tools.as_ref() { + + if turn_context.config.features.enabled(Feature::SearchTool) { + let mut selected_mcp_tools = + if let Some(selected_tools) = sess.get_mcp_tool_selection().await { + filter_mcp_tools_by_name(mcp_tools.clone(), &selected_tools) + } else { + HashMap::new() + }; + + if let Some(connectors) = connectors_for_tools.as_ref() { + let apps_mcp_tools = filter_codex_apps_mcp_tools_only(mcp_tools, connectors); + selected_mcp_tools.extend(apps_mcp_tools); + } + + mcp_tools = selected_mcp_tools; + } else if let Some(connectors) = connectors_for_tools.as_ref() { mcp_tools = filter_codex_apps_mcp_tools(mcp_tools, connectors); } + let router = Arc::new(ToolRouter::from_config( &turn_context.tools_config, Some( @@ -4958,6 +5000,8 @@ pub(super) fn get_last_assistant_message_from_turn(responses: &[ResponseItem]) - pub(crate) use tests::make_session_and_context; #[cfg(test)] pub(crate) use tests::make_session_and_context_with_rx; +#[cfg(test)] +pub(crate) use tests::make_session_configuration_for_tests; #[cfg(test)] mod tests { @@ -4967,6 +5011,7 @@ mod tests { use crate::config::test_config; use crate::exec::ExecToolCallOutput; use crate::function_tool::FunctionCallError; + use crate::mcp_connection_manager::ToolInfo; use crate::shell::default_user_shell; use crate::tools::format_exec_output_str; @@ -5006,6 +5051,8 @@ mod tests { use codex_protocol::mcp::CallToolResult as McpCallToolResult; use pretty_assertions::assert_eq; + use rmcp::model::JsonObject; + use rmcp::model::Tool; use serde::Deserialize; use serde_json::json; use std::path::PathBuf; @@ -5042,6 +5089,30 @@ mod tests { } } + fn make_mcp_tool( + server_name: &str, + tool_name: &str, + connector_id: Option<&str>, + connector_name: Option<&str>, + ) -> ToolInfo { + ToolInfo { + server_name: server_name.to_string(), + tool_name: tool_name.to_string(), + tool: Tool { + name: tool_name.to_string().into(), + title: None, + description: Some(format!("Test tool: {tool_name}").into()), + input_schema: Arc::new(JsonObject::default()), + output_schema: None, + annotations: None, + icons: None, + meta: None, + }, + connector_id: connector_id.map(str::to_string), + connector_name: connector_name.map(str::to_string), + } + } + #[tokio::test] async fn get_base_instructions_no_user_content() { let prompt_with_apply_patch_instructions = @@ -5145,6 +5216,93 @@ mod tests { assert_eq!(selected, Vec::new()); } + #[test] + fn search_tool_selection_keeps_codex_apps_tools_without_mentions() { + let selected_tool_names = vec![ + "mcp__codex_apps__calendar_create_event".to_string(), + "mcp__rmcp__echo".to_string(), + ]; + let mcp_tools = HashMap::from([ + ( + "mcp__codex_apps__calendar_create_event".to_string(), + make_mcp_tool( + CODEX_APPS_MCP_SERVER_NAME, + "calendar_create_event", + Some("calendar"), + Some("Calendar"), + ), + ), + ( + "mcp__rmcp__echo".to_string(), + make_mcp_tool("rmcp", "echo", None, None), + ), + ]); + + let mut selected_mcp_tools = + filter_mcp_tools_by_name(mcp_tools.clone(), &selected_tool_names); + let connectors = connectors::accessible_connectors_from_mcp_tools(&mcp_tools); + let connectors = filter_connectors_for_input( + connectors, + &[user_message("run the selected tools")], + &[], + &HashMap::new(), + ); + let apps_mcp_tools = filter_codex_apps_mcp_tools_only(mcp_tools, &connectors); + selected_mcp_tools.extend(apps_mcp_tools); + + let mut tool_names: Vec = selected_mcp_tools.into_keys().collect(); + tool_names.sort(); + assert_eq!( + tool_names, + vec![ + "mcp__codex_apps__calendar_create_event".to_string(), + "mcp__rmcp__echo".to_string(), + ] + ); + } + + #[test] + fn apps_mentions_add_codex_apps_tools_to_search_selected_set() { + let selected_tool_names = vec!["mcp__rmcp__echo".to_string()]; + let mcp_tools = HashMap::from([ + ( + "mcp__codex_apps__calendar_create_event".to_string(), + make_mcp_tool( + CODEX_APPS_MCP_SERVER_NAME, + "calendar_create_event", + Some("calendar"), + Some("Calendar"), + ), + ), + ( + "mcp__rmcp__echo".to_string(), + make_mcp_tool("rmcp", "echo", None, None), + ), + ]); + + let mut selected_mcp_tools = + filter_mcp_tools_by_name(mcp_tools.clone(), &selected_tool_names); + let connectors = connectors::accessible_connectors_from_mcp_tools(&mcp_tools); + let connectors = filter_connectors_for_input( + connectors, + &[user_message("use $calendar and then echo the response")], + &[], + &HashMap::new(), + ); + let apps_mcp_tools = filter_codex_apps_mcp_tools_only(mcp_tools, &connectors); + selected_mcp_tools.extend(apps_mcp_tools); + + let mut tool_names: Vec = selected_mcp_tools.into_keys().collect(); + tool_names.sort(); + assert_eq!( + tool_names, + vec![ + "mcp__codex_apps__calendar_create_event".to_string(), + "mcp__rmcp__echo".to_string(), + ] + ); + } + #[tokio::test] async fn reconstruct_history_matches_live_compactions() { let (session, turn_context) = make_session_and_context().await; @@ -5849,6 +6007,46 @@ mod tests { ) } + pub(crate) async fn make_session_configuration_for_tests() -> SessionConfiguration { + let codex_home = tempfile::tempdir().expect("create temp dir"); + let config = build_test_config(codex_home.path()).await; + let config = Arc::new(config); + let model = ModelsManager::get_model_offline(config.model.as_deref()); + let model_info = ModelsManager::construct_model_info_offline(model.as_str(), &config); + let reasoning_effort = config.model_reasoning_effort; + let collaboration_mode = CollaborationMode { + mode: ModeKind::Default, + settings: Settings { + model, + reasoning_effort, + developer_instructions: None, + }, + }; + + SessionConfiguration { + provider: config.model_provider.clone(), + collaboration_mode, + model_reasoning_summary: config.model_reasoning_summary, + developer_instructions: config.developer_instructions.clone(), + user_instructions: config.user_instructions.clone(), + personality: config.personality, + base_instructions: config + .base_instructions + .clone() + .unwrap_or_else(|| model_info.get_model_instructions(config.personality)), + compact_prompt: config.compact_prompt.clone(), + approval_policy: config.approval_policy.clone(), + sandbox_policy: config.sandbox_policy.clone(), + windows_sandbox_level: WindowsSandboxLevel::from_config(&config), + cwd: config.cwd.clone(), + codex_home: config.codex_home.clone(), + thread_name: None, + original_config_do_not_use: Arc::clone(&config), + session_source: SessionSource::Exec, + dynamic_tools: Vec::new(), + } + } + pub(crate) async fn make_session_and_context() -> (Session, TurnContext) { let (tx_event, _rx_event) = async_channel::unbounded(); let codex_home = tempfile::tempdir().expect("create temp dir"); diff --git a/codex-rs/core/src/features.rs b/codex-rs/core/src/features.rs index 7de37e78ad..976cad0f94 100644 --- a/codex-rs/core/src/features.rs +++ b/codex-rs/core/src/features.rs @@ -87,6 +87,8 @@ pub enum Feature { /// Allow the model to request web searches that fetch cached content. /// Takes precedence over `WebSearchRequest`. WebSearchCached, + /// Allow the model to search MCP tools via BM25 before exposing them. + SearchTool, /// Use the bubblewrap-based Linux sandbox pipeline. UseLinuxSandboxBwrap, /// Allow the model to request approval and propose exec rules. @@ -432,6 +434,12 @@ pub const FEATURES: &[FeatureSpec] = &[ stage: Stage::Deprecated, default_enabled: false, }, + FeatureSpec { + id: Feature::SearchTool, + key: "search_tool", + stage: Stage::UnderDevelopment, + default_enabled: false, + }, // Experimental program. Rendered in the `/experimental` menu for users. FeatureSpec { id: Feature::RuntimeMetrics, diff --git a/codex-rs/core/src/mcp_connection_manager.rs b/codex-rs/core/src/mcp_connection_manager.rs index 38e5bcb6d6..4a6357162f 100644 --- a/codex-rs/core/src/mcp_connection_manager.rs +++ b/codex-rs/core/src/mcp_connection_manager.rs @@ -843,6 +843,37 @@ fn filter_tools(tools: Vec, filter: ToolFilter) -> Vec { .collect() } +pub(crate) fn filter_codex_apps_mcp_tools_only( + mut mcp_tools: HashMap, + connectors: &[crate::connectors::AppInfo], +) -> HashMap { + let allowed: HashSet<&str> = connectors + .iter() + .map(|connector| connector.id.as_str()) + .collect(); + + mcp_tools.retain(|_, tool| { + if tool.server_name != CODEX_APPS_MCP_SERVER_NAME { + return false; + } + let Some(connector_id) = tool.connector_id.as_deref() else { + return false; + }; + allowed.contains(connector_id) + }); + + mcp_tools +} + +pub(crate) fn filter_mcp_tools_by_name( + mut mcp_tools: HashMap, + selected_tools: &[String], +) -> HashMap { + let allowed: HashSet<&str> = selected_tools.iter().map(String::as_str).collect(); + mcp_tools.retain(|name, _| allowed.contains(name.as_str())); + mcp_tools +} + fn normalize_codex_apps_tool_title( server_name: &str, connector_name: Option<&str>, diff --git a/codex-rs/core/src/state/session.rs b/codex-rs/core/src/state/session.rs index 03001b712b..dcbf817d99 100644 --- a/codex-rs/core/src/state/session.rs +++ b/codex-rs/core/src/state/session.rs @@ -29,6 +29,7 @@ pub(crate) struct SessionState { pub(crate) pending_resume_previous_model: Option, /// Startup regular task pre-created during session initialization. pub(crate) startup_regular_task: Option, + pub(crate) active_mcp_tool_selection: Option>, } impl SessionState { @@ -45,6 +46,7 @@ impl SessionState { initial_context_seeded: false, pending_resume_previous_model: None, startup_regular_task: None, + active_mcp_tool_selection: None, } } @@ -140,6 +142,32 @@ impl SessionState { pub(crate) fn take_startup_regular_task(&mut self) -> Option { self.startup_regular_task.take() } + + pub(crate) fn merge_mcp_tool_selection(&mut self, tool_names: Vec) -> Vec { + if tool_names.is_empty() { + return self.active_mcp_tool_selection.clone().unwrap_or_default(); + } + + let mut merged = self.active_mcp_tool_selection.take().unwrap_or_default(); + let mut seen: HashSet = merged.iter().cloned().collect(); + + for tool_name in tool_names { + if seen.insert(tool_name.clone()) { + merged.push(tool_name); + } + } + + self.active_mcp_tool_selection = Some(merged.clone()); + merged + } + + pub(crate) fn get_mcp_tool_selection(&self) -> Option> { + self.active_mcp_tool_selection.clone() + } + + pub(crate) fn clear_mcp_tool_selection(&mut self) { + self.active_mcp_tool_selection = None; + } } // Sometimes new snapshots don't include credits or plan information. @@ -155,3 +183,79 @@ fn merge_rate_limit_fields( } snapshot } + +#[cfg(test)] +mod tests { + use super::*; + use crate::codex::make_session_configuration_for_tests; + use pretty_assertions::assert_eq; + + #[tokio::test] + async fn merge_mcp_tool_selection_deduplicates_and_preserves_order() { + let session_configuration = make_session_configuration_for_tests().await; + let mut state = SessionState::new(session_configuration); + + let merged = state.merge_mcp_tool_selection(vec![ + "mcp__rmcp__echo".to_string(), + "mcp__rmcp__image".to_string(), + "mcp__rmcp__echo".to_string(), + ]); + assert_eq!( + merged, + vec![ + "mcp__rmcp__echo".to_string(), + "mcp__rmcp__image".to_string(), + ] + ); + + let merged = state.merge_mcp_tool_selection(vec![ + "mcp__rmcp__image".to_string(), + "mcp__rmcp__search".to_string(), + ]); + assert_eq!( + merged, + vec![ + "mcp__rmcp__echo".to_string(), + "mcp__rmcp__image".to_string(), + "mcp__rmcp__search".to_string(), + ] + ); + } + + #[tokio::test] + async fn merge_mcp_tool_selection_empty_input_is_noop() { + let session_configuration = make_session_configuration_for_tests().await; + let mut state = SessionState::new(session_configuration); + state.merge_mcp_tool_selection(vec![ + "mcp__rmcp__echo".to_string(), + "mcp__rmcp__image".to_string(), + ]); + + let merged = state.merge_mcp_tool_selection(Vec::new()); + assert_eq!( + merged, + vec![ + "mcp__rmcp__echo".to_string(), + "mcp__rmcp__image".to_string(), + ] + ); + assert_eq!( + state.get_mcp_tool_selection(), + Some(vec![ + "mcp__rmcp__echo".to_string(), + "mcp__rmcp__image".to_string(), + ]) + ); + } + + #[tokio::test] + async fn clear_mcp_tool_selection_removes_selection() { + let session_configuration = make_session_configuration_for_tests().await; + let mut state = SessionState::new(session_configuration); + state.merge_mcp_tool_selection(vec!["mcp__rmcp__echo".to_string()]); + + state.clear_mcp_tool_selection(); + + assert_eq!(state.get_mcp_tool_selection(), None); + } +} diff --git a/codex-rs/core/src/tasks/mod.rs b/codex-rs/core/src/tasks/mod.rs index 9a12c48679..cbb6cd4b37 100644 --- a/codex-rs/core/src/tasks/mod.rs +++ b/codex-rs/core/src/tasks/mod.rs @@ -120,6 +120,7 @@ impl Session { task: T, ) { self.abort_all_tasks(TurnAbortReason::Replaced).await; + self.clear_mcp_tool_selection().await; self.seed_initial_context_if_needed(turn_context.as_ref()) .await; diff --git a/codex-rs/core/src/tools/handlers/mod.rs b/codex-rs/core/src/tools/handlers/mod.rs index dda4760bd7..b824665deb 100644 --- a/codex-rs/core/src/tools/handlers/mod.rs +++ b/codex-rs/core/src/tools/handlers/mod.rs @@ -8,6 +8,7 @@ mod mcp_resource; mod plan; mod read_file; mod request_user_input; +mod search_tool_bm25; mod shell; mod test_sync; mod unified_exec; @@ -28,6 +29,8 @@ pub use plan::PlanHandler; pub use read_file::ReadFileHandler; pub use request_user_input::RequestUserInputHandler; pub(crate) use request_user_input::request_user_input_tool_description; +pub(crate) use search_tool_bm25::DEFAULT_LIMIT as SEARCH_TOOL_BM25_DEFAULT_LIMIT; +pub use search_tool_bm25::SearchToolBm25Handler; pub use shell::ShellCommandHandler; pub use shell::ShellHandler; pub use test_sync::TestSyncHandler; diff --git a/codex-rs/core/src/tools/handlers/search_tool_bm25.rs b/codex-rs/core/src/tools/handlers/search_tool_bm25.rs new file mode 100644 index 0000000000..1da8a3d03f --- /dev/null +++ b/codex-rs/core/src/tools/handlers/search_tool_bm25.rs @@ -0,0 +1,217 @@ +use async_trait::async_trait; +use bm25::Document; +use bm25::Language; +use bm25::SearchEngineBuilder; +use codex_protocol::models::FunctionCallOutputBody; +use serde::Deserialize; +use serde_json::json; + +use crate::function_tool::FunctionCallError; +use crate::mcp_connection_manager::ToolInfo; +use crate::tools::context::ToolInvocation; +use crate::tools::context::ToolOutput; +use crate::tools::context::ToolPayload; +use crate::tools::handlers::parse_arguments; +use crate::tools::registry::ToolHandler; +use crate::tools::registry::ToolKind; + +pub struct SearchToolBm25Handler; + +pub(crate) const DEFAULT_LIMIT: usize = 8; + +fn default_limit() -> usize { + DEFAULT_LIMIT +} + +#[derive(Deserialize)] +struct SearchToolBm25Args { + query: String, + #[serde(default = "default_limit")] + limit: usize, +} + +#[derive(Clone)] +struct ToolEntry { + name: String, + server_name: String, + title: Option, + description: Option, + connector_id: Option, + connector_name: Option, + input_keys: Vec, + search_text: String, +} + +impl ToolEntry { + fn new(name: String, info: ToolInfo) -> Self { + let input_keys = info + .tool + .input_schema + .get("properties") + .and_then(serde_json::Value::as_object) + .map(|map| map.keys().cloned().collect::>()) + .unwrap_or_default(); + let search_text = build_search_text(&name, &info, &input_keys); + Self { + name, + server_name: info.server_name, + title: info.tool.title, + description: info + .tool + .description + .map(|description| description.to_string()), + connector_id: info.connector_id, + connector_name: info.connector_name, + input_keys, + search_text, + } + } +} + +#[async_trait] +impl ToolHandler for SearchToolBm25Handler { + fn kind(&self) -> ToolKind { + ToolKind::Function + } + + async fn handle(&self, invocation: ToolInvocation) -> Result { + let ToolInvocation { + payload, session, .. + } = invocation; + + let arguments = match payload { + ToolPayload::Function { arguments } => arguments, + _ => { + return Err(FunctionCallError::Fatal( + "search_tool_bm25 handler received unsupported payload".to_string(), + )); + } + }; + + let args: SearchToolBm25Args = parse_arguments(&arguments)?; + let query = args.query.trim(); + if query.is_empty() { + return Err(FunctionCallError::RespondToModel( + "query must not be empty".to_string(), + )); + } + + if args.limit == 0 { + return Err(FunctionCallError::RespondToModel( + "limit must be greater than zero".to_string(), + )); + } + + let limit = args.limit; + + let mcp_tools = session + .services + .mcp_connection_manager + .read() + .await + .list_all_tools() + .await; + + let mut entries: Vec = mcp_tools + .into_iter() + .map(|(name, info)| ToolEntry::new(name, info)) + .collect(); + entries.sort_by(|a, b| a.name.cmp(&b.name)); + + if entries.is_empty() { + let active_selected_tools = session.get_mcp_tool_selection().await.unwrap_or_default(); + let content = json!({ + "query": query, + "total_tools": 0, + "active_selected_tools": active_selected_tools, + "tools": [], + }) + .to_string(); + return Ok(ToolOutput::Function { + body: FunctionCallOutputBody::Text(content), + success: Some(true), + }); + } + + let documents: Vec> = entries + .iter() + .enumerate() + .map(|(idx, entry)| Document::new(idx, entry.search_text.clone())) + .collect(); + let search_engine = + SearchEngineBuilder::::with_documents(Language::English, documents).build(); + let results = search_engine.search(query, limit); + + let mut selected_tools = Vec::new(); + let mut result_payloads = Vec::new(); + for result in results { + let Some(entry) = entries.get(result.document.id) else { + continue; + }; + selected_tools.push(entry.name.clone()); + result_payloads.push(json!({ + "name": entry.name.clone(), + "server": entry.server_name.clone(), + "title": entry.title.clone(), + "description": entry.description.clone(), + "connector_id": entry.connector_id.clone(), + "connector_name": entry.connector_name.clone(), + "input_keys": entry.input_keys.clone(), + "score": result.score, + })); + } + + let active_selected_tools = session.merge_mcp_tool_selection(selected_tools).await; + + let content = json!({ + "query": query, + "total_tools": entries.len(), + "active_selected_tools": active_selected_tools, + "tools": result_payloads, + }) + .to_string(); + + Ok(ToolOutput::Function { + body: FunctionCallOutputBody::Text(content), + success: Some(true), + }) + } +} + +fn build_search_text(name: &str, info: &ToolInfo, input_keys: &[String]) -> String { + let mut parts = vec![ + name.to_string(), + info.tool_name.clone(), + info.server_name.clone(), + ]; + + if let Some(title) = info.tool.title.as_deref() + && !title.trim().is_empty() + { + parts.push(title.to_string()); + } + + if let Some(description) = info.tool.description.as_deref() + && !description.trim().is_empty() + { + parts.push(description.to_string()); + } + + if let Some(connector_name) = info.connector_name.as_deref() + && !connector_name.trim().is_empty() + { + parts.push(connector_name.to_string()); + } + + if let Some(connector_id) = info.connector_id.as_deref() + && !connector_id.trim().is_empty() + { + parts.push(connector_id.to_string()); + } + + if !input_keys.is_empty() { + parts.extend(input_keys.iter().cloned()); + } + + parts.join(" ") +} diff --git a/codex-rs/core/src/tools/spec.rs b/codex-rs/core/src/tools/spec.rs index 729b108888..1daa2acc0e 100644 --- a/codex-rs/core/src/tools/spec.rs +++ b/codex-rs/core/src/tools/spec.rs @@ -4,6 +4,7 @@ use crate::client_common::tools::ToolSpec; use crate::features::Feature; use crate::features::Features; use crate::tools::handlers::PLAN_TOOL; +use crate::tools::handlers::SEARCH_TOOL_BM25_DEFAULT_LIMIT; use crate::tools::handlers::apply_patch::create_apply_patch_freeform_tool; use crate::tools::handlers::apply_patch::create_apply_patch_json_tool; use crate::tools::handlers::collab::DEFAULT_WAIT_TIMEOUT_MS; @@ -31,6 +32,7 @@ pub(crate) struct ToolsConfig { 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, pub request_rule_enabled: bool, @@ -54,6 +56,7 @@ impl ToolsConfig { let include_collab_tools = features.enabled(Feature::Collab); let include_collaboration_modes_tools = features.enabled(Feature::CollaborationModes); let request_rule_enabled = features.enabled(Feature::RequestRule); + let include_search_tool = features.enabled(Feature::SearchTool); let shell_type = if !features.enabled(Feature::ShellTool) { ConfigShellToolType::Disabled @@ -85,6 +88,7 @@ impl ToolsConfig { 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, request_rule_enabled, @@ -800,6 +804,36 @@ fn create_grep_files_tool() -> ToolSpec { }) } +fn create_search_tool_bm25_tool() -> ToolSpec { + let properties = BTreeMap::from([ + ( + "query".to_string(), + JsonSchema::String { + description: Some("Search query for MCP tools.".to_string()), + }, + ), + ( + "limit".to_string(), + JsonSchema::Number { + description: Some(format!( + "Maximum number of tools to return (defaults to {SEARCH_TOOL_BM25_DEFAULT_LIMIT})." + )), + }, + ), + ]); + + ToolSpec::Function(ResponsesApiTool { + name: "search_tool_bm25".to_string(), + description: "Searches MCP tool metadata with BM25 and exposes matching tools for the next model call.".to_string(), + strict: false, + parameters: JsonSchema::Object { + properties, + required: Some(vec!["query".to_string()]), + additional_properties: Some(false.into()), + }, + }) +} + fn create_read_file_tool() -> ToolSpec { let indentation_properties = BTreeMap::from([ ( @@ -1261,6 +1295,7 @@ pub(crate) fn build_specs( use crate::tools::handlers::PlanHandler; use crate::tools::handlers::ReadFileHandler; use crate::tools::handlers::RequestUserInputHandler; + use crate::tools::handlers::SearchToolBm25Handler; use crate::tools::handlers::ShellCommandHandler; use crate::tools::handlers::ShellHandler; use crate::tools::handlers::TestSyncHandler; @@ -1280,6 +1315,7 @@ pub(crate) fn build_specs( let mcp_resource_handler = Arc::new(McpResourceHandler); let shell_command_handler = Arc::new(ShellCommandHandler); let request_user_input_handler = Arc::new(RequestUserInputHandler); + let search_tool_handler = Arc::new(SearchToolBm25Handler); match &config.shell_type { ConfigShellToolType::Default => { @@ -1334,6 +1370,11 @@ pub(crate) fn build_specs( builder.register_handler("request_user_input", request_user_input_handler); } + if config.search_tool { + builder.push_spec_with_parallel_support(create_search_tool_bm25_tool(), true); + builder.register_handler("search_tool_bm25", search_tool_handler); + } + if let Some(apply_patch_tool_type) = &config.apply_patch_tool_type { match apply_patch_tool_type { ApplyPatchToolType::Freeform => { diff --git a/codex-rs/core/templates/search_tool/developer_instructions.md b/codex-rs/core/templates/search_tool/developer_instructions.md new file mode 100644 index 0000000000..5ff6a83e50 --- /dev/null +++ b/codex-rs/core/templates/search_tool/developer_instructions.md @@ -0,0 +1,29 @@ +# MCP tool discovery + +When `search_tool_bm25` is available, MCP tools (`mcp__...`) are hidden until you search for them. + +Follow this workflow: + +1. Call `search_tool_bm25` with: + - `query` (required): focused terms that describe the capability you need. + - `limit` (optional): maximum number of tools to return (default `8`). +2. Use the returned `tools` list to decide which MCP tools are relevant. +3. Matching tools are added to `active_selected_tools`. Only tools in `active_selected_tools` are available for the remainder of the current turn. +4. Repeated searches in the same turn are additive: new matches are unioned into `active_selected_tools`. +5. `active_selected_tools` resets at the start of the next turn. + +Notes: +- Core tools remain available without searching. +- If you are unsure, start with `limit` between 5 and 10 to see a broader set of tools. +- `query` is matched against MCP tool metadata fields: + - `name` + - `tool_name` + - `server_name` + - `title` + - `description` + - `connector_name` + - `connector_id` + - input schema property keys (`input_keys`) +- When the user asks to search/lookup/query any external system (logs, tickets, metrics, Slack, etc.), you must call `search_tool_bm25` first before running any shell command or repo search. +- Only use shell commands if (a) MCP tools for that system are not available or not sufficient, and (b) the user explicitly wants a local file/CLI search. +- If unsure which system/tool applies, ask a clarifying question after checking MCP tools. diff --git a/codex-rs/core/tests/suite/mod.rs b/codex-rs/core/tests/suite/mod.rs index b903f4e6ca..7818a5a67e 100644 --- a/codex-rs/core/tests/suite/mod.rs +++ b/codex-rs/core/tests/suite/mod.rs @@ -104,6 +104,7 @@ mod resume_warning; mod review; mod rmcp_client; mod rollout_list_find; +mod search_tool; mod seatbelt; mod shell_command; mod shell_serialization; diff --git a/codex-rs/core/tests/suite/search_tool.rs b/codex-rs/core/tests/suite/search_tool.rs new file mode 100644 index 0000000000..fae5956168 --- /dev/null +++ b/codex-rs/core/tests/suite/search_tool.rs @@ -0,0 +1,488 @@ +#![cfg(not(target_os = "windows"))] +#![allow(clippy::unwrap_used, clippy::expect_used)] + +use std::time::Duration; + +use anyhow::Result; +use codex_core::config::types::McpServerConfig; +use codex_core::config::types::McpServerTransportConfig; +use codex_core::features::Feature; +use codex_core::protocol::AskForApproval; +use codex_core::protocol::SandboxPolicy; +use core_test_support::responses::ResponsesRequest; +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_sse_sequence; +use core_test_support::responses::sse; +use core_test_support::responses::start_mock_server; +use core_test_support::skip_if_no_network; +use core_test_support::stdio_server_bin; +use core_test_support::test_codex::test_codex; +use pretty_assertions::assert_eq; +use serde_json::Value; +use serde_json::json; + +const SEARCH_TOOL_INSTRUCTION_SNIPPETS: [&str; 2] = [ + "MCP tools (`mcp__...`) are hidden until you search for them.", + "Matching tools are added to `active_selected_tools`.", +]; + +fn tool_names(body: &Value) -> Vec { + body.get("tools") + .and_then(Value::as_array) + .map(|tools| { + tools + .iter() + .filter_map(|tool| { + tool.get("name") + .or_else(|| tool.get("type")) + .and_then(Value::as_str) + .map(str::to_string) + }) + .collect() + }) + .unwrap_or_default() +} + +fn developer_messages(body: &Value) -> Vec { + body.get("input") + .and_then(Value::as_array) + .map(|items| { + items + .iter() + .filter_map(|item| { + if item.get("role").and_then(Value::as_str) != Some("developer") { + return None; + } + let content = item.get("content").and_then(Value::as_array)?; + let texts: Vec<&str> = content + .iter() + .filter_map(|entry| entry.get("text").and_then(Value::as_str)) + .collect(); + if texts.is_empty() { + None + } else { + Some(texts.join("\n")) + } + }) + .collect() + }) + .unwrap_or_default() +} + +fn search_tool_output_payload(request: &ResponsesRequest, call_id: &str) -> Value { + let (content, _success) = request + .function_call_output_content_and_success(call_id) + .expect("search_tool_bm25 function_call_output should be present"); + let content = content.expect("search_tool_bm25 output should include content"); + serde_json::from_str(&content).expect("search_tool_bm25 content should be valid JSON") +} + +fn active_selected_tools(payload: &Value) -> Vec { + payload + .get("active_selected_tools") + .and_then(Value::as_array) + .expect("active_selected_tools should be an array") + .iter() + .map(|value| { + value + .as_str() + .expect("active_selected_tools entries should be strings") + .to_string() + }) + .collect() +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn search_tool_flag_adds_tool() -> Result<()> { + skip_if_no_network!(Ok(())); + + let server = start_mock_server().await; + let mock = mount_sse_sequence( + &server, + vec![sse(vec![ + ev_response_created("resp-1"), + ev_assistant_message("msg-1", "done"), + ev_completed("resp-1"), + ])], + ) + .await; + + let mut builder = test_codex().with_config(|config| { + config.features.enable(Feature::SearchTool); + }); + let test = builder.build(&server).await?; + + test.submit_turn_with_policies( + "list tools", + AskForApproval::Never, + SandboxPolicy::DangerFullAccess, + ) + .await?; + + let body = mock.single_request().body_json(); + let tools = tool_names(&body); + assert!( + tools.iter().any(|name| name == "search_tool_bm25"), + "tools list should include search_tool_bm25 when enabled: {tools:?}" + ); + + Ok(()) +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn search_tool_adds_developer_instructions() -> Result<()> { + skip_if_no_network!(Ok(())); + + let server = start_mock_server().await; + let mock = mount_sse_sequence( + &server, + vec![sse(vec![ + ev_response_created("resp-1"), + ev_assistant_message("msg-1", "done"), + ev_completed("resp-1"), + ])], + ) + .await; + + let mut builder = test_codex().with_config(|config| { + config.features.enable(Feature::SearchTool); + }); + let test = builder.build(&server).await?; + + test.submit_turn_with_policies( + "list tools", + AskForApproval::Never, + SandboxPolicy::DangerFullAccess, + ) + .await?; + + let body = mock.single_request().body_json(); + let developer_texts = developer_messages(&body); + assert!( + developer_texts.iter().any(|text| { + SEARCH_TOOL_INSTRUCTION_SNIPPETS + .iter() + .all(|snippet| text.contains(snippet)) + }), + "developer instructions should include search tool workflow: {developer_texts:?}" + ); + + Ok(()) +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn search_tool_hides_mcp_tools_without_search() -> Result<()> { + skip_if_no_network!(Ok(())); + + let server = start_mock_server().await; + let mock = mount_sse_sequence( + &server, + vec![sse(vec![ + ev_response_created("resp-1"), + ev_assistant_message("msg-1", "done"), + ev_completed("resp-1"), + ])], + ) + .await; + + let rmcp_test_server_bin = stdio_server_bin()?; + let mut builder = test_codex().with_config(move |config| { + config.features.enable(Feature::SearchTool); + let mut servers = config.mcp_servers.get().clone(); + servers.insert( + "rmcp".to_string(), + McpServerConfig { + transport: McpServerTransportConfig::Stdio { + command: rmcp_test_server_bin, + args: Vec::new(), + env: None, + env_vars: Vec::new(), + cwd: None, + }, + enabled: true, + required: false, + disabled_reason: None, + startup_timeout_sec: Some(Duration::from_secs(10)), + tool_timeout_sec: None, + enabled_tools: None, + disabled_tools: None, + scopes: None, + }, + ); + config + .mcp_servers + .set(servers) + .expect("test mcp servers should accept any configuration"); + }); + let test = builder.build(&server).await?; + + test.submit_turn_with_policies( + "hello tools", + AskForApproval::Never, + SandboxPolicy::DangerFullAccess, + ) + .await?; + + let body = mock.single_request().body_json(); + let tools = tool_names(&body); + assert!( + tools.iter().any(|name| name == "search_tool_bm25"), + "tools list should include search_tool_bm25 when enabled: {tools:?}" + ); + assert!( + !tools.iter().any(|name| name == "mcp__rmcp__echo"), + "tools list should not include MCP tools before search: {tools:?}" + ); + assert!( + !tools.iter().any(|name| name == "mcp__rmcp__image"), + "tools list should not include MCP tools before search: {tools:?}" + ); + + Ok(()) +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn search_tool_selection_persists_within_turn_and_resets_next_turn() -> Result<()> { + skip_if_no_network!(Ok(())); + + let server = start_mock_server().await; + let call_id = "tool-search"; + let args = json!({ + "query": "echo", + "limit": 1, + }); + let responses = vec![ + sse(vec![ + ev_response_created("resp-1"), + ev_function_call(call_id, "search_tool_bm25", &serde_json::to_string(&args)?), + ev_completed("resp-1"), + ]), + sse(vec![ + ev_assistant_message("msg-1", "done"), + ev_completed("resp-2"), + ]), + sse(vec![ + ev_assistant_message("msg-2", "done again"), + ev_completed("resp-3"), + ]), + ]; + let mock = mount_sse_sequence(&server, responses).await; + + let rmcp_test_server_bin = stdio_server_bin()?; + let mut builder = test_codex().with_config(move |config| { + config.features.enable(Feature::SearchTool); + let mut servers = config.mcp_servers.get().clone(); + servers.insert( + "rmcp".to_string(), + McpServerConfig { + transport: McpServerTransportConfig::Stdio { + command: rmcp_test_server_bin, + args: Vec::new(), + env: None, + env_vars: Vec::new(), + cwd: None, + }, + enabled: true, + required: false, + disabled_reason: None, + startup_timeout_sec: Some(Duration::from_secs(10)), + tool_timeout_sec: None, + enabled_tools: None, + disabled_tools: None, + scopes: None, + }, + ); + config + .mcp_servers + .set(servers) + .expect("test mcp servers should accept any configuration"); + }); + let test = builder.build(&server).await?; + + test.submit_turn_with_policies( + "find the echo tool", + AskForApproval::Never, + SandboxPolicy::DangerFullAccess, + ) + .await?; + test.submit_turn_with_policies( + "hello again", + AskForApproval::Never, + SandboxPolicy::DangerFullAccess, + ) + .await?; + + let requests = mock.requests(); + assert_eq!( + requests.len(), + 3, + "expected 3 requests, got {}", + requests.len() + ); + + let first_tools = tool_names(&requests[0].body_json()); + assert!( + !first_tools.iter().any(|name| name == "mcp__rmcp__echo"), + "first request should not include MCP tools before search: {first_tools:?}" + ); + + let second_tools = tool_names(&requests[1].body_json()); + assert!( + second_tools.iter().any(|name| name == "mcp__rmcp__echo"), + "second request should include selected MCP tool: {second_tools:?}" + ); + assert!( + !second_tools.iter().any(|name| name == "mcp__rmcp__image"), + "second request should only include selected MCP tool: {second_tools:?}" + ); + + let search_output_payload = search_tool_output_payload(&requests[1], call_id); + assert!( + search_output_payload.get("selected_tools").is_none(), + "selected_tools should not be returned: {search_output_payload:?}" + ); + assert_eq!( + active_selected_tools(&search_output_payload), + vec!["mcp__rmcp__echo".to_string()], + ); + + let third_tools = tool_names(&requests[2].body_json()); + assert!( + !third_tools.iter().any(|name| name == "mcp__rmcp__echo"), + "third request should not include MCP tools after turn reset: {third_tools:?}" + ); + + Ok(()) +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn search_tool_selection_unions_results_within_turn() -> Result<()> { + skip_if_no_network!(Ok(())); + + let server = start_mock_server().await; + let first_call_id = "tool-search-echo"; + let second_call_id = "tool-search-image"; + let first_args = json!({ + "query": "echo", + "limit": 1, + }); + let second_args = json!({ + "query": "image", + "limit": 1, + }); + let responses = vec![ + sse(vec![ + ev_response_created("resp-1"), + ev_function_call( + first_call_id, + "search_tool_bm25", + &serde_json::to_string(&first_args)?, + ), + ev_completed("resp-1"), + ]), + sse(vec![ + ev_response_created("resp-2"), + ev_function_call( + second_call_id, + "search_tool_bm25", + &serde_json::to_string(&second_args)?, + ), + ev_completed("resp-2"), + ]), + sse(vec![ + ev_assistant_message("msg-1", "done"), + ev_completed("resp-3"), + ]), + ]; + let mock = mount_sse_sequence(&server, responses).await; + + let rmcp_test_server_bin = stdio_server_bin()?; + let mut builder = test_codex().with_config(move |config| { + config.features.enable(Feature::SearchTool); + let mut servers = config.mcp_servers.get().clone(); + servers.insert( + "rmcp".to_string(), + McpServerConfig { + transport: McpServerTransportConfig::Stdio { + command: rmcp_test_server_bin, + args: Vec::new(), + env: None, + env_vars: Vec::new(), + cwd: None, + }, + enabled: true, + required: false, + disabled_reason: None, + startup_timeout_sec: Some(Duration::from_secs(10)), + tool_timeout_sec: None, + enabled_tools: None, + disabled_tools: None, + scopes: None, + }, + ); + config + .mcp_servers + .set(servers) + .expect("test mcp servers should accept any configuration"); + }); + let test = builder.build(&server).await?; + + test.submit_turn_with_policies( + "find echo and image tools", + AskForApproval::Never, + SandboxPolicy::DangerFullAccess, + ) + .await?; + + let requests = mock.requests(); + assert_eq!( + requests.len(), + 3, + "expected 3 requests, got {}", + requests.len() + ); + + let first_tools = tool_names(&requests[0].body_json()); + assert!( + !first_tools.iter().any(|name| name == "mcp__rmcp__echo"), + "first request should not include MCP tools before search: {first_tools:?}" + ); + + let second_tools = tool_names(&requests[1].body_json()); + assert!( + second_tools.iter().any(|name| name == "mcp__rmcp__echo"), + "second request should include echo after first search: {second_tools:?}" + ); + assert!( + !second_tools.iter().any(|name| name == "mcp__rmcp__image"), + "second request should not include image before second search runs: {second_tools:?}" + ); + + let third_tools = tool_names(&requests[2].body_json()); + assert!( + third_tools.iter().any(|name| name == "mcp__rmcp__echo"), + "third request should still include echo: {third_tools:?}" + ); + assert!( + third_tools.iter().any(|name| name == "mcp__rmcp__image"), + "third request should include image after second search: {third_tools:?}" + ); + + let second_search_payload = search_tool_output_payload(&requests[2], second_call_id); + assert!( + second_search_payload.get("selected_tools").is_none(), + "selected_tools should not be returned: {second_search_payload:?}" + ); + assert_eq!( + active_selected_tools(&second_search_payload), + vec![ + "mcp__rmcp__echo".to_string(), + "mcp__rmcp__image".to_string(), + ], + ); + + Ok(()) +} From 74ecd6e3b23f2b43fd5e455a4c3de4e545557d14 Mon Sep 17 00:00:00 2001 From: jif-oai Date: Mon, 9 Feb 2026 21:04:20 +0000 Subject: [PATCH 08/23] state: add memory consolidation lock primitives (#11199) ## Summary - add a migration for memory_consolidation_locks - add acquire/release lock primitives to codex-state runtime - add core/state_db wrappers and cwd normalization for memory queries and lock keys ## Testing - cargo test -p codex-state memory_consolidation_lock_ - cargo test -p codex-core --lib state_db:: --- codex-rs/core/src/state_db.rs | 57 ++++++- .../0009_memory_consolidation_locks.sql | 8 + codex-rs/state/src/runtime.rs | 143 ++++++++++++++++++ 3 files changed, 207 insertions(+), 1 deletion(-) create mode 100644 codex-rs/state/migrations/0009_memory_consolidation_locks.sql diff --git a/codex-rs/core/src/state_db.rs b/codex-rs/core/src/state_db.rs index b1b53bcbc1..3707be77c7 100644 --- a/codex-rs/core/src/state_db.rs +++ b/codex-rs/core/src/state_db.rs @@ -1,5 +1,6 @@ use crate::config::Config; use crate::features::Feature; +use crate::path_utils::normalize_for_path_comparison; use crate::rollout::list::Cursor; use crate::rollout::list::ThreadSortKey; use crate::rollout::metadata; @@ -156,6 +157,10 @@ fn cursor_to_anchor(cursor: Option<&Cursor>) -> Option { Some(codex_state::Anchor { ts, id }) } +fn normalize_cwd_for_state_db(cwd: &Path) -> PathBuf { + normalize_for_path_comparison(cwd).unwrap_or_else(|_| cwd.to_path_buf()) +} + /// List thread ids from SQLite for parity checks without rollout scanning. #[allow(clippy::too_many_arguments)] pub async fn list_thread_ids_db( @@ -355,7 +360,11 @@ pub async fn get_last_n_thread_memories_for_cwd( stage: &str, ) -> Option> { let ctx = context?; - match ctx.get_last_n_thread_memories_for_cwd(cwd, n).await { + let normalized_cwd = normalize_cwd_for_state_db(cwd); + match ctx + .get_last_n_thread_memories_for_cwd(&normalized_cwd, n) + .await + { Ok(memories) => Some(memories), Err(err) => { warn!("state db get_last_n_thread_memories_for_cwd failed during {stage}: {err}"); @@ -364,6 +373,49 @@ pub async fn get_last_n_thread_memories_for_cwd( } } +/// Try to acquire or renew a per-cwd memory consolidation lock. +pub async fn try_acquire_memory_consolidation_lock( + context: Option<&codex_state::StateRuntime>, + cwd: &Path, + working_thread_id: ThreadId, + lease_seconds: i64, + stage: &str, +) -> Option { + let ctx = context?; + let normalized_cwd = normalize_cwd_for_state_db(cwd); + match ctx + .try_acquire_memory_consolidation_lock(&normalized_cwd, working_thread_id, lease_seconds) + .await + { + Ok(acquired) => Some(acquired), + Err(err) => { + warn!("state db try_acquire_memory_consolidation_lock failed during {stage}: {err}"); + None + } + } +} + +/// Release a per-cwd memory consolidation lock if held by `working_thread_id`. +pub async fn release_memory_consolidation_lock( + context: Option<&codex_state::StateRuntime>, + cwd: &Path, + working_thread_id: ThreadId, + stage: &str, +) -> Option { + let ctx = context?; + let normalized_cwd = normalize_cwd_for_state_db(cwd); + match ctx + .release_memory_consolidation_lock(&normalized_cwd, working_thread_id) + .await + { + Ok(released) => Some(released), + Err(err) => { + warn!("state db release_memory_consolidation_lock failed during {stage}: {err}"); + None + } + } +} + /// Reconcile rollout items into SQLite, falling back to scanning the rollout file. pub async fn reconcile_rollout( context: Option<&codex_state::StateRuntime>, @@ -400,6 +452,7 @@ pub async fn reconcile_rollout( } }; let mut metadata = outcome.metadata; + metadata.cwd = normalize_cwd_for_state_db(&metadata.cwd); match archived_only { Some(true) if metadata.archived_at.is_none() => { metadata.archived_at = Some(metadata.updated_at); @@ -447,6 +500,7 @@ pub async fn read_repair_rollout_path( && let Ok(Some(mut metadata)) = ctx.get_thread(thread_id).await { metadata.rollout_path = rollout_path.to_path_buf(); + metadata.cwd = normalize_cwd_for_state_db(&metadata.cwd); match archived_only { Some(true) if metadata.archived_at.is_none() => { metadata.archived_at = Some(metadata.updated_at); @@ -509,6 +563,7 @@ pub async fn apply_rollout_items( }, }; builder.rollout_path = rollout_path.to_path_buf(); + builder.cwd = normalize_cwd_for_state_db(&builder.cwd); if let Err(err) = ctx.apply_rollout_items(&builder, items, None).await { warn!( "state db apply_rollout_items failed during {stage} for {}: {err}", diff --git a/codex-rs/state/migrations/0009_memory_consolidation_locks.sql b/codex-rs/state/migrations/0009_memory_consolidation_locks.sql new file mode 100644 index 0000000000..793ab4dc3b --- /dev/null +++ b/codex-rs/state/migrations/0009_memory_consolidation_locks.sql @@ -0,0 +1,8 @@ +CREATE TABLE memory_consolidation_locks ( + cwd TEXT PRIMARY KEY, + working_thread_id TEXT NOT NULL, + updated_at INTEGER NOT NULL +); + +CREATE INDEX idx_memory_consolidation_locks_updated_at + ON memory_consolidation_locks(updated_at DESC); diff --git a/codex-rs/state/src/runtime.rs b/codex-rs/state/src/runtime.rs index 6e6fb90017..980443d5b1 100644 --- a/codex-rs/state/src/runtime.rs +++ b/codex-rs/state/src/runtime.rs @@ -583,6 +583,64 @@ LIMIT ? .collect() } + /// Try to acquire or renew the per-cwd memory consolidation lock. + /// + /// Returns `true` when the lock is acquired/renewed for `working_thread_id`. + /// Returns `false` when another owner holds a non-expired lease. + pub async fn try_acquire_memory_consolidation_lock( + &self, + cwd: &Path, + working_thread_id: ThreadId, + lease_seconds: i64, + ) -> anyhow::Result { + let now = Utc::now().timestamp(); + let stale_cutoff = now.saturating_sub(lease_seconds.max(0)); + let result = sqlx::query( + r#" +INSERT INTO memory_consolidation_locks ( + cwd, + working_thread_id, + updated_at +) VALUES (?, ?, ?) +ON CONFLICT(cwd) DO UPDATE SET + working_thread_id = excluded.working_thread_id, + updated_at = excluded.updated_at +WHERE memory_consolidation_locks.working_thread_id = excluded.working_thread_id + OR memory_consolidation_locks.updated_at <= ? + "#, + ) + .bind(cwd.display().to_string()) + .bind(working_thread_id.to_string()) + .bind(now) + .bind(stale_cutoff) + .execute(self.pool.as_ref()) + .await?; + + Ok(result.rows_affected() > 0) + } + + /// Release the per-cwd memory consolidation lock if held by `working_thread_id`. + /// + /// Returns `true` when a lock row was removed. + pub async fn release_memory_consolidation_lock( + &self, + cwd: &Path, + working_thread_id: ThreadId, + ) -> anyhow::Result { + let result = sqlx::query( + r#" +DELETE FROM memory_consolidation_locks +WHERE cwd = ? AND working_thread_id = ? + "#, + ) + .bind(cwd.display().to_string()) + .bind(working_thread_id.to_string()) + .execute(self.pool.as_ref()) + .await?; + + Ok(result.rows_affected() > 0) + } + /// Persist dynamic tools for a thread if none have been stored yet. /// /// Dynamic tools are defined at thread start and should not change afterward. @@ -1328,6 +1386,91 @@ mod tests { let _ = tokio::fs::remove_dir_all(codex_home).await; } + #[tokio::test] + async fn memory_consolidation_lock_enforces_owner_and_release() { + let codex_home = unique_temp_dir(); + let runtime = StateRuntime::init(codex_home.clone(), "test-provider".to_string(), None) + .await + .expect("initialize runtime"); + + let cwd = codex_home.join("workspace"); + let owner_a = ThreadId::from_string(&Uuid::new_v4().to_string()).expect("thread id"); + let owner_b = ThreadId::from_string(&Uuid::new_v4().to_string()).expect("thread id"); + + assert!( + runtime + .try_acquire_memory_consolidation_lock(cwd.as_path(), owner_a, 600) + .await + .expect("acquire for owner_a"), + "owner_a should acquire lock" + ); + assert!( + !runtime + .try_acquire_memory_consolidation_lock(cwd.as_path(), owner_b, 600) + .await + .expect("acquire for owner_b should fail"), + "owner_b should not steal active lock" + ); + assert!( + runtime + .try_acquire_memory_consolidation_lock(cwd.as_path(), owner_a, 600) + .await + .expect("owner_a should renew lock"), + "owner_a should renew lock" + ); + assert!( + !runtime + .release_memory_consolidation_lock(cwd.as_path(), owner_b) + .await + .expect("owner_b release should be no-op"), + "non-owner release should not remove lock" + ); + assert!( + runtime + .release_memory_consolidation_lock(cwd.as_path(), owner_a) + .await + .expect("owner_a release"), + "owner_a should release lock" + ); + assert!( + runtime + .try_acquire_memory_consolidation_lock(cwd.as_path(), owner_b, 600) + .await + .expect("owner_b acquire after release"), + "owner_b should acquire released lock" + ); + + let _ = tokio::fs::remove_dir_all(codex_home).await; + } + + #[tokio::test] + async fn memory_consolidation_lock_can_be_stolen_when_lease_expired() { + let codex_home = unique_temp_dir(); + let runtime = StateRuntime::init(codex_home.clone(), "test-provider".to_string(), None) + .await + .expect("initialize runtime"); + + let cwd = codex_home.join("workspace"); + let owner_a = ThreadId::from_string(&Uuid::new_v4().to_string()).expect("thread id"); + let owner_b = ThreadId::from_string(&Uuid::new_v4().to_string()).expect("thread id"); + + assert!( + runtime + .try_acquire_memory_consolidation_lock(cwd.as_path(), owner_a, 600) + .await + .expect("owner_a acquire") + ); + assert!( + runtime + .try_acquire_memory_consolidation_lock(cwd.as_path(), owner_b, 0) + .await + .expect("owner_b steal with expired lease"), + "owner_b should steal lock when lease cutoff marks previous lock stale" + ); + + let _ = tokio::fs::remove_dir_all(codex_home).await; + } + #[tokio::test] async fn deleting_thread_cascades_thread_memory() { let codex_home = unique_temp_dir(); From a33ee46e3b506efcc3947088bd4433abbe93d030 Mon Sep 17 00:00:00 2001 From: xl-openai Date: Mon, 9 Feb 2026 13:30:38 -0800 Subject: [PATCH 09/23] feat: extend skills/list to support additional roots. (#10835) Add an optional perCwdExtraUserRoots --- .../schema/json/ClientRequest.json | 29 +++ .../codex_app_server_protocol.schemas.json | 29 +++ .../schema/json/v2/SkillsListParams.json | 31 +++ .../v2/SkillsListExtraRootsForCwd.ts | 5 + .../schema/typescript/v2/SkillsListParams.ts | 7 +- .../schema/typescript/v2/index.ts | 1 + .../app-server-protocol/src/protocol/v2.rs | 31 ++- codex-rs/app-server/README.md | 13 +- .../app-server/src/codex_message_processor.rs | 45 +++- .../app-server/tests/common/mcp_process.rs | 10 + codex-rs/app-server/tests/suite/v2/mod.rs | 1 + .../app-server/tests/suite/v2/skills_list.rs | 216 ++++++++++++++++++ codex-rs/core/src/skills/manager.rs | 197 +++++++++++++++- 13 files changed, 598 insertions(+), 17 deletions(-) create mode 100644 codex-rs/app-server-protocol/schema/typescript/v2/SkillsListExtraRootsForCwd.ts create mode 100644 codex-rs/app-server/tests/suite/v2/skills_list.rs diff --git a/codex-rs/app-server-protocol/schema/json/ClientRequest.json b/codex-rs/app-server-protocol/schema/json/ClientRequest.json index 1f19f8f558..c981dfda62 100644 --- a/codex-rs/app-server-protocol/schema/json/ClientRequest.json +++ b/codex-rs/app-server-protocol/schema/json/ClientRequest.json @@ -2210,6 +2210,24 @@ ], "type": "object" }, + "SkillsListExtraRootsForCwd": { + "properties": { + "cwd": { + "type": "string" + }, + "extraUserRoots": { + "items": { + "type": "string" + }, + "type": "array" + } + }, + "required": [ + "cwd", + "extraUserRoots" + ], + "type": "object" + }, "SkillsListParams": { "properties": { "cwds": { @@ -2222,6 +2240,17 @@ "forceReload": { "description": "When true, bypass the skills cache and re-scan skills from disk.", "type": "boolean" + }, + "perCwdExtraUserRoots": { + "default": null, + "description": "Optional per-cwd extra roots to scan as user-scoped skills.", + "items": { + "$ref": "#/definitions/SkillsListExtraRootsForCwd" + }, + "type": [ + "array", + "null" + ] } }, "type": "object" diff --git a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json index 6f36a9a897..6f57754dd6 100644 --- a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json +++ b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json @@ -14079,6 +14079,24 @@ ], "type": "object" }, + "SkillsListExtraRootsForCwd": { + "properties": { + "cwd": { + "type": "string" + }, + "extraUserRoots": { + "items": { + "type": "string" + }, + "type": "array" + } + }, + "required": [ + "cwd", + "extraUserRoots" + ], + "type": "object" + }, "SkillsListParams": { "$schema": "http://json-schema.org/draft-07/schema#", "properties": { @@ -14092,6 +14110,17 @@ "forceReload": { "description": "When true, bypass the skills cache and re-scan skills from disk.", "type": "boolean" + }, + "perCwdExtraUserRoots": { + "default": null, + "description": "Optional per-cwd extra roots to scan as user-scoped skills.", + "items": { + "$ref": "#/definitions/v2/SkillsListExtraRootsForCwd" + }, + "type": [ + "array", + "null" + ] } }, "title": "SkillsListParams", diff --git a/codex-rs/app-server-protocol/schema/json/v2/SkillsListParams.json b/codex-rs/app-server-protocol/schema/json/v2/SkillsListParams.json index a9a8a9ef8d..77d12e9175 100644 --- a/codex-rs/app-server-protocol/schema/json/v2/SkillsListParams.json +++ b/codex-rs/app-server-protocol/schema/json/v2/SkillsListParams.json @@ -1,5 +1,25 @@ { "$schema": "http://json-schema.org/draft-07/schema#", + "definitions": { + "SkillsListExtraRootsForCwd": { + "properties": { + "cwd": { + "type": "string" + }, + "extraUserRoots": { + "items": { + "type": "string" + }, + "type": "array" + } + }, + "required": [ + "cwd", + "extraUserRoots" + ], + "type": "object" + } + }, "properties": { "cwds": { "description": "When empty, defaults to the current session working directory.", @@ -11,6 +31,17 @@ "forceReload": { "description": "When true, bypass the skills cache and re-scan skills from disk.", "type": "boolean" + }, + "perCwdExtraUserRoots": { + "default": null, + "description": "Optional per-cwd extra roots to scan as user-scoped skills.", + "items": { + "$ref": "#/definitions/SkillsListExtraRootsForCwd" + }, + "type": [ + "array", + "null" + ] } }, "title": "SkillsListParams", diff --git a/codex-rs/app-server-protocol/schema/typescript/v2/SkillsListExtraRootsForCwd.ts b/codex-rs/app-server-protocol/schema/typescript/v2/SkillsListExtraRootsForCwd.ts new file mode 100644 index 0000000000..c18cd4ba1d --- /dev/null +++ b/codex-rs/app-server-protocol/schema/typescript/v2/SkillsListExtraRootsForCwd.ts @@ -0,0 +1,5 @@ +// GENERATED CODE! DO NOT MODIFY BY HAND! + +// This file was generated by [ts-rs](https://github.com/Aleph-Alpha/ts-rs). Do not edit this file manually. + +export type SkillsListExtraRootsForCwd = { cwd: string, extraUserRoots: Array, }; diff --git a/codex-rs/app-server-protocol/schema/typescript/v2/SkillsListParams.ts b/codex-rs/app-server-protocol/schema/typescript/v2/SkillsListParams.ts index d44e6551fd..f584443810 100644 --- a/codex-rs/app-server-protocol/schema/typescript/v2/SkillsListParams.ts +++ b/codex-rs/app-server-protocol/schema/typescript/v2/SkillsListParams.ts @@ -1,6 +1,7 @@ // GENERATED CODE! DO NOT MODIFY BY HAND! // This file was generated by [ts-rs](https://github.com/Aleph-Alpha/ts-rs). Do not edit this file manually. +import type { SkillsListExtraRootsForCwd } from "./SkillsListExtraRootsForCwd"; export type SkillsListParams = { /** @@ -10,4 +11,8 @@ cwds?: Array, /** * When true, bypass the skills cache and re-scan skills from disk. */ -forceReload?: boolean, }; +forceReload?: boolean, +/** + * Optional per-cwd extra roots to scan as user-scoped skills. + */ +perCwdExtraUserRoots?: Array | null, }; diff --git a/codex-rs/app-server-protocol/schema/typescript/v2/index.ts b/codex-rs/app-server-protocol/schema/typescript/v2/index.ts index 05e8322e44..f54d94e7f5 100644 --- a/codex-rs/app-server-protocol/schema/typescript/v2/index.ts +++ b/codex-rs/app-server-protocol/schema/typescript/v2/index.ts @@ -124,6 +124,7 @@ export type { SkillToolDependency } from "./SkillToolDependency"; export type { SkillsConfigWriteParams } from "./SkillsConfigWriteParams"; export type { SkillsConfigWriteResponse } from "./SkillsConfigWriteResponse"; export type { SkillsListEntry } from "./SkillsListEntry"; +export type { SkillsListExtraRootsForCwd } from "./SkillsListExtraRootsForCwd"; export type { SkillsListParams } from "./SkillsListParams"; export type { SkillsListResponse } from "./SkillsListResponse"; export type { SkillsRemoteReadParams } from "./SkillsRemoteReadParams"; diff --git a/codex-rs/app-server-protocol/src/protocol/v2.rs b/codex-rs/app-server-protocol/src/protocol/v2.rs index cdd0b4bcf9..a75bdaab3d 100644 --- a/codex-rs/app-server-protocol/src/protocol/v2.rs +++ b/codex-rs/app-server-protocol/src/protocol/v2.rs @@ -1704,6 +1704,19 @@ pub struct SkillsListParams { /// When true, bypass the skills cache and re-scan skills from disk. #[serde(default, skip_serializing_if = "std::ops::Not::not")] pub force_reload: bool, + + /// Optional per-cwd extra roots to scan as user-scoped skills. + #[serde(default)] + #[ts(optional = nullable)] + pub per_cwd_extra_user_roots: Option>, +} + +#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema, TS)] +#[serde(rename_all = "camelCase")] +#[ts(export_to = "v2/")] +pub struct SkillsListExtraRootsForCwd { + pub cwd: PathBuf, + pub extra_user_roots: Vec, } #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema, TS)] @@ -3352,20 +3365,36 @@ mod tests { serde_json::to_value(SkillsListParams { cwds: Vec::new(), force_reload: false, + per_cwd_extra_user_roots: None, }) .unwrap(), - json!({}), + json!({ + "perCwdExtraUserRoots": null, + }), ); assert_eq!( serde_json::to_value(SkillsListParams { cwds: vec![PathBuf::from("/repo")], force_reload: true, + per_cwd_extra_user_roots: Some(vec![SkillsListExtraRootsForCwd { + cwd: PathBuf::from("/repo"), + extra_user_roots: vec![ + PathBuf::from("/shared/skills"), + PathBuf::from("/tmp/x") + ], + }]), }) .unwrap(), json!({ "cwds": ["/repo"], "forceReload": true, + "perCwdExtraUserRoots": [ + { + "cwd": "/repo", + "extraUserRoots": ["/shared/skills", "/tmp/x"], + } + ], }), ); } diff --git a/codex-rs/app-server/README.md b/codex-rs/app-server/README.md index 8946880f95..04a254a60a 100644 --- a/codex-rs/app-server/README.md +++ b/codex-rs/app-server/README.md @@ -663,11 +663,20 @@ $skill-creator Add a new skill for triaging flaky CI and include step-by-step us ``` Use `skills/list` to fetch the available skills (optionally scoped by `cwds`, with `forceReload`). +You can also add `perCwdExtraUserRoots` to scan additional absolute paths as `user` scope for specific `cwd` entries. +Entries whose `cwd` is not present in `cwds` are ignored. +`skills/list` might reuse a cached skills result per `cwd`; setting `forceReload` to `true` refreshes the result from disk. ```json { "method": "skills/list", "id": 25, "params": { - "cwds": ["/Users/me/project"], - "forceReload": false + "cwds": ["/Users/me/project", "/Users/me/other-project"], + "forceReload": true, + "perCwdExtraUserRoots": [ + { + "cwd": "/Users/me/project", + "extraUserRoots": ["/Users/me/shared-skills"] + } + ] } } { "id": 25, "result": { "data": [{ diff --git a/codex-rs/app-server/src/codex_message_processor.rs b/codex-rs/app-server/src/codex_message_processor.rs index e1bfe3a337..0c24ed30c0 100644 --- a/codex-rs/app-server/src/codex_message_processor.rs +++ b/codex-rs/app-server/src/codex_message_processor.rs @@ -4574,17 +4574,58 @@ impl CodexMessageProcessor { } async fn skills_list(&self, request_id: ConnectionRequestId, params: SkillsListParams) { - let SkillsListParams { cwds, force_reload } = params; + let SkillsListParams { + cwds, + force_reload, + per_cwd_extra_user_roots, + } = params; let cwds = if cwds.is_empty() { vec![self.config.cwd.clone()] } else { cwds }; + let cwd_set: HashSet = cwds.iter().cloned().collect(); + + let mut extra_roots_by_cwd: HashMap> = HashMap::new(); + for entry in per_cwd_extra_user_roots.unwrap_or_default() { + if !cwd_set.contains(&entry.cwd) { + warn!( + cwd = %entry.cwd.display(), + "ignoring per-cwd extra roots for cwd not present in skills/list cwds" + ); + continue; + } + + let mut valid_extra_roots = Vec::new(); + for root in entry.extra_user_roots { + if !root.is_absolute() { + self.send_invalid_request_error( + request_id, + format!( + "skills/list perCwdExtraUserRoots extraUserRoots paths must be absolute: {}", + root.display() + ), + ) + .await; + return; + } + valid_extra_roots.push(root); + } + extra_roots_by_cwd + .entry(entry.cwd) + .or_default() + .extend(valid_extra_roots); + } let skills_manager = self.thread_manager.skills_manager(); let mut data = Vec::new(); for cwd in cwds { - let outcome = skills_manager.skills_for_cwd(&cwd, force_reload).await; + let extra_roots = extra_roots_by_cwd + .get(&cwd) + .map_or(&[][..], std::vec::Vec::as_slice); + let outcome = skills_manager + .skills_for_cwd_with_extra_user_roots(&cwd, force_reload, extra_roots) + .await; let errors = errors_to_info(&outcome.errors); let skills = skills_to_info(&outcome.skills, &outcome.disabled_paths); data.push(codex_app_server_protocol::SkillsListEntry { diff --git a/codex-rs/app-server/tests/common/mcp_process.rs b/codex-rs/app-server/tests/common/mcp_process.rs index 57c29fcf9f..5e2d7fa42b 100644 --- a/codex-rs/app-server/tests/common/mcp_process.rs +++ b/codex-rs/app-server/tests/common/mcp_process.rs @@ -50,6 +50,7 @@ use codex_app_server_protocol::SendUserMessageParams; use codex_app_server_protocol::SendUserTurnParams; use codex_app_server_protocol::ServerRequest; use codex_app_server_protocol::SetDefaultModelParams; +use codex_app_server_protocol::SkillsListParams; use codex_app_server_protocol::ThreadArchiveParams; use codex_app_server_protocol::ThreadCompactStartParams; use codex_app_server_protocol::ThreadForkParams; @@ -490,6 +491,15 @@ impl McpProcess { self.send_request("app/list", params).await } + /// Send a `skills/list` JSON-RPC request. + pub async fn send_skills_list_request( + &mut self, + params: SkillsListParams, + ) -> anyhow::Result { + let params = Some(serde_json::to_value(params)?); + self.send_request("skills/list", params).await + } + /// Send a `collaborationMode/list` JSON-RPC request. pub async fn send_list_collaboration_modes_request( &mut self, diff --git a/codex-rs/app-server/tests/suite/v2/mod.rs b/codex-rs/app-server/tests/suite/v2/mod.rs index c25a6d0568..48622acddb 100644 --- a/codex-rs/app-server/tests/suite/v2/mod.rs +++ b/codex-rs/app-server/tests/suite/v2/mod.rs @@ -15,6 +15,7 @@ mod plan_item; mod rate_limits; mod request_user_input; mod review; +mod skills_list; mod thread_archive; mod thread_fork; mod thread_list; diff --git a/codex-rs/app-server/tests/suite/v2/skills_list.rs b/codex-rs/app-server/tests/suite/v2/skills_list.rs new file mode 100644 index 0000000000..e099bdb646 --- /dev/null +++ b/codex-rs/app-server/tests/suite/v2/skills_list.rs @@ -0,0 +1,216 @@ +use std::time::Duration; + +use anyhow::Result; +use app_test_support::McpProcess; +use app_test_support::to_response; +use codex_app_server_protocol::JSONRPCResponse; +use codex_app_server_protocol::RequestId; +use codex_app_server_protocol::SkillsListExtraRootsForCwd; +use codex_app_server_protocol::SkillsListParams; +use codex_app_server_protocol::SkillsListResponse; +use pretty_assertions::assert_eq; +use tempfile::TempDir; +use tokio::time::timeout; + +const DEFAULT_TIMEOUT: Duration = Duration::from_secs(10); + +fn write_skill(root: &TempDir, name: &str) -> Result<()> { + let skill_dir = root.path().join("skills").join(name); + std::fs::create_dir_all(&skill_dir)?; + let content = format!("---\nname: {name}\ndescription: {name} description\n---\n\n# Body\n"); + std::fs::write(skill_dir.join("SKILL.md"), content)?; + Ok(()) +} + +#[tokio::test] +async fn skills_list_includes_skills_from_per_cwd_extra_user_roots() -> Result<()> { + let codex_home = TempDir::new()?; + let cwd = TempDir::new()?; + let extra_root = TempDir::new()?; + write_skill(&extra_root, "extra-skill")?; + + let mut mcp = McpProcess::new(codex_home.path()).await?; + timeout(DEFAULT_TIMEOUT, mcp.initialize()).await??; + + let request_id = mcp + .send_skills_list_request(SkillsListParams { + cwds: vec![cwd.path().to_path_buf()], + force_reload: true, + per_cwd_extra_user_roots: Some(vec![SkillsListExtraRootsForCwd { + cwd: cwd.path().to_path_buf(), + extra_user_roots: vec![extra_root.path().to_path_buf()], + }]), + }) + .await?; + + let response: JSONRPCResponse = timeout( + DEFAULT_TIMEOUT, + mcp.read_stream_until_response_message(RequestId::Integer(request_id)), + ) + .await??; + let SkillsListResponse { data } = to_response(response)?; + assert_eq!(data.len(), 1); + assert_eq!(data[0].cwd, cwd.path().to_path_buf()); + assert!( + data[0] + .skills + .iter() + .any(|skill| skill.name == "extra-skill") + ); + Ok(()) +} + +#[tokio::test] +async fn skills_list_rejects_relative_extra_user_roots() -> Result<()> { + let codex_home = TempDir::new()?; + let cwd = TempDir::new()?; + + let mut mcp = McpProcess::new(codex_home.path()).await?; + timeout(DEFAULT_TIMEOUT, mcp.initialize()).await??; + + let request_id = mcp + .send_skills_list_request(SkillsListParams { + cwds: vec![cwd.path().to_path_buf()], + force_reload: true, + per_cwd_extra_user_roots: Some(vec![SkillsListExtraRootsForCwd { + cwd: cwd.path().to_path_buf(), + extra_user_roots: vec![std::path::PathBuf::from("relative/skills")], + }]), + }) + .await?; + + let err = timeout( + DEFAULT_TIMEOUT, + mcp.read_stream_until_error_message(RequestId::Integer(request_id)), + ) + .await??; + assert!( + err.error + .message + .contains("perCwdExtraUserRoots extraUserRoots paths must be absolute"), + "unexpected error: {}", + err.error.message + ); + Ok(()) +} + +#[tokio::test] +async fn skills_list_ignores_per_cwd_extra_roots_for_unknown_cwd() -> Result<()> { + let codex_home = TempDir::new()?; + let requested_cwd = TempDir::new()?; + let unknown_cwd = TempDir::new()?; + let extra_root = TempDir::new()?; + write_skill(&extra_root, "ignored-extra-skill")?; + + let mut mcp = McpProcess::new(codex_home.path()).await?; + timeout(DEFAULT_TIMEOUT, mcp.initialize()).await??; + + let request_id = mcp + .send_skills_list_request(SkillsListParams { + cwds: vec![requested_cwd.path().to_path_buf()], + force_reload: true, + per_cwd_extra_user_roots: Some(vec![SkillsListExtraRootsForCwd { + cwd: unknown_cwd.path().to_path_buf(), + extra_user_roots: vec![extra_root.path().to_path_buf()], + }]), + }) + .await?; + + let response: JSONRPCResponse = timeout( + DEFAULT_TIMEOUT, + mcp.read_stream_until_response_message(RequestId::Integer(request_id)), + ) + .await??; + let SkillsListResponse { data } = to_response(response)?; + assert_eq!(data.len(), 1); + assert_eq!(data[0].cwd, requested_cwd.path().to_path_buf()); + assert!( + data[0] + .skills + .iter() + .all(|skill| skill.name != "ignored-extra-skill") + ); + Ok(()) +} + +#[tokio::test] +async fn skills_list_uses_cached_result_until_force_reload() -> Result<()> { + let codex_home = TempDir::new()?; + let cwd = TempDir::new()?; + let extra_root = TempDir::new()?; + write_skill(&extra_root, "late-extra-skill")?; + + let mut mcp = McpProcess::new(codex_home.path()).await?; + timeout(DEFAULT_TIMEOUT, mcp.initialize()).await??; + + // Seed the cwd cache first without extra roots. + let first_request_id = mcp + .send_skills_list_request(SkillsListParams { + cwds: vec![cwd.path().to_path_buf()], + force_reload: false, + per_cwd_extra_user_roots: None, + }) + .await?; + let first_response: JSONRPCResponse = timeout( + DEFAULT_TIMEOUT, + mcp.read_stream_until_response_message(RequestId::Integer(first_request_id)), + ) + .await??; + let SkillsListResponse { data: first_data } = to_response(first_response)?; + assert_eq!(first_data.len(), 1); + assert!( + first_data[0] + .skills + .iter() + .all(|skill| skill.name != "late-extra-skill") + ); + + let second_request_id = mcp + .send_skills_list_request(SkillsListParams { + cwds: vec![cwd.path().to_path_buf()], + force_reload: false, + per_cwd_extra_user_roots: Some(vec![SkillsListExtraRootsForCwd { + cwd: cwd.path().to_path_buf(), + extra_user_roots: vec![extra_root.path().to_path_buf()], + }]), + }) + .await?; + let second_response: JSONRPCResponse = timeout( + DEFAULT_TIMEOUT, + mcp.read_stream_until_response_message(RequestId::Integer(second_request_id)), + ) + .await??; + let SkillsListResponse { data: second_data } = to_response(second_response)?; + assert_eq!(second_data.len(), 1); + assert!( + second_data[0] + .skills + .iter() + .all(|skill| skill.name != "late-extra-skill") + ); + + let third_request_id = mcp + .send_skills_list_request(SkillsListParams { + cwds: vec![cwd.path().to_path_buf()], + force_reload: true, + per_cwd_extra_user_roots: Some(vec![SkillsListExtraRootsForCwd { + cwd: cwd.path().to_path_buf(), + extra_user_roots: vec![extra_root.path().to_path_buf()], + }]), + }) + .await?; + let third_response: JSONRPCResponse = timeout( + DEFAULT_TIMEOUT, + mcp.read_stream_until_response_message(RequestId::Integer(third_request_id)), + ) + .await??; + let SkillsListResponse { data: third_data } = to_response(third_response)?; + assert_eq!(third_data.len(), 1); + assert!( + third_data[0] + .skills + .iter() + .any(|skill| skill.name == "late-extra-skill") + ); + Ok(()) +} diff --git a/codex-rs/core/src/skills/manager.rs b/codex-rs/core/src/skills/manager.rs index 61dcd98f52..cc6193f769 100644 --- a/codex-rs/core/src/skills/manager.rs +++ b/codex-rs/core/src/skills/manager.rs @@ -4,6 +4,7 @@ use std::path::Path; use std::path::PathBuf; use std::sync::RwLock; +use codex_protocol::protocol::SkillScope; use codex_utils_absolute_path::AbsolutePathBuf; use toml::Value as TomlValue; use tracing::info; @@ -15,6 +16,7 @@ use crate::config_loader::CloudRequirementsLoader; use crate::config_loader::LoaderOverrides; use crate::config_loader::load_config_layers_state; use crate::skills::SkillLoadOutcome; +use crate::skills::loader::SkillRoot; use crate::skills::loader::load_skills_from_roots; use crate::skills::loader::skill_roots_from_layer_stack_with_agents; use crate::skills::system::install_system_skills; @@ -40,11 +42,7 @@ impl SkillsManager { /// loading. This also seeds the per-cwd cache for subsequent lookups. pub fn skills_for_config(&self, config: &Config) -> SkillLoadOutcome { let cwd = &config.cwd; - let cached = match self.cache_by_cwd.read() { - Ok(cache) => cache.get(cwd).cloned(), - Err(err) => err.into_inner().get(cwd).cloned(), - }; - if let Some(outcome) = cached { + if let Some(outcome) = self.cached_outcome_for_cwd(cwd) { return outcome; } @@ -61,14 +59,25 @@ impl SkillsManager { } pub async fn skills_for_cwd(&self, cwd: &Path, force_reload: bool) -> SkillLoadOutcome { - let cached = match self.cache_by_cwd.read() { - Ok(cache) => cache.get(cwd).cloned(), - Err(err) => err.into_inner().get(cwd).cloned(), - }; - if !force_reload && let Some(outcome) = cached { + if !force_reload && let Some(outcome) = self.cached_outcome_for_cwd(cwd) { return outcome; } + self.skills_for_cwd_with_extra_user_roots(cwd, force_reload, &[]) + .await + } + + pub async fn skills_for_cwd_with_extra_user_roots( + &self, + cwd: &Path, + force_reload: bool, + extra_user_roots: &[PathBuf], + ) -> SkillLoadOutcome { + if !force_reload && let Some(outcome) = self.cached_outcome_for_cwd(cwd) { + return outcome; + } + let normalized_extra_user_roots = normalize_extra_user_roots(extra_user_roots); + let cwd_abs = match AbsolutePathBuf::try_from(cwd) { Ok(cwd_abs) => cwd_abs, Err(err) => { @@ -104,7 +113,16 @@ impl SkillsManager { } }; - let roots = skill_roots_from_layer_stack_with_agents(&config_layer_stack, cwd); + let mut roots = skill_roots_from_layer_stack_with_agents(&config_layer_stack, cwd); + roots.extend( + normalized_extra_user_roots + .iter() + .cloned() + .map(|path| SkillRoot { + path, + scope: SkillScope::User, + }), + ); let mut outcome = load_skills_from_roots(roots); outcome.disabled_paths = disabled_paths_from_stack(&config_layer_stack); let mut cache = match self.cache_by_cwd.write() { @@ -124,6 +142,13 @@ impl SkillsManager { cache.clear(); info!("skills cache cleared ({cleared} entries)"); } + + fn cached_outcome_for_cwd(&self, cwd: &Path) -> Option { + match self.cache_by_cwd.read() { + Ok(cache) => cache.get(cwd).cloned(), + Err(err) => err.into_inner().get(cwd).cloned(), + } + } } fn disabled_paths_from_stack( @@ -164,6 +189,16 @@ fn normalize_override_path(path: &Path) -> PathBuf { dunce::canonicalize(path).unwrap_or_else(|_| path.to_path_buf()) } +fn normalize_extra_user_roots(extra_user_roots: &[PathBuf]) -> Vec { + let mut normalized: Vec = extra_user_roots + .iter() + .map(|path| dunce::canonicalize(path).unwrap_or_else(|_| path.clone())) + .collect(); + normalized.sort_unstable(); + normalized.dedup(); + normalized +} + #[cfg(test)] mod tests { use super::*; @@ -171,6 +206,7 @@ mod tests { use crate::config::ConfigOverrides; use pretty_assertions::assert_eq; use std::fs; + use std::path::PathBuf; use tempfile::TempDir; fn write_user_skill(codex_home: &TempDir, dir: &str, name: &str, description: &str) { @@ -211,4 +247,143 @@ mod tests { assert_eq!(outcome2.errors, outcome1.errors); assert_eq!(outcome2.skills, outcome1.skills); } + + #[tokio::test] + async fn skills_for_cwd_reuses_cached_entry_even_when_entry_has_extra_roots() { + let codex_home = tempfile::tempdir().expect("tempdir"); + let cwd = tempfile::tempdir().expect("tempdir"); + let extra_root = tempfile::tempdir().expect("tempdir"); + + let config = ConfigBuilder::default() + .codex_home(codex_home.path().to_path_buf()) + .harness_overrides(ConfigOverrides { + cwd: Some(cwd.path().to_path_buf()), + ..Default::default() + }) + .build() + .await + .expect("defaults for test should always succeed"); + + let skills_manager = SkillsManager::new(codex_home.path().to_path_buf()); + let _ = skills_manager.skills_for_config(&config); + + write_user_skill(&extra_root, "x", "extra-skill", "from extra root"); + let extra_root_path = extra_root.path().to_path_buf(); + let outcome_with_extra = skills_manager + .skills_for_cwd_with_extra_user_roots( + cwd.path(), + true, + std::slice::from_ref(&extra_root_path), + ) + .await; + assert!( + outcome_with_extra + .skills + .iter() + .any(|skill| skill.name == "extra-skill") + ); + + // The cwd-only API returns the current cached entry for this cwd, even when that entry + // was produced with extra roots. + let outcome_without_extra = skills_manager.skills_for_cwd(cwd.path(), false).await; + assert_eq!(outcome_without_extra.skills, outcome_with_extra.skills); + assert_eq!(outcome_without_extra.errors, outcome_with_extra.errors); + } + + #[tokio::test] + async fn skills_for_cwd_with_extra_roots_only_refreshes_on_force_reload() { + let codex_home = tempfile::tempdir().expect("tempdir"); + let cwd = tempfile::tempdir().expect("tempdir"); + let extra_root_a = tempfile::tempdir().expect("tempdir"); + let extra_root_b = tempfile::tempdir().expect("tempdir"); + + let config = ConfigBuilder::default() + .codex_home(codex_home.path().to_path_buf()) + .harness_overrides(ConfigOverrides { + cwd: Some(cwd.path().to_path_buf()), + ..Default::default() + }) + .build() + .await + .expect("defaults for test should always succeed"); + + let skills_manager = SkillsManager::new(codex_home.path().to_path_buf()); + let _ = skills_manager.skills_for_config(&config); + + write_user_skill(&extra_root_a, "x", "extra-skill-a", "from extra root a"); + write_user_skill(&extra_root_b, "x", "extra-skill-b", "from extra root b"); + + let extra_root_a_path = extra_root_a.path().to_path_buf(); + let outcome_a = skills_manager + .skills_for_cwd_with_extra_user_roots( + cwd.path(), + true, + std::slice::from_ref(&extra_root_a_path), + ) + .await; + assert!( + outcome_a + .skills + .iter() + .any(|skill| skill.name == "extra-skill-a") + ); + assert!( + outcome_a + .skills + .iter() + .all(|skill| skill.name != "extra-skill-b") + ); + + let extra_root_b_path = extra_root_b.path().to_path_buf(); + let outcome_b = skills_manager + .skills_for_cwd_with_extra_user_roots( + cwd.path(), + false, + std::slice::from_ref(&extra_root_b_path), + ) + .await; + assert!( + outcome_b + .skills + .iter() + .any(|skill| skill.name == "extra-skill-a") + ); + assert!( + outcome_b + .skills + .iter() + .all(|skill| skill.name != "extra-skill-b") + ); + + let outcome_reloaded = skills_manager + .skills_for_cwd_with_extra_user_roots( + cwd.path(), + true, + std::slice::from_ref(&extra_root_b_path), + ) + .await; + assert!( + outcome_reloaded + .skills + .iter() + .any(|skill| skill.name == "extra-skill-b") + ); + assert!( + outcome_reloaded + .skills + .iter() + .all(|skill| skill.name != "extra-skill-a") + ); + } + + #[test] + fn normalize_extra_user_roots_is_stable_for_equivalent_inputs() { + let a = PathBuf::from("/tmp/a"); + let b = PathBuf::from("/tmp/b"); + + let first = normalize_extra_user_roots(&[a.clone(), b.clone(), a.clone()]); + let second = normalize_extra_user_roots(&[b, a]); + + assert_eq!(first, second); + } } From cca13fb03a00d5b109ae396b2256c32a786339eb Mon Sep 17 00:00:00 2001 From: xl-openai Date: Mon, 9 Feb 2026 13:37:27 -0800 Subject: [PATCH 10/23] skill-creator: Remove invalid reference. (#10960) Remove references to two files that do not exist. --- .../src/skills/assets/samples/skill-creator/SKILL.md | 9 --------- 1 file changed, 9 deletions(-) diff --git a/codex-rs/core/src/skills/assets/samples/skill-creator/SKILL.md b/codex-rs/core/src/skills/assets/samples/skill-creator/SKILL.md index 4c0220dd44..72bc0b97e7 100644 --- a/codex-rs/core/src/skills/assets/samples/skill-creator/SKILL.md +++ b/codex-rs/core/src/skills/assets/samples/skill-creator/SKILL.md @@ -318,15 +318,6 @@ Only include other optional interface fields when the user explicitly provides t When editing the (newly-generated or existing) skill, remember that the skill is being created for another instance of Codex to use. Include information that would be beneficial and non-obvious to Codex. Consider what procedural knowledge, domain-specific details, or reusable assets would help another Codex instance execute these tasks more effectively. -#### Learn Proven Design Patterns - -Consult these helpful guides based on your skill's needs: - -- **Multi-step processes**: See references/workflows.md for sequential workflows and conditional logic -- **Specific output formats or quality standards**: See references/output-patterns.md for template and example patterns - -These files contain established best practices for effective skill design. - #### Start with Reusable Skill Contents To begin implementation, start with the reusable resources identified above: `scripts/`, `references/`, and `assets/` files. Note that this step may require user input. For example, when implementing a `brand-guidelines` skill, the user may need to provide brand assets or templates to store in `assets/`, or documentation to store in `references/`. From c2ca51273f8a1cd8c3467fc8424456d5785f65ff Mon Sep 17 00:00:00 2001 From: jif-oai Date: Mon, 9 Feb 2026 22:14:33 +0000 Subject: [PATCH 11/23] feat: use a notify instead of grace to close ue process (#11219) --- codex-rs/core/src/unified_exec/process.rs | 22 +++++++++- .../core/src/unified_exec/process_manager.rs | 42 +++++++++++++++++-- 2 files changed, 59 insertions(+), 5 deletions(-) diff --git a/codex-rs/core/src/unified_exec/process.rs b/codex-rs/core/src/unified_exec/process.rs index d52d1a6a9d..69790dbad2 100644 --- a/codex-rs/core/src/unified_exec/process.rs +++ b/codex-rs/core/src/unified_exec/process.rs @@ -1,6 +1,8 @@ #![allow(clippy::module_inception)] use std::sync::Arc; +use std::sync::atomic::AtomicBool; +use std::sync::atomic::Ordering; use tokio::sync::Mutex; use tokio::sync::Notify; use tokio::sync::mpsc; @@ -26,6 +28,8 @@ pub(crate) type OutputBuffer = Arc>; pub(crate) struct OutputHandles { pub(crate) output_buffer: OutputBuffer, pub(crate) output_notify: Arc, + pub(crate) output_closed: Arc, + pub(crate) output_closed_notify: Arc, pub(crate) cancellation_token: CancellationToken, } @@ -34,6 +38,8 @@ pub(crate) struct UnifiedExecProcess { process_handle: ExecCommandSession, output_buffer: OutputBuffer, output_notify: Arc, + output_closed: Arc, + output_closed_notify: Arc, cancellation_token: CancellationToken, output_drained: Arc, output_task: JoinHandle<()>, @@ -48,11 +54,15 @@ impl UnifiedExecProcess { ) -> Self { let output_buffer = Arc::new(Mutex::new(HeadTailBuffer::default())); let output_notify = Arc::new(Notify::new()); + let output_closed = Arc::new(AtomicBool::new(false)); + let output_closed_notify = Arc::new(Notify::new()); let cancellation_token = CancellationToken::new(); let output_drained = Arc::new(Notify::new()); let mut receiver = initial_output_rx; let buffer_clone = Arc::clone(&output_buffer); let notify_clone = Arc::clone(&output_notify); + let output_closed_clone = Arc::clone(&output_closed); + let output_closed_notify_clone = Arc::clone(&output_closed_notify); let output_task = tokio::spawn(async move { loop { match receiver.recv().await { @@ -63,7 +73,11 @@ impl UnifiedExecProcess { notify_clone.notify_waiters(); } Err(tokio::sync::broadcast::error::RecvError::Lagged(_)) => continue, - Err(tokio::sync::broadcast::error::RecvError::Closed) => break, + Err(tokio::sync::broadcast::error::RecvError::Closed) => { + output_closed_clone.store(true, Ordering::Release); + output_closed_notify_clone.notify_waiters(); + break; + } }; } }); @@ -72,6 +86,8 @@ impl UnifiedExecProcess { process_handle, output_buffer, output_notify, + output_closed, + output_closed_notify, cancellation_token, output_drained, output_task, @@ -87,6 +103,8 @@ impl UnifiedExecProcess { OutputHandles { output_buffer: Arc::clone(&self.output_buffer), output_notify: Arc::clone(&self.output_notify), + output_closed: Arc::clone(&self.output_closed), + output_closed_notify: Arc::clone(&self.output_closed_notify), cancellation_token: self.cancellation_token.clone(), } } @@ -112,6 +130,8 @@ impl UnifiedExecProcess { } pub(super) fn terminate(&self) { + self.output_closed.store(true, Ordering::Release); + self.output_closed_notify.notify_waiters(); self.process_handle.terminate(); self.cancellation_token.cancel(); self.output_task.abort(); diff --git a/codex-rs/core/src/unified_exec/process_manager.rs b/codex-rs/core/src/unified_exec/process_manager.rs index a81b36c7a1..cee73fae25 100644 --- a/codex-rs/core/src/unified_exec/process_manager.rs +++ b/codex-rs/core/src/unified_exec/process_manager.rs @@ -4,6 +4,7 @@ use std::collections::HashMap; use std::collections::HashSet; use std::path::PathBuf; use std::sync::Arc; +use std::sync::atomic::AtomicBool; use tokio::sync::Notify; use tokio::sync::mpsc; use tokio::time::Duration; @@ -71,6 +72,8 @@ struct PreparedProcessHandles { writer_tx: mpsc::Sender>, output_buffer: OutputBuffer, output_notify: Arc, + output_closed: Arc, + output_closed_notify: Arc, cancellation_token: CancellationToken, command: Vec, process_id: String, @@ -161,12 +164,16 @@ impl UnifiedExecProcessManager { let OutputHandles { output_buffer, output_notify, + output_closed, + output_closed_notify, cancellation_token, } = process.output_handles(); let deadline = start + Duration::from_millis(yield_time_ms); let collected = Self::collect_output_until_deadline( &output_buffer, &output_notify, + &output_closed, + &output_closed_notify, &cancellation_token, deadline, ) @@ -248,6 +255,8 @@ impl UnifiedExecProcessManager { writer_tx, output_buffer, output_notify, + output_closed, + output_closed_notify, cancellation_token, command: session_command, process_id, @@ -279,6 +288,8 @@ impl UnifiedExecProcessManager { let collected = Self::collect_output_until_deadline( &output_buffer, &output_notify, + &output_closed, + &output_closed_notify, &cancellation_token, deadline, ) @@ -369,6 +380,8 @@ impl UnifiedExecProcessManager { let OutputHandles { output_buffer, output_notify, + output_closed, + output_closed_notify, cancellation_token, } = entry.process.output_handles(); @@ -376,6 +389,8 @@ impl UnifiedExecProcessManager { writer_tx: entry.process.writer_sender(), output_buffer, output_notify, + output_closed, + output_closed_notify, cancellation_token, command: entry.command.clone(), process_id: entry.process_id.clone(), @@ -532,13 +547,16 @@ impl UnifiedExecProcessManager { pub(super) async fn collect_output_until_deadline( output_buffer: &OutputBuffer, output_notify: &Arc, + output_closed: &Arc, + output_closed_notify: &Arc, cancellation_token: &CancellationToken, deadline: Instant, ) -> Vec { - const POST_EXIT_OUTPUT_GRACE: Duration = Duration::from_millis(50); + const POST_EXIT_CLOSE_WAIT_CAP: Duration = Duration::from_millis(50); let mut collected: Vec = Vec::with_capacity(4096); let mut exit_signal_received = cancellation_token.is_cancelled(); + let mut post_exit_deadline: Option = None; loop { let drained_chunks: Vec>; let mut wait_for_output = None; @@ -552,20 +570,36 @@ impl UnifiedExecProcessManager { if drained_chunks.is_empty() { exit_signal_received |= cancellation_token.is_cancelled(); + if exit_signal_received && output_closed.load(std::sync::atomic::Ordering::Acquire) + { + break; + } let remaining = deadline.saturating_duration_since(Instant::now()); if remaining == Duration::ZERO { break; } - let notified = wait_for_output.unwrap_or_else(|| output_notify.notified()); if exit_signal_received { - let grace = remaining.min(POST_EXIT_OUTPUT_GRACE); - if tokio::time::timeout(grace, notified).await.is_err() { + let now = Instant::now(); + let close_wait_deadline = *post_exit_deadline + .get_or_insert_with(|| now + remaining.min(POST_EXIT_CLOSE_WAIT_CAP)); + let close_wait_remaining = close_wait_deadline.saturating_duration_since(now); + if close_wait_remaining == Duration::ZERO { break; } + let notified = wait_for_output.unwrap_or_else(|| output_notify.notified()); + let closed = output_closed_notify.notified(); + tokio::pin!(notified); + tokio::pin!(closed); + tokio::select! { + _ = &mut notified => {} + _ = &mut closed => {} + _ = tokio::time::sleep(close_wait_remaining) => break, + } continue; } + let notified = wait_for_output.unwrap_or_else(|| output_notify.notified()); tokio::pin!(notified); let exit_notified = cancellation_token.cancelled(); tokio::pin!(exit_notified); From ffd4bd345caefcadd9d6b1bb0548f324a57c5f9e Mon Sep 17 00:00:00 2001 From: jif-oai Date: Mon, 9 Feb 2026 22:14:39 +0000 Subject: [PATCH 12/23] feat: tie shell snapshot to cwd (#11231) Fix for this: https://github.com/openai/codex/issues/11223 Basically we tie the shell snapshot to a `cwd` to handle `cwd`-based env setups --- codex-rs/core/src/codex.rs | 61 +++++++- codex-rs/core/src/shell_snapshot.rs | 144 +++++++++++++----- codex-rs/core/src/state/service.rs | 2 + codex-rs/core/src/tasks/user_shell.rs | 6 +- codex-rs/core/src/tools/handlers/shell.rs | 1 + codex-rs/core/src/tools/runtimes/mod.rs | 111 ++++++++++++-- codex-rs/core/src/tools/runtimes/shell.rs | 3 +- .../core/src/tools/runtimes/unified_exec.rs | 3 +- 8 files changed, 268 insertions(+), 63 deletions(-) diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index 80319fa8a0..2abb941653 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -1,6 +1,7 @@ use std::collections::HashMap; use std::collections::HashSet; use std::fmt::Debug; +use std::path::Path; use std::path::PathBuf; use std::sync::Arc; use std::sync::atomic::AtomicU64; @@ -87,6 +88,7 @@ use tokio::sync::Mutex; use tokio::sync::OnceCell; use tokio::sync::RwLock; use tokio::sync::oneshot; +use tokio::sync::watch; use tokio_util::sync::CancellationToken; use tracing::Instrument; use tracing::debug; @@ -234,8 +236,6 @@ use codex_protocol::protocol::InitialHistory; use codex_protocol::user_input::UserInput; use codex_utils_readiness::Readiness; use codex_utils_readiness::ReadinessFlag; -use tokio::sync::watch; - /// The high-level interface to the Codex system. /// It operates as a queue pair where you send submissions and receive events. pub struct Codex { @@ -1033,14 +1033,19 @@ impl Session { let mut default_shell = shell::default_user_shell(); // Create the mutable state for the Session. - if config.features.enabled(Feature::ShellSnapshot) { + let shell_snapshot_tx = if config.features.enabled(Feature::ShellSnapshot) { ShellSnapshot::start_snapshotting( config.codex_home.clone(), conversation_id, + session_configuration.cwd.clone(), &mut default_shell, otel_manager.clone(), - ); - } + ) + } else { + let (tx, rx) = watch::channel(None); + default_shell.shell_snapshot = rx; + tx + }; let thread_name = match session_index::find_thread_name_by_id(&config.codex_home, &conversation_id).await { @@ -1064,6 +1069,7 @@ impl Session { hooks: Hooks::new(config.as_ref()), rollout: Mutex::new(rollout_recorder), user_shell: Arc::new(default_shell), + shell_snapshot_tx, show_raw_agent_reasoning: config.show_raw_agent_reasoning, exec_policy, auth_manager: Arc::clone(&auth_manager), @@ -1405,6 +1411,30 @@ impl Session { state.pending_resume_previous_model.take() } + fn maybe_refresh_shell_snapshot_for_cwd( + &self, + previous_cwd: &Path, + next_cwd: &Path, + codex_home: &Path, + ) { + if previous_cwd == next_cwd { + return; + } + + if !self.features.enabled(Feature::ShellSnapshot) { + return; + } + + ShellSnapshot::refresh_snapshot( + codex_home.to_path_buf(), + self.conversation_id, + next_cwd.to_path_buf(), + self.services.user_shell.as_ref().clone(), + self.services.shell_snapshot_tx.clone(), + self.services.otel_manager.clone(), + ); + } + pub(crate) async fn update_settings( &self, updates: SessionSettingsUpdate, @@ -1413,7 +1443,14 @@ impl Session { match state.session_configuration.apply(&updates) { Ok(updated) => { + let previous_cwd = state.session_configuration.cwd.clone(); + let next_cwd = updated.cwd.clone(); + let codex_home = updated.codex_home.clone(); state.session_configuration = updated; + drop(state); + + self.maybe_refresh_shell_snapshot_for_cwd(&previous_cwd, &next_cwd, &codex_home); + Ok(()) } Err(err) => { @@ -1428,14 +1465,16 @@ impl Session { sub_id: String, updates: SessionSettingsUpdate, ) -> ConstraintResult> { - let (session_configuration, sandbox_policy_changed) = { + let (session_configuration, sandbox_policy_changed, previous_cwd, codex_home) = { let mut state = self.state.lock().await; match state.session_configuration.clone().apply(&updates) { Ok(next) => { + let previous_cwd = state.session_configuration.cwd.clone(); let sandbox_policy_changed = state.session_configuration.sandbox_policy != next.sandbox_policy; + let codex_home = next.codex_home.clone(); state.session_configuration = next.clone(); - (next, sandbox_policy_changed) + (next, sandbox_policy_changed, previous_cwd, codex_home) } Err(err) => { drop(state); @@ -1452,6 +1491,12 @@ impl Session { } }; + self.maybe_refresh_shell_snapshot_for_cwd( + &previous_cwd, + &session_configuration.cwd, + &codex_home, + ); + Ok(self .new_turn_from_configuration( sub_id, @@ -6123,6 +6168,7 @@ mod tests { hooks: Hooks::new(&config), rollout: Mutex::new(None), user_shell: Arc::new(default_user_shell()), + shell_snapshot_tx: watch::channel(None).0, show_raw_agent_reasoning: config.show_raw_agent_reasoning, exec_policy, auth_manager: auth_manager.clone(), @@ -6255,6 +6301,7 @@ mod tests { hooks: Hooks::new(&config), rollout: Mutex::new(None), user_shell: Arc::new(default_user_shell()), + shell_snapshot_tx: watch::channel(None).0, show_raw_agent_reasoning: config.show_raw_agent_reasoning, exec_policy, auth_manager: Arc::clone(&auth_manager), diff --git a/codex-rs/core/src/shell_snapshot.rs b/codex-rs/core/src/shell_snapshot.rs index 1277328244..6eb1ef71da 100644 --- a/codex-rs/core/src/shell_snapshot.rs +++ b/codex-rs/core/src/shell_snapshot.rs @@ -26,6 +26,7 @@ use tracing::info_span; #[derive(Clone, Debug, PartialEq, Eq)] pub struct ShellSnapshot { pub path: PathBuf, + pub cwd: PathBuf, } const SNAPSHOT_TIMEOUT: Duration = Duration::from_secs(10); @@ -37,22 +38,63 @@ impl ShellSnapshot { pub fn start_snapshotting( codex_home: PathBuf, session_id: ThreadId, + session_cwd: PathBuf, shell: &mut Shell, otel_manager: OtelManager, - ) { + ) -> watch::Sender>> { let (shell_snapshot_tx, shell_snapshot_rx) = watch::channel(None); shell.shell_snapshot = shell_snapshot_rx; - let snapshot_shell = shell.clone(); - let snapshot_session_id = session_id; - let snapshot_span = info_span!("shell_snapshot", thread_id = %snapshot_session_id); + Self::spawn_snapshot_task( + codex_home, + session_id, + session_cwd, + shell.clone(), + shell_snapshot_tx.clone(), + otel_manager, + ); + + shell_snapshot_tx + } + + pub fn refresh_snapshot( + codex_home: PathBuf, + session_id: ThreadId, + session_cwd: PathBuf, + shell: Shell, + shell_snapshot_tx: watch::Sender>>, + otel_manager: OtelManager, + ) { + Self::spawn_snapshot_task( + codex_home, + session_id, + session_cwd, + shell, + shell_snapshot_tx, + otel_manager, + ); + } + + fn spawn_snapshot_task( + codex_home: PathBuf, + session_id: ThreadId, + session_cwd: PathBuf, + snapshot_shell: Shell, + shell_snapshot_tx: watch::Sender>>, + otel_manager: OtelManager, + ) { + let snapshot_span = info_span!("shell_snapshot", thread_id = %session_id); tokio::spawn( async move { let timer = otel_manager.start_timer("codex.shell_snapshot.duration_ms", &[]); - let snapshot = - ShellSnapshot::try_new(&codex_home, snapshot_session_id, &snapshot_shell) - .await - .map(Arc::new); + let snapshot = ShellSnapshot::try_new( + &codex_home, + session_id, + session_cwd.as_path(), + &snapshot_shell, + ) + .await + .map(Arc::new); let success = if snapshot.is_some() { "true" } else { "false" }; let _ = timer.map(|timer| timer.record(&[("success", success)])); otel_manager.counter("codex.shell_snapshot", 1, &[("success", success)]); @@ -62,7 +104,12 @@ impl ShellSnapshot { ); } - async fn try_new(codex_home: &Path, session_id: ThreadId, shell: &Shell) -> Option { + async fn try_new( + codex_home: &Path, + session_id: ThreadId, + session_cwd: &Path, + shell: &Shell, + ) -> Option { // File to store the snapshot let extension = match shell.shell_type { ShellType::PowerShell => "ps1", @@ -82,22 +129,26 @@ impl ShellSnapshot { }); // Make the new snapshot. - let snapshot = match write_shell_snapshot(shell.shell_type.clone(), &path).await { - Ok(path) => { - tracing::info!("Shell snapshot successfully created: {}", path.display()); - Some(Self { path }) - } - Err(err) => { - tracing::warn!( - "Failed to create shell snapshot for {}: {err:?}", - shell.name() - ); - None - } - }; + let snapshot = + match write_shell_snapshot(shell.shell_type.clone(), &path, session_cwd).await { + Ok(path) => { + tracing::info!("Shell snapshot successfully created: {}", path.display()); + Some(Self { + path, + cwd: session_cwd.to_path_buf(), + }) + } + Err(err) => { + tracing::warn!( + "Failed to create shell snapshot for {}: {err:?}", + shell.name() + ); + None + } + }; if let Some(snapshot) = snapshot.as_ref() - && let Err(err) = validate_snapshot(shell, &snapshot.path).await + && let Err(err) = validate_snapshot(shell, &snapshot.path, session_cwd).await { tracing::error!("Shell snapshot validation failed: {err:?}"); return None; @@ -118,14 +169,18 @@ impl Drop for ShellSnapshot { } } -async fn write_shell_snapshot(shell_type: ShellType, output_path: &Path) -> Result { +async fn write_shell_snapshot( + shell_type: ShellType, + output_path: &Path, + cwd: &Path, +) -> Result { if shell_type == ShellType::PowerShell || shell_type == ShellType::Cmd { bail!("Shell snapshot not supported yet for {shell_type:?}"); } let shell = get_shell(shell_type.clone(), None) .with_context(|| format!("No available shell for {shell_type:?}"))?; - let raw_snapshot = capture_snapshot(&shell).await?; + let raw_snapshot = capture_snapshot(&shell, cwd).await?; let snapshot = strip_snapshot_preamble(&raw_snapshot)?; if let Some(parent) = output_path.parent() { @@ -143,13 +198,13 @@ async fn write_shell_snapshot(shell_type: ShellType, output_path: &Path) -> Resu Ok(output_path.to_path_buf()) } -async fn capture_snapshot(shell: &Shell) -> Result { +async fn capture_snapshot(shell: &Shell, cwd: &Path) -> Result { let shell_type = shell.shell_type.clone(); match shell_type { - ShellType::Zsh => run_shell_script(shell, &zsh_snapshot_script()).await, - ShellType::Bash => run_shell_script(shell, &bash_snapshot_script()).await, - ShellType::Sh => run_shell_script(shell, &sh_snapshot_script()).await, - ShellType::PowerShell => run_shell_script(shell, powershell_snapshot_script()).await, + ShellType::Zsh => run_shell_script(shell, &zsh_snapshot_script(), cwd).await, + ShellType::Bash => run_shell_script(shell, &bash_snapshot_script(), cwd).await, + ShellType::Sh => run_shell_script(shell, &sh_snapshot_script(), cwd).await, + ShellType::PowerShell => run_shell_script(shell, powershell_snapshot_script(), cwd).await, ShellType::Cmd => bail!("Shell snapshotting is not yet supported for {shell_type:?}"), } } @@ -163,16 +218,16 @@ fn strip_snapshot_preamble(snapshot: &str) -> Result { Ok(snapshot[start..].to_string()) } -async fn validate_snapshot(shell: &Shell, snapshot_path: &Path) -> Result<()> { +async fn validate_snapshot(shell: &Shell, snapshot_path: &Path, cwd: &Path) -> Result<()> { let snapshot_path_display = snapshot_path.display(); let script = format!("set -e; . \"{snapshot_path_display}\""); - run_script_with_timeout(shell, &script, SNAPSHOT_TIMEOUT, false) + run_script_with_timeout(shell, &script, SNAPSHOT_TIMEOUT, false, cwd) .await .map(|_| ()) } -async fn run_shell_script(shell: &Shell, script: &str) -> Result { - run_script_with_timeout(shell, script, SNAPSHOT_TIMEOUT, true).await +async fn run_shell_script(shell: &Shell, script: &str, cwd: &Path) -> Result { + run_script_with_timeout(shell, script, SNAPSHOT_TIMEOUT, true, cwd).await } async fn run_script_with_timeout( @@ -180,6 +235,7 @@ async fn run_script_with_timeout( script: &str, snapshot_timeout: Duration, use_login_shell: bool, + cwd: &Path, ) -> Result { let args = shell.derive_exec_args(script, use_login_shell); let shell_name = shell.name(); @@ -189,6 +245,7 @@ async fn run_script_with_timeout( let mut handler = Command::new(&args[0]); handler.args(&args[1..]); handler.stdin(Stdio::null()); + handler.current_dir(cwd); #[cfg(unix)] unsafe { handler.pre_exec(|| { @@ -550,7 +607,7 @@ mod tests { async fn get_snapshot(shell_type: ShellType) -> Result { let dir = tempdir()?; let path = dir.path().join("snapshot.sh"); - write_shell_snapshot(shell_type, &path).await?; + write_shell_snapshot(shell_type, &path, dir.path()).await?; let content = fs::read_to_string(&path).await?; Ok(content) } @@ -602,11 +659,12 @@ mod tests { shell_snapshot: crate::shell::empty_shell_snapshot_receiver(), }; - let snapshot = ShellSnapshot::try_new(dir.path(), ThreadId::new(), &shell) + let snapshot = ShellSnapshot::try_new(dir.path(), ThreadId::new(), dir.path(), &shell) .await .expect("snapshot should be created"); let path = snapshot.path.clone(); assert!(path.exists()); + assert_eq!(snapshot.cwd, dir.path().to_path_buf()); drop(snapshot); @@ -635,9 +693,10 @@ mod tests { "HOME=\"{home_display}\"; export HOME; {}", bash_snapshot_script() ); - let output = run_script_with_timeout(&shell, &script, Duration::from_millis(500), true) - .await - .context("run snapshot command")?; + let output = + run_script_with_timeout(&shell, &script, Duration::from_millis(500), true, home) + .await + .context("run snapshot command")?; assert!( output.contains("# Snapshot file"), @@ -665,9 +724,10 @@ mod tests { shell_snapshot: crate::shell::empty_shell_snapshot_receiver(), }; - let err = run_script_with_timeout(&shell, &script, Duration::from_secs(1), true) - .await - .expect_err("snapshot shell should time out"); + let err = + run_script_with_timeout(&shell, &script, Duration::from_secs(1), true, dir.path()) + .await + .expect_err("snapshot shell should time out"); assert!( err.to_string().contains("timed out"), "expected timeout error, got {err:?}" diff --git a/codex-rs/core/src/state/service.rs b/codex-rs/core/src/state/service.rs index d9f57cb5f5..0438119d42 100644 --- a/codex-rs/core/src/state/service.rs +++ b/codex-rs/core/src/state/service.rs @@ -17,6 +17,7 @@ use crate::unified_exec::UnifiedExecProcessManager; use codex_otel::OtelManager; use tokio::sync::Mutex; use tokio::sync::RwLock; +use tokio::sync::watch; use tokio_util::sync::CancellationToken; pub(crate) struct SessionServices { @@ -27,6 +28,7 @@ pub(crate) struct SessionServices { pub(crate) hooks: Hooks, pub(crate) rollout: Mutex>, pub(crate) user_shell: Arc, + pub(crate) shell_snapshot_tx: watch::Sender>>, pub(crate) show_raw_agent_reasoning: bool, pub(crate) exec_policy: ExecPolicyManager, pub(crate) auth_manager: Arc, diff --git a/codex-rs/core/src/tasks/user_shell.rs b/codex-rs/core/src/tasks/user_shell.rs index 0ee65eacea..c034e26f47 100644 --- a/codex-rs/core/src/tasks/user_shell.rs +++ b/codex-rs/core/src/tasks/user_shell.rs @@ -113,7 +113,11 @@ pub(crate) async fn execute_user_shell_command( let use_login_shell = true; let session_shell = session.user_shell(); let display_command = session_shell.derive_exec_args(&command, use_login_shell); - let exec_command = maybe_wrap_shell_lc_with_snapshot(&display_command, session_shell.as_ref()); + let exec_command = maybe_wrap_shell_lc_with_snapshot( + &display_command, + session_shell.as_ref(), + turn_context.cwd.as_path(), + ); let call_id = Uuid::new_v4().to_string(); let raw_command = command; diff --git a/codex-rs/core/src/tools/handlers/shell.rs b/codex-rs/core/src/tools/handlers/shell.rs index fd678c3a3c..18c594d997 100644 --- a/codex-rs/core/src/tools/handlers/shell.rs +++ b/codex-rs/core/src/tools/handlers/shell.rs @@ -455,6 +455,7 @@ mod tests { fn shell_command_handler_respects_explicit_login_flag() { let (_tx, shell_snapshot) = watch::channel(Some(Arc::new(ShellSnapshot { path: PathBuf::from("/tmp/snapshot.sh"), + cwd: PathBuf::from("/tmp"), }))); let shell = Shell { shell_type: ShellType::Bash, diff --git a/codex-rs/core/src/tools/runtimes/mod.rs b/codex-rs/core/src/tools/runtimes/mod.rs index 8e4c47722d..b3d7b32921 100644 --- a/codex-rs/core/src/tools/runtimes/mod.rs +++ b/codex-rs/core/src/tools/runtimes/mod.rs @@ -5,6 +5,7 @@ Concrete ToolRuntime implementations for specific tools. Each runtime stays small and focused and reuses the orchestrator for approvals + sandbox + retry. */ use crate::exec::ExecExpiration; +use crate::path_utils; use crate::sandboxing::CommandSpec; use crate::sandboxing::SandboxPermissions; use crate::shell::Shell; @@ -50,10 +51,12 @@ pub(crate) fn build_command_spec( /// => user_shell -c ". SNAPSHOT (best effort); exec shell -c