refactor: share Codex session execution - #917
mldangelo-oai wants to merge 19 commits into
Conversation
|
@codex review Please review exact head |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
Security findingsBlocking findings (1)
ℹ️ 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. |
|
Codex Review: Didn't find any major issues. 👍 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 review Please review exact head |
|
Codex Review: Didn't find any major issues. 🚀 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". |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
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: 0b43c3f474
ℹ️ 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.
| "model_reasoning_effort", | ||
| "model_reasoning_summary", | ||
| "service_tier", | ||
| "model_providers", |
There was a problem hiding this comment.
🛡️ Codex Security Review · Automatically triggered
When codexOverrides.model_providers contains an embedded credential (including in an unrelated provider), this loop copies the entire table into <CODEX_SECURITY_CONFIG_PATH>.workers.toml. Every scan child receives the base path, and its filesystem profile grants root read, so repository-controlled instructions can read the predictable sibling and return the credential in a model request. This bypasses the existing sanitized preflight projection. Store the worker snapshot outside scan-readable paths or project only the selected provider without exposing secrets.
SECURITY.md reference: SECURITY.md:L94-L100
Dismiss this finding: Reply with @codex security dismiss <reason> [context]. Codex will resolve this conversation automatically; GitHub may require a page refresh to show the result.
Valid reasons: false-positive, duplicate, out-of-scope, compensating-control, risk-accepted, or other. Example: @codex security dismiss duplicate Already flagged by another review
What each reason means
false-positive— Not a vulnerabilityduplicate— Already tracked elsewhereout-of-scope— Outside this review's scopecompensating-control— Mitigated by another controlrisk-accepted— Risk intentionally acceptedother— Another reason; context required
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2877c44477
ℹ️ 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".
| isObject(providers) && | ||
| Object.hasOwn(providers, selected) | ||
| ) { | ||
| result["model_providers"] = { [selected]: providers[selected]! }; |
There was a problem hiding this comment.
When the selected custom provider contains literal credentials such as experimental_bearer_token or an authorization value in http_headers, this assignment preserves them verbatim in the predictable <CODEX_SECURITY_CONFIG_PATH>.workers.toml file. The file is written before both Standard and Deep parent sessions start, and their root-read sandbox plus the exported config path lets repository-controlled instructions read the credential and return it in a model request or scan artifact. Fresh evidence in this head is the new config.test.ts case explicitly asserting that these selected-provider secret fields survive, even though unrelated providers are now removed. Project only non-secret settings, or explicitly deny the snapshot path to scan processes.
AGENTS.md reference: sdk/typescript/AGENTS.md:L8-L12
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b80972ffb3
ℹ️ 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".
| }; | ||
| const { model_providers: _providers, ...sdkModelConfig } = modelConfig; | ||
| const configOverrides = [ | ||
| ...modelProviderConfigOverride(modelConfig as JsonObject), |
There was a problem hiding this comment.
When the selected custom provider contains a literal experimental_bearer_token or authorization header, modelProviderConfigOverride serializes the full provider definition into the Codex worker's command-line arguments. On Linux, repository-controlled worker tools running with the configured root-read profile can inspect the ancestor command line through /proc or process-listing tools and return the credential in a model request. Although this head now denies access to the snapshot file, the new provider tests confirm these secret fields still survive into configOverrides, leaving this separate disclosure path; pass secret material through a mechanism unavailable to sandboxed tools rather than argv.
AGENTS.md reference: sdk/typescript/AGENTS.md:L8-L12
Useful? React with 👍 / 👎.
1 participant
Summary
Standard scans and Deep workers duplicate streamed-turn handling. Share that handling while retaining their execution and result contracts. The latest change makes the reconstructed-worker test wait for its child processes to close before removing their temporary executable.
Changes
Testing
b80972f..a82da86; the reports alone are structural evidence. Separate reviewer-eligibility and source verification are now complete, and the finalized receipt passed its exact-source check with zero candidates. This satisfies only the cleanup-increment source-review criterion. Each reviewer’s fixture command failed on missingesbuildbefore assertions, providing no test-pass or native-enforcement credit.a82da86composed with main6cb0d51: 45 direct dependencies matched that composition’s locks; the portable checks, SDK/MCP types and formatting, complete executor fixture, unchanged Bun wrapper and six existing pure timeout-helper tests passed. These results remain bound to that earlier composition.b9fa40dhad no conflicts and passed all 44 file-inventory tests. Earlier checks remain bound to their tested composition; they were not rerun as whole-suite results.a82da86composed with main32c8645(uncommitted tree3cbbfb1) passed nine checks: both Ruff checks, SDKbuild:ci, the source-compatibility checker and its nine tests, SDK/MCP types, formatting, the complete synthetic executor fixture and its unchanged Bun shutdown wrapper (one test). Frozen SDK/MCP installs matched all 45 direct dependency versions to the composition’s locks; all 688 tracked file hashes stayed unchanged. This qualifies those source/worker checks on that composition; it supplies no Windows, full-SDK, package-smoke, evaluation or native-enforcement result.b80972fhas a Windows CI fixture failure: 405 passed, three skipped and one failed; two coverage aggregates failed afterward. Its package job completed, but the production audit command exited 1 under continue-on-error. These failures are retained; the new test change still needs Windows CI.2877c44..b80972fwith main8fb0cde; this disclosure review did not rerun them. Historical full SDK orders at2877c44recorded 3,085 passed / 50 skipped / 1 pre-existing Git-fixture failure each; these are historical evidence, not final-head full-suite passes.Risk and rollout
This internal change adds no command, public setting, result field, dependency, migration or scheduler policy. Standard and Deep retain their intentional permission differences and follow-up behavior. Older SDKs without worker snapshots retain their inherited-home behavior.
The selected-provider snapshot finding and the later provider-argument review concern remain open. The security review merge gate is failing, and the earlier outdated review thread is unresolved. This update is not merge clearance. Acceptance of the final composition, provider-isolation compatibility, final integrated SDK/package checks and supported-platform qualification remain open. Linux lifecycle controls do not establish Windows file-lock behavior or native model-enforcement equivalence.
Public disclosure review