Conversation
![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: 569266739b
ℹ️ 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".
rhan-oai
force-pushed
the
pr18030
branch
6 times, most recently
from
b32fbad to
b7bd713
Compare
rhan-oai
force-pushed
the
pr18030
branch
9 times, most recently
from
070c807 to
4565f6d
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d7d374b355
ℹ️ 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".
rhan-oai
force-pushed
the
pr18030
branch
2 times, most recently
from
ea9b6e1 to
bf8fb10
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1d923c73d7
ℹ️ 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".
| emit_responses_api_call_attempt( | ||
| sess.as_ref(), | ||
| turn_context.as_ref(), | ||
| responses_api_call_attempt, | ||
| CodexResponsesApiCallStatus::Completed, | ||
| /*error*/ None, |
There was a problem hiding this comment.
run_sampling_request marks every Ok path as Completed, but try_run_sampling_request can break Ok(...) early when mailbox preemption fires before any ResponseEvent::Completed. In that case the stream is locally cut short and response_id/token usage may never be observed, so analytics misclassifies a partial attempt as a successful completed call.
Useful? React with 👍 / 👎.
| emit_responses_api_call_attempt( | ||
| sess.as_ref(), | ||
| turn_context.as_ref(), | ||
| responses_api_call_attempt, | ||
| status, | ||
| error, |
There was a problem hiding this comment.
try_run_sampling_request maps cancellation to TurnAborted before stream setup completes, yet run_sampling_request always emits an Interrupted analytics attempt for that error. This records phantom API attempts when no request was actually sent, inflating per-turn call counts and interruption metrics.
Useful? React with 👍 / 👎.
| let Some(content_items) = output.content_items() else { | ||
| return (None, None); | ||
| }; |
There was a problem hiding this comment.
function_call_output_part_counts returns (None, None) whenever output is the Text variant, even though that payload is one text part. As a result, text-only tool outputs are emitted with null part counts, undercounting modality statistics for Responses API item metadata.
Useful? React with 👍 / 👎.
1 participant
Why
The analytics crate can now represent and reduce Responses API call events, but core still needs to observe actual sampling request lifecycles and submit those observations. This PR wires the event emission into the turn execution path without changing model behavior.
What changed
run_turnnow owns a monotonically increasingturn_responses_call_indexcounter for the turn.run_sampling_requestallocates an index for each actual Responses API attempt, including retry attempts, so emitted analytics can be ordered within the turn.run_sampling_requestcreates aResponsesApiCallAttemptafter prompt construction. That attempt captures the prompt input items that are actually sent to the model, the call start time, completed output items observed from the stream, the Responses APIresponse_idwhen available, and token usage when the stream completes.To avoid cloning and retaining the full prompt for analytics, the attempt stores precomputed item metadata rather than raw
ResponseItems. Input metadata is computed from borrowed prompt items before the API call, and output metadata is appended as each completed output item arrives.try_run_sampling_requestnow receives the attempt state. It records completed output metadata onResponseEvent::OutputItemDone, recordsresponses_idand token usage onResponseEvent::Completed, and otherwise leaves the existing stream handling behavior intact.Terminal paths in
run_sampling_requestnow submit the attempt throughemit_responses_api_call_attempt. Calls that observeresponse.completedemitcompleted; context-window, usage-limit, exhausted-retry, non-retryable failures, and retryable failures before a retry/fallback emitfailed; aborted turns and locally preempted streams emitinterrupted. If cancellation wins before stream setup completes, the attempt is not emitted, avoiding phantom API-call analytics for requests that were never established. The emission remains gated byFeature::GeneralAnalytics.The analytics client also truncates Responses API call errors to 1024 bytes before building the final fact, keeping analytics-specific payload constraints in the analytics crate instead of core.
Verification
Verified with
cargo check -p codex-core. The stack was also exercised withcargo test -p codex-analyticsfor the analytics-side event handling.Stack created with Sapling. Best reviewed with ReviewStack.