-
Notifications
You must be signed in to change notification settings - Fork 6.4k
shell_command returns freeform output #6860
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
shell_command returns freeform output #6860
Conversation
…ations - Dropped the o3 model tools assertion from model_selects_expected_tools. - Added "apply_patch" as an expected tool for gpt-5-codex. - Clarified test description for gpt-5 toolset assertion.
…utput count tests: remove ShellModelOutput::ShellCommand cases to match updated model output formatting
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.
ℹ️ 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".
| Err(ToolError::Codex(CodexErr::Sandbox(SandboxErr::Timeout { output }))) | ||
| | Err(ToolError::Codex(CodexErr::Sandbox(SandboxErr::Denied { output }))) => { | ||
| let response = super::format_exec_output_for_model(&output); | ||
| let response = super::format_exec_output_for_model_structured(&output); | ||
| let event = ToolEventStage::Failure(ToolEventFailure::Output(*output)); |
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.
Freeform errors must avoid JSON leakage
We now skip reserializing shell_command in client_common::is_shell_tool_name, so any string returned from here must already be in the final freeform format. The success path handles that, but the timeout/denied branch still calls format_exec_output_for_model_structured, which produces the old JSON blob. Because shell_command is no longer picked up by reserialize_shell_outputs, those error responses will hit the model as raw JSON again. Could we route this branch through the same freeform formatter when self is the Shell { freeform: true, … } variant?
Useful? React with 👍 / 👎.
| "update_plan", | ||
| "view_image", | ||
| ], | ||
| ), |
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 still keep the variations here with gpt-5 to validate old behavior?
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.
they are never used
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.
only current model is being picked from the list
Introduce format_exec_output_for_model helper on ToolEmitter to select freeform vs structured formatting consistently. Use it for success and sandbox timeout/denied paths. Update freeform exec output truncation to use SHELL_OUTPUT_MAX_BYTES and a fixed 256 line limit.
…dOnly for shell_command output tests
…tput # Conflicts: # codex-rs/core/tests/suite/prompt_caching.rs
Instead of returning structured out and then re-formatting it into freeform, return the freeform output from shell_command tool.
Keep
shellas the default tool for GPT-5.