From 10ac2781eb7d83d6900686138242dfde12451453 Mon Sep 17 00:00:00 2001 From: Celia Chen Date: Fri, 22 May 2026 16:31:33 -0700 Subject: [PATCH 1/7] chore: add JSON schema policy fixture coverage (#24152) ## Why Before changing the Codex Bridge JSON schema policy, add integration coverage around real connector-like MCP tool schemas. The existing unit tests cover individual sanitizer behaviors, but they do not make it easy to see whether full fixture schemas keep model-visible guidance, prune only unreachable definitions, drop unsupported JSON Schema fields, and stay within the Responses API schema budget. ## What Changed - Added `tools/tests/json_schema_policy_fixtures.rs`, which converts MCP tool fixtures through `mcp_tool_to_responses_api_tool` and validates the resulting Responses tool parameters. - Added connector-style fixtures for Slack, Google Calendar, Google Drive, Notion, and Microsoft Outlook Email under `tools/tests/fixtures/json_schema_policy/`. - Added fixture assertions for preserved guidance, pruned definitions, expected field drops after `JsonSchema` conversion, marker count baselines, and dangling local `$ref` prevention. - Added a real oversized golden Notion `create_page` input schema fixture to exercise the compaction path that strips descriptions, drops root `$defs`, rewrites local refs, and fits the compacted schema under the budget. --- codex-rs/Cargo.lock | 1 + codex-rs/tools/Cargo.toml | 1 + .../json_schema_policy/google_calendar.json | 85 ++ .../json_schema_policy/google_drive.json | 65 + .../microsoft_outlook_email.json | 90 ++ .../fixtures/json_schema_policy/notion.json | 72 ++ ...sized_notion_create_page_input_schema.json | 1124 +++++++++++++++++ .../fixtures/json_schema_policy/slack.json | 75 ++ .../tests/json_schema_policy_fixtures.rs | 224 ++++ 9 files changed, 1737 insertions(+) create mode 100644 codex-rs/tools/tests/fixtures/json_schema_policy/google_calendar.json create mode 100644 codex-rs/tools/tests/fixtures/json_schema_policy/google_drive.json create mode 100644 codex-rs/tools/tests/fixtures/json_schema_policy/microsoft_outlook_email.json create mode 100644 codex-rs/tools/tests/fixtures/json_schema_policy/notion.json create mode 100644 codex-rs/tools/tests/fixtures/json_schema_policy/oversized_notion_create_page_input_schema.json create mode 100644 codex-rs/tools/tests/fixtures/json_schema_policy/slack.json create mode 100644 codex-rs/tools/tests/json_schema_policy_fixtures.rs diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index 779785704d..b843fcae6b 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -3770,6 +3770,7 @@ dependencies = [ "codex-features", "codex-protocol", "codex-utils-absolute-path", + "codex-utils-cargo-bin", "codex-utils-output-truncation", "codex-utils-pty", "codex-utils-string", diff --git a/codex-rs/tools/Cargo.toml b/codex-rs/tools/Cargo.toml index 7d02f1bf36..7cc0e34845 100644 --- a/codex-rs/tools/Cargo.toml +++ b/codex-rs/tools/Cargo.toml @@ -31,6 +31,7 @@ tracing = { workspace = true } urlencoding = { workspace = true } [dev-dependencies] +codex-utils-cargo-bin = { workspace = true } pretty_assertions = { workspace = true } [lib] diff --git a/codex-rs/tools/tests/fixtures/json_schema_policy/google_calendar.json b/codex-rs/tools/tests/fixtures/json_schema_policy/google_calendar.json new file mode 100644 index 0000000000..9673ecf596 --- /dev/null +++ b/codex-rs/tools/tests/fixtures/json_schema_policy/google_calendar.json @@ -0,0 +1,85 @@ +{ + "source": "standard/google_calendar/tools", + "tools": [ + { + "name": "google_calendar_create_event", + "description": "Create a calendar event.", + "input_schema": { + "type": "object", + "properties": { + "event": { + "$ref": "#/definitions/Event" + }, + "notify_attendees": { + "type": "boolean", + "description": "Whether attendees should receive notifications." + } + }, + "required": [ + "event" + ], + "definitions": { + "Event": { + "type": "object", + "description": "Calendar event payload.", + "properties": { + "title": { + "type": "string", + "description": "Event title." + }, + "start": { + "anyOf": [ + { + "$ref": "#/definitions/DateTime" + }, + { + "type": "null" + } + ] + } + } + }, + "DateTime": { + "type": "object", + "description": "Calendar date-time.", + "properties": { + "dateTime": { + "type": "string", + "format": "date-time", + "description": "RFC3339 date-time." + }, + "timeZone": { + "type": "string", + "enum": [ + "UTC", + "America/Los_Angeles" + ], + "description": "IANA time zone." + } + } + }, + "UnusedCalendarResource": { + "type": "string", + "description": "Unreachable calendar resource." + } + } + }, + "expected_preserved": [ + { + "pointer": "/properties/event/$ref", + "value": "#/definitions/Event" + }, + { + "pointer": "/definitions/DateTime/properties/timeZone/enum/1", + "value": "America/Los_Angeles" + } + ], + "expected_pruned": [ + "/definitions/UnusedCalendarResource" + ], + "expected_dropped_fields": [ + "/definitions/DateTime/properties/dateTime/format" + ] + } + ] +} diff --git a/codex-rs/tools/tests/fixtures/json_schema_policy/google_drive.json b/codex-rs/tools/tests/fixtures/json_schema_policy/google_drive.json new file mode 100644 index 0000000000..bf37296cc0 --- /dev/null +++ b/codex-rs/tools/tests/fixtures/json_schema_policy/google_drive.json @@ -0,0 +1,65 @@ +{ + "source": "standard/google_drive/tools", + "tools": [ + { + "name": "google_drive_copy_file", + "description": "Copy a Google Drive file.", + "input_schema": { + "type": "object", + "properties": { + "file": { + "description": "File selector.", + "oneOf": [ + { + "type": "string", + "description": "A Drive file ID." + }, + { + "type": "object", + "properties": { + "shared_drive_id": { + "type": "string", + "description": "Shared drive identifier." + } + } + } + ] + }, + "metadata": { + "description": "Optional copy metadata.", + "allOf": [ + { + "type": "object", + "description": "Base metadata.", + "properties": { + "source": { + "type": "string", + "description": "Metadata source." + } + } + } + ] + }, + "title": { + "type": "string", + "description": "Copied file title." + } + }, + "required": [ + "file" + ] + }, + "expected_preserved": [ + { + "pointer": "/properties/title/description", + "value": "Copied file title." + } + ], + "expected_pruned": [], + "expected_dropped_fields": [ + "/properties/file/oneOf", + "/properties/metadata/allOf" + ] + } + ] +} diff --git a/codex-rs/tools/tests/fixtures/json_schema_policy/microsoft_outlook_email.json b/codex-rs/tools/tests/fixtures/json_schema_policy/microsoft_outlook_email.json new file mode 100644 index 0000000000..bf0aff4118 --- /dev/null +++ b/codex-rs/tools/tests/fixtures/json_schema_policy/microsoft_outlook_email.json @@ -0,0 +1,90 @@ +{ + "source": "golden/microsoft_outlook_email/tools", + "tools": [ + { + "name": "microsoft_outlook_email_send", + "description": "Send an Outlook email.", + "input_schema": { + "type": "object", + "properties": { + "message_id": { + "$ref": "#/$defs/Message/properties/id" + }, + "attachment": { + "type": "array", + "items": { + "$ref": "#/$defs/Attachment" + } + } + }, + "$defs": { + "Message": { + "type": "object", + "description": "Outlook message.", + "properties": { + "id": { + "type": "string", + "description": "Message identifier." + }, + "sender": { + "$ref": "#/$defs/EmailAddress" + }, + "importance": { + "type": "string", + "enum": [ + "low", + "normal", + "high" + ], + "description": "Message importance." + } + } + }, + "Attachment": { + "type": "object", + "description": "Email attachment.", + "properties": { + "name": { + "type": "string", + "description": "Attachment file name." + } + } + }, + "EmailAddress": { + "type": "object", + "description": "Email address object.", + "properties": { + "address": { + "type": "string", + "description": "SMTP address." + } + } + }, + "GiantUnusedPayload": { + "type": "object", + "description": "Representative unreachable Outlook payload.", + "properties": { + "opaque": { + "type": "string" + } + } + } + } + }, + "expected_preserved": [ + { + "pointer": "/properties/message_id/$ref", + "value": "#/$defs/Message/properties/id" + }, + { + "pointer": "/$defs/Message/properties/importance/enum/2", + "value": "high" + } + ], + "expected_pruned": [ + "/$defs/GiantUnusedPayload" + ], + "expected_dropped_fields": [] + } + ] +} diff --git a/codex-rs/tools/tests/fixtures/json_schema_policy/notion.json b/codex-rs/tools/tests/fixtures/json_schema_policy/notion.json new file mode 100644 index 0000000000..49a362d49e --- /dev/null +++ b/codex-rs/tools/tests/fixtures/json_schema_policy/notion.json @@ -0,0 +1,72 @@ +{ + "source": "golden/notion/tools", + "tools": [ + { + "name": "notion_fetch_page", + "description": "Fetch a Notion page.", + "input_schema": { + "type": "object", + "properties": { + "page": { + "$ref": "#/$defs/Page%20Ref" + }, + "filter": { + "type": "object", + "additionalProperties": { + "$ref": "#/$defs/Filter" + } + }, + "content_mode": { + "description": "How much content to return.", + "anyOf": [ + { + "type": "string", + "enum": [ + "summary", + "full" + ] + }, + { + "type": "null" + } + ] + } + }, + "$defs": { + "Page Ref": { + "type": "string", + "description": "Notion page ID or URL." + }, + "Filter": { + "type": "object", + "description": "Filter object.", + "properties": { + "created_by": { + "type": "string", + "description": "Creator user ID." + } + } + }, + "UnusedDatabase": { + "type": "object", + "description": "Unreachable Notion database schema." + } + } + }, + "expected_preserved": [ + { + "pointer": "/properties/page/$ref", + "value": "#/$defs/Page%20Ref" + }, + { + "pointer": "/properties/content_mode/anyOf/0/enum/0", + "value": "summary" + } + ], + "expected_pruned": [ + "/$defs/UnusedDatabase" + ], + "expected_dropped_fields": [] + } + ] +} diff --git a/codex-rs/tools/tests/fixtures/json_schema_policy/oversized_notion_create_page_input_schema.json b/codex-rs/tools/tests/fixtures/json_schema_policy/oversized_notion_create_page_input_schema.json new file mode 100644 index 0000000000..b4957a1830 --- /dev/null +++ b/codex-rs/tools/tests/fixtures/json_schema_policy/oversized_notion_create_page_input_schema.json @@ -0,0 +1,1124 @@ +{ + "source": "golden/notion/tools", + "tools": [ + { + "name": "create_page", + "description": "Create a Notion page.", + "input_schema": { + "$schema": "https://json-schema.org/draft/2020-12/schema", + "title": "create_page.input", + "type": "object", + "description": "Input schema for create_page.", + "$defs": { + "rich_text": { + "type": "object", + "description": "Notion rich text object. This schema captures the stable high-value fields and permits provider extensions.", + "properties": { + "type": { + "type": "string", + "description": "Rich text discriminator such as `text`, `mention`, or `equation`." + }, + "plain_text": { + "type": "string", + "description": "Flattened plain-text representation." + }, + "href": { + "type": [ + "string", + "null" + ], + "description": "Optional hyperlink target when present." + }, + "annotations": { + "type": "object", + "description": "Text annotations applied to this run.", + "properties": { + "bold": { + "type": "boolean" + }, + "italic": { + "type": "boolean" + }, + "strikethrough": { + "type": "boolean" + }, + "underline": { + "type": "boolean" + }, + "code": { + "type": "boolean" + }, + "color": { + "type": "string" + } + }, + "additionalProperties": true + }, + "text": { + "type": "object", + "description": "Text payload for `type=text` entries.", + "properties": { + "content": { + "type": "string" + }, + "link": { + "anyOf": [ + { + "type": "null" + }, + { + "type": "object", + "properties": { + "url": { + "type": "string", + "format": "uri" + } + }, + "required": [ + "url" + ], + "additionalProperties": false + } + ] + } + }, + "additionalProperties": true + } + }, + "required": [ + "type" + ], + "additionalProperties": true + }, + "icon": { + "type": "object", + "description": "Notion icon union. Keep provider variants explicit and allow future variants.", + "properties": { + "type": { + "type": "string", + "description": "Icon discriminator such as `emoji`, `external`, `file_upload`, `custom_emoji`, or `icon`." + }, + "emoji": { + "type": "string", + "description": "Emoji character for `type=emoji`." + }, + "name": { + "type": "string", + "description": "Named native icon or custom emoji name when applicable." + }, + "color": { + "type": "string", + "description": "Optional icon color for native icon variants." + }, + "external": { + "type": "object", + "properties": { + "url": { + "type": "string", + "format": "uri" + } + }, + "additionalProperties": true + }, + "file_upload": { + "type": "object", + "properties": { + "id": { + "type": "string" + } + }, + "additionalProperties": true + }, + "custom_emoji": { + "type": "object", + "properties": { + "id": { + "type": "string" + }, + "name": { + "type": "string" + }, + "url": { + "type": "string", + "format": "uri" + } + }, + "additionalProperties": true + } + }, + "required": [ + "type" + ], + "additionalProperties": true + }, + "file_ref": { + "type": "object", + "description": "Notion file union used by cover and attachments.", + "properties": { + "type": { + "type": "string", + "description": "File discriminator such as `external`, `file`, or `file_upload`." + }, + "name": { + "type": "string" + }, + "external": { + "type": "object", + "properties": { + "url": { + "type": "string", + "format": "uri" + } + }, + "required": [ + "url" + ], + "additionalProperties": true + }, + "file": { + "type": "object", + "properties": { + "url": { + "type": "string", + "format": "uri" + }, + "expiry_time": { + "type": "string", + "format": "date-time" + } + }, + "additionalProperties": true + }, + "file_upload": { + "type": "object", + "properties": { + "id": { + "type": "string" + } + }, + "required": [ + "id" + ], + "additionalProperties": true + } + }, + "required": [ + "type" + ], + "additionalProperties": true + }, + "parent": { + "type": "object", + "description": "Notion parent reference. The exact allowed variants depend on the endpoint.", + "properties": { + "type": { + "type": "string", + "description": "Parent discriminator such as `page_id`, `data_source_id`, `database_id`, `block_id`, or `workspace`." + }, + "page_id": { + "type": "string" + }, + "data_source_id": { + "type": "string" + }, + "database_id": { + "type": "string" + }, + "block_id": { + "type": "string" + }, + "workspace": { + "type": "boolean" + } + }, + "required": [ + "type" + ], + "additionalProperties": true + }, + "date_value": { + "type": "object", + "properties": { + "start": { + "type": "string", + "description": "ISO 8601 date or datetime string." + }, + "end": { + "type": [ + "string", + "null" + ] + }, + "time_zone": { + "type": [ + "string", + "null" + ] + } + }, + "required": [ + "start" + ], + "additionalProperties": false + }, + "user": { + "type": "object", + "description": "Notion user or bot object.", + "properties": { + "object": { + "type": "string", + "const": "user" + }, + "id": { + "type": "string" + }, + "name": { + "type": [ + "string", + "null" + ] + }, + "avatar_url": { + "type": [ + "string", + "null" + ], + "format": "uri" + }, + "type": { + "type": "string", + "enum": [ + "person", + "bot" + ] + }, + "person": { + "type": "object", + "properties": { + "email": { + "type": [ + "string", + "null" + ], + "format": "email" + } + }, + "additionalProperties": true + }, + "bot": { + "type": "object", + "additionalProperties": true + } + }, + "required": [ + "object", + "id", + "type" + ], + "additionalProperties": true + }, + "property_value": { + "type": "object", + "description": "Provider-native page property value object. This schema captures common property families and leaves room for future Notion property variants.", + "properties": { + "type": { + "type": "string" + }, + "title": { + "type": "array", + "items": { + "$ref": "#/$defs/rich_text" + } + }, + "rich_text": { + "type": "array", + "items": { + "$ref": "#/$defs/rich_text" + } + }, + "number": { + "type": [ + "number", + "null" + ] + }, + "checkbox": { + "type": "boolean" + }, + "url": { + "type": [ + "string", + "null" + ], + "format": "uri" + }, + "email": { + "type": [ + "string", + "null" + ], + "format": "email" + }, + "phone_number": { + "type": [ + "string", + "null" + ] + }, + "date": { + "anyOf": [ + { + "$ref": "#/$defs/date_value" + }, + { + "type": "null" + } + ] + }, + "select": { + "anyOf": [ + { + "type": "null" + }, + { + "type": "object", + "properties": { + "id": { + "type": "string" + }, + "name": { + "type": "string" + }, + "color": { + "type": "string" + } + }, + "additionalProperties": true + } + ] + }, + "status": { + "anyOf": [ + { + "type": "null" + }, + { + "type": "object", + "properties": { + "id": { + "type": "string" + }, + "name": { + "type": "string" + }, + "color": { + "type": "string" + } + }, + "additionalProperties": true + } + ] + }, + "multi_select": { + "type": "array", + "items": { + "type": "object", + "properties": { + "id": { + "type": "string" + }, + "name": { + "type": "string" + }, + "color": { + "type": "string" + } + }, + "additionalProperties": true + } + }, + "people": { + "type": "array", + "items": { + "$ref": "#/$defs/user" + } + }, + "relation": { + "type": "array", + "items": { + "type": "object", + "properties": { + "id": { + "type": "string" + } + }, + "required": [ + "id" + ], + "additionalProperties": false + } + }, + "files": { + "type": "array", + "items": { + "$ref": "#/$defs/file_ref" + } + }, + "created_time": { + "type": "string", + "format": "date-time" + }, + "last_edited_time": { + "type": "string", + "format": "date-time" + }, + "formula": { + "type": "object", + "additionalProperties": true + }, + "rollup": { + "type": "object", + "additionalProperties": true + }, + "verification": { + "type": "object", + "additionalProperties": true + } + }, + "required": [ + "type" + ], + "additionalProperties": true + }, + "page": { + "type": "object", + "description": "Notion page object, modeled as a practical schema-authoring subset.", + "properties": { + "object": { + "type": "string", + "const": "page" + }, + "id": { + "type": "string" + }, + "created_time": { + "type": "string", + "format": "date-time" + }, + "last_edited_time": { + "type": "string", + "format": "date-time" + }, + "created_by": { + "$ref": "#/$defs/user" + }, + "last_edited_by": { + "$ref": "#/$defs/user" + }, + "cover": { + "anyOf": [ + { + "$ref": "#/$defs/file_ref" + }, + { + "type": "null" + } + ] + }, + "icon": { + "anyOf": [ + { + "$ref": "#/$defs/icon" + }, + { + "type": "null" + } + ] + }, + "parent": { + "$ref": "#/$defs/parent" + }, + "archived": { + "type": "boolean" + }, + "in_trash": { + "type": "boolean" + }, + "is_locked": { + "type": "boolean" + }, + "url": { + "type": "string", + "format": "uri" + }, + "properties": { + "type": "object", + "description": "Page property map keyed by property name.", + "additionalProperties": { + "$ref": "#/$defs/property_value" + } + } + }, + "required": [ + "object", + "id", + "parent", + "properties" + ], + "additionalProperties": true + }, + "text_block_payload": { + "type": "object", + "properties": { + "rich_text": { + "type": "array", + "items": { + "$ref": "#/$defs/rich_text" + } + }, + "color": { + "type": "string" + }, + "children": { + "type": "array", + "items": { + "$ref": "#/$defs/block" + } + } + }, + "additionalProperties": true + }, + "block": { + "type": "object", + "description": "Notion block object. The schema preserves the top-level discriminator and important typed payload families while allowing future provider expansion.", + "properties": { + "object": { + "type": "string", + "const": "block" + }, + "id": { + "type": "string" + }, + "type": { + "type": "string" + }, + "has_children": { + "type": "boolean" + }, + "archived": { + "type": "boolean" + }, + "in_trash": { + "type": "boolean" + }, + "created_time": { + "type": "string", + "format": "date-time" + }, + "last_edited_time": { + "type": "string", + "format": "date-time" + }, + "created_by": { + "$ref": "#/$defs/user" + }, + "last_edited_by": { + "$ref": "#/$defs/user" + }, + "parent": { + "$ref": "#/$defs/parent" + }, + "paragraph": { + "$ref": "#/$defs/text_block_payload" + }, + "heading_1": { + "$ref": "#/$defs/text_block_payload" + }, + "heading_2": { + "$ref": "#/$defs/text_block_payload" + }, + "heading_3": { + "$ref": "#/$defs/text_block_payload" + }, + "heading_4": { + "$ref": "#/$defs/text_block_payload" + }, + "bulleted_list_item": { + "$ref": "#/$defs/text_block_payload" + }, + "numbered_list_item": { + "$ref": "#/$defs/text_block_payload" + }, + "toggle": { + "$ref": "#/$defs/text_block_payload" + }, + "to_do": { + "allOf": [ + { + "$ref": "#/$defs/text_block_payload" + }, + { + "type": "object", + "properties": { + "checked": { + "type": "boolean" + } + }, + "additionalProperties": true + } + ] + }, + "callout": { + "allOf": [ + { + "$ref": "#/$defs/text_block_payload" + }, + { + "type": "object", + "properties": { + "icon": { + "anyOf": [ + { + "$ref": "#/$defs/icon" + }, + { + "type": "null" + } + ] + } + }, + "additionalProperties": true + } + ] + }, + "code": { + "type": "object", + "properties": { + "rich_text": { + "type": "array", + "items": { + "$ref": "#/$defs/rich_text" + } + }, + "caption": { + "type": "array", + "items": { + "$ref": "#/$defs/rich_text" + } + }, + "language": { + "type": "string" + } + }, + "additionalProperties": true + }, + "image": { + "type": "object", + "properties": { + "caption": { + "type": "array", + "items": { + "$ref": "#/$defs/rich_text" + } + }, + "type": { + "type": "string" + }, + "file": { + "$ref": "#/$defs/file_ref" + }, + "external": { + "type": "object", + "properties": { + "url": { + "type": "string", + "format": "uri" + } + }, + "additionalProperties": true + } + }, + "additionalProperties": true + }, + "tab": { + "type": "object", + "additionalProperties": true + }, + "meeting_notes": { + "type": "object", + "additionalProperties": true + }, + "unsupported": { + "type": "object", + "additionalProperties": true + } + }, + "required": [ + "object", + "id", + "type" + ], + "additionalProperties": true + }, + "property_schema": { + "type": "object", + "description": "Data source property schema entry.", + "properties": { + "id": { + "type": "string" + }, + "name": { + "type": "string" + }, + "description": { + "anyOf": [ + { + "type": "null" + }, + { + "type": "array", + "items": { + "$ref": "#/$defs/rich_text" + } + } + ] + }, + "type": { + "type": "string" + }, + "number": { + "type": "object", + "additionalProperties": true + }, + "select": { + "type": "object", + "additionalProperties": true + }, + "multi_select": { + "type": "object", + "additionalProperties": true + }, + "status": { + "type": "object", + "additionalProperties": true + }, + "date": { + "type": "object", + "additionalProperties": true + }, + "people": { + "type": "object", + "additionalProperties": true + }, + "files": { + "type": "object", + "additionalProperties": true + }, + "checkbox": { + "type": "object", + "additionalProperties": true + }, + "url": { + "type": "object", + "additionalProperties": true + }, + "email": { + "type": "object", + "additionalProperties": true + }, + "phone_number": { + "type": "object", + "additionalProperties": true + }, + "formula": { + "type": "object", + "additionalProperties": true + }, + "relation": { + "type": "object", + "additionalProperties": true + }, + "rollup": { + "type": "object", + "additionalProperties": true + }, + "created_time": { + "type": "object", + "additionalProperties": true + }, + "last_edited_time": { + "type": "object", + "additionalProperties": true + }, + "created_by": { + "type": "object", + "additionalProperties": true + }, + "last_edited_by": { + "type": "object", + "additionalProperties": true + }, + "verification": { + "type": "object", + "additionalProperties": true + } + }, + "required": [ + "type" + ], + "additionalProperties": true + }, + "data_source": { + "type": "object", + "description": "Notion data source object representing a table under a database.", + "properties": { + "object": { + "type": "string", + "const": "data_source" + }, + "id": { + "type": "string" + }, + "name": { + "type": [ + "string", + "null" + ] + }, + "title": { + "type": "array", + "items": { + "$ref": "#/$defs/rich_text" + } + }, + "icon": { + "anyOf": [ + { + "$ref": "#/$defs/icon" + }, + { + "type": "null" + } + ] + }, + "parent": { + "$ref": "#/$defs/parent" + }, + "url": { + "type": "string", + "format": "uri" + }, + "properties": { + "type": "object", + "description": "Schema map keyed by property name or id.", + "additionalProperties": { + "$ref": "#/$defs/property_schema" + } + }, + "in_trash": { + "type": "boolean" + } + }, + "required": [ + "object", + "id", + "parent" + ], + "additionalProperties": true + }, + "database": { + "type": "object", + "description": "Notion database container object.", + "properties": { + "object": { + "type": "string", + "const": "database" + }, + "id": { + "type": "string" + }, + "parent": { + "$ref": "#/$defs/parent" + }, + "title": { + "type": "array", + "items": { + "$ref": "#/$defs/rich_text" + } + }, + "description": { + "type": "array", + "items": { + "$ref": "#/$defs/rich_text" + } + }, + "icon": { + "anyOf": [ + { + "$ref": "#/$defs/icon" + }, + { + "type": "null" + } + ] + }, + "cover": { + "anyOf": [ + { + "$ref": "#/$defs/file_ref" + }, + { + "type": "null" + } + ] + }, + "is_inline": { + "type": "boolean" + }, + "is_locked": { + "type": "boolean" + }, + "in_trash": { + "type": "boolean" + }, + "url": { + "type": "string", + "format": "uri" + }, + "data_sources": { + "type": "array", + "items": { + "type": "object", + "properties": { + "id": { + "type": "string" + }, + "name": { + "type": [ + "string", + "null" + ] + } + }, + "required": [ + "id" + ], + "additionalProperties": true + } + } + }, + "required": [ + "object", + "id", + "parent" + ], + "additionalProperties": true + }, + "comment": { + "type": "object", + "description": "Notion comment object.", + "properties": { + "object": { + "type": "string", + "const": "comment" + }, + "id": { + "type": "string" + }, + "discussion_id": { + "type": "string" + }, + "created_time": { + "type": "string", + "format": "date-time" + }, + "last_edited_time": { + "type": [ + "string", + "null" + ], + "format": "date-time" + }, + "created_by": { + "$ref": "#/$defs/user" + }, + "parent": { + "$ref": "#/$defs/parent" + }, + "rich_text": { + "type": "array", + "items": { + "$ref": "#/$defs/rich_text" + } + }, + "markdown": { + "type": "string" + }, + "attachments": { + "type": "array", + "items": { + "$ref": "#/$defs/file_ref" + } + } + }, + "required": [ + "object", + "id", + "discussion_id", + "created_by" + ], + "additionalProperties": true + } + }, + "properties": { + "parent": { + "$ref": "#/$defs/parent", + "description": "Request body field. Parent reference controlling whether the page is standalone or a row under a data source." + }, + "properties": { + "type": "object", + "description": "Request body field. Page property map keyed by property name. Values should follow the target data source schema when the parent is a data source.", + "additionalProperties": { + "$ref": "#/$defs/property_value" + } + }, + "children": { + "type": "array", + "description": "Request body field. Initial child blocks. Prefer this structured provider-native content form over markdown when exact block structure matters.", + "items": { + "$ref": "#/$defs/block" + } + }, + "icon": { + "$ref": "#/$defs/icon", + "description": "Request body field. Optional page icon." + }, + "cover": { + "$ref": "#/$defs/file_ref", + "description": "Request body field. Optional page cover image/file reference." + }, + "template": { + "type": "object", + "description": "Request body field. Optional template application settings. Template application may be asynchronous.", + "additionalProperties": true + }, + "markdown": { + "type": "string", + "description": "Request body field. Optional provider-supported markdown body. Included because Notion supports it, but block children remain the preferred structural primitive in this package." + } + }, + "additionalProperties": false, + "required": [ + "parent", + "properties" + ], + "not": { + "required": [ + "children", + "markdown" + ] + } + } + } + ] +} diff --git a/codex-rs/tools/tests/fixtures/json_schema_policy/slack.json b/codex-rs/tools/tests/fixtures/json_schema_policy/slack.json new file mode 100644 index 0000000000..7461da73dd --- /dev/null +++ b/codex-rs/tools/tests/fixtures/json_schema_policy/slack.json @@ -0,0 +1,75 @@ +{ + "source": "standard/slack/tools", + "tools": [ + { + "name": "slack_schedule_message", + "description": "Schedule a Slack message.", + "input_schema": { + "type": "object", + "properties": { + "channel_id": { + "type": "string", + "description": "Slack channel ID." + }, + "message": { + "type": "string", + "description": "Message text." + }, + "post_at": { + "type": "integer", + "description": "Unix timestamp for delivery." + }, + "thread_ts": { + "description": "Optional parent thread timestamp.", + "anyOf": [ + { + "$ref": "#/$defs/SlackTs" + }, + { + "type": "null" + } + ] + } + }, + "required": [ + "channel_id", + "message", + "post_at" + ], + "additionalProperties": false, + "$defs": { + "SlackTs": { + "type": "string", + "description": "Slack timestamp string.", + "pattern": "^[0-9]+[.][0-9]+$" + }, + "UnusedPayload": { + "type": "object", + "description": "Large unreachable Slack payload.", + "properties": { + "debug": { + "type": "string" + } + } + } + } + }, + "expected_preserved": [ + { + "pointer": "/properties/thread_ts/anyOf/0/$ref", + "value": "#/$defs/SlackTs" + }, + { + "pointer": "/$defs/SlackTs/description", + "value": "Slack timestamp string." + } + ], + "expected_pruned": [ + "/$defs/UnusedPayload" + ], + "expected_dropped_fields": [ + "/$defs/SlackTs/pattern" + ] + } + ] +} diff --git a/codex-rs/tools/tests/json_schema_policy_fixtures.rs b/codex-rs/tools/tests/json_schema_policy_fixtures.rs new file mode 100644 index 0000000000..1e244ade1e --- /dev/null +++ b/codex-rs/tools/tests/json_schema_policy_fixtures.rs @@ -0,0 +1,224 @@ +use codex_tools::ToolName; +use codex_tools::mcp_tool_to_responses_api_tool; +use pretty_assertions::assert_eq; +use serde::Deserialize; +use serde::de::DeserializeOwned; +use serde_json::Value; +use serde_json::json; +use std::fs; +use std::sync::Arc; + +const FIXTURE_PATHS: [&str; 5] = [ + "tests/fixtures/json_schema_policy/slack.json", + "tests/fixtures/json_schema_policy/google_calendar.json", + "tests/fixtures/json_schema_policy/google_drive.json", + "tests/fixtures/json_schema_policy/notion.json", + "tests/fixtures/json_schema_policy/microsoft_outlook_email.json", +]; +const OVERSIZED_NOTION_CREATE_PAGE_SCHEMA_PATH: &str = + "tests/fixtures/json_schema_policy/oversized_notion_create_page_input_schema.json"; + +#[derive(Debug, Deserialize)] +struct FixtureFile { + source: String, + tools: Vec, +} + +#[derive(Debug, Deserialize)] +struct FixtureTool { + name: String, + description: String, + input_schema: Value, + #[serde(default)] + expected_preserved: Vec, + #[serde(default)] + expected_pruned: Vec, + #[serde(default)] + expected_dropped_fields: Vec, +} + +#[derive(Debug, Deserialize)] +struct ExpectedValue { + pointer: String, + value: Value, +} + +#[test] +fn json_schema_policy_fixtures_convert_to_responses_tools() { + for fixture in FIXTURE_PATHS.into_iter().map(load_fixture::) { + for fixture_tool in &fixture.tools { + let responses_tool = convert_fixture_tool(&fixture, fixture_tool); + let parameters = serde_json::to_value(&responses_tool.parameters) + .expect("responses parameters should serialize"); + + let expected_fields = [ + ( + "preserve the tool name", + json!(fixture_tool.name), + json!(responses_tool.name), + ), + ( + "preserve the tool description", + json!(fixture_tool.description), + json!(responses_tool.description), + ), + ( + "remain a strict:false tool", + json!(false), + json!(responses_tool.strict), + ), + ( + "produce object-shaped parameters", + json!("object"), + parameters.get("type").cloned().unwrap_or(Value::Null), + ), + ]; + + for (message, expected, actual) in expected_fields { + assert_eq!(actual, expected, "{} should {message}", fixture_tool.name); + } + assert!( + parameters.get("properties").is_some_and(Value::is_object), + "{} should produce a parameters.properties object", + fixture_tool.name + ); + + for expected in &fixture_tool.expected_preserved { + assert_eq!( + parameters.pointer(&expected.pointer), + Some(&expected.value), + "{} should preserve {}", + fixture_tool.name, + expected.pointer + ); + } + + for pointer in &fixture_tool.expected_pruned { + assert!( + parameters.pointer(pointer).is_none(), + "{} should prune unreachable definition {pointer}", + fixture_tool.name + ); + } + + for pointer in &fixture_tool.expected_dropped_fields { + assert!( + fixture_tool.input_schema.pointer(pointer).is_some(), + "{} fixture should contain expected dropped field {pointer}", + fixture_tool.name + ); + assert!( + parameters.pointer(pointer).is_none(), + "{} should drop field {pointer} after JsonSchema conversion", + fixture_tool.name + ); + } + } + } +} + +#[test] +fn json_schema_policy_oversized_golden_schema_triggers_compaction() { + let fixture: FixtureFile = load_fixture(OVERSIZED_NOTION_CREATE_PAGE_SCHEMA_PATH); + let fixture_tool = fixture + .tools + .first() + .expect("oversized fixture should contain a tool"); + let input_bytes = compact_json_len(&fixture_tool.input_schema); + + let responses_tool = convert_fixture_tool(&fixture, fixture_tool); + let parameters = + serde_json::to_value(&responses_tool.parameters).expect("responses parameters serialize"); + let output_bytes = compact_json_len(¶meters); + + assert!( + output_bytes < input_bytes, + "compaction should reduce schema size from {input_bytes} bytes" + ); + + let absent_pointers = [ + ("/description", "drop root description"), + ("/properties/parent/description", "drop nested descriptions"), + ( + "/$defs", + "drop root definitions after stripping descriptions is insufficient", + ), + ]; + for (pointer, message) in absent_pointers { + assert!( + parameters.pointer(pointer).is_none(), + "oversized schema should {message}" + ); + } + + let expected_values = [ + ( + "/properties/parent", + json!({}), + "rewrite local refs before dropping root definitions", + ), + ( + "/properties/children/items", + json!({}), + "rewrite nested local refs before dropping root definitions", + ), + ( + "/properties/markdown/type", + json!("string"), + "retain top-level argument shape", + ), + ( + "/properties/properties/type", + json!("object"), + "retain object argument shape", + ), + ]; + for (pointer, expected, message) in expected_values { + assert_eq!( + parameters.pointer(pointer), + Some(&expected), + "oversized schema should {message}" + ); + } +} + +fn load_fixture(path: &str) -> T { + let path = codex_utils_cargo_bin::find_resource!(path) + .unwrap_or_else(|err| panic!("resolve fixture {path}: {err}")); + let fixture = fs::read_to_string(&path) + .unwrap_or_else(|err| panic!("read fixture {}: {err}", path.display())); + serde_json::from_str(&fixture) + .unwrap_or_else(|err| panic!("parse fixture {}: {err}", path.display())) +} + +fn convert_fixture_tool( + fixture: &FixtureFile, + fixture_tool: &FixtureTool, +) -> codex_tools::ResponsesApiTool { + let name = &fixture_tool.name; + let input_schema = fixture_tool + .input_schema + .as_object() + .unwrap_or_else(|| panic!("{name} input_schema should be an object")) + .clone(); + let tool = rmcp::model::Tool { + name: name.to_string().into(), + title: None, + description: Some(fixture_tool.description.clone().into()), + input_schema: Arc::new(input_schema), + output_schema: None, + annotations: None, + execution: None, + icons: None, + meta: None, + }; + + mcp_tool_to_responses_api_tool(&ToolName::namespaced(&fixture.source, name), &tool) + .unwrap_or_else(|err| panic!("convert {name} from {}: {err}", fixture.source)) +} + +fn compact_json_len(value: &Value) -> usize { + serde_json::to_vec(value) + .unwrap_or_else(|err| panic!("serialize compact JSON: {err}")) + .len() +} From 6ad3a8350902f5825fe4172ef06cb5b506d10f3d Mon Sep 17 00:00:00 2001 From: pakrym-oai Date: Fri, 22 May 2026 16:46:25 -0700 Subject: [PATCH 2/7] [codex] Remove external client session reset plumbing (#24157) ## Why The turn loop no longer needs to decide when a `ModelClientSession` should reset its websocket state after compaction. That reset behavior belongs inside the model client, where the websocket cache and retry state are owned. The repo guidance now calls this out explicitly so future changes let the incremental request logic decide whether the previous request can be reused. ## What Changed - Removed the `reset_client_session` return value from pre-sampling and auto-compact helpers in `core/src/session/turn.rs`. - Changed compaction helpers to return `CodexResult<()>` so callers only handle success or failure. - Made `ModelClientSession::reset_websocket_session` private to `core/src/client.rs`, leaving it callable only from model-client internals. - Added `AGENTS.md` guidance not to call `reset_client_session` unnecessarily. ## Validation - `just test -p codex-core session::turn` --- AGENTS.md | 1 + codex-rs/core/src/client.rs | 2 +- codex-rs/core/src/session/turn.rs | 101 +++++++++++------------------- 3 files changed, 40 insertions(+), 64 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 9906d3039a..4714b1b8aa 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -30,6 +30,7 @@ In the codex-rs folder where the rust code lives: - Prefer private modules and explicitly exported public crate API. - If you change `ConfigToml` or nested config types, run `just write-config-schema` to update `codex-rs/core/config.schema.json`. - When working with MCP tool calls, prefer using `codex-rs/codex-mcp/src/mcp_connection_manager.rs` to handle mutation of tools and tool calls. Aim to minimize the footprint of changes and leverage existing abstractions rather than plumbing code through multiple levels of function calls. +- Do not call `reset_client_session` unnecessarily; let the incremental check logic decide whether to reuse the previous request. - If you change Rust dependencies (`Cargo.toml` or `Cargo.lock`), run `just bazel-lock-update` from the repo root to refresh `MODULE.bazel.lock`, and include that lockfile update in the same change. - After dependency changes, run `just bazel-lock-check` from the repo root so lockfile drift is caught diff --git a/codex-rs/core/src/client.rs b/codex-rs/core/src/client.rs index 07a1c2adf5..d84b32a8b4 100644 --- a/codex-rs/core/src/client.rs +++ b/codex-rs/core/src/client.rs @@ -938,7 +938,7 @@ impl Drop for ModelClientSession { } impl ModelClientSession { - pub(crate) fn reset_websocket_session(&mut self) { + fn reset_websocket_session(&mut self) { self.websocket_session.connection = None; self.websocket_session.last_request = None; self.websocket_session.last_response_rx = None; diff --git a/codex-rs/core/src/session/turn.rs b/codex-rs/core/src/session/turn.rs index f81b2b1f74..5a2ed71dda 100644 --- a/codex-rs/core/src/session/turn.rs +++ b/codex-rs/core/src/session/turn.rs @@ -142,25 +142,18 @@ pub(crate) async fn run_turn( // new user message are recorded. Estimate pending incoming items (context // diffs/full reinjection + user input) and trigger compaction preemptively // when they would push the thread over the compaction threshold. - let pre_sampling_compact = - match run_pre_sampling_compact(&sess, &turn_context, &mut client_session).await { - Ok(pre_sampling_compact) => pre_sampling_compact, - Err(err) => { - if err.to_codex_protocol_error() == CodexErrorInfo::UsageLimitExceeded - && let Err(err) = sess - .goal_runtime_apply(GoalRuntimeEvent::UsageLimitReached { - turn_context: turn_context.as_ref(), - }) - .await - { - warn!("failed to usage-limit active goal after usage-limit error: {err}"); - } - error!("Failed to run pre-sampling compact"); - return None; - } - }; - if pre_sampling_compact.reset_client_session { - client_session.reset_websocket_session(); + if let Err(err) = run_pre_sampling_compact(&sess, &turn_context, &mut client_session).await { + if err.to_codex_protocol_error() == CodexErrorInfo::UsageLimitExceeded + && let Err(err) = sess + .goal_runtime_apply(GoalRuntimeEvent::UsageLimitReached { + turn_context: turn_context.as_ref(), + }) + .await + { + warn!("failed to usage-limit active goal after usage-limit error: {err}"); + } + error!("Failed to run pre-sampling compact"); + return None; } sess.record_context_updates_and_set_reference_context_item(turn_context.as_ref()) @@ -287,7 +280,7 @@ pub(crate) async fn run_turn( // as long as compaction works well in getting us way below the token limit, we shouldn't worry about being in an infinite loop. if token_limit_reached && needs_follow_up { - let reset_client_session = match run_auto_compact( + if let Err(err) = run_auto_compact( &sess, &turn_context, &mut client_session, @@ -297,24 +290,18 @@ pub(crate) async fn run_turn( ) .await { - Ok(reset_client_session) => reset_client_session, - Err(err) => { - if err.to_codex_protocol_error() == CodexErrorInfo::UsageLimitExceeded - && let Err(err) = sess - .goal_runtime_apply(GoalRuntimeEvent::UsageLimitReached { - turn_context: turn_context.as_ref(), - }) - .await - { - warn!( - "failed to usage-limit active goal after usage-limit error: {err}" - ); - } - return None; + if err.to_codex_protocol_error() == CodexErrorInfo::UsageLimitExceeded + && let Err(err) = sess + .goal_runtime_apply(GoalRuntimeEvent::UsageLimitReached { + turn_context: turn_context.as_ref(), + }) + .await + { + warn!( + "failed to usage-limit active goal after usage-limit error: {err}" + ); } - }; - if reset_client_session { - client_session.reset_websocket_session(); + return None; } can_drain_pending_input = !model_needs_follow_up; continue; @@ -636,10 +623,6 @@ async fn track_turn_resolved_config_analytics( }); } -struct PreSamplingCompactResult { - reset_client_session: bool, -} - #[derive(Debug)] struct AutoCompactTokenStatus { // Full active context usage, independent of the configured auto-compact scope. @@ -710,14 +693,12 @@ async fn run_pre_sampling_compact( sess: &Arc, turn_context: &Arc, client_session: &mut ModelClientSession, -) -> CodexResult { - let mut pre_sampling_compacted = - maybe_run_previous_model_inline_compact(sess, turn_context, client_session).await?; - let mut reset_client_session = pre_sampling_compacted; +) -> CodexResult<()> { + maybe_run_previous_model_inline_compact(sess, turn_context, client_session).await?; let token_status = auto_compact_token_status(sess.as_ref(), turn_context.as_ref()).await; // Compact if the configured auto-compaction budget or usable context window is exhausted. if token_status.token_limit_reached { - reset_client_session |= run_auto_compact( + run_auto_compact( sess, turn_context, client_session, @@ -726,26 +707,21 @@ async fn run_pre_sampling_compact( CompactionPhase::PreTurn, ) .await?; - pre_sampling_compacted = true; } - Ok(PreSamplingCompactResult { - reset_client_session: pre_sampling_compacted && reset_client_session, - }) + Ok(()) } /// Runs pre-sampling compaction against the previous model when switching to a smaller /// context-window model. /// -/// Returns `Ok(true)` when compaction ran successfully, `Ok(false)` when compaction was skipped -/// because the model/context-window preconditions were not met, and `Err(_)` only when compaction -/// was attempted and failed. +/// Returns `Err(_)` only when compaction was attempted and failed. async fn maybe_run_previous_model_inline_compact( sess: &Arc, turn_context: &Arc, client_session: &mut ModelClientSession, -) -> CodexResult { +) -> CodexResult<()> { let Some(previous_turn_settings) = sess.previous_turn_settings().await else { - return Ok(false); + return Ok(()); }; let previous_model_turn_context = Arc::new( turn_context @@ -754,10 +730,10 @@ async fn maybe_run_previous_model_inline_compact( ); let Some(old_context_window) = previous_model_turn_context.model_context_window() else { - return Ok(false); + return Ok(()); }; let Some(new_context_window) = turn_context.model_context_window() else { - return Ok(false); + return Ok(()); }; let active_context_tokens = sess.get_total_token_usage().await; let previous_model_limit_reached = match turn_context @@ -778,7 +754,7 @@ async fn maybe_run_previous_model_inline_compact( && previous_model_turn_context.model_info.slug != turn_context.model_info.slug && old_context_window > new_context_window; if should_run { - let _ = run_auto_compact( + run_auto_compact( sess, &previous_model_turn_context, client_session, @@ -787,9 +763,8 @@ async fn maybe_run_previous_model_inline_compact( CompactionPhase::PreTurn, ) .await?; - return Ok(true); } - Ok(false) + Ok(()) } async fn run_auto_compact( @@ -799,7 +774,7 @@ async fn run_auto_compact( initial_context_injection: InitialContextInjection, reason: CompactionReason, phase: CompactionPhase, -) -> CodexResult { +) -> CodexResult<()> { if should_use_remote_compact_task(turn_context.provider.info()) { if turn_context.features.enabled(Feature::RemoteCompactionV2) { run_inline_remote_auto_compact_task_v2( @@ -811,7 +786,7 @@ async fn run_auto_compact( phase, ) .await?; - return Ok(false); + return Ok(()); } run_inline_remote_auto_compact_task( Arc::clone(sess), @@ -831,7 +806,7 @@ async fn run_auto_compact( ) .await?; } - Ok(true) + Ok(()) } pub(super) fn collect_explicit_app_ids_from_skill_items( From 4bcabbfbec668de26bdb2b1c39f4748c419962ff Mon Sep 17 00:00:00 2001 From: dhruvgupta-oai Date: Fri, 22 May 2026 19:58:49 -0400 Subject: [PATCH 3/7] Display workspace usage limit error copy from response header (#24114) ## Why `openai/openai#947613` adds `X-Codex-Rate-Limit-Reached-Type` for Codex workspace credit-depletion and spend-cap responses. The CLI currently reads the adjacent promo header but otherwise renders generic usage-limit copy, so those responses do not explain the workspace-specific action the user needs to take. Backend dependency: https://github.com/openai/openai/pull/947613 ## What Changed - Parse `X-Codex-Rate-Limit-Reached-Type` in the usage-limit error handling path alongside `x-codex-promo-message`. - Keep the header value parsing with the shared `RateLimitReachedType` enum. - Carry the parsed type on `UsageLimitReachedError` and render client-owned copy for the four workspace owner/member credit and spend-cap values. - Preserve existing promo and plan-based text for absent, generic, or unknown header values. - Keep the existing TUI workspace-owner nudge state path unchanged; the response header only selects the displayed error string. - Add focused display coverage for all specific type values and the generic fallback case. ## Test Plan - Added `usage_limit_reached_error_formats_rate_limit_reached_types` coverage. - Not run manually, per request; CI runs validation on the pushed commit. --- codex-rs/codex-api/src/api_bridge.rs | 4 ++ codex-rs/codex-api/src/api_bridge_tests.rs | 31 +++++++++++++ codex-rs/codex-api/src/rate_limits.rs | 8 ++++ codex-rs/protocol/src/error.rs | 34 ++++++++++++++ codex-rs/protocol/src/error_tests.rs | 54 ++++++++++++++++++++++ codex-rs/protocol/src/protocol.rs | 15 ++++++ 6 files changed, 146 insertions(+) diff --git a/codex-rs/codex-api/src/api_bridge.rs b/codex-rs/codex-api/src/api_bridge.rs index 401dfab3a9..1c34d8bbf2 100644 --- a/codex-rs/codex-api/src/api_bridge.rs +++ b/codex-rs/codex-api/src/api_bridge.rs @@ -2,6 +2,7 @@ use crate::TransportError; use crate::error::ApiError; use crate::rate_limits::parse_promo_message; use crate::rate_limits::parse_rate_limit_for_limit; +use crate::rate_limits::parse_rate_limit_reached_type; use base64::Engine; use chrono::DateTime; use chrono::Utc; @@ -85,6 +86,8 @@ pub fn map_api_error(err: ApiError) -> CodexErr { parse_rate_limit_for_limit(map, limit_id.as_deref()) }); let promo_message = headers.as_ref().and_then(parse_promo_message); + let rate_limit_reached_type = + headers.as_ref().and_then(parse_rate_limit_reached_type); let resets_at = err .error .resets_at @@ -94,6 +97,7 @@ pub fn map_api_error(err: ApiError) -> CodexErr { resets_at, rate_limits: rate_limits.map(Box::new), promo_message, + rate_limit_reached_type, }); } else if err.error.error_type.as_deref() == Some("usage_not_included") { return CodexErr::UsageNotIncluded; diff --git a/codex-rs/codex-api/src/api_bridge_tests.rs b/codex-rs/codex-api/src/api_bridge_tests.rs index af7e34a649..101e5566fe 100644 --- a/codex-rs/codex-api/src/api_bridge_tests.rs +++ b/codex-rs/codex-api/src/api_bridge_tests.rs @@ -194,6 +194,37 @@ fn map_api_error_does_not_fallback_limit_name_to_limit_id() { ); } +#[test] +fn map_api_error_ignores_unparseable_rate_limit_reached_type_headers() { + let values = [ + http::HeaderValue::from_static("future_rate_limit_reached_type"), + http::HeaderValue::from_bytes(&[0xff]).expect("valid opaque header value"), + ]; + + for value in values { + let mut headers = HeaderMap::new(); + headers.insert("x-codex-rate-limit-reached-type", value); + let body = serde_json::json!({ + "error": { + "type": "usage_limit_reached", + "plan_type": "pro", + } + }) + .to_string(); + let err = map_api_error(ApiError::Transport(TransportError::Http { + status: http::StatusCode::TOO_MANY_REQUESTS, + url: Some("http://example.com/v1/responses".to_string()), + headers: Some(headers), + body: Some(body), + })); + + let CodexErr::UsageLimitReached(usage_limit) = err else { + panic!("expected CodexErr::UsageLimitReached, got {err:?}"); + }; + assert_eq!(usage_limit.rate_limit_reached_type, None); + } +} + #[test] fn map_api_error_extracts_identity_auth_details_from_headers() { let mut headers = HeaderMap::new(); diff --git a/codex-rs/codex-api/src/rate_limits.rs b/codex-rs/codex-api/src/rate_limits.rs index 979500cdab..a2ad876671 100644 --- a/codex-rs/codex-api/src/rate_limits.rs +++ b/codex-rs/codex-api/src/rate_limits.rs @@ -1,5 +1,6 @@ use codex_protocol::account::PlanType; use codex_protocol::protocol::CreditsSnapshot; +use codex_protocol::protocol::RateLimitReachedType; use codex_protocol::protocol::RateLimitSnapshot; use codex_protocol::protocol::RateLimitWindow; use http::HeaderMap; @@ -178,6 +179,13 @@ pub fn parse_promo_message(headers: &HeaderMap) -> Option { .map(std::string::ToString::to_string) } +pub(crate) fn parse_rate_limit_reached_type(headers: &HeaderMap) -> Option { + parse_header_str(headers, "x-codex-rate-limit-reached-type")? + .trim() + .parse() + .ok() +} + fn parse_rate_limit_window( headers: &HeaderMap, used_percent_header: &str, diff --git a/codex-rs/protocol/src/error.rs b/codex-rs/protocol/src/error.rs index ef9c86cadf..d7e953af0b 100644 --- a/codex-rs/protocol/src/error.rs +++ b/codex-rs/protocol/src/error.rs @@ -7,6 +7,7 @@ use crate::exec_output::ExecToolCallOutput; use crate::network_policy::NetworkPolicyDecisionPayload; use crate::protocol::CodexErrorInfo; use crate::protocol::ErrorEvent; +use crate::protocol::RateLimitReachedType; use crate::protocol::RateLimitSnapshot; use crate::protocol::TruncationPolicy; use chrono::DateTime; @@ -451,6 +452,7 @@ pub struct UsageLimitReachedError { pub resets_at: Option>, pub rate_limits: Option>, pub promo_message: Option, + pub rate_limit_reached_type: Option, } impl std::fmt::Display for UsageLimitReachedError { @@ -470,6 +472,38 @@ impl std::fmt::Display for UsageLimitReachedError { ); } + if let Some(rate_limit_reached_type) = self.rate_limit_reached_type { + match rate_limit_reached_type { + RateLimitReachedType::WorkspaceOwnerCreditsDepleted => { + return write!( + f, + "Your workspace is out of credits. Add credits to continue." + ); + } + RateLimitReachedType::WorkspaceMemberCreditsDepleted => { + return write!( + f, + "Your workspace is out of credits. Ask your workspace owner to refill in order to continue." + ); + } + RateLimitReachedType::WorkspaceOwnerUsageLimitReached => { + return write!( + f, + "You hit your spend cap set in your workspace. Increase your spend cap to continue." + ); + } + RateLimitReachedType::WorkspaceMemberUsageLimitReached => { + return write!( + f, + "You hit your spend cap set by the owner of your workspace. Ask an owner to increase your spend cap to continue." + ); + } + RateLimitReachedType::RateLimitReached => { + // Generic limits intentionally use the existing promo or plan copy below. + } + } + } + if let Some(promo_message) = &self.promo_message { return write!( f, diff --git a/codex-rs/protocol/src/error_tests.rs b/codex-rs/protocol/src/error_tests.rs index aef7478607..11bd26133b 100644 --- a/codex-rs/protocol/src/error_tests.rs +++ b/codex-rs/protocol/src/error_tests.rs @@ -56,6 +56,7 @@ fn usage_limit_reached_error_formats_plus_plan() { resets_at: None, rate_limits: Some(Box::new(rate_limit_snapshot())), promo_message: None, + rate_limit_reached_type: None, }; assert_eq!( err.to_string(), @@ -63,6 +64,44 @@ fn usage_limit_reached_error_formats_plus_plan() { ); } +#[test] +fn usage_limit_reached_error_formats_rate_limit_reached_types() { + let cases = [ + ( + RateLimitReachedType::RateLimitReached, + "You've hit your usage limit. Upgrade to Pro (https://chatgpt.com/explore/pro), visit https://chatgpt.com/codex/settings/usage to purchase more credits or try again later.", + ), + ( + RateLimitReachedType::WorkspaceOwnerCreditsDepleted, + "Your workspace is out of credits. Add credits to continue.", + ), + ( + RateLimitReachedType::WorkspaceMemberCreditsDepleted, + "Your workspace is out of credits. Ask your workspace owner to refill in order to continue.", + ), + ( + RateLimitReachedType::WorkspaceOwnerUsageLimitReached, + "You hit your spend cap set in your workspace. Increase your spend cap to continue.", + ), + ( + RateLimitReachedType::WorkspaceMemberUsageLimitReached, + "You hit your spend cap set by the owner of your workspace. Ask an owner to increase your spend cap to continue.", + ), + ]; + + for (rate_limit_reached_type, expected) in cases { + let err = UsageLimitReachedError { + plan_type: Some(PlanType::Known(KnownPlan::Plus)), + resets_at: None, + rate_limits: Some(Box::new(rate_limit_snapshot())), + promo_message: None, + rate_limit_reached_type: Some(rate_limit_reached_type), + }; + + assert_eq!(err.to_string(), expected); + } +} + #[test] fn server_overloaded_maps_to_protocol() { let err = CodexErr::ServerOverloaded; @@ -177,6 +216,7 @@ fn usage_limit_reached_error_formats_free_plan() { resets_at: None, rate_limits: Some(Box::new(rate_limit_snapshot())), promo_message: None, + rate_limit_reached_type: None, }; assert_eq!( err.to_string(), @@ -191,6 +231,7 @@ fn usage_limit_reached_error_formats_go_plan() { resets_at: None, rate_limits: Some(Box::new(rate_limit_snapshot())), promo_message: None, + rate_limit_reached_type: None, }; assert_eq!( err.to_string(), @@ -205,6 +246,7 @@ fn usage_limit_reached_error_formats_default_when_none() { resets_at: None, rate_limits: Some(Box::new(rate_limit_snapshot())), promo_message: None, + rate_limit_reached_type: None, }; assert_eq!( err.to_string(), @@ -223,6 +265,7 @@ fn usage_limit_reached_error_formats_team_plan() { resets_at: Some(resets_at), rate_limits: Some(Box::new(rate_limit_snapshot())), promo_message: None, + rate_limit_reached_type: None, }; let expected = format!( "You've hit your usage limit. To get more access now, send a request to your admin or try again at {expected_time}." @@ -238,6 +281,7 @@ fn usage_limit_reached_error_formats_business_plan_without_reset() { resets_at: None, rate_limits: Some(Box::new(rate_limit_snapshot())), promo_message: None, + rate_limit_reached_type: None, }; assert_eq!( err.to_string(), @@ -252,6 +296,7 @@ fn usage_limit_reached_error_formats_self_serve_business_usage_based_plan() { resets_at: None, rate_limits: Some(Box::new(rate_limit_snapshot())), promo_message: None, + rate_limit_reached_type: None, }; assert_eq!( err.to_string(), @@ -266,6 +311,7 @@ fn usage_limit_reached_error_formats_enterprise_cbp_usage_based_plan() { resets_at: None, rate_limits: Some(Box::new(rate_limit_snapshot())), promo_message: None, + rate_limit_reached_type: None, }; assert_eq!( err.to_string(), @@ -280,6 +326,7 @@ fn usage_limit_reached_error_formats_default_for_other_plans() { resets_at: None, rate_limits: Some(Box::new(rate_limit_snapshot())), promo_message: None, + rate_limit_reached_type: None, }; assert_eq!( err.to_string(), @@ -298,6 +345,7 @@ fn usage_limit_reached_error_formats_pro_plan_with_reset() { resets_at: Some(resets_at), rate_limits: Some(Box::new(rate_limit_snapshot())), promo_message: None, + rate_limit_reached_type: None, }; let expected = format!( "You've hit your usage limit. Visit https://chatgpt.com/codex/settings/usage to purchase more credits or try again at {expected_time}." @@ -324,6 +372,7 @@ fn usage_limit_reached_error_hides_upsell_for_non_codex_limit_name() { "Visit https://chatgpt.com/codex/settings/usage to purchase more credits" .to_string(), ), + rate_limit_reached_type: None, }; let expected = format!( "You've hit your usage limit for codex_other. Switch to another model now, or try again at {expected_time}." @@ -343,6 +392,7 @@ fn usage_limit_reached_includes_minutes_when_available() { resets_at: Some(resets_at), rate_limits: Some(Box::new(rate_limit_snapshot())), promo_message: None, + rate_limit_reached_type: None, }; let expected = format!("You've hit your usage limit. Try again at {expected_time}."); assert_eq!(err.to_string(), expected); @@ -482,6 +532,7 @@ fn usage_limit_reached_includes_hours_and_minutes() { resets_at: Some(resets_at), rate_limits: Some(Box::new(rate_limit_snapshot())), promo_message: None, + rate_limit_reached_type: None, }; let expected = format!( "You've hit your usage limit. Upgrade to Pro (https://chatgpt.com/explore/pro), visit https://chatgpt.com/codex/settings/usage to purchase more credits or try again at {expected_time}." @@ -502,6 +553,7 @@ fn usage_limit_reached_includes_days_hours_minutes() { resets_at: Some(resets_at), rate_limits: Some(Box::new(rate_limit_snapshot())), promo_message: None, + rate_limit_reached_type: None, }; let expected = format!("You've hit your usage limit. Try again at {expected_time}."); assert_eq!(err.to_string(), expected); @@ -519,6 +571,7 @@ fn usage_limit_reached_less_than_minute() { resets_at: Some(resets_at), rate_limits: Some(Box::new(rate_limit_snapshot())), promo_message: None, + rate_limit_reached_type: None, }; let expected = format!("You've hit your usage limit. Try again at {expected_time}."); assert_eq!(err.to_string(), expected); @@ -538,6 +591,7 @@ fn usage_limit_reached_with_promo_message() { promo_message: Some( "To continue using Codex, start a free trial of today".to_string(), ), + rate_limit_reached_type: None, }; let expected = format!( "You've hit your usage limit. To continue using Codex, start a free trial of today, or try again at {expected_time}." diff --git a/codex-rs/protocol/src/protocol.rs b/codex-rs/protocol/src/protocol.rs index 7d955f1d1f..d3b8a9e820 100644 --- a/codex-rs/protocol/src/protocol.rs +++ b/codex-rs/protocol/src/protocol.rs @@ -2000,6 +2000,21 @@ pub enum RateLimitReachedType { WorkspaceMemberUsageLimitReached, } +impl FromStr for RateLimitReachedType { + type Err = String; + + fn from_str(value: &str) -> Result { + match value { + "rate_limit_reached" => Ok(Self::RateLimitReached), + "workspace_owner_credits_depleted" => Ok(Self::WorkspaceOwnerCreditsDepleted), + "workspace_member_credits_depleted" => Ok(Self::WorkspaceMemberCreditsDepleted), + "workspace_owner_usage_limit_reached" => Ok(Self::WorkspaceOwnerUsageLimitReached), + "workspace_member_usage_limit_reached" => Ok(Self::WorkspaceMemberUsageLimitReached), + other => Err(format!("unknown rate limit reached type: {other}")), + } + } +} + #[derive(Debug, Clone, PartialEq, Deserialize, Serialize, JsonSchema, TS)] pub struct RateLimitWindow { /// Percentage (0-100) of the window that has been consumed. From ed47f1ab1eba595df81e7ce62568b83328d9c42a Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Fri, 22 May 2026 17:08:59 -0700 Subject: [PATCH 4/7] release: build macOS x64 zsh artifact (#24165) ## Why The zsh release workflow currently publishes macOS arm64 and Linux zsh fork artifacts, but no macOS x64 artifact. The Codex package builder therefore cannot include codex-resources/zsh/bin/zsh for x86_64-apple-darwin packages. ## What Changed - Added an x86_64-apple-darwin row to the macOS zsh release matrix. - Runs that row on macos-15-large, the Intel macOS runner appropriate for the native zsh build. - Added the matching macos-x86_64 platform to the zsh DotSlash publish config so the generated release manifest can reference the new tarball. --- .github/dotslash-zsh-config.json | 5 +++++ .github/workflows/rust-release-zsh.yml | 4 ++++ 2 files changed, 9 insertions(+) diff --git a/.github/dotslash-zsh-config.json b/.github/dotslash-zsh-config.json index db2c416401..37285f19e2 100644 --- a/.github/dotslash-zsh-config.json +++ b/.github/dotslash-zsh-config.json @@ -7,6 +7,11 @@ "format": "tar.gz", "path": "codex-zsh/bin/zsh" }, + "macos-x86_64": { + "name": "codex-zsh-x86_64-apple-darwin.tar.gz", + "format": "tar.gz", + "path": "codex-zsh/bin/zsh" + }, "linux-x86_64": { "name": "codex-zsh-x86_64-unknown-linux-musl.tar.gz", "format": "tar.gz", diff --git a/.github/workflows/rust-release-zsh.yml b/.github/workflows/rust-release-zsh.yml index 492b8dc5e7..b55d2e714b 100644 --- a/.github/workflows/rust-release-zsh.yml +++ b/.github/workflows/rust-release-zsh.yml @@ -69,6 +69,10 @@ jobs: fail-fast: false matrix: include: + - runner: macos-15-large + target: x86_64-apple-darwin + variant: macos-15 + archive_name: codex-zsh-x86_64-apple-darwin.tar.gz - runner: macos-15-xlarge target: aarch64-apple-darwin variant: macos-15 From 03e6c5f60048b81f443f3573b1578320c2189fbc Mon Sep 17 00:00:00 2001 From: Anton Panasenko Date: Fri, 22 May 2026 17:38:22 -0700 Subject: [PATCH 5/7] fix(remote-control): cap reconnect backoff (#24164) ## Why Remote-control websocket reconnects currently use the shared exponential backoff helper without a local ceiling, so a long failure streak can stretch retries out indefinitely and leave the runtime behavior hard to inspect from logs. ## What Changed Cap the remote-control reconnect delay at 30 seconds, then reset the reconnect attempt counter once that capped delay is emitted so the next failure starts from the initial jittered delay again. The reconnect failure log now records the attempt number, chosen delay, and whether the cap triggered a reset, with a separate info log when the backoff counter is reset after the cap. ## Verification `just test -p codex-app-server-transport` Related issue: N/A --- .../src/transport/remote_control/websocket.rs | 52 +++++++++++++++++-- 1 file changed, 48 insertions(+), 4 deletions(-) diff --git a/codex-rs/app-server-transport/src/transport/remote_control/websocket.rs b/codex-rs/app-server-transport/src/transport/remote_control/websocket.rs index 74ef9c2301..1e696c2e46 100644 --- a/codex-rs/app-server-transport/src/transport/remote_control/websocket.rs +++ b/codex-rs/app-server-transport/src/transport/remote_control/websocket.rs @@ -64,6 +64,8 @@ const REMOTE_CONTROL_WEBSOCKET_PONG_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(60); const REMOTE_CONTROL_ACCOUNT_ID_RETRY_INTERVAL: std::time::Duration = std::time::Duration::from_secs(1); +const REMOTE_CONTROL_RECONNECT_BACKOFF_CAP: std::time::Duration = + std::time::Duration::from_secs(30); struct BoundedOutboundBuffer { buffer_by_stream: HashMap<(ClientId, StreamId), VecDeque>, @@ -514,12 +516,23 @@ impl RemoteControlWebsocket { } else { self.status_publisher .publish_status(RemoteControlConnectionStatus::Errored); + let reconnect_attempt = self.reconnect_attempt.saturating_add(1); + let (reconnect_delay, reconnect_backoff_reset) = + next_reconnect_delay(&mut self.reconnect_attempt); warn!( - "failed to connect to app-server remote control websocket: {}, err: {}", - remote_control_target.websocket_url, err + websocket_url = %remote_control_target.websocket_url, + error = %err, + reconnect_attempt, + reconnect_delay = ?reconnect_delay, + reconnect_backoff_reset, + "failed to connect to app-server remote control websocket" ); - let reconnect_delay = backoff(self.reconnect_attempt); - self.reconnect_attempt += 1; + if reconnect_backoff_reset { + info!( + reconnect_backoff_cap = ?REMOTE_CONTROL_RECONNECT_BACKOFF_CAP, + "reset app-server remote control websocket reconnect backoff after cap" + ); + } reconnect_delay }; tokio::select! { @@ -1036,6 +1049,17 @@ pub(crate) async fn load_remote_control_auth( }) } +fn next_reconnect_delay(reconnect_attempt: &mut u64) -> (std::time::Duration, bool) { + let reconnect_delay = backoff(*reconnect_attempt).min(REMOTE_CONTROL_RECONNECT_BACKOFF_CAP); + let reconnect_backoff_reset = reconnect_delay == REMOTE_CONTROL_RECONNECT_BACKOFF_CAP; + *reconnect_attempt = if reconnect_backoff_reset { + 0 + } else { + (*reconnect_attempt).saturating_add(1) + }; + (reconnect_delay, reconnect_backoff_reset) +} + pub(super) async fn connect_remote_control_websocket( remote_control_target: &RemoteControlTarget, state_db: Option<&StateRuntime>, @@ -1326,6 +1350,26 @@ mod tests { const TEST_HTTP_ACCEPT_TIMEOUT: Duration = Duration::from_secs(5); const TEST_INSTALLATION_ID: &str = "11111111-1111-4111-8111-111111111111"; + #[test] + fn next_reconnect_delay_resets_after_cap() { + let mut reconnect_attempt = 9; + + let (reconnect_delay, reconnect_backoff_reset) = + next_reconnect_delay(&mut reconnect_attempt); + + assert_eq!(reconnect_delay, REMOTE_CONTROL_RECONNECT_BACKOFF_CAP); + assert!(reconnect_backoff_reset); + assert_eq!(reconnect_attempt, 0); + + let (reconnect_delay, reconnect_backoff_reset) = + next_reconnect_delay(&mut reconnect_attempt); + + assert!(reconnect_delay >= Duration::from_millis(180)); + assert!(reconnect_delay <= Duration::from_millis(220)); + assert!(!reconnect_backoff_reset); + assert_eq!(reconnect_attempt, 1); + } + fn remote_control_status_channel() -> ( RemoteControlStatusPublisher, watch::Receiver, From c7bcb90f9b705f556ed0fb1bde13d836cbf71924 Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Fri, 22 May 2026 17:54:07 -0700 Subject: [PATCH 6/7] package: include zsh fork in Codex package (#23756) ## Why The package layout gives Codex a stable place for runtime helpers that should travel with the entrypoint. `shell_zsh_fork` still required users to configure `zsh_path` manually, even though we already publish prebuilt zsh fork artifacts. This PR builds on #24129 and uses the shared DotSlash artifact fetcher to include the zsh fork in Codex packages when a matching target artifact exists. Packaged Codex builds can then discover the bundled fork automatically; the user/profile `zsh_path` override is removed so the feature uses the package-managed artifact instead of a legacy path knob. ## What Changed - Added `scripts/codex_package/codex-zsh`, a checked-in DotSlash manifest for the current macOS arm64 and Linux zsh fork artifacts. - Taught `scripts/build_codex_package.py` to fetch the matching zsh fork artifact and install it at `codex-resources/zsh/bin/zsh` when available for the selected target. - Added package layout validation for the optional bundled zsh resource. - Added `InstallContext::bundled_zsh_path()` and `InstallContext::bundled_zsh_bin_dir()` for package-layout resource discovery. - Threaded the packaged zsh path through config loading as the runtime `zsh_path` for packaged installs, and removed the config/profile/CLI override path. - Kept the packaged default zsh override typed as `AbsolutePathBuf` until the existing runtime `Config::zsh_path` boundary. - Updated app-server zsh-fork integration tests to spawn `codex-app-server` from a temporary package layout with `codex-resources/zsh/bin/zsh`, matching the new packaged discovery path instead of setting `zsh_path` in config. - Switched package executable copying from metadata-preserving `copy2()` to `copyfile()` plus explicit executable bits, which avoids macOS file-flag failures when local smoke tests use system binaries as inputs. ## Testing To verify that the `zsh` executable from the Codex package is picked up correctly, first I ran: ```shell ./scripts/build_codex_package.py ``` which created: ``` /private/var/folders/vw/x2knqmks50sfhfpy27nftl900000gp/T/codex-package-pms94kdp/ ``` so then I ran: ``` /private/var/folders/vw/x2knqmks50sfhfpy27nftl900000gp/T/codex-package-pms94kdp/bin/codex exec --enable shell_zsh_fork 'run `echo $0`' ``` which reported the following, as expected: ``` /private/var/folders/vw/x2knqmks50sfhfpy27nftl900000gp/T/codex-package-pms94kdp/codex-resources/zsh/bin/zsh ``` --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/openai/codex/pull/23756). * #23768 * __->__ #23756 --- codex-rs/Cargo.lock | 1 + .../app-server/tests/common/mcp_process.rs | 23 ++++++ .../tests/suite/v2/turn_start_zsh_fork.rs | 77 +++++++++++++++---- codex-rs/config/src/config_toml.rs | 3 - codex-rs/config/src/profile_toml.rs | 2 - codex-rs/core/Cargo.toml | 1 + codex-rs/core/config.schema.json | 16 ---- codex-rs/core/src/config/config_tests.rs | 19 +++++ codex-rs/core/src/config/mod.rs | 16 +++- codex-rs/core/src/config/permissions_tests.rs | 2 +- codex-rs/core/src/session/session.rs | 4 +- codex-rs/core/src/session/tests.rs | 4 +- codex-rs/exec/src/lib.rs | 2 +- codex-rs/install-context/src/lib.rs | 35 +++++++++ scripts/codex_package/README.md | 7 ++ scripts/codex_package/cli.py | 4 +- scripts/codex_package/codex-zsh | 43 +++++++++++ scripts/codex_package/layout.py | 17 +++- scripts/codex_package/targets.py | 1 + scripts/codex_package/zsh.py | 22 ++++++ 20 files changed, 250 insertions(+), 49 deletions(-) create mode 100755 scripts/codex_package/codex-zsh create mode 100644 scripts/codex_package/zsh.py diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index b843fcae6b..a56343628f 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -2507,6 +2507,7 @@ dependencies = [ "codex-feedback", "codex-git-utils", "codex-hooks", + "codex-install-context", "codex-login", "codex-mcp", "codex-memories-read", diff --git a/codex-rs/app-server/tests/common/mcp_process.rs b/codex-rs/app-server/tests/common/mcp_process.rs index 993ffa35f3..2d2c26b5a6 100644 --- a/codex-rs/app-server/tests/common/mcp_process.rs +++ b/codex-rs/app-server/tests/common/mcp_process.rs @@ -173,6 +173,20 @@ impl McpProcess { .await } + pub async fn new_with_program_and_env( + codex_home: &Path, + program: &Path, + env_overrides: &[(&str, Option<&str>)], + ) -> anyhow::Result { + Self::new_with_program_env_and_args( + codex_home, + program, + env_overrides, + &[DISABLE_PLUGIN_STARTUP_TASKS_ARG], + ) + .await + } + async fn new_with_env_and_args( codex_home: &Path, env_overrides: &[(&str, Option<&str>)], @@ -180,6 +194,15 @@ impl McpProcess { ) -> anyhow::Result { let program = codex_utils_cargo_bin::cargo_bin("codex-app-server") .context("should find binary for codex-app-server")?; + Self::new_with_program_env_and_args(codex_home, &program, env_overrides, args).await + } + + async fn new_with_program_env_and_args( + codex_home: &Path, + program: &Path, + env_overrides: &[(&str, Option<&str>)], + args: &[&str], + ) -> anyhow::Result { let mut cmd = Command::new(program); cmd.stdin(Stdio::piped()); diff --git a/codex-rs/app-server/tests/suite/v2/turn_start_zsh_fork.rs b/codex-rs/app-server/tests/suite/v2/turn_start_zsh_fork.rs index 6fe93e36af..7b7a6c2563 100644 --- a/codex-rs/app-server/tests/suite/v2/turn_start_zsh_fork.rs +++ b/codex-rs/app-server/tests/suite/v2/turn_start_zsh_fork.rs @@ -37,6 +37,7 @@ use core_test_support::skip_if_no_network; use pretty_assertions::assert_eq; use std::collections::BTreeMap; use std::path::Path; +use std::path::PathBuf; use tempfile::TempDir; use tokio::time::timeout; @@ -94,10 +95,9 @@ async fn turn_start_shell_zsh_fork_executes_command_v2() -> Result<()> { (Feature::UnifiedExec, false), (Feature::ShellSnapshot, false), ]), - &zsh_path, )?; - let mut mcp = create_zsh_test_mcp_process(&codex_home, &workspace).await?; + let mut mcp = create_zsh_test_mcp_process(&codex_home, &workspace, &zsh_path).await?; timeout(DEFAULT_READ_TIMEOUT, mcp.initialize()).await??; let start_id = mcp @@ -162,7 +162,7 @@ async fn turn_start_shell_zsh_fork_executes_command_v2() -> Result<()> { }; assert_eq!(id, "call-zsh-fork"); assert_eq!(status, CommandExecutionStatus::InProgress); - assert!(command.starts_with(&zsh_path.display().to_string())); + assert!(command.starts_with(&command_packaged_zsh_path(&codex_home).display().to_string())); assert!(command.contains("/bin/sh -c")); assert!(command.contains("sleep 0.01")); assert!(command.contains(&release_marker.display().to_string())); @@ -213,10 +213,9 @@ async fn turn_start_shell_zsh_fork_exec_approval_decline_v2() -> Result<()> { (Feature::UnifiedExec, false), (Feature::ShellSnapshot, false), ]), - &zsh_path, )?; - let mut mcp = create_zsh_test_mcp_process(&codex_home, &workspace).await?; + let mut mcp = create_zsh_test_mcp_process(&codex_home, &workspace, &zsh_path).await?; timeout(DEFAULT_READ_TIMEOUT, mcp.initialize()).await??; let start_id = mcp @@ -346,10 +345,9 @@ async fn turn_start_shell_zsh_fork_exec_approval_cancel_v2() -> Result<()> { (Feature::UnifiedExec, false), (Feature::ShellSnapshot, false), ]), - &zsh_path, )?; - let mut mcp = create_zsh_test_mcp_process(&codex_home, &workspace).await?; + let mut mcp = create_zsh_test_mcp_process(&codex_home, &workspace, &zsh_path).await?; timeout(DEFAULT_READ_TIMEOUT, mcp.initialize()).await??; let start_id = mcp @@ -505,10 +503,9 @@ async fn turn_start_shell_zsh_fork_subcommand_decline_marks_parent_declined_v2() (Feature::UnifiedExec, false), (Feature::ShellSnapshot, false), ]), - &zsh_path, )?; - let mut mcp = create_zsh_test_mcp_process(&codex_home, &workspace).await?; + let mut mcp = create_zsh_test_mcp_process(&codex_home, &workspace, &zsh_path).await?; timeout(DEFAULT_READ_TIMEOUT, mcp.initialize()).await??; let start_id = mcp @@ -603,7 +600,8 @@ async fn turn_start_shell_zsh_fork_subcommand_decline_marks_parent_declined_v2() ); approved_subcommand_strings.push(approval_command.to_string()); } - let is_parent_approval = approval_command.contains(&zsh_path.display().to_string()) + let is_parent_approval = approval_command + .contains(&command_packaged_zsh_path(&codex_home).display().to_string()) && (approval_command.contains(&shell_command) || (has_first_file && has_second_file) || approval_command.contains(&parent_shell_hint)); @@ -738,9 +736,58 @@ async fn turn_start_shell_zsh_fork_subcommand_decline_marks_parent_declined_v2() Ok(()) } -async fn create_zsh_test_mcp_process(codex_home: &Path, zdotdir: &Path) -> Result { +async fn create_zsh_test_mcp_process( + codex_home: &Path, + zdotdir: &Path, + zsh_path: &Path, +) -> Result { + let app_server = create_test_package_app_server(codex_home, zsh_path)?; let zdotdir = zdotdir.to_string_lossy().into_owned(); - McpProcess::new_with_env(codex_home, &[("ZDOTDIR", Some(zdotdir.as_str()))]).await + McpProcess::new_with_program_and_env( + codex_home, + &app_server, + &[("ZDOTDIR", Some(zdotdir.as_str()))], + ) + .await +} + +fn create_test_package_app_server(codex_home: &Path, zsh_path: &Path) -> Result { + let package_dir = codex_home.join("test-package"); + let bin_dir = package_dir.join("bin"); + let package_zsh_path = packaged_zsh_path(codex_home); + let Some(zsh_bin_dir) = package_zsh_path.parent() else { + anyhow::bail!("packaged zsh path should have parent"); + }; + std::fs::create_dir_all(&bin_dir)?; + std::fs::create_dir_all(zsh_bin_dir)?; + std::fs::write(package_dir.join("codex-package.json"), "{}")?; + + let app_server = bin_dir.join("codex-app-server"); + copy_with_permissions( + &codex_utils_cargo_bin::cargo_bin("codex-app-server")?, + &app_server, + )?; + copy_with_permissions(zsh_path, &package_zsh_path)?; + Ok(app_server) +} + +fn packaged_zsh_path(codex_home: &Path) -> PathBuf { + codex_home + .join("test-package") + .join("codex-resources") + .join("zsh") + .join("bin") + .join("zsh") +} + +fn command_packaged_zsh_path(codex_home: &Path) -> PathBuf { + let path = packaged_zsh_path(codex_home); + std::fs::canonicalize(&path).unwrap_or(path) +} + +fn copy_with_permissions(source: &Path, destination: &Path) -> std::io::Result<()> { + std::fs::copy(source, destination)?; + std::fs::set_permissions(destination, std::fs::metadata(source)?.permissions()) } fn create_config_toml( @@ -748,7 +795,6 @@ fn create_config_toml( server_uri: &str, approval_policy: &str, feature_flags: &BTreeMap, - zsh_path: &Path, ) -> std::io::Result<()> { let mut features = BTreeMap::from([(Feature::RemoteModels, false)]); for (feature, enabled) in feature_flags { @@ -774,7 +820,6 @@ fn create_config_toml( model = "mock-model" approval_policy = "{approval_policy}" sandbox_mode = "read-only" -zsh_path = "{zsh_path}" model_provider = "mock_provider" @@ -787,9 +832,7 @@ base_url = "{server_uri}/v1" wire_api = "responses" request_max_retries = 0 stream_max_retries = 0 -"#, - approval_policy = approval_policy, - zsh_path = zsh_path.display() +"# ), ) } diff --git a/codex-rs/config/src/config_toml.rs b/codex-rs/config/src/config_toml.rs index b0e2a0d45c..34ae6a41b3 100644 --- a/codex-rs/config/src/config_toml.rs +++ b/codex-rs/config/src/config_toml.rs @@ -291,9 +291,6 @@ pub struct ConfigToml { #[schemars(skip)] pub js_repl_node_module_dirs: Option>, - /// Optional absolute path to patched zsh used by zsh-exec-bridge-backed shell execution. - pub zsh_path: Option, - /// Profile to use from the `profiles` map. pub profile: Option, diff --git a/codex-rs/config/src/profile_toml.rs b/codex-rs/config/src/profile_toml.rs index e7cddd3d67..7d13c02a41 100644 --- a/codex-rs/config/src/profile_toml.rs +++ b/codex-rs/config/src/profile_toml.rs @@ -48,8 +48,6 @@ pub struct ConfigProfile { /// Deprecated: ignored. #[schemars(skip)] pub js_repl_node_module_dirs: Option>, - /// Optional absolute path to patched zsh used by zsh-exec-bridge-backed shell execution. - pub zsh_path: Option, pub experimental_compact_prompt_file: Option, pub include_permissions_instructions: Option, pub include_apps_instructions: Option, diff --git a/codex-rs/core/Cargo.toml b/codex-rs/core/Cargo.toml index 8472918dca..a51c3db7ca 100644 --- a/codex-rs/core/Cargo.toml +++ b/codex-rs/core/Cargo.toml @@ -47,6 +47,7 @@ codex-shell-command = { workspace = true } codex-execpolicy = { workspace = true } codex-git-utils = { workspace = true } codex-hooks = { workspace = true } +codex-install-context = { workspace = true } codex-network-proxy = { workspace = true } codex-otel = { workspace = true } codex-plugin = { workspace = true } diff --git a/codex-rs/core/config.schema.json b/codex-rs/core/config.schema.json index 7d95eec379..8b653609de 100644 --- a/codex-rs/core/config.schema.json +++ b/codex-rs/core/config.schema.json @@ -711,14 +711,6 @@ } ], "default": null - }, - "zsh_path": { - "allOf": [ - { - "$ref": "#/definitions/AbsolutePathBuf" - } - ], - "description": "Optional absolute path to patched zsh used by zsh-exec-bridge-backed shell execution." } }, "type": "object" @@ -4986,14 +4978,6 @@ ], "default": null, "description": "Windows-specific configuration." - }, - "zsh_path": { - "allOf": [ - { - "$ref": "#/definitions/AbsolutePathBuf" - } - ], - "description": "Optional absolute path to patched zsh used by zsh-exec-bridge-backed shell execution." } }, "title": "ConfigToml", diff --git a/codex-rs/core/src/config/config_tests.rs b/codex-rs/core/src/config/config_tests.rs index 09f17e201d..58b6dcc0da 100644 --- a/codex-rs/core/src/config/config_tests.rs +++ b/codex-rs/core/src/config/config_tests.rs @@ -4470,6 +4470,25 @@ async fn add_dir_override_extends_workspace_writable_roots() -> std::io::Result< Ok(()) } +#[tokio::test] +async fn default_zsh_path_sets_runtime_zsh_path() -> std::io::Result<()> { + let codex_home = TempDir::new()?; + let default_zsh_path = codex_home.path().join("packaged-zsh"); + + let config = Config::load_from_base_config_with_overrides( + ConfigToml::default(), + ConfigOverrides { + default_zsh_path: Some(default_zsh_path.abs()), + ..Default::default() + }, + codex_home.abs(), + ) + .await?; + assert_eq!(config.zsh_path, Some(default_zsh_path)); + + Ok(()) +} + #[tokio::test] async fn sqlite_home_defaults_to_codex_home_for_workspace_write() -> std::io::Result<()> { let codex_home = TempDir::new()?; diff --git a/codex-rs/core/src/config/mod.rs b/codex-rs/core/src/config/mod.rs index 7044b9c676..261850c8a0 100644 --- a/codex-rs/core/src/config/mod.rs +++ b/codex-rs/core/src/config/mod.rs @@ -67,6 +67,7 @@ use codex_features::FeaturesToml; use codex_features::MultiAgentV2ConfigToml; use codex_features::NetworkProxyConfigToml; use codex_git_utils::resolve_root_git_project_for_trust; +use codex_install_context::InstallContext; use codex_login::AuthManagerConfig; use codex_mcp::McpConfig; use codex_memories_read::memory_root; @@ -1392,11 +1393,18 @@ impl Config { .effective_config() .try_into() .map_err(|err| std::io::Error::new(std::io::ErrorKind::InvalidData, err))?; + let default_zsh_path = refreshed_config + .zsh_path + .clone() + .map(AbsolutePathBuf::try_from) + .transpose()?; + Self::load_config_with_layer_stack( LOCAL_FS.as_ref(), cfg, ConfigOverrides { cwd: Some(self.cwd.to_path_buf()), + default_zsh_path, ..Default::default() }, refreshed_config.codex_home.clone(), @@ -2128,7 +2136,7 @@ pub struct ConfigOverrides { pub codex_self_exe: Option, pub codex_linux_sandbox_exe: Option, pub main_execve_wrapper_exe: Option, - pub zsh_path: Option, + pub default_zsh_path: Option, pub base_instructions: Option, pub developer_instructions: Option, pub personality: Option, @@ -2480,7 +2488,7 @@ impl Config { codex_self_exe, codex_linux_sandbox_exe, main_execve_wrapper_exe, - zsh_path: zsh_path_override, + default_zsh_path, base_instructions, developer_instructions, personality, @@ -3199,7 +3207,9 @@ impl Config { ) .await?; let compact_prompt = compact_prompt.or(file_compact_prompt); - let zsh_path = zsh_path_override.or(cfg.zsh_path.map(Into::into)); + let zsh_path = default_zsh_path + .or_else(|| InstallContext::current().bundled_zsh_path()) + .map(AbsolutePathBuf::into_path_buf); let review_model = override_review_model.or(cfg.review_model); diff --git a/codex-rs/core/src/config/permissions_tests.rs b/codex-rs/core/src/config/permissions_tests.rs index 09aa7606aa..8f9d6732e4 100644 --- a/codex-rs/core/src/config/permissions_tests.rs +++ b/codex-rs/core/src/config/permissions_tests.rs @@ -82,7 +82,7 @@ async fn restricted_read_implicitly_allows_helper_executables() -> std::io::Resu }, ConfigOverrides { cwd: Some(cwd.clone()), - zsh_path: Some(zsh_path.clone()), + default_zsh_path: Some(AbsolutePathBuf::try_from(zsh_path.clone())?), main_execve_wrapper_exe: Some(execve_wrapper), ..Default::default() }, diff --git a/codex-rs/core/src/session/session.rs b/codex-rs/core/src/session/session.rs index 6ef8aad91e..e430202ae1 100644 --- a/codex-rs/core/src/session/session.rs +++ b/codex-rs/core/src/session/session.rs @@ -826,13 +826,13 @@ impl Session { } else if use_zsh_fork_shell { let zsh_path = config.zsh_path.as_ref().ok_or_else(|| { anyhow::anyhow!( - "zsh fork feature enabled, but `zsh_path` is not configured; set `zsh_path` in config.toml" + "zsh fork feature enabled, but no packaged zsh fork is available for this install" ) })?; let zsh_path = zsh_path.to_path_buf(); shell::get_shell(shell::ShellType::Zsh, Some(&zsh_path)).ok_or_else(|| { anyhow::anyhow!( - "zsh fork feature enabled, but zsh_path `{}` is not usable; set `zsh_path` to a valid zsh executable", + "zsh fork feature enabled, but packaged zsh fork `{}` is not usable", zsh_path.display() ) })? diff --git a/codex-rs/core/src/session/tests.rs b/codex-rs/core/src/session/tests.rs index 30d26191d7..ac62a3482c 100644 --- a/codex-rs/core/src/session/tests.rs +++ b/codex-rs/core/src/session/tests.rs @@ -4319,7 +4319,7 @@ async fn absolute_cwd_update_with_turn_environment_is_allowed() { } #[tokio::test] -async fn session_new_fails_when_zsh_fork_enabled_without_zsh_path() { +async fn session_new_fails_when_zsh_fork_enabled_without_packaged_zsh() { let codex_home = tempfile::tempdir().expect("create temp dir"); let mut config = build_test_config(codex_home.path()).await; config @@ -4420,7 +4420,7 @@ async fn session_new_fails_when_zsh_fork_enabled_without_zsh_path() { Err(err) => err, }; let msg = format!("{err:#}"); - assert!(msg.contains("zsh fork feature enabled, but `zsh_path` is not configured")); + assert!(msg.contains("zsh fork feature enabled, but no packaged zsh fork is available")); } // todo: use online model info diff --git a/codex-rs/exec/src/lib.rs b/codex-rs/exec/src/lib.rs index 52590b56cc..9061227b6b 100644 --- a/codex-rs/exec/src/lib.rs +++ b/codex-rs/exec/src/lib.rs @@ -416,7 +416,7 @@ pub async fn run_main(cli: Cli, arg0_paths: Arg0DispatchPaths) -> anyhow::Result codex_self_exe: arg0_paths.codex_self_exe.clone(), codex_linux_sandbox_exe: arg0_paths.codex_linux_sandbox_exe.clone(), main_execve_wrapper_exe: arg0_paths.main_execve_wrapper_exe.clone(), - zsh_path: None, + default_zsh_path: None, base_instructions: None, developer_instructions: None, personality: None, diff --git a/codex-rs/install-context/src/lib.rs b/codex-rs/install-context/src/lib.rs index ec5c5217ca..d92a80cbef 100644 --- a/codex-rs/install-context/src/lib.rs +++ b/codex-rs/install-context/src/lib.rs @@ -11,6 +11,7 @@ const PATH_DIRNAME: &str = "codex-path"; const RELEASES_DIRNAME: &str = "releases"; const RESOURCES_DIRNAME: &str = "codex-resources"; const STANDALONE_PACKAGES_DIRNAME: &str = "standalone"; +const ZSH_DIRNAME: &str = "zsh"; static INSTALL_CONTEXT: OnceLock = OnceLock::new(); #[derive(Clone, Copy, Debug, Eq, PartialEq)] @@ -166,6 +167,18 @@ impl InstallContext { None } + + pub fn bundled_zsh_path(&self) -> Option { + if cfg!(windows) { + None + } else { + self.bundled_resource(zsh_resource_path()) + } + } + + pub fn bundled_zsh_bin_dir(&self) -> Option { + self.bundled_zsh_path()?.parent() + } } impl CodexPackageLayout { @@ -260,6 +273,10 @@ fn default_rg_command() -> PathBuf { } } +fn zsh_resource_path() -> PathBuf { + PathBuf::from(ZSH_DIRNAME).join(BIN_DIRNAME).join("zsh") +} + #[cfg(test)] mod tests { use super::*; @@ -345,6 +362,11 @@ mod tests { fs::write(&exe_path, "")?; fs::write(resources_dir.join(TEST_RESOURCE_NAME), "")?; fs::write(path_dir.join(default_rg_command()), "")?; + if !cfg!(windows) { + let zsh_path = resources_dir.join(zsh_resource_path()); + fs::create_dir_all(zsh_path.parent().expect("zsh path should have parent"))?; + fs::write(&zsh_path, "")?; + } let canonical_package_dir = AbsolutePathBuf::from_absolute_path(package_dir.path().canonicalize()?)?; let canonical_bin_dir = AbsolutePathBuf::from_absolute_path(bin_dir.canonicalize()?)?; @@ -382,6 +404,19 @@ mod tests { context.bundled_resource(TEST_RESOURCE_NAME), Some(canonical_resources_dir.join(TEST_RESOURCE_NAME)) ); + if cfg!(windows) { + assert_eq!(context.bundled_zsh_path(), None); + assert_eq!(context.bundled_zsh_bin_dir(), None); + } else { + assert_eq!( + context.bundled_zsh_path(), + Some(canonical_resources_dir.join(zsh_resource_path())) + ); + assert_eq!( + context.bundled_zsh_bin_dir(), + Some(canonical_resources_dir.join(ZSH_DIRNAME).join(BIN_DIRNAME)) + ); + } Ok(()) } diff --git a/scripts/codex_package/README.md b/scripts/codex_package/README.md index af070e2c44..323a3ce5ba 100644 --- a/scripts/codex_package/README.md +++ b/scripts/codex_package/README.md @@ -13,6 +13,7 @@ The builder creates a canonical Codex package directory: │ └── [.exe] ├── codex-resources │ ├── bwrap # Linux only +│ ├── zsh/bin/zsh # supported Unix targets only │ ├── codex-command-runner.exe # Windows only │ └── codex-windows-sandbox-setup.exe # Windows only └── codex-path @@ -67,3 +68,9 @@ DotSlash manifest at `scripts/codex_package/rg`. Downloaded archives are cached under `$TMPDIR/codex-package/-rg` and are reused only after the recorded size and SHA-256 digest have been verified. Pass `--rg-bin` to use a local ripgrep executable instead. + +The patched zsh fork used by `shell_zsh_fork` is fetched from the DotSlash +manifest at `scripts/codex_package/codex-zsh` when the selected target has a +matching prebuilt artifact. Downloaded archives are cached under +`$TMPDIR/codex-package/-zsh` and installed at +`codex-resources/zsh/bin/zsh`. diff --git a/scripts/codex_package/cli.py b/scripts/codex_package/cli.py index 9c0b16258d..0152002669 100644 --- a/scripts/codex_package/cli.py +++ b/scripts/codex_package/cli.py @@ -15,6 +15,7 @@ from .targets import TARGET_SPECS from .targets import PackageInputs from .targets import default_target from .targets import resolve_input_path +from .zsh import resolve_zsh_bin from .version import read_workspace_version @@ -161,13 +162,14 @@ def main() -> int: inputs = PackageInputs( entrypoint_bin=source_outputs.entrypoint_bin, rg_bin=resolve_rg_bin(spec, args.rg_bin), + zsh_bin=resolve_zsh_bin(spec), bwrap_bin=source_outputs.bwrap_bin, codex_command_runner_bin=source_outputs.codex_command_runner_bin, codex_windows_sandbox_setup_bin=source_outputs.codex_windows_sandbox_setup_bin, ) prepare_package_dir(package_dir, force=args.force) build_package_dir(package_dir, version, variant, spec, inputs) - validate_package_dir(package_dir, variant, spec) + validate_package_dir(package_dir, variant, spec, include_zsh=inputs.zsh_bin is not None) for archive_output in args.archive_output: archive_path = archive_output.resolve() diff --git a/scripts/codex_package/codex-zsh b/scripts/codex_package/codex-zsh new file mode 100755 index 0000000000..8876474d81 --- /dev/null +++ b/scripts/codex_package/codex-zsh @@ -0,0 +1,43 @@ +#!/usr/bin/env dotslash + +{ + "name": "codex-zsh", + "platforms": { + "macos-aarch64": { + "size": 358776, + "hash": "sha256", + "digest": "c6dbb063a0135b947ab1cacc655b2b750874699472f412ec7daba97543a90c3c", + "format": "tar.gz", + "path": "codex-zsh/bin/zsh", + "providers": [ + { + "url": "https://github.com/openai/codex/releases/download/rust-v0.132.0/codex-zsh-aarch64-apple-darwin.tar.gz" + } + ] + }, + "linux-x86_64": { + "size": 433413, + "hash": "sha256", + "digest": "5f42d9fc8e9c8c399a727512002906006ae9de966ea7b3d87ca36b47efc59938", + "format": "tar.gz", + "path": "codex-zsh/bin/zsh", + "providers": [ + { + "url": "https://github.com/openai/codex/releases/download/rust-v0.132.0/codex-zsh-x86_64-unknown-linux-musl.tar.gz" + } + ] + }, + "linux-aarch64": { + "size": 411653, + "hash": "sha256", + "digest": "6c6e32c297425db02b4dbffb10925895875d14647fc3eb2f18767be97dc6a945", + "format": "tar.gz", + "path": "codex-zsh/bin/zsh", + "providers": [ + { + "url": "https://github.com/openai/codex/releases/download/rust-v0.132.0/codex-zsh-aarch64-unknown-linux-musl.tar.gz" + } + ] + } + } +} diff --git a/scripts/codex_package/layout.py b/scripts/codex_package/layout.py index 6d8982a631..c763eb2604 100644 --- a/scripts/codex_package/layout.py +++ b/scripts/codex_package/layout.py @@ -8,6 +8,7 @@ from pathlib import Path from .targets import PackageInputs from .targets import PackageVariant from .targets import TargetSpec +from .zsh import ZSH_RESOURCE_PATH LAYOUT_VERSION = 1 @@ -50,6 +51,13 @@ def build_package_dir( ) copy_executable(inputs.rg_bin, path_dir / spec.rg_name, is_windows=spec.is_windows) + if inputs.zsh_bin is not None: + copy_executable( + inputs.zsh_bin, + resources_dir / ZSH_RESOURCE_PATH, + is_windows=False, + ) + if inputs.bwrap_bin is not None: copy_executable(inputs.bwrap_bin, resources_dir / "bwrap", is_windows=False) @@ -83,6 +91,8 @@ def validate_package_dir( package_dir: Path, variant: PackageVariant, spec: TargetSpec, + *, + include_zsh: bool, ) -> None: required_dirs = [ Path("bin"), @@ -122,6 +132,11 @@ def validate_package_dir( ] executable_files = list(required_files) + if include_zsh: + zsh_path = Path("codex-resources") / ZSH_RESOURCE_PATH + required_files.append(zsh_path) + executable_files.append(zsh_path) + if spec.is_linux: required_files.append(Path("codex-resources") / "bwrap") executable_files.append(Path("codex-resources") / "bwrap") @@ -148,7 +163,7 @@ def validate_package_dir( def copy_executable(src: Path, dest: Path, *, is_windows: bool) -> None: dest.parent.mkdir(parents=True, exist_ok=True) - shutil.copy2(src, dest) + shutil.copyfile(src, dest) if not is_windows: mode = dest.stat().st_mode dest.chmod(mode | stat.S_IXUSR | stat.S_IXGRP | stat.S_IXOTH) diff --git a/scripts/codex_package/targets.py b/scripts/codex_package/targets.py index 4af0d4a00d..8307a3e630 100644 --- a/scripts/codex_package/targets.py +++ b/scripts/codex_package/targets.py @@ -40,6 +40,7 @@ class PackageVariant: class PackageInputs: entrypoint_bin: Path rg_bin: Path + zsh_bin: Path | None bwrap_bin: Path | None codex_command_runner_bin: Path | None codex_windows_sandbox_setup_bin: Path | None diff --git a/scripts/codex_package/zsh.py b/scripts/codex_package/zsh.py new file mode 100644 index 0000000000..f4ee7e19dd --- /dev/null +++ b/scripts/codex_package/zsh.py @@ -0,0 +1,22 @@ +"""Fetch the patched zsh fork used by shell_zsh_fork.""" + +from pathlib import Path + +from .dotslash import fetch_dotslash_executable +from .targets import REPO_ROOT +from .targets import TargetSpec + + +ZSH_MANIFEST = REPO_ROOT / "scripts" / "codex_package" / "codex-zsh" +ZSH_RESOURCE_PATH = Path("zsh") / "bin" / "zsh" + + +def resolve_zsh_bin(spec: TargetSpec) -> Path | None: + return fetch_dotslash_executable( + spec, + manifest_path=ZSH_MANIFEST, + artifact_label="codex-zsh", + cache_key=f"{spec.target}-zsh", + dest_name="zsh", + missing_ok=True, + ) From 6995a8670a1c14e17475f0298dfe47ef894ac889 Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Fri, 22 May 2026 17:54:18 -0700 Subject: [PATCH 7/7] runtime: prepend zsh fork bin dir to PATH Summary: - prepend the configured zsh fork executable directory to PATH for zsh-fork shell-command and unified-exec launches - preserve that PATH value through shell snapshot wrapping so #!/usr/bin/env zsh can resolve to the packaged fork - cover duplicate PATH entries and snapshot preservation in codex-core runtime tests Test Plan: - just fmt - cargo test -p codex-core apply_zsh_fork_path_prepend - cargo test -p codex-core maybe_wrap_shell_lc_with_snapshot_preserves_zsh_fork_path_prepend - just fix -p codex-core Notes: - cargo test -p codex-core currently aborts in unrelated thread_manager::tests::resume_and_fork_do_not_restore_thread_environments_from_rollout with a stack overflow; rerunning that single test reproduces the same stack overflow. --- codex-rs/core/src/tools/runtimes/mod.rs | 25 +++++ codex-rs/core/src/tools/runtimes/mod_tests.rs | 93 +++++++++++++++++++ codex-rs/core/src/tools/runtimes/shell.rs | 17 +++- .../tools/runtimes/shell/unix_escalation.rs | 5 +- .../tools/runtimes/shell/zsh_fork_backend.rs | 10 +- .../core/src/tools/runtimes/unified_exec.rs | 13 ++- 6 files changed, 153 insertions(+), 10 deletions(-) diff --git a/codex-rs/core/src/tools/runtimes/mod.rs b/codex-rs/core/src/tools/runtimes/mod.rs index bba1c572ef..71a3fd0cbb 100644 --- a/codex-rs/core/src/tools/runtimes/mod.rs +++ b/codex-rs/core/src/tools/runtimes/mod.rs @@ -22,6 +22,8 @@ use codex_sandboxing::SandboxCommand; use codex_sandboxing::SandboxType; use codex_utils_absolute_path::AbsolutePathBuf; use std::collections::HashMap; +#[cfg(unix)] +use std::path::Path; pub(crate) mod apply_patch; pub(crate) mod shell; @@ -70,6 +72,29 @@ pub(crate) fn exec_env_for_sandbox_permissions( env } +#[cfg(unix)] +pub(crate) fn apply_zsh_fork_path_prepend( + env: &mut HashMap, + explicit_env_overrides: &mut HashMap, + shell_zsh_path: &Path, +) { + let Some(zsh_bin_dir) = shell_zsh_path.parent() else { + return; + }; + let zsh_bin_dir = zsh_bin_dir.to_string_lossy().to_string(); + let updated_path = match env.get("PATH") { + Some(path) if !path.is_empty() => std::iter::once(zsh_bin_dir.as_str()) + .chain(path.split(':').filter(|entry| *entry != zsh_bin_dir)) + .collect::>() + .join(":"), + _ => zsh_bin_dir, + }; + env.insert("PATH".to_string(), updated_path.clone()); + // Snapshot wrapping restores explicit overrides after sourcing the shell + // snapshot, so capture this PATH override there as well. + explicit_env_overrides.insert("PATH".to_string(), updated_path); +} + pub(crate) fn disable_powershell_profile_for_elevated_windows_sandbox( command: &[String], shell_type: Option<&ShellType>, diff --git a/codex-rs/core/src/tools/runtimes/mod_tests.rs b/codex-rs/core/src/tools/runtimes/mod_tests.rs index fd10f22242..539bbb90de 100644 --- a/codex-rs/core/src/tools/runtimes/mod_tests.rs +++ b/codex-rs/core/src/tools/runtimes/mod_tests.rs @@ -143,6 +143,48 @@ async fn explicit_escalation_prepares_exec_without_managed_network() -> anyhow:: Ok(()) } +#[cfg(unix)] +#[test] +fn apply_zsh_fork_path_prepend_uses_shell_parent() { + let mut env = HashMap::from([("PATH".to_string(), "/usr/bin:/bin".to_string())]); + let mut explicit_env_overrides = HashMap::new(); + + apply_zsh_fork_path_prepend( + &mut env, + &mut explicit_env_overrides, + PathBuf::from("/package/codex-resources/zsh/bin/zsh").as_path(), + ); + + let expected = "/package/codex-resources/zsh/bin:/usr/bin:/bin"; + assert_eq!(env.get("PATH").map(String::as_str), Some(expected)); + assert_eq!( + explicit_env_overrides.get("PATH").map(String::as_str), + Some(expected) + ); +} + +#[cfg(unix)] +#[test] +fn apply_zsh_fork_path_prepend_moves_existing_shell_parent_to_front() { + let mut env = HashMap::from([( + "PATH".to_string(), + "/usr/bin:/package/codex-resources/zsh/bin:/bin:/package/codex-resources/zsh/bin" + .to_string(), + )]); + let mut explicit_env_overrides = HashMap::new(); + + apply_zsh_fork_path_prepend( + &mut env, + &mut explicit_env_overrides, + PathBuf::from("/package/codex-resources/zsh/bin/zsh").as_path(), + ); + + assert_eq!( + env.get("PATH").map(String::as_str), + Some("/package/codex-resources/zsh/bin:/usr/bin:/bin") + ); +} + #[test] fn explicit_escalation_keeps_user_proxy_env_without_codex_marker() { let env = HashMap::from([ @@ -818,6 +860,57 @@ fn maybe_wrap_shell_lc_with_snapshot_applies_explicit_path_override() { assert_eq!(String::from_utf8_lossy(&output.stdout), "/worktree/bin"); } +#[cfg(unix)] +#[test] +fn maybe_wrap_shell_lc_with_snapshot_preserves_zsh_fork_path_prepend() { + let dir = tempdir().expect("create temp dir"); + let snapshot_path = dir.path().join("snapshot.sh"); + std::fs::write( + &snapshot_path, + "# Snapshot file\nexport PATH='/snapshot/bin'\n", + ) + .expect("write snapshot"); + let session_shell = shell_with_snapshot( + ShellType::Bash, + "/bin/bash", + snapshot_path.abs(), + dir.path().abs(), + ); + let command = vec![ + "/bin/bash".to_string(), + "-lc".to_string(), + "printf '%s' \"$PATH\"".to_string(), + ]; + let zsh_path = dir + .path() + .join("codex-resources") + .join("zsh") + .join("bin") + .join("zsh"); + let zsh_bin_dir = zsh_path.parent().expect("zsh path should have parent"); + let mut env = HashMap::from([("PATH".to_string(), "/worktree/bin".to_string())]); + let mut explicit_env_overrides = HashMap::new(); + apply_zsh_fork_path_prepend(&mut env, &mut explicit_env_overrides, zsh_path.as_path()); + let rewritten = maybe_wrap_shell_lc_with_snapshot( + &command, + &session_shell, + &dir.path().abs(), + &explicit_env_overrides, + &env, + ); + let output = Command::new(&rewritten[0]) + .args(&rewritten[1..]) + .env("PATH", env.get("PATH").expect("PATH should be set")) + .output() + .expect("run rewritten command"); + + assert!(output.status.success(), "command failed: {output:?}"); + assert_eq!( + String::from_utf8_lossy(&output.stdout), + format!("{}:/worktree/bin", zsh_bin_dir.display()) + ); +} + #[test] fn maybe_wrap_shell_lc_with_snapshot_does_not_embed_override_values_in_argv() { let dir = tempdir().expect("create temp dir"); diff --git a/codex-rs/core/src/tools/runtimes/shell.rs b/codex-rs/core/src/tools/runtimes/shell.rs index 6251267773..7de15b3672 100644 --- a/codex-rs/core/src/tools/runtimes/shell.rs +++ b/codex-rs/core/src/tools/runtimes/shell.rs @@ -20,6 +20,8 @@ use crate::shell::ShellType; use crate::tools::flat_tool_name; use crate::tools::network_approval::NetworkApprovalMode; use crate::tools::network_approval::NetworkApprovalSpec; +#[cfg(unix)] +use crate::tools::runtimes::apply_zsh_fork_path_prepend; use crate::tools::runtimes::build_sandbox_command; use crate::tools::runtimes::disable_powershell_profile_for_elevated_windows_sandbox; use crate::tools::runtimes::exec_env_for_sandbox_permissions; @@ -231,12 +233,19 @@ impl ToolRuntime for ShellRuntime { let session_shell = ctx.session.user_shell(); let managed_network = managed_network_for_sandbox_permissions(req.network.as_ref(), req.sandbox_permissions); - let env = exec_env_for_sandbox_permissions(&req.env, req.sandbox_permissions); + let mut env = exec_env_for_sandbox_permissions(&req.env, req.sandbox_permissions); + let mut explicit_env_overrides = req.explicit_env_overrides.clone(); + #[cfg(unix)] + if self.backend == ShellRuntimeBackend::ShellCommandZshFork + && let Some(shell_zsh_path) = ctx.session.services.shell_zsh_path.as_deref() + { + apply_zsh_fork_path_prepend(&mut env, &mut explicit_env_overrides, shell_zsh_path); + } let command = maybe_wrap_shell_lc_with_snapshot( &req.command, session_shell.as_ref(), &req.cwd, - &req.explicit_env_overrides, + &explicit_env_overrides, &env, ); let command = disable_powershell_profile_for_elevated_windows_sandbox( @@ -252,7 +261,9 @@ impl ToolRuntime for ShellRuntime { }; if self.backend == ShellRuntimeBackend::ShellCommandZshFork { - match zsh_fork_backend::maybe_run_shell_command(req, attempt, ctx, &command).await? { + match zsh_fork_backend::maybe_run_shell_command(req, attempt, ctx, &command, &env) + .await? + { Some(out) => return Ok(out), None => { tracing::warn!( diff --git a/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs b/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs index fef8db5ca9..597ab78dbe 100644 --- a/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs +++ b/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs @@ -15,7 +15,6 @@ use crate::sandboxing::ExecRequest; use crate::sandboxing::SandboxPermissions; use crate::shell::ShellType; use crate::tools::runtimes::build_sandbox_command; -use crate::tools::runtimes::exec_env_for_sandbox_permissions; use crate::tools::sandboxing::PermissionRequestPayload; use crate::tools::sandboxing::SandboxAttempt; use crate::tools::sandboxing::ToolCtx; @@ -102,6 +101,7 @@ pub(super) async fn try_run_zsh_fork( attempt: &SandboxAttempt<'_>, ctx: &ToolCtx, command: &[String], + env: &HashMap, ) -> Result, ToolError> { let Some(shell_zsh_path) = ctx.session.services.shell_zsh_path.as_ref() else { tracing::warn!("ZshFork backend specified, but shell_zsh_path is not configured."); @@ -116,9 +116,8 @@ pub(super) async fn try_run_zsh_fork( return Ok(None); } - let env = exec_env_for_sandbox_permissions(&req.env, req.sandbox_permissions); let command = - build_sandbox_command(command, &req.cwd, &env, req.additional_permissions.clone())?; + build_sandbox_command(command, &req.cwd, env, req.additional_permissions.clone())?; let options = ExecOptions { expiration: req.timeout_ms.into(), capture_policy: ExecCapturePolicy::ShellTool, diff --git a/codex-rs/core/src/tools/runtimes/shell/zsh_fork_backend.rs b/codex-rs/core/src/tools/runtimes/shell/zsh_fork_backend.rs index 819658ecca..093a60e038 100644 --- a/codex-rs/core/src/tools/runtimes/shell/zsh_fork_backend.rs +++ b/codex-rs/core/src/tools/runtimes/shell/zsh_fork_backend.rs @@ -7,6 +7,7 @@ use crate::tools::sandboxing::ToolError; use crate::unified_exec::SpawnLifecycleHandle; use codex_protocol::exec_output::ExecToolCallOutput; use codex_tools::ZshForkConfig; +use std::collections::HashMap; pub(crate) struct PreparedUnifiedExecSpawn { pub(crate) exec_request: ExecRequest, @@ -23,8 +24,9 @@ pub(crate) async fn maybe_run_shell_command( attempt: &SandboxAttempt<'_>, ctx: &ToolCtx, command: &[String], + env: &HashMap, ) -> Result, ToolError> { - imp::maybe_run_shell_command(req, attempt, ctx, command).await + imp::maybe_run_shell_command(req, attempt, ctx, command, env).await } /// Prepares unified exec to launch through the zsh-fork backend when the @@ -76,8 +78,9 @@ mod imp { attempt: &SandboxAttempt<'_>, ctx: &ToolCtx, command: &[String], + env: &HashMap, ) -> Result, ToolError> { - unix_escalation::try_run_zsh_fork(req, attempt, ctx, command).await + unix_escalation::try_run_zsh_fork(req, attempt, ctx, command, env).await } pub(super) async fn maybe_prepare_unified_exec( @@ -118,8 +121,9 @@ mod imp { attempt: &SandboxAttempt<'_>, ctx: &ToolCtx, command: &[String], + env: &HashMap, ) -> Result, ToolError> { - let _ = (req, attempt, ctx, command); + let _ = (req, attempt, ctx, command, env); Ok(None) } diff --git a/codex-rs/core/src/tools/runtimes/unified_exec.rs b/codex-rs/core/src/tools/runtimes/unified_exec.rs index c613f198e0..ec9de38679 100644 --- a/codex-rs/core/src/tools/runtimes/unified_exec.rs +++ b/codex-rs/core/src/tools/runtimes/unified_exec.rs @@ -17,6 +17,8 @@ use crate::shell::ShellType; use crate::tools::flat_tool_name; use crate::tools::network_approval::NetworkApprovalMode; use crate::tools::network_approval::NetworkApprovalSpec; +#[cfg(unix)] +use crate::tools::runtimes::apply_zsh_fork_path_prepend; use crate::tools::runtimes::build_sandbox_command; use crate::tools::runtimes::disable_powershell_profile_for_elevated_windows_sandbox; use crate::tools::runtimes::exec_env_for_sandbox_permissions; @@ -261,6 +263,15 @@ impl<'a> ToolRuntime for UnifiedExecRunt if let Some(network) = managed_network { network.apply_to_env(&mut env); } + let mut explicit_env_overrides = req.explicit_env_overrides.clone(); + #[cfg(unix)] + if let UnifiedExecShellMode::ZshFork(zsh_fork_config) = &self.shell_mode { + apply_zsh_fork_path_prepend( + &mut env, + &mut explicit_env_overrides, + zsh_fork_config.shell_zsh_path.as_path(), + ); + } let environment_is_remote = req.environment.is_remote(); let command = if environment_is_remote { base_command.to_vec() @@ -269,7 +280,7 @@ impl<'a> ToolRuntime for UnifiedExecRunt base_command, session_shell.as_ref(), &req.cwd, - &req.explicit_env_overrides, + &explicit_env_overrides, &env, ) };