Repository navigation
fix: preserve dash-prefixed sandbox copy paths - #31
Kayvan-Zahiri wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Hi, just noticed this draft PR. Let me know if this is something you are hitting and when it is ready for review.
There was a problem hiding this comment.
Not in production. I found it while reading the copy code: a source path starting with - gets parsed by cp as an option. It's ready for review now, thanks for taking a look.
There was a problem hiding this comment.
To confirm, you are not a Baseten user hitting this issue at this time? Or is this just from a scan and you aren't using this code yourself?
This also exists in other SDKs and in Blaxel at https://github.com/blaxel-ai/sdk-python/blob/e9b63fd6bf8eabd02c451510a733e29615644fbf/src/blaxel/core/sandbox/sync/filesystem.py#L175 and in other places. I want to confirm impact.
There was a problem hiding this comment.
Right, I'm not a Baseten user and haven't hit this myself. I found it while reading through sandbox SDK code, so it comes from a review, not a bug report. The impact is small: it only affects a relative path that starts with -, since absolute paths always start with /. If that isn't worth a change on your side, I'm fine with closing it.
There was a problem hiding this comment.
This is less of a concern at this time. I do think we'll close it and when we tackle, we can do it across all 3 SDKs.
There was a problem hiding this comment.
🟢 Approval recommended
The focused fix preserves existing behavior and has regression coverage for both APIs, with no unresolved findings.
No open issues 🎉
What changed in this PR
Fixes Sandbox.fs.copy so dash-prefixed paths are treated as operands rather than command options in both sync and async APIs.
Changes:
- Adds
--before copy operands while preserving shell quoting. - Adds six regression cases and updates the existing command assertion.
| File | Description |
|---|---|
| tests/sandbox/test_fs.py | Updates the expected copy command. |
| tests/sandbox/test_copy_paths.py | Tests dash-prefixed paths through both APIs. |
| baseten/sandbox/_fs.py | Ends option parsing before copy operands. |
Review effort: Balanced
Relative source paths beginning with
-cannot reliably be copied withSandbox.fs.copy: shell quoting preserves the string, butcpstill interprets it as an option. For example, copying-sourcetocopyfails instead of copying an existing file.Add
--before the two quoted operands in the command shared by sync and async copy. Existing quoting and process completion handling remain intact.Validation: six public sync/async API regression cases execute the emitted command against temporary local files (four fail before the change, six pass after). Full suite: 449 passed, 86 skipped. Repository lint and type checks, wheel/sdist builds and
git diff --checkpass. Live Baseten E2E tests were skipped; this reproduces command construction locally through HTTPX mock transport. Developed with AI assistance and verified locally.