diff --git a/crates/buzz-acp/README.md b/crates/buzz-acp/README.md index d9cd362cb8..957b0d8244 100644 --- a/crates/buzz-acp/README.md +++ b/crates/buzz-acp/README.md @@ -252,6 +252,65 @@ Each channel has at most one prompt in flight. Multiple channels can be processe > **Note:** On startup, the harness replays all unprocessed @mentions since the last run. Expect a burst of activity if there are stale events in the channel. +## Bring Your Own Harness (BYOH) + +Buzz Desktop supports registering any ACP-speaking agent tool as a selectable runtime without a PR. + +### How it works + +**Tier-1 — compiled-in runtimes** (Goose, Claude Code, Codex, Buzz Agent): have auto-installers, auth probes, and first-class onboarding. Their IDs (`goose`, `claude`, `codex`, `buzz-agent`) are reserved and cannot be overridden. + +**Tier-2 — preset catalog** (Cursor, Oh My Pi, Grok Build, OpenCode, Kimi Code, Amp, Hermes Agent, OpenClaw): static `HarnessDefinition` entries in `desktop/src-tauri/src/managed_agents/discovery.rs` (`PRESET_HARNESSES`). They are always present in the runtime catalog, PATH-probed for availability, not editable or deletable by the user. Displayed with bundled logos; if not installed, a docs link appears instead. + +> **Note — OpenClaw:** `openclaw acp` is a Gateway-backed bridge; PATH availability shows "Available" even when the OpenClaw Gateway daemon is not running. This is expected tier-2 semantics (same class as a preset with unconfigured auth). The Gateway URL is configured via `OPENCLAW_GATEWAY_URL` (or the equivalent env var from OpenClaw's docs) — set it in the agent's **env vars** in Edit Agent, not in the definition env (the preset definition carries no env entries). Note that `openclaw acp` executes tools inside the Gateway daemon, not the Desktop process, so Desktop-injected `BUZZ_*` env vars do NOT reach the execution locus unless you also set them on the Gateway's own environment. + +**Tier-3 — user custom harnesses**: JSON files in `/custom_harnesses/` that the user can create from the Settings UI or drop in directly. Each file describes one harness — no install scripts. + +### Custom harness JSON schema + +```json +{ + "id": "my-agent", + "label": "My Agent", + "command": "my-agent-bin", + "args": ["acp"], + "env": { + "MY_AGENT_MODE": "acp" + }, + "installInstructionsUrl": "https://example.com/docs", + "installHint": "Download from example.com" +} +``` + +Fields: +- `id` — `[a-z0-9_][a-z0-9_-]*` (used as the runtime picker value and file name) +- `label` — human-readable name shown in the UI +- `command` — the executable name or absolute path (must be non-empty) +- `args` — optional default CLI arguments (array); instance-level args override this when non-empty +- `env` — optional environment variables injected at spawn time (definition env is a floor; user/persona/global env overrides it; Buzz-reserved keys like `BUZZ_MANAGED_AGENT` are always stripped and cannot be overridden) +- `installInstructionsUrl` / `installHint` — shown when the binary is not on PATH + +Invalid files (bad JSON, unknown id, empty command) are skipped with a warning and do not break discovery for other entries. + +### Security guarantees + +- No install shell commands in preset or custom definitions — only the user's own PATH is consulted. +- `can_auto_install` is always `false` for preset and custom entries. +- No user-supplied icon URLs — icons are bundled assets keyed by id in `RuntimeIcon.tsx`. +- `BUZZ_MANAGED_AGENT` and other Buzz identity keys cannot be overridden by `env` in a custom definition; they are stripped before merging. + +### Adding a preset (contributor guide) + +To add a new runtime to the tier-2 gallery: + +1. **Verify the ACP entrypoint** from the vendor's own documentation — do not rely on a PR description alone. Test with the actual binary. +2. **Add a `HarnessDefinition` entry** to the `PRESET_HARNESSES` slice in `desktop/src-tauri/src/managed_agents/discovery.rs`. Fill `id`, `label`, `command`, `args`, `install_instructions_url`, `install_hint`. Leave `env` empty unless the harness requires a specific env var to enable ACP mode. +3. **Add the preset id to `BUILTIN_IDS`** in `desktop/src-tauri/src/managed_agents/custom_harnesses.rs` so custom JSON files cannot shadow it. +4. **Add a bundled logo** (64×64 PNG or optimised SVG) to `desktop/public/harness-logos/.png` and add a corresponding entry to `PRESET_LOGOS` in `desktop/src/features/onboarding/ui/RuntimeIcon.tsx`. Record the source and license in `desktop/public/harness-logos/CREDITS.md`. Only bundle a mark whose upstream license permits redistribution; skipping this step is caught by `presetLogos.test.mjs`, which asserts every `PRESET_HARNESSES` id has a mapped logo that exists on disk. +5. Run `cargo test --lib` and `just desktop-typecheck` to verify everything compiles. + +The built-in `BUILTIN_IDS` set (`goose`, `claude`, `codex`, `buzz-agent`, and all current preset ids) is the reserved namespace; every other id is available for custom harnesses. + ## Using Any ACP Agent The harness works with any agent that implements the [ACP spec](https://agentclientprotocol.com/) over stdio. The requirements are: diff --git a/desktop/playwright.config.ts b/desktop/playwright.config.ts index e59c68bfaf..ba35481bd0 100644 --- a/desktop/playwright.config.ts +++ b/desktop/playwright.config.ts @@ -120,6 +120,7 @@ export default defineConfig({ "**/inbox-live-update.spec.ts", "**/mesh-compute.spec.ts", "**/observer-archive-policy.spec.ts", + "**/harness-management.spec.ts", ], use: { ...devices["Desktop Chrome"], diff --git a/desktop/public/harness-logos/CREDITS.md b/desktop/public/harness-logos/CREDITS.md new file mode 100644 index 0000000000..654a6b7428 --- /dev/null +++ b/desktop/public/harness-logos/CREDITS.md @@ -0,0 +1,22 @@ +# Preset harness logos — provenance + +Third-party marks bundled to identify tier-2 preset harnesses in the runtime +gallery (`PRESET_LOGOS` in `desktop/src/features/onboarding/ui/RuntimeIcon.tsx`). +Nominative use only — each mark identifies its own vendor's harness. + +Add a row here when adding a preset logo; only bundle marks whose upstream +license permits redistribution. + +| File | Upstream | Commit | License | Source path | Modifications | +|---|---|---|---|---|---| +| `hermes.png` | [NousResearch/hermes-agent](https://github.com/NousResearch/hermes-agent) | `6ad632b` | MIT © 2025 Nous Research | `website/static/img/logo.png` | Cropped the baked-in border frame, padded to square, resized to 64×64, quantised to a 16-colour palette | +| `openclaw.svg` | [openclaw/openclaw](https://github.com/openclaw/openclaw) | `b06f40a` | MIT © 2026 OpenClaw Foundation | `ui/public/favicon.svg` | Removed the SMIL animation elements (renders the upstream rest pose statically — verified pixel-identical to the upstream frame at t=0); minified paths | +| `omp.svg` | [can1357/oh-my-pi](https://github.com/can1357/oh-my-pi) | `667111575ebba136dadfd6989379e7f67e0d40d9` | MIT © 2025 Mario Zechner; © 2025–2026 Can Bölük | `assets/icon.svg` | None | +| `kimi.png` | [MoonshotAI/kimi-cli](https://github.com/MoonshotAI/kimi-cli) | `4a550effdfcb29a25a5d325bf935296cc50cd417` | Apache-2.0; NOTICE: Kimi Code CLI © 2025 Moonshot AI | `web/public/logo.png` | None | +| `grok.svg` | [SpaceXAI brand guidelines](https://x.ai/legal/brand-guidelines) | Retrieved 2026-07-25 | xAI Brand Guidelines: marks may be used to accurately refer to xAI or its services; logos must be used exactly as provided | `SpaceXAI_Grok_Assets.zip` → `Grok_Logomark_Dark.svg` | None | + +`amp.png` and `opencode.svg` predate this file; their provenance was not +recorded when they were added. Cursor intentionally uses the generic terminal +fallback: Cursor's official brand page offers downloadable assets, but neither +that page nor its Terms of Service grants third parties permission to +redistribute them. The previous unproven `cursor.png` was removed. diff --git a/desktop/public/harness-logos/amp.png b/desktop/public/harness-logos/amp.png new file mode 100644 index 0000000000..00176e85bf Binary files /dev/null and b/desktop/public/harness-logos/amp.png differ diff --git a/desktop/public/harness-logos/grok.svg b/desktop/public/harness-logos/grok.svg new file mode 100644 index 0000000000..642d7b6b0d --- /dev/null +++ b/desktop/public/harness-logos/grok.svg @@ -0,0 +1,4 @@ + + + + diff --git a/desktop/public/harness-logos/hermes.png b/desktop/public/harness-logos/hermes.png new file mode 100644 index 0000000000..6acf0020b1 Binary files /dev/null and b/desktop/public/harness-logos/hermes.png differ diff --git a/desktop/public/harness-logos/kimi.png b/desktop/public/harness-logos/kimi.png new file mode 100644 index 0000000000..033c0f4fb2 Binary files /dev/null and b/desktop/public/harness-logos/kimi.png differ diff --git a/desktop/public/harness-logos/omp.svg b/desktop/public/harness-logos/omp.svg new file mode 100644 index 0000000000..f1ccf2a08a --- /dev/null +++ b/desktop/public/harness-logos/omp.svg @@ -0,0 +1,16 @@ + + + + + + + + + + + + + + + + diff --git a/desktop/public/harness-logos/openclaw.svg b/desktop/public/harness-logos/openclaw.svg new file mode 100644 index 0000000000..8c1496d622 --- /dev/null +++ b/desktop/public/harness-logos/openclaw.svg @@ -0,0 +1,16 @@ + + + + + + + + + + + + + + + + diff --git a/desktop/public/harness-logos/opencode.svg b/desktop/public/harness-logos/opencode.svg new file mode 100644 index 0000000000..157edc4d75 --- /dev/null +++ b/desktop/public/harness-logos/opencode.svg @@ -0,0 +1,7 @@ + + + + + \ No newline at end of file diff --git a/desktop/scripts/check-file-sizes.mjs b/desktop/scripts/check-file-sizes.mjs index f34b6dc332..19e376881c 100644 --- a/desktop/scripts/check-file-sizes.mjs +++ b/desktop/scripts/check-file-sizes.mjs @@ -131,7 +131,20 @@ const overrides = new Map([ // record_provider param + applies persona_field_with_record_fallback. +5 lines. // global-agent-config: spawn_agent_child loads global config and merges as // lowest env layer (+8 lines). Queued to split. - ["src-tauri/src/managed_agents/runtime.rs", 2216], + // +2: BYOH orphan-sweep fix — `!belongs && !has_buzz_marker` OR-gate replaces + // the old AND-gate so custom harness processes are not silently leaked on crash. + // +27: BYOH F4 fix — extract shared `buzz_sweep_owns_process` predicate, fix + // Linux AND-gate in sweep + orphan collectors, 4 production predicate tests. + // +12: BYOH F2 — record_agent_command / effective_agent_command check loaded + // harness registry for preset/custom ids after static-builtin lookup. + // +61: BYOH pass-2 — I2 (spawn_agent_child env+args from definition), I3 + // (valid_agent_runtime_receipt uses buzz_sweep_owns_process marker-only), + // I6 (cross-platform buzz_sweep_owns_process, drop #[cfg(unix)]), +2 new + // receipt-path collector-decision tests. + // +2: BYOH Phase A — resolve_effective_harness_descriptor single typed resolver; + // C-9 injectable sweep predicates (kill_stale_tracked_processes_with + + // valid_agent_runtime_receipt_with) with 5 discriminating tests. + ["src-tauri/src/managed_agents/runtime.rs", 2326], // config-bridge setup-payload env-boundary fix adds readiness wiring in // spawn_agent_child; load-bearing security fix, queued to split. ["src-tauri/src/managed_agents/config_bridge/reader.rs", 1016], @@ -177,11 +190,15 @@ const overrides = new Map([ // Windows Doctor install fix: cli_install_commands_windows field added to test stubs. // team-instructions-first-class: ManagedAgentRecord fixture gains the new // team_id field (+1 line). - ["src-tauri/src/managed_agents/readiness.rs", 1765], + ["src-tauri/src/managed_agents/readiness.rs", 1863], // Windows PATH-correctness fix: 3 #[cfg(windows)] test functions covering // .cmd shim rejection, .bat shim rejection, and .exe acceptance for // configure_runtime_cli (fix #2397). Test-only growth; queued to split. - ["src-tauri/src/managed_agents/runtime/tests.rs", 1041], + // +34: BYOH custom-harness sweep condition unit tests — 3 tests validating + // the OR-gate fix for custom-binary orphan cleanup. + // +26: BYOH pass-2 I3 — 2 collector-decision tests for receipt path + // ownership (valid_agent_runtime_receipt uses buzz_sweep_owns_process). + ["src-tauri/src/managed_agents/runtime/tests.rs", 1320], // applyWorkspace reposDir parameter plus the validateReposDir binding, // threaded through Tauri invokes for configurable repos_dir, plus the // harness-persona-sync `harnessOverride` create-input bit — load-bearing @@ -231,7 +248,12 @@ const overrides = new Map([ // doc comment) and AgentTeam/CreateTeamInput/UpdateTeamInput.instructions // (+3) — the new team-id spawn link and the runtime-layered instructions // field. - ["src/shared/api/types.ts", 1047], + // byoh-env-roundtrip: AcpRuntimeCatalogEntry.definitionEnv field + JSDoc + // (+12 lines) so the edit form can read back existing env vars on save. + // Load-bearing correctness fix. Queued to split. + // +2: AcpRuntimeCatalogEntry.requiresExternalCli field added by main + // (#2680) to indicate runtimes that need a separate CLI install. + ["src/shared/api/types.ts", 1051], // readiness-gate: PersonaDialog.tsx threads computeLocalModeGate + // requiredCredentialEnvKeys + RequiredFieldLabel so the "New agent" dialog // shows required markers and credential amber rows (parity with @@ -296,7 +318,37 @@ const overrides = new Map([ // Buzz-managed Node path helpers and resolution tests moved to // managed_node_paths.rs and discovery/tests/managed_path_resolution.rs; // ratcheting 1366 -> 1392 after adding the managed-path probes to discovery. - ["src-tauri/src/managed_agents/discovery.rs", 1393], + // +17: BYOH custom harness catalog merge phase-3 — append custom definitions + // from custom_harnesses_dir with PATH-probe availability; source tagging. + // +148: BYOH F2/F3 — PRESET_HARNESSES static data (6 presets), Phase 2.5 in + // discover_acp_runtimes_from (PATH-probe each preset, build catalog entries, + // populate loaded-harness registry), record/effective command resolution now + // checks loaded registry for preset/custom ids. Queued to split presets out. + // +3: BYOH F5 — seen_ids rejects preset/builtin collisions from custom files. + // +79: BYOH pass-2 C1 — 4 registry lifecycle tests (warm→spawn, delete→ + // dangling, immediate save+start, edit with rename); try_record_agent_command + // typed error for dangling ids wired into spawn; readiness/spawn_hash now + // include definition env floor. + // +7: BYOH pass-2 I2 env round-trip — definition_env field populated in + // custom catalog entries + 2 discriminating tests (custom env preserved, + // builtin env empty). Load-bearing edit round-trip fix. + // +16: BYOH scope addition — Hermes Agent + OpenClaw preset entries (two + // data-only PresetHarness structs; no new logic or test functions). + // +29: rebase over main (#2680) — discover_acp_runtime_phase1 extracted + // helper + discover_acp_runtime_availability; both load-bearing for + // post-install verification. Semantic composition with BYOH changes. + // +17: merge of main (#2767) — codex_adapter_is_outdated_with_path split out + // so Codex adapter planning takes an explicit PATH. Auto-merged cleanly; only + // the ceiling needed composing with the BYOH growth above. + ["src-tauri/src/managed_agents/discovery.rs", 1732], + // BYOH — save_custom_harness_to_dir (backup-swap atomic write) + save_and_warm / + // delete_and_warm (persist-mutex serialization for concurrent-safe registry + // refresh, B-6). Also: id/collision/load/registry tests (from the file base) + + // B-4 real persistence tests (create, same-id edit, rename, backup cleanup) + + // B-3 env validation boundary tests (malformed key, reserved shape, NUL, + // size limit, ownership marker). Load-bearing correctness/security coverage; + // queued to extract helper module once the feature stabilizes. + ["src-tauri/src/managed_agents/custom_harnesses.rs", 1045], // rebase over codex-acp-package-swap: its version-probe tests union with the // doctor-install-reliability nvm/login-shell/semver tests — each side alone // stayed under the 1000 default; the union exceeds it. @@ -307,7 +359,11 @@ const overrides = new Map([ // None regression, .cmd shim resolution, no-git-bash error hint. // +32: deterministic .cmd resolver + no-registry + install_shell_from tests. // Managed-path resolution test split to discovery/tests/managed_path_resolution.rs. - ["src-tauri/src/managed_agents/discovery/tests.rs", 1273], + // +227: BYOH pass-2 C1 — 4 registry lifecycle tests (warm→spawn, delete→dangling, + // immediate save+start, edit with rename) added to discovery/tests.rs. + // +64: BYOH pass-2 I2 env round-trip — 2 discriminating tests proving custom + // catalog entries carry definition_env and builtins do not. + ["src-tauri/src/managed_agents/discovery/tests.rs", 1576], // identity-import-keyring: the identity resolution state machine's behavioral // matrix (46 tests over FakeIdentityStore — probe × marker × file cells, // adoption / read-back-corruption / marker-failure arms, recovery-mode @@ -500,9 +556,20 @@ const overrides = new Map([ // Includes unit tests for detection, routing, and -Command body preservation. // +16: test_powershell_command_goose_catalog_dequoted proves the \$→$ escape // fix for the Goose Windows installer (PR #2680 interaction with #2750). - // +10: pass an explicit PATH through Codex adapter install planning so unit - // tests avoid the process-global login-shell PATH cache. - ["src-tauri/src/commands/agent_discovery.rs", 1836], + // +126: BYOH — save_custom_harness (validate, atomic write, return entry) + + // delete_custom_harness (id-guard, builtin reject, remove file) commands; + // discover_acp_providers updated to pass AppHandle + custom_harnesses dir. + // +30: BYOH F5 — atomic-write-file dep, original_id rename/delete support. + // +13: BYOH pass-2 C1 — warm_harness_registry_from_dir call in save and + // delete commands now verifies transactional registry refresh. + // +2: BYOH pass-2 I2 env round-trip — definition_env carried through save + // return value so the frontend immediately has the updated env. + // +1: rebase over main (#2680) — requires_external_cli: false added to + // save_custom_harness catalog entry construction (new required field). + // +10: merge of main (#2767) — pass an explicit PATH through Codex adapter + // install planning so unit tests avoid the process-global login-shell PATH + // cache. Independent of the BYOH growth above; the two deltas compose. + ["src-tauri/src/commands/agent_discovery.rs", 2048], // draft-persistence predicate: submit-time `loadDraft` check + inline comment // + deps-array entry in submitMessage closes the never-persisted-boundary // defect (Thufir Pass-3 finding). Load-bearing correctness fix; queued to diff --git a/desktop/src-tauri/.gitignore b/desktop/src-tauri/.gitignore index 8c86ed783a..4ed297a74c 100644 --- a/desktop/src-tauri/.gitignore +++ b/desktop/src-tauri/.gitignore @@ -7,4 +7,4 @@ /gen/schemas # Sidecar binaries (built by scripts/bundle-sidecars.sh) -/binaries/ +/binaries diff --git a/desktop/src-tauri/Cargo.lock b/desktop/src-tauri/Cargo.lock index 303c5eb1fa..37b2a03ad4 100644 --- a/desktop/src-tauri/Cargo.lock +++ b/desktop/src-tauri/Cargo.lock @@ -1079,6 +1079,7 @@ dependencies = [ "tokio-tungstenite 0.29.0", "tokio-util", "toml 0.8.2", + "tracing", "url", "user-idle", "uuid", diff --git a/desktop/src-tauri/Cargo.toml b/desktop/src-tauri/Cargo.toml index 638f0739da..5825b561b5 100644 --- a/desktop/src-tauri/Cargo.toml +++ b/desktop/src-tauri/Cargo.toml @@ -121,6 +121,7 @@ rubato = "3.0" audioadapter-buffers = "3.0" tempfile = "3" strip-ansi-escapes = "0.2" +tracing = "0.1" [dev-dependencies] # `test-util` enables tokio's paused-clock (`start_paused`) so the relay diff --git a/desktop/src-tauri/src/commands/agent_discovery.rs b/desktop/src-tauri/src/commands/agent_discovery.rs index e3d9b22189..6282a417fa 100644 --- a/desktop/src-tauri/src/commands/agent_discovery.rs +++ b/desktop/src-tauri/src/commands/agent_discovery.rs @@ -69,16 +69,167 @@ pub(crate) fn plan_adapter_install<'c>( } #[tauri::command] -pub async fn discover_acp_providers() -> Result, String> { - tokio::task::spawn_blocking(|| { +pub async fn discover_acp_providers( + app: tauri::AppHandle, +) -> Result, String> { + tokio::task::spawn_blocking(move || { + use tauri::Manager; crate::managed_agents::clear_resolve_cache(); crate::managed_agents::refresh_login_shell_path(); - crate::managed_agents::discover_acp_runtimes() + let custom_dir = app + .path() + .app_data_dir() + .ok() + .map(|d| d.join("custom_harnesses")); + crate::managed_agents::discover_acp_runtimes_from(custom_dir.as_deref()) }) .await .map_err(|e| format!("spawn_blocking failed: {e}")) } +/// Write a user-defined harness definition to `/custom_harnesses/.json`. +/// +/// Validates the definition (id regex, builtin-id collision, non-empty command +/// and label, env well-formedness) before touching the filesystem. Returns the +/// merged catalog entry so the UI can update the provider list without triggering +/// a full re-discover. +/// +/// Write a user-defined harness definition to `/custom_harnesses/.json`. +/// +/// Validates the definition (id regex, builtin-id collision, non-empty command +/// and label) before touching the filesystem. Returns the merged catalog entry +/// so the UI can update the provider list without triggering a full re-discover. +/// +/// `original_id` handles the rename case: when the user edits an existing +/// harness and changes its id, pass the old id here so the old file is removed +/// atomically as part of the write. If the id is unchanged or this is a new +/// harness, omit `original_id` (or pass `None`). +/// +/// The file is written using `atomic-write-file` (unique temp file + commit) +/// so concurrent saves do not race on a fixed temp path, and a partial write +/// never produces a corrupted JSON file. +#[tauri::command] +pub async fn save_custom_harness( + definition: crate::managed_agents::custom_harnesses::HarnessDefinition, + original_id: Option, + app: tauri::AppHandle, +) -> Result { + use crate::managed_agents::{ + custom_harnesses, AcpAvailabilityStatus, AuthStatus, HarnessSource, + }; + use tauri::Manager; + + // ── Phase 1: full validation before touching the filesystem ───────────── + // validate_harness_definition_pub now covers: id format, non-empty command/label, + // env key well-formedness + reserved-key check + NUL/size limits, and + // install_instructions_url scheme. + custom_harnesses::validate_harness_definition_pub(&definition)?; + custom_harnesses::check_id_collision(&definition.id)?; + + // Validate original_id BEFORE any filesystem mutation (validate-before-mutate). + let rename_old_id: Option = original_id.and_then(|oid| { + let oid = oid.trim().to_string(); + if oid.is_empty() || oid == definition.id { + None + } else { + Some(oid) + } + }); + if let Some(ref old_id) = rename_old_id { + custom_harnesses::check_id_collision(old_id) + .map_err(|_| format!("original_id {old_id:?} is a built-in and cannot be deleted"))?; + if !custom_harnesses::is_valid_harness_id_pub(old_id) { + return Err(format!("invalid original_id {old_id:?}")); + } + } + + let custom_dir = app + .path() + .app_data_dir() + .map_err(|e| format!("failed to resolve app data dir: {e}"))? + .join("custom_harnesses"); + std::fs::create_dir_all(&custom_dir) + .map_err(|e| format!("failed to create custom_harnesses dir: {e}"))?; + + // ── Phase 2+3: backup-swap write + rename (Windows-safe, rollback on failure) + // `save_and_warm` holds the persist mutex for the write + registry-warm pair + // so concurrent saves never produce a stale registry snapshot (B-6). + custom_harnesses::save_and_warm(&custom_dir, &definition, rename_old_id.as_deref())?; + + // Resolve availability for the returned catalog entry. + let (availability, command_opt, binary_path) = + match crate::managed_agents::find_command(&definition.command) { + Some(path) => ( + AcpAvailabilityStatus::Available, + Some(definition.command.clone()), + Some(path.display().to_string()), + ), + 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, + // Security: no user-supplied avatar URL in catalog entries. + avatar_url: String::new(), + availability, + command: command_opt, + binary_path, + default_args, + mcp_command: None, + model_env_var: None, + provider_env_var: None, + thinking_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, + // Carry definition env back so the edit form can read and preserve it. + definition_env: definition.env, + }) +} + +/// Remove a user-defined harness definition from `/custom_harnesses/`. +/// +/// Only `source: custom` harnesses may be deleted. Attempting to delete a +/// built-in id (goose, claude, codex, buzz-agent) returns an error without +/// touching the filesystem. +#[tauri::command] +pub async fn delete_custom_harness(id: String, app: tauri::AppHandle) -> Result<(), String> { + use crate::managed_agents::custom_harnesses; + use tauri::Manager; + + // Reject built-in ids early — they have no backing file to delete and + // must never be removable from the catalog. + custom_harnesses::check_id_collision(&id) + .map_err(|_| format!("harness {id:?} is a built-in and cannot be deleted"))?; + + // Validate the id so callers cannot use path-traversal tricks. + if !custom_harnesses::is_valid_harness_id_pub(&id) { + return Err(format!("invalid harness id {id:?}")); + } + + let custom_dir = app + .path() + .app_data_dir() + .map_err(|e| format!("failed to resolve app data dir: {e}"))? + .join("custom_harnesses"); + + // `delete_and_warm` holds the persist mutex for the delete + registry-warm + // pair so concurrent save/delete calls never produce a stale snapshot (B-6). + custom_harnesses::delete_and_warm(&custom_dir, &id)?; + + Ok(()) +} + #[tauri::command] pub async fn install_acp_runtime( runtime_id: String, diff --git a/desktop/src-tauri/src/commands/agent_models.rs b/desktop/src-tauri/src/commands/agent_models.rs index 425eadb5f6..cb3eda698c 100644 --- a/desktop/src-tauri/src/commands/agent_models.rs +++ b/desktop/src-tauri/src/commands/agent_models.rs @@ -11,10 +11,10 @@ use crate::{ app_state::AppState, managed_agents::{ build_managed_agent_summary, current_instance_id, discovery_env_with_baked_floor, - find_managed_agent_mut, known_acp_runtime, load_managed_agents, load_personas, - managed_agent_avatar_url, missing_command_message, normalize_agent_args, resolve_command, - save_managed_agents, sync_managed_agent_processes, try_regenerate_nest, AgentModelInfo, - AgentModelsResponse, UpdateManagedAgentRequest, UpdateManagedAgentResponse, + find_managed_agent_mut, known_acp_runtime, load_global_agent_config, load_managed_agents, + load_personas, managed_agent_avatar_url, missing_command_message, normalize_agent_args, + resolve_command, save_managed_agents, sync_managed_agent_processes, try_regenerate_nest, + AgentModelInfo, AgentModelsResponse, UpdateManagedAgentRequest, UpdateManagedAgentResponse, DEFAULT_ACP_COMMAND, }, relay::{relay_ws_url_with_override, sync_managed_agent_profile}, @@ -62,27 +62,34 @@ pub async fn get_agent_models( // so model discovery runs against the persona's current harness, not the // frozen record snapshot. An explicit per-agent override wins. let personas = load_personas(&app).unwrap_or_default(); - let effective_command = crate::managed_agents::record_agent_command(record, &personas); + let global = load_global_agent_config(&app).unwrap_or_default(); - let args = normalize_agent_args(&effective_command, record.agent_args.clone()); + // Single typed descriptor — same resolver as spawn_agent_child. + // Returns Err on dangling harness id, propagating it to the caller. + let descriptor = + crate::managed_agents::resolve_effective_harness_descriptor(record, &personas, &global) + .map_err(|e| format!("cannot discover models for {pubkey}: {e}"))?; - let resolved_agent = resolve_command(&effective_command) + let resolved_agent = resolve_command(&descriptor.command) .map(|p| p.display().to_string()) - .unwrap_or_else(|| effective_command.clone()); + .unwrap_or_else(|| descriptor.command.clone()); - // ModelPicker can persist a selected model but not rewrite the saved - // provider/env snapshot, and runtime spawn reads that same snapshot. - // Discover models against the record snapshot so an out-of-date persona - // cannot offer models for a provider this agent will not launch with. - let discovery = saved_agent_model_discovery_config(record, &effective_command); + let discovery_model = record.model.clone(); + let discovery_provider = descriptor + .env + .get("BUZZ_AGENT_PROVIDER") + .cloned() + .or_else(|| record.provider.clone()); + let discovery_env = descriptor.env; + let args = descriptor.args; ( resolved, resolved_agent, args, - discovery.model, - discovery.provider, - discovery.env, + discovery_model, + discovery_provider, + discovery_env, ) }; // store lock released — subprocess runs without holding the lock @@ -130,37 +137,6 @@ pub async fn get_agent_models( .await } -#[derive(Debug, PartialEq, Eq)] -struct SavedAgentModelDiscoveryConfig { - model: Option, - provider: Option, - env: BTreeMap, -} - -fn saved_agent_model_discovery_config( - record: &crate::managed_agents::ManagedAgentRecord, - agent_command: &str, -) -> SavedAgentModelDiscoveryConfig { - let mut derived_env = BTreeMap::new(); - if let Some(meta) = known_acp_runtime(agent_command) { - for (key, value) in crate::managed_agents::runtime_metadata_env_vars( - meta.model_env_var, - meta.provider_env_var, - meta.provider_locked, - record.model.as_deref(), - record.provider.as_deref(), - ) { - derived_env.insert(key.to_string(), value.to_string()); - } - } - - SavedAgentModelDiscoveryConfig { - model: record.model.clone(), - provider: record.provider.clone(), - env: crate::managed_agents::merged_user_env(&derived_env, &record.env_vars), - } -} - #[derive(Debug, Deserialize)] #[serde(rename_all = "camelCase")] pub struct DiscoverAgentModelsInput { @@ -173,6 +149,10 @@ pub struct DiscoverAgentModelsInput { pub provider: Option, #[serde(default)] pub env_vars: BTreeMap, + /// Definition-level env from the harness definition (custom/preset). + /// Merged below user `env_vars` so user overrides always win. + #[serde(default)] + pub definition_env: BTreeMap, } /// Query available models from an unsaved agent configuration. @@ -186,6 +166,8 @@ pub async fn discover_agent_models( state: State<'_, AppState>, ) -> Result { crate::managed_agents::validate_user_env_keys(&input.env_vars)?; + // Also validate definition_env (caller-supplied, same trust level as env_vars). + crate::managed_agents::validate_user_env_keys(&input.definition_env)?; let acp_command = input .acp_command @@ -218,7 +200,18 @@ pub async fn discover_agent_models( } } } - let merged_env = crate::managed_agents::merged_user_env(&derived_env, &input.env_vars); + // Layer definition_env below user env_vars so user overrides always win. + // Reserved keys are stripped, matching the same filter applied at spawn. + let mut filtered_definition_env = BTreeMap::new(); + for (key, value) in &input.definition_env { + if !crate::managed_agents::is_reserved_env_key(key) { + filtered_definition_env.insert(key.clone(), value.clone()); + } + } + // Merge: derived (metadata) → definition env → user env_vars. + let merged_with_def = + crate::managed_agents::merged_user_env(&derived_env, &filtered_definition_env); + let merged_env = crate::managed_agents::merged_user_env(&merged_with_def, &input.env_vars); let merged_env = discovery_env_with_baked_floor(merged_env); // Buzz shared compute discovery must not depend on the local OpenAI ingress: that diff --git a/desktop/src-tauri/src/commands/agent_models_tests.rs b/desktop/src-tauri/src/commands/agent_models_tests.rs index 0f99927c9e..863132eaa1 100644 --- a/desktop/src-tauri/src/commands/agent_models_tests.rs +++ b/desktop/src-tauri/src/commands/agent_models_tests.rs @@ -173,6 +173,7 @@ fn saved_agent_model_discovery_uses_record_snapshot() { "relay_url": "wss://localhost:3000", "acp_command": "buzz-acp", "agent_command": "goose", + "agent_command_override": "goose", "agent_args": [], "mcp_command": "", "turn_timeout_seconds": 320, @@ -193,23 +194,30 @@ fn saved_agent_model_discovery_uses_record_snapshot() { ) .expect("sample managed agent record"); - let config = saved_agent_model_discovery_config(&record, "goose"); + // resolve_effective_harness_descriptor is the single resolver used by + // get_agent_models — verify it layers env correctly and strips reserved keys. + let descriptor = crate::managed_agents::resolve_effective_harness_descriptor( + &record, + &[], + &Default::default(), + ) + .expect("descriptor should resolve for a valid record"); - assert_eq!(config.model.as_deref(), Some("record-model")); - assert_eq!(config.provider.as_deref(), Some("databricks")); + assert_eq!(descriptor.command.as_str(), "goose"); assert_eq!( - config.env.get("GOOSE_MODEL").map(String::as_str), + descriptor.env.get("GOOSE_MODEL").map(String::as_str), Some("record-model") ); assert_eq!( - config.env.get("GOOSE_PROVIDER").map(String::as_str), + descriptor.env.get("GOOSE_PROVIDER").map(String::as_str), Some("databricks") ); assert_eq!( - config.env.get("OPENAI_API_KEY").map(String::as_str), + descriptor.env.get("OPENAI_API_KEY").map(String::as_str), Some("record-key") ); - assert!(!config.env.contains_key("BUZZ_PRIVATE_KEY")); + // Reserved keys are stripped from the descriptor env. + assert!(!descriptor.env.contains_key("BUZZ_PRIVATE_KEY")); } // --------------------------------------------------------------------------- diff --git a/desktop/src-tauri/src/lib.rs b/desktop/src-tauri/src/lib.rs index c5b71987ce..64405d0440 100644 --- a/desktop/src-tauri/src/lib.rs +++ b/desktop/src-tauri/src/lib.rs @@ -438,6 +438,21 @@ pub fn run() { eprintln!("buzz-desktop: persona-snapshot backfill failed: {e}"); } + // Warm the loaded-harness registry BEFORE restore so cold-launch + // agent spawns can resolve custom/preset runtime ids without + // waiting for the frontend's discover_acp_providers call. This is + // a pure directory scan — no PATH probing, no async work. + { + let custom_dir = app_handle + .path() + .app_data_dir() + .ok() + .map(|d| d.join("custom_harnesses")); + managed_agents::custom_harnesses::warm_harness_registry_from_dir( + custom_dir.as_deref(), + ); + } + // Store the AppHandle so huddle commands can emit `huddle-state-changed` // events via `huddle::emit_huddle_state` without threading the handle // through every call site. @@ -704,6 +719,8 @@ pub fn run() { discover_acp_providers, discover_git_bash_prerequisite, install_acp_runtime, + save_custom_harness, + delete_custom_harness, connect_acp_runtime, discover_managed_agent_prereqs, sign_event, diff --git a/desktop/src-tauri/src/managed_agents/custom_harnesses.rs b/desktop/src-tauri/src/managed_agents/custom_harnesses.rs new file mode 100644 index 0000000000..8444113981 --- /dev/null +++ b/desktop/src-tauri/src/managed_agents/custom_harnesses.rs @@ -0,0 +1,1038 @@ +//! Loader for user-defined ACP harness definitions. +//! +//! Users drop JSON files into `/custom_harnesses/` to register +//! arbitrary ACP-speaking agents without modifying the app or opening a PR. +//! Each file describes a single harness; the loader validates, warns on +//! invalid entries, and never propagates errors to the discovery caller. +//! +//! **Security constraint (Will-ratified):** custom definitions carry NO install +//! shell commands. `can_auto_install` is always `false` for custom entries. +//! Only tier-1 compiled-in runtimes retain install-script power. +//! +//! **Avatar URL security:** custom/preset catalog entries MUST NOT carry +//! user-supplied avatar URLs. `HarnessDefinition` intentionally omits +//! `avatar_url` — all icons are bundled assets keyed via `RUNTIME_LOGOS`. + +use std::collections::BTreeMap; +use std::path::Path; + +use serde::{Deserialize, Serialize}; + +/// Regex-equivalent predicate for a valid harness ID. +/// +/// IDs must match `[a-z0-9_][a-z0-9_-]*` — lowercase alphanumeric plus +/// hyphens and underscores, starting with an alphanumeric or underscore. +/// This mirrors goose's `generate_id` validation and is intentionally +/// more restrictive than the filesystem to prevent path-traversal tricks. +fn is_valid_harness_id(id: &str) -> bool { + let mut chars = id.chars(); + match chars.next() { + Some(c) if c.is_ascii_lowercase() || c.is_ascii_digit() || c == '_' => {} + _ => return false, + } + chars.all(|c| c.is_ascii_lowercase() || c.is_ascii_digit() || c == '_' || c == '-') +} + +/// Public re-export of `is_valid_harness_id` for callers outside this module +/// (e.g., the `delete_custom_harness` command that must validate caller-supplied ids). +pub(crate) fn is_valid_harness_id_pub(id: &str) -> bool { + is_valid_harness_id(id) +} + +/// User-supplied harness definition deserialized from a JSON file. +/// +/// 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)] +#[serde(rename_all = "camelCase")] +pub(crate) struct HarnessDefinition { + /// Unique identifier, must match `[a-z0-9_][a-z0-9_-]*`. + pub id: String, + /// Human-readable name shown in the UI. + pub label: String, + /// Primary executable name or absolute path. May not be empty. + pub command: String, + /// Default CLI arguments passed to the command (array, not split-string). + #[serde(default)] + pub args: Vec, + /// Environment variables injected at spawn time. Definition env is applied + /// first and LOSES on conflict with Buzz-injected vars — `BUZZ_MANAGED_AGENT` + /// is always authoritative and cannot be overridden here. + #[serde(default)] + pub env: BTreeMap, + /// Link to external docs for manual install/setup instructions. + #[serde(default)] + pub install_instructions_url: String, + /// Human-readable install hint shown in Doctor. + #[serde(default)] + pub install_hint: String, +} + +/// Scan `dir` for `*.json` files and deserialize each into a `HarnessDefinition`. +/// +/// Errors per file are logged with `tracing::warn` and skipped — a single +/// malformed file never fails discovery for the rest. Returns only +/// structurally valid, individually validated definitions. +/// +/// **Callers must supply a fresh `dir` path on every `discover_acp_runtimes` +/// call** — this function performs no caching, mirroring goose's +/// `refresh_custom_providers()` pattern. +pub(crate) fn load_custom_harnesses(dir: &Path) -> Vec { + let entries = match std::fs::read_dir(dir) { + Ok(e) => e, + Err(err) if err.kind() == std::io::ErrorKind::NotFound => return vec![], + Err(err) => { + tracing::warn!( + "custom_harnesses: cannot read directory {}: {err}", + dir.display() + ); + return vec![]; + } + }; + + let mut definitions = Vec::new(); + + for entry in entries.flatten() { + let path = entry.path(); + if path.extension().and_then(|e| e.to_str()) != Some("json") { + continue; + } + + let contents = match std::fs::read_to_string(&path) { + Ok(s) => s, + Err(err) => { + tracing::warn!("custom_harnesses: failed to read {}: {err}", path.display()); + continue; + } + }; + + let def: HarnessDefinition = match serde_json::from_str(&contents) { + Ok(d) => d, + Err(err) => { + tracing::warn!( + "custom_harnesses: invalid JSON in {}: {err}", + path.display() + ); + continue; + } + }; + + if let Err(reason) = validate_harness_definition(&def) { + tracing::warn!("custom_harnesses: skipping {} — {reason}", path.display()); + continue; + } + + definitions.push(def); + } + + definitions +} + +/// Validate a deserialized `HarnessDefinition` against the invariants that +/// the rest of the discovery code depends on. +fn validate_harness_definition(def: &HarnessDefinition) -> Result<(), String> { + if def.id.is_empty() { + return Err("id must not be empty".into()); + } + if !is_valid_harness_id(&def.id) { + return Err(format!( + "id {:?} does not match [a-z0-9_][a-z0-9_-]* — use lowercase letters, digits, hyphens, and underscores only", + def.id + )); + } + if def.command.trim().is_empty() { + return Err("command must not be empty".into()); + } + if def.label.trim().is_empty() { + return Err("label must not be empty".into()); + } + // Validate env keys through the shared boundary validator. This closes the + // reserved-key bypass exploit (BUZZ_AUTH_TAG=x forgery shape), rejects + // NUL bytes that would panic Command::env, and enforces size limits. + crate::managed_agents::env_vars::validate_user_env_keys(&def.env) + .map_err(|e| format!("env: {e}"))?; + // Validate install instructions URL scheme when non-empty. + if !def.install_instructions_url.is_empty() { + let url = def.install_instructions_url.trim(); + if !url.starts_with("https://") && !url.starts_with("http://") { + return Err(format!( + "installInstructionsUrl must start with https:// or http://, got: {:?}", + url + )); + } + } + Ok(()) +} + +/// Public wrapper so the `save_custom_harness` Tauri command can validate +/// without duplicating the rules. +pub(crate) fn validate_harness_definition_pub(def: &HarnessDefinition) -> Result<(), String> { + validate_harness_definition(def) +} + +// ── Built-in ID set ────────────────────────────────────────────────────────── + +/// IDs reserved for the compiled-in catalog. A custom definition whose `id` +/// collides with a built-in or preset is rejected to prevent shadowing (e.g. a +/// file called `cursor.json` hiding the pre-existing tier-2 preset). +/// +/// Derived at compile time from `PRESET_HARNESSES` (tier-2) plus the four +/// tier-1 runtimes — no hand-maintained copy. Adding a preset to +/// `PRESET_HARNESSES` automatically reserves its ID without a separate edit. +fn builtin_ids() -> impl Iterator { + const TIER1: &[&str] = &["goose", "claude", "codex", "buzz-agent"]; + let tier2 = crate::managed_agents::discovery::preset_harness_ids(); + TIER1.iter().copied().chain(tier2.iter().copied()) +} + +/// Return an error string if `id` conflicts with a built-in harness ID. +pub(crate) fn check_id_collision(id: &str) -> Result<(), String> { + if builtin_ids().any(|reserved| reserved.eq_ignore_ascii_case(id)) { + return Err(format!( + "id {:?} is reserved for a built-in harness and cannot be overridden", + id + )); + } + Ok(()) +} + +// ── Loaded harness registry (F2 — spawn resolution for custom/preset) ──────── +// +// `known_acp_runtime` / `known_acp_runtime_exact` only search the static +// `KNOWN_ACP_RUNTIMES` table, so custom and preset harnesses were invisible at +// spawn time, causing silent fallback to buzz-agent. +// +// The fix: `discover_acp_runtimes_from` populates this registry with every +// non-builtin definition after each discovery run. Spawn, readiness, and +// summary paths query `lookup_loaded_harness` to get the live definition for a +// given id or command. If a harness id that an agent references is gone from the +// registry, the caller gets a typed error — never a silent buzz-agent fallback. + +use std::sync::{Arc, RwLock}; + +/// Mutex used by tests to serialize writes to the loaded-harness registry. +/// +/// The registry is a process-global singleton. Parallel test execution can +/// interleave warm → lookup pairs from different tests, causing false failures. +/// Every test that calls `warm_harness_registry_from_dir` or +/// `update_loaded_harness_registry` must hold this guard for the lifetime of +/// its assertion block. +#[cfg(test)] +pub(crate) fn registry_test_lock() -> std::sync::MutexGuard<'static, ()> { + use std::sync::{Mutex, OnceLock}; + static LOCK: OnceLock> = OnceLock::new(); + LOCK.get_or_init(|| Mutex::new(())) + .lock() + .unwrap_or_else(|e| e.into_inner()) +} + +/// Thread-safe registry of non-builtin (preset + custom) harness definitions, +/// populated on every `discover_acp_runtimes_from` call and queried at spawn time. +fn loaded_harness_registry() -> &'static RwLock>> { + use std::sync::OnceLock; + static REGISTRY: OnceLock>>> = OnceLock::new(); + REGISTRY.get_or_init(|| RwLock::new(Vec::new())) +} + +/// Replace the registry contents with `definitions`. Called once per +/// `discover_acp_runtimes_from` run AND on `save_custom_harness` / +/// `delete_custom_harness` so spawn can always resolve the harness without +/// waiting for the next full discovery. +pub(crate) fn update_loaded_harness_registry(definitions: Vec) { + let arcs: Vec> = definitions.into_iter().map(Arc::new).collect(); + // Use `into_inner` to recover from a poisoned lock — the registry is a + // plain replaceable Vec with no torn invariant, so poison recovery is safe. + let mut guard = match loaded_harness_registry().write() { + Ok(g) => g, + Err(poisoned) => { + tracing::warn!("custom_harnesses: loaded-harness registry was poisoned; recovering"); + poisoned.into_inner() + } + }; + *guard = arcs; +} + +/// Look up a loaded (non-builtin) harness by **id**. Returns `None` when the id +/// is unknown. Uses `into_inner` to recover from a poisoned lock so a panic in +/// one thread never permanently blocks all spawn attempts. +pub(crate) fn lookup_loaded_harness_by_id(id: &str) -> Option> { + let guard = match loaded_harness_registry().read() { + Ok(g) => g, + Err(poisoned) => { + tracing::warn!( + "custom_harnesses: loaded-harness registry read lock was poisoned; recovering" + ); + poisoned.into_inner() + } + }; + guard.iter().find(|d| d.id == id).cloned() +} + +/// Warm the loaded-harness registry synchronously from `custom_dir`. +/// +/// Must be called **before** `restore_managed_agents_on_launch` so that cold +/// relaunches can resolve custom/preset harness ids without a full discover +/// round-trip (which is driven by the frontend and arrives later). +/// +/// This is intentionally lightweight: it only loads the custom JSON files and +/// the static preset list — no PATH probing, no availability checks. +pub(crate) fn warm_harness_registry_from_dir(custom_dir: Option<&std::path::Path>) { + // Load only the preset list from the discovery module (static, free). + let preset_defs = crate::managed_agents::discovery::preset_harness_definitions(); + let custom_defs = custom_dir.map(load_custom_harnesses).unwrap_or_default(); + let mut all: Vec = preset_defs; + all.extend(custom_defs); + update_loaded_harness_registry(all); +} + +/// Global mutex that serializes save/delete filesystem mutations and the +/// subsequent registry warm as a single atomic unit. +/// +/// Without this lock, two concurrent `save_custom_harness` calls could +/// interleave: A writes file-A, B writes file-B, B re-warms (sees only B), +/// A re-warms (sees A + B, wins) — but if timing goes the other way B's warm +/// wins and misses A. Holding the lock for the write+warm pair guarantees that +/// the registry always reflects the complete set of files on disk at the time +/// of the warm. +fn persist_mutex() -> &'static std::sync::Mutex<()> { + use std::sync::{Mutex, OnceLock}; + static PERSIST: OnceLock> = OnceLock::new(); + PERSIST.get_or_init(|| Mutex::new(())) +} + +/// Write `definition` to `dir`, then atomically warm the loaded-harness +/// registry. Callers MUST use this function (not `save_custom_harness_to_dir` +/// or `warm_harness_registry_from_dir` separately) so that concurrent saves +/// cannot produce a stale registry snapshot. +pub(crate) fn save_and_warm( + dir: &Path, + definition: &HarnessDefinition, + rename_old_id: Option<&str>, +) -> Result { + let _guard = persist_mutex().lock().unwrap_or_else(|e| e.into_inner()); + let outcome = save_custom_harness_to_dir(dir, definition, rename_old_id)?; + warm_harness_registry_from_dir(Some(dir)); + Ok(outcome) +} + +/// Delete `id.json` from `dir`, then atomically warm the loaded-harness +/// registry. Callers MUST use this function (not `fs::remove_file` + +/// `warm_harness_registry_from_dir` separately) for the same reason as +/// `save_and_warm`. +pub(crate) fn delete_and_warm(dir: &Path, id: &str) -> Result<(), String> { + let _guard = persist_mutex().lock().unwrap_or_else(|e| e.into_inner()); + let target = dir.join(format!("{id}.json")); + match std::fs::remove_file(&target) { + Ok(()) => {} + Err(e) if e.kind() == std::io::ErrorKind::NotFound => {} + Err(e) => return Err(format!("failed to delete harness {id:?}: {e}")), + } + warm_harness_registry_from_dir(Some(dir)); + Ok(()) +} + +/// The outcome of a successful [`save_custom_harness_to_dir`] call. +#[allow(dead_code)] // Fields consumed by tests; production callers use the Result shape. +pub(crate) struct SaveOutcome { + /// The path of the newly written harness file. + pub target_path: std::path::PathBuf, + /// The path of the old file that was removed on a rename, if any. + /// `None` when the id was unchanged or this was a new harness. + pub removed_old_path: Option, +} + +/// Write a harness definition to `dir/.json` using a backup-swap strategy +/// that is safe on all platforms (including Windows where `fs::rename` over an +/// existing file fails with "access denied"): +/// +/// 1. Serialize the definition and write it to a unique temp file via +/// `atomic_write_file`. +/// 2. If the target already exists, rename it to `.bak` (the backup). +/// 3. `commit()` the temp file (renames temp → target). +/// * On success: delete `.bak` (best-effort; a stale `.bak` is harmless). +/// * On failure: restore `.bak` → target so the original is never lost. +/// 4. If `rename_old_id` is `Some`, remove `/.json` after the +/// new file is committed (non-fatal if NotFound). +/// +/// The caller is responsible for full validation (id, env, etc.) BEFORE +/// calling this function — no validation is performed here. +pub(crate) fn save_custom_harness_to_dir( + dir: &Path, + definition: &HarnessDefinition, + rename_old_id: Option<&str>, +) -> Result { + use atomic_write_file::AtomicWriteFile; + use std::io::Write; + + let json = serde_json::to_string_pretty(definition) + .map_err(|e| format!("failed to serialize harness definition: {e}"))?; + + let target_path = dir.join(format!("{}.json", definition.id)); + let bak_path = dir.join(format!("{}.json.bak", definition.id)); + + // Stage write to temp file. + let mut file = AtomicWriteFile::open(&target_path) + .map_err(|e| format!("failed to open {}: {e}", target_path.display()))?; + file.write_all(json.as_bytes()) + .map_err(|e| format!("failed to write harness definition: {e}"))?; + + // Back up the existing target before committing so we can restore on failure. + let had_backup = if target_path.exists() { + std::fs::rename(&target_path, &bak_path).map_err(|e| { + format!( + "failed to back up {} before replace: {e}", + target_path.display() + ) + })?; + true + } else { + false + }; + + // Commit temp → target. If commit fails and we made a backup, restore it. + if let Err(e) = file.commit() { + if had_backup { + if let Err(restore_err) = std::fs::rename(&bak_path, &target_path) { + // Both commit and restore failed — surface both so the user + // can recover manually. + return Err(format!( + "failed to finalize harness definition: {e}; \ + also failed to restore backup: {restore_err} \ + (original may be at {})", + bak_path.display() + )); + } + } + return Err(format!("failed to finalize harness definition: {e}")); + } + + // Commit succeeded — remove the backup (best-effort; stale .bak is harmless). + if had_backup { + let _ = std::fs::remove_file(&bak_path); + } + + // Remove old file on id rename, after the new file is committed. + let mut removed_old_path = None; + if let Some(old_id) = rename_old_id { + let old_path = dir.join(format!("{old_id}.json")); + match std::fs::remove_file(&old_path) { + Ok(()) => removed_old_path = Some(old_path), + Err(e) if e.kind() == std::io::ErrorKind::NotFound => {} + Err(e) => { + // New file is already committed — log and continue so the + // registry re-warm picks up the new id. + tracing::warn!( + "save_custom_harness_to_dir: failed to remove old harness {old_id:?}: {e}" + ); + } + } + } + + Ok(SaveOutcome { + target_path, + removed_old_path, + }) +} + +#[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 named "goose.json" with id "goose" must be rejected. + // The collision check is applied inside discover_acp_runtimes_from, not + // in load_custom_harnesses — the file loader only validates the struct. + // We test the check_id_collision fn directly here. + assert!(check_id_collision("goose").is_err()); + assert!(check_id_collision("custom-goose").is_ok()); + } + + // ── 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" + ); + } + + // ── 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" + ); + } + } +} diff --git a/desktop/src-tauri/src/managed_agents/discovery.rs b/desktop/src-tauri/src/managed_agents/discovery.rs index a2e47d1281..00aac443e8 100644 --- a/desktop/src-tauri/src/managed_agents/discovery.rs +++ b/desktop/src-tauri/src/managed_agents/discovery.rs @@ -7,6 +7,7 @@ use std::time::{Duration, Instant}; use crate::managed_agents::{ buzz_managed_command_path, buzz_managed_node_bin_dir, buzz_managed_npm_bin_dir, AcpAvailabilityStatus, AcpRuntimeCatalogEntry, AuthStatus, CommandAvailabilityInfo, + HarnessSource, }; mod runtime_metadata; @@ -286,6 +287,7 @@ pub fn default_agent_command() -> String { /// 1. explicit override (non-empty) — a deliberate per-instance pin; /// 2. the record's own `runtime` id mapped to its primary command — /// records materialize their runtime at create/migration time; +/// checks both static builtins AND the loaded preset/custom registry; /// 3. legacy fallback: the linked persona's `runtime` (records created /// before the unified model carry `persona_id` but no `runtime`); /// 4. `default_agent_command()`. @@ -302,13 +304,17 @@ pub fn record_agent_command( return pin.to_string(); } - if let Some(command) = record - .runtime - .as_deref() - .and_then(known_acp_runtime_exact) - .and_then(|r| r.commands.first().copied()) - { - return command.to_string(); + if let Some(id) = record.runtime.as_deref() { + // Check static builtins first. + if let Some(command) = known_acp_runtime_exact(id).and_then(|r| r.commands.first().copied()) + { + return command.to_string(); + } + // Fall back to loaded registry for preset/custom harnesses. + if let Some(def) = crate::managed_agents::custom_harnesses::lookup_loaded_harness_by_id(id) + { + return def.command.clone(); + } } effective_agent_command(record.persona_id.as_deref(), personas, None) @@ -320,7 +326,8 @@ pub fn record_agent_command( /// /// Resolution order: /// 1. explicit override (non-empty) — a deliberate per-instance pin; -/// 2. the linked persona's `runtime` id mapped to its primary command; +/// 2. the linked persona's `runtime` id mapped to its primary command +/// (checks builtins then loaded preset/custom registry); /// 3. `default_agent_command()` — no persona/runtime, or persona deleted. pub fn effective_agent_command( persona_id: Option<&str>, @@ -334,18 +341,86 @@ pub fn effective_agent_command( return pin.to_string(); } - persona_id + let runtime_id = persona_id .and_then(|pid| personas.iter().find(|p| p.id == pid)) - .and_then(|persona| persona.runtime.as_deref()) - .and_then(known_acp_runtime_exact) - .and_then(|r| r.commands.first().copied()) - .map(str::to_string) - .unwrap_or_else(default_agent_command) + .and_then(|persona| persona.runtime.as_deref()); + + if let Some(id) = runtime_id { + // Check static builtins first. + if let Some(command) = known_acp_runtime_exact(id).and_then(|r| r.commands.first().copied()) + { + return command.to_string(); + } + // Check loaded preset/custom registry. + if let Some(def) = crate::managed_agents::custom_harnesses::lookup_loaded_harness_by_id(id) + { + return def.command.clone(); + } + } + + default_agent_command() } mod overrides; pub use overrides::{apply_agent_command_update, create_time_agent_command_override}; +/// Spawn-time variant of `record_agent_command` that returns a typed error when +/// a record's `runtime` id or its persona's `runtime` id is set but cannot be +/// resolved (i.e. the definition was deleted after the agent was created). +/// +/// Returns `Err("DANGLING_HARNESS_ID:")` so callers can surface the error +/// without falling through to `buzz-agent`. When there is no runtime id at all +/// the fallback to `default_agent_command()` is intentional (legacy agents +/// pre-date the unified harness model). +pub fn try_record_agent_command( + record: &crate::managed_agents::types::ManagedAgentRecord, + personas: &[crate::managed_agents::types::AgentDefinition], +) -> Result { + // Explicit pin always wins — if the user set a raw override, honour it. + if let Some(pin) = record + .agent_command_override + .as_deref() + .map(str::trim) + .filter(|v| !v.is_empty()) + { + return Ok(pin.to_string()); + } + + // Record-level runtime id: if set but unresolvable → typed error. + if let Some(id) = record.runtime.as_deref() { + if let Some(cmd) = known_acp_runtime_exact(id).and_then(|r| r.commands.first().copied()) { + return Ok(cmd.to_string()); + } + if let Some(def) = crate::managed_agents::custom_harnesses::lookup_loaded_harness_by_id(id) + { + return Ok(def.command.clone()); + } + return Err(format!("DANGLING_HARNESS_ID:{id}")); + } + + // Persona-level runtime id. + if let Some(persona_id) = record.persona_id.as_deref() { + if let Some(persona) = personas.iter().find(|p| p.id == persona_id) { + if let Some(id) = persona.runtime.as_deref() { + if let Some(cmd) = + known_acp_runtime_exact(id).and_then(|r| r.commands.first().copied()) + { + return Ok(cmd.to_string()); + } + if let Some(def) = + crate::managed_agents::custom_harnesses::lookup_loaded_harness_by_id(id) + { + return Ok(def.command.clone()); + } + return Err(format!("DANGLING_HARNESS_ID:{id}")); + } + } + } + + // No runtime id set — legacy agent; use the safe default. + Ok(default_agent_command()) +} + fn default_agent_args(command: &str) -> Option> { match normalize_command_identity(command).as_str() { "goose" => Some(vec!["acp".to_string()]), @@ -1286,6 +1361,9 @@ fn discover_acp_runtime_phase1(runtime: &'static KnownAcpRuntime) -> PartialEntr // Filled in by the auth-probe phase in full catalog discovery. auth_status: AuthStatus::Unknown, login_hint: None, + source: HarnessSource::Builtin, + // Builtin entries have no user-editable env; definition_env is empty. + definition_env: Default::default(), }, } } @@ -1300,8 +1378,153 @@ pub(crate) fn discover_acp_runtime_availability(runtime_id: &str) -> Option Vec { - // Phase 1: build all entries (fast — no probes yet). +// ── Tier-2 preset harnesses ──────────────────────────────────────────────── +// +// Static data for well-known ACP harnesses that have bundled logos and +// verified command/args. PATH-probed at discovery time (Detected badge); +// not editable or deletable by users. Logos are bundled assets referenced +// by id in the frontend `RUNTIME_LOGOS` map. + +struct PresetHarness { + id: &'static str, + label: &'static str, + command: &'static str, + args: &'static [&'static str], + install_instructions_url: &'static str, + install_hint: &'static str, +} + +const PRESET_HARNESSES: &[PresetHarness] = &[ + PresetHarness { + id: "cursor", + label: "Cursor", + command: "cursor-agent", + args: &["acp"], + install_instructions_url: "https://cursor.com/downloads", + install_hint: "Install Cursor from cursor.com/downloads.", + }, + PresetHarness { + id: "omp", + label: "Oh My Pi", + command: "omp", + args: &["acp"], + install_instructions_url: "https://github.com/can1357/oh-my-pi", + install_hint: "Install Oh My Pi from github.com/can1357/oh-my-pi.", + }, + PresetHarness { + id: "grok", + label: "Grok Build", + command: "grok", + args: &["agent", "--always-approve", "stdio"], + install_instructions_url: "https://build.x.ai/docs", + install_hint: "Install Grok Build from build.x.ai.", + }, + PresetHarness { + id: "opencode", + label: "OpenCode", + command: "opencode", + args: &["acp"], + install_instructions_url: "https://opencode.ai/docs", + install_hint: "Install OpenCode from opencode.ai/docs.", + }, + PresetHarness { + id: "kimi", + label: "Kimi Code", + command: "kimi", + args: &["acp"], + install_instructions_url: "https://kimi.ai/download", + install_hint: "Install Kimi Code from kimi.ai/download.", + }, + PresetHarness { + id: "amp", + label: "Amp", + command: "amp-acp", + args: &[], + install_instructions_url: "https://github.com/tao12345666333/amp-acp", + install_hint: "Install the amp-acp npm adapter: npm install -g amp-acp.", + }, + PresetHarness { + id: "hermes", + label: "Hermes Agent", + command: "hermes-acp", + args: &[], + install_instructions_url: "https://hermes-agent.nousresearch.com", + install_hint: "Install Hermes Agent from hermes-agent.nousresearch.com.", + }, + PresetHarness { + id: "openclaw", + label: "OpenClaw", + command: "openclaw", + args: &["acp"], + install_instructions_url: "https://docs.openclaw.ai/start/getting-started", + install_hint: "Install OpenClaw: npm install -g openclaw@latest.\n\n\ + ⚠️ Execution-locus note: `openclaw acp` runs tools inside the \ + OpenClaw Gateway daemon, not in the Desktop process. \ + Desktop-injected BUZZ_* env vars are visible to the `openclaw` \ + harness process itself, but do NOT automatically reach the \ + Gateway's execution environment. If your tools or agent logic \ + needs BUZZ_* credentials at execution time, set them on the \ + Gateway's own environment separately.", + }, +]; + +/// Return the static preset harness definitions as `HarnessDefinition` values. +/// +/// Used by `warm_harness_registry_from_dir` to seed the loaded-harness registry +/// at startup before the frontend triggers a full discovery run. +pub(crate) fn preset_harness_definitions( +) -> Vec { + PRESET_HARNESSES + .iter() + .map( + |p| crate::managed_agents::custom_harnesses::HarnessDefinition { + id: p.id.to_string(), + label: p.label.to_string(), + command: p.command.to_string(), + args: p.args.iter().map(|s| s.to_string()).collect(), + env: std::collections::BTreeMap::new(), + install_instructions_url: p.install_instructions_url.to_string(), + install_hint: p.install_hint.to_string(), + }, + ) + .collect() +} + +/// Return the static slice of preset harness IDs. +/// +/// Used by `check_id_collision` in `custom_harnesses` to derive the reserved-ID +/// set from the single source of truth (`PRESET_HARNESSES`) rather than a +/// hand-maintained copy. Adding a preset automatically reserves its ID. +pub(crate) fn preset_harness_ids() -> &'static [&'static str] { + // SAFETY: `PRESET_HARNESSES` is `'static`; we project its `id` fields. + // Computed once via OnceLock to avoid repeated allocations on hot paths. + use std::sync::OnceLock; + static IDS: OnceLock> = OnceLock::new(); + IDS.get_or_init(|| PRESET_HARNESSES.iter().map(|p| p.id).collect()) + .as_slice() +} + +/// Discover all ACP runtimes, optionally merging user-defined custom harnesses +/// from `custom_harnesses_dir`. +/// +/// This is the primary entry point used by the Tauri command layer. It: +/// 1. Builds entries for all compiled-in (`Builtin`) runtimes. +/// 2. Runs auth probes in parallel. +/// 3. Inserts static `Preset` entries (PATH-probed, `source: Preset`). +/// 4. If `custom_harnesses_dir` is `Some`, loads `*.json` files from that +/// directory and appends `Custom` entries — no auth probe, command resolved +/// via PATH, availability is `Available` or `NotInstalled`. +/// +/// The custom dir is re-scanned on every call (goose `refresh_custom_providers` +/// pattern) — no caching, no restart needed to pick up new files. +/// +/// After building the catalog, updates the loaded-harness registry so spawn +/// and readiness paths can resolve preset/custom harness commands without +/// re-running discovery. +pub fn discover_acp_runtimes_from( + custom_harnesses_dir: Option<&Path>, +) -> Vec { + // Phase 1: build all builtin entries (fast — no probes yet). let mut partials: Vec = KNOWN_ACP_RUNTIMES .iter() .map(discover_acp_runtime_phase1) @@ -1356,7 +1579,145 @@ pub fn discover_acp_runtimes() -> Vec { } } - partials.into_iter().map(|p| p.entry).collect() + let mut entries: Vec = partials.into_iter().map(|p| p.entry).collect(); + + // Track all ids seen so far (builtins) to prevent preset/custom collisions. + let mut seen_ids: std::collections::HashSet = + entries.iter().map(|e| e.id.clone()).collect(); + + // Loaded (non-builtin) definitions collected for the registry. + let mut loaded_defs: Vec = + Vec::new(); + + // Phase 2.5: insert static preset entries (PATH-probed, not editable/deletable). + for def in PRESET_HARNESSES { + if seen_ids.contains(def.id) { + // Builtin or earlier preset shadowed this id — skip silently. + continue; + } + seen_ids.insert(def.id.to_string()); + + let (availability, command, binary_path) = match find_command(def.command) { + Some(path) => ( + AcpAvailabilityStatus::Available, + Some(def.command.to_string()), + Some(path.display().to_string()), + ), + None => (AcpAvailabilityStatus::NotInstalled, None, None), + }; + + let default_args = normalize_agent_args( + def.command, + def.args.iter().map(|s| s.to_string()).collect(), + ); + + entries.push(AcpRuntimeCatalogEntry { + id: def.id.to_string(), + label: def.label.to_string(), + // No remote URL — all preset icons are bundled assets. + avatar_url: String::new(), + availability, + command, + binary_path, + default_args, + mcp_command: None, + model_env_var: None, + provider_env_var: None, + thinking_env_var: None, + install_hint: def.install_hint.to_string(), + install_instructions_url: def.install_instructions_url.to_string(), + can_auto_install: false, + requires_external_cli: false, + underlying_cli_path: None, + node_required: false, + auth_status: AuthStatus::NotApplicable, + login_hint: None, + source: HarnessSource::Preset, + // Preset entries have static, non-editable env; definition_env is empty. + definition_env: Default::default(), + }); + + // Register for spawn-time resolution. + loaded_defs.push(crate::managed_agents::custom_harnesses::HarnessDefinition { + id: def.id.to_string(), + label: def.label.to_string(), + command: def.command.to_string(), + args: def.args.iter().map(|s| s.to_string()).collect(), + env: Default::default(), + install_instructions_url: def.install_instructions_url.to_string(), + install_hint: def.install_hint.to_string(), + }); + } + + // Phase 3: load and append custom harness definitions. + if let Some(dir) = custom_harnesses_dir { + for def in crate::managed_agents::custom_harnesses::load_custom_harnesses(dir) { + // Collision check: a custom file must not shadow a built-in or preset id. + if let Err(reason) = + crate::managed_agents::custom_harnesses::check_id_collision(&def.id) + { + tracing::warn!("custom_harnesses: skipping {}: {reason}", def.id); + continue; + } + // Reject duplicates within the custom set itself or against presets. + if !seen_ids.insert(def.id.clone()) { + tracing::warn!("custom_harnesses: skipping duplicate id {:?}", def.id); + continue; + } + + // Availability: command on PATH → Available, else NotInstalled. + let (availability, command, binary_path) = match find_command(&def.command) { + Some(path) => ( + AcpAvailabilityStatus::Available, + Some(def.command.clone()), + Some(path.display().to_string()), + ), + None => (AcpAvailabilityStatus::NotInstalled, None, None), + }; + + let default_args = normalize_agent_args(&def.command, def.args.clone()); + + entries.push(AcpRuntimeCatalogEntry { + id: def.id.clone(), + label: def.label.clone(), + // F1 security fix: never copy user-supplied avatar URL into the catalog. + // All icons are bundled assets; customs fall back to TerminalSquare in the UI. + avatar_url: String::new(), + availability, + command, + binary_path, + default_args, + // Custom harnesses are plain ACP — no MCP sidecar, no env-var + // model switching, no thinking knobs. + mcp_command: None, + model_env_var: None, + provider_env_var: None, + thinking_env_var: None, + install_hint: def.install_hint.clone(), + install_instructions_url: def.install_instructions_url.clone(), + // Security line: custom definitions carry no install scripts. + can_auto_install: false, + requires_external_cli: false, + underlying_cli_path: None, + node_required: false, + // No auth probe for custom harnesses. + auth_status: AuthStatus::NotApplicable, + login_hint: None, + source: HarnessSource::Custom, + // Carry definition env into the catalog so the edit form can + // read it back — prevents silently erasing env on save. + definition_env: def.env.clone(), + }); + + loaded_defs.push(def); + } + } + + // Populate the loaded registry so spawn, readiness, and summary paths can + // resolve custom/preset harness commands without re-running discovery. + crate::managed_agents::custom_harnesses::update_loaded_harness_registry(loaded_defs); + + entries } pub fn managed_agent_avatar_url(command: &str) -> Option { diff --git a/desktop/src-tauri/src/managed_agents/discovery/tests.rs b/desktop/src-tauri/src/managed_agents/discovery/tests.rs index 364caa452b..0763eaa979 100644 --- a/desktop/src-tauri/src/managed_agents/discovery/tests.rs +++ b/desktop/src-tauri/src/managed_agents/discovery/tests.rs @@ -7,8 +7,8 @@ use super::{ effective_agent_command, find_nvm_default_bin, find_via_login_shell, is_login_shell_path_uninit, is_safe_nvm_tag, managed_agent_avatar_url, normalize_agent_args, parse_semver_tag, probe_codex_acp_major_version, record_agent_command, - refresh_login_shell_path, BUZZ_AGENT_AVATAR_URL, CLAUDE_CODE_AVATAR_URL, CODEX_AVATAR_URL, - GOOSE_AVATAR_URL, + refresh_login_shell_path, try_record_agent_command, BUZZ_AGENT_AVATAR_URL, + CLAUDE_CODE_AVATAR_URL, CODEX_AVATAR_URL, GOOSE_AVATAR_URL, }; use crate::managed_agents::AcpAvailabilityStatus; @@ -314,6 +314,64 @@ fn record_agent_command_bare_record_defaults() { assert_eq!(record_agent_command(&record, &[]), default_agent_command()); } +// ── try_record_agent_command ───────────────────────────────────────────────── + +/// When the record carries a dangling (unknown) runtime id, `try_record_agent_command` +/// must return `Err` containing "DANGLING_HARNESS_ID" — NEVER the buzz-agent default. +/// This test would fail if the function silently fell back to `default_agent_command()`. +#[test] +fn try_record_agent_command_dangling_runtime_id_returns_err() { + let record = record_with(Some("my-deleted-harness"), None, None); + let result = try_record_agent_command(&record, &[]); + assert!( + result.is_err(), + "dangling runtime id must produce Err, got Ok({:?})", + result.ok() + ); + assert!( + result.unwrap_err().contains("DANGLING_HARNESS_ID"), + "error must name the dangling id" + ); +} + +/// When the persona carries a dangling runtime id, `try_record_agent_command` +/// must also error — the error must not silently resolve to the default. +#[test] +fn try_record_agent_command_dangling_persona_runtime_returns_err() { + let personas = vec![persona_with_runtime("p1", Some("ghost-harness"))]; + let record = record_with(None, Some("p1"), None); + let result = try_record_agent_command(&record, &personas); + assert!( + result.is_err(), + "dangling persona runtime id must produce Err" + ); +} + +/// When neither the record nor persona has any runtime id, `try_record_agent_command` +/// falls back to `default_agent_command()` — this is the legacy-agent path. +#[test] +fn try_record_agent_command_no_runtime_id_defaults_to_buzz_agent() { + let record = record_with(None, None, None); + let result = try_record_agent_command(&record, &[]); + assert_eq!( + result, + Ok(default_agent_command()), + "no runtime id must fall back to the safe default" + ); +} + +/// An explicit agent_command_override always wins, even for a dangling runtime id. +#[test] +fn try_record_agent_command_override_beats_dangling_id() { + let record = record_with(Some("gone-harness"), None, Some("cursor-agent")); + let result = try_record_agent_command(&record, &[]); + assert_eq!( + result, + Ok("cursor-agent".to_string()), + "explicit override must beat a dangling runtime id" + ); +} + #[test] fn effective_agent_command_inherits_persona_runtime() { // No override → persona runtime id maps to its primary command. @@ -1269,3 +1327,249 @@ fn test_install_shell_from_some_returns_path() { "install_shell_from(Some) must return the path as Ok" ); } + +// ── Registry lifecycle (C1) ─────────────────────────────────────────────────── +// +// These tests verify the "warm → spawn resolves → delete → spawn errors" lifecycle +// that Paul's C1 ruling requires. They call the production resolution functions +// directly so they would red if warm_harness_registry_from_dir, save/delete +// transactional refresh, or try_record_agent_command were reverted. + +/// After warm_harness_registry_from_dir, a record with a matching custom runtime +/// id resolves to the custom command — NOT the buzz-agent default. +/// +/// This test would fail if warm_harness_registry_from_dir is not called before +/// try_record_agent_command, or if try_record_agent_command ignores the registry. +#[test] +fn registry_warm_then_try_record_resolves_custom_id() { + use crate::managed_agents::custom_harnesses::{ + registry_test_lock, warm_harness_registry_from_dir, + }; + use std::fs; + use tempfile::tempdir; + + let _lock = registry_test_lock(); + let dir = tempdir().unwrap(); + fs::write( + dir.path().join("my-custom-cli.json"), + r#"{"id":"my-custom-cli","label":"My CLI","command":"my-custom-bin"}"#, + ) + .unwrap(); + + warm_harness_registry_from_dir(Some(dir.path())); + + let record = record_with(Some("my-custom-cli"), None, None); + let result = try_record_agent_command(&record, &[]); + assert_eq!( + result, + Ok("my-custom-bin".to_string()), + "warm registry must make custom id resolvable at spawn time" + ); +} + +/// After deleting a custom harness and re-warming the registry, a record that +/// still references the deleted id must produce a DANGLING_HARNESS_ID error — +/// NOT silently fall back to buzz-agent. +/// +/// This test would fail if save/delete commands do not call +/// warm_harness_registry_from_dir transactionally, or if try_record_agent_command +/// silently falls back to default_agent_command() for dangling ids. +#[test] +fn registry_delete_then_try_record_returns_dangling_error() { + use crate::managed_agents::custom_harnesses::{ + registry_test_lock, warm_harness_registry_from_dir, + }; + use std::fs; + use tempfile::tempdir; + + let _lock = registry_test_lock(); + let dir = tempdir().unwrap(); + let path = dir.path().join("soon-gone.json"); + fs::write( + &path, + r#"{"id":"soon-gone","label":"Gone","command":"soon-gone-bin"}"#, + ) + .unwrap(); + warm_harness_registry_from_dir(Some(dir.path())); + + // Verify it resolves before delete. + let record = record_with(Some("soon-gone"), None, None); + assert!( + try_record_agent_command(&record, &[]).is_ok(), + "must resolve before delete" + ); + + // Simulate delete + re-warm (as delete_custom_harness does). + fs::remove_file(&path).unwrap(); + warm_harness_registry_from_dir(Some(dir.path())); + + // Now must produce a typed error. + let result = try_record_agent_command(&record, &[]); + assert!( + result.is_err(), + "deleted id must produce Err after re-warm, got Ok({:?})", + result.ok() + ); + assert!( + result.unwrap_err().contains("DANGLING_HARNESS_ID"), + "error must contain DANGLING_HARNESS_ID" + ); +} + +/// After saving (writing) a harness JSON and re-warming, the record resolves +/// to the new definition — simulating an immediate-save-then-start flow. +#[test] +fn registry_save_immediate_start_resolves_new_command() { + use crate::managed_agents::custom_harnesses::{ + registry_test_lock, warm_harness_registry_from_dir, + }; + use std::fs; + use tempfile::tempdir; + + let _lock = registry_test_lock(); + let dir = tempdir().unwrap(); + + // Before save: must not resolve. + warm_harness_registry_from_dir(Some(dir.path())); + let record = record_with(Some("fast-harness"), None, None); + assert!( + try_record_agent_command(&record, &[]).is_err(), + "must not resolve before save" + ); + + // Simulate save + transactional re-warm. + fs::write( + dir.path().join("fast-harness.json"), + r#"{"id":"fast-harness","label":"Fast","command":"fast-bin"}"#, + ) + .unwrap(); + warm_harness_registry_from_dir(Some(dir.path())); + + let result = try_record_agent_command(&record, &[]); + assert_eq!( + result, + Ok("fast-bin".to_string()), + "immediate save+start must resolve without a discover_acp_providers round-trip" + ); +} + +/// Editing a custom harness (renaming id + updating command) and re-warming the +/// registry makes both the old id a dangling reference and the new id resolvable. +#[test] +fn registry_edit_with_id_rename_old_dangling_new_resolved() { + use crate::managed_agents::custom_harnesses::{ + registry_test_lock, warm_harness_registry_from_dir, + }; + use std::fs; + use tempfile::tempdir; + + // Serialize against all other tests that write to the global registry. + let _lock = registry_test_lock(); + let dir = tempdir().unwrap(); + + // Create original. + fs::write( + dir.path().join("original-id.json"), + r#"{"id":"original-id","label":"Orig","command":"orig-bin"}"#, + ) + .unwrap(); + warm_harness_registry_from_dir(Some(dir.path())); + + let old_record = record_with(Some("original-id"), None, None); + assert!( + try_record_agent_command(&old_record, &[]).is_ok(), + "original id must resolve" + ); + + // Simulate rename: write new file, remove old file (same as save_custom_harness + // with original_id set), then re-warm. + fs::write( + dir.path().join("renamed-id.json"), + r#"{"id":"renamed-id","label":"Renamed","command":"new-bin"}"#, + ) + .unwrap(); + fs::remove_file(dir.path().join("original-id.json")).unwrap(); + warm_harness_registry_from_dir(Some(dir.path())); + + // Old id must now be dangling. + let result = try_record_agent_command(&old_record, &[]); + assert!( + result.is_err() && result.unwrap_err().contains("DANGLING_HARNESS_ID"), + "original id must be dangling after rename" + ); + + // New id must resolve. + let new_record = record_with(Some("renamed-id"), None, None); + assert_eq!( + try_record_agent_command(&new_record, &[]), + Ok("new-bin".to_string()), + "new id must resolve after rename" + ); +} + +// ── I2: custom catalog entry carries definition_env for the edit round-trip ─── + +/// A custom harness definition that includes env vars must surface those vars +/// in the `definition_env` field of the resulting `AcpRuntimeCatalogEntry`. +/// +/// This proves the edit-form round-trip: the backend carries env into the +/// catalog, the frontend reads it back when opening the edit form, and Save +/// therefore preserves existing env vars rather than silently erasing them. +#[test] +fn custom_catalog_entry_carries_definition_env_for_edit_roundtrip() { + use crate::managed_agents::discovery::discover_acp_runtimes_from; + use std::{collections::BTreeMap, fs}; + use tempfile::tempdir; + + let dir = tempdir().unwrap(); + // Write a custom definition with two env vars. + fs::write( + dir.path().join("env-harness.json"), + r#"{ + "id": "env-harness", + "label": "Env Harness", + "command": "env-harness-bin", + "args": [], + "env": { "CURSOR_ACP": "1", "MY_TOKEN": "abc" } + }"#, + ) + .unwrap(); + + let entries = discover_acp_runtimes_from(Some(dir.path())); + let entry = entries + .iter() + .find(|e| e.id == "env-harness") + .expect("custom entry must appear in catalog"); + + let expected: BTreeMap = [ + ("CURSOR_ACP".to_string(), "1".to_string()), + ("MY_TOKEN".to_string(), "abc".to_string()), + ] + .into_iter() + .collect(); + + assert_eq!( + entry.definition_env, expected, + "catalog entry must carry definition env vars so the edit form can read them back" + ); +} + +/// A builtin catalog entry must have an empty `definition_env` — their env +/// is handled via the `KnownAcpRuntime` metadata path, not user-editable JSON. +#[test] +fn builtin_catalog_entry_has_empty_definition_env() { + use crate::managed_agents::discovery::discover_acp_runtimes_from; + + let entries = discover_acp_runtimes_from(None); + // Find any builtin entry (e.g. "goose" or "claude"). + let builtin = entries + .iter() + .find(|e| e.source == crate::managed_agents::HarnessSource::Builtin) + .expect("at least one builtin must exist"); + + assert!( + builtin.definition_env.is_empty(), + "builtin entry must not carry definition_env, got: {:?}", + builtin.definition_env + ); +} diff --git a/desktop/src-tauri/src/managed_agents/env_vars.rs b/desktop/src-tauri/src/managed_agents/env_vars.rs index 6318a5c838..5724653b92 100644 --- a/desktop/src-tauri/src/managed_agents/env_vars.rs +++ b/desktop/src-tauri/src/managed_agents/env_vars.rs @@ -81,6 +81,12 @@ pub(crate) const RESERVED_ENV_KEYS: &[&str] = &[ // ambient env var must not be able to forge setup mode (NotReady) on a // Ready agent or suppress it (empty/stale payload) on a NotReady one. "BUZZ_ACP_SETUP_PAYLOAD", + // Desktop ownership markers: these brand every spawned harness with the + // launching Desktop instance. A user-supplied override would let a + // definition masquerade as a different instance or fake the nonce used + // for same-session sweep decisions. + "BUZZ_MANAGED_AGENT", + "BUZZ_MANAGED_AGENT_START_NONCE", ]; pub(crate) fn is_reserved_env_key(key: &str) -> bool { diff --git a/desktop/src-tauri/src/managed_agents/mod.rs b/desktop/src-tauri/src/managed_agents/mod.rs index 66f3d882b9..97023e15b0 100644 --- a/desktop/src-tauri/src/managed_agents/mod.rs +++ b/desktop/src-tauri/src/managed_agents/mod.rs @@ -7,6 +7,7 @@ pub(crate) use agent_env::{ }; mod backend; pub(crate) mod config_bridge; +pub(crate) mod custom_harnesses; mod discovery; mod env_vars; pub(crate) mod git_bash; @@ -59,7 +60,8 @@ pub use personas::*; #[cfg(windows)] pub use process_lifecycle::*; pub(crate) use readiness::{ - agent_readiness, resolve_effective_agent_env, AgentReadiness, Requirement, + agent_readiness, resolve_effective_agent_env, resolve_effective_harness_descriptor, + AgentReadiness, Requirement, }; pub use relay_mesh::*; pub use repos::{ diff --git a/desktop/src-tauri/src/managed_agents/readiness.rs b/desktop/src-tauri/src/managed_agents/readiness.rs index c04dfa88a4..a82b4cd2c7 100644 --- a/desktop/src-tauri/src/managed_agents/readiness.rs +++ b/desktop/src-tauri/src/managed_agents/readiness.rs @@ -47,6 +47,7 @@ use crate::managed_agents::{ discovery::{known_acp_runtime, KnownAcpRuntime}, env_vars::merged_user_env, global_config::GlobalAgentConfig, + normalize_agent_args, types::{AcpAvailabilityStatus, AgentDefinition, ManagedAgentRecord}, }; @@ -77,6 +78,101 @@ pub(crate) struct EffectiveAgentEnv { pub effective_command: String, } +// ── Typed effective-harness descriptor ─────────────────────────────────────── +// +// A single owned type that fully describes what a spawn would run. Produced +// by `resolve_effective_harness_descriptor` and consumed by spawn_agent_child, +// spawn_config_hash, build_managed_agent_summary, get_agent_models, and +// agent_readiness — so the harness-definition lookup and arg/env resolution +// happen exactly once, in one place. + +/// The complete effective description of a harness spawn: resolved command, +/// args, and layered env. This is the single source of truth for what will +/// actually run — computed once and shared across every consumer that needs +/// the effective values. +#[derive(Debug, Clone)] +pub(crate) struct EffectiveHarnessDescriptor { + /// The raw effective command string (e.g. `"buzz-agent"`, `"my-acp-agent"`). + /// Used for `known_acp_runtime` lookup and hashing. + pub command: String, + /// Normalized effective args. Instance args win when non-empty; otherwise + /// the harness definition's args apply. + pub args: Vec, + /// The full layered process env: baked floor → runtime metadata → definition + /// env → global → persona → agent. + pub env: BTreeMap, +} + +/// Resolve the complete harness descriptor from a record + context — the single +/// authoritative path for command, args, and env. +/// +/// This is the only place where harness-definition lookup and arg/env layering +/// happen; spawn, hash, summary, and both model-probe paths all consume this. +/// +/// Returns `Err("DANGLING_HARNESS_ID:")` when the record (or its linked +/// persona) references a runtime id that no longer exists in the registry — +/// the same typed error produced by `try_record_agent_command`. Callers that +/// cannot meaningfully continue with a dangling id (e.g. `spawn_agent_child`) +/// propagate the error; callers that degrade gracefully may use +/// `.unwrap_or_else(|_| …)`. +/// +/// Does NOT require an `AppHandle` so it is fully unit-testable. +/// +/// # Arguments +/// * `record` — the managed agent record +/// * `personas` — all current personas (for command/env resolution) +/// * `global` — global agent config defaults +pub(crate) fn resolve_effective_harness_descriptor( + record: &ManagedAgentRecord, + personas: &[crate::managed_agents::types::AgentDefinition], + global: &crate::managed_agents::GlobalAgentConfig, +) -> Result { + let effective_command = crate::managed_agents::try_record_agent_command(record, personas)?; + let runtime_meta = known_acp_runtime(&effective_command); + + // Look up the harness definition once — used for both args and env. + // Resolution order: record.runtime → persona.runtime → "". + let harness_def = { + let runtime_id = record + .runtime + .as_deref() + .or_else(|| { + record.persona_id.as_deref().and_then(|pid| { + personas + .iter() + .find(|p| p.id == pid) + .and_then(|p| p.runtime.as_deref()) + }) + }) + .unwrap_or(""); + crate::managed_agents::custom_harnesses::lookup_loaded_harness_by_id(runtime_id) + }; + + // Args: explicit non-empty instance args win; otherwise use definition args. + let args = { + let record_args = record.agent_args.clone(); + let instance_has_args = record_args.iter().any(|a| !a.trim().is_empty()); + if instance_has_args { + normalize_agent_args(&effective_command, record_args) + } else if let Some(ref def) = harness_def { + normalize_agent_args(&effective_command, def.args.clone()) + } else { + normalize_agent_args(&effective_command, record_args) + } + }; + + // Env: full layered resolution (same as resolve_effective_agent_env). + // Pass harness_def directly to avoid a second lookup. + let effective_env = + resolve_effective_agent_env_with_def(record, personas, runtime_meta, global, harness_def); + + Ok(EffectiveHarnessDescriptor { + command: effective_command, + args, + env: effective_env.env, + }) +} + /// Assemble the effective agent env from a record, personas, optional /// known-runtime metadata, and the global agent config defaults — without an /// `AppHandle` so it is fully unit-testable. @@ -92,6 +188,38 @@ pub(crate) fn resolve_effective_agent_env( personas: &[AgentDefinition], runtime: Option<&KnownAcpRuntime>, global: &GlobalAgentConfig, +) -> EffectiveAgentEnv { + // Look up the harness definition for definition-level env (preset/custom). + // Same resolution logic as spawn_agent_child: record runtime id first, then + // persona runtime id, then nothing. + let harness_def = { + let runtime_id = record + .runtime + .as_deref() + .or_else(|| { + record.persona_id.as_deref().and_then(|pid| { + personas + .iter() + .find(|p| p.id == pid) + .and_then(|p| p.runtime.as_deref()) + }) + }) + .unwrap_or(""); + crate::managed_agents::custom_harnesses::lookup_loaded_harness_by_id(runtime_id) + }; + + resolve_effective_agent_env_with_def(record, personas, runtime, global, harness_def) +} + +/// Inner implementation that accepts a pre-fetched `harness_def` to avoid a +/// second registry lookup when the caller (e.g. `resolve_effective_harness_descriptor`) +/// already has the definition in hand. +fn resolve_effective_agent_env_with_def( + record: &ManagedAgentRecord, + personas: &[AgentDefinition], + runtime: Option<&KnownAcpRuntime>, + global: &GlobalAgentConfig, + harness_def: Option>, ) -> EffectiveAgentEnv { let effective_command = crate::managed_agents::record_agent_command(record, personas); @@ -118,6 +246,17 @@ pub(crate) fn resolve_effective_agent_env( } } + // Layer 2b: definition env — the harness author's defaults (e.g. CURSOR_ACP=1). + // Applied as a floor below global so user env always wins on collision. + // Reserved keys are stripped by the shared `is_reserved_env_key` predicate. + if let Some(ref def) = harness_def { + for (key, value) in &def.env { + if !super::env_vars::is_reserved_env_key(key) { + env.insert(key.clone(), value.clone()); + } + } + } + // Layer 3a: global env vars — the lowest user-settable layer. // Injected before persona/agent so per-agent values win on collision. // `merged_user_env` with an empty "lower" map applies reserved/malformed-key @@ -196,6 +335,14 @@ pub enum Requirement { /// Git for Windows is missing, so buzz-agent cannot launch buzz-dev-mcp's /// Bash-based shell tool. Doctor owns installation and re-checking. GitBash, + /// A custom harness command that cannot be resolved in the current PATH. + /// Displayed as a PATH badge in the harness card and a nudge in the agent + /// message stream. No in-app action can fix this — the user must install + /// the binary or update their PATH. + MissingBinary { + /// The command name that was not found (e.g. `\"my-acp-agent\"`). + command: String, + }, } // ── AgentReadiness ──────────────────────────────────────────────────────────── @@ -271,7 +418,13 @@ fn collect_missing_requirements( runtime: Option<&KnownAcpRuntime>, ) -> Vec { let Some(rt) = runtime else { - // Unknown/custom command — no requirements to check. + // Unknown/custom command — check that the binary is actually resolvable. + // No known requirement set beyond the PATH check. + if crate::managed_agents::resolve_command(&effective.effective_command).is_none() { + return vec![Requirement::MissingBinary { + command: effective.effective_command.clone(), + }]; + } return vec![]; }; @@ -1207,10 +1360,30 @@ mod tests { #[test] fn unknown_command_is_always_ready() { - let env = make_env("my-custom-harness", BTreeMap::new()); + // Since Phase B-7 (readiness exec-check), unknown/custom commands that are + // not resolvable in PATH produce a MissingBinary requirement rather than + // being unconditionally Ready. A command that IS resolvable should be Ready. + // Use a known-present binary so the test is not environment-sensitive. + let env = make_env("sh", BTreeMap::new()); assert!( agent_readiness(&env).is_ready(), - "unknown/custom command should always be Ready (no requirements)" + "unknown/custom command present in PATH should be Ready" + ); + } + + #[test] + fn unknown_command_missing_from_path_is_not_ready() { + let env = make_env("my-custom-harness-that-does-not-exist", BTreeMap::new()); + let readiness = agent_readiness(&env); + assert!( + !readiness.is_ready(), + "unknown/custom command absent from PATH should be NotReady" + ); + let reqs = readiness.requirements(); + assert_eq!(reqs.len(), 1); + assert!( + matches!(&reqs[0], Requirement::MissingBinary { command } if command == "my-custom-harness-that-does-not-exist"), + "should surface MissingBinary requirement" ); } diff --git a/desktop/src-tauri/src/managed_agents/runtime.rs b/desktop/src-tauri/src/managed_agents/runtime.rs index 6687cbbcf2..d8db5ad675 100644 --- a/desktop/src-tauri/src/managed_agents/runtime.rs +++ b/desktop/src-tauri/src/managed_agents/runtime.rs @@ -412,6 +412,27 @@ pub(crate) fn valid_agent_runtime_receipt( path: &std::path::Path, receipt: &super::ManagedAgentRuntimeReceipt, instance_id: &str, +) -> bool { + valid_agent_runtime_receipt_with( + path, + receipt, + instance_id, + process_is_running, + process_belongs_to_us, + process_has_buzz_marker, + ) +} + +/// Injectable version of `valid_agent_runtime_receipt` for testing. +/// `is_running(pid)`, `belongs_to_us(pid)`, and `has_marker(pid, instance_id)` +/// can be substituted by test doubles without spawning real processes. +pub(crate) fn valid_agent_runtime_receipt_with( + path: &std::path::Path, + receipt: &super::ManagedAgentRuntimeReceipt, + instance_id: &str, + is_running: impl Fn(u32) -> bool, + belongs_to_us: impl Fn(u32) -> bool, + has_marker: impl Fn(u32, &str) -> bool, ) -> bool { let Ok(canonical) = ManagedAgentRuntimeKey::new(receipt.key.pubkey.clone(), &receipt.key.relay_url) @@ -422,9 +443,16 @@ pub(crate) fn valid_agent_runtime_receipt( && path.file_name().and_then(|name| name.to_str()) == Some(&format!("{}.json", receipt.key.runtime_id())) && receipt.desktop_instance_id == instance_id - && process_is_running(receipt.pid) - && process_belongs_to_us(receipt.pid) - && process_has_buzz_marker(receipt.pid, &receipt.desktop_instance_id) + && is_running(receipt.pid) + // Receipts are written by THIS instance at spawn time, so they are + // Buzz-owned by construction. Use the shared ownership predicate + // (marker-only) so custom-harness binaries (not in KNOWN_AGENT_BINARIES) + // are not rejected here. `belongs_to_us` is passed as the fast- + // path hint but ignored by `buzz_sweep_owns_process`. + && buzz_sweep_owns_process( + belongs_to_us(receipt.pid), + has_marker(receipt.pid, &receipt.desktop_instance_id), + ) } fn terminate_runtime_receipt_with( @@ -508,7 +536,11 @@ pub(crate) fn sweep_orphaned_agent_processes(app: &AppHandle, skip_pids: &[u32]) if skip_pids.contains(pid) { return false; } - (process_is_running(*pid) && process_belongs_to_us(*pid)) || !process_is_running(*pid) + // Receipt/PID-file entries were written by this instance at spawn + // time — they are Buzz-owned by construction; no name gate needed. + // Kill live processes; dead ones fall through to receipt cleanup. + (process_is_running(*pid) && process_has_buzz_marker(*pid, &instance_id)) + || !process_is_running(*pid) }) .map(|pid| pid as i32) .collect(); @@ -522,7 +554,7 @@ pub(crate) fn sweep_orphaned_agent_processes(app: &AppHandle, skip_pids: &[u32]) if skip_pids.contains(pid) { continue; } - if !process_is_running(*pid) || !process_belongs_to_us(*pid) { + if !process_is_running(*pid) || !process_has_buzz_marker(*pid, &instance_id) { super::remove_agent_pid_file(app, pubkey); } } @@ -530,7 +562,7 @@ pub(crate) fn sweep_orphaned_agent_processes(app: &AppHandle, skip_pids: &[u32]) if skip_pids.contains(&receipt.pid) { continue; } - if !process_is_running(receipt.pid) || !process_belongs_to_us(receipt.pid) { + if !process_is_running(receipt.pid) || !process_has_buzz_marker(receipt.pid, &instance_id) { super::remove_agent_runtime_receipt(app, &receipt.key); } } @@ -578,6 +610,29 @@ const _: () = assert!(std::mem::size_of::() == 136); #[cfg(target_os = "macos")] pub(super) const PROC_PIDTBSDINFO: libc::c_int = 3; +// ── Shared sweep ownership predicate ───────────────────────────────────────── +// +// `BUZZ_MANAGED_AGENT` env marker is the sole authoritative ownership proof. +// The `_belongs_to_us` name-check is passed by callers but is intentionally +// IGNORED — custom harnesses have arbitrary binary names and would be missed +// by a name-gated predicate. + +/// Returns `true` when a process should be included in the orphan sweep. +/// +/// The `BUZZ_MANAGED_AGENT` env marker is the sole authoritative ownership +/// proof — any process carrying it and belonging to this instance is swept, +/// regardless of binary name. The `_belongs_to_us` parameter is accepted +/// for call-site symmetry but is intentionally ignored: the function returns +/// `has_buzz_marker` unconditionally. On Windows no `/proc`-based sweep +/// runs, so `process_has_buzz_marker` always returns `false`. +/// +/// This predicate is cross-platform and tested directly in the unit tests +/// below — no `#[cfg(unix)]` guard needed here. +pub(crate) fn buzz_sweep_owns_process(_belongs_to_us: bool, has_buzz_marker: bool) -> bool { + // Marker is the sole authoritative ownership gate. + has_buzz_marker +} + /// Enumerate all processes on the system owned by the current user and kill any /// agent binary stamped with *this* instance's `BUZZ_MANAGED_AGENT` marker /// (`instance_id`) that isn't in `skip_pids`. This catches orphans that escaped @@ -602,11 +657,7 @@ pub(crate) fn sweep_system_agent_processes(instance_id: &str, skip_pids: &[u32]) if skip_pids.contains(&upid) || pid == my_pid { continue; } - // Check binary name first (cheap proc_name call) before UID lookup. - if !process_belongs_to_us(upid) { - continue; - } - // Verify UID and PPID via proc_pidinfo. + // Verify UID and PPID via proc_pidinfo before the more expensive env scan. let mut info = std::mem::MaybeUninit::::zeroed(); let ret = unsafe { proc_pidinfo( @@ -624,7 +675,15 @@ pub(crate) fn sweep_system_agent_processes(instance_id: &str, skip_pids: &[u32]) if info.pbi_uid != my_uid { continue; } - if !process_has_buzz_marker(upid, instance_id) { + // Custom harnesses don't match KNOWN_AGENT_BINARIES by name; the + // BUZZ_MANAGED_AGENT env marker is the authoritative ownership proof. + // `buzz_sweep_owns_process` returns `has_buzz_marker` — the + // `_belongs_to_us` argument is accepted for call-site symmetry but + // is intentionally ignored (see the function's doc comment). + if !buzz_sweep_owns_process( + process_belongs_to_us(upid), + process_has_buzz_marker(upid, instance_id), + ) { continue; } // Live descendants of a tracked harness are exempt — see sweep::is_live_descendant_*. @@ -683,7 +742,13 @@ pub(crate) fn sweep_system_agent_processes(instance_id: &str, skip_pids: &[u32]) if meta.uid() != my_uid { continue; } - if !process_belongs_to_us(upid) || !process_has_buzz_marker(upid, instance_id) { + // Same ownership predicate as macOS: marker is the authoritative gate, + // `_belongs_to_us` is accepted for call-site symmetry but ignored. + // Fixes custom-harness orphan cleanup on Linux. + if !buzz_sweep_owns_process( + process_belongs_to_us(upid), + process_has_buzz_marker(upid, instance_id), + ) { continue; } // Live descendants of a tracked harness are exempt — see sweep::is_live_descendant_*. @@ -767,9 +832,6 @@ pub(crate) fn collect_same_instance_orphans( if skip_pids.contains(&upid) { continue; } - if !process_belongs_to_us(upid) { - continue; - } let mut info = std::mem::MaybeUninit::::zeroed(); let ret = unsafe { proc_pidinfo( @@ -787,7 +849,12 @@ pub(crate) fn collect_same_instance_orphans( if info.pbi_uid != my_uid { continue; } - if !process_has_buzz_marker(upid, instance_id) { + // Custom harnesses don't match KNOWN_AGENT_BINARIES by name; the + // BUZZ_MANAGED_AGENT env marker is the authoritative ownership proof. + if !buzz_sweep_owns_process( + process_belongs_to_us(upid), + process_has_buzz_marker(upid, instance_id), + ) { continue; } // Live descendants of a tracked harness are exempt — see sweep::is_live_descendant_*. @@ -833,7 +900,13 @@ pub(crate) fn collect_same_instance_orphans( if meta.uid() != my_uid { continue; } - if !process_belongs_to_us(upid) || !process_has_buzz_marker(upid, instance_id) { + // Same ownership predicate as macOS: marker is the authoritative gate, + // `_belongs_to_us` is accepted for call-site symmetry but ignored. + // Fixes custom-harness orphan cleanup on Linux. + if !buzz_sweep_owns_process( + process_belongs_to_us(upid), + process_has_buzz_marker(upid, instance_id), + ) { continue; } // Live descendants of a tracked harness are exempt — see sweep::is_live_descendant_*. @@ -1095,10 +1168,7 @@ pub(crate) fn reap_dead_instance_agents(our_instance_id: &str, skip_pids: &[u32] if skip_pids.contains(&upid) { continue; } - if !process_belongs_to_us(upid) { - continue; - } - // Verify UID. + // Verify UID and PPID via proc_pidinfo before the more expensive env scan. let mut info = std::mem::MaybeUninit::::zeroed(); let ret = unsafe { proc_pidinfo( @@ -1117,6 +1187,9 @@ pub(crate) fn reap_dead_instance_agents(our_instance_id: &str, skip_pids: &[u32] continue; } // Extract the instance ID from this agent's env. + // Do NOT name-gate via process_belongs_to_us — custom harnesses use + // arbitrary binary names and BUZZ_MANAGED_AGENT is the authoritative + // ownership proof. let Some(agent_instance_id) = extract_buzz_marker_value(upid) else { continue; }; @@ -1174,9 +1247,9 @@ pub(crate) fn reap_dead_instance_agents(our_instance_id: &str, skip_pids: &[u32] if meta.uid() != my_uid { continue; } - if !process_belongs_to_us(upid) { - continue; - } + // Do NOT name-gate via process_belongs_to_us — custom harnesses use + // arbitrary binary names and BUZZ_MANAGED_AGENT is the authoritative + // ownership proof. let Some(agent_instance_id) = extract_buzz_marker_value(upid) else { continue; }; @@ -1213,6 +1286,23 @@ pub fn kill_stale_tracked_processes( records: &mut [ManagedAgentRecord], runtimes: &HashMap, instance_id: &str, +) -> bool { + kill_stale_tracked_processes_with( + records, + runtimes, + |pid| process_has_buzz_marker(pid, instance_id), + terminate_process, + ) +} + +/// Injectable version of `kill_stale_tracked_processes` for testing. +/// `has_marker(pid)` returns true when the process carries this instance's +/// `BUZZ_MANAGED_AGENT` marker; `kill(pid)` performs the termination. +pub(crate) fn kill_stale_tracked_processes_with( + records: &mut [ManagedAgentRecord], + runtimes: &HashMap, + has_marker: impl Fn(u32) -> bool, + mut kill: impl FnMut(u32) -> Result<(), String>, ) -> bool { use crate::managed_agents::BackendKind; @@ -1225,8 +1315,11 @@ pub fn kill_stale_tracked_processes( continue; }; if !runtimes.keys().any(|key| key.pubkey == record.pubkey) { - if process_belongs_to_us(pid) && process_has_buzz_marker(pid, instance_id) { - let _ = terminate_process(pid); + // Name-gate is omitted intentionally: custom harnesses use arbitrary + // binary names not in KNOWN_AGENT_BINARIES. BUZZ_MANAGED_AGENT is the + // authoritative ownership proof; terminate only if it matches. + if has_marker(pid) { + let _ = kill(pid); } record.runtime_pid = None; record.last_stopped_at = Some(crate::util::now_iso()); @@ -1476,11 +1569,28 @@ pub fn build_managed_agent_summary( hash_drift || availability_drift }); - // Resolve the effective harness the same way, then derive args/mcp from it, - // so the UI reflects the persona's current harness (or an explicit pin). - let effective_command = crate::managed_agents::record_agent_command(record, personas); - let effective_args = normalize_agent_args(&effective_command, record.agent_args.clone()); - let effective_mcp_command = known_acp_runtime(&effective_command) + // Resolve the effective harness via the single typed descriptor — same resolver + // as spawn, so the UI reflects the persona's current harness (or explicit pin). + // Global config is loaded here for descriptor parity with spawn (env layering). + let global_for_summary = + crate::managed_agents::load_global_agent_config(app).unwrap_or_default(); + let descriptor = crate::managed_agents::resolve_effective_harness_descriptor( + record, + personas, + &global_for_summary, + ) + .unwrap_or_else(|_| { + // Dangling harness — show a best-effort command so the UI displays the + // record's configured runtime id rather than crashing. + let cmd = crate::managed_agents::record_agent_command(record, personas); + let args = normalize_agent_args(&cmd, record.agent_args.clone()); + crate::managed_agents::readiness::EffectiveHarnessDescriptor { + command: cmd, + args, + env: Default::default(), + } + }); + let effective_mcp_command = known_acp_runtime(&descriptor.command) .and_then(|r| r.mcp_command) .unwrap_or("") .to_string(); @@ -1492,9 +1602,9 @@ pub fn build_managed_agent_summary( team_id: record.team_id.clone(), relay_url: record.relay_url.clone(), acp_command: record.acp_command.clone(), - agent_command: effective_command, + agent_command: descriptor.command, agent_command_override: record.agent_command_override.clone(), - agent_args: effective_args, + agent_args: descriptor.args, mcp_command: effective_mcp_command, turn_timeout_seconds: record.turn_timeout_seconds, idle_timeout_seconds: record.idle_timeout_seconds, @@ -1660,11 +1770,18 @@ pub fn spawn_agent_child( // Load global config once; used for runtime_metadata_env_vars (model/provider fallback) // and for the env-var merge at spawn time. let global = crate::managed_agents::load_global_agent_config(app).unwrap_or_default(); - let effective_command = super::record_agent_command(record, &personas); - let agent_args = normalize_agent_args(&effective_command, record.agent_args.clone()); + // Single typed resolver: validates runtime id (dangling harness → Err), resolves + // command, args (instance wins over definition default), and the full env layer stack. + // This is the sole path for harness-definition lookup — spawn, hash, summary, and + // model probes all consume this descriptor rather than assembling values inline. + let descriptor = + crate::managed_agents::resolve_effective_harness_descriptor(record, &personas, &global) + .map_err(|e| format!("cannot spawn agent {}: {e}", record.pubkey))?; + let effective_command = &descriptor.command; + let agent_args = &descriptor.args; let resolved_acp_command = resolve_command(&record.acp_command) .ok_or_else(|| missing_command_message(&record.acp_command, "ACP harness command"))?; - let effective_mcp_command = known_acp_runtime(&effective_command) + let effective_mcp_command = known_acp_runtime(effective_command) .and_then(|r| r.mcp_command) .unwrap_or(""); let resolved_mcp_command: Option = if effective_mcp_command.is_empty() { @@ -1681,7 +1798,7 @@ pub fn spawn_agent_child( } }; // Resolve agent command to a full path (DMG launches have minimal PATH). - let resolved_agent_command = resolve_command(&effective_command) + let resolved_agent_command = resolve_command(effective_command) .map(|p| p.display().to_string()) .unwrap_or_else(|| effective_command.clone()); @@ -1732,7 +1849,7 @@ pub fn spawn_agent_child( } // Enable MCP hook tools (_Stop, _PostCompact) for agents that need them. // Uses "*" because build_mcp_servers() hard-codes the server name to "buzz-mcp". - let runtime_meta = known_acp_runtime(&effective_command); + let runtime_meta = known_acp_runtime(effective_command); if runtime_meta.is_some_and(|r| r.mcp_hooks) { command.env("MCP_HOOK_SERVERS", "*"); } @@ -1758,11 +1875,16 @@ pub fn spawn_agent_child( // stuck agents for auto-restart. let spawned_setup_mode; { - use crate::managed_agents::{ - agent_readiness, resolve_effective_agent_env, AgentReadiness, Requirement, + use crate::managed_agents::readiness::EffectiveAgentEnv; + use crate::managed_agents::{agent_readiness, AgentReadiness, Requirement}; + + // Construct EffectiveAgentEnv from the descriptor computed above — no second + // resolver call; the descriptor's env is already the fully layered result. + let effective = EffectiveAgentEnv { + env: descriptor.env.clone(), + config_file_path: runtime_meta.and_then(|r| r.config_file_path), + effective_command: descriptor.command.clone(), }; - - let effective = resolve_effective_agent_env(record, &personas, runtime_meta, &global); // Compute the optional payload before touching the command. let setup_payload_json = if let AgentReadiness::NotReady { requirements } = agent_readiness(&effective) { @@ -1800,6 +1922,10 @@ pub fn spawn_agent_child( Requirement::GitBash => serde_json::json!({ "surface": "git_bash", }), + Requirement::MissingBinary { command } => serde_json::json!({ + "surface": "missing_binary", + "command": command, + }), }) .collect(); let payload = serde_json::json!({ @@ -1967,28 +2093,15 @@ pub fn spawn_agent_child( ); } - // ── User env vars: live persona env under agent overrides ────────── + // ── User env vars: definition floor + global + live persona + agent overrides ── // - // The record's `env_vars` holds agent-level overrides only. The linked - // persona's env is read live and merged underneath (agent wins on - // collision), so persona credential edits reach the agent on the next - // spawn — same refresh semantics as prompt/model/provider above and the - // provider deploy path. Global env vars are the floor layer below persona. - // `merged_user_env` also applies the reserved-key / malformed-key / NUL - // filtering. Precedence: baked floor < Buzz-set env above < GLOBAL < - // PERSONA < per-agent. - // - // These writes go LAST so user-provided values win over every Buzz-set env - // above — EXCEPT reserved keys (BUZZ_PRIVATE_KEY, NOSTR_PRIVATE_KEY, - // BUZZ_AUTH_TAG, BUZZ_API_TOKEN, BUZZ_ACP_PRIVATE_KEY, BUZZ_ACP_API_TOKEN), - // which `merged_user_env` strips. Those carry Buzz's identity and must - // never be GUI-overridable. - // global < live persona < agent (last-wins on collision at each layer). - let persona_over_global = super::env_vars::merged_user_env( - &global.env_vars, - &super::env_vars::live_persona_env(&personas, record.persona_id.as_deref()), - ); - for (key, value) in super::env_vars::merged_user_env(&persona_over_global, &record.env_vars) { + // `descriptor.env` is the fully-layered result from `resolve_effective_harness_descriptor`: + // baked floor → runtime metadata → definition env (harness author defaults) → + // global → live persona → per-agent, with reserved-key and malformed-key filtering + // applied. Writing it last lets user-provided values win over every Buzz-set env + // written above — reserved keys were already stripped from descriptor.env so they + // cannot clobber BUZZ_PRIVATE_KEY, NOSTR_PRIVATE_KEY, etc. + for (key, value) in &descriptor.env { command.env(key, value); } configure_runtime_cli(&mut command, runtime_meta); diff --git a/desktop/src-tauri/src/managed_agents/runtime/tests.rs b/desktop/src-tauri/src/managed_agents/runtime/tests.rs index fd758047c3..ac82772017 100644 --- a/desktop/src-tauri/src/managed_agents/runtime/tests.rs +++ b/desktop/src-tauri/src/managed_agents/runtime/tests.rs @@ -1003,3 +1003,317 @@ fn invalid_pubkey_resolves_no_pair_key() { // the summary must fall back to the stopped/legacy-pid path, not panic. assert!(super::resolve_workspace_pair_key("not-a-key", "", "wss://one.example").is_none()); } + +// ── Custom-harness orphan sweep coverage ───────────────────────────────────── +// +// The system sweep gates must include any process carrying the +// `BUZZ_MANAGED_AGENT` env marker, regardless of whether the binary name +// matches `KNOWN_AGENT_BINARIES`. Custom harnesses use arbitrary binary names +// so name-match alone would silently leak their orphans on crash. +// +// Previously: macOS used a two-check OR+AND pattern (equivalent to just marker), +// Linux used an AND-gate (name + marker) — wrong for custom harnesses. +// Fix: all platforms use the shared `buzz_sweep_owns_process` predicate which +// returns `has_buzz_marker` only — the `_belongs_to_us` fast-skip is +// accepted for call-site symmetry but intentionally ignored. +// +// These tests call the production predicate directly so they fail if the +// predicate reverts to broken logic. + +use super::buzz_sweep_owns_process; + +/// A known-binary process WITHOUT the marker must be excluded — stray processes +/// with colliding names (e.g. another user's goose) are not ours. +#[test] +fn sweep_condition_known_binary_without_marker_is_excluded() { + assert!( + !buzz_sweep_owns_process(true, false), + "known binary without marker must be excluded" + ); +} + +/// A custom harness binary (not in KNOWN_AGENT_BINARIES) WITH the marker must +/// be included — this is the fix for the Linux AND-gate bug. +#[test] +fn sweep_condition_custom_binary_with_marker_is_included() { + assert!( + buzz_sweep_owns_process(false, true), + "custom binary with marker must be included" + ); +} + +/// A truly foreign process (not owned by name, no marker) must remain excluded. +#[test] +fn sweep_condition_foreign_process_is_excluded() { + assert!( + !buzz_sweep_owns_process(false, false), + "foreign process must always be excluded" + ); +} + +/// A known-binary process WITH the marker is owned — must be included. +#[test] +fn sweep_condition_known_binary_with_marker_is_included() { + assert!( + buzz_sweep_owns_process(true, true), + "known binary with marker must be included" + ); +} + +// ── I3: receipt path collector decision ───────────────────────────────────── +// +// `valid_agent_runtime_receipt` used to AND-gate process_belongs_to_us (a +// cheap name-check) with process_has_buzz_marker. Custom harnesses don't match +// KNOWN_AGENT_BINARIES, so their receipts would never be valid — the receipt +// cleanup loop would leave them running. The fix uses buzz_sweep_owns_process +// (marker-only) in valid_agent_runtime_receipt. +// +// These tests verify the predicate truth table that valid_agent_runtime_receipt +// now relies on. They would fail if the AND-gate were reinstated. + +/// Simulates valid_agent_runtime_receipt's ownership decision for a custom +/// harness: belongs_to_us=false (not in KNOWN_AGENT_BINARIES), has_marker=true. +/// Must be INCLUDED — the marker is authoritative, name is irrelevant. +/// +/// Would fail if valid_agent_runtime_receipt used process_belongs_to_us && +/// process_has_buzz_marker (AND-gate). +#[test] +fn receipt_ownership_custom_harness_with_marker_is_valid() { + // Custom binary: not in KNOWN_AGENT_BINARIES (belongs_to_us = false) + // but carries BUZZ_MANAGED_AGENT marker (has_buzz_marker = true). + assert!( + buzz_sweep_owns_process(false, true), + "custom harness with marker must be valid for receipt ownership" + ); +} + +/// Simulates valid_agent_runtime_receipt's ownership decision for a known +/// harness binary WITHOUT the marker (stray process, not owned by us). +/// Must be EXCLUDED. +#[test] +fn receipt_ownership_known_binary_without_marker_is_not_valid() { + // Known binary name (belongs_to_us = true) but no marker. + // This is a stray process that happens to share a binary name — must exclude. + assert!( + !buzz_sweep_owns_process(true, false), + "known binary without marker must not be valid for receipt ownership" + ); +} + +// ── Collector-discriminating sweep tests (C-9 / Thufir F6) ────────────────── +// +// `kill_stale_tracked_processes_with` and `valid_agent_runtime_receipt_with` +// accept injectable predicates so the sweep logic can be verified without +// spawning real processes. These tests drive the injection path directly, +// discriminating on custom-harness vs known-binary vs marker presence. + +#[test] +fn kill_stale_custom_harness_with_marker_is_terminated() { + // A record with a PID not in the live runtime map and with the Buzz marker + // should be terminated even though the binary name is not in KNOWN_AGENT_BINARIES. + let mut record = minimal_record("pubkey-custom"); + record.runtime_pid = Some(9001); + let mut records = vec![record]; + let runtimes = std::collections::HashMap::new(); + + let mut killed = vec![]; + let changed = super::kill_stale_tracked_processes_with( + &mut records, + &runtimes, + |_pid| true, // simulate: marker present (custom harness we own) + |pid| { + killed.push(pid); + Ok(()) + }, + ); + + assert!(changed, "stale record with marker should mark changed"); + assert_eq!(killed, vec![9001u32], "marked process must be killed"); + assert!( + records[0].runtime_pid.is_none(), + "runtime_pid must be cleared" + ); +} + +#[test] +fn kill_stale_process_without_marker_is_skipped() { + // A record PID without the marker (not our process — e.g. custom binary + // from another tool) should be skipped for termination but still cleared. + let mut record = minimal_record("pubkey-foreign"); + record.runtime_pid = Some(9002); + let mut records = vec![record]; + let runtimes = std::collections::HashMap::new(); + + let mut killed = vec![]; + let changed = super::kill_stale_tracked_processes_with( + &mut records, + &runtimes, + |_pid| false, // simulate: no marker (not our process) + |pid| { + killed.push(pid); + Ok(()) + }, + ); + + assert!( + changed, + "stale record without marker should still mark changed" + ); + assert!( + killed.is_empty(), + "process without marker must not be killed" + ); + assert!( + records[0].runtime_pid.is_none(), + "runtime_pid must be cleared regardless" + ); +} + +#[test] +fn kill_stale_live_pair_is_not_touched() { + // A record whose PID is in the live runtime map is NOT stale — skip it entirely. + use crate::managed_agents::ManagedAgentRuntimeKey; + let pubkey = "aa".repeat(32); // 64 hex chars — satisfies ManagedAgentRuntimeKey validation + let mut record = minimal_record(&pubkey); + record.runtime_pid = Some(9003); + let key = ManagedAgentRuntimeKey::new(pubkey, "wss://relay.example").unwrap(); + let mut runtimes = std::collections::HashMap::new(); + // Insert a placeholder runtime — value shape doesn't matter for the key lookup. + runtimes.insert(key, make_pair_runtime_placeholder()); + let original_pid = record.runtime_pid; + let mut records = vec![record]; + + let mut killed = vec![]; + let changed = super::kill_stale_tracked_processes_with( + &mut records, + &runtimes, + |_pid| true, + |pid| { + killed.push(pid); + Ok(()) + }, + ); + + assert!(!changed, "live pair must not be marked changed"); + assert!(killed.is_empty(), "live pair process must not be killed"); + assert_eq!( + records[0].runtime_pid, original_pid, + "live pair runtime_pid must not be cleared" + ); +} + +#[test] +fn receipt_valid_with_marker_and_running() { + // Custom harness receipt: belongs_to_us=false, has_marker=true, is_running=true. + // Must be valid — marker is the authoritative gate. + use crate::managed_agents::ManagedAgentRuntimeKey; + let key = ManagedAgentRuntimeKey::new("bb".repeat(32), "wss://relay.example").unwrap(); + let receipt = receipt_fixture(key.clone()); + let path = std::path::PathBuf::from(format!("{}.json", key.runtime_id())); + + let valid = super::valid_agent_runtime_receipt_with( + &path, + &receipt, + "test-instance", + |_pid| true, // is_running + |_pid| false, // belongs_to_us: false (custom binary) + |_pid, _iid| true, // has_marker: true + ); + assert!( + valid, + "custom harness with marker and running pid must be valid" + ); +} + +#[test] +fn receipt_invalid_known_binary_without_marker() { + // Known binary name but no marker — stray process, must not be valid. + use crate::managed_agents::ManagedAgentRuntimeKey; + let key = ManagedAgentRuntimeKey::new("cc".repeat(32), "wss://relay.example").unwrap(); + let receipt = receipt_fixture(key.clone()); + let path = std::path::PathBuf::from(format!("{}.json", key.runtime_id())); + + let valid = super::valid_agent_runtime_receipt_with( + &path, + &receipt, + "test-instance", + |_pid| true, // is_running + |_pid| true, // belongs_to_us: true (known binary) + |_pid, _iid| false, // has_marker: false (not our process) + ); + assert!(!valid, "known binary without marker must not be valid"); +} + +#[test] +fn receipt_invalid_when_process_not_running() { + // Even with marker, a non-running process must not be valid. + use crate::managed_agents::ManagedAgentRuntimeKey; + let key = ManagedAgentRuntimeKey::new("dd".repeat(32), "wss://relay.example").unwrap(); + let receipt = receipt_fixture(key.clone()); + let path = std::path::PathBuf::from(format!("{}.json", key.runtime_id())); + + let valid = super::valid_agent_runtime_receipt_with( + &path, + &receipt, + "test-instance", + |_pid| false, // is_running: false + |_pid| true, // belongs_to_us + |_pid, _iid| true, // has_marker + ); + assert!( + !valid, + "non-running process must not be valid regardless of marker" + ); +} + +// ── Test helpers ──────────────────────────────────────────────────────────── + +fn minimal_record(pubkey: &str) -> crate::managed_agents::ManagedAgentRecord { + serde_json::from_str(&format!( + r#"{{ + "pubkey": "{pubkey}", + "name": "test", + "private_key_nsec": "nsec1fake", + "relay_url": "", + "acp_command": "buzz-acp", + "agent_command": "buzz-agent", + "agent_args": [], + "mcp_command": "", + "turn_timeout_seconds": 320, + "system_prompt": null, + "model": null, + "provider": null, + "env_vars": {{}}, + "created_at": "2026-01-01T00:00:00Z", + "updated_at": "2026-01-01T00:00:00Z", + "last_started_at": null, + "last_stopped_at": null, + "last_exit_code": null, + "last_error": null + }}"# + )) + .expect("minimal_record fixture") +} + +fn make_pair_runtime_placeholder() -> crate::managed_agents::ManagedAgentPairRuntime { + use std::process::{Command, Stdio}; + // Spawn a real child so ManagedAgentProcess's Child field is satisfied. + // `true` exits immediately with 0 — just a handle we need for type purposes. + let child = Command::new("true") + .stdin(Stdio::null()) + .stdout(Stdio::null()) + .stderr(Stdio::null()) + .spawn() + .expect("spawn true for placeholder"); + let process = crate::managed_agents::ManagedAgentProcess { + child, + log_path: std::path::PathBuf::new(), + spawn_config_hash: 0, + setup_mode: false, + adapter_availability: None, + start_nonce: "test-nonce".to_string(), + #[cfg(windows)] + job: None, + }; + crate::managed_agents::ManagedAgentPairRuntime::starting(process) +} diff --git a/desktop/src-tauri/src/managed_agents/spawn_hash.rs b/desktop/src-tauri/src/managed_agents/spawn_hash.rs index 4d80549288..f358a745b2 100644 --- a/desktop/src-tauri/src/managed_agents/spawn_hash.rs +++ b/desktop/src-tauri/src/managed_agents/spawn_hash.rs @@ -30,7 +30,6 @@ use std::hash::{DefaultHasher, Hash, Hasher}; use super::{ known_acp_runtime, normalize_agent_args, persona_events::apply_persona_snapshot, - resolve_effective_agent_env, types::{AgentDefinition, ManagedAgentRecord, TeamRecord}, GlobalAgentConfig, }; @@ -86,24 +85,40 @@ pub(crate) fn spawn_config_hash( } let record = &record; - let effective_command = crate::managed_agents::record_agent_command(record, personas); - let runtime_meta = known_acp_runtime(&effective_command); - let effective = resolve_effective_agent_env(record, personas, runtime_meta, global); + // Resolve command, args, and env via the single typed descriptor — same path + // as spawn_agent_child. Dangling harness id falls back to the infallible + // record_agent_command (no-op: a dangling harness can't be spawned, so the + // hash never matters for that agent). + let descriptor = + crate::managed_agents::resolve_effective_harness_descriptor(record, personas, global) + .unwrap_or_else(|_| { + let cmd = crate::managed_agents::record_agent_command(record, personas); + let args = normalize_agent_args(&cmd, record.agent_args.clone()); + crate::managed_agents::readiness::EffectiveHarnessDescriptor { + command: cmd, + args, + env: Default::default(), + } + }); + let runtime_meta = known_acp_runtime(&descriptor.command); + + // `resolve_effective_agent_env` now includes definition env as a floor + // layer (below global/persona/agent), mirroring spawn_agent_child exactly. let mut hasher = DefaultHasher::new(); // Harness identity and derivations (live-persona-resolved, like spawn). record.acp_command.hash(&mut hasher); - effective_command.hash(&mut hasher); - normalize_agent_args(&effective_command, record.agent_args.clone()).hash(&mut hasher); + descriptor.command.hash(&mut hasher); + descriptor.args.hash(&mut hasher); runtime_meta .and_then(|r| r.mcp_command) .unwrap_or("") .hash(&mut hasher); - // Effective env layering (baked floor → runtime metadata → user env). - // BTreeMap iteration is ordered, so this is deterministic. - effective.env.hash(&mut hasher); + // Effective env layering (baked floor → runtime metadata → definition env + // → global → persona → agent). BTreeMap iteration is ordered, deterministic. + descriptor.env.hash(&mut hasher); // Record fields the spawn env writes read directly. The relay is hashed // resolved: every record spawns on the workspace relay (legacy pins diff --git a/desktop/src-tauri/src/managed_agents/spawn_hash/tests.rs b/desktop/src-tauri/src/managed_agents/spawn_hash/tests.rs index 6438b3b37d..cb030e7167 100644 --- a/desktop/src-tauri/src/managed_agents/spawn_hash/tests.rs +++ b/desktop/src-tauri/src/managed_agents/spawn_hash/tests.rs @@ -456,3 +456,123 @@ fn effective_spawn_prompt_matches_hash_semantics() { r.system_prompt = Some("real".into()); assert_eq!(effective_spawn_prompt(&r).as_deref(), Some("real")); } + +// ── I2: definition args and env reach spawn_config_hash ────────────────────── +// +// These tests prove that editing a custom harness definition's args or env +// changes spawn_config_hash, which trips the "restart required" badge. +// They would fail if spawn_config_hash used only record.agent_args without +// falling back to definition args, or if resolve_effective_agent_env did not +// include definition env. + +/// When a record has no instance args but the definition has default args, +/// changing the definition args changes the spawn hash. This would fail if +/// spawn_config_hash used only record.agent_args. +#[test] +fn spawn_hash_changes_when_definition_default_args_change() { + use crate::managed_agents::custom_harnesses::warm_harness_registry_from_dir; + use std::fs; + use tempfile::tempdir; + + let dir = tempdir().unwrap(); + + // Write v1 definition (args: ["--mode", "v1"]). + fs::write( + dir.path().join("my-def.json"), + r#"{"id":"my-def","label":"My Def","command":"my-def-bin","args":["--mode","v1"]}"#, + ) + .unwrap(); + warm_harness_registry_from_dir(Some(dir.path())); + + let mut r = record(); + r.runtime = Some("my-def".into()); + r.agent_args = vec![]; // no instance args → definition args are used + + let h1 = spawn_config_hash(&r, &[], &[], "ws://relay", &Default::default()); + + // Update to v2 args and re-warm (simulating save + transactional refresh). + fs::write( + dir.path().join("my-def.json"), + r#"{"id":"my-def","label":"My Def","command":"my-def-bin","args":["--mode","v2"]}"#, + ) + .unwrap(); + warm_harness_registry_from_dir(Some(dir.path())); + + let h2 = spawn_config_hash(&r, &[], &[], "ws://relay", &Default::default()); + + assert_ne!( + h1, h2, + "changing definition default args must change the spawn hash" + ); +} + +/// When a definition has env vars, adding them changes the spawn hash. This +/// proves resolve_effective_agent_env includes definition env in the layering. +#[test] +fn spawn_hash_changes_when_definition_env_changes() { + use crate::managed_agents::custom_harnesses::warm_harness_registry_from_dir; + use std::fs; + use tempfile::tempdir; + + let dir = tempdir().unwrap(); + + // Write definition without env. + fs::write( + dir.path().join("env-def.json"), + r#"{"id":"env-def","label":"Env Def","command":"env-def-bin"}"#, + ) + .unwrap(); + warm_harness_registry_from_dir(Some(dir.path())); + + let mut r = record(); + r.runtime = Some("env-def".into()); + + let h1 = spawn_config_hash(&r, &[], &[], "ws://relay", &Default::default()); + + // Update to include env and re-warm. + fs::write( + dir.path().join("env-def.json"), + r#"{"id":"env-def","label":"Env Def","command":"env-def-bin","env":{"MY_FLAG":"1"}}"#, + ) + .unwrap(); + warm_harness_registry_from_dir(Some(dir.path())); + + let h2 = spawn_config_hash(&r, &[], &[], "ws://relay", &Default::default()); + + assert_ne!(h1, h2, "adding definition env must change the spawn hash"); +} + +/// Instance-level args win over definition default args (non-empty instance +/// args must NOT be overridden by the definition). The hash must match a record +/// that has the same effective args from either source. +#[test] +fn spawn_hash_instance_args_win_over_definition_args() { + use crate::managed_agents::custom_harnesses::warm_harness_registry_from_dir; + use std::fs; + use tempfile::tempdir; + + let dir = tempdir().unwrap(); + fs::write( + dir.path().join("arg-def.json"), + r#"{"id":"arg-def","label":"Arg Def","command":"arg-def-bin","args":["--def-arg"]}"#, + ) + .unwrap(); + warm_harness_registry_from_dir(Some(dir.path())); + + let mut r_instance = record(); + r_instance.runtime = Some("arg-def".into()); + r_instance.agent_args = vec!["--instance-arg".to_string()]; + + let mut r_no_instance = record(); + r_no_instance.runtime = Some("arg-def".into()); + r_no_instance.agent_args = vec![]; + + let h_instance = spawn_config_hash(&r_instance, &[], &[], "ws://relay", &Default::default()); + let h_no_instance = + spawn_config_hash(&r_no_instance, &[], &[], "ws://relay", &Default::default()); + + assert_ne!( + h_instance, h_no_instance, + "instance args and definition args must produce different hashes" + ); +} diff --git a/desktop/src-tauri/src/managed_agents/types.rs b/desktop/src-tauri/src/managed_agents/types.rs index af4908911b..591729c83d 100644 --- a/desktop/src-tauri/src/managed_agents/types.rs +++ b/desktop/src-tauri/src/managed_agents/types.rs @@ -565,6 +565,19 @@ pub enum AuthStatus { Unknown, } +/// Origin of an ACP runtime catalog entry. Serializes as a lowercase string +/// so the TypeScript consumer can switch on it without numeric comparisons. +#[derive(Debug, Clone, Serialize, PartialEq, Eq)] +#[serde(rename_all = "snake_case")] +pub enum HarnessSource { + /// Compiled into the app — one of the four first-class runtimes. + Builtin, + /// Static preset entry with bundled logo, PATH-probed, not editable/deletable. + Preset, + /// Loaded at runtime from the user's `custom_harnesses/` directory. + Custom, +} + #[derive(Debug, Clone, Serialize)] pub struct AcpRuntimeCatalogEntry { pub id: String, @@ -596,6 +609,19 @@ pub struct AcpRuntimeCatalogEntry { /// Hint for completing authentication, shown when `auth_status` is not `logged_in`. #[serde(skip_serializing_if = "Option::is_none")] pub login_hint: Option, + /// Whether this entry came from the compiled-in catalog or a user-supplied + /// JSON file in `custom_harnesses/`. The UI uses this to decide editability. + pub source: HarnessSource, + /// Definition-level environment variables for `source: custom` entries. + /// + /// Populated from `HarnessDefinition.env` so the edit form can read them + /// back and the user doesn't silently lose env vars when saving. Always + /// empty for `builtin` and `preset` entries (those env values come from the + /// runtime metadata path, not user-editable JSON). + /// + /// Skipped in serialization when empty to keep the catalog payload compact. + #[serde(default, skip_serializing_if = "BTreeMap::is_empty")] + pub definition_env: BTreeMap, } /// Result of a single install step (CLI or adapter). diff --git a/desktop/src/app/App.tsx b/desktop/src/app/App.tsx index 9561b2138c..90b5bf5ebc 100644 --- a/desktop/src/app/App.tsx +++ b/desktop/src/app/App.tsx @@ -33,6 +33,7 @@ import { CommunityOnboardingFlow } from "@/features/onboarding/ui/CommunityOnboa import { MachineOnboardingFlow, type MachineOnboardingPage, + type PostOnboardingNavigation, } from "@/features/onboarding/ui/MachineOnboardingFlow"; import { OnboardingFlow } from "@/features/onboarding/ui/OnboardingFlow"; import { PendingInviteGate } from "@/features/onboarding/ui/PendingInviteGate"; @@ -592,6 +593,8 @@ function MachineBootstrap({ sharedIdentity }: { sharedIdentity: boolean }) { }); const [machineInitialPage, setMachineInitialPage] = useState(); + const [postOnboardingNav, setPostOnboardingNav] = + useState(null); const reopenMachineConfig = useCallback(() => { setMachineInitialPage("config"); @@ -606,6 +609,27 @@ function MachineBootstrap({ sharedIdentity }: { sharedIdentity: boolean }) { [machine.complete], ); + const navigateAfterOnboarding = useCallback( + (nav: PostOnboardingNavigation) => { + setPostOnboardingNav(nav); + }, + [], + ); + + // Execute the pending navigation once the RouterProvider is mounted (i.e. + // machine.stage transitions to "ready"). We wait for the ready stage rather + // than using setTimeout(0) so the router is guaranteed to exist before we call + // router.navigate(). + useEffect(() => { + if (machine.stage === "ready" && postOnboardingNav) { + void router.navigate({ + to: postOnboardingNav.to, + search: postOnboardingNav.search ?? {}, + }); + setPostOnboardingNav(null); + } + }, [machine.stage, postOnboardingNav]); + const openAddCommunity = useCallback( (payload: AddCommunityDeepLinkPayload & { requestId: string }) => activeCommunity @@ -664,6 +688,7 @@ function MachineBootstrap({ sharedIdentity }: { sharedIdentity: boolean }) { continueWithIdentity={machine.continueWithIdentity} identityLost={machine.identityLost} initialPage={machineInitialPage} + navigateAfterComplete={navigateAfterOnboarding} queryClient={machine.queryClient} /> {shouldAcknowledgeDeepLink ? : null} diff --git a/desktop/src/app/postOnboardingNav.test.mjs b/desktop/src/app/postOnboardingNav.test.mjs new file mode 100644 index 0000000000..2be2eab95f --- /dev/null +++ b/desktop/src/app/postOnboardingNav.test.mjs @@ -0,0 +1,123 @@ +/** + * Tests for the post-onboarding navigation contract (Thufir F8). + * + * When the user clicks "Set up agents" during machine onboarding, + * MachineOnboardingFlow calls complete() then navigateAfterComplete({to, search}). + * App.tsx records the pending nav and fires it only when machine.stage === "ready" + * (i.e. once RouterProvider is mounted), never before. + * + * These are pure-logic tests — they simulate the App.tsx useEffect predicate + * directly. No React rendering needed; the behavior under test is: + * pendingNav × stage → navigate() call count. + */ +import assert from "node:assert/strict"; +import test from "node:test"; + +// ── Simulate the App.tsx postOnboardingNav useEffect logic ────────────────── +// +// The production code: +// useEffect(() => { +// if (machine.stage === "ready" && postOnboardingNav) { +// void router.navigate({ to: postOnboardingNav.to, search: ... }); +// setPostOnboardingNav(null); +// } +// }, [machine.stage, postOnboardingNav]); +// +// We simulate this as a pure function and drive it through stage transitions. + +function runEffect(stage, nav, navigate) { + if (stage === "ready" && nav !== null) { + navigate({ to: nav.to, search: nav.search ?? {} }); + return null; // cleared + } + return nav; // unchanged +} + +test("navigate does not fire before stage reaches ready", () => { + const calls = []; + const navigate = (args) => calls.push(args); + + const nav = { to: "/settings", search: { section: "agents" } }; + + // Drive through non-ready stages — navigate must not fire. + for (const stage of ["loading", "onboarding", "blocking"]) { + runEffect(stage, nav, navigate); + } + + assert.equal(calls.length, 0, "navigate must not be called before ready"); +}); + +test("navigate fires exactly once when stage transitions to ready", () => { + const calls = []; + const navigate = (args) => calls.push(args); + + const nav = { to: "/settings", search: { section: "agents" } }; + + // Pre-ready stages — no call. + let pending = nav; + pending = runEffect("onboarding", pending, navigate); + + // Stage reaches ready — navigate fires once. + pending = runEffect("ready", pending, navigate); + + assert.equal(calls.length, 1, "navigate must fire exactly once on ready"); + assert.equal(calls[0].to, "/settings"); + assert.deepStrictEqual(calls[0].search, { section: "agents" }); +}); + +test("navigate does not fire again after nav is cleared", () => { + const calls = []; + const navigate = (args) => calls.push(args); + + const nav = { to: "/settings", search: { section: "agents" } }; + let pending = nav; + + // First ready transition fires and clears. + pending = runEffect("ready", pending, navigate); + assert.equal(calls.length, 1); + assert.equal(pending, null, "pending nav must be cleared after firing"); + + // Re-running the effect with null nav must not fire again. + pending = runEffect("ready", pending, navigate); + assert.equal( + calls.length, + 1, + "navigate must not fire again after being cleared", + ); +}); + +test("navigate fires immediately if nav is set while already ready", () => { + // Edge case: nav arrives after the machine is already ready (e.g. hot reload). + const calls = []; + const navigate = (args) => calls.push(args); + + const nav = { to: "/settings", search: { section: "agents" } }; + + // Start with no pending nav while ready. + let pending = runEffect("ready", null, navigate); + assert.equal(calls.length, 0, "no nav pending → no call"); + + // Nav arrives (complete() called while already ready). + pending = runEffect("ready", nav, navigate); + assert.equal(calls.length, 1, "nav must fire immediately when already ready"); + assert.equal(pending, null); +}); + +test("navigate intent carries exact to and search from navigateToAgentSettings", () => { + // Verifies the specific shape emitted by MachineOnboardingFlow's + // navigateToAgentSettings action: { to: '/settings', search: { section: 'agents' } } + const calls = []; + const navigate = (args) => calls.push(args); + + // Simulate MachineOnboardingFlow calling navigateAfterComplete. + const nav = { to: "/settings", search: { section: "agents" } }; + runEffect("ready", nav, navigate); + + assert.equal(calls.length, 1); + assert.equal(calls[0].to, "/settings", "must navigate to /settings"); + assert.equal( + calls[0].search.section, + "agents", + "must include section=agents", + ); +}); diff --git a/desktop/src/features/agents/channelAgents.ts b/desktop/src/features/agents/channelAgents.ts index edbb106817..70a431a1e5 100644 --- a/desktop/src/features/agents/channelAgents.ts +++ b/desktop/src/features/agents/channelAgents.ts @@ -233,7 +233,9 @@ export async function ensureChannelAgentPresetInChannel( name: expectedName, acpCommand: "buzz-acp", agentCommand: input.runtime.command, - agentArgs: input.runtime.defaultArgs, + // Do NOT seed agentArgs from runtime.defaultArgs (see instanceInputForDefinition.ts + // for the rationale — empty args let spawn resolve definition args live). + agentArgs: [], mcpCommand: input.runtime.mcpCommand ?? "", spawnAfterCreate: false, }); @@ -354,7 +356,9 @@ export async function provisionChannelManagedAgent( acpCommand: "buzz-acp", agentCommand: input.runtime.command, harnessOverride: input.harnessOverride ?? false, - agentArgs: input.runtime.defaultArgs, + // Do NOT seed agentArgs from runtime.defaultArgs (see instanceInputForDefinition.ts + // for the rationale — empty args let spawn resolve definition args live). + agentArgs: [], mcpCommand: input.runtime.mcpCommand ?? "", personaId: input.personaId ?? undefined, teamId: input.teamId ?? undefined, diff --git a/desktop/src/features/agents/hooks.ts b/desktop/src/features/agents/hooks.ts index 128ae9083b..199ee7e881 100644 --- a/desktop/src/features/agents/hooks.ts +++ b/desktop/src/features/agents/hooks.ts @@ -21,6 +21,7 @@ import { evictUsersBatchEntries } from "@/features/profile/hooks"; import { createManagedAgent, deleteManagedAgent, + deleteCustomHarness, discoverAcpRuntimes, discoverBackendProviders, discoverGitBashPrerequisite, @@ -34,8 +35,10 @@ import { installAcpRuntime, listManagedAgents, listRelayAgents, + saveCustomHarness, updateManagedAgent, } from "@/shared/api/tauri"; +import type { HarnessDefinitionInput } from "@/shared/api/tauri"; import { setManagedAgentAutoRestart, setManagedAgentStartOnAppLaunch, @@ -237,6 +240,32 @@ export function useInstallAcpRuntimeMutation() { }); } +export function useSaveCustomHarnessMutation() { + const queryClient = useQueryClient(); + return useMutation({ + mutationFn: ({ + definition, + originalId, + }: { + definition: HarnessDefinitionInput; + originalId?: string; + }) => saveCustomHarness(definition, originalId), + onSettled: () => { + void queryClient.invalidateQueries({ queryKey: acpRuntimesQueryKey }); + }, + }); +} + +export function useDeleteCustomHarnessMutation() { + const queryClient = useQueryClient(); + return useMutation({ + mutationFn: (id: string) => deleteCustomHarness(id), + onSettled: () => { + void queryClient.invalidateQueries({ queryKey: acpRuntimesQueryKey }); + }, + }); +} + export function useGitBashPrerequisiteQuery() { return useQuery({ queryKey: gitBashPrerequisiteQueryKey, diff --git a/desktop/src/features/agents/lib/instanceInputForDefinition.test.mjs b/desktop/src/features/agents/lib/instanceInputForDefinition.test.mjs index d4ba32e603..d79c7abcf8 100644 --- a/desktop/src/features/agents/lib/instanceInputForDefinition.test.mjs +++ b/desktop/src/features/agents/lib/instanceInputForDefinition.test.mjs @@ -142,7 +142,11 @@ test("mapping carries the runtime and definition fields", async () => { assert.equal(input.name, "Test Agent"); assert.equal(input.acpCommand, "buzz-acp"); assert.equal(input.agentCommand, "goose-cmd"); - assert.deepEqual(input.agentArgs, ["--acp"]); + // B-5: agentArgs is intentionally empty at create time — spawn reads args + // live from the definition on every start so definition edits take effect + // without recreating the agent. Seeding from runtime.defaultArgs here would + // freeze args at create-time and silently ignore later definition edits. + assert.deepEqual(input.agentArgs, []); assert.equal(input.mcpCommand, "goose-mcp"); assert.equal(input.personaId, "p-1"); assert.equal(input.systemPrompt, "prompt"); @@ -156,6 +160,8 @@ test("mapping carries the runtime and definition fields", async () => { test("no backend intent is byte-identical to the pre-intent mapping", async () => { // The 3 pre-B5 call sites (useManagedAgentActions, usePersonaActions, // UserProfilePanel) pass no intent; their output must not move. + // B-5: agentArgs is [] — args are NOT seeded from the definition at create + // time. Spawn reads live args from the definition on every start. const input = await buildInstanceInputForDefinition(persona(), gooseRuntime); assert.deepEqual(input, { name: "Test Agent", @@ -164,7 +170,7 @@ test("no backend intent is byte-identical to the pre-intent mapping", async () = avatarUrl: "https://example.com/a.png", acpCommand: "buzz-acp", agentCommand: "goose-cmd", - agentArgs: ["--acp"], + agentArgs: [], mcpCommand: "goose-mcp", harnessOverride: true, model: undefined, diff --git a/desktop/src/features/agents/lib/instanceInputForDefinition.ts b/desktop/src/features/agents/lib/instanceInputForDefinition.ts index 7db36b7241..5919309224 100644 --- a/desktop/src/features/agents/lib/instanceInputForDefinition.ts +++ b/desktop/src/features/agents/lib/instanceInputForDefinition.ts @@ -143,7 +143,13 @@ export async function buildInstanceInputForDefinition( ...base, acpCommand: "buzz-acp", agentCommand: runtime.command, - agentArgs: runtime.defaultArgs, + // Do NOT seed agentArgs from runtime.defaultArgs: record.agent_args must + // remain empty so spawn resolves args live from the definition on every + // start. Seeding here would freeze the args at create-time, silently + // ignoring any later definition-arg edits (Thufir F5 / phase B-5). + // envVars are intentionally never seeded for the same reason (see comment + // at top of this function). + agentArgs: [], mcpCommand: runtime.mcpCommand ?? "", harnessOverride: !persona.runtime || persona.runtime === runtime.id, model: persona.model ?? undefined, diff --git a/desktop/src/features/agents/ui/usePersonaModelDiscovery.ts b/desktop/src/features/agents/ui/usePersonaModelDiscovery.ts index b0ab0f8379..83ebe5396e 100644 --- a/desktop/src/features/agents/ui/usePersonaModelDiscovery.ts +++ b/desktop/src/features/agents/ui/usePersonaModelDiscovery.ts @@ -204,6 +204,7 @@ export function usePersonaModelDiscovery({ // abandon and re-issue an in-flight discovery IPC call. const selectedRuntimeAvailability = selectedRuntime?.availability; const selectedRuntimeDefaultArgs = selectedRuntime?.defaultArgs; + const selectedRuntimeDefinitionEnv = selectedRuntime?.definitionEnv; const canDiscoverModelOptions = open && modelFieldVisible && @@ -289,6 +290,7 @@ export function usePersonaModelDiscovery({ agentArgs: selectedRuntimeDefaultArgs ?? [], provider: trimmedProvider || undefined, envVars, + definitionEnv: selectedRuntimeDefinitionEnv ?? {}, }) .then((response) => { if (modelDiscoveryRequestRef.current !== requestId) { @@ -352,6 +354,7 @@ export function usePersonaModelDiscovery({ modelDiscoveryKey, selectedRuntimeAvailability, selectedRuntimeDefaultArgs, + selectedRuntimeDefinitionEnv, shouldDebounceModelDiscovery, trimmedProvider, ]); diff --git a/desktop/src/features/onboarding/ui/MachineOnboardingFlow.tsx b/desktop/src/features/onboarding/ui/MachineOnboardingFlow.tsx index e400d07a91..d7fc9071d8 100644 --- a/desktop/src/features/onboarding/ui/MachineOnboardingFlow.tsx +++ b/desktop/src/features/onboarding/ui/MachineOnboardingFlow.tsx @@ -28,18 +28,32 @@ export type MachineOnboardingPage = | "setup" | "config"; +/** A pending navigation the parent should execute after RouterProvider mounts. */ +export type PostOnboardingNavigation = { + to: string; + search?: Record; +}; + export function MachineOnboardingFlow({ complete, continueWithIdentity, identityLost, initialPage, queryClient, + navigateAfterComplete, }: { complete: (pubkey?: string) => void; continueWithIdentity: (pubkey: string) => void; identityLost: boolean; initialPage?: MachineOnboardingPage; queryClient: QueryClient; + /** + * Called when the user finishes onboarding and requests navigation to a + * specific route (e.g. Settings → Agents). The parent owns the RouterProvider, + * so navigation must be deferred to it — calling router.navigate() here races + * with RouterProvider mounting. + */ + navigateAfterComplete?: (nav: PostOnboardingNavigation) => void; }) { const [page, setPage] = React.useState( identityLost ? "key-import" : (initialPage ?? "identity"), @@ -225,6 +239,17 @@ export function MachineOnboardingFlow({ } setPage("config"); }, + navigateToAgentSettings: () => { + // Complete onboarding first, then delegate the Settings → Agents + // navigation to the parent. The parent owns RouterProvider, so + // navigation from within the onboarding flow races with the + // router mounting — calling router.navigate() here is unsafe. + complete(selectedPubkey ?? undefined); + navigateAfterComplete?.({ + to: "/settings", + search: { section: "agents" }, + }); + }, }} direction="forward" onReadyRuntimeIdsChange={handleReadyRuntimeIdsChange} diff --git a/desktop/src/features/onboarding/ui/RuntimeIcon.tsx b/desktop/src/features/onboarding/ui/RuntimeIcon.tsx index a410914377..2debea409b 100644 --- a/desktop/src/features/onboarding/ui/RuntimeIcon.tsx +++ b/desktop/src/features/onboarding/ui/RuntimeIcon.tsx @@ -9,12 +9,25 @@ import chatgptLogoUrl from "../assets/harness-logos/chatgpt.png?inline"; import claudeLogoUrl from "../assets/harness-logos/claude.png?inline"; import gooseLogoUrl from "../assets/harness-logos/goose.png?inline"; +// Bundled logos for compiled-in runtimes (inline base64, no network fetch). const RUNTIME_LOGOS: Record = { claude: claudeLogoUrl, codex: chatgptLogoUrl, goose: gooseLogoUrl, }; +// Public-path logos for bundled presets. Served from /harness-logos/ at runtime. +// Keys match the preset `id` values emitted by the backend PRESET_HARNESSES. +export const PRESET_LOGOS: Record = { + omp: "/harness-logos/omp.svg", + grok: "/harness-logos/grok.svg", + opencode: "/harness-logos/opencode.svg", + kimi: "/harness-logos/kimi.png", + amp: "/harness-logos/amp.png", + hermes: "/harness-logos/hermes.png", + openclaw: "/harness-logos/openclaw.svg", +}; + function isBuzzRuntime(runtime: AcpRuntimeCatalogEntry): boolean { return runtime.id.trim().toLowerCase() === "buzz-agent"; } @@ -26,7 +39,8 @@ export function getRuntimeDisplayLabel( } function getRuntimeLogoUrl(runtime: AcpRuntimeCatalogEntry): string | null { - return RUNTIME_LOGOS[runtime.id.trim().toLowerCase()] ?? null; + const id = runtime.id.trim().toLowerCase(); + return RUNTIME_LOGOS[id] ?? PRESET_LOGOS[id] ?? null; } export function RuntimeIcon({ @@ -38,9 +52,11 @@ export function RuntimeIcon({ }) { const [imageFailed, setImageFailed] = React.useState(false); const { isDark } = useTheme(); - const runtimeLogoUrl = getRuntimeLogoUrl(runtime); - const imageUrl = runtimeLogoUrl ?? runtime.avatarUrl; - const shouldForceForegroundColor = !runtimeLogoUrl && runtime.id === "goose"; + // Only use bundled logo maps — never render user-supplied avatar URLs for + // custom/preset entries (tracking pixel / spoofing vector, security line). + const id = runtime.id.trim().toLowerCase(); + const imageUrl = getRuntimeLogoUrl(runtime); + const shouldForceForegroundColor = !imageUrl && id === "goose"; if (isBuzzRuntime(runtime)) { return ; @@ -53,6 +69,8 @@ export function RuntimeIcon({ className={cn( "rounded-md object-contain", className, + id === "omp" && "bg-[#0d0d0d] p-1", + id === "grok" && "bg-white p-1", shouldForceForegroundColor && (isDark ? "brightness-0 invert" : "brightness-0"), )} diff --git a/desktop/src/features/onboarding/ui/SetupStep.tsx b/desktop/src/features/onboarding/ui/SetupStep.tsx index 288ca30259..c66e6e80c1 100644 --- a/desktop/src/features/onboarding/ui/SetupStep.tsx +++ b/desktop/src/features/onboarding/ui/SetupStep.tsx @@ -714,6 +714,23 @@ function SetupStepContent({ > Back + +

