feat: branch-per-story + worktree-defer + verify EMPTY edge-cases (#12)

Implementeert vier open stories uit PBI 'Veilige Claude-agent-workflow':

**Branch per story (cmon11tbe001zbortx35n155c)**
- `resolveBranchForJob`: zoek sibling-job in dezelfde story; reuse z'n
  branch (1 PR per story i.p.v. per task).
- Branch-naam: `feat/story-<8-char>` voor nieuwe stories.
- `createWorktreeForJob` kent nu `reuseBranch=true`: detecteert stale
  sibling-worktree die de branch nog vasthoudt en verwijdert die eerst.
- `attachWorktreeToJob` neemt `storyId` mee.

**PR-hergebruik (zelfde story)**
- `maybeCreateAutoPr`: als sibling-job in story al een pr_url heeft,
  hergebruik die zonder nieuwe `gh pr create`-call. PR-titel komt nu
  van de story (was task) zodat het als 'story-PR' leest.

**Worktree-cleanup uitgesteld bij actieve siblings**
- `cleanupWorktreeForTerminalStatus`: count active sibling-jobs in
  dezelfde story; defer als > 0 (volgende sub-task gebruikt branch).

**Worktree-cleanup logging (cmon0jc14001ubortjxf2a2ck)**
- Warning bij ontbrekende repoRoot, met productId + jobId in message.
- Warning bij removeWorktreeForJob-failure met keepBranch in message.

**resolveRepoRoot fallback (cmon0jc14001ubortjxf2a2ck)**
- Convention-based fallback: `~/Projects/<repo-name>` afgeleid uit
  `product.repo_url` als noch env-var noch config-bestand iets oplevert.
- `repoNameFromUrl` helper geëxporteerd voor herbruikbaarheid.

**Verify EMPTY-detection edge-case (cmon0kdq6001xbort2kgbcqmr)**
- `classifyDiffAgainstPlan`: na file-paths-check ook content-lines
  checken; als alle +/- regels alleen headers of whitespace zijn,
  return EMPTY met duidelijke reasoning.

Tests: 120/120 groen (3 nieuwe), tsc clean, build clean.

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Janpeter Visser 2026-05-01 17:04:54 +02:00 committed by GitHub
parent f87b20744b
commit f01fab8c38
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
7 changed files with 248 additions and 36 deletions

View file

@ -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<typeof vi.fn> }
task: { findUnique: ReturnType<typeof vi.fn> }
claudeJob: { findFirst: ReturnType<typeof vi.fn> }
}
const mockCreatePr = createPullRequest as ReturnType<typeof vi.fn>
@ -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' }),
)
})

View file

@ -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<typeof vi.fn>
const mockResolve = resolveRepoRoot as ReturnType<typeof vi.fn>
const mockPrisma = prisma as unknown as {
claudeJob: {
findUnique: ReturnType<typeof vi.fn>
count: ReturnType<typeof vi.fn>
}
}
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()
})
})

View file

@ -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<typeof vi.fn> }
const mockPrisma = prisma as unknown as {
$executeRaw: ReturnType<typeof vi.fn>
claudeJob: { findFirst: ReturnType<typeof vi.fn> }
product: { findUnique: ReturnType<typeof vi.fn> }
}
const mockCreateWorktree = createWorktreeForJob as ReturnType<typeof vi.fn>
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')

View file

@ -15,13 +15,39 @@ async function branchExists(repoRoot: string, name: string): Promise<boolean> {
}
}
async function findWorktreeForBranch(
repoRoot: string,
branchName: string,
): Promise<string | null> {
try {
const { stdout } = await exec('git', ['worktree', 'list', '--porcelain'], { cwd: repoRoot })
// Porcelain blocks: worktree <path>\nHEAD <sha>\nbranch refs/heads/<name>\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/<name>
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()}`
}

View file

@ -30,14 +30,45 @@ export async function cleanupWorktreeForTerminalStatus(
branch: string | undefined,
): Promise<void> {
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 }) =>

View file

@ -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/<owner>/<name>(.git)?` → `<name>`. */
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<string | null> {
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<string | null>
try {
const raw = await fs.readFile(configPath, 'utf-8')
const config = JSON.parse(raw) as { repoRoots?: Record<string, string> }
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/<repo-name> with .git/ inside.
// Lets the agent work without explicit env-config when checkouts follow
// the standard ~/Projects/<name> 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<void> {
`
}
/**
* 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 })
}

View file

@ -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) {