From 80fa582bfc3972a0d0cda7ef26be08918db3c901 Mon Sep 17 00:00:00 2001 From: David Gageot Date: Wed, 23 Sep 2026 13:56:53 +0200 Subject: [PATCH] fix(acp): report consistent tool-call lifecycles Signed-off-by: David Gageot Assisted-By: docker-agent --- docs/features/acp/index.md | 12 + lint/drain_run_stream_before_release.go | 12 +- lint/drain_run_stream_before_release_test.go | 5 + pkg/acp/agent.go | 50 +-- pkg/acp/elicitation_test.go | 3 +- pkg/acp/filediff_test.go | 2 +- pkg/acp/runagent_drain_test.go | 4 +- pkg/acp/runagent_test.go | 147 ++++---- pkg/acp/tool_lifecycle.go | 125 +++++++ pkg/acp/tool_lifecycle_test.go | 358 +++++++++++++++++++ pkg/acp/toolcall.go | 25 +- pkg/acp/toolcall_test.go | 17 +- 12 files changed, 647 insertions(+), 113 deletions(-) create mode 100644 pkg/acp/tool_lifecycle.go create mode 100644 pkg/acp/tool_lifecycle_test.go diff --git a/docs/features/acp/index.md b/docs/features/acp/index.md index 66e3c59bd..19a1be000 100644 --- a/docs/features/acp/index.md +++ b/docs/features/acp/index.md @@ -166,6 +166,18 @@ These are path checks, not an atomic sandbox around client I/O. The ACP client m Configured `post_edit` commands run **locally**, in the session toolset's working directory, only after a successful client write. They receive the checked target path in `${file}` and match patterns against that target, not a symlink alias. Hooks require the client and agent to share a coherent on-disk filesystem; editor-buffer-only writes are not mirrored to local disk. A hook failure reports that the write succeeded but the hook failed, without retrying or rolling back the write. +## Tool-Call Lifecycle + +ACP reports a tool awaiting permission as `pending`. Actual execution is reported as `in_progress`, followed by `completed` or `failed`. Permission requests and subsequent updates refer to the same opaque ACP tool-call ID; permission approval is not inferred from display status. + +Rejected, policy-denied, or unavailable tools can produce a terminal result without executing. These become failed tool items instead of aborting the conversation or inventing a running phase. Tool kinds use one display classifier across permission and execution updates: operation names take precedence over generic hints, and a destructive hint alone does not mean deletion. + +ACP assigns fresh IDs rather than exposing runtime IDs directly. Correlation distinguishes agent names and supports sequential runtime-ID reuse, including across turns. The runtime does not yet provide enough identity to disambiguate simultaneous identical IDs from two sub-sessions of the same agent. + +On interruption, ACP drains runtime events before finishing the prompt. Known results retain their actual status and output; visible calls with no result receive a best-effort failed update explaining that side effects may have occurred. Cancellation still determines the prompt's `cancelled` stop reason. Notification failure cannot undo tool execution, and the transport does not guarantee a hard timeout for blocked writes. Terminal write failures are not retried because delivery may have been partial. + +Partial tool arguments and incremental tool output are not streamed by this adapter. Completed results, including eligible file diffs, remain available; this avoids presenting unapproved partial input or treating independently transformed output chunks as whole-output redaction. + ## Tool Locations and File Diffs Tool-start and permission updates report absolute file locations. Relative paths are resolved lexically against the session's known workspace, with home-directory expansion; this display metadata does not read files or grant access. URI-shaped values and relative paths without an absolute workspace are omitted. diff --git a/lint/drain_run_stream_before_release.go b/lint/drain_run_stream_before_release.go index 784bb1503..ac780f907 100644 --- a/lint/drain_run_stream_before_release.go +++ b/lint/drain_run_stream_before_release.go @@ -271,10 +271,16 @@ func deferredStreamDrain(p *cop.Pass, d *ast.DeferStmt, source runStreamSource) case *ast.ExprStmt: // e.g. cancel() before draining continue case *ast.RangeStmt: - if matches(p, stmt.X) && len(stmt.Body.List) == 0 { - return kind + if !matches(p, stmt.X) { + return streamDrainNone } - return streamDrainNone + for _, bodyStmt := range stmt.Body.List { + // Calls can retain final results without leaving the drain loop. + if _, ok := bodyStmt.(*ast.ExprStmt); !ok { + return streamDrainNone + } + } + return kind default: return streamDrainNone } diff --git a/lint/drain_run_stream_before_release_test.go b/lint/drain_run_stream_before_release_test.go index c4cdc0530..f08fa30db 100644 --- a/lint/drain_run_stream_before_release_test.go +++ b/lint/drain_run_stream_before_release_test.go @@ -34,6 +34,8 @@ func TestDrainRunStreamBeforeRelease(t *testing.T) { {name: "conditional defer", body: `events := r.RunStream(); if stop { defer func(){ for range events {} }() }; for range events { return }`, want: 1}, {name: "unrelated channel defer", body: `events := r.RunStream(); defer func(){ for range other {} }(); for range events { return }`, want: 1}, {name: "deferred drain can return early", body: `events := r.RunStream(); defer func(){ for range events { return } }(); for range events { return }`, want: 1}, + {name: "deferred drain can break", body: `events := r.RunStream(); defer func(){ for range events { cancel(); break } }(); for range events { return }`, want: 1}, + {name: "deferred drain can nil channel", body: `events := r.RunStream(); defer func(){ events = nil; for range events { cancel() } }(); for range events { return }`, want: 1}, {name: "closure return does not drain owner", body: `events := r.RunStream(); _ = func(){ for range events {} }; for range events { return }`, want: 1}, {name: "shadowed stream", body: `events := r.RunStream(); for range events { events := other; for range events {}; return }`, want: 1}, {name: "select cancellation", body: `events := r.RunStream(); for events != nil { select { case _, ok := <-events: if !ok { events = nil; continue }; if stop { return }; case <-other: return } }`, want: 1}, @@ -51,6 +53,8 @@ func TestDrainRunStreamBeforeRelease(t *testing.T) { {name: "nested loop break", body: `events := r.RunStream(); for range events { for { break } }`}, {name: "nested function return", body: `events := r.RunStream(); for range events { func(){ return }() }`}, {name: "deferred drain", body: `events := r.RunStream(); defer func(){ cancel(); for range events {} }(); for range events { return }`}, + {name: "deferred drain retains results", body: `events := r.RunStream(); defer func(){ cancel(); for event := range events { retain(event) } }(); for range events { return }`}, + {name: "deferred snapshot retains results", body: `events := r.RunStream(); defer func(ch <-chan Event){ cancel(); for event := range ch { retain(event) } }(events); for range events { return }`}, {name: "deferred snapshot", body: `events := r.RunStream(); defer func(ch <-chan Event){ cancel(); for range ch {} }(events); for range events { return }`}, {name: "select until closed", body: `events := r.RunStream(); for events != nil { select { case _, ok := <-events: if !ok { events = nil; continue } } }`}, {name: "return only after closed", body: `events := r.RunStream(); for { _, ok := <-events; if !ok { return } }`}, @@ -84,6 +88,7 @@ func runDrainCop(t *testing.T, filename, body string) []cop.Offense { func (otherRuntime) RunStream() <-chan int { return nil } func cancel() {} func consume(<-chan Event) {} + func retain(Event) {} func run(r Runtime, other <-chan Event, stop bool) { ` + body + ` } ` fset := token.NewFileSet() diff --git a/pkg/acp/agent.go b/pkg/acp/agent.go index 0f6452dd8..ac9e47293 100644 --- a/pkg/acp/agent.go +++ b/pkg/acp/agent.go @@ -814,7 +814,7 @@ func (a *Agent) sendUpdate(ctx context.Context, sessionID string, update acp.Ses } // runAgent runs a single agent loop and streams updates to the ACP client. -func (a *Agent) runAgent(ctx context.Context, acpSess *Session) (acp.StopReason, error) { +func (a *Agent) runAgent(ctx context.Context, acpSess *Session) (stopReason acp.StopReason, retErr error) { slog.DebugContext(ctx, "Running agent turn", "session_id", acpSess.id) ctx = withSessionID(ctx, acpSess.id) @@ -825,17 +825,25 @@ func (a *Agent) runAgent(ctx context.Context, acpSess *Session) (acp.StopReason, runCtx, cancel := context.WithCancel(ctx) eventsChan := acpSess.rt.RunStream(runCtx, acpSess.sess) - // Cancel on handler errors too, before waiting for runtime teardown. + var toolCalls toolCallTracker + // Cancel before draining; final tool updates must precede the prompt response. defer func() { cancel() - for range eventsChan { + for event := range eventsChan { + toolCalls.retainResult(event) + } + cleanupCtx, stopCleanup := context.WithTimeout(context.WithoutCancel(ctx), 2*time.Second) + defer stopCleanup() + updateErr := toolCalls.interrupt(cleanupCtx, a, acpSess) + if retErr == nil && ctx.Err() == nil { + retErr = updateErr } }() - toolCalls := map[string]struct{}{} outcome := promptOutcome{sessionID: acpSess.sess.ID} for event := range eventsChan { if ctx.Err() != nil { + toolCalls.retainResult(event) return "", ctx.Err() } @@ -852,25 +860,23 @@ func (a *Agent) runAgent(ctx context.Context, acpSess *Session) (acp.StopReason, } case *runtime.ToolCallConfirmationEvent: - if err := a.handleToolCallConfirmation(ctx, acpSess, e); err != nil { + state, err := toolCalls.report(ctx, a, acpSess, e.AgentName, e.ToolCall, e.ToolDefinition, acp.ToolCallStatusPending) + if err != nil { + return "", err + } + rejected, err := a.handleToolCallConfirmation(ctx, acpSess, e, state.id) + state.rejected = rejected + if err != nil { return "", err } case *runtime.ToolCallEvent: - toolCalls[e.ToolCall.ID] = struct{}{} - workingDir, _ := acpSess.workspaceSnapshot() - if err := a.sendUpdate(ctx, acpSess.id, buildToolCallStart(e.ToolCall, e.ToolDefinition, workingDir)); err != nil { + if _, err := toolCalls.report(ctx, a, acpSess, e.AgentName, e.ToolCall, e.ToolDefinition, acp.ToolCallStatusInProgress); err != nil { return "", err } case *runtime.ToolCallResponseEvent: - _, ok := toolCalls[e.ToolCallID] - if !ok { - return "", fmt.Errorf("missing tool call arguments for tool call ID %s", e.ToolCallID) - } - delete(toolCalls, e.ToolCallID) - - if err := a.sendUpdate(ctx, acpSess.id, buildToolCallComplete(e)); err != nil { + if err := toolCalls.complete(ctx, a, acpSess, e); err != nil { return "", err } @@ -934,9 +940,10 @@ func (a *Agent) runAgent(ctx context.Context, acpSess *Session) (acp.StopReason, } // handleToolCallConfirmation handles tool call permission requests. -func (a *Agent) handleToolCallConfirmation(ctx context.Context, acpSess *Session, e *runtime.ToolCallConfirmationEvent) error { +func (a *Agent) handleToolCallConfirmation(ctx context.Context, acpSess *Session, e *runtime.ToolCallConfirmationEvent, id acp.ToolCallId) (bool, error) { workingDir, _ := acpSess.workspaceSnapshot() toolCallUpdate := buildToolCallUpdate(e.ToolCall, e.ToolDefinition, acp.ToolCallStatusPending, workingDir) + toolCallUpdate.ToolCallId = id permResp, err := a.conn.RequestPermission(ctx, acp.RequestPermissionRequest{ SessionId: acp.SessionId(acpSess.id), @@ -960,16 +967,16 @@ func (a *Agent) handleToolCallConfirmation(ctx context.Context, acpSess *Session }, }) if err != nil { - return err + return false, err } if permResp.Outcome.Cancelled != nil { acpSess.rt.Resume(ctx, runtime.ResumeRequest{Type: runtime.ResumeTypeReject}) - return nil + return true, nil } if permResp.Outcome.Selected == nil { - return errors.New("unexpected permission outcome") + return false, errors.New("unexpected permission outcome") } switch string(permResp.Outcome.Selected.OptionId) { @@ -979,11 +986,12 @@ func (a *Agent) handleToolCallConfirmation(ctx context.Context, acpSess *Session acpSess.rt.Resume(ctx, runtime.ResumeApproveTool(e.ToolCall.Function.Name)) case "reject": acpSess.rt.Resume(ctx, runtime.ResumeRequest{Type: runtime.ResumeTypeReject}) + return true, nil default: - return fmt.Errorf("unexpected permission option: %s", permResp.Outcome.Selected.OptionId) + return false, fmt.Errorf("unexpected permission option: %s", permResp.Outcome.Selected.OptionId) } - return nil + return false, nil } // handleMaxIterationsReached handles max iterations events. diff --git a/pkg/acp/elicitation_test.go b/pkg/acp/elicitation_test.go index 8b1283b8d..e8ae1c5f1 100644 --- a/pkg/acp/elicitation_test.go +++ b/pkg/acp/elicitation_test.go @@ -180,7 +180,8 @@ func TestPromptDeclinesUnsupportedElicitation(t *testing.T) { assert.Equal(t, "Done", s.sess.GetLastAssistantMessageContent()) reqs := peer.recordedRequests() require.Len(t, reqs, (turn+1)*2, "only permission and max-iteration requests should reach the client") - assert.Equal(t, acpsdk.ToolCallId("call-1"), reqs[turn*2].ToolCall.ToolCallId) + assert.NotEmpty(t, reqs[turn*2].ToolCall.ToolCallId) + assert.Equal(t, "ask_user", *reqs[turn*2].ToolCall.Title) assert.Equal(t, acpsdk.ToolCallId("max_iterations"), reqs[turn*2+1].ToolCall.ToolCallId) } assert.Equal(t, 2, loads) diff --git a/pkg/acp/filediff_test.go b/pkg/acp/filediff_test.go index 6b9961cf0..5344a48fb 100644 --- a/pkg/acp/filediff_test.go +++ b/pkg/acp/filediff_test.go @@ -300,7 +300,7 @@ func TestRuntimeEditSnapshotSurvivesOutputTransform(t *testing.T) { Params acpsdk.SessionNotification `json:"params"` } require.NoError(t, json.Unmarshal([]byte(line), &message)) - if update := message.Params.Update.ToolCallUpdate; update != nil && update.ToolCallId == "edit" { + if update := message.Params.Update.ToolCallUpdate; update != nil && update.Status != nil && *update.Status == acpsdk.ToolCallStatusCompleted { require.Len(t, update.Content, 2) diff = update.Content[1].Diff assert.Equal(t, "transformed response", update.Content[0].Content.Content.Text.Text) diff --git a/pkg/acp/runagent_drain_test.go b/pkg/acp/runagent_drain_test.go index f5c300d18..0f02a0fef 100644 --- a/pkg/acp/runagent_drain_test.go +++ b/pkg/acp/runagent_drain_test.go @@ -29,7 +29,7 @@ func (r *drainingPromptRuntime) RunStream(ctx context.Context, _ *session.Sessio defer close(events) defer close(r.stopped) if r.fail { - events <- &runtime.ToolCallResponseEvent{ToolCallID: "missing-start"} + events <- &runtime.ToolCallResponseEvent{ToolCallID: ""} } <-ctx.Done() events <- runtime.Warning("teardown started", "root") @@ -67,7 +67,7 @@ func TestPrompt_DrainsBeforeReleasingTurn(t *testing.T) { synctest.Wait() result := <-done if fail { - require.ErrorContains(t, result.err, "missing tool call arguments") + require.ErrorContains(t, result.err, "tool call ID is required") } else { require.NoError(t, result.err) assert.Equal(t, acpsdk.StopReasonCancelled, result.response.StopReason) diff --git a/pkg/acp/runagent_test.go b/pkg/acp/runagent_test.go index a85543fda..62636beb6 100644 --- a/pkg/acp/runagent_test.go +++ b/pkg/acp/runagent_test.go @@ -15,6 +15,7 @@ import ( "sync" "sync/atomic" "testing" + "testing/synctest" "time" acpsdk "github.com/coder/acp-go-sdk" @@ -806,16 +807,16 @@ func TestRunAgent_ToolCallLifecycle(t *testing.T) { start := updates[1].ToolCall require.NotNil(t, start) - assert.Equal(t, acpsdk.ToolCallId("call-1"), start.ToolCallId) + assert.NotEmpty(t, start.ToolCallId) assert.Equal(t, "Run Shell", start.Title) assert.Equal(t, acpsdk.ToolKindExecute, start.Kind) - assert.Equal(t, acpsdk.ToolCallStatusPending, start.Status) + assert.Equal(t, acpsdk.ToolCallStatusInProgress, start.Status) assert.Equal(t, map[string]any{"command": "ls", "path": "."}, start.RawInput) assert.Equal(t, []acpsdk.ToolCallLocation{{Path: wd}}, start.Locations) completed := updates[2].ToolCallUpdate require.NotNil(t, completed) - assert.Equal(t, acpsdk.ToolCallId("call-1"), completed.ToolCallId) + assert.Equal(t, start.ToolCallId, completed.ToolCallId) require.NotNil(t, completed.Status) assert.Equal(t, acpsdk.ToolCallStatusCompleted, *completed.Status) require.Len(t, completed.Content, 1) @@ -827,7 +828,7 @@ func TestRunAgent_ToolCallLifecycle(t *testing.T) { require.NotNil(t, updates[3].ToolCall) failed := updates[4].ToolCallUpdate require.NotNil(t, failed) - assert.Equal(t, acpsdk.ToolCallId("call-2"), failed.ToolCallId) + assert.Equal(t, updates[3].ToolCall.ToolCallId, failed.ToolCallId) require.NotNil(t, failed.Status) assert.Equal(t, acpsdk.ToolCallStatusFailed, *failed.Status) @@ -850,22 +851,29 @@ func TestRunAgent_ToolCallLifecycle(t *testing.T) { assert.Empty(t, rt.resumeRequests()) } -func TestRunAgent_ToolCallResponseWithoutStartFails(t *testing.T) { +func TestRunAgent_ToolCallResponseWithoutStartCreatesTerminalCall(t *testing.T) { t.Parallel() - - rt := &fakeRuntime{events: []runtime.Event{ - runtime.ToolCallResponse("orphan", tools.Tool{Name: "shell"}, tools.ResultSuccess("ok"), "ok", "root"), - runtime.AgentChoice("root", testSessionID, "never emitted"), - }} - f := newRunAgentFixture(t, rt, &captureWriter{}) - - err := f.runAgent(t.Context(), f.sess) - require.EqualError(t, err, "missing tool call arguments for tool call ID orphan") - - // The failure must stop the loop before later events are mapped. - updates := f.sessionUpdates(t) - require.Len(t, updates, 1) - requireAvailableCommands(t, updates[0]) + for _, failed := range []bool{false, true} { + result := tools.ResultSuccess("result") + status := acpsdk.ToolCallStatusCompleted + if failed { + result = tools.ResultError("rejected") + status = acpsdk.ToolCallStatusFailed + } + rt := &fakeRuntime{events: []runtime.Event{ + runtime.ToolCallResponse("orphan", tools.Tool{Name: "shell"}, result, result.Output, "root"), + runtime.AgentChoice("root", testSessionID, "continued"), + }} + f := newRunAgentFixture(t, rt, &captureWriter{}) + require.NoError(t, f.runAgent(t.Context(), f.sess)) + updates := f.sessionUpdates(t) + require.Len(t, updates, 3) + require.NotNil(t, updates[1].ToolCall) + assert.Equal(t, status, updates[1].ToolCall.Status) + assert.Equal(t, "shell", updates[1].ToolCall.Title) + assert.Nil(t, updates[1].ToolCall.RawInput) + assert.Equal(t, "continued", agentMessageText(t, updates[2])) + } } func TestRunAgent_ToolCallConfirmationRequestFields(t *testing.T) { @@ -890,7 +898,7 @@ func TestRunAgent_ToolCallConfirmationRequestFields(t *testing.T) { require.Len(t, reqs, 1) req := reqs[0] assert.Equal(t, acpsdk.SessionId(testSessionID), req.SessionId) - assert.Equal(t, acpsdk.ToolCallId("confirm-1"), req.ToolCall.ToolCallId) + assert.NotEmpty(t, req.ToolCall.ToolCallId) require.NotNil(t, req.ToolCall.Title) assert.Equal(t, "Run Shell", *req.ToolCall.Title) require.NotNil(t, req.ToolCall.Kind) @@ -909,9 +917,14 @@ func TestRunAgent_ToolCallConfirmationRequestFields(t *testing.T) { // The turn keeps streaming after the permission round trip and the // interleaved request does not disturb the captured notifications. updates := f.sessionUpdates(t) - require.Len(t, updates, 2) + require.Len(t, updates, 4) requireAvailableCommands(t, updates[0]) - assert.Equal(t, "approved", agentMessageText(t, updates[1])) + require.NotNil(t, updates[1].ToolCall) + assert.Equal(t, req.ToolCall.ToolCallId, updates[1].ToolCall.ToolCallId) + assert.Equal(t, acpsdk.ToolCallStatusPending, updates[1].ToolCall.Status) + assert.Equal(t, "approved", agentMessageText(t, updates[2])) + assert.Equal(t, req.ToolCall.ToolCallId, updates[3].ToolCallUpdate.ToolCallId) + assert.Equal(t, acpsdk.ToolCallStatusFailed, *updates[3].ToolCallUpdate.Status) } func TestRunAgent_ToolCallConfirmationOutcomes(t *testing.T) { @@ -1001,48 +1014,52 @@ func (p *approvalTestProvider) CreateChatCompletionStream(context.Context, []cha func TestRunAgent_AlwaysAllowKeepsOtherToolsGated(t *testing.T) { t.Parallel() - var executed []string - handler := func(_ context.Context, call tools.ToolCall, _ tools.Runtime) (*tools.ToolCallResult, error) { - executed = append(executed, call.Function.Name) - return tools.ResultSuccess("ok"), nil - } - prov := &approvalTestProvider{ - mockProvider: mockProvider{id: modelsdev.NewID("test", "approval")}, - toolNames: []string{"allowed_tool", "allowed_tool", "other_tool"}, - } - root := agentpkg.New("root", "You are a test agent", agentpkg.WithModel(prov), agentpkg.WithTools( - tools.Tool{Name: "allowed_tool", Parameters: map[string]any{"type": "object"}, Handler: handler}, - tools.Tool{Name: "other_tool", Parameters: map[string]any{"type": "object"}, Handler: handler}, - )) - rt, err := runtime.New(t.Context(), team.New(team.WithAgents(root)), runtime.WithSessionCompaction(false)) - require.NoError(t, err) - t.Cleanup(func() { require.NoError(t, rt.Close()) }) - - f := newRunAgentFixtureWithPermissions(t, &fakeRuntime{}, &captureWriter{}, func(req acpsdk.RequestPermissionRequest) any { - if req.ToolCall.ToolCallId == "call-1" { - return permissionSelected("allow-always") + synctest.Test(t, func(t *testing.T) { + var executed []string + handler := func(_ context.Context, call tools.ToolCall, _ tools.Runtime) (*tools.ToolCallResult, error) { + executed = append(executed, call.Function.Name) + return tools.ResultSuccess("ok"), nil } - return permissionSelected("allow") + prov := &approvalTestProvider{ + mockProvider: mockProvider{id: modelsdev.NewID("test", "approval")}, + toolNames: []string{"allowed_tool", "allowed_tool", "other_tool"}, + } + root := agentpkg.New("root", "You are a test agent", agentpkg.WithModel(prov), agentpkg.WithTools( + tools.Tool{Name: "allowed_tool", Parameters: map[string]any{"type": "object"}, Handler: handler}, + tools.Tool{Name: "other_tool", Parameters: map[string]any{"type": "object"}, Handler: handler}, + )) + rt, err := runtime.New(t.Context(), team.New(team.WithAgents(root)), runtime.WithSessionCompaction(false)) + require.NoError(t, err) + t.Cleanup(func() { require.NoError(t, rt.Close()) }) + + f := newRunAgentFixtureWithPermissions(t, &fakeRuntime{}, &captureWriter{}, func(req acpsdk.RequestPermissionRequest) any { + synctest.Wait() + if req.ToolCall.Title != nil && *req.ToolCall.Title == "allowed_tool" { + return permissionSelected("allow-always") + } + return permissionSelected("allow") + }) + f.sess.rt = rt + f.sess.sess = session.New( + session.WithUserMessage("Run the tools"), + session.WithSafetyPolicy(session.SafetyPolicyStrict), + ) + + ctx, cancel := context.WithTimeout(t.Context(), 5*time.Second) + defer cancel() + require.NoError(t, f.runAgent(ctx, f.sess)) + + assert.Equal(t, []string{"allowed_tool", "allowed_tool", "other_tool"}, executed) + reqs := f.peer.recordedRequests() + require.Len(t, reqs, 2) + assert.Equal(t, "allowed_tool", *reqs[0].ToolCall.Title) + assert.Equal(t, "other_tool", *reqs[1].ToolCall.Title) + assert.NotEqual(t, reqs[0].ToolCall.ToolCallId, reqs[1].ToolCall.ToolCallId) + assert.Equal(t, session.SafetyPolicyStrict, f.sess.sess.GetSafetyPolicy()) + perms := f.sess.sess.ClonePermissions() + require.NotNil(t, perms) + assert.Equal(t, []string{"allowed_tool"}, perms.Allow) }) - f.sess.rt = rt - f.sess.sess = session.New( - session.WithUserMessage("Run the tools"), - session.WithSafetyPolicy(session.SafetyPolicyStrict), - ) - - ctx, cancel := context.WithTimeout(t.Context(), 5*time.Second) - defer cancel() - require.NoError(t, f.runAgent(ctx, f.sess)) - - assert.Equal(t, []string{"allowed_tool", "allowed_tool", "other_tool"}, executed) - reqs := f.peer.recordedRequests() - require.Len(t, reqs, 2) - assert.Equal(t, acpsdk.ToolCallId("call-1"), reqs[0].ToolCall.ToolCallId) - assert.Equal(t, acpsdk.ToolCallId("call-3"), reqs[1].ToolCall.ToolCallId) - assert.Equal(t, session.SafetyPolicyStrict, f.sess.sess.GetSafetyPolicy()) - perms := f.sess.sess.ClonePermissions() - require.NotNil(t, perms) - assert.Equal(t, []string{"allowed_tool"}, perms.Allow) } func TestRunAgent_ToolCallConfirmationBadOutcomeFailsRun(t *testing.T) { @@ -1089,8 +1106,12 @@ func TestRunAgent_ToolCallConfirmationBadOutcomeFailsRun(t *testing.T) { assert.Empty(t, rt.resumeRequests()) // The failure must stop the loop before later events are mapped. updates := f.sessionUpdates(t) - require.Len(t, updates, 1) + require.Len(t, updates, 3) requireAvailableCommands(t, updates[0]) + require.NotNil(t, updates[1].ToolCall) + assert.Equal(t, acpsdk.ToolCallStatusPending, updates[1].ToolCall.Status) + require.NotNil(t, updates[2].ToolCallUpdate) + assert.Equal(t, acpsdk.ToolCallStatusFailed, *updates[2].ToolCallUpdate.Status) }) } } diff --git a/pkg/acp/tool_lifecycle.go b/pkg/acp/tool_lifecycle.go new file mode 100644 index 000000000..c7191df28 --- /dev/null +++ b/pkg/acp/tool_lifecycle.go @@ -0,0 +1,125 @@ +package acp + +import ( + "context" + "errors" + "time" + + "github.com/coder/acp-go-sdk" + "github.com/google/uuid" + + "github.com/docker/docker-agent/pkg/runtime" + "github.com/docker/docker-agent/pkg/tools" +) + +type toolCallKey struct { + agent string + id string +} + +type toolCallState struct { + id acp.ToolCallId + rejected bool + result *runtime.ToolCallResponseEvent +} + +type toolCallTracker struct { + active map[toolCallKey]*toolCallState +} + +func (t *toolCallTracker) report(ctx context.Context, a *Agent, s *Session, agentName string, call tools.ToolCall, definition tools.Tool, status acp.ToolCallStatus) (*toolCallState, error) { + if call.ID == "" { + return nil, errors.New("tool call ID is required") + } + key := toolCallKey{agent: agentName, id: call.ID} + state, exists := t.active[key] + if exists && state.rejected && status == acp.ToolCallStatusInProgress { + // Out-of-band rejections can synthesize a start without executing anything. + return state, nil + } + if !exists { + state = &toolCallState{id: acp.ToolCallId(uuid.NewString())} + if t.active == nil { + t.active = make(map[toolCallKey]*toolCallState) + } + } + call.ID = string(state.id) + workingDir, _ := s.workspaceSnapshot() + var update acp.SessionUpdate + if exists { + fields := buildToolCallUpdate(call, definition, status, workingDir) + update = acp.UpdateToolCall(state.id, + acp.WithUpdateTitle(*fields.Title), acp.WithUpdateKind(*fields.Kind), + acp.WithUpdateStatus(status), acp.WithUpdateRawInput(fields.RawInput), acp.WithUpdateLocations(fields.Locations), + ) + } else { + update = buildToolCallStart(call, definition, workingDir) + update.ToolCall.Status = status + } + if err := a.sendUpdate(ctx, s.id, update); err != nil { + return nil, err + } + t.active[key] = state + return state, nil +} + +func (t *toolCallTracker) complete(ctx context.Context, a *Agent, s *Session, event *runtime.ToolCallResponseEvent) error { + if event.ToolCallID == "" { + return errors.New("tool call ID is required") + } + key := toolCallKey{agent: event.AgentName, id: event.ToolCallID} + state, exists := t.active[key] + if !exists { + state = &toolCallState{id: acp.ToolCallId(uuid.NewString())} + } + mappedEvent := *event + mappedEvent.ToolCallID = string(state.id) + update := buildToolCallComplete(&mappedEvent) + if !exists { + fields := update.ToolCallUpdate + title := event.ToolDefinition.Annotations.Title + if title == "" { + title = event.ToolDefinition.Name + } + if title == "" { + title = "Tool call" + } + update = acp.StartToolCall(state.id, title, + acp.WithStartKind(determineToolKind(event.ToolDefinition.Name, event.ToolDefinition)), + acp.WithStartStatus(*fields.Status), acp.WithStartContent(fields.Content), acp.WithStartRawOutput(fields.RawOutput), + ) + } + // Never retry a terminal write: a writer error can mean partial delivery. + delete(t.active, key) + sendCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), 2*time.Second) + defer cancel() + return a.sendUpdate(sendCtx, s.id, update) +} + +func (t *toolCallTracker) retainResult(event runtime.Event) { + if response, ok := event.(*runtime.ToolCallResponseEvent); ok { + if state := t.active[toolCallKey{agent: response.AgentName, id: response.ToolCallID}]; state != nil { + state.result = response + } + } +} + +func (t *toolCallTracker) interrupt(ctx context.Context, a *Agent, s *Session) error { + for key, state := range t.active { + if state.result != nil { + if err := t.complete(ctx, a, s, state.result); err != nil { + return err + } + continue + } + update := acp.UpdateToolCall(state.id, + acp.WithUpdateStatus(acp.ToolCallStatusFailed), + acp.WithUpdateContent([]acp.ToolCallContent{acp.ToolContent(acp.TextBlock("Tool call interrupted before a result was available; side effects may have occurred."))}), + ) + if err := a.sendUpdate(ctx, s.id, update); err != nil { + return err + } + delete(t.active, key) + } + return nil +} diff --git a/pkg/acp/tool_lifecycle_test.go b/pkg/acp/tool_lifecycle_test.go new file mode 100644 index 000000000..09bf06bf0 --- /dev/null +++ b/pkg/acp/tool_lifecycle_test.go @@ -0,0 +1,358 @@ +package acp + +import ( + "context" + "io" + "path/filepath" + "strings" + "testing" + "testing/synctest" + "time" + + acpsdk "github.com/coder/acp-go-sdk" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/docker/docker-agent/pkg/agent" + "github.com/docker/docker-agent/pkg/modelsdev" + "github.com/docker/docker-agent/pkg/runtime" + "github.com/docker/docker-agent/pkg/session" + "github.com/docker/docker-agent/pkg/team" + "github.com/docker/docker-agent/pkg/tools" +) + +func toolUpdates(updates []acpsdk.SessionUpdate) []acpsdk.SessionUpdate { + var result []acpsdk.SessionUpdate + for _, update := range updates { + if update.ToolCall != nil || update.ToolCallUpdate != nil { + result = append(result, update) + } + } + return result +} + +func TestToolLifecyclePendingRunningCompletion(t *testing.T) { + t.Parallel() + wd := t.TempDir() + call := tools.ToolCall{ID: "runtime-id", Function: tools.FunctionCall{Name: "edit_file", Arguments: `{"file":"before.txt"}`}} + changed := call + changed.Function.Arguments = `{"file":"after.txt"}` + tool := tools.Tool{Name: "edit_file", Annotations: tools.ToolAnnotations{Title: "Edit", DestructiveHint: new(true)}} + rt := &fakeRuntime{events: []runtime.Event{ + runtime.ToolCallConfirmation(call, tool, "root", nil), + runtime.ToolCall(changed, tool, "root"), + runtime.ToolCallResponse(call.ID, tool, tools.ResultSuccess("done"), "done", "root"), + }} + f := newRunAgentFixtureWithPermissions(t, rt, &captureWriter{}, func(acpsdk.RequestPermissionRequest) any { return permissionSelected("allow") }) + f.sess.workingDir = wd + require.NoError(t, f.runAgent(t.Context(), f.sess)) + updates := toolUpdates(f.sessionUpdates(t)) + require.Len(t, updates, 3) + pending, running, finished := updates[0].ToolCall, updates[1].ToolCallUpdate, updates[2].ToolCallUpdate + require.NotNil(t, pending) + require.NotNil(t, running) + require.NotNil(t, finished) + assert.Equal(t, acpsdk.ToolCallStatusPending, pending.Status) + assert.Equal(t, acpsdk.ToolCallStatusInProgress, *running.Status) + assert.Equal(t, acpsdk.ToolCallStatusCompleted, *finished.Status) + assert.Equal(t, pending.ToolCallId, running.ToolCallId) + assert.Equal(t, pending.ToolCallId, finished.ToolCallId) + req := f.peer.recordedRequests()[0] + assert.Equal(t, pending.ToolCallId, req.ToolCall.ToolCallId) + assert.Equal(t, acpsdk.ToolKindEdit, pending.Kind) + assert.Equal(t, pending.Kind, *req.ToolCall.Kind) + assert.Equal(t, pending.Kind, *running.Kind) + assert.Equal(t, map[string]any{"file": "before.txt"}, pending.RawInput) + assert.Equal(t, pending.RawInput, req.ToolCall.RawInput) + assert.Equal(t, map[string]any{"file": "after.txt"}, running.RawInput) + assert.Equal(t, filepath.Join(wd, "after.txt"), running.Locations[0].Path) +} + +func TestToolLifecycleSuppressesRejectedSyntheticStart(t *testing.T) { + t.Parallel() + for _, outcome := range []string{"reject", "cancelled"} { + t.Run(outcome, func(t *testing.T) { + call := tools.ToolCall{ID: "nested", Function: tools.FunctionCall{Name: "shell", Arguments: `{"cmd":"never execute"}`}} + tool := tools.Tool{Name: "shell"} + rt := &fakeRuntime{events: []runtime.Event{ + runtime.ToolCallConfirmation(call, tool, "root", nil), + runtime.ToolCall(call, tool, "root"), + runtime.ToolCallResponse(call.ID, tool, tools.ResultError("rejected"), "rejected", "root"), + }} + f := newRunAgentFixtureWithPermissions(t, rt, &captureWriter{}, func(acpsdk.RequestPermissionRequest) any { + if outcome == "cancelled" { + return map[string]any{"outcome": map[string]any{"outcome": "cancelled"}} + } + return permissionSelected("reject") + }) + require.NoError(t, f.runAgent(t.Context(), f.sess)) + updates := toolUpdates(f.sessionUpdates(t)) + require.Len(t, updates, 2) + assert.Equal(t, acpsdk.ToolCallStatusPending, updates[0].ToolCall.Status) + assert.Equal(t, acpsdk.ToolCallStatusFailed, *updates[1].ToolCallUpdate.Status) + assert.Equal(t, updates[0].ToolCall.ToolCallId, updates[1].ToolCallUpdate.ToolCallId) + assert.Equal(t, []runtime.ResumeRequest{{Type: runtime.ResumeTypeReject}}, rt.resumeRequests()) + }) + } +} + +func TestToolLifecycleScopesAndReusesRuntimeIDs(t *testing.T) { + t.Parallel() + call := tools.ToolCall{ID: "same", Function: tools.FunctionCall{Name: "shell", Arguments: "{}"}} + tool := tools.Tool{Name: "shell"} + rt := &fakeRuntime{events: []runtime.Event{ + runtime.ToolCall(call, tool, "root"), runtime.ToolCall(call, tool, "worker"), + runtime.ToolCallResponse("same", tool, tools.ResultSuccess("worker"), "worker", "worker"), + runtime.ToolCallResponse("same", tool, tools.ResultSuccess("root"), "root", "root"), + runtime.ToolCall(call, tool, "root"), runtime.ToolCallResponse("same", tool, tools.ResultSuccess("reused"), "reused", "root"), + }} + f := newRunAgentFixture(t, rt, &captureWriter{}) + require.NoError(t, f.runAgent(t.Context(), f.sess)) + updates := toolUpdates(f.sessionUpdates(t)) + require.Len(t, updates, 6) + root, worker, reused := updates[0].ToolCall.ToolCallId, updates[1].ToolCall.ToolCallId, updates[4].ToolCall.ToolCallId + assert.NotEqual(t, root, worker) + assert.NotEqual(t, root, reused) + assert.NotEqual(t, worker, reused) + assert.Equal(t, worker, updates[2].ToolCallUpdate.ToolCallId) + assert.Equal(t, root, updates[3].ToolCallUpdate.ToolCallId) + assert.Equal(t, reused, updates[5].ToolCallUpdate.ToolCallId) + require.NoError(t, f.runAgent(t.Context(), f.sess)) + all := toolUpdates(f.sessionUpdates(t)) + assert.NotEqual(t, root, all[6].ToolCall.ToolCallId) +} + +func TestToolLifecycleRuntimeRejectAndPolicyDeny(t *testing.T) { + t.Parallel() + for _, mode := range []string{"allow", "reject", "policy deny", "unavailable"} { + t.Run(mode, func(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + var executed bool + tool := tools.Tool{Name: "change", Parameters: map[string]any{"type": "object"}, Handler: func(context.Context, tools.ToolCall, tools.Runtime) (*tools.ToolCallResult, error) { + executed = true + return tools.ResultSuccess("changed"), nil + }} + prov := &elicitationTestProvider{approvalTestProvider: approvalTestProvider{mockProvider: mockProvider{id: modelsdev.NewID("test", "lifecycle")}, toolNames: []string{"change"}}} + root := agent.New("root", "test", agent.WithModel(prov)) + if mode != "unavailable" { + agent.WithTools(tool)(root) + } + rt, err := runtime.New(t.Context(), team.New(team.WithAgents(root)), runtime.WithSessionCompaction(false)) + require.NoError(t, err) + t.Cleanup(func() { require.NoError(t, rt.Close()) }) + f := newRunAgentFixtureWithPermissions(t, &fakeRuntime{}, &captureWriter{}, func(acpsdk.RequestPermissionRequest) any { synctest.Wait(); return permissionSelected(mode) }) + f.sess.rt = rt + f.sess.sess.SetSafetyPolicy(session.SafetyPolicyStrict) + if mode == "policy deny" { + f.sess.sess.Permissions = &session.PermissionsConfig{Deny: []string{"change"}} + } + f.agent.sessions[testSessionID] = f.sess + response, err := f.agent.Prompt(t.Context(), promptRequest("do work")) + require.NoError(t, err) + assert.Equal(t, acpsdk.StopReasonEndTurn, response.StopReason) + assert.Equal(t, "Done", f.sess.sess.GetLastAssistantMessageContent()) + assert.Equal(t, mode == "allow", executed) + updates := toolUpdates(f.sessionUpdates(t)) + switch mode { + case "allow": + require.Len(t, updates, 3) + assert.Equal(t, acpsdk.ToolCallStatusInProgress, *updates[1].ToolCallUpdate.Status) + assert.Equal(t, acpsdk.ToolCallStatusCompleted, *updates[2].ToolCallUpdate.Status) + case "reject": + require.Len(t, updates, 2) + assert.Equal(t, acpsdk.ToolCallStatusFailed, *updates[1].ToolCallUpdate.Status) + default: + require.Len(t, updates, 1) + assert.Equal(t, acpsdk.ToolCallStatusFailed, updates[0].ToolCall.Status) + assert.Nil(t, updates[0].ToolCall.RawInput) + } + }) + }) + } +} + +type lateToolResultRuntime struct { + fakeRuntime + + started chan struct{} + resultReady chan struct{} + resultSent chan struct{} + finish chan struct{} +} + +func (r *lateToolResultRuntime) RunStream(ctx context.Context, _ *session.Session) <-chan runtime.Event { + events := make(chan runtime.Event) + go func() { + defer close(events) + call := tools.ToolCall{ID: "call", Function: tools.FunctionCall{Name: "shell", Arguments: "{}"}} + events <- runtime.ToolCall(call, tools.Tool{Name: "shell"}, "root") + close(r.started) + <-r.resultReady + events <- runtime.ToolCallResponse("call", tools.Tool{Name: "shell"}, tools.ResultSuccess("actual result"), "actual result", "root") + close(r.resultSent) + <-r.finish + }() + return events +} + +func TestCanceledPromptPreservesLateToolResultAfterSlowDrain(t *testing.T) { + t.Parallel() + synctest.Test(t, func(t *testing.T) { + rt := &lateToolResultRuntime{started: make(chan struct{}), resultReady: make(chan struct{}), resultSent: make(chan struct{}), finish: make(chan struct{})} + a, _, peer := newPromptTestAgent(t, rt) + done := promptAsync(a, t.Context(), promptRequest("work")) + <-rt.started + synctest.Wait() + require.NoError(t, a.Cancel(t.Context(), acpsdk.CancelNotification{SessionId: testSessionID})) + close(rt.resultReady) + <-rt.resultSent + // Runtime teardown may outlive the final-notification attempt budget. + time.Sleep(3 * time.Second) //nolint:forbidigo // fake time advances a deliberately slow teardown + close(rt.finish) + result := <-done + require.NoError(t, result.err) + assert.Equal(t, acpsdk.StopReasonCancelled, result.response.StopReason) + out := peer.out.(*captureWriter) + assert.Contains(t, strings.Join(out.lines(), "\n"), `"status":"completed"`) + assert.Contains(t, strings.Join(out.lines(), "\n"), "actual result") + assert.NotContains(t, strings.Join(out.lines(), "\n"), "interrupted") + }) +} + +func TestToolCompletionSurvivesCancellationBeforeSend(t *testing.T) { + t.Parallel() + f := newRunAgentFixture(t, &fakeRuntime{}, &captureWriter{}) + var tracker toolCallTracker + call := tools.ToolCall{ID: "call", Function: tools.FunctionCall{Name: "shell", Arguments: "{}"}} + _, err := tracker.report(t.Context(), f.agent, f.sess, "root", call, tools.Tool{Name: "shell"}, acpsdk.ToolCallStatusInProgress) + require.NoError(t, err) + ctx, cancel := context.WithCancel(t.Context()) + cancel() + err = tracker.complete(ctx, f.agent, f.sess, runtime.ToolCallResponse("call", tools.Tool{Name: "shell"}, tools.ResultSuccess("actual result"), "actual result", "root").(*runtime.ToolCallResponseEvent)) + require.NoError(t, err) + updates := toolUpdates(f.sessionUpdates(t)) + require.Len(t, updates, 2) + assert.Equal(t, acpsdk.ToolCallStatusCompleted, *updates[1].ToolCallUpdate.Status) + assert.Empty(t, tracker.active) +} + +func TestToolCompletionWriteFailureIsNotRetried(t *testing.T) { + t.Parallel() + out := &captureWriter{failOn: func(n int) error { + if n == 2 { + return io.ErrClosedPipe + } + return nil + }} + f := newRunAgentFixture(t, &fakeRuntime{}, out) + var tracker toolCallTracker + call := tools.ToolCall{ID: "call", Function: tools.FunctionCall{Name: "shell", Arguments: "{}"}} + _, err := tracker.report(t.Context(), f.agent, f.sess, "root", call, tools.Tool{Name: "shell"}, acpsdk.ToolCallStatusInProgress) + require.NoError(t, err) + err = tracker.complete(t.Context(), f.agent, f.sess, runtime.ToolCallResponse("call", tools.Tool{Name: "shell"}, tools.ResultSuccess("done"), "done", "root").(*runtime.ToolCallResponseEvent)) + require.Error(t, err) + require.NoError(t, tracker.interrupt(t.Context(), f.agent, f.sess)) + assert.Equal(t, 2, out.writes) +} + +type interruptedToolRuntime struct { + fakeRuntime + + started chan struct{} + release chan struct{} + fail bool +} + +func (r *interruptedToolRuntime) RunStream(ctx context.Context, _ *session.Session) <-chan runtime.Event { + events := make(chan runtime.Event) + go func() { + defer close(events) + call := tools.ToolCall{ID: "call", Function: tools.FunctionCall{Name: "shell", Arguments: "{}"}} + events <- runtime.ToolCall(call, tools.Tool{Name: "shell"}, "root") + close(r.started) + if r.fail { + events <- &runtime.ToolCallResponseEvent{} + } + <-ctx.Done() + <-r.release + }() + return events +} + +func TestInterruptedToolWithoutResultClosesAfterDrain(t *testing.T) { + t.Parallel() + for _, failure := range []bool{false, true} { + name := "cancellation" + if failure { + name = "handler error" + } + t.Run(name, func(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + rt := &interruptedToolRuntime{started: make(chan struct{}), release: make(chan struct{}), fail: failure} + a, _, peer := newPromptTestAgent(t, rt) + done := promptAsync(a, t.Context(), promptRequest("work")) + <-rt.started + synctest.Wait() + if !failure { + require.NoError(t, a.Cancel(t.Context(), acpsdk.CancelNotification{SessionId: testSessionID})) + } + synctest.Wait() + select { + case <-done: + t.Fatal("prompt returned before runtime teardown") + default: + } + out := peer.out.(*captureWriter) + assert.NotContains(t, strings.Join(out.lines(), "\n"), `"status":"failed"`) + close(rt.release) + result := <-done + if failure { + require.EqualError(t, result.err, "tool call ID is required") + } else { + require.NoError(t, result.err) + assert.Equal(t, acpsdk.StopReasonCancelled, result.response.StopReason) + } + f := &runAgentFixture{out: out} + updates := toolUpdates(f.sessionUpdates(t)) + require.Len(t, updates, 2) + assert.Equal(t, acpsdk.ToolCallStatusInProgress, updates[0].ToolCall.Status) + assert.Equal(t, updates[0].ToolCall.ToolCallId, updates[1].ToolCallUpdate.ToolCallId) + assert.Equal(t, acpsdk.ToolCallStatusFailed, *updates[1].ToolCallUpdate.Status) + assert.Contains(t, updates[1].ToolCallUpdate.Content[0].Content.Content.Text.Text, "side effects may have occurred") + }) + }) + } +} + +func TestToolLifecycleDoesNotExposePartialArgumentsOrOutput(t *testing.T) { + t.Parallel() + call := tools.ToolCall{ID: "call", Function: tools.FunctionCall{Name: "shell", Arguments: `{"secret":"partial`}} + rt := &fakeRuntime{events: []runtime.Event{ + runtime.PartialToolCall(call, tools.Tool{Name: "shell"}, "root"), + runtime.ToolCallOutput(call.ID, tools.Tool{Name: "shell"}, "unbuffered-secret", "root"), + }} + f := newRunAgentFixture(t, rt, &captureWriter{}) + require.NoError(t, f.runAgent(t.Context(), f.sess)) + assert.Empty(t, toolUpdates(f.sessionUpdates(t))) + assert.NotContains(t, strings.Join(f.out.lines(), "\n"), "secret") +} + +func TestToolLifecycleRepeatedStartUpdatesExistingCall(t *testing.T) { + t.Parallel() + call := tools.ToolCall{ID: "call", Function: tools.FunctionCall{Name: "shell", Arguments: "{}"}} + tool := tools.Tool{Name: "shell"} + start := runtime.ToolCall(call, tool, "root") + response := runtime.ToolCallResponse("call", tool, tools.ResultSuccess("private result"), "transformed result", "root") + rt := &fakeRuntime{events: []runtime.Event{start, start, response}} + f := newRunAgentFixture(t, rt, &captureWriter{}) + require.NoError(t, f.runAgent(t.Context(), f.sess)) + updates := toolUpdates(f.sessionUpdates(t)) + require.Len(t, updates, 3) + assert.Equal(t, updates[0].ToolCall.ToolCallId, updates[1].ToolCallUpdate.ToolCallId) + assert.Equal(t, updates[0].ToolCall.ToolCallId, updates[2].ToolCallUpdate.ToolCallId) + assert.Equal(t, acpsdk.ToolCallStatusInProgress, *updates[1].ToolCallUpdate.Status) + assert.Equal(t, map[string]any{"content": "transformed result"}, updates[2].ToolCallUpdate.RawOutput) + assert.NotContains(t, strings.Join(f.out.lines(), "\n"), "private result") + assert.Equal(t, "call", start.(*runtime.ToolCallEvent).ToolCall.ID) + assert.Equal(t, "call", response.(*runtime.ToolCallResponseEvent).ToolCallID) +} diff --git a/pkg/acp/toolcall.go b/pkg/acp/toolcall.go index ba6719a55..079fc340a 100644 --- a/pkg/acp/toolcall.go +++ b/pkg/acp/toolcall.go @@ -29,12 +29,13 @@ func buildToolCallStart(toolCall tools.ToolCall, tool tools.Tool, workingDir str opts := []acp.ToolCallStartOpt{ acp.WithStartKind(kind), - acp.WithStartStatus(acp.ToolCallStatusPending), + acp.WithStartStatus(acp.ToolCallStatusInProgress), acp.WithStartRawInput(args), } if len(locations) > 0 { - opts = append(opts, acp.WithStartLocations(locations)) + // WithStartLocations also injects rawInput.path, changing non-path arguments. + opts = append(opts, func(call *acp.SessionUpdateToolCall) { call.Locations = locations }) } return acp.StartToolCall( @@ -68,13 +69,9 @@ func buildToolCallComplete(event *runtime.ToolCallResponseEvent) acp.SessionUpda // buildToolCallUpdate creates a tool call update for permission requests. func buildToolCallUpdate(toolCall tools.ToolCall, tool tools.Tool, status acp.ToolCallStatus, workingDir string) acp.ToolCallUpdate { - kind := acp.ToolKindExecute + kind := determineToolKind(toolCall.Function.Name, tool) title := cmp.Or(tool.Annotations.Title, toolCall.Function.Name) - if tool.Annotations.ReadOnlyHint { - kind = acp.ToolKindRead - } - args := parseToolCallArguments(toolCall.Function.Arguments) return acp.ToolCallUpdate{ ToolCallId: acp.ToolCallId(toolCall.ID), @@ -88,13 +85,6 @@ func buildToolCallUpdate(toolCall tools.ToolCall, tool tools.Tool, status acp.To // determineToolKind maps tool names and annotations to ACP tool kinds. func determineToolKind(toolName string, tool tools.Tool) acp.ToolKind { - if tool.Annotations.ReadOnlyHint { - return acp.ToolKindRead - } - if tool.Annotations.DestructiveHint != nil && *tool.Annotations.DestructiveHint { - return acp.ToolKindDelete - } - switch { case strings.HasPrefix(toolName, "read_"), strings.HasPrefix(toolName, "get_"), @@ -110,8 +100,7 @@ func determineToolKind(toolName string, tool tools.Tool) acp.ToolKind { return acp.ToolKindEdit case strings.HasPrefix(toolName, "delete_"), - strings.HasPrefix(toolName, "remove_"), - strings.HasPrefix(toolName, "stop_"): + strings.HasPrefix(toolName, "remove_"): return acp.ToolKindDelete case strings.HasPrefix(toolName, "search_"), @@ -126,6 +115,7 @@ func determineToolKind(toolName string, tool tools.Tool) acp.ToolKind { return acp.ToolKindFetch case toolName == "shell", + strings.HasPrefix(toolName, "stop_"), strings.HasPrefix(toolName, "run_"), strings.HasPrefix(toolName, "exec_"): return acp.ToolKindExecute @@ -135,6 +125,9 @@ func determineToolKind(toolName string, tool tools.Tool) acp.ToolKind { return acp.ToolKindSwitchMode default: + if tool.Annotations.ReadOnlyHint { + return acp.ToolKindRead + } return acp.ToolKindOther } } diff --git a/pkg/acp/toolcall_test.go b/pkg/acp/toolcall_test.go index 6da91e4a8..f1abeabe5 100644 --- a/pkg/acp/toolcall_test.go +++ b/pkg/acp/toolcall_test.go @@ -22,9 +22,9 @@ func TestDetermineToolKind(t *testing.T) { annotations tools.ToolAnnotations want acpsdk.ToolKind }{ - {name: "read only hint wins", toolName: "delete_everything", annotations: tools.ToolAnnotations{ReadOnlyHint: true}, want: acpsdk.ToolKindRead}, - {name: "read only hint wins over destructive", toolName: "shell", annotations: tools.ToolAnnotations{ReadOnlyHint: true, DestructiveHint: new(true)}, want: acpsdk.ToolKindRead}, - {name: "destructive hint", toolName: "shell", annotations: tools.ToolAnnotations{DestructiveHint: new(true)}, want: acpsdk.ToolKindDelete}, + {name: "specific deletion wins over read hint", toolName: "delete_everything", annotations: tools.ToolAnnotations{ReadOnlyHint: true}, want: acpsdk.ToolKindDelete}, + {name: "shell remains execute despite hints", toolName: "shell", annotations: tools.ToolAnnotations{ReadOnlyHint: true, DestructiveHint: new(true)}, want: acpsdk.ToolKindExecute}, + {name: "destructive shell is not deletion", toolName: "shell", annotations: tools.ToolAnnotations{DestructiveHint: new(true)}, want: acpsdk.ToolKindExecute}, {name: "destructive hint false falls through", toolName: "shell", annotations: tools.ToolAnnotations{DestructiveHint: new(false)}, want: acpsdk.ToolKindExecute}, {name: "read_ prefix", toolName: "read_file", want: acpsdk.ToolKindRead}, {name: "get_ prefix", toolName: "get_issue", want: acpsdk.ToolKindRead}, @@ -37,7 +37,7 @@ func TestDetermineToolKind(t *testing.T) { {name: "add_ prefix", toolName: "add_comment", want: acpsdk.ToolKindEdit}, {name: "delete_ prefix", toolName: "delete_file", want: acpsdk.ToolKindDelete}, {name: "remove_ prefix", toolName: "remove_directory", want: acpsdk.ToolKindDelete}, - {name: "stop_ prefix", toolName: "stop_container", want: acpsdk.ToolKindDelete}, + {name: "stop_ prefix", toolName: "stop_container", want: acpsdk.ToolKindExecute}, {name: "search_ prefix", toolName: "search_files_content", want: acpsdk.ToolKindSearch}, {name: "find_ prefix", toolName: "find_symbol", want: acpsdk.ToolKindSearch}, {name: "think", toolName: "think", want: acpsdk.ToolKindThink}, @@ -49,6 +49,11 @@ func TestDetermineToolKind(t *testing.T) { {name: "transfer_task", toolName: "transfer_task", want: acpsdk.ToolKindSwitchMode}, {name: "handoff", toolName: "handoff", want: acpsdk.ToolKindSwitchMode}, {name: "unknown", toolName: "banana", want: acpsdk.ToolKindOther}, + {name: "read-only search", toolName: "search_files_content", annotations: tools.ToolAnnotations{ReadOnlyHint: true}, want: acpsdk.ToolKindSearch}, + {name: "destructive edit", toolName: "edit_file", annotations: tools.ToolAnnotations{DestructiveHint: new(true)}, want: acpsdk.ToolKindEdit}, + {name: "destructive write", toolName: "write_file", annotations: tools.ToolAnnotations{DestructiveHint: new(true)}, want: acpsdk.ToolKindEdit}, + {name: "unknown read-only", toolName: "banana", annotations: tools.ToolAnnotations{ReadOnlyHint: true}, want: acpsdk.ToolKindRead}, + {name: "unknown destructive", toolName: "banana", annotations: tools.ToolAnnotations{DestructiveHint: new(true)}, want: acpsdk.ToolKindOther}, } for _, tt := range tests { @@ -264,7 +269,7 @@ func TestBuildToolCallStart(t *testing.T) { assert.Equal(t, acpsdk.ToolCallId("call-1"), tc.ToolCallId) assert.Equal(t, "Read File", tc.Title) assert.Equal(t, acpsdk.ToolKindRead, tc.Kind) - assert.Equal(t, acpsdk.ToolCallStatusPending, tc.Status) + assert.Equal(t, acpsdk.ToolCallStatusInProgress, tc.Status) assert.Equal(t, map[string]any{"path": "a.txt"}, tc.RawInput) assert.Equal(t, []acpsdk.ToolCallLocation{{Path: filepath.Join(wd, "a.txt")}}, tc.Locations) }) @@ -308,7 +313,7 @@ func TestBuildToolCallUpdate(t *testing.T) { readOnly := tools.Tool{Name: "shell", Annotations: tools.ToolAnnotations{ReadOnlyHint: true}} update = buildToolCallUpdate(toolCall, readOnly, acpsdk.ToolCallStatusCompleted, "") require.NotNil(t, update.Kind) - assert.Equal(t, acpsdk.ToolKindRead, *update.Kind) + assert.Equal(t, acpsdk.ToolKindExecute, *update.Kind) } func TestIsTodoTool(t *testing.T) {