Add Corveil CLI path picker + auto-install /query-corveil on launch

[dgershman]

Summary

• Adds a generic defaults.binaries map (keyed by tool name) to ConfigDefaults, with a Settings → General → "Corveil CLI" picker for the corveil slot (file-only NSOpenPanel + Verify button that runs <path> --version).
• On every Crow launch, Scaffolder invokes <corveilPath> skill install --path {devRoot}/.claude/commands/query-corveil.md so the embedded slash command stays in sync with the user's locally-built corveil — no skill source in crow/skills/, corveil owns the content.
• Failures are surfaced via a new appState.corveilSkillInstallWarning banner using the existing <X>Warning pattern (matches githubScopeWarning/rateLimitWarning). Unset path = no-op. Non-executable or non-zero exit = log + transient Settings banner, never blocks startup.

Closes #482

Test plan

• swift build --arch arm64 succeeds (verified — clean compile)
• Launch Crow with the corveil path unset → no query-corveil.md written, no banner, no crash.
• Open Settings → General → Corveil CLI, browse to a real corveil binary → path persists to config.json under defaults.binaries.corveil.
• Click "Verify" → green ✓ corveil version ... line.
• Restart Crow → {devRoot}/.claude/commands/query-corveil.md exists and diff <(corveil skill show) <(cat ...) is empty.
• /query-corveil slash command loads in a Claude Code session under that devroot.
• Point picker at /tmp/not-corveil → relaunch shows the orange Settings banner; no query-corveil.md written.
• Idempotency: relaunch repeatedly, file content is byte-identical each time.

🤖 Generated with Claude Code

[dhilgaertner]

Code & Security Review

Clean, well-documented feature. The generic defaults.binaries map with decodeIfPresent backward-compat, the off-main Task.detached Verify path, and the non-fatal ScaffoldResult.warning surfaced via the established <X>Warning banner pattern are all done right. Two should-fix items below.

Critical Issues

None.

Security Review

Strengths:

• No shell involved — Process is invoked with executableURL + a fixed arguments array (["skill","install","--path",target] / ["--version"]), so a path or target with spaces/metacharacters cannot inject. Arg-injection safe.
• isExecutableFile(atPath:) gate before exec; non-executable path is a no-op warning, never a crash.
• The binary is the user's own locally-configured CLI — no trust-boundary crossing.

Concerns:

• None.

Code Quality

🟡 Launch-path subprocess blocks the main thread with no timeout — Sources/Crow/App/Scaffolder.swift:180. installCorveilSkill calls proc.waitUntilExit() synchronously, and scaffold(...) runs on the main thread during applicationDidFinishLaunching → launchMainApp (AppDelegate.swift:85,316,403). A corveil binary that hangs (blocks on stdin, opens a prompt, wrong executable) freezes app startup indefinitely — with no window yet drawn and the single-instance lock already held, the user has no escape. Note the inconsistency: the Verify button correctly uses Task.detached to avoid blocking the UI (SettingsView.swift), yet the riskier launch path runs unguarded. The isExecutableFile gate prevents a missing/non-exec binary but does not bound runtime. Suggest either running the install off the main actor (and assigning corveilSkillInstallWarning via MainActor.run) or adding a wall-clock timeout that terminates the process and returns a warning.

🟡 No test coverage for the new binaries field or the corveil helpers — the field added immediately before this one, ignoreReviewLabels, has both ignoreReviewLabelsRoundTrip() and ignoreReviewLabelsDefaultsEmptyWhenKeyMissing() (Packages/CrowCore/Tests/CrowCoreTests/AppConfigTests.swift:441,450). binaries has neither, so the round-trip and the decodeIfPresent ?? [:] backward-compat path are unverified. runCorveilVersion(at:) is nonisolated static and described in its own doc comment as "trivially testable," yet has no test. Adding a round-trip + missing-key test (matching the established convention) and a couple of runCorveilVersion cases (non-executable path, exit-code formatting) would close the gap.

🟢 corveil install runs twice on first-time setup — completeSetup runs scaffold(...) (AppDelegate.swift:303) and then calls launchMainApp() which runs scaffold(...) again (AppDelegate.swift:403), so the subprocess fires twice on the initial-setup launch. Harmless given idempotency, but the line-308 assignment is immediately superseded by line 408.

🟢 The corveilBinding empty-string-removes-key handling and trim-on-commit are a nice touch that keeps the persisted map clean.

