Merge pull request #2325 from zlh124/fix/approval-missing-params
fix: approval dialog shows empty params when model response includes text block
This commit is contained in:
@@ -1698,6 +1698,7 @@ impl Engine {
|
||||
.send(Event::ApprovalRequired {
|
||||
id: tool_id.clone(),
|
||||
tool_name: tool_name.clone(),
|
||||
input: tool_input.clone(),
|
||||
description: plan.approval_description.clone(),
|
||||
approval_key,
|
||||
approval_grouping_key,
|
||||
|
||||
@@ -226,6 +226,9 @@ pub enum Event {
|
||||
id: String,
|
||||
tool_name: String,
|
||||
description: String,
|
||||
/// Tool parameters for approval display. Carried on the event so the
|
||||
/// TUI does not need to reconstruct them from `pending_tool_uses`.
|
||||
input: Value,
|
||||
/// Exact-argument fingerprint, used to scope *denials* (#1617).
|
||||
approval_key: String,
|
||||
/// Lossy / arity-aware fingerprint, used to scope *approvals* so an
|
||||
|
||||
@@ -4216,6 +4216,7 @@ mod tests {
|
||||
id: "tool_stale".to_string(),
|
||||
tool_name: "exec_command".to_string(),
|
||||
description: "stale approval".to_string(),
|
||||
input: serde_json::json!({}),
|
||||
})
|
||||
.await?;
|
||||
|
||||
@@ -4289,6 +4290,7 @@ mod tests {
|
||||
id: "tool_external_allow".to_string(),
|
||||
tool_name: "exec_command".to_string(),
|
||||
description: "external allow".to_string(),
|
||||
input: serde_json::json!({}),
|
||||
})
|
||||
.await?;
|
||||
|
||||
@@ -4366,6 +4368,7 @@ mod tests {
|
||||
id: "tool_external_deny".to_string(),
|
||||
tool_name: "exec_command".to_string(),
|
||||
description: "external deny".to_string(),
|
||||
input: serde_json::json!({}),
|
||||
})
|
||||
.await?;
|
||||
|
||||
@@ -4552,6 +4555,7 @@ mod tests {
|
||||
id: "tool_remember".to_string(),
|
||||
tool_name: "exec_command".to_string(),
|
||||
description: "remember=true".to_string(),
|
||||
input: serde_json::json!({}),
|
||||
})
|
||||
.await?;
|
||||
|
||||
|
||||
+21
-14
@@ -1926,6 +1926,7 @@ async fn run_event_loop(
|
||||
id,
|
||||
tool_name,
|
||||
description,
|
||||
input,
|
||||
approval_key,
|
||||
approval_grouping_key,
|
||||
} => {
|
||||
@@ -1970,19 +1971,10 @@ async fn run_event_loop(
|
||||
app.status_message =
|
||||
Some(format!("Blocked tool '{tool_name}' (approval_mode=never)"));
|
||||
} else {
|
||||
let tool_input = app
|
||||
.pending_tool_uses
|
||||
.iter()
|
||||
.find(|(tool_id, _, _)| tool_id == &id)
|
||||
.map(|(_, _, input)| input.clone())
|
||||
.unwrap_or_else(|| serde_json::json!({}));
|
||||
let tool_input = input;
|
||||
|
||||
if tool_name == "apply_patch" {
|
||||
maybe_add_patch_preview(app, &tool_input);
|
||||
}
|
||||
|
||||
// Create approval request and show overlay
|
||||
let request = ApprovalRequest::new(
|
||||
push_approval_request_view(
|
||||
app,
|
||||
&id,
|
||||
&tool_name,
|
||||
&description,
|
||||
@@ -1998,8 +1990,6 @@ async fn run_event_loop(
|
||||
"mode": app.mode.label(),
|
||||
}),
|
||||
);
|
||||
app.view_stack
|
||||
.push(ApprovalView::new_for_locale(request, app.ui_locale));
|
||||
if let Some((method, _, _)) =
|
||||
crate::tui::notifications::settings(config)
|
||||
{
|
||||
@@ -6597,6 +6587,23 @@ async fn handle_view_events(
|
||||
Ok(false)
|
||||
}
|
||||
|
||||
fn push_approval_request_view(
|
||||
app: &mut App,
|
||||
id: &str,
|
||||
tool_name: &str,
|
||||
description: &str,
|
||||
tool_input: &serde_json::Value,
|
||||
approval_key: &str,
|
||||
) {
|
||||
if tool_name == "apply_patch" {
|
||||
maybe_add_patch_preview(app, tool_input);
|
||||
}
|
||||
|
||||
let request = ApprovalRequest::new(id, tool_name, description, tool_input, approval_key);
|
||||
app.view_stack
|
||||
.push(ApprovalView::new_for_locale(request, app.ui_locale));
|
||||
}
|
||||
|
||||
struct ApprovalDecisionEvent {
|
||||
tool_id: String,
|
||||
tool_name: String,
|
||||
|
||||
@@ -5502,6 +5502,49 @@ fn message_complete_drain_preserves_thinking_when_thinking_complete_lost() {
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn approval_prompt_uses_event_input_after_message_complete_drain() {
|
||||
let mut app = create_test_app();
|
||||
app.pending_tool_uses.push((
|
||||
"tool-1".to_string(),
|
||||
"exec_shell".to_string(),
|
||||
serde_json::json!({"command": "stale value from drained list"}),
|
||||
));
|
||||
|
||||
// Mirror the old race: MessageComplete drains pending tool uses before
|
||||
// ApprovalRequired is handled. The approval modal must still show the
|
||||
// non-empty input carried directly on the ApprovalRequired event.
|
||||
app.pending_tool_uses.clear();
|
||||
|
||||
let event_input = serde_json::json!({
|
||||
"command": "cargo test -p codewhale-tui approval",
|
||||
"workdir": "/repo",
|
||||
});
|
||||
push_approval_request_view(
|
||||
&mut app,
|
||||
"tool-1",
|
||||
"exec_shell",
|
||||
"Run cargo tests",
|
||||
&event_input,
|
||||
"approval-key",
|
||||
);
|
||||
|
||||
let mut view = app.view_stack.pop().expect("approval view");
|
||||
let approval = view
|
||||
.as_any_mut()
|
||||
.downcast_mut::<ApprovalView>()
|
||||
.expect("approval view");
|
||||
let action = approval.handle_key(KeyEvent::new(KeyCode::Char('v'), KeyModifiers::NONE));
|
||||
let ViewAction::Emit(ViewEvent::OpenTextPager { content, .. }) = action else {
|
||||
panic!("expected approval params pager");
|
||||
};
|
||||
|
||||
assert!(content.contains("cargo test -p codewhale-tui approval"));
|
||||
assert!(content.contains("/repo"));
|
||||
assert!(!content.contains("stale value from drained list"));
|
||||
assert_ne!(content.trim(), "{}");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn second_thinking_block_appends_new_entry_in_same_active_cell() {
|
||||
// Real V4 turns can emit Thinking → Tool → Thinking → Tool before any
|
||||
|
||||
@@ -149,7 +149,10 @@ def default_strings(tui_config_rs: str) -> set[str]:
|
||||
|
||||
|
||||
def missing_default_strings(providers_md: str, defaults: set[str]) -> list[str]:
|
||||
code_spans = set(re.findall(r"`([^`]+)`", providers_md))
|
||||
# Inline-code validation should not let fenced TOML/bash examples pair a
|
||||
# stray backtick with later prose; strip fenced blocks before scanning.
|
||||
inline_source = re.sub(r"```.*?```", "", providers_md, flags=re.DOTALL)
|
||||
code_spans = set(re.findall(r"`([^`]+)`", inline_source))
|
||||
return sorted(defaults - code_spans)
|
||||
|
||||
|
||||
|
||||
Reference in New Issue
Block a user