From b774dcaae50f570c284240932478610bc75d1d45 Mon Sep 17 00:00:00 2001 From: ejclaw Date: Wed, 29 Apr 2026 10:44:28 +0900 Subject: [PATCH] Extract paired task status helpers --- src/message-runtime.ts | 2 +- src/paired-arbiter-request.ts | 2 +- src/paired-execution-context-arbiter.ts | 2 +- src/paired-execution-context-owner.ts | 2 +- src/paired-execution-context-reviewer.ts | 2 +- src/paired-execution-context-shared.ts | 150 ++--------------------- src/paired-execution-context.ts | 2 +- src/paired-task-status.test.ts | 118 ++++++++++++++++++ src/paired-task-status.ts | 139 +++++++++++++++++++++ src/paired-workspace-manager.ts | 2 +- 10 files changed, 274 insertions(+), 147 deletions(-) create mode 100644 src/paired-task-status.test.ts create mode 100644 src/paired-task-status.ts diff --git a/src/message-runtime.ts b/src/message-runtime.ts index b2645dd..aa72b36 100644 --- a/src/message-runtime.ts +++ b/src/message-runtime.ts @@ -48,7 +48,7 @@ import { schedulePairedFollowUpWithMessageCheck, } from './message-runtime-follow-up.js'; import { type ScheduledPairedFollowUpIntentKind } from './paired-follow-up-scheduler.js'; -import { transitionPairedTaskStatus } from './paired-execution-context-shared.js'; +import { transitionPairedTaskStatus } from './paired-task-status.js'; import { Channel, NewMessage, diff --git a/src/paired-arbiter-request.ts b/src/paired-arbiter-request.ts index 92741a5..b653a75 100644 --- a/src/paired-arbiter-request.ts +++ b/src/paired-arbiter-request.ts @@ -1,6 +1,6 @@ import { isArbiterEnabled } from './config.js'; import { logger } from './logger.js'; -import { transitionPairedTaskStatus } from './paired-execution-context-shared.js'; +import { transitionPairedTaskStatus } from './paired-task-status.js'; import type { PairedTaskStatus } from './types.js'; export function requestArbiterOrEscalate(args: { diff --git a/src/paired-execution-context-arbiter.ts b/src/paired-execution-context-arbiter.ts index 3c58810..519228e 100644 --- a/src/paired-execution-context-arbiter.ts +++ b/src/paired-execution-context-arbiter.ts @@ -1,6 +1,6 @@ import { ARBITER_DEADLOCK_THRESHOLD } from './config.js'; import { logger } from './logger.js'; -import { transitionPairedTaskStatus } from './paired-execution-context-shared.js'; +import { transitionPairedTaskStatus } from './paired-task-status.js'; import { classifyArbiterVerdict } from './paired-verdict.js'; import type { PairedTask } from './types.js'; diff --git a/src/paired-execution-context-owner.ts b/src/paired-execution-context-owner.ts index cb72009..072985d 100644 --- a/src/paired-execution-context-owner.ts +++ b/src/paired-execution-context-owner.ts @@ -13,7 +13,7 @@ import { requestArbiterOrEscalate } from './paired-arbiter-request.js'; import { applyPairedTaskPatch, transitionPairedTaskStatus, -} from './paired-execution-context-shared.js'; +} from './paired-task-status.js'; import { resolveOwnerCompletionSignal } from './paired-completion-signals.js'; import { hasCodeChangesSinceRef, diff --git a/src/paired-execution-context-reviewer.ts b/src/paired-execution-context-reviewer.ts index 934d84d..37a6125 100644 --- a/src/paired-execution-context-reviewer.ts +++ b/src/paired-execution-context-reviewer.ts @@ -2,7 +2,7 @@ import { ARBITER_DEADLOCK_THRESHOLD } from './config.js'; import { getPairedWorkspace } from './db.js'; import { logger } from './logger.js'; import { requestArbiterOrEscalate } from './paired-arbiter-request.js'; -import { transitionPairedTaskStatus } from './paired-execution-context-shared.js'; +import { transitionPairedTaskStatus } from './paired-task-status.js'; import { resolveReviewerCompletionSignal, resolveReviewerFailureSignal, diff --git a/src/paired-execution-context-shared.ts b/src/paired-execution-context-shared.ts index 7b80b35..03a3a07 100644 --- a/src/paired-execution-context-shared.ts +++ b/src/paired-execution-context-shared.ts @@ -1,7 +1,3 @@ -import { updatePairedTaskIfUnchanged } from './db.js'; -import { logger } from './logger.js'; -import type { PairedTaskStatus } from './types.js'; - export { requestArbiterOrEscalate } from './paired-arbiter-request.js'; export type { RequestArbiterOrEscalateFn } from './paired-arbiter-request.js'; export { @@ -19,139 +15,13 @@ export { hasCodeChangesSinceRef, resolveCanonicalSourceRef, } from './paired-source-ref.js'; - -const ALLOWED_PAIRED_STATUS_TRANSITIONS: Record< - PairedTaskStatus, - ReadonlySet -> = { - active: new Set(['review_ready', 'arbiter_requested', 'completed']), - review_ready: new Set([ - 'active', - 'in_review', - 'arbiter_requested', - 'completed', - ]), - in_review: new Set([ - 'active', - 'review_ready', - 'merge_ready', - 'arbiter_requested', - 'completed', - ]), - merge_ready: new Set(['active', 'arbiter_requested', 'completed']), - completed: new Set(), - arbiter_requested: new Set(['in_arbitration', 'completed']), - in_arbitration: new Set(['active', 'arbiter_requested', 'completed']), -}; - -export function assertPairedTaskStatusTransition(args: { - currentStatus: PairedTaskStatus; - nextStatus: PairedTaskStatus; -}): void { - const { currentStatus, nextStatus } = args; - if (currentStatus === nextStatus) { - return; - } - - if (ALLOWED_PAIRED_STATUS_TRANSITIONS[currentStatus].has(nextStatus)) { - return; - } - - throw new Error( - `Invalid paired task status transition: ${currentStatus} -> ${nextStatus}`, - ); -} - -export function transitionPairedTaskStatus(args: { - taskId: string; - currentStatus: PairedTaskStatus; - nextStatus: PairedTaskStatus; - expectedUpdatedAt: string; - updatedAt: string; - patch?: { - title?: string | null; - source_ref?: string | null; - plan_notes?: string | null; - review_requested_at?: string | null; - round_trip_count?: number; - owner_failure_count?: number; - owner_step_done_streak?: number; - finalize_step_done_count?: number; - task_done_then_user_reopen_count?: number; - empty_step_done_streak?: number; - arbiter_verdict?: string | null; - arbiter_requested_at?: string | null; - completion_reason?: string | null; - }; -}): boolean { - assertPairedTaskStatusTransition({ - currentStatus: args.currentStatus, - nextStatus: args.nextStatus, - }); - - const updated = updatePairedTaskIfUnchanged( - args.taskId, - args.expectedUpdatedAt, - { - ...args.patch, - status: args.nextStatus, - updated_at: args.updatedAt, - }, - ); - if (!updated) { - logger.warn( - { - taskId: args.taskId, - currentStatus: args.currentStatus, - nextStatus: args.nextStatus, - expectedUpdatedAt: args.expectedUpdatedAt, - }, - 'Skipped stale paired task status transition because the task revision changed', - ); - } - return updated; -} - -export function applyPairedTaskPatch(args: { - taskId: string; - expectedUpdatedAt: string; - updatedAt: string; - patch: { - title?: string | null; - source_ref?: string | null; - plan_notes?: string | null; - review_requested_at?: string | null; - round_trip_count?: number; - owner_failure_count?: number; - owner_step_done_streak?: number; - finalize_step_done_count?: number; - task_done_then_user_reopen_count?: number; - empty_step_done_streak?: number; - status?: PairedTaskStatus; - arbiter_verdict?: string | null; - arbiter_requested_at?: string | null; - completion_reason?: string | null; - }; -}): boolean { - const updated = updatePairedTaskIfUnchanged( - args.taskId, - args.expectedUpdatedAt, - { - ...args.patch, - updated_at: args.updatedAt, - }, - ); - if (!updated) { - logger.warn( - { - taskId: args.taskId, - expectedUpdatedAt: args.expectedUpdatedAt, - }, - 'Skipped stale paired task patch because the task revision changed', - ); - } - return updated; -} - -export type TransitionPairedTaskStatusFn = typeof transitionPairedTaskStatus; -export type ApplyPairedTaskPatchFn = typeof applyPairedTaskPatch; +export { + ALLOWED_PAIRED_STATUS_TRANSITIONS, + applyPairedTaskPatch, + assertPairedTaskStatusTransition, + transitionPairedTaskStatus, +} from './paired-task-status.js'; +export type { + ApplyPairedTaskPatchFn, + TransitionPairedTaskStatusFn, +} from './paired-task-status.js'; diff --git a/src/paired-execution-context.ts b/src/paired-execution-context.ts index 5d72135..f78d2fa 100644 --- a/src/paired-execution-context.ts +++ b/src/paired-execution-context.ts @@ -49,7 +49,7 @@ import { import { applyPairedTaskPatch, transitionPairedTaskStatus, -} from './paired-execution-context-shared.js'; +} from './paired-task-status.js'; import { requestArbiterOrEscalate } from './paired-arbiter-request.js'; import { resolveCanonicalSourceRef } from './paired-source-ref.js'; import { diff --git a/src/paired-task-status.test.ts b/src/paired-task-status.test.ts new file mode 100644 index 0000000..ee39ba9 --- /dev/null +++ b/src/paired-task-status.test.ts @@ -0,0 +1,118 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +vi.mock('./db.js', () => ({ + updatePairedTaskIfUnchanged: vi.fn(() => true), +})); + +vi.mock('./logger.js', () => ({ + logger: { + warn: vi.fn(), + }, +})); + +import { updatePairedTaskIfUnchanged } from './db.js'; +import { logger } from './logger.js'; +import { + applyPairedTaskPatch, + assertPairedTaskStatusTransition, + transitionPairedTaskStatus, +} from './paired-task-status.js'; + +describe('paired task status transitions', () => { + beforeEach(() => { + vi.mocked(updatePairedTaskIfUnchanged).mockReset(); + vi.mocked(updatePairedTaskIfUnchanged).mockReturnValue(true); + vi.mocked(logger.warn).mockReset(); + }); + + it('allows valid transitions and same-status no-ops', () => { + expect(() => + assertPairedTaskStatusTransition({ + currentStatus: 'active', + nextStatus: 'review_ready', + }), + ).not.toThrow(); + expect(() => + assertPairedTaskStatusTransition({ + currentStatus: 'review_ready', + nextStatus: 'review_ready', + }), + ).not.toThrow(); + }); + + it('rejects invalid transitions', () => { + expect(() => + assertPairedTaskStatusTransition({ + currentStatus: 'completed', + nextStatus: 'active', + }), + ).toThrow('Invalid paired task status transition: completed -> active'); + }); + + it('writes status transitions with optimistic revision guard', () => { + const updated = transitionPairedTaskStatus({ + taskId: 'task-1', + currentStatus: 'active', + nextStatus: 'review_ready', + expectedUpdatedAt: '2026-04-29T01:00:00.000Z', + updatedAt: '2026-04-29T01:01:00.000Z', + patch: { + review_requested_at: '2026-04-29T01:01:00.000Z', + }, + }); + + expect(updated).toBe(true); + expect(updatePairedTaskIfUnchanged).toHaveBeenCalledWith( + 'task-1', + '2026-04-29T01:00:00.000Z', + { + review_requested_at: '2026-04-29T01:01:00.000Z', + status: 'review_ready', + updated_at: '2026-04-29T01:01:00.000Z', + }, + ); + }); + + it('logs stale status transitions without writing success', () => { + vi.mocked(updatePairedTaskIfUnchanged).mockReturnValue(false); + + const updated = transitionPairedTaskStatus({ + taskId: 'task-1', + currentStatus: 'active', + nextStatus: 'completed', + expectedUpdatedAt: 'old', + updatedAt: 'new', + patch: { + completion_reason: 'done', + }, + }); + + expect(updated).toBe(false); + expect(logger.warn).toHaveBeenCalledWith( + { + taskId: 'task-1', + currentStatus: 'active', + nextStatus: 'completed', + expectedUpdatedAt: 'old', + }, + 'Skipped stale paired task status transition because the task revision changed', + ); + }); + + it('applies non-status patches with optimistic revision guard', () => { + const updated = applyPairedTaskPatch({ + taskId: 'task-1', + expectedUpdatedAt: 'old', + updatedAt: 'new', + patch: { + owner_failure_count: 2, + }, + }); + + expect(updated).toBe(true); + expect(updatePairedTaskIfUnchanged).toHaveBeenCalledWith('task-1', 'old', { + owner_failure_count: 2, + updated_at: 'new', + }); + }); +}); diff --git a/src/paired-task-status.ts b/src/paired-task-status.ts new file mode 100644 index 0000000..c9a03dd --- /dev/null +++ b/src/paired-task-status.ts @@ -0,0 +1,139 @@ +import { updatePairedTaskIfUnchanged } from './db.js'; +import { logger } from './logger.js'; +import type { PairedTaskStatus } from './types.js'; + +export const ALLOWED_PAIRED_STATUS_TRANSITIONS: Record< + PairedTaskStatus, + ReadonlySet +> = { + active: new Set(['review_ready', 'arbiter_requested', 'completed']), + review_ready: new Set([ + 'active', + 'in_review', + 'arbiter_requested', + 'completed', + ]), + in_review: new Set([ + 'active', + 'review_ready', + 'merge_ready', + 'arbiter_requested', + 'completed', + ]), + merge_ready: new Set(['active', 'arbiter_requested', 'completed']), + completed: new Set(), + arbiter_requested: new Set(['in_arbitration', 'completed']), + in_arbitration: new Set(['active', 'arbiter_requested', 'completed']), +}; + +export function assertPairedTaskStatusTransition(args: { + currentStatus: PairedTaskStatus; + nextStatus: PairedTaskStatus; +}): void { + const { currentStatus, nextStatus } = args; + if (currentStatus === nextStatus) { + return; + } + + if (ALLOWED_PAIRED_STATUS_TRANSITIONS[currentStatus].has(nextStatus)) { + return; + } + + throw new Error( + `Invalid paired task status transition: ${currentStatus} -> ${nextStatus}`, + ); +} + +export function transitionPairedTaskStatus(args: { + taskId: string; + currentStatus: PairedTaskStatus; + nextStatus: PairedTaskStatus; + expectedUpdatedAt: string; + updatedAt: string; + patch?: { + title?: string | null; + source_ref?: string | null; + plan_notes?: string | null; + review_requested_at?: string | null; + round_trip_count?: number; + owner_failure_count?: number; + owner_step_done_streak?: number; + finalize_step_done_count?: number; + task_done_then_user_reopen_count?: number; + empty_step_done_streak?: number; + arbiter_verdict?: string | null; + arbiter_requested_at?: string | null; + completion_reason?: string | null; + }; +}): boolean { + assertPairedTaskStatusTransition({ + currentStatus: args.currentStatus, + nextStatus: args.nextStatus, + }); + + const updated = updatePairedTaskIfUnchanged( + args.taskId, + args.expectedUpdatedAt, + { + ...args.patch, + status: args.nextStatus, + updated_at: args.updatedAt, + }, + ); + if (!updated) { + logger.warn( + { + taskId: args.taskId, + currentStatus: args.currentStatus, + nextStatus: args.nextStatus, + expectedUpdatedAt: args.expectedUpdatedAt, + }, + 'Skipped stale paired task status transition because the task revision changed', + ); + } + return updated; +} + +export function applyPairedTaskPatch(args: { + taskId: string; + expectedUpdatedAt: string; + updatedAt: string; + patch: { + title?: string | null; + source_ref?: string | null; + plan_notes?: string | null; + review_requested_at?: string | null; + round_trip_count?: number; + owner_failure_count?: number; + owner_step_done_streak?: number; + finalize_step_done_count?: number; + task_done_then_user_reopen_count?: number; + empty_step_done_streak?: number; + status?: PairedTaskStatus; + arbiter_verdict?: string | null; + arbiter_requested_at?: string | null; + completion_reason?: string | null; + }; +}): boolean { + const updated = updatePairedTaskIfUnchanged( + args.taskId, + args.expectedUpdatedAt, + { + ...args.patch, + updated_at: args.updatedAt, + }, + ); + if (!updated) { + logger.warn( + { + taskId: args.taskId, + expectedUpdatedAt: args.expectedUpdatedAt, + }, + 'Skipped stale paired task patch because the task revision changed', + ); + } + return updated; +} + +export type TransitionPairedTaskStatusFn = typeof transitionPairedTaskStatus; +export type ApplyPairedTaskPatchFn = typeof applyPairedTaskPatch; diff --git a/src/paired-workspace-manager.ts b/src/paired-workspace-manager.ts index 966ce47..2191d1d 100644 --- a/src/paired-workspace-manager.ts +++ b/src/paired-workspace-manager.ts @@ -12,7 +12,7 @@ import { } from './db.js'; import { resolvePairedTaskWorkspacePath } from './group-folder.js'; import { logger } from './logger.js'; -import { transitionPairedTaskStatus } from './paired-execution-context-shared.js'; +import { transitionPairedTaskStatus } from './paired-task-status.js'; import type { PairedTask, PairedWorkspace } from './types.js'; import { ensureWorkspaceDependenciesInstalled } from './workspace-package-manager.js';