fix(lint): inspect runAs:'system' flows on the readonlyWhen branch (#14370)

The runAs:'system' exemption in validate-readonly-flow-writes was a single
flow-level early return, removing an elevated flow from BOTH branches of the
rule. Only the static branch warrants it: the engine skips stripReadonlyFields
under `if (!opCtx.context?.isSystem)`, while stripReadonlyWhenFields runs on the
update path with no isSystem guard at all.

The exemption now gates the static branch only, so a system flow whose
update_record node writes a readonlyWhen field reports the conditional branch's
existing warning. Rule ids and severities are untouched.

Co-authored-by: Claude <noreply@anthropic.com>

Author: claude[bot]

.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.

packages/lint/src/validate-readonly-flow-writes.test.ts

@@ -197,14 +197,133 @@ 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' })],
     });
     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 = {

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,