runtime: persist tool-call identity so restarts replay valid history (#5823) - #5840
Conversation
…5823) The live engine path persisted tool-call items without their identity: ToolCallStarted kept id/name/input only in the transient item.started event payload, and ToolCallComplete overwrote detail with the tool output and metadata with tool-output metadata. After a serve --http restart, reconstruct_messages_from_turns rebuilt assistant tool_calls with empty id/name and arguments "null", which strict OpenAI-compatible endpoints reject with 400 "missing field 'name'". Persist tool_use_id/tool_name/tool_input on the item snapshot at ToolCallStarted, carry them through ToolCallComplete, and mark the terminal result (tool_result_for/is_error) so one merged item rebuilds the paired tool_call/tool_result. Snapshots persisted before identity was durable now contribute nothing to rebuilt history instead of an empty tool_call shell. Tests: 11912 passed, 13 skipped (codewhale-tui lib, nextest ci profile). Fixes #5823
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_6f8eb76e-fa2d-465e-8e05-e8a13fe9e095) |
|
Claude finished @Hmbown's task in 3m 55s —— View job Review: runtime tool-call identity persistence (#5823)Summary: Solid, well-targeted fix. The root cause — Finding (see inline comment on Pre-existing, same-topic gap worth a follow-up (not in this diff): the Tests: The three new tests cover the important shapes well — completed paired call, in-flight/interrupted call, and legacy no-metadata skip — and the interrupted-call test's docstring correctly acknowledges reliance on Provider/model/route safety: N/A — this diff doesn't touch provider selection, model routing, or prefix inference. Reuse/simplification: Good — no new abstraction, reuses existing |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| let mut meta = match output.metadata { | ||
| Some(Value::Object(map)) => Value::Object(map), | ||
| _ => json!({}), | ||
| }; | ||
| if let Some(obj) = meta.as_object_mut() { | ||
| if let Some(started) = | ||
| item.metadata.as_ref().and_then(Value::as_object) | ||
| { | ||
| for key in ["tool_use_id", "tool_name", "tool_input"] { | ||
| if let Some(value) = started.get(key) { | ||
| obj.insert(key.to_string(), value.clone()); | ||
| } | ||
| } | ||
| } | ||
| obj.insert("tool_result_for".to_string(), json!(id)); | ||
| obj.insert("is_error".to_string(), json!(!output.success)); | ||
| } | ||
| item.metadata = Some(meta); |
There was a problem hiding this comment.
Good fix for the Ok branch, but note the Err(err) => branch a few lines below (crates/tui/src/runtime_threads.rs:8816-8821, outside this diff) has no equivalent: it never sets tool_result_for/is_error, so it keeps only the tool_use_id/tool_name/tool_input set at ToolCallStarted.
After a restart, reconstruct_messages_from_turns will replay that item as a ToolUse with no paired ToolResult. tool_history_repair::repair_tool_call_pairs_for_provider catches this before it hits the wire, but it synthesizes a generic "Tool call interrupted by process exit; terminal status: crashed_and_repaired." placeholder — discarding the real error text already captured in item.detail = Some(err.to_string()) and mislabeling a normal tool-execution failure as a process-crash repair.
Worth mirroring this same merge (tool_result_for: id, is_error: true) in the Err branch so the actual error survives replay.
There was a problem hiding this comment.
🟢 Approval recommended
The changes are localized, align with the reported root cause, and include targeted regression tests covering the key restart/replay scenarios.
Pull request overview
Fixes runtime thread restart behavior by persisting tool-call identity (id/name/input) in durable turn-item metadata, allowing history reconstruction to replay valid tool_call/tool_result messages for strict OpenAI-compatible providers (regression #5823).
Changes:
- Persist
tool_use_id,tool_name, and serializedtool_inputinTurnItemRecord.metadataatToolCallStarted. - On
ToolCallComplete, merge result metadata with the persisted identity and addtool_result_for/is_errormarkers (instead of overwriting metadata with output-only data). - Update
reconstruct_messages_from_turnsto rebuildToolUse/ToolResultblocks from metadata and skip legacy tool snapshots lacking durable identity; add regression tests covering completed, in-flight, and legacy cases.
File summaries
| File | Description |
|---|---|
| crates/tui/src/runtime_threads.rs | Persists tool-call identity in item metadata and reconstructs tool blocks from durable metadata on restart. |
| crates/tui/src/runtime_threads/tests.rs | Adds restart regression tests ensuring tool-call identity is preserved/replayed and legacy snapshots are safely skipped. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![codewhale-agent[bot]](https://avatars.githubusercontent.com/u/101357273?s=60&v=4){kind=small}
There was a problem hiding this comment.
Codewhale review
The PR persists tool-call identity at ToolCallStarted and merges it with result markers at ToolCallComplete so restart history rebuild can emit paired, non-empty tool_call/tool_result blocks. The reconstruction branch and regression tests cover the main completed-call and legacy-skip cases, but there are edge-case risks around parallel tool calls, non-object output metadata, and legacy in-progress items.
Findings
- [WARNING] Completed live tool-call items can split parallel tool calls into separate assistant/tool turns (
crates/tui/src/runtime_threads.rs:8357)
Each completed live tool-call item now carries both call identity andtool_result_for, and the result branch flushesassistant_blocksimmediately after each item. If one assistant turn issued multiple parallel tool calls, reconstruction will produce assistant(tool A) -> user(result A) -> assistant(tool B) -> user(result B) instead of a single assistant message containing all tool calls followed by their results. This changes the replayed history semantics and may be rejected by strict providers. - [WARNING] Completion merge discards non-object output metadata (
crates/tui/src/runtime_threads.rs:8794)
The newmatch output.metadata { Some(Value::Object(map)) => Value::Object(map), _ => json!({}) }resets any non-object metadata to an empty object. The previous code assignedoutput.metadata.clone()directly, preserving arbitraryserde_json::Valuemetadata. If a tool output can carry string/array/null metadata, that data is now lost at completion and may affect consumers ofitem.metadata. - [WARNING] Legacy in-progress item can still become a standalone ToolResult after upgrade (
crates/tui/src/runtime_threads.rs:8808)
For an item persisted before tool identity existed (item.metadataisNone), completion still insertstool_result_forandis_error. Reconstruction then sees a non-emptytool_result_forbut emptytool_use_id/tool_name, so it emits aToolResultwith no precedingToolUse. The PR explicitly plans to skip legacy items without durable identity, so the completion merge should avoid addingtool_result_forunless the start identity was copied. - [INFO] No test covers the actual ToolCallComplete metadata merge path (
crates/tui/src/runtime_threads/tests.rs)
The new regression tests constructTurnItemRecordvalues by hand and only exercisereconstruct_messages_from_turns. The completion branch that mergesoutput.metadatawith start-time tool identity and writestool_result_for/is_erroris not driven through the engine, so a bug in that merge (such as missing key copy, wrong id, or the non-object metadata handling) would not be caught.
Assessment
The core fix addresses #5823 for the common completed-call path, and the reconstruction tests are a solid start. Before merge, the parallel-tool-call grouping risk, non-object metadata preservation, and legacy in-progress completion behavior should be reviewed and preferably covered by tests.
Advisory review by Codewhale (codewhale review --pr 5840 --post, head 208ce7182d608244dc3b8cfc14d1ada858719124). Line-specific findings are also posted as inline review comments; mechanical fixes arrive as committable suggestions you can apply from the Files tab. CODEOWNERS approval still governs merge.
| }); | ||
| } | ||
| if !tool_result_for.is_empty() { | ||
| flush_assistant(&mut assistant_blocks, &mut messages); |
There was a problem hiding this comment.
[WARNING] Completed live tool-call items can split parallel tool calls into separate assistant/tool turns
Each completed live tool-call item now carries both call identity and tool_result_for, and the result branch flushes assistant_blocks immediately after each item. If one assistant turn issued multiple parallel tool calls, reconstruction will produce assistant(tool A) -> user(result A) -> assistant(tool B) -> user(result B) instead of a single assistant message containing all tool calls followed by their results. This changes the replayed history semantics and may be rejected by strict providers.
| // call identity persisted at start must be | ||
| // carried through metadata. Mark the | ||
| // terminal result too so restart history | ||
| // rebuild can re-emit the paired |
There was a problem hiding this comment.
[WARNING] Completion merge discards non-object output metadata
The new match output.metadata { Some(Value::Object(map)) => Value::Object(map), _ => json!({}) } resets any non-object metadata to an empty object. The previous code assigned output.metadata.clone() directly, preserving arbitrary serde_json::Value metadata. If a tool output can carry string/array/null metadata, that data is now lost at completion and may affect consumers of item.metadata.
| if let Some(value) = started.get(key) { | ||
| obj.insert(key.to_string(), value.clone()); | ||
| } | ||
| } |
There was a problem hiding this comment.
[WARNING] Legacy in-progress item can still become a standalone ToolResult after upgrade
For an item persisted before tool identity existed (item.metadata is None), completion still inserts tool_result_for and is_error. Reconstruction then sees a non-empty tool_result_for but empty tool_use_id/tool_name, so it emits a ToolResult with no preceding ToolUse. The PR explicitly plans to skip legacy items without durable identity, so the completion merge should avoid adding tool_result_for unless the start identity was copied.
Closes #5823.
Single-commit branch off current origin/main tip (66360de). Gates on the worktree: cargo fmt clean, check-versions OK (0.9.11 in sync; range advisory is pre-existing), dead-code budget PASS at 425. Clippy workspace + tui nextest to confirm via CI matrix.
Note
Medium Risk
Changes how tool turns are persisted and replayed into provider messages; mistakes could drop or mis-pair tool history on restart, but scope is localized to runtime thread reconstruction and event handling.
Overview
Fixes #5823: after a runtime restart, conversation history rebuilt from persisted turn items no longer emits empty
tool_callshells that strict OpenAI-compatible APIs reject.Persistence now records
tool_use_id,tool_name, andtool_inputin item metadata when a tool call starts (sincedetailis overwritten with output on completion). On completion, metadata merges those fields withtool_result_forandis_errorinstead of replacing them with output-only metadata.History rebuild (
reconstruct_messages_from_turns) treats call and result as separate signals on one or two items: it restoresToolUseonly when id and name are present,ToolResultwhentool_result_foris set, and skips legacy snapshots with no durable identity rather than replaying null arguments.Regression tests cover completed calls, in-flight calls after restart, and legacy items without metadata.
Reviewed by Cursor Bugbot for commit 208ce71. Bugbot is set up for automated code reviews on this repo. Configure here.