Conversation
4d132d9 to
a35e495
Compare
Collaborator
Author
|
Superseded by the split stack starting at #22327. |
bolinfest
added a commit
that referenced
this pull request
May 13, 2026
## Why This is the base PR in the split stack for the permissions migration. It isolates stack-safety work that had been mixed into the larger permissions PR, so reviewers can evaluate the async-future changes separately from the permissions model changes in #22267. The main risk this addresses is large or recursive multi-agent futures overflowing smaller runner stacks. A follow-up review also called out that `shutdown_live_agent` must remain quiescent: callers should not remove a live agent from tracking or release its spawn slot until the worker loop has actually terminated. ## What Changed - Boxes the large async futures in the multi-agent spawn, resume, and close tool handlers. - Boxes the `AgentControl` spawn and recursive close/shutdown paths that can otherwise build very deep futures. - Keeps `shutdown_live_agent` waiting for thread termination before removing/releasing the live agent, preserving the previous shutdown ordering while still boxing the recursive close path. ## Verification Strategy The focused local coverage was `cargo test -p codex-core multi_agents`, which exercises the multi-agent spawn/resume/close handlers, cascade close/resume behavior, and the shutdown path touched by this PR. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/openai/codex/pull/22266). * #22330 * #22329 * #22328 * #22327 * __->__ #22266
agogo233
pushed a commit
to agogo233/codex
that referenced
this pull request
May 13, 2026
## Why This is the base PR in the split stack for the permissions migration. It isolates stack-safety work that had been mixed into the larger permissions PR, so reviewers can evaluate the async-future changes separately from the permissions model changes in openai#22267. The main risk this addresses is large or recursive multi-agent futures overflowing smaller runner stacks. A follow-up review also called out that `shutdown_live_agent` must remain quiescent: callers should not remove a live agent from tracking or release its spawn slot until the worker loop has actually terminated. ## What Changed - Boxes the large async futures in the multi-agent spawn, resume, and close tool handlers. - Boxes the `AgentControl` spawn and recursive close/shutdown paths that can otherwise build very deep futures. - Keeps `shutdown_live_agent` waiting for thread termination before removing/releasing the live agent, preserving the previous shutdown ordering while still boxing the recursive close path. ## Verification Strategy The focused local coverage was `cargo test -p codex-core multi_agents`, which exercises the multi-agent spawn/resume/close handlers, cascade close/resume behavior, and the shutdown path touched by this PR. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/openai/codex/pull/22266). * openai#22330 * openai#22329 * openai#22328 * openai#22327 * __->__ openai#22266
agogo233
pushed a commit
to agogo233/codex
that referenced
this pull request
May 15, 2026
## Why This is the base PR in the split stack for the permissions migration. It isolates stack-safety work that had been mixed into the larger permissions PR, so reviewers can evaluate the async-future changes separately from the permissions model changes in openai#22267. The main risk this addresses is large or recursive multi-agent futures overflowing smaller runner stacks. A follow-up review also called out that `shutdown_live_agent` must remain quiescent: callers should not remove a live agent from tracking or release its spawn slot until the worker loop has actually terminated. ## What Changed - Boxes the large async futures in the multi-agent spawn, resume, and close tool handlers. - Boxes the `AgentControl` spawn and recursive close/shutdown paths that can otherwise build very deep futures. - Keeps `shutdown_live_agent` waiting for thread termination before removing/releasing the live agent, preserving the previous shutdown ordering while still boxing the recursive close path. ## Verification Strategy The focused local coverage was `cargo test -p codex-core multi_agents`, which exercises the multi-agent spawn/resume/close handlers, cascade close/resume behavior, and the shutdown path touched by this PR. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/openai/codex/pull/22266). * openai#22330 * openai#22329 * openai#22328 * openai#22327 * __->__ openai#22266
agogo233
pushed a commit
to agogo233/codex
that referenced
this pull request
May 15, 2026
## Why This is the base PR in the split stack for the permissions migration. It isolates stack-safety work that had been mixed into the larger permissions PR, so reviewers can evaluate the async-future changes separately from the permissions model changes in openai#22267. The main risk this addresses is large or recursive multi-agent futures overflowing smaller runner stacks. A follow-up review also called out that `shutdown_live_agent` must remain quiescent: callers should not remove a live agent from tracking or release its spawn slot until the worker loop has actually terminated. ## What Changed - Boxes the large async futures in the multi-agent spawn, resume, and close tool handlers. - Boxes the `AgentControl` spawn and recursive close/shutdown paths that can otherwise build very deep futures. - Keeps `shutdown_live_agent` waiting for thread termination before removing/releasing the live agent, preserving the previous shutdown ordering while still boxing the recursive close path. ## Verification Strategy The focused local coverage was `cargo test -p codex-core multi_agents`, which exercises the multi-agent spawn/resume/close handlers, cascade close/resume behavior, and the shutdown path touched by this PR. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/openai/codex/pull/22266). * openai#22330 * openai#22329 * openai#22328 * openai#22327 * __->__ openai#22266
dkropachev
pushed a commit
to dkropachev/codex
that referenced
this pull request
Jun 9, 2026
## Why This is the base PR in the split stack for the permissions migration. It isolates stack-safety work that had been mixed into the larger permissions PR, so reviewers can evaluate the async-future changes separately from the permissions model changes in openai#22267. The main risk this addresses is large or recursive multi-agent futures overflowing smaller runner stacks. A follow-up review also called out that `shutdown_live_agent` must remain quiescent: callers should not remove a live agent from tracking or release its spawn slot until the worker loop has actually terminated. ## What Changed - Boxes the large async futures in the multi-agent spawn, resume, and close tool handlers. - Boxes the `AgentControl` spawn and recursive close/shutdown paths that can otherwise build very deep futures. - Keeps `shutdown_live_agent` waiting for thread termination before removing/releasing the live agent, preserving the previous shutdown ordering while still boxing the recursive close path. ## Verification Strategy The focused local coverage was `cargo test -p codex-core multi_agents`, which exercises the multi-agent spawn/resume/close handlers, cascade close/resume behavior, and the shutdown path touched by this PR. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/openai/codex/pull/22266). * openai#22330 * openai#22329 * openai#22328 * openai#22327 * __->__ openai#22266
AIALRA-0
pushed a commit
to AIALRA-0/codex-turn-engine
that referenced
this pull request
Jun 10, 2026
## Why This is the base PR in the split stack for the permissions migration. It isolates stack-safety work that had been mixed into the larger permissions PR, so reviewers can evaluate the async-future changes separately from the permissions model changes in openai#22267. The main risk this addresses is large or recursive multi-agent futures overflowing smaller runner stacks. A follow-up review also called out that `shutdown_live_agent` must remain quiescent: callers should not remove a live agent from tracking or release its spawn slot until the worker loop has actually terminated. ## What Changed - Boxes the large async futures in the multi-agent spawn, resume, and close tool handlers. - Boxes the `AgentControl` spawn and recursive close/shutdown paths that can otherwise build very deep futures. - Keeps `shutdown_live_agent` waiting for thread termination before removing/releasing the live agent, preserving the previous shutdown ordering while still boxing the recursive close path. ## Verification Strategy The focused local coverage was `cargo test -p codex-core multi_agents`, which exercises the multi-agent spawn/resume/close handlers, cascade close/resume behavior, and the shutdown path touched by this PR. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/openai/codex/pull/22266). * openai#22330 * openai#22329 * openai#22328 * openai#22327 * __->__ openai#22266
AIALRA-0
pushed a commit
to AIALRA-0/codex-turn-engine
that referenced
this pull request
Jun 10, 2026
## Why This is the base PR in the split stack for the permissions migration. It isolates stack-safety work that had been mixed into the larger permissions PR, so reviewers can evaluate the async-future changes separately from the permissions model changes in openai#22267. The main risk this addresses is large or recursive multi-agent futures overflowing smaller runner stacks. A follow-up review also called out that `shutdown_live_agent` must remain quiescent: callers should not remove a live agent from tracking or release its spawn slot until the worker loop has actually terminated. ## What Changed - Boxes the large async futures in the multi-agent spawn, resume, and close tool handlers. - Boxes the `AgentControl` spawn and recursive close/shutdown paths that can otherwise build very deep futures. - Keeps `shutdown_live_agent` waiting for thread termination before removing/releasing the live agent, preserving the previous shutdown ordering while still boxing the recursive close path. ## Verification Strategy The focused local coverage was `cargo test -p codex-core multi_agents`, which exercises the multi-agent spawn/resume/close handlers, cascade close/resume behavior, and the shutdown path touched by this PR. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/openai/codex/pull/22266). * openai#22330 * openai#22329 * openai#22328 * openai#22327 * __->__ openai#22266
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
This is the permissions migration PR split out from the original large #21250 change. The goal is to finish moving app-server and runtime behavior away from treating
SandboxPolicy::WorkspaceWriteas the owner of writable workspace roots.The invariant after this PR is:
PermissionProfilebodies on thread lifecycle or turn APIs;permissions, and the server resolves and applies that named profile;workspaceRootsindependently, andcwdis only a workspace root when it is explicitly present in that list;SandboxPolicy::WorkspaceWriteno longer serializes its ownwritableRoots; old clients may still send the legacy field and the server maps it through compatibility logic.What Changed
ActivePermissionProfileModification/ profile modification overlay behavior and normalizes code through server-ownedPermissionProfilevalues.PermissionProfilevalues fromthread/start,thread/resume, andthread/forkresponses; clients now useactivePermissionProfile,workspaceRoots, and the compatibilitysandboxsummary.sandboxPolicy/sandboxrequests map to an existing named profile, including updating workspace roots when a legacy workspace-write request implies a new cwd root.workspaceRootsplus the legacy sandbox projection./statusto show effective user workspace roots, while hiding internal Codex roots such asCODEX_HOME/memories.Memory Compatibility
Memory writes remain supported without making
CODEX_HOME/memoriesa user workspace root. The runtime materializes the memory directory as an internal write grant on the effective profile, while the thread explicitworkspaceRootsremains the user-visible list.That separation is deliberate:
workspaceRoots;/statusand sandbox summaries filter the internal memories root so users do not see it as an added workspace directory;Relevant coverage includes
core/src/config/config_tests.rsfor memory-root preservation under explicit workspace roots,tui/src/status/tests.rsfor hiding the internal memory root in/status,utils/sandbox-summary/src/sandbox_summary.rstests for hidden writable roots, and rollout reconstruction coverage incore/src/session/rollout_reconstruction_tests.rs.Verification Strategy
I focused local verification on the areas most likely to regress:
codex-app-server-protocoltests cover generated schema fixtures, missing optional lifecycle fields, profile-id string selection, and legacySandboxPolicydeserialization.codex-app-servercrate passed earlier, and focused resume/turn tests cover named profile selection, workspace-root updates, and legacy sandbox compatibility.codex-tui app_server_session, focused/statuscoverage, and thecodex-execcrate cover reconstructing session permissions fromworkspaceRootsand compatibility sandbox responses after rawPermissionProfilewas removed from lifecycle responses.just fixcover the new privatePermissionsfields and accessor/mutator callsites, including the path that clears stale active-profile ids when requirements normalize the effective profile.codex-windows-sandboxtests cover the crate that failed Windows CI after the rebase.Stack created with Sapling. Best reviewed with ReviewStack.