diff --git a/src/functions/index.ts b/src/functions/index.ts index dd37a62..0bcbde2 100644 --- a/src/functions/index.ts +++ b/src/functions/index.ts @@ -11,6 +11,10 @@ // DECLARES it (`{ handler, effect: 'writes' }`) so a run reports // `unmeasuredEffect` rather than claiming it wrote nothing. +import { + DUTY_TIMEZONE_GUARD_HANDLER, + dulyValidateDutyTimezone, +} from '../hooks/duty.hook.js'; import { DISPATCH_HANDLER_NAME, dulyDispatch } from '../jobs/dispatch.job.js'; export const dulyFunctions = { @@ -20,4 +24,13 @@ export const dulyFunctions = { // has no flow graph around it to do the writing — see the note on data reach // in `src/jobs/dispatch.job.ts`. [DISPATCH_HANDLER_NAME]: { handler: dulyDispatch, effect: 'writes' as const }, + + // `duly_duty_timezone_guard`'s handler. A hook resolves a STRING handler + // against this map (`resolveHandler` -> `opts.functions[name]`), which is the + // whole reason the guard can use `Intl`: the alternative spelling — an inline + // handler — is lowered by `objectstack build` into a QuickJS `body`, where + // `Intl` does not exist. Registered in the bare form, which IS the + // declaration `effect: 'pure'`: it inspects the payload and either returns or + // throws, and writes nothing. + [DUTY_TIMEZONE_GUARD_HANDLER]: dulyValidateDutyTimezone, }; diff --git a/src/functions/period.ts b/src/functions/period.ts index 23f613b..0f2bab0 100644 --- a/src/functions/period.ts +++ b/src/functions/period.ts @@ -210,6 +210,45 @@ function formatterFor(timezone: string): Intl.DateTimeFormat { return formatter; } +/** + * Does this host resolve `timezone` as a real IANA zone? + * + * The ONE membership oracle for a duty's timezone, shared deliberately with + * {@link formatterFor} above rather than reimplemented next to the write path. + * A guard that admitted a different set than the engine would be wrong in one + * of two directions: refuse a zone that dispatches perfectly well, or pass one + * that still throws on dispatch night — which is the whole defect it exists to + * close. Sharing the constructor also shares its OPTIONS (`hourCycle: 'h23'`, + * `era: 'short'`), so an ICU build that rejected one of those would fail the + * guard too, instead of admitting a zone this module cannot actually format. + * + * ── Why the `Intl.DateTimeFormat` probe and NOT `Intl.supportedValuesOf` ── + * `supportedValuesOf('timeZone')` is the tempting spelling and it is the wrong + * one. Measured on this repo's Node 22 baseline it returns 418 CLDR + * *canonical* names and omits `UTC` — `duly_duty.timezone`'s own declared + * `defaultValue` — along with `GMT`, `Asia/Kolkata`, `Europe/Kyiv`, + * `Asia/Ho_Chi_Minh` and `US/Eastern`. Every one of those resolves here and + * gets correct boundaries out of this module, so a guard built on that list + * would refuse every duty created with the field default. This is also the + * definition the platform publishes for its own `iana_time_zone` value domain + * (`@objectstack/spec`, `system/settings-manifest.zod.ts`): *membership is the + * `Intl.DateTimeFormat` probe*, explicitly not the enumerated list. + * + * Case and aliases are the host's business, not this module's: `GMT`, + * `US/Eastern` and `america/new_york` all resolve, and boundaries are computed + * in whatever zone ICU maps them to. Nothing here rewrites the caller's + * spelling — canonicalising a stored value would be changing data, not + * validating it. + */ +export function isResolvableTimeZone(timezone: string): boolean { + try { + formatterFor(timezone); + return true; + } catch { + return false; + } +} + /** The wall-clock reading a zone shows at an instant. */ function wallTimeAt(instant: Date, timezone: string): WallTime { const parts = formatterFor(timezone).formatToParts(instant); diff --git a/src/hooks/duty.hook.ts b/src/hooks/duty.hook.ts new file mode 100644 index 0000000..527bd9f --- /dev/null +++ b/src/hooks/duty.hook.ts @@ -0,0 +1,147 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +import type { Hook, HookContext } from '@objectstack/spec/data'; + +import { isResolvableTimeZone } from '../functions/period.js'; + +/** + * `duly_duty.timezone` must be a zone the host can actually resolve. + * + * Period boundaries and due dates are computed in the duty's own zone + * (`src/functions/period.ts`). The engine deliberately REFUSES a zone it + * cannot resolve rather than falling back to UTC, because a duty quietly + * resolving "the 5th of the month" in the wrong zone is a wrong due date + * nobody can see is wrong. That is the right call for the consumer — but + * without this guard the author never hears about a typo until the nightly + * dispatcher touches the record, where the throw is attributed to the job + * rather than to the duty, and every other duty in the same run is collateral. + * + * ── Where the check belongs: WRITE time. Not author time, not dispatch ──── + * Three moments were available and only one of them is where the typo is: + * + * - AUTHOR TIME (`pnpm validate`) sees METADATA. Duties are records, created + * by people in a form at run time, and the linter never sees one. The only + * zone literals in this repo's metadata are the field's own `defaultValue` + * and nothing else — so an author-time check would be a check on a + * population that does not contain the defect. (It is not needed as a + * second line either: a typo'd field default would be caught by this guard + * on the very first duty anyone creates, loudly and immediately.) + * - WRITE TIME is where a person types `Europe/Munich` and presses save. It + * covers the form, REST/OpenAPI/MCP, the catalog-apply action, and the seed + * path — `skipTriggers` suppresses record-change automation, not hooks. + * - DISPATCH TIME is where it fails today. Too late and mis-attributed. + * + * ── Why a hook rather than a `validation` on the object ─────────────────── + * A validation rule was the obvious home and it cannot express this. Rules are + * CEL, and the whole stdlib in `@objectstack/formula` is `now today + * daysFromNow daysAgo isBlank coalesce trim joinNonEmpty daysBetween addDays + * addMonths date datetime abs round floor ceil min max upper lower contains + * startsWith endsWith matches len isEmpty` — no zone oracle, and no way for an + * application to register one. The only reachable spelling would be + * `matches(record.timezone, '')`, which either checks shape only (and + * `Europe/Munich` is perfectly well shaped) or freezes a tzdata snapshot into + * metadata that disagrees with the host's. `@objectstack/spec` publishes the + * right vocabulary for this — `valueDomain: 'iana_time_zone'` — but only on a + * settings specifier; an object field has no equivalent. Filed upstream as + * **objectstack-ai/objectstack#14168**; a lifecycle hook is what + * `validation.zod.ts` itself prescribes in the meantime ("Custom handler → a + * `beforeInsert` / `beforeUpdate` lifecycle hook, the typed, supported + * extension point for arbitrary validation code"). + * + * ── ⛔ Why `handler` is a STRING and must stay one ──────────────────────── + * This is the load-bearing line in the file. `objectstack build` lowers a + * self-contained INLINE handler into a metadata `body`, which runs in the + * QuickJS sandbox — and that sandbox has **no `Intl`** (measured on + * quickjs-emscripten 0.32.0, the variant the runtime wires through + * `QuickJSScriptRunner`: `typeof Intl` is `undefined`, while `Date` and `JSON` + * are present). No `HookBodyCapability` grants it either. `resolveHandler` + * prefers `body` over `handler` whenever both exist, so writing this as an + * inline function would ship a hook that throws `ReferenceError: Intl is not + * defined` on every duty write — and with `onError: 'abort'` that refuses + * every write to `duly_duty`. All four gates stay GREEN while that is true, + * because tests run the raw function in Node, where `Intl` exists. + * + * The string ref keeps the probe in Node: nothing inline for the extractor to + * lower, so no `body` is emitted, and `resolveHandler` falls through to + * `opts.functions[handler]` — the `defineStack({ functions })` map, the same + * path the dispatch job's handler already takes. `hook.zod.ts` marks `handler` + * deprecated in favour of `body`; following that here would break the check, + * which is half of what #14168 asks the platform to reconcile. + * + * DO NOT "modernise" this into an inline handler or a `body`. + * `test/duty-timezone.test.ts` pins the string form for that reason. + * + * ── Why no declarative `condition` ──────────────────────────────────────── + * A CEL `condition` of `!isBlank(record.timezone)` is the natural way to skip the + * handler when there is nothing to check, and it silently reopens half the + * defect: `isBlank('')` is TRUE, so an explicit empty string would be waved + * through — and `''` is one of the values that fails at dispatch, since + * `dispatch.plan.ts`'s `duty.timezone ?? DEFAULT_TIMEZONE` catches null and + * undefined but not `''`. Presence is therefore decided in the handler, where + * "key absent" and "key present and empty" are still distinguishable. + */ +export const DUTY_TIMEZONE_GUARD_HANDLER = 'dulyValidateDutyTimezone'; + +/** + * A payload that carries no `timezone` key is NOT this guard's business. + * + * On insert the field default supplies `'UTC'`; on update, an untouched zone + * belongs to the write that set it, and refusing an unrelated edit because of + * a pre-existing bad value would punish the wrong person. Where the value + * comes from in the first place is a different, still-open question — a duty + * has no source to resolve a zone from, which is duly#26 and deliberately not + * settled here. This guard judges values, never their absence. + */ +export const dulyValidateDutyTimezone = (ctx: HookContext): void => { + // `input.` IS the record field on a declarative code handler; the + // `input.data.` envelope spelling belongs to the raw + // `engine.registerHook` form and is not what this receives. + const input = ctx.input as Record; + + if (!('timezone' in input)) return; + + const value = input.timezone; + if (value === null || value === undefined) return; + + if (typeof value === 'string' && isResolvableTimeZone(value)) return; + + // Quote the value: the failures worth naming here are invisible otherwise — + // `"Asia/Shanghai "` with a trailing space and `""` both read as blank in an + // unquoted message. + const message = + `Timezone ${JSON.stringify(value)} is not a time zone this system can resolve. ` + + 'Use an IANA name such as Europe/Berlin, Asia/Shanghai or UTC. ' + + 'Period boundaries and due dates are computed in this zone, so an ' + + 'unresolvable one would fail later, inside the nightly dispatch, ' + + 'instead of here.'; + + // The ADR-0112 refusal envelope, shaped to match the engine's own + // `ValidationError` (name / `code` / `fields`) so a caller cannot tell this + // refusal apart from the object's declared validation rules. It is built by + // hand rather than imported because `ValidationError` is internal to + // `@objectstack/objectql`, which this app does not depend on directly. + // `status` is deliberately not set: the platform's own ValidationError + // carries none, and inventing one here would override the boundary's mapping. + const error = new Error(message) as Error & { code: string; fields: unknown[] }; + error.name = 'ValidationError'; + error.code = 'VALIDATION_FAILED'; + error.fields = [{ field: 'timezone', code: 'INVALID_VALUE', message }]; + throw error; +}; + +export const DutyTimezoneGuard: Hook = { + name: 'duly_duty_timezone_guard', + label: 'Duty timezone is a real IANA zone', + object: 'duly_duty', + events: ['beforeInsert', 'beforeUpdate'], + description: + 'Refuses a duly_duty write whose timezone is not a zone this host can resolve, using the ' + + 'same Intl probe the period engine uses — so a typo is caught on the record that has it, ' + + 'instead of throwing inside the nightly dispatch job days later.', + // A guard that fails open is not a guard. If the probe itself throws, the + // write must be refused rather than committed unchecked. + onError: 'abort', + // ⛔ A STRING, not the function. See the note above — an inline handler is + // lowered into the Intl-less QuickJS sandbox and would refuse every write. + handler: DUTY_TIMEZONE_GUARD_HANDLER, +}; diff --git a/src/hooks/index.ts b/src/hooks/index.ts index c78aa71..353240c 100644 --- a/src/hooks/index.ts +++ b/src/hooks/index.ts @@ -13,8 +13,9 @@ // makes `name` optional and fails the assignment. A named array is `never[]` // while empty and infers correctly the moment something is pushed into it. +import { DutyTimezoneGuard } from './duty.hook.js'; import { TaskLifecycleHook } from './task.hook.js'; -export { TaskLifecycleHook }; +export { DutyTimezoneGuard, TaskLifecycleHook }; -export const dulyHooks = [TaskLifecycleHook]; +export const dulyHooks = [DutyTimezoneGuard, TaskLifecycleHook]; diff --git a/test/dispatch.test.ts b/test/dispatch.test.ts index b1ad48b..95d66d6 100644 --- a/test/dispatch.test.ts +++ b/test/dispatch.test.ts @@ -521,7 +521,7 @@ afterEach(async () => { }); let seq = 0; -const seedDuty = async (over: AnyRow = {}): Promise => { +const seedDuty = async (over: AnyRow = {}, options?: AnyRow): Promise => { const created = await data.insert('duly_duty', { name: `Duty ${++seq}`, form: 'recurring', @@ -534,7 +534,7 @@ const seedDuty = async (over: AnyRow = {}): Promise => { lead_days: 0, timezone: 'UTC', ...over, - }); + }, options); const row = (Array.isArray(created) ? created[0] : created) as AnyRow; seeded.push(String(row.id)); return String(row.id); @@ -735,7 +735,20 @@ describe('the job handler', () => { it('reports degraded — not failed — when a duty could not be dispatched', async () => { // `degraded` is "ran to completion, work did not happen". It never retries, // which is right: retrying a typo'd timezone at 01:05 will not fix it. - await seedDuty({ timezone: 'Mars/Olympus' }); + // + // ── Why this row needs `skipAutomations` to exist at all (#24) ──────── + // `duly_duty_timezone_guard` now refuses an unresolvable zone on the way + // in, so an ordinary write can no longer produce this row — which is the + // point of that guard, and this assertion would otherwise have to be + // deleted along with the defect it describes. It must NOT be deleted: the + // rows it models still exist. `skipAutomations` is the platform's own + // "import with run automations unchecked" opt-out (`triggerHooks` skips + // metadata-bound hooks on it), and it is exactly how such a duty is born + // in the wild — an import that bypassed the guard, a row that predates it, + // or a zone the host's tzdata stopped recognising after the duty was + // saved. Dispatch must still degrade rather than fail on those, and must + // still not retry them. + await seedDuty({ timezone: 'Mars/Olympus' }, { context: { skipAutomations: true } }); bindDispatchEngine(data); const outcome = await dulyDispatch({ jobId: DISPATCH_JOB_NAME }); expect(outcome.outcome).toBe('degraded'); diff --git a/test/duty-timezone.test.ts b/test/duty-timezone.test.ts new file mode 100644 index 0000000..2c10aad --- /dev/null +++ b/test/duty-timezone.test.ts @@ -0,0 +1,222 @@ +// 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 stack from '../objectstack.config.js'; +import { dulyFunctions } from '../src/functions/index.js'; +import { isResolvableTimeZone, periodKeyFor } from '../src/functions/period.js'; +import { DutyTimezoneGuard, dulyHooks } from '../src/hooks/index.js'; + +/** + * #24 — `duly_duty.timezone` accepted any string, and the typo surfaced at + * dispatch. + * + * The defect was never "the engine is too strict". `period.ts` refusing an + * unresolvable zone is correct: a duty quietly resolving "the 5th of the + * month" in the wrong zone is a wrong due date nobody can see is wrong. What + * was missing is that the refusal reached the author days late, inside a batch + * job, attributed to the job rather than to the record. + * + * So the property under test is not "bad zones are rejected" on its own. It is + * **the guard and the period engine admit exactly the same set** — a guard + * that were merely strict-ish would trade a late failure for a wrong one. + * `admits the same zones the period engine does` below is the assertion that + * matters; the rest pin the places where it could rot. + */ + +type AnyRow = Record; + +let kernel: { getService(name: string): unknown; shutdown?(): Promise } | undefined; +let data: { + findOne(o: string, q?: AnyRow, x?: AnyRow): Promise; + insert(o: string, d: AnyRow, x?: AnyRow): Promise; + update(o: string, d: AnyRow, x?: AnyRow): Promise; +}; + +beforeAll(async () => { + const { plugins } = await createStandaloneStack({ + databaseDriver: 'memory', + skipSeedData: true, + // See task-hook.test.ts for why this must not resolve to a real path: a + // local `pnpm build` would make the suite report on the last BUILD instead + // of on `src/`, and the registration pins below would pass on dead code. + artifactPath: 'dist/objectstack.this-suite-must-not-load-an-artifact.json', + }); + const k = new ObjectKernel(); + for (const plugin of plugins) await k.use(plugin); + await k.use(new AppPlugin(stack, undefined, { skipSeedData: true })); + await k.bootstrap(); + kernel = k as unknown as typeof kernel; + data = k.getService('data') as typeof data; +}, 180_000); + +afterAll(async () => { + await kernel?.shutdown?.(); +}); + +/** Assert a refusal by its ENVELOPE (ADR-0112), never by the bare fact that it threw. */ +async function refusal(promise: Promise): Promise<{ code: unknown; message: string }> { + try { + await promise; + } catch (error: any) { + return { code: error?.code, message: String(error?.message ?? '') }; + } + throw new Error('expected the write to be refused, but it resolved'); +} + +let seq = 0; + +const insertDuty = async (over: AnyRow = {}): Promise => { + const created = await data.insert('duly_duty', { + name: `Duty ${++seq}`, + owner: `user_${seq}`, + source: 'self', + status: 'active', + ...over, + }); + return Array.isArray(created) ? created[0] : created; +}; + +/** + * Zones the host resolves. Deliberately not all `Europe/X` lookalikes: `GMT`, + * `US/Eastern` and `america/new_york` are the ones an enumerated-list oracle + * would wrongly refuse, and `UTC` is the field's own declared default. + */ +const RESOLVABLE = ['UTC', 'Europe/Berlin', 'Asia/Shanghai', 'Asia/Kolkata', 'GMT', 'US/Eastern', 'america/new_york', 'Pacific/Auckland']; + +/** Every one of these reached dispatch and threw there before this guard. */ +const UNRESOLVABLE = ['Europe/Munich', 'CET+1', 'Asia/Shanghai ', '', ' ', 'utc/utc']; + +describe('the membership oracle', () => { + it('admits the same zones the period engine does', () => { + // The whole point. If these two ever disagree, one of two bugs is live: + // the guard refuses a duty that would have dispatched perfectly well, or + // it passes one that still throws on dispatch night — the defect #24 is + // about, reintroduced behind a check that looks like it is working. + const instant = new Date('2026-08-21T12:00:00Z'); + const engineAccepts = (zone: string): boolean => { + try { + periodKeyFor('daily', instant, zone); + return true; + } catch { + return false; + } + }; + + for (const zone of [...RESOLVABLE, ...UNRESOLVABLE]) { + expect(isResolvableTimeZone(zone), `guard verdict for ${JSON.stringify(zone)}`) + .toBe(engineAccepts(zone)); + } + + // …and the two halves are not both trivially true. + expect(RESOLVABLE.every((z) => isResolvableTimeZone(z))).toBe(true); + expect(UNRESOLVABLE.some((z) => isResolvableTimeZone(z))).toBe(false); + }); + + it('is the Intl probe, not Intl.supportedValuesOf', () => { + // Kept as an executable footnote to the comment in `period.ts`: the + // enumerated list is the tempting oracle and it omits `UTC`, which is + // `duly_duty.timezone`'s own `defaultValue`. Building the guard on it + // would refuse every duty created with the default. + // + // If a future ICU adds these to the list, delete this pin — never the + // probe. The list being right about UTC would not make it right about + // `US/Eastern`, `GMT` or `Asia/Kolkata`. + const enumerated = new Set(Intl.supportedValuesOf('timeZone')); + const acceptedButNotEnumerated = RESOLVABLE.filter((z) => !enumerated.has(z)); + + expect(enumerated.has('UTC')).toBe(false); + expect(isResolvableTimeZone('UTC')).toBe(true); + expect(acceptedButNotEnumerated.length).toBeGreaterThan(0); + }); +}); + +describe('the write path', () => { + it.each(UNRESOLVABLE)('refuses an insert carrying %j', async (zone) => { + const { code, message } = await refusal(insertDuty({ timezone: zone })); + expect(code).toBe('VALIDATION_FAILED'); + // The value is quoted in the message because that is the only way a + // trailing space or an empty string is visible to the person who typed it. + expect(message).toContain(JSON.stringify(zone)); + expect(message).toContain('IANA'); + }); + + it.each(RESOLVABLE)('accepts %s and stores it verbatim', async (zone) => { + const created = await insertDuty({ timezone: zone }); + const stored = await data.findOne('duly_duty', { where: { id: created.id } }); + // Verbatim: the guard validates, it does not canonicalise. Rewriting + // `america/new_york` to `America/New_York` on the way in would be changing + // the author's data, and the period engine resolves both identically. + expect(stored?.timezone).toBe(zone); + }); + + it('refuses an update that introduces a bad zone', async () => { + const created = await insertDuty({ timezone: 'Europe/Berlin' }); + + const { code, message } = await refusal( + data.update('duly_duty', { id: created.id, timezone: 'Europe/Munich' }), + ); + expect(code).toBe('VALIDATION_FAILED'); + expect(message).toContain('"Europe/Munich"'); + + const stored = await data.findOne('duly_duty', { where: { id: created.id } }); + expect(stored?.timezone).toBe('Europe/Berlin'); + }); + + it('leaves a write that does not touch the timezone alone', async () => { + // The guard judges VALUES, never their absence — an unrelated edit must + // not be refused, and on insert the field default is what supplies the + // zone. Where a duty's zone should COME from is a separate, still-open + // question (#26); nothing here settles it, and in particular this guard is + // not a back-door `required: true`. + const created = await insertDuty(); + expect(created.timezone).toBe('UTC'); + + await data.update('duly_duty', { id: created.id, name: 'Renamed, zone untouched' }); + const stored = await data.findOne('duly_duty', { where: { id: created.id } }); + expect(stored?.name).toBe('Renamed, zone untouched'); + expect(stored?.timezone).toBe('UTC'); + }); +}); + +describe('the guard is wired the one way that works', () => { + it('is reachable from the hooks barrel', () => { + // A `*.hook.ts` missing from `dulyHooks` type-checks, reads as wired, and + // never runs. + expect(dulyHooks).toContain(DutyTimezoneGuard); + expect(DutyTimezoneGuard.object).toBe('duly_duty'); + expect(DutyTimezoneGuard.events).toEqual(['beforeInsert', 'beforeUpdate']); + expect(DutyTimezoneGuard.onError).toBe('abort'); + }); + + it('declares a STRING handler and no body', () => { + // ⛔ The tripwire for the trap described at length in `duty.hook.ts`. + // + // `objectstack build` lowers a self-contained INLINE handler into a + // metadata `body`, which runs in the QuickJS sandbox — and that sandbox + // has no `Intl` (measured: `typeof Intl === 'undefined'` on + // quickjs-emscripten 0.32.0, the variant the runtime wires up). Since + // `resolveHandler` prefers `body` over `handler`, "modernising" this hook + // into an inline function ships a guard that throws + // `ReferenceError: Intl is not defined` on every duty write and, with + // `onError: 'abort'`, refuses all of them — while all four gates stay + // green, because tests run the raw function in Node. + // + // This assertion is the only thing standing between that change and + // production. Filed upstream as objectstack-ai/objectstack#14168. + expect(typeof DutyTimezoneGuard.handler).toBe('string'); + expect(DutyTimezoneGuard.body).toBeUndefined(); + }); + + it('resolves that handler name against the functions map', () => { + // A string handler absent from `defineStack({ functions })` is not an + // error: `bindHooksToEngine` logs "skipping hook with unresolved handler" + // and moves on, leaving the guard dead and every gate green. The refusal + // tests above would catch it too — this one names the reason. + const name = DutyTimezoneGuard.handler as string; + const entry = (dulyFunctions as Record)[name]; + const handler = typeof entry === 'function' ? entry : (entry as { handler?: unknown })?.handler; + expect(typeof handler).toBe('function'); + }); +});