diff --git a/Packages/CrowUI/Sources/CrowUI/SettingsView.swift b/Packages/CrowUI/Sources/CrowUI/SettingsView.swift index 370f320..ed2beb0 100644 --- a/Packages/CrowUI/Sources/CrowUI/SettingsView.swift +++ b/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. diff --git a/Sources/Crow/App/AppDelegate.swift b/Sources/Crow/App/AppDelegate.swift index e5f2907..a1bc963 100644 --- a/Sources/Crow/App/AppDelegate.swift +++ b/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? + private var corveilInstallTail: Task? 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 { @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) } ) diff --git a/Sources/Crow/App/Scaffolder.swift b/Sources/Crow/App/Scaffolder.swift index b2cb46a..1a8e4f2 100644 --- a/Sources/Crow/App/Scaffolder.swift +++ b/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 {