Skip to content

Commit 3d89777

Browse files
os-helpclaude
andauthored
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) (#7222)
* fix(example-todo): make task completion possible for a normal user (#7036) `completed_date` is `readonly` (a non-system caller's write is stripped on the update path) and `completed_date_required` then refused the write for missing exactly the value that had just been dropped, so `completeTask` — and every user-driven status transition into `completed` — always failed. Fixed in the metadata, the platform's system-stamp shape: `task.hook.ts` gains a `beforeUpdate` leg that stamps `completed_date` on the transition into `completed` and clears it on the transition out. A hook's write survives the readonly strip by design (#2948/#5591), so the rule is satisfied by the server and callers send `status` alone. Also registers the hook in `defineStack({ hooks })` — it had never been in that array, so the whole file was dead metadata — and repairs the two existing legs, which read the record off `ctx.input` instead of `ctx.input.data`. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015fkdTyGmMD5s8ZtEifvuGy * fix(example-todo): log the completion hook through the kernel logger, not `console` The root type program has `lib: ["ES2020"]` and no node types, so `console` is undefined there. The two calls were pre-existing but had never been measured: `task.hook.ts` entered the root program for the first time in this branch, because until now nothing imported it. Routing them through the engine handle the hook context already carries is also the better shape for a reference app — it honours the kernel's configured log level. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015fkdTyGmMD5s8ZtEifvuGy * chore(changeset): declare the app-todo completion-stamp fix as a patch (#7036) `@objectstack/example-todo` is a changeset-versioned package (it carries a version and a CHANGELOG, and `.changeset/config.json` does not ignore it), and the sister repair #6882 shipped with one. This change is user-visible — `completeTask` went from always failing to working — so it declares a patch rather than taking the `skip-changeset` route. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015fkdTyGmMD5s8ZtEifvuGy --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent fb5ebc5 commit 3d89777

6 files changed

Lines changed: 364 additions & 24 deletions

File tree

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,62 @@
1+
---
2+
"@objectstack/example-todo": patch
3+
---
4+
5+
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)
6+
7+
`examples/app-todo` shipped two declarations on `todo_task` that could not both hold on
8+
the update path, so the app's headline action was unsatisfiable by construction.
9+
10+
**Before.** `completed_date` is `Field.datetime({ readonly: true })`, and `readonly` is a
11+
two-part contract: never editable in forms, **and** a non-system caller's write to it is
12+
stripped from the payload on the update path. The same object then declares a validation
13+
rule, `completed_date_required`, refusing any record whose `status` is `completed` while
14+
`completed_date` is blank. The strip runs first, so a payload carrying both keys lost
15+
`completed_date` and was then rejected for missing it. Measured against the real object on
16+
a real kernel:
17+
18+
```
19+
update status+completed_date (user ctx): REJECTED -> Completed date is required when status is Completed
20+
update status only (user ctx): REJECTED -> Completed date is required when status is Completed
21+
update status+completed_date (isSystem): OK
22+
insert already-completed: OK
23+
```
24+
25+
Both escapes are non-user paths — an elevated write bypasses the strip, and a create may
26+
legitimately seed a read-only column. Every ordinary user update was refused, which made
27+
the app's own `completeTask` and `massCompleteTasks` handlers fail every time they ran.
28+
29+
**After.** The column is server-owned, so the server writes it. `task.hook.ts` gains a
30+
`beforeUpdate` leg that stamps `completed_date` on the transition into `completed` and
31+
clears it on the transition back out; `completeTask` and `massCompleteTasks` now send
32+
`status` alone. A one-key user-context update completes the task and persists the stamp.
33+
34+
This works because the readonly strip is deliberately narrow rather than because it is
35+
bypassed: it runs *after* the before-hooks and deletes a key only when the caller supplied
36+
it **and** it still holds the caller's own value (`stripReadonlyFields`, the
37+
`suppliedValues` snapshot plus the `Object.is` identity check). A value a hook wrote is a
38+
platform value and survives — including when the caller echoed the same key back, which is
39+
what a whole-record form PUT does. The stamp is therefore written unconditionally: leaving
40+
a caller-supplied value in place would leave the caller's own value on the key, and the
41+
strip would delete it.
42+
43+
`completed_date_required` stays, and is now the assertion that the stamp actually
44+
happened — if the hook is ever unregistered or its transition guard breaks, the write is
45+
refused loudly instead of committing a completed task with no completion date.
46+
47+
**Two related repairs the fix required.**
48+
49+
- The hook was never registered. `task_logic` was not in `defineStack({ hooks })`, and
50+
`collectBundleHooks` reads that array and nothing else, so the whole file was dead
51+
metadata: it type-checked, it read as wired, and it never ran. Both sibling example apps
52+
already declare `hooks: allHooks`; `app-todo` now does too.
53+
- Both existing legs read the record off `ctx.input` rather than `ctx.input.data`.
54+
`HookContext.input` is an envelope — `{ data, options }` on insert, `{ id, data, options }`
55+
on update — so `ctx.input.priority = 'normal'` set a key no write path reads. The insert
56+
defaults and the after-update branches had never had any effect; they do now. The
57+
after-update logging also moved from `console` to the kernel logger reached through
58+
`ctx.ql`, so it honours the configured log level.
59+
60+
Reopening a completed task clears `completed_date`, documented in the object and hook
61+
metadata: the field means "when this task was completed", so a task that is not completed
62+
must not carry a stale one. An edit that carries no `status` is not treated as a reopen.