Summary Table

Color	Meaning	Verdict effect
Red	Must fix	Request changes
Yellow	Should fix	Request changes
Green	Consider	Approve allowed

Recommendation: Request Changes — driven by [0 Red, 2 Yellow, 2 Green] findings.

---

🐦‍⬛ Reviewed by Crow via Claude Code

[dhilgaertner]

Code & Security Review

Solid, well-documented feature. Forward-compatible config decode, non-fatal failure handling, a wall-clock timeout on the startup subprocess, and good test coverage. One Yellow asymmetry keeps this from an approve.

Critical Issues

None.

Security Review

Strengths:

• No shell injection: Process.executableURL + fixed argument array (["skill","install","--path",target]), never routed through a shell.
• target is a fixed, derived path ({devRoot}/.claude/commands/query-corveil.md) — no traversal from user input.
• Executable-bit gate (isExecutableFile) before launch; unset/empty path is a clean no-op.

Concerns:

• The configured corveil binary is auto-executed on every launch (skill install) and on demand (--version). This is by design and user-configured, so it's acceptable — worth being aware that a tampered config.json becomes code execution at launch, but that's already true of the devRoot/agent config surface. (Green)

Code Quality

• 🟡 runCorveilVersion / verifyCorveil (SettingsView.swift) has no timeout, unlike the Scaffolder path. installCorveilSkill deliberately bounds a hung binary with corveilInstallTimeout (5s + SIGTERM), with an explicit comment that a hung corveil binary must not block. The Verify button runs the same class of user-configured binary via proc.waitUntilExit() with no timeout. If the binary hangs on --version, the detached Task and child process leak indefinitely and the button is stuck on "Verifying…" with no recovery. It's off the main actor so the UI doesn't freeze, but the inconsistency is a real "should fix." Consider reusing the same poll-with-deadline + terminate() pattern. (Yellow)
• 🟡 Latent pipe-buffer deadlock in runCorveilVersion. stdout/stderr are read after waitUntilExit(). For --version the output is tiny, but a misconfigured binary that writes >64KB before exiting fills the pipe buffer, blocks on write, and — with no timeout (above) — hangs forever. Draining concurrently (or via the timeout loop) closes both gaps at once. (Yellow)
• 🟢 In Scaffolder.installCorveilSkill, stdout is an undrained Pipe(); a chatty corveil binary could fill the buffer and block, but the 5s timeout bounds it to a warning. Functionally safe; consider FileHandle.nullDevice to make the discard explicit and deadlock-proof. (Green)
• 🟢 onRescaffold uses try?; if scaffold throws, result is nil and the else if result != nil branch is skipped, so a stale warning from a prior launch persists across a failed rescaffold. Minor. (Green)
• 🟢 First-time-setup double-run avoidance (corveilBinaryPath: nil in completeSetup, real path in launchMainApp) is correctly reasoned — verified completeSetup calls launchMainApp() right after. (Green)

Tests

• CrowCore binariesRoundTrip / binariesDefaultsEmptyWhenKeyMissing pass locally; forward-compat decode is well covered.
• CrowUI RunCorveilVersionTests couldn't run locally (missing GhosttyKit xcframework binary), but the assumptions hold on macOS — verified /bin/echo --version → --version exit 0, /usr/bin/true exit 0, /usr/bin/false exit 1.

Summary Table

Color	Meaning	Verdict effect
Red	Must fix	Request changes
Yellow	Should fix	Request changes
Green	Consider	Approve allowed

Recommendation: Request Changes — driven by [0 Red, 2 Yellow, 3 Green] findings. The two Yellows are the same root cause: the Verify path lacks the timeout/pipe handling the startup path already has. Add a bounded timeout + concurrent drain to runCorveilVersion and this is an approve.

---

🐦‍⬛ Reviewed by Crow via Claude Code

[dhilgaertner]

Code & Security Review

Solid, well-documented feature: a generic defaults.binaries map, a Settings picker for the corveil slot, and a per-launch corveil skill install with non-fatal warning surfacing. Forward-compat decoding is tested, the timeout/SIGTERM handling is thoughtful, and the <X>Warning banner pattern matches existing conventions. go/swift build couldn't run here (the workspace's GhosttyKit.xcframework binary artifact is absent in this checkout — environment, not this PR), but CrowCore tests pass including the new binariesRoundTrip / binariesDefaultsEmptyWhenKeyMissing cases.

