Refuse a handoff recorded on a finished task card

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)).

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')

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)
+  }
+})

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 <id> …` 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

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')