From 83c8cae11a0b9cdf428cd8ff4f618838db79494e Mon Sep 17 00:00:00 2001 From: os-dev Date: Wed, 5 Aug 2026 22:02:42 +0000 Subject: [PATCH] fix(lint): report a config.timeRelative descriptor the sweep will refuse (#5496) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A flow start node declaring `config.timeRelative` got zero authoring-time diagnostics when the descriptor could not parse. The two rules that look at the slot each looked at something else: `lint-flow-patterns` decides "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. `TimeRelativeTriggerSchema` does reject a bad descriptor, but the only place it 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 — outside an AI author's feedback loop entirely. New rule `flow-time-relative-descriptor-invalid` (warning) runs that same schema at authoring time and forwards its issue list verbatim, rendered exactly as the bind-time warning renders it. No shape knowledge is re-implemented and no consumer-side tolerance is added: the verdict and its wording stay the schema's, so the rule tracks the descriptor's contract instead of drifting from a second copy of it. The new 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, only the schema knows the shape. Its guard mirrors the engine's routing predicate character for character, so the rule speaks for exactly the flows the engine hands to the time-relative trigger. Verified: every time-relative descriptor shipped in the repo parses, and `os validate` output on all three example apps is identical before and after. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01GX3sL71LFq8m2usg6VqTSE --- .../flow-time-relative-descriptor-lint.md | 47 ++++ packages/lint/src/index.ts | 1 + .../validate-flow-trigger-readiness.test.ts | 203 ++++++++++++++++++ .../src/validate-flow-trigger-readiness.ts | 100 ++++++++- 4 files changed, 345 insertions(+), 6 deletions(-) create mode 100644 .changeset/flow-time-relative-descriptor-lint.md 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 —