From 870d1d356ac960ecccf2f754a9f60f875ad69025 Mon Sep 17 00:00:00 2001 From: Scrum4Me Agent <30029041+madhura68@users.noreply.github.com> Date: Thu, 14 May 2026 17:49:40 +0200 Subject: [PATCH 1/4] feat(PBI-12): code-bindende sort_order + priority uit story/taak-orderings - Voeg parseCodeNumber-helper toe in src/lib/code.ts - create-story/create-task: sort_order = parseCodeNumber(code), sort_order-inputparam verwijderd - get-claude-context + wait-for-job: priority verwijderd uit story/taak orderBy Co-Authored-By: Claude Sonnet 4.6 --- src/lib/code.ts | 10 ++++++++++ src/tools/create-story.ts | 16 +++------------- src/tools/create-task.ts | 17 ++++------------- src/tools/get-claude-context.ts | 4 ++-- src/tools/wait-for-job.ts | 4 ++-- 5 files changed, 21 insertions(+), 30 deletions(-) create mode 100644 src/lib/code.ts diff --git a/src/lib/code.ts b/src/lib/code.ts new file mode 100644 index 0000000..6b7376d --- /dev/null +++ b/src/lib/code.ts @@ -0,0 +1,10 @@ +// Sync met Scrum4Me/lib/code.ts — bewust duplicate (geen gedeeld package) +// om de MCP-server eigenstandig te houden. +// +// Extraheert het achterste getal uit een code-string (bijv. "ST-001" → 1, +// "T-42" → 42). Gebruikt als sort_order bij create_story / create_task. +export function parseCodeNumber(code: string | null | undefined): number { + if (!code) return 0 + const m = code.match(/(\d+)$/) + return m ? Number.parseInt(m[1], 10) : 0 +} diff --git a/src/tools/create-story.ts b/src/tools/create-story.ts index 37caa59..2e419d4 100644 --- a/src/tools/create-story.ts +++ b/src/tools/create-story.ts @@ -12,6 +12,7 @@ import { prisma } from '../prisma.js' import { requireWriteAccess } from '../auth.js' import { userCanAccessProduct } from '../access.js' import { toolError, toolJson, withToolErrors } from '../errors.js' +import { parseCodeNumber } from '../lib/code.js' const STORY_AUTO_RE = /^ST-(\d+)$/ const MAX_CODE_ATTEMPTS = 3 @@ -46,7 +47,6 @@ const inputSchema = z.object({ description: z.string().max(4000).optional(), acceptance_criteria: z.string().max(4000).optional(), priority: z.number().int().min(1).max(4), - sort_order: z.number().optional(), // Optionele sprint-koppeling: bij creatie de story direct aan een sprint // hangen (status=IN_SPRINT). De sprint moet bij hetzelfde product horen. sprint_id: z.string().min(1).optional(), @@ -59,7 +59,6 @@ export async function handleCreateStory( description, acceptance_criteria, priority, - sort_order, sprint_id, }: z.infer, ) { @@ -90,19 +89,10 @@ export async function handleCreateStory( } } - let resolvedSortOrder = sort_order - if (resolvedSortOrder === undefined) { - const last = await prisma.story.findFirst({ - where: { pbi_id, priority }, - orderBy: { sort_order: 'desc' }, - select: { sort_order: true }, - }) - resolvedSortOrder = (last?.sort_order ?? 0) + 1.0 - } - let lastError: unknown for (let attempt = 0; attempt < MAX_CODE_ATTEMPTS; attempt++) { const code = await generateNextStoryCode(pbi.product_id) + const resolvedSortOrder = parseCodeNumber(code) try { const story = await prisma.story.create({ data: { @@ -146,7 +136,7 @@ export function registerCreateStoryTool(server: McpServer) { { title: 'Create story', description: - 'Add a story under an existing PBI. Optionally link it to a sprint via sprint_id — when given, the story is created with status=IN_SPRINT and the sprint must belong to the same product as the PBI; otherwise status=OPEN and the story lands in the product backlog. Sort_order auto-set to last+1 within the PBI/priority group if not provided. Forbidden for demo accounts.', + 'Add a story under an existing PBI. Optionally link it to a sprint via sprint_id — when given, the story is created with status=IN_SPRINT and the sprint must belong to the same product as the PBI; otherwise status=OPEN and the story lands in the product backlog. Sort_order is derived from the auto-generated code number. Forbidden for demo accounts.', inputSchema, }, handleCreateStory, diff --git a/src/tools/create-task.ts b/src/tools/create-task.ts index 8308146..3882220 100644 --- a/src/tools/create-task.ts +++ b/src/tools/create-task.ts @@ -10,6 +10,7 @@ import { prisma } from '../prisma.js' import { requireWriteAccess } from '../auth.js' import { userCanAccessProduct } from '../access.js' import { toolError, toolJson, withToolErrors } from '../errors.js' +import { parseCodeNumber } from '../lib/code.js' const TASK_AUTO_RE = /^T-(\d+)$/ const MAX_CODE_ATTEMPTS = 3 @@ -44,7 +45,6 @@ const inputSchema = z.object({ description: z.string().max(4000).optional(), implementation_plan: z.string().max(8000).optional(), priority: z.number().int().min(1).max(4), - sort_order: z.number().optional(), // Cross-repo override: zet expliciet de repo waarop de worker deze task // moet uitvoeren (overrides product.repo_url). Gebruik dit voor PBI's die // werk in meerdere repos coördineren — bv. PBI op Scrum4Me-product met @@ -60,10 +60,10 @@ export function registerCreateTaskTool(server: McpServer) { { title: 'Create task', description: - 'Add a task under an existing story. Inherits sprint_id from the story (denormalized). Status defaults to TO_DO. Sort_order auto-set to last+1 within the story/priority group if not provided. Optional repo_url overrides the product.repo_url for cross-repo work (e.g. tasks targeting scrum4me-mcp under a Scrum4Me PBI). Forbidden for demo accounts.', + 'Add a task under an existing story. Inherits sprint_id from the story (denormalized). Status defaults to TO_DO. Sort_order is derived from the auto-generated code number. Optional repo_url overrides the product.repo_url for cross-repo work (e.g. tasks targeting scrum4me-mcp under a Scrum4Me PBI). Forbidden for demo accounts.', inputSchema, }, - async ({ story_id, title, description, implementation_plan, priority, sort_order, repo_url }) => + async ({ story_id, title, description, implementation_plan, priority, repo_url }) => withToolErrors(async () => { const auth = await requireWriteAccess() @@ -76,19 +76,10 @@ export function registerCreateTaskTool(server: McpServer) { return toolError(`Story ${story_id} not accessible`) } - let resolvedSortOrder = sort_order - if (resolvedSortOrder === undefined) { - const last = await prisma.task.findFirst({ - where: { story_id, priority }, - orderBy: { sort_order: 'desc' }, - select: { sort_order: true }, - }) - resolvedSortOrder = (last?.sort_order ?? 0) + 1.0 - } - let lastError: unknown for (let attempt = 0; attempt < MAX_CODE_ATTEMPTS; attempt++) { const code = await generateNextTaskCode(story.product_id) + const resolvedSortOrder = parseCodeNumber(code) try { const task = await prisma.task.create({ data: { diff --git a/src/tools/get-claude-context.ts b/src/tools/get-claude-context.ts index 80f7a4c..2a94892 100644 --- a/src/tools/get-claude-context.ts +++ b/src/tools/get-claude-context.ts @@ -63,7 +63,7 @@ export function registerGetClaudeContextTool(server: McpServer) { { tasks: { some: { status: { not: 'DONE' } } } }, ], }, - orderBy: [{ priority: 'asc' }, { sort_order: 'asc' }], + orderBy: [{ sort_order: 'asc' }], select: { id: true, code: true, @@ -73,7 +73,7 @@ export function registerGetClaudeContextTool(server: McpServer) { priority: true, status: true, tasks: { - orderBy: [{ priority: 'asc' }, { sort_order: 'asc' }], + orderBy: [{ sort_order: 'asc' }], select: { id: true, title: true, diff --git a/src/tools/wait-for-job.ts b/src/tools/wait-for-job.ts index f3e11c0..dd322c9 100644 --- a/src/tools/wait-for-job.ts +++ b/src/tools/wait-for-job.ts @@ -606,10 +606,10 @@ export async function getFullJobContext(jobId: string) { }, tasks: { where: { status: 'TO_DO' }, - orderBy: [{ priority: 'asc' }, { sort_order: 'asc' }], + orderBy: [{ sort_order: 'asc' }], }, }, - orderBy: [{ priority: 'asc' }, { sort_order: 'asc' }], + orderBy: [{ sort_order: 'asc' }], }, }, }, From 84c194d4e52840379361f0c23e5d8bc88e612206 Mon Sep 17 00:00:00 2001 From: Janpeter Visser <30029041+madhura68@users.noreply.github.com> Date: Thu, 14 May 2026 19:16:15 +0200 Subject: [PATCH 2/4] fix(cross-repo): per-repo worktree-branch + PR resolutie (IDEA-062) (#49) Cross-repo sprints (sprint-product = repo X, maar een taak heeft task.repo_url naar repo Y) faalden op twee plekken omdat sprint-brede beslissingen werden toegepast op per-repo git-state. 1. createWorktreeForJob (src/git/worktree.ts) reuseBranch wordt sprint-breed bepaald in wait-for-job.ts. De eerste job die repo Y target krijgt reuseBranch=true terwijl de branch daar nooit is aangemaakt -> `git worktree add ` faalt met "invalid reference" -> job vast, worker UNHEALTHY. Idem na een container-recreate (clone is dan vers). Fix: 3-weg fallback in het reuseBranch-pad: - lokale branch bestaat -> hergebruik - alleen op origin -> recreate lokaal vanaf origin/ - nergens -> fresh vanaf baseRef Lost ook het container-recreate-verlies op. 2. maybeCreateAutoPr (src/tools/update-job-status.ts) De sprint/story sibling-lookup voor pr_url-hergebruik filterde niet op repo. Een repo-Y-job erfde de pr_url van een repo-X-sibling -> job.pr_url wees naar de verkeerde repo en er werd nooit een PR voor de repo-Y-branch aangemaakt (branch wel gepusht, maar PR-loos). Fix: siblings groeperen per repo-bucket ((task.repo_url ?? null)); alleen een sibling uit dezelfde bucket levert een herbruikbare pr_url. Geldt voor SPRINT- en STORY-mode. createPullRequest zelf was al repo-correct (gh pr create draait in de worktree). Tests: 3 nieuwe in worktree.test.ts (reuse-local / recreate-from-origin / fresh-fallback), 2 nieuwe in update-job-status-auto-pr.test.ts (cross-repo story + sprint). update-job-status-mock omgezet naar findMany. Alle 373 tests groen, build groen. package-lock.json: version 0.7.0 -> 0.8.0 (was niet mee-gesynced in de v0.8.0-bump commit 55fa133). Co-authored-by: Claude Opus 4.7 (1M context) --- __tests__/git/worktree.test.ts | 65 +++++++++++++++++++++ __tests__/update-job-status-auto-pr.test.ts | 51 ++++++++++++++-- package-lock.json | 4 +- src/git/worktree.ts | 35 ++++++++++- src/tools/update-job-status.ts | 31 +++++++--- 5 files changed, 171 insertions(+), 15 deletions(-) diff --git a/__tests__/git/worktree.test.ts b/__tests__/git/worktree.test.ts index 68f5e19..68cedfd 100644 --- a/__tests__/git/worktree.test.ts +++ b/__tests__/git/worktree.test.ts @@ -113,6 +113,71 @@ describe('createWorktreeForJob', () => { }), ).rejects.toThrow('Worktree path already exists') }) + + it('reuseBranch: reuses an existing local branch', async () => { + const { repoDir, originDir } = await setupRepo() + tmpDirs.push(repoDir, originDir) + await makeWorktreeParent() + + // Sibling already created the branch locally. + await git(['branch', 'feat/sprint-abc', 'origin/main'], repoDir) + + const result = await createWorktreeForJob({ + repoRoot: repoDir, + jobId: 'job-reuse-local', + branchName: 'feat/sprint-abc', + baseRef: 'origin/main', + reuseBranch: true, + }) + + const { stdout } = await git(['rev-parse', '--abbrev-ref', 'HEAD'], result.worktreePath) + expect(stdout.trim()).toBe('feat/sprint-abc') + expect(result.branchName).toBe('feat/sprint-abc') + }) + + it('reuseBranch: recreates a local branch from origin when only the remote has it', async () => { + const { repoDir, originDir } = await setupRepo() + tmpDirs.push(repoDir, originDir) + await makeWorktreeParent() + + // Branch exists on origin (a sibling pushed it, or the container was + // recreated and the local clone is fresh) but not as a local branch. + await git(['branch', 'feat/sprint-xyz', 'origin/main'], repoDir) + await git(['push', 'origin', 'feat/sprint-xyz'], repoDir) + await git(['branch', '-D', 'feat/sprint-xyz'], repoDir) + + const result = await createWorktreeForJob({ + repoRoot: repoDir, + jobId: 'job-reuse-origin', + branchName: 'feat/sprint-xyz', + baseRef: 'origin/main', + reuseBranch: true, + }) + + const { stdout } = await git(['rev-parse', '--abbrev-ref', 'HEAD'], result.worktreePath) + expect(stdout.trim()).toBe('feat/sprint-xyz') + }) + + it('reuseBranch: falls back to a fresh branch when it exists nowhere (cross-repo sprint)', async () => { + const { repoDir, originDir } = await setupRepo() + tmpDirs.push(repoDir, originDir) + await makeWorktreeParent() + + // reuseBranch is decided sprint-wide; for the first job targeting THIS + // repo the branch exists neither locally nor on origin. Must not throw + // "invalid reference" — should create it fresh from baseRef. + const result = await createWorktreeForJob({ + repoRoot: repoDir, + jobId: 'job-reuse-fresh', + branchName: 'feat/sprint-newrepo', + baseRef: 'origin/main', + reuseBranch: true, + }) + + const { stdout } = await git(['rev-parse', '--abbrev-ref', 'HEAD'], result.worktreePath) + expect(stdout.trim()).toBe('feat/sprint-newrepo') + expect(result.branchName).toBe('feat/sprint-newrepo') + }) }) describe('removeWorktreeForJob', () => { diff --git a/__tests__/update-job-status-auto-pr.test.ts b/__tests__/update-job-status-auto-pr.test.ts index 3218b3e..e92fdb3 100644 --- a/__tests__/update-job-status-auto-pr.test.ts +++ b/__tests__/update-job-status-auto-pr.test.ts @@ -4,7 +4,7 @@ vi.mock('../src/prisma.js', () => ({ prisma: { product: { findUnique: vi.fn() }, task: { findUnique: vi.fn() }, - claudeJob: { findFirst: vi.fn(), findUnique: vi.fn() }, + claudeJob: { findFirst: vi.fn(), findMany: vi.fn(), findUnique: vi.fn() }, }, })) @@ -22,6 +22,7 @@ const mockPrisma = prisma as unknown as { task: { findUnique: ReturnType } claudeJob: { findFirst: ReturnType + findMany: ReturnType findUnique: ReturnType } } @@ -41,9 +42,10 @@ beforeEach(() => { mockPrisma.product.findUnique.mockResolvedValue({ auto_pr: true }) mockPrisma.task.findUnique.mockResolvedValue({ title: 'Add feature', + repo_url: null, story: { id: 'story-1', code: 'SCRUM-42', title: 'Story title' }, }) - mockPrisma.claudeJob.findFirst.mockResolvedValue(null) // no sibling PR by default + mockPrisma.claudeJob.findMany.mockResolvedValue([]) // no sibling PRs by default // Default: legacy job zonder sprint_run (STORY-mode pad). mockPrisma.claudeJob.findUnique.mockResolvedValue({ sprint_run_id: null, sprint_run: null }) mockCreatePr.mockResolvedValue({ url: 'https://github.com/org/repo/pull/99' }) @@ -62,12 +64,27 @@ describe('maybeCreateAutoPr', () => { }) it('reuses sibling pr_url when another job in same story already opened a PR', async () => { - mockPrisma.claudeJob.findFirst.mockResolvedValue({ pr_url: 'https://github.com/org/repo/pull/77' }) + mockPrisma.claudeJob.findMany.mockResolvedValue([ + { pr_url: 'https://github.com/org/repo/pull/77', task: { repo_url: null } }, + ]) const url = await maybeCreateAutoPr(BASE_OPTS) expect(url).toBe('https://github.com/org/repo/pull/77') expect(mockCreatePr).not.toHaveBeenCalled() }) + it('does NOT reuse a sibling PR from a different repo (cross-repo story)', async () => { + // Sibling targeted another repo via task.repo_url — its PR must not leak in. + mockPrisma.claudeJob.findMany.mockResolvedValue([ + { + pr_url: 'https://github.com/org/other-repo/pull/12', + task: { repo_url: 'https://github.com/org/other-repo' }, + }, + ]) + const url = await maybeCreateAutoPr(BASE_OPTS) + expect(url).toBe('https://github.com/org/repo/pull/99') // fresh PR, not the sibling's + expect(mockCreatePr).toHaveBeenCalledOnce() + }) + it('returns null when auto_pr=false', async () => { mockPrisma.product.findUnique.mockResolvedValue({ auto_pr: false }) const url = await maybeCreateAutoPr(BASE_OPTS) @@ -78,6 +95,7 @@ describe('maybeCreateAutoPr', () => { it('uses story title without code prefix when story has no code', async () => { mockPrisma.task.findUnique.mockResolvedValue({ title: 'Add feature', + repo_url: null, story: { id: 'story-1', code: null, title: 'Story title' }, }) await maybeCreateAutoPr(BASE_OPTS) @@ -113,7 +131,9 @@ describe('maybeCreateAutoPr', () => { sprint_run_id: 'run-1', sprint_run: { id: 'run-1', pr_strategy: 'SPRINT', sprint: { sprint_goal: 'Goal' } }, }) - mockPrisma.claudeJob.findFirst.mockResolvedValue({ pr_url: 'https://github.com/org/repo/pull/55' }) + mockPrisma.claudeJob.findMany.mockResolvedValue([ + { pr_url: 'https://github.com/org/repo/pull/55', task: { repo_url: null } }, + ]) const url = await maybeCreateAutoPr(BASE_OPTS) @@ -121,6 +141,29 @@ describe('maybeCreateAutoPr', () => { expect(mockCreatePr).not.toHaveBeenCalled() }) + it('SPRINT-mode: cross-repo — sibling-PR van ander repo wordt niet hergebruikt', async () => { + mockPrisma.claudeJob.findUnique.mockResolvedValue({ + sprint_run_id: 'run-1', + sprint_run: { id: 'run-1', pr_strategy: 'SPRINT', sprint: { sprint_goal: 'Goal' } }, + }) + // Deze job target een ander repo via task.repo_url. + mockPrisma.task.findUnique.mockResolvedValue({ + title: 'MCP-taak', + repo_url: 'https://github.com/org/scrum4me-mcp', + story: { id: 'story-1', code: 'SCRUM-9', title: 'Story title' }, + }) + // Sibling met pr_url hoort bij het product-repo (repo_url null) → andere bucket. + mockPrisma.claudeJob.findMany.mockResolvedValue([ + { pr_url: 'https://github.com/org/repo/pull/201', task: { repo_url: null } }, + ]) + + const url = await maybeCreateAutoPr(BASE_OPTS) + + // Geen hergebruik van de product-repo PR → eigen draft-PR voor het mcp-repo. + expect(url).toBe('https://github.com/org/repo/pull/99') + expect(mockCreatePr).toHaveBeenCalledOnce() + }) + it('returns null and does not throw when gh fails', async () => { mockCreatePr.mockResolvedValue({ error: 'gh CLI not found' }) const url = await maybeCreateAutoPr(BASE_OPTS) diff --git a/package-lock.json b/package-lock.json index 61bcb4a..3514598 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "scrum4me-mcp", - "version": "0.7.0", + "version": "0.8.0", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "scrum4me-mcp", - "version": "0.7.0", + "version": "0.8.0", "hasInstallScript": true, "license": "MIT", "dependencies": { diff --git a/src/git/worktree.ts b/src/git/worktree.ts index 4d03443..a27aca6 100644 --- a/src/git/worktree.ts +++ b/src/git/worktree.ts @@ -15,6 +15,19 @@ async function branchExists(repoRoot: string, name: string): Promise { } } +async function remoteBranchExists(repoRoot: string, name: string): Promise { + try { + await exec( + 'git', + ['show-ref', '--verify', '--quiet', `refs/remotes/origin/${name}`], + { cwd: repoRoot }, + ) + return true + } catch { + return false + } +} + async function findWorktreeForBranch( repoRoot: string, branchName: string, @@ -75,7 +88,27 @@ export async function createWorktreeForJob(opts: { if (occupant) { await exec('git', ['worktree', 'remove', '--force', occupant], { cwd: repoRoot }) } - await exec('git', ['worktree', 'add', worktreePath, branchName], { cwd: repoRoot }) + // reuseBranch is decided sprint-wide, but git branches are per-repo. For a + // cross-repo sprint the first job targeting THIS repo gets reuseBranch=true + // even though the branch was never created here; a container recreate also + // wipes the local clone. Fall back gracefully instead of failing with + // "invalid reference": + // - local branch exists → reuse it + // - exists on origin only → recreate the local branch tracking origin + // - nowhere → create it fresh from baseRef + if (await branchExists(repoRoot, branchName)) { + await exec('git', ['worktree', 'add', worktreePath, branchName], { cwd: repoRoot }) + } else if (await remoteBranchExists(repoRoot, branchName)) { + await exec( + 'git', + ['worktree', 'add', '-b', branchName, worktreePath, `origin/${branchName}`], + { cwd: repoRoot }, + ) + } else { + await exec('git', ['worktree', 'add', '-b', branchName, worktreePath, baseRef], { + cwd: repoRoot, + }) + } return { worktreePath, branchName } } diff --git a/src/tools/update-job-status.ts b/src/tools/update-job-status.ts index 5e40988..e7a9495 100644 --- a/src/tools/update-job-status.ts +++ b/src/tools/update-job-status.ts @@ -420,24 +420,35 @@ export async function maybeCreateAutoPr(opts: { where: { id: taskId }, select: { title: true, + repo_url: true, story: { select: { id: true, code: true, title: true } }, }, }) if (!task) return null - // PBI-46 SPRINT-mode: hergebruik 1 draft-PR voor de hele SprintRun. + // Cross-repo sprints: een sprint kan taken hebben die via task.repo_url een + // ander repo targeten. PRs en branches zijn per-repo, dus een sibling-PR mag + // alleen hergebruikt worden als die sibling hetzelfde repo targette. null/leeg + // repo_url = het product-repo; twee taken zitten in dezelfde repo-bucket als + // hun (repo_url ?? null) gelijk is. + const thisRepoKey = task.repo_url ?? null + + // PBI-46 SPRINT-mode: hergebruik 1 draft-PR voor de hele SprintRun (per repo). // Mens zet 'm ready-for-review zodra de SprintRun DONE is. if (job?.sprint_run && job.sprint_run.pr_strategy === 'SPRINT') { - const sprintSibling = await prisma.claudeJob.findFirst({ + const sprintSiblings = await prisma.claudeJob.findMany({ where: { sprint_run_id: job.sprint_run_id, pr_url: { not: null }, id: { not: jobId }, }, - select: { pr_url: true }, + select: { pr_url: true, task: { select: { repo_url: true } } }, orderBy: { created_at: 'asc' }, }) - if (sprintSibling?.pr_url) return sprintSibling.pr_url + const sameRepoSibling = sprintSiblings.find( + (s) => (s.task?.repo_url ?? null) === thisRepoKey, + ) + if (sameRepoSibling?.pr_url) return sameRepoSibling.pr_url // Eerste DONE in deze SprintRun → maak draft-PR aan, geen auto-merge. const goal = job.sprint_run.sprint.sprint_goal @@ -459,17 +470,21 @@ export async function maybeCreateAutoPr(opts: { return null } - // STORY-mode (default of legacy): branch-per-story, sibling-tasks delen PR. - const sibling = await prisma.claudeJob.findFirst({ + // STORY-mode (default of legacy): branch-per-story, sibling-tasks delen PR + // — maar alleen siblings die hetzelfde repo targeten (zie thisRepoKey). + const storySiblings = await prisma.claudeJob.findMany({ where: { task: { story_id: task.story.id }, pr_url: { not: null }, id: { not: jobId }, }, - select: { pr_url: true }, + select: { pr_url: true, task: { select: { repo_url: true } } }, orderBy: { created_at: 'asc' }, }) - if (sibling?.pr_url) return sibling.pr_url + const sameRepoStorySibling = storySiblings.find( + (s) => (s.task?.repo_url ?? null) === thisRepoKey, + ) + if (sameRepoStorySibling?.pr_url) return sameRepoStorySibling.pr_url const storyTitle = task.story.code ? `${task.story.code}: ${task.story.title}` : task.story.title const body = summary From 51fc65e71548ede77435c590b7c7df6ea2434c45 Mon Sep 17 00:00:00 2001 From: Janpeter Visser <30029041+madhura68@users.noreply.github.com> Date: Thu, 14 May 2026 19:46:31 +0200 Subject: [PATCH 3/4] fix(update_idea_plan_reviewed): nooit stilzwijgend goedkeuren (IDEA-066) (#50) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit De status-logica sprak z'n eigen tool-beschrijving tegen. De code deed: approved -> PLAN_REVIEWED rejected -> PLAN_REVIEW_FAILED else -> PLAN_REVIEWED // "Default to approved if not specified" Een review die 'pending' (needs manual approval) of helemaal geen approval_status teruggaf, markeerde het idee dus als PLAN_REVIEWED (goedgekeurd) — precies omgekeerd aan wat de beschrijving belooft. Fix: alleen een expliciete approval_status='approved' brengt het idee naar PLAN_REVIEWED; 'rejected', 'pending' én een weggelaten approval_status gaan allemaal naar PLAN_REVIEW_FAILED (mens beslist). Nooit stilzwijgend goedkeuren. Verder: - Handler geextraheerd naar handleUpdateIdeaPlanReviewed + inputSchema geexporteerd, conform het create-sprint/update-sprint-patroon, zodat de logica zonder McpServer-wrapper testbaar is. - Tool-beschrijving + header-comment aangescherpt zodat code en docs niet meer divergeren. - Nieuw test-bestand: 6 tests (approved/rejected/pending/omitted status-transitie, not-found, log-persistentie). Build groen, 379 tests groen. Co-authored-by: Claude Opus 4.7 (1M context) --- __tests__/update-idea-plan-reviewed.test.ts | 140 ++++++++++++++++++++ src/tools/update-idea-plan-reviewed.ts | 118 +++++++++-------- 2 files changed, 204 insertions(+), 54 deletions(-) create mode 100644 __tests__/update-idea-plan-reviewed.test.ts diff --git a/__tests__/update-idea-plan-reviewed.test.ts b/__tests__/update-idea-plan-reviewed.test.ts new file mode 100644 index 0000000..257fce4 --- /dev/null +++ b/__tests__/update-idea-plan-reviewed.test.ts @@ -0,0 +1,140 @@ +import { describe, it, expect, vi, beforeEach } from 'vitest' + +vi.mock('../src/prisma.js', () => ({ + prisma: { + idea: { update: vi.fn() }, + ideaLog: { create: vi.fn() }, + $transaction: vi.fn(), + }, +})) + +vi.mock('../src/auth.js', () => ({ + requireWriteAccess: vi.fn(), + PermissionDeniedError: class PermissionDeniedError extends Error { + constructor(message = 'Demo accounts cannot perform write operations') { + super(message) + this.name = 'PermissionDeniedError' + } + }, +})) + +vi.mock('../src/access.js', () => ({ + userOwnsIdea: vi.fn(), +})) + +import { prisma } from '../src/prisma.js' +import { requireWriteAccess } from '../src/auth.js' +import { userOwnsIdea } from '../src/access.js' +import { handleUpdateIdeaPlanReviewed } from '../src/tools/update-idea-plan-reviewed.js' + +const mockPrisma = prisma as unknown as { + idea: { update: ReturnType } + ideaLog: { create: ReturnType } + $transaction: ReturnType +} +const mockRequireWriteAccess = requireWriteAccess as ReturnType +const mockUserOwnsIdea = userOwnsIdea as ReturnType + +const IDEA_ID = 'idea-1' +const USER_ID = 'user-1' +const REVIEW_LOG = { + rounds: [{ score: 88 }], + convergence: { stable_at_round: 2 }, + approval: { status: 'approved' }, +} + +beforeEach(() => { + vi.clearAllMocks() + mockRequireWriteAccess.mockResolvedValue({ + userId: USER_ID, + tokenId: 'tok-1', + username: 'alice', + isDemo: false, + }) + mockUserOwnsIdea.mockResolvedValue(true) + // $transaction returns the array of its two operations' results; the handler + // only reads result[0] (the idea.update result). + mockPrisma.$transaction.mockImplementation(async () => [ + { id: IDEA_ID, status: 'PLACEHOLDER', code: 'IDEA-1' }, + {}, + ]) +}) + +function parseResult(result: Awaited>) { + const text = result.content?.[0]?.type === 'text' ? result.content[0].text : '' + try { + return JSON.parse(text) + } catch { + return text + } +} + +// The handler builds `data.status` inside the idea.update call passed to +// $transaction. We capture it by inspecting the prisma.idea.update mock args. +function statusPassedToUpdate(): string | undefined { + const call = mockPrisma.idea.update.mock.calls[0] + return call?.[0]?.data?.status +} + +describe('handleUpdateIdeaPlanReviewed — status transition', () => { + it('approval_status="approved" → PLAN_REVIEWED', async () => { + await handleUpdateIdeaPlanReviewed({ + idea_id: IDEA_ID, + review_log: REVIEW_LOG, + approval_status: 'approved', + }) + expect(statusPassedToUpdate()).toBe('PLAN_REVIEWED') + }) + + it('approval_status="rejected" → PLAN_REVIEW_FAILED', async () => { + await handleUpdateIdeaPlanReviewed({ + idea_id: IDEA_ID, + review_log: REVIEW_LOG, + approval_status: 'rejected', + }) + expect(statusPassedToUpdate()).toBe('PLAN_REVIEW_FAILED') + }) + + it('approval_status="pending" → PLAN_REVIEW_FAILED (needs manual approval, never silently approved)', async () => { + await handleUpdateIdeaPlanReviewed({ + idea_id: IDEA_ID, + review_log: REVIEW_LOG, + approval_status: 'pending', + }) + expect(statusPassedToUpdate()).toBe('PLAN_REVIEW_FAILED') + }) + + it('omitted approval_status → PLAN_REVIEW_FAILED (safe default, not PLAN_REVIEWED)', async () => { + await handleUpdateIdeaPlanReviewed({ + idea_id: IDEA_ID, + review_log: REVIEW_LOG, + }) + expect(statusPassedToUpdate()).toBe('PLAN_REVIEW_FAILED') + }) + + it('returns "Idea not found" when the user does not own the idea', async () => { + mockUserOwnsIdea.mockResolvedValue(false) + const result = await handleUpdateIdeaPlanReviewed({ + idea_id: IDEA_ID, + review_log: REVIEW_LOG, + approval_status: 'approved', + }) + expect(parseResult(result)).toContain('Idea not found') + expect(mockPrisma.idea.update).not.toHaveBeenCalled() + }) + + it('persists review_log + reviewed_at and logs a PLAN_REVIEW_RESULT entry', async () => { + await handleUpdateIdeaPlanReviewed({ + idea_id: IDEA_ID, + review_log: REVIEW_LOG, + approval_status: 'approved', + }) + const updateArg = mockPrisma.idea.update.mock.calls[0]?.[0] + expect(updateArg?.data?.plan_review_log).toEqual(REVIEW_LOG) + expect(updateArg?.data?.reviewed_at).toBeInstanceOf(Date) + + const logArg = mockPrisma.ideaLog.create.mock.calls[0]?.[0] + expect(logArg?.data?.type).toBe('PLAN_REVIEW_RESULT') + expect(logArg?.data?.idea_id).toBe(IDEA_ID) + }) +}) diff --git a/src/tools/update-idea-plan-reviewed.ts b/src/tools/update-idea-plan-reviewed.ts index 0217c22..2e9f1ac 100644 --- a/src/tools/update-idea-plan-reviewed.ts +++ b/src/tools/update-idea-plan-reviewed.ts @@ -1,6 +1,8 @@ -// MCP-tool: writes the review-log result after a IDEA_REVIEW_PLAN grill-job -// and transitions the idea.status to PLAN_REVIEWED (on success) or -// PLAN_REVIEW_FAILED (on failure). +// MCP-tool: writes the review-log result after an IDEA_REVIEW_PLAN job and +// transitions idea.status. Only an explicit approval_status='approved' moves +// the idea to PLAN_REVIEWED; anything else (rejected, pending, or omitted) +// goes to PLAN_REVIEW_FAILED — a human must then decide. The tool never +// silently approves. // // Called by the worker as the final step of a review-plan session. @@ -12,7 +14,7 @@ import { requireWriteAccess } from '../auth.js' import { userOwnsIdea } from '../access.js' import { toolError, toolJson, withToolErrors } from '../errors.js' -const inputSchema = z.object({ +export const inputSchema = z.object({ idea_id: z.string().min(1), review_log: z.object({}).passthrough(), // Full ReviewLog from orchestrator (JSON object) approval_status: z @@ -20,64 +22,72 @@ const inputSchema = z.object({ .optional(), }) +export async function handleUpdateIdeaPlanReviewed( + { idea_id, review_log, approval_status }: z.infer, +) { + return withToolErrors(async () => { + const auth = await requireWriteAccess() + if (!(await userOwnsIdea(idea_id, auth.userId))) { + return toolError('Idea not found') + } + + // Alleen een expliciete 'approved' brengt het idee naar PLAN_REVIEWED. + // 'rejected', 'pending' én een weggelaten approval_status betekenen + // allemaal "niet auto-goedgekeurd — mens moet beslissen" en gaan naar + // PLAN_REVIEW_FAILED. Nooit stilzwijgend goedkeuren (de vorige + // `: 'PLAN_REVIEWED'`-default deed dat wel bij pending/undefined). + const nextStatus = + approval_status === 'approved' ? 'PLAN_REVIEWED' : 'PLAN_REVIEW_FAILED' + + // Log summary metrics from review_log + const logSummary = buildReviewLogSummary(review_log) + + const result = await prisma.$transaction([ + prisma.idea.update({ + where: { id: idea_id }, + data: { + plan_review_log: review_log as any, + reviewed_at: new Date(), + status: nextStatus, + }, + select: { id: true, status: true, code: true }, + }), + prisma.ideaLog.create({ + data: { + idea_id, + type: 'PLAN_REVIEW_RESULT', + content: logSummary.summary, + metadata: { + approval_status, + convergence_status: logSummary.convergence_status, + final_score: logSummary.final_score, + rounds_completed: logSummary.rounds_completed, + }, + }, + }), + ]) + + return toolJson({ + ok: true, + idea: result[0], + review_log_summary: logSummary, + }) + }) +} + export function registerUpdateIdeaPlanReviewedTool(server: McpServer) { server.registerTool( 'update_idea_plan_reviewed', { title: 'Mark plan as reviewed', description: - 'Save review-log after plan review cycle and transition idea.status to PLAN_REVIEWED (if approved) or PLAN_REVIEW_FAILED (if rejected/pending requires manual approval). Forbidden for demo accounts.', + 'Save review-log after a plan review cycle and transition idea.status. ' + + 'Only approval_status="approved" → PLAN_REVIEWED; "rejected", "pending", ' + + 'or an omitted approval_status → PLAN_REVIEW_FAILED (needs manual ' + + 'approval — never silently approved). Forbidden for demo accounts.', inputSchema, }, - async ({ idea_id, review_log, approval_status }) => - withToolErrors(async () => { - const auth = await requireWriteAccess() - if (!(await userOwnsIdea(idea_id, auth.userId))) { - return toolError('Idea not found') - } - - // Determine target status based on approval - const nextStatus = - approval_status === 'approved' - ? 'PLAN_REVIEWED' - : approval_status === 'rejected' - ? 'PLAN_REVIEW_FAILED' - : 'PLAN_REVIEWED' // Default to approved if not specified - - // Log summary metrics from review_log - const logSummary = buildReviewLogSummary(review_log) - - const result = await prisma.$transaction([ - prisma.idea.update({ - where: { id: idea_id }, - data: { - plan_review_log: review_log as any, - reviewed_at: new Date(), - status: nextStatus, - }, - select: { id: true, status: true, code: true }, - }), - prisma.ideaLog.create({ - data: { - idea_id, - type: 'PLAN_REVIEW_RESULT', - content: logSummary.summary, - metadata: { - approval_status, - convergence_status: logSummary.convergence_status, - final_score: logSummary.final_score, - rounds_completed: logSummary.rounds_completed, - }, - }, - }), - ]) - - return toolJson({ - ok: true, - idea: result[0], - review_log_summary: logSummary, - }) - }), + handleUpdateIdeaPlanReviewed, ) } From fba2d67796b10ed20c76b8eb74072d57ce2c9fa1 Mon Sep 17 00:00:00 2001 From: Janpeter Visser <30029041+madhura68@users.noreply.github.com> Date: Thu, 14 May 2026 23:21:44 +0200 Subject: [PATCH 4/4] fix(update_job_status): status-gedreven lifecycle-timestamps (#51) Een job kon CLAIMED -> done/failed/skipped gaan zonder ooit `running` te rapporteren, waardoor started_at NULL bleef terwijl finished_at wel gezet werd. Dat brak de invariant claimed_at <= started_at <= finished_at en elke duur-analyse. Nieuwe pure helper resolveJobTimestamps zet de lifecycle-timestamps set-once op basis van de status: started_at wordt gebackfild bij een terminale overgang, claimed_at defensief gevuld als die ontbreekt. De running-tak is nu set-once i.p.v. bij elke call overschrijven. Co-authored-by: Madhura68 Co-authored-by: Claude Opus 4.7 (1M context) --- .../update-job-status-timestamps.test.ts | 74 +++++++++++++++++++ src/tools/update-job-status.ts | 39 +++++++++- 2 files changed, 109 insertions(+), 4 deletions(-) create mode 100644 __tests__/update-job-status-timestamps.test.ts diff --git a/__tests__/update-job-status-timestamps.test.ts b/__tests__/update-job-status-timestamps.test.ts new file mode 100644 index 0000000..d4ab80f --- /dev/null +++ b/__tests__/update-job-status-timestamps.test.ts @@ -0,0 +1,74 @@ +// Unit-tests voor resolveJobTimestamps — de status-gedreven timestamp-helper +// van update_job_status. Pure functie, geen mocks (zoals update-job-status-gate). + +import { describe, it, expect } from 'vitest' +import { resolveJobTimestamps } from '../src/tools/update-job-status.js' + +const NOW = new Date('2026-05-14T12:00:00.000Z') +const EARLIER = new Date('2026-05-14T11:00:00.000Z') + +describe('resolveJobTimestamps', () => { + describe('running', () => { + it('sets started_at when not yet set, no finished_at', () => { + const r = resolveJobTimestamps('running', { claimed_at: EARLIER, started_at: null }, NOW) + expect(r.started_at).toBe(NOW) + expect(r.finished_at).toBeUndefined() + expect(r.claimed_at).toBeUndefined() + }) + + it('is set-once: does not re-stamp started_at when already set', () => { + const r = resolveJobTimestamps('running', { claimed_at: EARLIER, started_at: EARLIER }, NOW) + expect(r.started_at).toBeUndefined() + expect(r.finished_at).toBeUndefined() + expect(r.claimed_at).toBeUndefined() + }) + }) + + describe('terminal transitions (done/failed/skipped)', () => { + it.each(['done', 'failed', 'skipped'] as const)( + 'backfills started_at and sets finished_at for %s when started_at is null', + (status) => { + const r = resolveJobTimestamps(status, { claimed_at: EARLIER, started_at: null }, NOW) + expect(r.started_at).toBe(NOW) + expect(r.finished_at).toBe(NOW) + expect(r.claimed_at).toBeUndefined() + }, + ) + + it('only sets finished_at when started_at is already set', () => { + const r = resolveJobTimestamps('done', { claimed_at: EARLIER, started_at: EARLIER }, NOW) + expect(r.started_at).toBeUndefined() + expect(r.finished_at).toBe(NOW) + expect(r.claimed_at).toBeUndefined() + }) + }) + + describe('claimed_at backfill', () => { + it.each(['running', 'done', 'failed', 'skipped'] as const)( + 'backfills claimed_at for %s when it is null', + (status) => { + const r = resolveJobTimestamps(status, { claimed_at: null, started_at: null }, NOW) + expect(r.claimed_at).toBe(NOW) + }, + ) + + it('never returns claimed_at when it is already set', () => { + const r = resolveJobTimestamps('done', { claimed_at: EARLIER, started_at: EARLIER }, NOW) + expect(r.claimed_at).toBeUndefined() + }) + }) + + it('returns only finished_at when all timestamps are already set and status is terminal', () => { + const r = resolveJobTimestamps('failed', { claimed_at: EARLIER, started_at: EARLIER }, NOW) + expect(r).toEqual({ finished_at: NOW }) + }) + + it('defaults now to a fresh Date when omitted', () => { + const before = Date.now() + const r = resolveJobTimestamps('running', { claimed_at: EARLIER, started_at: null }) + const after = Date.now() + expect(r.started_at).toBeInstanceOf(Date) + expect(r.started_at!.getTime()).toBeGreaterThanOrEqual(before) + expect(r.started_at!.getTime()).toBeLessThanOrEqual(after) + }) +}) diff --git a/src/tools/update-job-status.ts b/src/tools/update-job-status.ts index e7a9495..9fcd08b 100644 --- a/src/tools/update-job-status.ts +++ b/src/tools/update-job-status.ts @@ -390,6 +390,32 @@ export function resolveNextAction( return queueCount > 0 ? 'wait_for_job_again' : 'queue_empty' } +export type JobTimestampUpdate = { + claimed_at?: Date + started_at?: Date + finished_at?: Date +} + +// Bepaalt welke lifecycle-timestamps update_job_status schrijft bij een +// status-overgang. Set-once (backfill alleen als nu null) houdt de invariant +// claimed_at ≤ started_at ≤ finished_at: een job die CLAIMED → done gaat +// zonder `running`-rapport krijgt alsnog een started_at, en claimed_at +// (normaal door wait_for_job bij claim gezet) wordt nooit overschreven. +export function resolveJobTimestamps( + status: 'running' | 'done' | 'failed' | 'skipped', + current: { claimed_at: Date | null; started_at: Date | null }, + now: Date = new Date(), +): JobTimestampUpdate { + const isTerminal = status === 'done' || status === 'failed' || status === 'skipped' + const update: JobTimestampUpdate = {} + if (current.claimed_at == null) update.claimed_at = now + if (current.started_at == null && (status === 'running' || isTerminal)) { + update.started_at = now + } + if (isTerminal) update.finished_at = now + return update +} + export async function maybeCreateAutoPr(opts: { jobId: string productId: string @@ -569,6 +595,8 @@ export function registerUpdateJobStatusTool(server: McpServer) { 'Report progress on a claimed ClaudeJob. Allowed transitions from CLAIMED/RUNNING: ' + 'running (start), done (finished), failed (error), skipped (no-op exit). ' + 'The Bearer token must match the token that claimed the job. ' + + 'Stamps started_at on running and finished_at on done/failed/skipped, and backfills ' + + 'claimed_at/started_at when missing so claimed_at ≤ started_at ≤ finished_at always holds. ' + 'Before marking done: call verify_task_against_plan first — done is rejected when ' + 'verify_result is null, EMPTY (unless task.verify_only is true), or when the verify level ' + 'doesn’t meet task.verify_required: ALIGNED-only is strict; ALIGNED_OR_PARTIAL accepts ' + @@ -608,6 +636,8 @@ export function registerUpdateJobStatusTool(server: McpServer) { select: { id: true, status: true, + claimed_at: true, + started_at: true, claimed_by_token_id: true, user_id: true, product_id: true, @@ -751,10 +781,11 @@ export function registerUpdateJobStatusTool(server: McpServer) { where: { id: job_id }, data: { status: dbStatus, - ...(actualStatus === 'running' ? { started_at: now } : {}), - ...(actualStatus === 'done' || actualStatus === 'failed' || actualStatus === 'skipped' - ? { finished_at: now } - : {}), + ...resolveJobTimestamps( + actualStatus, + { claimed_at: job.claimed_at, started_at: job.started_at }, + now, + ), ...(branchToWrite !== undefined ? { branch: branchToWrite } : {}), ...(pushedAt !== undefined ? { pushed_at: pushedAt } : {}), ...(summary !== undefined ? { summary } : {}),