Skip to content

fix(cli): plan sandbox uploads before provisioning - #4193

Merged
johntmyers merged 1 commit into
NVIDIA:mainfrom
ericcurtin:fix/4046-upload-preflight/ericcurtin
Oct 6, 2026
Merged

johntmyers merged 1 commit into
NVIDIA:mainfrom
ericcurtin:fix/4046-upload-preflight/ericcurtin

Conversation

@ericcurtin

Copy link
Copy Markdown
Contributor

Summary

sandbox create --upload now plans every upload before it provisions the sandbox. A bad upload no longer leaves a running sandbox.

Related Issue

Closes #4046

Changes

  • Plan all uploads up front in sandbox_create, so a missing path, Git failure, or empty selection fails before create.
  • Share one transfer helper between sandbox upload and sandbox create --upload.
  • Drop the duplicate missing-path check in main.rs.
  • Transfer failures after provisioning now name the kept sandbox and the recovery commands.
  • Update the sandbox overview docs and the CLI skill.

Testing

  • Checks appropriate to the affected code and behavior pass
  • Unit tests added/updated (if applicable)
  • E2E tests added/updated (if applicable)

cargo fmt, cargo clippy -p openshell-cli --all-targets -- -D warnings, and cargo test -p openshell-cli (lib, bin, sandbox_create_lifecycle_integration, sandbox_upload_integration) pass. The updated integration test covers Git failure, empty selection, missing path, and a valid upload followed by a rejected one: no create request and no SSH session.

Checklist

  • Follows Conventional Commits
  • Commits are signed off (DCO)
  • Architecture docs updated (if applicable)

Reject bad uploads before create. Share the upload transfer helper.

Signed-off-by: Eric Curtin <eric.curtin@docker.com>
@copy-pr-bot

copy-pr-bot Bot commented Oct 5, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@ericcurtin

Copy link
Copy Markdown
Contributor Author

If useful, please also try https://github.com/llmmanorg/llmman, which can launch agents in an OpenShell sandbox (--sandbox openshell).

@drew @krishicks PTAL when you get a chance, and /ok to test d3a0ca6a1d2e170a9cd78d3751d9f2b126b89a6f if it looks good. Thank you!

@johntmyers johntmyers added the test:e2e Requires end-to-end coverage label Oct 6, 2026
@johntmyers

Copy link
Copy Markdown
Collaborator

/ok to test d3a0ca6

@github-actions

github-actions Bot commented Oct 6, 2026

Copy link
Copy Markdown

Label test:e2e applied for d3a0ca6. Open Branch E2E Checks, find the run for commit d3a0ca6, and click Re-run all jobs to execute with the label set. The run will execute the standard E2E suite after building the required gateway, sandbox, and supervisor images once. The matching required CI gate status on this PR will flip green automatically once the run finishes.

@johntmyers

johntmyers commented Oct 6, 2026 •

Copy link
Copy Markdown
Collaborator

gator-agent

PR Review Status

Eric Curtin, I checked your request for review and test authorization against the complete patch and issue #4046. The independent review found no blocking defects: upload planning now precedes sandbox provisioning, transfer failures retain the sandbox with recovery guidance, and the main docs and CLI skill describe the behavior.

test:e2e is applied and /ok to test was posted for the current head. Branch Checks, Helm Lint, and Branch E2E Checks are running on the refreshed mirror. Trivy Changes is still held for workflow approval; the sandbox policy denied Gator's approval request.

Action required: a maintainer must approve the Trivy Changes run, or the operator must allow its workflow-approval endpoint in the sandbox policy.

Blocking findings: None.

Carried findings: None.

Non-blocking suggestion: update the final recovery sentence in docs/upgrade/0-1-0.mdx:85 to distinguish planning rejection, which creates no sandbox, from transfer failure, which retains it.

Gator metadata
  • Validation: Focused CLI fix implements the stated preflight and recovery requirements of bug(cli): validate all creation-time upload plans before provisioning #4046.
  • Docs: Sandbox overview and public CLI skill updated; no navigation change needed.
  • Checks: DCO and vouch pass. Branch Checks and Helm Lint started; Trivy Changes requires workflow approval.
  • E2E: test:e2e applied; fresh current-head Branch E2E Checks run 37527764498 started after the label was applied. Label Help's rerun hint refers to an existing run; this mirror had no earlier E2E run to rerun.
  • Head SHA: d3a0ca6a1d2e170a9cd78d3751d9f2b126b89a6f
  • Base SHA: 71c3cd957abef062eb7f37010056717cd49f2ed3
  • Merge base SHA: 71c3cd957abef062eb7f37010056717cd49f2ed3
  • Patch ID: 16d16f339803881bdaf685b2c30eb1511a3d9e39
  • Gator payload: 10
  • Review mode: initial
  • Previous reviewed SHA: none
  • Review budget exhausted: no
  • Maintainer decision required: no
  • Next state: gator:blocked
  • Blocked reason: workflow_approval_required

@johntmyers johntmyers added gator:blocked Gator is blocked by process or repository gates gator:watch-pipeline Gator is monitoring PR CI/CD status gator:approval-needed Gator completed review; maintainer approval needed and removed gator:blocked Gator is blocked by process or repository gates gator:watch-pipeline Gator is monitoring PR CI/CD status labels Oct 6, 2026
@johntmyers
johntmyers enabled auto-merge October 6, 2026 21:48
@johntmyers
johntmyers added this pull request to the merge queue Oct 6, 2026
@johntmyers johntmyers added gator:merge-ready and removed gator:approval-needed Gator completed review; maintainer approval needed labels Oct 6, 2026
Merged via the queue into NVIDIA:main with commit 3082a9a Oct 6, 2026
124 of 127 checks passed
@johntmyers

Copy link
Copy Markdown
Collaborator

gator-agent

Monitoring Complete

Monitoring is complete because this PR has merged.

Final status: The last Gator state was gator:merge-ready; the current head already received an independent review with no blocking findings. No further monitoring is needed.

The active gator:* label is being cleared as part of terminal cleanup.

Gator metadata
  • Head SHA: d3a0ca6a1d2e170a9cd78d3751d9f2b126b89a6f
  • Gator payload: 10

@ericcurtin
ericcurtin deleted the fix/4046-upload-preflight/ericcurtin branch October 6, 2026 22:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

test:e2e Requires end-to-end coverage

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug(cli): validate all creation-time upload plans before provisioning

2 participants