+ More harnesses (Cursor, Grok, Amp…){" "} + {actions.navigateToAgentSettings ? ( + + ) : ( + Settings → Agents + )}{" "} + after setup. +

); diff --git a/desktop/src/features/onboarding/ui/presetLogos.test.mjs b/desktop/src/features/onboarding/ui/presetLogos.test.mjs new file mode 100644 index 0000000000..6f477fff65 --- /dev/null +++ b/desktop/src/features/onboarding/ui/presetLogos.test.mjs @@ -0,0 +1,90 @@ +/** + * Preset-logo coverage guard. + * + * Every tier-2 preset the backend emits must have a bundled logo, or it renders + * as the generic TerminalSquare fallback next to siblings that show real marks. + * The two sides live in different languages — Rust `PRESET_HARNESSES` vs the TS + * `PRESET_LOGOS` record — so no compiler catches drift, and `RuntimeIcon`'s + * `onError` fallback hides a missing file at runtime. This test reads the Rust + * source as text (the same trick `motion.test.mjs` uses for CSS) and asserts + * both directions plus on-disk existence of every mapped file. + */ + +import assert from "node:assert/strict"; +import { existsSync, readFileSync } from "node:fs"; +import path from "node:path"; +import test from "node:test"; +import { fileURLToPath } from "node:url"; + +import { PRESET_LOGOS } from "./RuntimeIcon.tsx"; + +const desktopRoot = path.resolve( + path.dirname(fileURLToPath(import.meta.url)), + "../../../..", +); + +const discoveryRs = readFileSync( + path.join(desktopRoot, "src-tauri/src/managed_agents/discovery.rs"), + "utf8", +); + +const presetBlock = discoveryRs.match( + /const PRESET_HARNESSES: &\[PresetHarness\] = &\[([\s\S]*?)\n\];/, +); +assert.ok(presetBlock, "could not locate PRESET_HARNESSES in discovery.rs"); + +const presetIds = [...presetBlock[1].matchAll(/^\s{8}id: "([^"]+)",$/gm)].map( + (match) => match[1], +); + +test("PRESET_HARNESSES parse found the preset ids", () => { + // Guards the regex itself: a struct-field rename would otherwise silently + // yield zero ids and make every assertion below vacuously pass. + assert.ok( + presetIds.length >= 8, + `expected at least 8 preset ids, parsed ${presetIds.length}`, + ); +}); + +const FALLBACK_ONLY_PRESETS = new Set([ + // Cursor publishes official assets, but neither its brand page nor terms grant + // third parties permission to redistribute them. Keep the generic icon. + "cursor", +]); + +for (const id of presetIds) { + test(`preset "${id}" has a bundled logo or an approved fallback`, () => { + const logoPath = PRESET_LOGOS[id]; + if (FALLBACK_ONLY_PRESETS.has(id)) { + assert.equal( + logoPath, + undefined, + `preset "${id}" must keep the generic TerminalSquare fallback until ` + + "its vendor grants logo redistribution permission", + ); + return; + } + assert.ok( + logoPath, + `preset "${id}" has no PRESET_LOGOS entry — it renders the generic ` + + `TerminalSquare fallback. Add desktop/public${logoPath ?? `/harness-logos/${id}.png`} ` + + `and map it in RuntimeIcon.tsx.`, + ); + assert.ok( + existsSync(path.join(desktopRoot, "public", logoPath)), + `PRESET_LOGOS["${id}"] points at ${logoPath}, which is missing from ` + + `desktop/public — RuntimeIcon's onError would silently fall back.`, + ); + }); +} + +test("PRESET_LOGOS has no entries for unknown presets", () => { + const unknown = Object.keys(PRESET_LOGOS).filter( + (id) => !presetIds.includes(id), + ); + assert.deepEqual( + unknown, + [], + `PRESET_LOGOS maps ids the backend does not emit as presets: ${unknown.join(", ")}`, + ); +}); diff --git a/desktop/src/features/onboarding/ui/types.ts b/desktop/src/features/onboarding/ui/types.ts index 9b2c4be8d2..443a1f3f4e 100644 --- a/desktop/src/features/onboarding/ui/types.ts +++ b/desktop/src/features/onboarding/ui/types.ts @@ -59,6 +59,7 @@ export type ProfileStepActions = { export type SetupStepActions = { back: () => void; next: (readyRuntimeIds: readonly string[]) => void; + navigateToAgentSettings?: () => void; }; export type DefaultConfigStepActions = { diff --git a/desktop/src/features/settings/ui/HarnessManagementCard.tsx b/desktop/src/features/settings/ui/HarnessManagementCard.tsx new file mode 100644 index 0000000000..e993e77390 --- /dev/null +++ b/desktop/src/features/settings/ui/HarnessManagementCard.tsx @@ -0,0 +1,651 @@ +import * as React from "react"; +import { ExternalLink, Plus, Terminal, Trash2, X } from "lucide-react"; +import { openUrl } from "@tauri-apps/plugin-opener"; + +import { + useAcpRuntimesQuery, + useDeleteCustomHarnessMutation, + useManagedAgentPrereqsQuery, + useSaveCustomHarnessMutation, +} from "@/features/agents/hooks"; +import type { AcpRuntimeCatalogEntry } from "@/shared/api/types"; +import { cn } from "@/shared/lib/cn"; +import { Button } from "@/shared/ui/button"; +import { Input } from "@/shared/ui/input"; +import { Spinner } from "@/shared/ui/spinner"; + +import { + getRuntimeDisplayLabel, + RuntimeIcon, +} from "../../onboarding/ui/RuntimeIcon"; +import { + buildEnvRecord, + envPairsFromRecord, + filterArgs, + idFromLabel, +} from "./harnessFormLogic"; +import { + customEntries as getCustomEntries, + sortedPresetEntries, +} from "./harnessGalleryLogic"; +import { SettingsSectionHeader } from "./SettingsSectionHeader"; + +// ── Preset card ─────────────────────────────────────────────────────────────── +// +// Preset entries come from the backend catalog (source === "preset"), so we no +// longer maintain a duplicate HARNESS_PRESETS array here. The backend +// PRESET_HARNESSES static drives availability detection and canonical data. + +function PresetCard({ entry }: { entry: AcpRuntimeCatalogEntry }) { + const isDetected = entry.availability === "available"; + + return ( +
+
+ +
+

