From fba2d67796b10ed20c76b8eb74072d57ce2c9fa1 Mon Sep 17 00:00:00 2001 From: Janpeter Visser <30029041+madhura68@users.noreply.github.com> Date: Thu, 14 May 2026 23:21:44 +0200 Subject: [PATCH] fix(update_job_status): status-gedreven lifecycle-timestamps (#51) Een job kon CLAIMED -> done/failed/skipped gaan zonder ooit `running` te rapporteren, waardoor started_at NULL bleef terwijl finished_at wel gezet werd. Dat brak de invariant claimed_at <= started_at <= finished_at en elke duur-analyse. Nieuwe pure helper resolveJobTimestamps zet de lifecycle-timestamps set-once op basis van de status: started_at wordt gebackfild bij een terminale overgang, claimed_at defensief gevuld als die ontbreekt. De running-tak is nu set-once i.p.v. bij elke call overschrijven. Co-authored-by: Madhura68 Co-authored-by: Claude Opus 4.7 (1M context) --- .../update-job-status-timestamps.test.ts | 74 +++++++++++++++++++ src/tools/update-job-status.ts | 39 +++++++++- 2 files changed, 109 insertions(+), 4 deletions(-) create mode 100644 __tests__/update-job-status-timestamps.test.ts diff --git a/__tests__/update-job-status-timestamps.test.ts b/__tests__/update-job-status-timestamps.test.ts new file mode 100644 index 0000000..d4ab80f --- /dev/null +++ b/__tests__/update-job-status-timestamps.test.ts @@ -0,0 +1,74 @@ +// 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) + }) +}) diff --git a/src/tools/update-job-status.ts b/src/tools/update-job-status.ts index e7a9495..9fcd08b 100644 --- a/src/tools/update-job-status.ts +++ b/src/tools/update-job-status.ts @@ -390,6 +390,32 @@ 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 @@ -569,6 +595,8 @@ 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 ' + 'doesn’t meet task.verify_required: ALIGNED-only is strict; ALIGNED_OR_PARTIAL accepts ' + @@ -608,6 +636,8 @@ 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, @@ -751,10 +781,11 @@ export function registerUpdateJobStatusTool(server: McpServer) { where: { id: job_id }, data: { status: dbStatus, - ...(actualStatus === 'running' ? { started_at: now } : {}), - ...(actualStatus === 'done' || actualStatus === 'failed' || actualStatus === 'skipped' - ? { finished_at: now } - : {}), + ...resolveJobTimestamps( + actualStatus, + { claimed_at: job.claimed_at, started_at: job.started_at }, + now, + ), ...(branchToWrite !== undefined ? { branch: branchToWrite } : {}), ...(pushedAt !== undefined ? { pushed_at: pushedAt } : {}), ...(summary !== undefined ? { summary } : {}),