From 868c8a561b9995c15aca18c061904bf857bf2b86 Mon Sep 17 00:00:00 2001 From: Warren Date: Wed, 2 Sep 2026 04:47:19 +0000 Subject: [PATCH 1/4] Confirm and approve a duty before it dispatches (#107) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `duly_duty` gains `review_status` — `to_confirm → to_review → approved | returned` — and the dispatcher refuses anything that is not `approved`. A returned duty stops producing tasks the same night; the tasks it already produced stay where they are. The whole workflow rides the platform's own clickable status pipeline (`trackHistory` was already on) rather than declared actions, which render nowhere in the Console today (objectui#7234). Three enforcements, all platform surfaces, no authorization hook: - `review_status_transitions`, a `state_machine` rule, pins the legal steps and — through `initialStates` — stops a duty being born mid-pipeline. It adds one edge the card's table does not draw, `approved → returned`: measured, a terminal `approved` leaves a live duty with no correction path at all, for anyone. - `returned_needs_note` makes the reason mandatory, and it stays readable after the duty goes back up for review. - The two VERDICT options carry `visibleWhen: record.owner != current_user.id` — the one authoring surface whose predicate scope binds the caller, enforced by the rule validator on the write path, not just hidden in the picker. It names the record relationship rather than a position because `current_user.positions` is populated from rows a package may not declare, so a position predicate would fail closed in every deployment that has not done the manual binding step, `pnpm demo` included. Seed: 25 approved, 2 waiting on their owner, 2 waiting on a reviewer, 1 returned with a reason — two of them the demo account's own, so the pipeline is walkable on a record page without a second login. History is planned as `approved` throughout: those tasks were dispatched, which is exactly why returning a duty stops the next run rather than retracting the last six months. Views 「待我确认」 and 「待我审定」, both in the navigation, plus the zh-CN entries for every new label, option and view. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01SqkTcrxUFci7nqXdbBSe2p --- src/apps/duly.app.ts | 10 + src/data/demo-catalog.ts | 37 ++- src/data/demo-history.ts | 14 ++ src/data/duty.seed.ts | 22 ++ src/jobs/dispatch.job.ts | 15 +- src/jobs/dispatch.plan.ts | 31 +++ src/objects/duty.object.ts | 153 +++++++++++- src/translations/authored-text.ts | 14 ++ src/translations/zh-CN.ts | 37 +++ src/views/duty.view.ts | 66 ++++++ test/dispatch-wiring.test.ts | 10 +- test/dispatch.test.ts | 116 +++++++++- test/duty-review.test.ts | 370 ++++++++++++++++++++++++++++++ test/i18n-coverage.test.ts | 8 +- test/reminders.test.ts | 5 + test/seed.test.ts | 81 +++++++ test/task-hook.test.ts | 6 + 17 files changed, 985 insertions(+), 10 deletions(-) create mode 100644 test/duty-review.test.ts diff --git a/src/apps/duly.app.ts b/src/apps/duly.app.ts index 4b017d6..93f39b8 100644 --- a/src/apps/duly.app.ts +++ b/src/apps/duly.app.ts @@ -26,6 +26,11 @@ export const DulyApp = App.create({ children: [ { id: 'nav_my_week', type: 'object', objectName: 'duly_task', viewName: 'my_week', label: 'My week', icon: 'calendar-check' }, { id: 'nav_my_duties', type: 'object', objectName: 'duly_duty', viewName: 'mine', label: 'My duties', icon: 'clipboard-list' }, + // Directly under "My duties", because it is the same list filtered to + // the rows that are waiting on this person — to confirm, or to correct + // and send back up. An unconfirmed duty dispatches nothing, so this is + // the one entry in the group whose emptiness is the goal. + { id: 'nav_to_confirm', type: 'object', objectName: 'duly_duty', viewName: 'to_confirm', label: 'To confirm', icon: 'clipboard-check' }, { id: 'nav_standing', type: 'object', objectName: 'duly_duty', viewName: 'standing', label: 'Standing duties', icon: 'anchor' }, { id: 'nav_log', type: 'object', objectName: 'duly_log_entry', label: 'Work log', icon: 'notebook-pen' }, // The board lives HERE, not under Team, and that placement is a @@ -48,6 +53,11 @@ export const DulyApp = App.create({ // nav item carries `dashboardName` (resolved against the dashboards // barrel), never an `objectName` — nothing on it is entered. { id: 'nav_duty_health', type: 'dashboard', dashboardName: 'duly_duty_health', label: 'Duty health', icon: 'activity' }, + // A reviewer's queue, and the only entry in this group that is a + // WRITE surface. It sits with the manager's screens rather than under + // "My work" for the same reason the board does not: the rows in it + // belong to other people. + { id: 'nav_to_review', type: 'object', objectName: 'duly_duty', viewName: 'to_review', label: 'To review', icon: 'clipboard-pen' }, { id: 'nav_late', type: 'object', objectName: 'duly_task', viewName: 'late', label: 'Late', icon: 'alert-circle' }, { id: 'nav_stalled', type: 'object', objectName: 'duly_task', viewName: 'stalled', label: 'Not moving', icon: 'pause-circle' }, { id: 'nav_assignments', type: 'object', objectName: 'duly_assignment', viewName: 'sent_by_me', label: 'Assignments', icon: 'send' }, diff --git a/src/data/demo-catalog.ts b/src/data/demo-catalog.ts index 14bf704..a24c221 100644 --- a/src/data/demo-catalog.ts +++ b/src/data/demo-catalog.ts @@ -381,6 +381,19 @@ export interface DemoDuty { owner: string; source: 'catalog' | 'assigned' | 'self'; status?: 'active' | 'paused' | 'retired'; + /** + * Where this duty sits in the review pipeline TODAY (#107). Omitted means + * `approved` — the steady state of a duty that is actually being worked, + * and the only state that dispatches. + * + * The fixture keeps a handful of rows off `approved` on purpose: an empty + * "To confirm" and an empty "To review" are two screens a demo cannot show, + * and "unapproved duties do not dispatch" is not a claim anybody can check + * against a fixture where every duty is approved. + */ + review?: 'to_confirm' | 'to_review' | 'approved' | 'returned'; + /** Mandatory on a `returned` row — `returned_needs_note` refuses one without it. */ + reviewNote?: string; /** Self-declared duties carry their own cadence — there is no catalog row behind them. */ own?: Partial & { form: Form }; } @@ -403,7 +416,11 @@ export const DUTIES: readonly DemoDuty[] = [ { name: 'Emissions return — Northgate', item: 'Emissions return', owner: ADMIN, source: 'catalog' }, { name: 'Waste transfer log review — Northgate', item: 'Waste transfer log review', owner: ADMIN, source: 'catalog' }, { name: 'Permit condition review — Northgate', item: 'Permit condition review', owner: ADMIN, source: 'catalog' }, - { name: 'Site environmental audit — Northgate', item: 'Site environmental audit', owner: ADMIN, source: 'catalog' }, + // Waiting on the evaluator's OWN confirmation, so the pipeline is + // clickable on the first record page they open. Its in-flight task stays + // where it is — returning or un-confirming a duty stops the NEXT run, it + // does not retract work already dispatched. + { name: 'Site environmental audit — Northgate', item: 'Site environmental audit', owner: ADMIN, source: 'catalog', review: 'to_confirm' }, { name: 'Keep the permit register current — Northgate', item: 'Keep the permit register current', owner: ADMIN, source: 'catalog' }, { name: 'Keep up with regulator bulletins', @@ -411,6 +428,12 @@ export const DUTIES: readonly DemoDuty[] = [ owner: ADMIN, source: 'self', own: { form: 'recurring', frequency: 'monthly', dueAnchor: 'period_start', dueOffsetDays: 7, leadDays: 7, graceDays: 0, description: 'Read the month\'s bulletins and note anything that changes what the site owes.' }, + // The returned one. Owned by the evaluator, so `returned → to_review` — + // the leg only the OWNER may walk — is demonstrable without a second + // login. It is also the state that stops dispatch, which is the most + // persuasive thing this card has to show. + review: 'returned', + reviewNote: 'Reading the bulletins is not the duty — the duty is recording what changed and who has to act. Rewrite the acceptance bar and send it back.', }, // ── Northgate Quality ───────────────────────────────────────────────── @@ -422,12 +445,20 @@ export const DUTIES: readonly DemoDuty[] = [ owner: 'Priya Raman', source: 'self', own: { form: 'recurring', frequency: 'monthly', dueAnchor: 'period_start', dueOffsetDays: 5, leadDays: 7, graceDays: 0, description: 'Half an hour with the month\'s nonconformances and calibration deviations, looking for the shape rather than the individual events.' }, + // Self-declared duties are born `to_review`, not `to_confirm` — writing + // one down IS the confirmation. This row is what that default looks like + // on screen, and it gives the reviewer's queue a second entry that is not + // a catalog duty. + review: 'to_review', }, { name: 'Calibration verification — Lab 1', item: 'Calibration verification', owner: 'Rosa Delgado', source: 'catalog' }, - { name: 'Retained sample review — Lab 1', item: 'Retained sample review', owner: 'Rosa Delgado', source: 'catalog' }, + // A second unconfirmed list, on somebody else — so a manager opening + // "To review" sees a queue rather than one row, and so "To confirm" is + // visibly a per-person list rather than a global one. + { name: 'Retained sample review — Lab 1', item: 'Retained sample review', owner: 'Rosa Delgado', source: 'catalog', review: 'to_confirm' }, { name: 'Nonconformance log review — Northgate Quality', item: 'Nonconformance log review', owner: 'Rosa Delgado', source: 'catalog' }, { name: 'Calibration verification — Lab 2', item: 'Calibration verification', owner: 'Ibrahim Chaudhry', source: 'catalog' }, - { name: 'Instrument drift check — Lab 2', item: 'Instrument drift check', owner: 'Ibrahim Chaudhry', source: 'catalog' }, + { name: 'Instrument drift check — Lab 2', item: 'Instrument drift check', owner: 'Ibrahim Chaudhry', source: 'catalog', review: 'to_review' }, { name: 'Track my own training hours', item: null, diff --git a/src/data/demo-history.ts b/src/data/demo-history.ts index 792fbfe..8133ca6 100644 --- a/src/data/demo-history.ts +++ b/src/data/demo-history.ts @@ -89,6 +89,20 @@ export const DISPATCH_DUTIES: readonly DispatchDuty[] = DUTIES.map((duty) => { owner: duty.owner, business_unit: unit, source: duty.source, + // ── Always `approved`, whatever the duty's review state is TODAY ────── + // The planner refuses an unapproved duty (`not_approved`), so passing + // each row's real review state would delete the history of every duty + // this fixture leaves mid-pipeline — including the two the "Late" and + // "Not moving" stories are told with. + // + // That would also be the wrong history. `review_status` is a fact about + // now, not about the six months behind it: those tasks WERE dispatched, + // which is precisely why returning a duty stops the next run rather than + // retracting the work already owed (pinned in `test/dispatch.test.ts`). + // A duty sitting in `to_confirm` with tasks behind it is the import case + // this card exists for — the work was always being done; the list is only + // now being formalised. + review_status: 'approved', frequency: cadence.frequency ?? null, due_anchor: cadence.due_anchor ?? null, due_offset_days: cadence.due_offset_days ?? null, diff --git a/src/data/duty.seed.ts b/src/data/duty.seed.ts index 9862185..6399107 100644 --- a/src/data/duty.seed.ts +++ b/src/data/duty.seed.ts @@ -35,6 +35,12 @@ import { HISTORY_FROM } from './demo-history.js'; * obligation, and the duty stays on screen. * - **One one-off**, dispatched by hand rather than by the scheduler. Its * single task is seeded directly in `task.seed.ts`. + * - **Five duties off `approved`** (#107) — two waiting on their owner to + * confirm, two waiting on a reviewer, one returned with a reason. They hold + * zero NEW tasks for a third reason (`not_approved`) while keeping the + * history they were dispatched with, which is the import case the review + * pipeline exists for. Two of the five are the demo account's own, so the + * pipeline can be walked on a record page without a second login. */ /** A self-declared duty has no catalog row behind it, so it carries its own cadence. */ @@ -65,6 +71,22 @@ export const dutySeed = defineSeed(Duty, { // exercises the zone handling rather than leaving every row on UTC. timezone: timezoneOf(unit), status: duty.status ?? 'active', + // ── Review state: the seed states it, and states it on every row ────── + // Not left to the field default, for the same reason `source` is not + // (see the ⚠️ above): the default keys on `source`, so leaving it out + // would silently put every self-declared duty in `to_review` and every + // catalog duty in `to_confirm` — a demo where nothing dispatches and + // the dashboard reads zero, with nothing erroring anywhere. + // + // The seed loader writes with `seedReplay: true`, which is what lets it + // land `approved` and `returned` rows directly: the platform skips + // `state_machine` rules on that context, so the fixture does not have + // to walk 30 duties through the pipeline one legal transition at a + // time. `returned_needs_note` is an ordinary script rule and is NOT + // skipped — a returned row without a reason would be refused, which is + // why `reviewNote` is required on that shape in `demo-catalog.ts`. + review_status: duty.review ?? 'approved', + ...(duty.reviewNote ? { review_note: duty.reviewNote } : {}), // The history this seed backfills starts here, so the duties say so. // Without it the effective window is open-ended and a later backfill // would happily invent obligations that predate the demo. diff --git a/src/jobs/dispatch.job.ts b/src/jobs/dispatch.job.ts index be4602a..9b4130b 100644 --- a/src/jobs/dispatch.job.ts +++ b/src/jobs/dispatch.job.ts @@ -5,6 +5,7 @@ import { defineJob } from '@objectstack/spec'; import { DISPATCH_DUTY_FIELDS, DISPATCHABLE_FORM, + DISPATCHABLE_REVIEW_STATUS, DISPATCHABLE_STATUS, FAULT_SKIP_REASONS, nextDispatchedPeriod, @@ -300,14 +301,22 @@ export async function runDispatch(engine: DispatchEngine, options: DispatchOptio } /** - * Every active recurring duty, paged. + * Every active, approved, recurring duty, paged. * - * The filter is deliberately only the two zone-independent facts. The + * The filter is deliberately only the three zone-independent facts. The * effective-window test is NOT pushed into the query: `effective_from` and * `effective_to` are calendar days in the DUTY's own zone, and "today" is a * different day in Auckland and in Los Angeles at the same instant, so a single * SQL predicate cannot be right for every row it matches. The window test * belongs where the zone is known, which is the planner. + * + * `review_status` is pushed down here AND checked again in the planner, and + * that is not a redundant belt: this query is one of two callers. A backfill + * driven straight through `planDispatch` — and every planner test — never + * passes this filter at all, so the planner's own `not_approved` check is what + * makes the rule true for both. What the filter adds is that the nightly sweep + * does not read, page and walk every unapproved duty in the tenant to discard + * them one at a time. */ async function readDispatchableDuties(engine: DispatchEngine): Promise { const duties: DispatchDuty[] = []; @@ -315,7 +324,7 @@ async function readDispatchableDuties(engine: DispatchEngine): Promise str(ctx.ids.name); */ const OPAQUE: Readonly> = { 'object.indexes': 'index definitions — field names and uniqueness scopes', + // `{ fromState: [toState] }` — state VALUES on both sides, never a label. + // Opaque rather than one verdict per state, so adding a state to a pipeline + // does not also mean adding a row to this table. + 'object.validations[].transitions': 'state-machine transition table — stored state values', 'object.fields{}.summaryOperations': 'rollup wiring — object, field and function names', 'object.enable': 'capability flags', 'view.data': 'the view\'s data binding — provider and object name', @@ -232,6 +236,11 @@ const VERDICTS: Readonly> = { }), 'object.fields{}.options[].value': machine('the stored option value — the option key itself'), 'object.fields{}.options[].color': machine('option colour'), + // Per-option `visibleWhen` (#107) — a CEL predicate over `record` and + // `current_user`, evaluated by the form AND by the write path. Machine, like + // every other expression envelope in this table. + 'object.fields{}.options[].visibleWhen.dialect': machine('expression dialect'), + 'object.fields{}.options[].visibleWhen.source': machine('CEL source'), 'object.fields{}.type': machine('field type'), 'object.fields{}.reference': machine('lookup target object'), 'object.fields{}.deleteBehavior': machine('referential action'), @@ -253,6 +262,11 @@ const VERDICTS: Readonly> = { 'object.validations[].events[]': machine('lifecycle events'), 'object.validations[].condition.dialect': machine('expression dialect'), 'object.validations[].condition.source': machine('CEL source'), + // The `state_machine` variant's own keys (#107). `field` and + // `initialStates` name a field and stored values; the transition table + // itself is opaque above. + 'object.validations[].field': machine('the state field a state_machine rule governs'), + 'object.validations[].initialStates[]': machine('stored states a record may be created in'), // ── view (walked per expanded view item, so `list` and `listViews.*` // normalise to the same paths) ──────────────────────────────────────── diff --git a/src/translations/zh-CN.ts b/src/translations/zh-CN.ts index cf2b1b9..58c44ab 100644 --- a/src/translations/zh-CN.ts +++ b/src/translations/zh-CN.ts @@ -67,6 +67,21 @@ import { defineTranslationBundle } from '@objectstack/spec'; * below uses it — recorded so the next translator does not invent a second * word for it. * + * **confirm / review / return → 确认 / 审定 / 打回** (`review_status`, #107) + * The three moves in the review pipeline, and all three words come from the + * card that specified it — product wording, not a translator's choice, so + * they are transcribed rather than re-derived. + * **确认** is the OWNER's move: "yes, this list is mine". **审定** is the + * reviewer's: an organisational sign-off, which is why the field is + * `审定状态` even though the owner's step is inside it — 审定 names the whole + * flow, 确认 names one step of it. **打回** is the return, the ordinary + * Chinese office word for sending a submission back with a reason. + * ⛔ NOT 审批 for 审定 — 审批 is approval of a REQUEST somebody raised + * (leave, expense), and this is adjudication of a standing list; the frame + * is different and the demo audience hears the difference. + * ⛔ NOT 驳回 for 打回 — 驳回 is a rejection that ends the matter (a court + * dismisses a claim). A returned duty is expected back, corrected. + * * Supporting choices, same reasoning, less contested: * business unit → 部门 · role catalog → 岗位职责库 · assignment → 指派 · * dispatch → 派发 · not moving / stagnation → 停滞 · grace → 宽限期 · @@ -169,6 +184,24 @@ export const dulyChinese = defineTranslationBundle({ retired: '停用', }, }, + // 审定流水线（#107）。四个选项的中文取自卡片本身的用词——它同时是 + // 产品方的措辞裁决，不需要再译一遍。「状态」这个词此处不与上面的 + // `status`（启用／暂停／停用）冲突：那是职责本身还在不在，这是它有没有 + // 被认领与批准，两个都要读得出来，所以一个是「状态」一个是「审定状态」。 + review_status: { + label: '审定状态', + help: '这条职责在「确认—审定」流程中的位置。只有「已审定」才会派发任务——被打回的职责当天就不再生成新任务，这正是打回的意义。', + options: { + to_confirm: '待确认', + to_review: '待审定', + approved: '已审定', + returned: '已打回', + }, + }, + review_note: { + label: '打回原因', + help: '为什么把这条职责退回去，要写成负责人拿得起、改得动的话。打回时必填（`returned_needs_note`）；重新提交后不会被清空——正在照着修改的人还要再读一遍。', + }, effective_from: { label: '生效日期' }, effective_to: { label: '失效日期' }, last_dispatched_period: { @@ -178,6 +211,8 @@ export const dulyChinese = defineTranslationBundle({ }, _views: { mine: { label: '我的职责' }, + to_confirm: { label: '待我确认' }, + to_review: { label: '待我审定' }, standing: { label: '常设职责' }, catalog_tree: { label: '各团队应尽的职责' }, default: { label: '全部职责' }, @@ -459,11 +494,13 @@ export const dulyChinese = defineTranslationBundle({ group_me: { label: '我的工作' }, nav_my_week: { label: '我的本周' }, nav_my_duties: { label: '我的职责' }, + nav_to_confirm: { label: '待我确认' }, nav_standing: { label: '常设职责' }, nav_log: { label: '工作日志' }, nav_board: { label: '看板' }, group_team: { label: '团队' }, nav_duty_health: { label: '职责健康度' }, + nav_to_review: { label: '待我审定' }, nav_late: { label: '逾期' }, nav_stalled: { label: '停滞' }, nav_assignments: { label: '指派' }, diff --git a/src/views/duty.view.ts b/src/views/duty.view.ts index 9ff6508..504954d 100644 --- a/src/views/duty.view.ts +++ b/src/views/duty.view.ts @@ -15,6 +15,7 @@ export const DutyViews = defineView({ { field: 'frequency' }, { field: 'owner' }, { field: 'source' }, + { field: 'review_status' }, { field: 'status' }, ], }, @@ -29,11 +30,76 @@ export const DutyViews = defineView({ { field: 'form' }, { field: 'frequency' }, { field: 'due_anchor' }, + // Where each of my duties stands in the pipeline — including the ones + // that are approved and therefore NOT in "To confirm". A duty list + // that showed only `status` would say "Active" about a duty producing + // no tasks at all. + { field: 'review_status' }, { field: 'status' }, ], filter: [{ field: 'owner', operator: 'equals', value: '{current_user_id}' }], }, + /** + * "What is waiting on ME" — the owner's half of the review pipeline. + * + * Two states in one list, deliberately. `to_confirm` (the organisation + * put this on you; is it yours?) and `returned` (a reviewer sent it back) + * are different sentences, but they are the same ACTION — read it, fix it + * if it needs fixing, send it up — and splitting them would make the + * second one a list most people never open. `review_note` is a column + * rather than a detail-page trip: a returned duty whose reason you have + * to click into is a reason you read once. + */ + to_confirm: { + label: 'To confirm', + type: 'grid', + data, + columns: [ + { field: 'name' }, + { field: 'review_status' }, + { field: 'review_note' }, + { field: 'form' }, + { field: 'frequency' }, + { field: 'source' }, + ], + filter: [ + { field: 'owner', operator: 'equals', value: '{current_user_id}' }, + { field: 'review_status', operator: 'in', value: ['to_confirm', 'returned'] }, + ], + sort: [{ field: 'name', order: 'asc' }], + }, + + /** + * "What is waiting on a reviewer" — the other half. + * + * No owner filter, and that is the point: this list is somebody ELSE's + * duties. How far it reaches is not this view's decision — it is + * `duly_manager`'s `readScope: 'unit_and_below'` on `duly_duty` + * (`src/security/permission-sets.ts`), which resolves to owner-only on an + * open-edition boot and to the unit tree with `@objectstack/security- + * enterprise` installed. A view that tried to express reach in its own + * filter would be a second, weaker copy of the security model. + * + * `owner` is a column here for the same reason it is not a filter: the + * first question a reviewer asks of this list is whose duty each row is. + */ + to_review: { + label: 'To review', + type: 'grid', + data, + columns: [ + { field: 'name' }, + { field: 'owner' }, + { field: 'business_unit' }, + { field: 'form' }, + { field: 'frequency' }, + { field: 'source' }, + ], + filter: [{ field: 'review_status', operator: 'equals', value: 'to_review' }], + sort: [{ field: 'business_unit', order: 'asc' }, { field: 'name', order: 'asc' }], + }, + // Standing duties never produce a task, so they would otherwise be // invisible. They get their own view rather than an infinite backlog. standing: { diff --git a/test/dispatch-wiring.test.ts b/test/dispatch-wiring.test.ts index 2c36751..8cc7d08 100644 --- a/test/dispatch-wiring.test.ts +++ b/test/dispatch-wiring.test.ts @@ -99,7 +99,15 @@ describe('the dispatch engine is bound at boot, through the real onEnable path', due_offset_days: 4, lead_days: 0, timezone: 'UTC', - }); + // Only an APPROVED duty dispatches (#107). Written straight, through + // the platform's own historical-write door — `skipStateMachine` is what + // the REST import endpoint sets for `treatAsHistorical`, and what the + // seed loader reaches via `seedReplay`; the pipeline itself is exercised + // by ordinary writes in `test/duty-review.test.ts`. Without this the + // duty is born `to_confirm` and this suite would report the wiring + // broken when what is actually true is that nobody approved anything. + review_status: 'approved', + }, { context: { skipStateMachine: true } }); const dutyId = String((Array.isArray(created) ? created[0] : created).id); // If `registerDulyActionHandlers` never called `bindDispatchEngine`, this diff --git a/test/dispatch.test.ts b/test/dispatch.test.ts index a6622f5..2be8757 100644 --- a/test/dispatch.test.ts +++ b/test/dispatch.test.ts @@ -23,6 +23,7 @@ import { DEFAULT_GRACE_DAYS, DEFAULT_TIMEZONE, DISPATCH_DUTY_FIELDS, + FAULT_SKIP_REASONS, nextDispatchedPeriod, planDispatch, type DispatchDuty, @@ -149,6 +150,10 @@ describe('the duty projection covers every field the planner reads', () => { 'owner', 'business_unit', 'source', + // The dispatch gate. Omitted from the projection it comes back + // `undefined`, which the planner reads as "not approved" — every duty + // in the tenant would stop dispatching, quietly, on the next deploy. + 'review_status', 'frequency', 'due_anchor', 'due_offset_days', @@ -180,6 +185,11 @@ const duty = (over: Partial = {}): DispatchDuty => ({ owner: 'user_alice', business_unit: 'bu_plant', source: 'catalog', + // The steady state of a duty that is actually being worked. Stated on the + // fixture rather than defaulted inside the planner, so the gate below has + // something to turn OFF — a planner that silently treated a missing review + // state as approved would pass every test in this file. + review_status: 'approved', frequency: 'monthly', due_anchor: 'period_start', due_offset_days: 4, @@ -217,6 +227,49 @@ describe('what is never dispatched', () => { } }); + it('an unapproved duty produces nothing, whatever the review state is', () => { + // The four states the pipeline can be in, and the one that dispatches. + // Written as a sweep rather than one case, because the failure this + // guards against is a gate written as "not returned" — which would let + // `to_confirm` and `to_review` through and make the confirmation step + // decorative. + for (const review_status of ['to_confirm', 'to_review', 'returned']) { + expect(keysOf([duty({ review_status })], now), review_status).toEqual([]); + expect( + planDispatch({ duties: [duty({ review_status })], now }).skipped[0]?.reason, + review_status, + ).toBe('not_approved'); + } + expect(keysOf([duty({ review_status: 'approved' })], now)).toEqual(['2026-08']); + }); + + it('a duty carrying NO review state produces nothing either', () => { + // Fail-closed, and this is the case that decides the direction. A row + // that predates the column, or arrived through a path that skipped the + // default, is not "grandfathered in" — it stops until somebody approves + // it. The recoverable failure is the one that is visible. + expect(keysOf([duty({ review_status: null })], now)).toEqual([]); + expect(keysOf([duty({ review_status: undefined })], now)).toEqual([]); + expect(planDispatch({ duties: [duty({ review_status: null })], now }).skipped[0]?.reason) + .toBe('not_approved'); + }); + + it('an unapproved duty is an ordinary skip, not a degraded run', () => { + // `not_approved` must stay OUT of `FAULT_SKIP_REASONS`. A tenant part-way + // through a rollout has hundreds of them, and a nightly job that reports + // `degraded` every night is a job whose alerts get muted — after which the + // one real fault it exists to report goes unread too. + expect(FAULT_SKIP_REASONS as readonly string[]).not.toContain('not_approved'); + }); + + it('backfilling does not smuggle an unapproved duty past the gate', () => { + // A backfill skips the duty-level effective window on purpose, so it is + // worth pinning that it does NOT skip this. Returning a duty and then + // asking for last month must not re-create the work it stopped. + expect(keysOf([duty({ review_status: 'returned' })], now, { from: '2026-01-01', to: '2026-08-31' })) + .toEqual([]); + }); + it('un-pausing produces only the current period, never the gap', () => { // The duty was last dispatched in February and has been paused since. The // rule is "the current period", not "everything you missed" — a person @@ -605,8 +658,24 @@ const seedDuty = async (over: AnyRow = {}, options?: AnyRow): Promise => due_offset_days: 4, lead_days: 0, timezone: 'UTC', + review_status: 'approved', ...over, - }, options); + }, { + ...options, + context: { + // A duty may not be CREATED `approved` — `review_status_transitions` + // declares `initialStates: ['to_confirm', 'to_review']`, and that is the + // rule, not an obstacle to route around. This fixture is writing + // established history (a duty that was approved some time ago), so it + // takes the platform's own door for exactly that: `skipStateMachine` is + // the context key the REST import endpoint sets for a `treatAsHistorical` + // import, and the seed loader's `seedReplay` reaches the same branch. + // The pipeline itself is exercised through ordinary writes in + // `test/duty-review.test.ts`, which is where it must not be bypassed. + skipStateMachine: true, + ...((options?.context as AnyRow | undefined) ?? {}), + }, + }); const row = (Array.isArray(created) ? created[0] : created) as AnyRow; seeded.push(String(row.id)); return String(row.id); @@ -633,6 +702,51 @@ describe('the index is real on this engine', () => { }); }); +describe('the review gate, against the real engine', () => { + it('a returned duty generates nothing; walking it back to approved generates its tasks', async () => { + // The demo beat, end to end: change one field, tomorrow's work changes. + const dutyId = await seedDuty({ + lead_days: 0, + review_status: 'returned', + review_note: 'Monthly is wrong for this — the permit says quarterly.', + }); + + const blocked = await runDispatch(data, { now: NOW }); + expect(await tasksFor(dutyId), 'a returned duty owes nobody anything').toHaveLength(0); + // It is not even READ: the sweep's own query filters on `review_status`, + // so an unapproved duty costs the nightly run nothing at all. + expect(blocked.skipped.some((skip) => skip.duty === dutyId)).toBe(false); + + // The owner corrects it and sends it back up; the manager approves. Two + // writes, because `returned → approved` is not a step the machine has. + await data.update('duly_duty', { id: dutyId, review_status: 'to_review' }); + await data.update('duly_duty', { id: dutyId, review_status: 'approved' }); + + await runDispatch(data, { now: NOW }); + expect((await tasksFor(dutyId)).map((task) => task.period_key)).toEqual(['2026-08']); + }); + + it('returning an approved duty stops the NEXT run, and leaves the tasks already dispatched alone', async () => { + // The other half, and the one a "just filter the view" implementation + // would get wrong: returning a duty is not a retraction of work already + // owed. August's task stays; September's is never created. + const dutyId = await seedDuty({ lead_days: 0, frequency: 'monthly' }); + await runDispatch(data, { now: NOW }); + expect((await tasksFor(dutyId)).map((task) => task.period_key)).toEqual(['2026-08']); + + await data.update('duly_duty', { + id: dutyId, + review_status: 'returned', + review_note: 'Superseded by the group standard — rewrite the acceptance bar.', + }); + + const september = new Date('2026-09-15T09:00:00Z'); + const after = await runDispatch(data, { now: september }); + expect(after.created, 'a returned duty creates nothing on the next night').toBe(0); + expect((await tasksFor(dutyId)).map((task) => task.period_key)).toEqual(['2026-08']); + }); +}); + describe('running twice over the same clock', () => { it('the second pass inserts nothing, and that is a successful run', async () => { const dutyId = await seedDuty({ lead_days: 0 }); diff --git a/test/duty-review.test.ts b/test/duty-review.test.ts new file mode 100644 index 0000000..7c3c177 --- /dev/null +++ b/test/duty-review.test.ts @@ -0,0 +1,370 @@ +// 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 { Duty } from '../src/objects/index.js'; + +/** + * #107 — confirmation and approval: the pipeline `duly_duty.review_status` + * declares, and the three enforcements behind it. + * + * ── Why every assertion here goes through a booted engine ──────────────── + * All three mechanisms this card rests on are INVISIBLE to `pnpm validate`, + * and each is invisible in a different way: + * + * - The **conditional default** is a CEL expression. `field.zod.ts`'s + * authoring gate returns unconditionally on an expression envelope ("a CEL + * result type is unknowable at parse time"), so a structural pin + * (`Duty.fields.review_status.defaultValue === …`) proves the KEY exists + * and nothing about what it evaluates to. Same reasoning, same shape as + * `test/cadence-conditional-defaults.test.ts`. + * - The **state machine** is a `transitions` table the validator reads at + * write time with the prior row in hand. Nothing at author time compares it + * to the option list, so a table naming a state that does not exist — or + * omitting one that does — parses clean. (Omitting is the dangerous + * direction: measured on `@objectstack/objectql` 17.2.0, a state with NO + * row accepts every transition out of it, so a missing row reads like + * "locked" and behaves like "wide open".) + * - The **per-option `visibleWhen`** is the authorization half, and it is the + * one an author is most likely to mistake for decoration. It is not: the + * rule validator evaluates the PICKED option's predicate on every insert + * and update and refuses the write. The tests at the bottom drive it with + * three different callers, because a predicate that never refuses anybody + * is indistinguishable from no predicate at all. + * + * The dispatch half of #107 — an unapproved duty produces no tasks — lives in + * `test/dispatch.test.ts`, next to the rest of the planner. + */ + +type AnyRow = Record; + +let kernel: { getService(name: string): unknown; shutdown?(): Promise } | undefined; +let data: { + find(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 dispatch.test.ts: left to its default this resolves + // `/dist/objectstack.json`, and a local `pnpm build` would make the + // suite report on the last BUILD instead of on `src/`. + 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 OWNER = 'user_owner'; +const REVIEWER = 'user_reviewer'; + +/** + * A duty, inserted through the ordinary write path. + * + * No `skipStateMachine` and no `isSystem` anywhere in this file: the rules + * under test are exactly the ones those keys turn off, so a fixture that used + * either would be testing the escape hatch. Where an already-approved duty is + * needed, it is WALKED there through the pipeline, one legal transition at a + * time — which is also the shortest honest statement of what the pipeline is. + */ +const insertDuty = async (over: AnyRow = {}, options?: AnyRow): Promise => { + const created = await data.insert('duly_duty', { + name: `Duty ${++seq}`, + form: 'recurring', + owner: OWNER, + source: 'catalog', + status: 'active', + frequency: 'monthly', + timezone: 'UTC', + ...over, + }, options); + return (Array.isArray(created) ? created[0] : created) as AnyRow; +}; + +const readDuty = async (id: unknown): Promise => + (await data.find('duly_duty', { where: { id }, limit: 1 }))[0] as AnyRow; + +/** Move one duty one step, as `who` (a bare user id, or nobody at all). */ +const advance = (id: unknown, to: string, over: AnyRow = {}, who?: string) => + data.update( + 'duly_duty', + { id, review_status: to, ...over }, + who === undefined ? undefined : { context: { userId: who, positions: [] } }, + ); + +/** A duty walked all the way to `approved` by somebody who does not own it. */ +const approvedDuty = async (over: AnyRow = {}): Promise => { + const duty = await insertDuty(over); + await advance(duty.id, 'to_review', {}, OWNER); + await advance(duty.id, 'approved', {}, REVIEWER); + return await readDuty(duty.id); +}; + +// ─────────────────────────────────────────────────────────────────────────── +// Where a duty enters the pipeline +// ─────────────────────────────────────────────────────────────────────────── + +describe('a new duty enters the pipeline where its SOURCE puts it', () => { + it('an organisation-supplied duty waits for the owner to confirm it', async () => { + // The import / catalog case, and the reason the card exists: a list the + // organisation produced is visible immediately and productive only once + // the person who owes it has said "yes, this is mine". + for (const source of ['catalog', 'assigned']) { + const duty = await insertDuty({ source }); + expect((await readDuty(duty.id)).review_status, source).toBe('to_confirm'); + } + }); + + it('a self-declared duty is already confirmed — writing it down IS the confirmation', async () => { + const duty = await insertDuty({ source: 'self' }); + expect((await readDuty(duty.id)).review_status).toBe('to_review'); + }); + + it('the default is reached through `source`, not through the payload', async () => { + // `source` itself defaults to `self` (#54), so a duty created with no + // source at all is self-declared and lands in `to_review`. This is the + // hand-created case, and it is the one that proves the default is being + // EVALUATED rather than copied off a literal. + const duty = await insertDuty({ source: undefined }); + const row = await readDuty(duty.id); + expect(row.source).toBe('self'); + expect(row.review_status).toBe('to_review'); + }); + + it('is required, so no duty can sit outside the pipeline', () => { + expect(Duty.fields.review_status.required).toBe(true); + }); +}); + +describe('a duty cannot be BORN mid-pipeline', () => { + it('refuses an insert that arrives already approved', async () => { + // `transitions` governs updates only, and a `select` accepts any declared + // option on create — so without `initialStates` the whole card could be + // walked around by writing `approved` on the way in, which is exactly the + // defect it exists to close ("imported lists take effect and dispatch + // immediately"). + const { code, message } = await refusal(insertDuty({ review_status: 'approved' })); + expect(code).toBe('VALIDATION_FAILED'); + expect(message).toContain('not a step this review can take'); + }); + + it('refuses an insert that arrives already returned', async () => { + const { code } = await refusal(insertDuty({ review_status: 'returned', review_note: 'no' })); + expect(code).toBe('VALIDATION_FAILED'); + }); + + it('admits the two states a duty may legitimately start in', async () => { + for (const review_status of ['to_confirm', 'to_review']) { + const duty = await insertDuty({ review_status }); + expect((await readDuty(duty.id)).review_status, review_status).toBe(review_status); + } + }); +}); + +// ─────────────────────────────────────────────────────────────────────────── +// The transitions +// ─────────────────────────────────────────────────────────────────────────── + +describe('the pipeline admits exactly the steps it declares', () => { + it('refuses to_confirm → approved — the card\'s own example', async () => { + const duty = await insertDuty(); + const { code, message } = await refusal(advance(duty.id, 'approved', {}, REVIEWER)); + expect(code).toBe('VALIDATION_FAILED'); + expect(message).toContain('not a step this review can take'); + // And the refused write left the row where it was. + expect((await readDuty(duty.id)).review_status).toBe('to_confirm'); + }); + + it('walks to_confirm → to_review → approved', async () => { + const duty = await insertDuty(); + await advance(duty.id, 'to_review', {}, OWNER); + expect((await readDuty(duty.id)).review_status).toBe('to_review'); + await advance(duty.id, 'approved', {}, REVIEWER); + expect((await readDuty(duty.id)).review_status).toBe('approved'); + }); + + it('walks to_review → returned → to_review', async () => { + const duty = await insertDuty({ source: 'self' }); + await advance(duty.id, 'returned', { review_note: 'Quarterly, not monthly.' }, REVIEWER); + expect((await readDuty(duty.id)).review_status).toBe('returned'); + await advance(duty.id, 'to_review', {}, OWNER); + expect((await readDuty(duty.id)).review_status).toBe('to_review'); + }); + + it('refuses returned → approved — a correction goes back through review', async () => { + const duty = await insertDuty({ source: 'self' }); + await advance(duty.id, 'returned', { review_note: 'The owner is wrong.' }, REVIEWER); + const { code } = await refusal(advance(duty.id, 'approved', {}, REVIEWER)); + expect(code).toBe('VALIDATION_FAILED'); + }); + + it('lets an approved duty be RETURNED — the correction path', async () => { + // The one edge the card's table does not draw, added deliberately: with + // `approved` terminal there is no way to correct a live duty at all, by + // anyone. See the rule's comment in `duty.object.ts`. + const duty = await approvedDuty(); + await advance(duty.id, 'returned', { review_note: 'Superseded by the group standard.' }, REVIEWER); + expect((await readDuty(duty.id)).review_status).toBe('returned'); + }); + + it('refuses approved → to_review — the way back is a return, with a reason', async () => { + const duty = await approvedDuty(); + const { code } = await refusal(advance(duty.id, 'to_review', {}, REVIEWER)); + expect(code).toBe('VALIDATION_FAILED'); + expect((await readDuty(duty.id)).review_status).toBe('approved'); + }); + + it('every state the field declares has a row in the table', () => { + // The measured trapdoor: a state with no `transitions` row is not frozen, + // it is UNGOVERNED — `if (!Array.isArray(allowed)) return null` lets every + // transition out of it through. Nothing at author time compares the two + // lists, so this is the comparison. + const rule = (Duty.validations ?? []).find((v: any) => v.name === 'review_status_transitions') as any; + const declared = (Duty.fields.review_status.options ?? []).map((option: any) => String(option.value)); + expect(Object.keys(rule.transitions).sort()).toEqual([...declared].sort()); + // And every TARGET is a real state too — a typo'd target is a step nobody + // can ever take, which reads as "the pipeline is stuck" at run time. + for (const [from, targets] of Object.entries(rule.transitions as Record)) { + for (const target of targets) expect(declared, `${from} → ${target}`).toContain(target); + } + }); +}); + +// ─────────────────────────────────────────────────────────────────────────── +// The reason a return carries +// ─────────────────────────────────────────────────────────────────────────── + +describe('returning a duty says why', () => { + it('refuses a return with no reason', async () => { + const duty = await insertDuty({ source: 'self' }); + const { code, message } = await refusal(advance(duty.id, 'returned', {}, REVIEWER)); + expect(code).toBe('VALIDATION_FAILED'); + expect(message).toBe('Say why the duty is being returned — the owner needs something to act on.'); + expect((await readDuty(duty.id)).review_status).toBe('to_review'); + }); + + it('refuses blanking the reason while the duty is still returned', async () => { + // The rule reads the MERGED record, so an update carrying only + // `review_note: null` is judged against the stored `returned` — which is + // what stops a return from being emptied out after the fact. + const duty = await insertDuty({ source: 'self' }); + await advance(duty.id, 'returned', { review_note: 'The frequency is wrong.' }, REVIEWER); + const { code } = await refusal(data.update('duly_duty', { id: duty.id, review_note: null })); + expect(code).toBe('VALIDATION_FAILED'); + expect((await readDuty(duty.id)).review_note).toBe('The frequency is wrong.'); + }); + + it('keeps the last reason on the record after it goes back for review', async () => { + // Deliberately NOT cleared: the owner is correcting the duty while + // reading it, and a reason that vanishes the moment they act on it is a + // reason they cannot re-read. + const duty = await insertDuty({ source: 'self' }); + await advance(duty.id, 'returned', { review_note: 'Wrong owner — this is the Lab 2 duty.' }, REVIEWER); + await advance(duty.id, 'to_review', {}, OWNER); + const row = await readDuty(duty.id); + expect(row.review_status).toBe('to_review'); + expect(row.review_note).toBe('Wrong owner — this is the Lab 2 duty.'); + }); +}); + +// ─────────────────────────────────────────────────────────────────────────── +// Who may issue a verdict +// ─────────────────────────────────────────────────────────────────────────── + +describe('a review is not something you issue on your own list', () => { + it('refuses the OWNER approving their own duty', async () => { + // The per-option `visibleWhen` doing the job the spec says it must do: + // "Client-side hiding is UX, not authorization … the server MUST also + // reject writes of its value". This is that rejection, server-side, with + // no hook anywhere in the path. + const duty = await insertDuty(); + await advance(duty.id, 'to_review', {}, OWNER); + const { code, message } = await refusal(advance(duty.id, 'approved', {}, OWNER)); + expect(code).toBe('VALIDATION_FAILED'); + // The platform's own `option_unavailable` catalog entry, naming the field + // by its LABEL. Pinned rather than shrugged at, because this string is + // what the reviewer actually reads, and because it comes from the + // catalog the platform already ships translated — an authored rule + // message would not have (`object.validations[].message` has no bundle + // key at all; see `src/translations/authored-text.ts`). + expect(message).toBe("Review status: option 'approved' is not available"); + expect((await readDuty(duty.id)).review_status).toBe('to_review'); + }); + + it('refuses the OWNER returning their own duty', async () => { + const duty = await insertDuty(); + await advance(duty.id, 'to_review', {}, OWNER); + const { code } = await refusal( + advance(duty.id, 'returned', { review_note: 'I disagree with myself.' }, OWNER), + ); + expect(code).toBe('VALIDATION_FAILED'); + }); + + it('admits anybody else — WHO that is, is the permission set\'s question', async () => { + // The option predicate answers "may this value be written by this + // caller", not "may this caller write this record at all". The second + // question is `writeScope`'s, one layer up, and keeping them apart is why + // this predicate names the record relationship rather than a position. + const duty = await insertDuty(); + await advance(duty.id, 'to_review', {}, OWNER); + await advance(duty.id, 'approved', {}, REVIEWER); + expect((await readDuty(duty.id)).review_status).toBe('approved'); + }); + + it('lets the owner move their OWN duty into review — confirming is theirs to do', async () => { + const duty = await insertDuty(); + await advance(duty.id, 'to_review', {}, OWNER); + expect((await readDuty(duty.id)).review_status).toBe('to_review'); + }); + + it('admits a write with no acting user at all, and that is the seed path', async () => { + // Measured and deliberately pinned, because it is the half that surprises + // people: with no user in the context the predicate cannot bind + // `current_user`, the evaluation FAILS rather than returning false, and + // the platform logs `option visibleWhen … failed to evaluate — allowed + // through` and admits the write. That is what lets the demo seed carry + // approved rows and what keeps in-process jobs working — and it is why + // this predicate is a rule about people, not a containment boundary for + // server code. If this ever starts refusing, the seed goes with it. + const duty = await insertDuty(); + await advance(duty.id, 'to_review'); + await advance(duty.id, 'approved'); + expect((await readDuty(duty.id)).review_status).toBe('approved'); + }); + + it('gates the two VERDICTS and nothing else', () => { + // A predicate on `to_confirm` or `to_review` would put the owner's own + // confirmation behind an authorization test, which is not what any of + // this is for. + const gated = (Duty.fields.review_status.options ?? []) + .filter((option: any) => option.visibleWhen !== undefined) + .map((option: any) => String(option.value)) + .sort(); + expect(gated).toEqual(['approved', 'returned']); + }); +}); diff --git a/test/i18n-coverage.test.ts b/test/i18n-coverage.test.ts index 256a053..764387e 100644 --- a/test/i18n-coverage.test.ts +++ b/test/i18n-coverage.test.ts @@ -294,7 +294,13 @@ describe('untranslatable display text is declared rather than dropped', () => { const count = (prefix: string): number => walk.untranslatable.filter((entry) => entry.path.startsWith(prefix)).length; expect(count('view.bulkActionDefs'), 'bulk-action toolbar copy').toBe(35); - expect(count('object.validations'), 'custom validation messages').toBe(11); + // Was 11 before #107 added `review_status_transitions` and + // `returned_needs_note`. Both messages are read by whoever is stopped by + // them — a reviewer taking a step the pipeline does not have, an owner + // returning a duty with no reason — so both enlarge the same declared gap + // (a custom rule message has no bundle key anywhere in the platform's + // schema) rather than opening a new kind of one. + expect(count('object.validations'), 'custom validation messages').toBe(13); // Was 26 before #52 added the three on-time measures — `Done on time`, // `Completed late` and the `On-time rate` derived from them. A measure // label still has no bundle key anywhere in the platform's schema, so each diff --git a/test/reminders.test.ts b/test/reminders.test.ts index 53adb1b..17fca91 100644 --- a/test/reminders.test.ts +++ b/test/reminders.test.ts @@ -335,6 +335,11 @@ describe('reminder sweeps — volume discipline', () => { form: 'recurring', owner: 'u1', status: 'active', + // The control leg has to be a duty that really does dispatch, and + // since #107 that means approved as well as active and recurring — + // otherwise this leg proves the fixture inert for a second reason and + // the standing assertion above stops meaning anything. + review_status: 'approved', frequency: 'monthly', due_anchor: 'period_end', due_offset_days: 0, diff --git a/test/seed.test.ts b/test/seed.test.ts index 92921cd..5033f40 100644 --- a/test/seed.test.ts +++ b/test/seed.test.ts @@ -9,6 +9,7 @@ import { ADMIN, PEOPLE, UNITS } from '../src/data/demo-org.js'; import { CATALOG_ITEMS, DUTIES } from '../src/data/demo-catalog.js'; import { AD_HOC_TASKS, ASSIGNMENTS } from '../src/data/demo-assignments.js'; import { SEEDED_TASKS, SKIPS, TODAY } from '../src/data/demo-history.js'; +import { planDispatch } from '../src/jobs/dispatch.plan.js'; /** * The demo seed, asserted against a REAL BOOTED KERNEL with the declarative @@ -360,6 +361,86 @@ describe('view populations — what an evaluator actually opens', () => { }); }); +// ─────────────────────────────────────────────────────────────────────────── +describe('the review pipeline has something to show (#107)', () => { + it('seeds a mix, not thirty approved rows', async () => { + // A fixture where every duty is approved makes three screens untestable + // and two of them unshowable: "To confirm" and "To review" are empty, and + // "an unapproved duty dispatches nothing" is a claim with no instance + // behind it. + const duties = await all('duly_duty'); + const byState = new Map(); + for (const duty of duties) { + const state = String(duty.review_status); + byState.set(state, (byState.get(state) ?? 0) + 1); + } + expect(byState.get('to_confirm') ?? 0, 'duties waiting on their owner').toBeGreaterThanOrEqual(2); + expect(byState.get('to_review') ?? 0, 'duties waiting on a reviewer').toBeGreaterThanOrEqual(1); + expect(byState.get('returned') ?? 0, 'returned duties').toBe(1); + // …and the overwhelming majority still work, or the demo opens on an app + // that dispatches nothing. + expect(byState.get('approved') ?? 0).toBeGreaterThan(duties.length / 2); + // Every row is in one of the four declared states — a blank would not + // dispatch and would render as an empty pipeline stage. + const declared = new Set(['to_confirm', 'to_review', 'approved', 'returned']); + expect([...byState.keys()].filter((state) => !declared.has(state))).toEqual([]); + }); + + it('the returned duty carries the reason the rule requires', async () => { + const returned = (await all('duly_duty')).filter((duty) => duty.review_status === 'returned'); + expect(returned).toHaveLength(1); + // `returned_needs_note` is an ordinary script rule and is NOT skipped on + // the seed path (only `state_machine` is), so a returned row without a + // note would have been refused outright and this row would be missing — + // which is worth stating, because "the row is here" is the evidence. + expect(String(returned[0]?.review_note ?? '').length).toBeGreaterThan(20); + }); + + it('leaves the evaluator their own duty in each state they can advance alone', async () => { + // The demo walk: the record page's pipeline is clickable on the first + // login, without a second account. `to_confirm → to_review` and + // `returned → to_review` are the owner's own steps; the two verdicts are + // deliberately NOT self-issuable (see `duty.object.ts`), so those are + // walked on somebody else's row from "To review". + const me = await userId(ADMIN); + const mine = (await all('duly_duty')).filter((duty) => String(duty.owner) === me); + const states = new Set(mine.map((duty) => String(duty.review_status))); + expect(states.has('to_confirm'), 'no unconfirmed duty on the demo account').toBe(true); + expect(states.has('returned'), 'no returned duty on the demo account').toBe(true); + }); + + it('an unapproved duty stops the NEXT dispatch and keeps the history it has', async () => { + // The fixture's own statement of the product rule. History is planned as + // `approved` (see `demo-history.ts`), so these rows DO carry tasks — and + // the planner would refuse them today, which is what makes "returning a + // duty stops tomorrow's work, not yesterday's" visible in the demo rather + // than only in a unit test. + const unapproved = (await all('duly_duty')).filter((duty) => duty.review_status !== 'approved'); + expect(unapproved.length).toBeGreaterThanOrEqual(4); + const plan = planDispatch({ + duties: unapproved.map((duty) => ({ + id: String(duty.id), + name: String(duty.name), + form: String(duty.form), + status: String(duty.status), + owner: String(duty.owner), + source: String(duty.source), + review_status: String(duty.review_status), + frequency: duty.frequency as string | null, + due_anchor: duty.due_anchor as string | null, + due_offset_days: duty.due_offset_days as number | null, + lead_days: duty.lead_days as number | null, + timezone: String(duty.timezone), + })), + now: new Date(), + }); + expect(plan.drafts, 'an unapproved duty drafted a task').toEqual([]); + // And for the right reason on every one of them — `not_approved`, not + // `standing` or `not_active` happening to cover for it. + expect(new Set(plan.skipped.map((skip) => skip.reason))).toEqual(new Set(['not_approved'])); + }); +}); + // ─────────────────────────────────────────────────────────────────────────── describe('the standing invariant', () => { it('no task exists whose duty is standing', async () => { diff --git a/test/task-hook.test.ts b/test/task-hook.test.ts index f5c6f8f..ac212a9 100644 --- a/test/task-hook.test.ts +++ b/test/task-hook.test.ts @@ -731,6 +731,12 @@ describe('WRITE-ONCE — editing a duty\'s grace never rewrites history', () => owner: 'user_alice', business_unit: null, source: 'catalog', + // Only an approved duty dispatches (#107). This fixture is asking + // "what deadline would the dispatcher have stamped", so the duty it + // asks about has to be one the dispatcher would actually have picked + // up — otherwise there is no draft and the assertion below reads as a + // `late_after` bug rather than an unapproved duty. + review_status: 'approved', frequency: 'monthly', due_anchor: 'period_start', due_offset_days: 4, From a883f7b2616b01dc196ba966f679622e980bdfdc Mon Sep 17 00:00:00 2001 From: Warren Date: Wed, 2 Sep 2026 05:09:00 +0000 Subject: [PATCH 2/4] Point the record-page stepper at the review pipeline (#107) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Measured in the running console: with `stageField` absent the record-detail stepper falls back to a heuristic that takes the first field named `status` / `stage` / `state` / `phase` — so `duly_duty`'s stepper was Active · Paused · Retired, which is not a progression. ADR-0085 calls that an unordered state set and says the stage heuristics should be suppressed for it. `review_status` is the linear one, so it is what the key names. The pipeline now renders To confirm · To review · Approved · Returned on the record page, with completed stages ticked. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01SqkTcrxUFci7nqXdbBSe2p --- src/objects/duty.object.ts | 24 ++++++++++++++++++++++++ src/translations/authored-text.ts | 1 + 2 files changed, 25 insertions(+) diff --git a/src/objects/duty.object.ts b/src/objects/duty.object.ts index e482274..7fdfc25 100644 --- a/src/objects/duty.object.ts +++ b/src/objects/duty.object.ts @@ -403,6 +403,30 @@ export const Duty = ObjectSchema.create({ ], nameField: 'name', + + /** + * ── The record page's clickable pipeline renders THIS field ───────────── + * [ADR-0085] `stageField` names the record's LINEAR lifecycle, and the + * record-detail stepper is its consumer. Measured on the running console + * (`@objectstack/console` 17.2.0): with the key absent the renderer falls + * back to a heuristic that takes the first field named `status` / `stage` / + * `state` / `phase` — so this object's stepper was `status` + * (Active · Paused · Retired), which is not a progression at all. ADR-0085 + * calls that exact shape "an unordered state set" and says a consumer's + * stage heuristics should be suppressed for it. + * + * `review_status` IS linear — to confirm → to review → approved — so it is + * the honest answer to the question the key asks, and naming it is what + * makes the confirmation workflow clickable on the record page. That + * matters more than it sounds: an app-declared action renders NOWHERE in + * the Console today (objectui#7234), so this stepper is the only surface + * this workflow has. + * + * `status` loses its stepper and stays an ordinary select on the form, + * which is what it always should have been. + */ + stageField: 'review_status', + highlightFields: ['name', 'form', 'frequency', 'owner', 'status', 'review_status'], validations: [ diff --git a/src/translations/authored-text.ts b/src/translations/authored-text.ts index 852c4dd..c12221f 100644 --- a/src/translations/authored-text.ts +++ b/src/translations/authored-text.ts @@ -223,6 +223,7 @@ const VERDICTS: Readonly> = { 'object.sharingModel': machine('security posture'), 'object.datasource': machine('datasource name'), 'object.nameField': machine('field name'), + 'object.stageField': machine('field name — the record\'s lifecycle field (ADR-0085)'), 'object.highlightFields[]': machine('field names'), 'object.fields{}.label': translate((c) => id(c) ? ['objects', id(c)!, 'fields', c.path[1]!, 'label'] : undefined), From ea0509a4f21d2beb1763f1850ac6d42e4b60ad67 Mon Sep 17 00:00:00 2001 From: Warren Date: Wed, 2 Sep 2026 05:18:35 +0000 Subject: [PATCH 3/4] Keep the import sample gate about columns whose absence refuses a row (#107) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `review_status` is required, writable, and defaulted from `source` — the first field to be all three — so `test/import-samples.test.ts` demanded a column for it in `samples/duties.csv`. An imported catalog row lands `to_confirm` without one, which is exactly what the product wants an imported list to be, and the walkthrough already documents blank-cell-defers-to-default as the mechanism. The exclusion is narrow on purpose: only a RECORD-derived default (a CEL expression over other columns of the same row) drops out. A caller-derived default stays in the required set — `duly_duty.owner` defaults to `current_user`, so an omitted column silently gives every imported duty to whoever ran the import, which is the worse half of the failure this gate is for. Reverse-verified: deleting the `owner` column still fails the gate, naming `owner`. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01SqkTcrxUFci7nqXdbBSe2p --- test/import-samples.test.ts | 40 +++++++++++++++++++++++++++++++++---- 1 file changed, 36 insertions(+), 4 deletions(-) diff --git a/test/import-samples.test.ts b/test/import-samples.test.ts index f3f212e..3b8d5d5 100644 --- a/test/import-samples.test.ts +++ b/test/import-samples.test.ts @@ -94,7 +94,7 @@ function fieldEntries(schema: unknown): Array<[string, { readonly?: boolean; req if (!fields || typeof fields !== 'object') throw new Error('schema declares no fields'); return Object.entries(fields).map(([key, def]) => [ typeof def?.name === 'string' ? (def.name as string) : key, - def as { readonly?: boolean; required?: boolean }, + def as { readonly?: boolean; required?: boolean; defaultValue?: unknown }, ]); } @@ -104,11 +104,43 @@ const declaredFields = (schema: unknown): string[] => fieldEntries(schema).map(( const writableFields = (schema: unknown): string[] => fieldEntries(schema).filter(([, def]) => def?.readonly !== true).map(([name]) => name); -/** Fields a row cannot omit AND a caller may set. Both halves matter: a - * `required` field that is also `readonly` is the platform's to write. */ +/** A default the ROW supplies for itself — a CEL expression over `record.*`, + * as opposed to a scalar or a caller token like `'current_user'`. + * + * The distinction decides whether an omitted column is a defect (#107). Both + * kinds stop the import refusing the row, so neither is caught by "required"; + * what separates them is what lands instead: + * + * - **Record-derived** (`F`record.source == "self" ? … : …``): the value is + * computed from other columns of the SAME row, so an omitted column yields + * exactly what the sample would have had to type. Nothing is substituted + * and nothing is lost. + * - **Caller-derived** (`'current_user'` on `duly_duty.owner`): an omitted + * column yields the IMPORTER, silently, on every row — nineteen duties + * owned by whoever ran the import. That is the worse half of the failure + * this test exists for, so it stays in the required set even though it too + * would not be refused. + */ +const isRecordDerivedDefault = (value: unknown): boolean => + typeof value === 'object' && value !== null && 'dialect' in (value as Record); + +/** Fields a row cannot omit AND a caller may set AND the row cannot supply for + * itself. Each exclusion drops a field for its own reason: + * + * - `readonly` — the platform's to write, never the CSV's. + * - a record-derived default — see above. `duly_duty.review_status` is the + * first field to be required, writable and defaulted this way (#107): + * `record.source == "self" ? "to_review" : "to_confirm"`, so an imported + * catalog row lands `to_confirm`, which is precisely what the product + * wants an imported list to be. A column for it would restate the default + * the walkthrough already documents ("A blank cell means leave this field + * unset … the object's `defaultValue` then decides") and would invite an + * author to type `approved` there, which `initialStates` refuses row by + * row. */ const requiredWritableFields = (schema: unknown): string[] => fieldEntries(schema) - .filter(([, def]) => def?.required === true && def?.readonly !== true) + .filter(([, def]) => + def?.required === true && def?.readonly !== true && !isRecordDerivedDefault(def?.defaultValue)) .map(([name]) => name); describe('samples/ — the duty list an evaluator imports (#19)', () => { From ddcd075b51c40f326f622e8b82732a2da096d415 Mon Sep 17 00:00:00 2001 From: Warren Date: Wed, 2 Sep 2026 05:20:21 +0000 Subject: [PATCH 4/4] Widen the helper's return type to match its cast (#107) Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01SqkTcrxUFci7nqXdbBSe2p --- test/import-samples.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/import-samples.test.ts b/test/import-samples.test.ts index 3b8d5d5..9bf2581 100644 --- a/test/import-samples.test.ts +++ b/test/import-samples.test.ts @@ -89,7 +89,7 @@ function csvHeader(file: string): string[] { } /** `[fieldName, definition]` for every field the schema declares. */ -function fieldEntries(schema: unknown): Array<[string, { readonly?: boolean; required?: boolean }]> { +function fieldEntries(schema: unknown): Array<[string, { readonly?: boolean; required?: boolean; defaultValue?: unknown }]> { const fields = (schema as { fields?: Record> }).fields; if (!fields || typeof fields !== 'object') throw new Error('schema declares no fields'); return Object.entries(fields).map(([key, def]) => [