+ {getRuntimeDisplayLabel(entry)} +

+ {entry.command ? ( +

+ {entry.command} +

+ ) : null} +
+ {isDetected ? ( + + Detected + + ) : null} +
+ + {/* Docs link for non-detected presets */} + {!isDetected && entry.installInstructionsUrl ? ( +
+ +
+ ) : null} +
+ ); +} + +// ── Custom harness form ─────────────────────────────────────────────────────── + +interface CustomFormValues { + id: string; + label: string; + command: string; + /** Each element is one argument; no space-splitting round-trip. */ + args: string[]; + /** KEY=VALUE pairs for env injection at spawn time. */ + env: Array<{ key: string; value: string }>; + installInstructionsUrl: string; + installHint: string; +} + +const EMPTY_FORM: CustomFormValues = { + id: "", + label: "", + command: "", + args: [], + env: [], + installInstructionsUrl: "", + installHint: "", +}; + +function CommandAvailabilityBadge({ command }: { command: string }) { + const trimmed = command.trim(); + const prereqs = useManagedAgentPrereqsQuery(trimmed, "", { + enabled: trimmed.length > 0, + }); + + if (!trimmed || prereqs.isLoading) return null; + + const available = prereqs.data?.acp.available; + if (available === undefined) return null; + + return ( + + {available ? "Found on PATH" : "Not found on PATH"} + + ); +} + +function ArgsEditor({ + args, + onChange, +}: { + args: string[]; + onChange: (next: string[]) => void; +}) { + function set(index: number, value: string) { + const next = [...args]; + next[index] = value; + onChange(next); + } + + function add() { + onChange([...args, ""]); + } + + function remove(index: number) { + onChange(args.filter((_, i) => i !== index)); + } + + return ( +
+ {args.map((arg, i) => ( + // biome-ignore lint/suspicious/noArrayIndexKey: positional arg list +
+ set(i, e.target.value)} + placeholder={`arg ${i + 1}`} + value={arg} + /> + +
+ ))} + +
+ ); +} + +function EnvEditor({ + env, + onChange, +}: { + env: Array<{ key: string; value: string }>; + onChange: (next: Array<{ key: string; value: string }>) => void; +}) { + function set(index: number, field: "key" | "value", value: string) { + const next = env.map((e, i) => + i === index ? { ...e, [field]: value } : e, + ); + onChange(next); + } + + function add() { + onChange([...env, { key: "", value: "" }]); + } + + function remove(index: number) { + onChange(env.filter((_, i) => i !== index)); + } + + return ( +
+ {env.map((pair, i) => ( + // biome-ignore lint/suspicious/noArrayIndexKey: positional env list +
+ set(i, "key", e.target.value)} + placeholder="KEY" + value={pair.key} + /> + set(i, "value", e.target.value)} + placeholder="value" + value={pair.value} + /> + +
+ ))} + +
+ ); +} + +function CustomHarnessForm({ + initial, + originalId, + onCancel, + onSaved, +}: { + initial?: Partial; + /** Id of the harness being edited, if this is an edit (not new). Used to + * delete the old file when the id changes. */ + originalId?: string; + onCancel: () => void; + onSaved: () => void; +}) { + const [form, setForm] = React.useState({ + ...EMPTY_FORM, + ...initial, + }); + const [error, setError] = React.useState(null); + const save = useSaveCustomHarnessMutation(); + + function field( + key: keyof Pick< + CustomFormValues, + "id" | "label" | "command" | "installInstructionsUrl" | "installHint" + >, + ) { + return (e: React.ChangeEvent) => { + const value = e.target.value; + setForm((prev) => { + const next = { ...prev, [key]: value }; + // Auto-derive id from label when id is empty or was auto-derived. + if ( + key === "label" && + (!prev.id || prev.id === idFromLabel(prev.label)) + ) { + next.id = idFromLabel(value); + } + return next; + }); + }; + } + + async function handleSubmit(e: React.FormEvent) { + e.preventDefault(); + setError(null); + try { + await save.mutateAsync({ + definition: { + id: form.id.trim(), + label: form.label.trim(), + command: form.command.trim(), + args: filterArgs(form.args), + env: buildEnvRecord(form.env), + installInstructionsUrl: form.installInstructionsUrl.trim(), + installHint: form.installHint.trim(), + }, + originalId, + }); + onSaved(); + } catch (err) { + setError(err instanceof Error ? err.message : String(err)); + } + } + + return ( +
void handleSubmit(e)} + > +
+

+ {originalId ? "Edit harness" : "Add custom harness"} +

+ +
+ +
+
+

Name

+ +
+
+

+ ID (auto-derived) +

+ +
+
+ +
+
+

Command

+ +
+ +
+ +
+

Arguments

+ setForm((p) => ({ ...p, args }))} + /> +
+ +
+

+ Env vars{" "} + + (override at spawn time; Buzz-managed vars always win) + +

+ setForm((p) => ({ ...p, env }))} + /> +
+ +
+

+ Docs URL (optional) +

+ +
+ + {error ? ( +

+ {error} +

+ ) : null} + +
+ + +
+
+ ); +} + +// ── Custom harness row ──────────────────────────────────────────────────────── + +function CustomHarnessRow({ entry }: { entry: AcpRuntimeCatalogEntry }) { + const [editing, setEditing] = React.useState(false); + const [confirmingDelete, setConfirmingDelete] = React.useState(false); + const [deleteError, setDeleteError] = React.useState(null); + const del = useDeleteCustomHarnessMutation(); + + if (editing) { + return ( + setEditing(false)} + onSaved={() => setEditing(false)} + /> + ); + } + + return ( +
+
+ +
+

