Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 11 additions & 7 deletions .changeset/flow-runas-unscoped-region-descent.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 /
Expand Down
64 changes: 64 additions & 0 deletions .changeset/flow-runas-unscoped-two-way-true-wording.md
Original file line number Diff line number Diff line change
@@ -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.
115 changes: 109 additions & 6 deletions packages/lint/src/lint-flow-patterns.test.ts
Original file line number Diff line number Diff line change
@@ -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,
Expand Down Expand Up @@ -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'/);
});
Expand Down Expand Up @@ -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',
Expand All @@ -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');
Expand Down Expand Up @@ -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()));
});
});
});

/**
Expand Down
61 changes: 55 additions & 6 deletions packages/lint/src/lint-flow-patterns.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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 ` +
Expand Down
Loading