Skip to content

fix: Use cryptographic randomness for sandbox PTY IDs - #1929

Closed
jbeckwith-oai wants to merge 1 commit into
mainfrom
codex/fix-pty-process-randomness
Closed

jbeckwith-oai wants to merge 1 commit into
mainfrom
codex/fix-pty-process-randomness

Conversation

@jbeckwith-oai

Copy link
Copy Markdown
Contributor

Summary

Use Web Crypto with unbiased range mapping for sandbox PTY process IDs, preserving the existing numeric range and collision handling. If allocation fails, retain existing registry entries and await cleanup of newly created E2B and Daytona PTYs before returning the original error.

Includes focused regression coverage and a patch changeset for @openai/agents-extensions. The affected boundary is PTY registration and provider resource cleanup; maintainer security review is requested.

Test plan

  • Full repository verification wrapper passed: frozen install, build, workspace type checks, declaration checks, lint, formatting, and all 6,936 tests across 227 files.
  • Docker tests passed with local Colima and a shared temporary directory.
  • Focused PTY and provider suite: 466 tests passed across six files.
  • Two consecutive clean independent adversarial review rounds, covering correctness, security, failure cleanup, compatibility, and scope.
  • Changeset validation and git diff --check passed.

Checks

  • Added focused regression tests
  • Ran the full code-change verification wrapper; all steps passed
  • Completed independent review before submission
  • Added and validated a patch changeset
  • Documentation: no public API or usage changes
  • Examples and integration runners: not run separately; no examples or integration behavior changed

@jbeckwith-oai
jbeckwith-oai requested a review from a team as a code owner September 14, 2026 19:21
@changeset-bot

changeset-bot Bot commented Sep 14, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 9a3c47e

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 5 packages
Name Type
@openai/agents-extensions Patch
@openai/agents-core Patch
@openai/agents-openai Patch
@openai/agents-realtime Patch
@openai/agents Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 14, 2026

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review Completed 2026-09-14T19:23:42.028079Z 9a3c47e PR opened
🔒 Security Review Completed 2026-09-14T19:24:38.117118Z 9a3c47e PR opened
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@markstuart-oai markstuart-oai 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.

Need status: plausible but unproven. I left an inline request for the supported outcome or contract that requires unpredictable session-local PTY routing IDs. The existing collision-checked registry already provides routing and separation; the evidence does not yet justify expanding its allocation and provider-failure behavior. I am withholding approval pending that rationale, without claiming a demonstrated security vulnerability.

The implementation review found no introduced correctness or cleanup-ownership defect. Range mapping is unbiased, allocation precedes pruning, and failure cleanup captures only the new attempt's handle or socket. I traced E2B, Daytona, Blaxel and Cloudflare, including cleanup overlapping another registration or session teardown. Node-only randomness and UUIDs would be worse fits for the shared runtime boundary and numeric contract; retaining the current implementation remains the strongest alternative until the need is established.

All 12 hosted checks passed at this head. I inspected the Node 24 job: 6,936 tests across 227 files passed, including the changed PTY/provider suites, with build/type/lint checks. The patch changeset is present and its hosted validation passed. This was a desk review with independent review and existing hosted evidence; no local tests or live sandbox operations were run.

Comment thread packages/agents-extensions/src/sandbox/shared/pty.ts
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants