From 7805bbace54643e92128e060cc3f5ca7404c6cb4 Mon Sep 17 00:00:00 2001 From: Danny Gershman Date: Thu, 11 Jun 2026 11:12:37 -0400 Subject: [PATCH 1/8] Add Corveil CLI path picker + auto-install /query-corveil on launch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds a `defaults.binaries` map to config.json (keyed by tool name) and a Settings β†’ General β†’ "Corveil CLI" section so the user can point Crow at their locally-built corveil binary. On every launch, the Scaffolder runs ` skill install --path /.claude/commands/query-corveil.md` so the embedded /query-corveil slash command stays in sync with the user's corveil without Crow shipping a (drifting) copy of the skill body. Failures here are non-fatal: an unset path is a no-op, a missing/ non-executable binary or non-zero install exit logs and surfaces a transient banner in Settings via the existing `appState.Warning` pattern. Includes a "Verify" affordance that runs ` --version` off the main actor and inlines the result. Closes #482 πŸ¦β€β¬› Generated with Claude Code, orchestrated by Crow Co-Authored-By: Claude Crow-Session: 8E1A62FA-5A6E-4F07-948F-8B6F379F8FE3 --- .../CrowCore/Sources/CrowCore/AppState.swift | 6 + .../Sources/CrowCore/Models/AppConfig.swift | 21 ++- .../CrowUI/Sources/CrowUI/SettingsView.swift | 128 ++++++++++++++++++ Sources/Crow/App/AppDelegate.swift | 24 +++- Sources/Crow/App/Scaffolder.swift | 78 ++++++++++- 5 files changed, 245 insertions(+), 12 deletions(-) diff --git a/Packages/CrowCore/Sources/CrowCore/AppState.swift b/Packages/CrowCore/Sources/CrowCore/AppState.swift index 9192b29..8950ab2 100644 --- a/Packages/CrowCore/Sources/CrowCore/AppState.swift +++ b/Packages/CrowCore/Sources/CrowCore/AppState.swift @@ -233,6 +233,12 @@ public final class AppState { /// Set by `IssueTracker` when polling is suspended; cleared on next success. public var rateLimitWarning: String? + /// Non-fatal warning surfaced in Settings when the per-launch + /// `corveil skill install` run fails (CROW-482). `nil` means the install + /// either succeeded or wasn't attempted (no path configured). Set by + /// `AppDelegate.launchMainApp` from the `Scaffolder` result. + public var corveilSkillInstallWarning: String? + /// Terminal readiness state per terminal ID. public var terminalReadiness: [UUID: TerminalReadiness] = [:] diff --git a/Packages/CrowCore/Sources/CrowCore/Models/AppConfig.swift b/Packages/CrowCore/Sources/CrowCore/Models/AppConfig.swift index 6bd8efa..ca1ab9e 100644 --- a/Packages/CrowCore/Sources/CrowCore/Models/AppConfig.swift +++ b/Packages/CrowCore/Sources/CrowCore/Models/AppConfig.swift @@ -406,11 +406,22 @@ public struct ConfigDefaults: Codable, Sendable, Equatable { public var excludeReviewRepos: [String] public var excludeTicketRepos: [String] public var ignoreReviewLabels: [String] - /// Explicit absolute-path overrides for `CodingAgent` binary discovery, - /// keyed by `AgentKind.rawValue` (e.g. `"codex"`, `"cursor"`, `"claude-code"`). - /// Consulted before the PATH walk in `CodingAgent.findBinary()` β€” set this - /// when discovery doesn't find your install for any reason (exotic Node - /// manager, sandboxed PATH, etc.). See CROW-484. + /// Absolute-path overrides for executable binaries, keyed by tool name. + /// + /// Serves two callers that share the same map shape: + /// - **Agent binary discovery** (CROW-484): keyed by `AgentKind.rawValue` + /// (`"codex"`, `"cursor"`, `"claude-code"`). `CodingAgent.findBinary()` + /// consults this map before walking PATH β€” set this when discovery + /// doesn't find your install (exotic Node manager, sandboxed PATH, etc.). + /// - **External tool installers** (CROW-482): keyed by tool name (e.g. + /// `"corveil"`) and used by `Scaffolder` to run each tool's own skill + /// installer on launch. The Settings UI currently exposes only the + /// `corveil` slot; the map shape is intentionally generic so future + /// tools (soulstone, tanzanite, …) extend the same field without a + /// schema change. + /// + /// Agent keys (`claude-code`, `codex`, `cursor`) and tool keys (`corveil`, + /// …) don't overlap, so the two callers coexist in one map. public var binaries: [String: String] /// Characters that are invalid in git ref names (see `git check-ref-format`). diff --git a/Packages/CrowUI/Sources/CrowUI/SettingsView.swift b/Packages/CrowUI/Sources/CrowUI/SettingsView.swift index c74a9f9..c6b111e 100644 --- a/Packages/CrowUI/Sources/CrowUI/SettingsView.swift +++ b/Packages/CrowUI/Sources/CrowUI/SettingsView.swift @@ -18,6 +18,12 @@ public struct SettingsView: View { @State private var editingJob: JobConfig? /// A pre-filled copy of a job, presented in the form (create mode) to duplicate it. @State private var duplicatingJob: JobConfig? + /// Live result of the most recent corveil "Verify" run. `nil` until the + /// user has clicked Verify at least once this Settings session. Starts + /// with `βœ“` on success, `βœ—` on failure (CROW-482). + @State private var corveilVerifyResult: String? + /// True while the Verify button's subprocess is in flight. + @State private var corveilVerifying: Bool = false public var onSave: ((String, AppConfig) -> Void)? public var onRescaffold: ((String) -> Void)? @@ -181,6 +187,23 @@ public struct SettingsView: View { } } + @ViewBuilder + private var corveilSkillWarningBanner: some View { + if let warning = appState.corveilSkillInstallWarning { + HStack(alignment: .top, spacing: 8) { + Image(systemName: "exclamationmark.triangle.fill") + .foregroundStyle(.orange) + Text(warning) + .font(.caption) + .textSelection(.enabled) + Spacer() + } + .padding(10) + .background(Color.orange.opacity(0.12)) + .cornerRadius(6) + } + } + private var generalTab: some View { Form { if appState.githubScopeWarning != nil { @@ -192,6 +215,9 @@ public struct SettingsView: View { if appState.rateLimitWarning != nil { Section { rateLimitWarningBanner } } + if appState.corveilSkillInstallWarning != nil { + Section { corveilSkillWarningBanner } + } Section("Development Root") { HStack { TextField("Path", text: $devRoot) @@ -236,6 +262,37 @@ public struct SettingsView: View { .foregroundStyle(.secondary) } + Section("Corveil CLI") { + HStack { + TextField("Path to corveil binary", text: corveilBinding) + .textFieldStyle(.roundedBorder) + .onSubmit { save() } + Button("Browse...") { + let panel = NSOpenPanel() + panel.canChooseFiles = true + panel.canChooseDirectories = false + 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 + } + } + Button(corveilVerifying ? "Verifying…" : "Verify") { verifyCorveil() } + .disabled(corveilBinding.wrappedValue.isEmpty || corveilVerifying) + } + if let result = 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.") + .font(.caption) + .foregroundStyle(.secondary) + } + Section("Sidebar") { Toggle("Hide session details", isOn: $config.sidebar.hideSessionDetails) .onChange(of: config.sidebar.hideSessionDetails) { _, _ in save() } @@ -537,6 +594,77 @@ public struct SettingsView: View { onSave?(devRoot, config) } + /// 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 + /// pasted paths with stray whitespace are normalized. + private var corveilBinding: Binding { + Binding( + get: { config.defaults.binaries["corveil"] ?? "" }, + set: { newValue in + let trimmed = newValue.trimmingCharacters(in: .whitespacesAndNewlines) + if trimmed.isEmpty { + config.defaults.binaries.removeValue(forKey: "corveil") + } else { + config.defaults.binaries["corveil"] = trimmed + } + } + ) + } + + /// Run ` --version` and surface the result. Lives off the + /// main actor so the spinning UI doesn't block. Truncates noisy output + /// to keep the inline result line readable. + private func verifyCorveil() { + let path = corveilBinding.wrappedValue + guard !path.isEmpty else { return } + corveilVerifying = true + corveilVerifyResult = nil + Task.detached { + let result = SettingsView.runCorveilVersion(at: path) + await MainActor.run { + corveilVerifyResult = result + corveilVerifying = 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. + nonisolated static func runCorveilVersion(at path: String) -> String { + let fm = FileManager.default + guard fm.isExecutableFile(atPath: path) else { + return "βœ— Not executable: \(path)" + } + let proc = Process() + proc.executableURL = URL(fileURLWithPath: path) + proc.arguments = ["--version"] + let outPipe = Pipe() + let errPipe = Pipe() + proc.standardOutput = outPipe + proc.standardError = errPipe + do { + try proc.run() + } catch { + return "βœ— Could not launch: \(error.localizedDescription)" + } + proc.waitUntilExit() + let out = (try? outPipe.fileHandleForReading.readToEnd()) + .flatMap { String(data: $0, encoding: .utf8) }? + .trimmingCharacters(in: .whitespacesAndNewlines) ?? "" + let err = (try? errPipe.fileHandleForReading.readToEnd()) + .flatMap { String(data: $0, encoding: .utf8) }? + .trimmingCharacters(in: .whitespacesAndNewlines) ?? "" + let combined = [out, err].filter { !$0.isEmpty }.joined(separator: " β€” ") + let snippet = combined.split(separator: "\n").first.map(String.init) ?? combined + if proc.terminationStatus == 0 { + return snippet.isEmpty ? "βœ“ Verified" : "βœ“ \(snippet)" + } + let detail = snippet.isEmpty ? "exit code \(proc.terminationStatus)" : snippet + return "βœ— \(detail)" + } + /// One per-action agent picker (Coding/Reviews/Jobs). "Use default" /// removes the override; selecting a concrete agent writes the /// `config.agentsByKind` entry. Disabled until a second agent is diff --git a/Sources/Crow/App/AppDelegate.swift b/Sources/Crow/App/AppDelegate.swift index 3db5200..390df66 100644 --- a/Sources/Crow/App/AppDelegate.swift +++ b/Sources/Crow/App/AppDelegate.swift @@ -300,10 +300,12 @@ final class AppDelegate: NSObject, NSApplicationDelegate { // Scaffold directory structure let scaffolder = Scaffolder(devRoot: devRoot) - try scaffolder.scaffold( + let result = try scaffolder.scaffold( workspaceNames: config.workspaces.map(\.name), - managerAgentKind: config.agentKind(for: .manager) + managerAgentKind: config.agentKind(for: .manager), + corveilBinaryPath: config.defaults.binaries["corveil"] ) + appState.corveilSkillInstallWarning = result.warning // Save config try ConfigStore.saveConfig(config, devRoot: devRoot) @@ -403,10 +405,12 @@ final class AppDelegate: NSObject, NSApplicationDelegate { // Update skills and CLAUDE.md on every launch let scaffolder = Scaffolder(devRoot: devRoot) do { - try scaffolder.scaffold( + let result = try scaffolder.scaffold( workspaceNames: config.workspaces.map(\.name), - managerAgentKind: config.agentKind(for: .manager) + managerAgentKind: config.agentKind(for: .manager), + corveilBinaryPath: config.defaults.binaries["corveil"] ) + appState.corveilSkillInstallWarning = result.warning } catch { NSLog("[Crow] Scaffold update failed: %@", error.localizedDescription) } @@ -1068,10 +1072,18 @@ final class AppDelegate: NSObject, NSApplicationDelegate { onRescaffold: { [weak self] devRoot in let scaffolder = Scaffolder(devRoot: devRoot) let cfg = self?.appConfig - try? scaffolder.scaffold( + let result = try? scaffolder.scaffold( workspaceNames: cfg?.workspaces.map(\.name) ?? [], - managerAgentKind: cfg?.agentKind(for: .manager) ?? .claudeCode + managerAgentKind: cfg?.agentKind(for: .manager) ?? .claudeCode, + corveilBinaryPath: cfg?.defaults.binaries["corveil"] ) + if let warning = result?.warning { + self?.appState.corveilSkillInstallWarning = warning + } else if result != nil { + // Scaffold completed and corveil install (if attempted) succeeded β€” + // clear any stale warning from a prior failed launch. + self?.appState.corveilSkillInstallWarning = nil + } } ) diff --git a/Sources/Crow/App/Scaffolder.swift b/Sources/Crow/App/Scaffolder.swift index b432c5a..8a7a550 100644 --- a/Sources/Crow/App/Scaffolder.swift +++ b/Sources/Crow/App/Scaffolder.swift @@ -1,6 +1,14 @@ import CrowCore import Foundation +/// Outcome of a single `Scaffolder.scaffold(...)` run. `warning` is non-nil only +/// for non-fatal post-scaffold issues (today: a configured `corveil` binary +/// that's missing/non-executable or whose `skill install` returned non-zero). +/// Callers surface it via `AppState.corveilSkillInstallWarning`; never fatal. +struct ScaffoldResult { + var warning: String? +} + /// Creates the devRoot directory structure and copies bundled resources. struct Scaffolder { let devRoot: String @@ -10,7 +18,17 @@ struct Scaffolder { /// `managerAgentKind` drives `{{CROW_AGENT_DISPLAY_NAME}}` substitution in the /// dev-root skill bodies (issue #447). The Manager session is the consumer of /// these files, so its agent kind is the right one to bake in. - func scaffold(workspaceNames: [String], managerAgentKind: AgentKind = .claudeCode) throws { + /// + /// `corveilBinaryPath`, when set and executable, triggers a post-scaffold + /// `corveil skill install --path {devRoot}/.claude/commands/query-corveil.md` + /// so the embedded `/query-corveil` slash command stays in sync with the + /// user's locally-built corveil binary (CROW-482). Failures here are + /// non-fatal: they are returned as `ScaffoldResult.warning` and never + /// throw β€” the rest of the scaffold has already succeeded by that point. + @discardableResult + func scaffold(workspaceNames: [String], + managerAgentKind: AgentKind = .claudeCode, + corveilBinaryPath: String? = nil) throws -> ScaffoldResult { let fm = FileManager.default // Create devRoot @@ -113,6 +131,64 @@ struct Scaffolder { // Create prompts directory for crow-workspace prompt files let promptsDir = (claudeDir as NSString).appendingPathComponent("prompts") try fm.createDirectory(atPath: promptsDir, withIntermediateDirectories: true) + + // Re-install the embedded /query-corveil slash command from the + // user-configured corveil binary on every launch (CROW-482). Failure + // here is intentionally non-fatal β€” the rest of the scaffold is done. + let warning = installCorveilSkill(corveilBinaryPath) + return ScaffoldResult(warning: warning) + } + + /// Runs ` skill install --path {devRoot}/.claude/commands/query-corveil.md` + /// when the path is set and points at an executable. Returns a short + /// user-facing warning string on failure; `nil` on success or when the + /// feature is unconfigured (empty/nil path). + private func installCorveilSkill(_ corveilBinaryPath: String?) -> String? { + guard let path = corveilBinaryPath?.trimmingCharacters(in: .whitespaces), + !path.isEmpty else { + return nil + } + let fm = FileManager.default + guard fm.isExecutableFile(atPath: path) else { + NSLog("[Scaffolder] corveil binary not executable: %@", path) + return "Corveil skill install skipped β€” binary at \(path) is missing or not executable. Check Settings β†’ General β†’ Corveil CLI." + } + + let commandsDir = (devRoot as NSString).appendingPathComponent(".claude/commands") + do { + try fm.createDirectory(atPath: commandsDir, withIntermediateDirectories: true) + } catch { + NSLog("[Scaffolder] could not create commands dir: %@", error.localizedDescription) + return "Corveil skill install failed β€” could not create .claude/commands directory." + } + let target = (commandsDir as NSString).appendingPathComponent("query-corveil.md") + + let proc = Process() + proc.executableURL = URL(fileURLWithPath: path) + proc.arguments = ["skill", "install", "--path", target] + let stderrPipe = Pipe() + proc.standardError = stderrPipe + // Discard stdout β€” corveil prints a single diagnostic line we don't surface. + proc.standardOutput = Pipe() + + do { + try proc.run() + } catch { + NSLog("[Scaffolder] corveil launch failed: %@", error.localizedDescription) + return "Corveil skill install failed β€” \(error.localizedDescription). Check path in Settings." + } + proc.waitUntilExit() + if proc.terminationStatus != 0 { + let stderr = (try? stderrPipe.fileHandleForReading.readToEnd()) + .flatMap { String(data: $0, encoding: .utf8) }? + .trimmingCharacters(in: .whitespacesAndNewlines) ?? "" + NSLog("[Scaffolder] corveil skill install exit=%d stderr=%@", + proc.terminationStatus, stderr) + let detail = stderr.isEmpty ? "exit code \(proc.terminationStatus)" : stderr + return "Corveil skill install failed β€” \(detail). Check path in Settings." + } + NSLog("[Scaffolder] corveil skill installed at %@", target) + return nil } // MARK: - Bundled Templates From e2495b5d76776b67d042955826bae8d6b6b71f01 Mon Sep 17 00:00:00 2001 From: Danny Gershman Date: Thu, 11 Jun 2026 11:23:59 -0400 Subject: [PATCH 2/8] Address review feedback: timeout, dedup install, tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Bound the per-launch corveil install with a 5s wall-clock timeout (`Scaffolder.corveilInstallTimeout`). `scaffold(...)` runs on the main thread during applicationDidFinishLaunching, so a hung corveil binary would freeze startup before the window draws. Poll for completion in 50ms slices, SIGTERM on timeout, and return a warning instead of blocking forever. - Stop running the install twice on first-time setup. `completeSetup` scaffolds with `corveilBinaryPath: nil`, then `launchMainApp` (which it calls immediately after) does the real install β€” previously both ran the subprocess and only the second result was kept. - Add round-trip + missing-key tests for `defaults.binaries` matching the convention used by the adjacent `ignoreReviewLabels` tests. - Add five tests for `SettingsView.runCorveilVersion(at:)` covering the non-executable gate, empty input, the `βœ“ Verified` formatting when a successful binary has no output, the exit-code formatting on failure, and a successful binary that emits stdout. πŸ¦β€β¬› Generated with Claude Code, orchestrated by Crow Co-Authored-By: Claude Crow-Session: 8E1A62FA-5A6E-4F07-948F-8B6F379F8FE3 --- .../Tests/CrowCoreTests/AppConfigTests.swift | 25 +++++++++++++ .../Tests/CrowUITests/CrowUITests.swift | 35 ++++++++++++++++++ Sources/Crow/App/AppDelegate.swift | 10 +++--- Sources/Crow/App/Scaffolder.swift | 36 ++++++++++++++++++- 4 files changed, 101 insertions(+), 5 deletions(-) diff --git a/Packages/CrowCore/Tests/CrowCoreTests/AppConfigTests.swift b/Packages/CrowCore/Tests/CrowCoreTests/AppConfigTests.swift index 689dd4c..7d384aa 100644 --- a/Packages/CrowCore/Tests/CrowCoreTests/AppConfigTests.swift +++ b/Packages/CrowCore/Tests/CrowCoreTests/AppConfigTests.swift @@ -485,6 +485,31 @@ import Testing #expect(config.defaults.ignoreReviewLabels.isEmpty) } +// MARK: - defaults.binaries (CROW-482) + +@Test func binariesRoundTrip() throws { + let config = AppConfig( + defaults: ConfigDefaults(binaries: [ + "corveil": "/Users/jane/dev/corveil/corveil", + "soulstone": "/usr/local/bin/soulstone", + ]) + ) + let data = try JSONEncoder().encode(config) + let decoded = try JSONDecoder().decode(AppConfig.self, from: data) + #expect(decoded.defaults.binaries["corveil"] == "/Users/jane/dev/corveil/corveil") + #expect(decoded.defaults.binaries["soulstone"] == "/usr/local/bin/soulstone") +} + +@Test func binariesDefaultsEmptyWhenKeyMissing() throws { + // Configs written before CROW-482 don't have `binaries` β€” they must still + // decode cleanly with an empty map (forward compatibility). + let json = """ + {"defaults": {"provider": "github", "cli": "gh", "branchPrefix": "feature/", "excludeDirs": []}} + """.data(using: .utf8)! + let config = try JSONDecoder().decode(AppConfig.self, from: json) + #expect(config.defaults.binaries.isEmpty) +} + // MARK: - AI gateway (CROW-402) @Test func workspaceGatewayRoundTrip() throws { diff --git a/Packages/CrowUI/Tests/CrowUITests/CrowUITests.swift b/Packages/CrowUI/Tests/CrowUITests/CrowUITests.swift index 70b46d6..d9aed10 100644 --- a/Packages/CrowUI/Tests/CrowUITests/CrowUITests.swift +++ b/Packages/CrowUI/Tests/CrowUITests/CrowUITests.swift @@ -4,6 +4,41 @@ import Testing @testable import CrowCore @testable import CrowUI +// MARK: - SettingsView.runCorveilVersion (CROW-482) + +@Suite("SettingsView.runCorveilVersion") +struct RunCorveilVersionTests { + + @Test func nonExecutablePathReturnsError() { + let result = SettingsView.runCorveilVersion(at: "/this/path/definitely/does/not/exist") + #expect(result.hasPrefix("βœ— Not executable:")) + } + + @Test func emptyPathReturnsError() { + // isExecutableFile returns false for "", so the gate still trips. + let result = SettingsView.runCorveilVersion(at: "") + #expect(result.hasPrefix("βœ— Not executable:")) + } + + @Test func zeroExitWithNoOutputFormatsAsVerified() { + // /usr/bin/true ignores arguments and exits 0 with no stdout/stderr. + let result = SettingsView.runCorveilVersion(at: "/usr/bin/true") + #expect(result == "βœ“ Verified") + } + + @Test func nonZeroExitWithNoOutputSurfacesExitCode() { + // /usr/bin/false ignores arguments and exits 1 with no stdout/stderr. + let result = SettingsView.runCorveilVersion(at: "/usr/bin/false") + #expect(result == "βœ— exit code 1") + } + + @Test func zeroExitWithStdoutShowsSnippet() { + // /bin/echo --version β†’ exits 0, prints "--version\n" to stdout. + let result = SettingsView.runCorveilVersion(at: "/bin/echo") + #expect(result == "βœ“ --version") + } +} + // MARK: - SessionStatus Display Names @Test func sessionStatusDisplayNames() { diff --git a/Sources/Crow/App/AppDelegate.swift b/Sources/Crow/App/AppDelegate.swift index 390df66..4c80e3e 100644 --- a/Sources/Crow/App/AppDelegate.swift +++ b/Sources/Crow/App/AppDelegate.swift @@ -298,14 +298,16 @@ final class AppDelegate: NSObject, NSApplicationDelegate { // Save devRoot pointer try ConfigStore.saveDevRoot(devRoot) - // Scaffold directory structure + // Scaffold directory structure. Don't run the corveil skill install + // here β€” `launchMainApp()` runs `scaffold(...)` again immediately + // below, so doing it twice on first-time setup just fires the + // subprocess twice with the second result winning. let scaffolder = Scaffolder(devRoot: devRoot) - let result = try scaffolder.scaffold( + _ = try scaffolder.scaffold( workspaceNames: config.workspaces.map(\.name), managerAgentKind: config.agentKind(for: .manager), - corveilBinaryPath: config.defaults.binaries["corveil"] + corveilBinaryPath: nil ) - appState.corveilSkillInstallWarning = result.warning // Save config try ConfigStore.saveConfig(config, devRoot: devRoot) diff --git a/Sources/Crow/App/Scaffolder.swift b/Sources/Crow/App/Scaffolder.swift index 8a7a550..6041b2d 100644 --- a/Sources/Crow/App/Scaffolder.swift +++ b/Sources/Crow/App/Scaffolder.swift @@ -143,6 +143,13 @@ struct Scaffolder { /// when the path is set and points at an executable. Returns a short /// user-facing warning string on failure; `nil` on success or when the /// feature is unconfigured (empty/nil path). + /// + /// `Scaffolder.scaffold(...)` runs on the main thread during + /// `applicationDidFinishLaunching`, so a hung corveil binary (wrong + /// executable, stdin prompt, etc.) would freeze app startup with no + /// 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? { guard let path = corveilBinaryPath?.trimmingCharacters(in: .whitespaces), !path.isEmpty else { @@ -177,7 +184,27 @@ struct Scaffolder { NSLog("[Scaffolder] corveil launch failed: %@", error.localizedDescription) return "Corveil skill install failed β€” \(error.localizedDescription). Check path in Settings." } - proc.waitUntilExit() + + // Wall-clock timeout: poll for completion in short slices so a hung + // process gets SIGTERM'd instead of blocking app launch indefinitely. + // `waitUntilExit()` has no timeout overload; this is the standard + // Foundation workaround. + let deadline = Date().addingTimeInterval(Self.corveilInstallTimeout) + while proc.isRunning { + if Date() >= deadline { + proc.terminate() + // Give the process up to 500ms to honor SIGTERM before + // returning the warning, so we don't leave a zombie behind. + let graceDeadline = Date().addingTimeInterval(0.5) + while proc.isRunning, Date() < graceDeadline { + Thread.sleep(forTimeInterval: 0.05) + } + NSLog("[Scaffolder] corveil skill install timed out after %.1fs", Self.corveilInstallTimeout) + return "Corveil skill install timed out after \(Int(Self.corveilInstallTimeout))s β€” binary may be hung. Check path in Settings." + } + Thread.sleep(forTimeInterval: 0.05) + } + if proc.terminationStatus != 0 { let stderr = (try? stderrPipe.fileHandleForReading.readToEnd()) .flatMap { String(data: $0, encoding: .utf8) }? @@ -191,6 +218,13 @@ struct Scaffolder { return nil } + /// Wall-clock budget for the per-launch `corveil skill install` run. Tight + /// because `Scaffolder.scaffold(...)` runs on the main thread before the + /// app window is shown β€” a hung corveil binary delays first paint by this + /// many seconds. 5s is generous for a local subprocess that only writes + /// one ~10KB file. + static let corveilInstallTimeout: TimeInterval = 5.0 + // MARK: - Bundled Templates /// The CLAUDE.md template bundled with the app. From cd5a2b561c0bae826466d8c19cb3f684afc06765 Mon Sep 17 00:00:00 2001 From: Danny Gershman Date: Thu, 11 Jun 2026 12:57:20 -0400 Subject: [PATCH 3/8] Address review round 2: verify-path timeout, pipe drain, polish MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Bound `SettingsView.runCorveilVersion` with a 5s wall-clock timeout (`verifyTimeout`) using the same poll-with-deadline + SIGTERM pattern as the Scaffolder install path. A user-configured corveil that hangs on `--version` can no longer pin the detached Task or leak the child process indefinitely. - Drain stdout/stderr concurrently via readability handlers so a binary emitting >64KB before exit can't pipe-buffer-deadlock against an unread `Pipe()`. Accumulators go through a small `DataAccumulator` reference type with NSLock-serialized access β€” safe under Swift 6 sendable rules because the closure captures the class, not a `var`. - `installCorveilSkill` now uses `FileHandle.nullDevice` for stdout instead of an undrained `Pipe()`. Explicit discard, deadlock-proof even if the 5s install timeout weren't there. - `onRescaffold` does a `do/catch` instead of `try?` so a thrown scaffold failure replaces (not silently preserves) any stale corveil-install banner from a prior launch. πŸ¦β€β¬› Generated with Claude Code, orchestrated by Crow Co-Authored-By: Claude Crow-Session: 8E1A62FA-5A6E-4F07-948F-8B6F379F8FE3 --- .../CrowUI/Sources/CrowUI/SettingsView.swift | 87 +++++++++++++++++-- Sources/Crow/App/AppDelegate.swift | 28 +++--- Sources/Crow/App/Scaffolder.swift | 6 +- 3 files changed, 102 insertions(+), 19 deletions(-) diff --git a/Packages/CrowUI/Sources/CrowUI/SettingsView.swift b/Packages/CrowUI/Sources/CrowUI/SettingsView.swift index c6b111e..3a73764 100644 --- a/Packages/CrowUI/Sources/CrowUI/SettingsView.swift +++ b/Packages/CrowUI/Sources/CrowUI/SettingsView.swift @@ -632,6 +632,13 @@ public struct SettingsView: View { /// Pure helper for `verifyCorveil` β€” easier to reason about off the main /// actor and trivially testable. Returns a single-line summary suitable /// for inline display. + /// + /// Bounded by `verifyTimeout` so a misbehaving binary that hangs on + /// `--version` can't pin the detached Task forever (matches the + /// `Scaffolder.installCorveilSkill` install-path timeout). stdout/stderr + /// are drained concurrently via readability handlers so a binary that + /// emits more than the pipe buffer (~64KB) before exiting can't deadlock + /// against the unread pipe. nonisolated static func runCorveilVersion(at path: String) -> String { let fm = FileManager.default guard fm.isExecutableFile(atPath: path) else { @@ -644,20 +651,61 @@ public struct SettingsView: View { let errPipe = Pipe() proc.standardOutput = outPipe proc.standardError = errPipe + + // Concurrent drain. The readability handlers run on a Foundation- + // managed background queue, so we cannot capture local `var`s β€” use + // a thread-safe reference-typed accumulator that's safe to share. + let outAcc = DataAccumulator() + let errAcc = DataAccumulator() + outPipe.fileHandleForReading.readabilityHandler = { handle in + let chunk = handle.availableData + if !chunk.isEmpty { outAcc.append(chunk) } + } + errPipe.fileHandleForReading.readabilityHandler = { handle in + let chunk = handle.availableData + if !chunk.isEmpty { errAcc.append(chunk) } + } + // Ensure handlers detach on every exit path so the file handles + // close cleanly even if we return early on launch failure. + defer { + outPipe.fileHandleForReading.readabilityHandler = nil + errPipe.fileHandleForReading.readabilityHandler = nil + } + do { try proc.run() } catch { return "βœ— Could not launch: \(error.localizedDescription)" } - proc.waitUntilExit() - let out = (try? outPipe.fileHandleForReading.readToEnd()) - .flatMap { String(data: $0, encoding: .utf8) }? + + // Wall-clock timeout. Poll in 50ms slices so a hung binary is + // SIGTERM'd rather than blocking forever; a 500ms grace lets the + // process honor SIGTERM before we drop it. + let deadline = Date().addingTimeInterval(verifyTimeout) + var timedOut = false + while proc.isRunning { + if Date() >= deadline { + proc.terminate() + let graceDeadline = Date().addingTimeInterval(0.5) + while proc.isRunning, Date() < graceDeadline { + Thread.sleep(forTimeInterval: 0.05) + } + timedOut = true + break + } + Thread.sleep(forTimeInterval: 0.05) + } + + let outStr = String(data: outAcc.snapshot(), encoding: .utf8)? .trimmingCharacters(in: .whitespacesAndNewlines) ?? "" - let err = (try? errPipe.fileHandleForReading.readToEnd()) - .flatMap { String(data: $0, encoding: .utf8) }? + let errStr = String(data: errAcc.snapshot(), encoding: .utf8)? .trimmingCharacters(in: .whitespacesAndNewlines) ?? "" - let combined = [out, err].filter { !$0.isEmpty }.joined(separator: " β€” ") + let combined = [outStr, errStr].filter { !$0.isEmpty }.joined(separator: " β€” ") let snippet = combined.split(separator: "\n").first.map(String.init) ?? combined + + if timedOut { + return "βœ— Timed out after \(Int(verifyTimeout))s β€” binary may be hung." + } if proc.terminationStatus == 0 { return snippet.isEmpty ? "βœ“ Verified" : "βœ“ \(snippet)" } @@ -665,6 +713,13 @@ public struct SettingsView: View { return "βœ— \(detail)" } + /// Wall-clock budget for the "Verify" subprocess. Matches the install + /// path's `Scaffolder.corveilInstallTimeout` β€” a corveil that hangs on + /// `--version` is bounded to the same 5s window as one that hangs on + /// `skill install`. The Task wrapper runs off the main actor so the UI + /// stays responsive while this wait elapses. + nonisolated static let verifyTimeout: TimeInterval = 5.0 + /// One per-action agent picker (Coding/Reviews/Jobs). "Use default" /// removes the override; selecting a concrete agent writes the /// `config.agentsByKind` entry. Disabled until a second agent is @@ -702,3 +757,23 @@ public struct SettingsView: View { } } } + +/// Lock-serialized `Data` accumulator. Used by `SettingsView.runCorveilVersion` +/// so the off-queue readability handlers can share mutable state with the +/// polling loop without tripping Swift 6 sendable-capture rules. +/// `@unchecked Sendable` is sound here because every access goes through +/// the lock. +private final class DataAccumulator: @unchecked Sendable { + private let lock = NSLock() + private var data = Data() + + func append(_ chunk: Data) { + lock.lock(); defer { lock.unlock() } + data.append(chunk) + } + + func snapshot() -> Data { + lock.lock(); defer { lock.unlock() } + return data + } +} diff --git a/Sources/Crow/App/AppDelegate.swift b/Sources/Crow/App/AppDelegate.swift index 4c80e3e..8cfba50 100644 --- a/Sources/Crow/App/AppDelegate.swift +++ b/Sources/Crow/App/AppDelegate.swift @@ -1074,17 +1074,23 @@ final class AppDelegate: NSObject, NSApplicationDelegate { onRescaffold: { [weak self] devRoot in let scaffolder = Scaffolder(devRoot: devRoot) let cfg = self?.appConfig - let result = try? scaffolder.scaffold( - workspaceNames: cfg?.workspaces.map(\.name) ?? [], - managerAgentKind: cfg?.agentKind(for: .manager) ?? .claudeCode, - corveilBinaryPath: cfg?.defaults.binaries["corveil"] - ) - if let warning = result?.warning { - self?.appState.corveilSkillInstallWarning = warning - } else if result != nil { - // Scaffold completed and corveil install (if attempted) succeeded β€” - // clear any stale warning from a prior failed launch. - self?.appState.corveilSkillInstallWarning = nil + do { + let result = try scaffolder.scaffold( + workspaceNames: cfg?.workspaces.map(\.name) ?? [], + managerAgentKind: cfg?.agentKind(for: .manager) ?? .claudeCode, + corveilBinaryPath: cfg?.defaults.binaries["corveil"] + ) + // Always assign β€” clears a stale warning from a prior + // launch when the install now succeeds (`result.warning` + // is `nil` on success or no-op). + self?.appState.corveilSkillInstallWarning = result.warning + } catch { + NSLog("[Crow] Re-scaffold failed: %@", error.localizedDescription) + // Replace any existing corveil-install banner with a + // fresh "rescaffold failed" message so the user isn't + // looking at a stale message from a prior launch. + self?.appState.corveilSkillInstallWarning = + "Re-scaffold failed: \(error.localizedDescription)" } } ) diff --git a/Sources/Crow/App/Scaffolder.swift b/Sources/Crow/App/Scaffolder.swift index 6041b2d..c4f4f4d 100644 --- a/Sources/Crow/App/Scaffolder.swift +++ b/Sources/Crow/App/Scaffolder.swift @@ -175,8 +175,10 @@ struct Scaffolder { proc.arguments = ["skill", "install", "--path", target] let stderrPipe = Pipe() proc.standardError = stderrPipe - // Discard stdout β€” corveil prints a single diagnostic line we don't surface. - proc.standardOutput = Pipe() + // Discard stdout explicitly. We don't surface corveil's diagnostic + // line, and routing to /dev/null is deadlock-proof β€” an undrained + // `Pipe()` would block the child if it ever wrote >64KB before exit. + proc.standardOutput = FileHandle.nullDevice do { try proc.run() From 368b48ab805a5909e1b7779cf8fddee02b2e22d4 Mon Sep 17 00:00:00 2001 From: Danny Gershman Date: Thu, 11 Jun 2026 13:15:19 -0400 Subject: [PATCH 4/8] Address review round 3: deterministic pipe drains MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both pipe-handling paths now use a shared `PipeDrainer` pattern: a dedicated background thread runs `readDataToEndOfFile()` on the read end and signals a `DispatchSemaphore` on EOF. The caller waits on the semaphore (capped at `pipeDrainGrace` = 1s) before snapshotting, which establishes a happens-before barrier between the drain's final append and the read β€” no tail chunks dropped, no readability-handler race. - `Scaffolder.installCorveilSkill` drains stderr concurrently rather than leaving it as an undrained `Pipe()`. Closes the deadlock gap the existing comment on stdout's nullDevice claimed to avoid: a chatty failure (Go panic, assertion dump) can easily exceed the ~64KB pipe buffer. - `SettingsView.runCorveilVersion` replaces the readability-handler pattern with `PipeDrainer.start(...)` per pipe. The bounded semaphore wait is deterministic β€” no race between the polling thread observing `proc.isRunning == false` and an in-flight handler that may still hold the final tail chunk. `PipeDrainer` + `DataAccumulator` are duplicated across the Crow target (Scaffolder) and CrowUI (SettingsView) since they don't share a module today; both copies are ~25 lines each, which beats adding a CrowCore utility for two callers. πŸ¦β€β¬› Generated with Claude Code, orchestrated by Crow Co-Authored-By: Claude Crow-Session: 8E1A62FA-5A6E-4F07-948F-8B6F379F8FE3 --- .../CrowUI/Sources/CrowUI/SettingsView.swift | 104 +++++++++++++----- Sources/Crow/App/Scaffolder.swift | 77 ++++++++++++- 2 files changed, 149 insertions(+), 32 deletions(-) diff --git a/Packages/CrowUI/Sources/CrowUI/SettingsView.swift b/Packages/CrowUI/Sources/CrowUI/SettingsView.swift index 3a73764..21990b1 100644 --- a/Packages/CrowUI/Sources/CrowUI/SettingsView.swift +++ b/Packages/CrowUI/Sources/CrowUI/SettingsView.swift @@ -635,10 +635,15 @@ public struct SettingsView: View { /// /// Bounded by `verifyTimeout` so a misbehaving binary that hangs on /// `--version` can't pin the detached Task forever (matches the - /// `Scaffolder.installCorveilSkill` install-path timeout). stdout/stderr - /// are drained concurrently via readability handlers so a binary that - /// emits more than the pipe buffer (~64KB) before exiting can't deadlock - /// against the unread pipe. + /// `Scaffolder.installCorveilSkill` install-path timeout). Each pipe is + /// drained by a dedicated background reader running + /// `readDataToEndOfFile()`, which blocks until the child closes its end. + /// Snapshotting waits on a bounded `DispatchSemaphore`, so there's no + /// race between the polling thread and an in-flight readability handler: + /// either the drain finished cleanly (signal arrived β†’ snapshot is whole) + /// or the bounded wait expires and we report what's there. Combined, + /// these eliminate both the pipe-buffer deadlock and the EOF tail race + /// that `readabilityHandler` introduces. nonisolated static func runCorveilVersion(at path: String) -> String { let fm = FileManager.default guard fm.isExecutableFile(atPath: path) else { @@ -652,29 +657,14 @@ public struct SettingsView: View { proc.standardOutput = outPipe proc.standardError = errPipe - // Concurrent drain. The readability handlers run on a Foundation- - // managed background queue, so we cannot capture local `var`s β€” use - // a thread-safe reference-typed accumulator that's safe to share. - let outAcc = DataAccumulator() - let errAcc = DataAccumulator() - outPipe.fileHandleForReading.readabilityHandler = { handle in - let chunk = handle.availableData - if !chunk.isEmpty { outAcc.append(chunk) } - } - errPipe.fileHandleForReading.readabilityHandler = { handle in - let chunk = handle.availableData - if !chunk.isEmpty { errAcc.append(chunk) } - } - // Ensure handlers detach on every exit path so the file handles - // close cleanly even if we return early on launch failure. - defer { - outPipe.fileHandleForReading.readabilityHandler = nil - errPipe.fileHandleForReading.readabilityHandler = nil - } + let outDrain = PipeDrainer.start(outPipe) + let errDrain = PipeDrainer.start(errPipe) do { try proc.run() } catch { + outDrain.abandon() + errDrain.abandon() return "βœ— Could not launch: \(error.localizedDescription)" } @@ -696,9 +686,9 @@ public struct SettingsView: View { Thread.sleep(forTimeInterval: 0.05) } - let outStr = String(data: outAcc.snapshot(), encoding: .utf8)? + let outStr = String(data: outDrain.collect(within: pipeDrainGrace), encoding: .utf8)? .trimmingCharacters(in: .whitespacesAndNewlines) ?? "" - let errStr = String(data: errAcc.snapshot(), encoding: .utf8)? + let errStr = String(data: errDrain.collect(within: pipeDrainGrace), encoding: .utf8)? .trimmingCharacters(in: .whitespacesAndNewlines) ?? "" let combined = [outStr, errStr].filter { !$0.isEmpty }.joined(separator: " β€” ") let snippet = combined.split(separator: "\n").first.map(String.init) ?? combined @@ -720,6 +710,12 @@ public struct SettingsView: View { /// stays responsive while this wait elapses. nonisolated static let verifyTimeout: TimeInterval = 5.0 + /// Bounded post-exit wait for the pipe drain to flush. Once the child + /// is gone (or SIGTERM'd), EOF should arrive near-instantly; cap at 1s + /// so an uncooperative child that swallowed SIGTERM can't pin the + /// caller past `verifyTimeout`. + nonisolated static let pipeDrainGrace: TimeInterval = 1.0 + /// One per-action agent picker (Coding/Reviews/Jobs). "Use default" /// removes the override; selecting a concrete agent writes the /// `config.agentsByKind` entry. Disabled until a second agent is @@ -758,11 +754,59 @@ public struct SettingsView: View { } } -/// Lock-serialized `Data` accumulator. Used by `SettingsView.runCorveilVersion` -/// so the off-queue readability handlers can share mutable state with the -/// polling loop without tripping Swift 6 sendable-capture rules. -/// `@unchecked Sendable` is sound here because every access goes through -/// the lock. +/// Background drainer for a `Pipe`. Runs `readDataToEndOfFile()` off the +/// caller's thread so the child can't pipe-buffer-deadlock against an +/// unread `Pipe()`, and signals a semaphore on EOF so the caller can +/// collect deterministically without racing in-flight callbacks. +/// +/// `@unchecked Sendable` is sound because the only writer is the dispatched +/// closure (single-shot) and the only reader (`collect`) waits on the +/// semaphore β€” the signal establishes a happens-before relationship +/// between the drain's final `acc.append` and the snapshot read. +fileprivate final class PipeDrainer: @unchecked Sendable { + private let pipe: Pipe + private let acc = DataAccumulator() + private let done = DispatchSemaphore(value: 0) + + private init(pipe: Pipe) { + self.pipe = pipe + } + + /// Spawn the drain. `readDataToEndOfFile` blocks the worker thread + /// until the child closes its write end (clean exit or post-SIGTERM). + static func start(_ pipe: Pipe) -> PipeDrainer { + let drainer = PipeDrainer(pipe: pipe) + DispatchQueue.global(qos: .utility).async { + let data = drainer.pipe.fileHandleForReading.readDataToEndOfFile() + if !data.isEmpty { + drainer.acc.append(data) + } + drainer.done.signal() + } + return drainer + } + + /// Wait up to `timeout` seconds for the drain to complete, then return + /// whatever has been accumulated. Safe to call after `abandon()`. + func collect(within timeout: TimeInterval) -> Data { + _ = done.wait(timeout: .now() + timeout) + return acc.snapshot() + } + + /// Close the read end so the drain thread's `readDataToEndOfFile` can + /// return immediately. Used when the process never launched and the + /// pipe will never see a writer. + func abandon() { + try? pipe.fileHandleForReading.close() + } +} + +/// Lock-serialized `Data` accumulator. Used by `PipeDrainer` so the +/// background drain thread and the foreground collector share state +/// without tripping Swift 6 sendable rules. The lock is technically +/// redundant once `PipeDrainer.collect` waits on the semaphore (the +/// signal is a happens-before barrier), but it's cheap and keeps the +/// accumulator safe in isolation. private final class DataAccumulator: @unchecked Sendable { private let lock = NSLock() private var data = Data() diff --git a/Sources/Crow/App/Scaffolder.swift b/Sources/Crow/App/Scaffolder.swift index c4f4f4d..0fe01b2 100644 --- a/Sources/Crow/App/Scaffolder.swift +++ b/Sources/Crow/App/Scaffolder.swift @@ -180,9 +180,19 @@ struct Scaffolder { // `Pipe()` would block the child if it ever wrote >64KB before exit. proc.standardOutput = FileHandle.nullDevice + // Drain stderr concurrently for the same reason stdout uses + // /dev/null: a chatty failure mode (Go panic with stack trace, + // assertion dump) can easily exceed the ~64KB pipe buffer, and an + // undrained `Pipe()` would block the child on write. The dedicated + // reader blocks on `readDataToEndOfFile` (EOF arrives when corveil + // exits or post-SIGTERM) and signals a semaphore so we can collect + // deterministically. + let stderrDrain = PipeDrainer.start(stderrPipe) + do { try proc.run() } catch { + stderrDrain.abandon() NSLog("[Scaffolder] corveil launch failed: %@", error.localizedDescription) return "Corveil skill install failed β€” \(error.localizedDescription). Check path in Settings." } @@ -202,20 +212,23 @@ struct Scaffolder { Thread.sleep(forTimeInterval: 0.05) } NSLog("[Scaffolder] corveil skill install timed out after %.1fs", Self.corveilInstallTimeout) + stderrDrain.abandon() return "Corveil skill install timed out after \(Int(Self.corveilInstallTimeout))s β€” binary may be hung. Check path in Settings." } Thread.sleep(forTimeInterval: 0.05) } if proc.terminationStatus != 0 { - let stderr = (try? stderrPipe.fileHandleForReading.readToEnd()) - .flatMap { String(data: $0, encoding: .utf8) }? + let stderr = String(data: stderrDrain.collect(within: Self.pipeDrainGrace), encoding: .utf8)? .trimmingCharacters(in: .whitespacesAndNewlines) ?? "" NSLog("[Scaffolder] corveil skill install exit=%d stderr=%@", proc.terminationStatus, stderr) let detail = stderr.isEmpty ? "exit code \(proc.terminationStatus)" : stderr return "Corveil skill install failed β€” \(detail). Check path in Settings." } + // Success: drain the (typically empty) stderr to release the worker + // thread before returning. + _ = stderrDrain.collect(within: Self.pipeDrainGrace) NSLog("[Scaffolder] corveil skill installed at %@", target) return nil } @@ -227,6 +240,12 @@ struct Scaffolder { /// one ~10KB file. static let corveilInstallTimeout: TimeInterval = 5.0 + /// Bounded post-exit wait for the stderr drain to flush. Once the child + /// is gone (or SIGTERM'd), EOF should arrive near-instantly; cap at 1s + /// so an uncooperative child that swallowed SIGTERM can't pin startup + /// past `corveilInstallTimeout`. + static let pipeDrainGrace: TimeInterval = 1.0 + // MARK: - Bundled Templates /// The CLAUDE.md template bundled with the app. @@ -439,3 +458,57 @@ struct Scaffolder { return nil } } + +/// Background drainer for a `Pipe`. Runs `readDataToEndOfFile()` off the +/// caller's thread so the child can't pipe-buffer-deadlock against an +/// unread `Pipe()`, and signals a semaphore on EOF so the caller can +/// collect deterministically. Mirrors the helper used in +/// `SettingsView.runCorveilVersion`; duplicated here because Scaffolder +/// lives in the `Crow` target and SettingsView in `CrowUI` β€” no shared +/// internal home today, and the helper is small enough that two copies +/// beat adding a CrowCore utility just for two callers. +fileprivate final class PipeDrainer: @unchecked Sendable { + private let pipe: Pipe + private let acc = DataAccumulator() + private let done = DispatchSemaphore(value: 0) + + private init(pipe: Pipe) { + self.pipe = pipe + } + + static func start(_ pipe: Pipe) -> PipeDrainer { + let drainer = PipeDrainer(pipe: pipe) + DispatchQueue.global(qos: .utility).async { + let data = drainer.pipe.fileHandleForReading.readDataToEndOfFile() + if !data.isEmpty { + drainer.acc.append(data) + } + drainer.done.signal() + } + return drainer + } + + func collect(within timeout: TimeInterval) -> Data { + _ = done.wait(timeout: .now() + timeout) + return acc.snapshot() + } + + func abandon() { + try? pipe.fileHandleForReading.close() + } +} + +private final class DataAccumulator: @unchecked Sendable { + private let lock = NSLock() + private var data = Data() + + func append(_ chunk: Data) { + lock.lock(); defer { lock.unlock() } + data.append(chunk) + } + + func snapshot() -> Data { + lock.lock(); defer { lock.unlock() } + return data + } +} From 84f2fa5ef4bc13d43220b961a1e19ade74be9ff6 Mon Sep 17 00:00:00 2001 From: Danny Gershman Date: Thu, 11 Jun 2026 13:26:23 -0400 Subject: [PATCH 5/8] Close parent pipe write ends after spawn so drain gets EOF MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI test `zeroExitWithStdoutShowsSnippet` failed: `/bin/echo --version` produced an empty captured snippet, returning "βœ“ Verified" instead of "βœ“ --version". Root cause: Foundation's `Process` only closes the parent's copy of a `Pipe`'s write FD as part of `waitUntilExit()`. Both the install path (`Scaffolder.installCorveilSkill`) and the Verify path (`SettingsView.runCorveilVersion`) use a polling loop on `proc.isRunning` instead of `waitUntilExit()`, so the parent's writeFD stayed open. EOF never arrived on the read end, the drain thread's `readDataToEndOfFile()` blocked forever, and the snapshot returned empty after the 1-second `pipeDrainGrace` expired. The `true`/`false` test cases passed accidentally because empty captured output happens to produce the expected string for those exit codes. The echo case exposed the bug because it depended on captured stdout. Fix: after `proc.run()` succeeds, explicitly close the parent's copies of the pipe write ends so EOF is delivered when the child exits. The child already has its own dup'd FD, so closing the parent's copy is safe. πŸ¦β€β¬› Generated with Claude Code, orchestrated by Crow Co-Authored-By: Claude Crow-Session: 8E1A62FA-5A6E-4F07-948F-8B6F379F8FE3 --- Packages/CrowUI/Sources/CrowUI/SettingsView.swift | 11 +++++++++++ Sources/Crow/App/Scaffolder.swift | 7 +++++++ 2 files changed, 18 insertions(+) diff --git a/Packages/CrowUI/Sources/CrowUI/SettingsView.swift b/Packages/CrowUI/Sources/CrowUI/SettingsView.swift index 21990b1..509703c 100644 --- a/Packages/CrowUI/Sources/CrowUI/SettingsView.swift +++ b/Packages/CrowUI/Sources/CrowUI/SettingsView.swift @@ -667,6 +667,17 @@ public struct SettingsView: View { errDrain.abandon() return "βœ— Could not launch: \(error.localizedDescription)" } + // Close the parent's copies of the pipe write ends so EOF arrives at + // our read ends when the child exits. Foundation's `Process` only + // closes them as part of `waitUntilExit()`; this code uses a polling + // loop on `isRunning` instead, so if we don't close them ourselves + // the drain threads' `readDataToEndOfFile()` will block forever + // waiting for an EOF that never comes (which would show up as empty + // captured output even when the child wrote bytes to its stdout). + // The child has its own dup'd fd, so closing the parent's copy is + // safe β€” it doesn't affect the child's ability to write. + try? outPipe.fileHandleForWriting.close() + try? errPipe.fileHandleForWriting.close() // Wall-clock timeout. Poll in 50ms slices so a hung binary is // SIGTERM'd rather than blocking forever; a 500ms grace lets the diff --git a/Sources/Crow/App/Scaffolder.swift b/Sources/Crow/App/Scaffolder.swift index 0fe01b2..eeec320 100644 --- a/Sources/Crow/App/Scaffolder.swift +++ b/Sources/Crow/App/Scaffolder.swift @@ -196,6 +196,13 @@ struct Scaffolder { NSLog("[Scaffolder] corveil launch failed: %@", error.localizedDescription) return "Corveil skill install failed β€” \(error.localizedDescription). Check path in Settings." } + // Close the parent's copy of the stderr write end so EOF arrives at + // our read end when the child exits. Foundation's `Process` only + // closes it as part of `waitUntilExit()`; the polling loop below + // uses `isRunning` instead, so without this the drain thread's + // `readDataToEndOfFile()` would block past `pipeDrainGrace` and the + // failure stderr would be reported as empty. + try? stderrPipe.fileHandleForWriting.close() // Wall-clock timeout: poll for completion in short slices so a hung // process gets SIGTERM'd instead of blocking app launch indefinitely. From b6027118c61bf51ae83881b7ede9854c8c338505 Mon Sep 17 00:00:00 2001 From: Danny Gershman Date: Thu, 11 Jun 2026 13:41:33 -0400 Subject: [PATCH 6/8] Trigger Foundation pipe cleanup via waitUntilExit after polling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous fix (closing `outPipe.fileHandleForWriting` in the parent) was insufficient β€” CI continued to fail with the same empty captured-stdout symptom on `/bin/echo --version`. Root cause: Foundation's `Process` keeps an **internal** copy of each pipe's write FD (separate from the `Pipe.fileHandleForWriting` we expose), and only releases it as part of `waitUntilExit()`'s cleanup. Closing the exposed handle drops one reference but leaves Foundation's own copy open, so the pipe still has a live writer and EOF never reaches the bg drain's `readDataToEndOfFile()`. Fix: after the polling loop has observed `proc.isRunning == false`, call `proc.waitUntilExit()`. The wait itself is a no-op at that point β€” the child is already gone β€” but the cleanup side effect releases Foundation's internal FD copy so EOF is finally delivered to our read ends and the drains complete with the captured bytes. Verified locally: all 5 `SettingsView.runCorveilVersion` tests pass. `zeroExitWithStdoutShowsSnippet` (which depends on captured stdout) now correctly returns "βœ“ --version" for `/bin/echo`. πŸ¦β€β¬› Generated with Claude Code, orchestrated by Crow Co-Authored-By: Claude Crow-Session: 8E1A62FA-5A6E-4F07-948F-8B6F379F8FE3 --- .../CrowUI/Sources/CrowUI/SettingsView.swift | 20 ++++++++--------- Sources/Crow/App/Scaffolder.swift | 22 ++++++++++++------- 2 files changed, 23 insertions(+), 19 deletions(-) diff --git a/Packages/CrowUI/Sources/CrowUI/SettingsView.swift b/Packages/CrowUI/Sources/CrowUI/SettingsView.swift index 509703c..1efe1cc 100644 --- a/Packages/CrowUI/Sources/CrowUI/SettingsView.swift +++ b/Packages/CrowUI/Sources/CrowUI/SettingsView.swift @@ -667,17 +667,6 @@ public struct SettingsView: View { errDrain.abandon() return "βœ— Could not launch: \(error.localizedDescription)" } - // Close the parent's copies of the pipe write ends so EOF arrives at - // our read ends when the child exits. Foundation's `Process` only - // closes them as part of `waitUntilExit()`; this code uses a polling - // loop on `isRunning` instead, so if we don't close them ourselves - // the drain threads' `readDataToEndOfFile()` will block forever - // waiting for an EOF that never comes (which would show up as empty - // captured output even when the child wrote bytes to its stdout). - // The child has its own dup'd fd, so closing the parent's copy is - // safe β€” it doesn't affect the child's ability to write. - try? outPipe.fileHandleForWriting.close() - try? errPipe.fileHandleForWriting.close() // Wall-clock timeout. Poll in 50ms slices so a hung binary is // SIGTERM'd rather than blocking forever; a 500ms grace lets the @@ -696,6 +685,15 @@ public struct SettingsView: View { } Thread.sleep(forTimeInterval: 0.05) } + // `Process` keeps an internal copy of each pipe's write FD that is + // only released as part of `waitUntilExit()`'s cleanup. The polling + // loop above leaves that internal copy open, so the bg drainers' + // `readDataToEndOfFile()` never sees EOF and the snapshot below + // returns empty (the test failure that flagged this). `waitUntilExit` + // is a no-op for the wait itself at this point (the child is already + // gone or being torn down) but its cleanup releases Foundation's FD + // copy so EOF is finally delivered to our read ends. + proc.waitUntilExit() let outStr = String(data: outDrain.collect(within: pipeDrainGrace), encoding: .utf8)? .trimmingCharacters(in: .whitespacesAndNewlines) ?? "" diff --git a/Sources/Crow/App/Scaffolder.swift b/Sources/Crow/App/Scaffolder.swift index eeec320..e2e82c3 100644 --- a/Sources/Crow/App/Scaffolder.swift +++ b/Sources/Crow/App/Scaffolder.swift @@ -196,13 +196,6 @@ struct Scaffolder { NSLog("[Scaffolder] corveil launch failed: %@", error.localizedDescription) return "Corveil skill install failed β€” \(error.localizedDescription). Check path in Settings." } - // Close the parent's copy of the stderr write end so EOF arrives at - // our read end when the child exits. Foundation's `Process` only - // closes it as part of `waitUntilExit()`; the polling loop below - // uses `isRunning` instead, so without this the drain thread's - // `readDataToEndOfFile()` would block past `pipeDrainGrace` and the - // failure stderr would be reported as empty. - try? stderrPipe.fileHandleForWriting.close() // Wall-clock timeout: poll for completion in short slices so a hung // process gets SIGTERM'd instead of blocking app launch indefinitely. @@ -219,11 +212,24 @@ struct Scaffolder { Thread.sleep(forTimeInterval: 0.05) } NSLog("[Scaffolder] corveil skill install timed out after %.1fs", Self.corveilInstallTimeout) - stderrDrain.abandon() + // Even on timeout, run waitUntilExit so Foundation releases + // its internal copy of the stderr writeFD β€” that's the only + // way the bg drain can see EOF and return what little it + // captured (see comment above the success-path call below). + proc.waitUntilExit() + _ = stderrDrain.collect(within: Self.pipeDrainGrace) return "Corveil skill install timed out after \(Int(Self.corveilInstallTimeout))s β€” binary may be hung. Check path in Settings." } Thread.sleep(forTimeInterval: 0.05) } + // `Process` keeps an internal copy of the stderr pipe's write FD that + // is only released as part of `waitUntilExit()`'s cleanup. The polling + // loop above leaves that copy open, so the bg drain's + // `readDataToEndOfFile()` never sees EOF β€” its collect would time out + // empty. `waitUntilExit` is a no-op for the wait itself now (the child + // is already gone) but its cleanup releases Foundation's FD copy so + // EOF reaches our read end and the drain delivers any captured stderr. + proc.waitUntilExit() if proc.terminationStatus != 0 { let stderr = String(data: stderrDrain.collect(within: Self.pipeDrainGrace), encoding: .utf8)? From 71f9f996ba5ed432bffef710a65c0393e9160ea3 Mon Sep 17 00:00:00 2001 From: Danny Gershman Date: Thu, 11 Jun 2026 13:54:48 -0400 Subject: [PATCH 7/8] Drop bg-drain pattern: use waitUntilExit + TimeoutWatchdog + sync read MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The bg-drain + polling pattern was unreliable on CI: `zeroExitWithStdoutShowsSnippet` continued to fail with empty captured stdout from `/bin/echo --version` even after calling `waitUntilExit()` after the polling loop. Calling `waitUntilExit` post-poll is too late β€” polling consumed the exit observation, and Foundation's pipe-FD cleanup runs as a side effect of `waitUntilExit` blocking on the process, not as cleanup after the fact. The reliable pattern: install a `TimeoutWatchdog` (DispatchSourceTimer that SIGTERMs the child on expiry), then call `proc.waitUntilExit()` synchronously. Foundation owns the wait, runs its pipe cleanup, and by the time `waitUntilExit` returns the pipe write FDs are closed. Subsequent `readToEnd()` calls return the captured data immediately. Trade-off (vs the bg-drain approach the round-3 review asked for): a corveil binary that writes >64KB to stderr before exiting would block on a full pipe buffer until the watchdog SIGTERMs it, and we'd lose stderr beyond the buffer. Documented as a known limit β€” real corveil output is <1KB; the bg-drain approach turned out not to fix this anyway in CI, so the simpler pattern wins. Verified locally: all 5 RunCorveilVersion tests pass in 12ms. πŸ¦β€β¬› Generated with Claude Code, orchestrated by Crow Co-Authored-By: Claude Crow-Session: 8E1A62FA-5A6E-4F07-948F-8B6F379F8FE3 --- .../CrowUI/Sources/CrowUI/SettingsView.swift | 165 ++++++------------ Sources/Crow/App/Scaffolder.swift | 142 +++++---------- 2 files changed, 103 insertions(+), 204 deletions(-) diff --git a/Packages/CrowUI/Sources/CrowUI/SettingsView.swift b/Packages/CrowUI/Sources/CrowUI/SettingsView.swift index 1efe1cc..7696930 100644 --- a/Packages/CrowUI/Sources/CrowUI/SettingsView.swift +++ b/Packages/CrowUI/Sources/CrowUI/SettingsView.swift @@ -633,17 +633,15 @@ public struct SettingsView: View { /// actor and trivially testable. Returns a single-line summary suitable /// for inline display. /// - /// Bounded by `verifyTimeout` so a misbehaving binary that hangs on - /// `--version` can't pin the detached Task forever (matches the - /// `Scaffolder.installCorveilSkill` install-path timeout). Each pipe is - /// drained by a dedicated background reader running - /// `readDataToEndOfFile()`, which blocks until the child closes its end. - /// Snapshotting waits on a bounded `DispatchSemaphore`, so there's no - /// race between the polling thread and an in-flight readability handler: - /// either the drain finished cleanly (signal arrived β†’ snapshot is whole) - /// or the bounded wait expires and we report what's there. Combined, - /// these eliminate both the pipe-buffer deadlock and the EOF tail race - /// that `readabilityHandler` introduces. + /// Uses `proc.waitUntilExit()` (with a `TimeoutWatchdog` SIGTERM'ing the + /// child if it hangs) rather than a polling loop on `proc.isRunning`. + /// `waitUntilExit` is the only way to deterministically trigger + /// Foundation's pipe-write-FD cleanup; a polling loop leaves Foundation's + /// internal copy of the writeFD open and the post-exit reads either hang + /// (with a background drain) or return empty (Foundation closes its copy + /// on its own internal schedule). Once `waitUntilExit` returns, both + /// pipe writers (child + Foundation) have closed, so a synchronous + /// `readToEnd()` on each pipe returns immediately with the data. nonisolated static func runCorveilVersion(at path: String) -> String { let fm = FileManager.default guard fm.isExecutableFile(atPath: path) else { @@ -657,48 +655,22 @@ public struct SettingsView: View { proc.standardOutput = outPipe proc.standardError = errPipe - let outDrain = PipeDrainer.start(outPipe) - let errDrain = PipeDrainer.start(errPipe) - do { try proc.run() } catch { - outDrain.abandon() - errDrain.abandon() return "βœ— Could not launch: \(error.localizedDescription)" } - // Wall-clock timeout. Poll in 50ms slices so a hung binary is - // SIGTERM'd rather than blocking forever; a 500ms grace lets the - // process honor SIGTERM before we drop it. - let deadline = Date().addingTimeInterval(verifyTimeout) - var timedOut = false - while proc.isRunning { - if Date() >= deadline { - proc.terminate() - let graceDeadline = Date().addingTimeInterval(0.5) - while proc.isRunning, Date() < graceDeadline { - Thread.sleep(forTimeInterval: 0.05) - } - timedOut = true - break - } - Thread.sleep(forTimeInterval: 0.05) - } - // `Process` keeps an internal copy of each pipe's write FD that is - // only released as part of `waitUntilExit()`'s cleanup. The polling - // loop above leaves that internal copy open, so the bg drainers' - // `readDataToEndOfFile()` never sees EOF and the snapshot below - // returns empty (the test failure that flagged this). `waitUntilExit` - // is a no-op for the wait itself at this point (the child is already - // gone or being torn down) but its cleanup releases Foundation's FD - // copy so EOF is finally delivered to our read ends. + // Watchdog: SIGTERM after `verifyTimeout` so a hung binary unblocks + // `waitUntilExit` below. The watchdog also records the timeout so we + // can distinguish a normal exit-N from a wall-clock kill. + let watchdog = TimeoutWatchdog(deadline: verifyTimeout, proc: proc) + watchdog.start() proc.waitUntilExit() + let timedOut = watchdog.cancel() - let outStr = String(data: outDrain.collect(within: pipeDrainGrace), encoding: .utf8)? - .trimmingCharacters(in: .whitespacesAndNewlines) ?? "" - let errStr = String(data: errDrain.collect(within: pipeDrainGrace), encoding: .utf8)? - .trimmingCharacters(in: .whitespacesAndNewlines) ?? "" + let outStr = Self.readAll(outPipe) + let errStr = Self.readAll(errPipe) let combined = [outStr, errStr].filter { !$0.isEmpty }.joined(separator: " β€” ") let snippet = combined.split(separator: "\n").first.map(String.init) ?? combined @@ -712,6 +684,15 @@ public struct SettingsView: View { return "βœ— \(detail)" } + /// Synchronously read all bytes from a pipe's read end after the child + /// has exited (so `readToEnd()` returns immediately). Trims whitespace + /// and returns a UTF-8 string. + nonisolated static func readAll(_ pipe: Pipe) -> String { + let data = (try? pipe.fileHandleForReading.readToEnd()) ?? Data() + return String(data: data, encoding: .utf8)? + .trimmingCharacters(in: .whitespacesAndNewlines) ?? "" + } + /// Wall-clock budget for the "Verify" subprocess. Matches the install /// path's `Scaffolder.corveilInstallTimeout` β€” a corveil that hangs on /// `--version` is bounded to the same 5s window as one that hangs on @@ -719,12 +700,6 @@ public struct SettingsView: View { /// stays responsive while this wait elapses. nonisolated static let verifyTimeout: TimeInterval = 5.0 - /// Bounded post-exit wait for the pipe drain to flush. Once the child - /// is gone (or SIGTERM'd), EOF should arrive near-instantly; cap at 1s - /// so an uncooperative child that swallowed SIGTERM can't pin the - /// caller past `verifyTimeout`. - nonisolated static let pipeDrainGrace: TimeInterval = 1.0 - /// One per-action agent picker (Coding/Reviews/Jobs). "Use default" /// removes the override; selecting a concrete agent writes the /// `config.agentsByKind` entry. Disabled until a second agent is @@ -763,70 +738,44 @@ public struct SettingsView: View { } } -/// Background drainer for a `Pipe`. Runs `readDataToEndOfFile()` off the -/// caller's thread so the child can't pipe-buffer-deadlock against an -/// unread `Pipe()`, and signals a semaphore on EOF so the caller can -/// collect deterministically without racing in-flight callbacks. +/// SIGTERM a `Process` after `deadline` seconds if it's still running. Used +/// to bound `waitUntilExit` without a polling loop (a polling loop on +/// `proc.isRunning` defeats Foundation's pipe-write-FD cleanup, which only +/// runs as part of `waitUntilExit`). `cancel()` stops the timer and reports +/// whether it had already fired. /// -/// `@unchecked Sendable` is sound because the only writer is the dispatched -/// closure (single-shot) and the only reader (`collect`) waits on the -/// semaphore β€” the signal establishes a happens-before relationship -/// between the drain's final `acc.append` and the snapshot read. -fileprivate final class PipeDrainer: @unchecked Sendable { - private let pipe: Pipe - private let acc = DataAccumulator() - private let done = DispatchSemaphore(value: 0) - - private init(pipe: Pipe) { - self.pipe = pipe +/// `@unchecked Sendable` is sound here: every member is either immutable +/// (`proc`, `timer`) or guarded by `lock` (`didFire`). `proc.terminate()` +/// and `proc.isRunning` are thread-safe per Foundation. +fileprivate final class TimeoutWatchdog: @unchecked Sendable { + private let proc: Process + private let timer: DispatchSourceTimer + private let lock = NSLock() + private var didFire = false + + init(deadline: TimeInterval, proc: Process) { + self.proc = proc + self.timer = DispatchSource.makeTimerSource(queue: .global(qos: .userInitiated)) + self.timer.schedule(deadline: .now() + deadline) } - /// Spawn the drain. `readDataToEndOfFile` blocks the worker thread - /// until the child closes its write end (clean exit or post-SIGTERM). - static func start(_ pipe: Pipe) -> PipeDrainer { - let drainer = PipeDrainer(pipe: pipe) - DispatchQueue.global(qos: .utility).async { - let data = drainer.pipe.fileHandleForReading.readDataToEndOfFile() - if !data.isEmpty { - drainer.acc.append(data) + func start() { + timer.setEventHandler { [weak self] in + guard let self else { return } + self.lock.lock() + self.didFire = true + self.lock.unlock() + if self.proc.isRunning { + self.proc.terminate() } - drainer.done.signal() } - return drainer - } - - /// Wait up to `timeout` seconds for the drain to complete, then return - /// whatever has been accumulated. Safe to call after `abandon()`. - func collect(within timeout: TimeInterval) -> Data { - _ = done.wait(timeout: .now() + timeout) - return acc.snapshot() - } - - /// Close the read end so the drain thread's `readDataToEndOfFile` can - /// return immediately. Used when the process never launched and the - /// pipe will never see a writer. - func abandon() { - try? pipe.fileHandleForReading.close() - } -} - -/// Lock-serialized `Data` accumulator. Used by `PipeDrainer` so the -/// background drain thread and the foreground collector share state -/// without tripping Swift 6 sendable rules. The lock is technically -/// redundant once `PipeDrainer.collect` waits on the semaphore (the -/// signal is a happens-before barrier), but it's cheap and keeps the -/// accumulator safe in isolation. -private final class DataAccumulator: @unchecked Sendable { - private let lock = NSLock() - private var data = Data() - - func append(_ chunk: Data) { - lock.lock(); defer { lock.unlock() } - data.append(chunk) + timer.resume() } - func snapshot() -> Data { + /// Cancel the watchdog. Returns true if it had already fired (timeout). + func cancel() -> Bool { + timer.cancel() lock.lock(); defer { lock.unlock() } - return data + return didFire } } diff --git a/Sources/Crow/App/Scaffolder.swift b/Sources/Crow/App/Scaffolder.swift index e2e82c3..89c70d4 100644 --- a/Sources/Crow/App/Scaffolder.swift +++ b/Sources/Crow/App/Scaffolder.swift @@ -180,68 +180,37 @@ struct Scaffolder { // `Pipe()` would block the child if it ever wrote >64KB before exit. proc.standardOutput = FileHandle.nullDevice - // Drain stderr concurrently for the same reason stdout uses - // /dev/null: a chatty failure mode (Go panic with stack trace, - // assertion dump) can easily exceed the ~64KB pipe buffer, and an - // undrained `Pipe()` would block the child on write. The dedicated - // reader blocks on `readDataToEndOfFile` (EOF arrives when corveil - // exits or post-SIGTERM) and signals a semaphore so we can collect - // deterministically. - let stderrDrain = PipeDrainer.start(stderrPipe) - do { try proc.run() } catch { - stderrDrain.abandon() NSLog("[Scaffolder] corveil launch failed: %@", error.localizedDescription) return "Corveil skill install failed β€” \(error.localizedDescription). Check path in Settings." } - // Wall-clock timeout: poll for completion in short slices so a hung - // process gets SIGTERM'd instead of blocking app launch indefinitely. - // `waitUntilExit()` has no timeout overload; this is the standard - // Foundation workaround. - let deadline = Date().addingTimeInterval(Self.corveilInstallTimeout) - while proc.isRunning { - if Date() >= deadline { - proc.terminate() - // Give the process up to 500ms to honor SIGTERM before - // returning the warning, so we don't leave a zombie behind. - let graceDeadline = Date().addingTimeInterval(0.5) - while proc.isRunning, Date() < graceDeadline { - Thread.sleep(forTimeInterval: 0.05) - } - NSLog("[Scaffolder] corveil skill install timed out after %.1fs", Self.corveilInstallTimeout) - // Even on timeout, run waitUntilExit so Foundation releases - // its internal copy of the stderr writeFD β€” that's the only - // way the bg drain can see EOF and return what little it - // captured (see comment above the success-path call below). - proc.waitUntilExit() - _ = stderrDrain.collect(within: Self.pipeDrainGrace) - return "Corveil skill install timed out after \(Int(Self.corveilInstallTimeout))s β€” binary may be hung. Check path in Settings." - } - Thread.sleep(forTimeInterval: 0.05) - } - // `Process` keeps an internal copy of the stderr pipe's write FD that - // is only released as part of `waitUntilExit()`'s cleanup. The polling - // loop above leaves that copy open, so the bg drain's - // `readDataToEndOfFile()` never sees EOF β€” its collect would time out - // empty. `waitUntilExit` is a no-op for the wait itself now (the child - // is already gone) but its cleanup releases Foundation's FD copy so - // EOF reaches our read end and the drain delivers any captured stderr. + // Watchdog: SIGTERM after `corveilInstallTimeout` so a hung binary + // unblocks `waitUntilExit`. The watchdog records the timeout so we + // can distinguish exit-N from wall-clock kill below. `waitUntilExit` + // (not a polling loop) is what triggers Foundation's pipe-write-FD + // cleanup, which is the only way the post-exit `readToEnd()` below + // reliably sees EOF. + let watchdog = ScaffolderTimeoutWatchdog(deadline: Self.corveilInstallTimeout, proc: proc) + watchdog.start() proc.waitUntilExit() + let timedOut = watchdog.cancel() + if timedOut { + NSLog("[Scaffolder] corveil skill install timed out after %.1fs", Self.corveilInstallTimeout) + return "Corveil skill install timed out after \(Int(Self.corveilInstallTimeout))s β€” binary may be hung. Check path in Settings." + } if proc.terminationStatus != 0 { - let stderr = String(data: stderrDrain.collect(within: Self.pipeDrainGrace), encoding: .utf8)? + let stderr = (try? stderrPipe.fileHandleForReading.readToEnd()) + .flatMap { String(data: $0, encoding: .utf8) }? .trimmingCharacters(in: .whitespacesAndNewlines) ?? "" NSLog("[Scaffolder] corveil skill install exit=%d stderr=%@", proc.terminationStatus, stderr) let detail = stderr.isEmpty ? "exit code \(proc.terminationStatus)" : stderr return "Corveil skill install failed β€” \(detail). Check path in Settings." } - // Success: drain the (typically empty) stderr to release the worker - // thread before returning. - _ = stderrDrain.collect(within: Self.pipeDrainGrace) NSLog("[Scaffolder] corveil skill installed at %@", target) return nil } @@ -253,12 +222,6 @@ struct Scaffolder { /// one ~10KB file. static let corveilInstallTimeout: TimeInterval = 5.0 - /// Bounded post-exit wait for the stderr drain to flush. Once the child - /// is gone (or SIGTERM'd), EOF should arrive near-instantly; cap at 1s - /// so an uncooperative child that swallowed SIGTERM can't pin startup - /// past `corveilInstallTimeout`. - static let pipeDrainGrace: TimeInterval = 1.0 - // MARK: - Bundled Templates /// The CLAUDE.md template bundled with the app. @@ -472,56 +435,43 @@ struct Scaffolder { } } -/// Background drainer for a `Pipe`. Runs `readDataToEndOfFile()` off the -/// caller's thread so the child can't pipe-buffer-deadlock against an -/// unread `Pipe()`, and signals a semaphore on EOF so the caller can -/// collect deterministically. Mirrors the helper used in -/// `SettingsView.runCorveilVersion`; duplicated here because Scaffolder -/// lives in the `Crow` target and SettingsView in `CrowUI` β€” no shared -/// internal home today, and the helper is small enough that two copies -/// beat adding a CrowCore utility just for two callers. -fileprivate final class PipeDrainer: @unchecked Sendable { - private let pipe: Pipe - private let acc = DataAccumulator() - private let done = DispatchSemaphore(value: 0) - - private init(pipe: Pipe) { - self.pipe = pipe +/// SIGTERM a `Process` after `deadline` seconds if it's still running. Used +/// to bound `waitUntilExit` without a polling loop (a polling loop on +/// `proc.isRunning` consumes the exit observation Foundation needs to run +/// its pipe-write-FD cleanup, so post-exit `readToEnd()` reads return empty). +/// Mirrors `SettingsView`'s `TimeoutWatchdog`; duplicated here because +/// Scaffolder and SettingsView live in different targets with no shared +/// private utility module today, and the helper is small enough that two +/// copies beat introducing a CrowCore type for two callers. +fileprivate final class ScaffolderTimeoutWatchdog: @unchecked Sendable { + private let proc: Process + private let timer: DispatchSourceTimer + private let lock = NSLock() + private var didFire = false + + init(deadline: TimeInterval, proc: Process) { + self.proc = proc + self.timer = DispatchSource.makeTimerSource(queue: .global(qos: .userInitiated)) + self.timer.schedule(deadline: .now() + deadline) } - static func start(_ pipe: Pipe) -> PipeDrainer { - let drainer = PipeDrainer(pipe: pipe) - DispatchQueue.global(qos: .utility).async { - let data = drainer.pipe.fileHandleForReading.readDataToEndOfFile() - if !data.isEmpty { - drainer.acc.append(data) + func start() { + timer.setEventHandler { [weak self] in + guard let self else { return } + self.lock.lock() + self.didFire = true + self.lock.unlock() + if self.proc.isRunning { + self.proc.terminate() } - drainer.done.signal() } - return drainer - } - - func collect(within timeout: TimeInterval) -> Data { - _ = done.wait(timeout: .now() + timeout) - return acc.snapshot() - } - - func abandon() { - try? pipe.fileHandleForReading.close() - } -} - -private final class DataAccumulator: @unchecked Sendable { - private let lock = NSLock() - private var data = Data() - - func append(_ chunk: Data) { - lock.lock(); defer { lock.unlock() } - data.append(chunk) + timer.resume() } - func snapshot() -> Data { + /// Cancel the watchdog. Returns true if it had already fired (timeout). + func cancel() -> Bool { + timer.cancel() lock.lock(); defer { lock.unlock() } - return data + return didFire } } From f17b1d21302565435a7af9d57af76b150cf330c1 Mon Sep 17 00:00:00 2001 From: Danny Gershman Date: Thu, 11 Jun 2026 14:35:46 -0400 Subject: [PATCH 8/8] Align Scaffolder trim with SettingsView (.whitespacesAndNewlines) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-4 review Green: `Scaffolder.installCorveilSkill` trimmed `corveilBinaryPath` with `.whitespaces`, while the SettingsView `corveilBinding` setter trimmed with `.whitespacesAndNewlines`. A config edited by hand to contain a trailing newline would round-trip differently depending on which side processed it. Use the wider character set on both sides. πŸ¦β€β¬› Generated with Claude Code, orchestrated by Crow Co-Authored-By: Claude Crow-Session: 8E1A62FA-5A6E-4F07-948F-8B6F379F8FE3 --- Sources/Crow/App/Scaffolder.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Sources/Crow/App/Scaffolder.swift b/Sources/Crow/App/Scaffolder.swift index 89c70d4..7be35a2 100644 --- a/Sources/Crow/App/Scaffolder.swift +++ b/Sources/Crow/App/Scaffolder.swift @@ -151,7 +151,7 @@ struct Scaffolder { /// after `corveilInstallTimeout` seconds the process is sent SIGTERM and /// the install reports a warning rather than blocking forever. private func installCorveilSkill(_ corveilBinaryPath: String?) -> String? { - guard let path = corveilBinaryPath?.trimmingCharacters(in: .whitespaces), + guard let path = corveilBinaryPath?.trimmingCharacters(in: .whitespacesAndNewlines), !path.isEmpty else { return nil }