diff --git a/__tests__/update-job-status-auto-pr.test.ts b/__tests__/update-job-status-auto-pr.test.ts index 55db4cf..4a901ad 100644 --- a/__tests__/update-job-status-auto-pr.test.ts +++ b/__tests__/update-job-status-auto-pr.test.ts @@ -4,6 +4,7 @@ vi.mock('../src/prisma.js', () => ({ prisma: { product: { findUnique: vi.fn() }, task: { findUnique: vi.fn() }, + claudeJob: { findFirst: vi.fn() }, }, })) @@ -18,6 +19,7 @@ import { maybeCreateAutoPr } from '../src/tools/update-job-status.js' const mockPrisma = prisma as unknown as { product: { findUnique: ReturnType } task: { findUnique: ReturnType } + claudeJob: { findFirst: ReturnType } } const mockCreatePr = createPullRequest as ReturnType @@ -35,23 +37,31 @@ beforeEach(() => { mockPrisma.product.findUnique.mockResolvedValue({ auto_pr: true }) mockPrisma.task.findUnique.mockResolvedValue({ title: 'Add feature', - story: { code: 'SCRUM-42' }, + story: { id: 'story-1', code: 'SCRUM-42', title: 'Story title' }, }) + mockPrisma.claudeJob.findFirst.mockResolvedValue(null) // no sibling PR by default mockCreatePr.mockResolvedValue({ url: 'https://github.com/org/repo/pull/99' }) }) describe('maybeCreateAutoPr', () => { - it('returns PR URL when auto_pr=true and gh succeeds', async () => { + it('returns PR URL when auto_pr=true and gh succeeds (story-scoped title)', async () => { const url = await maybeCreateAutoPr(BASE_OPTS) expect(url).toBe('https://github.com/org/repo/pull/99') expect(mockCreatePr).toHaveBeenCalledWith({ worktreePath: BASE_OPTS.worktreePath, branchName: BASE_OPTS.branchName, - title: 'SCRUM-42: Add feature', + title: 'SCRUM-42: Story title', body: expect.stringContaining(BASE_OPTS.summary), }) }) + 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' }) + const url = await maybeCreateAutoPr(BASE_OPTS) + expect(url).toBe('https://github.com/org/repo/pull/77') + expect(mockCreatePr).not.toHaveBeenCalled() + }) + it('returns null when auto_pr=false', async () => { mockPrisma.product.findUnique.mockResolvedValue({ auto_pr: false }) const url = await maybeCreateAutoPr(BASE_OPTS) @@ -59,14 +69,14 @@ describe('maybeCreateAutoPr', () => { expect(mockCreatePr).not.toHaveBeenCalled() }) - it('uses task title without code prefix when story has no code', async () => { + it('uses story title without code prefix when story has no code', async () => { mockPrisma.task.findUnique.mockResolvedValue({ title: 'Add feature', - story: { code: null }, + story: { id: 'story-1', code: null, title: 'Story title' }, }) await maybeCreateAutoPr(BASE_OPTS) expect(mockCreatePr).toHaveBeenCalledWith( - expect.objectContaining({ title: 'Add feature' }), + expect.objectContaining({ title: 'Story title' }), ) }) diff --git a/__tests__/update-job-status-worktree.test.ts b/__tests__/update-job-status-worktree.test.ts index 5b084b4..e9a5c62 100644 --- a/__tests__/update-job-status-worktree.test.ts +++ b/__tests__/update-job-status-worktree.test.ts @@ -1,5 +1,11 @@ import { describe, it, expect, vi, beforeEach } from 'vitest' +vi.mock('../src/prisma.js', () => ({ + prisma: { + claudeJob: { findUnique: vi.fn(), count: vi.fn() }, + }, +})) + vi.mock('../src/git/worktree.js', () => ({ removeWorktreeForJob: vi.fn(), })) @@ -12,15 +18,25 @@ vi.mock('../src/tools/wait-for-job.js', async (importOriginal) => { } }) +import { prisma } from '../src/prisma.js' import { removeWorktreeForJob } from '../src/git/worktree.js' import { resolveRepoRoot } from '../src/tools/wait-for-job.js' import { cleanupWorktreeForTerminalStatus } from '../src/tools/update-job-status.js' const mockRemove = removeWorktreeForJob as ReturnType const mockResolve = resolveRepoRoot as ReturnType +const mockPrisma = prisma as unknown as { + claudeJob: { + findUnique: ReturnType + count: ReturnType + } +} beforeEach(() => { vi.clearAllMocks() + // Default: job exists, no active siblings — cleanup proceeds + mockPrisma.claudeJob.findUnique.mockResolvedValue({ task: { story_id: 'story-default' } }) + mockPrisma.claudeJob.count.mockResolvedValue(0) }) describe('cleanupWorktreeForTerminalStatus', () => { @@ -81,4 +97,14 @@ describe('cleanupWorktreeForTerminalStatus', () => { cleanupWorktreeForTerminalStatus('prod-001', 'job-abc', 'done', 'feat/job-abc'), ).resolves.toBeUndefined() }) + + it('defers cleanup when sibling jobs in same story are still active', async () => { + mockResolve.mockResolvedValue('/repos/my-project') + mockPrisma.claudeJob.findUnique.mockResolvedValue({ task: { story_id: 'story-shared' } }) + mockPrisma.claudeJob.count.mockResolvedValue(2) // 2 siblings active + + await cleanupWorktreeForTerminalStatus('prod-001', 'job-abc', 'done', 'feat/story-shared') + + expect(mockRemove).not.toHaveBeenCalled() + }) }) diff --git a/__tests__/wait-for-job-worktree.test.ts b/__tests__/wait-for-job-worktree.test.ts index 0f18052..c594fab 100644 --- a/__tests__/wait-for-job-worktree.test.ts +++ b/__tests__/wait-for-job-worktree.test.ts @@ -6,6 +6,8 @@ import * as fs from 'node:fs/promises' vi.mock('../src/prisma.js', () => ({ prisma: { $executeRaw: vi.fn(), + claudeJob: { findFirst: vi.fn() }, + product: { findUnique: vi.fn() }, }, })) @@ -17,7 +19,11 @@ import { prisma } from '../src/prisma.js' import { createWorktreeForJob } from '../src/git/worktree.js' import { resolveRepoRoot, rollbackClaim, attachWorktreeToJob } from '../src/tools/wait-for-job.js' -const mockPrisma = prisma as unknown as { $executeRaw: ReturnType } +const mockPrisma = prisma as unknown as { + $executeRaw: ReturnType + claudeJob: { findFirst: ReturnType } + product: { findUnique: ReturnType } +} const mockCreateWorktree = createWorktreeForJob as ReturnType beforeEach(() => { @@ -88,32 +94,51 @@ describe('attachWorktreeToJob', () => { Object.assign(process.env, originalEnv) }) - it('returns worktree_path and branch_name on success', async () => { + it('returns worktree_path and branch_name on success (no sibling → fresh story branch)', async () => { process.env['SCRUM4ME_REPO_ROOT_prod-001'] = '/repos/my-project' + mockPrisma.claudeJob.findFirst.mockResolvedValue(null) mockCreateWorktree.mockResolvedValue({ worktreePath: '/home/user/.scrum4me-agent-worktrees/job-abc12345', - branchName: 'feat/job-abc12345', + branchName: 'feat/story-XXXstory', }) mockPrisma.$executeRaw.mockResolvedValue(0) - const result = await attachWorktreeToJob('prod-001', 'job-abc12345') + const result = await attachWorktreeToJob('prod-001', 'job-abc12345', 'story-XXXstory') expect(result).toEqual({ worktree_path: '/home/user/.scrum4me-agent-worktrees/job-abc12345', - branch_name: 'feat/job-abc12345', + branch_name: 'feat/story-XXXstory', + reused_branch: false, }) expect(mockCreateWorktree).toHaveBeenCalledWith({ repoRoot: '/repos/my-project', jobId: 'job-abc12345', - branchName: 'feat/job-abc12345', + branchName: 'feat/story-XXXstory', + reuseBranch: false, }) }) + it('reuses sibling branch when sibling job already has a branch in same story', async () => { + process.env['SCRUM4ME_REPO_ROOT_prod-001'] = '/repos/my-project' + mockPrisma.claudeJob.findFirst.mockResolvedValue({ branch: 'feat/story-existing' }) + mockCreateWorktree.mockResolvedValue({ + worktreePath: '/home/user/.scrum4me-agent-worktrees/job-zzz', + branchName: 'feat/story-existing', + }) + mockPrisma.$executeRaw.mockResolvedValue(0) + + const result = await attachWorktreeToJob('prod-001', 'job-zzz', 'story-shared') + + expect(result).toMatchObject({ branch_name: 'feat/story-existing', reused_branch: true }) + expect(mockCreateWorktree).toHaveBeenCalledWith(expect.objectContaining({ reuseBranch: true })) + }) + it('rolls back claim and returns error when no repoRoot configured', async () => { delete process.env['SCRUM4ME_REPO_ROOT_prod-no-root'] + mockPrisma.product.findUnique.mockResolvedValue({ repo_url: null }) mockPrisma.$executeRaw.mockResolvedValue(0) - const result = await attachWorktreeToJob('prod-no-root', 'job-xyz') + const result = await attachWorktreeToJob('prod-no-root', 'job-xyz', 'story-y') expect('error' in result).toBe(true) expect((result as { error: string }).error).toContain('No repo root configured') @@ -124,10 +149,11 @@ describe('attachWorktreeToJob', () => { it('rolls back claim and returns error when createWorktreeForJob throws', async () => { process.env['SCRUM4ME_REPO_ROOT_prod-001'] = '/repos/my-project' + mockPrisma.claudeJob.findFirst.mockResolvedValue(null) mockCreateWorktree.mockRejectedValue(new Error('git fetch failed')) mockPrisma.$executeRaw.mockResolvedValue(0) - const result = await attachWorktreeToJob('prod-001', 'job-fail') + const result = await attachWorktreeToJob('prod-001', 'job-fail', 'story-z') expect('error' in result).toBe(true) expect((result as { error: string }).error).toContain('git fetch failed') diff --git a/src/git/worktree.ts b/src/git/worktree.ts index dd5e26d..1a2a7db 100644 --- a/src/git/worktree.ts +++ b/src/git/worktree.ts @@ -15,13 +15,39 @@ async function branchExists(repoRoot: string, name: string): Promise { } } +async function findWorktreeForBranch( + repoRoot: string, + branchName: string, +): Promise { + try { + const { stdout } = await exec('git', ['worktree', 'list', '--porcelain'], { cwd: repoRoot }) + // Porcelain blocks: worktree \nHEAD \nbranch refs/heads/\n\n + const blocks = stdout.split('\n\n').filter(Boolean) + for (const block of blocks) { + const lines = block.split('\n') + const wt = lines.find((l) => l.startsWith('worktree '))?.slice(9) + const br = lines.find((l) => l.startsWith('branch '))?.slice(7) // refs/heads/ + if (wt && br && br === `refs/heads/${branchName}`) return wt + } + return null + } catch { + return null + } +} + export async function createWorktreeForJob(opts: { repoRoot: string jobId: string branchName: string baseRef?: string + /** + * When true the branch is expected to exist already (sibling job created it). + * If a stale sibling worktree still occupies the branch, it is removed first + * — siblings are sequential, so this is safe. + */ + reuseBranch?: boolean }): Promise<{ worktreePath: string; branchName: string }> { - const { repoRoot, jobId, baseRef = 'origin/main' } = opts + const { repoRoot, jobId, baseRef = 'origin/main', reuseBranch = false } = opts let { branchName } = opts const parent = @@ -44,7 +70,18 @@ export async function createWorktreeForJob(opts: { await exec('git', ['fetch', 'origin', '--prune'], { cwd: repoRoot }) - // Suffix with timestamp when branch already exists + if (reuseBranch) { + // Sibling task already created the branch; check it out into a fresh worktree. + // If the branch is still attached to a stale sibling worktree, drop that first. + const occupant = await findWorktreeForBranch(repoRoot, branchName) + if (occupant) { + await exec('git', ['worktree', 'remove', '--force', occupant], { cwd: repoRoot }) + } + await exec('git', ['worktree', 'add', worktreePath, branchName], { cwd: repoRoot }) + return { worktreePath, branchName } + } + + // Fresh branch: suffix with timestamp when name collision occurs if (await branchExists(repoRoot, branchName)) { branchName = `${branchName}-${Date.now()}` } diff --git a/src/tools/update-job-status.ts b/src/tools/update-job-status.ts index 05f4dfd..4833c6c 100644 --- a/src/tools/update-job-status.ts +++ b/src/tools/update-job-status.ts @@ -30,14 +30,45 @@ export async function cleanupWorktreeForTerminalStatus( branch: string | undefined, ): Promise { const repoRoot = await resolveRepoRoot(productId) - if (!repoRoot) return + if (!repoRoot) { + console.warn( + `[update_job_status] cleanup skip for job=${jobId}: no repoRoot configured for product ${productId}`, + ) + return + } + + // Branch-per-story: only remove the worktree if no sibling job in the same + // story is still active. If siblings are queued/claimed/running they will + // re-use this branch — destroying the worktree now wastes the next claim. + const job = await prisma.claudeJob.findUnique({ + where: { id: jobId }, + select: { task: { select: { story_id: true } } }, + }) + if (job) { + const activeSiblings = await prisma.claudeJob.count({ + where: { + task: { story_id: job.task.story_id }, + status: { in: ['QUEUED', 'CLAIMED', 'RUNNING'] }, + id: { not: jobId }, + }, + }) + if (activeSiblings > 0) { + console.log( + `[update_job_status] cleanup deferred for job=${jobId}: ${activeSiblings} sibling(s) still active in story ${job.task.story_id}`, + ) + return + } + } // Keep branch when job is done and a branch was reported (agent pushed) const keepBranch = status === 'done' && branch !== undefined try { await removeWorktreeForJob({ repoRoot, jobId, keepBranch }) } catch (err) { - console.warn(`[update_job_status] Worktree cleanup failed for job ${jobId}:`, err) + console.warn( + `[update_job_status] cleanup FAILED for job=${jobId} keepBranch=${keepBranch}:`, + err, + ) } } @@ -144,16 +175,33 @@ export async function maybeCreateAutoPr(opts: { const task = await prisma.task.findUnique({ where: { id: taskId }, - select: { title: true, story: { select: { code: true } } }, + select: { + title: true, + story: { select: { id: true, code: true, title: true } }, + }, }) if (!task) return null - const title = task.story.code ? `${task.story.code}: ${task.title}` : task.title - const body = summary - ? `${summary}\n\n---\n\n*Auto-generated by Scrum4Me agent*` - : '*Auto-generated by Scrum4Me agent*' + // Branch-per-story: if a sibling job in the same story already opened a PR, + // reuse its URL. This avoids one PR per sub-task. + const sibling = await prisma.claudeJob.findFirst({ + where: { + task: { story_id: task.story.id }, + pr_url: { not: null }, + id: { not: jobId }, + }, + select: { pr_url: true }, + orderBy: { created_at: 'asc' }, + }) + if (sibling?.pr_url) return sibling.pr_url - const result = await createPullRequest({ worktreePath, branchName, title, body }) + // First DONE-task in the story → create a story-scoped PR + const storyTitle = task.story.code ? `${task.story.code}: ${task.story.title}` : task.story.title + const body = summary + ? `${summary}\n\n---\n\n*Auto-generated by Scrum4Me agent (first task in story; PR-body will accumulate as sibling tasks complete).*` + : '*Auto-generated by Scrum4Me agent (first task in story).*' + + const result = await createPullRequest({ worktreePath, branchName, title: storyTitle, body }) if ('url' in result) return result.url console.warn(`[update_job_status] auto-PR skipped for job ${jobId}:`, result.error) @@ -169,14 +217,10 @@ export function registerUpdateJobStatusTool(server: McpServer) { 'Report progress on a claimed ClaudeJob. Allowed transitions from CLAIMED/RUNNING: ' + 'running (start), done (finished), failed (error). ' + 'The Bearer token must match the token that claimed the job. ' + -<<<<<<< feat/job-mgskzyvx - 'Automatically emits an SSE event so the Scrum4Me UI updates in real time. ' + - 'Response includes next_action: when wait_for_job_again, immediately call wait_for_job again. When queue_empty, the agent batch is done.', -======= 'Before marking done: call verify_task_against_plan first — done is rejected when ' + 'verify_result is null or EMPTY (unless task.verify_only is true). ' + - 'Automatically emits an SSE event so the Scrum4Me UI updates in real time.', ->>>>>>> main + 'Automatically emits an SSE event so the Scrum4Me UI updates in real time. ' + + 'Response includes next_action: when wait_for_job_again, immediately call wait_for_job again. When queue_empty, the agent batch is done.', inputSchema, }, async ({ job_id, status, branch, summary, error }) => diff --git a/src/tools/wait-for-job.ts b/src/tools/wait-for-job.ts index 03ed979..a5b80b0 100644 --- a/src/tools/wait-for-job.ts +++ b/src/tools/wait-for-job.ts @@ -12,6 +12,13 @@ import { requireWriteAccess } from '../auth.js' import { toolJson, toolError, withToolErrors } from '../errors.js' import { createWorktreeForJob } from '../git/worktree.js' +/** Parse `https://github.com//(.git)?` → ``. */ +export function repoNameFromUrl(repoUrl: string | null | undefined): string | null { + if (!repoUrl) return null + const m = repoUrl.match(/[/:]([^/]+?)(?:\.git)?\/?$/) + return m ? m[1] : null +} + export async function resolveRepoRoot(productId: string): Promise { const envKey = `SCRUM4ME_REPO_ROOT_${productId}` if (process.env[envKey]) return process.env[envKey]! @@ -20,7 +27,24 @@ export async function resolveRepoRoot(productId: string): Promise try { const raw = await fs.readFile(configPath, 'utf-8') const config = JSON.parse(raw) as { repoRoots?: Record } - return config.repoRoots?.[productId] ?? null + if (config.repoRoots?.[productId]) return config.repoRoots[productId] + } catch { + // ignore — fall through to convention-based fallback + } + + // Convention-based fallback: ~/Projects/ with .git/ inside. + // Lets the agent work without explicit env-config when checkouts follow + // the standard ~/Projects/ layout. + try { + const product = await prisma.product.findUnique({ + where: { id: productId }, + select: { repo_url: true }, + }) + const name = repoNameFromUrl(product?.repo_url) + if (!name) return null + const candidate = path.join(os.homedir(), 'Projects', name) + await fs.access(path.join(candidate, '.git')) + return candidate } catch { return null } @@ -34,10 +58,39 @@ export async function rollbackClaim(jobId: string): Promise { ` } +/** + * Resolve the branch name for a newly-claimed job. + * + * Branch-per-story: if a sibling job in the same story already has a branch + * (assigned during its own claim), reuse it so all sub-tasks in the story + * land in one PR. Otherwise generate a fresh `feat/story-<8-char>` name. + * + * Returns also `siblingHasActiveWorktree` so the caller can decide to remove + * a stale sibling worktree before creating a new one (git refuses to check + * out the same branch in two worktrees). + */ +export async function resolveBranchForJob( + jobId: string, + storyId: string, +): Promise<{ branchName: string; reused: boolean }> { + const sibling = await prisma.claudeJob.findFirst({ + where: { + task: { story_id: storyId }, + branch: { not: null }, + id: { not: jobId }, + }, + orderBy: { created_at: 'asc' }, + select: { branch: true }, + }) + if (sibling?.branch) return { branchName: sibling.branch, reused: true } + return { branchName: `feat/story-${storyId.slice(-8)}`, reused: false } +} + export async function attachWorktreeToJob( productId: string, jobId: string, -): Promise<{ worktree_path: string; branch_name: string } | { error: string }> { + storyId: string, +): Promise<{ worktree_path: string; branch_name: string; reused_branch: boolean } | { error: string }> { const repoRoot = await resolveRepoRoot(productId) if (!repoRoot) { await rollbackClaim(jobId) @@ -48,14 +101,15 @@ export async function attachWorktreeToJob( } } - const branchName = `feat/job-${jobId.slice(-8)}` + const { branchName, reused } = await resolveBranchForJob(jobId, storyId) try { const { worktreePath, branchName: actualBranch } = await createWorktreeForJob({ repoRoot, jobId, branchName, + reuseBranch: reused, }) - return { worktree_path: worktreePath, branch_name: actualBranch } + return { worktree_path: worktreePath, branch_name: actualBranch, reused_branch: reused } } catch (err) { await rollbackClaim(jobId) return { error: `Worktree creation failed: ${(err as Error).message}` } @@ -280,7 +334,7 @@ export function registerWaitForJobTool(server: McpServer) { if (jobId) { const ctx = await getFullJobContext(jobId) if (!ctx) return toolError('Job claimed but context fetch failed') - const wt = await attachWorktreeToJob(ctx.product.id, jobId) + const wt = await attachWorktreeToJob(ctx.product.id, jobId, ctx.story.id) if ('error' in wt) return toolError(wt.error) return toolJson({ ...ctx, worktree_path: wt.worktree_path, branch_name: wt.branch_name }) } @@ -318,7 +372,7 @@ export function registerWaitForJobTool(server: McpServer) { if (jobId) { const ctx = await getFullJobContext(jobId) if (!ctx) return toolError('Job claimed but context fetch failed') - const wt = await attachWorktreeToJob(ctx.product.id, jobId) + const wt = await attachWorktreeToJob(ctx.product.id, jobId, ctx.story.id) if ('error' in wt) return toolError(wt.error) return toolJson({ ...ctx, worktree_path: wt.worktree_path, branch_name: wt.branch_name }) } diff --git a/src/verify/classify.ts b/src/verify/classify.ts index 49991bc..e713232 100644 --- a/src/verify/classify.ts +++ b/src/verify/classify.ts @@ -66,6 +66,21 @@ export function classifyDiffAgainstPlan(opts: { return { result: 'EMPTY', reasoning: 'Geen bestandswijzigingen in de diff.' } } + // Whitespace-only / no-content edge case: paths are present but every +/- + // line is a diff header (---/+++) or whitespace-only. Treat as EMPTY so the + // gate rejects DONE for tasks that didn't really change anything. + const meaningfulChange = diff.split('\n').some((l) => { + if (!/^[+-]/.test(l)) return false + if (/^[+-]{3}\s/.test(l)) return false // diff header line (--- / +++) + return l.slice(1).trim().length > 0 + }) + if (!meaningfulChange) { + return { + result: 'EMPTY', + reasoning: 'Diff bevat alleen headers of whitespace — geen daadwerkelijke content-wijzigingen.', + } + } + const changedLines = diff.split('\n').filter((l) => l.startsWith('+') || l.startsWith('-')).length if (!plan || plan.trim().length === 0) {