diff --git a/.changeset/todo-completion-date-hook-stamp.md b/.changeset/todo-completion-date-hook-stamp.md new file mode 100644 index 0000000000..ce1e02c08a --- /dev/null +++ b/.changeset/todo-completion-date-hook-stamp.md @@ -0,0 +1,62 @@ +--- +"@objectstack/example-todo": patch +--- + +fix(example-todo): a normal user can mark a task complete again — `completed_date` is stamped by the hook instead of demanded from the caller (#7036) + +`examples/app-todo` shipped two declarations on `todo_task` that could not both hold on +the update path, so the app's headline action was unsatisfiable by construction. + +**Before.** `completed_date` is `Field.datetime({ readonly: true })`, and `readonly` is a +two-part contract: never editable in forms, **and** a non-system caller's write to it is +stripped from the payload on the update path. The same object then declares a validation +rule, `completed_date_required`, refusing any record whose `status` is `completed` while +`completed_date` is blank. The strip runs first, so a payload carrying both keys lost +`completed_date` and was then rejected for missing it. Measured against the real object on +a real kernel: + +``` +update status+completed_date (user ctx): REJECTED -> Completed date is required when status is Completed +update status only (user ctx): REJECTED -> Completed date is required when status is Completed +update status+completed_date (isSystem): OK +insert already-completed: OK +``` + +Both escapes are non-user paths — an elevated write bypasses the strip, and a create may +legitimately seed a read-only column. Every ordinary user update was refused, which made +the app's own `completeTask` and `massCompleteTasks` handlers fail every time they ran. + +**After.** The column is server-owned, so the server writes it. `task.hook.ts` gains a +`beforeUpdate` leg that stamps `completed_date` on the transition into `completed` and +clears it on the transition back out; `completeTask` and `massCompleteTasks` now send +`status` alone. A one-key user-context update completes the task and persists the stamp. + +This works because the readonly strip is deliberately narrow rather than because it is +bypassed: it runs *after* the before-hooks and deletes a key only when the caller supplied +it **and** it still holds the caller's own value (`stripReadonlyFields`, the +`suppliedValues` snapshot plus the `Object.is` identity check). A value a hook wrote is a +platform value and survives — including when the caller echoed the same key back, which is +what a whole-record form PUT does. The stamp is therefore written unconditionally: leaving +a caller-supplied value in place would leave the caller's own value on the key, and the +strip would delete it. + +`completed_date_required` stays, and is now the assertion that the stamp actually +happened — if the hook is ever unregistered or its transition guard breaks, the write is +refused loudly instead of committing a completed task with no completion date. + +**Two related repairs the fix required.** + +- The hook was never registered. `task_logic` was not in `defineStack({ hooks })`, and + `collectBundleHooks` reads that array and nothing else, so the whole file was dead + metadata: it type-checked, it read as wired, and it never ran. Both sibling example apps + already declare `hooks: allHooks`; `app-todo` now does too. +- Both existing legs read the record off `ctx.input` rather than `ctx.input.data`. + `HookContext.input` is an envelope — `{ data, options }` on insert, `{ id, data, options }` + on update — so `ctx.input.priority = 'normal'` set a key no write path reads. The insert + defaults and the after-update branches had never had any effect; they do now. The + after-update logging also moved from `console` to the kernel logger reached through + `ctx.ql`, so it honours the configured log level. + +Reopening a completed task clears `completed_date`, documented in the object and hook +metadata: the field means "when this task was completed", so a task that is not completed +must not carry a stale one. An edit that carries no `status` is not treated as a reopen. diff --git a/examples/app-todo/objectstack.config.ts b/examples/app-todo/objectstack.config.ts index ab46b17dcb..8d3b83ecee 100644 --- a/examples/app-todo/objectstack.config.ts +++ b/examples/app-todo/objectstack.config.ts @@ -4,6 +4,11 @@ import { defineStack } from '@objectstack/spec'; // ─── Barrel Imports (one per metadata type) ───────────────────────── import * as objects from './src/objects/index.js'; +// [#7036] Lifecycle hooks are NOT collected from the objects barrel — the +// runtime reads them from `defineStack({ hooks })` only (`collectBundleHooks`). +// An unregistered `*.hook.ts` file is dead metadata: it type-checks, it reads +// as wired, and it never runs. +import taskHook from './src/objects/task.hook.js'; import * as actions from './src/actions/index.js'; import * as dashboards from './src/dashboards/index.js'; import * as datasets from './src/datasets/index.js'; @@ -45,6 +50,9 @@ export default defineStack({ // Seed Data (top-level, registered as metadata) data: TodoSeedData, + // Object Lifecycle Hooks (same shape as app-crm / app-showcase) + hooks: [taskHook], + // Auto-collected from barrel index files via Object.values() objects: Object.values(objects), views: Object.values(views), diff --git a/examples/app-todo/src/actions/task.handlers.ts b/examples/app-todo/src/actions/task.handlers.ts index 714d36a48e..ea100b6ddf 100644 --- a/examples/app-todo/src/actions/task.handlers.ts +++ b/examples/app-todo/src/actions/task.handlers.ts @@ -31,12 +31,20 @@ interface ActionContext { params?: Record; } -/** Mark a single task as complete */ +/** + * Mark a single task as complete. + * + * [#7036] `status` only. `completed_date` is `readonly` — server-owned — so a + * caller's write to it is stripped from the payload before the record is + * validated, and sending it here made this action refuse itself against + * `todo_task`'s `completed_date_required` rule. The stamp belongs to the + * `beforeUpdate` leg of `src/objects/task.hook.ts`, which runs on the + * transition and whose write the strip lets through. + */ export async function completeTask(ctx: ActionContext): Promise { const { record, engine } = ctx; await engine.update('todo_task', record.id as string, { status: 'completed', - completed_date: new Date().toISOString(), }); } @@ -59,15 +67,13 @@ export async function cloneTask(ctx: ActionContext): Promise<{ id: string }> { }); } -/** Mark all selected tasks as complete (bulk) */ +/** Mark all selected tasks as complete (bulk) — same `status`-only rule as {@link completeTask} (#7036) */ export async function massCompleteTasks(ctx: ActionContext): Promise { const { params, engine } = ctx; const ids = (params?.selectedIds ?? []) as string[]; - const now = new Date().toISOString(); for (const id of ids) { await engine.update('todo_task', id, { status: 'completed', - completed_date: now, }); } } diff --git a/examples/app-todo/src/objects/task.hook.ts b/examples/app-todo/src/objects/task.hook.ts index 781eb3e519..b9d98f1402 100644 --- a/examples/app-todo/src/objects/task.hook.ts +++ b/examples/app-todo/src/objects/task.hook.ts @@ -2,37 +2,112 @@ import { HookContext, Hook } from '@objectstack/spec/data'; +/** + * Lifecycle logic for `todo_task` — insert defaults and the completion stamp. + * + * ## `ctx.input` is an ENVELOPE, not the record (#7036) + * + * The engine builds one context shape per event and the record is always a + * slot inside it, never the context's own keys: + * + * - `beforeInsert` → `{ data, options }` (one context per row) + * - `beforeUpdate` → `{ id, data, options }` + * - `afterUpdate` → `{ id, data, options }` + * + * (the contract table on `HookContextSchema.input`, pinned against the real + * engine in `packages/objectql/src/hook-input-shape-contract.test.ts`). So the + * record lives at `ctx.input.data`; writing `ctx.input.priority` sets a key on + * the envelope that no write path ever reads. + * + * ## Why the completion stamp is written HERE, and unconditionally + * + * `completed_date` is `readonly: true` — a server-owned column. On the update + * path the engine strips a non-system caller's write to such a column, and + * `todo_task`'s own `completed_date_required` rule then refused the write for + * missing exactly the value it had just dropped. That made "Complete task" + * impossible for an ordinary user (#7036). + * + * A hook stamp is the platform's answer, and it works because the strip is + * deliberately narrow: it runs AFTER the before-hooks and only deletes a key + * that both (a) the caller supplied and (b) still holds *the caller's own + * value* (#2948 + #5591 — `stripReadonlyFields`). A value a hook wrote is a + * platform value and survives. + * + * That is also why the stamp below is UNCONDITIONAL rather than + * `data.completed_date ??= …`: if a caller supplied the key and the hook left + * it alone, the value would still be the caller's, the strip would delete it, + * and the rule would refuse the write again — the original bug, restored. The + * server owning the column means the server writes it on every transition. + * + * ## Leaving `completed` clears the stamp + * + * Reopening a completed task (completed → in_progress, …) nulls + * `completed_date`. The field means "when this task was completed", so a task + * that is not completed must not carry one; retaining it would leave a stale + * timestamp that every report and list view reads as fact. The same + * `readonly`/strip reasoning applies — only a hook can write the clear. + */ const taskHook: Hook = { name: 'task_logic', object: 'todo_task', - events: ['beforeInsert', 'afterUpdate'], + events: ['beforeInsert', 'beforeUpdate', 'afterUpdate'], handler: async (ctx: HookContext) => { + const data = (ctx.input as { data?: Record }).data; + const previous = ctx.previous as Record | undefined; + if (!data) return; + if (ctx.event === 'beforeInsert') { - const { input } = ctx; // Default priority - if (!input.priority) { - input.priority = 'normal'; + if (!data.priority) { + data.priority = 'normal'; } // Default status - if (!input.status) { - input.status = 'not_started'; + if (!data.status) { + data.status = 'not_started'; } // Validation - if (typeof input.subject === 'string' && input.subject.includes('spam')) { + if (typeof data.subject === 'string' && data.subject.includes('spam')) { throw new Error('Spam tasks are not allowed'); } } - + + if (ctx.event === 'beforeUpdate') { + // The transition INTO completed — `previous` is the engine's pre-update + // snapshot, bound before this hook runs, so "is this a transition?" is + // answerable here without a read of our own. + if (data.status === 'completed' && previous?.status !== 'completed') { + data.completed_date = new Date().toISOString(); + } + // ...and the transition back OUT of it. Guarded on `status` actually + // being part of this write: an unrelated edit of a completed task + // (`{ progress_percent: 100 }`) carries no `status` and must not be read + // as a reopen. + else if ( + data.status !== undefined && + data.status !== 'completed' && + previous?.status === 'completed' + ) { + data.completed_date = null; + } + } + if (ctx.event === 'afterUpdate') { + // The kernel's logger, reached through the engine handle the context + // carries. Not `console`: a hook runs inside the server, so its output + // belongs on the platform logger, which honours the kernel's configured + // level (a test booting `{ logger: { level: 'silent' } }` stays silent). + // `ctx.ql` is declared `unknown` on `HookContextSchema`, hence the cast. + const logger = (ctx.ql as { logger?: { info?: (message: string) => void } } | undefined)?.logger; + // Check if completed - if (ctx.input.status === 'completed' && ctx.previous && ctx.previous.status !== 'completed') { - console.log(`Task ${ctx.id} completed by ${ctx.session?.userId || 'unknown'}`); + if (data.status === 'completed' && previous && previous.status !== 'completed') { + logger?.info?.(`Task ${ctx.input.id} completed by ${ctx.session?.userId || 'unknown'}`); // Could trigger notifications or integrations here } - + // Check if task became overdue - if (ctx.input.is_overdue && ctx.previous && !ctx.previous.is_overdue) { - console.log(`Task ${ctx.id} is now overdue`); + if (data.is_overdue && previous && !previous.is_overdue) { + logger?.info?.(`Task ${ctx.input.id} is now overdue`); } } } diff --git a/examples/app-todo/src/objects/task.object.ts b/examples/app-todo/src/objects/task.object.ts index cdc9d39e35..393e8d5bc4 100644 --- a/examples/app-todo/src/objects/task.object.ts +++ b/examples/app-todo/src/objects/task.object.ts @@ -72,6 +72,12 @@ export const Task = ObjectSchema.create({ label: 'Reminder Date/Time', }), + // [#7036] Server-owned: `readonly` means "never editable in forms, AND a + // non-system caller's write is stripped on the write path". Nothing may + // hand this value in — `task.hook.ts` stamps it on the transition into + // `completed` and clears it on the transition back out, which is the one + // write the readonly strip is designed to let through (#2948/#5591). + // Callers (including `actions/task.handlers.ts`) send `status` alone. completed_date: Field.datetime({ label: 'Completed Date', readonly: true, @@ -186,6 +192,14 @@ export const Task = ObjectSchema.create({ highlightFields: ['subject', 'status', 'priority', 'due_date', 'owner'], validations: [ + // [#7036] This rule is satisfied by the SERVER, not by the caller. The + // `beforeUpdate` leg of `task.hook.ts` stamps `completed_date` before + // validation runs, so a completion write that carries only + // `status: 'completed'` passes. It stays as a rule rather than being + // deleted because it is the assertion that the stamp actually happened: + // if the hook is ever unregistered or its transition guard breaks, the + // write is refused loudly instead of committing a completed task with no + // completion date. { name: 'completed_date_required', type: 'script', @@ -206,8 +220,13 @@ export const Task = ObjectSchema.create({ // field — it was silently stripped at build and never ran (ADR-0032 "no // silent failure"). Record-triggered automation for this object lives in the // supported mechanisms instead: - // • `task.hook.ts` — lifecycle hook (defaults, completion logic) - // • `actions/task.handlers.ts` — stamps `completed_date` on completion + // • `task.hook.ts` — lifecycle hook (insert defaults; stamps and + // clears `completed_date` on the completion + // transition). Registered via + // `defineStack({ hooks })` in + // `objectstack.config.ts` — a hook that is not + // in that array never runs (#7036). + // • `actions/task.handlers.ts` — flips `status`; the stamp is the hook's // • `flows/task.flow.ts` — record_change + schedule flows (completion / // recurrence, reminders, overdue escalation) }); diff --git a/examples/app-todo/test/task-completion-trigger.test.ts b/examples/app-todo/test/task-completion-trigger.test.ts index 9837c8e674..5387819508 100644 --- a/examples/app-todo/test/task-completion-trigger.test.ts +++ b/examples/app-todo/test/task-completion-trigger.test.ts @@ -43,6 +43,8 @@ import { RecordChangeTriggerPlugin } from '@objectstack/trigger-record-change'; import { allFlows, TaskCompletionFlow } from '../src/flows/index.js'; import { Task } from '../src/objects/task.object.js'; +import taskHook from '../src/objects/task.hook.js'; +import TodoApp from '../objectstack.config.js'; const sleep = (ms: number) => new Promise((r) => setTimeout(r, ms)); @@ -82,6 +84,13 @@ async function bootTodoKernel(): Promise<{ objectql.registry.registerObject(Task, 'todo', 'todo'); await objectql.syncSchemas(); + // [#7036] The app's lifecycle hook, bound the way `AppPlugin` binds it from + // `defineStack({ hooks })`. This kernel is assembled by hand (no AppPlugin), + // so without this line the completion stamp would not exist here and every + // completion below would be refused — which is exactly the bug, and exactly + // why the old version of this file seeded `completed_date` on CREATE. + objectql.bindHooks([taskHook], { packageId: 'app:com.example.todo' }); + for (const flow of allFlows) automation.registerFlow(flow.name, flow); return { automation, data }; } @@ -170,15 +179,15 @@ describe('#6882 — app-todo `task_completion` is armed, not dead', () => { const ctx = { context: { userId: 'u_todo' } }; const runCount = async () => (await automation.listRuns('task_completion')).length; - // `completed_date` is seeded on CREATE (the engine allows a read-only field - // to be seeded by an insert, and the object's `completed_date_required` - // rule refuses the completion write otherwise — see the note on that rule). + // [#7036] No `completed_date` seed. The CREATE-seed workaround this test + // used to carry existed only because the completion UPDATE was impossible; + // the task is created exactly as a user creates one, and the transition + // below is a real user-context write. const created = await data.insert('todo_task', { subject: 'Water the plants', status: 'not_started', priority: 'normal', is_recurring: false, - completed_date: '2026-08-09T10:00:00.000Z', }, ctx); const id = Array.isArray(created) ? created[0].id : created.id; await sleep(150); @@ -205,3 +214,164 @@ describe('#6882 — app-todo `task_completion` is armed, not dead', () => { expect(await runCount(), 're-saving a completed task must not re-fire it').toBe(1); }, 30000); }); + +/** + * [#7036] An ordinary user can complete a task. + * + * The defect was a pair of metadata declarations that could not both hold on + * the update path: `completed_date` is `readonly` (a non-system caller's write + * is stripped) and `completed_date_required` then refused the write for + * missing the value that had just been stripped. Measured before the fix, with + * the app's real object on a real kernel: + * + * update status+completed_date (user ctx): REJECTED -> Completed date is required when status is Completed + * update status only (user ctx): REJECTED -> Completed date is required when status is Completed + * update status+completed_date (isSystem): OK + * insert already-completed: OK + * + * — i.e. every escape was a NON-user path, and `completeTask` always failed. + * + * The repair is the server owning the column: `task.hook.ts` stamps it on the + * transition, and the strip lets a hook's write through because it only + * deletes a key that still holds the *caller's own* value (#2948/#5591). The + * assertions below are written against that seam rather than against the + * message, so they stay meaningful if the wording changes. + */ +describe('#7036 — completing a task is possible for a normal user', () => { + const ctx = { context: { userId: 'u_todo' } }; + + const newTask = async (data: any, extra: Record = {}) => { + const created = await data.insert('todo_task', { + subject: 'Water the plants', + status: 'not_started', + priority: 'normal', + is_recurring: false, + ...extra, + }, ctx); + return Array.isArray(created) ? created[0].id : created.id; + }; + const read = async (data: any, id: string) => + await data.findOne('todo_task', { where: { id }, ...ctx }); + + it('THE CASE: a status-only user update completes the task and stamps the date', async () => { + const { data } = await bootTodoKernel(); + const id = await newTask(data); + + // Exactly what `completeTask` now sends — one key, no `completed_date`. + await data.update('todo_task', { status: 'completed' }, { where: { id }, ...ctx }); + + const row = await read(data, id); + expect(row.status).toBe('completed'); + // The stamp is PRESENT and is a real timestamp, not an empty string that + // would merely satisfy `isBlank`. + expect(row.completed_date).toBeTruthy(); + expect(Number.isNaN(Date.parse(String(row.completed_date)))).toBe(false); + }, 30000); + + it('a caller that still sends `completed_date` is not punished for it — the hook value wins', async () => { + // The #5591 direction: the caller echoes the key back (a form PUT of the + // whole record does this). The strip must not delete the hook's stamp just + // because the caller also named the column, and the caller's value must + // not be what lands. + const { data } = await bootTodoKernel(); + const id = await newTask(data); + const forged = '1999-01-01T00:00:00.000Z'; + + await data.update( + 'todo_task', + { status: 'completed', completed_date: forged }, + { where: { id }, ...ctx }, + ); + + const row = await read(data, id); + expect(row.status).toBe('completed'); + expect(row.completed_date).toBeTruthy(); + expect(row.completed_date).not.toBe(forged); + }, 30000); + + it('leaving `completed` clears the stamp; an unrelated edit does not', async () => { + const { data } = await bootTodoKernel(); + const id = await newTask(data); + + await data.update('todo_task', { status: 'completed' }, { where: { id }, ...ctx }); + const stamped = (await read(data, id)).completed_date; + expect(stamped).toBeTruthy(); + + // An edit that carries no `status` must not be read as a reopen. + await data.update('todo_task', { progress_percent: 100 }, { where: { id }, ...ctx }); + expect((await read(data, id)).completed_date).toBe(stamped); + + // A real reopen clears it — the documented choice (see `task.hook.ts`). + await data.update('todo_task', { status: 'in_progress' }, { where: { id }, ...ctx }); + const reopened = await read(data, id); + expect(reopened.status).toBe('in_progress'); + expect(reopened.completed_date == null).toBe(true); + }, 30000); + + it('UNCHANGED: a forged `completed_date` outside a transition is still stripped', async () => { + // The hook's existence is not a blanket exemption for the column. No + // status change means no stamp, so the caller's value is the value on the + // key — and #2948 deletes it. + const { data } = await bootTodoKernel(); + const id = await newTask(data); + + await data.update( + 'todo_task', + { subject: 'Water the plants twice', completed_date: '1999-01-01T00:00:00.000Z' }, + { where: { id }, ...ctx }, + ); + + const row = await read(data, id); + expect(row.subject).toBe('Water the plants twice'); + expect(row.completed_date == null).toBe(true); + }, 30000); + + it('REVERSE: without the hook bound, the same user write is refused', async () => { + // The deleted-limb direction, and it is RED-on-restore rather than merely + // "the stamp is missing": the object's own `completed_date_required` rule + // turns an unstamped completion into a refused write. This is the + // pre-#7036 behaviour, rebuilt by withholding exactly one thing — the hook + // binding — from an otherwise identical kernel. + const kernel = new ObjectKernel({ logger: { level: 'silent' } } as any); + await kernel.use(new ObjectQLPlugin()); + await kernel.bootstrap(); + const objectql: any = kernel.getService('objectql'); + const data: any = kernel.getService('data'); + const driver: any = new SqliteWasmDriver({ filename: ':memory:' }); + await driver.connect(); + objectql.registerDriver(driver, true); + openDrivers.push(driver); + objectql.registry.registerObject(Task, 'todo', 'todo'); + await objectql.syncSchemas(); + // NB: no `bindHooks` — that is the whole fixture. + + const id = await newTask(data); + + await expect( + data.update('todo_task', { status: 'completed' }, { where: { id }, ...ctx }), + ).rejects.toThrow(/Completed date is required/); + + // ...and supplying the value by hand does not rescue it, which is what + // made the pair unsatisfiable rather than merely inconvenient. + await expect( + data.update( + 'todo_task', + { status: 'completed', completed_date: '2026-08-09T10:00:00.000Z' }, + { where: { id }, ...ctx }, + ), + ).rejects.toThrow(/Completed date is required/); + }, 30000); + + it('the hook is registered on the app bundle — an unregistered hook never runs', async () => { + // The behavioural tests above bind the hook themselves (this file builds + // its kernel by hand). This one pins the wiring the RUNTIME reads: + // `collectBundleHooks` walks `defineStack({ hooks })` and nothing else, so + // a hook missing from that array is dead metadata no matter how correct + // the hook file is. `task.hook.ts` shipped for exactly that long. + const hooks = (TodoApp as { hooks?: Array<{ name?: string; object?: string; events?: string[] }> }).hooks ?? []; + const registered = hooks.find((h) => h.name === 'task_logic'); + expect(registered, '`task_logic` must be in defineStack({ hooks })').toBeDefined(); + expect(registered!.object).toBe('todo_task'); + expect(registered!.events).toContain('beforeUpdate'); + }); +});