Skip to content

Add provider-neutral sandbox execution with sbx and Islo backends - #22637

Open
zozo123 wants to merge 32 commits into
PrefectHQ:mainfrom
zozo123:add-sandbox-integration
Open

Add provider-neutral sandbox execution with sbx and Islo backends#22637
zozo123 wants to merge 32 commits into
PrefectHQ:mainfrom
zozo123:add-sandbox-integration

Conversation

@zozo123

@zozo123 zozo123 commented Jul 27, 2026

Copy link
Copy Markdown

Summary

Ready for focused design review against #22636. @desertaxle asked to settle what a sandbox represents in Prefect before any public API lands, so the prebuilt task API was removed and this branch now carries only the provider-neutral backend layer, its two adapters, and an explicitly provisional imperative operation.

  • add an in-tree prefect-sandbox integration with a provider-neutral lifecycle for creating, executing in, writing to, and destroying disposable sandboxes
  • add a Docker Sandboxes (sbx) backend with isolated per-run state, explicit egress-policy verification, bounded output, host-enforced timeouts, and failure-visible cleanup
  • add an Islo backend for hosted microVMs: API-key auth exchanged host-side for a short-lived session token, SSE-streamed output capped as it arrives, native file upload, egress denied at provisioning time, and a provider-side reaper for sandboxes a killed worker never tore down
  • add a cross-provider conformance suite that runs the same assertions against both backends, so the eight contract invariants are tested rather than documented

Two transports with nothing in common — a local CLI and a hosted REST API — implementing the same contract is what makes the abstraction credible. Neither backend's shape leaks into the other.

Changed since the first review

Both automated review findings on sbx.py from the first pass are fixed, each with a regression test that fails if the fix is reverted:

  • P1, sbx.py acreate — with egress="deny", an OSError launching the policy subprocess happens after sbx create has succeeded, and _acli reports it as SandboxUnavailableError. The cleanup branch keyed on that exception type, concluded nothing had been provisioned, removed only the workspace, and leaked a live microVM — still holding exactly the egress the caller asked to block. Cleanup now tracks whether the CLI was launched rather than inferring it from the exception type. The same bug class was found and fixed in IsloSandbox independently.

  • P2, _write_host_temp_file — a write that died after mkstemp never returned the path, so awrite_file's finally could not unlink it, stranding a partial file on the worker exactly when the disk was already full. The helper now cleans up after itself.

  • P2, operations.py aclose — a with block inside async code closes on Prefect's sync loop while the thread owning the provisioning loop is blocked, so cancelling that provisioning could only be scheduled, never awaited, and a sandbox already created was abandoned. Joining the foreign task is not an option — it deadlocks unconditionally, which I verified — so the handle is now published as soon as acreate returns and the closing loop destroys it itself. aclose's docstring had claimed the guarantee it did not provide, and its INFO line reported closing zero sandboxes while one was live; both are fixed and the api-ref is regenerated.

  • the four prebuilt tasks in tasks.py are removed. They committed to a public API ahead of the concept decision; SandboxOperation remains as the imperative escape hatch and is explicitly provisional.

  • design comparison of the three concepts @desertaxle named, with all eight lifecycle behaviors filled in, posted at Add a provider-neutral sandbox execution integration #22636 (comment)

Latest review fixes and runnable demo

