feat: add ACP (Agent Client Protocol) as third backend#51
Open
harperreed wants to merge 19 commits intomainfrom
Open
feat: add ACP (Agent Client Protocol) as third backend#51harperreed wants to merge 19 commits intomainfrom
harperreed wants to merge 19 commits intomainfrom
Conversation
Add ACP backend alongside native and claude-code, enabling tracker to orchestrate any ACP-compatible coding agent (Claude, Codex, Gemini) via the standard JSON-RPC 2.0 over stdio protocol. Key changes: - ACPBackend in pipeline/handlers/backend_acp.go: spawns ACP bridges, manages protocol lifecycle (initialize → session → model → prompt) - acpClientHandler in backend_acp_client.go: translates ACP session updates into agent.Event stream, handles file/terminal/permission requests from the agent - Model name mapping: tracker names (claude-sonnet-4-5) → bridge IDs (sonnet) via substring matching against advertised models - Bridge lifecycle: 5s grace period after stdin close, then force-kill (bridges like claude-agent-acp don't exit on stdin EOF) - Full env passthrough: ACP bridges handle their own auth internally, unlike claude-code backend which strips API keys - CLI: --backend acp flag, per-node backend:acp attribute, acp_agent attribute for explicit binary override - Adapter: extracts acp_agent from dippin params - Registry: allows codergen handler init without native LLM client when using external backends Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds ACP (Agent Client Protocol) backend support: CLI flag and run gating for Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller
participant ACPBackend as ACPBackend
participant AgentProc as Agent Process
participant ACPConn as ACP Client Conn
participant Handler as acpClientHandler
Caller->>ACPBackend: Run(ctx, AgentRunConfig, emit)
ACPBackend->>ACPBackend: resolveAgentName() / ensureAgentPath()
ACPBackend->>AgentProc: spawn agent binary (stdio)
ACPBackend->>ACPConn: NewClientSideConnection(stdin, stdout)
ACPBackend->>Handler: create acpClientHandler(emit)
ACPConn->>AgentProc: Initialize / NewSession / SetSessionModel?
ACPConn->>AgentProc: Prompt(prompt_blocks)
loop notifications
AgentProc-->>ACPConn: SessionNotification
ACPConn->>Handler: SessionUpdate(notification)
Handler->>Caller: emit(agent.Event)
end
ACPBackend->>AgentProc: close stdin, wait (grace)
alt not exited
ACPBackend->>AgentProc: force-kill
end
ACPBackend->>Caller: return SessionResult
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
pipeline/handlers/codergen.go (1)
97-106: Token tracking switch could benefit from a comment about extensibility.The type switch correctly handles token reporting for external backends that bypass the LLM client middleware. Consider adding a brief comment noting that new external backends would need a case here.
💡 Optional: Add extensibility note
// Report token usage from backends that bypass the LLM client middleware // (e.g., claude-code subprocess, ACP agents). Native backend usage flows // through the TokenTracker middleware automatically — skip to avoid double-counting. + // NOTE: Add a case here when introducing new external backends. switch backend.(type) { case *ClaudeCodeBackend:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pipeline/handlers/codergen.go` around lines 97 - 106, Add a short explanatory comment inside the type switch in pipeline/handlers/codergen.go (the switch that inspects backend.(type) and calls h.tokenTracker.AddUsage for *ClaudeCodeBackend and *ACPBackend) noting that this switch is the place to add handling for any new external/backdoor LLM backends that bypass the LLM client middleware; mention that new cases should follow the pattern of checking h.tokenTracker and sessResult.Usage.TotalTokens > 0 and calling h.tokenTracker.AddUsage with a unique backend key and sessResult.Usage so token reporting remains consistent.go.mod (1)
3-3: Go 1.25.5 is a valid version; consider updating to Go 1.26.1 if possible.Go 1.25.5 is confirmed as a legitimate release. However, as of April 2026, Go 1.26.1 is the latest stable version. If there is no specific requirement to use 1.25.5 (e.g., compatibility constraints or pending security updates), consider updating to the latest version for access to improvements and security patches.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@go.mod` at line 3, The go.mod specifies "go 1.25.5"; update that module directive to "go 1.26.1" (replace the "go 1.25.5" line with "go 1.26.1"), then run go mod tidy and a full test/build to ensure compatibility; if project constraints require 1.25.5, document the reason in the repo instead of leaving the outdated directive.pipeline/handlers/backend_acp_client.go (2)
165-179: Consider validating negative line numbers.If
p.Lineis negative (e.g.,*p.Line = -5), the condition*p.Line > 1is false, sostartremains 0. This may be acceptable (treating invalid values as "start from beginning"), but explicit validation could prevent confusion.Similarly, a negative
p.Limitwould causestart + *p.Limit < endto be true for any positivestart, potentially resulting inendbeing less thanstartand an empty or invalid slice. Consider adding a check:🛡️ Optional defensive validation
// Apply line/limit filtering if requested. if p.Line != nil || p.Limit != nil { lines := strings.Split(content, "\n") start := 0 if p.Line != nil && *p.Line > 1 { start = *p.Line - 1 // 1-based to 0-based } + if p.Line != nil && *p.Line < 0 { + return acp.ReadTextFileResponse{}, &acp.RequestError{Code: -32602, Message: "line must be positive"} + } if start > len(lines) { start = len(lines) } end := len(lines) - if p.Limit != nil && start+*p.Limit < end { + if p.Limit != nil && *p.Limit > 0 && start+*p.Limit < end { end = start + *p.Limit }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pipeline/handlers/backend_acp_client.go` around lines 165 - 179, Validate and guard against negative p.Line and p.Limit before computing start/end: ensure p.Line is treated as at least 1 (so start = max(0, *p.Line-1)) and treat non-positive p.Limit as "no limit" or clamp it to 0 so it cannot reduce end below start; after computing start and end ensure end >= start (clamp end = max(end, start)) before calling strings.Join on lines[start:end]; use the existing variables p.Line, p.Limit, lines, start, end, and content to implement these checks.
336-344: Blocking onts.donewithout cancellation path.Line 342 blocks on
<-ts.doneafter sending SIGKILL, without a timeout or context cancellation. While SIGKILL is uncatchable and should always terminate the process, defensive coding would add a timeout to avoid potential hangs in edge cases (e.g., zombie processes, kernel bugs).🛡️ Optional: Add timeout for safety
// Ensure process is dead before releasing. if ts.cmd.Process != nil { select { case <-ts.done: default: _ = syscall.Kill(-ts.cmd.Process.Pid, syscall.SIGKILL) - <-ts.done + select { + case <-ts.done: + case <-time.After(5 * time.Second): + log.Printf("[acp] warning: terminal %s did not exit after SIGKILL", p.TerminalId) + } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pipeline/handlers/backend_acp_client.go` around lines 336 - 344, The wait after sending SIGKILL on the process (syscall.Kill(-ts.cmd.Process.Pid, syscall.SIGKILL)) currently blocks indefinitely on <-ts.done; update the logic in the method that owns ts (the code around ts.cmd.Process != nil) to use a select with a timeout (e.g., time.After with a small configurable duration) so that after sending SIGKILL you wait for either ts.done or the timeout, and on timeout log/warn and continue; ensure you import time and still drain ts.done if it eventually closes to avoid leaks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pipeline/handlers/backend_acp_client.go`:
- Around line 211-217: CreateTerminal is using buildEnv() which strips API keys,
but terminals for ACP bridges must receive the full environment like the ACP
agent; update CreateTerminal to call buildEnvForACP() instead of buildEnv(),
then append request-level env vars as before (i.e., set cmd.Env =
buildEnvForACP() and keep the loop appending p.Env entries). Ensure you
reference the CreateTerminal function and replace the buildEnv() call so
credential routing matches the ACP bridge model used by backend_acp.go.
In `@pipeline/handlers/backend_acp_test.go`:
- Around line 422-442: The test TestACPCollectedText ignores errors returned by
acpClientHandler.SessionUpdate; update the two SessionUpdate calls to check
their returned error and fail the test on error (e.g., if err :=
h.SessionUpdate(...); err != nil { t.Fatalf("SessionUpdate failed: %v", err) }
or use require.NoError), ensuring the test fails if SessionUpdate returns an
error before asserting h.collectedText().
In `@pipeline/handlers/backend_acp.go`:
- Around line 210-231: The code currently treats a successful prompt with zero
tokens and zero tool calls as success; update the handler in this function to
treat empty agent responses as failures by validating the collected result after
buildACPResult(handler, promptResp). After ensuring promptErr == nil and before
treating forceKilled as OK, check that the response contains actual content
(e.g., promptResp token/count fields or text length > 0) or that the handler
recorded any tool calls; if both are empty, return result with an error (e.g.,
fmt.Errorf("acp: empty response from %s", agentName)) and optionally log the
condition. Ensure you reference buildACPResult, promptResp, handler (tool
calls), and agentName when adding this validation.
In `@pipeline/handlers/registry.go`:
- Around line 179-191: The code allows selecting the native backend when
cfg.llmClient is nil (e.g., defaultBackend="acp"), causing a panic when
NativeBackend.Run calls agent.NewSession with a nil client; update
ensureNativeBackend (the code path invoked by selectBackend) to detect when
cfg.llmClient (or handler.nativeBackend.client) is nil and either return an
explicit error or fall back to another backend, and/or modify selectBackend to
prevent routing to ensureNativeBackend if cfg.llmClient == nil; ensure any call
sites (NewNativeBackend, NativeBackend.Run) gracefully handle or refuse nil
client and surface a clear error instead of panicking.
---
Nitpick comments:
In `@go.mod`:
- Line 3: The go.mod specifies "go 1.25.5"; update that module directive to "go
1.26.1" (replace the "go 1.25.5" line with "go 1.26.1"), then run go mod tidy
and a full test/build to ensure compatibility; if project constraints require
1.25.5, document the reason in the repo instead of leaving the outdated
directive.
In `@pipeline/handlers/backend_acp_client.go`:
- Around line 165-179: Validate and guard against negative p.Line and p.Limit
before computing start/end: ensure p.Line is treated as at least 1 (so start =
max(0, *p.Line-1)) and treat non-positive p.Limit as "no limit" or clamp it to 0
so it cannot reduce end below start; after computing start and end ensure end >=
start (clamp end = max(end, start)) before calling strings.Join on
lines[start:end]; use the existing variables p.Line, p.Limit, lines, start, end,
and content to implement these checks.
- Around line 336-344: The wait after sending SIGKILL on the process
(syscall.Kill(-ts.cmd.Process.Pid, syscall.SIGKILL)) currently blocks
indefinitely on <-ts.done; update the logic in the method that owns ts (the code
around ts.cmd.Process != nil) to use a select with a timeout (e.g., time.After
with a small configurable duration) so that after sending SIGKILL you wait for
either ts.done or the timeout, and on timeout log/warn and continue; ensure you
import time and still drain ts.done if it eventually closes to avoid leaks.
In `@pipeline/handlers/codergen.go`:
- Around line 97-106: Add a short explanatory comment inside the type switch in
pipeline/handlers/codergen.go (the switch that inspects backend.(type) and calls
h.tokenTracker.AddUsage for *ClaudeCodeBackend and *ACPBackend) noting that this
switch is the place to add handling for any new external/backdoor LLM backends
that bypass the LLM client middleware; mention that new cases should follow the
pattern of checking h.tokenTracker and sessResult.Usage.TotalTokens > 0 and
calling h.tokenTracker.AddUsage with a unique backend key and sessResult.Usage
so token reporting remains consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 21d065df-2071-4b62-bd29-3baabff789cd
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (10)
cmd/tracker/flags.gocmd/tracker/run.gogo.modpipeline/backend.gopipeline/dippin_adapter.gopipeline/handlers/backend_acp.gopipeline/handlers/backend_acp_client.gopipeline/handlers/backend_acp_test.gopipeline/handlers/codergen.gopipeline/handlers/registry.go
pipeline/handlers/registry.go
Outdated
Comment on lines
179
to
191
| // Codergen: register when we have an LLM client OR an external backend | ||
| // that doesn't need one (claude-code, acp). External backends spawn | ||
| // their own subprocesses and don't use the native LLM client. | ||
| if cfg.llmClient != nil || cfg.defaultBackend == "claude-code" || cfg.defaultBackend == "acp" { | ||
| handler := NewCodergenHandler(cfg.llmClient, cfg.workingDir, WithGraphAttrs(graph.Attrs)) | ||
| handler.env = cfg.execEnv | ||
| handler.eventHandler = cfg.agentEvents | ||
| handler.nativeBackend = NewNativeBackend(cfg.llmClient, cfg.execEnv) | ||
| if cfg.llmClient != nil { | ||
| handler.nativeBackend = NewNativeBackend(cfg.llmClient, cfg.execEnv) | ||
| } | ||
| handler.defaultBackendName = cfg.defaultBackend | ||
| handler.tokenTracker = cfg.tokenTracker | ||
| registry.Register(handler) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how ensureNativeBackend and NewNativeBackend handle nil client
ast-grep --pattern 'func (h *CodergenHandler) ensureNativeBackend() $_ {
$$$
}'
echo "---"
ast-grep --pattern 'func NewNativeBackend($_, $_) $_ {
$$$
}'Repository: 2389-research/tracker
Length of output: 947
🏁 Script executed:
# Check NativeBackend implementation to see if/when client is used
rg -A 20 "type NativeBackend struct" pipeline/handlers/
echo "---"
# Check if client is accessed without nil checks
rg "\.client\." pipeline/handlers/backend_native.go | head -20
echo "---"
# Check if there's any code that validates backend selection when defaultBackend is acp
rg -B 3 -A 5 "selectBackend\|ensureNativeBackend" pipeline/handlers/codergen.go | head -50Repository: 2389-research/tracker
Length of output: 1693
🏁 Script executed:
# Check NativeBackend.Run method more completely
sed -n '32,100p' pipeline/handlers/backend_native.go
echo "---"
# Check where client is actually used in NativeBackend
rg "b\.client" pipeline/handlers/backend_native.go -A 2
echo "---"
# Check selectBackend logic to understand when backends are chosen
rg -A 30 "func (h \*CodergenHandler) selectBackend" pipeline/handlers/codergen.goRepository: 2389-research/tracker
Length of output: 1216
🏁 Script executed:
# Check selectBackend logic
rg -B 5 -A 40 "func (h \*CodergenHandler) selectBackend" pipeline/handlers/codergen.go
echo "---"
# Check if nodes can override backend via the node configuration
rg "backend.*native\|backend.*acp" pipeline/handlers/codergen.go | head -20
echo "---"
# Check agent.NewSession to see if it handles nil client
fd "session.go" --type f -x grep -l "func NewSession" {}
echo "---"
# Check what error agent.NewSession returns for nil client
rg -A 10 "func NewSession" agent/ 2>/dev/null | head -30Repository: 2389-research/tracker
Length of output: 696
🏁 Script executed:
# Look at codergen handler initialization to confirm client assignment
rg -B 5 -A 15 "func NewCodergenHandler" pipeline/handlers/codergen.go
echo "---"
# Search for selectBackend in all pipeline files
rg "selectBackend" pipeline/
echo "---"
# Check if there's backend validation/routing logic
rg -B 3 -A 10 "backend.*native\|backend.*acp\|defaultBackend" pipeline/handlers/codergen.go | head -60Repository: 2389-research/tracker
Length of output: 1559
🏁 Script executed:
# Get the complete selectBackend implementation
rg -A 30 "func \(h \*CodergenHandler\) selectBackend\(node" pipeline/handlers/codergen.go
echo "---"
# Check if there's validation for backend selection at node level
rg "backend.*:" pipeline/ -A 2 -B 2 | head -40Repository: 2389-research/tracker
Length of output: 3686
Nil client panic when node explicitly requests native backend with acp as default.
When defaultBackend="acp" and cfg.llmClient is nil, handler.nativeBackend is not initialized. If any node explicitly sets backend: native in its attributes, selectBackend() routes to ensureNativeBackend(), which creates a NativeBackend with a nil client. This causes a panic when NativeBackend.Run() calls agent.NewSession(b.client, ...) without validation.
Add a nil client check in ensureNativeBackend() or prevent native backend selection when no LLM client is available.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pipeline/handlers/registry.go` around lines 179 - 191, The code allows
selecting the native backend when cfg.llmClient is nil (e.g.,
defaultBackend="acp"), causing a panic when NativeBackend.Run calls
agent.NewSession with a nil client; update ensureNativeBackend (the code path
invoked by selectBackend) to detect when cfg.llmClient (or
handler.nativeBackend.client) is nil and either return an explicit error or fall
back to another backend, and/or modify selectBackend to prevent routing to
ensureNativeBackend if cfg.llmClient == nil; ensure any call sites
(NewNativeBackend, NativeBackend.Run) gracefully handle or refuse nil client and
surface a clear error instead of panicking.
- Empty agent responses (0 text, 0 tool calls) now fail instead of returning success silently (CLAUDE.md rule) - CreateTerminal uses buildEnvForACP() matching parent process env, fixing inconsistency where terminals got stripped keys but the bridge got full env - ensureNativeBackend returns error when LLM client is nil, preventing nil pointer panic when --backend acp is set globally but a node explicitly requests native backend - Unchecked error returns in TestACPCollectedText now checked Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The @zed-industries/codex-acp package installs as `codex-acp`, not `codex-agent-acp`. Verified working end-to-end with direct OpenAI key. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pipeline/handlers/backend_acp.go (1)
198-209: Consider respecting context cancellation during process cleanup.The select block waits up to 5 seconds for the process to exit but doesn't include a
ctx.Done()case. If the context is already cancelled (e.g., user hit Ctrl+C), the cleanup could still block for up to 5 seconds before force-killing.This is likely acceptable since it's cleanup code after the prompt completes, but adding
ctx.Done()would make cancellation more responsive.♻️ Optional: Add context cancellation case
select { case <-waitCh: +case <-ctx.Done(): + log.Printf("[acp] context cancelled during cleanup, killing pid %d", cmd.Process.Pid) + killProcess(cmd) + <-waitCh + forceKilled = true case <-time.After(5 * time.Second): log.Printf("[acp] process did not exit after 5s, killing pid %d", cmd.Process.Pid) killProcess(cmd) <-waitCh forceKilled = true }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pipeline/handlers/backend_acp.go` around lines 198 - 209, The select waiting on waitCh should also listen for ctx cancellation: update the select that currently checks waitCh and time.After(5*time.Second) to include a case <-ctx.Done() so cleanup is more responsive; when ctx is done, log/call killProcess(cmd) (same as the timeout path), wait for waitCh and set forceKilled as needed (or propagate cancellation) — reference waitCh, cmd.Wait(), killProcess(cmd), and ctx.Done() when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pipeline/handlers/backend_acp.go`:
- Around line 43-48: The "Default binary mapping" comment is inconsistent with
the actual mapping used in the code: update the comment text that currently
reads "anthropic → claude-code-acp" to match the implemented mapping "anthropic
→ claude-agent-acp" so the comment and the mapping are identical; locate the
"Default binary mapping" comment block in backend_acp.go and replace the binary
name in that comment to "claude-agent-acp".
- Around line 268-274: The verify step uses exec.Command(path, "--version") and
verifyCmd.Run() without a timeout, which can block ensureAgentPath/Run
indefinitely; change this to use exec.CommandContext with a context created by
context.WithTimeout (e.g., a short timeout like 2-5s), call verifyCmd :=
exec.CommandContext(ctx, path, "--version"), defer cancel(), and handle context
deadline errors by treating them like the current non-fatal verifyErr (logging
the timeout) so bridge binaries still pass LookPath; update references in
ensureAgentPath and any tests to reflect the timeout behavior.
---
Nitpick comments:
In `@pipeline/handlers/backend_acp.go`:
- Around line 198-209: The select waiting on waitCh should also listen for ctx
cancellation: update the select that currently checks waitCh and
time.After(5*time.Second) to include a case <-ctx.Done() so cleanup is more
responsive; when ctx is done, log/call killProcess(cmd) (same as the timeout
path), wait for waitCh and set forceKilled as needed (or propagate cancellation)
— reference waitCh, cmd.Wait(), killProcess(cmd), and ctx.Done() when making the
change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0eb5c38d-522c-46e3-9f0e-08c29d2a8245
📒 Files selected for processing (2)
pipeline/handlers/backend_acp.gopipeline/handlers/backend_acp_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pipeline/handlers/backend_acp_test.go
ACP agents (Claude Code, Codex, Gemini CLI) handle their own auth natively via subscription/OAuth. Passing tracker's API keys and base URLs overrides the agent's auth and can redirect it to wrong endpoints (e.g. Cloudflare AI Gateway that doesn't support WebSocket for Codex or uses a gateway-specific token instead of a real API key). Strips ANTHROPIC/OPENAI/GEMINI/GOOGLE API keys and base URLs from the ACP subprocess env. TRACKER_PASS_API_KEYS=1 overrides this. Verified all three agents authenticate natively: - claude-agent-acp: subscription auth (8.2s) - codex-acp: Codex native auth (7.0s) - gemini --acp: Google OAuth (56.0s) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This reverts commit 5456d9f.
- Fix data race: lock handler.mu before reading textParts/toolCount - Fix data race: replace bytes.Buffer with syncBuffer for terminal output - Validate InitializeResponse.ProtocolVersion (log warning on mismatch) - Fall back to os.Getwd() when WorkingDir is empty for NewSession Cwd - Add Pid > 0 guard before syscall.Kill to prevent killing pid 0 group - Fix TRACKER_PASS_API_KEYS truthiness: check == "1" not != "" - Fix Godoc comment typos: claude-code-acp → claude-agent-acp, codex-agent-acp → codex-acp Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
harperreed
added a commit
that referenced
this pull request
Apr 4, 2026
# Conflicts: # go.sum # pipeline/handlers/backend_acp.go # pipeline/handlers/backend_acp_test.go
…ests, merge main - Add 5s context timeout on --version verification to prevent indefinite blocking (backend_acp.go) - Fix comment: codex-acp install is npm, not cargo - Add tests: mapModelToBridge, buildACPResult, buildACPMcpServers, buildEnvForACP, killProcess, NewPipelineContextFrom, resolveAgentName - Merge main: resolve go.sum, acpStrippedPrefixes (add OPENAI_COMPAT_*), selectBackend test (node attr override) conflicts Note: pipeline coverage at 79.2% (below 80% threshold) due to untestable Run() integration path (needs real ACP binary). Pre-existing on this branch. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
… CLAUDE.md no-verify rule - Split Run() (cyclomatic 24, cognitive 30) into 6 focused functions: startProcess, initSession, setSessionModel, waitForProcess, logStderr, isEmpty. Run() now delegates to these — complexity under limits. - Add tests: waitForProcess, logStderr, isEmpty, setSessionModel, startProcess error path, Run agent-not-found path, mapModelToBridge, buildACPResult, buildACPMcpServers, buildEnvForACP (all paths), NewPipelineContextFrom — pipeline coverage now 80.0%. - Add --no-verify prohibition to CLAUDE.md Critical Rules section. - Fix comment: codex-acp install is npm, not cargo. - Add 5s timeout on --version verification. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Cyclomatic stays at 15. Cognitive drops from 25 to 15 to enforce tighter function decomposition. 46 existing violations need cleanup in follow-up work. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Pre-commit now checks cyclomatic (≤15) and cognitive (≤15) complexity on staged .go files. Existing violations in untouched files don't block commits, but new violations are caught. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…sholds Break up five over-budget functions (cyclomatic/cognitive): - ConsoleInterviewer.AskInterview (35/81) → askYesNoQuestion, askOptionQuestion, resolveOptionInput, askFreeformQuestion, fillRemainingEmpty helpers - ConsoleInterviewer.Ask (*/18) → printChoices + matchConsoleChoice helpers - HumanHandler.executeInterview (18/25) → resolveInterviewKeys, resolveAgentOutput, parseInterviewQuestions, executeInterviewFallback, runInterview helpers - ToolHandler.Execute (21/27) → expandAndValidateCommand, applyWorkingDir, parseTimeout, parseOutputLimit, execAndBuildOutcome helpers - parseInterviewResponse (21/34) → extractJSONObject, unmarshalInterviewEnvelope, mapInterviewAnswers, normaliseAnswer, anyAnswered helpers (also refactors parseDecision) - CodergenHandler.buildOutcome (18/19) → buildEmptyResponseOutcome, buildSuccessOutcome, buildTurnLimitMsg helpers No behaviour changes; all tests pass. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Extract render helpers, filter predicates, and scroll logic from View/rebuildFilter/autoScroll/renderNodeLineAt/prefill/collectAnswers in interview_content.go, agentlog.go, and nodelist.go so all functions stay at or below cyclomatic ≤ 15 and cognitive ≤ 15. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ty thresholds Extracted helpers across 31 files to bring all non-test functions under cyclomatic ≤ 15 and cognitive ≤ 15. Zero violations remain. No behavior changes. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…lations Reduced gocyclo and gocognit violations to zero across the non-test codebase. Extracted helpers in: dippin_adapter (FromDippinIR, extractNodeAttrs split into value/pointer dispatchers), agentlog (Update split into applyStreamMsg/applyControlMsg), audit (loadActivityLog → parseActivityLine + parseActivityTimestamp), diagnose (suggestOutputPatterns split into five single-pattern helpers), run.go (chooseInterviewer → chooseAutopilotInterviewer), validate.go (validateEdgeLabelConsistency → checkNodeEdgeLabelConsistency + countLabeledEdges), google/adapter (parseSSE → processSSELine), apply_patch (parseUpdateFile/parseHunk → parseUpdateHunks + processHunkLine), grep (searchDir closure → walkEntry method). All 15 packages pass go test ./... -short with no behavior changes. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…lexity 10 Extract helper functions across 44 files to bring every non-test function under the gocyclo ≤ 10 and gocognit ≤ 10 thresholds (previously 63 violations). Lower Makefile and .pre-commit thresholds from 15 to 10. No behavior changes. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Extract helper functions across 65 files to reduce cyclomatic and cognitive complexity to ≤ 8 in all non-test, non-conformance production code. All 119 violations (40 cyclomatic, 79 cognitive) resolved via extract-method refactoring without behavior changes. Makefile and .pre-commit thresholds set to 8/8 to enforce the new baseline. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Adds ACP (Agent Client Protocol) as a third agent backend (alongside native and claude-code), enabling tracker to orchestrate ACP-compatible coding agents via JSON-RPC over stdio, plus broad refactors to reduce complexity and improve maintainability across TUI/pipeline/LLM layers.
Changes:
- Introduce ACP backend plumbing (client handler, backend selection, config/flags, lifecycle + session/event translation).
- Refactor multiple subsystems (TUI rendering, pipeline parsing/validation/linting, engine loop control, LLM adapters/trace/retry, agent tools) into smaller helper functions to reduce complexity.
- Tighten developer workflow gates by lowering complexity thresholds and adding staged-file complexity checks in pre-commit.
Reviewed changes
Copilot reviewed 98 out of 98 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
tui/state.go |
Refactors state update dispatch into node vs pipeline/thinking handlers. |
tui/search.go |
Extracts ANSI stripping logic into helper functions. |
tui/review_hybrid.go |
Refactors review UI construction, key handling, and rendering into helpers. |
tui/nodelist.go |
Refactors node list rendering/scrolling into smaller functions. |
tui/modal.go |
Extracts choice/autopilot key handling into helpers. |
tui/interviewer.go |
Extracts default cursor selection and result fallback logic. |
tui/hybrid.go |
Refactors radio-mode key handling and view rendering into helpers. |
tui/history.go |
Extracts trail entry rendering. |
tui/header.go |
Splits header into left/right segment builders (backend/autopilot + tokens/cost). |
tui/adapter.go |
Deduplicates pipeline/agent event message extraction via helpers. |
pipeline/validate_semantic.go |
Refactors condition-syntax validation and compaction-threshold validation into helpers. |
pipeline/stylesheet.go |
Refactors stylesheet parsing and rule resolution into reusable helpers. |
pipeline/retry_policy.go |
Refactors retry policy resolution into base selection + overrides. |
pipeline/parser.go |
Refactors DOT node/edge building and DOT unescaping into helpers. |
pipeline/lint_dippin_extra.go |
Refactors DIP lints (DIP105/106/103/109) into focused helpers. |
pipeline/handlers/transcript.go |
Extracts transcript formatting for tool/text events into helpers. |
pipeline/handlers/registry.go |
Refactors handler registration and supports codergen init for external backends. |
pipeline/handlers/interview_result.go |
Extracts interview summary formatting/counting into helpers. |
pipeline/handlers/interview_parse.go |
Refactors JSON extraction and markdown question parsing into helpers. |
pipeline/handlers/fanin.go |
Extracts parallel result merging logic. |
pipeline/handlers/backend_native.go |
Refactors native session config construction into helpers. |
pipeline/handlers/backend_claudecode_ndjson.go |
Extracts NDJSON message dispatch into helpers. |
pipeline/handlers/backend_claudecode.go |
Adds safe emit, refactors argv/env building and error classification helpers. |
pipeline/handlers/backend_acp_client.go |
Refactors ACP session update handling, line filtering, and tool output extraction. |
pipeline/handlers/autopilot_claudecode.go |
Extracts Claude CLI subprocess run and interview attempt logic. |
pipeline/graph.go |
Refactors handler resolution and diamond overrides into helpers. |
pipeline/events_logger.go |
Refactors pipeline event logging into node/global writers. |
pipeline/events_jsonl.go |
Refactors JSONL log entry building and decision field application. |
pipeline/engine_checkpoint.go |
Refactors goal-gate retry targeting into smaller helpers. |
pipeline/engine.go |
Refactors strict-failure and completed-target handling into helpers. |
pipeline/dippin_adapter_edges.go |
Refactors fan-in indexing and edge synthesis into helpers. |
pipeline/context_test.go |
Adds test for NewPipelineContextFrom copy semantics. |
pipeline/condition.go |
Refactors equality clause evaluation and ctx namespace resolution. |
pipeline/backend.go |
Refactors MCP server line parsing into a helper. |
llm/trace_logger.go |
Refactors batching logic into helpers. |
llm/trace.go |
Refactors trace event building/formatting into helpers. |
llm/stream.go |
Refactors stream accumulation into focused handlers and content assembly helpers. |
llm/retry.go |
Refactors retry loop into a helper. |
llm/openaicompat/translate.go |
Extracts tool call ID resolution helper. |
llm/openai/translate.go |
Extracts text-part extraction and provider-options parsing helpers. |
llm/openai/adapter_sse.go |
Splits SSE event routing and error typing into helper functions. |
llm/openai/adapter.go |
Refactors streaming request setup and SSE parsing into helpers. |
llm/google/adapter.go |
Refactors SSE processing into helper and clarifies termination behavior. |
llm/errors.go |
Refactors status→typed error selection into helpers. |
llm/client.go |
Refactors provider option building and trace observer flushing/notify helpers. |
llm/anthropic/translate_response.go |
Extracts block→content translation helpers. |
llm/anthropic/translate.go |
Refactors system-role detection, beta header parsing, and content translation helpers. |
llm/anthropic/adapter.go |
Refactors stream request/body building and empty-response logging helper. |
llm/activity_tracker.go |
Refactors activity event construction into helpers. |
cmd/tracker/vcs.go |
Refactors VCS build-info extraction into helpers. |
cmd/tracker/validate.go |
Refactors validation pipeline loading/registry building/printing into helpers. |
cmd/tracker/update_check.go |
Extracts update-check core logic into runUpdateCheck. |
cmd/tracker/update.go |
Refactors install method detection and checksum/download/extract helpers. |
cmd/tracker/summary.go |
Refactors session stat aggregation, tool breakdown sorting, and totals printing. |
cmd/tracker/simulate.go |
Refactors BFS ordering + row printing into helpers. |
cmd/tracker/setup_model.go |
Extracts provider row rendering helper. |
cmd/tracker/resolve.go |
Refactors explicit-path detection and not-found error construction. |
cmd/tracker/nodes.go |
Refactors node entry building and parallel-branch detection helpers. |
cmd/tracker/main.go |
Extracts flag error handling and workdir resolution helpers. |
cmd/tracker/loading.go |
Refactors recursive subgraph loading into per-node helper. |
cmd/tracker/json_stream.go |
Refactors agent event→JSON stream entry construction into helpers. |
cmd/tracker/flags.go |
Refactors subcommand mapping and run-flag parsing into helpers; adds backend validation helper. |
cmd/tracker/doctor.go |
Refactors provider env-var checking into helper. |
agent/tools/grep.go |
Refactors grep tool parsing/search/walk formatting into helpers. |
agent/tools/edit.go |
Refactors edit tool create/replace paths into helpers. |
agent/tools/bash.go |
Refactors timeout resolution and output formatting into helpers. |
agent/tools/apply_patch.go |
Refactors patch parsing/hunk processing into helpers. |
agent/session_run.go |
Refactors tool cache policy and caching/invalidation behavior into helpers. |
agent/result.go |
Refactors session result string formatting into helpers. |
agent/exec/local.go |
Refactors limited/unlimited exec and error translation into helpers. |
agent/config.go |
Refactors config validation into grouped helper checks. |
agent/compaction.go |
Refactors compaction summaries and tool-message compaction into helpers. |
Makefile |
Lowers complexity thresholds (CYCLO_MAX, COGNITIVE_MAX) to 8. |
CLAUDE.md |
Documents policy forbidding --no-verify. |
.pre-commit |
Adds staged-file complexity checks aligned with the new thresholds. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
181
to
+189
| func (h *acpClientHandler) ReadTextFile(_ context.Context, p acp.ReadTextFileRequest) (acp.ReadTextFileResponse, error) { | ||
| if !filepath.IsAbs(p.Path) { | ||
| return acp.ReadTextFileResponse{}, &acp.RequestError{Code: -32602, Message: fmt.Sprintf("path must be absolute: %q", p.Path)} | ||
| } | ||
| data, err := os.ReadFile(p.Path) | ||
| if err != nil { | ||
| return acp.ReadTextFileResponse{}, &acp.RequestError{Code: -32603, Message: err.Error()} | ||
| } | ||
| content := string(data) | ||
| content := applyLineFilter(string(data), p.Line, p.Limit) |
There was a problem hiding this comment.
ReadTextFile currently requires an absolute path and then reads it directly from disk. This allows an ACP agent to read arbitrary files outside the workspace (e.g., /etc/passwd) and diverges from the tool-layer convention of restricting paths to the working directory. Consider validating that p.Path is under h.workingDir (or resolving relative paths against h.workingDir and rejecting escapes) and returning a -32602 error when the request targets files outside the workspace.
Comment on lines
+193
to
+210
| // applyLineFilter slices a file's content by optional 1-based start line and limit. | ||
| func applyLineFilter(content string, line, limit *int) string { | ||
| if line == nil && limit == nil { | ||
| return content | ||
| } | ||
|
|
||
| return acp.ReadTextFileResponse{Content: content}, nil | ||
| lines := strings.Split(content, "\n") | ||
| start := 0 | ||
| if line != nil && *line > 1 { | ||
| start = *line - 1 | ||
| } | ||
| if start > len(lines) { | ||
| start = len(lines) | ||
| } | ||
| end := len(lines) | ||
| if limit != nil && start+*limit < end { | ||
| end = start + *limit | ||
| } | ||
| return strings.Join(lines[start:end], "\n") |
There was a problem hiding this comment.
applyLineFilter can panic on malformed ACP requests: a negative *limit (or a very large value causing start+limit overflow) can produce end < start or a negative end, which will panic when slicing lines[start:end]. Consider validating that line/limit are positive (or treating non-positive limit as 'no limit'), and clamping end using bounds checks that avoid integer overflow.
…lter panic - ReadTextFile and WriteTextFile now validate paths are under workingDir via validatePathInWorkDir (prevents reading /etc/passwd etc.) - applyLineFilter decomposed into resolveLineRange/derefLineStart/clampInt to handle negative line/limit without panics and stay under complexity 8 - Tests: path validation (5 cases), negative line/limit, path rejection Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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.
Summary
claude-agent-acp(Anthropic),codex-agent-acp(OpenAI),gemini --acp(Google)claude-sonnet-4-5) to bridge model IDs (sonnet) via substring matchingUsage
Files changed
pipeline/handlers/backend_acp.gopipeline/handlers/backend_acp_client.gopipeline/handlers/backend_acp_test.gopipeline/handlers/codergen.gopipeline/handlers/registry.gopipeline/backend.gopipeline/dippin_adapter.goacp_agentfrom dippin paramscmd/tracker/flags.go--backend acpflag validationcmd/tracker/run.goTest plan
go build ./...passesgo test ./... -short— all 14 packages pass.scratch/acp_smoke_test.dipcompletes end-to-end viaclaude-agent-acpcodex-agent-acpbridge (requires npm install)gemini --acp(requires Gemini CLI)backend: acpmixed with native nodes🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Updates
Tests