Repository navigation
Add local execution and supervised worker controls - #5494
markstuart-oai wants to merge 71 commits into
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
Security findingsAdvisory findings (3)
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Approved at d7a076e. No actionable findings from the coordinator review and independent reviews of executor policy, recovery/receipts, and the supervised-worker example.
Reviewed local/remote executor selection, strict write-root replacement and restore checks, network and credential handling, recovery ownership and replay fencing, exact-turn steering, retained execution and instruction receipts, sparse API-key limit signals, API compatibility, and regression coverage. The five added native patch hashes match the manifest.
Static review only: no candidate code, tests, builds, or native tooling executed. Hosted lint/docs and completed language analysis checks passed; Linux, macOS, Windows native jobs and Rust CodeQL are still running. The automated review summary currently references an older commit, so it is not evidence of current-head automated review. Application acceptance and comparative latency remain unproven. Leave unmerged as requested; retain hosted CI follow-up.
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Re-reviewed the four-file update at be0951f with an independent reviewer. No actionable findings. The early sandbox-denial completion is emitted only after the orchestrator finishes approval/retry handling, retains the original output and call context, and does not duplicate the normal Begin/completion path. The deterministic regression exercises exit-before-start acknowledgement without relaxing the timeout; the patch checksum matches the manifest.
Static review only; no local code or tests run. Fresh hosted CI and automated reviews remain pending, so the prior timeout failures are not yet verified resolved by hosted checks. Leave both stacked PRs unmerged as requested.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: be0951f761
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5234d67e57
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 150d0f23b4
ℹ️ 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".
There was a problem hiding this comment.
🛡️ Codex Security Review · Automatically triggered
Here are some automated security review suggestions for this pull request.
Reviewed commit: dbb5812cf0
ℹ️ About Codex security reviews in GitHub
This is an experimental Codex feature. Security reviews are triggered when:
- You comment "@codex security review"
- A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review
Once complete, Codex will leave suggestions, or a comment if no findings are found.
|
@codex review |
|
Codex Review: Didn't find any major issues. Chef's kiss. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
jbeckwith-oai
left a comment
There was a problem hiding this comment.
The shared physical write-root and storage protections are sound: filesystem identities cover alternate paths, checklist aliases and hard links are checked, and private runtime symlinks fail closed. Disjoint storage and write grants remain supported. Both prior worker findings are addressed: verification output is compared exactly, and Unix-only helper cases are guarded on Windows. Acceptance is persisted only after owned resource cleanup succeeds; directory identity is captured once and primary read failures survive cleanup failures. The executor startup fixture removes the parent-selected port race.
Coordinator and independent static reviews of actual old/new file contents found no actionable issue. Hosted CI on this new head remains pending; prior-head results do not qualify it. No candidate code or local tests executed.
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Re-approved this merge-only update: its complete Git tree is identical to the previously reviewed head. No source, configuration, documentation or test content changed. Coordinator and independent static verification; current-head hosted CI is still pending. No candidate code or local tests executed.
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Hosted CI exposed a test portability regression in the new supervised-worker coverage; see the inline finding. Both Windows versions fail the directory-open operation. Windows 3.10 evidence. Windows 3.14 also has a separate 60-second Cargo fixture-build timeout; that alone does not establish a code defect. Coordinator and independent static investigation only; no candidate code, local tests or reruns executed.
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Approved the supported-platform guard. The only incremental change restricts the prior-snapshot publication test to Linux/macOS, matching the example’s directory-fsync requirements and neighboring tests. This resolves the reported Windows PermissionError without removing supported-platform coverage. Coordinator and independent static review found no new issue. Current-head hosted checks: {'SUCCESS': 6, 'IN_PROGRESS': 4, 'NEUTRAL': 1}. Native validation remains pending; the earlier Cargo fixture timeout is separate. No candidate code or local tests executed.
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Re-approved this refreshed commit after verifying its complete Git tree is identical to the previously reviewed commit. No source, test, or metadata changes. Existing feedback was rechecked; no new actionable finding. Coordinator and independent static review. Hosted native checks are running for the new commit; no candidate code or local tests executed.
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Re-reviewed db52bcc. The Linux long-path helper scan safely falls back to component-wise directory opens, rejects descendant symlinks, preserves distinct mount views, and keeps the directory descriptor alive through entry inspection. The patch and manifest propagation are consistent with the previously reviewed stack. No new actionable findings. Hosted CI remains in progress; this was static review only, with no candidate code or local tests executed.
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Re-approved 5997e7db6f43a697a0a43cdab70b541107987b97. The complete Git tree is identical to the previously approved commit: no source, documentation, configuration or test changes. Prior Linux shutdown and deep-path scanner fixes remain intact. No new actionable findings from coordinator and independent static review.
Fresh hosted native CI remains pending. No candidate code, builds or local tests executed. Existing merge holds remain in effect.
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Static re-review: immutable-record publication now retries directory fsync after an identical existing record, so retry success follows durable directory publication. The regression covers two failed directory syncs followed by successful idempotent publication. Other feature changes remain unchanged. Coordinator and independent review found no actionable issues. Hosted CI remains pending; no candidate code or local tests executed.
Review stack: #5504 (CI) > #5499 (turn controls) > #5500 (receipts) > #5501 (executor) > #5502 (recovery) > #5503 (example) > #5495 (docs) > #5509 (automatic compaction during cancellation). Combined reference: #5494.
Requirements
The worker example and integration tests address three new acceptance records:
These tests qualify the reference workflow. Deployment-specific acceptance and comparative latency measurements remain open. REQ731 is excluded. Model access uses API keys, and managed login and billing verification are outside this PR.
Summary
The combined diff matches the worker example layer in #5503. The adoption docs are in #5495, and #5509 adds the automatic-compaction checks. This reference includes child receipts, local write-root isolation, remote policy restoration and worker effect-journal validation. Local isolation covers the runtime home, durable storage and native helpers, including bind mounts in either direction on Linux. Linux and macOS also check whether a write grant contains an existing hard link to an installed helper executable.
This pull request adds controls for supervising API-key coding workers. Applications can steer a specific turn, inspect execution records, and resume a local session after recording an interrupted tool call's outcome.
Implicit remote workspaces can move during portable restore; explicit write policies and configured network rules still must match. The existing
Environment.url: strandTurn.steer(input)contracts remain supported; exact targeting uses the separateTurn.steer_exact()method. Reconciliation requires a supported idle local session; cold descendant recovery remains unsupported.UncertainCall.reconcile()builds a confirmation from the reported call's IDs. Public executor receipt projection omits the raw permission profile. The application owns duplicate prevention across new call IDs and publication. Model authentication remains API-key-only.Native changes extend the patch series without changing the pinned upstream Core revision. The isolated CI prerequisite is #5504. Behavioral docs are in #5495. This PR remains the combined reference; the five feature layers carry the review.
The supervised worker records acceptance after both owned resources close and checks exact verification output, including its final newline.
On macOS, shutdown waits for the starting wrapper to report its kernel process identity or for its control channel to finish. It can then resume a paused wrapper. Success requires both the drain acknowledgment and the wrapper's exit within the shutdown deadline.
After restore, new turns record the instruction files or inline instructions the caller actually supplied. Earlier receipts retain their original source. Detached commands that finish after the turn iterator ends remain observable through the session and its history.
At session opening and when changing write roots, the local executor checks whether a writable path is a hard link to a private SDK storage file. A writable alias can otherwise change saved state through a different filename.
A session saved before its first model request can reopen without a sampling receipt. After sampling, an explicit network policy or strict write grant needs recorded evidence. The SDK checks the owner of surviving modern receipt fields before allowing a legacy reopen. Older implicit remote sessions can reopen when every retained Core context matches the original default read-only or single-workspace policy. If all modern receipt fields were erased, a strict session that recorded the same Core context as an older implicit session can be indistinguishable from it. The caller can reopen that history only under a compatible implicit policy.
A writable root can expose private session history in either direction. It can contain a relocated sessions directory, or it can sit inside one and receive the next rollout. On Linux and macOS, Core compares directory identities and their ancestors so alternate spellings reach the same check. Disjoint storage relocations remain supported. On open, resume, and policy replacement, the SDK also checks the selected rollout file and the files directly inside its private checklist and budget directories. The SDK refuses symbolic links inside its temporary runtime home.
Test plan
The combined PR's Windows 3.10 and 3.14 jobs both failed two artifact tests on the missing Unix flag
O_NOFOLLOW. Those tests now run on the same Linux and macOS platforms supported by the local worker. The platform-independent worker tests still run on Windows.In an unprivileged Linux guest, the installed Python wheel passed 12 storage checks, including direct and relocated SQLite aliases, writable-root replacement and reopen. Sixteen installed Linux durable receipt cases passed, including corrupted identities, strict policies, and compatible legacy reopening. All 165 Rust binding tests passed. Five shared control-channel tests passed after the queued-identity regression failed on the previous source. They exercised the socket protocol with a simulated failed signal on Linux.
The combined reference has the same tree as the split worker source. The integrated follow-up passed 141 focused tests with five platform skips. A separate 27-test recovery run and five raw-input cases passed. The split PRs own the corresponding reviews and qualification.
The installed Linux wheel passed all 12 session-storage tests, including the compressed-rollout case and the disjoint relocation control.
A hosted child-approval test read the command output file while the command was still running. The fixture now polls the approved process until it exits successfully. A forced delay reproduced the failure before the change. Both delayed policy cases and all three ordinary controls passed with the installed wheel.
A sandbox command corrupted a saved checklist through a hard link before the fix. The checklist and budget correction passed seven installed Linux cases. Separately, the old native wheel created a real rollout inside a writable subdirectory on a case-insensitive volume. The final wheel passed all four overlap cases on case-insensitive ext4 and five controls on case-sensitive ext4, including resuming a disjoint rollout.
Issue number
None linked.
Checks
AGENTS.md, including.agents/skills/code-change-verification/scripts/run.shwhen requiredAGENTS.md