From 4f362f04738014d6b5d59229147e31aa943e5358 Mon Sep 17 00:00:00 2001 From: Hayt <41ea58f1e64c243627e8acde7c89be667052ee6e17d8f021c1195be4324ebf04@buzz.block.builderlab.xyz> Date: Wed, 12 Aug 2026 21:48:05 -0400 Subject: [PATCH] refactor(desktop): split custom_harnesses tests into sibling file custom_harnesses.rs is about to grow with keyring env projection, so lift its unit tests into a #[path]-included custom_harnesses_tests.rs sibling to stay under the desktop 1000-line file-size ratchet. Pure movement: production code is byte-identical, and the 43 test functions move unchanged into the sibling module. Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- .../src/managed_agents/custom_harnesses.rs | 740 +---------------- .../managed_agents/custom_harnesses_tests.rs | 741 ++++++++++++++++++ 2 files changed, 743 insertions(+), 738 deletions(-) create mode 100644 desktop/src-tauri/src/managed_agents/custom_harnesses_tests.rs diff --git a/desktop/src-tauri/src/managed_agents/custom_harnesses.rs b/desktop/src-tauri/src/managed_agents/custom_harnesses.rs index ba0448beaf..3329674f65 100644 --- a/desktop/src-tauri/src/managed_agents/custom_harnesses.rs +++ b/desktop/src-tauri/src/managed_agents/custom_harnesses.rs @@ -491,741 +491,5 @@ pub(crate) fn save_custom_harness_to_dir( } #[cfg(test)] -mod tests { - use super::*; - use std::fs; - - // ── ID validation ──────────────────────────────────────────────────────── - - #[test] - fn valid_id_lowercase_with_hyphen() { - assert!(is_valid_harness_id("my-agent")); - } - - #[test] - fn valid_id_underscore_start() { - assert!(is_valid_harness_id("_my_agent")); - } - - #[test] - fn valid_id_alphanumeric() { - assert!(is_valid_harness_id("agent42")); - } - - #[test] - fn invalid_id_uppercase() { - assert!(!is_valid_harness_id("MyAgent")); - } - - #[test] - fn invalid_id_starts_with_hyphen() { - assert!(!is_valid_harness_id("-bad-id")); - } - - #[test] - fn invalid_id_empty() { - assert!(!is_valid_harness_id("")); - } - - #[test] - fn invalid_id_path_traversal() { - assert!(!is_valid_harness_id("../etc/passwd")); - } - - // ── Collision check ────────────────────────────────────────────────────── - - #[test] - fn builtin_ids_are_rejected() { - // Tier-1 hard-coded IDs must always be reserved. - for id in &["goose", "claude", "codex", "buzz-agent"] { - assert!(check_id_collision(id).is_err(), "{id} should be rejected"); - } - // Tier-2 preset IDs must also be reserved (derived from PRESET_HARNESSES). - for id in crate::managed_agents::discovery::preset_harness_ids() { - assert!(check_id_collision(id).is_err(), "{id} should be rejected"); - } - } - - #[test] - fn unknown_id_passes_collision_check() { - assert!(check_id_collision("my-custom-agent").is_ok()); - } - - // ── File loading ───────────────────────────────────────────────────────── - - #[test] - fn load_valid_json_returns_definition() { - let dir = tempfile::tempdir().unwrap(); - fs::write( - dir.path().join("my-agent.json"), - r#"{"id":"my-agent","label":"My Agent","command":"my-agent-bin"}"#, - ) - .unwrap(); - - let defs = load_custom_harnesses(dir.path()); - assert_eq!(defs.len(), 1); - assert_eq!(defs[0].id, "my-agent"); - assert_eq!(defs[0].label, "My Agent"); - assert_eq!(defs[0].command, "my-agent-bin"); - } - - #[test] - fn load_skips_non_json_files() { - let dir = tempfile::tempdir().unwrap(); - fs::write(dir.path().join("my-agent.toml"), r#"id = "my-agent""#).unwrap(); - - let defs = load_custom_harnesses(dir.path()); - assert_eq!(defs.len(), 0, "non-JSON file should be ignored"); - } - - #[test] - fn load_skips_invalid_json_without_panicking() { - let dir = tempfile::tempdir().unwrap(); - fs::write(dir.path().join("bad.json"), "{ not valid json").unwrap(); - - // Must not panic or propagate an error. - let defs = load_custom_harnesses(dir.path()); - assert_eq!(defs.len(), 0); - } - - #[test] - fn load_skips_definition_with_invalid_id() { - let dir = tempfile::tempdir().unwrap(); - fs::write( - dir.path().join("Bad.json"), - r#"{"id":"Bad-Id","label":"Bad","command":"bad"}"#, - ) - .unwrap(); - - let defs = load_custom_harnesses(dir.path()); - assert_eq!( - defs.len(), - 0, - "invalid id should cause the entry to be skipped" - ); - } - - #[test] - fn load_skips_definition_with_empty_command() { - let dir = tempfile::tempdir().unwrap(); - fs::write( - dir.path().join("empty-cmd.json"), - r#"{"id":"empty-cmd","label":"Empty","command":""}"#, - ) - .unwrap(); - - let defs = load_custom_harnesses(dir.path()); - assert_eq!( - defs.len(), - 0, - "empty command should cause the entry to be skipped" - ); - } - - #[test] - fn load_skips_definition_with_non_http_install_url() { - // installInstructionsUrl must start with https:// or http://. - // A bare path, javascript: URI, or other scheme is rejected. - let dir = tempfile::tempdir().unwrap(); - fs::write( - dir.path().join("bad-url.json"), - r#"{"id":"bad-url","label":"Bad","command":"bad-bin","installInstructionsUrl":"file:///etc/passwd"}"#, - ) - .unwrap(); - - let defs = load_custom_harnesses(dir.path()); - assert_eq!( - defs.len(), - 0, - "non-http install URL should cause the entry to be skipped" - ); - } - - #[test] - fn load_accepts_empty_or_https_install_url() { - let dir = tempfile::tempdir().unwrap(); - // Empty URL is fine (optional field). - fs::write( - dir.path().join("no-url.json"), - r#"{"id":"no-url","label":"No URL","command":"no-url-bin"}"#, - ) - .unwrap(); - // https:// is accepted. - fs::write( - dir.path().join("good-url.json"), - r#"{"id":"good-url","label":"Good URL","command":"good-bin","installInstructionsUrl":"https://example.com/install"}"#, - ) - .unwrap(); - - let defs = load_custom_harnesses(dir.path()); - assert_eq!( - defs.len(), - 2, - "empty and https:// URLs must both be accepted" - ); - } - - #[test] - fn load_missing_dir_returns_empty_vec() { - let dir = tempfile::tempdir().unwrap(); - let nonexistent = dir.path().join("does_not_exist"); - - let defs = load_custom_harnesses(&nonexistent); - assert_eq!(defs.len(), 0); - } - - #[test] - fn load_continues_after_one_bad_entry() { - let dir = tempfile::tempdir().unwrap(); - fs::write(dir.path().join("bad.json"), "!!!").unwrap(); - fs::write( - dir.path().join("good.json"), - r#"{"id":"good-one","label":"Good","command":"good-binary"}"#, - ) - .unwrap(); - - let defs = load_custom_harnesses(dir.path()); - assert_eq!(defs.len(), 1, "bad entry skipped, good entry loaded"); - assert_eq!(defs[0].id, "good-one"); - } - - #[test] - fn load_applies_id_collision_check() { - // A custom file whose id shadows a built-in ("goose") must be dropped - // BY THE LOADER — `load_custom_harnesses` is the enforcement boundary - // shared by both the warm path and discovery. This exercises the real - // loader against a real file, not just the helper predicate. - let dir = tempfile::tempdir().unwrap(); - fs::write( - dir.path().join("goose.json"), - r#"{"id":"goose","label":"Not Goose","command":"goose","args":["--evil"]}"#, - ) - .unwrap(); - assert!( - load_custom_harnesses(dir.path()).is_empty(), - "loader must drop a file shadowing a builtin id" - ); - assert!(check_id_collision("goose").is_err()); - assert!(check_id_collision("custom-goose").is_ok()); - } - - #[test] - fn load_dedups_duplicate_ids_first_file_wins() { - // Two files carrying the same custom id: the loader must keep exactly - // one definition (directory-order first wins; the duplicate is dropped). - let dir = tempfile::tempdir().unwrap(); - fs::write( - dir.path().join("a.json"), - r#"{"id":"custom-dup","label":"First","command":"first-cmd"}"#, - ) - .unwrap(); - fs::write( - dir.path().join("b.json"), - r#"{"id":"custom-dup","label":"Second","command":"second-cmd"}"#, - ) - .unwrap(); - let loaded = load_custom_harnesses(dir.path()); - assert_eq!( - loaded.len(), - 1, - "loader must dedup duplicate ids within the directory" - ); - 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![], - env: BTreeMap::new(), - install_instructions_url: String::new(), - install_hint: String::new(), - } - } - - #[test] - fn save_to_dir_create_writes_file_and_loads_back() { - let dir = tempfile::tempdir().unwrap(); - let def = make_def("my-harness", "My Harness"); - - let outcome = save_custom_harness_to_dir(dir.path(), &def, None).unwrap(); - - assert_eq!(outcome.target_path, dir.path().join("my-harness.json")); - assert!(outcome.removed_old_path.is_none(), "no old file on create"); - - let loaded = load_custom_harnesses(dir.path()); - assert_eq!(loaded.len(), 1); - assert_eq!(loaded[0].id, "my-harness"); - assert_eq!(loaded[0].label, "My Harness"); - } - - #[test] - fn save_to_dir_same_id_edit_replaces_content() { - let dir = tempfile::tempdir().unwrap(); - 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()); - assert_eq!(loaded.len(), 1, "same-id edit must not duplicate entries"); - assert_eq!(loaded[0].label, "V2 Label", "v2 content must be present"); - } - - #[test] - fn save_to_dir_backup_is_cleaned_up_after_same_id_edit() { - let dir = tempfile::tempdir().unwrap(); - let v1 = make_def("my-harness", "V1"); - save_custom_harness_to_dir(dir.path(), &v1, None).unwrap(); - - 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(), - ".bak file must be removed after successful same-id edit" - ); - } - - #[test] - fn save_to_dir_rename_removes_old_file_and_creates_new() { - let dir = tempfile::tempdir().unwrap(); - 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); - assert_eq!(loaded[0].id, "new-id"); - } - - #[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). - 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(), - "NotFound old-id must not be reported as removed" - ); - assert!(load_custom_harnesses(dir.path()).len() == 1); - } - - #[test] - fn save_to_dir_roundtrip_with_env_preserves_values() { - let dir = tempfile::tempdir().unwrap(); - let mut env = BTreeMap::new(); - env.insert("MY_KEY".to_string(), "my_value".to_string()); - let def = HarnessDefinition { - id: "env-harness".to_string(), - label: "Env Harness".to_string(), - command: "env-bin".to_string(), - args: vec!["--flag".to_string()], - env, - install_instructions_url: "https://example.com".to_string(), - install_hint: "Install from example.com".to_string(), - }; - - save_custom_harness_to_dir(dir.path(), &def, None).unwrap(); - - let loaded = load_custom_harnesses(dir.path()); - assert_eq!(loaded.len(), 1); - assert_eq!(loaded[0].args, vec!["--flag"]); - 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: - // the key contains '=' so Command::env would produce - // `BUZZ_AUTH_TAG=x=forged` in the child env. - let mut env = BTreeMap::new(); - env.insert("BUZZ_AUTH_TAG=x".to_string(), "forged".to_string()); - let def = HarnessDefinition { - id: "bad-env".to_string(), - label: "Bad".to_string(), - command: "bad-bin".to_string(), - args: vec![], - env, - install_instructions_url: String::new(), - install_hint: String::new(), - }; - let err = validate_harness_definition_pub(&def).unwrap_err(); - assert!( - err.contains("env var keys must match"), - "malformed key must be rejected: {err}" - ); - assert!( - err.contains("BUZZ_AUTH_TAG"), - "error must name the offending key: {err}" - ); - } - - #[test] - fn validate_rejects_reserved_key_buzz_managed_agent() { - // BUZZ_MANAGED_AGENT and BUZZ_MANAGED_AGENT_START_NONCE are the - // ownership markers — supplying them in a definition must be rejected. - let mut env = BTreeMap::new(); - env.insert( - "BUZZ_MANAGED_AGENT".to_string(), - "fake-instance".to_string(), - ); - let def = HarnessDefinition { - id: "bad-marker".to_string(), - label: "Bad".to_string(), - command: "bad-bin".to_string(), - args: vec![], - env, - install_instructions_url: String::new(), - install_hint: String::new(), - }; - let err = validate_harness_definition_pub(&def).unwrap_err(); - assert!( - err.contains("reserved by Buzz"), - "ownership marker key must be rejected: {err}" - ); - } - - #[test] - fn validate_rejects_reserved_key_case_insensitive() { - // BUZZ_PRIVATE_KEY in any casing must be blocked. - let mut env = BTreeMap::new(); - env.insert("buzz_private_key".to_string(), "secret".to_string()); - let def = HarnessDefinition { - id: "ci-marker".to_string(), - label: "CI".to_string(), - command: "ci-bin".to_string(), - args: vec![], - env, - install_instructions_url: String::new(), - install_hint: String::new(), - }; - let err = validate_harness_definition_pub(&def).unwrap_err(); - assert!( - err.contains("reserved by Buzz"), - "reserved key must be blocked case-insensitively: {err}" - ); - } - - #[test] - fn validate_rejects_nul_byte_in_value() { - // A NUL in a value would cause Command::env to panic at spawn time. - let mut env = BTreeMap::new(); - env.insert("MY_KEY".to_string(), "val\x00ue".to_string()); - let def = HarnessDefinition { - id: "nul-val".to_string(), - label: "NUL".to_string(), - command: "nul-bin".to_string(), - args: vec![], - env, - install_instructions_url: String::new(), - install_hint: String::new(), - }; - let err = validate_harness_definition_pub(&def).unwrap_err(); - assert!( - err.contains("NUL bytes"), - "NUL value must be rejected at validation: {err}" - ); - } - - #[test] - fn validate_rejects_value_over_per_value_size_limit() { - use crate::managed_agents::env_vars::MAX_ENV_VALUE_BYTES; - let mut env = BTreeMap::new(); - // One byte over the per-value cap. - env.insert("BIG_VAL".to_string(), "x".repeat(MAX_ENV_VALUE_BYTES + 1)); - let def = HarnessDefinition { - id: "big-val".to_string(), - label: "Big".to_string(), - command: "big-bin".to_string(), - args: vec![], - env, - install_instructions_url: String::new(), - install_hint: String::new(), - }; - let err = validate_harness_definition_pub(&def).unwrap_err(); - assert!( - err.contains("per-value limit"), - "oversized value must be rejected: {err}" - ); - } - - #[test] - fn validate_accepts_well_formed_env() { - let mut env = BTreeMap::new(); - env.insert("ANTHROPIC_API_KEY".to_string(), "sk-test-123".to_string()); - env.insert("MODEL_VERSION".to_string(), "claude-3".to_string()); - let def = HarnessDefinition { - id: "good-env".to_string(), - label: "Good".to_string(), - command: "good-bin".to_string(), - args: vec![], - env, - install_instructions_url: String::new(), - install_hint: String::new(), - }; - assert!( - validate_harness_definition_pub(&def).is_ok(), - "well-formed definition must pass validation" - ); - } - - // ── Comma-in-args validation (transport-lossiness guard) ───────────────── - - /// A definition whose args contain a literal comma must be rejected at the - /// validation boundary — the comma-delimited `BUZZ_ACP_AGENT_ARGS` - /// transport would silently split it into two args at spawn time. - #[test] - fn validate_rejects_comma_in_args() { - let mut def = make_def("comma-args", "Comma"); - def.args = vec!["--name".to_string(), "a,b".to_string()]; - let err = validate_harness_definition_pub(&def).unwrap_err(); - assert!( - err.contains("comma"), - "error must explain the comma transport limit, got: {err}" - ); - } - - /// Comma-free args pass — including args with spaces and special chars. - #[test] - fn validate_accepts_comma_free_args() { - let mut def = make_def("clean-args", "Clean"); - def.args = vec!["acp".to_string(), "--flag=x y".to_string()]; - assert!(validate_harness_definition_pub(&def).is_ok()); - } - - /// The loader shares the same validator: a hand-authored file with a comma - /// in args is skipped, so the invariant holds regardless of how the - /// definition arrives (UI save or hand-edited JSON). - #[test] - fn load_skips_definition_with_comma_in_args() { - let dir = tempfile::tempdir().unwrap(); - fs::write( - dir.path().join("comma.json"), - r#"{"id":"comma-file","label":"Comma","command":"comma-bin","args":["a,b"]}"#, - ) - .unwrap(); - assert!( - load_custom_harnesses(dir.path()).is_empty(), - "comma-in-args definition must be skipped at the loader boundary" - ); - } - - // ── Discovery publish under persist_mutex (stale-snapshot regression) ──── - - /// Discovery's registry publish must re-read the directory at publish time - /// (under the persist mutex), not push a snapshot taken before the auth - /// probes ran. Regression shape: discovery scans dir → user saves harness X - /// (save_and_warm warms the registry with X) → discovery finishes. If - /// discovery published its pre-save snapshot, X would be on disk but - /// unresolvable at spawn until the next discover. - /// - /// Deterministic interleaving: we simulate it by calling the publish seam - /// (`warm_harness_registry_locked`) after a save that happened "during" - /// discovery — the fresh-read semantics mean the just-saved definition - /// survives the publish. - #[test] - fn discovery_publish_after_concurrent_save_keeps_saved_harness() { - let _lock = registry_test_lock(); - let dir = tempfile::tempdir().unwrap(); - - // Discovery "scans" the dir while it is empty (stale snapshot would be []). - let stale_snapshot = load_custom_harnesses(dir.path()); - assert!(stale_snapshot.is_empty()); - - // A save lands mid-discovery (save_and_warm: write + warm). - let def = make_def("mid-save", "Mid Save"); - save_and_warm(dir.path(), &def, None).unwrap(); - assert!(lookup_loaded_harness_by_id("mid-save").is_some()); - - // Discovery publishes — the locked warm re-reads the directory, so the - // just-saved harness must survive (a stale-snapshot publish would - // clobber it). - warm_harness_registry_locked(Some(dir.path())); - assert!( - lookup_loaded_harness_by_id("mid-save").is_some(), - "publish must re-read the directory, not clobber the mid-discovery save" - ); - } - - /// Same shape for delete: a delete landing mid-discovery must not be - /// resurrected by the discovery publish. - #[test] - fn discovery_publish_after_concurrent_delete_keeps_harness_gone() { - let _lock = registry_test_lock(); - let dir = tempfile::tempdir().unwrap(); - - let def = make_def("mid-delete", "Mid Delete"); - save_and_warm(dir.path(), &def, None).unwrap(); - - // Discovery "scans" while the file exists (stale snapshot would contain it). - let stale_snapshot = load_custom_harnesses(dir.path()); - assert_eq!(stale_snapshot.len(), 1); - - // Delete lands mid-discovery. - delete_and_warm(dir.path(), "mid-delete").unwrap(); - assert!(lookup_loaded_harness_by_id("mid-delete").is_none()); - - // Discovery publishes — fresh read keeps it gone. - warm_harness_registry_locked(Some(dir.path())); - assert!( - lookup_loaded_harness_by_id("mid-delete").is_none(), - "publish must not resurrect a harness deleted mid-discovery" - ); - } - - // ── Registry warm path ─────────────────────────────────────────────────── - - /// After `warm_harness_registry_from_dir` the registry contains preset + - /// custom definitions and `lookup_loaded_harness_by_id` resolves them. - #[test] - fn warm_registry_then_lookup_finds_custom_and_preset_entries() { - let _lock = registry_test_lock(); - let dir = tempfile::tempdir().unwrap(); - fs::write( - dir.path().join("my-custom.json"), - r#"{"id":"my-custom","label":"My Custom","command":"my-custom-bin"}"#, - ) - .unwrap(); - - warm_harness_registry_from_dir(Some(dir.path())); - - // Custom entry must be findable. - let found = lookup_loaded_harness_by_id("my-custom"); - assert!( - found.is_some(), - "warm registry must contain the custom entry" - ); - assert_eq!(found.unwrap().command, "my-custom-bin"); - - // At least one preset entry must be in the registry (e.g. "cursor"). - let preset = lookup_loaded_harness_by_id("cursor"); - assert!( - preset.is_some(), - "warm registry must contain preset entries" - ); - } - - /// `warm_harness_registry_from_dir` with `None` still loads presets. - #[test] - fn warm_registry_with_no_custom_dir_loads_presets_only() { - let _lock = registry_test_lock(); - warm_harness_registry_from_dir(None); - // At least the "cursor" preset must be present. - assert!( - lookup_loaded_harness_by_id("cursor").is_some(), - "presets must be reachable even without a custom dir" - ); - } - - /// `warm_harness_registry_from_dir` followed by `update_loaded_harness_registry` - /// with an empty slice clears the registry (transactional save/delete contract). - #[test] - fn warm_then_clear_registry_empties_lookup() { - let _lock = registry_test_lock(); - let dir = tempfile::tempdir().unwrap(); - fs::write( - dir.path().join("tmp-agent.json"), - r#"{"id":"tmp-agent","label":"Tmp","command":"tmp-bin"}"#, - ) - .unwrap(); - - warm_harness_registry_from_dir(Some(dir.path())); - assert!(lookup_loaded_harness_by_id("tmp-agent").is_some()); - - // Simulate delete — re-warm with empty dir. - let empty_dir = tempfile::tempdir().unwrap(); - warm_harness_registry_from_dir(Some(empty_dir.path())); - assert!( - lookup_loaded_harness_by_id("tmp-agent").is_none(), - "deleted harness must not appear after re-warm" - ); - } - - // ── Legacy avatarUrl regression (F1) ───────────────────────────────────── - - /// A JSON file that contains a legacy `avatarUrl` field (from pre-BYOH code) - /// must still deserialize without error (unknown-field handling) and the - /// loaded `HarnessDefinition` must NOT carry the URL — the field is absent - /// from the struct so serde drops it. - #[test] - fn legacy_avatar_url_in_json_is_silently_dropped_on_load() { - let dir = tempfile::tempdir().unwrap(); - fs::write( - dir.path().join("legacy.json"), - r#"{ - "id": "legacy-agent", - "label": "Legacy Agent", - "command": "legacy-bin", - "avatarUrl": "https://tracking.example.com/logo.png" - }"#, - ) - .unwrap(); - - let defs = load_custom_harnesses(dir.path()); - // The file must deserialize successfully (serde ignores unknown fields). - assert_eq!(defs.len(), 1, "legacy file with avatarUrl must still load"); - assert_eq!(defs[0].id, "legacy-agent"); - // HarnessDefinition has no avatar_url field — prove the URL cannot - // be routed to a catalog entry by serializing back and checking. - let json = serde_json::to_string(&defs[0]).unwrap(); - assert!( - !json.contains("https://tracking.example.com"), - "serialized HarnessDefinition must not contain the legacy avatar URL" - ); - } - - // ── Preset id reservation ──────────────────────────────────────────────── - - /// All preset ids must be blocked by `check_id_collision`. - #[test] - fn preset_ids_are_reserved_and_cannot_be_used_as_custom_ids() { - // Derived from PRESET_HARNESSES — no hard-coded copy here so this test - // automatically covers any future preset additions. - for id in crate::managed_agents::discovery::preset_harness_ids() { - assert!( - check_id_collision(id).is_err(), - "preset id {id:?} should be rejected by check_id_collision" - ); - } - } -} +#[path = "custom_harnesses_tests.rs"] +mod tests; diff --git a/desktop/src-tauri/src/managed_agents/custom_harnesses_tests.rs b/desktop/src-tauri/src/managed_agents/custom_harnesses_tests.rs new file mode 100644 index 0000000000..09273f036a --- /dev/null +++ b/desktop/src-tauri/src/managed_agents/custom_harnesses_tests.rs @@ -0,0 +1,741 @@ +//! Unit tests for `managed_agents/custom_harnesses.rs`. +//! +//! Kept in a sibling file so `custom_harnesses.rs` stays closer to the +//! 1000-line gate; `#[path]`-included from there. + +use super::*; +use std::fs; + +// ── ID validation ──────────────────────────────────────────────────────── + +#[test] +fn valid_id_lowercase_with_hyphen() { + assert!(is_valid_harness_id("my-agent")); +} + +#[test] +fn valid_id_underscore_start() { + assert!(is_valid_harness_id("_my_agent")); +} + +#[test] +fn valid_id_alphanumeric() { + assert!(is_valid_harness_id("agent42")); +} + +#[test] +fn invalid_id_uppercase() { + assert!(!is_valid_harness_id("MyAgent")); +} + +#[test] +fn invalid_id_starts_with_hyphen() { + assert!(!is_valid_harness_id("-bad-id")); +} + +#[test] +fn invalid_id_empty() { + assert!(!is_valid_harness_id("")); +} + +#[test] +fn invalid_id_path_traversal() { + assert!(!is_valid_harness_id("../etc/passwd")); +} + +// ── Collision check ────────────────────────────────────────────────────── + +#[test] +fn builtin_ids_are_rejected() { + // Tier-1 hard-coded IDs must always be reserved. + for id in &["goose", "claude", "codex", "buzz-agent"] { + assert!(check_id_collision(id).is_err(), "{id} should be rejected"); + } + // Tier-2 preset IDs must also be reserved (derived from PRESET_HARNESSES). + for id in crate::managed_agents::discovery::preset_harness_ids() { + assert!(check_id_collision(id).is_err(), "{id} should be rejected"); + } +} + +#[test] +fn unknown_id_passes_collision_check() { + assert!(check_id_collision("my-custom-agent").is_ok()); +} + +// ── File loading ───────────────────────────────────────────────────────── + +#[test] +fn load_valid_json_returns_definition() { + let dir = tempfile::tempdir().unwrap(); + fs::write( + dir.path().join("my-agent.json"), + r#"{"id":"my-agent","label":"My Agent","command":"my-agent-bin"}"#, + ) + .unwrap(); + + let defs = load_custom_harnesses(dir.path()); + assert_eq!(defs.len(), 1); + assert_eq!(defs[0].id, "my-agent"); + assert_eq!(defs[0].label, "My Agent"); + assert_eq!(defs[0].command, "my-agent-bin"); +} + +#[test] +fn load_skips_non_json_files() { + let dir = tempfile::tempdir().unwrap(); + fs::write(dir.path().join("my-agent.toml"), r#"id = "my-agent""#).unwrap(); + + let defs = load_custom_harnesses(dir.path()); + assert_eq!(defs.len(), 0, "non-JSON file should be ignored"); +} + +#[test] +fn load_skips_invalid_json_without_panicking() { + let dir = tempfile::tempdir().unwrap(); + fs::write(dir.path().join("bad.json"), "{ not valid json").unwrap(); + + // Must not panic or propagate an error. + let defs = load_custom_harnesses(dir.path()); + assert_eq!(defs.len(), 0); +} + +#[test] +fn load_skips_definition_with_invalid_id() { + let dir = tempfile::tempdir().unwrap(); + fs::write( + dir.path().join("Bad.json"), + r#"{"id":"Bad-Id","label":"Bad","command":"bad"}"#, + ) + .unwrap(); + + let defs = load_custom_harnesses(dir.path()); + assert_eq!( + defs.len(), + 0, + "invalid id should cause the entry to be skipped" + ); +} + +#[test] +fn load_skips_definition_with_empty_command() { + let dir = tempfile::tempdir().unwrap(); + fs::write( + dir.path().join("empty-cmd.json"), + r#"{"id":"empty-cmd","label":"Empty","command":""}"#, + ) + .unwrap(); + + let defs = load_custom_harnesses(dir.path()); + assert_eq!( + defs.len(), + 0, + "empty command should cause the entry to be skipped" + ); +} + +#[test] +fn load_skips_definition_with_non_http_install_url() { + // installInstructionsUrl must start with https:// or http://. + // A bare path, javascript: URI, or other scheme is rejected. + let dir = tempfile::tempdir().unwrap(); + fs::write( + dir.path().join("bad-url.json"), + r#"{"id":"bad-url","label":"Bad","command":"bad-bin","installInstructionsUrl":"file:///etc/passwd"}"#, + ) + .unwrap(); + + let defs = load_custom_harnesses(dir.path()); + assert_eq!( + defs.len(), + 0, + "non-http install URL should cause the entry to be skipped" + ); +} + +#[test] +fn load_accepts_empty_or_https_install_url() { + let dir = tempfile::tempdir().unwrap(); + // Empty URL is fine (optional field). + fs::write( + dir.path().join("no-url.json"), + r#"{"id":"no-url","label":"No URL","command":"no-url-bin"}"#, + ) + .unwrap(); + // https:// is accepted. + fs::write( + dir.path().join("good-url.json"), + r#"{"id":"good-url","label":"Good URL","command":"good-bin","installInstructionsUrl":"https://example.com/install"}"#, + ) + .unwrap(); + + let defs = load_custom_harnesses(dir.path()); + assert_eq!( + defs.len(), + 2, + "empty and https:// URLs must both be accepted" + ); +} + +#[test] +fn load_missing_dir_returns_empty_vec() { + let dir = tempfile::tempdir().unwrap(); + let nonexistent = dir.path().join("does_not_exist"); + + let defs = load_custom_harnesses(&nonexistent); + assert_eq!(defs.len(), 0); +} + +#[test] +fn load_continues_after_one_bad_entry() { + let dir = tempfile::tempdir().unwrap(); + fs::write(dir.path().join("bad.json"), "!!!").unwrap(); + fs::write( + dir.path().join("good.json"), + r#"{"id":"good-one","label":"Good","command":"good-binary"}"#, + ) + .unwrap(); + + let defs = load_custom_harnesses(dir.path()); + assert_eq!(defs.len(), 1, "bad entry skipped, good entry loaded"); + assert_eq!(defs[0].id, "good-one"); +} + +#[test] +fn load_applies_id_collision_check() { + // A custom file whose id shadows a built-in ("goose") must be dropped + // BY THE LOADER — `load_custom_harnesses` is the enforcement boundary + // shared by both the warm path and discovery. This exercises the real + // loader against a real file, not just the helper predicate. + let dir = tempfile::tempdir().unwrap(); + fs::write( + dir.path().join("goose.json"), + r#"{"id":"goose","label":"Not Goose","command":"goose","args":["--evil"]}"#, + ) + .unwrap(); + assert!( + load_custom_harnesses(dir.path()).is_empty(), + "loader must drop a file shadowing a builtin id" + ); + assert!(check_id_collision("goose").is_err()); + assert!(check_id_collision("custom-goose").is_ok()); +} + +#[test] +fn load_dedups_duplicate_ids_first_file_wins() { + // Two files carrying the same custom id: the loader must keep exactly + // one definition (directory-order first wins; the duplicate is dropped). + let dir = tempfile::tempdir().unwrap(); + fs::write( + dir.path().join("a.json"), + r#"{"id":"custom-dup","label":"First","command":"first-cmd"}"#, + ) + .unwrap(); + fs::write( + dir.path().join("b.json"), + r#"{"id":"custom-dup","label":"Second","command":"second-cmd"}"#, + ) + .unwrap(); + let loaded = load_custom_harnesses(dir.path()); + assert_eq!( + loaded.len(), + 1, + "loader must dedup duplicate ids within the directory" + ); + 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![], + env: BTreeMap::new(), + install_instructions_url: String::new(), + install_hint: String::new(), + } +} + +#[test] +fn save_to_dir_create_writes_file_and_loads_back() { + let dir = tempfile::tempdir().unwrap(); + let def = make_def("my-harness", "My Harness"); + + let outcome = save_custom_harness_to_dir(dir.path(), &def, None).unwrap(); + + assert_eq!(outcome.target_path, dir.path().join("my-harness.json")); + assert!(outcome.removed_old_path.is_none(), "no old file on create"); + + let loaded = load_custom_harnesses(dir.path()); + assert_eq!(loaded.len(), 1); + assert_eq!(loaded[0].id, "my-harness"); + assert_eq!(loaded[0].label, "My Harness"); +} + +#[test] +fn save_to_dir_same_id_edit_replaces_content() { + let dir = tempfile::tempdir().unwrap(); + 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()); + assert_eq!(loaded.len(), 1, "same-id edit must not duplicate entries"); + assert_eq!(loaded[0].label, "V2 Label", "v2 content must be present"); +} + +#[test] +fn save_to_dir_backup_is_cleaned_up_after_same_id_edit() { + let dir = tempfile::tempdir().unwrap(); + let v1 = make_def("my-harness", "V1"); + save_custom_harness_to_dir(dir.path(), &v1, None).unwrap(); + + 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(), + ".bak file must be removed after successful same-id edit" + ); +} + +#[test] +fn save_to_dir_rename_removes_old_file_and_creates_new() { + let dir = tempfile::tempdir().unwrap(); + 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); + assert_eq!(loaded[0].id, "new-id"); +} + +#[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). + 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(), + "NotFound old-id must not be reported as removed" + ); + assert!(load_custom_harnesses(dir.path()).len() == 1); +} + +#[test] +fn save_to_dir_roundtrip_with_env_preserves_values() { + let dir = tempfile::tempdir().unwrap(); + let mut env = BTreeMap::new(); + env.insert("MY_KEY".to_string(), "my_value".to_string()); + let def = HarnessDefinition { + id: "env-harness".to_string(), + label: "Env Harness".to_string(), + command: "env-bin".to_string(), + args: vec!["--flag".to_string()], + env, + install_instructions_url: "https://example.com".to_string(), + install_hint: "Install from example.com".to_string(), + }; + + save_custom_harness_to_dir(dir.path(), &def, None).unwrap(); + + let loaded = load_custom_harnesses(dir.path()); + assert_eq!(loaded.len(), 1); + assert_eq!(loaded[0].args, vec!["--flag"]); + 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: + // the key contains '=' so Command::env would produce + // `BUZZ_AUTH_TAG=x=forged` in the child env. + let mut env = BTreeMap::new(); + env.insert("BUZZ_AUTH_TAG=x".to_string(), "forged".to_string()); + let def = HarnessDefinition { + id: "bad-env".to_string(), + label: "Bad".to_string(), + command: "bad-bin".to_string(), + args: vec![], + env, + install_instructions_url: String::new(), + install_hint: String::new(), + }; + let err = validate_harness_definition_pub(&def).unwrap_err(); + assert!( + err.contains("env var keys must match"), + "malformed key must be rejected: {err}" + ); + assert!( + err.contains("BUZZ_AUTH_TAG"), + "error must name the offending key: {err}" + ); +} + +#[test] +fn validate_rejects_reserved_key_buzz_managed_agent() { + // BUZZ_MANAGED_AGENT and BUZZ_MANAGED_AGENT_START_NONCE are the + // ownership markers — supplying them in a definition must be rejected. + let mut env = BTreeMap::new(); + env.insert( + "BUZZ_MANAGED_AGENT".to_string(), + "fake-instance".to_string(), + ); + let def = HarnessDefinition { + id: "bad-marker".to_string(), + label: "Bad".to_string(), + command: "bad-bin".to_string(), + args: vec![], + env, + install_instructions_url: String::new(), + install_hint: String::new(), + }; + let err = validate_harness_definition_pub(&def).unwrap_err(); + assert!( + err.contains("reserved by Buzz"), + "ownership marker key must be rejected: {err}" + ); +} + +#[test] +fn validate_rejects_reserved_key_case_insensitive() { + // BUZZ_PRIVATE_KEY in any casing must be blocked. + let mut env = BTreeMap::new(); + env.insert("buzz_private_key".to_string(), "secret".to_string()); + let def = HarnessDefinition { + id: "ci-marker".to_string(), + label: "CI".to_string(), + command: "ci-bin".to_string(), + args: vec![], + env, + install_instructions_url: String::new(), + install_hint: String::new(), + }; + let err = validate_harness_definition_pub(&def).unwrap_err(); + assert!( + err.contains("reserved by Buzz"), + "reserved key must be blocked case-insensitively: {err}" + ); +} + +#[test] +fn validate_rejects_nul_byte_in_value() { + // A NUL in a value would cause Command::env to panic at spawn time. + let mut env = BTreeMap::new(); + env.insert("MY_KEY".to_string(), "val\x00ue".to_string()); + let def = HarnessDefinition { + id: "nul-val".to_string(), + label: "NUL".to_string(), + command: "nul-bin".to_string(), + args: vec![], + env, + install_instructions_url: String::new(), + install_hint: String::new(), + }; + let err = validate_harness_definition_pub(&def).unwrap_err(); + assert!( + err.contains("NUL bytes"), + "NUL value must be rejected at validation: {err}" + ); +} + +#[test] +fn validate_rejects_value_over_per_value_size_limit() { + use crate::managed_agents::env_vars::MAX_ENV_VALUE_BYTES; + let mut env = BTreeMap::new(); + // One byte over the per-value cap. + env.insert("BIG_VAL".to_string(), "x".repeat(MAX_ENV_VALUE_BYTES + 1)); + let def = HarnessDefinition { + id: "big-val".to_string(), + label: "Big".to_string(), + command: "big-bin".to_string(), + args: vec![], + env, + install_instructions_url: String::new(), + install_hint: String::new(), + }; + let err = validate_harness_definition_pub(&def).unwrap_err(); + assert!( + err.contains("per-value limit"), + "oversized value must be rejected: {err}" + ); +} + +#[test] +fn validate_accepts_well_formed_env() { + let mut env = BTreeMap::new(); + env.insert("ANTHROPIC_API_KEY".to_string(), "sk-test-123".to_string()); + env.insert("MODEL_VERSION".to_string(), "claude-3".to_string()); + let def = HarnessDefinition { + id: "good-env".to_string(), + label: "Good".to_string(), + command: "good-bin".to_string(), + args: vec![], + env, + install_instructions_url: String::new(), + install_hint: String::new(), + }; + assert!( + validate_harness_definition_pub(&def).is_ok(), + "well-formed definition must pass validation" + ); +} + +// ── Comma-in-args validation (transport-lossiness guard) ───────────────── + +/// A definition whose args contain a literal comma must be rejected at the +/// validation boundary — the comma-delimited `BUZZ_ACP_AGENT_ARGS` +/// transport would silently split it into two args at spawn time. +#[test] +fn validate_rejects_comma_in_args() { + let mut def = make_def("comma-args", "Comma"); + def.args = vec!["--name".to_string(), "a,b".to_string()]; + let err = validate_harness_definition_pub(&def).unwrap_err(); + assert!( + err.contains("comma"), + "error must explain the comma transport limit, got: {err}" + ); +} + +/// Comma-free args pass — including args with spaces and special chars. +#[test] +fn validate_accepts_comma_free_args() { + let mut def = make_def("clean-args", "Clean"); + def.args = vec!["acp".to_string(), "--flag=x y".to_string()]; + assert!(validate_harness_definition_pub(&def).is_ok()); +} + +/// The loader shares the same validator: a hand-authored file with a comma +/// in args is skipped, so the invariant holds regardless of how the +/// definition arrives (UI save or hand-edited JSON). +#[test] +fn load_skips_definition_with_comma_in_args() { + let dir = tempfile::tempdir().unwrap(); + fs::write( + dir.path().join("comma.json"), + r#"{"id":"comma-file","label":"Comma","command":"comma-bin","args":["a,b"]}"#, + ) + .unwrap(); + assert!( + load_custom_harnesses(dir.path()).is_empty(), + "comma-in-args definition must be skipped at the loader boundary" + ); +} + +// ── Discovery publish under persist_mutex (stale-snapshot regression) ──── + +/// Discovery's registry publish must re-read the directory at publish time +/// (under the persist mutex), not push a snapshot taken before the auth +/// probes ran. Regression shape: discovery scans dir → user saves harness X +/// (save_and_warm warms the registry with X) → discovery finishes. If +/// discovery published its pre-save snapshot, X would be on disk but +/// unresolvable at spawn until the next discover. +/// +/// Deterministic interleaving: we simulate it by calling the publish seam +/// (`warm_harness_registry_locked`) after a save that happened "during" +/// discovery — the fresh-read semantics mean the just-saved definition +/// survives the publish. +#[test] +fn discovery_publish_after_concurrent_save_keeps_saved_harness() { + let _lock = registry_test_lock(); + let dir = tempfile::tempdir().unwrap(); + + // Discovery "scans" the dir while it is empty (stale snapshot would be []). + let stale_snapshot = load_custom_harnesses(dir.path()); + assert!(stale_snapshot.is_empty()); + + // A save lands mid-discovery (save_and_warm: write + warm). + let def = make_def("mid-save", "Mid Save"); + save_and_warm(dir.path(), &def, None).unwrap(); + assert!(lookup_loaded_harness_by_id("mid-save").is_some()); + + // Discovery publishes — the locked warm re-reads the directory, so the + // just-saved harness must survive (a stale-snapshot publish would + // clobber it). + warm_harness_registry_locked(Some(dir.path())); + assert!( + lookup_loaded_harness_by_id("mid-save").is_some(), + "publish must re-read the directory, not clobber the mid-discovery save" + ); +} + +/// Same shape for delete: a delete landing mid-discovery must not be +/// resurrected by the discovery publish. +#[test] +fn discovery_publish_after_concurrent_delete_keeps_harness_gone() { + let _lock = registry_test_lock(); + let dir = tempfile::tempdir().unwrap(); + + let def = make_def("mid-delete", "Mid Delete"); + save_and_warm(dir.path(), &def, None).unwrap(); + + // Discovery "scans" while the file exists (stale snapshot would contain it). + let stale_snapshot = load_custom_harnesses(dir.path()); + assert_eq!(stale_snapshot.len(), 1); + + // Delete lands mid-discovery. + delete_and_warm(dir.path(), "mid-delete").unwrap(); + assert!(lookup_loaded_harness_by_id("mid-delete").is_none()); + + // Discovery publishes — fresh read keeps it gone. + warm_harness_registry_locked(Some(dir.path())); + assert!( + lookup_loaded_harness_by_id("mid-delete").is_none(), + "publish must not resurrect a harness deleted mid-discovery" + ); +} + +// ── Registry warm path ─────────────────────────────────────────────────── + +/// After `warm_harness_registry_from_dir` the registry contains preset + +/// custom definitions and `lookup_loaded_harness_by_id` resolves them. +#[test] +fn warm_registry_then_lookup_finds_custom_and_preset_entries() { + let _lock = registry_test_lock(); + let dir = tempfile::tempdir().unwrap(); + fs::write( + dir.path().join("my-custom.json"), + r#"{"id":"my-custom","label":"My Custom","command":"my-custom-bin"}"#, + ) + .unwrap(); + + warm_harness_registry_from_dir(Some(dir.path())); + + // Custom entry must be findable. + let found = lookup_loaded_harness_by_id("my-custom"); + assert!( + found.is_some(), + "warm registry must contain the custom entry" + ); + assert_eq!(found.unwrap().command, "my-custom-bin"); + + // At least one preset entry must be in the registry (e.g. "cursor"). + let preset = lookup_loaded_harness_by_id("cursor"); + assert!( + preset.is_some(), + "warm registry must contain preset entries" + ); +} + +/// `warm_harness_registry_from_dir` with `None` still loads presets. +#[test] +fn warm_registry_with_no_custom_dir_loads_presets_only() { + let _lock = registry_test_lock(); + warm_harness_registry_from_dir(None); + // At least the "cursor" preset must be present. + assert!( + lookup_loaded_harness_by_id("cursor").is_some(), + "presets must be reachable even without a custom dir" + ); +} + +/// `warm_harness_registry_from_dir` followed by `update_loaded_harness_registry` +/// with an empty slice clears the registry (transactional save/delete contract). +#[test] +fn warm_then_clear_registry_empties_lookup() { + let _lock = registry_test_lock(); + let dir = tempfile::tempdir().unwrap(); + fs::write( + dir.path().join("tmp-agent.json"), + r#"{"id":"tmp-agent","label":"Tmp","command":"tmp-bin"}"#, + ) + .unwrap(); + + warm_harness_registry_from_dir(Some(dir.path())); + assert!(lookup_loaded_harness_by_id("tmp-agent").is_some()); + + // Simulate delete — re-warm with empty dir. + let empty_dir = tempfile::tempdir().unwrap(); + warm_harness_registry_from_dir(Some(empty_dir.path())); + assert!( + lookup_loaded_harness_by_id("tmp-agent").is_none(), + "deleted harness must not appear after re-warm" + ); +} + +// ── Legacy avatarUrl regression (F1) ───────────────────────────────────── + +/// A JSON file that contains a legacy `avatarUrl` field (from pre-BYOH code) +/// must still deserialize without error (unknown-field handling) and the +/// loaded `HarnessDefinition` must NOT carry the URL — the field is absent +/// from the struct so serde drops it. +#[test] +fn legacy_avatar_url_in_json_is_silently_dropped_on_load() { + let dir = tempfile::tempdir().unwrap(); + fs::write( + dir.path().join("legacy.json"), + r#"{ + "id": "legacy-agent", + "label": "Legacy Agent", + "command": "legacy-bin", + "avatarUrl": "https://tracking.example.com/logo.png" + }"#, + ) + .unwrap(); + + let defs = load_custom_harnesses(dir.path()); + // The file must deserialize successfully (serde ignores unknown fields). + assert_eq!(defs.len(), 1, "legacy file with avatarUrl must still load"); + assert_eq!(defs[0].id, "legacy-agent"); + // HarnessDefinition has no avatar_url field — prove the URL cannot + // be routed to a catalog entry by serializing back and checking. + let json = serde_json::to_string(&defs[0]).unwrap(); + assert!( + !json.contains("https://tracking.example.com"), + "serialized HarnessDefinition must not contain the legacy avatar URL" + ); +} + +// ── Preset id reservation ──────────────────────────────────────────────── + +/// All preset ids must be blocked by `check_id_collision`. +#[test] +fn preset_ids_are_reserved_and_cannot_be_used_as_custom_ids() { + // Derived from PRESET_HARNESSES — no hard-coded copy here so this test + // automatically covers any future preset additions. + for id in crate::managed_agents::discovery::preset_harness_ids() { + assert!( + check_id_collision(id).is_err(), + "preset id {id:?} should be rejected by check_id_collision" + ); + } +}