‎examples/app-todo/objectstack.config.ts‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,11 @@ import { defineStack } from '@objectstack/spec';
44

55
// ─── Barrel Imports (one per metadata type) ─────────────────────────
66
import * as objects from './src/objects/index.js';
7+
// [#7036] Lifecycle hooks are NOT collected from the objects barrel — the
8+
// runtime reads them from `defineStack({ hooks })` only (`collectBundleHooks`).
9+
// An unregistered `*.hook.ts` file is dead metadata: it type-checks, it reads
10+
// as wired, and it never runs.
11+
import taskHook from './src/objects/task.hook.js';
712
import * as actions from './src/actions/index.js';
813
import * as dashboards from './src/dashboards/index.js';
914
import * as datasets from './src/datasets/index.js';
@@ -46,6 +51,9 @@ export default defineStack({
4651
// Seed Data (top-level, registered as metadata)
4752
data: TodoSeedData,
4853

54+
// Object Lifecycle Hooks (same shape as app-crm / app-showcase)
55+
hooks: [taskHook],
56+
4957
// Auto-collected from barrel index files via Object.values()
5058
objects: Object.values(objects),
5159
views: Object.values(views),

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

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -31,12 +31,20 @@ interface ActionContext {
3131
params?: Record<string, unknown>;
3232
}
3333

34-
/** Mark a single task as complete */
34+
/**
35+
* Mark a single task as complete.
36+
*
37+
* [#7036] `status` only. `completed_date` is `readonly` — server-owned — so a
38+
* caller's write to it is stripped from the payload before the record is
39+
* validated, and sending it here made this action refuse itself against
40+
* `todo_task`'s `completed_date_required` rule. The stamp belongs to the
41+
* `beforeUpdate` leg of `src/objects/task.hook.ts`, which runs on the
42+
* transition and whose write the strip lets through.
43+
*/
3544
export async function completeTask(ctx: ActionContext): Promise<void> {
3645
const { record, engine } = ctx;
3746
await engine.update('todo_task', record.id as string, {
3847
status: 'completed',
39-
completed_date: new Date().toISOString(),
4048
});
4149
}
4250

@@ -59,15 +67,13 @@ export async function cloneTask(ctx: ActionContext): Promise<{ id: string }> {
5967
});
6068
}
6169

62-
/** Mark all selected tasks as complete (bulk) */
70+
/** Mark all selected tasks as complete (bulk) — same `status`-only rule as {@link completeTask} (#7036) */
6371
export async function massCompleteTasks(ctx: ActionContext): Promise<void> {
6472
const { params, engine } = ctx;
6573
const ids = (params?.selectedIds ?? []) as string[];
66-
const now = new Date().toISOString();
6774
for (const id of ids) {
6875
await engine.update('todo_task', id, {
6976
status: 'completed',
70-
completed_date: now,
7177
});
7278
}
7379
}

‎examples/app-todo/src/objects/task.hook.ts‎

Lines changed: 88 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -2,37 +2,112 @@
22

