diff --git a/.changeset/lint-readonly-when-system-flows.md b/.changeset/lint-readonly-when-system-flows.md new file mode 100644 index 0000000000..17c93a949d --- /dev/null +++ b/.changeset/lint-readonly-when-system-flows.md @@ -0,0 +1,21 @@ +--- +'@objectstack/lint': patch +--- + +lint: `flow-update-readonly-when-field` now inspects `runAs:'system'` flows + +The `runAs:'system'` exemption in `validate-readonly-flow-writes` was a single +flow-level early return, so it removed an elevated flow from **both** branches of +the rule. Only the static branch warrants it: the engine skips +`stripReadonlyFields` under `if (!opCtx.context?.isSystem)`, but +`stripReadonlyWhenFields` runs on the update path with no `isSystem` guard at all +(`packages/objectql/src/engine.ts`, the #9107 note: "`isSystem` is still NOT an +exemption here, unlike the static strip below"), pinned as "LOCK 2 — isSystem does +NOT exempt a caller-supplied value". + +The exemption now gates the static branch only. A `runAs:'system'` flow whose +`update_record` node writes a `readonlyWhen` field reports the branch's existing +`warning` — the same silent-no-op the rule exists to surface, on the flow class the +rule's own hint tells the author elevation cannot save. A system flow writing a +static `readonly:true` field stays silent, as before; rule ids and severities are +unchanged, and the new finding is advisory and never blocks a build. diff --git a/packages/lint/src/validate-readonly-flow-writes.test.ts b/packages/lint/src/validate-readonly-flow-writes.test.ts index 6c7fadb82a..aec906ead7 100644 --- a/packages/lint/src/validate-readonly-flow-writes.test.ts +++ b/packages/lint/src/validate-readonly-flow-writes.test.ts @@ -197,7 +197,12 @@ describe('validateReadonlyFlowWrites', () => { }); // ── clean: runAs:system is the intended maintenance channel ─────────── - it('does NOT flag a runAs:system flow (elevated writer bypasses the strip)', () => { + // …for the STATIC strip, and only for it. The engine skips + // `stripReadonlyFields` under `if (!opCtx.context?.isSystem)`, so an elevated + // flow maintaining a `readonly:true` column is the intended channel and stays + // silent. Paired with the `readonlyWhen` case below, which is the OTHER half + // of the same run identity — the two must not move together (#14201). + it('does NOT flag a runAs:system flow writing a STATIC readonly field (elevated writer bypasses that strip)', () => { const findings = validateReadonlyFlowWrites({ objects: [opportunityObject], flows: [flowWith({ approval_status: 'approved' }, { runAs: 'system' })], @@ -205,6 +210,120 @@ describe('validateReadonlyFlowWrites', () => { expect(findings).toEqual([]); }); + // ── runAs:system + readonlyWhen → still a WARNING (#14201) ──────────── + // `stripReadonlyWhenFields` is called on the update path with NO `isSystem` + // guard at all (engine.ts, the #9107 note: "`isSystem` is still NOT an + // exemption here, unlike the static strip below"), pinned from both sides as + // "LOCK 2 — isSystem does NOT exempt a caller-supplied value" + // (`engine-readonly-when-derived-writes.test.ts`) and "covers readonlyWhen + // too — the arm a trusted (isSystem) caller can still hit" + // (`engine-readonly-strict-writes.test.ts`). So the elevated flow's write + // vanishes on a locked record exactly as a user run's does, and the rule that + // exists to surface that silent no-op has to say so on the very flow class + // its own hint tells the author elevation cannot save. + it('warns when a runAs:system flow writes a readonlyWhen field (elevation does NOT waive the conditional strip)', () => { + const findings = validateReadonlyFlowWrites({ + objects: [opportunityObject], + flows: [flowWith({ amount: 5000 }, { runAs: 'system' })], + }); + expect(findings).toHaveLength(1); + expect(findings[0].severity).toBe('warning'); + expect(findings[0].rule).toBe(FLOW_UPDATE_READONLY_WHEN_FIELD); + expect(findings[0].path).toBe('flows[0].nodes[1].config.fields.amount'); + // The message states the run identity it was judged under, so a reader of + // the finding cannot mistake it for the user-run case. + expect(findings[0].message).toContain("runAs:'system'"); + expect(findings[0].message).toContain('#3042'); + expect(findings[0].hint).toContain('NOT waived by a system context'); + }); + + it('reports ONLY the conditional half for a runAs:system node writing both kinds in one payload', () => { + const findings = validateReadonlyFlowWrites({ + objects: [opportunityObject], + flows: [flowWith({ approval_status: 'approved', amount: 5000, notes: 'hi' }, { runAs: 'system' })], + }); + expect(findings).toHaveLength(1); + expect(findings[0].severity).toBe('warning'); + expect(findings[0].path).toBe('flows[0].nodes[1].config.fields.amount'); + expect(findings.some((f) => f.rule === FLOW_UPDATE_READONLY_FIELD)).toBe(false); + }); + + // A field declaring BOTH flags: under `runAs:'system'` the static strip is + // skipped and the conditional one is not, so the truthful finding is the + // warning — not silence (the old flow-level skip) and not the error (which + // would state something false about an elevated write). + it('falls through to the conditional branch for a field declaring readonly AND readonlyWhen under runAs:system', () => { + const bothFlags = { + name: 'crm_opportunity', + fields: { + approval_status: { type: 'text', readonly: true, readonlyWhen: "record.stage == 'closed_won'" }, + }, + }; + const systemFindings = validateReadonlyFlowWrites({ + objects: [bothFlags], + flows: [flowWith({ approval_status: 'approved' }, { runAs: 'system' })], + }); + expect(systemFindings).toHaveLength(1); + expect(systemFindings[0].severity).toBe('warning'); + expect(systemFindings[0].rule).toBe(FLOW_UPDATE_READONLY_WHEN_FIELD); + + // Unchanged for a user run: the static strip applies there, and the certain + // no-op outranks the conditional one. + const userFindings = validateReadonlyFlowWrites({ + objects: [bothFlags], + flows: [flowWith({ approval_status: 'approved' }, { runAs: 'user' })], + }); + expect(userFindings).toHaveLength(1); + expect(userFindings[0].severity).toBe('error'); + expect(userFindings[0].rule).toBe(FLOW_UPDATE_READONLY_FIELD); + }); + + // Nesting is orthogonal to run identity: the walk reaches an elevated flow's + // nested regions on the conditional branch too. + it('reaches a readonlyWhen write nested in a loop body under runAs:system', () => { + const flow = { + name: 'sweep_system', + runAs: 'system', + nodes: [ + { + id: 'each', + type: 'loop', + label: 'Each', + config: { + collection: '{items}', + body: { + nodes: [ + { id: 'u', type: 'update_record', label: 'U', config: { objectName: 'crm_opportunity', fields: { amount: 1 } } }, + ], + edges: [], + }, + }, + }, + ], + edges: [], + }; + const findings = validateReadonlyFlowWrites({ objects: [opportunityObject], flows: [flow] }); + expect(findings).toHaveLength(1); + expect(findings[0].severity).toBe('warning'); + expect(findings[0].path).toBe('flows[0].nodes[0].config.body.nodes[0].config.fields.amount'); + }); + + // create_record stays exempt on BOTH branches under elevation: a + // `readonlyWhen` predicate has no prior record to evaluate on an insert. + it('does NOT flag create_record writing a readonlyWhen field under runAs:system', () => { + const flow = { + name: 'seed_opp_system', + type: 'record_change', + runAs: 'system', + nodes: [ + { id: 'start', type: 'start', config: {} }, + { id: 'c', type: 'create_record', label: 'Create', config: { objectName: 'crm_opportunity', fields: { amount: 10 } } }, + ], + edges: [], + }; + expect(validateReadonlyFlowWrites({ objects: [opportunityObject], flows: [flow] })).toEqual([]); + }); + // ── clean: create_record is engine-exempt from the readonly strip ───── it('does NOT flag create_record writing a readonly field', () => { const flow = { diff --git a/packages/lint/src/validate-readonly-flow-writes.ts b/packages/lint/src/validate-readonly-flow-writes.ts index b2c8032f5c..daace3043d 100644 --- a/packages/lint/src/validate-readonly-flow-writes.ts +++ b/packages/lint/src/validate-readonly-flow-writes.ts @@ -17,21 +17,26 @@ // by calling the data engine directly), so a create writing a readonly // field is NOT a no-op and is never flagged. // -// • Only `runAs !== 'system'`. A `runAs:'system'` run is elevated and the -// engine skips the STATIC `readonly` strip, so a system flow legitimately -// MAINTAINS readonly fields ("users can't edit this, but automation does"). -// That is the intended channel, so it is never flagged. +// • `runAs:'system'` exempts the STATIC branch ONLY - it is not a flow-level +// skip. An elevated run bypasses the static `readonly` strip, so a system +// flow legitimately MAINTAINS readonly fields ("users can't edit this, but +// automation does"). That is the intended channel, so it is never flagged. // -// ⚠️ That exemption is the STATIC strip's alone. `stripReadonlyWhenFields` -// runs with no `isSystem` guard at all (engine.ts, the #9107 note: "`isSystem` -// is still NOT an exemption here, unlike the static strip below"), pinned as -// "LOCK 2 - isSystem does NOT exempt a caller-supplied value" in -// `engine-readonly-when-derived-writes.test.ts`. So elevation is NOT a -// `readonlyWhen` remedy, and this rule's hint must never offer it. The skip -// above is therefore WIDER than the conditional lock warrants - a -// `runAs:'system'` flow writing a `readonlyWhen` field is still stripped on a -// locked record and goes unflagged. Left as-is deliberately: the match set is -// out of scope for the message-text correction that fixed the hint. +// ⚠️ The exemption stops there. `stripReadonlyWhenFields` runs with no +// `isSystem` guard at all (engine.ts, the #9107 note: "`isSystem` is still +// NOT an exemption here, unlike the static strip below"), pinned as "LOCK 2 +// - isSystem does NOT exempt a caller-supplied value" in +// `engine-readonly-when-derived-writes.test.ts` and from the other side in +// `engine-readonly-strict-writes.test.ts` ("covers readonlyWhen too - the +// arm a trusted (isSystem) caller can still hit"). So a `runAs:'system'` +// flow writing a `readonlyWhen` field IS still stripped on a locked record, +// and the conditional branch inspects an elevated flow exactly like any +// other, at its usual `warning` severity. Narrowing this exemption to the +// branch it belongs to (#14201) is what stops the rule from going silent on +// the one flow class its own hint tells the author elevation cannot save - +// the same split the action sibling was born with +// (`validate-readonly-action-writes.ts`: an action body is system-elevated +// BY DESIGN, so it carries the conditional half and only that half). // // • Static `readonly:true` + a LITERAL field name is a 100%-certain no-op → // ERROR (gates the build). `readonlyWhen` is per-record-state — it strips @@ -150,10 +155,13 @@ export function validateReadonlyFlowWrites(stack: AnyRec): ReadonlyFlowWriteFind flows.forEach((flow, flowIndex) => { // `runAs` defaults to 'user' (schema default). Only an explicit 'system' - // run bypasses the strip, so treat anything else — including an unauthored - // (undefined) runAs — as strip-subject. - if (flow.runAs === 'system') return; + // run bypasses the STATIC strip, so treat anything else — including an + // unauthored (undefined) runAs — as subject to both strips. ⛔ Not a + // flow-level skip: the conditional strip has no `isSystem` guard, so an + // elevated flow stays in the walk and is judged on the `readonlyWhen` + // branch below (#14201). const runAs = flow.runAs === 'user' || flow.runAs === 'system' ? flow.runAs : 'user'; + const isSystemRun = runAs === 'system'; const flowName = typeof flow.name === 'string' ? flow.name : `#${flowIndex}`; // Every node, INCLUDING those nested in try_catch / loop / parallel regions. @@ -189,7 +197,12 @@ export function validateReadonlyFlowWrites(stack: AnyRec): ReadonlyFlowWriteFind // the two never double-report the same key. if (!meta) continue; - if (meta.readonly) { + // The static branch is the one an elevated run really does bypass, so + // `isSystem` gates it HERE rather than at flow level. A field declaring + // BOTH flags therefore still falls through to the conditional branch + // under `runAs:'system'` — which is the truth about that write: the + // static strip is skipped, the conditional one is not. + if (meta.readonly && !isSystemRun) { findings.push({ severity: 'error', rule: FLOW_UPDATE_READONLY_FIELD,