chore: bump go dependencies (acp-go-sdk, goja)#2995
Conversation
Assisted-By: docker-agent
…o v0.0.0-20260603143327-1f200ca63355 Assisted-By: docker-agent
| // Logout implements [acp.Agent] (optional, not supported). | ||
| func (a *Agent) Logout(ctx context.Context, _ acp.LogoutRequest) (acp.LogoutResponse, error) { | ||
| slog.DebugContext(ctx, "ACP Logout called (not supported)") | ||
| return acp.LogoutResponse{}, acp.NewMethodNotFound(acp.AgentMethodLogout) |
There was a problem hiding this comment.
[MEDIUM] Logout returns acp.NewMethodNotFound — verify this is the correct error for an optional-but-present method
The new Logout stub returns acp.NewMethodNotFound(acp.AgentMethodLogout) to signal the method is not supported. The adjacent LoadSession stub (on the separate, optional acp.AgentLoader interface) uses errors.New("load session not supported") instead.
The concern is about protocol semantics: if the ACP framework treats MethodNotFound as a hard protocol error (e.g., causing the client to drop the connection or enter an error state), any client that sends a Logout request will experience a disruption rather than graceful degradation.
Logout is part of the core acp.Agent interface (not an optional sub-interface), so NewMethodNotFound may be the idiomatic way to say "this method exists in the interface but is not implemented here." However, it's worth confirming against the SDK changelog or the ACP protocol spec that this error code is handled gracefully by clients. If there's any doubt, using a plain errors.New("logout not supported") (matching the LoadSession pattern) would be the safer choice until the protocol semantics are confirmed.
aheritier
left a comment
There was a problem hiding this comment.
Looks good to me.
Reviewed the dependency bumps and the ACP SDK interface adjustment:
go.modupdates the intended direct dependencies only:github.com/coder/acp-go-sdktov0.13.5andgithub.com/dop251/gojatov0.0.0-20260603143327-1f200ca63355(go.mod:30,go.mod:35).go.sumcontains matching checksums for those versions and removes the now-unusedgithub.com/dlclark/regexp2 v1.12.0, while retaininggithub.com/dlclark/regexp2/v2 v2.1.1(go.sum:176-177,go.sum:200-221).- The new ACP SDK requires
Logout(ctx, LogoutRequest) (LogoutResponse, error)onacp.Agent; the implementation adds that method atpkg/acp/agent.go:219-223. - Returning
acp.NewMethodNotFound(acp.AgentMethodLogout)is consistent with the agent not advertising logout capability inInitialize(pkg/acp/agent.go:95-111), and it maps to the SDK's JSON-RPC "Method not found" error path. - The existing compile-time assertion
var _ acp.Agent = (*Agent)(nil)(pkg/acp/agent.go:37) verifies the implementation satisfies the bumped SDK interface. - I did not find correctness, security, duplication, readability, or project-convention issues in the diff.
CI is green for head SHA 59ad11e41d8accd63a92cca507e2056947c5e37a: build-and-test, lint, license-check, build-image, and PR Review all completed successfully; build-and-push-image was skipped.
Bumps two direct Go dependencies and handles interface changes in the ACP SDK.
The
github.com/coder/acp-go-sdkbump from v0.13.0 to v0.13.5 introduces a newLogoutmethod on theacp.Agentinterface. This requires implementing the method inpkg/acp/agent.go.A third dependency,
google.golang.org/adk, was intentionally skipped in this PR. Version v1.4.0 deprecatesserver/adka2ain favor ofserver/adka2a/v2, requiring a non-trivial API migration (queue-based to iterator-based executor) and a major version bump ofa2a-go/v2. This will be addressed in a separate PR.Lint and tests pass.