diff --git a/crates/buzz-acp/src/setup_mode.rs b/crates/buzz-acp/src/setup_mode.rs index 9fe51f415f..2f162d84f3 100644 --- a/crates/buzz-acp/src/setup_mode.rs +++ b/crates/buzz-acp/src/setup_mode.rs @@ -43,6 +43,31 @@ use nostr::EventId; use serde::{Deserialize, Serialize}; use uuid::Uuid; +// ── Availability mirror ──────────────────────────────────────────────────────── + +/// Granular install/auth state for a CLI-backed ACP harness. +/// +/// Mirrors the desktop `AcpAvailabilityStatus` enum (and the FE +/// `AcpAvailabilityStatus` type in `api/types.ts`). Carried on +/// `RequirementPayload::CliLogin` so the sentinel JSON the desktop parses +/// contains the exact wire literals the FE expects. +/// +/// buzz-acp is a separate crate and must NOT depend on desktop types — +/// this explicit mirror is the correct pattern (same as the rest of +/// `RequirementPayload` as "the Rust counterpart to desktop's `Requirement`"). +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq)] +#[serde(rename_all = "snake_case")] +pub(crate) enum AcpAvailabilityStatus { + /// Adapter + CLI both present; may still need login. + Available, + /// ACP adapter binary missing; underlying CLI may be present. + AdapterMissing, + /// CLI binary missing; ACP adapter may be present. + CliMissing, + /// Neither adapter nor CLI found. + NotInstalled, +} + use crate::{ author_allowed, config::Config, @@ -72,6 +97,11 @@ pub(crate) enum RequirementPayload { CliLogin { probe_args: Vec, setup_copy: String, + /// Granular install/auth state — determines copy and CTA routing on + /// the desktop card. `Available` means tooling is present but login + /// is needed; the other three variants mean the tooling itself is + /// missing and the probe was skipped. + availability: AcpAvailabilityStatus, }, } @@ -85,7 +115,40 @@ impl RequirementPayload { RequirementPayload::EnvKey { key } => { format!("set `{}` in Edit Agent → Environment variables", key) } - RequirementPayload::CliLogin { setup_copy, .. } => setup_copy.clone(), + RequirementPayload::CliLogin { + setup_copy, + availability, + probe_args, + } => match availability { + AcpAvailabilityStatus::Available => setup_copy.clone(), + AcpAvailabilityStatus::AdapterMissing => { + let harness = probe_args + .first() + .map(String::as_str) + .unwrap_or("the agent"); + format!( + "install the {} ACP adapter (open Doctor in Settings to diagnose)", + harness + ) + } + AcpAvailabilityStatus::CliMissing => { + let harness = probe_args + .first() + .map(String::as_str) + .unwrap_or("the agent"); + format!( + "install {} CLI (open Doctor in Settings to diagnose)", + harness + ) + } + AcpAvailabilityStatus::NotInstalled => { + let harness = probe_args + .first() + .map(String::as_str) + .unwrap_or("the agent"); + format!("install {} (open Doctor in Settings to diagnose)", harness) + } + }, } } } @@ -588,7 +651,8 @@ mod tests { "login".to_string(), "status".to_string(), ], - setup_copy: "run `codex login --with-api-key`".to_string(), + setup_copy: "run `codex login`".to_string(), + availability: AcpAvailabilityStatus::Available, }], }; let body = payload.nudge_body(); @@ -655,6 +719,7 @@ mod tests { RequirementPayload::CliLogin { probe_args: vec!["codex".to_string(), "login".to_string()], setup_copy: "run `codex login`".to_string(), + availability: AcpAvailabilityStatus::Available, }, ], }; @@ -766,4 +831,94 @@ mod tests { "replay of the same event-id must be rejected (dedup)" ); } + + // ── availability round-trip tests ───────────────────────────────────────── + // + // These tests prove the desktop→buzz-acp→sentinel path preserves the + // `availability` field. They simulate what actually happens at runtime: + // desktop serializes a `cli_login` JSON blob → buzz-acp parses it via + // `from_raw_env_value` → `nudge_body()` re-serializes into the sentinel → + // the sentinel JSON is extracted and checked for the `availability` field. + // + // This guards the prior regression where `RequirementPayload::CliLogin` + // had no `availability` field, so serde silently dropped it during + // deserialization and the desktop card never rendered. + + fn extract_sentinel_json(body: &str) -> String { + let fence_open = "```buzz:config-nudge\n"; + let fence_close = "\n```"; + let start = body + .rfind(fence_open) + .expect("sentinel open fence not found") + + fence_open.len(); + let end = body[start..] + .rfind(fence_close) + .expect("sentinel close fence not found") + + start; + body[start..end].to_string() + } + + fn make_desktop_cli_login_json(availability: &str) -> String { + // Simulate the JSON desktop's runtime.rs emits — a full SetupPayload + // with one cli_login requirement carrying the given availability state. + format!( + r#"{{"agent_name":"TestAgent","agent_pubkey":"aa","requirements":[{{"surface":"cli_login","probe_args":["claude"],"setup_copy":"run claude login","availability":"{availability}"}}]}}"# + ) + } + + #[test] + fn cli_login_availability_available_survives_sentinel_round_trip() { + let raw = make_desktop_cli_login_json("available"); + let payload = SetupPayload::from_raw_env_value(Some(raw)) + .unwrap() + .expect("must parse"); + let body = payload.nudge_body(); + let sentinel_json = extract_sentinel_json(&body); + assert!( + sentinel_json.contains(r#""availability":"available""#), + "sentinel must carry availability=available; got: {sentinel_json:?}" + ); + } + + #[test] + fn cli_login_availability_adapter_missing_survives_sentinel_round_trip() { + let raw = make_desktop_cli_login_json("adapter_missing"); + let payload = SetupPayload::from_raw_env_value(Some(raw)) + .unwrap() + .expect("must parse"); + let body = payload.nudge_body(); + let sentinel_json = extract_sentinel_json(&body); + assert!( + sentinel_json.contains(r#""availability":"adapter_missing""#), + "sentinel must carry availability=adapter_missing; got: {sentinel_json:?}" + ); + } + + #[test] + fn cli_login_availability_cli_missing_survives_sentinel_round_trip() { + let raw = make_desktop_cli_login_json("cli_missing"); + let payload = SetupPayload::from_raw_env_value(Some(raw)) + .unwrap() + .expect("must parse"); + let body = payload.nudge_body(); + let sentinel_json = extract_sentinel_json(&body); + assert!( + sentinel_json.contains(r#""availability":"cli_missing""#), + "sentinel must carry availability=cli_missing; got: {sentinel_json:?}" + ); + } + + #[test] + fn cli_login_availability_not_installed_survives_sentinel_round_trip() { + let raw = make_desktop_cli_login_json("not_installed"); + let payload = SetupPayload::from_raw_env_value(Some(raw)) + .unwrap() + .expect("must parse"); + let body = payload.nudge_body(); + let sentinel_json = extract_sentinel_json(&body); + assert!( + sentinel_json.contains(r#""availability":"not_installed""#), + "sentinel must carry availability=not_installed; got: {sentinel_json:?}" + ); + } } diff --git a/desktop/playwright.config.ts b/desktop/playwright.config.ts index 8ca25b81d2..4beec218f0 100644 --- a/desktop/playwright.config.ts +++ b/desktop/playwright.config.ts @@ -40,6 +40,7 @@ export default defineConfig({ "**/local-archive-screenshots.spec.ts", "**/agent-readiness-screenshots.spec.ts", "**/edit-agent.spec.ts", + "**/doctor-cta-screenshots.spec.ts", "**/pubkey-display-screenshots.spec.ts", "**/file-attachment.spec.ts", "**/image-attachment-gallery.spec.ts", diff --git a/desktop/scripts/check-file-sizes.mjs b/desktop/scripts/check-file-sizes.mjs index d03e67e2b1..7b3af6b0cf 100644 --- a/desktop/scripts/check-file-sizes.mjs +++ b/desktop/scripts/check-file-sizes.mjs @@ -116,7 +116,16 @@ const overrides = new Map([ // +2 readiness integration tests for flat-DATABRICKS_HOST canonicalization fix. // +1 cargo fmt whitespace reformat (readiness.rs closures inline after rebase). // +2 unit tests for cli_login_requirements resolve_command integration (DMG PATH fix). - ["src-tauri/src/managed_agents/readiness.rs", 1215], + // Doctor-CTA: reworked cli_login_requirements to carry AcpAvailabilityStatus, + // skip login probe for not-installed/adapter-missing/cli-missing states, and + // added 4 unit tests covering each arm. Load-bearing discoverability fix. + // Updated existing codex_not_ready test to use make_cli_runtime stub. + // +4 lines: #1640 persona-env-vars-refresh rebase added availability-classification + // growth in the live-persona env merge path. Feature plumbing, not generic debt. + // Windows-CI portability: replaced POSIX true/false probes with current_exe() + // stand-in + present_binary_str()/static_commands() helpers (+29 lines). + // Tests now pass on windows-latest CI shard without POSIX shell utilities. + ["src-tauri/src/managed_agents/readiness.rs", 1403], // 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 diff --git a/desktop/src-tauri/src/managed_agents/discovery.rs b/desktop/src-tauri/src/managed_agents/discovery.rs index 09ac897f3c..059ab988f1 100644 --- a/desktop/src-tauri/src/managed_agents/discovery.rs +++ b/desktop/src-tauri/src/managed_agents/discovery.rs @@ -678,7 +678,7 @@ pub fn login_shell_path() -> Option { .clone() } -fn find_command(command: &str) -> Option { +pub(crate) fn find_command(command: &str) -> Option { resolve_command(command) } @@ -701,7 +701,7 @@ pub fn missing_command_message(command: &str, role: &str) -> String { ) } -fn classify_runtime( +pub(crate) fn classify_runtime( adapter_result: Option<(&str, PathBuf)>, underlying_cli: Option<&str>, underlying_cli_found: bool, diff --git a/desktop/src-tauri/src/managed_agents/readiness.rs b/desktop/src-tauri/src/managed_agents/readiness.rs index c5f3ef440c..b69a97b57d 100644 --- a/desktop/src-tauri/src/managed_agents/readiness.rs +++ b/desktop/src-tauri/src/managed_agents/readiness.rs @@ -45,9 +45,11 @@ use serde::{Deserialize, Serialize}; use crate::managed_agents::{ agent_env::baked_build_env, config_bridge::read_goose_file_config, - discovery::{known_acp_runtime, resolve_command, KnownAcpRuntime}, + discovery::{ + classify_runtime, find_command, known_acp_runtime, resolve_command, KnownAcpRuntime, + }, env_vars::merged_user_env, - types::{ManagedAgentRecord, PersonaRecord}, + types::{AcpAvailabilityStatus, ManagedAgentRecord, PersonaRecord}, }; // ── EffectiveAgentEnv ───────────────────────────────────────────────────────── @@ -149,8 +151,13 @@ pub enum Requirement { /// Arguments for the login-status probe (e.g. `["claude", "auth", "status"]`). probe_args: Vec, /// Human-readable instruction for completing the login - /// (e.g. `"run \`codex login --with-api-key\`"`). + /// (e.g. `"run \`codex login\`"`). setup_copy: String, + /// Granular install/auth state for this runtime — distinguishes + /// "not installed" from "logged out" from "adapter missing". + /// Carried to the FE so the nudge card can show the right message + /// and route to Doctor with accurate context. + availability: AcpAvailabilityStatus, }, } @@ -242,11 +249,9 @@ fn collect_missing_requirements( "claude" => cli_login_requirements( &["claude", "auth", "status"], "complete Claude Code authentication by running the Claude CLI", + rt, ), - "codex" => cli_login_requirements( - &["codex", "login", "status"], - "run `codex login --with-api-key`", - ), + "codex" => cli_login_requirements(&["codex", "login", "status"], "run `codex login`", rt), _ => vec![], } } @@ -422,39 +427,72 @@ fn goose_requirements( /// Requirements for CLI-login runtimes (claude, codex). /// -/// We probe the CLI's login-status command synchronously. These probes are +/// Probes the CLI's login-status command synchronously. These probes are /// fast (<300ms) and the results are memoized by the caller for the session /// lifetime if desired. -fn cli_login_requirements(probe_args: &[&str], setup_copy: &str) -> Vec { - // Resolve the binary through the full PATH-search + login-shell path so - // the probe works in a packaged macOS DMG where the GUI PATH lacks - // npm/homebrew directories (where `claude` / `codex` typically live). - // - // If the binary genuinely does not exist, stay NotReady — the user needs - // to install it. If it exists but is not on the GUI PATH, resolve_command - // finds it via the login-shell fallback. - let Some(binary_path) = resolve_command(probe_args[0]) else { - // Binary not found → not installed → NotReady. - return vec![Requirement::CliLogin { - probe_args: probe_args.iter().map(|s| s.to_string()).collect(), - setup_copy: setup_copy.to_string(), - }]; - }; - - // Run the probe at the resolved absolute path so the GUI-PATH gap is bypassed. - let logged_in = std::process::Command::new(&binary_path) - .args(&probe_args[1..]) - .output() - .map(|o| o.status.success()) +/// +/// Computes a granular `AcpAvailabilityStatus` by running the same +/// classifier as Doctor (`classify_runtime`) before deciding whether to +/// probe — this lets the nudge card distinguish "not installed" from +/// "adapter missing" from "logged out" without a new backend probe. +fn cli_login_requirements( + probe_args: &[&str], + setup_copy: &str, + runtime: &KnownAcpRuntime, +) -> Vec { + // Resolve each adapter command to find the ACP adapter binary. + let adapter_result = runtime + .commands + .iter() + .find_map(|cmd| find_command(cmd).map(|path| (*cmd, path))); + + // Check whether the underlying CLI itself (e.g. "claude", "codex") is on PATH. + let underlying_cli_found = runtime + .underlying_cli + .map(|cli| find_command(cli).is_some()) .unwrap_or(false); - if logged_in { - vec![] - } else { - vec![Requirement::CliLogin { + let (availability, _cmd, _path) = + classify_runtime(adapter_result, runtime.underlying_cli, underlying_cli_found); + + match availability { + AcpAvailabilityStatus::Available => { + // Both adapter and CLI are present — probe login status. + // Resolve via the full login-shell PATH so the probe works in a + // packaged macOS DMG where the GUI PATH lacks npm/homebrew. + let Some(binary_path) = resolve_command(probe_args[0]) else { + // Unexpectedly not resolvable (race or PATH edge case). + return vec![Requirement::CliLogin { + probe_args: probe_args.iter().map(|s| s.to_string()).collect(), + setup_copy: setup_copy.to_string(), + availability: AcpAvailabilityStatus::Available, + }]; + }; + + let logged_in = std::process::Command::new(&binary_path) + .args(&probe_args[1..]) + .output() + .map(|o| o.status.success()) + .unwrap_or(false); + + if logged_in { + vec![] + } else { + vec![Requirement::CliLogin { + probe_args: probe_args.iter().map(|s| s.to_string()).collect(), + setup_copy: setup_copy.to_string(), + availability: AcpAvailabilityStatus::Available, + }] + } + } + // Tooling is not fully installed — emit CliLogin with the precise + // state so the nudge card can show the right message. Skip the probe + // (can't run a missing or misconfigured CLI). + other => vec![Requirement::CliLogin { probe_args: probe_args.iter().map(|s| s.to_string()).collect(), setup_copy: setup_copy.to_string(), - }] + availability: other, + }], } } @@ -802,14 +840,12 @@ mod tests { #[test] fn codex_not_ready_copy_does_not_mention_openai_api_key() { - // codex uses its own credential store via `codex login --with-api-key`. + // codex uses its own credential store via `codex login` (OAuth or API key). // The nudge copy must NOT say "set OPENAI_API_KEY". - // We test the static not-logged-in path by constructing the requirement - // directly (the binary may or may not be installed in CI). - let reqs = cli_login_requirements( - &["codex", "login", "status"], - "run `codex login --with-api-key`", - ); + // Use a not-installed runtime so the requirement is always emitted + // regardless of whether codex is on the test machine's PATH. + let rt = make_cli_runtime(&["__buzz_nonexistent_adapter_xyz789__"], None); + let reqs = cli_login_requirements(&["codex", "login", "status"], "run `codex login`", &rt); // Whether codex is installed or not, the copy (if any) must not mention OPENAI_API_KEY. for req in &reqs { if let Requirement::CliLogin { setup_copy, .. } = req { @@ -827,14 +863,71 @@ mod tests { // ── cli_login_requirements: resolve_command integration ───────────── + /// Construct a minimal `KnownAcpRuntime` stub for testing cli_login_requirements. + /// `commands` are the adapter binaries; `underlying_cli` is the CLI name. + fn make_cli_runtime( + commands: &'static [&'static str], + underlying_cli: Option<&'static str>, + ) -> KnownAcpRuntime { + KnownAcpRuntime { + id: "test-cli-runtime", + label: "Test CLI", + commands, + aliases: &[], + avatar_url: "", + mcp_command: None, + mcp_hooks: false, + underlying_cli, + cli_install_commands: &[], + adapter_install_commands: &[], + install_instructions_url: "", + cli_install_hint: "", + adapter_install_hint: "", + skill_dir: None, + supports_acp_model_switching: false, + config_file_path: None, + config_file_format: None, + model_env_var: None, + provider_env_var: None, + provider_locked: false, + default_env: &[], + supports_acp_native_config: false, + thinking_env_var: None, + max_tokens_env_var: None, + context_limit_env_var: None, + required_normalized_fields: &[], + } + } + + /// Returns the absolute path of the currently-running test binary as a + /// `&'static str`. Host-portable stand-in for a "present" binary: + /// the path is absolute so `find_command` resolves it via `path.exists()` + /// rather than searching `PATH`, and the file always exists on the host. + /// + /// The tiny allocation is intentionally leaked — this runs at most once per + /// test process and the process exits immediately after tests complete. + fn present_binary_str() -> &'static str { + let path = std::env::current_exe().expect("current_exe must be available in tests"); + Box::leak(path.to_string_lossy().into_owned().into_boxed_str()) + } + + /// Leak a runtime slice of `'static` strs for use in `make_cli_runtime`. + fn static_commands(commands: Vec<&'static str>) -> &'static [&'static str] { + Box::leak(commands.into_boxed_slice()) + } + #[test] fn cli_login_requirements_missing_binary_is_not_ready() { - // A binary that cannot possibly exist on any system → binary not found - // → resolve_command returns None → function must return CliLogin - // requirement (NotReady), not panic or return Ready. + // Both adapter and underlying CLI are nonexistent → NotInstalled state + // → must return a CliLogin requirement with availability=NotInstalled. + let rt = make_cli_runtime( + &["__buzz_nonexistent_adapter_abc123__"], + Some("__buzz_nonexistent_cli_abc123__"), + ); let reqs = cli_login_requirements( &["__buzz_nonexistent_binary_abc123__", "status"], "install the tool first", + &rt, ); assert!( !reqs.is_empty(), @@ -845,31 +938,116 @@ mod tests { "requirement must be CliLogin; got {:?}", reqs[0] ); + if let Requirement::CliLogin { + ref availability, .. + } = reqs[0] + { + assert_eq!( + *availability, + crate::managed_agents::AcpAvailabilityStatus::NotInstalled, + "both missing → NotInstalled" + ); + } + } + + #[test] + fn cli_login_requirements_adapter_missing_emits_adapter_missing() { + // Underlying CLI present (use the running test binary as a portable + // stand-in — it's always present and resolves via absolute path), + // adapter absent. + // → AdapterMissing state → no probe run → CliLogin{AdapterMissing}. + let exe = present_binary_str(); + let rt = make_cli_runtime(&["__buzz_nonexistent_adapter_xyz789__"], Some(exe)); + let reqs = cli_login_requirements(&[exe, "--list"], "install the adapter", &rt); + assert!( + !reqs.is_empty(), + "adapter missing must produce a CliLogin requirement" + ); + if let Requirement::CliLogin { + ref availability, .. + } = reqs[0] + { + assert_eq!( + *availability, + crate::managed_agents::AcpAvailabilityStatus::AdapterMissing, + "adapter absent, CLI present → AdapterMissing" + ); + } + } + + #[test] + fn cli_login_requirements_cli_missing_emits_cli_missing() { + // Adapter present (use the running test binary as a portable stand-in), + // underlying CLI absent. + // → CliMissing state → no probe run → CliLogin{CliMissing}. + let exe = present_binary_str(); + let rt = make_cli_runtime( + static_commands(vec![exe]), // adapter found via absolute path + Some("__buzz_nonexistent_cli_abc123__"), // underlying CLI missing + ); + let reqs = cli_login_requirements(&[exe, "--list"], "install the CLI", &rt); + assert!( + !reqs.is_empty(), + "CLI missing must produce a CliLogin requirement" + ); + if let Requirement::CliLogin { + ref availability, .. + } = reqs[0] + { + assert_eq!( + *availability, + crate::managed_agents::AcpAvailabilityStatus::CliMissing, + "adapter present, CLI absent → CliMissing" + ); + } } #[test] fn cli_login_requirements_resolvable_binary_runs_probe_at_resolved_path() { - // `true` is a POSIX built-in available on every CI runner and always - // exits 0. Use it as a probe: probe_args = ["true", "--probe-arg"] - // → resolved path probed → exit 0 → logged_in = true → requirements - // is empty (Ready). This exercises the resolve_command fast path - // (binary found on PATH) + the probe-at-resolved-path branch. - // - // `true` is universally resolvable on POSIX/macOS/Linux CI, so we - // assert the ready (empty) outcome directly rather than hedging. + // Both adapter and CLI present (use the running test binary as a + // portable stand-in — always present, resolves via absolute path), + // probe exits 0 (run with `--list` which lists tests and exits 0). + // → logged_in = true → requirements is empty (Ready). + let exe = present_binary_str(); + let rt = make_cli_runtime(static_commands(vec![exe]), Some(exe)); let reqs = cli_login_requirements( - &["true", "--probe-arg"], - "this should not show (true always succeeds)", + &[exe, "--list"], + "this should not show (probe exits 0)", + &rt, ); assert!( reqs.is_empty(), "expected Ready (no requirements) when probe binary resolves and exits 0; \ - got {:?} — `true` is always on PATH on POSIX/macOS/Linux CI so this \ - must be empty", + got {:?}", reqs ); } + #[test] + fn cli_login_requirements_logged_out_emits_available() { + // Both adapter and CLI present, but probe exits non-zero (logged out). + // Use the test binary with an unrecognized argument as the probe — + // libtest exits non-zero for unknown flags on all platforms. + // → CliLogin{Available} (tooling installed, needs login). + let exe = present_binary_str(); + let rt = make_cli_runtime(static_commands(vec![exe]), Some(exe)); + let reqs = cli_login_requirements(&[exe, "--buzz-probe-fail-xyz"], "run `tool login`", &rt); + assert!( + !reqs.is_empty(), + "non-zero probe must produce a CliLogin requirement (logged out)" + ); + if let Requirement::CliLogin { + ref availability, .. + } = reqs[0] + { + assert_eq!( + *availability, + crate::managed_agents::AcpAvailabilityStatus::Available, + "tooling installed, probe fails → Available (logged-out)" + ); + } + } + // ── custom/unknown command ───────────────────────────────────────────── #[test] @@ -929,7 +1107,8 @@ mod tests { "login".to_string(), "status".to_string(), ], - setup_copy: "run `codex login --with-api-key`".to_string(), + setup_copy: "run `codex login`".to_string(), + availability: crate::managed_agents::AcpAvailabilityStatus::Available, }; let json = serde_json::to_value(&r).unwrap(); assert_eq!(json["surface"], "cli_login"); diff --git a/desktop/src-tauri/src/managed_agents/runtime.rs b/desktop/src-tauri/src/managed_agents/runtime.rs index 3fa953dd05..557bdccbdb 100644 --- a/desktop/src-tauri/src/managed_agents/runtime.rs +++ b/desktop/src-tauri/src/managed_agents/runtime.rs @@ -1635,10 +1635,12 @@ pub fn spawn_agent_child( Requirement::CliLogin { probe_args, setup_copy, + availability, } => serde_json::json!({ "surface": "cli_login", "probe_args": probe_args, "setup_copy": setup_copy, + "availability": availability, }), }) .collect(); diff --git a/desktop/src-tauri/src/managed_agents/types.rs b/desktop/src-tauri/src/managed_agents/types.rs index 633e74eea5..16e4cfc79e 100644 --- a/desktop/src-tauri/src/managed_agents/types.rs +++ b/desktop/src-tauri/src/managed_agents/types.rs @@ -544,7 +544,7 @@ pub struct ManagedAgentLogResponse { pub log_path: String, } -#[derive(Debug, Clone, Serialize, PartialEq, Eq)] +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] #[serde(rename_all = "snake_case")] pub enum AcpAvailabilityStatus { Available, diff --git a/desktop/src/app/AppShell.tsx b/desktop/src/app/AppShell.tsx index e35c41c0d2..02d0574e54 100644 --- a/desktop/src/app/AppShell.tsx +++ b/desktop/src/app/AppShell.tsx @@ -619,6 +619,7 @@ export function AppShell() { threadActivityItems, threadActivityFeedItems, feedItemState, + onOpenSettings: handleOpenSettings, }} > diff --git a/desktop/src/app/AppShellContext.tsx b/desktop/src/app/AppShellContext.tsx index 3266210fd9..30d67fd56b 100644 --- a/desktop/src/app/AppShellContext.tsx +++ b/desktop/src/app/AppShellContext.tsx @@ -3,6 +3,7 @@ import type { ContextParentResolver } from "@/features/channels/readState/readSt import type { ThreadActivityItem } from "@/features/channels/useUnreadChannels"; import type { FeedItemState } from "@/features/home/useFeedItemState"; import type { FeedItem } from "@/shared/api/types"; +import type { SettingsSection } from "@/features/settings/ui/SettingsPanels"; const EMPTY_SET = new Set(); @@ -45,6 +46,10 @@ type AppShellContextValue = { threadActivityItems: ThreadActivityItem[]; threadActivityFeedItems: FeedItem[]; feedItemState: FeedItemState; + // Open the Settings panel at the given section. Available on all surfaces + // that render under AppShell (channel, home, projects, pulse, agents). + // Used by config-nudge cards to deep-link to Settings → Doctor. + onOpenSettings: ((section: SettingsSection) => void) | null; }; const AppShellContext = React.createContext({ @@ -75,6 +80,7 @@ const AppShellContext = React.createContext({ undoUnread: () => {}, unreadSet: EMPTY_SET, }, + onOpenSettings: null, }); export function AppShellProvider({ diff --git a/desktop/src/shared/lib/configNudge.test.mjs b/desktop/src/shared/lib/configNudge.test.mjs index adee297f75..a2967fd956 100644 --- a/desktop/src/shared/lib/configNudge.test.mjs +++ b/desktop/src/shared/lib/configNudge.test.mjs @@ -62,13 +62,33 @@ test("extractConfigNudge parses cli_login requirement", () => { { surface: "cli_login", probe_args: ["codex", "login", "status"], - setup_copy: "run `codex login --with-api-key`", + setup_copy: "run `codex login`", + availability: "available", }, ], }; assert.deepEqual(extractConfigNudge(withSentinel("prose", payload)), payload); }); +test("extractConfigNudge returns null for cli_login without availability", () => { + // availability is required — old-format payloads (no availability field) + // must not parse so stale nudge JSON from before the Doctor-CTA update + // does not silently render a broken card. + const payload = { + agent_name: "Codex", + agent_pubkey: CODEX_PUBKEY, + requirements: [ + { + surface: "cli_login", + probe_args: ["codex", "login", "status"], + setup_copy: "run `codex login`", + // no availability field + }, + ], + }; + assert.equal(extractConfigNudge(withSentinel("prose", payload)), null); +}); + test("extractConfigNudge parses multiple requirements of mixed types", () => { const payload = { agent_name: "Atlas", @@ -80,6 +100,7 @@ test("extractConfigNudge parses multiple requirements of mixed types", () => { surface: "cli_login", probe_args: ["codex", "login"], setup_copy: "run `codex login`", + availability: "not_installed", }, ], }; diff --git a/desktop/src/shared/lib/configNudge.ts b/desktop/src/shared/lib/configNudge.ts index 627e0f6e8c..a806489481 100644 --- a/desktop/src/shared/lib/configNudge.ts +++ b/desktop/src/shared/lib/configNudge.ts @@ -17,6 +17,8 @@ // ── Types ───────────────────────────────────────────────────────────────────── +import type { AcpAvailabilityStatus } from "@/shared/api/types"; + /** A single missing requirement — mirrors the Rust `RequirementPayload` enum. */ export type ConfigNudgeRequirement = | { surface: "normalized_field"; field: string } @@ -25,6 +27,15 @@ export type ConfigNudgeRequirement = surface: "cli_login"; probe_args: string[]; setup_copy: string; + /** + * Granular install/auth state — mirrors `AcpAvailabilityStatus` from Rust. + * Determines which message and CTA the nudge card shows: + * - "available" → tooling installed, needs login + * - "adapter_missing" → CLI installed but ACP adapter missing + * - "cli_missing" → ACP adapter installed but CLI missing + * - "not_installed" → neither adapter nor CLI found + */ + availability: AcpAvailabilityStatus; }; /** @@ -114,7 +125,11 @@ function isConfigNudgeRequirement(v: unknown): v is ConfigNudgeRequirement { return ( Array.isArray(r.probe_args) && r.probe_args.every((a) => typeof a === "string") && - typeof r.setup_copy === "string" + typeof r.setup_copy === "string" && + (r.availability === "available" || + r.availability === "adapter_missing" || + r.availability === "cli_missing" || + r.availability === "not_installed") ); default: return false; diff --git a/desktop/src/shared/ui/config-nudge-attachment.tsx b/desktop/src/shared/ui/config-nudge-attachment.tsx index 767a83417a..67cdbca0f0 100644 --- a/desktop/src/shared/ui/config-nudge-attachment.tsx +++ b/desktop/src/shared/ui/config-nudge-attachment.tsx @@ -1,6 +1,7 @@ import { AlertTriangle } from "lucide-react"; import { requestOpenEditAgent } from "@/features/agents/openEditAgentEvent"; +import { useAppShell } from "@/app/AppShellContext"; import type { ConfigNudgePayload } from "@/shared/lib/configNudge"; import { cn } from "@/shared/lib/cn"; import { useProfilePanel } from "@/shared/context/ProfilePanelContext"; @@ -32,6 +33,57 @@ function requirementKey( } } +/** + * Returns true when every requirement in the nudge is a `cli_login` surface. + * Non-authOnly all-cli_login cards (at least one install-state row) route to + * Doctor — install/login problems can't be fixed in Edit Agent. AuthOnly cards + * (every row is `availability === "available"`) are purely informational and + * do not route anywhere. + */ +function isAllCliLogin(reqs: ConfigNudgePayload["requirements"]): boolean { + return reqs.length > 0 && reqs.every((r) => r.surface === "cli_login"); +} + +/** + * Returns true when the card is all-cli_login AND every requirement is in the + * `available` state (tooling installed, just needs login). In this case Doctor + * has no auth functionality and is a misleading dead-end — the card becomes + * purely informational (no trigger, no CTA, no pointer/hover affordance). + */ +function isAuthOnly(reqs: ConfigNudgePayload["requirements"]): boolean { + return ( + reqs.length > 0 && + reqs.every( + (r) => r.surface === "cli_login" && r.availability === "available", + ) + ); +} + +/** + * Per-state human-readable copy for a cli_login requirement. + * Uses the probe_args[0] as a best-effort harness name. + */ +function cliLoginMessage( + req: Extract< + ConfigNudgePayload["requirements"][number], + { surface: "cli_login" } + >, +): string { + const harness = req.probe_args[0] ?? "the CLI tool"; + switch (req.availability) { + case "not_installed": + return `${harness} isn't installed`; + case "cli_missing": + return `${harness} CLI is missing`; + case "adapter_missing": + return `${harness} ACP adapter isn't installed`; + case "available": + // Tooling is present but authentication is needed — fall back to + // the backend-supplied copy which has the exact login command. + return req.setup_copy; + } +} + /** * Inline card rendered when the desktop detects a `buzz:config-nudge` * sentinel in a kind:9 message body. @@ -40,11 +92,24 @@ function requirementKey( * variant so it is visually distinct and consistent with other error states in * the system. * - * The card's trigger opens the Edit Agent dialog for the agent by: - * 1. Calling `openProfilePanel(pubkey)` (from ProfilePanelContext) to ensure - * the profile panel is visible. - * 2. Dispatching `requestOpenEditAgent(pubkey)` so that `UserProfilePanel` - * auto-opens the edit dialog once it mounts with that pubkey. + * Routing: + * (A) When ALL requirements are `cli_login` in an install state + * (`not_installed` / `cli_missing` / `adapter_missing`): the card trigger + * opens Settings → Doctor. A card-level "Open Doctor →" label in + * `AttachmentActions` confirms the action at rest. + * (A-auth) When ALL requirements are `cli_login` with `availability === + * "available"` (tooling installed, just needs login): Doctor has no auth + * functionality and would be a misleading dead-end. The card is purely + * informational — no trigger, no CTA, no pointer/hover affordance. The + * inline copy (`setup_copy`) already tells the user the exact command. + * (B) Otherwise (mixed card), the card trigger opens Edit Agent as the + * card-level fallback. Each row carries its own inline CTA sharing one + * right edge so the actions are clearly paired with their requirement: + * - `cli_login` rows in an install state → "Open Doctor →" (Doctor). + * - `cli_login` rows with `availability === "available"` → no per-row CTA. + * - `env_key` / `normalized_field` rows → "Edit Agent →" (Edit Agent). + * The `AttachmentActions` column is omitted on mixed cards — per-row CTAs + * replace it. */ export function ConfigNudgeCard({ className, @@ -54,15 +119,60 @@ export function ConfigNudgeCard({ nudge: ConfigNudgePayload; }) { const { openProfilePanel } = useProfilePanel(); + const { onOpenSettings } = useAppShell(); - const handleOpen = () => { + const allCliLogin = isAllCliLogin(nudge.requirements); + const authOnly = isAuthOnly(nudge.requirements); + + const openDoctor = () => { + if (!onOpenSettings) { + console.warn( + "[ConfigNudgeCard] onOpenSettings is null — Doctor deep-link unavailable on this surface", + ); + } + onOpenSettings?.("doctor"); + }; + + const openEditAgent = () => { openProfilePanel?.(nudge.agent_pubkey); requestOpenEditAgent(nudge.agent_pubkey); }; + const handleOpen = () => { + if (allCliLogin) { + // (A) Non-authOnly install-state all-cli_login card — route to Doctor. + // AuthOnly cards never mount this trigger, so this branch only runs for + // install-state cards where Doctor is the correct destination. + openDoctor(); + } else { + // (B) Mixed card — card-level fallback to Edit Agent. + openEditAgent(); + } + }; + + const handleOpenDoctor = (e: React.MouseEvent) => { + // (B) Per-row Doctor CTA — stop propagation so the card trigger doesn't + // double-fire to Edit Agent on mixed cards. + e.stopPropagation(); + openDoctor(); + }; + + const handleOpenEditAgent = (e: React.MouseEvent) => { + // (B) Per-row Edit Agent CTA — stop propagation so the card trigger + // doesn't double-fire. + e.stopPropagation(); + openEditAgent(); + }; + return ( @@ -75,50 +185,113 @@ export function ConfigNudgeCard({
{nudge.requirements.map((req, i) => ( - + ))}
- - - Edit Agent - - - + {/* (A) Install-state all-cli_login card only — single card-level CTA + confirming the action at rest. Auth-only cards are informational + (no CTA); mixed cards render per-row CTAs instead. */} + {allCliLogin && !authOnly && ( + + Open Doctor → + + )} + {/* Auth-only cards are purely informational — no trigger, no routing. */} + {!authOnly && ( + + )} ); } function RequirementRow({ + allCliLogin, + onOpenDoctor, + onOpenEditAgent, requirement, }: { + allCliLogin: boolean; + onOpenDoctor: (e: React.MouseEvent) => void; + onOpenEditAgent: (e: React.MouseEvent) => void; requirement: ConfigNudgePayload["requirements"][number]; }) { switch (requirement.surface) { case "env_key": return ( -
- Set{" "} - - {requirement.key} - {" "} - in Edit Agent → Environment variables +
+ + Set{" "} + + {requirement.key} + {" "} + in Edit Agent → Environment variables + + {!allCliLogin && ( + + )}
); case "normalized_field": return ( -
- Set the {requirement.field} field in Edit Agent - dropdowns +
+ + Set the {requirement.field} field in Edit Agent + dropdowns + + {!allCliLogin && ( + + )}
); case "cli_login": return ( -
- {requirement.setup_copy} +
+ + {cliLoginMessage(requirement)} + + {/* (B) Per-row Doctor CTA — shown only on mixed cards where the + card-level trigger opens Edit Agent (not auth-only cards). When + allCliLogin is true the card trigger already routes to Doctor; the + per-row button is redundant and is suppressed. Also suppressed for + `available` cli_login rows — Doctor has no auth functionality and + the setup_copy already provides the exact login command. + stopPropagation prevents double-fire on mixed cards where both + card and row CTAs are visible. */} + {!allCliLogin && requirement.availability !== "available" && ( + + )}
); } diff --git a/desktop/tests/e2e/doctor-cta-screenshots.spec.ts b/desktop/tests/e2e/doctor-cta-screenshots.spec.ts new file mode 100644 index 0000000000..9bd2732175 --- /dev/null +++ b/desktop/tests/e2e/doctor-cta-screenshots.spec.ts @@ -0,0 +1,300 @@ +import { expect, test } from "@playwright/test"; + +import { installMockBridge, TEST_IDENTITIES } from "../helpers/bridge"; + +const SHOTS = "test-results/doctor-cta"; + +// Channel ID for the seeded #general mock channel. +const GENERAL_CHANNEL_ID = "9a1657ac-f7aa-5db0-b632-d8bbeb6dfb50"; + +// Tyler is a managed agent seed in the mock bridge — used as the nudge sender. +const AGENT_PUBKEY = TEST_IDENTITIES.tyler.pubkey; +const AGENT_NAME = "Tyler Agent"; + +/** Settle CSS / Web Animations before capture. */ +async function settleAnimations(page: import("@playwright/test").Page) { + await page.evaluate(() => + Promise.all(document.getAnimations().map((a) => a.finished)), + ); +} + +/** + * Wait for the Tauri invoke mock to be available, then call a mock command. + * Mirrors the helper used in config-bridge-screenshots.spec.ts. + */ +async function invokeMockCommand( + page: import("@playwright/test").Page, + command: string, + payload?: Record, +): Promise { + await page.waitForFunction( + () => { + const w = window as Window & { + __BUZZ_E2E_INVOKE_MOCK_COMMAND__?: unknown; + __TAURI_INTERNALS__?: { invoke?: unknown }; + }; + return ( + typeof w.__BUZZ_E2E_INVOKE_MOCK_COMMAND__ === "function" || + typeof w.__TAURI_INTERNALS__?.invoke === "function" + ); + }, + null, + { timeout: 5_000 }, + ); + return page.evaluate( + async ({ command: cmd, payload: pl }) => { + const w = window as Window & { + __BUZZ_E2E_INVOKE_MOCK_COMMAND__?: ( + command: string, + payload?: Record, + ) => Promise; + __TAURI_INTERNALS__?: { + invoke?: ( + command: string, + payload?: Record, + ) => Promise; + }; + }; + const invoke = + w.__BUZZ_E2E_INVOKE_MOCK_COMMAND__ ?? w.__TAURI_INTERNALS__?.invoke; + if (!invoke) throw new Error("Mock invoke bridge is unavailable."); + return invoke(cmd, pl); + }, + { command, payload }, + ); +} + +/** + * Build a `buzz:config-nudge` sentinel body from a requirements array. + * Mirrors the format nudge_body() in setup_mode.rs produces. + */ +function makeNudgeSentinel( + agentName: string, + agentPubkey: string, + requirements: unknown[], +): string { + const payload = JSON.stringify({ + agent_name: agentName, + agent_pubkey: agentPubkey, + requirements, + }); + return `**${agentName}** needs configuration before it can respond.\n\n\`\`\`buzz:config-nudge\n${payload}\n\`\`\``; +} + +/** + * Inject a nudge message from the agent into #general, then navigate to the + * channel so the card renders in the message list. + */ +async function injectNudgeAndNavigate( + page: import("@playwright/test").Page, + content: string, +): Promise { + await invokeMockCommand(page, "send_managed_agent_channel_message", { + agentPubkey: AGENT_PUBKEY, + channelId: GENERAL_CHANNEL_ID, + content, + }); + + // Navigate to #general — the injected message is live-pushed so the card + // renders in the current message list. + await page.getByTestId("channel-general").click(); + await expect(page.getByTestId("chat-title")).toHaveText("general"); +} + +test.describe("doctor CTA nudge card screenshots", () => { + test.use({ viewport: { width: 1280, height: 900 } }); + + test.beforeEach(async ({ page }) => { + page.on("pageerror", (err) => { + console.error( + "PAGE ERROR:", + err.message, + err.stack?.split("\n").slice(0, 5).join("\n"), + ); + }); + page.on("console", (msg) => { + if (msg.type() === "error") { + console.error("CONSOLE ERROR:", msg.text().slice(0, 500)); + } + }); + }); + + /** + * 01 — pure cli_login card (all requirements are cli_login, availability=available): + * Tooling is installed but needs login — Doctor has no auth functionality + * and would be a misleading dead-end. The card is purely informational: + * no trigger, no CTA, no pointer/hover affordance. The inline copy + * ("run `claude auth login` to authenticate") already tells the user + * the exact command to run. + */ + test("01-cli-login-available-informational", async ({ page }) => { + await installMockBridge(page, { + managedAgents: [ + { + pubkey: AGENT_PUBKEY, + name: AGENT_NAME, + status: "running" as const, + channelNames: ["general"], + }, + ], + }); + + await page.goto("/", { waitUntil: "domcontentloaded" }); + + const content = makeNudgeSentinel(AGENT_NAME, AGENT_PUBKEY, [ + { + surface: "cli_login", + probe_args: ["claude"], + setup_copy: "run `claude auth login` to authenticate", + availability: "available", + }, + ]); + + await injectNudgeAndNavigate(page, content); + + // Wait for the nudge card to render. + const card = page.locator("[data-config-nudge]").last(); + await expect(card).toBeVisible({ timeout: 10_000 }); + // Auth-only card is informational — no Doctor CTA anywhere. + await expect(card.getByText("Open Doctor →")).toHaveCount(0); + + await card.scrollIntoViewIfNeeded(); + await settleAnimations(page); + + await card.screenshot({ + path: `${SHOTS}/01-cli-login-available-informational.png`, + }); + }); + + /** + * 02 — not_installed state: neither adapter nor CLI found. + * Card shows "claude isn't installed" copy + inline "Open Doctor →" CTA. + */ + test("02-cli-login-not-installed-state", async ({ page }) => { + await installMockBridge(page, { + managedAgents: [ + { + pubkey: AGENT_PUBKEY, + name: AGENT_NAME, + status: "stopped" as const, + channelNames: ["general"], + }, + ], + }); + + await page.goto("/", { waitUntil: "domcontentloaded" }); + + const content = makeNudgeSentinel(AGENT_NAME, AGENT_PUBKEY, [ + { + surface: "cli_login", + probe_args: ["claude"], + setup_copy: "install Claude Code", + availability: "not_installed", + }, + ]); + + await injectNudgeAndNavigate(page, content); + + const card = page.locator("[data-config-nudge]").last(); + await expect(card).toBeVisible({ timeout: 10_000 }); + await expect(card.getByText(/isn't installed/)).toBeVisible(); + + await card.scrollIntoViewIfNeeded(); + await settleAnimations(page); + + await card.screenshot({ + path: `${SHOTS}/02-cli-login-not-installed-state.png`, + }); + }); + + /** + * 03 — mixed card: one cli_login (adapter_missing) + one env_key requirement. + * Each requirement row owns its CTA, right-aligned to a shared edge: + * the cli_login row shows "Open Doctor →" and the env_key row shows + * "Edit Agent →", both at the same x (vertically aligned). + */ + test("03-mixed-requirements-inline-doctor-cta", async ({ page }) => { + await installMockBridge(page, { + managedAgents: [ + { + pubkey: AGENT_PUBKEY, + name: AGENT_NAME, + status: "stopped" as const, + channelNames: ["general"], + }, + ], + }); + + await page.goto("/", { waitUntil: "domcontentloaded" }); + + const content = makeNudgeSentinel(AGENT_NAME, AGENT_PUBKEY, [ + { + surface: "cli_login", + probe_args: ["claude"], + setup_copy: "install the Claude Code ACP adapter", + availability: "adapter_missing", + }, + { + surface: "env_key", + key: "ANTHROPIC_API_KEY", + }, + ]); + + await injectNudgeAndNavigate(page, content); + + const card = page.locator("[data-config-nudge]").last(); + await expect(card).toBeVisible({ timeout: 10_000 }); + // Mixed card: cli_login row shows "Open Doctor →", env_key row shows "Edit Agent →". + await expect(card.getByText("Open Doctor →")).toBeVisible(); + // Both per-row CTAs share the same right edge (vertically aligned). + await expect(card.getByText("Edit Agent →", { exact: true })).toBeVisible(); + + await card.scrollIntoViewIfNeeded(); + await settleAnimations(page); + + await card.screenshot({ + path: `${SHOTS}/03-mixed-requirements-inline-doctor-cta.png`, + }); + }); + + /** + * 04 — cli_missing state: ACP adapter present but underlying CLI absent. + * Shows "claude CLI is missing" copy. + */ + test("04-cli-login-cli-missing-state", async ({ page }) => { + await installMockBridge(page, { + managedAgents: [ + { + pubkey: AGENT_PUBKEY, + name: AGENT_NAME, + status: "stopped" as const, + channelNames: ["general"], + }, + ], + }); + + await page.goto("/", { waitUntil: "domcontentloaded" }); + + const content = makeNudgeSentinel(AGENT_NAME, AGENT_PUBKEY, [ + { + surface: "cli_login", + probe_args: ["claude"], + setup_copy: "install the Claude CLI", + availability: "cli_missing", + }, + ]); + + await injectNudgeAndNavigate(page, content); + + const card = page.locator("[data-config-nudge]").last(); + await expect(card).toBeVisible({ timeout: 10_000 }); + await expect(card.getByText(/CLI is missing/)).toBeVisible(); + + await card.scrollIntoViewIfNeeded(); + await settleAnimations(page); + + await card.screenshot({ + path: `${SHOTS}/04-cli-login-cli-missing-state.png`, + }); + }); +});