Current head 9995253 closes every automated review and security finding, with a regression test for each:

  • P1, bounded Islo SSE framing — one unterminated stdout/stderr event can no longer bypass max_output_bytes; the incremental decoder retains a fixed framing budget, marks the retained output truncated, discards the rest of that event, and still parses the following exit event.
  • P1, cancellation-safe multi-process closeSandboxOperation.aclose() now shields the complete captured cleanup generation, so cancellation is re-delivered only after every moved-out process has been visited.
  • P2, endpoint-stable Islo handles — auth, exec, upload, and destroy all use the api_url recorded in Sandbox.metadata, including from a differently configured block instance.
  • Live-only cold-create edge case — the real Islo E2E showed that a cold public-image create can take longer than the old 60-second control timeout and that Islo briefly rejects deletion while status is creating. Create now receives the configured provisioning budget, and failed-create cleanup polls and retries deletion through that transition.
  • Complete two-provider demoexamples/hello_sandbox.py runs the same Prefect flow and uploaded Python file with either sbx or Islo. The README and integration guide contain the two commands.
  • Concurrent lifecycle cleanup — same-loop close callers join one teardown generation, cross-loop callers avoid deadlock with idempotent out-of-band deletion, and failed teardown/provisioning cleanup remains retryable instead of losing the sandbox handle.
  • Strict command input and bounded Islo cleanup retries — bare string/bytes commands are rejected consistently before normalization by the operation layer and both providers; Islo retries the first transition to running immediately, then backs off after another 400/409 instead of hot-looping.
  • Authenticated, rotation-safe Islo handles — every exec, upload, and destroy requires an HMAC binding the sandbox name and creation endpoint to a dedicated stable handle_signing_key, independent of the provider API key. Prefect persists its generated value with a saved block; independently constructed workers can share it through PREFECT_SANDBOX_ISLO_HANDLE_SIGNING_KEY. Missing or altered metadata is rejected before any credential or request is sent, while rotating the Islo API key leaves live handles usable for teardown.
  • Atomic, key-bound Islo sessions — token, expiry, endpoint, and private credential identity live in one immutable snapshot, so sync/async loops cannot observe a token beside another tenant's key during rotation. Each operation captures one credential consistently across auth and lifecycle calls. The SecretStr identity is compared in constant time, and the complete snapshot is explicitly removed from pickle/cloudpickle payloads so process workers re-authenticate and no raw provider key crosses the serialization boundary.
  • Authenticated Docker Sandbox handles — every sbx exec, copy, and destroy requires a host-signed HMAC binding the sandbox id and canonical workspace. The 0600 signing key is atomically published outside the guest mount; unsigned or altered handles are rejected before any targeting subprocess.
  • Failure-safe Docker handle-key publication — cleanup of the 0600 key's staging file now encloses writing, flushing, syncing, and atomic publication; an unlink failure is surfaced explicitly. Parameterized regressions cover all three disk-pressure write stages, so retries cannot accumulate partial host keys when cleanup succeeds.
  • Byte-faithful cross-platform staging — sbx uploads stage explicit UTF-8 bytes in binary mode, preventing Windows newline conversion while preserving Unicode and exact file contents.
  • Portable fallback stays within argv limits — the full 256 KiB inline ceiling is retained, but base64 now travels in 32 KiB positional chunks rather than one Linux-incompatible argument; the exact-ceiling command is executed byte-exact in tests.
  • Cancellation-safe, failure-visible sbx file staging — cancellation joins an in-flight host staging thread and completes deletion before propagating; final post-copy deletion is shielded too, and any deletion error other than an already-absent path is reported.
  • Crash-durable sbx handle keys — POSIX hosts fsync a newly created key directory in its parent, then fsync the key directory after atomic publication and before trusting an existing key, so a power loss cannot silently rotate the authenticator for live handles.

Fresh validation on current head: 567 tests passed with 96% statement coverage; mypy, Ruff, formatting, interrogate (100%), pre-commit, and package build passed. The committed demo also passed against both real providers: Docker Sandboxes v0.37.0 printed guest kernel 7.0.12; Islo printed 6.16.9+; both exited 0 and destroyed their sandbox. Deeper live matrices on both providers verified large-file fidelity, no ambient Prefect/AWS credentials, explicit env only, denied egress, exactly 1024/200000 output bytes retained, nonzero exits as data, timeout destruction, idempotent teardown, and zero test resources afterward.

Deliberately not in this PR

Two registration points that other integrations have are left out on purpose, because both reference a package that is not on PyPI yet. Flagging them rather than guessing at the convention:

  • prefect[sandbox] extra in the root pyproject.toml, alongside docker = ["prefect-docker>=0.6.0"] and shell = [...]. Adding it before the first release would make pip install prefect[sandbox] resolve to nothing.
  • The IsloSandbox block logo is currently https://avatars.githubusercontent.com/islo-labs, a third-party-controlled avatar endpoint that whoever owns that org can change. Every other in-tree block uses a Prefect-hosted CDN asset; happy to swap in whatever you upload.
  • src/prefect/server/collection_blocks_data.json, the whitelist that shows a collection's blocks in the UI catalog before it is installed. An entry here would advertise Sbx Sandbox and Islo Sandbox to users who cannot install them yet. Its entries carry no version or checksum, so it can be added whenever you prefer.

