mirror of
https://github.com/openai/codex.git
synced 2026-11-11 21:10:39 +00:00
Merge remote-tracking branch 'origin/main' into pakrym/core-cwd-absolutepathbuf
# Conflicts: # codex-rs/core/src/tools/runtimes/apply_patch.rs # codex-rs/core/src/tools/runtimes/apply_patch_tests.rs # codex-rs/core/tests/common/lib.rs # codex-rs/tui/src/test_support.rs
This commit is contained in:
Generated
+5
@@ -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",
|
||||
|
||||
@@ -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"
|
||||
},
|
||||
|
||||
@@ -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"
|
||||
|
||||
@@ -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"
|
||||
|
||||
+22
-21
@@ -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"
|
||||
|
||||
@@ -16,7 +16,7 @@
|
||||
"type": "array"
|
||||
},
|
||||
"watchId": {
|
||||
"description": "Watch identifier returned by `fs/watch`.",
|
||||
"description": "Watch identifier previously provided to `fs/watch`.",
|
||||
"type": "string"
|
||||
}
|
||||
},
|
||||
|
||||
@@ -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"
|
||||
}
|
||||
},
|
||||
|
||||
@@ -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"
|
||||
|
||||
@@ -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"
|
||||
|
||||
@@ -294,6 +294,7 @@
|
||||
},
|
||||
"McpToolCallResult": {
|
||||
"properties": {
|
||||
"_meta": true,
|
||||
"content": {
|
||||
"items": true,
|
||||
"type": "array"
|
||||
|
||||
@@ -294,6 +294,7 @@
|
||||
},
|
||||
"McpToolCallResult": {
|
||||
"properties": {
|
||||
"_meta": true,
|
||||
"content": {
|
||||
"items": true,
|
||||
"type": "array"
|
||||
|
||||
@@ -430,6 +430,7 @@
|
||||
},
|
||||
"McpToolCallResult": {
|
||||
"properties": {
|
||||
"_meta": true,
|
||||
"content": {
|
||||
"items": true,
|
||||
"type": "array"
|
||||
|
||||
@@ -518,6 +518,7 @@
|
||||
},
|
||||
"McpToolCallResult": {
|
||||
"properties": {
|
||||
"_meta": true,
|
||||
"content": {
|
||||
"items": true,
|
||||
"type": "array"
|
||||
|
||||
@@ -456,6 +456,7 @@
|
||||
},
|
||||
"McpToolCallResult": {
|
||||
"properties": {
|
||||
"_meta": true,
|
||||
"content": {
|
||||
"items": true,
|
||||
"type": "array"
|
||||
|
||||
@@ -456,6 +456,7 @@
|
||||
},
|
||||
"McpToolCallResult": {
|
||||
"properties": {
|
||||
"_meta": true,
|
||||
"content": {
|
||||
"items": true,
|
||||
"type": "array"
|
||||
|
||||
@@ -456,6 +456,7 @@
|
||||
},
|
||||
"McpToolCallResult": {
|
||||
"properties": {
|
||||
"_meta": true,
|
||||
"content": {
|
||||
"items": true,
|
||||
"type": "array"
|
||||
|
||||
@@ -518,6 +518,7 @@
|
||||
},
|
||||
"McpToolCallResult": {
|
||||
"properties": {
|
||||
"_meta": true,
|
||||
"content": {
|
||||
"items": true,
|
||||
"type": "array"
|
||||
|
||||
@@ -456,6 +456,7 @@
|
||||
},
|
||||
"McpToolCallResult": {
|
||||
"properties": {
|
||||
"_meta": true,
|
||||
"content": {
|
||||
"items": true,
|
||||
"type": "array"
|
||||
|
||||
@@ -518,6 +518,7 @@
|
||||
},
|
||||
"McpToolCallResult": {
|
||||
"properties": {
|
||||
"_meta": true,
|
||||
"content": {
|
||||
"items": true,
|
||||
"type": "array"
|
||||
|
||||
@@ -456,6 +456,7 @@
|
||||
},
|
||||
"McpToolCallResult": {
|
||||
"properties": {
|
||||
"_meta": true,
|
||||
"content": {
|
||||
"items": true,
|
||||
"type": "array"
|
||||
|
||||
@@ -456,6 +456,7 @@
|
||||
},
|
||||
"McpToolCallResult": {
|
||||
"properties": {
|
||||
"_meta": true,
|
||||
"content": {
|
||||
"items": true,
|
||||
"type": "array"
|
||||
|
||||
@@ -430,6 +430,7 @@
|
||||
},
|
||||
"McpToolCallResult": {
|
||||
"properties": {
|
||||
"_meta": true,
|
||||
"content": {
|
||||
"items": true,
|
||||
"type": "array"
|
||||
|
||||
@@ -430,6 +430,7 @@
|
||||
},
|
||||
"McpToolCallResult": {
|
||||
"properties": {
|
||||
"_meta": true,
|
||||
"content": {
|
||||
"items": true,
|
||||
"type": "array"
|
||||
|
||||
@@ -430,6 +430,7 @@
|
||||
},
|
||||
"McpToolCallResult": {
|
||||
"properties": {
|
||||
"_meta": true,
|
||||
"content": {
|
||||
"items": true,
|
||||
"type": "array"
|
||||
|
||||
@@ -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,
|
||||
/**
|
||||
|
||||
@@ -7,6 +7,6 @@
|
||||
*/
|
||||
export type FsUnwatchParams = {
|
||||
/**
|
||||
* Watch identifier returned by `fs/watch`.
|
||||
* Watch identifier previously provided to `fs/watch`.
|
||||
*/
|
||||
watchId: string, };
|
||||
|
||||
@@ -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.
|
||||
*/
|
||||
|
||||
@@ -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.
|
||||
*/
|
||||
|
||||
@@ -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<JsonValue>, structuredContent: JsonValue | null, };
|
||||
export type McpToolCallResult = { content: Array<JsonValue>, structuredContent: JsonValue | null, _meta: JsonValue | null, };
|
||||
|
||||
@@ -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")
|
||||
}
|
||||
}),
|
||||
|
||||
@@ -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::<Vec<_>>();
|
||||
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![
|
||||
|
||||
@@ -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<AbsolutePathBuf>,
|
||||
@@ -5048,6 +5048,9 @@ pub struct McpToolCallResult {
|
||||
// representations). Using `JsonValue` keeps the payload wire-shaped and easy to export.
|
||||
pub content: Vec<JsonValue>,
|
||||
pub structured_content: Option<JsonValue>,
|
||||
#[serde(rename = "_meta")]
|
||||
#[ts(rename = "_meta")]
|
||||
pub meta: Option<JsonValue>,
|
||||
}
|
||||
|
||||
#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema, TS)]
|
||||
|
||||
@@ -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": {
|
||||
|
||||
@@ -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),
|
||||
|
||||
@@ -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<FsWatchResponse, JSONRPCErrorError> {
|
||||
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<_>>(),
|
||||
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<_>>(),
|
||||
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));
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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();
|
||||
|
||||
@@ -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::<ConnectionId, OutboundConnectionState>::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();
|
||||
|
||||
@@ -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>,
|
||||
config_warnings: Arc<Vec<ConfigWarningNotification>>,
|
||||
rpc_transport: AppServerRpcTransport,
|
||||
remote_control_handle: Option<RemoteControlHandle>,
|
||||
}
|
||||
|
||||
#[derive(Clone, Debug, Default)]
|
||||
@@ -195,6 +197,7 @@ pub(crate) struct MessageProcessorArgs {
|
||||
pub(crate) session_source: SessionSource,
|
||||
pub(crate) auth_manager: Arc<AuthManager>,
|
||||
pub(crate) rpc_transport: AppServerRpcTransport,
|
||||
pub(crate) remote_control_handle: Option<RemoteControlHandle>,
|
||||
}
|
||||
|
||||
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,
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -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<oneshot::Sender<()>>,
|
||||
}
|
||||
|
||||
#[derive(Clone)]
|
||||
pub(crate) struct RemoteControlHandle {
|
||||
enabled_tx: Arc<watch::Sender<bool>>,
|
||||
}
|
||||
|
||||
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<Arc<StateRuntime>>,
|
||||
@@ -36,21 +52,38 @@ pub(crate) async fn start_remote_control(
|
||||
transport_event_tx: mpsc::Sender<TransportEvent>,
|
||||
shutdown_token: CancellationToken,
|
||||
app_server_client_name_rx: Option<oneshot::Receiver<String>>,
|
||||
) -> io::Result<JoinHandle<()>> {
|
||||
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(
|
||||
|
||||
@@ -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::<TransportEvent>(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::<TransportEvent>(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::<TransportEvent>(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::<TransportEvent>(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::<TransportEvent>(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::<TransportEvent>(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::<TransportEvent>(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::<TransportEvent>(CHANNEL_CAPACITY);
|
||||
let (app_server_client_name_tx, app_server_client_name_rx) = oneshot::channel::<String>();
|
||||
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::<TransportEvent>(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::<TransportEvent>(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)]
|
||||
|
||||
@@ -111,7 +111,8 @@ struct WebsocketState {
|
||||
}
|
||||
|
||||
pub(crate) struct RemoteControlWebsocket {
|
||||
remote_control_target: RemoteControlTarget,
|
||||
remote_control_url: String,
|
||||
remote_control_target: Option<RemoteControlTarget>,
|
||||
state_db: Option<Arc<StateRuntime>>,
|
||||
auth_manager: Arc<AuthManager>,
|
||||
shutdown_token: CancellationToken,
|
||||
@@ -122,15 +123,24 @@ pub(crate) struct RemoteControlWebsocket {
|
||||
state: Arc<Mutex<WebsocketState>>,
|
||||
server_event_rx: Arc<Mutex<mpsc::Receiver<super::QueuedServerEnvelope>>>,
|
||||
used_rx: watch::Receiver<usize>,
|
||||
enabled_rx: watch::Receiver<bool>,
|
||||
}
|
||||
|
||||
enum ConnectOutcome {
|
||||
Connected(Box<WebSocketStream<MaybeTlsStream<TcpStream>>>),
|
||||
Disabled,
|
||||
Shutdown,
|
||||
}
|
||||
|
||||
impl RemoteControlWebsocket {
|
||||
pub(crate) fn new(
|
||||
remote_control_target: RemoteControlTarget,
|
||||
remote_control_url: String,
|
||||
remote_control_target: Option<RemoteControlTarget>,
|
||||
state_db: Option<Arc<StateRuntime>>,
|
||||
auth_manager: Arc<AuthManager>,
|
||||
transport_event_tx: mpsc::Sender<TransportEvent>,
|
||||
shutdown_token: CancellationToken,
|
||||
enabled_rx: watch::Receiver<bool>,
|
||||
) -> 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<WebSocketStream<MaybeTlsStream<TcpStream>>> {
|
||||
) -> 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
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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 }
|
||||
|
||||
|
||||
@@ -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()
|
||||
|
||||
+325
-134
@@ -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::<Vec<&Path>>();
|
||||
|
||||
// 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<PathBuf>,
|
||||
pub modified: Vec<PathBuf>,
|
||||
@@ -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<AffectedPaths> {
|
||||
async fn apply_hunks_to_files(
|
||||
hunks: &[Hunk],
|
||||
cwd: &AbsolutePathBuf,
|
||||
fs: &dyn ExecutorFileSystem,
|
||||
) -> anyhow::Result<AffectedPaths> {
|
||||
if hunks.is_empty() {
|
||||
anyhow::bail!("No files were modified.");
|
||||
}
|
||||
@@ -285,48 +266,97 @@ fn apply_hunks_to_files(hunks: &[Hunk]) -> anyhow::Result<AffectedPaths> {
|
||||
let mut modified: Vec<PathBuf> = Vec::new();
|
||||
let mut deleted: Vec<PathBuf> = 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<AppliedPatch, ApplyPatchError> {
|
||||
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<String> = 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<ApplyPatchFileUpdate, ApplyPatchError> {
|
||||
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<ApplyPatchFileUpdate, ApplyPatchError> {
|
||||
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());
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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();
|
||||
|
||||
@@ -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<assert_cmd::assert::Assert> {
|
||||
@@ -16,9 +17,14 @@ fn apply_patch_command(dir: &Path) -> anyhow::Result<Command> {
|
||||
Ok(cmd)
|
||||
}
|
||||
|
||||
fn resolved_under(root: &Path, path: &str) -> anyhow::Result<PathBuf> {
|
||||
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");
|
||||
|
||||
|
||||
@@ -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 }
|
||||
|
||||
@@ -99,7 +99,24 @@ pub fn arg0_dispatch() -> Option<Arg0PathEntryGuard> {
|
||||
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,
|
||||
}
|
||||
|
||||
@@ -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()
|
||||
})
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -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(),
|
||||
|
||||
@@ -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));
|
||||
}
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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<AbsolutePathBuf> {
|
||||
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<AbsolutePathBuf> {
|
||||
keys
|
||||
}
|
||||
|
||||
fn to_abs_path(cwd: &Path, path: &Path) -> Option<AbsolutePathBuf> {
|
||||
fn to_abs_path(cwd: &AbsolutePathBuf, path: &Path) -> Option<AbsolutePathBuf> {
|
||||
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<PermissionProfile> {
|
||||
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::<BTreeSet<_>>()
|
||||
.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<u64>,
|
||||
session: Arc<Session>,
|
||||
turn: Arc<TurnContext>,
|
||||
@@ -262,7 +273,7 @@ pub(crate) async fn intercept_apply_patch(
|
||||
call_id: &str,
|
||||
tool_name: &str,
|
||||
) -> Result<Option<FunctionToolOutput>, 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(
|
||||
|
||||
@@ -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)),
|
||||
|
||||
@@ -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(),
|
||||
|
||||
@@ -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(),
|
||||
|
||||
@@ -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<ApplyPatchRequest> 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 {
|
||||
|
||||
@@ -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(),
|
||||
},
|
||||
|
||||
@@ -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;
|
||||
}
|
||||
|
||||
@@ -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<ParsedUnifiedExecOutput> {
|
||||
let cleaned = raw.replace("\r\n", "\n");
|
||||
let (metadata, output) = cleaned
|
||||
.rsplit_once("\nOutput:")
|
||||
static OUTPUT_REGEX: OnceLock<Regex> = OnceLock::new();
|
||||
let regex = OUTPUT_REGEX.get_or_init(|| {
|
||||
Regex::new(concat!(
|
||||
r#"(?s)^(?:Total output lines: \d+\n\n)?"#,
|
||||
r#"(?:Chunk ID: (?P<chunk_id>[^\n]+)\n)?"#,
|
||||
r#"Wall time: (?P<wall_time>-?\d+(?:\.\d+)?) seconds\n"#,
|
||||
r#"(?:Process exited with code (?P<exit_code>-?\d+)\n)?"#,
|
||||
r#"(?:Process running with session ID (?P<process_id>-?\d+)\n)?"#,
|
||||
r#"(?:Original token count: (?P<original_token_count>\d+)\n)?"#,
|
||||
r#"Output:\n?(?P<output>.*)$"#,
|
||||
))
|
||||
.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::<f64>()
|
||||
.context("failed to parse wall time seconds")?,
|
||||
);
|
||||
} else if let Some(value) = line.strip_prefix("Process exited with code ") {
|
||||
exit_code = Some(
|
||||
value
|
||||
.parse::<i32>()
|
||||
.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::<usize>()
|
||||
.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::<f64>()
|
||||
.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::<i32>()
|
||||
.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::<usize>()
|
||||
.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<ParsedUnifiedExecOutput> {
|
||||
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(())
|
||||
}
|
||||
|
||||
@@ -39,6 +39,12 @@ pub type FileSystemResult<T> = io::Result<T>;
|
||||
pub trait ExecutorFileSystem: Send + Sync {
|
||||
async fn read_file(&self, path: &AbsolutePathBuf) -> FileSystemResult<Vec<u8>>;
|
||||
|
||||
/// Reads a file and decodes it as UTF-8 text.
|
||||
async fn read_file_text(&self, path: &AbsolutePathBuf) -> FileSystemResult<String> {
|
||||
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<u8>) -> FileSystemResult<()>;
|
||||
|
||||
async fn create_directory(
|
||||
|
||||
@@ -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),
|
||||
|
||||
@@ -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),
|
||||
|
||||
@@ -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()
|
||||
}
|
||||
|
||||
@@ -130,6 +130,54 @@ impl From<AbsolutePathBuf> 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;
|
||||
|
||||
|
||||
Reference in New Issue
Block a user