{entry.label}

+

+ {entry.command ?? entry.id} + {(entry.defaultArgs ?? []).length > 0 + ? ` ${(entry.defaultArgs ?? []).join(" ")}` + : ""} +

+
+ {entry.availability === "available" ? ( + + Detected + + ) : ( + + Not installed + + )} +
+ + {confirmingDelete ? ( + <> + + + + ) : ( + + )} +
+
+ {deleteError ? ( +

+ {deleteError} +

+ ) : null} +
+ ); +} + +// ── Main component ──────────────────────────────────────────────────────────── + +export function HarnessManagementCard() { + const runtimesQuery = useAcpRuntimesQuery(); + const catalog = runtimesQuery.data ?? []; + const [showForm, setShowForm] = React.useState(false); + const [presetPrefill, setPresetPrefill] = React.useState< + Partial | undefined + >(undefined); + + // Preset entries come from the backend catalog; sort detected-first then + // alphabetically within each group — via the tested harnessGalleryLogic helper. + const presetEntries = React.useMemo( + () => sortedPresetEntries(catalog), + [catalog], + ); + + const customEntries = getCustomEntries(catalog); + + function handleFormClose() { + setShowForm(false); + setPresetPrefill(undefined); + } + + return ( +
+ + + {/* Preset gallery — driven from backend catalog, detected-first */} + {presetEntries.length > 0 ? ( +
+

Presets

+
+ {presetEntries.map((entry) => ( + + ))} +
+
+ ) : null} + + {/* Custom harnesses list */} + {customEntries.length > 0 ? ( +
+

Custom harnesses

+
+ {customEntries.map((entry) => ( + + ))} +
+
+ ) : null} + + {/* Add custom / form toggle */} + {showForm ? ( + + ) : ( + + )} + + {runtimesQuery.error instanceof Error ? ( +

+ {runtimesQuery.error.message} +

+ ) : null} +
+ ); +} diff --git a/desktop/src/features/settings/ui/SettingsPanels.tsx b/desktop/src/features/settings/ui/SettingsPanels.tsx index e2b95f249a..1940277774 100644 --- a/desktop/src/features/settings/ui/SettingsPanels.tsx +++ b/desktop/src/features/settings/ui/SettingsPanels.tsx @@ -70,6 +70,7 @@ import { } from "@/shared/theme/useThemePreviewVars"; import { ChannelTemplatesSettingsCard } from "./ChannelTemplatesSettingsCard"; import { DoctorSettingsPanel } from "./DoctorSettingsPanel"; +import { HarnessManagementCard } from "./HarnessManagementCard"; import { ExperimentalFeaturesCard } from "./ExperimentalFeaturesCard"; import { KeyboardShortcutsCard } from "./KeyboardShortcutsCard"; import { MeshComputeSettingsCard } from "@/features/mesh-compute/ui/MeshComputeSettingsCard"; @@ -815,6 +816,7 @@ export function renderSettingsSection(
+
diff --git a/desktop/src/features/settings/ui/harnessFormLogic.test.mjs b/desktop/src/features/settings/ui/harnessFormLogic.test.mjs new file mode 100644 index 0000000000..b12a8501d9 --- /dev/null +++ b/desktop/src/features/settings/ui/harnessFormLogic.test.mjs @@ -0,0 +1,172 @@ +import assert from "node:assert/strict"; +import test from "node:test"; + +import { + idFromLabel, + buildEnvRecord, + filterArgs, + envPairsFromRecord, +} from "./harnessFormLogic.ts"; + +// ── idFromLabel ────────────────────────────────────────────────────────────── + +test("idFromLabel_typicalName_producesHyphenated", () => { + assert.equal(idFromLabel("My Runtime"), "my-runtime"); +}); + +test("idFromLabel_alreadyLowercase_returnsUnchanged", () => { + assert.equal(idFromLabel("cursor"), "cursor"); +}); + +test("idFromLabel_specialChars_replacedWithHyphens", () => { + assert.equal(idFromLabel("Foo Bar!Baz"), "foo-bar-baz"); +}); + +test("idFromLabel_consecutiveSpecialChars_collapsedToOneHyphen", () => { + assert.equal(idFromLabel("foo bar"), "foo-bar"); +}); + +test("idFromLabel_trailingSpecialChars_stripped", () => { + assert.equal(idFromLabel("my-runtime--"), "my-runtime"); +}); + +test("idFromLabel_leadingHyphen_stripped", () => { + // A label starting with a non-[a-z0-9_] char produces a leading hyphen; + // the regex strips it so the id stays valid. + assert.equal(idFromLabel("-bad"), "bad"); +}); + +test("idFromLabel_underscorePreserved", () => { + assert.equal(idFromLabel("my_agent"), "my_agent"); +}); + +test("idFromLabel_emptyString_returnsEmpty", () => { + assert.equal(idFromLabel(""), ""); +}); + +test("idFromLabel_uppercaseOnly_lowercased", () => { + assert.equal(idFromLabel("CURSOR"), "cursor"); +}); + +// ── buildEnvRecord ─────────────────────────────────────────────────────────── + +test("buildEnvRecord_twoValidPairs_returnsBothEntries", () => { + assert.deepEqual( + buildEnvRecord([ + { key: "FOO", value: "bar" }, + { key: "BAZ", value: "qux" }, + ]), + { FOO: "bar", BAZ: "qux" }, + ); +}); + +test("buildEnvRecord_emptyKey_skipped", () => { + assert.deepEqual( + buildEnvRecord([ + { key: "", value: "orphaned-value" }, + { key: "KEEP", value: "yes" }, + ]), + { KEEP: "yes" }, + ); +}); + +test("buildEnvRecord_whitespaceOnlyKey_skipped", () => { + assert.deepEqual(buildEnvRecord([{ key: " ", value: "x" }]), {}); +}); + +test("buildEnvRecord_keyWithLeadingTrailingSpaces_trimmed", () => { + // Key is trimmed; value is preserved verbatim. + assert.deepEqual(buildEnvRecord([{ key: " MY_VAR ", value: " val " }]), { + MY_VAR: " val ", + }); +}); + +test("buildEnvRecord_emptyPairs_returnsEmptyObject", () => { + assert.deepEqual(buildEnvRecord([]), {}); +}); + +test("buildEnvRecord_duplicateKeys_lastValueWins", () => { + assert.deepEqual( + buildEnvRecord([ + { key: "X", value: "first" }, + { key: "X", value: "second" }, + ]), + { X: "second" }, + ); +}); + +// ── filterArgs ─────────────────────────────────────────────────────────────── + +test("filterArgs_normalArgs_allPreserved", () => { + assert.deepEqual(filterArgs(["--flag", "--output", "file.txt"]), [ + "--flag", + "--output", + "file.txt", + ]); +}); + +test("filterArgs_emptyString_removed", () => { + assert.deepEqual(filterArgs([""]), []); +}); + +test("filterArgs_whitespaceOnlyArg_removed", () => { + assert.deepEqual(filterArgs([" "]), []); +}); + +test("filterArgs_mixedEmptyAndReal_onlyRealKept", () => { + assert.deepEqual(filterArgs(["acp", "", "--verbose", " "]), [ + "acp", + "--verbose", + ]); +}); + +test("filterArgs_argWithInternalSpaces_preserved", () => { + // An arg with leading/trailing spaces but non-whitespace content is kept + // (trim only tests emptiness, not the full value). + assert.deepEqual(filterArgs(["--name", "hello world"]), [ + "--name", + "hello world", + ]); +}); + +test("filterArgs_emptyArray_returnsEmpty", () => { + assert.deepEqual(filterArgs([]), []); +}); + +// ── envPairsFromRecord ──────────────────────────────────────────────────────── + +test("envPairsFromRecord_undefinedRecord_returnsEmptyArray", () => { + assert.deepEqual(envPairsFromRecord(undefined), []); +}); + +test("envPairsFromRecord_emptyRecord_returnsEmptyArray", () => { + assert.deepEqual(envPairsFromRecord({}), []); +}); + +test("envPairsFromRecord_singleEntry_returnsSinglePair", () => { + assert.deepEqual(envPairsFromRecord({ FOO: "bar" }), [ + { key: "FOO", value: "bar" }, + ]); +}); + +test("envPairsFromRecord_multipleEntries_returnsAllPairs", () => { + const result = envPairsFromRecord({ ALPHA: "1", BETA: "2" }); + // BTreeMap serializes in sorted key order; verify both pairs are present. + assert.equal(result.length, 2); + assert.ok(result.some((p) => p.key === "ALPHA" && p.value === "1")); + assert.ok(result.some((p) => p.key === "BETA" && p.value === "2")); +}); + +test("envPairsFromRecord_valueCanBeEmptyString", () => { + assert.deepEqual(envPairsFromRecord({ EMPTY_VAL: "" }), [ + { key: "EMPTY_VAL", value: "" }, + ]); +}); + +test("envPairsFromRecord_roundTrip_buildEnvRecord_restoresOriginal", () => { + // Proves the edit round-trip: catalog env → pairs → save payload → same map. + const original = { FOO: "bar", BAZ: "qux" }; + const pairs = envPairsFromRecord(original); + const restored = buildEnvRecord(pairs); + assert.deepEqual(restored, original); +}); diff --git a/desktop/src/features/settings/ui/harnessFormLogic.ts b/desktop/src/features/settings/ui/harnessFormLogic.ts new file mode 100644 index 0000000000..a276f66509 --- /dev/null +++ b/desktop/src/features/settings/ui/harnessFormLogic.ts @@ -0,0 +1,69 @@ +/** + * Pure logic helpers for the custom-harness form. + * + * Extracted so they can be tested independently from the React layer. + */ + +/** + * Derive a valid harness ID from a human-readable display name. + * + * Rules match the backend `is_valid_harness_id` predicate: + * [a-z0-9_][a-z0-9_-]* + * + * The transform: + * 1. Lowercase the label. + * 2. Replace any character outside [a-z0-9_-] with a hyphen. + * 3. Strip a leading character that isn't [a-z0-9_] (covers e.g. "-foo"). + * 4. Collapse runs of hyphens. + * 5. Strip trailing hyphens. + */ +export function idFromLabel(label: string): string { + return label + .toLowerCase() + .replace(/[^a-z0-9_-]/g, "-") + .replace(/^[^a-z0-9_]/, "") + .replace(/-{2,}/g, "-") + .replace(/-+$/, ""); +} + +/** + * Build the env-variable record that goes into the save payload. + * + * Pairs with empty keys are silently skipped — they represent blank + * rows the user hasn't filled in yet. + */ +export function buildEnvRecord( + pairs: ReadonlyArray<{ key: string; value: string }>, +): Record { + const out: Record = {}; + for (const { key, value } of pairs) { + const k = key.trim(); + if (k) out[k] = value; + } + return out; +} + +/** + * Convert a `Record` env map (as stored on a catalog entry or + * harness definition) into the array of `{ key, value }` pairs that + * `CustomHarnessForm` and `EnvEditor` work with. + * + * Used when opening the edit form for an existing custom harness so that + * existing env vars are pre-filled instead of silently reset to empty. + */ +export function envPairsFromRecord( + record: Record | undefined, +): Array<{ key: string; value: string }> { + if (!record) return []; + return Object.entries(record).map(([key, value]) => ({ key, value })); +} + +/** + * Remove empty/whitespace-only argument rows from the args list before + * sending to the backend. Preserves legitimate blank-string args only + * when they contain non-whitespace content so quoted/complex args are + * never silently dropped. + */ +export function filterArgs(args: ReadonlyArray): string[] { + return args.filter((a) => a.trim() !== ""); +} diff --git a/desktop/src/features/settings/ui/harnessGalleryLogic.test.mjs b/desktop/src/features/settings/ui/harnessGalleryLogic.test.mjs new file mode 100644 index 0000000000..dcf240cd29 --- /dev/null +++ b/desktop/src/features/settings/ui/harnessGalleryLogic.test.mjs @@ -0,0 +1,174 @@ +import assert from "node:assert/strict"; +import { describe, it } from "node:test"; + +import { + sortedPresetEntries, + customEntries, + isEditableEntry, +} from "./harnessGalleryLogic.ts"; + +// ── Minimal catalog entry factory ──────────────────────────────────────────── + +function entry(overrides = {}) { + return { + id: overrides.id ?? "test-id", + label: overrides.label ?? "Test", + source: overrides.source ?? "custom", + availability: overrides.availability ?? "not_installed", + avatarUrl: "", + command: overrides.command ?? null, + binaryPath: null, + defaultArgs: [], + mcpCommand: null, + modelEnvVar: null, + providerEnvVar: null, + thinkingEnvVar: null, + installHint: "", + installInstructionsUrl: "", + canAutoInstall: false, + underlyingCliPath: null, + nodeRequired: false, + authStatus: { status: "not_applicable" }, + loginHint: null, + }; +} + +// ── sortedPresetEntries ─────────────────────────────────────────────────────── + +describe("sortedPresetEntries", () => { + it("returns only preset-source entries", () => { + const catalog = [ + entry({ id: "p1", source: "preset" }), + entry({ id: "c1", source: "custom" }), + entry({ id: "b1", source: "builtin" }), + ]; + const result = sortedPresetEntries(catalog); + assert.equal(result.length, 1); + assert.equal(result[0].id, "p1"); + }); + + it("places detected (available) entries before not-installed", () => { + const catalog = [ + entry({ + id: "not-there", + source: "preset", + availability: "not_installed", + label: "Alpha", + }), + entry({ + id: "detected", + source: "preset", + availability: "available", + label: "Beta", + }), + ]; + const result = sortedPresetEntries(catalog); + assert.equal(result[0].id, "detected", "detected entry must come first"); + assert.equal(result[1].id, "not-there"); + }); + + it("sorts alphabetically within detected group", () => { + const catalog = [ + entry({ + id: "z", + source: "preset", + availability: "available", + label: "Zebra", + }), + entry({ + id: "a", + source: "preset", + availability: "available", + label: "Aardvark", + }), + ]; + const result = sortedPresetEntries(catalog); + assert.equal(result[0].id, "a"); + assert.equal(result[1].id, "z"); + }); + + it("sorts alphabetically within not-installed group", () => { + const catalog = [ + entry({ + id: "z", + source: "preset", + availability: "not_installed", + label: "Zebra", + }), + entry({ + id: "a", + source: "preset", + availability: "not_installed", + label: "Aardvark", + }), + ]; + const result = sortedPresetEntries(catalog); + assert.equal(result[0].id, "a"); + assert.equal(result[1].id, "z"); + }); + + it("returns empty array when no preset entries", () => { + const catalog = [entry({ source: "custom" }), entry({ source: "builtin" })]; + assert.deepEqual(sortedPresetEntries(catalog), []); + }); + + it("does not mutate the input array", () => { + const catalog = [ + entry({ + id: "z", + source: "preset", + availability: "available", + label: "Z", + }), + entry({ + id: "a", + source: "preset", + availability: "available", + label: "A", + }), + ]; + const original = [...catalog]; + sortedPresetEntries(catalog); + assert.deepEqual( + catalog.map((e) => e.id), + original.map((e) => e.id), + "input array must not be mutated", + ); + }); +}); + +// ── customEntries ───────────────────────────────────────────────────────────── + +describe("customEntries", () => { + it("returns only custom-source entries", () => { + const catalog = [ + entry({ id: "p1", source: "preset" }), + entry({ id: "c1", source: "custom" }), + entry({ id: "c2", source: "custom" }), + ]; + const result = customEntries(catalog); + assert.equal(result.length, 2); + assert.ok(result.every((e) => e.source === "custom")); + }); + + it("returns empty when no custom entries", () => { + const catalog = [entry({ source: "preset" }), entry({ source: "builtin" })]; + assert.deepEqual(customEntries(catalog), []); + }); +}); + +// ── isEditableEntry ─────────────────────────────────────────────────────────── + +describe("isEditableEntry", () => { + it("returns true for custom entries", () => { + assert.ok(isEditableEntry(entry({ source: "custom" }))); + }); + + it("returns false for preset entries", () => { + assert.ok(!isEditableEntry(entry({ source: "preset" }))); + }); + + it("returns false for builtin entries", () => { + assert.ok(!isEditableEntry(entry({ source: "builtin" }))); + }); +}); diff --git a/desktop/src/features/settings/ui/harnessGalleryLogic.ts b/desktop/src/features/settings/ui/harnessGalleryLogic.ts new file mode 100644 index 0000000000..b8cd3318a1 --- /dev/null +++ b/desktop/src/features/settings/ui/harnessGalleryLogic.ts @@ -0,0 +1,43 @@ +/** + * Pure logic helpers for the harness gallery (HarnessManagementCard). + * + * Extracted for deterministic unit-testing — no React, no Tauri, no network. + */ + +import type { AcpRuntimeCatalogEntry } from "@/shared/api/types"; + +/** + * Filter catalog entries to preset-only, sorted detected-first then + * alphabetically within each group. + * + * "Detected" means availability === "available". This mirrors the + * React.useMemo sort inside HarnessManagementCard. + */ +export function sortedPresetEntries( + catalog: readonly AcpRuntimeCatalogEntry[], +): AcpRuntimeCatalogEntry[] { + const presets = catalog.filter((e) => e.source === "preset"); + return [...presets].sort((a, b) => { + const aDetected = a.availability === "available" ? 0 : 1; + const bDetected = b.availability === "available" ? 0 : 1; + if (aDetected !== bDetected) return aDetected - bDetected; + return a.label.localeCompare(b.label); + }); +} + +/** + * Filter catalog entries to custom-only. + */ +export function customEntries( + catalog: readonly AcpRuntimeCatalogEntry[], +): AcpRuntimeCatalogEntry[] { + return catalog.filter((e) => e.source === "custom"); +} + +/** + * Returns true iff the given catalog entry is editable by the user. + * Only `source === "custom"` entries are editable/deletable. + */ +export function isEditableEntry(entry: AcpRuntimeCatalogEntry): boolean { + return entry.source === "custom"; +} diff --git a/desktop/src/shared/api/agentModels.ts b/desktop/src/shared/api/agentModels.ts index 892d2314f0..b59a958617 100644 --- a/desktop/src/shared/api/agentModels.ts +++ b/desktop/src/shared/api/agentModels.ts @@ -7,6 +7,8 @@ export type DiscoverAgentModelsInput = { agentArgs?: string[]; provider?: string; envVars?: Record; + /** Definition-level env from the harness definition (custom/preset). Merged below user envVars. */ + definitionEnv?: Record; }; export async function discoverAgentModels(input: DiscoverAgentModelsInput) { diff --git a/desktop/src/shared/api/tauri.test.mjs b/desktop/src/shared/api/tauri.test.mjs index 68205d7298..f4692a7d2a 100644 --- a/desktop/src/shared/api/tauri.test.mjs +++ b/desktop/src/shared/api/tauri.test.mjs @@ -112,6 +112,105 @@ test("relay rate-limited: prefix check is case-sensitive (Rust always emits lowe ); }); +// ── fromRawAcpRuntimeCatalogEntry: custom row API-boundary (B-2) ───────────── +// +// These tests feed real raw custom catalog rows through fromRawAcpRuntimeCatalogEntry +// and verify the Rust→TypeScript mapping boundary: definition_env (snake_case) +// arrives as definitionEnv (camelCase), source "custom" is preserved, and the +// env round-trips end-to-end so a save-then-edit cycle cannot erase env. + +const { fromRawAcpRuntimeCatalogEntry } = await import("./tauri.ts"); + +test("fromRawAcpRuntimeCatalogEntry maps definition_env to definitionEnv", () => { + const raw = { + id: "my-harness", + label: "My Harness", + availability: "available", + command: "my-bin", + source: "custom", + definition_env: { ANTHROPIC_API_KEY: "sk-test", MODEL: "claude-3" }, + default_args: [], + can_auto_install: false, + requires_external_cli: false, + install_hint: "", + install_instructions_url: "", + }; + const entry = fromRawAcpRuntimeCatalogEntry(raw); + assert.deepStrictEqual(entry.definitionEnv, { + ANTHROPIC_API_KEY: "sk-test", + MODEL: "claude-3", + }); + assert.equal(entry.source, "custom"); +}); + +test("fromRawAcpRuntimeCatalogEntry defaults definitionEnv to {} when absent", () => { + // Rust serialization skips empty BTreeMap, so definition_env will be absent + // for harnesses with no env defined — the mapper must default to {}. + const raw = { + id: "no-env-harness", + label: "No Env", + availability: "available", + command: "no-env-bin", + source: "custom", + default_args: [], + can_auto_install: false, + requires_external_cli: false, + install_hint: "", + install_instructions_url: "", + }; + const entry = fromRawAcpRuntimeCatalogEntry(raw); + assert.deepStrictEqual( + entry.definitionEnv, + {}, + "absent definition_env must map to empty object, not undefined", + ); +}); + +test("fromRawAcpRuntimeCatalogEntry preserves source preset", () => { + const raw = { + id: "cursor", + label: "Cursor", + availability: "available", + command: "cursor", + source: "preset", + default_args: [], + can_auto_install: false, + requires_external_cli: false, + install_hint: "", + install_instructions_url: "", + }; + const entry = fromRawAcpRuntimeCatalogEntry(raw); + assert.equal(entry.source, "preset"); + assert.deepStrictEqual(entry.definitionEnv, {}); +}); + +test("fromRawAcpRuntimeCatalogEntry env round-trips through edit payload shape", () => { + // Simulate the full save → re-open cycle: raw entry comes back from Rust + // with definition_env populated; the edit form reads entry.definitionEnv. + // Verify the env values are identical before and after the mapper. + const envValues = { OPENAI_API_KEY: "sk-live-abc", REGION: "us-east-1" }; + const raw = { + id: "openai-harness", + label: "OpenAI", + availability: "not_installed", + command: "openai-agent", + source: "custom", + definition_env: envValues, + default_args: ["--acp"], + can_auto_install: false, + requires_external_cli: true, + install_hint: "Install the OpenAI CLI", + install_instructions_url: "https://platform.openai.com/docs", + }; + const entry = fromRawAcpRuntimeCatalogEntry(raw); + // The edit form reads entry.definitionEnv; it must equal the original env. + assert.deepStrictEqual( + entry.definitionEnv, + envValues, + "env must round-trip: edit form must see the same values that Rust serialized", + ); +}); + // ── Teardown ────────────────────────────────────────────────────────────────── test("teardown — restore Date.now", () => { diff --git a/desktop/src/shared/api/tauri.ts b/desktop/src/shared/api/tauri.ts index 5406cf820d..28bfe20db8 100644 --- a/desktop/src/shared/api/tauri.ts +++ b/desktop/src/shared/api/tauri.ts @@ -191,6 +191,12 @@ export type RawAcpRuntimeCatalogEntry = { /** Tagged union with snake_case status values — same shape as `AuthStatus`. */ auth_status: AuthStatus; login_hint?: string; + source: "builtin" | "preset" | "custom"; + /** + * Definition-level env vars for `source: custom` entries. + * Omitted/absent for builtin and preset — skipped in Rust serialization when empty. + */ + definition_env?: Record; }; export type RawInstallStepResult = { @@ -727,7 +733,7 @@ export function fromRawManagedAgent(agent: RawManagedAgent): ManagedAgent { }; } -function fromRawAcpRuntimeCatalogEntry( +export function fromRawAcpRuntimeCatalogEntry( entry: RawAcpRuntimeCatalogEntry, ): AcpRuntimeCatalogEntry { return { @@ -750,6 +756,10 @@ function fromRawAcpRuntimeCatalogEntry( nodeRequired: entry.node_required, authStatus: entry.auth_status, loginHint: entry.login_hint ?? null, + source: entry.source, + // Map definition_env (snake_case from Rust) to definitionEnv (camelCase). + // Absent when empty (Rust serialization skips empty BTreeMap) — default to {}. + definitionEnv: entry.definition_env ?? {}, }; } @@ -931,6 +941,45 @@ export async function discoverAcpRuntimes(): Promise { ).map(fromRawAcpRuntimeCatalogEntry); } +/** Input shape for creating or updating a custom harness. */ +export type HarnessDefinitionInput = { + id: string; + label: string; + command: string; + args?: string[]; + env?: Record; + installInstructionsUrl?: string; + installHint?: string; +}; + +/** Save (create or overwrite) a custom harness definition. Returns the catalog entry. */ +export async function saveCustomHarness( + definition: HarnessDefinitionInput, + originalId?: string, +): Promise { + const raw = await invokeTauri( + "save_custom_harness", + { + definition: { + id: definition.id, + label: definition.label, + command: definition.command, + args: definition.args ?? [], + env: definition.env ?? {}, + installInstructionsUrl: definition.installInstructionsUrl ?? "", + installHint: definition.installHint ?? "", + }, + originalId: originalId ?? null, + }, + ); + return fromRawAcpRuntimeCatalogEntry(raw); +} + +/** Delete a custom harness definition by id. No-op if already gone. */ +export async function deleteCustomHarness(id: string): Promise { + await invokeTauri("delete_custom_harness", { id }); +} + export async function installAcpRuntime( runtimeId: string, ): Promise { diff --git a/desktop/src/shared/api/types.ts b/desktop/src/shared/api/types.ts index a82f766b63..a7ec2bcd8c 100644 --- a/desktop/src/shared/api/types.ts +++ b/desktop/src/shared/api/types.ts @@ -557,6 +557,21 @@ export type AcpRuntimeCatalogEntry = { authStatus: AuthStatus; /** Hint for completing authentication; null when not applicable or already logged in. */ loginHint: string | null; + /** + * Whether this entry is compiled into the app ("builtin"), a bundled preset + * ("preset" — PATH-probed, not editable/deletable), or loaded from a user + * JSON file in `custom_harnesses/` ("custom"). Controls editability in the + * UI — only "custom" entries can be edited or deleted. + */ + source: "builtin" | "preset" | "custom"; + /** + * Definition-level environment variables for `source: custom` entries. + * + * Populated by the backend from `HarnessDefinition.env` so the edit form can + * read them back without losing existing env vars on save. Always absent/empty + * for `builtin` and `preset` entries. + */ + definitionEnv?: Record; }; /** An AcpRuntimeCatalogEntry that is confirmed available — command and binaryPath are non-null. */ diff --git a/desktop/src/shared/lib/configNudge.ts b/desktop/src/shared/lib/configNudge.ts index 7893236c28..82ed8f306a 100644 --- a/desktop/src/shared/lib/configNudge.ts +++ b/desktop/src/shared/lib/configNudge.ts @@ -50,7 +50,17 @@ export type ConfigNudgeRequirement = /** One-line stderr excerpt from the CLI's parse error. */ diagnostic: string; } - | { surface: "git_bash" }; + | { surface: "git_bash" } + | { + /** + * A custom harness command that cannot be resolved in the current PATH. + * No in-app action can fix this — the user must install the binary or + * update their PATH. + */ + surface: "missing_binary"; + /** The command name that was not found (e.g. "my-acp-agent"). */ + command: string; + }; /** * The structured payload embedded in the `buzz:config-nudge` sentinel block. @@ -155,6 +165,8 @@ function isConfigNudgeRequirement(v: unknown): v is ConfigNudgeRequirement { typeof r.setup_copy === "string" && typeof r.diagnostic === "string" ); + case "missing_binary": + return typeof r.command === "string"; default: return false; } diff --git a/desktop/src/shared/ui/config-nudge-attachment.tsx b/desktop/src/shared/ui/config-nudge-attachment.tsx index e1294b7dde..3f7b75ada3 100644 --- a/desktop/src/shared/ui/config-nudge-attachment.tsx +++ b/desktop/src/shared/ui/config-nudge-attachment.tsx @@ -37,6 +37,8 @@ function requirementKey( return `cli_config_invalid:${req.probe_args.join(",")}:${index}`; case "git_bash": return `git_bash:${index}`; + case "missing_binary": + return `missing_binary:${req.command}:${index}`; } } @@ -381,6 +383,20 @@ function RequirementRow({ ); + case "missing_binary": { + // Missing-binary rows are purely informational — the user must install the + // binary or update their PATH. No in-app action can fix this. + return ( +
+ + + {requirement.command} + {" "} + not found in PATH — install it or check your PATH settings + +
+ ); + } case "cli_config_invalid": { // Config-invalid rows are purely informational — the user must edit an // external file. No Agent runtimes CTA (Buzz can't repair ~/.codex/config.toml) diff --git a/desktop/src/testing/e2eBridge.ts b/desktop/src/testing/e2eBridge.ts index da398ca4ba..23edc74a4b 100644 --- a/desktop/src/testing/e2eBridge.ts +++ b/desktop/src/testing/e2eBridge.ts @@ -3,6 +3,11 @@ import { mockIPC, mockWindows } from "@tauri-apps/api/mocks"; import { decode } from "nostr-tools/nip19"; import { finalizeEvent, getPublicKey } from "nostr-tools/pure"; import { parse as yamlParse } from "yaml"; +import { + mergeMockCustomHarnesses, + handleSaveCustomHarness, + handleDeleteCustomHarness, +} from "./e2eBridgeCustomHarnesses.ts"; import { relayClient } from "@/shared/api/relayClient"; import type { ConnectionState } from "@/shared/api/relayClientShared"; @@ -175,6 +180,8 @@ type E2eConfig = { acpAuthMethods?: Record; acpAuthMethodsErrors?: Record; acpAuthMethodsError?: string; + /** When set, the `delete_custom_harness` mock command throws with this message. */ + deleteCustomHarnessError?: string; connectAcpRuntimeResult?: RawConnectAcpRuntimeResult; connectAcpRuntimeDelayMs?: number; connectAcpRuntimeError?: string; @@ -6957,7 +6964,9 @@ async function handleDiscoverAcpRuntimes( } const configured = config?.mock?.acpRuntimesCatalog; if (configured) { - return configured.map(withMockRuntimeConfigMetadata); + return mergeMockCustomHarnesses( + configured.map(withMockRuntimeConfigMetadata), + ); } const defaultCatalog: RawAcpRuntimeCatalogEntry[] = [ { @@ -6976,6 +6985,7 @@ async function handleDiscoverAcpRuntimes( underlying_cli_path: null, node_required: false, auth_status: { status: "not_applicable" }, + source: "builtin", login_hint: undefined, }, { @@ -6995,6 +7005,7 @@ async function handleDiscoverAcpRuntimes( underlying_cli_path: "/usr/local/bin/claude", node_required: false, auth_status: { status: "unknown" }, + source: "builtin", login_hint: undefined, }, { @@ -7014,6 +7025,7 @@ async function handleDiscoverAcpRuntimes( underlying_cli_path: null, node_required: false, auth_status: { status: "unknown" }, + source: "builtin", login_hint: undefined, }, { @@ -7032,10 +7044,13 @@ async function handleDiscoverAcpRuntimes( underlying_cli_path: null, node_required: false, auth_status: { status: "not_applicable" }, + source: "builtin", login_hint: undefined, }, ]; - return defaultCatalog.map(withMockRuntimeConfigMetadata); + return mergeMockCustomHarnesses( + defaultCatalog.map(withMockRuntimeConfigMetadata), + ); } async function handleDiscoverAcpAuthMethods( @@ -10004,6 +10019,15 @@ export function maybeInstallE2eTauriMocks() { return activeConfig?.mock?.relayRequiresMembership ?? false; case "discover_acp_providers": return handleDiscoverAcpRuntimes(activeConfig); + case "save_custom_harness": + return handleSaveCustomHarness( + payload as Parameters[0], + ); + case "delete_custom_harness": + return handleDeleteCustomHarness( + payload as Parameters[0], + activeConfig, + ); case "discover_acp_auth_methods": return handleDiscoverAcpAuthMethods( payload as { runtimeId?: string }, diff --git a/desktop/src/testing/e2eBridgeCustomHarnesses.test.mjs b/desktop/src/testing/e2eBridgeCustomHarnesses.test.mjs new file mode 100644 index 0000000000..0a0fb36a92 --- /dev/null +++ b/desktop/src/testing/e2eBridgeCustomHarnesses.test.mjs @@ -0,0 +1,246 @@ +/** + * Unit tests for the e2eBridge custom harness handlers (C-10). + * + * Tests are imported directly from the extracted e2eBridgeCustomHarnesses.ts + * module — no Tauri mock or browser environment needed — to prove: + * + * 1. save returns a catalog entry with source "custom" and correct fields + * 2. definition_env is preserved through save (non-empty env) + * 3. empty env produces absent definition_env (mirrors Rust BTreeMap serialization) + * 4. same-ID edit replaces the existing entry in the store (no duplicates) + * 5. rename (originalId ≠ id) removes the old key and inserts the new one + * 6. delete removes the entry from the store + * 7. delete is idempotent (not-found does not throw) + * 8. discover integration: saved harness appears in the discover result set + * alongside the default catalog (verifies the Map is shared by reference) + */ +import assert from "node:assert/strict"; +import { beforeEach, describe, it } from "node:test"; + +import { + mockCustomHarnesses, + mergeMockCustomHarnesses, + resetMockCustomHarnesses, + handleSaveCustomHarness, + handleDeleteCustomHarness, +} from "./e2eBridgeCustomHarnesses.ts"; + +function makeArgs(overrides = {}) { + return { + definition: { + id: overrides.id ?? "test-harness", + label: overrides.label ?? "Test Harness", + command: overrides.command ?? "test-bin", + args: overrides.args ?? [], + env: overrides.env ?? {}, + installInstructionsUrl: overrides.installInstructionsUrl ?? "", + installHint: overrides.installHint ?? "", + }, + originalId: overrides.originalId ?? null, + }; +} + +// Reset the store before every test so tests are independent. +beforeEach(() => resetMockCustomHarnesses()); + +// ── save_custom_harness ─────────────────────────────────────────────────────── + +describe("handleSaveCustomHarness", () => { + it("returns a catalog entry with source 'custom'", () => { + const entry = handleSaveCustomHarness( + makeArgs({ id: "my-rt", label: "My RT" }), + ); + assert.equal(entry.id, "my-rt"); + assert.equal(entry.label, "My RT"); + assert.equal(entry.source, "custom"); + }); + + it("stores the entry in mockCustomHarnesses", () => { + handleSaveCustomHarness(makeArgs({ id: "stored" })); + assert.ok( + mockCustomHarnesses.has("stored"), + "store must contain the saved id", + ); + }); + + it("preserves non-empty definition_env", () => { + const env = { ANTHROPIC_API_KEY: "sk-test", MODEL: "claude-3" }; + const entry = handleSaveCustomHarness(makeArgs({ id: "env-rt", env })); + assert.deepStrictEqual(entry.definition_env, env); + assert.deepStrictEqual( + mockCustomHarnesses.get("env-rt")?.definition_env, + env, + ); + }); + + it("produces absent definition_env for empty env (mirrors Rust BTreeMap skip)", () => { + const entry = handleSaveCustomHarness(makeArgs({ id: "no-env", env: {} })); + assert.ok( + entry.definition_env === undefined || entry.definition_env === null, + "empty env must yield absent definition_env", + ); + }); + + it("same-ID edit replaces entry — no duplicates in the store", () => { + handleSaveCustomHarness(makeArgs({ id: "dup", label: "V1" })); + handleSaveCustomHarness( + makeArgs({ id: "dup", label: "V2", originalId: "dup" }), + ); + assert.equal( + mockCustomHarnesses.size, + 1, + "same-ID edit must not duplicate store entries", + ); + assert.equal(mockCustomHarnesses.get("dup")?.label, "V2"); + }); + + it("rename removes old key and inserts new key", () => { + handleSaveCustomHarness(makeArgs({ id: "old-rt", label: "Old" })); + handleSaveCustomHarness( + makeArgs({ id: "new-rt", label: "New", originalId: "old-rt" }), + ); + assert.ok( + !mockCustomHarnesses.has("old-rt"), + "old key must be removed on rename", + ); + assert.ok( + mockCustomHarnesses.has("new-rt"), + "new key must be present after rename", + ); + assert.equal(mockCustomHarnesses.get("new-rt")?.label, "New"); + }); +}); + +// ── delete_custom_harness ──────────────────────────────────────────────────── + +describe("handleDeleteCustomHarness", () => { + it("removes an existing entry from the store", () => { + handleSaveCustomHarness(makeArgs({ id: "to-delete" })); + assert.ok(mockCustomHarnesses.has("to-delete")); + + handleDeleteCustomHarness({ id: "to-delete" }); + assert.ok( + !mockCustomHarnesses.has("to-delete"), + "entry must be removed after delete", + ); + }); + + it("is idempotent — deleting non-existent id does not throw", () => { + assert.doesNotThrow( + () => handleDeleteCustomHarness({ id: "never-existed" }), + "delete of non-existent id must not throw", + ); + }); + + it("throws when deleteCustomHarnessError knob is set", () => { + handleSaveCustomHarness(makeArgs({ id: "keep-alive" })); + const config = { mock: { deleteCustomHarnessError: "permission denied" } }; + assert.throws( + () => handleDeleteCustomHarness({ id: "keep-alive" }, config), + /permission denied/, + "must throw the injected error message", + ); + // Entry must remain in store — the error means delete did not complete. + assert.ok( + mockCustomHarnesses.has("keep-alive"), + "entry must remain when delete throws", + ); + }); +}); + +// ── discover integration: store is shared by reference ─────────────────────── + +describe("mergeMockCustomHarnesses", () => { + const seeded = (id, label = id) => ({ id, label, source: "custom" }); + + it("appends a newly saved harness that is not in the seeded catalog", () => { + handleSaveCustomHarness(makeArgs({ id: "added", label: "Added" })); + const merged = mergeMockCustomHarnesses([seeded("preset-a")]); + assert.deepEqual( + merged.map((e) => e.id), + ["preset-a", "added"], + ); + }); + + it("replaces a seeded entry in place on same-id save — no duplicate row", () => { + handleSaveCustomHarness(makeArgs({ id: "seeded-one", label: "V2" })); + const merged = mergeMockCustomHarnesses([ + seeded("preset-a"), + seeded("seeded-one", "V1"), + ]); + assert.deepEqual( + merged.map((e) => e.id), + ["preset-a", "seeded-one"], + "must not duplicate the id", + ); + assert.equal( + merged.find((e) => e.id === "seeded-one").label, + "V2", + "saved entry must win over the seed", + ); + }); + + it("drops a deleted seeded entry (tombstone, not just store removal)", () => { + // The regression: a seeded row has no store entry to delete, so without a + // tombstone the row survived the delete and the spec failed. + handleDeleteCustomHarness({ id: "seeded-one" }); + const merged = mergeMockCustomHarnesses([ + seeded("preset-a"), + seeded("seeded-one"), + ]); + assert.deepEqual( + merged.map((e) => e.id), + ["preset-a"], + ); + }); + + it("drops the vacated old id after a rename and surfaces the new one", () => { + handleSaveCustomHarness( + makeArgs({ id: "new-id", label: "New", originalId: "old-id" }), + ); + const merged = mergeMockCustomHarnesses([seeded("old-id", "Old")]); + assert.deepEqual( + merged.map((e) => e.id), + ["new-id"], + ); + }); + + it("re-saving a deleted id resurrects it", () => { + handleDeleteCustomHarness({ id: "seeded-one" }); + handleSaveCustomHarness(makeArgs({ id: "seeded-one", label: "Back" })); + const merged = mergeMockCustomHarnesses([seeded("seeded-one", "Original")]); + assert.deepEqual( + merged.map((e) => e.id), + ["seeded-one"], + ); + assert.equal(merged[0].label, "Back"); + }); + + it("leaves a seeded catalog untouched when nothing has been mutated", () => { + const base = [seeded("preset-a"), seeded("preset-b")]; + assert.deepEqual( + mergeMockCustomHarnesses(base).map((e) => e.id), + ["preset-a", "preset-b"], + ); + }); +}); + +describe("mockCustomHarnesses Map reference", () => { + it("handler writes are immediately visible to callers that read the exported Map", () => { + // handleDiscoverAcpRuntimes merges this store into the catalog it returns. + // The exported Map is the same object by reference, so writes via the + // handler are visible to any reader of the Map. + assert.equal( + mockCustomHarnesses.size, + 0, + "store must be empty after reset", + ); + + handleSaveCustomHarness(makeArgs({ id: "visible", label: "Visible" })); + assert.equal(mockCustomHarnesses.size, 1); + + const [entry] = Array.from(mockCustomHarnesses.values()); + assert.equal(entry.id, "visible"); + assert.equal(entry.source, "custom"); + }); +}); diff --git a/desktop/src/testing/e2eBridgeCustomHarnesses.ts b/desktop/src/testing/e2eBridgeCustomHarnesses.ts new file mode 100644 index 0000000000..b72ecadd34 --- /dev/null +++ b/desktop/src/testing/e2eBridgeCustomHarnesses.ts @@ -0,0 +1,127 @@ +/** + * In-memory custom harness store for the e2e bridge. + * + * Extracted as a separate module so the handler logic can be unit-tested + * independently of the full e2eBridge.ts context (which requires a browser + * environment and full Playwright setup). + */ +import type { RawAcpRuntimeCatalogEntry } from "../shared/api/tauri.ts"; + +/** In-memory store for custom harnesses saved via `save_custom_harness`. */ +export const mockCustomHarnesses = new Map(); + +/** + * Ids removed via `delete_custom_harness` (or vacated by a rename). + * + * Needed because a test's `acpRuntimesCatalog` seed is static config, not the + * mutation store: deleting a seeded row leaves nothing to remove from + * `mockCustomHarnesses`, so without a tombstone the row would survive the + * delete and the mock would report success while the UI still shows it. + */ +export const mockDeletedCustomHarnesses = new Set(); + +/** Reset the store between tests. */ +export function resetMockCustomHarnesses(): void { + mockCustomHarnesses.clear(); + mockDeletedCustomHarnesses.clear(); +} + +/** + * Overlay the mutation store onto a seeded catalog. + * + * Deleted ids drop out, saved ids replace their seeded entry in place (so a + * same-id edit updates rather than duplicates), and newly added ids append. + */ +export function mergeMockCustomHarnesses( + base: RawAcpRuntimeCatalogEntry[], +): RawAcpRuntimeCatalogEntry[] { + const merged = base.filter( + (entry) => !mockDeletedCustomHarnesses.has(entry.id), + ); + for (const entry of mockCustomHarnesses.values()) { + const index = merged.findIndex((existing) => existing.id === entry.id); + if (index === -1) { + merged.push(entry); + } else { + merged[index] = entry; + } + } + return merged; +} + +/** + * Handle `save_custom_harness`. + * + * Persists the definition into `mockCustomHarnesses` so that the next + * `discover_acp_providers` call includes it. Mirrors the Rust command's + * return shape: an `AcpRuntimeCatalogEntry` for the saved harness. + */ +export function handleSaveCustomHarness(args: { + definition?: { + id?: string; + label?: string; + command?: string; + args?: string[]; + env?: Record; + installInstructionsUrl?: string; + installHint?: string; + }; + originalId?: string | null; +}): RawAcpRuntimeCatalogEntry { + const def = args.definition ?? {}; + const id = def.id ?? ""; + const originalId = args.originalId ?? null; + + // On rename: remove the old entry so the old id is no longer in the catalog. + if (originalId && originalId !== id) { + mockCustomHarnesses.delete(originalId); + mockDeletedCustomHarnesses.add(originalId); + } + // A save resurrects an id that an earlier test step deleted. + mockDeletedCustomHarnesses.delete(id); + + const entry: RawAcpRuntimeCatalogEntry = { + id, + label: def.label ?? id, + avatar_url: "", + availability: "not_installed", // PATH not probed in e2e mock + command: def.command ?? null, + binary_path: null, + default_args: def.args ?? [], + mcp_command: null, + install_hint: def.installHint ?? "", + install_instructions_url: def.installInstructionsUrl ?? "", + can_auto_install: false, + requires_external_cli: true, + underlying_cli_path: null, + node_required: false, + auth_status: { status: "not_applicable" }, + source: "custom", + // Omit definition_env when the env map is empty — mirrors Rust's BTreeMap + // serialization which skips empty maps so the field is absent on the wire. + definition_env: + def.env && Object.keys(def.env).length > 0 ? def.env : undefined, + login_hint: undefined, + }; + mockCustomHarnesses.set(id, entry); + return entry; +} + +/** + * Handle `delete_custom_harness`. + * Removes the harness from the in-memory store. Idempotent (not-found is OK). + * When `config?.mock?.deleteCustomHarnessError` is set, throws with that message + * to exercise the UI's inline error path. + */ +export function handleDeleteCustomHarness( + args: { id?: string }, + config?: { mock?: { deleteCustomHarnessError?: string } } | undefined, +): void { + const errorMsg = config?.mock?.deleteCustomHarnessError; + if (errorMsg) { + throw new Error(errorMsg); + } + const id = args.id ?? ""; + mockCustomHarnesses.delete(id); + mockDeletedCustomHarnesses.add(id); +} diff --git a/desktop/test-loader-hooks.mjs b/desktop/test-loader-hooks.mjs index c71cc18599..dd6f343274 100644 --- a/desktop/test-loader-hooks.mjs +++ b/desktop/test-loader-hooks.mjs @@ -54,7 +54,20 @@ const stubModules = new Map([ const STUB_URL_PREFIX = "buzz-test-stub:"; +// Vite resolves asset imports (`./logo.png`, `./logo.png?inline`) to a URL or +// base64 string at bundle time; node's ESM resolver has no such loader and +// throws on the query suffix. Serve an inert string so components that embed +// assets stay unit-testable. +const ASSET_SPECIFIER = /\.(?:png|jpe?g|gif|svg|webp|avif|ico)(?:\?[^/]*)?$/; +const ASSET_URL_PREFIX = "buzz-test-asset:"; + export function resolve(specifier, context, nextResolve) { + if (ASSET_SPECIFIER.test(specifier)) { + return { + shortCircuit: true, + url: `${ASSET_URL_PREFIX}${specifier}`, + }; + } if (stubModules.has(specifier)) { return { shortCircuit: true, @@ -98,6 +111,14 @@ export function resolve(specifier, context, nextResolve) { } export async function load(url, context, nextLoad) { + if (url.startsWith(ASSET_URL_PREFIX)) { + return { + format: "module", + shortCircuit: true, + source: 'export default "test-asset";\n', + }; + } + if (url.startsWith(STUB_URL_PREFIX)) { return { format: "module", diff --git a/desktop/tests/e2e/harness-management.spec.ts b/desktop/tests/e2e/harness-management.spec.ts new file mode 100644 index 0000000000..cf0064da44 --- /dev/null +++ b/desktop/tests/e2e/harness-management.spec.ts @@ -0,0 +1,441 @@ +/** + * E2E spec for the Bring-Your-Own-Harness management UI. + * + * Covers: + * - Preset gallery renders with Detected badge for an available preset + * - Preset gallery renders without badge / with install link for a missing preset + * - Add custom harness (form → save → row appears in list) + * - Edit preserves env vars (round-trip through definitionEnv boundary) + * - Same-ID edit replaces entry (no duplicate row) + * - Rename removes old row and shows new row + * - Delete success removes row + * - Delete failure shows error inline (error-injection knob) + * - PATH badge: custom harness row shows Detected when availability === "available" + * - Onboarding navigate: setup-page "More harnesses" click → Settings → Agents (F8) + */ +import { expect, test } from "@playwright/test"; + +import { installMockBridge } from "../helpers/bridge"; + +// ── Shared catalog fixtures ─────────────────────────────────────────────────── + +/** Hermes preset with availability "available" — renders the Detected badge. */ +const HERMES_AVAILABLE = { + id: "hermes", + label: "Hermes", + avatar_url: "", + availability: "available", + command: "hermes-acp", + binary_path: "/usr/local/bin/hermes-acp", + default_args: [], + mcp_command: null, + install_hint: "Install Hermes Agent from hermes-agent.nousresearch.com.", + install_instructions_url: "https://hermes-agent.nousresearch.com", + can_auto_install: false, + requires_external_cli: true, + underlying_cli_path: null, + node_required: false, + auth_status: { status: "unknown" }, + source: "preset", +} as const; + +/** OpenClaw preset with availability "not_installed" — renders install link. */ +const OPENCLAW_NOT_INSTALLED = { + id: "openclaw", + label: "OpenClaw", + avatar_url: "", + availability: "not_installed", + command: "openclaw", + binary_path: null, + default_args: ["acp"], + mcp_command: null, + install_hint: "Install OpenClaw: npm install -g openclaw@latest.", + install_instructions_url: "https://docs.openclaw.ai/start/getting-started", + can_auto_install: false, + requires_external_cli: true, + underlying_cli_path: null, + node_required: false, + auth_status: { status: "unknown" }, + source: "preset", +} as const; + +/** Custom harness entry already persisted — shown in the custom list. */ +function makeCustomEntry( + overrides: { + id?: string; + label?: string; + command?: string; + availability?: "available" | "not_installed"; + definition_env?: Record; + } = {}, +) { + return { + id: overrides.id ?? "my-custom-agent", + label: overrides.label ?? "My Custom Agent", + avatar_url: "", + availability: overrides.availability ?? "not_installed", + command: overrides.command ?? "my-custom-acp", + binary_path: + overrides.availability === "available" + ? "/usr/local/bin/my-custom-acp" + : null, + default_args: [], + mcp_command: null, + install_hint: "", + install_instructions_url: "", + can_auto_install: false, + requires_external_cli: true, + underlying_cli_path: null, + node_required: false, + auth_status: { status: "unknown" }, + source: "custom", + definition_env: overrides.definition_env, + }; +} + +// ── Navigation helpers ──────────────────────────────────────────────────────── + +/** + * Open Settings → Agents through the normal UI path. + * CI serves the app as a static SPA; direct navigation to /settings 404s + * before the client router starts. + */ +async function openHarnessSettings(page: import("@playwright/test").Page) { + await page.goto("/", { waitUntil: "domcontentloaded" }); + await page.getByTestId("open-settings").click(); + await page.getByTestId("profile-popover-settings").click(); + await expect(page.getByTestId("settings-view")).toBeVisible(); + await page.getByTestId("settings-nav-agents").click(); + await expect(page.getByTestId("settings-harness-management")).toBeVisible({ + timeout: 10_000, + }); +} + +/** + * Fill and submit the custom harness add/edit form. + * Caller must have the form visible before calling. + */ +async function fillHarnessForm( + page: import("@playwright/test").Page, + values: { + label: string; + id: string; + command: string; + env?: Array<{ key: string; value: string }>; + }, +) { + await page.fill("#ch-label", values.label); + // ID may auto-derive; overwrite it. + await page.fill("#ch-id", values.id); + await page.fill("#ch-command", values.command); + for (const pair of values.env ?? []) { + await page.getByRole("button", { name: "Add env var" }).click(); + // Fill last appended row. + const keyInputs = page.locator('input[placeholder="KEY"]'); + const valInputs = page.locator('input[placeholder="value"]'); + await keyInputs.last().fill(pair.key); + await valInputs.last().fill(pair.value); + } +} + +// ── Preset gallery ──────────────────────────────────────────────────────────── + +test.describe("preset gallery", () => { + test("detected preset shows Detected badge", async ({ page }) => { + await installMockBridge(page, { + acpRuntimesCatalog: [HERMES_AVAILABLE, OPENCLAW_NOT_INSTALLED], + }); + await openHarnessSettings(page); + + const hermesCard = page.getByTestId("harness-preset-hermes"); + await expect(hermesCard).toBeVisible(); + await expect(hermesCard.getByText("Detected")).toBeVisible(); + // Not-installed preset must NOT show Detected badge. + const openclawCard = page.getByTestId("harness-preset-openclaw"); + await expect(openclawCard).toBeVisible(); + await expect(openclawCard.getByText("Detected")).not.toBeVisible(); + }); + + test("not-installed preset shows Install link, not Detected badge", async ({ + page, + }) => { + await installMockBridge(page, { + acpRuntimesCatalog: [HERMES_AVAILABLE, OPENCLAW_NOT_INSTALLED], + }); + await openHarnessSettings(page); + + const openclawCard = page.getByTestId("harness-preset-openclaw"); + await expect(openclawCard).toBeVisible(); + await expect(openclawCard.getByText("Install")).toBeVisible(); + await expect(openclawCard.getByText("Detected")).not.toBeVisible(); + }); +}); + +// ── Custom harness add ──────────────────────────────────────────────────────── + +test.describe("add custom harness", () => { + test("form saves and row appears in list", async ({ page }) => { + await installMockBridge(page, { + acpRuntimesCatalog: [HERMES_AVAILABLE, OPENCLAW_NOT_INSTALLED], + }); + await openHarnessSettings(page); + + // No custom rows yet. + // The list container may exist but have no row children. + await expect( + page.getByTestId("custom-harness-row-my-custom-agent"), + ).not.toBeVisible(); + + // Open the add form. + await page.getByTestId("harness-add-custom-button").click(); + await expect(page.getByTestId("custom-harness-form")).toBeVisible(); + + await fillHarnessForm(page, { + label: "My Custom Agent", + id: "my-custom-agent", + command: "my-custom-acp", + }); + + // Submit. + await page + .getByTestId("custom-harness-form") + .getByRole("button", { name: "Save", exact: true }) + .click(); + + // Row must appear in the list after save. + await expect( + page.getByTestId("custom-harness-row-my-custom-agent"), + ).toBeVisible({ timeout: 5_000 }); + }); + + test("edit preserves env vars (definitionEnv round-trip)", async ({ + page, + }) => { + await installMockBridge(page, { + acpRuntimesCatalog: [ + HERMES_AVAILABLE, + OPENCLAW_NOT_INSTALLED, + makeCustomEntry({ definition_env: { MY_API_KEY: "sk-test" } }), + ], + }); + await openHarnessSettings(page); + + // Open edit form for the existing custom entry. + await expect( + page.getByTestId("custom-harness-row-my-custom-agent"), + ).toBeVisible(); + await page.getByTestId("custom-harness-edit-my-custom-agent").click(); + await expect(page.getByTestId("custom-harness-form")).toBeVisible(); + + // Env KEY and value must be pre-populated. + await expect(page.locator('input[placeholder="KEY"]').first()).toHaveValue( + "MY_API_KEY", + ); + await expect( + page.locator('input[placeholder="value"]').first(), + ).toHaveValue("sk-test"); + }); + + test("same-ID edit replaces row — no duplicate", async ({ page }) => { + await installMockBridge(page, { + acpRuntimesCatalog: [ + HERMES_AVAILABLE, + OPENCLAW_NOT_INSTALLED, + makeCustomEntry({ label: "V1 Label" }), + ], + }); + await openHarnessSettings(page); + + // Edit, keep same ID, change label. + await page.getByTestId("custom-harness-edit-my-custom-agent").click(); + await expect(page.getByTestId("custom-harness-form")).toBeVisible(); + await page.fill("#ch-label", "V2 Label"); + await page + .getByTestId("custom-harness-form") + .getByRole("button", { name: "Save", exact: true }) + .click(); + + // Exactly one row with the same ID; label updated. + const rows = page.locator( + '[data-testid^="custom-harness-row-my-custom-agent"]', + ); + await expect(rows).toHaveCount(1); + await expect(rows.first()).toContainText("V2 Label"); + }); + + test("rename removes old row and inserts new row", async ({ page }) => { + await installMockBridge(page, { + acpRuntimesCatalog: [ + HERMES_AVAILABLE, + OPENCLAW_NOT_INSTALLED, + makeCustomEntry({ id: "old-harness", label: "Old" }), + ], + }); + await openHarnessSettings(page); + + await page.getByTestId("custom-harness-edit-old-harness").click(); + await expect(page.getByTestId("custom-harness-form")).toBeVisible(); + await page.fill("#ch-label", "New Harness"); + await page.fill("#ch-id", "new-harness"); + await page + .getByTestId("custom-harness-form") + .getByRole("button", { name: "Save", exact: true }) + .click(); + + // Old row gone; new row present. + await expect( + page.getByTestId("custom-harness-row-old-harness"), + ).not.toBeVisible({ timeout: 5_000 }); + await expect( + page.getByTestId("custom-harness-row-new-harness"), + ).toBeVisible({ timeout: 5_000 }); + }); +}); + +// ── Delete flow ─────────────────────────────────────────────────────────────── + +test.describe("delete custom harness", () => { + test("delete success removes the row", async ({ page }) => { + await installMockBridge(page, { + acpRuntimesCatalog: [ + HERMES_AVAILABLE, + OPENCLAW_NOT_INSTALLED, + makeCustomEntry(), + ], + }); + await openHarnessSettings(page); + + // Enter confirm-delete mode. + await page.getByTestId("custom-harness-delete-my-custom-agent").click(); + // The confirm button must appear. + await expect( + page.getByTestId("custom-harness-delete-confirm-my-custom-agent"), + ).toBeVisible(); + await page + .getByTestId("custom-harness-delete-confirm-my-custom-agent") + .click(); + + // Row disappears after successful delete. + await expect( + page.getByTestId("custom-harness-row-my-custom-agent"), + ).not.toBeVisible({ timeout: 5_000 }); + }); + + test("delete failure shows inline error and keeps the row", async ({ + page, + }) => { + await installMockBridge(page, { + acpRuntimesCatalog: [ + HERMES_AVAILABLE, + OPENCLAW_NOT_INSTALLED, + makeCustomEntry(), + ], + deleteCustomHarnessError: "permission denied: could not remove file", + }); + await openHarnessSettings(page); + + await page.getByTestId("custom-harness-delete-my-custom-agent").click(); + await page + .getByTestId("custom-harness-delete-confirm-my-custom-agent") + .click(); + + // Error text visible; row still present. + await expect( + page.getByText("permission denied: could not remove file"), + ).toBeVisible({ timeout: 5_000 }); + await expect( + page.getByTestId("custom-harness-row-my-custom-agent"), + ).toBeVisible(); + }); +}); + +// ── PATH badge on custom harness row ───────────────────────────────────────── + +test("custom harness row shows Detected badge when command is on PATH", async ({ + page, +}) => { + await installMockBridge(page, { + acpRuntimesCatalog: [ + HERMES_AVAILABLE, + OPENCLAW_NOT_INSTALLED, + makeCustomEntry({ availability: "available" }), + ], + }); + await openHarnessSettings(page); + + const row = page.getByTestId("custom-harness-row-my-custom-agent"); + await expect(row).toBeVisible(); + await expect(row.getByText("Detected")).toBeVisible(); +}); + +// ── F8: onboarding navigate-after-complete ──────────────────────────────────── +// +// Verifies the parent-owned route intent introduced in B-8: +// 1. User reaches the machine-onboarding setup page. +// 2. Clicks "More harnesses" (onboarding-setup-more-harnesses). +// 3. App completes onboarding and immediately navigates to Settings → Agents. +// +// This test exercises the real App.tsx effect that gates router.navigate() on +// machine.stage === "ready", which the pure-logic tests in +// postOnboardingNav.test.mjs cannot cover (they simulate the predicate, not +// the real render path). + +test("onboarding setup More-harnesses click navigates to Settings → Agents", async ({ + page, +}) => { + // Start with a fresh machine (no machine-onboarding-complete flag). + // skipCommunitySeed: true so the user goes through machine onboarding. + // skipOnboardingSeed: true so the community/identity banner doesn't appear. + await installMockBridge(page, undefined, { + skipCommunitySeed: true, + skipOnboardingSeed: true, + }); + // Seed a community stamped with a *foreign* pubkey. This is the only shape + // that satisfies both preconditions of this test at once: + // - machine onboarding must still run, so the community must NOT vouch for + // the active identity (migrateMachineOnboardingCompletion only accepts a + // community whose recorded pubkey matches — see machineOnboarding.ts:70). + // - after onboarding completes, useCommunityInit must NOT report + // needsSetup, or App.tsx:499 renders WelcomeSetup instead of the router + // and the navigation lands on a screen that has no settings tree. + // The default seed vouches (it uses the active pubkey) and skipping it + // entirely leaves zero communities, so neither default gets there. + await page.addInitScript(() => { + const communityId = "e2e-default-community"; + window.localStorage.setItem( + "buzz-communities", + JSON.stringify([ + { + id: communityId, + name: "E2E Test", + relayUrl: "ws://127.0.0.1:7777", + pubkey: "f".repeat(64), + addedAt: new Date().toISOString(), + }, + ]), + ); + window.localStorage.setItem("buzz-active-community-id", communityId); + }); + await page.goto("/"); + + // Reach the setup page: create a new identity key → skip backup step. + await page.getByRole("button", { name: "Create a new identity key" }).click(); + await expect(page.getByTestId("onboarding-page-backup")).toBeVisible({ + timeout: 10_000, + }); + await page.getByTestId("onboarding-next").click(); + + // Now on the setup page. + await expect( + page.getByRole("heading", { name: "Set up your agent harnesses" }), + ).toBeVisible({ timeout: 10_000 }); + + // Click the "More harnesses" link — fires navigateToAgentSettings. + await page.getByTestId("onboarding-setup-more-harnesses").click(); + + // After onboarding completes + router mounts, the app must land on + // Settings → Agents (harness management section visible). + await expect(page.getByTestId("settings-harness-management")).toBeVisible({ + timeout: 15_000, + }); +}); diff --git a/desktop/tests/helpers/bridge.ts b/desktop/tests/helpers/bridge.ts index 41fdeae6fe..e9a0675141 100644 --- a/desktop/tests/helpers/bridge.ts +++ b/desktop/tests/helpers/bridge.ts @@ -154,6 +154,8 @@ type MockBridgeOptions = { acpRuntimesDelayMs?: number; acpAuthMethods?: Record[] }>; acpAuthMethodsError?: string; + /** When set, the `delete_custom_harness` mock command throws with this message. */ + deleteCustomHarnessError?: string; connectAcpRuntimeResult?: { launched: boolean }; connectAcpRuntimeDelayMs?: number; connectAcpRuntimeError?: string;