Hot-trigger corveil skill install when picker value changes (CROW-490)

[dgershman]

Summary

• Settings → Corveil CLI picker now re-runs corveil skill install the moment a new path is committed (Browse confirm or Enter), so /query-corveil reflects the picked binary without an app restart.
• Promoted Scaffolder.installCorveilSkill from private to internal and added a new onCorveilReinstall closure to SettingsView so AppDelegate can dispatch just the install step (off the main thread, bounded by the existing 5s corveilInstallTimeout) instead of re-running the whole Scaffolder.scaffold(...) pass.
• Result is always assigned to the existing AppState.corveilSkillInstallWarning banner — successful or empty/cleared paths clear stale warnings; failures show the same diagnostic as the launch-time path. Verify stays --version only.

Closes #490

Test plan

• Initial pick: Clear defaults.binaries.corveil and delete {devRoot}/.claude/commands/query-corveil.md. Open Settings → Browse to a working corveil binary. Without restarting Crow, confirm query-corveil.md is created.
• Path change: Point the picker at a freshly rebuilt corveil binary. Confirm query-corveil.md content updates to match <newPath> skill install --path /dev/stdout.
• Clear: Empty the field and press Enter. query-corveil.md stays in place; any prior banner clears.
• Broken path: Browse to a non-executable file. Banner shows the same "Corveil skill install skipped — binary at … is missing or not executable" warning that the launch-time path produces. No crash.
• Verify still --version only: Click Verify with a stable path; query-corveil.md mtime is unchanged.

🤖 Generated with Claude Code

[dhilgaertner]

Code & Security Review

Tight, well-scoped change that closes the "must restart to pick up a new corveil binary" gap. The inline documentation (AppDelegate.swift:1103-1119, Scaffolder.swift:231-234) is genuinely good — it explains the off-main-thread rationale, the always-assign banner semantics, and the devRoot capture. Reusing installCorveilSkill + the existing corveilSkillInstallWarning surface instead of re-running the whole Scaffolder.scaffold(...) pass is the right call.

Critical Issues

None.

Security Review

Strengths:

• No new attack surface. The corveil path is user-supplied via the same Settings field that already drove launch-time install; installCorveilSkill re-validates isExecutableFile and runs the binary with a fixed argv (skill install --path <target>) — no shell, no interpolation, no injection vector.
• Subprocess is bounded by the existing 5s corveilInstallTimeout watchdog (SIGTERM), stdout routed to /dev/null, stderr drained after exit — the deadlock-safety story is preserved on the new code path.
• Moving the install off the main thread via Task.detached is correct and avoids freezing the Settings window for up to 5s.

Concerns:

• None security-specific.

Code Quality

Yellow — unsynchronized concurrent installs race on the output file and the banner (AppDelegate.swift:1120-1126)
Each commit (Browse confirm or Enter) fires onCorveilReinstall, which spawns a fresh Task.detached with no serialization, no cancellation of a prior in-flight install, and no staleness guard. Two commits within the install window (~hundreds of ms) produce two problems:

1. Stale banner: completion order is not guaranteed. Browse to a broken binary (task A → warning), then immediately Browse/Enter to a good binary (task B → nil); if A finishes after B, the banner shows a failure for the superseded path while the current path is actually fine. The warning no longer reliably reflects the committed path.
2. Concurrent writes to the same query-corveil.md: two corveil skill install processes writing the same --path target at once can interleave/corrupt the file unless corveil writes atomically (which we can't assume).

The launch-time path is single-shot and serialized; this new path removes that guarantee. Likelihood is low (requires two commits within the install window), but the fix is small — e.g. a monotonically-increasing generation counter captured into the task, applying the result only if it's still the latest, and/or routing installs through a serial actor/queue so they don't overlap. Worth landing in this round trip.

Considerations (Green)

• SettingsView.swift:297 passes the raw url.path while the stored binding (corveilBinding, line 619) trims whitespace; installCorveilSkill trims internally so this is harmless, but passing corveilBinding.wrappedValue would keep the two commit paths consistent.
• The onSubmit and Browse handlers duplicate the "commit → reinstall" logic; a tiny commitCorveil(_:) helper would DRY the two call sites and centralize the nil-on-empty rule.

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, 2 Green] findings. The Yellow (unsynchronized concurrent installs racing on the banner and the output file) is a genuine should-fix; everything else is positive or optional.

---

🐦‍⬛ Reviewed by Crow via Claude Code

[dhilgaertner]

Code & Security Review

Critical Issues

None.

Security Review

Strengths:

• No new attack surface. The picker only invokes a user-chosen local binary (<path> skill install --path <devRoot>/.claude/commands/query-corveil.md) that the user must explicitly select via NSOpenPanel or type — the same trust boundary as the pre-existing launch-time path.
• Executable check is preserved: installCorveilSkill still gates on FileManager.isExecutableFile and surfaces the identical "missing or not executable" warning, so a non-executable pick can't be run.
• No command-string interpolation — Process.arguments is an array, so the binary path can't inject extra args. stdout is routed to nullDevice (deadlock-proof), stderr is bounded and trimmed.
• The 5s corveilInstallTimeout SIGTERM watchdog still bounds a hung binary, now off the main thread so it can't freeze the Settings window.

Concerns: None.

Code Quality

• Correct serialization: enqueueCorveilInstall chains onto corveilInstallTail exactly like the existing reviewKickoffTail (await predecessor → run → assign on main). Two rapid picks can't race on query-corveil.md or clobber corveilSkillInstallWarning out of order, and the last-committed path is the last to write the banner.
• Off-main execution is right: the blocking subprocess runs in a nested Task.detached over value types (Scaffolder struct + String devRoot, no self capture), result assigned back on @MainActor. AppDelegate is @MainActor-isolated, so the closure/enqueueCorveilInstall isolation hops compile cleanly (consistent with the adjacent onRescaffold closure mutating appState directly).
• "Always assign" banner semantics match the launch-time / onRescaffold path: a successful or nil/empty path clears a stale warning; failures show the same diagnostic. The nil-on-empty rule is funneled through the single commitCorveilPath site for both Browse and onSubmit.
• Whitespace normalization is handled — commitCorveilPath reads corveilBinding.wrappedValue (already trimmed by the binding's setter), so the install sees the same string the next launch's scaffolder would.
• installCorveilSkill visibility was widened only from private to internal (not public) — the minimum needed for the same-module AppDelegate caller. Good restraint.
• saveSettings only persists config (no scaffold/install), so commitCorveilPath's save() + onCorveilReinstall is a single install trigger — no accidental double-install.

Consider (non-blocking):

• The "Re-scaffold .claude/ directory" button (onRescaffold) runs a full scaffold — including its own corveil skill install writing the same query-corveil.md and corveilSkillInstallWarning — on a separate domain from corveilInstallTail. A user who commits a picker change (starting a detached install) and then clicks Re-scaffold within the 5s window could run two corveil skill install subprocesses against the same target concurrently. It's narrow (requires deliberate concurrent action), self-healing (the next install/launch rewrites the file), and carries no crash/security impact, so it's out of this PR's stated picker-vs-picker scope — but routing onRescaffold's corveil step through the same tail later would close it fully.

Notes

• Could not run swift build / tests in this review checkout: the vendored Frameworks/GhosttyKit.xcframework has no binary artifact here, so the app target won't link. This is an environment limitation, not a PR defect. Review is based on close reading; the change is small, type-safe Swift consistent with an existing in-repo pattern. The PR's test plan is manual (UI + concurrency glue that's impractical to unit-test).

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, 1 Green] findings.

---

🐦‍⬛ Reviewed by Crow via Claude Code