app-server: preserve remote environment cwd#28146
Conversation
## Why This is the second-to-last place in the exec-server protocol that needs to migrate to URIs to support cross-OS operation. ## What - Change `ExecParams.cwd` to `PathUri`. - Keep the cwd URI-shaped through core and rmcp producers, converting it to `AbsolutePathBuf` only in `LocalProcess::start_process`. - Reject non-native cwd URIs before launch and update the affected protocol documentation and call sites.
anp-oai
force-pushed
the
codex/app-server-remote-environment-cwd
branch
from
2ea8378 to
0899d52
Compare
| ); | ||
| } | ||
|
|
||
| #[test] |
There was a problem hiding this comment.
for human reviewers: this layer can't enforce that absolute paths are required anymore, this is moved to a thread start test
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 167b3b9a5a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| pub argv: Vec<String>, | ||
| pub cwd: PathBuf, | ||
| /// Working directory URI, interpreted using the exec-server host's path rules at launch time. | ||
| pub cwd: PathUri, |
There was a problem hiding this comment.
PathUri changes process/start.cwd from a plain path to file:///...; during mixed-version remote exec-server rollouts, older servers still parse it as PathBuf and launch in a literal/invalid file: path. Accept both encodings or version-gate the client. guidance
Useful? React with 👍 / 👎.
| let cwd_uri = PathUri::from_abs_path(&cwd); | ||
| Self { | ||
| ) -> std::io::Result<Self> { | ||
| let cwd = cwd_uri.to_abs_path()?; |
There was a problem hiding this comment.
to_abs_path() parses cwd_uri using the current host's path rules, so a Linux/macOS app-server rejects a Windows UNC URI before it can reach a remote Windows exec-server. That makes the remote exec path non-portable; keep the URI until the target-host launch boundary. guidance
Useful? React with 👍 / 👎.
| codex-protocol = { workspace = true } | ||
| codex-shell-command = { workspace = true } | ||
| codex-utils-absolute-path = { workspace = true } | ||
| codex-utils-path-uri = { workspace = true } |
There was a problem hiding this comment.
Adding this workspace dependency without updating MODULE.bazel.lock leaves Bazel lock state behind Cargo, so Bazel builds or lock checks can fail even though Cargo resolves the new crate. Please run just bazel-lock-update and commit the lockfile update. guidance
Useful? React with 👍 / 👎.
1 participant
Why
App-server environment selections can target an OS different from the app-server host. Applying host-native path rules at the API boundary rejects or rewrites a Windows cwd before it reaches a Windows exec-server, so a remote turn cannot reliably preserve its selected working directory and shell.
This is stacked on #27819, which provides host-independent path rendering. It is okay for the earlier exec-server cwd fix to remain visible in this diff.
What
PathUriuntil the execution or sandbox boundary.thread/startandturn/start, infer their absolute path convention, and convert them toPathUribefore core selection validation.C:\windowsis the execution cwd.AGENTS.mdfixture and pin the thread-start metadata fields that still use host fallback values, with separate follow-up TODOs.Testing
bazel test //codex-rs/core/tests/remote_env_windows:smoke-test --test_output=errorsjust test -p codex-app-serverjust test -p codex-app-server-protocolcodex-coreenvironment-selection tests