Happy to add either now if you would rather they land with the code. I checked the other places a new integration might need registering and they need nothing: the test matrix is generated from changed paths by scripts/generate_integration_package_tests_matrix.py (which is why this PR already ran the suite on 3.10 through 3.13), and dependabot.yml, CODEOWNERS and labeler.yml do not enumerate integrations.

Also proposed on the issue but deliberately not built, since they add public API ahead of the concept decision: aread_file (the contract is write-only today) and a validate_env denylist that makes invariant 7 enforced rather than documented.

Test plan

  • uv run coverage run --branch -m pytest -q && uv run coverage report — 567 passed, 96% statement coverage
  • full suite on Python 3.10, 3.13 and 3.14
  • uv run ruff format --check src/integrations/prefect-sandbox
  • uv run ruff check src/integrations/prefect-sandbox
  • uv run mypy prefect_sandbox
  • uv run interrogate prefect_sandbox --config pyproject.toml — 100%
  • generated API reference is current
  • docs navigation and internal links verified: every page in docs.json resolves, no broken internal links in the changed pages, nothing references the removed api-ref page (mint broken-links itself does not run on Node 25+, so this was checked directly)
  • the fixes below are mutation-tested: reverting any one of them fails a named test. This is per-fix, not a claim of
    full mutation coverage — reverting invariant 5 in sbx.py from cap-while-streaming to buffer-then-trim, for instance,
    is not currently caught, because the distinguishing property is memory rather than output

The Islo backend's tests use httpx.MockTransport, so no network access and no credentials are needed in CI.

Validated against the real providers, outside CI. SbxSandbox was run end to end against a real Docker Sandboxes microVM on macOS (sbx v0.37.0, revalidated after upgrade), and all checks passed:

Checked live Result
Kernel isolation is real guest Linux 7.0.12 aarch64, host macOS
No ambient credentials (invariant 7) PREFECT_API_KEY and AWS_SECRET_ACCESS_KEY both absent in the guest; the caller's env present
awrite_file fidelity 300 KB round-tripped byte-exact, quoting and unicode intact, parents created
Output cap (invariant 5) exactly 1024 of 200 KB retained, truncated set
Timeout honesty (invariant 6) 20 s budget stopped at 25.6 s with timed_out and sandbox_terminated, and the microVM was confirmed gone from the host
egress="deny" DNS resolution inside the guest returned BLOCKED
Teardown destroy idempotent, host workspace removed, no prefect-sandbox-* left behind

IsloSandbox was then run end to end against the real hosted API on api.islo.dev, and every check passed:

Checked live Result
Key-to-session-token exchange POST /auth/token in 0.5 s; the raw API key is not accepted as a bearer token, which is the one part of this backend that cannot be inferred from the public OpenAPI document
acreate a microVM in 2.2 s, provider id carried in Sandbox.metadata
aexec stdout, stderr and exit 0 over SSE; a nonzero exit returned as data
No ambient credentials (invariant 7) PREFECT_API_KEY absent in the guest, the caller's env present
awrite_file 400 KB uploaded natively and read back byte-exact, well past the inline fallback ceiling
Output cap (invariant 5) exactly 1024 bytes retained, truncated set
Timeout honesty (invariant 6) timed_out and sandbox_terminated set, and the sandbox really destroyed
Teardown adestroy idempotent; asession destroys, after which exec correctly fails
Failure paths a rejected create surfaced the provider's own code and left no sandbox behind (invariant 4, live); adestroy on a never-existent name succeeded (invariant 3, live); /exec/stream and /files against an unknown sandbox both surfaced SANDBOX_NOT_FOUND legibly

