Record realtime close marker on replacement#13058
Conversation
|
@codex review this |
![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: fde5bb62aa
ℹ️ 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".
| pub(crate) async fn handle_close(sess: &Arc<Session>, sub_id: String) { | ||
| match sess.conversation.shutdown().await { | ||
| Ok(()) => { | ||
| record_realtime_end_developer_message(sess, &sub_id, "requested").await; |
There was a problem hiding this comment.
handle_close now always records a requested realtime end developer message on any successful shutdown(). But shutdown() returns Ok(()) even when no conversation is running, so duplicate/stray close ops will inject a false "realtime ended" instruction into history and affect later model requests.
Useful? React with 👍 / 👎.
| if let Some(()) = sess_clone.conversation.running_state().await { | ||
| info!("realtime conversation transport closed"); | ||
| record_realtime_end_developer_message(&sess_clone, &sub_id, "transport_closed").await; |
There was a problem hiding this comment.
The spawned event-forwarding task checks only global running_state() before recording transport_closed. During session replacement, an old task can exit after a new session is installed, see running_state == Some, and write a spurious close marker for the replacement path. This races and can add incorrect developer instructions.
Useful? React with 👍 / 👎.
charley-oai
left a comment
charley-oai
left a comment
There was a problem hiding this comment.
Hmm does this realtime conversation state need to be recorded in TurnContext / TurnContextItem so that it can be properly handled by our new compaction/resume/backtracking logic? I think all model visible state should be handled similarly right?
Ensure replaced realtime sessions write the closing developer message and cover it through the mocked responses request.\n\nCo-authored-by: Codex <noreply@openai.com>
Restore the existing replacement runtime test and add a separate request-level integration test using the shared response mock helpers.\n\nCo-authored-by: Codex <noreply@openai.com>
Align the realtime start and end prompts with the intermediary backend-executor framing without naming the backend directly.\n\nCo-authored-by: Codex <noreply@openai.com>
Co-authored-by: Codex <noreply@openai.com>
Co-authored-by: Codex <noreply@openai.com>
aibrahim-oai
force-pushed
the
ibrahim/realtime-replace-close-marker
branch
from
d43d8d2 to
da467b0
Compare
Co-authored-by: Codex <noreply@openai.com>
Co-authored-by: Codex <noreply@openai.com>
|
@codex review this |
Co-authored-by: Codex <noreply@openai.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ea82d5956f
ℹ️ 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".
charley-oai
force-pushed
the
ibrahim/realtime-replace-close-marker
branch
from
75a9f45 to
065e8d1
Compare
Replace previous_model with previous_turn_settings, read turn baselines from session state in initial and steady-state context updates, add remote compaction coverage for realtime transition reinjection, and fix synthetic TurnContextItem initializers for the new realtime_active field. Co-authored-by: Codex <noreply@openai.com>
charley-oai
force-pushed
the
ibrahim/realtime-replace-close-marker
branch
from
065e8d1 to
60b3bc9
Compare
|
@codex review this |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 60b3bc944d
ℹ️ 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".
charley-oai
left a comment
charley-oai
left a comment
There was a problem hiding this comment.
Made some changes to fix edge cases of realtime true -> false (but reference_context_item set to none by compaction), I think this previous_turn_settings object will be useful in the future to have as a scratch space for any useful settings to carry over from the previous user turn (protected from being set to None by compaction, unlike reference_turn_context)
A nice-to-have followup PR would allow model-visible state changes even within a single turn (this is useful both for this realtime setting and e.g. changing permissions mid turn), but that's out of scope for this PR
| @@ -0,0 +1,5 @@ | |||
| Realtime conversation ended. | |||
|
|
|||
| You are still operating behind an intermediary rather than speaking to the user directly. Use the conversation transcript and current context to decide whether backend help is actually needed, and avoid verbose responses that only add latency. | |||
There was a problem hiding this comment.
Hmm is it true that after realtime ends the model is still not speaking to the user directly? Is it impossible to return to a normal codex session after turning on/off realtime mode?
2 participants
Summary