Compare commits

..

1 commit

Author SHA1 Message Date
Janpeter Visser
30860dd8ea fix(update_idea_plan_reviewed): nooit stilzwijgend goedkeuren (IDEA-066)
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) <noreply@anthropic.com>
2026-05-14 19:45:15 +02:00
2 changed files with 4 additions and 109 deletions

View file

@ -1,74 +0,0 @@
// 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)
})
})

View file

@ -390,32 +390,6 @@ 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
@ -595,8 +569,6 @@ 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 ' +
'doesnt meet task.verify_required: ALIGNED-only is strict; ALIGNED_OR_PARTIAL accepts ' +
@ -636,8 +608,6 @@ 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,
@ -781,11 +751,10 @@ export function registerUpdateJobStatusTool(server: McpServer) {
where: { id: job_id },
data: {
status: dbStatus,
...resolveJobTimestamps(
actualStatus,
{ claimed_at: job.claimed_at, started_at: job.started_at },
now,
),
...(actualStatus === 'running' ? { started_at: now } : {}),
...(actualStatus === 'done' || actualStatus === 'failed' || actualStatus === 'skipped'
? { finished_at: now }
: {}),
...(branchToWrite !== undefined ? { branch: branchToWrite } : {}),
...(pushedAt !== undefined ? { pushed_at: pushedAt } : {}),
...(summary !== undefined ? { summary } : {}),