Add Reinstall skill button to Settings → Corveil CLI (CROW-491)

[dgershman]

Summary

• Adds a Reinstall skill button next to the existing Verify button in Settings → General → Corveil CLI.
• Runs the same corveil skill install --path … flow as the per-launch path (Add Corveil CLI path picker + auto-install /query-corveil on launch #483), on demand — no restart, workspace switch, or path re-pick required. Serves the common "I just rebuilt corveil locally; install the new embedded skill" loop.
• Disabled when the picker is unset; shows ✓ Skill reinstalled / ✗ <diagnostic> inline (coalesces with Verify's result line — only one runs at a time). A successful manual reinstall also clears the launch-time warning banner; a failed one updates it, so "is corveil broken?" stays single-sourced.

Plumbing

SettingsView lives in the CrowUI package, which cannot import the Crow app target where Scaffolder lives. The call therefore goes through a new AppState.onReinstallCorveilSkill async closure wired in AppDelegate.launchMainApp — same callback-injection pattern as onCreateManager, onRestartManager, etc. The closure offloads the synchronous Process.waitUntilExit work to a detached task internally, so awaiting it from the main actor doesn't pin the UI for the 5-second install timeout.

Scaffolder.installCorveilSkill access is loosened from private to internal so AppDelegate can call it on a fresh Scaffolder instance (this is the "function-accessibility refactor" the ticket references — #490 has not yet landed in this branch).

Test plan

• Configure a valid corveil path → click Reinstall skill → expect transient ✓ Skill reinstalled; {devRoot}/.claude/commands/query-corveil.md byte-matches <corveilPath> skill show
• Rebuild corveil locally with a tweaked embedded skill string → click Reinstall skill again without restarting Crow → expect file content to reflect the new build
• Set path to a non-executable junk value → click → expect ✗ <diagnostic> inline AND the orange warning banner at the top of General (existing corveilSkillInstallWarning surface)
• Successful reinstall after a failure clears the orange banner
• Clear the path → button is disabled with tooltip "Set the Corveil CLI path first."
• Click repeatedly with a valid path → no errors (install is idempotent via os.WriteFile + os.Chmod)
• Verify button and Reinstall button mutually disable while either is running

Closes #491

🤖 Generated with Claude Code

[dhilgaertner]

Code & Security Review

Tidy, well-documented feature. The callback-injection pattern matches the existing onCreateManager/onListWorkspaceRepos precedent, the loosening of installCorveilSkill to internal is appropriately scoped, the single coalesced result line with mutual disabled is a clean way to avoid two competing status lines, and the threading story (async closure offloading Process.waitUntilExit to a detached task) keeps the main actor unpinned for the 5s timeout. CrowCore builds clean; CrowUI/app target couldn't be built locally (missing GhosttyKit.xcframework artifact — pre-existing infra, unrelated to this PR), so those two files were reviewed by inspection.

Security Review

Strengths:

• No new external input surface. The reinstall path reuses Scaffolder.installCorveilSkill, which already validates the binary is executable (isExecutableFile), bounds runtime with a SIGTERM watchdog, discards stdout to a null device to avoid pipe-write deadlock, and routes the binary via executableURL + arguments (no shell, no injection vector).
• Failure diagnostics surfaced to the user come from corveil's own stderr, already truncated/trimmed; nothing sensitive added.

No security concerns.

Code Quality

Yellow — stale devRoot capture: reinstall can target the wrong directory and still report success.
Sources/Crow/App/AppDelegate.swift:433-438 — the closure captures the launch-time let devRoot (line 330) and builds Scaffolder(devRoot: devRoot) from it. But saveSettings(devRoot:config:) (AppDelegate.swift:1153-1154) reassigns self.devRoot when the user edits Development Root — which lives in the same Settings window as the Reinstall button. So the sequence "change devRoot → scroll down → click Reinstall skill" installs query-corveil.md into the previous devRoot's .claude/commands/, while the UI still reports ✓ Skill reinstalled. This diverges from the sibling closure onListWorkspaceRepos (AppDelegate.swift:847-848), which deliberately uses [weak self] + live self.devRoot for exactly this reason. Since the whole selling point of the button is "no restart required," silently honoring a stale devRoot is surprising. Suggest { [weak self] path in ... read self?.devRoot on the main actor, then pass into Task.detached }, mirroring onListWorkspaceRepos.

Green — false ✓ Skill reinstalled when the closure is unwired.
SettingsView.swift:675-688 — await appState.onReinstallCorveilSkill?(path) returns nil both on real success and when the closure isn't set (previews/unit tests). nil falls into the success branch, printing ✓ Skill reinstalled and clearing the warning banner for an install that never ran. Harmless in production (always wired), but the "degrade gracefully" comment oversells it — it degrades to a false positive. Consider a sentinel/explicit nil-closure branch if previews ever exercise this.

Green — result line not cleared on manual path edits.
The corveilReinstallResult doc comment says it's cleared "on path edits so stale results don't outlive a binary swap," but only the Browse button (SettingsView.swift:285-287) clears it. Typing a new path into the TextField + onSubmit { save() } (SettingsView.swift:274-276) leaves a stale ✓ Skill reinstalled visible against the new path. Functionality is unaffected (disabled/run logic reads the live binding); only the displayed line is briefly stale. Consider clearing both results in an .onChange of the binding, or soften the comment.

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 (stale devRoot) is the only blocker; addressing it in the same round avoids a follow-up.

---

🐦‍⬛ Reviewed by Crow via Claude Code

[dhilgaertner]

Code & Security Review

Critical Issues

None.

Security Review

Strengths:

• No new attack surface. Reinstall reuses the existing per-launch installCorveilSkill flow — the user-configured corveil binary is invoked with an argument array (["skill", "install", "--path", target]), so there is no shell/command-injection vector.
• isExecutableFile is still gated before launch; the failure path returns a sanitized, user-facing diagnostic and logs via NSLog (no secrets, no network).
• target is derived from devRoot, not from untrusted input.

Concerns: None.

Code Quality

Strong, idiomatic work — the threading claims in the description check out:

• Concurrency is race-free. SettingsView is main-actor-isolated (confirmed by the pre-existing synchronous @State mutation at SettingsView.swift:648, which only compiles under swift-tools 6.0 if the method is @MainActor). The new Task { } in reinstallCorveilSkill (SettingsView.swift:679) therefore inherits main-actor isolation, making the direct @State/appState mutations safe, while the blocking Process.waitUntilExit is correctly offloaded to Task.detached inside the AppDelegate closure (AppDelegate.swift:438).
• Live devRoot read (AppDelegate.swift:437) correctly avoids installing into a stale launch-time devRoot after an in-window Settings change.
• Unwired-callback guard (SettingsView.swift:685) properly distinguishes "real success (nil)" from "never ran," preventing a false ✓.
• Single source of truth for the warning banner is preserved — success clears corveilSkillInstallWarning, failure updates it.
• private→internal on installCorveilSkill is module-scoped and well documented.

Green considerations (non-blocking):

• In-flight path edit staleness (SettingsView.swift:671): the path TextField stays editable while a reinstall is running (only the buttons disable). If the user edits the path during the install window, the completing Task writes ✓ Skill reinstalled next to the newly-typed path. The install itself is correct (it captured the path at click time + reads devRoot live); this is purely cosmetic and self-corrects on the next action.
• Theoretical false-success (AppDelegate.swift:437): if self (AppDelegate) were deallocated, the closure returns nil, which the caller reads as success. AppDelegate lives for the app lifetime, so this is not reachable in practice.
• No automated tests for the new glue. The success/failure/unwired branching is thin async UI glue, but unlike verifyCorveil (which extracted runCorveilVersion as a pure, testable helper) nothing here is unit-tested. Acceptable given the manual test plan.

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

---

🐦‍⬛ Reviewed by Crow via Claude Code