feat(allowedpaths): skip nonexistent paths instead of failing#164
feat(allowedpaths): skip nonexistent paths instead of failing#164
Conversation
|
@codex conduct a comprehensive security and code review |
![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: c152bfe631
ℹ️ 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".
|
@codex conduct a comprehensive security and code review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6b23b97053
ℹ️ 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".
|
@codex conduct a comprehensive security and code review. do not request changes that are beyond the scope of this PR and are already existing issues. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6b23b97053
ℹ️ 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".
|
@codex conduct a comprehensive security and code review. do not request changes that are beyond the scope of this PR and are already existing issues. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d8a62a429a
ℹ️ 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".
AllowedPaths is an allowlist — paths that don't exist at construction time are silently skipped. The sandbox operates with whatever paths are available. Tests: - TestNewSkipsNonexistentPaths: existing dir + nonexistent path - TestNewAllPathsNonexistent: all paths missing, empty sandbox - TestNewEmptyPaths: empty list, blocks all access - TestNewMixedExistingAndNonexistent: 3 paths, middle one missing - YAML scenario: nonexistent path skipped, existing dir works Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Success-path scenario (cat on existing file) produces identical output in both rshell and bash — no skip needed. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
AllowedPaths are suggestions — if we can't open a path for any reason (missing, not a directory, no permission, etc.), skip it and work with whatever paths are available. Also use |+ block scalars in scenario YAML per AGENTS.md convention. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Write a warning to os.Stderr when a path cannot be opened, so operators can see which paths were dropped. Temporary approach until a proper logging mechanism is added. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Needed for the stderr warning when skipping unavailable paths in New(). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
New() now accepts an io.Writer for warnings instead of writing to os.Stderr directly. The AllowedPaths RunnerOption passes r.stderr when available, falling back to os.Stderr if not yet configured. This respects the runner's I/O isolation — warnings go to the configured stderr stream, not the process-global one. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
matt-dz
force-pushed
the
matt-dz/no-fail-on-bad-path
branch
from
4cb3155 to
ad6a970
Compare
|
@codex conduct a comprehensive security and code review. do not request changes that are beyond the scope of this PR and are already existing issues. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ad6a97070d
ℹ️ 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".
…logic
The test harness filters out non-existent absolute paths via os.Stat
before passing them to interp.AllowedPaths. Switch to a relative path
("nonexistent_dir") which the harness resolves to a temp-dir child
that genuinely doesn't exist, ensuring the skip logic in
allowedpaths.New is actually exercised. Also expect the stderr warning
and mark skip_assert_against_bash since this is rshell-specific.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@codex conduct a comprehensive security and code review. do not request changes that are beyond the scope of this PR and are already existing issues. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 85e090213a
ℹ️ 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".
allowedpaths.New no longer accepts an io.Writer for warnings. Instead it collects diagnostics into a byte buffer and returns them alongside the sandbox. The AllowedPaths RunnerOption stores the warnings on the runner, and interp.New flushes them to r.stderr after all options have been applied and defaults set. This makes warning output independent of option ordering — callers can pass AllowedPaths before or after StdIO and warnings always reach the configured stderr. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@codex conduct a comprehensive security and code review. do not request changes that are beyond the scope of this PR and are already existing issues. |
|
Codex Review: Didn't find any major issues. Another round soon, please! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
3 participants
Summary
AllowedPaths is now treated as an allowlist — paths that don't exist at construction time are silently skipped instead of causing
New()to fail. The sandbox operates with whatever paths are available.This supports use cases where paths may not exist yet (e.g., config files created after sandbox construction by the agent).
Changes
New()usescontinueinstead ofreturn errorwhenos.OpenRootfailsappend-based accumulationcloseAllcleanup (no mid-construction failures)Tests
TestNewSkipsNonexistentPaths— existing dir + nonexistent path, existing dir worksTestNewAllPathsNonexistent— all paths missing, sandbox succeeds but blocks all accessTestNewEmptyPaths— empty list, blocks all accessTestNewMixedExistingAndNonexistent— 3 paths (existing, missing, existing), both existing workTest plan
go test ./allowedpaths/...passesgo test ./interp/...passesgo test ./tests/ -run TestShellScenariospasses🤖 Generated with Claude Code