diff --git a/apps/backend/cmd/mock-agent/scenarios.go b/apps/backend/cmd/mock-agent/scenarios.go index bdfc3ffb0d..067a4ec784 100644 --- a/apps/backend/cmd/mock-agent/scenarios.go +++ b/apps/backend/cmd/mock-agent/scenarios.go @@ -29,6 +29,7 @@ var scenarioRegistry = map[string]func(e *emitter){ "untracked-file-modify": scenarioUntrackedFileModify, "clarification": scenarioClarification, "clarification-timeout": scenarioClarificationTimeout, + "multi-permission": scenarioMultiPermission, "review-cumulative-setup": scenarioReviewCumulativeSetup, "symlink-file-setup": scenarioSymlinkFileSetup, } @@ -96,6 +97,34 @@ func scenarioReadAndEdit(e *emitter) { e.text("Read and edit scenario complete.") } +// scenarioMultiPermission: three sequential bash tools, each requiring permission. +// Reproduces the "approvals reappear at turn complete" bug — user approves all +// three, then when the turn ends the approval prompts must NOT come back. +func scenarioMultiPermission(e *emitter) { + fixedDelay(50) + e.text("Running three commands that need approval.") + + for i, label := range []string{"first", "second", "third"} { + fixedDelay(50) + id := nextToolID() + input := map[string]any{ + "command": fmt.Sprintf("echo %s", label), + "description": fmt.Sprintf("Step %d: %s", i+1, label), + } + e.startTool(id, fmt.Sprintf("Run %s command", label), acp.ToolKindExecute, input) + allowed := e.requestPermission(id, fmt.Sprintf("Run %s command", label), acp.ToolKindExecute, input) + fixedDelay(50) + if allowed { + e.completeTool(id, map[string]any{"output": label}) + } else { + e.completeTool(id, map[string]any{"output": "denied"}) + } + } + + fixedDelay(50) + e.text("Multi-permission scenario complete.") +} + // scenarioPermissionFlow: tool requiring permission with fixed delays. func scenarioPermissionFlow(e *emitter) { fixedDelay(50) diff --git a/apps/backend/internal/task/repository/session_repository_test.go b/apps/backend/internal/task/repository/session_repository_test.go index 741039f5a8..6a4aafa36b 100644 --- a/apps/backend/internal/task/repository/session_repository_test.go +++ b/apps/backend/internal/task/repository/session_repository_test.go @@ -372,8 +372,29 @@ func TestSQLiteRepository_CompletePendingToolCallsForTurn(t *testing.T) { Type: models.MessageTypeToolCall, Metadata: map[string]interface{}{"tool_call_id": "tc-6", "status": "error"}, } + // Permission_request messages share `tool_call_id` in metadata but their `status` + // is the user's approve/reject decision, not the tool call state. The sweep must + // leave them alone — otherwise an "approved" prompt would be reset and re-shown. + approvedPermission := &models.Message{ + ID: "msg-perm-approved-1", TaskSessionID: sessionID, TaskID: task.ID, TurnID: turnID, + AuthorType: models.MessageAuthorAgent, Content: "Approve this?", + Type: models.MessageTypePermissionRequest, + Metadata: map[string]interface{}{"tool_call_id": "tc-7", "pending_id": "p-1", "status": "approved"}, + } + rejectedPermission := &models.Message{ + ID: "msg-perm-rejected-1", TaskSessionID: sessionID, TaskID: task.ID, TurnID: turnID, + AuthorType: models.MessageAuthorAgent, Content: "Approve this?", + Type: models.MessageTypePermissionRequest, + Metadata: map[string]interface{}{"tool_call_id": "tc-8", "pending_id": "p-2", "status": "rejected"}, + } + pendingPermission := &models.Message{ + ID: "msg-perm-pending-1", TaskSessionID: sessionID, TaskID: task.ID, TurnID: turnID, + AuthorType: models.MessageAuthorAgent, Content: "Approve this?", + Type: models.MessageTypePermissionRequest, + Metadata: map[string]interface{}{"tool_call_id": "tc-9", "pending_id": "p-3", "status": "pending"}, + } - for _, msg := range []*models.Message{runningTool, completeTool, regularMsg, runningTool2, pendingTool, inProgressTool, errorTool} { + for _, msg := range []*models.Message{runningTool, completeTool, regularMsg, runningTool2, pendingTool, inProgressTool, errorTool, approvedPermission, rejectedPermission, pendingPermission} { if err := repo.CreateMessage(ctx, msg); err != nil { t.Fatalf("failed to create message %s: %v", msg.ID, err) } @@ -385,7 +406,8 @@ func TestSQLiteRepository_CompletePendingToolCallsForTurn(t *testing.T) { t.Fatalf("CompletePendingToolCallsForTurn failed: %v", err) } - // Should have updated 4 non-terminal tool call messages (running x2, pending, in_progress) + // Should have updated 4 non-terminal tool call messages (running x2, pending, in_progress). + // Permission_request messages must be excluded even though they carry tool_call_id. if affected != 4 { t.Errorf("expected 4 affected rows, got %d", affected) } @@ -430,6 +452,21 @@ func TestSQLiteRepository_CompletePendingToolCallsForTurn(t *testing.T) { t.Errorf("expected msg-error-1 status 'error' (unchanged), got %v", msg7.Metadata["status"]) } + // Verify permission_request messages were NOT affected — their status carries the + // user's decision (approve/reject) which the sweep must never overwrite. + msgPermApproved, _ := repo.GetMessage(ctx, "msg-perm-approved-1") + if msgPermApproved.Metadata["status"] != "approved" { + t.Errorf("expected msg-perm-approved-1 status 'approved' (unchanged), got %v", msgPermApproved.Metadata["status"]) + } + msgPermRejected, _ := repo.GetMessage(ctx, "msg-perm-rejected-1") + if msgPermRejected.Metadata["status"] != "rejected" { + t.Errorf("expected msg-perm-rejected-1 status 'rejected' (unchanged), got %v", msgPermRejected.Metadata["status"]) + } + msgPermPending, _ := repo.GetMessage(ctx, "msg-perm-pending-1") + if msgPermPending.Metadata["status"] != "pending" { + t.Errorf("expected msg-perm-pending-1 status 'pending' (unchanged), got %v", msgPermPending.Metadata["status"]) + } + // Running again should affect 0 rows affected2, err := repo.CompletePendingToolCallsForTurn(ctx, turnID) if err != nil { @@ -440,6 +477,64 @@ func TestSQLiteRepository_CompletePendingToolCallsForTurn(t *testing.T) { } } +// TestSQLiteRepository_GetMessageByToolCallID_ExcludesPermissionRequest locks +// in the second half of the approval-reappear fix: a tool_call message and a +// permission_request message can both carry the same tool_call_id (the +// permission_request stores it for FE pairing). The lookup must always return +// the tool_call row — otherwise a downstream tool_update would land on the +// permission_request and overwrite the user's approve/reject decision. +func TestSQLiteRepository_GetMessageByToolCallID_ExcludesPermissionRequest(t *testing.T) { + repo, cleanup := createTestSQLiteRepo(t) + defer cleanup() + ctx := context.Background() + + workflow := &models.Workflow{ID: "wf-1", Name: "Test Workflow"} + _ = repo.CreateWorkflow(ctx, workflow) + task := &models.Task{ID: "task-1", WorkflowID: "wf-1", WorkflowStepID: "step-1", Title: "Test Task"} + _ = repo.CreateTask(ctx, task) + sessionID := setupSQLiteTestSession(ctx, repo, task.ID, "session-1") + turnID := setupSQLiteTestTurn(ctx, repo, sessionID, task.ID, "turn-1") + + // Permission_request created FIRST so an ORDER BY created_at ASC sweep + // without the type filter would race to it. The fix must ignore type + // ordering and always return the tool_call row. + permMsg := &models.Message{ + ID: "msg-perm-1", TaskSessionID: sessionID, TaskID: task.ID, TurnID: turnID, + AuthorType: models.MessageAuthorAgent, Content: "Approve?", + Type: models.MessageTypePermissionRequest, + Metadata: map[string]interface{}{"tool_call_id": "tc-1", "pending_id": "p-1", "status": "pending"}, + } + if err := repo.CreateMessage(ctx, permMsg); err != nil { + t.Fatalf("create permission message: %v", err) + } + toolMsg := &models.Message{ + ID: "msg-tool-1", TaskSessionID: sessionID, TaskID: task.ID, TurnID: turnID, + AuthorType: models.MessageAuthorAgent, Content: "Tool", + Type: models.MessageTypeToolCall, + Metadata: map[string]interface{}{"tool_call_id": "tc-1", "status": "running"}, + } + if err := repo.CreateMessage(ctx, toolMsg); err != nil { + t.Fatalf("create tool_call message: %v", err) + } + + got, err := repo.GetMessageByToolCallID(ctx, sessionID, "tc-1") + if err != nil { + t.Fatalf("GetMessageByToolCallID: %v", err) + } + if got.Type != models.MessageTypeToolCall { + t.Errorf("expected tool_call row, got type=%s id=%s", got.Type, got.ID) + } + if got.ID != toolMsg.ID { + t.Errorf("expected msg-tool-1, got %s", got.ID) + } + + // And when only the permission_request exists, the lookup must return an + // error (not match the permission_request as a fallback). + if _, err := repo.GetMessageByToolCallID(ctx, sessionID, "tc-not-here"); err == nil { + t.Error("expected error for missing tool_call_id, got nil") + } +} + // TestGetPrimarySessionInfoByTaskIDs_PopulatesID locks in the SQL fix: the // SELECT now includes ts.id so session.ID is populated on the returned // TaskSession. Before the fix, publishTaskEvent saw sessionInfo.ID == "" and diff --git a/apps/backend/internal/task/repository/sqlite/message.go b/apps/backend/internal/task/repository/sqlite/message.go index 815ebc1a3e..77db8a51db 100644 --- a/apps/backend/internal/task/repository/sqlite/message.go +++ b/apps/backend/internal/task/repository/sqlite/message.go @@ -297,8 +297,14 @@ func (r *Repository) getMessageByMetadataField(ctx context.Context, sessionID, f // GetMessageByToolCallID retrieves a tool message by session ID and tool_call_id in metadata. // Searches all message types that have a tool_call_id in metadata (tool_call, tool_read, tool_edit, // tool_execute, tool_search, todo, etc.). +// +// Permission_request messages also store tool_call_id but represent the user-approval card, +// not the tool call itself. Excluded so a tool_update never lands on a permission_request +// row — that would overwrite metadata.status (the user's approve/reject) and retype it to +// tool_execute, making the prompt buttons reappear after the turn ends. func (r *Repository) GetMessageByToolCallID(ctx context.Context, sessionID, toolCallID string) (*models.Message, error) { - return r.getMessageByMetadataField(ctx, sessionID, "tool_call_id", toolCallID, "ORDER BY created_at ASC LIMIT 1") + return r.getMessageByMetadataField(ctx, sessionID, "tool_call_id", toolCallID, + "AND type != 'permission_request' ORDER BY created_at ASC LIMIT 1") } // GetMessageByPendingID retrieves a message by session ID and pending_id in metadata @@ -335,12 +341,17 @@ func (r *Repository) FindMessageByPendingID(ctx context.Context, pendingID strin // CompletePendingToolCallsForTurn marks all non-terminal tool call messages for a turn as "complete". // This is a safety net to ensure no tool calls remain stuck in a non-terminal state (pending, // running, in_progress, etc.) after a turn completes. +// +// Excludes permission_request messages: they share `tool_call_id` in metadata but their +// `status` is the user's approve/reject decision, not the tool call state. Forcing them to +// "complete" wipes "approved"/"rejected" and re-shows the prompt buttons in the UI. func (r *Repository) CompletePendingToolCallsForTurn(ctx context.Context, turnID string) (int64, error) { drv := r.db.DriverName() query := fmt.Sprintf(` UPDATE task_session_messages SET metadata = %s WHERE turn_id = ? + AND type != 'permission_request' AND %s NOT IN ('complete', 'error') AND %s `, dialect.JSONSet(drv, "metadata", "status", "complete"), diff --git a/apps/backend/internal/task/service/service_messages.go b/apps/backend/internal/task/service/service_messages.go index 14d49e753b..8be14b9867 100644 --- a/apps/backend/internal/task/service/service_messages.go +++ b/apps/backend/internal/task/service/service_messages.go @@ -462,7 +462,22 @@ func (s *Service) createToolCallMessageFallback(ctx context.Context, sessionID, } // applyToolCallMessageUpdate applies status, result, normalized data, and title to a tool call message. +// +// Defensive guard: a permission_request row also carries `tool_call_id` in metadata, so +// any future code path that hands such a row in here would otherwise silently overwrite +// the user's approve/reject decision and retype it to tool_execute. The repository's +// GetMessageByToolCallID excludes permission_request, but the guard makes the invariant +// explicit at the layer that does the writing. func (s *Service) applyToolCallMessageUpdate(message *models.Message, status, result, title string, normalized *streams.NormalizedPayload) { + if message.Type == models.MessageTypePermissionRequest { + // Error severity: the repo-layer GetMessageByToolCallID filter is supposed + // to make this branch unreachable. Reaching it means a caller bug; surface + // it loudly so the invariant violation isn't silently swallowed. + s.logger.Error("applyToolCallMessageUpdate refusing to overwrite permission_request", + zap.String("message_id", message.ID), + zap.String("incoming_status", status)) + return + } if message.Metadata == nil { message.Metadata = make(map[string]interface{}) } diff --git a/apps/web/components/task/chat/messages/permission-action-row.tsx b/apps/web/components/task/chat/messages/permission-action-row.tsx index ee3b60501c..7236562e26 100644 --- a/apps/web/components/task/chat/messages/permission-action-row.tsx +++ b/apps/web/components/task/chat/messages/permission-action-row.tsx @@ -17,7 +17,10 @@ export const PermissionActionRow = memo(function PermissionActionRow({ isResponding = false, }: PermissionActionRowProps) { return ( -
+
Approve this action? @@ -26,6 +29,7 @@ export const PermissionActionRow = memo(function PermissionActionRow({ variant="outline" onClick={onReject} disabled={isResponding} + data-testid="permission-reject" className="h-6 px-3 text-foreground border-border bg-background hover:bg-muted hover:border-foreground/40 transition-colors cursor-pointer" > @@ -36,6 +40,7 @@ export const PermissionActionRow = memo(function PermissionActionRow({ variant="outline" onClick={onApprove} disabled={isResponding} + data-testid="permission-approve" className="h-6 px-3 text-foreground border-border bg-background hover:bg-muted hover:border-foreground/40 transition-colors cursor-pointer" > {isResponding ? ( diff --git a/apps/web/e2e/fixtures/backend.ts b/apps/web/e2e/fixtures/backend.ts index 2cc2c7b56f..e0c97e4889 100644 --- a/apps/web/e2e/fixtures/backend.ts +++ b/apps/web/e2e/fixtures/backend.ts @@ -28,8 +28,14 @@ export type BackendContext = { frontendPort: number; frontendUrl: string; tmpDir: string; - /** Kill the backend process and respawn with the same config (DB, ports, tmpDir persist). */ - restart: () => Promise; + /** + * Kill the backend process and respawn with the same config (DB, ports, + * tmpDir persist). Optional `envOverrides` mutates the captured env *for the + * lifetime of the worker*; pass them again on a subsequent restart to + * revert. Only in-memory execution state (running agents, WS connections) + * is lost. + */ + restart: (envOverrides?: Record) => Promise; }; async function waitForHealth(url: string, timeoutMs: number, proc?: ChildProcess): Promise { @@ -243,6 +249,11 @@ exec git "$@" KANDEV_LOG_LEVEL: process.env.KANDEV_LOG_LEVEL ?? "warn", AGENTCTL_INSTANCE_PORT_BASE: String(agentctlPortBase), AGENTCTL_INSTANCE_PORT_MAX: String(agentctlPortMax), + // Most E2E tests rely on auto-approve being on so tools that require + // permission (Edit, Bash) just complete without UI interaction. Tests + // that exercise the approve/reject flow itself opt in by spawning their + // own backend with this env var set to "false" — see permission-approval.spec.ts. + AGENTCTL_AUTO_APPROVE_PERMISSIONS: process.env.AGENTCTL_AUTO_APPROVE_PERMISSIONS ?? "true", // Short window makes coalesce-boundary tests practical without wall-clock waits. KANDEV_PLAN_COALESCE_WINDOW_MS: process.env.KANDEV_PLAN_COALESCE_WINDOW_MS ?? "2000", GIT_AUTHOR_NAME: "E2E Test", @@ -296,7 +307,12 @@ exec git "$@" * SQLite DB, tmpDir, and all persisted data survive the restart. * Only in-memory execution state (running agents, WS connections) is lost. */ - const restart = async () => { + const restart = async (envOverrides?: Record) => { + if (envOverrides) { + for (const [k, v] of Object.entries(envOverrides)) { + (backendEnv as Record)[k] = v; + } + } await killProcessGroup(backendProc); // Wait for OS to release the TCP port — Linux TIME_WAIT can exceed 500ms under load await new Promise((r) => setTimeout(r, 2_000)); diff --git a/apps/web/e2e/pages/session-page.ts b/apps/web/e2e/pages/session-page.ts index 3ca0eef77e..7a5293425c 100644 --- a/apps/web/e2e/pages/session-page.ts +++ b/apps/web/e2e/pages/session-page.ts @@ -242,6 +242,16 @@ export class SessionPage { return this.clarificationOverlay().getByTestId("clarification-option-description"); } + /** All visible "Approve / Deny" rows for pending permission requests. */ + permissionActionRows(): Locator { + return this.chat.getByTestId("permission-action-row"); + } + + /** All "Approve" buttons for pending permission requests. */ + permissionApproveButtons(): Locator { + return this.chat.getByTestId("permission-approve"); + } + /** Reset context button in the chat input toolbar. */ resetContextButton(): Locator { return this.page.getByTestId("reset-context-button"); diff --git a/apps/web/e2e/tests/chat/permission-approval.spec.ts b/apps/web/e2e/tests/chat/permission-approval.spec.ts new file mode 100644 index 0000000000..620482d259 --- /dev/null +++ b/apps/web/e2e/tests/chat/permission-approval.spec.ts @@ -0,0 +1,81 @@ +import { type Page } from "@playwright/test"; +import { test, expect } from "../../fixtures/test-base"; +import type { SeedData } from "../../fixtures/test-base"; +import type { ApiClient } from "../../helpers/api-client"; +import { SessionPage } from "../../pages/session-page"; + +/** + * Seed a task that runs the multi-permission scenario, then navigate to it. + * The mock agent will request three permissions in sequence and block on each. + */ +async function seedMultiPermissionTask( + testPage: Page, + apiClient: ApiClient, + seedData: SeedData, +): Promise { + const task = await apiClient.createTaskWithAgent( + seedData.workspaceId, + "Multi-permission approval", + seedData.agentProfileId, + { + description: "/e2e:multi-permission", + workflow_id: seedData.workflowId, + workflow_step_id: seedData.startStepId, + repository_ids: [seedData.repositoryId], + }, + ); + + if (!task.session_id) throw new Error("createTaskWithAgent did not return a session_id"); + + await testPage.goto(`/t/${task.id}`); + + const session = new SessionPage(testPage); + await session.waitForLoad(); + + return session; +} + +test.describe("Permission approval persistence", () => { + test.describe.configure({ retries: 1 }); + + // Most E2E tests run with auto-approve on so tools-needing-permission scenarios + // (e.g. /e2e:read-and-edit) don't block the agent. This test specifically + // exercises the approve UI, so it needs auto-approve OFF — restart the worker + // backend with the override before this file's tests, then restore. + test.beforeAll(async ({ backend }) => { + await backend.restart({ AGENTCTL_AUTO_APPROVE_PERMISSIONS: "false" }); + }); + test.afterAll(async ({ backend }) => { + await backend.restart({ AGENTCTL_AUTO_APPROVE_PERMISSIONS: "true" }); + }); + + test("approved prompts stay approved after the agent's turn ends", async ({ + testPage, + apiClient, + seedData, + }) => { + const session = await seedMultiPermissionTask(testPage, apiClient, seedData); + + // Approve all three permission prompts as they appear. Each click unblocks + // the agent which then emits the next prompt; the previous one's button + // detaches before the next one mounts, so wait for the count to be back + // at 1 between clicks. + for (let i = 0; i < 3; i++) { + await expect(session.permissionApproveButtons()).toHaveCount(1, { timeout: 30_000 }); + await session.permissionApproveButtons().first().click(); + } + + // After the agent finishes its turn, no permission action row should be + // visible — the previous bug had them re-appear at turn-complete because a + // safety-net loop overwrote the approved status with "complete". + await expect(session.idleInput()).toBeVisible({ timeout: 30_000 }); + await expect(session.permissionActionRows()).toHaveCount(0); + + // And the resolved state must survive a page reload — i.e. backend must + // have persisted the approve decisions, not "complete". + await testPage.reload(); + await session.waitForLoad(); + await expect(session.idleInput()).toBeVisible({ timeout: 30_000 }); + await expect(session.permissionActionRows()).toHaveCount(0); + }); +}); diff --git a/apps/web/lib/ws/handlers/turns.ts b/apps/web/lib/ws/handlers/turns.ts index 5d8d385614..042bf75f41 100644 --- a/apps/web/lib/ws/handlers/turns.ts +++ b/apps/web/lib/ws/handlers/turns.ts @@ -39,9 +39,14 @@ export function registerTurnsHandlers(store: StoreApi): WsHandlers { // Safety net: mark any tool calls still in a non-terminal state as "complete". // This handles edge cases where tool_update events were dropped or not processed. + // Permission_request messages also carry `tool_call_id` in metadata, but their + // `status` represents the user's approve/reject decision — not the tool call + // state — so they must be excluded from the sweep. Forcing them to "complete" + // wipes "approved"/"rejected" and re-shows the prompt buttons in the UI. const messages = store.getState().messages.bySession[payload.session_id]; if (messages) { for (const msg of messages) { + if (msg.type === "permission_request") continue; const meta = msg.metadata as Record | undefined; if (meta?.tool_call_id && meta?.status !== "complete" && meta?.status !== "error") { store.getState().updateMessage({