-
Notifications
You must be signed in to change notification settings - Fork 7.6k
Chore: plan mode do not include free form question and always include isOther #10210
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
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: 8dc00a2779
ℹ️ 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".
| - Step 1 | ||
| - Step 2 | ||
| </proposed_plan> | ||
| </proposed_plan> |
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.
Fix example closing tag indentation
The example shows the closing </proposed_plan> tag with two leading spaces, which contradicts the immediately preceding rule that the tags must be exactly <proposed_plan> and </proposed_plan> on their own lines. Since this example is likely to be copied verbatim, it can lead the client to emit a malformed closing tag that won’t be recognized by the renderer.
Useful? React with 👍 / 👎.
8dc00a2 to
6df3651
Compare
6df3651 to
0b4f5bd
Compare
codex-rs/core/src/tools/spec.rs
Outdated
| @@ -606,8 +606,7 @@ fn create_request_user_input_tool() -> ToolSpec { | |||
| "isOther".to_string(), | |||
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 delete isOther from the tool? Why expose this as a tool arg if it should always be true?
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.
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.
Causing rendering UI and I am too lazy to update the logic there lol
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.
Ah but maybe we could just make the model visible tool not include isOther, but hardcode it to true when we send the toolcall to the client? Generally keeping model visible things clean is good for model quality
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.
ya let me do that!
💡 Codex Reviewcodex/codex-rs/core/src/skills/env_var_dependencies.rs Lines 46 to 49 in 0b4f5bd
The new dependency resolver treats any present environment variable as satisfied, but Lines 358 to 360 in 0b4f5bd
The log DB layer is only created if ℹ️ 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". |
| for question in &mut args.questions { | ||
| question.is_other = true; | ||
| } |
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.
We hardcode the isOther property to true passing it to downstream consumer. isOther itself is removed from the tool spec.
f950327 to
54fbfd5
Compare
charley-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.
Nice looks good
We should never ask a freeform question when planning and we should always include isOther as an escape hatch.