Make defaults.binaries.* take precedence in spawned terminals (CROW-487)#489
Merged
Merged
Conversation
…d them first (CROW-487)
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.<name> entry
at {devRoot}/.claude/bin/<name>. 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.
dhilgaertner
approved these changes
Jun 11, 2026
dhilgaertner
left a comment
Contributor
There was a problem hiding this comment.
Code & Security Review
Critical Issues
None.
Security Review
Strengths:
- Symlink reaper uses
attributesOfItem(atPath:)(does not traverse the final link), so only Crow-owned symlinks are removed — a hand-dropped real file in.claude/binis preserved (covered bynonSymlinkFilesInBinDirAreLeftAlone). - Misconfigured / non-executable / nonexistent targets are skipped rather than turned into broken links, so a bad config can never shadow a working PATH install — it just falls through to PATH. This is the right failure mode.
- No secrets, no injection surface: targets are validated with
isExecutableFileand linked verbatim;CROW_BIN_DIRis exported through tmux-e, not interpolated into a shell string.
Code Quality
- Precedence is layered correctly: Swift seeds
PATH=crowBinDir:resolvedPATHviatmux new-window -e(covers fish / non-rc shells and wrapper-bypassing processes), and the wrapper re-prepends$CROW_BIN_DIRafter sourcing user rc — the only insertion point that survives a userexport PATH=…in.zshrc. The front-of-PATHcaseguard prevents unbounded growth on shell re-exec. - Idempotent:
removeItem+createSymbolicLinkgivesln -sfsemantics; reconfiguration re-points and stale keys are reaped (both tested). - Best-effort error handling (log + swallow) honors the documented contract that this step never fails an otherwise-successful scaffold. All three
scaffold(...)call sites passbinaryOverridesconsistently; the[:]default keeps the new parameter safe. - Strong test coverage: 6 Scaffolder symlink tests (executable / non-executable / nonexistent / stale-reap / non-link-preservation / re-point) + 2 TmuxBackend propagation tests.
Green (consider only — non-blocking):
isExecutableFileresolves relative to Crow's CWD whilecreateSymbolicLink(withDestinationPath:)makes the link relative to the bin dir. A relativedefaults.binariesvalue could in theory pass the check yet link to a broken target. In practice values come from the CROW-482 absolute-path picker, and a relative path would almost certainly failisExecutableFileand be skipped — so no broken link materializes.- A configured key colliding with a pre-existing real (non-symlink) file in
.claude/binwill replace that file (the no-clobber guarantee only applies to the reap path). Harmless in practice for a Crow-managed dir.
Note
swift build/swift test could not be run in this review checkout — Frameworks/GhosttyKit.xcframework ships no binary artifact here (environment limitation, not a PR defect). Review is based on full static reading; the PR reports both suites pass locally.
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, 2 Green] findings.
This was referenced Jun 11, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #487
Summary
${devRoot}/.claude/bin/<name>from everydefaults.binariesentry whose target is executable. Loop is idempotent, reaps stale symlinks when keys are removed, and skips non-executable / nonexistent targets so a misconfigured path never shadows a working PATH install.configure(crowBinDir:)threads the bin dir through;registerTerminalexportsCROW_BIN_DIRand seeds the spawned window'sPATHwith the bin dir in front viatmux new-window -e.$CROW_BIN_DIRtoPATHafter sourcing the user's.zshrc/.bashrcso a userexport PATH=…can't shadow the symlink farm. Fish / unknown-shell branches inherit the already-seededPATHfrom the wrapper's outer scope.Effect
Embedded skills like
/query-corveil(shipped inside thecorveilbinary itself) keep using barecorveilinvocations — Crow makes those resolve to the user-configured binary without touching the skill content. Generic in the map:codex,cursor, and any future tools share the same mechanism via #484's existingdefaults.binariesschema.Test plan
swift test --filter "ScaffolderBinarySymlinkTests"— 6 tests covering executable targets, non-executable skip, nonexistent skip, stale reaping, non-symlink files left alone, reconfiguration re-points.swift test --filter "TmuxBackendCrowBinDirTests"—configure(crowBinDir:)propagation + default empty.defaults.binaries.corveilin Settings to an executable; open a Claude Code session; runwhich corveil→ reports${devRoot}/.claude/bin/corveil;corveil --versionmatches the configured binary;/query-corveilinvokes that binary.defaults.binaries.corveil; relaunch; symlink removed; barecorveilfalls back to PATH (or unresolved if not installed).defaults.binaries.corveilto a nonexistent path; relaunch; one-line warning in[Scaffolder]log; no broken symlink created.defaults.binaries.codex/defaults.binaries.cursor; both symlinks materialize.Related
defaults.binaries.corveil.defaults.binariesoverride for agent binary discovery; this PR reuses its schema ([String: String]map).🐦⬛ Generated with Crow via Claude Code