Fix apps extension: coerce string arguments from inner LLM responses#8030
Merged
Fix apps extension: coerce string arguments from inner LLM responses#8030
Conversation
![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.
Reviewed commit: a0da2dd303
ℹ️ 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".
| Ok(event) => { | ||
| if event.seq <= replay_max_seq { continue; } | ||
| let json = serde_json::to_string(&event.event).unwrap_or_default(); | ||
| if tx.send(format!("data: {}\n\n", json)).await.is_err() { return; } |
There was a problem hiding this comment.
When the /reply SSE writer fails to send (for example, a client aborts the HTTP stream), this branch returns without canceling the request token, so the spawned agent task keeps running in the background and can continue consuming model/tool work after the user has disconnected. The old inline path canceled on send failure; this regression affects clients that rely on dropping the stream to stop work.
Useful? React with 👍 / 👎.
Comment on lines
+522
to
+524
| let (replay, replay_max_seq, mut live_rx) = match bus.subscribe(None).await { | ||
| Ok(result) => result, | ||
| Err(_) => return, |
There was a problem hiding this comment.
This subscribes with bus.subscribe(None) and replays all buffered session events before streaming live ones, but /reply is a single-request stream and does not filter by request_id. On subsequent calls in the same session, clients can receive stale Message/Finish/Error events from earlier requests before the new reply, which can corrupt output handling. Replay/live forwarding should be scoped to the generated request_id for this invocation.
Useful? React with 👍 / 👎.
When the apps extension calls an inner LLM to generate app content, the LLM sometimes returns numeric values as strings (e.g., "600" instead of 600 for window width). This caused serde deserialization to fail with 'invalid type: string "600", expected u32'. Normal tool calls already go through coerce_tool_arguments() in reply_parts.rs (via categorize_tool_requests), which handles this string-to-number coercion. The inner LLM tool calls in the apps extension bypassed this by deserializing directly from the raw response. Fix: reuse the existing coerce_tool_arguments() in extract_tool_response() so inner LLM responses get the same type coercion as normal tool calls. Closes #8028 Signed-off-by: Douwe Osinga <[email protected]>
DOsinga
force-pushed
the
fix/apps-tool-argument-coercion
branch
from
a0da2dd to
e499dc4
Compare
alexhancock
approved these changes
Mar 20, 2026
elijahsgh
pushed a commit
to elijahsgh/goose
that referenced
this pull request
Mar 21, 2026
…lock#8030) Signed-off-by: Douwe Osinga <[email protected]> Co-authored-by: Douwe Osinga <[email protected]> Signed-off-by: esnyder <[email protected]>
elijahsgh
pushed a commit
to elijahsgh/goose
that referenced
this pull request
Mar 21, 2026
…lock#8030) Signed-off-by: Douwe Osinga <[email protected]> Co-authored-by: Douwe Osinga <[email protected]> Signed-off-by: esnyder <[email protected]>
6 tasks
wpfleger96
added a commit
that referenced
this pull request
Mar 23, 2026
* origin/main: (62 commits) Tweak the release process: no more merge to main (#7994) fix: gemini models via databricks (#8042) feat(apps): Pass toolInfo to MCP Apps via hostContext (#7506) fix: remove configured marker when deleting oauth provider configuration (#7887) docs: add vmware-aiops MCP extension documentation (#8055) Show setup instructions for ACP providers in settings modal (#8065) deps: replace sigstore-verification with sigstore-verify to kill vulns (#8064) feat(acp): add session/set_config and stabilize list, delete and close (#7984) docs: Correct `gosoe` typo to `goose` (#8062) fix: use default provider and model when provider in session no longer exists (#8035) feat: add GOOSE_SHELL env var to configure preferred shell (#7909) fix(desktop): fullscreen header bar + always-visible close controls (#8033) docs: add Claude Code approve mode permission routing documentation (#7949) chatgpt_codex: Support reasoning and gpt-5.4 (#7941) refactor(anthropic): fix N+1 thinking message storage issue (#7958) fix: handle mid-stream error events in OpenAI SSE streaming (#8031) Fix apps extension: coerce string arguments from inner LLM responses (#8030) feat: ability to expand sidebar to see chats names (#7816) Fix config for GOOSE_MAX_BACKGROUND_TASKS (#7940) set MACOSX_DEPLOYMENT_TARGET=12.0 (#7947) ...
wpfleger96
added a commit
that referenced
this pull request
Mar 23, 2026
…pstream * wpfleger/socket-support: (62 commits) Tweak the release process: no more merge to main (#7994) fix: gemini models via databricks (#8042) feat(apps): Pass toolInfo to MCP Apps via hostContext (#7506) fix: remove configured marker when deleting oauth provider configuration (#7887) docs: add vmware-aiops MCP extension documentation (#8055) Show setup instructions for ACP providers in settings modal (#8065) deps: replace sigstore-verification with sigstore-verify to kill vulns (#8064) feat(acp): add session/set_config and stabilize list, delete and close (#7984) docs: Correct `gosoe` typo to `goose` (#8062) fix: use default provider and model when provider in session no longer exists (#8035) feat: add GOOSE_SHELL env var to configure preferred shell (#7909) fix(desktop): fullscreen header bar + always-visible close controls (#8033) docs: add Claude Code approve mode permission routing documentation (#7949) chatgpt_codex: Support reasoning and gpt-5.4 (#7941) refactor(anthropic): fix N+1 thinking message storage issue (#7958) fix: handle mid-stream error events in OpenAI SSE streaming (#8031) Fix apps extension: coerce string arguments from inner LLM responses (#8030) feat: ability to expand sidebar to see chats names (#7816) Fix config for GOOSE_MAX_BACKGROUND_TASKS (#7940) set MACOSX_DEPLOYMENT_TARGET=12.0 (#7947) ...
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
When the apps extension calls an inner LLM to generate app content (e.g.
create_app_content,update_app_content), the LLM sometimes returns numeric values as strings (e.g."600"instead of600for window width). This caused serde deserialization to fail with:Root Cause
Normal (outer) tool calls already go through
coerce_tool_arguments()inreply_parts.rsviacategorize_tool_requests, which coerces string values to their correct types based on the tool's JSON Schema. The inner LLM tool calls in the apps extension bypassed this —extract_tool_response()deserialized directly from the raw response without coercion.Fix
Reuse the existing
coerce_tool_arguments()inextract_tool_response()so inner LLM responses get the same type coercion as normal tool calls.Closes #8028