diff --git a/.changeset/flow-time-relative-descriptor-lint.md b/.changeset/flow-time-relative-descriptor-lint.md new file mode 100644 index 0000000000..1b64b5e7e7 --- /dev/null +++ b/.changeset/flow-time-relative-descriptor-lint.md @@ -0,0 +1,47 @@ +--- +"@objectstack/lint": patch +--- + +fix(lint): report a `config.timeRelative` descriptor the sweep will refuse, at authoring time (#5496) + +A flow start node declaring `config.timeRelative` got **zero** authoring-time +diagnostics when its descriptor could not parse. The two rules that look at the +slot each looked at something else: `lint-flow-patterns` decides "this is a +time-relative flow" from `timeRelative != null` alone (never the shape), and +`validate-flow-trigger-readiness`'s existing check reads only +`timeRelative.object`, to compare it against the stack's objects. So + +```ts +config: { timeRelative: { object: 'task', field: 'due_at', offsetDays: -1 } } +``` + +— three separate schema violations: `dateField` missing, `offsetDays` declared +as an int **array** and written as a scalar, and `field` an unrecognized key — +passed `os validate` silently. `TimeRelativeTriggerSchema` does reject it, but +the only place that schema ran was **bind time**, inside +`TimeRelativeTriggerPlugin.start()`, which warns and returns: the sweep is never +installed, the flow reports itself armed, and the author's sole feedback is one +line in a server log. For an AI author that line is outside the feedback loop +entirely; `os validate` is what it reads. + +**New rule — `flow-time-relative-descriptor-invalid` (warning).** A start node +whose `config.timeRelative` is present runs that same schema at authoring time, +and a failure is reported naming `config.timeRelative` with the schema's own +issue list forwarded — so the diagnostic carries the missing key, the wrong type, +and, for an unrecognized key, the "did you mean" the schema already computes +(`field` → `dateField`) plus its wrong-layer guidance (a `schedule` written +*inside* the descriptor is told it belongs beside it). The list is rendered +exactly as the bind-time warning renders it, so the two channels tell one story. + +Nothing is shifted except **when** the schema runs. No shape knowledge is +re-implemented in the rule and no consumer-side tolerance is added: the verdict +and every word of its wording remain `TimeRelativeTriggerSchema`'s, so the rule +tracks the descriptor's contract as it evolves instead of drifting from a second +copy of it. + +The rule and the existing object-name check decide different facts and cannot +report the same one twice — only the stack knows whether an object name exists, +and only the schema knows the descriptor's shape. A descriptor wrong in both ways +gets both findings, at their own paths. Canonical descriptors are unaffected: +every one shipped in the repo (the showcase `Task Due Reminder`, the +`content/docs` examples) parses, so this adds no diagnostic to existing apps. diff --git a/packages/lint/src/index.ts b/packages/lint/src/index.ts index f00a28077b..1cb0e2230b 100644 --- a/packages/lint/src/index.ts +++ b/packages/lint/src/index.ts @@ -53,6 +53,7 @@ export { validateFlowTriggerReadiness, FLOW_TRIGGER_UNKNOWN_OBJECT, FLOW_DRAFT_STATUS_AMBIGUOUS, + FLOW_TIME_RELATIVE_DESCRIPTOR_INVALID, } from './validate-flow-trigger-readiness.js'; export type { FlowTriggerReadinessFinding, diff --git a/packages/lint/src/validate-flow-trigger-readiness.test.ts b/packages/lint/src/validate-flow-trigger-readiness.test.ts index a9e5a9e829..2e67b69236 100644 --- a/packages/lint/src/validate-flow-trigger-readiness.test.ts +++ b/packages/lint/src/validate-flow-trigger-readiness.test.ts @@ -1,11 +1,13 @@ // Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. import { describe, it, expect } from 'vitest'; +import { TimeRelativeTriggerSchema } from '@objectstack/spec/automation'; import { validateFlowTriggerReadiness, FLOW_TRIGGER_UNKNOWN_OBJECT, FLOW_DRAFT_STATUS_AMBIGUOUS, FLOW_TRIGGER_UNKNOWN_EVENT, + FLOW_TIME_RELATIVE_DESCRIPTOR_INVALID, } from './validate-flow-trigger-readiness.js'; function recordFlow(overrides: Record = {}) { @@ -174,6 +176,207 @@ describe('validateFlowTriggerReadiness', () => { expect(findings[0].rule).toBe(FLOW_TRIGGER_UNKNOWN_OBJECT); expect(findings[0].message).toContain("'contract'"); expect(findings[0].path).toBe('flows[0].nodes[0].config.timeRelative.object'); + // The SHAPE is canonical, so the descriptor rule stays out of it — the two + // halves of 1b decide different facts and must not both fire on one. + expect(TimeRelativeTriggerSchema.safeParse({ + object: 'contract', dateField: 'end_date', withinDays: 60, + }).success).toBe(true); + expect(findings.some((f) => f.rule === FLOW_TIME_RELATIVE_DESCRIPTOR_INVALID)).toBe(false); + }); + + // ── #5496 — the descriptor's SHAPE ──────────────────────────────────────── + // + // `TimeRelativeTriggerSchema` is the only thing that can judge a + // `config.timeRelative` descriptor (the node `config` slot is open by design, + // ADR-0018, so no outer flow gate sees inside it), and until this rule the only + // place it ran was BIND time — one warn in a server log, nothing in + // `os validate`. These tests pin the forwarding, not a second copy of the + // shape: where a message is asserted it is asserted against what the schema + // itself produces, so the rule cannot drift from the contract it speaks for. + describe('config.timeRelative descriptor shape (#5496)', () => { + /** The stack from the issue: `task` EXISTS, flow is active and runs as system. */ + function timeRelativeStack(timeRelative: unknown, objectName = 'task') { + return { + objects: [{ name: objectName, label: 'Task', fields: {} }], + flows: [ + { + name: 'task_due_reminder', + type: 'schedule', + status: 'active', + runAs: 'system', + nodes: [ + { id: 'start', type: 'start', config: { timeRelative } }, + { id: 'end', type: 'end' }, + ], + edges: [{ id: 'e1', source: 'start', target: 'end' }], + }, + ], + }; + } + + /** The exact descriptor #5496 was filed for — three separate zod issues. */ + const badDescriptor = { object: 'task', field: 'due_at', offsetDays: -1 }; + + it('flags the descriptor from #5496 and names every key zod named', () => { + const findings = validateFlowTriggerReadiness(timeRelativeStack(badDescriptor)); + expect(findings).toHaveLength(1); + const [f] = findings; + expect(f.rule).toBe(FLOW_TIME_RELATIVE_DESCRIPTOR_INVALID); + expect(f.severity).toBe('warning'); + // Criterion 1: the finding NAMES config.timeRelative, in both channels the + // CLI prints (`• where: message` then `at path`). + expect(f.path).toBe('flows[0].nodes[0].config.timeRelative'); + expect(f.message).toContain('config.timeRelative'); + expect(f.where).toBe('flow "task_due_reminder" › start node'); + // …and carries zod's own key names: the missing `dateField`, the scalar + // `offsetDays`, and the unrecognized `field` with the schema's suggestion. + expect(f.message).toContain('dateField: Invalid input: expected string, received undefined'); + expect(f.message).toContain('offsetDays: Invalid input: expected array, received number'); + expect(f.message).toContain('Unrecognized key(s)'); + expect(f.message).toContain('`field`'); + expect(f.message).toContain('Did you mean `field` → `dateField`?'); + // The consequence, which is the whole reason this is not just a log line. + expect(f.message).toMatch(/never runs/); + expect(f.hint).toContain('TimeRelativeTriggerSchema'); + }); + + it('forwards the schema verbatim rather than restating it (anti-drift pin)', () => { + // Every problem segment is `TimeRelativeTriggerSchema`'s own text, rendered + // the way `TimeRelativeTrigger.start()` renders the identical issue list at + // bind time. Derived from the schema HERE too, so this assertion tracks the + // contract instead of freezing today's wording: if the schema's message for + // a rejected descriptor changes, the rule's output changes with it and this + // test keeps passing — but a hand-written copy in the rule would not. + const parsed = TimeRelativeTriggerSchema.safeParse(badDescriptor); + expect(parsed.success).toBe(false); + const expected = parsed.error!.issues + .map((i) => `${i.path.join('.') || '(root)'}: ${i.message.replace(/\s+/g, ' ').trim()}`) + .join('; '); + const [f] = validateFlowTriggerReadiness(timeRelativeStack(badDescriptor)); + expect(f.message).toContain(expected); + // Single-line, so the CLI's bulleted list stays aligned (the schema's + // guidance bullets arrive with newlines in them). + expect(f.message).not.toContain('\n'); + expect(f.hint).not.toContain('\n'); + }); + + it('stays silent on the canonical descriptors — including the ones shipped in the repo', () => { + // Criterion 2. Each is pinned against the schema as well as against the + // rule, so a fixture cannot rot into an unbindable descriptor and keep this + // test green for the wrong reason (#4966's lesson, one layer down). + const canonical: Array<[string, Record]> = [ + ['#5496 acceptance shape', { object: 'task', dateField: 'due_at', offsetDays: [-1] }], + // examples/app-showcase `Task Due Reminder` (#1874) — the showcase flow + // criterion 2 names by hand. + ['showcase Task Due Reminder', { + object: 'task', + dateField: 'due_date', + offsetDays: [3, 1], + filter: { status: { $ne: 'done' } }, + }], + // content/docs/references/automation/time-relative-trigger.mdx, all three + // examples, and content/docs/automation/flows.mdx's `renewalReminder`. + ['docs T-minus example', { + object: 'task', dateField: 'end_date', offsetDays: [60, 30, 7], filter: { status: 'active' }, + }], + ['docs expiring-soon example', { object: 'task', dateField: 'expires_on', withinDays: 30 }], + ['docs overdue example', { + object: 'task', dateField: 'due_date', withinDays: -14, filter: { status: 'open' }, + }], + ['with maxRecords', { object: 'task', dateField: 'due_at', withinDays: 7, maxRecords: 50 }], + ]; + for (const [label, descriptor] of canonical) { + expect(TimeRelativeTriggerSchema.safeParse(descriptor).success, `${label} must be spec-valid`).toBe(true); + expect(validateFlowTriggerReadiness(timeRelativeStack(descriptor)), label).toEqual([]); + } + }); + + it('reports a wrong object name and a wrong shape as two facts, not one twice', () => { + // Criterion 3. `contract` is not in the stack AND the descriptor does not + // parse. The two findings are distinguishable by rule id and by path, and + // neither restates the other's fact: the unknown-object warning says nothing + // about the shape, and the schema — which has no stack knowledge — cannot + // say anything about the name. + const findings = validateFlowTriggerReadiness( + timeRelativeStack({ object: 'contract', field: 'end_date', withinDays: 60 }), + ); + expect(findings).toHaveLength(2); + expect(findings.map((f) => f.rule)).toEqual([ + FLOW_TRIGGER_UNKNOWN_OBJECT, + FLOW_TIME_RELATIVE_DESCRIPTOR_INVALID, + ]); + expect(findings.map((f) => f.path)).toEqual([ + 'flows[0].nodes[0].config.timeRelative.object', + 'flows[0].nodes[0].config.timeRelative', + ]); + // The name warning talks only about the name… + expect(findings[0].message).toContain("'contract'"); + expect(findings[0].message).not.toContain('dateField'); + // …and the shape warning only about the shape (it never echoes the name). + expect(findings[1].message).toContain('dateField'); + expect(findings[1].message).not.toContain("'contract'"); + }); + + it('forwards the exactly-one-window rule (both modes, and neither)', () => { + for (const descriptor of [ + { object: 'task', dateField: 'due_at', withinDays: 3, offsetDays: [1] }, + { object: 'task', dateField: 'due_at' }, + ]) { + const findings = validateFlowTriggerReadiness(timeRelativeStack(descriptor)); + expect(findings.map((f) => f.rule)).toEqual([FLOW_TIME_RELATIVE_DESCRIPTOR_INVALID]); + expect(findings[0].message).toContain('exactly one of `withinDays`'); + } + }); + + it("forwards the schema's wrong-layer guidance for a `schedule` written INSIDE the descriptor", () => { + // The cadence knob is a SIBLING of `timeRelative` on the same config. The + // schema carries that prescription; the value of forwarding is that the + // author reads it from `os validate` instead of from a server log. + const findings = validateFlowTriggerReadiness( + timeRelativeStack({ + object: 'task', + dateField: 'due_at', + withinDays: 3, + schedule: { type: 'cron', expression: '0 8 * * *' }, + }), + ); + expect(findings.map((f) => f.rule)).toEqual([FLOW_TIME_RELATIVE_DESCRIPTOR_INVALID]); + expect(findings[0].message).toContain('`schedule` is a sibling of `timeRelative`'); + }); + + it('flags an array descriptor — the engine routes it, so the trigger refuses it', () => { + const findings = validateFlowTriggerReadiness(timeRelativeStack([{ object: 'task' }])); + expect(findings.map((f) => f.rule)).toEqual([FLOW_TIME_RELATIVE_DESCRIPTOR_INVALID]); + expect(findings[0].message).toContain('expected object, received array'); + }); + + it('says nothing about a non-object timeRelative — the engine does not route it here', () => { + // `AutomationEngine`'s trigger resolution requires `typeof … === 'object'`, + // so `timeRelative: 'daily'` never reaches the time-relative trigger and no + // descriptor verdict applies to it. Whatever that flow's defect is, it is + // not this rule's, and guessing here would make the rule speak for flows + // the engine hands somewhere else. + const findings = validateFlowTriggerReadiness(timeRelativeStack('daily')); + expect(findings.some((f) => f.rule === FLOW_TIME_RELATIVE_DESCRIPTOR_INVALID)).toBe(false); + }); + + it('is inert on flows that declare no timeRelative at all', () => { + const findings = validateFlowTriggerReadiness({ + objects: [candidateObject], + flows: [recordFlow({ status: 'active' })], + }); + expect(findings).toEqual([]); + }); + + it('still flags the draft-status ambiguity alongside a bad descriptor', () => { + const stack = timeRelativeStack(badDescriptor); + delete (stack.flows[0] as Record).status; + const findings = validateFlowTriggerReadiness(stack); + expect(findings.map((f) => f.rule)).toEqual([ + FLOW_TIME_RELATIVE_DESCRIPTOR_INVALID, + FLOW_DRAFT_STATUS_AMBIGUOUS, + ]); + }); }); it('passes the record-after-write (create-OR-update) token (#3427)', () => { diff --git a/packages/lint/src/validate-flow-trigger-readiness.ts b/packages/lint/src/validate-flow-trigger-readiness.ts index ea68340419..ef371ee90e 100644 --- a/packages/lint/src/validate-flow-trigger-readiness.ts +++ b/packages/lint/src/validate-flow-trigger-readiness.ts @@ -4,8 +4,8 @@ // third-party eval: a record-change flow that silently never fires). // // A pure `(stack) => Finding[]` rule (ADR-0019), run from `os validate` and -// reusable by AI authoring. It catches the two authoring mistakes that produce -// a flow which LOOKS armed but never launches — with zero runtime output: +// reusable by AI authoring. It catches the authoring mistakes that produce a +// flow which LOOKS armed but never launches — with zero runtime output: // // 1. `objectName` mismatch — the start node targets an object name that is // not defined in this stack. The runtime binds an ObjectQL hook filtered @@ -23,6 +23,20 @@ // deliberately or `'obsolete'` to disable. Only auto-triggered flows are // flagged (manual/screen flows have no arming semantics to be unclear // about). +// +// 3. A `config.timeRelative` descriptor that does not PARSE — the shape half +// of the time-relative sweep (#5496). Only `TimeRelativeTriggerSchema` can +// judge it, and until this rule the only place it ran was BIND time, so an +// unparseable descriptor produced one warn in a server log and nothing at +// all in `os validate`. The judgement is not re-implemented here: the rule +// runs that schema and forwards its issue list verbatim. +// +// The spec import is deliberate and is what makes rule 3 possible without a +// second copy of the descriptor's shape living in this file. It stays inside the +// package's stated dependency direction — lint → `@objectstack/spec`, never onto +// a runtime. + +import { TimeRelativeTriggerSchema } from '@objectstack/spec/automation'; export type FlowTriggerReadinessSeverity = 'error' | 'warning'; @@ -41,6 +55,16 @@ export interface FlowTriggerReadinessFinding { export const FLOW_TRIGGER_UNKNOWN_OBJECT = 'flow-trigger-unknown-object'; export const FLOW_DRAFT_STATUS_AMBIGUOUS = 'flow-draft-status-ambiguous'; export const FLOW_TRIGGER_UNKNOWN_EVENT = 'flow-trigger-unknown-event'; +/** + * #5496 — `config.timeRelative` is present but `TimeRelativeTriggerSchema` + * rejects it, so the sweep is never installed. + * + * Named for the DESCRIPTOR rather than for the rule that reads it, because this + * is the first of a family: every flow-node `config` slot whose contract a + * schema (or the engine) can already decide, yet which nothing checks at + * authoring time. `flow--` is the shape the next one takes. + */ +export const FLOW_TIME_RELATIVE_DESCRIPTOR_INVALID = 'flow-time-relative-descriptor-invalid'; type AnyRec = Record; @@ -76,7 +100,11 @@ function startNodeOf(flow: AnyRec): { node: AnyRec; index: number } | undefined /** * Validate auto-launched flow trigger wiring against the stack definition. - * Pure and dependency-free; safe on pre- or post-parse stacks. + * + * Pure — no I/O, no runtime, no mutation of `stack` — and safe on pre- or + * post-parse stacks. Its one dependency is the `@objectstack/spec` schema that + * owns the `timeRelative` descriptor's contract, which is the point: the rule + * ASKS that schema rather than restating it. */ export function validateFlowTriggerReadiness(stack: AnyRec): FlowTriggerReadinessFinding[] { const findings: FlowTriggerReadinessFinding[] = []; @@ -128,11 +156,32 @@ export function validateFlowTriggerReadiness(stack: AnyRec): FlowTriggerReadines } } - // 1b. Time-relative flow sweeping an object this stack does not define. Like - // the record-change case, a wrong object name makes the sweep match - // nothing forever with no runtime output. + // 1b. Two facts about the same `config.timeRelative` descriptor, from the two + // places that can decide them. The split is what keeps them from + // reporting the same thing twice: + // + // - the NAME in `object` is checked against this stack (1b-i). Only the + // stack knows it; `TimeRelativeTriggerSchema` has no stack knowledge + // and can never raise it. + // - the SHAPE of everything else is checked by that schema (1b-ii). + // Only it knows the descriptor's contract; this rule reads no other + // key of `tr`. + // + // So a descriptor that is wrong in both ways reports both, at two + // different paths (`…timeRelative.object` and `…timeRelative`) — two + // facts, not one fact twice. + // + // The `isTimeRelative` guard is deliberately the ENGINE's routing + // predicate, character for character (`AutomationEngine`'s trigger + // resolution: `config.timeRelative != null && typeof … === 'object'`). + // This rule therefore speaks for exactly the flows the engine hands to + // the time-relative trigger, and stays silent about the ones it does not. if (isTimeRelative && start) { const tr = config.timeRelative as AnyRec; + + // 1b-i. Sweeping an object this stack does not define. Like the + // record-change case, a wrong object name makes the sweep match + // nothing forever with no runtime output. const objectName = typeof tr.object === 'string' ? tr.object : undefined; if (objectName && !objectNames.has(objectName) && !objectName.startsWith('sys_')) { findings.push({ @@ -148,6 +197,45 @@ export function validateFlowTriggerReadiness(stack: AnyRec): FlowTriggerReadines `If the object comes from another installed package, this warning can be ignored.`, }); } + + // 1b-ii. #5496 — the descriptor does not parse, so `TimeRelativeTrigger` + // refuses it at bind time and the sweep is never installed. The flow + // declares a time-relative trigger, passes every gate, and never runs. + // + // Before this rule the author's ONLY feedback was one warn in the + // server log at bind time — a channel an AI author's loop never reads, + // unlike `os validate`. Nothing is shifted except WHEN the schema runs: + // the verdict, and every word of its wording, is still + // `TimeRelativeTriggerSchema`'s. Re-deriving any of it here would put a + // second copy of the descriptor's contract in a consumer, which is the + // drift this forwards precisely to avoid — `field` is rejected here + // because the SCHEMA rejects it, and it will keep tracking the schema + // when the descriptor gains a key. + const parsed = TimeRelativeTriggerSchema.safeParse(tr); + if (!parsed.success) { + // Rendered exactly as the bind-time warn renders the same issue list + // (`TimeRelativeTrigger.start`), so an author who sees both channels sees + // one story told twice, not two dialects. Whitespace is collapsed because + // a finding is one line here (the CLI prints `• where: message`) while a + // log line is free to wrap — the schema's guidance bullets carry newlines. + const problems = parsed.error.issues + .map((i) => `${i.path.join('.') || '(root)'}: ${i.message.replace(/\s+/g, ' ').trim()}`) + .join('; '); + findings.push({ + severity: 'warning', + rule: FLOW_TIME_RELATIVE_DESCRIPTOR_INVALID, + where: `flow "${flowName}" › start node`, + path: `flows[${flowIndex}].nodes[${start.index}].config.timeRelative`, + message: + `has a config.timeRelative descriptor the time-relative trigger REFUSES at bind time, so the ` + + `sweep is never installed — the flow declares a time-relative trigger and then never runs ` + + `(the only trace is one warn in the server log). ${problems}`, + hint: + `Those messages are TimeRelativeTriggerSchema's own — the same schema the trigger safeParses at ` + + `bind time, so a descriptor that satisfies them binds. An unrecognized key names the declared key ` + + `it was probably meant to be; see content/docs/references/automation/time-relative-trigger.mdx.`, + }); + } } // 1c. A `record-`-prefixed triggerType the trigger cannot map to any hook —