diff --git a/tests/e2e/test_task_approval.spec.ts b/tests/e2e/test_task_approval.spec.ts new file mode 100644 index 00000000..58ecba37 --- /dev/null +++ b/tests/e2e/test_task_approval.spec.ts @@ -0,0 +1,370 @@ +/** + * E2E Tests: Task Approval API Contract + * + * Tests the task approval flow to catch request body mismatches. + * This test was added after a bug where the frontend sent { task_ids: [...] } + * but the backend expected { approved: bool, excluded_task_ids: [...] }, + * causing HTTP 422 validation errors. + * + * Key validations: + * 1. Request body format matches backend Pydantic model + * 2. No 422 validation errors occur + * 3. API response matches expected format + */ + +import { test, expect, type Route, type Request } from '@playwright/test'; +import { + loginUser, + setupErrorMonitoring, + checkTestErrors, + ExtendedPage, + waitForAPIResponse +} from './test-utils'; +import { FRONTEND_URL, BACKEND_URL, TEST_PROJECT_IDS } from './e2e-config'; + +// Skip tests if no API key (can't generate tasks without Claude) +const HAS_API_KEY = !!process.env.ANTHROPIC_API_KEY; + +// Use the planning phase project for task approval tests +const PROJECT_ID = TEST_PROJECT_IDS.PLANNING; + +test.describe('Task Approval API Contract', () => { + test.beforeEach(async ({ context, page }) => { + const errorMonitor = setupErrorMonitoring(page); + (page as ExtendedPage).__errorMonitor = errorMonitor; + + await context.clearCookies(); + await loginUser(page); + }); + + test.afterEach(async ({ page }) => { + checkTestErrors(page, 'Task approval test', [ + 'net::ERR_ABORTED', // Normal when navigation cancels pending requests + 'Failed to fetch RSC payload', // Next.js RSC during navigation - transient + 'NS_BINDING_ABORTED', // Firefox: Normal when navigation cancels requests + 'Load request cancelled' // WebKit/Safari: Normal when navigation cancels requests + ]); + }); + + /** + * Test that the task approval request body matches the backend contract. + * + * Backend expects (Pydantic model in tasks.py): + * { + * "approved": bool, + * "excluded_task_ids": List[int] + * } + * + * Frontend must NOT send: + * - { task_ids: [...] } // Wrong field name + * - { approved_task_ids: [...] } // Wrong approach + * - Missing "approved" field + */ + test('should send correct request body format for task approval', async ({ page }) => { + let capturedRequestBody: any = null; + let approvalRequestIntercepted = false; + + // Intercept the task approval API call to capture and validate request body + await page.route('**/api/projects/*/tasks/approve', async (route: Route, request: Request) => { + approvalRequestIntercepted = true; + const postData = request.postData(); + + if (postData) { + try { + capturedRequestBody = JSON.parse(postData); + } catch { + capturedRequestBody = { _parseError: postData }; + } + } + + // Continue with the actual request + await route.continue(); + }); + + // Navigate to project with tasks in planning phase + await page.goto(`${FRONTEND_URL}/projects/${PROJECT_ID}`); + await page.waitForLoadState('networkidle'); + + // Navigate to Tasks tab + const tasksTab = page.locator('[data-testid="tasks-tab"]'); + await tasksTab.waitFor({ state: 'visible', timeout: 10000 }).catch(() => {}); + + if (await tasksTab.isVisible()) { + await tasksTab.click(); + + // Look for TaskReview component (only present in planning phase) + const taskReview = page.locator('[role="tree"]'); // TaskReview uses role="tree" + const approveButton = page.getByRole('button', { name: /approve/i }); + + // Only proceed if we're in planning phase with task review available + if (await taskReview.isVisible().catch(() => false) && await approveButton.isVisible().catch(() => false)) { + // Click approve button + await approveButton.click(); + + // Wait for API call to complete (success or error) + await page.waitForResponse( + response => response.url().includes('/tasks/approve'), + { timeout: 10000 } + ).catch(() => {}); + + // Validate the request body format + if (approvalRequestIntercepted && capturedRequestBody) { + // CRITICAL ASSERTIONS: These would have caught the original bug + + // 1. Must have "approved" field (boolean) + expect(capturedRequestBody).toHaveProperty('approved'); + expect(typeof capturedRequestBody.approved).toBe('boolean'); + + // 2. Must have "excluded_task_ids" field (array) + expect(capturedRequestBody).toHaveProperty('excluded_task_ids'); + expect(Array.isArray(capturedRequestBody.excluded_task_ids)).toBe(true); + + // 3. Must NOT have "task_ids" field (the buggy format) + expect(capturedRequestBody).not.toHaveProperty('task_ids'); + + // 4. excluded_task_ids should contain integers, not strings + if (capturedRequestBody.excluded_task_ids.length > 0) { + capturedRequestBody.excluded_task_ids.forEach((id: any) => { + expect(typeof id).toBe('number'); + }); + } + + console.log('[Task Approval] Request body validated successfully:', capturedRequestBody); + } + } else { + // Not in planning phase - skip test but don't fail + console.log('[Task Approval] Project not in planning phase - skipping approval validation'); + test.skip(true, 'Project not in planning phase'); + } + } + }); + + /** + * Test that 422 validation errors from task approval are properly handled. + * + * This test mocks a 422 response to verify the frontend handles it gracefully. + */ + test('should handle 422 validation error gracefully', async ({ page }) => { + let responseStatus: number | null = null; + + // Mock a 422 validation error response + await page.route('**/api/projects/*/tasks/approve', async (route: Route) => { + await route.fulfill({ + status: 422, + contentType: 'application/json', + body: JSON.stringify({ + detail: [ + { + loc: ['body', 'approved'], + msg: 'field required', + type: 'value_error.missing' + } + ] + }) + }); + }); + + // Navigate to project + await page.goto(`${FRONTEND_URL}/projects/${PROJECT_ID}`); + await page.waitForLoadState('networkidle'); + + // Navigate to Tasks tab + const tasksTab = page.locator('[data-testid="tasks-tab"]'); + await tasksTab.waitFor({ state: 'visible', timeout: 10000 }).catch(() => {}); + + if (await tasksTab.isVisible()) { + await tasksTab.click(); + + // Look for approve button + const approveButton = page.getByRole('button', { name: /approve/i }); + + if (await approveButton.isVisible().catch(() => false)) { + // Click approve button + await approveButton.click(); + + // Wait for response + const response = await page.waitForResponse( + r => r.url().includes('/tasks/approve'), + { timeout: 10000 } + ).catch(() => null); + + if (response) { + responseStatus = response.status(); + + // Verify 422 was received + expect(responseStatus).toBe(422); + + // Verify UI shows error message (not a blank screen) + const errorMessage = page.locator('[class*="destructive"], [class*="error"], [role="alert"]'); + await errorMessage.first().waitFor({ state: 'visible', timeout: 5000 }).catch(() => {}); + + // Should display some error feedback + const hasErrorUI = await errorMessage.count() > 0 || + await page.getByText(/failed|error|try again/i).isVisible().catch(() => false); + + expect(hasErrorUI).toBe(true); + } + } else { + console.log('[Task Approval] Approve button not visible - project may not be in planning phase'); + test.skip(true, 'Project not in planning phase'); + } + } + }); + + /** + * Test the full task approval flow with API response validation. + * + * Only runs when ANTHROPIC_API_KEY is available (needed for task generation). + */ + test(HAS_API_KEY ? 'should complete task approval with correct API response' : 'skip: requires ANTHROPIC_API_KEY', + async ({ page }) => { + test.skip(!HAS_API_KEY, 'Requires ANTHROPIC_API_KEY for task generation'); + + let approvalResponse: any = null; + + // Capture the approval response + page.on('response', async response => { + if (response.url().includes('/tasks/approve') && response.request().method() === 'POST') { + const status = response.status(); + + // Fail fast on 422 - this was the original bug + if (status === 422) { + const body = await response.json().catch(() => ({})); + throw new Error( + `Task approval returned 422 Validation Error!\n` + + `This indicates a frontend/backend contract mismatch.\n` + + `Response: ${JSON.stringify(body, null, 2)}` + ); + } + + if (status === 200) { + approvalResponse = await response.json().catch(() => null); + } + } + }); + + // Navigate to project + await page.goto(`${FRONTEND_URL}/projects/${PROJECT_ID}`); + await page.waitForLoadState('networkidle'); + + // Navigate to Tasks tab + const tasksTab = page.locator('[data-testid="tasks-tab"]'); + if (await tasksTab.isVisible().catch(() => false)) { + await tasksTab.click(); + + const approveButton = page.getByRole('button', { name: /approve/i }); + + if (await approveButton.isVisible().catch(() => false)) { + await approveButton.click(); + + // Wait for approval to complete + await page.waitForResponse( + r => r.url().includes('/tasks/approve'), + { timeout: 15000 } + ).catch(() => {}); + + // Validate response format + if (approvalResponse) { + expect(approvalResponse).toHaveProperty('success'); + expect(approvalResponse).toHaveProperty('phase'); + expect(approvalResponse).toHaveProperty('approved_count'); + expect(approvalResponse).toHaveProperty('excluded_count'); + expect(approvalResponse).toHaveProperty('message'); + + console.log('[Task Approval] API response validated:', approvalResponse); + } + } + } + } + ); +}); + +/** + * Integration test for API contract between frontend and backend. + * + * These tests verify the contract without requiring the full UI flow. + */ +test.describe('Task Approval API Contract - Direct API Tests', () => { + test.beforeEach(async ({ context, page }) => { + await context.clearCookies(); + await loginUser(page); + }); + + /** + * Test the API directly to verify contract. + * + * This catches mismatches even if the UI is never exercised. + */ + test('should accept correct request body format via direct API call', async ({ page, request }) => { + // Get auth token + const authToken = await page.evaluate(() => localStorage.getItem('auth_token')); + test.skip(!authToken, 'No auth token available'); + + // Make direct API call with correct format + const response = await request.post( + `${BACKEND_URL}/api/projects/${PROJECT_ID}/tasks/approve`, + { + headers: { + 'Authorization': `Bearer ${authToken}`, + 'Content-Type': 'application/json' + }, + data: { + approved: true, + excluded_task_ids: [] + } + } + ); + + const status = response.status(); + + // Should NOT be 422 (validation error) with correct format + expect(status).not.toBe(422); + + // Acceptable statuses: 200 (success), 400 (not in planning phase), 404 (project not found) + // All of these indicate the request format was valid + expect([200, 400, 403, 404]).toContain(status); + + if (status === 200) { + const body = await response.json(); + expect(body).toHaveProperty('success'); + } + + console.log(`[API Contract] Direct API call returned ${status} - format accepted`); + }); + + /** + * Negative test: Verify 422 is returned for incorrect format. + * + * This documents the expected backend behavior. + */ + test('should return 422 for incorrect request body format', async ({ page, request }) => { + // Get auth token + const authToken = await page.evaluate(() => localStorage.getItem('auth_token')); + test.skip(!authToken, 'No auth token available'); + + // Make direct API call with INCORRECT format (the buggy format) + const response = await request.post( + `${BACKEND_URL}/api/projects/${PROJECT_ID}/tasks/approve`, + { + headers: { + 'Authorization': `Bearer ${authToken}`, + 'Content-Type': 'application/json' + }, + data: { + task_ids: ['task-1', 'task-2', 'task-3'] // WRONG FORMAT + } + } + ); + + const status = response.status(); + + // Should be 422 (validation error) because "approved" field is missing + // and "task_ids" is an unknown field + expect(status).toBe(422); + + const body = await response.json(); + expect(body).toHaveProperty('detail'); + + console.log('[API Contract] Incorrect format correctly rejected with 422'); + }); +}); diff --git a/web-ui/__tests__/components/TaskReview.test.tsx b/web-ui/__tests__/components/TaskReview.test.tsx index 7bbd9257..3c40b578 100644 --- a/web-ui/__tests__/components/TaskReview.test.tsx +++ b/web-ui/__tests__/components/TaskReview.test.tsx @@ -446,7 +446,7 @@ describe('TaskReview', () => { }); describe('Approval Flow', () => { - it('should call approval API with selected task IDs when approved', async () => { + it('should call approval API with both selected and all task IDs', async () => { const user = userEvent.setup(); render(); @@ -458,17 +458,18 @@ describe('TaskReview', () => { const approveButton = screen.getByRole('button', { name: /approve/i }); await user.click(approveButton); + // The API should receive selectedTaskIds (all selected) AND allTaskIds + // Since all are selected, both arrays should be identical await waitFor(() => { - expect(projectsApi.approveTaskBreakdown).toHaveBeenCalledWith(1, [ - 'task-1', - 'task-2', - 'task-3', - 'task-4', - ]); + expect(projectsApi.approveTaskBreakdown).toHaveBeenCalledWith( + 1, + expect.arrayContaining(['task-1', 'task-2', 'task-3', 'task-4']), + expect.arrayContaining(['task-1', 'task-2', 'task-3', 'task-4']) + ); }); }); - it('should only include selected tasks in approval', async () => { + it('should pass excluded tasks via the allTaskIds parameter', async () => { const user = userEvent.setup(); render(); @@ -488,12 +489,21 @@ describe('TaskReview', () => { const approveButton = screen.getByRole('button', { name: /approve/i }); await user.click(approveButton); - await waitFor(() => { - expect(projectsApi.approveTaskBreakdown).toHaveBeenCalledWith(1, [ - 'task-2', - 'task-3', - 'task-4', - ]); + // API should receive: + // - selectedTaskIds: 3 tasks (task-2, task-3, task-4) + // - allTaskIds: all 4 tasks (so backend can compute excluded = [task-1]) + await waitFor(() => { + expect(projectsApi.approveTaskBreakdown).toHaveBeenCalledWith( + 1, + expect.arrayContaining(['task-2', 'task-3', 'task-4']), + expect.arrayContaining(['task-1', 'task-2', 'task-3', 'task-4']) + ); + + // Verify task-1 is NOT in selectedTaskIds (first argument after projectId) + const calls = (projectsApi.approveTaskBreakdown as jest.Mock).mock.calls; + const selectedTaskIds = calls[0][1]; + expect(selectedTaskIds).not.toContain('task-1'); + expect(selectedTaskIds).toHaveLength(3); }); }); diff --git a/web-ui/__tests__/lib/api.test.ts b/web-ui/__tests__/lib/api.test.ts index d1f222da..3779222d 100644 --- a/web-ui/__tests__/lib/api.test.ts +++ b/web-ui/__tests__/lib/api.test.ts @@ -185,6 +185,153 @@ describe('projectsApi.startProject', () => { }); }); +describe('projectsApi.approveTaskBreakdown', () => { + it('should send correct request body matching backend contract', async () => { + const mockResponse = { + data: { + success: true, + message: 'Tasks approved successfully', + approved_count: 3, + project_phase: 'active', + }, + }; + + mockPost.mockResolvedValueOnce(mockResponse); + + // Frontend has 4 tasks total, user selected 3 (excluded task-4) + const allTaskIds = ['task-1', 'task-2', 'task-3', 'task-4']; + const selectedTaskIds = ['task-1', 'task-2', 'task-3']; + + await projectsApi.approveTaskBreakdown(42, selectedTaskIds, allTaskIds); + + // Backend expects: { approved: true, excluded_task_ids: [integers of excluded tasks] } + // Excluded = allTaskIds - selectedTaskIds = ['task-4'] -> need to convert to integers + expect(mockPost).toHaveBeenCalledWith( + '/api/projects/42/tasks/approve', + { + approved: true, + excluded_task_ids: [4], // task-4 was excluded, converted to integer + } + ); + }); + + it('should handle when all tasks are selected (no exclusions)', async () => { + const mockResponse = { + data: { + success: true, + message: 'All tasks approved', + approved_count: 4, + project_phase: 'active', + }, + }; + + mockPost.mockResolvedValueOnce(mockResponse); + + const allTaskIds = ['task-1', 'task-2', 'task-3', 'task-4']; + const selectedTaskIds = ['task-1', 'task-2', 'task-3', 'task-4']; + + await projectsApi.approveTaskBreakdown(1, selectedTaskIds, allTaskIds); + + expect(mockPost).toHaveBeenCalledWith( + '/api/projects/1/tasks/approve', + { + approved: true, + excluded_task_ids: [], // No exclusions + } + ); + }); + + it('should handle numeric task IDs correctly', async () => { + const mockResponse = { + data: { success: true, approved_count: 2, project_phase: 'active' }, + }; + + mockPost.mockResolvedValueOnce(mockResponse); + + // Some backends use numeric IDs directly + const allTaskIds = ['1', '2', '3']; + const selectedTaskIds = ['1', '3']; + + await projectsApi.approveTaskBreakdown(1, selectedTaskIds, allTaskIds); + + expect(mockPost).toHaveBeenCalledWith( + '/api/projects/1/tasks/approve', + { + approved: true, + excluded_task_ids: [2], // task ID '2' excluded, converted to integer + } + ); + }); + + it('should handle 422 validation error (schema mismatch)', async () => { + const errorResponse = { + response: { + status: 422, + data: { detail: 'Validation error' }, + }, + }; + + mockPost.mockRejectedValueOnce(errorResponse); + + await expect( + projectsApi.approveTaskBreakdown(1, [], []) + ).rejects.toMatchObject(errorResponse); + }); + + it('should reject malformed task IDs defensively', async () => { + const mockResponse = { + data: { success: true, approved_count: 0, project_phase: 'active' }, + }; + + mockPost.mockResolvedValueOnce(mockResponse); + + // Malformed IDs should be filtered out (return NaN, then filtered by .filter(!isNaN)) + const allTaskIds = ['task-1-2-3', 'abc123def', 'task-4', '5', 'invalid']; + const selectedTaskIds = ['task-4']; // Only task-4 is selected + + await projectsApi.approveTaskBreakdown(1, selectedTaskIds, allTaskIds); + + // Only valid excluded IDs should be sent: + // - 'task-1-2-3' -> matches /-(\d+)$/ -> '3' (last number after hyphen) + // - 'abc123def' -> no match -> NaN -> filtered out + // - '5' -> matches /^(\d+)$/ -> 5 + // - 'invalid' -> no match -> NaN -> filtered out + expect(mockPost).toHaveBeenCalledWith( + '/api/projects/1/tasks/approve', + { + approved: true, + excluded_task_ids: [3, 5], // Only valid IDs extracted + } + ); + }); + + it('should handle prefix-number format correctly', async () => { + const mockResponse = { + data: { success: true, approved_count: 1, project_phase: 'active' }, + }; + + mockPost.mockResolvedValueOnce(mockResponse); + + // Various valid formats + const allTaskIds = ['task-10', 'issue-25', 'T-100', '42']; + const selectedTaskIds = ['task-10']; + + await projectsApi.approveTaskBreakdown(1, selectedTaskIds, allTaskIds); + + // All should be valid: + // - 'issue-25' -> 25 + // - 'T-100' -> 100 + // - '42' -> 42 + expect(mockPost).toHaveBeenCalledWith( + '/api/projects/1/tasks/approve', + { + approved: true, + excluded_task_ids: [25, 100, 42], + } + ); + }); +}); + describe('blockersApi (T019 - 049-human-in-loop)', () => { describe('list() method', () => { it('should call correct endpoint with projectId', async () => { diff --git a/web-ui/src/components/TaskReview.tsx b/web-ui/src/components/TaskReview.tsx index cba596a7..e9d38d8c 100644 --- a/web-ui/src/components/TaskReview.tsx +++ b/web-ui/src/components/TaskReview.tsx @@ -207,9 +207,14 @@ const TaskReview = memo(function TaskReview({ setApprovalError(null); try { + // Get all task IDs for exclusion calculation + // Backend uses exclusion model: all tasks approved by default, specify excluded + const allTaskIds = getAllTaskIds(issues); + await projectsApi.approveTaskBreakdown( numericProjectId, - Array.from(selectedTaskIds) + Array.from(selectedTaskIds), + allTaskIds ); onApprovalSuccess?.(); @@ -260,7 +265,7 @@ const TaskReview = memo(function TaskReview({ } finally { setApproving(false); } - }, [projectId, selectedTaskIds, onApprovalSuccess, onApprovalError, router]); + }, [projectId, selectedTaskIds, issues, onApprovalSuccess, onApprovalError, router]); // Check if all tasks in an issue are selected const isIssueFullySelected = (issue: Issue): boolean => { diff --git a/web-ui/src/lib/api.ts b/web-ui/src/lib/api.ts index de088814..784d5869 100644 --- a/web-ui/src/lib/api.ts +++ b/web-ui/src/lib/api.ts @@ -102,13 +102,42 @@ export const projectsApi = { api.post<{ success: boolean; message: string; tasks_already_exist?: boolean }>( `/api/projects/${projectId}/discovery/generate-tasks` ), - approveTaskBreakdown: (projectId: number, taskIds: string[]) => - api.post<{ + /** + * Approve task breakdown and transition project to development phase. + * + * The backend uses an exclusion model - all tasks are approved by default, + * and you specify which ones to exclude. This function converts from the + * frontend's selection model (which tasks ARE selected). + * + * @param projectId - Project ID + * @param selectedTaskIds - Array of task IDs that the user selected (will be approved) + * @param allTaskIds - Array of all task IDs (needed to compute exclusions) + * @returns Approval response with success status and counts + */ + approveTaskBreakdown: (projectId: number, selectedTaskIds: string[], allTaskIds: string[]) => { + // Compute excluded task IDs: tasks that are in allTaskIds but NOT in selectedTaskIds + const selectedSet = new Set(selectedTaskIds); + const excludedTaskIds = allTaskIds + .filter((id) => !selectedSet.has(id)) + .map((id) => { + // Extract numeric ID from strings like 'task-4' or just '4' + // Defensive: only accept 'prefix-digits' or pure digits, reject malformed IDs + const match = id.match(/-(\d+)$/) || id.match(/^(\d+)$/); + if (!match) return NaN; + return parseInt(match[1], 10); + }) + .filter((id) => !isNaN(id)); + + return api.post<{ success: boolean; message: string; approved_count: number; project_phase: string; - }>(`/api/projects/${projectId}/tasks/approve`, { task_ids: taskIds }), + }>(`/api/projects/${projectId}/tasks/approve`, { + approved: true, + excluded_task_ids: excludedTaskIds, + }); + }, }; export const agentsApi = {