fix(providers): reject non-object tool-call arguments instead of panicking by lunchboxfortwo · Pull Request #9832 · aaif-goose/goose

lunchboxfortwo · 2026-06-16T22:09:24Z

Summary

Weak/cheap models occasionally emit tool-call arguments that are valid JSON but not a JSON object (a bare array, string, or number). Both the OpenAI/OpenRouter (crates/goose-providers/src/formats/openai.rs) and Databricks (crates/goose/src/providers/formats/databricks.rs) response decoders passed the parsed value straight to rmcp's object(), whose debug_assert!(value.is_object()) panics the entire agent process in debug builds (and in release silently coerces to an empty object, running the tool with no arguments).

This guards the non-object case at both decoder sites and returns an INVALID_PARAMS tool error — the same way malformed-JSON arguments are already handled — so the model gets feedback and can retry instead of crashing the run.

Found while working on #9708: the panic repeatedly killed eval runs that exercise cheap models, which is exactly the population that emits these malformed tool calls.

Testing

Added regression tests at both decoder sites (test_response_to_message_non_object_arguments): a bare-array arguments value now yields an INVALID_PARAMS tool error instead of panicking.
cargo test -p goose-providers --lib formats::openai and cargo test -p goose --lib providers::formats::databricks — pass.
cargo fmt clean; cargo clippy --all-targets -- -D warnings clean.

Steps to reproduce (before this fix):

Point goose at a cheap tool-using model (e.g. deepseek/deepseek-chat via OpenRouter).
Run any task that triggers tool calls.
The model occasionally emits "arguments": "[...]" — valid JSON, but not an object.
In a debug build the run aborts with thread 'goose-cli-main' panicked ... assertion failed: value.is_object() in rmcp .../model.rs. (In release it silently runs the tool with empty arguments.)

Related Issues

Relates to #9708 (ground-truth completion gate) — independent robustness bug found while working on it, split out as its own PR.

…cking Weak models occasionally emit tool-call `arguments` that are valid JSON but not an object (a bare array/string/number). Both the OpenAI/OpenRouter and Databricks response decoders passed this straight to rmcp's `object()`, whose `debug_assert!(value.is_object())` panics the entire run in debug builds (and silently coerces to an empty object in release). Guard the non-object case and surface an INVALID_PARAMS tool error so the model can retry. Adds regression tests at both decoder sites. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Lunchbox <lunchboxfortwo@gmail.com>

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 956e37ad27

ℹ️ 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".

