fix(web): validate activeSessionId belongs to activeTaskId before use (#614)
This commit is contained in:
@@ -532,16 +532,24 @@ function useTaskPageData(
|
||||
initialRepositories: Repository[],
|
||||
) {
|
||||
const activeTaskId = useAppStore((state) => state.tasks.activeTaskId);
|
||||
const activeSessionId = useAppStore((state) => state.tasks.activeSessionId);
|
||||
const setActiveSession = useAppStore((state) => state.setActiveSession);
|
||||
const setActiveTask = useAppStore((state) => state.setActiveTask);
|
||||
|
||||
// Validate that activeSessionId belongs to activeTaskId to prevent showing
|
||||
// messages from an unrelated session when navigating to a task without sessions.
|
||||
const validatedActiveSessionId = useAppStore((state) => {
|
||||
const sid = state.tasks.activeSessionId;
|
||||
if (!sid || !activeTaskId) return null;
|
||||
const session = state.taskSessions.items[sid];
|
||||
return session?.task_id === activeTaskId ? sid : null;
|
||||
});
|
||||
|
||||
const { task } = useTaskDetails(activeTaskId, initialTask);
|
||||
|
||||
const agent = useSessionAgent(task);
|
||||
useAutoStartSession(task, agent.handleStartAgent);
|
||||
const initialSessionId = sessionId ?? agent.taskSessionId ?? null;
|
||||
const effectiveSessionId = activeSessionId ?? initialSessionId;
|
||||
const effectiveSessionId = validatedActiveSessionId ?? initialSessionId;
|
||||
|
||||
useEffect(() => {
|
||||
syncActiveTaskSession({
|
||||
|
||||
@@ -0,0 +1,134 @@
|
||||
import { test, expect } from "../../fixtures/test-base";
|
||||
import { SessionPage } from "../../pages/session-page";
|
||||
|
||||
/** Mock response text from the simple-message scenario */
|
||||
const SIMPLE_MOCK_RESPONSE = "This is a simple mock response for e2e testing.";
|
||||
|
||||
/**
|
||||
* Test that navigating to a task without sessions does NOT show messages
|
||||
* from the previously viewed task's session.
|
||||
*
|
||||
* Reproduces bug: when activeSessionId holds an old session ID and the user
|
||||
* navigates to a new task (created with start_agent=false), the chat panel
|
||||
* was incorrectly showing messages from the old session.
|
||||
*/
|
||||
test.describe("Session isolation", () => {
|
||||
test("navigating to session-less task does not show messages from previous task", async ({
|
||||
testPage,
|
||||
apiClient,
|
||||
seedData,
|
||||
}) => {
|
||||
test.setTimeout(60_000);
|
||||
|
||||
// 1. Create a task WITH an agent session that will have messages
|
||||
const taskWithSession = await apiClient.createTaskWithAgent(
|
||||
seedData.workspaceId,
|
||||
"Task With Messages",
|
||||
seedData.agentProfileId,
|
||||
{
|
||||
description: "/e2e:simple-message",
|
||||
workflow_id: seedData.workflowId,
|
||||
workflow_step_id: seedData.startStepId,
|
||||
repository_ids: [seedData.repositoryId],
|
||||
},
|
||||
);
|
||||
|
||||
// 2. Navigate to the task with session and wait for agent to complete
|
||||
await testPage.goto(`/t/${taskWithSession.id}`);
|
||||
const session = new SessionPage(testPage);
|
||||
await session.waitForLoad();
|
||||
await expect(session.idleInput()).toBeVisible({ timeout: 30_000 });
|
||||
|
||||
// 3. Verify there are messages in the chat (agent has responded)
|
||||
const chatPanel = testPage.getByTestId("session-chat");
|
||||
await expect(chatPanel.getByText(SIMPLE_MOCK_RESPONSE)).toBeVisible({ timeout: 10_000 });
|
||||
|
||||
// 4. Create a task WITHOUT an agent session (start_agent=false)
|
||||
const taskWithoutSession = await apiClient.createTask(
|
||||
seedData.workspaceId,
|
||||
"Task Without Session",
|
||||
{
|
||||
workflow_id: seedData.workflowId,
|
||||
workflow_step_id: seedData.startStepId,
|
||||
repository_ids: [seedData.repositoryId],
|
||||
},
|
||||
);
|
||||
|
||||
// 5. Navigate to the session-less task directly via URL
|
||||
await testPage.goto(`/t/${taskWithoutSession.id}`);
|
||||
await session.waitForLoad();
|
||||
|
||||
// 6. The chat panel should NOT show the message from the previous task
|
||||
// This is the core assertion - we're testing that messages don't leak
|
||||
await expect(chatPanel.getByText(SIMPLE_MOCK_RESPONSE)).not.toBeVisible({ timeout: 5_000 });
|
||||
|
||||
// 7. Also verify the task title in the page matches the new task
|
||||
// (ensuring we actually navigated to the correct task)
|
||||
await expect(testPage.getByRole("link", { name: "Task Without Session" })).toBeVisible({
|
||||
timeout: 5_000,
|
||||
});
|
||||
});
|
||||
|
||||
test("switching tasks via sidebar does not show messages from previous task", async ({
|
||||
testPage,
|
||||
apiClient,
|
||||
seedData,
|
||||
}) => {
|
||||
test.setTimeout(60_000);
|
||||
|
||||
// 1. Create first task with agent session
|
||||
const taskA = await apiClient.createTaskWithAgent(
|
||||
seedData.workspaceId,
|
||||
"First Task A",
|
||||
seedData.agentProfileId,
|
||||
{
|
||||
description: "/e2e:simple-message",
|
||||
workflow_id: seedData.workflowId,
|
||||
workflow_step_id: seedData.startStepId,
|
||||
repository_ids: [seedData.repositoryId],
|
||||
},
|
||||
);
|
||||
|
||||
// 2. Create second task with a DIFFERENT agent session (different message)
|
||||
const taskB = await apiClient.createTaskWithAgent(
|
||||
seedData.workspaceId,
|
||||
"Second Task B",
|
||||
seedData.agentProfileId,
|
||||
{
|
||||
description: "/e2e:read-and-edit",
|
||||
workflow_id: seedData.workflowId,
|
||||
workflow_step_id: seedData.startStepId,
|
||||
repository_ids: [seedData.repositoryId],
|
||||
},
|
||||
);
|
||||
|
||||
// 3. Navigate to first task and wait for agent to complete
|
||||
await testPage.goto(`/t/${taskA.id}`);
|
||||
const session = new SessionPage(testPage);
|
||||
await session.waitForLoad();
|
||||
await expect(session.idleInput()).toBeVisible({ timeout: 30_000 });
|
||||
|
||||
// 4. Verify mock response message is visible for first task
|
||||
const chatPanel = testPage.getByTestId("session-chat");
|
||||
await expect(chatPanel.getByText(SIMPLE_MOCK_RESPONSE)).toBeVisible({ timeout: 10_000 });
|
||||
|
||||
// 5. Click on second task in the sidebar to switch
|
||||
await session.clickTaskInSidebar("Second Task B");
|
||||
|
||||
// 6. Wait for navigation to complete
|
||||
await expect(testPage).toHaveURL((url) => url.pathname.includes(`/t/${taskB.id}`), {
|
||||
timeout: 10_000,
|
||||
});
|
||||
await session.waitForLoad();
|
||||
await expect(session.idleInput()).toBeVisible({ timeout: 30_000 });
|
||||
|
||||
// 7. The chat should NOT show messages from the first task's session
|
||||
// (the "simple-message" text should not appear for task B which uses "read-and-edit")
|
||||
await expect(chatPanel.getByText(SIMPLE_MOCK_RESPONSE)).not.toBeVisible({ timeout: 5_000 });
|
||||
|
||||
// 8. Verify we're on the correct task
|
||||
await expect(testPage.getByRole("link", { name: "Second Task B" })).toBeVisible({
|
||||
timeout: 5_000,
|
||||
});
|
||||
});
|
||||
});
|
||||
@@ -0,0 +1,136 @@
|
||||
import { describe, it, expect, vi, beforeEach } from "vitest";
|
||||
import { renderHook } from "@testing-library/react";
|
||||
import type { TaskSession } from "@/lib/types/http";
|
||||
|
||||
// Mock state
|
||||
let mockActiveTaskId: string | null = null;
|
||||
let mockActiveSessionId: string | null = null;
|
||||
let mockSessionItems: Record<string, TaskSession> = {};
|
||||
let mockSession: TaskSession | null = null;
|
||||
let mockTask: { id: string; description: string } | null = null;
|
||||
|
||||
vi.mock("@/components/state-provider", () => ({
|
||||
useAppStore: (selector: (state: Record<string, unknown>) => unknown) =>
|
||||
selector({
|
||||
tasks: {
|
||||
activeTaskId: mockActiveTaskId,
|
||||
activeSessionId: mockActiveSessionId,
|
||||
},
|
||||
taskSessions: {
|
||||
items: mockSessionItems,
|
||||
},
|
||||
}),
|
||||
}));
|
||||
|
||||
vi.mock("@/hooks/domains/session/use-session", () => ({
|
||||
useSession: (id: string | null) => ({ session: id ? mockSession : null }),
|
||||
}));
|
||||
|
||||
vi.mock("@/hooks/use-task", () => ({
|
||||
useTask: (id: string | null) => (id ? mockTask : null),
|
||||
}));
|
||||
|
||||
import { useSessionState } from "./use-session-state";
|
||||
|
||||
const createMockSession = (
|
||||
id: string,
|
||||
taskId: string,
|
||||
state: TaskSession["state"] = "IDLE",
|
||||
): TaskSession =>
|
||||
({
|
||||
id,
|
||||
task_id: taskId,
|
||||
state,
|
||||
error_message: "",
|
||||
}) as TaskSession;
|
||||
|
||||
describe("useSessionState", () => {
|
||||
beforeEach(() => {
|
||||
vi.clearAllMocks();
|
||||
mockActiveTaskId = null;
|
||||
mockActiveSessionId = null;
|
||||
mockSessionItems = {};
|
||||
mockSession = null;
|
||||
mockTask = null;
|
||||
});
|
||||
|
||||
describe("resolvedSessionId", () => {
|
||||
it("uses sessionId directly when provided", () => {
|
||||
mockActiveTaskId = "task-1";
|
||||
mockActiveSessionId = "session-old";
|
||||
mockSessionItems = { "session-old": createMockSession("session-old", "task-old") };
|
||||
|
||||
const { result } = renderHook(() => useSessionState("session-explicit"));
|
||||
|
||||
expect(result.current.resolvedSessionId).toBe("session-explicit");
|
||||
});
|
||||
|
||||
it("uses activeSessionId when sessionId is null and session belongs to active task", () => {
|
||||
mockActiveTaskId = "task-1";
|
||||
mockActiveSessionId = "session-1";
|
||||
mockSessionItems = { "session-1": createMockSession("session-1", "task-1") };
|
||||
|
||||
const { result } = renderHook(() => useSessionState(null));
|
||||
|
||||
expect(result.current.resolvedSessionId).toBe("session-1");
|
||||
});
|
||||
|
||||
it("returns null when sessionId is null and activeSessionId belongs to different task", () => {
|
||||
mockActiveTaskId = "task-2";
|
||||
mockActiveSessionId = "session-1";
|
||||
mockSessionItems = { "session-1": createMockSession("session-1", "task-1") };
|
||||
|
||||
const { result } = renderHook(() => useSessionState(null));
|
||||
|
||||
expect(result.current.resolvedSessionId).toBeNull();
|
||||
});
|
||||
|
||||
it("returns null when sessionId is null and session not yet in store", () => {
|
||||
mockActiveTaskId = "task-1";
|
||||
mockActiveSessionId = "session-1";
|
||||
mockSessionItems = {}; // Session not loaded yet
|
||||
|
||||
const { result } = renderHook(() => useSessionState(null));
|
||||
|
||||
expect(result.current.resolvedSessionId).toBeNull();
|
||||
});
|
||||
|
||||
it("returns null when sessionId is null and activeSessionId is null", () => {
|
||||
mockActiveTaskId = "task-1";
|
||||
mockActiveSessionId = null;
|
||||
mockSessionItems = {};
|
||||
|
||||
const { result } = renderHook(() => useSessionState(null));
|
||||
|
||||
expect(result.current.resolvedSessionId).toBeNull();
|
||||
});
|
||||
});
|
||||
|
||||
describe("session flags", () => {
|
||||
it("sets isStarting when session state is STARTING", () => {
|
||||
mockSession = createMockSession("session-1", "task-1", "STARTING");
|
||||
|
||||
const { result } = renderHook(() => useSessionState("session-1"));
|
||||
|
||||
expect(result.current.isStarting).toBe(true);
|
||||
expect(result.current.isWorking).toBe(true);
|
||||
});
|
||||
|
||||
it("sets isAgentBusy when session state is RUNNING", () => {
|
||||
mockSession = createMockSession("session-1", "task-1", "RUNNING");
|
||||
|
||||
const { result } = renderHook(() => useSessionState("session-1"));
|
||||
|
||||
expect(result.current.isAgentBusy).toBe(true);
|
||||
expect(result.current.isWorking).toBe(true);
|
||||
});
|
||||
|
||||
it("sets isFailed when session state is FAILED", () => {
|
||||
mockSession = createMockSession("session-1", "task-1", "FAILED");
|
||||
|
||||
const { result } = renderHook(() => useSessionState("session-1"));
|
||||
|
||||
expect(result.current.isFailed).toBe(true);
|
||||
});
|
||||
});
|
||||
});
|
||||
@@ -13,8 +13,20 @@ function deriveSessionFlags(state: TaskSession["state"] | undefined, errorMessag
|
||||
}
|
||||
|
||||
export function useSessionState(sessionId: string | null) {
|
||||
const activeTaskId = useAppStore((state) => state.tasks.activeTaskId);
|
||||
const activeSessionId = useAppStore((state) => state.tasks.activeSessionId);
|
||||
const resolvedSessionId = sessionId ?? activeSessionId;
|
||||
|
||||
// Validate that active session belongs to the active task before using it.
|
||||
// This prevents showing messages from an unrelated session when navigating
|
||||
// to a task that has no sessions yet (activeSessionId may still hold the
|
||||
// old session from the previous task).
|
||||
const activeSessionData = useAppStore((state) =>
|
||||
activeSessionId ? (state.taskSessions.items[activeSessionId] ?? null) : null,
|
||||
);
|
||||
const validatedActiveSessionId =
|
||||
activeSessionData && activeSessionData.task_id === activeTaskId ? activeSessionId : null;
|
||||
|
||||
const resolvedSessionId = sessionId ?? validatedActiveSessionId;
|
||||
|
||||
const { session } = useSession(resolvedSessionId);
|
||||
const task = useTask(session?.task_id ?? null);
|
||||
|
||||
Reference in New Issue
Block a user