diff --git a/.changeset/flow-runas-unscoped-region-descent.md b/.changeset/flow-runas-unscoped-region-descent.md index d9d048d97b..a1d5d73c70 100644 --- a/.changeset/flow-runas-unscoped-region-descent.md +++ b/.changeset/flow-runas-unscoped-region-descent.md @@ -36,16 +36,20 @@ is a flow property and the region only supplies the evidence. The region is name in the **message** so you can find the node: ``` -flow 'nightly_sweep' · runAs: schedule-triggered flow runs as `runAs:'user'`, but a -schedule run has no trigger user — so its data node 'touch' (update_record), in loop -'loop_rows' body, has no identity to scope to and will be REFUSED at run time. +flow 'nightly_sweep' · runAs: schedule-triggered flow runs under `runAs:'user'` +(the default when none is declared), but a schedule run has no trigger user — so its +data node 'touch' (update_record), in loop 'loop_rows' body, has no identity to scope +to and will be REFUSED at run time. ``` +(The sentence's opening was re-worded by #5693 in this same release window; the +sample above is the wording that actually ships.) + **Nothing about the top-level case moved.** A flow whose evidence is a top-level -data node produces the byte-identical message it always did (no region clause), -and when a flow has data nodes at both altitudes the top-level one is still the -node cited — `collectFlowGraphs` yields the flow's own graph before it descends. -Both are pinned by tests. +data node produces the same message with no region clause, and when a flow has +data nodes at both altitudes the top-level one is still the node cited — +`collectFlowGraphs` yields the flow's own graph before it descends. Both are +pinned by tests. **If this newly fails your build:** the flow was already broken at run time. Add `runAs: 'system'` to declare the elevation the sweep needs (a schedule / diff --git a/.changeset/flow-runas-unscoped-two-way-true-wording.md b/.changeset/flow-runas-unscoped-two-way-true-wording.md new file mode 100644 index 0000000000..c0c1c89037 --- /dev/null +++ b/.changeset/flow-runas-unscoped-two-way-true-wording.md @@ -0,0 +1,64 @@ +--- +"@objectstack/lint": patch +--- + +fix(lint): `flow-runas-unscoped` stops telling an author they declared a `runAs` they never wrote (#5693) + +The rule's message branched on whether `runAs` was **authored** or **defaulted**: + +```ts +typeof flow.runAs === 'string' ? `runAs:'user'` : `the default runAs:'user'` +``` + +That distinction is real and useful — "you wrote something incoherent" is not +"you inherited a default that does not fit a user-less trigger" — but the rule +cannot observe it, and which arm an author got depended on the **surface** rather +than on their file. + +**On the CLI, only the explicit arm was reachable.** `FlowSchema.runAs` carries +`.default('user')` and the registry wires this rule `input: 'parsed'`, so +`flow.runAs` is the string `'user'` whether the author wrote it or not. `os lint` +does not Zod-parse, and would have escaped that — except `defineStack` / +`defineFlow` parse at *definition* time, so the config module hands even the +non-parsing command a stack with the default already filled in. + +Measured on `examples/app-todo`, `overdue_escalation` with its `runAs` line +deleted — the author declared nothing: + +``` +BEFORE — os validate + flow 'overdue_escalation' · runAs: schedule-triggered flow runs as `runAs:'user'`, but a + schedule run has no trigger user — so its data node 'get_overdue_tasks' (get_record) … + +BEFORE — os lint + ✗ flow 'overdue_escalation' · runAs: schedule-triggered flow runs as `runAs:'user'`, but a + schedule run has no trigger user — so its data node 'get_overdue_tasks' (get_record) … +``` + +Both commands told someone who had written no `runAs` that their flow "runs as +`runAs:'user'`" — which invites *"I never wrote that, the tool is confused"* at +exactly the moment the tool is right and the fix is one line away. + +Meanwhile the **runtime publish gate** (#4463) judges the verbatim authored body, +so it really did reach the other arm — the same flow was told two different +things by two shipped surfaces. + +**What changed.** One sentence, true of both authoring inputs, on every surface: + +``` +AFTER — os validate and os lint, identical + flow 'overdue_escalation' · runAs: schedule-triggered flow runs under `runAs:'user'` + (the default when none is declared), but a schedule run has no trigger user — so its + data node 'get_overdue_tasks' (get_record) has no identity to scope to and will be + REFUSED at run time. +``` + +The parenthetical is a statement about the **value**, not an accusation about the +author, so it stays true for someone who did write `runAs:'user'`. This is the +house pattern rather than a new one: `flow-draft-status-ambiguous` says `has +status 'draft' (the default when none is authored)` for the same reason, on the +same mechanism. + +Only the wording moved: the same flows are flagged, with the same +`severity: 'error'`, the same `where`, the same `hint`, and the same region +clause when the evidence node is nested. diff --git a/packages/lint/src/lint-flow-patterns.test.ts b/packages/lint/src/lint-flow-patterns.test.ts index 49233f534d..f48ddde3f0 100644 --- a/packages/lint/src/lint-flow-patterns.test.ts +++ b/packages/lint/src/lint-flow-patterns.test.ts @@ -1,7 +1,8 @@ // Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. import { describe, it, expect } from 'vitest'; -import { TimeRelativeTriggerSchema, LoopConfigSchema } from '@objectstack/spec/automation'; +import { TimeRelativeTriggerSchema, LoopConfigSchema, FlowSchema } from '@objectstack/spec/automation'; +import { AUTHORING_RULES } from './authoring-rules.js'; import { lintFlowPatterns, FLOW_TIME_RELATIVE_ANTIPATTERN, @@ -339,7 +340,8 @@ describe('lintFlowPatterns — user-less runAs unscoped (#1888 / ADR-0049 / ADR- expect(fnds).toHaveLength(1); expect(fnds[0].rule).toBe(FLOW_RUNAS_UNSCOPED); expect(fnds[0].where).toContain('nightly_sweep'); - expect(fnds[0].message).toMatch(/default .*runAs:'user'/); + // #5693 — one wording for both authoring inputs; see the dedicated block below. + expect(fnds[0].message).toMatch(/runAs:'user'` \(the default when none is declared\)/); expect(fnds[0].message).toMatch(/REFUSED/); expect(fnds[0].hint).toMatch(/runAs:'system'/); }); @@ -552,8 +554,11 @@ describe('lintFlowPatterns — user-less runAs unscoped (#1888 / ADR-0049 / ADR- }); // The A/B twin. Same flow, same node, moved out of the body — this was - // already flagged before #5633, and its message must not have moved a byte. - it('leaves the TOP-LEVEL twin byte-identical (no region clause, no regression)', () => { + // already flagged before #5633, and its message must carry no region clause. + // (The sentence itself was re-worded once since, by #5693 — see the block at + // the end of this file for why the authored-vs-defaulted branch it used to + // carry could not survive.) + it('leaves the TOP-LEVEL twin without a region clause (no regression)', () => { const fnds = lintFlowPatterns({ flows: [{ name: 'nightly_sweep', @@ -568,8 +573,8 @@ describe('lintFlowPatterns — user-less runAs unscoped (#1888 / ADR-0049 / ADR- expect(fnds).toHaveLength(1); expect(fnds[0].where).toBe("flow 'nightly_sweep' · runAs"); expect(fnds[0].message).toBe( - "schedule-triggered flow runs as the default `runAs:'user'`, but a schedule run has no trigger " + - "user — so its data node 'touch' (update_record) has no identity to scope to and " + + "schedule-triggered flow runs under `runAs:'user'` (the default when none is declared), but a " + + "schedule run has no trigger user — so its data node 'touch' (update_record) has no identity to scope to and " + "will be REFUSED at run time.", ); expect(fnds[0].message).not.toContain('in loop'); @@ -673,6 +678,104 @@ describe('lintFlowPatterns — user-less runAs unscoped (#1888 / ADR-0049 / ADR- }); }); }); + + /** + * #5693 — ONE wording, true whether the author declared `runAs:'user'` or + * declared nothing. + * + * The message used to branch: `` `runAs:'user'` `` when `flow.runAs` was a + * string, `the default …` when it was absent. The distinction is real and + * useful — "you wrote something incoherent" is not "you inherited a default + * that does not fit a user-less trigger" — but *this rule cannot observe it*, + * and which arm an author got depended on the SURFACE rather than on their + * file: + * + * - **CLI** — always the explicit arm. `FlowSchema.runAs` carries + * `.default('user')` and the registry wires this rule `input: 'parsed'`, so + * `flow.runAs` is the string `'user'` either way. `os lint` does not parse, + * but `defineStack`/`defineFlow` parse at *definition* time, so even it + * receives the default already materialized. Measured on `app-todo` with the + * `runAs` line deleted: `os validate` AND `os lint` both told an author who + * had declared nothing that their flow "runs as `runAs:'user'`". + * - **Runtime publish gate (#4463)** — both arms, because it judges the + * verbatim authored body (`saveMetaItem` keeps `request.item` past the + * schema check). + * + * So the branch was not merely dead: it made one flow get two different + * sentences from two shipped surfaces, and on the surface authors meet first it + * produced the one that reads as an accusation. #5693 removed it in favour of a + * sentence that is true of both inputs on every surface. + * + * These two cases split the work deliberately, and only the second has teeth + * against a re-introduction — say so rather than let the pair read as one + * assertion made twice: + * + * - the PARSED case pins *why* the branch was pointless (both inputs arrive as + * the same object). A re-introduced branch would still pass it — that is the + * point: the CLI cannot tell these apart, which is the whole defect. + * - the UNPARSED case is the regression guard. It is the one input shape where + * the two authoring choices are still distinguishable (and the shape the + * runtime gate really passes), so any future `typeof flow.runAs === 'string'` + * branch in the message fails it immediately. + */ + describe("#5693 — one wording for authored `runAs:'user'` and for none at all", () => { + /** + * Authorable on purpose: `FlowSchema` requires `label` on the flow and on + * every node, so the raw literals the rest of this file feeds would fail the + * parse — and a fixture that cannot be parsed cannot demonstrate anything + * about the parsed tier. This one is the same sweep, declared in full. + */ + const sweep = (runAs?: 'user') => ({ + name: 'nightly_sweep', + label: 'Nightly Sweep', + type: 'schedule', + ...(runAs ? { runAs } : {}), + nodes: [ + { id: 'start', type: 'start', label: 'Start', config: { triggerType: 'schedule', cron: '0 8 * * *' } }, + { id: 'op', type: 'update_record', label: 'Touch', config: { objectName: 'thing', fields: { a: 1 } } }, + ], + edges: [{ id: 'e1', source: 'start', target: 'op' }], + }); + + const EXPECTED = + "schedule-triggered flow runs under `runAs:'user'` (the default when none is declared), but a " + + "schedule run has no trigger user — so its data node 'op' (update_record) has no identity to scope to and " + + "will be REFUSED at run time."; + + const messagesFor = (flow: unknown) => + lintFlowPatterns({ flows: [flow] }) + .filter((f) => f.rule === FLOW_RUNAS_UNSCOPED) + .map((f) => f.message); + + it('is wired to the tier where the default has already been filled in', () => { + const wiring = AUTHORING_RULES.find((r) => r.name === 'lintFlowPatterns'); + expect(wiring?.input).toBe('parsed'); + }); + + it('PARSED input: the two authoring choices are literally the same object here', () => { + const authored = FlowSchema.parse(sweep('user')); + const defaulted = FlowSchema.parse(sweep()); + // The premise, asserted rather than assumed: the parse materializes the + // default, so `flow.runAs` carries no trace of what was authored. + expect(defaulted.runAs).toBe('user'); + expect(authored.runAs).toBe('user'); + + expect(messagesFor(defaulted)).toEqual([EXPECTED]); + expect(messagesFor(authored)).toEqual([EXPECTED]); + }); + + it('UNPARSED input: an absent key and an explicit one still get the same sentence', () => { + // The runtime publish gate's shape — the only surface where the omission + // survives to the rule. Both must read the same, or the surfaces disagree + // about one flow again. + expect(messagesFor(sweep())).toEqual([EXPECTED]); + expect(messagesFor(sweep('user'))).toEqual([EXPECTED]); + }); + + it('says the same thing to both surfaces about the same flow', () => { + expect(messagesFor(FlowSchema.parse(sweep()))).toEqual(messagesFor(sweep())); + }); + }); }); /** diff --git a/packages/lint/src/lint-flow-patterns.ts b/packages/lint/src/lint-flow-patterns.ts index b6f20c348c..48c4413801 100644 --- a/packages/lint/src/lint-flow-patterns.ts +++ b/packages/lint/src/lint-flow-patterns.ts @@ -122,14 +122,18 @@ * flow-level, and the region is named in the **message** rather than in `where` * (`its data node 'touch' (update_record), in loop 'loop_rows' body,`): `where` * says which declaration is wrong, the message says where to find the proof. A - * top-level hit yields the byte-identical message it always did — pinned in the - * tests, since the wording is what every existing author already sees. + * top-level hit adds no region clause at all — pinned in the tests, since a + * top-level finding is what most authors see. * * What this fixes is not a corner. Query a set, loop it, write per item is *the* * shape of a scheduled data flow, so the write is almost always the nested node — * and because this rule gates the build, the shape it was missing built clean and * then could not run at all. That is precisely what promoting it to `error` * (#3760) was for. + * + * #5693 rewrote how that same message names the identity — one wording true of + * both authoring inputs instead of a branch on `flow.runAs` that no CLI command + * could take. See {@link RUNAS_EFFECTIVE_IDENTITY}. */ import { @@ -249,6 +253,49 @@ const INERT_CONDITION_NODE_TYPES = new Set([ /** Node types that perform a data operation — the ones `flow.runAs` governs (#1888). */ const DATA_NODE_TYPES = new Set(['get_record', 'create_record', 'update_record', 'delete_record']); +/** + * How {@link FLOW_RUNAS_UNSCOPED} names the identity the run would use — ONE + * wording, true whether the author wrote `runAs:'user'` or wrote nothing (#5693). + * + * It replaces a ternary that told the author which of the two they had done: + * `` `runAs:'user'` `` when `flow.runAs` was a string, `the default …` when it + * was absent. That distinction is real, but the rule cannot observe it, and the + * arm it picked depended on the SURFACE rather than on the metadata: + * + * - **CLI (`os validate` / `os build` / `os lint`) — always the explicit arm.** + * `FlowSchema.runAs` carries `.default('user')`, and the registry wires this + * rule `input: 'parsed'`, so `flow.runAs` is the string `'user'` either way. + * `os lint` does not parse and would have escaped that, except that + * `defineStack` (and `defineFlow`) parse at *definition* time, so the config + * module hands even the non-parsing command a stack with the default already + * materialized. Measured on `examples/app-todo` with the `runAs` line deleted: + * both commands printed the EXPLICIT arm at an author who had declared nothing. + * - **Runtime publish gate (#4463) — both arms.** It judges `request.item`, the + * verbatim authored body (`saveMetaItem` keeps it verbatim past the schema + * check), so an omitted key really is absent there. + * + * So the same flow was told two different things by two shipped surfaces, and on + * the surface an author uses first it was told the one that reads as an + * accusation: *"you declared `runAs:'user'`"* to someone who declared nothing — + * inviting "the tool is confused" at the exact moment the tool is right and the + * fix is one line. Restoring the distinction would mean giving a `parsed`-tier + * rule a second, pre-parse input; #5693 chose the wording instead. + * + * The parenthetical is a statement about the VALUE, not an accusation about the + * author, so it stays true for someone who did write `runAs:'user'`. That is the + * house pattern, not a new one: `flow-draft-status-ambiguous` says `has status + * 'draft' (the default when none is authored)` for exactly this reason, on + * exactly this mechanism (`validate-flow-trigger-readiness.ts`). + * + * `'user'` is spelled out rather than interpolated from `flow.runAs` because the + * branch that uses this has already excluded `'system'` and the enum holds only + * those two — so on every surface that reaches the message (CLI: parsed; runtime + * gate: `safeParse`d against the overlay schema before the gate runs) the + * effective identity IS `'user'`. Interpolating would re-introduce a limb only + * an off-spec literal could reach, which is the defect this replaced. + */ +const RUNAS_EFFECTIVE_IDENTITY = "`runAs:'user'` (the default when none is declared)"; + /** * The first data node ANYWHERE in a flow, with the region it was found in — * {@link FLOW_RUNAS_UNSCOPED}'s evidence that the flow performs a data operation @@ -1034,22 +1081,24 @@ export function lintFlowPatterns(stack: AnyRec): FlowLintFinding[] { // build-GATING rule is its own change with its own blast radius — this // is that change. The shape it was missing is the DEFAULT one for a // scheduled data flow: query a set, loop it, write per item. + // #5693 — the message states the EFFECTIVE identity in one wording that + // is true of both authoring inputs, and does not branch on whether the + // author wrote `runAs` (see {@link RUNAS_EFFECTIVE_IDENTITY}). const runAs = typeof flow.runAs === 'string' ? flow.runAs : 'user'; const userLessKind = userLessTriggerKind(flow, startCfg); if (userLessKind && runAs !== 'system') { const dataNode = findDataNodeAnywhere(nodes, edges); if (dataNode) { - const declared = typeof flow.runAs === 'string' ? `\`runAs:'${runAs}'\`` : `the default \`runAs:'user'\``; // The region is named in the MESSAGE, not in `where`: `where` says which // declaration is wrong (`flow 'x' · runAs`, unchanged), the message says // where to look for the node that proves it. A top-level hit adds nothing - // here, so its wording is byte-identical to before (pinned in the tests). + // here, so its wording carries no region clause (pinned in the tests). const at = dataNode.scope ? `, in ${dataNode.scope},` : ''; findings.push({ where: `flow '${flowName}' · runAs`, message: - `${userLessKind}-triggered flow runs as ${declared}, but a ${userLessKind} run has no trigger ` + - `user — so its data node '${dataNode.node.id}' (${dataNode.node.type})${at} has no identity to scope to and ` + + `${userLessKind}-triggered flow runs under ${RUNAS_EFFECTIVE_IDENTITY}, but a ${userLessKind} run ` + + `has no trigger user — so its data node '${dataNode.node.id}' (${dataNode.node.type})${at} has no identity to scope to and ` + `will be REFUSED at run time.`, hint: `Declare \`runAs:'system'\` to make the elevation explicit and intended (the run reads/writes ` +