fix(tui): render hydrated deferred-tool results as loaded, not run done
When a deferred tool is used for the first time, the engine returns a
schema-hydration result (event tool.schema_hydrated, executed=false,
retry_required=true) instead of executing the tool. The transcript and sidebar
previously rendered this as a completed run ("run done"), which was
indistinguishable from a real successful execution and misled both the user and
the model. Hydrated results now render as "tool loaded - retry required" via a
dedicated ToolStatus::Hydrated, threaded through tool routing, history, sidebar,
and theme. A successful real execution still renders as run done, a failed tool
with hydration metadata stays Failed.
Local correction on top of the PR: a hydrated row ranks with active work
(ToolStatus::Running) in the sidebar rather than alongside completed successes,
matching the "not run done" intent. The contributor's hydration detection and
missing-metadata handling are kept as-is (the sole emitter always sets
executed=false, consistent with the engine's own check).
Harvested from PR #2757 by @mvanhorn. Fixes #2648.
Co-authored-by: mvanhorn <455140+mvanhorn@users.noreply.github.com>
This commit is contained in:
@@ -182,6 +182,7 @@ impl Theme {
|
||||
match status {
|
||||
ToolStatus::Running => self.tool_running_accent,
|
||||
ToolStatus::Success => self.tool_success_accent,
|
||||
ToolStatus::Hydrated => self.tool_running_accent,
|
||||
ToolStatus::Failed => self.tool_failed_accent,
|
||||
}
|
||||
}
|
||||
@@ -278,6 +279,10 @@ mod tests {
|
||||
theme.tool_status_color(ToolStatus::Success),
|
||||
theme.tool_success_accent
|
||||
);
|
||||
assert_eq!(
|
||||
theme.tool_status_color(ToolStatus::Hydrated),
|
||||
theme.tool_running_accent
|
||||
);
|
||||
assert_eq!(
|
||||
theme.tool_status_color(ToolStatus::Failed),
|
||||
theme.tool_failed_accent
|
||||
|
||||
@@ -880,6 +880,7 @@ pub fn tool_run_summary(run: &ToolRun) -> String {
|
||||
pub enum ToolStatus {
|
||||
Running,
|
||||
Success,
|
||||
Hydrated,
|
||||
Failed,
|
||||
}
|
||||
|
||||
@@ -1001,8 +1002,16 @@ impl ExploringCell {
|
||||
.entries
|
||||
.iter()
|
||||
.all(|entry| entry.status != ToolStatus::Running);
|
||||
let any_hydrated = self
|
||||
.entries
|
||||
.iter()
|
||||
.any(|entry| entry.status == ToolStatus::Hydrated);
|
||||
let status = if all_done {
|
||||
ToolStatus::Success
|
||||
if any_hydrated {
|
||||
ToolStatus::Hydrated
|
||||
} else {
|
||||
ToolStatus::Success
|
||||
}
|
||||
} else {
|
||||
ToolStatus::Running
|
||||
};
|
||||
@@ -1010,7 +1019,11 @@ impl ExploringCell {
|
||||
lines.push(render_tool_header_with_summary(
|
||||
"Workspace",
|
||||
header_summary.as_deref(),
|
||||
if all_done { "done" } else { "running" },
|
||||
if all_done {
|
||||
tool_status_label(status)
|
||||
} else {
|
||||
"running"
|
||||
},
|
||||
status,
|
||||
None,
|
||||
low_motion,
|
||||
@@ -1020,6 +1033,7 @@ impl ExploringCell {
|
||||
let prefix = match entry.status {
|
||||
ToolStatus::Running => "live",
|
||||
ToolStatus::Success => "done",
|
||||
ToolStatus::Hydrated => "loaded",
|
||||
ToolStatus::Failed => "issue",
|
||||
};
|
||||
lines.extend(render_compact_kv(
|
||||
@@ -3161,7 +3175,7 @@ fn status_symbol(started_at: Option<Instant>, status: ToolStatus, low_motion: bo
|
||||
.map_or(0, |d| d % (TOOL_RUNNING_SYMBOLS.len() as u128));
|
||||
TOOL_RUNNING_SYMBOLS[usize::try_from(idx).unwrap_or_default()].to_string()
|
||||
}
|
||||
ToolStatus::Success => TOOL_DONE_SYMBOL.to_string(),
|
||||
ToolStatus::Success | ToolStatus::Hydrated => TOOL_DONE_SYMBOL.to_string(),
|
||||
ToolStatus::Failed => TOOL_FAILED_SYMBOL.to_string(),
|
||||
}
|
||||
}
|
||||
@@ -3452,6 +3466,7 @@ fn tool_status_label(status: ToolStatus) -> &'static str {
|
||||
match status {
|
||||
ToolStatus::Running => "running",
|
||||
ToolStatus::Success => "done",
|
||||
ToolStatus::Hydrated => "tool loaded - retry required",
|
||||
ToolStatus::Failed => "issue",
|
||||
}
|
||||
}
|
||||
|
||||
@@ -1651,7 +1651,9 @@ fn normalize_activity_text(text: &str) -> String {
|
||||
fn tool_row_rank(row: &SidebarToolRow) -> u8 {
|
||||
match row.status {
|
||||
ToolStatus::Failed => 0,
|
||||
ToolStatus::Running => 1,
|
||||
// A schema-hydrated deferred tool is not "run done" — it must be
|
||||
// retried — so it ranks with active work, not completed successes.
|
||||
ToolStatus::Running | ToolStatus::Hydrated => 1,
|
||||
ToolStatus::Success if is_low_value_tool(&row.name) => 3,
|
||||
ToolStatus::Success => 2,
|
||||
}
|
||||
@@ -1701,6 +1703,7 @@ fn tool_status_marker(
|
||||
match status {
|
||||
ToolStatus::Running => ("[~]", theme.warning),
|
||||
ToolStatus::Success => ("[✓]", theme.success),
|
||||
ToolStatus::Hydrated => ("[~]", theme.warning),
|
||||
ToolStatus::Failed => ("[!]", theme.error_fg),
|
||||
}
|
||||
}
|
||||
|
||||
@@ -469,10 +469,7 @@ pub(super) fn handle_tool_call_complete(
|
||||
app.cell_at_virtual_index_mut(cell_index)
|
||||
&& let Some(entry) = cell.entries.get_mut(entry_index)
|
||||
{
|
||||
entry.status = match result.as_ref() {
|
||||
Ok(tool_result) if tool_result.success => ToolStatus::Success,
|
||||
Ok(_) | Err(_) => ToolStatus::Failed,
|
||||
};
|
||||
entry.status = tool_status_from_result(result);
|
||||
app.mark_history_updated();
|
||||
// Mutating the in-flight exploring cell needs an active-cell
|
||||
// revision bump so the transcript cache invalidates the synthetic
|
||||
@@ -501,26 +498,7 @@ pub(super) fn handle_tool_call_complete(
|
||||
store_tool_detail_output(app, id, cell_index, result);
|
||||
let in_active = cell_index >= app.history.len();
|
||||
|
||||
let status = match result.as_ref() {
|
||||
Ok(tool_result) => match tool_result.metadata.as_ref() {
|
||||
Some(meta)
|
||||
if meta
|
||||
.get("status")
|
||||
.and_then(|v| v.as_str())
|
||||
.is_some_and(|s| s == "Running") =>
|
||||
{
|
||||
ToolStatus::Running
|
||||
}
|
||||
_ => {
|
||||
if tool_result.success {
|
||||
ToolStatus::Success
|
||||
} else {
|
||||
ToolStatus::Failed
|
||||
}
|
||||
}
|
||||
},
|
||||
Err(_) => ToolStatus::Failed,
|
||||
};
|
||||
let status = tool_status_from_result(result);
|
||||
|
||||
if let Some(cell) = app.cell_at_virtual_index_mut(cell_index) {
|
||||
match cell {
|
||||
@@ -610,7 +588,9 @@ pub(super) fn handle_tool_call_complete(
|
||||
match result.as_ref() {
|
||||
Ok(tool_result) => {
|
||||
let summary = summarize_mcp_output(&tool_result.content);
|
||||
if summary.is_error == Some(true) {
|
||||
if status == ToolStatus::Hydrated {
|
||||
mcp.status = status;
|
||||
} else if summary.is_error == Some(true) {
|
||||
mcp.status = ToolStatus::Failed;
|
||||
} else {
|
||||
mcp.status = status;
|
||||
@@ -764,16 +744,7 @@ fn push_orphan_tool_completion(
|
||||
name: &str,
|
||||
result: &Result<ToolResult, ToolError>,
|
||||
) {
|
||||
let status = match result.as_ref() {
|
||||
Ok(tool_result) => {
|
||||
if tool_result.success {
|
||||
ToolStatus::Success
|
||||
} else {
|
||||
ToolStatus::Failed
|
||||
}
|
||||
}
|
||||
Err(_) => ToolStatus::Failed,
|
||||
};
|
||||
let status = tool_status_from_result(result);
|
||||
let output = match result.as_ref() {
|
||||
Ok(tool_result) => Some(summarize_tool_output(&tool_result.content)),
|
||||
Err(err) => Some(err.to_string()),
|
||||
@@ -836,6 +807,47 @@ fn push_orphan_tool_completion(
|
||||
}
|
||||
}
|
||||
|
||||
fn tool_status_from_result(result: &Result<ToolResult, ToolError>) -> ToolStatus {
|
||||
match result.as_ref() {
|
||||
Ok(tool_result) if is_deferred_schema_hydration(tool_result) => ToolStatus::Hydrated,
|
||||
Ok(tool_result) => match tool_result.metadata.as_ref() {
|
||||
Some(meta)
|
||||
if meta
|
||||
.get("status")
|
||||
.and_then(|v| v.as_str())
|
||||
.is_some_and(|s| s == "Running") =>
|
||||
{
|
||||
ToolStatus::Running
|
||||
}
|
||||
_ => {
|
||||
if tool_result.success {
|
||||
ToolStatus::Success
|
||||
} else {
|
||||
ToolStatus::Failed
|
||||
}
|
||||
}
|
||||
},
|
||||
Err(_) => ToolStatus::Failed,
|
||||
}
|
||||
}
|
||||
|
||||
fn is_deferred_schema_hydration(tool_result: &ToolResult) -> bool {
|
||||
if !tool_result.success {
|
||||
return false;
|
||||
}
|
||||
let Some(metadata) = tool_result.metadata.as_ref() else {
|
||||
return false;
|
||||
};
|
||||
metadata
|
||||
.get("event")
|
||||
.and_then(serde_json::Value::as_str)
|
||||
.is_some_and(|event| event == "tool.schema_hydrated")
|
||||
&& metadata
|
||||
.get("executed")
|
||||
.and_then(serde_json::Value::as_bool)
|
||||
.is_some_and(|executed| !executed)
|
||||
}
|
||||
|
||||
fn is_exploring_tool(name: &str) -> bool {
|
||||
matches!(name, "read_file" | "list_dir" | "grep_files" | "list_files")
|
||||
}
|
||||
|
||||
@@ -8698,6 +8698,7 @@ fn activity_cell_rank(cell: &HistoryCell) -> Option<u8> {
|
||||
HistoryCell::Tool(tool) => match tool_status_for_activity(tool) {
|
||||
Some(ToolStatus::Running) => Some(0),
|
||||
Some(ToolStatus::Failed) => Some(1),
|
||||
Some(ToolStatus::Hydrated) => Some(2),
|
||||
Some(ToolStatus::Success) => Some(2),
|
||||
None => Some(2),
|
||||
},
|
||||
@@ -8929,6 +8930,12 @@ fn tool_status_for_activity(tool: &ToolCell) -> Option<ToolStatus> {
|
||||
.any(|entry| entry.status == ToolStatus::Failed)
|
||||
{
|
||||
Some(ToolStatus::Failed)
|
||||
} else if cell
|
||||
.entries
|
||||
.iter()
|
||||
.any(|entry| entry.status == ToolStatus::Hydrated)
|
||||
{
|
||||
Some(ToolStatus::Hydrated)
|
||||
} else {
|
||||
Some(ToolStatus::Success)
|
||||
}
|
||||
@@ -8964,6 +8971,7 @@ fn activity_status_label(status: ToolStatus) -> &'static str {
|
||||
match status {
|
||||
ToolStatus::Running => "running",
|
||||
ToolStatus::Success => "done",
|
||||
ToolStatus::Hydrated => "tool loaded - retry required",
|
||||
ToolStatus::Failed => "failed",
|
||||
}
|
||||
}
|
||||
|
||||
@@ -5529,6 +5529,174 @@ fn ok_result(
|
||||
Ok(crate::tools::spec::ToolResult::success(content))
|
||||
}
|
||||
|
||||
fn hydrated_result(
|
||||
content: &str,
|
||||
) -> Result<crate::tools::spec::ToolResult, crate::tools::spec::ToolError> {
|
||||
Ok(
|
||||
crate::tools::spec::ToolResult::success(content).with_metadata(serde_json::json!({
|
||||
"event": "tool.schema_hydrated",
|
||||
"tool": "exec_shell",
|
||||
"executed": false,
|
||||
"retry_required": true,
|
||||
"deferred_tool_loaded": true,
|
||||
"tool_name": "exec_shell",
|
||||
})),
|
||||
)
|
||||
}
|
||||
|
||||
fn rendered_text(lines: &[ratatui::text::Line<'_>]) -> String {
|
||||
lines
|
||||
.iter()
|
||||
.map(|line| {
|
||||
line.spans
|
||||
.iter()
|
||||
.map(|span| span.content.as_ref())
|
||||
.collect::<String>()
|
||||
})
|
||||
.collect::<Vec<_>>()
|
||||
.join("\n")
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn completed_exec_tool_result_still_renders_run_done() {
|
||||
let mut app = create_test_app();
|
||||
handle_tool_call_started(
|
||||
&mut app,
|
||||
"shell-ok",
|
||||
"exec_shell",
|
||||
&serde_json::json!({"command": "echo hi"}),
|
||||
);
|
||||
handle_tool_call_complete(&mut app, "shell-ok", "exec_shell", &ok_result("hi"));
|
||||
|
||||
let exec = app
|
||||
.active_cell
|
||||
.as_ref()
|
||||
.expect("active cell")
|
||||
.entries()
|
||||
.iter()
|
||||
.find_map(|cell| match cell {
|
||||
HistoryCell::Tool(ToolCell::Exec(exec)) => Some(exec),
|
||||
_ => None,
|
||||
})
|
||||
.expect("exec cell");
|
||||
|
||||
assert_eq!(exec.status, ToolStatus::Success);
|
||||
let text = rendered_text(&exec.lines_with_motion(100, true));
|
||||
assert!(text.contains("run done"), "{text}");
|
||||
assert!(!text.contains("tool loaded - retry required"), "{text}");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn hydrated_exec_tool_result_renders_retry_required_not_run_done() {
|
||||
let mut app = create_test_app();
|
||||
handle_tool_call_started(
|
||||
&mut app,
|
||||
"shell-hydrated",
|
||||
"exec_shell",
|
||||
&serde_json::json!({"command": "cargo test"}),
|
||||
);
|
||||
handle_tool_call_complete(
|
||||
&mut app,
|
||||
"shell-hydrated",
|
||||
"exec_shell",
|
||||
&hydrated_result(
|
||||
"Tool exec_shell was deferred and has now been loaded.\n\
|
||||
The tool was not executed. Retry with the loaded schema.",
|
||||
),
|
||||
);
|
||||
|
||||
let exec = app
|
||||
.active_cell
|
||||
.as_ref()
|
||||
.expect("active cell")
|
||||
.entries()
|
||||
.iter()
|
||||
.find_map(|cell| match cell {
|
||||
HistoryCell::Tool(ToolCell::Exec(exec)) => Some(exec),
|
||||
_ => None,
|
||||
})
|
||||
.expect("exec cell");
|
||||
|
||||
assert_eq!(exec.status, ToolStatus::Hydrated);
|
||||
let text = rendered_text(&exec.lines_with_motion(120, true));
|
||||
assert!(text.contains("run tool loaded - retry required"), "{text}");
|
||||
assert!(!text.contains("run done"), "{text}");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn hydrated_tool_with_validation_body_still_uses_hydrated_status() {
|
||||
let mut app = create_test_app();
|
||||
handle_tool_call_started(
|
||||
&mut app,
|
||||
"generic-hydrated",
|
||||
"deferred_tool",
|
||||
&serde_json::json!({"unexpected": true}),
|
||||
);
|
||||
handle_tool_call_complete(
|
||||
&mut app,
|
||||
"generic-hydrated",
|
||||
"deferred_tool",
|
||||
&hydrated_result(
|
||||
"Tool deferred_tool was deferred and has now been loaded.\n\n\
|
||||
Missing required fields:\n command\n\n\
|
||||
Unexpected fields:\n unexpected",
|
||||
),
|
||||
);
|
||||
|
||||
let generic = app
|
||||
.active_cell
|
||||
.as_ref()
|
||||
.expect("active cell")
|
||||
.entries()
|
||||
.iter()
|
||||
.find_map(|cell| match cell {
|
||||
HistoryCell::Tool(ToolCell::Generic(generic)) => Some(generic),
|
||||
_ => None,
|
||||
})
|
||||
.expect("generic cell");
|
||||
|
||||
assert_eq!(generic.status, ToolStatus::Hydrated);
|
||||
let text = rendered_text(&HistoryCell::Tool(ToolCell::Generic(generic.clone())).lines(120));
|
||||
assert!(text.contains("tool loaded - retry required"), "{text}");
|
||||
assert!(!text.contains("tool done"), "{text}");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn failed_tool_result_with_hydration_metadata_stays_failed() {
|
||||
let mut app = create_test_app();
|
||||
handle_tool_call_started(
|
||||
&mut app,
|
||||
"generic-failed",
|
||||
"deferred_tool",
|
||||
&serde_json::json!({}),
|
||||
);
|
||||
let result = Ok(crate::tools::spec::ToolResult::error("boom").with_metadata(
|
||||
serde_json::json!({
|
||||
"event": "tool.schema_hydrated",
|
||||
"executed": false,
|
||||
"retry_required": true,
|
||||
}),
|
||||
));
|
||||
handle_tool_call_complete(&mut app, "generic-failed", "deferred_tool", &result);
|
||||
|
||||
let generic = app
|
||||
.active_cell
|
||||
.as_ref()
|
||||
.expect("active cell")
|
||||
.entries()
|
||||
.iter()
|
||||
.find_map(|cell| match cell {
|
||||
HistoryCell::Tool(ToolCell::Generic(generic)) => Some(generic),
|
||||
_ => None,
|
||||
})
|
||||
.expect("generic cell");
|
||||
|
||||
assert_eq!(generic.status, ToolStatus::Failed);
|
||||
let text = rendered_text(&HistoryCell::Tool(ToolCell::Generic(generic.clone())).lines(120));
|
||||
assert!(text.contains("tool issue"), "{text}");
|
||||
assert!(!text.contains("tool loaded - retry required"), "{text}");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn shell_wait_without_command_uses_task_id_until_command_metadata_arrives() {
|
||||
let mut app = create_test_app();
|
||||
|
||||
Reference in New Issue
Block a user