From fd4e18cea96de760bed7447396e3e324bc0e8119 Mon Sep 17 00:00:00 2001 From: "[._.]/ Adam Eivy" Date: Thu, 3 Sep 2026 23:26:28 -0700 Subject: [PATCH 1/2] fix([issue-6210]): layer the claim-work reviewer override into the JIRA play-button prompt resolveClaimReviewerPrompt resolved the Code Review Defaults alone, so a reviewer chain pinned for claims reached GitHub/GitLab/PLAN.md runs but silently not JIRA ones, and GET /api/apps/:id/claim-reviewers previewed a chain the JIRA run would not use. It now takes the app and layers claim-work metadata over the defaults through the same claimReviewersFrom the scheduled path uses; buildJiraTicketTask passes its app through. --- server/routes/cos.test.js | 4 +++ server/services/cosTaskGenerator.js | 21 +++++++------ server/services/cosTaskGenerator.test.js | 39 ++++++++++++++++++------ 3 files changed, 45 insertions(+), 19 deletions(-) diff --git a/server/routes/cos.test.js b/server/routes/cos.test.js index 9585cbcbc9..995b4477f3 100644 --- a/server/routes/cos.test.js +++ b/server/routes/cos.test.js @@ -125,6 +125,10 @@ vi.mock('../services/cosTaskGenerator.js', async (importActual) => ({ vi.mock('../services/apps.js', () => ({ getAppById: vi.fn(), getAppWorkTracker: vi.fn(), + // buildJiraTicketTask layers the app's claim-work metadata over the Code + // Review Defaults (#6210), so the route exercises getAppTaskTypeOverrides + // through it — default to no per-app override. + getAppTaskTypeOverrides: vi.fn(async () => ({})), PORTOS_APP_ID: 'portos-default' })); diff --git a/server/services/cosTaskGenerator.js b/server/services/cosTaskGenerator.js index 9577fa3c94..cc10083f3c 100644 --- a/server/services/cosTaskGenerator.js +++ b/server/services/cosTaskGenerator.js @@ -514,14 +514,15 @@ export async function buildClaimWorkTask(app, { * reads the CSV's `~effort=` suffix), and `taskMetadata` is PERSISTED so the * prompt builder resolves the same list back off the task record (#4770). */ -async function resolveClaimReviewerPrompt() { - const codeReviewDefaults = await getCodeReviewDefaults().catch(() => null); - // No task record exists yet for the play button, so the whole bundle resolves - // from the Code Review Defaults: the reviewer list, the `@user` tokens that - // gate the merge, the `~opt` set, the `~max=` caps, and the model/effort - // pins (which resolve together — an agy model id can carry its effort as a - // suffix, so the bracket and the appended instruction can't disagree). - const config = resolveClaimReviewerConfig({}, codeReviewDefaults, codeReviewDefaults?.reviewers); +async function resolveClaimReviewerPrompt(app) { + const [{ metadata }, codeReviewDefaults] = await Promise.all([ + resolveClaimWorkMetadata(app), + getCodeReviewDefaults().catch(() => null) + ]); + // The app's configured claim-work metadata layers over the Code Review + // Defaults through the same claimReviewersFrom the scheduled path uses — + // resolving from the defaults alone would silently drop a pinned override. + const config = claimReviewersFrom(metadata, codeReviewDefaults); const { reviewers: list, reviewerModels, reviewerEfforts, csv } = config; return { csv, @@ -556,10 +557,10 @@ export async function buildJiraTicketTask(app, ticketKey) { // null (unnormalizable) key can only come from a direct service caller. const key = normalizeWorkItemRef(ticketKey); - // Independent reads (prompt body + Code Review Defaults) — fetch concurrently. + // Independent reads (prompt body + claim reviewers) — fetch concurrently. const [template, { csv: reviewersCsv, taskMetadata: reviewerMetadata, effortBlock, localReviewerBlock }] = await Promise.all([ getTaskPrompt('claim-issue-jira'), - resolveClaimReviewerPrompt(), + resolveClaimReviewerPrompt(app), ]); const prompt = template .replace(/\{appName\}/g, app.name) diff --git a/server/services/cosTaskGenerator.test.js b/server/services/cosTaskGenerator.test.js index 2a7d7c6979..04f618374e 100644 --- a/server/services/cosTaskGenerator.test.js +++ b/server/services/cosTaskGenerator.test.js @@ -461,7 +461,10 @@ describe('{reviewers} interpolation honors Code Review Defaults', () => { // one site and silently miss another. It also applies the claim copilot // guard, which is what keeps the retired Copilot fallback from reappearing. expect(GEN_SRC).toContain('resolveClaimReviewerConfig(metadata, codeReviewDefaults, codeReviewDefaults?.reviewers)'); - expect(GEN_SRC).toContain('resolveClaimReviewerConfig({}, codeReviewDefaults, codeReviewDefaults?.reviewers)'); + // Every claim path layers the app's claim-work metadata over the defaults + // through claimReviewersFrom — no path may resolve the defaults alone with + // a bare `{}` (that silently drops a pinned override; #6210). + expect(GEN_SRC).not.toContain('resolveClaimReviewerConfig({}, codeReviewDefaults, codeReviewDefaults?.reviewers)'); expect(GEN_SRC).not.toMatch(/normalizeReviewers\(metadata\)(?!,)/); }); @@ -522,8 +525,11 @@ describe('{reviewers} interpolation honors Code Review Defaults', () => { // rejects, while the other paths emitted the split form. expect(GEN_SRC).toContain('reviewerModels: reviewerModels ?? metadata?.reviewerModels'); expect(GEN_SRC).toContain('reviewerEfforts: reviewerEfforts ?? metadata?.reviewerEfforts'); - // The play-button path reads the defaults directly (no task metadata to layer). - expect(GEN_SRC).toContain('resolveClaimReviewerConfig({}, codeReviewDefaults, codeReviewDefaults?.reviewers)'); + // All three claim paths layer claim-work metadata over the defaults via + // claimReviewersFrom — including the JIRA play button (#6210). No path may + // resolve the defaults alone with a bare `{}`. + expect(GEN_SRC).toContain('claimReviewersFrom(metadata, codeReviewDefaults)'); + expect(GEN_SRC).not.toContain('resolveClaimReviewerConfig({}, codeReviewDefaults, codeReviewDefaults?.reviewers)'); // No path may resolve one map without the other — or reach past the shared // claim resolver, which wraps `resolveReviewerPins` for all three sites. expect(GEN_SRC).not.toContain('resolveReviewerModels('); @@ -763,11 +769,13 @@ describe('buildJiraTicketTask', () => { const { ticketKey, prompt, taskMetadata } = await buildJiraTicketTask(app, 'proj-1234'); // Placeholders resolved from the app object. expect(prompt).toContain('App Acme App at /repos/acme (id acme)'); - // {reviewers} substituted (no literal placeholder left) with the Code Review - // Defaults reviewer + @username token. + // {reviewers} substituted (no literal placeholder left) with the claim-work + // override (['codex','claude'] in this file's taskSchedule mock) layered + // over the Code Review Defaults' `@alice` token — the JIRA play button + // honors the same override the /do:next claim does (#6210). expect(prompt).not.toContain('{reviewers}'); - expect(prompt).toContain('ollama'); - expect(prompt).toContain('Local Reviewer Procedure'); + expect(prompt).toContain('codex'); + expect(prompt).toContain('claude'); expect(prompt).not.toContain('copilot'); expect(prompt).toContain('@alice'); // Target-ticket constraint pins the uppercased key. @@ -775,7 +783,7 @@ describe('buildJiraTicketTask', () => { expect(prompt).toContain('PROJ-1234'); // Ticket key normalized to upper-case. expect(ticketKey).toBe('PROJ-1234'); - // claim-issue-jira self-manages worktree + PR; claimFlow keeps that + // claim-issue-jira self-manages its worktree + PR; claimFlow keeps that // lifecycle from falling into CoS's generic false/false handoff. The // resolved reviewer bundle rides along so the prompt builder's reviewer pin // names the same tokens this prompt does (#4770) — the play button's claim @@ -784,7 +792,7 @@ describe('buildJiraTicketTask', () => { useWorktree: false, openPR: false, claimFlow: true, - reviewers: ['ollama'], + reviewers: ['codex', 'claude'], usernames: ['alice'], optionalReviewers: [], reviewerMaxRounds: {}, @@ -793,11 +801,24 @@ describe('buildJiraTicketTask', () => { }); }); + it('falls through to the Code Review Defaults when claim-work pins no list', async () => { + getTaskInterval.mockResolvedValueOnce({ prompt: null, taskMetadata: { issueAuthorFilter: 'owner' } }); + const { prompt, taskMetadata } = await buildJiraTicketTask(app, 'proj-1234'); + expect(prompt).toContain('ollama'); + expect(prompt).toContain('Local Reviewer Procedure'); + expect(prompt).toContain('@alice'); + expect(taskMetadata.reviewers).toEqual(['ollama']); + }); + it('is exported so the /tasks/jira-ticket route reuses the shared assembly', () => { expect(GEN_SRC).toContain('export async function buildJiraTicketTask('); // Routes the JIRA flow directly, not via buildClaimWorkTask. expect(GEN_SRC).toMatch(/buildJiraTicketTask[\s\S]*getTaskPrompt\('claim-issue-jira'\)/); expect(GEN_SRC).toMatch(/buildJiraTicketTask[\s\S]*appendTargetWorkItemBlock\('claim-issue-jira', key\)/); + // The play button layers the app's claim-work metadata, not the defaults + // alone (#6210) — the app travels into the reviewer resolution. + expect(GEN_SRC).toMatch(/buildJiraTicketTask[\s\S]*resolveClaimReviewerPrompt\(app\)/); + expect(GEN_SRC).toMatch(/async function resolveClaimReviewerPrompt\(app\)/); }); }); From a7861d106b00cf9b9fd4d9efb2b5e21b31e718ea Mon Sep 17 00:00:00 2001 From: "[._.]/ Adam Eivy" Date: Thu, 3 Sep 2026 23:28:00 -0700 Subject: [PATCH 2/2] docs([issue-6210]): qualify the JIRA reviewer helper docstring to the default claim path --- server/services/cosTaskGenerator.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/server/services/cosTaskGenerator.js b/server/services/cosTaskGenerator.js index cc10083f3c..a29718c1a1 100644 --- a/server/services/cosTaskGenerator.js +++ b/server/services/cosTaskGenerator.js @@ -503,10 +503,10 @@ export async function buildClaimWorkTask(app, { } /** - * Resolve the reviewer prompt pieces for the claim flow exactly as - * buildClaimWorkTask does (including local-LLM reviewers). Mirrors the - * scheduled claim-work resolution so the JIRA play button honors the user's - * reviewer choice. + * Resolve the reviewer prompt pieces for the claim flow as buildClaimWorkTask + * does on its default (no-explicit-option) path (including local-LLM + * reviewers). Mirrors the scheduled claim-work resolution so the JIRA play + * button honors the user's reviewer choice. * * Returns each piece separately because they travel differently: `csv` fills the * template's `{reviewers}` placeholder, `effortBlock` is appended prose (the