chatgpt-codex-connector · 2026-06-16T22:13:03Z

                    match safely_parse_json(&arguments_str) {
-                        Ok(params) => {
+                        // Valid JSON, and an object: the normal path.
+                        Ok(params) if params.is_object() => {


Guard streamed tool-call arguments as well

This guard only protects the non-streaming response_to_message path. The default OpenAI-compatible providers I checked route streaming responses through stream_openai_compat into response_to_streaming_message, and that decoder still accepts Ok(params) and calls object(params) without an object check at openai.rs:1130-1134. When a streamed tool call ends with arguments such as [1, 2, 3], the same rmcp debug assertion or release empty-args behavior described by this fix still occurs for streaming-enabled OpenAI/OpenRouter/Databricks-compatible runs.

Useful? React with 👍 / 👎.

Review caught that the initial fix only covered the non-streaming `response_to_message`; `response_to_streaming_message` had the same unguarded `object(params)` call. Streaming is the default path for OpenAI/OpenRouter, so it's the more common trigger. Applies the same INVALID_PARAMS handling and adds a streaming regression test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Lunchbox <lunchboxfortwo@gmail.com>

lunchboxfortwo · 2026-06-16T23:40:39Z

Good catch — fixed in 3ad8e98.

The streaming decoder (response_to_streaming_message) had the same unguarded object(params) call, and since OpenAI/OpenRouter stream by default it's actually the more common trigger. Applied the same is_object() guard + INVALID_PARAMS handling there, and added a streaming regression test (test_streaming_non_object_arguments_does_not_panic) that feeds arguments: "[1, 2, 3]" through the streaming path and asserts a tool error instead of a panic.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3ad8e9840d

ℹ️ 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".

chatgpt-codex-connector · 2026-06-16T23:44:33Z

+                            content.push(MessageContent::tool_request_with_metadata(
+                                id,
+                                Err(error),
+                                metadata.as_ref(),


Feed non-object argument errors back to the model

For the newly handled case where a model emits valid JSON that is not an object, this creates a ToolRequest whose tool_call is Err. I checked the agent loop in crates/goose/src/agents/agent.rs:2128-2163: errored tool requests are not added as an assistant tool call plus tool response for the next model turn; the agent logs the parse error, emits a generic assistant message, sets exit_chat = true, and breaks. That means the exact weak-model scenario this branch handles terminates the run instead of giving the model the INVALID_PARAMS feedback needed to retry.

Useful? React with 👍 / 👎.

…— infra flake, unrelated to the 2-file decoder change) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Lunchbox <lunchboxfortwo@gmail.com>

michaelneale · 2026-06-17T01:37:10Z

@DOsinga I kind of like this, may help with more modest models

…terminating An unparseable tool call (non-object arguments, or a truncated/malformed response) previously set exit_chat and ended the run. Instead, attach the error as a tool response so the model receives structured feedback and can retry on the next turn. Signed-off-by: Douwe M Osinga <douwe@sidewalklabs.com>

DOsinga · 2026-06-18T13:38:39Z

Thanks for this @lunchboxfortwo — good catch on the panic.

I pushed a follow-up commit to address codex's P2. Previously an unparseable tool call (your new non-object case, but also malformed/truncated responses) set exit_chat and ended the run, so the model never actually got the chance to retry. I changed the agent loop to attach the parse error as a tool response instead, so the model receives structured feedback and can correct itself on the next turn. Added an integration test in crates/goose/tests/agent.rs that returns a bad tool call on turn 1 and verifies the loop feeds back the error and continues to a second turn.

I merged main into the branch as well. Could you take a look and confirm you're happy with the change?

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5597831725

ℹ️ 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".

chatgpt-codex-connector · 2026-06-18T13:42:42Z

+                                    request_msg = request_msg
+                                        .with_tool_request_with_metadata(
+                                            request.id.clone(),
+                                            request.tool_call.clone(),
+                                            request.metadata.as_ref(),


Keep a matching assistant tool call for parse errors

When request.tool_call is Err (the new non-object/invalid-JSON path), this stores the error as a ToolRequest in conversation history. I checked the OpenAI formatter: ToolRequest(Err(_)) is emitted as a role: "tool" message (crates/goose-providers/src/formats/openai.rs:262-267), not as an assistant tool_calls entry; the Err branch below also adds a ToolResponse for the same id. The retry request sent to OpenAI-compatible providers therefore contains tool-role messages with no preceding assistant tool call, which strict APIs reject, so the model still cannot correct its bad arguments. Persist a valid assistant tool-call representation for the bad call, or only add a tool response in a form the formatter can pair with it.

Useful? React with 👍 / 👎.

lunchboxfortwo · 2026-06-18T14:00:04Z

Thanks for this @lunchboxfortwo — good catch on the panic.

I pushed a follow-up commit to address codex's P2. Previously an unparseable tool call (your new non-object case, but also malformed/truncated responses) set exit_chat and ended the run, so the model never actually got the chance to retry. I changed the agent loop to attach the parse error as a tool response instead, so the model receives structured feedback and can correct itself on the next turn. Added an integration test in crates/goose/tests/agent.rs that returns a bad tool call on turn 1 and verifies the loop feeds back the error and continues to a second turn.

I merged main into the branch as well. Could you take a look and confirm you're happy with the change?

Thanks for jumping on this. I agree with the direction, but I think this version still has a history-shape bug. (See codex review message to your edits)

The new agent path stores the bad parse as ToolRequest(Err) and then also adds a ToolResponse(Err). For OpenAI-compatible formatting, ToolRequest(Err) does not become an assistant message with tool_calls; it formats as a role: "tool" message. The added ToolResponse(Err) also formats as a role: "tool" message.

So on the next provider call, strict OpenAI-style APIs see tool messages without a preceding assistant tool_calls entry for that id, and reject the request before the model can retry.

The new test proves the loop reaches a second provider turn, but the mock ignores the messages it receives, so it doesn_t catch whether the second request would be valid for OpenAI/Databricks.

I think we need either:

a valid assistant tool-call placeholder for the failed parse, paired with the error tool response, or
a different feedback representation that doesn_t serialize as an orphan tool message.

Could you update the fix and add a regression that formats the post-error conversation through the OpenAI-compatible formatter?

kimnamu

Thanks for this fix, and thanks to @DOsinga for the agent-loop follow-up — the panic guard at both decoder sites is clearly the right call. I'm not a maintainer, just another OpenAI/Bedrock-compatible-provider user who reviewed this carefully and ran the branch locally. I confirmed both decoder regression tests pass and that the is_object() guards correctly turn a bare-array arguments into an INVALID_PARAMS tool error instead of tripping rmcp's object() debug-assert.

On the open history-shape concern that @lunchboxfortwo and codex P1 raised on the latest commit — I reproduced it locally and it reproduces exactly as described, so I wanted to bring the regression test + a working fix sketch you asked for.

Repro (post-error history → OpenAI formatter). I reconstructed the history the new agent loop builds (assistant request_msg holding ToolRequest(Err), then final_response holding ToolResponse(Err)) and ran it through format_messages:

[0] role="user"
[1] role="tool"  tool_call_id="call_bad"
[2] role="tool"  tool_call_id="call_bad"

The assistant request_msg produces no message at all — ToolRequest(Err) serializes via the Err arm at openai.rs:262-267 as a role:"tool" entry, and the assistant wrapper has no content/tool_calls, so has_message_payload is false and it's dropped (openai.rs:418). Result: two consecutive role:"tool" messages with no preceding assistant tool_calls. Strict OpenAI-compatible APIs reject this — which is the exact cheap-model/OpenRouter population this PR targets. (Databricks' formatter at databricks.rs:193-195 instead emits the error as assistant text, so the following tool response is still orphaned there too — same bug, different shape.)

Regression test you asked for (drop into crates/goose-providers/src/formats/openai.rs tests — it fails on 5597831 and passes once the history is well-formed):

#[test]
fn formatter_post_parse_error_history_is_wellformed() {
    use rmcp::model::{ErrorCode, ErrorData};
    let err = ErrorData::new(
        ErrorCode::INVALID_PARAMS,
        "Tool arguments for id call_bad must be a JSON object".to_string(),
        None,
    );
    // Shape the agent loop builds today for a failed parse:
    let request_msg = Message::assistant().with_tool_request("call_bad", Err(err.clone()));
    let mut final_resp = Message::user();
    final_resp.add_tool_response_with_metadata("call_bad", Err(err), None);
    let messages = vec![
        Message::user().with_text("do the thing"),
        request_msg,
        final_resp,
    ];

    let spec = format_messages(&messages, &ImageFormat::OpenAi);

    // Every role:"tool" must follow an assistant tool_calls entry with the same id.
    let mut open = std::collections::HashSet::new();
    for m in &spec {
        match m.get("role").and_then(|v| v.as_str()) {
            Some("assistant") => {
                for tc in m.get("tool_calls").and_then(|v| v.as_array()).into_iter().flatten() {
                    if let Some(id) = tc.get("id").and_then(|v| v.as_str()) {
                        open.insert(id.to_string());
                    }
                }
            }
            Some("tool") => {
                let id = m.get("tool_call_id").and_then(|v| v.as_str()).unwrap_or("");
                assert!(open.contains(id), "orphan role:tool message for id {id:?}");
            }
            _ => {}
        }
    }
}

One fix that makes this pass (of your two options, the "valid assistant tool-call placeholder" one): in the agent loop's Err branch, give request_msg a valid Ok placeholder tool-call instead of cloning the Err, and keep the error on the paired tool response. I verified this produces a well-formed assistant{tool_calls:[{id, name, arguments:"{}"}]} + tool{error} pair that the formatter accepts. A real model-emitted name isn't available from Err(ErrorData), so a synthetic name ("unparseable_tool_call", arguments:"{}") is the minimal option; if you'd rather preserve the original function name, that'd need threading it from the decoder.

Not blocking, just flagging: the new integration test in tests/agent.rs proves the loop reaches a 2nd provider turn, but its mock ignores incoming messages, so it can't catch this — the formatter-level test above is what closes that gap.

Everything else looks clean and well-scoped — I'd love to see this land once the post-error history is OpenAI-valid.

This review was done with the help of an AI agent (Claude Code); I reviewed the reasoning, ran the branch locally, and verified the outputs myself.

@kimnamu

…eholder tool-call The parse-error feedback added in 5597831 (feed the error back so the model can retry instead of terminating) produced a malformed conversation. A failed parse is stored as ToolRequest(Err): the OpenAI formatter serialized it as a bare role:"tool" message and the Databricks formatter as assistant text — in both cases with no assistant `tool_calls` entry. The paired error tool response then has no preceding tool_calls for its id, so strict OpenAI-compatible APIs reject the next request and the model never gets to retry — exactly the cheap-model / OpenRouter population this PR targets. Both formatters now emit a placeholder assistant tool-call (same id, name "unparseable_tool_call") for an unparseable request; the parse error rides on the following tool response, keeping the history well-formed. Adds formatter-level regression tests at both sites asserting no orphan role:"tool" message survives (each fails before the fix). Diagnosis, the OpenAI regression test, and the placeholder approach are from @kimnamu's review, verified locally. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Goose <goose@aaif.dev>

lunchboxfortwo · 2026-06-20T02:35:31Z

Thanks @kimnamu — verified your diagnosis locally and it's exactly right. Your regression test fails on 5597831 with orphan role:tool message for id "call_bad": the failed-parse ToolRequest(Err) serializes (OpenAI) as a bare role:"tool" / (Databricks) as assistant text with no tool_calls, so the paired error tool response is orphaned and strict OpenAI-compatible APIs reject the retry — the exact population this targets.

Pushed d6fd0db. I took the formatter-level route (rather than a placeholder in the agent loop) for two reasons: it makes your verbatim test pass, and it leaves @DOsinga's parse-error-feedback loop untouched while hardening any ToolRequest(Err) against orphaning. Both formatters now emit a placeholder assistant tool-call (same id, unparseable_tool_call) and the parse error rides on the following tool response.

Added your OpenAI regression test verbatim (formatter_post_parse_error_history_is_wellformed).
Mirrored it for Databricks (the second shape you flagged) — confirmed it also fails before the fix (orphan role:tool) and passes after.
Both fail-before / pass-after; cargo fmt + clippy -D warnings clean.

@DOsinga — your loop change is unchanged and now produces valid OpenAI/Databricks history through the formatter; ready for another look. (Note: the agent integration tests can't run in my env — pre-existing sqlx-runtime panics on a clean checkout — so the formatter-level tests are what cover this.)

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d6fd0db3bf

ℹ️ 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".

chatgpt-codex-connector · 2026-06-20T02:41:46Z

+                                        Err(error) => {
+                                            error!("Tool call could not be parsed: {error}");
+                                            let mut response = request_to_response_map
+                                                .remove(&request.id)
+                                                .unwrap_or_else(|| Message::user().with_generated_id());
+                                            response.add_tool_response_with_metadata(
                                                request.id.clone(),
-                                                request.tool_call.clone(),
+                                                Err(error.clone()),
                                                request.metadata.as_ref(),


Avoid retrying parse errors through unsupported formats

This new Err path feeds parse-error tool calls back to the model for every provider, but only the OpenAI/Databricks formatters in this change know how to serialize the matching assistant tool call. For example, Anthropic streaming can produce MessageContent::tool_request(..., Err(...)) when streamed tool arguments fail to parse (crates/goose/src/providers/formats/anthropic.rs:907-923), while its formatter skips ToolRequest(Err) (crates/goose/src/providers/formats/anthropic.rs:195-208) and still serializes the ToolResponse added here as a tool_result; the next Anthropic request therefore contains a tool result with no preceding tool_use, so the retry path fails instead of letting the model correct the arguments.

Useful? React with 👍 / 👎.

lunchboxfortwo · 2026-06-20T02:57:38Z

Heads up @DOsinga — CI is red, but it's not the formatter change (d6fd0db only touches openai.rs/databricks.rs). It's branch-behind-main drift: this branch is ~22 commits behind main, and your tests/agent.rs no longer compiles against current main:

error[E0432]: unresolved import goose::model`` — crates/goose/tests/agent.rs:507 (the module moved on main)
error[E0050]: method from_env has 2 parameters but the trait declaration ... has 3 — crates/goose/tests/agent.rs:551 (the ProviderDef::from_env signature gained a param on main; there's now also from_env_with_working_dir)

Since it's your integration test and you're closest to the from_env change, could you re-merge main and adjust the test for the new signature + import path? Happy to take it if you'd rather — just didn't want to rework your test against a main API change without checking. The formatter commit itself is green (fmt + clippy clean, both formatter regression tests pass).

… fix test for main drift The agent loop now feeds unparseable tool-call errors back to the model for every provider. Anthropic's formatter skipped ToolRequest(Err) while still emitting the paired tool_result, producing an orphaned tool_result that Anthropic rejects. Emit a placeholder tool_use with the same id so the history stays valid, matching the OpenAI/Databricks formatters. Also update tests/agent.rs for main drift: ModelConfig moved to goose_providers::model and ProviderDef::from_env gained a tls_config arg.

DOsinga · 2026-06-22T12:06:42Z

Pushed two commits to the branch:

Merged main to clear the CI failure. Your tests/agent.rs had drifted: ModelConfig moved to goose_providers::model and ProviderDef::from_env gained a tls_config arg. Fixed both.
Handled the codex Anthropic P1. The agent-loop change feeds unparseable tool-call errors back to the model for every provider, but only the OpenAI/Databricks formatters had the placeholder fix. Anthropic's formatter skipped ToolRequest(Err) while still emitting the paired tool_result, so on Anthropic the retry produced an orphaned tool_result with no preceding tool_use — which Anthropic rejects. It now emits a placeholder tool_use with the same id (mirroring the OpenAI/Databricks formatters), with a regression test.

cargo fmt clean; the changed crates are clippy-clean; the new and existing format/agent tests pass. The PR's core fix is solid — good catch on the rmcp object() debug-assert panic.

(Note: there's a pre-existing clear_env dead-code clippy warning and a sqlx feature-unification quirk in tests/providers.rs/api_client.rs when those crates are built in isolation — both exist on main and are unrelated to this PR.)

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 6be421d3b6

ℹ️ 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".

chatgpt-codex-connector · 2026-06-22T12:10:36Z

+                                    request_msg = request_msg
+                                        .with_tool_request_with_metadata(
+                                            request.id.clone(),
+                                            request.tool_call.clone(),
+                                            request.metadata.as_ref(),


Add Bedrock replay support before storing parse-error calls

Fresh evidence: Bedrock still emits MessageContent::tool_request(..., Err(...)) when streamed tool arguments fail to parse (crates/goose/src/providers/bedrock.rs:655-663), but its formatter turns a ToolRequest(Err) into a ToolUseBlock with only the id and no name/input (crates/goose/src/providers/formats/bedrock.rs:152-156). Because this new path now persists every errored tool request in history and pairs it with a tool-response error, the next Bedrock turn fails during request formatting instead of letting the model correct the arguments; either add a valid Bedrock placeholder or keep the old terminal behavior for formats that cannot replay parse errors.

Useful? React with 👍 / 👎.

Completes unparseable-tool-call handling across all formatters. The agent loop feeds ToolRequest(Err) back to the model for every provider, but the Bedrock formatter built a ToolUseBlock with only the id (no name/input), so the paired tool_result is orphaned and Bedrock rejects it — the same class of bug fixed for OpenAI/Databricks/Anthropic. Both the ToolRequest and FrontendToolRequest Err arms now emit a placeholder tool_use (name "unparseable_tool_call"); regression test added. Catch via codex's review (P2, Bedrock replay). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Goose <goose@aaif.dev>

lunchboxfortwo · 2026-06-22T13:12:11Z

Pushed 8bc3cf5 for codex's Bedrock P2 — good catch. The Bedrock formatter built a ToolUseBlock with only the id (no name/input) for ToolRequest(Err), so the paired tool_result was orphaned. Both the ToolRequest and FrontendToolRequest Err arms now emit a placeholder tool_use (unparseable_tool_call), mirroring the OpenAI/Databricks/Anthropic fixes, with a regression test (tool_request_parse_error_gets_placeholder_name, gated behind --features aws-providers; verified locally with the feature). That completes the placeholder handling across all four formatters.

The other four items in codex's latest pass look stale — they're already addressed on the branch (CI is green):

Guard streamed args — already guarded at openai.rs Ok(params) if params.is_object() (streaming response_to_streaming_message).
Feed errors back to the model — done by @DOsinga's 5597831 (agent loop no longer sets exit_chat).
Keep a matching assistant tool call (OpenAI orphan) — fixed; ToolRequest(Err) now serializes as a placeholder assistant tool_calls entry (openai.rs / databricks.rs).
Anthropic orphan — fixed by @DOsinga's 6be421d (placeholder tool_use).

So with Bedrock in, all of codex's findings should be resolved.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8bc3cf5b4c

ℹ️ 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".

chatgpt-codex-connector · 2026-06-22T13:18:45Z

+                        tool_calls.as_array_mut().unwrap().push(json!({
+                            "id": request.id,
+                            "type": "function",
+                            "function": {
+                                "name": "unparseable_tool_call",


Preserve provider metadata on placeholder calls

When the malformed tool call carried provider metadata, this placeholder drops it even though the normal Ok branch copies every request.metadata field onto the replayed tool call. The new decoders keep metadata on ToolRequest(Err) (for example streamed OpenAI-compatible calls attach thoughtSignature/extra_content before returning the error), and the retry path in agent.rs now relies on this formatter to replay that assistant call. For Gemini-style OpenAI-compatible endpoints, losing the thought signature on exactly these parse-error retries can make the next request fail or lose reasoning continuity before the model gets a chance to correct the arguments; copy the metadata onto the placeholder as the Ok branch does.

Useful? React with 👍 / 👎.

chatgpt-codex-connector · 2026-06-22T13:18:45Z

+                                            response.add_tool_response_with_metadata(
                                                request.id.clone(),
-                                                request.tool_call.clone(),
+                                                Err(error.clone()),
                                                request.metadata.as_ref(),


Avoid orphaning Google function responses

Fresh evidence: the Google formatter still serializes ToolRequest(Err) as plain model text (crates/goose/src/providers/formats/google.rs:141-143), while the ToolResponse(Err) added here serializes as a functionResponse (google.rs:181-188). With this new branch, a Gemini/Google response that hits the existing invalid-function-name path (google.rs:274-283) is persisted as text plus a function response with no preceding functionCall, so the next Google request cannot pair the result with a tool call; either add Google placeholder replay support or keep the old terminal behavior for formats that cannot replay errored requests.

Useful? React with 👍 / 👎.

lunchboxfortwo · 2026-06-22T13:39:25Z

CI's red on 8bc3cf5, but it's a flake unrelated to the Bedrock change (the formatter commit only touches bedrock.rs; all 1715 lib tests incl. the new bedrock test passed).

The failure is @DOsinga's tests::unparseable_tool_call_tests::test_unparseable_tool_call_feeds_back_and_continues. I reproduced it locally (--features rustls-tls), running it 3× back-to-back:

run 1: FAILED — thread 'sqlx-sqlite-worker-0' panicked at sqlx-sqlite-0.8.6/src/row.rs:43
run 2: ok
run 3: ok

So it's a nondeterministic sqlx-sqlite race in the test's session storage (sometimes it panics in the worker; sometimes the session read misses the fed-back tool response and the saw_tool_response_error assert at agent.rs:651 fires). It passed on 6be421d by luck. @DOsinga — could you re-run the failed job? It should go green. Might also be worth hardening the test's DB isolation (fresh/unique sqlite per test, or serializing those session reads) so it stops flaking.

Happy to push an empty commit to re-trigger if you'd prefer that over a manual re-run.

…source Codex's iterative review surfaced the same orphaned-tool-call bug in formatter after formatter (OpenAI, Databricks, Anthropic, Bedrock, and now Google), plus that each per-formatter placeholder drops provider metadata the Ok path keeps. Fix it once at the source instead: when the agent loop feeds an unparseable tool call back to the model, store a placeholder Ok tool-call (name "unparseable_tool_call") in history rather than ToolRequest(Err). Every formatter's existing Ok path then serializes it correctly — Google and any future formatter included — and provider metadata is preserved (passed through to with_tool_request_with_metadata and copied by the Ok path). The parse error still rides on the paired tool response. The per-formatter Err-arm placeholders remain as defense-in-depth for other ToolRequest(Err) sources. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Goose <goose@aaif.dev>

lunchboxfortwo · 2026-06-22T13:51:49Z

@DOsinga — codex's latest pass found the same orphan bug in a 5th formatter (Google: google.rs:141-143 emits ToolRequest(Err) as plain text while the paired ToolResponse(Err) becomes a functionResponse → orphaned), plus that every per-formatter placeholder drops provider metadata the Ok path copies (e.g. Gemini thoughtSignature). Both verified.

Five formatters patched one-by-one, each re-dropping metadata, is a sign the fix is in the wrong layer — so I pushed a926b55 to fix it once at the source: the agent loop now stores a placeholder Ok tool-call (unparseable_tool_call) in history instead of ToolRequest(Err). Consequences:

Every formatter's existing Ok path serializes it correctly — Google and any future formatter included, no per-formatter patch needed (closes codex's Google P2).
Metadata is preserved — request.metadata is already passed to with_tool_request_with_metadata and the Ok path copies it onto the call (closes codex's metadata P2).
The parse error still rides on the paired tool response, so your retry-feedback behavior is unchanged.

The per-formatter Err-arm placeholders (OpenAI/Databricks/Anthropic/Bedrock) now act as defense-in-depth for other ToolRequest(Err) sources. If you'd rather keep just the source fix and revert those for a tighter diff, happy to — your call on the final shape since it's your loop code.

Verified locally: your test_unparseable_tool_call_feeds_back_and_continues passes 3/3, the openai (79) + goose formatter (195, incl. bedrock under aws-providers) suites pass, fmt/clippy clean.

(Separately: the earlier red on 8bc3cf5 was the flaky sqlx-sqlite race in that same test — 1/3 locally — not a real failure. A re-run should be green.)

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a926b55a22

ℹ️ 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".

chatgpt-codex-connector · 2026-06-22T13:57:38Z

+                                            response.add_tool_response_with_metadata(
                                                request.id.clone(),
-                                                request.tool_call.clone(),
+                                                Err(error.clone()),
                                                request.metadata.as_ref(),


Avoid adding chat-mode skip before parse-error response

When goose_mode == Chat, the chat branch at agent.rs:2024-2033 has already added CHAT_MODE_TOOL_SKIPPED_RESPONSE to the same request_to_response_map entry for every remaining_request, including ToolRequest(Err). In that scenario this new append adds a second ToolResponse with the same id carrying the parse error, so the next model turn gets one placeholder tool call paired with both a successful “tool skipped” result and an error result; skip the chat-mode response for parse-error requests or clear it before adding the parse error.

Useful? React with 👍 / 👎.

…at mode In Chat mode the skip branch already adds CHAT_MODE_TOOL_SKIPPED_RESPONSE for each tool-call id; the parse-error path then added a second ToolResponse(Err) for the same id, producing a duplicate tool_call_id that strict providers reject. Only feed the parse error back when the id isn't already answered. Catch via codex's review. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Goose <goose@aaif.dev>

lunchboxfortwo · 2026-06-22T14:32:02Z

Pushed 2cd76c8 for codex's newest finding (the real one this pass): in Chat mode the skip branch already adds CHAT_MODE_TOOL_SKIPPED_RESPONSE for each tool-call id, and the parse-error path then added a second ToolResponse(Err) for the same id → duplicate tool_call_id. Verified it's reachable: every request (incl. Err) gets a response-map entry (agent.rs:2008), the chat-skip block (2024-2033) populates it, and the shared parse-error block (2196+, after the chat/non-chat if/else closes at 2172) then added a duplicate. Fix: only feed the parse error back when the id isn't already answered.

Verified: your test_unparseable_tool_call_feeds_back_and_continues (non-Chat) still passes 3/3 — the error is still fed back when nothing answered it; fmt/clippy clean. I didn't add a Chat-mode integration test (it'd need the reply loop in Chat mode + the mock, and those agent tests are sqlx-flaky here) — the guard itself is a simple already-answered check; a Chat-mode regression test would be a good small follow-up if you want one (happy to add, or you may have a cleaner harness for it).

The other items in codex's a926b55 pass (Google orphan, metadata-on-placeholder, Bedrock) are stale — the source-level placeholder fix (a926b55) resolves all three via each formatter's Ok path; codex re-anchors them to the new commit each pass.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2cd76c82c3

ℹ️ 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".

chatgpt-codex-connector · 2026-06-22T14:40:38Z

+                                            let already_answered = response.content.iter().any(|c| {
+                                                matches!(c, MessageContent::ToolResponse(r) if r.id == request.id)
+                                            });
+                                            if !already_answered {


Replace chat skips with parse-error feedback

Fresh evidence in this revision is that already_answered now suppresses Err(error.clone()) whenever any response for the id exists. In GooseMode::Chat, the branch at agent.rs:2024-2031 pre-populates every remaining request — including ToolRequest(Err) — with the successful CHAT_MODE_TOOL_SKIPPED_RESPONSE, so malformed tool arguments are replayed as a placeholder assistant tool call plus a successful skip result instead of the INVALID_PARAMS error. In chat sessions the model then cannot correct the bad arguments and may explain a skipped unparseable_tool_call; clear or avoid the chat skip for Err requests and send the parse error instead.

Useful? React with 👍 / 👎.

lunchboxfortwo · 2026-06-22T14:59:12Z

State of the PR (consolidated) — @DOsinga

Functionally complete; CI is green except a known flake (below). Summary so you don't have to reconstruct it from the thread:

What's in:

Panic fix (the original bug): non-object tool-call args no longer hit rmcp's object() debug-assert; guarded in the OpenAI (streaming + non-streaming) and Databricks decoders, with tests.
Your retry-feedback (5597831): unparseable calls are fed back so the model can self-correct — kept as-is.
Centralized history fix (a926b55): the agent loop stores a placeholder Ok tool-call (unparseable_tool_call) instead of ToolRequest(Err), so every formatter's normal Ok path produces valid history (incl. Google + any future formatter) and provider metadata is preserved. This is the load-bearing fix.
Per-formatter placeholders (OpenAI/Databricks/Anthropic/Bedrock): now defense-in-depth for other ToolRequest(Err) sources.
Chat-mode dedup (2cd76c8): stops a duplicate tool_call_id when the chat-skip and parse-error paths both fired.

Decisions that are yours to make:

(A) Final shape of the centralized fix. Happy to revert the four per-formatter placeholders now that the source fix covers everything, for a tighter diff — or keep them as defense. Your call; I'll do whichever.
(B) Codex's latest P2 (chat-mode semantics). With the dedup guard, a malformed call in Chat mode surfaces as a skipped-success rather than the INVALID_PARAMS error. Minor, and it's your chat-mode logic — if you want malformed calls to surface the error in chat too, I'll make the chat-skip branch skip Err requests.
(C) The CI flake. The only red is your test_unparseable_tool_call_feeds_back_and_continues — a sqlx-sqlite race (locally ~1/3 fail, 2/3 pass; all 1715 lib tests + the new code pass). Needs a re-run, and ideally DB-isolation hardening so it stops flaking.

Please ignore Codex's re-flags of: streaming-args guard, OpenAI/Anthropic orphan, Bedrock replay, Google orphan, metadata-on-placeholder. All are resolved (the source fix handles them via each formatter's Ok path); Codex just re-anchors them to each new commit.

Tell me your preference on (A)/(B) and I'll push it; (C) needs your re-run.

… in Chat mode Codex follow-up: with the dedup guard, a malformed tool call in Chat mode was represented as a successful CHAT_MODE_TOOL_SKIPPED_RESPONSE rather than the INVALID_PARAMS parse error, so the model couldn't tell the arguments were bad. The chat-skip now applies only to valid (Ok) tool calls; unparseable ones fall through to the parse-error branch. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Goose <goose@aaif.dev>

lunchboxfortwo · 2026-06-22T15:04:41Z

Went ahead and handled (B) from the summary — pushed 8ba8df2: the Chat-mode skip now applies only to valid tool calls, so an unparseable call surfaces the INVALID_PARAMS parse error instead of a false "skipped OK" (addresses codex's latest P2). Verified your test_unparseable_tool_call_feeds_back_and_continues still passes; fmt/clippy clean.

That leaves just (A) (keep vs. revert the per-formatter placeholders — I left them as defense-in-depth, no action needed unless you want them gone) and a re-run to clear the sqlx flake. Otherwise it's ready for your review + merge.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8ba8df284e

ℹ️ 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".

chatgpt-codex-connector · 2026-06-22T15:10:22Z

+                                    let history_tool_call = match &request.tool_call {
+                                        Ok(_) => request.tool_call.clone(),
+                                        Err(_) => Ok(CallToolRequestParams::new(
+                                            "unparseable_tool_call",
+                                        )
+                                        .with_arguments(serde_json::Map::new())),


Preserve the original tool name for retries

When a provider returns an unparseable argument payload for a valid tool, this stores the history as a call to unparseable_tool_call with {} and the paired error only mentions the id/raw args, so the next model turn no longer sees which real tool (for example developer__shell or search) failed. This matters especially when a response had multiple possible tools or the user request is ambiguous: the retry feedback can no longer direct the model to correct the original tool call. Preserve the original function name in the stored placeholder or at least include it in the error metadata/message before replacing the call.

Useful? React with 👍 / 👎.

When a tool call has non-object arguments, the agent loop replaces the call with a generic unparseable_tool_call placeholder, so the retry feedback relied entirely on the paired tool-response error to tell the model what failed. That message only mentioned the id and raw args, not the real tool, so on an ambiguous turn the model couldn't tell which tool to correct. Include the original function name in the OpenAI (streaming + non-streaming) and Databricks non-object error messages.

DOsinga · 2026-06-22T16:00:28Z

Addressed the P2 (preserve the original tool name for retries) in dd5a6af.

The single-source placeholder approach in the agent loop is the right call, but it does mean the retry feedback can no longer name the failed tool from the placeholder itself. Rather than thread the original name through the agent loop (which only sees ToolRequest(Err)), I put the name where the model actually reads it on retry: the paired tool-response error. The OpenAI (streaming + non-streaming) and Databricks non-object error messages now read Tool arguments for <name> (id <id>) must be a JSON object... instead of just the id. Tests updated to assert the real tool name appears in the message.

Resolves conflicts with main's tool-argument parsing refactor: - openai/databricks now use parse_tool_arguments(); kept the non-object guard (is_object) so bare array/string/number arguments surface a tool error instead of panicking in rmcp's object(). - agent loop: kept the placeholder-and-continue approach over main's exit_chat bail-out, preserving main's response_thinking handling. - isolated test_unparseable_tool_call_feeds_back_and_continues on a TempDir-backed SessionManager and updated its mock provider to the new ProviderDescriptor/ProviderDef/ModelConfig API.

DOsinga

Merged main and resolved the conflicts:

main refactored tool-argument parsing into a shared parse_tool_arguments() helper. That helper still returns Some for non-object JSON, so the panic this PR fixes is still present on main — I kept the is_object() guard on the Some arm in both the OpenAI and Databricks decoders (with a small describe_json_value helper to dedup the error message).
In the agent loop I kept this PR's placeholder-and-continue approach over main's new exit_chat-on-parse-error bail-out, while preserving main's response_thinking (Kimi/DeepSeek) handling.
Fixed the flaky test_unparseable_tool_call_feeds_back_and_continues: it was using the default global session manager and racing other parallel tests on the shared sqlite DB. It now uses a TempDir-isolated SessionManager via Agent::with_config, matching the pattern used by the other agent tests. Updated the mock provider to main's new ProviderDescriptor/ProviderDef/ModelConfig API.

Verified locally: full cargo test --test agent passes (stable over repeated runs), openai/databricks/bedrock regression tests pass, fmt + clippy clean. The remaining codex comments are stale re-anchors of findings already resolved on earlier commits.

Approving — letting CI confirm green on the merge commit. Thanks for the careful repro work on this one.

* origin/main: (28 commits) docs: name the message field in the hooks payload guide (aaif-goose#9913) fix(cli): save /edit prompts to history (aaif-goose#10011) feat(i18n): add fr, de, it, pt, id, ms, vi, zh-TW desktop locales (aaif-goose#10072) feat (acp+): Use ACP permission manager for tool permissions (aaif-goose#10066) feat (ui): Remove ACP chat feature flag and turn on chat using ACP (aaif-goose#10062) fix(schedule): use session.message_count for schedule sessions listing (aaif-goose#10026) feat(acp): migrate getDictationConfig and transcribeDictation to ACP (aaif-goose#10048) fix: sync the sesion store after session provider and model update and also update thinking effort (aaif-goose#10060) feat: handling platform event mcp notification for acp (aaif-goose#10038) feat(acp+): config in desktop on ACP+ (aaif-goose#10057) feat(acp+): models/providers in desktop on ACP+ (aaif-goose#9987) Support streamed tool calls without indexes (aaif-goose#10023) add ACP+ handlers for prompt editing (aaif-goose#10031) refactor: replace getTunnelStatus with GOOSE_DISABLE_NOSTR_SHARING config flag (aaif-goose#10044) chore: ignore Zed settings (aaif-goose#10043) fix: removed fallback rest api call in ACP mode for edit in place (aaif-goose#10034) feat(acp): migrate upsertPermissions to setToolPermissions ACP method (aaif-goose#9999) Apply ACP recipe state during session load and fork (aaif-goose#9998) feat: used acp agent capabilities for local Inference support check (aaif-goose#9996) move anthropic provider into goose-providers (aaif-goose#9985) ...

* origin/main: task(acp): upgrade SDK and use new HTTP/WS crate (#10082) fix(providers): reject non-object tool-call arguments instead of panicking (#9832) # Conflicts: # crates/goose/src/agents/agent.rs

* main: task(acp): upgrade SDK and use new HTTP/WS crate (#10082) fix(providers): reject non-object tool-call arguments instead of panicking (#9832) docs: name the message field in the hooks payload guide (#9913) fix(cli): save /edit prompts to history (#10011) feat(i18n): add fr, de, it, pt, id, ms, vi, zh-TW desktop locales (#10072) feat (acp+): Use ACP permission manager for tool permissions (#10066)

* main: chore: Remove legacy MCP-UI proxy support (#10086) Remove session_id from provider streaming trait methods (#9984) fix(cli): update help text for --session-id (#10077) task(acp): upgrade SDK and use new HTTP/WS crate (#10082) fix(providers): reject non-object tool-call arguments instead of panicking (#9832) docs: name the message field in the hooks payload guide (#9913) fix(cli): save /edit prompts to history (#10011) feat(i18n): add fr, de, it, pt, id, ms, vi, zh-TW desktop locales (#10072) feat (acp+): Use ACP permission manager for tool permissions (#10066) feat (ui): Remove ACP chat feature flag and turn on chat using ACP (#10062) fix(schedule): use session.message_count for schedule sessions listing (#10026) feat(acp): migrate getDictationConfig and transcribeDictation to ACP (#10048) fix: sync the sesion store after session provider and model update and also update thinking effort (#10060)

chatgpt-codex-connector Bot reviewed Jun 16, 2026

View reviewed changes

ci: re-trigger (linker SIGBUS while linking goose_server test binary …

2aa8189

…— infra flake, unrelated to the 2-file decoder change) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Lunchbox <lunchboxfortwo@gmail.com>

michaelneale assigned DOsinga Jun 17, 2026

lunchboxfortwo mentioned this pull request Jun 17, 2026

Feature: ground-truth completion gate — verify the agent's success claims against actual tool execution before a turn can end #9708

Closed

1 task

Douwe M Osinga added 2 commits June 18, 2026 08:34

Merge remote-tracking branch 'origin/main' into wt-goose-9832

c7a9d6f

DOsinga added the snoozed:20260621 label Jun 18, 2026

chatgpt-codex-connector Bot reviewed Jun 18, 2026

View reviewed changes

kimnamu reviewed Jun 20, 2026

View reviewed changes

chatgpt-codex-connector Bot reviewed Jun 20, 2026

View reviewed changes

Douwe M Osinga added 2 commits June 22, 2026 07:54

Merge remote-tracking branch 'origin/main' into fix/tool-args-non-object

318d489

DOsinga added snoozed:20260625 and removed snoozed:20260621 labels Jun 22, 2026

chatgpt-codex-connector Bot reviewed Jun 22, 2026

View reviewed changes

DOsinga approved these changes Jun 25, 2026

View reviewed changes

michaelneale enabled auto-merge June 29, 2026 04:07

michaelneale added this pull request to the merge queue Jun 29, 2026

Merged via the queue into aaif-goose:main with commit a1711e7 Jun 29, 2026
24 checks passed

This was referenced Jun 30, 2026

每日上游同步 2026-06-30 apecloud/apemind-agent#83

Closed

Sync upstream changes through 2026-06-30 #10102

Closed

Sync upstream changes through 2026-06-30 apecloud/apemind-agent#84

Merged

github-actions Bot mentioned this pull request Jun 30, 2026

chore(release): release version 1.40.0 #10118

Closed

BrewTestBot mentioned this pull request Jul 2, 2026

block-goose-cli 1.40.0 Homebrew/homebrew-core#290949

Merged

BrewTestBot mentioned this pull request Aug 12, 2026

block-goose-cli 1.46.0 Homebrew/homebrew-core#298471

Merged

Uh oh!

Conversation

lunchboxfortwo commented Jun 16, 2026

Summary

Testing

Related Issues

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Jun 16, 2026

Choose a reason for hiding this comment

Uh oh!

lunchboxfortwo commented Jun 16, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Jun 16, 2026

Choose a reason for hiding this comment

Uh oh!

michaelneale commented Jun 17, 2026

Uh oh!

DOsinga commented Jun 18, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Jun 18, 2026

Choose a reason for hiding this comment

Uh oh!

lunchboxfortwo commented Jun 18, 2026

Uh oh!

kimnamu left a comment

Choose a reason for hiding this comment

Uh oh!

lunchboxfortwo commented Jun 20, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Jun 20, 2026

Choose a reason for hiding this comment

Uh oh!

lunchboxfortwo commented Jun 20, 2026

Uh oh!

DOsinga commented Jun 22, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Jun 22, 2026

Choose a reason for hiding this comment

Uh oh!

lunchboxfortwo commented Jun 22, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Jun 22, 2026

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector Bot Jun 22, 2026

Choose a reason for hiding this comment

Uh oh!

lunchboxfortwo commented Jun 22, 2026

Uh oh!

lunchboxfortwo commented Jun 22, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Jun 22, 2026