Checklist

  • This pull request references a related issue using closes.
  • A maintainer has confirmed the proposed approach on the linked issue.
  • Functionality includes behavior-focused unit and lifecycle-race tests.
  • User-facing behavior includes installation, security, and provider-onboarding documentation.
  • No documentation files are removed.
  • Added functions and classes include docstrings.

Closes #22636

AI assistance was used to draft and review portions of the implementation. The contributor reviewed the full diff and ran the checks listed above locally.

Introduce a hardened sandbox lifecycle contract and an initial Docker Sandboxes backend so untrusted commands can run in disposable isolation while future providers share the same Prefect API.

Co-authored-by: Cursor <cursoragent@cursor.com>

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5d8e470b4c

ℹ️ About Codex in GitHub

Your team has set up Codex to 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 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/integrations/prefect-sandbox/prefect_sandbox/sbx.py Outdated
Comment thread src/integrations/prefect-sandbox/prefect_sandbox/sbx.py Outdated
Comment thread src/integrations/prefect-sandbox/prefect_sandbox/operations.py Outdated

@desertaxle desertaxle left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for putting this together @zozo123! I’m supportive of sandbox execution in Prefect, but I don’t think pre-built tasks are the correct Prefect concept for this integration. Before adding public APIs, we need to decide what a sandbox represents in Prefect. I think the design discussion should compare:

  • A worker/work-pool type and infrastructure decorator if the sandbox owns an entire flow run.
  • A task runner if the sandbox determines where submitted Prefect tasks execute.
  • A new durable SandboxRun-like concept if it executes arbitrary untrusted code while keeping the Prefect runtime and credentials outside the guest.

Whichever concept we choose needs to define the unit of isolation, durable identity, input and result transfer, logs, cancellation, retries, credentials, and orphan cleanup. Those lifecycle behaviors are where Prefect can provide differentiated value; run_in_sandbox tasks alone do not.

Can we move this back to the issue for a focused design discussion before committing to a design? The existing sbx implementation may still be useful as the first provider adapter once we agree on the Prefect concept.

zozo123 and others added 2 commits July 28, 2026 23:07
Implement the sandbox contract a second time against a hosted REST provider so the lifecycle is demonstrably vendor-neutral: an API key is exchanged host-side for a short-lived session token, commands stream over SSE with output capped as it arrives, files upload natively, egress is denied at provisioning time, and a provider-side reaper deletes sandboxes a killed worker never tore down. A cross-provider conformance suite runs the same assertions against both backends.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Prebuilt tasks commit to a public API before the design discussion on PrefectHQ#22636 has settled what a sandbox is in Prefect, so remove them and leave the provider-neutral backend contract, its two adapters, and the imperative SandboxOperation escape hatch. Documentation now reaches the same outcomes through SandboxOperation.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@zozo123 zozo123 changed the title Add provider-neutral sandbox execution integration Add provider-neutral sandbox execution with sbx and Islo backends Jul 28, 2026
@zozo123

zozo123 commented Jul 28, 2026

Copy link
Copy Markdown
Author

