From d9e6572a9ca41f707b8a43c905b3fb3c2f712956 Mon Sep 17 00:00:00 2001 From: Jack Zhuang <277994282+os-zhuang@users.noreply.github.com> Date: Fri, 24 Jul 2026 23:32:44 +0800 Subject: [PATCH] feat(lint,cli): flag flow update_record writes to readonly fields at design time (#3425) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A flow `update_record` node that writes a field the target object declares `readonly: true`, under the default `runAs: 'user'`, is a silent no-op: the objectql engine strips static-`readonly` fields from a non-system UPDATE payload (#2948) so the write never lands, while the step still reports success. #3407/#3413 surfaced the strip at run time; this shifts discovery left to `os validate` / `os build`. - New `@objectstack/lint` rule `validateReadonlyFlowWrites`: static readonly + literal field under runAs!=='system' → error (gates); readonlyWhen → warning. Skips create_record (INSERT is engine-exempt), runAs:'system' flows, templated object names, and non-literal fields maps to stay false-positive-free. - Wired into `os validate` and `os compile`/`os build` (mirrors the security posture gate). Verified: reds on a violating app-crm flow, clean on all example apps. - Documents the formal contract in the objectstack-data / -automation skills: readonly governs the user/API surface; system writers (runAs:'system', hooks, seeds) maintain it. Co-Authored-By: Claude Fable 5 --- .changeset/readonly-flow-write-lint.md | 31 +++ packages/cli/src/commands/compile.ts | 34 +++ packages/cli/src/commands/validate.ts | 36 +++ packages/lint/src/index.ts | 10 + .../src/validate-readonly-flow-writes.test.ts | 243 ++++++++++++++++++ .../lint/src/validate-readonly-flow-writes.ts | 197 ++++++++++++++ skills/objectstack-automation/SKILL.md | 12 + skills/objectstack-data/SKILL.md | 10 + 8 files changed, 573 insertions(+) create mode 100644 .changeset/readonly-flow-write-lint.md create mode 100644 packages/lint/src/validate-readonly-flow-writes.test.ts create mode 100644 packages/lint/src/validate-readonly-flow-writes.ts diff --git a/.changeset/readonly-flow-write-lint.md b/.changeset/readonly-flow-write-lint.md new file mode 100644 index 000000000..80634aa16 --- /dev/null +++ b/.changeset/readonly-flow-write-lint.md @@ -0,0 +1,31 @@ +--- +"@objectstack/lint": minor +"@objectstack/cli": minor +--- + +feat(lint,cli): flag flow `update_record` writes to readonly fields at design time (#3425) + +A flow `update_record` node that writes a field the target object declares +`readonly: true`, under the default `runAs: 'user'` identity, is a **silent +no-op**: the objectql engine strips static-`readonly` fields from a non-system +UPDATE payload (#2948), so the intended write never lands — yet the step still +reports `success`. #3407/#3413 surfaced the strip as a run-time step warning; +this moves the discovery **left** to `os validate` / `os build` so an author +finds the mismatch at design time instead of by reading server WARN logs days +later. + +- New `@objectstack/lint` rule `validateReadonlyFlowWrites(stack)` — a pure + `(stack) => Finding[]` check (ADR-0019). A static `readonly:true` field + written by a literal `update_record` under `runAs !== 'system'` is a + 100%-certain no-op → **error** (gates the build). A `readonlyWhen` field is + per-record-state → **warning** (advisory). Deliberately narrow to stay + false-positive-free: `create_record` (INSERT is engine-exempt from the strip), + `runAs: 'system'` flows (the intended "automation maintains it" channel), + templated object names, and non-literal `fields` maps are all skipped. +- Wired into `os validate` and `os compile`/`os build`, mirroring the existing + security-posture gate (errors fail; advisories print dimmed). + +The formal contract, unchanged in behavior: `readonly` governs the end-user / +API surface (REST/UI and `runAs:'user'` flows strip it); trusted system writers +(`runAs:'system'`, system hooks, seeds) maintain it. To let a flow maintain a +readonly field, declare `runAs: 'system'`. diff --git a/packages/cli/src/commands/compile.ts b/packages/cli/src/commands/compile.ts index 9f171478d..68cc2319c 100644 --- a/packages/cli/src/commands/compile.ts +++ b/packages/cli/src/commands/compile.ts @@ -14,6 +14,7 @@ import { validateWidgetBindings } from '@objectstack/lint'; import { validateDashboardActionRefs } from '@objectstack/lint'; import { validateResponsiveStyles } from '@objectstack/lint'; import { validateSecurityPosture, buildAccessMatrix, diffAccessMatrix } from '@objectstack/lint'; +import { validateReadonlyFlowWrites } from '@objectstack/lint'; import { lintFlowPatterns } from '../utils/lint-flow-patterns.js'; import { lintAutonumberFormats } from '../utils/lint-autonumber-formats.js'; import { lintLivenessProperties } from '../utils/lint-liveness-properties.js'; @@ -451,6 +452,39 @@ export default class Compile extends Command { } } + // 3e2. [#3425] Readonly flow-write guardrail. A `runAs:user` update_record + // writing a static-`readonly` field is a silent no-op — the engine + // strips it from the UPDATE payload (#2948) while the step reports + // success. This GATES the build (shift-left of the #3407/#3413 + // run-time strip warning); `readonlyWhen` writes are per-record-state, + // so they are advisory, printed dimmed and never fatal. + if (!flags.json) printStep('Checking readonly flow writes (#3425)...'); + const readonlyWriteFindings = validateReadonlyFlowWrites(result.data as Record); + const readonlyWriteErrors = readonlyWriteFindings.filter((f) => f.severity === 'error'); + const readonlyWriteAdvisories = readonlyWriteFindings.filter((f) => f.severity !== 'error'); + if (readonlyWriteErrors.length > 0) { + if (flags.json) { + console.log(JSON.stringify({ success: false, error: 'readonly flow-write validation failed', issues: readonlyWriteErrors })); + this.exit(1); + } + console.log(''); + printError(`Readonly flow-write check failed (${readonlyWriteErrors.length} issue${readonlyWriteErrors.length > 1 ? 's' : ''})`); + for (const f of readonlyWriteErrors.slice(0, 50)) { + console.log(` • ${f.where}: ${f.message}`); + console.log(chalk.dim(` ${f.hint}`)); + console.log(chalk.dim(` rule: ${f.rule} at ${f.path}`)); + } + this.exit(1); + } + if (readonlyWriteAdvisories.length > 0 && !flags.json) { + console.log(''); + for (const f of readonlyWriteAdvisories) { + printWarning(`${f.where}: ${f.message}`); + console.log(chalk.dim(` ${f.hint}`)); + console.log(chalk.dim(` rule: ${f.rule}`)); + } + } + // 3f. [ADR-0090 D6] Access-matrix snapshot gate. Opt-in per app: when // `access-matrix.json` sits next to the config, the (permission set // × object) capability matrix derived from THIS build must match it diff --git a/packages/cli/src/commands/validate.ts b/packages/cli/src/commands/validate.ts index b11ca5e29..cc2487246 100644 --- a/packages/cli/src/commands/validate.ts +++ b/packages/cli/src/commands/validate.ts @@ -19,6 +19,7 @@ import { validateCapabilityReferences } from '@objectstack/lint'; import { validateVisibilityPredicates } from '@objectstack/lint'; import { validateSecurityPosture } from '@objectstack/lint'; import { validateFlowTriggerReadiness } from '@objectstack/lint'; +import { validateReadonlyFlowWrites } from '@objectstack/lint'; import { preflightRequiredCapabilities, renderCapabilityMessage } from '../utils/capability-preflight.js'; import { printHeader, @@ -427,6 +428,41 @@ export default class Validate extends Command { } } + // 3e2. [#3425] Readonly flow-write guardrail. A `runAs:user` update_record + // that writes a static-`readonly` field is a SILENT no-op — the engine + // strips it from the UPDATE payload (#2948) yet the step reports + // success. This is the shift-left of the run-time strip warning + // (#3407/#3413): a static readonly + literal field is a certain no-op + // → error (gates); a `readonlyWhen` field is per-record-state → advisory. + if (!flags.json) printStep('Checking readonly flow writes (#3425)...'); + const readonlyWriteFindings = validateReadonlyFlowWrites(normalized as Record); + const readonlyWriteErrors = readonlyWriteFindings.filter((f) => f.severity === 'error'); + const readonlyWriteWarnings = readonlyWriteFindings.filter((f) => f.severity === 'warning'); + if (readonlyWriteErrors.length > 0) { + if (flags.json) { + console.log(JSON.stringify({ + valid: false, + errors: readonlyWriteErrors, + duration: timer.elapsed(), + }, null, 2)); + this.exit(1); + } + console.log(''); + printError(`Readonly flow-write check failed (${readonlyWriteErrors.length} issue${readonlyWriteErrors.length > 1 ? 's' : ''})`); + for (const f of readonlyWriteErrors.slice(0, 50)) { + console.log(` • ${f.where}: ${f.message}`); + console.log(chalk.dim(` ${f.hint}`)); + console.log(chalk.dim(` rule: ${f.rule} at ${f.path}`)); + } + this.exit(1); + } + if (!flags.json) { + for (const f of readonlyWriteWarnings.slice(0, 50)) { + console.log(chalk.yellow(` ⚠ ${f.where}: ${f.message}`)); + console.log(chalk.dim(` ${f.hint}`)); + } + } + // 3f. [ADR-0090 D7] Security posture — the same gate `os compile`/`os build` // run. Without it here, `os validate` passed a stack (e.g. a custom // object with no explicit sharingModel) that the build then rejected, diff --git a/packages/lint/src/index.ts b/packages/lint/src/index.ts index af580d86c..68554e4b7 100644 --- a/packages/lint/src/index.ts +++ b/packages/lint/src/index.ts @@ -39,6 +39,16 @@ export type { FlowTriggerReadinessSeverity, } from './validate-flow-trigger-readiness.js'; +export { + validateReadonlyFlowWrites, + FLOW_UPDATE_READONLY_FIELD, + FLOW_UPDATE_READONLY_WHEN_FIELD, +} from './validate-readonly-flow-writes.js'; +export type { + ReadonlyFlowWriteFinding, + ReadonlyFlowWriteSeverity, +} from './validate-readonly-flow-writes.js'; + export { validateViewContainers, VIEW_CONTAINER_SHAPE } from './validate-view-containers.js'; export type { ViewContainerFinding, ViewContainerSeverity } from './validate-view-containers.js'; diff --git a/packages/lint/src/validate-readonly-flow-writes.test.ts b/packages/lint/src/validate-readonly-flow-writes.test.ts new file mode 100644 index 000000000..6b3debeb2 --- /dev/null +++ b/packages/lint/src/validate-readonly-flow-writes.test.ts @@ -0,0 +1,243 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +import { describe, it, expect } from 'vitest'; +import { + validateReadonlyFlowWrites, + FLOW_UPDATE_READONLY_FIELD, + FLOW_UPDATE_READONLY_WHEN_FIELD, +} from './validate-readonly-flow-writes.js'; + +// Target object: a static-readonly field, a conditional readonlyWhen field, and +// a plain writable field. Map-shaped `fields` (the common authoring form). +const opportunityObject = { + name: 'crm_opportunity', + label: 'Opportunity', + fields: { + approval_status: { type: 'text', readonly: true }, + amount: { type: 'currency', readonlyWhen: "record.stage == 'closed_won'" }, + notes: { type: 'text' }, + }, +}; + +/** A flow with a single `update_record` node (nodes[1]) writing `fields`. */ +function flowWith( + fields: unknown, + flowOverrides: Record = {}, + nodeConfigOverrides: Record = {}, +) { + return { + name: 'stamp_approval', + type: 'record_change', + nodes: [ + { id: 'start', type: 'start', config: {} }, + { + id: 'stamp', + type: 'update_record', + label: 'Stamp approval', + config: { objectName: 'crm_opportunity', filter: { id: '{recordId}' }, fields, ...nodeConfigOverrides }, + }, + { id: 'end', type: 'end' }, + ], + edges: [], + ...flowOverrides, + }; +} + +describe('validateReadonlyFlowWrites', () => { + // ── static readonly → ERROR ────────────────────────────────────────── + it('errors when a runAs:user update_record writes a static-readonly field', () => { + const findings = validateReadonlyFlowWrites({ + objects: [opportunityObject], + flows: [flowWith({ approval_status: 'approved' }, { runAs: 'user' })], + }); + expect(findings).toHaveLength(1); + expect(findings[0].severity).toBe('error'); + expect(findings[0].rule).toBe(FLOW_UPDATE_READONLY_FIELD); + expect(findings[0].path).toBe('flows[0].nodes[1].config.fields.approval_status'); + expect(findings[0].message).toContain('approval_status'); + expect(findings[0].message).toContain('crm_opportunity'); + expect(findings[0].message).toContain('#2948'); + expect(findings[0].where).toBe('flow "stamp_approval" › node "Stamp approval"'); + }); + + it('errors when runAs is unauthored (defaults to user)', () => { + const findings = validateReadonlyFlowWrites({ + objects: [opportunityObject], + flows: [flowWith({ approval_status: 'approved' })], // no runAs + }); + expect(findings).toHaveLength(1); + expect(findings[0].severity).toBe('error'); + }); + + it('resolves the target object via the `object` alias', () => { + const findings = validateReadonlyFlowWrites({ + objects: [opportunityObject], + flows: [flowWith({ approval_status: 'approved' }, { runAs: 'user' }, { objectName: undefined, object: 'crm_opportunity' })], + }); + expect(findings).toHaveLength(1); + expect(findings[0].rule).toBe(FLOW_UPDATE_READONLY_FIELD); + }); + + it('flags each readonly field written in one node', () => { + const twoReadonly = { + name: 'crm_case', + fields: { + is_sla_violated: { type: 'boolean', readonly: true }, + closed_at: { type: 'datetime', readonly: true }, + subject: { type: 'text' }, + }, + }; + const flow = { + name: 'close_case', + type: 'record_change', + runAs: 'user', + nodes: [ + { id: 'start', type: 'start', config: {} }, + { + id: 'u', + type: 'update_record', + label: 'Close', + config: { objectName: 'crm_case', fields: { is_sla_violated: true, closed_at: '{now}', subject: 'x' } }, + }, + ], + edges: [], + }; + const findings = validateReadonlyFlowWrites({ objects: [twoReadonly], flows: [flow] }); + expect(findings).toHaveLength(2); + expect(findings.every((f) => f.severity === 'error')).toBe(true); + expect(findings.map((f) => f.path)).toEqual([ + 'flows[0].nodes[1].config.fields.is_sla_violated', + 'flows[0].nodes[1].config.fields.closed_at', + ]); + }); + + it('handles array-shaped object.fields', () => { + const arrObject = { + name: 'crm_lead', + fields: [ + { name: 'converted_account', type: 'lookup', readonly: true }, + { name: 'company', type: 'text' }, + ], + }; + const flow = { + name: 'convert', + type: 'record_change', + runAs: 'user', + nodes: [ + { id: 'start', type: 'start', config: {} }, + { id: 'u', type: 'update_record', label: 'Convert', config: { objectName: 'crm_lead', fields: { converted_account: '{acct}' } } }, + ], + edges: [], + }; + const findings = validateReadonlyFlowWrites({ objects: [arrObject], flows: [flow] }); + expect(findings).toHaveLength(1); + expect(findings[0].rule).toBe(FLOW_UPDATE_READONLY_FIELD); + }); + + // ── readonlyWhen → WARNING ─────────────────────────────────────────── + it('warns (not errors) when writing a readonlyWhen field', () => { + const findings = validateReadonlyFlowWrites({ + objects: [opportunityObject], + flows: [flowWith({ amount: 5000 }, { runAs: 'user' })], + }); + expect(findings).toHaveLength(1); + expect(findings[0].severity).toBe('warning'); + expect(findings[0].rule).toBe(FLOW_UPDATE_READONLY_WHEN_FIELD); + expect(findings[0].message).toContain('#3042'); + }); + + it('separates readonly (error) + readonlyWhen (warning) + plain (clean) in one node', () => { + const findings = validateReadonlyFlowWrites({ + objects: [opportunityObject], + flows: [flowWith({ approval_status: 'approved', amount: 1, notes: 'hi' }, { runAs: 'user' })], + }); + expect(findings).toHaveLength(2); + expect(findings.find((f) => f.severity === 'error')?.path).toBe('flows[0].nodes[1].config.fields.approval_status'); + expect(findings.find((f) => f.severity === 'warning')?.path).toBe('flows[0].nodes[1].config.fields.amount'); + }); + + // ── clean: runAs:system is the intended maintenance channel ─────────── + it('does NOT flag a runAs:system flow (elevated writer bypasses the strip)', () => { + const findings = validateReadonlyFlowWrites({ + objects: [opportunityObject], + flows: [flowWith({ approval_status: 'approved' }, { runAs: 'system' })], + }); + expect(findings).toEqual([]); + }); + + // ── clean: create_record is engine-exempt from the readonly strip ───── + it('does NOT flag create_record writing a readonly field', () => { + const flow = { + name: 'seed_opp', + type: 'record_change', + runAs: 'user', + nodes: [ + { id: 'start', type: 'start', config: {} }, + { id: 'c', type: 'create_record', label: 'Create', config: { objectName: 'crm_opportunity', fields: { approval_status: 'approved' } } }, + ], + edges: [], + }; + const findings = validateReadonlyFlowWrites({ objects: [opportunityObject], flows: [flow] }); + expect(findings).toEqual([]); + }); + + // ── clean: plain writable field ────────────────────────────────────── + it('does NOT flag writes to a plain writable field', () => { + const findings = validateReadonlyFlowWrites({ + objects: [opportunityObject], + flows: [flowWith({ notes: 'updated' }, { runAs: 'user' })], + }); + expect(findings).toEqual([]); + }); + + // ── clean: not statically knowable ─────────────────────────────────── + it('skips a templated objectName (dynamic target)', () => { + const findings = validateReadonlyFlowWrites({ + objects: [opportunityObject], + flows: [flowWith({ approval_status: 'approved' }, { runAs: 'user' }, { objectName: '{targetObject}' })], + }); + expect(findings).toEqual([]); + }); + + it('skips a non-literal fields map (dynamic write payload)', () => { + const findings = validateReadonlyFlowWrites({ + objects: [opportunityObject], + flows: [flowWith('{allFields}', { runAs: 'user' })], + }); + expect(findings).toEqual([]); + }); + + it('skips an object not defined in this stack (another package)', () => { + const findings = validateReadonlyFlowWrites({ + objects: [], // crm_opportunity not present + flows: [flowWith({ approval_status: 'approved' }, { runAs: 'user' })], + }); + expect(findings).toEqual([]); + }); + + it('does NOT flag an unknown field (not this rule’s concern)', () => { + const findings = validateReadonlyFlowWrites({ + objects: [opportunityObject], + flows: [flowWith({ nonexistent_field: 'x' }, { runAs: 'user' })], + }); + expect(findings).toEqual([]); + }); + + // ── shape robustness ───────────────────────────────────────────────── + it('returns [] for a stack with no flows', () => { + expect(validateReadonlyFlowWrites({ objects: [opportunityObject] })).toEqual([]); + expect(validateReadonlyFlowWrites({})).toEqual([]); + }); + + it('falls back to node id then index for the location label', () => { + const flow = { + name: 'f', + runAs: 'user', + nodes: [{ id: 'my_node', type: 'update_record', config: { objectName: 'crm_opportunity', fields: { approval_status: 'x' } } }], + edges: [], + }; + const findings = validateReadonlyFlowWrites({ objects: [opportunityObject], flows: [flow] }); + expect(findings[0].where).toBe('flow "f" › node "my_node"'); + expect(findings[0].path).toBe('flows[0].nodes[0].config.fields.approval_status'); + }); +}); diff --git a/packages/lint/src/validate-readonly-flow-writes.ts b/packages/lint/src/validate-readonly-flow-writes.ts new file mode 100644 index 000000000..284af14e3 --- /dev/null +++ b/packages/lint/src/validate-readonly-flow-writes.ts @@ -0,0 +1,197 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// Build-time guardrail: a flow `update_record` node that writes a field the +// target object declares `readonly: true`, under a non-system run identity, is +// a SILENT NO-OP. The objectql engine strips static-`readonly` fields from a +// non-system UPDATE payload (#2948), so the intended write never lands — yet +// the step still reports `success`. #3407/#3413 made that strip observable at +// RUN time (a step warning + `droppedFields`); this rule shifts the discovery +// LEFT to `os validate` / `os build`, so an author finds the mismatch at design +// time instead of by reading server WARN logs days later (#3425). +// +// Scope — deliberately narrow to keep it false-positive-free: +// +// • Only `update_record`. INSERT is engine-exempt from the readonly strip (a +// `create_record` may legitimately seed readonly columns; the ingress strip +// added in #3043 lives in metadata-protocol, which the flow engine bypasses +// 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 strip entirely, 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. +// +// • Static `readonly:true` + a LITERAL field name is a 100%-certain no-op → +// ERROR (gates the build). `readonlyWhen` is per-record-state — it strips +// only on records whose predicate is TRUE at run time, so it MAY silently +// not land → WARNING (advisory). A templated object name or a non-literal +// `fields` map is not statically knowable → skipped, no guess. +// +// A pure `(stack) => Finding[]` rule (ADR-0019): no I/O, no runtime. Shared by +// the CLI and any other consumer (AI authoring), so hand-authored and generated +// flows are held to the same bar. + +export type ReadonlyFlowWriteSeverity = 'error' | 'warning'; + +export interface ReadonlyFlowWriteFinding { + severity: ReadonlyFlowWriteSeverity; + rule: string; + /** Human-readable location, e.g. `flow "approve_deal" › node "Mark approved"`. */ + where: string; + /** Config path, e.g. `flows[0].nodes[3].config.fields.approval_status`. */ + path: string; + message: string; + hint: string; +} + +// Rule ids (registry entries). +export const FLOW_UPDATE_READONLY_FIELD = 'flow-update-readonly-field'; +export const FLOW_UPDATE_READONLY_WHEN_FIELD = 'flow-update-readonly-when-field'; + +type AnyRec = Record; + +/** Coerce an array-or-name-keyed-map collection to an array (name injected). */ +function asArray(v: unknown): AnyRec[] { + if (Array.isArray(v)) return v as AnyRec[]; + if (v && typeof v === 'object') { + return Object.entries(v as AnyRec).map(([name, def]) => ({ + name, + ...(def as AnyRec), + })); + } + return []; +} + +interface FieldReadonlyMeta { + /** Static `readonly: true`. */ + readonly: boolean; + /** A non-empty `readonlyWhen` predicate is declared. */ + readonlyWhen: boolean; +} + +/** + * object name → (field name → readonly metadata). Handles both `fields` shapes + * (array of `{name, readonly, readonlyWhen}` and name-keyed map). A field with + * neither flag is recorded as `{false, false}` so callers can distinguish a + * "known-writable field" from an "unknown field" (absent from the map). + */ +function buildReadonlyIndex(objects: AnyRec[]): Map> { + const idx = new Map>(); + for (const obj of objects) { + const name = typeof obj.name === 'string' ? obj.name : undefined; + if (!name) continue; + const fieldMap = new Map(); + const collect = (fieldName: string, def: AnyRec): void => { + const rw = def?.readonlyWhen; + const readonlyWhen = rw != null && !(typeof rw === 'string' && rw.trim() === ''); + fieldMap.set(fieldName, { readonly: def?.readonly === true, readonlyWhen }); + }; + const fields = obj.fields; + if (Array.isArray(fields)) { + for (const f of fields as AnyRec[]) { + const fn = (f as AnyRec)?.name; + if (typeof fn === 'string') collect(fn, f as AnyRec); + } + } else if (fields && typeof fields === 'object') { + for (const [fn, def] of Object.entries(fields as AnyRec)) collect(fn, def as AnyRec); + } + idx.set(name, fieldMap); + } + return idx; +} + +/** + * The target object of an `update_record` node, when statically knowable. Reads + * the canonical `objectName` and its historical `object` alias (the same pair + * `readAliasedConfig` resolves at run time). A templated value (contains `{`) is + * dynamic — return undefined so the node is skipped rather than guessed. + */ +function readLiteralObjectName(config: AnyRec): string | undefined { + const raw = config.objectName ?? config.object; + if (typeof raw !== 'string' || raw.includes('{')) return undefined; + return raw || undefined; +} + +/** + * Validate flow `update_record` writes against target-object readonly + * declarations. Pure and dependency-free; safe on pre- or post-parse stacks. + */ +export function validateReadonlyFlowWrites(stack: AnyRec): ReadonlyFlowWriteFinding[] { + const findings: ReadonlyFlowWriteFinding[] = []; + const flows = asArray(stack.flows); + if (flows.length === 0) return findings; + + const roIndex = buildReadonlyIndex(asArray(stack.objects)); + + 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; + const runAs = flow.runAs === 'user' || flow.runAs === 'system' ? flow.runAs : 'user'; + + const flowName = typeof flow.name === 'string' ? flow.name : `#${flowIndex}`; + const nodes = Array.isArray(flow.nodes) ? (flow.nodes as AnyRec[]) : []; + + nodes.forEach((node, nodeIndex) => { + if (node?.type !== 'update_record') return; + const config = (node.config ?? {}) as AnyRec; + + const objectName = readLiteralObjectName(config); + if (!objectName) return; // templated / dynamic object — not statically knowable + const fieldMap = roIndex.get(objectName); + if (!fieldMap) return; // object defined by another package — cannot judge its fields + + const fields = config.fields; + // A non-literal write map (templated string, spread, array) is not + // statically knowable — skip rather than guess. + if (!fields || typeof fields !== 'object' || Array.isArray(fields)) return; + + const nodeName = + typeof node.label === 'string' && node.label + ? node.label + : typeof node.id === 'string' && node.id + ? node.id + : `#${nodeIndex}`; + + for (const fieldName of Object.keys(fields as AnyRec)) { + const meta = fieldMap.get(fieldName); + if (!meta) continue; // unknown field — a form/field-layout lint concern, not this rule's + + if (meta.readonly) { + findings.push({ + severity: 'error', + rule: FLOW_UPDATE_READONLY_FIELD, + where: `flow "${flowName}" › node "${nodeName}"`, + path: `flows[${flowIndex}].nodes[${nodeIndex}].config.fields.${fieldName}`, + message: + `writes field '${fieldName}', which object '${objectName}' declares readonly:true. Under ` + + `runAs:'${runAs}' the engine silently strips readonly fields from the UPDATE payload (#2948), ` + + `so this write never lands — while the step still reports success.`, + hint: + `If automation is meant to maintain this field, declare the flow runAs:'system' (the intended ` + + `channel — readonly governs the end-user/API surface, not trusted system writers). Otherwise ` + + `remove '${fieldName}' from this update_record node.`, + }); + } else if (meta.readonlyWhen) { + findings.push({ + severity: 'warning', + rule: FLOW_UPDATE_READONLY_WHEN_FIELD, + where: `flow "${flowName}" › node "${nodeName}"`, + path: `flows[${flowIndex}].nodes[${nodeIndex}].config.fields.${fieldName}`, + message: + `writes field '${fieldName}', which object '${objectName}' declares readonlyWhen. On records ` + + `where that predicate is TRUE, a runAs:'${runAs}' UPDATE strips the field (#3042), so this ` + + `write may silently not land depending on the record's state.`, + hint: + `If automation must maintain this field regardless of record state, run the flow runAs:'system'. ` + + `Otherwise confirm this node only targets records whose readonlyWhen predicate is FALSE.`, + }); + } + } + }); + }); + + return findings; +} diff --git a/skills/objectstack-automation/SKILL.md b/skills/objectstack-automation/SKILL.md index d2b141cfc..adeb9d196 100644 --- a/skills/objectstack-automation/SKILL.md +++ b/skills/objectstack-automation/SKILL.md @@ -138,6 +138,18 @@ variables: [ > (a single call updates *every* matching row — no per-row loop needed). > `label` is **required** on the flow and on every node. +> **Writing a `readonly` field? Set `runAs: 'system'`.** `readonly: true` +> governs the end-user surface: under the default `runAs: 'user'`, the engine +> **silently strips** a `readonly` field from an `update_record` payload +> (#2948) — the step reports success but the value never lands. A flow that +> maintains a `readonly` field (approval stamps, conversion flags, SLA +> markers, rollups) must run `runAs: 'system'`, the trusted-writer channel. +> `os validate` / `os build` fail a `runAs:'user'` `update_record` that writes +> a `readonly` field, so the mismatch surfaces at build time, not as wrong data +> days later. (`readonlyWhen` fields are the same story, per record state — +> flagged as a warning.) Do **not** work around this by removing `readonly`; +> that loses the field's edit protection. + ```typescript { name: 'escalate_overdue_cases', diff --git a/skills/objectstack-data/SKILL.md b/skills/objectstack-data/SKILL.md index 75cf2a7f5..bafed90b1 100644 --- a/skills/objectstack-data/SKILL.md +++ b/skills/objectstack-data/SKILL.md @@ -186,6 +186,16 @@ export const Invoice = ObjectSchema.create({ - Use `visibleWhen` to hide irrelevant fields in ObjectUI forms. - Use `readonlyWhen` for state-locked fields; the ObjectQL write path ignores incoming changes when the predicate is `TRUE`. +- **`readonly: true` governs the end-user surface, not trusted system writers.** + A non-system write (REST/UI, and any `runAs:'user'` flow — the default) has + the field **silently stripped** from an UPDATE payload; the write reports + success but the value never lands (#2948). System-context writes — + `runAs:'system'` flows, system hooks, seeds, imports, migrations — are exempt + and DO write it. So the pattern "users can't edit this, but automation + maintains it" is expressed by declaring the field `readonly` **and** running + the maintaining flow `runAs:'system'` (see **objectstack-automation**), not by + removing `readonly`. Writing a `readonly` field from a `runAs:'user'` + `update_record` node is a build-time **error** (`os validate` / `os build`). - Use `requiredWhen` for conditional requiredness; the ObjectQL validator enforces it on submit. `conditionalRequired` is a deprecated compatibility alias, not the preferred authoring field.