diff --git a/doc/SPEC-implementation.md b/doc/SPEC-implementation.md index 053a49216d9..26f97cf0568 100644 --- a/doc/SPEC-implementation.md +++ b/doc/SPEC-implementation.md @@ -1520,6 +1520,20 @@ timestamps, including on repeated release. For unfinished issues it clears the agent assignee; only `in_progress` changes to `todo`. Existing company access, assignee/run ownership checks, and the `issue.released` activity event still apply. +A status change releases the checkout and execution locks but does not stop the +run that holds them. An agent request (`PATCH /issues/:issueId` or a recovery +resolution) that sets a status other than `in_progress` (even its current value) on +a task whose lock is held by a different queued or running run is refused with +`409` and code `issue_status_change_live_run`; +otherwise the run would keep going with no task bound to it, which is the effect +of interrupting another agent's run. The denial is final and names the person's +controls on the linked task: the run's Stop or Interrupt, or More task actions, +then Pause subtree on the parent. The lock-holding run, board users, and paths +that stop the run themselves (cancelling the task, reassigning it, and completing +through `PATCH` a task whose Runner goal is paused first) are unaffected. The write +is conditional on the lock the check saw; a run that acquires the lock in between returns `409` +`issue_execution_lock_changed`. + `POST /issues/:issueId/admin/force-release` is an operator recovery endpoint for stale harness locks. It requires board access to the issue company, clears checkout and execution run lock fields, and may clear the agent assignee when `clearAssignee=true` is passed. The route must write an `issue.admin_force_release` activity log entry containing the previous checkout and execution run IDs. ## 10.5 Projects diff --git a/packages/shared/src/issue-write-denial.ts b/packages/shared/src/issue-write-denial.ts index 737bab7bd7a..8e551dabfa1 100644 --- a/packages/shared/src/issue-write-denial.ts +++ b/packages/shared/src/issue-write-denial.ts @@ -29,6 +29,7 @@ export const ISSUE_WRITE_DENIAL_CODES = [ "issue_write_responsible_user_ceiling", "issue_write_responsible_user_unavailable", "issue_write_assignee_run_lock", + "issue_status_change_live_run", "cross_issue_influence_cap_exceeded", "cross_issue_influence_run_context_required", "issue_write_attribution_spoof_rejected", @@ -69,6 +70,8 @@ export interface IssueWriteDenialContext { assigneeLabel?: string | null; /** Target issue identifier, e.g. `TASK-482`. */ issueIdentifier?: string | null; + /** Board path of the target issue, e.g. `/TASK/issues/TASK-482`. */ + issuePath?: string | null; /** Per-run cross-issue influence cap. */ cap?: number | null; /** Attempt count that tripped the cap. */ @@ -221,6 +224,32 @@ export function describeIssueWriteDenial( `wait for the run to release the lock and retry.`, }; + case "issue_status_change_live_run": { + const identifier = context.issueIdentifier?.trim(); + const path = context.issuePath?.trim(); + const taskLink = identifier && path ? `[${identifier}](${path})` : issue; + return { + code, + status: 409, + tone: "boundary", + boundary: "Live run execution lock", + title: "Another run is executing this task", + description: + `A run is live on ${issue} and holds its execution lock. Changing the status ` + + `would release that lock without stopping the run, so the run would keep ` + + `going with no task bound to it. Agents cannot stop or interrupt another ` + + `agent's run, so this status change is refused.`, + whoCanAct: + `A board user, or the run of ${assignee} that holds the lock.`, + sanctionedPath: + `This refusal is final: do not retry with another status, assignee or route. ` + + `Reply now and send the person to ${taskLink}. To stop the live run they use its ` + + `Stop or Interrupt control there; to hold this task and the tasks under it, they ` + + `open the parent task and choose More task actions, then Pause subtree. To pass ` + + `a request to the running agent, comment on the task instead.`, + }; + } + case "cross_issue_influence_cap_exceeded": { const cap = context.cap ?? 20; const attempt = context.count ?? null; diff --git a/server/src/__tests__/issue-agent-mutation-ownership-routes.test.ts b/server/src/__tests__/issue-agent-mutation-ownership-routes.test.ts index 2318870258c..8529d65d0e1 100644 --- a/server/src/__tests__/issue-agent-mutation-ownership-routes.test.ts +++ b/server/src/__tests__/issue-agent-mutation-ownership-routes.test.ts @@ -3251,6 +3251,128 @@ describe("agent issue mutation checkout ownership", () => { }); }); + describe("agent status change on a task another run is executing", () => { + const liveRun = { + id: ownerRunId, + companyId, + agentId: ownerAgentId, + runtimeMode: "legacy", + status: "running", + contextSnapshot: { issueId }, + }; + // A manager (or CEO) holds tasks:manage_active_checkouts over its report's + // checked-out task. That grant lets it edit fields, not stop the report's run. + const grantActiveCheckoutManagement = () => { + mockAccessService.decide.mockImplementation(async (input: { action: string }) => ({ + allowed: ["issue:mutate", "issue:read", "tasks:manage_active_checkouts", "tasks:assign"].includes(input.action), + action: input.action, + reason: "allow_manager_chain", + explanation: "Allowed by test manager chain.", + })); + }; + + beforeEach(() => { + grantActiveCheckoutManagement(); + mockIssueService.getById.mockResolvedValue(makeIssue({ executionRunId: ownerRunId, checkoutRunId: ownerRunId })); + mockHeartbeatService.getRun.mockResolvedValue(liveRun); + }); + + it.each(["backlog", "todo", "blocked", "in_review", "done"])( + "refuses a lead moving a running report's task to %s, which would detach the live run", + async (status) => { + const res = await request(createApp(peerActor())).patch(`/api/issues/${issueId}`).send({ status }); + + expect(res.status, JSON.stringify(res.body)).toBe(409); + expect(res.body.details).toMatchObject({ + code: "issue_status_change_live_run", + issueId, + runId: ownerRunId, + requestedStatus: status, + issuePath: "/PAP/issues/PAP-1649", + }); + // The denial is final and names the person's controls and the task link. + expect(res.body.error).toContain("final"); + expect(res.body.error).toContain("More task actions, then Pause subtree"); + expect(res.body.error).toContain("Stop"); + expect(res.body.error).toContain("[PAP-1649](/PAP/issues/PAP-1649)"); + expect(mockIssueService.update).not.toHaveBeenCalled(); + expect(mockHeartbeatService.cancelRun).not.toHaveBeenCalled(); + }, + ); + + it("refuses moving a todo task whose queued run holds the execution lock", async () => { + mockIssueService.getById.mockResolvedValue(makeIssue({ status: "todo", executionRunId: ownerRunId })); + mockHeartbeatService.getRun.mockResolvedValue({ ...liveRun, status: "queued" }); + + const res = await request(createApp(peerActor())).patch(`/api/issues/${issueId}`).send({ status: "backlog" }); + + expect(res.status, JSON.stringify(res.body)).toBe(409); + expect(res.body.details).toMatchObject({ code: "issue_status_change_live_run", runId: ownerRunId }); + expect(mockIssueService.update).not.toHaveBeenCalled(); + }); + + it("refuses repeating the current status, which also releases the lock", async () => { + mockIssueService.getById.mockResolvedValue(makeIssue({ status: "todo", executionRunId: ownerRunId })); + mockHeartbeatService.getRun.mockResolvedValue({ ...liveRun, status: "queued" }); + + const res = await request(createApp(peerActor())).patch(`/api/issues/${issueId}`).send({ status: "todo" }); + + expect(res.status, JSON.stringify(res.body)).toBe(409); + expect(res.body.details).toMatchObject({ code: "issue_status_change_live_run", requestedStatus: "todo" }); + expect(mockIssueService.update).not.toHaveBeenCalled(); + }); + + it("makes an allowed agent status change conditional on the lock it checked", async () => { + mockHeartbeatService.getRun.mockResolvedValue({ ...liveRun, status: "succeeded" }); + + const res = await request(createApp(peerActor())).patch(`/api/issues/${issueId}`).send({ status: "backlog" }); + + expect(res.status, JSON.stringify(res.body)).toBe(200); + expect(mockIssueService.update.mock.calls[0]?.[1]).toMatchObject({ + status: "backlog", + expectedExecutionLock: { executionRunId: ownerRunId, checkoutRunId: ownerRunId }, + }); + }); + + it("still lets the lead edit other fields of the running task", async () => { + const res = await request(createApp(peerActor())).patch(`/api/issues/${issueId}`).send({ title: "Renamed by lead" }); + + expect(res.status, JSON.stringify(res.body)).toBe(200); + expect(mockIssueService.update).toHaveBeenCalledTimes(1); + }); + + it("lets the run that holds the lock change its own task status", async () => { + const res = await request(createApp(ownerActor())).patch(`/api/issues/${issueId}`).send({ status: "in_review" }); + + expect(res.status, JSON.stringify(res.body)).toBe(200); + expect(mockIssueService.update.mock.calls[0]?.[1]).toMatchObject({ status: "in_review" }); + }); + + it("lets a board user change the status", async () => { + const res = await request(createApp(boardActor())).patch(`/api/issues/${issueId}`).send({ status: "todo" }); + + expect(res.status, JSON.stringify(res.body)).toBe(200); + expect(mockIssueService.update.mock.calls[0]?.[1]).toMatchObject({ status: "todo" }); + }); + + it("leaves cancellation to the path that cancels the run", async () => { + const res = await request(createApp(peerActor())).patch(`/api/issues/${issueId}`).send({ status: "cancelled" }); + + expect(res.status, JSON.stringify(res.body)).toBe(200); + expect(mockIssueService.update.mock.calls[0]?.[1]).toMatchObject({ status: "cancelled" }); + expect(mockHeartbeatService.cancelRun).toHaveBeenCalledWith(ownerRunId); + }); + + it.each(["succeeded", "failed", "cancelled"])("allows the change when the lock holder already %s", async (runStatus) => { + mockHeartbeatService.getRun.mockResolvedValue({ ...liveRun, status: runStatus }); + + const res = await request(createApp(peerActor())).patch(`/api/issues/${issueId}`).send({ status: "backlog" }); + + expect(res.status, JSON.stringify(res.body)).toBe(200); + expect(mockIssueService.update.mock.calls[0]?.[1]).toMatchObject({ status: "backlog" }); + }); + }); + describe("task watchdog scope grants", () => { const watchdogRunId = "aaaaaaaa-aaaa-4aaa-8aaa-aaaaaaaaaaab"; const watchdogReportIssueId = "cccccccc-cccc-4ccc-8ccc-cccccccccccd"; diff --git a/server/src/__tests__/issue-stale-execution-lock-routes.test.ts b/server/src/__tests__/issue-stale-execution-lock-routes.test.ts index 887b4822663..fcf751fb071 100644 --- a/server/src/__tests__/issue-stale-execution-lock-routes.test.ts +++ b/server/src/__tests__/issue-stale-execution-lock-routes.test.ts @@ -21,6 +21,7 @@ import { } from "./helpers/embedded-postgres.js"; import { errorHandler } from "../middleware/index.js"; import { issueRoutes } from "../routes/issues.js"; +import { issueService } from "../services/issues.js"; const embeddedPostgresSupport = await getEmbeddedPostgresTestSupport(); const describeEmbeddedPostgres = embeddedPostgresSupport.supported ? describe : describe.skip; @@ -573,4 +574,117 @@ describeEmbeddedPostgres("stale issue execution lock routes", () => { executionRunId: currentRunId, }); }); + + it("keeps a report's live run bound when its manager tries to move the running task to backlog", async () => { + // A lead "pausing" work by editing statuses: before this guard the status + // change released the execution lock and left the report's run running, + // no longer bound to its task. + const { companyId, agentId: reportAgentId, currentRunId: reportRunId } = await seedCompanyAgentAndRuns(); + const leadAgentId = randomUUID(); + const leadRunId = randomUUID(); + const issueId = randomUUID(); + await db.insert(agents).values({ + id: leadAgentId, + companyId, + name: "LaunchLead", + role: "engineer", + status: "active", + adapterType: "codex_local", + adapterConfig: {}, + runtimeConfig: {}, + permissions: {}, + }); + await db.update(agents).set({ reportsTo: leadAgentId }).where(eq(agents.id, reportAgentId)); + await db.insert(heartbeatRuns).values({ + id: leadRunId, + companyId, + agentId: leadAgentId, + status: "running", + invocationSource: "manual", + startedAt: new Date(), + }); + await db.insert(issues).values({ + id: issueId, + companyId, + title: "Build the launch website", + status: "in_progress", + priority: "high", + assigneeAgentId: reportAgentId, + checkoutRunId: reportRunId, + executionRunId: reportRunId, + executionAgentNameKey: "codexcoder", + executionLockedAt: new Date(), + }); + await db.update(heartbeatRuns) + .set({ contextSnapshot: { issueId } }) + .where(eq(heartbeatRuns.id, reportRunId)); + + const res = await request(createApp(agentActor(companyId, leadAgentId, leadRunId))) + .patch(`/api/issues/${issueId}`) + .send({ status: "backlog" }); + + expect(res.status, JSON.stringify(res.body)).toBe(409); + expect(res.body.details).toMatchObject({ code: "issue_status_change_live_run", runId: reportRunId }); + expect(res.body.error).toContain("More task actions, then Pause subtree"); + + const row = await db + .select({ + status: issues.status, + assigneeAgentId: issues.assigneeAgentId, + checkoutRunId: issues.checkoutRunId, + executionRunId: issues.executionRunId, + }) + .from(issues) + .where(eq(issues.id, issueId)) + .then((rows) => rows[0]); + expect(row).toEqual({ + status: "in_progress", + assigneeAgentId: reportAgentId, + checkoutRunId: reportRunId, + executionRunId: reportRunId, + }); + const run = await db.select({ status: heartbeatRuns.status }).from(heartbeatRuns) + .where(eq(heartbeatRuns.id, reportRunId)).then((rows) => rows[0]); + expect(run?.status).toBe("running"); + }); + + it("rejects a status change whose checked execution lock changed before the write", async () => { + // The route checks the lock, then a checkout lands before the write. The + // write must not release the newly acquired lock. + const { companyId, agentId, currentRunId } = await seedCompanyAgentAndRuns(); + const issueId = randomUUID(); + await db.insert(issues).values({ + id: issueId, + companyId, + title: "Checked out after the check", + status: "in_progress", + priority: "high", + assigneeAgentId: agentId, + checkoutRunId: currentRunId, + executionRunId: currentRunId, + executionAgentNameKey: "codexcoder", + executionLockedAt: new Date(), + }); + + await expect(issueService(db).update(issueId, { + status: "backlog", + expectedExecutionLock: { executionRunId: null, checkoutRunId: null }, + })).rejects.toMatchObject({ status: 409, details: { code: "issue_execution_lock_changed" } }); + + const row = await db + .select({ status: issues.status, checkoutRunId: issues.checkoutRunId, executionRunId: issues.executionRunId }) + .from(issues) + .where(eq(issues.id, issueId)) + .then((rows) => rows[0]); + expect(row).toEqual({ status: "in_progress", checkoutRunId: currentRunId, executionRunId: currentRunId }); + + // A lock that was released after the check (the route's own confirmed Stop) + // does not block the write. + await db.update(issues).set({ checkoutRunId: null, executionRunId: null }).where(eq(issues.id, issueId)); + const updated = await issueService(db).update(issueId, { + status: "backlog", + expectedExecutionLock: { executionRunId: currentRunId, checkoutRunId: currentRunId }, + }); + expect(updated?.status).toBe("backlog"); + }); }); diff --git a/server/src/routes/issues.ts b/server/src/routes/issues.ts index b250c86541d..cee0bb74056 100644 --- a/server/src/routes/issues.ts +++ b/server/src/routes/issues.ts @@ -10605,6 +10605,13 @@ export function issueRoutes( if (sourceStatusChanged) { await assertNativeRunKeepsOwnExecutionLock(req, lockedIssue, sourceIssueStatus); + // The source row is locked in this transaction, so the check holds until the write. + await assertAgentStatusChangeKeepsLiveRunBound(req, lockedIssue, { + requestedStatus: sourceIssueStatus, + assigneeWillChange: false, + stopsGoalOnCompletion: false, + refreshLock: false, + }); const safeHandBack = outcome === "restored" && sourceIssueStatus === "todo" && @@ -14389,6 +14396,84 @@ export function issueRoutes( ); } + /** + * A status change releases the execution lock without stopping the run that + * holds it (the service clears the lock for any status other than + * `in_progress`, even when the status is unchanged). When an agent other than + * the lock-holding run asks for it, the run keeps going but is no longer + * bound to its task: the effect of interrupting another agent's run, which + * agents are not allowed to do (run cancel and tree pause are board-only). + * Refuse it before any side effect, with a final denial that sends the person + * to the controls that really stop or hold the work. + * + * Paths that stop the run themselves stay open: cancelling the task, + * reassigning it, and (only where the caller pauses the goal first, which the + * PATCH route does) completing a task whose Runner goal is active. + * + * Returns the lock the decision was based on, so the caller can make the + * write conditional on it (`expectedExecutionLock`); null when the change + * does not release a lock under this rule. + */ + async function assertAgentStatusChangeKeepsLiveRunBound( + req: Request, + issue: { + id: string; + companyId: string; + identifier?: string | null; + status: string; + assigneeAgentId: string | null; + checkoutRunId: string | null; + executionRunId: string | null; + }, + change: { + requestedStatus: unknown; + assigneeWillChange: boolean; + /** The caller pauses an active Runner goal (and stops its run) before completing the task. */ + stopsGoalOnCompletion: boolean; + /** Re-read the lock instead of trusting a snapshot taken earlier in the request. */ + refreshLock: boolean; + }, + ): Promise<{ executionRunId: string | null; checkoutRunId: string | null } | null> { + if (req.actor.type !== "agent") return null; + const { requestedStatus } = change; + if (typeof requestedStatus !== "string") return null; + if (requestedStatus === "in_progress" || requestedStatus === "cancelled") return null; + if (change.assigneeWillChange) return null; + const current = change.refreshLock ? (await svc.getById(issue.id)) ?? issue : issue; + const lock = { + executionRunId: current.executionRunId ?? null, + checkoutRunId: current.checkoutRunId ?? null, + }; + const actorRunId = req.actor.runId?.trim() || null; + const lockRunIds = [...new Set([lock.executionRunId, lock.checkoutRunId])] + .filter((runId): runId is string => Boolean(runId) && runId !== actorRunId); + for (const runId of lockRunIds) { + const run = await heartbeat.getRun(runId); + if (!run || run.companyId !== issue.companyId || run.id === actorRunId) continue; + if (run.status !== "running" && run.status !== "queued") continue; + if (change.stopsGoalOnCompletion && requestedStatus === "done" && issue.assigneeAgentId) { + const goal = await runnerGoals.projection(issue.companyId, issue.id, issue.assigneeAgentId); + // Terminalization pauses the goal and stops this run before the write. + if (goal?.goal && goal.goal.status !== "complete") return null; + } + const company = await companiesSvc.getById(issue.companyId).catch(() => null); + const identifier = issue.identifier?.trim() || null; + const issuePath = company?.issuePrefix && identifier + ? `/${company.issuePrefix}/issues/${identifier}` + : null; + const labels = await issueWriteDenialLabels(req, issue); + const { body } = issueWriteDenialResponse("issue_status_change_live_run", { ...labels, issuePath }); + throw conflict(body.error, { + ...body.details, + issueId: issue.id, + runId: run.id, + requestedStatus, + issuePath, + }); + } + return lock; + } + router.patch( "/issues/:id", (req, _res, next) => { @@ -15144,6 +15229,12 @@ export function issueRoutes( const assigneeWillChange = nextAssigneeAgentId !== existing.assigneeAgentId || nextAssigneeUserId !== existing.assigneeUserId; + const checkedExecutionLock = await assertAgentStatusChangeKeepsLiveRunBound(req, existing, { + requestedStatus: updateFields.status, + assigneeWillChange, + stopsGoalOnCompletion: true, + refreshLock: true, + }); const isAgentReturningIssueToCreator = req.actor.type === "agent" && !!req.actor.agentId && @@ -15344,6 +15435,7 @@ export function issueRoutes( workspaceSelectionActor: req.actor, expectedExecutionPolicy, expectedStatusVersion, + ...(checkedExecutionLock ? { expectedExecutionLock: checkedExecutionLock } : {}), actorAgentId: actor.agentId ?? null, actorRunId: actor.agentId ? actor.runId : null, actorRunStopId: actor.agentId && interruptedRunId === actor.runId ? issueMutationStopId : null, diff --git a/server/src/services/issues.ts b/server/src/services/issues.ts index deef4eb5a50..4fa914fa93b 100644 --- a/server/src/services/issues.ts +++ b/server/src/services/issues.ts @@ -11218,6 +11218,13 @@ export function issueService(db: Db) { workspaceSelectionActor?: AuthorizationActor; expectedExecutionPolicy?: typeof issues.$inferInsert.executionPolicy; expectedStatusVersion?: number; + /** + * The execution lock the caller checked before allowing a status change + * that releases it. A run holding the lock under the row lock that the + * caller did not check (for example a checkout that landed after the + * check) is rejected instead of released; a lock released meanwhile is fine. + */ + expectedExecutionLock?: { executionRunId: string | null; checkoutRunId: string | null }; /** Defaults to `flatten` for parent changes. Board-user routes pass `allow_nested`. */ parentDepthPolicy?: IssueParentDepthPolicy; /** @@ -11310,6 +11317,7 @@ export function issueService(db: Db) { workspaceSelectionActor, expectedExecutionPolicy, expectedStatusVersion, + expectedExecutionLock, parentDepthPolicy: _parentDepthPolicy, expectedParentId: _expectedParentId, // Creation provenance never changes after create. @@ -11584,6 +11592,16 @@ export function issueService(db: Db) { .for("update") .then((rows: Array) => rows[0] ?? null); if (!receiptExisting) return null; + // Only a run the caller did not check is protected. A lock released in + // between (for example by the route's own confirmed Stop) is fine. + if (expectedExecutionLock && ( + (receiptExisting.executionRunId && receiptExisting.executionRunId !== expectedExecutionLock.executionRunId) || + (receiptExisting.checkoutRunId && receiptExisting.checkoutRunId !== expectedExecutionLock.checkoutRunId) + )) { + throw conflict("The task's execution changed while this status change was checked. Read the task again and retry.", { + code: "issue_execution_lock_changed", + }); + } if (issueData.executionPolicy !== undefined) assertQualityReviewPolicyMutation({ previousPolicy: receiptExisting.executionPolicy, nextPolicy: issueData.executionPolicy, actorAgentId, actorUserId }); if ((patch.status ?? receiptExisting.status) === "done" && (receiptExisting.status !== "done" || issueData.executionPolicy !== undefined)) {