From 65bcbf1a35f41ade57f4573e224867000577fa4b Mon Sep 17 00:00:00 2001 From: Lukas May Date: Fri, 6 Mar 2026 09:41:28 +0100 Subject: [PATCH] fix: Fix review task completion bug + add initiative-level Request Changes Critical: review/merge tasks hit an early return in handleTaskCompleted() that skipped the phase completion check, leaving phases stuck in in_progress forever. Changed to an if-block wrapping only the merge step. Also adds requestChangesOnInitiative() which creates/reuses a "Finalization" phase for initiative-level review feedback, with dedup guards for both phase and initiative request-changes flows. --- apps/server/events/index.ts | 1 + apps/server/events/types.ts | 12 +- apps/server/execution/orchestrator.ts | 112 +++++++++++++++--- apps/server/trpc/routers/initiative.ts | 14 +++ apps/server/trpc/routers/phase.ts | 2 +- .../components/review/InitiativeReview.tsx | 28 ++++- 6 files changed, 152 insertions(+), 17 deletions(-) diff --git a/apps/server/events/index.ts b/apps/server/events/index.ts index e53920b..64a5e0e 100644 --- a/apps/server/events/index.ts +++ b/apps/server/events/index.ts @@ -52,6 +52,7 @@ export type { AccountCredentialsValidatedEvent, InitiativePendingReviewEvent, InitiativeReviewApprovedEvent, + InitiativeChangesRequestedEvent, DomainEventMap, DomainEventType, } from './types.js'; diff --git a/apps/server/events/types.ts b/apps/server/events/types.ts index fb6a8a6..2f4b84c 100644 --- a/apps/server/events/types.ts +++ b/apps/server/events/types.ts @@ -591,6 +591,15 @@ export interface InitiativeReviewApprovedEvent extends DomainEvent { }; } +export interface InitiativeChangesRequestedEvent extends DomainEvent { + type: 'initiative:changes_requested'; + payload: { + initiativeId: string; + phaseId: string; + taskId: string; + }; +} + /** * Chat Session Events */ @@ -668,7 +677,8 @@ export type DomainEventMap = | ChatMessageCreatedEvent | ChatSessionClosedEvent | InitiativePendingReviewEvent - | InitiativeReviewApprovedEvent; + | InitiativeReviewApprovedEvent + | InitiativeChangesRequestedEvent; /** * Event type literal union for type checking diff --git a/apps/server/execution/orchestrator.ts b/apps/server/execution/orchestrator.ts index 55de53a..b48b821 100644 --- a/apps/server/execution/orchestrator.ts +++ b/apps/server/execution/orchestrator.ts @@ -11,7 +11,7 @@ * - Review per-phase: pause after each phase for diff review */ -import type { EventBus, TaskCompletedEvent, PhasePendingReviewEvent, PhaseChangesRequestedEvent, PhaseMergedEvent, TaskMergedEvent, PhaseQueuedEvent, AgentStoppedEvent, InitiativePendingReviewEvent, InitiativeReviewApprovedEvent } from '../events/index.js'; +import type { EventBus, TaskCompletedEvent, PhasePendingReviewEvent, PhaseChangesRequestedEvent, PhaseMergedEvent, TaskMergedEvent, PhaseQueuedEvent, AgentStoppedEvent, InitiativePendingReviewEvent, InitiativeReviewApprovedEvent, InitiativeChangesRequestedEvent } from '../events/index.js'; import type { BranchManager } from '../git/branch-manager.js'; import type { PhaseRepository } from '../db/repositories/phase-repository.js'; import type { TaskRepository } from '../db/repositories/task-repository.js'; @@ -150,20 +150,20 @@ export class ExecutionOrchestrator { const phase = await this.phaseRepository.findById(task.phaseId); if (!phase) return; - // Skip merge/review tasks — they already work on the phase branch directly - if (task.category === 'merge' || task.category === 'review') return; + // Skip merge for review/merge tasks — they already work on the phase branch directly + if (task.category !== 'merge' && task.category !== 'review') { + const initBranch = initiative.branch; + const phBranch = phaseBranchName(initBranch, phase.name); + const tBranch = taskBranchName(initBranch, task.id); - const initBranch = initiative.branch; - const phBranch = phaseBranchName(initBranch, phase.name); - const tBranch = taskBranchName(initBranch, task.id); - - // Serialize merges per phase - const lock = this.phaseMergeLocks.get(task.phaseId) ?? Promise.resolve(); - const mergeOp = lock.then(async () => { - await this.mergeTaskIntoPhase(taskId, task.phaseId!, tBranch, phBranch); - }); - this.phaseMergeLocks.set(task.phaseId, mergeOp.catch(() => {})); - await mergeOp; + // Serialize merges per phase + const lock = this.phaseMergeLocks.get(task.phaseId) ?? Promise.resolve(); + const mergeOp = lock.then(async () => { + await this.mergeTaskIntoPhase(taskId, task.phaseId!, tBranch, phBranch); + }); + this.phaseMergeLocks.set(task.phaseId, mergeOp.catch(() => {})); + await mergeOp; + } // Check if all phase tasks are done const phaseTasks = await this.taskRepository.findByPhaseId(task.phaseId); @@ -356,6 +356,15 @@ export class ExecutionOrchestrator { const initiative = await this.initiativeRepository.findById(phase.initiativeId); if (!initiative) throw new Error(`Initiative not found: ${phase.initiativeId}`); + // Guard: don't create duplicate review tasks + const existingTasks = await this.taskRepository.findByPhaseId(phaseId); + const activeReview = existingTasks.find( + (t) => t.category === 'review' && (t.status === 'pending' || t.status === 'in_progress'), + ); + if (activeReview) { + return { taskId: activeReview.id }; + } + // Build revision task description from comments + summary const lines: string[] = []; if (summary) { @@ -418,6 +427,81 @@ export class ExecutionOrchestrator { return { taskId: task.id }; } + /** + * Request changes on an initiative that's pending review. + * Creates/reuses a "Finalization" phase and adds a review task to it. + */ + async requestChangesOnInitiative( + initiativeId: string, + summary: string, + ): Promise<{ taskId: string }> { + const initiative = await this.initiativeRepository.findById(initiativeId); + if (!initiative) throw new Error(`Initiative not found: ${initiativeId}`); + if (initiative.status !== 'pending_review') { + throw new Error(`Initiative ${initiativeId} is not pending review (status: ${initiative.status})`); + } + + // Find or create a "Finalization" phase + const phases = await this.phaseRepository.findByInitiativeId(initiativeId); + let finalizationPhase = phases.find((p) => p.name === 'Finalization'); + + if (!finalizationPhase) { + finalizationPhase = await this.phaseRepository.create({ + initiativeId, + name: 'Finalization', + status: 'in_progress', + }); + } else if (finalizationPhase.status === 'completed' || finalizationPhase.status === 'pending_review') { + await this.phaseRepository.update(finalizationPhase.id, { status: 'in_progress' as any }); + } + + // Guard: don't create duplicate review tasks + const existingTasks = await this.taskRepository.findByPhaseId(finalizationPhase.id); + const activeReview = existingTasks.find( + (t) => t.category === 'review' && (t.status === 'pending' || t.status === 'in_progress'), + ); + if (activeReview) { + // Still reset initiative to active + await this.initiativeRepository.update(initiativeId, { status: 'active' as any }); + this.scheduleDispatch(); + return { taskId: activeReview.id }; + } + + // Create review task + const task = await this.taskRepository.create({ + phaseId: finalizationPhase.id, + initiativeId, + name: `Address initiative review feedback`, + description: `## Summary\n\n${summary}`, + category: 'review', + priority: 'high', + }); + + // Reset initiative status to active + await this.initiativeRepository.update(initiativeId, { status: 'active' as any }); + + // Queue task for dispatch + await this.dispatchManager.queue(task.id); + + // Emit event + const event: InitiativeChangesRequestedEvent = { + type: 'initiative:changes_requested', + timestamp: new Date(), + payload: { + initiativeId, + phaseId: finalizationPhase.id, + taskId: task.id, + }, + }; + this.eventBus.emit(event); + + log.info({ initiativeId, phaseId: finalizationPhase.id, taskId: task.id }, 'changes requested on initiative'); + + this.scheduleDispatch(); + + return { taskId: task.id }; + } + /** * Re-queue approved phases for an initiative into the in-memory dispatch queue. * Self-healing: ensures phases aren't lost if the server restarted since the diff --git a/apps/server/trpc/routers/initiative.ts b/apps/server/trpc/routers/initiative.ts index 4b8fb66..37b77b3 100644 --- a/apps/server/trpc/routers/initiative.ts +++ b/apps/server/trpc/routers/initiative.ts @@ -335,5 +335,19 @@ export function initiativeProcedures(publicProcedure: ProcedureBuilder) { await orchestrator.approveInitiative(input.initiativeId, input.strategy); return { success: true }; }), + + requestInitiativeChanges: publicProcedure + .input(z.object({ + initiativeId: z.string().min(1), + summary: z.string().trim().min(1), + })) + .mutation(async ({ ctx, input }) => { + const orchestrator = requireExecutionOrchestrator(ctx); + const result = await orchestrator.requestChangesOnInitiative( + input.initiativeId, + input.summary, + ); + return { success: true, taskId: result.taskId }; + }), }; } diff --git a/apps/server/trpc/routers/phase.ts b/apps/server/trpc/routers/phase.ts index 4762232..d2ec9e8 100644 --- a/apps/server/trpc/routers/phase.ts +++ b/apps/server/trpc/routers/phase.ts @@ -371,7 +371,7 @@ export function phaseProcedures(publicProcedure: ProcedureBuilder) { requestPhaseChanges: publicProcedure .input(z.object({ phaseId: z.string().min(1), - summary: z.string().optional(), + summary: z.string().trim().min(1).optional(), })) .mutation(async ({ ctx, input }) => { const orchestrator = requireExecutionOrchestrator(ctx); diff --git a/apps/web/src/components/review/InitiativeReview.tsx b/apps/web/src/components/review/InitiativeReview.tsx index fc0cd7a..5f7a36b 100644 --- a/apps/web/src/components/review/InitiativeReview.tsx +++ b/apps/web/src/components/review/InitiativeReview.tsx @@ -1,6 +1,6 @@ import { useCallback, useMemo, useRef, useState } from "react"; import { toast } from "sonner"; -import { Loader2, GitBranch, ArrowRight, FileCode, Plus, Minus, Upload, GitMerge } from "lucide-react"; +import { Loader2, GitBranch, ArrowRight, FileCode, Plus, Minus, Upload, GitMerge, RotateCcw } from "lucide-react"; import { Button } from "@/components/ui/button"; import { trpc } from "@/lib/trpc"; import { parseUnifiedDiff } from "./parse-diff"; @@ -82,6 +82,14 @@ export function InitiativeReview({ initiativeId, onCompleted }: InitiativeReview onError: (err) => toast.error(`Failed to stop: ${err.message}`), }); + const requestChangesMutation = trpc.requestInitiativeChanges.useMutation({ + onSuccess: () => { + toast.success("Changes requested — review task created"); + onCompleted(); + }, + onError: (err) => toast.error(err.message), + }); + const approveMutation = trpc.approveInitiativeReview.useMutation({ onSuccess: (_data, variables) => { const msg = variables.strategy === "merge_and_push" @@ -189,6 +197,24 @@ export function InitiativeReview({ initiativeId, onCompleted }: InitiativeReview {/* Right: preview + action buttons */}
{previewState && } +