diff --git a/src/hooks/task.hook.ts b/src/hooks/task.hook.ts index 5ee7cfa..4dc7374 100644 --- a/src/hooks/task.hook.ts +++ b/src/hooks/task.hook.ts @@ -74,6 +74,45 @@ import type { Hook, HookContext } from '@objectstack/spec/data'; * The single-record path (`dispatch.mode === 'record'`) has a payload of its * own, so the row-conditional stamp is sound there and is unchanged. * + * ── The same path and `last_update_at`: write nothing, do not refuse ───── + * The stagnation stamp is row-conditional too, in the other direction: it + * fires when THIS row's `status`, `note` or `skip_reason` differs from THIS + * row's pre-image. D3 applies unchanged — that value goes into the one shared + * payload and is written to every matched row, so a single genuine edit inside + * a 200-row bulk write refreshes all 200 clocks and the stalled list quietly + * empties. Measured: two open tasks, a `multi: true` write setting the same + * `note` both already needed, one of which already held it — the unchanged + * row's clock moved 6ms anyway, stamped by the other row's dispatch. + * + * The response here is the opposite of the guard above, and the difference is + * what is at stake. A wrong `completed_at` corrupts a historical fact, so that + * write must be refused. A `last_update_at` that was not refreshed corrupts + * nothing — the clock only moves forward — so refusing would cost a working + * feature to protect a value that, on this path, nothing reads. The honest + * answer is to write nothing. + * + * WHY nothing reads it, stated plainly because it is only safe while it stays + * true: stagnation is defined over OPEN work. `duly_stagnation` filters + * `status IN ('open','in_progress')` on every measure, and the "Not moving" + * lens does the same. The two bulk actions this product ships — `complete` + * (`status: 'done'`) and `skip` (`status: 'skipped'`) — move every row they + * touch OUT of that set, so a row leaving a bulk write with a stale clock is + * one no stagnation query will ever evaluate again. "Bulk completion would + * look like stagnation" describes a state that cannot occur. + * + * That premise is a fact about `bulkActionDefs`, not about this hook, and it + * is exactly what a bulk "set note" or a bulk reassign would break — silently, + * because the symptom is a frozen clock rather than an error. So it is pinned + * where it can go stale: `test/bulk-stagnation-premise.test.ts` reads the real + * defs out of `src/views/task.view.ts` and the real status set out of + * `duly_stagnation` and the `stalled` view, and fails if any bulk action's + * patch leaves rows inside the stagnation set. If that test ever goes red, the + * decision below is what has to change — not the test. + * + * The single-record path keeps the row-conditional stamp for this column too: + * `mode: 'record'` has a payload of its own, so there is no batch to leak + * onto. + * * ── 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 @@ -162,6 +201,22 @@ const stampTaskLifecycle = (ctx: HookContext): void => { // // Compared against the pre-image rather than merely tested for presence: a // re-save carrying an unchanged `status` is not progress. + // ── …and nothing at all on the shared-payload path ───────────────────── + // The loop below is row-conditional in the other direction from + // `completed_at`: it fires on a difference against THIS row's pre-image. So + // under D3 the value it writes lands in the one shared payload and is + // applied to every matched row — one genuine note edit inside a 200-row + // batch used to refresh all 200 clocks, measured. + // + // Unlike `completed_at` there is nothing here to corrupt — the clock only + // moves forward and no historical fact is overwritten — so the sanctioned + // refusal would cost a working feature to protect a value that, on this + // path, nothing reads. Same detection as the guard above, opposite response: + // write NOTHING rather than write one row's truth onto all of them. The + // module header carries why that is safe; the premise it rests on is guarded + // in `test/bulk-stagnation-premise.test.ts`. + if (ctx.dispatch?.mode === 'per-row') return; + for (const field of ['status', 'note', 'skip_reason']) { if (!(field in input)) continue; const next = input[field]; @@ -183,9 +238,10 @@ 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. ' - + '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).', + + 'never on an administrative write, which would reset the stagnation signal. On a ' + + 'predicate (bulk) write both row-conditional stamps are handled by ADR-0058 ' + + 'Addendum II D3: one payload for the whole batch, so a re-stamp of an already-done ' + + 'row is refused outright and last_update_at is not stamped at all.', // 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/test/bulk-stagnation-premise.test.ts b/test/bulk-stagnation-premise.test.ts new file mode 100644 index 0000000..08544ca --- /dev/null +++ b/test/bulk-stagnation-premise.test.ts @@ -0,0 +1,229 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +import { describe, expect, it } from 'vitest'; + +import { Stagnation } from '../src/datasets/index.js'; +import { Task } from '../src/objects/index.js'; +import { dulyViews } from '../src/views/index.js'; + +/** + * The premise under `task.hook.ts`'s decision NOT to stamp `last_update_at` on + * a predicate (bulk) write — and the one thing that can silently invalidate it. + * + * ── What the hook decided, and what it rests on ─────────────────────────── + * A `multi: true` write carries ONE payload for all N matched rows (ADR-0058 + * Addendum II D3), so a stamp computed from any single row's pre-image is + * written to every row in the batch. `task.hook.ts` therefore writes no + * `last_update_at` at all on that path — the honest answer, rather than + * writing one row's truth onto all of them. + * + * That is only harmless because of a fact about the BULK ACTIONS, not about + * the hook: stagnation is defined over OPEN work, and every bulk action this + * product ships moves the rows it touches out of that set. A row that leaves a + * bulk write with an unrefreshed clock is a row no stagnation query will ever + * evaluate again — the clock is a value nothing reads. + * + * ── Why this file exists ────────────────────────────────────────────────── + * Add a bulk "set note", a bulk reassign, or a bulk re-date, and the premise + * is false: those rows stay `open`, their clocks stay frozen, and the "Not + * moving" list quietly stops listing them. Nothing errors. There is no stack + * trace and no wrong number to notice — the signal simply goes silent, which + * is the exact failure the stagnation column was designed to prevent. + * + * So the premise is asserted here, against the REAL metadata on both sides: + * + * - the stagnation set is read out of `duly_stagnation`'s own measure filters + * and out of the "Not moving" lens's own filter, and the two are required + * to agree — a set restated here by hand would keep agreeing with itself + * after somebody widened the real one; + * - the bulk actions are read out of `dulyViews`, so a def added to any view + * bound to `duly_task` is inspected whether or not this file is edited. + * + * If this test goes red, `src/hooks/task.hook.ts` is what has to change — its + * per-row skip, or the new action. Do not relax the assertion. + */ + +type Rec = Record; + +const TASK_OBJECT = 'duly_task'; + +/** Every view in the app, flattened, with the key it is reachable under. */ +function everyView(): Array<{ where: string; view: Rec }> { + const out: Array<{ where: string; view: Rec }> = []; + for (const [i, group] of (dulyViews as Rec[]).entries()) { + if (group?.list) out.push({ where: `dulyViews[${i}].list`, view: group.list }); + for (const [key, view] of Object.entries((group?.listViews ?? {}) as Rec)) { + if (view) out.push({ where: `dulyViews[${i}].listViews.${key}`, view: view as Rec }); + } + } + return out; +} + +/** Views bound to `duly_task` — the only ones the stagnation clock is about. */ +const taskViews = () => everyView().filter(({ view }) => view?.data?.object === TASK_OBJECT); + +/** Every bulk def declared on a `duly_task` view, with where it was found. */ +function taskBulkDefs(): Array<{ where: string; def: Rec }> { + return taskViews().flatMap(({ where, view }) => + ((view.bulkActionDefs ?? []) as Rec[]).map((def) => ({ where: `${where}.bulkActionDefs`, def })), + ); +} + +// ── The stagnation set, read from the two places that define it ──────────── + +/** + * `duly_stagnation` is the stagnation picture; every measure on it filters the + * population it counts. Union across the measures rather than reaching for one + * by name, so a measure added with a wider status filter widens this set too. + */ +function statusesFromDataset(): string[] { + const seen = new Set(); + for (const measure of (Stagnation as Rec).measures as Rec[]) { + const clause = measure?.filter?.status?.$in; + if (Array.isArray(clause)) for (const value of clause) seen.add(String(value)); + } + return [...seen].sort(); +} + +/** + * The "Not moving" lens, found STRUCTURALLY: a stagnation lens is any + * `duly_task` view that filters on `last_update_at`. Found by shape rather + * than by the key `stalled`, so a second lens added later is read too. + */ +function stagnationLenses(): Array<{ where: string; statuses: string[] }> { + const out: Array<{ where: string; statuses: string[] }> = []; + for (const { where, view } of taskViews()) { + const filter = (view.filter ?? []) as Rec[]; + if (!filter.some((f) => f?.field === 'last_update_at')) continue; + const status = filter.find((f) => f?.field === 'status' && f?.operator === 'in'); + out.push({ where, statuses: [...((status?.value ?? []) as string[])].map(String).sort() }); + } + return out; +} + +describe('the stagnation set — read, not restated', () => { + it('duly_stagnation declares one', () => { + const statuses = statusesFromDataset(); + expect( + statuses.length, + 'no status filter found on duly_stagnation — this guard would be vacuous', + ).toBeGreaterThan(0); + // Every value must be a real option, or the dataset counts nothing and the + // guard below would be comparing against a set that matches no row. + const declared = (Task.fields.status as { options: { value: string }[] }).options.map((o) => o.value); + for (const status of statuses) expect(declared, `${status} must be a duly_task status`).toContain(status); + }); + + it('every measure on it agrees — one population, not a per-measure opinion', () => { + const union = statusesFromDataset(); + for (const measure of (Stagnation as Rec).measures as Rec[]) { + expect( + [...((measure?.filter?.status?.$in ?? []) as string[])].map(String).sort(), + `measure '${measure?.name}' must count the same population as the rest`, + ).toEqual(union); + } + }); + + it('the "Not moving" lens is scoped to the SAME set as the dataset', () => { + const lenses = stagnationLenses(); + expect(lenses.length, 'no view filters on last_update_at — the lens has gone missing').toBeGreaterThan(0); + for (const lens of lenses) { + expect( + lens.statuses, + `${lens.where} shows a different population than duly_stagnation counts`, + ).toEqual(statusesFromDataset()); + } + }); +}); + +// ── The guard ────────────────────────────────────────────────────────────── + +describe('every bulk action must move its rows OUT of the stagnation set', () => { + /** + * The union of both sources. Union rather than either one alone is the + * fail-safe direction: if they ever disagree (the test above goes red at the + * same time) this guard is the stricter of the two readings, never the + * looser. + */ + const stagnationStatuses = () => + [...new Set([...statusesFromDataset(), ...stagnationLenses().flatMap((l) => l.statuses)])].sort(); + + it('found the real defs — a renamed key must not make this vacuous', () => { + const found = taskBulkDefs(); + expect(found.length, 'no bulkActionDefs found on any duly_task view').toBeGreaterThan(0); + const names = found.map(({ def }) => def?.name); + // The two the hook's decision was measured against. Not a list to maintain + // — it is proof that the walk above reaches `src/views/task.view.ts` and + // did not silently return nothing after a rename or a refactor. + expect(names).toContain('duly_task_bulk_complete'); + expect(names).toContain('duly_task_bulk_skip'); + }); + + it('the same def name means the same patch on every view offering it', () => { + const byName = new Map>(); + for (const entry of taskBulkDefs()) { + const list = byName.get(entry.def?.name) ?? []; + list.push(entry); + byName.set(entry.def?.name, list); + } + for (const [name, entries] of byName) { + const first = JSON.stringify(entries[0].def?.patch ?? null); + for (const entry of entries) { + expect( + JSON.stringify(entry.def?.patch ?? null), + `'${name}' writes a different patch at ${entry.where} — one variant could leave rows stagnant`, + ).toBe(first); + } + } + }); + + it.each(taskBulkDefs().map(({ where, def }) => [String(def?.name ?? '(unnamed)'), where, def] as const))( + '%s writes a status outside the stagnation set', + (name, where, def) => { + const stagnant = stagnationStatuses(); + + // 1. Only a declarative data-plane patch can be certified from here. An + // `operation: 'custom'` def runs a handler this file cannot read, so + // the premise has to be re-argued by hand rather than assumed. + expect( + def?.operation, + `${name} at ${where} is not an 'update' — task.hook.ts's per-row skip cannot be certified ` + + 'against it; re-argue the premise on the card before adding it', + ).toBe('update'); + + // 2. A patch with no `status` leaves every row exactly where it was. If + // any of them were open, their clocks are now frozen and invisible. + const patch = (def?.patch ?? {}) as Rec; + expect( + Object.prototype.hasOwnProperty.call(patch, 'status'), + `${name} writes no status, so its rows keep the one they had — including ${stagnant.join('/')}, ` + + 'whose stagnation clock task.hook.ts no longer refreshes on this path', + ).toBe(true); + + // 3. It must be a fixed value. A caller-chosen status is a status this + // file cannot certify. + expect(typeof patch.status, `${name} must write a literal status`).toBe('string'); + const declared = (Task.fields.status as { options: { value: string }[] }).options.map((o) => o.value); + expect(declared, `${name} writes '${String(patch.status)}', which is not a duly_task status`) + .toContain(patch.status); + + // 4. …and outside the stagnation set. THE assertion. + expect( + stagnant, + `${name} leaves its rows in the stagnation set ('${String(patch.status)}'). ` + + 'task.hook.ts skips the last_update_at stamp on the bulk path because every bulk action ' + + 'moves its rows out of that set — this def breaks that premise, so those rows will sit in ' + + '"Not moving" with a clock that never advances again. Fix the hook, not this test.', + ).not.toContain(patch.status); + + // 5. A param may not put `status` back in play: params are collected + // from the user and merged OVER the static patch, so a `status` param + // hands the caller the very choice step 3 refused. + const params = (def?.params ?? []) as Rec[]; + expect( + params.map((p) => p?.name), + `${name} exposes status as a parameter, which overrides the patch above`, + ).not.toContain('status'); + }, + ); +}); diff --git a/test/task-hook.test.ts b/test/task-hook.test.ts index a14e329..e7e7552 100644 --- a/test/task-hook.test.ts +++ b/test/task-hook.test.ts @@ -485,3 +485,153 @@ describe('completed_at on a predicate write — one payload, N rows', () => { } }); }); + +// ── The shared-payload path again: last_update_at ────────────────────────── +// +// Same mechanism as the `completed_at` block above, opposite response. +// +// The stamp is row-conditional in the other direction: it fires when THIS +// row's `status`, `note` or `skip_reason` differs from THIS row's pre-image. +// Under D3 that value lands in the one shared payload and is written to every +// matched row, so a single genuine edit inside a 200-row batch refreshes all +// 200 clocks. Unlike `completed_at`, there is nothing to corrupt — the clock +// only moves forward, and nothing historical is overwritten — so refusing the +// write would cost a feature to protect a value that, on this path, nothing +// reads. The hook writes nothing instead. +// +// Why writing nothing is SAFE and not merely convenient: stagnation is defined +// over open work (`duly_stagnation` and the "Not moving" lens both filter +// `status IN ('open','in_progress')`), and both bulk actions this product +// ships move every row they touch OUT of that set. An unrefreshed clock on a +// done or skipped row is a value nothing will ever evaluate again. +// +// That premise is a fact about `bulkActionDefs`, not about this hook, so it is +// guarded where it can go stale: `test/bulk-stagnation-premise.test.ts` fails +// if a bulk action is ever added whose patch leaves rows inside the stagnation +// set. +describe('last_update_at on a predicate write — one payload, N rows', () => { + const SHARED_NOTE = 'chased the vendor, all of them'; + + it('does NOT advance the clock of a row that changed nothing', async () => { + // THE assertion. A batch of exactly the shape the defect was measured on: + // row A genuinely changes its note, row B already holds that same note. + // Row B's `status`, `note` and `skip_reason` are all unchanged, so nothing + // about row B is progress — but row A's dispatch used to write the stamp + // into the shared payload and row B's clock moved with it. + const changing = (await newTask({ subject: 'row A — the genuine edit', note: 'as found' })).id; + const unchanged = (await newTask({ subject: 'row B — already holds it', note: SHARED_NOTE })).id; + const before = (await read(unchanged)).last_update_at as string; + + await tick(); + const affected = await data.update('duly_task', { note: SHARED_NOTE }, { + multi: true, + where: { id: { $in: [changing, unchanged] } }, + }); + expect(affected).toBe(2); + + const rowB = await read(unchanged); + expect(rowB.note, 'the batch must still land — this is not a refusal').toBe(SHARED_NOTE); + expect( + rowB.last_update_at, + "row A's edit must not quiet row B's stagnation clock", + ).toBe(before); + }); + + it('does not advance the clock of the row that DID change either', async () => { + // The honest statement of the decision, pinned so a later "restore it just + // for the row that changed" cannot land quietly: there is no such thing on + // this path. One payload, N rows — a stamp aimed at the changing row IS + // the stamp every other row receives. + const changing = (await newTask({ subject: 'row A alone', note: 'as found' })).id; + const bystander = (await newTask({ subject: 'row B alone', note: SHARED_NOTE })).id; + const before = (await read(changing)).last_update_at as string; + + await tick(); + await data.update('duly_task', { note: SHARED_NOTE }, { + multi: true, + where: { id: { $in: [changing, bystander] } }, + }); + + const rowA = await read(changing); + expect(rowA.note, 'the edit itself still commits').toBe(SHARED_NOTE); + expect(rowA.last_update_at, 'no row is stamped on the shared-payload path').toBe(before); + }); + + it('leaves an all-unchanged batch alone, and commits it', async () => { + // The control for the option this decision rejected. Refusing any batch + // containing an unchanged row would have refused this one too, where + // nothing leaks because nothing is stamped. + const ids: string[] = []; + for (let i = 0; i < 3; i += 1) { + ids.push((await newTask({ subject: `all unchanged ${i}`, note: SHARED_NOTE })).id); + } + const before = await Promise.all(ids.map(async (id) => (await read(id)).last_update_at as string)); + + await tick(); + const affected = await data.update('duly_task', { note: SHARED_NOTE }, { + multi: true, + where: { id: { $in: ids } }, + }); + expect(affected).toBe(3); + + for (const [i, id] of ids.entries()) { + expect((await read(id)).last_update_at, `${id} was not touched`).toBe(before[i]); + } + }); + + it('bulk complete still completes — the unrefreshed clock is a value nothing reads', async () => { + // The cost of the decision, measured rather than asserted away. `done` is + // outside the stagnation set, so a task that comes out of this batch with + // a stale `last_update_at` can never be counted as stalled again. + const ids: string[] = []; + for (let i = 0; i < 3; i += 1) ids.push((await newTask({ subject: `bulk complete ${i}` })).id); + const before = await Promise.all(ids.map(async (id) => (await read(id)).last_update_at as string)); + + await tick(); + const affected = await data.update('duly_task', { status: 'done' }, { + multi: true, + where: { id: { $in: ids } }, + }); + expect(affected).toBe(3); + + for (const [i, id] of ids.entries()) { + const row = await read(id); + expect(row.status).toBe('done'); + expect(row.completed_at, 'completed_at is still stamped — it is a different column').toBeTruthy(); + expect(row.last_update_at, 'and the stagnation clock is left where it was').toBe(before[i]); + } + }); + + it('bulk skip still skips, reason and all', async () => { + const ids: string[] = []; + for (let i = 0; i < 3; i += 1) ids.push((await newTask({ subject: `bulk skip ${i}` })).id); + const before = await Promise.all(ids.map(async (id) => (await read(id)).last_update_at as string)); + + await tick(); + await data.update('duly_task', { status: 'skipped', skip_reason: 'plant shutdown, week 34' }, { + multi: true, + where: { id: { $in: ids } }, + }); + + for (const [i, id] of ids.entries()) { + const row = await read(id); + expect(row.status).toBe('skipped'); + expect(row.skip_reason).toBe('plant shutdown, week 34'); + expect(row.last_update_at, 'skipped is outside the stagnation set too').toBe(before[i]); + } + }); + + it('does NOT leak into the single-record path — a by-id note edit still stamps', async () => { + // The boundary. `mode: 'record'` has a payload of its own, so the + // row-conditional stamp is sound there and stays exactly as it was. This + // is the assertion that would go red if the skip were written as "never + // stamp on update" instead of "never stamp on the shared payload". + const task = await newTask({ note: 'as found' }); + const before = task.last_update_at as string; + + await tick(); + const edited = await data.update('duly_task', { id: task.id, note: SHARED_NOTE }); + + expect(edited.last_update_at as string > before, 'the by-id path is unchanged').toBe(true); + }); +});