From 7f1520c448f26a3394cf2f643806c685adb4d9fb Mon Sep 17 00:00:00 2001 From: Danyang Chen Date: Sun, 8 Nov 2026 12:54:51 +0800 Subject: [PATCH] fix(tasks): refuse a handoff on a finished card A handoff recorded on an accepted, cancelled or failed card still went through requeue because routeToPool only looked at claimedBy and the claimed/working/blocked statuses, so a finished card was written back to open, 0% and handed to the next role. routeToPool now refuses it with the same stale refusal blockForInput makes, before the handoff field or queueRole lands, and the subsystem doc states that rule. Fixes #2667 --- .../2667-task-handoff-terminal.fixed.md | 1 + core/src/host/tasks.js | 11 +- core/test/task-handoff-terminal.test.js | 171 ++++++++++++++++++ docs/subsystems/core-host.md | 4 +- patches/ds-bot/files/src/host/tasks.js | 11 +- .../files/test/task-handoff-terminal.test.js | 171 ++++++++++++++++++ 6 files changed, 363 insertions(+), 6 deletions(-) create mode 100644 changelog.d/2667-task-handoff-terminal.fixed.md create mode 100644 core/test/task-handoff-terminal.test.js create mode 100644 patches/ds-bot/files/test/task-handoff-terminal.test.js diff --git a/changelog.d/2667-task-handoff-terminal.fixed.md b/changelog.d/2667-task-handoff-terminal.fixed.md new file mode 100644 index 000000000..0b2ffdd75 --- /dev/null +++ b/changelog.d/2667-task-handoff-terminal.fixed.md @@ -0,0 +1 @@ +A card that has already finished — `accepted`, `cancelled` or `failed` — now refuses a late `handoff` with `A finished task cannot be handed on` instead of reopening itself as `open`, `not-started` and 0% under the next role, so a card that shows 100% can no longer slide back to the start of the board; the refusal is written the way the same card's "waiting on a question" write already is, and the refused write lands nothing at all, with `claimedBy` and the card's terminal state kept. A `result-ready` card still hands on, because that column is allowed to go back to working ([#2667](https://github.com/ThinkFlowLab/FlyBot/issues/2667)). diff --git a/core/src/host/tasks.js b/core/src/host/tasks.js index 77e13fe54..1db062532 100644 --- a/core/src/host/tasks.js +++ b/core/src/host/tasks.js @@ -505,8 +505,11 @@ export function install(rt) { } if (taskClass !== undefined) task.taskClass = taskClass if (reworkOf !== undefined) task.reworkOf = reworkOf - if (handoff !== undefined) task.handoff = handoff - if (handoff !== undefined) routeToPool(task, handoff) + // The route answers before the field lands, so a refused handoff leaves the card as it was. + if (handoff !== undefined) { + routeToPool(task, handoff) + task.handoff = handoff + } if (evidence !== undefined) task.evidence = evidence if (clearingLease) delete task.lease if (lease !== undefined) task.lease = lease @@ -517,6 +520,10 @@ export function install(rt) { // member is holding hands the card on in the same write, so the producer lets go of it and the // target role can take it with no dispatch from a supervisor. function routeToPool(task, handoff) { + // A finished card keeps its terminal state (#2667): the same refusal `blockForInput` makes, + // because no later write may drag accepted/cancelled/failed back into the queue. result-ready + // is not terminal — it is allowed to go back to working, so a handoff from it still lands. + if (TERMINAL_MATCH.has(task.status)) throw stale('A finished task cannot be handed on') const held = task.claimedBy !== undefined || task.status === 'claimed' || task.status === 'working' || task.status === 'blocked' task.queueRole = handoff.to if (held) requeue(task, `handoff to ${handoff.to}`, 0, 'handed on') diff --git a/core/test/task-handoff-terminal.test.js b/core/test/task-handoff-terminal.test.js new file mode 100644 index 000000000..857380c3e --- /dev/null +++ b/core/test/task-handoff-terminal.test.js @@ -0,0 +1,171 @@ +// #2667: a handoff recorded on a finished card must be refused, not applied. The card state +// machine is one way: accepted, cancelled and failed are terminal and `applyColumn` already +// refuses to move one, but `routeToPool` used to look only at `claimedBy`/status ∈ +// claimed|working|blocked, so a finished card that still carried `claimedBy` (only `requeue` +// clears it) was written back to `open`, 0% and handed to the next role. The refusal is the +// same one `blockForInput` makes — `stale`, before anything lands — and `result-ready` still +// hands on, because that column is allowed to go back to `working`. +import assert from 'node:assert/strict' +import { mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { test } from 'node:test' +import { createRuntime } from '../index.js' +import { columnOf } from '../src/shared/task-board.js' +import { fakeContext } from './fake-ctx.js' + +const team = () => ({ + version: 2, + revision: 1, + mainBotIds: ['a'], + members: { 'owner-1': { id: 'owner-1', createdAt: 1, designatedChiefId: 'a' } }, + bots: { + a: { id: 'a', name: 'Chief', role: 'Lead', sessionId: 'a', createdAt: 1, ownerId: 'owner-1' }, + d1: { id: 'd1', name: 'Dev1', role: 'Dev', sessionId: 'd1', createdAt: 2, ownerId: 'owner-1', createdBy: 'a' }, + d2: { id: 'd2', name: 'Dev2', role: 'Dev', sessionId: 'd2', createdAt: 3, ownerId: 'owner-1', createdBy: 'a' }, + r1: { id: 'r1', name: 'Rev1', role: 'Reviewer', sessionId: 'r1', createdAt: 4, ownerId: 'owner-1', createdBy: 'a' }, + }, + rooms: { + war: { + id: 'war', name: 'War', members: ['a', 'd1', 'd2', 'r1'], admin: 'a', mode: 'everyone', named: true, + sessions: {}, log: [], seq: 0, seen: {}, + participants: [ + { kind: 'human', id: 'owner-1', role: 'member', displayName: 'Alice', joinedSeq: 0, joinedAt: 1 }, + { kind: 'bot', id: 'a', role: 'admin', displayName: 'Chief', joinedSeq: 0, joinedAt: 1 }, + { kind: 'bot', id: 'd1', role: 'member', displayName: 'Dev1', joinedSeq: 0, joinedAt: 1 }, + { kind: 'bot', id: 'd2', role: 'member', displayName: 'Dev2', joinedSeq: 0, joinedAt: 1 }, + { kind: 'bot', id: 'r1', role: 'member', displayName: 'Rev1', joinedSeq: 0, joinedAt: 1 }, + ], + }, + }, + grants: {}, + agents: {}, exchanges: [], exchangeTurns: {}, questions: {}, prefs: {}, +}) + +async function boot(home) { + const ctx = fakeContext() + const agents = new Map() + const agentOf = (sessionId) => { + if (!agents.has(sessionId)) { + const inbox = { + nextStep: [], nextTurn: [], + remove(id) { const keep = message => message.id !== id; this.nextTurn = this.nextTurn.filter(keep); this.nextStep = this.nextStep.filter(keep); return true }, + } + agents.set(sessionId, { + session: { id: sessionId }, id: sessionId, calls: [], status: 'idle', inbox, + followup(message) { this.calls.push(message); this.inbox.nextTurn.push(message) }, + steer() {}, inject() {}, cancel() {}, async whenIdle() {}, + }) + } + return agents.get(sessionId) + } + Object.assign(ctx, { + llm: { listProviders: () => [], listModels: async () => [], resolveModelInfo: async () => ({}) }, + workspaceRegistry: { get: () => undefined, resolveByPath: async () => undefined, create: async path => ({ id: 'w1', path }), archiveSession: async () => {} }, + sessionController: { create: async () => ({ sessionId: 'made-1' }), resolveAgent: async id => ({ agent: agentOf(id) }), selectModel: async () => {}, page: async () => ({ records: [], hasMore: false }) }, + sessionTitle: { rename: async () => {} }, + sessions: { flush: async () => {} }, + }) + const rt = createRuntime(ctx, { home }) + await rt.load() + const track = (sessionId, args) => ctx.registeredTools.get('track_tasks').execute(args ?? {}, { agent: { id: sessionId } }) + const idOf = (body, title) => { + const line = body.split('\n').find(item => item.startsWith('- ') && item.includes(title)) + const id = line?.match(/task-[0-9a-f]+/)?.[0] + assert.ok(id, body) + return id + } + return { ctx, rt, track, idOf, home, statePath: join(home, 'state.json') } +} + +async function start(t) { + const home = mkdtempSync(join(tmpdir(), 'ds-bot-handoff-')) + writeFileSync(join(home, 'state.json'), JSON.stringify(team())) + const app = await boot(home) + t.after(async () => { + try { await app.ctx.dispose() } catch {} + rmSync(home, { recursive: true, force: true, maxRetries: 5 }) + }) + return app +} + +// The card the bug needs: enqueued by the lead, claimed from the queue, so `claimedBy` names the +// member and stays behind after the card is finished (only `requeue` clears it). +async function claimedCard(app, title, member = 'd1') { + const queued = await app.track('a', { tasks: [{ group: 'War', title, queue: true, role: 'Dev' }] }) + const id = app.idOf(queued, title) + const claimed = await app.track(member, { claim: true, group: 'War' }) + assert.match(claimed, new RegExp(`Claimed ${id}`), claimed) + assert.equal(app.rt.state.tasks[id].claimedBy, member) + return id +} + +const handoff = (to = 'Reviewer') => ({ from: 'Dev', to, reason: 'the review is next' }) + +test('a finished card keeps its terminal state and refuses the handoff (#2667)', async (t) => { + const app = await start(t) + const id = await claimedCard(app, 'Ship the release note') + + assert.match(await app.track('d1', { tasks: [{ id, column: 'done' }] }), /done/) + const finished = app.rt.state.tasks[id] + assert.equal(finished.status, 'accepted', 'column done is the accepted state') + assert.equal(columnOf(finished), 'done') + assert.equal(finished.progress, 100) + assert.equal(finished.claimedBy, 'd1', 'a finished card keeps the member that worked it') + + // The reported repro: the next write carries only the handoff, and the card used to come back + // as open/0% under the next role. + const before = readFileSync(app.statePath, 'utf8') + const refused = await app.track('d1', { tasks: [{ id, handoff: handoff() }] }) + assert.match(refused, /A finished task cannot be handed on/, refused) + assert.equal(finished.status, 'accepted') + assert.equal(columnOf(finished), 'done') + assert.equal(finished.progress, 100) + assert.equal(finished.claimedBy, 'd1') + assert.equal(finished.handoff, undefined, 'a refused handoff is not recorded either') + assert.equal(finished.queueRole, 'Dev', 'the refused write does not route the card anywhere') + assert.equal(readFileSync(app.statePath, 'utf8'), before, 'the refused write lands nothing on disk') + + // The same refusal on the HTTP entry (#1720's second path into writeContract). + await assert.rejects( + () => app.rt.taskEndpoint('task-update', { id, handoff: handoff() }), + error => error.code === 'bot/stale' && /A finished task cannot be handed on/.test(error.message), + ) + assert.equal(app.rt.state.tasks[id].status, 'accepted') + assert.equal(app.rt.state.tasks[id].handoff, undefined) +}) + +test('a result-ready card still hands on to the next role (#2667)', async (t) => { + const app = await start(t) + const id = await claimedCard(app, 'Review the diff') + await app.track('d1', { tasks: [{ id, column: 'in-progress' }] }) + await app.rt.taskEndpoint('task-return', { id, summary: 'the diff is ready' }) + assert.equal(app.rt.state.tasks[id].status, 'result-ready') + + const handed = await app.track('d1', { tasks: [{ id, handoff: handoff() }] }) + assert.match(handed, /Review the diff/, handed) + const card = app.rt.state.tasks[id] + assert.equal(card.handoff.to, 'Reviewer') + assert.equal(card.queueRole, 'Reviewer') + assert.equal(card.status, 'open', 'result-ready is not terminal: the card goes back to the queue') + assert.equal(card.progress, 0) + assert.equal(card.claimedBy, undefined) +}) + +test('failed and cancelled cards refuse a handoff too (#2667)', async (t) => { + const app = await start(t) + const cancelled = await claimedCard(app, 'Drop the stale note') + await app.rt.taskEndpoint('task-cancel', { id: cancelled }) + assert.equal(app.rt.state.tasks[cancelled].status, 'cancelled') + + const failedId = await claimedCard(app, 'Publish the broken build', 'd2') + app.rt.taskBotDeleted('d2') + assert.equal(app.rt.state.tasks[failedId].status, 'failed') + + for (const [id, status, writer] of [[cancelled, 'cancelled', 'd1'], [failedId, 'failed', 'd2']]) { + const refused = await app.track(writer, { tasks: [{ id, handoff: handoff() }] }) + assert.match(refused, /A finished task cannot be handed on/, `${status}: ${refused}`) + assert.equal(app.rt.state.tasks[id].status, status) + assert.equal(app.rt.state.tasks[id].handoff, undefined) + } +}) diff --git a/docs/subsystems/core-host.md b/docs/subsystems/core-host.md index 5f62801cb..620c6afcf 100644 --- a/docs/subsystems/core-host.md +++ b/docs/subsystems/core-host.md @@ -600,11 +600,11 @@ A discarded `.catch(() => {})` in the host sources either starts with `void` (fi ## Handoff routing (#1775) -A card carries its own route. `handoff.to` names the role that owns the next step, and that role is the card's pool: `writeContract` calls `routeToPool` (`src/host/tasks.js`) on every handoff it records, so `queueRole` becomes `handoff.to` in the same write. A card created with a handoff starts in that pool (the Chief's one dispatch), and a handoff recorded on a card a member is holding (`claimed`, `working` or `blocked`) hands the card on through `requeue` in the same write — the producer lets go of it, `claimedBy` and the lease are cleared, and the card is `open` in the target pool. Recording a handoff is therefore the whole handover: no supervisor step moves the card afterwards. +A card carries its own route. `handoff.to` names the role that owns the next step, and that role is the card's pool: `writeContract` calls `routeToPool` (`src/host/tasks.js`) on every handoff it records, so `queueRole` becomes `handoff.to` in the same write. A card created with a handoff starts in that pool (the Chief's one dispatch), and a handoff recorded on a card a member is holding (`claimed`, `working` or `blocked`) hands the card on through `requeue` in the same write — the producer lets go of it, `claimedBy` and the lease are cleared, and the card is `open` in the target pool. Recording a handoff is therefore the whole handover: no supervisor step moves the card afterwards. A card that has already finished (`accepted`, `cancelled`, `failed`) refuses the handoff instead — `stale`, `A finished task cannot be handed on`, the same refusal `blockForInput` makes — so a finished card is never dragged back to `open`/0% by a late handoff, and `routeToPool` answers before the field lands, so the refused write leaves the card exactly as it was (`claimedBy` included, since only `requeue` clears it). `result-ready` is not terminal: it may go back to `working`, so a handoff from it still lands (#2667). `task-drive`'s queue pass offers the pool to one idle member of the role. A card with a room is offered to that room's members of the role, as the room queue always was; a card with no room is offered to every local Bot carrying the role, so a chain between roles needs no shared group and no graph. The offer is the existing `Take …` wake, `inflight` and the per-Session hold are unchanged, and a role with no idle member (`holdsCard`, `busy`, or a wake already waiting) leaves the card in the pool: the next pass offers it again, so a card is never lost for want of a worker. -A claim works the same way (#1247's compare-and-set and WIP = 1, unchanged): `track_tasks { claim: true }` without a group takes the highest-priority card of the caller's role whose `handoff.to` matches, and `{ claim: true, id }` takes a named one; a card in another role's pool is refused by name. The target role's own admission is either the claim (accept) or `track_tasks { rework: { task, reason } }` (return): only the member that claimed the card may return it, and the write opens one child card with `taskClass: 'rework'`, `reworkOf` naming the card it fixes and `handoff.to` the producing role — the same `openRework` `task-send-back` uses, now also reachable from the role that received the card. The child is queued for the producer's role, so the producer takes it from its own pool, and the original stays `working` until the rework child is accepted (`closeRework`). A return with no producer role (`handoff.from` missing or `requester`) is refused rather than handed nowhere. Tests: `core/test/task-handoff-route.test.js`. +A claim works the same way (#1247's compare-and-set and WIP = 1, unchanged): `track_tasks { claim: true }` without a group takes the highest-priority card of the caller's role whose `handoff.to` matches, and `{ claim: true, id }` takes a named one; a card in another role's pool is refused by name. The target role's own admission is either the claim (accept) or `track_tasks { rework: { task, reason } }` (return): only the member that claimed the card may return it, and the write opens one child card with `taskClass: 'rework'`, `reworkOf` naming the card it fixes and `handoff.to` the producing role — the same `openRework` `task-send-back` uses, now also reachable from the role that received the card. The child is queued for the producer's role, so the producer takes it from its own pool, and the original stays `working` until the rework child is accepted (`closeRework`). A return with no producer role (`handoff.from` missing or `requester`) is refused rather than handed nowhere. Tests: `core/test/task-handoff-route.test.js`, and `core/test/task-handoff-terminal.test.js` for the finished-card refusal. ## Tests diff --git a/patches/ds-bot/files/src/host/tasks.js b/patches/ds-bot/files/src/host/tasks.js index 77e13fe54..1db062532 100644 --- a/patches/ds-bot/files/src/host/tasks.js +++ b/patches/ds-bot/files/src/host/tasks.js @@ -505,8 +505,11 @@ export function install(rt) { } if (taskClass !== undefined) task.taskClass = taskClass if (reworkOf !== undefined) task.reworkOf = reworkOf - if (handoff !== undefined) task.handoff = handoff - if (handoff !== undefined) routeToPool(task, handoff) + // The route answers before the field lands, so a refused handoff leaves the card as it was. + if (handoff !== undefined) { + routeToPool(task, handoff) + task.handoff = handoff + } if (evidence !== undefined) task.evidence = evidence if (clearingLease) delete task.lease if (lease !== undefined) task.lease = lease @@ -517,6 +520,10 @@ export function install(rt) { // member is holding hands the card on in the same write, so the producer lets go of it and the // target role can take it with no dispatch from a supervisor. function routeToPool(task, handoff) { + // A finished card keeps its terminal state (#2667): the same refusal `blockForInput` makes, + // because no later write may drag accepted/cancelled/failed back into the queue. result-ready + // is not terminal — it is allowed to go back to working, so a handoff from it still lands. + if (TERMINAL_MATCH.has(task.status)) throw stale('A finished task cannot be handed on') const held = task.claimedBy !== undefined || task.status === 'claimed' || task.status === 'working' || task.status === 'blocked' task.queueRole = handoff.to if (held) requeue(task, `handoff to ${handoff.to}`, 0, 'handed on') diff --git a/patches/ds-bot/files/test/task-handoff-terminal.test.js b/patches/ds-bot/files/test/task-handoff-terminal.test.js new file mode 100644 index 000000000..857380c3e --- /dev/null +++ b/patches/ds-bot/files/test/task-handoff-terminal.test.js @@ -0,0 +1,171 @@ +// #2667: a handoff recorded on a finished card must be refused, not applied. The card state +// machine is one way: accepted, cancelled and failed are terminal and `applyColumn` already +// refuses to move one, but `routeToPool` used to look only at `claimedBy`/status ∈ +// claimed|working|blocked, so a finished card that still carried `claimedBy` (only `requeue` +// clears it) was written back to `open`, 0% and handed to the next role. The refusal is the +// same one `blockForInput` makes — `stale`, before anything lands — and `result-ready` still +// hands on, because that column is allowed to go back to `working`. +import assert from 'node:assert/strict' +import { mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { test } from 'node:test' +import { createRuntime } from '../index.js' +import { columnOf } from '../src/shared/task-board.js' +import { fakeContext } from './fake-ctx.js' + +const team = () => ({ + version: 2, + revision: 1, + mainBotIds: ['a'], + members: { 'owner-1': { id: 'owner-1', createdAt: 1, designatedChiefId: 'a' } }, + bots: { + a: { id: 'a', name: 'Chief', role: 'Lead', sessionId: 'a', createdAt: 1, ownerId: 'owner-1' }, + d1: { id: 'd1', name: 'Dev1', role: 'Dev', sessionId: 'd1', createdAt: 2, ownerId: 'owner-1', createdBy: 'a' }, + d2: { id: 'd2', name: 'Dev2', role: 'Dev', sessionId: 'd2', createdAt: 3, ownerId: 'owner-1', createdBy: 'a' }, + r1: { id: 'r1', name: 'Rev1', role: 'Reviewer', sessionId: 'r1', createdAt: 4, ownerId: 'owner-1', createdBy: 'a' }, + }, + rooms: { + war: { + id: 'war', name: 'War', members: ['a', 'd1', 'd2', 'r1'], admin: 'a', mode: 'everyone', named: true, + sessions: {}, log: [], seq: 0, seen: {}, + participants: [ + { kind: 'human', id: 'owner-1', role: 'member', displayName: 'Alice', joinedSeq: 0, joinedAt: 1 }, + { kind: 'bot', id: 'a', role: 'admin', displayName: 'Chief', joinedSeq: 0, joinedAt: 1 }, + { kind: 'bot', id: 'd1', role: 'member', displayName: 'Dev1', joinedSeq: 0, joinedAt: 1 }, + { kind: 'bot', id: 'd2', role: 'member', displayName: 'Dev2', joinedSeq: 0, joinedAt: 1 }, + { kind: 'bot', id: 'r1', role: 'member', displayName: 'Rev1', joinedSeq: 0, joinedAt: 1 }, + ], + }, + }, + grants: {}, + agents: {}, exchanges: [], exchangeTurns: {}, questions: {}, prefs: {}, +}) + +async function boot(home) { + const ctx = fakeContext() + const agents = new Map() + const agentOf = (sessionId) => { + if (!agents.has(sessionId)) { + const inbox = { + nextStep: [], nextTurn: [], + remove(id) { const keep = message => message.id !== id; this.nextTurn = this.nextTurn.filter(keep); this.nextStep = this.nextStep.filter(keep); return true }, + } + agents.set(sessionId, { + session: { id: sessionId }, id: sessionId, calls: [], status: 'idle', inbox, + followup(message) { this.calls.push(message); this.inbox.nextTurn.push(message) }, + steer() {}, inject() {}, cancel() {}, async whenIdle() {}, + }) + } + return agents.get(sessionId) + } + Object.assign(ctx, { + llm: { listProviders: () => [], listModels: async () => [], resolveModelInfo: async () => ({}) }, + workspaceRegistry: { get: () => undefined, resolveByPath: async () => undefined, create: async path => ({ id: 'w1', path }), archiveSession: async () => {} }, + sessionController: { create: async () => ({ sessionId: 'made-1' }), resolveAgent: async id => ({ agent: agentOf(id) }), selectModel: async () => {}, page: async () => ({ records: [], hasMore: false }) }, + sessionTitle: { rename: async () => {} }, + sessions: { flush: async () => {} }, + }) + const rt = createRuntime(ctx, { home }) + await rt.load() + const track = (sessionId, args) => ctx.registeredTools.get('track_tasks').execute(args ?? {}, { agent: { id: sessionId } }) + const idOf = (body, title) => { + const line = body.split('\n').find(item => item.startsWith('- ') && item.includes(title)) + const id = line?.match(/task-[0-9a-f]+/)?.[0] + assert.ok(id, body) + return id + } + return { ctx, rt, track, idOf, home, statePath: join(home, 'state.json') } +} + +async function start(t) { + const home = mkdtempSync(join(tmpdir(), 'ds-bot-handoff-')) + writeFileSync(join(home, 'state.json'), JSON.stringify(team())) + const app = await boot(home) + t.after(async () => { + try { await app.ctx.dispose() } catch {} + rmSync(home, { recursive: true, force: true, maxRetries: 5 }) + }) + return app +} + +// The card the bug needs: enqueued by the lead, claimed from the queue, so `claimedBy` names the +// member and stays behind after the card is finished (only `requeue` clears it). +async function claimedCard(app, title, member = 'd1') { + const queued = await app.track('a', { tasks: [{ group: 'War', title, queue: true, role: 'Dev' }] }) + const id = app.idOf(queued, title) + const claimed = await app.track(member, { claim: true, group: 'War' }) + assert.match(claimed, new RegExp(`Claimed ${id}`), claimed) + assert.equal(app.rt.state.tasks[id].claimedBy, member) + return id +} + +const handoff = (to = 'Reviewer') => ({ from: 'Dev', to, reason: 'the review is next' }) + +test('a finished card keeps its terminal state and refuses the handoff (#2667)', async (t) => { + const app = await start(t) + const id = await claimedCard(app, 'Ship the release note') + + assert.match(await app.track('d1', { tasks: [{ id, column: 'done' }] }), /done/) + const finished = app.rt.state.tasks[id] + assert.equal(finished.status, 'accepted', 'column done is the accepted state') + assert.equal(columnOf(finished), 'done') + assert.equal(finished.progress, 100) + assert.equal(finished.claimedBy, 'd1', 'a finished card keeps the member that worked it') + + // The reported repro: the next write carries only the handoff, and the card used to come back + // as open/0% under the next role. + const before = readFileSync(app.statePath, 'utf8') + const refused = await app.track('d1', { tasks: [{ id, handoff: handoff() }] }) + assert.match(refused, /A finished task cannot be handed on/, refused) + assert.equal(finished.status, 'accepted') + assert.equal(columnOf(finished), 'done') + assert.equal(finished.progress, 100) + assert.equal(finished.claimedBy, 'd1') + assert.equal(finished.handoff, undefined, 'a refused handoff is not recorded either') + assert.equal(finished.queueRole, 'Dev', 'the refused write does not route the card anywhere') + assert.equal(readFileSync(app.statePath, 'utf8'), before, 'the refused write lands nothing on disk') + + // The same refusal on the HTTP entry (#1720's second path into writeContract). + await assert.rejects( + () => app.rt.taskEndpoint('task-update', { id, handoff: handoff() }), + error => error.code === 'bot/stale' && /A finished task cannot be handed on/.test(error.message), + ) + assert.equal(app.rt.state.tasks[id].status, 'accepted') + assert.equal(app.rt.state.tasks[id].handoff, undefined) +}) + +test('a result-ready card still hands on to the next role (#2667)', async (t) => { + const app = await start(t) + const id = await claimedCard(app, 'Review the diff') + await app.track('d1', { tasks: [{ id, column: 'in-progress' }] }) + await app.rt.taskEndpoint('task-return', { id, summary: 'the diff is ready' }) + assert.equal(app.rt.state.tasks[id].status, 'result-ready') + + const handed = await app.track('d1', { tasks: [{ id, handoff: handoff() }] }) + assert.match(handed, /Review the diff/, handed) + const card = app.rt.state.tasks[id] + assert.equal(card.handoff.to, 'Reviewer') + assert.equal(card.queueRole, 'Reviewer') + assert.equal(card.status, 'open', 'result-ready is not terminal: the card goes back to the queue') + assert.equal(card.progress, 0) + assert.equal(card.claimedBy, undefined) +}) + +test('failed and cancelled cards refuse a handoff too (#2667)', async (t) => { + const app = await start(t) + const cancelled = await claimedCard(app, 'Drop the stale note') + await app.rt.taskEndpoint('task-cancel', { id: cancelled }) + assert.equal(app.rt.state.tasks[cancelled].status, 'cancelled') + + const failedId = await claimedCard(app, 'Publish the broken build', 'd2') + app.rt.taskBotDeleted('d2') + assert.equal(app.rt.state.tasks[failedId].status, 'failed') + + for (const [id, status, writer] of [[cancelled, 'cancelled', 'd1'], [failedId, 'failed', 'd2']]) { + const refused = await app.track(writer, { tasks: [{ id, handoff: handoff() }] }) + assert.match(refused, /A finished task cannot be handed on/, `${status}: ${refused}`) + assert.equal(app.rt.state.tasks[id].status, status) + assert.equal(app.rt.state.tasks[id].handoff, undefined) + } +})