33
import { HookContext, Hook } from '@objectstack/spec/data';
44

5+
/**
6+
* Lifecycle logic for `todo_task` — insert defaults and the completion stamp.
7+
*
8+
* ## `ctx.input` is an ENVELOPE, not the record (#7036)
9+
*
10+
* The engine builds one context shape per event and the record is always a
11+
* slot inside it, never the context's own keys:
12+
*
13+
* - `beforeInsert` → `{ data, options }` (one context per row)
14+
* - `beforeUpdate` → `{ id, data, options }`
15+
* - `afterUpdate` → `{ id, data, options }`
16+
*
17+
* (the contract table on `HookContextSchema.input`, pinned against the real
18+
* engine in `packages/objectql/src/hook-input-shape-contract.test.ts`). So the
19+
* record lives at `ctx.input.data`; writing `ctx.input.priority` sets a key on
20+
* the envelope that no write path ever reads.
21+
*
22+
* ## Why the completion stamp is written HERE, and unconditionally
23+
*
24+
* `completed_date` is `readonly: true` — a server-owned column. On the update
25+
* path the engine strips a non-system caller's write to such a column, and
26+
* `todo_task`'s own `completed_date_required` rule then refused the write for
27+
* missing exactly the value it had just dropped. That made "Complete task"
28+
* impossible for an ordinary user (#7036).
29+
*
30+
* A hook stamp is the platform's answer, and it works because the strip is
31+
* deliberately narrow: it runs AFTER the before-hooks and only deletes a key
32+
* that both (a) the caller supplied and (b) still holds *the caller's own
33+
* value* (#2948 + #5591 — `stripReadonlyFields`). A value a hook wrote is a
34+
* platform value and survives.
35+
*
36+
* That is also why the stamp below is UNCONDITIONAL rather than
37+
* `data.completed_date ??= …`: if a caller supplied the key and the hook left
38+
* it alone, the value would still be the caller's, the strip would delete it,
39+
* and the rule would refuse the write again — the original bug, restored. The
40+
* server owning the column means the server writes it on every transition.
41+
*
42+
* ## Leaving `completed` clears the stamp
43+
*
44+
* Reopening a completed task (completed → in_progress, …) nulls
45+
* `completed_date`. The field means "when this task was completed", so a task
46+
* that is not completed must not carry one; retaining it would leave a stale
47+
* timestamp that every report and list view reads as fact. The same
48+
* `readonly`/strip reasoning applies — only a hook can write the clear.
49+
*/
550
const taskHook: Hook = {
651
name: 'task_logic',
752
object: 'todo_task',
8-
events: ['beforeInsert', 'afterUpdate'],
53+
events: ['beforeInsert', 'beforeUpdate', 'afterUpdate'],
954
handler: async (ctx: HookContext) => {
55+
const data = (ctx.input as { data?: Record<string, unknown> }).data;
56+
const previous = ctx.previous as Record<string, unknown> | undefined;
57+
if (!data) return;
58+
1059
if (ctx.event === 'beforeInsert') {
11-
const { input } = ctx;
1260
// Default priority
13-
if (!input.priority) {
14-
input.priority = 'normal';
61+
if (!data.priority) {
62+
data.priority = 'normal';
1563
}
1664
// Default status
17-
if (!input.status) {
18-
input.status = 'not_started';
65+
if (!data.status) {
66+
data.status = 'not_started';
1967
}
2068
// Validation
21-
if (typeof input.subject === 'string' && input.subject.includes('spam')) {
69+
if (typeof data.subject === 'string' && data.subject.includes('spam')) {
2270
throw new Error('Spam tasks are not allowed');
2371
}
2472
}
25-
73+
74+
if (ctx.event === 'beforeUpdate') {
75+
// The transition INTO completed — `previous` is the engine's pre-update
76+
// snapshot, bound before this hook runs, so "is this a transition?" is
77+
// answerable here without a read of our own.
78+
if (data.status === 'completed' && previous?.status !== 'completed') {
79+
data.completed_date = new Date().toISOString();
80+
}
81+
// ...and the transition back OUT of it. Guarded on `status` actually
82+
// being part of this write: an unrelated edit of a completed task
83+
// (`{ progress_percent: 100 }`) carries no `status` and must not be read
84+
// as a reopen.
85+
else if (
86+
data.status !== undefined &&
87+
data.status !== 'completed' &&
88+
previous?.status === 'completed'
89+
) {
90+
data.completed_date = null;
91+
}
92+
}
93+
2694
if (ctx.event === 'afterUpdate') {
95+
// The kernel's logger, reached through the engine handle the context
96+
// carries. Not `console`: a hook runs inside the server, so its output
97+
// belongs on the platform logger, which honours the kernel's configured
98+
// level (a test booting `{ logger: { level: 'silent' } }` stays silent).
99+
// `ctx.ql` is declared `unknown` on `HookContextSchema`, hence the cast.
100+
const logger = (ctx.ql as { logger?: { info?: (message: string) => void } } | undefined)?.logger;
101+
27102
// Check if completed
28-
if (ctx.input.status === 'completed' && ctx.previous && ctx.previous.status !== 'completed') {
29-
console.log(`Task ${ctx.id} completed by ${ctx.session?.userId || 'unknown'}`);
103+
if (data.status === 'completed' && previous && previous.status !== 'completed') {
104+
logger?.info?.(`Task ${ctx.input.id} completed by ${ctx.session?.userId || 'unknown'}`);
30105
// Could trigger notifications or integrations here
31106
}
32-
107+
33108
// Check if task became overdue
34-
if (ctx.input.is_overdue && ctx.previous && !ctx.previous.is_overdue) {
35-
console.log(`Task ${ctx.id} is now overdue`);
109+
if (data.is_overdue && previous && !previous.is_overdue) {
110+
logger?.info?.(`Task ${ctx.input.id} is now overdue`);
36111
}
37112
}
38113
}

‎examples/app-todo/src/objects/task.object.ts‎

Lines changed: 21 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -72,6 +72,12 @@ export const Task = ObjectSchema.create({
7272
label: 'Reminder Date/Time',
7373
}),
7474

75+
// [#7036] Server-owned: `readonly` means "never editable in forms, AND a
76+
// non-system caller's write is stripped on the write path". Nothing may
77+
// hand this value in — `task.hook.ts` stamps it on the transition into
78+
// `completed` and clears it on the transition back out, which is the one
79+
// write the readonly strip is designed to let through (#2948/#5591).
80+
// Callers (including `actions/task.handlers.ts`) send `status` alone.
7581
completed_date: Field.datetime({
7682
label: 'Completed Date',
7783
readonly: true,
@@ -186,6 +192,14 @@ export const Task = ObjectSchema.create({
186192
highlightFields: ['subject', 'status', 'priority', 'due_date', 'owner'],
187193

188194
validations: [
195+
// [#7036] This rule is satisfied by the SERVER, not by the caller. The
196+
// `beforeUpdate` leg of `task.hook.ts` stamps `completed_date` before
197+
// validation runs, so a completion write that carries only
198+
// `status: 'completed'` passes. It stays as a rule rather than being
199+
// deleted because it is the assertion that the stamp actually happened:
200+
// if the hook is ever unregistered or its transition guard breaks, the
201+
// write is refused loudly instead of committing a completed task with no
202+
// completion date.
189203
{
190204
name: 'completed_date_required',
191205
type: 'script',
@@ -206,8 +220,13 @@ export const Task = ObjectSchema.create({
206220
// field — it was silently stripped at build and never ran (ADR-0032 "no
207221
// silent failure"). Record-triggered automation for this object lives in the
208222
// supported mechanisms instead:
209-
// • `task.hook.ts` — lifecycle hook (defaults, completion logic)
210-
// • `actions/task.handlers.ts` — stamps `completed_date` on completion
223+
// • `task.hook.ts` — lifecycle hook (insert defaults; stamps and
224+
// clears `completed_date` on the completion
225+
// transition). Registered via
226+
// `defineStack({ hooks })` in
227+
// `objectstack.config.ts` — a hook that is not
228+
// in that array never runs (#7036).
229+
// • `actions/task.handlers.ts` — flips `status`; the stamp is the hook's
211230
// • `flows/task.flow.ts` — record_change + schedule flows (completion /
212231
// recurrence, reminders, overdue escalation)
213232
});

0 commit comments

Comments
 (0)