-
Notifications
You must be signed in to change notification settings - Fork 6.1k
[exec] Add MCP tool arguments and results #5899
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
Conversation
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".
… option type throughout - Change structured_content in McpToolCallItemResult from JsonValue (default empty map) to Option<JsonValue> - Update event processing logic to preserve Option type - Update test to use structured_content: None for empty content - Adjust typescript SDK to use structured_content name and make it optional/unknown
…tion type; clean up unused import
codex-rs/exec/src/exec_events.rs
Outdated
| pub arguments: JsonValue, | ||
| #[serde(skip_serializing_if = "Option::is_none")] | ||
| pub result: Option<McpToolCallItemResult>, | ||
| #[serde(skip_serializing_if = "Option::is_none")] |
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 remove these annotations? I think it's worth dropping these going forward
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.
do we need to deserialize old McpToolCallItems? if so, we'd probably want to add #[serde(default)] so things don't break
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.
We do not need to deserialize. Removed.
…ExecutionItem and McpToolCallItem
Extends mcp_tool_call item to include arguments and results.