mirror of
https://github.com/fabro-sh/fabro.git
synced 2026-10-10 03:30:59 +00:00
fix(agent): apply the Kimi vocabulary to every tool, not just the early ones
Renaming ran as a pass at the end of profile construction, so it only covered tools registered by that point. Subagent tools arrive later via `register_subagent_tools`, and the skill tool is registered when a session discovers skills, so a Kimi profile actually exposed a mixed set: Read Write Edit Bash Grep Glob FetchURL TodoList renamed spawn_agent send_input close_agent wait use_skill missed Move the vocabulary into ToolRegistry instead of applying it as a pass. `register` renames built-ins on the way in, so registration order stops mattering and a late registration cannot slip through. `ToolRegistry::new` keeps the fabro vocabulary, so no other profile changes. `use_skill` now exposes as `Skill`, matching Kimi Code, which has the same semantics. The subagent tools stay under fabro's names on purpose: Kimi Code's `Agent` launches a subagent and returns its result, while fabro's spawn_agent returns a handle that send_input, wait, and close_agent drive. Borrowing the name without the semantics would promise a result the tool does not return -- the same mistake as exposing incremental task tools under a whole-list name. The skills prompt section hardcoded `use_skill`, which under this vocabulary names a tool the model was not given. It takes the exposed name now, threaded through EmbeddedPrompt so a profile's prompt and its registry cannot disagree. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
ddddc5bb33
commit
9604d2ac9d
7 changed files with 112 additions and 70 deletions
|
|
@ -12,6 +12,7 @@ use fabro_types::{
|
|||
};
|
||||
|
||||
use crate::memory::MemoryDocument;
|
||||
use crate::native_tool::NativeTool;
|
||||
use crate::skills::{Skill, format_skills_prompt_section};
|
||||
use crate::tool_registry::{ToolDefinitionWithSource, ToolSource};
|
||||
|
||||
|
|
@ -221,7 +222,7 @@ fn memory_prompt_suffix(memory: &[MemoryDocument]) -> String {
|
|||
}
|
||||
|
||||
fn skills_prompt_suffix(skills: &[Skill]) -> String {
|
||||
let section = format_skills_prompt_section(skills);
|
||||
let section = format_skills_prompt_section(skills, NativeTool::UseSkill.canonical_name());
|
||||
if section.is_empty() {
|
||||
String::new()
|
||||
} else {
|
||||
|
|
|
|||
|
|
@ -109,9 +109,16 @@ impl NativeTool {
|
|||
Self::Glob => "Glob",
|
||||
Self::WebSearch => "WebSearch",
|
||||
Self::WebFetch => "FetchURL",
|
||||
// Kimi Code has no counterpart with these semantics: its
|
||||
// TodoList replaces a whole list rather than mutating tasks,
|
||||
// and it has no equivalent of the remaining tools.
|
||||
Self::UseSkill => "Skill",
|
||||
// Deliberately unmapped. Kimi Code's `Agent` launches a
|
||||
// subagent and returns its result; fabro's spawn_agent returns
|
||||
// a handle that send_input, wait, and close_agent then drive.
|
||||
// Borrowing the name without the semantics would promise a
|
||||
// result the tool does not return -- the same mistake as
|
||||
// exposing incremental task tools under a whole-list name.
|
||||
Self::SpawnAgent | Self::SendInput | Self::Wait | Self::CloseAgent => {
|
||||
self.canonical_name()
|
||||
}
|
||||
other => other.canonical_name(),
|
||||
},
|
||||
}
|
||||
|
|
|
|||
|
|
@ -1,8 +1,6 @@
|
|||
use std::sync::Arc;
|
||||
|
||||
use fabro_llm::types::ToolDefinition;
|
||||
use fabro_model::{AgentProfileKind, Catalog, ProviderId};
|
||||
use strum::VariantArray;
|
||||
|
||||
use super::EnvContext;
|
||||
use crate::agent_profile::AgentProfile;
|
||||
|
|
@ -13,7 +11,7 @@ use crate::sandbox::Sandbox;
|
|||
use crate::skills::Skill;
|
||||
use crate::todo_runtime::TodoRuntime;
|
||||
use crate::todo_tools::make_todo_list_tool;
|
||||
use crate::tool_registry::{RegisteredTool, ToolRegistry};
|
||||
use crate::tool_registry::ToolRegistry;
|
||||
use crate::tools::{WebFetchSummarizer, make_edit_file_tool, register_core_tools};
|
||||
|
||||
const CORE_PROMPT: &str = include_str!("prompts/kimi.md.j2");
|
||||
|
|
@ -39,48 +37,6 @@ files that have not been read, and the call will fail. Prefer Edit for any incre
|
|||
change: Write replaces the entire file, so using it to make a small edit discards \
|
||||
everything you did not restate.";
|
||||
|
||||
/// Expose every registered built-in tool under Kimi Code's names.
|
||||
///
|
||||
/// Only the exposed name changes: executors, schemas, and the identity that
|
||||
/// permissions and telemetry resolve through are untouched, because
|
||||
/// `canonical_tool_name` maps every vocabulary back to the same
|
||||
/// [`NativeTool`].
|
||||
fn apply_vocabulary(registry: &mut ToolRegistry, vocabulary: ToolVocabulary) {
|
||||
for tool in NativeTool::VARIANTS {
|
||||
let exposed = tool.name(vocabulary);
|
||||
if exposed == tool.canonical_name() {
|
||||
continue;
|
||||
}
|
||||
let Some(registered) = registry.unregister(tool.canonical_name()) else {
|
||||
continue;
|
||||
};
|
||||
registry.register(RegisteredTool {
|
||||
definition: ToolDefinition {
|
||||
name: exposed.to_string(),
|
||||
..registered.definition
|
||||
},
|
||||
..registered
|
||||
});
|
||||
}
|
||||
}
|
||||
|
||||
/// Replace a registered tool's description, keeping its executor and schema.
|
||||
///
|
||||
/// Profiles own their registries, so tailoring wording per model is a local
|
||||
/// change and does not affect what other profiles expose.
|
||||
fn redescribe(registry: &mut ToolRegistry, tool: NativeTool, description: &str) {
|
||||
let Some(tool) = registry.unregister(tool.canonical_name()) else {
|
||||
return;
|
||||
};
|
||||
registry.register(RegisteredTool {
|
||||
definition: ToolDefinition {
|
||||
description: description.to_string(),
|
||||
..tool.definition
|
||||
},
|
||||
..tool
|
||||
});
|
||||
}
|
||||
|
||||
pub struct KimiProfile {
|
||||
base: BaseProfile,
|
||||
}
|
||||
|
|
@ -97,12 +53,14 @@ impl KimiProfile {
|
|||
options: &NativeToolOptions,
|
||||
summarizer: Option<WebFetchSummarizer>,
|
||||
) -> Self {
|
||||
let mut registry = ToolRegistry::new();
|
||||
// The registry carries the vocabulary, so tools registered later
|
||||
// (subagent tools, skills) are renamed too.
|
||||
let mut registry = ToolRegistry::with_vocabulary(ToolVocabulary::KimiCode);
|
||||
|
||||
register_core_tools(&mut registry, options, summarizer);
|
||||
registry.register(make_edit_file_tool());
|
||||
redescribe(&mut registry, NativeTool::EditFile, EDIT_FILE_DESCRIPTION);
|
||||
redescribe(&mut registry, NativeTool::WriteFile, WRITE_FILE_DESCRIPTION);
|
||||
registry.redescribe(NativeTool::EditFile, EDIT_FILE_DESCRIPTION);
|
||||
registry.redescribe(NativeTool::WriteFile, WRITE_FILE_DESCRIPTION);
|
||||
|
||||
// Kimi Code drives todos with one replace-whole-list call. The
|
||||
// Anthropic task tools model the opposite interaction -- incremental
|
||||
|
|
@ -111,9 +69,6 @@ impl KimiProfile {
|
|||
let todo_runtime = Arc::new(TodoRuntime::new());
|
||||
registry.register(make_todo_list_tool(todo_runtime));
|
||||
|
||||
// Applied last so every built-in registered above is renamed.
|
||||
apply_vocabulary(&mut registry, ToolVocabulary::KimiCode);
|
||||
|
||||
Self {
|
||||
base: BaseProfile {
|
||||
profile_kind: AgentProfileKind::Kimi,
|
||||
|
|
@ -175,7 +130,8 @@ impl AgentProfile for KimiProfile {
|
|||
user_instructions: Option<&str>,
|
||||
skills: &[Skill],
|
||||
) -> String {
|
||||
let template = EmbeddedPrompt::new("kimi.md.j2", CORE_PROMPT);
|
||||
let template = EmbeddedPrompt::new("kimi.md.j2", CORE_PROMPT)
|
||||
.with_vocabulary(self.base.registry.vocabulary());
|
||||
|
||||
profiles::assemble_system_prompt(
|
||||
template,
|
||||
|
|
@ -194,6 +150,8 @@ mod tests {
|
|||
use fabro_types::AgentToolCategory;
|
||||
|
||||
use super::*;
|
||||
use crate::skills::make_use_skill_tool;
|
||||
use crate::subagent::{SessionFactory, SubAgentSupervisor};
|
||||
use crate::test_support::MockSandbox;
|
||||
use crate::tool_permissions::{known_tool_category, tool_category};
|
||||
|
||||
|
|
@ -278,10 +236,32 @@ mod tests {
|
|||
"{canonical} should have been renamed"
|
||||
);
|
||||
}
|
||||
// No Kimi Code counterpart, so these keep fabro's names.
|
||||
assert!(names.contains(&"TodoList".to_string()));
|
||||
}
|
||||
|
||||
/// Tools registered after the profile is constructed must also land in the
|
||||
/// Kimi vocabulary, or the model sees a mixed-case tool set.
|
||||
#[test]
|
||||
fn post_construction_tools_also_use_kimi_names() {
|
||||
let mut profile = KimiProfile::new("kimi-k3");
|
||||
let factory: SessionFactory = Arc::new(|| panic!("unused"));
|
||||
profile.register_subagent_tools(SubAgentSupervisor::new(3), factory, 0);
|
||||
profile
|
||||
.tool_registry_mut()
|
||||
.register(make_use_skill_tool(Arc::new(vec![Skill {
|
||||
name: "demo".into(),
|
||||
description: "d".into(),
|
||||
template: "t".into(),
|
||||
}])));
|
||||
|
||||
let names = profile.tool_registry().names();
|
||||
assert!(names.contains(&"Skill".to_string()), "got {names:?}");
|
||||
assert!(!names.contains(&"use_skill".to_string()), "got {names:?}");
|
||||
// Deliberately not renamed to Kimi Code's `Agent`: fabro's subagent
|
||||
// tools are a supervisor model, not a call-and-return one.
|
||||
assert!(names.contains(&"spawn_agent".to_string()), "got {names:?}");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn edit_and_write_descriptions_drill_read_before_write() {
|
||||
let profile = KimiProfile::new("kimi-k3");
|
||||
|
|
|
|||
|
|
@ -15,6 +15,7 @@ pub use openai::OpenAiProfile;
|
|||
|
||||
use crate::agent_profile::AgentProfile;
|
||||
use crate::config::{NativeToolOptions, ToolSecrets};
|
||||
use crate::native_tool::{NativeTool, ToolVocabulary};
|
||||
use crate::sandbox::Sandbox;
|
||||
use crate::skills::{Skill, format_skills_prompt_section};
|
||||
use crate::tool_registry::ToolRegistry;
|
||||
|
|
@ -125,9 +126,11 @@ pub struct EnvContext {
|
|||
/// The environment block is supplied by [`assemble_system_prompt`] and cannot
|
||||
/// be overridden by callers.
|
||||
pub struct EmbeddedPrompt {
|
||||
name: &'static str,
|
||||
source: &'static str,
|
||||
inputs: HashMap<String, toml::Value>,
|
||||
name: &'static str,
|
||||
source: &'static str,
|
||||
inputs: HashMap<String, toml::Value>,
|
||||
/// Vocabulary the surrounding prompt sections should name tools in.
|
||||
vocabulary: ToolVocabulary,
|
||||
}
|
||||
|
||||
impl EmbeddedPrompt {
|
||||
|
|
@ -137,9 +140,17 @@ impl EmbeddedPrompt {
|
|||
name,
|
||||
source,
|
||||
inputs: HashMap::new(),
|
||||
vocabulary: ToolVocabulary::Fabro,
|
||||
}
|
||||
}
|
||||
|
||||
/// Name tools in `vocabulary` in the generated sections.
|
||||
#[must_use]
|
||||
pub fn with_vocabulary(mut self, vocabulary: ToolVocabulary) -> Self {
|
||||
self.vocabulary = vocabulary;
|
||||
self
|
||||
}
|
||||
|
||||
#[must_use]
|
||||
pub fn with_string(mut self, name: &'static str, value: impl Into<String>) -> Self {
|
||||
self.inputs
|
||||
|
|
@ -184,6 +195,7 @@ pub fn assemble_system_prompt(
|
|||
skills: &[Skill],
|
||||
) -> String {
|
||||
let env_block = build_env_context_block_with(env, env_context);
|
||||
let skill_tool = NativeTool::UseSkill.name(template.vocabulary);
|
||||
let prompt = template.render(env_block);
|
||||
|
||||
let docs_section = if memory.is_empty() {
|
||||
|
|
@ -192,7 +204,7 @@ pub fn assemble_system_prompt(
|
|||
format!("\n\n{}", memory.join("\n\n"))
|
||||
};
|
||||
let skills_section = {
|
||||
let s = format_skills_prompt_section(skills);
|
||||
let s = format_skills_prompt_section(skills, skill_tool);
|
||||
if s.is_empty() {
|
||||
String::new()
|
||||
} else {
|
||||
|
|
|
|||
|
|
@ -196,16 +196,22 @@ pub fn make_use_skill_tool(skills: Arc<Vec<Skill>>) -> RegisteredTool {
|
|||
}
|
||||
}
|
||||
|
||||
pub fn format_skills_prompt_section(skills: &[Skill]) -> String {
|
||||
/// Render the skills section of a system prompt.
|
||||
///
|
||||
/// `skill_tool` is the name the skill tool is exposed under, which depends on
|
||||
/// the profile's vocabulary — telling a model to call a tool it was not given
|
||||
/// is worse than omitting the guidance.
|
||||
pub fn format_skills_prompt_section(skills: &[Skill], skill_tool: &str) -> String {
|
||||
if skills.is_empty() {
|
||||
return String::new();
|
||||
}
|
||||
|
||||
let mut lines = vec![
|
||||
"# Available Skills".to_string(),
|
||||
"When the user's request matches a skill below, call the `use_skill` tool \
|
||||
to load its instructions, then follow them."
|
||||
.to_string(),
|
||||
format!(
|
||||
"When the user's request matches a skill below, call the `{skill_tool}` tool \
|
||||
to load its instructions, then follow them."
|
||||
),
|
||||
];
|
||||
for skill in skills {
|
||||
if skill.description.is_empty() {
|
||||
|
|
@ -452,13 +458,13 @@ name: trimmed
|
|||
|
||||
#[test]
|
||||
fn format_empty() {
|
||||
assert_eq!(format_skills_prompt_section(&[]), "");
|
||||
assert_eq!(format_skills_prompt_section(&[], "use_skill"), "");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn format_lists_skills() {
|
||||
let skills = test_skills();
|
||||
let section = format_skills_prompt_section(&skills);
|
||||
let section = format_skills_prompt_section(&skills, "use_skill");
|
||||
assert!(section.contains("# Available Skills"));
|
||||
assert!(section.contains("call the `use_skill` tool"));
|
||||
assert!(section.contains("- `commit`: Create a commit"));
|
||||
|
|
|
|||
|
|
@ -15,6 +15,7 @@ use futures::stream;
|
|||
|
||||
use crate::agent_profile::AgentProfile;
|
||||
use crate::config::SessionOptions;
|
||||
use crate::native_tool::NativeTool;
|
||||
use crate::profiles::EnvContext;
|
||||
use crate::sandbox::*;
|
||||
use crate::session::Session;
|
||||
|
|
@ -80,7 +81,8 @@ impl AgentProfile for TestProfile {
|
|||
user_instructions: Option<&str>,
|
||||
skills: &[Skill],
|
||||
) -> String {
|
||||
let skills_section = format_skills_prompt_section(skills);
|
||||
let skills_section =
|
||||
format_skills_prompt_section(skills, NativeTool::UseSkill.canonical_name());
|
||||
let skills_part = if skills_section.is_empty() {
|
||||
String::new()
|
||||
} else {
|
||||
|
|
|
|||
|
|
@ -8,6 +8,7 @@ use fabro_types::{AgentToolCategory, AgentToolSource, AgentToolSummary};
|
|||
use tokio_util::sync::CancellationToken;
|
||||
|
||||
use crate::config::{ToolAccessPolicy, ToolExposureMode};
|
||||
use crate::native_tool::{NativeTool, ToolVocabulary};
|
||||
use crate::sandbox::Sandbox;
|
||||
use crate::session::ToolEnvProvider;
|
||||
use crate::tool_permissions;
|
||||
|
|
@ -125,21 +126,54 @@ fn agent_tool_source(source: &ToolSource) -> AgentToolSource {
|
|||
}
|
||||
|
||||
pub struct ToolRegistry {
|
||||
tools: HashMap<String, RegisteredTool>,
|
||||
tools: HashMap<String, RegisteredTool>,
|
||||
/// Naming scheme applied to built-in tools as they are registered.
|
||||
///
|
||||
/// Held by the registry rather than applied as a pass after construction,
|
||||
/// so tools registered later — subagent tools, skills — cannot miss it and
|
||||
/// leave the model with a mixed-vocabulary tool set.
|
||||
vocabulary: ToolVocabulary,
|
||||
}
|
||||
|
||||
impl ToolRegistry {
|
||||
#[must_use]
|
||||
pub fn new() -> Self {
|
||||
Self::with_vocabulary(ToolVocabulary::Fabro)
|
||||
}
|
||||
|
||||
/// A registry that exposes built-in tools under `vocabulary`.
|
||||
#[must_use]
|
||||
pub fn with_vocabulary(vocabulary: ToolVocabulary) -> Self {
|
||||
Self {
|
||||
tools: HashMap::new(),
|
||||
vocabulary,
|
||||
}
|
||||
}
|
||||
|
||||
pub fn register(&mut self, tool: RegisteredTool) {
|
||||
#[must_use]
|
||||
pub fn vocabulary(&self) -> ToolVocabulary {
|
||||
self.vocabulary
|
||||
}
|
||||
|
||||
pub fn register(&mut self, mut tool: RegisteredTool) {
|
||||
if let Some(native) = NativeTool::from_any_name(&tool.definition.name) {
|
||||
tool.definition.name = native.name(self.vocabulary).to_string();
|
||||
}
|
||||
self.tools.insert(tool.definition.name.clone(), tool);
|
||||
}
|
||||
|
||||
/// Replace a built-in tool's description, keeping its executor and schema.
|
||||
///
|
||||
/// Resolves through the registry's vocabulary, so callers name the tool by
|
||||
/// identity rather than by whatever string it is currently exposed under.
|
||||
pub fn redescribe(&mut self, tool: NativeTool, description: impl Into<String>) {
|
||||
let exposed = tool.name(self.vocabulary);
|
||||
if let Some(mut registered) = self.tools.remove(exposed) {
|
||||
registered.definition.description = description.into();
|
||||
self.tools.insert(exposed.to_string(), registered);
|
||||
}
|
||||
}
|
||||
|
||||
pub fn unregister(&mut self, name: &str) -> Option<RegisteredTool> {
|
||||
self.tools.remove(name)
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue