From dfd0a4895561f99ba22317565519644efff5256a Mon Sep 17 00:00:00 2001 From: Warren Date: Tue, 1 Sep 2026 09:48:39 +0000 Subject: [PATCH] Refuse a bulk status write that would re-stamp an already-done task A predicate (`multi: true`) update carries ONE payload for all N matched rows -- `driver.updateMany` takes a single SET clause -- so the `completed_at` that `task.hook.ts` stamps for a row genuinely transitioning into `done` was written to every row in the batch. A task completed days ago, swept into the same `status: 'done'` write, silently had its completion instant moved to now. ADR-0058 Addendum II D3 governs this: every per-row `beforeUpdate` context carries that one payload, so "a rewrite takes effect on the WHOLE batch, whichever row's dispatch made it", and a rewrite conditioned on the row is outside the contract. D3 names the sanctioned route -- per-row `previous` is supplied "so a guard can REFUSE the write, not so a rewrite can be aimed at one row". The hook now throws `DULY_TASK_BULK_ALREADY_DONE` (409), naming the row to remove. The guard is decided from the ROW alone -- its own pre-image plus the payload -- never from what an earlier dispatch left in the shared payload, so it holds in either dispatch order. It turns on `status` being in the payload, which is the only shape that computes a stamp, so an administrative bulk backfill and the seed's `mode: 'update'` backdating pass are untouched. Only the stamping direction is guarded: a batch moving rows out of `done` writes `completed_at = null`, which is correct for every row in such a batch and so is genuinely row-invariant. The `visible` predicate on `bulkActionDefs` is kept as the outer layer -- it stops the console assembling a batch the server would refuse -- but it is no longer the only thing between an import, a backfill, the dispatcher or an MCP caller and moved completion history. Its docblock, which claimed the predicate was the sole defence, is corrected. `test/task-actions.test.ts` previously PINNED the hazard (`.not.toBe(original)`); that assertion is replaced by the refusal, with the predicate half kept as its own case. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01SqkTcrxUFci7nqXdbBSe2p --- src/hooks/task.hook.ts | 62 ++++++++++++++- src/views/task.view.ts | 31 +++++--- test/task-actions.test.ts | 36 ++++++--- test/task-hook.test.ts | 162 ++++++++++++++++++++++++++++++++++++++ 4 files changed, 270 insertions(+), 21 deletions(-) diff --git a/src/hooks/task.hook.ts b/src/hooks/task.hook.ts index 854f79f..5ee7cfa 100644 --- a/src/hooks/task.hook.ts +++ b/src/hooks/task.hook.ts @@ -32,6 +32,48 @@ import type { Hook, HookContext } from '@objectstack/spec/data'; * holding exactly what the caller supplied, so a value derived here * replaces a caller-supplied one instead of being dropped with it. * + * ── The shared-payload path, and why this hook REFUSES it ──────────────── + * A predicate (`multi: true`) write carries ONE payload for all N matched rows + * — `driver.updateMany` takes a single SET clause — and ADR-0058 Addendum II + * governs what a hook may do with it. D1: a `before*` event dispatches once per + * matched row. D3: every per-row context carries THAT one payload, so "a + * rewrite takes effect on the WHOLE batch, whichever row's dispatch made it", + * and therefore "a rewrite CONDITIONED on the row (`ctx.previous`, + * `ctx.input.id`) is outside this contract". D3 names the sanctioned route in + * as many words: per-row `previous` is supplied "so a guard can REFUSE the + * write, not so a rewrite can be aimed at one row". + * + * `completed_at` is precisely such a rewrite: it is read off THIS row's + * pre-image. Measured against a booted engine — a batch of one `open` row and + * one already-`done` row, both written `status: 'done'` — the open row's + * dispatch stamps `completed_at = now` and the done row's completion instant, + * days old, is silently overwritten. Nothing errors; the history just moves. + * + * So on this path the hook throws. The guard is decided from the ROW ALONE — + * its own pre-image plus the payload — never from what an earlier dispatch left + * in the shared payload, because dispatch order is not the caller's to control + * and an accumulator would only catch the orders in which the done row happens + * to come second. + * + * Three boundaries this guard deliberately holds: + * + * - It turns on `status` being IN THE PAYLOAD. A predicate write that does not + * write `status` computes no stamp, so there is nothing to leak — an + * administrative bulk backfill over done rows, and the seed's `mode: + * 'update'` backdating pass, are untouched. + * - Only the stamping direction is guarded. A batch moving rows OUT of done + * writes `completed_at = null`, and null is the correct value for every row + * being moved out of done, including one that was never completed. That + * rewrite is genuinely row-invariant, so it is allowed. + * - A batch in which EVERY row is already done is refused too, even though + * nothing would leak. The hook cannot see the batch — `dispatch.index` is a + * position, not a total — and a rule stated on the row is one a caller can + * predict and a test can pin. Re-completing a done task in bulk is a caller + * mistake either way, and the answer is now loud instead of silent. + * + * The single-record path (`dispatch.mode === 'record'`) has a payload of its + * own, so the row-conditional stamp is sound there and is unchanged. + * * ── Why the handler is one self-contained function ─────────────────────── * `objectstack build` lowers an inline handler into a metadata `body`, and a * body ships without its module scope. A handler that referenced a @@ -76,6 +118,22 @@ const stampTaskLifecycle = (ctx: HookContext): void => { const wasDone = previous.status === 'done'; const isDone = nextStatus === 'done'; + // ── The shared-payload guard — refuse, never aim a rewrite at one row ── + // `ctx.dispatch` is the engine's own dispatch marker: `'record'` for a + // single-record write, `'per-row'` for one dispatch of a predicate write. + // Conditioned on `status` being in the payload because that is the only + // shape that computes a stamp at all — see the module header. + if (ctx.dispatch?.mode === 'per-row' && 'status' in input && wasDone && isDone) { + throw Object.assign( + new Error( + `Task ${String(input.id ?? previous.id ?? '')} is already done. A bulk status write carries one ` + + 'payload for every matched row, so completing this batch would overwrite that task\'s original ' + + 'completion timestamp. Leave the done rows out of the selection, or write them one at a time.', + ), + { code: 'DULY_TASK_BULK_ALREADY_DONE', status: 409 }, + ); + } + if (!wasDone && isDone) { input.completed_at = now; } else if (wasDone && !isDone) { @@ -125,7 +183,9 @@ export const TaskLifecycleHook: Hook = { description: 'Server-owned timestamps on duly_task: completed_at on the transition into and out of ' + 'done, and last_update_at only when status, note or skip_reason actually changed — ' - + 'never on an administrative or bulk write, which would reset the stagnation signal.', + + 'never on an administrative or bulk write, which would reset the stagnation signal. ' + + 'A predicate write that would re-stamp an already-done row is refused outright ' + + '(ADR-0058 Addendum II D3: one payload for the whole batch, so a guard throws).', // Explicit because it is load-bearing rather than a default worth inheriting: // if this handler throws, the write MUST be refused. Committing a task whose // stamps were not applied is the exact silent corruption the diff --git a/src/views/task.view.ts b/src/views/task.view.ts index 3d6d51e..943e37a 100644 --- a/src/views/task.view.ts +++ b/src/views/task.view.ts @@ -27,17 +27,28 @@ const columns = [ * habit and a chore, so this is not a convenience: it is the same interaction * budget as the row tick, applied to the week. * - * ── `visible` here is load-bearing, not decoration ──────────────────────── + * ── `visible` here is the OUTER of two layers ───────────────────────────── * It is evaluated once PER SELECTED RECORD, and the run covers only the rows - * that pass. That is what keeps an already-`done` row out of the batch — and - * it has to, for a reason that is MEASURED rather than theoretical: a - * predicate update carries ONE payload for all N rows (`driver.updateMany` - * takes one SET clause), so `task.hook.ts` stamping `completed_at` for a row - * that is genuinely transitioning writes that timestamp to the whole batch. - * Verified against a booted engine: bulk-completing a selection that already - * contains a done row moves that row's `completed_at` to now. The predicate - * is what makes such a selection unreachable from the UI; - * `test/task-actions.test.ts` pins both halves. + * that pass, which is what keeps an already-`done` row out of the batch. That + * matters for a reason that is MEASURED rather than theoretical: a predicate + * update carries ONE payload for all N rows (`driver.updateMany` takes one SET + * clause), so `task.hook.ts` stamping `completed_at` for a row that is + * genuinely transitioning would write that timestamp to the whole batch — + * silently re-dating a task completed days ago. + * + * A view predicate is a client-side hide, though, and the write it guards is + * server-side: an import, a backfill, the dispatcher or an MCP caller + * reassembles the same batch without ever reading this file. So the authority + * lives at the write. `task.hook.ts` REFUSES a predicate write that would + * re-stamp an already-done row (`DULY_TASK_BULK_ALREADY_DONE`, 409) — the one + * route ADR-0058 Addendum II D3 sanctions for a row-conditional decision on a + * batch-scoped payload. + * + * This predicate is kept because it is still the right UX: it stops the + * console from assembling a batch the server would refuse, so a user gets an + * unavailable action rather than an error they did not cause. + * `test/task-hook.test.ts` pins the refusal; `test/task-actions.test.ts` pins + * both layers. * * Labels are plain strings: an authored def is not i18n-resolved. That is a * real cost, accepted here because the repo carries no translation bundle yet diff --git a/test/task-actions.test.ts b/test/task-actions.test.ts index b62d382..a3cba0c 100644 --- a/test/task-actions.test.ts +++ b/test/task-actions.test.ts @@ -531,26 +531,42 @@ describe('bulk', () => { } }); - it('the visible predicate is what keeps an already-done row out of the batch', async () => { - // MEASURED, and the reason the predicate is load-bearing rather than - // decoration: a predicate update carries ONE payload for all N rows - // (`driver.updateMany` takes one SET clause), so the `completed_at` the - // hook stamps for a row that IS transitioning is written to every row in - // the batch — including one that was completed days ago. + it('an already-done row in the batch is refused by the SERVER, not merely hidden', async () => { + // The layer that actually decides. A predicate update carries ONE payload + // for all N rows (`driver.updateMany` takes one SET clause), so the + // `completed_at` the hook stamps for a row that IS transitioning would be + // written to every row in the batch — including one completed days ago. + // ADR-0058 Addendum II D3 hands a `before*` hook exactly one way out of + // that, and `task.hook.ts` takes it: refuse the write. + // + // This is the assertion that makes the `visible` predicate below a + // convenience rather than the only thing between a caller and moved + // history — an import, a backfill, the dispatcher or an MCP caller never + // goes near a view predicate. const open = (await newTask({ subject: 'still open' })).id; const alreadyDone = (await newTask({ subject: 'done last week' })).id; await dispatch(TASK_COMPLETE_ACTION, { recordId: alreadyDone }); const original = (await read(alreadyDone)).completed_at; await tick(); - await data.update('duly_task', { ...COMPLETE_PATCH }, { multi: true, where: { id: { $in: [open, alreadyDone] } } }); + const { code, status } = await refusal( + data.update('duly_task', { ...COMPLETE_PATCH }, { multi: true, where: { id: { $in: [open, alreadyDone] } } }), + ); + expect(code).toBe('DULY_TASK_BULK_ALREADY_DONE'); + expect(status).toBe(409); expect( (await read(alreadyDone)).completed_at, - 'a done row inside the batch has its completion instant overwritten — which is why the def excludes it', - ).not.toBe(original); + 'the original completion instant must survive a batch that tried to re-stamp it', + ).toBe(original); + expect((await read(open)).status, 'and the refusal writes nothing at all').toBe('open'); + }); - // So the declaration has to exclude it, and does. + it('and the visible predicate still keeps such a batch from being assembled', async () => { + // The outer layer, kept. `visible` is evaluated once per selected record + // and the run covers only the passing rows, so the console cannot build + // the batch the server now refuses. Defence in depth: the user gets a + // greyed-out row instead of an error they did not cause. const complete = allBulkDefs().find((d) => d.name === BULK_COMPLETE); const source = String(complete.visible?.source ?? ''); expect(source).not.toContain('"done"'); diff --git a/test/task-hook.test.ts b/test/task-hook.test.ts index 6c24387..a14e329 100644 --- a/test/task-hook.test.ts +++ b/test/task-hook.test.ts @@ -323,3 +323,165 @@ describe('last_update_at — the stagnation signal', () => { expect(edited.last_update_at as string < forged).toBe(true); }); }); + +// ── The shared-payload path: a predicate (bulk) write ────────────────────── +// +// A `multi: true` update carries ONE payload for all N matched rows — +// `driver.updateMany` takes a single SET clause — and ADR-0058 Addendum II D3 +// says what that means for a hook: every per-row `beforeUpdate` context +// carries THAT payload, so "a rewrite takes effect on the WHOLE batch, +// whichever row's dispatch made it". D3 names the consequence outright: a +// rewrite CONDITIONED on the row is *expressible and wrong*, and the sanctioned +// route for row-specific work in a `before*` hook is to THROW. +// +// `completed_at` is exactly such a rewrite — it is stamped on the TRANSITION, +// read off this row's own pre-image. So on this path the hook refuses instead +// of stamping. These assertions run against the real engine, and the dispatch +// mode they turn on is the engine's own (`ctx.dispatch.mode`), measured here +// rather than inferred. +describe('completed_at on a predicate write — one payload, N rows', () => { + /** Assert a refusal by its ENVELOPE (ADR-0112), never by the bare fact that it threw. */ + const refusal = async (promise: Promise) => { + try { + await promise; + } catch (error: any) { + return { code: error?.code, status: error?.status, message: String(error?.message ?? '') }; + } + throw new Error('expected the predicate write to be refused, but it resolved'); + }; + + const complete = async (id: string) => data.update('duly_task', { id, status: 'done' }); + + it('refuses a done batch that contains an already-done row, and does not move its clock', async () => { + // THE assertion this guard exists for. Without it the open row's dispatch + // stamps `completed_at = now` into the shared payload and the already-done + // row — completed days ago — is silently re-dated. + const open = (await newTask({ subject: 'still open' })).id; + const alreadyDone = (await newTask({ subject: 'done last week' })).id; + await complete(alreadyDone); + const original = (await read(alreadyDone)).completed_at; + expect(original).toBeTruthy(); + + await tick(); + const { code, status, message } = await refusal( + data.update('duly_task', { status: 'done' }, { multi: true, where: { id: { $in: [open, alreadyDone] } } }), + ); + + expect(code).toBe('DULY_TASK_BULK_ALREADY_DONE'); + expect(status).toBe(409); + expect(message, 'the refusal must name the row a caller has to remove').toContain(alreadyDone); + + expect( + (await read(alreadyDone)).completed_at, + 'the original completion instant must survive the refused batch', + ).toBe(original); + }); + + it('writes nothing at all — the refusal is not a partial batch', async () => { + const open = (await newTask({ subject: 'untouched by a refused batch' })).id; + const alreadyDone = (await newTask({ subject: 'already done' })).id; + await complete(alreadyDone); + + await refusal( + data.update('duly_task', { status: 'done' }, { multi: true, where: { id: { $in: [open, alreadyDone] } } }), + ); + + expect((await read(open)).status, 'the transitioning row must not commit either').toBe('open'); + expect((await read(open)).completed_at ?? null).toBeNull(); + }); + + it('refuses whichever dispatch order the batch arrives in', async () => { + // The guard is decided from the ROW alone — its own pre-image and the + // payload — never from what an earlier dispatch happened to leave behind. + // An accumulator would only catch the order in which the done row is + // dispatched second. + for (const doneFirst of [true, false]) { + const openRow = (await newTask({ subject: `order open ${doneFirst}` })).id; + const doneRow = (await newTask({ subject: `order done ${doneFirst}` })).id; + await complete(doneRow); + const original = (await read(doneRow)).completed_at; + await tick(); + + const ids = doneFirst ? [doneRow, openRow] : [openRow, doneRow]; + const { code } = await refusal( + data.update('duly_task', { status: 'done' }, { multi: true, where: { id: { $in: ids } } }), + ); + + expect(code, `done-first=${doneFirst} must refuse`).toBe('DULY_TASK_BULK_ALREADY_DONE'); + expect((await read(doneRow)).completed_at).toBe(original); + } + }); + + it('still completes a homogeneous batch — every row stamped, in one write', async () => { + // The negative control. The guard must refuse the mixed batch WITHOUT + // taking bulk complete away: a week of ticks in one gesture is the feature. + const ids: string[] = []; + for (let i = 0; i < 5; i += 1) ids.push((await newTask({ subject: `homogeneous ${i}` })).id); + + const affected = await data.update('duly_task', { status: 'done' }, { + multi: true, + where: { id: { $in: ids } }, + }); + expect(affected).toBe(5); + + for (const id of ids) { + const row = await read(id); + expect(row.status).toBe('done'); + expect(row.completed_at, `${id} must be stamped like any other write`).toBeTruthy(); + } + }); + + it('leaves an administrative predicate write alone — the guard turns on the STATUS in the payload', async () => { + // The over-refusal control, and the one that keeps the seed's second pass + // working: a bulk write that does not carry `status` computes no stamp, so + // there is nothing to leak and nothing to refuse — even over a done row. + const alreadyDone = (await newTask({ subject: 'admin backfill target' })).id; + await complete(alreadyDone); + const original = (await read(alreadyDone)).completed_at; + await tick(); + + await data.update('duly_task', { business_unit: 'bu_north' }, { + multi: true, + where: { id: { $in: [alreadyDone] } }, + }); + + const row = await read(alreadyDone); + expect(row.business_unit, 'a backfill must still land').toBe('bu_north'); + expect(row.completed_at, 'and must not disturb the completion instant').toBe(original); + }); + + it('does not fire on the single-record path — a re-save of a done task is still a no-op', async () => { + // `mode: 'record'` has a payload of its own, so the row-conditional stamp + // is sound there. Re-sending `status: 'done'` on a done task must keep + // behaving as it always has: accepted, and NOT re-stamped. + const task = await newTask(); + const done = await data.update('duly_task', { id: task.id, status: 'done' }); + const first = done.completed_at; + + await tick(); + const resaved = await data.update('duly_task', { id: task.id, status: 'done', note: 'after the fact' }); + + expect(resaved.completed_at, 'the by-id path is unchanged by the bulk guard').toBe(first); + }); + + it('a predicate write clearing done is NOT refused — that direction is row-invariant', async () => { + // Reopening a batch sets `completed_at = null`, and null is the correct + // value for EVERY row being moved out of done, including one that was + // never completed. Nothing row-specific leaks, so nothing is refused. + const wasDone = (await newTask({ subject: 'reopen me' })).id; + const neverDone = (await newTask({ subject: 'never completed' })).id; + await complete(wasDone); + + const affected = await data.update('duly_task', { status: 'in_progress' }, { + multi: true, + where: { id: { $in: [wasDone, neverDone] } }, + }); + expect(affected).toBe(2); + + for (const id of [wasDone, neverDone]) { + const row = await read(id); + expect(row.status).toBe('in_progress'); + expect(row.completed_at ?? null, `${id} must come out of done with no completion`).toBeNull(); + } + }); +});