feat: implement startReview functionality and enhance review state management
- Added startReview function to initiate the review process for tasks, allowing transition to the review column without requiring completed status. - Updated getCurrentReviewState and getEffectiveReviewState functions to include 'review_started' event type for accurate review state tracking. - Enhanced task history management by introducing a new TaskReviewStartedEvent type. - Updated documentation and user instructions to reflect the new review process, emphasizing the importance of calling review_start before review actions. - Improved test coverage for the new startReview functionality and its integration with existing review processes.
This commit is contained in:
parent
0f91698fa8
commit
5686d60d99
15 changed files with 199 additions and 14 deletions
|
|
@ -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: "<your-name>" }\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,
|
||||
};
|
||||
|
|
|
|||
|
|
@ -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') {
|
||||
|
|
|
|||
|
|
@ -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: "<taskId>", from: "<your-name>", reviewer: "<reviewer-name>" }
|
||||
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: "<taskId>", from: "<your-name>" }
|
||||
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: "<taskId>", note?: "<optional 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:
|
||||
|
|
|
|||
|
|
@ -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 });
|
||||
|
|
|
|||
1
mcp-server/src/agent-teams-controller.d.ts
vendored
1
mcp-server/src/agent-teams-controller.d.ts
vendored
|
|
@ -43,6 +43,7 @@ declare module 'agent-teams-controller' {
|
|||
requestReview(taskId: string, flags?: Record<string, unknown>): unknown;
|
||||
approveReview(taskId: string, flags?: Record<string, unknown>): unknown;
|
||||
requestChanges(taskId: string, flags?: Record<string, unknown>): unknown;
|
||||
startReview(taskId: string, flags?: Record<string, unknown>): unknown;
|
||||
}
|
||||
|
||||
export interface ControllerMessageApi {
|
||||
|
|
|
|||
|
|
@ -34,6 +34,24 @@ export function registerReviewTools(server: Pick<FastMCP, 'addTool'>) {
|
|||
),
|
||||
});
|
||||
|
||||
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<string, unknown>
|
||||
)
|
||||
),
|
||||
});
|
||||
|
||||
server.addTool({
|
||||
name: 'review_approve',
|
||||
description: 'Approve task review and move kanban state accordingly',
|
||||
|
|
|
|||
|
|
@ -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(
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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.
|
||||
|
|
|
|||
|
|
@ -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';
|
||||
}
|
||||
|
|
|
|||
|
|
@ -122,6 +122,13 @@ const EventContent = ({
|
|||
) : null}
|
||||
</span>
|
||||
);
|
||||
case 'review_started':
|
||||
return (
|
||||
<span className="flex items-center gap-1">
|
||||
<Eye size={10} className="text-purple-400" />
|
||||
Review started
|
||||
</span>
|
||||
);
|
||||
case 'review_changes_requested':
|
||||
return (
|
||||
<span className="flex items-center gap-1">
|
||||
|
|
@ -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':
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
/**
|
||||
|
|
|
|||
|
|
@ -26,7 +26,8 @@ export function getDerivedReviewState(task: Pick<TeamTask, 'historyEvents'>): 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;
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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');
|
||||
});
|
||||
});
|
||||
|
|
|
|||
Loading…
Reference in a new issue