Walk PATH (and honor defaults.binaries) for agent binary discovery

[dgershman]

Closes #484

Summary

• OpenAICodexAgent.findBinary() and CursorAgent.findBinary() only checked three hardcoded install paths and returned nil for npm-global / nvm / Volta / pnpm / asdf installs, silently hiding installed agents from the per-session picker.
• Refactors discovery into a default CodingAgent.findBinary() impl: explicit defaults.binaries.<kind> override → PATH walk (ShellEnvironment.findExecutable, same way command -v resolves) → hardcoded fallbackCandidates.
• Each agent now just declares its fallbackCandidates; the discovery logic lives in one place.
• AppDelegate.launchMainApp loads config first, populates BinaryOverrides.shared from defaults.binaries, then registers agents and logs the resolved path so users can verify which install was picked up.

Acceptance

• A system with codex installed via any of these methods registers and shows in the picker:
  • brew install-style at /opt/homebrew/bin/codex (existing path, still covered via fallback)
  • npm i -g @openai/codex under standard npm prefix (resolved via PATH)
  • npm-global under nvm ~/.nvm/versions/node/*/bin/codex (resolved via PATH)
  • Volta, pnpm, bun, asdf/mise (resolved via PATH)
  • ~/.local/bin/codex (existing path, still covered via fallback)
• Same for cursor (agent binary).
• Setting defaults.binaries.codex to an explicit path causes Crow to use it regardless of discovery (verified by findBinaryHonorsBinaryOverride test).
• A system with codex not installed continues to silently skip registration (no regression — findBinary() returns nil when override + PATH + fallback all miss).
• NSLog now prints "OpenAI Codex agent registered at <resolved path>" so users can verify which install was picked up.
• Manual workaround documented in the ticket (symlink to ~/.local/bin/codex) stops being necessary.

Note: the Corveil binary picker referenced in #482 has not landed on this branch yet. This PR introduces the defaults.binaries schema field; when #482 merges it just adds a corveil row to the same map.

Test plan

• swift test --arch arm64 in each of the package roots (CrowCore, CrowCodex, CrowCursor, CrowClaude, CrowPersistence) and at the repo root — all green.
• New tests: BinaryOverrides (4 cases), ShellEnvironment.findExecutable (3 cases), ConfigDefaults.binaries decode (2 cases), OpenAICodexAgent.findBinary honors/ignores stale override (2 cases).
• Manual smoke on a machine with codex under nvm only — verify the new "registered at <path>" log line and picker visibility.

🤖 Generated with Claude Code

[dhilgaertner]

Code & Security Review

Solid, well-scoped refactor: collapsing three near-identical findBinary() impls into one default on CodingAgent (override → PATH walk → fallback) is the right shape, the kind raw-values line up with the JSON keys, the stale-override fall-through is handled, and the config field decodes forward-compatibly. Verified launchCommandToken (codex/agent/claude) and kind (codex/cursor/claude-code) both resolve correctly, and CrowCore builds clean. One blocking test defect and one behavioral concern.

Critical Issues

🔴 Red — BinaryOverrides test suite is racy and fails reproducibly under parallel execution
Packages/CrowCore/Tests/CrowCoreTests/BinaryOverridesTests.swift

All four tests mutate the process-global BinaryOverrides.shared singleton, and Swift Testing runs tests in parallel within a suite by default. The per-test defer { set([:]) } resets don't isolate anything — a concurrent test's set(...) clobbers another's state mid-assertion. The PR's "all green" claim doesn't hold: running swift test --filter BinaryOverrides 6× here failed 5 times, e.g.:

✘ emptyByDefault() — (path(for: .cursor) → "/tmp/agent") == nil
✘ setStoresRawKeyedByAgentKind() — (path(for: .cursor) → nil) == "/tmp/agent"
✘ setReplacesPreviousMap() — (path(for: .cursor) → nil) == "/tmp/agent"

This will flake CI. Fix: serialize the suite — @Suite("BinaryOverrides", .serialized) — and likewise add .serialized to @Suite("OpenAICodexAgent") in Packages/CrowCodex/Tests/CrowCodexTests/OpenAICodexAgentTests.swift, since its two new findBinary* tests mutate the same global (that suite passed 6/6 here only by timing luck — findBinaryHonorsBinaryOverride vs findBinaryIgnoresOverrideWhenPathMissing race on codex). .serialized is sufficient because only these two suites touch the global, and they live in separate targets.

Code Quality

🟡 Yellow — PATH walk for Cursor's generic agent binary name risks false-positive registration
Packages/CrowCore/Sources/CrowCore/Agent/CodingAgent.swift:129 + CursorAgent.launchCommandToken = "agent"

The old impl checked only three explicit .../agent paths. The new PATH walk registers Cursor for any executable named agent anywhere on the user's $PATH. agent is a notably generic name — CI build agents (Azure DevOps, TeamCity), etc., ship a binary literally called agent. On such a machine Crow would now mis-register Cursor and later launch the wrong binary. codex/claude are specific enough not to collide; agent is the outlier. Worth at minimum a code comment acknowledging the tradeoff, and ideally a sanity check (or documenting defaults.binaries.cursor as the escape hatch in the user-facing config docs).

Security Review

Strengths:

• No injection surface added — findExecutable does filesystem isExecutableFile checks, not shell evaluation, and POSIX empty-PATH entries (which would mean CWD) are dropped by split(separator:), avoiding accidental CWD execution.
• BinaryOverrides is an opt-in absolute-path override from the user's own config; stale/missing paths fall through safely rather than breaking registration.

Concerns: None beyond the Yellow above.

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 [1 Red, 1 Yellow, 0 Green] findings.

---

🐦‍⬛ Reviewed by Crow via Claude Code

[dhilgaertner]

Code & Security Review

Refactors per-agent binary discovery into a single default CodingAgent.findBinary() impl: explicit defaults.binaries.<kind> override → PATH walk (ShellEnvironment.findExecutable) → hardcoded fallbackCandidates. This correctly fixes the CROW-484 gap where npm-global / nvm / Volta / pnpm / asdf installs were invisible. Verified by building and running the full test suites for CrowCore, CrowCodex, CrowCursor, and CrowClaude — all green (269 + 29 + 29 + 41 tests).

Critical Issues

None.

Security Review

Strengths:

• No new attack surface: discovery is read-only (isExecutableFile), no shell-out, no command construction from user input.
• Stale-override handling is correct and defensive — findBinary() re-verifies isExecutableFile(atPath:) on the configured path before returning it (CodingAgent.swift:124), so a moved/uninstalled binary falls through to PATH/fallback rather than pinning a dead path or breaking registration. Covered by findBinaryIgnoresOverrideWhenPathMissing.
• BinaryOverrides singleton is properly guarded by NSLock; test suites that mutate the shared state are .serialized to avoid cross-suite leakage.

Concerns:

• None blocking. The Cursor agent PATH-collision risk (a CI runner's agent binary could resolve as Cursor's CLI on a build machine) is real but is thoroughly documented at CursorAgent.swift and fully mitigated by the defaults.binaries.cursor escape hatch. Acceptable tradeoff for a desktop workstation app.

Code Quality

• Clean abstraction: the discovery logic now lives in one place and each agent just declares fallbackCandidates; hasCommand is correctly re-expressed as findExecutable(_:) != nil so the two stay in sync (pinned by hasCommandAgreesWithFindExecutable).
• ConfigDefaults.binaries decoding is forward/backward-compatible — a missing key decodes to [:] (configDefaultsBinariesDefaultsEmpty), and the field round-trips through the synthesized encoder.
• AppDelegate.launchMainApp config-load reorder is safe: config is loaded once up front, BinaryOverrides.shared is populated before the registration gates run, and the later duplicate load was correctly removed. The new registered at <path> NSLog aids field debugging.
• Good test coverage across all three layers (override map, PATH resolver, config decode, agent-level honor/ignore).

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. The single Green item (Cursor agent name collision) is already documented and mitigated in the PR.

---

🐦‍⬛ Reviewed by Crow via Claude Code