diff --git a/.github/workflows/close-stale-contributor-prs.yml b/.github/workflows/close-stale-contributor-prs.yml index e01bc3881d..43e6992883 100644 --- a/.github/workflows/close-stale-contributor-prs.yml +++ b/.github/workflows/close-stale-contributor-prs.yml @@ -12,6 +12,8 @@ permissions: jobs: close-stale-contributor-prs: + # Prevent scheduled runs on forks + if: github.repository == 'openai/codex' runs-on: ubuntu-latest steps: - name: Close inactive PRs from contributors diff --git a/.github/workflows/issue-deduplicator.yml b/.github/workflows/issue-deduplicator.yml index c78b1f3161..4b417ae59d 100644 --- a/.github/workflows/issue-deduplicator.yml +++ b/.github/workflows/issue-deduplicator.yml @@ -9,7 +9,8 @@ on: jobs: gather-duplicates: name: Identify potential duplicates - if: ${{ github.event.action == 'opened' || (github.event.action == 'labeled' && github.event.label.name == 'codex-deduplicate') }} + # Prevent runs on forks (requires OpenAI API key, wastes Actions minutes) + if: github.repository == 'openai/codex' && (github.event.action == 'opened' || (github.event.action == 'labeled' && github.event.label.name == 'codex-deduplicate')) runs-on: ubuntu-latest permissions: contents: read diff --git a/.github/workflows/issue-labeler.yml b/.github/workflows/issue-labeler.yml index 424c7e7263..da77812fec 100644 --- a/.github/workflows/issue-labeler.yml +++ b/.github/workflows/issue-labeler.yml @@ -9,7 +9,8 @@ on: jobs: gather-labels: name: Generate label suggestions - if: ${{ github.event.action == 'opened' || (github.event.action == 'labeled' && github.event.label.name == 'codex-label') }} + # Prevent runs on forks (requires OpenAI API key, wastes Actions minutes) + if: github.repository == 'openai/codex' && (github.event.action == 'opened' || (github.event.action == 'labeled' && github.event.label.name == 'codex-label')) runs-on: ubuntu-latest permissions: contents: read diff --git a/.github/workflows/rust-release-prepare.yml b/.github/workflows/rust-release-prepare.yml index b62a855055..c9f11f54fc 100644 --- a/.github/workflows/rust-release-prepare.yml +++ b/.github/workflows/rust-release-prepare.yml @@ -14,6 +14,8 @@ permissions: jobs: prepare: + # Prevent scheduled runs on forks (no secrets, wastes Actions minutes) + if: github.repository == 'openai/codex' runs-on: ubuntu-latest steps: - uses: actions/checkout@v6 diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index d8b99165e9..90acb1e772 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -42,7 +42,7 @@ dependencies = [ "bitflags 2.10.0", "bytes", "bytestring", - "derive_more 2.0.1", + "derive_more 2.1.1", "encoding_rs", "foldhash 0.1.5", "futures-core", @@ -137,7 +137,7 @@ dependencies = [ "bytes", "bytestring", "cfg-if", - "derive_more 2.0.1", + "derive_more 2.1.1", "encoding_rs", "foldhash 0.1.5", "futures-core", @@ -360,7 +360,7 @@ dependencies = [ "objc2-foundation", "parking_lot", "percent-encoding", - "windows-sys 0.60.2", + "windows-sys 0.52.0", "wl-clipboard-rs", "x11rb", ] @@ -912,9 +912,9 @@ dependencies = [ [[package]] name = "clap_complete" -version = "4.5.57" +version = "4.5.64" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "4d9501bd3f5f09f7bbee01da9a511073ed30a80cd7a509f1214bb74eadea71ad" +checksum = "4c0da80818b2d95eca9aa614a30783e42f62bf5fdfee24e68cfb960b071ba8d1" dependencies = [ "clap", ] @@ -1433,7 +1433,7 @@ dependencies = [ "allocative", "anyhow", "clap", - "derive_more 2.0.1", + "derive_more 2.1.1", "env_logger", "log", "multimap", @@ -1454,6 +1454,7 @@ dependencies = [ "codex-protocol", "pretty_assertions", "sentry", + "tracing", "tracing-subscriber", ] @@ -1609,7 +1610,6 @@ dependencies = [ "serde_json", "strum_macros 0.27.2", "tokio", - "tonic", "tracing", "tracing-opentelemetry", "tracing-subscriber", @@ -1732,7 +1732,7 @@ dependencies = [ "codex-windows-sandbox", "color-eyre", "crossterm", - "derive_more 2.0.1", + "derive_more 2.1.1", "diffy", "dirs", "dunce", @@ -1805,7 +1805,7 @@ dependencies = [ "codex-windows-sandbox", "color-eyre", "crossterm", - "derive_more 2.0.1", + "derive_more 2.1.1", "diffy", "dirs", "dunce", @@ -1820,6 +1820,7 @@ dependencies = [ "pulldown-cmark", "rand 0.9.2", "ratatui", + "ratatui-core", "ratatui-macros", "regex-lite", "reqwest", @@ -1841,6 +1842,7 @@ dependencies = [ "tracing-subscriber", "tree-sitter-bash", "tree-sitter-highlight", + "tui-scrollbar", "unicode-segmentation", "unicode-width 0.2.1", "url", @@ -2005,6 +2007,20 @@ dependencies = [ "static_assertions", ] +[[package]] +name = "compact_str" +version = "0.9.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3fdb1325a1cece981e8a296ab8f0f9b63ae357bd0784a9faaf548cc7b480707a" +dependencies = [ + "castaway", + "cfg-if", + "itoa", + "rustversion", + "ryu", + "static_assertions", +] + [[package]] name = "concurrent-queue" version = "2.5.0" @@ -2026,6 +2042,18 @@ dependencies = [ "windows-sys 0.59.0", ] +[[package]] +name = "const-hex" +version = "1.17.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3bb320cac8a0750d7f25280aa97b09c26edfe161164238ecbbb31092b079e735" +dependencies = [ + "cfg-if", + "cpufeatures", + "proptest", + "serde_core", +] + [[package]] name = "convert_case" version = "0.6.0" @@ -2037,9 +2065,9 @@ dependencies = [ [[package]] name = "convert_case" -version = "0.7.1" +version = "0.10.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "bb402b8d4c85569410425650ce3eddc7d698ed96d39a73f941b08fb63082f1e7" +checksum = "633458d4ef8c78b72454de2d54fd6ab2e60f9e02be22f3c6104cdc8a4e0fceb9" dependencies = [ "unicode-segmentation", ] @@ -2426,11 +2454,11 @@ dependencies = [ [[package]] name = "derive_more" -version = "2.0.1" +version = "2.1.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "093242cf7570c207c83073cf82f79706fe7b8317e98620a47d5be7c3d8497678" +checksum = "d751e9e49156b02b44f9c1815bcb94b984cdcc4396ecc32521c739452808b134" dependencies = [ - "derive_more-impl 2.0.1", + "derive_more-impl 2.1.1", ] [[package]] @@ -2448,13 +2476,14 @@ dependencies = [ [[package]] name = "derive_more-impl" -version = "2.0.1" +version = "2.1.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "bda628edc44c4bb645fbe0f758797143e4e07926f7ebf4e9bdfbd3d2ce621df3" +checksum = "799a97264921d8623a957f6c3b9011f3b5492f557bbb7a5a19b7fa6d06ba8dcb" dependencies = [ - "convert_case 0.7.1", + "convert_case 0.10.0", "proc-macro2", "quote", + "rustc_version", "syn 2.0.104", "unicode-xid", ] @@ -2570,6 +2599,15 @@ version = "0.3.3" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "fea41bba32d969b513997752735605054bc0dfa92b4c56bf1189f2e174be7a10" +[[package]] +name = "document-features" +version = "0.2.12" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "d4b8a88685455ed29a21542a33abd9cb6510b6b129abadabdcef0f4c55bc8f61" +dependencies = [ + "litrs", +] + [[package]] name = "dotenvy" version = "0.15.7" @@ -2743,7 +2781,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "778e2ac28f6c47af28e4907f13ffd1e1ddbd400980a9abd7c8df189bf578a5ad" dependencies = [ "libc", - "windows-sys 0.60.2", + "windows-sys 0.52.0", ] [[package]] @@ -2851,7 +2889,7 @@ checksum = "0ce92ff622d6dadf7349484f42c93271a0d49b7cc4d466a936405bacbe10aa78" dependencies = [ "cfg-if", "rustix 1.0.8", - "windows-sys 0.59.0", + "windows-sys 0.52.0", ] [[package]] @@ -3736,13 +3774,14 @@ dependencies = [ [[package]] name = "insta" -version = "1.44.3" +version = "1.46.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "b5c943d4415edd8153251b6f197de5eb1640e56d84e8d9159bea190421c73698" +checksum = "1b66886d14d18d420ab5052cbff544fc5d34d0b2cdd35eb5976aaa10a4a472e5" dependencies = [ "console", "once_cell", "similar", + "tempfile", ] [[package]] @@ -3791,7 +3830,7 @@ checksum = "e04d7f318608d35d4b61ddd75cbdaee86b023ebe2bd5a66ee0915f0bf93095a9" dependencies = [ "hermit-abi", "libc", - "windows-sys 0.59.0", + "windows-sys 0.52.0", ] [[package]] @@ -3895,6 +3934,16 @@ dependencies = [ "wasm-bindgen", ] +[[package]] +name = "kasuari" +version = "0.4.11" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "8fe90c1150662e858c7d5f945089b7517b0a80d8bf7ba4b1b5ffc984e7230a5b" +dependencies = [ + "hashbrown 0.16.0", + "thiserror 2.0.17", +] + [[package]] name = "keyring" version = "3.6.3" @@ -4040,6 +4089,12 @@ version = "0.8.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "241eaef5fd12c88705a01fc1066c48c4b36e0dd4377dcdc7ec3942cea7a69956" +[[package]] +name = "litrs" +version = "1.0.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "11d3d7f243d5c5a8b9bb5d6dd2b1602c0cb0b9db1621bafc7ed66e35ff9fe092" + [[package]] name = "local-waker" version = "0.1.4" @@ -4661,9 +4716,9 @@ dependencies = [ [[package]] name = "opentelemetry" -version = "0.30.0" +version = "0.31.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "aaf416e4cb72756655126f7dd7bb0af49c674f4c1b9903e80c009e0c37e552e6" +checksum = "b84bcd6ae87133e903af7ef497404dda70c60d0ea14895fc8a5e6722754fc2a0" dependencies = [ "futures-core", "futures-sink", @@ -4675,9 +4730,9 @@ dependencies = [ [[package]] name = "opentelemetry-appender-tracing" -version = "0.30.1" +version = "0.31.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "e68f63eca5fad47e570e00e893094fc17be959c80c79a7d6ec1abdd5ae6ffc16" +checksum = "ef6a1ac5ca3accf562b8c306fa8483c85f4390f768185ab775f242f7fe8fdcc2" dependencies = [ "opentelemetry", "tracing", @@ -4687,9 +4742,9 @@ dependencies = [ [[package]] name = "opentelemetry-http" -version = "0.30.0" +version = "0.31.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "50f6639e842a97dbea8886e3439710ae463120091e2e064518ba8e716e6ac36d" +checksum = "d7a6d09a73194e6b66df7c8f1b680f156d916a1a942abf2de06823dd02b7855d" dependencies = [ "async-trait", "bytes", @@ -4700,9 +4755,9 @@ dependencies = [ [[package]] name = "opentelemetry-otlp" -version = "0.30.0" +version = "0.31.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "dbee664a43e07615731afc539ca60c6d9f1a9425e25ca09c57bc36c87c55852b" +checksum = "7a2366db2dca4d2ad033cad11e6ee42844fd727007af5ad04a1730f4cb8163bf" dependencies = [ "http 1.3.1", "opentelemetry", @@ -4720,30 +4775,32 @@ dependencies = [ [[package]] name = "opentelemetry-proto" -version = "0.30.0" +version = "0.31.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "2e046fd7660710fe5a05e8748e70d9058dc15c94ba914e7c4faa7c728f0e8ddc" +checksum = "a7175df06de5eaee9909d4805a3d07e28bb752c34cab57fa9cff549da596b30f" dependencies = [ "base64", - "hex", + "const-hex", "opentelemetry", "opentelemetry_sdk", "prost", "serde", + "serde_json", "tonic", + "tonic-prost", ] [[package]] name = "opentelemetry-semantic-conventions" -version = "0.30.0" +version = "0.31.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "83d059a296a47436748557a353c5e6c5705b9470ef6c95cfc52c21a8814ddac2" +checksum = "e62e29dfe041afb8ed2a6c9737ab57db4907285d999ef8ad3a59092a36bdc846" [[package]] name = "opentelemetry_sdk" -version = "0.30.0" +version = "0.31.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "11f644aa9e5e31d11896e024305d7e3c98a88884d9f8919dbf37a9991bc47a4b" +checksum = "e14ae4f5991976fd48df6d843de219ca6d31b01daaab2dad5af2badeded372bd" dependencies = [ "futures-channel", "futures-executor", @@ -4751,7 +4808,6 @@ dependencies = [ "opentelemetry", "percent-encoding", "rand 0.9.2", - "serde_json", "thiserror 2.0.17", "tokio", "tokio-stream", @@ -5134,10 +5190,25 @@ dependencies = [ ] [[package]] -name = "prost" -version = "0.13.5" +name = "proptest" +version = "1.9.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "2796faa41db3ec313a31f7624d9286acf277b52de526150b7e69f3debf891ee5" +checksum = "bee689443a2bd0a16ab0348b52ee43e3b2d1b1f931c8aa5c9f8de4c86fbe8c40" +dependencies = [ + "bitflags 2.10.0", + "num-traits", + "rand 0.9.2", + "rand_chacha 0.9.0", + "rand_xorshift", + "regex-syntax 0.8.5", + "unarray", +] + +[[package]] +name = "prost" +version = "0.14.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "7231bd9b3d3d33c86b58adbac74b5ec0ad9f496b19d22801d773636feaa95f3d" dependencies = [ "bytes", "prost-derive", @@ -5145,9 +5216,9 @@ dependencies = [ [[package]] name = "prost-derive" -version = "0.13.5" +version = "0.14.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "8a56d757972c98b346a9b766e3f02746cde6dd1cd1d1d563472929fdd74bec4d" +checksum = "9120690fafc389a67ba3803df527d0ec9cbbc9cc45e4cc20b332996dfb672425" dependencies = [ "anyhow", "itertools 0.14.0", @@ -5260,7 +5331,7 @@ dependencies = [ "once_cell", "socket2 0.6.1", "tracing", - "windows-sys 0.60.2", + "windows-sys 0.52.0", ] [[package]] @@ -5347,6 +5418,15 @@ dependencies = [ "getrandom 0.3.3", ] +[[package]] +name = "rand_xorshift" +version = "0.4.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "513962919efc330f829edb2535844d1b912b0fbe2ca165d613e4e8788bb05a5a" +dependencies = [ + "rand_core 0.9.3", +] + [[package]] name = "ratatui" version = "0.29.0" @@ -5354,7 +5434,7 @@ source = "git+https://github.com/nornagon/ratatui?branch=nornagon-v0.29.0-patch# dependencies = [ "bitflags 2.10.0", "cassowary", - "compact_str", + "compact_str 0.8.1", "crossterm", "indoc", "instability", @@ -5363,7 +5443,27 @@ dependencies = [ "paste", "strum 0.26.3", "unicode-segmentation", - "unicode-truncate", + "unicode-truncate 1.1.0", + "unicode-width 0.2.1", +] + +[[package]] +name = "ratatui-core" +version = "0.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "5ef8dea09a92caaf73bff7adb70b76162e5937524058a7e5bff37869cbbec293" +dependencies = [ + "bitflags 2.10.0", + "compact_str 0.9.0", + "hashbrown 0.16.0", + "indoc", + "itertools 0.14.0", + "kasuari", + "lru 0.16.2", + "strum 0.27.2", + "thiserror 2.0.17", + "unicode-segmentation", + "unicode-truncate 2.0.0", "unicode-width 0.2.1", ] @@ -5610,7 +5710,7 @@ dependencies = [ "errno", "libc", "linux-raw-sys 0.4.15", - "windows-sys 0.59.0", + "windows-sys 0.52.0", ] [[package]] @@ -5623,7 +5723,7 @@ dependencies = [ "errno", "libc", "linux-raw-sys 0.9.4", - "windows-sys 0.60.2", + "windows-sys 0.52.0", ] [[package]] @@ -6530,6 +6630,9 @@ name = "strum" version = "0.27.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "af23d6f6c1a224baef9d3f61e287d2761385a5b88fdab4eb4c6f11aeb54c4bcf" +dependencies = [ + "strum_macros 0.27.2", +] [[package]] name = "strum_macros" @@ -6969,9 +7072,9 @@ dependencies = [ [[package]] name = "tokio-stream" -version = "0.1.17" +version = "0.1.18" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "eca58d7bba4a75707817a2c44174253f9236b2d5fbd055602e9d5c07c139a047" +checksum = "32da49809aab5c3bc678af03902d4ccddea2a87d028d86392a4b1560c6906c70" dependencies = [ "futures-core", "pin-project-lite", @@ -7082,15 +7185,13 @@ checksum = "ab16f14aed21ee8bfd8ec22513f7287cd4a91aa92e44edfe2c17ddd004e92607" [[package]] name = "tonic" -version = "0.13.1" +version = "0.14.2" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "7e581ba15a835f4d9ea06c55ab1bd4dce26fc53752c69a04aac00703bfb49ba9" +checksum = "eb7613188ce9f7df5bfe185db26c5814347d110db17920415cf2fbcad85e7203" dependencies = [ "async-trait", - "axum", "base64", "bytes", - "h2", "http 1.3.1", "http-body", "http-body-util", @@ -7099,9 +7200,8 @@ dependencies = [ "hyper-util", "percent-encoding", "pin-project", - "prost", "rustls-native-certs", - "socket2 0.5.10", + "sync_wrapper", "tokio", "tokio-rustls", "tokio-stream", @@ -7111,6 +7211,17 @@ dependencies = [ "tracing", ] +[[package]] +name = "tonic-prost" +version = "0.14.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "66bd50ad6ce1252d87ef024b3d64fe4c3cf54a86fb9ef4c631fdd0ded7aeaa67" +dependencies = [ + "bytes", + "prost", + "tonic", +] + [[package]] name = "tower" version = "0.5.2" @@ -7228,15 +7339,16 @@ dependencies = [ [[package]] name = "tracing-opentelemetry" -version = "0.31.0" +version = "0.32.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "ddcf5959f39507d0d04d6413119c04f33b623f4f951ebcbdddddfad2d0623a9c" +checksum = "1e6e5658463dd88089aba75c7791e1d3120633b1bfde22478b28f625a9bb1b8e" dependencies = [ "js-sys", - "once_cell", "opentelemetry", "opentelemetry_sdk", + "rustversion", "smallvec", + "thiserror 2.0.17", "tracing", "tracing-core", "tracing-log", @@ -7367,6 +7479,16 @@ dependencies = [ "termcolor", ] +[[package]] +name = "tui-scrollbar" +version = "0.2.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "c42613099915b2e30e9f144670666e858e2538366f77742e1cf1c2f230efcacd" +dependencies = [ + "document-features", + "ratatui-core", +] + [[package]] name = "typenum" version = "1.18.0" @@ -7393,6 +7515,12 @@ dependencies = [ "libc", ] +[[package]] +name = "unarray" +version = "0.1.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "eaea85b334db583fe3274d12b4cd1880032beab409c0d774be044d4480ab9a94" + [[package]] name = "unicase" version = "2.8.1" @@ -7428,6 +7556,17 @@ dependencies = [ "unicode-width 0.1.14", ] +[[package]] +name = "unicode-truncate" +version = "2.0.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "8fbf03860ff438702f3910ca5f28f8dac63c1c11e7efb5012b8b175493606330" +dependencies = [ + "itertools 0.13.0", + "unicode-segmentation", + "unicode-width 0.2.1", +] + [[package]] name = "unicode-width" version = "0.1.14" @@ -7872,7 +8011,7 @@ version = "0.1.9" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "cf221c93e13a30d793f7645a0e7762c55d169dbb0a49671918a2319d289b10bb" dependencies = [ - "windows-sys 0.59.0", + "windows-sys 0.52.0", ] [[package]] diff --git a/codex-rs/Cargo.toml b/codex-rs/Cargo.toml index 20fd10ee9d..645d5aa210 100644 --- a/codex-rs/Cargo.toml +++ b/codex-rs/Cargo.toml @@ -145,7 +145,7 @@ ignore = "0.4.23" image = { version = "^0.25.9", default-features = false } include_dir = "0.7.4" indexmap = "2.12.0" -insta = "1.44.3" +insta = "1.46.0" itertools = "0.14.0" keyring = { version = "3.6", default-features = false } landlock = "0.4.4" @@ -160,12 +160,12 @@ notify = "8.2.0" nucleo-matcher = "0.3.1" once_cell = "1.20.2" openssl-sys = "*" -opentelemetry = "0.30.0" -opentelemetry-appender-tracing = "0.30.0" -opentelemetry-otlp = "0.30.0" -opentelemetry-semantic-conventions = "0.30.0" -opentelemetry_sdk = "0.30.0" -tracing-opentelemetry = "0.31.0" +opentelemetry = "0.31.0" +opentelemetry-appender-tracing = "0.31.0" +opentelemetry-otlp = "0.31.0" +opentelemetry-semantic-conventions = "0.31.0" +opentelemetry_sdk = "0.31.0" +tracing-opentelemetry = "0.32.0" os_info = "3.12.0" owo-colors = "4.2.0" path-absolutize = "3.1.1" @@ -176,6 +176,7 @@ pretty_assertions = "1.4.1" pulldown-cmark = "0.10" rand = "0.9" ratatui = "0.29.0" +ratatui-core = "0.1.0" ratatui-macros = "0.6.0" regex = "1.12.2" regex-lite = "0.1.8" @@ -206,12 +207,11 @@ thiserror = "2.0.17" time = "0.3" tiny_http = "0.12" tokio = "1" -tokio-stream = "0.1.17" +tokio-stream = "0.1.18" tokio-test = "0.4" tokio-util = "0.7.16" toml = "0.9.5" toml_edit = "0.24.0" -tonic = "0.13.1" tracing = "0.1.43" tracing-appender = "0.2.3" tracing-subscriber = "0.3.22" @@ -220,6 +220,7 @@ tree-sitter = "0.25.10" tree-sitter-bash = "0.25" tree-sitter-highlight = "0.25.10" ts-rs = "11" +tui-scrollbar = "0.2.1" uds_windows = "1.1.0" unicode-segmentation = "1.12.0" unicode-width = "0.2" diff --git a/codex-rs/README.md b/codex-rs/README.md index a3d1b82fb8..cbe1fe3779 100644 --- a/codex-rs/README.md +++ b/codex-rs/README.md @@ -15,8 +15,8 @@ You can also install via Homebrew (`brew install --cask codex`) or download a pl ## Documentation quickstart -- First run with Codex? Follow the walkthrough in [`docs/getting-started.md`](../docs/getting-started.md) for prompts, keyboard shortcuts, and session management. -- Already shipping with Codex and want deeper control? Jump to [`docs/advanced.md`](../docs/advanced.md) and the configuration reference at [`docs/config.md`](../docs/config.md). +- First run with Codex? Start with [`docs/getting-started.md`](../docs/getting-started.md) (links to the walkthrough for prompts, keyboard shortcuts, and session management). +- Want deeper control? See [`docs/config.md`](../docs/config.md) and [`docs/install.md`](../docs/install.md). ## What's new in the Rust CLI @@ -30,7 +30,7 @@ Codex supports a rich set of configuration options. Note that the Rust CLI uses #### MCP client -Codex CLI functions as an MCP client that allows the Codex CLI and IDE extension to connect to MCP servers on startup. See the [`configuration documentation`](../docs/config.md#mcp_servers) for details. +Codex CLI functions as an MCP client that allows the Codex CLI and IDE extension to connect to MCP servers on startup. See the [`configuration documentation`](../docs/config.md#connecting-to-mcp-servers) for details. #### MCP server (experimental) diff --git a/codex-rs/app-server-protocol/src/protocol/v1.rs b/codex-rs/app-server-protocol/src/protocol/v1.rs index df39f8809e..8aad35e41c 100644 --- a/codex-rs/app-server-protocol/src/protocol/v1.rs +++ b/codex-rs/app-server-protocol/src/protocol/v1.rs @@ -384,6 +384,8 @@ pub struct SendUserTurnParams { pub model: String, pub effort: Option, pub summary: ReasoningSummary, + /// Optional JSON Schema used to constrain the final assistant message for this turn. + pub output_schema: Option, } #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema, TS)] diff --git a/codex-rs/app-server-protocol/src/protocol/v2.rs b/codex-rs/app-server-protocol/src/protocol/v2.rs index 7f09216eab..13a6545359 100644 --- a/codex-rs/app-server-protocol/src/protocol/v2.rs +++ b/codex-rs/app-server-protocol/src/protocol/v2.rs @@ -1319,6 +1319,8 @@ pub struct TurnStartParams { pub effort: Option, /// Override the reasoning summary for this turn and subsequent turns. pub summary: Option, + /// Optional JSON Schema used to constrain the final assistant message for this turn. + pub output_schema: Option, } #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema, TS)] diff --git a/codex-rs/app-server/README.md b/codex-rs/app-server/README.md index 787ec398d3..267d7577e5 100644 --- a/codex-rs/app-server/README.md +++ b/codex-rs/app-server/README.md @@ -162,7 +162,7 @@ Turns attach user input (text or images) to a thread and trigger Codex generatio - `{"type":"image","url":"https://…png"}` - `{"type":"localImage","path":"/tmp/screenshot.png"}` -You can optionally specify config overrides on the new turn. If specified, these settings become the default for subsequent turns on the same thread. +You can optionally specify config overrides on the new turn. If specified, these settings become the default for subsequent turns on the same thread. `outputSchema` applies only to the current turn. ```json { "method": "turn/start", "id": 30, "params": { @@ -178,7 +178,14 @@ You can optionally specify config overrides on the new turn. If specified, these }, "model": "gpt-5.1-codex", "effort": "medium", - "summary": "concise" + "summary": "concise", + // Optional JSON Schema to constrain the final assistant message for this turn. + "outputSchema": { + "type": "object", + "properties": { "answer": { "type": "string" } }, + "required": ["answer"], + "additionalProperties": false + } } } { "id": 30, "result": { "turn": { "id": "turn_456", diff --git a/codex-rs/app-server/src/codex_message_processor.rs b/codex-rs/app-server/src/codex_message_processor.rs index d1804801d5..f5e9fce59e 100644 --- a/codex-rs/app-server/src/codex_message_processor.rs +++ b/codex-rs/app-server/src/codex_message_processor.rs @@ -2579,6 +2579,7 @@ impl CodexMessageProcessor { let _ = conversation .submit(Op::UserInput { items: mapped_items, + final_output_json_schema: None, }) .await; @@ -2598,6 +2599,7 @@ impl CodexMessageProcessor { model, effort, summary, + output_schema, } = params; let Ok(conversation) = self @@ -2632,7 +2634,7 @@ impl CodexMessageProcessor { model, effort, summary, - final_output_json_schema: None, + final_output_json_schema: output_schema, }) .await; @@ -2741,6 +2743,7 @@ impl CodexMessageProcessor { let turn_id = conversation .submit(Op::UserInput { items: mapped_items, + final_output_json_schema: params.output_schema, }) .await; diff --git a/codex-rs/app-server/src/lib.rs b/codex-rs/app-server/src/lib.rs index 622672dc16..224e0da10b 100644 --- a/codex-rs/app-server/src/lib.rs +++ b/codex-rs/app-server/src/lib.rs @@ -17,13 +17,11 @@ use tokio::io::BufReader; use tokio::io::{self}; use tokio::sync::mpsc; use toml::Value as TomlValue; -use tracing::Level; use tracing::debug; use tracing::error; use tracing::info; use tracing_subscriber::EnvFilter; use tracing_subscriber::Layer; -use tracing_subscriber::filter::Targets; use tracing_subscriber::layer::SubscriberExt; use tracing_subscriber::util::SubscriberInitExt; @@ -103,11 +101,8 @@ pub async fn run_main( .with_span_events(tracing_subscriber::fmt::format::FmtSpan::FULL) .with_filter(EnvFilter::from_default_env()); - let feedback_layer = tracing_subscriber::fmt::layer() - .with_writer(feedback.make_writer()) - .with_ansi(false) - .with_target(false) - .with_filter(Targets::new().with_default(Level::TRACE)); + let feedback_layer = feedback.logger_layer(); + let feedback_metadata_layer = feedback.metadata_layer(); let otel_logger_layer = otel.as_ref().and_then(|o| o.logger_layer()); @@ -116,6 +111,7 @@ pub async fn run_main( let _ = tracing_subscriber::registry() .with(stderr_fmt) .with(feedback_layer) + .with(feedback_metadata_layer) .with(otel_logger_layer) .with(otel_tracing_layer) .try_init(); diff --git a/codex-rs/app-server/tests/suite/codex_message_processor_flow.rs b/codex-rs/app-server/tests/suite/codex_message_processor_flow.rs index be94dd822e..c044e1c4cf 100644 --- a/codex-rs/app-server/tests/suite/codex_message_processor_flow.rs +++ b/codex-rs/app-server/tests/suite/codex_message_processor_flow.rs @@ -305,6 +305,7 @@ async fn test_send_user_turn_changes_approval_policy_behavior() -> Result<()> { model: "mock-model".to_string(), effort: Some(ReasoningEffort::Medium), summary: ReasoningSummary::Auto, + output_schema: None, }) .await?; // Acknowledge sendUserTurn @@ -418,6 +419,7 @@ async fn test_send_user_turn_updates_sandbox_and_cwd_between_turns() -> Result<( model: model.clone(), effort: Some(ReasoningEffort::Medium), summary: ReasoningSummary::Auto, + output_schema: None, }) .await?; timeout( @@ -443,6 +445,7 @@ async fn test_send_user_turn_updates_sandbox_and_cwd_between_turns() -> Result<( model: model.clone(), effort: Some(ReasoningEffort::Medium), summary: ReasoningSummary::Auto, + output_schema: None, }) .await?; timeout( diff --git a/codex-rs/app-server/tests/suite/mod.rs b/codex-rs/app-server/tests/suite/mod.rs index 37f7659f42..916a8f5a72 100644 --- a/codex-rs/app-server/tests/suite/mod.rs +++ b/codex-rs/app-server/tests/suite/mod.rs @@ -7,6 +7,7 @@ mod fuzzy_file_search; mod interrupt; mod list_resume; mod login; +mod output_schema; mod send_message; mod set_default_model; mod user_agent; diff --git a/codex-rs/app-server/tests/suite/output_schema.rs b/codex-rs/app-server/tests/suite/output_schema.rs new file mode 100644 index 0000000000..4ec500a245 --- /dev/null +++ b/codex-rs/app-server/tests/suite/output_schema.rs @@ -0,0 +1,282 @@ +use anyhow::Result; +use app_test_support::McpProcess; +use app_test_support::to_response; +use codex_app_server_protocol::AddConversationListenerParams; +use codex_app_server_protocol::InputItem; +use codex_app_server_protocol::JSONRPCResponse; +use codex_app_server_protocol::NewConversationParams; +use codex_app_server_protocol::NewConversationResponse; +use codex_app_server_protocol::RequestId; +use codex_app_server_protocol::SendUserTurnParams; +use codex_app_server_protocol::SendUserTurnResponse; +use codex_core::protocol::AskForApproval; +use codex_core::protocol::SandboxPolicy; +use codex_protocol::config_types::ReasoningSummary; +use codex_protocol::openai_models::ReasoningEffort; +use core_test_support::responses; +use core_test_support::skip_if_no_network; +use pretty_assertions::assert_eq; +use std::path::Path; +use tempfile::TempDir; +use tokio::time::timeout; + +const DEFAULT_READ_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(10); + +#[tokio::test] +async fn send_user_turn_accepts_output_schema_v1() -> Result<()> { + skip_if_no_network!(Ok(())); + + let server = responses::start_mock_server().await; + let body = responses::sse(vec![ + responses::ev_response_created("resp-1"), + responses::ev_assistant_message("msg-1", "Done"), + responses::ev_completed("resp-1"), + ]); + let response_mock = responses::mount_sse_once(&server, body).await; + + let codex_home = TempDir::new()?; + create_config_toml(codex_home.path(), &server.uri())?; + + let mut mcp = McpProcess::new(codex_home.path()).await?; + timeout(DEFAULT_READ_TIMEOUT, mcp.initialize()).await??; + + let new_conv_id = mcp + .send_new_conversation_request(NewConversationParams { + ..Default::default() + }) + .await?; + let new_conv_resp: JSONRPCResponse = timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_response_message(RequestId::Integer(new_conv_id)), + ) + .await??; + let NewConversationResponse { + conversation_id, .. + } = to_response::(new_conv_resp)?; + + let listener_id = mcp + .send_add_conversation_listener_request(AddConversationListenerParams { + conversation_id, + experimental_raw_events: false, + }) + .await?; + timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_response_message(RequestId::Integer(listener_id)), + ) + .await??; + + let output_schema = serde_json::json!({ + "type": "object", + "properties": { + "answer": { "type": "string" } + }, + "required": ["answer"], + "additionalProperties": false + }); + + let send_turn_id = mcp + .send_send_user_turn_request(SendUserTurnParams { + conversation_id, + items: vec![InputItem::Text { + text: "Hello".to_string(), + }], + cwd: codex_home.path().to_path_buf(), + approval_policy: AskForApproval::Never, + sandbox_policy: SandboxPolicy::new_read_only_policy(), + model: "mock-model".to_string(), + effort: Some(ReasoningEffort::Medium), + summary: ReasoningSummary::Auto, + output_schema: Some(output_schema.clone()), + }) + .await?; + let _send_turn_resp: SendUserTurnResponse = to_response::( + timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_response_message(RequestId::Integer(send_turn_id)), + ) + .await??, + )?; + + timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_notification_message("codex/event/task_complete"), + ) + .await??; + + let request = response_mock.single_request(); + let payload = request.body_json(); + let text = payload.get("text").expect("request missing text field"); + let format = text + .get("format") + .expect("request missing text.format field"); + assert_eq!( + format, + &serde_json::json!({ + "name": "codex_output_schema", + "type": "json_schema", + "strict": true, + "schema": output_schema, + }) + ); + + Ok(()) +} + +#[tokio::test] +async fn send_user_turn_output_schema_is_per_turn_v1() -> Result<()> { + skip_if_no_network!(Ok(())); + + let server = responses::start_mock_server().await; + let body1 = responses::sse(vec![ + responses::ev_response_created("resp-1"), + responses::ev_assistant_message("msg-1", "Done"), + responses::ev_completed("resp-1"), + ]); + let response_mock1 = responses::mount_sse_once(&server, body1).await; + + let codex_home = TempDir::new()?; + create_config_toml(codex_home.path(), &server.uri())?; + + let mut mcp = McpProcess::new(codex_home.path()).await?; + timeout(DEFAULT_READ_TIMEOUT, mcp.initialize()).await??; + + let new_conv_id = mcp + .send_new_conversation_request(NewConversationParams { + ..Default::default() + }) + .await?; + let new_conv_resp: JSONRPCResponse = timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_response_message(RequestId::Integer(new_conv_id)), + ) + .await??; + let NewConversationResponse { + conversation_id, .. + } = to_response::(new_conv_resp)?; + + let listener_id = mcp + .send_add_conversation_listener_request(AddConversationListenerParams { + conversation_id, + experimental_raw_events: false, + }) + .await?; + timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_response_message(RequestId::Integer(listener_id)), + ) + .await??; + + let output_schema = serde_json::json!({ + "type": "object", + "properties": { + "answer": { "type": "string" } + }, + "required": ["answer"], + "additionalProperties": false + }); + + let send_turn_id = mcp + .send_send_user_turn_request(SendUserTurnParams { + conversation_id, + items: vec![InputItem::Text { + text: "Hello".to_string(), + }], + cwd: codex_home.path().to_path_buf(), + approval_policy: AskForApproval::Never, + sandbox_policy: SandboxPolicy::new_read_only_policy(), + model: "mock-model".to_string(), + effort: Some(ReasoningEffort::Medium), + summary: ReasoningSummary::Auto, + output_schema: Some(output_schema.clone()), + }) + .await?; + let _send_turn_resp: SendUserTurnResponse = to_response::( + timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_response_message(RequestId::Integer(send_turn_id)), + ) + .await??, + )?; + + timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_notification_message("codex/event/task_complete"), + ) + .await??; + + let payload1 = response_mock1.single_request().body_json(); + assert_eq!( + payload1.pointer("/text/format"), + Some(&serde_json::json!({ + "name": "codex_output_schema", + "type": "json_schema", + "strict": true, + "schema": output_schema, + })) + ); + + let body2 = responses::sse(vec![ + responses::ev_response_created("resp-2"), + responses::ev_assistant_message("msg-2", "Done"), + responses::ev_completed("resp-2"), + ]); + let response_mock2 = responses::mount_sse_once(&server, body2).await; + + let send_turn_id_2 = mcp + .send_send_user_turn_request(SendUserTurnParams { + conversation_id, + items: vec![InputItem::Text { + text: "Hello again".to_string(), + }], + cwd: codex_home.path().to_path_buf(), + approval_policy: AskForApproval::Never, + sandbox_policy: SandboxPolicy::new_read_only_policy(), + model: "mock-model".to_string(), + effort: Some(ReasoningEffort::Medium), + summary: ReasoningSummary::Auto, + output_schema: None, + }) + .await?; + let _send_turn_resp_2: SendUserTurnResponse = to_response::( + timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_response_message(RequestId::Integer(send_turn_id_2)), + ) + .await??, + )?; + + timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_notification_message("codex/event/task_complete"), + ) + .await??; + + let payload2 = response_mock2.single_request().body_json(); + assert_eq!(payload2.pointer("/text/format"), None); + + Ok(()) +} + +fn create_config_toml(codex_home: &Path, server_uri: &str) -> std::io::Result<()> { + let config_toml = codex_home.join("config.toml"); + std::fs::write( + config_toml, + format!( + r#" +model = "mock-model" +approval_policy = "never" +sandbox_mode = "read-only" + +model_provider = "mock_provider" + +[model_providers.mock_provider] +name = "Mock provider for test" +base_url = "{server_uri}/v1" +wire_api = "responses" +request_max_retries = 0 +stream_max_retries = 0 +"# + ), + ) +} diff --git a/codex-rs/app-server/tests/suite/v2/mod.rs b/codex-rs/app-server/tests/suite/v2/mod.rs index 16d2142b2e..f23f792d39 100644 --- a/codex-rs/app-server/tests/suite/v2/mod.rs +++ b/codex-rs/app-server/tests/suite/v2/mod.rs @@ -1,6 +1,7 @@ mod account; mod config_rpc; mod model_list; +mod output_schema; mod rate_limits; mod review; mod thread_archive; diff --git a/codex-rs/app-server/tests/suite/v2/output_schema.rs b/codex-rs/app-server/tests/suite/v2/output_schema.rs new file mode 100644 index 0000000000..f23c037037 --- /dev/null +++ b/codex-rs/app-server/tests/suite/v2/output_schema.rs @@ -0,0 +1,231 @@ +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::ThreadStartParams; +use codex_app_server_protocol::ThreadStartResponse; +use codex_app_server_protocol::TurnStartParams; +use codex_app_server_protocol::TurnStartResponse; +use codex_app_server_protocol::UserInput as V2UserInput; +use core_test_support::responses; +use core_test_support::skip_if_no_network; +use pretty_assertions::assert_eq; +use std::path::Path; +use tempfile::TempDir; +use tokio::time::timeout; + +const DEFAULT_READ_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(10); + +#[tokio::test] +async fn turn_start_accepts_output_schema_v2() -> Result<()> { + skip_if_no_network!(Ok(())); + + let server = responses::start_mock_server().await; + let body = responses::sse(vec![ + responses::ev_response_created("resp-1"), + responses::ev_assistant_message("msg-1", "Done"), + responses::ev_completed("resp-1"), + ]); + let response_mock = responses::mount_sse_once(&server, body).await; + + let codex_home = TempDir::new()?; + create_config_toml(codex_home.path(), &server.uri())?; + + let mut mcp = McpProcess::new(codex_home.path()).await?; + timeout(DEFAULT_READ_TIMEOUT, mcp.initialize()).await??; + + let thread_req = mcp + .send_thread_start_request(ThreadStartParams { + ..Default::default() + }) + .await?; + let thread_resp: JSONRPCResponse = timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_response_message(RequestId::Integer(thread_req)), + ) + .await??; + let ThreadStartResponse { thread, .. } = to_response::(thread_resp)?; + + let output_schema = serde_json::json!({ + "type": "object", + "properties": { + "answer": { "type": "string" } + }, + "required": ["answer"], + "additionalProperties": false + }); + + let turn_req = mcp + .send_turn_start_request(TurnStartParams { + thread_id: thread.id.clone(), + input: vec![V2UserInput::Text { + text: "Hello".to_string(), + }], + output_schema: Some(output_schema.clone()), + ..Default::default() + }) + .await?; + let turn_resp: JSONRPCResponse = timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_response_message(RequestId::Integer(turn_req)), + ) + .await??; + let _turn: TurnStartResponse = to_response::(turn_resp)?; + + timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_notification_message("turn/completed"), + ) + .await??; + + let request = response_mock.single_request(); + let payload = request.body_json(); + let text = payload.get("text").expect("request missing text field"); + let format = text + .get("format") + .expect("request missing text.format field"); + assert_eq!( + format, + &serde_json::json!({ + "name": "codex_output_schema", + "type": "json_schema", + "strict": true, + "schema": output_schema, + }) + ); + + Ok(()) +} + +#[tokio::test] +async fn turn_start_output_schema_is_per_turn_v2() -> Result<()> { + skip_if_no_network!(Ok(())); + + let server = responses::start_mock_server().await; + let body1 = responses::sse(vec![ + responses::ev_response_created("resp-1"), + responses::ev_assistant_message("msg-1", "Done"), + responses::ev_completed("resp-1"), + ]); + let response_mock1 = responses::mount_sse_once(&server, body1).await; + + let codex_home = TempDir::new()?; + create_config_toml(codex_home.path(), &server.uri())?; + + let mut mcp = McpProcess::new(codex_home.path()).await?; + timeout(DEFAULT_READ_TIMEOUT, mcp.initialize()).await??; + + let thread_req = mcp + .send_thread_start_request(ThreadStartParams { + ..Default::default() + }) + .await?; + let thread_resp: JSONRPCResponse = timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_response_message(RequestId::Integer(thread_req)), + ) + .await??; + let ThreadStartResponse { thread, .. } = to_response::(thread_resp)?; + + let output_schema = serde_json::json!({ + "type": "object", + "properties": { + "answer": { "type": "string" } + }, + "required": ["answer"], + "additionalProperties": false + }); + + let turn_req_1 = mcp + .send_turn_start_request(TurnStartParams { + thread_id: thread.id.clone(), + input: vec![V2UserInput::Text { + text: "Hello".to_string(), + }], + output_schema: Some(output_schema.clone()), + ..Default::default() + }) + .await?; + let turn_resp_1: JSONRPCResponse = timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_response_message(RequestId::Integer(turn_req_1)), + ) + .await??; + let _turn: TurnStartResponse = to_response::(turn_resp_1)?; + + timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_notification_message("turn/completed"), + ) + .await??; + + let payload1 = response_mock1.single_request().body_json(); + assert_eq!( + payload1.pointer("/text/format"), + Some(&serde_json::json!({ + "name": "codex_output_schema", + "type": "json_schema", + "strict": true, + "schema": output_schema, + })) + ); + + let body2 = responses::sse(vec![ + responses::ev_response_created("resp-2"), + responses::ev_assistant_message("msg-2", "Done"), + responses::ev_completed("resp-2"), + ]); + let response_mock2 = responses::mount_sse_once(&server, body2).await; + + let turn_req_2 = mcp + .send_turn_start_request(TurnStartParams { + thread_id: thread.id.clone(), + input: vec![V2UserInput::Text { + text: "Hello again".to_string(), + }], + output_schema: None, + ..Default::default() + }) + .await?; + let turn_resp_2: JSONRPCResponse = timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_response_message(RequestId::Integer(turn_req_2)), + ) + .await??; + let _turn: TurnStartResponse = to_response::(turn_resp_2)?; + + timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_notification_message("turn/completed"), + ) + .await??; + + let payload2 = response_mock2.single_request().body_json(); + assert_eq!(payload2.pointer("/text/format"), None); + + Ok(()) +} + +fn create_config_toml(codex_home: &Path, server_uri: &str) -> std::io::Result<()> { + let config_toml = codex_home.join("config.toml"); + std::fs::write( + config_toml, + format!( + r#" +model = "mock-model" +approval_policy = "never" +sandbox_mode = "read-only" + +model_provider = "mock_provider" + +[model_providers.mock_provider] +name = "Mock provider for test" +base_url = "{server_uri}/v1" +wire_api = "responses" +request_max_retries = 0 +stream_max_retries = 0 +"# + ), + ) +} diff --git a/codex-rs/app-server/tests/suite/v2/turn_start.rs b/codex-rs/app-server/tests/suite/v2/turn_start.rs index 1948487d14..60fa656b19 100644 --- a/codex-rs/app-server/tests/suite/v2/turn_start.rs +++ b/codex-rs/app-server/tests/suite/v2/turn_start.rs @@ -540,6 +540,7 @@ async fn turn_start_updates_sandbox_and_cwd_between_turns_v2() -> Result<()> { model: Some("mock-model".to_string()), effort: Some(ReasoningEffort::Medium), summary: Some(ReasoningSummary::Auto), + output_schema: None, }) .await?; timeout( @@ -566,6 +567,7 @@ async fn turn_start_updates_sandbox_and_cwd_between_turns_v2() -> Result<()> { model: Some("mock-model".to_string()), effort: Some(ReasoningEffort::Medium), summary: Some(ReasoningSummary::Auto), + output_schema: None, }) .await?; timeout( diff --git a/codex-rs/codex-api/src/requests/chat.rs b/codex-rs/codex-api/src/requests/chat.rs index d5ac188efc..60f450ca0d 100644 --- a/codex-rs/codex-api/src/requests/chat.rs +++ b/codex-rs/codex-api/src/requests/chat.rs @@ -204,24 +204,16 @@ impl<'a> ChatRequestBuilder<'a> { call_id, .. } => { - let mut msg = json!({ - "role": "assistant", - "content": null, - "tool_calls": [{ - "id": call_id, - "type": "function", - "function": { - "name": name, - "arguments": arguments, - } - }] + let reasoning = reasoning_by_anchor_index.get(&idx).map(String::as_str); + let tool_call = json!({ + "id": call_id, + "type": "function", + "function": { + "name": name, + "arguments": arguments, + } }); - if let Some(reasoning) = reasoning_by_anchor_index.get(&idx) - && let Some(obj) = msg.as_object_mut() - { - obj.insert("reasoning".to_string(), json!(reasoning)); - } - messages.push(msg); + push_tool_call_message(&mut messages, tool_call, reasoning); } ResponseItem::LocalShellCall { id, @@ -229,22 +221,14 @@ impl<'a> ChatRequestBuilder<'a> { status, action, } => { - let mut msg = json!({ - "role": "assistant", - "content": null, - "tool_calls": [{ - "id": id.clone().unwrap_or_default(), - "type": "local_shell_call", - "status": status, - "action": action, - }] + let reasoning = reasoning_by_anchor_index.get(&idx).map(String::as_str); + let tool_call = json!({ + "id": id.clone().unwrap_or_default(), + "type": "local_shell_call", + "status": status, + "action": action, }); - if let Some(reasoning) = reasoning_by_anchor_index.get(&idx) - && let Some(obj) = msg.as_object_mut() - { - obj.insert("reasoning".to_string(), json!(reasoning)); - } - messages.push(msg); + push_tool_call_message(&mut messages, tool_call, reasoning); } ResponseItem::FunctionCallOutput { call_id, output } => { let content_value = if let Some(items) = &output.content_items { @@ -277,18 +261,16 @@ impl<'a> ChatRequestBuilder<'a> { input, status: _, } => { - messages.push(json!({ - "role": "assistant", - "content": null, - "tool_calls": [{ - "id": id, - "type": "custom", - "custom": { - "name": name, - "input": input, - } - }] - })); + let tool_call = json!({ + "id": id, + "type": "custom", + "custom": { + "name": name, + "input": input, + } + }); + let reasoning = reasoning_by_anchor_index.get(&idx).map(String::as_str); + push_tool_call_message(&mut messages, tool_call, reasoning); } ResponseItem::CustomToolCallOutput { call_id, output } => { messages.push(json!({ @@ -328,11 +310,50 @@ impl<'a> ChatRequestBuilder<'a> { } } +fn push_tool_call_message(messages: &mut Vec, tool_call: Value, reasoning: Option<&str>) { + // Chat Completions requires that tool calls are grouped into a single assistant message + // (with `tool_calls: [...]`) followed by tool role responses. + if let Some(Value::Object(obj)) = messages.last_mut() + && obj.get("role").and_then(Value::as_str) == Some("assistant") + && obj.get("content").is_some_and(Value::is_null) + && let Some(tool_calls) = obj.get_mut("tool_calls").and_then(Value::as_array_mut) + { + tool_calls.push(tool_call); + if let Some(reasoning) = reasoning { + if let Some(Value::String(existing)) = obj.get_mut("reasoning") { + if !existing.is_empty() { + existing.push('\n'); + } + existing.push_str(reasoning); + } else { + obj.insert( + "reasoning".to_string(), + Value::String(reasoning.to_string()), + ); + } + } + return; + } + + let mut msg = json!({ + "role": "assistant", + "content": null, + "tool_calls": [tool_call], + }); + if let Some(reasoning) = reasoning + && let Some(obj) = msg.as_object_mut() + { + obj.insert("reasoning".to_string(), json!(reasoning)); + } + messages.push(msg); +} + #[cfg(test)] mod tests { use super::*; use crate::provider::RetryConfig; use crate::provider::WireApi; + use codex_protocol::models::FunctionCallOutputPayload; use codex_protocol::protocol::SessionSource; use codex_protocol::protocol::SubAgentSource; use http::HeaderValue; @@ -385,4 +406,89 @@ mod tests { Some(&HeaderValue::from_static("review")) ); } + + #[test] + fn groups_consecutive_tool_calls_into_a_single_assistant_message() { + let prompt_input = vec![ + ResponseItem::Message { + id: None, + role: "user".to_string(), + content: vec![ContentItem::InputText { + text: "read these".to_string(), + }], + }, + ResponseItem::FunctionCall { + id: None, + name: "read_file".to_string(), + arguments: r#"{"path":"a.txt"}"#.to_string(), + call_id: "call-a".to_string(), + }, + ResponseItem::FunctionCall { + id: None, + name: "read_file".to_string(), + arguments: r#"{"path":"b.txt"}"#.to_string(), + call_id: "call-b".to_string(), + }, + ResponseItem::FunctionCall { + id: None, + name: "read_file".to_string(), + arguments: r#"{"path":"c.txt"}"#.to_string(), + call_id: "call-c".to_string(), + }, + ResponseItem::FunctionCallOutput { + call_id: "call-a".to_string(), + output: FunctionCallOutputPayload { + content: "A".to_string(), + ..Default::default() + }, + }, + ResponseItem::FunctionCallOutput { + call_id: "call-b".to_string(), + output: FunctionCallOutputPayload { + content: "B".to_string(), + ..Default::default() + }, + }, + ResponseItem::FunctionCallOutput { + call_id: "call-c".to_string(), + output: FunctionCallOutputPayload { + content: "C".to_string(), + ..Default::default() + }, + }, + ]; + + let req = ChatRequestBuilder::new("gpt-test", "inst", &prompt_input, &[]) + .build(&provider()) + .expect("request"); + + let messages = req + .body + .get("messages") + .and_then(|v| v.as_array()) + .expect("messages array"); + // system + user + assistant(tool_calls=[...]) + 3 tool outputs + assert_eq!(messages.len(), 6); + + assert_eq!(messages[0]["role"], "system"); + assert_eq!(messages[1]["role"], "user"); + + let tool_calls_msg = &messages[2]; + assert_eq!(tool_calls_msg["role"], "assistant"); + assert_eq!(tool_calls_msg["content"], serde_json::Value::Null); + let tool_calls = tool_calls_msg["tool_calls"] + .as_array() + .expect("tool_calls array"); + assert_eq!(tool_calls.len(), 3); + assert_eq!(tool_calls[0]["id"], "call-a"); + assert_eq!(tool_calls[1]["id"], "call-b"); + assert_eq!(tool_calls[2]["id"], "call-c"); + + assert_eq!(messages[3]["role"], "tool"); + assert_eq!(messages[3]["tool_call_id"], "call-a"); + assert_eq!(messages[4]["role"], "tool"); + assert_eq!(messages[4]["tool_call_id"], "call-b"); + assert_eq!(messages[5]["role"], "tool"); + assert_eq!(messages[5]["tool_call_id"], "call-c"); + } } diff --git a/codex-rs/codex-api/src/sse/chat.rs b/codex-rs/codex-api/src/sse/chat.rs index 21adfa571a..dec35890b7 100644 --- a/codex-rs/codex-api/src/sse/chat.rs +++ b/codex-rs/codex-api/src/sse/chat.rs @@ -30,6 +30,21 @@ pub(crate) fn spawn_chat_stream( ResponseStream { rx_event } } +/// Processes Server-Sent Events from the legacy Chat Completions streaming API. +/// +/// The upstream protocol terminates a streaming response with a final sentinel event +/// (`data: [DONE]`). Historically, some of our test stubs have emitted `data: DONE` +/// (without brackets) instead. +/// +/// `eventsource_stream` delivers these sentinels as regular events rather than signaling +/// end-of-stream. If we try to parse them as JSON, we log and skip them, then keep +/// polling for more events. +/// +/// On servers that keep the HTTP connection open after emitting the sentinel (notably +/// wiremock on Windows), skipping the sentinel means we never emit `ResponseEvent::Completed`. +/// Higher-level workflows/tests that wait for completion before issuing subsequent model +/// calls will then stall, which shows up as "expected N requests, got 1" verification +/// failures in the mock server. pub async fn process_chat_sse( stream: S, tx_event: mpsc::Sender>, @@ -57,6 +72,31 @@ pub async fn process_chat_sse( let mut reasoning_item: Option = None; let mut completed_sent = false; + async fn flush_and_complete( + tx_event: &mpsc::Sender>, + reasoning_item: &mut Option, + assistant_item: &mut Option, + ) { + if let Some(reasoning) = reasoning_item.take() { + let _ = tx_event + .send(Ok(ResponseEvent::OutputItemDone(reasoning))) + .await; + } + + if let Some(assistant) = assistant_item.take() { + let _ = tx_event + .send(Ok(ResponseEvent::OutputItemDone(assistant))) + .await; + } + + let _ = tx_event + .send(Ok(ResponseEvent::Completed { + response_id: String::new(), + token_usage: None, + })) + .await; + } + loop { let start = Instant::now(); let response = timeout(idle_timeout, stream.next()).await; @@ -70,24 +110,8 @@ pub async fn process_chat_sse( return; } Ok(None) => { - if let Some(reasoning) = reasoning_item { - let _ = tx_event - .send(Ok(ResponseEvent::OutputItemDone(reasoning))) - .await; - } - - if let Some(assistant) = assistant_item { - let _ = tx_event - .send(Ok(ResponseEvent::OutputItemDone(assistant))) - .await; - } if !completed_sent { - let _ = tx_event - .send(Ok(ResponseEvent::Completed { - response_id: String::new(), - token_usage: None, - })) - .await; + flush_and_complete(&tx_event, &mut reasoning_item, &mut assistant_item).await; } return; } @@ -101,16 +125,25 @@ pub async fn process_chat_sse( trace!("SSE event: {}", sse.data); - if sse.data.trim().is_empty() { + let data = sse.data.trim(); + + if data.is_empty() { continue; } - let value: serde_json::Value = match serde_json::from_str(&sse.data) { + if data == "[DONE]" || data == "DONE" { + if !completed_sent { + flush_and_complete(&tx_event, &mut reasoning_item, &mut assistant_item).await; + } + return; + } + + let value: serde_json::Value = match serde_json::from_str(data) { Ok(val) => val, Err(err) => { debug!( "Failed to parse ChatCompletions SSE event: {err}, data: {}", - &sse.data + data ); continue; } @@ -362,6 +395,16 @@ mod tests { body } + /// Regression test: the stream should complete when we see a `[DONE]` sentinel. + /// + /// This is important for tests/mocks that don't immediately close the underlying + /// connection after emitting the sentinel. + #[tokio::test] + async fn completes_on_done_sentinel_without_json() { + let events = collect_events("event: message\ndata: [DONE]\n\n").await; + assert_matches!(&events[..], [ResponseEvent::Completed { .. }]); + } + async fn collect_events(body: &str) -> Vec { let reader = ReaderStream::new(std::io::Cursor::new(body.to_string())) .map_err(|err| codex_client::TransportError::Network(err.to_string())); diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index 9372cab575..e853a2e384 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -88,6 +88,7 @@ use crate::error::Result as CodexResult; #[cfg(test)] use crate::exec::StreamOutput; use crate::exec_policy::ExecPolicyUpdateError; +use crate::feedback_tags; use crate::mcp::auth::compute_auth_statuses; use crate::mcp_connection_manager::McpConnectionManager; use crate::model_provider_info::CHAT_WIRE_API_DEPRECATION_SUMMARY; @@ -810,6 +811,13 @@ impl Session { .await; } + // Seed usage info from the recorded rollout so UIs can show token counts + // immediately on resume/fork. + if let Some(info) = Self::last_token_info_from_rollout(&rollout_items) { + let mut state = self.state.lock().await; + state.set_token_info(Some(info)); + } + // If persisting, persist all rollout items as-is (recorder filters) if persist && !rollout_items.is_empty() { self.persist_rollout_items(&rollout_items).await; @@ -820,6 +828,13 @@ impl Session { } } + fn last_token_info_from_rollout(rollout_items: &[RolloutItem]) -> Option { + rollout_items.iter().rev().find_map(|item| match item { + RolloutItem::EventMsg(EventMsg::TokenCount(ev)) => ev.info.clone(), + _ => None, + }) + } + pub(crate) async fn update_settings( &self, updates: SessionSettingsUpdate, @@ -1761,7 +1776,16 @@ mod handlers { final_output_json_schema: Some(final_output_json_schema), }, ), - Op::UserInput { items } => (items, SessionSettingsUpdate::default()), + Op::UserInput { + items, + final_output_json_schema, + } => ( + items, + SessionSettingsUpdate { + final_output_json_schema: Some(final_output_json_schema), + ..Default::default() + }, + ), _ => unreachable!(), }; @@ -2053,7 +2077,7 @@ mod handlers { review_request: ReviewRequest, ) { let turn_context = sess.new_default_turn_with_sub_id(sub_id.clone()).await; - match resolve_review_request(review_request, config.cwd.as_path()) { + match resolve_review_request(review_request, turn_context.cwd.as_path()) { Ok(resolved) => { spawn_review_thread( Arc::clone(sess), @@ -2525,6 +2549,15 @@ async fn try_run_turn( truncation_policy: Some(turn_context.truncation_policy.into()), }); + feedback_tags!( + model = turn_context.client.get_model(), + approval_policy = turn_context.approval_policy, + sandbox_policy = turn_context.sandbox_policy, + effort = turn_context.client.get_reasoning_effort(), + auth_mode = sess.services.auth_manager.get_auth_mode(), + features = sess.features.enabled_features(), + ); + sess.persist_rollout_items(&[rollout_item]).await; let mut stream = turn_context .client @@ -2617,7 +2650,10 @@ async fn try_run_turn( } ResponseEvent::ModelsEtag(etag) => { // Update internal state with latest models etag - sess.services.models_manager.refresh_if_new_etag(etag).await; + sess.services + .models_manager + .refresh_if_new_etag(etag, sess.features.enabled(Feature::RemoteModels)) + .await; } ResponseEvent::Completed { response_id: _, @@ -2759,6 +2795,9 @@ mod tests { use crate::protocol::RateLimitSnapshot; use crate::protocol::RateLimitWindow; use crate::protocol::ResumedHistory; + use crate::protocol::TokenCountEvent; + use crate::protocol::TokenUsage; + use crate::protocol::TokenUsageInfo; use crate::state::TaskKind; use crate::tasks::SessionTask; use crate::tasks::SessionTaskContext; @@ -2813,6 +2852,83 @@ mod tests { assert_eq!(expected, actual); } + #[tokio::test] + async fn record_initial_history_seeds_token_info_from_rollout() { + let (session, turn_context) = make_session_and_context().await; + let (mut rollout_items, _expected) = sample_rollout(&session, &turn_context); + + let info1 = TokenUsageInfo { + total_token_usage: TokenUsage { + input_tokens: 10, + cached_input_tokens: 0, + output_tokens: 20, + reasoning_output_tokens: 0, + total_tokens: 30, + }, + last_token_usage: TokenUsage { + input_tokens: 3, + cached_input_tokens: 0, + output_tokens: 4, + reasoning_output_tokens: 0, + total_tokens: 7, + }, + model_context_window: Some(1_000), + }; + let info2 = TokenUsageInfo { + total_token_usage: TokenUsage { + input_tokens: 100, + cached_input_tokens: 50, + output_tokens: 200, + reasoning_output_tokens: 25, + total_tokens: 375, + }, + last_token_usage: TokenUsage { + input_tokens: 10, + cached_input_tokens: 0, + output_tokens: 20, + reasoning_output_tokens: 5, + total_tokens: 35, + }, + model_context_window: Some(2_000), + }; + + rollout_items.push(RolloutItem::EventMsg(EventMsg::TokenCount( + TokenCountEvent { + info: Some(info1), + rate_limits: None, + }, + ))); + rollout_items.push(RolloutItem::EventMsg(EventMsg::TokenCount( + TokenCountEvent { + info: None, + rate_limits: None, + }, + ))); + rollout_items.push(RolloutItem::EventMsg(EventMsg::TokenCount( + TokenCountEvent { + info: Some(info2.clone()), + rate_limits: None, + }, + ))); + rollout_items.push(RolloutItem::EventMsg(EventMsg::TokenCount( + TokenCountEvent { + info: None, + rate_limits: None, + }, + ))); + + session + .record_initial_history(InitialHistory::Resumed(ResumedHistory { + conversation_id: ConversationId::default(), + history: rollout_items, + rollout_path: PathBuf::from("/tmp/resume.jsonl"), + })) + .await; + + let actual = session.state.lock().await.token_info(); + assert_eq!(actual, Some(info2)); + } + #[tokio::test] async fn record_initial_history_reconstructs_forked_transcript() { let (session, turn_context) = make_session_and_context().await; diff --git a/codex-rs/core/src/codex_delegate.rs b/codex-rs/core/src/codex_delegate.rs index 9d60684b7a..e767abf4ad 100644 --- a/codex-rs/core/src/codex_delegate.rs +++ b/codex-rs/core/src/codex_delegate.rs @@ -118,7 +118,11 @@ pub(crate) async fn run_codex_conversation_one_shot( .await?; // Send the initial input to kick off the one-shot turn. - io.submit(Op::UserInput { items: input }).await?; + io.submit(Op::UserInput { + items: input, + final_output_json_schema: None, + }) + .await?; // Bridge events so we can observe completion and shut down automatically. let (tx_bridge, rx_bridge) = async_channel::bounded(SUBMISSION_CHANNEL_CAPACITY); @@ -184,6 +188,10 @@ async fn forward_events( id: _, msg: EventMsg::AgentMessageDelta(_) | EventMsg::AgentReasoningDelta(_), } => {} + Event { + id: _, + msg: EventMsg::TokenCount(_), + } => {} Event { id: _, msg: EventMsg::SessionConfigured(_), diff --git a/codex-rs/core/src/config/mod.rs b/codex-rs/core/src/config/mod.rs index 53864851ae..8eac13fd2e 100644 --- a/codex-rs/core/src/config/mod.rs +++ b/codex-rs/core/src/config/mod.rs @@ -2050,6 +2050,7 @@ trust_level = "trusted" managed_config_path: Some(managed_path.clone()), #[cfg(target_os = "macos")] managed_preferences_base64: None, + macos_managed_config_requirements_base64: None, }; let cwd = AbsolutePathBuf::try_from(codex_home.path())?; @@ -2170,6 +2171,7 @@ trust_level = "trusted" managed_config_path: Some(managed_path), #[cfg(target_os = "macos")] managed_preferences_base64: None, + macos_managed_config_requirements_base64: None, }; let cwd = AbsolutePathBuf::try_from(codex_home.path())?; diff --git a/codex-rs/core/src/config/service.rs b/codex-rs/core/src/config/service.rs index bc6d96bcb8..211a12fa03 100644 --- a/codex-rs/core/src/config/service.rs +++ b/codex-rs/core/src/config/service.rs @@ -755,6 +755,7 @@ remote_compaction = true managed_config_path: Some(managed_path.clone()), #[cfg(target_os = "macos")] managed_preferences_base64: None, + macos_managed_config_requirements_base64: None, }, ); @@ -835,6 +836,7 @@ remote_compaction = true managed_config_path: Some(managed_path.clone()), #[cfg(target_os = "macos")] managed_preferences_base64: None, + macos_managed_config_requirements_base64: None, }, ); @@ -937,6 +939,7 @@ remote_compaction = true managed_config_path: Some(managed_path.clone()), #[cfg(target_os = "macos")] managed_preferences_base64: None, + macos_managed_config_requirements_base64: None, }, ); @@ -984,6 +987,7 @@ remote_compaction = true managed_config_path: Some(managed_path.clone()), #[cfg(target_os = "macos")] managed_preferences_base64: None, + macos_managed_config_requirements_base64: None, }, ); @@ -1029,6 +1033,7 @@ remote_compaction = true managed_config_path: Some(managed_path.clone()), #[cfg(target_os = "macos")] managed_preferences_base64: None, + macos_managed_config_requirements_base64: None, }, ); diff --git a/codex-rs/core/src/config_loader/layer_io.rs b/codex-rs/core/src/config_loader/layer_io.rs index d431272968..84a29a6119 100644 --- a/codex-rs/core/src/config_loader/layer_io.rs +++ b/codex-rs/core/src/config_loader/layer_io.rs @@ -33,11 +33,13 @@ pub(super) async fn load_config_layers_internal( let LoaderOverrides { managed_config_path, managed_preferences_base64, + .. } = overrides; #[cfg(not(target_os = "macos"))] let LoaderOverrides { managed_config_path, + .. } = overrides; let managed_config_path = AbsolutePathBuf::from_absolute_path( diff --git a/codex-rs/core/src/config_loader/macos.rs b/codex-rs/core/src/config_loader/macos.rs index 4a80267b90..8d2289e915 100644 --- a/codex-rs/core/src/config_loader/macos.rs +++ b/codex-rs/core/src/config_loader/macos.rs @@ -1,3 +1,4 @@ +use super::config_requirements::ConfigRequirementsToml; use base64::Engine; use base64::prelude::BASE64_STANDARD; use core_foundation::base::TCFType; @@ -10,6 +11,7 @@ use toml::Value as TomlValue; const MANAGED_PREFERENCES_APPLICATION_ID: &str = "com.openai.codex"; const MANAGED_PREFERENCES_CONFIG_KEY: &str = "config_toml_base64"; +const MANAGED_PREFERENCES_REQUIREMENTS_KEY: &str = "requirements_toml_base64"; pub(crate) async fn load_managed_admin_config_layer( override_base64: Option<&str>, @@ -19,82 +21,126 @@ pub(crate) async fn load_managed_admin_config_layer( return if trimmed.is_empty() { Ok(None) } else { - parse_managed_preferences_base64(trimmed).map(Some) + parse_managed_config_base64(trimmed).map(Some) }; } - const LOAD_ERROR: &str = "Failed to load managed preferences configuration"; - match task::spawn_blocking(load_managed_admin_config).await { Ok(result) => result, Err(join_err) => { if join_err.is_cancelled() { - tracing::error!("Managed preferences load task was cancelled"); + tracing::error!("Managed config load task was cancelled"); } else { - tracing::error!("Managed preferences load task failed: {join_err}"); + tracing::error!("Managed config load task failed: {join_err}"); } - Err(io::Error::other(LOAD_ERROR)) + Err(io::Error::other("Failed to load managed config")) } } } fn load_managed_admin_config() -> io::Result> { + load_managed_preference(MANAGED_PREFERENCES_CONFIG_KEY)? + .as_deref() + .map(str::trim) + .map(parse_managed_config_base64) + .transpose() +} + +pub(crate) async fn load_managed_admin_requirements_toml( + target: &mut ConfigRequirementsToml, + override_base64: Option<&str>, +) -> io::Result<()> { + if let Some(encoded) = override_base64 { + let trimmed = encoded.trim(); + if !trimmed.is_empty() { + target.merge_unset_fields(parse_managed_requirements_base64(trimmed)?); + } + return Ok(()); + } + + match task::spawn_blocking(load_managed_admin_requirements).await { + Ok(result) => { + if let Some(requirements) = result? { + target.merge_unset_fields(requirements); + } + Ok(()) + } + Err(join_err) => { + if join_err.is_cancelled() { + tracing::error!("Managed requirements load task was cancelled"); + } else { + tracing::error!("Managed requirements load task failed: {join_err}"); + } + Err(io::Error::other("Failed to load managed requirements")) + } + } +} + +fn load_managed_admin_requirements() -> io::Result> { + load_managed_preference(MANAGED_PREFERENCES_REQUIREMENTS_KEY)? + .as_deref() + .map(str::trim) + .map(parse_managed_requirements_base64) + .transpose() +} + +fn load_managed_preference(key_name: &str) -> io::Result> { #[link(name = "CoreFoundation", kind = "framework")] unsafe extern "C" { fn CFPreferencesCopyAppValue(key: CFStringRef, application_id: CFStringRef) -> *mut c_void; } - let application_id = CFString::new(MANAGED_PREFERENCES_APPLICATION_ID); - let key = CFString::new(MANAGED_PREFERENCES_CONFIG_KEY); - let value_ref = unsafe { CFPreferencesCopyAppValue( - key.as_concrete_TypeRef(), - application_id.as_concrete_TypeRef(), + CFString::new(key_name).as_concrete_TypeRef(), + CFString::new(MANAGED_PREFERENCES_APPLICATION_ID).as_concrete_TypeRef(), ) }; if value_ref.is_null() { tracing::debug!( - "Managed preferences for {} key {} not found", - MANAGED_PREFERENCES_APPLICATION_ID, - MANAGED_PREFERENCES_CONFIG_KEY + "Managed preferences for {MANAGED_PREFERENCES_APPLICATION_ID} key {key_name} not found", ); return Ok(None); } - let value = unsafe { CFString::wrap_under_create_rule(value_ref as _) }; - let contents = value.to_string(); - let trimmed = contents.trim(); - - parse_managed_preferences_base64(trimmed).map(Some) + let value = unsafe { CFString::wrap_under_create_rule(value_ref as _) }.to_string(); + Ok(Some(value)) } -fn parse_managed_preferences_base64(encoded: &str) -> io::Result { - let decoded = BASE64_STANDARD.decode(encoded.as_bytes()).map_err(|err| { - tracing::error!("Failed to decode managed preferences as base64: {err}"); - io::Error::new(io::ErrorKind::InvalidData, err) - })?; - - let decoded_str = String::from_utf8(decoded).map_err(|err| { - tracing::error!("Managed preferences base64 contents were not valid UTF-8: {err}"); - io::Error::new(io::ErrorKind::InvalidData, err) - })?; - - match toml::from_str::(&decoded_str) { +fn parse_managed_config_base64(encoded: &str) -> io::Result { + match toml::from_str::(&decode_managed_preferences_base64(encoded)?) { Ok(TomlValue::Table(parsed)) => Ok(TomlValue::Table(parsed)), Ok(other) => { - tracing::error!( - "Managed preferences TOML must have a table at the root, found {other:?}", - ); + tracing::error!("Managed config TOML must have a table at the root, found {other:?}",); Err(io::Error::new( io::ErrorKind::InvalidData, - "managed preferences root must be a table", + "managed config root must be a table", )) } Err(err) => { - tracing::error!("Failed to parse managed preferences TOML: {err}"); + tracing::error!("Failed to parse managed config TOML: {err}"); Err(io::Error::new(io::ErrorKind::InvalidData, err)) } } } + +fn parse_managed_requirements_base64(encoded: &str) -> io::Result { + toml::from_str::(&decode_managed_preferences_base64(encoded)?).map_err( + |err| { + tracing::error!("Failed to parse managed requirements TOML: {err}"); + io::Error::new(io::ErrorKind::InvalidData, err) + }, + ) +} + +fn decode_managed_preferences_base64(encoded: &str) -> io::Result { + String::from_utf8(BASE64_STANDARD.decode(encoded.as_bytes()).map_err(|err| { + tracing::error!("Failed to decode managed value as base64: {err}",); + io::Error::new(io::ErrorKind::InvalidData, err) + })?) + .map_err(|err| { + tracing::error!("Managed value base64 contents were not valid UTF-8: {err}",); + io::Error::new(io::ErrorKind::InvalidData, err) + }) +} diff --git a/codex-rs/core/src/config_loader/mod.rs b/codex-rs/core/src/config_loader/mod.rs index 73624c83c7..2dbba678ac 100644 --- a/codex-rs/core/src/config_loader/mod.rs +++ b/codex-rs/core/src/config_loader/mod.rs @@ -78,8 +78,14 @@ pub async fn load_config_layers_state( ) -> io::Result { let mut config_requirements_toml = ConfigRequirementsToml::default(); - // TODO(gt): Support an entry in MDM for config requirements and use it - // with `config_requirements_toml.merge_unset_fields(...)`, if present. + #[cfg(target_os = "macos")] + macos::load_managed_admin_requirements_toml( + &mut config_requirements_toml, + overrides + .macos_managed_config_requirements_base64 + .as_deref(), + ) + .await?; // Honor /etc/codex/requirements.toml. if cfg!(unix) { @@ -101,8 +107,6 @@ pub async fn load_config_layers_state( let mut layers = Vec::::new(); - // TODO(gt): Honor managed preferences (macOS only). - // Include an entry for the "system" config folder, loading its config.toml, // if it exists. let system_config_toml_file = if cfg!(unix) { diff --git a/codex-rs/core/src/config_loader/state.rs b/codex-rs/core/src/config_loader/state.rs index efb33dfac5..0ef14403d6 100644 --- a/codex-rs/core/src/config_loader/state.rs +++ b/codex-rs/core/src/config_loader/state.rs @@ -12,11 +12,14 @@ use std::collections::HashMap; use std::path::PathBuf; use toml::Value as TomlValue; +/// LoaderOverrides overrides managed configuration inputs (primarily for tests). #[derive(Debug, Default, Clone)] pub struct LoaderOverrides { pub managed_config_path: Option, + //TODO(gt): Add a macos_ prefix to this field and remove the target_os check. #[cfg(target_os = "macos")] pub managed_preferences_base64: Option, + pub macos_managed_config_requirements_base64: Option, } #[derive(Debug, Clone, PartialEq)] diff --git a/codex-rs/core/src/config_loader/tests.rs b/codex-rs/core/src/config_loader/tests.rs index bb8898129c..b80f00c71c 100644 --- a/codex-rs/core/src/config_loader/tests.rs +++ b/codex-rs/core/src/config_loader/tests.rs @@ -9,6 +9,8 @@ use crate::config_loader::config_requirements::ConfigRequirementsToml; use crate::config_loader::fingerprint::version_for_toml; use crate::config_loader::load_requirements_toml; use codex_protocol::protocol::AskForApproval; +#[cfg(target_os = "macos")] +use codex_protocol::protocol::SandboxPolicy; use codex_utils_absolute_path::AbsolutePathBuf; use pretty_assertions::assert_eq; use tempfile::tempdir; @@ -43,6 +45,7 @@ extra = true managed_config_path: Some(managed_path), #[cfg(target_os = "macos")] managed_preferences_base64: None, + macos_managed_config_requirements_base64: None, }; let cwd = AbsolutePathBuf::try_from(tmp.path()).expect("cwd"); @@ -73,10 +76,12 @@ extra = true async fn returns_empty_when_all_layers_missing() { let tmp = tempdir().expect("tempdir"); let managed_path = tmp.path().join("managed_config.toml"); + let overrides = LoaderOverrides { managed_config_path: Some(managed_path), #[cfg(target_os = "macos")] managed_preferences_base64: None, + macos_managed_config_requirements_base64: None, }; let cwd = AbsolutePathBuf::try_from(tmp.path()).expect("cwd"); @@ -141,12 +146,6 @@ async fn returns_empty_when_all_layers_missing() { async fn managed_preferences_take_highest_precedence() { use base64::Engine; - let managed_payload = r#" -[nested] -value = "managed" -flag = false -"#; - let encoded = base64::prelude::BASE64_STANDARD.encode(managed_payload.as_bytes()); let tmp = tempdir().expect("tempdir"); let managed_path = tmp.path().join("managed_config.toml"); @@ -168,7 +167,17 @@ flag = true let overrides = LoaderOverrides { managed_config_path: Some(managed_path), - managed_preferences_base64: Some(encoded), + managed_preferences_base64: Some( + base64::prelude::BASE64_STANDARD.encode( + r#" +[nested] +value = "managed" +flag = false +"# + .as_bytes(), + ), + ), + macos_managed_config_requirements_base64: None, }; let cwd = AbsolutePathBuf::try_from(tmp.path()).expect("cwd"); @@ -192,6 +201,108 @@ flag = true assert_eq!(nested.get("flag"), Some(&TomlValue::Boolean(false))); } +#[cfg(target_os = "macos")] +#[tokio::test] +async fn managed_preferences_requirements_are_applied() -> anyhow::Result<()> { + use base64::Engine; + + let tmp = tempdir()?; + + let state = load_config_layers_state( + tmp.path(), + Some(AbsolutePathBuf::try_from(tmp.path())?), + &[] as &[(String, TomlValue)], + LoaderOverrides { + managed_config_path: Some(tmp.path().join("managed_config.toml")), + managed_preferences_base64: Some(String::new()), + macos_managed_config_requirements_base64: Some( + base64::prelude::BASE64_STANDARD.encode( + r#" +allowed_approval_policies = ["never"] +allowed_sandbox_modes = ["read-only"] +"# + .as_bytes(), + ), + ), + }, + ) + .await?; + + assert_eq!( + state.requirements().approval_policy.value(), + AskForApproval::Never + ); + assert_eq!( + *state.requirements().sandbox_policy.get(), + SandboxPolicy::ReadOnly + ); + assert!( + state + .requirements() + .approval_policy + .can_set(&AskForApproval::OnRequest) + .is_err() + ); + assert!( + state + .requirements() + .sandbox_policy + .can_set(&SandboxPolicy::WorkspaceWrite { + writable_roots: Vec::new(), + network_access: false, + exclude_tmpdir_env_var: false, + exclude_slash_tmp: false, + }) + .is_err() + ); + + Ok(()) +} + +#[cfg(target_os = "macos")] +#[tokio::test] +async fn managed_preferences_requirements_take_precedence() -> anyhow::Result<()> { + use base64::Engine; + + let tmp = tempdir()?; + let managed_path = tmp.path().join("managed_config.toml"); + + tokio::fs::write(&managed_path, "approval_policy = \"on-request\"\n").await?; + + let state = load_config_layers_state( + tmp.path(), + Some(AbsolutePathBuf::try_from(tmp.path())?), + &[] as &[(String, TomlValue)], + LoaderOverrides { + managed_config_path: Some(managed_path), + managed_preferences_base64: Some(String::new()), + macos_managed_config_requirements_base64: Some( + base64::prelude::BASE64_STANDARD.encode( + r#" +allowed_approval_policies = ["never"] +"# + .as_bytes(), + ), + ), + }, + ) + .await?; + + assert_eq!( + state.requirements().approval_policy.value(), + AskForApproval::Never + ); + assert!( + state + .requirements() + .approval_policy + .can_set(&AskForApproval::OnRequest) + .is_err() + ); + + Ok(()) +} + #[tokio::test(flavor = "current_thread")] async fn load_requirements_toml_produces_expected_constraints() -> anyhow::Result<()> { let tmp = tempdir()?; diff --git a/codex-rs/core/src/features.rs b/codex-rs/core/src/features.rs index 242dd38c89..3b22bfc3f4 100644 --- a/codex-rs/core/src/features.rs +++ b/codex-rs/core/src/features.rs @@ -255,6 +255,10 @@ impl Features { features } + + pub fn enabled_features(&self) -> Vec { + self.enabled.iter().copied().collect() + } } /// Keys accepted in `[features]` tables. diff --git a/codex-rs/core/src/mcp_connection_manager.rs b/codex-rs/core/src/mcp_connection_manager.rs index 6c0b48b1bd..dcd1edf80c 100644 --- a/codex-rs/core/src/mcp_connection_manager.rs +++ b/codex-rs/core/src/mcp_connection_manager.rs @@ -79,26 +79,60 @@ pub const DEFAULT_STARTUP_TIMEOUT: Duration = Duration::from_secs(10); /// Default timeout for individual tool calls. const DEFAULT_TOOL_TIMEOUT: Duration = Duration::from_secs(60); +/// The Responses API requires tool names to match `^[a-zA-Z0-9_-]+$`. +/// MCP server/tool names are user-controlled, so sanitize the fully-qualified +/// name we expose to the model by replacing any disallowed character with `_`. +fn sanitize_responses_api_tool_name(name: &str) -> String { + let mut sanitized = String::with_capacity(name.len()); + for c in name.chars() { + if c.is_ascii_alphanumeric() || c == '_' || c == '-' { + sanitized.push(c); + } else { + sanitized.push('_'); + } + } + + if sanitized.is_empty() { + "_".to_string() + } else { + sanitized + } +} + +fn sha1_hex(s: &str) -> String { + let mut hasher = Sha1::new(); + hasher.update(s.as_bytes()); + let sha1 = hasher.finalize(); + format!("{sha1:x}") +} + fn qualify_tools(tools: I) -> HashMap where I: IntoIterator, { let mut used_names = HashSet::new(); + let mut seen_raw_names = HashSet::new(); let mut qualified_tools = HashMap::new(); for tool in tools { - let mut qualified_name = format!( + let qualified_name_raw = format!( "mcp{}{}{}{}", MCP_TOOL_NAME_DELIMITER, tool.server_name, MCP_TOOL_NAME_DELIMITER, tool.tool_name ); + if !seen_raw_names.insert(qualified_name_raw.clone()) { + warn!("skipping duplicated tool {}", qualified_name_raw); + continue; + } + + // Start from a "pretty" name (sanitized), then deterministically disambiguate on + // collisions by appending a hash of the *raw* (unsanitized) qualified name. This + // ensures tools like `foo.bar` and `foo_bar` don't collapse to the same key. + let mut qualified_name = sanitize_responses_api_tool_name(&qualified_name_raw); + + // Enforce length constraints early; use the raw name for the hash input so the + // output remains stable even when sanitization changes. if qualified_name.len() > MAX_TOOL_NAME_LENGTH { - let mut hasher = Sha1::new(); - hasher.update(qualified_name.as_bytes()); - let sha1 = hasher.finalize(); - let sha1_str = format!("{sha1:x}"); - - // Truncate to make room for the hash suffix + let sha1_str = sha1_hex(&qualified_name_raw); let prefix_len = MAX_TOOL_NAME_LENGTH - sha1_str.len(); - qualified_name = format!("{}{}", &qualified_name[..prefix_len], sha1_str); } @@ -1035,6 +1069,28 @@ mod tests { ); } + #[test] + fn test_qualify_tools_sanitizes_invalid_characters() { + let tools = vec![create_test_tool("server.one", "tool.two")]; + + let qualified_tools = qualify_tools(tools); + + assert_eq!(qualified_tools.len(), 1); + let (qualified_name, tool) = qualified_tools.into_iter().next().expect("one tool"); + assert_eq!(qualified_name, "mcp__server_one__tool_two"); + + // The key is sanitized for OpenAI, but we keep original parts for the actual MCP call. + assert_eq!(tool.server_name, "server.one"); + assert_eq!(tool.tool_name, "tool.two"); + + assert!( + qualified_name + .chars() + .all(|c| c.is_ascii_alphanumeric() || c == '_' || c == '-'), + "qualified name must be Responses API compatible: {qualified_name:?}" + ); + } + #[test] fn tool_filter_allows_by_default() { let filter = ToolFilter::default(); diff --git a/codex-rs/core/src/models_manager/manager.rs b/codex-rs/core/src/models_manager/manager.rs index 85bb761ecd..38e561ac13 100644 --- a/codex-rs/core/src/models_manager/manager.rs +++ b/codex-rs/core/src/models_manager/manager.rs @@ -91,11 +91,15 @@ impl ModelsManager { if self.try_load_cache().await { return Ok(()); } - self.refresh_available_models_no_cache().await + self.refresh_available_models_no_cache(config.features.enabled(Feature::RemoteModels)) + .await } - async fn refresh_available_models_no_cache(&self) -> CoreResult<()> { - if self.auth_manager.get_auth_mode() == Some(AuthMode::ApiKey) { + pub(crate) async fn refresh_available_models_no_cache( + &self, + remote_models_feature: bool, + ) -> CoreResult<()> { + if !remote_models_feature || self.auth_manager.get_auth_mode() == Some(AuthMode::ApiKey) { return Ok(()); } let auth = self.auth_manager.auth(); @@ -175,12 +179,15 @@ impl ModelsManager { } OPENAI_DEFAULT_API_MODEL.to_string() } - pub async fn refresh_if_new_etag(&self, etag: String) { + pub async fn refresh_if_new_etag(&self, etag: String, remote_models_feature: bool) { let current_etag = self.get_etag().await; if current_etag.clone().is_some() && current_etag.as_deref() == Some(etag.as_str()) { return; } - if let Err(err) = self.refresh_available_models_no_cache().await { + if let Err(err) = self + .refresh_available_models_no_cache(remote_models_feature) + .await + { error!("failed to refresh available models: {err}"); } } diff --git a/codex-rs/core/src/project_doc.rs b/codex-rs/core/src/project_doc.rs index cb2499cbbb..79f82c4598 100644 --- a/codex-rs/core/src/project_doc.rs +++ b/codex-rs/core/src/project_doc.rs @@ -513,9 +513,9 @@ mod tests { ) .unwrap_or_else(|_| cfg.codex_home.join("skills/pdf-processing/SKILL.md")); let expected_path_str = expected_path.to_string_lossy().replace('\\', "/"); - let usage_rules = "- Discovery: Available skills are listed in project docs and may also appear in a runtime \"## Skills\" section (name + description + file path). These are the sources of truth; skill bodies live on disk at the listed paths.\n- Trigger rules: If the user names a skill (with `$SkillName` or plain text) OR the task clearly matches a skill's description, you must use that skill for that turn. Multiple mentions mean use them all. Do not carry skills across turns unless re-mentioned.\n- Missing/blocked: If a named skill isn't in the list or the path can't be read, say so briefly and continue with the best fallback.\n- How to use a skill (progressive disclosure):\n 1) After deciding to use a skill, open its `SKILL.md`. Read only enough to follow the workflow.\n 2) If `SKILL.md` points to extra folders such as `references/`, load only the specific files needed for the request; don't bulk-load everything.\n 3) If `scripts/` exist, prefer running or patching them instead of retyping large code blocks.\n 4) If `assets/` or templates exist, reuse them instead of recreating from scratch.\n- Description as trigger: The YAML `description` in `SKILL.md` is the primary trigger signal; rely on it to decide applicability. If unsure, ask a brief clarification before proceeding.\n- Coordination and sequencing:\n - If multiple skills apply, choose the minimal set that covers the request and state the order you'll use them.\n - Announce which skill(s) you're using and why (one short line). If you skip an obvious skill, say why.\n- Context hygiene:\n - Keep context small: summarize long sections instead of pasting them; only load extra files when needed.\n - Avoid deeply nested references; prefer one-hop files explicitly linked from `SKILL.md`.\n - When variants exist (frameworks, providers, domains), pick only the relevant reference file(s) and note that choice.\n- Safety and fallback: If a skill can't be applied cleanly (missing files, unclear instructions), state the issue, pick the next-best approach, and continue."; + let usage_rules = "- Discovery: The list above is the skills available in this session (name + description + file path). Skill bodies live on disk at the listed paths.\n- Trigger rules: If the user names a skill (with `$SkillName` or plain text) OR the task clearly matches a skill's description shown above, you must use that skill for that turn. Multiple mentions mean use them all. Do not carry skills across turns unless re-mentioned.\n- Missing/blocked: If a named skill isn't in the list or the path can't be read, say so briefly and continue with the best fallback.\n- How to use a skill (progressive disclosure):\n 1) After deciding to use a skill, open its `SKILL.md`. Read only enough to follow the workflow.\n 2) If `SKILL.md` points to extra folders such as `references/`, load only the specific files needed for the request; don't bulk-load everything.\n 3) If `scripts/` exist, prefer running or patching them instead of retyping large code blocks.\n 4) If `assets/` or templates exist, reuse them instead of recreating from scratch.\n- Coordination and sequencing:\n - If multiple skills apply, choose the minimal set that covers the request and state the order you'll use them.\n - Announce which skill(s) you're using and why (one short line). If you skip an obvious skill, say why.\n- Context hygiene:\n - Keep context small: summarize long sections instead of pasting them; only load extra files when needed.\n - Avoid deep reference-chasing: prefer opening only files directly linked from `SKILL.md` unless you're blocked.\n - When variants exist (frameworks, providers, domains), pick only the relevant reference file(s) and note that choice.\n- Safety and fallback: If a skill can't be applied cleanly (missing files, unclear instructions), state the issue, pick the next-best approach, and continue."; let expected = format!( - "base doc\n\n## Skills\nThese skills are discovered at startup from multiple local sources. Each entry includes a name, description, and file path so you can open the source for full instructions.\n- pdf-processing: extract from pdfs (file: {expected_path_str})\n{usage_rules}" + "base doc\n\n## Skills\nA skill is a set of local instructions to follow that is stored in a `SKILL.md` file. Below is the list of skills that can be used. Each entry includes a name, description, and file path so you can open the source for full instructions when using a specific skill.\n### Available skills\n- pdf-processing: extract from pdfs (file: {expected_path_str})\n### How to use skills\n{usage_rules}" ); assert_eq!(res, expected); } @@ -537,9 +537,9 @@ mod tests { dunce::canonicalize(cfg.codex_home.join("skills/linting/SKILL.md").as_path()) .unwrap_or_else(|_| cfg.codex_home.join("skills/linting/SKILL.md")); let expected_path_str = expected_path.to_string_lossy().replace('\\', "/"); - let usage_rules = "- Discovery: Available skills are listed in project docs and may also appear in a runtime \"## Skills\" section (name + description + file path). These are the sources of truth; skill bodies live on disk at the listed paths.\n- Trigger rules: If the user names a skill (with `$SkillName` or plain text) OR the task clearly matches a skill's description, you must use that skill for that turn. Multiple mentions mean use them all. Do not carry skills across turns unless re-mentioned.\n- Missing/blocked: If a named skill isn't in the list or the path can't be read, say so briefly and continue with the best fallback.\n- How to use a skill (progressive disclosure):\n 1) After deciding to use a skill, open its `SKILL.md`. Read only enough to follow the workflow.\n 2) If `SKILL.md` points to extra folders such as `references/`, load only the specific files needed for the request; don't bulk-load everything.\n 3) If `scripts/` exist, prefer running or patching them instead of retyping large code blocks.\n 4) If `assets/` or templates exist, reuse them instead of recreating from scratch.\n- Description as trigger: The YAML `description` in `SKILL.md` is the primary trigger signal; rely on it to decide applicability. If unsure, ask a brief clarification before proceeding.\n- Coordination and sequencing:\n - If multiple skills apply, choose the minimal set that covers the request and state the order you'll use them.\n - Announce which skill(s) you're using and why (one short line). If you skip an obvious skill, say why.\n- Context hygiene:\n - Keep context small: summarize long sections instead of pasting them; only load extra files when needed.\n - Avoid deeply nested references; prefer one-hop files explicitly linked from `SKILL.md`.\n - When variants exist (frameworks, providers, domains), pick only the relevant reference file(s) and note that choice.\n- Safety and fallback: If a skill can't be applied cleanly (missing files, unclear instructions), state the issue, pick the next-best approach, and continue."; + let usage_rules = "- Discovery: The list above is the skills available in this session (name + description + file path). Skill bodies live on disk at the listed paths.\n- Trigger rules: If the user names a skill (with `$SkillName` or plain text) OR the task clearly matches a skill's description shown above, you must use that skill for that turn. Multiple mentions mean use them all. Do not carry skills across turns unless re-mentioned.\n- Missing/blocked: If a named skill isn't in the list or the path can't be read, say so briefly and continue with the best fallback.\n- How to use a skill (progressive disclosure):\n 1) After deciding to use a skill, open its `SKILL.md`. Read only enough to follow the workflow.\n 2) If `SKILL.md` points to extra folders such as `references/`, load only the specific files needed for the request; don't bulk-load everything.\n 3) If `scripts/` exist, prefer running or patching them instead of retyping large code blocks.\n 4) If `assets/` or templates exist, reuse them instead of recreating from scratch.\n- Coordination and sequencing:\n - If multiple skills apply, choose the minimal set that covers the request and state the order you'll use them.\n - Announce which skill(s) you're using and why (one short line). If you skip an obvious skill, say why.\n- Context hygiene:\n - Keep context small: summarize long sections instead of pasting them; only load extra files when needed.\n - Avoid deep reference-chasing: prefer opening only files directly linked from `SKILL.md` unless you're blocked.\n - When variants exist (frameworks, providers, domains), pick only the relevant reference file(s) and note that choice.\n- Safety and fallback: If a skill can't be applied cleanly (missing files, unclear instructions), state the issue, pick the next-best approach, and continue."; let expected = format!( - "## Skills\nThese skills are discovered at startup from multiple local sources. Each entry includes a name, description, and file path so you can open the source for full instructions.\n- linting: run clippy (file: {expected_path_str})\n{usage_rules}" + "## Skills\nA skill is a set of local instructions to follow that is stored in a `SKILL.md` file. Below is the list of skills that can be used. Each entry includes a name, description, and file path so you can open the source for full instructions when using a specific skill.\n### Available skills\n- linting: run clippy (file: {expected_path_str})\n### How to use skills\n{usage_rules}" ); assert_eq!(res, expected); } diff --git a/codex-rs/core/src/skills/render.rs b/codex-rs/core/src/skills/render.rs index f767849b24..f998a51042 100644 --- a/codex-rs/core/src/skills/render.rs +++ b/codex-rs/core/src/skills/render.rs @@ -7,7 +7,8 @@ pub fn render_skills_section(skills: &[SkillMetadata]) -> Option { let mut lines: Vec = Vec::new(); lines.push("## Skills".to_string()); - lines.push("These skills are discovered at startup from multiple local sources. Each entry includes a name, description, and file path so you can open the source for full instructions.".to_string()); + lines.push("A skill is a set of local instructions to follow that is stored in a `SKILL.md` file. Below is the list of skills that can be used. Each entry includes a name, description, and file path so you can open the source for full instructions when using a specific skill.".to_string()); + lines.push("### Available skills".to_string()); for skill in skills { let path_str = skill.path.to_string_lossy().replace('\\', "/"); @@ -16,22 +17,22 @@ pub fn render_skills_section(skills: &[SkillMetadata]) -> Option { lines.push(format!("- {name}: {description} (file: {path_str})")); } + lines.push("### How to use skills".to_string()); lines.push( - r###"- Discovery: Available skills are listed in project docs and may also appear in a runtime "## Skills" section (name + description + file path). These are the sources of truth; skill bodies live on disk at the listed paths. -- Trigger rules: If the user names a skill (with `$SkillName` or plain text) OR the task clearly matches a skill's description, you must use that skill for that turn. Multiple mentions mean use them all. Do not carry skills across turns unless re-mentioned. + r###"- Discovery: The list above is the skills available in this session (name + description + file path). Skill bodies live on disk at the listed paths. +- Trigger rules: If the user names a skill (with `$SkillName` or plain text) OR the task clearly matches a skill's description shown above, you must use that skill for that turn. Multiple mentions mean use them all. Do not carry skills across turns unless re-mentioned. - Missing/blocked: If a named skill isn't in the list or the path can't be read, say so briefly and continue with the best fallback. - How to use a skill (progressive disclosure): 1) After deciding to use a skill, open its `SKILL.md`. Read only enough to follow the workflow. 2) If `SKILL.md` points to extra folders such as `references/`, load only the specific files needed for the request; don't bulk-load everything. 3) If `scripts/` exist, prefer running or patching them instead of retyping large code blocks. 4) If `assets/` or templates exist, reuse them instead of recreating from scratch. -- Description as trigger: The YAML `description` in `SKILL.md` is the primary trigger signal; rely on it to decide applicability. If unsure, ask a brief clarification before proceeding. - Coordination and sequencing: - If multiple skills apply, choose the minimal set that covers the request and state the order you'll use them. - Announce which skill(s) you're using and why (one short line). If you skip an obvious skill, say why. - Context hygiene: - Keep context small: summarize long sections instead of pasting them; only load extra files when needed. - - Avoid deeply nested references; prefer one-hop files explicitly linked from `SKILL.md`. + - Avoid deep reference-chasing: prefer opening only files directly linked from `SKILL.md` unless you're blocked. - When variants exist (frameworks, providers, domains), pick only the relevant reference file(s) and note that choice. - Safety and fallback: If a skill can't be applied cleanly (missing files, unclear instructions), state the issue, pick the next-best approach, and continue."### .to_string(), diff --git a/codex-rs/core/src/unified_exec/session_manager.rs b/codex-rs/core/src/unified_exec/session_manager.rs index c97820e464..0a1e29e8fb 100644 --- a/codex-rs/core/src/unified_exec/session_manager.rs +++ b/codex-rs/core/src/unified_exec/session_manager.rs @@ -47,7 +47,7 @@ use super::session::OutputBuffer; use super::session::OutputHandles; use super::session::UnifiedExecSession; -const UNIFIED_EXEC_ENV: [(&str, &str); 8] = [ +const UNIFIED_EXEC_ENV: [(&str, &str); 9] = [ ("NO_COLOR", "1"), ("TERM", "dumb"), ("LANG", "C.UTF-8"), @@ -56,6 +56,7 @@ const UNIFIED_EXEC_ENV: [(&str, &str); 8] = [ ("COLORTERM", ""), ("PAGER", "cat"), ("GIT_PAGER", "cat"), + ("GH_PAGER", "cat"), ]; fn apply_unified_exec_env(mut env: HashMap) -> HashMap { @@ -679,6 +680,7 @@ mod tests { ("COLORTERM".to_string(), String::new()), ("PAGER".to_string(), "cat".to_string()), ("GIT_PAGER".to_string(), "cat".to_string()), + ("GH_PAGER".to_string(), "cat".to_string()), ]); assert_eq!(env, expected); diff --git a/codex-rs/core/src/util.rs b/codex-rs/core/src/util.rs index db2d0e74eb..a100f28443 100644 --- a/codex-rs/core/src/util.rs +++ b/codex-rs/core/src/util.rs @@ -9,6 +9,31 @@ use tracing::error; const INITIAL_DELAY_MS: u64 = 200; const BACKOFF_FACTOR: f64 = 2.0; +/// Emit structured feedback metadata as key/value pairs. +/// +/// This logs a tracing event with `target: "feedback_tags"`. If +/// `codex_feedback::CodexFeedback::metadata_layer()` is installed, these fields are captured and +/// later attached as tags when feedback is uploaded. +/// +/// Values are wrapped with [`tracing::field::DebugValue`], so the expression only needs to +/// implement [`std::fmt::Debug`]. +/// +/// Example: +/// +/// ```rust +/// codex_core::feedback_tags!(model = "gpt-5", cached = true); +/// codex_core::feedback_tags!(provider = provider_id, request_id = request_id); +/// ``` +#[macro_export] +macro_rules! feedback_tags { + ($( $key:ident = $value:expr ),+ $(,)?) => { + ::tracing::info!( + target: "feedback_tags", + $( $key = ::tracing::field::debug(&$value) ),+ + ); + }; +} + pub(crate) fn backoff(attempt: u64) -> Duration { let exp = BACKOFF_FACTOR.powi(attempt.saturating_sub(1) as i32); let base = (INITIAL_DELAY_MS as f64 * exp) as u64; @@ -74,4 +99,12 @@ mod tests { let message = try_parse_error_message(text); assert_eq!(message, r#"{"message": "test"}"#); } + + #[test] + fn feedback_tags_macro_compiles() { + #[derive(Debug)] + struct OnlyDebug; + + feedback_tags!(model = "gpt-5", cached = true, debug_only = OnlyDebug); + } } diff --git a/codex-rs/core/tests/suite/abort_tasks.rs b/codex-rs/core/tests/suite/abort_tasks.rs index 0d4a807a3c..53d32e140d 100644 --- a/codex-rs/core/tests/suite/abort_tasks.rs +++ b/codex-rs/core/tests/suite/abort_tasks.rs @@ -49,6 +49,7 @@ async fn interrupt_long_running_tool_emits_turn_aborted() { items: vec![UserInput::Text { text: "start sleep".into(), }], + final_output_json_schema: None, }) .await .unwrap(); @@ -101,6 +102,7 @@ async fn interrupt_tool_records_history_entries() { items: vec![UserInput::Text { text: "start history recording".into(), }], + final_output_json_schema: None, }) .await .unwrap(); @@ -117,6 +119,7 @@ async fn interrupt_tool_records_history_entries() { items: vec![UserInput::Text { text: "follow up".into(), }], + final_output_json_schema: None, }) .await .unwrap(); diff --git a/codex-rs/core/tests/suite/client.rs b/codex-rs/core/tests/suite/client.rs index a22027f99f..5ce6e9f2ff 100644 --- a/codex-rs/core/tests/suite/client.rs +++ b/codex-rs/core/tests/suite/client.rs @@ -290,6 +290,7 @@ async fn resume_includes_initial_messages_and_sends_prior_items() { items: vec![UserInput::Text { text: "hello".into(), }], + final_output_json_schema: None, }) .await .unwrap(); @@ -365,6 +366,7 @@ async fn includes_conversation_id_and_model_headers_in_request() { items: vec![UserInput::Text { text: "hello".into(), }], + final_output_json_schema: None, }) .await .unwrap(); @@ -424,6 +426,7 @@ async fn includes_base_instructions_override_in_request() { items: vec![UserInput::Text { text: "hello".into(), }], + final_output_json_schema: None, }) .await .unwrap(); @@ -488,6 +491,7 @@ async fn chatgpt_auth_sends_correct_request() { items: vec![UserInput::Text { text: "hello".into(), }], + final_output_json_schema: None, }) .await .unwrap(); @@ -582,6 +586,7 @@ async fn prefers_apikey_when_config_prefers_apikey_even_with_chatgpt_tokens() { items: vec![UserInput::Text { text: "hello".into(), }], + final_output_json_schema: None, }) .await .unwrap(); @@ -622,6 +627,7 @@ async fn includes_user_instructions_message_in_request() { items: vec![UserInput::Text { text: "hello".into(), }], + final_output_json_schema: None, }) .await .unwrap(); @@ -692,6 +698,7 @@ async fn skills_append_to_instructions() { items: vec![UserInput::Text { text: "hello".into(), }], + final_output_json_schema: None, }) .await .unwrap(); @@ -741,6 +748,7 @@ async fn includes_configured_effort_in_request() -> anyhow::Result<()> { items: vec![UserInput::Text { text: "hello".into(), }], + final_output_json_schema: None, }) .await .unwrap(); @@ -777,6 +785,7 @@ async fn includes_no_effort_in_request() -> anyhow::Result<()> { items: vec![UserInput::Text { text: "hello".into(), }], + final_output_json_schema: None, }) .await .unwrap(); @@ -811,6 +820,7 @@ async fn includes_default_reasoning_effort_in_request_when_defined_by_model_fami items: vec![UserInput::Text { text: "hello".into(), }], + final_output_json_schema: None, }) .await .unwrap(); @@ -849,6 +859,7 @@ async fn configured_reasoning_summary_is_sent() -> anyhow::Result<()> { items: vec![UserInput::Text { text: "hello".into(), }], + final_output_json_schema: None, }) .await .unwrap(); @@ -887,6 +898,7 @@ async fn reasoning_summary_is_omitted_when_disabled() -> anyhow::Result<()> { items: vec![UserInput::Text { text: "hello".into(), }], + final_output_json_schema: None, }) .await .unwrap(); @@ -919,6 +931,7 @@ async fn includes_default_verbosity_in_request() -> anyhow::Result<()> { items: vec![UserInput::Text { text: "hello".into(), }], + final_output_json_schema: None, }) .await .unwrap(); @@ -958,6 +971,7 @@ async fn configured_verbosity_not_sent_for_models_without_support() -> anyhow::R items: vec![UserInput::Text { text: "hello".into(), }], + final_output_json_schema: None, }) .await .unwrap(); @@ -996,6 +1010,7 @@ async fn configured_verbosity_is_sent() -> anyhow::Result<()> { items: vec![UserInput::Text { text: "hello".into(), }], + final_output_json_schema: None, }) .await .unwrap(); @@ -1050,6 +1065,7 @@ async fn includes_developer_instructions_message_in_request() { items: vec![UserInput::Text { text: "hello".into(), }], + final_output_json_schema: None, }) .await .unwrap(); @@ -1280,6 +1296,7 @@ async fn token_count_includes_rate_limits_snapshot() { items: vec![UserInput::Text { text: "hello".into(), }], + final_output_json_schema: None, }) .await .unwrap(); @@ -1437,6 +1454,7 @@ async fn usage_limit_error_emits_rate_limit_event() -> anyhow::Result<()> { items: vec![UserInput::Text { text: "hello".into(), }], + final_output_json_schema: None, }) .await .expect("submission should succeed while emitting usage limit error events"); @@ -1506,6 +1524,7 @@ async fn context_window_error_sets_total_tokens_to_model_window() -> anyhow::Res items: vec![UserInput::Text { text: "seed turn".into(), }], + final_output_json_schema: None, }) .await?; @@ -1516,6 +1535,7 @@ async fn context_window_error_sets_total_tokens_to_model_window() -> anyhow::Res items: vec![UserInput::Text { text: "trigger context window".into(), }], + final_output_json_schema: None, }) .await?; @@ -1635,6 +1655,7 @@ async fn azure_overrides_assign_properties_used_for_responses_url() { items: vec![UserInput::Text { text: "hello".into(), }], + final_output_json_schema: None, }) .await .unwrap(); @@ -1717,6 +1738,7 @@ async fn env_var_overrides_loaded_auth() { items: vec![UserInput::Text { text: "hello".into(), }], + final_output_json_schema: None, }) .await .unwrap(); @@ -1800,6 +1822,7 @@ async fn history_dedupes_streamed_and_final_messages_across_turns() { codex .submit(Op::UserInput { items: vec![UserInput::Text { text: "U1".into() }], + final_output_json_schema: None, }) .await .unwrap(); @@ -1809,6 +1832,7 @@ async fn history_dedupes_streamed_and_final_messages_across_turns() { codex .submit(Op::UserInput { items: vec![UserInput::Text { text: "U2".into() }], + final_output_json_schema: None, }) .await .unwrap(); @@ -1818,6 +1842,7 @@ async fn history_dedupes_streamed_and_final_messages_across_turns() { codex .submit(Op::UserInput { items: vec![UserInput::Text { text: "U3".into() }], + final_output_json_schema: None, }) .await .unwrap(); diff --git a/codex-rs/core/tests/suite/compact.rs b/codex-rs/core/tests/suite/compact.rs index 4f57330a28..09b5fb18b8 100644 --- a/codex-rs/core/tests/suite/compact.rs +++ b/codex-rs/core/tests/suite/compact.rs @@ -8,11 +8,14 @@ use codex_core::compact::SUMMARIZATION_PROMPT; use codex_core::compact::SUMMARY_PREFIX; use codex_core::config::Config; use codex_core::features::Feature; +use codex_core::protocol::AskForApproval; use codex_core::protocol::EventMsg; use codex_core::protocol::Op; use codex_core::protocol::RolloutItem; use codex_core::protocol::RolloutLine; +use codex_core::protocol::SandboxPolicy; use codex_core::protocol::WarningEvent; +use codex_protocol::config_types::ReasoningSummary; use codex_protocol::user_input::UserInput; use core_test_support::load_default_config_for_test; use core_test_support::responses::ev_local_shell_call; @@ -158,6 +161,7 @@ async fn summarize_context_three_requests_and_instructions() { items: vec![UserInput::Text { text: "hello world".into(), }], + final_output_json_schema: None, }) .await .unwrap(); @@ -178,6 +182,7 @@ async fn summarize_context_three_requests_and_instructions() { items: vec![UserInput::Text { text: THIRD_USER_MSG.into(), }], + final_output_json_schema: None, }) .await .unwrap(); @@ -577,6 +582,7 @@ async fn multiple_auto_compact_per_task_runs_after_token_limit_hit() { items: vec![UserInput::Text { text: user_message.into(), }], + final_output_json_schema: None, }) .await .expect("submit user input"); @@ -1081,6 +1087,7 @@ async fn auto_compact_runs_after_token_limit_hit() { items: vec![UserInput::Text { text: FIRST_AUTO_MSG.into(), }], + final_output_json_schema: None, }) .await .unwrap(); @@ -1092,6 +1099,7 @@ async fn auto_compact_runs_after_token_limit_hit() { items: vec![UserInput::Text { text: SECOND_AUTO_MSG.into(), }], + final_output_json_schema: None, }) .await .unwrap(); @@ -1103,6 +1111,7 @@ async fn auto_compact_runs_after_token_limit_hit() { items: vec![UserInput::Text { text: POST_AUTO_USER_MSG.into(), }], + final_output_json_schema: None, }) .await .unwrap(); @@ -1228,6 +1237,117 @@ async fn auto_compact_runs_after_token_limit_hit() { ); } +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn auto_compact_runs_after_resume_when_token_usage_is_over_limit() { + skip_if_no_network!(); + + let server = start_mock_server().await; + + let limit = 200_000; + let over_limit_tokens = 250_000; + let remote_summary = "REMOTE_COMPACT_SUMMARY"; + + let compacted_history = vec![ + codex_protocol::models::ResponseItem::Message { + id: None, + role: "assistant".to_string(), + content: vec![codex_protocol::models::ContentItem::OutputText { + text: remote_summary.to_string(), + }], + }, + codex_protocol::models::ResponseItem::Compaction { + encrypted_content: "ENCRYPTED_COMPACTION_SUMMARY".to_string(), + }, + ]; + let compact_mock = + mount_compact_json_once(&server, serde_json::json!({ "output": compacted_history })).await; + + let mut builder = test_codex().with_config(move |config| { + set_test_compact_prompt(config); + config.model_auto_compact_token_limit = Some(limit); + config.features.enable(Feature::RemoteCompaction); + }); + let initial = builder.build(&server).await.unwrap(); + let home = initial.home.clone(); + let rollout_path = initial.session_configured.rollout_path.clone(); + + // A single over-limit completion should not auto-compact until the next user message. + mount_sse_once( + &server, + sse(vec![ + ev_assistant_message("m1", FIRST_REPLY), + ev_completed_with_tokens("r1", over_limit_tokens), + ]), + ) + .await; + initial.submit_turn("OVER_LIMIT_TURN").await.unwrap(); + + assert!( + compact_mock.requests().is_empty(), + "remote compaction should not run before the next user message" + ); + + let mut resume_builder = test_codex().with_config(move |config| { + set_test_compact_prompt(config); + config.model_auto_compact_token_limit = Some(limit); + config.features.enable(Feature::RemoteCompaction); + }); + let resumed = resume_builder + .resume(&server, home, rollout_path) + .await + .unwrap(); + + let follow_up_user = "AFTER_RESUME_USER"; + let sse_follow_up = sse(vec![ + ev_assistant_message("m2", FINAL_REPLY), + ev_completed("r2"), + ]); + + let follow_up_matcher = move |req: &wiremock::Request| { + let body = std::str::from_utf8(&req.body).unwrap_or(""); + body.contains(follow_up_user) && body.contains(remote_summary) + }; + mount_sse_once_match(&server, follow_up_matcher, sse_follow_up).await; + + resumed + .codex + .submit(Op::UserTurn { + items: vec![UserInput::Text { + text: follow_up_user.into(), + }], + final_output_json_schema: None, + cwd: resumed.cwd.path().to_path_buf(), + approval_policy: AskForApproval::Never, + sandbox_policy: SandboxPolicy::DangerFullAccess, + model: resumed.session_configured.model.clone(), + effort: None, + summary: ReasoningSummary::Auto, + }) + .await + .unwrap(); + + wait_for_event(&resumed.codex, |event| { + matches!(event, EventMsg::ContextCompacted(_)) + }) + .await; + wait_for_event(&resumed.codex, |event| { + matches!(event, EventMsg::TaskComplete(_)) + }) + .await; + + let compact_requests = compact_mock.requests(); + assert_eq!( + compact_requests.len(), + 1, + "remote compaction should run once after resume" + ); + assert_eq!( + compact_requests[0].path(), + "/v1/responses/compact", + "remote compaction should hit the compact endpoint" + ); +} + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn auto_compact_persists_rollout_entries() { skip_if_no_network!(); @@ -1304,6 +1424,7 @@ async fn auto_compact_persists_rollout_entries() { items: vec![UserInput::Text { text: FIRST_AUTO_MSG.into(), }], + final_output_json_schema: None, }) .await .unwrap(); @@ -1314,6 +1435,7 @@ async fn auto_compact_persists_rollout_entries() { items: vec![UserInput::Text { text: SECOND_AUTO_MSG.into(), }], + final_output_json_schema: None, }) .await .unwrap(); @@ -1324,6 +1446,7 @@ async fn auto_compact_persists_rollout_entries() { items: vec![UserInput::Text { text: POST_AUTO_USER_MSG.into(), }], + final_output_json_schema: None, }) .await .unwrap(); @@ -1415,6 +1538,7 @@ async fn manual_compact_retries_after_context_window_error() { items: vec![UserInput::Text { text: "first turn".into(), }], + final_output_json_schema: None, }) .await .unwrap(); @@ -1547,6 +1671,7 @@ async fn manual_compact_twice_preserves_latest_user_messages() { items: vec![UserInput::Text { text: first_user_message.into(), }], + final_output_json_schema: None, }) .await .unwrap(); @@ -1560,6 +1685,7 @@ async fn manual_compact_twice_preserves_latest_user_messages() { items: vec![UserInput::Text { text: second_user_message.into(), }], + final_output_json_schema: None, }) .await .unwrap(); @@ -1573,6 +1699,7 @@ async fn manual_compact_twice_preserves_latest_user_messages() { items: vec![UserInput::Text { text: final_user_message.into(), }], + final_output_json_schema: None, }) .await .unwrap(); @@ -1752,6 +1879,7 @@ async fn auto_compact_allows_multiple_attempts_when_interleaved_with_other_turn_ codex .submit(Op::UserInput { items: vec![UserInput::Text { text: user.into() }], + final_output_json_schema: None, }) .await .unwrap(); @@ -1864,6 +1992,7 @@ async fn auto_compact_triggers_after_function_call_over_95_percent_usage() { items: vec![UserInput::Text { text: FUNCTION_CALL_LIMIT_MSG.into(), }], + final_output_json_schema: None, }) .await .unwrap(); @@ -1875,6 +2004,7 @@ async fn auto_compact_triggers_after_function_call_over_95_percent_usage() { items: vec![UserInput::Text { text: follow_up_user.into(), }], + final_output_json_schema: None, }) .await .unwrap(); @@ -1989,6 +2119,7 @@ async fn auto_compact_counts_encrypted_reasoning_before_last_user() { codex .submit(Op::UserInput { items: vec![UserInput::Text { text: user.into() }], + final_output_json_schema: None, }) .await .unwrap(); diff --git a/codex-rs/core/tests/suite/compact_remote.rs b/codex-rs/core/tests/suite/compact_remote.rs index 34e44419b4..5a2fb54530 100644 --- a/codex-rs/core/tests/suite/compact_remote.rs +++ b/codex-rs/core/tests/suite/compact_remote.rs @@ -74,6 +74,7 @@ async fn remote_compact_replaces_history_for_followups() -> Result<()> { items: vec![UserInput::Text { text: "hello remote compact".into(), }], + final_output_json_schema: None, }) .await?; wait_for_event(&codex, |ev| matches!(ev, EventMsg::TaskComplete(_))).await; @@ -86,6 +87,7 @@ async fn remote_compact_replaces_history_for_followups() -> Result<()> { items: vec![UserInput::Text { text: "after compact".into(), }], + final_output_json_schema: None, }) .await?; wait_for_event(&codex, |ev| matches!(ev, EventMsg::TaskComplete(_))).await; @@ -191,6 +193,7 @@ async fn remote_compact_runs_automatically() -> Result<()> { items: vec![UserInput::Text { text: "hello remote compact".into(), }], + final_output_json_schema: None, }) .await?; let message = wait_for_event_match(&codex, |ev| match ev { @@ -263,6 +266,7 @@ async fn remote_compact_persists_replacement_history_in_rollout() -> Result<()> items: vec![UserInput::Text { text: "needs compaction".into(), }], + final_output_json_schema: None, }) .await?; wait_for_event(&codex, |ev| matches!(ev, EventMsg::TaskComplete(_))).await; diff --git a/codex-rs/core/tests/suite/compact_resume_fork.rs b/codex-rs/core/tests/suite/compact_resume_fork.rs index 75468ae145..3e38c89b33 100644 --- a/codex-rs/core/tests/suite/compact_resume_fork.rs +++ b/codex-rs/core/tests/suite/compact_resume_fork.rs @@ -884,6 +884,7 @@ async fn user_turn(conversation: &Arc, text: &str) { conversation .submit(Op::UserInput { items: vec![UserInput::Text { text: text.into() }], + final_output_json_schema: None, }) .await .expect("submit user turn"); diff --git a/codex-rs/core/tests/suite/fork_conversation.rs b/codex-rs/core/tests/suite/fork_conversation.rs index d302b4d77a..dab856c80b 100644 --- a/codex-rs/core/tests/suite/fork_conversation.rs +++ b/codex-rs/core/tests/suite/fork_conversation.rs @@ -74,6 +74,7 @@ async fn fork_conversation_twice_drops_to_first_message() { items: vec![UserInput::Text { text: text.to_string(), }], + final_output_json_schema: None, }) .await .unwrap(); diff --git a/codex-rs/core/tests/suite/items.rs b/codex-rs/core/tests/suite/items.rs index 4ec23e9de6..3c6cf5ff3e 100644 --- a/codex-rs/core/tests/suite/items.rs +++ b/codex-rs/core/tests/suite/items.rs @@ -43,6 +43,7 @@ async fn user_message_item_is_emitted() -> anyhow::Result<()> { items: (vec![UserInput::Text { text: "please inspect sample.txt".into(), }]), + final_output_json_schema: None, }) .await?; @@ -99,6 +100,7 @@ async fn assistant_message_item_is_emitted() -> anyhow::Result<()> { items: vec![UserInput::Text { text: "please summarize results".into(), }], + final_output_json_schema: None, }) .await?; @@ -155,6 +157,7 @@ async fn reasoning_item_is_emitted() -> anyhow::Result<()> { items: vec![UserInput::Text { text: "explain your reasoning".into(), }], + final_output_json_schema: None, }) .await?; @@ -213,6 +216,7 @@ async fn web_search_item_is_emitted() -> anyhow::Result<()> { items: vec![UserInput::Text { text: "find the weather".into(), }], + final_output_json_schema: None, }) .await?; @@ -265,6 +269,7 @@ async fn agent_message_content_delta_has_item_metadata() -> anyhow::Result<()> { items: vec![UserInput::Text { text: "please stream text".into(), }], + final_output_json_schema: None, }) .await?; @@ -330,6 +335,7 @@ async fn reasoning_content_delta_has_item_metadata() -> anyhow::Result<()> { items: vec![UserInput::Text { text: "reason through it".into(), }], + final_output_json_schema: None, }) .await?; @@ -387,6 +393,7 @@ async fn reasoning_raw_content_delta_respects_flag() -> anyhow::Result<()> { items: vec![UserInput::Text { text: "show raw reasoning".into(), }], + final_output_json_schema: None, }) .await?; diff --git a/codex-rs/core/tests/suite/otel.rs b/codex-rs/core/tests/suite/otel.rs index e19c41da86..4fa45f0108 100644 --- a/codex-rs/core/tests/suite/otel.rs +++ b/codex-rs/core/tests/suite/otel.rs @@ -46,6 +46,7 @@ async fn responses_api_emits_api_request_event() { items: vec![UserInput::Text { text: "hello".into(), }], + final_output_json_schema: None, }) .await .unwrap(); @@ -87,6 +88,7 @@ async fn process_sse_emits_tracing_for_output_item() { items: vec![UserInput::Text { text: "hello".into(), }], + final_output_json_schema: None, }) .await .unwrap(); @@ -125,6 +127,7 @@ async fn process_sse_emits_failed_event_on_parse_error() { items: vec![UserInput::Text { text: "hello".into(), }], + final_output_json_schema: None, }) .await .unwrap(); @@ -164,6 +167,7 @@ async fn process_sse_records_failed_event_when_stream_closes_without_completed() items: vec![UserInput::Text { text: "hello".into(), }], + final_output_json_schema: None, }) .await .unwrap(); @@ -223,6 +227,7 @@ async fn process_sse_failed_event_records_response_error_message() { items: vec![UserInput::Text { text: "hello".into(), }], + final_output_json_schema: None, }) .await .unwrap(); @@ -280,6 +285,7 @@ async fn process_sse_failed_event_logs_parse_error() { items: vec![UserInput::Text { text: "hello".into(), }], + final_output_json_schema: None, }) .await .unwrap(); @@ -324,6 +330,7 @@ async fn process_sse_failed_event_logs_missing_error() { items: vec![UserInput::Text { text: "hello".into(), }], + final_output_json_schema: None, }) .await .unwrap(); @@ -377,6 +384,7 @@ async fn process_sse_failed_event_logs_response_completed_parse_error() { items: vec![UserInput::Text { text: "hello".into(), }], + final_output_json_schema: None, }) .await .unwrap(); @@ -427,6 +435,7 @@ async fn process_sse_emits_completed_telemetry() { items: vec![UserInput::Text { text: "hello".into(), }], + final_output_json_schema: None, }) .await .unwrap(); @@ -494,6 +503,7 @@ async fn handle_responses_span_records_response_kind_and_tool_name() { items: vec![UserInput::Text { text: "hello".into(), }], + final_output_json_schema: None, }) .await .unwrap(); @@ -558,6 +568,7 @@ async fn record_responses_sets_span_fields_for_response_events() { items: vec![UserInput::Text { text: "hello".into(), }], + final_output_json_schema: None, }) .await .unwrap(); @@ -637,6 +648,7 @@ async fn handle_response_item_records_tool_result_for_custom_tool_call() { items: vec![UserInput::Text { text: "hello".into(), }], + final_output_json_schema: None, }) .await .unwrap(); @@ -704,6 +716,7 @@ async fn handle_response_item_records_tool_result_for_function_call() { items: vec![UserInput::Text { text: "hello".into(), }], + final_output_json_schema: None, }) .await .unwrap(); @@ -781,6 +794,7 @@ async fn handle_response_item_records_tool_result_for_local_shell_missing_ids() items: vec![UserInput::Text { text: "hello".into(), }], + final_output_json_schema: None, }) .await .unwrap(); @@ -842,6 +856,7 @@ async fn handle_response_item_records_tool_result_for_local_shell_call() { items: vec![UserInput::Text { text: "hello".into(), }], + final_output_json_schema: None, }) .await .unwrap(); @@ -946,6 +961,7 @@ async fn handle_container_exec_autoapprove_from_config_records_tool_decision() { items: vec![UserInput::Text { text: "hello".into(), }], + final_output_json_schema: None, }) .await .unwrap(); @@ -994,6 +1010,7 @@ async fn handle_container_exec_user_approved_records_tool_decision() { items: vec![UserInput::Text { text: "approved".into(), }], + final_output_json_schema: None, }) .await .unwrap(); @@ -1052,6 +1069,7 @@ async fn handle_container_exec_user_approved_for_session_records_tool_decision() items: vec![UserInput::Text { text: "persist".into(), }], + final_output_json_schema: None, }) .await .unwrap(); @@ -1110,6 +1128,7 @@ async fn handle_sandbox_error_user_approves_retry_records_tool_decision() { items: vec![UserInput::Text { text: "retry".into(), }], + final_output_json_schema: None, }) .await .unwrap(); @@ -1168,6 +1187,7 @@ async fn handle_container_exec_user_denies_records_tool_decision() { items: vec![UserInput::Text { text: "deny".into(), }], + final_output_json_schema: None, }) .await .unwrap(); @@ -1226,6 +1246,7 @@ async fn handle_sandbox_error_user_approves_for_session_records_tool_decision() items: vec![UserInput::Text { text: "persist".into(), }], + final_output_json_schema: None, }) .await .unwrap(); @@ -1285,6 +1306,7 @@ async fn handle_sandbox_error_user_denies_records_tool_decision() { items: vec![UserInput::Text { text: "deny".into(), }], + final_output_json_schema: None, }) .await .unwrap(); diff --git a/codex-rs/core/tests/suite/prompt_caching.rs b/codex-rs/core/tests/suite/prompt_caching.rs index c21174014d..f815e2e8a3 100644 --- a/codex-rs/core/tests/suite/prompt_caching.rs +++ b/codex-rs/core/tests/suite/prompt_caching.rs @@ -102,6 +102,7 @@ async fn prompt_tools_are_consistent_across_requests() -> anyhow::Result<()> { items: vec![UserInput::Text { text: "hello 1".into(), }], + final_output_json_schema: None, }) .await?; wait_for_event(&codex, |ev| matches!(ev, EventMsg::TaskComplete(_))).await; @@ -111,6 +112,7 @@ async fn prompt_tools_are_consistent_across_requests() -> anyhow::Result<()> { items: vec![UserInput::Text { text: "hello 2".into(), }], + final_output_json_schema: None, }) .await?; wait_for_event(&codex, |ev| matches!(ev, EventMsg::TaskComplete(_))).await; @@ -175,6 +177,7 @@ async fn codex_mini_latest_tools() -> anyhow::Result<()> { items: vec![UserInput::Text { text: "hello 1".into(), }], + final_output_json_schema: None, }) .await?; @@ -184,6 +187,7 @@ async fn codex_mini_latest_tools() -> anyhow::Result<()> { items: vec![UserInput::Text { text: "hello 2".into(), }], + final_output_json_schema: None, }) .await?; @@ -238,6 +242,7 @@ async fn prefixes_context_and_instructions_once_and_consistently_across_requests items: vec![UserInput::Text { text: "hello 1".into(), }], + final_output_json_schema: None, }) .await?; wait_for_event(&codex, |ev| matches!(ev, EventMsg::TaskComplete(_))).await; @@ -247,6 +252,7 @@ async fn prefixes_context_and_instructions_once_and_consistently_across_requests items: vec![UserInput::Text { text: "hello 2".into(), }], + final_output_json_schema: None, }) .await?; wait_for_event(&codex, |ev| matches!(ev, EventMsg::TaskComplete(_))).await; @@ -307,6 +313,7 @@ async fn overrides_turn_context_but_keeps_cached_prefix_and_key_constant() -> an items: vec![UserInput::Text { text: "hello 1".into(), }], + final_output_json_schema: None, }) .await?; wait_for_event(&codex, |ev| matches!(ev, EventMsg::TaskComplete(_))).await; @@ -334,6 +341,7 @@ async fn overrides_turn_context_but_keeps_cached_prefix_and_key_constant() -> an items: vec![UserInput::Text { text: "hello 2".into(), }], + final_output_json_schema: None, }) .await?; wait_for_event(&codex, |ev| matches!(ev, EventMsg::TaskComplete(_))).await; @@ -412,6 +420,7 @@ async fn override_before_first_turn_emits_environment_context() -> anyhow::Resul items: vec![UserInput::Text { text: "first message".into(), }], + final_output_json_schema: None, }) .await?; @@ -504,6 +513,7 @@ async fn per_turn_overrides_keep_cached_prefix_and_key_constant() -> anyhow::Res items: vec![UserInput::Text { text: "hello 1".into(), }], + final_output_json_schema: None, }) .await?; wait_for_event(&codex, |ev| matches!(ev, EventMsg::TaskComplete(_))).await; diff --git a/codex-rs/core/tests/suite/quota_exceeded.rs b/codex-rs/core/tests/suite/quota_exceeded.rs index 0156c8d116..e7ccd53847 100644 --- a/codex-rs/core/tests/suite/quota_exceeded.rs +++ b/codex-rs/core/tests/suite/quota_exceeded.rs @@ -44,6 +44,7 @@ async fn quota_exceeded_emits_single_error_event() -> Result<()> { items: vec![UserInput::Text { text: "quota?".into(), }], + final_output_json_schema: None, }) .await .unwrap(); diff --git a/codex-rs/core/tests/suite/resume.rs b/codex-rs/core/tests/suite/resume.rs index bda27533cd..1fee3858e2 100644 --- a/codex-rs/core/tests/suite/resume.rs +++ b/codex-rs/core/tests/suite/resume.rs @@ -37,6 +37,7 @@ async fn resume_includes_initial_messages_from_rollout_events() -> Result<()> { items: vec![UserInput::Text { text: "Record some messages".into(), }], + final_output_json_schema: None, }) .await?; @@ -89,6 +90,7 @@ async fn resume_includes_initial_messages_from_reasoning_events() -> Result<()> items: vec![UserInput::Text { text: "Record reasoning messages".into(), }], + final_output_json_schema: None, }) .await?; diff --git a/codex-rs/core/tests/suite/review.rs b/codex-rs/core/tests/suite/review.rs index fba7af588c..b35213f7e6 100644 --- a/codex-rs/core/tests/suite/review.rs +++ b/codex-rs/core/tests/suite/review.rs @@ -665,6 +665,7 @@ async fn review_history_surfaces_in_parent_session() { items: vec![UserInput::Text { text: followup.clone(), }], + final_output_json_schema: None, }) .await .unwrap(); @@ -709,6 +710,105 @@ async fn review_history_surfaces_in_parent_session() { server.verify().await; } +/// `/review` should use the session's current cwd (including runtime overrides) +/// when resolving base-branch review prompts (merge-base computation). +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn review_uses_overridden_cwd_for_base_branch_merge_base() { + skip_if_no_network!(); + + let sse_raw = r#"[{"type":"response.completed", "response": {"id": "__ID__"}}]"#; + let server = start_responses_server_with_sse(sse_raw, 1).await; + + let initial_cwd = TempDir::new().unwrap(); + + let repo_dir = TempDir::new().unwrap(); + let repo_path = repo_dir.path(); + + fn run_git(repo_path: &std::path::Path, args: &[&str]) { + let output = std::process::Command::new("git") + .arg("-C") + .arg(repo_path) + .args(args) + .output() + .expect("spawn git"); + assert!( + output.status.success(), + "git {:?} failed: stdout={:?} stderr={:?}", + args, + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ); + } + + run_git(repo_path, &["init", "-b", "main"]); + run_git(repo_path, &["config", "user.email", "test@example.com"]); + run_git(repo_path, &["config", "user.name", "Test User"]); + std::fs::write(repo_path.join("file.txt"), "hello\n").unwrap(); + run_git(repo_path, &["add", "."]); + run_git(repo_path, &["commit", "-m", "initial"]); + + let head_sha = std::process::Command::new("git") + .arg("-C") + .arg(repo_path) + .args(["rev-parse", "HEAD"]) + .output() + .expect("rev-parse HEAD"); + assert!(head_sha.status.success()); + let head_sha = String::from_utf8(head_sha.stdout) + .expect("utf8 sha") + .trim() + .to_string(); + + let codex_home = TempDir::new().unwrap(); + let codex = new_conversation_for_server(&server, &codex_home, |config| { + config.cwd = initial_cwd.path().to_path_buf(); + }) + .await; + + codex + .submit(Op::OverrideTurnContext { + cwd: Some(repo_path.to_path_buf()), + approval_policy: None, + sandbox_policy: None, + model: None, + effort: None, + summary: None, + }) + .await + .unwrap(); + + codex + .submit(Op::Review { + review_request: ReviewRequest { + target: ReviewTarget::BaseBranch { + branch: "main".to_string(), + }, + user_facing_hint: None, + }, + }) + .await + .unwrap(); + + let _entered = wait_for_event(&codex, |ev| matches!(ev, EventMsg::EnteredReviewMode(_))).await; + let _complete = wait_for_event(&codex, |ev| matches!(ev, EventMsg::TaskComplete(_))).await; + + let requests = get_responses_requests(&server).await; + assert_eq!(requests.len(), 1); + let body = requests[0].body_json::().unwrap(); + let input = body["input"].as_array().expect("input array"); + + let saw_merge_base_sha = input + .iter() + .filter_map(|msg| msg["content"][0]["text"].as_str()) + .any(|text| text.contains(&head_sha)); + assert!( + saw_merge_base_sha, + "expected review prompt to include merge-base sha {head_sha}" + ); + + server.verify().await; +} + /// Start a mock Responses API server and mount the given SSE stream body. async fn start_responses_server_with_sse(sse_raw: &str, expected_requests: usize) -> MockServer { let server = MockServer::start().await; diff --git a/codex-rs/core/tests/suite/stream_error_allows_next_turn.rs b/codex-rs/core/tests/suite/stream_error_allows_next_turn.rs index e6f8aa9560..e7a6091264 100644 --- a/codex-rs/core/tests/suite/stream_error_allows_next_turn.rs +++ b/codex-rs/core/tests/suite/stream_error_allows_next_turn.rs @@ -89,6 +89,7 @@ async fn continue_after_stream_error() { items: vec![UserInput::Text { text: "first message".into(), }], + final_output_json_schema: None, }) .await .unwrap(); @@ -106,6 +107,7 @@ async fn continue_after_stream_error() { items: vec![UserInput::Text { text: "follow up".into(), }], + final_output_json_schema: None, }) .await .unwrap(); diff --git a/codex-rs/core/tests/suite/stream_no_completed.rs b/codex-rs/core/tests/suite/stream_no_completed.rs index 17a19e7c34..a203658c2f 100644 --- a/codex-rs/core/tests/suite/stream_no_completed.rs +++ b/codex-rs/core/tests/suite/stream_no_completed.rs @@ -96,6 +96,7 @@ async fn retries_on_early_close() { items: vec![UserInput::Text { text: "hello".into(), }], + final_output_json_schema: None, }) .await .unwrap(); diff --git a/codex-rs/core/tests/suite/user_notification.rs b/codex-rs/core/tests/suite/user_notification.rs index 0d1e941d28..a3289b87f7 100644 --- a/codex-rs/core/tests/suite/user_notification.rs +++ b/codex-rs/core/tests/suite/user_notification.rs @@ -61,6 +61,7 @@ echo -n "${@: -1}" > $(dirname "${0}")/notify.txt"#, items: vec![UserInput::Text { text: "hello world".into(), }], + final_output_json_schema: None, }) .await?; wait_for_event(&codex, |ev| matches!(ev, EventMsg::TaskComplete(_))).await; diff --git a/codex-rs/docs/codex_mcp_interface.md b/codex-rs/docs/codex_mcp_interface.md index 124e2f91dc..edd5ac1b2d 100644 --- a/codex-rs/docs/codex_mcp_interface.md +++ b/codex-rs/docs/codex_mcp_interface.md @@ -70,7 +70,9 @@ Response: `{ conversationId, model, reasoningEffort?, rolloutPath }` Send input to the active turn: - `sendUserMessage` → enqueue items to the conversation -- `sendUserTurn` → structured turn with explicit `cwd`, `approvalPolicy`, `sandboxPolicy`, `model`, optional `effort`, and `summary` +- `sendUserTurn` → structured turn with explicit `cwd`, `approvalPolicy`, `sandboxPolicy`, `model`, optional `effort`, `summary`, and optional `outputSchema` (JSON Schema for the final assistant message) + +For v2 threads, `turn/start` also accepts `outputSchema` to constrain the final assistant message for that turn. Interrupt a running turn: `interruptConversation`. diff --git a/codex-rs/feedback/Cargo.toml b/codex-rs/feedback/Cargo.toml index 0ac0351333..73803af86a 100644 --- a/codex-rs/feedback/Cargo.toml +++ b/codex-rs/feedback/Cargo.toml @@ -8,6 +8,7 @@ license.workspace = true anyhow = { workspace = true } codex-protocol = { workspace = true } sentry = { version = "0.46" } +tracing = { workspace = true } tracing-subscriber = { workspace = true } [dev-dependencies] diff --git a/codex-rs/feedback/src/lib.rs b/codex-rs/feedback/src/lib.rs index eaa949717a..2096f4505b 100644 --- a/codex-rs/feedback/src/lib.rs +++ b/codex-rs/feedback/src/lib.rs @@ -1,4 +1,6 @@ +use std::collections::BTreeMap; use std::collections::VecDeque; +use std::collections::btree_map::Entry; use std::fs; use std::io::Write; use std::io::{self}; @@ -11,12 +13,20 @@ use anyhow::Result; use anyhow::anyhow; use codex_protocol::ConversationId; use codex_protocol::protocol::SessionSource; +use tracing::Event; +use tracing::Level; +use tracing::field::Visit; +use tracing_subscriber::Layer; +use tracing_subscriber::filter::Targets; use tracing_subscriber::fmt::writer::MakeWriter; +use tracing_subscriber::registry::LookupSpan; const DEFAULT_MAX_BYTES: usize = 4 * 1024 * 1024; // 4 MiB const SENTRY_DSN: &str = "https://ae32ed50620d7a7792c1ce5df38b3e3e@o33249.ingest.us.sentry.io/4510195390611458"; const UPLOAD_TIMEOUT_SECS: u64 = 10; +const FEEDBACK_TAGS_TARGET: &str = "feedback_tags"; +const MAX_FEEDBACK_TAGS: usize = 64; #[derive(Clone)] pub struct CodexFeedback { @@ -46,13 +56,50 @@ impl CodexFeedback { } } + /// Returns a [`tracing_subscriber`] layer that captures full-fidelity logs into this feedback + /// ring buffer. + /// + /// This is intended for initialization code so call sites don't have to duplicate the exact + /// `fmt::layer()` configuration and filter logic. + pub fn logger_layer(&self) -> impl Layer + Send + Sync + 'static + where + S: tracing::Subscriber + for<'a> LookupSpan<'a>, + { + tracing_subscriber::fmt::layer() + .with_writer(self.make_writer()) + .with_ansi(false) + .with_target(false) + // Capture everything, regardless of the caller's `RUST_LOG`, so feedback includes the + // full trace when the user uploads a report. + .with_filter(Targets::new().with_default(Level::TRACE)) + } + + /// Returns a [`tracing_subscriber`] layer that collects structured metadata for feedback. + /// + /// Events with `target: "feedback_tags"` are treated as key/value tags to attach to feedback + /// uploads later. + pub fn metadata_layer(&self) -> impl Layer + Send + Sync + 'static + where + S: tracing::Subscriber + for<'a> LookupSpan<'a>, + { + FeedbackMetadataLayer { + inner: self.inner.clone(), + } + .with_filter(Targets::new().with_target(FEEDBACK_TAGS_TARGET, Level::TRACE)) + } + pub fn snapshot(&self, session_id: Option) -> CodexLogSnapshot { let bytes = { let guard = self.inner.ring.lock().expect("mutex poisoned"); guard.snapshot_bytes() }; + let tags = { + let guard = self.inner.tags.lock().expect("mutex poisoned"); + guard.clone() + }; CodexLogSnapshot { bytes, + tags, thread_id: session_id .map(|id| id.to_string()) .unwrap_or("no-active-thread-".to_string() + &ConversationId::new().to_string()), @@ -62,12 +109,14 @@ impl CodexFeedback { struct FeedbackInner { ring: Mutex, + tags: Mutex>, } impl FeedbackInner { fn new(max_bytes: usize) -> Self { Self { ring: Mutex::new(RingBuffer::new(max_bytes)), + tags: Mutex::new(BTreeMap::new()), } } } @@ -152,6 +201,7 @@ impl RingBuffer { pub struct CodexLogSnapshot { bytes: Vec, + tags: BTreeMap, pub thread_id: String, } @@ -212,6 +262,22 @@ impl CodexLogSnapshot { tags.insert(String::from("reason"), r.to_string()); } + let reserved = [ + "thread_id", + "classification", + "cli_version", + "session_source", + "reason", + ]; + for (key, value) in &self.tags { + if reserved.contains(&key.as_str()) { + continue; + } + if let Entry::Vacant(entry) = tags.entry(key.clone()) { + entry.insert(value.clone()); + } + } + let level = match classification { "bug" | "bad_result" => Level::Error, _ => Level::Info, @@ -280,9 +346,80 @@ fn display_classification(classification: &str) -> String { } } +#[derive(Clone)] +struct FeedbackMetadataLayer { + inner: Arc, +} + +impl Layer for FeedbackMetadataLayer +where + S: tracing::Subscriber + for<'a> LookupSpan<'a>, +{ + fn on_event(&self, event: &Event<'_>, _ctx: tracing_subscriber::layer::Context<'_, S>) { + // This layer is filtered by `Targets`, but keep the guard anyway in case it is used without + // the filter. + if event.metadata().target() != FEEDBACK_TAGS_TARGET { + return; + } + + let mut visitor = FeedbackTagsVisitor::default(); + event.record(&mut visitor); + if visitor.tags.is_empty() { + return; + } + + let mut guard = self.inner.tags.lock().expect("mutex poisoned"); + for (key, value) in visitor.tags { + if guard.len() >= MAX_FEEDBACK_TAGS && !guard.contains_key(&key) { + continue; + } + guard.insert(key, value); + } + } +} + +#[derive(Default)] +struct FeedbackTagsVisitor { + tags: BTreeMap, +} + +impl Visit for FeedbackTagsVisitor { + fn record_i64(&mut self, field: &tracing::field::Field, value: i64) { + self.tags + .insert(field.name().to_string(), value.to_string()); + } + + fn record_u64(&mut self, field: &tracing::field::Field, value: u64) { + self.tags + .insert(field.name().to_string(), value.to_string()); + } + + fn record_bool(&mut self, field: &tracing::field::Field, value: bool) { + self.tags + .insert(field.name().to_string(), value.to_string()); + } + + fn record_f64(&mut self, field: &tracing::field::Field, value: f64) { + self.tags + .insert(field.name().to_string(), value.to_string()); + } + + fn record_str(&mut self, field: &tracing::field::Field, value: &str) { + self.tags + .insert(field.name().to_string(), value.to_string()); + } + + fn record_debug(&mut self, field: &tracing::field::Field, value: &dyn std::fmt::Debug) { + self.tags + .insert(field.name().to_string(), format!("{value:?}")); + } +} + #[cfg(test)] mod tests { use super::*; + use tracing_subscriber::layer::SubscriberExt; + use tracing_subscriber::util::SubscriberInitExt; #[test] fn ring_buffer_drops_front_when_full() { @@ -296,4 +433,18 @@ mod tests { // Capacity 8: after writing 10 bytes, we should keep the last 8. pretty_assertions::assert_eq!(std::str::from_utf8(snap.as_bytes()).unwrap(), "cdefghij"); } + + #[test] + fn metadata_layer_records_tags_from_feedback_target() { + let fb = CodexFeedback::new(); + let _guard = tracing_subscriber::registry() + .with(fb.metadata_layer()) + .set_default(); + + tracing::info!(target: FEEDBACK_TAGS_TARGET, model = "gpt-5", cached = true, "tags"); + + let snap = fb.snapshot(None); + pretty_assertions::assert_eq!(snap.tags.get("model").map(String::as_str), Some("gpt-5")); + pretty_assertions::assert_eq!(snap.tags.get("cached").map(String::as_str), Some("true")); + } } diff --git a/codex-rs/login/src/device_code_auth.rs b/codex-rs/login/src/device_code_auth.rs index d9e7d90ce2..5864ff65d9 100644 --- a/codex-rs/login/src/device_code_auth.rs +++ b/codex-rs/login/src/device_code_auth.rs @@ -137,26 +137,27 @@ async fn poll_for_token( } } -fn print_device_code_prompt(code: &str) { +fn print_device_code_prompt(code: &str, issuer_base_url: &str) { println!( "\nWelcome to Codex [v{ANSI_GRAY}{version}{ANSI_RESET}]\n{ANSI_GRAY}OpenAI's command-line coding agent{ANSI_RESET}\n\ \nFollow these steps to sign in with ChatGPT using device code authorization:\n\ -\n1. Open this link in your browser and sign in to your account\n {ANSI_BLUE}https://auth.openai.com/codex/device{ANSI_RESET}\n\ +\n1. Open this link in your browser and sign in to your account\n {ANSI_BLUE}{issuer_base_url}/codex/device{ANSI_RESET}\n\ \n2. Enter this one-time code {ANSI_GRAY}(expires in 15 minutes){ANSI_RESET}\n {ANSI_BLUE}{code}{ANSI_RESET}\n\ \n{ANSI_GRAY}Device codes are a common phishing target. Never share this code.{ANSI_RESET}\n", version = env!("CARGO_PKG_VERSION"), - code = code + code = code, + issuer_base_url = issuer_base_url ); } /// Full device code login flow. pub async fn run_device_code_login(opts: ServerOptions) -> std::io::Result<()> { let client = reqwest::Client::new(); - let base_url = opts.issuer.trim_end_matches('/'); - let api_base_url = format!("{}/api/accounts", opts.issuer.trim_end_matches('/')); + let issuer_base_url = opts.issuer.trim_end_matches('/'); + let api_base_url = format!("{issuer_base_url}/api/accounts"); let uc = request_user_code(&client, &api_base_url, &opts.client_id).await?; - print_device_code_prompt(&uc.user_code); + print_device_code_prompt(&uc.user_code, issuer_base_url); let code_resp = poll_for_token( &client, @@ -171,10 +172,10 @@ pub async fn run_device_code_login(opts: ServerOptions) -> std::io::Result<()> { code_verifier: code_resp.code_verifier, code_challenge: code_resp.code_challenge, }; - let redirect_uri = format!("{base_url}/deviceauth/callback"); + let redirect_uri = format!("{issuer_base_url}/deviceauth/callback"); let tokens = crate::server::exchange_code_for_tokens( - base_url, + issuer_base_url, &opts.client_id, &redirect_uri, &pkce, diff --git a/codex-rs/mcp-server/src/codex_tool_runner.rs b/codex-rs/mcp-server/src/codex_tool_runner.rs index 39ae7486e5..b50d0c7640 100644 --- a/codex-rs/mcp-server/src/codex_tool_runner.rs +++ b/codex-rs/mcp-server/src/codex_tool_runner.rs @@ -94,6 +94,7 @@ pub async fn run_codex_tool_session( items: vec![UserInput::Text { text: initial_prompt.clone(), }], + final_output_json_schema: None, }, }; @@ -128,6 +129,7 @@ pub async fn run_codex_tool_session_reply( if let Err(e) = conversation .submit(Op::UserInput { items: vec![UserInput::Text { text: prompt }], + final_output_json_schema: None, }) .await { diff --git a/codex-rs/otel/Cargo.toml b/codex-rs/otel/Cargo.toml index 8c99326a4c..a703808e80 100644 --- a/codex-rs/otel/Cargo.toml +++ b/codex-rs/otel/Cargo.toml @@ -44,11 +44,6 @@ serde = { workspace = true, features = ["derive"] } serde_json = { workspace = true } strum_macros = { workspace = true } tokio = { workspace = true } -tonic = { workspace = true, features = [ - "transport", - "tls-native-roots", - "tls-ring", -] } tracing = { workspace = true } tracing-opentelemetry = { workspace = true } tracing-subscriber = { workspace = true } diff --git a/codex-rs/otel/src/otel_manager.rs b/codex-rs/otel/src/otel_manager.rs index 38d69001cd..fbdd322722 100644 --- a/codex-rs/otel/src/otel_manager.rs +++ b/codex-rs/otel/src/otel_manager.rs @@ -70,7 +70,7 @@ impl OtelManager { let session_span = trace_span!("new_session", conversation_id = %conversation_id, session_source = %session_source); if let Some(context) = traceparent_context_from_env() { - session_span.set_parent(context); + let _ = session_span.set_parent(context); } Self { diff --git a/codex-rs/otel/src/otel_provider.rs b/codex-rs/otel/src/otel_provider.rs index b9d9559325..8a777e7fdd 100644 --- a/codex-rs/otel/src/otel_provider.rs +++ b/codex-rs/otel/src/otel_provider.rs @@ -22,6 +22,10 @@ use opentelemetry_otlp::SpanExporter; use opentelemetry_otlp::WithExportConfig; use opentelemetry_otlp::WithHttpConfig; use opentelemetry_otlp::WithTonicConfig; +use opentelemetry_otlp::tonic_types::metadata::MetadataMap; +use opentelemetry_otlp::tonic_types::transport::Certificate as TonicCertificate; +use opentelemetry_otlp::tonic_types::transport::ClientTlsConfig; +use opentelemetry_otlp::tonic_types::transport::Identity as TonicIdentity; use opentelemetry_sdk::Resource; use opentelemetry_sdk::logs::SdkLoggerProvider; use opentelemetry_sdk::propagation::TraceContextPropagator; @@ -44,10 +48,6 @@ use std::io::{self}; use std::path::PathBuf; use std::sync::OnceLock; use std::time::Duration; -use tonic::metadata::MetadataMap; -use tonic::transport::Certificate as TonicCertificate; -use tonic::transport::ClientTlsConfig; -use tonic::transport::Identity as TonicIdentity; use tracing::debug; use tracing::level_filters::LevelFilter; use tracing::warn; @@ -102,7 +102,7 @@ impl OtelProvider { .map(|provider| provider.tracer(settings.service_name.clone())); if let Some(provider) = tracer_provider.clone() { - let _ = global::set_tracer_provider(provider); + global::set_tracer_provider(provider); global::set_text_map_propagator(TraceContextPropagator::new()); } if tracer.is_some() { diff --git a/codex-rs/protocol/src/protocol.rs b/codex-rs/protocol/src/protocol.rs index cff2a8ad98..f9369de9c5 100644 --- a/codex-rs/protocol/src/protocol.rs +++ b/codex-rs/protocol/src/protocol.rs @@ -74,6 +74,9 @@ pub enum Op { UserInput { /// User input items, see `InputItem` items: Vec, + /// Optional JSON Schema used to constrain the final assistant message for this turn. + #[serde(skip_serializing_if = "Option::is_none")] + final_output_json_schema: Option, }, /// Similar to [`Op::UserInput`], but contains additional context required @@ -844,7 +847,7 @@ pub struct TaskStartedEvent { pub model_context_window: Option, } -#[derive(Debug, Clone, Deserialize, Serialize, Default, JsonSchema, TS)] +#[derive(Debug, Clone, Deserialize, Serialize, Default, PartialEq, Eq, JsonSchema, TS)] pub struct TokenUsage { #[ts(type = "number")] pub input_tokens: i64, @@ -858,7 +861,7 @@ pub struct TokenUsage { pub total_tokens: i64, } -#[derive(Debug, Clone, Deserialize, Serialize, JsonSchema, TS)] +#[derive(Debug, Clone, Deserialize, Serialize, PartialEq, Eq, JsonSchema, TS)] pub struct TokenUsageInfo { pub total_token_usage: TokenUsage, pub last_token_usage: TokenUsage, @@ -1928,6 +1931,62 @@ mod tests { assert!(event.as_legacy_events(false).is_empty()); } + #[test] + fn user_input_serialization_omits_final_output_json_schema_when_none() -> Result<()> { + let op = Op::UserInput { + items: Vec::new(), + final_output_json_schema: None, + }; + + let json_op = serde_json::to_value(op)?; + assert_eq!(json_op, json!({ "type": "user_input", "items": [] })); + + Ok(()) + } + + #[test] + fn user_input_deserializes_without_final_output_json_schema_field() -> Result<()> { + let op: Op = serde_json::from_value(json!({ "type": "user_input", "items": [] }))?; + + assert_eq!( + op, + Op::UserInput { + items: Vec::new(), + final_output_json_schema: None, + } + ); + + Ok(()) + } + + #[test] + fn user_input_serialization_includes_final_output_json_schema_when_some() -> Result<()> { + let schema = json!({ + "type": "object", + "properties": { + "answer": { "type": "string" } + }, + "required": ["answer"], + "additionalProperties": false + }); + let op = Op::UserInput { + items: Vec::new(), + final_output_json_schema: Some(schema.clone()), + }; + + let json_op = serde_json::to_value(op)?; + assert_eq!( + json_op, + json!({ + "type": "user_input", + "items": [], + "final_output_json_schema": schema, + }) + ); + + Ok(()) + } + /// Serialize Event to verify that its JSON representation has the expected /// amount of nesting. #[test] diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index aad9f46457..b3cb3ff24d 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -1985,7 +1985,10 @@ impl ChatWidget { } self.codex_op_tx - .send(Op::UserInput { items }) + .send(Op::UserInput { + items, + final_output_json_schema: None, + }) .unwrap_or_else(|e| { tracing::error!("failed to send message: {e}"); }); diff --git a/codex-rs/tui/src/lib.rs b/codex-rs/tui/src/lib.rs index bce9a350f4..6b784affce 100644 --- a/codex-rs/tui/src/lib.rs +++ b/codex-rs/tui/src/lib.rs @@ -29,7 +29,6 @@ use std::path::PathBuf; use tracing::error; use tracing_appender::non_blocking; use tracing_subscriber::EnvFilter; -use tracing_subscriber::filter::Targets; use tracing_subscriber::prelude::*; mod additional_dirs; @@ -282,13 +281,8 @@ pub async fn run_main( .with_filter(env_filter()); let feedback = codex_feedback::CodexFeedback::new(); - let targets = Targets::new().with_default(tracing::Level::TRACE); - - let feedback_layer = tracing_subscriber::fmt::layer() - .with_writer(feedback.make_writer()) - .with_ansi(false) - .with_target(false) - .with_filter(targets); + let feedback_layer = feedback.logger_layer(); + let feedback_metadata_layer = feedback.metadata_layer(); if cli.oss && model_provider_override.is_some() { // We're in the oss section, so provider_id should be Some @@ -323,6 +317,7 @@ pub async fn run_main( let _ = tracing_subscriber::registry() .with(file_layer) .with(feedback_layer) + .with(feedback_metadata_layer) .with(otel_logger_layer) .with(otel_tracing_layer) .try_init(); diff --git a/codex-rs/tui/src/terminal_palette.rs b/codex-rs/tui/src/terminal_palette.rs index 5c6f32cd9e..6349c007eb 100644 --- a/codex-rs/tui/src/terminal_palette.rs +++ b/codex-rs/tui/src/terminal_palette.rs @@ -1,5 +1,13 @@ use crate::color::perceptual_distance; use ratatui::style::Color; +use std::sync::atomic::AtomicU64; +use std::sync::atomic::Ordering; + +static DEFAULT_PALETTE_VERSION: AtomicU64 = AtomicU64::new(0); + +fn bump_palette_version() { + DEFAULT_PALETTE_VERSION.fetch_add(1, Ordering::Relaxed); +} /// Returns the closest color to the target color that the terminal can display. pub fn best_color(target: (u8, u8, u8)) -> Color { @@ -27,6 +35,7 @@ pub fn best_color(target: (u8, u8, u8)) -> Color { pub fn requery_default_colors() { imp::requery_default_colors(); + bump_palette_version(); } #[derive(Clone, Copy)] @@ -47,6 +56,14 @@ pub fn default_bg() -> Option<(u8, u8, u8)> { default_colors().map(|c| c.bg) } +/// Returns a monotonic counter that increments whenever `requery_default_colors()` runs +/// successfully so cached renderers can know when their styling assumptions (e.g. +/// background colors baked into cached transcript rows) are stale and need invalidation. +#[allow(dead_code)] +pub fn palette_version() -> u64 { + DEFAULT_PALETTE_VERSION.load(Ordering::Relaxed) +} + #[cfg(all(unix, not(test)))] mod imp { use super::DefaultColors; diff --git a/codex-rs/tui2/Cargo.toml b/codex-rs/tui2/Cargo.toml index eb4e9cebde..3108e5561e 100644 --- a/codex-rs/tui2/Cargo.toml +++ b/codex-rs/tui2/Cargo.toml @@ -62,6 +62,7 @@ ratatui = { workspace = true, features = [ "unstable-rendered-line-info", "unstable-widget-ref", ] } +ratatui-core = { workspace = true } ratatui-macros = { workspace = true } regex-lite = { workspace = true } reqwest = { version = "0.12", features = ["json"] } @@ -73,6 +74,7 @@ strum_macros = { workspace = true } supports-color = { workspace = true } tempfile = { workspace = true } textwrap = { workspace = true } +tui-scrollbar = { workspace = true } tokio = { workspace = true, features = [ "io-std", "macros", diff --git a/codex-rs/tui2/src/app.rs b/codex-rs/tui2/src/app.rs index c9f6d6f64c..0bee111f7d 100644 --- a/codex-rs/tui2/src/app.rs +++ b/codex-rs/tui2/src/app.rs @@ -3,7 +3,6 @@ use crate::app_event::AppEvent; use crate::app_event_sender::AppEventSender; use crate::bottom_pane::ApprovalRequest; use crate::chatwidget::ChatWidget; -use crate::clipboard_copy; use crate::custom_terminal::Frame; use crate::diff_render::DiffSummary; use crate::exec_command::strip_bash_lc_and_escape; @@ -16,11 +15,19 @@ use crate::pager_overlay::Overlay; use crate::render::highlight::highlight_bash_to_lines; use crate::render::renderable::Renderable; use crate::resume_picker::ResumeSelection; +use crate::transcript_copy_action::TranscriptCopyAction; +use crate::transcript_copy_action::TranscriptCopyFeedback; use crate::transcript_copy_ui::TranscriptCopyUi; use crate::transcript_multi_click::TranscriptMultiClick; +use crate::transcript_scrollbar::render_transcript_scrollbar_if_active; +use crate::transcript_scrollbar::split_transcript_area; +use crate::transcript_scrollbar_ui::TranscriptScrollbarMouseEvent; +use crate::transcript_scrollbar_ui::TranscriptScrollbarMouseHandling; +use crate::transcript_scrollbar_ui::TranscriptScrollbarUi; use crate::transcript_selection::TRANSCRIPT_GUTTER_COLS; use crate::transcript_selection::TranscriptSelection; use crate::transcript_selection::TranscriptSelectionPoint; +use crate::transcript_view_cache::TranscriptViewCache; use crate::tui; use crate::tui::TuiEvent; use crate::tui::scrolling::MouseScrollState; @@ -28,7 +35,6 @@ use crate::tui::scrolling::ScrollConfig; use crate::tui::scrolling::ScrollConfigOverrides; use crate::tui::scrolling::ScrollDirection; use crate::tui::scrolling::ScrollUpdate; -use crate::tui::scrolling::TranscriptLineMeta; use crate::tui::scrolling::TranscriptScroll; use crate::update_action::UpdateAction; use codex_ansi_escape::ansi_escape_line; @@ -164,6 +170,7 @@ pub(crate) struct App { pub(crate) file_search: FileSearchManager, pub(crate) transcript_cells: Vec>, + transcript_view_cache: TranscriptViewCache, #[allow(dead_code)] transcript_scroll: TranscriptScroll, @@ -172,6 +179,8 @@ pub(crate) struct App { transcript_view_top: usize, transcript_total_lines: usize, transcript_copy_ui: TranscriptCopyUi, + transcript_copy_action: TranscriptCopyAction, + transcript_scrollbar_ui: TranscriptScrollbarUi, // Pager overlay state (Transcript or Static like Diff) pub(crate) overlay: Option, @@ -333,12 +342,15 @@ impl App { file_search, enhanced_keys_supported, transcript_cells: Vec::new(), + transcript_view_cache: TranscriptViewCache::new(), transcript_scroll: TranscriptScroll::default(), transcript_selection: TranscriptSelection::default(), transcript_multi_click: TranscriptMultiClick::default(), transcript_view_top: 0, transcript_total_lines: 0, transcript_copy_ui: TranscriptCopyUi::new_with_shortcut(copy_selection_shortcut), + transcript_copy_action: TranscriptCopyAction::default(), + transcript_scrollbar_ui: TranscriptScrollbarUi::default(), overlay: None, deferred_history_lines: Vec::new(), has_emitted_history_lines: false, @@ -506,11 +518,14 @@ impl App { self.transcript_total_lines, )) }; + let copy_selection_key = self.copy_selection_key(); + let copy_feedback = self.transcript_copy_feedback_for_footer(); self.chat_widget.set_transcript_ui_state( transcript_scrolled, selection_active, scroll_position, - self.copy_selection_key(), + copy_selection_key, + copy_feedback, ); } } @@ -541,35 +556,33 @@ impl App { return area.y; } - let transcript_area = Rect { + let transcript_full_area = Rect { x: area.x, y: area.y, width: area.width, height: max_transcript_height, }; + let (transcript_area, _) = split_transcript_area(transcript_full_area); - let transcript = - crate::transcript_render::build_wrapped_transcript_lines(cells, transcript_area.width); - let (lines, line_meta) = (transcript.lines, transcript.meta); - if lines.is_empty() { - Clear.render_ref(transcript_area, frame.buffer); + self.transcript_view_cache + .ensure_wrapped(cells, transcript_area.width); + let total_lines = self.transcript_view_cache.lines().len(); + if total_lines == 0 { + Clear.render_ref(transcript_full_area, frame.buffer); self.transcript_scroll = TranscriptScroll::default(); self.transcript_view_top = 0; self.transcript_total_lines = 0; return area.y; } - let is_user_cell: Vec = cells - .iter() - .map(|c| c.as_any().is::()) - .collect(); - - let total_lines = lines.len(); self.transcript_total_lines = total_lines; let max_visible = std::cmp::min(max_transcript_height as usize, total_lines); let max_start = total_lines.saturating_sub(max_visible); - let (scroll_state, top_offset) = self.transcript_scroll.resolve_top(&line_meta, max_start); + let (scroll_state, top_offset) = { + let line_meta = self.transcript_view_cache.line_meta(); + self.transcript_scroll.resolve_top(line_meta, max_start) + }; self.transcript_scroll = scroll_state; self.transcript_view_top = top_offset; @@ -596,12 +609,19 @@ impl App { ); } - let transcript_area = Rect { + let transcript_full_area = Rect { x: area.x, y: area.y, width: area.width, height: transcript_visible_height, }; + let (transcript_area, transcript_scrollbar_area) = + split_transcript_area(transcript_full_area); + + // Cache a few viewports worth of rasterized rows so redraws during streaming can cheaply + // copy already-rendered `Cell`s instead of re-running grapheme segmentation. + self.transcript_view_cache + .set_raster_capacity(max_visible.saturating_mul(4).max(256)); for (row_index, line_index) in (top_offset..total_lines).enumerate() { if row_index >= max_visible { @@ -616,21 +636,8 @@ impl App { height: 1, }; - let is_user_row = line_meta - .get(line_index) - .and_then(TranscriptLineMeta::cell_index) - .map(|cell_index| is_user_cell.get(cell_index).copied().unwrap_or(false)) - .unwrap_or(false); - if is_user_row { - let base_style = crate::style::user_message_style(); - for x in row_area.x..row_area.right() { - let cell = &mut frame.buffer[(x, y)]; - let style = cell.style().patch(base_style); - cell.set_style(style); - } - } - - lines[line_index].render_ref(row_area, frame.buffer); + self.transcript_view_cache + .render_row_index_into(line_index, row_area, frame.buffer); } self.apply_transcript_selection(transcript_area, frame.buffer); @@ -650,6 +657,13 @@ impl App { } else { self.transcript_copy_ui.clear_affordance(); } + render_transcript_scrollbar_if_active( + frame.buffer, + transcript_scrollbar_area, + total_lines, + max_visible, + top_offset, + ); chat_top } @@ -698,21 +712,45 @@ impl App { return; } - let transcript_area = Rect { + let transcript_full_area = Rect { x: 0, y: 0, width, height: transcript_height, }; + let (transcript_area, transcript_scrollbar_area) = + split_transcript_area(transcript_full_area); let base_x = transcript_area.x.saturating_add(TRANSCRIPT_GUTTER_COLS); let max_x = transcript_area.right().saturating_sub(1); + if matches!( + self.transcript_scrollbar_ui + .handle_mouse_event(TranscriptScrollbarMouseEvent { + tui, + mouse_event, + transcript_area, + scrollbar_area: transcript_scrollbar_area, + transcript_cells: &self.transcript_cells, + transcript_view_cache: &mut self.transcript_view_cache, + transcript_scroll: &mut self.transcript_scroll, + transcript_view_top: &mut self.transcript_view_top, + transcript_total_lines: &mut self.transcript_total_lines, + mouse_scroll_state: &mut self.scroll_state, + }), + TranscriptScrollbarMouseHandling::Handled + ) { + return; + } + // Treat the transcript as the only interactive region for transcript selection. // // This prevents clicks in the composer/footer from starting or extending a transcript // selection, while still allowing a left-click outside the transcript to clear an // existing highlight. - if mouse_event.row < transcript_area.y || mouse_event.row >= transcript_area.bottom() { + if !self.transcript_scrollbar_ui.pointer_capture_active() + && (mouse_event.row < transcript_full_area.y + || mouse_event.row >= transcript_full_area.bottom()) + { if matches!( mouse_event.kind, MouseEventKind::Down(MouseButton::Left) | MouseEventKind::Up(MouseButton::Left) @@ -743,7 +781,14 @@ impl App { .transcript_copy_ui .hit_test(mouse_event.column, mouse_event.row) { - self.copy_transcript_selection(tui); + if self.transcript_copy_action.copy_and_handle( + tui, + chat_height, + &self.transcript_cells, + self.transcript_selection, + ) { + self.transcript_selection = TranscriptSelection::default(); + } return; } @@ -919,7 +964,15 @@ impl App { return None; } - Some((transcript_height as usize, width)) + let transcript_full_area = Rect { + x: 0, + y: 0, + width, + height: transcript_height, + }; + let (transcript_area, _) = split_transcript_area(transcript_full_area); + + Some((transcript_height as usize, transcript_area.width)) } /// Scroll the transcript by a number of visual lines. @@ -943,12 +996,12 @@ impl App { return; } - let transcript = - crate::transcript_render::build_wrapped_transcript_lines(&self.transcript_cells, width); - let line_meta = transcript.meta; + self.transcript_view_cache + .ensure_wrapped(&self.transcript_cells, width); + let line_meta = self.transcript_view_cache.line_meta(); self.transcript_scroll = self.transcript_scroll - .scrolled_by(delta_lines, &line_meta, visible_lines); + .scrolled_by(delta_lines, line_meta, visible_lines); if schedule_frame { // Request a redraw; the frame scheduler coalesces bursts and clamps to 60fps. @@ -968,9 +1021,10 @@ impl App { return; } - let transcript = - crate::transcript_render::build_wrapped_transcript_lines(&self.transcript_cells, width); - let (lines, line_meta) = (transcript.lines, transcript.meta); + self.transcript_view_cache + .ensure_wrapped(&self.transcript_cells, width); + let lines = self.transcript_view_cache.lines(); + let line_meta = self.transcript_view_cache.line_meta(); if lines.is_empty() || line_meta.is_empty() { return; } @@ -984,13 +1038,14 @@ impl App { let max_start = total_lines.saturating_sub(max_visible); let top_offset = match self.transcript_scroll { TranscriptScroll::ToBottom => max_start, - TranscriptScroll::Scrolled { .. } => { + TranscriptScroll::Scrolled { .. } + | TranscriptScroll::ScrolledSpacerBeforeCell { .. } => { // Already anchored; nothing to lock. return; } }; - if let Some(scroll_state) = TranscriptScroll::anchor_for(&line_meta, top_offset) { + if let Some(scroll_state) = TranscriptScroll::anchor_for(line_meta, top_offset) { self.transcript_scroll = scroll_state; } } @@ -1087,46 +1142,8 @@ impl App { } } - /// Copy the currently selected transcript region to the system clipboard. - /// - /// The selection is defined in terms of flattened wrapped transcript line - /// indices and columns, and this method reconstructs the same wrapped - /// transcript used for on-screen rendering so the copied text closely - /// matches the highlighted region. - /// - /// Important: copy operates on the selection's full content-relative range, - /// not just the current viewport. A selection can extend outside the visible - /// region (for example, by scrolling after selecting, or by selecting while - /// autoscrolling), and we still want the clipboard payload to reflect the - /// entire selected transcript. - fn copy_transcript_selection(&mut self, tui: &tui::Tui) { - let size = tui.terminal.last_known_screen_size; - let width = size.width; - let height = size.height; - if width == 0 || height == 0 { - return; - } - - let chat_height = self.chat_widget.desired_height(width); - if chat_height >= height { - return; - } - - let transcript_height = height.saturating_sub(chat_height); - if transcript_height == 0 { - return; - } - - let Some(text) = crate::transcript_copy::selection_to_copy_text_for_cells( - &self.transcript_cells, - self.transcript_selection, - width, - ) else { - return; - }; - if let Err(err) = clipboard_copy::copy_text(text) { - tracing::error!(error = %err, "failed to copy selection to clipboard"); - } + fn transcript_copy_feedback_for_footer(&mut self) -> Option { + self.transcript_copy_action.footer_feedback() } fn copy_selection_key(&self) -> crate::key_hint::KeyBinding { @@ -1732,7 +1749,22 @@ impl App { kind: KeyEventKind::Press | KeyEventKind::Repeat, .. } if self.transcript_copy_ui.is_copy_key(ch, modifiers) => { - self.copy_transcript_selection(tui); + let size = tui.terminal.last_known_screen_size; + let width = size.width; + let height = size.height; + if width == 0 || height == 0 { + return; + } + + let chat_height = self.chat_widget.desired_height(width); + if self.transcript_copy_action.copy_and_handle( + tui, + chat_height, + &self.transcript_cells, + self.transcript_selection, + ) { + self.transcript_selection = TranscriptSelection::default(); + } } KeyEvent { code: KeyCode::PageUp, @@ -1876,6 +1908,7 @@ mod tests { use crate::history_cell::UserHistoryCell; use crate::history_cell::new_session_info; use crate::transcript_copy_ui::CopySelectionShortcut; + use crate::tui::scrolling::TranscriptLineMeta; use codex_core::AuthManager; use codex_core::CodexAuth; use codex_core::ConversationManager; @@ -1913,6 +1946,7 @@ mod tests { active_profile: None, file_search, transcript_cells: Vec::new(), + transcript_view_cache: TranscriptViewCache::new(), transcript_scroll: TranscriptScroll::default(), transcript_selection: TranscriptSelection::default(), transcript_multi_click: TranscriptMultiClick::default(), @@ -1921,6 +1955,8 @@ mod tests { transcript_copy_ui: TranscriptCopyUi::new_with_shortcut( CopySelectionShortcut::CtrlShiftC, ), + transcript_copy_action: TranscriptCopyAction::default(), + transcript_scrollbar_ui: TranscriptScrollbarUi::default(), overlay: None, deferred_history_lines: Vec::new(), has_emitted_history_lines: false, @@ -1963,6 +1999,7 @@ mod tests { active_profile: None, file_search, transcript_cells: Vec::new(), + transcript_view_cache: TranscriptViewCache::new(), transcript_scroll: TranscriptScroll::default(), transcript_selection: TranscriptSelection::default(), transcript_multi_click: TranscriptMultiClick::default(), @@ -1971,6 +2008,8 @@ mod tests { transcript_copy_ui: TranscriptCopyUi::new_with_shortcut( CopySelectionShortcut::CtrlShiftC, ), + transcript_copy_action: TranscriptCopyAction::default(), + transcript_scrollbar_ui: TranscriptScrollbarUi::default(), overlay: None, deferred_history_lines: Vec::new(), has_emitted_history_lines: false, diff --git a/codex-rs/tui2/src/bottom_pane/chat_composer.rs b/codex-rs/tui2/src/bottom_pane/chat_composer.rs index f3472b2933..0073173fdc 100644 --- a/codex-rs/tui2/src/bottom_pane/chat_composer.rs +++ b/codex-rs/tui2/src/bottom_pane/chat_composer.rs @@ -1,6 +1,7 @@ use crate::key_hint; use crate::key_hint::KeyBinding; use crate::key_hint::has_ctrl_or_alt; +use crate::transcript_copy_action::TranscriptCopyFeedback; use crossterm::event::KeyCode; use crossterm::event::KeyEvent; use crossterm::event::KeyEventKind; @@ -124,6 +125,7 @@ pub(crate) struct ChatComposer { transcript_selection_active: bool, transcript_scroll_position: Option<(usize, usize)>, transcript_copy_selection_key: KeyBinding, + transcript_copy_feedback: Option, skills: Option>, dismissed_skill_popup_token: Option, } @@ -176,6 +178,7 @@ impl ChatComposer { transcript_selection_active: false, transcript_scroll_position: None, transcript_copy_selection_key: key_hint::ctrl_shift(KeyCode::Char('c')), + transcript_copy_feedback: None, skills: None, dismissed_skill_popup_token: None, }; @@ -1545,6 +1548,7 @@ impl ChatComposer { transcript_selection_active: self.transcript_selection_active, transcript_scroll_position: self.transcript_scroll_position, transcript_copy_selection_key: self.transcript_copy_selection_key, + transcript_copy_feedback: self.transcript_copy_feedback, } } @@ -1577,11 +1581,13 @@ impl ChatComposer { selection_active: bool, scroll_position: Option<(usize, usize)>, copy_selection_key: KeyBinding, + copy_feedback: Option, ) -> bool { if self.transcript_scrolled == scrolled && self.transcript_selection_active == selection_active && self.transcript_scroll_position == scroll_position && self.transcript_copy_selection_key == copy_selection_key + && self.transcript_copy_feedback == copy_feedback { return false; } @@ -1590,6 +1596,7 @@ impl ChatComposer { self.transcript_selection_active = selection_active; self.transcript_scroll_position = scroll_position; self.transcript_copy_selection_key = copy_selection_key; + self.transcript_copy_feedback = copy_feedback; true } diff --git a/codex-rs/tui2/src/bottom_pane/footer.rs b/codex-rs/tui2/src/bottom_pane/footer.rs index 57bffd5639..f4ead67be6 100644 --- a/codex-rs/tui2/src/bottom_pane/footer.rs +++ b/codex-rs/tui2/src/bottom_pane/footer.rs @@ -4,6 +4,7 @@ use crate::key_hint; use crate::key_hint::KeyBinding; use crate::render::line_utils::prefix_lines; use crate::status::format_tokens_compact; +use crate::transcript_copy_action::TranscriptCopyFeedback; use crate::ui_consts::FOOTER_INDENT_COLS; use crossterm::event::KeyCode; use ratatui::buffer::Buffer; @@ -26,6 +27,7 @@ pub(crate) struct FooterProps { pub(crate) transcript_selection_active: bool, pub(crate) transcript_scroll_position: Option<(usize, usize)>, pub(crate) transcript_copy_selection_key: KeyBinding, + pub(crate) transcript_copy_feedback: Option, } #[derive(Clone, Copy, Debug, Eq, PartialEq)] @@ -80,11 +82,26 @@ pub(crate) fn render_footer(area: Rect, buf: &mut Buffer, props: FooterProps) { } fn footer_lines(props: FooterProps) -> Vec> { + fn apply_copy_feedback(lines: &mut [Line<'static>], feedback: Option) { + let Some(line) = lines.first_mut() else { + return; + }; + let Some(feedback) = feedback else { + return; + }; + + line.push_span(" · ".dim()); + match feedback { + TranscriptCopyFeedback::Copied => line.push_span("Copied".green().bold()), + TranscriptCopyFeedback::Failed => line.push_span("Copy failed".red().bold()), + } + } + // Show the context indicator on the left, appended after the primary hint // (e.g., "? for shortcuts"). Keep it visible even when typing (i.e., when // the shortcut hint is hidden). Hide it only for the multi-line // ShortcutOverlay. - match props.mode { + let mut lines = match props.mode { FooterMode::CtrlCReminder => vec![ctrl_c_reminder_line(CtrlCReminderState { is_task_running: props.is_task_running, })], @@ -139,7 +156,9 @@ fn footer_lines(props: FooterProps) -> Vec> { props.context_window_percent, props.context_window_used_tokens, )], - } + }; + apply_copy_feedback(&mut lines, props.transcript_copy_feedback); + lines } #[derive(Clone, Copy, Debug)] @@ -469,6 +488,7 @@ mod tests { transcript_selection_active: false, transcript_scroll_position: None, transcript_copy_selection_key: key_hint::ctrl_shift(KeyCode::Char('c')), + transcript_copy_feedback: None, }, ); @@ -485,6 +505,7 @@ mod tests { transcript_selection_active: true, transcript_scroll_position: Some((3, 42)), transcript_copy_selection_key: key_hint::ctrl_shift(KeyCode::Char('c')), + transcript_copy_feedback: None, }, ); @@ -501,6 +522,7 @@ mod tests { transcript_selection_active: false, transcript_scroll_position: None, transcript_copy_selection_key: key_hint::ctrl_shift(KeyCode::Char('c')), + transcript_copy_feedback: None, }, ); @@ -517,6 +539,7 @@ mod tests { transcript_selection_active: false, transcript_scroll_position: None, transcript_copy_selection_key: key_hint::ctrl_shift(KeyCode::Char('c')), + transcript_copy_feedback: None, }, ); @@ -533,6 +556,7 @@ mod tests { transcript_selection_active: false, transcript_scroll_position: None, transcript_copy_selection_key: key_hint::ctrl_shift(KeyCode::Char('c')), + transcript_copy_feedback: None, }, ); @@ -549,6 +573,7 @@ mod tests { transcript_selection_active: false, transcript_scroll_position: None, transcript_copy_selection_key: key_hint::ctrl_shift(KeyCode::Char('c')), + transcript_copy_feedback: None, }, ); @@ -565,6 +590,7 @@ mod tests { transcript_selection_active: false, transcript_scroll_position: None, transcript_copy_selection_key: key_hint::ctrl_shift(KeyCode::Char('c')), + transcript_copy_feedback: None, }, ); @@ -581,6 +607,7 @@ mod tests { transcript_selection_active: false, transcript_scroll_position: None, transcript_copy_selection_key: key_hint::ctrl_shift(KeyCode::Char('c')), + transcript_copy_feedback: None, }, ); @@ -597,6 +624,24 @@ mod tests { transcript_selection_active: false, transcript_scroll_position: None, transcript_copy_selection_key: key_hint::ctrl_shift(KeyCode::Char('c')), + transcript_copy_feedback: None, + }, + ); + + snapshot_footer( + "footer_copy_feedback_copied", + FooterProps { + mode: FooterMode::ShortcutSummary, + esc_backtrack_hint: false, + use_shift_enter_hint: false, + is_task_running: false, + context_window_percent: None, + context_window_used_tokens: None, + transcript_scrolled: false, + transcript_selection_active: false, + transcript_scroll_position: None, + transcript_copy_selection_key: key_hint::ctrl_shift(KeyCode::Char('c')), + transcript_copy_feedback: Some(TranscriptCopyFeedback::Copied), }, ); } diff --git a/codex-rs/tui2/src/bottom_pane/mod.rs b/codex-rs/tui2/src/bottom_pane/mod.rs index 466da8cda8..2ebd0715e7 100644 --- a/codex-rs/tui2/src/bottom_pane/mod.rs +++ b/codex-rs/tui2/src/bottom_pane/mod.rs @@ -388,12 +388,14 @@ impl BottomPane { selection_active: bool, scroll_position: Option<(usize, usize)>, copy_selection_key: crate::key_hint::KeyBinding, + copy_feedback: Option, ) { let updated = self.composer.set_transcript_ui_state( scrolled, selection_active, scroll_position, copy_selection_key, + copy_feedback, ); if updated { self.request_redraw(); diff --git a/codex-rs/tui2/src/bottom_pane/snapshots/codex_tui2__bottom_pane__footer__tests__footer_copy_feedback_copied.snap b/codex-rs/tui2/src/bottom_pane/snapshots/codex_tui2__bottom_pane__footer__tests__footer_copy_feedback_copied.snap new file mode 100644 index 0000000000..77a9306adc --- /dev/null +++ b/codex-rs/tui2/src/bottom_pane/snapshots/codex_tui2__bottom_pane__footer__tests__footer_copy_feedback_copied.snap @@ -0,0 +1,6 @@ +--- +source: tui2/src/bottom_pane/footer.rs +assertion_line: 473 +expression: terminal.backend() +--- +" 100% context left · ? for shortcuts · Copied " diff --git a/codex-rs/tui2/src/chatwidget.rs b/codex-rs/tui2/src/chatwidget.rs index 490e280753..ca2652cf88 100644 --- a/codex-rs/tui2/src/chatwidget.rs +++ b/codex-rs/tui2/src/chatwidget.rs @@ -1792,7 +1792,10 @@ impl ChatWidget { } self.codex_op_tx - .send(Op::UserInput { items }) + .send(Op::UserInput { + items, + final_output_json_schema: None, + }) .unwrap_or_else(|e| { tracing::error!("failed to send message: {e}"); }); @@ -3138,12 +3141,14 @@ impl ChatWidget { selection_active: bool, scroll_position: Option<(usize, usize)>, copy_selection_key: crate::key_hint::KeyBinding, + copy_feedback: Option, ) { self.bottom_pane.set_transcript_ui_state( scrolled, selection_active, scroll_position, copy_selection_key, + copy_feedback, ); } diff --git a/codex-rs/tui2/src/lib.rs b/codex-rs/tui2/src/lib.rs index 4583a5d84a..8cd3fde133 100644 --- a/codex-rs/tui2/src/lib.rs +++ b/codex-rs/tui2/src/lib.rs @@ -29,7 +29,6 @@ use std::path::PathBuf; use tracing::error; use tracing_appender::non_blocking; use tracing_subscriber::EnvFilter; -use tracing_subscriber::filter::Targets; use tracing_subscriber::prelude::*; mod additional_dirs; @@ -78,10 +77,14 @@ mod terminal_palette; mod text_formatting; mod tooltips; mod transcript_copy; +mod transcript_copy_action; mod transcript_copy_ui; mod transcript_multi_click; mod transcript_render; +mod transcript_scrollbar; +mod transcript_scrollbar_ui; mod transcript_selection; +mod transcript_view_cache; mod tui; mod ui_consts; pub mod update_action; @@ -292,13 +295,8 @@ pub async fn run_main( .with_filter(env_filter()); let feedback = codex_feedback::CodexFeedback::new(); - let targets = Targets::new().with_default(tracing::Level::TRACE); - - let feedback_layer = tracing_subscriber::fmt::layer() - .with_writer(feedback.make_writer()) - .with_ansi(false) - .with_target(false) - .with_filter(targets); + let feedback_layer = feedback.logger_layer(); + let feedback_metadata_layer = feedback.metadata_layer(); if cli.oss && model_provider_override.is_some() { // We're in the oss section, so provider_id should be Some @@ -333,6 +331,7 @@ pub async fn run_main( let _ = tracing_subscriber::registry() .with(file_layer) .with(feedback_layer) + .with(feedback_metadata_layer) .with(otel_tracing_layer) .with(otel_logger_layer) .try_init(); diff --git a/codex-rs/tui2/src/terminal_palette.rs b/codex-rs/tui2/src/terminal_palette.rs index 5c6f32cd9e..941cf78a2b 100644 --- a/codex-rs/tui2/src/terminal_palette.rs +++ b/codex-rs/tui2/src/terminal_palette.rs @@ -1,5 +1,13 @@ use crate::color::perceptual_distance; use ratatui::style::Color; +use std::sync::atomic::AtomicU64; +use std::sync::atomic::Ordering; + +static DEFAULT_PALETTE_VERSION: AtomicU64 = AtomicU64::new(0); + +fn bump_palette_version() { + DEFAULT_PALETTE_VERSION.fetch_add(1, Ordering::Relaxed); +} /// Returns the closest color to the target color that the terminal can display. pub fn best_color(target: (u8, u8, u8)) -> Color { @@ -27,6 +35,7 @@ pub fn best_color(target: (u8, u8, u8)) -> Color { pub fn requery_default_colors() { imp::requery_default_colors(); + bump_palette_version(); } #[derive(Clone, Copy)] @@ -47,6 +56,10 @@ pub fn default_bg() -> Option<(u8, u8, u8)> { default_colors().map(|c| c.bg) } +pub fn palette_version() -> u64 { + DEFAULT_PALETTE_VERSION.load(Ordering::Relaxed) +} + #[cfg(all(unix, not(test)))] mod imp { use super::DefaultColors; diff --git a/codex-rs/tui2/src/transcript_copy_action.rs b/codex-rs/tui2/src/transcript_copy_action.rs new file mode 100644 index 0000000000..49a894d335 --- /dev/null +++ b/codex-rs/tui2/src/transcript_copy_action.rs @@ -0,0 +1,228 @@ +//! Performs "copy selection" and manages transient UI feedback. +//! +//! `transcript_copy` is intentionally pure: it reconstructs clipboard text from a +//! [`TranscriptSelection`], preserving wrapping, indentation, and Markdown markers. +//! +//! This module is the side-effecting layer on top of that pure logic: +//! - writes the reconstructed text to the system clipboard +//! - stores short-lived state so the footer can show `"Copied"` / `"Copy failed"` +//! - schedules redraws so feedback appears promptly and then clears itself +//! +//! Keeping these responsibilities separate reduces cognitive load: +//! - `transcript_copy` answers *what text should be copied?* +//! - `transcript_copy_action` answers *do the copy and tell the user it happened* + +use std::sync::Arc; +use std::time::Duration; +use std::time::Instant; + +use crate::history_cell::HistoryCell; +use crate::transcript_scrollbar::split_transcript_area; +use crate::transcript_selection::TranscriptSelection; +use crate::tui; +use ratatui::layout::Rect; + +/// User-visible feedback shown briefly after a copy attempt. +/// +/// The footer renders this value when present, and it expires automatically. +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +pub(crate) enum TranscriptCopyFeedback { + /// Copy succeeded and the clipboard was updated. + Copied, + /// Copy failed (typically due to OS clipboard integration issues). + Failed, +} + +/// The outcome of attempting to copy the current selection. +/// +/// This is a compact signal for UI code: +/// - `NoSelection` means the action is a no-op (nothing to dismiss). +/// - `Copied`/`Failed` mean the action was triggered and the selection should be dismissed. +#[derive(Debug, Clone, Copy, Eq, PartialEq)] +pub(crate) enum CopySelectionOutcome { + /// No active selection exists (or the terminal is too small to compute one). + NoSelection, + /// Clipboard write succeeded. + Copied, + /// Clipboard write failed. + Failed, +} + +const TRANSCRIPT_COPY_FEEDBACK_DURATION: Duration = Duration::from_millis(1500); + +#[derive(Debug, Clone, Copy)] +struct TranscriptCopyFeedbackState { + kind: TranscriptCopyFeedback, + expires_at: Instant, +} + +/// Performs the copy action and tracks transient footer feedback. +/// +/// `App` owns one instance and calls [`Self::copy_and_handle`] when the user triggers "copy +/// selection" (either via the on-screen copy pill or the keyboard shortcut). +#[derive(Debug, Default)] +pub(crate) struct TranscriptCopyAction { + feedback: Option, +} + +impl TranscriptCopyAction { + /// Attempt to copy the current selection and record feedback. + /// + /// Returns `true` when a copy attempt was made (success or failure). Callers should treat that + /// as a signal to dismiss the selection highlight. + pub(crate) fn copy_and_handle( + &mut self, + tui: &mut tui::Tui, + chat_height: u16, + transcript_cells: &[Arc], + transcript_selection: TranscriptSelection, + ) -> bool { + let outcome = + copy_transcript_selection(tui, chat_height, transcript_cells, transcript_selection); + self.handle_copy_outcome(tui, outcome) + } + + /// Return footer feedback to render for the current frame, if any. + /// + /// This is called from `App`'s render loop. It clears expired feedback lazily so callers do + /// not need separate timer plumbing. + pub(crate) fn footer_feedback(&mut self) -> Option { + let state = self.feedback?; + + if Instant::now() >= state.expires_at { + self.feedback = None; + return None; + } + + Some(state.kind) + } + + /// Record the outcome of a copy attempt and schedule redraws. + /// + /// Returns `true` when a copy attempt happened (success or failure). This is the signal to + /// dismiss the selection highlight. + pub(crate) fn handle_copy_outcome( + &mut self, + tui: &mut tui::Tui, + outcome: CopySelectionOutcome, + ) -> bool { + match outcome { + CopySelectionOutcome::NoSelection => false, + CopySelectionOutcome::Copied => { + self.set_feedback(tui, TranscriptCopyFeedback::Copied); + true + } + CopySelectionOutcome::Failed => { + self.set_feedback(tui, TranscriptCopyFeedback::Failed); + true + } + } + } + + /// Store feedback state and schedule a redraw for its appearance + expiration. + fn set_feedback(&mut self, tui: &mut tui::Tui, kind: TranscriptCopyFeedback) { + let expires_at = Instant::now() + .checked_add(TRANSCRIPT_COPY_FEEDBACK_DURATION) + .unwrap_or_else(Instant::now); + self.feedback = Some(TranscriptCopyFeedbackState { kind, expires_at }); + + tui.frame_requester().schedule_frame(); + tui.frame_requester() + .schedule_frame_in(TRANSCRIPT_COPY_FEEDBACK_DURATION); + } +} + +/// Copy the current transcript selection to the system clipboard. +/// +/// This function ties together layout validation, selection-to-text reconstruction via +/// `transcript_copy`, and the actual clipboard write. +pub(crate) fn copy_transcript_selection( + tui: &tui::Tui, + chat_height: u16, + transcript_cells: &[Arc], + transcript_selection: TranscriptSelection, +) -> CopySelectionOutcome { + // This function is intentionally "dumb plumbing": + // - validate layout prerequisites + // - reconstruct clipboard text (`transcript_copy`) + // - write to clipboard + // + // UI state management (feedback + redraw scheduling) lives in `TranscriptCopyAction`. + let size = tui.terminal.last_known_screen_size; + let width = size.width; + let height = size.height; + if width == 0 || height == 0 { + return CopySelectionOutcome::NoSelection; + } + + if chat_height >= height { + return CopySelectionOutcome::NoSelection; + } + + let transcript_height = height.saturating_sub(chat_height); + if transcript_height == 0 { + return CopySelectionOutcome::NoSelection; + } + + let transcript_full_area = Rect { + x: 0, + y: 0, + width, + height: transcript_height, + }; + let (transcript_area, _) = split_transcript_area(transcript_full_area); + + let Some(text) = crate::transcript_copy::selection_to_copy_text_for_cells( + transcript_cells, + transcript_selection, + transcript_area.width, + ) else { + return CopySelectionOutcome::NoSelection; + }; + + if let Err(err) = crate::clipboard_copy::copy_text(text) { + tracing::error!(error = %err, "failed to copy selection to clipboard"); + return CopySelectionOutcome::Failed; + } + + CopySelectionOutcome::Copied +} + +#[cfg(test)] +mod tests { + use super::*; + use pretty_assertions::assert_eq; + + #[test] + fn footer_feedback_returns_value_before_expiration() { + let mut action = TranscriptCopyAction { + feedback: Some(TranscriptCopyFeedbackState { + kind: TranscriptCopyFeedback::Copied, + expires_at: Instant::now() + Duration::from_secs(10), + }), + }; + + assert_eq!( + action.footer_feedback(), + Some(TranscriptCopyFeedback::Copied) + ); + assert_eq!( + action.footer_feedback(), + Some(TranscriptCopyFeedback::Copied) + ); + } + + #[test] + fn footer_feedback_clears_after_expiration() { + let mut action = TranscriptCopyAction { + feedback: Some(TranscriptCopyFeedbackState { + kind: TranscriptCopyFeedback::Copied, + expires_at: Instant::now() - Duration::from_secs(1), + }), + }; + + assert_eq!(action.footer_feedback(), None); + assert!(action.feedback.is_none()); + assert_eq!(action.footer_feedback(), None); + } +} diff --git a/codex-rs/tui2/src/transcript_copy_ui.rs b/codex-rs/tui2/src/transcript_copy_ui.rs index 6c852c2341..6ec76a1de9 100644 --- a/codex-rs/tui2/src/transcript_copy_ui.rs +++ b/codex-rs/tui2/src/transcript_copy_ui.rs @@ -249,8 +249,16 @@ impl TranscriptCopyUi { let Some((y, to_x)) = last_visible_segment else { return; }; - // Place the pill on the row below the last visible selection segment. - let Some(y) = y.checked_add(1).filter(|y| *y < area.bottom()) else { + // Prefer placing the pill on the row below the last visible selection segment. If the + // selection ends on the last visible row, fall back to placing it above (or, if the view + // is only one row tall, on the same row). + let Some(y) = y + .checked_add(1) + .filter(|y| *y < area.bottom()) + .or_else(|| y.checked_sub(1).filter(|y| *y >= area.y)) + .or(Some(y)) + .filter(|y| *y < area.bottom()) + else { return; }; @@ -275,8 +283,8 @@ impl TranscriptCopyUi { }; let pill_area = Rect::new(x, y, pill_width, 1); - let base_style = Style::new().bg(Color::DarkGray); - let icon_style = base_style.fg(Color::Cyan); + let base_style = Style::new().bg(Color::DarkGray).fg(Color::White); + let icon_style = base_style.add_modifier(Modifier::BOLD).fg(Color::LightCyan); let bold_style = base_style.add_modifier(Modifier::BOLD); let mut spans: Vec> = vec![ @@ -329,4 +337,26 @@ mod tests { assert!(!rendered.contains("ctrl + shift + c")); assert!(ui.affordance_rect.is_some()); } + + #[test] + fn pill_renders_when_selection_on_last_row() { + let area = Rect::new(0, 0, 60, 3); + let mut buf = Buffer::empty(area); + for y in 0..area.height { + for x in 2..area.width.saturating_sub(1) { + buf[(x, y)].set_symbol("X"); + } + } + + let mut ui = TranscriptCopyUi::new_with_shortcut(CopySelectionShortcut::CtrlShiftC); + ui.render_copy_pill(area, &mut buf, (2, 2), (2, 6), 0, 3); + + let rendered = buf_to_string(&buf, area); + assert!(rendered.contains("copy")); + assert!(rendered.contains("ctrl + shift + c")); + + let rect = ui.affordance_rect.expect("expected pill to render"); + assert_eq!(rect.y, 1); + assert!(ui.hit_test(rect.x, rect.y)); + } } diff --git a/codex-rs/tui2/src/transcript_render.rs b/codex-rs/tui2/src/transcript_render.rs index ceee03d897..729ef54bc3 100644 --- a/codex-rs/tui2/src/transcript_render.rs +++ b/codex-rs/tui2/src/transcript_render.rs @@ -113,9 +113,6 @@ pub(crate) fn build_wrapped_transcript_lines( cells: &[Arc], width: u16, ) -> TranscriptLines { - use crate::render::line_utils::line_to_static; - use ratatui::style::Color; - if width == 0 { return TranscriptLines { lines: Vec::new(), @@ -124,110 +121,140 @@ pub(crate) fn build_wrapped_transcript_lines( }; } + let mut transcript = TranscriptLines { + lines: Vec::new(), + meta: Vec::new(), + joiner_before: Vec::new(), + }; + let mut has_emitted_lines = false; let base_opts: crate::wrapping::RtOptions<'_> = crate::wrapping::RtOptions::new(width.max(1) as usize); - let mut lines: Vec> = Vec::new(); - let mut meta: Vec = Vec::new(); - let mut joiner_before: Vec> = Vec::new(); - let mut has_emitted_lines = false; - for (cell_index, cell) in cells.iter().enumerate() { - // Start from each cell's transcript view (prefixes/indents already applied), then apply - // viewport wrapping to prose while keeping preformatted content intact. - let rendered = cell.transcript_lines_with_joiners(width); - if rendered.lines.is_empty() { + append_wrapped_transcript_cell( + &mut transcript, + &mut has_emitted_lines, + cell_index, + cell, + width, + &base_opts, + ); + } + + transcript +} + +/// Append a single history cell to an existing wrapped transcript. +/// +/// This is the incremental building block used by transcript caching: it applies the same +/// flattening and viewport-wrapping rules as [`build_wrapped_transcript_lines`], but for one cell +/// at a time. +/// +/// `has_emitted_lines` tracks whether the output already contains any non-spacer lines and is used +/// to decide when to insert an inter-cell spacer row. +pub(crate) fn append_wrapped_transcript_cell( + out: &mut TranscriptLines, + has_emitted_lines: &mut bool, + cell_index: usize, + cell: &Arc, + width: u16, + base_opts: &crate::wrapping::RtOptions<'_>, +) { + use crate::render::line_utils::line_to_static; + use ratatui::style::Color; + + if width == 0 { + return; + } + + // Start from each cell's transcript view (prefixes/indents already applied), then apply + // viewport wrapping to prose while keeping preformatted content intact. + let rendered = cell.transcript_lines_with_joiners(width); + if rendered.lines.is_empty() { + return; + } + + if !cell.is_stream_continuation() { + if *has_emitted_lines { + out.lines.push(Line::from("")); + out.meta.push(TranscriptLineMeta::Spacer); + out.joiner_before.push(None); + } else { + *has_emitted_lines = true; + } + } + + // `visual_line_in_cell` counts the output visual lines produced from this cell *after* any + // viewport wrapping. This is distinct from `base_idx` (the index into the cell's input + // lines), since a single input line may wrap into multiple visual lines. + let mut visual_line_in_cell: usize = 0; + let mut first = true; + for (base_idx, base_line) in rendered.lines.iter().enumerate() { + // Preserve code blocks (and other preformatted text) by not applying + // viewport wrapping, so indentation remains meaningful for copy/paste. + if base_line.style.fg == Some(Color::Cyan) { + out.lines.push(base_line.clone()); + out.meta.push(TranscriptLineMeta::CellLine { + cell_index, + line_in_cell: visual_line_in_cell, + }); + visual_line_in_cell = visual_line_in_cell.saturating_add(1); + // Preformatted lines are treated as hard breaks; we keep the cell-provided joiner + // (which is typically `None`). + out.joiner_before.push( + rendered + .joiner_before + .get(base_idx) + .cloned() + .unwrap_or(None), + ); + first = false; continue; } - if !cell.is_stream_continuation() { - if has_emitted_lines { - lines.push(Line::from("")); - meta.push(TranscriptLineMeta::Spacer); - joiner_before.push(None); - } else { - has_emitted_lines = true; - } - } + let opts = if first { + base_opts.clone() + } else { + // For subsequent input lines within a cell, treat the "initial" indent as the cell's + // subsequent indent (matches textarea wrapping expectations). + base_opts + .clone() + .initial_indent(base_opts.subsequent_indent.clone()) + }; + // `word_wrap_line_with_joiners` returns both the wrapped visual lines and, for each + // continuation segment, the exact joiner substring that should be inserted instead of a + // newline when copying as a logical line. + let (wrapped, wrapped_joiners) = + crate::wrapping::word_wrap_line_with_joiners(base_line, opts); - // `visual_line_in_cell` counts the output visual lines produced from this cell *after* any - // viewport wrapping. This is distinct from `base_idx` (the index into the cell's input - // lines), since a single input line may wrap into multiple visual lines. - let mut visual_line_in_cell: usize = 0; - let mut first = true; - for (base_idx, base_line) in rendered.lines.iter().enumerate() { - // Preserve code blocks (and other preformatted text) by not applying - // viewport wrapping, so indentation remains meaningful for copy/paste. - if base_line.style.fg == Some(Color::Cyan) { - lines.push(base_line.clone()); - meta.push(TranscriptLineMeta::CellLine { - cell_index, - line_in_cell: visual_line_in_cell, - }); - visual_line_in_cell = visual_line_in_cell.saturating_add(1); - // Preformatted lines are treated as hard breaks; we keep the cell-provided joiner - // (which is typically `None`). - joiner_before.push( + for (seg_idx, (wrapped_line, seg_joiner)) in + wrapped.into_iter().zip(wrapped_joiners).enumerate() + { + out.lines.push(line_to_static(&wrapped_line)); + out.meta.push(TranscriptLineMeta::CellLine { + cell_index, + line_in_cell: visual_line_in_cell, + }); + visual_line_in_cell = visual_line_in_cell.saturating_add(1); + + if seg_idx == 0 { + // The first wrapped segment corresponds to the original input line, so we use the + // cell-provided joiner (hard break vs soft break *between input lines*). + out.joiner_before.push( rendered .joiner_before .get(base_idx) .cloned() .unwrap_or(None), ); - first = false; - continue; - } - - let opts = if first { - base_opts.clone() } else { - // For subsequent input lines within a cell, treat the "initial" indent as the - // cell's subsequent indent (matches textarea wrapping expectations). - base_opts - .clone() - .initial_indent(base_opts.subsequent_indent.clone()) - }; - // `word_wrap_line_with_joiners` returns both the wrapped visual lines and, for each - // continuation segment, the exact joiner substring that should be inserted instead of a - // newline when copying as a logical line. - let (wrapped, wrapped_joiners) = - crate::wrapping::word_wrap_line_with_joiners(base_line, opts); - - for (seg_idx, (wrapped_line, seg_joiner)) in - wrapped.into_iter().zip(wrapped_joiners).enumerate() - { - lines.push(line_to_static(&wrapped_line)); - meta.push(TranscriptLineMeta::CellLine { - cell_index, - line_in_cell: visual_line_in_cell, - }); - visual_line_in_cell = visual_line_in_cell.saturating_add(1); - - if seg_idx == 0 { - // The first wrapped segment corresponds to the original input line, so we use - // the cell-provided joiner (hard break vs soft break *between input lines*). - joiner_before.push( - rendered - .joiner_before - .get(base_idx) - .cloned() - .unwrap_or(None), - ); - } else { - // Subsequent wrapped segments are soft-wrap continuations produced by viewport - // wrapping, so we use the wrap-derived joiner. - joiner_before.push(seg_joiner); - } + // Subsequent wrapped segments are soft-wrap continuations produced by viewport + // wrapping, so we use the wrap-derived joiner. + out.joiner_before.push(seg_joiner); } - - first = false; } - } - TranscriptLines { - lines, - meta, - joiner_before, + first = false; } } @@ -396,4 +423,56 @@ mod tests { ] ); } + + #[test] + fn append_wrapped_transcript_cell_matches_full_build() { + use ratatui::style::Color; + use ratatui::style::Style; + + let cells: Vec> = vec![ + Arc::new(FakeCell { + lines: vec![Line::from("• hello world")], + joiner_before: vec![None], + is_stream_continuation: false, + }), + // A preformatted line should not be viewport-wrapped. + Arc::new(FakeCell { + lines: vec![Line::from("• 1234567890").style(Style::default().fg(Color::Cyan))], + joiner_before: vec![None], + is_stream_continuation: false, + }), + // A stream continuation should not get an inter-cell spacer row. + Arc::new(FakeCell { + lines: vec![Line::from("• wrap me please")], + joiner_before: vec![None], + is_stream_continuation: true, + }), + ]; + + let width = 7; + let full = build_wrapped_transcript_lines(&cells, width); + + let mut out = TranscriptLines { + lines: Vec::new(), + meta: Vec::new(), + joiner_before: Vec::new(), + }; + let mut has_emitted_lines = false; + let base_opts: crate::wrapping::RtOptions<'_> = + crate::wrapping::RtOptions::new(width.max(1) as usize); + for (cell_index, cell) in cells.iter().enumerate() { + append_wrapped_transcript_cell( + &mut out, + &mut has_emitted_lines, + cell_index, + cell, + width, + &base_opts, + ); + } + + assert_eq!(out.lines, full.lines); + assert_eq!(out.meta, full.meta); + assert_eq!(out.joiner_before, full.joiner_before); + } } diff --git a/codex-rs/tui2/src/transcript_scrollbar.rs b/codex-rs/tui2/src/transcript_scrollbar.rs new file mode 100644 index 0000000000..d58d758d17 --- /dev/null +++ b/codex-rs/tui2/src/transcript_scrollbar.rs @@ -0,0 +1,535 @@ +//! Transcript scrollbar rendering. +//! +//! The transcript in `codex-tui2` is rendered as a flattened list of wrapped visual lines. The +//! viewport is tracked as a top-row offset (`transcript_view_top`) into that flattened list (see +//! `tui/scrolling.rs` and `tui_viewport_and_history.md`). +//! +//! This module adds a scrollbar to that viewport using the `tui-scrollbar` widget, but does so in +//! a way that keeps the transcript hot path simple and avoids visual layout jank. +//! +//! # Layout and invariants +//! +//! The transcript area is split into: +//! +//! - `content_area`: where transcript text is rendered +//! - `scrollbar_area`: a 1-column region used to render the scrollbar +//! +//! Additionally, we reserve a 1-column *gap* between content and scrollbar. This produces a +//! slightly more stable/intentional look (the scrollbar reads as an affordance, not part of the +//! transcript content) and avoids accidental overlap with selection/copy UI. +//! +//! Important invariant: **any code that computes transcript wrapping, scrolling, selection, or +//! copy must use the same width as on-screen transcript rendering**. In practice that means: +//! +//! - Use [`split_transcript_area`] and pass `content_area.width` into anything that depends on +//! transcript width (wrapping, scroll deltas, selection reconstruction for copy, etc.). +//! - Do not mix `terminal.width` and `content_area.width` for transcript operations; doing so +//! causes off-by-one/off-by-two behaviors where the selection highlights and copied text do not +//! match what the user sees. +//! +//! `App` follows this rule by deriving `content_area.width` anywhere it needs transcript width. +//! +//! # When the scrollbar is shown +//! +//! The scrollbar is only drawn when the transcript is *not* pinned to the bottom: +//! +//! - `offset < max_offset` → draw scrollbar +//! - `offset == max_offset` → keep the column reserved but blank +//! +//! This keeps the UI clean during normal operation (where the viewport follows streaming output), +//! while still providing a clear affordance when the user is actively reading scrollback. +//! +//! # Styling and theme heuristics +//! +//! `tui-scrollbar` 0.2.1 changed defaults (no arrows, space track + dark background). We keep +//! default glyphs/arrows so the widget controls its own shape, but override track/thumb colors so +//! it matches `codex-tui2`’s existing "user prompt block" styling: +//! +//! - The track background is a small blend toward the terminal foreground so it looks like a +//! subtle indent against the terminal background. +//! - The thumb foreground is a stronger blend so it reads as the active element. +//! - In light themes (terminal background is light), the thumb is intentionally a *darker* shade +//! than the track so it reads as an inset element. +//! +//! We derive these colors from the terminal’s default foreground/background (when available via +//! `terminal_palette`). When defaults are unknown (tests / unsupported terminals), we fall back to +//! ANSI colors so the scrollbar remains visible. +//! +//! # Pointer interaction (mouse click/drag) +//! +//! `tui-scrollbar` includes an interaction helper that translates pointer events into +//! `ScrollCommand::SetOffset(...)` updates, including "grab offset" handling so dragging keeps the +//! pointer anchored within the thumb. +//! +//! `codex-tui2` uses that helper rather than reimplementing scrollbar hit testing and drag math. +//! The app owns the actual transcript scroll state (anchors in `tui/scrolling.rs`), so we only use +//! `tui-scrollbar` to decide *which* offset the user requested. `App` then converts that raw +//! `offset` back into a stable [`TranscriptScroll`] anchor. +//! +//! Note: we use `tui-scrollbar`’s backend-agnostic [`ScrollEvent`] types instead of its optional +//! `crossterm` adapter, because the workspace uses a patched `crossterm` and we want to avoid +//! pulling in multiple `crossterm` versions (which would make `MouseEvent` types incompatible). +//! +//! Because the scrollbar is visually hidden while pinned-to-bottom, `App` also keeps a tiny +//! "pointer capture" bool so a drag that reaches the bottom doesn't accidentally turn into a text +//! selection once the scrollbar disappears. +//! +//! # `ratatui` vs `ratatui-core` +//! +//! `codex-tui2` uses the `ratatui` crate, while `tui-scrollbar` is built on `ratatui-core`. +//! Because the buffer and style types are distinct, we render the scrollbar into a small +//! `ratatui-core` scratch buffer and then copy the resulting glyphs into the main `ratatui` +//! buffer with `ratatui` styles. +//! +//! ## Upgrade note: Ratatui 0.30+ +//! +//! Ratatui 0.30 split many core types (including `Buffer`, `Rect`, and `Widget`) into the new +//! `ratatui-core` crate. `codex-tui2` is currently pinned to an older Ratatui, so it still works +//! with `ratatui::buffer::Buffer` / `ratatui::layout::Rect`, while `tui-scrollbar` is already on +//! `ratatui-core`. +//! +//! That mismatch forces two bits of "glue" that should go away once `codex-tui2` upgrades to +//! Ratatui 0.30: +//! +//! - Rendering: `render_transcript_scrollbar_if_active` currently renders into a `ratatui-core` +//! scratch buffer and copies glyphs/styles into the `ratatui` buffer. With Ratatui 0.30, the +//! app’s buffer/rect types should unify with `tui-scrollbar`’s `ratatui-core` types, so we can +//! render directly without copying. +//! - Input: we currently translate `crossterm::MouseEvent` into `tui-scrollbar`’s backend-agnostic +//! `ScrollEvent` types (and intentionally avoid `tui-scrollbar`’s optional `crossterm` adapter) +//! to prevent multiple `crossterm` versions in the dependency graph. Once the Ratatui upgrade +//! is complete, this should be revisited; if the workspace’s `crossterm` resolves to a single +//! version, we can use `tui-scrollbar`’s adapter and reduce more local glue. + +use ratatui::buffer::Buffer; +use ratatui::layout::Rect; +use ratatui::style::Color; +use ratatui::style::Style; +use ratatui_core::buffer::Buffer as CoreBuffer; +use ratatui_core::layout::Rect as CoreRect; +use ratatui_core::widgets::Widget as _; +use tui_scrollbar::PointerButton; +use tui_scrollbar::PointerEvent; +use tui_scrollbar::PointerEventKind; +use tui_scrollbar::ScrollBar; +use tui_scrollbar::ScrollBarInteraction; +use tui_scrollbar::ScrollCommand; +use tui_scrollbar::ScrollEvent; +use tui_scrollbar::ScrollLengths; +use tui_scrollbar::TrackClickBehavior; + +/// Number of columns reserved between transcript content and the scrollbar track. +/// +/// This exists purely for visual separation and to avoid selection/copy UI feeling "attached" to +/// the scrollbar. +const TRANSCRIPT_SCROLLBAR_GAP_COLS: u16 = 1; +/// Width of the scrollbar track itself (in terminal cells). +/// +/// `tui-scrollbar` renders a vertical scrollbar into a 1-column area. +const TRANSCRIPT_SCROLLBAR_TRACK_COLS: u16 = 1; +/// Total columns reserved for transcript scrollbar UI (gap + track). +pub(crate) const TRANSCRIPT_SCROLLBAR_COLS: u16 = + TRANSCRIPT_SCROLLBAR_GAP_COLS + TRANSCRIPT_SCROLLBAR_TRACK_COLS; + +/// Split a transcript viewport into content + scrollbar regions. +/// +/// `codex-tui2` reserves space for the transcript scrollbar even when it is not visible so the +/// transcript does not "reflow" when the user scrolls away from the bottom. +/// +/// Layout: +/// - `content_area`: original area minus [`TRANSCRIPT_SCROLLBAR_COLS`] on the right. +/// - `scrollbar_area`: the last column of the original area (1 cell wide). +/// - The remaining column (immediately left of `scrollbar_area`) is the "gap" and is intentionally +/// left unused so the scrollbar reads as a separate affordance. +/// +/// Returns `(area, None)` when the terminal is too narrow to reserve the required columns. +pub(crate) fn split_transcript_area(area: Rect) -> (Rect, Option) { + if area.width <= TRANSCRIPT_SCROLLBAR_COLS { + return (area, None); + } + + let content_width = area.width.saturating_sub(TRANSCRIPT_SCROLLBAR_COLS); + let content_area = Rect { + x: area.x, + y: area.y, + width: content_width, + height: area.height, + }; + let scrollbar_area = Rect { + x: area.right().saturating_sub(1), + y: area.y, + width: TRANSCRIPT_SCROLLBAR_TRACK_COLS, + height: area.height, + }; + + (content_area, Some(scrollbar_area)) +} + +/// Whether the transcript scrollbar should be visible. +/// +/// The scrollbar is treated as "active" when the transcript is scrollable and the viewport is not +/// pinned to the bottom. This is used both for rendering (draw vs. keep blank) and for interaction +/// (whether the scrollbar should be hit-testable). +/// +/// Note that `codex-tui2` still reserves space for the scrollbar even when it is inactive; see +/// [`split_transcript_area`]. +pub(crate) fn is_transcript_scrollbar_active( + total_lines: usize, + viewport_lines: usize, + top_offset: usize, +) -> bool { + if total_lines <= viewport_lines { + return false; + } + + let max_offset = total_lines.saturating_sub(viewport_lines); + top_offset < max_offset +} + +/// Render the transcript scrollbar into `buf` when the viewport is scrolled away from bottom. +/// +/// The scrollbar is hidden (but its column(s) remain reserved) while the viewport follows the +/// latest output. +/// +/// Implementation notes: +/// - We keep `tui-scrollbar`’s default glyph selection and shape logic, but override colors to +/// better match `codex-tui2`’s theme heuristics (see module docs). +/// - Because `tui-scrollbar` renders into a `ratatui-core` buffer while `codex-tui2` uses `ratatui` +/// (pre-0.30), we render into a scratch buffer and then copy the resulting symbols into the main +/// buffer. +pub(crate) fn render_transcript_scrollbar_if_active( + buf: &mut Buffer, + scrollbar_area: Option, + total_lines: usize, + viewport_lines: usize, + top_offset: usize, +) { + let Some(scrollbar_area) = scrollbar_area else { + return; + }; + + if scrollbar_area.width == 0 || scrollbar_area.height == 0 { + return; + } + + if !is_transcript_scrollbar_active(total_lines, viewport_lines, top_offset) { + return; + } + + let lengths = ScrollLengths { + content_len: total_lines, + viewport_len: viewport_lines, + }; + + let scrollbar = ScrollBar::vertical(lengths).offset(top_offset); + + let core_bar_area = CoreRect { + x: scrollbar_area.x, + y: scrollbar_area.y, + width: scrollbar_area.width, + height: scrollbar_area.height, + }; + let mut scratch = CoreBuffer::empty(core_bar_area); + (&scrollbar).render(core_bar_area, &mut scratch); + + let (track_style, thumb_style) = scrollbar_styles(); + for row in 0..scrollbar_area.height { + let x = scrollbar_area.x; + let y = scrollbar_area.y + row; + let src = &scratch[(x, y)]; + let dst = &mut buf[(x, y)]; + let symbol = src.symbol(); + dst.set_symbol(symbol); + if symbol == " " { + dst.set_style(track_style); + } else { + dst.set_style(thumb_style); + } + } +} + +/// Convert a `crossterm` mouse event into a requested transcript offset for the scrollbar. +/// +/// This is a thin wrapper over `tui-scrollbar`’s pointer interaction logic: +/// - It builds a `ScrollBar` configured with the current `top_offset`. +/// - It translates the mouse event into a backend-agnostic [`ScrollEvent`]. +/// - It passes the event through `tui-scrollbar`’s hit testing and drag state (`interaction`). +/// +/// `clamp_to_track` exists for `App`’s "pointer capture" behavior: once the user starts a drag on +/// the scrollbar, we keep treating the gesture as a scrollbar drag even if the pointer moves +/// outside the 1-column track. Without this clamp, the drag could stop producing offsets, and the +/// same mouse gesture could then be interpreted as transcript selection. +/// +/// Returns `None` when: +/// - the scrollbar area is empty, +/// - the transcript does not scroll (`total_lines <= viewport_lines`), +/// - or the event is not a left-button down/drag/up. +pub(crate) fn transcript_scrollbar_offset_for_mouse_event( + scrollbar_area: Rect, + total_lines: usize, + viewport_lines: usize, + top_offset: usize, + mut event: crossterm::event::MouseEvent, + interaction: &mut ScrollBarInteraction, + clamp_to_track: bool, +) -> Option { + if scrollbar_area.width == 0 || scrollbar_area.height == 0 { + return None; + } + + if total_lines <= viewport_lines { + return None; + } + + if clamp_to_track { + let max_x = scrollbar_area.right().saturating_sub(1); + let max_y = scrollbar_area.bottom().saturating_sub(1); + event.column = event.column.clamp(scrollbar_area.x, max_x); + event.row = event.row.clamp(scrollbar_area.y, max_y); + } + + let lengths = ScrollLengths { + content_len: total_lines, + viewport_len: viewport_lines, + }; + let scrollbar = ScrollBar::vertical(lengths) + .offset(top_offset) + .track_click_behavior(TrackClickBehavior::JumpToClick); + + let core_bar_area = CoreRect { + x: scrollbar_area.x, + y: scrollbar_area.y, + width: scrollbar_area.width, + height: scrollbar_area.height, + }; + let scroll_event = match event.kind { + crossterm::event::MouseEventKind::Down(crossterm::event::MouseButton::Left) => { + Some(ScrollEvent::Pointer(PointerEvent { + column: event.column, + row: event.row, + kind: PointerEventKind::Down, + button: PointerButton::Primary, + })) + } + crossterm::event::MouseEventKind::Up(crossterm::event::MouseButton::Left) => { + Some(ScrollEvent::Pointer(PointerEvent { + column: event.column, + row: event.row, + kind: PointerEventKind::Up, + button: PointerButton::Primary, + })) + } + crossterm::event::MouseEventKind::Drag(crossterm::event::MouseButton::Left) => { + Some(ScrollEvent::Pointer(PointerEvent { + column: event.column, + row: event.row, + kind: PointerEventKind::Drag, + button: PointerButton::Primary, + })) + } + _ => None, + }; + scroll_event + .and_then(|scroll_event| scrollbar.handle_event(core_bar_area, scroll_event, interaction)) + .map(|command| match command { + ScrollCommand::SetOffset(offset) => offset, + }) +} + +/// Derive track/thumb styles for the scrollbar from terminal defaults. +/// +/// We prefer using the terminal’s default background/foreground so the scrollbar feels like a +/// native part of the theme (and stays readable across 16-color / 256-color / truecolor +/// backends). +/// +/// When terminal defaults are unavailable (tests / unsupported terminals), we fall back to fixed +/// ANSI colors that are likely to be visible. +fn scrollbar_styles() -> (Style, Style) { + let Some(terminal_bg) = crate::terminal_palette::default_bg() else { + let track_style = Style::new().bg(Color::DarkGray); + let thumb_style = Style::new().fg(Color::Gray).bg(Color::DarkGray); + return (track_style, thumb_style); + }; + + let terminal_fg = crate::terminal_palette::default_fg(); + + let (track_rgb, thumb_rgb) = scrollbar_colors(terminal_bg, terminal_fg); + let track_bg = crate::terminal_palette::best_color(track_rgb); + let thumb_fg = crate::terminal_palette::best_color(thumb_rgb); + + let track_style = Style::new().bg(track_bg); + let thumb_style = Style::new().fg(thumb_fg).bg(track_bg); + (track_style, thumb_style) +} + +/// Compute `(track_bg_rgb, thumb_fg_rgb)` for the transcript scrollbar. +/// +/// The scrollbar is styled to feel consistent with the user prompt background (see +/// `style::user_message_bg`), but is tuned separately so the thumb reads as an inset control: +/// +/// - Dark themes: track is a subtle brightening of the background; thumb is brighter than the track +/// (but not pure white). +/// - Light themes: track is a subtle darkening of the background; thumb is darker than the track. +fn scrollbar_colors( + terminal_bg: (u8, u8, u8), + terminal_fg: Option<(u8, u8, u8)>, +) -> ((u8, u8, u8), (u8, u8, u8)) { + let is_light = crate::color::is_light(terminal_bg); + let fallback_fg = if is_light { (0, 0, 0) } else { (255, 255, 255) }; + let terminal_fg = terminal_fg.unwrap_or(fallback_fg); + + // We want the scrollbar to feel visually related to the user message block background + // (`style::user_message_bg` uses 0.1), but slightly more subtle: + // + // - Light mode: keep both colors closer to the background (alpha < 0.1), with the thumb darker + // than the track. + // - Dark mode: keep the track slightly darker than the prompt block, but make the thumb + // brighter so it's easy to pick out without becoming "white". + let (track_alpha, thumb_alpha) = if is_light { (0.04, 0.08) } else { (0.08, 0.18) }; + + let track_rgb = crate::color::blend(terminal_fg, terminal_bg, track_alpha); + let thumb_rgb = crate::color::blend(terminal_fg, terminal_bg, thumb_alpha); + (track_rgb, thumb_rgb) +} + +#[cfg(test)] +mod tests { + use super::*; + use pretty_assertions::assert_eq; + + fn scrollbar_bg(buf: &Buffer, scrollbar_area: Rect) -> Vec { + use ratatui::style::Color; + + let x = scrollbar_area.x; + (0..scrollbar_area.height) + .map(|row| { + buf[(x, scrollbar_area.y + row)] + .style() + .bg + .unwrap_or(Color::Reset) + }) + .collect() + } + + #[test] + fn does_not_render_when_pinned_to_bottom() { + let full_area = Rect::new(0, 0, 10, 6); + let (_, scrollbar_area) = split_transcript_area(full_area); + let mut buf = Buffer::empty(full_area); + + render_transcript_scrollbar_if_active(&mut buf, scrollbar_area, 100, 6, 94); + + assert_eq!( + scrollbar_bg(&buf, scrollbar_area.expect("scrollbar area")), + vec![ + ratatui::style::Color::Reset, + ratatui::style::Color::Reset, + ratatui::style::Color::Reset, + ratatui::style::Color::Reset, + ratatui::style::Color::Reset, + ratatui::style::Color::Reset + ] + ); + } + + #[test] + fn renders_when_scrolled_away_from_bottom() { + let full_area = Rect::new(0, 0, 10, 6); + let (_, scrollbar_area) = split_transcript_area(full_area); + let mut buf = Buffer::empty(full_area); + + render_transcript_scrollbar_if_active(&mut buf, scrollbar_area, 100, 6, 80); + + assert_eq!( + scrollbar_bg(&buf, scrollbar_area.expect("scrollbar area")), + vec![ + ratatui::style::Color::DarkGray, + ratatui::style::Color::DarkGray, + ratatui::style::Color::DarkGray, + ratatui::style::Color::DarkGray, + ratatui::style::Color::DarkGray, + ratatui::style::Color::DarkGray + ] + ); + } + + #[test] + fn split_leaves_gap_before_scrollbar() { + let full_area = Rect::new(0, 0, 10, 6); + let (content, scrollbar) = split_transcript_area(full_area); + + assert_eq!(content.width, 8); + assert_eq!(scrollbar.expect("scrollbar").x, 9); + } + + #[test] + fn scrollbar_mouse_drag_moves_offset_downward() { + use crossterm::event::KeyModifiers; + use crossterm::event::MouseButton; + use crossterm::event::MouseEvent; + use crossterm::event::MouseEventKind; + + let scrollbar_area = Rect::new(9, 0, 1, 10); + let mut interaction = ScrollBarInteraction::new(); + + let down = MouseEvent { + kind: MouseEventKind::Down(MouseButton::Left), + column: 9, + row: 0, + modifiers: KeyModifiers::empty(), + }; + let mut offset = 0; + if let Some(next) = transcript_scrollbar_offset_for_mouse_event( + scrollbar_area, + 100, + 10, + offset, + down, + &mut interaction, + true, + ) { + offset = next; + } + + let drag = MouseEvent { + kind: MouseEventKind::Drag(MouseButton::Left), + column: 9, + row: 9, + modifiers: KeyModifiers::empty(), + }; + let dragged = transcript_scrollbar_offset_for_mouse_event( + scrollbar_area, + 100, + 10, + offset, + drag, + &mut interaction, + true, + ) + .expect("drag should set an offset"); + + assert!(dragged > offset); + } + + #[test] + fn light_mode_thumb_is_darker_than_track() { + let bg = (255, 255, 255); + let fg = Some((0, 0, 0)); + let (track, thumb) = scrollbar_colors(bg, fg); + + assert!(thumb.0 < track.0); + assert!(thumb.1 < track.1); + assert!(thumb.2 < track.2); + } + + #[test] + fn dark_mode_thumb_is_brighter_than_track() { + let bg = (0, 0, 0); + let fg = Some((255, 255, 255)); + let (track, thumb) = scrollbar_colors(bg, fg); + + assert!(thumb.0 > track.0); + assert!(thumb.1 > track.1); + assert!(thumb.2 > track.2); + } +} diff --git a/codex-rs/tui2/src/transcript_scrollbar_ui.rs b/codex-rs/tui2/src/transcript_scrollbar_ui.rs new file mode 100644 index 0000000000..92f19d1bc5 --- /dev/null +++ b/codex-rs/tui2/src/transcript_scrollbar_ui.rs @@ -0,0 +1,225 @@ +//! Transcript scrollbar mouse interaction. +//! +//! This module handles pointer interaction (click/drag) for the transcript scrollbar rendered by +//! [`crate::transcript_scrollbar`]. It exists to keep `app.rs` from growing further: the transcript +//! is a particularly stateful part of the UI (selection, wrapping, scroll anchoring, copy, etc.), +//! and scrollbar interaction needs to coordinate with several of those subsystems. +//! +//! # Responsibilities +//! +//! - Translate `crossterm` mouse events into `tui-scrollbar` interaction events (backend-agnostic +//! [`tui_scrollbar::ScrollEvent`]). +//! - Maintain `tui-scrollbar`’s drag interaction state (`ScrollBarInteraction`) across frames so +//! the thumb "grab offset" behaves naturally. +//! - Maintain a tiny "pointer capture" flag so a drag that reaches the bottom doesn't fall through +//! into transcript selection once the scrollbar becomes visually hidden (because the view is now +//! pinned to bottom). +//! +//! This module does *not* render anything. Rendering lives in `transcript_scrollbar.rs`. +//! +//! # Interaction model and transcript anchors +//! +//! `tui-scrollbar` reports requested scroll positions as a raw `offset` (a top-row index). The +//! transcript scroll state in `codex-tui2` is represented as a stable anchor +//! ([`crate::tui::scrolling::TranscriptScroll`]) so it survives transcript growth and re-wrapping. +//! +//! The conversion happens here: +//! - Ask `tui-scrollbar` for a `next_offset`. +//! - Convert that concrete offset back into a stable anchor using +//! [`crate::tui::scrolling::TranscriptScroll::anchor_for`]. +//! - If the requested offset is the bottom-most valid position, use `ToBottom` rather than a fixed +//! anchor, restoring auto-follow behavior. +//! +//! This keeps scrollbar interaction consistent with other scroll mechanisms (wheel, PgUp/PgDn), +//! which also operate in terms of the `TranscriptScroll` state machine. +//! +//! # Upgrade note: Ratatui 0.30+ +//! +//! This module intentionally uses `tui-scrollbar`’s backend-agnostic event types instead of its +//! optional `crossterm` adapter. The workspace uses a patched `crossterm`, and enabling the adapter +//! would pull in a second `crossterm` version, making `MouseEvent` types incompatible. +//! +//! Once `codex-tui2` upgrades to Ratatui 0.30 (and the workspace converges on a single `crossterm` +//! version), we should revisit whether we can remove this translation layer. + +use crate::history_cell::HistoryCell; +use crate::transcript_scrollbar::is_transcript_scrollbar_active; +use crate::transcript_scrollbar::transcript_scrollbar_offset_for_mouse_event; +use crate::transcript_view_cache::TranscriptViewCache; +use crate::tui; +use crate::tui::scrolling::MouseScrollState; +use crate::tui::scrolling::TranscriptScroll; +use crossterm::event::MouseButton; +use crossterm::event::MouseEvent; +use crossterm::event::MouseEventKind; +use ratatui::layout::Rect; +use std::sync::Arc; + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(crate) enum TranscriptScrollbarMouseHandling { + /// The event is unrelated to the scrollbar; callers may handle it normally (e.g. selection). + NotHandled, + /// The event was handled by the scrollbar logic and should not be interpreted as selection. + Handled, +} + +/// Persistent UI state for transcript scrollbar pointer interaction. +/// +/// This stores `tui-scrollbar`’s drag state (`ScrollBarInteraction`) plus a small "pointer capture" +/// flag used by `codex-tui2`: +/// +/// - When the user clicks the scrollbar thumb/track, we enter pointer capture. +/// - While capture is active, subsequent drag events are treated as scrollbar drags even if the +/// pointer leaves the 1-column track. +/// - Capture is released on `MouseUp`. +/// +/// The capture flag is important because the transcript scrollbar is hidden while pinned to +/// bottom; without capture, a drag that reaches the bottom could stop producing offsets and fall +/// through into transcript selection mid-gesture. +#[derive(Debug, Default)] +pub(crate) struct TranscriptScrollbarUi { + interaction: tui_scrollbar::ScrollBarInteraction, + pointer_capture: bool, +} + +/// Bundles the arguments needed to handle a transcript scrollbar mouse event. +/// +/// This is intentionally a struct (rather than a long argument list) because scrollbar interaction +/// touches several pieces of transcript state at once: wrapping cache, scroll anchor state, the +/// concrete top-row offset, and the wheel-scroll stream state machine. Grouping them makes call +/// sites easier to scan and helps keep `app.rs` glue minimal. +pub(crate) struct TranscriptScrollbarMouseEvent<'a> { + pub(crate) tui: &'a mut tui::Tui, + pub(crate) mouse_event: MouseEvent, + pub(crate) transcript_area: Rect, + pub(crate) scrollbar_area: Option, + pub(crate) transcript_cells: &'a [Arc], + pub(crate) transcript_view_cache: &'a mut TranscriptViewCache, + pub(crate) transcript_scroll: &'a mut TranscriptScroll, + pub(crate) transcript_view_top: &'a mut usize, + pub(crate) transcript_total_lines: &'a mut usize, + pub(crate) mouse_scroll_state: &'a mut MouseScrollState, +} + +impl TranscriptScrollbarUi { + pub(crate) fn pointer_capture_active(&self) -> bool { + self.pointer_capture + } + + /// Handle click/drag events for the transcript scrollbar. + /// + /// The caller is expected to provide the transcript layout for the current terminal size: + /// `transcript_area` for content and `scrollbar_area` for the 1-column scrollbar track. See + /// [`crate::transcript_scrollbar::split_transcript_area`]. + /// + /// Returns [`TranscriptScrollbarMouseHandling::Handled`] when the event should not be + /// interpreted as transcript selection (either because it updated the scroll position or + /// because an in-progress scrollbar drag is being captured). + pub(crate) fn handle_mouse_event( + &mut self, + event: TranscriptScrollbarMouseEvent<'_>, + ) -> TranscriptScrollbarMouseHandling { + let TranscriptScrollbarMouseEvent { + tui, + mouse_event, + transcript_area, + scrollbar_area, + transcript_cells, + transcript_view_cache, + transcript_scroll, + transcript_view_top, + transcript_total_lines, + mouse_scroll_state, + } = event; + let is_scrollbar_event = matches!( + mouse_event.kind, + MouseEventKind::Down(MouseButton::Left) + | MouseEventKind::Drag(MouseButton::Left) + | MouseEventKind::Up(MouseButton::Left) + ); + if !is_scrollbar_event { + return TranscriptScrollbarMouseHandling::NotHandled; + } + + let Some(scrollbar_area) = scrollbar_area else { + if matches!(mouse_event.kind, MouseEventKind::Up(MouseButton::Left)) { + self.pointer_capture = false; + } + return if self.pointer_capture { + TranscriptScrollbarMouseHandling::Handled + } else { + TranscriptScrollbarMouseHandling::NotHandled + }; + }; + + let is_over_scrollbar = mouse_event.column >= scrollbar_area.x + && mouse_event.column < scrollbar_area.right() + && mouse_event.row >= scrollbar_area.y + && mouse_event.row < scrollbar_area.bottom(); + + if !self.pointer_capture && !is_over_scrollbar { + return TranscriptScrollbarMouseHandling::NotHandled; + } + + let viewport_lines = transcript_area.height as usize; + let scrollbar_is_visible = if viewport_lines > 0 && !transcript_cells.is_empty() { + transcript_view_cache.ensure_wrapped(transcript_cells, transcript_area.width); + let total_lines = transcript_view_cache.lines().len(); + let max_visible = std::cmp::min(total_lines, viewport_lines); + is_transcript_scrollbar_active(total_lines, max_visible, *transcript_view_top) + } else { + false + }; + + // When the transcript is pinned to bottom, we intentionally hide the scrollbar (but still + // reserve its column). In that state, we avoid hit-testing the scrollbar track so the + // reserved column doesn't become an invisible interactive region. Pointer capture remains + // active for an in-progress drag so a gesture that reaches the bottom doesn't fall through + // into transcript selection mid-drag. + if !self.pointer_capture && !scrollbar_is_visible { + return TranscriptScrollbarMouseHandling::NotHandled; + } + + if matches!(mouse_event.kind, MouseEventKind::Down(MouseButton::Left)) && is_over_scrollbar + { + self.pointer_capture = true; + } + + if viewport_lines > 0 && !transcript_cells.is_empty() { + // `ensure_wrapped` was already called above when checking visibility. + let total_lines = transcript_view_cache.lines().len(); + let max_visible = std::cmp::min(total_lines, viewport_lines); + let max_offset = total_lines.saturating_sub(max_visible); + + if let Some(next_offset) = transcript_scrollbar_offset_for_mouse_event( + scrollbar_area, + total_lines, + max_visible, + *transcript_view_top, + mouse_event, + &mut self.interaction, + self.pointer_capture, + ) { + let next_offset = next_offset.min(max_offset); + let line_meta = transcript_view_cache.line_meta(); + + *transcript_scroll = if next_offset >= max_offset { + TranscriptScroll::ToBottom + } else { + TranscriptScroll::anchor_for(line_meta, next_offset) + .unwrap_or(TranscriptScroll::ToBottom) + }; + *transcript_view_top = next_offset.min(max_offset); + *transcript_total_lines = total_lines; + *mouse_scroll_state = MouseScrollState::default(); + tui.frame_requester().schedule_frame(); + } + } + + if matches!(mouse_event.kind, MouseEventKind::Up(MouseButton::Left)) { + self.pointer_capture = false; + } + + TranscriptScrollbarMouseHandling::Handled + } +} diff --git a/codex-rs/tui2/src/transcript_view_cache.rs b/codex-rs/tui2/src/transcript_view_cache.rs new file mode 100644 index 0000000000..a32094b118 --- /dev/null +++ b/codex-rs/tui2/src/transcript_view_cache.rs @@ -0,0 +1,1033 @@ +//! Caches for transcript rendering in `codex-tui2`. +//! +//! The inline transcript view is drawn every frame. Two parts of that draw can +//! be expensive in steady state: +//! +//! - Building the *wrapped transcript* (`HistoryCell` → flattened `Line`s + +//! per-line metadata). This work is needed for rendering and for scroll math. +//! - Rendering each visible `Line` into the frame buffer. Ratatui's rendering +//! path performs grapheme segmentation and width/layout work; repeatedly +//! rerendering the same visible lines can dominate CPU during streaming. +//! +//! This module provides a pair of caches: +//! +//! - [`WrappedTranscriptCache`] memoizes the wrapped transcript for a given +//! terminal width and supports incremental append when new history cells are +//! added. +//! - [`TranscriptRasterCache`] memoizes the *rasterized* representation of +//! individual wrapped lines (a single terminal row of `Cell`s) so redraws can +//! cheaply copy already-rendered cells instead of re-running grapheme +//! segmentation for every frame. +//! +//! Notes: +//! - All caches are invalidated on width changes because wrapping and layout +//! depend on the viewport width. +//! - Rasterization is cached for base transcript content only; selection +//! highlight and copy affordances are applied after the rows are drawn, so +//! they do not pollute the cache. +//! +//! ## Algorithm overview +//! +//! At a high level, transcript rendering is a two-stage pipeline: +//! +//! 1. **Build wrapped transcript lines**: flatten the logical `HistoryCell` list into a single +//! vector of visual [`Line`]s and a parallel `meta` vector (`TranscriptLineMeta`) that maps each +//! visual line back to `(cell_index, line_in_cell)` or `Spacer`. +//! 2. **Render visible lines into the frame buffer**: draw the subset of wrapped lines that are +//! currently visible in the viewport. +//! +//! The cache mirrors that pipeline: +//! +//! - [`WrappedTranscriptCache`] memoizes stage (1) for the current `width` and supports incremental +//! append when new cells are pushed during streaming. +//! - [`TranscriptRasterCache`] memoizes stage (2) per line by caching the final rendered row +//! (`Vec`) for a given `(line_index, is_user_row)` at the current `width`. +//! +//! ### Per draw tick +//! +//! Callers typically do the following during a draw tick: +//! +//! 1. Call [`TranscriptViewCache::ensure_wrapped`] with the current `cells` and viewport `width`. +//! This may append new cells or rebuild from scratch (on width change/truncation/replacement). +//! 2. Use [`TranscriptViewCache::lines`] and [`TranscriptViewCache::line_meta`] for scroll math and +//! to resolve the visible `line_index` range. +//! 3. Configure row caching via [`TranscriptViewCache::set_raster_capacity`] (usually a few +//! viewports worth). +//! 4. For each visible `line_index`, call [`TranscriptViewCache::render_row_index_into`] to draw a +//! single terminal row. +//! +//! ### Rasterization details +//! +//! `render_row_index_into` delegates to `TranscriptRasterCache::render_row_into`: +//! +//! - On a **cache hit**, it copies cached cells into the destination buffer (no grapheme +//! segmentation, no span layout). +//! - On a **cache miss**, it renders the wrapped [`Line`] into a scratch `Buffer` with height 1, +//! copies out the resulting cells, inserts them into the cache, and then copies them into the +//! destination buffer. +//! +//! Cached rows are invalidated when: +//! - the wrapped transcript is rebuilt (line indices shift) +//! - the width changes (layout changes) +//! +//! The raster cache is bounded by `capacity` using an approximate LRU so it does not grow without +//! bound during long sessions. + +use crate::history_cell::HistoryCell; +use crate::history_cell::UserHistoryCell; +use crate::transcript_render::TranscriptLines; +use crate::tui::scrolling::TranscriptLineMeta; +use ratatui::buffer::Buffer; +use ratatui::prelude::Rect; +use ratatui::text::Line; +use ratatui::widgets::WidgetRef; +use std::collections::HashMap; +use std::collections::VecDeque; +use std::sync::Arc; + +/// Top-level cache for the inline transcript viewport. +/// +/// This combines two caches that are used together during a draw tick: +/// +/// - [`WrappedTranscriptCache`] produces the flattened wrapped transcript lines and metadata used +/// for rendering, scrolling, and selection/copy mapping. +/// - [`TranscriptRasterCache`] caches the expensive conversion from a wrapped [`Line`] into a row +/// of terminal [`ratatui::buffer::Cell`]s so repeated redraws can copy cells instead of redoing +/// grapheme segmentation. +/// +/// The caches are intentionally coupled: +/// - width changes invalidate both layers +/// - wrapped transcript rebuilds invalidate the raster cache because line indices shift +pub(crate) struct TranscriptViewCache { + /// Memoized flattened wrapped transcript content for the current width. + wrapped: WrappedTranscriptCache, + /// Per-line row rasterization cache for the current width. + raster: TranscriptRasterCache, +} + +impl TranscriptViewCache { + /// Create an empty transcript view cache. + pub(crate) fn new() -> Self { + Self { + wrapped: WrappedTranscriptCache::new(), + raster: TranscriptRasterCache::new(), + } + } + + /// Ensure the wrapped transcript cache is up to date for `cells` at `width`. + /// + /// This is the shared entrypoint for the transcript renderer and scroll math. It ensures the + /// cache reflects the current transcript and viewport width while preserving scroll/copy + /// invariants (`lines`, `meta`, and `joiner_before` remain aligned). + /// + /// Rebuild conditions: + /// - `width` changes (wrapping/layout is width-dependent) + /// - the transcript is truncated (fewer `cells` than last time), which means the previously + /// cached suffix may refer to cells that no longer exist and the cached `(cell_index, + /// line_in_cell)` mapping is no longer valid. In `tui2` today, this happens when the user + /// backtracks/forks a conversation: `app_backtrack` trims `App::transcript_cells` to preserve + /// only content up to the selected user message. + /// - the transcript is replaced (detected by a change in the first cell pointer), which + /// commonly happens when history is rotated/dropped from the front while keeping a similar + /// length (e.g. to cap history size) or when switching to a different transcript. We don't + /// currently replace the transcript list in the main render loop, but we keep this guard so + /// future history-capping or transcript-reload features can't accidentally treat a shifted + /// list as an append. In that case, treating the new list as an append would misattribute + /// line origins and break scroll anchors and selection/copy mapping. + /// + /// The raster cache is invalidated whenever the wrapped transcript is rebuilt or the width no + /// longer matches. + pub(crate) fn ensure_wrapped(&mut self, cells: &[Arc], width: u16) { + let update = self.wrapped.ensure(cells, width); + if update == WrappedTranscriptUpdate::Rebuilt { + self.raster.width = width; + self.raster.clear(); + } else if width != self.raster.width { + // Keep the invariant that raster cache always matches the active wrapped width. + self.raster.clear(); + self.raster.width = width; + } + } + + /// Return the cached flattened wrapped transcript lines. + /// + /// This is primarily used for: + /// - computing `total_lines` for scroll/viewport logic + /// - any code that needs a read-only view of the current flattened transcript + /// + /// Callers should generally avoid iterating these lines to render them in the draw hot path; + /// use [`Self::render_row_index_into`] so redraws can take advantage of the raster cache. + pub(crate) fn lines(&self) -> &[Line<'static>] { + &self.wrapped.transcript.lines + } + + /// Return per-line origin metadata aligned with [`Self::lines`]. + /// + /// This mapping is what makes scroll/selection stable as the transcript grows and reflows: + /// each visible line index can be mapped back to the originating `(cell_index, line_in_cell)` + /// pair (or to a `Spacer` row). + /// + /// Typical uses: + /// - scroll anchoring (`TranscriptScroll` resolves/anchors using this metadata) + /// - determining whether a visible row is a user-authored row (`cell_index → is_user_cell`) + pub(crate) fn line_meta(&self) -> &[TranscriptLineMeta] { + &self.wrapped.transcript.meta + } + + /// Configure the per-line raster cache capacity. + /// + /// When `capacity == 0`, raster caching is disabled and rows are rendered directly into the + /// destination buffer (but wrapped transcript caching still applies). + pub(crate) fn set_raster_capacity(&mut self, capacity: usize) { + self.raster.set_capacity(capacity); + } + + /// Whether a flattened transcript line belongs to a user-authored history cell. + /// + /// User rows apply a row-wide base style (background). This is a property of the originating + /// cell, not of the line content, so it is derived from the cached `line_meta` mapping. + pub(crate) fn is_user_row(&self, line_index: usize) -> bool { + let Some(cell_index) = self + .wrapped + .transcript + .meta + .get(line_index) + .and_then(TranscriptLineMeta::cell_index) + else { + return false; + }; + + self.wrapped + .is_user_cell + .get(cell_index) + .copied() + .unwrap_or(false) + } + + /// Render a single cached line index into the destination `buf`. + /// + /// This is the draw hot-path helper: it looks up the wrapped `Line` for `line_index`, applies + /// user-row styling if needed, and then either rasterizes the line or copies cached cells into + /// place. + /// + /// Callers are expected to have already ensured the cache via [`Self::ensure_wrapped`]. + pub(crate) fn render_row_index_into( + &mut self, + line_index: usize, + row_area: Rect, + buf: &mut Buffer, + ) { + let is_user_row = self.is_user_row(line_index); + let line = &self.wrapped.transcript.lines[line_index]; + self.raster + .render_row_into(line_index, is_user_row, line, row_area, buf); + } +} + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum WrappedTranscriptUpdate { + /// The cache already represented the provided `cells` and `width`. + Unchanged, + /// The cache appended additional cells without rebuilding. + Appended, + /// The cache rebuilt from scratch (width change, truncation, or replacement). + Rebuilt, +} + +/// Incremental memoization of wrapped transcript lines for a given width. +/// +/// This cache exists so callers doing tight-loop scroll math (mouse wheel, PgUp/PgDn) and render +/// ticks do not repeatedly rebuild the wrapped transcript (`HistoryCell` → flattened `Line`s). +/// +/// It assumes the transcript is append-mostly: when new cells arrive, they are appended to the end +/// of `cells` and existing cells do not mutate. If the underlying cell list is replaced or +/// truncated, the cache rebuilds from scratch. +struct WrappedTranscriptCache { + /// Width this cache was last built for. + width: u16, + /// Number of leading cells already incorporated into [`Self::transcript`]. + cell_count: usize, + /// Pointer identity of the first cell at the time the cache was built. + /// + /// This is a cheap replacement/truncation detector: if the caller swaps the transcript list + /// (for example, drops old cells from the front to cap history length), the length may remain + /// the same while the content shifts. In that case, we must rebuild because `(cell_index, + /// line_in_cell)` mappings and scroll anchors would otherwise become inconsistent. + first_cell_ptr: Option<*const dyn HistoryCell>, + /// Cached flattened wrapped transcript output. + /// + /// Invariant: `lines.len() == meta.len() == joiner_before.len()`. + transcript: TranscriptLines, + /// Whether the flattened transcript has emitted at least one non-spacer line. + /// + /// This is used to decide whether to insert a spacer line between non-continuation cells. + has_emitted_lines: bool, + /// Per-cell marker indicating whether a logical cell is a [`UserHistoryCell`]. + /// + /// We store this alongside the wrapped transcript so user-row styling can be derived cheaply + /// from `TranscriptLineMeta::cell_index()` without re-inspecting the cell type every frame. + is_user_cell: Vec, +} + +impl WrappedTranscriptCache { + /// Create an empty wrapped transcript cache. + /// + /// The cache is inert until the first [`Self::ensure`] call; until then it contains no + /// rendered transcript state. + fn new() -> Self { + Self { + width: 0, + cell_count: 0, + first_cell_ptr: None, + transcript: TranscriptLines { + lines: Vec::new(), + meta: Vec::new(), + joiner_before: Vec::new(), + }, + has_emitted_lines: false, + is_user_cell: Vec::new(), + } + } + + /// Ensure the wrapped transcript represents `cells` at `width`. + /// + /// This cache is intentionally single-entry and width-scoped: + /// - when `width` is unchanged and `cells` has grown, append only the new cells + /// - when `width` changes or the transcript is replaced/truncated, rebuild from scratch + /// + /// The cache assumes history cells are append-only and immutable once inserted. If existing + /// cell contents can change without changing identity, callers must treat that as a rebuild. + fn ensure(&mut self, cells: &[Arc], width: u16) -> WrappedTranscriptUpdate { + if width == 0 { + self.width = width; + self.cell_count = cells.len(); + self.first_cell_ptr = cells.first().map(Arc::as_ptr); + self.transcript.lines.clear(); + self.transcript.meta.clear(); + self.transcript.joiner_before.clear(); + self.has_emitted_lines = false; + self.is_user_cell.clear(); + return WrappedTranscriptUpdate::Rebuilt; + } + + let current_first_ptr = cells.first().map(Arc::as_ptr); + if self.width != width + || self.cell_count > cells.len() + || (self.cell_count > 0 + && current_first_ptr.is_some() + && self.first_cell_ptr != current_first_ptr) + { + self.rebuild(cells, width); + return WrappedTranscriptUpdate::Rebuilt; + } + + if self.cell_count == cells.len() { + return WrappedTranscriptUpdate::Unchanged; + } + + let old_cell_count = self.cell_count; + self.cell_count = cells.len(); + self.first_cell_ptr = current_first_ptr; + let base_opts: crate::wrapping::RtOptions<'_> = + crate::wrapping::RtOptions::new(width.max(1) as usize); + for (cell_index, cell) in cells.iter().enumerate().skip(old_cell_count) { + self.is_user_cell + .push(cell.as_any().is::()); + crate::transcript_render::append_wrapped_transcript_cell( + &mut self.transcript, + &mut self.has_emitted_lines, + cell_index, + cell, + width, + &base_opts, + ); + } + + WrappedTranscriptUpdate::Appended + } + + /// Rebuild the wrapped transcript cache from scratch. + /// + /// This is used when width changes, the transcript is truncated, or the caller provides a new + /// cell list that cannot be treated as an append to the previous one. + fn rebuild(&mut self, cells: &[Arc], width: u16) { + self.width = width; + self.cell_count = cells.len(); + self.first_cell_ptr = cells.first().map(Arc::as_ptr); + self.transcript.lines.clear(); + self.transcript.meta.clear(); + self.transcript.joiner_before.clear(); + self.has_emitted_lines = false; + self.is_user_cell.clear(); + self.is_user_cell.reserve(cells.len()); + + let base_opts: crate::wrapping::RtOptions<'_> = + crate::wrapping::RtOptions::new(width.max(1) as usize); + for (cell_index, cell) in cells.iter().enumerate() { + self.is_user_cell + .push(cell.as_any().is::()); + crate::transcript_render::append_wrapped_transcript_cell( + &mut self.transcript, + &mut self.has_emitted_lines, + cell_index, + cell, + width, + &base_opts, + ); + } + } +} + +/// Bounded cache of rasterized transcript rows. +/// +/// Each cached entry stores the final rendered [`ratatui::buffer::Cell`] values for a single +/// transcript line rendered into a 1-row buffer. +/// +/// Keying: +/// - The cache key includes `(line_index, is_user_row)`. +/// - Width is stored out-of-band and any width change clears the cache. +/// +/// Eviction: +/// - The cache uses an approximate LRU implemented with a monotonic stamp (`clock`) and an +/// `(key, stamp)` queue. +/// - This avoids per-access list manipulation while still keeping memory bounded. +struct TranscriptRasterCache { + /// Width this cache's rasterized rows were rendered at. + width: u16, + /// Maximum number of rasterized rows to retain. + capacity: usize, + /// Monotonic counter used to stamp accesses for eviction. + clock: u64, + /// Version of the terminal palette used for the cached rows. + palette_version: u64, + /// Access log used for approximate LRU eviction. + lru: VecDeque<(u64, u64)>, + /// Cached rasterized rows by key. + rows: HashMap, +} + +/// Cached raster for a single transcript line at a particular width. +#[derive(Clone)] +struct RasterizedRow { + /// The last access stamp recorded for this row. + /// + /// Eviction only removes a row when a popped `(key, stamp)` matches this value. + last_used: u64, + /// The full row of rendered cells (length is `width` at the time of rasterization). + cells: Vec, +} + +impl TranscriptRasterCache { + /// Create an empty raster cache (caching disabled until a non-zero capacity is set). + fn new() -> Self { + Self { + width: 0, + capacity: 0, + clock: 0, + palette_version: crate::terminal_palette::palette_version(), + lru: VecDeque::new(), + rows: HashMap::new(), + } + } + + /// Drop all cached rasterized rows and reset access tracking. + /// + /// This is used on width changes and when disabling caching so we don't retain stale rows or + /// unbounded memory. + fn clear(&mut self) { + self.lru.clear(); + self.rows.clear(); + self.clock = 0; + } + + /// Set the maximum number of cached rasterized rows. + /// + /// When set to 0, caching is disabled and any existing cached rows are dropped. + fn set_capacity(&mut self, capacity: usize) { + self.capacity = capacity; + self.evict_if_needed(); + } + + /// Render a single wrapped transcript line into `buf`, using a cached raster when possible. + /// + /// The cache key includes `is_user_row` because user rows apply a row-wide base style, so the + /// final raster differs even when the text spans are identical. + fn render_row_into( + &mut self, + line_index: usize, + is_user_row: bool, + line: &Line<'static>, + row_area: Rect, + buf: &mut Buffer, + ) { + if row_area.width == 0 || row_area.height == 0 { + return; + } + + let palette_version = crate::terminal_palette::palette_version(); + if palette_version != self.palette_version { + self.palette_version = palette_version; + self.clear(); + } + + if self.width != row_area.width { + self.width = row_area.width; + self.clear(); + } + + if self.capacity == 0 { + let cells = rasterize_line(line, row_area.width, is_user_row); + copy_row(row_area, buf, &cells); + return; + } + + let key = raster_key(line_index, is_user_row); + let stamp = self.bump_clock(); + if let Some(row) = self.rows.get_mut(&key) { + row.last_used = stamp; + self.lru.push_back((key, stamp)); + copy_row(row_area, buf, &row.cells); + return; + } + + let cells = rasterize_line(line, row_area.width, is_user_row); + copy_row(row_area, buf, &cells); + self.rows.insert( + key, + RasterizedRow { + last_used: stamp, + cells, + }, + ); + self.lru.push_back((key, stamp)); + self.evict_if_needed(); + } + + /// Return a new access stamp. + /// + /// The stamp is used only for equality checks ("is this the latest access for this key?") so a + /// wrapping counter is sufficient; `u64` wraparound is effectively unreachable in practice for + /// a UI cache. + fn bump_clock(&mut self) -> u64 { + let stamp = self.clock; + self.clock = self.clock.wrapping_add(1); + stamp + } + + /// Evict old cached rows until `rows.len() <= capacity`. + /// + /// The cache uses an approximate LRU: we push `(key, stamp)` on every access, and only evict a + /// row when the popped entry matches the row's current `last_used` stamp. + fn evict_if_needed(&mut self) { + if self.capacity == 0 { + self.clear(); + return; + } + while self.rows.len() > self.capacity { + let Some((key, stamp)) = self.lru.pop_front() else { + break; + }; + if self + .rows + .get(&key) + .is_some_and(|row| row.last_used == stamp) + { + self.rows.remove(&key); + } + } + } +} + +/// Compute the cache key for a rasterized transcript row. +/// +/// We key by `line_index` (not by hashing line content) because: +/// - it is effectively free in the draw loop +/// - the wrapped transcript cache defines a stable `(index → Line)` mapping until the next rebuild +/// - rebuilds clear the raster cache, so indices cannot alias across different transcripts +/// +/// `is_user_row` is included because user rows apply a row-wide base style that affects every cell. +fn raster_key(line_index: usize, is_user_row: bool) -> u64 { + (line_index as u64) << 1 | u64::from(is_user_row) +} + +/// Rasterize a single wrapped transcript [`Line`] into a 1-row cell vector. +/// +/// This is the expensive step we want to avoid repeating on every redraw: it runs Ratatui's +/// rendering for the line (including grapheme segmentation) into a scratch buffer and then copies +/// out the rendered cells. +/// +/// For user rows, we pre-fill the row with the base user style so the cached raster includes the +/// full-width background, matching the viewport behavior. +fn rasterize_line( + line: &Line<'static>, + width: u16, + is_user_row: bool, +) -> Vec { + let scratch_area = Rect::new(0, 0, width, 1); + let mut scratch = Buffer::empty(scratch_area); + + if is_user_row { + let base_style = crate::style::user_message_style(); + for x in 0..width { + scratch[(x, 0)].set_style(base_style); + } + } + + line.render_ref(scratch_area, &mut scratch); + + let mut out = Vec::with_capacity(width as usize); + for x in 0..width { + out.push(scratch[(x, 0)].clone()); + } + out +} + +/// Copy a cached rasterized row into a destination buffer at `area`. +/// +/// This is the "fast path" for redraws: once a row is cached, a redraw copies the pre-rendered +/// cells into the frame buffer without re-running span layout/grapheme segmentation. +fn copy_row(area: Rect, buf: &mut Buffer, cells: &[ratatui::buffer::Cell]) { + let y = area.y; + for (dx, cell) in cells.iter().enumerate() { + let x = area.x.saturating_add(dx as u16); + if x >= area.right() { + break; + } + buf[(x, y)] = cell.clone(); + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::history_cell::TranscriptLinesWithJoiners; + use crate::history_cell::UserHistoryCell; + use pretty_assertions::assert_eq; + use ratatui::style::Color; + use ratatui::style::Style; + use ratatui::style::Stylize; + use ratatui::text::Span; + use std::sync::atomic::AtomicUsize; + use std::sync::atomic::Ordering; + + #[derive(Debug)] + struct FakeCell { + lines: Vec>, + joiner_before: Vec>, + is_stream_continuation: bool, + transcript_calls: Arc, + } + + impl FakeCell { + fn new( + lines: Vec>, + joiner_before: Vec>, + is_stream_continuation: bool, + transcript_calls: Arc, + ) -> Self { + Self { + lines, + joiner_before, + is_stream_continuation, + transcript_calls, + } + } + } + + impl HistoryCell for FakeCell { + fn display_lines(&self, _width: u16) -> Vec> { + self.lines.clone() + } + + fn transcript_lines_with_joiners(&self, _width: u16) -> TranscriptLinesWithJoiners { + self.transcript_calls.fetch_add(1, Ordering::Relaxed); + TranscriptLinesWithJoiners { + lines: self.lines.clone(), + joiner_before: self.joiner_before.clone(), + } + } + + fn is_stream_continuation(&self) -> bool { + self.is_stream_continuation + } + } + + #[test] + fn wrapped_cache_matches_build_wrapped_transcript_lines() { + let calls0 = Arc::new(AtomicUsize::new(0)); + let calls1 = Arc::new(AtomicUsize::new(0)); + let calls2 = Arc::new(AtomicUsize::new(0)); + + let cells: Vec> = vec![ + // Wrapping case: expect a soft-wrap joiner for the continuation segment. + Arc::new(FakeCell::new( + vec![Line::from("• hello world")], + vec![None], + false, + calls0, + )), + // Preformatted (cyan) lines are not wrapped by the viewport wrapper. + Arc::new(FakeCell::new( + vec![Line::from(" let x = 12345;").cyan()], + vec![None], + true, + calls1, + )), + // New non-continuation cell inserts a spacer. + Arc::new(FakeCell::new( + vec![Line::from("• foo bar")], + vec![None], + false, + calls2, + )), + ]; + + let width = 8; + let expected = crate::transcript_render::build_wrapped_transcript_lines(&cells, width); + + let mut cache = TranscriptViewCache::new(); + cache.ensure_wrapped(&cells, width); + + assert_eq!(cache.lines(), expected.lines.as_slice()); + assert_eq!(cache.line_meta(), expected.meta.as_slice()); + assert_eq!( + cache.wrapped.transcript.joiner_before, + expected.joiner_before + ); + assert_eq!(cache.lines().len(), cache.line_meta().len()); + assert_eq!( + cache.lines().len(), + cache.wrapped.transcript.joiner_before.len() + ); + } + + #[test] + fn wrapped_cache_ensure_appends_only_new_cells_when_width_is_unchanged() { + let calls0 = Arc::new(AtomicUsize::new(0)); + let calls1 = Arc::new(AtomicUsize::new(0)); + let cells: Vec> = vec![ + Arc::new(FakeCell::new( + vec![Line::from("• hello world")], + vec![None], + false, + calls0.clone(), + )), + Arc::new(FakeCell::new( + vec![Line::from("• foo bar")], + vec![None], + false, + calls1.clone(), + )), + ]; + + let mut cache = TranscriptViewCache::new(); + cache.ensure_wrapped(&cells[..1], 8); + cache.ensure_wrapped(&cells, 8); + + assert_eq!(calls0.load(Ordering::Relaxed), 1); + assert_eq!(calls1.load(Ordering::Relaxed), 1); + + assert_eq!( + cache.lines(), + &[ + Line::from("• hello"), + Line::from("world"), + Line::from(""), + Line::from("• foo"), + Line::from("bar") + ] + ); + assert_eq!( + cache.line_meta(), + &[ + TranscriptLineMeta::CellLine { + cell_index: 0, + line_in_cell: 0 + }, + TranscriptLineMeta::CellLine { + cell_index: 0, + line_in_cell: 1 + }, + TranscriptLineMeta::Spacer, + TranscriptLineMeta::CellLine { + cell_index: 1, + line_in_cell: 0 + }, + TranscriptLineMeta::CellLine { + cell_index: 1, + line_in_cell: 1 + }, + ] + ); + assert_eq!( + cache.wrapped.transcript.joiner_before.as_slice(), + &[ + None, + Some(" ".to_string()), + None, + None, + Some(" ".to_string()), + ] + ); + } + + #[test] + fn wrapped_cache_ensure_rebuilds_on_width_change() { + let calls0 = Arc::new(AtomicUsize::new(0)); + let calls1 = Arc::new(AtomicUsize::new(0)); + let cells: Vec> = vec![ + Arc::new(FakeCell::new( + vec![Line::from("• hello world")], + vec![None], + false, + calls0.clone(), + )), + Arc::new(FakeCell::new( + vec![Line::from("• foo bar")], + vec![None], + false, + calls1.clone(), + )), + ]; + + let mut cache = TranscriptViewCache::new(); + cache.ensure_wrapped(&cells, 8); + cache.ensure_wrapped(&cells, 10); + + assert_eq!(calls0.load(Ordering::Relaxed), 2); + assert_eq!(calls1.load(Ordering::Relaxed), 2); + + let expected = crate::transcript_render::build_wrapped_transcript_lines(&cells, 10); + assert_eq!(cache.lines(), expected.lines.as_slice()); + assert_eq!(cache.line_meta(), expected.meta.as_slice()); + assert_eq!( + cache.wrapped.transcript.joiner_before, + expected.joiner_before + ); + } + + #[test] + fn wrapped_cache_ensure_rebuilds_on_truncation() { + let calls0 = Arc::new(AtomicUsize::new(0)); + let calls1 = Arc::new(AtomicUsize::new(0)); + let cells: Vec> = vec![ + Arc::new(FakeCell::new( + vec![Line::from("• hello world")], + vec![None], + false, + calls0.clone(), + )), + Arc::new(FakeCell::new( + vec![Line::from("• foo bar")], + vec![None], + false, + calls1.clone(), + )), + ]; + + let mut cache = TranscriptViewCache::new(); + cache.ensure_wrapped(&cells, 8); + cache.ensure_wrapped(&cells[..1], 8); + + // The second ensure is a rebuild of the truncated prefix; only the first cell is rendered. + assert_eq!(calls0.load(Ordering::Relaxed), 2); + assert_eq!(calls1.load(Ordering::Relaxed), 1); + + let expected = crate::transcript_render::build_wrapped_transcript_lines(&cells[..1], 8); + assert_eq!(cache.lines(), expected.lines.as_slice()); + assert_eq!(cache.line_meta(), expected.meta.as_slice()); + } + + #[test] + fn wrapped_cache_ensure_with_zero_width_clears_without_calling_cell_render() { + let calls = Arc::new(AtomicUsize::new(0)); + let cells: Vec> = vec![Arc::new(FakeCell::new( + vec![Line::from("• hello world")], + vec![None], + false, + calls.clone(), + ))]; + + let mut cache = TranscriptViewCache::new(); + cache.ensure_wrapped(&cells, 0); + + assert_eq!(calls.load(Ordering::Relaxed), 0); + assert_eq!(cache.lines(), &[]); + assert_eq!(cache.line_meta(), &[]); + assert_eq!( + cache.wrapped.transcript.joiner_before, + Vec::>::new() + ); + } + + #[test] + fn wrapped_cache_ensure_rebuilds_when_first_cell_pointer_changes() { + let calls_a = Arc::new(AtomicUsize::new(0)); + let calls_b = Arc::new(AtomicUsize::new(0)); + + let cell_a0: Arc = Arc::new(FakeCell::new( + vec![Line::from("• a")], + vec![None], + false, + calls_a.clone(), + )); + let cell_a1: Arc = Arc::new(FakeCell::new( + vec![Line::from("• b")], + vec![None], + false, + calls_b.clone(), + )); + + let mut cache = TranscriptViewCache::new(); + cache.ensure_wrapped(&[cell_a0.clone(), cell_a1.clone()], 10); + assert_eq!(calls_a.load(Ordering::Relaxed), 1); + assert_eq!(calls_b.load(Ordering::Relaxed), 1); + + // Replace the transcript with a different first cell but keep the length the same. + let calls_c = Arc::new(AtomicUsize::new(0)); + let cell_b0: Arc = Arc::new(FakeCell::new( + vec![Line::from("• c")], + vec![None], + false, + calls_c.clone(), + )); + + cache.ensure_wrapped(&[cell_b0.clone(), cell_a1.clone()], 10); + + // This should be treated as a replacement and rebuilt from scratch. + assert_eq!(calls_c.load(Ordering::Relaxed), 1); + assert_eq!(calls_b.load(Ordering::Relaxed), 2); + } + + #[test] + fn raster_cache_reuses_rows_and_clears_on_width_change() { + let mut cache = TranscriptViewCache::new(); + let calls = Arc::new(AtomicUsize::new(0)); + let cells: Vec> = vec![Arc::new(FakeCell::new( + vec![Line::from(vec![ + Span::from("• hello").style(Style::default().fg(Color::Magenta)), + ])], + vec![None], + false, + calls, + ))]; + + cache.ensure_wrapped(&cells, 20); + cache.set_raster_capacity(8); + + let area = Rect::new(0, 0, 10, 1); + let mut buf = Buffer::empty(area); + + cache.render_row_index_into(0, area, &mut buf); + assert_eq!(cache.raster.rows.len(), 1); + + cache.render_row_index_into(0, area, &mut buf); + assert_eq!(cache.raster.rows.len(), 1); + + let mut buf_wide = Buffer::empty(Rect::new(0, 0, 12, 1)); + cache.render_row_index_into(0, Rect::new(0, 0, 12, 1), &mut buf_wide); + assert_eq!(cache.raster.width, 12); + assert_eq!(cache.raster.rows.len(), 1); + } + + fn direct_render_cells( + line: &Line<'static>, + width: u16, + is_user_row: bool, + ) -> Vec { + let area = Rect::new(0, 0, width, 1); + let mut scratch = Buffer::empty(area); + if is_user_row { + let base_style = crate::style::user_message_style(); + for x in 0..width { + scratch[(x, 0)].set_style(base_style); + } + } + line.render_ref(area, &mut scratch); + (0..width).map(|x| scratch[(x, 0)].clone()).collect() + } + + #[test] + fn rasterize_line_matches_direct_render_for_user_and_non_user_rows() { + let width = 12; + let line = Line::from(vec!["hello".into(), " ".into(), "world".magenta()]); + + let non_user = rasterize_line(&line, width, false); + assert_eq!(non_user, direct_render_cells(&line, width, false)); + + let user = rasterize_line(&line, width, true); + assert_eq!(user, direct_render_cells(&line, width, true)); + } + + #[test] + fn raster_cache_evicts_old_rows_when_over_capacity() { + let mut cache = TranscriptViewCache::new(); + let calls = Arc::new(AtomicUsize::new(0)); + let cells: Vec> = vec![Arc::new(FakeCell::new( + vec![Line::from("first"), Line::from("second")], + vec![None, None], + false, + calls, + ))]; + + cache.ensure_wrapped(&cells, 10); + cache.set_raster_capacity(1); + + let area = Rect::new(0, 0, 10, 1); + let mut buf = Buffer::empty(area); + + cache.render_row_index_into(0, area, &mut buf); + assert_eq!(cache.raster.rows.len(), 1); + assert!(cache.raster.rows.contains_key(&raster_key(0, false))); + + cache.render_row_index_into(1, area, &mut buf); + assert_eq!(cache.raster.rows.len(), 1); + assert!(cache.raster.rows.contains_key(&raster_key(1, false))); + } + + #[test] + fn raster_cache_resets_when_palette_version_changes() { + let mut cache = TranscriptViewCache::new(); + let calls = Arc::new(AtomicUsize::new(0)); + let cells: Vec> = vec![Arc::new(FakeCell::new( + vec![Line::from("palette")], + vec![None], + false, + calls, + ))]; + + cache.ensure_wrapped(&cells, 20); + cache.set_raster_capacity(1); + + let area = Rect::new(0, 0, 10, 1); + let mut buf = Buffer::empty(area); + + cache.render_row_index_into(0, area, &mut buf); + assert_eq!(cache.raster.clock, 1); + + cache.render_row_index_into(0, area, &mut buf); + assert_eq!(cache.raster.clock, 2); + + crate::terminal_palette::requery_default_colors(); + cache.render_row_index_into(0, area, &mut buf); + assert_eq!(cache.raster.clock, 1); + } + + #[test] + fn render_row_index_into_treats_user_history_cells_as_user_rows() { + let mut cache = TranscriptViewCache::new(); + let cells: Vec> = vec![Arc::new(UserHistoryCell { + message: "hello".to_string(), + })]; + + cache.ensure_wrapped(&cells, 20); + cache.set_raster_capacity(8); + + let area = Rect::new(0, 0, 20, 1); + let mut buf = Buffer::empty(area); + + cache.render_row_index_into(0, area, &mut buf); + assert!(cache.is_user_row(0)); + assert!(cache.raster.rows.contains_key(&raster_key(0, true))); + } +} diff --git a/codex-rs/tui2/src/tui/scrolling.rs b/codex-rs/tui2/src/tui/scrolling.rs index c3ca6e94de..4b7b23c150 100644 --- a/codex-rs/tui2/src/tui/scrolling.rs +++ b/codex-rs/tui2/src/tui/scrolling.rs @@ -17,7 +17,7 @@ //! the newly flattened line list on the next frame. //! //! Spacer rows between non-continuation cells are represented as `TranscriptLineMeta::Spacer`. -//! They are not valid anchors; `anchor_for` will pick the nearest non-spacer line when needed. +//! They are valid scroll anchors so 1-line scrolling does not "stick" at cell boundaries. pub(crate) mod mouse; pub(crate) use mouse::MouseScrollState; @@ -78,6 +78,13 @@ pub(crate) enum TranscriptScroll { cell_index: usize, line_in_cell: usize, }, + /// Anchor the viewport to the spacer row immediately before a cell. + /// + /// This exists because spacer rows are real, visible transcript rows, and users may scroll + /// through them one line at a time (especially with trackpads). Without a dedicated spacer + /// anchor, a 1-line scroll that lands on a spacer would snap back to the adjacent cell line + /// and appear to "stick" at boundaries. + ScrolledSpacerBeforeCell { cell_index: usize }, } impl TranscriptScroll { @@ -108,6 +115,13 @@ impl TranscriptScroll { None => (Self::ToBottom, max_start), } } + Self::ScrolledSpacerBeforeCell { cell_index } => { + let anchor = spacer_before_cell_index(line_meta, cell_index); + match anchor { + Some(idx) => (self, idx.min(max_start)), + None => (Self::ToBottom, max_start), + } + } } } @@ -142,6 +156,11 @@ impl TranscriptScroll { } => anchor_index(line_meta, cell_index, line_in_cell) .unwrap_or(max_start) .min(max_start), + Self::ScrolledSpacerBeforeCell { cell_index } => { + spacer_before_cell_index(line_meta, cell_index) + .unwrap_or(max_start) + .min(max_start) + } }; let new_top = if delta_lines < 0 { @@ -164,15 +183,35 @@ impl TranscriptScroll { /// This is the inverse of "resolving a scroll state to a top-row offset": /// given a concrete flattened line index, pick a stable `(cell_index, line_in_cell)` anchor. /// - /// See `resolve_top` for `line_meta` semantics. This prefers the nearest line at or after `start` - /// (skipping spacer rows), falling back to the nearest line before it when needed. + /// See `resolve_top` for `line_meta` semantics. This prefers the line at `start` (including + /// spacer rows), falling back to the nearest non-spacer line after or before it when needed. pub(crate) fn anchor_for(line_meta: &[TranscriptLineMeta], start: usize) -> Option { - let anchor = - anchor_at_or_after(line_meta, start).or_else(|| anchor_at_or_before(line_meta, start)); - anchor.map(|(cell_index, line_in_cell)| Self::Scrolled { - cell_index, - line_in_cell, - }) + if line_meta.is_empty() { + return None; + } + + let start = start.min(line_meta.len().saturating_sub(1)); + match line_meta[start] { + TranscriptLineMeta::CellLine { + cell_index, + line_in_cell, + } => Some(Self::Scrolled { + cell_index, + line_in_cell, + }), + TranscriptLineMeta::Spacer => { + if let Some((cell_index, _)) = anchor_at_or_after(line_meta, start) { + Some(Self::ScrolledSpacerBeforeCell { cell_index }) + } else { + anchor_at_or_before(line_meta, start).map(|(cell_index, line_in_cell)| { + Self::Scrolled { + cell_index, + line_in_cell, + } + }) + } + } + } } } @@ -198,6 +237,26 @@ fn anchor_index( }) } +/// Locate the flattened line index for the spacer row immediately before `cell_index`. +/// +/// The spacer itself is not uniquely tagged in `TranscriptLineMeta`, so we locate the first +/// visual line of the cell (`line_in_cell == 0`) and, if it is preceded by a spacer row, return +/// that spacer's index. If the spacer is missing (for example when the cell is a stream +/// continuation), we fall back to the cell's first line index so scrolling remains usable. +fn spacer_before_cell_index(line_meta: &[TranscriptLineMeta], cell_index: usize) -> Option { + let cell_first = anchor_index(line_meta, cell_index, 0)?; + if cell_first > 0 + && matches!( + line_meta.get(cell_first.saturating_sub(1)), + Some(TranscriptLineMeta::Spacer) + ) + { + Some(cell_first.saturating_sub(1)) + } else { + Some(cell_first) + } +} + /// Find the first transcript line at or after the given flattened index. fn anchor_at_or_after(line_meta: &[TranscriptLineMeta], start: usize) -> Option<(usize, usize)> { if line_meta.is_empty() { @@ -272,6 +331,33 @@ mod tests { assert_eq!(top, 2); } + #[test] + fn scrolled_by_can_land_on_spacer_rows() { + let meta = meta(&[ + cell_line(0, 0), + TranscriptLineMeta::Spacer, + cell_line(1, 0), + cell_line(1, 1), + ]); + + let scroll = TranscriptScroll::Scrolled { + cell_index: 1, + line_in_cell: 0, + }; + + assert_eq!( + scroll.scrolled_by(-1, &meta, 2), + TranscriptScroll::ScrolledSpacerBeforeCell { cell_index: 1 } + ); + assert_eq!( + TranscriptScroll::ScrolledSpacerBeforeCell { cell_index: 1 }.scrolled_by(-1, &meta, 2), + TranscriptScroll::Scrolled { + cell_index: 0, + line_in_cell: 0 + } + ); + } + #[test] fn resolve_top_scrolled_falls_back_when_anchor_missing() { let meta = meta(&[cell_line(0, 0), TranscriptLineMeta::Spacer, cell_line(1, 0)]); @@ -350,17 +436,11 @@ mod tests { assert_eq!( TranscriptScroll::anchor_for(&meta, 0), - Some(TranscriptScroll::Scrolled { - cell_index: 0, - line_in_cell: 0 - }) + Some(TranscriptScroll::ScrolledSpacerBeforeCell { cell_index: 0 }) ); assert_eq!( TranscriptScroll::anchor_for(&meta, 2), - Some(TranscriptScroll::Scrolled { - cell_index: 1, - line_in_cell: 0 - }) + Some(TranscriptScroll::ScrolledSpacerBeforeCell { cell_index: 1 }) ); assert_eq!( TranscriptScroll::anchor_for(&meta, 3), diff --git a/codex-rs/windows-sandbox-rs/src/audit.rs b/codex-rs/windows-sandbox-rs/src/audit.rs index 9e02f86c14..659ea57b6f 100644 --- a/codex-rs/windows-sandbox-rs/src/audit.rs +++ b/codex-rs/windows-sandbox-rs/src/audit.rs @@ -30,7 +30,7 @@ const SKIP_DIR_SUFFIXES: &[&str] = &[ "/programdata", ]; -fn normalize_path_key(p: &Path) -> String { +pub(crate) fn normalize_path_key(p: &Path) -> String { let n = dunce::canonicalize(p).unwrap_or_else(|_| p.to_path_buf()); n.to_string_lossy().replace('\\', "/").to_ascii_lowercase() } diff --git a/codex-rs/windows-sandbox-rs/src/firewall.rs b/codex-rs/windows-sandbox-rs/src/firewall.rs new file mode 100644 index 0000000000..c75214bb36 --- /dev/null +++ b/codex-rs/windows-sandbox-rs/src/firewall.rs @@ -0,0 +1,151 @@ +#![cfg(target_os = "windows")] + +use anyhow::Result; +use std::fs::File; +use std::io::Write; + +use windows::core::Interface; +use windows::core::BSTR; +use windows::Win32::Foundation::VARIANT_TRUE; +use windows::Win32::NetworkManagement::WindowsFirewall::INetFwPolicy2; +use windows::Win32::NetworkManagement::WindowsFirewall::INetFwRule3; +use windows::Win32::NetworkManagement::WindowsFirewall::NetFwPolicy2; +use windows::Win32::NetworkManagement::WindowsFirewall::NetFwRule; +use windows::Win32::NetworkManagement::WindowsFirewall::NET_FW_ACTION_BLOCK; +use windows::Win32::NetworkManagement::WindowsFirewall::NET_FW_IP_PROTOCOL_ANY; +use windows::Win32::NetworkManagement::WindowsFirewall::NET_FW_PROFILE2_ALL; +use windows::Win32::NetworkManagement::WindowsFirewall::NET_FW_RULE_DIR_OUT; +use windows::Win32::System::Com::CoCreateInstance; +use windows::Win32::System::Com::CoInitializeEx; +use windows::Win32::System::Com::CoUninitialize; +use windows::Win32::System::Com::CLSCTX_INPROC_SERVER; +use windows::Win32::System::Com::COINIT_APARTMENTTHREADED; + +// This is the stable identifier we use to find/update the rule idempotently. +// It intentionally does not change between installs. +const OFFLINE_BLOCK_RULE_NAME: &str = "codex_sandbox_offline_block_outbound"; + +// Friendly text shown in the firewall UI. +const OFFLINE_BLOCK_RULE_FRIENDLY: &str = "Codex Sandbox Offline - Block Outbound"; + +pub fn ensure_offline_outbound_block(offline_sid: &str, log: &mut File) -> Result<()> { + let local_user_spec = format!("O:LSD:(A;;CC;;;{offline_sid})"); + + let hr = unsafe { CoInitializeEx(None, COINIT_APARTMENTTHREADED) }; + if hr.is_err() { + return Err(anyhow::anyhow!("CoInitializeEx failed: {hr:?}")); + } + + let result = unsafe { + (|| -> Result<()> { + let policy: INetFwPolicy2 = CoCreateInstance(&NetFwPolicy2, None, CLSCTX_INPROC_SERVER) + .map_err(|e| anyhow::anyhow!("CoCreateInstance NetFwPolicy2: {e:?}"))?; + let rules = policy + .Rules() + .map_err(|e| anyhow::anyhow!("INetFwPolicy2::Rules: {e:?}"))?; + + // Block all outbound IP protocols for this user. + ensure_block_rule( + &rules, + OFFLINE_BLOCK_RULE_NAME, + OFFLINE_BLOCK_RULE_FRIENDLY, + NET_FW_IP_PROTOCOL_ANY.0, + &local_user_spec, + offline_sid, + log, + )?; + Ok(()) + })() + }; + + unsafe { + CoUninitialize(); + } + result +} + +fn ensure_block_rule( + rules: &windows::Win32::NetworkManagement::WindowsFirewall::INetFwRules, + internal_name: &str, + friendly_desc: &str, + protocol: i32, + local_user_spec: &str, + offline_sid: &str, + log: &mut File, +) -> Result<()> { + let name = BSTR::from(internal_name); + let rule: INetFwRule3 = match unsafe { rules.Item(&name) } { + Ok(existing) => existing + .cast() + .map_err(|e| anyhow::anyhow!("cast existing firewall rule to INetFwRule3: {e:?}"))?, + Err(_) => { + let new_rule: INetFwRule3 = + unsafe { CoCreateInstance(&NetFwRule, None, CLSCTX_INPROC_SERVER) } + .map_err(|e| anyhow::anyhow!("CoCreateInstance NetFwRule: {e:?}"))?; + unsafe { new_rule.SetName(&name) }.map_err(|e| anyhow::anyhow!("SetName: {e:?}"))?; + // Set all properties before adding the rule so we don't leave half-configured rules. + configure_rule( + &new_rule, + friendly_desc, + protocol, + local_user_spec, + offline_sid, + )?; + unsafe { rules.Add(&new_rule) }.map_err(|e| anyhow::anyhow!("Rules::Add: {e:?}"))?; + new_rule + } + }; + + // Always re-apply fields to keep the setup idempotent. + configure_rule(&rule, friendly_desc, protocol, local_user_spec, offline_sid)?; + + log_line( + log, + &format!( + "firewall rule configured name={internal_name} protocol={protocol} LocalUserAuthorizedList={local_user_spec}" + ), + )?; + Ok(()) +} + +fn configure_rule( + rule: &INetFwRule3, + friendly_desc: &str, + protocol: i32, + local_user_spec: &str, + offline_sid: &str, +) -> Result<()> { + unsafe { + rule.SetDescription(&BSTR::from(friendly_desc)) + .map_err(|e| anyhow::anyhow!("SetDescription: {e:?}"))?; + rule.SetDirection(NET_FW_RULE_DIR_OUT) + .map_err(|e| anyhow::anyhow!("SetDirection: {e:?}"))?; + rule.SetAction(NET_FW_ACTION_BLOCK) + .map_err(|e| anyhow::anyhow!("SetAction: {e:?}"))?; + rule.SetEnabled(VARIANT_TRUE) + .map_err(|e| anyhow::anyhow!("SetEnabled: {e:?}"))?; + rule.SetProfiles(NET_FW_PROFILE2_ALL.0) + .map_err(|e| anyhow::anyhow!("SetProfiles: {e:?}"))?; + rule.SetProtocol(protocol) + .map_err(|e| anyhow::anyhow!("SetProtocol: {e:?}"))?; + rule.SetLocalUserAuthorizedList(&BSTR::from(local_user_spec)) + .map_err(|e| anyhow::anyhow!("SetLocalUserAuthorizedList: {e:?}"))?; + } + + // Read-back verification: ensure we actually wrote the expected SID scope. + let actual = unsafe { rule.LocalUserAuthorizedList() } + .map_err(|e| anyhow::anyhow!("LocalUserAuthorizedList (read-back): {e:?}"))?; + let actual_str = actual.to_string(); + if !actual_str.contains(offline_sid) { + anyhow::bail!( + "offline firewall rule user scope mismatch: expected SID {offline_sid}, got {actual_str}" + ); + } + Ok(()) +} + +fn log_line(log: &mut File, msg: &str) -> Result<()> { + let ts = chrono::Utc::now().to_rfc3339(); + writeln!(log, "[{ts}] {msg}")?; + Ok(()) +} diff --git a/codex-rs/windows-sandbox-rs/src/identity.rs b/codex-rs/windows-sandbox-rs/src/identity.rs index 4418b0b077..835acc5d8b 100644 --- a/codex-rs/windows-sandbox-rs/src/identity.rs +++ b/codex-rs/windows-sandbox-rs/src/identity.rs @@ -119,9 +119,10 @@ pub fn require_logon_sandbox_creds( ) -> Result { let sandbox_dir = crate::setup::sandbox_dir(codex_home); let needed_read = gather_read_roots(command_cwd, policy); - let mut needed_write = gather_write_roots(policy, policy_cwd, command_cwd, env_map); - // Ensure the sandbox directory itself is writable by sandbox users. - needed_write.push(sandbox_dir.clone()); + let needed_write = gather_write_roots(policy, policy_cwd, command_cwd, env_map); + // NOTE: Do not add CODEX_HOME/.sandbox to `needed_write`; it must remain non-writable by the + // restricted capability token. The setup helper's `lock_sandbox_dir` is responsible for + // granting the sandbox group access to this directory without granting the capability SID. let mut setup_reason: Option = None; let mut _existing_marker: Option = None; diff --git a/codex-rs/windows-sandbox-rs/src/setup_main_win.rs b/codex-rs/windows-sandbox-rs/src/setup_main_win.rs index 4796cda5da..d6cf9d0337 100644 --- a/codex-rs/windows-sandbox-rs/src/setup_main_win.rs +++ b/codex-rs/windows-sandbox-rs/src/setup_main_win.rs @@ -1,5 +1,7 @@ #![cfg(target_os = "windows")] +mod firewall; + use anyhow::Context; use anyhow::Result; use base64::engine::general_purpose::STANDARD as BASE64; @@ -28,22 +30,6 @@ use std::path::PathBuf; use std::process::Command; use std::process::Stdio; use std::sync::mpsc; -use windows::core::Interface; -use windows::core::BSTR; -use windows::Win32::Foundation::VARIANT_TRUE; -use windows::Win32::NetworkManagement::WindowsFirewall::INetFwPolicy2; -use windows::Win32::NetworkManagement::WindowsFirewall::INetFwRule3; -use windows::Win32::NetworkManagement::WindowsFirewall::NetFwPolicy2; -use windows::Win32::NetworkManagement::WindowsFirewall::NetFwRule; -use windows::Win32::NetworkManagement::WindowsFirewall::NET_FW_ACTION_BLOCK; -use windows::Win32::NetworkManagement::WindowsFirewall::NET_FW_IP_PROTOCOL_ANY; -use windows::Win32::NetworkManagement::WindowsFirewall::NET_FW_PROFILE2_ALL; -use windows::Win32::NetworkManagement::WindowsFirewall::NET_FW_RULE_DIR_OUT; -use windows::Win32::System::Com::CoCreateInstance; -use windows::Win32::System::Com::CoInitializeEx; -use windows::Win32::System::Com::CoUninitialize; -use windows::Win32::System::Com::CLSCTX_INPROC_SERVER; -use windows::Win32::System::Com::COINIT_APARTMENTTHREADED; use windows_sys::Win32::Foundation::GetLastError; use windows_sys::Win32::Foundation::LocalFree; use windows_sys::Win32::Foundation::HLOCAL; @@ -239,79 +225,6 @@ fn read_mask_allows_or_log( } } -fn run_netsh_firewall(sid: &str, log: &mut File) -> Result<()> { - let local_user_spec = format!("O:LSD:(A;;CC;;;{sid})"); - let hr = unsafe { CoInitializeEx(None, COINIT_APARTMENTTHREADED) }; - if hr.is_err() { - return Err(anyhow::anyhow!("CoInitializeEx failed: {hr:?}")); - } - let result = unsafe { - (|| -> Result<()> { - let policy: INetFwPolicy2 = CoCreateInstance(&NetFwPolicy2, None, CLSCTX_INPROC_SERVER) - .map_err(|e| anyhow::anyhow!("CoCreateInstance NetFwPolicy2: {e:?}"))?; - let rules = policy - .Rules() - .map_err(|e| anyhow::anyhow!("INetFwPolicy2::Rules: {e:?}"))?; - let name = BSTR::from("Codex Sandbox Offline - Block Outbound"); - let rule: INetFwRule3 = match rules.Item(&name) { - Ok(existing) => existing.cast().map_err(|e| { - anyhow::anyhow!("cast existing firewall rule to INetFwRule3: {e:?}") - })?, - Err(_) => { - let new_rule: INetFwRule3 = - CoCreateInstance(&NetFwRule, None, CLSCTX_INPROC_SERVER) - .map_err(|e| anyhow::anyhow!("CoCreateInstance NetFwRule: {e:?}"))?; - new_rule - .SetName(&name) - .map_err(|e| anyhow::anyhow!("SetName: {e:?}"))?; - new_rule - .SetDirection(NET_FW_RULE_DIR_OUT) - .map_err(|e| anyhow::anyhow!("SetDirection: {e:?}"))?; - new_rule - .SetAction(NET_FW_ACTION_BLOCK) - .map_err(|e| anyhow::anyhow!("SetAction: {e:?}"))?; - new_rule - .SetEnabled(VARIANT_TRUE) - .map_err(|e| anyhow::anyhow!("SetEnabled: {e:?}"))?; - new_rule - .SetProfiles(NET_FW_PROFILE2_ALL.0) - .map_err(|e| anyhow::anyhow!("SetProfiles: {e:?}"))?; - new_rule - .SetProtocol(NET_FW_IP_PROTOCOL_ANY.0) - .map_err(|e| anyhow::anyhow!("SetProtocol: {e:?}"))?; - rules - .Add(&new_rule) - .map_err(|e| anyhow::anyhow!("Rules::Add: {e:?}"))?; - new_rule - } - }; - rule.SetLocalUserAuthorizedList(&BSTR::from(local_user_spec.as_str())) - .map_err(|e| anyhow::anyhow!("SetLocalUserAuthorizedList: {e:?}"))?; - rule.SetEnabled(VARIANT_TRUE) - .map_err(|e| anyhow::anyhow!("SetEnabled: {e:?}"))?; - rule.SetProfiles(NET_FW_PROFILE2_ALL.0) - .map_err(|e| anyhow::anyhow!("SetProfiles: {e:?}"))?; - rule.SetAction(NET_FW_ACTION_BLOCK) - .map_err(|e| anyhow::anyhow!("SetAction: {e:?}"))?; - rule.SetDirection(NET_FW_RULE_DIR_OUT) - .map_err(|e| anyhow::anyhow!("SetDirection: {e:?}"))?; - rule.SetProtocol(NET_FW_IP_PROTOCOL_ANY.0) - .map_err(|e| anyhow::anyhow!("SetProtocol: {e:?}"))?; - log_line( - log, - &format!( - "firewall rule configured via COM with LocalUserAuthorizedList={local_user_spec}" - ), - )?; - Ok(()) - })() - }; - unsafe { - CoUninitialize(); - } - result -} - fn lock_sandbox_dir( dir: &Path, real_user: &str, @@ -549,7 +462,7 @@ fn run_setup_full(payload: &Payload, log: &mut File, sbx_dir: &Path) -> Result<( }; let mut refresh_errors: Vec = Vec::new(); if !refresh_only { - run_netsh_firewall(&offline_sid_str, log)?; + firewall::ensure_offline_outbound_block(&offline_sid_str, log)?; } if payload.read_roots.is_empty() { diff --git a/codex-rs/windows-sandbox-rs/src/setup_orchestrator.rs b/codex-rs/windows-sandbox-rs/src/setup_orchestrator.rs index 97ff1f0a8e..c46e43349a 100644 --- a/codex-rs/windows-sandbox-rs/src/setup_orchestrator.rs +++ b/codex-rs/windows-sandbox-rs/src/setup_orchestrator.rs @@ -408,15 +408,12 @@ fn build_payload_roots( read_roots_override: Option>, write_roots_override: Option>, ) -> (Vec, Vec) { - let sbx_dir = sandbox_dir(codex_home); - let mut write_roots = if let Some(roots) = write_roots_override { + let write_roots = if let Some(roots) = write_roots_override { canonical_existing(&roots) } else { gather_write_roots(policy, policy_cwd, command_cwd, env_map) }; - if !write_roots.contains(&sbx_dir) { - write_roots.push(sbx_dir.clone()); - } + let write_roots = filter_sensitive_write_roots(write_roots, codex_home); let mut read_roots = if let Some(roots) = read_roots_override { canonical_existing(&roots) } else { @@ -426,3 +423,17 @@ fn build_payload_roots( read_roots.retain(|root| !write_root_set.contains(root)); (read_roots, write_roots) } + +fn filter_sensitive_write_roots(mut roots: Vec, codex_home: &Path) -> Vec { + // Never grant capability write access to CODEX_HOME or anything under CODEX_HOME/.sandbox. + // These locations contain sandbox control/state and must remain tamper-resistant. + let codex_home_key = crate::audit::normalize_path_key(codex_home); + let sbx_dir_key = crate::audit::normalize_path_key(&sandbox_dir(codex_home)); + let sbx_dir_prefix = format!("{}/", sbx_dir_key.trim_end_matches('/')); + + roots.retain(|root| { + let key = crate::audit::normalize_path_key(root); + key != codex_home_key && key != sbx_dir_key && !key.starts_with(&sbx_dir_prefix) + }); + roots +} diff --git a/docs/config.md b/docs/config.md index 950a631fc5..2b64253d30 100644 --- a/docs/config.md +++ b/docs/config.md @@ -5,3 +5,15 @@ For basic configuration instructions, see [this documentation](https://developer For advanced configuration instructions, see [this documentation](https://developers.openai.com/codex/config-advanced). For a full configuration reference, see [this documentation](https://developers.openai.com/codex/config-reference). + +## Connecting to MCP servers + +Codex can connect to MCP servers configured in `~/.codex/config.toml`. See the configuration reference for the latest MCP server options: + +- https://developers.openai.com/codex/config-reference + +## Notify + +Codex can run a notification hook when the agent finishes a turn. See the configuration reference for the latest notification settings: + +- https://developers.openai.com/codex/config-reference diff --git a/docs/install.md b/docs/install.md index a9905144e3..20d8b54a54 100644 --- a/docs/install.md +++ b/docs/install.md @@ -26,7 +26,7 @@ rustup component add rustfmt rustup component add clippy # Install helper tools used by the workspace justfile: cargo install just -# Optional: install nextest for the `just test` helper (or use `cargo test --all-features` as a fallback) +# Optional: install nextest for the `just test` helper cargo install cargo-nextest # Build Codex. @@ -41,9 +41,9 @@ just fix -p # Run the relevant tests (project-specific is fastest), for example: cargo test -p codex-tui -# If you have cargo-nextest installed, `just test` runs the full suite: +# If you have cargo-nextest installed, `just test` runs the test suite via nextest: just test -# Otherwise, fall back to: +# If you specifically want the full `--all-features` matrix, use: cargo test --all-features ``` diff --git a/docs/tui2/performance-testing.md b/docs/tui2/performance-testing.md new file mode 100644 index 0000000000..ea5f36288a --- /dev/null +++ b/docs/tui2/performance-testing.md @@ -0,0 +1,97 @@ +# Performance testing (`codex-tui2`) + +This doc captures a repeatable workflow for investigating `codex-tui2` performance issues +(especially high idle CPU and high CPU while streaming) and validating optimizations to the draw +hot path. + +## Scope (this round) + +The current focus is the transcript draw hot path, specifically the cost of repeatedly rendering +the same visible transcript lines via Ratatui’s `Line::render_ref` (notably grapheme segmentation +and span layout). + +The intended mitigation is a **rasterization cache**: render a wrapped transcript `Line` into a +row of `Cell`s once, cache it, and on subsequent redraws copy cached cells into the frame buffer. + +Key invariants: + +- The cache is width-scoped (invalidate on terminal width changes). +- The cache stores **base content** only; selection highlight and copy affordances are applied + after rendering, so they don’t pollute cached rows. + +## Roles + +- Human: runs `codex-tui2` in an interactive terminal (e.g. Ghostty), triggers “idle” and + “streaming” scenarios, and captures profiles. +- Assistant (or a script): reads profile output and extracts hotspots and deltas. + +## Baseline setup + +Build from a clean checkout: + +```sh +cd codex-rs +cargo build -p codex-tui2 +``` + +Run `codex-tui2` in a terminal and get a PID (macOS): + +```sh +pgrep -n codex-tui2 +``` + +Track CPU quickly while reproducing: + +```sh +top -pid "$(pgrep -n codex-tui2)" +``` + +## Capture profiles (macOS) + +Capture both an “idle” and a “streaming” profile so hotspots are not conflated: + +```sh +sample "$(pgrep -n codex-tui2)" 1 -file /tmp/tui2.idle.sample.txt +sample "$(pgrep -n codex-tui2)" 1 -file /tmp/tui2.streaming.sample.txt +``` + +For the streaming sample, trigger a response that emits many deltas (e.g. “Tell me a story”) so +the stream runs long enough to sample. + +## Quick hotspot extraction + +These `rg` patterns keep the investigation grounded in the data: + +```sh +# Buffer diff hot path (idle) +rg -n "custom_terminal::diff_buffers|diff_buffers" /tmp/tui2.*.sample.txt | head -n 80 + +# Transcript rendering hot path (streaming) +rg -n "App::render_transcript_cells|Line::render|render_spans|styled_graphemes|GraphemeCursor::next_boundary" /tmp/tui2.*.sample.txt | head -n 120 +``` + +## Rasterization-cache validation checklist + +After implementing a transcript rasterization cache, re-run the same scenarios and confirm: + +- Streaming sample shifts away from `unicode_segmentation::grapheme::GraphemeCursor::next_boundary` + stacks dominating the main thread. +- CPU during streaming drops materially vs baseline for the same streaming load. +- Idle CPU does not regress (redraw gating changes can mask rendering improvements; always measure + both idle and streaming). + +## Notes to record per run + +- Terminal size: width × height +- Scenario: idle vs streaming (prompt + approximate response length) +- CPU snapshot: `top` (directional) +- Profile excerpt: 20–50 relevant lines for the dominant stacks + +## Code pointers + +- `codex-rs/tui2/src/transcript_view_cache.rs`: wrapped transcript memoization + per-line + rasterization cache (cached `Cell` rows). +- `codex-rs/tui2/src/transcript_render.rs`: incremental helper used by the wrapped-line cache + (`append_wrapped_transcript_cell`). +- `codex-rs/tui2/src/app.rs`: wiring in `App::render_transcript_cells` (uses cached rows instead of + calling `Line::render_ref` every frame). diff --git a/justfile b/justfile index 79b691e0a0..43ebae74aa 100644 --- a/justfile +++ b/justfile @@ -14,10 +14,6 @@ codex *args: exec *args: cargo run --bin codex -- exec "$@" -# `codex tui` -tui *args: - cargo run --bin codex -- tui "$@" - # Run the CLI version of the file-search crate. file-search *args: cargo run --bin codex-file-search -- "$@"