current time reminders impl for system clock (2/n)#28824
Merged
Conversation
rka-oai
force-pushed
the
codex/varlatency-reminders
branch
2 times, most recently
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}
Contributor
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".
pakrym-oai
reviewed
Jun 18, 2026
| .expect("test config should allow current-time reminders"); | ||
| config.current_time_reminder = Some(CurrentTimeReminderConfig { | ||
| reminder_interval_model_requests: interval, | ||
| clock_source: CurrentTimeSource::AppServerClient, |
Collaborator
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?; |
Collaborator
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.
pakrym-oai
reviewed
Jun 18, 2026
|
|
||
| match config.clock_source { | ||
| CurrentTimeSource::System => Ok(Some(Arc::new(SystemCurrentTimeProvider))), | ||
| CurrentTimeSource::AppServerClient => external_provider.map(Some).ok_or_else(|| { |
Collaborator
There was a problem hiding this comment.
nit: Probably should be named external
Contributor
Author
There was a problem hiding this comment.
renamed to external everywhere in the stack
pakrym-oai
reviewed
Jun 18, 2026
| state_db: Option<StateDbHandle>, | ||
| installation_id: String, | ||
| attestation_provider: Option<Arc<dyn AttestationProvider>>, | ||
| external_current_time_provider: Option<Arc<dyn CurrentTimeProvider>>, |
Collaborator
There was a problem hiding this comment.
nit: external_time_provider
pakrym-oai
reviewed
Jun 18, 2026
| .services | ||
| .current_time_provider | ||
| .as_ref() | ||
| .ok_or_else(|| CodexErr::Fatal("current-time provider is not configured".to_string()))?; |
Collaborator
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.
pakrym-oai
reviewed
Jun 18, 2026
| .await; | ||
|
|
||
| let mut state = sess.state.lock().await; | ||
| state.current_time_reminder.record_delivery(window_id); |
Collaborator
There was a problem hiding this comment.
should we have 1 method?
pakrym-oai
reviewed
Jun 18, 2026
Comment on lines
+229
to
+236
| 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; | ||
| } |
Collaborator
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
pakrym-oai
approved these changes
Jun 18, 2026
rka-oai
force-pushed
the
codex/varlatency-config
branch
from
7a0e21d to
88be248
Compare
rka-oai
force-pushed
the
codex/varlatency-reminders
branch
3 times, most recently
from
38a6d16 to
d45f58b
Compare
rka-oai
force-pushed
the
codex/varlatency-config
branch
from
5df1f8f to
221ef10
Compare
rka-oai
force-pushed
the
codex/varlatency-reminders
branch
2 times, most recently
from
4d678ec to
f09e32e
Compare
rka-oai
force-pushed
the
codex/varlatency-config
branch
from
3f08ca5 to
98dbb60
Compare
rka-oai
force-pushed
the
codex/varlatency-reminders
branch
from
14e4299 to
0539ddc
Compare
rka-oai
force-pushed
the
codex/varlatency-config
branch
from
98dbb60 to
c8424ce
Compare
rka-oai
force-pushed
the
codex/varlatency-reminders
branch
from
0539ddc to
671d4f5
Compare
rka-oai
force-pushed
the
codex/varlatency-config
branch
from
c8424ce to
88a9dc6
Compare
rka-oai
force-pushed
the
codex/varlatency-reminders
branch
from
671d4f5 to
933b6ab
Compare
rka-oai
force-pushed
the
codex/varlatency-reminders
branch
from
933b6ab to
5396ac2
Compare
rka-oai
force-pushed
the
codex/varlatency-reminders
branch
from
5396ac2 to
ba927fa
Compare
rka-oai
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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 clippy -p codex-core -p codex-app-server -p codex-mcp-server -p codex-thread-manager-samplejust fmt