chore(deps): migrate ACP SDK from agentcooper fork to upstream v0.13.0#921
Open
carlosflorencio wants to merge 5 commits into
Open
chore(deps): migrate ACP SDK from agentcooper fork to upstream v0.13.0#921carlosflorencio wants to merge 5 commits into
carlosflorencio wants to merge 5 commits into
Conversation
Upstream coder/acp-go-sdk#8 (merged 2026-02-24) fixed the notification ordering race that originally justified the fork. v0.13.0 is a strict superset of the fork on the session API surface and adds CloseSession, plus the WaitGroup-reuse fix in #30. - Drop replace directive; bump require to v0.13.0 - Rename UnstableModelId/Info/SessionModelState -> ModelId/Info/SessionModelState (UnstableSetSessionModelRequest still uses UnstableModelId upstream) - Rename KillTerminalCommand -> KillTerminal - Wrap SetSessionConfigOptionRequest in new ValueId tagged-union variant - Collapse new AuthMethod tagged union ({Agent,Terminal,EnvVar}) back to flat (id, name, description, meta) via helpers in convertAuthMethods and probe payload builder - Add CloseSession / ListSessions / ResumeSession stubs to mock-agent to satisfy the wider upstream Agent interface - Refresh stale comments referencing the fork
📝 WalkthroughWalkthroughThis PR upgrades the ACP SDK from v0.6.3 to v0.13.0, stabilizing model and session types throughout the adapter and mock layers, adding auth-method extraction helpers, implementing session lifecycle handlers in the mock agent, and updating ACP protocol method signatures. ChangesACP SDK Upgrade and Stabilization
🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Claude finished @carlosflorencio's task in 1m 37s —— View job Code Review
Verdict: Ready to merge. Clean mechanical SDK migration with no blockers. One suggestion posted as an inline comment: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
| Filename | Overview |
|---|---|
| apps/backend/internal/agentctl/server/acp/authmethod.go | New shared helper that collapses the upstream AuthMethod tagged union into a flat (id, name, desc, meta) tuple; eliminates the duplicate-site drift risk flagged in a previous review thread. |
| apps/backend/internal/agentctl/server/acp/authmethod_test.go | Covers all four AuthMethod variants (Agent, Terminal, EnvVar, zero-value) including description nil-handling; complete and correct. |
| apps/backend/internal/agentctl/server/adapter/transport/acp/adapter.go | Two targeted changes: UnstableModelInfo→ModelInfo/UnstableSessionModelState→SessionModelState rename, and SetSessionConfigOptionRequest wrapped in ValueId tagged-union variant. |
| apps/backend/internal/agentctl/server/adapter/transport/acp/meta_convert.go | convertAuthMethods now delegates to the shared AuthMethodFields helper; convertSessionModels gets the ModelInfo rename. Zero-value filtering logic is consistent with acp_executor.go. |
| apps/backend/cmd/mock-agent/main.go | Adds CloseSession/ListSessions/ResumeSession stubs to satisfy widened interface; CloseSession correctly tears down session state; emitAvailableCommandsOnce gains a well-ordered guard for closed sessions. |
| apps/backend/internal/agentctl/server/utility/acp_executor.go | buildInitProbeFields updated to use shared AuthMethodFields helper with the same zero-value filter; stale fork-specific comment removed from toACPMcpServers. |
| apps/backend/go.mod | Fork replace directive removed; acp-go-sdk bumped to v0.13.0; docker/go-connections and lumberjack.v2 promoted from indirect to direct (expected go mod tidy side-effect of the SDK bump). |
| apps/backend/internal/agentctl/server/acp/client.go | Mechanical rename of KillTerminalCommand→KillTerminal with matching request/response type updates; no logic changes. |
| apps/backend/internal/agentctl/server/adapter/transport/acp/ordering_race_test.go | Comment updated to reference upstream fix (coder/acp-go-sdk#8) instead of the fork; test logic unchanged and continues to guard the notification ordering regression. |
| apps/backend/internal/agent/acpdbg/ops.go | Stale comment about fork-specific unstable/stable fallback strategy removed; underlying code unchanged. |
| AGENTS.md | New 'Worktrees and commit hooks' section documents pnpm install prerequisite for fresh worktrees; accurate and useful developer guidance. |
Sequence Diagram
sequenceDiagram
participant Host as Kandev Backend
participant Adapter as ACP Adapter
participant Helper as AuthMethodFields(authmethod.go)
participant SDK as coder/acp-go-sdk v0.13.0
participant Agent as ACP Agent
Host->>SDK: Initialize
SDK->>Agent: initialize request
Agent-->>SDK: "InitializeResponse{AuthMethods: []AuthMethod}"
SDK-->>Adapter: InitializeResponse (tagged union)
Adapter->>Helper: AuthMethodFields(m) for each AuthMethod
Helper-->>Adapter: (id, name, desc, meta)
Adapter-->>Host: "ProbeAuthMethod{ID, Name, ...}"
Host->>SDK: NewSession / LoadSession
SDK->>Agent: session/new or session/load
Agent-->>SDK: "SessionModelState (stable, no Unstable* prefix)"
SDK-->>Adapter: []ModelInfo (was UnstableModelInfo)
Host->>SDK: SetConfigOption(configID, value)
SDK->>Agent: "SetSessionConfigOptionRequest{ValueId: &{SessionId, ConfigId, Value}}"
Agent-->>SDK: SetSessionConfigOptionResponse
Host->>SDK: CloseSession
SDK->>Agent: CloseSession (new in v0.13.0)
Agent-->>SDK: CloseSessionResponse
Reviews (5): Last reviewed commit: "docs(claude-md): note pnpm install requi..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
FindingsSuggestion (recommended, doesn't block)
Summary
Verdict: Ready to merge Clean, mechanical migration. The |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Both meta_convert.go and acp_executor.go had byte-for-byte identical helpers for collapsing the new tagged-union acp.AuthMethod into a flat (id, name, description, meta) tuple. A new upstream variant (e.g. OAuth) would have to be added to both sites or one path silently drops auth methods of that type. Move the helper into the existing acpclient package (internal/agentctl/server/acp) which both call sites already depend on. Addresses PR #921 review feedback from Claude and Greptile.
|
Claude finished @carlosflorencio's task in 2m 43s —— View job Code Review
FindingsSuggestion (recommended, doesn't block)
Summary
Verdict: Ready to merge The consolidation commit ( The rest of the migration is unchanged from what was already reviewed: type renames, |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/backend/cmd/mock-agent/main.go`:
- Around line 193-199: The CloseSession implementation removes session state but
doesn't prevent already-scheduled emitAvailableCommandsAfterDelay goroutines
from emitting updates for closed sessions; update
emitAvailableCommandsAfterDelay (or the code path that sends
available_commands_update) to acquire a.mu and check that the session id still
exists in a.sessions (or that commandsEmitted entry still exists) before
performing any emit, returning early if the session was removed; reference
mockAgent, CloseSession, emitAvailableCommandsAfterDelay,
available_commands_update, commandsEmitted, sessions and mu when adding this
active-session guard.
🪄 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: a5bcd301-ada2-4eb7-9798-8960e7ee4de7
⛔ Files ignored due to path filters (1)
apps/backend/go.sumis excluded by!**/*.sum
📒 Files selected for processing (9)
apps/backend/cmd/mock-agent/main.goapps/backend/go.modapps/backend/internal/agent/acpdbg/ops.goapps/backend/internal/agentctl/server/acp/authmethod.goapps/backend/internal/agentctl/server/acp/client.goapps/backend/internal/agentctl/server/adapter/transport/acp/adapter.goapps/backend/internal/agentctl/server/adapter/transport/acp/meta_convert.goapps/backend/internal/agentctl/server/adapter/transport/acp/ordering_race_test.goapps/backend/internal/agentctl/server/utility/acp_executor.go
💤 Files with no reviewable changes (1)
- apps/backend/internal/agent/acpdbg/ops.go
emitAvailableCommandsAfterDelay sleeps 50ms before emitting the available_commands_update notification. If CloseSession ran during that window, the goroutine would still emit for a session the mock has already forgotten about. Bail out in emitAvailableCommandsOnce when the session is no longer registered. Addresses CodeRabbit review on PR #921.
|
Claude finished @carlosflorencio's task in 1m 56s —— View job Code Review
FindingsBlocker (must fix before merge)
Suggestion (recommended, doesn't block)
Previously resolved
Summary
Verdict: Blocked — add the |
- Span on Client.KillTerminal still read "request.kill_terminal_command" after the method rename; switch to "request.kill_terminal" so traces match the method label. - Add table-driven tests for AuthMethodFields covering each variant (Agent, Terminal, EnvVar) plus the zero-value case so a future upstream variant that we forget to handle surfaces as a test failure instead of silently dropping auth methods. Addresses Claude review on PR #921.
|
Claude finished @carlosflorencio's task in 1m 42s —— View job Code Review
FindingsAll previously flagged issues have been resolved in this iteration.
No new issues found. The implementation is correct throughout:
Verdict: Ready to merge. |
The commit-msg hook runs commitlint via `cd apps && pnpm exec`, which fails in a fresh git worktree because worktrees share `.git/` but not `apps/node_modules/`. Surfaced as `Command "commitlint" not found` / `ERR_PNPM_RECURSIVE_EXEC_FIRST_FAIL` on the first commit attempt. Hit during the ACP SDK migration in this PR.
|
Claude finished @carlosflorencio's task in 1m 31s —— View job Code Review
FindingsNo issues found. All previously flagged items have been resolved:
The implementation is correct throughout:
Summary
Verdict: Ready to merge. |
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
1 participant
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.
The fork was vendored via
go.modreplaceto pick up a notification ordering fix (coder/acp-go-sdk#20), but the fix landed upstream in PR #8 (2026-02-24) — three days before the fork commit we pinned. Upstream v0.13.0 is a strict superset of the fork on the session API (gainsCloseSession; promotesListSessions/ResumeSessionout of Unstable) and also includes the WaitGroup-reuse fix in #30.Important Changes
replace github.com/coder/acp-go-sdk => github.com/agentcooper/acp-go-sdk ...; bump require to v0.13.0.acp.UnstableModelId/UnstableModelInfo/UnstableSessionModelStateto their un-prefixed names.UnstableSetSessionModelRequest.ModelIdstill usesUnstableModelIdupstream — kept as-is.acp.SetSessionConfigOptionRequestis now a tagged union; wrap our existing call in theValueIdvariant.acp.AuthMethodis now a tagged union of{Agent, Terminal, EnvVar}; addedflattenAuthMethod/authMethodFieldshelpers to collapse it back to the flat(id, name, description, meta)shape our streams and probe layers expect.KillTerminalCommand*→KillTerminal*ininternal/agentctl/server/acp/client.go.CloseSession/ListSessions/ResumeSessionstubs tocmd/mock-agentto satisfy the wider upstreamacp.Agentinterface.Validation
go test ./...fromapps/backend— 5979 tests passing across 196 packages.make lint— 0 issues.TestNotificationOrderingFix— 3 consecutive-count=3runs pass, confirming upstream's serialized notification path preserves ordering.Possible Improvements
Low risk. Behavior on the wire is unchanged; SDK type-shape changes are mechanical and covered by existing tests. The new
AuthMethodunion has typedTerminal/EnvVarvariants we could use instead of the_meta.terminal-authreflection path, but that is a follow-up.Checklist
apps/web/), I have added or updated Playwright e2e tests inapps/web/e2e/and verified them withmake test-e2e.Preview Environment
5de8495