From 70cb4ad2b0412e2902d98b1153583aa7766b267d Mon Sep 17 00:00:00 2001 From: Janpeter Visser <30029041+madhura68@users.noreply.github.com> Date: Thu, 14 May 2026 19:13:38 +0200 Subject: [PATCH] fix(cross-repo): per-repo worktree-branch + PR resolutie (IDEA-062) 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