Conversation
![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: 3f811565a3
ℹ️ 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 is_directory: bool, | ||
| pub is_file: bool, | ||
| pub is_symlink: bool, | ||
| pub link_count: u64, |
There was a problem hiding this comment.
When a newer client talks to an already-running remote exec-server via CODEX_EXEC_SERVER_URL, older servers will still return fs/getMetadata results without linkCount; because this new field is required during deserialization and initialize has no protocol-version/capability negotiation, every remote get_metadata call starts failing before the caller can decide whether the hard-link check actually needs the value. This breaks normal metadata users during rolling upgrades or mixed client/server deployments; make the field backward-compatible/defaulted or version-gated, and fail closed only in the apply-patch hard-link check when the count is unavailable.
Useful? React with 👍 / 👎.
| let Some(sandbox) = sandbox.filter(|sandbox| sandbox.should_run_in_sandbox()) else { | ||
| return Ok(()); | ||
| }; |
There was a problem hiding this comment.
With approval policies that allow no-sandbox retries (on-failure, on-request, or preapproved patch permissions), a hard-link write first returns PermissionDenied with Failed to write file..., which is_likely_sandbox_denied classifies as a sandbox denial; the orchestrator can then rerun the same patch with SandboxType::None, and this early return disables the hard-link guard so the second attempt writes through the hard link. The hard-link refusal needs to be treated as a terminal policy error, or the guard must also run/fail closed on no-sandbox retries for apply_patch.
Useful? React with 👍 / 👎.
bolinfest
changed the title
bolinfest
force-pushed
the
pr21819
branch
3 times, most recently
from
7402d29 to
b6dc57e
Compare
| } | ||
|
|
||
| #[derive(Clone, Copy, Debug, Eq, PartialEq)] | ||
| enum WorkspaceLinkKind { |
There was a problem hiding this comment.
nit: can this be a bool? codex overreacted here a bit it feels
| "{FS_WRITE_FILE_METHOD} requires valid base64 dataBase64: {err}" | ||
| )) | ||
| })?; | ||
| file_system |
There was a problem hiding this comment.
is file_system.write_file still used?
There was a problem hiding this comment.
would be nice to keep the pattern or all logic living in local_file_system eventually and keep this file purely a router.
|
Update: I think this is not an appropriate fix, as written. Sandboxed Codex cannot create hard links outside the sandbox, but if the user created such a hardlink, then I think (1) we should preserve it rather than de-link it by doing a |
bolinfest
changed the title
bolinfest
force-pushed
the
pr21819
branch
2 times, most recently
from
b50e4f8 to
a38c847
Compare
bolinfest
force-pushed
the
pr21819
branch
2 times, most recently
from
16614d5 to
ebddba6
Compare
4 participants
Why
PR #1705 moved
apply_patchexecution under the configured sandbox and called out the need for integration coverage. We already covered textual../escapes, but did not have coverage for link aliases that live inside a writable workspace while pointing at, or aliasing, files visible outside it.This PR locks in the current sandbox boundary without changing production write semantics. Symlink escapes into a read-only outside root should fail and leave the outside file unchanged. Existing hard links are characterized separately: if a user-created hard link already exists inside the writable root, sandboxed writes preserve normal hard-link semantics rather than replacing the link and silently breaking that relationship.
What Changed
apply_patch_cli_does_not_write_through_symlink_escape_outside_workspaceto verifyapply_patchcannot update a symlink that targets a file outside the writable workspace.apply_patch_cli_preserves_existing_hard_link_outside_workspaceto verifyapply_patchintentionally writes through an existing hard link and does not unlink or replace it.file_system_sandboxed_write_preserves_existing_hard_linkto verify sandboxedfs/writeFilepreserves an existing hard link and writes the shared inode.Testing
cargo test -p codex-exec-server file_system_sandboxed_writecargo test -p codex-core apply_patch_cli_does_not_write_through_symlink_escape_outside_workspacecargo test -p codex-core apply_patch_cli_preserves_existing_hard_link_outside_workspacejust fix -p codex-exec-server -p codex-corejust fix -p codex-coreStack created with Sapling. Best reviewed with ReviewStack.