diff --git a/desktop/playwright.config.ts b/desktop/playwright.config.ts index 76f27b6176d..524fbd05492 100644 --- a/desktop/playwright.config.ts +++ b/desktop/playwright.config.ts @@ -171,6 +171,7 @@ export default defineConfig({ "**/observer-archive-policy.spec.ts", "**/harness-management.spec.ts", "**/harness-catalog-screenshots.spec.ts", + "**/harness-profile-variants-screenshots.spec.ts", "**/inline-custom-harness.spec.ts", "**/where-to-run-config.spec.ts", "**/huddle-transcription.spec.ts", diff --git a/desktop/src-tauri/src/commands/agent_discovery.rs b/desktop/src-tauri/src/commands/agent_discovery.rs index 99dcc98145a..b22fa0e2582 100644 --- a/desktop/src-tauri/src/commands/agent_discovery.rs +++ b/desktop/src-tauri/src/commands/agent_discovery.rs @@ -81,9 +81,7 @@ pub async fn save_custom_harness( original_id: Option, app: tauri::AppHandle, ) -> Result { - use crate::managed_agents::{ - custom_harnesses, AcpAvailabilityStatus, AuthStatus, HarnessSource, - }; + use crate::managed_agents::{custom_harnesses, AcpAvailabilityStatus}; use tauri::Manager; // ── Phase 1: full validation before touching the filesystem ───────────── @@ -134,37 +132,12 @@ pub async fn save_custom_harness( None => (AcpAvailabilityStatus::NotInstalled, None, None), }; - let default_args = - crate::managed_agents::normalize_agent_args(&definition.command, definition.args.clone()); - - Ok(AcpRuntimeCatalogEntry { - id: definition.id, - label: definition.label, - avatar_url: String::new(), + Ok(custom_harnesses::custom_harness_entry( + &definition, availability, - command: command_opt, + command_opt, binary_path, - default_args, - mcp_command: None, - model_env_var: None, - provider_env_var: None, - thinking_env_var: None, - effort_canonical_values: None, - max_tokens_env_var: None, - context_limit_env_var: None, - max_rounds_env_var: None, - install_hint: definition.install_hint, - install_instructions_url: definition.install_instructions_url, - can_auto_install: false, - requires_external_cli: false, - underlying_cli_path: None, - node_required: false, - auth_status: AuthStatus::NotApplicable, - login_hint: None, - source: HarnessSource::Custom, - definition_env: definition.env, - max_parallelism: crate::managed_agents::harness_max_parallelism(&definition.command), - }) + )) } /// Remove a user-defined harness definition from `/custom_harnesses/`. diff --git a/desktop/src-tauri/src/managed_agents/config_bridge/effort_tests.rs b/desktop/src-tauri/src/managed_agents/config_bridge/effort_tests.rs index eca4a36bc49..02297e317fb 100644 --- a/desktop/src-tauri/src/managed_agents/config_bridge/effort_tests.rs +++ b/desktop/src-tauri/src/managed_agents/config_bridge/effort_tests.rs @@ -133,6 +133,7 @@ fn harness_def(env: BTreeMap) -> HarnessDefinition { env, install_instructions_url: String::new(), install_hint: String::new(), + ..Default::default() } } diff --git a/desktop/src-tauri/src/managed_agents/custom_harnesses.rs b/desktop/src-tauri/src/managed_agents/custom_harnesses.rs index ba0448beaff..662d123706b 100644 --- a/desktop/src-tauri/src/managed_agents/custom_harnesses.rs +++ b/desktop/src-tauri/src/managed_agents/custom_harnesses.rs @@ -18,6 +18,10 @@ use std::path::Path; use serde::{Deserialize, Serialize}; +use super::types::{ + AcpAvailabilityStatus, AcpRuntimeCatalogEntry, AuthStatus, HarnessSource, ModelSelection, +}; + /// Regex-equivalent predicate for a valid harness ID. /// /// IDs must match `[a-z0-9_][a-z0-9_-]*` — lowercase alphanumeric plus @@ -44,7 +48,7 @@ pub(crate) fn is_valid_harness_id_pub(id: &str) -> bool { /// Only the fields a custom harness definition is permitted to carry are /// included here — install commands and avatar URLs are intentionally absent /// (security line: no remote icon URLs from user-editable config). -#[derive(Debug, Clone, Deserialize, Serialize)] +#[derive(Debug, Clone, Default, Deserialize, Serialize)] #[serde(rename_all = "camelCase")] pub(crate) struct HarnessDefinition { /// Unique identifier, must match `[a-z0-9_][a-z0-9_-]*`. @@ -67,8 +71,39 @@ pub(crate) struct HarnessDefinition { /// Human-readable install hint shown in Doctor. #[serde(default)] pub install_hint: String, + /// Who chooses the LLM model for agents on this harness. Defaults to + /// `User` (Buzz shows the model picker). `Harness` declares that the + /// harness decides — see [`ModelSelection`]. + #[serde(default)] + pub model_selection: ModelSelection, + /// Optional profile-variant expansion. When present, the definition also + /// yields one generated entry per profile directory found under + /// [`HarnessVariants::dir`] (see [`expand_variant_definitions`]). + #[serde(default, skip_serializing_if = "Option::is_none")] + pub variants: Option, + /// True when this definition was materialized from another definition's + /// `variants` block instead of being read from its own file. + /// + /// Set by the loader and never serialized: it describes provenance (and + /// gates the UI's edit/delete affordances), not configuration. Generated + /// entries are managed by editing the template file that produced them. + #[serde(default, skip)] + pub generated: bool, + /// For a generated variant, the id of the definition whose `variants` block + /// produced it. Not serialized for the same reason as [`Self::generated`]: + /// it describes provenance, not configuration. + #[serde(default, skip)] + pub generated_from: Option, } +// ── Profile variants ───────────────────────────────────────────────────────── +// +// The expansion itself lives in `variants.rs` so this module keeps its own +// responsibilities (loading, validation, the built-in id set) and stays +// reviewable. +pub(crate) mod variants; +pub(crate) use variants::{expand_variant_definitions, HarnessVariants}; + /// Scan `dir` for `*.json` files and deserialize each into a `HarnessDefinition`. /// /// Errors per file are logged with `tracing::warn` and skipped — a single @@ -131,25 +166,37 @@ pub(crate) fn load_custom_harnesses(dir: &Path) -> Vec { continue; } - // A custom file must never shadow a built-in or preset id — enforced at - // the loader so the warm path can't admit what discovery would reject. - if let Err(reason) = check_id_collision(&def.id) { - tracing::warn!("custom_harnesses: skipping {} — {reason}", path.display()); - continue; - } + // A definition carrying a `variants` block expands to itself (the + // file-backed template entry the user can edit) plus one generated entry + // per detected profile. Plain definitions fall through as a + // single-element expansion. + for variant in expand_variant_definitions(&def) { + // A custom file must never shadow a built-in or preset id — + // enforced at the loader so the warm path can't admit what + // discovery would reject. + if let Err(reason) = check_id_collision(&variant.id) { + tracing::warn!( + "custom_harnesses: skipping {} (id {:?}) — {reason}", + path.display(), + variant.id + ); + continue; + } - // Dedup within the directory itself (a file's id is taken from its JSON - // content, not its filename, so two files can carry the same id). - if !seen_ids.insert(def.id.clone()) { - tracing::warn!( - "custom_harnesses: skipping {} — duplicate id {:?}", - path.display(), - def.id - ); - continue; - } + // Dedup within the directory itself (an id is taken from JSON + // content, not the filename, so two files — or two profiles + // slugified the same way — can carry the same id). + if !seen_ids.insert(variant.id.clone()) { + tracing::warn!( + "custom_harnesses: skipping {} — duplicate id {:?}", + path.display(), + variant.id + ); + continue; + } - definitions.push(def); + definitions.push(variant); + } } definitions @@ -173,6 +220,10 @@ fn validate_harness_definition(def: &HarnessDefinition) -> Result<(), String> { if def.label.trim().is_empty() { return Err("label must not be empty".into()); } + // A `variants` block is a template the loader expands into real entries, so + // its dir, args, and env must satisfy the same invariants the template's own + // The block's own invariants are validated in `variants`. + variants::validate_variants(def)?; // 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 @@ -210,6 +261,52 @@ pub(crate) fn validate_harness_definition_pub(def: &HarnessDefinition) -> Result validate_harness_definition(def) } +/// Build the catalog entry for a user-defined harness definition. +/// +/// The command layer resolves availability, the command to launch, and the +/// resolved binary path; every field the definition file itself contributes is +/// applied here, so adding a field to [`HarnessDefinition`] means editing one +/// constructor instead of every caller. +pub(crate) fn custom_harness_entry( + definition: &HarnessDefinition, + availability: AcpAvailabilityStatus, + command_opt: Option, + binary_path: Option, +) -> AcpRuntimeCatalogEntry { + AcpRuntimeCatalogEntry { + id: definition.id.clone(), + label: definition.label.clone(), + avatar_url: String::new(), + availability, + command: command_opt, + binary_path, + default_args: super::normalize_agent_args(&definition.command, definition.args.clone()), + mcp_command: None, + model_env_var: None, + provider_env_var: None, + thinking_env_var: None, + effort_canonical_values: None, + max_tokens_env_var: None, + context_limit_env_var: None, + max_rounds_env_var: None, + install_hint: definition.install_hint.clone(), + install_instructions_url: definition.install_instructions_url.clone(), + can_auto_install: false, + requires_external_cli: false, + underlying_cli_path: None, + node_required: false, + auth_status: AuthStatus::NotApplicable, + login_hint: None, + source: HarnessSource::Custom, + definition_env: definition.env.clone(), + definition_variants: definition.variants.clone(), + model_selection: Some(definition.model_selection), + generated: definition.generated, + generated_from: definition.generated_from.clone(), + max_parallelism: super::harness_max_parallelism(&definition.command), + } +} + // ── Built-in ID set ────────────────────────────────────────────────────────── /// IDs reserved for the compiled-in catalog. A custom definition whose `id` @@ -739,7 +836,7 @@ mod tests { // 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 { + pub(super) fn make_def(id: &str, label: &str) -> HarnessDefinition { HarnessDefinition { id: id.to_string(), label: label.to_string(), @@ -748,6 +845,7 @@ mod tests { env: BTreeMap::new(), install_instructions_url: String::new(), install_hint: String::new(), + ..Default::default() } } @@ -857,6 +955,7 @@ mod tests { env, install_instructions_url: "https://example.com".to_string(), install_hint: "Install from example.com".to_string(), + ..Default::default() }; save_custom_harness_to_dir(dir.path(), &def, None).unwrap(); @@ -888,6 +987,7 @@ mod tests { env, install_instructions_url: String::new(), install_hint: String::new(), + ..Default::default() }; let err = validate_harness_definition_pub(&def).unwrap_err(); assert!( @@ -917,6 +1017,7 @@ mod tests { env, install_instructions_url: String::new(), install_hint: String::new(), + ..Default::default() }; let err = validate_harness_definition_pub(&def).unwrap_err(); assert!( @@ -938,6 +1039,7 @@ mod tests { env, install_instructions_url: String::new(), install_hint: String::new(), + ..Default::default() }; let err = validate_harness_definition_pub(&def).unwrap_err(); assert!( @@ -959,6 +1061,7 @@ mod tests { env, install_instructions_url: String::new(), install_hint: String::new(), + ..Default::default() }; let err = validate_harness_definition_pub(&def).unwrap_err(); assert!( @@ -981,6 +1084,7 @@ mod tests { env, install_instructions_url: String::new(), install_hint: String::new(), + ..Default::default() }; let err = validate_harness_definition_pub(&def).unwrap_err(); assert!( @@ -1002,6 +1106,7 @@ mod tests { env, install_instructions_url: String::new(), install_hint: String::new(), + ..Default::default() }; assert!( validate_harness_definition_pub(&def).is_ok(), diff --git a/desktop/src-tauri/src/managed_agents/custom_harnesses/variants.rs b/desktop/src-tauri/src/managed_agents/custom_harnesses/variants.rs new file mode 100644 index 00000000000..befcda72279 --- /dev/null +++ b/desktop/src-tauri/src/managed_agents/custom_harnesses/variants.rs @@ -0,0 +1,414 @@ +//! Profile-variant expansion for a custom harness definition. +//! +//! Kept in its own module so `custom_harnesses.rs` stays within the +//! repository's file-size ratchet. + +use std::collections::BTreeMap; +use std::path::{Path, PathBuf}; + +use serde::{Deserialize, Serialize}; + +use super::{validate_harness_definition, HarnessDefinition}; + +/// Profile-variant expansion for a custom harness definition. +/// +/// One definition file then covers a whole family of harnesses that differ only +/// by directory: a Hermes profile root, a per-project agent config dir, and so +/// on. Each immediate subdirectory of `dir` that carries `marker` becomes one +/// catalog entry, with `{name}` and `{dir}` available in the templates below. +/// +/// A definition with this block is a TEMPLATE. The template itself stays in the +/// catalog as the harness's own default entry (it carries none of the variant +/// `env`, so it resolves exactly like the bare command), and it is also the +/// entry the user edits — the generated variants are read-only. +#[derive(Debug, Clone, Deserialize, Serialize)] +#[serde(rename_all = "camelCase")] +pub(crate) struct HarnessVariants { + /// Directory whose immediate subdirectories are the profiles. A leading + /// `~/` resolves against the user's home directory. + pub dir: String, + /// File that must exist inside a subdirectory for it to count as a profile. + /// Empty means every immediate subdirectory counts. + #[serde(default)] + pub marker: String, + /// Optional cap on expanded entries. Values above + /// [`MAX_HARNESS_VARIANTS`] are clamped to it. + #[serde(default)] + pub max: Option, + /// Template for each variant's id. `{id}` is this definition's id, `{slug}` + /// the id-safe form of the profile directory name. Defaults to + /// `{id}-{slug}`. + #[serde(default)] + pub id_template: Option, + /// Template for each variant's label. `{label}` is this definition's label, + /// `{name}` the profile directory name as it appears on disk, `{meta}` the + /// value read by [`HarnessVariants::label_from`]. Defaults to + /// `{label} ({name})`, or to `{label} [{meta}] ({name})` when `label_from` + /// is declared and resolves. + #[serde(default)] + pub label_template: Option, + /// Optional labelled metadata read from each profile directory, used to + /// fill the `{meta}` placeholder (a Hermes persona title, say). When the + /// declared `label_template` uses `{meta}` and the value cannot be read, the + /// label falls back to the `{label} ({name})` default rather than rendering + /// an empty bracket. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub label_from: Option, + /// Extra env vars for each variant. Placeholders: `{name}` (profile + /// directory name), `{slug}` (its id-safe form), `{dir}` (the profile + /// directory), `{root}` (the scanned directory). + #[serde(default)] + pub env: BTreeMap, + /// Extra args appended to each variant, same placeholders as `env`. + #[serde(default)] + pub args: Vec, +} + +/// One value to read out of a file inside each profile directory. +/// +/// The reading is presentation-only and fully optional: the referenced file +/// lives inside a directory the definition already names, and the definition +/// can already name the command Buzz spawns, so this adds no reach a harness +/// file did not have. Failures degrade to the directory-name label. +#[derive(Debug, Clone, Deserialize, Serialize)] +#[serde(rename_all = "camelCase")] +pub(crate) struct HarnessVariantLabel { + /// File inside the profile directory, e.g. `profile.yaml`. + pub file: String, + /// Dotted path to the value, e.g. `ui_meta.hermes-bots.title`. + pub key: String, +} + +/// Maximum size of a `label_from` file read during expansion. +/// +/// The value is a display label; a profile directory must not be able to make +/// discovery read an arbitrarily large file. +pub(crate) const MAX_VARIANT_LABEL_FILE_BYTES: u64 = 64 * 1024; + +/// Maximum number of catalog entries a single `variants` block may expand to. +/// +/// A template pointed at a large or unexpected directory must not flood the +/// runtime dropdown or the readiness registry. +pub(crate) const MAX_HARNESS_VARIANTS: usize = 64; + +/// Expand a definition into the catalog entries it stands for. +/// +/// Definitions without a `variants` block return themselves as a single entry, +/// so callers have one code path for both shapes. A definition WITH a block +/// returns itself first (the file-backed template entry, which is also the +/// harness's own default entry because it carries none of the variant `env`), +/// followed by one generated entry per profile directory (see +/// [`HarnessVariants`]). +/// +/// A profile is an immediate subdirectory of `variants.dir` that contains +/// `variants.marker` (when a marker is declared). An unreadable directory, a +/// directory whose slugified name is empty, and a variant that fails validation +/// are logged and skipped so one bad profile never blocks the rest. +pub(crate) fn expand_variant_definitions(def: &HarnessDefinition) -> Vec { + // `generated` is provenance, never authored — the template is file-backed. + let base = HarnessDefinition { + generated: false, + generated_from: None, + ..def.clone() + }; + let Some(variants) = def.variants.as_ref() else { + return vec![base]; + }; + + let root = expand_tilde(&variants.dir); + let entries = match std::fs::read_dir(&root) { + Ok(entries) => entries, + Err(err) => { + tracing::warn!( + "custom_harnesses: cannot read variants dir {} for {:?}: {err}", + root.display(), + def.id + ); + return vec![base]; + } + }; + + let marker = variants.marker.trim().to_string(); + let mut profiles: Vec<(String, PathBuf)> = entries + .flatten() + .filter(|entry| entry.file_type().map(|kind| kind.is_dir()).unwrap_or(false)) + .filter_map(|entry| { + let path = entry.path(); + let name = path.file_name()?.to_str()?.to_string(); + if !marker.is_empty() && !path.join(&marker).is_file() { + return None; + } + Some((name, path)) + }) + .collect(); + profiles.sort_by(|a, b| a.0.cmp(&b.0)); + let cap = variants.max.unwrap_or(MAX_HARNESS_VARIANTS); + profiles.truncate(cap.min(MAX_HARNESS_VARIANTS)); + + let id_template = variants.id_template.as_deref().unwrap_or("{id}-{slug}"); + let default_label_template = "{label} ({name})"; + let meta_label_template = "{label} [{meta}] ({name})"; + let declared_label_template = variants.label_template.as_deref(); + + let mut expanded = Vec::with_capacity(profiles.len() + 1); + expanded.push(base); + for (name, dir) in profiles { + let slug = slugify_harness_id_fragment(&name); + if slug.is_empty() { + tracing::warn!( + "custom_harnesses: skipping profile {:?} under {} for {:?}: name has no usable id characters", + name, + root.display(), + def.id + ); + continue; + } + + let meta = variants + .label_from + .as_ref() + .and_then(|source| read_variant_label_meta(&dir, source)); + // A template that asks for `{meta}` on a profile with no readable value + // would render an empty bracket, so fall back to the plain default. + let label_template = match (declared_label_template, meta.as_deref()) { + (Some(template), None) if template.contains("{meta}") => { + tracing::debug!( + "custom_harnesses: profile {:?} has no readable labelFrom value; using {:?}", + name, + default_label_template + ); + default_label_template + } + (Some(template), _) => template, + (None, Some(_)) => meta_label_template, + (None, None) => default_label_template, + }; + + let root_str = root.to_string_lossy().to_string(); + let dir_str = dir.to_string_lossy().to_string(); + let meta_str = meta.as_deref().unwrap_or(""); + let replacements = [ + ("{name}", name.as_str()), + ("{dir}", dir_str.as_str()), + ("{root}", root_str.as_str()), + ("{id}", def.id.as_str()), + ("{label}", def.label.as_str()), + ("{slug}", slug.as_str()), + ("{meta}", meta_str), + ]; + let render = |template: &str| { + let mut out = template.to_string(); + for (needle, value) in replacements { + out = out.replace(needle, value); + } + out + }; + + let mut variant = def.clone(); + variant.id = render(id_template); + variant.label = render(label_template); + variant.args = def + .args + .iter() + .chain(variants.args.iter()) + .map(|arg| render(arg)) + .collect(); + for (key, value) in &variants.env { + variant.env.insert(key.clone(), render(value)); + } + // The template's own block must not survive onto its variants: a + // materialized entry is a real harness, not another template. + variant.variants = None; + variant.generated = true; + variant.generated_from = Some(def.id.clone()); + + if let Err(reason) = validate_harness_definition(&variant) { + tracing::warn!( + "custom_harnesses: skipping variant {:?} (profile {:?}): {reason}", + variant.id, + name + ); + continue; + } + + expanded.push(variant); + } + + expanded +} + +/// Read one dotted-path value out of a file inside a profile directory. +/// +/// `key` is a dotted path into the document (`ui_meta.hermes-bots.title`). +/// Missing files, oversized files, unparseable YAML, absent keys, and non-scalar +/// values all return `None`: a label is presentation, so a profile whose +/// metadata cannot be read still appears in the catalog under its directory +/// name. +fn read_variant_label_meta(dir: &Path, source: &HarnessVariantLabel) -> Option { + let path = dir.join(source.file.trim()); + let metadata = match std::fs::metadata(&path) { + Ok(metadata) => metadata, + Err(err) => { + tracing::debug!( + "custom_harnesses: no label metadata at {}: {err}", + path.display() + ); + return None; + } + }; + if !metadata.is_file() { + return None; + } + if metadata.len() > MAX_VARIANT_LABEL_FILE_BYTES { + tracing::warn!( + "custom_harnesses: label metadata {} is {} bytes, above the {} byte cap — ignoring", + path.display(), + metadata.len(), + MAX_VARIANT_LABEL_FILE_BYTES + ); + return None; + } + + let text = match std::fs::read_to_string(&path) { + Ok(text) => text, + Err(err) => { + tracing::debug!( + "custom_harnesses: cannot read label metadata {}: {err}", + path.display() + ); + return None; + } + }; + let document: serde_yaml::Value = match serde_yaml::from_str(&text) { + Ok(document) => document, + Err(err) => { + tracing::debug!( + "custom_harnesses: label metadata {} is not valid YAML: {err}", + path.display() + ); + return None; + } + }; + + let mut cursor = &document; + for segment in source.key.split('.').map(str::trim) { + if segment.is_empty() { + return None; + } + cursor = cursor.get(segment)?; + } + + let value = match cursor { + serde_yaml::Value::String(text) => text.clone(), + serde_yaml::Value::Number(number) => number.to_string(), + serde_yaml::Value::Bool(flag) => flag.to_string(), + _ => return None, + }; + let value = value.trim().to_string(); + (!value.is_empty()).then_some(value) +} + +/// Resolve a leading `~` against the user's home directory. +/// +/// Harness definition files are hand-written, so `~/.hermes/profiles` must mean +/// the same thing here as it does in the user's shell. +fn expand_tilde(dir: &str) -> PathBuf { + let trimmed = dir.trim(); + let rest = trimmed + .strip_prefix("~/") + .or_else(|| trimmed.strip_prefix("~\\")) + .or_else(|| (trimmed == "~").then_some("")); + match rest { + Some(rest) => match dirs::home_dir() { + Some(home) if rest.is_empty() => home, + Some(home) => home.join(rest), + None => { + tracing::warn!( + "custom_harnesses: no home directory available to expand {:?}", + trimmed + ); + PathBuf::from(trimmed) + } + }, + None => PathBuf::from(trimmed), + } +} + +/// Lowercase a profile directory name into the `[a-z0-9_-]` fragment the id +/// grammar allows, collapsing every other run of characters to a single hyphen. +fn slugify_harness_id_fragment(name: &str) -> String { + let mut out = String::new(); + let mut pending_hyphen = false; + for ch in name.chars() { + if ch.is_ascii_alphanumeric() { + if pending_hyphen && !out.is_empty() { + out.push('-'); + } + pending_hyphen = false; + out.push(ch.to_ascii_lowercase()); + } else { + pending_hyphen = true; + } + } + out +} + +/// Reject a `variants` block the spawn path would reject later. +/// +/// A `variants` block is a template the loader expands into real entries, so +/// its dir, args, and env must satisfy the same invariants the template's own +/// fields do — otherwise the whole family vanishes at load with no signal. +pub(crate) fn validate_variants(def: &HarnessDefinition) -> Result<(), String> { + let Some(variants) = def.variants.as_ref() else { + return Ok(()); + }; + if variants.dir.trim().is_empty() { + return Err("variants.dir must not be empty".into()); + } + if let Some(arg) = variants.args.iter().find(|a| a.contains(',')) { + return Err(format!( + "variants.args: argument {arg:?} contains a comma — arguments are passed via a \ + comma-delimited transport and would be split at spawn time; \ + use separate argument entries instead" + )); + } + crate::managed_agents::env_vars::validate_user_env_keys(&variants.env) + .map_err(|e| format!("variants.env: {e}"))?; + if let Some(label_from) = variants.label_from.as_ref() { + if label_from.file.trim().is_empty() { + return Err("variants.labelFrom.file must not be empty".into()); + } + if label_from.key.trim().is_empty() { + return Err("variants.labelFrom.key must not be empty".into()); + } + // The file is opened relative to a profile directory, so a value + // that escapes it would read arbitrary paths under a name the + // reader does not expect. Rejected outright: + // * anything with a root (`/x`, `\x`, `C:\x`); + // * a Windows drive-relative prefix (`C:x`) — `has_root` is false + // for that shape, and on Linux it is not a prefix at all, so it + // is checked syntactically to behave the same on both; + // * any `..` component, wherever it appears. + // A nested relative path (`nested/meta.yaml`) stays legal. + let file = label_from.file.trim(); + let drive_relative = file.len() >= 2 && file.as_bytes()[1] == b':'; + let escapes = Path::new(file).has_root() + || drive_relative + || Path::new(file).components().any(|component| { + matches!( + component, + std::path::Component::Prefix(_) | std::path::Component::ParentDir + ) + }); + if escapes { + return Err(format!( + "variants.labelFrom.file {file:?} must be a relative path inside the profile \ + directory" + )); + } + } + Ok(()) +} + +#[cfg(test)] +mod tests; diff --git a/desktop/src-tauri/src/managed_agents/custom_harnesses/variants/tests.rs b/desktop/src-tauri/src/managed_agents/custom_harnesses/variants/tests.rs new file mode 100644 index 00000000000..707fd18c7cf --- /dev/null +++ b/desktop/src-tauri/src/managed_agents/custom_harnesses/variants/tests.rs @@ -0,0 +1,328 @@ +//! Regression tests for profile-variant expansion. +//! +//! Every test below drives `load_custom_harnesses`, the seam the whole family +//! flow runs through, so deleting the expansion call in the loader, the +//! `variants` validation, or the `label_from` read fails these tests. + +use std::collections::BTreeMap; +use std::fs; +use std::path::{Path, PathBuf}; + +use super::super::tests::make_def; +use super::super::{ + load_custom_harnesses, save_custom_harness_to_dir, validate_harness_definition, + HarnessDefinition, +}; +use super::{HarnessVariantLabel, HarnessVariants, MAX_HARNESS_VARIANTS}; + +fn variant_template(parent_dir: &str) -> HarnessDefinition { + HarnessDefinition { + // NOT "hermes": `presets.rs` still ships a preset with that id, so + // `check_id_collision` rejects it and the loader would drop the + // template entry before these tests ever see it. The shipped + // declaration uses `hermes-profiles` for the same reason. + id: "hermes-profiles".to_string(), + label: "Hermes Agent".to_string(), + command: "hermes-acp".to_string(), + variants: Some(HarnessVariants { + dir: parent_dir.to_string(), + marker: "profile.yaml".to_string(), + max: None, + // Keep the generated ids short so the assertions below stay + // readable; the default (`{id}-{slug}`) is covered by the + // template's own id. + id_template: Some("hermes-{slug}".to_string()), + label_template: None, + label_from: None, + env: BTreeMap::from([("HERMES_HOME".to_string(), "{dir}".to_string())]), + args: Vec::new(), + }), + ..Default::default() + } +} + +/// Write a profile directory (carrying the `profile.yaml` marker) under +/// `parent` and return its path. +fn write_profile(parent: &Path, name: &str, profile_yaml: &str) -> PathBuf { + let dir = parent.join(name); + fs::create_dir_all(&dir).unwrap(); + fs::write(dir.join("profile.yaml"), profile_yaml).unwrap(); + dir +} + +/// Persist `def` through the real save path and read it back through the +/// real loader — the same two calls the app uses. +fn save_and_load(harness_dir: &Path, def: &HarnessDefinition) -> Vec { + save_custom_harness_to_dir(harness_dir, def, None).unwrap(); + load_custom_harnesses(harness_dir) +} + +#[test] +fn definition_without_variants_loads_as_a_single_entry() { + let harness_dir = tempfile::tempdir().unwrap(); + + let loaded = save_and_load(harness_dir.path(), &make_def("plain", "Plain")); + + assert_eq!(loaded.len(), 1, "no variants block means no expansion"); + assert_eq!(loaded[0].id, "plain"); + assert!(!loaded[0].generated); +} + +#[test] +fn variants_expand_one_entry_per_profile_directory() { + let harness_dir = tempfile::tempdir().unwrap(); + let parent = tempfile::tempdir().unwrap(); + let alpha = write_profile(parent.path(), "alpha", "version: 1\n"); + write_profile(parent.path(), "beta", "version: 1\n"); + + let def = variant_template(&parent.path().to_string_lossy()); + let loaded = save_and_load(harness_dir.path(), &def); + + assert_eq!(loaded.len(), 3, "template plus two profiles: {loaded:#?}"); + + let template = loaded.iter().find(|d| d.id == "hermes-profiles").unwrap(); + assert!( + !template.generated, + "the file-backed template entry is not generated" + ); + assert!(template.variants.is_some(), "the template keeps its block"); + assert!( + template.env.is_empty(), + "the template carries none of the variant env" + ); + + let alpha_entry = loaded.iter().find(|d| d.id == "hermes-alpha").unwrap(); + assert!(alpha_entry.generated); + assert_eq!( + alpha_entry.generated_from.as_deref(), + Some("hermes-profiles"), + "a variant names the definition that produced it so the UI can point at that file" + ); + assert_eq!(alpha_entry.label, "Hermes Agent (alpha)"); + assert_eq!(alpha_entry.command, "hermes-acp"); + assert!( + alpha_entry.variants.is_none(), + "a materialized variant is not another template" + ); + let alpha_home = alpha.to_string_lossy().to_string(); + assert_eq!( + alpha_entry.env.get("HERMES_HOME").map(String::as_str), + Some(alpha_home.as_str()), + "each variant points its env at its own profile directory" + ); + assert_eq!(loaded.iter().filter(|d| d.generated).count(), 2); +} + +#[test] +fn variants_read_label_from_profile_metadata() { + let harness_dir = tempfile::tempdir().unwrap(); + let parent = tempfile::tempdir().unwrap(); + write_profile( + parent.path(), + "generalist", + "version: 1\nui_meta:\n hermes-bots:\n title: Generalist\n", + ); + + let mut def = variant_template(&parent.path().to_string_lossy()); + { + let variants = def.variants.as_mut().unwrap(); + variants.label_template = Some("Hermes Agent [{meta}] ({name})".to_string()); + variants.label_from = Some(HarnessVariantLabel { + file: "profile.yaml".to_string(), + key: "ui_meta.hermes-bots.title".to_string(), + }); + } + let loaded = save_and_load(harness_dir.path(), &def); + + let entry = loaded.iter().find(|d| d.generated).unwrap(); + assert_eq!(entry.id, "hermes-generalist"); + assert_eq!( + entry.label, "Hermes Agent [Generalist] (generalist)", + "the persona title fills the bracket; the directory name stays the suffix" + ); +} + +#[test] +fn variants_default_label_includes_metadata_when_no_template_is_declared() { + let harness_dir = tempfile::tempdir().unwrap(); + let parent = tempfile::tempdir().unwrap(); + write_profile( + parent.path(), + "generalist", + "ui_meta:\n hermes-bots:\n title: Generalist\n", + ); + + let mut def = variant_template(&parent.path().to_string_lossy()); + def.variants.as_mut().unwrap().label_from = Some(HarnessVariantLabel { + file: "profile.yaml".to_string(), + key: "ui_meta.hermes-bots.title".to_string(), + }); + let loaded = save_and_load(harness_dir.path(), &def); + + let entry = loaded.iter().find(|d| d.generated).unwrap(); + assert_eq!(entry.label, "Hermes Agent [Generalist] (generalist)"); +} + +#[test] +fn variants_fall_back_to_a_plain_label_when_metadata_is_unreadable() { + let harness_dir = tempfile::tempdir().unwrap(); + let parent = tempfile::tempdir().unwrap(); + // Marker present, `ui_meta` absent: the profile still appears. + write_profile(parent.path(), "generalist", "version: 1\n"); + + let mut def = variant_template(&parent.path().to_string_lossy()); + { + let variants = def.variants.as_mut().unwrap(); + variants.label_template = Some("Hermes Agent [{meta}] ({name})".to_string()); + variants.label_from = Some(HarnessVariantLabel { + file: "profile.yaml".to_string(), + key: "ui_meta.hermes-bots.title".to_string(), + }); + } + let loaded = save_and_load(harness_dir.path(), &def); + + let entry = loaded.iter().find(|d| d.generated).unwrap(); + assert_eq!( + entry.label, "Hermes Agent (generalist)", + "an unreadable value must not render as an empty bracket" + ); +} + +#[test] +fn variants_skip_directories_without_the_marker() { + let harness_dir = tempfile::tempdir().unwrap(); + let parent = tempfile::tempdir().unwrap(); + write_profile(parent.path(), "alpha", "version: 1\n"); + fs::create_dir_all(parent.path().join("not-a-profile")).unwrap(); + fs::write(parent.path().join("loose-file.json"), "{}").unwrap(); + + let def = variant_template(&parent.path().to_string_lossy()); + let loaded = save_and_load(harness_dir.path(), &def); + + let mut ids: Vec<&str> = loaded.iter().map(|d| d.id.as_str()).collect(); + ids.sort_unstable(); + assert_eq!(ids, vec!["hermes-alpha", "hermes-profiles"]); +} + +#[test] +fn variants_are_capped_at_the_documented_maximum() { + let harness_dir = tempfile::tempdir().unwrap(); + let parent = tempfile::tempdir().unwrap(); + for index in 0..(MAX_HARNESS_VARIANTS + 6) { + write_profile(parent.path(), &format!("p{index:03}"), "version: 1\n"); + } + + let def = variant_template(&parent.path().to_string_lossy()); + let loaded = save_and_load(harness_dir.path(), &def); + + assert_eq!( + loaded.iter().filter(|d| d.generated).count(), + MAX_HARNESS_VARIANTS, + "a template pointed at a huge directory must not flood the catalog" + ); + assert_eq!(loaded.len(), MAX_HARNESS_VARIANTS + 1); +} + +#[test] +fn variants_honour_a_declared_max() { + let harness_dir = tempfile::tempdir().unwrap(); + let parent = tempfile::tempdir().unwrap(); + for name in ["alpha", "beta", "gamma"] { + write_profile(parent.path(), name, "version: 1\n"); + } + + let mut def = variant_template(&parent.path().to_string_lossy()); + def.variants.as_mut().unwrap().max = Some(2); + let loaded = save_and_load(harness_dir.path(), &def); + + let mut ids: Vec<&str> = loaded.iter().map(|d| d.id.as_str()).collect(); + ids.sort_unstable(); + assert_eq!( + ids, + vec!["hermes-alpha", "hermes-beta", "hermes-profiles"], + "the cap applies to generated entries only; the template always survives" + ); +} + +#[test] +fn variants_validation_rejects_label_files_outside_the_profile_dir() { + let mut def = variant_template("~/.hermes/profiles"); + fn with_file(def: &mut HarnessDefinition, file: &str) { + def.variants.as_mut().unwrap().label_from = Some(HarnessVariantLabel { + file: file.to_string(), + key: "ui_meta.hermes-bots.title".to_string(), + }); + } + + for escaping in [ + "../secrets.yaml", + "nested/../../up.yaml", + "/etc/passwd", + "\\Windows\\win.ini", + "C:\\secrets.yaml", + "C:secrets.yaml", + ] { + with_file(&mut def, escaping); + let err = validate_harness_definition(&def) + .expect_err("a label file outside the profile directory must be rejected"); + assert!( + format!("{err:?}").contains("labelFrom.file"), + "the error must name the offending field, got: {err:?}" + ); + } + + with_file(&mut def, "nested/meta.yaml"); + assert!( + validate_harness_definition(&def).is_ok(), + "a nested relative path inside the profile directory stays legal" + ); +} + +#[test] +fn variants_validation_rejects_empty_label_file_or_key() { + let mut def = variant_template("~/.hermes/profiles"); + def.variants.as_mut().unwrap().label_from = Some(HarnessVariantLabel { + file: " ".to_string(), + key: "ui_meta.hermes-bots.title".to_string(), + }); + assert!(validate_harness_definition(&def).is_err()); + + def.variants.as_mut().unwrap().label_from = Some(HarnessVariantLabel { + file: "profile.yaml".to_string(), + key: "".to_string(), + }); + assert!(validate_harness_definition(&def).is_err()); +} + +#[test] +fn variants_validation_rejects_env_that_the_spawn_path_would_reject() { + let mut def = variant_template("~/.hermes/profiles"); + def.variants + .as_mut() + .unwrap() + .env + .insert("BUZZ_AUTH_TAG".to_string(), "forged".to_string()); + + let err = validate_harness_definition(&def) + .expect_err("a reserved key inside variants.env must be rejected at load"); + assert!( + format!("{err:?}").contains("variants.env"), + "the error must name the offending block, got: {err:?}" + ); +} + +#[test] +fn variants_with_an_unreadable_dir_still_load_the_template() { + let harness_dir = tempfile::tempdir().unwrap(); + let missing = harness_dir.path().join("no-such-parent"); + + let def = variant_template(&missing.to_string_lossy()); + let loaded = save_and_load(harness_dir.path(), &def); + + assert_eq!( + loaded.len(), + 1, + "an unreadable variants dir degrades to the bare template, never to nothing" + ); + assert_eq!(loaded[0].id, "hermes-profiles"); +} diff --git a/desktop/src-tauri/src/managed_agents/discovery.rs b/desktop/src-tauri/src/managed_agents/discovery.rs index e4b87e7557a..6ea9eedc0e7 100644 --- a/desktop/src-tauri/src/managed_agents/discovery.rs +++ b/desktop/src-tauri/src/managed_agents/discovery.rs @@ -1058,6 +1058,12 @@ fn discover_acp_runtime_phase1(runtime: &'static KnownAcpRuntime, force: bool) - login_hint: None, source: HarnessSource::Builtin, definition_env: Default::default(), + // Builtin runtimes are always user-selected and are never generated + // from another definition's `variants` block. + definition_variants: None, + model_selection: None, + generated: false, + generated_from: None, max_parallelism: super::parallelism::harness_max_parallelism(runtime.id), }, } @@ -1199,6 +1205,12 @@ pub fn discover_acp_runtimes_from( login_hint: None, source: HarnessSource::Custom, definition_env: def.env.clone(), // preserve for edit round-trip + // Same round-trip reason: the harness form does not author this + // block, so carry it or an edit would erase it. + definition_variants: def.variants.clone(), + model_selection: Some(def.model_selection), + generated: def.generated, + generated_from: def.generated_from.clone(), max_parallelism: super::parallelism::harness_max_parallelism(&def.command), }); } diff --git a/desktop/src-tauri/src/managed_agents/discovery/presets.rs b/desktop/src-tauri/src/managed_agents/discovery/presets.rs index d46145dbd68..adc6ed782fd 100644 --- a/desktop/src-tauri/src/managed_agents/discovery/presets.rs +++ b/desktop/src-tauri/src/managed_agents/discovery/presets.rs @@ -96,6 +96,12 @@ pub(super) fn preset_catalog_entry( login_hint: None, source: HarnessSource::Preset, definition_env: Default::default(), + // Presets are always user-selected and are never generated from another + // definition's `variants` block. + definition_variants: None, + model_selection: None, + generated: false, + generated_from: None, // Derived from the static preset command (`def.command`). This ensures // unavailable entries (command: null in JSON, None here) still carry // the cap — the harness cap is command-keyed, not availability-gated. @@ -241,6 +247,7 @@ pub(crate) fn preset_harness_definitions( env: Default::default(), install_instructions_url: preset.install_instructions_url.to_string(), install_hint: preset.install_hint.to_string(), + ..Default::default() }, ) .collect() diff --git a/desktop/src-tauri/src/managed_agents/discovery/tests.rs b/desktop/src-tauri/src/managed_agents/discovery/tests.rs index 7121151cd47..c8d9942a3a7 100644 --- a/desktop/src-tauri/src/managed_agents/discovery/tests.rs +++ b/desktop/src-tauri/src/managed_agents/discovery/tests.rs @@ -1541,10 +1541,7 @@ fn deleted_harness_summary_display_and_spawn_sentence_agree() { id: "doomed".to_string(), label: "Doomed".to_string(), command: "doomed-bin".to_string(), - args: vec![], - env: Default::default(), - install_instructions_url: String::new(), - install_hint: String::new(), + ..Default::default() }; save_and_warm(dir.path(), &def, None).unwrap(); let record = record_with(Some("doomed"), None, None); @@ -1685,10 +1682,7 @@ fn harness_def( id: id.to_string(), label: label.to_string(), command: command.to_string(), - args: vec![], - env: Default::default(), - install_instructions_url: String::new(), - install_hint: String::new(), + ..Default::default() } } /// A `save_and_warm` landing mid-discovery (after the scan, before the diff --git a/desktop/src-tauri/src/managed_agents/persona_events/stale_pin_tests.rs b/desktop/src-tauri/src/managed_agents/persona_events/stale_pin_tests.rs index 9e41aee21f5..8fc52e88a91 100644 --- a/desktop/src-tauri/src/managed_agents/persona_events/stale_pin_tests.rs +++ b/desktop/src-tauri/src/managed_agents/persona_events/stale_pin_tests.rs @@ -134,6 +134,7 @@ fn apply_persona_snapshot_goose_to_custom_harness_drops_stale_goose_pin() { env: BTreeMap::new(), install_instructions_url: String::new(), install_hint: String::new(), + ..Default::default() }]); let mut record = sample_record(); diff --git a/desktop/src-tauri/src/managed_agents/types.rs b/desktop/src-tauri/src/managed_agents/types.rs index 18c0747e58c..f8abc053c8d 100644 --- a/desktop/src-tauri/src/managed_agents/types.rs +++ b/desktop/src-tauri/src/managed_agents/types.rs @@ -1,6 +1,8 @@ use serde::{Deserialize, Serialize}; use std::{collections::BTreeMap, path::PathBuf, process::Child}; +use super::custom_harnesses::HarnessVariants; + #[derive(Debug, Clone, Default, Serialize, Deserialize, PartialEq)] #[serde(tag = "type", rename_all = "snake_case")] pub enum BackendKind { @@ -660,6 +662,23 @@ pub enum HarnessSource { Custom, } +/// Who chooses the LLM model for agents running on a harness. +/// +/// Serializes as a lowercase string so the TypeScript consumer can switch on it +/// and custom-harness JSON files can spell it out: `"modelSelection": "harness"`. +#[derive(Debug, Clone, Copy, Default, PartialEq, Eq, Deserialize, Serialize)] +#[serde(rename_all = "lowercase")] +pub enum ModelSelection { + /// Buzz owns the choice: the model picker is shown and the selected model + /// is stored on the agent. The default for every runtime. + #[default] + User, + /// The harness owns the choice (a profile directory, a config file, or the + /// CLI itself decides). Buzz hides the model picker for agents on this + /// harness and stores no model override for them. + Harness, +} + #[derive(Debug, Clone, Serialize)] pub struct AcpRuntimeCatalogEntry { pub id: String, @@ -717,6 +736,30 @@ pub struct AcpRuntimeCatalogEntry { /// Spawn-time parallelism cap; absent for uncapped harnesses. #[serde(skip_serializing_if = "Option::is_none")] pub max_parallelism: Option, + /// Definition-level profile-variant template for `source: custom` entries; + /// populated from `HarnessDefinition.variants` for the same reason + /// `definition_env` exists: the harness form does not author this block, so + /// without the round-trip an edit in the UI would erase a hand-authored one. + /// Absent for builtin/preset entries. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub definition_variants: Option, + /// Who chooses the LLM model for agents on this harness. `Some(Harness)` + /// means the harness decides (a profile directory, its own config file), so + /// the UI must not offer a model picker for agents pinned to this entry. + /// Absent for builtin/preset entries, which are always user-selected. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub model_selection: Option, + /// True when this entry was materialized from another definition's + /// `variants` block (a detected profile) rather than read from its own file. + /// Generated entries are read-only in the UI: they are edited and deleted + /// through the definition file that produced them. + #[serde(default)] + pub generated: bool, + /// Id of the definition whose `variants` block produced this entry, when + /// `generated` is true. Lets the UI name the file to edit instead of + /// telling the user to go find it. Absent for every other entry. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub generated_from: Option, } /// Result of a single install step (CLI or adapter). diff --git a/desktop/src/features/agents/AGENTS.md b/desktop/src/features/agents/AGENTS.md index aa586b206a0..daaf1085ea3 100644 --- a/desktop/src/features/agents/AGENTS.md +++ b/desktop/src/features/agents/AGENTS.md @@ -36,6 +36,19 @@ X" flag): add it to `KnownAcpRuntime` first, expose it on `AcpRuntimeCatalogEntry`, then project it through the core. Do not shortcut with a TypeScript lookup table or an id comparison in a component. +**Third source: definition-authored facts on user-defined harnesses.** +A custom harness definition file may declare capabilities that no preset can +know, and those belong to the definition, not to a TypeScript table. Today that +is `model_selection: "user" | "harness"` (the harness picks the model, so no +picker may render) plus the `variants` block, which expands one definition into +one read-only catalog entry per detected profile. Both reach the UI the same +way as preset facts: the definition is projected onto `AcpRuntimeCatalogEntry` +(`modelSelection`, `generated`, `generatedFrom`) and consumed through +`lib/agentConfigCore.ts`. Components must never test a runtime id, a definition +field, or `generated` to decide whether a control renders — they ask the field +model. `generated` is a provenance flag for edit surfaces only +(`harnessGalleryLogic.isEditableEntry`); it is not a rendering capability. + ## Rules 1. **No hardcoded harness-ID checks in render code.** `runtime.id === "claude"` @@ -50,7 +63,10 @@ with a TypeScript lookup table or an id comparison in a component. Goose/Claude — do not "fix" one to match the other without doing the migration work. 3. **Field absence has a named reason, not a boolean.** Codex effort is - `ownedByModelId`; Claude effort is `deferredUntilNativeOptionsAvailable`. + `ownedByModelId`; Claude effort is `deferredUntilNativeOptionsAvailable`; + model is `ownedByHarnessSelection` when the catalog entry reports + `modelSelection: "harness"` (the harness picks the model for that entry, so + the picker is omitted rather than shown and ignored). New absences get new named reasons in `AgentConfigOmission` / `render` — never a `showX` prop. 4. **The clearing policy is the named types.** `onContextChange: @@ -336,7 +352,9 @@ buzz messages send --channel --reply-to \ ## The tests that enforce this - `lib/agentConfigCore.test.mjs` — field model per harness × scope, clearing - policy. Update when the capability model changes. + policy, and the harness-owned model omission + (`ownedByHarnessSelection` + `harnessOwnsModelSelection`). Update when the + capability model changes. - `ui/agentConfigFieldsContract.test.mjs` — canonical behaviors + disclosure presets + `shouldShowModelStatusMessage` status-bypass + `shouldRenderModelControl` (successful-empty omit vs failure keep). If this diff --git a/desktop/src/features/agents/lib/agentConfigCore.test.mjs b/desktop/src/features/agents/lib/agentConfigCore.test.mjs index 520b51b99ea..dde600d5f97 100644 --- a/desktop/src/features/agents/lib/agentConfigCore.test.mjs +++ b/desktop/src/features/agents/lib/agentConfigCore.test.mjs @@ -4,6 +4,7 @@ import test from "node:test"; import { deriveAgentConfigFieldModel, deriveNumericDescriptors, + harnessOwnsModelSelection, structuredEnvKeys, } from "./agentConfigCore.ts"; import { NUMERIC_KIND_MIN } from "../ui/buzzAgentModelTuningFields.tsx"; @@ -721,3 +722,78 @@ test("buzz_agent_optionSource_unchanged_still_buzzAgentCatalog", () => { "buzz-agent optionSource must remain buzzAgentCatalog", ); }); + +// A harness that selects its own model (a profile-variant entry declaring +// `modelSelection: "harness"`) must not offer a model picker. The catalog fact +// is the only source; the persona dialogs ask this predicate instead of +// re-deriving the answer, so both paths are pinned here. +test("harnessOwnsModelSelection_is_true_only_for_the_harness_selection", () => { + assert.equal( + harnessOwnsModelSelection(undefined), + false, + "an unknown runtime never owns the model", + ); + assert.equal( + harnessOwnsModelSelection(runtime("hermes-profiles")), + false, + "an entry with no modelSelection fact keeps the picker", + ); + assert.equal( + harnessOwnsModelSelection( + runtime("hermes-profiles", { modelSelection: "user" }), + ), + false, + ); + assert.equal( + harnessOwnsModelSelection( + runtime("hermes-profiles", { modelSelection: "harness" }), + ), + true, + ); +}); + +test("harness_owned_model_selection_omits_the_model_field_with_a_named_reason", () => { + const owned = deriveAgentConfigFieldModel({ + config, + runtime: runtime("hermes-profiles", { + modelSelection: "harness", + modelEnvVar: "HERMES_MODEL", + }), + scope: "global", + }); + + assert.equal( + field(owned, "model"), + undefined, + "no model control can render for a harness-owned selection", + ); + assert.equal( + owned.fields.some((item) => item.kind === "model"), + false, + ); + assert.deepEqual( + owned.omissions.find((item) => item.kind === "model"), + { kind: "model", reason: "ownedByHarnessSelection" }, + "the absence carries a named reason, never a boolean", + ); +}); + +test("user_model_selection_keeps_the_model_field", () => { + const chosen = deriveAgentConfigFieldModel({ + config, + runtime: runtime("hermes-profiles", { + modelSelection: "user", + modelEnvVar: "HERMES_MODEL", + }), + scope: "global", + }); + + assert.deepEqual(field(chosen, "model").targetApplication, { + kind: "envVar", + key: "HERMES_MODEL", + }); + assert.equal( + chosen.omissions.some((item) => item.kind === "model"), + false, + ); +}); diff --git a/desktop/src/features/agents/lib/agentConfigCore.ts b/desktop/src/features/agents/lib/agentConfigCore.ts index de31e724cc1..ed70e677c2f 100644 --- a/desktop/src/features/agents/lib/agentConfigCore.ts +++ b/desktop/src/features/agents/lib/agentConfigCore.ts @@ -90,10 +90,26 @@ export type AgentConfigFieldDescriptor = value: string | null; }; -export type AgentConfigOmission = { - kind: "effort"; - reason: "ownedByModelId" | "unsupportedByHarness"; -}; +export type AgentConfigOmission = + | { kind: "effort"; reason: "ownedByModelId" | "unsupportedByHarness" } + | { kind: "model"; reason: "ownedByHarnessSelection" }; + +/** + * Whether `runtime` resolves its own LLM model (a Hermes profile root, a + * per-project config file). The model is then a property of the harness rather + * than of the agent, so no picker is rendered. + * + * This is the single projection of the catalog fact `modelSelection`. The + * shared field renderer reads it through `deriveAgentConfigFieldModel`, which + * omits the model descriptor; the persona dialogs render their own control + * instead of that renderer, so they call this predicate to reach the same + * answer rather than re-deriving it from a boolean prop. + */ +export function harnessOwnsModelSelection( + runtime: AcpRuntimeCatalogEntry | undefined, +) { + return runtime?.modelSelection === "harness"; +} /** * A numeric tuning descriptor: one of the three env-var-backed number fields @@ -192,16 +208,22 @@ export function deriveAgentConfigFieldModel({ }); } - fields.push({ - kind: "model", - optionSource: "acpModels", - persistence: { kind: "normalizedField", field: "model" }, - targetApplication: runtime?.modelEnvVar - ? { kind: "envVar", key: runtime.modelEnvVar } - : { kind: "acpNative" }, - render: "control", - value: config.model, - }); + if (harnessOwnsModelSelection(runtime)) { + // Omitted with a named reason, not a per-surface flag: the harness picks the + // model, so the absence is a capability fact about this runtime. + omissions.push({ kind: "model", reason: "ownedByHarnessSelection" }); + } else { + fields.push({ + kind: "model", + optionSource: "acpModels", + persistence: { kind: "normalizedField", field: "model" }, + targetApplication: runtime?.modelEnvVar + ? { kind: "envVar", key: runtime.modelEnvVar } + : { kind: "acpNative" }, + render: "control", + value: config.model, + }); + } if (runtime?.thinkingEnvVar) { // targetApplication is always the runtime's native key — how the harness diff --git a/desktop/src/features/agents/ui/AgentDefinitionDialog.tsx b/desktop/src/features/agents/ui/AgentDefinitionDialog.tsx index c0968ad0e9d..5d1d2cdff0a 100644 --- a/desktop/src/features/agents/ui/AgentDefinitionDialog.tsx +++ b/desktop/src/features/agents/ui/AgentDefinitionDialog.tsx @@ -67,6 +67,7 @@ import { MODEL_DISCOVERY_LOADING_VALUE, usePersonaModelDiscovery, } from "./usePersonaModelDiscovery"; +import { harnessOwnsModelSelection } from "../lib/agentConfigCore"; import { useBakedBuildEnvKeysQuery, useRuntimeFileConfigQuery } from "../hooks"; import { useAgentDialogDefaults } from "./useAgentDialogDefaults"; import { AgentDefaultsDialog } from "./AgentDefaultsDialog"; @@ -403,6 +404,10 @@ export function AgentDefinitionDialog({ const runtimeCanChooseLlmProvider = runtimeSupportsLlmProviderSelection(runtime) || blankRuntimeModelProviderEditable; + // Harness-owned model: the runtime resolves its own model, so the field is + // absent by capability (agentConfigCore omits it with a named reason) rather + // than by a per-surface decision. + const runtimeOwnsModel = harnessOwnsModelSelection(selectedRuntime); const llmProviderFieldVisible = (runtime.trim().length > 0 && runtimeCanChooseLlmProvider) || blankRuntimeModelProviderEditable; @@ -487,12 +492,15 @@ export function AgentDefinitionDialog({ const providerIsRequired = aiConfigurationMode === "custom" && runtimeCanChooseLlmProvider; const modelFieldVisible = - runtime.trim().length > 0 || blankRuntimeModelProviderEditable; - const isExplicitModelRequired = aiConfigurationMode === "custom"; + (runtime.trim().length > 0 || blankRuntimeModelProviderEditable) && + !runtimeOwnsModel; + const isExplicitModelRequired = + aiConfigurationMode === "custom" && !runtimeOwnsModel; const customAiPairSatisfied = agentAiConfigurationModeSatisfied( aiConfigurationMode, { provider, model }, runtimeCanChooseLlmProvider, + !runtimeOwnsModel, ); const selectedRuntimeIsAvailable = runtime.trim().length === 0 || diff --git a/desktop/src/features/agents/ui/agentAiConfigurationPolicy.ts b/desktop/src/features/agents/ui/agentAiConfigurationPolicy.ts index d39797cda09..2f62763e042 100644 --- a/desktop/src/features/agents/ui/agentAiConfigurationPolicy.ts +++ b/desktop/src/features/agents/ui/agentAiConfigurationPolicy.ts @@ -66,10 +66,12 @@ export function agentAiConfigurationModeSatisfied( mode: AgentAiConfigurationMode, pair: AgentAiConfigurationPair, needsProviderSelection = true, + needsModelSelection = true, ) { if (mode === "defaults") { return true; } const providerOk = !needsProviderSelection || pair.provider.trim().length > 0; - return providerOk && pair.model.trim().length > 0; + const modelOk = !needsModelSelection || pair.model.trim().length > 0; + return providerOk && modelOk; } diff --git a/desktop/src/features/settings/ui/HarnessRow.tsx b/desktop/src/features/settings/ui/HarnessRow.tsx index 4e471967d40..3c69a907305 100644 --- a/desktop/src/features/settings/ui/HarnessRow.tsx +++ b/desktop/src/features/settings/ui/HarnessRow.tsx @@ -41,7 +41,11 @@ import { isDownloadPageUrl, } from "./harnessCatalogLogic"; import { formValuesFromCatalogEntry } from "./harnessFormLogic"; -import { deleteConfirmState } from "./harnessGalleryLogic"; +import { + deleteConfirmState, + generatedEntryNote, + isEditableEntry, +} from "./harnessGalleryLogic"; /** Link label for the row's install-instructions URL. Distinct from the * catalog's `installLinkLabel` — rows spell out what the guide covers @@ -303,7 +307,11 @@ export function HarnessRow({ resetEpoch: number; runtime: AcpRuntimeCatalogEntry; }) { - const isCustom = runtime.source === "custom"; + // Generated (profile-variant) entries are read-only in this row: they are + // owned by the definition that produced them, so edit/delete would only + // create drift that the next scan throws away. + const canEdit = isEditableEntry(runtime); + const generatedNote = generatedEntryNote(runtime); const [terminalLaunchMethodId, setTerminalLaunchMethodId] = React.useState< string | null >(null); @@ -439,14 +447,14 @@ export function HarnessRow({ ); }} onDelete={ - isCustom + canEdit ? () => { setDeleteError(null); setConfirmingDelete(true); } : undefined } - onEdit={isCustom ? () => setEditing(true) : undefined} + onEdit={canEdit ? () => setEditing(true) : undefined} onInstall={() => { if (runtime.availability === "adapter_outdated") { setIsUpdateWarningOpen(true); @@ -458,6 +466,15 @@ export function HarnessRow({ /> + {generatedNote ? ( +

+ {generatedNote} +

+ ) : null} + {runtime.authStatus.status === "config_invalid" ? (

{ it("returns false for builtin entries", () => { assert.ok(!isEditableEntry(entry({ source: "builtin" }))); }); + + // A generated entry is `source: "custom"` but is owned by the definition that + // produced it, so the row must not offer edit/delete for it. + it("returns false for a generated custom entry", () => { + assert.ok( + !isEditableEntry( + entry({ source: "custom", generated: true, generatedFrom: "hermes" }), + ), + ); + }); +}); + +// ── generatedEntryNote ──────────────────────────────────────────────────────── + +describe("generatedEntryNote", () => { + it("returns null for an authored entry", () => { + assert.equal(generatedEntryNote(entry({ source: "custom" })), null); + }); + + it("names the parent definition so the user knows which file to edit", () => { + const note = generatedEntryNote( + entry({ source: "custom", generated: true, generatedFrom: "hermes" }), + ); + assert.ok(note); + assert.ok(note.includes('"hermes"')); + }); + + it("falls back to a definition-agnostic sentence when the parent is unknown", () => { + const note = generatedEntryNote( + entry({ source: "custom", generated: true }), + ); + assert.ok(note); + assert.ok(!note.includes("undefined")); + assert.ok(!note.includes("null")); + }); }); // ── countAgentsReferencingHarness ───────────────────────────────────────────── diff --git a/desktop/src/features/settings/ui/harnessGalleryLogic.ts b/desktop/src/features/settings/ui/harnessGalleryLogic.ts index 0c67ebf65d7..c4fee0ac7e5 100644 --- a/desktop/src/features/settings/ui/harnessGalleryLogic.ts +++ b/desktop/src/features/settings/ui/harnessGalleryLogic.ts @@ -8,10 +8,32 @@ import type { AcpRuntimeCatalogEntry } from "@/shared/api/types"; /** * Returns true iff the given catalog entry is editable by the user. - * Only `source === "custom"` entries are editable/deletable. + * + * Only `source === "custom"` entries are editable/deletable, and a *generated* + * entry is not: it exists only while its parent definition's `variants` block + * keeps detecting that profile, so a local edit would be discarded on the next + * scan. The way to change one is to edit the definition that produced it, or to + * stop it matching the marker; `generatedEntryNote` says so in the row. */ export function isEditableEntry(entry: AcpRuntimeCatalogEntry): boolean { - return entry.source === "custom"; + return entry.source === "custom" && entry.generated !== true; +} + +/** + * One-line explanation of where a generated entry comes from, or null for + * entries the user edits directly. + * + * Names the parent definition when the backend reports it (`generatedFrom`): + * "edit the definition" is useless advice without knowing which one. + */ +export function generatedEntryNote( + entry: AcpRuntimeCatalogEntry, +): string | null { + if (entry.generated !== true) return null; + const from = entry.generatedFrom?.trim(); + return from + ? `Generated from the "${from}" harness definition. Change that definition to change this entry.` + : "Generated from a harness definition that declares profile variants. Change that definition to change this entry."; } /** diff --git a/desktop/src/shared/api/tauri.ts b/desktop/src/shared/api/tauri.ts index e3de3d54046..522b016914d 100644 --- a/desktop/src/shared/api/tauri.ts +++ b/desktop/src/shared/api/tauri.ts @@ -35,6 +35,7 @@ import type { InstallRuntimeResult, GitBashPrerequisite, RuntimeConfigSurface, + HarnessVariants, } from "@/shared/api/types"; export * from "@/shared/api/tauriChannels"; @@ -192,6 +193,14 @@ export type RawAcpRuntimeCatalogEntry = { source: "builtin" | "preset" | "custom"; /** Definition-level env vars for `source: custom` entries; absent for builtin/preset. */ definition_env?: Record; + /** Who chooses the model for agents on this harness; absent for builtin/preset. */ + model_selection?: "user" | "harness"; + /** Definition-level profile-variant template for `source: custom` entries. */ + definition_variants?: HarnessVariants; + /** True when the entry was generated from another definition's `variants` block. */ + generated?: boolean; + /** Id of the definition that generated this entry; absent for authored entries. */ + generated_from?: string; max_parallelism?: number; effort_canonical_values?: string[] | null; }; @@ -683,6 +692,12 @@ export function fromRawAcpRuntimeCatalogEntry( loginHint: entry.login_hint ?? null, source: entry.source, definitionEnv: entry.definition_env ?? {}, + modelSelection: entry.model_selection ?? "user", + definitionVariants: entry.definition_variants, + generated: entry.generated ?? false, + ...(entry.generated_from !== undefined && { + generatedFrom: entry.generated_from, + }), effortCanonicalValues: entry.effort_canonical_values ?? null, ...(entry.max_parallelism !== undefined && { maxParallelism: entry.max_parallelism, diff --git a/desktop/src/shared/api/types.ts b/desktop/src/shared/api/types.ts index 95bbdd96429..568eebdc794 100644 --- a/desktop/src/shared/api/types.ts +++ b/desktop/src/shared/api/types.ts @@ -540,10 +540,69 @@ export type AcpRuntimeCatalogEntry = { * builtin/preset entries. */ definitionEnv?: Record; + /** + * Who chooses the LLM model for agents on this harness. `"harness"` means the + * harness decides (a profile directory, its own config file), so the UI hides + * the model picker for agents pinned to this entry. Absent for builtin and + * preset entries, which are always user-selected. + */ + modelSelection?: "user" | "harness"; + /** + * Definition-level profile-variant template for `source: custom` entries. + * Carried so a save through the harness form cannot erase a hand-authored + * block the form does not edit. + */ + definitionVariants?: HarnessVariants; + /** + * True when this entry was materialized from another definition's `variants` + * block (a detected profile). Generated entries are read-only: they are edited + * and deleted through the definition file that produced them. Absent on every + * other entry, and absent from entries produced before this field existed, so + * consumers must treat `undefined` as false. + */ + generated?: boolean; + /** + * Id of the definition whose `variants` block produced this entry. Present + * only alongside `generated: true`; names the file the user must edit. + */ + generatedFrom?: string; /** Spawn-time parallelism cap; absent for uncapped harnesses. */ maxParallelism?: number; }; +/** + * Profile-variant expansion for a custom harness definition: one definition file + * then covers a whole family of harnesses that differ only by directory. + * + * Mirrors `HarnessVariants` in `src-tauri/src/managed_agents/custom_harnesses.rs`. + */ +export type HarnessVariants = { + /** Directory whose immediate subdirectories are the profiles. Supports `~`. */ + dir: string; + /** File that must exist inside a subdirectory for it to count as a profile. */ + marker?: string; + /** Cap on expanded entries; clamped to 64. */ + max?: number; + /** Variant id template. `{id}`, `{slug}`. Defaults to `"{id}-{slug}"`. */ + idTemplate?: string; + /** Variant label template. `{label}`, `{name}`, `{meta}`. Defaults to + * `"{label} ({name})"`, or `"{label} [{meta}] ({name})"` when `labelFrom` is + * set. `{meta}` renders as the empty string when the value is missing, so a + * template conditional on it never prints empty brackets. */ + labelTemplate?: string; + /** + * Optional label source read from a file inside each profile directory. + * `file` must resolve inside the profile directory (no absolute paths, no + * `..`); `key` is a dotted path into the document (`ui_meta.hermes-bots.title`). + * Missing file, missing key, or an unparseable document yields no metadata. + */ + labelFrom?: { file: string; key: string }; + /** Extra env for each variant. `{name}`, `{slug}`, `{dir}`, `{root}`. */ + env?: Record; + /** Extra args appended to the template's args; same placeholders. */ + args?: string[]; +}; + /** An AcpRuntimeCatalogEntry that is confirmed available — command and binaryPath are non-null. */ export type AcpRuntime = AcpRuntimeCatalogEntry & { availability: "available"; diff --git a/desktop/tests/e2e/harness-profile-variants-screenshots.spec.ts b/desktop/tests/e2e/harness-profile-variants-screenshots.spec.ts new file mode 100644 index 00000000000..c883e49ce58 --- /dev/null +++ b/desktop/tests/e2e/harness-profile-variants-screenshots.spec.ts @@ -0,0 +1,211 @@ +import { expect, test } from "@playwright/test"; + +import { installMockBridge } from "../helpers/bridge"; +import { openSettings } from "../helpers/settings"; + +const SHOTS = "test-results/harness-profile-variants"; + +/** + * Profile-variant harness definitions. + * + * A custom definition may declare a `variants` block (a profile directory plus a + * marker file); the Rust catalog then materializes one entry per detected + * profile. Two UI consequences follow from the definition, and both are pinned + * here because a component could regress them without any Rust test noticing: + * + * 1. generated entries are read-only in Settings -> Agents -> Harnesses and + * name the definition file to edit instead (`harnessGalleryLogic`); + * 2. an entry whose definition declares `model_selection: "harness"` owns its + * own model, so the agent dialog omits the model picker (`agentConfigCore` + * reason `ownedByHarnessSelection`). + */ + +type RawEntry = Record; + +/** Shape shared by every available catalog entry in this fixture. */ +function availableEntry(overrides: RawEntry): RawEntry { + return { + avatar_url: "", + availability: "available", + default_args: [], + mcp_command: null, + install_hint: "", + install_instructions_url: "https://hermes-agent.nousresearch.com/docs", + can_auto_install: false, + underlying_cli_path: null, + node_required: false, + auth_status: { status: "not_applicable" }, + ...overrides, + }; +} + +/** The hand-authored definition a user writes: one file, one variants block. */ +const TEMPLATE = availableEntry({ + id: "hermes-profiles", + label: "Hermes Agent (profiles)", + command: "hermes-acp", + binary_path: "/usr/local/bin/hermes-acp", + source: "custom", + model_selection: "harness", + definition_variants: { + dir: "~/.hermes/profiles", + marker: "profile.yaml", + labelFrom: { file: "profile.yaml", key: "ui_meta.hermes-bots.title" }, + env: { HERMES_HOME: "{dir}" }, + }, +}); + +/** One entry per profile directory the Rust expander found. */ +function generatedEntry(slug: string, label: string): RawEntry { + return availableEntry({ + id: `hermes-profile-${slug}`, + label, + command: "hermes-acp", + binary_path: "/usr/local/bin/hermes-acp", + source: "custom", + model_selection: "harness", + generated: true, + generated_from: "hermes-profiles", + }); +} + +const GENERATED = [ + generatedEntry("generalist", "Hermes Agent [Generalist] (generalist)"), + generatedEntry("default", "Hermes Agent [Developer] (default)"), +]; + +/** A hand-authored custom harness: no variants block, still user-editable. */ +const HAND_AUTHORED = availableEntry({ + id: "my-custom", + label: "My Custom Harness", + command: "my-acp", + binary_path: "/usr/local/bin/my-acp", + source: "custom", +}); + +const BUZZ_AGENT = availableEntry({ + id: "buzz-agent", + label: "Buzz Agent", + command: "buzz-agent", + binary_path: "/usr/local/bin/buzz-agent", +}); + +const CATALOG = [BUZZ_AGENT, TEMPLATE, ...GENERATED, HAND_AUTHORED]; + +async function openHarnessesSettings(page: import("@playwright/test").Page) { + await page.goto("/", { waitUntil: "domcontentloaded" }); + await openSettings(page, "agents"); + await expect(page.getByTestId("settings-harnesses")).toBeVisible({ + timeout: 10_000, + }); + await expect(page.locator(".animate-spin").first()).not.toBeVisible({ + timeout: 5_000, + }); +} + +/** Menu-based PersonaDropdownField, not a native