From 2fd3f4e00690d7a334bf6ae5d75a7e01fecfc02e Mon Sep 17 00:00:00 2001 From: Danny Gershman Date: Thu, 11 Jun 2026 19:26:19 -0400 Subject: [PATCH 1/3] =?UTF-8?q?Add=20Reinstall=20skill=20button=20to=20Set?= =?UTF-8?q?tings=20=E2=86=92=20Corveil=20CLI=20(CROW-491)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The launch-time path (#483) installs the embedded /query-corveil skill into the devroot via `corveil skill install`. After rebuilding corveil locally, the embedded skill version drifts from what's installed in the devroot, and the only ways to fix it were heavy: restart Crow (closes every session), switch workspaces, or re-pick the same path in Settings. Add a dedicated "Reinstall skill" button next to the existing Verify button that runs the same install flow on demand. Disabled when the picker is unset; surfaces βœ“/βœ— inline (coalesced with Verify's result line β€” only one operation runs at a time); also updates the launch-time warning banner so "is corveil broken?" has a single source of truth. Plumbing: SettingsView lives in the CrowUI package which can't import the Crow app target where Scaffolder lives, so the call goes through a new `AppState.onReinstallCorveilSkill` async closure wired in AppDelegate (matches the existing on* callback pattern). The closure offloads the synchronous Process.waitUntilExit work to a detached task internally so awaiting it from the main actor doesn't pin the UI for the install timeout. `Scaffolder.installCorveilSkill` access is loosened from private to internal so AppDelegate can call it on a fresh Scaffolder instance. Closes #491 πŸ¦β€β¬› Generated with Claude Code, orchestrated by Crow Co-Authored-By: Claude Crow-Session: F2C4CB9B-B99D-4C06-AD44-E04F5F5E11A4 --- .../CrowCore/Sources/CrowCore/AppState.swift | 10 +++ .../CrowUI/Sources/CrowUI/SettingsView.swift | 69 +++++++++++++++++-- Sources/Crow/App/AppDelegate.swift | 14 ++++ Sources/Crow/App/Scaffolder.swift | 12 ++-- 4 files changed, 97 insertions(+), 8 deletions(-) diff --git a/Packages/CrowCore/Sources/CrowCore/AppState.swift b/Packages/CrowCore/Sources/CrowCore/AppState.swift index 8950ab2..9420dcf 100644 --- a/Packages/CrowCore/Sources/CrowCore/AppState.swift +++ b/Packages/CrowCore/Sources/CrowCore/AppState.swift @@ -384,6 +384,16 @@ public final class AppState { /// Fire a job immediately, ignoring its enabled flag and schedule (job ID). public var onRunJob: ((UUID) -> Void)? + /// Triggered by Settings β†’ General β†’ Corveil CLI β†’ "Reinstall skill" + /// (issue #491). Wired by `AppDelegate.launchMainApp` to construct a + /// fresh `Scaffolder` and call `installCorveilSkill(path)` β€” the same + /// flow as the per-launch install, on demand. Returns `nil` on success, + /// a user-facing warning string on failure. The closure is `async` so + /// the wiring can offload the synchronous `Process.waitUntilExit` work + /// to a detached task without the call site blocking the main actor + /// for the 5-second install timeout. + public var onReinstallCorveilSkill: ((String?) async -> String?)? + // MARK: - Computed Properties public var selectedSession: Session? { diff --git a/Packages/CrowUI/Sources/CrowUI/SettingsView.swift b/Packages/CrowUI/Sources/CrowUI/SettingsView.swift index 370f320..1a5ce1b 100644 --- a/Packages/CrowUI/Sources/CrowUI/SettingsView.swift +++ b/Packages/CrowUI/Sources/CrowUI/SettingsView.swift @@ -24,6 +24,13 @@ 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)? @@ -283,21 +290,33 @@ public struct SettingsView: View { panel.allowsMultipleSelection = false if panel.runModal() == .OK, let url = panel.url { corveilBinding.wrappedValue = url.path - // Clear stale verify result β€” it's about a previous binary. + // Clear stale results β€” they're about a previous binary. corveilVerifyResult = nil + corveilReinstallResult = 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) } @@ -644,6 +663,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 +675,45 @@ 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 `AppState.onReinstallCorveilSkill` + /// because SettingsView (CrowUI) can't import the app target where + /// `Scaffolder` lives. The closure is async and offloads its blocking + /// work to a detached task internally, so awaiting it from the main + /// actor doesn't pin the UI for the install timeout. + 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 { + // Closure may not be wired in unit tests / previews β€” degrade + // gracefully. Mirrors the `onListWorkspaceRepos` invocation + // pattern used elsewhere in this view. + let warning = await appState.onReinstallCorveilSkill?(path) + if let warning { + corveilReinstallResult = "βœ— \(warning)" + // Mirror the launch-time surface so the orange banner at + // the top of General also reflects the latest attempt β€” + // keeps a single source of truth for "is corveil broken?" + appState.corveilSkillInstallWarning = warning + } else { + corveilReinstallResult = "βœ“ Skill reinstalled" + // A successful manual reinstall clears any stale + // launch-time warning β€” if the user has just made it + // work, they shouldn't keep staring at the old banner. + appState.corveilSkillInstallWarning = nil + } + 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..f25b386 100644 --- a/Sources/Crow/App/AppDelegate.swift +++ b/Sources/Crow/App/AppDelegate.swift @@ -457,6 +457,20 @@ final class AppDelegate: NSObject, NSApplicationDelegate { NSLog("[Crow] Scaffold update failed: %@", error.localizedDescription) } + // Settings β†’ Corveil CLI β†’ "Reinstall skill" (issue #491). Runs the + // same `corveil skill install` flow as the per-launch path, on + // demand. Captures `devRoot` (a let) β€” Scaffolder is a value-type + // struct so we construct fresh per click. The closure is async and + // offloads the synchronous `Process.waitUntilExit` work to a + // detached task; SettingsView awaits this without blocking the + // main actor for the install timeout. + appState.onReinstallCorveilSkill = { path in + await Task.detached(priority: .userInitiated) { + let scaffolder = Scaffolder(devRoot: devRoot) + return scaffolder.installCorveilSkill(path) + }.value + } + // 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. 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 { From 03d48dcf98d8c8721b01846d109f17286fceb856 Mon Sep 17 00:00:00 2001 From: Danny Gershman Date: Thu, 11 Jun 2026 19:39:14 -0400 Subject: [PATCH 2/3] Address PR #498 review: live devRoot, false-positive guard, clear on edit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 1. Read self.devRoot live on each Reinstall click (Yellow blocker): the launch-time `let devRoot` capture went stale when the user edits Development Root in the same Settings window, silently installing into the previous devRoot while reporting success. Now mirrors the `onListWorkspaceRepos` pattern β€” `[weak self]` and reads `self.devRoot` from the main actor before dispatching to the detached task. 2. Guard against unwired closure to prevent false βœ“ in previews (Green): when AppDelegate hasn't wired `onReinstallCorveilSkill`, `await callback?(path)` returns nil and falls into the success branch, printing "Skill reinstalled" for an install that never ran. Explicit `guard let callback` inside the Task now reports "Reinstall unavailable in this build" instead. 3. Clear inline results when the path binding changes (Green): the doc comment promised path-edit clearing but only Browse did it. Typed/pasted edits to the TextField left a stale βœ“/βœ— visible against the new path. `.onChange` of the binding's source-of-truth (the config dict slot) now clears both results for any edit path; Browse's redundant manual clears are removed. πŸ¦β€β¬› Generated with Claude Code, orchestrated by Crow Co-Authored-By: Claude Crow-Session: F2C4CB9B-B99D-4C06-AD44-E04F5F5E11A4 --- .../CrowUI/Sources/CrowUI/SettingsView.swift | 29 ++++++++++++++----- Sources/Crow/App/AppDelegate.swift | 20 ++++++++----- 2 files changed, 34 insertions(+), 15 deletions(-) diff --git a/Packages/CrowUI/Sources/CrowUI/SettingsView.swift b/Packages/CrowUI/Sources/CrowUI/SettingsView.swift index 1a5ce1b..9da1205 100644 --- a/Packages/CrowUI/Sources/CrowUI/SettingsView.swift +++ b/Packages/CrowUI/Sources/CrowUI/SettingsView.swift @@ -283,16 +283,24 @@ 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 results β€” they're about a previous binary. - corveilVerifyResult = nil - corveilReinstallResult = nil commitCorveilPath() } } @@ -693,10 +701,17 @@ public struct SettingsView: View { // doesn't shadow this reinstall with stale output. corveilVerifyResult = nil Task { - // Closure may not be wired in unit tests / previews β€” degrade - // gracefully. Mirrors the `onListWorkspaceRepos` invocation - // pattern used elsewhere in this view. - let warning = await appState.onReinstallCorveilSkill?(path) + // 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 = appState.onReinstallCorveilSkill else { + corveilReinstallResult = "βœ— Reinstall unavailable in this build (callback not wired)." + corveilReinstalling = false + return + } + let warning = await callback(path) if let warning { corveilReinstallResult = "βœ— \(warning)" // Mirror the launch-time surface so the orange banner at diff --git a/Sources/Crow/App/AppDelegate.swift b/Sources/Crow/App/AppDelegate.swift index f25b386..dbe6448 100644 --- a/Sources/Crow/App/AppDelegate.swift +++ b/Sources/Crow/App/AppDelegate.swift @@ -459,14 +459,18 @@ final class AppDelegate: NSObject, NSApplicationDelegate { // Settings β†’ Corveil CLI β†’ "Reinstall skill" (issue #491). Runs the // same `corveil skill install` flow as the per-launch path, on - // demand. Captures `devRoot` (a let) β€” Scaffolder is a value-type - // struct so we construct fresh per click. The closure is async and - // offloads the synchronous `Process.waitUntilExit` work to a - // detached task; SettingsView awaits this without blocking the - // main actor for the install timeout. - appState.onReinstallCorveilSkill = { path in - await Task.detached(priority: .userInitiated) { - let scaffolder = Scaffolder(devRoot: devRoot) + // demand. Reads `self.devRoot` live (not the captured launch-time + // `let devRoot`) because Settings can mutate devRoot via + // `saveSettings(devRoot:config:)` in the same window β€” capturing + // the launch-time value would silently install into the previous + // devRoot. Mirrors the `onListWorkspaceRepos` pattern. The closure + // is async and offloads the synchronous `Process.waitUntilExit` + // work to a detached task; SettingsView awaits this without + // blocking the main actor for the install timeout. + appState.onReinstallCorveilSkill = { [weak self] path in + guard let currentDevRoot = self?.devRoot else { return nil } + return await Task.detached(priority: .userInitiated) { + let scaffolder = Scaffolder(devRoot: currentDevRoot) return scaffolder.installCorveilSkill(path) }.value } From 155fefada9df8dd92655981da1dabb020c7b9435 Mon Sep 17 00:00:00 2001 From: Danny Gershman Date: Thu, 11 Jun 2026 19:53:41 -0400 Subject: [PATCH 3/3] Unify CROW-490 picker hook + CROW-491 Reinstall button on one closure MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit After rebasing onto #497 (CROW-490 picker-change hot-trigger), CROW-491 had two parallel install hooks pointing at the same Scaffolder call: the new AppState.onReinstallCorveilSkill and #490's SettingsView.onCorveilReinstall. Drop the duplicate and route the Reinstall button through #490's existing infrastructure: - Widen `SettingsView.onCorveilReinstall` from `((String?) -> Void)?` β†’ `((String?) async -> String?)?`. Picker commits fire-and-forget the return; the Reinstall button awaits it for inline βœ“/βœ— feedback. - Refactor `AppDelegate.enqueueCorveilInstall` to return the warning per call (each task reports its own result; serialization through `corveilInstallTail` is preserved). `corveilInstallTail` becomes `Task?`. - Read `self.devRoot` live inside the `onCorveilReinstall` wiring closure β€” fixes the same stale-capture bug both #490 and the v1 CROW-491 implementation had (saveSettings mutates devRoot in the same Settings window). - Drop `AppState.onReinstallCorveilSkill` and its launch-time wiring. The Reinstall button now naturally serializes behind any in-flight picker-driven install, which the original AppState-callback path bypassed. πŸ¦β€β¬› Generated with Claude Code, orchestrated by Crow Co-Authored-By: Claude Crow-Session: F2C4CB9B-B99D-4C06-AD44-E04F5F5E11A4 --- .../CrowCore/Sources/CrowCore/AppState.swift | 10 --- .../CrowUI/Sources/CrowUI/SettingsView.swift | 45 ++++++----- Sources/Crow/App/AppDelegate.swift | 81 ++++++++++--------- 3 files changed, 64 insertions(+), 72 deletions(-) diff --git a/Packages/CrowCore/Sources/CrowCore/AppState.swift b/Packages/CrowCore/Sources/CrowCore/AppState.swift index 9420dcf..8950ab2 100644 --- a/Packages/CrowCore/Sources/CrowCore/AppState.swift +++ b/Packages/CrowCore/Sources/CrowCore/AppState.swift @@ -384,16 +384,6 @@ public final class AppState { /// Fire a job immediately, ignoring its enabled flag and schedule (job ID). public var onRunJob: ((UUID) -> Void)? - /// Triggered by Settings β†’ General β†’ Corveil CLI β†’ "Reinstall skill" - /// (issue #491). Wired by `AppDelegate.launchMainApp` to construct a - /// fresh `Scaffolder` and call `installCorveilSkill(path)` β€” the same - /// flow as the per-launch install, on demand. Returns `nil` on success, - /// a user-facing warning string on failure. The closure is `async` so - /// the wiring can offload the synchronous `Process.waitUntilExit` work - /// to a detached task without the call site blocking the main actor - /// for the 5-second install timeout. - public var onReinstallCorveilSkill: ((String?) async -> String?)? - // MARK: - Computed Properties public var selectedSession: Session? { diff --git a/Packages/CrowUI/Sources/CrowUI/SettingsView.swift b/Packages/CrowUI/Sources/CrowUI/SettingsView.swift index 9da1205..ed2beb0 100644 --- a/Packages/CrowUI/Sources/CrowUI/SettingsView.swift +++ b/Packages/CrowUI/Sources/CrowUI/SettingsView.swift @@ -35,17 +35,20 @@ public struct SettingsView: View { 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) @@ -642,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 @@ -687,11 +694,13 @@ public struct SettingsView: View { /// 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 `AppState.onReinstallCorveilSkill` - /// because SettingsView (CrowUI) can't import the app target where - /// `Scaffolder` lives. The closure is async and offloads its blocking - /// work to a detached task internally, so awaiting it from the main - /// actor doesn't pin the UI for the install timeout. + /// 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 } @@ -706,7 +715,7 @@ public struct SettingsView: View { // 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 = appState.onReinstallCorveilSkill else { + guard let callback = onCorveilReinstall else { corveilReinstallResult = "βœ— Reinstall unavailable in this build (callback not wired)." corveilReinstalling = false return @@ -714,16 +723,8 @@ public struct SettingsView: View { let warning = await callback(path) if let warning { corveilReinstallResult = "βœ— \(warning)" - // Mirror the launch-time surface so the orange banner at - // the top of General also reflects the latest attempt β€” - // keeps a single source of truth for "is corveil broken?" - appState.corveilSkillInstallWarning = warning } else { corveilReinstallResult = "βœ“ Skill reinstalled" - // A successful manual reinstall clears any stale - // launch-time warning β€” if the user has just made it - // work, they shouldn't keep staring at the old banner. - appState.corveilSkillInstallWarning = nil } corveilReinstalling = false } diff --git a/Sources/Crow/App/AppDelegate.swift b/Sources/Crow/App/AppDelegate.swift index dbe6448..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,23 +467,12 @@ final class AppDelegate: NSObject, NSApplicationDelegate { NSLog("[Crow] Scaffold update failed: %@", error.localizedDescription) } - // Settings β†’ Corveil CLI β†’ "Reinstall skill" (issue #491). Runs the - // same `corveil skill install` flow as the per-launch path, on - // demand. Reads `self.devRoot` live (not the captured launch-time - // `let devRoot`) because Settings can mutate devRoot via - // `saveSettings(devRoot:config:)` in the same window β€” capturing - // the launch-time value would silently install into the previous - // devRoot. Mirrors the `onListWorkspaceRepos` pattern. The closure - // is async and offloads the synchronous `Process.waitUntilExit` - // work to a detached task; SettingsView awaits this without - // blocking the main actor for the install timeout. - appState.onReinstallCorveilSkill = { [weak self] path in - guard let currentDevRoot = self?.devRoot else { return nil } - return await Task.detached(priority: .userInitiated) { - let scaffolder = Scaffolder(devRoot: currentDevRoot) - return scaffolder.installCorveilSkill(path) - }.value - } + // 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 @@ -1153,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) } )