diff --git a/src/hooks/task.hook.ts b/src/hooks/task.hook.ts index caf3561..ed57174 100644 --- a/src/hooks/task.hook.ts +++ b/src/hooks/task.hook.ts @@ -17,7 +17,9 @@ import type { Hook, HookContext } from '@objectstack/spec/data'; * joined object (objectstack#14104). Both halves become knowable at a definite * instant, so each is resolved at that instant and stored: * - * `late_after` = `due_date + grace_days` · at DISPATCH, by the planner + * `late_after` = `due_date + grace_days` · at CREATION — by the planner + * for a dispatched task, here + * for every other one * `completed_late` = `completed_at > late_after` · at COMPLETION, here * * Neither is a MAINTAINED flag — the shape `AGENTS.md` rule 5 forbids, which @@ -213,15 +215,56 @@ import type { Hook, HookContext } from '@objectstack/spec/data'; * `mode: 'record'` has a payload of its own, so there is no batch to leak * onto. * + * ── A task created on a duty gets that duty's grace — not only a dispatched one ── + * The planner stamps `late_after` with the duty's grace itself, and through + * #52 every other producer fell back to `late_after = due_date`. That was right + * for the rows no duty governs — the assignment fan-out creates tasks with + * `duty` unset, and a member's own task has none — and wrong for a task created + * by hand ON a duty that grants grace: today, any `one_off` duty, which #61 + * keeps `grace_days` on deliberately and which is dispatched by hand. The + * overdue escalation reads the duty and waited out the grace; the Late lens + * read the row and listed the owner the morning after the due date (#100). + * + * So when a row arrives carrying a `duty`, a `due_date` and no stamp, this hook + * reads THAT duty's `grace_days` and stamps `due_date + grace` — the planner's + * own rule, from the one writer that already holds the pen. A row with no duty + * keeps zero grace exactly as before, and a row the planner already stamped is + * never read and never rewritten. The update leg's fill-a-blank applies the + * same rule, off the row's stored duty, for a task that acquires its due date + * after it was created. + * + * This used to be impossible here: through 17.x the lowered `body` had no + * engine to read `duly_duty` with, which is why #100 was filed with a flow and + * a producer-side stamp as the options. On 18.2 a hook body runs as + * `(async (ctx) => { … })` with `ctx.api`, and `objectstack build` infers the + * `api.read` capability from the `ctx.api.object(…).findOne` call below. + * + * Exactly one column is read — `grace_days`, nothing else of the duty — and it + * is read ELEVATED (`runAs: 'system'` on the hook). Without it the read runs + * as whoever pressed save, and a creator who cannot see the duty would get + * `null` back and a zero-grace deadline: the very defect, re-opened in the + * corner of the permission model nobody tests. The deadline a row is born with + * must not depend on who created it. Nothing leaks by it — the one number lands + * in a `readonly` column of the row being created, against a duty id the + * caller already named — and `runAs` governs `ctx.api` alone, not the readonly + * strip on this payload or anything else about the write. + * + * Only a positive whole grace counts. A blank, zero or unreadable value is zero + * grace, the same answer the planner gives a null `grace_days`. (A duty id + * that resolves to nothing never gets this far: the engine refuses a dangling + * lookup on the write itself — "no duly_duty record has id …".) + * * ── 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 * module-level constant or helper still BUILDS — it silently falls back to the * legacy bundled runtime module with a warning — so the pressure to keep this - * self-contained is a build warning, not an error. Keep the field list and the - * comparisons inside the function. + * self-contained is a build warning, not an error. Keep the field list, the + * comparisons and the date arithmetic inside the function. + * `test/task-hook-body.test.ts` lowers it with the build's own extractor and + * runs the result in the real QuickJS runner. */ -const stampTaskLifecycle = (ctx: HookContext): void => { +const stampTaskLifecycle = async (ctx: HookContext): Promise => { // `input.` IS the record field on a declarative hook — reads resolve // against the payload, writes land in it. The envelope spelling // `input.data.` is deliberately NOT used: it works only off the raw @@ -251,31 +294,57 @@ const stampTaskLifecycle = (ctx: HookContext): void => { return completedDay > deadline; }; + // `day` plus `days` civil days, in UTC — the same boundary as `dayOf`. Grace + // is granted in whole days, so this is calendar arithmetic, not 24h steps. + const plusDays = (day: string, days: number): string => { + if (days === 0) return day; + const at = new Date(`${day}T00:00:00.000Z`); + at.setUTCDate(at.getUTCDate() + days); + return at.toISOString().slice(0, 10); + }; + + // The grace the row's duty grants, read off that duty — see the header. + // `grace_days` and nothing else. A lookup may arrive as an id or as an + // expanded record; anything else, a missing duty, and a blank or + // non-positive grace are all zero grace. + const graceOf = async (duty: unknown): Promise => { + const id = typeof duty === 'string' + ? duty + : duty !== null && typeof duty === 'object' && typeof (duty as { id?: unknown }).id === 'string' + ? (duty as { id: string }).id + : ''; + if (id === '') return 0; + if (!ctx.api) { + // Both execution surfaces bind `ctx.api`. A context without it is not a + // write this hook can stamp correctly, and stamping zero grace instead + // would be the defect again with no trace — so refuse it, loudly. + throw new Error('duly_task_lifecycle_stamps: ctx.api is unavailable, so the duty\'s grace cannot be read.'); + } + const row = await ctx.api.object('duly_duty').findOne({ where: { id }, fields: ['grace_days'] }); + const grace = Number(row?.grace_days); + return Number.isInteger(grace) && grace > 0 ? grace : 0; + }; + if (ctx.event === 'beforeInsert') { // A brand-new task has just been touched, by definition. input.last_update_at = now; - // ── late_after — the zero-grace fallback, for the paths with no duty ── + // ── late_after — the due date plus the row's duty's grace ───────────── // The dispatcher stamps this itself, with the duty's own grace - // (`dispatch.plan.ts`). Every other producer has no duty to read: the - // assignment fan-out creates tasks with `duty` unset, and a member - // hand-creating their own task has none either. Zero grace is what "no - // duty governs this row" already means to the overdue escalation, so the - // two surfaces agree rather than one of them silently never firing. - // - // ⚠ The one row this is WRONG for is a task created by hand ON A DUTY - // that grants grace — today, any `one_off` duty (#61 keeps `grace_days` - // on that form deliberately). The escalation reads the duty and waits; - // this fallback does not and stamps the due date. Filed as #100 with the - // options, because the fix is a producer this handler cannot be: a - // lowered `body` ships without its module scope and has no engine to read - // `duly_duty` with. It is not a regression — before the stamps existed the - // `late` view was grace-free for every task alike. + // (`dispatch.plan.ts`), and that stamp is kept as it arrived — no read. + // Every other producer gets the same rule here: a task created ON a duty + // — by hand, through the form, a one-off above all (#100) — is late + // after `due_date + duty.grace_days`, exactly what the overdue escalation + // waits out. A row with no duty — the assignment fan-out sets none, and a + // member's own task has none — is zero grace, which is what "no duty + // governs this row" already means to the escalation. // // A task with no `due_date` keeps a blank stamp: there is no deadline to // derive, and a task with no due date genuinely cannot be late. const dueDay = dayOf(input.due_date); - if (dueDay !== '' && dayOf(input.late_after) === '') input.late_after = dueDay; + if (dueDay !== '' && dayOf(input.late_after) === '') { + input.late_after = plusDays(dueDay, await graceOf(input.duty)); + } // A row inserted ALREADY done — a seeded history, an import — carries its // verdict from the same beat, because both halves are on the payload. The @@ -335,9 +404,16 @@ const stampTaskLifecycle = (ctx: HookContext): void => { // with. Nothing is written instead: an unstamped row stays unstamped, which // is where it already was, rather than a stamped row being corrupted. An // administrative bulk re-date is left alone, as it is for `last_update_at`. + // + // The fill applies the insert leg's rule — the row's duty's grace — so a + // task dated after it was created is judged the way one dated at creation + // is. The duty is read off the payload if the write moves it, else off the + // stored row. if (ctx.dispatch?.mode !== 'per-row' && dayOf(previous.late_after) === '') { const dueDay = dayOf('due_date' in input ? input.due_date : previous.due_date); - if (dueDay !== '') input.late_after = dueDay; + if (dueDay !== '') { + input.late_after = plusDays(dueDay, await graceOf('duty' in input ? input.duty : previous.duty)); + } } const lateAfter = 'late_after' in input ? input.late_after : previous.late_after; @@ -478,7 +554,9 @@ export const TaskLifecycleHook: Hook = { description: 'Server-owned columns on duly_task: completed_at and the completed_late verdict on the ' + 'transition into and out of done, late_after filled at insert for the paths the ' - + 'dispatcher does not stamp, and last_update_at only when status, progress, note or ' + + 'dispatcher does not stamp (due date plus the grace of the duty the row carries, read ' + + 'elevated — grace_days only — or zero grace when it carries none), and last_update_at ' + + 'only when status, progress, note or ' + 'skip_reason ' + 'actually changed — never on an administrative write, which would reset the stagnation ' + 'signal. Both lateness stamps are write-once: a later change to the duty\'s grace never ' @@ -491,5 +569,10 @@ export const TaskLifecycleHook: Hook = { // stamps were not applied is the exact silent corruption the // `completed_at_required_when_done` rule exists to make impossible. onError: 'abort', + // Elevated for the one `ctx.api` call this hook makes: reading `grace_days` + // off the duty the row names. The deadline a row is born with must not + // depend on whether its creator can see that duty — see the module header. + // Governs `ctx.api` only; the readonly strip on this payload is unchanged. + runAs: 'system', handler: stampTaskLifecycle, }; diff --git a/src/objects/task.object.ts b/src/objects/task.object.ts index 209a98c..bc4a390 100644 --- a/src/objects/task.object.ts +++ b/src/objects/task.object.ts @@ -215,10 +215,13 @@ export const Task = ObjectSchema.create({ * Blank only when the task has no `due_date` at all — nothing to be late * against, so it never appears in a lateness surface. Every task that HAS a * due date gets one: the planner stamps it with the duty's grace, and - * `task.hook.ts` stamps `late_after = due_date` (zero grace) for the paths - * that have no duty to read — the assignment fan-out and a hand-created - * task. That is the same reading of "no duty governs this row" the overdue - * escalation already uses (`src/flows/reminders.flow.ts`). + * `task.hook.ts` stamps every other row at creation — `due_date` plus the + * grace of the duty the row carries, so a task created by hand on a + * one-off duty is judged exactly as the overdue escalation judges it + * (#100), and `due_date` itself (zero grace) for a row with no duty: the + * assignment fan-out and a member's own task. That is the same reading of + * "no duty governs this row" the escalation already uses + * (`src/flows/reminders.flow.ts`). */ late_after: Field.date({ label: 'Late after', diff --git a/src/translations/authored-text.ts b/src/translations/authored-text.ts index 3a180d9..5bd2aec 100644 --- a/src/translations/authored-text.ts +++ b/src/translations/authored-text.ts @@ -678,6 +678,7 @@ const VERDICTS: Readonly> = { 'hook.handler': machine('handler registration key'), 'hook.events[]': machine('lifecycle events'), 'hook.onError': machine('failure posture'), + 'hook.runAs': machine('execution identity'), 'hook.label': untranslatable(ADMIN_WHY('hook')), 'hook.description': untranslatable(ADMIN_WHY('hook')), diff --git a/test/task-hook-body.test.ts b/test/task-hook-body.test.ts new file mode 100644 index 0000000..d01a102 --- /dev/null +++ b/test/task-hook-body.test.ts @@ -0,0 +1,145 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +import { afterAll, beforeAll, describe, expect, it } from 'vitest'; +import { AppPlugin, ObjectKernel, createStandaloneStack } from '@objectstack/runtime'; +import { createIdentityObjectsPlugin } from '@objectstack/plugin-auth'; +import { extractHookBody } from '@objectstack/cli/hook-body'; + +import stack from '../objectstack.config.js'; +import { TaskLifecycleHook } from '../src/hooks/task.hook.js'; +import { ensureUsers } from './support/users.js'; + +/** + * `duly_task_lifecycle_stamps` as it SHIPS — lowered, and run in QuickJS. + * + * `test/task-hook.test.ts` runs the inline handler in Node. What a deployment + * runs is different: `objectstack build` lowers that handler into a metadata + * `body`, and the runtime executes the body in the QuickJS sandbox, where the + * module scope is gone, `Intl` does not exist and `ctx.api` is only usable for + * the capabilities the body declares. All four gates can stay green while the + * lowered body is broken, because none of them runs it. + * + * #100 is what makes that gap matter here: the hook now READS `duly_duty` + * through `ctx.api` to apply the duty's grace. So this suite lowers the handler + * with the build's own extractor (`@objectstack/cli/hook-body`, the function + * `os build` and `os lint` both call), swaps the result into the real config in + * place of the function, and boots that — `AppPlugin` then binds it through + * the QuickJS body runner, exactly as it binds `dist/objectstack.json`. + */ + +const lowered = extractHookBody(TaskLifecycleHook.handler as (...a: unknown[]) => unknown, 'task.hook.ts'); + +let data: any; +let kernel: any; + +const SYSTEM = { isSystem: true } as const; + +beforeAll(async () => { + const bodyOnly = { + ...stack, + hooks: (stack.hooks ?? []).map((hook: any) => + hook.name === TaskLifecycleHook.name + ? { + ...hook, + // No function left to fall back to: if the body does not run, the + // stamps below are simply missing. + handler: undefined, + body: { language: 'js', source: lowered.source, capabilities: lowered.capabilities }, + } + : hook), + }; + + const { plugins } = await createStandaloneStack({ + databaseDriver: 'sqlite', + databaseUrl: ':memory:', + skipSeedData: true, + // See test/task-hook.test.ts: never let a local build artifact stand in + // for the config under test. + artifactPath: 'dist/objectstack.this-suite-must-not-load-an-artifact.json', + }); + kernel = new ObjectKernel(); + for (const plugin of plugins) await kernel.use(plugin); + await kernel.use(createIdentityObjectsPlugin()); + await kernel.use(new AppPlugin(bodyOnly as typeof stack, undefined, { skipSeedData: true })); + await kernel.bootstrap(); + data = kernel.getService('data'); + await ensureUsers(data, ['user_alice']); +}, 120_000); + +afterAll(async () => { + await kernel?.shutdown?.(); +}); + +const DUE = '2026-11-10'; + +const oneOffDuty = async (name: string, grace: number) => + data.insert('duly_duty', { + name, + owner: 'user_alice', + form: 'one_off', + status: 'active', + source: 'catalog', + grace_days: grace, + timezone: 'UTC', + }, { context: SYSTEM }); + +const newTask = async (over: Record = {}) => + data.insert('duly_task', { + subject: 'Hand over the commissioning file', + owner: 'user_alice', + source: 'catalog', + status: 'open', + ...over, + }); + +describe('the lowered body', () => { + it('lowers without a module-scope reference, and declares exactly the read it makes', () => { + // `extractHookBody` throws on a free identifier, so reaching this line is + // half the assertion. The other half: the grace read is the body's only + // `ctx.api` use, so `api.read` is its only capability — a write + // capability appearing here would mean the hook had grown a second effect. + expect(lowered.isExpression).toBe(false); + expect(lowered.capabilities).toEqual(['api.read']); + expect(lowered.source).toContain("ctx.api.object(\"duly_duty\").findOne"); + }); + + it('makes exactly one elevated call: grace_days off the duty the row names', () => { + // `runAs: 'system'` elevates the body's own `ctx.api` calls and nothing + // else — the triggering task write keeps its caller's authorization (the + // spec's `runAs` scope note; measured live on `pnpm demo` for #100: a + // member who gets 404 reading the duty still gets the grace on their + // task, and still gets 404 updating a task that is not theirs). So the + // whole of what the elevation reaches is this one read, and it is pinned. + const calls = lowered.source.match(/ctx\.api\b[^;]*/g) ?? []; + const apiCalls = calls.filter((c) => c.includes('.object(')); + expect(apiCalls, 'one ctx.api data call in the whole body').toHaveLength(1); + expect(apiCalls[0].replace(/\s+/g, ' ')) + .toBe('ctx.api.object("duly_duty").findOne({ where: { id }, fields: ["grace_days"] })'); + }); + + it('stamps a task on a one-off duty with the duty\'s grace (#100)', async () => { + const duty = await oneOffDuty('One-off granting a week — lowered body', 7); + const task = await newTask({ duty: duty.id, due_date: DUE }); + expect(task.late_after).toBe('2026-11-17'); + }); + + it('keeps zero grace for a task with no duty', async () => { + const task = await newTask({ subject: 'my own task, lowered', due_date: DUE }); + expect(task.late_after).toBe(DUE); + }); + + it('fills a blank deadline with the duty\'s grace on the update leg', async () => { + const duty = await oneOffDuty('One-off dated later — lowered body', 7); + const task = await newTask({ duty: duty.id }); + const dated = await data.update('duly_task', { id: task.id, due_date: DUE }); + expect(dated.late_after).toBe('2026-11-17'); + }); + + it('still stamps the completion — the rest of the hook survived the lowering', async () => { + const task = await newTask({ subject: 'complete me, lowered', due_date: '2099-01-01' }); + const done = await data.update('duly_task', { id: task.id, status: 'done' }); + expect(done.completed_at, 'completed_at_required_when_done would refuse the write otherwise') + .toBeTruthy(); + expect(done.completed_late).toBe(false); + }); +}); diff --git a/test/task-hook.test.ts b/test/task-hook.test.ts index efabe7c..f97f0ac 100644 --- a/test/task-hook.test.ts +++ b/test/task-hook.test.ts @@ -858,6 +858,119 @@ describe('late_after — the deadline the row is born with', () => { }); }); +/** + * #100 — a task created ON a duty carries that duty's grace, whoever creates it. + * + * Through #52 only the planner applied `grace_days`; every other producer got + * `late_after = due_date`. Measured on 18.2.0 before this change: a task on a + * `one_off` duty granting 7 days, created as a member, as the system and + * through `protocol.createData`, came back with `late_after` equal to its due + * date on all three — so the owner was on the Late lens the morning after the + * due date while the overdue escalation (correctly) waited out the week. + */ +describe('late_after on a task created on a duty that grants grace (#100)', () => { + const DUE = '2026-11-10'; + const DUE_PLUS_7 = '2026-11-17'; + + const oneOffDuty = async (name: string, grace: number | null) => + data.insert('duly_duty', { + name, + owner: 'user_alice', + form: 'one_off', + status: 'active', + source: 'catalog', + grace_days: grace, + timezone: 'UTC', + }, { context: SYSTEM }); + + it('a member\'s create on a one-off duty is late after the due date PLUS the grace', async () => { + const duty = await oneOffDuty('One-off granting a week — member create', 7); + expect(duty.grace_days).toBe(7); + const task = await newTask({ duty: duty.id, due_date: DUE }); + expect(task.late_after, 'due_date + duty.grace_days, as the escalation reads it').toBe(DUE_PLUS_7); + }); + + it('a system create that carries no stamp gets the same deadline', async () => { + const duty = await oneOffDuty('One-off granting a week — system create', 7); + const task = await dispatchedTask({ duty: duty.id, due_date: DUE }); + expect(task.late_after).toBe(DUE_PLUS_7); + }); + + it('so does a create through the protocol — the REST, OpenAPI and MCP write face', async () => { + const duty = await oneOffDuty('One-off granting a week — protocol create', 7); + const protocol = kernel.getService('protocol'); + const created = await protocol.createData({ + object: 'duly_task', + data: { + subject: 'Hand over the commissioning file', + owner: 'user_alice', + source: 'catalog', + status: 'open', + duty: duty.id, + due_date: DUE, + }, + }); + expect(created.record.late_after).toBe(DUE_PLUS_7); + }); + + it('agrees with the planner across a month end', async () => { + // Civil-day arithmetic, not 24h steps: the planner's `addCalendarDays` + // and this hook must land on the same day for the same inputs. + const duty = await oneOffDuty('One-off granting a week — month end', 7); + const task = await newTask({ duty: duty.id, due_date: '2027-02-25' }); + expect(task.late_after).toBe('2027-03-04'); + }); + + it('a duty granting no grace is zero grace — blank and 0 alike', async () => { + const zero = await oneOffDuty('One-off granting nothing', 0); + expect((await newTask({ duty: zero.id, due_date: DUE })).late_after).toBe(DUE); + + const blank = await oneOffDuty('One-off with a blank grace', null); + expect((await newTask({ duty: blank.id, due_date: DUE })).late_after).toBe(DUE); + }); + + it('a row with no duty keeps zero grace — the assignment fan-out and a member\'s own task', async () => { + // The card's boundary, restated where it is enforced: no duty governs + // these rows, so there is no grace to apply. + const own = await newTask({ subject: 'my own task', due_date: DUE }); + expect(own.late_after).toBe(DUE); + const fannedOut = await dispatchedTask({ subject: 'from an assignment', duty: null, due_date: DUE }); + expect(fannedOut.late_after).toBe(DUE); + }); + + it('the planner\'s stamp is kept as it arrived — the duty is not re-read', async () => { + // Grace changed between planning and insert: the planner's value wins, + // because write-once means the stamp a producer supplies is the stamp. + const duty = await oneOffDuty('One-off whose grace the planner already applied', 7); + const task = await dispatchedTask({ duty: duty.id, due_date: DUE, late_after: '2026-11-12' }); + expect(task.late_after).toBe('2026-11-12'); + }); + + it('a task dated after it was created is judged with its duty\'s grace too', async () => { + const duty = await oneOffDuty('One-off dated later', 7); + const task = await newTask({ duty: duty.id }); + expect(task.late_after ?? null).toBeNull(); + const dated = await data.update('duly_task', { id: task.id, due_date: DUE }); + expect(dated.late_after).toBe(DUE_PLUS_7); + }); + + it('is still write-once: widening the duty\'s grace later does not move the stamp', async () => { + const duty = await oneOffDuty('One-off widened after creation', 7); + const task = await newTask({ duty: duty.id, due_date: DUE }); + await data.update('duly_duty', { id: duty.id, grace_days: 21 }, { context: SYSTEM }); + await data.update('duly_task', { id: task.id, note: 'touched after the widening' }); + expect((await read(task.id)).late_after).toBe(DUE_PLUS_7); + }); + + it('reads the duty elevated — runAs system is declared on the hook', () => { + // A creator who cannot see the duty must not get a zero-grace deadline. + // This harness runs no security middleware, so the elevation itself is + // verified against a live `pnpm demo`; here the declaration is pinned. + const hook = dulyHooks.find((h) => h.name === 'duly_task_lifecycle_stamps'); + expect(hook?.runAs).toBe('system'); + }); +}); + describe('WRITE-ONCE — editing a duty\'s grace never rewrites history', () => { /** * The assertion this whole card turns on.