Support thread-level originator overrides#29477
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 877c56ccf4
ℹ️ 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".
66a650c to
4cf080c
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4cf080c27e
ℹ️ 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".
4cf080c to
54bf697
Compare
54bf697 to
751c3f9
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 751c3f93ef
ℹ️ 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".
| if service_name.eq_ignore_ascii_case("CODEX_WORK_LOCAL") { | ||
| return Some("codex_work_desktop_local".to_string()); | ||
| } | ||
| if service_name.eq_ignore_ascii_case("CODEX_WORK_CLOUD") { | ||
| return Some("codex_work_desktop_cloud".to_string()); |
There was a problem hiding this comment.
Document reserved serviceName originator mappings
When an app-server thread/start request uses serviceName of CODEX_WORK_LOCAL or CODEX_WORK_CLOUD, this branch now changes the persisted Responses originator, but the public app-server README still describes serviceName only as an optional metrics tag. Update the app-server docs/schema comments so clients know these two values have reserved originator semantics rather than being ordinary metric labels.
AGENTS.md reference: AGENTS.md:L288-L290
Useful? React with 👍 / 👎.
| provider: SharedModelProvider, | ||
| auth_env_telemetry: AuthEnvTelemetry, | ||
| session_source: SessionSource, | ||
| originator: String, |
There was a problem hiding this comment.
Route thread originator through realtime requests
When a Desktop Work thread starts a realtime conversation, this new session-scoped originator is never applied to the realtime startup paths: prepare_realtime_start builds realtime headers without an originator, and both websocket and WebRTC sideband setup fall back to default_headers(), so those model requests still use the process default originator instead of the persisted Work originator. If Work threads can use realtime, add the thread originator to the realtime extra headers just like the Responses/compaction paths.
Useful? React with 👍 / 👎.
…t-originator # Conflicts: # codex-rs/core/src/client_tests.rs
jif-oai
left a comment
There was a problem hiding this comment.
Fix the cargo shear please
And I see a bit of redundance in the tests.. can we prefer one (or a few) strong tests that covers more widely the code? (and do not test what is trivial btw)
|
|
||
| fn originator_from_service_name(service_name: Option<&str>) -> Option<String> { | ||
| let service_name = service_name?.trim(); | ||
| if service_name.eq_ignore_ascii_case("codex_work_desktop") { |
There was a problem hiding this comment.
why? This sounds super ad-hoc... what about CODEX_WORK_LOCAL/CODEX_WORK_CLOUD you discuss in the PR?
There was a problem hiding this comment.
Previously, I recall being able to start a cloud task from Work in the desktop app, but in the latest nightly build, I can only start a local task. So I renamed it to codex_work_desktop to differentiate it from codex_desktop, and Work on web.
| self.state.beta_features_header.as_deref(), | ||
| turn_state.as_ref(), | ||
| )); | ||
| add_originator_header(&mut extra_headers, self.state.originator.as_str()); |
There was a problem hiding this comment.
Can we apply the thread originator to summarize_memories() too? It is the remaining ModelClient request path that falls back to the process-global originator, so Work-thread memory summaries are attributed differently from the other model requests
| .await | ||
| .expect("child thread should be registered"); | ||
| let child_snapshot = child_thread.config_snapshot().await; | ||
| assert_eq!(child_snapshot.originator, parent_snapshot.originator); |
There was a problem hiding this comment.
These tests stop at config_snapshot()...
Why
Work(TPP) threads can be launched from the Desktop app, but if they all keep the Desktop app's default originator then downstream attribution cannot distinguish local Work launches from cloud-backed Work launches.
thread/start.serviceNamealready carries that launch signal, whileSessionMeta.originatoris the durable thread-level value that survives resume and fork.This change converts the Desktop Work service names into an effective originator at thread creation time, persists that originator with the thread, and keeps using it for later model requests and memory writes.
What changed
CODEX_WORK_LOCALandCODEX_WORK_CLOUDservice names to per-thread originators, while preservingCODEX_INTERNAL_ORIGINATOR_OVERRIDEas the highest-precedence override.SessionMeta.originator, read it back on resume/fork, and inherit the parent originator for subagent spawns when there is no persisted session metadata.SpawnAgentForkMode::LastNTurnsforks by falling back to the live parent originator when the forked history no longer includesSessionMeta.Verification
just test -p codex-core agent::control::tests::spawn_thread_subagent_inherits_parent_originator_without_fork agent::control::tests::spawn_thread_subagent_fork_last_n_turns_inherits_parent_originator_without_session_meta thread_manager::tests::originator_override_precedes_service_name_remappingjust test -p codex-core agent::control::tests::resume_thread_subagent_restores_stored_metadata_and_effective_multi_agent_modejust test -p codex-memories-writejust fix -p codex-core -p codex-memories-writegit diff --check