fix(example-todo): make task completion possible for a normal user — stamp completed_date in the hook instead of demanding it from the caller (#7036)

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

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

examples/app-todo/src/actions/task.handlers.ts

@@ -31,12 +31,20 @@ interface ActionContext {
   params?: Record<string, unknown>;
 }
 
-/** 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<void> {
   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<void> {
   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,
     });
   }
 }

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<string, unknown> }).data;
+    const previous = ctx.previous as Record<string, unknown> | 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`);
       }
     }
   }

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