diff --git a/.github/workflows/rust-ci-full.yml b/.github/workflows/rust-ci-full.yml index 146e57524c..97fa33283e 100644 --- a/.github/workflows/rust-ci-full.yml +++ b/.github/workflows/rust-ci-full.yml @@ -3,6 +3,7 @@ on: push: branches: - main + - "**full-ci**" workflow_dispatch: # CI builds in debug (dev) for faster signal. diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index 73b62f36ee..f754bdfb90 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -1543,11 +1543,14 @@ dependencies = [ "anyhow", "assert_cmd", "assert_matches", + "codex-exec-server", + "codex-utils-absolute-path", "codex-utils-cargo-bin", "pretty_assertions", "similar", "tempfile", "thiserror 2.0.18", + "tokio", "tree-sitter", "tree-sitter-bash", ] @@ -1558,9 +1561,11 @@ version = "0.0.0" dependencies = [ "anyhow", "codex-apply-patch", + "codex-exec-server", "codex-linux-sandbox", "codex-sandboxing", "codex-shell-escalation", + "codex-utils-absolute-path", "codex-utils-home-dir", "dotenvy", "tempfile", diff --git a/codex-rs/app-server-protocol/schema/json/ClientRequest.json b/codex-rs/app-server-protocol/schema/json/ClientRequest.json index 88c828ba7a..ab7ea7b42e 100644 --- a/codex-rs/app-server-protocol/schema/json/ClientRequest.json +++ b/codex-rs/app-server-protocol/schema/json/ClientRequest.json @@ -800,7 +800,7 @@ "description": "Stop filesystem watch notifications for a prior `fs/watch`.", "properties": { "watchId": { - "description": "Watch identifier returned by `fs/watch`.", + "description": "Watch identifier previously provided to `fs/watch`.", "type": "string" } }, @@ -819,10 +819,15 @@ } ], "description": "Absolute file or directory path to watch." + }, + "watchId": { + "description": "Connection-scoped watch identifier used for `fs/unwatch` and `fs/changed`.", + "type": "string" } }, "required": [ - "path" + "path", + "watchId" ], "type": "object" }, diff --git a/codex-rs/app-server-protocol/schema/json/ServerNotification.json b/codex-rs/app-server-protocol/schema/json/ServerNotification.json index 116844171b..d550f523ce 100644 --- a/codex-rs/app-server-protocol/schema/json/ServerNotification.json +++ b/codex-rs/app-server-protocol/schema/json/ServerNotification.json @@ -1013,7 +1013,7 @@ "type": "array" }, "watchId": { - "description": "Watch identifier returned by `fs/watch`.", + "description": "Watch identifier previously provided to `fs/watch`.", "type": "string" } }, @@ -1736,6 +1736,7 @@ }, "McpToolCallResult": { "properties": { + "_meta": true, "content": { "items": true, "type": "array" diff --git a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json index 936831f003..d6124b6802 100644 --- a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json +++ b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json @@ -7511,7 +7511,7 @@ "type": "array" }, "watchId": { - "description": "Watch identifier returned by `fs/watch`.", + "description": "Watch identifier previously provided to `fs/watch`.", "type": "string" } }, @@ -7781,7 +7781,7 @@ "description": "Stop filesystem watch notifications for a prior `fs/watch`.", "properties": { "watchId": { - "description": "Watch identifier returned by `fs/watch`.", + "description": "Watch identifier previously provided to `fs/watch`.", "type": "string" } }, @@ -7808,25 +7808,6 @@ } ], "description": "Absolute file or directory path to watch." - } - }, - "required": [ - "path" - ], - "title": "FsWatchParams", - "type": "object" - }, - "FsWatchResponse": { - "$schema": "http://json-schema.org/draft-07/schema#", - "description": "Created watch handle returned by `fs/watch`.", - "properties": { - "path": { - "allOf": [ - { - "$ref": "#/definitions/v2/AbsolutePathBuf" - } - ], - "description": "Canonicalized path associated with the watch." }, "watchId": { "description": "Connection-scoped watch identifier used for `fs/unwatch` and `fs/changed`.", @@ -7837,6 +7818,25 @@ "path", "watchId" ], + "title": "FsWatchParams", + "type": "object" + }, + "FsWatchResponse": { + "$schema": "http://json-schema.org/draft-07/schema#", + "description": "Successful response for `fs/watch`.", + "properties": { + "path": { + "allOf": [ + { + "$ref": "#/definitions/v2/AbsolutePathBuf" + } + ], + "description": "Canonicalized path associated with the watch." + } + }, + "required": [ + "path" + ], "title": "FsWatchResponse", "type": "object" }, @@ -9184,6 +9184,7 @@ }, "McpToolCallResult": { "properties": { + "_meta": true, "content": { "items": true, "type": "array" diff --git a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json index 8fa00a7207..6eee862a38 100644 --- a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json +++ b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json @@ -4179,7 +4179,7 @@ "type": "array" }, "watchId": { - "description": "Watch identifier returned by `fs/watch`.", + "description": "Watch identifier previously provided to `fs/watch`.", "type": "string" } }, @@ -4449,7 +4449,7 @@ "description": "Stop filesystem watch notifications for a prior `fs/watch`.", "properties": { "watchId": { - "description": "Watch identifier returned by `fs/watch`.", + "description": "Watch identifier previously provided to `fs/watch`.", "type": "string" } }, @@ -4476,25 +4476,6 @@ } ], "description": "Absolute file or directory path to watch." - } - }, - "required": [ - "path" - ], - "title": "FsWatchParams", - "type": "object" - }, - "FsWatchResponse": { - "$schema": "http://json-schema.org/draft-07/schema#", - "description": "Created watch handle returned by `fs/watch`.", - "properties": { - "path": { - "allOf": [ - { - "$ref": "#/definitions/AbsolutePathBuf" - } - ], - "description": "Canonicalized path associated with the watch." }, "watchId": { "description": "Connection-scoped watch identifier used for `fs/unwatch` and `fs/changed`.", @@ -4505,6 +4486,25 @@ "path", "watchId" ], + "title": "FsWatchParams", + "type": "object" + }, + "FsWatchResponse": { + "$schema": "http://json-schema.org/draft-07/schema#", + "description": "Successful response for `fs/watch`.", + "properties": { + "path": { + "allOf": [ + { + "$ref": "#/definitions/AbsolutePathBuf" + } + ], + "description": "Canonicalized path associated with the watch." + } + }, + "required": [ + "path" + ], "title": "FsWatchResponse", "type": "object" }, @@ -6007,6 +6007,7 @@ }, "McpToolCallResult": { "properties": { + "_meta": true, "content": { "items": true, "type": "array" diff --git a/codex-rs/app-server-protocol/schema/json/v2/FsChangedNotification.json b/codex-rs/app-server-protocol/schema/json/v2/FsChangedNotification.json index ab26588ecb..cfb9f4e5ba 100644 --- a/codex-rs/app-server-protocol/schema/json/v2/FsChangedNotification.json +++ b/codex-rs/app-server-protocol/schema/json/v2/FsChangedNotification.json @@ -16,7 +16,7 @@ "type": "array" }, "watchId": { - "description": "Watch identifier returned by `fs/watch`.", + "description": "Watch identifier previously provided to `fs/watch`.", "type": "string" } }, diff --git a/codex-rs/app-server-protocol/schema/json/v2/FsUnwatchParams.json b/codex-rs/app-server-protocol/schema/json/v2/FsUnwatchParams.json index 4b988d97aa..f46800e997 100644 --- a/codex-rs/app-server-protocol/schema/json/v2/FsUnwatchParams.json +++ b/codex-rs/app-server-protocol/schema/json/v2/FsUnwatchParams.json @@ -3,7 +3,7 @@ "description": "Stop filesystem watch notifications for a prior `fs/watch`.", "properties": { "watchId": { - "description": "Watch identifier returned by `fs/watch`.", + "description": "Watch identifier previously provided to `fs/watch`.", "type": "string" } }, diff --git a/codex-rs/app-server-protocol/schema/json/v2/FsWatchParams.json b/codex-rs/app-server-protocol/schema/json/v2/FsWatchParams.json index cf80c7b101..29a1ceea14 100644 --- a/codex-rs/app-server-protocol/schema/json/v2/FsWatchParams.json +++ b/codex-rs/app-server-protocol/schema/json/v2/FsWatchParams.json @@ -15,10 +15,15 @@ } ], "description": "Absolute file or directory path to watch." + }, + "watchId": { + "description": "Connection-scoped watch identifier used for `fs/unwatch` and `fs/changed`.", + "type": "string" } }, "required": [ - "path" + "path", + "watchId" ], "title": "FsWatchParams", "type": "object" diff --git a/codex-rs/app-server-protocol/schema/json/v2/FsWatchResponse.json b/codex-rs/app-server-protocol/schema/json/v2/FsWatchResponse.json index b516636a09..abc7d466bb 100644 --- a/codex-rs/app-server-protocol/schema/json/v2/FsWatchResponse.json +++ b/codex-rs/app-server-protocol/schema/json/v2/FsWatchResponse.json @@ -6,7 +6,7 @@ "type": "string" } }, - "description": "Created watch handle returned by `fs/watch`.", + "description": "Successful response for `fs/watch`.", "properties": { "path": { "allOf": [ @@ -15,15 +15,10 @@ } ], "description": "Canonicalized path associated with the watch." - }, - "watchId": { - "description": "Connection-scoped watch identifier used for `fs/unwatch` and `fs/changed`.", - "type": "string" } }, "required": [ - "path", - "watchId" + "path" ], "title": "FsWatchResponse", "type": "object" diff --git a/codex-rs/app-server-protocol/schema/json/v2/ItemCompletedNotification.json b/codex-rs/app-server-protocol/schema/json/v2/ItemCompletedNotification.json index 3964107865..2883670c88 100644 --- a/codex-rs/app-server-protocol/schema/json/v2/ItemCompletedNotification.json +++ b/codex-rs/app-server-protocol/schema/json/v2/ItemCompletedNotification.json @@ -294,6 +294,7 @@ }, "McpToolCallResult": { "properties": { + "_meta": true, "content": { "items": true, "type": "array" diff --git a/codex-rs/app-server-protocol/schema/json/v2/ItemStartedNotification.json b/codex-rs/app-server-protocol/schema/json/v2/ItemStartedNotification.json index abb8aee5dc..c2e71ccba9 100644 --- a/codex-rs/app-server-protocol/schema/json/v2/ItemStartedNotification.json +++ b/codex-rs/app-server-protocol/schema/json/v2/ItemStartedNotification.json @@ -294,6 +294,7 @@ }, "McpToolCallResult": { "properties": { + "_meta": true, "content": { "items": true, "type": "array" diff --git a/codex-rs/app-server-protocol/schema/json/v2/ReviewStartResponse.json b/codex-rs/app-server-protocol/schema/json/v2/ReviewStartResponse.json index 4bdeebf573..a7fe2e8d60 100644 --- a/codex-rs/app-server-protocol/schema/json/v2/ReviewStartResponse.json +++ b/codex-rs/app-server-protocol/schema/json/v2/ReviewStartResponse.json @@ -430,6 +430,7 @@ }, "McpToolCallResult": { "properties": { + "_meta": true, "content": { "items": true, "type": "array" diff --git a/codex-rs/app-server-protocol/schema/json/v2/ThreadForkResponse.json b/codex-rs/app-server-protocol/schema/json/v2/ThreadForkResponse.json index a79ac103c7..9b1870198a 100644 --- a/codex-rs/app-server-protocol/schema/json/v2/ThreadForkResponse.json +++ b/codex-rs/app-server-protocol/schema/json/v2/ThreadForkResponse.json @@ -518,6 +518,7 @@ }, "McpToolCallResult": { "properties": { + "_meta": true, "content": { "items": true, "type": "array" diff --git a/codex-rs/app-server-protocol/schema/json/v2/ThreadListResponse.json b/codex-rs/app-server-protocol/schema/json/v2/ThreadListResponse.json index e8211b2521..426f34ce35 100644 --- a/codex-rs/app-server-protocol/schema/json/v2/ThreadListResponse.json +++ b/codex-rs/app-server-protocol/schema/json/v2/ThreadListResponse.json @@ -456,6 +456,7 @@ }, "McpToolCallResult": { "properties": { + "_meta": true, "content": { "items": true, "type": "array" diff --git a/codex-rs/app-server-protocol/schema/json/v2/ThreadMetadataUpdateResponse.json b/codex-rs/app-server-protocol/schema/json/v2/ThreadMetadataUpdateResponse.json index 08c4696bb3..c869a79749 100644 --- a/codex-rs/app-server-protocol/schema/json/v2/ThreadMetadataUpdateResponse.json +++ b/codex-rs/app-server-protocol/schema/json/v2/ThreadMetadataUpdateResponse.json @@ -456,6 +456,7 @@ }, "McpToolCallResult": { "properties": { + "_meta": true, "content": { "items": true, "type": "array" diff --git a/codex-rs/app-server-protocol/schema/json/v2/ThreadReadResponse.json b/codex-rs/app-server-protocol/schema/json/v2/ThreadReadResponse.json index f6d67d5c0c..9569860c38 100644 --- a/codex-rs/app-server-protocol/schema/json/v2/ThreadReadResponse.json +++ b/codex-rs/app-server-protocol/schema/json/v2/ThreadReadResponse.json @@ -456,6 +456,7 @@ }, "McpToolCallResult": { "properties": { + "_meta": true, "content": { "items": true, "type": "array" diff --git a/codex-rs/app-server-protocol/schema/json/v2/ThreadResumeResponse.json b/codex-rs/app-server-protocol/schema/json/v2/ThreadResumeResponse.json index 1697435049..5143545ec1 100644 --- a/codex-rs/app-server-protocol/schema/json/v2/ThreadResumeResponse.json +++ b/codex-rs/app-server-protocol/schema/json/v2/ThreadResumeResponse.json @@ -518,6 +518,7 @@ }, "McpToolCallResult": { "properties": { + "_meta": true, "content": { "items": true, "type": "array" diff --git a/codex-rs/app-server-protocol/schema/json/v2/ThreadRollbackResponse.json b/codex-rs/app-server-protocol/schema/json/v2/ThreadRollbackResponse.json index e664490df9..502dd3961f 100644 --- a/codex-rs/app-server-protocol/schema/json/v2/ThreadRollbackResponse.json +++ b/codex-rs/app-server-protocol/schema/json/v2/ThreadRollbackResponse.json @@ -456,6 +456,7 @@ }, "McpToolCallResult": { "properties": { + "_meta": true, "content": { "items": true, "type": "array" diff --git a/codex-rs/app-server-protocol/schema/json/v2/ThreadStartResponse.json b/codex-rs/app-server-protocol/schema/json/v2/ThreadStartResponse.json index 98e022c751..c07b4d8258 100644 --- a/codex-rs/app-server-protocol/schema/json/v2/ThreadStartResponse.json +++ b/codex-rs/app-server-protocol/schema/json/v2/ThreadStartResponse.json @@ -518,6 +518,7 @@ }, "McpToolCallResult": { "properties": { + "_meta": true, "content": { "items": true, "type": "array" diff --git a/codex-rs/app-server-protocol/schema/json/v2/ThreadStartedNotification.json b/codex-rs/app-server-protocol/schema/json/v2/ThreadStartedNotification.json index f640c40969..ff87af2069 100644 --- a/codex-rs/app-server-protocol/schema/json/v2/ThreadStartedNotification.json +++ b/codex-rs/app-server-protocol/schema/json/v2/ThreadStartedNotification.json @@ -456,6 +456,7 @@ }, "McpToolCallResult": { "properties": { + "_meta": true, "content": { "items": true, "type": "array" diff --git a/codex-rs/app-server-protocol/schema/json/v2/ThreadUnarchiveResponse.json b/codex-rs/app-server-protocol/schema/json/v2/ThreadUnarchiveResponse.json index 1c10ea3b84..daf821c374 100644 --- a/codex-rs/app-server-protocol/schema/json/v2/ThreadUnarchiveResponse.json +++ b/codex-rs/app-server-protocol/schema/json/v2/ThreadUnarchiveResponse.json @@ -456,6 +456,7 @@ }, "McpToolCallResult": { "properties": { + "_meta": true, "content": { "items": true, "type": "array" diff --git a/codex-rs/app-server-protocol/schema/json/v2/TurnCompletedNotification.json b/codex-rs/app-server-protocol/schema/json/v2/TurnCompletedNotification.json index fc1deb675f..82c2b3c76c 100644 --- a/codex-rs/app-server-protocol/schema/json/v2/TurnCompletedNotification.json +++ b/codex-rs/app-server-protocol/schema/json/v2/TurnCompletedNotification.json @@ -430,6 +430,7 @@ }, "McpToolCallResult": { "properties": { + "_meta": true, "content": { "items": true, "type": "array" diff --git a/codex-rs/app-server-protocol/schema/json/v2/TurnStartResponse.json b/codex-rs/app-server-protocol/schema/json/v2/TurnStartResponse.json index 5b79f4df11..ebb2065cb8 100644 --- a/codex-rs/app-server-protocol/schema/json/v2/TurnStartResponse.json +++ b/codex-rs/app-server-protocol/schema/json/v2/TurnStartResponse.json @@ -430,6 +430,7 @@ }, "McpToolCallResult": { "properties": { + "_meta": true, "content": { "items": true, "type": "array" diff --git a/codex-rs/app-server-protocol/schema/json/v2/TurnStartedNotification.json b/codex-rs/app-server-protocol/schema/json/v2/TurnStartedNotification.json index ccbe93e9c2..8b7c2bc410 100644 --- a/codex-rs/app-server-protocol/schema/json/v2/TurnStartedNotification.json +++ b/codex-rs/app-server-protocol/schema/json/v2/TurnStartedNotification.json @@ -430,6 +430,7 @@ }, "McpToolCallResult": { "properties": { + "_meta": true, "content": { "items": true, "type": "array" diff --git a/codex-rs/app-server-protocol/schema/typescript/v2/FsChangedNotification.ts b/codex-rs/app-server-protocol/schema/typescript/v2/FsChangedNotification.ts index 2e9bd0d6ec..fc0fa9cf6d 100644 --- a/codex-rs/app-server-protocol/schema/typescript/v2/FsChangedNotification.ts +++ b/codex-rs/app-server-protocol/schema/typescript/v2/FsChangedNotification.ts @@ -8,7 +8,7 @@ import type { AbsolutePathBuf } from "../AbsolutePathBuf"; */ export type FsChangedNotification = { /** - * Watch identifier returned by `fs/watch`. + * Watch identifier previously provided to `fs/watch`. */ watchId: string, /** diff --git a/codex-rs/app-server-protocol/schema/typescript/v2/FsUnwatchParams.ts b/codex-rs/app-server-protocol/schema/typescript/v2/FsUnwatchParams.ts index b21befdb5c..34b70a9307 100644 --- a/codex-rs/app-server-protocol/schema/typescript/v2/FsUnwatchParams.ts +++ b/codex-rs/app-server-protocol/schema/typescript/v2/FsUnwatchParams.ts @@ -7,6 +7,6 @@ */ export type FsUnwatchParams = { /** - * Watch identifier returned by `fs/watch`. + * Watch identifier previously provided to `fs/watch`. */ watchId: string, }; diff --git a/codex-rs/app-server-protocol/schema/typescript/v2/FsWatchParams.ts b/codex-rs/app-server-protocol/schema/typescript/v2/FsWatchParams.ts index d6d956b288..60755538cb 100644 --- a/codex-rs/app-server-protocol/schema/typescript/v2/FsWatchParams.ts +++ b/codex-rs/app-server-protocol/schema/typescript/v2/FsWatchParams.ts @@ -7,6 +7,10 @@ import type { AbsolutePathBuf } from "../AbsolutePathBuf"; * Start filesystem watch notifications for an absolute path. */ export type FsWatchParams = { +/** + * Connection-scoped watch identifier used for `fs/unwatch` and `fs/changed`. + */ +watchId: string, /** * Absolute file or directory path to watch. */ diff --git a/codex-rs/app-server-protocol/schema/typescript/v2/FsWatchResponse.ts b/codex-rs/app-server-protocol/schema/typescript/v2/FsWatchResponse.ts index 1927272821..b640306d82 100644 --- a/codex-rs/app-server-protocol/schema/typescript/v2/FsWatchResponse.ts +++ b/codex-rs/app-server-protocol/schema/typescript/v2/FsWatchResponse.ts @@ -4,13 +4,9 @@ import type { AbsolutePathBuf } from "../AbsolutePathBuf"; /** - * Created watch handle returned by `fs/watch`. + * Successful response for `fs/watch`. */ export type FsWatchResponse = { -/** - * Connection-scoped watch identifier used for `fs/unwatch` and `fs/changed`. - */ -watchId: string, /** * Canonicalized path associated with the watch. */ diff --git a/codex-rs/app-server-protocol/schema/typescript/v2/McpToolCallResult.ts b/codex-rs/app-server-protocol/schema/typescript/v2/McpToolCallResult.ts index f493a86094..916a5f5bb3 100644 --- a/codex-rs/app-server-protocol/schema/typescript/v2/McpToolCallResult.ts +++ b/codex-rs/app-server-protocol/schema/typescript/v2/McpToolCallResult.ts @@ -3,4 +3,4 @@ // This file was generated by [ts-rs](https://github.com/Aleph-Alpha/ts-rs). Do not edit this file manually. import type { JsonValue } from "../serde_json/JsonValue"; -export type McpToolCallResult = { content: Array, structuredContent: JsonValue | null, }; +export type McpToolCallResult = { content: Array, structuredContent: JsonValue | null, _meta: JsonValue | null, }; diff --git a/codex-rs/app-server-protocol/src/protocol/common.rs b/codex-rs/app-server-protocol/src/protocol/common.rs index 8fbff67893..3ac96fa0eb 100644 --- a/codex-rs/app-server-protocol/src/protocol/common.rs +++ b/codex-rs/app-server-protocol/src/protocol/common.rs @@ -1635,6 +1635,7 @@ mod tests { let request = ClientRequest::FsWatch { request_id: RequestId::Integer(10), params: v2::FsWatchParams { + watch_id: "watch-git".to_string(), path: absolute_path("tmp/repo/.git"), }, }; @@ -1643,6 +1644,7 @@ mod tests { "method": "fs/watch", "id": 10, "params": { + "watchId": "watch-git", "path": absolute_path_string("tmp/repo/.git") } }), diff --git a/codex-rs/app-server-protocol/src/protocol/thread_history.rs b/codex-rs/app-server-protocol/src/protocol/thread_history.rs index 10d55bb231..27d393629b 100644 --- a/codex-rs/app-server-protocol/src/protocol/thread_history.rs +++ b/codex-rs/app-server-protocol/src/protocol/thread_history.rs @@ -520,6 +520,7 @@ impl ThreadHistoryBuilder { Some(McpToolCallResult { content: value.content.clone(), structured_content: value.structured_content.clone(), + meta: value.meta.clone(), }), None, ), @@ -1167,6 +1168,7 @@ mod tests { use codex_protocol::items::TurnItem as CoreTurnItem; use codex_protocol::items::UserMessageItem as CoreUserMessageItem; use codex_protocol::items::build_hook_prompt_message; + use codex_protocol::mcp::CallToolResult; use codex_protocol::models::MessagePhase as CoreMessagePhase; use codex_protocol::models::WebSearchAction as CoreWebSearchAction; use codex_protocol::parse_command::ParsedCommand; @@ -1861,6 +1863,67 @@ mod tests { ); } + #[test] + fn reconstructs_mcp_tool_result_meta_from_persisted_completion_events() { + let events = vec![ + EventMsg::TurnStarted(TurnStartedEvent { + turn_id: "turn-1".into(), + started_at: None, + model_context_window: None, + collaboration_mode_kind: Default::default(), + }), + EventMsg::McpToolCallEnd(McpToolCallEndEvent { + call_id: "mcp-1".into(), + invocation: McpInvocation { + server: "docs".into(), + tool: "lookup".into(), + arguments: Some(serde_json::json!({"id":"123"})), + }, + duration: Duration::from_millis(8), + result: Ok(CallToolResult { + content: vec![serde_json::json!({ + "type": "text", + "text": "result" + })], + structured_content: Some(serde_json::json!({"id":"123"})), + is_error: Some(false), + meta: Some(serde_json::json!({ + "ui/resourceUri": "ui://widget/lookup.html" + })), + }), + }), + ]; + + let items = events + .into_iter() + .map(RolloutItem::EventMsg) + .collect::>(); + let turns = build_turns_from_rollout_items(&items); + assert_eq!(turns.len(), 1); + assert_eq!( + turns[0].items[0], + ThreadItem::McpToolCall { + id: "mcp-1".into(), + server: "docs".into(), + tool: "lookup".into(), + status: McpToolCallStatus::Completed, + arguments: serde_json::json!({"id":"123"}), + result: Some(McpToolCallResult { + content: vec![serde_json::json!({ + "type": "text", + "text": "result" + })], + structured_content: Some(serde_json::json!({"id":"123"})), + meta: Some(serde_json::json!({ + "ui/resourceUri": "ui://widget/lookup.html" + })), + }), + error: None, + duration_ms: Some(8), + } + ); + } + #[test] fn reconstructs_dynamic_tool_items_from_request_and_response_events() { let events = vec![ diff --git a/codex-rs/app-server-protocol/src/protocol/v2.rs b/codex-rs/app-server-protocol/src/protocol/v2.rs index 1457d9f377..60d2665bbe 100644 --- a/codex-rs/app-server-protocol/src/protocol/v2.rs +++ b/codex-rs/app-server-protocol/src/protocol/v2.rs @@ -2349,17 +2349,17 @@ pub struct FsCopyResponse {} #[serde(rename_all = "camelCase")] #[ts(export_to = "v2/")] pub struct FsWatchParams { + /// Connection-scoped watch identifier used for `fs/unwatch` and `fs/changed`. + pub watch_id: String, /// Absolute file or directory path to watch. pub path: AbsolutePathBuf, } -/// Created watch handle returned by `fs/watch`. +/// Successful response for `fs/watch`. #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq, JsonSchema, TS)] #[serde(rename_all = "camelCase")] #[ts(export_to = "v2/")] pub struct FsWatchResponse { - /// Connection-scoped watch identifier used for `fs/unwatch` and `fs/changed`. - pub watch_id: String, /// Canonicalized path associated with the watch. pub path: AbsolutePathBuf, } @@ -2369,7 +2369,7 @@ pub struct FsWatchResponse { #[serde(rename_all = "camelCase")] #[ts(export_to = "v2/")] pub struct FsUnwatchParams { - /// Watch identifier returned by `fs/watch`. + /// Watch identifier previously provided to `fs/watch`. pub watch_id: String, } @@ -2384,7 +2384,7 @@ pub struct FsUnwatchResponse {} #[serde(rename_all = "camelCase")] #[ts(export_to = "v2/")] pub struct FsChangedNotification { - /// Watch identifier returned by `fs/watch`. + /// Watch identifier previously provided to `fs/watch`. pub watch_id: String, /// File or directory paths associated with this event. pub changed_paths: Vec, @@ -5048,6 +5048,9 @@ pub struct McpToolCallResult { // representations). Using `JsonValue` keeps the payload wire-shaped and easy to export. pub content: Vec, pub structured_content: Option, + #[serde(rename = "_meta")] + #[ts(rename = "_meta")] + pub meta: Option, } #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema, TS)] diff --git a/codex-rs/app-server/README.md b/codex-rs/app-server/README.md index 13d3c3fe7a..b1652e5cc7 100644 --- a/codex-rs/app-server/README.md +++ b/codex-rs/app-server/README.md @@ -169,7 +169,7 @@ Example with notification opt-out: - `fs/readDirectory` — list direct child entries for an absolute directory path; each entry contains `fileName`, `isDirectory`, and `isFile`, and `fileName` is just the child name, not a path. - `fs/remove` — remove an absolute file or directory tree; `recursive` and `force` default to `true`. - `fs/copy` — copy between absolute paths; directory copies require `recursive: true`. -- `fs/watch` — subscribe this connection to filesystem change notifications for an absolute file or directory path; returns a `watchId` and canonicalized `path`. +- `fs/watch` — subscribe this connection to filesystem change notifications for an absolute file or directory path and caller-provided `watchId`; returns the canonicalized `path`. - `fs/unwatch` — stop sending notifications for a prior `fs/watch`; returns `{}`. - `fs/changed` — notification emitted when watched paths change, including the `watchId` and `changedPaths`. - `model/list` — list available models (set `includeHidden: true` to include entries with `hidden: true`), with reasoning effort options, optional legacy `upgrade` model ids, optional `upgradeInfo` metadata (`model`, `upgradeCopy`, `modelLink`, `migrationMarkdown`), and optional `availabilityNux` metadata. @@ -816,10 +816,10 @@ All filesystem paths in this section must be absolute. ```json { "method": "fs/watch", "id": 44, "params": { + "watchId": "0195ec6b-1d6f-7c2e-8c7a-56f2c4a8b9d1", "path": "/Users/me/project/.git/HEAD" } } { "id": 44, "result": { - "watchId": "0195ec6b-1d6f-7c2e-8c7a-56f2c4a8b9d1", "path": "/Users/me/project/.git/HEAD" } } { "method": "fs/changed", "params": { diff --git a/codex-rs/app-server/src/bespoke_event_handling.rs b/codex-rs/app-server/src/bespoke_event_handling.rs index 141ee78bdb..f5c60fe9dc 100644 --- a/codex-rs/app-server/src/bespoke_event_handling.rs +++ b/codex-rs/app-server/src/bespoke_event_handling.rs @@ -2843,6 +2843,7 @@ async fn construct_mcp_tool_call_end_notification( Some(McpToolCallResult { content: value.content.clone(), structured_content: value.structured_content.clone(), + meta: value.meta.clone(), }), None, ), @@ -4233,7 +4234,9 @@ mod tests { content: content.clone(), is_error: Some(false), structured_content: None, - meta: None, + meta: Some(serde_json::json!({ + "ui/resourceUri": "ui://widget/list-resources.html" + })), }; let end_event = McpToolCallEndEvent { @@ -4268,6 +4271,9 @@ mod tests { result: Some(McpToolCallResult { content, structured_content: None, + meta: Some(serde_json::json!({ + "ui/resourceUri": "ui://widget/list-resources.html" + })), }), error: None, duration_ms: Some(0), diff --git a/codex-rs/app-server/src/fs_watch.rs b/codex-rs/app-server/src/fs_watch.rs index c8214c2b8a..3a5b226248 100644 --- a/codex-rs/app-server/src/fs_watch.rs +++ b/codex-rs/app-server/src/fs_watch.rs @@ -1,3 +1,4 @@ +use crate::fs_api::invalid_request; use crate::outgoing_message::ConnectionId; use crate::outgoing_message::OutgoingMessageSender; use codex_app_server_protocol::FsChangedNotification; @@ -16,6 +17,7 @@ use codex_core::file_watcher::WatchRegistration; use codex_utils_absolute_path::AbsolutePathBuf; use std::collections::HashMap; use std::collections::HashSet; +use std::collections::hash_map::Entry; use std::hash::Hash; use std::path::PathBuf; use std::sync::Arc; @@ -26,7 +28,6 @@ use tokio::sync::mpsc; use tokio::sync::oneshot; use tokio::time::Instant; use tracing::warn; -use uuid::Uuid; const FS_CHANGED_NOTIFICATION_DEBOUNCE: Duration = Duration::from_millis(200); @@ -120,7 +121,11 @@ impl FsWatchManager { connection_id: ConnectionId, params: FsWatchParams, ) -> Result { - let watch_id = Uuid::now_v7().to_string(); + let watch_id = params.watch_id; + let watch_key = WatchKey { + connection_id, + watch_id: watch_id.clone(), + }; let outgoing = self.outgoing.clone(); let (subscriber, rx) = self.file_watcher.add_subscriber(); let watch_root = params.path.to_path_buf().clone(); @@ -130,17 +135,20 @@ impl FsWatchManager { }]); let (terminate_tx, terminate_rx) = oneshot::channel(); - self.state.lock().await.entries.insert( - WatchKey { - connection_id, - watch_id: watch_id.clone(), - }, - WatchEntry { - terminate_tx, - _subscriber: subscriber, - _registration: registration, - }, - ); + match self.state.lock().await.entries.entry(watch_key) { + Entry::Occupied(_) => { + return Err(invalid_request(format!( + "watchId already exists: {watch_id}" + ))); + } + Entry::Vacant(entry) => { + entry.insert(WatchEntry { + terminate_tx, + _subscriber: subscriber, + _registration: registration, + }); + } + } let task_watch_id = watch_id.clone(); tokio::spawn(async move { @@ -175,10 +183,7 @@ impl FsWatchManager { } }); - Ok(FsWatchResponse { - watch_id, - path: params.path, - }) + Ok(FsWatchResponse { path: params.path }) } pub(crate) async fn unwatch( @@ -216,7 +221,6 @@ mod tests { use codex_utils_absolute_path::AbsolutePathBuf; use pretty_assertions::assert_eq; use tempfile::TempDir; - use uuid::Version; fn absolute_path(path: PathBuf) -> AbsolutePathBuf { assert!( @@ -237,28 +241,33 @@ mod tests { } #[tokio::test] - async fn watch_returns_a_v7_id_and_tracks_the_owner_scoped_entry() { + async fn watch_uses_client_id_and_tracks_the_owner_scoped_entry() { let temp_dir = TempDir::new().expect("temp dir"); let head_path = temp_dir.path().join("HEAD"); std::fs::write(&head_path, "ref: refs/heads/main\n").expect("write HEAD"); let manager = manager_with_noop_watcher(); let path = absolute_path(head_path); + let watch_id = "watch-head".to_string(); let response = manager - .watch(ConnectionId(1), FsWatchParams { path: path.clone() }) + .watch( + ConnectionId(1), + FsWatchParams { + watch_id: watch_id.clone(), + path: path.clone(), + }, + ) .await .expect("watch should succeed"); assert_eq!(response.path, path); - let watch_id = Uuid::parse_str(&response.watch_id).expect("watch id should be a UUID"); - assert_eq!(watch_id.get_version(), Some(Version::SortRand)); let state = manager.state.lock().await; assert_eq!( state.entries.keys().cloned().collect::>(), HashSet::from([WatchKey { connection_id: ConnectionId(1), - watch_id: response.watch_id, + watch_id, }]) ); } @@ -270,10 +279,11 @@ mod tests { std::fs::write(&head_path, "ref: refs/heads/main\n").expect("write HEAD"); let manager = manager_with_noop_watcher(); - let response = manager + manager .watch( ConnectionId(1), FsWatchParams { + watch_id: "watch-head".to_string(), path: absolute_path(head_path), }, ) @@ -281,14 +291,14 @@ mod tests { .expect("watch should succeed"); let watch_key = WatchKey { connection_id: ConnectionId(1), - watch_id: response.watch_id.clone(), + watch_id: "watch-head".to_string(), }; manager .unwatch( ConnectionId(2), FsUnwatchParams { - watch_id: response.watch_id.clone(), + watch_id: "watch-head".to_string(), }, ) .await @@ -299,7 +309,7 @@ mod tests { .unwatch( ConnectionId(1), FsUnwatchParams { - watch_id: response.watch_id, + watch_id: "watch-head".to_string(), }, ) .await @@ -307,6 +317,41 @@ mod tests { assert!(!manager.state.lock().await.entries.contains_key(&watch_key)); } + #[tokio::test] + async fn watch_rejects_duplicate_id_for_the_same_connection() { + let temp_dir = TempDir::new().expect("temp dir"); + let head_path = temp_dir.path().join("HEAD"); + let fetch_head_path = temp_dir.path().join("FETCH_HEAD"); + std::fs::write(&head_path, "ref: refs/heads/main\n").expect("write HEAD"); + std::fs::write(&fetch_head_path, "old-fetch\n").expect("write FETCH_HEAD"); + + let manager = manager_with_noop_watcher(); + manager + .watch( + ConnectionId(1), + FsWatchParams { + watch_id: "watch-head".to_string(), + path: absolute_path(head_path), + }, + ) + .await + .expect("first watch should succeed"); + + let error = manager + .watch( + ConnectionId(1), + FsWatchParams { + watch_id: "watch-head".to_string(), + path: absolute_path(fetch_head_path), + }, + ) + .await + .expect_err("duplicate watch should fail"); + + assert_eq!(error.message, "watchId already exists: watch-head"); + assert_eq!(manager.state.lock().await.entries.len(), 1); + } + #[tokio::test] async fn connection_closed_removes_only_that_connections_watches() { let temp_dir = TempDir::new().expect("temp dir"); @@ -318,28 +363,31 @@ mod tests { std::fs::write(&packed_refs_path, "refs\n").expect("write packed-refs"); let manager = manager_with_noop_watcher(); - let response_1 = manager + let response = manager .watch( ConnectionId(1), FsWatchParams { - path: absolute_path(head_path), + watch_id: "watch-head".to_string(), + path: absolute_path(head_path.clone()), }, ) .await .expect("first watch should succeed"); - let response_2 = manager + manager .watch( ConnectionId(1), FsWatchParams { + watch_id: "watch-fetch-head".to_string(), path: absolute_path(fetch_head_path), }, ) .await .expect("second watch should succeed"); - let response_3 = manager + manager .watch( ConnectionId(2), FsWatchParams { + watch_id: "watch-packed-refs".to_string(), path: absolute_path(packed_refs_path), }, ) @@ -359,9 +407,9 @@ mod tests { .collect::>(), HashSet::from([WatchKey { connection_id: ConnectionId(2), - watch_id: response_3.watch_id, + watch_id: "watch-packed-refs".to_string(), }]) ); - assert_ne!(response_1.watch_id, response_2.watch_id); + assert_eq!(response.path, absolute_path(head_path)); } } diff --git a/codex-rs/app-server/src/in_process.rs b/codex-rs/app-server/src/in_process.rs index 5d8cf052c5..7bddd8a714 100644 --- a/codex-rs/app-server/src/in_process.rs +++ b/codex-rs/app-server/src/in_process.rs @@ -398,6 +398,7 @@ fn start_uninitialized(args: InProcessStartArgs) -> InProcessClientHandle { session_source: args.session_source, auth_manager, rpc_transport: AppServerRpcTransport::InProcess, + remote_control_handle: None, }); let mut thread_created_rx = processor.thread_created_receiver(); let mut session = ConnectionSessionState::default(); diff --git a/codex-rs/app-server/src/lib.rs b/codex-rs/app-server/src/lib.rs index 5e02cdb948..a837d9c754 100644 --- a/codex-rs/app-server/src/lib.rs +++ b/codex-rs/app-server/src/lib.rs @@ -564,25 +564,26 @@ pub async fn run_main_with_transport( let auth_manager = AuthManager::shared_from_config(&config, /*enable_codex_api_key_env*/ false); - if config.features.enabled(Feature::RemoteControl) { - let accept_handle = start_remote_control( - config.chatgpt_base_url.clone(), - state_db.clone(), - auth_manager.clone(), - transport_event_tx.clone(), - transport_shutdown_token.clone(), - app_server_client_name_rx, - ) - .await?; - transport_accept_handles.push(accept_handle); - } - if transport_accept_handles.is_empty() { + let remote_control_enabled = config.features.enabled(Feature::RemoteControl); + if transport_accept_handles.is_empty() && !remote_control_enabled { return Err(std::io::Error::new( ErrorKind::InvalidInput, "no transport configured; use --listen or enable remote control", )); } + let (remote_control_accept_handle, remote_control_handle) = start_remote_control( + config.chatgpt_base_url.clone(), + state_db.clone(), + auth_manager.clone(), + transport_event_tx.clone(), + transport_shutdown_token.clone(), + app_server_client_name_rx, + remote_control_enabled, + ) + .await?; + transport_accept_handles.push(remote_control_accept_handle); + let outbound_handle = tokio::spawn(async move { let mut outbound_connections = HashMap::::new(); loop { @@ -659,6 +660,7 @@ pub async fn run_main_with_transport( session_source, auth_manager, rpc_transport: analytics_rpc_transport(transport), + remote_control_handle: Some(remote_control_handle), }); let mut thread_created_rx = processor.thread_created_receiver(); let mut running_turn_count_rx = processor.subscribe_running_assistant_turn_count(); diff --git a/codex-rs/app-server/src/message_processor.rs b/codex-rs/app-server/src/message_processor.rs index 15df5a2a54..fbc1bd6d10 100644 --- a/codex-rs/app-server/src/message_processor.rs +++ b/codex-rs/app-server/src/message_processor.rs @@ -19,6 +19,7 @@ use crate::outgoing_message::ConnectionRequestId; use crate::outgoing_message::OutgoingMessageSender; use crate::outgoing_message::RequestContext; use crate::transport::AppServerTransport; +use crate::transport::RemoteControlHandle; use async_trait::async_trait; use codex_analytics::AnalyticsEventsClient; use codex_analytics::AppServerRpcTransport; @@ -170,6 +171,7 @@ pub(crate) struct MessageProcessor { config: Arc, config_warnings: Arc>, rpc_transport: AppServerRpcTransport, + remote_control_handle: Option, } #[derive(Clone, Debug, Default)] @@ -195,6 +197,7 @@ pub(crate) struct MessageProcessorArgs { pub(crate) session_source: SessionSource, pub(crate) auth_manager: Arc, pub(crate) rpc_transport: AppServerRpcTransport, + pub(crate) remote_control_handle: Option, } impl MessageProcessor { @@ -215,6 +218,7 @@ impl MessageProcessor { session_source, auth_manager, rpc_transport, + remote_control_handle, } = args; auth_manager.set_external_auth(Arc::new(ExternalAuthRefreshBridge { outgoing: outgoing.clone(), @@ -285,6 +289,7 @@ impl MessageProcessor { config, config_warnings: Arc::new(config_warnings), rpc_transport, + remote_control_handle, } } @@ -969,13 +974,36 @@ impl MessageProcessor { ) { match result { Ok(response) => { - self.codex_message_processor.handle_config_mutation(); + self.handle_config_mutation().await; self.outgoing.send_response(request_id, response).await; } Err(error) => self.outgoing.send_error(request_id, error).await, } } + async fn handle_config_mutation(&self) { + self.codex_message_processor.handle_config_mutation(); + let Some(remote_control_handle) = &self.remote_control_handle else { + return; + }; + + match self + .config_api + .load_latest_config(/*fallback_cwd*/ None) + .await + { + Ok(config) => { + remote_control_handle.set_enabled(config.features.enabled(Feature::RemoteControl)); + } + Err(error) => { + tracing::warn!( + "failed to load config for remote control enablement refresh after config mutation: {}", + error.message + ); + } + } + } + async fn handle_config_requirements_read(&self, request_id: ConnectionRequestId) { match self.config_api.config_requirements_read().await { Ok(response) => self.outgoing.send_response(request_id, response).await, diff --git a/codex-rs/app-server/src/message_processor/tracing_tests.rs b/codex-rs/app-server/src/message_processor/tracing_tests.rs index 2e8781606f..ef88364a3a 100644 --- a/codex-rs/app-server/src/message_processor/tracing_tests.rs +++ b/codex-rs/app-server/src/message_processor/tracing_tests.rs @@ -251,6 +251,7 @@ fn build_test_processor( session_source: SessionSource::VSCode, auth_manager, rpc_transport: AppServerRpcTransport::Stdio, + remote_control_handle: None, }); (processor, outgoing_rx) } diff --git a/codex-rs/app-server/src/transport/mod.rs b/codex-rs/app-server/src/transport/mod.rs index 0f1b8bec8d..50508952f4 100644 --- a/codex-rs/app-server/src/transport/mod.rs +++ b/codex-rs/app-server/src/transport/mod.rs @@ -33,6 +33,7 @@ mod remote_control; mod stdio; mod websocket; +pub(crate) use remote_control::RemoteControlHandle; pub(crate) use remote_control::start_remote_control; pub(crate) use stdio::start_stdio_connection; pub(crate) use websocket::start_websocket_acceptor; diff --git a/codex-rs/app-server/src/transport/remote_control/mod.rs b/codex-rs/app-server/src/transport/remote_control/mod.rs index 6d9d65e8a3..1ea89bb643 100644 --- a/codex-rs/app-server/src/transport/remote_control/mod.rs +++ b/codex-rs/app-server/src/transport/remote_control/mod.rs @@ -19,6 +19,7 @@ use std::io; use std::sync::Arc; use tokio::sync::mpsc; use tokio::sync::oneshot; +use tokio::sync::watch; use tokio::task::JoinHandle; use tokio_util::sync::CancellationToken; @@ -29,6 +30,21 @@ pub(super) struct QueuedServerEnvelope { pub(super) write_complete_tx: Option>, } +#[derive(Clone)] +pub(crate) struct RemoteControlHandle { + enabled_tx: Arc>, +} + +impl RemoteControlHandle { + pub(crate) fn set_enabled(&self, enabled: bool) { + self.enabled_tx.send_if_modified(|state| { + let changed = *state != enabled; + *state = enabled; + changed + }); + } +} + pub(crate) async fn start_remote_control( remote_control_url: String, state_db: Option>, @@ -36,21 +52,38 @@ pub(crate) async fn start_remote_control( transport_event_tx: mpsc::Sender, shutdown_token: CancellationToken, app_server_client_name_rx: Option>, -) -> io::Result> { - let remote_control_target = normalize_remote_control_url(&remote_control_url)?; - validate_remote_control_auth(&auth_manager).await?; + initial_enabled: bool, +) -> io::Result<(JoinHandle<()>, RemoteControlHandle)> { + let remote_control_target = if initial_enabled { + Some(normalize_remote_control_url(&remote_control_url)?) + } else { + None + }; + if initial_enabled { + validate_remote_control_auth(&auth_manager).await?; + } - Ok(tokio::spawn(async move { + let (enabled_tx, enabled_rx) = watch::channel(initial_enabled); + let join_handle = tokio::spawn(async move { RemoteControlWebsocket::new( + remote_control_url, remote_control_target, state_db, auth_manager, transport_event_tx, shutdown_token, + enabled_rx, ) .run(app_server_client_name_rx) .await; - })) + }); + + Ok(( + join_handle, + RemoteControlHandle { + enabled_tx: Arc::new(enabled_tx), + }, + )) } pub(crate) async fn validate_remote_control_auth( diff --git a/codex-rs/app-server/src/transport/remote_control/tests.rs b/codex-rs/app-server/src/transport/remote_control/tests.rs index b403fac484..9d430ccfb3 100644 --- a/codex-rs/app-server/src/transport/remote_control/tests.rs +++ b/codex-rs/app-server/src/transport/remote_control/tests.rs @@ -125,13 +125,14 @@ async fn remote_control_transport_manages_virtual_clients_and_routes_messages() let (transport_event_tx, mut transport_event_rx) = mpsc::channel::(CHANNEL_CAPACITY); let shutdown_token = CancellationToken::new(); - let remote_handle = start_remote_control( + let (remote_task, _remote_handle) = start_remote_control( remote_control_url, Some(remote_control_state_runtime(&codex_home).await), remote_control_auth_manager(), transport_event_tx, shutdown_token.clone(), /*app_server_client_name_rx*/ None, + /*initial_enabled*/ true, ) .await .expect("remote control should start"); @@ -376,7 +377,7 @@ async fn remote_control_transport_manages_virtual_clients_and_routes_messages() ); shutdown_token.cancel(); - let _ = remote_handle.await; + let _ = remote_task.await; } #[tokio::test] @@ -389,13 +390,14 @@ async fn remote_control_transport_reconnects_after_disconnect() { let (transport_event_tx, mut transport_event_rx) = mpsc::channel::(CHANNEL_CAPACITY); let shutdown_token = CancellationToken::new(); - let remote_handle = start_remote_control( + let (remote_task, _remote_handle) = start_remote_control( remote_control_url, Some(remote_control_state_runtime(&codex_home).await), remote_control_auth_manager(), transport_event_tx, shutdown_token.clone(), /*app_server_client_name_rx*/ None, + /*initial_enabled*/ true, ) .await .expect("remote control should start"); @@ -452,7 +454,84 @@ async fn remote_control_transport_reconnects_after_disconnect() { } shutdown_token.cancel(); - let _ = remote_handle.await; + let _ = remote_task.await; +} + +#[tokio::test] +async fn remote_control_start_allows_remote_control_invalid_url_when_disabled() { + let (transport_event_tx, _transport_event_rx) = + mpsc::channel::(CHANNEL_CAPACITY); + let shutdown_token = CancellationToken::new(); + let (remote_task, _remote_handle) = start_remote_control( + "https://internal.example.com/backend-api/".to_string(), + /*state_db*/ None, + remote_control_auth_manager(), + transport_event_tx, + shutdown_token.clone(), + /*app_server_client_name_rx*/ None, + /*initial_enabled*/ false, + ) + .await + .expect("disabled remote control should not validate the URL at startup"); + + shutdown_token.cancel(); + timeout(Duration::from_secs(1), remote_task) + .await + .expect("remote control task should stop") + .expect("remote control task should join"); +} + +#[tokio::test] +async fn remote_control_handle_set_enabled_stops_and_restarts_connections() { + let listener = TcpListener::bind("127.0.0.1:0") + .await + .expect("listener should bind"); + let remote_control_url = remote_control_url_for_listener(&listener); + let codex_home = TempDir::new().expect("temp dir should create"); + let (transport_event_tx, _transport_event_rx) = + mpsc::channel::(CHANNEL_CAPACITY); + let shutdown_token = CancellationToken::new(); + let (remote_task, remote_handle) = start_remote_control( + remote_control_url, + Some(remote_control_state_runtime(&codex_home).await), + remote_control_auth_manager(), + transport_event_tx, + shutdown_token.clone(), + /*app_server_client_name_rx*/ None, + /*initial_enabled*/ true, + ) + .await + .expect("remote control should start"); + + let enroll_request = accept_http_request(&listener).await; + assert_eq!( + enroll_request.request_line, + "POST /backend-api/wham/remote/control/server/enroll HTTP/1.1" + ); + respond_with_json( + enroll_request.stream, + json!({ "server_id": "srv_e_test", "environment_id": "env_test" }), + ) + .await; + let mut first_websocket = accept_remote_control_connection(&listener).await; + + remote_handle.set_enabled(/*enabled*/ false); + timeout(Duration::from_secs(1), first_websocket.next()) + .await + .expect("disabling remote control should close the websocket"); + timeout(Duration::from_millis(100), listener.accept()) + .await + .expect_err("disabled remote control should not reconnect"); + + remote_handle.set_enabled(/*enabled*/ true); + let mut second_websocket = accept_remote_control_connection(&listener).await; + second_websocket + .close(None) + .await + .expect("second websocket should close"); + + shutdown_token.cancel(); + let _ = remote_task.await; } #[tokio::test] @@ -465,13 +544,14 @@ async fn remote_control_transport_clears_outgoing_buffer_when_backend_acks() { let (transport_event_tx, mut transport_event_rx) = mpsc::channel::(CHANNEL_CAPACITY); let shutdown_token = CancellationToken::new(); - let remote_handle = start_remote_control( + let (remote_task, _remote_handle) = start_remote_control( remote_control_url, Some(remote_control_state_runtime(&codex_home).await), remote_control_auth_manager(), transport_event_tx, shutdown_token.clone(), /*app_server_client_name_rx*/ None, + /*initial_enabled*/ true, ) .await .expect("remote control should start"); @@ -617,7 +697,7 @@ async fn remote_control_transport_clears_outgoing_buffer_when_backend_acks() { ); shutdown_token.cancel(); - let _ = remote_handle.await; + let _ = remote_task.await; } #[tokio::test] @@ -631,13 +711,14 @@ async fn remote_control_http_mode_enrolls_before_connecting() { mpsc::channel::(CHANNEL_CAPACITY); let expected_server_name = gethostname().to_string_lossy().trim().to_string(); let shutdown_token = CancellationToken::new(); - let remote_handle = start_remote_control( + let (remote_task, _remote_handle) = start_remote_control( remote_control_url, Some(remote_control_state_runtime(&codex_home).await), remote_control_auth_manager(), transport_event_tx, shutdown_token.clone(), /*app_server_client_name_rx*/ None, + /*initial_enabled*/ true, ) .await .expect("remote control should start"); @@ -815,7 +896,7 @@ async fn remote_control_http_mode_enrolls_before_connecting() { ); shutdown_token.cancel(); - let _ = remote_handle.await; + let _ = remote_task.await; } #[tokio::test] @@ -847,13 +928,14 @@ async fn remote_control_http_mode_reuses_persisted_enrollment_before_reenrolling let (transport_event_tx, _transport_event_rx) = mpsc::channel::(CHANNEL_CAPACITY); let shutdown_token = CancellationToken::new(); - let remote_handle = start_remote_control( + let (remote_task, _remote_handle) = start_remote_control( remote_control_url, Some(state_db.clone()), remote_control_auth_manager_with_home(&codex_home), transport_event_tx, shutdown_token.clone(), /*app_server_client_name_rx*/ None, + /*initial_enabled*/ true, ) .await .expect("remote control should start"); @@ -879,7 +961,7 @@ async fn remote_control_http_mode_reuses_persisted_enrollment_before_reenrolling ); shutdown_token.cancel(); - let _ = remote_handle.await; + let _ = remote_task.await; } #[tokio::test] @@ -913,13 +995,14 @@ async fn remote_control_stdio_mode_waits_for_client_name_before_connecting() { mpsc::channel::(CHANNEL_CAPACITY); let (app_server_client_name_tx, app_server_client_name_rx) = oneshot::channel::(); let shutdown_token = CancellationToken::new(); - let remote_handle = start_remote_control( + let (remote_task, _remote_handle) = start_remote_control( remote_control_url, Some(state_db.clone()), remote_control_auth_manager_with_home(&codex_home), transport_event_tx, shutdown_token.clone(), Some(app_server_client_name_rx), + /*initial_enabled*/ true, ) .await .expect("remote control should start"); @@ -936,7 +1019,7 @@ async fn remote_control_stdio_mode_waits_for_client_name_before_connecting() { ); shutdown_token.cancel(); - let _ = remote_handle.await; + let _ = remote_task.await; } #[tokio::test] @@ -969,13 +1052,14 @@ async fn remote_control_waits_for_account_id_before_enrolling() { let (transport_event_tx, _transport_event_rx) = mpsc::channel::(CHANNEL_CAPACITY); let shutdown_token = CancellationToken::new(); - let remote_handle = start_remote_control( + let (remote_task, _remote_handle) = start_remote_control( remote_control_url, Some(state_db.clone()), auth_manager, transport_event_tx, shutdown_token.clone(), /*app_server_client_name_rx*/ None, + /*initial_enabled*/ true, ) .await .expect("remote control should start before account id is available"); @@ -1012,7 +1096,7 @@ async fn remote_control_waits_for_account_id_before_enrolling() { ); shutdown_token.cancel(); - let _ = remote_handle.await; + let _ = remote_task.await; } #[tokio::test] @@ -1051,13 +1135,14 @@ async fn remote_control_http_mode_clears_stale_persisted_enrollment_after_404() let (transport_event_tx, _transport_event_rx) = mpsc::channel::(CHANNEL_CAPACITY); let shutdown_token = CancellationToken::new(); - let remote_handle = start_remote_control( + let (remote_task, _remote_handle) = start_remote_control( remote_control_url, Some(state_db.clone()), remote_control_auth_manager_with_home(&codex_home), transport_event_tx, shutdown_token.clone(), /*app_server_client_name_rx*/ None, + /*initial_enabled*/ true, ) .await .expect("remote control should start"); @@ -1104,7 +1189,7 @@ async fn remote_control_http_mode_clears_stale_persisted_enrollment_after_404() ); shutdown_token.cancel(); - let _ = remote_handle.await; + let _ = remote_task.await; } #[derive(Debug)] diff --git a/codex-rs/app-server/src/transport/remote_control/websocket.rs b/codex-rs/app-server/src/transport/remote_control/websocket.rs index 3f42a7bbbe..a0387ef6c3 100644 --- a/codex-rs/app-server/src/transport/remote_control/websocket.rs +++ b/codex-rs/app-server/src/transport/remote_control/websocket.rs @@ -111,7 +111,8 @@ struct WebsocketState { } pub(crate) struct RemoteControlWebsocket { - remote_control_target: RemoteControlTarget, + remote_control_url: String, + remote_control_target: Option, state_db: Option>, auth_manager: Arc, shutdown_token: CancellationToken, @@ -122,15 +123,24 @@ pub(crate) struct RemoteControlWebsocket { state: Arc>, server_event_rx: Arc>>, used_rx: watch::Receiver, + enabled_rx: watch::Receiver, +} + +enum ConnectOutcome { + Connected(Box>>), + Disabled, + Shutdown, } impl RemoteControlWebsocket { pub(crate) fn new( - remote_control_target: RemoteControlTarget, + remote_control_url: String, + remote_control_target: Option, state_db: Option>, auth_manager: Arc, transport_event_tx: mpsc::Sender, shutdown_token: CancellationToken, + enabled_rx: watch::Receiver, ) -> Self { let shutdown_token = shutdown_token.child_token(); let (server_event_tx, server_event_rx) = mpsc::channel(super::CHANNEL_CAPACITY); @@ -140,6 +150,7 @@ impl RemoteControlWebsocket { let auth_recovery = auth_manager.unauthorized_recovery(); Self { + remote_control_url, remote_control_target, state_db, auth_manager, @@ -155,6 +166,7 @@ impl RemoteControlWebsocket { })), server_event_rx: Arc::new(Mutex::new(server_event_rx)), used_rx, + enabled_rx, } } @@ -174,13 +186,18 @@ impl RemoteControlWebsocket { }; loop { + if !self.wait_until_enabled().await { + break; + } + let shutdown_token = self.shutdown_token.child_token(); let websocket_connection = match self .connect(&shutdown_token, app_server_client_name.as_deref()) .await { - Some(websocket_connection) => websocket_connection, - None => break, + ConnectOutcome::Connected(websocket_connection) => *websocket_connection, + ConnectOutcome::Disabled => continue, + ConnectOutcome::Shutdown => break, }; self.run_connection(websocket_connection, shutdown_token) @@ -208,50 +225,93 @@ impl RemoteControlWebsocket { } } + async fn wait_until_enabled(&mut self) -> bool { + tokio::select! { + _ = self.shutdown_token.cancelled() => false, + enabled = self.enabled_rx.wait_for(|enabled| *enabled) => enabled.is_ok(), + } + } + async fn connect( &mut self, shutdown_token: &CancellationToken, app_server_client_name: Option<&str>, - ) -> Option>> { + ) -> ConnectOutcome { + let remote_control_target = match self.remote_control_target.as_ref() { + Some(remote_control_target) => remote_control_target.clone(), + None => match super::protocol::normalize_remote_control_url(&self.remote_control_url) { + Ok(remote_control_target) => { + self.remote_control_target = Some(remote_control_target.clone()); + remote_control_target + } + Err(err) => { + warn!("remote control is enabled but the URL is invalid: {err}"); + tokio::select! { + _ = shutdown_token.cancelled() => return ConnectOutcome::Shutdown, + changed = self.enabled_rx.wait_for(|enabled| !*enabled) => { + if changed.is_err() { + return ConnectOutcome::Shutdown; + } + return ConnectOutcome::Disabled; + } + } + } + }, + }; + loop { let subscribe_cursor = self.state.lock().await.subscribe_cursor.clone(); - tokio::select! { - _ = shutdown_token.cancelled() => return None, + let connect_result = tokio::select! { + _ = shutdown_token.cancelled() => return ConnectOutcome::Shutdown, + changed = self.enabled_rx.wait_for(|enabled| !*enabled) => { + if changed.is_err() { + return ConnectOutcome::Shutdown; + } + return ConnectOutcome::Disabled; + } connect_result = connect_remote_control_websocket( - &self.remote_control_target, + &remote_control_target, self.state_db.as_deref(), &self.auth_manager, &mut self.auth_recovery, &mut self.enrollment, subscribe_cursor.as_deref(), app_server_client_name, - ) => { - match connect_result { - Ok((websocket_connection, response)) => { - self.reconnect_attempt = 0; - self.auth_recovery = self.auth_manager.unauthorized_recovery(); - info!( - "connected to app-server remote control websocket: {}, {}", - self.remote_control_target.websocket_url, - format_headers(response.headers()) - ); - return Some(websocket_connection); - } - Err(err) => { - let reconnect_delay = if err.kind() == ErrorKind::WouldBlock { - info!("{err}"); - REMOTE_CONTROL_ACCOUNT_ID_RETRY_INTERVAL - } else { - warn!("{err}"); - let reconnect_delay = backoff(self.reconnect_attempt); - self.reconnect_attempt += 1; - reconnect_delay - }; - tokio::select! { - _ = shutdown_token.cancelled() => return None, - _ = tokio::time::sleep(reconnect_delay) => {} + ) => connect_result, + }; + + match connect_result { + Ok((websocket_connection, response)) => { + self.reconnect_attempt = 0; + self.auth_recovery = self.auth_manager.unauthorized_recovery(); + info!( + "connected to app-server remote control websocket: {}, {}", + remote_control_target.websocket_url, + format_headers(response.headers()) + ); + return ConnectOutcome::Connected(Box::new(websocket_connection)); + } + Err(err) => { + let reconnect_delay = if err.kind() == ErrorKind::WouldBlock { + REMOTE_CONTROL_ACCOUNT_ID_RETRY_INTERVAL + } else { + warn!( + "failed to connect to app-server remote control websocket: {}, err: {}", + remote_control_target.websocket_url, err + ); + let reconnect_delay = backoff(self.reconnect_attempt); + self.reconnect_attempt += 1; + reconnect_delay + }; + tokio::select! { + _ = shutdown_token.cancelled() => return ConnectOutcome::Shutdown, + changed = self.enabled_rx.wait_for(|enabled| !*enabled) => { + if changed.is_err() { + return ConnectOutcome::Shutdown; } + return ConnectOutcome::Disabled; } + _ = tokio::time::sleep(reconnect_delay) => {} } } } @@ -282,8 +342,10 @@ impl RemoteControlWebsocket { shutdown_token.clone(), )); + let mut enabled_rx = self.enabled_rx.clone(); tokio::select! { _ = shutdown_token.cancelled() => {} + _ = enabled_rx.wait_for(|enabled| !*enabled) => shutdown_token.cancel(), _ = join_set.join_next() => shutdown_token.cancel(), } @@ -1124,15 +1186,18 @@ mod tests { let (transport_event_tx, transport_event_rx) = mpsc::channel(1); drop(transport_event_rx); let shutdown_token = CancellationToken::new(); + let (_enabled_tx, enabled_rx) = watch::channel(true); let websocket_task = tokio::spawn({ let shutdown_token = shutdown_token.clone(); async move { RemoteControlWebsocket::new( - remote_control_target, + remote_control_url, + Some(remote_control_target), /*state_db*/ None, remote_control_auth_manager(), transport_event_tx, shutdown_token, + enabled_rx, ) .run(/*app_server_client_name_rx*/ None) .await diff --git a/codex-rs/app-server/tests/suite/v2/fs.rs b/codex-rs/app-server/tests/suite/v2/fs.rs index e113904809..3fd5d62c89 100644 --- a/codex-rs/app-server/tests/suite/v2/fs.rs +++ b/codex-rs/app-server/tests/suite/v2/fs.rs @@ -21,8 +21,6 @@ use std::path::PathBuf; use tempfile::TempDir; use tokio::time::Duration; use tokio::time::timeout; -use uuid::Uuid; -use uuid::Version; #[cfg(unix)] use std::os::unix::fs::symlink; @@ -628,8 +626,10 @@ async fn fs_watch_directory_reports_changed_child_paths_and_unwatch_stops_notifi std::fs::write(&fetch_head, "old\n")?; let mut mcp = initialized_mcp(&codex_home).await?; + let watch_id = "watch-git-dir".to_string(); let watch_request_id = mcp .send_fs_watch_request(codex_app_server_protocol::FsWatchParams { + watch_id: watch_id.clone(), path: absolute_path(git_dir.clone()), }) .await?; @@ -641,8 +641,6 @@ async fn fs_watch_directory_reports_changed_child_paths_and_unwatch_stops_notifi .await??, )?; assert_eq!(watch_response.path, absolute_path(git_dir.clone())); - let watch_id = Uuid::parse_str(&watch_response.watch_id)?; - assert_eq!(watch_id.get_version(), Some(Version::SortRand)); std::fs::write(&fetch_head, "updated\n")?; @@ -650,7 +648,7 @@ async fn fs_watch_directory_reports_changed_child_paths_and_unwatch_stops_notifi // Keep validating notification shape when the backend does emit, but do not // fail the whole suite if no OS event arrives. if let Some(changed) = maybe_fs_changed_notification(&mut mcp).await? { - assert_eq!(changed.watch_id, watch_response.watch_id.clone()); + assert_eq!(changed.watch_id, watch_id.clone()); assert_eq!( changed.changed_paths, vec![absolute_path(fetch_head.clone())] @@ -665,9 +663,7 @@ async fn fs_watch_directory_reports_changed_child_paths_and_unwatch_stops_notifi {} let unwatch_request_id = mcp - .send_fs_unwatch_request(FsUnwatchParams { - watch_id: watch_response.watch_id, - }) + .send_fs_unwatch_request(FsUnwatchParams { watch_id }) .await?; timeout( DEFAULT_READ_TIMEOUT, @@ -698,8 +694,10 @@ async fn fs_watch_file_reports_atomic_replace_events() -> Result<()> { std::fs::write(&head_path, "ref: refs/heads/main\n")?; let mut mcp = initialized_mcp(&codex_home).await?; + let watch_id = "watch-head".to_string(); let watch_request_id = mcp .send_fs_watch_request(codex_app_server_protocol::FsWatchParams { + watch_id: watch_id.clone(), path: absolute_path(head_path.clone()), }) .await?; @@ -718,7 +716,7 @@ async fn fs_watch_file_reports_atomic_replace_events() -> Result<()> { assert_eq!( changed, FsChangedNotification { - watch_id: watch_response.watch_id, + watch_id, changed_paths: vec![absolute_path(head_path.clone())], } ); @@ -735,8 +733,10 @@ async fn fs_watch_allows_missing_file_targets() -> Result<()> { std::fs::create_dir_all(&git_dir)?; let mut mcp = initialized_mcp(&codex_home).await?; + let watch_id = "watch-fetch-head".to_string(); let watch_request_id = mcp .send_fs_watch_request(codex_app_server_protocol::FsWatchParams { + watch_id: watch_id.clone(), path: absolute_path(fetch_head.clone()), }) .await?; @@ -755,7 +755,7 @@ async fn fs_watch_allows_missing_file_targets() -> Result<()> { assert_eq!( changed, FsChangedNotification { - watch_id: watch_response.watch_id, + watch_id, changed_paths: vec![absolute_path(fetch_head.clone())], } ); @@ -770,7 +770,10 @@ async fn fs_watch_rejects_relative_paths() -> Result<()> { let mut mcp = initialized_mcp(&codex_home).await?; let watch_id = mcp - .send_raw_request("fs/watch", Some(json!({ "path": "relative-path" }))) + .send_raw_request( + "fs/watch", + Some(json!({ "watchId": "watch-relative", "path": "relative-path" })), + ) .await?; expect_error_message( &mut mcp, diff --git a/codex-rs/apply-patch/Cargo.toml b/codex-rs/apply-patch/Cargo.toml index 2fad46c2af..ba4fa5e48e 100644 --- a/codex-rs/apply-patch/Cargo.toml +++ b/codex-rs/apply-patch/Cargo.toml @@ -17,8 +17,11 @@ workspace = true [dependencies] anyhow = { workspace = true } +codex-exec-server = { workspace = true } +codex-utils-absolute-path = { workspace = true } similar = { workspace = true } thiserror = { workspace = true } +tokio = { workspace = true, features = ["macros", "rt"] } tree-sitter = { workspace = true } tree-sitter-bash = { workspace = true } diff --git a/codex-rs/apply-patch/src/invocation.rs b/codex-rs/apply-patch/src/invocation.rs index 519671798d..3b0db2fa9c 100644 --- a/codex-rs/apply-patch/src/invocation.rs +++ b/codex-rs/apply-patch/src/invocation.rs @@ -2,6 +2,8 @@ use std::collections::HashMap; use std::path::Path; use std::sync::LazyLock; +use codex_exec_server::ExecutorFileSystem; +use codex_utils_absolute_path::AbsolutePathBuf; use tree_sitter::Parser; use tree_sitter::Query; use tree_sitter::QueryCursor; @@ -129,7 +131,11 @@ pub fn maybe_parse_apply_patch(argv: &[String]) -> MaybeApplyPatch { /// cwd must be an absolute path so that we can resolve relative paths in the /// patch. -pub fn maybe_parse_apply_patch_verified(argv: &[String], cwd: &Path) -> MaybeApplyPatchVerified { +pub async fn maybe_parse_apply_patch_verified( + argv: &[String], + cwd: &AbsolutePathBuf, + fs: &dyn ExecutorFileSystem, +) -> MaybeApplyPatchVerified { // Detect a raw patch body passed directly as the command or as the body of a shell // script. In these cases, report an explicit error rather than applying the patch. if let [body] = argv @@ -151,24 +157,20 @@ pub fn maybe_parse_apply_patch_verified(argv: &[String], cwd: &Path) -> MaybeApp }) => { let effective_cwd = workdir .as_ref() - .map(|dir| { - let path = Path::new(dir); - if path.is_absolute() { - path.to_path_buf() - } else { - cwd.join(path) - } - }) - .unwrap_or_else(|| cwd.to_path_buf()); + .map(|dir| cwd.join(Path::new(dir))) + .unwrap_or_else(|| cwd.clone()); let mut changes = HashMap::new(); for hunk in hunks { let path = hunk.resolve_path(&effective_cwd); match hunk { Hunk::AddFile { contents, .. } => { - changes.insert(path, ApplyPatchFileChange::Add { content: contents }); + changes.insert( + path.into_path_buf(), + ApplyPatchFileChange::Add { content: contents }, + ); } Hunk::DeleteFile { .. } => { - let content = match std::fs::read_to_string(&path) { + let content = match fs.read_file_text(&path).await { Ok(content) => content, Err(e) => { return MaybeApplyPatchVerified::CorrectnessError( @@ -179,7 +181,10 @@ pub fn maybe_parse_apply_patch_verified(argv: &[String], cwd: &Path) -> MaybeApp ); } }; - changes.insert(path, ApplyPatchFileChange::Delete { content }); + changes.insert( + path.into_path_buf(), + ApplyPatchFileChange::Delete { content }, + ); } Hunk::UpdateFile { move_path, chunks, .. @@ -187,17 +192,17 @@ pub fn maybe_parse_apply_patch_verified(argv: &[String], cwd: &Path) -> MaybeApp let ApplyPatchFileUpdate { unified_diff, content: contents, - } = match unified_diff_from_chunks(&path, &chunks) { + } = match unified_diff_from_chunks(&path, &chunks, fs).await { Ok(diff) => diff, Err(e) => { return MaybeApplyPatchVerified::CorrectnessError(e); } }; changes.insert( - path, + path.into_path_buf(), ApplyPatchFileChange::Update { unified_diff, - move_path: move_path.map(|p| effective_cwd.join(p)), + move_path: move_path.map(|p| effective_cwd.join(p).into_path_buf()), new_content: contents, }, ); @@ -371,7 +376,10 @@ fn extract_apply_patch_from_bash( #[cfg(test)] mod tests { use super::*; + use crate::unified_diff_from_chunks; use assert_matches::assert_matches; + use codex_exec_server::LOCAL_FS; + use codex_utils_absolute_path::test_support::PathExt; use pretty_assertions::assert_eq; use std::fs; use std::path::PathBuf; @@ -450,30 +458,40 @@ mod tests { ); } - #[test] - fn test_implicit_patch_single_arg_is_error() { + #[tokio::test] + async fn test_implicit_patch_single_arg_is_error() { let patch = "*** Begin Patch\n*** Add File: foo\n+hi\n*** End Patch".to_string(); let args = vec![patch]; let dir = tempdir().unwrap(); assert_matches!( - maybe_parse_apply_patch_verified(&args, dir.path()), + maybe_parse_apply_patch_verified( + &args, + &AbsolutePathBuf::from_absolute_path(dir.path()).unwrap(), + LOCAL_FS.as_ref() + ) + .await, MaybeApplyPatchVerified::CorrectnessError(ApplyPatchError::ImplicitInvocation) ); } - #[test] - fn test_implicit_patch_bash_script_is_error() { + #[tokio::test] + async fn test_implicit_patch_bash_script_is_error() { let script = "*** Begin Patch\n*** Add File: foo\n+hi\n*** End Patch"; let args = args_bash(script); let dir = tempdir().unwrap(); assert_matches!( - maybe_parse_apply_patch_verified(&args, dir.path()), + maybe_parse_apply_patch_verified( + &args, + &AbsolutePathBuf::from_absolute_path(dir.path()).unwrap(), + LOCAL_FS.as_ref() + ) + .await, MaybeApplyPatchVerified::CorrectnessError(ApplyPatchError::ImplicitInvocation) ); } - #[test] - fn test_literal() { + #[tokio::test] + async fn test_literal() { let args = strs_to_strings(&[ "apply_patch", r#"*** Begin Patch @@ -497,8 +515,8 @@ mod tests { } } - #[test] - fn test_literal_applypatch() { + #[tokio::test] + async fn test_literal_applypatch() { let args = strs_to_strings(&[ "applypatch", r#"*** Begin Patch @@ -522,20 +540,20 @@ mod tests { } } - #[test] - fn test_heredoc() { + #[tokio::test] + async fn test_heredoc() { assert_match(&heredoc_script(""), /*expected_workdir*/ None); } - #[test] - fn test_heredoc_non_login_shell() { + #[tokio::test] + async fn test_heredoc_non_login_shell() { let script = heredoc_script(""); let args = strs_to_strings(&["bash", "-c", &script]); assert_match_args(args, /*expected_workdir*/ None); } - #[test] - fn test_heredoc_applypatch() { + #[tokio::test] + async fn test_heredoc_applypatch() { let args = strs_to_strings(&[ "bash", "-lc", @@ -562,96 +580,96 @@ PATCH"#, } } - #[test] - fn test_powershell_heredoc() { + #[tokio::test] + async fn test_powershell_heredoc() { let script = heredoc_script(""); assert_match_args(args_powershell(&script), /*expected_workdir*/ None); } - #[test] - fn test_powershell_heredoc_no_profile() { + #[tokio::test] + async fn test_powershell_heredoc_no_profile() { let script = heredoc_script(""); assert_match_args( args_powershell_no_profile(&script), /*expected_workdir*/ None, ); } - #[test] - fn test_pwsh_heredoc() { + #[tokio::test] + async fn test_pwsh_heredoc() { let script = heredoc_script(""); assert_match_args(args_pwsh(&script), /*expected_workdir*/ None); } - #[test] - fn test_cmd_heredoc_with_cd() { + #[tokio::test] + async fn test_cmd_heredoc_with_cd() { let script = heredoc_script("cd foo && "); assert_match_args(args_cmd(&script), Some("foo")); } - #[test] - fn test_heredoc_with_leading_cd() { + #[tokio::test] + async fn test_heredoc_with_leading_cd() { assert_match(&heredoc_script("cd foo && "), Some("foo")); } - #[test] - fn test_cd_with_semicolon_is_ignored() { + #[tokio::test] + async fn test_cd_with_semicolon_is_ignored() { assert_not_match(&heredoc_script("cd foo; ")); } - #[test] - fn test_cd_or_apply_patch_is_ignored() { + #[tokio::test] + async fn test_cd_or_apply_patch_is_ignored() { assert_not_match(&heredoc_script("cd bar || ")); } - #[test] - fn test_cd_pipe_apply_patch_is_ignored() { + #[tokio::test] + async fn test_cd_pipe_apply_patch_is_ignored() { assert_not_match(&heredoc_script("cd bar | ")); } - #[test] - fn test_cd_single_quoted_path_with_spaces() { + #[tokio::test] + async fn test_cd_single_quoted_path_with_spaces() { assert_match(&heredoc_script("cd 'foo bar' && "), Some("foo bar")); } - #[test] - fn test_cd_double_quoted_path_with_spaces() { + #[tokio::test] + async fn test_cd_double_quoted_path_with_spaces() { assert_match(&heredoc_script("cd \"foo bar\" && "), Some("foo bar")); } - #[test] - fn test_echo_and_apply_patch_is_ignored() { + #[tokio::test] + async fn test_echo_and_apply_patch_is_ignored() { assert_not_match(&heredoc_script("echo foo && ")); } - #[test] - fn test_apply_patch_with_arg_is_ignored() { + #[tokio::test] + async fn test_apply_patch_with_arg_is_ignored() { let script = "apply_patch foo <<'PATCH'\n*** Begin Patch\n*** Add File: foo\n+hi\n*** End Patch\nPATCH"; assert_not_match(script); } - #[test] - fn test_double_cd_then_apply_patch_is_ignored() { + #[tokio::test] + async fn test_double_cd_then_apply_patch_is_ignored() { assert_not_match(&heredoc_script("cd foo && cd bar && ")); } - #[test] - fn test_cd_two_args_is_ignored() { + #[tokio::test] + async fn test_cd_two_args_is_ignored() { assert_not_match(&heredoc_script("cd foo bar && ")); } - #[test] - fn test_cd_then_apply_patch_then_extra_is_ignored() { + #[tokio::test] + async fn test_cd_then_apply_patch_then_extra_is_ignored() { let script = heredoc_script_ps("cd bar && ", " && echo done"); assert_not_match(&script); } - #[test] - fn test_echo_then_cd_and_apply_patch_is_ignored() { + #[tokio::test] + async fn test_echo_then_cd_and_apply_patch_is_ignored() { // Ensure preceding commands before the `cd && apply_patch <<...` sequence do not match. assert_not_match(&heredoc_script("echo foo; cd bar && ")); } - #[test] - fn test_unified_diff_last_line_replacement() { + #[tokio::test] + async fn test_unified_diff_last_line_replacement() { // Replace the very last line of the file. let dir = tempdir().unwrap(); let path = dir.path().join("last.txt"); @@ -674,7 +692,10 @@ PATCH"#, _ => panic!("Expected a single UpdateFile hunk"), }; - let diff = unified_diff_from_chunks(&path, chunks).unwrap(); + let path_abs = path.as_path().abs(); + let diff = unified_diff_from_chunks(&path_abs, chunks, LOCAL_FS.as_ref()) + .await + .unwrap(); let expected_diff = r#"@@ -2,2 +2,2 @@ bar -baz @@ -687,8 +708,8 @@ PATCH"#, assert_eq!(expected, diff); } - #[test] - fn test_unified_diff_insert_at_eof() { + #[tokio::test] + async fn test_unified_diff_insert_at_eof() { // Insert a new line at end‑of‑file. let dir = tempdir().unwrap(); let path = dir.path().join("insert.txt"); @@ -709,7 +730,10 @@ PATCH"#, _ => panic!("Expected a single UpdateFile hunk"), }; - let diff = unified_diff_from_chunks(&path, chunks).unwrap(); + let path_abs = path.as_path().abs(); + let diff = unified_diff_from_chunks(&path_abs, chunks, LOCAL_FS.as_ref()) + .await + .unwrap(); let expected_diff = r#"@@ -3 +3,2 @@ baz +quux @@ -721,8 +745,8 @@ PATCH"#, assert_eq!(expected, diff); } - #[test] - fn test_apply_patch_should_resolve_absolute_paths_in_cwd() { + #[tokio::test] + async fn test_apply_patch_should_resolve_absolute_paths_in_cwd() { let session_dir = tempdir().unwrap(); let relative_path = "source.txt"; @@ -742,7 +766,12 @@ PATCH"#, .to_string(), ]; - let result = maybe_parse_apply_patch_verified(&argv, session_dir.path()); + let result = maybe_parse_apply_patch_verified( + &argv, + &AbsolutePathBuf::from_absolute_path(session_dir.path()).unwrap(), + LOCAL_FS.as_ref(), + ) + .await; // Verify the patch contents - as otherwise we may have pulled contents // from the wrong file (as we're using relative paths) @@ -762,13 +791,13 @@ PATCH"#, }, )]), patch: argv[1].clone(), - cwd: session_dir.path().to_path_buf(), + cwd: AbsolutePathBuf::from_absolute_path(session_dir.path()).unwrap(), }) ); } - #[test] - fn test_apply_patch_resolves_move_path_with_effective_cwd() { + #[tokio::test] + async fn test_apply_patch_resolves_move_path_with_effective_cwd() { let session_dir = tempdir().unwrap(); let worktree_rel = "alt"; let worktree_dir = session_dir.path().join(worktree_rel); @@ -790,13 +819,18 @@ PATCH"#, let shell_script = format!("cd {worktree_rel} && apply_patch <<'PATCH'\n{patch}\nPATCH"); let argv = vec!["bash".into(), "-lc".into(), shell_script]; - let result = maybe_parse_apply_patch_verified(&argv, session_dir.path()); + let result = maybe_parse_apply_patch_verified( + &argv, + &AbsolutePathBuf::from_absolute_path(session_dir.path()).unwrap(), + LOCAL_FS.as_ref(), + ) + .await; let action = match result { MaybeApplyPatchVerified::Body(action) => action, other => panic!("expected verified body, got {other:?}"), }; - assert_eq!(action.cwd, worktree_dir); + assert_eq!(action.cwd.as_path(), worktree_dir.as_path()); let change = action .changes() diff --git a/codex-rs/apply-patch/src/lib.rs b/codex-rs/apply-patch/src/lib.rs index 3041c4b2cd..6b6aba2df3 100644 --- a/codex-rs/apply-patch/src/lib.rs +++ b/codex-rs/apply-patch/src/lib.rs @@ -4,11 +4,16 @@ mod seek_sequence; mod standalone_executable; use std::collections::HashMap; +use std::io; use std::path::Path; use std::path::PathBuf; use anyhow::Context; use anyhow::Result; +use codex_exec_server::CreateDirectoryOptions; +use codex_exec_server::ExecutorFileSystem; +use codex_exec_server::RemoveOptions; +use codex_utils_absolute_path::AbsolutePathBuf; pub use parser::Hunk; pub use parser::ParseError; use parser::ParseError::*; @@ -134,7 +139,7 @@ pub struct ApplyPatchAction { pub patch: String, /// The working directory that was used to resolve relative paths in the patch. - pub cwd: PathBuf, + pub cwd: AbsolutePathBuf, } impl ApplyPatchAction { @@ -149,11 +154,7 @@ impl ApplyPatchAction { /// Should be used exclusively for testing. (Not worth the overhead of /// creating a feature flag for this.) - pub fn new_add_for_test(path: &Path, content: String) -> Self { - if !path.is_absolute() { - panic!("path must be absolute"); - } - + pub fn new_add_for_test(path: &AbsolutePathBuf, content: String) -> Self { #[expect(clippy::expect_used)] let filename = path .file_name() @@ -170,20 +171,19 @@ impl ApplyPatchAction { #[expect(clippy::expect_used)] Self { changes, - cwd: path - .parent() - .expect("path should have parent") - .to_path_buf(), + cwd: path.parent().expect("path should have parent"), patch, } } } /// Applies the patch and prints the result to stdout/stderr. -pub fn apply_patch( +pub async fn apply_patch( patch: &str, + cwd: &AbsolutePathBuf, stdout: &mut impl std::io::Write, stderr: &mut impl std::io::Write, + fs: &dyn ExecutorFileSystem, ) -> Result<(), ApplyPatchError> { let hunks = match parse_patch(patch) { Ok(source) => source.hunks, @@ -207,45 +207,21 @@ pub fn apply_patch( } }; - apply_hunks(&hunks, stdout, stderr)?; + apply_hunks(&hunks, cwd, stdout, stderr, fs).await?; Ok(()) } /// Applies hunks and continues to update stdout/stderr -pub fn apply_hunks( +pub async fn apply_hunks( hunks: &[Hunk], + cwd: &AbsolutePathBuf, stdout: &mut impl std::io::Write, stderr: &mut impl std::io::Write, + fs: &dyn ExecutorFileSystem, ) -> Result<(), ApplyPatchError> { - let _existing_paths: Vec<&Path> = hunks - .iter() - .filter_map(|hunk| match hunk { - Hunk::AddFile { .. } => { - // The file is being added, so it doesn't exist yet. - None - } - Hunk::DeleteFile { path } => Some(path.as_path()), - Hunk::UpdateFile { - path, move_path, .. - } => match move_path { - Some(move_path) => { - if std::fs::metadata(move_path) - .map(|m| m.is_file()) - .unwrap_or(false) - { - Some(move_path.as_path()) - } else { - None - } - } - None => Some(path.as_path()), - }, - }) - .collect::>(); - // Delegate to a helper that applies each hunk to the filesystem. - match apply_hunks_to_files(hunks) { + match apply_hunks_to_files(hunks, cwd, fs).await { Ok(affected) => { print_summary(&affected, stdout).map_err(ApplyPatchError::from)?; Ok(()) @@ -267,7 +243,8 @@ pub fn apply_hunks( /// Applies each parsed patch hunk to the filesystem. /// Returns an error if any of the changes could not be applied. -/// Tracks file paths affected by applying a patch. +/// Tracks file paths affected by applying a patch, preserving the path spelling +/// from the patch for user-facing summaries. pub struct AffectedPaths { pub added: Vec, pub modified: Vec, @@ -276,7 +253,11 @@ pub struct AffectedPaths { /// Apply the hunks to the filesystem, returning which files were added, modified, or deleted. /// Returns an error if the patch could not be applied. -fn apply_hunks_to_files(hunks: &[Hunk]) -> anyhow::Result { +async fn apply_hunks_to_files( + hunks: &[Hunk], + cwd: &AbsolutePathBuf, + fs: &dyn ExecutorFileSystem, +) -> anyhow::Result { if hunks.is_empty() { anyhow::bail!("No files were modified."); } @@ -285,48 +266,97 @@ fn apply_hunks_to_files(hunks: &[Hunk]) -> anyhow::Result { let mut modified: Vec = Vec::new(); let mut deleted: Vec = Vec::new(); for hunk in hunks { + let affected_path = hunk.path().to_path_buf(); + let path_abs = hunk.resolve_path(cwd); match hunk { - Hunk::AddFile { path, contents } => { - if let Some(parent) = path.parent() - && !parent.as_os_str().is_empty() - { - std::fs::create_dir_all(parent).with_context(|| { - format!("Failed to create parent directories for {}", path.display()) - })?; + Hunk::AddFile { contents, .. } => { + if let Some(parent_abs) = path_abs.parent() { + fs.create_directory(&parent_abs, CreateDirectoryOptions { recursive: true }) + .await + .with_context(|| { + format!( + "Failed to create parent directories for {}", + path_abs.display() + ) + })?; } - std::fs::write(path, contents) - .with_context(|| format!("Failed to write file {}", path.display()))?; - added.push(path.clone()); + fs.write_file(&path_abs, contents.clone().into_bytes()) + .await + .with_context(|| format!("Failed to write file {}", path_abs.display()))?; + added.push(affected_path); } - Hunk::DeleteFile { path } => { - std::fs::remove_file(path) - .with_context(|| format!("Failed to delete file {}", path.display()))?; - deleted.push(path.clone()); + Hunk::DeleteFile { .. } => { + let result: io::Result<()> = async { + let metadata = fs.get_metadata(&path_abs).await?; + if metadata.is_directory { + return Err(io::Error::new( + io::ErrorKind::InvalidInput, + "path is a directory", + )); + } + fs.remove( + &path_abs, + RemoveOptions { + recursive: false, + force: false, + }, + ) + .await + } + .await; + result.with_context(|| format!("Failed to delete file {}", path_abs.display()))?; + deleted.push(affected_path); } Hunk::UpdateFile { - path, - move_path, - chunks, + move_path, chunks, .. } => { let AppliedPatch { new_contents, .. } = - derive_new_contents_from_chunks(path, chunks)?; + derive_new_contents_from_chunks(&path_abs, chunks, fs).await?; if let Some(dest) = move_path { - if let Some(parent) = dest.parent() - && !parent.as_os_str().is_empty() - { - std::fs::create_dir_all(parent).with_context(|| { - format!("Failed to create parent directories for {}", dest.display()) + let dest_abs = AbsolutePathBuf::resolve_path_against_base(dest, cwd); + if let Some(parent_abs) = dest_abs.parent() { + fs.create_directory( + &parent_abs, + CreateDirectoryOptions { recursive: true }, + ) + .await + .with_context(|| { + format!( + "Failed to create parent directories for {}", + dest_abs.display() + ) })?; } - std::fs::write(dest, new_contents) - .with_context(|| format!("Failed to write file {}", dest.display()))?; - std::fs::remove_file(path) - .with_context(|| format!("Failed to remove original {}", path.display()))?; - modified.push(dest.clone()); + fs.write_file(&dest_abs, new_contents.into_bytes()) + .await + .with_context(|| format!("Failed to write file {}", dest_abs.display()))?; + let result: io::Result<()> = async { + let metadata = fs.get_metadata(&path_abs).await?; + if metadata.is_directory { + return Err(io::Error::new( + io::ErrorKind::InvalidInput, + "path is a directory", + )); + } + fs.remove( + &path_abs, + RemoveOptions { + recursive: false, + force: false, + }, + ) + .await + } + .await; + result.with_context(|| { + format!("Failed to remove original {}", path_abs.display()) + })?; + modified.push(affected_path); } else { - std::fs::write(path, new_contents) - .with_context(|| format!("Failed to write file {}", path.display()))?; - modified.push(path.clone()); + fs.write_file(&path_abs, new_contents.into_bytes()) + .await + .with_context(|| format!("Failed to write file {}", path_abs.display()))?; + modified.push(affected_path); } } } @@ -345,19 +375,17 @@ struct AppliedPatch { /// Return *only* the new file contents (joined into a single `String`) after /// applying the chunks to the file at `path`. -fn derive_new_contents_from_chunks( - path: &Path, +async fn derive_new_contents_from_chunks( + path_abs: &AbsolutePathBuf, chunks: &[UpdateFileChunk], + fs: &dyn ExecutorFileSystem, ) -> std::result::Result { - let original_contents = match std::fs::read_to_string(path) { - Ok(contents) => contents, - Err(err) => { - return Err(ApplyPatchError::IoError(IoError { - context: format!("Failed to read file to update {}", path.display()), - source: err, - })); - } - }; + let original_contents = fs.read_file_text(path_abs).await.map_err(|err| { + ApplyPatchError::IoError(IoError { + context: format!("Failed to read file to update {}", path_abs.display()), + source: err, + }) + })?; let mut original_lines: Vec = original_contents.split('\n').map(String::from).collect(); @@ -367,7 +395,7 @@ fn derive_new_contents_from_chunks( original_lines.pop(); } - let replacements = compute_replacements(&original_lines, path, chunks)?; + let replacements = compute_replacements(&original_lines, path_abs.as_path(), chunks)?; let new_lines = apply_replacements(original_lines, &replacements); let mut new_lines = new_lines; if !new_lines.last().is_some_and(String::is_empty) { @@ -508,22 +536,24 @@ pub struct ApplyPatchFileUpdate { content: String, } -pub fn unified_diff_from_chunks( - path: &Path, +pub async fn unified_diff_from_chunks( + path_abs: &AbsolutePathBuf, chunks: &[UpdateFileChunk], + fs: &dyn ExecutorFileSystem, ) -> std::result::Result { - unified_diff_from_chunks_with_context(path, chunks, /*context*/ 1) + unified_diff_from_chunks_with_context(path_abs, chunks, /*context*/ 1, fs).await } -pub fn unified_diff_from_chunks_with_context( - path: &Path, +pub async fn unified_diff_from_chunks_with_context( + path_abs: &AbsolutePathBuf, chunks: &[UpdateFileChunk], context: usize, + fs: &dyn ExecutorFileSystem, ) -> std::result::Result { let AppliedPatch { original_contents, new_contents, - } = derive_new_contents_from_chunks(path, chunks)?; + } = derive_new_contents_from_chunks(path_abs, chunks, fs).await?; let text_diff = TextDiff::from_lines(&original_contents, &new_contents); let unified_diff = text_diff.unified_diff().context_radius(context).to_string(); Ok(ApplyPatchFileUpdate { @@ -554,6 +584,8 @@ pub fn print_summary( #[cfg(test)] mod tests { use super::*; + use codex_exec_server::LOCAL_FS; + use codex_utils_absolute_path::test_support::PathExt; use pretty_assertions::assert_eq; use std::fs; use std::string::ToString; @@ -564,8 +596,8 @@ mod tests { format!("*** Begin Patch\n{body}\n*** End Patch") } - #[test] - fn test_add_file_hunk_creates_file_with_contents() { + #[tokio::test] + async fn test_add_file_hunk_creates_file_with_contents() { let dir = tempdir().unwrap(); let path = dir.path().join("add.txt"); let patch = wrap_patch(&format!( @@ -576,7 +608,15 @@ mod tests { )); let mut stdout = Vec::new(); let mut stderr = Vec::new(); - apply_patch(&patch, &mut stdout, &mut stderr).unwrap(); + apply_patch( + &patch, + &AbsolutePathBuf::from_absolute_path(dir.path()).unwrap(), + &mut stdout, + &mut stderr, + LOCAL_FS.as_ref(), + ) + .await + .unwrap(); // Verify expected stdout and stderr outputs. let stdout_str = String::from_utf8(stdout).unwrap(); let stderr_str = String::from_utf8(stderr).unwrap(); @@ -590,15 +630,88 @@ mod tests { assert_eq!(contents, "ab\ncd\n"); } - #[test] - fn test_delete_file_hunk_removes_file() { + #[tokio::test] + async fn test_apply_patch_hunks_accept_relative_and_absolute_paths() { + let dir = tempdir().unwrap(); + let cwd = dir.path().abs(); + let relative_add = dir.path().join("relative-add.txt"); + let absolute_add = dir.path().join("absolute-add.txt"); + let relative_delete = dir.path().join("relative-delete.txt"); + let absolute_delete = dir.path().join("absolute-delete.txt"); + let relative_update = dir.path().join("relative-update.txt"); + let absolute_update = dir.path().join("absolute-update.txt"); + fs::write(&relative_delete, "delete relative\n").unwrap(); + fs::write(&absolute_delete, "delete absolute\n").unwrap(); + fs::write(&relative_update, "relative old\n").unwrap(); + fs::write(&absolute_update, "absolute old\n").unwrap(); + + let patch = wrap_patch(&format!( + r#"*** Add File: relative-add.txt ++relative add +*** Add File: {} ++absolute add +*** Delete File: relative-delete.txt +*** Delete File: {} +*** Update File: relative-update.txt +@@ +-relative old ++relative new +*** Update File: {} +@@ +-absolute old ++absolute new"#, + absolute_add.display(), + absolute_delete.display(), + absolute_update.display(), + )); + let mut stdout = Vec::new(); + let mut stderr = Vec::new(); + + apply_patch(&patch, &cwd, &mut stdout, &mut stderr, LOCAL_FS.as_ref()) + .await + .unwrap(); + + assert_eq!(fs::read_to_string(&relative_add).unwrap(), "relative add\n"); + assert_eq!(fs::read_to_string(&absolute_add).unwrap(), "absolute add\n"); + assert!(!relative_delete.exists()); + assert!(!absolute_delete.exists()); + assert_eq!( + fs::read_to_string(&relative_update).unwrap(), + "relative new\n" + ); + assert_eq!( + fs::read_to_string(&absolute_update).unwrap(), + "absolute new\n" + ); + assert_eq!(String::from_utf8(stderr).unwrap(), ""); + assert_eq!( + String::from_utf8(stdout).unwrap(), + format!( + "Success. Updated the following files:\nA relative-add.txt\nA {}\nM relative-update.txt\nM {}\nD relative-delete.txt\nD {}\n", + absolute_add.display(), + absolute_update.display(), + absolute_delete.display(), + ) + ); + } + + #[tokio::test] + async fn test_delete_file_hunk_removes_file() { let dir = tempdir().unwrap(); let path = dir.path().join("del.txt"); fs::write(&path, "x").unwrap(); let patch = wrap_patch(&format!("*** Delete File: {}", path.display())); let mut stdout = Vec::new(); let mut stderr = Vec::new(); - apply_patch(&patch, &mut stdout, &mut stderr).unwrap(); + apply_patch( + &patch, + &AbsolutePathBuf::from_absolute_path(dir.path()).unwrap(), + &mut stdout, + &mut stderr, + LOCAL_FS.as_ref(), + ) + .await + .unwrap(); let stdout_str = String::from_utf8(stdout).unwrap(); let stderr_str = String::from_utf8(stderr).unwrap(); let expected_out = format!( @@ -610,8 +723,8 @@ mod tests { assert!(!path.exists()); } - #[test] - fn test_update_file_hunk_modifies_content() { + #[tokio::test] + async fn test_update_file_hunk_modifies_content() { let dir = tempdir().unwrap(); let path = dir.path().join("update.txt"); fs::write(&path, "foo\nbar\n").unwrap(); @@ -625,7 +738,15 @@ mod tests { )); let mut stdout = Vec::new(); let mut stderr = Vec::new(); - apply_patch(&patch, &mut stdout, &mut stderr).unwrap(); + apply_patch( + &patch, + &AbsolutePathBuf::from_absolute_path(dir.path()).unwrap(), + &mut stdout, + &mut stderr, + LOCAL_FS.as_ref(), + ) + .await + .unwrap(); // Validate modified file contents and expected stdout/stderr. let stdout_str = String::from_utf8(stdout).unwrap(); let stderr_str = String::from_utf8(stderr).unwrap(); @@ -639,8 +760,8 @@ mod tests { assert_eq!(contents, "foo\nbaz\n"); } - #[test] - fn test_update_file_hunk_can_move_file() { + #[tokio::test] + async fn test_update_file_hunk_can_move_file() { let dir = tempdir().unwrap(); let src = dir.path().join("src.txt"); let dest = dir.path().join("dst.txt"); @@ -656,7 +777,15 @@ mod tests { )); let mut stdout = Vec::new(); let mut stderr = Vec::new(); - apply_patch(&patch, &mut stdout, &mut stderr).unwrap(); + apply_patch( + &patch, + &AbsolutePathBuf::from_absolute_path(dir.path()).unwrap(), + &mut stdout, + &mut stderr, + LOCAL_FS.as_ref(), + ) + .await + .unwrap(); // Validate move semantics and expected stdout/stderr. let stdout_str = String::from_utf8(stdout).unwrap(); let stderr_str = String::from_utf8(stderr).unwrap(); @@ -673,8 +802,8 @@ mod tests { /// Verify that a single `Update File` hunk with multiple change chunks can update different /// parts of a file and that the file is listed only once in the summary. - #[test] - fn test_multiple_update_chunks_apply_to_single_file() { + #[tokio::test] + async fn test_multiple_update_chunks_apply_to_single_file() { // Start with a file containing four lines. let dir = tempdir().unwrap(); let path = dir.path().join("multi.txt"); @@ -696,7 +825,15 @@ mod tests { )); let mut stdout = Vec::new(); let mut stderr = Vec::new(); - apply_patch(&patch, &mut stdout, &mut stderr).unwrap(); + apply_patch( + &patch, + &AbsolutePathBuf::from_absolute_path(dir.path()).unwrap(), + &mut stdout, + &mut stderr, + LOCAL_FS.as_ref(), + ) + .await + .unwrap(); let stdout_str = String::from_utf8(stdout).unwrap(); let stderr_str = String::from_utf8(stderr).unwrap(); let expected_out = format!( @@ -713,8 +850,8 @@ mod tests { /// replacements in separate chunks that appear in non‑adjacent parts of the /// file. Verifies that all edits are applied and that the summary lists the /// file only once. - #[test] - fn test_update_file_hunk_interleaved_changes() { + #[tokio::test] + async fn test_update_file_hunk_interleaved_changes() { let dir = tempdir().unwrap(); let path = dir.path().join("interleaved.txt"); @@ -745,7 +882,15 @@ mod tests { let mut stdout = Vec::new(); let mut stderr = Vec::new(); - apply_patch(&patch, &mut stdout, &mut stderr).unwrap(); + apply_patch( + &patch, + &AbsolutePathBuf::from_absolute_path(dir.path()).unwrap(), + &mut stdout, + &mut stderr, + LOCAL_FS.as_ref(), + ) + .await + .unwrap(); let stdout_str = String::from_utf8(stdout).unwrap(); let stderr_str = String::from_utf8(stderr).unwrap(); @@ -761,8 +906,8 @@ mod tests { assert_eq!(contents, "a\nB\nc\nd\nE\nf\ng\n"); } - #[test] - fn test_pure_addition_chunk_followed_by_removal() { + #[tokio::test] + async fn test_pure_addition_chunk_followed_by_removal() { let dir = tempdir().unwrap(); let path = dir.path().join("panic.txt"); fs::write(&path, "line1\nline2\nline3\n").unwrap(); @@ -780,7 +925,15 @@ mod tests { )); let mut stdout = Vec::new(); let mut stderr = Vec::new(); - apply_patch(&patch, &mut stdout, &mut stderr).unwrap(); + apply_patch( + &patch, + &AbsolutePathBuf::from_absolute_path(dir.path()).unwrap(), + &mut stdout, + &mut stderr, + LOCAL_FS.as_ref(), + ) + .await + .unwrap(); let contents = fs::read_to_string(path).unwrap(); assert_eq!( contents, @@ -794,8 +947,8 @@ mod tests { /// internal matcher failed requiring an exact byte-for-byte match. The /// fuzzy-matching pass that normalises common punctuation should now bridge /// the gap. - #[test] - fn test_update_line_with_unicode_dash() { + #[tokio::test] + async fn test_update_line_with_unicode_dash() { let dir = tempdir().unwrap(); let path = dir.path().join("unicode.py"); @@ -814,7 +967,15 @@ mod tests { let mut stdout = Vec::new(); let mut stderr = Vec::new(); - apply_patch(&patch, &mut stdout, &mut stderr).unwrap(); + apply_patch( + &patch, + &AbsolutePathBuf::from_absolute_path(dir.path()).unwrap(), + &mut stdout, + &mut stderr, + LOCAL_FS.as_ref(), + ) + .await + .unwrap(); // File should now contain the replaced comment. let expected = "import asyncio # HELLO\n"; @@ -833,8 +994,8 @@ mod tests { assert_eq!(String::from_utf8(stderr).unwrap(), ""); } - #[test] - fn test_unified_diff() { + #[tokio::test] + async fn test_unified_diff() { // Start with a file containing four lines. let dir = tempdir().unwrap(); let path = dir.path().join("multi.txt"); @@ -857,7 +1018,10 @@ mod tests { [Hunk::UpdateFile { chunks, .. }] => chunks, _ => panic!("Expected a single UpdateFile hunk"), }; - let diff = unified_diff_from_chunks(&path, update_file_chunks).unwrap(); + let path_abs = path.as_path().abs(); + let diff = unified_diff_from_chunks(&path_abs, update_file_chunks, LOCAL_FS.as_ref()) + .await + .unwrap(); let expected_diff = r#"@@ -1,4 +1,4 @@ foo -bar @@ -873,8 +1037,8 @@ mod tests { assert_eq!(expected, diff); } - #[test] - fn test_unified_diff_first_line_replacement() { + #[tokio::test] + async fn test_unified_diff_first_line_replacement() { // Replace the very first line of the file. let dir = tempdir().unwrap(); let path = dir.path().join("first.txt"); @@ -896,7 +1060,10 @@ mod tests { _ => panic!("Expected a single UpdateFile hunk"), }; - let diff = unified_diff_from_chunks(&path, chunks).unwrap(); + let path_abs = path.as_path().abs(); + let diff = unified_diff_from_chunks(&path_abs, chunks, LOCAL_FS.as_ref()) + .await + .unwrap(); let expected_diff = r#"@@ -1,2 +1,2 @@ -foo +FOO @@ -909,8 +1076,8 @@ mod tests { assert_eq!(expected, diff); } - #[test] - fn test_unified_diff_last_line_replacement() { + #[tokio::test] + async fn test_unified_diff_last_line_replacement() { // Replace the very last line of the file. let dir = tempdir().unwrap(); let path = dir.path().join("last.txt"); @@ -933,7 +1100,10 @@ mod tests { _ => panic!("Expected a single UpdateFile hunk"), }; - let diff = unified_diff_from_chunks(&path, chunks).unwrap(); + let path_abs = path.as_path().abs(); + let diff = unified_diff_from_chunks(&path_abs, chunks, LOCAL_FS.as_ref()) + .await + .unwrap(); let expected_diff = r#"@@ -2,2 +2,2 @@ bar -baz @@ -946,8 +1116,8 @@ mod tests { assert_eq!(expected, diff); } - #[test] - fn test_unified_diff_insert_at_eof() { + #[tokio::test] + async fn test_unified_diff_insert_at_eof() { // Insert a new line at end‑of‑file. let dir = tempdir().unwrap(); let path = dir.path().join("insert.txt"); @@ -968,7 +1138,10 @@ mod tests { _ => panic!("Expected a single UpdateFile hunk"), }; - let diff = unified_diff_from_chunks(&path, chunks).unwrap(); + let path_abs = path.as_path().abs(); + let diff = unified_diff_from_chunks(&path_abs, chunks, LOCAL_FS.as_ref()) + .await + .unwrap(); let expected_diff = r#"@@ -3 +3,2 @@ baz +quux @@ -980,8 +1153,8 @@ mod tests { assert_eq!(expected, diff); } - #[test] - fn test_unified_diff_interleaved_changes() { + #[tokio::test] + async fn test_unified_diff_interleaved_changes() { // Original file with six lines. let dir = tempdir().unwrap(); let path = dir.path().join("interleaved.txt"); @@ -1014,7 +1187,10 @@ mod tests { _ => panic!("Expected a single UpdateFile hunk"), }; - let diff = unified_diff_from_chunks(&path, chunks).unwrap(); + let path_abs = path.as_path().abs(); + let diff = unified_diff_from_chunks(&path_abs, chunks, LOCAL_FS.as_ref()) + .await + .unwrap(); let expected_diff = r#"@@ -1,6 +1,7 @@ a @@ -1037,7 +1213,15 @@ mod tests { let mut stdout = Vec::new(); let mut stderr = Vec::new(); - apply_patch(&patch, &mut stdout, &mut stderr).unwrap(); + apply_patch( + &patch, + &AbsolutePathBuf::from_absolute_path(dir.path()).unwrap(), + &mut stdout, + &mut stderr, + LOCAL_FS.as_ref(), + ) + .await + .unwrap(); let contents = fs::read_to_string(path).unwrap(); assert_eq!( contents, @@ -1052,8 +1236,8 @@ g ); } - #[test] - fn test_apply_patch_fails_on_write_error() { + #[tokio::test] + async fn test_apply_patch_fails_on_write_error() { let dir = tempdir().unwrap(); let path = dir.path().join("readonly.txt"); fs::write(&path, "before\n").unwrap(); @@ -1068,7 +1252,14 @@ g let mut stdout = Vec::new(); let mut stderr = Vec::new(); - let result = apply_patch(&patch, &mut stdout, &mut stderr); + let result = apply_patch( + &patch, + &AbsolutePathBuf::from_absolute_path(dir.path()).unwrap(), + &mut stdout, + &mut stderr, + LOCAL_FS.as_ref(), + ) + .await; assert!(result.is_err()); } } diff --git a/codex-rs/apply-patch/src/parser.rs b/codex-rs/apply-patch/src/parser.rs index 274a45497a..24ebcb1463 100644 --- a/codex-rs/apply-patch/src/parser.rs +++ b/codex-rs/apply-patch/src/parser.rs @@ -23,6 +23,9 @@ //! The parser below is a little more lenient than the explicit spec and allows for //! leading/trailing whitespace around patch markers. use crate::ApplyPatchArgs; +use codex_utils_absolute_path::AbsolutePathBuf; +#[cfg(test)] +use codex_utils_absolute_path::test_support::PathBufExt; use std::path::Path; use std::path::PathBuf; @@ -76,11 +79,28 @@ pub enum Hunk { } impl Hunk { - pub fn resolve_path(&self, cwd: &Path) -> PathBuf { + pub fn resolve_path(&self, cwd: &AbsolutePathBuf) -> AbsolutePathBuf { + let path = match self { + Hunk::UpdateFile { path, .. } => path, + Hunk::AddFile { .. } | Hunk::DeleteFile { .. } => self.path(), + }; + AbsolutePathBuf::resolve_path_against_base(path, cwd) + } + + /// Returns the path affected by this hunk, using the move destination for rename hunks. + pub fn path(&self) -> &Path { match self { - Hunk::AddFile { path, .. } => cwd.join(path), - Hunk::DeleteFile { path } => cwd.join(path), - Hunk::UpdateFile { path, .. } => cwd.join(path), + Hunk::AddFile { path, .. } => path, + Hunk::DeleteFile { path } => path, + Hunk::UpdateFile { + move_path: Some(path), + .. + } => path, + Hunk::UpdateFile { + path, + move_path: None, + .. + } => path, } } } @@ -583,6 +603,108 @@ fn test_parse_patch() { ); } +#[test] +fn test_parse_patch_accepts_relative_and_absolute_hunk_paths() { + let dir = tempfile::tempdir().unwrap(); + let absolute_delete = dir.path().join("absolute-delete.py").abs(); + let absolute_update = dir.path().join("absolute-update.py").abs(); + let patch_text = format!( + r#"*** Begin Patch +*** Add File: relative-add.py ++content +*** Delete File: {} +*** Update File: {} +@@ +-old ++new +*** End Patch"#, + absolute_delete.display(), + absolute_update.display() + ); + + assert_eq!( + parse_patch_text(&patch_text, ParseMode::Strict) + .unwrap() + .hunks, + vec![ + AddFile { + path: PathBuf::from("relative-add.py"), + contents: "content\n".to_string() + }, + DeleteFile { + path: absolute_delete.to_path_buf() + }, + UpdateFile { + path: absolute_update.to_path_buf(), + move_path: None, + chunks: vec![UpdateFileChunk { + change_context: None, + old_lines: vec!["old".to_string()], + new_lines: vec!["new".to_string()], + is_end_of_file: false + }] + }, + ] + ); +} + +#[test] +fn test_hunk_resolve_path_accepts_relative_and_absolute_paths() { + let cwd_dir = tempfile::tempdir().unwrap(); + let cwd = cwd_dir.path().to_path_buf().abs(); + let absolute_dir = tempfile::tempdir().unwrap(); + let absolute_add = absolute_dir.path().join("absolute-add.py").abs(); + let absolute_delete = absolute_dir.path().join("absolute-delete.py").abs(); + let absolute_update = absolute_dir.path().join("absolute-update.py").abs(); + + for (hunk, expected_path) in [ + ( + AddFile { + path: PathBuf::from("relative-add.py"), + contents: String::new(), + }, + cwd.join("relative-add.py"), + ), + ( + DeleteFile { + path: PathBuf::from("relative-delete.py"), + }, + cwd.join("relative-delete.py"), + ), + ( + UpdateFile { + path: PathBuf::from("relative-update.py"), + move_path: None, + chunks: Vec::new(), + }, + cwd.join("relative-update.py"), + ), + ( + AddFile { + path: absolute_add.to_path_buf(), + contents: String::new(), + }, + absolute_add, + ), + ( + DeleteFile { + path: absolute_delete.to_path_buf(), + }, + absolute_delete, + ), + ( + UpdateFile { + path: absolute_update.to_path_buf(), + move_path: None, + chunks: Vec::new(), + }, + absolute_update, + ), + ] { + assert_eq!(hunk.resolve_path(&cwd), expected_path); + } +} + #[test] fn test_parse_patch_lenient() { let patch_text = r#"*** Begin Patch diff --git a/codex-rs/apply-patch/src/standalone_executable.rs b/codex-rs/apply-patch/src/standalone_executable.rs index d77a82fa95..149bfd3382 100644 --- a/codex-rs/apply-patch/src/standalone_executable.rs +++ b/codex-rs/apply-patch/src/standalone_executable.rs @@ -48,7 +48,30 @@ pub fn run_main() -> i32 { let mut stdout = std::io::stdout(); let mut stderr = std::io::stderr(); - match crate::apply_patch(&patch_arg, &mut stdout, &mut stderr) { + let cwd = match codex_utils_absolute_path::AbsolutePathBuf::current_dir() { + Ok(cwd) => cwd, + Err(err) => { + eprintln!("Error: Failed to determine current directory.\n{err}"); + return 1; + } + }; + let runtime = match tokio::runtime::Builder::new_current_thread() + .enable_all() + .build() + { + Ok(runtime) => runtime, + Err(err) => { + eprintln!("Error: Failed to initialize runtime.\n{err}"); + return 1; + } + }; + match runtime.block_on(crate::apply_patch( + &patch_arg, + &cwd, + &mut stdout, + &mut stderr, + codex_exec_server::LOCAL_FS.as_ref(), + )) { Ok(()) => { // Flush to ensure output ordering when used in pipelines. let _ = stdout.flush(); diff --git a/codex-rs/apply-patch/tests/suite/tool.rs b/codex-rs/apply-patch/tests/suite/tool.rs index 56cd6e57a7..8499d0fb90 100644 --- a/codex-rs/apply-patch/tests/suite/tool.rs +++ b/codex-rs/apply-patch/tests/suite/tool.rs @@ -2,6 +2,7 @@ use assert_cmd::Command; use pretty_assertions::assert_eq; use std::fs; use std::path::Path; +use std::path::PathBuf; use tempfile::tempdir; fn run_apply_patch_in_dir(dir: &Path, patch: &str) -> anyhow::Result { @@ -16,9 +17,14 @@ fn apply_patch_command(dir: &Path) -> anyhow::Result { Ok(cmd) } +fn resolved_under(root: &Path, path: &str) -> anyhow::Result { + Ok(root.canonicalize()?.join(path)) +} + #[test] fn test_apply_patch_cli_applies_multiple_operations() -> anyhow::Result<()> { let tmp = tempdir()?; + let add_path = tmp.path().join("nested/new.txt"); let modify_path = tmp.path().join("modify.txt"); let delete_path = tmp.path().join("delete.txt"); @@ -31,10 +37,7 @@ fn test_apply_patch_cli_applies_multiple_operations() -> anyhow::Result<()> { "Success. Updated the following files:\nA nested/new.txt\nM modify.txt\nD delete.txt\n", ); - assert_eq!( - fs::read_to_string(tmp.path().join("nested/new.txt"))?, - "created\n" - ); + assert_eq!(fs::read_to_string(add_path)?, "created\n"); assert_eq!(fs::read_to_string(&modify_path)?, "line1\nchanged\n"); assert!(!delete_path.exists()); @@ -98,13 +101,17 @@ fn test_apply_patch_cli_rejects_empty_patch() -> anyhow::Result<()> { fn test_apply_patch_cli_reports_missing_context() -> anyhow::Result<()> { let tmp = tempdir()?; let target_path = tmp.path().join("modify.txt"); + let expected_target_path = resolved_under(tmp.path(), "modify.txt")?; fs::write(&target_path, "line1\nline2\n")?; apply_patch_command(tmp.path())? .arg("*** Begin Patch\n*** Update File: modify.txt\n@@\n-missing\n+changed\n*** End Patch") .assert() .failure() - .stderr("Failed to find expected lines in modify.txt:\nmissing\n"); + .stderr(format!( + "Failed to find expected lines in {}:\nmissing\n", + expected_target_path.display() + )); assert_eq!(fs::read_to_string(&target_path)?, "line1\nline2\n"); Ok(()) @@ -113,12 +120,16 @@ fn test_apply_patch_cli_reports_missing_context() -> anyhow::Result<()> { #[test] fn test_apply_patch_cli_rejects_missing_file_delete() -> anyhow::Result<()> { let tmp = tempdir()?; + let missing_path = resolved_under(tmp.path(), "missing.txt")?; apply_patch_command(tmp.path())? .arg("*** Begin Patch\n*** Delete File: missing.txt\n*** End Patch") .assert() .failure() - .stderr("Failed to delete file missing.txt\n"); + .stderr(format!( + "Failed to delete file {}\n", + missing_path.display() + )); Ok(()) } @@ -139,14 +150,16 @@ fn test_apply_patch_cli_rejects_empty_update_hunk() -> anyhow::Result<()> { #[test] fn test_apply_patch_cli_requires_existing_file_for_update() -> anyhow::Result<()> { let tmp = tempdir()?; + let missing_path = resolved_under(tmp.path(), "missing.txt")?; apply_patch_command(tmp.path())? .arg("*** Begin Patch\n*** Update File: missing.txt\n@@\n-old\n+new\n*** End Patch") .assert() .failure() - .stderr( - "Failed to read file to update missing.txt: No such file or directory (os error 2)\n", - ); + .stderr(format!( + "Failed to read file to update {}: No such file or directory (os error 2)\n", + missing_path.display() + )); Ok(()) } @@ -195,13 +208,18 @@ fn test_apply_patch_cli_add_overwrites_existing_file() -> anyhow::Result<()> { #[test] fn test_apply_patch_cli_delete_directory_fails() -> anyhow::Result<()> { let tmp = tempdir()?; - fs::create_dir(tmp.path().join("dir"))?; + let dir = tmp.path().join("dir"); + let expected_dir = resolved_under(tmp.path(), "dir")?; + fs::create_dir(&dir)?; apply_patch_command(tmp.path())? .arg("*** Begin Patch\n*** Delete File: dir\n*** End Patch") .assert() .failure() - .stderr("Failed to delete file dir\n"); + .stderr(format!( + "Failed to delete file {}\n", + expected_dir.display() + )); Ok(()) } @@ -243,13 +261,17 @@ fn test_apply_patch_cli_updates_file_appends_trailing_newline() -> anyhow::Resul fn test_apply_patch_cli_failure_after_partial_success_leaves_changes() -> anyhow::Result<()> { let tmp = tempdir()?; let new_file = tmp.path().join("created.txt"); + let missing_file = resolved_under(tmp.path(), "missing.txt")?; apply_patch_command(tmp.path())? .arg("*** Begin Patch\n*** Add File: created.txt\n+hello\n*** Update File: missing.txt\n@@\n-old\n+new\n*** End Patch") .assert() .failure() .stdout("") - .stderr("Failed to read file to update missing.txt: No such file or directory (os error 2)\n"); + .stderr(format!( + "Failed to read file to update {}: No such file or directory (os error 2)\n", + missing_file.display() + )); assert_eq!(fs::read_to_string(&new_file)?, "hello\n"); diff --git a/codex-rs/arg0/Cargo.toml b/codex-rs/arg0/Cargo.toml index cd409fedd8..8da0fcbd0b 100644 --- a/codex-rs/arg0/Cargo.toml +++ b/codex-rs/arg0/Cargo.toml @@ -14,9 +14,11 @@ workspace = true [dependencies] anyhow = { workspace = true } codex-apply-patch = { workspace = true } +codex-exec-server = { workspace = true } codex-linux-sandbox = { workspace = true } codex-sandboxing = { workspace = true } codex-shell-escalation = { workspace = true } +codex-utils-absolute-path = { workspace = true } codex-utils-home-dir = { workspace = true } dotenvy = { workspace = true } tempfile = { workspace = true } diff --git a/codex-rs/arg0/src/lib.rs b/codex-rs/arg0/src/lib.rs index efad6c2481..f8b61796b5 100644 --- a/codex-rs/arg0/src/lib.rs +++ b/codex-rs/arg0/src/lib.rs @@ -99,7 +99,24 @@ pub fn arg0_dispatch() -> Option { Some(patch_arg) => { let mut stdout = std::io::stdout(); let mut stderr = std::io::stderr(); - match codex_apply_patch::apply_patch(&patch_arg, &mut stdout, &mut stderr) { + let cwd = match codex_utils_absolute_path::AbsolutePathBuf::current_dir() { + Ok(cwd) => cwd, + Err(_) => std::process::exit(1), + }; + let runtime = match tokio::runtime::Builder::new_current_thread() + .enable_all() + .build() + { + Ok(runtime) => runtime, + Err(_) => std::process::exit(1), + }; + match runtime.block_on(codex_apply_patch::apply_patch( + &patch_arg, + &cwd, + &mut stdout, + &mut stderr, + codex_exec_server::LOCAL_FS.as_ref(), + )) { Ok(()) => 0, Err(_) => 1, } diff --git a/codex-rs/core/src/apply_patch_tests.rs b/codex-rs/core/src/apply_patch_tests.rs index 1b9e722d5a..c0190c3708 100644 --- a/codex-rs/core/src/apply_patch_tests.rs +++ b/codex-rs/core/src/apply_patch_tests.rs @@ -1,4 +1,5 @@ use super::*; +use core_test_support::PathBufExt; use pretty_assertions::assert_eq; use tempfile::tempdir; @@ -6,14 +7,14 @@ use tempfile::tempdir; #[test] fn convert_apply_patch_maps_add_variant() { let tmp = tempdir().expect("tmp"); - let p = tmp.path().join("a.txt"); + let p = tmp.path().join("a.txt").abs(); // Create an action with a single Add change let action = ApplyPatchAction::new_add_for_test(&p, "hello".to_string()); let got = convert_apply_patch_to_protocol(&action); assert_eq!( - got.get(&p), + got.get(p.as_path()), Some(&FileChange::Add { content: "hello".to_string() }) diff --git a/codex-rs/core/src/plugins/startup_sync_tests.rs b/codex-rs/core/src/plugins/startup_sync_tests.rs index cd0b91f4db..74dd0d0fdb 100644 --- a/codex-rs/core/src/plugins/startup_sync_tests.rs +++ b/codex-rs/core/src/plugins/startup_sync_tests.rs @@ -34,6 +34,7 @@ fn has_plugins_clone_dirs(codex_home: &Path) -> bool { }) } +#[cfg(unix)] fn write_executable_script(path: &Path, contents: &str) { #[cfg(unix)] use std::os::unix::fs::PermissionsExt; diff --git a/codex-rs/core/src/safety_tests.rs b/codex-rs/core/src/safety_tests.rs index 6e5795e59a..9ea59efd34 100644 --- a/codex-rs/core/src/safety_tests.rs +++ b/codex-rs/core/src/safety_tests.rs @@ -5,6 +5,7 @@ use codex_protocol::protocol::FileSystemSandboxEntry; use codex_protocol::protocol::FileSystemSpecialPath; use codex_protocol::protocol::GranularApprovalConfig; use codex_utils_absolute_path::AbsolutePathBuf; +use core_test_support::PathBufExt; use pretty_assertions::assert_eq; use tempfile::TempDir; @@ -17,7 +18,10 @@ fn test_writable_roots_constraint() { let parent = cwd.parent().unwrap().to_path_buf(); // Helper to build a single‑entry patch that adds a file at `p`. - let make_add_change = |p: PathBuf| ApplyPatchAction::new_add_for_test(&p, "".to_string()); + let make_add_change = |p: PathBuf| { + let p = p.abs(); + ApplyPatchAction::new_add_for_test(&p, "".to_string()) + }; let add_inside = make_add_change(cwd.join("inner.txt")); let add_outside = make_add_change(parent.join("outside.txt")); @@ -64,7 +68,8 @@ fn test_writable_roots_constraint() { fn external_sandbox_auto_approves_in_on_request() { let tmp = TempDir::new().unwrap(); let cwd = tmp.path().to_path_buf(); - let add_inside = ApplyPatchAction::new_add_for_test(&cwd.join("inner.txt"), "".to_string()); + let add_inside_path = cwd.join("inner.txt").abs(); + let add_inside = ApplyPatchAction::new_add_for_test(&add_inside_path, "".to_string()); let policy = SandboxPolicy::ExternalSandbox { network_access: codex_protocol::protocol::NetworkAccess::Enabled, @@ -91,8 +96,8 @@ fn granular_with_all_flags_true_matches_on_request_for_out_of_root_patch() { let tmp = TempDir::new().unwrap(); let cwd = tmp.path().to_path_buf(); let parent = cwd.parent().unwrap().to_path_buf(); - let add_outside = - ApplyPatchAction::new_add_for_test(&parent.join("outside.txt"), "".to_string()); + let outside_path = parent.join("outside.txt").abs(); + let add_outside = ApplyPatchAction::new_add_for_test(&outside_path, "".to_string()); let policy_workspace_only = SandboxPolicy::WorkspaceWrite { writable_roots: vec![], read_only_access: Default::default(), @@ -136,8 +141,8 @@ fn granular_sandbox_approval_false_rejects_out_of_root_patch() { let tmp = TempDir::new().unwrap(); let cwd = tmp.path().to_path_buf(); let parent = cwd.parent().unwrap().to_path_buf(); - let add_outside = - ApplyPatchAction::new_add_for_test(&parent.join("outside.txt"), "".to_string()); + let outside_path = parent.join("outside.txt").abs(); + let add_outside = ApplyPatchAction::new_add_for_test(&outside_path, "".to_string()); let policy_workspace_only = SandboxPolicy::WorkspaceWrite { writable_roots: vec![], read_only_access: Default::default(), @@ -171,7 +176,8 @@ fn granular_sandbox_approval_false_rejects_out_of_root_patch() { fn read_only_policy_rejects_patch_with_read_only_reason() { let tmp = TempDir::new().unwrap(); let cwd = tmp.path().to_path_buf(); - let action = ApplyPatchAction::new_add_for_test(&cwd.join("inside.txt"), "".to_string()); + let inside_path = cwd.join("inside.txt").abs(); + let action = ApplyPatchAction::new_add_for_test(&inside_path, "".to_string()); let sandbox_policy = SandboxPolicy::new_read_only_policy(); let file_system_sandbox_policy = FileSystemSandboxPolicy::from_legacy_sandbox_policy(&sandbox_policy, &cwd); @@ -200,8 +206,8 @@ fn explicit_unreadable_paths_prevent_auto_approval_for_external_sandbox() { let tmp = TempDir::new().unwrap(); let cwd = tmp.path().to_path_buf(); let blocked_path = cwd.join("blocked.txt"); - let blocked_absolute = AbsolutePathBuf::from_absolute_path(blocked_path.clone()).unwrap(); - let action = ApplyPatchAction::new_add_for_test(&blocked_path, "".to_string()); + let blocked_absolute = blocked_path.abs(); + let action = ApplyPatchAction::new_add_for_test(&blocked_absolute, "".to_string()); let sandbox_policy = SandboxPolicy::ExternalSandbox { network_access: codex_protocol::protocol::NetworkAccess::Restricted, }; @@ -243,8 +249,9 @@ fn explicit_read_only_subpaths_prevent_auto_approval_for_external_sandbox() { let tmp = TempDir::new().unwrap(); let cwd = tmp.path().to_path_buf(); let blocked_path = cwd.join("docs").join("blocked.txt"); + let blocked_absolute = blocked_path.abs(); let docs_absolute = AbsolutePathBuf::resolve_path_against_base("docs", &cwd); - let action = ApplyPatchAction::new_add_for_test(&blocked_path, "".to_string()); + let action = ApplyPatchAction::new_add_for_test(&blocked_absolute, "".to_string()); let sandbox_policy = SandboxPolicy::ExternalSandbox { network_access: codex_protocol::protocol::NetworkAccess::Restricted, }; @@ -285,8 +292,8 @@ fn explicit_read_only_subpaths_prevent_auto_approval_for_external_sandbox() { fn missing_project_dot_codex_config_requires_approval() { let tmp = TempDir::new().unwrap(); let cwd = tmp.path().to_path_buf(); - let action = - ApplyPatchAction::new_add_for_test(&cwd.join(".codex").join("config.toml"), "".to_string()); + let config_path = cwd.join(".codex").join("config.toml").abs(); + let action = ApplyPatchAction::new_add_for_test(&config_path, "".to_string()); let sandbox_policy = SandboxPolicy::WorkspaceWrite { writable_roots: vec![], read_only_access: Default::default(), diff --git a/codex-rs/core/src/tools/context.rs b/codex-rs/core/src/tools/context.rs index 7c486640ee..01dd0548b4 100644 --- a/codex-rs/core/src/tools/context.rs +++ b/codex-rs/core/src/tools/context.rs @@ -364,13 +364,6 @@ impl ExecCommandToolOutput { fn response_text(&self) -> String { let mut sections = Vec::new(); - if let Some(command) = &self.session_command { - sections.push(format!( - "Command: {}", - codex_shell_command::parse_command::shlex_join(command) - )); - } - if !self.chunk_id.is_empty() { sections.push(format!("Chunk ID: {}", self.chunk_id)); } diff --git a/codex-rs/core/src/tools/context_tests.rs b/codex-rs/core/src/tools/context_tests.rs index f7641a1db5..07fcfda981 100644 --- a/codex-rs/core/src/tools/context_tests.rs +++ b/codex-rs/core/src/tools/context_tests.rs @@ -249,11 +249,7 @@ fn exec_command_tool_output_formats_truncated_response() { process_id: None, exit_code: Some(0), original_token_count: Some(10), - session_command: Some(vec![ - "/bin/zsh".to_string(), - "-lc".to_string(), - "rm -rf /tmp/example.sqlite".to_string(), - ]), + session_command: None, } .to_response_item("call-42", &payload); @@ -267,8 +263,7 @@ fn exec_command_tool_output_formats_truncated_response() { .expect("exec output should serialize as text"); assert_regex_match( r#"(?sx) - ^Command:\ /bin/zsh\ -lc\ 'rm\ -rf\ /tmp/example\.sqlite' - \nChunk\ ID:\ abc123 + ^Chunk\ ID:\ abc123 \nWall\ time:\ \d+\.\d{4}\ seconds \nProcess\ exited\ with\ code\ 0 \nOriginal\ token\ count:\ 10 diff --git a/codex-rs/core/src/tools/handlers/apply_patch.rs b/codex-rs/core/src/tools/handlers/apply_patch.rs index a972566b92..0be920feb3 100644 --- a/codex-rs/core/src/tools/handlers/apply_patch.rs +++ b/codex-rs/core/src/tools/handlers/apply_patch.rs @@ -23,6 +23,7 @@ use crate::tools::runtimes::apply_patch::ApplyPatchRuntime; use crate::tools::sandboxing::ToolCtx; use codex_apply_patch::ApplyPatchAction; use codex_apply_patch::ApplyPatchFileChange; +use codex_exec_server::ExecutorFileSystem; use codex_protocol::models::FileSystemPermissions; use codex_protocol::models::PermissionProfile; use codex_sandboxing::policy_transforms::effective_file_system_sandbox_policy; @@ -37,7 +38,7 @@ pub struct ApplyPatchHandler; fn file_paths_for_action(action: &ApplyPatchAction) -> Vec { let mut keys = Vec::new(); - let cwd = action.cwd.as_path(); + let cwd = &action.cwd; for (path, change) in action.changes() { if let Some(key) = to_abs_path(cwd, path) { @@ -55,14 +56,14 @@ fn file_paths_for_action(action: &ApplyPatchAction) -> Vec { keys } -fn to_abs_path(cwd: &Path, path: &Path) -> Option { +fn to_abs_path(cwd: &AbsolutePathBuf, path: &Path) -> Option { Some(AbsolutePathBuf::resolve_path_against_base(path, cwd)) } fn write_permissions_for_paths( file_paths: &[AbsolutePathBuf], file_system_sandbox_policy: &codex_protocol::permissions::FileSystemSandboxPolicy, - cwd: &Path, + cwd: &AbsolutePathBuf, ) -> Option { let write_paths = file_paths .iter() @@ -71,7 +72,9 @@ fn write_permissions_for_paths( .unwrap_or_else(|| path.clone()) .into_path_buf() }) - .filter(|path| !file_system_sandbox_policy.can_write_path_with_cwd(path.as_path(), cwd)) + .filter(|path| { + !file_system_sandbox_policy.can_write_path_with_cwd(path.as_path(), cwd.as_path()) + }) .collect::>() .into_iter() .map(AbsolutePathBuf::from_absolute_path) @@ -110,7 +113,7 @@ async fn effective_patch_permissions( let effective_additional_permissions = apply_granted_turn_permissions( session, crate::sandboxing::SandboxPermissions::UseDefault, - write_permissions_for_paths(&file_paths, &file_system_sandbox_policy, turn.cwd.as_path()), + write_permissions_for_paths(&file_paths, &file_system_sandbox_policy, &turn.cwd), ) .await; @@ -167,7 +170,14 @@ impl ToolHandler for ApplyPatchHandler { // Avoid building temporary ExecParams/command vectors; derive directly from inputs. let cwd = turn.cwd.clone(); let command = vec!["apply_patch".to_string(), patch_input.clone()]; - match codex_apply_patch::maybe_parse_apply_patch_verified(&command, &cwd) { + let Some(environment) = turn.environment.as_ref() else { + return Err(FunctionCallError::RespondToModel( + "apply_patch is unavailable in this session".to_string(), + )); + }; + let fs = environment.get_filesystem(); + match codex_apply_patch::maybe_parse_apply_patch_verified(&command, &cwd, fs.as_ref()).await + { codex_apply_patch::MaybeApplyPatchVerified::Body(changes) => { let (file_paths, effective_additional_permissions, file_system_sandbox_policy) = effective_patch_permissions(session.as_ref(), turn.as_ref(), &changes).await; @@ -254,7 +264,8 @@ impl ToolHandler for ApplyPatchHandler { #[allow(clippy::too_many_arguments)] pub(crate) async fn intercept_apply_patch( command: &[String], - cwd: &Path, + cwd: &AbsolutePathBuf, + fs: &dyn ExecutorFileSystem, timeout_ms: Option, session: Arc, turn: Arc, @@ -262,7 +273,7 @@ pub(crate) async fn intercept_apply_patch( call_id: &str, tool_name: &str, ) -> Result, FunctionCallError> { - match codex_apply_patch::maybe_parse_apply_patch_verified(command, cwd) { + match codex_apply_patch::maybe_parse_apply_patch_verified(command, cwd, fs).await { codex_apply_patch::MaybeApplyPatchVerified::Body(changes) => { session .record_model_warning( diff --git a/codex-rs/core/src/tools/handlers/apply_patch_tests.rs b/codex-rs/core/src/tools/handlers/apply_patch_tests.rs index 86ae586929..afe2e09db4 100644 --- a/codex-rs/core/src/tools/handlers/apply_patch_tests.rs +++ b/codex-rs/core/src/tools/handlers/apply_patch_tests.rs @@ -1,17 +1,21 @@ use super::*; use codex_apply_patch::MaybeApplyPatchVerified; +use codex_exec_server::LOCAL_FS; use codex_protocol::permissions::FileSystemSandboxPolicy; use codex_protocol::protocol::SandboxPolicy; +use core_test_support::PathBufExt; +use core_test_support::PathExt; use pretty_assertions::assert_eq; use tempfile::TempDir; -#[test] -fn approval_keys_include_move_destination() { +#[tokio::test] +async fn approval_keys_include_move_destination() { let tmp = TempDir::new().expect("tmp"); - let cwd = tmp.path(); - std::fs::create_dir_all(cwd.join("old")).expect("create old dir"); - std::fs::create_dir_all(cwd.join("renamed/dir")).expect("create dest dir"); - std::fs::write(cwd.join("old/name.txt"), "old content\n").expect("write old file"); + let cwd_path = tmp.path(); + let cwd = cwd_path.abs(); + std::fs::create_dir_all(cwd_path.join("old")).expect("create old dir"); + std::fs::create_dir_all(cwd_path.join("renamed/dir")).expect("create dest dir"); + std::fs::write(cwd_path.join("old/name.txt"), "old content\n").expect("write old file"); let patch = r#"*** Begin Patch *** Update File: old/name.txt *** Move to: renamed/dir/name.txt @@ -20,10 +24,13 @@ fn approval_keys_include_move_destination() { +new content *** End Patch"#; let argv = vec!["apply_patch".to_string(), patch.to_string()]; - let action = match codex_apply_patch::maybe_parse_apply_patch_verified(&argv, cwd) { - MaybeApplyPatchVerified::Body(action) => action, - other => panic!("expected patch body, got: {other:?}"), - }; + let action = + match codex_apply_patch::maybe_parse_apply_patch_verified(&argv, &cwd, LOCAL_FS.as_ref()) + .await + { + MaybeApplyPatchVerified::Body(action) => action, + other => panic!("expected patch body, got: {other:?}"), + }; let keys = file_paths_for_action(&action); assert_eq!(keys.len(), 2); @@ -32,8 +39,9 @@ fn approval_keys_include_move_destination() { #[test] fn write_permissions_for_paths_skip_dirs_already_writable_under_workspace_root() { let tmp = TempDir::new().expect("tmp"); - let cwd = tmp.path(); - let nested = cwd.join("nested"); + let cwd_path = tmp.path(); + let cwd = cwd_path.abs(); + let nested = cwd_path.join("nested"); std::fs::create_dir_all(&nested).expect("create nested dir"); let file_path = AbsolutePathBuf::try_from(nested.join("file.txt")) .expect("nested file path should be absolute"); @@ -45,7 +53,7 @@ fn write_permissions_for_paths_skip_dirs_already_writable_under_workspace_root() exclude_slash_tmp: false, }); - let permissions = write_permissions_for_paths(&[file_path], &sandbox_policy, cwd); + let permissions = write_permissions_for_paths(&[file_path], &sandbox_policy, &cwd); assert_eq!(permissions, None); } @@ -59,6 +67,7 @@ fn write_permissions_for_paths_keep_dirs_outside_workspace_root() { std::fs::create_dir_all(&outside).expect("create outside dir"); let file_path = AbsolutePathBuf::try_from(outside.join("file.txt")) .expect("outside file path should be absolute"); + let cwd_abs = cwd.abs(); let sandbox_policy = FileSystemSandboxPolicy::from(&SandboxPolicy::WorkspaceWrite { writable_roots: vec![], read_only_access: Default::default(), @@ -67,11 +76,9 @@ fn write_permissions_for_paths_keep_dirs_outside_workspace_root() { exclude_slash_tmp: true, }); - let permissions = write_permissions_for_paths(&[file_path], &sandbox_policy, &cwd); - let expected_outside = AbsolutePathBuf::from_absolute_path(dunce::simplified( - &outside.canonicalize().expect("canonicalize outside dir"), - )) - .expect("outside dir should be absolute"); + let permissions = write_permissions_for_paths(&[file_path], &sandbox_policy, &cwd_abs); + let expected_outside = + dunce::simplified(&outside.canonicalize().expect("canonicalize outside dir")).abs(); assert_eq!( permissions.and_then(|profile| profile.file_system.and_then(|fs| fs.write)), diff --git a/codex-rs/core/src/tools/handlers/shell.rs b/codex-rs/core/src/tools/handlers/shell.rs index 6f662ff493..e835566247 100644 --- a/codex-rs/core/src/tools/handlers/shell.rs +++ b/codex-rs/core/src/tools/handlers/shell.rs @@ -39,6 +39,7 @@ use codex_protocol::models::PermissionProfile; use codex_protocol::protocol::ExecCommandSource; use codex_shell_command::is_safe_command::is_known_safe_command; use codex_tools::ShellCommandBackendConfig; +use codex_utils_absolute_path::AbsolutePathBuf; pub struct ShellHandler; @@ -395,6 +396,13 @@ impl ShellHandler { } = args; let mut exec_params = exec_params; + let Some(environment) = turn.environment.as_ref() else { + return Err(FunctionCallError::RespondToModel( + "shell is unavailable in this session".to_string(), + )); + }; + let fs = environment.get_filesystem(); + let dependency_env = session.dependency_env().await; if !dependency_env.is_empty() { exec_params.env.extend(dependency_env.clone()); @@ -458,9 +466,16 @@ impl ShellHandler { } // Intercept apply_patch if present. + let apply_patch_cwd = + AbsolutePathBuf::from_absolute_path(&exec_params.cwd).map_err(|err| { + FunctionCallError::RespondToModel(format!( + "apply_patch verification failed: failed to resolve cwd: {err}" + )) + })?; if let Some(output) = intercept_apply_patch( &exec_params.command, - &exec_params.cwd, + &apply_patch_cwd, + fs.as_ref(), exec_params.expiration.timeout_ms(), session.clone(), turn.clone(), diff --git a/codex-rs/core/src/tools/handlers/unified_exec.rs b/codex-rs/core/src/tools/handlers/unified_exec.rs index 4486cd7917..b83068c66c 100644 --- a/codex-rs/core/src/tools/handlers/unified_exec.rs +++ b/codex-rs/core/src/tools/handlers/unified_exec.rs @@ -30,6 +30,7 @@ use codex_protocol::protocol::EventMsg; use codex_protocol::protocol::TerminalInteractionEvent; use codex_shell_command::is_safe_command::is_known_safe_command; use codex_tools::UnifiedExecShellMode; +use codex_utils_absolute_path::AbsolutePathBuf; use serde::Deserialize; use std::path::PathBuf; use std::sync::Arc; @@ -176,6 +177,13 @@ impl ToolHandler for UnifiedExecHandler { } }; + let Some(environment) = turn.environment.as_ref() else { + return Err(FunctionCallError::RespondToModel( + "unified exec is unavailable in this session".to_string(), + )); + }; + let fs = environment.get_filesystem(); + let manager: &UnifiedExecProcessManager = &session.services.unified_exec_manager; let context = UnifiedExecContext::new(session.clone(), turn.clone(), call_id.clone()); @@ -274,9 +282,19 @@ impl ToolHandler for UnifiedExecHandler { } }; + let apply_patch_cwd = match AbsolutePathBuf::from_absolute_path(&cwd) { + Ok(cwd) => cwd, + Err(err) => { + manager.release_process_id(process_id).await; + return Err(FunctionCallError::RespondToModel(format!( + "apply_patch verification failed: failed to resolve cwd: {err}" + ))); + } + }; if let Some(output) = intercept_apply_patch( &command, - &cwd, + &apply_patch_cwd, + fs.as_ref(), Some(yield_time_ms), context.session.clone(), context.turn.clone(), diff --git a/codex-rs/core/src/tools/runtimes/apply_patch.rs b/codex-rs/core/src/tools/runtimes/apply_patch.rs index 2be859d79e..03e66f4ec6 100644 --- a/codex-rs/core/src/tools/runtimes/apply_patch.rs +++ b/codex-rs/core/src/tools/runtimes/apply_patch.rs @@ -55,18 +55,10 @@ impl ApplyPatchRuntime { fn build_guardian_review_request( req: &ApplyPatchRequest, call_id: &str, - fallback_cwd: &AbsolutePathBuf, ) -> GuardianApprovalRequest { - let cwd = match AbsolutePathBuf::try_from(req.action.cwd.as_path()) { - Ok(cwd) => cwd, - Err(err) => { - tracing::warn!("apply_patch cwd is not absolute: {err}"); - fallback_cwd.clone() - } - }; GuardianApprovalRequest::ApplyPatch { id: call_id.to_string(), - cwd, + cwd: req.action.cwd.clone(), files: req.file_paths.clone(), patch: req.action.patch.clone(), } @@ -109,7 +101,7 @@ impl ApplyPatchRuntime { CODEX_CORE_APPLY_PATCH_ARG1.to_string(), req.action.patch.clone(), ], - cwd: req.action.cwd.clone(), + cwd: req.action.cwd.to_path_buf(), // Run apply_patch with a minimal environment for determinism and to avoid leaks. env: HashMap::new(), additional_permissions: req.additional_permissions.clone(), @@ -157,8 +149,7 @@ impl Approvable for ApplyPatchRuntime { return ReviewDecision::Approved; } if routes_approval_to_guardian(turn) { - let action = - ApplyPatchRuntime::build_guardian_review_request(req, ctx.call_id, &turn.cwd); + let action = ApplyPatchRuntime::build_guardian_review_request(req, ctx.call_id); return review_approval_request(session, turn, action, retry_reason).await; } if let Some(reason) = retry_reason { diff --git a/codex-rs/core/src/tools/runtimes/apply_patch_tests.rs b/codex-rs/core/src/tools/runtimes/apply_patch_tests.rs index 874e171f11..e1fc4ee475 100644 --- a/codex-rs/core/src/tools/runtimes/apply_patch_tests.rs +++ b/codex-rs/core/src/tools/runtimes/apply_patch_tests.rs @@ -1,5 +1,6 @@ use super::*; use codex_protocol::protocol::GranularApprovalConfig; +use core_test_support::PathBufExt; use pretty_assertions::assert_eq; use std::collections::HashMap; #[cfg(not(target_os = "windows"))] @@ -31,18 +32,17 @@ fn wants_no_sandbox_approval_granular_respects_sandbox_flag() { #[test] fn guardian_review_request_includes_patch_context() { - let path = std::env::temp_dir().join("guardian-apply-patch-test.txt"); + let path = std::env::temp_dir() + .join("guardian-apply-patch-test.txt") + .abs(); let action = ApplyPatchAction::new_add_for_test(&path, "hello".to_string()); - let expected_cwd = - AbsolutePathBuf::from_absolute_path(&action.cwd).expect("cwd should be absolute"); + let expected_cwd = action.cwd.clone(); let expected_patch = action.patch.clone(); let request = ApplyPatchRequest { action, - file_paths: vec![ - AbsolutePathBuf::from_absolute_path(&path).expect("temp path should be absolute"), - ], + file_paths: vec![path.clone()], changes: HashMap::from([( - path, + path.to_path_buf(), FileChange::Add { content: "hello".to_string(), }, @@ -56,8 +56,7 @@ fn guardian_review_request_includes_patch_context() { timeout_ms: None, }; - let guardian_request = - ApplyPatchRuntime::build_guardian_review_request(&request, "call-1", &expected_cwd); + let guardian_request = ApplyPatchRuntime::build_guardian_review_request(&request, "call-1"); assert_eq!( guardian_request, @@ -73,15 +72,15 @@ fn guardian_review_request_includes_patch_context() { #[cfg(not(target_os = "windows"))] #[test] fn build_sandbox_command_prefers_configured_codex_self_exe_for_apply_patch() { - let path = std::env::temp_dir().join("apply-patch-current-exe-test.txt"); + let path = std::env::temp_dir() + .join("apply-patch-current-exe-test.txt") + .abs(); let action = ApplyPatchAction::new_add_for_test(&path, "hello".to_string()); let request = ApplyPatchRequest { action, - file_paths: vec![ - AbsolutePathBuf::from_absolute_path(&path).expect("temp path should be absolute"), - ], + file_paths: vec![path.clone()], changes: HashMap::from([( - path, + path.to_path_buf(), FileChange::Add { content: "hello".to_string(), }, @@ -105,15 +104,15 @@ fn build_sandbox_command_prefers_configured_codex_self_exe_for_apply_patch() { #[cfg(not(target_os = "windows"))] #[test] fn build_sandbox_command_falls_back_to_current_exe_for_apply_patch() { - let path = std::env::temp_dir().join("apply-patch-current-exe-test.txt"); + let path = std::env::temp_dir() + .join("apply-patch-current-exe-test.txt") + .abs(); let action = ApplyPatchAction::new_add_for_test(&path, "hello".to_string()); let request = ApplyPatchRequest { action, - file_paths: vec![ - AbsolutePathBuf::from_absolute_path(&path).expect("temp path should be absolute"), - ], + file_paths: vec![path.clone()], changes: HashMap::from([( - path, + path.to_path_buf(), FileChange::Add { content: "hello".to_string(), }, diff --git a/codex-rs/core/tests/common/lib.rs b/codex-rs/core/tests/common/lib.rs index 883d3503d1..d16054e6db 100644 --- a/codex-rs/core/tests/common/lib.rs +++ b/codex-rs/core/tests/common/lib.rs @@ -13,8 +13,9 @@ use codex_core::config::Config; use codex_core::config::ConfigBuilder; use codex_core::config::ConfigOverrides; use codex_utils_absolute_path::AbsolutePathBuf; +pub use codex_utils_absolute_path::test_support::PathBufExt; +pub use codex_utils_absolute_path::test_support::PathExt; use regex_lite::Regex; -use std::path::Path; use std::path::PathBuf; pub mod apps_test_server; @@ -105,36 +106,6 @@ pub fn test_absolute_path(unix_path: &str) -> AbsolutePathBuf { test_absolute_path_with_windows(unix_path, /*windows_path*/ None) } -pub trait PathExt { - fn abs(&self) -> AbsolutePathBuf; -} - -impl PathExt for Path { - fn abs(&self) -> AbsolutePathBuf { - if let Ok(path) = AbsolutePathBuf::try_from(self.to_path_buf()) { - return path; - } - if cfg!(windows) - && let Some(path) = self.to_str() - && path.starts_with('/') - { - return AbsolutePathBuf::try_from(test_path_buf(path)) - .expect("windows test path should be absolute"); - } - panic!("path should already be absolute"); - } -} - -pub trait PathBufExt { - fn abs(&self) -> AbsolutePathBuf; -} - -impl PathBufExt for PathBuf { - fn abs(&self) -> AbsolutePathBuf { - self.as_path().abs() - } -} - pub trait TempDirExt { fn abs(&self) -> AbsolutePathBuf; } diff --git a/codex-rs/core/tests/suite/unified_exec.rs b/codex-rs/core/tests/suite/unified_exec.rs index a165db0451..7470717a04 100644 --- a/codex-rs/core/tests/suite/unified_exec.rs +++ b/codex-rs/core/tests/suite/unified_exec.rs @@ -1,6 +1,7 @@ use std::collections::HashMap; use std::ffi::OsStr; use std::fs; +use std::sync::OnceLock; use anyhow::Context; use anyhow::Result; @@ -33,6 +34,7 @@ use core_test_support::wait_for_event; use core_test_support::wait_for_event_match; use core_test_support::wait_for_event_with_timeout; use pretty_assertions::assert_eq; +use regex_lite::Regex; use serde_json::Value; use serde_json::json; use tokio::time::Duration; @@ -57,49 +59,65 @@ struct ParsedUnifiedExecOutput { #[allow(clippy::expect_used)] fn parse_unified_exec_output(raw: &str) -> Result { - let cleaned = raw.replace("\r\n", "\n"); - let (metadata, output) = cleaned - .rsplit_once("\nOutput:") + static OUTPUT_REGEX: OnceLock = OnceLock::new(); + let regex = OUTPUT_REGEX.get_or_init(|| { + Regex::new(concat!( + r#"(?s)^(?:Total output lines: \d+\n\n)?"#, + r#"(?:Chunk ID: (?P[^\n]+)\n)?"#, + r#"Wall time: (?P-?\d+(?:\.\d+)?) seconds\n"#, + r#"(?:Process exited with code (?P-?\d+)\n)?"#, + r#"(?:Process running with session ID (?P-?\d+)\n)?"#, + r#"(?:Original token count: (?P\d+)\n)?"#, + r#"Output:\n?(?P.*)$"#, + )) + .expect("valid unified exec output regex") + }); + + let cleaned = raw.trim_matches('\r'); + let captures = regex + .captures(cleaned) .ok_or_else(|| anyhow::anyhow!("missing Output section in unified exec output {raw}"))?; - let output = output.strip_prefix('\n').unwrap_or(output); - let mut chunk_id = None; - let mut wall_time_seconds = None; - let mut process_id = None; - let mut exit_code = None; - let mut original_token_count = None; + let chunk_id = captures + .name("chunk_id") + .map(|value| value.as_str().to_string()); - for line in metadata.lines() { - if let Some(value) = line.strip_prefix("Chunk ID: ") { - chunk_id = Some(value.to_string()); - } else if let Some(value) = line.strip_prefix("Wall time: ") { - let value = value.strip_suffix(" seconds").ok_or_else(|| { - anyhow::anyhow!("invalid wall time line in unified exec output: {line}") - })?; - wall_time_seconds = Some( - value - .parse::() - .context("failed to parse wall time seconds")?, - ); - } else if let Some(value) = line.strip_prefix("Process exited with code ") { - exit_code = Some( - value - .parse::() - .context("failed to parse exit code from unified exec output")?, - ); - } else if let Some(value) = line.strip_prefix("Process running with session ID ") { - process_id = Some(value.to_string()); - } else if let Some(value) = line.strip_prefix("Original token count: ") { - original_token_count = Some( - value - .parse::() - .context("failed to parse original token count from unified exec output")?, - ); - } - } + let wall_time_seconds = captures + .name("wall_time") + .expect("wall_time group present") + .as_str() + .parse::() + .context("failed to parse wall time seconds")?; - let wall_time_seconds = wall_time_seconds - .ok_or_else(|| anyhow::anyhow!("missing wall time in unified exec output {raw}"))?; + let exit_code = captures + .name("exit_code") + .map(|value| { + value + .as_str() + .parse::() + .context("failed to parse exit code from unified exec output") + }) + .transpose()?; + + let process_id = captures + .name("process_id") + .map(|value| value.as_str().to_string()); + + let original_token_count = captures + .name("original_token_count") + .map(|value| { + value + .as_str() + .parse::() + .context("failed to parse original token count from unified exec output") + }) + .transpose()?; + + let output = captures + .name("output") + .expect("output group present") + .as_str() + .to_string(); Ok(ParsedUnifiedExecOutput { chunk_id, @@ -107,7 +125,7 @@ fn parse_unified_exec_output(raw: &str) -> Result { process_id, exit_code, original_token_count, - output: output.to_string(), + output, }) } @@ -2223,22 +2241,8 @@ PY let large_output = outputs.get(call_id).expect("missing large output summary"); let output_text = large_output.output.replace("\r\n", "\n"); - assert!( - output_text.starts_with("Total output lines: "), - "expected large output summary header, got {output_text:?}" - ); - assert!( - output_text.contains("…") && output_text.contains("tokens truncated"), - "expected truncation marker in large output summary, got {output_text:?}" - ); - assert!( - output_text.contains("token token \ntoken token \ntoken token \n"), - "expected preserved output prefix in large output summary, got {output_text:?}" - ); - assert!( - output_text.ends_with("token token ") || output_text.ends_with("token token \n"), - "expected preserved output suffix in large output summary, got {output_text:?}" - ); + let truncated_pattern = r"(?s)^Total output lines: \d+\n\n(token token \n){5,}.*…\d+ tokens truncated….*(token token \n){5,}$"; + assert_regex_match(truncated_pattern, &output_text); let original_tokens = large_output .original_token_count @@ -2323,7 +2327,7 @@ async fn unified_exec_runs_under_sandbox() -> Result<()> { let outputs = collect_tool_outputs(&bodies)?; let output = outputs.get(call_id).expect("missing output"); - assert_eq!(output.output.trim_end_matches(['\r', '\n']), "hello"); + assert_regex_match("hello[\r\n]+", &output.output); Ok(()) } diff --git a/codex-rs/exec-server/src/file_system.rs b/codex-rs/exec-server/src/file_system.rs index 35c2243f8e..b04b480893 100644 --- a/codex-rs/exec-server/src/file_system.rs +++ b/codex-rs/exec-server/src/file_system.rs @@ -39,6 +39,12 @@ pub type FileSystemResult = io::Result; pub trait ExecutorFileSystem: Send + Sync { async fn read_file(&self, path: &AbsolutePathBuf) -> FileSystemResult>; + /// Reads a file and decodes it as UTF-8 text. + async fn read_file_text(&self, path: &AbsolutePathBuf) -> FileSystemResult { + let bytes = self.read_file(path).await?; + String::from_utf8(bytes).map_err(|err| io::Error::new(io::ErrorKind::InvalidData, err)) + } + async fn write_file(&self, path: &AbsolutePathBuf, contents: Vec) -> FileSystemResult<()>; async fn create_directory( diff --git a/codex-rs/exec-server/tests/file_system.rs b/codex-rs/exec-server/tests/file_system.rs index dea47e8fcc..ca55587f5a 100644 --- a/codex-rs/exec-server/tests/file_system.rs +++ b/codex-rs/exec-server/tests/file_system.rs @@ -122,6 +122,12 @@ async fn file_system_methods_cover_surface_area(use_remote: bool) -> Result<()> .with_context(|| format!("mode={use_remote}"))?; assert_eq!(nested_file_contents, b"hello from trait"); + let nested_file_text = file_system + .read_file_text(&absolute_path(nested_file.clone())) + .await + .with_context(|| format!("mode={use_remote}"))?; + assert_eq!(nested_file_text, "hello from trait"); + file_system .copy( &absolute_path(nested_file), diff --git a/codex-rs/exec/tests/event_processor_with_json_output.rs b/codex-rs/exec/tests/event_processor_with_json_output.rs index a6b25b66b8..bb29366561 100644 --- a/codex-rs/exec/tests/event_processor_with_json_output.rs +++ b/codex-rs/exec/tests/event_processor_with_json_output.rs @@ -482,6 +482,7 @@ fn mcp_tool_call_begin_and_end_emit_item_events() { result: Some(McpToolCallResult { content: Vec::new(), structured_content: None, + meta: None, }), error: None, duration_ms: Some(1_000), @@ -613,6 +614,7 @@ fn mcp_tool_call_defaults_arguments_and_preserves_structured_content() { "text": "done", })], structured_content: Some(json!({ "status": "ok" })), + meta: None, }), error: None, duration_ms: Some(10), diff --git a/codex-rs/tui/src/test_support.rs b/codex-rs/tui/src/test_support.rs index 1d63c95065..ae2e4c714d 100644 --- a/codex-rs/tui/src/test_support.rs +++ b/codex-rs/tui/src/test_support.rs @@ -1,37 +1,6 @@ -use codex_utils_absolute_path::AbsolutePathBuf; -use std::path::Path; +pub(crate) use codex_utils_absolute_path::test_support::PathBufExt; use std::path::PathBuf; -pub(crate) trait PathExt { - fn abs(&self) -> AbsolutePathBuf; -} - -impl PathExt for Path { - fn abs(&self) -> AbsolutePathBuf { - if let Ok(path) = AbsolutePathBuf::try_from(self.to_path_buf()) { - return path; - } - if cfg!(windows) - && let Some(path) = self.to_str() - && path.starts_with('/') - { - return AbsolutePathBuf::try_from(test_path_buf(path)) - .expect("windows test path should be absolute"); - } - panic!("path should already be absolute"); - } -} - -pub(crate) trait PathBufExt { - fn abs(&self) -> AbsolutePathBuf; -} - -impl PathBufExt for PathBuf { - fn abs(&self) -> AbsolutePathBuf { - self.as_path().abs() - } -} - pub(crate) fn test_path_display(path: &str) -> String { test_path_buf(path).abs().display().to_string() } diff --git a/codex-rs/utils/absolute-path/src/lib.rs b/codex-rs/utils/absolute-path/src/lib.rs index 8e222f05a5..ca76d52cf3 100644 --- a/codex-rs/utils/absolute-path/src/lib.rs +++ b/codex-rs/utils/absolute-path/src/lib.rs @@ -130,6 +130,54 @@ impl From for PathBuf { } } +/// Helpers for constructing absolute paths in tests. +pub mod test_support { + use super::AbsolutePathBuf; + use std::path::Path; + use std::path::PathBuf; + + /// Extension methods for converting paths into [`AbsolutePathBuf`] values in tests. + pub trait PathExt { + /// Converts an already absolute path into an [`AbsolutePathBuf`]. + fn abs(&self) -> AbsolutePathBuf; + } + + impl PathExt for Path { + #[expect(clippy::expect_used)] + fn abs(&self) -> AbsolutePathBuf { + if let Ok(path) = AbsolutePathBuf::try_from(self) { + return path; + } + if cfg!(windows) + && let Some(path) = self.to_str() + && path.starts_with('/') + { + let mut windows_path = PathBuf::from(r"C:\"); + windows_path.extend( + path.trim_start_matches('/') + .split('/') + .filter(|segment| !segment.is_empty()), + ); + return AbsolutePathBuf::try_from(windows_path) + .expect("windows test path should be absolute"); + } + panic!("path should already be absolute"); + } + } + + /// Extension methods for converting path buffers into [`AbsolutePathBuf`] values in tests. + pub trait PathBufExt { + /// Converts an already absolute path buffer into an [`AbsolutePathBuf`]. + fn abs(&self) -> AbsolutePathBuf; + } + + impl PathBufExt for PathBuf { + fn abs(&self) -> AbsolutePathBuf { + self.as_path().abs() + } + } +} + impl TryFrom<&Path> for AbsolutePathBuf { type Error = std::io::Error;