-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Cleanup collaboration mode variants #10404
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 this |
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: 675868a725
ℹ️ 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".
675868a to
d298a6d
Compare
|
@codex review this |
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: d298a6d112
ℹ️ 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".
|
@codex review this |
|
@codex review this |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
| ], | ||
| "default": null, | ||
| "description": "Start the TUI in the specified collaboration mode (plan/execute/etc.). Defaults to unset." | ||
| "description": "Start the TUI in the specified collaboration mode (plan/default). Defaults to unset." |
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.
Defaults to unset - is this still accurate?
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.
"Defaults to unset" is still technically accurate for the config field itself:
- tui.experimental_mode is Option with #[serde(default)], so missing value deserializes to None (schema default: null).
But behavior-wise, None now means:
- no explicit override selected by the user, and then
- if collaboration modes are enabled, startup resolves to the Default preset (not Plan) via the runtime fallback.
| // TODO (aibrahim): Consolidate config.model and config.model_reasoning_effort into config.collaboration_mode | ||
| // to avoid extracting these fields separately and constructing CollaborationMode here. |
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.
Just want to point out that we need to think hard about migration path forward if we need to do that. Currently we are not even tracking them.
| Steer, | ||
| /// Enable collaboration modes (Plan, Code, Pair Programming, Execute). | ||
| /// Enable collaboration modes (Plan, Default). | ||
| CollaborationModes, |
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.
Should we just remove this? I am not sure if we are going to turn this off at anytime in the future.
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.
I guess we have to also remove the enable flag on app side as we do that.
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.
Makes sense, will do in followup PR
shijie-oai
left a comment
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.
Overall looks straight forward - one concern I do have (might be outdated info) is when we go from plan -> code, we also tell the model that we are exiting plan mode. want to make sure that behavior is not lost. Also let @aibrahim-oai take another look.
Summary
This PR simplifies collaboration modes to the visible set
default | plan, while preserving backward compatibility for older partners that may still send legacy modenames.
Specifically:
PairProgrammingandExecuteinternally for compatibility plumbing, while removing them from schema/API and UI visibility.What Changed
ModeKindnow usesPlan+Defaultas active/public modes.ModeKind::Defaultdeserialization accepts legacy values:codepair_programmingexecutecustomPairProgrammingandExecutevariants remain in code but are hidden from protocol/schema generation.Customvariant is removed; previous custom fallbacks now map toDefault.PlanDefaultcore/templates/collaboration_mode/code.md->default.mdexecute.mdandpair_programming.mdremain on disk but are not surfaced in visible preset lists.PlanandDefault.request_user_inputremains allowed only inPlanmode.Default.plandefaultBackward Compatibility Notes
code,pair_programming,execute,custom) are accepted and coerced todefault.plan | default.Codex author
codex fork 019c1fae-693b-7840-b16e-9ad38ea0bd00