diff --git a/crates/buzz-acp/README.md b/crates/buzz-acp/README.md index d863729d54..e6164b02dd 100644 --- a/crates/buzz-acp/README.md +++ b/crates/buzz-acp/README.md @@ -261,6 +261,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 9649e9bbfb..61dc61da00 100644 --- a/desktop/scripts/check-file-sizes.mjs +++ b/desktop/scripts/check-file-sizes.mjs @@ -80,15 +80,16 @@ const overrides = new Map([ // ratcheting 1443 -> 1295. Queued to split further in the A2 fold. // global-agent-config: resolve_deploy_model_provider + visibility exports // add ~40 lines on top of the 1A.1 ratchet. Queued to split. - // +22 (1340 -> 1360): agent-config-resolver — start_local_agent_with_preflight + // +29 (1340 -> 1369, main): agent-config-resolver — start_local_agent_with_preflight // uses resolve_effective_relay_mesh_model_id at both preflight call sites; // preview_prospective_persona_snapshot helper extracted; orphan guard threaded - // through restore path. Load-bearing feature changes; queued to split. - // +9 (1360 -> 1369): start_local_agent_pairs_with_preflight — added - // personas/global load + resolve_effective_relay_mesh_model_id call to - // replace stale record-byte preflight. Same resolver pattern as - // start_local_agent_with_preflight. Load-bearing; queued to split. - ["src-tauri/src/commands/agents.rs", 1369], + // through restore path; start_local_agent_pairs_with_preflight resolver + // preflight. Load-bearing feature changes; queued to split. + // +47 (#2773): review fix — load_global_agent_config hoisted out of + // build_managed_agent_summary into callers, dangling-harness summaries render + // the deleted id, and spawn errors surface as sentences (tests included). + // +1: merge of the two deltas above (actual post-merge count). + ["src-tauri/src/commands/agents.rs", 1418], // agent-lifecycle-fixes: cascade-delete in delete_persona restructured into // 3-phase (stage/stop/commit) + commit_cascade_agents injectable helper for // retry-safety. Load-bearing reviewer-required change; queued to split. @@ -172,13 +173,22 @@ 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. - // +7 (1041 -> 1048): rebase onto main — this PR's resolver tests land on top - // of main's #2397 Windows shim tests. Test-only; queued to split. - ["src-tauri/src/managed_agents/runtime/tests.rs", 1055], + // +7 (main): this PR's resolver tests land on top of main's #2397 Windows + // shim tests, plus main's restart_eligible orphan-gate tests. + // +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], + // runtime.rs re-entered the list after the #1968 merge: main's + // definition-authoritative resolver comments grew it to 982, and this PR's + // typed harness-descriptor resolution in spawn_agent_child (+38) lands on + // top. Queued to shrink with the next runtime split pass (#2974 follow-up). + ["src-tauri/src/managed_agents/runtime.rs", 1020], // 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 @@ -229,7 +239,14 @@ 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. + // +6: ManagedAgent.runtime record-level pin + JSDoc so the harness delete + // confirmation can count referencing agents (review fix for #2773). + ["src/shared/api/types.ts", 1058], // harness-persona-sync feature growth, queued to split in the resolver-unify // refactor followup. discovery.rs is dominated by the new test module // (the effective_agent_command / divergent / create-time override matrix); @@ -280,7 +297,52 @@ 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. + // +13: review fix for #2773 — discovery publishes the registry by re-reading + // the harness dir under persist_mutex (publish_harness_registry_from_dir call + // + doc comment), closing the stale-snapshot clobber race. + // +35: review round 2 (#2773) — cfg(test) pre_publish_test_hook seam so the + // stale-publish regression is pinned through the REAL discover_acp_runtimes_from + // path (Wren's finding: the seam-only tests stayed green under a stale-publish + // mutant). Test-only code, zero release-build footprint. + ["src-tauri/src/managed_agents/discovery.rs", 1780], + // 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. + // +153: review fix for #2773 — collision/dup filtering moved into + // load_custom_harnesses so warm + discovery inherit identical shadowing + // rules, publish_harness_registry_from_dir (mutex-scoped publish seam), and + // comma-in-args validation at validate_harness_definition, with tests. + // +34: review round 2 (#2773) — Dawn's mutation finding: the loader-boundary + // collision/dedup enforcement was untested (deleting it left the suite green). + // load_applies_id_collision_check now drives the real loader against a real + // shadowing file, plus a dedup twin; both verified to kill the mutants. + ["src-tauri/src/managed_agents/custom_harnesses.rs", 1232], // 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. @@ -291,7 +353,20 @@ 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. + // +90: review fix for #2773 — deterministic interleaving regressions for the + // discovery publish race (save-during-discovery survives publish; + // delete-during-discovery stays gone). + // +103: review round 2 (#2773) — production-path interleaving regressions: + // discovery_publish_path_survives_mid_flight_save / _drops_mid_flight_delete + // drive the real discover_acp_runtimes_from with a save/delete landed via the + // pre_publish_test_hook; verified to red under a stale-publish mutant. + // +18: flake fix — lock_path_mutex + registry_test_lock guards (with lock- + // order comments) on the four tests that drive discovery's global caches. + ["src-tauri/src/managed_agents/discovery/tests.rs", 1787], // 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 @@ -449,15 +524,23 @@ const overrides = new Map([ // (if let Some(provider_update) = input.provider { record.provider = provider_update; }). // +8: harness_override thread-through in update_managed_agent so a deliberate // Custom pin routes to update_time_agent_command_override (comment + call). - // +22 (1079 -> 1101): Finding 2 — model discovery now resolves through - // resolve_effective_model_provider instead of raw record bytes - // (saved_agent_model_discovery_config takes personas/global and the - // get_agent_models call site loads global config), plus + // +22 (1079 -> 1101, main): Finding 2 — model discovery now resolves through + // resolve_effective_model_provider instead of raw record bytes, plus // apply_model_provider_prompt_update's linked-instance write-guard // extraction and its regression tests. // +4 (1101 -> 1105): rebase onto agents-everywhere — agents.rs function // signatures updated for ManagedAgentRuntimeKey-keyed runtimes map. - ["src-tauri/src/commands/agent_models.rs", 1113], + // +1 (#2773): model_discovery_error helper routes dangling-harness + // resolution errors through user_facing_harness_error (sentence, not raw + // DANGLING_HARNESS_ID sentinel) for the get_agent_models surface. The PR's + // descriptor path also deletes saved_agent_model_discovery_config, whose + // callers now use resolve_effective_model_provider + the descriptor env + // directly (net wash after the merge of the deltas above). + // +38 (1114 -> 1152): agent_model_discovery_config extracted as a pure, + // test-bindable seam (struct + helper + docs) so the linked-agent + // regression test kills the stale-record mutation at get_agent_models' + // consumption point (review finding, Wren + Dawn). + ["src-tauri/src/commands/agent_models.rs", 1152], // global-agent-config: get_agent_config_surface / write_agent_config_field / // put_agent_session_config commands + GlobalAgentConfig serde types. New file // in this PR; queued to split with the command module refactor. @@ -502,15 +585,26 @@ const overrides = new Map([ // 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. - // +59: run install commands under `pipefail` so a failing `curl` in a + // +59 (main): run install commands under `pipefail` so a failing `curl` in a // `curl … | bash` install fails the `cli` step instead of being masked by // `bash`'s exit 0, plus tests for the arg shape and the real pipeline status. - // +81: install_shell_args re-exports the composed PATH inside the command + // +81 (main): install_shell_args re-exports the composed PATH inside the command // body so login startup files can't clear or reorder it, plus an isolated // hostile-profile regression the pure composition tests structurally miss. - // +42: gate that re-export off Windows, where join_paths is `;`-separated and - // bash would collapse it into one entry, plus a platform-shape test. - ["src-tauri/src/commands/agent_discovery.rs", 2022], + // +42 (main): gate that re-export off Windows, where join_paths is `;`-separated + // and bash would collapse it into one entry, plus a platform-shape test. + // +126 (#2773): 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). + ["src-tauri/src/commands/agent_discovery.rs", 2167], // 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 080584c17f..d67ebb8f10 100644 --- a/desktop/src-tauri/Cargo.lock +++ b/desktop/src-tauri/Cargo.lock @@ -1080,6 +1080,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 0ed7417333..50cc1464a6 100644 --- a/desktop/src-tauri/Cargo.toml +++ b/desktop/src-tauri/Cargo.toml @@ -125,6 +125,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 d97e26001d..84a00433c0 100644 --- a/desktop/src-tauri/src/commands/agent_discovery.rs +++ b/desktop/src-tauri/src/commands/agent_discovery.rs @@ -69,16 +69,161 @@ 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. +/// +/// `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 2e423ed6dd..ca1fe9bdf6 100644 --- a/desktop/src-tauri/src/commands/agent_models.rs +++ b/desktop/src-tauri/src/commands/agent_models.rs @@ -18,10 +18,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}, @@ -38,15 +38,7 @@ pub async fn get_agent_models( app: AppHandle, state: State<'_, AppState>, ) -> Result { - let ( - resolved_acp, - agent_command, - agent_args, - persisted_model, - saved_provider, - provider_env_var, - merged_env, - ) = { + let (resolved_acp, agent_command, discovery) = { let _store_guard = state .managed_agents_store_lock .lock() @@ -77,35 +69,31 @@ 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 pure helper — descriptor + authoritative model/provider + // resolver, packaged so the linked-agent regression test binds the + // exact values this command consumes. Returns Err on dangling harness + // id, propagating it to the caller. + let discovery = agent_model_discovery_config(record, &personas, &global) + .map_err(|e| model_discovery_error(&pubkey, &e))?; - let resolved_agent = resolve_command(&effective_command) + let resolved_agent = resolve_command(&discovery.command) .map(|p| p.display().to_string()) - .unwrap_or_else(|| effective_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 resolver's effective model/provider — - // definition-authoritative for linked instances — so discovery can - // never query a stale provider this agent will not actually launch - // with. - let global = crate::managed_agents::load_global_agent_config(&app).unwrap_or_default(); - let discovery = - saved_agent_model_discovery_config(record, &effective_command, &personas, &global); - - ( - resolved, - resolved_agent, - args, - discovery.model, - discovery.provider, - discovery.provider_env_var, - discovery.env, - ) + .unwrap_or_else(|| discovery.command.clone()); + + (resolved, resolved_agent, discovery) }; // store lock released — subprocess runs without holding the lock + let AgentModelDiscoveryConfig { + args: agent_args, + model: persisted_model, + provider: saved_provider, + provider_env_var, + env: merged_env, + command: _, + } = discovery; + let merged_env = discovery_env_with_baked_floor(merged_env); // Resolve against the baked/process env when the record saved no provider, // so a build-provided provider still gets live discovery. @@ -154,52 +142,80 @@ pub async fn get_agent_models( .await } +/// Error copy for a failed harness resolution during model discovery. +/// +/// Routes through `user_facing_harness_error` so a dangling harness id renders +/// as a sentence, never as the raw `DANGLING_HARNESS_ID:` sentinel — the same +/// contract spawn and summary rows honor. +fn model_discovery_error(pubkey: &str, error: &str) -> String { + format!( + "cannot discover models for {pubkey}: {}", + crate::managed_agents::user_facing_harness_error(error) + ) +} + +/// Everything `get_agent_models` needs from the record + context, resolved in +/// one pure step so the linked-agent regression test can bind the exact values +/// the command consumes. #[derive(Debug, PartialEq, Eq)] -struct SavedAgentModelDiscoveryConfig { +struct AgentModelDiscoveryConfig { + /// Effective harness command (descriptor-resolved), for `resolve_command`. + command: String, + /// Effective harness args (descriptor-resolved). + args: Vec, + /// Model from the authoritative resolver spawn uses — linked instances + /// read their definition, never stale `record.model` bytes. model: Option, + /// Provider from the same authoritative resolver — never stale + /// `record.provider` bytes for linked instances. provider: Option, - /// The runtime's provider env var (e.g. `BUZZ_AGENT_PROVIDER`), so discovery - /// can recover the provider from the env when the record has none. `None` - /// for runtimes that do not take a provider, or an unknown command. + /// The runtime's provider env var (e.g. `GOOSE_PROVIDER`), so discovery + /// can recover the provider from the env when the resolver yields none. + /// `None` for runtimes that do not take a provider, or an unknown command. provider_env_var: Option<&'static str>, + /// The descriptor's fully layered env (definition/persona/global/agent). env: BTreeMap, } -/// Resolve the model/provider discovery config from the same authoritative -/// source spawn uses (`resolve_effective_model_provider`) — linked instances -/// read their definition, never a stale materialized `record.model`/ -/// `record.provider`, so model discovery cannot query a provider this agent -/// will not actually launch with. Definition-less instances keep their own -/// record values, matching spawn's `resolve_definition_less` arm. -fn saved_agent_model_discovery_config( +/// Resolve the model-discovery config for a saved agent — the descriptor-backed +/// successor to the old `saved_agent_model_discovery_config`. +/// +/// Command/args/env come from `resolve_effective_harness_descriptor` (the same +/// resolver as `spawn_agent_child`); model/provider come from +/// `resolve_effective_model_provider` (#1968's definition-authoritative +/// contract) — linked instances read their definition, never a stale +/// materialized `record.model`/`record.provider`, so discovery cannot query a +/// provider this agent will not actually launch with. Definition-less +/// instances keep their own record values, matching spawn's +/// `resolve_definition_less` arm. When the resolver yields no provider, +/// `effective_discovery_provider` recovers the provider the agent will +/// actually launch with from the runtime's own provider env var, read out of +/// the descriptor env (which already layers definition/persona/global values +/// the same way spawn does). +/// +/// Returns `Err("DANGLING_HARNESS_ID:")` from the descriptor resolver when +/// the harness id no longer exists; the caller routes it through +/// `model_discovery_error`. +fn agent_model_discovery_config( record: &crate::managed_agents::ManagedAgentRecord, - agent_command: &str, personas: &[crate::managed_agents::AgentDefinition], global: &crate::managed_agents::GlobalAgentConfig, -) -> SavedAgentModelDiscoveryConfig { - let runtime_meta = known_acp_runtime(agent_command); - let (effective_model, effective_provider) = +) -> Result { + let descriptor = + crate::managed_agents::resolve_effective_harness_descriptor(record, personas, global)?; + let (model, provider) = crate::managed_agents::resolve_effective_model_provider(record, personas, global); - - let mut derived_env = BTreeMap::new(); - if let Some(meta) = runtime_meta { - for (key, value) in crate::managed_agents::runtime_metadata_env_vars( - meta.model_env_var, - meta.provider_env_var, - meta.provider_locked, - effective_model.as_deref(), - effective_provider.as_deref(), - ) { - derived_env.insert(key.to_string(), value.to_string()); - } - } - - SavedAgentModelDiscoveryConfig { - model: effective_model, - provider: effective_provider, - provider_env_var: runtime_meta.and_then(|meta| meta.provider_env_var), - env: crate::managed_agents::merged_user_env(&derived_env, &record.env_vars), - } + let provider_env_var = + known_acp_runtime(&descriptor.command).and_then(|meta| meta.provider_env_var); + + Ok(AgentModelDiscoveryConfig { + command: descriptor.command, + args: descriptor.args, + model, + provider, + provider_env_var, + env: descriptor.env, + }) } #[derive(Debug, Deserialize)] @@ -214,6 +230,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. @@ -227,6 +247,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 @@ -260,7 +282,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); // Recover a build-provided provider when the form has none, so the create // dialog discovers live models instead of falling through to the subprocess. @@ -987,7 +1020,13 @@ pub async fn update_managed_agent( let summary = { let personas = load_personas(&app).unwrap_or_default(); - build_managed_agent_summary(&app, record, &runtimes, &personas)? + build_managed_agent_summary( + &app, + record, + &runtimes, + &personas, + &crate::managed_agents::load_global_agent_config(&app).unwrap_or_default(), + )? }; let rollback = name_changed.then(|| AgentUpdateRollback::new(previous_record, record)); (summary, sync_params, rollback) diff --git a/desktop/src-tauri/src/commands/agent_models_tests.rs b/desktop/src-tauri/src/commands/agent_models_tests.rs index d962545bdc..d98460109f 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_for_definition_less_agent() "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,29 +194,33 @@ fn saved_agent_model_discovery_uses_record_snapshot_for_definition_less_agent() ) .expect("sample managed agent record"); - let config = saved_agent_model_discovery_config( - &record, - "goose", - &[], - &crate::managed_agents::GlobalAgentConfig::default(), - ); + // agent_model_discovery_config is the single helper get_agent_models + // consumes — verify it layers env correctly, strips reserved keys, and + // keeps the record's own model/provider for a definition-less instance + // (matching spawn's `resolve_definition_less` arm). + let discovery = agent_model_discovery_config(&record, &[], &Default::default()) + .expect("discovery config 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!(discovery.command.as_str(), "goose"); + assert_eq!(discovery.model.as_deref(), Some("record-model")); + assert_eq!(discovery.provider.as_deref(), Some("databricks")); assert_eq!( - config.env.get("GOOSE_MODEL").map(String::as_str), + discovery.env.get("GOOSE_MODEL").map(String::as_str), Some("record-model") ); assert_eq!( - config.env.get("GOOSE_PROVIDER").map(String::as_str), + discovery.env.get("GOOSE_PROVIDER").map(String::as_str), Some("databricks") ); assert_eq!( - config.env.get("OPENAI_API_KEY").map(String::as_str), + discovery.env.get("OPENAI_API_KEY").map(String::as_str), Some("record-key") ); - assert!(!config.env.contains_key("BUZZ_PRIVATE_KEY")); - assert_eq!(config.provider_env_var, Some("GOOSE_PROVIDER")); + // Reserved keys are stripped from the descriptor env. + assert!(!discovery.env.contains_key("BUZZ_PRIVATE_KEY")); + // The provider env var is recovered from the runtime metadata for the + // effective command (the old SavedAgentModelDiscoveryConfig.provider_env_var). + assert_eq!(discovery.provider_env_var, Some("GOOSE_PROVIDER")); } // --------------------------------------------------------------------------- @@ -351,7 +356,7 @@ fn effective_discovery_provider_reads_the_runtimes_own_env_var() { /// linked definition's current model/provider wins, mirroring spawn's /// `resolve_effective_model_provider`. #[test] -fn saved_agent_model_discovery_ignores_stale_record_for_linked_agent() { +fn model_discovery_ignores_stale_record_for_linked_agent() { let record: crate::managed_agents::ManagedAgentRecord = serde_json::from_str( r#"{ "pubkey": "abcd1234", @@ -383,7 +388,7 @@ fn saved_agent_model_discovery_ignores_stale_record_for_linked_agent() { display_name: "Persona".to_string(), avatar_url: None, system_prompt: "You are a persona.".to_string(), - runtime: None, + runtime: Some("goose".to_string()), model: Some("persona-model".to_string()), provider: Some("anthropic".to_string()), name_pool: Vec::new(), @@ -399,19 +404,27 @@ fn saved_agent_model_discovery_ignores_stale_record_for_linked_agent() { updated_at: "".to_string(), }; - let config = saved_agent_model_discovery_config( - &record, - "goose", - &[persona], - &crate::managed_agents::GlobalAgentConfig::default(), - ); - - assert_eq!(config.model.as_deref(), Some("persona-model")); - assert_eq!(config.provider.as_deref(), Some("anthropic")); + // agent_model_discovery_config is the single helper get_agent_models + // consumes — the stale record bytes must lose to the persona's current + // model/provider (the same authoritative resolver spawn uses). + let personas = [persona]; + let global = crate::managed_agents::GlobalAgentConfig::default(); + let discovery = agent_model_discovery_config(&record, &personas, &global) + .expect("discovery config should resolve for a linked record"); + assert_eq!(discovery.model.as_deref(), Some("persona-model")); + assert_eq!(discovery.provider.as_deref(), Some("anthropic")); + + // And the discovery env comes from the descriptor, whose layering also + // resolves through the definition — the derived model env var must carry + // the persona's model, not the stale record snapshot. assert_eq!( - config.env.get("GOOSE_MODEL").map(String::as_str), + discovery.env.get("GOOSE_MODEL").map(String::as_str), Some("persona-model") ); + assert_eq!( + discovery.env.get("GOOSE_PROVIDER").map(String::as_str), + Some("anthropic") + ); } // --------------------------------------------------------------------------- @@ -560,3 +573,18 @@ fn is_databricks_provider_matches_both_variants() { assert!(!is_databricks_provider(Some("anthropic"))); assert!(!is_databricks_provider(None)); } + +#[test] +fn model_discovery_error_converts_dangling_sentinel_to_sentence() { + // get_agent_models is a user-facing surface: a dangling harness must + // render as a sentence, never as the raw DANGLING_HARNESS_ID: sentinel. + let raw = format!("{}doomed", crate::managed_agents::DANGLING_HARNESS_PREFIX); + let msg = model_discovery_error("agent-pk", &raw); + assert!(msg.contains("cannot discover models for agent-pk")); + assert!(msg.contains("\"doomed\"") && msg.contains("deleted")); + assert!(!msg.contains(crate::managed_agents::DANGLING_HARNESS_PREFIX)); + + // Non-dangling errors pass through untouched. + let plain = model_discovery_error("agent-pk", "plain failure"); + assert_eq!(plain, "cannot discover models for agent-pk: plain failure"); +} diff --git a/desktop/src-tauri/src/commands/agent_settings.rs b/desktop/src-tauri/src/commands/agent_settings.rs index 4f043d2403..2317930c1e 100644 --- a/desktop/src-tauri/src/commands/agent_settings.rs +++ b/desktop/src-tauri/src/commands/agent_settings.rs @@ -57,7 +57,13 @@ pub async fn set_managed_agent_start_on_app_launch( .find(|record| record.pubkey == pubkey) .ok_or_else(|| format!("agent {pubkey} not found"))?; let personas = load_personas(&app).unwrap_or_default(); - build_managed_agent_summary(&app, record, &runtimes, &personas) + build_managed_agent_summary( + &app, + record, + &runtimes, + &personas, + &crate::managed_agents::load_global_agent_config(&app).unwrap_or_default(), + ) }) .await .map_err(|e| format!("spawn_blocking failed: {e}"))? @@ -102,7 +108,13 @@ pub async fn set_managed_agent_auto_restart( .find(|record| record.pubkey == pubkey) .ok_or_else(|| format!("agent {pubkey} not found"))?; let personas = load_personas(&app).unwrap_or_default(); - build_managed_agent_summary(&app, record, &runtimes, &personas) + build_managed_agent_summary( + &app, + record, + &runtimes, + &personas, + &crate::managed_agents::load_global_agent_config(&app).unwrap_or_default(), + ) }) .await .map_err(|e| format!("spawn_blocking failed: {e}"))? diff --git a/desktop/src-tauri/src/commands/agents.rs b/desktop/src-tauri/src/commands/agents.rs index 61e4034b9f..3b5ebeca4f 100644 --- a/desktop/src-tauri/src/commands/agents.rs +++ b/desktop/src-tauri/src/commands/agents.rs @@ -389,7 +389,13 @@ pub(super) async fn start_local_agent_pairs_with_preflight( .iter() .find(|record| record.pubkey == pubkey) .ok_or_else(|| format!("agent {pubkey} not found"))?; - build_managed_agent_summary(app, record, &runtimes, &personas) + build_managed_agent_summary( + app, + record, + &runtimes, + &personas, + &crate::managed_agents::load_global_agent_config(app).unwrap_or_default(), + ) } pub(super) async fn start_local_agent_with_preflight( @@ -476,7 +482,13 @@ pub(super) async fn start_local_agent_with_preflight( .iter() .find(|record| record.pubkey == pubkey) .ok_or_else(|| format!("agent {pubkey} not found"))?; - build_managed_agent_summary(app, record, &runtimes, &personas) + build_managed_agent_summary( + app, + record, + &runtimes, + &personas, + &crate::managed_agents::load_global_agent_config(app).unwrap_or_default(), + ) } /// Deploy an agent to a provider backend. Resolves the binary, calls deploy via @@ -577,9 +589,16 @@ pub async fn list_managed_agents(app: AppHandle) -> Result Err(format!( "agent {pubkey} has unsupported backend kind: {backend:?}" @@ -1257,7 +1300,13 @@ pub async fn stop_managed_agent( .find(|record| record.pubkey == pubkey) .ok_or_else(|| format!("agent {pubkey} not found"))?; let personas = load_personas(&app).unwrap_or_default(); - build_managed_agent_summary(&app, record, &runtimes, &personas) + build_managed_agent_summary( + &app, + record, + &runtimes, + &personas, + &crate::managed_agents::load_global_agent_config(&app).unwrap_or_default(), + ) }) .await .map_err(|e| format!("spawn_blocking failed: {e}"))? 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..e6bc09496c --- /dev/null +++ b/desktop/src-tauri/src/managed_agents/custom_harnesses.rs @@ -0,0 +1,1231 @@ +//! 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. +/// +/// Filtering is applied HERE — at the loader boundary — so every consumer +/// (discovery Phase 3, `warm_harness_registry_from_dir`, and through it the +/// spawn/readiness registry) inherits the same guarantees: +/// * `check_id_collision` — a hand-placed `goose.json` can never shadow a +/// built-in or preset id, even on the cold-launch warm path; +/// * duplicate-id dedup within the directory (first file wins). +/// +/// **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(); + let mut seen_ids = std::collections::HashSet::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; + } + + // A custom file must never shadow a built-in or preset id — enforced at + // the loader so the warm path can't admit what discovery would reject. + if let Err(reason) = check_id_collision(&def.id) { + tracing::warn!("custom_harnesses: skipping {} — {reason}", path.display()); + continue; + } + + // Dedup within the directory itself (a file's id is taken from its JSON + // content, not its filename, so two files can carry the same id). + if !seen_ids.insert(def.id.clone()) { + tracing::warn!( + "custom_harnesses: skipping {} — duplicate id {:?}", + path.display(), + def.id + ); + continue; + } + + 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()); + } + // Args travel to the harness through the comma-delimited + // `BUZZ_ACP_AGENT_ARGS` env transport (clap `value_delimiter = ','` on the + // buzz-acp side), so a literal comma inside one argument would silently + // split into two arguments at runtime. Reject at the validation boundary — + // shared by save (UI/Tauri) and load (hand-authored files) — so the + // invariant holds regardless of how the definition arrives. + if let Some(arg) = def.args.iter().find(|a| a.contains(',')) { + return Err(format!( + "args: argument {arg:?} contains a comma — arguments are passed via a \ + comma-delimited transport and would be split at spawn time; \ + use separate argument entries instead" + )); + } + // 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); +} + +/// Publish the loaded-harness registry from a **fresh** directory read while +/// holding the persist mutex — the discovery-side counterpart of +/// `save_and_warm` / `delete_and_warm`. +/// +/// `discover_acp_runtimes_from` runs slow auth probes between its directory +/// scan and its registry publish. Publishing the pre-probe snapshot could +/// clobber a `save_and_warm` that landed in that window, leaving a freshly +/// saved harness unresolvable at spawn until the next discovery. Re-reading +/// the directory at publish time, under the same mutex as save/delete, makes +/// the documented guarantee ("the registry always reflects disk at warm time") +/// actually hold. Only the publish is locked — never the probes. +pub(crate) fn warm_harness_registry_locked(custom_dir: Option<&std::path::Path>) { + let _guard = persist_mutex().lock().unwrap_or_else(|e| e.into_inner()); + warm_harness_registry_from_dir(custom_dir); +} + +/// 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 whose id shadows a built-in ("goose") must be dropped + // BY THE LOADER — `load_custom_harnesses` is the enforcement boundary + // shared by both the warm path and discovery. This exercises the real + // loader against a real file, not just the helper predicate. + let dir = tempfile::tempdir().unwrap(); + fs::write( + dir.path().join("goose.json"), + r#"{"id":"goose","label":"Not Goose","command":"goose","args":["--evil"]}"#, + ) + .unwrap(); + assert!( + load_custom_harnesses(dir.path()).is_empty(), + "loader must drop a file shadowing a builtin id" + ); + assert!(check_id_collision("goose").is_err()); + assert!(check_id_collision("custom-goose").is_ok()); + } + + #[test] + fn load_dedups_duplicate_ids_first_file_wins() { + // Two files carrying the same custom id: the loader must keep exactly + // one definition (directory-order first wins; the duplicate is dropped). + let dir = tempfile::tempdir().unwrap(); + fs::write( + dir.path().join("a.json"), + r#"{"id":"custom-dup","label":"First","command":"first-cmd"}"#, + ) + .unwrap(); + fs::write( + dir.path().join("b.json"), + r#"{"id":"custom-dup","label":"Second","command":"second-cmd"}"#, + ) + .unwrap(); + let loaded = load_custom_harnesses(dir.path()); + assert_eq!( + loaded.len(), + 1, + "loader must dedup duplicate ids within the directory" + ); + assert_eq!(loaded[0].id, "custom-dup"); + } + + // ── Round-trip via save_custom_harness_to_dir (B-4) ───────────────────── + // + // These tests exercise the REAL persistence helper, not raw fs::write. + // They prove: create, same-ID edit (backup-swap), rename (old file removed), + // backup file cleaned up on success. + + fn make_def(id: &str, label: &str) -> HarnessDefinition { + HarnessDefinition { + id: id.to_string(), + label: label.to_string(), + command: format!("{id}-bin"), + args: vec![], + env: BTreeMap::new(), + install_instructions_url: String::new(), + install_hint: String::new(), + } + } + + #[test] + fn save_to_dir_create_writes_file_and_loads_back() { + let dir = tempfile::tempdir().unwrap(); + let def = make_def("my-harness", "My Harness"); + + let outcome = save_custom_harness_to_dir(dir.path(), &def, None).unwrap(); + + assert_eq!(outcome.target_path, dir.path().join("my-harness.json")); + assert!(outcome.removed_old_path.is_none(), "no old file on create"); + + let loaded = load_custom_harnesses(dir.path()); + assert_eq!(loaded.len(), 1); + assert_eq!(loaded[0].id, "my-harness"); + assert_eq!(loaded[0].label, "My Harness"); + } + + #[test] + fn save_to_dir_same_id_edit_replaces_content() { + let dir = tempfile::tempdir().unwrap(); + let v1 = make_def("my-harness", "V1 Label"); + save_custom_harness_to_dir(dir.path(), &v1, None).unwrap(); + + // Same-ID edit: label changes. + let v2 = make_def("my-harness", "V2 Label"); + let outcome = save_custom_harness_to_dir(dir.path(), &v2, None).unwrap(); + + // No old-path reported (id unchanged). + assert!(outcome.removed_old_path.is_none()); + + let loaded = load_custom_harnesses(dir.path()); + assert_eq!(loaded.len(), 1, "same-id edit must not duplicate entries"); + assert_eq!(loaded[0].label, "V2 Label", "v2 content must be present"); + } + + #[test] + fn save_to_dir_backup_is_cleaned_up_after_same_id_edit() { + let dir = tempfile::tempdir().unwrap(); + let v1 = make_def("my-harness", "V1"); + save_custom_harness_to_dir(dir.path(), &v1, None).unwrap(); + + let v2 = make_def("my-harness", "V2"); + save_custom_harness_to_dir(dir.path(), &v2, None).unwrap(); + + // .bak file must be gone after a successful commit. + let bak = dir.path().join("my-harness.json.bak"); + assert!( + !bak.exists(), + ".bak file must be removed after successful same-id edit" + ); + } + + #[test] + fn save_to_dir_rename_removes_old_file_and_creates_new() { + let dir = tempfile::tempdir().unwrap(); + let old_def = make_def("old-id", "Old"); + save_custom_harness_to_dir(dir.path(), &old_def, None).unwrap(); + + // Rename: new id, old_id supplied. + let new_def = make_def("new-id", "New"); + let outcome = save_custom_harness_to_dir(dir.path(), &new_def, Some("old-id")).unwrap(); + + // The outcome carries the old path that was removed. + let expected_old = dir.path().join("old-id.json"); + assert_eq!( + outcome.removed_old_path, + Some(expected_old.clone()), + "removed_old_path must be the old file" + ); + + // Old file gone, new file present. + assert!(!expected_old.exists(), "old-id.json must be removed"); + let loaded = load_custom_harnesses(dir.path()); + assert_eq!(loaded.len(), 1); + assert_eq!(loaded[0].id, "new-id"); + } + + #[test] + fn save_to_dir_rename_nonexistent_old_id_is_non_fatal() { + // rename_old_id pointing to a file that does not exist must succeed + // (NotFound is silently ignored by the helper). + let dir = tempfile::tempdir().unwrap(); + let def = make_def("alpha", "Alpha"); + let outcome = save_custom_harness_to_dir(dir.path(), &def, Some("ghost-id")).unwrap(); + + // New file created, no old path removed. + assert_eq!(outcome.target_path, dir.path().join("alpha.json")); + assert!( + outcome.removed_old_path.is_none(), + "NotFound old-id must not be reported as removed" + ); + assert!(load_custom_harnesses(dir.path()).len() == 1); + } + + #[test] + fn save_to_dir_roundtrip_with_env_preserves_values() { + let dir = tempfile::tempdir().unwrap(); + let mut env = BTreeMap::new(); + env.insert("MY_KEY".to_string(), "my_value".to_string()); + let def = HarnessDefinition { + id: "env-harness".to_string(), + label: "Env Harness".to_string(), + command: "env-bin".to_string(), + args: vec!["--flag".to_string()], + env, + install_instructions_url: "https://example.com".to_string(), + install_hint: "Install from example.com".to_string(), + }; + + save_custom_harness_to_dir(dir.path(), &def, None).unwrap(); + + let loaded = load_custom_harnesses(dir.path()); + assert_eq!(loaded.len(), 1); + assert_eq!(loaded[0].args, vec!["--flag"]); + assert_eq!( + loaded[0].env.get("MY_KEY").map(String::as_str), + Some("my_value"), + "env must round-trip through save_custom_harness_to_dir" + ); + } + + // ── B-3: env validation boundary (validate_harness_definition_pub integration) ── + + #[test] + fn validate_rejects_malformed_key_with_equals_sign() { + // BUZZ_AUTH_TAG=x is the documented reserved-key bypass shape: + // the key contains '=' so Command::env would produce + // `BUZZ_AUTH_TAG=x=forged` in the child env. + let mut env = BTreeMap::new(); + env.insert("BUZZ_AUTH_TAG=x".to_string(), "forged".to_string()); + let def = HarnessDefinition { + id: "bad-env".to_string(), + label: "Bad".to_string(), + command: "bad-bin".to_string(), + args: vec![], + env, + install_instructions_url: String::new(), + install_hint: String::new(), + }; + let err = validate_harness_definition_pub(&def).unwrap_err(); + assert!( + err.contains("env var keys must match"), + "malformed key must be rejected: {err}" + ); + assert!( + err.contains("BUZZ_AUTH_TAG"), + "error must name the offending key: {err}" + ); + } + + #[test] + fn validate_rejects_reserved_key_buzz_managed_agent() { + // BUZZ_MANAGED_AGENT and BUZZ_MANAGED_AGENT_START_NONCE are the + // ownership markers — supplying them in a definition must be rejected. + let mut env = BTreeMap::new(); + env.insert( + "BUZZ_MANAGED_AGENT".to_string(), + "fake-instance".to_string(), + ); + let def = HarnessDefinition { + id: "bad-marker".to_string(), + label: "Bad".to_string(), + command: "bad-bin".to_string(), + args: vec![], + env, + install_instructions_url: String::new(), + install_hint: String::new(), + }; + let err = validate_harness_definition_pub(&def).unwrap_err(); + assert!( + err.contains("reserved by Buzz"), + "ownership marker key must be rejected: {err}" + ); + } + + #[test] + fn validate_rejects_reserved_key_case_insensitive() { + // BUZZ_PRIVATE_KEY in any casing must be blocked. + let mut env = BTreeMap::new(); + env.insert("buzz_private_key".to_string(), "secret".to_string()); + let def = HarnessDefinition { + id: "ci-marker".to_string(), + label: "CI".to_string(), + command: "ci-bin".to_string(), + args: vec![], + env, + install_instructions_url: String::new(), + install_hint: String::new(), + }; + let err = validate_harness_definition_pub(&def).unwrap_err(); + assert!( + err.contains("reserved by Buzz"), + "reserved key must be blocked case-insensitively: {err}" + ); + } + + #[test] + fn validate_rejects_nul_byte_in_value() { + // A NUL in a value would cause Command::env to panic at spawn time. + let mut env = BTreeMap::new(); + env.insert("MY_KEY".to_string(), "val\x00ue".to_string()); + let def = HarnessDefinition { + id: "nul-val".to_string(), + label: "NUL".to_string(), + command: "nul-bin".to_string(), + args: vec![], + env, + install_instructions_url: String::new(), + install_hint: String::new(), + }; + let err = validate_harness_definition_pub(&def).unwrap_err(); + assert!( + err.contains("NUL bytes"), + "NUL value must be rejected at validation: {err}" + ); + } + + #[test] + fn validate_rejects_value_over_per_value_size_limit() { + use crate::managed_agents::env_vars::MAX_ENV_VALUE_BYTES; + let mut env = BTreeMap::new(); + // One byte over the per-value cap. + env.insert("BIG_VAL".to_string(), "x".repeat(MAX_ENV_VALUE_BYTES + 1)); + let def = HarnessDefinition { + id: "big-val".to_string(), + label: "Big".to_string(), + command: "big-bin".to_string(), + args: vec![], + env, + install_instructions_url: String::new(), + install_hint: String::new(), + }; + let err = validate_harness_definition_pub(&def).unwrap_err(); + assert!( + err.contains("per-value limit"), + "oversized value must be rejected: {err}" + ); + } + + #[test] + fn validate_accepts_well_formed_env() { + let mut env = BTreeMap::new(); + env.insert("ANTHROPIC_API_KEY".to_string(), "sk-test-123".to_string()); + env.insert("MODEL_VERSION".to_string(), "claude-3".to_string()); + let def = HarnessDefinition { + id: "good-env".to_string(), + label: "Good".to_string(), + command: "good-bin".to_string(), + args: vec![], + env, + install_instructions_url: String::new(), + install_hint: String::new(), + }; + assert!( + validate_harness_definition_pub(&def).is_ok(), + "well-formed definition must pass validation" + ); + } + + // ── Comma-in-args validation (transport-lossiness guard) ───────────────── + + /// A definition whose args contain a literal comma must be rejected at the + /// validation boundary — the comma-delimited `BUZZ_ACP_AGENT_ARGS` + /// transport would silently split it into two args at spawn time. + #[test] + fn validate_rejects_comma_in_args() { + let mut def = make_def("comma-args", "Comma"); + def.args = vec!["--name".to_string(), "a,b".to_string()]; + let err = validate_harness_definition_pub(&def).unwrap_err(); + assert!( + err.contains("comma"), + "error must explain the comma transport limit, got: {err}" + ); + } + + /// Comma-free args pass — including args with spaces and special chars. + #[test] + fn validate_accepts_comma_free_args() { + let mut def = make_def("clean-args", "Clean"); + def.args = vec!["acp".to_string(), "--flag=x y".to_string()]; + assert!(validate_harness_definition_pub(&def).is_ok()); + } + + /// The loader shares the same validator: a hand-authored file with a comma + /// in args is skipped, so the invariant holds regardless of how the + /// definition arrives (UI save or hand-edited JSON). + #[test] + fn load_skips_definition_with_comma_in_args() { + let dir = tempfile::tempdir().unwrap(); + fs::write( + dir.path().join("comma.json"), + r#"{"id":"comma-file","label":"Comma","command":"comma-bin","args":["a,b"]}"#, + ) + .unwrap(); + assert!( + load_custom_harnesses(dir.path()).is_empty(), + "comma-in-args definition must be skipped at the loader boundary" + ); + } + + // ── Discovery publish under persist_mutex (stale-snapshot regression) ──── + + /// Discovery's registry publish must re-read the directory at publish time + /// (under the persist mutex), not push a snapshot taken before the auth + /// probes ran. Regression shape: discovery scans dir → user saves harness X + /// (save_and_warm warms the registry with X) → discovery finishes. If + /// discovery published its pre-save snapshot, X would be on disk but + /// unresolvable at spawn until the next discover. + /// + /// Deterministic interleaving: we simulate it by calling the publish seam + /// (`warm_harness_registry_locked`) after a save that happened "during" + /// discovery — the fresh-read semantics mean the just-saved definition + /// survives the publish. + #[test] + fn discovery_publish_after_concurrent_save_keeps_saved_harness() { + let _lock = registry_test_lock(); + let dir = tempfile::tempdir().unwrap(); + + // Discovery "scans" the dir while it is empty (stale snapshot would be []). + let stale_snapshot = load_custom_harnesses(dir.path()); + assert!(stale_snapshot.is_empty()); + + // A save lands mid-discovery (save_and_warm: write + warm). + let def = make_def("mid-save", "Mid Save"); + save_and_warm(dir.path(), &def, None).unwrap(); + assert!(lookup_loaded_harness_by_id("mid-save").is_some()); + + // Discovery publishes — the locked warm re-reads the directory, so the + // just-saved harness must survive (a stale-snapshot publish would + // clobber it). + warm_harness_registry_locked(Some(dir.path())); + assert!( + lookup_loaded_harness_by_id("mid-save").is_some(), + "publish must re-read the directory, not clobber the mid-discovery save" + ); + } + + /// Same shape for delete: a delete landing mid-discovery must not be + /// resurrected by the discovery publish. + #[test] + fn discovery_publish_after_concurrent_delete_keeps_harness_gone() { + let _lock = registry_test_lock(); + let dir = tempfile::tempdir().unwrap(); + + let def = make_def("mid-delete", "Mid Delete"); + save_and_warm(dir.path(), &def, None).unwrap(); + + // Discovery "scans" while the file exists (stale snapshot would contain it). + let stale_snapshot = load_custom_harnesses(dir.path()); + assert_eq!(stale_snapshot.len(), 1); + + // Delete lands mid-discovery. + delete_and_warm(dir.path(), "mid-delete").unwrap(); + assert!(lookup_loaded_harness_by_id("mid-delete").is_none()); + + // Discovery publishes — fresh read keeps it gone. + warm_harness_registry_locked(Some(dir.path())); + assert!( + lookup_loaded_harness_by_id("mid-delete").is_none(), + "publish must not resurrect a harness deleted mid-discovery" + ); + } + + // ── Registry warm path ─────────────────────────────────────────────────── + + /// After `warm_harness_registry_from_dir` the registry contains preset + + /// custom definitions and `lookup_loaded_harness_by_id` resolves them. + #[test] + fn warm_registry_then_lookup_finds_custom_and_preset_entries() { + let _lock = registry_test_lock(); + let dir = tempfile::tempdir().unwrap(); + fs::write( + dir.path().join("my-custom.json"), + r#"{"id":"my-custom","label":"My Custom","command":"my-custom-bin"}"#, + ) + .unwrap(); + + warm_harness_registry_from_dir(Some(dir.path())); + + // Custom entry must be findable. + let found = lookup_loaded_harness_by_id("my-custom"); + assert!( + found.is_some(), + "warm registry must contain the custom entry" + ); + assert_eq!(found.unwrap().command, "my-custom-bin"); + + // At least one preset entry must be in the registry (e.g. "cursor"). + let preset = lookup_loaded_harness_by_id("cursor"); + assert!( + preset.is_some(), + "warm registry must contain preset entries" + ); + } + + /// `warm_harness_registry_from_dir` with `None` still loads presets. + #[test] + fn warm_registry_with_no_custom_dir_loads_presets_only() { + let _lock = registry_test_lock(); + warm_harness_registry_from_dir(None); + // At least the "cursor" preset must be present. + assert!( + lookup_loaded_harness_by_id("cursor").is_some(), + "presets must be reachable even without a custom dir" + ); + } + + /// `warm_harness_registry_from_dir` followed by `update_loaded_harness_registry` + /// with an empty slice clears the registry (transactional save/delete contract). + #[test] + fn warm_then_clear_registry_empties_lookup() { + let _lock = registry_test_lock(); + let dir = tempfile::tempdir().unwrap(); + fs::write( + dir.path().join("tmp-agent.json"), + r#"{"id":"tmp-agent","label":"Tmp","command":"tmp-bin"}"#, + ) + .unwrap(); + + warm_harness_registry_from_dir(Some(dir.path())); + assert!(lookup_loaded_harness_by_id("tmp-agent").is_some()); + + // Simulate delete — re-warm with empty dir. + let empty_dir = tempfile::tempdir().unwrap(); + warm_harness_registry_from_dir(Some(empty_dir.path())); + assert!( + lookup_loaded_harness_by_id("tmp-agent").is_none(), + "deleted harness must not appear after re-warm" + ); + } + + // ── Legacy avatarUrl regression (F1) ───────────────────────────────────── + + /// A JSON file that contains a legacy `avatarUrl` field (from pre-BYOH code) + /// must still deserialize without error (unknown-field handling) and the + /// loaded `HarnessDefinition` must NOT carry the URL — the field is absent + /// from the struct so serde drops it. + #[test] + fn legacy_avatar_url_in_json_is_silently_dropped_on_load() { + let dir = tempfile::tempdir().unwrap(); + fs::write( + dir.path().join("legacy.json"), + r#"{ + "id": "legacy-agent", + "label": "Legacy Agent", + "command": "legacy-bin", + "avatarUrl": "https://tracking.example.com/logo.png" + }"#, + ) + .unwrap(); + + let defs = load_custom_harnesses(dir.path()); + // The file must deserialize successfully (serde ignores unknown fields). + assert_eq!(defs.len(), 1, "legacy file with avatarUrl must still load"); + assert_eq!(defs[0].id, "legacy-agent"); + // HarnessDefinition has no avatar_url field — prove the URL cannot + // be routed to a catalog entry by serializing back and checking. + let json = serde_json::to_string(&defs[0]).unwrap(); + assert!( + !json.contains("https://tracking.example.com"), + "serialized HarnessDefinition must not contain the legacy avatar URL" + ); + } + + // ── Preset id reservation ──────────────────────────────────────────────── + + /// All preset ids must be blocked by `check_id_collision`. + #[test] + fn preset_ids_are_reserved_and_cannot_be_used_as_custom_ids() { + // Derived from PRESET_HARNESSES — no hard-coded copy here so this test + // automatically covers any future preset additions. + for id in crate::managed_agents::discovery::preset_harness_ids() { + assert!( + check_id_collision(id).is_err(), + "preset id {id:?} should be rejected by check_id_collision" + ); + } + } +} diff --git a/desktop/src-tauri/src/managed_agents/discovery.rs b/desktop/src-tauri/src/managed_agents/discovery.rs index a2e47d1281..2941b69ce5 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,119 @@ 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}; +/// Prefix of the typed dangling-harness error produced by +/// `try_record_agent_command` / `resolve_effective_harness_descriptor`. +/// +/// This sentinel is an internal Rust contract: user-facing surfaces must +/// convert it to a sentence via [`user_facing_harness_error`] (spawn) or to +/// the missing id via [`dangling_harness_id`] (summary) — never show it raw. +pub(crate) const DANGLING_HARNESS_PREFIX: &str = "DANGLING_HARNESS_ID:"; + +/// Extract the missing harness id from a `DANGLING_HARNESS_ID:` error. +/// Returns `None` for any other error string. +pub(crate) fn dangling_harness_id(error: &str) -> Option<&str> { + error.strip_prefix(DANGLING_HARNESS_PREFIX) +} + +/// Convert a harness-resolution error to a user-facing sentence. Dangling +/// harness ids become an actionable message; other errors pass through. +pub(crate) fn user_facing_harness_error(error: &str) -> String { + match dangling_harness_id(error) { + Some(id) => format!( + "harness \"{id}\" was deleted — pick a new harness for this agent or restore the harness definition" + ), + None => error.to_string(), + } +} + +/// Summary-row display for a dangling harness id: shows the *missing* id so +/// the agent list tells the same story as spawn (which refuses with the +/// sentence above), rather than silently falling back to the default command +/// as if the agent were healthy. +pub(crate) fn dangling_harness_display(id: &str) -> String { + format!("harness (deleted): {id}") +} + +/// 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 +1394,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 +1411,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] { + // `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 +1612,162 @@ 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(); + + // 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(), + }); + } + + // Phase 3: load and append custom harness definitions. + if let Some(dir) = custom_harnesses_dir { + // The loader applies collision + duplicate filtering at the boundary, + // so anything it returns is safe to surface in the catalog. Builtin + // shadowing is impossible here (check_id_collision covers builtins and + // presets); `seen_ids` guards only same-run duplicates. + for def in crate::managed_agents::custom_harnesses::load_custom_harnesses(dir) { + 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(), + }); + } + } + + // Publish the loaded-harness registry from a FRESH directory read under the + // persist mutex — never from the snapshot taken before the auth probes ran. + // A save/delete landing during Phase 2 already re-warmed the registry; a + // stale-snapshot publish here would clobber it (the just-saved harness + // would become unresolvable at spawn until the next discovery). + // + // This exact line is pinned by `discovery_publish_path_survives_mid_flight_save` + // / `..._drops_mid_flight_delete` (discovery tests), which land a save/delete + // through the pre-publish test hook below and red if this reverts to + // publishing a stale snapshot. + #[cfg(test)] + pre_publish_test_hook::run(); + crate::managed_agents::custom_harnesses::warm_harness_registry_locked(custom_harnesses_dir); + + entries +} + +/// Test-only seam: a callback invoked between discovery's directory scan and +/// its registry publish, so tests can land a `save_and_warm`/`delete_and_warm` +/// in exactly the window the stale-snapshot bug lived in — through the REAL +/// `discover_acp_runtimes_from` call path, not a hand-called seam. +#[cfg(test)] +pub(crate) mod pre_publish_test_hook { + use std::sync::{Mutex, OnceLock}; + + type Hook = Box; + + fn cell() -> &'static Mutex> { + static CELL: OnceLock>> = OnceLock::new(); + CELL.get_or_init(|| Mutex::new(None)) + } + + /// Install (or clear, with `None`) the hook. Callers must serialize via + /// `registry_test_lock` — the hook is process-global. + pub(crate) fn set(hook: Option) { + *cell().lock().unwrap_or_else(|e| e.into_inner()) = hook; + } + + pub(crate) fn run() { + if let Some(hook) = cell().lock().unwrap_or_else(|e| e.into_inner()).as_ref() { + hook(); + } + } } 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..2e44a283f4 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,460 @@ 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" + ); +} + +// ── Dangling-harness sentinel → user-facing surfaces ───────────────────────── + +/// The internal `DANGLING_HARNESS_ID:` sentinel round-trips through +/// `dangling_harness_id` and is never confused with other error strings. +#[test] +fn dangling_harness_id_extracts_id_and_rejects_other_errors() { + use super::{dangling_harness_id, DANGLING_HARNESS_PREFIX}; + assert_eq!( + dangling_harness_id(&format!("{DANGLING_HARNESS_PREFIX}my-harness")), + Some("my-harness") + ); + assert_eq!(dangling_harness_id("some other error"), None); + assert_eq!(dangling_harness_id(""), None); + // Prefix must anchor at the start — a wrapped sentinel is not a sentinel. + assert_eq!( + dangling_harness_id("cannot spawn: DANGLING_HARNESS_ID:x"), + None + ); +} + +/// Spawn surfaces the sentinel as an actionable sentence naming the id; +/// non-sentinel errors pass through untouched. +#[test] +fn user_facing_harness_error_converts_sentinel_to_sentence() { + use super::{user_facing_harness_error, DANGLING_HARNESS_PREFIX}; + let msg = user_facing_harness_error(&format!("{DANGLING_HARNESS_PREFIX}foo")); + assert!( + msg.contains("\"foo\"") && msg.contains("deleted"), + "sentence must name the missing harness, got: {msg}" + ); + assert!( + !msg.contains(DANGLING_HARNESS_PREFIX), + "raw sentinel must never reach the user, got: {msg}" + ); + assert_eq!( + user_facing_harness_error("plain failure"), + "plain failure", + "non-sentinel errors pass through" + ); +} + +/// Composed coherence test (delete → summary display → spawn sentence): after +/// a harness is deleted, the single resolver errors with the sentinel, the +/// summary path renders the *missing id* (not a silent buzz-agent fallback), +/// and the spawn path renders the actionable sentence — both halves tell the +/// same story from the same error. +#[test] +fn deleted_harness_summary_display_and_spawn_sentence_agree() { + use crate::managed_agents::custom_harnesses::{ + delete_and_warm, registry_test_lock, save_and_warm, + }; + use crate::managed_agents::{resolve_effective_harness_descriptor, GlobalAgentConfig}; + + let _lock = registry_test_lock(); + let dir = tempfile::tempdir().unwrap(); + + // Save a custom harness and pin a record to it. + let def = crate::managed_agents::custom_harnesses::HarnessDefinition { + id: "doomed".to_string(), + label: "Doomed".to_string(), + command: "doomed-bin".to_string(), + args: vec![], + env: Default::default(), + install_instructions_url: String::new(), + install_hint: String::new(), + }; + save_and_warm(dir.path(), &def, None).unwrap(); + let record = record_with(Some("doomed"), None, None); + let global = GlobalAgentConfig::default(); + assert!( + resolve_effective_harness_descriptor(&record, &[], &global).is_ok(), + "must resolve before delete" + ); + + // Delete it — the shared resolver used by BOTH spawn and summary errors. + delete_and_warm(dir.path(), "doomed").unwrap(); + let err = resolve_effective_harness_descriptor(&record, &[], &global).unwrap_err(); + + // Summary half: renders the missing id, never the default command. + let id = super::dangling_harness_id(&err).expect("resolver must emit the typed sentinel"); + let display = super::dangling_harness_display(id); + assert_eq!(display, "harness (deleted): doomed"); + assert!(!display.contains(&default_agent_command())); + + // Spawn half: renders a sentence naming the same id, no raw sentinel. + let sentence = super::user_facing_harness_error(&err); + assert!(sentence.contains("\"doomed\"") && sentence.contains("deleted")); + assert!(!sentence.contains(super::DANGLING_HARNESS_PREFIX)); +} + +// ── 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::custom_harnesses::registry_test_lock; + use crate::managed_agents::discovery::discover_acp_runtimes_from; + use std::{collections::BTreeMap, fs}; + use tempfile::tempdir; + + // Discovery's auth probes read/warm the process-global PATH and + // login-shell-PATH caches, and its final step publishes to the global + // harness registry — hold both guards so parallel tests (e.g. the + // PATH-swapping resolution tests) can't observe or absorb torn state. + // Lock order for tests that need both: path lock first, then registry. + let _path_guard = crate::managed_agents::lock_path_mutex(); + let _lock = registry_test_lock(); + let dir = tempdir().unwrap(); + // Write a custom definition with two env vars. + fs::write( + dir.path().join("env-harness.json"), + r#"{ + "id": "env-harness", + "label": "Env Harness", + "command": "env-harness-bin", + "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::custom_harnesses::registry_test_lock; + use crate::managed_agents::discovery::discover_acp_runtimes_from; + + // Same guards as above: discovery probes PATH-dependent caches and + // publishes to the global registry. + let _path_guard = crate::managed_agents::lock_path_mutex(); + let _lock = registry_test_lock(); + 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 + ); +} + +// ── Discovery publish via the PRODUCTION call path (stale-snapshot regression) ─ +// +// These drive `discover_acp_runtimes_from` itself and land a save/delete in +// the window between its directory scan and its registry publish (via the +// `pre_publish_test_hook` seam). They red if discovery's final line reverts +// to publishing its pre-probe `loaded_defs` snapshot — the original bug — +// unlike the `custom_harnesses` seam tests, which pin only the fresh-read +// contract of `warm_harness_registry_locked`. + +/// RAII guard: installs the pre-publish hook, clears it on drop (even on +/// panic) so a failing test cannot poison later ones. +struct PrePublishHookGuard; + +impl PrePublishHookGuard { + fn install(hook: Box) -> Self { + super::pre_publish_test_hook::set(Some(hook)); + PrePublishHookGuard + } +} + +impl Drop for PrePublishHookGuard { + fn drop(&mut self) { + super::pre_publish_test_hook::set(None); + } +} + +fn harness_def( + id: &str, + label: &str, + command: &str, +) -> crate::managed_agents::custom_harnesses::HarnessDefinition { + crate::managed_agents::custom_harnesses::HarnessDefinition { + id: id.to_string(), + label: label.to_string(), + command: command.to_string(), + args: vec![], + env: Default::default(), + install_instructions_url: String::new(), + install_hint: String::new(), + } +} + +/// A `save_and_warm` landing mid-discovery (after the scan, before the +/// publish) must survive discovery's registry publish — through the real +/// `discover_acp_runtimes_from` path. +#[test] +fn discovery_publish_path_survives_mid_flight_save() { + use crate::managed_agents::custom_harnesses::{ + lookup_loaded_harness_by_id, registry_test_lock, save_and_warm, + }; + use crate::managed_agents::discovery::discover_acp_runtimes_from; + + // Path lock first, then registry — discovery's probes touch the global + // PATH caches (see the definition_env tests above for the full rationale). + let _path_guard = crate::managed_agents::lock_path_mutex(); + let _lock = registry_test_lock(); + let dir = tempfile::tempdir().unwrap(); + + // Discovery scans the dir while it is EMPTY; the save lands in the + // pre-publish window. A stale-snapshot publish would clobber it. + let hook_dir = dir.path().to_path_buf(); + let _guard = PrePublishHookGuard::install(Box::new(move || { + let def = harness_def("mid-flight-save", "Mid Flight", "mid-flight-bin"); + save_and_warm(&hook_dir, &def, None).unwrap(); + assert!(lookup_loaded_harness_by_id("mid-flight-save").is_some()); + })); + + let _entries = discover_acp_runtimes_from(Some(dir.path())); + + assert!( + lookup_loaded_harness_by_id("mid-flight-save").is_some(), + "discovery's publish must re-read the directory — a stale-snapshot \ + publish clobbers a save that landed mid-discovery" + ); +} + +/// A `delete_and_warm` landing mid-discovery must stay gone after discovery's +/// publish — a stale snapshot (taken while the file existed) would resurrect it. +#[test] +fn discovery_publish_path_drops_mid_flight_delete() { + use crate::managed_agents::custom_harnesses::{ + delete_and_warm, lookup_loaded_harness_by_id, registry_test_lock, save_and_warm, + }; + use crate::managed_agents::discovery::discover_acp_runtimes_from; + + // Path lock first, then registry (same rationale as the sibling test). + let _path_guard = crate::managed_agents::lock_path_mutex(); + let _lock = registry_test_lock(); + let dir = tempfile::tempdir().unwrap(); + + // File exists at scan time — discovery's snapshot would contain it. + let def = harness_def("mid-flight-delete", "Mid Flight Del", "mid-flight-del-bin"); + save_and_warm(dir.path(), &def, None).unwrap(); + + let hook_dir = dir.path().to_path_buf(); + let _guard = PrePublishHookGuard::install(Box::new(move || { + delete_and_warm(&hook_dir, "mid-flight-delete").unwrap(); + assert!(lookup_loaded_harness_by_id("mid-flight-delete").is_none()); + })); + + let _entries = discover_acp_runtimes_from(Some(dir.path())); + + assert!( + lookup_loaded_harness_by_id("mid-flight-delete").is_none(), + "discovery's publish must not resurrect a harness deleted mid-discovery" + ); +} diff --git a/desktop/src-tauri/src/managed_agents/env_vars.rs b/desktop/src-tauri/src/managed_agents/env_vars.rs index c652dae427..592a5cbbd9 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 d213142352..b0e86f8edb 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; pub(crate) mod effective_config; mod env_vars; @@ -60,7 +61,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 fb45cc9a4e..c5480b2479 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); @@ -113,6 +241,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 @@ -195,6 +334,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 ──────────────────────────────────────────────────────────── @@ -270,7 +417,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![]; }; @@ -1206,10 +1359,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 79ee8df1ac..32b3b328e9 100644 --- a/desktop/src-tauri/src/managed_agents/runtime.rs +++ b/desktop/src-tauri/src/managed_agents/runtime.rs @@ -36,7 +36,7 @@ mod process; #[cfg(test)] use process::{ buzz_marker_entry, name_matches_interpreter, name_matches_known_binary, - terminate_runtime_receipt_with, + terminate_runtime_receipt_with, valid_agent_runtime_receipt_with, }; pub(crate) use process::{ current_instance_id, process_belongs_to_us, process_has_buzz_marker, process_is_running, @@ -63,6 +63,8 @@ use instance_reaper::{buffer_contains_identifier, is_desktop_binary}; // Exact-path harness sweep lives in runtime/sweep.rs (re-exported above). mod lifecycle; +#[cfg(test)] +use lifecycle::kill_stale_tracked_processes_with; pub use lifecycle::{kill_stale_tracked_processes, sync_managed_agent_processes}; /// Classify an agent's persona against the live catalog for the Agents-menu @@ -130,6 +132,7 @@ pub fn build_managed_agent_summary( record: &ManagedAgentRecord, runtimes: &HashMap, personas: &[crate::managed_agents::types::AgentDefinition], + global_config: &crate::managed_agents::GlobalAgentConfig, ) -> Result { use crate::managed_agents::BackendKind; @@ -233,12 +236,15 @@ pub fn build_managed_agent_summary( // stamped at spawn. This catches out-of-band adapter changes (manual // npm install/downgrade) that Phase-1 auto-restart doesn't cover. The // cache is read-only here — no subprocess is spawned. + // + // Global config drives both the restart-drift hash and descriptor env + // layering below — the caller loads it once and passes it in, so + // list-style callers pay one disk read per call rather than one per record. + let needs_restart = pair_key .as_ref() .and_then(|key| runtimes.get(key).map(|runtime| (key, runtime))) .is_some_and(|(key, runtime)| { - let global_for_hash = - crate::managed_agents::load_global_agent_config(app).unwrap_or_default(); let teams_for_hash = crate::managed_agents::load_teams(app).unwrap_or_default(); let hash_drift = runtime.spawn_config_hash != crate::managed_agents::spawn_hash::spawn_config_hash( @@ -246,7 +252,7 @@ pub fn build_managed_agent_summary( personas, &teams_for_hash, &key.relay_url, - &global_for_hash, + global_config, ); let availability_drift = super::availability_drift( runtime.adapter_availability.as_ref(), @@ -261,11 +267,29 @@ pub fn build_managed_agent_summary( restart_eligible(persona_orphaned, 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). + let descriptor = crate::managed_agents::resolve_effective_harness_descriptor( + record, + personas, + global_config, + ) + .unwrap_or_else(|e| { + // Dangling harness — surface the missing id so the UI tells the same + // story as spawn (which refuses with a sentence), rather than silently + // showing the default-command fallback as if the agent were healthy. + let cmd = match crate::managed_agents::dangling_harness_id(&e) { + Some(id) => crate::managed_agents::dangling_harness_display(id), + None => 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(); @@ -274,12 +298,13 @@ pub fn build_managed_agent_summary( pubkey: record.pubkey.clone(), name: record.name.clone(), persona_id: record.persona_id.clone(), + runtime: record.runtime.clone(), 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, @@ -461,6 +486,24 @@ pub fn spawn_agent_child( ) .require_resolved()?; + // 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. + // Like the orphan refusal above, this runs before any side effect so a refused + // spawn leaves no trace. + let descriptor = + crate::managed_agents::resolve_effective_harness_descriptor(record, &personas, &global) + .map_err(|e| { + format!( + "cannot spawn agent {}: {}", + record.pubkey, + crate::managed_agents::user_facing_harness_error(&e) + ) + })?; + let effective_command = &descriptor.command; + let agent_args = &descriptor.args; + let log_path = super::managed_agent_runtime_log_path(app, &runtime_key)?; append_log_marker( &log_path, @@ -476,11 +519,9 @@ pub fn spawn_agent_child( let stderr = stdout .try_clone() .map_err(|error| format!("failed to clone log handle: {error}"))?; - let effective_command = super::record_agent_command(record, &personas); - let agent_args = normalize_agent_args(&effective_command, record.agent_args.clone()); 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() { @@ -497,7 +538,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()); @@ -548,7 +589,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", "*"); } @@ -574,11 +615,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) { @@ -616,6 +662,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!({ @@ -786,28 +836,15 @@ pub fn spawn_agent_child( ); } - // ── User env vars: live persona env under 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. + // ── User env vars: definition floor + global + live persona + agent overrides ── // - // 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/instance_reaper.rs b/desktop/src-tauri/src/managed_agents/runtime/instance_reaper.rs index 27fefd0303..dec8565330 100644 --- a/desktop/src-tauri/src/managed_agents/runtime/instance_reaper.rs +++ b/desktop/src-tauri/src/managed_agents/runtime/instance_reaper.rs @@ -249,10 +249,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( @@ -271,6 +268,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; }; @@ -328,9 +328,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; }; diff --git a/desktop/src-tauri/src/managed_agents/runtime/lifecycle.rs b/desktop/src-tauri/src/managed_agents/runtime/lifecycle.rs index ec64433af0..6363b15d2a 100644 --- a/desktop/src-tauri/src/managed_agents/runtime/lifecycle.rs +++ b/desktop/src-tauri/src/managed_agents/runtime/lifecycle.rs @@ -7,6 +7,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; @@ -19,8 +36,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()); diff --git a/desktop/src-tauri/src/managed_agents/runtime/orphan_sweep.rs b/desktop/src-tauri/src/managed_agents/runtime/orphan_sweep.rs index 899f47b2c6..336bbd55ce 100644 --- a/desktop/src-tauri/src/managed_agents/runtime/orphan_sweep.rs +++ b/desktop/src-tauri/src/managed_agents/runtime/orphan_sweep.rs @@ -33,7 +33,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(); @@ -47,7 +51,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::super::remove_agent_pid_file(app, pubkey); } } @@ -55,7 +59,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::super::remove_agent_runtime_receipt(app, &receipt.key); } } @@ -103,6 +107,17 @@ const _: () = assert!(std::mem::size_of::() == 136); #[cfg(target_os = "macos")] pub(super) const PROC_PIDTBSDINFO: libc::c_int = 3; +// ── Sweep ownership rule ────────────────────────────────────────────────────── +// +// The `BUZZ_MANAGED_AGENT` env marker is the SOLE authoritative ownership +// proof for sweep/receipt decisions. Do NOT name-gate via +// `process_belongs_to_us` here — custom harnesses use arbitrary binary names +// and a name-gated predicate would silently leak their orphans (the old Linux +// AND-gate bug). `process_belongs_to_us` remains in use only as a cheap +// pre-check on paths that already know the binary (see runtime/stop.rs). +// On Windows no `/proc`-based sweep runs, so `process_has_buzz_marker` +// always returns `false`. + /// 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 @@ -127,11 +142,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( @@ -149,6 +160,8 @@ pub(crate) fn sweep_system_agent_processes(instance_id: &str, skip_pids: &[u32]) if info.pbi_uid != my_uid { continue; } + // Custom harnesses don't match KNOWN_AGENT_BINARIES by name; the + // BUZZ_MANAGED_AGENT env marker is the authoritative ownership proof. if !process_has_buzz_marker(upid, instance_id) { continue; } @@ -200,7 +213,9 @@ 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 rule as macOS: the marker is the authoritative gate. + // Fixes custom-harness orphan cleanup on Linux. + if !process_has_buzz_marker(upid, instance_id) { continue; } // Live descendants of a tracked harness are exempt — see sweep::is_live_descendant_*. @@ -284,9 +299,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( @@ -304,6 +316,8 @@ pub(crate) fn collect_same_instance_orphans( if info.pbi_uid != my_uid { continue; } + // Custom harnesses don't match KNOWN_AGENT_BINARIES by name; the + // BUZZ_MANAGED_AGENT env marker is the authoritative ownership proof. if !process_has_buzz_marker(upid, instance_id) { continue; } @@ -350,7 +364,9 @@ 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 rule as macOS: the marker is the authoritative gate. + // Fixes custom-harness orphan cleanup on Linux. + if !process_has_buzz_marker(upid, instance_id) { continue; } // Live descendants of a tracked harness are exempt — see sweep::is_live_descendant_*. diff --git a/desktop/src-tauri/src/managed_agents/runtime/process.rs b/desktop/src-tauri/src/managed_agents/runtime/process.rs index 335080a3cb..37eb5659a4 100644 --- a/desktop/src-tauri/src/managed_agents/runtime/process.rs +++ b/desktop/src-tauri/src/managed_agents/runtime/process.rs @@ -383,6 +383,25 @@ pub(crate) fn valid_agent_runtime_receipt( path: &std::path::Path, receipt: &super::super::ManagedAgentRuntimeReceipt, instance_id: &str, +) -> bool { + valid_agent_runtime_receipt_with( + path, + receipt, + instance_id, + process_is_running, + process_has_buzz_marker, + ) +} + +/// Injectable version of `valid_agent_runtime_receipt` for testing. +/// `is_running(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::super::ManagedAgentRuntimeReceipt, + instance_id: &str, + is_running: 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) @@ -393,9 +412,12 @@ 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. Marker-only ownership: custom-harness + // binaries (not in KNOWN_AGENT_BINARIES) must not be rejected by a + // name gate — see the sweep ownership rule in runtime/orphan_sweep.rs. + && has_marker(receipt.pid, &receipt.desktop_instance_id) } pub(super) fn terminate_runtime_receipt_with( diff --git a/desktop/src-tauri/src/managed_agents/runtime/tests.rs b/desktop/src-tauri/src/managed_agents/runtime/tests.rs index 55fa8efba7..8deb0c4da9 100644 --- a/desktop/src-tauri/src/managed_agents/runtime/tests.rs +++ b/desktop/src-tauri/src/managed_agents/runtime/tests.rs @@ -708,14 +708,30 @@ fn grandchild_inherits_pgid_of_process_group_leader() { // spawns an intermediate child which in turn spawns a grandchild. // This mirrors the real tree: buzz-acp → goose → buzz-dev-mcp. // - // The intermediate `sh` uses exec to replace itself with another sh - // that backgrounds the grandchild, so the grandchild's ppid is the - // intermediate (not the harness). + // The intermediate `sh` backgrounds the grandchild and echoes its PID, + // so the grandchild's ppid is the intermediate (not the harness). + // + // The trailing `sleep 10` keeps the harness (the process group leader) + // alive through the assertions below: without it the harness exits as + // soon as the intermediate echoes, and under parallel test load it can + // be reaped before `getpgid(harness_pid)` runs (observed flake — + // getpgid returned -1). The group is killed in cleanup, so the sleep + // never runs to term. + // + // Absolute `/bin/sh` and `/bin/sleep` rather than bare names: parallel + // tests holding `lock_path_mutex` legitimately swap PATH to a tempdir, + // and this test doesn't need the lock — but a PATH lookup during the + // swap window fails with NotFound, and a child spawned during it + // inherits the poisoned PATH for its lifetime, so the script's inner + // lookups must be absolute too (observed flake). let mut harness = { - let mut cmd = Command::new("sh"); - cmd.args(["-c", "sh -c 'sleep 10 & echo $!' & wait $!"]) - .stdout(std::process::Stdio::piped()) - .process_group(0); + let mut cmd = Command::new("/bin/sh"); + cmd.args([ + "-c", + "/bin/sh -c '/bin/sleep 10 & echo $!' & wait $!; /bin/sleep 10", + ]) + .stdout(std::process::Stdio::piped()) + .process_group(0); cmd.spawn().expect("spawn harness") }; @@ -799,9 +815,15 @@ fn own_group_grandchild_detected_by_ancestor_walk() { // The test process is the "harness". Spawn an intermediate with its own // process group (mirrors the node shim). It backgrounds a grandchild // (sleep 30) and prints the grandchild PID so we can inspect it. + // + // Absolute `/bin/sh` and `/bin/sleep` — no PATH lookups anywhere in this + // tree. Parallel tests holding `lock_path_mutex` legitimately swap PATH to + // a tempdir; the outer spawn during that window fails with NotFound, and a + // child spawned during it inherits the poisoned PATH for its lifetime, so + // inner lookups must be absolute too (observed flake). let mut intermediate = { - let mut cmd = Command::new("sh"); - cmd.args(["-c", "sleep 30 & echo $!; wait"]) + let mut cmd = Command::new("/bin/sh"); + cmd.args(["-c", "/bin/sleep 30 & echo $!; wait"]) .stdout(std::process::Stdio::piped()) .process_group(0); cmd.spawn().expect("spawn intermediate") @@ -1019,6 +1041,244 @@ fn invalid_pubkey_resolves_no_pair_key() { assert!(super::resolve_workspace_pair_key("not-a-key", "", "wss://one.example").is_none()); } +// ── Custom-harness orphan sweep coverage ───────────────────────────────────── +// +// The sweep/receipt ownership gate 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 gate on `process_has_buzz_marker` alone; the receipt path +// is verified below via `valid_agent_runtime_receipt_with` (injectable), +// which no longer takes a name-check predicate at all — reinstating an +// AND-gate would be a signature change these tests would catch. + +// ── 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: custom binary (not in KNOWN_AGENT_BINARIES), 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, _iid| true, // has_marker: true (custom binary — no name gate) + ); + 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, _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, _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. + // + // Absolute `/usr/bin/true` on unix (present on both macOS and Linux): + // parallel tests holding `lock_path_mutex` swap PATH to a tempdir, and a + // bare `true` lookup during that window fails with NotFound (observed + // flake). Windows keeps the PATH lookup — no test there swaps PATH. + #[cfg(unix)] + let program = "/usr/bin/true"; + #[cfg(windows)] + let program = "true"; + let child = Command::new(program) + .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) +} + // ── restart_eligible tests ────────────────────────────────────────────── #[test] diff --git a/desktop/src-tauri/src/managed_agents/spawn_hash.rs b/desktop/src-tauri/src/managed_agents/spawn_hash.rs index 8cf0014e36..e2884bfa15 100644 --- a/desktop/src-tauri/src/managed_agents/spawn_hash.rs +++ b/desktop/src-tauri/src/managed_agents/spawn_hash.rs @@ -31,7 +31,6 @@ use super::{ effective_config::{resolve_effective_config, EffectiveConfigResult}, known_acp_runtime, normalize_agent_args, persona_events::preview_prospective_persona_snapshot, - resolve_effective_agent_env, types::{AgentDefinition, ManagedAgentRecord, TeamRecord}, GlobalAgentConfig, }; @@ -66,29 +65,42 @@ pub(crate) fn spawn_config_hash( // restart would actually run. Idempotent, so the spawn-time stamp // (post-snapshot record) and later recomputes (persisted record) agree // when nothing changed. The persona env itself reaches the hash through - // `resolve_effective_agent_env` below; `persona_source_version` is set on + // the descriptor's layered env below; `persona_source_version` is set on // the clone but is not a hash input. let record = preview_prospective_persona_snapshot(record, personas); 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); 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 44ad7f4bbc..292cdbbb79 100644 --- a/desktop/src-tauri/src/managed_agents/spawn_hash/tests.rs +++ b/desktop/src-tauri/src/managed_agents/spawn_hash/tests.rs @@ -561,3 +561,137 @@ fn linked_instance_prompt_model_provider_resolve_from_one_call() { "linked instance prompt must resolve from the live definition, not stale record bytes" ); } + +// ── 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::{ + registry_test_lock, warm_harness_registry_from_dir, + }; + use std::fs; + use tempfile::tempdir; + + // The loaded-harness registry is process-global: a parallel test re-warming + // it between the two hash computations makes both resolve to no-definition + // and h1 == h2 (observed on Windows CI). + let _lock = registry_test_lock(); + 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::{ + registry_test_lock, warm_harness_registry_from_dir, + }; + use std::fs; + use tempfile::tempdir; + + // Serialize against parallel registry re-warms (see the args test above). + let _lock = registry_test_lock(); + 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::{ + registry_test_lock, warm_harness_registry_from_dir, + }; + use std::fs; + use tempfile::tempdir; + + // Serialize against parallel registry re-warms (see the args test above). + let _lock = registry_test_lock(); + 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 cce2cd4039..1226ee50e6 100644 --- a/desktop/src-tauri/src/managed_agents/types.rs +++ b/desktop/src-tauri/src/managed_agents/types.rs @@ -465,6 +465,10 @@ pub struct ManagedAgentSummary { pub pubkey: String, pub name: String, pub persona_id: Option, + /// The record's harness/runtime id (mirror of `ManagedAgentRecord.runtime`). + /// Lets the UI count agents referencing a harness definition (e.g. in the + /// delete-confirmation flow). `None` = inherit from the linked persona. + pub runtime: Option, pub team_id: Option, pub relay_url: String, pub acp_command: String, @@ -580,6 +584,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, @@ -611,6 +628,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..122c872e54 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, @@ -254,8 +283,9 @@ export function useBackendProvidersQuery(options?: { enabled?: boolean }) { }); } -export function usePersonasQuery() { +export function usePersonasQuery(options?: { enabled?: boolean }) { return useQuery({ + enabled: options?.enabled ?? true, queryKey: personasQueryKey, queryFn: listPersonas, staleTime: 30_000, 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/DoctorSettingsPanel.tsx b/desktop/src/features/settings/ui/DoctorSettingsPanel.tsx index 894d88bd22..96127876a8 100644 --- a/desktop/src/features/settings/ui/DoctorSettingsPanel.tsx +++ b/desktop/src/features/settings/ui/DoctorSettingsPanel.tsx @@ -10,6 +10,7 @@ import { useInstallAcpRuntimeMutation, } from "@/features/agents/hooks"; import { ProfileAvatar } from "@/features/profile/ui/ProfileAvatar"; +import { RuntimeIcon } from "@/features/onboarding/ui/RuntimeIcon"; import type { AcpAuthMethod, AcpRuntimeCatalogEntry } from "@/shared/api/types"; import { getInstallErrorMessage } from "@/shared/lib/installError"; import { cn } from "@/shared/lib/cn"; @@ -61,6 +62,25 @@ function runtimeInstallGuideLabel(runtime: AcpRuntimeCatalogEntry) { } function RuntimeLogo({ runtime }: { runtime: AcpRuntimeCatalogEntry }) { + // Presets deliberately emit an empty avatar_url (no remote or user-supplied + // icon URLs), so route them through RuntimeIcon — the same component the + // preset gallery uses — which owns the PRESET_LOGOS map, the per-logo + // contrast treatments (omp needs a dark chip, grok a light one), and the + // terminal-glyph fallback for logo-less presets like Cursor (brand assets + // not licensed for bundling). Keying on `source` rather than logo presence + // keeps both surfaces identical for every preset. Builtins keep the + // ProfileAvatar path below. + if (runtime.source === "preset") { + return ( + + + + ); + } + const avatarUrl = RUNTIME_LOGO_URLS[runtime.id] ?? runtime.avatarUrl; return ( diff --git a/desktop/src/features/settings/ui/HarnessManagementCard.tsx b/desktop/src/features/settings/ui/HarnessManagementCard.tsx new file mode 100644 index 0000000000..758bf1ebbd --- /dev/null +++ b/desktop/src/features/settings/ui/HarnessManagementCard.tsx @@ -0,0 +1,669 @@ +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, + useManagedAgentsQuery, + usePersonasQuery, + 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 { + commaArgError, + type CustomFormValues, + definitionFromFormValues, + formValuesFromCatalogEntry, + idFromLabel, +} from "./harnessFormLogic"; +import { + customEntries as getCustomEntries, + deleteConfirmState, + 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 ─────────────────────────────────────────────────────── + +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); + // Mirror the backend comma-in-args rejection so the user gets an inline + // error naming the offending argument before the round-trip. + const commaError = commaArgError(form.args); + if (commaError) { + setError(commaError); + return; + } + try { + await save.mutateAsync({ + definition: definitionFromFormValues(form), + 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) +

+ +
+ +
+

+ Install hint{" "} + (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(); + // Blast-radius data for the delete confirmation — only fetched while the + // confirmation is open, so the row list doesn't poll agents. Confirm stays + // disabled until both queries settle (deleteConfirmState) so a quick click + // can't beat the "N agents will stop launching" warning. + const agentsQuery = useManagedAgentsQuery({ enabled: confirmingDelete }); + const personasQuery = usePersonasQuery({ enabled: confirmingDelete }); + const confirmState = deleteConfirmState( + entry.id, + entry.label, + agentsQuery, + personasQuery, + ); + + 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 ? ( + <> + + + + ) : ( + + )} +
+
+ {confirmingDelete ? ( +

+ {confirmState.message} +

+ ) : null} + {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..d236f827e1 --- /dev/null +++ b/desktop/src/features/settings/ui/harnessFormLogic.test.mjs @@ -0,0 +1,291 @@ +import assert from "node:assert/strict"; +import test from "node:test"; + +import { + idFromLabel, + buildEnvRecord, + filterArgs, + envPairsFromRecord, + formValuesFromCatalogEntry, + definitionFromFormValues, + commaArgError, +} 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); +}); + +// ── edit round-trip (entry → form → definition) ────────────────────────────── + +/** A fully-populated definition as it appears on a catalog entry. */ +const FULL_ENTRY = { + id: "my-harness", + label: "My Harness", + command: "my-harness", + defaultArgs: ["acp", "--verbose"], + definitionEnv: { FOO: "bar", BAZ: "qux" }, + installInstructionsUrl: "https://example.com/docs", + installHint: "npm install -g my-harness", +}; + +test("formValuesFromCatalogEntry_fullEntry_populatesEveryField", () => { + const form = formValuesFromCatalogEntry(FULL_ENTRY); + assert.equal(form.id, "my-harness"); + assert.equal(form.label, "My Harness"); + assert.equal(form.command, "my-harness"); + assert.deepEqual(form.args, ["acp", "--verbose"]); + assert.deepEqual(form.env, [ + { key: "FOO", value: "bar" }, + { key: "BAZ", value: "qux" }, + ]); + assert.equal(form.installInstructionsUrl, "https://example.com/docs"); + assert.equal(form.installHint, "npm install -g my-harness"); +}); + +test("editRoundTrip_openThenSaveWithoutChanges_losesNothing", () => { + // Regression for the installHint edit-loss bug: opening the edit form and + // immediately saving must reproduce the original definition verbatim. + const definition = definitionFromFormValues( + formValuesFromCatalogEntry(FULL_ENTRY), + ); + assert.deepEqual(definition, { + id: FULL_ENTRY.id, + label: FULL_ENTRY.label, + command: FULL_ENTRY.command, + args: FULL_ENTRY.defaultArgs, + env: FULL_ENTRY.definitionEnv, + installInstructionsUrl: FULL_ENTRY.installInstructionsUrl, + installHint: FULL_ENTRY.installHint, + }); +}); + +test("editRoundTrip_definitionKeys_matchFormSeedFields", () => { + // Structural guard: any user-editable field added to the save payload must + // also be seeded on edit, or open-edit-then-save silently erases it (the + // installHint bug). `args`/`env` map to `defaultArgs`/`definitionEnv`. + const seeded = new Set(Object.keys(formValuesFromCatalogEntry(FULL_ENTRY))); + const saved = Object.keys( + definitionFromFormValues(formValuesFromCatalogEntry(FULL_ENTRY)), + ); + for (const key of saved) { + assert.ok( + seeded.has(key), + `save payload field "${key}" not seeded on edit`, + ); + } +}); + +test("formValuesFromCatalogEntry_nullishOptionalFields_defaultsSafely", () => { + const form = formValuesFromCatalogEntry({ + id: "min", + label: "Min", + command: null, + defaultArgs: undefined, + definitionEnv: undefined, + installInstructionsUrl: "", + installHint: "", + }); + assert.equal(form.command, ""); + assert.deepEqual(form.args, []); + assert.deepEqual(form.env, []); +}); + +test("definitionFromFormValues_trimsScalarFields", () => { + const definition = definitionFromFormValues({ + id: " my-id ", + label: " Label ", + command: " cmd ", + args: ["keep", " "], + env: [{ key: " K ", value: "v" }], + installInstructionsUrl: " https://x ", + installHint: " hint ", + }); + assert.equal(definition.id, "my-id"); + assert.equal(definition.label, "Label"); + assert.equal(definition.command, "cmd"); + assert.deepEqual(definition.args, ["keep"]); + assert.deepEqual(definition.env, { K: "v" }); + assert.equal(definition.installInstructionsUrl, "https://x"); + assert.equal(definition.installHint, "hint"); +}); + +// ── commaArgError ───────────────────────────────────────────────────────────── + +test("commaArgError_noCommas_returnsNull", () => { + assert.equal(commaArgError(["acp", "--verbose", "--name", "x y"]), null); +}); + +test("commaArgError_emptyArgs_returnsNull", () => { + assert.equal(commaArgError([]), null); +}); + +test("commaArgError_commaInArg_namesOffender", () => { + const err = commaArgError(["--models", "a,b"]); + assert.ok(err !== null); + assert.ok(err.includes('"a,b"'), "error must name the offending argument"); + assert.ok(err.includes("comma"), "error must name the transport limit"); +}); + +test("commaArgError_firstOffenderReported", () => { + const err = commaArgError(["x,y", "p,q"]); + assert.ok(err?.includes('"x,y"')); +}); diff --git a/desktop/src/features/settings/ui/harnessFormLogic.ts b/desktop/src/features/settings/ui/harnessFormLogic.ts new file mode 100644 index 0000000000..8332f8ae71 --- /dev/null +++ b/desktop/src/features/settings/ui/harnessFormLogic.ts @@ -0,0 +1,158 @@ +/** + * 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() !== ""); +} + +/** Form state for the custom-harness create/edit form. */ +export 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; +} + +/** The subset of a catalog entry that seeds the edit form. */ +export interface EditableCatalogEntry { + id: string; + label: string; + command: string | null; + defaultArgs: string[] | undefined; + definitionEnv?: Record; + installInstructionsUrl: string; + installHint: string; +} + +/** Definition payload sent to `save_custom_harness`. */ +export interface HarnessDefinitionPayload { + id: string; + label: string; + command: string; + args: string[]; + env: Record; + installInstructionsUrl: string; + installHint: string; +} + +/** + * Seed form values from an existing catalog entry (edit flow). + * + * This is the single source of truth for edit initialization: every + * user-editable definition field must appear here, or an open-edit-then-save + * silently erases it. `formValuesRoundTrip` in the tests proves the + * entry → form → save-payload loop is lossless. + */ +export function formValuesFromCatalogEntry( + entry: EditableCatalogEntry, +): CustomFormValues { + return { + id: entry.id, + label: entry.label, + command: entry.command ?? "", + args: entry.defaultArgs ?? [], + env: envPairsFromRecord(entry.definitionEnv), + installInstructionsUrl: entry.installInstructionsUrl, + installHint: entry.installHint, + }; +} + +/** Build the `save_custom_harness` definition payload from form values. */ +export function definitionFromFormValues( + form: CustomFormValues, +): HarnessDefinitionPayload { + return { + 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(), + }; +} + +/** + * Client-side mirror of the backend comma-in-args rejection. + * + * Args travel to the harness through the comma-delimited + * `BUZZ_ACP_AGENT_ARGS` env transport (clap `value_delimiter = ','` on the + * buzz-acp side), so a literal comma inside one argument would silently split + * into two at spawn time. The backend validator rejects it; this mirror gives + * the user an inline error naming the offending argument before the round-trip. + * + * Returns the error message, or `null` when all args are comma-free. + */ +export function commaArgError(args: ReadonlyArray): string | null { + const offender = args.find((a) => a.includes(",")); + if (offender === undefined) return null; + return `Argument "${offender}" contains a comma — arguments are passed via a comma-delimited transport and would be split at spawn time. Use separate argument entries instead.`; +} 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..d7150f1ef3 --- /dev/null +++ b/desktop/src/features/settings/ui/harnessGalleryLogic.test.mjs @@ -0,0 +1,307 @@ +import assert from "node:assert/strict"; +import { describe, it } from "node:test"; + +import { + sortedPresetEntries, + customEntries, + isEditableEntry, + countAgentsReferencingHarness, + deleteHarnessConfirmMessage, + deleteConfirmState, +} 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" }))); + }); +}); + +// ── countAgentsReferencingHarness ───────────────────────────────────────────── + +describe("countAgentsReferencingHarness", () => { + it("counts direct record-level runtime pins", () => { + const agents = [ + { runtime: "my-harness", personaId: null }, + { runtime: "other", personaId: null }, + { runtime: "my-harness", personaId: "p1" }, + ]; + assert.equal(countAgentsReferencingHarness("my-harness", agents, []), 2); + }); + + it("counts persona-inherited references when record runtime is null", () => { + const agents = [{ runtime: null, personaId: "p1" }]; + const personas = [{ id: "p1", runtime: "my-harness" }]; + assert.equal( + countAgentsReferencingHarness("my-harness", agents, personas), + 1, + ); + }); + + it("record-level pin shadows persona runtime (no double counting, pin wins)", () => { + // Agent pinned to "other" whose persona uses "my-harness" does NOT count: + // the effective harness is the pin. + const agents = [{ runtime: "other", personaId: "p1" }]; + const personas = [{ id: "p1", runtime: "my-harness" }]; + assert.equal( + countAgentsReferencingHarness("my-harness", agents, personas), + 0, + ); + }); + + it("agents with no runtime and no persona do not count", () => { + const agents = [{ runtime: null, personaId: null }]; + assert.equal(countAgentsReferencingHarness("my-harness", agents, []), 0); + }); + + it("persona reference to a different harness does not count", () => { + const agents = [{ runtime: null, personaId: "p1" }]; + const personas = [{ id: "p1", runtime: "other" }]; + assert.equal( + countAgentsReferencingHarness("my-harness", agents, personas), + 0, + ); + }); +}); + +// ── deleteHarnessConfirmMessage ─────────────────────────────────────────────── + +describe("deleteHarnessConfirmMessage", () => { + it("plain confirmation when nothing references the harness", () => { + assert.equal( + deleteHarnessConfirmMessage("My Harness", 0), + "Delete My Harness?", + ); + }); + + it("singular copy for one referencing agent", () => { + assert.equal( + deleteHarnessConfirmMessage("My Harness", 1), + "1 agent uses this harness and will stop launching. Delete My Harness?", + ); + }); + + it("plural copy for multiple referencing agents", () => { + assert.equal( + deleteHarnessConfirmMessage("My Harness", 3), + "3 agents use this harness and will stop launching. Delete My Harness?", + ); + }); +}); + +// ── deleteConfirmState ──────────────────────────────────────────────────────── + +describe("deleteConfirmState", () => { + const settled = (data) => ({ isPending: false, isError: false, data }); + const pending = { isPending: true, isError: false, data: undefined }; + const failed = { isPending: false, isError: true, data: undefined }; + + it("disables confirm while agents query is still loading", () => { + const state = deleteConfirmState("h1", "My Harness", pending, settled([])); + assert.equal(state.canConfirm, false); + assert.match(state.message, /Checking which agents/); + }); + + it("disables confirm while personas query is still loading", () => { + const state = deleteConfirmState("h1", "My Harness", settled([]), pending); + assert.equal(state.canConfirm, false); + assert.match(state.message, /Checking which agents/); + }); + + it("query failure does not claim zero dependents", () => { + const state = deleteConfirmState("h1", "My Harness", failed, settled([])); + assert.equal(state.canConfirm, true); + assert.match(state.message, /Couldn't check/); + assert.doesNotMatch(state.message, /^Delete My Harness\?$/); + }); + + it("persona query failure also reports unknown blast radius", () => { + const state = deleteConfirmState("h1", "My Harness", settled([]), failed); + assert.equal(state.canConfirm, true); + assert.match(state.message, /Couldn't check/); + }); + + it("settled queries produce the counted warning and enable confirm", () => { + const agents = settled([ + { runtime: "h1", personaId: null }, + { runtime: null, personaId: "p1" }, + ]); + const personas = settled([{ id: "p1", runtime: "h1" }]); + const state = deleteConfirmState("h1", "My Harness", agents, personas); + assert.equal(state.canConfirm, true); + assert.equal( + state.message, + "2 agents use this harness and will stop launching. Delete My Harness?", + ); + }); + + it("settled queries with zero dependents use the plain confirmation", () => { + const state = deleteConfirmState( + "h1", + "My Harness", + settled([]), + settled([]), + ); + assert.equal(state.canConfirm, true); + assert.equal(state.message, "Delete My Harness?"); + }); +}); diff --git a/desktop/src/features/settings/ui/harnessGalleryLogic.ts b/desktop/src/features/settings/ui/harnessGalleryLogic.ts new file mode 100644 index 0000000000..d3c782c530 --- /dev/null +++ b/desktop/src/features/settings/ui/harnessGalleryLogic.ts @@ -0,0 +1,137 @@ +/** + * 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"; +} + +/** + * Count managed agents whose effective harness is the given definition id — + * either a direct record-level `runtime` pin, or inheritance from a linked + * persona whose `runtime` is that id. + * + * Drives the delete-confirmation copy: deleting a harness that agents still + * reference is allowed (blocking would turn cleanup into dependency + * untangling), but the user is told those agents will stop launching. + */ +export function countAgentsReferencingHarness( + harnessId: string, + agents: ReadonlyArray<{ runtime: string | null; personaId: string | null }>, + personas: ReadonlyArray<{ id: string; runtime: string | null }>, +): number { + const personaRuntime = new Map(personas.map((p) => [p.id, p.runtime])); + return agents.filter((agent) => { + if (agent.runtime !== null) return agent.runtime === harnessId; + if (agent.personaId === null) return false; + return personaRuntime.get(agent.personaId) === harnessId; + }).length; +} + +/** + * Confirmation copy for deleting a custom harness. Names the blast radius + * when agents still reference the definition; stays quiet when none do. + */ +export function deleteHarnessConfirmMessage( + label: string, + referencingAgents: number, +): string { + if (referencingAgents === 0) return `Delete ${label}?`; + const noun = referencingAgents === 1 ? "agent uses" : "agents use"; + return `${referencingAgents} ${noun} this harness and will stop launching. Delete ${label}?`; +} + +/** Minimal query-state shape consumed by `deleteConfirmState` — matches the + * relevant subset of a TanStack `useQuery` result so the logic stays pure. */ +export type BlastRadiusQuery = { + /** True during the initial fetch (no data yet). */ + isPending: boolean; + isError: boolean; + data: T | undefined; +}; + +export type DeleteConfirmState = { + /** False while the blast radius is still unknown — the destructive button + * must be disabled so a quick click can't beat the warning. */ + canConfirm: boolean; + message: string; +}; + +/** + * Drive the delete-confirmation UI from the two blast-radius queries. + * + * - While either query is initial-loading, confirmation is DISABLED and the + * copy says so: the promised "N agents will stop launching" warning must + * render before the destructive action is clickable. + * - On query failure, deletion stays possible (blocking cleanup on a failed + * read would be worse) but the copy states the count is unknown — it never + * silently claims zero dependents. + */ +export function deleteConfirmState( + harnessId: string, + label: string, + agents: BlastRadiusQuery< + ReadonlyArray<{ runtime: string | null; personaId: string | null }> + >, + personas: BlastRadiusQuery< + ReadonlyArray<{ id: string; runtime: string | null }> + >, +): DeleteConfirmState { + if (agents.isPending || personas.isPending) { + return { + canConfirm: false, + message: "Checking which agents use this harness…", + }; + } + if (agents.isError || personas.isError) { + return { + canConfirm: true, + message: `Couldn't check which agents use this harness — some agents may stop launching. Delete ${label}?`, + }; + } + const count = countAgentsReferencingHarness( + harnessId, + agents.data ?? [], + personas.data ?? [], + ); + return { + canConfirm: true, + message: deleteHarnessConfirmMessage(label, count), + }; +} 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 8c77f69313..e8a8b885cf 100644 --- a/desktop/src/shared/api/tauri.ts +++ b/desktop/src/shared/api/tauri.ts @@ -118,6 +118,8 @@ export type RawManagedAgent = { pubkey: string; name: string; persona_id: string | null; + // Optional: pre-feature fixtures may omit it. The record's harness/runtime id. + runtime?: string | null; team_id?: string | null; relay_url: string; acp_command: string; @@ -192,6 +194,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 = { @@ -688,6 +696,7 @@ export function fromRawManagedAgent(agent: RawManagedAgent): ManagedAgent { pubkey: agent.pubkey, name: agent.name, personaId: agent.persona_id, + runtime: agent.runtime ?? null, teamId: agent.team_id ?? null, relayUrl: agent.relay_url, acpCommand: agent.acp_command, @@ -730,7 +739,7 @@ export function fromRawManagedAgent(agent: RawManagedAgent): ManagedAgent { }; } -function fromRawAcpRuntimeCatalogEntry( +export function fromRawAcpRuntimeCatalogEntry( entry: RawAcpRuntimeCatalogEntry, ): AcpRuntimeCatalogEntry { return { @@ -753,6 +762,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 ?? {}, }; } @@ -934,6 +947,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 231996b35c..50842d0d0a 100644 --- a/desktop/src/shared/api/types.ts +++ b/desktop/src/shared/api/types.ts @@ -343,6 +343,12 @@ export type ManagedAgent = { pubkey: string; name: string; personaId: string | null; + /** + * The record's harness/runtime id (e.g. "goose", "my-custom-harness"). + * `null` means the agent inherits its harness from the linked persona. + * Used to count agents referencing a harness definition (delete confirm). + */ + runtime: string | null; teamId?: string | null; relayUrl: string; acpCommand: string; @@ -558,6 +564,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..e9a7266eff 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"; @@ -70,6 +75,8 @@ type MockManagedAgentSeed = { name: string; avatarUrl?: string | null; personaId?: string | null; + /** Harness/runtime id pin; `null` = inherit from persona (native default). */ + runtime?: string | null; status?: RawManagedAgent["status"]; channelNames?: string[]; channelIds?: string[]; @@ -175,6 +182,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; @@ -701,6 +710,8 @@ type RawManagedAgent = { pubkey: string; name: string; persona_id: string | null; + /** Record-level harness/runtime pin (`null` when inheriting from the persona). */ + runtime: string | null; relay_url: string; acp_command: string; agent_command: string; @@ -1452,6 +1463,7 @@ function cloneManagedAgent(agent: MockManagedAgent): RawManagedAgent { pubkey: agent.pubkey, name: agent.name, persona_id: agent.persona_id, + runtime: agent.runtime ?? null, relay_url: agent.relay_url, acp_command: agent.acp_command, agent_command: agent.agent_command, @@ -1985,6 +1997,9 @@ function buildSeededManagedAgent(seed: MockManagedAgentSeed): MockManagedAgent { pubkey: seed.pubkey, name: seed.name, persona_id: seed.personaId ?? null, + // Native serde always emits this key (`null` when unpinned) — the bridge + // must mirror the wire shape, not omit the key. + runtime: seed.runtime ?? null, relay_url: DEFAULT_RELAY_WS_URL, acp_command: "buzz-acp", agent_command: "goose", @@ -6957,7 +6972,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 +6993,7 @@ async function handleDiscoverAcpRuntimes( underlying_cli_path: null, node_required: false, auth_status: { status: "not_applicable" }, + source: "builtin", login_hint: undefined, }, { @@ -6995,6 +7013,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 +7033,7 @@ async function handleDiscoverAcpRuntimes( underlying_cli_path: null, node_required: false, auth_status: { status: "unknown" }, + source: "builtin", login_hint: undefined, }, { @@ -7032,10 +7052,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( @@ -7638,6 +7661,8 @@ async function handleCreateManagedAgent( pubkey, name, persona_id: args.input.personaId ?? null, + // Create never pins a harness id — the record inherits from the persona. + runtime: null, relay_url: args.input.relayUrl ?? DEFAULT_RELAY_WS_URL, acp_command: args.input.acpCommand ?? "buzz-acp", agent_command: agentCommand, @@ -10004,6 +10029,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..e66a85fac9 --- /dev/null +++ b/desktop/tests/e2e/harness-management.spec.ts @@ -0,0 +1,498 @@ +/** + * 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; + +/** Cursor preset — deliberately has NO bundled logo (brand assets not + * licensed for redistribution; see FALLBACK_ONLY_PRESETS). Must render the + * terminal glyph, never initials. */ +const CURSOR_AVAILABLE = { + id: "cursor", + label: "Cursor", + avatar_url: "", + availability: "available", + command: "cursor-agent", + binary_path: "/usr/local/bin/cursor-agent", + default_args: [], + mcp_command: null, + install_hint: "Install Cursor CLI from cursor.com.", + install_instructions_url: "https://cursor.com/cli", + 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(); + }); +}); + +// ── Preset logos in the Agent runtimes list ────────────────────────────────── + +test("Agent runtimes rows render bundled preset logos, not initials", async ({ + page, +}) => { + await installMockBridge(page, { + acpRuntimesCatalog: [ + HERMES_AVAILABLE, + OPENCLAW_NOT_INSTALLED, + CURSOR_AVAILABLE, + ], + }); + await openHarnessSettings(page); + + // Preset rows in "Agent runtimes" must show the same bundled logo the + // preset gallery uses (PRESET_LOGOS via RuntimeIcon), even though presets + // emit an empty avatar_url (the no-remote-icon security line). + for (const [id, file] of [ + ["hermes", "/harness-logos/hermes.png"], + ["openclaw", "/harness-logos/openclaw.svg"], + ] as const) { + const logo = page.getByTestId(`doctor-runtime-logo-${id}`); + await expect(logo).toBeVisible(); + await expect(logo.locator("img")).toHaveAttribute("src", file); + } + + // Cursor has no bundled logo (licensing) — it must fall through to + // RuntimeIcon's terminal glyph like the preset gallery, not initials. + const cursorLogo = page.getByTestId("doctor-runtime-logo-cursor"); + await expect(cursorLogo).toBeVisible(); + await expect(cursorLogo.locator("svg")).toBeVisible(); + await expect(cursorLogo.locator("img")).not.toBeVisible(); + await expect(cursorLogo).not.toContainText("C"); +}); + +// ── 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;