diff --git a/cmd/ai.go b/cmd/ai.go index 0e74be10..e6bb47fa 100644 --- a/cmd/ai.go +++ b/cmd/ai.go @@ -499,9 +499,6 @@ func handleAICopilot(r *fastglue.Request) error { history = append(history, aimodels.ChatMessage{Role: aimodels.RoleUser, Content: req.Message}) scope := ai.AgentRunScope{AgentID: auser.ID, ConversationID: conv.ID, ConversationUUID: conv.UUID, Surface: aimodels.ToolInvocationCopilot, UserMessage: req.Message} - if _, approval := app.ai.PendingAgentApproval(scope); approval != nil { - return sendErrorEnvelope(r, envelope.NewError(envelope.ConflictError, app.i18n.T("ai.toolApprovalPending"), nil)) - } toolIDs, err := app.ai.GetEnabledCopilotToolIDs() if err != nil { return sendErrorEnvelope(r, err) diff --git a/i18n/en-US.json b/i18n/en-US.json index 6de71e61..9eda9e75 100644 --- a/i18n/en-US.json +++ b/i18n/en-US.json @@ -602,7 +602,7 @@ "ai.noTagsConfigured": "No tags exist yet. Tag suggestions need an existing tag list to choose from. Ask an admin to create tags first.", "ai.tagSuggestEmptyConversation": "There are no messages to suggest tags from yet.", "ai.toolApprovalExpired": "This tool request is no longer available. Run the AI action again to create a new request.", - "ai.toolApprovalPending": "A tool request is waiting for approval. Approve or decline it before sending another Copilot message.", + "ai.toolApprovalPending": "A tool request is waiting for approval. The AI cannot run again until it is reviewed. Approve or decline the pending request first.", "ai.toolApprovalUnavailable": "This tool request cannot be approved for this conversation. Run the AI action again to create a new request.", "ai.toolChangedBeforeApproval": "This tool changed after approval was requested and was not run. Run the AI action again to review the current tool.", "ai.toolApprovalTitle": "Review tool request", diff --git a/internal/ai/agent_approval.go b/internal/ai/agent_approval.go index 9adae8b0..3e5dcb06 100644 --- a/internal/ai/agent_approval.go +++ b/internal/ai/agent_approval.go @@ -118,7 +118,11 @@ func (m *Manager) ClearPendingAgentRuns(scope AgentRunScope) { } func (m *Manager) continueAgentRun(ctx context.Context, run *pendingAgentRun) (models.AgentRunResult, error) { - if result := m.processAgentToolCalls(ctx, run); result != nil { + result, err := m.processAgentToolCalls(ctx, run) + if err != nil { + return models.AgentRunResult{}, err + } + if result != nil { return *result, nil } for run.Step < run.MaxSteps { @@ -136,7 +140,11 @@ func (m *Manager) continueAgentRun(ctx context.Context, run *pendingAgentRun) (m run.ToolCalls = res.ToolCalls run.NextToolCall = 0 run.Step++ - if result := m.processAgentToolCalls(ctx, run); result != nil { + result, err := m.processAgentToolCalls(ctx, run) + if err != nil { + return models.AgentRunResult{}, err + } + if result != nil { return *result, nil } } @@ -153,7 +161,7 @@ func (m *Manager) continueAgentRun(ctx context.Context, run *pendingAgentRun) (m return completedAgentRun(res.Content), nil } -func (m *Manager) processAgentToolCalls(ctx context.Context, run *pendingAgentRun) *models.AgentRunResult { +func (m *Manager) processAgentToolCalls(ctx context.Context, run *pendingAgentRun) (*models.AgentRunResult, error) { for run.NextToolCall < len(run.ToolCalls) { toolCall := run.ToolCalls[run.NextToolCall] if run.Approvals { @@ -163,15 +171,17 @@ func (m *Manager) processAgentToolCalls(ctx context.Context, run *pendingAgentRu run.ToolUpdatedAt = customTool.toolUpdatedAt() run.CreatedAt = time.Now() run.ExpiresAt = run.CreatedAt.Add(pendingAgentRunTTL) - m.storeAgentRun(run) - return &models.AgentRunResult{Status: models.AgentRunApprovalRequired, Approval: agentRunApproval(run)} + if err := m.storeAgentRun(run); err != nil { + return nil, err + } + return &models.AgentRunResult{Status: models.AgentRunApprovalRequired, Approval: agentRunApproval(run)}, nil } } m.appendToolResult(run, toolCall, m.executeToolCall(ctx, run.Registry, toolCall)) run.NextToolCall++ } run.ToolCalls = nil - return nil + return nil, nil } func (m *Manager) appendToolResult(run *pendingAgentRun, toolCall models.ToolCall, result string) { @@ -183,11 +193,19 @@ func (m *Manager) appendToolResult(run *pendingAgentRun, toolCall models.ToolCal }) } -func (m *Manager) storeAgentRun(run *pendingAgentRun) { +// storeAgentRun admits one pending run per scope, so two requests racing on the same +// conversation and surface cannot both park a tool call the agent could approve twice. +func (m *Manager) storeAgentRun(run *pendingAgentRun) error { m.pendingRunMu.Lock() defer m.pendingRunMu.Unlock() m.deleteExpiredAgentRunsLocked(time.Now()) + for _, pending := range m.pendingRuns { + if sameAgentRunScope(pending.Scope, run.Scope) { + return envelope.NewError(envelope.ConflictError, m.i18n.T("ai.toolApprovalPending"), nil) + } + } m.pendingRuns[run.ID] = run + return nil } func (m *Manager) claimAgentRun(runID string, agentID int) (*pendingAgentRun, error) { diff --git a/internal/ai/agent_approval_test.go b/internal/ai/agent_approval_test.go index f67d5884..450bb1cc 100644 --- a/internal/ai/agent_approval_test.go +++ b/internal/ai/agent_approval_test.go @@ -124,6 +124,33 @@ func TestAgentToolDecisionIsFinalWhenFollowUpFails(t *testing.T) { } } +func TestSecondRunInSameScopeIsRejectedWhileOneAwaitsApproval(t *testing.T) { + m := newTestManager(t) + tool := &approvalTestTool{} + scope := AgentRunScope{AgentID: 7, ConversationID: 9, ConversationUUID: "conversation-1", Surface: models.ToolInvocationCopilot} + newRun := func() *pendingAgentRun { + return &pendingAgentRun{ + Scope: scope, + Client: &approvalTestProvider{responses: []models.ChatCompletionResult{ + {ToolCalls: []models.ToolCall{{ID: "call-1", Type: "function", Function: models.ToolCallFunction{Name: tool.Name(), Arguments: `{}`}}}}, + }}, + Registry: map[string]Tool{tool.Name(): tool}, + MaxSteps: 5, + Approvals: true, + } + } + + if _, err := m.continueAgentRun(t.Context(), newRun()); err != nil { + t.Fatalf("first run: %v", err) + } + if _, err := m.continueAgentRun(t.Context(), newRun()); err == nil { + t.Fatal("expected the second run in the same scope to be rejected") + } + if len(m.pendingRuns) != 1 { + t.Fatalf("expected one pending run, got %d", len(m.pendingRuns)) + } +} + func (t *approvalTestTool) approvalRequired() bool { return true } func (t *approvalTestTool) toolID() int { return 1 }