Skip to content

fix(#1649): add UploadFile for file-to-file sandbox upload semantics - #1651

Closed
fullsend-ai-coder[bot] wants to merge 1 commit into
mainfrom
agent/1649-fix-upload-file-staging
Closed

fix(#1649): add UploadFile for file-to-file sandbox upload semantics#1651
fullsend-ai-coder[bot] wants to merge 1 commit into
mainfrom
agent/1649-fix-upload-file-staging

Conversation

@fullsend-ai-coder

Copy link
Copy Markdown
Contributor

openshell sandbox upload treats the remote path as a directory, placing the uploaded file inside it with the source basename. This caused host_files staging to create a directory at the destination path (e.g. prior-review.txt/) with the file nested inside, producing EISDIR errors when the agent tried to read the file.

Add sandbox.UploadFile() — analogous to the existing DownloadFile() — which uploads to the parent directory and renames in-sandbox if the basename differs. Update all three sandbox.Upload() calls in bootstrapEnv's host_files staging and .env upload to use UploadFile instead.

Note: Go tests could not run in sandbox (go.mod requires go >= 1.26.0, sandbox has 1.24.13 with read-only GOPATH). Pre-commit could not run (same Go toolchain issue). Manual verification of go-test is required.


Closes #1649

Post-script verification

  • Branch is not main/master (agent/1649-fix-upload-file-staging)
  • Secret scan passed (gitleaks — 557c0a1189eed1adca4ec44cbdb53bf81f423703..HEAD)
  • Pre-commit hooks passed (authoritative run on runner)
  • Tests ran inside sandbox

@github-actions

github-actions Bot commented May 28, 2026

Copy link
Copy Markdown

Site preview

Preview: https://4ecbbe03-site.fullsend-ai.workers.dev

Commit: ea8b30e1b25092b24ebc05ea20341caa60e61c39

@fullsend-ai-review

fullsend-ai-review Bot commented May 28, 2026

Copy link
Copy Markdown

Review

Findings

Low

  • [shell-quoting] internal/sandbox/sandbox.go:395UploadFile constructs the in-sandbox mv command via fmt.Sprintf("mv %s %s", uploadedPath, remotePath) without shell-quoting either path. If a path ever contains spaces or shell metacharacters, the command will fail or behave unexpectedly. Current callers pass os.CreateTemp-generated names and harness-config destinations, which are safe. This matches the pre-existing pattern in UploadDir (line 402) and is not a regression, but worth quoting defensively in a future cleanup.
    Remediation: Use fmt.Sprintf("mv '%s' '%s'", ...) or fmt.Sprintf("mv %q %q", ...) with proper shell escaping, consistent with a broader cleanup of the same pattern in UploadDir.

Info

  • [docs-gap] docs/guides/dev/cli-internals.md:348 — The "Key Sandbox Operations" table lists Upload(), Download(), and SafeDownload() but omits the helper variants UploadFile, DownloadFile, and UploadDir. This is a pre-existing gap (not introduced by this PR) but the new function widens it slightly.
    Remediation: Add UploadFile(), DownloadFile(), and UploadDir() rows to the table in a follow-up.
Previous run

Review

Findings

Low

  • [correctness] internal/cli/run.go — Only 3 of ~15 sandbox.Upload() calls were migrated to UploadFile(). Other single-file upload sites (lines 725, 763, 801, 826, 919, 1188, 1528, 1559, 1629, 1651) may have the same directory-creation bug if openshell consistently treats the remote path as a directory. Consider auditing remaining call sites and migrating any that expect file-to-file semantics.
    Remediation: Audit each remaining sandbox.Upload() call in run.go to determine whether it expects file-to-file or file-to-directory semantics, and migrate file-to-file callers to UploadFile().

  • [injection-defense] internal/sandbox/sandbox.go:397 — The mv command in UploadFile uses fmt.Sprintf("mv %s %s", ...) without shell-quoting the paths. If a path contained spaces or shell metacharacters, the command would fail or behave unexpectedly. This is a pre-existing pattern in the codebase (e.g., UploadDir, chmod calls) and paths are infrastructure-controlled, but it is not defensive.
    Remediation: Use shell-safe quoting for paths in sandbox Exec commands. Consider a helper function for constructing sandbox shell commands with proper quoting.

Info

  • [documentation-currency] docs/guides/dev/cli-internals.md:346 — The sandbox function reference table lists Upload(), Download(), and SafeDownload() but omits UploadFile() and the pre-existing DownloadFile(). This is a pre-existing gap widened by the new function.
    Remediation: Add UploadFile() and DownloadFile() to the sandbox function table in the CLI internals guide.

// If that differs from the desired remotePath, rename it in-sandbox.
uploadedPath := filepath.Join(remoteDir, filepath.Base(localPath))
if uploadedPath != remotePath {
mvCmd := fmt.Sprintf("mv %s %s", uploadedPath, remotePath)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[low] injection-defense

The mv command in UploadFile uses fmt.Sprintf without shell-quoting paths. Pre-existing pattern but not defensive against paths with spaces or metacharacters.

Suggested fix: Use shell-safe quoting for paths in sandbox Exec commands. Consider a helper function for constructing sandbox shell commands.

@fullsend-ai-review fullsend-ai-review Bot added the ready-for-merge All reviewers approved — ready to merge label May 28, 2026
openshell sandbox upload treats the remote path as a directory,
placing the uploaded file inside it with the source basename.
This caused host_files staging to create a directory at the
destination path (e.g. prior-review.txt/) with the file nested
inside, producing EISDIR errors when the agent tried to read
the file.

Add sandbox.UploadFile() — analogous to the existing
DownloadFile() — which uploads to the parent directory and
renames in-sandbox if the basename differs. Update all three
sandbox.Upload() calls in bootstrapEnv's host_files staging
and .env upload to use UploadFile instead.

Note: Go tests could not run in sandbox (go.mod requires
go >= 1.26.0, sandbox has 1.24.13 with read-only GOPATH).
Pre-commit could not run (same Go toolchain issue). Manual
verification of go-test is required.

Closes #1649
@rh-hemartin rh-hemartin self-assigned this Jun 1, 2026
@rh-hemartin
rh-hemartin force-pushed the agent/1649-fix-upload-file-staging branch from 907d482 to ea8b30e Compare June 1, 2026 10:10

// openshell places the file as <remoteDir>/<basename(localPath)>.
// If that differs from the desired remotePath, rename it in-sandbox.
uploadedPath := filepath.Join(remoteDir, filepath.Base(localPath))

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[low] shell-quoting

UploadFile constructs the in-sandbox mv command via fmt.Sprintf("mv %s %s", ...) without shell-quoting either path. If a path contains spaces or shell metacharacters, the command will fail or behave unexpectedly. Current callers pass safe paths. This matches the pre-existing pattern in UploadDir and is not a regression.

Suggested fix: Use shell-quoted format strings (e.g. single-quote wrapping) consistent with a broader cleanup of the same pattern in UploadDir.

@fullsend-ai-review fullsend-ai-review Bot added ready-for-merge All reviewers approved — ready to merge and removed ready-for-merge All reviewers approved — ready to merge labels Jun 1, 2026
@rh-hemartin

Copy link
Copy Markdown
Member

Closing in favour of #1887

@rh-hemartin rh-hemartin closed this Jun 4, 2026
@github-actions
github-actions Bot deleted the agent/1649-fix-upload-file-staging branch July 5, 2026 04:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-for-merge All reviewers approved — ready to merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Fix prior review artifact staging: file downloaded as directory causes EISDIR

1 participant