fix(#6691)!: keep OpenShell 0.0.111+ sandboxes alive for later exec - #6696
Conversation
Site previewPreview: https://ebfc6b46-site.fullsend-ai.workers.dev Commit: |
|
🤖 Finished Review · ✅ Success · Started 7:10 PM UTC · Completed 7:28 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $5.10 |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
Risk Assessment: moderate (2/5) DetailsModerate risk. The change touches a high-churn area (sandbox.go has 21 commits and 40 fix/reverts in 90d) which elevates git-history risk, but the change is well-scoped (339 lines, 8 files) with 227 test lines added, authored by a bot with repo history, and targets a clearly-specified high-priority bug with all acceptance criteria addressed. Rollback safety is the main concern as this modifies existing sandbox behavior without a feature flag. Previous runRisk Assessment: moderate (2/5) DetailsModerate risk. The change touches a high-churn area (sandbox.go has 21 commits and 34 fix/reverts in recent history) which elevates git-history risk. However, the change itself is well-scoped (334 lines, 7 files), authored by a bot with repo history, and targets a clearly-specified high-priority bug with concrete acceptance criteria all addressed. Additional lines are predominantly tests (227 added), strengthening confidence. Previous run (2)Risk Assessment: moderate (2/5) DetailsModerate risk. The change touches a high-churn area (sandbox.go has 21 commits and 40 fix/reverts in recent history) which elevates git-history risk. However, the change itself is well-scoped (160 lines, 6 files), authored by a bot with repo history, and targets a clearly-specified high-priority bug with concrete acceptance criteria. |
ReviewFindingsMedium
Low
Previous runReviewFindingsMedium
Low
Previous run (2)ReviewFindingsCritical
Medium
Low
Next steps:
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
🤖 Finished Fix · ❌ Failure (post-script /home/runner/work/fullsend/fullsend/.fullsend/.fullsend-cache/resources/sha256/416e3495dbe6b3d0ece160ca879d45f30595f26b71c301116c148872b408d019/scripts/post-fix.sh failed: exit status 1) · Started 7:30 PM UTC · Completed 7:39 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $1.77 |
|
🤖 Review · Commit: |
|
The fix agent completed, but the post-fix script failed before finishing. Workflow run: https://github.com/fullsend-ai/fullsend/actions/runs/33108617191 Details: |
2287af4 to
b58c640
Compare
|
🤖 Review · Commit: |
b58c640 to
d7ef5d9
Compare
|
🤖 Review · Commit: |
waynesun09
changed the title
d7ef5d9 to
db3710d
Compare
|
🤖 Finished Review · ✅ Success · Started 8:03 PM UTC · Completed 8:21 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $4.97 |
Superseded by updated review
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
added
the
requires-manual-review
OpenShell 0.0.111 introduced canonical main-process lifecycle semantics: when the bootstrap command exits, the sandbox becomes terminal instead of staying Ready. Fullsend's `-- true` invocation triggered this, causing sandbox creation to retry and fail. Changes: - Replace `-- true` with `--detach -- sleep infinity` so the sandbox keeps a long-running process and stays Ready for subsequent sandbox exec calls. - Detect terminal sandbox phases during the ready-polling loop and fail immediately instead of burning the full timeout. - Preserve the original sandbox create stderr/output in error messages even when sandbox get succeeds, so diagnostic context is not silently discarded. - Bump the OpenShell version pin from 0.0.83 to 0.0.115 across the version file, VM setup fallbacks, and docs. - Make the version-specific fallback comment in setup.sh version-neutral. Closes #6691 BREAKING CHANGE: Fullsend now requires OpenShell 0.0.111 or newer. `sandbox create` is invoked with `--detach`, a flag that does not exist in 0.0.83 (the previous pin), so a local `openshell` older than 0.0.111 fails sandbox creation outright. Reinstall the pinned version before upgrading: the install snippet in docs/guides/user/running-agents-locally.md now pins 0.0.115. CI and the GitLab runner VMs pick this up automatically from .github/scripts/openshell-version.sh.
The version pin moved to 0.0.115 but OPENSHELL_SHA still pointed at v0.0.83's commit (e3d26dd), so .github/scripts/install-openshell.sh fetched the old release's install.sh. Resolve the tag the same way scripts/renovate/update-openshell-sha.sh does and pin the v0.0.115 commit, keeping the version and SHA consistent. Assisted-by: Claude (fix) Signed-off-by: Wayne Sun <gsun@redhat.com>
…field Phase detection searched the entire `openshell sandbox get` output for a phase name. That output also carries the sandbox name, labels, annotations and the formatted active policy YAML, so an unrelated occurrence of "Error" or "Completed" would abort a healthy creation on its first poll, and one of "Ready" would report a still-provisioning sandbox as ready. Match the "Phase:" field for both, tolerating ANSI escapes around either the label or the value. Also correct the terminal phase set. OpenShell v0.0.115 has no "Dead" phase (phase_name maps Unspecified, Provisioning, Ready, Error, Deleting, Stopping, Stopped, Starting, Unknown), so drop it. "Error" is what 0.0.111+ reports when the main process exits; "Completed" is retained for the pending upstream exit-zero mapping. Wrap the exec error in the immediate create-failure path: when openshell cannot execute at all the combined output is empty and that error is the only diagnostic available. Assisted-by: Claude (fix), Codex (review), Gemini (review), Grok (review) Signed-off-by: Wayne Sun <gsun@redhat.com>
awaitDeletion and awaitCreation only observed cancellation through a select against time.After(delay). Tests set resetRetryDelay to 0, so both channels are ready on every iteration and Go picks between them at random; when the timer won every round the loop ran to exhaustion and returned nil (or the "not visible" error) instead of the cancellation error. TestAwaitDeletion_ContextCancellation and TestAwaitCreation_ContextCancellation failed three consecutive CI runs on this basis. Check ctx.Err() at the top of each attempt so a cancelled context is honoured deterministically, before any further forge call. The select still covers cancellation that arrives mid-backoff. Assisted-by: Claude (fix) Signed-off-by: Wayne Sun <gsun@redhat.com>
ADR 0030 records the sandbox lifecycle with `-- true` as the entry command and explains `--keep` in terms of that command exiting. #6691 changes the invocation to `--detach -- sleep infinity`, leaving the recorded command inaccurate. ADR 0030 is Accepted on main, so per docs/contributing/adrs.md its Decision section is not rewritten; this adds only the short annotation that convention allows, pointing at the newer decision. Assisted-by: Claude (fix), Codex (review), Gemini (review), Grok (review) Signed-off-by: Wayne Sun <gsun@redhat.com>
db3710d to
1a46686
Compare
|
🤖 Finished Review · ✅ Success · Started 8:28 PM UTC · Completed 8:49 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $7.57 |
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Took over from the bot; reviewed with Grok, Codex, and Gemini, all findings addressed. Protected-path change (.github/scripts/openshell-version.sh) is the OpenShell pin bump authorized by #6691; SHA refreshed to the v0.0.115 tag commit. Validated against real OpenShell 0.0.115 via functional-tests and e2e.
|
🤖 Finished Retro · ✅ Success · Started 9:26 PM UTC · Completed 9:39 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $4.82 |
Retro: PR #6696 — keep OpenShell 0.0.111+ sandboxes aliveTimeline
What went well
Evidence for existing open issues
Proposals filed
|
Renovate only tracks .github/scripts/openshell-version.sh. The runner VM
fallbacks in hack/gitlab-runner-vm/{create-vm,setup}.sh and the install
snippet in docs/guides/user/running-agents-locally.md carry the same
version by hand and were left at 0.0.115, so move them together with the
pin as #6696 did.
The docs snippet matters beyond consistency: 0.0.115's Apple Silicon
binaries were linked against a Nix store libiconv and could not start
(NVIDIA/OpenShell#2997), and the guide pointed macOS users at exactly
that build. 0.0.116 ships the upstream fix (NVIDIA/OpenShell#3006), so
pointing the guide at it resolves the macOS install path.
Closes #6707
Assisted-by: Claude
Signed-off-by: Wayne Sun <gsun@redhat.com>
1 participant
Summary
Adapts Fullsend's persistent sandbox creation to work with OpenShell 0.0.111+, which introduced canonical main-process lifecycle semantics. The old
-- truebootstrap command exits immediately, making the sandbox terminal; the new--detach -- sleep infinitykeeps a long-running process so the sandbox stays Ready for subsequentsandbox execcalls.Breaking change
Fullsend now requires OpenShell 0.0.111 or newer.
sandbox createisinvoked with
--detach, which does not exist in 0.0.83 (the previous pin), soa local
openshellolder than 0.0.111 fails sandbox creation outright.Reinstall the pinned version before upgrading — the install snippet in
docs/guides/user/running-agents-locally.mdnow pins 0.0.115. CI and theGitLab runner VMs pick this up automatically from
.github/scripts/openshell-version.sh.Related Issue
Closes #6691
Changes
-- truewith--detach -- sleep infinityincreateOnceso sandboxes remain Ready after creationsandbox createstderr/output in error messages for better diagnosticsOPENSHELL_SHAto thev0.0.115tag commit so the pinned installer matches the pinned versionhack/gitlab-runner-vm/setup.shversion-neutralUnrelated CI flake fixed here (blocking)
pkg/behaviourtest/drivers/install/ensure.gois not part of the sandboxchange.
awaitDeletionandawaitCreationobserved context cancellationonly through a
selectagainsttime.After(delay); their tests setresetRetryDelay = 0, so both channels are ready every iteration and Gopicks between them at random. When the timer won every round the loop ran to
exhaustion and returned the wrong result.
TestAwaitDeletion_ContextCancellation/TestAwaitCreation_ContextCancellationfailed three consecutive
testruns on this branch and reruns were notconverging, so the fix is included here to unblock the merge queue. The file
is byte-identical between
origin/mainand this branch, so the flake ispre-existing and not caused by this PR. It can be split into its own PR on
request.
Notes on phase detection
Both the terminal-phase check and the readiness check are anchored to the
Phase:field ofopenshell sandbox getoutput rather than searching thewhole output. That output also carries the sandbox name, labels, annotations,
and the formatted active policy YAML, so an unanchored substring search could
abort a healthy creation on its first poll, or report a still-provisioning
sandbox as ready. The match tolerates ANSI escapes around either the field
label or the value.
Anchoring is fail-open for terminal detection (a parse miss simply stops the
fast-fail and the loop times out as before) but would be fail-closed for the
readiness check — a future output-format change would strand every healthy
sandbox until the timeout. So readiness falls back to the historical substring
check when, and only when, no phase field parses at all. A decoy cannot slip
through that fallback, because a decoy requires a phase field to be present and
say otherwise.
The terminal set is
ErrorandCompleted. OpenShell v0.0.115 reportsErroronce the canonical main process exits;Completedis reserved for thepending upstream exit-zero mapping (NVIDIA/OpenShell#2884) and is inert until
that lands. There is no
Deadphase —phase_name()incrates/openshell-cli/src/commands/common.rsemits onlyUnspecified,Provisioning,Ready,Error,Deleting,Stopping,Stopped,Starting, andUnknown.Edit to an accepted ADR
Per
docs/contributing/adrs.md, edits to Accepted ADRs must be called out here.docs/ADRs/0030-openshell-sandbox-interaction-model.mdrecords the sandboxlifecycle with
-- trueas the entry command, which this PR makes inaccurate.Its Decision section is not rewritten; only the short annotation the
convention permits is added, pointing at #6691.
Testing
go test ./internal/sandbox/...passesgo vet ./internal/sandbox/...passesgofmtpassesshellcheckpasses on the touched shell scriptspre-commitpassesCloses #6691
Post-script verification
agent/6691-openshell-persistent-sandbox)