Repository navigation
Conversation
Pass raw model path strings into sandbox path normalization instead of first converting them through the host-native pathlib.Path. This keeps backslash-containing workdirs and image paths deterministic across host operating systems. Add focused regression coverage for shell and view_image path handling.
![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: 361e7276ed
ℹ️ 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".
seratch
left a comment
seratch
left a comment
There was a problem hiding this comment.
Thanks for the focused fix. The view_image change looks correct, but the shell workdir fix does not yet cover UnixLocalSandboxSession.
UnixLocalSandboxSession.normalize_path() uses resolve_symlinks=True, and that branch reconstructs a native Path from the raw string. On POSIX, src\project therefore remains a literal backslash-containing path instead of resolving to src/project. The new shell regression test only uses scripted_sandbox_session, whose normalization path behaves differently.
Please normalize the model-provided workdir as a POSIX sandbox path before Unix-local host resolution, and add a regression test using UnixLocalSandboxSession that proves workdir=r"src\project" resolves beneath src/project. The view_image coverage can remain as-is.
seratch
left a comment
seratch
left a comment
There was a problem hiding this comment.
Can you resolve the conflicts with your other PR?
|
Addressed the requested Unix-local path normalization in the latest commit and added the |
|
@sylvesterkaczmarek the changes are fine now; I am asking to resolve the conflicts |
Head branch was pushed to by a user without write access
|
Conflicts are resolved against current |
Summary
Keep sandbox tool path handling independent of the SDK host operating system.
exec_commandandview_imagecurrently convert model-provided path strings through the host-nativepathlib.Pathbefore sandbox normalization. On POSIX hosts, a path such assrc\\projectremains a literal backslash-containing filename; on Windows the same input is interpreted as path separators. That conflicts with the sandbox path layer, which deliberately normalizes sandbox paths using POSIX semantics regardless of host OS.This change passes the original strings directly into the existing sandbox path policy. The policy then performs the established backslash-to-POSIX normalization consistently for shell workdirs and image paths.
No public API or accepted path scope changes.
Test plan
Added focused regression tests asserting that:
exec_command(workdir="src\\project")resolves to/workspace/src/projectview_image(path="images\\plot.png")reads/workspace/images/plot.pngThe full repository verification is left to CI because this environment cannot materialize the repository dependencies locally.
Issue number
None. Found while auditing sandbox path handling around #4401.
Checks
.agents/skills/code-change-verification/scripts/run.sh/reviewbefore submitting this PR