From 8d593bcddb6825697fb9a8941f38b2d02c245e3e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Flor=C3=AAncio?= Date: Wed, 13 May 2026 20:57:03 +0100 Subject: [PATCH] fix(web): scope pending-permission scan to current turn (#899) --- .../tests/chat/permission-approval.spec.ts | 49 ++++++++- .../lib/utils/pending-clarification.test.ts | 101 ++++++++++++++++++ apps/web/lib/utils/pending-clarification.ts | 22 +++- 3 files changed, 167 insertions(+), 5 deletions(-) diff --git a/apps/web/e2e/tests/chat/permission-approval.spec.ts b/apps/web/e2e/tests/chat/permission-approval.spec.ts index 620482d259..c5271d5637 100644 --- a/apps/web/e2e/tests/chat/permission-approval.spec.ts +++ b/apps/web/e2e/tests/chat/permission-approval.spec.ts @@ -4,18 +4,27 @@ import type { SeedData } from "../../fixtures/test-base"; import type { ApiClient } from "../../helpers/api-client"; import { SessionPage } from "../../pages/session-page"; +// Matches the number of permission prompts emitted by the mock-agent +// `/e2e:multi-permission` scenario (apps/backend/cmd/mock-agent/scenarios.go). +// Both approval loops below depend on this — bump together if the scenario +// changes. +const MULTI_PERMISSION_COUNT = 3; + /** * 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. + * The sidebar test passes a custom `title` so it can query the sidebar row by + * title text. */ async function seedMultiPermissionTask( testPage: Page, apiClient: ApiClient, seedData: SeedData, + title = "Multi-permission approval", ): Promise { const task = await apiClient.createTaskWithAgent( seedData.workspaceId, - "Multi-permission approval", + title, seedData.agentProfileId, { description: "/e2e:multi-permission", @@ -56,11 +65,11 @@ test.describe("Permission approval persistence", () => { }) => { const session = await seedMultiPermissionTask(testPage, apiClient, seedData); - // Approve all three permission prompts as they appear. Each click unblocks + // Approve all 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++) { + for (let i = 0; i < MULTI_PERMISSION_COUNT; i++) { await expect(session.permissionApproveButtons()).toHaveCount(1, { timeout: 30_000 }); await session.permissionApproveButtons().first().click(); } @@ -78,4 +87,38 @@ test.describe("Permission approval persistence", () => { await expect(session.idleInput()).toBeVisible({ timeout: 30_000 }); await expect(session.permissionActionRows()).toHaveCount(0); }); + + // While an agent is blocked on a permission_request, the sidebar entry for + // that task swaps the running spinner for the amber pending-permission icon + // (introduced in #882). The icon goes away once the prompt is resolved. + test("sidebar shows pending-permission icon while a permission prompt is open", async ({ + testPage, + apiClient, + seedData, + }) => { + const taskTitle = "Sidebar pending permission"; + const session = await seedMultiPermissionTask(testPage, apiClient, seedData, taskTitle); + + // First permission prompt blocks the agent. + await expect(session.permissionApproveButtons()).toHaveCount(1, { timeout: 30_000 }); + + // Sidebar swaps the running spinner for the amber pending-permission icon. + // No `.first()` — `e2eReset` runs before every test (and every retry), so + // the worker workspace has exactly one task with this title; if a duplicate + // ever appears, strict locator resolution should fail the test rather than + // silently picking one row. + const sidebarItem = session.sidebarTaskItem(taskTitle); + await expect(sidebarItem.getByTestId("task-state-pending-permission")).toBeVisible({ + timeout: 10_000, + }); + await expect(sidebarItem.getByTestId("task-state-running")).toHaveCount(0); + + // Approve all prompts; once the agent's turn ends, the icon is gone. + for (let i = 0; i < MULTI_PERMISSION_COUNT; i++) { + await expect(session.permissionApproveButtons()).toHaveCount(1, { timeout: 30_000 }); + await session.permissionApproveButtons().first().click(); + } + await expect(session.idleInput()).toBeVisible({ timeout: 30_000 }); + await expect(sidebarItem.getByTestId("task-state-pending-permission")).toHaveCount(0); + }); }); diff --git a/apps/web/lib/utils/pending-clarification.test.ts b/apps/web/lib/utils/pending-clarification.test.ts index 6e5cccef48..423c457969 100644 --- a/apps/web/lib/utils/pending-clarification.test.ts +++ b/apps/web/lib/utils/pending-clarification.test.ts @@ -183,4 +183,105 @@ describe("hasPendingPermissionRequest", () => { expect(hasPendingPermissionRequest(null)).toBe(false); expect(hasPendingPermissionRequest(undefined)).toBe(false); }); + + // Mixed-state: only the latest permission_request drives the UI. A stale + // pending row left behind by an earlier crash followed by a newer approved + // one must not light the amber icon — the agent is no longer blocked on + // the old row. + it("returns false when an older permission is still pending but a newer one is approved", () => { + expect( + hasPendingPermissionRequest([ + message({ id: "old", type: "permission_request", metadata: { status: "pending" } }), + message({ id: "new", type: "permission_request", metadata: { status: "approved" } }), + ]), + ).toBe(false); + }); + + it("returns true when the latest permission is pending and earlier ones are resolved", () => { + expect( + hasPendingPermissionRequest([ + message({ id: "a", type: "permission_request", metadata: { status: "approved" } }), + message({ id: "b", type: "permission_request", metadata: { status: "rejected" } }), + message({ id: "c", type: "permission_request", metadata: { status: "pending" } }), + ]), + ).toBe(true); + }); + + it("returns false when all permission requests are resolved regardless of order", () => { + expect( + hasPendingPermissionRequest([ + message({ id: "a", type: "permission_request", metadata: { status: "approved" } }), + message({ id: "b", type: "permission_request", metadata: { status: "rejected" } }), + message({ id: "c", type: "permission_request", metadata: { status: "expired" } }), + ]), + ).toBe(false); + }); +}); + +// Turn-scoped boundary: walking back across a different turn_id ends the +// scan, so a pending row from a previous (crashed) turn must not leak into +// the current turn's indicator. +describe("hasPendingPermissionRequest — turn scoping", () => { + it("ignores a pending permission from a previous turn", () => { + expect( + hasPendingPermissionRequest([ + message({ + id: "old", + turn_id: "t1", + type: "permission_request", + metadata: { status: "pending" }, + }), + message({ id: "current", turn_id: "t2", type: "message", content: "hi" }), + ]), + ).toBe(false); + }); + + it("detects a pending permission in the current turn", () => { + expect( + hasPendingPermissionRequest([ + message({ + id: "stale", + turn_id: "t1", + type: "permission_request", + metadata: { status: "pending" }, + }), + message({ + id: "active", + turn_id: "t2", + type: "permission_request", + metadata: { status: "pending" }, + }), + ]), + ).toBe(true); + }); + + it("stops at the turn boundary even when an older turn has a pending permission", () => { + expect( + hasPendingPermissionRequest([ + message({ + id: "old-pending", + turn_id: "t1", + type: "permission_request", + metadata: { status: "pending" }, + }), + message({ + id: "new-approved", + turn_id: "t2", + type: "permission_request", + metadata: { status: "approved" }, + }), + ]), + ).toBe(false); + }); + + // A legacy permission_request with no turn_id, sitting in a session whose + // latest message *does* have a turn_id, must not bypass the boundary. + it("ignores a legacy null-turn_id pending permission when a turn is active", () => { + expect( + hasPendingPermissionRequest([ + message({ id: "legacy", type: "permission_request", metadata: { status: "pending" } }), + message({ id: "current", turn_id: "t2", type: "message", content: "hi" }), + ]), + ).toBe(false); + }); }); diff --git a/apps/web/lib/utils/pending-clarification.ts b/apps/web/lib/utils/pending-clarification.ts index b0c4869b2f..5690e37589 100644 --- a/apps/web/lib/utils/pending-clarification.ts +++ b/apps/web/lib/utils/pending-clarification.ts @@ -63,10 +63,28 @@ export function isPendingPermissionMessage(message: Message): boolean { return !metadata?.status || metadata.status === "pending"; } +// hasPendingPermissionRequest reports whether the *current turn* is blocked +// on a permission_request. Scope: +// - Scans backwards from the end and stops at the first permission_request +// it sees — only the latest one drives the UI. A stale pending row left +// behind by an earlier crash followed by a newer approved one must not +// light up the amber icon (the agent is no longer blocked on the old row). +// - Honours turn boundaries: when the latest message has a turn_id, walking +// back to any row that doesn't share it ends the scan (including legacy +// rows with null/undefined turn_id). Old turns' permissions can never +// leak into the indicator and the scan stays bounded by the current turn +// size (typically 5–50 messages) regardless of total session length. +// - When the latest message itself has no turn_id (entirely pre-turn-scope +// data), boundary enforcement is disabled and we fall back to plain +// latest-only semantics across the whole array. export function hasPendingPermissionRequest(messages?: readonly Message[] | null): boolean { - if (!messages) return false; + if (!messages?.length) return false; + const latestTurnId = messages[messages.length - 1].turn_id; for (let i = messages.length - 1; i >= 0; i--) { - if (isPendingPermissionMessage(messages[i])) return true; + const m = messages[i]; + if (latestTurnId && m.turn_id !== latestTurnId) break; + if (m.type !== "permission_request") continue; + return isPendingPermissionMessage(m); } return false; }