diff --git a/packages/shared/src/types/issue.ts b/packages/shared/src/types/issue.ts index 343d93f577..1fdab004f7 100644 --- a/packages/shared/src/types/issue.ts +++ b/packages/shared/src/types/issue.ts @@ -631,6 +631,12 @@ export interface IssueExecutionPolicy { monitor?: IssueExecutionMonitorPolicy | null; reviewPreset?: LowTrustReviewPresetPolicy; authorizationPolicy?: TrustAuthorizationPolicy; + /** + * Maximum consecutive agent-initiated changes-requested rounds before the + * pending stage escalates to the responsible human. Null uses the server + * default. Human decisions reset the round counter. + */ + maxReviewRounds?: number | null; } export interface IssueExecutionMonitorState { @@ -666,6 +672,8 @@ export interface IssueExecutionState { lastDecisionId: string | null; lastDecisionOutcome: IssueExecutionDecisionOutcome | null; monitor?: IssueExecutionMonitorState | null; + /** Consecutive agent-initiated changes-requested rounds on the current stage. */ + changesRequestedCount?: number; } export interface IssueExecutionDecision { diff --git a/packages/shared/src/validators/issue.ts b/packages/shared/src/validators/issue.ts index e7a2746ca1..0510d45ea9 100644 --- a/packages/shared/src/validators/issue.ts +++ b/packages/shared/src/validators/issue.ts @@ -249,6 +249,7 @@ export const issueExecutionPolicySchema = z.object({ monitor: issueExecutionMonitorPolicySchema.optional().nullable(), reviewPreset: lowTrustReviewPresetPolicySchema.optional(), authorizationPolicy: trustAuthorizationPolicySchema.optional(), + maxReviewRounds: z.number().int().positive().max(50).optional().nullable().default(null), }); export const issueExecutionMonitorStateSchema = z.object({ @@ -284,6 +285,7 @@ export const issueExecutionStateSchema = z.object({ lastDecisionId: z.string().uuid().nullable(), lastDecisionOutcome: z.enum(ISSUE_EXECUTION_DECISION_OUTCOMES).nullable(), monitor: issueExecutionMonitorStateSchema.optional().nullable(), + changesRequestedCount: z.number().int().nonnegative().optional().default(0), }); export const issueRecoveryActionReadModelSchema = z.object({ diff --git a/server/src/__tests__/issue-execution-policy.test.ts b/server/src/__tests__/issue-execution-policy.test.ts index d0e348b036..e066f30b3e 100644 --- a/server/src/__tests__/issue-execution-policy.test.ts +++ b/server/src/__tests__/issue-execution-policy.test.ts @@ -1619,3 +1619,294 @@ describe("issue execution policy transitions", () => { }); }); }); + +describe("review round circuit breaker", () => { + const policy = reviewOnlyPolicy(); + const reviewStageId = policy.stages[0].id; + + function reviewPendingIssue(overrides: Record = {}, stateOverrides: Record = {}) { + return { + status: "in_review", + assigneeAgentId: qaAgentId, + assigneeUserId: null, + responsibleUserId: boardUserId, + executionPolicy: policy, + executionState: { + status: "pending", + currentStageId: reviewStageId, + currentStageIndex: 0, + currentStageType: "review", + currentParticipant: { type: "agent", agentId: qaAgentId }, + returnAssignee: { type: "agent", agentId: coderAgentId }, + completedStageIds: [], + lastDecisionId: null, + lastDecisionOutcome: null, + ...stateOverrides, + }, + ...overrides, + }; + } + + it("counts agent-initiated changes-requested rounds on the hand-back", () => { + const result = applyIssueExecutionPolicyTransition({ + issue: reviewPendingIssue(), + policy, + requestedStatus: "in_progress", + requestedAssigneePatch: {}, + actor: { agentId: qaAgentId }, + commentBody: "Round one feedback", + }); + + expect(result.patch.status).toBe("in_progress"); + expect(result.patch.assigneeAgentId).toBe(coderAgentId); + expect(result.patch.executionState).toMatchObject({ + status: "changes_requested", + changesRequestedCount: 1, + }); + }); + + it("carries the round count through the executor's resubmission", () => { + const result = applyIssueExecutionPolicyTransition({ + issue: { + status: "in_progress", + assigneeAgentId: coderAgentId, + assigneeUserId: null, + responsibleUserId: boardUserId, + executionPolicy: policy, + executionState: { + status: "changes_requested", + currentStageId: reviewStageId, + currentStageIndex: 0, + currentStageType: "review", + currentParticipant: { type: "agent", agentId: qaAgentId }, + returnAssignee: { type: "agent", agentId: coderAgentId }, + completedStageIds: [], + lastDecisionId: null, + lastDecisionOutcome: "changes_requested", + changesRequestedCount: 2, + }, + }, + policy, + requestedStatus: "done", + requestedAssigneePatch: {}, + actor: { agentId: coderAgentId }, + commentBody: "Addressed round two", + }); + + expect(result.patch.status).toBe("in_review"); + expect(result.patch.executionState).toMatchObject({ + status: "pending", + changesRequestedCount: 2, + }); + }); + + it("escalates the pending stage to the responsible human at the round cap", () => { + const result = applyIssueExecutionPolicyTransition({ + issue: reviewPendingIssue({}, { changesRequestedCount: 2 }), + policy, + requestedStatus: "in_progress", + requestedAssigneePatch: {}, + actor: { agentId: qaAgentId }, + commentBody: "Round three feedback — still not converging", + }); + + // The decision is still recorded, but the stage stays pending with the + // responsible human as participant instead of bouncing to the executor. + expect(result.decision).toMatchObject({ outcome: "changes_requested" }); + expect(result.patch.status).toBe("in_review"); + expect(result.patch.assigneeAgentId).toBeNull(); + expect(result.patch.assigneeUserId).toBe(boardUserId); + expect(result.patch.executionState).toMatchObject({ + status: "pending", + currentStageId: reviewStageId, + currentParticipant: { type: "user", userId: boardUserId }, + changesRequestedCount: 3, + }); + }); + + it("keeps the escalated hold sticky across unrelated transitions", () => { + const result = applyIssueExecutionPolicyTransition({ + issue: reviewPendingIssue( + { assigneeAgentId: null, assigneeUserId: boardUserId }, + { + currentParticipant: { type: "user", userId: boardUserId }, + changesRequestedCount: 3, + }, + ), + policy, + requestedAssigneePatch: {}, + actor: { agentId: coderAgentId }, + }); + + expect(result.patch.executionState).toBeUndefined(); + expect(result.patch.assigneeAgentId).toBeUndefined(); + }); + + it("rejects a non-escalated actor advancing the stage during the hold", () => { + expect(() => + applyIssueExecutionPolicyTransition({ + issue: reviewPendingIssue( + { assigneeAgentId: null, assigneeUserId: boardUserId }, + { + currentParticipant: { type: "user", userId: boardUserId }, + changesRequestedCount: 3, + }, + ), + policy, + requestedStatus: "done", + requestedAssigneePatch: {}, + actor: { agentId: qaAgentId }, + commentBody: "Agent trying to close it anyway", + }), + ).toThrow("Only the escalated reviewer can advance the current execution stage"); + }); + + it("rejects a non-escalated actor reassigning the issue during the hold", () => { + expect(() => + applyIssueExecutionPolicyTransition({ + issue: reviewPendingIssue( + { assigneeAgentId: null, assigneeUserId: boardUserId }, + { + currentParticipant: { type: "user", userId: boardUserId }, + changesRequestedCount: 3, + }, + ), + policy, + requestedAssigneePatch: { assigneeAgentId: coderAgentId }, + actor: { agentId: coderAgentId }, + }), + ).toThrow("Only the escalated reviewer can advance the current execution stage"); + }); + + it("re-asserts the hold when the assignee has drifted away from the escalated human", () => { + const result = applyIssueExecutionPolicyTransition({ + issue: reviewPendingIssue( + // Assignee drifted back to the agent reviewer while the state still + // records the escalated human as participant. + { assigneeAgentId: qaAgentId, assigneeUserId: null }, + { + currentParticipant: { type: "user", userId: boardUserId }, + changesRequestedCount: 3, + }, + ), + policy, + requestedAssigneePatch: {}, + actor: { agentId: coderAgentId }, + }); + + expect(result.patch.status).toBe("in_review"); + expect(result.patch.assigneeAgentId).toBeNull(); + expect(result.patch.assigneeUserId).toBe(boardUserId); + expect(result.patch.executionState).toMatchObject({ + status: "pending", + currentParticipant: { type: "user", userId: boardUserId }, + changesRequestedCount: 3, + }); + }); + + it("resets the counter when the escalated human requests changes", () => { + const result = applyIssueExecutionPolicyTransition({ + issue: reviewPendingIssue( + { assigneeAgentId: null, assigneeUserId: boardUserId }, + { + currentParticipant: { type: "user", userId: boardUserId }, + changesRequestedCount: 3, + }, + ), + policy, + requestedStatus: "in_progress", + requestedAssigneePatch: {}, + actor: { userId: boardUserId }, + commentBody: "Human direction: do X instead", + }); + + expect(result.patch.status).toBe("in_progress"); + expect(result.patch.assigneeAgentId).toBe(coderAgentId); + expect(result.patch.executionState).toMatchObject({ + status: "changes_requested", + changesRequestedCount: 0, + }); + }); + + it("lets the escalated human approve the stage", () => { + const result = applyIssueExecutionPolicyTransition({ + issue: reviewPendingIssue( + { assigneeAgentId: null, assigneeUserId: boardUserId }, + { + currentParticipant: { type: "user", userId: boardUserId }, + changesRequestedCount: 3, + }, + ), + policy, + requestedStatus: "done", + requestedAssigneePatch: {}, + actor: { userId: boardUserId }, + commentBody: "Good enough — shipping", + }); + + expect(result.decision).toMatchObject({ outcome: "approved" }); + expect(result.patch.executionState).toMatchObject({ + status: "completed", + changesRequestedCount: 0, + }); + }); + + it("keeps handing back to the executor when no responsible human exists", () => { + const result = applyIssueExecutionPolicyTransition({ + issue: reviewPendingIssue({ responsibleUserId: null }, { changesRequestedCount: 9 }), + policy, + requestedStatus: "in_progress", + requestedAssigneePatch: {}, + actor: { agentId: qaAgentId }, + commentBody: "Round ten feedback", + }); + + expect(result.patch.status).toBe("in_progress"); + expect(result.patch.assigneeAgentId).toBe(coderAgentId); + expect(result.patch.executionState).toMatchObject({ + status: "changes_requested", + changesRequestedCount: 10, + }); + }); + + it("honors a policy maxReviewRounds override", () => { + const strictPolicy = normalizeIssueExecutionPolicy({ + stages: [{ type: "review", participants: [{ type: "agent", agentId: qaAgentId }] }], + maxReviewRounds: 1, + })!; + const stageId = strictPolicy.stages[0].id; + + const result = applyIssueExecutionPolicyTransition({ + issue: { + status: "in_review", + assigneeAgentId: qaAgentId, + assigneeUserId: null, + responsibleUserId: boardUserId, + executionPolicy: strictPolicy, + executionState: { + status: "pending", + currentStageId: stageId, + currentStageIndex: 0, + currentStageType: "review", + currentParticipant: { type: "agent", agentId: qaAgentId }, + returnAssignee: { type: "agent", agentId: coderAgentId }, + completedStageIds: [], + lastDecisionId: null, + lastDecisionOutcome: null, + }, + }, + policy: strictPolicy, + requestedStatus: "in_progress", + requestedAssigneePatch: {}, + actor: { agentId: qaAgentId }, + commentBody: "First and only agent round", + }); + + expect(result.patch.assigneeUserId).toBe(boardUserId); + expect(result.patch.executionState).toMatchObject({ + status: "pending", + currentParticipant: { type: "user", userId: boardUserId }, + changesRequestedCount: 1, + }); + }); +}); diff --git a/server/src/services/issue-execution-policy.ts b/server/src/services/issue-execution-policy.ts index ec04838fdd..1c7a054d8d 100644 --- a/server/src/services/issue-execution-policy.ts +++ b/server/src/services/issue-execution-policy.ts @@ -20,6 +20,8 @@ type AssigneeLike = { type IssueLike = AssigneeLike & { status: string; + responsibleUserId?: string | null; + createdByUserId?: string | null; executionPolicy?: IssueExecutionPolicy | Record | null; executionState?: IssueExecutionState | Record | null; monitorNextCheckAt?: Date | null; @@ -58,6 +60,13 @@ type TransitionResult = { workflowControlledAssignment?: boolean; }; +/** + * Consecutive agent-initiated changes-requested rounds tolerated on one stage + * before the pending review escalates to the responsible human. Policies can + * override via `maxReviewRounds`; human decisions always reset the counter. + */ +export const DEFAULT_MAX_REVIEW_ROUNDS = 3; + const COMPLETED_STATUS: IssueExecutionState["status"] = "completed"; const PENDING_STATUS: IssueExecutionState["status"] = "pending"; const CHANGES_REQUESTED_STATUS: IssueExecutionState["status"] = "changes_requested"; @@ -400,6 +409,7 @@ export function normalizeIssueExecutionPolicy(input: unknown): IssueExecutionPol ...(monitor ? { monitor } : {}), ...(reviewPreset ? { reviewPreset } : {}), ...(authorizationPolicy ? { authorizationPolicy } : {}), + ...(parsed.data.maxReviewRounds != null ? { maxReviewRounds: parsed.data.maxReviewRounds } : {}), }; } @@ -432,6 +442,24 @@ function principalsEqual(a: IssueExecutionStagePrincipal | null, b: IssueExecuti return a.type === "agent" ? a.agentId === b.agentId : a.userId === b.userId; } +function resolveMaxReviewRounds(policy: IssueExecutionPolicy | null): number { + const configured = policy?.maxReviewRounds; + return typeof configured === "number" && configured > 0 ? configured : DEFAULT_MAX_REVIEW_ROUNDS; +} + +/** + * The human a review stage escalates to when agents exhaust their + * changes-requested rounds. Without one the loop keeps handing back to the + * return assignee (pre-existing behavior) rather than stalling the stage. + */ +function reviewEscalationUserId(issue: IssueLike): string | null { + const responsible = issue.responsibleUserId?.trim(); + if (responsible) return responsible; + const creator = issue.createdByUserId?.trim(); + if (creator) return creator; + return null; +} + function findStageById(policy: IssueExecutionPolicy, stageId: string | null | undefined) { if (!stageId) return null; return policy.stages.find((stage) => stage.id === stageId) ?? null; @@ -497,6 +525,7 @@ function buildCompletedState(previous: IssueExecutionState | null, currentStage: lastDecisionId: previous?.lastDecisionId ?? null, lastDecisionOutcome: "approved", monitor: previous?.monitor ?? null, + changesRequestedCount: 0, }; } @@ -547,6 +576,7 @@ function buildPendingState(input: { participant: IssueExecutionStagePrincipal; returnAssignee: IssueExecutionStagePrincipal | null; reviewRequest?: IssueExecutionState["reviewRequest"] | null; + changesRequestedCount?: number; }): IssueExecutionState { return { status: PENDING_STATUS, @@ -560,10 +590,15 @@ function buildPendingState(input: { lastDecisionId: input.previous?.lastDecisionId ?? null, lastDecisionOutcome: input.previous?.lastDecisionOutcome ?? null, monitor: input.previous?.monitor ?? null, + changesRequestedCount: input.changesRequestedCount ?? input.previous?.changesRequestedCount ?? 0, }; } -function buildChangesRequestedState(previous: IssueExecutionState, currentStage: IssueExecutionStage): IssueExecutionState { +function buildChangesRequestedState( + previous: IssueExecutionState, + currentStage: IssueExecutionStage, + changesRequestedCount: number, +): IssueExecutionState { return { ...previous, status: CHANGES_REQUESTED_STATUS, @@ -571,6 +606,7 @@ function buildChangesRequestedState(previous: IssueExecutionState, currentStage: currentStageType: currentStage.type, reviewRequest: null, lastDecisionOutcome: "changes_requested", + changesRequestedCount, }; } @@ -582,6 +618,7 @@ function buildPendingStagePatch(input: { participant: IssueExecutionStagePrincipal; returnAssignee: IssueExecutionStagePrincipal | null; reviewRequest?: IssueExecutionState["reviewRequest"] | null; + changesRequestedCount?: number; }) { input.patch.status = "in_review"; Object.assign(input.patch, patchForPrincipal(input.participant)); @@ -592,6 +629,7 @@ function buildPendingStagePatch(input: { participant: input.participant, returnAssignee: input.returnAssignee, reviewRequest: input.reviewRequest, + changesRequestedCount: input.changesRequestedCount, }); } @@ -676,7 +714,45 @@ function applyIssueExecutionStageTransition(input: TransitionInput): TransitionR throw unprocessable(`No eligible ${activeStage.type} participant is configured for this issue`); } - if (!stageHasParticipant(activeStage, currentParticipant)) { + // An escalated review is deliberately held by a human who is not in the + // stage's configured participants. Re-selecting a configured (agent) + // participant would silently undo the escalation on the next unrelated + // PATCH, so the hold is sticky until the escalated human decides — their + // own decisions fall through to the participant decision branch below. + const escalatedHold = + currentParticipant.type === "user" && + !stageHasParticipant(activeStage, currentParticipant) && + (existingState?.changesRequestedCount ?? 0) >= resolveMaxReviewRounds(input.policy); + if (escalatedHold && !principalsEqual(currentParticipant, actor)) { + // An empty patch would not override the caller's own requested fields, + // so a status or assignee change from a non-escalated actor must be + // rejected outright — mirroring the "only the active participant can + // advance" rule below — and a drifted assignee must be re-asserted + // rather than left pointing away from the escalated human. + const attemptedAdvanceDuringHold = + (requestedStatus !== undefined && requestedStatus !== "in_review") || + (requestedAssigneePatchProvided && !principalsEqual(explicitAssignee, currentParticipant)); + if (attemptedAdvanceDuringHold) { + throw unprocessable("Only the escalated reviewer can advance the current execution stage"); + } + const holdDrifted = + input.issue.status !== "in_review" || + !principalsEqual(currentAssignee, currentParticipant); + if (holdDrifted) { + patch.status = "in_review"; + Object.assign(patch, patchForPrincipal(currentParticipant)); + patch.executionState = buildPendingState({ + previous: existingState, + stage: activeStage, + stageIndex: input.policy.stages.findIndex((candidate) => candidate.id === activeStage.id), + participant: currentParticipant, + returnAssignee: existingState?.returnAssignee ?? null, + reviewRequest: effectiveReviewRequest, + }); + } + return { patch }; + } + if (!escalatedHold && !stageHasParticipant(activeStage, currentParticipant)) { const participant = selectStageParticipant(activeStage, { preferred: explicitAssignee ?? existingState?.currentParticipant ?? null, exclude: existingState?.returnAssignee ?? null, @@ -768,17 +844,47 @@ function applyIssueExecutionStageTransition(input: TransitionInput): TransitionR if (!existingState?.returnAssignee) { throw unprocessable("This execution stage has no return assignee"); } + const decision = { + stageId: activeStage.id, + stageType: activeStage.type, + outcome: "changes_requested" as const, + body: input.commentBody.trim(), + }; + // Human decisions reset the round counter: the cap exists to stop + // unattended agent↔agent ping-pong, not to limit human review. + const actorIsHuman = actor?.type === "user"; + const nextRounds = actorIsHuman ? 0 : (existingState.changesRequestedCount ?? 0) + 1; + if (!actorIsHuman && nextRounds >= resolveMaxReviewRounds(input.policy)) { + const escalationUserId = reviewEscalationUserId(input.issue); + if (escalationUserId) { + // Rounds exhausted: keep the stage pending but hand it to the + // responsible human instead of bouncing back to the implementer. + // The recorded changes-requested decision carries the reviewer's + // reasoning; the human approves, requests changes (resetting the + // counter), or re-scopes. + buildPendingStagePatch({ + patch, + previous: existingState, + policy: input.policy, + stage: activeStage, + participant: { type: "user", agentId: null, userId: escalationUserId }, + returnAssignee: existingState.returnAssignee, + reviewRequest: effectiveReviewRequest, + changesRequestedCount: nextRounds, + }); + return { + patch, + decision, + workflowControlledAssignment: true, + }; + } + } patch.status = "in_progress"; Object.assign(patch, patchForPrincipal(existingState.returnAssignee)); - patch.executionState = buildChangesRequestedState(existingState, activeStage); + patch.executionState = buildChangesRequestedState(existingState, activeStage, nextRounds); return { patch, - decision: { - stageId: activeStage.id, - stageType: activeStage.type, - outcome: "changes_requested", - body: input.commentBody.trim(), - }, + decision, workflowControlledAssignment: true, }; }