From 7b4816bdbdcdf89064095c5dcd32cbeb85009c39 Mon Sep 17 00:00:00 2001 From: Danny Gershman Date: Thu, 11 Jun 2026 19:14:18 -0400 Subject: [PATCH 1/2] Hot-trigger corveil skill install when picker value changes (CROW-490) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously, picking a corveil binary in Settings persisted the path but did not re-run `corveil skill install` β€” the user had to quit and relaunch Crow before `/query-corveil` reflected the new binary. Now SettingsView fires a new `onCorveilReinstall` closure on the two commit sites (Browse confirm, TextField Enter), and AppDelegate dispatches just `Scaffolder.installCorveilSkill` off the main thread, routing the result through the existing `AppState.corveilSkillInstallWarning` banner. - Promote `Scaffolder.installCorveilSkill` from private to internal so AppDelegate can call it directly without re-running the full scaffold pass. - Add `onCorveilReinstall: ((String?) -> Void)?` to `SettingsView.init` and call it (with the new path, or `nil` when the field was cleared) alongside `save()` on Browse confirm and `onSubmit`. Verify stays `--version` only β€” no install side-effect there. - Wire the closure in `AppDelegate.showSettings` to run the install in `Task.detached` (bounded by the existing 5s `corveilInstallTimeout`) and write the result on the main actor. Always-assign behavior mirrors the adjacent `onRescaffold` flow so a cleared field clears any stale warning. πŸ¦β€β¬› Generated with Claude Code, orchestrated by Crow Co-Authored-By: Claude Crow-Session: 14B927FC-A2CB-43A9-B5C4-318BE4578E2B --- .../CrowUI/Sources/CrowUI/SettingsView.swift | 22 ++++++++++++++-- Sources/Crow/App/AppDelegate.swift | 25 +++++++++++++++++++ Sources/Crow/App/Scaffolder.swift | 7 +++++- 3 files changed, 51 insertions(+), 3 deletions(-) diff --git a/Packages/CrowUI/Sources/CrowUI/SettingsView.swift b/Packages/CrowUI/Sources/CrowUI/SettingsView.swift index 7696930..c15348a 100644 --- a/Packages/CrowUI/Sources/CrowUI/SettingsView.swift +++ b/Packages/CrowUI/Sources/CrowUI/SettingsView.swift @@ -27,15 +27,24 @@ 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)? public init(appState: AppState, devRoot: String, config: AppConfig, onSave: ((String, AppConfig) -> Void)? = nil, - onRescaffold: ((String) -> Void)? = nil) { + onRescaffold: ((String) -> Void)? = nil, + onCorveilReinstall: ((String?) -> Void)? = nil) { self.appState = appState self._devRoot = State(initialValue: devRoot) self._config = State(initialValue: config) self.onSave = onSave self.onRescaffold = onRescaffold + self.onCorveilReinstall = onCorveilReinstall } /// Names of all workspaces except the one currently being edited. @@ -266,7 +275,14 @@ public struct SettingsView: View { HStack { TextField("Path to corveil binary", text: corveilBinding) .textFieldStyle(.roundedBorder) - .onSubmit { save() } + .onSubmit { + save() + // Hot-trigger install on Enter β€” passes nil if the + // user committed an empty field so callers can + // clear any stale warning banner (CROW-490). + let path = corveilBinding.wrappedValue + onCorveilReinstall?(path.isEmpty ? nil : path) + } Button("Browse...") { let panel = NSOpenPanel() panel.canChooseFiles = true @@ -277,6 +293,8 @@ public struct SettingsView: View { save() // Clear stale verify result β€” it's about a previous binary. corveilVerifyResult = nil + // Hot-trigger install on Browse confirm (CROW-490). + onCorveilReinstall?(url.path) } } Button(corveilVerifying ? "Verifying…" : "Verify") { verifyCorveil() } diff --git a/Sources/Crow/App/AppDelegate.swift b/Sources/Crow/App/AppDelegate.swift index 4783a8a..4a99832 100644 --- a/Sources/Crow/App/AppDelegate.swift +++ b/Sources/Crow/App/AppDelegate.swift @@ -1099,6 +1099,31 @@ final class AppDelegate: NSObject, NSApplicationDelegate { self?.appState.corveilSkillInstallWarning = "Re-scaffold failed: \(error.localizedDescription)" } + }, + onCorveilReinstall: { [weak self] newPath in + // CROW-490: when the user commits a new corveil binary path in + // Settings, re-run just the `corveil skill install` step β€” + // not the whole Scaffolder pass β€” so `/query-corveil` reflects + // the new binary without requiring an app restart. + // + // `installCorveilSkill` calls `proc.waitUntilExit()` (bounded + // by `Scaffolder.corveilInstallTimeout`), so it must run off + // the main thread or it would freeze the Settings window for + // up to 5s. Result is always assigned: `nil` (success or + // empty/cleared path) clears any prior banner; non-nil shows + // the same diagnostic the launch-time path produces via the + // existing `AppState.corveilSkillInstallWarning` surface. + guard let self else { return } + // `devRoot` is the non-optional local from the outer guard at + // `showSettings()` line 1066 β€” captured here so the closure + // doesn't need to re-unwrap `self.devRoot`. + Task.detached { + let scaffolder = Scaffolder(devRoot: devRoot) + let warning = scaffolder.installCorveilSkill(newPath) + await MainActor.run { + self.appState.corveilSkillInstallWarning = warning + } + } } ) diff --git a/Sources/Crow/App/Scaffolder.swift b/Sources/Crow/App/Scaffolder.swift index fa4272a..b2cb46a 100644 --- a/Sources/Crow/App/Scaffolder.swift +++ b/Sources/Crow/App/Scaffolder.swift @@ -227,7 +227,12 @@ struct Scaffolder { /// window drawn yet. The hard wall-clock timeout bounds the worst case: /// after `corveilInstallTimeout` seconds the process is sent SIGTERM and /// the install reports a warning rather than blocking forever. - private func installCorveilSkill(_ corveilBinaryPath: String?) -> String? { + /// + /// 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. + func installCorveilSkill(_ corveilBinaryPath: String?) -> String? { guard let path = corveilBinaryPath?.trimmingCharacters(in: .whitespacesAndNewlines), !path.isEmpty else { return nil From 060e09b6ce9ce0b5ed80816fbdaea8b9dcc3a98c Mon Sep 17 00:00:00 2001 From: Danny Gershman Date: Thu, 11 Jun 2026 19:24:51 -0400 Subject: [PATCH 2/2] Serialize corveil installs + DRY the SettingsView commit path (CROW-490 review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses dhilgaertner's review on PR #497: - Yellow: two rapid picker commits could race on `query-corveil.md` (concurrent `corveil skill install` subprocesses) and on the `corveilSkillInstallWarning` banner (out-of-order completion). Adds `corveilInstallTail`/`enqueueCorveilInstall` mirroring the existing `reviewKickoffTail`/`enqueueReviewKickoff` serial-tail pattern (#266): each new commit chains a `Task { @MainActor }` that awaits its predecessor before running the install, so the last-committed path is also the last to write the banner. The blocking subprocess still runs via a nested `Task.detached`, bounded by `corveilInstallTimeout`. - Green: extract `commitCorveilPath()` in SettingsView so the Browse confirm and `onSubmit` handlers funnel through a single "persist β†’ reinstall" path. Reads the picker via `corveilBinding.wrappedValue` rather than the raw `url.path`, so the binding's whitespace-trim normalization wins and the install closure sees the same string the next launch's scaffolder would see. πŸ¦β€β¬› Generated with Claude Code, orchestrated by Crow Co-Authored-By: Claude Crow-Session: 14B927FC-A2CB-43A9-B5C4-318BE4578E2B --- .../CrowUI/Sources/CrowUI/SettingsView.swift | 28 +++++---- Sources/Crow/App/AppDelegate.swift | 59 +++++++++++++------ 2 files changed, 57 insertions(+), 30 deletions(-) diff --git a/Packages/CrowUI/Sources/CrowUI/SettingsView.swift b/Packages/CrowUI/Sources/CrowUI/SettingsView.swift index c15348a..370f320 100644 --- a/Packages/CrowUI/Sources/CrowUI/SettingsView.swift +++ b/Packages/CrowUI/Sources/CrowUI/SettingsView.swift @@ -275,14 +275,7 @@ public struct SettingsView: View { HStack { TextField("Path to corveil binary", text: corveilBinding) .textFieldStyle(.roundedBorder) - .onSubmit { - save() - // Hot-trigger install on Enter β€” passes nil if the - // user committed an empty field so callers can - // clear any stale warning banner (CROW-490). - let path = corveilBinding.wrappedValue - onCorveilReinstall?(path.isEmpty ? nil : path) - } + .onSubmit { commitCorveilPath() } Button("Browse...") { let panel = NSOpenPanel() panel.canChooseFiles = true @@ -290,11 +283,9 @@ public struct SettingsView: View { panel.allowsMultipleSelection = false if panel.runModal() == .OK, let url = panel.url { corveilBinding.wrappedValue = url.path - save() // Clear stale verify result β€” it's about a previous binary. corveilVerifyResult = nil - // Hot-trigger install on Browse confirm (CROW-490). - onCorveilReinstall?(url.path) + commitCorveilPath() } } Button(corveilVerifying ? "Verifying…" : "Verify") { verifyCorveil() } @@ -612,6 +603,21 @@ public struct SettingsView: View { onSave?(devRoot, config) } + /// Commit a corveil picker change: persist the config and hot-trigger + /// the `/query-corveil` install for the new path (CROW-490). Both + /// commit sites (Browse confirm and TextField `onSubmit`) funnel + /// through here so the "persist β†’ reinstall" pair stays atomic and + /// the `nil`-on-empty rule has a single source of truth. We read the + /// path through `corveilBinding.wrappedValue` rather than the raw + /// input so the binding's whitespace-trim normalization wins β€” the + /// install closure sees the same string that the next launch's + /// scaffolder would see. + private func commitCorveilPath() { + save() + let path = corveilBinding.wrappedValue + onCorveilReinstall?(path.isEmpty ? nil : path) + } + /// Two-way binding into `config.defaults.binaries["corveil"]` that treats /// an empty string as "unset" (so the map doesn't accumulate stale empty /// entries when the user clears the field). Trimming happens on commit so diff --git a/Sources/Crow/App/AppDelegate.swift b/Sources/Crow/App/AppDelegate.swift index 4a99832..e5f2907 100644 --- a/Sources/Crow/App/AppDelegate.swift +++ b/Sources/Crow/App/AppDelegate.swift @@ -53,6 +53,16 @@ final class AppDelegate: NSObject, NSApplicationDelegate { /// replaced. private var reviewKickoffTail: Task? + /// Tail of the serial corveil-skill-install queue. Settings picker + /// commits (`SettingsView.onCorveilReinstall`) chain new installs onto + /// this tail so two rapid picks don't race on `query-corveil.md` + /// (concurrent `corveil skill install` subprocesses writing the same + /// `--path`) or on `corveilSkillInstallWarning` (out-of-order + /// 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? + func applicationDidFinishLaunching(_ notification: Notification) { // Must be the very first call so the next exit (graceful or not) // lands somewhere readable. Also redirects stderr so Swift runtime @@ -187,6 +197,30 @@ 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. + @MainActor + private func enqueueCorveilInstall(path: String?, devRoot: String) { + let previous = corveilInstallTail + corveilInstallTail = Task { @MainActor [weak self] in + await previous?.value + let warning = await Task.detached { + let scaffolder = Scaffolder(devRoot: devRoot) + return scaffolder.installCorveilSkill(path) + }.value + self?.appState.corveilSkillInstallWarning = warning + } + } + // MARK: - tmux watchdog alert /// Suppress repeated alerts while one is already on screen. Each alert @@ -1102,28 +1136,15 @@ final class AppDelegate: NSObject, NSApplicationDelegate { }, onCorveilReinstall: { [weak self] newPath in // CROW-490: when the user commits a new corveil binary path in - // Settings, re-run just the `corveil skill install` step β€” + // 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. // - // `installCorveilSkill` calls `proc.waitUntilExit()` (bounded - // by `Scaffolder.corveilInstallTimeout`), so it must run off - // the main thread or it would freeze the Settings window for - // up to 5s. Result is always assigned: `nil` (success or - // empty/cleared path) clears any prior banner; non-nil shows - // the same diagnostic the launch-time path produces via the - // existing `AppState.corveilSkillInstallWarning` surface. - guard let self else { return } - // `devRoot` is the non-optional local from the outer guard at - // `showSettings()` line 1066 β€” captured here so the closure - // doesn't need to re-unwrap `self.devRoot`. - Task.detached { - let scaffolder = Scaffolder(devRoot: devRoot) - let warning = scaffolder.installCorveilSkill(newPath) - await MainActor.run { - self.appState.corveilSkillInstallWarning = warning - } - } + // `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) } )