@desertaxle acted on the review rather than arguing it: the four prebuilt tasks are gone, and the concept comparison you asked for is on the issue (#22636 (comment)) with all eight lifecycle behaviors filled in. Short version — the credential boundary is what makes a sandbox a sandbox, a worker-submitted flow run needs PREFECT_API_KEY inside the guest, so I'd make the durable run the primary concept and keep the work pool type as a later, clearly-scoped layer for trusted code.

This PR stays a draft until you weigh in. What's left in it is the layer you said was useful: the SandboxBackend contract and its adapters. A second adapter now exists alongside sbx — Islo, hosted microVMs over a REST API with key-for-token exchange, SSE-streamed output and native file transfer — and a cross-provider conformance suite runs the same assertions against both, which is how the contract gets tested instead of asserted. Happy to split the contract and the concept into separate PRs if that's easier to review.

Comment thread src/integrations/prefect-sandbox/tests/test_islo.py Fixed
zozo123 and others added 7 commits July 28, 2026 23:14
The unreachable-endpoint test compared the error message against a hardcoded URL, which CodeQL reads as incomplete URL substring sanitization. Reading the constant is both quieter and one less literal to drift.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A token whose payload cannot be read, an error body that is not the provider's JSON shape, and an empty output event once the cap is full were all reachable and untested. Covering them takes the backend to 99 percent and pins the cap arithmetic, which is easy to get wrong once there is no room left.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
With egress="deny" an OSError launching the policy subprocess lands after sbx create has already succeeded, and _acli reports it as SandboxUnavailableError, so keying cleanup on the exception type concluded nothing existed and leaked a live microVM still holding the egress the caller asked to block. Cleanup now tracks whether the CLI was launched. The staging file for sbx cp also cleans up after a failed write, which previously stranded a partial file on a disk that was already full.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
prefect-sandbox was the only one of the nineteen in-tree integrations without one, so its sdist relied on setuptools defaults instead of the shared include and exclude rules. The file is byte-identical to its siblings.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
awrite_file creates the parent directory with aexec, which raises rather than returning a nonzero exit when the sandbox itself is gone, so a failed write surfaced a message about running a command and named neither the path nor the write. Found by pointing the backend at the real API with a sandbox name it had never seen.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A with block inside async code closes on Prefect's sync loop while the thread owning the provisioning loop is blocked, so cancellation of that provisioning could only be scheduled, never awaited, and a sandbox already created was abandoned. Joining the foreign task would deadlock unconditionally, so the handle is published as soon as acreate returns and the closer destroys it from its own loop instead. aclose's docstring claimed the guarantee it did not provide, and its INFO line reported closing zero sandboxes while one was live.

Also drops a guard in the Islo stream reader that became unreachable when the reader started breaking at the exit event, and rewrites two tests that could not fail: one exercised that dead guard, the other asserted URL normalization that httpx performs regardless.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Source permalinks across all four pages pointed at pre-fix line numbers, and prefect_sandbox-operations.mdx still published the cleanup guarantee aclose no longer claims.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@zozo123

zozo123 commented Jul 29, 2026

Copy link
Copy Markdown
Author

@codex review

Since your first pass the branch has changed substantially, and most of it you have not seen:

  • a second backend, IsloSandbox — hosted microVMs over a REST API, API key exchanged host-side for a short-lived session token, SSE-streamed output capped as it arrives, native multipart file upload, egress denied at provisioning time
  • a cross-provider conformance suite running the same assertions against both backends
  • the four prebuilt tasks in tasks.py removed
  • all three of your earlier findings addressed (replies on each thread)

Please pay particular attention to prefect_sandbox/islo.py: the session-token cache is deliberately unguarded by a lock, a fresh httpx.AsyncClient is built per call on purpose because a Block outlives any one event loop, the wall clock is enforced client-side because the provider documents timeout_secs as advisory, and aexec leaves the stream the instant an exit event arrives. Those are the four decisions most likely to be wrong.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 155e20cb78

ℹ️ About Codex in GitHub

Your team has set up Codex to 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 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/integrations/prefect-sandbox/prefect_sandbox/islo.py Outdated
Comment thread src/integrations/prefect-sandbox/prefect_sandbox/operations.py Outdated
Comment thread src/integrations/prefect-sandbox/prefect_sandbox/islo.py Outdated

zozo123 commented Jul 29, 2026

Copy link
Copy Markdown
Author

@desertaxle The implementation side is ready for your design call now:

  • the prebuilt task API you objected to is removed; this PR is narrowed to the provider-neutral backend contract, the two adapters, and an explicitly provisional imperative operation
  • all 7 review threads are resolved with regression coverage
  • 513 tests pass locally with 98% branch coverage; the PR's Python 3.10–3.13 matrix, docs links, pre-commit, type checks, Markdown tests, and CodeQL are green
  • the committed one-file demo passed against both real providers and left no sandbox or temporary credential behind

I have left the PR in draft because the maintainer-confirmation checklist is still genuinely open. Could you confirm whether this narrowed adapter-first scope is acceptable, or whether you want the backend contract split out / a short design doc before it is marked ready?

@zozo123
zozo123 marked this pull request as ready for review July 29, 2026 10:55

zozo123 commented Jul 29, 2026

Copy link
Copy Markdown
Author

Revalidated after the host upgrade and marking this ready for the focused design re-review:

  • Docker Sandboxes sbx v0.37.0: committed demo plus deep matrix passed
  • Islo: committed demo plus the same deep matrix passed using a disposable key that was revoked afterward
  • both providers verified large-file fidelity, ambient-credential isolation, explicit env only, denied egress, bounded output, timeout destruction, and idempotent teardown
  • final inventories: no test sandboxes, no test API keys, no temporary credential files, and no sandbox-scoped sbx policy residue

This does not mark the design checkbox approved; it means the narrowed adapter-first implementation and the #22636 comparison are ready for that decision. Re-requesting @desertaxle.

zozo123 commented Jul 29, 2026

Copy link
Copy Markdown
Author

@codex review

Please review the current head (6be7748). All prior review threads have been addressed and resolved, and the Docker Sandbox v0.37.0 plus Islo live E2E matrices pass with clean final inventories.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 6be77489ef

ℹ️ About Codex in GitHub

Your team has set up Codex to 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 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/integrations/prefect-sandbox/prefect_sandbox/operations.py Outdated

zozo123 commented Jul 29, 2026

Copy link
Copy Markdown
Author

@codex review

Please review exact head ae9bdf0. This pass is scoped to the two P1 findings: Islo cached sessions are now bound to the API-key fingerprint used for signing/provisioning, and Docker Sandbox handles now authenticate sandbox id + canonical workspace before any exec/copy/destroy subprocess. Both threads have regression evidence; 555 tests, Ruff, mypy, and both real-provider demos pass with zero disposable residue.

zozo123 commented Jul 29, 2026

Copy link
Copy Markdown
Author

@codex review

Please review exact head df98dd2. It contains no code change after P1 fix ae9bdf0; the empty commit only retriggers a Python 3.10 CI job that hit a cross-xdist /tmp snapshot race. Scope remains the two P1 fixes and their regressions.

Comment thread src/integrations/prefect-sandbox/prefect_sandbox/islo.py Fixed

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: df98dd2df2

ℹ️ About Codex in GitHub

Your team has set up Codex to 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 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/integrations/prefect-sandbox/prefect_sandbox/sbx.py Outdated

zozo123 commented Jul 29, 2026

Copy link
Copy Markdown
Author

@codex review

Please review exact head 555adc7. P1 code remains in ae9bdf0; this follow-up replaces the raw SHA-256 API-key cache discriminator flagged high by CodeQL with a process-private keyed HMAC fingerprint. Rotation regression, Ruff, mypy, all 555 tests, and a fresh real Islo demo pass; zero disposable sandboxes/keys remain.

Comment thread src/integrations/prefect-sandbox/prefect_sandbox/islo.py Fixed

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 555adc7a37

ℹ️ About Codex in GitHub

Your team has set up Codex to 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 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/integrations/prefect-sandbox/prefect_sandbox/islo.py

zozo123 commented Jul 29, 2026

Copy link
Copy Markdown
Author

@codex review

Please review exact head 76b0fcc. The Islo cache remains tenant-bound, but no longer derives any hash from the API key: it stores the identity only as a redacted, non-serialized SecretStr PrivateAttr and compares it in constant time. Rotation regression, Ruff, mypy, all 555 tests, and a fresh real Islo demo pass; zero disposable residue.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 76b0fccbf0

ℹ️ About Codex in GitHub

Your team has set up Codex to 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 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/integrations/prefect-sandbox/prefect_sandbox/islo.py Outdated

zozo123 commented Jul 29, 2026

Copy link
Copy Markdown
Author

@codex review

Please review exact head aac8539 and report only P1 correctness/security issues. Islo handles now use a dedicated stable signing secret independent of the provider API key; session tokens remain bound to the actual API-key identity. A second worker using key B can exec, upload, and destroy the sandbox created with key A. 557 tests, Ruff, mypy, block save/load, and the real A→B Islo rotation pass with zero disposable residue.

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Swish!

Reviewed commit: aac8539884

ℹ️ About Codex in GitHub

Your team has set up Codex to 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 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

zozo123 commented Jul 29, 2026

Copy link
Copy Markdown
Author

@codex review

Please review exact head 9ae883e and report only P1 correctness or security issues.

This head adds explicit pickle/cloudpickle stripping for all ephemeral Islo session and API-key cache fields. It retains stable, API-key-independent handle signing, and the regression suite now has 558 passing tests at 96% coverage. The real Islo proof created with key A, serialized without key A bytes, then unpickled and re-authenticated with key B to upload, execute, and destroy the original sandbox; cleanup left zero test resources.

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. You're on a roll.

Reviewed commit: 9ae883e9db

ℹ️ About Codex in GitHub

Your team has set up Codex to 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 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

zozo123 commented Jul 29, 2026

Copy link
Copy Markdown
Author

@codex review

Please review exact head 880cae8 for correctness and security issues at every priority.

All prior inline threads are resolved. This head closes the last one by making sbx host-key staging cleanup span write, flush, fsync, and atomic publication, with explicit cleanup-error reporting and parameterized regressions for all three write-stage failures. Full package validation is 561 tests; mypy, Ruff, formatting, interrogate, pre-commit, build, and the real sbx demo pass.

zozo123 commented Jul 29, 2026

Copy link
Copy Markdown
Author

@desertaxle Re-review requested on exact head 9995253.

The branch now implements the concrete changes from your requested-design review:

  • removed the four prebuilt tasks and their public API commitment;
  • narrowed the PR to the provider-neutral backend contract, the sbx and Islo adapters, and an explicitly provisional imperative operation;
  • posted the requested worker/work-pool vs task-runner vs durable SandboxRun comparison on Add a provider-neutral sandbox execution integration #22636, covering isolation, durable identity, transfer, logs, cancellation, retries, credentials, and orphan cleanup;
  • retained runnable two-provider demos and conformance coverage as evidence for the adapter contract without choosing the eventual public Prefect concept.

All automated threads through the latest review are resolved. The commits after the original request are implementation hardening only and do not broaden the public concept. Please clear the existing CHANGES_REQUESTED review if this addresses the design concern; otherwise, please identify the specific remaining gap against the narrowed scope.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 880cae83d7

ℹ️ About Codex in GitHub

Your team has set up Codex to 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 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/integrations/prefect-sandbox/prefect_sandbox/base.py Outdated
Comment thread src/integrations/prefect-sandbox/prefect_sandbox/sbx.py Outdated
Comment thread src/integrations/prefect-sandbox/prefect_sandbox/sbx.py

zozo123 commented Jul 29, 2026

Copy link
Copy Markdown
Author

@codex review

Please review exact head 9a813a2 for correctness and security issues at every priority.

All prior inline threads are resolved. This head addresses the latest three findings with: 32 KiB argv chunks plus an executed 256 KiB ceiling regression; cancellation-safe joining and deletion of sbx host staging; and POSIX directory fsync before a published or newly observed handle key is trusted. Full validation is 564 tests, 96% statement coverage, all static/build/pre-commit checks, and a fresh-home real sbx demo with zero sandbox residue.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9a813a25fe

ℹ️ About Codex in GitHub

Your team has set up Codex to 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 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/integrations/prefect-sandbox/prefect_sandbox/islo.py Outdated
Comment thread src/integrations/prefect-sandbox/prefect_sandbox/sbx.py
Comment thread src/integrations/prefect-sandbox/prefect_sandbox/sbx.py Outdated
Comment thread src/integrations/prefect-sandbox/prefect_sandbox/sbx.py Outdated

zozo123 commented Jul 29, 2026

Copy link
Copy Markdown
Author

@codex review

Please review exact head d6bdd98 for correctness and security issues at every priority.

All prior threads are resolved. This head adds one immutable cross-thread Islo session snapshot, fsyncs a new sbx key directory in its parent before publishing the key, stages UTF-8 bytes in binary mode, and reports every staged-file deletion failure except an already-absent path. Deterministic race and cleanup regressions pass. Full validation: 567 tests, 96% statement coverage, all static/build/pre-commit checks, and a real Islo A→B pickle/upload/exec/destroy proof with zero resources.

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add a provider-neutral sandbox execution integration

3 participants