Conversation
rhan-oai
force-pushed
the
pr17088
branch
9 times, most recently
from
8dce026 to
845d3dc
Compare
rhan-oai
force-pushed
the
pr17088
branch
3 times, most recently
from
0af69cf to
391565f
Compare
rhan-oai
force-pushed
the
pr17088
branch
2 times, most recently
from
433eaa2 to
9ce54f6
Compare
| let (outgoing_tx, mut outgoing_rx) = mpsc::channel::<OutgoingEnvelope>(channel_capacity); | ||
| let outgoing_message_sender = Arc::new(OutgoingMessageSender::new(outgoing_tx)); | ||
| let auth_manager = | ||
| AuthManager::shared_from_config(args.config.as_ref(), args.enable_codex_api_key_env); |
There was a problem hiding this comment.
don't we have an auth_manager on InProcessStartArgs?
There was a problem hiding this comment.
right now is not the case
codex/codex-rs/app-server/src/in_process.rs
Line 111 in a9c111d
rhan-oai
force-pushed
the
pr17088
branch
3 times, most recently
from
32d2d05 to
840f271
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 98f89b7ea8
ℹ️ 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".
| let auth_manager = Arc::downgrade(&auth_manager); | ||
| tokio::spawn(async move { | ||
| let mut reducer = AnalyticsReducer::default(); | ||
| while let Some(input) = receiver.recv().await { | ||
| let mut events = Vec::new(); | ||
| reducer.ingest(input, &mut events).await; | ||
| let Some(auth_manager) = auth_manager.upgrade() else { | ||
| break; |
There was a problem hiding this comment.
AnalyticsEventsQueue::new downgrades auth_manager to Weak and exits the worker when upgrade fails. Because AnalyticsEventsClient::new no longer retains a strong AuthManager, callers that pass a temporary Arc will silently stop analytics processing after startup. This is a behavioral regression from the previous ownership model and can drop all later events.
Useful? React with 👍 / 👎.
|
Codex Review: Didn't find any major issues. Breezy! ℹ️ 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". |
| } | ||
|
|
||
| #[derive(Debug, Clone, Copy)] | ||
| pub enum AuthManagerRetention { |
There was a problem hiding this comment.
something doesn't feel right here.
rhan-oai
force-pushed
the
pr17088
branch
2 times, most recently
from
7fafa13 to
9bca771
Compare
rhan-oai
force-pushed
the
pr17088
branch
6 times, most recently
from
b771b23 to
9195578
Compare
owenlin0
left a comment
owenlin0
left a comment
There was a problem hiding this comment.
nit: perhaps call codex-rs/app-server/src/analytics_events.rs -> analytics_utils.py?
[followup?] also, now we decode the same JSON payload twice in the happy path: once for analytics, then again in the actual handler. as a followup it'd be nice to see if there's a way we can decode the JSON payload just once (probably not a big deal atm in practice since the client likely doesn't send massive JSON responses - the only use case where that should be possible is dynamic tool calls)
3 participants
Why
Codex analytics needs a typed seam for app-server-originated request/response traffic so future tool-approval analytics can consume those facts without adding bespoke callsite tracking each time. Server responses arrive as JSON-RPC
id + resultpayloads, so analytics has to reconstruct the matching typed response from the original typed request while that request context still exists in app-server.This also puts analytics on the app-server outbound path, which needs to avoid keeping the runtime alive during shutdown. The final ownership fix keeps the normal strong auth-manager retention in analytics and makes the external-auth refresh bridge hold a weak back-reference to
OutgoingMessageSender, breaking the runtime cycle at the bridge boundary instead of exposing retention policy through the analytics client API.What changed
ServerRequestandServerResponseanalytics facts, plusAnalyticsEventsClient::track_server_requestandtrack_server_response.ClientRequestandClientResponseso reducers can distinguish client-to-server traffic from server-to-client traffic.ServerRequest::response_from_result, allowing a stored typed request to decode the matching typed server response from a raw JSON-RPC result payload.AnalyticsEventsClientthroughOutgoingMessageSenderand records targeted server requests, replayed targeted requests, and matching targeted responses with the responding connection id needed for correlation.Weak<OutgoingMessageSender>inExternalAuthRefreshBridgeand upgrading it only when an external-auth refresh is actually requested.Verification
cargo test -p codex-analyticscargo test -p codex-app-server outgoing_message::tests::Follow-up
This PR intentionally stops at ingestion plumbing, so
ServerRequestandServerResponsefacts are still reducer no-ops. Once a follow-up PR adds real downstream analytics output for those facts:app-server/tests/suite/v2/analytics.rsfor the real app-server workflow and captured analytics payload;Stack created with Sapling. Best reviewed with ReviewStack.