Add Reinstall skill button to Settings → Corveil CLI (CROW-491)

Packages/CrowUI/Sources/CrowUI/SettingsView.swift

@@ -24,21 +24,31 @@ public struct SettingsView: View {
     @State private var corveilVerifyResult: String?
     /// True while the Verify button's subprocess is in flight.
     @State private var corveilVerifying: Bool = false
+    /// Live result of the most recent "Reinstall skill" click. Shares the
+    /// `✓ … / ✗ …` convention with `corveilVerifyResult` and is rendered in
+    /// the same inline result line (only one operation runs at a time). Set
+    /// to `nil` on path edits so stale results don't outlive a binary swap.
+    @State private var corveilReinstallResult: String?
+    /// True while the Reinstall skill button's subprocess is in flight.
+    @State private var corveilReinstalling: Bool = false
 
     public var onSave: ((String, AppConfig) -> Void)?
     public var onRescaffold: ((String) -> Void)?
     /// Fired when the user commits a new value into the corveil picker
-    /// (Browse confirm or Enter on the TextField), so AppDelegate can
-    /// re-run just `Scaffolder.installCorveilSkill` instead of waiting for
-    /// the next app restart (CROW-490). `nil` argument means "the user
-    /// cleared the field" — the install is a no-op then but the caller
-    /// still gets the signal to clear any stale warning banner.
-    public var onCorveilReinstall: ((String?) -> Void)?
+    /// (Browse confirm, Enter on the TextField) or clicks "Reinstall skill",
+    /// so AppDelegate can re-run just `Scaffolder.installCorveilSkill`
+    /// instead of waiting for the next app restart (CROW-490, CROW-491).
+    /// `nil` argument means "the user cleared the field" — the install is
+    /// a no-op then but the caller still gets the signal to clear any stale
+    /// warning banner. Returns the warning string (`nil` on success) so the
+    /// Reinstall button can show inline `✓ / ✗` feedback; picker-change
+    /// callers may ignore the return.
+    public var onCorveilReinstall: ((String?) async -> String?)?
 
     public init(appState: AppState, devRoot: String, config: AppConfig,
                 onSave: ((String, AppConfig) -> Void)? = nil,
                 onRescaffold: ((String) -> Void)? = nil,
-                onCorveilReinstall: ((String?) -> Void)? = nil) {
+                onCorveilReinstall: ((String?) async -> String?)? = nil) {
         self.appState = appState
         self._devRoot = State(initialValue: devRoot)
         self._config = State(initialValue: config)
@@ -276,28 +286,48 @@ public struct SettingsView: View {
                     TextField("Path to corveil binary", text: corveilBinding)
                         .textFieldStyle(.roundedBorder)
                         .onSubmit { commitCorveilPath() }
+                        // Typing/pasting a new path into the field (not just
+                        // Browse) also makes prior results stale. Watch the
+                        // binding's source-of-truth — the config dict slot —
+                        // and clear both inline results when it changes.
+                        .onChange(of: config.defaults.binaries["corveil"] ?? "") { _, _ in
+                            corveilVerifyResult = nil
+                            corveilReinstallResult = nil
+                        }
                     Button("Browse...") {
                         let panel = NSOpenPanel()
                         panel.canChooseFiles = true
                         panel.canChooseDirectories = false
                         panel.allowsMultipleSelection = false
                         if panel.runModal() == .OK, let url = panel.url {
+                            // Mutating the binding triggers the .onChange
+                            // above, which clears stale verify/reinstall
+                            // results — no manual clears needed here.
                             corveilBinding.wrappedValue = url.path
-                            // Clear stale verify result — it's about a previous binary.
-                            corveilVerifyResult = nil
                             commitCorveilPath()
                         }
                     }
                     Button(corveilVerifying ? "Verifying…" : "Verify") { verifyCorveil() }
-                        .disabled(corveilBinding.wrappedValue.isEmpty || corveilVerifying)
+                        .disabled(corveilBinding.wrappedValue.isEmpty || corveilVerifying || corveilReinstalling)
+                    Button(corveilReinstalling ? "Reinstalling…" : "Reinstall skill") {
+                        reinstallCorveilSkill()
+                    }
+                    .disabled(corveilBinding.wrappedValue.isEmpty || corveilVerifying || corveilReinstalling)
+                    .help(corveilBinding.wrappedValue.isEmpty
+                          ? "Set the Corveil CLI path first."
+                          : "Reinstall the bundled /query-corveil skill from this binary — picks up a rebuilt corveil without restarting Crow.")
                 }
-                if let result = corveilVerifyResult {
+                // Single result line — coalesces Verify and Reinstall output.
+                // Only one operation runs at a time (mutual `disabled`), and
+                // newer clicks clear the older result first, so there's no
+                // ambiguity about which click this line refers to.
+                if let result = corveilReinstallResult ?? corveilVerifyResult {
                     Text(result)
                         .font(.caption)
                         .foregroundStyle(result.hasPrefix("✓") ? .green : .orange)
                         .textSelection(.enabled)
                 }
-                Text("On launch, Crow runs `corveil skill install --path` to install the `/query-corveil` slash command into this devRoot. Leave blank to skip.")
+                Text("On launch, Crow runs `corveil skill install --path` to install the `/query-corveil` slash command into this devRoot. Use **Reinstall skill** after rebuilding corveil to pick up the new embedded skill without restarting Crow. Leave blank to skip.")
                     .font(.caption)
                     .foregroundStyle(.secondary)
             }
@@ -615,7 +645,11 @@ public struct SettingsView: View {
     private func commitCorveilPath() {
         save()
         let path = corveilBinding.wrappedValue
-        onCorveilReinstall?(path.isEmpty ? nil : path)
+        // Picker commits are fire-and-forget — the closure now returns the
+        // install warning, but `corveilSkillInstallWarning` is already
+        // updated inside the closure for the banner, and there is no
+        // inline result line for picker-driven installs.
+        Task { _ = await onCorveilReinstall?(path.isEmpty ? nil : path) }
     }
 
     /// Two-way binding into `config.defaults.binaries["corveil"]` that treats
@@ -644,6 +678,9 @@ public struct SettingsView: View {
         guard !path.isEmpty else { return }
         corveilVerifying = true
         corveilVerifyResult = nil
+        // Clear the older reinstall result so the coalesced result line
+        // doesn't shadow this verify with stale output.
+        corveilReinstallResult = nil
         Task.detached {
             let result = SettingsView.runCorveilVersion(at: path)
             await MainActor.run {
@@ -653,6 +690,46 @@ public struct SettingsView: View {
         }
     }
 
+    /// Re-run `corveil skill install --path …` on demand — the same flow as
+    /// the per-launch path (CROW-482), without requiring a restart, a
+    /// workspace switch, or re-picking the binary (CROW-491). The common
+    /// loop this serves is "I rebuilt corveil locally; install the new
+    /// embedded skill." Goes through the existing `onCorveilReinstall`
+    /// hook (also used by picker commits, CROW-490) so the click serializes
+    /// behind any in-flight install via `AppDelegate.enqueueCorveilInstall`.
+    /// The closure offloads blocking work to a detached task internally, so
+    /// awaiting it from the main actor doesn't pin the UI for the install
+    /// timeout. `appState.corveilSkillInstallWarning` is already updated
+    /// inside the closure, so we only need to set the inline result line.
+    private func reinstallCorveilSkill() {
+        let path = corveilBinding.wrappedValue
+        guard !path.isEmpty else { return }
+        corveilReinstalling = true
+        corveilReinstallResult = nil
+        // Clear the older verify result so the coalesced result line
+        // doesn't shadow this reinstall with stale output.
+        corveilVerifyResult = nil
+        Task {
+            // Distinguish "closure returned nil (real success)" from "closure
+            // unwired (previews/tests)" — otherwise the unwired case would
+            // print a false `✓ Skill reinstalled` for an install that never
+            // ran. Explicit guard inside the Task so we don't capture a
+            // non-Sendable closure across the actor boundary.
+            guard let callback = onCorveilReinstall else {
+                corveilReinstallResult = "✗ Reinstall unavailable in this build (callback not wired)."
+                corveilReinstalling = false
+                return
+            }
+            let warning = await callback(path)
+            if let warning {
+                corveilReinstallResult = "✗ \(warning)"
+            } else {
+                corveilReinstallResult = "✓ Skill reinstalled"
+            }
+            corveilReinstalling = false
+        }
+    }
+
     /// Pure helper for `verifyCorveil` — easier to reason about off the main
     /// actor and trivially testable. Returns a single-line summary suitable
     /// for inline display.

Sources/Crow/App/AppDelegate.swift

@@ -61,7 +61,7 @@ final class AppDelegate: NSObject, NSApplicationDelegate {
     /// completion clobbering a fresher banner with a stale one). The last
     /// committed path is also the last to write the banner. See the
     /// CROW-490 review for the race this replaced.
-    private var corveilInstallTail: Task<Void, Never>?
+    private var corveilInstallTail: Task<String?, Never>?
 
     func applicationDidFinishLaunching(_ notification: Notification) {
         // Must be the very first call so the next exit (graceful or not)
@@ -198,27 +198,37 @@ final class AppDelegate: NSObject, NSApplicationDelegate {
     }
 
     /// Hot-trigger a single `corveil skill install` run for a path the user
-    /// just committed in Settings (CROW-490). Mirrors the
-    /// `reviewKickoffTail` pattern: writes to `corveilInstallTail` happen
-    /// on the main actor, and each task awaits its predecessor before
-    /// running, so the last-committed path is also the last to update the
-    /// banner. The blocking subprocess runs in a nested `Task.detached`
-    /// (bounded by `Scaffolder.corveilInstallTimeout`) so it can't freeze
-    /// the Settings window, then the result is assigned back on main.
-    /// `nil` path is a deliberate no-op for the subprocess but still
-    /// clears any stale warning, matching the launch-time scaffolder's
-    /// "always assign" semantics at the `onRescaffold` call site.
+    /// just committed in Settings (CROW-490) or clicked Reinstall on
+    /// (CROW-491). Mirrors the `reviewKickoffTail` pattern: writes to
+    /// `corveilInstallTail` happen on the main actor, and each task awaits
+    /// its predecessor before running, so the last-committed path is also
+    /// the last to update the banner. The blocking subprocess runs in a
+    /// nested `Task.detached` (bounded by `Scaffolder.corveilInstallTimeout`)
+    /// so it can't freeze the Settings window, then the result is assigned
+    /// back on main. `nil` path is a deliberate no-op for the subprocess
+    /// but still clears any stale warning, matching the launch-time
+    /// scaffolder's "always assign" semantics at the `onRescaffold` call site.
+    ///
+    /// Returns the warning produced by this specific call (not whichever
+    /// task happens to be tail). Picker-change callers may ignore it;
+    /// the Reinstall button awaits it to show inline `✓ / ✗` feedback.
     @MainActor
-    private func enqueueCorveilInstall(path: String?, devRoot: String) {
+    @discardableResult
+    private func enqueueCorveilInstall(path: String?, devRoot: String) async -> String? {
         let previous = corveilInstallTail
-        corveilInstallTail = Task { @MainActor [weak self] in
-            await previous?.value
+        let myTask = Task<String?, Never> { @MainActor [weak self] in
+            // Serialize behind any in-flight install. We don't care about the
+            // predecessor's warning — each task reports its own result.
+            _ = await previous?.value
             let warning = await Task.detached {
                 let scaffolder = Scaffolder(devRoot: devRoot)
                 return scaffolder.installCorveilSkill(path)
             }.value
             self?.appState.corveilSkillInstallWarning = warning
+            return warning
         }
+        corveilInstallTail = myTask
+        return await myTask.value
     }
 
     // MARK: - tmux watchdog alert
@@ -457,6 +467,13 @@ final class AppDelegate: NSObject, NSApplicationDelegate {
             NSLog("[Crow] Scaffold update failed: %@", error.localizedDescription)
         }
 
+        // Settings → Corveil CLI → "Reinstall skill" (issue #491) reuses
+        // the existing `onCorveilReinstall` hook wired in `showSettings`,
+        // which already routes through `enqueueCorveilInstall` so the
+        // button click serializes correctly against any in-flight picker
+        // commit (CROW-490). No separate AppState callback needed.
+
+
         // Codex-specific dev-root and global config — only when Codex is
         // registered. AGENTS.md goes into devRoot; hooks.json + config.toml
         // go into ~/.codex (or $CODEX_HOME). All idempotent; safe to re-run.
@@ -1135,16 +1152,18 @@ final class AppDelegate: NSObject, NSApplicationDelegate {
                 }
             },
             onCorveilReinstall: { [weak self] newPath in
-                // CROW-490: when the user commits a new corveil binary path in
-                // Settings, hot-trigger just the `corveil skill install` step —
-                // not the whole Scaffolder pass — so `/query-corveil` reflects
-                // the new binary without requiring an app restart.
+                // Hot-trigger a single `corveil skill install` run, serialized
+                // through `corveilInstallTail` against any in-flight install
+                // (mirrors the `reviewKickoffTail` pattern). Two paths reach
+                // this closure: the user committing a new path in the picker
+                // (CROW-490) and the user clicking "Reinstall skill" (CROW-491).
                 //
-                // `enqueueCorveilInstall` serializes back-to-back picks
-                // through `corveilInstallTail` (mirrors the `reviewKickoffTail`
-                // pattern) so two rapid commits can't race on
-                // `query-corveil.md` or on the warning banner.
-                self?.enqueueCorveilInstall(path: newPath, devRoot: devRoot)
+                // Read `self.devRoot` live, not the launch-time / show-time
+                // capture: `saveSettings(devRoot:config:)` mutates it in the
+                // same Settings window, and the stale capture would silently
+                // install into the previous devRoot while reporting success.
+                guard let self, let currentDevRoot = self.devRoot else { return nil }
+                return await self.enqueueCorveilInstall(path: newPath, devRoot: currentDevRoot)
             }
         )
 

Sources/Crow/App/Scaffolder.swift

@@ -228,10 +228,14 @@ struct Scaffolder {
     /// after `corveilInstallTimeout` seconds the process is sent SIGTERM and
     /// the install reports a warning rather than blocking forever.
     ///
-    /// Settings can also call this directly (CROW-490) when the user picks a
-    /// new corveil binary, to avoid the "must restart" gap. Those callers
-    /// dispatch the call off the main thread (`Task.detached`) so the 5s
-    /// worst-case doesn't freeze the Settings window.
+    /// Internal, not private — Settings calls this directly via callbacks
+    /// wired in `AppDelegate.launchMainApp`: when the user picks a new
+    /// corveil binary (CROW-490) and when the user clicks "Reinstall skill"
+    /// (CROW-491). Both avoid the "must restart Crow" gap of the per-launch
+    /// path. SettingsView itself lives in CrowUI and cannot import the app
+    /// target, so closure injection through `AppState` is the only path.
+    /// Settings-side callers dispatch off the main thread (`Task.detached`)
+    /// so the 5s worst-case doesn't freeze the Settings window.
     func installCorveilSkill(_ corveilBinaryPath: String?) -> String? {
         guard let path = corveilBinaryPath?.trimmingCharacters(in: .whitespacesAndNewlines),
               !path.isEmpty else {