Skip already-current CLIs when Settings installs agent hooks

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

src/cascadia/TerminalSettingsEditor/AIAgentsViewModel.cpp

@@ -1151,9 +1151,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.
@@ -1328,7 +1328,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()

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

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;
 
@@ -387,21 +386,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<Option<String>, 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.

tools/wta/src/agent_hooks_installer.rs

@@ -91,7 +91,7 @@
 // Public surface for `wta hooks <action>` (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<String>| v.as_deref().and_then(|s| s.parse::<Version>().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<InstallFailure> {
+/// 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<InstallFailure> {
     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<Version>) -> 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<Version>,
+) -> 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<Version>) -
                 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<Version>) -
         "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<Version>) -
         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()
+        ))
     }
 }