Type parent item ids for network approval reviews#15469
Type parent item ids for network approval reviews#15469charley-oai wants to merge 12 commits intomainfrom
Conversation
Co-authored-by: Codex <noreply@openai.com>
Co-authored-by: Codex <noreply@openai.com>
Co-authored-by: Codex <noreply@openai.com>
Co-authored-by: Codex <noreply@openai.com>
Do not collapse legacy network approval review notifications onto the parent tool item id. Co-authored-by: Codex <noreply@openai.com>
Thread a typed parent tool item id through guardian assessment events and legacy app-server guardian review notifications for network approvals. Co-authored-by: Codex <noreply@openai.com>
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep it up! ℹ️ 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". |
Keep `targetItemId` as a deprecated compatibility alias while emitting the clearer `reviewId` field on legacy guardian approval review notifications.\n\nCo-authored-by: Codex <noreply@openai.com>
Populate parent tool item ids for direct guardian-reviewed tool actions while preserving distinct network approval review ids. Co-authored-by: Codex <noreply@openai.com>
|
@codex review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. ℹ️ 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". |
Co-authored-by: Codex <noreply@openai.com>
| let args = json!({ | ||
| "command": script, | ||
| "timeout_ms": 1_000, | ||
| "timeout_ms": 3_000, |
There was a problem hiding this comment.
CI was flaking on me but I can pull this into a separate PR if you prefer
Thread a stable network owner id through managed proxy credentials and use it to resolve the exact active tool call for blocked network requests. This makes concurrent network approvals attribute deterministically and preserves parent tool item ids downstream. Co-authored-by: Codex <noreply@openai.com>
Remove the synthetic network approval token and carry parent_tool_item_id directly through the managed proxy attribution path. Co-authored-by: Codex <noreply@openai.com>
|
@codex review |
Bundle proxy-disabled response parameters into a single typed args struct so the callsites stay readable and clippy stops flagging the helper for too many arguments. Co-authored-by: Codex <noreply@openai.com>
![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: da3dc36b63
ℹ️ 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".
| .record_blocked(BlockedRequest::new(BlockedRequestArgs { | ||
| host: host.clone(), | ||
| reason: REASON_MITM_REQUIRED.to_string(), | ||
| parent_tool_item_id: None, |
There was a problem hiding this comment.
parent_tool_item_id is extracted earlier in http_connect_accept but dropped here (None). In limited mode without MITM, blocked CONNECT requests become unattributed. With concurrent active tool calls, core cannot map the denial back to the originating call, so the right tool call may not receive the expected policy-rejection outcome.
Useful? React with 👍 / 👎.
| .record_blocked(BlockedRequest::new(BlockedRequestArgs { | ||
| host: policy.target_host.clone(), | ||
| reason: reason.to_string(), | ||
| parent_tool_item_id: None, |
There was a problem hiding this comment.
MITM-time blocks are recorded with parent_tool_item_id: None, so method/local-address denials after CONNECT lose ownership attribution. In multi-call concurrency, these denials cannot be reliably tied to the originating tool item, which can drop or misroute network-approval outcomes.
Useful? React with 👍 / 👎.
|
Superseded by the split stack:
|
1 participant
Summary
reviewIdandparentToolItemIdfields to the legacy app-server guardian review notificationsparent_tool_item_idthroughGuardianAssessmentEventso app-server clients do not need to recover parent linkage from untypedactionJSONparentToolItemIdfor direct guardian-reviewed tool actions, where it matchesreviewIdreviewIddistinct fromparentToolItemIdfor attributednetwork_accessreviews, so concurrent network reviews stay uniquely identifiabletargetItemIdas a deprecated alias forreviewIdfor backwards compatibilityNotes
parentToolItemIdcan still be absent when anetwork_accessreview cannot be attributed to a parent tool item.Supersedes #15439.