fix(sandbox): forward --deny/--usd/--tokens/--api-base into the container - #168
Conversation
…iner
The --sandbox path returned before any policy was built, and the
container ran with a bare --yes: a user hard-blocked --deny pattern
was auto-approved inside the sandbox, and --usd/--tokens/--api-base
were silently dropped. build_run_command and run_sandboxed now accept
and forward them, and cli.py passes them through.
Tests: two regression tests (flags forwarded when set, omitted when
unset). Note: several existing sandbox tests fail on Windows on clean
main too ('C\keep.txt' path handling in commit-back); pre-existing
and unrelated.
Closes vedaant00#163
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
Add CLI wiring coverage and address incompatible local API endpoints with disabled or isolated networking.
Review effort: Lite
Findings: None
What changed in this PR
Forwards sandbox policy, budget, and API-base options into containerized runs.
Changes:
- Added forwarding for
--deny,--usd,--tokens, and--api-base. - Updated CLI-to-sandbox wiring.
- Added regression tests for set and unset flags.
| File | Description |
|---|---|
tests/test_sandbox.py |
Tests forwarded and omitted flags. |
src/opendot/sandbox.py |
Appends options to container arguments. |
src/opendot/cli.py |
Passes parsed options into sandbox execution. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
CI lint job runs ruff format --check; the new call sites were not formatted (magic trailing comma style). No behaviour change.
|
Fixed the failing Verified locally with the exact CI commands: |
…tainer Address the review note on vedaant00#168. Forwarding --api-base is correct, but the flag names a server the *container* has to reach, and two common cases now fail obscurely instead of loudly: - with the default --network none the container has no network at all; - a loopback URL resolves to the container itself, not the host. Both now print an actionable warning before the run. This matters more than it looks because --api-base defaults to $OPENAI_API_BASE, so anyone with that exported picks it up on every --sandbox run without typing the flag. Isolation semantics are unchanged; this only adds a diagnostic. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Good fix @t957095, the flag forwarding is exactly right and I like that the argv is unchanged when nothing is set. Verified locally: repeated --deny survives, budgets forward, and the new test fails on pre-fix code. I pushed one follow-up for the review note about --api-base. Forwarding it is correct, but it names a server the container has to reach, and two cases still failed obscurely: the default --network none leaves no network at all, and a loopback URL means the container itself rather than your host. Both now warn with a suggested fix. This matters more than it looks because --api-base defaults to $OPENAI_API_BASE, so anyone with that exported picks it up on every sandbox run without typing the flag. |
Problem
The
--sandboxbranch incli.pyreturned before any policy was built, and the container ranopendot -p ... --yeswith no other flags: a--denypattern the user explicitly hard-blocked became auto-approved inside the sandbox, and--usd,--tokens,--api-basewere silently dropped.Fix
build_run_commandandrun_sandboxednow acceptdeny,usd,tokens, andapi_baseand append them to the container argv;cli.pyforwards them fromargs. Isolation semantics unchanged (still--network noneby default, scoped env forwarding).Tests: two regression tests (flags present when set, absent when unset) — both pass.
Note: a few existing sandbox tests fail on Windows on clean main (commit-back path handling, e.g.
C\keep.txt); pre-existing and unrelated.Closes #163