PBI-57: 'skipped' no-op exit + cascade preserves original error
When verify_task_against_plan returns EMPTY because the requested changes
already live in origin/main (parallel work, earlier PR, race between
siblings), the worker had no clean exit: update_job_status only accepted
running|done|failed. 'failed' triggered the PBI fail-cascade which then
overwrote the error column with 'cancelled_by_self' and cancelled all
sibling tasks of the PBI — see Scrum4Me job cmovkur8 / T-695 for the
reference incident.
This change introduces a fourth status and tightens the cascade:
ST-1273 — 'skipped' exit in update_job_status (T-706 + T-707)
- src/tools/update-job-status.ts: status enum + DB_STATUS_MAP +
resolveNextAction now include 'skipped'. cleanupWorktreeForTerminalStatus
signature widened to ('done'|'failed'|'skipped'); SKIPPED uses keepBranch
semantics identical to FAILED (no push, no branch keep). New input guard:
'skipped' is only valid for TASK_IMPLEMENTATION jobs and requires a
non-empty error (≥10 chars) explaining the reason — it bypasses the
verify-gate, the auto-PR, the SprintRun finalize/fail paths and the
PBI fail-cascade. Locks are still released on terminal exit.
- Tool description spells out when to pick 'skipped' so MCP clients see it.
- New __tests__/update-job-status-skipped.test.ts: resolveNextAction with
'skipped' (wait_for_job_again / queue_empty), and cleanupWorktreeForTerminalStatus
with status='skipped' (keepBranch=false even with a branch reported,
defers cleanup with active siblings).
ST-1274 — cascade ignores SKIPPED + appends trace (T-708 + T-709)
- src/cancel/pbi-cascade.ts: runCascade reads job.status, returns EMPTY
when status === 'SKIPPED' (no sibling cancel). Trace persistence now
reads the current error first and writes `${original}\n---\n${trace}`
(truncated at 1900 chars), so the original failure cause is preserved
for forensics instead of being overwritten.
- New cases in __tests__/cancel-pbi-cascade.test.ts: SKIPPED entry-guard
(no findMany / updateMany / update), original error preserved with
trace appended after '---', trace-only fallback when no original
error, 1900-char truncation keeps the head of the original.
All 282 scrum4me-mcp tests pass; tsc build clean.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
8ffb680a1a
commit
458b7a7d45
4 changed files with 212 additions and 9 deletions
|
|
@ -285,4 +285,66 @@ describe('cancelPbiOnFailure', () => {
|
|||
|
||||
expect(out.warnings.some((w) => w.includes('boom'))).toBe(true)
|
||||
})
|
||||
|
||||
it('no-ops when failed job has status SKIPPED (no-op exit, niet een echte fail)', async () => {
|
||||
mockPrisma.claudeJob.findUnique.mockResolvedValue({ ...FAILED_JOB, status: 'SKIPPED' })
|
||||
|
||||
const out = await cancelPbiOnFailure('job-failed')
|
||||
|
||||
expect(out.cancelled_job_ids).toEqual([])
|
||||
expect(mockPrisma.claudeJob.findMany).not.toHaveBeenCalled()
|
||||
expect(mockPrisma.claudeJob.updateMany).not.toHaveBeenCalled()
|
||||
expect(mockPrisma.claudeJob.update).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('appends the cascade trace to an existing error (preserves original cause)', async () => {
|
||||
// findUnique wordt twee keer aangeroepen: eerst voor failedJob (status FAILED + originele error),
|
||||
// daarna door de append-trace om de huidige error te lezen vóór update.
|
||||
mockPrisma.claudeJob.findUnique
|
||||
.mockResolvedValueOnce({ ...FAILED_JOB, status: 'FAILED' })
|
||||
.mockResolvedValueOnce({ error: 'timeout: agent died after 5min' })
|
||||
mockPrisma.claudeJob.findMany.mockResolvedValue([])
|
||||
|
||||
await cancelPbiOnFailure('job-failed')
|
||||
|
||||
expect(mockPrisma.claudeJob.update).toHaveBeenCalledWith(
|
||||
expect.objectContaining({
|
||||
where: { id: 'job-failed' },
|
||||
data: expect.objectContaining({
|
||||
error: expect.stringMatching(/timeout: agent died after 5min[\s\S]*---[\s\S]*cancelled_by_self/),
|
||||
}),
|
||||
}),
|
||||
)
|
||||
})
|
||||
|
||||
it('falls back to trace-only when there is no existing error', async () => {
|
||||
mockPrisma.claudeJob.findUnique
|
||||
.mockResolvedValueOnce({ ...FAILED_JOB, status: 'FAILED' })
|
||||
.mockResolvedValueOnce({ error: null })
|
||||
mockPrisma.claudeJob.findMany.mockResolvedValue([])
|
||||
|
||||
await cancelPbiOnFailure('job-failed')
|
||||
|
||||
const updateCall = mockPrisma.claudeJob.update.mock.calls[0]?.[0] as
|
||||
| { data: { error: string } }
|
||||
| undefined
|
||||
expect(updateCall?.data.error).toMatch(/^cancelled_by_self/)
|
||||
expect(updateCall?.data.error).not.toContain('---')
|
||||
})
|
||||
|
||||
it('truncates the merged error at 1900 chars while preserving the head of the original', async () => {
|
||||
const longOriginal = 'X'.repeat(1800)
|
||||
mockPrisma.claudeJob.findUnique
|
||||
.mockResolvedValueOnce({ ...FAILED_JOB, status: 'FAILED' })
|
||||
.mockResolvedValueOnce({ error: longOriginal })
|
||||
mockPrisma.claudeJob.findMany.mockResolvedValue([])
|
||||
|
||||
await cancelPbiOnFailure('job-failed')
|
||||
|
||||
const updateCall = mockPrisma.claudeJob.update.mock.calls[0]?.[0] as
|
||||
| { data: { error: string } }
|
||||
| undefined
|
||||
expect(updateCall?.data.error.length).toBeLessThanOrEqual(1900)
|
||||
expect(updateCall?.data.error.startsWith('X')).toBe(true)
|
||||
})
|
||||
})
|
||||
|
|
|
|||
95
__tests__/update-job-status-skipped.test.ts
Normal file
95
__tests__/update-job-status-skipped.test.ts
Normal file
|
|
@ -0,0 +1,95 @@
|
|||
// Unit-tests voor de no-op SKIPPED exit-route in update_job_status (PBI-57 ST-1273).
|
||||
// Volle handler-integratie wordt niet hier getest — die hangt aan tientallen
|
||||
// MCP/Prisma-mocks. Wel testen we de geëxporteerde helpers die expliciet
|
||||
// SKIPPED-aware zijn gemaakt: resolveNextAction en cleanupWorktreeForTerminalStatus.
|
||||
|
||||
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(),
|
||||
}))
|
||||
|
||||
vi.mock('../src/tools/wait-for-job.js', async (importOriginal) => {
|
||||
const original = await importOriginal<typeof import('../src/tools/wait-for-job.js')>()
|
||||
return {
|
||||
...original,
|
||||
resolveRepoRoot: vi.fn(),
|
||||
}
|
||||
})
|
||||
|
||||
import { prisma } from '../src/prisma.js'
|
||||
import { removeWorktreeForJob } from '../src/git/worktree.js'
|
||||
import { resolveRepoRoot } from '../src/tools/wait-for-job.js'
|
||||
import {
|
||||
cleanupWorktreeForTerminalStatus,
|
||||
resolveNextAction,
|
||||
} 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()
|
||||
mockPrisma.claudeJob.findUnique.mockResolvedValue({ task: { story_id: 'story-default' } })
|
||||
mockPrisma.claudeJob.count.mockResolvedValue(0)
|
||||
})
|
||||
|
||||
describe('resolveNextAction — skipped pad', () => {
|
||||
it('returns wait_for_job_again when queue has jobs after skipped', () => {
|
||||
expect(resolveNextAction(2, 'skipped')).toBe('wait_for_job_again')
|
||||
})
|
||||
|
||||
it('returns queue_empty when queue is empty after skipped', () => {
|
||||
expect(resolveNextAction(0, 'skipped')).toBe('queue_empty')
|
||||
})
|
||||
})
|
||||
|
||||
describe('cleanupWorktreeForTerminalStatus — skipped pad', () => {
|
||||
it('calls removeWorktreeForJob with keepBranch=false when skipped (no push happened)', async () => {
|
||||
mockResolve.mockResolvedValue('/repos/my-project')
|
||||
mockRemove.mockResolvedValue({ removed: true })
|
||||
|
||||
await cleanupWorktreeForTerminalStatus('prod-001', 'job-skip', 'skipped', undefined)
|
||||
|
||||
expect(mockRemove).toHaveBeenCalledWith({
|
||||
repoRoot: '/repos/my-project',
|
||||
jobId: 'job-skip',
|
||||
keepBranch: false,
|
||||
})
|
||||
})
|
||||
|
||||
it('keeps keepBranch=false when skipped even if a branch is reported', async () => {
|
||||
mockResolve.mockResolvedValue('/repos/my-project')
|
||||
mockRemove.mockResolvedValue({ removed: true })
|
||||
|
||||
await cleanupWorktreeForTerminalStatus('prod-001', 'job-skip', 'skipped', 'feat/job-skip')
|
||||
|
||||
expect(mockRemove).toHaveBeenCalledWith({
|
||||
repoRoot: '/repos/my-project',
|
||||
jobId: 'job-skip',
|
||||
keepBranch: false,
|
||||
})
|
||||
})
|
||||
|
||||
it('defers cleanup when sibling jobs in same story are still active (skipped path)', async () => {
|
||||
mockResolve.mockResolvedValue('/repos/my-project')
|
||||
mockPrisma.claudeJob.findUnique.mockResolvedValue({ task: { story_id: 'story-shared' } })
|
||||
mockPrisma.claudeJob.count.mockResolvedValue(1)
|
||||
|
||||
await cleanupWorktreeForTerminalStatus('prod-001', 'job-skip', 'skipped', undefined)
|
||||
|
||||
expect(mockRemove).not.toHaveBeenCalled()
|
||||
})
|
||||
})
|
||||
Loading…
Add table
Add a link
Reference in a new issue