diff --git a/.github/dotslash-config.json b/.github/dotslash-config.json index 5e28cdf207..00e9032cf1 100644 --- a/.github/dotslash-config.json +++ b/.github/dotslash-config.json @@ -55,6 +55,30 @@ "path": "codex-responses-api-proxy.exe" } } + }, + "codex-command-runner": { + "platforms": { + "windows-x86_64": { + "regex": "^codex-command-runner-x86_64-pc-windows-msvc\\.exe\\.zst$", + "path": "codex-command-runner.exe" + }, + "windows-aarch64": { + "regex": "^codex-command-runner-aarch64-pc-windows-msvc\\.exe\\.zst$", + "path": "codex-command-runner.exe" + } + } + }, + "codex-windows-sandbox-setup": { + "platforms": { + "windows-x86_64": { + "regex": "^codex-windows-sandbox-setup-x86_64-pc-windows-msvc\\.exe\\.zst$", + "path": "codex-windows-sandbox-setup.exe" + }, + "windows-aarch64": { + "regex": "^codex-windows-sandbox-setup-aarch64-pc-windows-msvc\\.exe\\.zst$", + "path": "codex-windows-sandbox-setup.exe" + } + } } } } diff --git a/.github/workflows/rust-release-prepare.yml b/.github/workflows/rust-release-prepare.yml new file mode 100644 index 0000000000..0c4656c0b7 --- /dev/null +++ b/.github/workflows/rust-release-prepare.yml @@ -0,0 +1,51 @@ +name: rust-release-prepare +on: + workflow_dispatch: + schedule: + - cron: "0 */4 * * *" + +concurrency: + group: ${{ github.workflow }} + cancel-in-progress: false + +permissions: + contents: write + pull-requests: write + +jobs: + prepare: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v6 + with: + ref: main + fetch-depth: 0 + + - name: Update models.json + env: + OPENAI_API_KEY: ${{ secrets.CODEX_OPENAI_API_KEY }} + run: | + set -euo pipefail + + client_version="99.99.99" + terminal_info="github-actions" + user_agent="codex_cli_rs/99.99.99 (Linux $(uname -r); $(uname -m)) ${terminal_info}" + base_url="${OPENAI_BASE_URL:-https://chatgpt.com/backend-api/codex}" + + headers=( + -H "Authorization: Bearer ${OPENAI_API_KEY}" + -H "User-Agent: ${user_agent}" + ) + + url="${base_url%/}/models?client_version=${client_version}" + curl --http1.1 --fail --show-error --location "${headers[@]}" "${url}" | jq '.' > codex-rs/core/models.json + + - name: Open pull request (if changed) + uses: peter-evans/create-pull-request@v7 + with: + commit-message: "Update models.json" + title: "Update models.json" + body: "Automated update of models.json." + branch: "bot/update-models-json" + reviewers: "pakrym-oai,aibrahim-oai" + delete-branch: true diff --git a/AGENTS.md b/AGENTS.md index cc7994efbe..50c10b1da1 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -75,6 +75,7 @@ If you don’t have the tool: - Tests should use pretty_assertions::assert_eq for clearer diffs. Import this at the top of the test module if it isn't already. - Prefer deep equals comparisons whenever possible. Perform `assert_eq!()` on entire objects, rather than individual fields. +- Avoid mutating process environment in tests; prefer passing environment-derived flags or dependencies from above. ### Integration tests (core) diff --git a/codex-cli/scripts/build_npm_package.py b/codex-cli/scripts/build_npm_package.py index ef96bef2ee..bf0eb5f469 100755 --- a/codex-cli/scripts/build_npm_package.py +++ b/codex-cli/scripts/build_npm_package.py @@ -20,9 +20,14 @@ PACKAGE_NATIVE_COMPONENTS: dict[str, list[str]] = { "codex-responses-api-proxy": ["codex-responses-api-proxy"], "codex-sdk": ["codex"], } +WINDOWS_ONLY_COMPONENTS: dict[str, list[str]] = { + "codex": ["codex-windows-sandbox-setup", "codex-command-runner"], +} COMPONENT_DEST_DIR: dict[str, str] = { "codex": "codex", "codex-responses-api-proxy": "codex-responses-api-proxy", + "codex-windows-sandbox-setup": "codex", + "codex-command-runner": "codex", "rg": "path", } @@ -103,7 +108,7 @@ def main() -> int: "pointing to a directory containing pre-installed binaries." ) - copy_native_binaries(vendor_src, staging_dir, native_components) + copy_native_binaries(vendor_src, staging_dir, package, native_components) if release_version: staging_dir_str = str(staging_dir) @@ -232,7 +237,12 @@ def stage_codex_sdk_sources(staging_dir: Path) -> None: shutil.copy2(license_src, staging_dir / "LICENSE") -def copy_native_binaries(vendor_src: Path, staging_dir: Path, components: list[str]) -> None: +def copy_native_binaries( + vendor_src: Path, + staging_dir: Path, + package: str, + components: list[str], +) -> None: vendor_src = vendor_src.resolve() if not vendor_src.exists(): raise RuntimeError(f"Vendor source directory not found: {vendor_src}") @@ -250,6 +260,9 @@ def copy_native_binaries(vendor_src: Path, staging_dir: Path, components: list[s if not target_dir.is_dir(): continue + if "windows" in target_dir.name: + components_set.update(WINDOWS_ONLY_COMPONENTS.get(package, [])) + dest_target_dir = vendor_dest / target_dir.name dest_target_dir.mkdir(parents=True, exist_ok=True) diff --git a/codex-cli/scripts/install_native_deps.py b/codex-cli/scripts/install_native_deps.py index 8d3909c9e1..f2c3987b2d 100755 --- a/codex-cli/scripts/install_native_deps.py +++ b/codex-cli/scripts/install_native_deps.py @@ -36,8 +36,11 @@ class BinaryComponent: artifact_prefix: str # matches the artifact filename prefix (e.g. codex-.zst) dest_dir: str # directory under vendor// where the binary is installed binary_basename: str # executable name inside dest_dir (before optional .exe) + targets: tuple[str, ...] | None = None # limit installation to specific targets +WINDOWS_TARGETS = tuple(target for target in BINARY_TARGETS if "windows" in target) + BINARY_COMPONENTS = { "codex": BinaryComponent( artifact_prefix="codex", @@ -49,6 +52,18 @@ BINARY_COMPONENTS = { dest_dir="codex-responses-api-proxy", binary_basename="codex-responses-api-proxy", ), + "codex-windows-sandbox-setup": BinaryComponent( + artifact_prefix="codex-windows-sandbox-setup", + dest_dir="codex", + binary_basename="codex-windows-sandbox-setup", + targets=WINDOWS_TARGETS, + ), + "codex-command-runner": BinaryComponent( + artifact_prefix="codex-command-runner", + dest_dir="codex", + binary_basename="codex-command-runner", + targets=WINDOWS_TARGETS, + ), } RG_TARGET_PLATFORM_PAIRS: list[tuple[str, str]] = [ @@ -79,7 +94,8 @@ def parse_args() -> argparse.Namespace: choices=tuple(list(BINARY_COMPONENTS) + ["rg"]), help=( "Limit installation to the specified components." - " May be repeated. Defaults to 'codex' and 'rg'." + " May be repeated. Defaults to codex, codex-windows-sandbox-setup," + " codex-command-runner, and rg." ), ) parser.add_argument( @@ -101,7 +117,12 @@ def main() -> int: vendor_dir = codex_cli_root / VENDOR_DIR_NAME vendor_dir.mkdir(parents=True, exist_ok=True) - components = args.components or ["codex", "rg"] + components = args.components or [ + "codex", + "codex-windows-sandbox-setup", + "codex-command-runner", + "rg", + ] workflow_url = (args.workflow_url or DEFAULT_WORKFLOW_URL).strip() if not workflow_url: @@ -116,8 +137,7 @@ def main() -> int: install_binary_components( artifacts_dir, vendor_dir, - BINARY_TARGETS, - [name for name in components if name in BINARY_COMPONENTS], + [BINARY_COMPONENTS[name] for name in components if name in BINARY_COMPONENTS], ) if "rg" in components: @@ -206,23 +226,19 @@ def _download_artifacts(workflow_id: str, dest_dir: Path) -> None: def install_binary_components( artifacts_dir: Path, vendor_dir: Path, - targets: Iterable[str], - component_names: Sequence[str], + selected_components: Sequence[BinaryComponent], ) -> None: - selected_components = [BINARY_COMPONENTS[name] for name in component_names if name in BINARY_COMPONENTS] if not selected_components: return - targets = list(targets) - if not targets: - return - for component in selected_components: + component_targets = list(component.targets or BINARY_TARGETS) + print( f"Installing {component.binary_basename} binaries for targets: " - + ", ".join(targets) + + ", ".join(component_targets) ) - max_workers = min(len(targets), max(1, (os.cpu_count() or 1))) + max_workers = min(len(component_targets), max(1, (os.cpu_count() or 1))) with ThreadPoolExecutor(max_workers=max_workers) as executor: futures = { executor.submit( @@ -232,7 +248,7 @@ def install_binary_components( target, component, ): target - for target in targets + for target in component_targets } for future in as_completed(futures): installed_path = future.result() diff --git a/codex-rs/app-server-protocol/src/protocol/common.rs b/codex-rs/app-server-protocol/src/protocol/common.rs index dca9f03e75..bd7fd8e28c 100644 --- a/codex-rs/app-server-protocol/src/protocol/common.rs +++ b/codex-rs/app-server-protocol/src/protocol/common.rs @@ -144,9 +144,9 @@ client_request_definitions! { response: v2::McpServerOauthLoginResponse, }, - McpServersList => "mcpServers/list" { - params: v2::ListMcpServersParams, - response: v2::ListMcpServersResponse, + McpServerStatusList => "mcpServerStatus/list" { + params: v2::ListMcpServerStatusParams, + response: v2::ListMcpServerStatusResponse, }, LoginAccount => "account/login/start" { @@ -525,6 +525,8 @@ server_notification_definitions! { TurnPlanUpdated => "turn/plan/updated" (v2::TurnPlanUpdatedNotification), ItemStarted => "item/started" (v2::ItemStartedNotification), ItemCompleted => "item/completed" (v2::ItemCompletedNotification), + /// This event is internal-only. Used by Codex Cloud. + RawResponseItemCompleted => "rawResponseItem/completed" (v2::RawResponseItemCompletedNotification), AgentMessageDelta => "item/agentMessage/delta" (v2::AgentMessageDeltaNotification), CommandExecutionOutputDelta => "item/commandExecution/outputDelta" (v2::CommandExecutionOutputDeltaNotification), TerminalInteraction => "item/commandExecution/terminalInteraction" (v2::TerminalInteractionNotification), diff --git a/codex-rs/app-server-protocol/src/protocol/v2.rs b/codex-rs/app-server-protocol/src/protocol/v2.rs index 310a7a595b..19fa0c6633 100644 --- a/codex-rs/app-server-protocol/src/protocol/v2.rs +++ b/codex-rs/app-server-protocol/src/protocol/v2.rs @@ -770,7 +770,7 @@ pub struct ModelListResponse { #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema, TS)] #[serde(rename_all = "camelCase")] #[ts(export_to = "v2/")] -pub struct ListMcpServersParams { +pub struct ListMcpServerStatusParams { /// Opaque pagination cursor returned by a previous call. pub cursor: Option, /// Optional page size; defaults to a server-defined value. @@ -780,7 +780,7 @@ pub struct ListMcpServersParams { #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema, TS)] #[serde(rename_all = "camelCase")] #[ts(export_to = "v2/")] -pub struct McpServer { +pub struct McpServerStatus { pub name: String, pub tools: std::collections::HashMap, pub resources: Vec, @@ -791,8 +791,8 @@ pub struct McpServer { #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema, TS)] #[serde(rename_all = "camelCase")] #[ts(export_to = "v2/")] -pub struct ListMcpServersResponse { - pub data: Vec, +pub struct ListMcpServerStatusResponse { + pub data: Vec, /// Opaque cursor to pass to the next call to continue after the last item. /// If None, there are no more items to return. pub next_cursor: Option, @@ -869,6 +869,12 @@ pub struct ThreadStartParams { pub config: Option>, pub base_instructions: Option, pub developer_instructions: Option, + /// If true, opt into emitting raw response items on the event stream. + /// + /// This is for internal use only (e.g. Codex Cloud). + /// (TODO): Figure out a better way to categorize internal / experimental events & protocols. + #[serde(default)] + pub experimental_raw_events: bool, } #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema, TS)] @@ -990,6 +996,7 @@ pub struct SkillsListResponse { pub enum SkillScope { User, Repo, + Public, } #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema, TS)] @@ -1035,6 +1042,7 @@ impl From for SkillScope { match value { CoreSkillScope::User => Self::User, CoreSkillScope::Repo => Self::Repo, + CoreSkillScope::Public => Self::Public, } } } @@ -1590,6 +1598,15 @@ pub struct ItemCompletedNotification { pub turn_id: String, } +#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema, TS)] +#[serde(rename_all = "camelCase")] +#[ts(export_to = "v2/")] +pub struct RawResponseItemCompletedNotification { + pub thread_id: String, + pub turn_id: String, + pub item: ResponseItem, +} + // Item-specific progress notifications #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema, TS)] #[serde(rename_all = "camelCase")] diff --git a/codex-rs/app-server/README.md b/codex-rs/app-server/README.md index 80e882adfc..460418c81e 100644 --- a/codex-rs/app-server/README.md +++ b/codex-rs/app-server/README.md @@ -3,6 +3,7 @@ `codex app-server` is the interface Codex uses to power rich interfaces such as the [Codex VS Code extension](https://marketplace.visualstudio.com/items?itemName=openai.chatgpt). ## Table of Contents + - [Protocol](#protocol) - [Message Schema](#message-schema) - [Core Primitives](#core-primitives) @@ -28,6 +29,7 @@ codex app-server generate-json-schema --out DIR ## Core Primitives The API exposes three top level primitives representing an interaction between a user and Codex: + - **Thread**: A conversation between a user and the Codex agent. Each thread contains multiple turns. - **Turn**: One turn of the conversation, typically starting with a user message and finishing with an agent message. Each turn contains multiple items. - **Item**: Represents user inputs and agent outputs as part of the turn, persisted and used as the context for future conversations. Example items include user message, agent reasoning, agent message, shell command, file edit, etc. @@ -49,13 +51,23 @@ Clients must send a single `initialize` request before invoking any other method Applications building on top of `codex app-server` should identify themselves via the `clientInfo` parameter. Example (from OpenAI's official VSCode extension): + ```json -{ "method": "initialize", "id": 0, "params": { - "clientInfo": { "name": "codex-vscode", "title": "Codex VS Code Extension", "version": "0.1.0" } -} } +{ + "method": "initialize", + "id": 0, + "params": { + "clientInfo": { + "name": "codex-vscode", + "title": "Codex VS Code Extension", + "version": "0.1.0" + } + } +} ``` ## API Overview + - `thread/start` — create a new thread; emits `thread/started` and auto-subscribes you to turn/item events for that thread. - `thread/resume` — reopen an existing thread by id so subsequent `turn/start` calls append to it. - `thread/list` — page through stored rollouts; supports cursor-based pagination and optional `modelProviders` filtering. @@ -67,7 +79,7 @@ Example (from OpenAI's official VSCode extension): - `model/list` — list available models (with reasoning effort options). - `skills/list` — list skills for one or more `cwd` values. - `mcpServer/oauth/login` — start an OAuth login for a configured MCP server; returns an `authorization_url` and later emits `mcpServer/oauthLogin/completed` once the browser flow finishes. -- `mcpServers/list` — enumerate configured MCP servers with their tools, resources, resource templates, and auth status; supports cursor+limit pagination. +- `mcpServerStatus/list` — enumerate configured MCP servers with their tools, resources, resource templates, and auth status; supports cursor+limit pagination. - `feedback/upload` — submit a feedback report (classification + optional reason/logs and conversation_id); returns the tracking thread id. - `command/exec` — run a single command under the server sandbox without starting a thread/turn (handy for utilities and validation). - `config/read` — fetch the effective config on disk after resolving config layering. @@ -108,6 +120,7 @@ To continue a stored session, call `thread/resume` with the `thread.id` you prev ### Example: List threads (with pagination & filters) `thread/list` lets you render a history UI. Pass any combination of: + - `cursor` — opaque string from a prior response; omit for the first page. - `limit` — server defaults to a reasonable page size if unset. - `modelProviders` — restrict results to specific providers; unset, null, or an empty array will include all providers. @@ -228,22 +241,32 @@ Codex streams the usual `turn/started` notification followed by an `item/started with an `enteredReviewMode` item so clients can show progress: ```json -{ "method": "item/started", "params": { "item": { - "type": "enteredReviewMode", - "id": "turn_900", - "review": "current changes" -} } } +{ + "method": "item/started", + "params": { + "item": { + "type": "enteredReviewMode", + "id": "turn_900", + "review": "current changes" + } + } +} ``` When the reviewer finishes, the server emits `item/started` and `item/completed` containing an `exitedReviewMode` item with the final review text: ```json -{ "method": "item/completed", "params": { "item": { - "type": "exitedReviewMode", - "id": "turn_900", - "review": "Looks solid overall...\n\n- Prefer Stylize helpers — app.rs:10-20\n ..." -} } } +{ + "method": "item/completed", + "params": { + "item": { + "type": "exitedReviewMode", + "id": "turn_900", + "review": "Looks solid overall...\n\n- Prefer Stylize helpers — app.rs:10-20\n ..." + } + } +} ``` The `review` string is plain text that already bundles the overall explanation plus a bullet list for each structured finding (matching `ThreadItem::ExitedReviewMode` in the generated schema). Use this notification to render the reviewer output in your client. @@ -263,6 +286,7 @@ Run a standalone command (argv vector) in the server’s sandbox without creatin ``` Notes: + - Empty `command` arrays are rejected. - `sandboxPolicy` accepts the same shape used by `turn/start` (e.g., `dangerFullAccess`, `readOnly`, `workspaceWrite` with flags). - When omitted, `timeoutMs` falls back to the server default. @@ -285,6 +309,7 @@ Today both notifications carry an empty `items` array even when item events were #### Items `ThreadItem` is the tagged union carried in turn responses and `item/*` notifications. Currently we support events for the following items: + - `userMessage` — `{id, content}` where `content` is a list of user inputs (`text`, `image`, or `localImage`). - `agentMessage` — `{id, text}` containing the accumulated agent reply. - `reasoning` — `{id, summary, content}` where `summary` holds streamed reasoning summaries (applicable for most OpenAI models) and `content` holds raw reasoning blocks (applicable for e.g. open source models). @@ -298,37 +323,48 @@ Today both notifications carry an empty `items` array even when item events were - `compacted` - `{threadId, turnId}` when codex compacts the conversation history. This can happen automatically. All items emit two shared lifecycle events: + - `item/started` — emits the full `item` when a new unit of work begins so the UI can render it immediately; the `item.id` in this payload matches the `itemId` used by deltas. - `item/completed` — sends the final `item` once that work finishes (e.g., after a tool call or message completes); treat this as the authoritative state. There are additional item-specific events: + #### agentMessage + - `item/agentMessage/delta` — appends streamed text for the agent message; concatenate `delta` values for the same `itemId` in order to reconstruct the full reply. + #### reasoning + - `item/reasoning/summaryTextDelta` — streams readable reasoning summaries; `summaryIndex` increments when a new summary section opens. - `item/reasoning/summaryPartAdded` — marks the boundary between reasoning summary sections for an `itemId`; subsequent `summaryTextDelta` entries share the same `summaryIndex`. - `item/reasoning/textDelta` — streams raw reasoning text (only applicable for e.g. open source models); use `contentIndex` to group deltas that belong together before showing them in the UI. + #### commandExecution + - `item/commandExecution/outputDelta` — streams stdout/stderr for the command; append deltas in order to render live output alongside `aggregatedOutput` in the final item. -Final `commandExecution` items include parsed `commandActions`, `status`, `exitCode`, and `durationMs` so the UI can summarize what ran and whether it succeeded. + Final `commandExecution` items include parsed `commandActions`, `status`, `exitCode`, and `durationMs` so the UI can summarize what ran and whether it succeeded. + #### fileChange + - `item/fileChange/outputDelta` - contains the tool call response of the underlying `apply_patch` tool call. ### Errors + `error` event is emitted whenever the server hits an error mid-turn (for example, upstream model errors or quota limits). Carries the same `{ error: { message, codexErrorInfo? } }` payload as `turn.status: "failed"` and may precede that terminal notification. - `codexErrorInfo` maps to the `CodexErrorInfo` enum. Common values: - - `ContextWindowExceeded` - - `UsageLimitExceeded` - - `HttpConnectionFailed { httpStatusCode? }`: upstream HTTP failures including 4xx/5xx - - `ResponseStreamConnectionFailed { httpStatusCode? }`: failure to connect to the response SSE stream - - `ResponseStreamDisconnected { httpStatusCode? }`: disconnect of the response SSE stream in the middle of a turn before completion - - `ResponseTooManyFailedAttempts { httpStatusCode? }` - - `BadRequest` - - `Unauthorized` - - `SandboxError` - - `InternalServerError` - - `Other`: all unclassified errors +`codexErrorInfo` maps to the `CodexErrorInfo` enum. Common values: + +- `ContextWindowExceeded` +- `UsageLimitExceeded` +- `HttpConnectionFailed { httpStatusCode? }`: upstream HTTP failures including 4xx/5xx +- `ResponseStreamConnectionFailed { httpStatusCode? }`: failure to connect to the response SSE stream +- `ResponseStreamDisconnected { httpStatusCode? }`: disconnect of the response SSE stream in the middle of a turn before completion +- `ResponseTooManyFailedAttempts { httpStatusCode? }` +- `BadRequest` +- `Unauthorized` +- `SandboxError` +- `InternalServerError` +- `Other`: all unclassified errors When an upstream HTTP status is available (for example, from the Responses API or a provider), it is forwarded in `httpStatusCode` on the relevant `codexErrorInfo` variant. @@ -342,6 +378,7 @@ Certain actions (shell commands or modifying files) may require explicit user ap ### Command execution approvals Order of messages: + 1. `item/started` — shows the pending `commandExecution` item with `command`, `cwd`, and other fields so you can render the proposed action. 2. `item/commandExecution/requestApproval` (request) — carries the same `itemId`, `threadId`, `turnId`, optionally `reason` or `risk`, plus `parsedCmd` for friendly display. 3. Client response — `{ "decision": "accept", "acceptSettings": { "forSession": false } }` or `{ "decision": "decline" }`. @@ -350,6 +387,7 @@ Order of messages: ### File change approvals Order of messages: + 1. `item/started` — emits a `fileChange` item with `changes` (diff chunk summaries) and `status: "inProgress"`. Show the proposed edits and paths to the user. 2. `item/fileChange/requestApproval` (request) — includes `itemId`, `threadId`, `turnId`, and an optional `reason`. 3. Client response — `{ "decision": "accept" }` or `{ "decision": "decline" }`. @@ -362,6 +400,7 @@ UI guidance for IDEs: surface an approval dialog as soon as the request arrives. The JSON-RPC auth/account surface exposes request/response methods plus server-initiated notifications (no `id`). Use these to determine auth state, start or cancel logins, logout, and inspect ChatGPT rate limits. ### API Overview + - `account/read` — fetch current account info; optionally refresh tokens. - `account/login/start` — begin login (`apiKey` or `chatgpt`). - `account/login/completed` (notify) — emitted when a login attempt finishes (success or error). @@ -375,11 +414,13 @@ The JSON-RPC auth/account surface exposes request/response methods plus server-i ### 1) Check auth state Request: + ```json { "method": "account/read", "id": 1, "params": { "refreshToken": false } } ``` Response examples: + ```json { "id": 1, "result": { "account": null, "requiresOpenaiAuth": false } } // No OpenAI auth needed (e.g., OSS/local models) { "id": 1, "result": { "account": null, "requiresOpenaiAuth": true } } // OpenAI auth required (typical for OpenAI-hosted models) @@ -388,6 +429,7 @@ Response examples: ``` Field notes: + - `refreshToken` (bool): set `true` to force a token refresh. - `requiresOpenaiAuth` reflects the active provider; when `false`, Codex can run without OpenAI credentials. @@ -395,7 +437,11 @@ Field notes: 1. Send: ```json - { "method": "account/login/start", "id": 2, "params": { "type": "apiKey", "apiKey": "sk-…" } } + { + "method": "account/login/start", + "id": 2, + "params": { "type": "apiKey", "apiKey": "sk-…" } + } ``` 2. Expect: ```json @@ -445,6 +491,7 @@ Field notes: ``` Field notes: + - `usedPercent` is current usage within the OpenAI quota window. - `windowDurationMins` is the quota window length. - `resetsAt` is a Unix timestamp (seconds) for the next reset. diff --git a/codex-rs/app-server/src/bespoke_event_handling.rs b/codex-rs/app-server/src/bespoke_event_handling.rs index b0161cd9fd..dec9d8c089 100644 --- a/codex-rs/app-server/src/bespoke_event_handling.rs +++ b/codex-rs/app-server/src/bespoke_event_handling.rs @@ -31,6 +31,7 @@ use codex_app_server_protocol::McpToolCallResult; use codex_app_server_protocol::McpToolCallStatus; use codex_app_server_protocol::PatchApplyStatus; use codex_app_server_protocol::PatchChangeKind as V2PatchChangeKind; +use codex_app_server_protocol::RawResponseItemCompletedNotification; use codex_app_server_protocol::ReasoningSummaryPartAddedNotification; use codex_app_server_protocol::ReasoningSummaryTextDeltaNotification; use codex_app_server_protocol::ReasoningTextDeltaNotification; @@ -451,6 +452,16 @@ pub(crate) async fn apply_bespoke_event_handling( .send_server_notification(ServerNotification::ItemCompleted(completed)) .await; } + EventMsg::RawResponseItem(raw_response_item_event) => { + maybe_emit_raw_response_item_completed( + api_version, + conversation_id, + &event_turn_id, + raw_response_item_event.item, + outgoing.as_ref(), + ) + .await; + } EventMsg::PatchApplyBegin(patch_begin_event) => { // Until we migrate the core to be aware of a first class FileChangeItem // and emit the corresponding EventMsg, we repurpose the call_id as the item_id. @@ -820,6 +831,27 @@ async fn complete_command_execution_item( .await; } +async fn maybe_emit_raw_response_item_completed( + api_version: ApiVersion, + conversation_id: ConversationId, + turn_id: &str, + item: codex_protocol::models::ResponseItem, + outgoing: &OutgoingMessageSender, +) { + let ApiVersion::V2 = api_version else { + return; + }; + + let notification = RawResponseItemCompletedNotification { + thread_id: conversation_id.to_string(), + turn_id: turn_id.to_string(), + item, + }; + outgoing + .send_server_notification(ServerNotification::RawResponseItemCompleted(notification)) + .await; +} + async fn find_and_remove_turn_summary( conversation_id: ConversationId, turn_summary_store: &TurnSummaryStore, diff --git a/codex-rs/app-server/src/codex_message_processor.rs b/codex-rs/app-server/src/codex_message_processor.rs index d017d22943..412b1c7d62 100644 --- a/codex-rs/app-server/src/codex_message_processor.rs +++ b/codex-rs/app-server/src/codex_message_processor.rs @@ -46,8 +46,8 @@ use codex_app_server_protocol::InterruptConversationParams; use codex_app_server_protocol::JSONRPCErrorError; use codex_app_server_protocol::ListConversationsParams; use codex_app_server_protocol::ListConversationsResponse; -use codex_app_server_protocol::ListMcpServersParams; -use codex_app_server_protocol::ListMcpServersResponse; +use codex_app_server_protocol::ListMcpServerStatusParams; +use codex_app_server_protocol::ListMcpServerStatusResponse; use codex_app_server_protocol::LoginAccountParams; use codex_app_server_protocol::LoginApiKeyParams; use codex_app_server_protocol::LoginApiKeyResponse; @@ -55,10 +55,10 @@ use codex_app_server_protocol::LoginChatGptCompleteNotification; use codex_app_server_protocol::LoginChatGptResponse; use codex_app_server_protocol::LogoutAccountResponse; use codex_app_server_protocol::LogoutChatGptResponse; -use codex_app_server_protocol::McpServer; use codex_app_server_protocol::McpServerOauthLoginCompletedNotification; use codex_app_server_protocol::McpServerOauthLoginParams; use codex_app_server_protocol::McpServerOauthLoginResponse; +use codex_app_server_protocol::McpServerStatus; use codex_app_server_protocol::ModelListParams; use codex_app_server_protocol::ModelListResponse; use codex_app_server_protocol::NewConversationParams; @@ -398,8 +398,8 @@ impl CodexMessageProcessor { ClientRequest::McpServerOauthLogin { request_id, params } => { self.mcp_server_oauth_login(request_id, params).await; } - ClientRequest::McpServersList { request_id, params } => { - self.list_mcp_servers(request_id, params).await; + ClientRequest::McpServerStatusList { request_id, params } => { + self.list_mcp_server_status(request_id, params).await; } ClientRequest::LoginAccount { request_id, params } => { self.login_v2(request_id, params).await; @@ -1373,9 +1373,13 @@ impl CodexMessageProcessor { }; // Auto-attach a conversation listener when starting a thread. - // Use the same behavior as the v1 API with experimental_raw_events=false. + // Use the same behavior as the v1 API, with opt-in support for raw item events. if let Err(err) = self - .attach_conversation_listener(conversation_id, false, ApiVersion::V2) + .attach_conversation_listener( + conversation_id, + params.experimental_raw_events, + ApiVersion::V2, + ) .await { tracing::warn!( @@ -2052,7 +2056,12 @@ impl CodexMessageProcessor { } } - async fn list_mcp_servers(&self, request_id: RequestId, params: ListMcpServersParams) { + async fn list_mcp_server_status( + &self, + request_id: RequestId, + params: ListMcpServerStatusParams, + ) { + let outgoing = Arc::clone(&self.outgoing); let config = match self.load_latest_config().await { Ok(config) => config, Err(error) => { @@ -2061,6 +2070,17 @@ impl CodexMessageProcessor { } }; + tokio::spawn(async move { + Self::list_mcp_server_status_task(outgoing, request_id, params, config).await; + }); + } + + async fn list_mcp_server_status_task( + outgoing: Arc, + request_id: RequestId, + params: ListMcpServerStatusParams, + config: Config, + ) { let snapshot = collect_mcp_snapshot(&config).await; let tools_by_server = group_tools_by_server(&snapshot.tools); @@ -2088,7 +2108,7 @@ impl CodexMessageProcessor { message: format!("invalid cursor: {cursor}"), data: None, }; - self.outgoing.send_error(request_id, error).await; + outgoing.send_error(request_id, error).await; return; } }, @@ -2101,15 +2121,15 @@ impl CodexMessageProcessor { message: format!("cursor {start} exceeds total MCP servers {total}"), data: None, }; - self.outgoing.send_error(request_id, error).await; + outgoing.send_error(request_id, error).await; return; } let end = start.saturating_add(effective_limit).min(total); - let data: Vec = server_names[start..end] + let data: Vec = server_names[start..end] .iter() - .map(|name| McpServer { + .map(|name| McpServerStatus { name: name.clone(), tools: tools_by_server.get(name).cloned().unwrap_or_default(), resources: snapshot.resources.get(name).cloned().unwrap_or_default(), @@ -2133,9 +2153,9 @@ impl CodexMessageProcessor { None }; - let response = ListMcpServersResponse { data, next_cursor }; + let response = ListMcpServerStatusResponse { data, next_cursor }; - self.outgoing.send_response(request_id, response).await; + outgoing.send_response(request_id, response).await; } async fn handle_resume_conversation( diff --git a/codex-rs/apply-patch/tests/fixtures/scenarios/005_rejects_empty_patch/expected/foo.txt b/codex-rs/apply-patch/tests/fixtures/scenarios/005_rejects_empty_patch/expected/foo.txt new file mode 100644 index 0000000000..2bf5ad0447 --- /dev/null +++ b/codex-rs/apply-patch/tests/fixtures/scenarios/005_rejects_empty_patch/expected/foo.txt @@ -0,0 +1 @@ +stable diff --git a/codex-rs/apply-patch/tests/fixtures/scenarios/005_rejects_empty_patch/input/foo.txt b/codex-rs/apply-patch/tests/fixtures/scenarios/005_rejects_empty_patch/input/foo.txt new file mode 100644 index 0000000000..2bf5ad0447 --- /dev/null +++ b/codex-rs/apply-patch/tests/fixtures/scenarios/005_rejects_empty_patch/input/foo.txt @@ -0,0 +1 @@ +stable diff --git a/codex-rs/apply-patch/tests/fixtures/scenarios/007_rejects_missing_file_delete/expected/foo.txt b/codex-rs/apply-patch/tests/fixtures/scenarios/007_rejects_missing_file_delete/expected/foo.txt new file mode 100644 index 0000000000..2bf5ad0447 --- /dev/null +++ b/codex-rs/apply-patch/tests/fixtures/scenarios/007_rejects_missing_file_delete/expected/foo.txt @@ -0,0 +1 @@ +stable diff --git a/codex-rs/apply-patch/tests/fixtures/scenarios/007_rejects_missing_file_delete/input/foo.txt b/codex-rs/apply-patch/tests/fixtures/scenarios/007_rejects_missing_file_delete/input/foo.txt new file mode 100644 index 0000000000..2bf5ad0447 --- /dev/null +++ b/codex-rs/apply-patch/tests/fixtures/scenarios/007_rejects_missing_file_delete/input/foo.txt @@ -0,0 +1 @@ +stable diff --git a/codex-rs/apply-patch/tests/fixtures/scenarios/008_rejects_empty_update_hunk/expected/foo.txt b/codex-rs/apply-patch/tests/fixtures/scenarios/008_rejects_empty_update_hunk/expected/foo.txt new file mode 100644 index 0000000000..2bf5ad0447 --- /dev/null +++ b/codex-rs/apply-patch/tests/fixtures/scenarios/008_rejects_empty_update_hunk/expected/foo.txt @@ -0,0 +1 @@ +stable diff --git a/codex-rs/apply-patch/tests/fixtures/scenarios/008_rejects_empty_update_hunk/input/foo.txt b/codex-rs/apply-patch/tests/fixtures/scenarios/008_rejects_empty_update_hunk/input/foo.txt new file mode 100644 index 0000000000..2bf5ad0447 --- /dev/null +++ b/codex-rs/apply-patch/tests/fixtures/scenarios/008_rejects_empty_update_hunk/input/foo.txt @@ -0,0 +1 @@ +stable diff --git a/codex-rs/apply-patch/tests/fixtures/scenarios/009_requires_existing_file_for_update/expected/foo.txt b/codex-rs/apply-patch/tests/fixtures/scenarios/009_requires_existing_file_for_update/expected/foo.txt new file mode 100644 index 0000000000..2bf5ad0447 --- /dev/null +++ b/codex-rs/apply-patch/tests/fixtures/scenarios/009_requires_existing_file_for_update/expected/foo.txt @@ -0,0 +1 @@ +stable diff --git a/codex-rs/apply-patch/tests/fixtures/scenarios/009_requires_existing_file_for_update/input/foo.txt b/codex-rs/apply-patch/tests/fixtures/scenarios/009_requires_existing_file_for_update/input/foo.txt new file mode 100644 index 0000000000..2bf5ad0447 --- /dev/null +++ b/codex-rs/apply-patch/tests/fixtures/scenarios/009_requires_existing_file_for_update/input/foo.txt @@ -0,0 +1 @@ +stable diff --git a/codex-rs/apply-patch/tests/fixtures/scenarios/012_delete_directory_fails/expected/dir/foo.txt b/codex-rs/apply-patch/tests/fixtures/scenarios/012_delete_directory_fails/expected/dir/foo.txt new file mode 100644 index 0000000000..2bf5ad0447 --- /dev/null +++ b/codex-rs/apply-patch/tests/fixtures/scenarios/012_delete_directory_fails/expected/dir/foo.txt @@ -0,0 +1 @@ +stable diff --git a/codex-rs/apply-patch/tests/fixtures/scenarios/012_delete_directory_fails/input/dir/foo.txt b/codex-rs/apply-patch/tests/fixtures/scenarios/012_delete_directory_fails/input/dir/foo.txt new file mode 100644 index 0000000000..2bf5ad0447 --- /dev/null +++ b/codex-rs/apply-patch/tests/fixtures/scenarios/012_delete_directory_fails/input/dir/foo.txt @@ -0,0 +1 @@ +stable diff --git a/codex-rs/apply-patch/tests/fixtures/scenarios/013_rejects_invalid_hunk_header/expected/foo.txt b/codex-rs/apply-patch/tests/fixtures/scenarios/013_rejects_invalid_hunk_header/expected/foo.txt new file mode 100644 index 0000000000..2bf5ad0447 --- /dev/null +++ b/codex-rs/apply-patch/tests/fixtures/scenarios/013_rejects_invalid_hunk_header/expected/foo.txt @@ -0,0 +1 @@ +stable diff --git a/codex-rs/apply-patch/tests/fixtures/scenarios/013_rejects_invalid_hunk_header/input/foo.txt b/codex-rs/apply-patch/tests/fixtures/scenarios/013_rejects_invalid_hunk_header/input/foo.txt new file mode 100644 index 0000000000..2bf5ad0447 --- /dev/null +++ b/codex-rs/apply-patch/tests/fixtures/scenarios/013_rejects_invalid_hunk_header/input/foo.txt @@ -0,0 +1 @@ +stable diff --git a/codex-rs/apply-patch/tests/fixtures/scenarios/019_unicode_simple/expected/foo.txt b/codex-rs/apply-patch/tests/fixtures/scenarios/019_unicode_simple/expected/foo.txt new file mode 100644 index 0000000000..99d5a6e9c2 --- /dev/null +++ b/codex-rs/apply-patch/tests/fixtures/scenarios/019_unicode_simple/expected/foo.txt @@ -0,0 +1,3 @@ +line1 +naïve café ✅ +line3 diff --git a/codex-rs/apply-patch/tests/fixtures/scenarios/019_unicode_simple/input/foo.txt b/codex-rs/apply-patch/tests/fixtures/scenarios/019_unicode_simple/input/foo.txt new file mode 100644 index 0000000000..b170948713 --- /dev/null +++ b/codex-rs/apply-patch/tests/fixtures/scenarios/019_unicode_simple/input/foo.txt @@ -0,0 +1,3 @@ +line1 +naïve café +line3 diff --git a/codex-rs/apply-patch/tests/fixtures/scenarios/019_unicode_simple/patch.txt b/codex-rs/apply-patch/tests/fixtures/scenarios/019_unicode_simple/patch.txt new file mode 100644 index 0000000000..9514207fde --- /dev/null +++ b/codex-rs/apply-patch/tests/fixtures/scenarios/019_unicode_simple/patch.txt @@ -0,0 +1,7 @@ +*** Begin Patch +*** Update File: foo.txt +@@ + line1 +-naïve café ++naïve café ✅ +*** End Patch diff --git a/codex-rs/cli/src/main.rs b/codex-rs/cli/src/main.rs index 113c6a7515..e29e5a5e50 100644 --- a/codex-rs/cli/src/main.rs +++ b/codex-rs/cli/src/main.rs @@ -410,7 +410,7 @@ fn stage_str(stage: codex_core::features::Stage) -> &'static str { use codex_core::features::Stage; match stage { Stage::Experimental => "experimental", - Stage::Beta => "beta", + Stage::Beta { .. } => "beta", Stage::Stable => "stable", Stage::Deprecated => "deprecated", Stage::Removed => "removed", diff --git a/codex-rs/codex-api/src/endpoint/responses.rs b/codex-rs/codex-api/src/endpoint/responses.rs index a300b5a70d..487ed55b80 100644 --- a/codex-rs/codex-api/src/endpoint/responses.rs +++ b/codex-rs/codex-api/src/endpoint/responses.rs @@ -32,6 +32,7 @@ pub struct ResponsesOptions { pub store_override: Option, pub conversation_id: Option, pub session_source: Option, + pub extra_headers: HeaderMap, } impl ResponsesClient { @@ -73,6 +74,7 @@ impl ResponsesClient { store_override, conversation_id, session_source, + extra_headers, } = options; let request = ResponsesRequestBuilder::new(model, &prompt.instructions, &prompt.input) @@ -85,6 +87,7 @@ impl ResponsesClient { .conversation(conversation_id) .session_source(session_source) .store_override(store_override) + .extra_headers(extra_headers) .build(self.streaming.provider())?; self.stream_request(request).await diff --git a/codex-rs/common/src/config_summary.rs b/codex-rs/common/src/config_summary.rs index 5a5901880f..2254eeae85 100644 --- a/codex-rs/common/src/config_summary.rs +++ b/codex-rs/common/src/config_summary.rs @@ -9,7 +9,7 @@ pub fn create_config_summary_entries(config: &Config, model: &str) -> Vec<(&'sta ("workdir", config.cwd.display().to_string()), ("model", model.to_string()), ("provider", config.model_provider_id.clone()), - ("approval", config.approval_policy.to_string()), + ("approval", config.approval_policy.value().to_string()), ("sandbox", summarize_sandbox_policy(&config.sandbox_policy)), ]; if config.model_provider.wire_api == WireApi::Responses { diff --git a/codex-rs/core/src/client.rs b/codex-rs/core/src/client.rs index 8ab56019c6..aaf3b0ea35 100644 --- a/codex-rs/core/src/client.rs +++ b/codex-rs/core/src/client.rs @@ -45,6 +45,7 @@ use crate::config::Config; use crate::default_client::build_reqwest_client; use crate::error::CodexErr; use crate::error::Result; +use crate::features::FEATURES; use crate::flags::CODEX_RS_SSE_FIXTURE; use crate::model_provider_info::ModelProviderInfo; use crate::model_provider_info::WireApi; @@ -261,6 +262,7 @@ impl ModelClient { store_override: None, conversation_id: Some(conversation_id.clone()), session_source: Some(session_source.clone()), + extra_headers: beta_feature_headers(&self.config), }; let stream_result = client @@ -396,6 +398,27 @@ fn build_api_prompt(prompt: &Prompt, instructions: String, tools_json: Vec ApiHeaderMap { + let enabled = FEATURES + .iter() + .filter_map(|spec| { + if spec.stage.beta_menu_description().is_some() && config.features.enabled(spec.id) { + Some(spec.key) + } else { + None + } + }) + .collect::>(); + let value = enabled.join(","); + let mut headers = ApiHeaderMap::new(); + if !value.is_empty() + && let Ok(header_value) = HeaderValue::from_str(value.as_str()) + { + headers.insert("x-codex-beta-features", header_value); + } + headers +} + fn map_response_stream(api_stream: S, otel_manager: OtelManager) -> ResponseStream where S: futures::Stream> diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index 5f0e322dae..71be4bb0f3 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -77,6 +77,9 @@ use crate::client_common::Prompt; use crate::client_common::ResponseEvent; use crate::compact::collect_user_messages; use crate::config::Config; +use crate::config::Constrained; +use crate::config::ConstraintError; +use crate::config::ConstraintResult; use crate::config::GhostSnapshotConfig; use crate::config::types::ShellEnvironmentPolicy; use crate::context_manager::ContextManager; @@ -96,6 +99,7 @@ use crate::protocol::ApplyPatchApprovalRequestEvent; use crate::protocol::AskForApproval; use crate::protocol::BackgroundEventEvent; use crate::protocol::DeprecationNoticeEvent; +use crate::protocol::ErrorEvent; use crate::protocol::Event; use crate::protocol::EventMsg; use crate::protocol::ExecApprovalRequestEvent; @@ -260,7 +264,7 @@ impl Codex { user_instructions, base_instructions: config.base_instructions.clone(), compact_prompt: config.compact_prompt.clone(), - approval_policy: config.approval_policy, + approval_policy: config.approval_policy.clone(), sandbox_policy: config.sandbox_policy.clone(), cwd: config.cwd.clone(), original_config_do_not_use: Arc::clone(&config), @@ -411,7 +415,7 @@ pub(crate) struct SessionConfiguration { compact_prompt: Option, /// When to escalate for approval for execution - approval_policy: AskForApproval, + approval_policy: Constrained, /// How to sandbox commands executed in the system sandbox_policy: SandboxPolicy, @@ -434,7 +438,7 @@ pub(crate) struct SessionConfiguration { } impl SessionConfiguration { - pub(crate) fn apply(&self, updates: &SessionSettingsUpdate) -> Self { + pub(crate) fn apply(&self, updates: &SessionSettingsUpdate) -> ConstraintResult { let mut next_configuration = self.clone(); if let Some(model) = updates.model.clone() { next_configuration.model = model; @@ -446,7 +450,7 @@ impl SessionConfiguration { next_configuration.model_reasoning_summary = summary; } if let Some(approval_policy) = updates.approval_policy { - next_configuration.approval_policy = approval_policy; + next_configuration.approval_policy.set(approval_policy)?; } if let Some(sandbox_policy) = updates.sandbox_policy.clone() { next_configuration.sandbox_policy = sandbox_policy; @@ -454,7 +458,7 @@ impl SessionConfiguration { if let Some(cwd) = updates.cwd.clone() { next_configuration.cwd = cwd; } - next_configuration + Ok(next_configuration) } } @@ -523,7 +527,7 @@ impl Session { base_instructions: session_configuration.base_instructions.clone(), compact_prompt: session_configuration.compact_prompt.clone(), user_instructions: session_configuration.user_instructions.clone(), - approval_policy: session_configuration.approval_policy, + approval_policy: session_configuration.approval_policy.value(), sandbox_policy: session_configuration.sandbox_policy.clone(), shell_environment_policy: per_turn_config.shell_environment_policy.clone(), tools_config, @@ -640,7 +644,7 @@ impl Session { config.model_reasoning_summary, config.model_context_window, config.model_auto_compact_token_limit, - config.approval_policy, + config.approval_policy.value(), config.sandbox_policy.clone(), config.mcp_servers.keys().map(String::as_str).collect(), config.active_profile.clone(), @@ -690,7 +694,7 @@ impl Session { session_id: conversation_id, model: session_configuration.model.clone(), model_provider_id: config.model_provider_id.clone(), - approval_policy: session_configuration.approval_policy, + approval_policy: session_configuration.approval_policy.value(), sandbox_policy: session_configuration.sandbox_policy.clone(), cwd: session_configuration.cwd.clone(), reasoning_effort: session_configuration.model_reasoning_effort, @@ -729,6 +733,30 @@ impl Session { // record_initial_history can emit events. We record only after the SessionConfiguredEvent is emitted. sess.record_initial_history(initial_history).await; + if sess.enabled(Feature::Skills) { + let mut rx = sess + .services + .skills_manager + .subscribe_skills_update_notifications(); + let sess = Arc::downgrade(&sess); + tokio::spawn(async move { + loop { + match rx.recv().await { + Ok(()) => { + let Some(sess) = sess.upgrade() else { + break; + }; + let turn_context = sess.new_default_turn().await; + sess.send_event(turn_context.as_ref(), EventMsg::SkillsUpdateAvailable) + .await; + } + Err(tokio::sync::broadcast::error::RecvError::Lagged(_)) => continue, + Err(tokio::sync::broadcast::error::RecvError::Closed) => break, + } + } + }); + } + Ok(sess) } @@ -762,7 +790,7 @@ impl Session { } async fn record_initial_history(&self, conversation_history: InitialHistory) { - let turn_context = self.new_turn(SessionSettingsUpdate::default()).await; + let turn_context = self.new_default_turn().await; match conversation_history { InitialHistory::New => { // Build and record initial items (user instructions + environment context) @@ -821,30 +849,76 @@ impl Session { } } - pub(crate) async fn update_settings(&self, updates: SessionSettingsUpdate) { + pub(crate) async fn update_settings( + &self, + updates: SessionSettingsUpdate, + ) -> ConstraintResult<()> { let mut state = self.state.lock().await; - state.session_configuration = state.session_configuration.apply(&updates); - } - - pub(crate) async fn new_turn(&self, updates: SessionSettingsUpdate) -> Arc { - let sub_id = self.next_internal_sub_id(); - self.new_turn_with_sub_id(sub_id, updates).await + match state.session_configuration.apply(&updates) { + Ok(updated) => { + state.session_configuration = updated; + Ok(()) + } + Err(err) => { + let wrapped = ConstraintError { + message: format!("Could not update config: {err}"), + }; + warn!(%wrapped, "rejected session settings update"); + Err(wrapped) + } + } } pub(crate) async fn new_turn_with_sub_id( &self, sub_id: String, updates: SessionSettingsUpdate, - ) -> Arc { + ) -> ConstraintResult> { let (session_configuration, sandbox_policy_changed) = { let mut state = self.state.lock().await; - let session_configuration = state.session_configuration.clone().apply(&updates); - let sandbox_policy_changed = - state.session_configuration.sandbox_policy != session_configuration.sandbox_policy; - state.session_configuration = session_configuration.clone(); - (session_configuration, sandbox_policy_changed) + match state.session_configuration.clone().apply(&updates) { + Ok(next) => { + let sandbox_policy_changed = + state.session_configuration.sandbox_policy != next.sandbox_policy; + state.session_configuration = next.clone(); + (next, sandbox_policy_changed) + } + Err(err) => { + drop(state); + let wrapped = ConstraintError { + message: format!("Could not update config: {err}"), + }; + self.send_event_raw(Event { + id: sub_id.clone(), + msg: EventMsg::Error(ErrorEvent { + message: wrapped.to_string(), + codex_error_info: Some(CodexErrorInfo::BadRequest), + }), + }) + .await; + return Err(wrapped); + } + } }; + + Ok(self + .new_turn_from_configuration( + sub_id, + session_configuration, + updates.final_output_json_schema, + sandbox_policy_changed, + ) + .await) + } + + async fn new_turn_from_configuration( + &self, + sub_id: String, + session_configuration: SessionConfiguration, + final_output_json_schema: Option>, + sandbox_policy_changed: bool, + ) -> Arc { let per_turn_config = Self::build_per_turn_config(&session_configuration); if sandbox_policy_changed { @@ -880,12 +954,26 @@ impl Session { self.conversation_id, sub_id, ); - if let Some(final_schema) = updates.final_output_json_schema { + if let Some(final_schema) = final_output_json_schema { turn_context.final_output_json_schema = final_schema; } Arc::new(turn_context) } + pub(crate) async fn new_default_turn(&self) -> Arc { + self.new_default_turn_with_sub_id(self.next_internal_sub_id()) + .await + } + + pub(crate) async fn new_default_turn_with_sub_id(&self, sub_id: String) -> Arc { + let session_configuration = { + let state = self.state.lock().await; + state.session_configuration.clone() + }; + self.new_turn_from_configuration(sub_id, session_configuration, None, false) + .await + } + fn build_environment_update_item( &self, previous: Option<&Arc>, @@ -1529,8 +1617,7 @@ impl Session { async fn submission_loop(sess: Arc, config: Arc, rx_sub: Receiver) { // Seed with context in case there is an OverrideTurnContext first. - let mut previous_context: Option> = - Some(sess.new_turn(SessionSettingsUpdate::default()).await); + let mut previous_context: Option> = Some(sess.new_default_turn().await); // To break out of this loop, send Op::Shutdown. while let Ok(sub) = rx_sub.recv().await { @@ -1549,6 +1636,7 @@ async fn submission_loop(sess: Arc, config: Arc, rx_sub: Receiv } => { handlers::override_turn_context( &sess, + sub.id.clone(), SessionSettingsUpdate { cwd, approval_policy, @@ -1584,8 +1672,8 @@ async fn submission_loop(sess: Arc, config: Arc, rx_sub: Receiv Op::ListCustomPrompts => { handlers::list_custom_prompts(&sess, sub.id.clone()).await; } - Op::ListSkills { cwds } => { - handlers::list_skills(&sess, sub.id.clone(), cwds).await; + Op::ListSkills { cwds, force_reload } => { + handlers::list_skills(&sess, sub.id.clone(), cwds, force_reload).await; } Op::Undo => { handlers::undo(&sess, sub.id.clone()).await; @@ -1666,8 +1754,21 @@ mod handlers { sess.interrupt_task().await; } - pub async fn override_turn_context(sess: &Session, updates: SessionSettingsUpdate) { - sess.update_settings(updates).await; + pub async fn override_turn_context( + sess: &Session, + sub_id: String, + updates: SessionSettingsUpdate, + ) { + if let Err(err) = sess.update_settings(updates).await { + sess.send_event_raw(Event { + id: sub_id, + msg: EventMsg::Error(ErrorEvent { + message: err.to_string(), + codex_error_info: Some(CodexErrorInfo::BadRequest), + }), + }) + .await; + } } pub async fn user_input_or_turn( @@ -1702,7 +1803,10 @@ mod handlers { _ => unreachable!(), }; - let current_context = sess.new_turn_with_sub_id(sub_id, updates).await; + let Ok(current_context) = sess.new_turn_with_sub_id(sub_id, updates).await else { + // new_turn_with_sub_id already emits the error event. + return; + }; current_context .client .get_otel_manager() @@ -1729,9 +1833,7 @@ mod handlers { command: String, previous_context: &mut Option>, ) { - let turn_context = sess - .new_turn_with_sub_id(sub_id, SessionSettingsUpdate::default()) - .await; + let turn_context = sess.new_default_turn_with_sub_id(sub_id).await; sess.spawn_task( Arc::clone(&turn_context), Vec::new(), @@ -1885,7 +1987,12 @@ mod handlers { sess.send_event_raw(event).await; } - pub async fn list_skills(sess: &Session, sub_id: String, cwds: Vec) { + pub async fn list_skills( + sess: &Session, + sub_id: String, + cwds: Vec, + force_reload: bool, + ) { let cwds = if cwds.is_empty() { let state = sess.state.lock().await; vec![state.session_configuration.cwd.clone()] @@ -1896,7 +2003,7 @@ mod handlers { let skills_manager = &sess.services.skills_manager; cwds.into_iter() .map(|cwd| { - let outcome = skills_manager.skills_for_cwd(&cwd); + let outcome = skills_manager.skills_for_cwd_with_options(&cwd, force_reload); let errors = super::errors_to_info(&outcome.errors); let skills = super::skills_to_info(&outcome.skills); SkillsListEntry { @@ -1923,17 +2030,13 @@ mod handlers { } pub async fn undo(sess: &Arc, sub_id: String) { - let turn_context = sess - .new_turn_with_sub_id(sub_id, SessionSettingsUpdate::default()) - .await; + let turn_context = sess.new_default_turn_with_sub_id(sub_id).await; sess.spawn_task(turn_context, Vec::new(), UndoTask::new()) .await; } pub async fn compact(sess: &Arc, sub_id: String) { - let turn_context = sess - .new_turn_with_sub_id(sub_id, SessionSettingsUpdate::default()) - .await; + let turn_context = sess.new_default_turn_with_sub_id(sub_id).await; sess.spawn_task( Arc::clone(&turn_context), @@ -1987,9 +2090,7 @@ mod handlers { sub_id: String, review_request: ReviewRequest, ) { - let turn_context = sess - .new_turn_with_sub_id(sub_id.clone(), SessionSettingsUpdate::default()) - .await; + let turn_context = sess.new_default_turn_with_sub_id(sub_id.clone()).await; match resolve_review_request(review_request, config.cwd.as_path()) { Ok(resolved) => { spawn_review_thread( @@ -2774,7 +2875,7 @@ mod tests { user_instructions: config.user_instructions.clone(), base_instructions: config.base_instructions.clone(), compact_prompt: config.compact_prompt.clone(), - approval_policy: config.approval_policy, + approval_policy: config.approval_policy.clone(), sandbox_policy: config.sandbox_policy.clone(), cwd: config.cwd.clone(), original_config_do_not_use: Arc::clone(&config), @@ -2846,7 +2947,7 @@ mod tests { user_instructions: config.user_instructions.clone(), base_instructions: config.base_instructions.clone(), compact_prompt: config.compact_prompt.clone(), - approval_policy: config.approval_policy, + approval_policy: config.approval_policy.clone(), sandbox_policy: config.sandbox_policy.clone(), cwd: config.cwd.clone(), original_config_do_not_use: Arc::clone(&config), @@ -3050,7 +3151,7 @@ mod tests { user_instructions: config.user_instructions.clone(), base_instructions: config.base_instructions.clone(), compact_prompt: config.compact_prompt.clone(), - approval_policy: config.approval_policy, + approval_policy: config.approval_policy.clone(), sandbox_policy: config.sandbox_policy.clone(), cwd: config.cwd.clone(), original_config_do_not_use: Arc::clone(&config), @@ -3141,7 +3242,7 @@ mod tests { user_instructions: config.user_instructions.clone(), base_instructions: config.base_instructions.clone(), compact_prompt: config.compact_prompt.clone(), - approval_policy: config.approval_policy, + approval_policy: config.approval_policy.clone(), sandbox_policy: config.sandbox_policy.clone(), cwd: config.cwd.clone(), original_config_do_not_use: Arc::clone(&config), diff --git a/codex-rs/core/src/config/constraint.rs b/codex-rs/core/src/config/constraint.rs new file mode 100644 index 0000000000..f10df09ddd --- /dev/null +++ b/codex-rs/core/src/config/constraint.rs @@ -0,0 +1,195 @@ +use std::fmt; +use std::sync::Arc; + +use thiserror::Error; + +#[derive(Debug, Error, PartialEq, Eq)] +#[error("{message}")] +pub struct ConstraintError { + pub message: String, +} + +impl ConstraintError { + pub fn invalid_value(candidate: impl Into, allowed: impl Into) -> Self { + Self { + message: format!( + "value `{}` is not in the allowed set {}", + candidate.into(), + allowed.into() + ), + } + } +} + +pub type ConstraintResult = Result; + +impl From for std::io::Error { + fn from(err: ConstraintError) -> Self { + std::io::Error::new(std::io::ErrorKind::InvalidInput, err) + } +} + +type ConstraintValidator = dyn Fn(&T) -> ConstraintResult<()> + Send + Sync; + +#[derive(Clone)] +pub struct Constrained { + value: T, + validator: Arc>, +} + +impl Constrained { + pub fn new( + initial_value: T, + validator: impl Fn(&T) -> ConstraintResult<()> + Send + Sync + 'static, + ) -> ConstraintResult { + let validator: Arc> = Arc::new(validator); + validator(&initial_value)?; + Ok(Self { + value: initial_value, + validator, + }) + } + + pub fn allow_any(initial_value: T) -> Self { + Self { + value: initial_value, + validator: Arc::new(|_| Ok(())), + } + } + + pub fn allow_values(initial_value: T, allowed: Vec) -> ConstraintResult + where + T: PartialEq + Send + Sync + fmt::Debug + 'static, + { + Self::new(initial_value, move |candidate| { + if allowed.contains(candidate) { + Ok(()) + } else { + Err(ConstraintError::invalid_value( + format!("{candidate:?}"), + format!("{allowed:?}"), + )) + } + }) + } + + pub fn get(&self) -> &T { + &self.value + } + + pub fn value(&self) -> T + where + T: Copy, + { + self.value + } + + pub fn can_set(&self, candidate: &T) -> ConstraintResult<()> { + (self.validator)(candidate) + } + + pub fn set(&mut self, value: T) -> ConstraintResult<()> { + (self.validator)(&value)?; + self.value = value; + Ok(()) + } +} + +impl std::ops::Deref for Constrained { + type Target = T; + + fn deref(&self) -> &Self::Target { + &self.value + } +} + +impl fmt::Debug for Constrained { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.debug_struct("Constrained") + .field("value", &self.value) + .finish() + } +} + +impl PartialEq for Constrained { + fn eq(&self, other: &Self) -> bool { + self.value == other.value + } +} + +#[cfg(test)] +mod tests { + use super::*; + use pretty_assertions::assert_eq; + + #[test] + fn constrained_allow_any_accepts_any_value() { + let mut constrained = Constrained::allow_any(5); + constrained.set(-10).expect("allow any accepts all values"); + assert_eq!(constrained.value(), -10); + } + + #[test] + fn constrained_new_rejects_invalid_initial_value() { + let result = Constrained::new(0, |value| { + if *value > 0 { + Ok(()) + } else { + Err(ConstraintError::invalid_value( + value.to_string(), + "positive values", + )) + } + }); + + assert_eq!( + result, + Err(ConstraintError::invalid_value("0", "positive values")) + ); + } + + #[test] + fn constrained_set_rejects_invalid_value_and_leaves_previous() { + let mut constrained = Constrained::new(1, |value| { + if *value > 0 { + Ok(()) + } else { + Err(ConstraintError::invalid_value( + value.to_string(), + "positive values", + )) + } + }) + .expect("initial value should be accepted"); + + let err = constrained + .set(-5) + .expect_err("negative values should be rejected"); + assert_eq!(err, ConstraintError::invalid_value("-5", "positive values")); + assert_eq!(constrained.value(), 1); + } + + #[test] + fn constrained_can_set_allows_probe_without_setting() { + let constrained = Constrained::new(1, |value| { + if *value > 0 { + Ok(()) + } else { + Err(ConstraintError::invalid_value( + value.to_string(), + "positive values", + )) + } + }) + .expect("initial value should be accepted"); + + constrained + .can_set(&2) + .expect("can_set should accept positive value"); + let err = constrained + .can_set(&-1) + .expect_err("can_set should reject negative value"); + assert_eq!(err, ConstraintError::invalid_value("-1", "positive values")); + assert_eq!(constrained.value(), 1); + } +} diff --git a/codex-rs/core/src/config/mod.rs b/codex-rs/core/src/config/mod.rs index a7e11f45eb..9a1ad16e94 100644 --- a/codex-rs/core/src/config/mod.rs +++ b/codex-rs/core/src/config/mod.rs @@ -26,7 +26,6 @@ use crate::project_doc::DEFAULT_PROJECT_DOC_FILENAME; use crate::project_doc::LOCAL_PROJECT_DOC_FILENAME; use crate::protocol::AskForApproval; use crate::protocol::SandboxPolicy; -use crate::util::resolve_path; use codex_app_server_protocol::Tools; use codex_app_server_protocol::UserSavedConfig; use codex_protocol::config_types::ForcedLoginMethod; @@ -54,10 +53,14 @@ use crate::config::profile::ConfigProfile; use toml::Value as TomlValue; use toml_edit::DocumentMut; +mod constraint; pub mod edit; pub mod profile; pub mod service; pub mod types; +pub use constraint::Constrained; +pub use constraint::ConstraintError; +pub use constraint::ConstraintResult; pub use service::ConfigService; pub use service::ConfigServiceError; @@ -106,7 +109,7 @@ pub struct Config { pub model_provider: ModelProviderInfo, /// Approval policy for executing commands. - pub approval_policy: AskForApproval, + pub approval_policy: Constrained, pub sandbox_policy: SandboxPolicy, @@ -688,8 +691,8 @@ pub struct ConfigToml { pub notice: Option, /// Legacy, now use features - pub experimental_instructions_file: Option, - pub experimental_compact_prompt_file: Option, + pub experimental_instructions_file: Option, + pub experimental_compact_prompt_file: Option, pub experimental_use_unified_exec_tool: Option, pub experimental_use_rmcp_client: Option, pub experimental_use_freeform_apply_patch: Option, @@ -762,9 +765,11 @@ pub struct GhostSnapshotToml { #[serde(alias = "ignore_untracked_files_over_bytes")] pub ignore_large_untracked_files: Option, /// Ignore untracked directories that contain this many files or more. - /// (Still emits a warning.) + /// (Still emits a warning unless warnings are disabled.) #[serde(alias = "large_untracked_dir_warning_threshold")] pub ignore_large_untracked_dirs: Option, + /// Disable all ghost snapshot warning events. + pub disable_warnings: Option, } #[derive(Debug, PartialEq, Eq)] @@ -1026,15 +1031,14 @@ impl Config { .or(cfg.approval_policy) .unwrap_or_else(|| { if active_project.is_trusted() { - // If no explicit approval policy is set, but we trust cwd, default to OnRequest AskForApproval::OnRequest } else if active_project.is_untrusted() { - // If project is explicitly marked untrusted, require approval for non-safe commands AskForApproval::UnlessTrusted } else { AskForApproval::default() } }); + let approval_policy = Constrained::allow_any(approval_policy); let did_user_set_custom_approval_policy_or_sandbox_mode = approval_policy_override .is_some() || config_profile.approval_policy.is_some() @@ -1084,6 +1088,11 @@ impl Config { config.ignore_large_untracked_dirs = if threshold > 0 { Some(threshold) } else { None }; } + if let Some(ghost_snapshot) = cfg.ghost_snapshot.as_ref() + && let Some(disable_warnings) = ghost_snapshot.disable_warnings + { + config.disable_warnings = disable_warnings; + } config }; @@ -1122,9 +1131,8 @@ impl Config { .experimental_instructions_file .as_ref() .or(cfg.experimental_instructions_file.as_ref()); - let file_base_instructions = Self::load_override_from_file( + let file_base_instructions = Self::try_read_non_empty_file( experimental_instructions_path, - &resolved_cwd, "experimental instructions file", )?; let base_instructions = base_instructions.or(file_base_instructions); @@ -1134,9 +1142,8 @@ impl Config { .experimental_compact_prompt_file .as_ref() .or(cfg.experimental_compact_prompt_file.as_ref()); - let file_compact_prompt = Self::load_override_from_file( + let file_compact_prompt = Self::try_read_non_empty_file( experimental_compact_prompt_path, - &resolved_cwd, "experimental compact prompt file", )?; let compact_prompt = compact_prompt.or(file_compact_prompt); @@ -1268,21 +1275,21 @@ impl Config { None } - fn load_override_from_file( - path: Option<&PathBuf>, - cwd: &Path, - description: &str, + /// If `path` is `Some`, attempts to read the file at the given path and + /// returns its contents as a trimmed `String`. If the file is empty, or + /// is `Some` but cannot be read, returns an `Err`. + fn try_read_non_empty_file( + path: Option<&AbsolutePathBuf>, + context: &str, ) -> std::io::Result> { - let Some(p) = path else { + let Some(path) = path else { return Ok(None); }; - let full_path = resolve_path(cwd, p); - - let contents = std::fs::read_to_string(&full_path).map_err(|e| { + let contents = std::fs::read_to_string(path).map_err(|e| { std::io::Error::new( e.kind(), - format!("failed to read {description} {}: {e}", full_path.display()), + format!("failed to read {context} {}: {e}", path.display()), ) })?; @@ -1290,7 +1297,7 @@ impl Config { if s.is_empty() { Err(std::io::Error::new( std::io::ErrorKind::InvalidData, - format!("{description} is empty: {}", full_path.display()), + format!("{context} is empty: {}", path.display()), )) } else { Ok(Some(s)) @@ -2794,7 +2801,9 @@ model = "gpt-5.1-codex" std::fs::write(&prompt_path, " summarize differently ")?; let cfg = ConfigToml { - experimental_compact_prompt_file: Some(PathBuf::from("compact_prompt.txt")), + experimental_compact_prompt_file: Some(AbsolutePathBuf::from_absolute_path( + prompt_path, + )?), ..Default::default() }; @@ -2945,7 +2954,7 @@ model_verbosity = "high" model_auto_compact_token_limit: None, model_provider_id: "openai".to_string(), model_provider: fixture.openai_provider.clone(), - approval_policy: AskForApproval::Never, + approval_policy: Constrained::allow_any(AskForApproval::Never), sandbox_policy: SandboxPolicy::new_read_only_policy(), did_user_set_custom_approval_policy_or_sandbox_mode: true, forced_auto_mode_downgraded_on_windows: false, @@ -3020,7 +3029,7 @@ model_verbosity = "high" model_auto_compact_token_limit: None, model_provider_id: "openai-chat-completions".to_string(), model_provider: fixture.openai_chat_completions_provider.clone(), - approval_policy: AskForApproval::UnlessTrusted, + approval_policy: Constrained::allow_any(AskForApproval::UnlessTrusted), sandbox_policy: SandboxPolicy::new_read_only_policy(), did_user_set_custom_approval_policy_or_sandbox_mode: true, forced_auto_mode_downgraded_on_windows: false, @@ -3110,7 +3119,7 @@ model_verbosity = "high" model_auto_compact_token_limit: None, model_provider_id: "openai".to_string(), model_provider: fixture.openai_provider.clone(), - approval_policy: AskForApproval::OnFailure, + approval_policy: Constrained::allow_any(AskForApproval::OnFailure), sandbox_policy: SandboxPolicy::new_read_only_policy(), did_user_set_custom_approval_policy_or_sandbox_mode: true, forced_auto_mode_downgraded_on_windows: false, @@ -3186,7 +3195,7 @@ model_verbosity = "high" model_auto_compact_token_limit: None, model_provider_id: "openai".to_string(), model_provider: fixture.openai_provider.clone(), - approval_policy: AskForApproval::OnFailure, + approval_policy: Constrained::allow_any(AskForApproval::OnFailure), sandbox_policy: SandboxPolicy::new_read_only_policy(), did_user_set_custom_approval_policy_or_sandbox_mode: true, forced_auto_mode_downgraded_on_windows: false, @@ -3500,26 +3509,21 @@ trust_level = "untrusted" } #[test] - fn test_untrusted_project_gets_unless_trusted_approval_policy() -> std::io::Result<()> { + fn test_untrusted_project_gets_unless_trusted_approval_policy() -> anyhow::Result<()> { let codex_home = TempDir::new()?; let test_project_dir = TempDir::new()?; let test_path = test_project_dir.path(); - let mut projects = std::collections::HashMap::new(); - projects.insert( - test_path.to_string_lossy().to_string(), - ProjectConfig { - trust_level: Some(TrustLevel::Untrusted), - }, - ); - - let cfg = ConfigToml { - projects: Some(projects), - ..Default::default() - }; - let config = Config::load_from_base_config_with_overrides( - cfg, + ConfigToml { + projects: Some(HashMap::from([( + test_path.to_string_lossy().to_string(), + ProjectConfig { + trust_level: Some(TrustLevel::Untrusted), + }, + )])), + ..Default::default() + }, ConfigOverrides { cwd: Some(test_path.to_path_buf()), ..Default::default() @@ -3529,7 +3533,7 @@ trust_level = "untrusted" // Verify that untrusted projects get UnlessTrusted approval policy assert_eq!( - config.approval_policy, + config.approval_policy.value(), AskForApproval::UnlessTrusted, "Expected UnlessTrusted approval policy for untrusted project" ); diff --git a/codex-rs/core/src/config/profile.rs b/codex-rs/core/src/config/profile.rs index 978e1fcb63..b74b70887d 100644 --- a/codex-rs/core/src/config/profile.rs +++ b/codex-rs/core/src/config/profile.rs @@ -1,5 +1,5 @@ +use codex_utils_absolute_path::AbsolutePathBuf; use serde::Deserialize; -use std::path::PathBuf; use crate::protocol::AskForApproval; use codex_protocol::config_types::ReasoningSummary; @@ -21,8 +21,8 @@ pub struct ConfigProfile { pub model_reasoning_summary: Option, pub model_verbosity: Option, pub chatgpt_base_url: Option, - pub experimental_instructions_file: Option, - pub experimental_compact_prompt_file: Option, + pub experimental_instructions_file: Option, + pub experimental_compact_prompt_file: Option, pub include_apply_patch_tool: Option, pub experimental_use_unified_exec_tool: Option, pub experimental_use_rmcp_client: Option, diff --git a/codex-rs/core/src/config/service.rs b/codex-rs/core/src/config/service.rs index 59bd128582..398a74c0d7 100644 --- a/codex-rs/core/src/config/service.rs +++ b/codex-rs/core/src/config/service.rs @@ -7,6 +7,7 @@ use crate::config_loader::ConfigLayerStack; use crate::config_loader::LoaderOverrides; use crate::config_loader::load_config_layers_state; use crate::config_loader::merge_toml_values; +use crate::path_utils; use codex_app_server_protocol::Config as ApiConfig; use codex_app_server_protocol::ConfigBatchWriteParams; use codex_app_server_protocol::ConfigLayerMetadata; @@ -470,9 +471,10 @@ fn validate_config(value: &TomlValue) -> Result<(), toml::de::Error> { } fn paths_match(expected: &Path, provided: &Path) -> bool { - if let (Ok(expanded_expected), Ok(expanded_provided)) = - (expected.canonicalize(), provided.canonicalize()) - { + if let (Ok(expanded_expected), Ok(expanded_provided)) = ( + path_utils::normalize_for_path_comparison(expected), + path_utils::normalize_for_path_comparison(provided), + ) { return expanded_expected == expanded_provided; } diff --git a/codex-rs/core/src/features.rs b/codex-rs/core/src/features.rs index 431eabe3c6..3239adc109 100644 --- a/codex-rs/core/src/features.rs +++ b/codex-rs/core/src/features.rs @@ -18,12 +18,33 @@ pub(crate) use legacy::LegacyFeatureToggles; #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum Stage { Experimental, - Beta, + Beta { + menu_description: &'static str, + announcement: &'static str, + }, Stable, Deprecated, Removed, } +impl Stage { + pub fn beta_menu_description(self) -> Option<&'static str> { + match self { + Stage::Beta { + menu_description, .. + } => Some(menu_description), + _ => None, + } + } + + pub fn beta_announcement(self) -> Option<&'static str> { + match self { + Stage::Beta { announcement, .. } => Some(announcement), + _ => None, + } + } +} + /// Unique features toggled via configuration. #[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)] pub enum Feature { @@ -292,13 +313,44 @@ pub const FEATURES: &[FeatureSpec] = &[ stage: Stage::Stable, default_enabled: true, }, - // Unstable features. FeatureSpec { - id: Feature::UnifiedExec, - key: "unified_exec", + id: Feature::WebSearchRequest, + key: "web_search_request", + stage: Stage::Stable, + default_enabled: false, + }, + // Beta program. Rendered in the `/experimental` menu for users. + FeatureSpec { + id: Feature::Skills, + key: "skills", + // stage: Stage::Beta { + // menu_description: "Define new `skills` for the model", + // announcement: "NEW! Try the new `skills` features. Enable in /experimental!", + // }, stage: Stage::Experimental, default_enabled: false, }, + FeatureSpec { + id: Feature::UnifiedExec, + key: "unified_exec", + // stage: Stage::Beta { + // menu_description: "Run long-running terminal commands in the background.", + // announcement: "NEW! Try Background terminals for long running processes. Enable in /experimental!", + // }, + stage: Stage::Experimental, + default_enabled: false, + }, + FeatureSpec { + id: Feature::ShellSnapshot, + key: "shell_snapshot", + // stage: Stage::Beta { + // menu_description: "Snapshot your shell environment to avoid re-running login scripts for every command.", + // announcement: "NEW! Try shell snapshotting to make your Codex faster. Enable in /experimental!", + // }, + stage: Stage::Experimental, + default_enabled: false, + }, + // Unstable features. FeatureSpec { id: Feature::RmcpClient, key: "rmcp_client", @@ -308,13 +360,7 @@ pub const FEATURES: &[FeatureSpec] = &[ FeatureSpec { id: Feature::ApplyPatchFreeform, key: "apply_patch_freeform", - stage: Stage::Beta, - default_enabled: false, - }, - FeatureSpec { - id: Feature::WebSearchRequest, - key: "web_search_request", - stage: Stage::Stable, + stage: Stage::Experimental, default_enabled: false, }, FeatureSpec { @@ -347,12 +393,6 @@ pub const FEATURES: &[FeatureSpec] = &[ stage: Stage::Experimental, default_enabled: false, }, - FeatureSpec { - id: Feature::Skills, - key: "skills", - stage: Stage::Experimental, - default_enabled: false, - }, FeatureSpec { id: Feature::ShellSnapshot, key: "shell_snapshot", diff --git a/codex-rs/core/src/lib.rs b/codex-rs/core/src/lib.rs index 11b49c78c4..f78c19328f 100644 --- a/codex-rs/core/src/lib.rs +++ b/codex-rs/core/src/lib.rs @@ -41,6 +41,7 @@ mod mcp_tool_call; mod message_history; mod model_provider_info; pub mod parse_command; +pub mod path_utils; pub mod powershell; pub mod sandboxing; mod stream_events_utils; diff --git a/codex-rs/core/src/path_utils.rs b/codex-rs/core/src/path_utils.rs new file mode 100644 index 0000000000..9a7007e4f1 --- /dev/null +++ b/codex-rs/core/src/path_utils.rs @@ -0,0 +1,116 @@ +use std::path::Path; +use std::path::PathBuf; + +use crate::env; + +pub fn normalize_for_path_comparison(path: &Path) -> std::io::Result { + let canonical = path.canonicalize()?; + Ok(normalize_for_wsl(canonical)) +} + +fn normalize_for_wsl(path: PathBuf) -> PathBuf { + normalize_for_wsl_with_flag(path, env::is_wsl()) +} + +fn normalize_for_wsl_with_flag(path: PathBuf, is_wsl: bool) -> PathBuf { + if !is_wsl { + return path; + } + + if !is_wsl_case_insensitive_path(&path) { + return path; + } + + lower_ascii_path(path) +} + +fn is_wsl_case_insensitive_path(path: &Path) -> bool { + #[cfg(target_os = "linux")] + { + use std::os::unix::ffi::OsStrExt; + use std::path::Component; + + let mut components = path.components(); + let Some(Component::RootDir) = components.next() else { + return false; + }; + let Some(Component::Normal(mnt)) = components.next() else { + return false; + }; + if !ascii_eq_ignore_case(mnt.as_bytes(), b"mnt") { + return false; + } + let Some(Component::Normal(drive)) = components.next() else { + return false; + }; + let drive_bytes = drive.as_bytes(); + drive_bytes.len() == 1 && drive_bytes[0].is_ascii_alphabetic() + } + #[cfg(not(target_os = "linux"))] + { + let _ = path; + false + } +} + +#[cfg(target_os = "linux")] +fn ascii_eq_ignore_case(left: &[u8], right: &[u8]) -> bool { + left.len() == right.len() + && left + .iter() + .zip(right) + .all(|(lhs, rhs)| lhs.to_ascii_lowercase() == *rhs) +} + +#[cfg(target_os = "linux")] +fn lower_ascii_path(path: PathBuf) -> PathBuf { + use std::ffi::OsString; + use std::os::unix::ffi::OsStrExt; + use std::os::unix::ffi::OsStringExt; + + // WSL mounts Windows drives under /mnt/, which are case-insensitive. + let bytes = path.as_os_str().as_bytes(); + let mut lowered = Vec::with_capacity(bytes.len()); + for byte in bytes { + lowered.push(byte.to_ascii_lowercase()); + } + PathBuf::from(OsString::from_vec(lowered)) +} + +#[cfg(not(target_os = "linux"))] +fn lower_ascii_path(path: PathBuf) -> PathBuf { + path +} + +#[cfg(test)] +mod tests { + #[cfg(target_os = "linux")] + mod wsl { + use super::super::normalize_for_wsl_with_flag; + use pretty_assertions::assert_eq; + use std::path::PathBuf; + + #[test] + fn wsl_mnt_drive_paths_lowercase() { + let normalized = normalize_for_wsl_with_flag(PathBuf::from("/mnt/C/Users/Dev"), true); + + assert_eq!(normalized, PathBuf::from("/mnt/c/users/dev")); + } + + #[test] + fn wsl_non_drive_paths_unchanged() { + let path = PathBuf::from("/mnt/cc/Users/Dev"); + let normalized = normalize_for_wsl_with_flag(path.clone(), true); + + assert_eq!(normalized, path); + } + + #[test] + fn wsl_non_mnt_paths_unchanged() { + let path = PathBuf::from("/home/Dev"); + let normalized = normalize_for_wsl_with_flag(path.clone(), true); + + assert_eq!(normalized, path); + } + } +} diff --git a/codex-rs/core/src/project_doc.rs b/codex-rs/core/src/project_doc.rs index cd05520110..208b035565 100644 --- a/codex-rs/core/src/project_doc.rs +++ b/codex-rs/core/src/project_doc.rs @@ -522,7 +522,7 @@ mod tests { let expected_path_str = expected_path.to_string_lossy().replace('\\', "/"); let usage_rules = "- Discovery: Available skills are listed in project docs and may also appear in a runtime \"## Skills\" section (name + description + file path). These are the sources of truth; skill bodies live on disk at the listed paths.\n- Trigger rules: If the user names a skill (with `$SkillName` or plain text) OR the task clearly matches a skill's description, you must use that skill for that turn. Multiple mentions mean use them all. Do not carry skills across turns unless re-mentioned.\n- Missing/blocked: If a named skill isn't in the list or the path can't be read, say so briefly and continue with the best fallback.\n- How to use a skill (progressive disclosure):\n 1) After deciding to use a skill, open its `SKILL.md`. Read only enough to follow the workflow.\n 2) If `SKILL.md` points to extra folders such as `references/`, load only the specific files needed for the request; don't bulk-load everything.\n 3) If `scripts/` exist, prefer running or patching them instead of retyping large code blocks.\n 4) If `assets/` or templates exist, reuse them instead of recreating from scratch.\n- Description as trigger: The YAML `description` in `SKILL.md` is the primary trigger signal; rely on it to decide applicability. If unsure, ask a brief clarification before proceeding.\n- Coordination and sequencing:\n - If multiple skills apply, choose the minimal set that covers the request and state the order you'll use them.\n - Announce which skill(s) you're using and why (one short line). If you skip an obvious skill, say why.\n- Context hygiene:\n - Keep context small: summarize long sections instead of pasting them; only load extra files when needed.\n - Avoid deeply nested references; prefer one-hop files explicitly linked from `SKILL.md`.\n - When variants exist (frameworks, providers, domains), pick only the relevant reference file(s) and note that choice.\n- Safety and fallback: If a skill can't be applied cleanly (missing files, unclear instructions), state the issue, pick the next-best approach, and continue."; let expected = format!( - "base doc\n\n## Skills\nThese skills are discovered at startup from ~/.codex/skills; each entry shows name, description, and file path so you can open the source for full instructions. Content is not inlined to keep context lean.\n- pdf-processing: extract from pdfs (file: {expected_path_str})\n{usage_rules}" + "base doc\n\n## Skills\nThese skills are discovered at startup from multiple local sources. Each entry includes a name, description, and file path so you can open the source for full instructions.\n- pdf-processing: extract from pdfs (file: {expected_path_str})\n{usage_rules}" ); assert_eq!(res, expected); } @@ -546,7 +546,7 @@ mod tests { let expected_path_str = expected_path.to_string_lossy().replace('\\', "/"); let usage_rules = "- Discovery: Available skills are listed in project docs and may also appear in a runtime \"## Skills\" section (name + description + file path). These are the sources of truth; skill bodies live on disk at the listed paths.\n- Trigger rules: If the user names a skill (with `$SkillName` or plain text) OR the task clearly matches a skill's description, you must use that skill for that turn. Multiple mentions mean use them all. Do not carry skills across turns unless re-mentioned.\n- Missing/blocked: If a named skill isn't in the list or the path can't be read, say so briefly and continue with the best fallback.\n- How to use a skill (progressive disclosure):\n 1) After deciding to use a skill, open its `SKILL.md`. Read only enough to follow the workflow.\n 2) If `SKILL.md` points to extra folders such as `references/`, load only the specific files needed for the request; don't bulk-load everything.\n 3) If `scripts/` exist, prefer running or patching them instead of retyping large code blocks.\n 4) If `assets/` or templates exist, reuse them instead of recreating from scratch.\n- Description as trigger: The YAML `description` in `SKILL.md` is the primary trigger signal; rely on it to decide applicability. If unsure, ask a brief clarification before proceeding.\n- Coordination and sequencing:\n - If multiple skills apply, choose the minimal set that covers the request and state the order you'll use them.\n - Announce which skill(s) you're using and why (one short line). If you skip an obvious skill, say why.\n- Context hygiene:\n - Keep context small: summarize long sections instead of pasting them; only load extra files when needed.\n - Avoid deeply nested references; prefer one-hop files explicitly linked from `SKILL.md`.\n - When variants exist (frameworks, providers, domains), pick only the relevant reference file(s) and note that choice.\n- Safety and fallback: If a skill can't be applied cleanly (missing files, unclear instructions), state the issue, pick the next-best approach, and continue."; let expected = format!( - "## Skills\nThese skills are discovered at startup from ~/.codex/skills; each entry shows name, description, and file path so you can open the source for full instructions. Content is not inlined to keep context lean.\n- linting: run clippy (file: {expected_path_str})\n{usage_rules}" + "## Skills\nThese skills are discovered at startup from multiple local sources. Each entry includes a name, description, and file path so you can open the source for full instructions.\n- linting: run clippy (file: {expected_path_str})\n{usage_rules}" ); assert_eq!(res, expected); } diff --git a/codex-rs/core/src/rollout/policy.rs b/codex-rs/core/src/rollout/policy.rs index 2980e768cf..07c8af1144 100644 --- a/codex-rs/core/src/rollout/policy.rs +++ b/codex-rs/core/src/rollout/policy.rs @@ -88,6 +88,7 @@ pub(crate) fn should_persist_event_msg(ev: &EventMsg) -> bool { | EventMsg::ItemCompleted(_) | EventMsg::AgentMessageContentDelta(_) | EventMsg::ReasoningContentDelta(_) - | EventMsg::ReasoningRawContentDelta(_) => false, + | EventMsg::ReasoningRawContentDelta(_) + | EventMsg::SkillsUpdateAvailable => false, } } diff --git a/codex-rs/core/src/skills/loader.rs b/codex-rs/core/src/skills/loader.rs index 54b859caaf..2596f3d76f 100644 --- a/codex-rs/core/src/skills/loader.rs +++ b/codex-rs/core/src/skills/loader.rs @@ -3,9 +3,11 @@ use crate::git_info::resolve_root_git_project_for_trust; use crate::skills::model::SkillError; use crate::skills::model::SkillLoadOutcome; use crate::skills::model::SkillMetadata; +use crate::skills::public::public_cache_root_dir; use codex_protocol::protocol::SkillScope; use dunce::canonicalize as normalize_path; use serde::Deserialize; +use std::collections::HashSet; use std::collections::VecDeque; use std::error::Error; use std::fmt; @@ -71,6 +73,11 @@ where discover_skills_under_root(&root.path, root.scope, &mut outcome); } + let mut seen: HashSet = HashSet::new(); + outcome + .skills + .retain(|skill| seen.insert(skill.name.clone())); + outcome .skills .sort_by(|a, b| a.name.cmp(&b.name).then_with(|| a.path.cmp(&b.path))); @@ -85,22 +92,57 @@ pub(crate) fn user_skills_root(codex_home: &Path) -> SkillRoot { } } +pub(crate) fn public_skills_root(codex_home: &Path) -> SkillRoot { + SkillRoot { + path: public_cache_root_dir(codex_home), + scope: SkillScope::Public, + } +} + pub(crate) fn repo_skills_root(cwd: &Path) -> Option { - resolve_root_git_project_for_trust(cwd).map(|repo_root| SkillRoot { - path: repo_root - .join(REPO_ROOT_CONFIG_DIR_NAME) - .join(SKILLS_DIR_NAME), - scope: SkillScope::Repo, + let base = if cwd.is_dir() { cwd } else { cwd.parent()? }; + let base = normalize_path(base).unwrap_or_else(|_| base.to_path_buf()); + + let repo_root = + resolve_root_git_project_for_trust(&base).map(|root| normalize_path(&root).unwrap_or(root)); + + let scope = SkillScope::Repo; + if let Some(repo_root) = repo_root.as_deref() { + for dir in base.ancestors() { + let skills_root = dir.join(REPO_ROOT_CONFIG_DIR_NAME).join(SKILLS_DIR_NAME); + if skills_root.is_dir() { + return Some(SkillRoot { + path: skills_root, + scope, + }); + } + + if dir == repo_root { + break; + } + } + return None; + } + + let skills_root = base.join(REPO_ROOT_CONFIG_DIR_NAME).join(SKILLS_DIR_NAME); + skills_root.is_dir().then_some(SkillRoot { + path: skills_root, + scope, }) } fn skill_roots(config: &Config) -> Vec { - let mut roots = vec![user_skills_root(&config.codex_home)]; + let mut roots = Vec::new(); if let Some(repo_root) = repo_skills_root(&config.cwd) { roots.push(repo_root); } + // Load order matters: we dedupe by name, keeping the first occurrence. + // This makes repo/user skills win over public skills. + roots.push(user_skills_root(&config.codex_home)); + roots.push(public_skills_root(&config.codex_home)); + roots } @@ -149,11 +191,17 @@ fn discover_skills_under_root(root: &Path, scope: SkillScope, outcome: &mut Skil if file_type.is_file() && file_name == SKILLS_FILENAME { match parse_skill_file(&path, scope) { - Ok(skill) => outcome.skills.push(skill), - Err(err) => outcome.errors.push(SkillError { - path, - message: err.to_string(), - }), + Ok(skill) => { + outcome.skills.push(skill); + } + Err(err) => { + if scope != SkillScope::Public { + outcome.errors.push(SkillError { + path, + message: err.to_string(), + }); + } + } } } } @@ -233,6 +281,7 @@ mod tests { use super::*; use crate::config::ConfigOverrides; use crate::config::ConfigToml; + use codex_protocol::protocol::SkillScope; use pretty_assertions::assert_eq; use std::path::Path; use std::process::Command; @@ -251,11 +300,11 @@ mod tests { } fn write_skill(codex_home: &TempDir, dir: &str, name: &str, description: &str) -> PathBuf { - write_skill_at(codex_home.path(), dir, name, description) + write_skill_at(&codex_home.path().join("skills"), dir, name, description) } fn write_skill_at(root: &Path, dir: &str, name: &str, description: &str) -> PathBuf { - let skill_dir = root.join(format!("skills/{dir}")); + let skill_dir = root.join(dir); fs::create_dir_all(&skill_dir).unwrap(); let indented_description = description.replace('\n', "\n "); let content = format!( @@ -375,4 +424,316 @@ mod tests { assert_eq!(skill.name, "repo-skill"); assert!(skill.path.starts_with(&repo_root)); } + + #[test] + fn loads_skills_from_nearest_codex_dir_under_repo_root() { + let codex_home = tempfile::tempdir().expect("tempdir"); + let repo_dir = tempfile::tempdir().expect("tempdir"); + + let status = Command::new("git") + .arg("init") + .current_dir(repo_dir.path()) + .status() + .expect("git init"); + assert!(status.success(), "git init failed"); + + let nested_dir = repo_dir.path().join("nested/inner"); + fs::create_dir_all(&nested_dir).unwrap(); + + write_skill_at( + &repo_dir + .path() + .join(REPO_ROOT_CONFIG_DIR_NAME) + .join(SKILLS_DIR_NAME), + "root", + "root-skill", + "from root", + ); + write_skill_at( + &repo_dir + .path() + .join("nested") + .join(REPO_ROOT_CONFIG_DIR_NAME) + .join(SKILLS_DIR_NAME), + "nested", + "nested-skill", + "from nested", + ); + + let mut cfg = make_config(&codex_home); + cfg.cwd = nested_dir; + + let outcome = load_skills(&cfg); + assert!( + outcome.errors.is_empty(), + "unexpected errors: {:?}", + outcome.errors + ); + assert_eq!(outcome.skills.len(), 1); + assert_eq!(outcome.skills[0].name, "nested-skill"); + } + + #[test] + fn loads_skills_from_codex_dir_when_not_git_repo() { + let codex_home = tempfile::tempdir().expect("tempdir"); + let work_dir = tempfile::tempdir().expect("tempdir"); + + write_skill_at( + &work_dir + .path() + .join(REPO_ROOT_CONFIG_DIR_NAME) + .join(SKILLS_DIR_NAME), + "local", + "local-skill", + "from cwd", + ); + + let mut cfg = make_config(&codex_home); + cfg.cwd = work_dir.path().to_path_buf(); + + let outcome = load_skills(&cfg); + assert!( + outcome.errors.is_empty(), + "unexpected errors: {:?}", + outcome.errors + ); + assert_eq!(outcome.skills.len(), 1); + assert_eq!(outcome.skills[0].name, "local-skill"); + assert_eq!(outcome.skills[0].scope, SkillScope::Repo); + } + + #[test] + fn deduplicates_by_name_preferring_repo_over_user() { + let codex_home = tempfile::tempdir().expect("tempdir"); + let repo_dir = tempfile::tempdir().expect("tempdir"); + + let status = Command::new("git") + .arg("init") + .current_dir(repo_dir.path()) + .status() + .expect("git init"); + assert!(status.success(), "git init failed"); + + write_skill(&codex_home, "user", "dupe-skill", "from user"); + write_skill_at( + &repo_dir + .path() + .join(REPO_ROOT_CONFIG_DIR_NAME) + .join(SKILLS_DIR_NAME), + "repo", + "dupe-skill", + "from repo", + ); + + let mut cfg = make_config(&codex_home); + cfg.cwd = repo_dir.path().to_path_buf(); + + let outcome = load_skills(&cfg); + assert!( + outcome.errors.is_empty(), + "unexpected errors: {:?}", + outcome.errors + ); + assert_eq!(outcome.skills.len(), 1); + assert_eq!(outcome.skills[0].name, "dupe-skill"); + assert_eq!(outcome.skills[0].scope, SkillScope::Repo); + } + + #[test] + fn repo_skills_search_does_not_escape_repo_root() { + let codex_home = tempfile::tempdir().expect("tempdir"); + let outer_dir = tempfile::tempdir().expect("tempdir"); + let repo_dir = outer_dir.path().join("repo"); + fs::create_dir_all(&repo_dir).unwrap(); + + write_skill_at( + &outer_dir + .path() + .join(REPO_ROOT_CONFIG_DIR_NAME) + .join(SKILLS_DIR_NAME), + "outer", + "outer-skill", + "from outer", + ); + + let status = Command::new("git") + .arg("init") + .current_dir(&repo_dir) + .status() + .expect("git init"); + assert!(status.success(), "git init failed"); + + let mut cfg = make_config(&codex_home); + cfg.cwd = repo_dir; + + let outcome = load_skills(&cfg); + assert!( + outcome.errors.is_empty(), + "unexpected errors: {:?}", + outcome.errors + ); + assert_eq!(outcome.skills.len(), 0); + } + + #[test] + fn loads_skills_when_cwd_is_file_in_repo() { + let codex_home = tempfile::tempdir().expect("tempdir"); + let repo_dir = tempfile::tempdir().expect("tempdir"); + + let status = Command::new("git") + .arg("init") + .current_dir(repo_dir.path()) + .status() + .expect("git init"); + assert!(status.success(), "git init failed"); + + write_skill_at( + &repo_dir + .path() + .join(REPO_ROOT_CONFIG_DIR_NAME) + .join(SKILLS_DIR_NAME), + "repo", + "repo-skill", + "from repo", + ); + let file_path = repo_dir.path().join("some-file.txt"); + fs::write(&file_path, "contents").unwrap(); + + let mut cfg = make_config(&codex_home); + cfg.cwd = file_path; + + let outcome = load_skills(&cfg); + assert!( + outcome.errors.is_empty(), + "unexpected errors: {:?}", + outcome.errors + ); + assert_eq!(outcome.skills.len(), 1); + assert_eq!(outcome.skills[0].name, "repo-skill"); + assert_eq!(outcome.skills[0].scope, SkillScope::Repo); + } + + #[test] + fn non_git_repo_skills_search_does_not_walk_parents() { + let codex_home = tempfile::tempdir().expect("tempdir"); + let outer_dir = tempfile::tempdir().expect("tempdir"); + let nested_dir = outer_dir.path().join("nested/inner"); + fs::create_dir_all(&nested_dir).unwrap(); + + write_skill_at( + &outer_dir + .path() + .join(REPO_ROOT_CONFIG_DIR_NAME) + .join(SKILLS_DIR_NAME), + "outer", + "outer-skill", + "from outer", + ); + + let mut cfg = make_config(&codex_home); + cfg.cwd = nested_dir; + + let outcome = load_skills(&cfg); + assert!( + outcome.errors.is_empty(), + "unexpected errors: {:?}", + outcome.errors + ); + assert_eq!(outcome.skills.len(), 0); + } + + #[test] + fn loads_skills_from_public_cache_when_present() { + let codex_home = tempfile::tempdir().expect("tempdir"); + let work_dir = tempfile::tempdir().expect("tempdir"); + + write_skill_at( + &codex_home.path().join("skills").join(".public"), + "public", + "public-skill", + "from public", + ); + + let mut cfg = make_config(&codex_home); + cfg.cwd = work_dir.path().to_path_buf(); + + let outcome = load_skills(&cfg); + assert!( + outcome.errors.is_empty(), + "unexpected errors: {:?}", + outcome.errors + ); + assert_eq!(outcome.skills.len(), 1); + assert_eq!(outcome.skills[0].name, "public-skill"); + assert_eq!(outcome.skills[0].scope, SkillScope::Public); + } + + #[test] + fn deduplicates_by_name_preferring_user_over_public() { + let codex_home = tempfile::tempdir().expect("tempdir"); + let work_dir = tempfile::tempdir().expect("tempdir"); + + write_skill(&codex_home, "user", "dupe-skill", "from user"); + write_skill_at( + &codex_home.path().join("skills").join(".public"), + "public", + "dupe-skill", + "from public", + ); + + let mut cfg = make_config(&codex_home); + cfg.cwd = work_dir.path().to_path_buf(); + + let outcome = load_skills(&cfg); + assert!( + outcome.errors.is_empty(), + "unexpected errors: {:?}", + outcome.errors + ); + assert_eq!(outcome.skills.len(), 1); + assert_eq!(outcome.skills[0].name, "dupe-skill"); + assert_eq!(outcome.skills[0].scope, SkillScope::User); + } + + #[test] + fn deduplicates_by_name_preferring_repo_over_public() { + let codex_home = tempfile::tempdir().expect("tempdir"); + let repo_dir = tempfile::tempdir().expect("tempdir"); + + let status = Command::new("git") + .arg("init") + .current_dir(repo_dir.path()) + .status() + .expect("git init"); + assert!(status.success(), "git init failed"); + + write_skill_at( + &repo_dir + .path() + .join(REPO_ROOT_CONFIG_DIR_NAME) + .join(SKILLS_DIR_NAME), + "repo", + "dupe-skill", + "from repo", + ); + write_skill_at( + &codex_home.path().join("skills").join(".public"), + "public", + "dupe-skill", + "from public", + ); + + let mut cfg = make_config(&codex_home); + cfg.cwd = repo_dir.path().to_path_buf(); + + let outcome = load_skills(&cfg); + assert!( + outcome.errors.is_empty(), + "unexpected errors: {:?}", + outcome.errors + ); + assert_eq!(outcome.skills.len(), 1); + assert_eq!(outcome.skills[0].name, "dupe-skill"); + assert_eq!(outcome.skills[0].scope, SkillScope::Repo); + } } diff --git a/codex-rs/core/src/skills/manager.rs b/codex-rs/core/src/skills/manager.rs index a031d66a98..f1c9f36a38 100644 --- a/codex-rs/core/src/skills/manager.rs +++ b/codex-rs/core/src/skills/manager.rs @@ -2,38 +2,82 @@ use std::collections::HashMap; use std::path::Path; use std::path::PathBuf; use std::sync::RwLock; +use std::sync::atomic::AtomicBool; +use std::sync::atomic::Ordering; use crate::skills::SkillLoadOutcome; use crate::skills::loader::load_skills_from_roots; +use crate::skills::loader::public_skills_root; use crate::skills::loader::repo_skills_root; use crate::skills::loader::user_skills_root; +use crate::skills::public::refresh_public_skills; +use tokio::sync::broadcast; pub struct SkillsManager { codex_home: PathBuf, cache_by_cwd: RwLock>, + attempted_public_refresh: AtomicBool, + skills_update_tx: broadcast::Sender<()>, } impl SkillsManager { pub fn new(codex_home: PathBuf) -> Self { + let (skills_update_tx, _skills_update_rx) = broadcast::channel(1); Self { codex_home, cache_by_cwd: RwLock::new(HashMap::new()), + attempted_public_refresh: AtomicBool::new(false), + skills_update_tx, } } + pub(crate) fn subscribe_skills_update_notifications(&self) -> broadcast::Receiver<()> { + self.skills_update_tx.subscribe() + } + pub fn skills_for_cwd(&self, cwd: &Path) -> SkillLoadOutcome { + self.skills_for_cwd_with_options(cwd, false) + } + + pub(crate) fn skills_for_cwd_with_options( + &self, + cwd: &Path, + force_reload: bool, + ) -> SkillLoadOutcome { + // Best-effort refresh: attempt at most once per manager instance. + if self + .attempted_public_refresh + .compare_exchange(false, true, Ordering::Acquire, Ordering::Relaxed) + .is_ok() + { + let codex_home = self.codex_home.clone(); + let skills_update_tx = self.skills_update_tx.clone(); + std::thread::spawn(move || match refresh_public_skills(&codex_home) { + Ok(outcome) => { + if outcome.updated() { + let _ = skills_update_tx.send(()); + } + } + Err(err) => { + tracing::error!("failed to refresh public skills: {err}"); + } + }); + } + let cached = match self.cache_by_cwd.read() { Ok(cache) => cache.get(cwd).cloned(), Err(err) => err.into_inner().get(cwd).cloned(), }; - if let Some(outcome) = cached { + if !force_reload && let Some(outcome) = cached { return outcome; } - let mut roots = vec![user_skills_root(&self.codex_home)]; + let mut roots = Vec::new(); if let Some(repo_root) = repo_skills_root(cwd) { roots.push(repo_root); } + roots.push(user_skills_root(&self.codex_home)); + roots.push(public_skills_root(&self.codex_home)); let outcome = load_skills_from_roots(roots); match self.cache_by_cwd.write() { Ok(mut cache) => { diff --git a/codex-rs/core/src/skills/mod.rs b/codex-rs/core/src/skills/mod.rs index 9d15f0333c..4872b6a2f1 100644 --- a/codex-rs/core/src/skills/mod.rs +++ b/codex-rs/core/src/skills/mod.rs @@ -2,6 +2,7 @@ pub mod injection; pub mod loader; pub mod manager; pub mod model; +pub mod public; pub mod render; pub(crate) use injection::SkillInjections; diff --git a/codex-rs/core/src/skills/public.rs b/codex-rs/core/src/skills/public.rs new file mode 100644 index 0000000000..8909d6a55b --- /dev/null +++ b/codex-rs/core/src/skills/public.rs @@ -0,0 +1,397 @@ +use std::fs; +use std::path::Path; +use std::path::PathBuf; +use std::process::ExitStatus; +use std::time::SystemTime; +use std::time::UNIX_EPOCH; + +use thiserror::Error; + +const PUBLIC_SKILLS_REPO_URL: &str = "https://github.com/openai/skills.git"; +const PUBLIC_SKILLS_DIR_NAME: &str = ".public"; +const SKILLS_DIR_NAME: &str = "skills"; + +struct TempDirCleanup { + path: PathBuf, + // Disable Drop cleanup after explicit cleanup to avoid double-delete. + active: bool, +} + +impl TempDirCleanup { + fn new(path: PathBuf) -> Self { + Self { path, active: true } + } + + fn cleanup(&mut self) -> Result<(), PublicSkillsError> { + if self.active && self.path.exists() { + fs::remove_dir_all(&self.path) + .map_err(|source| PublicSkillsError::io("remove public skills tmp dir", source))?; + } + self.active = false; + Ok(()) + } +} + +impl Drop for TempDirCleanup { + fn drop(&mut self) { + if self.active && self.path.exists() { + let _ = fs::remove_dir_all(&self.path); + } + } +} + +pub(crate) fn public_cache_root_dir(codex_home: &Path) -> PathBuf { + codex_home + .join(SKILLS_DIR_NAME) + .join(PUBLIC_SKILLS_DIR_NAME) +} + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(crate) enum PublicSkillsRefreshOutcome { + Skipped, + Updated, +} + +impl PublicSkillsRefreshOutcome { + pub(crate) fn updated(self) -> bool { + matches!(self, Self::Updated) + } +} + +pub(crate) fn refresh_public_skills( + codex_home: &Path, +) -> Result { + // Keep tests deterministic and offline-safe. Tests that want to exercise the + // refresh behavior should call `refresh_public_skills_from_repo_url`. + if cfg!(test) { + return Ok(PublicSkillsRefreshOutcome::Skipped); + } + refresh_public_skills_inner(codex_home, PUBLIC_SKILLS_REPO_URL) +} + +#[cfg(test)] +pub(crate) fn refresh_public_skills_from_repo_url( + codex_home: &Path, + repo_url: &str, +) -> Result { + refresh_public_skills_inner(codex_home, repo_url) +} + +fn refresh_public_skills_inner( + codex_home: &Path, + repo_url: &str, +) -> Result { + // Best-effort refresh: clone the repo to a temp dir, stage its `skills/`, then atomically swap + // the staged directory into the public cache. + let skills_root_dir = codex_home.join(SKILLS_DIR_NAME); + fs::create_dir_all(&skills_root_dir) + .map_err(|source| PublicSkillsError::io("create skills root dir", source))?; + + let dest_public = public_cache_root_dir(codex_home); + + let tmp_dir = skills_root_dir.join(format!(".public-tmp-{}", rand_suffix())); + if tmp_dir.exists() { + fs::remove_dir_all(&tmp_dir).map_err(|source| { + PublicSkillsError::io("remove existing public skills tmp dir", source) + })?; + } + fs::create_dir_all(&tmp_dir) + .map_err(|source| PublicSkillsError::io("create public skills tmp dir", source))?; + let mut tmp_dir_cleanup = TempDirCleanup::new(tmp_dir.clone()); + + let checkout_dir = tmp_dir.join("checkout"); + clone_repo(repo_url, &checkout_dir)?; + + let src_skills = checkout_dir.join(SKILLS_DIR_NAME); + let src_skills_metadata = fs::symlink_metadata(&src_skills) + .map_err(|source| PublicSkillsError::io("read skills dir metadata", source))?; + let src_skills_type = src_skills_metadata.file_type(); + if src_skills_type.is_symlink() || !src_skills_type.is_dir() { + return Err(PublicSkillsError::RepoMissingSkillsDir { + skills_dir_name: SKILLS_DIR_NAME, + }); + } + + let staged_public = tmp_dir.join(PUBLIC_SKILLS_DIR_NAME); + stage_skills_dir(&src_skills, &staged_public)?; + + atomic_swap_dir(&staged_public, &dest_public, &skills_root_dir)?; + + tmp_dir_cleanup.cleanup()?; + Ok(PublicSkillsRefreshOutcome::Updated) +} + +fn stage_skills_dir(src: &Path, staged: &Path) -> Result<(), PublicSkillsError> { + fs::rename(src, staged).map_err(|source| PublicSkillsError::io("stage skills dir", source))?; + + prune_symlinks_and_special_files(staged)?; + Ok(()) +} + +fn prune_symlinks_and_special_files(root: &Path) -> Result<(), PublicSkillsError> { + let mut stack: Vec = vec![root.to_path_buf()]; + while let Some(dir) = stack.pop() { + for entry in fs::read_dir(&dir) + .map_err(|source| PublicSkillsError::io("read staged skills dir", source))? + { + let entry = entry + .map_err(|source| PublicSkillsError::io("read staged skills dir entry", source))?; + let file_type = entry + .file_type() + .map_err(|source| PublicSkillsError::io("read staged skills entry type", source))?; + let path = entry.path(); + + if file_type.is_symlink() { + fs::remove_file(&path).map_err(|source| { + PublicSkillsError::io("remove symlink from staged skills", source) + })?; + continue; + } + + if file_type.is_dir() { + stack.push(path); + continue; + } + + if file_type.is_file() { + continue; + } + + fs::remove_file(&path).map_err(|source| { + PublicSkillsError::io("remove special file from staged skills", source) + })?; + } + } + + Ok(()) +} + +fn clone_repo(repo_url: &str, checkout_dir: &Path) -> Result<(), PublicSkillsError> { + let out = std::process::Command::new("git") + .env("GIT_TERMINAL_PROMPT", "0") + .env("GIT_ASKPASS", "true") + .arg("clone") + .arg("--depth") + .arg("1") + .arg(repo_url) + .arg(checkout_dir) + .stdin(std::process::Stdio::null()) + .output() + .map_err(|source| PublicSkillsError::io("spawn `git clone`", source))?; + if !out.status.success() { + let stderr = String::from_utf8_lossy(&out.stderr); + let stderr = stderr.trim(); + return if stderr.is_empty() { + Err(PublicSkillsError::GitCloneFailed { status: out.status }) + } else { + Err(PublicSkillsError::GitCloneFailedWithStderr { + status: out.status, + stderr: stderr.to_owned(), + }) + }; + } + Ok(()) +} + +fn atomic_swap_dir(staged: &Path, dest: &Path, parent: &Path) -> Result<(), PublicSkillsError> { + if let Some(dest_parent) = dest.parent() { + fs::create_dir_all(dest_parent) + .map_err(|source| PublicSkillsError::io("create public skills dest parent", source))?; + } + + let backup_base = dest + .file_name() + .and_then(|name| name.to_str()) + .unwrap_or("skills"); + let backup = parent.join(format!("{backup_base}.old-{}", rand_suffix())); + if backup.exists() { + fs::remove_dir_all(&backup) + .map_err(|source| PublicSkillsError::io("remove old public skills backup", source))?; + } + + if dest.exists() { + fs::rename(dest, &backup) + .map_err(|source| PublicSkillsError::io("rename public skills to backup", source))?; + } + + if let Err(err) = fs::rename(staged, dest) { + if backup.exists() { + let _ = fs::rename(&backup, dest); + } + return Err(PublicSkillsError::io( + "rename staged public skills into place", + err, + )); + } + + if backup.exists() { + fs::remove_dir_all(&backup) + .map_err(|source| PublicSkillsError::io("remove public skills backup", source))?; + } + + Ok(()) +} + +fn rand_suffix() -> String { + let pid = std::process::id(); + let nanos = SystemTime::now() + .duration_since(UNIX_EPOCH) + .unwrap_or_default() + .as_nanos(); + format!("{pid:x}-{nanos:x}") +} + +#[derive(Debug, Error)] +pub(crate) enum PublicSkillsError { + #[error("io error while {action}: {source}")] + Io { + action: &'static str, + #[source] + source: std::io::Error, + }, + + #[error("repo did not contain a `{skills_dir_name}` directory")] + RepoMissingSkillsDir { skills_dir_name: &'static str }, + + #[error("`git clone` failed with status {status}")] + GitCloneFailed { status: ExitStatus }, + + #[error("`git clone` failed with status {status}: {stderr}")] + GitCloneFailedWithStderr { status: ExitStatus, stderr: String }, +} + +impl PublicSkillsError { + fn io(action: &'static str, source: std::io::Error) -> Self { + Self::Io { action, source } + } +} + +#[cfg(test)] +mod tests { + use super::*; + use pretty_assertions::assert_eq; + use tempfile::TempDir; + + fn write_public_skill(repo_dir: &TempDir, name: &str, description: &str) { + let skills_dir = repo_dir.path().join("skills").join(name); + fs::create_dir_all(&skills_dir).unwrap(); + let content = format!("---\nname: {name}\ndescription: {description}\n---\n\n# Body\n"); + fs::write(skills_dir.join("SKILL.md"), content).unwrap(); + } + + fn git(repo_dir: &TempDir, args: &[&str]) { + let status = std::process::Command::new("git") + .args([ + "-c", + "user.name=codex-test", + "-c", + "user.email=codex-test@example.com", + ]) + .args(args) + .current_dir(repo_dir.path()) + .status() + .unwrap(); + assert!(status.success(), "git command failed: {args:?}"); + } + + #[tokio::test] + async fn refresh_copies_skills_subdir_into_public_cache() { + let codex_home = tempfile::tempdir().unwrap(); + let repo_dir = tempfile::tempdir().unwrap(); + git(&repo_dir, &["init"]); + write_public_skill(&repo_dir, "demo", "from repo"); + git(&repo_dir, &["add", "."]); + git(&repo_dir, &["commit", "-m", "init"]); + + refresh_public_skills_from_repo_url(codex_home.path(), repo_dir.path().to_str().unwrap()) + .unwrap(); + + let path = public_cache_root_dir(codex_home.path()) + .join("demo") + .join("SKILL.md"); + let contents = fs::read_to_string(path).unwrap(); + assert!(contents.contains("name: demo")); + assert!(contents.contains("description: from repo")); + } + + #[tokio::test] + async fn refresh_overwrites_existing_public_cache() { + let codex_home = tempfile::tempdir().unwrap(); + let repo_dir = tempfile::tempdir().unwrap(); + git(&repo_dir, &["init"]); + write_public_skill(&repo_dir, "demo", "v1"); + git(&repo_dir, &["add", "."]); + git(&repo_dir, &["commit", "-m", "v1"]); + + refresh_public_skills_from_repo_url(codex_home.path(), repo_dir.path().to_str().unwrap()) + .unwrap(); + + write_public_skill(&repo_dir, "demo", "v2"); + git(&repo_dir, &["add", "."]); + git(&repo_dir, &["commit", "-m", "v2"]); + + refresh_public_skills_from_repo_url(codex_home.path(), repo_dir.path().to_str().unwrap()) + .unwrap(); + + let path = public_cache_root_dir(codex_home.path()) + .join("demo") + .join("SKILL.md"); + let contents = fs::read_to_string(path).unwrap(); + assert_eq!(contents.matches("description:").count(), 1); + assert!(contents.contains("description: v2")); + } + + #[cfg(unix)] + #[tokio::test] + async fn refresh_prunes_symlinks_inside_skills_dir() { + use std::os::unix::fs::symlink; + + let codex_home = tempfile::tempdir().unwrap(); + let repo_dir = tempfile::tempdir().unwrap(); + git(&repo_dir, &["init"]); + write_public_skill(&repo_dir, "demo", "from repo"); + + let demo_dir = repo_dir.path().join("skills").join("demo"); + symlink("SKILL.md", demo_dir.join("link-to-skill")).unwrap(); + git(&repo_dir, &["add", "."]); + git(&repo_dir, &["commit", "-m", "init"]); + + refresh_public_skills_from_repo_url(codex_home.path(), repo_dir.path().to_str().unwrap()) + .unwrap(); + + assert!( + !public_cache_root_dir(codex_home.path()) + .join("demo") + .join("link-to-skill") + .exists() + ); + } + + #[cfg(unix)] + #[tokio::test] + async fn refresh_rejects_symlinked_skills_dir() { + use std::os::unix::fs::symlink; + + let codex_home = tempfile::tempdir().unwrap(); + let repo_dir = tempfile::tempdir().unwrap(); + git(&repo_dir, &["init"]); + + let skills_target = repo_dir.path().join("skills-target"); + fs::create_dir_all(skills_target.join("demo")).unwrap(); + fs::write( + skills_target.join("demo").join("SKILL.md"), + "---\nname: demo\ndescription: from repo\n---\n", + ) + .unwrap(); + symlink("skills-target", repo_dir.path().join("skills")).unwrap(); + git(&repo_dir, &["add", "."]); + git(&repo_dir, &["commit", "-m", "init"]); + + let err = refresh_public_skills_from_repo_url( + codex_home.path(), + repo_dir.path().to_str().unwrap(), + ) + .unwrap_err(); + assert!(err.to_string().contains("repo did not contain")); + } +} diff --git a/codex-rs/core/src/skills/render.rs b/codex-rs/core/src/skills/render.rs index b664565459..f767849b24 100644 --- a/codex-rs/core/src/skills/render.rs +++ b/codex-rs/core/src/skills/render.rs @@ -7,14 +7,13 @@ pub fn render_skills_section(skills: &[SkillMetadata]) -> Option { let mut lines: Vec = Vec::new(); lines.push("## Skills".to_string()); - lines.push("These skills are discovered at startup from ~/.codex/skills; each entry shows name, description, and file path so you can open the source for full instructions. Content is not inlined to keep context lean.".to_string()); + lines.push("These skills are discovered at startup from multiple local sources. Each entry includes a name, description, and file path so you can open the source for full instructions.".to_string()); for skill in skills { let path_str = skill.path.to_string_lossy().replace('\\', "/"); - lines.push(format!( - "- {}: {} (file: {})", - skill.name, skill.description, path_str - )); + let name = skill.name.as_str(); + let description = skill.description.as_str(); + lines.push(format!("- {name}: {description} (file: {path_str})")); } lines.push( diff --git a/codex-rs/core/src/tasks/ghost_snapshot.rs b/codex-rs/core/src/tasks/ghost_snapshot.rs index ec6f829c79..48a6137c6d 100644 --- a/codex-rs/core/src/tasks/ghost_snapshot.rs +++ b/codex-rs/core/src/tasks/ghost_snapshot.rs @@ -41,31 +41,36 @@ impl SessionTask for GhostSnapshotTask { ) -> Option { tokio::task::spawn(async move { let token = self.token; + let warnings_enabled = !ctx.ghost_snapshot.disable_warnings; // Channel used to signal when the snapshot work has finished so the // timeout warning task can exit early without sending a warning. let (snapshot_done_tx, snapshot_done_rx) = oneshot::channel::<()>(); - let ctx_for_warning = ctx.clone(); - let cancellation_token_for_warning = cancellation_token.clone(); - let session_for_warning = session.clone(); - // Fire a generic warning if the snapshot is still running after - // three minutes; this helps users discover large untracked files - // that might need to be added to .gitignore. - tokio::task::spawn(async move { - tokio::select! { - _ = tokio::time::sleep(SNAPSHOT_WARNING_THRESHOLD) => { - session_for_warning.session - .send_event( - &ctx_for_warning, - EventMsg::Warning(WarningEvent { - message: "Repository snapshot is taking longer than expected. Large untracked or ignored files can slow snapshots; consider adding large files or directories to .gitignore or disabling `undo` in your config.".to_string() - }), - ) - .await; + if warnings_enabled { + let ctx_for_warning = ctx.clone(); + let cancellation_token_for_warning = cancellation_token.clone(); + let session_for_warning = session.clone(); + // Fire a generic warning if the snapshot is still running after + // three minutes; this helps users discover large untracked files + // that might need to be added to .gitignore. + tokio::task::spawn(async move { + tokio::select! { + _ = tokio::time::sleep(SNAPSHOT_WARNING_THRESHOLD) => { + session_for_warning.session + .send_event( + &ctx_for_warning, + EventMsg::Warning(WarningEvent { + message: "Repository snapshot is taking longer than expected. Large untracked or ignored files can slow snapshots; consider adding large files or directories to .gitignore or disabling `undo` in your config.".to_string() + }), + ) + .await; + } + _ = snapshot_done_rx => {} + _ = cancellation_token_for_warning.cancelled() => {} } - _ = snapshot_done_rx => {} - _ = cancellation_token_for_warning.cancelled() => {} - } - }); + }); + } else { + drop(snapshot_done_rx); + } let ctx_for_task = ctx.clone(); let cancelled = tokio::select! { @@ -84,18 +89,20 @@ impl SessionTask for GhostSnapshotTask { { Ok(Ok((ghost_commit, report))) => { info!("ghost snapshot blocking task finished"); - for message in format_snapshot_warnings( - ghost_snapshot.ignore_large_untracked_files, - ghost_snapshot.ignore_large_untracked_dirs, - &report, - ) { - session - .session - .send_event( - &ctx_for_task, - EventMsg::Warning(WarningEvent { message }), - ) - .await; + if warnings_enabled { + for message in format_snapshot_warnings( + ghost_snapshot.ignore_large_untracked_files, + ghost_snapshot.ignore_large_untracked_dirs, + &report, + ) { + session + .session + .send_event( + &ctx_for_task, + EventMsg::Warning(WarningEvent { message }), + ) + .await; + } } session .session diff --git a/codex-rs/core/src/tools/context.rs b/codex-rs/core/src/tools/context.rs index b226fb11e3..abe488681e 100644 --- a/codex-rs/core/src/tools/context.rs +++ b/codex-rs/core/src/tools/context.rs @@ -37,9 +37,6 @@ pub enum ToolPayload { LocalShell { params: ShellToolCallParams, }, - UnifiedExec { - arguments: String, - }, Mcp { server: String, tool: String, @@ -53,7 +50,6 @@ impl ToolPayload { ToolPayload::Function { arguments } => Cow::Borrowed(arguments), ToolPayload::Custom { input } => Cow::Borrowed(input), ToolPayload::LocalShell { params } => Cow::Owned(params.command.join(" ")), - ToolPayload::UnifiedExec { arguments } => Cow::Borrowed(arguments), ToolPayload::Mcp { raw_arguments, .. } => Cow::Borrowed(raw_arguments), } } diff --git a/codex-rs/core/src/tools/handlers/unified_exec.rs b/codex-rs/core/src/tools/handlers/unified_exec.rs index e81c35186e..00418d8a65 100644 --- a/codex-rs/core/src/tools/handlers/unified_exec.rs +++ b/codex-rs/core/src/tools/handlers/unified_exec.rs @@ -77,16 +77,15 @@ impl ToolHandler for UnifiedExecHandler { } fn matches_kind(&self, payload: &ToolPayload) -> bool { - matches!( - payload, - ToolPayload::Function { .. } | ToolPayload::UnifiedExec { .. } - ) + matches!(payload, ToolPayload::Function { .. }) } async fn is_mutating(&self, invocation: &ToolInvocation) -> bool { - let (ToolPayload::Function { arguments } | ToolPayload::UnifiedExec { arguments }) = - &invocation.payload - else { + let ToolPayload::Function { arguments } = &invocation.payload else { + tracing::error!( + "This should never happen, invocation payload is wrong: {:?}", + invocation.payload + ); return true; }; @@ -110,7 +109,6 @@ impl ToolHandler for UnifiedExecHandler { let arguments = match payload { ToolPayload::Function { arguments } => arguments, - ToolPayload::UnifiedExec { arguments } => arguments, _ => { return Err(FunctionCallError::RespondToModel( "unified_exec handler received unsupported payload".to_string(), diff --git a/codex-rs/core/src/tools/router.rs b/codex-rs/core/src/tools/router.rs index 66e6026b3a..3fb482bb7c 100644 --- a/codex-rs/core/src/tools/router.rs +++ b/codex-rs/core/src/tools/router.rs @@ -78,15 +78,10 @@ impl ToolRouter { }, })) } else { - let payload = if name == "unified_exec" { - ToolPayload::UnifiedExec { arguments } - } else { - ToolPayload::Function { arguments } - }; Ok(Some(ToolCall { tool_name: name, call_id, - payload, + payload: ToolPayload::Function { arguments }, })) } } diff --git a/codex-rs/core/src/tools/spec.rs b/codex-rs/core/src/tools/spec.rs index ea445e8789..35ebaf736f 100644 --- a/codex-rs/core/src/tools/spec.rs +++ b/codex-rs/core/src/tools/spec.rs @@ -43,7 +43,12 @@ impl ToolsConfig { let shell_type = if !features.enabled(Feature::ShellTool) { ConfigShellToolType::Disabled } else if features.enabled(Feature::UnifiedExec) { - ConfigShellToolType::UnifiedExec + // If ConPTY not supported (for old Windows versions), fallback on ShellCommand. + if codex_utils_pty::conpty_supported() { + ConfigShellToolType::UnifiedExec + } else { + ConfigShellToolType::ShellCommand + } } else { model_family.shell_type }; @@ -153,8 +158,7 @@ fn create_exec_command_tool() -> ToolSpec { "login".to_string(), JsonSchema::Boolean { description: Some( - "Whether to run the shell with -l/-i semantics. Defaults to false unless a shell snapshot is available." - .to_string(), + "Whether to run the shell with -l/-i semantics. Defaults to true.".to_string(), ), }, ); @@ -336,7 +340,7 @@ fn create_shell_command_tool() -> ToolSpec { "login".to_string(), JsonSchema::Boolean { description: Some( - "Whether to run the shell with login shell semantics. Defaults to false unless a shell snapshot is available." + "Whether to run the shell with login shell semantics. Defaults to true." .to_string(), ), }, diff --git a/codex-rs/core/src/unified_exec/session_manager.rs b/codex-rs/core/src/unified_exec/session_manager.rs index 4b24c574ac..17d5ef671a 100644 --- a/codex-rs/core/src/unified_exec/session_manager.rs +++ b/codex-rs/core/src/unified_exec/session_manager.rs @@ -175,7 +175,6 @@ impl UnifiedExecSessionManager { // Short‑lived command: emit ExecCommandEnd immediately using the // same helper as the background watcher, so all end events share // one implementation. - self.release_process_id(&request.process_id).await; let exit = exit_code.unwrap_or(-1); emit_exec_end_for_unified_exec( Arc::clone(&context.session), @@ -191,6 +190,7 @@ impl UnifiedExecSessionManager { ) .await; + self.release_process_id(&request.process_id).await; session.check_for_sandbox_denial_with_text(&text).await?; } else { // Long‑lived command: persist the session so write_stdin can reuse diff --git a/codex-rs/core/tests/suite/approvals.rs b/codex-rs/core/tests/suite/approvals.rs index d1b1a515df..c228680091 100644 --- a/codex-rs/core/tests/suite/approvals.rs +++ b/codex-rs/core/tests/suite/approvals.rs @@ -1,6 +1,7 @@ #![allow(clippy::unwrap_used, clippy::expect_used)] use anyhow::Result; +use codex_core::config::Constrained; use codex_core::features::Feature; use codex_core::protocol::ApplyPatchApprovalRequestEvent; use codex_core::protocol::AskForApproval; @@ -126,7 +127,7 @@ impl ActionKind { ); let command = format!("python3 -c \"{script}\""); - let event = shell_event(call_id, &command, 3_000, sandbox_permissions)?; + let event = shell_event(call_id, &command, 5_000, sandbox_permissions)?; Ok((event, Some(command))) } ActionKind::RunCommand { command } => { @@ -1462,7 +1463,7 @@ async fn run_scenario(scenario: &ScenarioSpec) -> Result<()> { let model = model_override.unwrap_or("gpt-5.1"); let mut builder = test_codex().with_model(model).with_config(move |config| { - config.approval_policy = approval_policy; + config.approval_policy = Constrained::allow_any(approval_policy); config.sandbox_policy = sandbox_policy.clone(); for feature in features { config.features.enable(feature); @@ -1568,7 +1569,7 @@ async fn approving_execpolicy_amendment_persists_policy_and_skips_future_prompts let sandbox_policy = SandboxPolicy::ReadOnly; let sandbox_policy_for_config = sandbox_policy.clone(); let mut builder = test_codex().with_config(move |config| { - config.approval_policy = approval_policy; + config.approval_policy = Constrained::allow_any(approval_policy); config.sandbox_policy = sandbox_policy_for_config; }); let test = builder.build(&server).await?; diff --git a/codex-rs/core/tests/suite/codex_delegate.rs b/codex-rs/core/tests/suite/codex_delegate.rs index 2bd156d6a8..f0c4cb9fe1 100644 --- a/codex-rs/core/tests/suite/codex_delegate.rs +++ b/codex-rs/core/tests/suite/codex_delegate.rs @@ -1,3 +1,4 @@ +use codex_core::config::Constrained; use codex_core::protocol::AskForApproval; use codex_core::protocol::EventMsg; use codex_core::protocol::Op; @@ -61,7 +62,7 @@ async fn codex_delegate_forwards_exec_approval_and_proceeds_on_approval() { // Build a conversation configured to require approvals so the delegate // routes ExecApprovalRequest via the parent. let mut builder = test_codex().with_model("gpt-5.1").with_config(|config| { - config.approval_policy = AskForApproval::OnRequest; + config.approval_policy = Constrained::allow_any(AskForApproval::OnRequest); config.sandbox_policy = SandboxPolicy::ReadOnly; }); let test = builder.build(&server).await.expect("build test codex"); @@ -137,7 +138,7 @@ async fn codex_delegate_forwards_patch_approval_and_proceeds_on_decision() { mount_sse_sequence(&server, vec![sse1, sse2]).await; let mut builder = test_codex().with_model("gpt-5.1").with_config(|config| { - config.approval_policy = AskForApproval::OnRequest; + config.approval_policy = Constrained::allow_any(AskForApproval::OnRequest); // Use a restricted sandbox so patch approval is required config.sandbox_policy = SandboxPolicy::ReadOnly; config.include_apply_patch_tool = true; diff --git a/codex-rs/core/tests/suite/otel.rs b/codex-rs/core/tests/suite/otel.rs index 65e96a4fec..d3def4a1cc 100644 --- a/codex-rs/core/tests/suite/otel.rs +++ b/codex-rs/core/tests/suite/otel.rs @@ -1,3 +1,4 @@ +use codex_core::config::Constrained; use codex_core::features::Feature; use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::EventMsg; @@ -930,7 +931,7 @@ async fn handle_container_exec_autoapprove_from_config_records_tool_decision() { let TestCodex { codex, .. } = test_codex() .with_config(|config| { - config.approval_policy = AskForApproval::OnRequest; + config.approval_policy = Constrained::allow_any(AskForApproval::OnRequest); config.sandbox_policy = SandboxPolicy::DangerFullAccess; }) .build(&server) @@ -979,7 +980,7 @@ async fn handle_container_exec_user_approved_records_tool_decision() { let TestCodex { codex, .. } = test_codex() .with_config(|config| { - config.approval_policy = AskForApproval::UnlessTrusted; + config.approval_policy = Constrained::allow_any(AskForApproval::UnlessTrusted); }) .build(&server) .await @@ -1037,7 +1038,7 @@ async fn handle_container_exec_user_approved_for_session_records_tool_decision() let TestCodex { codex, .. } = test_codex() .with_config(|config| { - config.approval_policy = AskForApproval::UnlessTrusted; + config.approval_policy = Constrained::allow_any(AskForApproval::UnlessTrusted); }) .build(&server) .await @@ -1095,7 +1096,7 @@ async fn handle_sandbox_error_user_approves_retry_records_tool_decision() { let TestCodex { codex, .. } = test_codex() .with_config(|config| { - config.approval_policy = AskForApproval::UnlessTrusted; + config.approval_policy = Constrained::allow_any(AskForApproval::UnlessTrusted); }) .build(&server) .await @@ -1153,7 +1154,7 @@ async fn handle_container_exec_user_denies_records_tool_decision() { .await; let TestCodex { codex, .. } = test_codex() .with_config(|config| { - config.approval_policy = AskForApproval::UnlessTrusted; + config.approval_policy = Constrained::allow_any(AskForApproval::UnlessTrusted); }) .build(&server) .await @@ -1211,7 +1212,7 @@ async fn handle_sandbox_error_user_approves_for_session_records_tool_decision() let TestCodex { codex, .. } = test_codex() .with_config(|config| { - config.approval_policy = AskForApproval::UnlessTrusted; + config.approval_policy = Constrained::allow_any(AskForApproval::UnlessTrusted); }) .build(&server) .await @@ -1270,7 +1271,7 @@ async fn handle_sandbox_error_user_denies_records_tool_decision() { let TestCodex { codex, .. } = test_codex() .with_config(|config| { - config.approval_policy = AskForApproval::UnlessTrusted; + config.approval_policy = Constrained::allow_any(AskForApproval::UnlessTrusted); }) .build(&server) .await diff --git a/codex-rs/core/tests/suite/prompt_caching.rs b/codex-rs/core/tests/suite/prompt_caching.rs index e11139090e..77fd19ad90 100644 --- a/codex-rs/core/tests/suite/prompt_caching.rs +++ b/codex-rs/core/tests/suite/prompt_caching.rs @@ -593,7 +593,7 @@ async fn send_user_turn_with_no_changes_does_not_send_environment_context() -> a .await?; let default_cwd = config.cwd.clone(); - let default_approval_policy = config.approval_policy; + let default_approval_policy = config.approval_policy.value(); let default_sandbox_policy = config.sandbox_policy.clone(); let default_model = session_configured.model; let default_effort = config.model_reasoning_effort; @@ -685,7 +685,7 @@ async fn send_user_turn_with_changes_sends_environment_context() -> anyhow::Resu .await?; let default_cwd = config.cwd.clone(); - let default_approval_policy = config.approval_policy; + let default_approval_policy = config.approval_policy.value(); let default_sandbox_policy = config.sandbox_policy.clone(); let default_model = session_configured.model; let default_effort = config.model_reasoning_effort; diff --git a/codex-rs/core/tests/suite/resume_warning.rs b/codex-rs/core/tests/suite/resume_warning.rs index cb83ab06dc..4b6a133150 100644 --- a/codex-rs/core/tests/suite/resume_warning.rs +++ b/codex-rs/core/tests/suite/resume_warning.rs @@ -24,7 +24,7 @@ fn resume_history( ) -> InitialHistory { let turn_ctx = TurnContextItem { cwd: config.cwd.clone(), - approval_policy: config.approval_policy, + approval_policy: config.approval_policy.value(), sandbox_policy: config.sandbox_policy.clone(), model: previous_model.to_string(), effort: config.model_reasoning_effort, diff --git a/codex-rs/core/tests/suite/shell_snapshot.rs b/codex-rs/core/tests/suite/shell_snapshot.rs index 1d4642ca02..2e3a8f5f1e 100644 --- a/codex-rs/core/tests/suite/shell_snapshot.rs +++ b/codex-rs/core/tests/suite/shell_snapshot.rs @@ -309,6 +309,41 @@ async fn shell_command_snapshot_still_intercepts_apply_patch() -> Result<()> { Ok(()) } +#[cfg_attr(target_os = "windows", ignore)] +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn shell_snapshot_deleted_after_shutdown_with_skills_enabled() -> Result<()> { + let builder = test_codex().with_config(|config| { + config.features.enable(Feature::ShellSnapshot); + config.features.enable(Feature::Skills); + }); + let harness = TestCodexHarness::with_builder(builder).await?; + let home = harness.test().home.clone(); + let codex_home = home.path().to_path_buf(); + let codex = harness.test().codex.clone(); + + let mut entries = fs::read_dir(codex_home.join("shell_snapshots")).await?; + let snapshot_path = entries + .next_entry() + .await? + .map(|entry| entry.path()) + .expect("shell snapshot created"); + assert!(snapshot_path.exists()); + + codex.submit(Op::Shutdown {}).await?; + wait_for_event(&codex, |ev| matches!(ev, EventMsg::ShutdownComplete)).await; + + drop(codex); + drop(harness); + + assert_eq!( + snapshot_path.exists(), + false, + "snapshot should be removed after shutdown" + ); + + Ok(()) +} + #[cfg_attr(not(target_os = "macos"), ignore)] #[cfg_attr( target_os = "macos", diff --git a/codex-rs/core/tests/suite/skills.rs b/codex-rs/core/tests/suite/skills.rs index 25a68be763..b23b3fd5b1 100644 --- a/codex-rs/core/tests/suite/skills.rs +++ b/codex-rs/core/tests/suite/skills.rs @@ -27,6 +27,20 @@ fn write_skill(home: &Path, name: &str, description: &str, body: &str) -> std::p path } +fn write_public_skill( + home: &Path, + name: &str, + description: &str, + body: &str, +) -> std::path::PathBuf { + let skill_dir = home.join("skills").join(".public").join(name); + fs::create_dir_all(&skill_dir).unwrap(); + let contents = format!("---\nname: {name}\ndescription: {description}\n---\n\n{body}\n"); + let path = skill_dir.join("SKILL.md"); + fs::write(&path, contents).unwrap(); + path +} + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn user_turn_includes_skill_instructions() -> Result<()> { skip_if_no_network!(Ok(())); @@ -115,7 +129,10 @@ async fn skill_load_errors_surface_in_session_configured() -> Result<()> { let test = builder.build(&server).await?; test.codex - .submit(Op::ListSkills { cwds: Vec::new() }) + .submit(Op::ListSkills { + cwds: Vec::new(), + force_reload: false, + }) .await?; let response = core_test_support::wait_for_event_match(test.codex.as_ref(), |event| match event { @@ -133,8 +150,13 @@ async fn skill_load_errors_surface_in_session_configured() -> Result<()> { .unwrap_or_default(); assert!( - skills.is_empty(), - "expected no skills loaded, got {skills:?}" + skills.iter().all(|skill| { + !skill + .path + .to_string_lossy() + .ends_with("skills/broken/SKILL.md") + }), + "expected broken skill not loaded, got {skills:?}" ); assert_eq!(errors.len(), 1, "expected one load error"); let error_path = errors[0].path.to_string_lossy(); @@ -145,3 +167,52 @@ async fn skill_load_errors_surface_in_session_configured() -> Result<()> { Ok(()) } + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn list_skills_includes_public_cache_entries() -> Result<()> { + skip_if_no_network!(Ok(())); + + let server = start_mock_server().await; + let mut builder = test_codex() + .with_config(|cfg| { + cfg.features.enable(Feature::Skills); + }) + .with_pre_build_hook(|home| { + write_public_skill(home, "public-demo", "public skill", "public body"); + }); + let test = builder.build(&server).await?; + + test.codex + .submit(Op::ListSkills { + cwds: Vec::new(), + force_reload: true, + }) + .await?; + let response = + core_test_support::wait_for_event_match(test.codex.as_ref(), |event| match event { + codex_core::protocol::EventMsg::ListSkillsResponse(response) => Some(response.clone()), + _ => None, + }) + .await; + + let cwd = test.cwd_path(); + let (skills, _errors) = response + .skills + .iter() + .find(|entry| entry.cwd.as_path() == cwd) + .map(|entry| (entry.skills.clone(), entry.errors.clone())) + .unwrap_or_default(); + + let skill = skills + .iter() + .find(|skill| skill.name == "public-demo") + .expect("expected public skill to be present"); + assert_eq!(skill.scope, codex_protocol::protocol::SkillScope::Public); + let path_str = skill.path.to_string_lossy().replace('\\', "/"); + assert!( + path_str.ends_with("/skills/.public/public-demo/SKILL.md"), + "unexpected skill path: {path_str}" + ); + + Ok(()) +} diff --git a/codex-rs/core/tests/suite/tool_parallelism.rs b/codex-rs/core/tests/suite/tool_parallelism.rs index 0ec5221f40..5a5b443209 100644 --- a/codex-rs/core/tests/suite/tool_parallelism.rs +++ b/codex-rs/core/tests/suite/tool_parallelism.rs @@ -398,8 +398,8 @@ async fn shell_tools_start_before_response_completed_when_stream_delayed() -> an for timestamp in timestamps { assert!( - timestamp < completed_at, - "timestamp {timestamp} should be before completed {completed_at}" + timestamp <= completed_at, + "timestamp {timestamp} should be before or equal to completed {completed_at}" ); } diff --git a/codex-rs/core/tests/suite/unified_exec.rs b/codex-rs/core/tests/suite/unified_exec.rs index af5e4a1497..b540cea35c 100644 --- a/codex-rs/core/tests/suite/unified_exec.rs +++ b/codex-rs/core/tests/suite/unified_exec.rs @@ -229,7 +229,6 @@ async fn unified_exec_intercepts_apply_patch_exec_command() -> Result<()> { false } EventMsg::ExecCommandBegin(event) if event.call_id == call_id => { - println!("Saw it"); saw_exec_begin = true; false } @@ -686,9 +685,10 @@ async fn unified_exec_full_lifecycle_with_background_end_event() -> Result<()> { } = builder.build(&server).await?; let call_id = "uexec-full-lifecycle"; + // This timing force the long-standing PTY let args = json!({ - "cmd": "printf 'HELLO-FULL-LIFECYCLE'", - "yield_time_ms": 250, + "cmd": "sleep 0.5; printf 'HELLO-FULL-LIFECYCLE'", + "yield_time_ms": 1000, }); let responses = vec![ @@ -724,26 +724,28 @@ async fn unified_exec_full_lifecycle_with_background_end_event() -> Result<()> { let mut begin_event = None; let mut end_event = None; - let mut saw_delta_with_marker = 0; + let mut task_completed = false; loop { let msg = wait_for_event(&codex, |_| true).await; match msg { EventMsg::ExecCommandBegin(ev) if ev.call_id == call_id => begin_event = Some(ev), - EventMsg::ExecCommandOutputDelta(ev) if ev.call_id == call_id => { - let text = String::from_utf8_lossy(&ev.chunk); - if text.contains("HELLO-FULL-LIFECYCLE") { - saw_delta_with_marker += 1; - } - } EventMsg::ExecCommandEnd(ev) if ev.call_id == call_id => { assert!( end_event.is_none(), "expected a single ExecCommandEnd event for this call id" ); end_event = Some(ev); + if task_completed && end_event.is_some() { + break; + } + } + EventMsg::TaskComplete(_) => { + task_completed = true; + if task_completed && end_event.is_some() { + break; + } } - EventMsg::TaskComplete(_) => break, _ => {} } } @@ -755,11 +757,6 @@ async fn unified_exec_full_lifecycle_with_background_end_event() -> Result<()> { "begin event should include a process_id for a long-lived session" ); - assert_eq!( - saw_delta_with_marker, 0, - "no ExecCommandOutputDelta should be sent for early exit commands" - ); - let end_event = end_event.expect("expected ExecCommandEnd event"); assert_eq!(end_event.call_id, call_id); assert_eq!(end_event.exit_code, 0); diff --git a/codex-rs/docs/protocol_v1.md b/codex-rs/docs/protocol_v1.md index 075377f61e..682a38bfc8 100644 --- a/codex-rs/docs/protocol_v1.md +++ b/codex-rs/docs/protocol_v1.md @@ -68,7 +68,7 @@ For complete documentation of the `Op` and `EventMsg` variants, refer to [protoc - `Op::UserInput` – Any input from the user to kick off a `Task` - `Op::Interrupt` – Interrupts a running task - `Op::ExecApproval` – Approve or deny code execution - - `Op::ListSkills` – Request skills for one or more cwd values + - `Op::ListSkills` – Request skills for one or more cwd values (optionally `force_reload`) - `EventMsg` - `EventMsg::AgentMessage` – Messages from the `Model` - `EventMsg::ExecApprovalRequest` – Request approval from user to execute a command @@ -77,6 +77,7 @@ For complete documentation of the `Op` and `EventMsg` variants, refer to [protoc - `EventMsg::Warning` – A non-fatal warning that the client should surface to the user - `EventMsg::TurnComplete` – Contains a `response_id` bookmark for last `response_id` executed by the task. This can be used to continue the task at a later point in time, perhaps with additional user input. - `EventMsg::ListSkillsResponse` – Response payload with per-cwd skill entries (`cwd`, `skills`, `errors`) + - `EventMsg::SkillsUpdateAvailable` – Notification that skills may have changed and clients may want to reload The `response_id` returned from each task matches the OpenAI `response_id` stored in the API's `/responses` endpoint. It can be stored and used in future `Sessions` to resume threads of work. diff --git a/codex-rs/exec/src/event_processor_with_human_output.rs b/codex-rs/exec/src/event_processor_with_human_output.rs index a833426dc2..a43718d569 100644 --- a/codex-rs/exec/src/event_processor_with_human_output.rs +++ b/codex-rs/exec/src/event_processor_with_human_output.rs @@ -585,6 +585,7 @@ impl EventProcessor for EventProcessorWithHumanOutput { | EventMsg::AgentMessageContentDelta(_) | EventMsg::ReasoningContentDelta(_) | EventMsg::ReasoningRawContentDelta(_) + | EventMsg::SkillsUpdateAvailable | EventMsg::UndoCompleted(_) | EventMsg::UndoStarted(_) => {} } diff --git a/codex-rs/exec/src/lib.rs b/codex-rs/exec/src/lib.rs index ee0ae45d4e..98debd5201 100644 --- a/codex-rs/exec/src/lib.rs +++ b/codex-rs/exec/src/lib.rs @@ -257,7 +257,7 @@ pub async fn run_main(cli: Cli, codex_linux_sandbox_exe: Option) -> any } let default_cwd = config.cwd.to_path_buf(); - let default_approval_policy = config.approval_policy; + let default_approval_policy = config.approval_policy.value(); let default_sandbox_policy = config.sandbox_policy.clone(); let default_effort = config.model_reasoning_effort; let default_summary = config.model_reasoning_summary; diff --git a/codex-rs/mcp-server/src/codex_tool_runner.rs b/codex-rs/mcp-server/src/codex_tool_runner.rs index 73e56a605c..39ae7486e5 100644 --- a/codex-rs/mcp-server/src/codex_tool_runner.rs +++ b/codex-rs/mcp-server/src/codex_tool_runner.rs @@ -304,6 +304,7 @@ async fn run_codex_tool_session_inner( | EventMsg::AgentMessageContentDelta(_) | EventMsg::ReasoningContentDelta(_) | EventMsg::ReasoningRawContentDelta(_) + | EventMsg::SkillsUpdateAvailable | EventMsg::UndoStarted(_) | EventMsg::UndoCompleted(_) | EventMsg::ExitedReviewMode(_) diff --git a/codex-rs/protocol/src/protocol.rs b/codex-rs/protocol/src/protocol.rs index c333d431ca..0fd01675ca 100644 --- a/codex-rs/protocol/src/protocol.rs +++ b/codex-rs/protocol/src/protocol.rs @@ -193,6 +193,10 @@ pub enum Op { /// When empty, the session default working directory is used. #[serde(default, skip_serializing_if = "Vec::is_empty")] cwds: Vec, + + /// When true, recompute skills even if a cached result exists. + #[serde(default, skip_serializing_if = "std::ops::Not::not")] + force_reload: bool, }, /// Request the agent to summarize the current conversation context. @@ -600,6 +604,9 @@ pub enum EventMsg { /// List of skills available to the agent. ListSkillsResponse(ListSkillsResponseEvent), + /// Notification that skill data may have been updated and clients may want to reload. + SkillsUpdateAvailable, + PlanUpdate(UpdatePlanArgs), TurnAborted(TurnAbortedEvent), @@ -1674,6 +1681,7 @@ pub struct ListSkillsResponseEvent { pub enum SkillScope { User, Repo, + Public, } #[derive(Debug, Clone, Deserialize, Serialize, JsonSchema, TS)] diff --git a/codex-rs/tui/src/app.rs b/codex-rs/tui/src/app.rs index 14be12cb51..c394b6417a 100644 --- a/codex-rs/tui/src/app.rs +++ b/codex-rs/tui/src/app.rs @@ -23,6 +23,7 @@ use codex_ansi_escape::ansi_escape_line; use codex_core::AuthManager; use codex_core::ConversationManager; use codex_core::config::Config; +use codex_core::config::edit::ConfigEdit; use codex_core::config::edit::ConfigEditsBuilder; #[cfg(target_os = "windows")] use codex_core::features::Feature; @@ -944,6 +945,42 @@ impl App { } } } + AppEvent::UpdateFeatureFlags { updates } => { + if updates.is_empty() { + return Ok(true); + } + let mut builder = ConfigEditsBuilder::new(&self.config.codex_home) + .with_profile(self.active_profile.as_deref()); + for (feature, enabled) in &updates { + let feature_key = feature.key(); + if *enabled { + // Update the in-memory configs. + self.config.features.enable(*feature); + self.chat_widget.set_feature_enabled(*feature, true); + builder = builder.set_feature_enabled(feature_key, true); + } else { + // Update the in-memory configs. + self.config.features.disable(*feature); + self.chat_widget.set_feature_enabled(*feature, false); + if feature.default_enabled() { + builder = builder.set_feature_enabled(feature_key, false); + } else { + // If the feature already default to `false`, we drop the key + // in the config file so that the user does not miss the feature + // once it gets globally released. + builder = builder.with_edits(vec![ConfigEdit::ClearPath { + segments: vec!["features".to_string(), feature_key.to_string()], + }]); + } + } + } + if let Err(err) = builder.apply().await { + tracing::error!(error = %err, "failed to persist feature flags"); + self.chat_widget.add_error_message(format!( + "Failed to update experimental features: {err}" + )); + } + } AppEvent::SkipNextWorldWritableScan => { self.skip_world_writable_scan_once = true; } diff --git a/codex-rs/tui/src/app_event.rs b/codex-rs/tui/src/app_event.rs index adb9c1308e..0be556de8b 100644 --- a/codex-rs/tui/src/app_event.rs +++ b/codex-rs/tui/src/app_event.rs @@ -10,6 +10,7 @@ use codex_protocol::openai_models::ModelPreset; use crate::bottom_pane::ApprovalRequest; use crate::history_cell::HistoryCell; +use codex_core::features::Feature; use codex_core::protocol::AskForApproval; use codex_core::protocol::SandboxPolicy; use codex_protocol::openai_models::ReasoningEffort; @@ -117,6 +118,11 @@ pub(crate) enum AppEvent { /// Update the current sandbox policy in the running app and widget. UpdateSandboxPolicy(SandboxPolicy), + /// Update feature flags and persist them to the top-level config. + UpdateFeatureFlags { + updates: Vec<(Feature, bool)>, + }, + /// Update whether the full access warning prompt has been acknowledged. UpdateFullAccessWarningAcknowledged(bool), diff --git a/codex-rs/tui/src/bottom_pane/command_popup.rs b/codex-rs/tui/src/bottom_pane/command_popup.rs index e1e35ae948..dc123f6c2c 100644 --- a/codex-rs/tui/src/bottom_pane/command_popup.rs +++ b/codex-rs/tui/src/bottom_pane/command_popup.rs @@ -185,6 +185,7 @@ impl CommandPopup { display_shortcut: None, description: Some(description), wrap_indent: None, + disabled_reason: None, } }) .collect() diff --git a/codex-rs/tui/src/bottom_pane/experimental_features_view.rs b/codex-rs/tui/src/bottom_pane/experimental_features_view.rs new file mode 100644 index 0000000000..dd0e84eb6d --- /dev/null +++ b/codex-rs/tui/src/bottom_pane/experimental_features_view.rs @@ -0,0 +1,292 @@ +use crossterm::event::KeyCode; +use crossterm::event::KeyEvent; +use crossterm::event::KeyModifiers; +use ratatui::buffer::Buffer; +use ratatui::layout::Constraint; +use ratatui::layout::Layout; +use ratatui::layout::Rect; +use ratatui::style::Stylize; +use ratatui::text::Line; +use ratatui::widgets::Block; +use ratatui::widgets::Widget; + +use crate::app_event::AppEvent; +use crate::app_event_sender::AppEventSender; +use crate::key_hint; +use crate::render::Insets; +use crate::render::RectExt as _; +use crate::render::renderable::ColumnRenderable; +use crate::render::renderable::Renderable; +use crate::style::user_message_style; + +use codex_core::features::Feature; + +use super::CancellationEvent; +use super::bottom_pane_view::BottomPaneView; +use super::popup_consts::MAX_POPUP_ROWS; +use super::scroll_state::ScrollState; +use super::selection_popup_common::GenericDisplayRow; +use super::selection_popup_common::measure_rows_height; +use super::selection_popup_common::render_rows; + +pub(crate) struct BetaFeatureItem { + pub feature: Feature, + pub name: String, + pub description: String, + pub enabled: bool, +} + +pub(crate) struct ExperimentalFeaturesView { + features: Vec, + state: ScrollState, + complete: bool, + app_event_tx: AppEventSender, + header: Box, + footer_hint: Line<'static>, +} + +impl ExperimentalFeaturesView { + pub(crate) fn new(features: Vec, app_event_tx: AppEventSender) -> Self { + let mut header = ColumnRenderable::new(); + header.push(Line::from("Experimental features".bold())); + header.push(Line::from( + "Toggle beta features. Changes are saved to config.toml.".dim(), + )); + + let mut view = Self { + features, + state: ScrollState::new(), + complete: false, + app_event_tx, + header: Box::new(header), + footer_hint: experimental_popup_hint_line(), + }; + view.initialize_selection(); + view + } + + fn initialize_selection(&mut self) { + if self.visible_len() == 0 { + self.state.selected_idx = None; + } else if self.state.selected_idx.is_none() { + self.state.selected_idx = Some(0); + } + } + + fn visible_len(&self) -> usize { + self.features.len() + } + + fn build_rows(&self) -> Vec { + let mut rows = Vec::with_capacity(self.features.len()); + let selected_idx = self.state.selected_idx; + for (idx, item) in self.features.iter().enumerate() { + let prefix = if selected_idx == Some(idx) { + '›' + } else { + ' ' + }; + let marker = if item.enabled { 'x' } else { ' ' }; + let name = format!("{prefix} [{marker}] {}", item.name); + rows.push(GenericDisplayRow { + name, + description: Some(item.description.clone()), + ..Default::default() + }); + } + + rows + } + + fn move_up(&mut self) { + let len = self.visible_len(); + if len == 0 { + return; + } + self.state.move_up_wrap(len); + self.state.ensure_visible(len, MAX_POPUP_ROWS.min(len)); + } + + fn move_down(&mut self) { + let len = self.visible_len(); + if len == 0 { + return; + } + self.state.move_down_wrap(len); + self.state.ensure_visible(len, MAX_POPUP_ROWS.min(len)); + } + + fn toggle_selected(&mut self) { + let Some(selected_idx) = self.state.selected_idx else { + return; + }; + + if let Some(item) = self.features.get_mut(selected_idx) { + item.enabled = !item.enabled; + } + } + + fn rows_width(total_width: u16) -> u16 { + total_width.saturating_sub(2) + } +} + +impl BottomPaneView for ExperimentalFeaturesView { + fn handle_key_event(&mut self, key_event: KeyEvent) { + match key_event { + KeyEvent { + code: KeyCode::Up, .. + } + | KeyEvent { + code: KeyCode::Char('p'), + modifiers: KeyModifiers::CONTROL, + .. + } + | KeyEvent { + code: KeyCode::Char('\u{0010}'), + modifiers: KeyModifiers::NONE, + .. + } => self.move_up(), + KeyEvent { + code: KeyCode::Char('k'), + modifiers: KeyModifiers::NONE, + .. + } => self.move_up(), + KeyEvent { + code: KeyCode::Down, + .. + } + | KeyEvent { + code: KeyCode::Char('n'), + modifiers: KeyModifiers::CONTROL, + .. + } + | KeyEvent { + code: KeyCode::Char('\u{000e}'), + modifiers: KeyModifiers::NONE, + .. + } => self.move_down(), + KeyEvent { + code: KeyCode::Char('j'), + modifiers: KeyModifiers::NONE, + .. + } => self.move_down(), + KeyEvent { + code: KeyCode::Enter, + modifiers: KeyModifiers::NONE, + .. + } => self.toggle_selected(), + KeyEvent { + code: KeyCode::Esc, .. + } => { + self.on_ctrl_c(); + } + _ => {} + } + } + + fn is_complete(&self) -> bool { + self.complete + } + + fn on_ctrl_c(&mut self) -> CancellationEvent { + // Save the updates + if !self.features.is_empty() { + let updates = self + .features + .iter() + .map(|item| (item.feature, item.enabled)) + .collect(); + self.app_event_tx + .send(AppEvent::UpdateFeatureFlags { updates }); + } + + self.complete = true; + CancellationEvent::Handled + } +} + +impl Renderable for ExperimentalFeaturesView { + fn render(&self, area: Rect, buf: &mut Buffer) { + if area.height == 0 || area.width == 0 { + return; + } + + let [content_area, footer_area] = + Layout::vertical([Constraint::Fill(1), Constraint::Length(1)]).areas(area); + + Block::default() + .style(user_message_style()) + .render(content_area, buf); + + let header_height = self + .header + .desired_height(content_area.width.saturating_sub(4)); + let rows = self.build_rows(); + let rows_width = Self::rows_width(content_area.width); + let rows_height = measure_rows_height( + &rows, + &self.state, + MAX_POPUP_ROWS, + rows_width.saturating_add(1), + ); + let [header_area, _, list_area] = Layout::vertical([ + Constraint::Max(header_height), + Constraint::Max(1), + Constraint::Length(rows_height), + ]) + .areas(content_area.inset(Insets::vh(1, 2))); + + self.header.render(header_area, buf); + + if list_area.height > 0 { + let render_area = Rect { + x: list_area.x.saturating_sub(2), + y: list_area.y, + width: rows_width.max(1), + height: list_area.height, + }; + render_rows( + render_area, + buf, + &rows, + &self.state, + MAX_POPUP_ROWS, + " No experimental features available for now", + ); + } + + let hint_area = Rect { + x: footer_area.x + 2, + y: footer_area.y, + width: footer_area.width.saturating_sub(2), + height: footer_area.height, + }; + self.footer_hint.clone().dim().render(hint_area, buf); + } + + fn desired_height(&self, width: u16) -> u16 { + let rows = self.build_rows(); + let rows_width = Self::rows_width(width); + let rows_height = measure_rows_height( + &rows, + &self.state, + MAX_POPUP_ROWS, + rows_width.saturating_add(1), + ); + + let mut height = self.header.desired_height(width.saturating_sub(4)); + height = height.saturating_add(rows_height + 3); + height.saturating_add(1) + } +} + +fn experimental_popup_hint_line() -> Line<'static> { + Line::from(vec![ + "Press ".into(), + key_hint::plain(KeyCode::Enter).into(), + " to toggle or ".into(), + key_hint::plain(KeyCode::Esc).into(), + " to save for next conversation".into(), + ]) +} diff --git a/codex-rs/tui/src/bottom_pane/file_search_popup.rs b/codex-rs/tui/src/bottom_pane/file_search_popup.rs index 064e4f0137..48de1cff54 100644 --- a/codex-rs/tui/src/bottom_pane/file_search_popup.rs +++ b/codex-rs/tui/src/bottom_pane/file_search_popup.rs @@ -132,6 +132,7 @@ impl WidgetRef for &FileSearchPopup { display_shortcut: None, description: None, wrap_indent: None, + disabled_reason: None, }) .collect() }; diff --git a/codex-rs/tui/src/bottom_pane/list_selection_view.rs b/codex-rs/tui/src/bottom_pane/list_selection_view.rs index 46d6daac60..e387b66861 100644 --- a/codex-rs/tui/src/bottom_pane/list_selection_view.rs +++ b/codex-rs/tui/src/bottom_pane/list_selection_view.rs @@ -44,6 +44,7 @@ pub(crate) struct SelectionItem { pub actions: Vec, pub dismiss_on_select: bool, pub search_value: Option, + pub disabled_reason: Option, } pub(crate) struct SelectionViewParams { @@ -217,6 +218,7 @@ impl ListSelectionView { match_indices: None, description, wrap_indent, + disabled_reason: item.disabled_reason.clone(), } }) }) @@ -228,6 +230,7 @@ impl ListSelectionView { self.state.move_up_wrap(len); let visible = Self::max_visible_rows(len); self.state.ensure_visible(len, visible); + self.skip_disabled_up(); } fn move_down(&mut self) { @@ -235,12 +238,14 @@ impl ListSelectionView { self.state.move_down_wrap(len); let visible = Self::max_visible_rows(len); self.state.ensure_visible(len, visible); + self.skip_disabled_down(); } fn accept(&mut self) { if let Some(idx) = self.state.selected_idx && let Some(actual_idx) = self.filtered_indices.get(idx) && let Some(item) = self.items.get(*actual_idx) + && item.disabled_reason.is_none() { self.last_selected_actual_idx = Some(*actual_idx); for act in &item.actions { @@ -267,6 +272,40 @@ impl ListSelectionView { fn rows_width(total_width: u16) -> u16 { total_width.saturating_sub(2) } + + fn skip_disabled_down(&mut self) { + let len = self.visible_len(); + for _ in 0..len { + if let Some(idx) = self.state.selected_idx + && let Some(actual_idx) = self.filtered_indices.get(idx) + && self + .items + .get(*actual_idx) + .is_some_and(|item| item.disabled_reason.is_some()) + { + self.state.move_down_wrap(len); + } else { + break; + } + } + } + + fn skip_disabled_up(&mut self) { + let len = self.visible_len(); + for _ in 0..len { + if let Some(idx) = self.state.selected_idx + && let Some(actual_idx) = self.filtered_indices.get(idx) + && self + .items + .get(*actual_idx) + .is_some_and(|item| item.disabled_reason.is_some()) + { + self.state.move_up_wrap(len); + } else { + break; + } + } + } } impl BottomPaneView for ListSelectionView { @@ -348,6 +387,10 @@ impl BottomPaneView for ListSelectionView { .map(|d| d as usize) .and_then(|d| d.checked_sub(1)) && idx < self.items.len() + && self + .items + .get(idx) + .is_some_and(|item| item.disabled_reason.is_none()) { self.state.selected_idx = Some(idx); self.accept(); diff --git a/codex-rs/tui/src/bottom_pane/mod.rs b/codex-rs/tui/src/bottom_pane/mod.rs index 8516687284..ea599698b9 100644 --- a/codex-rs/tui/src/bottom_pane/mod.rs +++ b/codex-rs/tui/src/bottom_pane/mod.rs @@ -3,6 +3,7 @@ use std::path::PathBuf; use crate::app_event_sender::AppEventSender; use crate::bottom_pane::queued_user_messages::QueuedUserMessages; +use crate::bottom_pane::unified_exec_footer::UnifiedExecFooter; use crate::render::renderable::FlexRenderable; use crate::render::renderable::Renderable; use crate::render::renderable::RenderableItem; @@ -25,6 +26,7 @@ mod chat_composer; mod chat_composer_history; mod command_popup; pub mod custom_prompt_view; +mod experimental_features_view; mod file_search_popup; mod footer; mod list_selection_view; @@ -40,6 +42,7 @@ mod queued_user_messages; mod scroll_state; mod selection_popup_common; mod textarea; +mod unified_exec_footer; pub(crate) use feedback_view::FeedbackNoteView; #[derive(Debug, Clone, Copy, PartialEq, Eq)] @@ -53,6 +56,8 @@ pub(crate) use chat_composer::InputResult; use codex_protocol::custom_prompts::CustomPrompt; use crate::status_indicator_widget::StatusIndicatorWidget; +pub(crate) use experimental_features_view::BetaFeatureItem; +pub(crate) use experimental_features_view::ExperimentalFeaturesView; pub(crate) use list_selection_view::SelectionAction; pub(crate) use list_selection_view::SelectionItem; @@ -76,6 +81,8 @@ pub(crate) struct BottomPane { /// Inline status indicator shown above the composer while a task is running. status: Option, + /// Unified exec session summary shown above the composer. + unified_exec_footer: UnifiedExecFooter, /// Queued user messages to show above the composer while a turn is running. queued_user_messages: QueuedUserMessages, context_window_percent: Option, @@ -123,6 +130,7 @@ impl BottomPane { is_task_running: false, ctrl_c_quit_hint: false, status: None, + unified_exec_footer: UnifiedExecFooter::new(), queued_user_messages: QueuedUserMessages::new(), esc_backtrack_hint: false, animations_enabled, @@ -393,6 +401,12 @@ impl BottomPane { self.request_redraw(); } + pub(crate) fn set_unified_exec_sessions(&mut self, sessions: Vec) { + if self.unified_exec_footer.set_sessions(sessions) { + self.request_redraw(); + } + } + /// Update custom prompts available for the slash popup. pub(crate) fn set_custom_prompts(&mut self, prompts: Vec) { self.composer.set_custom_prompts(prompts); @@ -523,8 +537,14 @@ impl BottomPane { if let Some(status) = &self.status { flex.push(0, RenderableItem::Borrowed(status)); } + if !self.unified_exec_footer.is_empty() { + flex.push(0, RenderableItem::Borrowed(&self.unified_exec_footer)); + } flex.push(1, RenderableItem::Borrowed(&self.queued_user_messages)); - if self.status.is_some() || !self.queued_user_messages.messages.is_empty() { + if self.status.is_some() + || !self.unified_exec_footer.is_empty() + || !self.queued_user_messages.messages.is_empty() + { flex.push(0, RenderableItem::Owned("".into())); } let mut flex2 = FlexRenderable::new(); diff --git a/codex-rs/tui/src/bottom_pane/selection_popup_common.rs b/codex-rs/tui/src/bottom_pane/selection_popup_common.rs index 5107ab0ca9..d44283aa14 100644 --- a/codex-rs/tui/src/bottom_pane/selection_popup_common.rs +++ b/codex-rs/tui/src/bottom_pane/selection_popup_common.rs @@ -15,11 +15,13 @@ use crate::key_hint::KeyBinding; use super::scroll_state::ScrollState; /// A generic representation of a display row for selection popups. +#[derive(Default)] pub(crate) struct GenericDisplayRow { pub name: String, pub display_shortcut: Option, pub match_indices: Option>, // indices to bold (char positions) pub description: Option, // optional grey text after the name + pub disabled_reason: Option, // optional disabled message pub wrap_indent: Option, // optional indent for wrapped lines } @@ -37,7 +39,13 @@ fn compute_desc_col( .iter() .enumerate() .filter(|(i, _)| visible_range.contains(i)) - .map(|(_, r)| Line::from(r.name.clone()).width()) + .map(|(_, r)| { + let mut spans: Vec = vec![r.name.clone().into()]; + if r.disabled_reason.is_some() { + spans.push(" (disabled)".dim()); + } + Line::from(spans).width() + }) .max() .unwrap_or(0); let mut desc_col = max_name_width.saturating_add(2); @@ -51,7 +59,7 @@ fn compute_desc_col( fn wrap_indent(row: &GenericDisplayRow, desc_col: usize, max_width: u16) -> usize { let max_indent = max_width.saturating_sub(1) as usize; let indent = row.wrap_indent.unwrap_or_else(|| { - if row.description.is_some() { + if row.description.is_some() || row.disabled_reason.is_some() { desc_col } else { 0 @@ -64,10 +72,16 @@ fn wrap_indent(row: &GenericDisplayRow, desc_col: usize, max_width: u16) -> usiz /// at `desc_col`. Applies fuzzy-match bolding when indices are present and /// dims the description. fn build_full_line(row: &GenericDisplayRow, desc_col: usize) -> Line<'static> { + let combined_description = match (&row.description, &row.disabled_reason) { + (Some(desc), Some(reason)) => Some(format!("{desc} (disabled: {reason})")), + (Some(desc), None) => Some(desc.clone()), + (None, Some(reason)) => Some(format!("disabled: {reason}")), + (None, None) => None, + }; + // Enforce single-line name: allow at most desc_col - 2 cells for name, // reserving two spaces before the description column. - let name_limit = row - .description + let name_limit = combined_description .as_ref() .map(|_| desc_col.saturating_sub(2)) .unwrap_or(usize::MAX); @@ -113,6 +127,10 @@ fn build_full_line(row: &GenericDisplayRow, desc_col: usize) -> Line<'static> { name_spans.push("…".into()); } + if row.disabled_reason.is_some() { + name_spans.push(" (disabled)".dim()); + } + let this_name_width = Line::from(name_spans.clone()).width(); let mut full_spans: Vec = name_spans; if let Some(display_shortcut) = row.display_shortcut { @@ -120,7 +138,7 @@ fn build_full_line(row: &GenericDisplayRow, desc_col: usize) -> Line<'static> { full_spans.push(display_shortcut.into()); full_spans.push(")".into()); } - if let Some(desc) = row.description.as_ref() { + if let Some(desc) = combined_description.as_ref() { let gap = desc_col.saturating_sub(this_name_width); if gap > 0 { full_spans.push(" ".repeat(gap).into()); diff --git a/codex-rs/tui/src/bottom_pane/skill_popup.rs b/codex-rs/tui/src/bottom_pane/skill_popup.rs index 3e0f79f84b..2e1e5878c6 100644 --- a/codex-rs/tui/src/bottom_pane/skill_popup.rs +++ b/codex-rs/tui/src/bottom_pane/skill_popup.rs @@ -92,6 +92,7 @@ impl SkillPopup { match_indices: indices, display_shortcut: None, description: Some(description), + disabled_reason: None, wrap_indent: None, } }) diff --git a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__unified_exec_footer__tests__render_more_sessions.snap b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__unified_exec_footer__tests__render_more_sessions.snap new file mode 100644 index 0000000000..90bfa7600a --- /dev/null +++ b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__unified_exec_footer__tests__render_more_sessions.snap @@ -0,0 +1,22 @@ +--- +source: tui/src/bottom_pane/unified_exec_footer.rs +expression: "format!(\"{buf:?}\")" +--- +Buffer { + area: Rect { x: 0, y: 0, width: 50, height: 2 }, + content: [ + "Background terminal running: echo hello · rg "foo"", + " src · 1 more running ", + ], + styles: [ + x: 0, y: 0, fg: Reset, bg: Reset, underline: Reset, modifier: DIM, + x: 28, y: 0, fg: Reset, bg: Reset, underline: Reset, modifier: NONE, + x: 29, y: 0, fg: Cyan, bg: Reset, underline: Reset, modifier: NONE, + x: 39, y: 0, fg: Reset, bg: Reset, underline: Reset, modifier: DIM, + x: 42, y: 0, fg: Cyan, bg: Reset, underline: Reset, modifier: NONE, + x: 0, y: 1, fg: Reset, bg: Reset, underline: Reset, modifier: NONE, + x: 29, y: 1, fg: Cyan, bg: Reset, underline: Reset, modifier: NONE, + x: 32, y: 1, fg: Reset, bg: Reset, underline: Reset, modifier: DIM, + x: 49, y: 1, fg: Reset, bg: Reset, underline: Reset, modifier: NONE, + ] +} diff --git a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__unified_exec_footer__tests__render_two_sessions.snap b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__unified_exec_footer__tests__render_two_sessions.snap new file mode 100644 index 0000000000..0828a62efa --- /dev/null +++ b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__unified_exec_footer__tests__render_two_sessions.snap @@ -0,0 +1,21 @@ +--- +source: tui/src/bottom_pane/unified_exec_footer.rs +expression: "format!(\"{buf:?}\")" +--- +Buffer { + area: Rect { x: 0, y: 0, width: 50, height: 2 }, + content: [ + "Background terminal running: echo hello · rg "foo"", + " src ", + ], + styles: [ + x: 0, y: 0, fg: Reset, bg: Reset, underline: Reset, modifier: DIM, + x: 28, y: 0, fg: Reset, bg: Reset, underline: Reset, modifier: NONE, + x: 29, y: 0, fg: Cyan, bg: Reset, underline: Reset, modifier: NONE, + x: 39, y: 0, fg: Reset, bg: Reset, underline: Reset, modifier: DIM, + x: 42, y: 0, fg: Cyan, bg: Reset, underline: Reset, modifier: NONE, + x: 0, y: 1, fg: Reset, bg: Reset, underline: Reset, modifier: NONE, + x: 29, y: 1, fg: Cyan, bg: Reset, underline: Reset, modifier: NONE, + x: 32, y: 1, fg: Reset, bg: Reset, underline: Reset, modifier: NONE, + ] +} diff --git a/codex-rs/tui/src/bottom_pane/unified_exec_footer.rs b/codex-rs/tui/src/bottom_pane/unified_exec_footer.rs new file mode 100644 index 0000000000..80ec1fb62b --- /dev/null +++ b/codex-rs/tui/src/bottom_pane/unified_exec_footer.rs @@ -0,0 +1,125 @@ +use ratatui::buffer::Buffer; +use ratatui::layout::Rect; +use ratatui::style::Stylize; +use ratatui::text::Line; +use ratatui::widgets::Paragraph; + +use crate::render::renderable::Renderable; +use crate::text_formatting::truncate_text; +use crate::wrapping::RtOptions; +use crate::wrapping::word_wrap_lines; + +const MAX_SESSION_LABEL_GRAPHEMES: usize = 48; +const MAX_VISIBLE_SESSIONS: usize = 2; + +pub(crate) struct UnifiedExecFooter { + sessions: Vec, +} + +impl UnifiedExecFooter { + pub(crate) fn new() -> Self { + Self { + sessions: Vec::new(), + } + } + + pub(crate) fn set_sessions(&mut self, sessions: Vec) -> bool { + if self.sessions == sessions { + return false; + } + self.sessions = sessions; + true + } + + pub(crate) fn is_empty(&self) -> bool { + self.sessions.is_empty() + } + + fn render_lines(&self, width: u16) -> Vec> { + if self.sessions.is_empty() || width < 4 { + return Vec::new(); + } + + let label = "Background terminal running:"; + let mut spans = Vec::new(); + spans.push(label.dim()); + spans.push(" ".into()); + + let visible = self.sessions.iter().take(MAX_VISIBLE_SESSIONS); + let mut visible_count = 0usize; + for (idx, command) in visible.enumerate() { + if idx > 0 { + spans.push(" · ".dim()); + } + let truncated = truncate_text(command, MAX_SESSION_LABEL_GRAPHEMES); + spans.push(truncated.cyan()); + visible_count += 1; + } + + let remaining = self.sessions.len().saturating_sub(visible_count); + if remaining > 0 { + spans.push(" · ".dim()); + spans.push(format!("{remaining} more running").dim()); + } + + let indent = " ".repeat(label.len() + 1); + let line = Line::from(spans); + word_wrap_lines( + std::iter::once(line), + RtOptions::new(width as usize).subsequent_indent(Line::from(indent).dim()), + ) + } +} + +impl Renderable for UnifiedExecFooter { + fn render(&self, area: Rect, buf: &mut Buffer) { + if area.is_empty() { + return; + } + + Paragraph::new(self.render_lines(area.width)).render(area, buf); + } + + fn desired_height(&self, width: u16) -> u16 { + self.render_lines(width).len() as u16 + } +} + +#[cfg(test)] +mod tests { + use super::*; + use insta::assert_snapshot; + use pretty_assertions::assert_eq; + + #[test] + fn desired_height_empty() { + let footer = UnifiedExecFooter::new(); + assert_eq!(footer.desired_height(40), 0); + } + + #[test] + fn render_two_sessions() { + let mut footer = UnifiedExecFooter::new(); + footer.set_sessions(vec!["echo hello".to_string(), "rg \"foo\" src".to_string()]); + let width = 50; + let height = footer.desired_height(width); + let mut buf = Buffer::empty(Rect::new(0, 0, width, height)); + footer.render(Rect::new(0, 0, width, height), &mut buf); + assert_snapshot!("render_two_sessions", format!("{buf:?}")); + } + + #[test] + fn render_more_sessions() { + let mut footer = UnifiedExecFooter::new(); + footer.set_sessions(vec![ + "echo hello".to_string(), + "rg \"foo\" src".to_string(), + "cat README.md".to_string(), + ]); + let width = 50; + let height = footer.desired_height(width); + let mut buf = Buffer::empty(Rect::new(0, 0, width, height)); + footer.render(Rect::new(0, 0, width, height), &mut buf); + assert_snapshot!("render_more_sessions", format!("{buf:?}")); + } +} diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index 37cd004a15..a7bf4e3fa2 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -9,6 +9,8 @@ use codex_app_server_protocol::AuthMode; use codex_backend_client::Client as BackendClient; use codex_core::config::Config; use codex_core::config::types::Notifications; +use codex_core::features::FEATURES; +use codex_core::features::Feature; use codex_core::git_info::current_branch_name; use codex_core::git_info::local_git_branches; use codex_core::openai_models::model_family::ModelFamily; @@ -85,9 +87,11 @@ use tracing::debug; use crate::app_event::AppEvent; use crate::app_event_sender::AppEventSender; use crate::bottom_pane::ApprovalRequest; +use crate::bottom_pane::BetaFeatureItem; use crate::bottom_pane::BottomPane; use crate::bottom_pane::BottomPaneParams; use crate::bottom_pane::CancellationEvent; +use crate::bottom_pane::ExperimentalFeaturesView; use crate::bottom_pane::InputResult; use crate::bottom_pane::SelectionAction; use crate::bottom_pane::SelectionItem; @@ -99,6 +103,7 @@ use crate::diff_render::display_path_for; use crate::exec_cell::CommandOutput; use crate::exec_cell::ExecCell; use crate::exec_cell::new_active_exec_command; +use crate::exec_command::strip_bash_lc_and_escape; use crate::get_git_diff::get_git_diff; use crate::history_cell; use crate::history_cell::AgentMessageCell; @@ -149,6 +154,11 @@ struct RunningCommand { source: ExecCommandSource, } +struct UnifiedExecSessionSummary { + key: String, + command_display: String, +} + struct UnifiedExecWaitState { command_display: String, } @@ -163,6 +173,20 @@ impl UnifiedExecWaitState { } } +fn is_unified_exec_source(source: ExecCommandSource) -> bool { + matches!( + source, + ExecCommandSource::UnifiedExecStartup | ExecCommandSource::UnifiedExecInteraction + ) +} + +fn is_standard_tool_call(parsed_cmd: &[ParsedCommand]) -> bool { + !parsed_cmd.is_empty() + && parsed_cmd + .iter() + .all(|parsed| !matches!(parsed, ParsedCommand::Unknown { .. })) +} + const RATE_LIMIT_WARNING_THRESHOLDS: [f64; 3] = [75.0, 90.0, 95.0]; const NUDGE_MODEL_SLUG: &str = "gpt-5.1-codex-mini"; const RATE_LIMIT_SWITCH_PROMPT_THRESHOLD: f64 = 90.0; @@ -300,6 +324,7 @@ pub(crate) struct ChatWidget { suppressed_exec_calls: HashSet, last_unified_wait: Option, task_complete_pending: bool, + unified_exec_sessions: Vec, mcp_startup_status: Option>, // Queue of interruptive UI events deferred during an active write cycle interrupts: InterruptManager, @@ -410,7 +435,10 @@ impl ChatWidget { } // Ask codex-core to enumerate custom prompts for this session. self.submit_op(Op::ListCustomPrompts); - self.submit_op(Op::ListSkills { cwds: Vec::new() }); + self.submit_op(Op::ListSkills { + cwds: Vec::new(), + force_reload: false, + }); if let Some(user_message) = self.initial_user_message.take() { self.submit_user_message(user_message); } @@ -828,6 +856,12 @@ impl ChatWidget { fn on_exec_command_begin(&mut self, ev: ExecCommandBeginEvent) { self.flush_answer_stream_with_separator(); + if is_unified_exec_source(ev.source) { + self.track_unified_exec_session_begin(&ev); + if !is_standard_tool_call(&ev.parsed_cmd) { + return; + } + } let ev2 = ev.clone(); self.defer_or_handle(|q| q.push_exec_begin(ev), |s| s.handle_exec_begin_now(ev2)); } @@ -839,8 +873,17 @@ impl ChatWidget { // TODO: Handle streaming exec output if/when implemented } - fn on_terminal_interaction(&mut self, _ev: TerminalInteractionEvent) { - // TODO: Handle once design is ready + fn on_terminal_interaction(&mut self, ev: TerminalInteractionEvent) { + self.flush_answer_stream_with_separator(); + let command_display = self + .unified_exec_sessions + .iter() + .find(|session| session.key == ev.process_id) + .map(|session| session.command_display.clone()); + self.add_to_history(history_cell::new_unified_exec_interaction( + command_display, + ev.stdin, + )); } fn on_patch_apply_begin(&mut self, event: PatchApplyBeginEvent) { @@ -868,10 +911,56 @@ impl ChatWidget { } fn on_exec_command_end(&mut self, ev: ExecCommandEndEvent) { + if is_unified_exec_source(ev.source) { + self.track_unified_exec_session_end(&ev); + if !self.bottom_pane.is_task_running() { + return; + } + } let ev2 = ev.clone(); self.defer_or_handle(|q| q.push_exec_end(ev), |s| s.handle_exec_end_now(ev2)); } + fn track_unified_exec_session_begin(&mut self, ev: &ExecCommandBeginEvent) { + if ev.source != ExecCommandSource::UnifiedExecStartup { + return; + } + let key = ev.process_id.clone().unwrap_or(ev.call_id.to_string()); + let command_display = strip_bash_lc_and_escape(&ev.command); + if let Some(existing) = self + .unified_exec_sessions + .iter_mut() + .find(|session| session.key == key) + { + existing.command_display = command_display; + } else { + self.unified_exec_sessions.push(UnifiedExecSessionSummary { + key, + command_display, + }); + } + self.sync_unified_exec_footer(); + } + + fn track_unified_exec_session_end(&mut self, ev: &ExecCommandEndEvent) { + let key = ev.process_id.clone().unwrap_or(ev.call_id.to_string()); + let before = self.unified_exec_sessions.len(); + self.unified_exec_sessions + .retain(|session| session.key != key); + if self.unified_exec_sessions.len() != before { + self.sync_unified_exec_footer(); + } + } + + fn sync_unified_exec_footer(&mut self) { + let sessions = self + .unified_exec_sessions + .iter() + .map(|session| session.command_display.clone()) + .collect(); + self.bottom_pane.set_unified_exec_sessions(sessions); + } + fn on_mcp_tool_call_begin(&mut self, ev: McpToolCallBeginEvent) { let ev2 = ev.clone(); self.defer_or_handle(|q| q.push_mcp_begin(ev), |s| s.handle_mcp_begin_now(ev2)); @@ -1319,6 +1408,7 @@ impl ChatWidget { suppressed_exec_calls: HashSet::new(), last_unified_wait: None, task_complete_pending: false, + unified_exec_sessions: Vec::new(), mcp_startup_status: None, interrupts: InterruptManager::new(), reasoning_buffer: String::new(), @@ -1404,6 +1494,7 @@ impl ChatWidget { suppressed_exec_calls: HashSet::new(), last_unified_wait: None, task_complete_pending: false, + unified_exec_sessions: Vec::new(), mcp_startup_status: None, interrupts: InterruptManager::new(), reasoning_buffer: String::new(), @@ -1568,6 +1659,9 @@ impl ChatWidget { SlashCommand::Approvals => { self.open_approvals_popup(); } + SlashCommand::Experimental => { + self.open_experimental_popup(); + } SlashCommand::Quit | SlashCommand::Exit => { self.request_exit(); } @@ -1886,6 +1980,12 @@ impl ChatWidget { EventMsg::McpListToolsResponse(ev) => self.on_list_mcp_tools(ev), EventMsg::ListCustomPromptsResponse(ev) => self.on_list_custom_prompts(ev), EventMsg::ListSkillsResponse(ev) => self.on_list_skills(ev), + EventMsg::SkillsUpdateAvailable => { + self.submit_op(Op::ListSkills { + cwds: Vec::new(), + force_reload: true, + }); + } EventMsg::ShutdownComplete => self.on_shutdown_complete(), EventMsg::TurnDiff(TurnDiffEvent { unified_diff }) => self.on_turn_diff(unified_diff), EventMsg::DeprecationNotice(ev) => self.on_deprecation_notice(ev), @@ -1952,15 +2052,8 @@ impl ChatWidget { self.app_event_tx .send(AppEvent::InsertHistoryCell(Box::new(body_cell))); } - } else { - let message_text = - codex_core::review_format::format_review_findings_block(&output.findings, None); - let mut message_lines: Vec> = Vec::new(); - append_markdown(&message_text, None, &mut message_lines); - let body_cell = AgentMessageCell::new(message_lines, true); - self.app_event_tx - .send(AppEvent::InsertHistoryCell(Box::new(body_cell))); } + // Final message is rendered as part of the AgentMessage. } self.is_review_mode = false; @@ -2554,7 +2647,7 @@ impl ChatWidget { /// Open a popup to choose the approvals mode (ask for approval policy + sandbox policy). pub(crate) fn open_approvals_popup(&mut self) { - let current_approval = self.config.approval_policy; + let current_approval = self.config.approval_policy.value(); let current_sandbox = self.config.sandbox_policy.clone(); let mut items: Vec = Vec::new(); let presets: Vec = builtin_approval_presets(); @@ -2562,8 +2655,11 @@ impl ChatWidget { let is_current = Self::preset_matches_current(current_approval, ¤t_sandbox, &preset); let name = preset.label.to_string(); - let description_text = preset.description; - let description = Some(description_text.to_string()); + let description = Some(preset.description.to_string()); + let disabled_reason = match self.config.approval_policy.can_set(&preset.approval) { + Ok(()) => None, + Err(err) => Some(err.to_string()), + }; let requires_confirmation = preset.id == "full-access" && !self .config @@ -2616,6 +2712,7 @@ impl ChatWidget { is_current, actions, dismiss_on_select: true, + disabled_reason, ..Default::default() }); } @@ -2629,6 +2726,24 @@ impl ChatWidget { }); } + pub(crate) fn open_experimental_popup(&mut self) { + let features: Vec = FEATURES + .iter() + .filter_map(|spec| { + let description = spec.stage.beta_menu_description()?; + Some(BetaFeatureItem { + feature: spec.id, + name: feature_label_from_key(spec.key), + description: description.to_string(), + enabled: self.config.features.enabled(spec.id), + }) + }) + .collect(); + + let view = ExperimentalFeaturesView::new(features, self.app_event_tx.clone()); + self.bottom_pane.show_view(Box::new(view)); + } + fn approval_preset_actions( approval: AskForApproval, sandbox: SandboxPolicy, @@ -2952,7 +3067,9 @@ impl ChatWidget { /// Set the approval policy in the widget's config copy. pub(crate) fn set_approval_policy(&mut self, policy: AskForApproval) { - self.config.approval_policy = policy; + if let Err(err) = self.config.approval_policy.set(policy) { + tracing::warn!(%err, "failed to set approval_policy on chat config"); + } } /// Set the sandbox policy in the widget's config copy. @@ -2969,6 +3086,14 @@ impl ChatWidget { } } + pub(crate) fn set_feature_enabled(&mut self, feature: Feature, enabled: bool) { + if enabled { + self.config.features.enable(feature); + } else { + self.config.features.disable(feature); + } + } + pub(crate) fn set_full_access_warning_acknowledged(&mut self, acknowledged: bool) { self.config.notices.hide_full_access_warning = Some(acknowledged); } @@ -3302,6 +3427,23 @@ impl ChatWidget { } } +fn feature_label_from_key(key: &str) -> String { + let mut out = String::with_capacity(key.len()); + let mut capitalize = true; + for ch in key.chars() { + if ch == '_' || ch == '-' { + out.push(' '); + capitalize = true; + } else if capitalize { + out.push(ch.to_ascii_uppercase()); + capitalize = false; + } else { + out.push(ch); + } + } + out +} + impl Drop for ChatWidget { fn drop(&mut self) { self.stop_rate_limit_poller(); diff --git a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__experimental_features_popup.snap b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__experimental_features_popup.snap new file mode 100644 index 0000000000..c0de5e4eef --- /dev/null +++ b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__experimental_features_popup.snap @@ -0,0 +1,11 @@ +--- +source: tui/src/chatwidget/tests.rs +expression: popup +--- + Experimental features + Toggle beta features. Changes are saved to config.toml. + +› [ ] Ghost snapshots Capture undo snapshots each turn. + [x] Shell tool Allow the model to run shell commands. + + Press enter to toggle or esc to save for next conversation diff --git a/codex-rs/tui/src/chatwidget/tests.rs b/codex-rs/tui/src/chatwidget/tests.rs index 362b1678f4..07b2be0405 100644 --- a/codex-rs/tui/src/chatwidget/tests.rs +++ b/codex-rs/tui/src/chatwidget/tests.rs @@ -10,6 +10,8 @@ use codex_core::CodexAuth; use codex_core::config::Config; use codex_core::config::ConfigOverrides; use codex_core::config::ConfigToml; +use codex_core::config::Constrained; +use codex_core::config::ConstraintError; use codex_core::openai_models::models_manager::ModelsManager; use codex_core::protocol::AgentMessageDeltaEvent; use codex_core::protocol::AgentMessageEvent; @@ -33,10 +35,6 @@ use codex_core::protocol::Op; use codex_core::protocol::PatchApplyBeginEvent; use codex_core::protocol::PatchApplyEndEvent; use codex_core::protocol::RateLimitWindow; -use codex_core::protocol::ReviewCodeLocation; -use codex_core::protocol::ReviewFinding; -use codex_core::protocol::ReviewLineRange; -use codex_core::protocol::ReviewOutputEvent; use codex_core::protocol::ReviewRequest; use codex_core::protocol::ReviewTarget; use codex_core::protocol::StreamErrorEvent; @@ -195,41 +193,6 @@ fn entered_review_mode_defaults_to_current_changes_banner() { assert!(chat.is_review_mode); } -/// Completing review with findings shows the selection popup and finishes with -/// the closing banner while clearing review mode state. -#[test] -fn exited_review_mode_emits_results_and_finishes() { - let (mut chat, mut rx, _ops) = make_chatwidget_manual(None); - - let review = ReviewOutputEvent { - findings: vec![ReviewFinding { - title: "[P1] Fix bug".to_string(), - body: "Something went wrong".to_string(), - confidence_score: 0.9, - priority: 1, - code_location: ReviewCodeLocation { - absolute_file_path: PathBuf::from("src/lib.rs"), - line_range: ReviewLineRange { start: 10, end: 12 }, - }, - }], - overall_correctness: "needs work".to_string(), - overall_explanation: "Investigate the failure".to_string(), - overall_confidence_score: 0.5, - }; - - chat.handle_codex_event(Event { - id: "review-end".into(), - msg: EventMsg::ExitedReviewMode(ExitedReviewModeEvent { - review_output: Some(review), - }), - }); - - let cells = drain_insert_history(&mut rx); - let banner = lines_to_single_string(cells.last().expect("finished banner")); - assert_eq!(banner, "\n<< Code review finished >>\n"); - assert!(!chat.is_review_mode); -} - /// Exiting review restores the pre-review context window indicator. #[test] fn review_restores_context_window_indicator() { @@ -426,6 +389,7 @@ fn make_chatwidget_manual( suppressed_exec_calls: HashSet::new(), last_unified_wait: None, task_complete_pending: false, + unified_exec_sessions: Vec::new(), mcp_startup_status: None, interrupts: InterruptManager::new(), reasoning_buffer: String::new(), @@ -1221,6 +1185,7 @@ fn exec_end_without_begin_uses_event_command() { #[test] fn exec_history_shows_unified_exec_startup_commands() { let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(None); + chat.on_task_started(); let begin = begin_exec_with_source( &mut chat, @@ -1244,6 +1209,46 @@ fn exec_history_shows_unified_exec_startup_commands() { ); } +#[test] +fn exec_history_shows_unified_exec_tool_calls() { + let (mut chat, _rx, _op_rx) = make_chatwidget_manual(None); + chat.on_task_started(); + + let begin = begin_exec_with_source( + &mut chat, + "call-startup", + "ls", + ExecCommandSource::UnifiedExecStartup, + ); + end_exec(&mut chat, begin, "", "", 0); + + let blob = active_blob(&chat); + assert_eq!(blob, "• Explored\n └ List ls\n"); +} + +#[test] +fn unified_exec_end_after_task_complete_is_suppressed() { + let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(None); + chat.on_task_started(); + + let begin = begin_exec_with_source( + &mut chat, + "call-startup", + "echo unified exec startup", + ExecCommandSource::UnifiedExecStartup, + ); + drain_insert_history(&mut rx); + + chat.on_task_complete(None); + end_exec(&mut chat, begin, "", "", 0); + + let cells = drain_insert_history(&mut rx); + assert!( + cells.is_empty(), + "expected unified exec end after task complete to be suppressed" + ); +} + /// Selecting the custom prompt option from the review popup sends /// OpenReviewCustomPrompt to the app event channel. #[test] @@ -1775,6 +1780,71 @@ fn render_bottom_popup(chat: &ChatWidget, width: u16) -> String { lines.join("\n") } +#[test] +fn experimental_features_popup_snapshot() { + let (mut chat, _rx, _op_rx) = make_chatwidget_manual(None); + + let features = vec![ + BetaFeatureItem { + feature: Feature::GhostCommit, + name: "Ghost snapshots".to_string(), + description: "Capture undo snapshots each turn.".to_string(), + enabled: false, + }, + BetaFeatureItem { + feature: Feature::ShellTool, + name: "Shell tool".to_string(), + description: "Allow the model to run shell commands.".to_string(), + enabled: true, + }, + ]; + let view = ExperimentalFeaturesView::new(features, chat.app_event_tx.clone()); + chat.bottom_pane.show_view(Box::new(view)); + + let popup = render_bottom_popup(&chat, 80); + assert_snapshot!("experimental_features_popup", popup); +} + +#[test] +fn experimental_features_toggle_saves_on_exit() { + let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(None); + + let expected_feature = Feature::GhostCommit; + let view = ExperimentalFeaturesView::new( + vec![BetaFeatureItem { + feature: expected_feature, + name: "Ghost snapshots".to_string(), + description: "Capture undo snapshots each turn.".to_string(), + enabled: false, + }], + chat.app_event_tx.clone(), + ); + chat.bottom_pane.show_view(Box::new(view)); + + chat.handle_key_event(KeyEvent::new(KeyCode::Enter, KeyModifiers::NONE)); + + assert!( + rx.try_recv().is_err(), + "expected no updates until exiting the popup" + ); + + chat.handle_key_event(KeyEvent::new(KeyCode::Esc, KeyModifiers::NONE)); + + let mut updates = None; + while let Ok(event) = rx.try_recv() { + if let AppEvent::UpdateFeatureFlags { + updates: event_updates, + } = event + { + updates = Some(event_updates); + break; + } + } + + let updates = updates.expect("expected UpdateFeatureFlags event"); + assert_eq!(updates, vec![(expected_feature, true)]); +} + #[test] fn model_selection_popup_snapshot() { let (mut chat, _rx, _op_rx) = make_chatwidget_manual(Some("gpt-5-codex")); @@ -2078,17 +2148,125 @@ fn disabled_slash_command_while_task_running_snapshot() { assert_snapshot!(blob); } +#[test] +fn approvals_popup_shows_disabled_presets() { + let (mut chat, _rx, _op_rx) = make_chatwidget_manual(None); + + chat.config.approval_policy = + Constrained::new(AskForApproval::OnRequest, |candidate| match candidate { + AskForApproval::OnRequest => Ok(()), + _ => Err(ConstraintError { + message: "this message should be printed in the description".to_string(), + }), + }) + .expect("construct constrained approval policy"); + + chat.open_approvals_popup(); + + let width = 80; + let height = chat.desired_height(width); + let mut terminal = + ratatui::Terminal::new(VT100Backend::new(width, height)).expect("create terminal"); + terminal.set_viewport_area(Rect::new(0, 0, width, height)); + terminal + .draw(|f| chat.render(f.area(), f.buffer_mut())) + .expect("render approvals popup"); + + let screen = terminal.backend().vt100().screen().contents(); + let collapsed = screen.split_whitespace().collect::>().join(" "); + assert!( + collapsed.contains("(disabled)"), + "disabled preset label should be shown" + ); + assert!( + collapsed.contains("this message should be printed in the description"), + "disabled preset reason should be shown" + ); +} + +#[test] +fn approvals_popup_navigation_skips_disabled() { + let (mut chat, mut rx, mut op_rx) = make_chatwidget_manual(None); + + chat.config.approval_policy = + Constrained::new(AskForApproval::OnRequest, |candidate| match candidate { + AskForApproval::OnRequest => Ok(()), + _ => Err(ConstraintError { + message: "disabled preset".to_string(), + }), + }) + .expect("construct constrained approval policy"); + + chat.open_approvals_popup(); + + // The approvals popup is the active bottom-pane view; drive navigation via chat handle_key_event. + // Start selected at idx 0 (enabled), move down twice; the disabled option should be skipped + // and selection should wrap back to idx 0 (also enabled). + chat.handle_key_event(KeyEvent::from(KeyCode::Down)); + chat.handle_key_event(KeyEvent::from(KeyCode::Down)); + + // Press numeric shortcut for the disabled row (3 => idx 2); should not close or accept. + chat.handle_key_event(KeyEvent::from(KeyCode::Char('3'))); + + // Ensure the popup remains open and no selection actions were sent. + let width = 80; + let height = chat.desired_height(width); + let mut terminal = + ratatui::Terminal::new(VT100Backend::new(width, height)).expect("create terminal"); + terminal.set_viewport_area(Rect::new(0, 0, width, height)); + terminal + .draw(|f| chat.render(f.area(), f.buffer_mut())) + .expect("render approvals popup after disabled selection"); + let screen = terminal.backend().vt100().screen().contents(); + assert!( + screen.contains("Select Approval Mode"), + "popup should remain open after selecting a disabled entry" + ); + assert!( + op_rx.try_recv().is_err(), + "no actions should be dispatched yet" + ); + assert!(rx.try_recv().is_err(), "no history should be emitted"); + + // Press Enter; selection should land on an enabled preset and dispatch updates. + chat.handle_key_event(KeyEvent::from(KeyCode::Enter)); + let mut app_events = Vec::new(); + while let Ok(ev) = rx.try_recv() { + app_events.push(ev); + } + assert!( + app_events.iter().any(|ev| matches!( + ev, + AppEvent::CodexOp(Op::OverrideTurnContext { + approval_policy: Some(AskForApproval::OnRequest), + .. + }) + )), + "enter should select an enabled preset" + ); + assert!( + !app_events.iter().any(|ev| matches!( + ev, + AppEvent::CodexOp(Op::OverrideTurnContext { + approval_policy: Some(AskForApproval::Never), + .. + }) + )), + "disabled preset should not be selected" + ); +} + // // Snapshot test: command approval modal // // Synthesizes a Codex ExecApprovalRequest event to trigger the approval modal // and snapshots the visual output using the ratatui TestBackend. #[test] -fn approval_modal_exec_snapshot() { +fn approval_modal_exec_snapshot() -> anyhow::Result<()> { // Build a chat widget with manual channels to avoid spawning the agent. let (mut chat, _rx, _op_rx) = make_chatwidget_manual(None); // Ensure policy allows surfacing approvals explicitly (not strictly required for direct event). - chat.config.approval_policy = AskForApproval::OnRequest; + chat.config.approval_policy.set(AskForApproval::OnRequest)?; // Inject an exec approval request to display the approval modal. let ev = ExecApprovalRequestEvent { call_id: "call-approve-cmd".into(), @@ -2134,14 +2312,16 @@ fn approval_modal_exec_snapshot() { "approval_modal_exec", terminal.backend().vt100().screen().contents() ); + + Ok(()) } // Snapshot test: command approval modal without a reason // Ensures spacing looks correct when no reason text is provided. #[test] -fn approval_modal_exec_without_reason_snapshot() { +fn approval_modal_exec_without_reason_snapshot() -> anyhow::Result<()> { let (mut chat, _rx, _op_rx) = make_chatwidget_manual(None); - chat.config.approval_policy = AskForApproval::OnRequest; + chat.config.approval_policy.set(AskForApproval::OnRequest)?; let ev = ExecApprovalRequestEvent { call_id: "call-approve-cmd-noreason".into(), @@ -2173,13 +2353,15 @@ fn approval_modal_exec_without_reason_snapshot() { "approval_modal_exec_no_reason", terminal.backend().vt100().screen().contents() ); + + Ok(()) } // Snapshot test: patch approval modal #[test] -fn approval_modal_patch_snapshot() { +fn approval_modal_patch_snapshot() -> anyhow::Result<()> { let (mut chat, _rx, _op_rx) = make_chatwidget_manual(None); - chat.config.approval_policy = AskForApproval::OnRequest; + chat.config.approval_policy.set(AskForApproval::OnRequest)?; // Build a small changeset and a reason/grant_root to exercise the prompt text. let mut changes = HashMap::new(); @@ -2213,6 +2395,8 @@ fn approval_modal_patch_snapshot() { "approval_modal_patch", terminal.backend().vt100().screen().contents() ); + + Ok(()) } #[test] @@ -2775,10 +2959,10 @@ fn apply_patch_full_flow_integration_like() { } #[test] -fn apply_patch_untrusted_shows_approval_modal() { +fn apply_patch_untrusted_shows_approval_modal() -> anyhow::Result<()> { let (mut chat, _rx, _op_rx) = make_chatwidget_manual(None); // Ensure approval policy is untrusted (OnRequest) - chat.config.approval_policy = AskForApproval::OnRequest; + chat.config.approval_policy.set(AskForApproval::OnRequest)?; // Simulate a patch approval request from backend let mut changes = HashMap::new(); @@ -2817,14 +3001,16 @@ fn apply_patch_untrusted_shows_approval_modal() { contains_title, "expected approval modal to be visible with title 'Would you like to make the following edits?'" ); + + Ok(()) } #[test] -fn apply_patch_request_shows_diff_summary() { +fn apply_patch_request_shows_diff_summary() -> anyhow::Result<()> { let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(None); // Ensure we are in OnRequest so an approval is surfaced - chat.config.approval_policy = AskForApproval::OnRequest; + chat.config.approval_policy.set(AskForApproval::OnRequest)?; // Simulate backend asking to apply a patch adding two lines to README.md let mut changes = HashMap::new(); @@ -2883,6 +3069,8 @@ fn apply_patch_request_shows_diff_summary() { saw_line1 && saw_line2, "expected modal to show per-line diff summary" ); + + Ok(()) } #[test] diff --git a/codex-rs/tui/src/history_cell.rs b/codex-rs/tui/src/history_cell.rs index 5440040f6b..2c0a37ecea 100644 --- a/codex-rs/tui/src/history_cell.rs +++ b/codex-rs/tui/src/history_cell.rs @@ -375,6 +375,72 @@ impl HistoryCell for PrefixedWrappedHistoryCell { } } +#[derive(Debug)] +pub(crate) struct UnifiedExecInteractionCell { + command_display: Option, + stdin: String, +} + +impl UnifiedExecInteractionCell { + pub(crate) fn new(command_display: Option, stdin: String) -> Self { + Self { + command_display, + stdin, + } + } +} + +impl HistoryCell for UnifiedExecInteractionCell { + fn display_lines(&self, width: u16) -> Vec> { + if width == 0 { + return Vec::new(); + } + let wrap_width = width as usize; + + let mut header_spans = vec!["↳ ".dim(), "Interacted with background terminal".bold()]; + if let Some(command) = &self.command_display + && !command.is_empty() + { + header_spans.push(" · ".dim()); + header_spans.push(command.clone().dim()); + } + let header = Line::from(header_spans); + + let mut out: Vec> = Vec::new(); + let header_wrapped = word_wrap_line(&header, RtOptions::new(wrap_width)); + push_owned_lines(&header_wrapped, &mut out); + + let input_lines: Vec> = if self.stdin.is_empty() { + vec![vec!["(waited)".dim()].into()] + } else { + self.stdin + .lines() + .map(|line| Line::from(line.to_string())) + .collect() + }; + + let input_wrapped = word_wrap_lines( + input_lines, + RtOptions::new(wrap_width) + .initial_indent(Line::from(" └ ".dim())) + .subsequent_indent(Line::from(" ".dim())), + ); + out.extend(input_wrapped); + out + } + + fn desired_height(&self, width: u16) -> u16 { + self.display_lines(width).len() as u16 + } +} + +pub(crate) fn new_unified_exec_interaction( + command_display: Option, + stdin: String, +) -> UnifiedExecInteractionCell { + UnifiedExecInteractionCell::new(command_display, stdin) +} + fn truncate_exec_snippet(full_cmd: &str) -> String { let mut snippet = match full_cmd.split_once('\n') { Some((first, _)) => format!("{first} ..."), @@ -1558,6 +1624,31 @@ mod tests { render_lines(&cell.transcript_lines(u16::MAX)) } + #[test] + fn unified_exec_interaction_cell_renders_input() { + let cell = + new_unified_exec_interaction(Some("echo hello".to_string()), "ls\npwd".to_string()); + let lines = render_transcript(&cell); + assert_eq!( + lines, + vec![ + "↳ Interacted with background terminal · echo hello", + " └ ls", + " pwd", + ], + ); + } + + #[test] + fn unified_exec_interaction_cell_renders_wait() { + let cell = new_unified_exec_interaction(None, String::new()); + let lines = render_transcript(&cell); + assert_eq!( + lines, + vec!["↳ Interacted with background terminal", " └ (waited)"], + ); + } + #[test] fn mcp_tools_output_masks_sensitive_values() { let mut config = test_config(); diff --git a/codex-rs/tui/src/resume_picker.rs b/codex-rs/tui/src/resume_picker.rs index f2c6a3269d..7f3665d563 100644 --- a/codex-rs/tui/src/resume_picker.rs +++ b/codex-rs/tui/src/resume_picker.rs @@ -10,6 +10,7 @@ use codex_core::ConversationsPage; use codex_core::Cursor; use codex_core::INTERACTIVE_SESSION_SOURCES; use codex_core::RolloutRecorder; +use codex_core::path_utils; use codex_protocol::items::TurnItem; use color_eyre::eyre::Result; use crossterm::event::KeyCode; @@ -670,7 +671,10 @@ fn extract_session_meta_from_head(head: &[serde_json::Value]) -> (Option bool { - if let (Ok(ca), Ok(cb)) = (a.canonicalize(), b.canonicalize()) { + if let (Ok(ca), Ok(cb)) = ( + path_utils::normalize_for_path_comparison(a), + path_utils::normalize_for_path_comparison(b), + ) { return ca == cb; } a == b diff --git a/codex-rs/tui/src/skill_error_prompt.rs b/codex-rs/tui/src/skill_error_prompt.rs index 33d3b5dce1..9a9f803adb 100644 --- a/codex-rs/tui/src/skill_error_prompt.rs +++ b/codex-rs/tui/src/skill_error_prompt.rs @@ -1,6 +1,8 @@ use crate::tui::FrameRequester; use crate::tui::Tui; use crate::tui::TuiEvent; +use crate::wrapping::RtOptions; +use crate::wrapping::word_wrap_line; use codex_core::skills::SkillError; use crossterm::event::KeyCode; use crossterm::event::KeyEvent; @@ -16,7 +18,6 @@ use ratatui::widgets::Clear; use ratatui::widgets::Paragraph; use ratatui::widgets::Widget; use ratatui::widgets::WidgetRef; -use ratatui::widgets::Wrap; use tokio_stream::StreamExt; #[derive(Clone, Copy, Debug, PartialEq, Eq)] @@ -31,16 +32,24 @@ pub(crate) async fn run_skill_error_prompt( ) -> SkillErrorPromptOutcome { struct AltScreenGuard<'a> { tui: &'a mut Tui, + stashed_history_lines: Vec>, } impl<'a> AltScreenGuard<'a> { fn enter(tui: &'a mut Tui) -> Self { let _ = tui.enter_alt_screen(); - Self { tui } + let stashed_history_lines = tui.stash_pending_history_lines(); + Self { + tui, + stashed_history_lines, + } } } impl Drop for AltScreenGuard<'_> { fn drop(&mut self) { let _ = self.tui.leave_alt_screen(); + let stashed_history_lines = std::mem::take(&mut self.stashed_history_lines); + self.tui + .restore_pending_history_lines(stashed_history_lines); } } @@ -76,28 +85,16 @@ pub(crate) async fn run_skill_error_prompt( struct SkillErrorScreen { request_frame: FrameRequester, - lines: Vec>, + errors: Vec, done: bool, exit: bool, } impl SkillErrorScreen { fn new(request_frame: FrameRequester, errors: &[SkillError]) -> Self { - let mut lines: Vec> = Vec::new(); - lines.push(Line::from("Skill validation errors detected".bold())); - lines.push(Line::from( - "Fix these SKILL.md files and restart. Invalid skills are ignored until resolved. Press enter or esc to continue, Ctrl+C or Ctrl+D to exit.", - )); - lines.push(Line::from("")); - - for error in errors { - let message = format!("- {}: {}", error.path.display(), error.message); - lines.push(Line::from(message)); - } - Self { request_frame, - lines, + errors: errors.to_vec(), done: false, exit: false, } @@ -153,12 +150,44 @@ impl SkillErrorScreen { impl WidgetRef for &SkillErrorScreen { fn render_ref(&self, area: Rect, buf: &mut Buffer) { Clear.render(area, buf); + let block = Block::default() .title("Skill errors".bold()) .borders(Borders::ALL); - Paragraph::new(self.lines.clone()) - .block(block) - .wrap(Wrap { trim: true }) - .render(area, buf); + + let inner = block.inner(area); + let width = usize::from(inner.width).max(1); + + let mut base_lines: Vec> = vec![ + Line::from("Skill validation errors detected".bold()), + Line::from("Fix these SKILL.md files and restart."), + Line::from("Invalid skills are ignored until resolved."), + Line::from("Press enter or esc to continue. Ctrl+C or Ctrl+D to exit."), + Line::from(""), + ]; + + let error_start = base_lines.len(); + for error in &self.errors { + base_lines.push(Line::from(vec![ + error.path.display().to_string().dim(), + ": ".into(), + error.message.clone().red(), + ])); + } + + let error_wrap_opts = RtOptions::new(width) + .initial_indent(Line::from("- ")) + .subsequent_indent(Line::from(" ")); + + let mut lines: Vec> = Vec::new(); + for (idx, line) in base_lines.iter().enumerate() { + if idx < error_start { + lines.extend(word_wrap_line(line, width)); + } else { + lines.extend(word_wrap_line(line, error_wrap_opts.clone())); + } + } + + Paragraph::new(lines).block(block).render(area, buf); } } diff --git a/codex-rs/tui/src/slash_command.rs b/codex-rs/tui/src/slash_command.rs index e0c676812c..bfc5616e26 100644 --- a/codex-rs/tui/src/slash_command.rs +++ b/codex-rs/tui/src/slash_command.rs @@ -14,6 +14,7 @@ pub enum SlashCommand { // more frequently used commands should be listed first. Model, Approvals, + Experimental, Skills, Review, New, @@ -51,6 +52,7 @@ impl SlashCommand { SlashCommand::Status => "show current session configuration and token usage", SlashCommand::Model => "choose what model and reasoning effort to use", SlashCommand::Approvals => "choose what Codex can do without approval", + SlashCommand::Experimental => "toggle beta features", SlashCommand::Mcp => "list configured MCP tools", SlashCommand::Logout => "log out of Codex", SlashCommand::Rollout => "print the rollout file path", @@ -74,6 +76,7 @@ impl SlashCommand { | SlashCommand::Undo | SlashCommand::Model | SlashCommand::Approvals + | SlashCommand::Experimental | SlashCommand::Review | SlashCommand::Logout => false, SlashCommand::Diff diff --git a/codex-rs/tui/src/test_backend.rs b/codex-rs/tui/src/test_backend.rs index a5460af2e0..6e340fa201 100644 --- a/codex-rs/tui/src/test_backend.rs +++ b/codex-rs/tui/src/test_backend.rs @@ -25,6 +25,7 @@ pub struct VT100Backend { impl VT100Backend { /// Creates a new `TestBackend` with the specified width and height. pub fn new(width: u16, height: u16) -> Self { + crossterm::style::force_color_output(true); Self { crossterm_backend: CrosstermBackend::new(vt100::Parser::new(height, width, 0)), } diff --git a/codex-rs/tui/src/tooltips.rs b/codex-rs/tui/src/tooltips.rs index eb419c2ea3..8a3a1c1e96 100644 --- a/codex-rs/tui/src/tooltips.rs +++ b/codex-rs/tui/src/tooltips.rs @@ -1,14 +1,28 @@ +use codex_core::features::FEATURES; use lazy_static::lazy_static; use rand::Rng; const RAW_TOOLTIPS: &str = include_str!("../tooltips.txt"); +fn beta_tooltips() -> Vec<&'static str> { + FEATURES + .iter() + .filter_map(|spec| spec.stage.beta_announcement()) + .collect() +} + lazy_static! { static ref TOOLTIPS: Vec<&'static str> = RAW_TOOLTIPS .lines() .map(str::trim) .filter(|line| !line.is_empty() && !line.starts_with('#')) .collect(); + static ref ALL_TOOLTIPS: Vec<&'static str> = { + let mut tips = Vec::new(); + tips.extend(TOOLTIPS.iter().copied()); + tips.extend(beta_tooltips()); + tips + }; } pub(crate) fn random_tooltip() -> Option<&'static str> { @@ -17,10 +31,12 @@ pub(crate) fn random_tooltip() -> Option<&'static str> { } fn pick_tooltip(rng: &mut R) -> Option<&'static str> { - if TOOLTIPS.is_empty() { + if ALL_TOOLTIPS.is_empty() { None } else { - TOOLTIPS.get(rng.random_range(0..TOOLTIPS.len())).copied() + ALL_TOOLTIPS + .get(rng.random_range(0..ALL_TOOLTIPS.len())) + .copied() } } diff --git a/codex-rs/tui/src/tui.rs b/codex-rs/tui/src/tui.rs index f9566da908..94190181a2 100644 --- a/codex-rs/tui/src/tui.rs +++ b/codex-rs/tui/src/tui.rs @@ -374,6 +374,14 @@ impl Tui { self.frame_requester().schedule_frame(); } + pub(crate) fn stash_pending_history_lines(&mut self) -> Vec> { + std::mem::take(&mut self.pending_history_lines) + } + + pub(crate) fn restore_pending_history_lines(&mut self, lines: Vec>) { + self.pending_history_lines = lines; + } + pub fn draw( &mut self, height: u16, diff --git a/codex-rs/tui2/docs/tui_viewport_and_history.md b/codex-rs/tui2/docs/tui_viewport_and_history.md index 8485e93646..3f7733f103 100644 --- a/codex-rs/tui2/docs/tui_viewport_and_history.md +++ b/codex-rs/tui2/docs/tui_viewport_and_history.md @@ -25,10 +25,12 @@ Historically, the legacy TUI tried to “cooperate” with the terminal’s own This had several failure modes: - **Terminal‑dependent behavior.** + - Different terminals handle scroll regions, clears, and resize semantics differently. - What looked correct in one terminal could drop or duplicate content in another. - **Resizes and layout churn.** + - The TUI reacts to resizes, focus changes, and overlay transitions. - When the viewport moved or its size changed, our attempts to keep scrollback “aligned” with the in‑memory history could go out of sync. @@ -57,24 +59,28 @@ order, and appears exactly once” across terminals, resizes, suspend/resume, an The redesign is guided by a few explicit goals: 1. **Codex, not the terminal, owns the viewport.** + - The in‑memory transcript (a list of history entries) is the single source of truth for what’s on screen. - The TUI decides how to map that transcript into the current viewport; scrollback becomes an output target, not an extra data structure we try to maintain. 2. **History must be correct, ordered, and never silently dropped.** + - Every logical history cell should either: - Be visible in the TUI, or - Have been printed into scrollback as part of a suspend/exit flow. - We would rather (rarely) duplicate content than risk losing it. 3. **Avoid unnecessary duplication.** + - When emitting history to scrollback (on suspend or exit), print each logical cell’s content at most once. - Streaming cells are allowed to be “re‑seen” as they grow, but finished cells should not keep reappearing. 4. **Behave sensibly under resizes.** + - TUI rendering should reflow to the current width on every frame. - History printed to scrollback may have been wrapped at different widths over time; that is acceptable, but it must not cause missing content or unbounded duplication. @@ -136,10 +142,12 @@ The TUI uses the terminal’s alternate screen for: Conceptually: - Entering alt screen: + - Switches the terminal into alt screen and expands the viewport to cover the full terminal. - Clears that alt‑screen buffer. - Leaving alt screen: + - Disables “alternate scroll” so mouse wheel events behave predictably. - Returns to the normal screen. @@ -159,11 +167,13 @@ is the in‑memory state. Mouse interaction is a first‑class part of the new design: - **Scrolling.** + - Mouse wheel scrolls the transcript in fixed line increments. - Keyboard shortcuts (PgUp/PgDn/Home/End) use the same scroll model, so the footer can show consistent hints regardless of input device. - **Selection.** + - A click‑and‑drag gesture defines a linear text selection in terms of the flattened transcript lines (not raw buffer coordinates). - Selection tracks the _content_ rather than a fixed screen row. When the transcript scrolls, the @@ -410,16 +420,103 @@ prints those lines before the token usage and resume hints. ## 10. Future Work and Open Questions +### 10.1 Current status + +This design shipped behind the `tui2` feature flag (as a separate crate, duplicating the legacy +`tui` crate to enable rollout without breaking existing behavior). The following items from early +feedback are already implemented: + +- Bottom pane positioning is pegged high with an empty transcript and moves down as the transcript + fills (including on resume). +- Wheel-based transcript scrolling is enabled on top of the new scroll model. +- While a selection is active, streaming stops “follow latest output” so the selection remains + stable, and follow mode resumes after the selection is cleared. + +### 10.2 Roadmap (prioritized) + +This section captures a prioritized list of improvements we want to add to TUI2 based on early +feedback, with the goal of making scrolling/selection/copy feel as close to “native terminal” (and +Vim) behavior as we can while still owning the viewport. + +**P0 — must-have (usability/correctness):** + +- **Scrolling behavior.** Default to small scroll increments (ideally 1 line per wheel tick) with + acceleration/velocity for faster navigation, and ensure we stop scrolling when the user stops + input (avoid redraw/event-loop backlog that makes scrolling feel “janky”). +- **Mouse event bounds.** Ignore mouse events outside the transcript region so clicks in the + composer/footer don’t start or mutate transcript selection state. +- **Copy includes offscreen lines.** Make copy operate on the full selection range even when part (or + all) of the selection is outside the current viewport. +- **Copy fidelity.** Preserve meaningful indentation (especially code blocks), treat soft-wrapped + prose as a single logical line when copying, and copy markdown _source_ (including backticks and + heading markers) even if we render it differently. + +**P1 — should-have (UX polish and power user workflows):** + +- **Streaming wrapping polish.** Ensure all streaming paths use display-time wrapping only, and add + tests that cover resizing after streaming has started. +- **Copy shortcut and discoverability.** Switch copy from `Ctrl+Y` to `Ctrl+Shift+C`, and add an + on-screen copy affordance (e.g. a small button near the selection) that also displays the + shortcut. +- **Selection semantics.** Define and implement selection behavior across multi-step output (and + whether step boundaries should be copy boundaries), while continuing to exclude the left gutter + from copied text. +- **Auto-scroll during drag.** While dragging a selection, auto-scroll when the cursor is at/near the + top or bottom of the transcript viewport to allow selecting beyond the visible window. +- **Width-aware selection.** Ensure selection highlighting and copy reconstruction handle wide glyphs + correctly (emoji, CJK), matching terminal display width rather than raw character count. +- **Multi-click selection.** Support double/triple/quad click selection (word/line/paragraph), + implemented on top of the transcript/viewport model rather than terminal buffer coordinates. +- **Find in transcript.** Add text search over the transcript (and consider integrating match + markers with any future scroll indicator work). +- **Cross-terminal behavior checks.** Validate copy/selection behavior across common terminals (incl. + terminal-provided “override selection” modes like holding Shift) and document the tradeoffs. + +**P2 — nice-to-have (polish, configuration, and interactivity):** + +- **Suspend printing.** Decide whether printing history on suspend is desirable at all (it is not + implemented yet). If we keep it, finalize the config shape/defaults, wire it through TUI startup, + and document it in the appropriate config docs. +- **Terminal integration.** Consider guiding (or optionally managing) terminal-emulator-specific + settings that affect TUI behavior (for example iTerm’s clipboard opt-in prompts or Ghostty + keybinding quirks), so the “works well out of the box” path is consistent across terminals. +- **Interactive cells (unlocked by transcript ownership).** Because transcript entries are structured + objects (not dead text in terminal scrollback), we can attach metadata to rendered regions and map + mouse/keys back to the underlying cell reliably across resizes and reflow. Examples: + - **Drill into a specific tool/command output.** Click (or press Enter) on a tool call / command + cell to open a focused overlay that shows the command, exit status, timing, and stdout/stderr as + separate sections, with dedicated “copy output” actions. This enables copying _just_ one command’s + output even when multiple commands are interleaved in a turn. + - **Copy an entire cell or entire turn.** Provide an action to copy a whole logical unit (one cell, + or “user prompt + assistant response”), without gutters and with well-defined boundaries. This is + hard to do with raw selection because step boundaries and padding aren’t reliably expressible in + terminal coordinates once the viewport moves or reflows. + - **Expand/collapse structured subregions with source-aware copy.** Tool calls, diffs, and + markdown can render in a compact form by default and expand in place. Copy actions can choose + between “copy rendered view” and “copy source” (e.g. raw markdown, raw JSON arguments, raw diff), + since we retain the original source alongside the rendered lines. + - **Cell-scoped actions.** Actions like “copy command”, “yank into composer”, “retry tool call”, or + “open related view” (diff/pager) can be offered per cell and behave deterministically, because the + UI can address cells by stable IDs rather than by fragile screen coordinates. +- **Additional affordances.** Consider an ephemeral scrollbar and/or a more explicit “selecting…” + status if footer hints aren’t sufficient. +- **UX capture.** Maintain short “golden path” clips showing scrolling (mouse + keys), selection and + copy, streaming under resize, and suspend/resume + exit printing. + +### 10.3 Open questions + This section collects design questions that follow naturally from the current model and are worth explicit discussion before we commit to further UI changes. - **“Scroll mode” vs “live follow” UI.** + - We already distinguish “scrolled away from bottom” vs “following the latest output” in the footer and scroll state. Do we need a more explicit “scroll mode vs live mode” affordance (e.g., a dedicated indicator or toggle), or is the current behavior sufficient and adding more chrome would be noise? - **Ephemeral scroll indicator.** + - For long sessions, a more visible sense of “where am I?” could help. One option is a minimalist scrollbar that appears while the user is actively scrolling and fades out when idle. A full “mini‑map” is probably too heavy for a TUI given the limited vertical space, but we could @@ -427,16 +524,19 @@ explicit discussion before we commit to further UI changes. where text search matches are, without trying to render a full preview of the buffer. - **Selection affordances.** + - Today, the primary hint that selection is active is the reversed text and the “Ctrl+Y copy selection” footer text. Do we want an explicit “Selecting… (Esc to cancel)” status while a drag is in progress, or would that be redundant/clutter for most users? - **Suspend banners in scrollback.** + - When printing history on suspend, should we also emit a small banner such as `--- codex suspended; history up to here ---` to make those boundaries obvious in scrollback? This would slightly increase noise but could make multi‑suspend sessions easier to read. - **Configuring suspend printing behavior.** + - The design already assumes that suspend‑time printing can be gated by config. Questions to resolve: - Should printing on suspend be on or off by default? @@ -451,4 +551,3 @@ explicit discussion before we commit to further UI changes. suspend‑time printing be our escape hatch for users who care about exact de‑duplication?\*\*\* --- - diff --git a/codex-rs/tui2/src/chatwidget.rs b/codex-rs/tui2/src/chatwidget.rs index 5461bea729..71a3014d1f 100644 --- a/codex-rs/tui2/src/chatwidget.rs +++ b/codex-rs/tui2/src/chatwidget.rs @@ -410,7 +410,10 @@ impl ChatWidget { } // Ask codex-core to enumerate custom prompts for this session. self.submit_op(Op::ListCustomPrompts); - self.submit_op(Op::ListSkills { cwds: Vec::new() }); + self.submit_op(Op::ListSkills { + cwds: Vec::new(), + force_reload: false, + }); if let Some(user_message) = self.initial_user_message.take() { self.submit_user_message(user_message); } @@ -1886,6 +1889,12 @@ impl ChatWidget { EventMsg::McpListToolsResponse(ev) => self.on_list_mcp_tools(ev), EventMsg::ListCustomPromptsResponse(ev) => self.on_list_custom_prompts(ev), EventMsg::ListSkillsResponse(ev) => self.on_list_skills(ev), + EventMsg::SkillsUpdateAvailable => { + self.submit_op(Op::ListSkills { + cwds: Vec::new(), + force_reload: true, + }); + } EventMsg::ShutdownComplete => self.on_shutdown_complete(), EventMsg::TurnDiff(TurnDiffEvent { unified_diff }) => self.on_turn_diff(unified_diff), EventMsg::DeprecationNotice(ev) => self.on_deprecation_notice(ev), @@ -1952,15 +1961,8 @@ impl ChatWidget { self.app_event_tx .send(AppEvent::InsertHistoryCell(Box::new(body_cell))); } - } else { - let message_text = - codex_core::review_format::format_review_findings_block(&output.findings, None); - let mut message_lines: Vec> = Vec::new(); - append_markdown(&message_text, None, &mut message_lines); - let body_cell = AgentMessageCell::new(message_lines, true); - self.app_event_tx - .send(AppEvent::InsertHistoryCell(Box::new(body_cell))); } + // Final message is rendered as part of the AgentMessage. } self.is_review_mode = false; @@ -2554,7 +2556,7 @@ impl ChatWidget { /// Open a popup to choose the approvals mode (ask for approval policy + sandbox policy). pub(crate) fn open_approvals_popup(&mut self) { - let current_approval = self.config.approval_policy; + let current_approval = self.config.approval_policy.value(); let current_sandbox = self.config.sandbox_policy.clone(); let mut items: Vec = Vec::new(); let presets: Vec = builtin_approval_presets(); @@ -2952,7 +2954,9 @@ impl ChatWidget { /// Set the approval policy in the widget's config copy. pub(crate) fn set_approval_policy(&mut self, policy: AskForApproval) { - self.config.approval_policy = policy; + if let Err(err) = self.config.approval_policy.set(policy) { + tracing::warn!(%err, "failed to set approval_policy on chat config"); + } } /// Set the sandbox policy in the widget's config copy. diff --git a/codex-rs/tui2/src/chatwidget/tests.rs b/codex-rs/tui2/src/chatwidget/tests.rs index 362b1678f4..15517cea22 100644 --- a/codex-rs/tui2/src/chatwidget/tests.rs +++ b/codex-rs/tui2/src/chatwidget/tests.rs @@ -10,6 +10,7 @@ use codex_core::CodexAuth; use codex_core::config::Config; use codex_core::config::ConfigOverrides; use codex_core::config::ConfigToml; +use codex_core::config::Constrained; use codex_core::openai_models::models_manager::ModelsManager; use codex_core::protocol::AgentMessageDeltaEvent; use codex_core::protocol::AgentMessageEvent; @@ -33,10 +34,6 @@ use codex_core::protocol::Op; use codex_core::protocol::PatchApplyBeginEvent; use codex_core::protocol::PatchApplyEndEvent; use codex_core::protocol::RateLimitWindow; -use codex_core::protocol::ReviewCodeLocation; -use codex_core::protocol::ReviewFinding; -use codex_core::protocol::ReviewLineRange; -use codex_core::protocol::ReviewOutputEvent; use codex_core::protocol::ReviewRequest; use codex_core::protocol::ReviewTarget; use codex_core::protocol::StreamErrorEvent; @@ -195,41 +192,6 @@ fn entered_review_mode_defaults_to_current_changes_banner() { assert!(chat.is_review_mode); } -/// Completing review with findings shows the selection popup and finishes with -/// the closing banner while clearing review mode state. -#[test] -fn exited_review_mode_emits_results_and_finishes() { - let (mut chat, mut rx, _ops) = make_chatwidget_manual(None); - - let review = ReviewOutputEvent { - findings: vec![ReviewFinding { - title: "[P1] Fix bug".to_string(), - body: "Something went wrong".to_string(), - confidence_score: 0.9, - priority: 1, - code_location: ReviewCodeLocation { - absolute_file_path: PathBuf::from("src/lib.rs"), - line_range: ReviewLineRange { start: 10, end: 12 }, - }, - }], - overall_correctness: "needs work".to_string(), - overall_explanation: "Investigate the failure".to_string(), - overall_confidence_score: 0.5, - }; - - chat.handle_codex_event(Event { - id: "review-end".into(), - msg: EventMsg::ExitedReviewMode(ExitedReviewModeEvent { - review_output: Some(review), - }), - }); - - let cells = drain_insert_history(&mut rx); - let banner = lines_to_single_string(cells.last().expect("finished banner")); - assert_eq!(banner, "\n<< Code review finished >>\n"); - assert!(!chat.is_review_mode); -} - /// Exiting review restores the pre-review context window indicator. #[test] fn review_restores_context_window_indicator() { @@ -2088,7 +2050,7 @@ fn approval_modal_exec_snapshot() { // Build a chat widget with manual channels to avoid spawning the agent. let (mut chat, _rx, _op_rx) = make_chatwidget_manual(None); // Ensure policy allows surfacing approvals explicitly (not strictly required for direct event). - chat.config.approval_policy = AskForApproval::OnRequest; + chat.config.approval_policy = Constrained::allow_any(AskForApproval::OnRequest); // Inject an exec approval request to display the approval modal. let ev = ExecApprovalRequestEvent { call_id: "call-approve-cmd".into(), @@ -2141,7 +2103,7 @@ fn approval_modal_exec_snapshot() { #[test] fn approval_modal_exec_without_reason_snapshot() { let (mut chat, _rx, _op_rx) = make_chatwidget_manual(None); - chat.config.approval_policy = AskForApproval::OnRequest; + chat.config.approval_policy = Constrained::allow_any(AskForApproval::OnRequest); let ev = ExecApprovalRequestEvent { call_id: "call-approve-cmd-noreason".into(), @@ -2179,7 +2141,7 @@ fn approval_modal_exec_without_reason_snapshot() { #[test] fn approval_modal_patch_snapshot() { let (mut chat, _rx, _op_rx) = make_chatwidget_manual(None); - chat.config.approval_policy = AskForApproval::OnRequest; + chat.config.approval_policy = Constrained::allow_any(AskForApproval::OnRequest); // Build a small changeset and a reason/grant_root to exercise the prompt text. let mut changes = HashMap::new(); @@ -2778,7 +2740,7 @@ fn apply_patch_full_flow_integration_like() { fn apply_patch_untrusted_shows_approval_modal() { let (mut chat, _rx, _op_rx) = make_chatwidget_manual(None); // Ensure approval policy is untrusted (OnRequest) - chat.config.approval_policy = AskForApproval::OnRequest; + chat.config.approval_policy = Constrained::allow_any(AskForApproval::OnRequest); // Simulate a patch approval request from backend let mut changes = HashMap::new(); @@ -2824,7 +2786,7 @@ fn apply_patch_request_shows_diff_summary() { let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(None); // Ensure we are in OnRequest so an approval is surfaced - chat.config.approval_policy = AskForApproval::OnRequest; + chat.config.approval_policy = Constrained::allow_any(AskForApproval::OnRequest); // Simulate backend asking to apply a patch adding two lines to README.md let mut changes = HashMap::new(); diff --git a/codex-rs/tui2/src/resume_picker.rs b/codex-rs/tui2/src/resume_picker.rs index f2c6a3269d..7f3665d563 100644 --- a/codex-rs/tui2/src/resume_picker.rs +++ b/codex-rs/tui2/src/resume_picker.rs @@ -10,6 +10,7 @@ use codex_core::ConversationsPage; use codex_core::Cursor; use codex_core::INTERACTIVE_SESSION_SOURCES; use codex_core::RolloutRecorder; +use codex_core::path_utils; use codex_protocol::items::TurnItem; use color_eyre::eyre::Result; use crossterm::event::KeyCode; @@ -670,7 +671,10 @@ fn extract_session_meta_from_head(head: &[serde_json::Value]) -> (Option bool { - if let (Ok(ca), Ok(cb)) = (a.canonicalize(), b.canonicalize()) { + if let (Ok(ca), Ok(cb)) = ( + path_utils::normalize_for_path_comparison(a), + path_utils::normalize_for_path_comparison(b), + ) { return ca == cb; } a == b diff --git a/codex-rs/tui2/src/skill_error_prompt.rs b/codex-rs/tui2/src/skill_error_prompt.rs index 41aaefa4c9..b97fb02d16 100644 --- a/codex-rs/tui2/src/skill_error_prompt.rs +++ b/codex-rs/tui2/src/skill_error_prompt.rs @@ -1,6 +1,8 @@ use crate::tui::FrameRequester; use crate::tui::Tui; use crate::tui::TuiEvent; +use crate::wrapping::RtOptions; +use crate::wrapping::word_wrap_line; use codex_core::skills::SkillError; use crossterm::event::KeyCode; use crossterm::event::KeyEvent; @@ -16,7 +18,6 @@ use ratatui::widgets::Clear; use ratatui::widgets::Paragraph; use ratatui::widgets::Widget; use ratatui::widgets::WidgetRef; -use ratatui::widgets::Wrap; use tokio_stream::StreamExt; #[derive(Clone, Copy, Debug, PartialEq, Eq)] @@ -31,16 +32,24 @@ pub(crate) async fn run_skill_error_prompt( ) -> SkillErrorPromptOutcome { struct AltScreenGuard<'a> { tui: &'a mut Tui, + stashed_history_lines: Vec>, } impl<'a> AltScreenGuard<'a> { fn enter(tui: &'a mut Tui) -> Self { let _ = tui.enter_alt_screen(); - Self { tui } + let stashed_history_lines = tui.stash_pending_history_lines(); + Self { + tui, + stashed_history_lines, + } } } impl Drop for AltScreenGuard<'_> { fn drop(&mut self) { let _ = self.tui.leave_alt_screen(); + let stashed_history_lines = std::mem::take(&mut self.stashed_history_lines); + self.tui + .restore_pending_history_lines(stashed_history_lines); } } @@ -77,28 +86,16 @@ pub(crate) async fn run_skill_error_prompt( struct SkillErrorScreen { request_frame: FrameRequester, - lines: Vec>, + errors: Vec, done: bool, exit: bool, } impl SkillErrorScreen { fn new(request_frame: FrameRequester, errors: &[SkillError]) -> Self { - let mut lines: Vec> = Vec::new(); - lines.push(Line::from("Skill validation errors detected".bold())); - lines.push(Line::from( - "Fix these SKILL.md files and restart. Invalid skills are ignored until resolved. Press enter or esc to continue, Ctrl+C or Ctrl+D to exit.", - )); - lines.push(Line::from("")); - - for error in errors { - let message = format!("- {}: {}", error.path.display(), error.message); - lines.push(Line::from(message)); - } - Self { request_frame, - lines, + errors: errors.to_vec(), done: false, exit: false, } @@ -154,12 +151,44 @@ impl SkillErrorScreen { impl WidgetRef for &SkillErrorScreen { fn render_ref(&self, area: Rect, buf: &mut Buffer) { Clear.render(area, buf); + let block = Block::default() .title("Skill errors".bold()) .borders(Borders::ALL); - Paragraph::new(self.lines.clone()) - .block(block) - .wrap(Wrap { trim: true }) - .render(area, buf); + + let inner = block.inner(area); + let width = usize::from(inner.width).max(1); + + let mut base_lines: Vec> = vec![ + Line::from("Skill validation errors detected".bold()), + Line::from("Fix these SKILL.md files and restart."), + Line::from("Invalid skills are ignored until resolved."), + Line::from("Press enter or esc to continue. Ctrl+C or Ctrl+D to exit."), + Line::from(""), + ]; + + let error_start = base_lines.len(); + for error in &self.errors { + base_lines.push(Line::from(vec![ + error.path.display().to_string().dim(), + ": ".into(), + error.message.clone().red(), + ])); + } + + let error_wrap_opts = RtOptions::new(width) + .initial_indent(Line::from("- ")) + .subsequent_indent(Line::from(" ")); + + let mut lines: Vec> = Vec::new(); + for (idx, line) in base_lines.iter().enumerate() { + if idx < error_start { + lines.extend(word_wrap_line(line, width)); + } else { + lines.extend(word_wrap_line(line, error_wrap_opts.clone())); + } + } + + Paragraph::new(lines).block(block).render(area, buf); } } diff --git a/codex-rs/tui2/src/tui.rs b/codex-rs/tui2/src/tui.rs index 807c807c87..595a8dd465 100644 --- a/codex-rs/tui2/src/tui.rs +++ b/codex-rs/tui2/src/tui.rs @@ -334,6 +334,14 @@ impl Tui { self.frame_requester().schedule_frame(); } + pub(crate) fn stash_pending_history_lines(&mut self) -> Vec> { + std::mem::take(&mut self.pending_history_lines) + } + + pub(crate) fn restore_pending_history_lines(&mut self, lines: Vec>) { + self.pending_history_lines = lines; + } + pub fn draw( &mut self, height: u16, diff --git a/codex-rs/utils/git/src/ghost_commits.rs b/codex-rs/utils/git/src/ghost_commits.rs index 1e3c606f09..4555781185 100644 --- a/codex-rs/utils/git/src/ghost_commits.rs +++ b/codex-rs/utils/git/src/ghost_commits.rs @@ -65,6 +65,7 @@ pub struct RestoreGhostCommitOptions<'a> { pub struct GhostSnapshotConfig { pub ignore_large_untracked_files: Option, pub ignore_large_untracked_dirs: Option, + pub disable_warnings: bool, } impl Default for GhostSnapshotConfig { @@ -72,6 +73,7 @@ impl Default for GhostSnapshotConfig { Self { ignore_large_untracked_files: Some(DEFAULT_IGNORE_LARGE_UNTRACKED_FILES), ignore_large_untracked_dirs: Some(DEFAULT_IGNORE_LARGE_UNTRACKED_DIRS), + disable_warnings: false, } } } @@ -1164,6 +1166,7 @@ mod tests { let snapshot_config = GhostSnapshotConfig { ignore_large_untracked_files: Some(DEFAULT_IGNORE_LARGE_UNTRACKED_FILES), ignore_large_untracked_dirs: Some(threshold), + disable_warnings: false, }; let (ghost, _report) = create_ghost_commit_with_report( &CreateGhostCommitOptions::new(repo).ghost_snapshot(snapshot_config), diff --git a/codex-rs/utils/pty/src/lib.rs b/codex-rs/utils/pty/src/lib.rs index 38e86bd2a6..9e05416ed8 100644 --- a/codex-rs/utils/pty/src/lib.rs +++ b/codex-rs/utils/pty/src/lib.rs @@ -133,6 +133,15 @@ pub struct SpawnedPty { pub exit_rx: oneshot::Receiver, } +#[allow(unreachable_code)] +pub fn conpty_supported() -> bool { + // Annotation required because `win` can't be compiled on other OS. + #[cfg(windows)] + return win::conpty_supported(); + + true +} + #[cfg(windows)] fn platform_native_pty_system() -> Box { Box::new(win::ConPtySystem::default()) diff --git a/codex-rs/utils/pty/src/win/mod.rs b/codex-rs/utils/pty/src/win/mod.rs index 8206c9b890..27057a3642 100644 --- a/codex-rs/utils/pty/src/win/mod.rs +++ b/codex-rs/utils/pty/src/win/mod.rs @@ -41,6 +41,7 @@ mod procthreadattr; mod psuedocon; pub use conpty::ConPtySystem; +pub use psuedocon::conpty_supported; #[derive(Debug)] pub struct WinChild { diff --git a/codex-rs/utils/pty/src/win/psuedocon.rs b/codex-rs/utils/pty/src/win/psuedocon.rs index a8db98eefe..4365180c73 100644 --- a/codex-rs/utils/pty/src/win/psuedocon.rs +++ b/codex-rs/utils/pty/src/win/psuedocon.rs @@ -42,6 +42,8 @@ use std::path::Path; use std::ptr; use std::sync::Mutex; use winapi::shared::minwindef::DWORD; +use winapi::shared::ntdef::NTSTATUS; +use winapi::shared::ntstatus::STATUS_SUCCESS; use winapi::shared::winerror::HRESULT; use winapi::shared::winerror::S_OK; use winapi::um::handleapi::*; @@ -52,6 +54,7 @@ use winapi::um::winbase::STARTF_USESTDHANDLES; use winapi::um::winbase::STARTUPINFOEXW; use winapi::um::wincon::COORD; use winapi::um::winnt::HANDLE; +use winapi::um::winnt::OSVERSIONINFOW; pub type HPCON = HANDLE; @@ -59,6 +62,10 @@ pub const PSEUDOCONSOLE_RESIZE_QUIRK: DWORD = 0x2; #[allow(dead_code)] pub const PSEUDOCONSOLE_PASSTHROUGH_MODE: DWORD = 0x8; +// https://learn.microsoft.com/en-gb/windows/console/createpseudoconsole +// https://learn.microsoft.com/en-gb/windows/release-health/release-information +const MIN_CONPTY_BUILD: u32 = 17_763; + shared_library!(ConPtyFuncs, pub fn CreatePseudoConsole( size: COORD, @@ -71,6 +78,12 @@ shared_library!(ConPtyFuncs, pub fn ClosePseudoConsole(hpc: HPCON), ); +shared_library!(Ntdll, + pub fn RtlGetVersion( + version_info: *mut OSVERSIONINFOW + ) -> NTSTATUS, +); + fn load_conpty() -> ConPtyFuncs { let kernel = ConPtyFuncs::open(Path::new("kernel32.dll")).expect( "this system does not support conpty. Windows 10 October 2018 or newer is required", @@ -87,6 +100,22 @@ lazy_static! { static ref CONPTY: ConPtyFuncs = load_conpty(); } +pub fn conpty_supported() -> bool { + windows_build_number().is_some_and(|build| build >= MIN_CONPTY_BUILD) +} + +fn windows_build_number() -> Option { + let ntdll = Ntdll::open(Path::new("ntdll.dll")).ok()?; + let mut info: OSVERSIONINFOW = unsafe { mem::zeroed() }; + info.dwOSVersionInfoSize = mem::size_of::() as u32; + let status = unsafe { (ntdll.RtlGetVersion)(&mut info) }; + if status == STATUS_SUCCESS { + Some(info.dwBuildNumber) + } else { + None + } +} + pub struct PsuedoCon { con: HPCON, } @@ -320,3 +349,17 @@ fn append_quoted(arg: &OsStr, cmdline: &mut Vec) { } cmdline.push('"' as u16); } + +#[cfg(test)] +mod tests { + use super::windows_build_number; + use super::MIN_CONPTY_BUILD; + + #[test] + fn windows_build_number_returns_value() { + // We can't stably check the version of the GH workers, but we can + // at least check that this. + let version = windows_build_number().unwrap(); + assert!(version > MIN_CONPTY_BUILD); + } +} diff --git a/docs/config.md b/docs/config.md index 364298c2b5..a66e0cc89b 100644 --- a/docs/config.md +++ b/docs/config.md @@ -944,6 +944,7 @@ Valid values: | `tui.animations` | boolean | Enable terminal animations (welcome screen, shimmer, spinner). Defaults to true; set to `false` to disable visual motion. | | `instructions` | string | Currently ignored; use `experimental_instructions_file` or `AGENTS.md`. | | `features.` | boolean | See [feature flags](#feature-flags) for details | +| `ghost_snapshot.disable_warnings` | boolean | Disable every warnings around ghost snapshot (large files, directory, ...) | | `ghost_snapshot.ignore_large_untracked_files` | number | Exclude untracked files larger than this many bytes from ghost snapshots (default: 10 MiB). Set to `0` to disable. | | `ghost_snapshot.ignore_large_untracked_dirs` | number | Ignore untracked directories with at least this many files (default: 200). Set to `0` to disable. | | `mcp_servers..command` | string | MCP server launcher command (stdio servers only). | diff --git a/docs/slash_commands.md b/docs/slash_commands.md index ed5b4dc85a..c1f9daf9d4 100644 --- a/docs/slash_commands.md +++ b/docs/slash_commands.md @@ -10,24 +10,25 @@ Slash commands are special commands you can type that start with `/`. Control Codex’s behavior during an interactive session with slash commands. -| Command | Purpose | -| ------------ | -------------------------------------------------------------------------- | -| `/model` | choose what model and reasoning effort to use | -| `/approvals` | choose what Codex can do without approval | -| `/review` | review my current changes and find issues | -| `/new` | start a new chat during a conversation | -| `/resume` | resume an old chat | -| `/init` | create an AGENTS.md file with instructions for Codex | -| `/compact` | summarize conversation to prevent hitting the context limit | -| `/undo` | ask Codex to undo a turn | -| `/diff` | show git diff (including untracked files) | -| `/mention` | mention a file | -| `/status` | show current session configuration and token usage | -| `/mcp` | list configured MCP tools | -| `/skills` | browse and insert skills (experimental; see [docs/skills.md](./skills.md)) | -| `/logout` | log out of Codex | -| `/quit` | exit Codex | -| `/exit` | exit Codex | -| `/feedback` | send logs to maintainers | +| Command | Purpose | +| --------------- | -------------------------------------------------------------------------- | +| `/model` | choose what model and reasoning effort to use | +| `/approvals` | choose what Codex can do without approval | +| `/review` | review my current changes and find issues | +| `/new` | start a new chat during a conversation | +| `/resume` | resume an old chat | +| `/init` | create an AGENTS.md file with instructions for Codex | +| `/compact` | summarize conversation to prevent hitting the context limit | +| `/undo` | ask Codex to undo a turn | +| `/diff` | show git diff (including untracked files) | +| `/mention` | mention a file | +| `/status` | show current session configuration and token usage | +| `/mcp` | list configured MCP tools | +| `/experimental` | open the experimental menu to enable features from our beta program | +| `/skills` | browse and insert skills (experimental; see [docs/skills.md](./skills.md)) | +| `/logout` | log out of Codex | +| `/quit` | exit Codex | +| `/exit` | exit Codex | +| `/feedback` | send logs to maintainers | ---