Repository navigation
fix(codex): project session McpToolCall into tool_called - #363
Oscar-Williams wants to merge 1 commit into
Conversation
Session JSONL records MCP tools as event_msg item_completed with a nested TurnItem type McpToolCall (PascalCase). Expand that item and map it through codexMCPToolName so tool_called sees mcp__<server>__<tool>. Leave custom_tool_call names like exec alone. Closes alibaba#359
|
|
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: 18bc36246c
ℹ️ 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".
| return []transcript.Message{ | ||
| *call, |
There was a problem hiding this comment.
For direct MCP invocations, Codex records both a response_item/function_call and an event_msg/item_completed/McpToolCall with the same call ID, as reproduced with codex-cli 0.163.0. parseCodexSessionResponseItem already emits the former, so unconditionally emitting another call here makes one invocation appear twice in Transcript.ToolCalls(). This inflated session transcript can also replace the correctly counted stdout transcript in buildSessionResult, exposing duplicated tool usage to script and agent judges. Reconcile the records by call ID while retaining the normalized MCP name and arguments.
Useful? React with 👍 / 👎.
| ToolResult: &transcript.ToolResultInfo{ | ||
| CallID: item.ID, | ||
| Status: codexMCPToolResultStatus(item.Status), | ||
| Content: codexMCPToolResultContent(item.Result), |
There was a problem hiding this comment.
When an MCP invocation fails at the RPC or transport level, Codex writes status: "failed" and error: {"message": ...} with no result, as reproduced with codex-cli 0.163.0. The new codexSessionTurnItem ignores error, so serializing only item.Result produces an empty ToolResult.Content and discards the failure diagnostic from this normalized result. Consumers inspecting the failed tool result therefore cannot determine its cause. Decode and forward the error when the result is absent; the added failed-status test only covers a non-null result.
Useful? React with 👍 / 👎.
Summary
Codex session JSONL records MCP tool use as
event_msgwithtype: item_completedand a nested TurnItem whosetypeisMcpToolCall(PascalCase, per openai/codex). Session parsing previously only handledagent_message/token_count, so those tools never appeared intool_called.This change expands the nested
itemon the session payload and projectsMcpToolCallthrough the existingcodexMCPToolNamehelper asmcp__<server>__<tool>, including the tool result andstatus: failedmapped to error.Related issues
Closes #359
Changes
event_msgitem_completednested TurnItem forMcpToolCall/mcpToolCall/mcp_tool_callmcp__<server>__<tool>with arguments and result statusstatus: failedsession shapes inTestParseCodexSessionFile*Test plan
TestParseCodexSessionFilewith issue bug(codex): tool_called misses MCP calls Codex makes from its code tool #359 session shapes (custom_tool_callname=exec +item_completed/McpToolCall); assertsmcp__shop__get_order, Arguments, and thatexecis not projected as a tool callTestParseCodexSessionFile_McpToolCallFailedStatusforstatus: failedgo test ./internal/agent/make test(includesgo test -race ./...)go vet ./...andrevivepass;make verify/golangci-lintstill fail on clean main with the same examples export data mismatch (reproduced at 99894b3)