Two issues, both in subprocess pipe handling.

Security Review

Strengths:

• No injection surface: Process.arguments is an argv array (no shell), and the binary path is user-chosen via NSOpenPanel / explicit text entry — not attacker-controlled.
• isExecutableFile gate before launch; failures are non-fatal and never block startup.
• No secrets logged; stderr is truncated to a single line for display.

Concerns: none security-blocking.

Code Quality

• 🟡 Scaffolder.installCorveilSkill leaves stderr as an undrained Pipe() — the exact deadlock the code's own comment claims to avoid. Sources/Crow/App/Scaffolder.swift: stdout is routed to FileHandle.nullDevice with the comment "routing to /dev/null is deadlock-proof — an undrained Pipe() would block the child if it ever wrote >64KB before exit." But stderr is a plain Pipe() that is only read via readToEnd() after the process exits. If corveil writes more than the ~64KB pipe buffer to stderr before exiting, the child blocks on the full pipe, never exits, and you hit the 5s timeout + SIGTERM — and since scaffold(...) runs on the main thread during applicationDidFinishLaunching, that's a 5s stall before first paint. The Verify path (runCorveilVersion) already solves this correctly by draining both pipes concurrently via DataAccumulator; the install path should do the same (or accept that stderr can be lost). It's bounded by the timeout and a >64KB stderr from skill install is unlikely, hence yellow not red — but it directly contradicts the stated invariant and is the inconsistency most worth closing.

• 🟡 runCorveilVersion snapshots the accumulators with no EOF barrier — a tail chunk delivered exactly at process exit can be dropped. Packages/CrowUI/Sources/CrowUI/SettingsView.swift: once the poll loop observes proc.isRunning == false, it immediately calls outAcc.snapshot() / errAcc.snapshot(), then defer nils the readability handlers. The handlers run on a Foundation-managed background queue with no happens-before relationship to the polling thread, so the final readable event delivered at EOF may not have been appended yet when you snapshot (and nilling the handler then drops it). In practice the 50ms poll slack makes this near-deterministic for tiny --version output (why the exact-equality tests pass), but it's technically racy and could intermittently truncate or blank the result. Draining any remaining data synchronously after exit (e.g. readDataToEndOfFile() on each handle before nilling handlers) would make it deterministic.

Notes (non-blocking)

• 🟢 The 5s main-thread block in installCorveilSkill on a misconfigured/hung binary delays first paint. It's documented and bounded, and the Verify path runs off-actor via Task.detached; consider mirroring that here if startup latency ever matters.
• 🟢 Persistence on the corveil TextField only fires via .onSubmit/Browse (not per keystroke) — consistent with the existing devRoot field, just noting typing + closing Settings without Enter won't persist.

Summary Table

Color	Meaning	Verdict effect
Red	Must fix	Request changes
Yellow	Should fix	Request changes
Green	Consider	Approve allowed

Recommendation: Request Changes — driven by [0 Red, 2 Yellow, 2 Green] findings. Both yellows are pipe-drain consistency fixes; addressing the stderr drain in installCorveilSkill to match the Verify path closes the strongest one.

---

🐦‍⬛ Reviewed by Crow via Claude Code

[dhilgaertner]

Code & Security Review

Nicely structured change — generic defaults.binaries map with forward-compatible decoding, fixed-array Process args (no shell, so no command injection), and a non-fatal warning surfaced through the existing <X>Warning pattern. The pipe-drain/timeout work is thoughtful and well-commented. One concurrency bug in the shared PipeDrainer keeps this from approval.

Critical Issues

None blocking, but see the Yellow below — it can crash app startup.

Code Quality

🟡 PipeDrainer.abandon() can raise an uncaught exception → startup crash — Scaffolder.swift:705 / SettingsView.swift:368

abandon() closes pipe.fileHandleForReading, the same handle the background drain thread is reading via readDataToEndOfFile(). On the proc.run()-throws path this is reachable and largely deterministic:

• installCorveilSkill gates on fm.isExecutableFile(atPath:), but that only checks the exec bit — a file with +x that isn't a launchable image (bad shebang, non–Mach-O, ENOEXEC) still passes the gate and makes proc.run() throw (Scaffolder.swift:606-612, SettingsView.swift:262-268).
• In the catch, abandon() runs synchronously and immediately — typically before the GCD global-queue closure has even been scheduled. The closure then calls readDataToEndOfFile() on an already-closed FileHandle, which raises NSFileHandleOperationException ("Bad file descriptor"). That's an Obj-C exception Swift can't catch → crash.
• Even when the read is already in flight, yanking its fd out from under the blocked read() is undefined and can raise the same way.

