-
Notifications
You must be signed in to change notification settings - Fork 7.4k
Add collaboration_mode override to turns #9408
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
@codex review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 05a57fc43e
ℹ️ 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 collaboration_mode = collaboration_mode.or_else(|| { | ||
| Some(CollaborationMode::Custom(Settings { | ||
| model: model.clone(), | ||
| reasoning_effort: effort, | ||
| developer_instructions: None, | ||
| })) | ||
| }); | ||
| ( | ||
| items, | ||
| SessionSettingsUpdate { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honor collaboration_mode developer instructions
When collaboration_mode is supplied on Op::UserTurn, the update only writes the collaboration_mode field into SessionSettingsUpdate and never propagates collaboration_mode.settings().developer_instructions into SessionConfiguration.developer_instructions, which is what build_initial_context uses to inject developer messages. As a result, clients that set collaboration_mode with developer_instructions (which the new API doc says should take precedence) will silently lose those instructions and get the old developer prompt instead. Consider mapping the collaboration-mode developer instructions into session settings or otherwise applying them to the turn context.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
on purpose. future PRs
| /// EXPERIMENTAL - set a pre-set collaboration mode. | ||
| /// Takes precedence over model, reasoning_effort, and developer instructions if set. | ||
| pub collaboration_mode: Option<CollaborationMode>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❤️
Add optional collaboration_mode to turn start/override paths and assert precedence in integration tests.