Simplify tool implemetations#4160
Conversation
- Handles errors in function/tool/cmd calls using `Result<String, FunctionCallError>` instead of wrapping outputs in `ResponseInputItem`. - Removes unnecessary `ResponseInputItem` conversions and related From impls. - Refactors exec handling, sandbox error, patch, plan/mcp tool call, and tool output mapping to propagate errors via Result. - Demotes exec_command result_into_payload; now directly uses text output or error
| @@ -0,0 +1,7 @@ | |||
| use thiserror::Error; | |||
|
|
|||
| #[derive(Debug, Error, PartialEq)] | |||
There was a problem hiding this comment.
Plan to put more function-tool related code here.
| /// of sandbox, because the user explicitly approved it. This is the | ||
| /// result to use with the `shell` function call that contained `apply_patch`. | ||
| Output(ResponseInputItem), | ||
| Output(Result<String, FunctionCallError>), |
There was a problem hiding this comment.
This is the part that's I'm not sure is the cleanest.
There was a problem hiding this comment.
I definitely prefer this than the pub success: Option<bool>,
# Conflicts: # codex-rs/core/src/codex.rs
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 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
Move conversion of MCP tool call results to structured output (ResponseInputItem) into handle_mcp_tool_call. Simplify error handling for invalid arguments and return errors as structured responses. Remove in-place stringification of results—conversion now occurs in higher layers.
| /// of sandbox, because the user explicitly approved it. This is the | ||
| /// result to use with the `shell` function call that contained `apply_patch`. | ||
| Output(ResponseInputItem), | ||
| Output(Result<String, FunctionCallError>), |
There was a problem hiding this comment.
I definitely prefer this than the pub success: Option<bool>,
| } | ||
| .into(), | ||
| SafetyCheck::Reject { reason } => InternalApplyPatchInvocation::Output(Err( | ||
| FunctionCallError::RespondToModel(format!("patch rejected: {reason}")), |
There was a problem hiding this comment.
PatchRejected could be a variation of the FunctionCallError
There was a problem hiding this comment.
Frankly this function should itself be returning Result<... > today the return type is strange in that it encapsulates an error deep inside the return value.
But I don't want to poke apply_patch internals too much yet.
| content, | ||
| success: Some(true), | ||
| }, | ||
| Err(FunctionCallError::RespondToModel(msg)) => FunctionCallOutputPayload { |
There was a problem hiding this comment.
You can implement a From to make this cleaner as I suspect we may need this somewhere else and we will grow FunctionCallError
There was a problem hiding this comment.
I don't want to make API types dependent on the agent loop types. Feels like the wrong dependency direction.
There was a problem hiding this comment.
I'd like to split client into a separate crate one day.
| success: Some(false), | ||
| }, | ||
| }; | ||
| return Err(FunctionCallError::RespondToModel(format!( |
There was a problem hiding this comment.
Another type of error I guess ?
You will have multiple of them but would be cool to have a few standard one and then you can create a generic FunctionCallError::RawRespondToModel(String) for specific use cases
There was a problem hiding this comment.
Why another type?
| @@ -2414,15 +2439,10 @@ async fn handle_function_call( | |||
| name: String, | |||
| arguments: String, | |||
| call_id: String, | |||
There was a problem hiding this comment.
In general, always print the error with the Debug trait (i.e. {err:?})
Otherwise, if the error implement the Display trait you don't see the exact type of the error. This is mainly true for thiserror where if you have this:
pub enum FunctionCallError {
#[error("{0}")]
RespondToModel(String),
}
...
FunctionCallError::RespondToModel("An error".to_string())
You would see "An error" with the Display print but RespondToModel("An error") with the debug one
There was a problem hiding this comment.
What was this a comment for? Seems not to fit the line
There was a problem hiding this comment.
Hmm it got shifted. Basically everywhere where you do {e}
There was a problem hiding this comment.
Will fix, good point!
| } | ||
| }; | ||
| let args: ApplyPatchToolArgs = serde_json::from_str(&arguments).map_err(|e| { | ||
| FunctionCallError::RespondToModel(format!( |
There was a problem hiding this comment.
If you have more specific error, you can use the #from in the error definition and just use ? here
There was a problem hiding this comment.
The extra formatted message part stopped me failed to parse function arguments from doing a generic from serde conversion.
Changed SerializedUnifiedExecResult to own the output field (String) instead of borrowing (&str) to simplify serialization. Removed obsolete comments in related code.
|
For the record: remaining points were discussed on Slack |
- Use debug formatting ({err:?}, {output:?}, etc.) for error messages in function call handling and execution paths for more informative diagnostics.
2 participants
Use Result<String, FunctionCallError> for all tool handling code and rely on error propagation instead of creating failed items everywhere.