current time reminders impl for system clock (varlatency 2/n)#28824
current time reminders impl for system clock (varlatency 2/n)#28824rka-oai wants to merge 5 commits into
Conversation
rka-oai
force-pushed
the
codex/varlatency-reminders
branch
from
3110411 to
eccf43d
Compare
rka-oai
force-pushed
the
codex/varlatency-reminders
branch
from
eccf43d to
05da7e6
Compare
rka-oai
changed the title
rka-oai
changed the title
![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: e8c21f1b20
ℹ️ 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".
| .expect("test config should allow current-time reminders"); | ||
| config.current_time_reminder = Some(CurrentTimeReminderConfig { | ||
| reminder_interval_model_requests: interval, | ||
| clock_source: CurrentTimeSource::AppServerClient, |
There was a problem hiding this comment.
[found by codex] These tests only exercise AppServerClient; please add coverage for the production System source.
| .build(&server) | ||
| .await?; | ||
|
|
||
| test.submit_turn("first turn").await?; |
There was a problem hiding this comment.
[found by codex] This proves cadence across turns, not model requests; please cover a tool-call follow-up within one turn.
|
|
||
| match config.clock_source { | ||
| CurrentTimeSource::System => Ok(Some(Arc::new(SystemCurrentTimeProvider))), | ||
| CurrentTimeSource::AppServerClient => external_provider.map(Some).ok_or_else(|| { |
There was a problem hiding this comment.
nit: Probably should be named external
| state_db: Option<StateDbHandle>, | ||
| installation_id: String, | ||
| attestation_provider: Option<Arc<dyn AttestationProvider>>, | ||
| external_current_time_provider: Option<Arc<dyn CurrentTimeProvider>>, |
There was a problem hiding this comment.
nit: external_time_provider
| .services | ||
| .current_time_provider | ||
| .as_ref() | ||
| .ok_or_else(|| CodexErr::Fatal("current-time provider is not configured".to_string()))?; |
There was a problem hiding this comment.
it's a lil strange that we have 2 sources of truth for how this feature is enabled. Pick one.
| .await; | ||
|
|
||
| let mut state = sess.state.lock().await; | ||
| state.current_time_reminder.record_delivery(window_id); |
There was a problem hiding this comment.
should we have 1 method?
| info!("Turn error: {err:#}"); | ||
| sess.emit_turn_error_lifecycle(turn_context.as_ref(), err.to_codex_protocol_error()) | ||
| .await; | ||
| sess.track_turn_codex_error(turn_context.as_ref(), &err); | ||
| let event = EventMsg::Error(err.to_error_event(/*message_prefix*/ None)); | ||
| sess.send_event(&turn_context, event).await; | ||
| return None; | ||
| } |
There was a problem hiding this comment.
I'm not in love with this special pile of error handling. Can we have all error handling localized? maybe by reordering things
2 participants
Stacked on #28822.
Summary
This does NOT include the app server client <-> server clock logic. This PR is only for the reminder message & system clock that will be used in prod.
Testing
just test -p codex-core varlatency_just clippy -p codex-core -p codex-app-server -p codex-mcp-server -p codex-thread-manager-samplejust fmt