From 010ec2e5cfc507f18930b9885a7ab2a593b62d24 Mon Sep 17 00:00:00 2001 From: Zbigniew Sobiecki Date: Thu, 16 Apr 2026 15:47:17 +0000 Subject: [PATCH] fix(prWorkItems): preserve workItemId across racing pipeline writers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When the implementation pipeline (PM-triggered) opens a PR, the GitHub pr-opened webhook fires immediately and queues a review pipeline. The review pipeline captures workItemId synchronously from a DB lookup that returns null (the implementation hasn't yet linked the PR), then later writes that stale null back to pr_work_items, clobbering the correct work_item_id the implementation wrote in between. Effect: lookupWorkItemForPR returns null for the PR, the pr-ready-to-merge trigger bails with "No work item linked to PR", and PRs flagged with `auto` on Linear never auto-merge. Affected every recent llmist PR (15 in a row); Trello/JIRA-backed projects appear unaffected so far but share the racing pipeline code path. Three intersecting fixes, all narrow: - linkPRToWorkItem Step 2 ON CONFLICT now uses COALESCE(EXCLUDED.work_item_id, pr_work_items.work_item_id) so work_item_id is one-way set — a later writer with null can't erase a known link. - linkPRToWorkItem Step 1 deletes any racing orphan (projectId, NULL workItemId, prNumber) row before promoting the work-item-only row, preventing the partial-unique-index violation the implementation otherwise hits silently. - runAgentExecutionPipeline re-resolves workItemId via the existing resolveWorkItemId helper at run time and patches agentInput so the corrected value flows into runAgent (and into agent_runs.work_item_id), not just into the post-execution link. Tests: +2 integration (preservation, orphan cleanup), +5 unit (re-resolution paths + delete/no-delete coverage). All 7814 unit and 524 integration tests pass. Drive-by cleanups so lint+typecheck are clean: drop unused `renderManifestStep` import in pm-wizard.tsx, rename a pm-conformance describe string that triggered noTemplateCurlyInString. Out of scope: backfilling the 15 broken historical llmist rows (separate manual SQL), no PR-body-parsing fallback in resolveWorkItemId, no rewrite of how PROpenedTrigger captures workItemId. Co-Authored-By: Claude Opus 4.6 (1M context) --- src/db/repositories/prWorkItemsRepository.ts | 37 +++++++- src/triggers/shared/agent-execution.ts | 18 +++- .../db/prWorkItemsRepository.test.ts | 57 +++++++++++ .../prWorkItemsRepository.test.ts | 31 ++++-- .../unit/integrations/pm-conformance.test.ts | 2 +- .../triggers/shared/agent-execution.test.ts | 94 +++++++++++++++++++ web/src/components/projects/pm-wizard.tsx | 1 - 7 files changed, 225 insertions(+), 15 deletions(-) diff --git a/src/db/repositories/prWorkItemsRepository.ts b/src/db/repositories/prWorkItemsRepository.ts index 822816be..68da0906 100644 --- a/src/db/repositories/prWorkItemsRepository.ts +++ b/src/db/repositories/prWorkItemsRepository.ts @@ -8,6 +8,7 @@ import { isNull, max, type SQL, + sql, sum, } from 'drizzle-orm'; import { getDb } from '../client.js'; @@ -80,10 +81,20 @@ export async function createWorkItem( * Upsert a PR ↔ work item link. * * Two-step logic: - * 1. If a work-item-only row exists for (projectId, workItemId), UPDATE it with PR data. + * 1. If `workItemId` is provided, drop any racing orphan `(projectId, NULL workItemId, + * prNumber)` row, then UPDATE the existing work-item-only row with PR data. * 2. Otherwise INSERT a new row, using onConflictDoUpdate on (projectId, prNumber). * - * workItemId is optional to support "orphan" PRs (PRs created without a linked work item). + * `workItemId` is optional to support "orphan" PRs (PRs created without a linked + * work item). + * + * **Link-preservation invariant** — the `work_item_id` column is one-way set: once + * an earlier writer landed a non-null value, a later writer that happens to lack + * the workItemId must NOT unset it. Step 2's ON CONFLICT clause uses + * `COALESCE(EXCLUDED.work_item_id, pr_work_items.work_item_id)` to enforce this. + * Without it, a stale review-pipeline writeback (workItemId captured at PR-opened + * time, before the implementation linked the PR) would clobber the correct value + * and break downstream lookups (notably the `pr-ready-to-merge` auto-merge trigger). */ export async function linkPRToWorkItem( projectId: string, @@ -96,8 +107,23 @@ export async function linkPRToWorkItem( const now = new Date(); const { workItemUrl, workItemTitle, prUrl, prTitle } = options; - // Step 1: If workItemId is provided, try to update the existing work-item-only row + // Step 1: If workItemId is provided, try to update the existing work-item-only row. if (workItemId) { + // First, drop any racing orphan row (projectId, NULL workItemId, prNumber). It + // only exists because no link existed when an earlier writer (e.g. the review + // pipeline's pre-execution pass on a freshly opened PR) inserted it. Promoting + // our work-item-only row to the same prNumber would otherwise hit + // uq_pr_work_items_project_pr. + await db + .delete(prWorkItems) + .where( + and( + eq(prWorkItems.projectId, projectId), + eq(prWorkItems.prNumber, prNumber), + isNull(prWorkItems.workItemId), + ), + ); + const updated = await db .update(prWorkItems) .set({ @@ -142,7 +168,10 @@ export async function linkPRToWorkItem( target: [prWorkItems.projectId, prWorkItems.prNumber], targetWhere: isNotNull(prWorkItems.prNumber), set: { - workItemId, + // COALESCE: never erase a known workItemId with null. The link is + // one-way set — once an earlier writer landed it, later writers that + // happen to lack the workItemId must not unset it. + workItemId: sql`COALESCE(${workItemId}, ${prWorkItems.workItemId})`, repoFullName, workItemUrl, workItemTitle, diff --git a/src/triggers/shared/agent-execution.ts b/src/triggers/shared/agent-execution.ts index 156b66ef..e3261617 100644 --- a/src/triggers/shared/agent-execution.ts +++ b/src/triggers/shared/agent-execution.ts @@ -395,7 +395,21 @@ export async function runAgentExecutionPipeline( const { skipPrepareForAgent = false, onSuccess, onFailure, logLabel = 'Agent' } = executionConfig; - const workItemId = result.workItemId; + // Re-resolve workItemId at run time. The trigger handler (e.g. PROpenedTrigger) + // captures workItemId synchronously at webhook arrival, before any other + // pipeline has had time to link the PR. By the time we run, the DB may have + // caught up — preferring the live value avoids carrying a stale `undefined` + // into runAgent (and therefore agent_runs.work_item_id) and into the + // post-execution linkPRToWorkItem write. + const workItemId = await resolveWorkItemId(result.workItemId, project.id, result.prNumber); + + // If we recovered a workItemId the trigger didn't have, patch agentInput so + // the corrected value flows into runAgent and into the agent_runs row that + // tryCreateRun (src/agents/shared/runTracking.ts) writes. + const agentInput = + workItemId && workItemId !== result.workItemId + ? { ...result.agentInput, workItemId } + : result.agentInput; let remainingBudgetUsd: number | undefined; if (workItemId) { @@ -448,7 +462,7 @@ export async function runAgentExecutionPipeline( } const agentResult = await runAgent(agentType, { - ...result.agentInput, + ...agentInput, remainingBudgetUsd, project, config, diff --git a/tests/integration/db/prWorkItemsRepository.test.ts b/tests/integration/db/prWorkItemsRepository.test.ts index 8758462b..763c30fc 100644 --- a/tests/integration/db/prWorkItemsRepository.test.ts +++ b/tests/integration/db/prWorkItemsRepository.test.ts @@ -2,6 +2,7 @@ import { and, eq } from 'drizzle-orm'; import { beforeEach, describe, expect, it } from 'vitest'; import { getDb } from '../../../src/db/client.js'; import { + createWorkItem, linkPRToWorkItem, listPRsForProject, listPRsForWorkItem, @@ -105,6 +106,62 @@ describe('prWorkItemsRepository (integration)', () => { }); }); + // ========================================================================= + // Link-preservation invariant — workItemId is one-way set + // ========================================================================= + + describe('linkPRToWorkItem — link-preservation invariant', () => { + it('preserves an existing non-null workItemId when called with workItemId=null', async () => { + // First writer (e.g. implementation pipeline post-execution): correct link. + await linkPRToWorkItem('test-project', 'owner/repo', 568, 'MNG-93', { + workItemUrl: 'https://linear.app/mongrel/issue/MNG-93/...', + prUrl: 'https://github.com/owner/repo/pull/568', + prTitle: 'feat: add unit tests', + }); + + // Second writer (e.g. review pipeline post-execution) carries no workItemId. + await linkPRToWorkItem('test-project', 'owner/repo', 568, null, { + prUrl: 'https://github.com/owner/repo/pull/568', + prTitle: 'feat: add unit tests', + }); + + // The known link must NOT be erased. + const workItemId = await lookupWorkItemForPR('test-project', 568); + expect(workItemId).toBe('MNG-93'); + }); + + it('removes a stale orphan (projectId, NULL, prNumber) row when promoting a work-item-only row', async () => { + // Step 1: review pipeline pre-execution insert an orphan row. + await linkPRToWorkItem('test-project', 'owner/repo', 568, null, { + prUrl: 'https://github.com/owner/repo/pull/568', + prTitle: 'feat: add unit tests', + }); + // Step 2: implementation pipeline createWorkItem inserted earlier + // (we do it here in a different order to simulate the race). + await createWorkItem('test-project', 'MNG-93', { + workItemUrl: 'https://linear.app/mongrel/issue/MNG-93/...', + workItemTitle: 'add unit tests', + }); + + // Step 3: implementation completes — promote work-item-only row. + // Without orphan cleanup this throws on uq_pr_work_items_project_pr. + await linkPRToWorkItem('test-project', 'owner/repo', 568, 'MNG-93', { + workItemUrl: 'https://linear.app/mongrel/issue/MNG-93/...', + prUrl: 'https://github.com/owner/repo/pull/568', + prTitle: 'feat: add unit tests', + }); + + const db = getDb(); + const rows = await db + .select() + .from(prWorkItems) + .where(and(eq(prWorkItems.projectId, 'test-project'), eq(prWorkItems.prNumber, 568))); + expect(rows).toHaveLength(1); + expect(rows[0].workItemId).toBe('MNG-93'); + expect(rows[0].workItemUrl).toBe('https://linear.app/mongrel/issue/MNG-93/...'); + }); + }); + describe('lookupWorkItemForPR', () => { it('returns null for non-existent link', async () => { const result = await lookupWorkItemForPR('test-project', 999); diff --git a/tests/unit/db/repositories/prWorkItemsRepository.test.ts b/tests/unit/db/repositories/prWorkItemsRepository.test.ts index debdc1e6..bbf2d15a 100644 --- a/tests/unit/db/repositories/prWorkItemsRepository.test.ts +++ b/tests/unit/db/repositories/prWorkItemsRepository.test.ts @@ -120,6 +120,7 @@ function makeQueryChainWithRows(rows: unknown[]): ReturnType { let chain: ReturnType; + let deleteWhere: ReturnType; let mockDb: { select: ReturnType; insert: ReturnType; @@ -131,12 +132,14 @@ describe('prWorkItemsRepository', () => { // linkPRToWorkItem's two-step logic: first update (returns []), then insert. // Default: update returns [] (no existing work-item row), insert proceeds. chain = createMockChain([]); + // Step 1's orphan-cleanup uses db.delete(table).where(condition). + deleteWhere = vi.fn().mockResolvedValue(undefined); mockDb = { select: vi.fn().mockReturnValue({ from: chain.from }), insert: vi.fn().mockReturnValue({ values: chain.values }), update: vi.fn().mockReturnValue({ set: chain.set }), - delete: vi.fn(), + delete: vi.fn().mockReturnValue({ where: deleteWhere }), }; mockGetDb.mockReturnValue(mockDb as never); }); @@ -269,12 +272,26 @@ describe('prWorkItemsRepository', () => { await linkPRToWorkItem('proj-1', 'owner/repo', 42, 'wi-abc'); expect(chain.onConflictDoUpdate).toHaveBeenCalledTimes(1); - expect(chain.onConflictDoUpdate).toHaveBeenCalledWith( - expect.objectContaining({ - target: expect.arrayContaining([expect.anything(), expect.anything()]), - set: expect.objectContaining({ workItemId: 'wi-abc', repoFullName: 'owner/repo' }), - }), - ); + // workItemId in `set` is a COALESCE SQL expression (one-way set semantics), + // not the literal string. Other fields are passed through unchanged. + const conflictArg = chain.onConflictDoUpdate.mock.calls[0][0]; + expect(conflictArg.target).toHaveLength(2); + expect(conflictArg.set.repoFullName).toBe('owner/repo'); + expect(conflictArg.set.workItemId).toBeDefined(); + expect(conflictArg.set.workItemId).not.toBe('wi-abc'); // wrapped in SQL + }); + + it('deletes any racing orphan (NULL workItemId, prNumber) row before Step 1 UPDATE', async () => { + await linkPRToWorkItem('proj-1', 'owner/repo', 42, 'wi-abc'); + + expect(mockDb.delete).toHaveBeenCalledTimes(1); + expect(deleteWhere).toHaveBeenCalledTimes(1); + }); + + it('does NOT call delete when workItemId is null (no Step 1 path)', async () => { + await linkPRToWorkItem('proj-1', 'owner/repo', 42, null); + + expect(mockDb.delete).not.toHaveBeenCalled(); }); it('persists optional display fields when provided', async () => { diff --git a/tests/unit/integrations/pm-conformance.test.ts b/tests/unit/integrations/pm-conformance.test.ts index b976d5bd..596129f2 100644 --- a/tests/unit/integrations/pm-conformance.test.ts +++ b/tests/unit/integrations/pm-conformance.test.ts @@ -43,7 +43,7 @@ describe('PM provider conformance (every registered provider)', () => { expect(manifest.category).toBe('pm'); }); - it('webhookRoute matches the /${id}/webhook convention', () => { + it('webhookRoute matches the //webhook convention', () => { expect(manifest.webhookRoute).toBe(`/${id}/webhook`); }); diff --git a/tests/unit/triggers/shared/agent-execution.test.ts b/tests/unit/triggers/shared/agent-execution.test.ts index 5c8fc22d..b4250bd6 100644 --- a/tests/unit/triggers/shared/agent-execution.test.ts +++ b/tests/unit/triggers/shared/agent-execution.test.ts @@ -819,3 +819,97 @@ describe('pre-execution PR linking (via runAgentExecutionPipeline)', () => { ); }); }); + +// --------------------------------------------------------------------------- +// workItemId staleness recovery (via runAgentExecutionPipeline) +// --------------------------------------------------------------------------- + +describe('workItemId staleness recovery (via runAgentExecutionPipeline)', () => { + beforeEach(() => { + mockCreatePMProvider.mockReturnValue({}); + mockResolveProjectPMConfig.mockReturnValue(PM_CONFIG); + mockValidateIntegrations.mockResolvedValue({ valid: true, errors: [] }); + mockCheckBudgetExceeded.mockResolvedValue(null); + mockHandleAgentResultArtifacts.mockResolvedValue(undefined); + mockShouldTriggerDebug.mockResolvedValue(null); + mockGetSessionState.mockReturnValue({}); + mockRunAgent.mockResolvedValue({ success: true, output: '', runId: 'run-1' }); + }); + + it('re-resolves workItemId from DB when result.workItemId is undefined but PR is already linked', async () => { + // Implementation has already linked PR #42 to card-from-db + mockLookupWorkItemForPR.mockResolvedValueOnce('card-from-db'); + + // PROpenedTrigger-style result captured before the link existed + await runAgentExecutionPipeline( + { + agentType: 'review', + agentInput: { prNumber: 42 }, + prNumber: 42, + prUrl: 'https://github.com/acme/myapp/pull/42', + prTitle: 'Test PR', + }, + PROJECT, + CONFIG, + ); + + // runAgent receives the resolved workItemId in agentInput + expect(mockRunAgent).toHaveBeenCalledWith( + 'review', + expect.objectContaining({ workItemId: 'card-from-db' }), + ); + + // linkPRToWorkItem is called with the resolved workItemId, not null + expect(vi.mocked(linkPRToWorkItem)).toHaveBeenCalledWith( + 'project-1', + 'acme/myapp', + 42, + 'card-from-db', + expect.anything(), + ); + }); + + it('preserves trigger-supplied workItemId when DB lookup is unnecessary', async () => { + // Trigger already carries a workItemId — no DB lookup expected + await runAgentExecutionPipeline( + { + agentType: 'review', + agentInput: { prNumber: 42, workItemId: 'card-from-trigger' }, + prNumber: 42, + workItemId: 'card-from-trigger', + prUrl: 'https://github.com/acme/myapp/pull/42', + }, + PROJECT, + CONFIG, + ); + + expect(mockLookupWorkItemForPR).not.toHaveBeenCalled(); + expect(mockRunAgent).toHaveBeenCalledWith( + 'review', + expect.objectContaining({ workItemId: 'card-from-trigger' }), + ); + }); + + it('leaves workItemId undefined when neither trigger nor DB has one', async () => { + // Default mockLookupWorkItemForPR returns null + await runAgentExecutionPipeline( + { + agentType: 'review', + agentInput: { prNumber: 42 }, + prNumber: 42, + prUrl: 'https://github.com/acme/myapp/pull/42', + }, + PROJECT, + CONFIG, + ); + + expect(mockLookupWorkItemForPR).toHaveBeenCalledWith('project-1', 42); + expect(vi.mocked(linkPRToWorkItem)).toHaveBeenCalledWith( + 'project-1', + 'acme/myapp', + 42, + null, + expect.anything(), + ); + }); +}); diff --git a/web/src/components/projects/pm-wizard.tsx b/web/src/components/projects/pm-wizard.tsx index 87287235..192b5dbe 100644 --- a/web/src/components/projects/pm-wizard.tsx +++ b/web/src/components/projects/pm-wizard.tsx @@ -11,7 +11,6 @@ import './pm-providers/jira/index.js'; import './pm-providers/linear/index.js'; import { ManifestProviderWizardSection } from './pm-providers/manifest-section.js'; import { getProviderWizard } from './pm-providers/registry.js'; -import { renderManifestStep } from './pm-providers/render.js'; import { SaveStep, WebhookStep } from './pm-wizard-common-steps.js'; import { useLinearWebhookInfo,