diff --git a/doc/execution-semantics.md b/doc/execution-semantics.md index 180bd820b6..1e3901303d 100644 --- a/doc/execution-semantics.md +++ b/doc/execution-semantics.md @@ -521,7 +521,7 @@ tasks hide both waiting surfaces even if a stale schedule remains in the respons Because `serviceName` and `notes` remain visible in issue activity and wake context, operators should keep them short and non-secret. Put enough context for the assignee to know what to inspect, but do not include signed URLs, bearer tokens, customer secrets, tenant-private identifiers, or provider links with embedded credentials. -Monitor bounds are enforced. Paperclip rejects attempts to re-arm a monitor whose `timeoutAt` or `maxAttempts` is already exhausted. When a scheduled monitor reaches an exhausted bound at trigger time, Paperclip clears it and follows `recoveryPolicy`: `wake_owner` queues a bounded recovery wake for the assignee, `create_recovery_issue` opens visible issue-backed recovery work, and `escalate_to_board` records a board-visible escalation comment/activity. +Monitor bounds are enforced, and they bound one monitor rather than the issue. `maxAttempts` counts the attempts of the monitor that is currently scheduled, so a `maxAttempts` below the attempts that monitor already spent is rejected. A monitor that has fired or been cleared has completed its lifecycle: the next `nextCheckAt` is a new monitor and starts at attempt zero. A `timeoutAt` that has already passed is rejected, because that monitor can never fire. Every rejection names the bound that stopped it, the attempts already spent, and the minimum `maxAttempts` that is accepted. When a scheduled monitor reaches an exhausted bound at trigger time, Paperclip clears it and follows `recoveryPolicy`: `wake_owner` queues a bounded recovery wake for the assignee, `create_recovery_issue` opens visible issue-backed recovery work, and `escalate_to_board` records a board-visible escalation comment/activity. Use `blocked` instead of a monitor when no Paperclip assignee owns a responsible polling path. In that case, name the external owner/action or create first-class recovery/blocker work. diff --git a/server/src/__tests__/issue-execution-policy-routes.test.ts b/server/src/__tests__/issue-execution-policy-routes.test.ts index 4478356f78..a3e643220a 100644 --- a/server/src/__tests__/issue-execution-policy-routes.test.ts +++ b/server/src/__tests__/issue-execution-policy-routes.test.ts @@ -708,6 +708,80 @@ describe("issue execution policy routes", () => { ); }); + it("schedules a fresh monitor on an issue whose previous monitor exhausted its attempts", async () => { + const issue = { + id: "aaaaaaaa-aaaa-4aaa-8aaa-aaaaaaaaaaaa", + companyId: "company-1", + status: "in_review", + assigneeAgentId: "33333333-3333-4333-8333-333333333333", + assigneeUserId: null, + createdByUserId: "local-board", + identifier: "PAP-1008", + title: "Monitor re-armed after exhaustion", + executionPolicy: null, + executionState: { + status: "idle", + currentStageId: null, + currentStageIndex: null, + currentStageType: null, + currentParticipant: null, + returnAssignee: null, + completedStageIds: [], + lastDecisionId: null, + lastDecisionOutcome: null, + monitor: { + status: "cleared", + nextCheckAt: null, + lastTriggeredAt: "2026-11-01T12:00:00.000Z", + attemptCount: 3, + maxAttempts: 3, + notes: null, + scheduledBy: "assignee", + clearedAt: "2026-11-01T12:00:00.000Z", + clearReason: "max_attempts_exhausted", + }, + }, + monitorAttemptCount: 3, + monitorNextCheckAt: null, + monitorLastTriggeredAt: new Date("2026-11-01T12:00:00.000Z"), + monitorNotes: null, + monitorScheduledBy: "assignee", + }; + mockIssueService.getById.mockResolvedValue(issue); + mockIssueService.update.mockImplementation(async (_id: string, patch: Record) => ({ + ...issue, + ...patch, + updatedAt: new Date(), + })); + + const res = await request(await createApp({ + type: "agent", + agentId: "33333333-3333-4333-8333-333333333333", + companyId: "company-1", + runId: "55555555-5555-4555-8555-555555555555", + })) + .patch("/api/issues/aaaaaaaa-aaaa-4aaa-8aaa-aaaaaaaaaaaa") + .send({ + executionPolicy: { + monitor: { + nextCheckAt: "2026-12-01T12:00:00.000Z", + scheduledBy: "assignee", + maxAttempts: 3, + notes: "Wait for the follow-up review.", + }, + }, + }); + + expect(res.status).toBe(200); + expect(mockIssueService.update).toHaveBeenCalledWith( + "aaaaaaaa-aaaa-4aaa-8aaa-aaaaaaaaaaaa", + expect.objectContaining({ + monitorNextCheckAt: new Date("2026-12-01T12:00:00.000Z"), + monitorAttemptCount: 0, + }), + ); + }); + it("allows board-authored in_review repair updates without a review path", async () => { const issue = { id: "aaaaaaaa-aaaa-4aaa-8aaa-aaaaaaaaaaaa", diff --git a/server/src/__tests__/issue-execution-policy.test.ts b/server/src/__tests__/issue-execution-policy.test.ts index 8b9d560191..8c1e4f4afe 100644 --- a/server/src/__tests__/issue-execution-policy.test.ts +++ b/server/src/__tests__/issue-execution-policy.test.ts @@ -1760,7 +1760,7 @@ describe("issue execution policy transitions", () => { ).toThrow("Monitor can only be scheduled"); }); - it("rejects explicitly re-arming a monitor after max attempts are exhausted", () => { + it("starts a new monitor at attempt zero once the previous monitor is spent", () => { const policy = normalizeIssueExecutionPolicy({ stages: [], monitor: { @@ -1770,6 +1770,102 @@ describe("issue execution policy transitions", () => { }, })!; + const result = applyIssueExecutionPolicyTransition({ + issue: { + status: "in_review", + assigneeAgentId: coderAgentId, + assigneeUserId: null, + executionPolicy: null, + executionState: null, + monitorAttemptCount: 1, + monitorNextCheckAt: null, + monitorLastTriggeredAt: new Date("2026-04-11T12:30:00.000Z"), + monitorNotes: null, + monitorScheduledBy: "assignee", + }, + policy, + previousPolicy: null, + requestedAssigneePatch: {}, + actor: { agentId: coderAgentId }, + monitorExplicitlyUpdated: true, + }); + + expect(result.patch.monitorNextCheckAt).toEqual(new Date("2099-04-11T12:30:00.000Z")); + expect(result.patch.monitorAttemptCount).toBe(0); + expect(result.patch.executionState).toMatchObject({ + monitor: { + status: "scheduled", + attemptCount: 0, + maxAttempts: 1, + }, + }); + }); + + it("still rejects a maxAttempts below the attempts the scheduled monitor already spent", () => { + const policy = normalizeIssueExecutionPolicy({ + stages: [], + monitor: { + nextCheckAt: "2099-04-11T12:30:00.000Z", + maxAttempts: 2, + scheduledBy: "assignee", + }, + })!; + + expect(() => + applyIssueExecutionPolicyTransition({ + issue: { + status: "in_review", + assigneeAgentId: coderAgentId, + assigneeUserId: null, + executionPolicy: null, + executionState: { + status: "idle", + currentStageId: null, + currentStageIndex: null, + currentStageType: null, + currentParticipant: null, + returnAssignee: null, + completedStageIds: [], + lastDecisionId: null, + lastDecisionOutcome: null, + monitor: { + status: "scheduled", + nextCheckAt: "2098-04-11T12:30:00.000Z", + lastTriggeredAt: "2026-04-11T12:30:00.000Z", + attemptCount: 2, + notes: null, + scheduledBy: "assignee", + clearedAt: null, + clearReason: null, + }, + }, + monitorAttemptCount: 2, + monitorNextCheckAt: new Date("2098-04-11T12:30:00.000Z"), + monitorLastTriggeredAt: new Date("2026-04-11T12:30:00.000Z"), + monitorNotes: null, + monitorScheduledBy: "assignee", + }, + policy, + previousPolicy: null, + requestedAssigneePatch: {}, + actor: { agentId: coderAgentId }, + monitorExplicitlyUpdated: true, + }), + ).toThrow( + "Monitor bounds are already exhausted: the scheduled monitor has already used 2 attempt(s) and maxAttempts is 2. Supply maxAttempts of at least 3.", + ); + }); + + it("names the offending timeoutAt when the monitor deadline has already passed", () => { + const policy = normalizeIssueExecutionPolicy({ + stages: [], + monitor: { + nextCheckAt: "2099-04-11T12:30:00.000Z", + timeoutAt: "2020-04-11T12:30:00.000Z", + scheduledBy: "assignee", + }, + })!; + expect(() => applyIssueExecutionPolicyTransition({ issue: { @@ -1778,11 +1874,6 @@ describe("issue execution policy transitions", () => { assigneeUserId: null, executionPolicy: null, executionState: null, - monitorAttemptCount: 1, - monitorNextCheckAt: null, - monitorLastTriggeredAt: null, - monitorNotes: null, - monitorScheduledBy: "assignee", }, policy, previousPolicy: null, @@ -1790,7 +1881,7 @@ describe("issue execution policy transitions", () => { actor: { agentId: coderAgentId }, monitorExplicitlyUpdated: true, }), - ).toThrow("Monitor bounds are already exhausted"); + ).toThrow("timeoutAt 2020-04-11T12:30:00.000Z has already passed"); }); }); }); diff --git a/server/src/services/issue-execution-policy.ts b/server/src/services/issue-execution-policy.ts index 859dc5fe1e..f638a9760b 100644 --- a/server/src/services/issue-execution-policy.ts +++ b/server/src/services/issue-execution-policy.ts @@ -210,6 +210,25 @@ function derivePersistedMonitorState(input: { return null; } +/** + * Attempts carried into a newly scheduled monitor. + * + * `maxAttempts` bounds one monitor, not the issue. Only a monitor that is still + * `scheduled` carries its attempts forward, so lowering `maxAttempts` below the + * attempts the live monitor already spent is still rejected. Once a monitor has + * fired (`triggered`) or been cleared its lifecycle is over — the policy monitor + * is stripped on trigger — so the next `nextCheckAt` is a new monitor and starts + * at zero. + * + * Without the reset the count is a lifetime cap on the issue: every monitor the + * issue ever held is charged against whatever `maxAttempts` the next caller + * supplies, so one exhausted monitor permanently blocks every later one and + * strands `in_review` issues with no wake path at all. + */ +function carriedMonitorAttemptCount(previous: IssueExecutionMonitorState | null | undefined): number { + return previous?.status === "scheduled" ? previous.attemptCount ?? 0 : 0; +} + function buildScheduledMonitorState( previous: IssueExecutionMonitorState | null, monitor: IssueExecutionMonitorPolicy, @@ -218,7 +237,7 @@ function buildScheduledMonitorState( status: "scheduled", nextCheckAt: monitor.nextCheckAt, lastTriggeredAt: previous?.lastTriggeredAt ?? null, - attemptCount: previous?.attemptCount ?? 0, + attemptCount: carriedMonitorAttemptCount(previous), notes: monitor.notes ?? null, scheduledBy: monitor.scheduledBy, ...monitorMetadataFromPolicy(monitor), @@ -302,6 +321,36 @@ function exhaustedMonitorClearReason(input: { return null; } +/** + * Name the bound that rejected the monitor, the count it was compared against, + * and the value that would be accepted. "Monitor bounds are already exhausted" + * on its own tells the caller neither which bound tripped nor how to satisfy it. + */ +function monitorBoundsExhaustedError(input: { + clearReason: IssueExecutionMonitorClearReason; + monitor: IssueExecutionMonitorPolicy; + attemptCount: number; +}) { + const maxAttempts = input.monitor.maxAttempts ?? null; + const details: Record = { + clearReason: input.clearReason, + attemptCount: input.attemptCount, + maxAttempts, + timeoutAt: input.monitor.timeoutAt ?? null, + }; + if (input.clearReason === "timeout_exceeded") { + return unprocessable( + `${MONITOR_BOUNDS_EXHAUSTED_MESSAGE}: timeoutAt ${input.monitor.timeoutAt} has already passed. Supply a timeoutAt in the future, or omit it.`, + details, + ); + } + const minimumMaxAttempts = input.attemptCount + 1; + return unprocessable( + `${MONITOR_BOUNDS_EXHAUSTED_MESSAGE}: the scheduled monitor has already used ${input.attemptCount} attempt(s) and maxAttempts is ${maxAttempts}. Supply maxAttempts of at least ${minimumMaxAttempts}.`, + { ...details, minimumMaxAttempts }, + ); +} + function nextAssigneeIds(input: { issue: IssueLike; requestedAssigneePatch: RequestedAssigneePatch; @@ -1086,14 +1135,19 @@ function applyMonitorTransition(input: TransitionInput, stagePatch: Record { expect(decision).toEqual({ kind: "skip", reason: "review issue still has a maintained path" }); }); + it("fingerprints monitor wakes per monitor, not per attempt count", () => { + const refFor = (nextCheckAt: string) => reviewPathConsumedRefFromRun({ + runId: "run-1", + issueId: "issue-1", + contextSnapshot: { + wakeReason: "issue_monitor", + nextCheckAt, + monitorAttemptCount: 1, + }, + }); + + // Two monitors, each on its first attempt: distinct fingerprints. + expect(refFor("2026-12-01T12:00:00.000Z")).not.toEqual(refFor("2026-12-08T12:00:00.000Z")); + + // Contexts without a scheduled instant still fall back to the attempt count. + expect(reviewPathConsumedRefFromRun({ + runId: "run-1", + issueId: "issue-1", + contextSnapshot: { wakeReason: "issue_monitor", monitorAttemptCount: 2 }, + })).toBe("monitor:issue-1:cleared:2"); + }); + it("recognizes wrapped atomic deduplication conflicts without swallowing unrelated uniqueness errors", () => { expect(isReviewPathRecoveryIdempotencyConflict({ cause: { diff --git a/server/src/services/recovery/review-path-recovery.ts b/server/src/services/recovery/review-path-recovery.ts index 27ef24234f..9c6c4b986b 100644 --- a/server/src/services/recovery/review-path-recovery.ts +++ b/server/src/services/recovery/review-path-recovery.ts @@ -21,7 +21,13 @@ export function reviewPathConsumedRefFromRun(input: { ?? readNonEmptyString(context.interactionId) ?? readNonEmptyString(context.approvalId) ?? (readNonEmptyString(context.wakeReason)?.startsWith("issue_monitor") - ? `monitor:${input.issueId}:${readNonEmptyString(context.clearReason) ?? "cleared"}:${String(context.monitorAttemptCount ?? "unknown")}` + // Prefer the monitor's scheduled instant. The attempt count restarts at + // zero for each new monitor, so counting alone would let two monitors on + // the same issue share a fingerprint and silently suppress the second + // one's recovery wake. + ? `monitor:${input.issueId}:${readNonEmptyString(context.clearReason) ?? "cleared"}:${ + readNonEmptyString(context.nextCheckAt) ?? String(context.monitorAttemptCount ?? "unknown") + }` : null) ?? input.runId; }