From 92e4bcd6c50170fd9cb2d47ec4aa581954e0d50b Mon Sep 17 00:00:00 2001 From: Danny Gershman Date: Thu, 11 Jun 2026 17:25:31 -0400 Subject: [PATCH] Symlink defaults.binaries.* into .claude/bin so spawned terminals find them first (CROW-487) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Embedded skills like /query-corveil call bare `corveil`, but Crow's configured path was only consulted at Scaffolder time — once inside an agent terminal the resolution went straight through PATH. This bridges the gap without touching the skill content: - Scaffolder materializes a symlink per defaults.binaries. entry at {devRoot}/.claude/bin/. Loop is idempotent and reaps stale links when keys are removed or targets become non-executable, so a broken pointer can never shadow a working PATH install. - TmuxBackend.configure(crowBinDir:) threads the bin dir through; every registerTerminal call seeds the spawned window with PATH prefixed by the bin dir and exports CROW_BIN_DIR for the shell wrapper. - crow-shell-wrapper.sh re-prepends $CROW_BIN_DIR to PATH AFTER sourcing user rc, so a user `export PATH=…` in .zshrc can't shadow the symlink farm. The wrapper's outer scope already inherits the seeded PATH for fish / unknown-shell branches that don't source rc. Generic in the map — codex / cursor / future tools share the same mechanism via #484's existing schema, no per-tool wiring needed. --- .../Resources/crow-shell-wrapper.sh | 28 ++++ .../Sources/CrowTerminal/TmuxBackend.swift | 21 ++- .../CrowTerminalTests/TmuxBackendTests.swift | 30 ++++ Sources/Crow/App/AppDelegate.swift | 15 +- Sources/Crow/App/Scaffolder.swift | 79 ++++++++- .../ScaffolderBinarySymlinkTests.swift | 154 ++++++++++++++++++ 6 files changed, 321 insertions(+), 6 deletions(-) create mode 100644 Tests/CrowTests/ScaffolderBinarySymlinkTests.swift diff --git a/Packages/CrowTerminal/Sources/CrowTerminal/Resources/crow-shell-wrapper.sh b/Packages/CrowTerminal/Sources/CrowTerminal/Resources/crow-shell-wrapper.sh index 6db6da5..baebb6c 100755 --- a/Packages/CrowTerminal/Sources/CrowTerminal/Resources/crow-shell-wrapper.sh +++ b/Packages/CrowTerminal/Sources/CrowTerminal/Resources/crow-shell-wrapper.sh @@ -33,6 +33,15 @@ export CROW_SENTINEL if [ -n "${CROW_AGENT_KIND:-}" ]; then export CROW_AGENT_KIND; fi if [ -n "${CROW_AGENT_DISPLAY_NAME:-}" ]; then export CROW_AGENT_DISPLAY_NAME; fi +# CROW-487: per-devroot bin dir holds symlinks for every configured +# `defaults.binaries.`. We export it here so the embedded zsh / bash +# rc files (below) can prepend it to PATH *after* sourcing the user's rc — +# that's the only insertion point that survives the user doing +# `export PATH=…` in `.zshrc`. The Swift side already seeded PATH with this +# dir in front via tmux `new-window -e`, so non-rc shells (fish, custom) +# also resolve symlinks correctly. +if [ -n "${CROW_BIN_DIR:-}" ]; then export CROW_BIN_DIR; fi + # CROW_WRAPPER_LOG is optional. Default to /dev/null so the helper is always # safe to call without an unset-var guard. Issue #256. CROW_WRAPPER_LOG="${CROW_WRAPPER_LOG:-/dev/null}" @@ -95,6 +104,17 @@ else crow_log "user_rc_skipped reason=missing rc=$ZDOTDIR/.zshrc" fi +# CROW-487: re-prepend the Crow-managed bin dir AFTER user rc, so a user +# `export PATH=…` in `.zshrc` cannot shadow the symlink farm. Skip the +# re-prepend when the dir is already at the front (avoids unbounded growth +# if a new shell is exec'd inside the existing one). +if [ -n "${CROW_BIN_DIR:-}" ] && [ -d "$CROW_BIN_DIR" ]; then + case ":$PATH:" in + "$CROW_BIN_DIR:"*) ;; # already first — nothing to do + *) export PATH="$CROW_BIN_DIR:$PATH" ;; + esac +fi + _crow_precmd() { crow_log "precmd_fired" if [ -n "${TMUX:-}" ]; then @@ -153,6 +173,14 @@ if [ -f "$HOME/.bashrc" ]; then else crow_log "user_rc_skipped reason=missing rc=$HOME/.bashrc" fi +# CROW-487: re-prepend the Crow-managed bin dir AFTER user rc — see the +# zsh branch above for rationale. +if [ -n "${CROW_BIN_DIR:-}" ] && [ -d "$CROW_BIN_DIR" ]; then + case ":$PATH:" in + "$CROW_BIN_DIR:"*) ;; # already first + *) export PATH="$CROW_BIN_DIR:$PATH" ;; + esac +fi _crow_precmd() { crow_log "precmd_fired" if [ -n "${TMUX:-}" ]; then diff --git a/Packages/CrowTerminal/Sources/CrowTerminal/TmuxBackend.swift b/Packages/CrowTerminal/Sources/CrowTerminal/TmuxBackend.swift index 9251a32..e3a7fdc 100644 --- a/Packages/CrowTerminal/Sources/CrowTerminal/TmuxBackend.swift +++ b/Packages/CrowTerminal/Sources/CrowTerminal/TmuxBackend.swift @@ -111,9 +111,17 @@ public final class TmuxBackend { /// AppDelegate guarantees only one owner). public private(set) var socketPath: String = "" - public func configure(tmuxBinary: String, socketPath: String) { + /// Per-devroot bin dir containing symlinks for `defaults.binaries.` + /// (CROW-487). When non-empty, `registerTerminal` exports `CROW_BIN_DIR` + /// into the spawned tmux window and seeds the window's `PATH` with this + /// directory in front. The shell wrapper re-prepends it after sourcing + /// the user's rc so a user `export PATH=…` can't shadow the symlink farm. + public private(set) var crowBinDir: String = "" + + public func configure(tmuxBinary: String, socketPath: String, crowBinDir: String = "") { self.tmuxBinary = tmuxBinary self.socketPath = socketPath + self.crowBinDir = crowBinDir } // MARK: - Lifecycle @@ -222,6 +230,17 @@ public final class TmuxBackend { } if !cwd.isEmpty { env["PWD"] = cwd } + // CROW-487: hand the per-devroot bin dir to the wrapper so it can + // prepend it to PATH *after* user rc sourcing — that's the only + // insertion point that survives `export PATH=…` in `.zshrc`. We also + // seed the window's PATH directly so non-rc shells (fish, the + // unknown-shell fallback branch of the wrapper, processes that + // bypass the wrapper entirely) still find the symlink farm. + if !crowBinDir.isEmpty { + env["CROW_BIN_DIR"] = crowBinDir + env["PATH"] = "\(crowBinDir):\(ShellEnvironment.shared.resolvedPATH)" + } + let windowIndex = try ctrl.newWindow( name: name, cwd: cwd.isEmpty ? nil : cwd, diff --git a/Packages/CrowTerminal/Tests/CrowTerminalTests/TmuxBackendTests.swift b/Packages/CrowTerminal/Tests/CrowTerminalTests/TmuxBackendTests.swift index eff5f60..1ec8b53 100644 --- a/Packages/CrowTerminal/Tests/CrowTerminalTests/TmuxBackendTests.swift +++ b/Packages/CrowTerminal/Tests/CrowTerminalTests/TmuxBackendTests.swift @@ -548,6 +548,36 @@ struct TmuxBackendTests { } } +// MARK: - CROW-487 crowBinDir propagation + +/// `configure(...)` stores the per-devroot bin dir so `registerTerminal` can +/// inject `CROW_BIN_DIR` (consumed by the shell wrapper to win PATH precedence +/// after user rc sourcing — see crow-shell-wrapper.sh). Separate suite so it +/// runs even when tmux isn't installed — the integration suite is gated on +/// `discoveredTmuxBinary != nil`, but this check is pure state propagation. +@MainActor +@Suite("TmuxBackend crowBinDir") +struct TmuxBackendCrowBinDirTests { + @Test func configurePropagatesCrowBinDir() { + let backend = TmuxBackend() + backend.configure( + tmuxBinary: "/usr/bin/tmux", + socketPath: "/tmp/crow-487-probe.sock", + crowBinDir: "/devroot/.claude/bin" + ) + #expect(backend.crowBinDir == "/devroot/.claude/bin") + } + + @Test func configureDefaultsCrowBinDirToEmpty() { + let backend = TmuxBackend() + backend.configure( + tmuxBinary: "/usr/bin/tmux", + socketPath: "/tmp/crow-487-probe.sock" + ) + #expect(backend.crowBinDir == "") + } +} + /// Pure-policy tests for `shouldReconcile`. No tmux required, so the suite is /// always enabled (unlike the integration suite above). @Suite("TmuxBackend stale-config policy") diff --git a/Sources/Crow/App/AppDelegate.swift b/Sources/Crow/App/AppDelegate.swift index 8cfba50..4783a8a 100644 --- a/Sources/Crow/App/AppDelegate.swift +++ b/Sources/Crow/App/AppDelegate.swift @@ -306,7 +306,8 @@ final class AppDelegate: NSObject, NSApplicationDelegate { _ = try scaffolder.scaffold( workspaceNames: config.workspaces.map(\.name), managerAgentKind: config.agentKind(for: .manager), - corveilBinaryPath: nil + corveilBinaryPath: nil, + binaryOverrides: config.defaults.binaries ) // Save config @@ -394,7 +395,11 @@ final class AppDelegate: NSObject, NSApplicationDelegate { let tmpdir = ProcessInfo.processInfo.environment["TMPDIR"] ?? "/tmp" let socketPath = (tmpdir as NSString) .appendingPathComponent("crow-tmux.sock") - TmuxBackend.shared.configure(tmuxBinary: tmuxBinary, socketPath: socketPath) + TmuxBackend.shared.configure( + tmuxBinary: tmuxBinary, + socketPath: socketPath, + crowBinDir: (devRoot as NSString).appendingPathComponent(".claude/bin") + ) TmuxBackend.shared.onUnresponsive = { [weak self] error in Task { @MainActor in self?.handleTmuxUnresponsive(error: error) } } @@ -410,7 +415,8 @@ final class AppDelegate: NSObject, NSApplicationDelegate { let result = try scaffolder.scaffold( workspaceNames: config.workspaces.map(\.name), managerAgentKind: config.agentKind(for: .manager), - corveilBinaryPath: config.defaults.binaries["corveil"] + corveilBinaryPath: config.defaults.binaries["corveil"], + binaryOverrides: config.defaults.binaries ) appState.corveilSkillInstallWarning = result.warning } catch { @@ -1078,7 +1084,8 @@ final class AppDelegate: NSObject, NSApplicationDelegate { let result = try scaffolder.scaffold( workspaceNames: cfg?.workspaces.map(\.name) ?? [], managerAgentKind: cfg?.agentKind(for: .manager) ?? .claudeCode, - corveilBinaryPath: cfg?.defaults.binaries["corveil"] + corveilBinaryPath: cfg?.defaults.binaries["corveil"], + binaryOverrides: cfg?.defaults.binaries ?? [:] ) // Always assign — clears a stale warning from a prior // launch when the install now succeeds (`result.warning` diff --git a/Sources/Crow/App/Scaffolder.swift b/Sources/Crow/App/Scaffolder.swift index 7be35a2..fa4272a 100644 --- a/Sources/Crow/App/Scaffolder.swift +++ b/Sources/Crow/App/Scaffolder.swift @@ -25,10 +25,19 @@ struct Scaffolder { /// 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. + /// + /// `binaryOverrides` is the full `defaults.binaries` map. Every entry + /// whose target is executable becomes a symlink at + /// `{devRoot}/.claude/bin/` (CROW-487). Combined with the shell + /// wrapper's PATH prepend, that dir wins precedence for bare invocations + /// of `corveil` / `codex` / `cursor` inside spawned agent terminals, so + /// embedded skills (e.g. `/query-corveil`) resolve to the user-configured + /// binary instead of whatever happens to be on PATH. @discardableResult func scaffold(workspaceNames: [String], managerAgentKind: AgentKind = .claudeCode, - corveilBinaryPath: String? = nil) throws -> ScaffoldResult { + corveilBinaryPath: String? = nil, + binaryOverrides: [String: String] = [:]) throws -> ScaffoldResult { let fm = FileManager.default // Create devRoot @@ -132,6 +141,14 @@ struct Scaffolder { let promptsDir = (claudeDir as NSString).appendingPathComponent("prompts") try fm.createDirectory(atPath: promptsDir, withIntermediateDirectories: true) + // Per-devroot bin dir is the precedence anchor for bare-command + // invocations inside spawned agent terminals (CROW-487). Every + // configured `defaults.binaries.` becomes a symlink here, and + // the tmux shell wrapper prepends this dir to PATH after sourcing + // user rc — so `corveil`, `codex`, `cursor` resolve to the + // user-configured binary regardless of what's on PATH. + installBinarySymlinks(binaryOverrides, claudeDir: claudeDir) + // 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. @@ -139,6 +156,66 @@ struct Scaffolder { return ScaffoldResult(warning: warning) } + /// Materialize `{devRoot}/.claude/bin/` symlinks for every + /// `defaults.binaries.` whose target is an executable file + /// (CROW-487). Idempotent — re-run on every Scaffolder pass: + /// + /// - Reaps symlinks whose key was removed from config, so a stale entry + /// never shadows a working PATH install. Only removes entries that are + /// actually symlinks (we never own non-link files in this dir). + /// - Skips non-executable / empty targets, dropping any prior link for + /// that key. Prevents a misconfigured path from hiding `corveil` on + /// the user's PATH. + /// - Recreates good links with `removeItem` + `createSymbolicLink`, + /// matching `ln -sf` semantics. + /// + /// All errors are logged + swallowed; this step is best-effort and must + /// never fail an otherwise-successful scaffold pass. + private func installBinarySymlinks(_ overrides: [String: String], claudeDir: String) { + let fm = FileManager.default + let binDir = (claudeDir as NSString).appendingPathComponent("bin") + do { + try fm.createDirectory(atPath: binDir, withIntermediateDirectories: true) + } catch { + NSLog("[Scaffolder] could not create bin dir %@: %@", binDir, error.localizedDescription) + return + } + + // Reap stale symlinks whose key is no longer in config. Skip + // anything that isn't a symlink — we never want to nuke a real + // file that someone dropped here by hand. + let existing = (try? fm.contentsOfDirectory(atPath: binDir)) ?? [] + for name in existing where overrides[name] == nil { + let link = (binDir as NSString).appendingPathComponent(name) + if let attrs = try? fm.attributesOfItem(atPath: link), + (attrs[.type] as? FileAttributeType) == .typeSymbolicLink { + try? fm.removeItem(atPath: link) + } + } + + for (name, target) in overrides { + let trimmed = target.trimmingCharacters(in: .whitespacesAndNewlines) + let link = (binDir as NSString).appendingPathComponent(name) + guard !trimmed.isEmpty, fm.isExecutableFile(atPath: trimmed) else { + // Misconfigured: drop any stale link for this key so a + // broken pointer doesn't shadow a working PATH install. + try? fm.removeItem(atPath: link) + if !trimmed.isEmpty { + NSLog("[Scaffolder] defaults.binaries.%@ not executable at %@ — skipping symlink", + name, trimmed) + } + continue + } + try? fm.removeItem(atPath: link) + do { + try fm.createSymbolicLink(atPath: link, withDestinationPath: trimmed) + } catch { + NSLog("[Scaffolder] failed to symlink %@ -> %@: %@", + link, trimmed, error.localizedDescription) + } + } + } + /// 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 diff --git a/Tests/CrowTests/ScaffolderBinarySymlinkTests.swift b/Tests/CrowTests/ScaffolderBinarySymlinkTests.swift new file mode 100644 index 0000000..6387950 --- /dev/null +++ b/Tests/CrowTests/ScaffolderBinarySymlinkTests.swift @@ -0,0 +1,154 @@ +import CrowCore +import Foundation +import Testing +@testable import Crow + +/// Coverage for the per-devroot bin-dir symlink farm that `Scaffolder` builds +/// from `defaults.binaries` (CROW-487). Combined with the tmux shell wrapper's +/// post-rc PATH prepend, this farm gives configured binaries precedence over +/// whatever's on PATH inside spawned agent terminals. +/// +/// We exercise the public `scaffold(...)` entry point against a tmp devRoot +/// instead of the private helper — the loop runs late in scaffold and we want +/// the integration to be wired up, not just the helper in isolation. +@Suite("Scaffolder binary symlinks") +struct ScaffolderBinarySymlinkTests { + + private static func makeTempDevRoot() throws -> String { + let base = NSTemporaryDirectory() + let unique = "crow-487-\(UUID().uuidString)" + let path = (base as NSString).appendingPathComponent(unique) + try FileManager.default.createDirectory(atPath: path, withIntermediateDirectories: true) + return path + } + + /// Lay down a tiny executable shell script and return its absolute path. + /// Cheap stand-in for a real configured binary — the symlink loop only + /// checks `isExecutableFile`, not what the file actually does. + private static func makeExecutable(in dir: String, name: String) throws -> String { + let path = (dir as NSString).appendingPathComponent(name) + try "#!/bin/sh\necho \(name)\n".write(toFile: path, atomically: true, encoding: .utf8) + try FileManager.default.setAttributes([.posixPermissions: 0o755], ofItemAtPath: path) + return path + } + + @Test func executableTargetsBecomeSymlinks() throws { + let devRoot = try Self.makeTempDevRoot() + defer { try? FileManager.default.removeItem(atPath: devRoot) } + + let toolsDir = (devRoot as NSString).appendingPathComponent("_tools") + try FileManager.default.createDirectory(atPath: toolsDir, withIntermediateDirectories: true) + let corveilPath = try Self.makeExecutable(in: toolsDir, name: "corveil") + let codexPath = try Self.makeExecutable(in: toolsDir, name: "codex") + + _ = try Scaffolder(devRoot: devRoot).scaffold( + workspaceNames: [], + binaryOverrides: ["corveil": corveilPath, "codex": codexPath] + ) + + let binDir = (devRoot as NSString).appendingPathComponent(".claude/bin") + let corveilLink = (binDir as NSString).appendingPathComponent("corveil") + let codexLink = (binDir as NSString).appendingPathComponent("codex") + + #expect(FileManager.default.fileExists(atPath: corveilLink)) + #expect(FileManager.default.fileExists(atPath: codexLink)) + #expect(try FileManager.default.destinationOfSymbolicLink(atPath: corveilLink) == corveilPath) + #expect(try FileManager.default.destinationOfSymbolicLink(atPath: codexLink) == codexPath) + } + + @Test func nonExecutableTargetIsSkipped() throws { + let devRoot = try Self.makeTempDevRoot() + defer { try? FileManager.default.removeItem(atPath: devRoot) } + + // Existing file but not executable — should be skipped, no symlink. + let toolsDir = (devRoot as NSString).appendingPathComponent("_tools") + try FileManager.default.createDirectory(atPath: toolsDir, withIntermediateDirectories: true) + let nonExec = (toolsDir as NSString).appendingPathComponent("corveil") + try "not executable".write(toFile: nonExec, atomically: true, encoding: .utf8) + try FileManager.default.setAttributes([.posixPermissions: 0o644], ofItemAtPath: nonExec) + + _ = try Scaffolder(devRoot: devRoot).scaffold( + workspaceNames: [], + binaryOverrides: ["corveil": nonExec] + ) + + let corveilLink = (devRoot as NSString) + .appendingPathComponent(".claude/bin/corveil") + #expect(!FileManager.default.fileExists(atPath: corveilLink)) + } + + @Test func nonexistentTargetIsSkipped() throws { + let devRoot = try Self.makeTempDevRoot() + defer { try? FileManager.default.removeItem(atPath: devRoot) } + + _ = try Scaffolder(devRoot: devRoot).scaffold( + workspaceNames: [], + binaryOverrides: ["corveil": "/path/that/does/not/exist/crow487"] + ) + + let corveilLink = (devRoot as NSString) + .appendingPathComponent(".claude/bin/corveil") + #expect(!FileManager.default.fileExists(atPath: corveilLink)) + } + + @Test func staleSymlinkIsReapedWhenKeyRemoved() throws { + let devRoot = try Self.makeTempDevRoot() + defer { try? FileManager.default.removeItem(atPath: devRoot) } + + let toolsDir = (devRoot as NSString).appendingPathComponent("_tools") + try FileManager.default.createDirectory(atPath: toolsDir, withIntermediateDirectories: true) + let corveilPath = try Self.makeExecutable(in: toolsDir, name: "corveil") + + let scaffolder = Scaffolder(devRoot: devRoot) + // First pass: corveil is configured -> link created. + _ = try scaffolder.scaffold( + workspaceNames: [], + binaryOverrides: ["corveil": corveilPath] + ) + let corveilLink = (devRoot as NSString) + .appendingPathComponent(".claude/bin/corveil") + #expect(FileManager.default.fileExists(atPath: corveilLink)) + + // Second pass: user cleared the setting -> stale link reaped. + _ = try scaffolder.scaffold(workspaceNames: [], binaryOverrides: [:]) + #expect(!FileManager.default.fileExists(atPath: corveilLink)) + } + + @Test func nonSymlinkFilesInBinDirAreLeftAlone() throws { + // Defensive: if a user (or a future Crow feature) drops a real file + // into `.claude/bin`, the reaper must not nuke it. Only symlinks we + // own should ever disappear. + let devRoot = try Self.makeTempDevRoot() + defer { try? FileManager.default.removeItem(atPath: devRoot) } + + let binDir = (devRoot as NSString).appendingPathComponent(".claude/bin") + try FileManager.default.createDirectory(atPath: binDir, withIntermediateDirectories: true) + let strangerFile = (binDir as NSString).appendingPathComponent("stranger") + try "user dropped this".write(toFile: strangerFile, atomically: true, encoding: .utf8) + + _ = try Scaffolder(devRoot: devRoot).scaffold(workspaceNames: [], binaryOverrides: [:]) + + #expect(FileManager.default.fileExists(atPath: strangerFile)) + } + + @Test func reconfigurationRepointsExistingSymlink() throws { + // When the user picks a different binary path for the same key, the + // symlink should re-point — matches `ln -sf` semantics and prevents + // a stale link from quietly resolving to the prior path. + let devRoot = try Self.makeTempDevRoot() + defer { try? FileManager.default.removeItem(atPath: devRoot) } + + let toolsDir = (devRoot as NSString).appendingPathComponent("_tools") + try FileManager.default.createDirectory(atPath: toolsDir, withIntermediateDirectories: true) + let firstPath = try Self.makeExecutable(in: toolsDir, name: "corveil-a") + let secondPath = try Self.makeExecutable(in: toolsDir, name: "corveil-b") + + let scaffolder = Scaffolder(devRoot: devRoot) + _ = try scaffolder.scaffold(workspaceNames: [], binaryOverrides: ["corveil": firstPath]) + _ = try scaffolder.scaffold(workspaceNames: [], binaryOverrides: ["corveil": secondPath]) + + let corveilLink = (devRoot as NSString) + .appendingPathComponent(".claude/bin/corveil") + #expect(try FileManager.default.destinationOfSymbolicLink(atPath: corveilLink) == secondPath) + } +}