exec-server: canonicalize bound filesystem paths#25149
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: ab25267ba1
ℹ️ 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".
starr-openai
force-pushed
the
starr/skills-path-ref-1b-bound-canonicalize
branch
3 times, most recently
from
865271a to
1170d1c
Compare
starr-openai
force-pushed
the
starr/skills-path-ref-1-env-path-ref
branch
from
f1f663f to
b444724
Compare
starr-openai
force-pushed
the
starr/skills-path-ref-1b-bound-canonicalize
branch
2 times, most recently
from
1119f59 to
641000f
Compare
starr-openai
force-pushed
the
starr/skills-path-ref-1-env-path-ref
branch
from
b444724 to
55ec07d
Compare
starr-openai
force-pushed
the
starr/skills-path-ref-1b-bound-canonicalize
branch
from
641000f to
8c24577
Compare
jif-oai
left a comment
jif-oai
left a comment
There was a problem hiding this comment.
Good for me after having double checked my comments
| base_path: &AbsolutePathBuf, | ||
| path: &Path, | ||
| ) -> FileSystemResult<AbsolutePathBuf> { | ||
| Ok(base_path.join(path)) |
There was a problem hiding this comment.
I might be crazy... but what if
- A user gets an
EnvironmentPathReffor one allowed skill/root path. - Some later code calls something like
join("../outside"),join("/absolute/path"), orjoin("~/.codex") AbsolutePathBuf::joinnormalizes that into a valid absolute path outside the original ref.- Now the caller has a same-filesystem
EnvironmentPathRefto a path it was
never handed
I we want to preserve path authority, should we do everything in relative for this function?
I might be completely wrong here so I invite you to double check
There was a problem hiding this comment.
I moved the EnvironmentPathRef struct here to make it general purpose / agnostic of skills, and instead tried to keep the API to mirror AbsolutePathBuf as closely as possible to avoid confusion and simplify migrating existing code paths.
IMO forcing join() to be nested relative to base is a pretty opinionated choice. I'd rather have a helper for that which does the join first, then just checks for the output requirement afterward.
| .map(|path| path.map(|path| self.with_path(path))) | ||
| } | ||
|
|
||
| pub async fn canonicalize( |
There was a problem hiding this comment.
The new canonicalize method accepts a sandbox, but local() binds LOCAL_FS, which has no runtime paths, so won't the callers hit the helper-path error instead of sandboxed canonicalization?
There was a problem hiding this comment.
mmm good catch. I think in practice this is less of a concern, since my intention is to always pass in an explicit Environment (even for local) rather than use this constructor, which is really meant for unsandboxed/internal use-cases.
| EnvironmentPathRef::new(Arc::clone(&LOCAL_FS), path) | ||
| } | ||
|
|
||
| #[async_trait] |
There was a problem hiding this comment.
Side note but if you want to learn cool stuff about Rust, ask ChatGPT more about async_trait, how does it work, why this is useful and why this is necessary
This is a very interesting example
starr-openai
force-pushed
the
starr/skills-path-ref-1b-bound-canonicalize
branch
from
8c24577 to
111adfb
Compare
2 participants
Summary
Stack
Validation
cd /Users/starr/code/codex-worktrees/pr-25098-restack-review-pr1b/codex-rs && just fmt