Skip to content

fix chat help #1830

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

Merged
merged 6 commits into from
May 14, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions crates/chat-cli/src/cli/chat/cli.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,9 @@ pub struct Chat {
/// prompt requests permissions to use a tool, unless --trust-all-tools is also used.
#[arg(long)]
pub no_interactive: bool,
/// Start a new conversation and overwrites any previous conversation from this directory.
#[arg(long)]
pub new: bool,
/// Resumes the previous conversation from this directory.
#[arg(short, long)]
pub resume: bool,
/// The first question to ask
pub input: Option<String>,
/// Context profile to use
Expand Down
12 changes: 6 additions & 6 deletions crates/chat-cli/src/cli/chat/command.rs
Original file line number Diff line number Diff line change
Expand Up @@ -51,10 +51,10 @@ pub enum Command {
subcommand: Option<PromptsSubcommand>,
},
Usage,
Import {
Load {
path: String,
},
Export {
Save {
path: String,
force: bool,
},
Expand Down Expand Up @@ -818,15 +818,15 @@ impl Command {
}
},
"usage" => Self::Usage,
"import" => {
"load" => {
let Some(path) = parts.get(1) else {
return Err("path is required".to_string());
};
Self::Import {
Self::Load {
path: (*path).to_string(),
}
},
"export" => {
"save" => {
let force = parts.contains(&"-f") || parts.contains(&"--force");
let Some(path) = parts.get(1) else {
return Err("path is required".to_string());
Expand All @@ -835,7 +835,7 @@ impl Command {
if !path.ends_with(".json") {
path.push_str(".json");
}
Self::Export { path, force }
Self::Save { path, force }
},
unknown_command => {
let looks_like_path = {
Expand Down
149 changes: 48 additions & 101 deletions crates/chat-cli/src/cli/chat/conversation_state.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
use std::collections::{
HashMap,
HashSet,
VecDeque,
};
use std::sync::Arc;
Expand Down Expand Up @@ -32,7 +33,6 @@ use super::hooks::{
};
use super::message::{
AssistantMessage,
AssistantToolUse,
ToolUseResult,
ToolUseResultBlock,
UserMessage,
Expand Down Expand Up @@ -60,7 +60,6 @@ use crate::api_client::model::{
ToolInputSchema,
ToolResult,
ToolResultContentBlock,
ToolResultStatus,
ToolSpecification,
ToolUse,
UserInputMessage,
Expand Down Expand Up @@ -347,7 +346,7 @@ impl ConversationState {
}
}

self.enforce_tool_use_history_invariants(true);
self.enforce_tool_use_history_invariants();
}

/// Here we also need to make sure that the tool result corresponds to one of the tools
Expand All @@ -362,105 +361,52 @@ impl ConversationState {
/// intervention here is to substitute the ambiguous, partial name with a dummy.
/// 3. The model had decided to call a tool that does not exist. The intervention here is to
/// substitute the non-existent tool name with a dummy.
pub fn enforce_tool_use_history_invariants(&mut self, last_only: bool) {
let tool_name_list = self.tool_manager.tn_map.keys().map(String::as_str).collect::<Vec<_>>();
// We need to first determine what the range of interest is. There are two places where we
// would call this function:
// 1. When there are changes to the list of available tools, in which case we comb through the
// entire conversation
// 2. When we send a message, in which case we only examine the most recent entry
let (tool_use_results, mut tool_uses) = if last_only {
if let (Some((_, AssistantMessage::ToolUse { ref mut tool_uses, .. })), Some(user_msg)) = (
self.history
.range_mut(self.valid_history_range.0..self.valid_history_range.1)
.last(),
&mut self.next_message,
) {
let tool_use_results = user_msg
.tool_use_results()
.map_or(Vec::new(), |results| results.iter().collect::<Vec<_>>());
let tool_uses = tool_uses.iter_mut().collect::<Vec<_>>();
(tool_use_results, tool_uses)
} else {
(Vec::new(), Vec::new())
}
} else {
let tool_use_results = self.next_message.as_ref().map_or(Vec::new(), |user_msg| {
user_msg
.tool_use_results()
.map_or(Vec::new(), |results| results.iter().collect::<Vec<_>>())
});
self.history
.iter_mut()
.filter_map(|(user_msg, asst_msg)| {
if let (Some(tool_use_results), AssistantMessage::ToolUse { ref mut tool_uses, .. }) =
(user_msg.tool_use_results(), asst_msg)
{
Some((tool_use_results, tool_uses))
} else {
None
}
pub fn enforce_tool_use_history_invariants(&mut self) {
let tool_names: HashSet<_> = self
.tools
.values()
.flat_map(|tools| {
tools.iter().map(|tool| match tool {
Tool::ToolSpecification(tool_specification) => tool_specification.name.as_str(),
})
.fold(
(tool_use_results, Vec::<&mut AssistantToolUse>::new()),
|(mut tool_use_results, mut tool_uses), (results, uses)| {
let mut results = results.iter().collect::<Vec<_>>();
let mut uses = uses.iter_mut().collect::<Vec<_>>();
tool_use_results.append(&mut results);
tool_uses.append(&mut uses);
(tool_use_results, tool_uses)
},
)
};
})
.filter(|name| *name != DUMMY_TOOL_NAME)
.collect();

for (_, assistant) in &mut self.history {
if let AssistantMessage::ToolUse { ref mut tool_uses, .. } = assistant {
for tool_use in tool_uses {
if tool_names.contains(tool_use.name.as_str()) {
continue;
}

// Replace tool uses associated with tools that does not exist / no longer exists with
// dummy (i.e. put them to sleep / dormant)
for result in tool_use_results {
let tool_use_id = result.tool_use_id.as_str();
let corresponding_tool_use = tool_uses.iter_mut().find(|tool_use| tool_use_id == tool_use.id);
if let Some(tool_use) = corresponding_tool_use {
if tool_name_list.contains(&tool_use.name.as_str()) {
// If this tool matches of the tools in our list, this is not our
// concern, error or not.
continue;
}
if let ToolResultStatus::Error = result.status {
// case 2 and 3
tool_use.name = DUMMY_TOOL_NAME.to_string();
tool_use.args = serde_json::json!({});
} else {
// case 1
let full_name = tool_name_list.iter().find(|name| name.ends_with(&tool_use.name));
// We should be able to find a match but if not we'll just treat it as
// a dummy and move on
if let Some(full_name) = full_name {
tool_use.name = (*full_name).to_string();
} else {
tool_use.name = DUMMY_TOOL_NAME.to_string();
tool_use.args = serde_json::json!({});
if tool_names.contains(tool_use.orig_name.as_str()) {
tool_use.name = tool_use.orig_name.clone();
tool_use.args = tool_use.orig_args.clone();
continue;
}
}
}
}

// Revive tools that were previously dormant if they now corresponds to one of the tools in
// our list of available tools. Note that this check only works because tn_map does NOT
// contain names of native tools.
for tool_use in tool_uses {
if tool_use.name == DUMMY_TOOL_NAME
&& tool_use
.orig_name
.as_ref()
.is_some_and(|name| tool_name_list.contains(&(*name).as_str()))
{
tool_use.name = tool_use
.orig_name
.as_ref()
.map_or(DUMMY_TOOL_NAME.to_string(), |name| name.clone());
tool_use.args = tool_use
.orig_args
.as_ref()
.map_or(serde_json::json!({}), |args| args.clone());
let names: Vec<&str> = tool_names
.iter()
.filter_map(|name| {
if name.ends_with(&tool_use.name) {
Some(*name)
} else {
None
}
})
.collect();

// There's only one tool use matching, so we can just replace it with the
// found name.
if names.len() == 1 {
tool_use.name = (*names.first().unwrap()).to_string();
continue;
}

// Otherwise, we have to replace it with a dummy.
tool_use.name = DUMMY_TOOL_NAME.to_string();
}
}
}
}
Expand Down Expand Up @@ -514,8 +460,8 @@ impl ConversationState {
.expect("unable to construct conversation state")
}

pub async fn update_state(&mut self) {
let needs_update = self.tool_manager.has_new_stuff.load(Ordering::Acquire);
pub async fn update_state(&mut self, force_update: bool) {
let needs_update = self.tool_manager.has_new_stuff.load(Ordering::Acquire) || force_update;
if !needs_update {
return;
}
Expand All @@ -540,12 +486,13 @@ impl ConversationState {
// We call this in [Self::enforce_conversation_invariants] as well. But we need to call it
// here as well because when it's being called in [Self::enforce_conversation_invariants]
// it is only checking the last entry.
self.enforce_tool_use_history_invariants(false);
self.enforce_tool_use_history_invariants();
}

/// Returns a conversation state representation which reflects the exact conversation to send
/// back to the model.
pub async fn backend_conversation_state(&mut self, run_hooks: bool, quiet: bool) -> BackendConversationState<'_> {
self.update_state(false).await;
self.enforce_conversation_invariants();

// Run hooks and add to conversation start and next user message.
Expand Down
4 changes: 2 additions & 2 deletions crates/chat-cli/src/cli/chat/message.rs
Original file line number Diff line number Diff line change
Expand Up @@ -349,11 +349,11 @@ pub struct AssistantToolUse {
/// The name for the tool as exposed to the model
pub name: String,
/// Original name of the tool
pub orig_name: Option<String>,
pub orig_name: String,
/// The input to pass to the tool as exposed to the model
pub args: serde_json::Value,
/// Original input passed to the tool
pub orig_args: Option<serde_json::Value>,
pub orig_args: serde_json::Value,
}

impl From<AssistantToolUse> for ToolUse {
Expand Down
Loading
Loading