diff --git a/doc/security-model.md b/doc/security-model.md index df8f69122..c1d9d8ac3 100644 --- a/doc/security-model.md +++ b/doc/security-model.md @@ -316,7 +316,7 @@ Install path: ```text Settings UI / WTA setup / wta hooks install - -> agent_hooks_installer::ensure_installed() + -> agent_hooks_installer::apply_install_plan() -> resolve wt-agent-hooks bundle -> Claude / Copilot plugin manager or Gemini extension manager -> persistent CLI hook registration diff --git a/src/cascadia/TerminalSettingsEditor/AIAgentsViewModel.cpp b/src/cascadia/TerminalSettingsEditor/AIAgentsViewModel.cpp index 843983898..da4320239 100644 --- a/src/cascadia/TerminalSettingsEditor/AIAgentsViewModel.cpp +++ b/src/cascadia/TerminalSettingsEditor/AIAgentsViewModel.cpp @@ -1153,9 +1153,9 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation // build/scripts/Verify-AgentHooks.ps1 consumes — so the Settings UI // and the verify script can never disagree about install state. // - // The single primary "Install hooks" button still delegates to - // `wta install-hooks`; afterwards we re-invoke the status query to - // refresh the rows. + // The single primary "Install hooks" button delegates to + // `wta hooks install --only-missing`; afterwards we re-invoke the status + // query to refresh the rows. // _ResolveWtaExePath and _RunWtaCaptureStdout moved to // src/cascadia/inc/WtaProcess.h for shared use. @@ -1330,7 +1330,20 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation _installingAgentHooks = true; _agentHooksInstallSummary = RS_(L"AIAgents_HooksInstallingSummary"); _NotifyChanges(L"IsInstallingAgentHooks", L"AgentHooksInstallSummary", L"HasAgentHooksInstallSummary"); - _RunHooksWtaAsync(L"hooks install"); + // `--only-missing` builds a per-CLI plan from a status pre-pass: + // complete-and-current CLIs are left alone, a complete but + // out-of-date bridge is upgraded, and anything missing, partial, + // disabled or pointing at a stale path is installed. + // + // The distinction matters. Re-running `plugin install` on a complete + // bridge changes nothing — every CLI answers "already installed" — + // costs two Node spawns per CLI, and fails outright when a running + // agent CLI holds its plugin directory open, so the button used to be + // slow and could report a failure for work that never needed doing. + // Routing an out-of-date bridge there would be worse still: it would + // no-op and then report success. Upgrading needs `plugin update` / + // `extensions update` / a Codex reinstall, which is what wta runs. + _RunHooksWtaAsync(L"hooks install --only-missing"); } void AIAgentsViewModel::RemoveCopilotHooks() diff --git a/src/cascadia/TerminalSettingsEditor/AIAgentsViewModel.idl b/src/cascadia/TerminalSettingsEditor/AIAgentsViewModel.idl index a00f8770a..aa103b204 100644 --- a/src/cascadia/TerminalSettingsEditor/AIAgentsViewModel.idl +++ b/src/cascadia/TerminalSettingsEditor/AIAgentsViewModel.idl @@ -121,8 +121,11 @@ namespace Microsoft.Terminal.Settings.Editor // ── Agent Hooks ────────────────────────────────────────────────── // Drives the "Agent session tracking (hooks)" expander. Shape: - // Row 1 always: "Install hooks" button — runs `wta hooks install` - // across every CLI on PATH (idempotent). + // Row 1 always: "Install hooks" button — runs + // `wta hooks install --only-missing` across every CLI + // on PATH. Missing / partial / stale bridges are + // installed, complete but out-of-date ones are + // upgraded, and complete current ones are left alone. // Rows 2+ : one per CLI that currently has any hook state on // disk (marketplace registered OR plugin installed), // whether fully installed or partial. Each row exposes diff --git a/tools/wta/src/agent_check.rs b/tools/wta/src/agent_check.rs index edc71dc17..b6eb79c04 100644 --- a/tools/wta/src/agent_check.rs +++ b/tools/wta/src/agent_check.rs @@ -8,7 +8,6 @@ //! //! Composite functions (combine basics): //! - `check_agent` — find_exe → AgentStatus -//! - `ensure_installed` — find_exe → install if missing → refresh_path → find_exe use crate::agent_registry; use std::ffi::OsStr; @@ -501,21 +500,6 @@ pub async fn check_agent_in_source( } } -/// Ensure an agent is installed: find → install if missing → refresh PATH → find again. -pub async fn ensure_installed( - agent_id: &str, - on_line: impl FnMut(String) + Send + 'static, -) -> Result, String> { - if let Some(path) = find_exe(agent_id) { - return Ok(Some(path)); - } - - install(agent_id, on_line).await?; - refresh_path(); - - Ok(find_exe(agent_id)) -} - // ─── Internal helpers ─────────────────────────────────────────────────────── /// Install GitHub Copilot via winget with streaming output. diff --git a/tools/wta/src/agent_hooks_installer.rs b/tools/wta/src/agent_hooks_installer.rs index 4309c12e5..ef099bb86 100644 --- a/tools/wta/src/agent_hooks_installer.rs +++ b/tools/wta/src/agent_hooks_installer.rs @@ -91,7 +91,7 @@ // Public surface for `wta hooks ` (Track 2 / #18) // ------------------------------------------------------- // -// In addition to the install entry point [`ensure_installed`], this module +// In addition to the install entry point [`apply_install_plan`], this module // exposes two read-only / best-effort APIs the Settings UI and // `Verify-AgentHooks.ps1` consume: // @@ -676,54 +676,108 @@ pub struct InstallFailure { pub reason: String, } -/// Top-level entry point. Run once at wta startup. Idempotent and silent on -/// failure: if a CLI isn't installed, we skip it; if its settings.json is -/// malformed, we leave it alone. -pub fn ensure_installed() { - let _ = ensure_installed_scoped(CliScope::All); +/// What an install pass should do for one CLI. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum InstallAction { + /// The bridge is complete and not known to be behind the bundle. Nothing + /// to do. + Skip, + /// Nothing usable is registered — or what is registered is partial, + /// disabled, or points at a path that no longer exists. Run the first-run + /// install flow. + Install, + /// The bridge is complete but older than the bundle. Run the per-CLI + /// upgrade flow, **not** the install flow: every supported CLI answers a + /// second `plugin install` with "already installed" and changes nothing, + /// so installing here would report a success that never happened. + Upgrade, +} + +/// Decide what an install pass should do for one CLI, from its status row. +/// +/// Pure — no IO, no spawns. Splits the three cases the Settings "Install +/// hooks" button has to tell apart: +/// +/// * incomplete in any way (not on PATH, marketplace missing or pointing at +/// a pruned path, plugin missing or disabled, or a verdict that came from +/// filesystem heuristics rather than the CLI itself) → [`InstallAction::Install`]; +/// * complete but a release behind the bundle → [`InstallAction::Upgrade`], +/// because `install` cannot upgrade — that needs `plugin update` / +/// `extensions update` / a Codex reinstall; +/// * complete and not provably behind → [`InstallAction::Skip`]. +/// +/// An unreadable version on either side lands in `Skip`: we can't prove the +/// bridge is stale, running `install` against it would no-op anyway, and +/// [`upgrade_installed_hooks`] re-checks it at master startup with a richer +/// probe than [`CliStatus`] carries. +pub fn decide_install_action(status: &CliStatus) -> InstallAction { + let complete = status.binary_on_path + && status.marketplace_registered + && status.marketplace_path_valid + && status.plugin_installed + && status.plugin_enabled + && status.detection_fallback.is_none(); + if !complete { + return InstallAction::Install; + } + let parse = |v: &Option| v.as_deref().and_then(|s| s.parse::().ok()); + match ( + parse(&status.installed_version), + parse(&status.bundle_version), + ) { + (Some(installed), Some(bundled)) if installed < bundled => InstallAction::Upgrade, + _ => InstallAction::Skip, + } } -/// Install hooks for the specified scope (all CLIs or a single one). +/// Execute a per-CLI plan of [`InstallAction`]s. /// -/// Returns the CLIs whose install actively failed. Startup callers ignore it — -/// a failed hook install must never block wta from starting — but the -/// `wta hooks install` command reports it, because an install that failed -/// silently is indistinguishable from one that worked. -pub fn ensure_installed_scoped(scope: CliScope) -> Vec { +/// Per-CLI failures are recorded and the loop continues — one CLI's broken +/// install must not hide the others. `Skip` entries are accepted and ignored +/// so callers may pass a full plan or a pre-filtered one. +pub fn apply_install_plan(plan: &[(CliKind, InstallAction)]) -> Vec { let Some(home) = home_dir() else { tracing::debug!(target: "agent_hooks", "no HOME/USERPROFILE; skipping"); return Vec::new(); }; let mut failures = Vec::new(); - let mut record = |cli: CliKind, outcome: InstallOutcome| { - if let InstallOutcome::Failed(reason) = outcome { + for (cli, action) in plan.iter().copied() { + let failure = match action { + InstallAction::Skip => None, + InstallAction::Install => match install_one(cli, &home) { + InstallOutcome::Failed(reason) => Some(reason), + InstallOutcome::Installed | InstallOutcome::Skipped => None, + }, + InstallAction::Upgrade => { + upgrade_one_cli(cli, &home, read_bundled_version(cli)).err() + } + }; + if let Some(reason) = failure { failures.push(InstallFailure { cli: cli.name(), reason, }); } - }; - if scope.includes(CliKind::Claude) { - record(CliKind::Claude, install_for_claude(&home)); - } - if scope.includes(CliKind::Copilot) { - record(CliKind::Copilot, install_for_copilot(&home)); - } - if scope.includes(CliKind::Gemini) { - record(CliKind::Gemini, install_for_gemini(&home)); - } - if scope.includes(CliKind::Codex) { - record(CliKind::Codex, install_for_codex(&home)); - } - if scope.includes(CliKind::OpenCode) { - record(CliKind::OpenCode, install_for_opencode(&home)); } failures } -/// Run the installer against a specific home directory. Split out from -/// [`ensure_installed`] so tests can drive it with an isolated tempdir -/// without mutating `USERPROFILE`/`HOME` for the whole process. +/// Per-CLI dispatch for the first-run install flow. +fn install_one(cli: CliKind, home: &Path) -> InstallOutcome { + match cli { + CliKind::Copilot => install_for_copilot(home), + CliKind::Claude => install_for_claude(home), + CliKind::Gemini => install_for_gemini(home), + CliKind::Codex => install_for_codex(home), + CliKind::OpenCode => install_for_opencode(home), + } +} + +/// Run every per-CLI install flow against a specific home directory. +/// +/// Test-only: it exists so tests can drive the installers against an isolated +/// tempdir without mutating `USERPROFILE`/`HOME` for the whole process. +#[cfg(test)] fn ensure_installed_in(home: &Path) { install_for_claude(home); install_for_copilot(home); @@ -4228,7 +4282,7 @@ pub fn upgrade_installed_hooks() { } // Cache miss (or first ever run): do the full per-CLI check. - let completed = upgrade_one_cli(cli, &home, bundle_version); + let completed = upgrade_one_cli(cli, &home, bundle_version).is_ok(); // Cache completed checks, including intentional skips. Failed // OpenCode file copies must retry on the next startup. @@ -4295,7 +4349,15 @@ fn probe_installed(cli: CliKind, home: &Path) -> InstalledProbe { } /// Per-CLI upgrade entry: read installed state, decide, dispatch. -fn upgrade_one_cli(cli: CliKind, home: &Path, bundle_version: Option) -> bool { +/// +/// `Err` carries a user-facing reason so `wta hooks install` can name what +/// went wrong per CLI; `upgrade_installed_hooks` only needs the pass/fail bit +/// because the individual upgrade helpers already log their own errors. +fn upgrade_one_cli( + cli: CliKind, + home: &Path, + bundle_version: Option, +) -> Result<(), String> { let probe = probe_installed(cli, home); let installed = match probe { Ok(installed) => installed, @@ -4306,7 +4368,7 @@ fn upgrade_one_cli(cli: CliKind, home: &Path, bundle_version: Option) - err = %error, "failed to detect installed hook version; leaving cache unchanged for retry", ); - return false; + return Err(format!("failed to detect the installed hook version: {error}")); } }; @@ -4327,7 +4389,7 @@ fn upgrade_one_cli(cli: CliKind, home: &Path, bundle_version: Option) - "upgrade decision", ); - match action { + let succeeded = match action { UpgradeAction::Skip(_) => true, UpgradeAction::UpdatePlugin => match cli { CliKind::Copilot => upgrade_copilot(home), @@ -4374,6 +4436,17 @@ fn upgrade_one_cli(cli: CliKind, home: &Path, bundle_version: Option) - UpgradeAction::GeminiUpdateInPlace => upgrade_gemini_in_place(), UpgradeAction::GeminiReinstall => upgrade_gemini_reinstall(home), UpgradeAction::OpenCodeCopy => install_for_opencode(home).installed(), + }; + if succeeded { + Ok(()) + } else { + // The helper that failed has already logged the concrete command and + // stderr; threading that string back through five `bool`-returning + // upgrade paths would be a bigger change than the report is worth. + Err(format!( + "{} hook upgrade failed; see wta-install-hooks.log", + cli.name() + )) } } diff --git a/tools/wta/src/agent_hooks_installer_tests.rs b/tools/wta/src/agent_hooks_installer_tests.rs index 3d2e3b864..6c61bac8d 100644 --- a/tools/wta/src/agent_hooks_installer_tests.rs +++ b/tools/wta/src/agent_hooks_installer_tests.rs @@ -2135,6 +2135,155 @@ fn gemini_extensions_list_json_parser_reports_the_installed_version() { assert_eq!(parsed.version.map(|v| v.to_string()), Some("0.1.5".into())); } +// ---- decide_install_action (`hooks install --only-missing`) ---------- + +fn installed_status(name: &'static str) -> CliStatus { + CliStatus { + name, + binary_on_path: true, + binary_path: None, + marketplace_registered: true, + marketplace_path: None, + marketplace_path_valid: true, + plugin_installed: true, + plugin_enabled: true, + installed_version: Some("0.1.6".into()), + bundle_version: Some("0.1.6".into()), + detection_fallback: None, + } +} + +/// A complete bridge at the bundled version has nothing left to do. Installed +/// being *newer* counts too — that is a dev worktree pointed at a fresher +/// bundle, and "upgrading" it would be a downgrade. +#[test] +fn install_action_skips_a_complete_current_bridge() { + assert_eq!( + decide_install_action(&installed_status("copilot")), + InstallAction::Skip + ); + assert_eq!( + decide_install_action(&CliStatus { + installed_version: Some("0.2.0".into()), + ..installed_status("copilot") + }), + InstallAction::Skip + ); +} + +/// The case this three-way split exists for: the bridge is complete, so +/// `install` would answer "already installed" and change nothing. Only the +/// per-CLI upgrade flow can move it to the bundled version. +#[test] +fn install_action_upgrades_a_complete_but_outdated_bridge() { + assert_eq!( + decide_install_action(&CliStatus { + installed_version: Some("0.1.5".into()), + ..installed_status("copilot") + }), + InstallAction::Upgrade + ); +} + +/// An unreadable version on either side is not proof of staleness. `install` +/// would no-op against a complete bridge, and master startup re-checks it +/// with a richer probe than `CliStatus` carries, so skipping is both honest +/// and cheap. +#[test] +fn install_action_skips_when_a_version_is_unreadable() { + for status in [ + CliStatus { + installed_version: None, + ..installed_status("copilot") + }, + CliStatus { + bundle_version: None, + ..installed_status("copilot") + }, + CliStatus { + installed_version: Some("1.2".into()), + ..installed_status("copilot") + }, + ] { + assert_eq!(decide_install_action(&status), InstallAction::Skip, "{status:?}"); + } +} + +/// Every partial state must stay eligible for a real install. Each of these +/// reads as "something is installed" to a casual check, which is why they are +/// listed out rather than folded into one assertion. +#[test] +fn install_action_installs_any_partial_bridge() { + let partials = [ + CliStatus { + marketplace_registered: false, + ..installed_status("copilot") + }, + CliStatus { + marketplace_path_valid: false, + ..installed_status("copilot") + }, + CliStatus { + plugin_installed: false, + ..installed_status("copilot") + }, + CliStatus { + plugin_enabled: false, + ..installed_status("copilot") + }, + ]; + for status in partials { + assert_eq!( + decide_install_action(&status), + InstallAction::Install, + "{status:?} must stay installable" + ); + } +} + +/// An out-of-date bridge that is also partial must still be installed, not +/// upgraded: the upgrade flow refuses a disabled or unregistered plugin, so +/// routing it there would leave it broken. +#[test] +fn install_action_prefers_install_over_upgrade_for_a_broken_outdated_bridge() { + assert_eq!( + decide_install_action(&CliStatus { + plugin_enabled: false, + installed_version: Some("0.1.5".into()), + ..installed_status("copilot") + }), + InstallAction::Install + ); +} + +/// A CLI that isn't on PATH can't be skipped as "already done" — the install +/// path has its own reason for passing on it, and conflating the two would +/// hide a CLI that vanished from PATH after its hooks were installed. +#[test] +fn install_action_installs_when_the_cli_is_not_on_path() { + assert_eq!( + decide_install_action(&CliStatus { + binary_on_path: false, + ..installed_status("copilot") + }), + InstallAction::Install + ); +} + +/// The fs fallback is a guess about another tool's private on-disk layout. +/// It is good enough to report a state; it is not good enough to decline the +/// work the user explicitly asked for. +#[test] +fn install_action_installs_when_the_verdict_came_from_the_fs_fallback() { + assert_eq!( + decide_install_action(&CliStatus { + detection_fallback: Some("fs"), + ..installed_status("copilot") + }), + InstallAction::Install + ); +} + // ---- run_plugin_cli idempotency (#17) ------------------------------- #[test] diff --git a/tools/wta/src/app.rs b/tools/wta/src/app.rs index f3b0e1c9b..737a5c00a 100644 --- a/tools/wta/src/app.rs +++ b/tools/wta/src/app.rs @@ -1141,8 +1141,6 @@ pub struct App { /// truth) and the `agents_view::render` call in `ui/layout.rs`. See /// [`MVP_SESSIONS_ORIGIN_FILTER`] for the gate to flip when un-MVP. pub sessions_origin_filter: crate::agent_sessions::OriginFilter, - // Onboarding: signals main.rs to install agent hook plugins on demand. - install_request_tx: Option>, /// Posts `AppEvent::AgentSessionEvent` from background callbacks /// (split-pane callback in `dispatch_resume`) back into the main /// event loop so they can apply to `agent_sessions` on the UI thread. @@ -1405,7 +1403,6 @@ impl App { agent_supports_load_session: false, agent_supports_image: false, sessions_origin_filter: resolve_sessions_origin_filter(), - install_request_tx: None, agent_event_tx: None, session_hook_tx: None, delegate_agents: None, @@ -1734,13 +1731,6 @@ impl App { } } - /// Wire a sender that signals main.rs to run the agent-hooks installer - /// (Settings UI -> Install button -> main.rs spawns - /// `agent_hooks_installer::ensure_installed`). - pub fn set_install_request_tx(&mut self, tx: mpsc::UnboundedSender<()>) { - self.install_request_tx = Some(tx); - } - /// Wire the main loop's `AppEvent` sender so background callbacks /// (e.g. `dispatch_resume`'s split-pane completion) can post /// `AgentSessionEvent`s back into the event loop instead of needing @@ -2581,15 +2571,6 @@ impl App { } } - /// Trigger an install-hooks request. No-op if no channel is wired - /// (e.g. running outside the packaged app). - #[allow(dead_code)] - pub fn request_install_hooks(&self) { - if let Some(tx) = &self.install_request_tx { - let _ = tx.send(()); - } - } - /// Filter to apply to the session management view based on which /// agent CLI the WTA agent pane is currently driving. Returns /// `Some(CliSource::*)` when `current_agent_id` resolves to a tracked diff --git a/tools/wta/src/cli/args.rs b/tools/wta/src/cli/args.rs index 035654a65..cfe1a8978 100644 --- a/tools/wta/src/cli/args.rs +++ b/tools/wta/src/cli/args.rs @@ -494,12 +494,19 @@ impl SessionsOriginArg { #[derive(Subcommand, Debug)] pub(crate) enum HooksAction { /// (Re-)install the wt-agent-hooks bridge. Installs for all supported - /// CLIs by default, or a single CLI with `--cli`. With `--json` returns - /// a structured per-CLI outcome report. + /// CLIs by default, or a single CLI with `--cli`. `--only-missing` skips + /// CLIs that are already current and upgrades the ones that are behind. + /// With `--json` returns a structured per-CLI outcome report. Install { /// Which CLI to install for. Default: `all`. #[arg(long, value_enum, default_value_t = HooksCliFilter::All)] cli: HooksCliFilter, + /// Skip CLIs whose hook bridge is already complete and current. A + /// complete but out-of-date bridge is upgraded rather than + /// re-installed, because a second `install` no-ops; missing, partial, + /// disabled and stale-path bridges are installed as usual. + #[arg(long)] + only_missing: bool, }, /// Print per-CLI install state. Returns JSON with `--json`, /// or a human-readable table by default. diff --git a/tools/wta/src/cli/hooks.rs b/tools/wta/src/cli/hooks.rs index 2a6181020..fef2f5ff1 100644 --- a/tools/wta/src/cli/hooks.rs +++ b/tools/wta/src/cli/hooks.rs @@ -2,11 +2,24 @@ use anyhow::Result; use super::args::HooksCliFilter; -pub(crate) fn run_install(cli: HooksCliFilter, json_mode: bool) -> Result<()> { +pub(crate) fn run_install(cli: HooksCliFilter, only_missing: bool, json_mode: bool) -> Result<()> { // Logging is initialized in `main()`; the install attempt is observable in // %LOCALAPPDATA%\IntelligentTerminal\logs\wta-install-hooks.log. let scope = cli.into_scope(); - let spawn_failures = crate::agent_hooks_installer::ensure_installed_scoped(scope); + + // `--only-missing` trades one status pass up front for a per-CLI plan. + // Without it every in-scope CLI gets the install flow, which is what a + // user reaches for when something is broken that status can't see. + // + // The plan matters as much as the saving. ` plugin install` is two + // Node spawns that a complete bridge answers with "already installed" and + // no-ops — so re-running it on a CLI that is merely *out of date* reports + // a success that never happened. Upgrading needs `plugin update` / + // `extensions update` / a Codex reinstall, so out-of-date CLIs are routed + // to the upgrade flow and complete-and-current ones are left alone. + let pre_status = only_missing.then(|| crate::agent_hooks_installer::status_scoped(scope)); + let plan = plan_install(scope, pre_status.as_ref()); + let spawn_failures = crate::agent_hooks_installer::apply_install_plan(&plan); // Two independent failure signals, because neither one alone is sufficient. // @@ -21,7 +34,14 @@ pub(crate) fn run_install(cli: HooksCliFilter, json_mode: bool) -> Result<()> { // // The status check stays because it catches the opposite case: a command // that reports success without leaving anything usable behind. - let report = crate::agent_hooks_installer::status_scoped(scope); + // + // When the plan came out empty, the pre-pass IS the verification: nothing + // ran, so nothing on disk moved, and re-querying would pay a second round + // of per-CLI Node spawns to re-derive a report we still hold. + let report = match pre_status { + Some(pre) if plan.is_empty() => pre, + _ => crate::agent_hooks_installer::status_scoped(scope), + }; let missing: Vec<&str> = report .clis .iter() @@ -84,6 +104,59 @@ pub(crate) fn run_install(cli: HooksCliFilter, json_mode: bool) -> Result<()> { anyhow::bail!(message) } +/// Build the per-CLI plan the install pass will execute. +/// +/// `pre_status` is `Some` only for `--only-missing`. Each in-scope CLI is +/// classified by +/// [`decide_install_action`](crate::agent_hooks_installer::decide_install_action); +/// `Skip` entries are dropped so an empty result means "nothing to do". +/// Without a pre-pass every in-scope CLI gets `Install`, which is the +/// historical `wta hooks install` behavior. +/// +/// A CLI missing from the report is treated as `Install`: absent evidence is +/// not evidence of a working bridge. +/// +/// Split out from [`run_install`] so the plan is testable without spawning a +/// single agent CLI. +fn plan_install( + scope: crate::agent_hooks_installer::CliScope, + pre_status: Option<&crate::agent_hooks_installer::StatusReport>, +) -> Vec<( + crate::agent_hooks_installer::CliKind, + crate::agent_hooks_installer::InstallAction, +)> { + use crate::agent_hooks_installer::{decide_install_action, CliKind, CliScope, InstallAction}; + + CliKind::ALL + .iter() + .copied() + .filter(|kind| match scope { + CliScope::All => true, + CliScope::One(only) => only == *kind, + }) + .filter_map(|kind| { + let Some(status) = pre_status else { + return Some((kind, InstallAction::Install)); + }; + let action = status + .clis + .iter() + .find(|c| c.name == kind.name()) + .map_or(InstallAction::Install, decide_install_action); + tracing::info!( + target: "agent_hooks", + cli = kind.name(), + action = ?action, + "hook install plan", + ); + match action { + InstallAction::Skip => None, + other => Some((kind, other)), + } + }) + .collect() +} + /// Fold the two independent failure signals and the post-install status /// check into one per-CLI verdict. /// @@ -355,6 +428,7 @@ fn yn(b: bool) -> &'static str { mod tests { use super::{ build_install_report, format_bundle_source, format_install_failure, format_version_column, + plan_install, }; use crate::agent_hooks_installer::{ BundleSourceInfo, CliScope, CliStatus, InstallFailure, StatusReport, @@ -681,4 +755,116 @@ mod tests { build_install_report(CliScope::All, &status_of(vec![]), &no_failures(), &[]); assert_eq!(report.schema_version, 1); } + + // ---- `--only-missing` planning --------------------------------------- + + fn installed_cli(name: &'static str, version: &str) -> CliStatus { + CliStatus { + installed_version: Some(version.to_string()), + ..cli_with_bundle(name, Some(version)) + } + } + + /// The `--only-missing` contract the Settings "Install hooks" button relies + /// on: complete-and-current CLIs drop out, out-of-date ones are routed to + /// the upgrade flow (an install would no-op), and everything incomplete is + /// installed. + #[test] + fn only_missing_plans_skip_upgrade_and_install_separately() { + use crate::agent_hooks_installer::{CliKind, InstallAction}; + + let status = status_of(vec![ + installed_cli("copilot", "0.1.6"), + // Complete but a release behind — `plugin install` would answer + // "already installed", so this has to go through `plugin update`. + CliStatus { + installed_version: Some("0.1.5".to_string()), + ..cli_with_bundle("claude", Some("0.1.6")) + }, + // Marketplace registered but the plugin never landed. + CliStatus { + plugin_installed: false, + plugin_enabled: false, + ..cli_with_bundle("gemini", Some("0.1.6")) + }, + // Present but disabled — a partial state the button repairs. + CliStatus { + plugin_enabled: false, + ..installed_cli("codex", "0.1.6") + }, + absent_cli("opencode"), + ]); + + assert_eq!( + plan_install(CliScope::All, Some(&status)), + vec![ + (CliKind::Claude, InstallAction::Upgrade), + (CliKind::Gemini, InstallAction::Install), + (CliKind::Codex, InstallAction::Install), + (CliKind::OpenCode, InstallAction::Install), + ], + ); + } + + /// A CLI the status pass never reported on is unknown, not installed. + /// Absent evidence must fall back to doing the work. + #[test] + fn only_missing_installs_clis_absent_from_the_status_report() { + use crate::agent_hooks_installer::{CliKind, InstallAction}; + + let status = status_of(vec![installed_cli("copilot", "0.1.6")]); + + assert_eq!( + plan_install(CliScope::All, Some(&status)), + vec![ + (CliKind::Claude, InstallAction::Install), + (CliKind::Gemini, InstallAction::Install), + (CliKind::Codex, InstallAction::Install), + (CliKind::OpenCode, InstallAction::Install), + ], + ); + } + + /// Without the flag, `wta hooks install` stays a full (re)install — the + /// escape hatch for a break that status can't see. It must never plan an + /// upgrade, because it has no status to base one on. + #[test] + fn a_plain_install_plans_install_for_every_in_scope_cli() { + use crate::agent_hooks_installer::{CliKind, InstallAction}; + + assert_eq!( + plan_install(CliScope::All, None), + CliKind::ALL + .iter() + .map(|k| (*k, InstallAction::Install)) + .collect::>() + ); + assert_eq!( + plan_install(CliScope::One(CliKind::Codex), None), + vec![(CliKind::Codex, InstallAction::Install)] + ); + } + + /// Scope wins over state in both directions: another CLI needing work must + /// not widen a `--cli` run, and a complete CLI must still be skipped when + /// it is the one named. + #[test] + fn only_missing_respects_a_single_cli_scope() { + use crate::agent_hooks_installer::{CliKind, InstallAction}; + + let status = status_of(vec![ + CliStatus { + plugin_installed: false, + plugin_enabled: false, + ..cli_with_bundle("copilot", Some("0.1.6")) + }, + installed_cli("codex", "0.1.6"), + ]); + + assert!(plan_install(CliScope::One(CliKind::Codex), Some(&status)).is_empty()); + assert_eq!( + plan_install(CliScope::One(CliKind::Copilot), Some(&status)), + vec![(CliKind::Copilot, InstallAction::Install)] + ); + } } diff --git a/tools/wta/src/cli/mod.rs b/tools/wta/src/cli/mod.rs index 1d87e3fd6..c13f3fc1b 100644 --- a/tools/wta/src/cli/mod.rs +++ b/tools/wta/src/cli/mod.rs @@ -60,7 +60,9 @@ pub(crate) async fn run(command: Command, json_mode: bool) -> Result<()> { } }, Command::Hooks { action } => match action { - HooksAction::Install { cli } => hooks::run_install(cli, json_mode), + HooksAction::Install { cli, only_missing } => { + hooks::run_install(cli, only_missing, json_mode) + } HooksAction::Status => hooks::run_status(json_mode), HooksAction::Uninstall { cli } => hooks::run_uninstall(cli, json_mode), }, diff --git a/tools/wta/src/cli_tests.rs b/tools/wta/src/cli_tests.rs index cd8f38146..64f1bfe86 100644 --- a/tools/wta/src/cli_tests.rs +++ b/tools/wta/src/cli_tests.rs @@ -232,7 +232,7 @@ fn process_label_subcommands() { let probe_sources = Cli::try_parse_from(["wta", "probe-agent-sources", "--wsl-distro", "Ubuntu-24.04"]) - .unwrap(); + .unwrap(); assert_eq!(process_label(&probe_sources), "probe"); assert!(Cli::try_parse_from(["wta", "probe-agent-sources"]).is_err()); @@ -322,6 +322,31 @@ fn hooks_cli_filter_into_scope_maps_each_variant() { )); } +/// `--only-missing` is the Settings "Install hooks" button's contract with +/// wta. It must stay opt-in: a bare `wta hooks install` remains the full +/// (re)install a user reaches for when something is broken. +#[test] +fn hooks_install_only_missing_is_opt_in() { + use crate::cli::args::HooksAction; + + let default = Cli::try_parse_from(["wta", "hooks", "install"]).expect("flags must parse"); + match default.command { + Some(Command::Hooks { + action: HooksAction::Install { only_missing, .. }, + }) => assert!(!only_missing), + other => panic!("expected Command::Hooks/Install, got {other:?}"), + } + + let opted = Cli::try_parse_from(["wta", "hooks", "install", "--only-missing"]) + .expect("flags must parse"); + match opted.command { + Some(Command::Hooks { + action: HooksAction::Install { only_missing, .. }, + }) => assert!(only_missing), + other => panic!("expected Command::Hooks/Install, got {other:?}"), + } +} + // ── json_str_or_num: tolerant scalar extraction for human table rows ───────── #[test] diff --git a/tools/wta/src/helper/runtime.rs b/tools/wta/src/helper/runtime.rs index 7d383fc4e..3f3b220f7 100644 --- a/tools/wta/src/helper/runtime.rs +++ b/tools/wta/src/helper/runtime.rs @@ -14,9 +14,7 @@ use std::sync::Arc; use crate::shell::wt_channel::{CliChannel, WtChannel}; use crate::shell::ShellManager; -use crate::{ - agent_hooks_installer, agent_registry, app, event, logging, protocol, shell, -}; +use crate::{agent_registry, app, event, logging, protocol, shell}; use super::config::{HelperConfig, InitialView}; @@ -921,25 +919,6 @@ async fn run_acp_app( let _ = event_tx.send(app::AppEvent::PreflightComplete(preflight_result)); } - // ── install-hooks request channel ───────────────────────────── - // The Settings UI / in-TUI install button signals via this - // channel; main.rs runs `agent_hooks_installer::ensure_installed` - // off the UI thread so the TUI stays responsive. - let (install_req_tx, mut install_req_rx) = - tokio::sync::mpsc::unbounded_channel::<()>(); - tokio::task::spawn_local(async move { - while let Some(()) = install_req_rx.recv().await { - tracing::info!(target: "install_hooks", "received install request"); - // Run the (potentially slow, IO-bound) installer on the - // blocking pool so we don't park the LocalSet. - let _ = tokio::task::spawn_blocking(|| { - agent_hooks_installer::ensure_installed(); - }) - .await; - } - }); - app_state.set_install_request_tx(install_req_tx); - // Wire the agent_event channel so dispatch_resume's split-pane // background callback can post AgentSessionEvent (specifically // ResumePaneAssigned) back into the event loop. diff --git a/tools/wta/wt-agent-hooks/README.md b/tools/wta/wt-agent-hooks/README.md index fe29c2d2b..ea163ae83 100644 --- a/tools/wta/wt-agent-hooks/README.md +++ b/tools/wta/wt-agent-hooks/README.md @@ -43,11 +43,13 @@ Claude and Copilot share the same plugin manifest and event schema. ## How install works -The `wta` binary auto-installs each CLI on startup via -`agent_hooks_installer::ensure_installed()`: +Installation is always explicit — the Settings "Install hooks" button, the +first-run setup flow, or `wta hooks install`. Nothing installs hooks on an +ordinary `wta` startup. Each entry point ends up in +`agent_hooks_installer::apply_install_plan()`, which dispatches per CLI: ``` - wta startup + wta hooks install │ ┌───────────────┼───────────────┐ ▼ ▼ ▼ @@ -71,6 +73,11 @@ claude/ copilot/ gemini-extension/ @wt-local @wt-local ``` +Keeping an already-installed bridge at the bundled version is a separate +concern: `upgrade_installed_hooks()` runs at `wta-master` startup and uses +each CLI's own update command, because a second `install` is a no-op once +the plugin is registered. + OpenCode has no separate hook marketplace. `wta hooks install --cli opencode` copies `wt-agent-hooks.js` into `%XDG_CONFIG_HOME%\opencode\plugins\` when `XDG_CONFIG_HOME` is set, or `%USERPROFILE%\.config\opencode\plugins\`