diff --git a/agent-teams-controller/src/internal/review.js b/agent-teams-controller/src/internal/review.js index 19fd468b..bf4dcade 100644 --- a/agent-teams-controller/src/internal/review.js +++ b/agent-teams-controller/src/internal/review.js @@ -36,7 +36,7 @@ function getCurrentReviewState(task) { const events = Array.isArray(task.historyEvents) ? task.historyEvents : []; for (let i = events.length - 1; i >= 0; i--) { const e = events[i]; - if (e.type === 'review_requested' || e.type === 'review_changes_requested' || e.type === 'review_approved') { + if (e.type === 'review_requested' || e.type === 'review_changes_requested' || e.type === 'review_approved' || e.type === 'review_started') { return e.to; } if (e.type === 'status_changed' && e.to === 'in_progress') { @@ -46,6 +46,44 @@ function getCurrentReviewState(task) { return 'none'; } +function startReview(context, taskId, flags = {}) { + const task = tasks.getTask(context, taskId); + if (task.status === 'deleted') { + throw new Error(`Task #${task.displayId || task.id} is deleted`); + } + + const from = + typeof flags.from === 'string' && flags.from.trim() ? flags.from.trim() : 'reviewer'; + const prevReviewState = getCurrentReviewState(task); + + // Idempotent: already in review → return ok without duplicate history event + if (prevReviewState === 'review') { + return { ok: true, taskId: task.id, displayId: task.displayId, column: 'review' }; + } + + try { + kanban.setKanbanColumn(context, task.id, 'review'); + tasks.updateTask(context, task.id, (t) => { + t.historyEvents = tasks.appendHistoryEvent(t.historyEvents, { + type: 'review_started', + from: prevReviewState, + to: 'review', + actor: from, + }); + t.reviewState = 'review'; + return t; + }); + return { ok: true, taskId: task.id, displayId: task.displayId, column: 'review' }; + } catch (error) { + try { + kanban.clearKanban(context, task.id); + } catch { + // Best-effort rollback + } + throw error; + } +} + function requestReview(context, taskId, flags = {}) { const task = tasks.getTask(context, taskId); if (task.status !== 'completed') { @@ -84,7 +122,9 @@ function requestReview(context, taskId, flags = {}) { text: `**Please review** task #${task.displayId || task.id}\n\n` + wrapAgentBlock( - `When approved, use MCP tool review_approve:\n` + + `FIRST call review_start to signal you are beginning the review:\n` + + `{ teamName: "${context.teamName}", taskId: "${task.id}", from: "" }\n\n` + + `When approved, use MCP tool review_approve:\n` + `{ teamName: "${context.teamName}", taskId: "${task.id}", notifyOwner: true }\n\n` + `If changes are needed, use MCP tool review_request_changes:\n` + `{ teamName: "${context.teamName}", taskId: "${task.id}", comment: "..." }` @@ -210,4 +250,5 @@ module.exports = { approveReview, requestReview, requestChanges, + startReview, }; diff --git a/agent-teams-controller/src/internal/taskStore.js b/agent-teams-controller/src/internal/taskStore.js index af44be5d..f5be7d0e 100644 --- a/agent-teams-controller/src/internal/taskStore.js +++ b/agent-teams-controller/src/internal/taskStore.js @@ -660,7 +660,7 @@ function getEffectiveReviewState(kanbanEntry, task) { const events = Array.isArray(task.historyEvents) ? task.historyEvents : []; for (let i = events.length - 1; i >= 0; i--) { const e = events[i]; - if (e.type === 'review_requested' || e.type === 'review_changes_requested' || e.type === 'review_approved') { + if (e.type === 'review_requested' || e.type === 'review_changes_requested' || e.type === 'review_approved' || e.type === 'review_started') { return e.to; } if (e.type === 'status_changed' && e.to === 'in_progress') { diff --git a/agent-teams-controller/src/internal/tasks.js b/agent-teams-controller/src/internal/tasks.js index e2809bb2..1a211c0a 100644 --- a/agent-teams-controller/src/internal/tasks.js +++ b/agent-teams-controller/src/internal/tasks.js @@ -384,6 +384,13 @@ function buildMemberTaskProtocol(teamName) { - After that follow-up work finishes, add a short task comment with the result, what changed, or what you verified. - After that, run task_complete again before your reply. - Never do comment-driven implementation/fix work while the task is still shown as pending, review, completed, or approved. + - After task_complete, if the task needs review AND the team has a member whose role includes reviewing (e.g. "reviewer", "tech-lead", "qa"), IMMEDIATELY call review_request to move it to the review column and notify the reviewer: + { teamName: "${teamName}", taskId: "", from: "", reviewer: "" } + Do NOT leave a completed task without sending it to review when review is expected and a reviewer exists. + If no team member has a reviewer role, skip review_request — the task stays completed. +3b. When you BEGIN reviewing a task, FIRST call review_start to ensure it appears in the REVIEW column: + { teamName: "${teamName}", taskId: "", from: "" } + This is MANDATORY before review_approve or review_request_changes. Without this step, the kanban board may not show the task in REVIEW during your review. 4. If you are asked to review and the task is accepted, move it to APPROVED (not DONE) with MCP tool review_approve: { teamName: "${teamName}", taskId: "", note?: "", notifyOwner: true } 5. If review fails and changes are needed, use MCP tool review_request_changes: @@ -402,8 +409,10 @@ function buildMemberTaskProtocol(teamName) { - If you are reviewing work for task #X, run review_approve/review_request_changes on #X (the work task). - Do NOT approve a separate "review task" (e.g. #2 created just to ask for a review) — that will put the wrong task into APPROVED. - Typical flow: - a) Owner finishes work on #X -> task_complete #X - b) Reviewer accepts -> review_approve #X + a) Owner finishes work on #X -> task_complete #X -> review_request #X (moves to review column, notifies reviewer) + b) Reviewer begins reviewing -> review_start #X (ensures task is in REVIEW column on kanban) + c) Reviewer accepts -> review_approve #X + d) Reviewer rejects -> review_request_changes #X (moves back to pending with needsFix) 12. CLARIFICATION PROTOCOL (CRITICAL — MANDATORY): When you are blocked and need information to continue a task, you MUST do ALL steps below — skipping the board update or comment breaks traceability: a) STEP 1 — FIRST, set the clarification flag with MCP tool task_set_clarification: diff --git a/agent-teams-controller/test/controller.test.js b/agent-teams-controller/test/controller.test.js index 9f9fd4f6..9e828cf8 100644 --- a/agent-teams-controller/test/controller.test.js +++ b/agent-teams-controller/test/controller.test.js @@ -530,6 +530,50 @@ describe('agent-teams-controller API', () => { expect(inbox[0].leadSessionId).toBe('lead-session-1'); }); + it('starts review idempotently without requiring completed status', () => { + const claudeDir = makeClaudeDir(); + const controller = createController({ teamName: 'my-team', claudeDir }); + const task = controller.tasks.createTask({ subject: 'Review me', owner: 'bob' }); + + // startReview does not require completed status + const result = controller.review.startReview(task.id, { from: 'alice' }); + expect(result.ok).toBe(true); + expect(result.taskId).toBe(task.id); + expect(result.displayId).toBe(task.displayId); + expect(result.column).toBe('review'); + + // Verify kanban state + const kanbanState = controller.kanban.getKanbanState(); + expect(kanbanState.tasks[task.id].column).toBe('review'); + + // Verify task reviewState + const updatedTask = controller.tasks.getTask(task.id); + expect(updatedTask.reviewState).toBe('review'); + + // Verify history event + const reviewEvent = updatedTask.historyEvents.find((e) => e.type === 'review_started'); + expect(reviewEvent).toBeDefined(); + expect(reviewEvent.from).toBe('none'); + expect(reviewEvent.to).toBe('review'); + expect(reviewEvent.actor).toBe('alice'); + + // Idempotent: calling again should also succeed without duplicate events + const again = controller.review.startReview(task.id, { from: 'alice' }); + expect(again.ok).toBe(true); + const reloaded = controller.tasks.getTask(task.id); + const startedEvents = reloaded.historyEvents.filter((e) => e.type === 'review_started'); + expect(startedEvents).toHaveLength(1); + }); + + it('throws when starting review on a deleted task', () => { + const claudeDir = makeClaudeDir(); + const controller = createController({ teamName: 'my-team', claudeDir }); + const task = controller.tasks.createTask({ subject: 'Deleted task', owner: 'bob' }); + controller.tasks.softDeleteTask(task.id, 'bob'); + + expect(() => controller.review.startReview(task.id, { from: 'alice' })).toThrow('is deleted'); + }); + it('persists full inbox metadata through controller messages.sendMessage', () => { const claudeDir = makeClaudeDir(); const controller = createController({ teamName: 'my-team', claudeDir }); diff --git a/mcp-server/src/agent-teams-controller.d.ts b/mcp-server/src/agent-teams-controller.d.ts index f87f31c0..4b0a6a82 100644 --- a/mcp-server/src/agent-teams-controller.d.ts +++ b/mcp-server/src/agent-teams-controller.d.ts @@ -43,6 +43,7 @@ declare module 'agent-teams-controller' { requestReview(taskId: string, flags?: Record): unknown; approveReview(taskId: string, flags?: Record): unknown; requestChanges(taskId: string, flags?: Record): unknown; + startReview(taskId: string, flags?: Record): unknown; } export interface ControllerMessageApi { diff --git a/mcp-server/src/tools/reviewTools.ts b/mcp-server/src/tools/reviewTools.ts index bf8b6e43..60a8ef8b 100644 --- a/mcp-server/src/tools/reviewTools.ts +++ b/mcp-server/src/tools/reviewTools.ts @@ -34,6 +34,24 @@ export function registerReviewTools(server: Pick) { ), }); + server.addTool({ + name: 'review_start', + description: 'Signal that reviewer is beginning to review a task (moves to REVIEW column)', + parameters: z.object({ + ...toolContextSchema, + taskId: z.string().min(1), + from: z.string().optional(), + }), + execute: async ({ teamName, claudeDir, taskId, from }) => + await Promise.resolve( + jsonTextContent( + getController(teamName, claudeDir).review.startReview(taskId, { + ...(from ? { from } : {}), + }) as Record + ) + ), + }); + server.addTool({ name: 'review_approve', description: 'Approve task review and move kanban state accordingly', diff --git a/mcp-server/test/tools.test.ts b/mcp-server/test/tools.test.ts index 60536214..b16b5892 100644 --- a/mcp-server/test/tools.test.ts +++ b/mcp-server/test/tools.test.ts @@ -49,6 +49,7 @@ describe('agent-teams-mcp tools', () => { 'review_approve', 'review_request', 'review_request_changes', + 'review_start', 'task_add_comment', 'task_attach_comment_file', 'task_attach_file', @@ -774,6 +775,27 @@ describe('agent-teams-mcp tools', () => { ); expect(kanbanCleared.tasks[createdTask.id]).toBeUndefined(); + // review_start: moves task to review without requiring completed status + const pendingTask = parseJsonToolResult( + await getTool('task_create').execute({ + claudeDir, + teamName, + subject: 'Start review test', + owner: 'bob', + }) + ); + const reviewStarted = parseJsonToolResult( + await getTool('review_start').execute({ + claudeDir, + teamName, + taskId: pendingTask.id, + from: 'alice', + }) + ); + expect(reviewStarted.ok).toBe(true); + expect(reviewStarted.column).toBe('review'); + expect(reviewStarted.taskId).toBe(pendingTask.id); + const pid = process.pid; const registered = parseJsonToolResult( diff --git a/src/main/services/team/TeamDataService.ts b/src/main/services/team/TeamDataService.ts index 2a1304f9..880ac553 100644 --- a/src/main/services/team/TeamDataService.ts +++ b/src/main/services/team/TeamDataService.ts @@ -155,6 +155,9 @@ export class TeamDataService { if (event.type === 'review_approved' && event.actor) { return event.actor; } + if (event.type === 'review_started' && event.actor) { + return event.actor; + } if (event.type === 'review_requested' && event.reviewer) { return event.reviewer; } diff --git a/src/main/services/team/TeamProvisioningService.ts b/src/main/services/team/TeamProvisioningService.ts index 9129bdb0..e5ac9aed 100644 --- a/src/main/services/team/TeamProvisioningService.ts +++ b/src/main/services/team/TeamProvisioningService.ts @@ -563,7 +563,7 @@ function buildTeamCtlOpsInstructions(teamName: string, leadName: string): string `- Use blockedBy when a task cannot start until another is done.`, `- If you set blockedBy, create the task in pending (for example with startImmediately: false). Do NOT put blocked tasks into in_progress.`, `- Use related to link related work (e.g. frontend + backend) without blocking.`, - `- Review tasks: Prefer NOT creating a separate "review task". Reviews apply to the work task (#X) via review_approve/review_request_changes on #X.`, + `- Review tasks: Prefer NOT creating a separate "review task". Reviews apply to the work task (#X) via review_start/review_approve/review_request_changes on #X.`, ` - If you must create a separate review reminder/assignment task, keep it pending and link it to #X with related (and optionally blockedBy #X if it truly cannot start yet).`, ` - Dependencies do not auto-start tasks; the owner must explicitly start it when ready.`, `- Avoid over-specifying. Only add dependencies when execution order matters.`, @@ -571,6 +571,7 @@ function buildTeamCtlOpsInstructions(teamName: string, leadName: string): string `Notification policy:`, `- Task assignment notifications are handled by the board runtime, so do NOT send a separate SendMessage for the same assignment unless you have extra context that is not already on the task.`, `- Review requests are also handled by the board runtime: review_request already notifies the reviewer, so do NOT send a second manual SendMessage for the same review request unless you are adding materially new context that is not already on the task.`, + `- When beginning a review, always call review_start first to move the task into the REVIEW column on the kanban board.`, `- If you receive a task-scoped system notification like "Comment on #...", treat it as requiring an on-task reply. Reply via task_add_comment on that task; do NOT continue the same discussion only in direct messages.`, `- Teammate task comments are auto-forwarded to you. When that happens, you MUST reply on-task first. Direct messages are allowed only as an additional urgent wake-up ping or clearly non-task coordination, never as the only reply to the task comment.`, `- When you skip sending a message because it would be a duplicate or was already delivered, produce NO text output about it. Do not write meta-commentary like "(Already relayed…)", "(No additional relay needed…)", or similar. Just silently move on.`, @@ -826,7 +827,7 @@ function buildProvisioningPrompt(request: TeamCreateRequest): string { - When tasks have natural ordering (e.g. setup -> implementation -> testing), use blockedBy relationships. - If a task is blocked (uses blockedBy), it MUST be created as pending (for example with task_create + startImmediately: false). Do NOT mark blocked tasks in_progress. - Review guidance: - - Prefer NOT creating a separate "review task". Our workflow reviews the work task itself: run review_approve/review_request_changes on the implementation task #X. + - Prefer NOT creating a separate "review task". Our workflow reviews the work task itself: call review_start when beginning review, then review_approve/review_request_changes on the implementation task #X. - If you MUST create a separate review reminder/assignment task, create it as pending and link it to the work task: - Use related to connect it to #X (non-blocking link). - If the review truly cannot start until #X is done, ALSO add blockedBy #X. diff --git a/src/main/workers/team-fs-worker.ts b/src/main/workers/team-fs-worker.ts index 3dbdd0ac..d4f8b7ab 100644 --- a/src/main/workers/team-fs-worker.ts +++ b/src/main/workers/team-fs-worker.ts @@ -535,7 +535,12 @@ function deriveReviewStateFromEvents(events: RawHistoryEvent[] | undefined): str for (let i = events.length - 1; i >= 0; i--) { const e = events[i]; const t = e.type; - if (t === 'review_requested' || t === 'review_changes_requested' || t === 'review_approved') { + if ( + t === 'review_requested' || + t === 'review_changes_requested' || + t === 'review_approved' || + t === 'review_started' + ) { const to = typeof e.to === 'string' ? e.to : 'none'; return to === 'review' || to === 'needsFix' || to === 'approved' ? to : 'none'; } diff --git a/src/renderer/components/team/dialogs/StatusHistoryTimeline.tsx b/src/renderer/components/team/dialogs/StatusHistoryTimeline.tsx index f56b486a..50223281 100644 --- a/src/renderer/components/team/dialogs/StatusHistoryTimeline.tsx +++ b/src/renderer/components/team/dialogs/StatusHistoryTimeline.tsx @@ -122,6 +122,13 @@ const EventContent = ({ ) : null} ); + case 'review_started': + return ( + + + Review started + + ); case 'review_changes_requested': return ( @@ -176,6 +183,8 @@ function dotColor(event: TaskHistoryEvent): string { return dotColorForStatus(event.to); case 'review_requested': return 'bg-purple-400'; + case 'review_started': + return 'bg-purple-400'; case 'review_changes_requested': return 'bg-amber-400'; case 'review_approved': diff --git a/src/renderer/components/team/dialogs/TaskDetailDialog.tsx b/src/renderer/components/team/dialogs/TaskDetailDialog.tsx index a86c5e50..a22cbd91 100644 --- a/src/renderer/components/team/dialogs/TaskDetailDialog.tsx +++ b/src/renderer/components/team/dialogs/TaskDetailDialog.tsx @@ -565,10 +565,12 @@ export const TaskDetailDialog = ({ borderRight: `1px solid ${getThemedBorder(colors, isLight)}40`, borderBottom: `1px solid ${getThemedBorder(colors, isLight)}40`, }; - const reviewEventType = - currentTask.reviewState === 'approved' ? 'review_approved' : 'review_requested'; const lastReviewEvent = currentTask.historyEvents - ?.filter((e) => e.type === reviewEventType) + ?.filter((e) => + currentTask.reviewState === 'approved' + ? e.type === 'review_approved' + : e.type === 'review_requested' || e.type === 'review_started' + ) .at(-1); const reviewDate = lastReviewEvent ? new Date(lastReviewEvent.timestamp) diff --git a/src/shared/types/team.ts b/src/shared/types/team.ts index a59c2251..5a3090ad 100644 --- a/src/shared/types/team.ts +++ b/src/shared/types/team.ts @@ -108,12 +108,19 @@ export interface TaskReviewApprovedEvent extends TaskHistoryEventBase { note?: string; } +export interface TaskReviewStartedEvent extends TaskHistoryEventBase { + type: 'review_started'; + from: TeamReviewState; + to: 'review'; +} + export type TaskHistoryEvent = | TaskCreatedEvent | TaskStatusChangedEvent | TaskReviewRequestedEvent | TaskReviewChangesRequestedEvent - | TaskReviewApprovedEvent; + | TaskReviewApprovedEvent + | TaskReviewStartedEvent; export type TaskCommentType = 'regular' | 'review_request' | 'review_approved'; @@ -713,7 +720,12 @@ export interface TeamMessageNotificationData { /** Optional sender color for visual context. */ color?: string; /** Team event sub-type for notification categorization. */ - teamEventType?: 'task_clarification' | 'task_status_change' | 'task_comment' | 'task_created' | 'all_tasks_completed'; + teamEventType?: + | 'task_clarification' + | 'task_status_change' + | 'task_comment' + | 'task_created' + | 'all_tasks_completed'; /** Stable key for storage deduplication. Required — no fallback to Date.now(). */ dedupeKey?: string; /** diff --git a/src/shared/utils/taskHistory.ts b/src/shared/utils/taskHistory.ts index a4bc4fa7..18e71ace 100644 --- a/src/shared/utils/taskHistory.ts +++ b/src/shared/utils/taskHistory.ts @@ -26,7 +26,8 @@ export function getDerivedReviewState(task: Pick): Te if ( event.type === 'review_requested' || event.type === 'review_changes_requested' || - event.type === 'review_approved' + event.type === 'review_approved' || + event.type === 'review_started' ) { return event.to; } diff --git a/test/shared/utils/reviewState.test.ts b/test/shared/utils/reviewState.test.ts index 783a4637..4261f977 100644 --- a/test/shared/utils/reviewState.test.ts +++ b/test/shared/utils/reviewState.test.ts @@ -17,4 +17,21 @@ describe('reviewState utils', () => { it('does not map needsFix to a kanban column', () => { expect(getKanbanColumnFromReviewState('needsFix')).toBeUndefined(); }); + + it('derives review state from review_started history event', () => { + expect( + getReviewStateFromTask({ + historyEvents: [ + { + id: '1', + timestamp: '2026-01-01T00:00:00Z', + type: 'review_started', + from: 'none', + to: 'review', + actor: 'alice', + }, + ], + }) + ).toBe('review'); + }); });