Repository navigation
feat: edit detached native sampling requests - #5518
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ 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. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bb800c9163
ℹ️ 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".
jbeckwith-oai
left a comment
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Changes requested for the independently validated existing finding #5518 (comment) . Both entry points validate the entire opening Agent tool roster before resolving per-operation selection. An omitted deferred or hosted declaration therefore rejects an otherwise supported eager-only or empty request. Validate the effective selected roster after resolving operation defaults while retaining the global environment/transport restrictions. No duplicate inline comment added.
The operation/turn/store ownership, cancellation joining, immutable capture, dispatch revalidation and canonical-history isolation otherwise look sound. Ordinary runs borrow the prompt and pending manual resumes reject a newly supplied callback. Hosted native builds remain pending with no observed failures. Static review only; no candidate code or local tests ran.
|
@codex review |
jbeckwith-oai
left a comment
jbeckwith-oai
left a comment
There was a problem hiding this comment.
The selected-roster validation fixes the prior finding: excluded deferred/hosted tools no longer reject supported operations, dynamic rosters retain native validation before editor/provider dispatch, and fixed hosted registrations remain excluded. Selected manual tools require a handler for edited one-shot runs. No new findings. Hosted native CI is pending. Coordinator and independent static review; no candidate code or local tests executed.
🛡️ 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. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5d4eb2f366
ℹ️ 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".
|
@codex review |
🛡️ 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. |
|
Codex Review: Didn't find any major issues. Nice work! 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". |
jbeckwith-oai
left a comment
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Approved at d26ada5. Combined forwarding preserves parent_trace and prepare_request, captured workflow policy, restored/manual continuations, compaction and output repair. Integrity-reason handling and callback admission fixes remain intact. All four stack heads contain the exact reviewed W3C security patch from 5451026; the inherited tracing blocker is addressed here. Final ancestry commits do not change the reviewed trees.
Hosted checks: 12 SUCCESS, 8 SKIPPED. Windows, typing and macOS sandbox checks were skipped, not passed. Coordinator and independent static review found no actionable integration regressions. No candidate code or local tests executed; no CI reruns requested.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 41d1f49209
ℹ️ 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".
jbeckwith-oai
left a comment
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Request changes at 41d1f49. Coordinator and independent static review confirm the existing P2 delegated-provider finding; no duplicate inline finding added. Agent.run checks the agent/default provider before invoking an overridden Agent.session factory. With no agent-level provider, it treats the provider as WebSocket-enabled and rejects prepare_request even when the override supplies a supported HTTP NativeProvider. Defer this provider validation to the opened session (Session.submit already checks its actual transport), or limit the preliminary check to the default factory. Add async/sync regression coverage for an override supplying HTTP and a control rejecting an actual WebSocket session.
All 43 files changed by the merge match the landed base exactly; all 19 feature-specific files are unchanged from the previous reviewed head. This is a newly confirmed existing feature issue, not a regression introduced by the merge. Hosted CI remains pending. No candidate code, local tests, builds or CI reruns executed.
jbeckwith-oai
left a comment
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Approved at af77762. The delegated-provider finding is addressed: preliminary output/profile validation now applies only to the default session factory. Delegated sessions still validate their actual transport, output and environment in Session.submit. Four regressions cover async/sync HTTP success and actual WebSocket rejection before editor/provider work. The three CodeQL cache-removal files match the landed base exactly.
Coordinator and independent static review found no actionable issues. This supersedes my prior request-changes decision. Hosted CI remains pending. No candidate code, local tests, builds or CI reruns executed.
jbeckwith-oai
left a comment
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Approved at 73498b9. The three-line guard skips only post-turn compaction when request preparation is active, preserving the completed answer. Pinned upstream and patch 0018 confirm the previous Fatal propagation from this caller; pre/mid-turn rejection and general error handling remain intact. The editor and ordinary control regressions check final output, one versus two provider requests, and callback indices/count. Patch checksum matches the manifest; delegated-provider fix is unchanged.
Coordinator and independent static review found no actionable issues. Hosted CI remains pending. No candidate code, local tests, builds or CI reruns executed.
Summary
Add
prepare_requesttoAgent.run,Agent.run_sync, andSession.submit. The callback edits a detached sampling copy of instructions, input, tool descriptions, and JSON schemas. Canonical history, execution handlers, argument validators, and approval policies keep their existing owners.The supported profile uses direct root Responses HTTP requests with eager ordinary function and MCP tools. Admission checks the actual opened Session, including delegated async/sync
Agent.sessionoverrides. Each new sampling request invokes the callback; HTTP retries reuse its frozen edit. The callback retains the submitting workflow's trace identity and privacy restrictions. Default calls preserve existing method overrides.Pre-turn and mid-turn automatic compaction reject before hidden model dispatch. Post-turn compaction skips that hidden request and retains the completed answer. Active editor state cannot be exported; portable restore requires a fresh callback. Dynamic
before_stepselection controls whether an unhandled manual tool needs an interaction handler.Test plan
Current head
73498b9215includes the actual targeta1b704ad. Normal source preparation verifies the complete native recipe at Core4f78f452. A new wheel and fixture passed 154 installed feature checks, 68 tracing checks, and 3 archive checks with zero skips. All 50 installed SDK/native files match the wheel. The focused 10-case matrix covers delegated HTTP/WebSocket admission and pre/mid/post-turn compaction.The old native build reproduced the post-turn bug: normal compaction retained the answer, while an editor raised
Fatalafter completion. The corrected build passes both cases. Two fresh independent source reviews passed on the current head.Required formatting, lint, type checks, and the full SDK gate passed: 4,445 parallel tests passed with 176 skips, followed by 41 serial passes. The run used eight workers. Source fingerprints and all installed SDK/native files stayed unchanged before and after verification. All 29 current hosted checks are terminal green or skipped.
Issue number
REQ106
Checks