feat: add Islo sandbox provider#3616
Conversation
Co-authored-by: Cursor <cursoragent@cursor.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 63390d51a4
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| except Exception as e: | ||
| logger.debug("islo sandbox get() failed, will recreate: %s", e) |
There was a problem hiding this comment.
When get_sandbox() or resume_sandbox() fails for a transient/server/auth error rather than a 404, this broad catch still falls through to the recreation path. In that scenario a paused or running Islo sandbox can still exist with the user's workspace, but resume() mutates the saved state to a newly created sandbox (and, without a restorable local snapshot, start() will just apply the manifest), effectively losing access to the existing workspace and orphaning the original sandbox. Only not-found/stale-sandbox errors should trigger recreation; other provider errors should be retried or surfaced.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch, agreed. I tightened resume() so it only recreates when get_sandbox() / resume_sandbox() reports not-found or stale metadata; other provider failures are now surfaced and the client is closed instead of mutating saved state to a new sandbox. I also added regression coverage for transient get_sandbox() and resume_sandbox() failures to ensure we do not call create_sandbox() in those cases.
Verification: make format && make lint && make typecheck && make tests.
Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 52ed710ce2
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| raise | ||
| logger.debug("islo sandbox metadata was stale, will recreate: %s", e) | ||
|
|
||
| if not reconnected: |
There was a problem hiding this comment.
When get_sandbox() succeeds but returns a non-terminal transitional status such as starting or stopping, reconnected stays false and this path immediately falls through to creating a replacement sandbox. Because the original sandbox name can still be reserved, the name-conflict fallback creates a generated-name sandbox and mutates the saved state to it, orphaning the existing workspace even though it may become usable moments later. Only definitely unavailable/stale states should recreate; transitional statuses should be retried or surfaced.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed. I pushed 586cda7. resume() now treats a live but not-ready status as an error, so starting and similar states surface instead of entering the recreate path. The recreate path remains limited to not-found or stale metadata. I added a regression test for starting that fails before the fix because create_sandbox() is reached, and passes now with no create call.
Verification:
uv run pytest tests/extensions/sandbox/test_islo.py -q
make format && make lint && make typecheck && make tests
Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 586cda7d78
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| await self._delete_backend() | ||
| except Exception: | ||
| pass |
There was a problem hiding this comment.
When hydrating an Islo native snapshot and delete_sandbox fails for transient/auth/server reasons, this catch ignores it and proceeds to create a replacement. If the old sandbox still exists, the name-conflict fallback below can create a generated-name sandbox and _apply_sandbox_response mutates state to it, orphaning the original sandbox and losing access to its current workspace. Only a not-found delete should be ignored; other provider errors should abort the restore.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Intentional. _replace_sandbox_from_snapshot is a destructive replace, not a reconnect. The delete is best-effort cleanup before create_sandbox recreates from the snapshot. If the delete fails (404, transient, auth), the create still needs to happen because the caller asked for a snapshot restore. Aborting on a delete failure would turn a recoverable restore into a hard error. The name-conflict fallback on L848-859 correctly handles the case where the old sandbox is still lingering after a failed delete.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5bd91e0418
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| try: | ||
| snapshot = await asyncio.wait_for( | ||
| self._client.snapshots.create_snapshot( | ||
| sandbox_name=self.state.sandbox_name, |
There was a problem hiding this comment.
When users select workspace_persistence="snapshot" with the declared openai-agents[islo] extra, the lockfile currently resolves islo==0.3.1, whose async create_snapshot method takes sandbox_id rather than sandbox_name. This call therefore raises TypeError during stop()/snapshot persistence before any workspace state is saved, so the new snapshot mode is broken for the minimum supported Islo SDK; please branch on the SDK signature or use self.state.sandbox_id for older versions.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Re: #3616 (comment)
Good catch. The lockfile resolved islo==0.3.1 which uses sandbox_id in create_snapshot, but the code passes sandbox_name. Bumped the minimum to islo>=0.3.3 which already uses sandbox_name (c696518).
islo 0.3.1 uses sandbox_id in create_snapshot; 0.3.3+ uses sandbox_name which matches the provider code. Co-authored-by: Cursor <cursoragent@cursor.com>
2 participants
Summary
Test plan
make lintmake typecheckmake testsMade with Cursor