Scaffolder runs inside applicationDidFinishLaunching, so a mis-pointed corveil path turns into a crash on launch rather than the intended orange banner.

Fix: have abandon() close the write end instead of the read end:

func abandon() {
    try? pipe.fileHandleForWriting.close()
}

Closing the write end delivers a clean EOF, so readDataToEndOfFile() returns normally and signals the semaphore. In the launch-failure case the parent still owns the only write end (the child never spawned), so this reliably unblocks the reader; in the timeout case Process has already closed the parent's write copy, so it's a harmless no-op. Applies to both copies of the helper.

🟢 Main thread blocks up to ~5.5s on a hung corveil — Scaffolder.swift:618-633

The poll-loop + 0.5s SIGTERM grace runs on the main thread before first paint (acknowledged in the comments). 5s is generous, but a hung binary still delays window draw by the full budget. Consider running the install off-main and updating corveilSkillInstallWarning when it lands, so a misbehaving corveil never delays launch at all. Defensible as-is given the timeout bound.

🟢 Trim inconsistency — Scaffolder.swift:568 trims .whitespaces; the UI binding (SettingsView.swift:204) trims .whitespacesAndNewlines. Harmless, but aligning them avoids a path that round-trips differently than it's validated.

🟢 Duplicated PipeDrainer/DataAccumulator across Scaffolder (Crow target) and SettingsView (CrowUI). Already acknowledged in the comments; fine for now, but if a third caller appears a small CrowCore utility would pay off — and would mean the fix above only lands once.

Security Review

Strengths:

• Process.arguments is a fixed array — no shell interpolation, no command injection via path or devRoot.
• isExecutableFile gate before exec; failures are non-fatal and never block startup.
• Forward-compatible config decode (decodeIfPresent ... ?? [:]) — pre-CROW-482 configs still load.

Concerns:

• 🟢 The feature executes a user-configured binary on every launch. That's the intended design and the threat model is local (an attacker who can write config.json already has code-exec footing), so no change needed — just noting the launch-time exec surface.

Tests

• CrowCore binariesRoundTrip / binariesDefaultsEmptyWhenKeyMissing pass.
• The CrowUI/Crow targets don't compile in this review checkout (missing Frameworks/GhosttyKit.xcframework binary artifact — unrelated to this PR), so the runCorveilVersion suite and swift build weren't executed here. The PR notes a clean swift build --arch arm64 locally.

Summary Table

Color	Meaning	Verdict effect
Red	Must fix	Request changes
Yellow	Should fix	Request changes
Green	Consider	Approve allowed

Recommendation: Request Changes — driven by [0 Red, 1 Yellow, 4 Green] findings. The Yellow abandon() fix is small and worth landing in this round; the Greens are optional.

---

🐦‍⬛ Reviewed by Crow via Claude Code

[dgershman]

@dhilgaertner — round-4 review landed against 78fd54f (the bg-drain commit). Since then I pushed two follow-ups (c2bb601, 1a62527) because CI kept failing zeroExitWithStdoutShowsSnippet: proc.waitUntilExit() is the only thing that reliably triggers Foundation's pipe-write-FD cleanup, and calling it after a polling loop is too late — Foundation needs to own the wait.

So the current code (HEAD 301bb5a) drops PipeDrainer/DataAccumulator entirely in favor of TimeoutWatchdog (DispatchSourceTimer SIGTERMing the child on expiry) + proc.waitUntilExit() + synchronous readToEnd() after exit. This means:

• 🟡 PipeDrainer.abandon() close-the-read-end bug — no longer applies, the helper is gone. The launch-failure (proc.run() throws) path now just returns the warning with no fd manipulation. Verified locally that all 5 RunCorveilVersion tests pass under arch -arm64 swift test --filter "RunCorveilVersion".
• 🟢 5.5s main-thread block on hung corveil — same trade-off, same bound. Off-main install is a reasonable future change but not in this round.
• 🟢 Trim inconsistency — fixed in 301bb5a; both sides now use .whitespacesAndNewlines.
• 🟢 Duplicated helper — only one helper (TimeoutWatchdog) is duplicated now, ~30 lines × 2; still under the "extract when there's a third caller" threshold.

