mirror of
https://github.com/fabro-sh/fabro.git
synced 2026-08-28 05:27:41 +00:00
feat(config): add [run.agent] skill_dirs for workflow runs
Skill discovery for workflow and automation runs was fixed to the three convention directories. `SessionOptions.skill_dirs` existed in the agent SDK but was only settable by constructing a `Session` in Rust, so a repository keeping its skills elsewhere (`.agents/skills`, the Open Agent Skills convention) had no configuration-driven way to surface them. Add `[run.agent] skill_dirs`, resolved like every other run setting and carried through the run spec into each agent session. Configured directories are *added to* the conventions and searched last, so a skill defined there overrides a same-named convention skill; relative entries resolve against the repository root. Subagents inherit the directories. The additive semantics land in a new `SessionOptions.extra_skill_dirs` rather than reusing `skill_dirs`, which keeps its replace-the-defaults meaning for SDK callers and `fabro exec --skills-dir`. Also document that skill discovery follows a symlinked search root but does not recurse through symlinks below it, which is why symlinking `.fabro/skills/<name>` at a directory elsewhere in the repo never registered. Closes #746 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
parent
535e333970
commit
fa6aca9a1b
20 changed files with 478 additions and 11 deletions
|
|
@ -21,8 +21,28 @@ Within each directory, Fabro looks for `*/SKILL.md` files (one level of nesting)
|
|||
|
||||
When the same skill name appears in multiple directories, **later directories override earlier ones**. This means project-level skills take precedence over global skills, letting you customize behavior per-repo.
|
||||
|
||||
Fabro follows a symlinked search directory, but it does not recurse through symlinks below it. Symlinking `.fabro/skills/<name>` at a skill directory elsewhere in the repo therefore does not register that skill; add the real directory with `[run.agent] skill_dirs` instead.
|
||||
|
||||
### Extra directories for workflow runs
|
||||
|
||||
Repositories that keep skills somewhere else — `.agents/skills/` is a common choice — add those directories with `[run.agent] skill_dirs`:
|
||||
|
||||
```toml title="workflow.toml"
|
||||
[run.agent]
|
||||
skill_dirs = [".agents/skills"]
|
||||
```
|
||||
|
||||
The key layers like every other run setting, so it can live in `workflow.toml`, `.fabro/project.toml`, or `settings.toml`. Configured directories are **added to** the convention directories and searched after them, so a skill defined there overrides a same-named convention skill. Relative entries resolve against the repository root, which keeps them stable regardless of a stage's working directory; absolute entries are used verbatim. Agent stages and their subagents both see the extra directories.
|
||||
|
||||
To add a directory while keeping the ones a lower layer configured, splice them in with `"..."`:
|
||||
|
||||
```toml title="workflow.toml"
|
||||
[run.agent]
|
||||
skill_dirs = ["...", ".agents/skills"]
|
||||
```
|
||||
|
||||
<Note>
|
||||
You can override the default directories by setting `skill_dirs` in the session configuration. When set, only the specified directories are searched.
|
||||
The Rust SDK's `SessionOptions.skill_dirs` **replaces** the convention directories, and `fabro exec --skills-dir` does the same for a single ad-hoc session. `[run.agent] skill_dirs` is the additive, configuration-driven equivalent for workflow and automation runs.
|
||||
</Note>
|
||||
|
||||
## Invocation
|
||||
|
|
|
|||
|
|
@ -14804,10 +14804,17 @@ components:
|
|||
|
||||
RunAgentSettings:
|
||||
type: object
|
||||
required: [fabro_tools, mcps]
|
||||
required: [fabro_tools, skill_dirs, mcps]
|
||||
properties:
|
||||
fabro_tools:
|
||||
type: boolean
|
||||
skill_dirs:
|
||||
description: >-
|
||||
Extra skill discovery directories, searched after the convention
|
||||
directories. Relative entries resolve against the repository root.
|
||||
type: array
|
||||
items:
|
||||
type: string
|
||||
mcps:
|
||||
type: object
|
||||
additionalProperties:
|
||||
|
|
|
|||
|
|
@ -480,13 +480,21 @@ Configure workflow agent behavior that is not tied to a single stage.
|
|||
```toml title="run.toml"
|
||||
[run.agent]
|
||||
fabro_tools = true
|
||||
skill_dirs = [".agents/skills"]
|
||||
```
|
||||
|
||||
| Field | Description |
|
||||
|---|---|
|
||||
| `fabro_tools` | Allow the run's agents to use the Fabro run-management tools. Defaults to `false`. |
|
||||
| `skill_dirs` | Extra [skill](/agents/skills) discovery directories, added to the convention directories. Defaults to none. |
|
||||
|
||||
`fabro_tools` defaults to `false`. Set it to `true` only for runs whose agents should be able to use the same Fabro run-management MCP tool catalog exposed to human MCP clients: create, search, get, interact, gather, events, and pair.
|
||||
|
||||
One workflow-agent exception is intentional: `fabro_run_create` always creates [child runs](/execution/child-runs) parented to the current run. If an agent supplies `parent_id`, it must match the current run ID.
|
||||
|
||||
This setting is independent of `[run.agent.mcps]`, which configures external MCP servers available to the agent.
|
||||
`skill_dirs` is searched **after** the convention directories (`~/.fabro/skills`, `{git_root}/.fabro/skills`, `{git_root}/skills`), so a skill defined in a configured directory overrides a same-named convention skill. Relative entries resolve against the repository root; absolute entries are used verbatim. Agent stages and their subagents both see the extra directories. As with other list settings, a higher layer's list replaces a lower layer's unless it splices the inherited entries in with `"..."`.
|
||||
|
||||
These settings are independent of `[run.agent.mcps]`, which configures external MCP servers available to the agent.
|
||||
|
||||
### `[run.agent.mcps]`
|
||||
|
||||
|
|
|
|||
|
|
@ -436,17 +436,19 @@ enabled = true
|
|||
|
||||
## `[run.agent]`
|
||||
|
||||
`[run.agent]` — agent knobs only (Fabro tools and MCPs)
|
||||
`[run.agent]` — agent knobs only (Fabro tools, skill directories, and MCPs)
|
||||
|
||||
```toml title="settings.toml"
|
||||
[run.agent]
|
||||
fabro_tools = true
|
||||
skill_dirs = [".agents/skills"]
|
||||
```
|
||||
|
||||
| Key | Type / values | Default | Description |
|
||||
|---|---|---|---|
|
||||
| `fabro_tools` | boolean | false | Allow workflow agents to use Fabro run-management tools. |
|
||||
| `mcps` | table | None | Agent-scoped MCP server entries, keyed by name. |
|
||||
| `skill_dirs` | array | None | Extra skill discovery directories for workflow agents, searched after<br />the convention directories (`~/.fabro/skills`,<br />`{git_root}/.fabro/skills`, `{git_root}/skills`), so a skill defined<br />here overrides a same-named convention skill. Relative entries resolve<br />against the repository root. Splice marker supported at layering time. |
|
||||
|
||||
## `[run.agent.mcps.<name>]`
|
||||
|
||||
|
|
|
|||
|
|
@ -925,7 +925,8 @@ fn attach_json_errors_without_prompting_for_human_input() {
|
|||
"run": {
|
||||
"agent": {
|
||||
"fabro_tools": false,
|
||||
"mcps": {}
|
||||
"mcps": {},
|
||||
"skill_dirs": []
|
||||
},
|
||||
"artifacts": {
|
||||
"include": []
|
||||
|
|
|
|||
|
|
@ -193,6 +193,7 @@ fn inspect_resolves_selector_via_server_endpoint() {
|
|||
},
|
||||
"agent": {
|
||||
"fabro_tools": false,
|
||||
"skill_dirs": [],
|
||||
"mcps": {}
|
||||
},
|
||||
"hooks": [],
|
||||
|
|
|
|||
|
|
@ -187,6 +187,14 @@ pub struct SessionOptions {
|
|||
/// Skill directories. `None` = use convention defaults, `Some(dirs)` = use
|
||||
/// these instead.
|
||||
pub skill_dirs: Option<Vec<String>>,
|
||||
/// Extra skill directories appended to `skill_dirs` (or, when that is
|
||||
/// `None`, to the convention defaults). Searched last, so a skill defined
|
||||
/// here wins over a same-named skill from an earlier directory. Relative
|
||||
/// entries resolve against the git root, falling back to the sandbox
|
||||
/// working directory. This is what `[run.agent] skill_dirs` configures for
|
||||
/// workflow and automation runs, which need to *add* a discovery directory
|
||||
/// without restating the conventions.
|
||||
pub extra_skill_dirs: Vec<String>,
|
||||
/// MCP server configurations to connect to on session startup.
|
||||
pub mcp_servers: Vec<McpServerSettings>,
|
||||
/// Wall-clock timeout for the entire `process_input` call.
|
||||
|
|
@ -224,6 +232,7 @@ impl std::fmt::Debug for SessionOptions {
|
|||
)
|
||||
.field("compaction_preserve_turns", &self.compaction_preserve_turns)
|
||||
.field("skill_dirs", &self.skill_dirs)
|
||||
.field("extra_skill_dirs", &self.extra_skill_dirs)
|
||||
.field("mcp_servers", &self.mcp_servers.len())
|
||||
.field("wall_clock_timeout", &self.wall_clock_timeout)
|
||||
.finish()
|
||||
|
|
@ -251,6 +260,7 @@ impl Default for SessionOptions {
|
|||
compaction_threshold_percent: 80,
|
||||
compaction_preserve_turns: 6,
|
||||
skill_dirs: None,
|
||||
extra_skill_dirs: Vec::new(),
|
||||
mcp_servers: Vec::new(),
|
||||
wall_clock_timeout: None,
|
||||
}
|
||||
|
|
|
|||
|
|
@ -44,7 +44,7 @@ use crate::profiles::EnvContext;
|
|||
use crate::question_tools::AgentToolRuntime;
|
||||
use crate::sandbox::Sandbox;
|
||||
use crate::skills::{
|
||||
ExpandedInput, Skill, default_skill_dirs, discover_skills, expand_skill,
|
||||
ExpandedInput, Skill, default_skill_dirs, discover_skills, expand_skill, extend_skill_dirs,
|
||||
make_use_skill_tool_for_vocabulary,
|
||||
};
|
||||
use crate::subagent::{SubAgentCallbackEvent, SubAgentEventCallback, SubAgentSupervisor};
|
||||
|
|
@ -655,13 +655,14 @@ impl Session {
|
|||
});
|
||||
|
||||
// Discover skills
|
||||
let skill_dirs = if let Some(dirs) = &self.config.skill_dirs {
|
||||
let mut skill_dirs = if let Some(dirs) = &self.config.skill_dirs {
|
||||
dirs.clone()
|
||||
} else {
|
||||
let skills_dir = fabro_util::Home::from_env().skills_dir();
|
||||
let skills_str = skills_dir.to_string_lossy().to_string();
|
||||
default_skill_dirs(Some(&skills_str), Some(&doc_root))
|
||||
};
|
||||
extend_skill_dirs(&mut skill_dirs, &self.config.extra_skill_dirs, &doc_root);
|
||||
self.skills = discover_skills(self.sandbox.as_ref(), &skill_dirs, &cancel_token).await?;
|
||||
debug!(skill_count = self.skills.len(), "Skills discovered");
|
||||
|
||||
|
|
@ -5970,6 +5971,75 @@ mod tests {
|
|||
assert_eq!(skills[0].description, "Make a commit");
|
||||
}
|
||||
|
||||
/// `extra_skill_dirs` (what `[run.agent] skill_dirs` feeds) adds to the
|
||||
/// convention directories instead of replacing them, resolves a relative
|
||||
/// entry against the git root, and is searched last so its skills win.
|
||||
#[tokio::test]
|
||||
async fn initialize_appends_extra_skill_dirs_to_the_convention_dirs() {
|
||||
let sandbox = Arc::new(MockSandbox::linux());
|
||||
let config = SessionOptions {
|
||||
git_root: Some("/home/test".into()),
|
||||
extra_skill_dirs: vec![".agents/skills".into(), "/opt/shared/skills".into()],
|
||||
..Default::default()
|
||||
};
|
||||
let mut session = build_initialized_session(sandbox, config).await;
|
||||
let mut rx = session.subscribe();
|
||||
session.initialize().await.unwrap();
|
||||
|
||||
let mut source_dirs = None;
|
||||
while let Ok(envelope) = rx.try_recv() {
|
||||
if let AgentEvent::SkillsDiscovered {
|
||||
source_dirs: dirs, ..
|
||||
} = envelope.event
|
||||
{
|
||||
source_dirs = Some(dirs);
|
||||
break;
|
||||
}
|
||||
}
|
||||
let source_dirs = source_dirs.expect("SkillsDiscovered must be emitted");
|
||||
assert_eq!(&source_dirs[source_dirs.len() - 4..], [
|
||||
"/home/test/.fabro/skills".to_string(),
|
||||
"/home/test/skills".to_string(),
|
||||
"/home/test/.agents/skills".to_string(),
|
||||
"/opt/shared/skills".to_string(),
|
||||
]);
|
||||
}
|
||||
|
||||
/// An explicit `skill_dirs` override still gets the extra directories
|
||||
/// appended, so the two knobs compose rather than shadowing each other.
|
||||
#[tokio::test]
|
||||
async fn initialize_appends_extra_skill_dirs_to_an_explicit_override() {
|
||||
let sandbox = Arc::new(MockSandbox::linux());
|
||||
let config = SessionOptions {
|
||||
git_root: Some("/home/test".into()),
|
||||
skill_dirs: Some(vec!["/skills".into()]),
|
||||
extra_skill_dirs: vec!["/skills".into(), ".agents/skills".into()],
|
||||
..Default::default()
|
||||
};
|
||||
let mut session = build_initialized_session(sandbox, config).await;
|
||||
let mut rx = session.subscribe();
|
||||
session.initialize().await.unwrap();
|
||||
|
||||
let mut source_dirs = None;
|
||||
while let Ok(envelope) = rx.try_recv() {
|
||||
if let AgentEvent::SkillsDiscovered {
|
||||
source_dirs: dirs, ..
|
||||
} = envelope.event
|
||||
{
|
||||
source_dirs = Some(dirs);
|
||||
break;
|
||||
}
|
||||
}
|
||||
// `/skills` is already in the list, so it is not globbed twice.
|
||||
assert_eq!(
|
||||
source_dirs.expect("SkillsDiscovered must be emitted"),
|
||||
vec![
|
||||
"/skills".to_string(),
|
||||
"/home/test/.agents/skills".to_string()
|
||||
]
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn initialize_emits_skills_discovered_event_when_no_skills() {
|
||||
let sandbox = Arc::new(MockSandbox::linux());
|
||||
|
|
|
|||
|
|
@ -299,6 +299,33 @@ pub fn default_skill_dirs(fabro_skills_dir: Option<&str>, git_root: Option<&str>
|
|||
dirs
|
||||
}
|
||||
|
||||
/// Append configured extra discovery directories to `dirs`.
|
||||
///
|
||||
/// Extra directories are searched last, so a skill they define wins over a
|
||||
/// same-named skill from a convention directory (`discover_skills` keys skills
|
||||
/// by name and later directories overwrite earlier ones). A relative entry
|
||||
/// resolves against `doc_root` — the git root when known, otherwise the
|
||||
/// sandbox working directory — so a repo-relative path such as
|
||||
/// `.agents/skills` means the same thing no matter which subdirectory the
|
||||
/// agent runs in. Duplicate directories are dropped so the same path is not
|
||||
/// globbed twice.
|
||||
pub fn extend_skill_dirs(dirs: &mut Vec<String>, extra: &[String], doc_root: &str) {
|
||||
for dir in extra {
|
||||
let dir = dir.trim();
|
||||
if dir.is_empty() {
|
||||
continue;
|
||||
}
|
||||
let resolved = if std::path::Path::new(dir).is_absolute() {
|
||||
dir.to_string()
|
||||
} else {
|
||||
format!("{}/{dir}", doc_root.trim_end_matches('/'))
|
||||
};
|
||||
if !dirs.contains(&resolved) {
|
||||
dirs.push(resolved);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
pub async fn discover_skills(
|
||||
env: &dyn Sandbox,
|
||||
dirs: &[String],
|
||||
|
|
@ -642,6 +669,58 @@ name: trimmed
|
|||
assert_eq!(dirs, vec!["/home/user/.fabro/skills"]);
|
||||
}
|
||||
|
||||
// --- extend_skill_dirs tests ---
|
||||
|
||||
#[test]
|
||||
fn extra_dirs_are_appended_after_the_convention_dirs() {
|
||||
let mut dirs = default_skill_dirs(Some("/home/user/.fabro/skills"), Some("/repo"));
|
||||
extend_skill_dirs(&mut dirs, &[".agents/skills".to_string()], "/repo");
|
||||
assert_eq!(dirs, vec![
|
||||
"/home/user/.fabro/skills",
|
||||
"/repo/.fabro/skills",
|
||||
"/repo/skills",
|
||||
"/repo/.agents/skills",
|
||||
]);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn extra_dirs_keep_absolute_paths_verbatim() {
|
||||
let mut dirs = Vec::new();
|
||||
extend_skill_dirs(&mut dirs, &["/opt/shared/skills".to_string()], "/repo");
|
||||
assert_eq!(dirs, vec!["/opt/shared/skills"]);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn extra_dirs_resolve_against_a_doc_root_with_a_trailing_slash() {
|
||||
let mut dirs = Vec::new();
|
||||
extend_skill_dirs(&mut dirs, &[".agents/skills".to_string()], "/repo/");
|
||||
assert_eq!(dirs, vec!["/repo/.agents/skills"]);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn extra_dirs_skip_duplicates_and_blank_entries() {
|
||||
let mut dirs = vec!["/repo/skills".to_string()];
|
||||
extend_skill_dirs(
|
||||
&mut dirs,
|
||||
&[
|
||||
"skills".to_string(),
|
||||
" ".to_string(),
|
||||
String::new(),
|
||||
".agents/skills".to_string(),
|
||||
".agents/skills".to_string(),
|
||||
],
|
||||
"/repo",
|
||||
);
|
||||
assert_eq!(dirs, vec!["/repo/skills", "/repo/.agents/skills"]);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn extend_skill_dirs_is_a_no_op_without_extra_dirs() {
|
||||
let mut dirs = vec!["/repo/skills".to_string()];
|
||||
extend_skill_dirs(&mut dirs, &[], "/repo");
|
||||
assert_eq!(dirs, vec!["/repo/skills"]);
|
||||
}
|
||||
|
||||
// --- make_use_skill_tool tests ---
|
||||
|
||||
#[tokio::test]
|
||||
|
|
|
|||
|
|
@ -641,6 +641,9 @@ pub struct AgentApiBackend {
|
|||
emitted_plan_notices: Mutex<HashSet<String>>,
|
||||
tool_env: Option<Arc<dyn ToolEnvProvider>>,
|
||||
mcp_servers: Vec<McpServerSettings>,
|
||||
/// Extra skill discovery directories from `[run.agent] skill_dirs`, added
|
||||
/// to the convention directories every agent session already searches.
|
||||
skill_dirs: Vec<String>,
|
||||
tool_secrets: ToolSecrets,
|
||||
run_model_controls: RunModelControls,
|
||||
source: Arc<dyn CredentialSource>,
|
||||
|
|
@ -820,6 +823,7 @@ impl AgentApiBackend {
|
|||
emitted_plan_notices: Mutex::new(HashSet::new()),
|
||||
tool_env: None,
|
||||
mcp_servers: Vec::new(),
|
||||
skill_dirs: Vec::new(),
|
||||
tool_secrets: ToolSecrets::default(),
|
||||
run_model_controls: RunModelControls::default(),
|
||||
source,
|
||||
|
|
@ -847,6 +851,12 @@ impl AgentApiBackend {
|
|||
self
|
||||
}
|
||||
|
||||
#[must_use]
|
||||
pub fn with_skill_dirs(mut self, dirs: Vec<String>) -> Self {
|
||||
self.skill_dirs = dirs;
|
||||
self
|
||||
}
|
||||
|
||||
#[must_use]
|
||||
pub fn with_tool_secrets(mut self, tool_secrets: ToolSecrets) -> Self {
|
||||
self.tool_secrets = tool_secrets;
|
||||
|
|
@ -1026,6 +1036,7 @@ impl AgentApiBackend {
|
|||
self.tool_env.as_ref(),
|
||||
tool_hooks,
|
||||
self.mcp_servers.clone(),
|
||||
self.skill_dirs.clone(),
|
||||
self.tool_secrets.clone(),
|
||||
self.fabro_run_tools.clone(),
|
||||
)
|
||||
|
|
@ -1050,6 +1061,7 @@ impl AgentApiBackend {
|
|||
tool_env: Option<&Arc<dyn ToolEnvProvider>>,
|
||||
tool_hooks: Option<Arc<dyn fabro_agent::ToolHookCallback>>,
|
||||
mcp_servers: Vec<McpServerSettings>,
|
||||
skill_dirs: Vec<String>,
|
||||
tool_secrets: ToolSecrets,
|
||||
fabro_run_tools: Option<FabroRunToolServices>,
|
||||
) -> Result<Session, Error> {
|
||||
|
|
@ -1083,6 +1095,11 @@ impl AgentApiBackend {
|
|||
speed: controls.speed,
|
||||
tool_hooks,
|
||||
mcp_servers,
|
||||
// `[run.agent] skill_dirs` adds to the convention directories
|
||||
// rather than replacing them, so it lands in `extra_skill_dirs`
|
||||
// and leaves `skill_dirs` (the SDK's replace-the-defaults knob)
|
||||
// alone.
|
||||
extra_skill_dirs: skill_dirs,
|
||||
// Workflow agents run with no `tool_access_policy`, which exposes
|
||||
// the entire tool registry (read, write, shell, subagent, MCP) and
|
||||
// skips approval gating. Report that truthfully so the UI doesn't
|
||||
|
|
@ -1106,6 +1123,9 @@ impl AgentApiBackend {
|
|||
let factory_fabro_run_tools = fabro_run_tools.clone();
|
||||
let factory_permission_level = config.permission_level;
|
||||
let factory_tool_hooks = config.tool_hooks.clone();
|
||||
// Subagents run the same repository's skills, so they inherit the
|
||||
// run's extra discovery directories.
|
||||
let factory_skill_dirs = config.extra_skill_dirs.clone();
|
||||
let factory: SessionFactory = Arc::new(move || {
|
||||
let mut child_profile = factory_profile_builder.build();
|
||||
if let Some(services) = factory_fabro_run_tools.clone() {
|
||||
|
|
@ -1121,6 +1141,7 @@ impl AgentApiBackend {
|
|||
speed: controls.speed,
|
||||
tool_hooks: factory_tool_hooks.clone(),
|
||||
permission_level: factory_permission_level,
|
||||
extra_skill_dirs: factory_skill_dirs.clone(),
|
||||
..SessionOptions::default()
|
||||
},
|
||||
None,
|
||||
|
|
@ -1241,6 +1262,7 @@ impl AgentApiBackend {
|
|||
self.tool_env.as_ref(),
|
||||
request.tool_hooks.clone(),
|
||||
self.mcp_servers.clone(),
|
||||
self.skill_dirs.clone(),
|
||||
self.tool_secrets.clone(),
|
||||
self.fabro_run_tools.clone(),
|
||||
)
|
||||
|
|
@ -3893,6 +3915,81 @@ enabled = true
|
|||
assert_eq!(text, r#"{"passed":true}"#);
|
||||
}
|
||||
|
||||
/// `[run.agent] skill_dirs` reaches the agent session: a skill living in a
|
||||
/// configured directory is discovered even though that directory is not one
|
||||
/// of the conventions.
|
||||
#[tokio::test]
|
||||
async fn agent_run_discovers_skills_from_configured_skill_dirs() {
|
||||
let server = MockServer::start();
|
||||
let completion = server.mock(|when, then| {
|
||||
when.method(POST).path("/chat/completions");
|
||||
then.status(200)
|
||||
.header("content-type", "text/event-stream")
|
||||
.body(chat_completion_stream("Done", 10, 1));
|
||||
});
|
||||
let workspace = tempfile::tempdir().unwrap();
|
||||
let skill_dir = workspace.path().join(".agents/skills/greet");
|
||||
tokio::fs::create_dir_all(&skill_dir).await.unwrap();
|
||||
tokio::fs::write(
|
||||
skill_dir.join("SKILL.md"),
|
||||
"---\nname: greet\ndescription: Greet the user\n---\nSay hello.",
|
||||
)
|
||||
.await
|
||||
.unwrap();
|
||||
|
||||
let backend = mock_api_backend(&server).with_skill_dirs(vec![".agents/skills".to_string()]);
|
||||
let node = Node::new("greeter");
|
||||
let context = Context::new();
|
||||
let emitter = Arc::new(Emitter::new(fabro_types::RunId::new()));
|
||||
let discovered = Arc::new(Mutex::new(Vec::new()));
|
||||
let discovered_for_listener = Arc::clone(&discovered);
|
||||
emitter.on_event(move |event| {
|
||||
if let fabro_types::EventBody::AgentSkillsDiscovered(props) = &event.body {
|
||||
discovered_for_listener.lock().unwrap().push((
|
||||
props.source_dirs.clone(),
|
||||
props
|
||||
.skills
|
||||
.iter()
|
||||
.map(|skill| skill.name.clone())
|
||||
.collect::<Vec<_>>(),
|
||||
));
|
||||
}
|
||||
});
|
||||
let sandbox: Arc<dyn fabro_agent::Sandbox> =
|
||||
Arc::new(LocalSandbox::new(workspace.path().to_path_buf()));
|
||||
|
||||
backend
|
||||
.run(CodergenRunRequest {
|
||||
node: &node,
|
||||
prompt: "Greet",
|
||||
context: &context,
|
||||
thread_id: None,
|
||||
emitter: &emitter,
|
||||
sandbox: &sandbox,
|
||||
tool_hooks: None,
|
||||
cancel_token: CancellationToken::new(),
|
||||
agent_tool_runtime: fabro_agent::AgentToolRuntime::default(),
|
||||
})
|
||||
.await
|
||||
.unwrap();
|
||||
|
||||
completion.assert_calls(1);
|
||||
let discovered = discovered.lock().unwrap();
|
||||
let (source_dirs, skills) = discovered
|
||||
.first()
|
||||
.expect("agent.skills.discovered should be emitted");
|
||||
let configured = workspace
|
||||
.path()
|
||||
.join(".agents/skills")
|
||||
.to_string_lossy()
|
||||
.into_owned();
|
||||
assert_eq!(source_dirs.last(), Some(&configured));
|
||||
assert!(
|
||||
skills.iter().any(|name| name == "greet"),
|
||||
"configured skill dir should contribute its skills, got {skills:?}"
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn agent_run_web_search_uses_configured_brave_search_key() {
|
||||
let server = MockServer::start();
|
||||
|
|
|
|||
|
|
@ -480,6 +480,7 @@ impl RunSession {
|
|||
provider_id: llm.provider_id.clone(),
|
||||
fallbacks: llm.fallbacks.policy,
|
||||
mcp_servers,
|
||||
skill_dirs: resolved.agent.skill_dirs.clone(),
|
||||
model_controls: resolved.model.controls.clone(),
|
||||
dry_run: resolved.execution.mode == RunMode::DryRun,
|
||||
},
|
||||
|
|
@ -1536,6 +1537,41 @@ reasoning = false
|
|||
);
|
||||
}
|
||||
|
||||
/// `[run.agent] skill_dirs` has to reach the LLM spec, which is what hands
|
||||
/// the directories to every agent session the run creates.
|
||||
#[tokio::test]
|
||||
async fn run_session_new_carries_run_agent_skill_dirs_into_the_llm_spec() {
|
||||
let temp = tempfile::tempdir().unwrap();
|
||||
let (storage_root, _run_dir) = storage_root_and_run_dir(&temp);
|
||||
let mut settings = settings_from_run_layer(RunLayer {
|
||||
execution: Some(RunExecutionLayer {
|
||||
mode: Some(RunMode::DryRun),
|
||||
..RunExecutionLayer::default()
|
||||
}),
|
||||
..RunLayer::default()
|
||||
});
|
||||
settings.run.agent.skill_dirs = vec![
|
||||
".agents/skills".to_string(),
|
||||
"/opt/shared/skills".to_string(),
|
||||
];
|
||||
let (persisted, store) =
|
||||
persisted_workflow_with_settings(MINIMAL_DOT, &storage_root, settings).await;
|
||||
let emitter = Arc::new(Emitter::new(fixtures::RUN_1));
|
||||
let registry = Arc::new(test_registry());
|
||||
|
||||
let session = RunSession::new(
|
||||
&persisted,
|
||||
test_start_services(&store, &storage_root, emitter, registry).await,
|
||||
)
|
||||
.await
|
||||
.unwrap();
|
||||
|
||||
assert_eq!(session.llm.skill_dirs, vec![
|
||||
".agents/skills".to_string(),
|
||||
"/opt/shared/skills".to_string(),
|
||||
]);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn run_session_new_missing_secret_fails_startup() {
|
||||
let temp = tempfile::tempdir().unwrap();
|
||||
|
|
|
|||
|
|
@ -267,6 +267,7 @@ async fn execute_test_run_with_options(
|
|||
provider_id: fabro_model::ProviderId::anthropic(),
|
||||
fallbacks: ModelFallbackPolicy::default(),
|
||||
mcp_servers: Vec::new(),
|
||||
skill_dirs: Vec::new(),
|
||||
model_controls: RunModelControls::default(),
|
||||
dry_run: true,
|
||||
},
|
||||
|
|
@ -327,6 +328,7 @@ async fn execute_runs_start_to_exit_and_returns_final_context() {
|
|||
provider_id: fabro_model::ProviderId::anthropic(),
|
||||
fallbacks: ModelFallbackPolicy::default(),
|
||||
mcp_servers: Vec::new(),
|
||||
skill_dirs: Vec::new(),
|
||||
model_controls: RunModelControls::default(),
|
||||
dry_run: true,
|
||||
},
|
||||
|
|
@ -468,6 +470,7 @@ async fn resumed_in_flight_node_starts_a_new_stage_execution() {
|
|||
provider_id: fabro_model::ProviderId::anthropic(),
|
||||
fallbacks: ModelFallbackPolicy::default(),
|
||||
mcp_servers: Vec::new(),
|
||||
skill_dirs: Vec::new(),
|
||||
model_controls: RunModelControls::default(),
|
||||
dry_run: true,
|
||||
},
|
||||
|
|
@ -582,6 +585,7 @@ async fn run_with_lifecycle(
|
|||
provider_id: fabro_model::ProviderId::anthropic(),
|
||||
fallbacks: ModelFallbackPolicy::default(),
|
||||
mcp_servers: Vec::new(),
|
||||
skill_dirs: Vec::new(),
|
||||
model_controls: RunModelControls::default(),
|
||||
dry_run: true,
|
||||
},
|
||||
|
|
|
|||
|
|
@ -171,6 +171,7 @@ async fn build_registry(
|
|||
let provider_id = spec.provider_id.clone();
|
||||
let fallbacks = spec.fallbacks.clone();
|
||||
let mcp_servers = spec.mcp_servers.clone();
|
||||
let skill_dirs = spec.skill_dirs.clone();
|
||||
let model_controls = spec.model_controls.clone();
|
||||
let tool_secrets_for_api = tool_secrets.clone();
|
||||
let llm_source_for_api = Arc::clone(&llm_source);
|
||||
|
|
@ -191,7 +192,8 @@ async fn build_registry(
|
|||
.with_run_model_controls(model_controls.clone())
|
||||
.with_tool_env_provider(tool_env_provider.clone())
|
||||
.with_tool_secrets(tool_secrets_for_api.clone())
|
||||
.with_mcp_servers(mcp_servers.clone());
|
||||
.with_mcp_servers(mcp_servers.clone())
|
||||
.with_skill_dirs(skill_dirs.clone());
|
||||
if let Some(services) = fabro_run_tools_for_api.clone() {
|
||||
api = api.with_fabro_run_tools(services);
|
||||
}
|
||||
|
|
@ -806,6 +808,7 @@ mod tests {
|
|||
provider_id: fabro_model::ProviderId::anthropic(),
|
||||
fallbacks: ModelFallbackPolicy::default(),
|
||||
mcp_servers: Vec::new(),
|
||||
skill_dirs: Vec::new(),
|
||||
model_controls: RunModelControls::default(),
|
||||
dry_run: true,
|
||||
},
|
||||
|
|
@ -1133,6 +1136,7 @@ mod tests {
|
|||
provider_id: fabro_model::ProviderId::anthropic(),
|
||||
fallbacks: ModelFallbackPolicy::default(),
|
||||
mcp_servers: Vec::new(),
|
||||
skill_dirs: Vec::new(),
|
||||
model_controls: RunModelControls::default(),
|
||||
dry_run: false,
|
||||
},
|
||||
|
|
@ -1255,6 +1259,7 @@ mod tests {
|
|||
provider_id: fabro_model::ProviderId::openai(),
|
||||
fallbacks: ModelFallbackPolicy::default(),
|
||||
mcp_servers: Vec::new(),
|
||||
skill_dirs: Vec::new(),
|
||||
model_controls: RunModelControls::default(),
|
||||
dry_run: false,
|
||||
},
|
||||
|
|
@ -1350,6 +1355,7 @@ mod tests {
|
|||
provider_id: fabro_model::ProviderId::anthropic(),
|
||||
fallbacks: ModelFallbackPolicy::default(),
|
||||
mcp_servers: Vec::new(),
|
||||
skill_dirs: Vec::new(),
|
||||
model_controls: RunModelControls::default(),
|
||||
dry_run: true,
|
||||
},
|
||||
|
|
@ -1492,6 +1498,7 @@ mod tests {
|
|||
provider_id: fabro_model::ProviderId::anthropic(),
|
||||
fallbacks: ModelFallbackPolicy::default(),
|
||||
mcp_servers: Vec::new(),
|
||||
skill_dirs: Vec::new(),
|
||||
model_controls: RunModelControls::default(),
|
||||
dry_run: true,
|
||||
},
|
||||
|
|
|
|||
|
|
@ -239,6 +239,9 @@ pub struct LlmSpec {
|
|||
pub provider_id: ProviderId,
|
||||
pub fallbacks: ModelFallbackPolicy,
|
||||
pub mcp_servers: Vec<McpServerSettings>,
|
||||
/// Extra skill discovery directories from `[run.agent] skill_dirs`,
|
||||
/// searched in addition to the agent's convention directories.
|
||||
pub skill_dirs: Vec<String>,
|
||||
pub model_controls: RunModelControls,
|
||||
pub dry_run: bool,
|
||||
}
|
||||
|
|
|
|||
|
|
@ -395,7 +395,7 @@ pub struct InterviewProviderLayer {
|
|||
pub channel: Option<InterpString>,
|
||||
}
|
||||
|
||||
/// `[run.agent]` — agent knobs only (Fabro tools and MCPs).
|
||||
/// `[run.agent]` — agent knobs only (Fabro tools, skill directories, and MCPs).
|
||||
#[derive(
|
||||
Debug,
|
||||
Clone,
|
||||
|
|
@ -413,6 +413,15 @@ pub struct RunAgentLayer {
|
|||
#[option(default = "false", value_type = "boolean")]
|
||||
pub fabro_tools: Option<bool>,
|
||||
|
||||
/// Extra skill discovery directories for workflow agents, searched after
|
||||
/// the convention directories (`~/.fabro/skills`,
|
||||
/// `{git_root}/.fabro/skills`, `{git_root}/skills`), so a skill defined
|
||||
/// here overrides a same-named convention skill. Relative entries resolve
|
||||
/// against the repository root. Splice marker supported at layering time.
|
||||
#[serde(default, skip_serializing_if = "Vec::is_empty")]
|
||||
#[option(value_type = "array")]
|
||||
pub skill_dirs: Vec<StringOrSplice>,
|
||||
|
||||
/// Agent-scoped MCP server entries, keyed by name.
|
||||
#[serde(default, skip_serializing_if = "StickyMap::is_empty")]
|
||||
#[option(value_type = "table")]
|
||||
|
|
|
|||
|
|
@ -324,6 +324,14 @@ fn resolve_agent(
|
|||
|
||||
RunAgentSettings {
|
||||
fabro_tools: agent.fabro_tools.unwrap_or(false),
|
||||
skill_dirs: agent
|
||||
.skill_dirs
|
||||
.iter()
|
||||
.filter_map(|dir| match dir {
|
||||
StringOrSplice::Value(value) => Some(value.clone()),
|
||||
StringOrSplice::Splice => None,
|
||||
})
|
||||
.collect(),
|
||||
mcps: resolve_mcp_entries(&agent.mcps, mcp_server_catalog, errors),
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -971,6 +971,96 @@ fabro_tools = true
|
|||
|
||||
assert!(!settings.agent.fabro_tools);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn skill_dirs_default_to_empty() {
|
||||
let settings = super::workflow_settings_from_layer(SettingsLayer::default())
|
||||
.expect("empty settings should resolve")
|
||||
.run;
|
||||
|
||||
assert!(settings.agent.skill_dirs.is_empty());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn resolves_skill_dirs_in_declared_order() {
|
||||
let settings = super::workflow_settings_from_toml(
|
||||
r#"
|
||||
_version = 1
|
||||
|
||||
[run.agent]
|
||||
skill_dirs = [".agents/skills", "/opt/shared/skills"]
|
||||
"#,
|
||||
)
|
||||
.expect("run.agent.skill_dirs should resolve");
|
||||
|
||||
assert_eq!(settings.run.agent.skill_dirs, vec![
|
||||
".agents/skills".to_string(),
|
||||
"/opt/shared/skills".to_string(),
|
||||
]);
|
||||
}
|
||||
|
||||
/// A higher layer's list replaces the lower layer's, matching every other
|
||||
/// splice-aware list setting.
|
||||
#[test]
|
||||
fn higher_layer_skill_dirs_replace_lower_layer() {
|
||||
let workflow = parse_settings(
|
||||
r#"
|
||||
_version = 1
|
||||
|
||||
[run.agent]
|
||||
skill_dirs = [".agents/skills"]
|
||||
"#,
|
||||
);
|
||||
let user = parse_settings(
|
||||
r#"
|
||||
_version = 1
|
||||
|
||||
[run.agent]
|
||||
skill_dirs = ["/home/user/skills"]
|
||||
"#,
|
||||
);
|
||||
let merged = workflow.combine(user);
|
||||
|
||||
let settings = super::workflow_settings_from_layer(merged)
|
||||
.expect("merged settings should resolve")
|
||||
.run;
|
||||
|
||||
assert_eq!(settings.agent.skill_dirs, vec![
|
||||
".agents/skills".to_string()
|
||||
]);
|
||||
}
|
||||
|
||||
/// `"..."` splices the lower layer's entries in place, so a project can add
|
||||
/// a directory without restating the ones it inherits.
|
||||
#[test]
|
||||
fn splice_marker_expands_the_lower_layer_skill_dirs() {
|
||||
let workflow = parse_settings(
|
||||
r#"
|
||||
_version = 1
|
||||
|
||||
[run.agent]
|
||||
skill_dirs = ["...", ".agents/skills"]
|
||||
"#,
|
||||
);
|
||||
let user = parse_settings(
|
||||
r#"
|
||||
_version = 1
|
||||
|
||||
[run.agent]
|
||||
skill_dirs = ["/home/user/skills"]
|
||||
"#,
|
||||
);
|
||||
let merged = workflow.combine(user);
|
||||
|
||||
let settings = super::workflow_settings_from_layer(merged)
|
||||
.expect("merged settings should resolve")
|
||||
.run;
|
||||
|
||||
assert_eq!(settings.agent.skill_dirs, vec![
|
||||
"/home/user/skills".to_string(),
|
||||
".agents/skills".to_string(),
|
||||
]);
|
||||
}
|
||||
}
|
||||
|
||||
mod run_checkpoint {
|
||||
|
|
|
|||
|
|
@ -131,8 +131,9 @@ enabled = true",
|
|||
),
|
||||
Section::of::<fabro_config::RunAgentLayer>(
|
||||
"[run.agent]",
|
||||
r"[run.agent]
|
||||
fabro_tools = true",
|
||||
r#"[run.agent]
|
||||
fabro_tools = true
|
||||
skill_dirs = [".agents/skills"]"#,
|
||||
),
|
||||
]
|
||||
}
|
||||
|
|
|
|||
|
|
@ -1350,6 +1350,16 @@ pub struct InterviewProviderSettings {
|
|||
pub struct RunAgentSettings {
|
||||
#[serde(default)]
|
||||
pub fabro_tools: bool,
|
||||
/// Extra skill discovery directories, searched in addition to the
|
||||
/// convention directories (`~/.fabro/skills`, `{git_root}/.fabro/skills`,
|
||||
/// `{git_root}/skills`) and after them, so a skill defined here wins over a
|
||||
/// same-named convention skill. Relative entries resolve against the
|
||||
/// repository root.
|
||||
///
|
||||
/// `#[serde(default)]`: run specs persisted before this field existed have
|
||||
/// no `skill_dirs` key, and they must stay loadable.
|
||||
#[serde(default)]
|
||||
pub skill_dirs: Vec<String>,
|
||||
pub mcps: HashMap<String, ResolvedMcpEntry>,
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -19,5 +19,9 @@ import type { McpServerSettings } from './mcp-server-settings';
|
|||
|
||||
export interface RunAgentSettings {
|
||||
'fabro_tools': boolean;
|
||||
/**
|
||||
* Extra skill discovery directories, searched after the convention directories. Relative entries resolve against the repository root.
|
||||
*/
|
||||
'skill_dirs': Array<string>;
|
||||
'mcps': { [key: string]: McpServerSettings; };
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue