diff --git a/packages/app/e2e/new-workspace-mode-cycle-safety.spec.ts b/packages/app/e2e/new-workspace-mode-cycle-safety.spec.ts new file mode 100644 index 000000000..9a8948669 --- /dev/null +++ b/packages/app/e2e/new-workspace-mode-cycle-safety.spec.ts @@ -0,0 +1,146 @@ +import { expect, test, type Page } from "./fixtures"; +import { daemonWsRoutePattern } from "./helpers/daemon-port"; +import { openAgentRoute } from "./helpers/mock-agent"; +import { openGlobalNewWorkspaceComposer, selectNewWorkspaceProject } from "./helpers/new-workspace"; +import { seedWorkspace } from "./helpers/seed-client"; +import { getServerId } from "./helpers/server-id"; + +const CREATE_AGENT_PREFERENCES_KEY = "@paseo:create-agent-preferences"; + +type WebSocketMessage = string | Buffer; + +function parseWebSocketJson(message: WebSocketMessage): unknown { + const rawMessage = typeof message === "string" ? message : message.toString("utf8"); + try { + return JSON.parse(rawMessage); + } catch { + return null; + } +} + +function getSessionMessage(message: WebSocketMessage): Record | null { + const envelope = parseWebSocketJson(message); + if (!envelope || typeof envelope !== "object") { + return null; + } + const maybeEnvelope = envelope as { type?: unknown; message?: unknown }; + if (maybeEnvelope.type !== "session" || !maybeEnvelope.message) { + return null; + } + if (typeof maybeEnvelope.message !== "object") { + return null; + } + return maybeEnvelope.message as Record; +} + +// The draft mode control in New Workspace only mutates local form state; it never sends +// set_agent_mode_request. So the only source of such a request while the New Workspace +// composer is focused is a *live* agent's mode control. Recording those, keyed by agentId, +// gives a direct signal that Shift+Tab leaked into a backgrounded agent. +async function recordSetAgentModeRequests(page: Page): Promise<{ + requestsForAgent(agentId: string): Array<{ agentId: string; modeId: string }>; +}> { + const seen: Array<{ agentId: string; modeId: string }> = []; + await page.routeWebSocket(daemonWsRoutePattern(), (ws) => { + const server = ws.connectToServer(); + ws.onMessage((message) => { + const sessionMessage = getSessionMessage(message); + if (sessionMessage?.type === "set_agent_mode_request") { + const agentId = typeof sessionMessage.agentId === "string" ? sessionMessage.agentId : ""; + const modeId = typeof sessionMessage.modeId === "string" ? sessionMessage.modeId : ""; + seen.push({ agentId, modeId }); + } + server.send(message); + }); + server.onMessage((message) => ws.send(message)); + }); + return { + requestsForAgent: (agentId: string) => seen.filter((request) => request.agentId === agentId), + }; +} + +async function seedCodexDefaultPreferences(page: Page, serverId: string): Promise { + await page.addInitScript( + ({ preferencesKey, serverId: seededServerId }) => { + localStorage.setItem( + preferencesKey, + JSON.stringify({ + serverId: seededServerId, + provider: "codex", + providerPreferences: { + codex: { + model: "gpt-5.4-mini", + mode: "auto", + thinkingByModel: { "gpt-5.4-mini": "low" }, + }, + mock: { model: "ten-second-stream" }, + }, + }), + ); + }, + { preferencesKey: CREATE_AGENT_PREFERENCES_KEY, serverId }, + ); +} + +// Focus the New Workspace composer and cycle the execution mode with the keyboard. +// Kept out of the test body so the test reads as intent rather than key mechanics. +async function cycleNewWorkspaceMode(page: Page, presses: number): Promise { + const composer = page.getByRole("textbox", { name: "Message agent..." }); + await expect(composer).toBeVisible({ timeout: 30_000 }); + await composer.click(); + for (let i = 0; i < presses; i++) { + await page.keyboard.press("Shift+Tab"); + } +} + +test.describe("New Workspace mode cycle safety", () => { + test.describe.configure({ timeout: 240_000 }); + + // Regression guard for the P1 safety bug: cycling the execution mode with Shift+Tab in + // the New Workspace composer must never reach a backgrounded, still-mounted agent's mode + // control and silently change that (possibly running) agent's mode — e.g. into a + // permissive/bypass mode. See use-keyboard-action-handler.ts. + test("Shift+Tab in New Workspace never changes a backgrounded agent's mode", async ({ page }) => { + const serverId = getServerId(); + const seeded = await seedWorkspace({ repoPrefix: "mode-cycle-safety-" }); + await seedCodexDefaultPreferences(page, serverId); + const modeRequests = await recordSetAgentModeRequests(page); + + try { + const agent = await seeded.client.createAgent({ + provider: "codex", + cwd: seeded.repoPath, + workspaceId: seeded.workspaceId, + title: "mode cycle safety e2e", + modeId: "auto", + model: "gpt-5.4-mini", + }); + + // Mount the live agent tab: its mode control registers a mode-cycle keyboard handler. + await openAgentRoute(page, { workspaceId: seeded.workspaceId, agentId: agent.id }); + await expect(page.getByTestId("mode-control").first()).toContainText("Default permissions", { + timeout: 30_000, + }); + + // Move to the New Workspace composer. The agent tab stays mounted in the background, + // so its handler is still registered when we cycle here. + await openGlobalNewWorkspaceComposer(page); + await selectNewWorkspaceProject(page, { + projectKey: seeded.projectId, + projectDisplayName: seeded.projectDisplayName, + }); + + await cycleNewWorkspaceMode(page, 6); + + // fetchAgents is a real daemon round-trip; once it resolves, any mode change the + // presses would have triggered has already landed. Assert the running agent is + // untouched — both its committed mode and on the wire — with no fixed sleep. + const agents = await seeded.client.fetchAgents(); + const backgroundAgent = agents.entries.find((entry) => entry.agent.id === agent.id)?.agent; + expect(backgroundAgent?.currentModeId).toBe("auto"); + expect(modeRequests.requestsForAgent(agent.id)).toEqual([]); + } finally { + await seeded.cleanup(); + } + }); +}); diff --git a/packages/app/src/hooks/use-global-workspace-pin-action.ts b/packages/app/src/hooks/use-global-workspace-pin-action.ts index 6450b3259..d07da74d6 100644 --- a/packages/app/src/hooks/use-global-workspace-pin-action.ts +++ b/packages/app/src/hooks/use-global-workspace-pin-action.ts @@ -5,7 +5,7 @@ import type { KeyboardActionId } from "@/keyboard/keyboard-action-dispatcher"; import { useHostFeature } from "@/runtime/host-features"; import { useActiveWorkspaceSelection } from "@/stores/navigation-active-workspace-store"; import { useWorkspaceFields } from "@/stores/session-store-hooks"; -import { buildWorkspaceTabPersistenceKey } from "@/stores/workspace-tabs-store"; +import { buildWorkspaceTabPersistenceKey } from "@/workspace-tabs/model"; const WORKSPACE_PIN_ACTIONS: readonly KeyboardActionId[] = ["workspace.pin"]; diff --git a/packages/app/src/hooks/use-keyboard-action-handler.ts b/packages/app/src/hooks/use-keyboard-action-handler.ts index dda2bd85d..79a3410c8 100644 --- a/packages/app/src/hooks/use-keyboard-action-handler.ts +++ b/packages/app/src/hooks/use-keyboard-action-handler.ts @@ -1,4 +1,4 @@ -import { useEffect } from "react"; +import { useEffect, useRef } from "react"; import { keyboardActionDispatcher, @@ -15,15 +15,48 @@ interface UseKeyboardActionHandlerInput { handle: (action: KeyboardActionDefinition) => boolean; } +/** + * Registers a keyboard action handler with the global dispatcher. + * + * The dispatcher is driven by a native window keydown listener and calls the + * most-recently-registered matching handler first (see keyboard-action-dispatcher.ts). + * Two properties must hold or handlers bound to the same action step on each other: + * + * 1. Stable registration order. registerHandler bumps a registration counter, so + * re-registering on every render reshuffles which handler the dispatcher reaches + * first. A frequently re-rendering control (e.g. a running agent's mode control) + * would then jump ahead of another (e.g. the New Workspace draft) and consume the + * key, silently acting on the wrong surface. So we register once per + * (handlerId, priority, actions) and never re-register just because handle, + * isActive, or enabled changed. + * + * 2. Fresh callbacks. Because we do not re-register on every render, the registered + * entry must read the latest props at dispatch time. handle and isActive are read + * live from a ref, and enabled is folded into isActive so the dispatcher + * re-evaluates it fresh (the plain enabled field it filters on is captured at + * registration and would otherwise go stale). + */ export function useKeyboardActionHandler(input: UseKeyboardActionHandlerInput) { + const inputRef = useRef(input); + inputRef.current = input; + + // Only these identity-affecting fields trigger a re-register. actions is compared by + // content, not array identity, so inline literals do not churn the registration. + const actionsKey = input.actions.join(" "); useEffect(() => { return keyboardActionDispatcher.registerHandler({ - handlerId: input.handlerId, - actions: input.actions, - enabled: input.enabled, - priority: input.priority, - isActive: input.isActive, - handle: input.handle, + handlerId: inputRef.current.handlerId, + actions: inputRef.current.actions, + // Always-on at the coarse filter; the real enable/active gate is re-checked fresh + // inside isActive below so it can never be stale in the registry entry. + enabled: true, + priority: inputRef.current.priority, + isActive: () => { + const current = inputRef.current; + if (!current.enabled) return false; + return current.isActive ? current.isActive() : true; + }, + handle: (action) => inputRef.current.handle(action), }); - }, [input.actions, input.enabled, input.handle, input.handlerId, input.isActive, input.priority]); + }, [input.handlerId, input.priority, actionsKey]); }