diff --git a/changelog.d/1720-typed-task-contract.added.md b/changelog.d/1720-typed-task-contract.added.md new file mode 100644 index 00000000..d45c1492 --- /dev/null +++ b/changelog.d/1720-typed-task-contract.added.md @@ -0,0 +1 @@ +A task card now carries a typed contract: `taskClass` (`work`, `rework`, `release`, `triage`, `meeting`), `reworkOf` for a card that fixes a rejected one, `handoff` naming the roles it moved between and why, `evidence` for the sha/PR/check pointers that prove the work, and `lease` for the claim on it. Every field is validated on write and refused by name, a review rejection opens exactly one rework card beside the original instead of flipping its status, an expired lease returns a queued card to the queue while a live one refuses a second claim naming the holder, and the Project board reads the same row without a second copy. A card stored before this slice reads back unchanged ([#1720](https://github.com/ThinkFlowLab/FlyBot/issues/1720)). diff --git a/core/src/host/project-board.js b/core/src/host/project-board.js index 78db0acc..f2892a42 100644 --- a/core/src/host/project-board.js +++ b/core/src/host/project-board.js @@ -60,6 +60,14 @@ export function install(rt) { column: columnOf(task), assignee: task.assignee ?? null, updatedAt: task.updatedAt ?? task.createdAt ?? null, + // The typed contract (#1720) is read from the same row, not a second copy: a board that + // shows a rework or a held lease shows what the task store wrote. A record stored before + // the slice carries none of the keys and reads back exactly as it did. + ...(task.taskClass ? { taskClass: task.taskClass } : {}), + ...(task.reworkOf ? { reworkOf: task.reworkOf } : {}), + ...(task.handoff && typeof task.handoff === 'object' ? { handoff: task.handoff } : {}), + ...(Array.isArray(task.evidence) ? { evidence: task.evidence } : {}), + ...(task.lease && typeof task.lease === 'object' ? { lease: task.lease } : {}), ...(fromRun ? { runId: ref.runId, ...(ref.nodeId ? { nodeId: ref.nodeId } : {}) } : {}), }) } diff --git a/core/src/host/tasks.js b/core/src/host/tasks.js index 4b94494c..10bd7901 100644 --- a/core/src/host/tasks.js +++ b/core/src/host/tasks.js @@ -353,7 +353,10 @@ export function install(rt) { transition(task, next, actor, extra) if (next === 'result-ready') task.result = { kind: 'summary', summary: (task.note || 'Completed').slice(0, 2000) } } - if (target === 'accepted') settleOverall(task) + if (target === 'accepted') { + settleOverall(task) + closeRework(task, actor) + } return true } @@ -377,6 +380,178 @@ export function install(rt) { return moved } + // ---------------------------------------------------------------- the typed contract (#1720) + // Five additive fields, none of which changes an existing one: `taskClass` (the typed kind, + // beside the group-card `kind`), `reworkOf` (a rework child's link to the card it reworks, + // beside the part-of-overall `parentId`), `handoff`, `evidence` and `lease`. Every one is + // validated by name — a refusal names the field, never a silently dropped value. + const TASK_CLASSES = ['work', 'rework', 'release', 'triage', 'meeting'] + const HANDOFF_REASON_MAX = 300 + const EVIDENCE_MAX = 8 + const LEASE_TTL_SEC = 30 * 60 + // A role is known when a Bot carries it; `requester` is the human side of a handoff, which no + // Bot record names. Anything else is refused with the role quoted back (#1720). + const RESERVED_ROLES = new Set(['requester']) + const knownRole = role => RESERVED_ROLES.has(role) || Object.values(state.bots ?? {}).some(bot => Boolean(bot?.role) && sameRole(bot.role, role)) + const boundedLine = (value, max, name) => { + const text = String(value ?? '').replace(/[\u0000-\u001f\u007f]/g, ' ').replace(/\s+/g, ' ').trim() + if (text.length > max) throw invalid(`${name} is at most ${max} characters`) + return text + } + const readTaskClass = (value) => { + if (value === undefined || value === null || value === '') return undefined + const taskClass = String(value) + if (!TASK_CLASSES.includes(taskClass)) throw invalid(`taskClass must be one of ${TASK_CLASSES.join(', ')}`) + return taskClass + } + const readReworkOf = (value) => { + if (value === undefined || value === null || String(value).trim() === '') return undefined + const id = boundedLine(value, 80, 'reworkOf') + if (find(id) === undefined) throw invalid(`reworkOf names no task: ${id}`) + return id + } + const readStampOf = (value, name) => { + if (typeof value === 'number' && Number.isFinite(value)) return value + const parsed = typeof value === 'string' && value.trim() !== '' ? Date.parse(value) : Number.NaN + if (!Number.isFinite(parsed)) throw invalid(`${name} must be a time (milliseconds or an ISO date)`) + return parsed + } + const readHandoff = (value) => { + if (value === undefined || value === null) return undefined + if (typeof value !== 'object' || Array.isArray(value)) throw invalid('handoff must be an object with from, to, at and reason') + const from = boundedLine(value.from, ROLE_MAX, 'handoff.from') + const to = boundedLine(value.to, ROLE_MAX, 'handoff.to') + if (from === '') throw invalid('handoff needs the role it came from (from)') + if (to === '') throw invalid('handoff needs the role it goes to (to)') + if (!knownRole(from)) throw refuse('unknown-role', `handoff.from names the role "${from}", which no Bot carries`) + if (!knownRole(to)) throw refuse('unknown-role', `handoff.to names the role "${to}", which no Bot carries`) + const reason = boundedLine(value.reason, HANDOFF_REASON_MAX, 'handoff.reason') + const at = value.at === undefined || value.at === null ? stamp() : readStampOf(value.at, 'handoff.at') + return { from, to, at, reason } + } + const readEvidence = (value) => { + if (value === undefined || value === null) return undefined + if (!Array.isArray(value)) throw invalid('evidence must be a list of at most 8 entries of { sha, pr, check, at }') + if (value.length > EVIDENCE_MAX) throw invalid(`evidence carries at most ${EVIDENCE_MAX} entries`) + return value.map((entry) => { + if (entry === null || typeof entry !== 'object' || Array.isArray(entry)) throw invalid('each evidence entry is an object of { sha, pr, check, at }') + const sha = boundedLine(entry.sha, 64, 'evidence.sha') + const check = boundedLine(entry.check, 200, 'evidence.check') + let pr + if (entry.pr !== undefined && entry.pr !== null && entry.pr !== '') { + pr = typeof entry.pr === 'number' ? entry.pr : Number(entry.pr) + if (!Number.isInteger(pr) || pr < 1) throw invalid('evidence.pr must be a pull request number') + } + const at = entry.at === undefined || entry.at === null ? undefined : readStampOf(entry.at, 'evidence.at') + if (sha === '' && pr === undefined && check === '' && at === undefined) throw invalid('an evidence entry needs a sha, a pr, a check or an at') + return { sha, ...(pr === undefined ? {} : { pr }), check, ...(at === undefined ? {} : { at }) } + }) + } + const readLease = (value) => { + if (value === undefined || value === null) return undefined + if (typeof value !== 'object' || Array.isArray(value)) throw invalid('lease must be an object with holder, takenAt and expiresAt') + const holder = boundedLine(value.holder, 200, 'lease.holder') + if (holder === '') throw invalid('lease needs a holder') + const takenAt = readStampOf(value.takenAt, 'lease.takenAt') + const expiresAt = readStampOf(value.expiresAt, 'lease.expiresAt') + if (expiresAt <= takenAt) throw invalid('lease.expiresAt must be after lease.takenAt') + return { holder, takenAt, expiresAt } + } + // One write path for the five fields (#1720): the tool, the conversation path and the HTTP + // endpoints all reach it, so a refusal is the same refusal wherever the value came from. The + // reads happen before any assignment, so a refused write leaves the card exactly as it was. + function writeContract(task, body = {}) { + const taskClass = readTaskClass(body.taskClass) + const reworkOf = readReworkOf(body.reworkOf) + const handoff = readHandoff(body.handoff) + const evidence = readEvidence(body.evidence) + const clearingLease = body.lease === null + const lease = readLease(body.lease) + if (taskClass !== undefined) task.taskClass = taskClass + if (reworkOf !== undefined) task.reworkOf = reworkOf + if (handoff !== undefined) task.handoff = handoff + if (evidence !== undefined) task.evidence = evidence + if (clearingLease) delete task.lease + if (lease !== undefined) task.lease = lease + return { taskClass, reworkOf, handoff, evidence, lease } + } + // The typed contract on a card that is created here rather than written by a caller. + const defaultContract = (task) => { + task.taskClass = 'work' + return task + } + const leaseOf = task => (task?.lease !== null && typeof task?.lease === 'object' ? task.lease : undefined) + const leaseLive = (task, now = Date.now()) => { + const lease = leaseOf(task) + return lease !== undefined && Number.isFinite(lease.expiresAt) && lease.expiresAt > now + } + const holderName = id => botRecord(id)?.name ?? String(id ?? '') + // An expired lease returns a queued card to the queue (#1720): the member holding it is gone + // or stalled, so the item is offered again instead of staying claimed for ever. A card that + // is not a queued item keeps its status; only the dead lease is dropped. + function expireLeases(now = Date.now()) { + let expired = 0 + for (const task of Object.values(store())) { + const lease = leaseOf(task) + if (lease === undefined || !Number.isFinite(lease.expiresAt) || lease.expiresAt > now) continue + if (!task.queueRole || (task.status !== 'claimed' && task.status !== 'working')) { + delete task.lease + continue + } + requeue(task, 'lease expired', 0) + delete task.lease + expired += 1 + } + return expired + } + // A rejection is a handover, not a status flip (#1720): the original's `handoff` names the + // roles, and the rework is one child card linked by `reworkOf` — never `parentId`, which + // means "part of an overall card" and must never make `settleOverall` fire. A replay of the + // same rejection reuses the child that is still open instead of creating a second one. + function openRework(original, note) { + const reason = boundedLine(note, HANDOFF_REASON_MAX, 'handoff.reason') + const to = oneLine(botRecord(original.ownerBotId)?.role ?? '', ROLE_MAX) || 'requester' + const from = 'requester' + const at = stamp() + original.handoff = { from, to, at, reason } + const existing = Object.values(store()).find(task => task.reworkOf === original.id && openTask(task)) + if (existing !== undefined) { + existing.handoff = { from, to, at, reason } + existing.actors = [...(existing.actors ?? []), { status: existing.status, at, by: 'rework replayed' }] + existing.updatedAt = at + return existing + } + const child = { + id: `task-${randomBytes(8).toString('hex')}`, + status: 'open', + taskClass: 'rework', + reworkOf: original.id, + createdAt: at, + updatedAt: at, + requesterId: original.requesterId ?? null, + ownerBotId: original.ownerBotId, + title: oneLine(`${original.title} (rework)`, 300), + note: reason, + source: 'rework', + handoff: { from, to, at, reason }, + origin: { ...(original.origin ?? {}) }, + actors: [{ status: 'open', at, by: 'rework opened' }], + } + store()[child.id] = child + return child + } + // The one write that returns the original to `result-ready` (#1720): it runs where a task is + // accepted, and the guard is the original still being `working`, so accepting a rework child + // twice moves the original once and a replay after a restart is a no-op. + function closeRework(child, actor) { + if (child?.taskClass !== 'rework' || typeof child.reworkOf !== 'string') return false + const original = find(child.reworkOf) + if (original === undefined || original.status !== 'working') return false + transition(original, 'result-ready', actor, { by: 'rework accepted' }) + original.result = { kind: 'summary', summary: 'Rework accepted' } + return true + } + function applyFields(task, body) { const title = body.title !== undefined ? readTitle(body.title) : undefined if (title !== undefined) { @@ -472,6 +647,7 @@ export function install(rt) { delete task.inputApprovalIds delete task.approvalId delete task.claimedBy + delete task.lease task.status = 'open' task.ownerBotId = task.queueBy ?? task.ownerBotId task.progress = 0 @@ -598,6 +774,7 @@ export function install(rt) { origin: { roomId: room.id }, actors: [{ status: 'open', at, by: 'dispatch', ...(by ? { botId: by } : {}) }], } + defaultContract(task) store()[task.id] = task await save() return view(task) @@ -610,6 +787,7 @@ export function install(rt) { const room = resolveRoom(botId, body.group, roomId) const entry = room ? state.rooms?.[room] : undefined if (!entry) throw invalid('A claim needs a group') + if (expireLeases() > 0) await rt.save() const myRole = oneLine(botRecord(botId)?.role ?? '', ROLE_MAX) if (myRole === '') throw refuse('not-in-role', 'this Bot has no role to claim with') // WIP = 1: one card in progress fills the member's slot. A blocked card waits on a person, @@ -624,7 +802,12 @@ export function install(rt) { if (wantedId !== undefined) { pick = find(wantedId) if (pick === undefined) throw stale(`no task ${wantedId}`) - if (!unclaimed(pick) || pick.origin?.roomId !== entry.id) throw stale(`no queued item ${wantedId} in this group`) + if (!unclaimed(pick) || pick.origin?.roomId !== entry.id) { + if (leaseLive(pick)) { + throw refuse('lease-held', `task ${pick.id} is held by ${holderName(pick.lease.holder)} until ${new Date(pick.lease.expiresAt).toISOString()}`) + } + throw stale(`no queued item ${wantedId} in this group`) + } if (!sameRole(pick.queueRole, myRole)) throw refuse('not-in-role', `that item is queued for ${pick.queueRole}, and this Bot is ${myRole}`) if (!offered(pick)) return { item: null, reason: 'waiting' } } else { @@ -643,6 +826,8 @@ export function install(rt) { live.ownerBotId = botId delete live.retryAt delete live.queueReason + const claimedAt = Date.now() + live.lease = { holder: botId, takenAt: claimedAt, expiresAt: claimedAt + LEASE_TTL_SEC * 1000 } transition(live, 'claimed', null, { by: 'claimed from the queue' }) announceClaim(live) await save() @@ -699,6 +884,7 @@ export function install(rt) { origin: { ...(roomId ? { roomId } : {}), ...(origin.sessionId ? { sessionId: String(origin.sessionId) } : {}) }, actors: [{ status: 'open', at, by: 'conversation', ...(actor?.memberId ? { memberId: actor.memberId } : {}) }], } + defaultContract(task) store()[task.id] = task return task } @@ -795,6 +981,14 @@ export function install(rt) { throw stale('This task is waiting on an answer. Ask, then pass answer with what was said.') } if (title && task.title !== title) task.title = title + // A refused value must not leave a half-created card behind: the writer validates before + // it assigns, and a new row it refuses is removed again (#1720). + try { + writeContract(task, item) + } catch (error) { + if (isNew) delete store()[task.id] + throw error + } const { progress } = applyFields(task, item) if (!task.source) task.source = 'conversation' const before = task.updatedAt @@ -990,6 +1184,8 @@ export function install(rt) { origin: { ...(roomId ? { roomId } : {}), ...(body.sessionId ? { sessionId: String(body.sessionId) } : {}) }, actors: [{ status: 'open', at: stamp(), ...(actor?.memberId ? { memberId: actor.memberId } : {}) }], } + defaultContract(task) + writeContract(task, body) const { progress } = applyFields(task, body) const column = readColumn(body.column) applyColumn(task, column, actor) @@ -1004,6 +1200,7 @@ export function install(rt) { const task = find(body.id) if (task === undefined) throw stale(`no task ${String(body.id ?? '')}`) if (!mayUpdate(task, actor)) throw forbidden('Only the requester or the Bot\'s owner may update this task') + writeContract(task, body) const { progress } = applyFields(task, body) const column = readColumn(body.column) const before = task.updatedAt @@ -1073,6 +1270,7 @@ export function install(rt) { requireRequester(task, actor) transition(task, 'accepted', actor) settleOverall(task) + closeRework(task, actor) requestWriteBack(task, 'accepted', undefined, actor) await save() return view(task) @@ -1086,6 +1284,7 @@ export function install(rt) { if (note === '') throw invalid('Send back needs a note for the Bot') transition(task, 'working', actor, { by: 'sent back' }) task.followUp = { note, at: task.updatedAt } + openRework(task, note) requestWriteBack(task, 'working', note, actor) await save() return view(task) diff --git a/core/src/host/tools.js b/core/src/host/tools.js index 3d5a21b5..51f6f3ec 100644 --- a/core/src/host/tools.js +++ b/core/src/host/tools.js @@ -933,6 +933,43 @@ ${task.writeBack.body}`) ask: { type: 'string', description: 'The question you need before this task can continue. The task waits. Call ask_user in your own chat. For a group card, post one short question in the group only when a person has to decide.' }, answer: { type: 'string', description: 'What the user or the group answered. Pass it to resume a task that was waiting.' }, target: { type: 'string', description: 'Where a decision on this task is written back: a GitHub issue or pull request, as https://github.com/owner/repo/issues/12 or owner/repo#12. Optional.' }, + taskClass: { type: 'string', enum: ['work', 'rework', 'release', 'triage', 'meeting'], description: 'The typed class of the card. Work is the default; a rework card fixes a card that was rejected.' }, + reworkOf: { type: 'string', description: 'The id of the card this rework card fixes. A rework card is not a part of a group card, so pass reworkOf, never parent.' }, + handoff: { + type: 'object', + description: 'Who handed this card to whom, why and when. Both roles must be roles a Bot carries (or requester).', + properties: { + from: { type: 'string', description: 'The role the card came from.' }, + to: { type: 'string', description: 'The role the card went to.' }, + at: { type: 'number', description: 'When, in milliseconds since the epoch. Defaults to now.' }, + reason: { type: 'string', description: 'One line, at most 300 characters.' }, + }, + additionalProperties: false, + }, + evidence: { + type: 'array', + description: 'Up to 8 pointers to what proves the work: a sha, a pull request, a check name, when.', + items: { + type: 'object', + additionalProperties: false, + properties: { + sha: { type: 'string' }, + pr: { type: 'integer' }, + check: { type: 'string' }, + at: { type: 'number', description: 'When, in milliseconds since the epoch.' }, + }, + }, + }, + lease: { + type: 'object', + description: 'The claim held on this card: who holds it and when that claim lapses.', + properties: { + holder: { type: 'string' }, + takenAt: { type: 'number', description: 'When the claim was taken, in milliseconds since the epoch.' }, + expiresAt: { type: 'number', description: 'When the claim lapses, in milliseconds since the epoch.' }, + }, + additionalProperties: false, + }, postResult: { type: 'object', description: 'How the decision comment a task asked you to post went. Pass task (its id) and ok; when ok is false, pass the error. One report per decision, and no retrying on its own.', diff --git a/core/test/task-contract.test.js b/core/test/task-contract.test.js new file mode 100644 index 00000000..88c20d87 --- /dev/null +++ b/core/test/task-contract.test.js @@ -0,0 +1,251 @@ +// The typed Task/Handoff contract (#1720): five additive fields on the one task record — +// `taskClass`, `reworkOf`, `handoff`, `evidence`, `lease` — validated on write and read by the +// Project board from the same row. The rework child is a card linked by `reworkOf` (never +// `parentId`), a lease returns an abandoned queued card to the queue, and a record stored before +// the slice still reads back and renders. +import assert from 'node:assert/strict' +import { mkdtempSync, 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 { install as installProjectBoard } from '../src/host/project-board.js' +import { formatRoomBoard } from '../src/host/room-context.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 + } + const create = async (spec = {}) => { + const body = await track(spec.bot ?? 'd1', { tasks: [{ group: 'War', ...spec }] }) + return { body, id: idOf(body, spec.title) } + } + return { ctx, rt, track, idOf, create, home, agents } +} + +async function start(t, seed = team()) { + const home = mkdtempSync(join(tmpdir(), 'ds-bot-contract-')) + writeFileSync(join(home, 'state.json'), JSON.stringify(seed)) + const app = await boot(home) + app.stop = async () => rmSync(home, { recursive: true, force: true, maxRetries: 5 }) + t.after(async () => { + try { await app.ctx.dispose() } catch {} + await app.stop() + }) + return app +} + +// A restart is the same home and a fresh runtime: the state on disk is what the new process reads. +async function restart(app) { + await app.ctx.dispose() + return boot(app.home) +} + +const childrenOf = (app, originalId) => Object.values(app.rt.state.tasks).filter(task => task.reworkOf === originalId) +const byActor = (task, why) => (task.actors ?? []).filter(entry => entry.by === why) + +test('a handoff records from/to/reason and an unknown role is refused by name (#1720)', async (t) => { + const app = await start(t) + const { id } = await app.create({ title: 'Hand the diff on', handoff: { from: 'Lead', to: 'Dev', at: 1_700_000_000_000, reason: 'needs the test' } }) + const task = app.rt.state.tasks[id] + assert.equal(task.taskClass, 'work', 'a card created without a class is a work card') + assert.deepEqual({ ...task.handoff }, { from: 'Lead', to: 'Dev', at: 1_700_000_000_000, reason: 'needs the test' }) + + const refused = await app.track('d1', { tasks: [{ group: 'War', title: 'Another card', handoff: { from: 'Lead', to: 'Nobody', reason: 'x' } }] }) + assert.match(refused, /unknown-role/) + assert.match(refused, /Nobody/) + assert.equal(Object.values(app.rt.state.tasks).some(task => task.title === 'Another card'), false, 'a refused write leaves no half-created card') + + // The field name is the refusal: a bad class is not silently dropped either (the HTTP + // endpoint carries a value the tool schema would already refuse). + await assert.rejects( + () => app.rt.taskEndpoint('task-create', { botId: 'd1', title: 'Third card', taskClass: 'kind' }), + /taskClass must be one of work, rework, release, triage, meeting/, + ) +}) + +test('a rejection creates exactly one rework child with reworkOf and no parentId (#1720)', async (t) => { + const app = await start(t) + const { id } = await app.create({ title: 'Fix the flaky test' }) + assert.match(await app.track('d1', { tasks: [{ group: 'War', id, column: 'in-progress' }] }), /Fix the flaky test/) + await app.rt.taskEndpoint('task-return', { id, summary: 'done, see the diff' }) + await app.rt.taskEndpoint('task-send-back', { id, note: 'the diff has no test' }) + + const original = app.rt.state.tasks[id] + assert.equal(original.status, 'working', 'the original goes back to working') + assert.equal(original.handoff.from, 'requester') + assert.equal(original.handoff.to, 'Dev') + assert.equal(original.handoff.reason, 'the diff has no test') + const children = childrenOf(app, id) + assert.equal(children.length, 1) + assert.equal(children[0].taskClass, 'rework') + assert.equal(children[0].reworkOf, id) + assert.equal(children[0].parentId, undefined, 'a rework card is not part of an overall card') + assert.equal(children[0].ownerBotId, 'd1') + + // The rejection replayed on the same card: a returning result is a second rejection, and it + // reuses the child it already opened instead of minting a second one. + await app.rt.taskEndpoint('task-return', { id, summary: 'done again' }) + await app.rt.taskEndpoint('task-send-back', { id, note: 'still no test' }) + assert.equal(childrenOf(app, id).length, 1, 'one rework child per rejected card') +}) + +test('a rejection replayed after a restart adds no child and returns the original once (#1720)', async (t) => { + const app = await start(t) + const { id } = await app.create({ title: 'Fix the flaky test' }) + await app.track('d1', { tasks: [{ group: 'War', id, column: 'in-progress' }] }) + await app.rt.taskEndpoint('task-return', { id, summary: 'done' }) + await app.rt.taskEndpoint('task-send-back', { id, note: 'the diff has no test' }) + const childId = childrenOf(app, id)[0].id + await app.rt.taskEndpoint('task-return', { id, summary: 'reworked' }) + await app.rt.save() + + const again = await restart(app) + const { rt } = again + assert.equal(childrenOf(again, id).length, 1, 'the restart reads the same one child') + // The same rejection lands again: the open child is reused, never duplicated. + await rt.taskEndpoint('task-send-back', { id, note: 'the same rejection' }) + assert.equal(childrenOf(again, id).length, 1) + assert.equal(childrenOf(again, id)[0].id, childId) + + // The child's acceptance is the single write that returns the original to result-ready. + await rt.taskEndpoint('task-update', { id: childId, column: 'done' }) + assert.equal(rt.state.tasks[id].status, 'result-ready') + assert.equal(byActor(rt.state.tasks[id], 'rework accepted').length, 1) + // A replayed acceptance (the endpoint retried) is not a second transition. + await assert.rejects(() => rt.taskEndpoint('task-accept-result', { id: childId })) + assert.equal(rt.state.tasks[id].status, 'result-ready') + assert.equal(byActor(rt.state.tasks[id], 'rework accepted').length, 1, 'the original moves once') +}) + +test('an expired lease requeues and a live lease refuses a second claim naming the holder (#1720)', async (t) => { + const app = await start(t) + const body = await app.track('a', { tasks: [{ group: 'War', queue: true, role: 'Dev', title: 'Ship the note' }] }) + const id = app.idOf(body, 'Ship the note') + assert.match(await app.track('d1', { claim: true, group: 'War' }), new RegExp(`^Claimed ${id} `)) + const held = app.rt.state.tasks[id] + assert.equal(held.lease.holder, 'd1') + assert.ok(held.lease.expiresAt > held.lease.takenAt) + + const refused = await app.track('d2', { claim: true, group: 'War', id }) + assert.match(refused, /lease-held/) + assert.match(refused, /Dev1/, 'the refusal names the holder') + assert.equal(app.rt.state.tasks[id].claimedBy, 'd1', 'a live lease keeps the card claimed') + + // A restart with a live lease changes nothing. + const again = await restart(app) + assert.match(await again.track('d2', { claim: true, group: 'War', id }), /lease-held/) + assert.equal(again.rt.state.tasks[id].claimedBy, 'd1') + + // Expiry is what returns the item: the second Dev takes it from the queue, not by force. + again.rt.state.tasks[id].lease.expiresAt = Date.now() - 1000 + await again.rt.save() + assert.match(await again.track('d2', { claim: true, group: 'War', id }), new RegExp(`^Claimed ${id} `)) + const taken = again.rt.state.tasks[id] + assert.equal(taken.claimedBy, 'd2') + assert.equal(taken.status, 'claimed') + assert.equal(taken.lease.holder, 'd2') + assert.equal(byActor(taken, 'returned to the queue').length >= 1, true, 'the requeue is recorded on the card') +}) + +test('a pre-slice record reads back and renders without the new fields (#1720)', async (t) => { + const seed = team() + seed.tasks = { + 'task-old': { + id: 'task-old', status: 'working', ownerBotId: 'd1', requesterId: 'owner-1', + title: 'An old card', note: 'stored before the slice', source: 'conversation', + origin: { roomId: 'war' }, createdAt: 1, updatedAt: 2, + actors: [{ status: 'working', at: 2 }], + }, + } + const app = await start(t, seed) + const listed = await app.track('d1', {}) + assert.match(listed, /An old card/) + const row = app.rt.state.tasks['task-old'] + assert.equal(row.taskClass, undefined, 'migration is a read, never a rewrite') + assert.equal(row.reworkOf, undefined) + assert.match(formatRoomBoard(app.rt.state.tasks, { roomId: 'war', now: 10, nameOf: () => 'Dev1' }), /An old card/) +}) + +test('the Project board reads the typed fields from the same row (#1720)', () => { + const tasks = { + 'task-old': { id: 'task-old', status: 'working', ownerBotId: 'b1', title: 'Old', createdAt: 1, updatedAt: 2 }, + 'task-new': { + id: 'task-new', status: 'working', ownerBotId: 'b1', title: 'New', createdAt: 3, updatedAt: 4, + taskClass: 'rework', reworkOf: 'task-old', + handoff: { from: 'requester', to: 'Dev', at: 4, reason: 'no test' }, + evidence: [{ sha: 'a'.repeat(40), pr: 12, check: 'tests-ok', at: 4 }], + lease: { holder: 'b1', takenAt: 4, expiresAt: 5 }, + }, + } + const rt = { state: { tasks }, readProject: id => ({ id }), projectOfMember: () => ({ id: 'p' }), taskVisible: () => true } + installProjectBoard(rt) + const rows = rt.projectBoard('p') + const old = rows.find(row => row.id === 'task-old') + const fresh = rows.find(row => row.id === 'task-new') + assert.equal(old.taskClass, undefined, 'a pre-slice row keeps its old shape') + assert.equal(fresh.taskClass, 'rework') + assert.equal(fresh.reworkOf, 'task-old') + assert.equal(fresh.handoff.reason, 'no test') + assert.equal(fresh.evidence[0].pr, 12) + assert.equal(fresh.lease.holder, 'b1') + assert.equal(new Set(rows.map(row => row.id)).size, rows.length, 'one row per task id') +}) diff --git a/core/test/tasks.test.js b/core/test/tasks.test.js index 73cf4480..9c1616bc 100644 --- a/core/test/tasks.test.js +++ b/core/test/tasks.test.js @@ -797,9 +797,14 @@ test('the requester sends a result back with a note and the same task returns to const accepted = await app.call('task-accept-result', { id: returned.id }, alice.sessionToken) assert.equal(accepted.ok, true, JSON.stringify(accepted)) assert.equal(accepted.value.status, 'accepted') - // One task record, moved — never a second task for the second attempt. + // One original record, moved — and exactly one rework card beside it, linked by 'reworkOf' + // and never 'parentId' (#1720): the second attempt is tracked, not a second original. const saved = JSON.parse(readFileSync(app.statePath, 'utf8')).tasks - assert.deepEqual(Object.values(saved).map(task => task.id), [returned.id]) + assert.deepEqual(Object.values(saved).filter(task => task.reworkOf === undefined).map(task => task.id), [returned.id]) + const rework = Object.values(saved).filter(task => task.reworkOf === returned.id) + assert.equal(rework.length, 1, 'one rework card for one rejection') + assert.equal(rework[0].taskClass, 'rework') + assert.equal(rework[0].parentId, undefined, 'a rework card is not part of an overall card') assert.equal(saved[returned.id].followUp.note, 'the second file is missing') }) diff --git a/docs/subsystems/core-host.md b/docs/subsystems/core-host.md index b21e60d6..9da705b8 100644 --- a/docs/subsystems/core-host.md +++ b/docs/subsystems/core-host.md @@ -245,6 +245,10 @@ side-effect rollback is implied. A tracked request with a stable id (`task-`) and a small state machine, owned by `src/host/tasks.js` with `state.tasks` (additive; corrupt values reset, no version bump). States: `open → claimed → working → blocked ⇄ working → result-ready → accepted`, with `cancelled`/`failed` terminal and `accepted` reachable only from `result-ready` — a failed or cancelled task can never display as accepted (#54). Transitions after create are the requester's (server-derived `requesterId`, the signed-in actor or the single owner; `bot/forbidden` otherwise, `bot/stale` on invalid moves), recorded with per-status actor entries. Create does not require owning the Bot: a matching per-Bot grant, or both the requester and the Bot as current members of the named room, is enough; outgoing holds stay the owner's (#52 card). A working task's held send blocks **that** task (`meta.taskId` stamped on the send, matching origin when the Bot has several working tasks; `updatedAt` is strictly increasing, so two starts in one millisecond still order) with the approval id; the owner's decide resumes (approve) or cancels (reject) it; deleting the responsible Bot fails its open tasks. `result-ready` is protected from prune while it waits for acceptance. Endpoints `tasks` / `task-create` / `task-accept` / `task-start` / `task-return` / `task-accept-result` / `task-cancel` are an explicit authz allow-list and are record-checked in tasks.js. Records store pointers and a copied summary, never private transcripts. Clarification and `workBotId` wait on #55's exchange wiring. Team mode stays disabled. After `task-create`, `task-update` and `track_tasks` save, the host asks `rt.onTaskWrite` (hooks exposes it as `flybot.tasks.onWrite`, [plugin-runtime](plugin-runtime.md)). A returned `ownerBotId` that names a different local Bot is stored and saved once more; that second save does not ask again. A missing, remote or identical id leaves the owner (`core/test/task-route.test.js`, #1318). `track_tasks` routes every card it saved, after its own save and before it answers, so a card a Bot records itself reaches the same listeners as a board write made through the channel API (#1462). +### The typed task contract (#1720) + +Five additive fields sit beside the record above; none changes what an existing field means, and a record stored before this slice reads back with them absent and nothing rewritten. `taskClass` is the typed kind (`work | rework | release | triage | meeting`) — it is not `kind`, which keeps its group-card meaning, so a group card is `kind: 'group'` with `taskClass: 'work'`. `reworkOf` names the card a rework card fixes and is not `parentId`, which stays the one-level part-of-overall link (`settleOverall` is untouched); a rework card therefore fires no overall-card settle. `handoff` is `{from, to, at, reason}` with both roles named (a role is known when a Bot carries it, or it is `requester`; an unknown role is refused with the role quoted back), `reason` at most 300 characters. `evidence` is at most eight `{sha, pr, check, at}` pointers, and `lease` is `{holder, takenAt, expiresAt}` on a claimed card. Every value is validated on write by one function, `writeContract` in `src/host/tasks.js`, reached from `track_tasks`, `task-create` and `task-update`; it reads every field before it assigns one, so a refusal (`bot/invalid`, or `bot/unknown-role` for a role no Bot carries) leaves the card exactly as it was. A review rejection (`task-send-back`) records the handover on the original, moves it to `working` and opens exactly one child card with `taskClass: 'rework'` and `reworkOf` naming the original — the single write that returns the original to `result-ready` is the acceptance of that child, guarded by the original still being `working`, so a replay creates no second child and moves the original once. A claim takes a lease (30 minutes); a claim on a live lease is refused as `bot/lease-held` with the holder named, and an expired lease returns a queued card to the queue. The read model does not copy: `project-board.js` puts the five fields on the row it already emits when the card carries them, so a pre-slice card keeps its old row shape. Tests: `core/test/task-contract.test.js`. + ### Task input waits `task-wait-input { id, question }` is a requester-only bookkeeping action on a diff --git a/patches/ds-bot/0002-flybot-api-host.patch b/patches/ds-bot/0002-flybot-api-host.patch index e472f271..4729a171 100644 --- a/patches/ds-bot/0002-flybot-api-host.patch +++ b/patches/ds-bot/0002-flybot-api-host.patch @@ -1365,7 +1365,7 @@ diff --git a/src/host/tools.js b/src/host/tools.js export const output = { schema: { type: 'string' }, render: (_args, value) => [{ type: 'text', text: String(value) }] } export const GROUP_REF = { type: 'string', required: true, description: 'Exact group chat name from list_bots' } -@@ -18,6 +18,21 @@ const WORK_DIR_PARAM = { type: 'string', description: 'Full path of the folder t +@@ -18,6 +18,21 @@ export function install(rt) { const { ctx, state } = rt @@ -1387,7 +1387,7 @@ diff --git a/src/host/tools.js b/src/host/tools.js const callerBot = (exec) => rt.botOf(rt.selfOf(exec.agent?.id)) const callerRoom = (exec) => { const roomId = rt.groupOwners.get(exec.agent?.id)?.roomId -@@ -27,39 +27,112 @@ export function install(rt) { +@@ -27,39 +27,112 @@ const roomLine = (room, self) => { const members = room.members.map(rt.botOf).filter(Boolean) .map(bot => `${bot.name}${bot.id === room.admin ? ' (admin)' : ''}${bot.id === self ? ' (you)' : ''}`) @@ -1507,7 +1507,7 @@ diff --git a/src/host/tools.js b/src/host/tools.js ].join('\n') }, })) -@@ -81,12 +81,22 @@ export function install(rt) { +@@ -81,12 +81,22 @@ // A tool that throws shows as failed in the chat, instead of as sent or stopped. const refuse = (message) => { throw new Error(message) } @@ -1531,7 +1531,7 @@ diff --git a/src/host/tools.js b/src/host/tools.js message: { type: 'string', required: true, description: 'Self-contained message; the other Bot cannot see this conversation' }, reply_to: { type: 'string', -@@ -104,8 +104,18 @@ export function install(rt) { +@@ -104,8 +104,18 @@ await rt.load() const sender = callerBot(exec) if (sender === undefined) return 'Only Bots on the team can use message_bot.' @@ -1552,7 +1552,7 @@ diff --git a/src/host/tools.js b/src/host/tools.js if (target.id === sender.id) return 'You cannot message yourself.' const turn = rt.live.get(exec.agent?.id) const hop = Math.max(turn?.info?.hop ?? 0, ...(turn?.joined ?? []).map(joined => joined.info?.hop ?? 0)) -@@ -118,7 +118,11 @@ export function install(rt) { +@@ -118,7 +118,11 @@ const toUser = args.reply_to === 'user' const working = rt.live.get(chat)?.turn !== undefined const steer = args.when !== 'queue' && !toUser @@ -1565,7 +1565,7 @@ diff --git a/src/host/tools.js b/src/host/tools.js const sent = toUser ? `Delivered to ${target.name}. It will answer the user in its own chat; nothing comes back to you, so point the user to ${target.name}'s chat instead of relaying.` : working && steer -@@ -152,8 +152,8 @@ export function install(rt) { +@@ -152,8 +152,8 @@ await rt.load() const sender = callerBot(exec) if (sender === undefined || !rt.isMain(sender.id)) refuse('Only a Main Bot can stop other Bots.') @@ -1576,7 +1576,7 @@ diff --git a/src/host/tools.js b/src/host/tools.js if (target.id === sender.id) refuse('You cannot stop yourself.') const { running } = activeSessions(target.id) if (running.length === 0) refuse(`${target.name} is not running right now, so there is nothing to stop. Use message_bot to reach it.`) -@@ -162,21 +162,32 @@ export function install(rt) { +@@ -162,21 +162,32 @@ agent.cancel({ kind: 'hook', reason: 'ds-bot/stopped' }) } const why = String(args.reason ?? '').trim() || 'Stop what you are doing; the previous turn was ended on purpose. Acknowledge briefly.' @@ -1612,7 +1612,7 @@ diff --git a/src/host/tools.js b/src/host/tools.js }, output, async execute(args, exec) { -@@ -186,8 +186,8 @@ export function install(rt) { +@@ -186,8 +186,8 @@ const workDir = args.work_dir ? await rt.cleanWorkDir(args.work_dir) : undefined const name = String(args.name ?? '').trim() await rt.approveFolder(exec, { path: workDir, who: { en: `the new Bot ${name}`, zh: `新 Bot ${name} ` } }) @@ -1623,7 +1623,7 @@ diff --git a/src/host/tools.js b/src/host/tools.js } catch (error) { return `Could not create the Bot: ${error.message}` } -@@ -212,7 +212,7 @@ export function install(rt) { +@@ -212,7 +212,7 @@ const self = rt.selfOf(exec.agent?.id) const caller = rt.botOf(self) if (caller === undefined) return 'Only Bots on the team can use update_bot.' @@ -1632,7 +1632,7 @@ diff --git a/src/host/tools.js b/src/host/tools.js if (bot === undefined) return `No Bot named ${args.bot}.` if (bot.id !== caller.id && !rt.isMain(self)) return 'Only a Main Bot can change another Bot. Leave out bot to change yourself, or ask a Main Bot with message_bot.' try { -@@ -231,6 +231,30 @@ export function install(rt) { +@@ -231,6 +231,30 @@ })) ctx.tools.register(defineTool({ @@ -1663,7 +1663,7 @@ diff --git a/src/host/tools.js b/src/host/tools.js name: 'list_models', description: 'List the models a Bot can run on, as provider/model refs for create_bot and update_bot.', parameters: {}, -@@ -251,13 +251,13 @@ export function install(rt) { +@@ -251,13 +251,13 @@ const bots = [] const unknown = [] for (const ref of nameList(refs)) { @@ -1679,7 +1679,7 @@ diff --git a/src/host/tools.js b/src/host/tools.js const groupNames = self => visibleRooms(self).map(room => `"${room.name}"`).join(', ') || 'none' const pickRoom = (ref, self) => { const matches = rt.findRooms(ref).filter(room => visibleRooms(self).includes(room)) -@@ -283,7 +283,7 @@ export function install(rt) { +@@ -283,7 +283,7 @@ if (!rt.isMain(self)) return 'Only a Main Bot can create group chats. Ask a Main Bot instead.' const { bots, unknown } = resolveBots(args.members) if (unknown.length > 0) return `No Bot named ${unknown.join(', ')}. Team: ${teamNames()}` @@ -1688,7 +1688,7 @@ diff --git a/src/host/tools.js b/src/host/tools.js if (admin === undefined) return `No Bot named ${args.admin}. Team: ${teamNames()}` const members = [...new Set([self, ...bots.map(bot => bot.id), admin.id])] try { -@@ -297,7 +297,7 @@ export function install(rt) { +@@ -297,7 +297,7 @@ ctx.tools.register(defineTool({ name: 'update_group', @@ -1697,7 +1697,7 @@ diff --git a/src/host/tools.js b/src/host/tools.js parameters: { group: GROUP_REF, name: { type: 'string', description: 'New group name; an empty string names it after its members again' }, -@@ -305,6 +305,7 @@ export function install(rt) { +@@ -305,6 +305,7 @@ add_members: { type: 'array', items: { type: 'string' }, description: 'Exact names of Bots to add' }, remove_members: { type: 'array', items: { type: 'string' }, description: 'Exact names of Bots to remove' }, admin: { type: 'string', description: 'Member to make the admin; "none" leaves the group without one (Main Bots only)' }, @@ -1705,7 +1705,7 @@ diff --git a/src/host/tools.js b/src/host/tools.js mode: MODE_PARAM, work_dir: { ...WORK_DIR_PARAM, description: `${WORK_DIR_PARAM.description}. Every member works there in this group; an empty string lets each member work in its own folder again (Main Bots only)` }, }, -@@ -325,10 +325,18 @@ export function install(rt) { +@@ -325,10 +325,18 @@ if (args.admin !== undefined && args.admin !== '') { if (/^(none|nobody|null|无|没有)$/i.test(String(args.admin).trim())) admin = null else { @@ -1725,7 +1725,7 @@ diff --git a/src/host/tools.js b/src/host/tools.js try { const workDir = moving ? await rt.cleanWorkDir(args.work_dir) : undefined // A Bot that joins a group with a folder works there from its first turn. -@@ -345,6 +345,7 @@ export function install(rt) { +@@ -345,6 +345,7 @@ notice: args.notice, mode: args.mode, admin, @@ -1733,7 +1733,7 @@ diff --git a/src/host/tools.js b/src/host/tools.js add: add.bots.map(bot => bot.id), remove: remove.bots.map(bot => bot.id), ...(moving ? { workDir: workDir ?? '' } : {}), -@@ -395,12 +395,11 @@ export function install(rt) { +@@ -395,12 +395,11 @@ if ((info?.hop ?? 0) >= MAX_HOPS) return 'This chain of Bot messages is already long. Stop here and report to the user instead.' const text = String(args.message ?? '').trim() if (text === '') return 'Write a message to post.' @@ -1749,7 +1749,7 @@ diff --git a/src/host/tools.js b/src/host/tools.js } catch (failure) { return `Could not post in "${room.name}": ${failure.message}` } -@@ -445,6 +445,10 @@ export function install(rt) { +@@ -445,6 +445,10 @@ .filter(option => option.label !== '') .slice(0, 6) if (options.length === 0) return 'Give at least one option.' @@ -1760,7 +1760,7 @@ diff --git a/src/host/tools.js b/src/host/tools.js // A hidden Bot-to-Bot turn shows none of its calls, so its card goes to the // composer dock, which takes questions without a call. const turn = rt.live.get(id)?.turn -@@ -465,6 +465,77 @@ export function install(rt) { +@@ -465,6 +465,77 @@ }, })) @@ -1838,7 +1838,7 @@ diff --git a/src/host/tools.js b/src/host/tools.js // dsh's own question tool holds the turn open until the user answers, and every // message from another Bot or a group round queues behind it. Bot Sessions ask // through ask_user instead, so the tool is taken out of their tool list. -@@ -525,7 +525,7 @@ export function install(rt) { +@@ -525,7 +525,7 @@ name: 'save_capability', description: 'Create or update a shared MCP server, skill, or pack. Changes apply from your next message.', parameters: { @@ -1847,7 +1847,7 @@ diff --git a/src/host/tools.js b/src/host/tools.js kind: { type: 'string', required: true, enum: ['mcp', 'skill', 'pack'] }, description: { type: 'string', description: 'What this capability provides' }, transport: { type: 'string', enum: ['stdio', 'streamable-http'], description: 'How to reach an MCP server: a local command or an HTTP URL' }, -@@ -545,7 +545,7 @@ export function install(rt) { +@@ -545,7 +545,7 @@ try { const item = await rt.capabilitySave(args, { botId: self }) const approval = item.kind === 'mcp' && item.transport === 'stdio' && !item.approved @@ -1856,7 +1856,7 @@ diff --git a/src/host/tools.js b/src/host/tools.js : '' return `Saved ${item.name}. Bots with it attached use the change from their next message.${approval}` } catch (error) { return error.message } -@@ -587,7 +587,7 @@ export function install(rt) { +@@ -587,7 +587,7 @@ : attached ? [attached] : [] const approvals = members .filter(item => item.kind === 'mcp' && item.transport === 'stdio' && item.approved !== true) @@ -1865,7 +1865,7 @@ diff --git a/src/host/tools.js b/src/host/tools.js .join('') return `${verb} ${args.name} ${args.bot ? `to ${result.bot}` : 'to yourself'}. Changes apply from ${result.bot}'s next message.${endpoint === 'attach' ? approvals : ''}` } catch (error) { return error.message } -@@ -595,5 +595,121 @@ export function install(rt) { +@@ -595,5 +595,158 @@ })) } @@ -1939,6 +1939,43 @@ diff --git a/src/host/tools.js b/src/host/tools.js + ask: { type: 'string', description: 'The question you need before this task can continue. The task waits. Call ask_user in your own chat. For a group card, post one short question in the group only when a person has to decide.' }, + answer: { type: 'string', description: 'What the user or the group answered. Pass it to resume a task that was waiting.' }, + target: { type: 'string', description: 'Where a decision on this task is written back: a GitHub issue or pull request, as https://github.com/owner/repo/issues/12 or owner/repo#12. Optional.' }, ++ taskClass: { type: 'string', enum: ['work', 'rework', 'release', 'triage', 'meeting'], description: 'The typed class of the card. Work is the default; a rework card fixes a card that was rejected.' }, ++ reworkOf: { type: 'string', description: 'The id of the card this rework card fixes. A rework card is not a part of a group card, so pass reworkOf, never parent.' }, ++ handoff: { ++ type: 'object', ++ description: 'Who handed this card to whom, why and when. Both roles must be roles a Bot carries (or requester).', ++ properties: { ++ from: { type: 'string', description: 'The role the card came from.' }, ++ to: { type: 'string', description: 'The role the card went to.' }, ++ at: { type: 'number', description: 'When, in milliseconds since the epoch. Defaults to now.' }, ++ reason: { type: 'string', description: 'One line, at most 300 characters.' }, ++ }, ++ additionalProperties: false, ++ }, ++ evidence: { ++ type: 'array', ++ description: 'Up to 8 pointers to what proves the work: a sha, a pull request, a check name, when.', ++ items: { ++ type: 'object', ++ additionalProperties: false, ++ properties: { ++ sha: { type: 'string' }, ++ pr: { type: 'integer' }, ++ check: { type: 'string' }, ++ at: { type: 'number', description: 'When, in milliseconds since the epoch.' }, ++ }, ++ }, ++ }, ++ lease: { ++ type: 'object', ++ description: 'The claim held on this card: who holds it and when that claim lapses.', ++ properties: { ++ holder: { type: 'string' }, ++ takenAt: { type: 'number', description: 'When the claim was taken, in milliseconds since the epoch.' }, ++ expiresAt: { type: 'number', description: 'When the claim lapses, in milliseconds since the epoch.' }, ++ }, ++ additionalProperties: false, ++ }, + postResult: { + type: 'object', + description: 'How the decision comment a task asked you to post went. Pass task (its id) and ok; when ok is false, pass the error. One report per decision, and no retrying on its own.', @@ -1987,6 +2024,7 @@ diff --git a/src/host/tools.js b/src/host/tools.js + Object.assign(rt, { pickRoom, hideBlockingQuestion }) } + diff --git a/test/blocking-question.test.js b/test/blocking-question.test.js --- a/test/blocking-question.test.js +++ b/test/blocking-question.test.js diff --git a/patches/ds-bot/files/src/host/project-board.js b/patches/ds-bot/files/src/host/project-board.js index 78db0acc..f2892a42 100644 --- a/patches/ds-bot/files/src/host/project-board.js +++ b/patches/ds-bot/files/src/host/project-board.js @@ -60,6 +60,14 @@ export function install(rt) { column: columnOf(task), assignee: task.assignee ?? null, updatedAt: task.updatedAt ?? task.createdAt ?? null, + // The typed contract (#1720) is read from the same row, not a second copy: a board that + // shows a rework or a held lease shows what the task store wrote. A record stored before + // the slice carries none of the keys and reads back exactly as it did. + ...(task.taskClass ? { taskClass: task.taskClass } : {}), + ...(task.reworkOf ? { reworkOf: task.reworkOf } : {}), + ...(task.handoff && typeof task.handoff === 'object' ? { handoff: task.handoff } : {}), + ...(Array.isArray(task.evidence) ? { evidence: task.evidence } : {}), + ...(task.lease && typeof task.lease === 'object' ? { lease: task.lease } : {}), ...(fromRun ? { runId: ref.runId, ...(ref.nodeId ? { nodeId: ref.nodeId } : {}) } : {}), }) } diff --git a/patches/ds-bot/files/src/host/tasks.js b/patches/ds-bot/files/src/host/tasks.js index 4b94494c..10bd7901 100644 --- a/patches/ds-bot/files/src/host/tasks.js +++ b/patches/ds-bot/files/src/host/tasks.js @@ -353,7 +353,10 @@ export function install(rt) { transition(task, next, actor, extra) if (next === 'result-ready') task.result = { kind: 'summary', summary: (task.note || 'Completed').slice(0, 2000) } } - if (target === 'accepted') settleOverall(task) + if (target === 'accepted') { + settleOverall(task) + closeRework(task, actor) + } return true } @@ -377,6 +380,178 @@ export function install(rt) { return moved } + // ---------------------------------------------------------------- the typed contract (#1720) + // Five additive fields, none of which changes an existing one: `taskClass` (the typed kind, + // beside the group-card `kind`), `reworkOf` (a rework child's link to the card it reworks, + // beside the part-of-overall `parentId`), `handoff`, `evidence` and `lease`. Every one is + // validated by name — a refusal names the field, never a silently dropped value. + const TASK_CLASSES = ['work', 'rework', 'release', 'triage', 'meeting'] + const HANDOFF_REASON_MAX = 300 + const EVIDENCE_MAX = 8 + const LEASE_TTL_SEC = 30 * 60 + // A role is known when a Bot carries it; `requester` is the human side of a handoff, which no + // Bot record names. Anything else is refused with the role quoted back (#1720). + const RESERVED_ROLES = new Set(['requester']) + const knownRole = role => RESERVED_ROLES.has(role) || Object.values(state.bots ?? {}).some(bot => Boolean(bot?.role) && sameRole(bot.role, role)) + const boundedLine = (value, max, name) => { + const text = String(value ?? '').replace(/[\u0000-\u001f\u007f]/g, ' ').replace(/\s+/g, ' ').trim() + if (text.length > max) throw invalid(`${name} is at most ${max} characters`) + return text + } + const readTaskClass = (value) => { + if (value === undefined || value === null || value === '') return undefined + const taskClass = String(value) + if (!TASK_CLASSES.includes(taskClass)) throw invalid(`taskClass must be one of ${TASK_CLASSES.join(', ')}`) + return taskClass + } + const readReworkOf = (value) => { + if (value === undefined || value === null || String(value).trim() === '') return undefined + const id = boundedLine(value, 80, 'reworkOf') + if (find(id) === undefined) throw invalid(`reworkOf names no task: ${id}`) + return id + } + const readStampOf = (value, name) => { + if (typeof value === 'number' && Number.isFinite(value)) return value + const parsed = typeof value === 'string' && value.trim() !== '' ? Date.parse(value) : Number.NaN + if (!Number.isFinite(parsed)) throw invalid(`${name} must be a time (milliseconds or an ISO date)`) + return parsed + } + const readHandoff = (value) => { + if (value === undefined || value === null) return undefined + if (typeof value !== 'object' || Array.isArray(value)) throw invalid('handoff must be an object with from, to, at and reason') + const from = boundedLine(value.from, ROLE_MAX, 'handoff.from') + const to = boundedLine(value.to, ROLE_MAX, 'handoff.to') + if (from === '') throw invalid('handoff needs the role it came from (from)') + if (to === '') throw invalid('handoff needs the role it goes to (to)') + if (!knownRole(from)) throw refuse('unknown-role', `handoff.from names the role "${from}", which no Bot carries`) + if (!knownRole(to)) throw refuse('unknown-role', `handoff.to names the role "${to}", which no Bot carries`) + const reason = boundedLine(value.reason, HANDOFF_REASON_MAX, 'handoff.reason') + const at = value.at === undefined || value.at === null ? stamp() : readStampOf(value.at, 'handoff.at') + return { from, to, at, reason } + } + const readEvidence = (value) => { + if (value === undefined || value === null) return undefined + if (!Array.isArray(value)) throw invalid('evidence must be a list of at most 8 entries of { sha, pr, check, at }') + if (value.length > EVIDENCE_MAX) throw invalid(`evidence carries at most ${EVIDENCE_MAX} entries`) + return value.map((entry) => { + if (entry === null || typeof entry !== 'object' || Array.isArray(entry)) throw invalid('each evidence entry is an object of { sha, pr, check, at }') + const sha = boundedLine(entry.sha, 64, 'evidence.sha') + const check = boundedLine(entry.check, 200, 'evidence.check') + let pr + if (entry.pr !== undefined && entry.pr !== null && entry.pr !== '') { + pr = typeof entry.pr === 'number' ? entry.pr : Number(entry.pr) + if (!Number.isInteger(pr) || pr < 1) throw invalid('evidence.pr must be a pull request number') + } + const at = entry.at === undefined || entry.at === null ? undefined : readStampOf(entry.at, 'evidence.at') + if (sha === '' && pr === undefined && check === '' && at === undefined) throw invalid('an evidence entry needs a sha, a pr, a check or an at') + return { sha, ...(pr === undefined ? {} : { pr }), check, ...(at === undefined ? {} : { at }) } + }) + } + const readLease = (value) => { + if (value === undefined || value === null) return undefined + if (typeof value !== 'object' || Array.isArray(value)) throw invalid('lease must be an object with holder, takenAt and expiresAt') + const holder = boundedLine(value.holder, 200, 'lease.holder') + if (holder === '') throw invalid('lease needs a holder') + const takenAt = readStampOf(value.takenAt, 'lease.takenAt') + const expiresAt = readStampOf(value.expiresAt, 'lease.expiresAt') + if (expiresAt <= takenAt) throw invalid('lease.expiresAt must be after lease.takenAt') + return { holder, takenAt, expiresAt } + } + // One write path for the five fields (#1720): the tool, the conversation path and the HTTP + // endpoints all reach it, so a refusal is the same refusal wherever the value came from. The + // reads happen before any assignment, so a refused write leaves the card exactly as it was. + function writeContract(task, body = {}) { + const taskClass = readTaskClass(body.taskClass) + const reworkOf = readReworkOf(body.reworkOf) + const handoff = readHandoff(body.handoff) + const evidence = readEvidence(body.evidence) + const clearingLease = body.lease === null + const lease = readLease(body.lease) + if (taskClass !== undefined) task.taskClass = taskClass + if (reworkOf !== undefined) task.reworkOf = reworkOf + if (handoff !== undefined) task.handoff = handoff + if (evidence !== undefined) task.evidence = evidence + if (clearingLease) delete task.lease + if (lease !== undefined) task.lease = lease + return { taskClass, reworkOf, handoff, evidence, lease } + } + // The typed contract on a card that is created here rather than written by a caller. + const defaultContract = (task) => { + task.taskClass = 'work' + return task + } + const leaseOf = task => (task?.lease !== null && typeof task?.lease === 'object' ? task.lease : undefined) + const leaseLive = (task, now = Date.now()) => { + const lease = leaseOf(task) + return lease !== undefined && Number.isFinite(lease.expiresAt) && lease.expiresAt > now + } + const holderName = id => botRecord(id)?.name ?? String(id ?? '') + // An expired lease returns a queued card to the queue (#1720): the member holding it is gone + // or stalled, so the item is offered again instead of staying claimed for ever. A card that + // is not a queued item keeps its status; only the dead lease is dropped. + function expireLeases(now = Date.now()) { + let expired = 0 + for (const task of Object.values(store())) { + const lease = leaseOf(task) + if (lease === undefined || !Number.isFinite(lease.expiresAt) || lease.expiresAt > now) continue + if (!task.queueRole || (task.status !== 'claimed' && task.status !== 'working')) { + delete task.lease + continue + } + requeue(task, 'lease expired', 0) + delete task.lease + expired += 1 + } + return expired + } + // A rejection is a handover, not a status flip (#1720): the original's `handoff` names the + // roles, and the rework is one child card linked by `reworkOf` — never `parentId`, which + // means "part of an overall card" and must never make `settleOverall` fire. A replay of the + // same rejection reuses the child that is still open instead of creating a second one. + function openRework(original, note) { + const reason = boundedLine(note, HANDOFF_REASON_MAX, 'handoff.reason') + const to = oneLine(botRecord(original.ownerBotId)?.role ?? '', ROLE_MAX) || 'requester' + const from = 'requester' + const at = stamp() + original.handoff = { from, to, at, reason } + const existing = Object.values(store()).find(task => task.reworkOf === original.id && openTask(task)) + if (existing !== undefined) { + existing.handoff = { from, to, at, reason } + existing.actors = [...(existing.actors ?? []), { status: existing.status, at, by: 'rework replayed' }] + existing.updatedAt = at + return existing + } + const child = { + id: `task-${randomBytes(8).toString('hex')}`, + status: 'open', + taskClass: 'rework', + reworkOf: original.id, + createdAt: at, + updatedAt: at, + requesterId: original.requesterId ?? null, + ownerBotId: original.ownerBotId, + title: oneLine(`${original.title} (rework)`, 300), + note: reason, + source: 'rework', + handoff: { from, to, at, reason }, + origin: { ...(original.origin ?? {}) }, + actors: [{ status: 'open', at, by: 'rework opened' }], + } + store()[child.id] = child + return child + } + // The one write that returns the original to `result-ready` (#1720): it runs where a task is + // accepted, and the guard is the original still being `working`, so accepting a rework child + // twice moves the original once and a replay after a restart is a no-op. + function closeRework(child, actor) { + if (child?.taskClass !== 'rework' || typeof child.reworkOf !== 'string') return false + const original = find(child.reworkOf) + if (original === undefined || original.status !== 'working') return false + transition(original, 'result-ready', actor, { by: 'rework accepted' }) + original.result = { kind: 'summary', summary: 'Rework accepted' } + return true + } + function applyFields(task, body) { const title = body.title !== undefined ? readTitle(body.title) : undefined if (title !== undefined) { @@ -472,6 +647,7 @@ export function install(rt) { delete task.inputApprovalIds delete task.approvalId delete task.claimedBy + delete task.lease task.status = 'open' task.ownerBotId = task.queueBy ?? task.ownerBotId task.progress = 0 @@ -598,6 +774,7 @@ export function install(rt) { origin: { roomId: room.id }, actors: [{ status: 'open', at, by: 'dispatch', ...(by ? { botId: by } : {}) }], } + defaultContract(task) store()[task.id] = task await save() return view(task) @@ -610,6 +787,7 @@ export function install(rt) { const room = resolveRoom(botId, body.group, roomId) const entry = room ? state.rooms?.[room] : undefined if (!entry) throw invalid('A claim needs a group') + if (expireLeases() > 0) await rt.save() const myRole = oneLine(botRecord(botId)?.role ?? '', ROLE_MAX) if (myRole === '') throw refuse('not-in-role', 'this Bot has no role to claim with') // WIP = 1: one card in progress fills the member's slot. A blocked card waits on a person, @@ -624,7 +802,12 @@ export function install(rt) { if (wantedId !== undefined) { pick = find(wantedId) if (pick === undefined) throw stale(`no task ${wantedId}`) - if (!unclaimed(pick) || pick.origin?.roomId !== entry.id) throw stale(`no queued item ${wantedId} in this group`) + if (!unclaimed(pick) || pick.origin?.roomId !== entry.id) { + if (leaseLive(pick)) { + throw refuse('lease-held', `task ${pick.id} is held by ${holderName(pick.lease.holder)} until ${new Date(pick.lease.expiresAt).toISOString()}`) + } + throw stale(`no queued item ${wantedId} in this group`) + } if (!sameRole(pick.queueRole, myRole)) throw refuse('not-in-role', `that item is queued for ${pick.queueRole}, and this Bot is ${myRole}`) if (!offered(pick)) return { item: null, reason: 'waiting' } } else { @@ -643,6 +826,8 @@ export function install(rt) { live.ownerBotId = botId delete live.retryAt delete live.queueReason + const claimedAt = Date.now() + live.lease = { holder: botId, takenAt: claimedAt, expiresAt: claimedAt + LEASE_TTL_SEC * 1000 } transition(live, 'claimed', null, { by: 'claimed from the queue' }) announceClaim(live) await save() @@ -699,6 +884,7 @@ export function install(rt) { origin: { ...(roomId ? { roomId } : {}), ...(origin.sessionId ? { sessionId: String(origin.sessionId) } : {}) }, actors: [{ status: 'open', at, by: 'conversation', ...(actor?.memberId ? { memberId: actor.memberId } : {}) }], } + defaultContract(task) store()[task.id] = task return task } @@ -795,6 +981,14 @@ export function install(rt) { throw stale('This task is waiting on an answer. Ask, then pass answer with what was said.') } if (title && task.title !== title) task.title = title + // A refused value must not leave a half-created card behind: the writer validates before + // it assigns, and a new row it refuses is removed again (#1720). + try { + writeContract(task, item) + } catch (error) { + if (isNew) delete store()[task.id] + throw error + } const { progress } = applyFields(task, item) if (!task.source) task.source = 'conversation' const before = task.updatedAt @@ -990,6 +1184,8 @@ export function install(rt) { origin: { ...(roomId ? { roomId } : {}), ...(body.sessionId ? { sessionId: String(body.sessionId) } : {}) }, actors: [{ status: 'open', at: stamp(), ...(actor?.memberId ? { memberId: actor.memberId } : {}) }], } + defaultContract(task) + writeContract(task, body) const { progress } = applyFields(task, body) const column = readColumn(body.column) applyColumn(task, column, actor) @@ -1004,6 +1200,7 @@ export function install(rt) { const task = find(body.id) if (task === undefined) throw stale(`no task ${String(body.id ?? '')}`) if (!mayUpdate(task, actor)) throw forbidden('Only the requester or the Bot\'s owner may update this task') + writeContract(task, body) const { progress } = applyFields(task, body) const column = readColumn(body.column) const before = task.updatedAt @@ -1073,6 +1270,7 @@ export function install(rt) { requireRequester(task, actor) transition(task, 'accepted', actor) settleOverall(task) + closeRework(task, actor) requestWriteBack(task, 'accepted', undefined, actor) await save() return view(task) @@ -1086,6 +1284,7 @@ export function install(rt) { if (note === '') throw invalid('Send back needs a note for the Bot') transition(task, 'working', actor, { by: 'sent back' }) task.followUp = { note, at: task.updatedAt } + openRework(task, note) requestWriteBack(task, 'working', note, actor) await save() return view(task) diff --git a/patches/ds-bot/files/test/task-contract.test.js b/patches/ds-bot/files/test/task-contract.test.js new file mode 100644 index 00000000..88c20d87 --- /dev/null +++ b/patches/ds-bot/files/test/task-contract.test.js @@ -0,0 +1,251 @@ +// The typed Task/Handoff contract (#1720): five additive fields on the one task record — +// `taskClass`, `reworkOf`, `handoff`, `evidence`, `lease` — validated on write and read by the +// Project board from the same row. The rework child is a card linked by `reworkOf` (never +// `parentId`), a lease returns an abandoned queued card to the queue, and a record stored before +// the slice still reads back and renders. +import assert from 'node:assert/strict' +import { mkdtempSync, 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 { install as installProjectBoard } from '../src/host/project-board.js' +import { formatRoomBoard } from '../src/host/room-context.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 + } + const create = async (spec = {}) => { + const body = await track(spec.bot ?? 'd1', { tasks: [{ group: 'War', ...spec }] }) + return { body, id: idOf(body, spec.title) } + } + return { ctx, rt, track, idOf, create, home, agents } +} + +async function start(t, seed = team()) { + const home = mkdtempSync(join(tmpdir(), 'ds-bot-contract-')) + writeFileSync(join(home, 'state.json'), JSON.stringify(seed)) + const app = await boot(home) + app.stop = async () => rmSync(home, { recursive: true, force: true, maxRetries: 5 }) + t.after(async () => { + try { await app.ctx.dispose() } catch {} + await app.stop() + }) + return app +} + +// A restart is the same home and a fresh runtime: the state on disk is what the new process reads. +async function restart(app) { + await app.ctx.dispose() + return boot(app.home) +} + +const childrenOf = (app, originalId) => Object.values(app.rt.state.tasks).filter(task => task.reworkOf === originalId) +const byActor = (task, why) => (task.actors ?? []).filter(entry => entry.by === why) + +test('a handoff records from/to/reason and an unknown role is refused by name (#1720)', async (t) => { + const app = await start(t) + const { id } = await app.create({ title: 'Hand the diff on', handoff: { from: 'Lead', to: 'Dev', at: 1_700_000_000_000, reason: 'needs the test' } }) + const task = app.rt.state.tasks[id] + assert.equal(task.taskClass, 'work', 'a card created without a class is a work card') + assert.deepEqual({ ...task.handoff }, { from: 'Lead', to: 'Dev', at: 1_700_000_000_000, reason: 'needs the test' }) + + const refused = await app.track('d1', { tasks: [{ group: 'War', title: 'Another card', handoff: { from: 'Lead', to: 'Nobody', reason: 'x' } }] }) + assert.match(refused, /unknown-role/) + assert.match(refused, /Nobody/) + assert.equal(Object.values(app.rt.state.tasks).some(task => task.title === 'Another card'), false, 'a refused write leaves no half-created card') + + // The field name is the refusal: a bad class is not silently dropped either (the HTTP + // endpoint carries a value the tool schema would already refuse). + await assert.rejects( + () => app.rt.taskEndpoint('task-create', { botId: 'd1', title: 'Third card', taskClass: 'kind' }), + /taskClass must be one of work, rework, release, triage, meeting/, + ) +}) + +test('a rejection creates exactly one rework child with reworkOf and no parentId (#1720)', async (t) => { + const app = await start(t) + const { id } = await app.create({ title: 'Fix the flaky test' }) + assert.match(await app.track('d1', { tasks: [{ group: 'War', id, column: 'in-progress' }] }), /Fix the flaky test/) + await app.rt.taskEndpoint('task-return', { id, summary: 'done, see the diff' }) + await app.rt.taskEndpoint('task-send-back', { id, note: 'the diff has no test' }) + + const original = app.rt.state.tasks[id] + assert.equal(original.status, 'working', 'the original goes back to working') + assert.equal(original.handoff.from, 'requester') + assert.equal(original.handoff.to, 'Dev') + assert.equal(original.handoff.reason, 'the diff has no test') + const children = childrenOf(app, id) + assert.equal(children.length, 1) + assert.equal(children[0].taskClass, 'rework') + assert.equal(children[0].reworkOf, id) + assert.equal(children[0].parentId, undefined, 'a rework card is not part of an overall card') + assert.equal(children[0].ownerBotId, 'd1') + + // The rejection replayed on the same card: a returning result is a second rejection, and it + // reuses the child it already opened instead of minting a second one. + await app.rt.taskEndpoint('task-return', { id, summary: 'done again' }) + await app.rt.taskEndpoint('task-send-back', { id, note: 'still no test' }) + assert.equal(childrenOf(app, id).length, 1, 'one rework child per rejected card') +}) + +test('a rejection replayed after a restart adds no child and returns the original once (#1720)', async (t) => { + const app = await start(t) + const { id } = await app.create({ title: 'Fix the flaky test' }) + await app.track('d1', { tasks: [{ group: 'War', id, column: 'in-progress' }] }) + await app.rt.taskEndpoint('task-return', { id, summary: 'done' }) + await app.rt.taskEndpoint('task-send-back', { id, note: 'the diff has no test' }) + const childId = childrenOf(app, id)[0].id + await app.rt.taskEndpoint('task-return', { id, summary: 'reworked' }) + await app.rt.save() + + const again = await restart(app) + const { rt } = again + assert.equal(childrenOf(again, id).length, 1, 'the restart reads the same one child') + // The same rejection lands again: the open child is reused, never duplicated. + await rt.taskEndpoint('task-send-back', { id, note: 'the same rejection' }) + assert.equal(childrenOf(again, id).length, 1) + assert.equal(childrenOf(again, id)[0].id, childId) + + // The child's acceptance is the single write that returns the original to result-ready. + await rt.taskEndpoint('task-update', { id: childId, column: 'done' }) + assert.equal(rt.state.tasks[id].status, 'result-ready') + assert.equal(byActor(rt.state.tasks[id], 'rework accepted').length, 1) + // A replayed acceptance (the endpoint retried) is not a second transition. + await assert.rejects(() => rt.taskEndpoint('task-accept-result', { id: childId })) + assert.equal(rt.state.tasks[id].status, 'result-ready') + assert.equal(byActor(rt.state.tasks[id], 'rework accepted').length, 1, 'the original moves once') +}) + +test('an expired lease requeues and a live lease refuses a second claim naming the holder (#1720)', async (t) => { + const app = await start(t) + const body = await app.track('a', { tasks: [{ group: 'War', queue: true, role: 'Dev', title: 'Ship the note' }] }) + const id = app.idOf(body, 'Ship the note') + assert.match(await app.track('d1', { claim: true, group: 'War' }), new RegExp(`^Claimed ${id} `)) + const held = app.rt.state.tasks[id] + assert.equal(held.lease.holder, 'd1') + assert.ok(held.lease.expiresAt > held.lease.takenAt) + + const refused = await app.track('d2', { claim: true, group: 'War', id }) + assert.match(refused, /lease-held/) + assert.match(refused, /Dev1/, 'the refusal names the holder') + assert.equal(app.rt.state.tasks[id].claimedBy, 'd1', 'a live lease keeps the card claimed') + + // A restart with a live lease changes nothing. + const again = await restart(app) + assert.match(await again.track('d2', { claim: true, group: 'War', id }), /lease-held/) + assert.equal(again.rt.state.tasks[id].claimedBy, 'd1') + + // Expiry is what returns the item: the second Dev takes it from the queue, not by force. + again.rt.state.tasks[id].lease.expiresAt = Date.now() - 1000 + await again.rt.save() + assert.match(await again.track('d2', { claim: true, group: 'War', id }), new RegExp(`^Claimed ${id} `)) + const taken = again.rt.state.tasks[id] + assert.equal(taken.claimedBy, 'd2') + assert.equal(taken.status, 'claimed') + assert.equal(taken.lease.holder, 'd2') + assert.equal(byActor(taken, 'returned to the queue').length >= 1, true, 'the requeue is recorded on the card') +}) + +test('a pre-slice record reads back and renders without the new fields (#1720)', async (t) => { + const seed = team() + seed.tasks = { + 'task-old': { + id: 'task-old', status: 'working', ownerBotId: 'd1', requesterId: 'owner-1', + title: 'An old card', note: 'stored before the slice', source: 'conversation', + origin: { roomId: 'war' }, createdAt: 1, updatedAt: 2, + actors: [{ status: 'working', at: 2 }], + }, + } + const app = await start(t, seed) + const listed = await app.track('d1', {}) + assert.match(listed, /An old card/) + const row = app.rt.state.tasks['task-old'] + assert.equal(row.taskClass, undefined, 'migration is a read, never a rewrite') + assert.equal(row.reworkOf, undefined) + assert.match(formatRoomBoard(app.rt.state.tasks, { roomId: 'war', now: 10, nameOf: () => 'Dev1' }), /An old card/) +}) + +test('the Project board reads the typed fields from the same row (#1720)', () => { + const tasks = { + 'task-old': { id: 'task-old', status: 'working', ownerBotId: 'b1', title: 'Old', createdAt: 1, updatedAt: 2 }, + 'task-new': { + id: 'task-new', status: 'working', ownerBotId: 'b1', title: 'New', createdAt: 3, updatedAt: 4, + taskClass: 'rework', reworkOf: 'task-old', + handoff: { from: 'requester', to: 'Dev', at: 4, reason: 'no test' }, + evidence: [{ sha: 'a'.repeat(40), pr: 12, check: 'tests-ok', at: 4 }], + lease: { holder: 'b1', takenAt: 4, expiresAt: 5 }, + }, + } + const rt = { state: { tasks }, readProject: id => ({ id }), projectOfMember: () => ({ id: 'p' }), taskVisible: () => true } + installProjectBoard(rt) + const rows = rt.projectBoard('p') + const old = rows.find(row => row.id === 'task-old') + const fresh = rows.find(row => row.id === 'task-new') + assert.equal(old.taskClass, undefined, 'a pre-slice row keeps its old shape') + assert.equal(fresh.taskClass, 'rework') + assert.equal(fresh.reworkOf, 'task-old') + assert.equal(fresh.handoff.reason, 'no test') + assert.equal(fresh.evidence[0].pr, 12) + assert.equal(fresh.lease.holder, 'b1') + assert.equal(new Set(rows.map(row => row.id)).size, rows.length, 'one row per task id') +}) diff --git a/patches/ds-bot/files/test/tasks.test.js b/patches/ds-bot/files/test/tasks.test.js index 73cf4480..9c1616bc 100644 --- a/patches/ds-bot/files/test/tasks.test.js +++ b/patches/ds-bot/files/test/tasks.test.js @@ -797,9 +797,14 @@ test('the requester sends a result back with a note and the same task returns to const accepted = await app.call('task-accept-result', { id: returned.id }, alice.sessionToken) assert.equal(accepted.ok, true, JSON.stringify(accepted)) assert.equal(accepted.value.status, 'accepted') - // One task record, moved — never a second task for the second attempt. + // One original record, moved — and exactly one rework card beside it, linked by 'reworkOf' + // and never 'parentId' (#1720): the second attempt is tracked, not a second original. const saved = JSON.parse(readFileSync(app.statePath, 'utf8')).tasks - assert.deepEqual(Object.values(saved).map(task => task.id), [returned.id]) + assert.deepEqual(Object.values(saved).filter(task => task.reworkOf === undefined).map(task => task.id), [returned.id]) + const rework = Object.values(saved).filter(task => task.reworkOf === returned.id) + assert.equal(rework.length, 1, 'one rework card for one rejection') + assert.equal(rework[0].taskClass, 'rework') + assert.equal(rework[0].parentId, undefined, 'a rework card is not part of an overall card') assert.equal(saved[returned.id].followUp.note, 'the second file is missing') })