Add Cursor as a third agent under the CodingAgent protocol (#417)#418
Conversation
Lands Cursor as a third agent on the existing CodingAgent protocol from PR #197. Mirrors CrowCodex's package shape but the hook layer (writer + signal source) more closely mirrors CrowClaude — Cursor's hook engine is a superset of Claude Code's, not a no-op like Codex's per-session writer. * Add AgentKind.cursor * New package Packages/CrowCursor with CursorAgent, CursorHookConfigWriter, CursorSignalSource, CursorScaffolder, CursorLauncher * CursorHookConfigWriter collapses the camelCase ↔ PascalCase event-name mapping into the writer (Cursor camelCase JSON key → Crow-canonical PascalCase --event arg), letting CursorSignalSource share Claude/Codex's event vocabulary verbatim * CursorAgent.supportsRemoteControl = true (enables RemoteControlBadge and crow send — Cursor's stop.followup_message gives us auto-continue) * CursorScaffolder reuses Resources/AGENTS.md.template; co-existence with CodexScaffolder is idempotent (byte-identical writes, same Known Issues marker preservation) * AppDelegate registers CursorAgent gated on findBinary() and installs ~/.cursor/hooks.json (or $CURSOR_CONFIG_DIR) on launch when present * 25 new tests covering protocol members, autoLaunchCommand for work and review sessions, hook event mapping, idempotency, user-key preservation, scaffolder roundtrip, full StateSignalSource transition table No CLI changes needed — crow hook-event --agent and crow new-session --agent already accept arbitrary agent kinds. CreateSessionView auto-grows from AgentRegistry.shared.allAgents(), so the picker picks up the third entry for free. Closes #417 🐦⬛ Generated with Claude Code, orchestrated by Crow Co-Authored-By: Claude <noreply@anthropic.com> Crow-Session: 968C71B6-8487-41C2-A841-E982DC017A5F
dhilgaertner
left a comment
There was a problem hiding this comment.
Code & Security Review
Solid, well-scoped addition. CrowCursor mirrors CrowCodex/CrowClaude faithfully, the dynamic AgentRegistry keeps the picker auto-growing (no UI/CLI changes needed), and the 25 package tests pass locally (swift test --package-path Packages/CrowCursor). One should-fix robustness issue around the chosen launch token, plus a few minor notes.
Note: I could not run the full-app
swift build/swift test— the root build needsFrameworks/GhosttyKit.xcframework, which isn't present in this review checkout (error: local binary target 'GhosttyKit' ... does not contain a binary artifact). I verified theCrowCursorpackage in isolation instead and trust the PR's full-suite results.
Critical Issues
None.
Security Review
Strengths:
installGlobalConfigbuildshooks.jsonfrom a trusted resolvedcrowPath(ClaudeHookConfigWriter.findCrowBinary()) and hardcoded event names — no untrusted interpolation.CursorLaunchersingle-quote-escapes the worktree path and prompt-file path; the temp prompt file is written0o600.- No secrets, no injection surface, no auth/crypto concerns.
Concerns:
- None blocking.
Code Quality
Yellow — launchCommandToken = \"agent\" collides as a substring (CursorAgent.swift:17, consumed at AppDelegate.swift:1840 via command.contains(agent.launchCommandToken)).
prepareAgentLaunchText decides a command "launches the agent" by substring-matching the token. \"claude\"/\"codex\" rarely appear incidentally, but \"agent\" is a common English word. For a Cursor session, any crow send text containing \"agent\" (e.g. a prompt like "refactor the agent registry") is treated as an agent launch and flips terminalReadiness[…] = .agentLaunched.
Practical impact today is small (the OTEL prefix is Claude-only and Cursor's writeHookConfig is a no-op, so the forwarded text is never mutated), but list-terminals exposes readiness to CLI callers like setup.sh (AppDelegate.swift:1437-1448) specifically so they can "verify the agent actually started." A user who crow sends text containing \"agent\" before the bare-agent launch would report .agentLaunched falsely. Recommend anchoring the match (prefix/word-boundary) rather than a bare contains — small fix, removes the footgun the generic token introduces.
Green — supportsRemoteControl = true justification is aspirational (CursorAgent.swift:6-7,66). The comment attributes remote control to stop.followup_message in hooks.json, but generateHooks (CursorHookConfigWriter.swift:66) never writes a followup_message — only command hooks. Crow's actual remote control is crow send into the TUI (SessionService.swift:535, agent-agnostic), so the badge and behavior are correct; only the stated mechanism is inaccurate. Worth aligning the comment with reality (the PR body already flags the headless/empirical unknowns).
Green — scaffolder fallback isn't byte-identical to Codex (CursorScaffolder.swift:54-64 vs CodexScaffolder.swift:44-54). The PR claims byte-identical co-existence. That holds when Resources/AGENTS.md.template is found (the normal path — both reuse it). In the fallback path the two emit different headers ("Cursor Workspace Context" vs "Codex Workspace Context") to the same AGENTS.md. With both agents registered each launch has Codex write then Cursor overwrite — deterministic (Cursor wins, since its block runs after Codex's in AppDelegate), so no real fight, but the "byte-identical" framing only strictly holds when the bundled template exists.
Green — findBinary() path-list vs dependency-check PATH mismatch (CursorAgent.swift:26-48). Registration checks 3 hardcoded paths while the launch-dependency check uses ShellEnvironment.hasCommand(\"agent\") (PATH). A user with agent installed elsewhere won't get Cursor registered yet won't be warned. This mirrors OpenAICodexAgent exactly, so it's a pre-existing accepted pattern — noting only for parity.
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, 3 Green] findings. The Yellow (anchor the \"agent\" token match) is the only one that needs to land; the Greens are consider-only and fine to fold in or defer.
…fixes * Yellow: replace bare command.contains(token) in prepareAgentLaunchText with commandLaunchesToken — a shell-word-boundary check anchored at start-of-string or after `;`, `&&`, `||`, `|`. Cursor's `agent` token collides with English prose; this prevents `crow send "refactor the agent registry"` from falsely flipping terminalReadiness = .agentLaunched and contaminating list-terminals readiness reporting. * Green: fix CursorAgent doc comment — remove the aspirational claim that remote control rides on `stop.followup_message` in hooks.json. The hook writer only emits command hooks; actual remote control is `crow send` typing into the TUI (agent-agnostic, in SessionService). * Green: soften the CursorScaffolder co-existence claim — byte-identical only holds when the bundled template is present; the in-source fallback strings differ between Codex and Cursor. Document the fallback ordering (Cursor wins, runs after Codex in AppDelegate) so the behavior is predictable rather than implied. * Green deferred: findBinary path-list vs ShellEnvironment.hasCommand PATH mismatch is a pre-existing accepted pattern (mirrors OpenAICodexAgent exactly per the reviewer's own note); deferred for cross-agent consistency. Adds 13 new tests in Tests/CrowTests/CommandLaunchesTokenTests.swift covering the Cursor footgun (prose like "agent registry", "--agent flag"), all known auto-launch shapes (bare, env-prefixed, cd-prefixed for each of the three agents), and edge cases (empty command, token at end-of-string, semicolon separator). Test run: 165 root + 25 CrowCursor + 27 CrowCodex + 37 CrowClaude + 205 CrowCore — all green. 🐦⬛ Generated with Claude Code, orchestrated by Crow Co-Authored-By: Claude <noreply@anthropic.com> Crow-Session: 968C71B6-8487-41C2-A841-E982DC017A5F
|
Thanks for the review. Pushed cca3697 addressing the Yellow plus the two actionable Greens: Yellow — anchored token match. Replaced Green #1 — Green #2 — scaffolder co-existence framing. Softened the doc comment on Green #3 — Re-requesting review. 🐦⬛ Replied with Claude Code via Crow |
dhilgaertner
left a comment
There was a problem hiding this comment.
Code & Security Review
Solid, well-documented package that faithfully mirrors the CrowCodex shape under the CodingAgent protocol. Tests are thorough (25 in CrowCursor, plus the new CommandLaunchesTokenTests), the scaffolder co-existence reasoning is sound, and the camelCase↔PascalCase event collapse into the writer is a clean way to let CursorSignalSource reuse the canonical vocabulary. The commandLaunchesToken anchoring is genuinely needed (crow send prose flows through this gate via the crow send RPC at AppDelegate.swift:1522).
But that same anchoring change regresses the Claude launch path, which is why this is request-changes.
Critical Issues
🔴 commandLaunchesToken breaks hook-config + OTEL injection for Claude managed sessions (regression).
AppDelegate.swift:1840 swapped command.contains(token) for the new anchored commandLaunchesToken. The left boundary only accepts start-of-string or a shell separator [;&|] — it does not accept a path separator /. But by the time a Claude command reaches prepareAgentLaunchText on the standard deferred-launch path (#408), the bare claude token has already been rewritten to an absolute path by resolveClaudeInCommand (AppDelegate.swift:1331, called in new-terminal), e.g. /opt/homebrew/bin/claude --rc …. The resolved command is stored in pendingLaunchCommands (AppDelegate.swift:1379) and later handed to prepareAgentLaunchText by SessionService.pasteDeferredLaunch (SessionService.swift:409).
Empirically:
commandLaunchesToken("/opt/homebrew/bin/claude --rc …", "claude") -> false # was true with .contains
commandLaunchesToken("claude --rc", "claude") -> true
claude in /…/bin/claude is preceded by /, which matches neither ^ nor [;&|], so the guard returns (command, false) early — skipping agent.hookConfigWriter.writeHookConfig(...) (the per-worktree .claude/settings.local.json) and the OTEL telemetry env injection at AppDelegate.swift:1843 / 1855-1864.
Impact for a brand-new managed Claude work session (the common path — pasteDeferredLaunch is where new managed terminals paste per #408): hooks are not written for the fresh worktree, so Claude's hook events don't route back to the session and the sidebar activity-state machine (idle/working/done) breaks; OTEL telemetry stops being exported. The other two writeHookConfig call sites (SessionService.swift:279 adopt, :492 launchAgent) only cover restart/recovery, not first launch, and no write happens at worktree creation — so a fresh session has no fallback. The regression only manifests when claude is actually installed at a standard candidate path (SessionService.swift:1819), i.e. the real-user case; if it's not found, resolveClaudeInCommand leaves the token bare and the regex matches — which is likely why the existing suite didn't catch it. (Codex/Cursor are unaffected: their tokens aren't path-rewritten, and their writers are no-ops with no OTEL.)
Suggested fix: add the path separator to the left boundary so a path-prefixed binary still matches while space-preceded prose stays rejected:
let pattern = "(?:^|[;&|]\\s*|/)\(escaped)(?=\\s|$|[\"'])""refactor the agent registry" (space-preceded) still correctly returns false. Please add a CommandLaunchesTokenTests case for the absolute-path shape (/opt/homebrew/bin/claude --rc, /usr/local/bin/agent) to lock this in.
Security Review
Strengths:
CursorLauncher.launchCommandwrites the prompt temp file0o600and shell-escapes both the worktree path and prompt path via single-quote wrapping; the$(cat …)substitution is double-quoted, so no word-splitting/injection (CursorLauncher.swift:350-362).installGlobalConfigmerges rather than clobbers — user-authored hook entries for events outsideeventMappingare preserved (CursorHookConfigWriter.swift:256-278); covered byinstallGlobalConfigPreservesUserEntries.- Hook commands carry no session-derived interpolation —
cwd-based resolution server-side, no--sessioninjection surface.
Concerns: none beyond the critical issue above.
Code Quality
- 🟢
AppDelegate.swift:1096addsagentto the runtime-dependency probe list. Sinceagentis a generic name, every machine without Cursor now surfaces it in the missing-tools warning — consistent with howcodex/glab/codeare already listed, so acceptable, but worth a glance. - 🟢
CursorScaffolderfallback string ("Cursor Workspace Context") differs from Codex's, so when the bundled template is absent the two scaffolders rewriteAGENTS.mdon each launch (Codex then Cursor). Idempotent and the user## Known Issues / Correctionssection is preserved, so harmless — already acknowledged in the doc comment. - 🟢
CursorLauncher/launchCommandare not yet wired into the auto-launch path (MVP launches bareagent); fine as documented, just dead-until-follow-up.
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, 0 Yellow, 3 Green] findings. The Cursor package itself is in good shape; the blocker is the collateral regression the shared commandLaunchesToken change introduces on the Claude path.
Note: full swift build couldn't run here (the vendored GhosttyKit.xcframework binary artifact is absent in this checkout); swift test in Packages/CrowCursor passes all 25 tests.
cca3697's anchored regex rejected /opt/homebrew/bin/claude because `/` wasn't a recognized left boundary. resolveClaudeInCommand rewrites the bare `claude` token to an absolute path before pasteDeferredLaunch hands the command to prepareAgentLaunchText (the standard #408 path for brand-new managed Claude sessions), so the guard returned (command, false) early and silently skipped both writeHookConfig (per-worktree .claude/settings.local.json) and the OTEL env injection. Hooks didn't route back to the session, sidebar idle/working/done state broke, and telemetry stopped exporting. Adds `/` to the left-boundary alternation: (?:^|[;&|]\s*|/)<token>(?=\s|$|["']) The right boundary stays at \s|$|["'], so incidental path substrings like /tmp/agent_log still reject (token followed by `_`, not a boundary char). Codex and Cursor weren't impacted — their tokens aren't path- resolved, and their writers are no-ops with no OTEL — which is why the existing suite didn't catch it. Adds 5 tests covering: - /opt/homebrew/bin/claude --rc --name 'foo' - /usr/local/bin/claude --rc - /opt/homebrew/bin/agent (Cursor) - env-prefixed path-resolved Claude (the full pasteDeferredLaunch shape) - /tmp/agent_log negative case (confirms `/` doesn't open the door to incidental path substrings) 🐦⬛ Generated with Claude Code, orchestrated by Crow Co-Authored-By: Claude <noreply@anthropic.com> Crow-Session: 968C71B6-8487-41C2-A841-E982DC017A5F
|
Good catch — the anchored regex broke Claude's deferred-launch path. Fixed in 759bb4d. The bug. The fix. Took your suggested boundary verbatim: let pattern = "(?:^|[;&|]\\s*|/)\(escaped)(?=\\s|$|[\"'])"The right boundary stays at
All 170 root tests + 25 CrowCursor tests still green. Rebased onto the auto-merge from main (d370968) before pushing. Re-requesting review. 🐦⬛ Replied with Claude Code via Crow |
dhilgaertner
left a comment
There was a problem hiding this comment.
Code & Security Review
Lands Cursor as a third CodingAgent conformer. The change is tightly scoped, faithfully mirrors the established Codex/Claude patterns, and is well-tested. Verified locally: CrowCursor package builds and all 25 package tests pass; the new commandLaunchesToken regex validated independently against all 18 documented cases (18/18 pass), including the path-resolved-binary regression fixed in 759bb4d. (The full-app swift build couldn't run here due to a missing local GhosttyKit.xcframework artifact — an environment issue, not a code problem.)
Critical Issues
None.
Security Review
Strengths:
CursorLauncher.launchCommandshell-escapes both the worktree path and the prompt-file path via single-quote wrapping (shellEscape), and writes the prompt temp file0o600. No injection surface from session data.installGlobalConfigbuildshooks.jsonviaJSONSerialization(no string concatenation of untrusted data into JSON) and merges conservatively — user-authored entries for events outsideeventMappingsurvive, matchingCodexHookConfigWriterexactly.- No secrets handled; Cursor reads
CURSOR_API_KEYfrom the inherited shell, nothing logged.
Concerns:
- None blocking.
Code Quality
commandLaunchesTokenis the one piece of non-trivial new logic and it's the right fix: anchoringagentat start / shell-separator //boundary prevents the.contains("agent")footgun oncrow sendprose. Anchor + the regression-driven/left boundary are both covered by tests.AppDelegateCursor block (registration + scaffold + global-config install) is a line-for-line mirror of the Codex block, gated onAgentRegistry.shared.agent(for: .cursor)and ordered deterministically after Codex's scaffold so the sharedAGENTS.mdwrite is idempotent.- Per-session
HookConfigWritermethods are documented no-ops (Cursor is global-only) — consistent with Codex.
Green (consider — non-blocking)
- Residual
agentfalse-positive surface. The anchor still matchesagentafter/or a shell separator, socrow sendprose liketail /var/agentorbuild | agentto a Cursor session could flipterminalReadiness = .agentLaunched. Impact is minimal (Cursor'swriteHookConfigis a no-op and no text mutation occurs for non-Claude agents — only a wrong readiness flag), and the/boundary is genuinely required for path-resolved binaries. Acceptable tradeoff; flagging only for awareness. - Binary-detection mechanisms differ.
findBinary()checks 3 hardcoded paths while the launch-time dependency check uses PATH (hasCommand("agent")), so the two can disagree. Pre-existing pattern shared with Codex — not introduced here. .jobsessions return nil fromautoLaunchCommand(work-only guard). Identical to Codex; the inline comment only mentions review sessions, so a one-word mention of job sessions would round out the doc. Cosmetic.- Empirical unknowns (headless
agent -pfiring~/.cursor/hooks.json, real exit codes) are explicitly deferred per the ticket, withafterAgentResponse → Notificationas a documented safety net.
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, 4 Green] findings. Clean, well-tested, and faithful to the existing multi-agent architecture.
Summary
Lands Cursor as a third agent on the
CodingAgentprotocol established by PR #197. MirrorsPackages/CrowCodex/'s package shape, but theHookConfigWriter/StateSignalSourceimplementations are closer toPackages/CrowClaude/'s — Cursor's hook engine is a superset of Claude Code's, not a no-op like Codex's per-session writer.Packages/CrowCursor/withCursorAgent,CursorHookConfigWriter,CursorSignalSource,CursorScaffolder,CursorLauncherAgentKind.cursoradded toCrowCoreAppDelegateregistersCursorAgentgated onfindBinary()and installs~/.cursor/hooks.json($CURSOR_CONFIG_DIRaware) on launchpreToolUsekey →--event PreToolUsearg), lettingCursorSignalSourceshare Claude/Codex's canonical vocabulary verbatimsupportsRemoteControl = true— Cursor'sstop.followup_messageenables auto-continue and theRemoteControlBadgeCursorScaffolderreusesResources/AGENTS.md.template; co-existence withCodexScaffolderis idempotent (byte-identical writes, same## Known Issues / Correctionsmarker preservation)No CLI changes —
crow hook-event --agentandcrow new-session --agentalready accept arbitrary agent kinds from #197. No UI changes —CreateSessionViewauto-grows fromAgentRegistry.shared.allAgents().Things deferred to follow-ups (called out in the ticket as "empirical")
No `crow cursor-notify` subcommand — Cursor's hook engine fires `crow hook-event --agent cursor` directly. The Codex `notify` analog was a Tier-2 fallback for Codex's missing hook-engine pieces; Cursor doesn't need it.
Test plan
Closes #417
🤖 Generated with Claude Code