From 936109ff5cd7f2b4af61aca0611765cf35c0032e Mon Sep 17 00:00:00 2001 From: Thomas Michnicki Date: Sun, 12 Jul 2026 08:07:18 +0200 Subject: [PATCH 1/8] test: wire vitest browser mode with Chromium Splits vitest into node and browser projects so UI interaction tests can run against real Chromium via @vitest/browser-playwright instead of jsdom. Migrates the existing dashboard spec into test/browser/ and adds a test:browser script that runs independently of the Postgres/env setup the node project requires. --- package.json | 1 + scripts/test.mjs | 2 +- test/{e2e => browser}/dashboard.spec.tsx | 37 +++++++----------------- test/browser/render.tsx | 13 +++++++++ test/browser/setup.ts | 7 +++++ vitest.config.ts | 32 +++++++++++++++++--- 6 files changed, 61 insertions(+), 31 deletions(-) rename test/{e2e => browser}/dashboard.spec.tsx (71%) create mode 100644 test/browser/render.tsx create mode 100644 test/browser/setup.ts diff --git a/package.json b/package.json index 5801706..bc673ff 100644 --- a/package.json +++ b/package.json @@ -25,6 +25,7 @@ "migrate": "node scripts/migrate.mjs", "test": "node scripts/test.mjs", "test:watch": "vitest", + "test:browser": "vitest run --project browser", "typecheck": "tsc --noEmit" }, "devDependencies": { diff --git a/scripts/test.mjs b/scripts/test.mjs index 804fba1..39b5cf3 100644 --- a/scripts/test.mjs +++ b/scripts/test.mjs @@ -74,4 +74,4 @@ if (!usableEnvValue(process.env.TEST_DATABASE_URL)) { process.env.DATABASE_URL = process.env.TEST_DATABASE_URL; run(process.execPath, ['scripts/migrate.mjs']); -run(process.execPath, ['node_modules/vitest/vitest.mjs', 'run']); +run(process.execPath, ['node_modules/vitest/vitest.mjs', 'run', '--project', 'node']); diff --git a/test/e2e/dashboard.spec.tsx b/test/browser/dashboard.spec.tsx similarity index 71% rename from test/e2e/dashboard.spec.tsx rename to test/browser/dashboard.spec.tsx index 1245255..542718e 100644 --- a/test/e2e/dashboard.spec.tsx +++ b/test/browser/dashboard.spec.tsx @@ -1,13 +1,9 @@ -/** - * @vitest-environment jsdom - */ import { expect, it, describe, vi, beforeEach } from 'vitest'; -import { render, screen } from '@testing-library/react'; +import { screen } from '@testing-library/react'; import { LoginPage } from '@client/pages/login'; import { DashboardPage } from '@client/pages/dashboard'; -import { MemoryRouter } from 'react-router-dom'; import { api } from '@client/lib/api'; -import { ThemeProvider } from '@client/lib/theme'; +import { renderPage } from './render'; // Mock the API client vi.mock('@client/lib/api', () => ({ @@ -20,7 +16,7 @@ vi.mock('@client/lib/api', () => ({ } })); -describe('Frontend UI Flows (JSDOM)', () => { +describe('Frontend UI Flows', () => { beforeEach(() => { vi.clearAllMocks(); @@ -32,13 +28,7 @@ describe('Frontend UI Flows (JSDOM)', () => { }); it('renders the GitHub sign-in flow', async () => { - render( - - - - - - ); + renderPage(); const signInLink = screen.getByRole('link', { name: 'Sign in with GitHub' }); expect(signInLink.getAttribute('href')).toBe('/auth/github'); @@ -77,22 +67,17 @@ describe('Frontend UI Flows (JSDOM)', () => { total: 1 }); - render( - - - - ); + renderPage(); // Check for dashboard title (from PageHeader) - expect(await screen.findByText('Dashboard')).toBeDefined(); - + expect(await screen.findByText('Dashboard')).toBeInTheDocument(); + // Check for stats totals (using data from getStats mock) - // Note: fmtNumber might format 500 as "500" or similar - expect(screen.getByText('10')).toBeDefined(); - expect(screen.getByText('500')).toBeDefined(); + expect(await screen.findByText('10')).toBeInTheDocument(); + expect(screen.getByText('500')).toBeInTheDocument(); // Check for activity stream item - expect(screen.getByText('test-owner/test-repo')).toBeDefined(); - expect(screen.getByRole('link', { name: 'Fixing bug' })).toBeDefined(); + expect(await screen.findByText('test-owner/test-repo')).toBeInTheDocument(); + expect(screen.getByRole('link', { name: 'Fixing bug' })).toBeInTheDocument(); }); }); diff --git a/test/browser/render.tsx b/test/browser/render.tsx new file mode 100644 index 0000000..e48ddaf --- /dev/null +++ b/test/browser/render.tsx @@ -0,0 +1,13 @@ +import type { ReactNode } from 'react'; +import { render } from '@testing-library/react'; +import { MemoryRouter } from 'react-router-dom'; +import { ThemeProvider } from '@client/lib/theme'; + +export function renderPage(ui: ReactNode, options: { route?: string } = {}) { + const { route = '/' } = options; + return render( + + {ui} + , + ); +} diff --git a/test/browser/setup.ts b/test/browser/setup.ts new file mode 100644 index 0000000..01d18c2 --- /dev/null +++ b/test/browser/setup.ts @@ -0,0 +1,7 @@ +import '@testing-library/jest-dom/vitest'; +import { afterEach } from 'vitest'; +import { cleanup } from '@testing-library/react'; + +afterEach(() => { + cleanup(); +}); diff --git a/vitest.config.ts b/vitest.config.ts index 6431215..d1b4e65 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -1,5 +1,6 @@ import { defineConfig } from 'vitest/config'; import react from '@vitejs/plugin-react'; +import { playwright } from '@vitest/browser-playwright'; import { resolve } from 'path'; export default defineConfig({ @@ -15,9 +16,32 @@ export default defineConfig({ }, test: { globals: true, - environment: 'node', - include: ['test/**/*.spec.ts', 'test/**/*.spec.tsx'], - setupFiles: ['./test/setup.ts'], - fileParallelism: false, + projects: [ + { + extends: true, + test: { + name: 'node', + environment: 'node', + include: ['test/**/*.spec.ts'], + exclude: ['test/browser/**'], + setupFiles: ['./test/setup.ts'], + fileParallelism: false, + }, + }, + { + extends: true, + test: { + name: 'browser', + include: ['test/browser/**/*.spec.tsx'], + setupFiles: ['./test/browser/setup.ts'], + browser: { + enabled: true, + headless: true, + provider: playwright(), + instances: [{ browser: 'chromium' }], + }, + }, + }, + ], }, }); From cfa3e1208ea6a2e87af7bee28be313924fa12d72 Mon Sep 17 00:00:00 2001 From: Thomas Michnicki Date: Sun, 12 Jul 2026 08:07:22 +0200 Subject: [PATCH 2/8] test: add jobs list and theme toggle browser interaction tests Covers real click/type/select interactions the previous jsdom-only setup couldn't exercise: search debouncing and page reset, the Radix status dropdown, the filtered empty state, pagination controls, and theme persistence via real matchMedia/localStorage. --- test/browser/jobs.spec.tsx | 119 ++++++++++++++++++++++++++++++ test/browser/theme-login.spec.tsx | 42 +++++++++++ 2 files changed, 161 insertions(+) create mode 100644 test/browser/jobs.spec.tsx create mode 100644 test/browser/theme-login.spec.tsx diff --git a/test/browser/jobs.spec.tsx b/test/browser/jobs.spec.tsx new file mode 100644 index 0000000..80361df --- /dev/null +++ b/test/browser/jobs.spec.tsx @@ -0,0 +1,119 @@ +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { screen, waitFor } from '@testing-library/react'; +import userEvent from '@testing-library/user-event'; +import { JobsPage } from '@client/pages/jobs'; +import { api } from '@client/lib/api'; +import { renderPage } from './render'; + +vi.mock('@client/lib/api', () => ({ + api: { + getJobs: vi.fn(), + getDlqMessages: vi.fn(), + getUpdatesEmailStatus: vi.fn(), + subscribeUpdates: vi.fn(), + }, +})); + +function makeJob(overrides: Partial> = {}) { + return { + id: '1', + owner: 'test-owner', + repo: 'test-repo', + prNumber: 101, + prTitle: 'Fixing bug', + status: 'done', + trigger: 'auto', + createdAt: new Date().toISOString(), + commentCount: 2, + ...overrides, + } as any; +} + +describe('JobsPage filters and pagination', () => { + beforeEach(() => { + vi.clearAllMocks(); + vi.mocked(api.getDlqMessages).mockResolvedValue({ messages: [], count: 0 }); + vi.mocked(api.getJobs).mockResolvedValue({ jobs: [makeJob()], total: 1 }); + vi.mocked(api.getUpdatesEmailStatus).mockResolvedValue({ + status: 'subscribed', + email: 'user@example.com', + updatedAt: new Date().toISOString(), + }); + }); + + it('renders jobs from the initial load', async () => { + renderPage(); + + expect(await screen.findByText('test-owner/test-repo')).toBeInTheDocument(); + expect(screen.getByRole('link', { name: 'Fixing bug' })).toBeInTheDocument(); + }); + + it('sends the search term and resets to page 1', async () => { + const user = userEvent.setup(); + renderPage(); + + await screen.findByText('test-owner/test-repo'); + + const searchInput = screen.getByPlaceholderText('Title or #number…'); + await user.type(searchInput, 'flaky'); + + await waitFor(() => { + const lastCall = vi.mocked(api.getJobs).mock.calls.at(-1)?.[0]; + expect(lastCall).toMatchObject({ search: 'flaky', offset: 0 }); + }); + }); + + it('filters by status through the dropdown', async () => { + const user = userEvent.setup(); + renderPage(); + + await screen.findByText('test-owner/test-repo'); + + await user.click(screen.getByRole('button', { name: /All statuses/i })); + await user.click(await screen.findByRole('menuitem', { name: 'Done' })); + + await waitFor(() => { + const lastCall = vi.mocked(api.getJobs).mock.calls.at(-1)?.[0]; + expect(lastCall).toMatchObject({ status: 'done', offset: 0 }); + }); + }); + + it('shows a filtered empty state when no jobs match', async () => { + vi.mocked(api.getJobs).mockResolvedValue({ jobs: [], total: 0 }); + const user = userEvent.setup(); + renderPage(); + + const searchInput = await screen.findByPlaceholderText('Title or #number…'); + await user.type(searchInput, 'nonexistent'); + + expect(await screen.findByText('No jobs match your filters. Try adjusting them.')).toBeInTheDocument(); + }); + + it('paginates using the Next and Prev controls', async () => { + vi.mocked(api.getJobs).mockResolvedValue({ + jobs: [makeJob()], + total: 45, // 3 pages at limit=20 + }); + + const user = userEvent.setup(); + renderPage(); + + expect(await screen.findByText(/Page 1 of 3/)).toBeInTheDocument(); + const prevButton = screen.getByRole('button', { name: /Prev/i }); + expect(prevButton).toBeDisabled(); + + await user.click(screen.getByRole('button', { name: /Next/i })); + + await waitFor(() => { + const lastCall = vi.mocked(api.getJobs).mock.calls.at(-1)?.[0]; + expect(lastCall).toMatchObject({ offset: 20 }); + }); + expect(await screen.findByText(/Page 2 of 3/)).toBeInTheDocument(); + + await user.click(screen.getByRole('button', { name: /Prev/i })); + await waitFor(() => { + const lastCall = vi.mocked(api.getJobs).mock.calls.at(-1)?.[0]; + expect(lastCall).toMatchObject({ offset: 0 }); + }); + }); +}); diff --git a/test/browser/theme-login.spec.tsx b/test/browser/theme-login.spec.tsx new file mode 100644 index 0000000..e390c26 --- /dev/null +++ b/test/browser/theme-login.spec.tsx @@ -0,0 +1,42 @@ +import { describe, it, expect, beforeEach } from 'vitest'; +import { screen } from '@testing-library/react'; +import userEvent from '@testing-library/user-event'; +import { LoginPage } from '@client/pages/login'; +import { renderPage } from './render'; + +describe('Theme toggle', () => { + beforeEach(() => { + localStorage.setItem('codra-theme', 'light'); + document.documentElement.classList.remove('dark'); + document.documentElement.setAttribute('data-theme', 'light'); + }); + + it('starts in the seeded light theme', async () => { + renderPage(); + + expect(document.documentElement.classList.contains('dark')).toBe(false); + }); + + it('toggles to dark and persists the choice in localStorage', async () => { + const user = userEvent.setup(); + renderPage(); + + await user.click(screen.getByRole('button', { name: 'Toggle theme' })); + + expect(document.documentElement.classList.contains('dark')).toBe(true); + expect(document.documentElement.getAttribute('data-theme')).toBe('dark'); + expect(localStorage.getItem('codra-theme')).toBe('dark'); + }); + + it('toggles back to light on a second click', async () => { + const user = userEvent.setup(); + renderPage(); + + const toggleButton = screen.getByRole('button', { name: 'Toggle theme' }); + await user.click(toggleButton); + await user.click(toggleButton); + + expect(document.documentElement.classList.contains('dark')).toBe(false); + expect(localStorage.getItem('codra-theme')).toBe('light'); + }); +}); From 0965c2f9194ad23d4bcc8ca26756152abf551155 Mon Sep 17 00:00:00 2001 From: Thomas Michnicki Date: Sun, 12 Jul 2026 08:07:27 +0200 Subject: [PATCH 3/8] test: add settings, repos, and job-detail browser interaction tests Exercises the largest untested page surfaces: provider API-key editing and save-failure handling in Settings, repo enable/pause toggling and GitHub sync in Repos, and finding expansion plus the re-run/retry button in the job detail view. --- test/browser/job-detail.spec.tsx | 158 +++++++++++++++++++++++++++++++ test/browser/repos.spec.tsx | 84 ++++++++++++++++ test/browser/settings.spec.tsx | 121 +++++++++++++++++++++++ 3 files changed, 363 insertions(+) create mode 100644 test/browser/job-detail.spec.tsx create mode 100644 test/browser/repos.spec.tsx create mode 100644 test/browser/settings.spec.tsx diff --git a/test/browser/job-detail.spec.tsx b/test/browser/job-detail.spec.tsx new file mode 100644 index 0000000..695c0cc --- /dev/null +++ b/test/browser/job-detail.spec.tsx @@ -0,0 +1,158 @@ +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { render, screen, waitFor } from '@testing-library/react'; +import userEvent from '@testing-library/user-event'; +import { MemoryRouter, Routes, Route } from 'react-router-dom'; +import { JobDetailPage } from '@client/pages/job-detail'; +import { api } from '@client/lib/api'; +import { ThemeProvider } from '@client/lib/theme'; +import type { JobDetail } from '@shared/schema'; + +vi.mock('@client/lib/api', () => ({ + api: { + getJob: vi.fn(), + retryJob: vi.fn(), + getUpdatesEmailStatus: vi.fn(), + subscribeUpdates: vi.fn(), + }, +})); + +const JOB_ID = '22222222-2222-2222-2222-222222222222'; + +const JOB: JobDetail = { + id: JOB_ID, + owner: 'acme', + repo: 'widgets', + installationId: '1', + prNumber: 42, + prTitle: 'Add retry handling', + prAuthor: 'octocat', + commitSha: 'abc123def456', + trigger: 'auto', + status: 'done', + verdict: 'comment', + fileCount: 1, + commentCount: 2, + totalInputTokens: 1200, + totalOutputTokens: 400, + createdAt: new Date().toISOString(), + updatedAt: new Date().toISOString(), + nextRetryAt: null, + startedAt: new Date().toISOString(), + finishedAt: new Date().toISOString(), + errorMessage: null, + steps: [], + checkRunId: null, + configSnapshot: null, + retryOfJobId: null, + baseSha: 'base123', + headRef: 'feature-branch', + baseRef: 'main', + summaryMarkdown: null, + reviewId: null, + summaryModel: null, + files: [ + { + id: '33333333-3333-3333-3333-333333333333', + jobId: JOB_ID, + filePath: 'src/index.ts', + fileStatus: 'done', + modelUsed: 'gpt-4o-mini', + diffLineCount: 12, + diffInput: null, + rawAiOutput: null, + parsedComments: [ + { + path: 'src/index.ts', + line: 10, + position: 3, + severity: 'P0', + category: 'security', + title: 'SQL injection risk', + body: 'User input is concatenated directly into the query string.', + codeSuggestion: null, + }, + { + path: 'src/index.ts', + line: 20, + position: 8, + severity: 'nit', + category: 'quality', + title: 'Prefer const over let', + body: 'This binding is never reassigned.', + codeSuggestion: null, + }, + ], + inputTokens: 1200, + outputTokens: 400, + durationMs: 2500, + verdict: 'comment', + fileSummary: 'Found one security issue and one style nit.', + errorMessage: null, + createdAt: new Date().toISOString(), + }, + ], +}; + +function renderJobDetail() { + return render( + + + + } /> + + + , + ); +} + +describe('JobDetailPage findings and retry', () => { + beforeEach(() => { + vi.clearAllMocks(); + vi.mocked(api.getJob).mockResolvedValue({ + status: 200, + etag: null, + lastModified: null, + notModified: false, + data: { job: JOB }, + }); + vi.mocked(api.getUpdatesEmailStatus).mockResolvedValue({ + status: 'subscribed', + email: 'user@example.com', + updatedAt: new Date().toISOString(), + }); + }); + + it('renders the job verdict and file findings', async () => { + renderJobDetail(); + + expect(await screen.findByText('Add retry handling')).toBeInTheDocument(); + expect(screen.getByText('src/index.ts', { selector: 'summary span' })).toBeInTheDocument(); + }); + + it('expands a file to reveal its inline findings', async () => { + const user = userEvent.setup(); + renderJobDetail(); + + const fileSummary = await screen.findByText('src/index.ts', { selector: 'summary span' }); + await user.click(fileSummary); + + expect(await screen.findByText('SQL injection risk')).toBeInTheDocument(); + expect(screen.getByText('Prefer const over let')).toBeInTheDocument(); + }); + + it('triggers a retry when the re-run button is clicked', async () => { + vi.mocked(api.retryJob).mockResolvedValue({ + job: { ...JOB, id: 'new-job-id', status: 'queued' } as any, + }); + + const user = userEvent.setup(); + renderJobDetail(); + + await screen.findByText('Add retry handling'); + await user.click(screen.getByRole('button', { name: 'Re-run job' })); + + await waitFor(() => { + expect(api.retryJob).toHaveBeenCalledWith(JOB_ID); + }); + }); +}); diff --git a/test/browser/repos.spec.tsx b/test/browser/repos.spec.tsx new file mode 100644 index 0000000..5356aa7 --- /dev/null +++ b/test/browser/repos.spec.tsx @@ -0,0 +1,84 @@ +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { screen, waitFor } from '@testing-library/react'; +import userEvent from '@testing-library/user-event'; +import { ReposPage } from '@client/pages/repos'; +import { api } from '@client/lib/api'; +import { renderPage } from './render'; +import type { RepoConfigRecord } from '@shared/schema'; + +vi.mock('@client/lib/api', () => ({ + api: { + getRepos: vi.fn(), + getGlobalConfig: vi.fn(), + getModelConfigs: vi.fn(), + updateRepoConfig: vi.fn(), + syncRepos: vi.fn(), + getUpdatesEmailStatus: vi.fn(), + subscribeUpdates: vi.fn(), + }, +})); + +const REPO: RepoConfigRecord = { + installationId: '1', + owner: 'acme', + repo: 'widgets', + parsedJson: {} as any, + updatedAt: new Date().toISOString(), + lastJobCreatedAt: null, + lastJobVerdict: null, + mainModel: null, + fallbackModels: null, + sizeOverrides: null, + enabled: true, +}; + +describe('ReposPage repository management', () => { + beforeEach(() => { + vi.clearAllMocks(); + vi.mocked(api.getRepos).mockResolvedValue({ repos: [REPO] }); + vi.mocked(api.getGlobalConfig).mockResolvedValue({ config: { main: null, fallbacks: [], size_overrides: [] } }); + vi.mocked(api.getModelConfigs).mockResolvedValue({ providers: [], configs: [], syncErrors: [] }); + vi.mocked(api.getUpdatesEmailStatus).mockResolvedValue({ + status: 'subscribed', + email: 'user@example.com', + updatedAt: new Date().toISOString(), + }); + }); + + it('renders the repo list with its enabled state', async () => { + renderPage(); + + expect(await screen.findByText('acme/widgets')).toBeInTheDocument(); + expect(screen.getByText('Enabled')).toBeInTheDocument(); + }); + + it('toggling the enabled switch patches the repo config', async () => { + vi.mocked(api.updateRepoConfig).mockResolvedValue({ ok: true }); + const user = userEvent.setup(); + renderPage(); + + await screen.findByText('acme/widgets'); + + await user.click(screen.getByRole('checkbox', { name: 'Pause reviews for acme/widgets' })); + + await waitFor(() => { + expect(api.updateRepoConfig).toHaveBeenCalledWith('acme', 'widgets', { enabled: false }); + }); + }); + + it('syncing repositories calls the sync endpoint and reloads the list', async () => { + vi.mocked(api.syncRepos).mockResolvedValue({ ok: true, synced: ['acme/widgets'] }); + const user = userEvent.setup(); + renderPage(); + + await screen.findByText('acme/widgets'); + vi.mocked(api.getRepos).mockClear(); + + await user.click(screen.getByRole('button', { name: /Sync/i })); + + await waitFor(() => { + expect(api.syncRepos).toHaveBeenCalled(); + expect(api.getRepos).toHaveBeenCalled(); + }); + }); +}); diff --git a/test/browser/settings.spec.tsx b/test/browser/settings.spec.tsx new file mode 100644 index 0000000..6d4ccf3 --- /dev/null +++ b/test/browser/settings.spec.tsx @@ -0,0 +1,121 @@ +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { screen, waitFor } from '@testing-library/react'; +import userEvent from '@testing-library/user-event'; +import { SettingsPage } from '@client/pages/settings'; +import { api } from '@client/lib/api'; +import { renderPage } from './render'; +import type { LlmProvider, ModelConfig } from '@shared/schema'; +import type { ModelConfigsResponse } from '@shared/api'; + +vi.mock('@client/lib/api', () => ({ + api: { + getModelConfigs: vi.fn(), + getGlobalConfig: vi.fn(), + refreshModelCatalog: vi.fn(), + updateProvider: vi.fn(), + createProvider: vi.fn(), + deleteProvider: vi.fn(), + updateModelConfig: vi.fn(), + deleteModelConfig: vi.fn(), + testModelConfig: vi.fn(), + updateGlobalConfig: vi.fn(), + getUpdatesEmailStatus: vi.fn(), + subscribeUpdates: vi.fn(), + }, +})); + +const PROVIDER: LlmProvider = { + id: '11111111-1111-1111-1111-111111111111', + name: 'Custom OpenAI', + apiFormat: 'openai', + baseUrl: 'https://api.example.com/v1', + enabled: false, + hasApiKey: false, + createdAt: new Date().toISOString(), + updatedAt: new Date().toISOString(), +}; + +const MODEL_CONFIG: ModelConfig = { + modelId: 'gpt-4o-mini', + providerId: PROVIDER.id, + providerName: PROVIDER.name, + apiFormat: 'openai', + modelName: 'gpt-4o-mini', + rpm: null, + tpm: null, + rpd: null, + updatedAt: new Date().toISOString(), +}; + +function modelConfigsResponse(overrides: Partial = {}): ModelConfigsResponse { + return { + providers: [PROVIDER], + configs: [MODEL_CONFIG], + syncErrors: [], + ...overrides, + }; +} + +describe('SettingsPage provider management', () => { + beforeEach(() => { + vi.clearAllMocks(); + vi.mocked(api.getModelConfigs).mockResolvedValue(modelConfigsResponse()); + vi.mocked(api.refreshModelCatalog).mockResolvedValue(modelConfigsResponse()); + vi.mocked(api.getGlobalConfig).mockResolvedValue({ config: { main: null, fallbacks: [], size_overrides: [] } }); + vi.mocked(api.getUpdatesEmailStatus).mockResolvedValue({ + status: 'subscribed', + email: 'user@example.com', + updatedAt: new Date().toISOString(), + }); + }); + + it('renders providers and models loaded from the API', async () => { + renderPage(); + + expect((await screen.findAllByText('Custom OpenAI')).length).toBeGreaterThan(0); + expect(screen.getAllByText('gpt-4o-mini').length).toBeGreaterThan(0); + }); + + it('edits a provider API key and saves the expected payload', async () => { + vi.mocked(api.updateProvider).mockResolvedValue({ + provider: { ...PROVIDER, hasApiKey: true }, + }); + + const user = userEvent.setup(); + renderPage(); + + await screen.findAllByText('Custom OpenAI'); + + await user.click(screen.getByRole('button', { name: 'Configure' })); + const apiKeyInput = await screen.findByPlaceholderText('sk-…'); + await user.type(apiKeyInput, 'sk-test-123'); + + await user.click(screen.getByRole('button', { name: 'Save' })); + + await waitFor(() => { + expect(api.updateProvider).toHaveBeenCalledWith(PROVIDER.id, { + name: 'Custom OpenAI', + apiFormat: 'openai', + baseUrl: 'https://api.example.com/v1', + enabled: false, + apiKey: 'sk-test-123', + }); + }); + }); + + it('shows an error alert when saving a provider fails', async () => { + vi.mocked(api.updateProvider).mockRejectedValue(new Error('Provider update failed: bad credentials')); + + const user = userEvent.setup(); + renderPage(); + + await screen.findAllByText('Custom OpenAI'); + + await user.click(screen.getByRole('button', { name: 'Configure' })); + const apiKeyInput = await screen.findByPlaceholderText('sk-…'); + await user.type(apiKeyInput, 'sk-test-123'); + await user.click(screen.getByRole('button', { name: 'Save' })); + + expect(await screen.findByText('Provider update failed: bad credentials')).toBeInTheDocument(); + }); +}); From fd2193dae7e958b2fe7c197a7258b281d26d2d8c Mon Sep 17 00:00:00 2001 From: Thomas Michnicki Date: Sun, 12 Jul 2026 08:07:33 +0200 Subject: [PATCH 4/8] test: add unit tests for FormatterService The PR comment/summary formatter had zero test coverage despite being on the critical path for every posted review. Covers verdict mapping, severity icons, legacy tag stripping, title/body dedupe, and the review overview markdown. --- test/formatter.spec.ts | 148 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 148 insertions(+) create mode 100644 test/formatter.spec.ts diff --git a/test/formatter.spec.ts b/test/formatter.spec.ts new file mode 100644 index 0000000..4585ee5 --- /dev/null +++ b/test/formatter.spec.ts @@ -0,0 +1,148 @@ +import { describe, it, expect } from 'vitest'; +import { FormatterService } from '@server/services/formatter'; +import type { ParsedReviewComment } from '@shared/schema'; + +const BASE_URL = 'https://app.codra.example.com'; +const formatter = new FormatterService(BASE_URL); + +function comment(overrides: Partial = {}): ParsedReviewComment { + return { + path: 'src/index.ts', + line: 10, + position: 3, + severity: 'P1', + category: 'quality', + title: 'Example finding', + body: 'Something worth looking at.', + codeSuggestion: null, + ...overrides, + }; +} + +describe('FormatterService.toReviewEvent', () => { + it('maps approve to APPROVE', () => { + expect(formatter.toReviewEvent('approve')).toBe('APPROVE'); + }); + + it('maps comment to COMMENT', () => { + expect(formatter.toReviewEvent('comment')).toBe('COMMENT'); + }); +}); + +describe('FormatterService.severityIcon', () => { + it.each(['P0', 'P1', 'P2', 'P3', 'nit'] as const)('renders an icon for %s', (severity) => { + const icon = formatter.severityIcon(severity); + expect(icon).toContain(` { + expect(formatter.severityIcon('unknown' as ParsedReviewComment['severity'])).toBe('⚪'); + }); +}); + +describe('FormatterService.stripLeadingTags', () => { + it('strips a leading emoji', () => { + expect(formatter.stripLeadingTags('🔒 Possible SQL injection')).toBe('Possible SQL injection'); + }); + + it('strips legacy bracketed severity/category tags', () => { + expect(formatter.stripLeadingTags('[P0] [SECURITY] Possible SQL injection')).toBe('Possible SQL injection'); + }); + + it('strips a leading bracketed BUG tag', () => { + expect(formatter.stripLeadingTags('[BUG] Off-by-one error')).toBe('Off-by-one error'); + }); + + it('strips bare (unbracketed) legacy tag words', () => { + expect(formatter.stripLeadingTags('SECURITY: Possible SQL injection')).toBe('Possible SQL injection'); + }); + + it('strips mixed emoji and bracketed tags together', () => { + expect(formatter.stripLeadingTags('🐛 [P2] [BUG] Null pointer dereference')).toBe('Null pointer dereference'); + }); + + it('leaves already-clean text untouched', () => { + expect(formatter.stripLeadingTags('Possible SQL injection')).toBe('Possible SQL injection'); + }); +}); + +describe('FormatterService.formatInlineComment', () => { + it('includes the severity icon and title', () => { + const output = formatter.formatInlineComment(comment({ severity: 'P0', title: 'SQL injection risk', body: 'Details here.' })); + expect(output).toContain(`SQL injection risk'); + expect(output).toContain('Details here.'); + }); + + it('dedupes a body whose first line repeats the title', () => { + const output = formatter.formatInlineComment( + comment({ title: 'SQL injection risk', body: 'SQL injection risk\n\nUser input is concatenated into the query.' }), + ); + expect(output).toBe( + `${formatter.severityIcon('P1')} SQL injection risk\n\nUser input is concatenated into the query.`, + ); + expect(output.match(/SQL injection risk/g)).toHaveLength(1); + }); + + it('dedupes when the title itself is a prefix of the first body line', () => { + const output = formatter.formatInlineComment( + comment({ title: 'SQL injection', body: 'SQL injection risk in query builder\n\nUser input is concatenated into the query.' }), + ); + expect(output.match(/SQL injection/g)).toHaveLength(1); + expect(output).toContain('User input is concatenated into the query.'); + }); + + it('keeps the body intact when it does not duplicate the title', () => { + const output = formatter.formatInlineComment( + comment({ title: 'Null check missing', body: 'This value can be undefined at runtime.' }), + ); + expect(output).toContain('Null check missing'); + expect(output).toContain('This value can be undefined at runtime.'); + }); +}); + +describe('FormatterService.summarizeVerdict', () => { + it('approves when there are no P0/P1/P2 comments and no failures', () => { + expect(formatter.summarizeVerdict([comment({ severity: 'P3' }), comment({ severity: 'nit' })], false)).toEqual({ + verdict: 'approve', + errors: 0, + warnings: 0, + }); + }); + + it('requires comment verdict with P0 findings, counted as errors', () => { + const result = formatter.summarizeVerdict([comment({ severity: 'P0' }), comment({ severity: 'P0' })], false); + expect(result).toEqual({ verdict: 'comment', errors: 2, warnings: 0 }); + }); + + it('requires comment verdict with P1 findings, counted as errors', () => { + const result = formatter.summarizeVerdict([comment({ severity: 'P1' })], false); + expect(result).toEqual({ verdict: 'comment', errors: 1, warnings: 0 }); + }); + + it('requires comment verdict with P2 findings, counted as warnings only', () => { + const result = formatter.summarizeVerdict([comment({ severity: 'P2' })], false); + expect(result).toEqual({ verdict: 'comment', errors: 0, warnings: 1 }); + }); + + it('requires comment verdict when hasFailures is true even with no comments', () => { + expect(formatter.summarizeVerdict([], true)).toEqual({ verdict: 'comment', errors: 0, warnings: 0 }); + }); +}); + +describe('FormatterService.formatReviewOverview', () => { + it('truncates the commit sha to 10 characters and interpolates the bot username', () => { + const output = formatter.formatReviewOverview('abcdef1234567890', 'codra-app'); + + expect(output).toContain('**Reviewed commit:** `abcdef1234`'); + expect(output).toContain('@codra-app review'); + expect(output).toContain('@codra-app address that feedback'); + expect(output).toContain(`${BASE_URL}/repos`); + }); + + it('handles a short commit sha without throwing', () => { + const output = formatter.formatReviewOverview('abc', 'codra-app'); + expect(output).toContain('**Reviewed commit:** `abc`'); + }); +}); From c21ca1fcc360cd06f122bab13d02984fa7f3c17e Mon Sep 17 00:00:00 2001 From: Thomas Michnicki Date: Sun, 12 Jul 2026 08:07:40 +0200 Subject: [PATCH 5/8] test: add end-to-end PR review pipeline test against real GitHubClient Every existing review-flow test mocks @server/services/github and @server/services/model entirely, so the real GitHubClient (check runs, review posting, the 422 retry-without-comments fallback, label lifecycle) and the real ModelService were never exercised together. Adds installGitHubFetchMock, which stubs global fetch at the api.github.com boundary instead of mocking the service modules, plus test helpers to seed a cached installation token (bypassing JWT/crypto entirely) and a default model strategy. The new spec drives a signed webhook through the full prepare/review/finalize pipeline with zero vi.mock calls, asserting on the actual GitHub API requests: check-run lifecycle, inline comment positions computed from a real diff, review body formatting, and label create/add. Also extracts the webhook signing helper out of webhook-handling.spec into test/helpers so both specs share it. --- test/github-fetch-mock.ts | 131 ++++++++++++++++ test/helpers.ts | 62 ++++++++ test/pr-review-pipeline.spec.ts | 255 ++++++++++++++++++++++++++++++++ test/webhook-handling.spec.ts | 17 +-- 4 files changed, 449 insertions(+), 16 deletions(-) create mode 100644 test/github-fetch-mock.ts create mode 100644 test/pr-review-pipeline.spec.ts diff --git a/test/github-fetch-mock.ts b/test/github-fetch-mock.ts new file mode 100644 index 0000000..81936a1 --- /dev/null +++ b/test/github-fetch-mock.ts @@ -0,0 +1,131 @@ +import { vi } from 'vitest'; + +export type RecordedGitHubCall = { + method: string; + path: string; + accept: string | null; + body: any; +}; + +export type ReviewResponseScript = Array<{ status: number; id?: number }>; + +export type GitHubFetchMockFixtures = { + owner: string; + repo: string; + prNumber: number; + pull: { + number: number; + title: string | null; + body: string | null; + draft: boolean; + head: { sha: string; ref: string }; + base: { sha: string; ref: string }; + user: { login: string }; + }; + diff: string; + /** Scripted status sequence for successive POST .../reviews calls. Defaults to a single 200. */ + reviewResponses?: ReviewResponseScript; +}; + +/** + * Stubs global fetch so the real GitHubClient (core/github.ts) can run end-to-end + * against a fake api.github.com. Every response is terminal (2xx/404/422) so + * GitHubClient's retry/backoff logic never triggers a real-time sleep. + */ +export function installGitHubFetchMock(fixtures: GitHubFetchMockFixtures) { + const calls: RecordedGitHubCall[] = []; + const originalFetch = globalThis.fetch; + const repoPrefix = `/repos/${fixtures.owner}/${fixtures.repo}`; + const reviewResponses = fixtures.reviewResponses ?? [{ status: 200, id: 5150 }]; + let reviewCallIndex = 0; + + const existingLabels = new Map(); + const issueLabels = new Set(); + + async function handler(input: RequestInfo | URL, init?: RequestInit): Promise { + const rawUrl = typeof input === 'string' ? input : input instanceof Request ? input.url : String(input); + const url = new URL(rawUrl); + + if (url.hostname !== 'api.github.com') { + return originalFetch(input as any, init); + } + + const method = (init?.method ?? 'GET').toUpperCase(); + const headers = (init?.headers ?? {}) as Record; + const accept = headers.Accept ?? null; + let body: any = null; + if (typeof init?.body === 'string') { + try { + body = JSON.parse(init.body); + } catch { + body = init.body; + } + } + + calls.push({ method, path: url.pathname, accept, body }); + + const json = (data: unknown, status = 200) => + new Response(JSON.stringify(data), { status, headers: { 'content-type': 'application/json' } }); + + if (method === 'GET' && url.pathname === `${repoPrefix}/pulls/${fixtures.prNumber}`) { + if (accept === 'application/vnd.github.v3.diff') { + return new Response(fixtures.diff, { status: 200 }); + } + return json(fixtures.pull); + } + + if (method === 'POST' && url.pathname === `${repoPrefix}/check-runs`) { + return json({ id: 9001 }, 201); + } + + if (method === 'PATCH' && /\/check-runs\/\d+$/.test(url.pathname)) { + return json({}); + } + + if (method === 'POST' && url.pathname === `${repoPrefix}/pulls/${fixtures.prNumber}/reviews`) { + const script = reviewResponses[Math.min(reviewCallIndex, reviewResponses.length - 1)]; + reviewCallIndex += 1; + if (script.status >= 400) { + return json({ message: 'Unprocessable Entity' }, script.status); + } + return json({ id: script.id ?? 5150 }, script.status); + } + + const labelLookup = new RegExp(`^${repoPrefix}/labels/([^/]+)$`).exec(url.pathname); + if (method === 'GET' && labelLookup) { + const name = decodeURIComponent(labelLookup[1]); + return existingLabels.has(name) ? json({ name }) : json({ message: 'Not Found' }, 404); + } + + if (method === 'POST' && url.pathname === `${repoPrefix}/labels`) { + existingLabels.set(body.name, body.color); + return json({ name: body.name, color: body.color }, 201); + } + + if (method === 'GET' && url.pathname === `${repoPrefix}/issues/${fixtures.prNumber}/labels`) { + return json(Array.from(issueLabels, (name) => ({ name }))); + } + + if (method === 'POST' && url.pathname === `${repoPrefix}/issues/${fixtures.prNumber}/labels`) { + for (const name of body?.labels ?? []) issueLabels.add(name); + return json([]); + } + + const labelRemoval = new RegExp(`^${repoPrefix}/issues/${fixtures.prNumber}/labels/([^/]+)$`).exec(url.pathname); + if (method === 'DELETE' && labelRemoval) { + issueLabels.delete(decodeURIComponent(labelRemoval[1])); + return json([]); + } + + return json({ message: `Unhandled mock GitHub route: ${method} ${url.pathname}` }, 404); + } + + vi.stubGlobal('fetch', handler); + + return { + calls, + restore() { + vi.stubGlobal('fetch', originalFetch); + }, + }; +} diff --git a/test/helpers.ts b/test/helpers.ts index 1f5cabb..3525004 100644 --- a/test/helpers.ts +++ b/test/helpers.ts @@ -1,6 +1,8 @@ import type { AppBindings } from '@server/env'; import { encryptLlmApiKey } from '@server/core/llm-crypto'; import { queryRows } from '@server/db/client'; +import { updateModelConfig } from '@server/db/model-configs'; +import { updateGlobalConfig } from '@server/core/config'; export class MemoryKV { private readonly store = new Map(); @@ -186,6 +188,66 @@ ${lines.map((l) => `+${l}`).join('\n')}`; .join('\n'); } +/** + * Seeds a cached installation token so the real GitHubClient (core/github.ts) + * can run without touching APP_PRIVATE_KEY/GITHUB_APP_ID (which throw in + * createTestEnv). GitHubClient.getInstallationToken() checks this cache key + * before doing any JWT signing. + */ +export async function seedInstallationToken(env: AppBindings, installationId: string, token = 'test-installation-token') { + await env.APP_KV.put( + `install:${installationId}`, + JSON.stringify({ + token, + expiresAt: new Date(Date.now() + 60 * 60 * 1000).toISOString(), + }), + ); +} + +/** + * Registers a model as the global review strategy so the real ModelService + * (services/model.ts) can resolve a model instead of throwing "No review + * model strategy is configured". + */ +export async function seedDefaultModelStrategy(env: AppBindings, modelId: string, providerName = 'Cloudflare') { + const [provider] = await queryRows<{ id: string }>( + env, + `SELECT id FROM llm_providers WHERE name = $1`, + [providerName], + ); + if (!provider) { + throw new Error(`Test provider "${providerName}" not found; check that migrations seeded it.`); + } + + await updateModelConfig(env, { + modelId, + providerId: provider.id, + modelName: modelId, + rpm: null, + tpm: null, + rpd: null, + }); + await updateGlobalConfig(env, { main: modelId, fallbacks: [], size_overrides: [] }); +} + +/** + * Signs a raw webhook body the same way GitHub does, for driving /webhook directly. + */ +export async function signWebhookPayload(secret: string, payload: string) { + const encoder = new TextEncoder(); + const key = await crypto.subtle.importKey( + 'raw', + encoder.encode(secret), + { name: 'HMAC', hash: 'SHA-256' }, + false, + ['sign'], + ); + const signature = await crypto.subtle.sign('HMAC', key, encoder.encode(payload)); + return `sha256=${Array.from(new Uint8Array(signature)) + .map((b) => b.toString(16).padStart(2, '0')) + .join('')}`; +} + /** * Creates a mock GitHub Webhook payload for a PR opened event. */ diff --git a/test/pr-review-pipeline.spec.ts b/test/pr-review-pipeline.spec.ts new file mode 100644 index 0000000..360479e --- /dev/null +++ b/test/pr-review-pipeline.spec.ts @@ -0,0 +1,255 @@ +import { createApp } from '@server/app'; +import { runReviewJob } from '@server/core/review'; +import { runWithDb } from '@server/db/client'; +import { findExistingJobForHead } from '@server/db/jobs'; +import { + createMockPRWebhook, + createTestEnv, + generateMockDiff, + hasConfiguredTestDatabaseUrl, + seedDefaultModelStrategy, + seedInstallationToken, + signWebhookPayload, +} from './helpers'; +import { installGitHubFetchMock } from './github-fetch-mock'; + +const dbDescribe = hasConfiguredTestDatabaseUrl() ? describe : describe.skip; +const PIPELINE_TEST_TIMEOUT_MS = 60_000; +const MODEL_ID = 'test-review-model'; +const INSTALLATION_ID = 987654; + +const AI_WITH_ISSUE = JSON.stringify({ + findings: [ + { + title: 'Possible null dereference', + body: 'This value could be null at runtime and is not checked before use.', + priority: 0, + code_location: { line: 3 }, + }, + ], + overall_explanation: 'Found a null-safety issue.', + overall_correctness: 'patch is incorrect', + overall_confidence_score: 0.9, +}); + +const AI_CLEAN = JSON.stringify({ + findings: [], + overall_explanation: 'No issues found.', + overall_correctness: 'patch is correct', + overall_confidence_score: 0.95, +}); + +function fakeAiBinding(responseText: string) { + return { + async run() { + return { response: responseText, usage: { prompt_tokens: 120, completion_tokens: 40 } }; + }, + }; +} + +function buildFixture(repo: string, prNumber: number) { + const headSha = 'a'.repeat(40); + const baseSha = 'b'.repeat(40); + const diff = generateMockDiff([{ path: 'src/app.ts', content: 'line1\nline2\nline3\nline4\nline5' }]); + + const pull = { + number: prNumber, + title: 'Add null check', + body: 'This PR adds a null check.', + draft: false, + head: { sha: headSha, ref: 'feature' }, + base: { sha: baseSha, ref: 'main' }, + user: { login: 'octocat' }, + }; + + const webhookPayload = createMockPRWebhook({ + action: 'opened', + installation: { id: INSTALLATION_ID }, + repository: { name: repo, owner: { login: 'test-owner' } }, + pull_request: pull, + }); + + return { headSha, baseSha, diff, pull, webhookPayload }; +} + +async function postWebhook(app: ReturnType, env: ReturnType, payload: unknown) { + const body = JSON.stringify(payload); + const signature = await signWebhookPayload(env.GITHUB_APP_WEBHOOK_SECRET, body); + + return app.request( + 'http://codra.test/webhook', + { + method: 'POST', + headers: { + 'x-github-event': 'pull_request', + 'x-github-delivery': `delivery-${Date.now()}-${Math.random()}`, + 'x-hub-signature-256': signature, + 'content-type': 'application/json', + }, + body, + }, + env, + ); +} + +async function drainQueue(env: ReturnType) { + await runWithDb(env, async () => { + const queue = env.REVIEW_QUEUE as any; + while (queue.sent.length > 0) { + const next = queue.sent.shift(); + await runReviewJob(env, next); + } + }); +} + +dbDescribe('PR review pipeline (real GitHubClient + real ModelService)', () => { + const app = createApp(); + + it('posts a COMMENT review with correctly positioned inline comments and manages labels', async () => { + const repo = `pipeline-repo-${Date.now()}-comment`; + const env = createTestEnv({ AI: fakeAiBinding(AI_WITH_ISSUE) as any }); + await seedInstallationToken(env, String(INSTALLATION_ID)); + await seedDefaultModelStrategy(env, MODEL_ID); + + const { headSha, diff, pull, webhookPayload } = buildFixture(repo, 11); + const { calls, restore } = installGitHubFetchMock({ + owner: 'test-owner', + repo, + prNumber: 11, + pull, + diff, + }); + + try { + const response = await postWebhook(app, env, webhookPayload); + expect(response.status).toBe(202); + const json = await response.json() as any; + expect(json.message).toBe('queued'); + expect((env.REVIEW_QUEUE as any).sent).toHaveLength(1); + + await drainQueue(env); + + const finalJob = await findExistingJobForHead(env, { + owner: 'test-owner', + repo, + prNumber: 11, + commitSha: headSha, + trigger: 'auto', + }); + expect(finalJob?.status).toBe('done'); + + const checkRunCreate = calls.find((c) => c.method === 'POST' && c.path.endsWith('/check-runs')); + expect(checkRunCreate?.body).toMatchObject({ name: 'Codra', head_sha: headSha }); + + const checkRunUpdates = calls.filter((c) => c.method === 'PATCH' && /\/check-runs\/\d+$/.test(c.path)); + expect(checkRunUpdates.length).toBeGreaterThan(0); + expect(checkRunUpdates.at(-1)?.body).toMatchObject({ status: 'completed' }); + + const reviewCall = calls.find((c) => c.method === 'POST' && c.path.endsWith('/reviews')); + expect(reviewCall?.body).toMatchObject({ + commit_id: headSha, + event: 'COMMENT', + }); + expect(reviewCall?.body.comments).toEqual([ + expect.objectContaining({ + path: 'src/app.ts', + position: 3, + }), + ]); + expect(reviewCall?.body.comments[0].body).toContain('Possible null dereference'); + + const labelLookup = calls.find((c) => c.method === 'GET' && c.path.includes('/labels/')); + expect(labelLookup).toBeDefined(); + const labelCreate = calls.find((c) => c.method === 'POST' && c.path.endsWith('/labels') && !c.path.includes('/issues/')); + expect(labelCreate?.body).toMatchObject({ name: 'review: needs-attention' }); + const issueLabelAdd = calls.find((c) => c.method === 'POST' && /\/issues\/\d+\/labels$/.test(c.path)); + expect(issueLabelAdd?.body).toMatchObject({ labels: ['review: needs-attention'] }); + } finally { + restore(); + } + }, PIPELINE_TEST_TIMEOUT_MS); + + it('approves a clean PR with no inline comments', async () => { + const repo = `pipeline-repo-${Date.now()}-approve`; + const env = createTestEnv({ AI: fakeAiBinding(AI_CLEAN) as any }); + await seedInstallationToken(env, String(INSTALLATION_ID)); + await seedDefaultModelStrategy(env, MODEL_ID); + + const { headSha, diff, pull, webhookPayload } = buildFixture(repo, 22); + const { calls, restore } = installGitHubFetchMock({ + owner: 'test-owner', + repo, + prNumber: 22, + pull, + diff, + }); + + try { + const response = await postWebhook(app, env, webhookPayload); + expect(response.status).toBe(202); + + await drainQueue(env); + + const finalJob = await findExistingJobForHead(env, { + owner: 'test-owner', + repo, + prNumber: 22, + commitSha: headSha, + trigger: 'auto', + }); + expect(finalJob?.status).toBe('done'); + expect(finalJob?.verdict).toBe('approve'); + + const reviewCall = calls.find((c) => c.method === 'POST' && c.path.endsWith('/reviews')); + expect(reviewCall?.body).toMatchObject({ event: 'APPROVE', comments: [] }); + + const checkRunUpdates = calls.filter((c) => c.method === 'PATCH' && /\/check-runs\/\d+$/.test(c.path)); + expect(checkRunUpdates.at(-1)?.body).toMatchObject({ status: 'completed', conclusion: 'success' }); + + const labelCreate = calls.find((c) => c.method === 'POST' && c.path.endsWith('/labels') && !c.path.includes('/issues/')); + expect(labelCreate?.body).toMatchObject({ name: 'review: approved' }); + } finally { + restore(); + } + }, PIPELINE_TEST_TIMEOUT_MS); + + it('retries review creation without inline comments when GitHub returns 422', async () => { + const repo = `pipeline-repo-${Date.now()}-422`; + const env = createTestEnv({ AI: fakeAiBinding(AI_WITH_ISSUE) as any }); + await seedInstallationToken(env, String(INSTALLATION_ID)); + await seedDefaultModelStrategy(env, MODEL_ID); + + const { headSha, diff, pull, webhookPayload } = buildFixture(repo, 33); + const { calls, restore } = installGitHubFetchMock({ + owner: 'test-owner', + repo, + prNumber: 33, + pull, + diff, + reviewResponses: [{ status: 422 }, { status: 200, id: 7001 }], + }); + + try { + const response = await postWebhook(app, env, webhookPayload); + expect(response.status).toBe(202); + + await drainQueue(env); + + const finalJob = await findExistingJobForHead(env, { + owner: 'test-owner', + repo, + prNumber: 33, + commitSha: headSha, + trigger: 'auto', + }); + expect(finalJob?.status).toBe('done'); + + const reviewCalls = calls.filter((c) => c.method === 'POST' && c.path.endsWith('/reviews')); + expect(reviewCalls).toHaveLength(2); + expect(reviewCalls[0].body.comments.length).toBeGreaterThan(0); + expect(reviewCalls[1].body.comments).toEqual([]); + } finally { + restore(); + } + }, PIPELINE_TEST_TIMEOUT_MS); +}); diff --git a/test/webhook-handling.spec.ts b/test/webhook-handling.spec.ts index 965e603..9114211 100644 --- a/test/webhook-handling.spec.ts +++ b/test/webhook-handling.spec.ts @@ -1,5 +1,5 @@ import { createApp } from '@server/app'; -import { createMockPRWebhook, createTestEnv } from './helpers'; +import { createMockPRWebhook, createTestEnv, signWebhookPayload as signPayload } from './helpers'; import { vi } from 'vitest'; // Mock GitHubClient to avoid real JWT signing and network calls @@ -14,21 +14,6 @@ vi.mock('@server/core/github', async (importOriginal) => { }; }); -async function signPayload(secret: string, payload: string) { - const encoder = new TextEncoder(); - const key = await crypto.subtle.importKey( - 'raw', - encoder.encode(secret), - { name: 'HMAC', hash: 'SHA-256' }, - false, - ['sign'], - ); - const signature = await crypto.subtle.sign('HMAC', key, encoder.encode(payload)); - return `sha256=${Array.from(new Uint8Array(signature)) - .map((b) => b.toString(16).padStart(2, '0')) - .join('')}`; -} - describe('Webhook Handling Suite', () => { const env = createTestEnv(); const app = createApp(); From 289a250e848d7747a6c018d04a1aeb4ac1e8ab9f Mon Sep 17 00:00:00 2001 From: Thomas Michnicki Date: Sun, 12 Jul 2026 08:07:46 +0200 Subject: [PATCH 6/8] ci: install Chromium and run browser tests Adds a cached Playwright Chromium install step and a Browser Tests step to the existing verify job so the new UI interaction suite runs on every PR. Also ignores vitest browser-mode screenshot/attachment directories generated locally on failure. --- .github/workflows/ci.yml | 12 ++++++++++++ .gitignore | 4 ++++ 2 files changed, 16 insertions(+) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 56df09a..df7e7b4 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -60,8 +60,20 @@ jobs: - name: Install dependencies run: npm ci + - name: Cache Playwright browsers + uses: actions/cache@v4 + with: + path: ~/.cache/ms-playwright + key: playwright-${{ runner.os }}-${{ hashFiles('package-lock.json') }} + + - name: Install Playwright Chromium + run: npx playwright install --with-deps chromium + - name: Static Analysis (Typecheck) run: npm run typecheck - name: Automated Tests run: npm test + + - name: Browser Tests + run: npm run test:browser diff --git a/.gitignore b/.gitignore index e6c2e3a..b172741 100644 --- a/.gitignore +++ b/.gitignore @@ -143,3 +143,7 @@ vite.config.ts.timestamp-* .wrangler .agent + +# Vitest browser mode failure screenshots and attachments +test/browser/__screenshots__/ +.vitest-attachments/ From 6c552ddfa89ef1e21dbc6ab42e7f2fd6c5ca69ea Mon Sep 17 00:00:00 2001 From: Thomas Michnicki Date: Sun, 12 Jul 2026 08:17:34 +0200 Subject: [PATCH 7/8] fix: remove unused Cloudflare vitest pool and update vitest to patch npm audit findings @cloudflare/vitest-pool-workers was declared but never wired into vitest.config.ts or referenced anywhere in the codebase. Its pinned transitive deps (esbuild, miniflare, wrangler, undici, ws) accounted for 7 of the 9 reported vulnerabilities (3 critical, 5 high), so it's removed outright rather than force-upgraded to a breaking major. vitest/@vitest/browser/@vitest/browser-playwright were resolving to 4.1.7, which has a critical Vitest Browser Mode RCE advisory (GHSA-g8mr-85jm-7xhm). A clean reinstall (needed since these packages peer-pin each other to an exact matching version) resolved all three to the patched 4.1.10 within the existing ^4.1.4 range. npm audit now reports 0 vulnerabilities. Full node (119) and browser (19) suites verified green against the updated dependencies. --- package-lock.json | 103 +++++++++++++++++++++++++++++----------------- package.json | 1 - 2 files changed, 66 insertions(+), 38 deletions(-) diff --git a/package-lock.json b/package-lock.json index 2cffbcc..fdf8e23 100644 --- a/package-lock.json +++ b/package-lock.json @@ -36,7 +36,6 @@ "zod": "^4.3.6" }, "devDependencies": { - "@cloudflare/vitest-pool-workers": "^0.16.20", "@tailwindcss/vite": "^4.2.2", "@testing-library/dom": "^10.4.1", "@testing-library/jest-dom": "^6.9.1", @@ -201,35 +200,6 @@ } } }, - "node_modules/@cloudflare/vitest-pool-workers": { - "version": "0.16.20", - "resolved": "https://registry.npmjs.org/@cloudflare/vitest-pool-workers/-/vitest-pool-workers-0.16.20.tgz", - "integrity": "sha512-buw0YgsAMT7s60wcmyxbtciEJjMJzKcWzayDMPhWaqMqfQzW+0WPLV67Lobn4C80nkNQhYocEJPnrEhLWnOf+A==", - "dev": true, - "license": "MIT", - "dependencies": { - "cjs-module-lexer": "1.2.3", - "esbuild": "0.28.1", - "miniflare": "4.20260625.0", - "wrangler": "4.105.0", - "zod": "3.25.76" - }, - "peerDependencies": { - "@vitest/runner": "^4.1.0", - "@vitest/snapshot": "^4.1.0", - "vitest": "^4.1.0" - } - }, - "node_modules/@cloudflare/vitest-pool-workers/node_modules/zod": { - "version": "3.25.76", - "resolved": "https://registry.npmjs.org/zod/-/zod-3.25.76.tgz", - "integrity": "sha512-gzUt/qt81nXsFGKIFcC3YnfEAx5NkunCfnDlvuBSSFS02bcXu4Lmea0AFIUwbLWxWPx3d9p8S5QoaujKcNQxcQ==", - "dev": true, - "license": "MIT", - "funding": { - "url": "https://github.com/sponsors/colinhacks" - } - }, "node_modules/@cloudflare/workerd-darwin-64": { "version": "1.20260625.1", "resolved": "https://registry.npmjs.org/@cloudflare/workerd-darwin-64/-/workerd-darwin-64-1.20260625.1.tgz", @@ -2920,6 +2890,72 @@ "node": ">=14.0.0" } }, + "node_modules/@tailwindcss/oxide-wasm32-wasi/node_modules/@emnapi/core": { + "version": "1.11.1", + "dev": true, + "inBundle": true, + "license": "MIT", + "optional": true, + "dependencies": { + "@emnapi/wasi-threads": "1.2.2", + "tslib": "^2.4.0" + } + }, + "node_modules/@tailwindcss/oxide-wasm32-wasi/node_modules/@emnapi/runtime": { + "version": "1.11.1", + "dev": true, + "inBundle": true, + "license": "MIT", + "optional": true, + "dependencies": { + "tslib": "^2.4.0" + } + }, + "node_modules/@tailwindcss/oxide-wasm32-wasi/node_modules/@emnapi/wasi-threads": { + "version": "1.2.2", + "dev": true, + "inBundle": true, + "license": "MIT", + "optional": true, + "dependencies": { + "tslib": "^2.4.0" + } + }, + "node_modules/@tailwindcss/oxide-wasm32-wasi/node_modules/@napi-rs/wasm-runtime": { + "version": "1.1.4", + "dev": true, + "inBundle": true, + "license": "MIT", + "optional": true, + "dependencies": { + "@tybys/wasm-util": "^0.10.1" + }, + "funding": { + "type": "github", + "url": "https://github.com/sponsors/Brooooooklyn" + }, + "peerDependencies": { + "@emnapi/core": "^1.7.1", + "@emnapi/runtime": "^1.7.1" + } + }, + "node_modules/@tailwindcss/oxide-wasm32-wasi/node_modules/@tybys/wasm-util": { + "version": "0.10.2", + "dev": true, + "inBundle": true, + "license": "MIT", + "optional": true, + "dependencies": { + "tslib": "^2.4.0" + } + }, + "node_modules/@tailwindcss/oxide-wasm32-wasi/node_modules/tslib": { + "version": "2.8.1", + "dev": true, + "inBundle": true, + "license": "0BSD", + "optional": true + }, "node_modules/@tailwindcss/oxide-win32-arm64-msvc": { "version": "4.3.2", "resolved": "https://registry.npmjs.org/@tailwindcss/oxide-win32-arm64-msvc/-/oxide-win32-arm64-msvc-4.3.2.tgz", @@ -3600,13 +3636,6 @@ "url": "https://github.com/sponsors/wooorm" } }, - "node_modules/cjs-module-lexer": { - "version": "1.2.3", - "resolved": "https://registry.npmjs.org/cjs-module-lexer/-/cjs-module-lexer-1.2.3.tgz", - "integrity": "sha512-0TNiGstbQmCFwt4akjjBg5pLRTSyj/PkWQ1ZoO2zntmg9yLqSRxwEa4iCfQLGjqhiqBfOJa7W/E8wfGrTDmlZQ==", - "dev": true, - "license": "MIT" - }, "node_modules/class-variance-authority": { "version": "0.7.1", "resolved": "https://registry.npmjs.org/class-variance-authority/-/class-variance-authority-0.7.1.tgz", diff --git a/package.json b/package.json index bc673ff..52a08ba 100644 --- a/package.json +++ b/package.json @@ -29,7 +29,6 @@ "typecheck": "tsc --noEmit" }, "devDependencies": { - "@cloudflare/vitest-pool-workers": "^0.16.20", "@tailwindcss/vite": "^4.2.2", "@testing-library/dom": "^10.4.1", "@testing-library/jest-dom": "^6.9.1", From 914046a540c5023f7248755405c7ecca8235d9c3 Mon Sep 17 00:00:00 2001 From: Thomas Michnicki Date: Sun, 12 Jul 2026 09:08:26 +0200 Subject: [PATCH 8/8] test: fix suite against upstream's Workflow-based review rewrite Rebasing onto the new upstream main surfaced real breakage beyond merge conflicts: the review pipeline moved from queue re-enqueuing to Cloudflare Workflows, DLQ was removed, ModelConfig dropped its per-model rpm/tpm/rpd fields, job-detail gained separate rerun/stop/ delete actions, and the Select component switched from a Radix menu to a custom listbox. - pr-review-pipeline.spec.ts: runReviewJob no longer re-enqueues via REVIEW_QUEUE, it returns {action:'next_phase', phase, delaySeconds} for the caller to chase. Replaces the queue-drain loop with the same runAndDrain pattern review-flow.spec.ts uses, and defensively clears stray 'running' jobs before each test since global (unscoped) concurrency admission control otherwise throttles fresh jobs left over by other specs in the shared test DB. - github-fetch-mock.ts: stop passing non-GitHub hosts through to the real network (core/telemetry.ts fires a real POST to codra.run on every finalize) and add the GET .../reviews route findBotReviewForCommit uses. - helpers.ts: drop rpm/tpm/rpd from seedDefaultModelStrategy's updateModelConfig call. - jobs.spec.tsx: remove the deleted getDlqMessages mock, query Select options by role="option" instead of the old Radix "menuitem", and match the current pagination/empty-state copy. - settings.spec.tsx: drop rpm/tpm/rpd from the model fixture, mock the new getReviewSettings call the page now makes on load, and assert against the provider's model-count text instead of a since-removed flat model list. - job-detail.spec.tsx: switch from the removed retryJob to rerunJob. Full node (188) and browser (19) suites verified green, twice in a row to confirm the admission-control fix isn't order-dependent. --- test/browser/job-detail.spec.tsx | 8 ++--- test/browser/jobs.spec.tsx | 24 +++++++------- test/browser/settings.spec.tsx | 12 +++---- test/github-fetch-mock.ts | 13 ++++++-- test/helpers.ts | 3 -- test/pr-review-pipeline.spec.ts | 55 ++++++++++++++++++++++++++------ 6 files changed, 77 insertions(+), 38 deletions(-) diff --git a/test/browser/job-detail.spec.tsx b/test/browser/job-detail.spec.tsx index 695c0cc..21081d3 100644 --- a/test/browser/job-detail.spec.tsx +++ b/test/browser/job-detail.spec.tsx @@ -10,7 +10,7 @@ import type { JobDetail } from '@shared/schema'; vi.mock('@client/lib/api', () => ({ api: { getJob: vi.fn(), - retryJob: vi.fn(), + rerunJob: vi.fn(), getUpdatesEmailStatus: vi.fn(), subscribeUpdates: vi.fn(), }, @@ -140,8 +140,8 @@ describe('JobDetailPage findings and retry', () => { expect(screen.getByText('Prefer const over let')).toBeInTheDocument(); }); - it('triggers a retry when the re-run button is clicked', async () => { - vi.mocked(api.retryJob).mockResolvedValue({ + it('triggers a rerun when the re-run button is clicked', async () => { + vi.mocked(api.rerunJob).mockResolvedValue({ job: { ...JOB, id: 'new-job-id', status: 'queued' } as any, }); @@ -152,7 +152,7 @@ describe('JobDetailPage findings and retry', () => { await user.click(screen.getByRole('button', { name: 'Re-run job' })); await waitFor(() => { - expect(api.retryJob).toHaveBeenCalledWith(JOB_ID); + expect(api.rerunJob).toHaveBeenCalledWith(JOB_ID); }); }); }); diff --git a/test/browser/jobs.spec.tsx b/test/browser/jobs.spec.tsx index 80361df..0cddead 100644 --- a/test/browser/jobs.spec.tsx +++ b/test/browser/jobs.spec.tsx @@ -8,7 +8,6 @@ import { renderPage } from './render'; vi.mock('@client/lib/api', () => ({ api: { getJobs: vi.fn(), - getDlqMessages: vi.fn(), getUpdatesEmailStatus: vi.fn(), subscribeUpdates: vi.fn(), }, @@ -32,7 +31,6 @@ function makeJob(overrides: Partial> = {}) { describe('JobsPage filters and pagination', () => { beforeEach(() => { vi.clearAllMocks(); - vi.mocked(api.getDlqMessages).mockResolvedValue({ messages: [], count: 0 }); vi.mocked(api.getJobs).mockResolvedValue({ jobs: [makeJob()], total: 1 }); vi.mocked(api.getUpdatesEmailStatus).mockResolvedValue({ status: 'subscribed', @@ -54,7 +52,7 @@ describe('JobsPage filters and pagination', () => { await screen.findByText('test-owner/test-repo'); - const searchInput = screen.getByPlaceholderText('Title or #number…'); + const searchInput = screen.getByPlaceholderText('Title or #number...'); await user.type(searchInput, 'flaky'); await waitFor(() => { @@ -70,7 +68,7 @@ describe('JobsPage filters and pagination', () => { await screen.findByText('test-owner/test-repo'); await user.click(screen.getByRole('button', { name: /All statuses/i })); - await user.click(await screen.findByRole('menuitem', { name: 'Done' })); + await user.click(await screen.findByRole('option', { name: 'Done' })); await waitFor(() => { const lastCall = vi.mocked(api.getJobs).mock.calls.at(-1)?.[0]; @@ -78,39 +76,39 @@ describe('JobsPage filters and pagination', () => { }); }); - it('shows a filtered empty state when no jobs match', async () => { + it('shows the empty state when no jobs are returned', async () => { vi.mocked(api.getJobs).mockResolvedValue({ jobs: [], total: 0 }); const user = userEvent.setup(); renderPage(); - const searchInput = await screen.findByPlaceholderText('Title or #number…'); + const searchInput = await screen.findByPlaceholderText('Title or #number...'); await user.type(searchInput, 'nonexistent'); - expect(await screen.findByText('No jobs match your filters. Try adjusting them.')).toBeInTheDocument(); + expect(await screen.findByText('No jobs yet')).toBeInTheDocument(); }); - it('paginates using the Next and Prev controls', async () => { + it('paginates using the Next and Previous page controls', async () => { vi.mocked(api.getJobs).mockResolvedValue({ jobs: [makeJob()], - total: 45, // 3 pages at limit=20 + total: 25, // 3 pages at the default itemsPerPage of 10 }); const user = userEvent.setup(); renderPage(); expect(await screen.findByText(/Page 1 of 3/)).toBeInTheDocument(); - const prevButton = screen.getByRole('button', { name: /Prev/i }); + const prevButton = screen.getByRole('button', { name: 'Previous page' }); expect(prevButton).toBeDisabled(); - await user.click(screen.getByRole('button', { name: /Next/i })); + await user.click(screen.getByRole('button', { name: 'Next page' })); await waitFor(() => { const lastCall = vi.mocked(api.getJobs).mock.calls.at(-1)?.[0]; - expect(lastCall).toMatchObject({ offset: 20 }); + expect(lastCall).toMatchObject({ offset: 10 }); }); expect(await screen.findByText(/Page 2 of 3/)).toBeInTheDocument(); - await user.click(screen.getByRole('button', { name: /Prev/i })); + await user.click(screen.getByRole('button', { name: 'Previous page' })); await waitFor(() => { const lastCall = vi.mocked(api.getJobs).mock.calls.at(-1)?.[0]; expect(lastCall).toMatchObject({ offset: 0 }); diff --git a/test/browser/settings.spec.tsx b/test/browser/settings.spec.tsx index 6d4ccf3..c3b403e 100644 --- a/test/browser/settings.spec.tsx +++ b/test/browser/settings.spec.tsx @@ -11,13 +11,11 @@ vi.mock('@client/lib/api', () => ({ api: { getModelConfigs: vi.fn(), getGlobalConfig: vi.fn(), + getReviewSettings: vi.fn(), refreshModelCatalog: vi.fn(), updateProvider: vi.fn(), createProvider: vi.fn(), deleteProvider: vi.fn(), - updateModelConfig: vi.fn(), - deleteModelConfig: vi.fn(), - testModelConfig: vi.fn(), updateGlobalConfig: vi.fn(), getUpdatesEmailStatus: vi.fn(), subscribeUpdates: vi.fn(), @@ -41,9 +39,6 @@ const MODEL_CONFIG: ModelConfig = { providerName: PROVIDER.name, apiFormat: 'openai', modelName: 'gpt-4o-mini', - rpm: null, - tpm: null, - rpd: null, updatedAt: new Date().toISOString(), }; @@ -62,6 +57,7 @@ describe('SettingsPage provider management', () => { vi.mocked(api.getModelConfigs).mockResolvedValue(modelConfigsResponse()); vi.mocked(api.refreshModelCatalog).mockResolvedValue(modelConfigsResponse()); vi.mocked(api.getGlobalConfig).mockResolvedValue({ config: { main: null, fallbacks: [], size_overrides: [] } }); + vi.mocked(api.getReviewSettings).mockResolvedValue({ settings: { concurrencyLevel: 'medium', maxComments: 10 } }); vi.mocked(api.getUpdatesEmailStatus).mockResolvedValue({ status: 'subscribed', email: 'user@example.com', @@ -73,7 +69,9 @@ describe('SettingsPage provider management', () => { renderPage(); expect((await screen.findAllByText('Custom OpenAI')).length).toBeGreaterThan(0); - expect(screen.getAllByText('gpt-4o-mini').length).toBeGreaterThan(0); + // Proves the model catalog loaded: the provider row shows its model count. + expect(screen.getByText('· 1 model')).toBeInTheDocument(); + expect(screen.getByText('Default models')).toBeInTheDocument(); }); it('edits a provider API key and saves the expected payload', async () => { diff --git a/test/github-fetch-mock.ts b/test/github-fetch-mock.ts index 81936a1..116e7e3 100644 --- a/test/github-fetch-mock.ts +++ b/test/github-fetch-mock.ts @@ -36,6 +36,7 @@ export function installGitHubFetchMock(fixtures: GitHubFetchMockFixtures) { const calls: RecordedGitHubCall[] = []; const originalFetch = globalThis.fetch; const repoPrefix = `/repos/${fixtures.owner}/${fixtures.repo}`; + const reviewsListPath = `${repoPrefix}/pulls/${fixtures.prNumber}/reviews`; const reviewResponses = fixtures.reviewResponses ?? [{ status: 200, id: 5150 }]; let reviewCallIndex = 0; @@ -47,7 +48,9 @@ export function installGitHubFetchMock(fixtures: GitHubFetchMockFixtures) { const url = new URL(rawUrl); if (url.hostname !== 'api.github.com') { - return originalFetch(input as any, init); + // core/telemetry.ts fires a real POST to codra.run on every finalize; return a fast synthetic + // response instead of letting it reach the real network from a test run. + return new Response('{}', { status: 200, headers: { 'content-type': 'application/json' } }); } const method = (init?.method ?? 'GET').toUpperCase(); @@ -82,7 +85,13 @@ export function installGitHubFetchMock(fixtures: GitHubFetchMockFixtures) { return json({}); } - if (method === 'POST' && url.pathname === `${repoPrefix}/pulls/${fixtures.prNumber}/reviews`) { + if (method === 'GET' && url.pathname === reviewsListPath) { + // findBotReviewForCommit's existing-review lookup (only hit when a finalize retries past + // the posting step). No prior review exists in these fixtures. + return json([]); + } + + if (method === 'POST' && url.pathname === reviewsListPath) { const script = reviewResponses[Math.min(reviewCallIndex, reviewResponses.length - 1)]; reviewCallIndex += 1; if (script.status >= 400) { diff --git a/test/helpers.ts b/test/helpers.ts index 3525004..1ea8d64 100644 --- a/test/helpers.ts +++ b/test/helpers.ts @@ -223,9 +223,6 @@ export async function seedDefaultModelStrategy(env: AppBindings, modelId: string modelId, providerId: provider.id, modelName: modelId, - rpm: null, - tpm: null, - rpd: null, }); await updateGlobalConfig(env, { main: modelId, fallbacks: [], size_overrides: [] }); } diff --git a/test/pr-review-pipeline.spec.ts b/test/pr-review-pipeline.spec.ts index 360479e..d926696 100644 --- a/test/pr-review-pipeline.spec.ts +++ b/test/pr-review-pipeline.spec.ts @@ -1,6 +1,6 @@ import { createApp } from '@server/app'; import { runReviewJob } from '@server/core/review'; -import { runWithDb } from '@server/db/client'; +import { queryRows, runWithDb } from '@server/db/client'; import { findExistingJobForHead } from '@server/db/jobs'; import { createMockPRWebhook, @@ -92,16 +92,47 @@ async function postWebhook(app: ReturnType, env: ReturnType) { +// The review pipeline now runs through Cloudflare Workflows in production: runReviewJob no longer +// re-enqueues the next phase onto REVIEW_QUEUE itself, it returns `{action:'next_phase', phase, +// delaySeconds}` and the caller (normally ReviewWorkflow's step loop) decides whether to continue. +// This mirrors the `runAndDrain` helper in test/review-flow.spec.ts: drive runReviewJob directly, +// chasing the returned phase, and backdate last_queue_message_at so the next claim doesn't read as +// 'busy' (claimJobLease treats a future last_queue_message_at as a fresh lease held elsewhere). +async function runAndDrain(env: ReturnType, message: Parameters[1]) { await runWithDb(env, async () => { - const queue = env.REVIEW_QUEUE as any; - while (queue.sent.length > 0) { - const next = queue.sent.shift(); - await runReviewJob(env, next); + let currentMessage: typeof message | null = message; + let retries = 0; + const MAX_RETRIES = 5; + + while (currentMessage) { + const result = await runReviewJob(env, currentMessage); + if (result.action === 'next_phase') { + currentMessage = { ...currentMessage, phase: result.phase }; + retries = 0; + const jobId = (currentMessage as any).jobId; + if (jobId) { + await queryRows(env, `UPDATE jobs SET last_queue_message_at = now() - interval '5 seconds' WHERE id = $1`, [jobId]); + } + } else if (result.action === 'retry') { + if (++retries > MAX_RETRIES) throw new Error('Max retries exceeded'); + break; + } else { + currentMessage = null; + } } }); } +// runReviewJob throttles admission of a fresh 'queued' job once REVIEW_CONCURRENCY_LIMITS[level] +// other jobs are 'running' (globally, with no per-installation/repo scoping). Other spec files in +// this shared-DB suite intentionally leave jobs 'running' to exercise that same admission-control +// path, which would otherwise make this spec's outcome depend on file execution order. Since +// fileParallelism is false, nothing else touches the DB concurrently with this file, so it's safe +// to clear stray 'running' rows before each test. +async function clearStrayRunningJobs(env: ReturnType) { + await queryRows(env, `UPDATE jobs SET status = 'failed', error_msg = 'stray running job cleared before pipeline test' WHERE status = 'running'`); +} + dbDescribe('PR review pipeline (real GitHubClient + real ModelService)', () => { const app = createApp(); @@ -110,6 +141,7 @@ dbDescribe('PR review pipeline (real GitHubClient + real ModelService)', () => { const env = createTestEnv({ AI: fakeAiBinding(AI_WITH_ISSUE) as any }); await seedInstallationToken(env, String(INSTALLATION_ID)); await seedDefaultModelStrategy(env, MODEL_ID); + await clearStrayRunningJobs(env); const { headSha, diff, pull, webhookPayload } = buildFixture(repo, 11); const { calls, restore } = installGitHubFetchMock({ @@ -127,7 +159,8 @@ dbDescribe('PR review pipeline (real GitHubClient + real ModelService)', () => { expect(json.message).toBe('queued'); expect((env.REVIEW_QUEUE as any).sent).toHaveLength(1); - await drainQueue(env); + const initialMessage = (env.REVIEW_QUEUE as any).sent[0]; + await runAndDrain(env, initialMessage); const finalJob = await findExistingJobForHead(env, { owner: 'test-owner', @@ -174,6 +207,7 @@ dbDescribe('PR review pipeline (real GitHubClient + real ModelService)', () => { const env = createTestEnv({ AI: fakeAiBinding(AI_CLEAN) as any }); await seedInstallationToken(env, String(INSTALLATION_ID)); await seedDefaultModelStrategy(env, MODEL_ID); + await clearStrayRunningJobs(env); const { headSha, diff, pull, webhookPayload } = buildFixture(repo, 22); const { calls, restore } = installGitHubFetchMock({ @@ -188,7 +222,8 @@ dbDescribe('PR review pipeline (real GitHubClient + real ModelService)', () => { const response = await postWebhook(app, env, webhookPayload); expect(response.status).toBe(202); - await drainQueue(env); + const initialMessage = (env.REVIEW_QUEUE as any).sent[0]; + await runAndDrain(env, initialMessage); const finalJob = await findExistingJobForHead(env, { owner: 'test-owner', @@ -218,6 +253,7 @@ dbDescribe('PR review pipeline (real GitHubClient + real ModelService)', () => { const env = createTestEnv({ AI: fakeAiBinding(AI_WITH_ISSUE) as any }); await seedInstallationToken(env, String(INSTALLATION_ID)); await seedDefaultModelStrategy(env, MODEL_ID); + await clearStrayRunningJobs(env); const { headSha, diff, pull, webhookPayload } = buildFixture(repo, 33); const { calls, restore } = installGitHubFetchMock({ @@ -233,7 +269,8 @@ dbDescribe('PR review pipeline (real GitHubClient + real ModelService)', () => { const response = await postWebhook(app, env, webhookPayload); expect(response.status).toBe(202); - await drainQueue(env); + const initialMessage = (env.REVIEW_QUEUE as any).sent[0]; + await runAndDrain(env, initialMessage); const finalJob = await findExistingJobForHead(env, { owner: 'test-owner',