Preserve executor paths in read command actions (#36223)

## Why

Read command actions were omitted when a selected environment used a path
convention foreign to the app-server host. Clients need these actions to refer
to files in the executor's filesystem, not paths interpreted on the host.

## What changed

- Resolve read paths against the executor's `PathUri` and serialize them using
  the executor's native POSIX, Windows drive, or UNC convention.
- Accept same-drive Windows-relative paths while continuing to reject paths
  that require another drive's executor-local current directory.
- Document the path convention used by `commandExecution` notifications.

## Testing

Cover native and cross-platform path serialization, Windows drive-relative
joins, and selected-environment `item/started` and `item/completed`
notifications.

GitOrigin-RevId: 3c5a9bb1caafd35e04a0a0cceab91fee590372b9
This commit is contained in:
iceweasel-oai
2026-07-30 21:12:41 +00:00
committed by copyberry
parent 745603a5a1
commit a01a2d9146
29 changed files with 259 additions and 92 deletions
@@ -2449,7 +2449,7 @@ async fn item_lifecycle_notifications_publish_command_execution_event() {
CommandAction::Read {
command: "cat README.md".to_string(),
name: "README.md".to_string(),
path: test_path_buf("/tmp/README.md").abs(),
path: test_path_buf("/tmp/README.md").abs().into(),
},
CommandAction::ListFiles {
command: "ls".to_string(),
@@ -1,10 +1,6 @@
{
"$schema": "http://json-schema.org/draft-07/schema#",
"definitions": {
"AbsolutePathBuf": {
"description": "A path that is guaranteed to be absolute and normalized (though it is not guaranteed to be canonicalized or exist on the filesystem).\n\nIMPORTANT: When deserializing an `AbsolutePathBuf`, a base path must be set using [AbsolutePathBufGuard::new]. If no base path is set, the deserialization will fail unless the path being deserialized is already absolute.",
"type": "string"
},
"AdditionalFileSystemPermissions": {
"properties": {
"entries": {
@@ -95,7 +91,7 @@
"type": "string"
},
"path": {
"$ref": "#/definitions/AbsolutePathBuf"
"$ref": "#/definitions/LegacyAppPathString"
},
"type": {
"enum": [
@@ -798,7 +798,7 @@
"type": "string"
},
"path": {
"$ref": "#/definitions/AbsolutePathBuf"
"$ref": "#/definitions/LegacyAppPathString"
},
"type": {
"enum": [
+1 -1
View File
@@ -164,7 +164,7 @@
"type": "string"
},
"path": {
"$ref": "#/definitions/AbsolutePathBuf"
"$ref": "#/definitions/LegacyAppPathString"
},
"type": {
"enum": [
@@ -7649,7 +7649,7 @@
"type": "string"
},
"path": {
"$ref": "#/definitions/v2/AbsolutePathBuf"
"$ref": "#/definitions/v2/LegacyAppPathString"
},
"type": {
"enum": [
@@ -3827,7 +3827,7 @@
"type": "string"
},
"path": {
"$ref": "#/definitions/AbsolutePathBuf"
"$ref": "#/definitions/LegacyAppPathString"
},
"type": {
"enum": [
@@ -82,7 +82,7 @@
"type": "string"
},
"path": {
"$ref": "#/definitions/AbsolutePathBuf"
"$ref": "#/definitions/LegacyAppPathString"
},
"type": {
"enum": [
@@ -82,7 +82,7 @@
"type": "string"
},
"path": {
"$ref": "#/definitions/AbsolutePathBuf"
"$ref": "#/definitions/LegacyAppPathString"
},
"type": {
"enum": [
@@ -220,7 +220,7 @@
"type": "string"
},
"path": {
"$ref": "#/definitions/AbsolutePathBuf"
"$ref": "#/definitions/LegacyAppPathString"
},
"type": {
"enum": [
@@ -301,7 +301,7 @@
"type": "string"
},
"path": {
"$ref": "#/definitions/AbsolutePathBuf"
"$ref": "#/definitions/LegacyAppPathString"
},
"type": {
"enum": [
@@ -223,7 +223,7 @@
"type": "string"
},
"path": {
"$ref": "#/definitions/AbsolutePathBuf"
"$ref": "#/definitions/LegacyAppPathString"
},
"type": {
"enum": [
@@ -223,7 +223,7 @@
"type": "string"
},
"path": {
"$ref": "#/definitions/AbsolutePathBuf"
"$ref": "#/definitions/LegacyAppPathString"
},
"type": {
"enum": [
@@ -223,7 +223,7 @@
"type": "string"
},
"path": {
"$ref": "#/definitions/AbsolutePathBuf"
"$ref": "#/definitions/LegacyAppPathString"
},
"type": {
"enum": [
@@ -301,7 +301,7 @@
"type": "string"
},
"path": {
"$ref": "#/definitions/AbsolutePathBuf"
"$ref": "#/definitions/LegacyAppPathString"
},
"type": {
"enum": [
@@ -223,7 +223,7 @@
"type": "string"
},
"path": {
"$ref": "#/definitions/AbsolutePathBuf"
"$ref": "#/definitions/LegacyAppPathString"
},
"type": {
"enum": [
@@ -301,7 +301,7 @@
"type": "string"
},
"path": {
"$ref": "#/definitions/AbsolutePathBuf"
"$ref": "#/definitions/LegacyAppPathString"
},
"type": {
"enum": [
@@ -223,7 +223,7 @@
"type": "string"
},
"path": {
"$ref": "#/definitions/AbsolutePathBuf"
"$ref": "#/definitions/LegacyAppPathString"
},
"type": {
"enum": [
@@ -223,7 +223,7 @@
"type": "string"
},
"path": {
"$ref": "#/definitions/AbsolutePathBuf"
"$ref": "#/definitions/LegacyAppPathString"
},
"type": {
"enum": [
@@ -220,7 +220,7 @@
"type": "string"
},
"path": {
"$ref": "#/definitions/AbsolutePathBuf"
"$ref": "#/definitions/LegacyAppPathString"
},
"type": {
"enum": [
@@ -220,7 +220,7 @@
"type": "string"
},
"path": {
"$ref": "#/definitions/AbsolutePathBuf"
"$ref": "#/definitions/LegacyAppPathString"
},
"type": {
"enum": [
@@ -220,7 +220,7 @@
"type": "string"
},
"path": {
"$ref": "#/definitions/AbsolutePathBuf"
"$ref": "#/definitions/LegacyAppPathString"
},
"type": {
"enum": [
@@ -1,6 +1,6 @@
// GENERATED CODE! DO NOT MODIFY BY HAND!
// This file was generated by [ts-rs](https://github.com/Aleph-Alpha/ts-rs). Do not edit this file manually.
import type { AbsolutePathBuf } from "../AbsolutePathBuf";
import type { LegacyAppPathString } from "../LegacyAppPathString";
export type CommandAction = { "type": "read", command: string, name: string, path: AbsolutePathBuf, } | { "type": "listFiles", command: string, path: string | null, } | { "type": "search", command: string, query: string | null, path: string | null, } | { "type": "unknown", command: string, };
export type CommandAction = { "type": "read", command: string, name: string, path: LegacyAppPathString, } | { "type": "listFiles", command: string, path: string | null, } | { "type": "search", command: string, query: string | null, path: string | null, } | { "type": "unknown", command: string, };
@@ -37,7 +37,6 @@ use codex_protocol::review_format::REVIEW_FALLBACK_MESSAGE;
use codex_protocol::review_format::render_review_output_text;
use codex_shell_command::parse_command::parse_command;
use codex_shell_command::parse_command::shlex_join;
use codex_utils_path_uri::PathConvention;
use codex_utils_path_uri::PathUri;
use std::collections::HashMap;
use std::path::PathBuf;
@@ -122,33 +121,32 @@ pub(crate) fn command_actions_for_path_uri(
parsed_cmd: &[ParsedCommand],
cwd: &PathUri,
) -> Vec<CommandAction> {
// TODO(anp): Carry PathUri into CommandAction so foreign Read actions retain resolved paths.
// Until then, omit those actions rather than project a foreign cwd onto the host.
let native_cwd = if cwd.infer_path_convention() == Some(PathConvention::native()) {
cwd.to_abs_path().ok()
} else {
None
};
parsed_cmd
.iter()
.cloned()
.filter_map(|parsed| match parsed {
ParsedCommand::Read { cmd, name, path } => match native_cwd.as_ref() {
Some(native_cwd) => Some(CommandAction::Read {
command: cmd,
name,
path: native_cwd.join(path),
}),
None => {
warn!(
command = cmd,
%cwd,
"omitting read command action whose path cannot be resolved against a foreign cwd"
);
None
ParsedCommand::Read { cmd, name, path } => {
// Resolve against the executor's URI, not the app-server's filesystem. POSIX
// non-UTF-8 paths are percent-encoded, not opaque. Expanding `~` or resolving a
// genuinely opaque cwd would require executor-native state unavailable here.
match cwd.join(path.to_string_lossy().as_ref()) {
Ok(path) => Some(CommandAction::Read {
command: cmd,
name,
path: path.into(),
}),
Err(error) => {
warn!(
command = cmd,
%cwd,
file_path = %path.display(),
%error,
"omitting read action: invalid file path or cwd"
);
None
}
}
},
}
ParsedCommand::ListFiles { cmd, path } => {
Some(CommandAction::ListFiles { command: cmd, path })
}
@@ -1,41 +1,73 @@
use super::*;
use pretty_assertions::assert_eq;
use serde_json::json;
#[test]
fn foreign_read_is_omitted_without_dropping_other_command_actions() {
#[cfg(windows)]
let cwd = PathUri::parse("file:///usr/local/src").expect("valid foreign POSIX cwd");
#[cfg(not(windows))]
let cwd = PathUri::parse("file:///C:/src").expect("valid foreign Windows cwd");
let parsed_cmd = vec![
ParsedCommand::Read {
cmd: "cat file.txt".to_string(),
name: "file.txt".to_string(),
path: PathBuf::from("file.txt"),
},
ParsedCommand::ListFiles {
cmd: "ls".to_string(),
path: Some("subdir".to_string()),
},
ParsedCommand::Search {
cmd: "rg needle".to_string(),
query: Some("needle".to_string()),
path: Some("src".to_string()),
},
];
assert_eq!(
command_actions_for_path_uri(&parsed_cmd, &cwd),
vec![
CommandAction::ListFiles {
command: "ls".to_string(),
fn read_command_actions_preserve_native_and_foreign_paths() {
for (cwd_uri, relative_path, expected_path) in [
(
"file:///home/alice/repo",
"src/main.rs",
"/home/alice/repo/src/main.rs",
),
(
"file:///C:/Users/Alice%20Smith/repo",
r"src\main.rs",
r"C:\Users\Alice Smith\repo\src\main.rs",
),
(
"file:///C:/Users/Alice%20Smith/repo",
r"C:src\main.rs",
r"C:\Users\Alice Smith\repo\src\main.rs",
),
(
"file://server/share/repo",
r"src\main.rs",
r"\\server\share\repo\src\main.rs",
),
] {
let cwd = PathUri::parse(cwd_uri).expect("valid cross-platform cwd");
let command = format!("cat {relative_path}");
let parsed_cmd = vec![
ParsedCommand::Read {
cmd: command.clone(),
name: "main.rs".to_string(),
path: PathBuf::from(relative_path),
},
ParsedCommand::ListFiles {
cmd: "ls".to_string(),
path: Some("subdir".to_string()),
},
CommandAction::Search {
command: "rg needle".to_string(),
ParsedCommand::Search {
cmd: "rg needle".to_string(),
query: Some("needle".to_string()),
path: Some("src".to_string()),
},
]
);
];
assert_eq!(
serde_json::to_value(command_actions_for_path_uri(&parsed_cmd, &cwd))
.expect("command actions should serialize"),
json!([
{
"type": "read",
"command": command,
"name": "main.rs",
"path": expected_path,
},
{
"type": "listFiles",
"command": "ls",
"path": "subdir",
},
{
"type": "search",
"command": "rg needle",
"query": "needle",
"path": "src",
},
]),
"resolving command actions against {cwd_uri}",
);
}
}
@@ -122,7 +122,7 @@ pub enum CommandAction {
Read {
command: String,
name: String,
path: AbsolutePathBuf,
path: LegacyAppPathString,
},
ListFiles {
command: String,
@@ -186,7 +186,7 @@ impl CommandAction {
} => CoreParsedCommand::Read {
cmd,
name,
path: path.into_path_buf(),
path: PathBuf::from(path.into_string()),
},
CommandAction::ListFiles { command: cmd, path } => {
CoreParsedCommand::ListFiles { cmd, path }
@@ -205,7 +205,7 @@ impl CommandAction {
CoreParsedCommand::Read { cmd, name, path } => CommandAction::Read {
command: cmd,
name,
path: cwd.join(path),
path: cwd.join(path).into(),
},
CoreParsedCommand::ListFiles { cmd, path } => {
CommandAction::ListFiles { command: cmd, path }
+1
View File
@@ -1502,6 +1502,7 @@ The app-server streams JSON-RPC notifications while a turn is running. Each turn
- `plan` — `{id, text}` emitted for plan-mode turns; plan text can stream via `item/plan/delta` (experimental).
- `reasoning` — `{id, summary, content}` where `summary` holds streamed reasoning summaries (applicable for most OpenAI models) and `content` holds raw reasoning blocks (applicable for e.g. open source models).
- `commandExecution` — `{id, pluginId?, scriptPath?, command, cwd, status, commandActions, aggregatedOutput?, exitCode?, durationMs?}` for sandboxed commands; `pluginId` is present only for commands attributed to a trusted first-party plugin, newly attributed items also include `scriptPath` as a safe `/`-separated path relative to the trusted plugin root, older history may omit `scriptPath`, and `status` is `inProgress`, `completed`, `failed`, or `declined`.
`cwd` and read `commandActions[].path` use the executor's native path convention, even when the app-server runs on a different operating system. For example, an app-server running on Linux can return `C:\repo\src\main.rs` for a Windows executor; clients must not interpret that path as local to the app-server.
- `fileChange` — `{id, changes, status}` describing proposed edits; `changes` list `{path, kind, diff}` and `status` is `inProgress`, `completed`, `failed`, or `declined`.
- `mcpToolCall` — `{id, server, tool, status, arguments, appContext, mcpAppResourceUri?, pluginId, readOnlyHint, result?, error?}` describing MCP calls; `appContext` is `{connectorId, linkId, resourceUri, appName, actionName}` for calls through a trusted MCP app, where `connectorId` identifies the connector that owns the tool, `linkId` identifies the app link, `resourceUri` points to the widget template, `appName` is the connector's display name, and `actionName` is the stable connector `Action.name`. `readOnlyHint` is `true` for read-only tools, `false` for write-capable tools, and `null` when the annotation is unavailable, including older rollout entries. The hint describes tool capability, not whether an invocation succeeded or performed a write; use `status`, `result`, and `error` to determine the execution outcome. `appName` and `actionName` may be null for older rollout entries. The top-level `mcpAppResourceUri` is deprecated and temporarily duplicated for client migration. `tool` identifies the raw MCP tool. `status` is `inProgress`, `completed`, or `failed`.
- `collabToolCall` — `{id, tool, status, senderThreadId, receiverThreadId?, newThreadId?, prompt?, agentStatus?}` describing collab tool calls (`spawn_agent`, `send_input`, `resume_agent`, `wait`, `close_agent`); `status` is `inProgress`, `completed`, or `failed`.
@@ -6,12 +6,18 @@ use anyhow::Result;
use app_test_support::MockResponsesConfig;
use app_test_support::PathBufExt;
use app_test_support::TestAppServer;
use app_test_support::create_final_assistant_message_sse_response;
use app_test_support::create_mock_responses_server_sequence;
use codex_app_server_protocol::ThreadStartParams;
use codex_app_server_protocol::ThreadStartResponse;
use codex_app_server_protocol::TurnStartParams;
use codex_app_server_protocol::UserInput as V2UserInput;
use codex_features::Feature;
use codex_shell_command::shell_detect::ShellType;
use codex_shell_command::shell_detect::detect_shell_type;
use core_test_support::responses;
use pretty_assertions::assert_eq;
use serde_json::json;
use tempfile::TempDir;
use tokio::time::timeout;
@@ -206,3 +212,106 @@ async fn turn_model_context_uses_selected_environment() -> Result<()> {
);
Ok(())
}
#[tokio::test]
async fn command_execution_notifications_preserve_selected_environment_paths() -> Result<()> {
let command_arguments = serde_json::to_string(&json!({
"cmd": "cat main.rs",
"yield_time_ms": 10_000,
}))?;
let server = create_mock_responses_server_sequence(vec![
responses::sse(vec![
responses::ev_response_created("resp-1"),
responses::ev_function_call(
"selected-environment-read",
"exec_command",
&command_arguments,
),
responses::ev_completed("resp-1"),
]),
create_final_assistant_message_sse_response("done")?,
])
.await;
let codex_home = TempDir::new()?;
MockResponsesConfig::new(&server.uri())
.with_root_config("compact_prompt = \"compact\"\nmodel_auto_compact_token_limit = 100000")
.with_provider_config("supports_websockets = false")
.with_sandbox_mode("danger-full-access")
.enable_feature(Feature::UnifiedExec)
.write(codex_home.path())?;
let mut app_server = TestAppServer::builder()
.with_codex_home(codex_home.path())
.build_initialized()
.await?;
let (expected_path, shell) = {
let environment = app_server.auto_env()?;
let path = environment.selection().cwd.join("main.rs")?;
environment
.environment()
.get_filesystem()
.write_file(&path, b"fn main() {}\n".to_vec(), /*sandbox*/ None)
.await?;
(
path.inferred_native_path_string(),
environment.environment().info().await?.shell,
)
};
let thread = app_server
.start_thread(ThreadStartParams::default())
.await?
.thread;
timeout(
DEFAULT_READ_TIMEOUT,
app_server.start_turn_and_wait_for_completion(text_turn_params(thread.id, "read main.rs")),
)
.await??;
let expected_actions = match shell.name.as_str() {
// Windows shell scripts are not yet parsed into file-read command actions.
"powershell" => {
let command = if detect_shell_type(&shell.path) == Some(ShellType::PowerShell) {
"cat main.rs".to_string()
} else {
shlex::try_join([shell.path.as_str(), "-Command", "cat main.rs"])?
};
json!([{
"type": "unknown",
"command": command,
}])
}
"cmd" => {
let command = shlex::try_join([shell.path.as_str(), "/c", "cat main.rs"])?;
json!([{
"type": "unknown",
"command": command,
}])
}
_ => json!([{
"type": "read",
"command": "cat main.rs",
"name": "main.rs",
"path": expected_path,
}]),
};
for method in ["item/started", "item/completed"] {
let notification = timeout(
DEFAULT_READ_TIMEOUT,
app_server.read_stream_until_matching_notification(method, |notification| {
notification.method == method
&& notification
.params
.as_ref()
.is_some_and(|params| params["item"]["id"] == "selected-environment-read")
}),
)
.await??;
let params = notification
.params
.context("command execution notification should include params")?;
assert_eq!(params["item"]["commandActions"], expected_actions);
}
Ok(())
}
+23 -7
View File
@@ -325,14 +325,17 @@ impl PathUri {
/// Path text is interpreted using the POSIX or Windows convention inferred
/// from the base URI. An absolute path replaces the base URI's path, while a
/// relative path is appended lexically. Windows root-relative paths retain
/// the base drive or UNC share, while drive-relative paths are rejected.
/// the base drive or UNC share. Same-drive relative paths are appended to
/// the base, while other-drive relative paths are rejected because their
/// current directory belongs to the executor.
/// Empty and `.` segments are ignored, while `..` removes one segment
/// without escaping the POSIX root, Windows drive, or UNC share. Literal
/// `%`, `?`, and `#` characters are percent-encoded as filename text. Paths
/// containing a null character are rejected because they cannot be safely
/// converted to native paths.
/// Opaque fallback URIs created by [`Self::from_abs_path`] reject non-empty
/// joins.
/// joins. Home-directory expansion also requires executor-native context
/// and is intentionally not performed.
pub fn join(&self, path: &str) -> Result<Self, PathUriParseError> {
if path.contains('\0') {
return Err(PathUriParseError::InvalidFileUriPath {
@@ -353,13 +356,26 @@ impl PathUri {
return Ok(absolute);
}
let path_bytes = path.as_bytes();
if convention == PathConvention::Windows
let path = if convention == PathConvention::Windows
&& matches!(path_bytes, [drive, b':', ..] if drive.is_ascii_alphabetic())
{
return Err(PathUriParseError::InvalidFileUriPath {
path: path.to_string(),
});
}
let same_drive = self
.0
.path_segments()
.and_then(|mut segments| segments.find(|segment| !segment.is_empty()))
.is_some_and(|segment| {
matches!(segment.as_bytes(), [drive, b':'] if drive.eq_ignore_ascii_case(&path_bytes[0]))
});
if !same_drive {
return Err(PathUriParseError::InvalidFileUriPath {
path: path.to_string(),
});
}
&path[2..]
} else {
path
};
let path_bytes = path.as_bytes();
if decode_bad_path_uri(&self.0).is_some() {
return Err(PathUriParseError::InvalidFileUriPath {
path: self.to_string(),
+16 -1
View File
@@ -915,7 +915,22 @@ fn join_windows_root_relative_path_preserves_drive_or_share() {
}
#[test]
fn join_rejects_windows_drive_relative_path() {
fn join_resolves_windows_same_drive_relative_path() {
for (base, path, expected) in [
("file:///C:/base", r"C:tmp", "file:///C:/base/tmp"),
("file:///C:/base", r"c:tmp", "file:///C:/base/tmp"),
("file:///C:/base/dir", r"C:..\tmp", "file:///C:/base/tmp"),
("file:///C:/base", "C:", "file:///C:/base"),
] {
let base = PathUri::parse(base).expect("valid base URI");
let expected = PathUri::parse(expected).expect("valid expected URI");
assert_eq!(base.join(path), Ok(expected), "joining {path}");
}
}
#[test]
fn join_rejects_windows_other_drive_relative_path() {
let base = PathUri::parse("file:///C:/base").expect("valid base URI");
assert_eq!(