Trade-off vs the round-3 bg-drain request: a corveil binary writing >64KB to stderr before exit would now block on a full pipe buffer until the watchdog SIGTERMs it, losing the tail. Documented in-comment. Real corveil emits <1KB and the bg-drain approach turned out not to deliver EOF reliably in CI anyway, so the simpler pattern won.

Ready for re-review against 301bb5a.

[dhilgaertner]

Code & Security Review

Critical Issues

None.

Security Review

Strengths:

• No shell injection. Both subprocess call sites (Scaffolder.installCorveilSkill, SettingsView.runCorveilVersion) use Process with executableURL + an arguments array — never a shell string. The user-supplied path and --path target are passed as discrete argv entries, so paths with spaces/metacharacters can't inject commands.
• Defensive pre-exec gate. FileManager.isExecutableFile(atPath:) is checked before proc.run() in both paths, so a missing/non-executable path degrades to a warning rather than a launch error.
• Inherent trust boundary is appropriate. Executing a user-picked binary is the explicit purpose of the feature (a local dev tool the user built), not an escalation — no untrusted input crosses a boundary.
• Warning surfacing is safe. corveil's stderr is shown in a textSelection-enabled banner; it's the user's own machine output, no secret exposure, and it's truncated to a single line.
• Forward-compatible config decode. binaries uses decodeIfPresent ?? [:], and AppConfigTests.binariesDefaultsEmptyWhenKeyMissing locks in that pre-CROW-482 configs still decode. Verified passing locally (262 CrowCore tests green).

Code Quality

• Clean two-way corveilBinding that treats empty as "unset" (no stale empty map entries) and trims on commit — and 301bb5a aligns the Scaffolder trim charset (.whitespacesAndNewlines) with it.
• First-launch double-install is correctly avoided: completeSetup passes corveilBinaryPath: nil, and launchMainApp (always invoked right after) re-scaffolds with the real path. Verified the control flow at AppDelegate.swift:306 → :318 → :402.
• Warning lifecycle is handled well — onRescaffold always assigns result.warning (clearing a stale banner on later success) and replaces it with a fresh message in the catch.
• Verify runs off the main actor via Task.detached with mutations marshaled back through MainActor.run; the button is disabled while in-flight. UI stays responsive.

Consider (Green — non-blocking)

• Watchdog uses SIGTERM only. TimeoutWatchdog/ScaffolderTimeoutWatchdog call proc.terminate() (SIGTERM) with no SIGKILL escalation. Since Scaffolder.scaffold runs synchronously on the main thread during applicationDidFinishLaunching, a pathological binary that ignores SIGTERM would block first paint indefinitely. Real-world risk is very low (a skill install command terminating on default SIGTERM disposition), but a SIGKILL fallback after a short grace window would make the startup path provably non-hanging.
• Main-thread cost on every launch. Even in the happy path, configured users pay subprocess spawn+exec latency before the window draws (bounded at corveilInstallTimeout = 5s). The tradeoff (skill synced before the Manager session starts) is reasonable and thoroughly documented; flagging only so it's a conscious choice.
• Narrow false-timeout window. If the child exits at almost exactly the deadline, the watchdog handler could set didFire between waitUntilExit() returning and cancel() reading it, mislabeling a success as a timeout. Microsecond window, cosmetic-only impact.
• Watchdog duplicated across two targets. Author documented the rationale (Scaffolder and SettingsView are separate targets with no shared private util); two small copies is a defensible call.

Verification notes

• swift test in Packages/CrowCore passes (262 tests), including both new binaries round-trip/forward-compat tests.
• The main Crow/CrowUI target could not be built here: the prebuilt GhosttyKit.xcframework contains no binary artifact in this checkout (environment limitation, unrelated to this PR). The SettingsView.swift/Scaffolder.swift changes and the runCorveilVersion UI tests were therefore reviewed by inspection; the PR author reports swift build --arch arm64 clean.

Summary Table

Color	Meaning	Verdict effect
Red	Must fix	Request changes
Yellow	Should fix	Request changes
Green	Consider	Approve allowed

Recommendation: Approve — driven by [0 Red, 0 Yellow, 4 Green] findings. Well-engineered, well-documented, tests pass; the Green items are optional hardening/awareness notes the author can weigh.

---

🐦‍⬛ Reviewed by Crow via Claude Code