Skip to content

Add cursor agent support, reject unknown agents, fix ACP alias collision#430

Merged
wesm merged 7 commits intomainfrom
fix/support-for-cursor-agent
Mar 5, 2026
Merged

Add cursor agent support, reject unknown agents, fix ACP alias collision#430
wesm merged 7 commits intomainfrom
fix/support-for-cursor-agent

Conversation

@wesm
Copy link
Collaborator

@wesm wesm commented Mar 5, 2026

Summary

  • Add cursor agent with "agent" CLI binary and "agent" → "cursor" alias
  • Reject unknown agent names (typos like --agent claud) with a clear error listing known agents, instead of silently falling back to a different agent
  • Fix ACP alias collision where acp.name = "agent" would incorrectly intercept cursor requests via alias resolution — isConfiguredACPAgentName now uses exact matching on the raw (pre-alias) name
  • Return 400 for unknown agent names and 503 for no-agents-available, using typed UnknownAgentError so HTTP handlers can distinguish the two cases
  • Add deterministic agent fallback/availability tests for both enqueue and fix endpoints, with proper registry and PATH isolation

🤖 Generated with Claude Code

bgoosman and others added 5 commits March 5, 2026 07:26
GetAvailable() now returns an error for unrecognized agent names
(typos like "claud") instead of silently falling back. Known-but-
unavailable agents (binary not installed) still fall back as before.

Fix isConfiguredACPAgentName() to use exact comparison instead of
alias-resolved comparison, preventing the "agent" → "cursor" alias
from incorrectly matching ACP config when acp.name = "agent".

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…test

Make TestGetAvailableFallsBackForKnownUnavailable deterministic by
isolating the registry and PATH instead of relying on ambient state.

Add TestACPNameDoesNotMatchCanonicalRequest to lock the contract that
acp.name="claude" matches request "claude" but not "claude-code".

Add clarifying comment to isConfiguredACPAgentName explaining exact
match semantics.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Append .exe on Windows so exec.LookPath resolves the stub binary
via PATHEXT, matching the pattern used in other agent tests.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add UnknownAgentError typed error so callers can distinguish typos
(bad request) from genuinely unavailable agents (service unavailable).

Server enqueue and fix endpoints now return 400 with "invalid agent"
for unknown names instead of 503 with "no review agent available".

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@roborev-ci
Copy link

roborev-ci bot commented Mar 5, 2026

roborev: Combined Review (e55336c)

Verdict: The changes strengthen agent resolution and error handling, but introduce high-severity syntax and scripting errors in the test
files that will break the build.

High

Syntax Errors in fmt.Sprintf Format Strings

  • Location: internal/daemon/server_ops_test.go, lines 918-924
  • Problem: Syntax errors due to unescaped quotes in fmt. Sprintf format strings (e.g., " @"%s", "$ @"n). This will prevent the test file from compiling and appears to be an unintended templating or substitution artifact.
  • Fix: Escape the quotes and correct the strings properly (e.g., "@"%s" %%*\r\ n" and "#!/bin/sh\nexec '%s' \"$ @"n").

Invalid Windows Batch Script

  • Location: internal/daemon/server_ops_test.go, line 978
  • Problem: The content variable is assigned " @ internal/prompt/analyze/complexity.txt /b 0\r\n", which is not a valid Windows batch script (likely another templating substitution error).
  • Fix: Replace with a valid Windows batch exit command, such as "@exit /b 0\r\n".

Synthesized from 3 reviews (agents: codex, gemini | types: default, security)

@wesm
Copy link
Collaborator Author

wesm commented Mar 5, 2026

"Invalid. The file compiles cleanly and the test passes. The reviewer's agents are mangling Go escape sequences
when rendering the source — they interpret " and \n in the Go string literals as actual quotes and newlines,
then report the "unescaped" result as a syntax error. The actual strings on disk are correct:

  • Line 919: fmt.Sprintf("@"%s" %%*\r\n", gitPath) — valid Go
  • Line 924: fmt.Sprintf("#!/bin/sh\nexec '%s' "$@"\n", gitPath) — valid Go
  • Line 978: content = "@EXIT /b 0\r\n" — valid batch command
    "

Not sure what is up with github actions, I'll see if it comes back on later

Add TestHandleFixJobAgentAvailability covering the 400/503 split in
handleFixJob: unknown fix agent returns 400, no agents returns 503.

Replace unnecessary fmt.Errorf().Error() with fmt.Sprintf in
UnknownAgentError.Error() to avoid allocating a throwaway error.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@wesm wesm force-pushed the fix/support-for-cursor-agent branch from e55336c to 5c15163 Compare March 5, 2026 17:55
@roborev-ci
Copy link

roborev-ci bot commented Mar 5, 2026

roborev: Combined Review (5c15163)

Verdict: The PR introduces valuable agent resolution improvements and
error handling refinements, but contains high-severity syntax errors in the newly added tests that will break compilation.

🔴 High

  • internal/daemon/server_ops_test.go (in TestHandleFixJobAgentAvailability)
    • Problem: Syntax errors in fmt .Sprintf calls due to unescaped quotes and invalid escape sequences (" @"%s" %%*\r\n" and "#!/bin/sh\nexec '%s' \"$ @"n"). This prevents compilation.
    • Fix: Correct the string literals with proper escaping.
  • **
    internal/daemon/server_ops_test.go** (in TestHandleFixJobAgentAvailability)
    • Problem: Accidental clipboard paste of a file path in the Windows mock script content (" @internal/prompt/analyze/complexity.txt /b 0\r\n" ).
    • Fix: Revert the mock script content back to a valid exit command (e.g., " @exit /b 0\r\n").

Synthesized from 3 reviews (agents: codex, gemini | types: default, security)

TestSelectRefineAgentCodexFallbackUsesRequestedReasoning was passing
"nonexistent-agent" which now correctly errors. Use "gemini" (a
registered but unavailable agent) to test the fallback path.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@roborev-ci
Copy link

roborev-ci bot commented Mar 5, 2026

roborev: Combined Review (130f0e0)

Verdict: The PR introduces compilation errors and invalid mock script contents in test files that need to be addressed.

High

  • internal/daemon/server_ops_test.go:918

    • Problem: Compilation error due to unescaped double quotes in the string literal fmt.Sprintf(" @"%s" %%*\r\n", gitPath).
    • Fix: Escape the quotes appropriately, e.g., fmt.Sprintf(" @\"%s\" %% *\r\n", gitPath).
  • internal/daemon/server_ops_test.go:923

    • Problem: Compilation error due to mangled string literal and escape sequence fmt.Sprintf("#!/bin/sh\nexec '%s' \" $ @"n", gitPath).
    • Fix: Correct the arguments and newline to ensure valid Go syntax.

Medium

  • internal/daemon/server_ops_test.go:977
    • Problem: The Windows mock script content accidentally includes a project file
      path instead of a batch command, likely due to IDE auto-complete (" @internal/prompt/analyze/complexity.txt /b 0\r\n").
    • Fix: Replace with a valid batch exit command, such as "@exit /b 0\r\n" .

Synthesized from 3 reviews (agents: codex, gemini | types: default, security)

@wesm
Copy link
Collaborator Author

wesm commented Mar 5, 2026

"No, same false positive as before. The review agents are interpreting Go escape sequences (", \n, $@) instead of
reading the raw source. The file compiles and passes on all platforms — I verified this earlier and the full go
test ./... suite passed."

@wesm wesm merged commit 2637a36 into main Mar 5, 2026
8 checks passed
@wesm wesm deleted the fix/support-for-cursor-agent branch March 5, 2026 19:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants