mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-11 05:11:49 +01:00
fix(issues): refuse agent status changes that detach another run's live execution (#20271)
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - A task that an agent run executes holds a checkout lock and an execution lock (`checkoutRunId`, `executionRunId`) > - A status change away from `in_progress` clears these locks. It does not stop the run that holds them > - Agents cannot stop another agent's run: run cancel and the task-tree pause are board-only > - But an agent that can edit a task (for example a lead with the manage-active-checkouts grant over its report) can change its status. The run then keeps running and is no longer bound to its task > - In chat evals, a lead "paused" a launch tree. It moved running subtasks to backlog. The runs kept running, detached > - This pull request refuses an agent's status change when a different queued or running run holds the task's lock. The denial is final, links the task, and names the person's controls > - The benefit is that agents cannot interrupt another agent's run through a status edit, and runs are not left orphaned ## Linked Issues or Issue Description Refs #20261 (chat guidance sends a task-tree pause to the person's Pause subtree control). Refs #19394 (resume restarts the work a subtree pause interrupted). **What happened** An agent changed the status of a task while another agent's run executed it. The request succeeded. The server cleared `checkoutRunId` and `executionRunId`. The run continued, but it was no longer bound to the task. In the chat evals `steering.stop-versus-pause` and `task-coordination.pause-tree`, a lead moved running subtasks to backlog to "pause" a launch. Their runs kept running, detached. **Expected behavior** An agent cannot get the effect of interrupting another agent's run. When a different live run holds the task's lock, the agent's status change is refused before any write. The refusal says it is final. It sends the person to the task and names the controls that really stop or hold the work: the run's Stop or Interrupt, or More task actions, then Pause subtree on the parent task. The lock-holding run, board users, and paths that stop the run themselves continue to work. **Steps to reproduce** 1. Create agent A and agent B, with B reporting to A (A gets `tasks:manage_active_checkouts` through the manager chain). 2. Give B an `in_progress` task. Set `checkoutRunId` and `executionRunId` to a `running` heartbeat run of B. 3. As A (agent JWT with its own run), send `PATCH /api/issues/:id` with `{ "status": "backlog" }`. 4. Before this change: `200`, the status is `backlog`, both lock fields are `null`, and B's run is still `running`. ## What Changed - `server/src/routes/issues.ts`: new `assertAgentStatusChangeKeepsLiveRunBound`. For an agent actor, it refuses a status change when `executionRunId` or `checkoutRunId` names a different run in the same company that is `queued` or `running`. It runs in `PATCH /issues/:id` after the final status and assignee are known (execution-policy transitions included) and before any run stop or write. It also runs in recovery resolution when the source status changes. - The guard applies to any status other than `in_progress`, also to the current status, because the service clears the locks for those too. - The write is conditional on the lock the guard checked. The route re-reads the lock and passes it to `svc.update` as `expectedExecutionLock`. The service compares it under the row lock and returns `409 issue_execution_lock_changed` if a checkout landed in between. - These cases stay allowed: the lock-holding run itself, board users, requests with no status, a change to `in_progress`, `cancelled` (the route cancels the run), reassignment (the route stops the run first), `done` through PATCH while a Runner goal is active (terminalization pauses the goal and stops the run; recovery resolution does not get this exception), and a lock holder that already finished. - `packages/shared/src/issue-write-denial.ts`: new denial code `issue_status_change_live_run` (409). The copy says the refusal is final, links the task as `[identifier](/prefix/issues/identifier)`, and names Stop or Interrupt and More task actions, then Pause subtree. A new optional `issuePath` context field carries the link. The UI notice renders it through the existing generic path. - `doc/SPEC-implementation.md`: documents the rule in the checkout contract section. - Paths checked with no change needed: there is no bulk issue update route; tree holds and run cancel are board-only; `POST /issues/:id/release` already lets only the assignee's checkout run release; API-catalog actions call the same PATCH route; MCP status changes already refuse with `MCP_ACTIVE_EXECUTION`. ## Verification - Red on master, green with this change: - `server/src/__tests__/issue-stale-execution-lock-routes.test.ts` (embedded Postgres, real issue service): a manager moves its report's running task to backlog. On master: `200`, status `backlog`, `checkoutRunId`/`executionRunId` changed to `null`. With the fix: `409 issue_status_change_live_run`, row unchanged, run still `running`. - `server/src/__tests__/issue-agent-mutation-ownership-routes.test.ts`: refusals for `backlog`, `todo`, `blocked`, `in_review`, `done`, a queued lock holder, and a repeated current status (red on master). Allowed cases: field edits, the lock-holding run, board, `cancelled` (run is cancelled), and finished lock holders; an allowed change carries `expectedExecutionLock`. - Same embedded-Postgres file: `svc.update` with an `expectedExecutionLock` that differs from the row returns `409 issue_execution_lock_changed` and leaves the row unchanged. - Also passed locally: `issue-closed-workspace-routes`, `issue-comment-reopen-routes`, `issue-recovery-actions`, `issue-scheduled-retry-routes`, `low-trust-red-team-routes`, `issue-update-comment-wakeup-routes`, `external-object-routes`, `packages/shared` `issue-write-denial`, `ui` `IssueWriteDenialNotice`. - `tsc --noEmit` shows no errors in the changed files. Full typecheck, build and test are left to CI. ## Risks - Behavior change for agents: a lead or CEO that used status edits to take over a report's running task now gets a 409. The supported paths (comment, cancel, reassign, or the person's Stop or Pause subtree) still work. - A queued run that holds the lock also blocks the change. This is deliberate: the queued run would otherwise start later with no task bound to it. - Agents can still cancel another agent's run by cancelling its task. That is existing, documented behavior ("task cancellation cancels the intent"), and this PR does not change it. ## Model Used - Anthropic Claude Opus 5.5 (`claude-opus-5-5`), via Claude Code with tool use (shell, file edits, test execution). ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [ ] All Paperclip CI gates are green - [ ] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
1 parent
6459488984
commit
1b09920a64
6 files changed
+389
No files matched your search
@@ -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
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -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";
|
||||
|
||||
@@ -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");
|
||||
});
|
||||
});
|
||||
@@ -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,
|
||||
|
||||
@@ -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<typeof issues.$inferSelect>) => 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)) {
|
||||
|
||||
Reference in new issue
Block a user