Skip to content
Open
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
2 changes: 1 addition & 1 deletion desktop/src-tauri/src/commands/agent_discovery.rs
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,7 @@ pub async fn save_custom_harness(
command: command_opt,
binary_path,
default_args,
mcp_command: None,
mcp_command: definition.mcp_command.clone(),
model_env_var: None,
provider_env_var: None,
thinking_env_var: None,
Expand Down
33 changes: 16 additions & 17 deletions desktop/src-tauri/src/managed_agents/custom_harnesses.rs
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,9 @@ pub(crate) struct HarnessDefinition {
/// Default CLI arguments passed to the command (array, not split-string).
#[serde(default)]
pub args: Vec<String>,
/// Optional MCP sidecar passed to `buzz-acp` for this harness.
#[serde(default, skip_serializing_if = "Option::is_none")]
pub mcp_command: Option<String>,
/// Environment variables injected at spawn time. Definition env is applied
/// first and LOSES on conflict with Buzz-injected vars — `BUZZ_MANAGED_AGENT`
/// is always authoritative and cannot be overridden here.
Expand Down Expand Up @@ -173,6 +176,9 @@ fn validate_harness_definition(def: &HarnessDefinition) -> Result<(), String> {
if def.label.trim().is_empty() {
return Err("label must not be empty".into());
}
if matches!(def.mcp_command.as_deref(), Some(command) if command.trim().is_empty()) {
return Err("mcpCommand must not be blank when provided".into());
}
// Args travel to the harness through the comma-delimited
// `BUZZ_ACP_AGENT_ARGS` env transport (clap `value_delimiter = ','` on the
// buzz-acp side), so a literal comma inside one argument would silently
Expand Down Expand Up @@ -733,18 +739,13 @@ mod tests {
assert_eq!(loaded[0].id, "custom-dup");
}

// ── Round-trip via save_custom_harness_to_dir (B-4) ─────────────────────
//
// These tests exercise the REAL persistence helper, not raw fs::write.
// They prove: create, same-ID edit (backup-swap), rename (old file removed),
// backup file cleaned up on success.

fn make_def(id: &str, label: &str) -> HarnessDefinition {
HarnessDefinition {
id: id.to_string(),
label: label.to_string(),
command: format!("{id}-bin"),
args: vec![],
mcp_command: None,
env: BTreeMap::new(),
install_instructions_url: String::new(),
install_hint: String::new(),
Expand Down Expand Up @@ -773,11 +774,9 @@ mod tests {
let v1 = make_def("my-harness", "V1 Label");
save_custom_harness_to_dir(dir.path(), &v1, None).unwrap();

// Same-ID edit: label changes.
let v2 = make_def("my-harness", "V2 Label");
let outcome = save_custom_harness_to_dir(dir.path(), &v2, None).unwrap();

// No old-path reported (id unchanged).
assert!(outcome.removed_old_path.is_none());

let loaded = load_custom_harnesses(dir.path());
Expand All @@ -794,7 +793,6 @@ mod tests {
let v2 = make_def("my-harness", "V2");
save_custom_harness_to_dir(dir.path(), &v2, None).unwrap();

// .bak file must be gone after a successful commit.
let bak = dir.path().join("my-harness.json.bak");
assert!(
!bak.exists(),
Expand All @@ -808,19 +806,16 @@ mod tests {
let old_def = make_def("old-id", "Old");
save_custom_harness_to_dir(dir.path(), &old_def, None).unwrap();

// Rename: new id, old_id supplied.
let new_def = make_def("new-id", "New");
let outcome = save_custom_harness_to_dir(dir.path(), &new_def, Some("old-id")).unwrap();

// The outcome carries the old path that was removed.
let expected_old = dir.path().join("old-id.json");
assert_eq!(
outcome.removed_old_path,
Some(expected_old.clone()),
"removed_old_path must be the old file"
);

// Old file gone, new file present.
assert!(!expected_old.exists(), "old-id.json must be removed");
let loaded = load_custom_harnesses(dir.path());
assert_eq!(loaded.len(), 1);
Expand All @@ -829,13 +824,11 @@ mod tests {

#[test]
fn save_to_dir_rename_nonexistent_old_id_is_non_fatal() {
// rename_old_id pointing to a file that does not exist must succeed
// (NotFound is silently ignored by the helper).
// NotFound for rename_old_id is ignored.
let dir = tempfile::tempdir().unwrap();
let def = make_def("alpha", "Alpha");
let outcome = save_custom_harness_to_dir(dir.path(), &def, Some("ghost-id")).unwrap();

// New file created, no old path removed.
assert_eq!(outcome.target_path, dir.path().join("alpha.json"));
assert!(
outcome.removed_old_path.is_none(),
Expand All @@ -854,6 +847,7 @@ mod tests {
label: "Env Harness".to_string(),
command: "env-bin".to_string(),
args: vec!["--flag".to_string()],
mcp_command: Some("mcp-sidecar".to_string()),
env,
install_instructions_url: "https://example.com".to_string(),
install_hint: "Install from example.com".to_string(),
Expand All @@ -864,15 +858,14 @@ mod tests {
let loaded = load_custom_harnesses(dir.path());
assert_eq!(loaded.len(), 1);
assert_eq!(loaded[0].args, vec!["--flag"]);
assert_eq!(loaded[0].mcp_command.as_deref(), Some("mcp-sidecar"));
assert_eq!(
loaded[0].env.get("MY_KEY").map(String::as_str),
Some("my_value"),
"env must round-trip through save_custom_harness_to_dir"
);
}

// ── B-3: env validation boundary (validate_harness_definition_pub integration) ──

#[test]
fn validate_rejects_malformed_key_with_equals_sign() {
// BUZZ_AUTH_TAG=x is the documented reserved-key bypass shape:
Expand All @@ -885,6 +878,7 @@ mod tests {
label: "Bad".to_string(),
command: "bad-bin".to_string(),
args: vec![],
mcp_command: None,
env,
install_instructions_url: String::new(),
install_hint: String::new(),
Expand Down Expand Up @@ -914,6 +908,7 @@ mod tests {
label: "Bad".to_string(),
command: "bad-bin".to_string(),
args: vec![],
mcp_command: None,
env,
install_instructions_url: String::new(),
install_hint: String::new(),
Expand All @@ -935,6 +930,7 @@ mod tests {
label: "CI".to_string(),
command: "ci-bin".to_string(),
args: vec![],
mcp_command: None,
env,
install_instructions_url: String::new(),
install_hint: String::new(),
Expand All @@ -956,6 +952,7 @@ mod tests {
label: "NUL".to_string(),
command: "nul-bin".to_string(),
args: vec![],
mcp_command: None,
env,
install_instructions_url: String::new(),
install_hint: String::new(),
Expand All @@ -978,6 +975,7 @@ mod tests {
label: "Big".to_string(),
command: "big-bin".to_string(),
args: vec![],
mcp_command: None,
env,
install_instructions_url: String::new(),
install_hint: String::new(),
Expand All @@ -999,6 +997,7 @@ mod tests {
label: "Good".to_string(),
command: "good-bin".to_string(),
args: vec![],
mcp_command: None,
env,
install_instructions_url: String::new(),
install_hint: String::new(),
Expand Down
4 changes: 1 addition & 3 deletions desktop/src-tauri/src/managed_agents/discovery.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1565,9 +1565,7 @@ pub fn discover_acp_runtimes_from(
command,
binary_path,
default_args,
// Custom harnesses are plain ACP — no MCP sidecar, no env-var
// model switching, no thinking knobs.
mcp_command: None,
mcp_command: def.mcp_command.clone(),
model_env_var: None,
provider_env_var: None,
thinking_env_var: None,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,7 @@ pub(crate) fn preset_harness_definitions(
label: preset.label.to_string(),
command: preset.command.to_string(),
args: preset.args.iter().map(|arg| arg.to_string()).collect(),
mcp_command: None,
env: Default::default(),
install_instructions_url: preset.install_instructions_url.to_string(),
install_hint: preset.install_hint.to_string(),
Expand Down
24 changes: 11 additions & 13 deletions desktop/src-tauri/src/managed_agents/discovery/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1622,6 +1622,7 @@ fn deleted_harness_summary_display_and_spawn_sentence_agree() {
label: "Doomed".to_string(),
command: "doomed-bin".to_string(),
args: vec![],
mcp_command: None,
env: Default::default(),
install_instructions_url: String::new(),
install_hint: String::new(),
Expand Down Expand Up @@ -1652,34 +1653,25 @@ fn deleted_harness_summary_display_and_spawn_sentence_agree() {

// ── I2: custom catalog entry carries definition_env for the edit round-trip ───

/// A custom harness definition that includes env vars must surface those vars
/// in the `definition_env` field of the resulting `AcpRuntimeCatalogEntry`.
///
/// This proves the edit-form round-trip: the backend carries env into the
/// catalog, the frontend reads it back when opening the edit form, and Save
/// therefore preserves existing env vars rather than silently erasing them.
/// Custom definition fields must surface in the catalog for edit round-trips.
#[test]
fn custom_catalog_entry_carries_definition_env_for_edit_roundtrip() {
fn custom_catalog_entry_carries_definition_fields_for_edit_roundtrip() {
use crate::managed_agents::custom_harnesses::registry_test_lock;
use crate::managed_agents::discovery::discover_acp_runtimes_from;
use std::{collections::BTreeMap, fs};
use tempfile::tempdir;

// Discovery's auth probes read/warm the process-global PATH and
// login-shell-PATH caches, and its final step publishes to the global
// harness registry — hold both guards so parallel tests (e.g. the
// PATH-swapping resolution tests) can't observe or absorb torn state.
// Lock order for tests that need both: path lock first, then registry.
// Discovery probes PATH and publishes the global harness registry.
let _path_guard = crate::managed_agents::lock_path_mutex();
let _lock = registry_test_lock();
let dir = tempdir().unwrap();
// Write a custom definition with two env vars.
fs::write(
dir.path().join("env-harness.json"),
r#"{
"id": "env-harness",
"label": "Env Harness",
"command": "env-harness-bin",
"mcpCommand": "read-only-mcp",
"args": [],
"env": { "CURSOR_ACP": "1", "MY_TOKEN": "abc" }
}"#,
Expand All @@ -1703,6 +1695,11 @@ fn custom_catalog_entry_carries_definition_env_for_edit_roundtrip() {
entry.definition_env, expected,
"catalog entry must carry definition env vars so the edit form can read them back"
);
assert_eq!(
entry.mcp_command.as_deref(),
Some("read-only-mcp"),
"catalog entry must carry the MCP command so the edit form preserves it"
);
}

/// A builtin catalog entry must have an empty `definition_env` — their env
Expand Down Expand Up @@ -1766,6 +1763,7 @@ fn harness_def(
label: label.to_string(),
command: command.to_string(),
args: vec![],
mcp_command: None,
env: Default::default(),
install_instructions_url: String::new(),
install_hint: String::new(),
Expand Down
32 changes: 16 additions & 16 deletions desktop/src-tauri/src/managed_agents/readiness.rs
Original file line number Diff line number Diff line change
Expand Up @@ -78,18 +78,7 @@ pub(crate) struct EffectiveAgentEnv {
pub effective_command: String,
}

// ── Typed effective-harness descriptor ───────────────────────────────────────
//
// A single owned type that fully describes what a spawn would run. Produced
// by `resolve_effective_harness_descriptor` and consumed by spawn_agent_child,
// spawn_config_hash, build_managed_agent_summary, get_agent_models, and
// agent_readiness — so the harness-definition lookup and arg/env resolution
// happen exactly once, in one place.

/// The complete effective description of a harness spawn: resolved command,
/// args, and layered env. This is the single source of truth for what will
/// actually run — computed once and shared across every consumer that needs
/// the effective values.
/// Complete effective description of a harness spawn.
#[derive(Debug, Clone)]
pub(crate) struct EffectiveHarnessDescriptor {
/// The raw effective command string (e.g. `"buzz-agent"`, `"my-acp-agent"`).
Expand All @@ -98,15 +87,17 @@ pub(crate) struct EffectiveHarnessDescriptor {
/// Normalized effective args. Instance args win when non-empty; otherwise
/// the harness definition's args apply.
pub args: Vec<String>,
/// MCP sidecar from the custom definition or compiled runtime metadata.
pub mcp_command: Option<String>,
/// The full layered process env: baked floor → runtime metadata → definition
/// env → global → persona → agent.
pub env: BTreeMap<String, String>,
}

/// Resolve the complete harness descriptor from a record + context — the single
/// authoritative path for command, args, and env.
/// authoritative path for command, args, MCP sidecar, and env.
///
/// This is the only place where harness-definition lookup and arg/env layering
/// This is the only place where harness lookup and capability/env layering
/// happen; spawn, hash, summary, and both model-probe paths all consume this.
///
/// Returns `Err("DANGLING_HARNESS_ID:<id>")` when the record (or its linked
Expand Down Expand Up @@ -161,14 +152,23 @@ pub(crate) fn resolve_effective_harness_descriptor(
}
};

// Env: full layered resolution (same as resolve_effective_agent_env).
// Pass harness_def directly to avoid a second lookup.
// Custom definition wins; built-ins fall back to compiled metadata.
let mcp_command = harness_def
.as_ref()
.and_then(|definition| definition.mcp_command.clone())
.or_else(|| {
runtime_meta
.and_then(|runtime| runtime.mcp_command)
.map(str::to_string)
});

let effective_env =
resolve_effective_agent_env_with_def(record, personas, runtime_meta, global, harness_def);

Ok(EffectiveHarnessDescriptor {
command: effective_command,
args,
mcp_command,
env: effective_env.env,
})
}
Expand Down
16 changes: 6 additions & 10 deletions desktop/src-tauri/src/managed_agents/runtime.rs
Original file line number Diff line number Diff line change
Expand Up @@ -289,13 +289,11 @@ pub fn build_managed_agent_summary(
crate::managed_agents::readiness::EffectiveHarnessDescriptor {
command: cmd,
args,
mcp_command: None,
env: Default::default(),
}
});
let effective_mcp_command = known_acp_runtime(&descriptor.command)
.and_then(|r| r.mcp_command)
.unwrap_or("")
.to_string();
let effective_mcp_command = descriptor.mcp_command.clone().unwrap_or_default();

Ok(ManagedAgentSummary {
pubkey: record.pubkey.clone(),
Expand Down Expand Up @@ -464,9 +462,9 @@ pub fn spawn_agent_child(
let runtime_key = ManagedAgentRuntimeKey::new(record.pubkey.clone(), relay_url)?;
// Resolve the effective harness (agent command) from the linked persona, so
// persona harness edits propagate on the next spawn; an explicit per-agent
// override wins. `agent_args` and `mcp_command` are pure derivations of the
// command, so we recompute them from the effective value rather than the
// frozen record snapshot. Mirrors the model resolution below.
// override wins. `agent_args` and `mcp_command` are resolved from the
// effective harness descriptor rather than the frozen record snapshot.
// Mirrors the model resolution below.
let personas = super::load_personas(app).unwrap_or_default();
let teams = super::load_teams(app).unwrap_or_default();
// Load global config once; used for runtime_metadata_env_vars (model/provider fallback)
Expand Down Expand Up @@ -524,9 +522,7 @@ pub fn spawn_agent_child(
.map_err(|error| format!("failed to clone log handle: {error}"))?;
let resolved_acp_command = resolve_command(&record.acp_command)
.ok_or_else(|| missing_command_message(&record.acp_command, "ACP harness command"))?;
let effective_mcp_command = known_acp_runtime(effective_command)
.and_then(|r| r.mcp_command)
.unwrap_or("");
let effective_mcp_command = descriptor.mcp_command.as_deref().unwrap_or("");
let resolved_mcp_command: Option<std::path::PathBuf> = if effective_mcp_command.is_empty() {
None
} else {
Expand Down
Loading