Simplify tool implemetations #4160

pakrym-oai · 2025-09-24T15:51:02Z

Use Result<String, FunctionCallError> for all tool handling code and rely on error propagation instead of creating failed items everywhere.

- 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

pakrym-oai · 2025-09-24T15:51:25Z

codex-rs/core/src/function_tool.rs

@@ -0,0 +1,7 @@
+use thiserror::Error;
+
+#[derive(Debug, Error, PartialEq)]


Plan to put more function-tool related code here.

pakrym-oai · 2025-09-24T15:55:43Z

codex-rs/core/src/apply_patch.rs

    /// 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>),


This is the part that's I'm not sure is the cleanest.

I definitely prefer this than the pub success: Option<bool>,

# Conflicts: # codex-rs/core/src/codex.rs

chatgpt-codex-connector

💡 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 fix this CI failure
@codex address that feedback

codex-rs/core/src/codex.rs

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.

jif-oai · 2025-09-24T16:02:57Z

codex-rs/core/src/apply_patch.rs

    /// 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>),


I definitely prefer this than the pub success: Option<bool>,

codex-rs/core/src/codex.rs

jif-oai · 2025-09-24T16:05:31Z

codex-rs/core/src/apply_patch.rs

-        }
-        .into(),
+        SafetyCheck::Reject { reason } => InternalApplyPatchInvocation::Output(Err(
+            FunctionCallError::RespondToModel(format!("patch rejected: {reason}")),


PatchRejected could be a variation of the FunctionCallError

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.

jif-oai · 2025-09-24T16:06:10Z

codex-rs/core/src/codex.rs

+                        content,
+                        success: Some(true),
+                    },
+                    Err(FunctionCallError::RespondToModel(msg)) => FunctionCallOutputPayload {


You can implement a From to make this cleaner as I suspect we may need this somewhere else and we will grow FunctionCallError

I don't want to make API types dependent on the agent loop types. Feels like the wrong dependency direction.

I'd like to split client into a separate crate one day.

jif-oai · 2025-09-24T16:07:42Z

codex-rs/core/src/codex.rs

-                        success: Some(false),
-                    },
-                };
+                return Err(FunctionCallError::RespondToModel(format!(


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

Why another type?

jif-oai · 2025-09-24T16:17:41Z

codex-rs/core/src/codex.rs

@@ -2414,15 +2439,10 @@ async fn handle_function_call(
    name: String,
    arguments: String,
    call_id: String,


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

What was this a comment for? Seems not to fit the line

Hmm it got shifted. Basically everywhere where you do {e}

Will fix, good point!

jif-oai · 2025-09-24T16:18:48Z

codex-rs/core/src/codex.rs

-                }
-            };
+            let args: ApplyPatchToolArgs = serde_json::from_str(&arguments).map_err(|e| {
+                FunctionCallError::RespondToModel(format!(


If you have more specific error, you can use the #from in the error definition and just use ? here

The extra formatted message part stopped me failed to parse function arguments from doing a generic from serde conversion.

codex-rs/core/src/codex.rs

Changed SerializedUnifiedExecResult to own the output field (String) instead of borrowing (&str) to simplify serialization. Removed obsolete comments in related code.

jif-oai · 2025-09-24T17:15:05Z

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.

pakrym-oai added 2 commits September 23, 2025 19:29

wip

80f2bf2

pakrym-oai commented Sep 24, 2025

View reviewed changes

Merge branch 'main' into pakrym/cleanup-function-results

6068019

# Conflicts: # codex-rs/core/src/codex.rs

chatgpt-codex-connector bot reviewed Sep 24, 2025

View reviewed changes

codex-rs/core/src/codex.rs Outdated Show resolved Hide resolved

jif-oai reviewed Sep 24, 2025

View reviewed changes

codex-rs/core/src/codex.rs Outdated Show resolved Hide resolved

core: own value of output in SerializedUnifiedExecResult

7510b13

Changed SerializedUnifiedExecResult to own the output field (String) instead of borrowing (&str) to simplify serialization. Removed obsolete comments in related code.

jif-oai approved these changes Sep 24, 2025

View reviewed changes

error: improve error formatting with debug output

38dbebd

- Use debug formatting ({err:?}, {output:?}, etc.) for error messages in function call handling and execution paths for more informative diagnostics.

pakrym-oai enabled auto-merge (squash) September 24, 2025 17:19

pakrym-oai merged commit addc946 into main Sep 24, 2025
19 checks passed

pakrym-oai deleted the pakrym/cleanup-function-results branch September 24, 2025 17:27

github-actions bot locked and limited conversation to collaborators Sep 24, 2025

		@@ -0,0 +1,7 @@
		use thiserror::Error;

		#[derive(Debug, Error, PartialEq)]

Simplify tool implemetations #4160

Simplify tool implemetations #4160

Uh oh!

Conversation

pakrym-oai commented Sep 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jif-oai commented Sep 24, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

pakrym-oai commented Sep 24, 2025 •

edited

Loading