From 33ca99a52d4b29f4b127a45bc6cb1c4b5a1092c4 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 6 Sep 2026 06:22:28 +0000 Subject: [PATCH 1/3] wip: widen readonly flow/hook rules to the create verb (static shape) Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01Vbw3RPgdtqesx4azk9SbW8 --- .changeset/lint-readonly-create-scan-gap.md | 22 +++ content/docs/automation/hook-bodies.mdx | 4 +- .../src/validate-flow-node-writes.test.ts | 11 +- .../src/validate-readonly-flow-writes.test.ts | 153 ++++++++++++++-- .../lint/src/validate-readonly-flow-writes.ts | 104 ++++++++--- .../src/validate-readonly-hook-writes.test.ts | 168 +++++++++++++++--- .../lint/src/validate-readonly-hook-writes.ts | 136 +++++++++----- 7 files changed, 484 insertions(+), 114 deletions(-) create mode 100644 .changeset/lint-readonly-create-scan-gap.md diff --git a/.changeset/lint-readonly-create-scan-gap.md b/.changeset/lint-readonly-create-scan-gap.md new file mode 100644 index 0000000000..1b763dc6b9 --- /dev/null +++ b/.changeset/lint-readonly-create-scan-gap.md @@ -0,0 +1,22 @@ +--- +"@objectstack/lint": minor +--- + +`flow-update-readonly-field` and `hook-api-update-readonly-field` now report a non-system **create** of a static-`readonly` field — a new **error**-severity finding that fails `os lint` / `os validate` / `os build` on a shape they used to accept. + +Both rules scanned only the update verb (`update_record`; `ctx.api…update()` / `.updateById()`) and justified the omission with the same sentence: INSERT is engine-exempt from the author-declared `readonly` strip, so a create that seeds a `readonly` column is not a no-op. The maintainer ruling of 2026-09-03 (option C, #14147) made that false — `engine.insert` now runs the same `isSystem`-gated `stripReadonlyFields` the update path runs — so a flow `create_record` without `runAs: 'system'`, or a hook body's `ctx.api.object('…').insert()` under a non-system trigger, that writes a `readonly` field became a **silent no-op**: the row lands without the column (which falls back to its `defaultValue`), the step reports `success`, and only a run-time warning names the dropped field (measured end to end in `@objectstack/service-automation`'s `create-record-readonly-drop.test.ts`). Nothing reported it at build time. This closes that scan gap (#15394). + +**What now fails that passed before.** Exactly one new shape per rule, at `error`: + +- a flow `create_record` node whose literal `fields` map writes a field the target object declares `readonly: true`, on a flow that does not declare `runAs: 'system'`; +- an L2 hook body's literal `ctx.api.object('').insert({ … })` writing such a field, on a hook that does not declare `runAs: 'system'`. + +The rule ids and severities are the update ones — one id per shape, not per verb — and each finding's message names the verb it was judged on and what actually happens to a create. Everything the rules already skipped is still skipped: a templated object name, a non-literal payload, an object outside the stack or declaring no fields, an unknown field (the unknown-field rules' question), and any `runAs: 'system'` flow or hook, because seeding a `readonly` column at create time is a system act and that write lands. + +**Deliberately not reported.** + +- No `readonlyWhen` (conditional) finding on a create, on either surface: a conditional lock is evaluated against the record being written over, which a create does not have, and the engine runs no conditional strip on INSERT ("INSERT stays exempt"). A warning there would state something false about a write that lands. +- The hook rule judges `.insert()` only, not `.create()`. The host `ObjectRepository` aliases `create()` to `insert()`, but L2 bodies run in QuickJS and the VM-side `ctx.api.object()` installs no `create` leaf — a body calling `.create()` throws `TypeError: not a function` on its first run, a loud failure rather than the silent drop this rule reports. The silence is recorded as a reasoned method exclusion (`READONLY_HOOK_METHOD_EXCLUSIONS`) and pinned. +- `validate-readonly-action-writes` is unchanged: an action body runs system-elevated by design, so its create genuinely lands. + +**Migration.** If your build reds on the new finding, the fix is one of: declare `runAs: 'system'` on the flow or hook when seeding the `readonly` column is the intent (the intended channel — `readonly` governs the end-user/API surface, not trusted system writers); remove the key from the `create_record` `fields` / `insert()` payload when it is not; or stamp it in a `beforeInsert` hook on the target object (`ctx.input. = …`), which is a server value the strip does not touch. Measured over this repository's shipped examples (`app-crm`, `app-showcase`, `app-todo`): zero in-repo flows or hooks go red — the two `create_record` nodes that target an object carrying a `readonly` field write none of its `readonly` fields, and the one flow that creates unauthenticated already declares `runAs: 'system'`; no shipped hook body inserts through `ctx.api`. diff --git a/content/docs/automation/hook-bodies.mdx b/content/docs/automation/hook-bodies.mdx index 1983282f55..50226a8c2c 100644 --- a/content/docs/automation/hook-bodies.mdx +++ b/content/docs/automation/hook-bodies.mdx @@ -265,10 +265,10 @@ There is an asymmetry here that costs data if you learn it the hard way, so lear The dropped case is the dangerous one: nothing fails, the step reports success, and the column is simply always null. Because both halves of that judgement are declared in your own stack, it is checked at author time and **gates the build**: -- `hook-api-update-readonly-field` — **error**. A body's literal `ctx.api.object('…').update()` / `.updateById()` writes a field the named object declares `readonly: true`. +- `hook-api-update-readonly-field` — **error**. A body's literal `ctx.api.object('…').update()` / `.updateById()` / `.insert()` writes a field the named object declares `readonly: true`. Since [#15394](https://github.com/objectstack-ai/objectstack/issues/15394) the `insert` row of the table above is reported at build time exactly like the `update` row — same id, same severity, a message naming the verb — unless the hook declares `runAs: 'system'`. Only the static shape is judged on an insert: a `readonlyWhen` field has no prior record to lock on and the engine runs no conditional strip on INSERT, so no warning is produced there. - `hook-api-update-readonly-when-field` — **warning**. The same write against a `readonlyWhen` field, which strips per record *state*. The own-hook stamp **is** the workaround here, exactly as it is for static `readonly`: since [#9107](https://github.com/objectstack-ai/objectstack/issues/9107) the conditional strip judges the *caller's* entry payload, so a value a `beforeUpdate` hook **derives** is not caller-supplied and lands even on a locked record. (Deriving is the operative word — a hook that merely echoes the caller's own value back has written nothing the strip can tell from the caller's, and it still goes.) What does **not** help is elevation: unlike the static strip, the conditional lock is **not** waived by a system context, so neither `runAs: 'system'` nor the `sudo()` a body cannot reach makes a caller-supplied value survive. On this shape, confirm the write only targets records whose predicate is `false`, or derive the field in a `beforeUpdate` hook on the target object. -Only literal object names and literal payload keys are seen; a `sudo()` chain, a dynamic object name and an object this stack does not declare are all skipped, so the rule has no opinion on them. `insert`/`create` are skipped too — but since the 2026-09-03 ruling that is a **scan gap**, not an exemption: the write is dropped exactly as the table says, and nothing reports it at build time yet ([#15394](https://github.com/objectstack-ai/objectstack/issues/15394)). The flow surface has carried the same gate as `flow-update-readonly-field` since [#3425](https://github.com/objectstack-ai/objectstack/issues/3425), with the same gap on `create_record`. +Only literal object names and literal payload keys are seen; a `sudo()` chain, a dynamic object name and an object this stack does not declare are all skipped, so the rule has no opinion on them. `.create()` is skipped too, for a reason about the **sandbox** rather than the engine: the VM-side `ctx.api.object()` installs `insert` / `update` / `delete` / `updateMany` / `deleteMany` / `upsert` and no `create` leaf, so a body calling `.create()` throws `TypeError: not a function` on its first run — a loud failure, not a silent drop — and the same payload spelled `.insert()` is what the rule judges. The flow surface has carried the same gate as `flow-update-readonly-field` since [#3425](https://github.com/objectstack-ai/objectstack/issues/3425), and since [#15394](https://github.com/objectstack-ai/objectstack/issues/15394) it reports a non-`runAs: 'system'` `create_record` node's static-`readonly` write at the same **error**, again with no conditional finding on a create. The table above is about a **hook** body. An **action** body is the one surface where the answer changes, so read this before you move a body from one to the other: an action body runs **elevated** — its `ctx.api` is built over the caller's envelope with `isSystem` set, which is the same trusted posture that lets an action bypass row and field permissions — and the static strip applies only to non-system callers. So `ctx.api.object('x').update({ someReadonlyField })` **lands** in an action, and there is no finding for it. Elevation does not waive the *conditional* lock, though, so that half does carry across: `action-api-update-readonly-when-field` — a **warning** — on an action body's literal `ctx.api` update to a `readonlyWhen` field ([#13770](https://github.com/objectstack-ai/objectstack/issues/13770)). Net effect when you move a body: a `readonly` write changes behaviour, a `readonlyWhen` write does not. diff --git a/packages/lint/src/validate-flow-node-writes.test.ts b/packages/lint/src/validate-flow-node-writes.test.ts index 7844e5a237..98ed2a1ad7 100644 --- a/packages/lint/src/validate-flow-node-writes.test.ts +++ b/packages/lint/src/validate-flow-node-writes.test.ts @@ -407,11 +407,12 @@ describe('validateFlowNodeWrites', () => { }); it('does NOT flag a readonly field on create_record — this rule asks whether the column EXISTS, not whether the write lands', () => { - // The readonly sibling does not scan create_record today — a scan gap - // (#15394) since the 2026-09-03 ruling put the static-`readonly` strip - // inside `engine.insert` (#14147), not an exemption. This rule asks a - // different question either way, so a DECLARED readonly field is clean here - // for its own reason: it resolves to a column. + // Since #15394 the readonly sibling (`flow-update-readonly-field`) DOES + // report this exact write — a non-system create_record of a static- + // readonly field is stripped inside `engine.insert` since the 2026-09-03 + // ruling (#14147). This rule asks a different question, so a DECLARED + // readonly field is clean here for its own reason: it resolves to a + // column. The two never double-report one key. const withReadonly = { name: 'deal', fields: { stage: { type: 'text' }, approval_status: { type: 'text', readonly: true } }, diff --git a/packages/lint/src/validate-readonly-flow-writes.test.ts b/packages/lint/src/validate-readonly-flow-writes.test.ts index 09ce288e72..c6989c8ac2 100644 --- a/packages/lint/src/validate-readonly-flow-writes.test.ts +++ b/packages/lint/src/validate-readonly-flow-writes.test.ts @@ -326,30 +326,147 @@ describe('validateReadonlyFlowWrites', () => { expect(validateReadonlyFlowWrites({ objects: [opportunityObject], flows: [flow] })).toEqual([]); }); - // ── clean TODAY: create_record is a SCAN GAP, not an exemption (#15394) ── - // [#14147] This case used to be justified by "create_record is engine-exempt - // from the readonly strip". The maintainer ruling of 2026-09-03 (option C) - // SUPERSEDED that row: `engine.insert` runs the static-`readonly` strip for a - // non-system caller, and a `create_record` node without `runAs:'system'` is - // exactly that caller — the write below is a silent no-op at run time - // (measured end to end in `create-record-readonly-drop.test.ts`, - // service-automation). The verdict is kept because it is still TRUE of what - // this rule scans (`update_record` only); the reason is that the scan gap is - // filed as #15394, not that the engine exempts anything. When that lands, - // this case flips to RED with severity `error`. - it('does NOT flag create_record writing a readonly field — the #15394 scan gap, not an exemption', () => { - const flow = { + // ── create_record: the STATIC shape is the same certain no-op (#15394) ── + // + // This block used to be a GREEN control justified by "create_record is + // engine-exempt from the readonly strip", then (after #14147) a GREEN control + // that named itself a scan gap. The maintainer ruling of 2026-09-03 (option C) + // put `stripReadonlyFields` inside `engine.insert` for a non-system caller, + // and a `create_record` node without `runAs:'system'` is exactly that caller: + // the row lands WITHOUT the column and the step reports success (measured end + // to end in service-automation's `create-record-readonly-drop.test.ts`). So + // the case FLIPS to red here, at the severity the static shape carries on + // `update_record` — and its conditional-shape twins below stay green, because + // the engine still runs no `readonlyWhen` strip on INSERT. + describe('create_record', () => { + const createFlow = (fields: Record, flowOverrides: Record = {}, config: Record = {}) => ({ 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' } } }, + { id: 'c', type: 'create_record', label: 'Create', config: { objectName: 'crm_opportunity', fields, ...config } }, ], edges: [], - }; - const findings = validateReadonlyFlowWrites({ objects: [opportunityObject], flows: [flow] }); - expect(findings).toEqual([]); + ...flowOverrides, + }); + + it('errors when a runAs:user create_record writes a static-readonly field — flipped from the #15394 GREEN control', () => { + const findings = validateReadonlyFlowWrites({ objects: [opportunityObject], flows: [createFlow({ 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].where).toBe('flow "seed_opp" › node "Create"'); + // The message states the verb it was judged on and what actually happens + // — the row is created without the column — not the update sentence. + expect(findings[0].message).toContain('INSERT'); + expect(findings[0].message).toContain('created WITHOUT this column'); + expect(findings[0].message).toContain('#14147'); + expect(findings[0].message).not.toContain('UPDATE payload (#2948)'); + // The remedy names the create verb, the system channel and the own-object + // beforeInsert stamp. + expect(findings[0].hint).toContain("runAs:'system'"); + expect(findings[0].hint).toContain('create_record'); + expect(findings[0].hint).toContain('beforeInsert'); + }); + + it('errors when runAs is unauthored (defaults to user) — same as update_record', () => { + const findings = validateReadonlyFlowWrites({ objects: [opportunityObject], flows: [createFlow({ approval_status: 'approved' })] }); + expect(findings).toHaveLength(1); + expect(findings[0].severity).toBe('error'); + expect(findings[0].rule).toBe(FLOW_UPDATE_READONLY_FIELD); + }); + + it('resolves the create target via the `object` alias, exactly like update_record', () => { + const findings = validateReadonlyFlowWrites({ + objects: [opportunityObject], + flows: [createFlow({ approval_status: 'approved' }, { runAs: 'user' }, { objectName: undefined, object: 'crm_opportunity' })], + }); + expect(findings).toHaveLength(1); + expect(findings[0].rule).toBe(FLOW_UPDATE_READONLY_FIELD); + }); + + // The elevation exemption gates the static branch on create exactly as on + // update: a `runAs:'system'` create legitimately SEEDS a readonly column + // (the runtime measurement's second case: "seeding a readonly column at + // create time is a SYSTEM act"). + it('does NOT flag a runAs:system create_record writing a static-readonly field (the intended seeding channel)', () => { + expect(validateReadonlyFlowWrites({ objects: [opportunityObject], flows: [createFlow({ approval_status: 'approved' }, { runAs: 'system' })] })).toEqual([]); + }); + + // ⛔ No conditional finding on a create: the engine runs no `readonlyWhen` + // strip on INSERT ("INSERT stays exempt", engine.ts), so a warning here + // would state something false about a write that lands. The control is the + // same payload on an update_record, which does draw the warning. + it('does NOT warn on a runAs:user create_record writing a readonlyWhen field — INSERT stays exempt from the conditional strip', () => { + expect(validateReadonlyFlowWrites({ objects: [opportunityObject], flows: [createFlow({ amount: 10 }, { runAs: 'user' })] })).toEqual([]); + const control = validateReadonlyFlowWrites({ objects: [opportunityObject], flows: [flowWith({ amount: 10 }, { runAs: 'user' })] }); + expect(control).toHaveLength(1); + expect(control[0].rule).toBe(FLOW_UPDATE_READONLY_WHEN_FIELD); + }); + + it('separates readonly (error) + readonlyWhen (silent) + plain (clean) in one create_record', () => { + const findings = validateReadonlyFlowWrites({ + objects: [opportunityObject], + flows: [createFlow({ approval_status: 'approved', amount: 10, notes: 'hi' }, { runAs: 'user' })], + }); + expect(findings).toHaveLength(1); + expect(findings[0].rule).toBe(FLOW_UPDATE_READONLY_FIELD); + expect(findings[0].path).toBe('flows[0].nodes[1].config.fields.approval_status'); + }); + + // Where create and update DIFFER under elevation: a field declaring BOTH + // flags falls through to the conditional warning on an elevated update + // (the strip that is not waived), but is clean on an elevated create — no + // static strip (elevation) and no conditional strip (INSERT) apply. + it('is clean for a field declaring readonly AND readonlyWhen under runAs:system — unlike the same update_record', () => { + const bothFlags = { + name: 'crm_opportunity', + fields: { approval_status: { type: 'text', readonly: true, readonlyWhen: "record.stage == 'closed_won'" } }, + }; + expect(validateReadonlyFlowWrites({ objects: [bothFlags], flows: [createFlow({ approval_status: 'approved' }, { runAs: 'system' })] })).toEqual([]); + const updateControl = validateReadonlyFlowWrites({ objects: [bothFlags], flows: [flowWith({ approval_status: 'approved' }, { runAs: 'system' })] }); + expect(updateControl).toHaveLength(1); + expect(updateControl[0].rule).toBe(FLOW_UPDATE_READONLY_WHEN_FIELD); + // ...and under a user run the static error outranks, on both verbs. + const userCreate = validateReadonlyFlowWrites({ objects: [bothFlags], flows: [createFlow({ approval_status: 'approved' }, { runAs: 'user' })] }); + expect(userCreate).toHaveLength(1); + expect(userCreate[0].rule).toBe(FLOW_UPDATE_READONLY_FIELD); + }); + + it('reaches a create_record nested in a loop body', () => { + const flow = { + name: 'fan_out', + runAs: 'user', + nodes: [ + { + id: 'each', + type: 'loop', + label: 'Each', + config: { + collection: '{items}', + body: { + nodes: [ + { id: 'c', type: 'create_record', label: 'C', config: { objectName: 'crm_opportunity', fields: { approval_status: 'approved' } } }, + ], + edges: [], + }, + }, + }, + ], + edges: [], + }; + const findings = validateReadonlyFlowWrites({ objects: [opportunityObject], flows: [flow] }); + expect(findings).toHaveLength(1); + expect(findings[0].severity).toBe('error'); + expect(findings[0].path).toBe('flows[0].nodes[0].config.body.nodes[0].config.fields.approval_status'); + expect(findings[0].where).toBe('flow "fan_out" › loop "Each" › body › node "C"'); + }); + + it('skips a templated objectName and a non-literal fields map on create, as on update', () => { + expect(validateReadonlyFlowWrites({ objects: [opportunityObject], flows: [createFlow({ approval_status: 'x' }, { runAs: 'user' }, { objectName: '{target}' })] })).toEqual([]); + expect(validateReadonlyFlowWrites({ objects: [opportunityObject], flows: [createFlow('{payload}' as unknown as Record, { runAs: 'user' })] })).toEqual([]); + }); }); // ── clean: plain writable field ────────────────────────────────────── diff --git a/packages/lint/src/validate-readonly-flow-writes.ts b/packages/lint/src/validate-readonly-flow-writes.ts index cf57452589..890b5b04ba 100644 --- a/packages/lint/src/validate-readonly-flow-writes.ts +++ b/packages/lint/src/validate-readonly-flow-writes.ts @@ -11,19 +11,26 @@ // // Scope — deliberately narrow to keep it false-positive-free: // -// • Only `update_record`, and ⚠️ this bullet's REASON is spent. It used to -// be that INSERT was engine-exempt from the author-declared static-`readonly` -// strip (#3043/#3413: "a `create_record` may legitimately seed readonly -// columns", with an ingress copy in metadata-protocol that the flow engine -// bypassed by calling the data engine directly). The maintainer ruling of -// 2026-09-03 (option C, #14147) SUPERSEDED that row: `engine.insert` runs -// the static strip for a non-system caller, the ingress copy is deleted, -// and a `create_record` node without `runAs:'system'` is exactly such a -// caller. So a create writing a readonly field IS a silent no-op now, and -// this rule does not yet report it — a scan gap, not a decision, recorded -// here and filed rather than widened inside #14147's PR (a new -// error-severity finding class is its own change). The hook sibling's -// `insert`/`create` gap rests on the same superseded premise. +// • `update_record` AND `create_record` — the two nodes whose `fields` map +// is a caller-supplied write payload. The create verb used to be excluded +// because INSERT was engine-exempt from the author-declared static- +// `readonly` strip (#3043/#3413: "a `create_record` may legitimately seed +// readonly columns", with an ingress copy in metadata-protocol that the +// flow engine bypassed by calling the data engine directly). The maintainer +// ruling of 2026-09-03 (option C, #14147) SUPERSEDED that row: `engine.insert` +// runs the SAME `stripReadonlyFields` the update path runs, under the same +// `isSystem` gate, and a `create_record` node without `runAs:'system'` is +// exactly such a caller — the row lands WITHOUT the column and the step +// still reports `success` (measured end to end in service-automation's +// `create-record-readonly-drop.test.ts`). So the STATIC branch below judges +// both verbs alike (#15394 closed the scan gap #14147 left open). +// +// ⚠️ The CONDITIONAL branch stays `update_record`-only, on purpose: a +// `readonlyWhen` predicate is evaluated against the record being written +// over, which a create does not have, and `engine.ts` says so at the strip +// ("INSERT stays exempt" — `stripReadonlyWhenFields` is update-path-only). +// A `readonlyWhen` finding on a `create_record` would state something false +// about a write that lands, so none is ever produced. // // • `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 @@ -72,10 +79,27 @@ export interface ReadonlyFlowWriteFinding { hint: string; } -// Rule ids (registry entries). +// Rule ids (registry entries). One id per SHAPE, not per verb: since #15394 +// `flow-update-readonly-field` covers the static shape on `create_record` too, +// because the finding is the same fact (a caller-supplied write to a declared- +// readonly field the engine strips) and the same strip — the message names the +// verb it was judged on, and a second id would only split one finding's +// suppression, docs and counts in two. export const FLOW_UPDATE_READONLY_FIELD = 'flow-update-readonly-field'; export const FLOW_UPDATE_READONLY_WHEN_FIELD = 'flow-update-readonly-when-field'; +/** The node type whose payload the STATIC branch alone judges (#15394). */ +const CREATE_NODE_TYPE = 'create_record'; +/** The node type both branches judge. */ +const UPDATE_NODE_TYPE = 'update_record'; +/** + * Flow nodes whose `config.fields` is a caller-supplied write payload the engine + * runs `stripReadonlyFields` over. Declared as data so a third CRUD verb cannot + * land silently in a branch never written for it. + */ +export const READONLY_FLOW_WRITE_NODE_TYPES: readonly string[] = [UPDATE_NODE_TYPE, CREATE_NODE_TYPE]; +const WRITE_NODE_TYPES: ReadonlySet = new Set(READONLY_FLOW_WRITE_NODE_TYPES); + type AnyRec = Record; export interface FieldReadonlyMeta { @@ -126,7 +150,11 @@ export function buildReadonlyIndex(objects: AnyRec[]): Map { - if (node?.type !== 'update_record') return; + const nodeType = node?.type; + if (typeof nodeType !== 'string' || !WRITE_NODE_TYPES.has(nodeType)) return; + // A create has no prior record, so only the STATIC branch applies to it + // (the header's second bullet); the verb also reaches the message. + const isCreate = nodeType === CREATE_NODE_TYPE; const config = (node.config ?? {}) as AnyRec; const objectName = readLiteralObjectName(config); @@ -205,16 +238,33 @@ export function validateReadonlyFlowWrites(stack: AnyRec): ReadonlyFlowWriteFind rule: FLOW_UPDATE_READONLY_FIELD, where, path: `${nodePath}.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.`, + message: isCreate + ? // The create-side strip is the 2026-09-03 ruling (#14147): the + // same `stripReadonlyFields`, now run by `engine.insert` too. + `writes field '${fieldName}', which object '${objectName}' declares readonly:true. Under ` + + `runAs:'${runAs}' the engine silently strips readonly fields from the INSERT payload too ` + + `(#14147 — the same strip the UPDATE path runs), so the row is created WITHOUT this column ` + + `(it falls back to the field's defaultValue) — while the create_record step still reports ` + + `success, with only a run-time warning naming the dropped field.` + : `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: isCreate + ? `Seeding a readonly column at create time is a SYSTEM act: declare the flow runAs:'system' ` + + `(the intended channel — readonly governs the end-user/API surface, not trusted system ` + + `writers). Otherwise remove '${fieldName}' from this create_record node, or stamp it in a ` + + `beforeInsert hook on '${objectName}' — a hook-assigned key is the hook's write, not a ` + + `caller-supplied one, and survives the strip.` + : `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) { + } else if (meta.readonlyWhen && !isCreate) { + // `!isCreate`: the conditional lock has no prior record to evaluate on + // an insert and the engine does not run it there ("INSERT stays + // exempt"), so a create is judged on the static branch alone — under + // `runAs:'system'` that means a create writing BOTH kinds is clean, + // where the same update would still draw the conditional warning. findings.push({ severity: 'warning', rule: FLOW_UPDATE_READONLY_WHEN_FIELD, diff --git a/packages/lint/src/validate-readonly-hook-writes.test.ts b/packages/lint/src/validate-readonly-hook-writes.test.ts index 41ce72bc13..e6cd7b9197 100644 --- a/packages/lint/src/validate-readonly-hook-writes.test.ts +++ b/packages/lint/src/validate-readonly-hook-writes.test.ts @@ -17,6 +17,8 @@ import { HOOK_API_UPDATE_READONLY_WHEN_FIELD, READONLY_HOOK_WRITE_PATTERN_IDS, READONLY_HOOK_WRITE_EXCLUSIONS, + READONLY_HOOK_STRIP_SUBJECT_METHODS, + READONLY_HOOK_METHOD_EXCLUSIONS, } from './validate-readonly-hook-writes.js'; /** @@ -212,36 +214,162 @@ describe('validateReadonlyHookWrites - GREEN: the elevated channel', () => { }); }); -describe('validateReadonlyHookWrites - GREEN today: insert()/create() are a SCAN GAP, not an exemption', () => { - // [#14147] This block used to be titled "INSERT is engine-exempt" and rested - // on exactly that sentence ("a create may legitimately seed read-only - // columns", #3043/#3413). The maintainer ruling of 2026-09-03 (option C) - // SUPERSEDED it: `engine.insert` now runs the static-`readonly` strip for a - // non-system caller, `isSystem`-gated, exactly as `engine.update` does — and - // a hook body's `ctx.api` under a non-system trigger IS such a caller. A green - // case whose justification has been overturned is indistinguishable from a - // scan gap, so the verdicts below are kept — they are still TRUE of what this - // rule scans (`update` / `updateById` only) — and the reason is restated: - // - the CONDITIONAL lock has no prior record to evaluate on a create, so - // `hook-api-update-readonly-when-field` is right to stay silent; - // - the STATIC half IS a silent no-op now and is NOT reported — a scan gap, - // filed as #15394. When that lands, `insert()` of a static-`readonly` - // column flips to RED here, and this block's title goes with it. - it('never flags insert() — the conditional lock has no prior record on a create; the static half is the #15394 gap', () => { - expect( - validateReadonlyHookWrites( - crmStack("await ctx.api.object('crm_account').insert({ last_activity_date: now });"), +describe('validateReadonlyHookWrites - RED: insert() of a static-readonly field is the same certain no-op (#15394)', () => { + // This block used to be titled "INSERT is engine-exempt" and rested on + // exactly that sentence ("a create may legitimately seed read-only columns", + // #3043/#3413); after #14147 it was a GREEN control that named itself a scan + // gap. The maintainer ruling of 2026-09-03 (option C) put the static- + // `readonly` strip inside `engine.insert` for a non-system caller, + // `isSystem`-gated, exactly as `engine.update` runs it — and a hook body's + // `ctx.api` under a non-system trigger IS such a caller. So the `insert()` + // case FLIPS to red here, at the severity the static shape carries on + // `update()`. Two things stay green, each for its own measured reason: + // - the CONDITIONAL lock has no prior record to evaluate on a create and + // the engine runs no `readonlyWhen` strip on INSERT ("INSERT stays + // exempt"), so `hook-api-update-readonly-when-field` stays silent; + // - `create()` is not a subject: the QuickJS `ctx.api.object()` installs no + // `create` leaf, so a body's `.create()` is a TypeError on its first run + // — loud, not silent — and a "silently dropped" finding would be false. + it('flags insert() writing a static-readonly field — flipped from the #15394 GREEN control', () => { + const findings = validateReadonlyHookWrites( + crmStack("await ctx.api.object('crm_account').insert({ last_activity_date: now });"), + ); + expect(findings).toHaveLength(1); + expect(findings[0].rule).toBe(HOOK_API_UPDATE_READONLY_FIELD); + expect(findings[0].severity).toBe('error'); + expect(findings[0].where).toBe('hook "touch_account" > body'); + expect(findings[0].path).toBe('hooks[0].body.source'); + expect(findings[0].message).toContain("'last_activity_date'"); + expect(findings[0].message).toContain("ctx.api.object('crm_account').insert(...)"); + // The message says what actually happens to a create — the row is made + // without the column — and names the verb; it is not the update sentence. + expect(findings[0].message).toContain('INSERT payload'); + expect(findings[0].message).toContain('created WITHOUT this column'); + // The remedy names the declared elevation knob and the own-object + // beforeInsert stamp, and keeps refusing sudo() for the #14010 reason. + expect(findings[0].hint).toContain("runAs: 'system'"); + expect(findings[0].hint).toContain('beforeInsert'); + expect(findings[0].hint).toContain('ctx.input.last_activity_date'); + expect(findings[0].hint).toMatch(/sudo\(\) is NOT an option|not marshalled into the sandbox/); + expect(findings[0].hint).not.toMatch(/make the elevation explicit|write it through ctx\.api\.sudo/); + }); + + it('flags each distinct readonly field once across insert() and update() of the same object', () => { + const findings = validateReadonlyHookWrites( + crmStack( + "await ctx.api.object('crm_account').insert({ last_activity_date: now }); " + + "await ctx.api.object('crm_account').update({ id: accountId, last_activity_date: now });", ), + ); + expect(findings).toHaveLength(1); + }); + + it('reports a field carrying BOTH flags as the certain (static readonly) finding on insert()', () => { + const findings = validateReadonlyHookWrites({ + objects: [ + { name: 'crm_account', fields: { locked: { type: 'boolean', readonly: true, readonlyWhen: 'status == "closed"' } } }, + ], + hooks: [ + { + name: 'touch', + object: 'crm_case', + events: ['afterInsert'], + body: { language: 'js', source: "await ctx.api.object('crm_account').insert({ locked: true });" }, + }, + ], + }); + expect(findings).toHaveLength(1); + expect(findings[0].rule).toBe(HOOK_API_UPDATE_READONLY_FIELD); + expect(findings[0].severity).toBe('error'); + }); + + // `id` is excluded as the row ADDRESS only on the payload-addressed UPDATE + // (#8141). On an insert the payload has no address, so a caller-supplied `id` + // is a field write like any other — judged on the object's own declaration, + // which the reference stack does not make, so it goes to the unknown-field + // rule; this pins that the address exclusion did not widen to insert. + it('does not treat `id` in an insert payload as an address — it is judged like any field', () => { + const stack = { + objects: [{ name: 'crm_account', fields: { id: { type: 'text', readonly: true }, name: { type: 'text' } } }], + hooks: [ + { + name: 'touch', + object: 'crm_case', + events: ['afterInsert'], + body: { language: 'js', source: "await ctx.api.object('crm_account').insert({ id: 'x', name: 'n' });" }, + }, + ], + }; + const findings = validateReadonlyHookWrites(stack); + expect(findings).toHaveLength(1); + expect(findings[0].message).toContain("'id'"); + // The control: the same key on an update() IS the address, and stays silent. + const update = { ...stack, hooks: [{ ...stack.hooks[0], body: { language: 'js', source: "await ctx.api.object('crm_account').update({ id: 'x', name: 'n' });" } }] }; + expect(validateReadonlyHookWrites(update)).toEqual([]); + }); + + it("skips a hook that declares runAs: 'system' on insert() — seeding a readonly column at create is a SYSTEM act", () => { + const stack = crmStack("await ctx.api.object('crm_account').insert({ last_activity_date: now });"); + const elevated = { ...stack, hooks: (stack.hooks as any[]).map((h) => ({ ...h, runAs: 'system' })) }; + expect(validateReadonlyHookWrites(elevated)).toEqual([]); + // ...and the control, so the skip is attributable to the declaration alone. + expect(validateReadonlyHookWrites(stack)).toHaveLength(1); + }); +}); + +describe('validateReadonlyHookWrites - GREEN: what a create is NOT judged on', () => { + // ⛔ No conditional finding on an insert: `stripReadonlyWhenFields` needs the + // record being written over, and engine.ts says "INSERT stays exempt" at that + // strip. A warning here would state something false about a write that + // lands. The control is the same payload through update(), which does warn. + it('never warns on insert() of a readonlyWhen field — INSERT stays exempt from the conditional strip', () => { + expect( + validateReadonlyHookWrites(crmStack("await ctx.api.object('crm_account').insert({ credit_hold: true });")), ).toEqual([]); + const control = validateReadonlyHookWrites( + crmStack("await ctx.api.object('crm_account').update({ credit_hold: true });"), + ); + expect(control).toHaveLength(1); + expect(control[0].rule).toBe(HOOK_API_UPDATE_READONLY_WHEN_FIELD); }); - it('never flags create() — same two facts, same gap', () => { + // `create()` exists on the HOST ObjectRepository as an alias of insert(), but + // this rule reads L2 bodies, which run in QuickJS, and the VM-side + // `ctx.api.object()` installs exactly insert / update / delete / updateMany / + // deleteMany / upsert (`installCtx`, runtime/src/sandbox/quickjs-runner.ts; + // the runner's test pins its ctx.api surface exhaustively). A body calling + // `.create()` therefore throws `TypeError: not a function` on its first run — + // a loud failure, not the silent no-op this rule reports — so a finding would + // be false in exactly the way the sudo() hint used to be (#14010). + it('never flags create() — the sandbox has no such leaf, so the call throws rather than silently dropping', () => { expect( validateReadonlyHookWrites( crmStack("await ctx.api.object('crm_account').create({ last_activity_date: now });"), ), ).toEqual([]); }); + + it('records the create() silence as a reasoned method exclusion — never as "INSERT is exempt"', () => { + expect(READONLY_HOOK_METHOD_EXCLUSIONS.map((e) => e.method)).toEqual(['create']); + const [create] = READONLY_HOOK_METHOD_EXCLUSIONS; + expect(create.reason).toMatch(/QuickJS/); + expect(create.reason).toMatch(/TypeError: not a function/); + expect(create.reason).not.toMatch(/INSERT is exempt|engine exempts INSERT/i); + }); + + it('partitions the extractor\'s ctx.api write verbs exactly — every verb is a subject or a reasoned exclusion', () => { + // The extractor's `API_WRITE_METHODS` is module-local, so its verbs are + // read off the shared ledger's declared `syntax` line for the shape + // ("ctx.api.object('').insert({…}) | .create({…}) | …") instead + // of being restated here — a fifth verb added there fails this case until + // it is classified. + const apiPattern = HOOK_BODY_WRITE_PATTERNS.find((p) => p.id === 'api-crud-literal')!; + const verbs = [...apiPattern.syntax.matchAll(/\.(\w+)\(/g)].map((m) => m[1]).filter((v) => v !== 'object').sort(); + expect(verbs).toEqual(['create', 'insert', 'update', 'updateById']); + const classified = [...READONLY_HOOK_STRIP_SUBJECT_METHODS, ...READONLY_HOOK_METHOD_EXCLUSIONS.map((e) => e.method)].sort(); + expect(classified).toEqual(verbs); + expect(READONLY_HOOK_STRIP_SUBJECT_METHODS.filter((m) => READONLY_HOOK_METHOD_EXCLUSIONS.some((e) => e.method === m))).toEqual([]); + }); }); describe('validateReadonlyHookWrites - GREEN: nothing statically knowable is guessed', () => { diff --git a/packages/lint/src/validate-readonly-hook-writes.ts b/packages/lint/src/validate-readonly-hook-writes.ts index 475b640cf7..8210949e15 100644 --- a/packages/lint/src/validate-readonly-hook-writes.ts +++ b/packages/lint/src/validate-readonly-hook-writes.ts @@ -30,8 +30,8 @@ // - `ctx.input. = ...` / `Object.assign(ctx.input, ...)` -> the hook's // own in-flight payload, a server stamp, survives the strip -> NEVER // flagged. -// - `ctx.api.object('').update|updateById({ })` -> a fresh -// non-elevated operation whose payload IS caller-supplied -> flagged. +// - `ctx.api.object('').update|updateById|insert({ })` -> a +// fresh non-elevated operation whose payload IS caller-supplied -> flagged. // // A blanket "readonly field in any write set" rule would fail every correct // before-hook stamp in the corpus on day one and be switched off by the first @@ -39,17 +39,29 @@ // // --- SCOPE - deliberately narrow, so a finding is worth gating on ---------- // -// - Only `update` / `updateById`, and ⚠️ this bullet's REASON is spent. It -// used to be that INSERT was engine-exempt from the author-declared -// static-`readonly` strip (#3043/#3413: "a create may legitimately seed -// read-only columns"). The maintainer ruling of 2026-09-03 (option C, -// #14147) SUPERSEDED that row: `engine.insert` runs the static strip for a -// non-system caller, and a hook body's `ctx.api` under a non-system trigger -// is exactly that. So a hook `insert` of a static-`readonly` column IS a -// silent no-op now, and this rule does not yet report it — a scan gap, not -// a decision, recorded here and filed rather than widened inside #14147's -// PR (a new error-severity finding class is its own change). The flow -// sibling's `create_record` gap rests on the same superseded premise. +// - `update` / `updateById` on BOTH branches, `insert` on the STATIC branch +// only. `insert` used to be excluded because INSERT was engine-exempt from +// the author-declared static-`readonly` strip (#3043/#3413: "a create may +// legitimately seed read-only columns"). The maintainer ruling of +// 2026-09-03 (option C, #14147) SUPERSEDED that row: `engine.insert` runs +// the SAME `stripReadonlyFields` under the SAME `isSystem` gate, and a hook +// body's `ctx.api` under a non-system trigger is exactly that caller — the +// row is created WITHOUT the column while the call returns success. So +// since #15394 a non-system `insert` of a static-`readonly` column is the +// same `error` an `update` of it is. The conditional half stays +// update-only: `stripReadonlyWhenFields` needs the record being written +// over, and `engine.ts` says at the strip that "INSERT stays exempt". +// +// `create` is NOT a subject, for a reason that is about the SANDBOX, not +// the engine — see {@link READONLY_HOOK_METHOD_EXCLUSIONS}. The host +// `ObjectRepository` does alias `create()` to `insert()`, but this rule +// reads L2 bodies, which run in QuickJS, and the VM-side `ctx.api.object()` +// installs exactly `insert`/`update`/`delete`/`updateMany`/`deleteMany`/ +// `upsert` (`installCtx`, runtime/src/sandbox/quickjs-runner.ts) — so a +// body's `.create()` is `TypeError: not a function` at run time: a LOUD +// failure on the first run, not the silent no-op this rule exists to +// report. Gating it as a silent drop would state something false, the +// mirror of the `sudo()` hint defect below. // // - Only a NON-ELEVATED `ctx.api`. `ScopedContext.sudo()` returns a context // with `isSystem: true`, which the strip skips entirely. A `.sudo()` chain @@ -205,20 +217,44 @@ export const READONLY_HOOK_WRITE_EXCLUSIONS: readonly BodyWritePatternExclusion[ const APPLICABLE_PATTERN_IDS: ReadonlySet = new Set(READONLY_HOOK_WRITE_PATTERN_IDS); /** - * `ctx.api` write methods whose payload this rule judges against the strip. - * - * `insert` / `create` are absent as a SCAN GAP, not by decision. This docblock - * used to say the opposite — that the engine exempts INSERT from the - * author-declared static-`readonly` strip so a create may legitimately seed - * read-only columns (#3043/#3413). The maintainer ruling of 2026-09-03 - * (option C, #14147) superseded that row — the header above carries the full - * reading: `engine.insert` now runs the same strip under the same `isSystem` - * gate, so a non-system hook `insert` of a static-`readonly` column is a - * silent no-op this rule does not yet report. Widening the set is filed as its - * own change rather than ridden in here, and the flow sibling's `create_record` - * gap is the same finding one surface over. + * `ctx.api` write methods whose payload this rule judges against the STATIC + * strip. `insert` joined in #15394, once the 2026-09-03 ruling (option C, + * #14147) put `stripReadonlyFields` on the create path under the same + * `isSystem` gate — the header carries the full reading. Exported so the test + * can pin the partition against {@link READONLY_HOOK_METHOD_EXCLUSIONS}. + */ +export const READONLY_HOOK_STRIP_SUBJECT_METHODS: readonly string[] = ['update', 'updateById', 'insert']; +const STRIP_SUBJECT_METHODS: ReadonlySet = new Set(READONLY_HOOK_STRIP_SUBJECT_METHODS); + +/** + * The methods {@link READONLY_HOOK_STRIP_SUBJECT_METHODS} judges on the + * CONDITIONAL branch too. A create has no prior record for a `readonlyWhen` + * predicate to read, and the engine runs no conditional strip on INSERT + * ("INSERT stays exempt" at the bulk strip in engine.ts), so `insert` is + * judged on the static branch alone. */ -const STRIP_SUBJECT_METHODS: ReadonlySet = new Set(['update', 'updateById']); +const CONDITIONAL_SUBJECT_METHODS: ReadonlySet = new Set(['update', 'updateById']); + +/** + * `ctx.api.object()` write methods the shared extractor recognises + * (`API_WRITE_METHODS` in `validate-hook-body-writes.ts`) that this rule + * deliberately does NOT judge, each with its reason — the same discipline + * {@link READONLY_HOOK_WRITE_EXCLUSIONS} applies to pattern shapes. ⛔ A reason + * here may never be "INSERT is exempt": that sentence is false about the + * engine since the 2026-09-03 ruling. + */ +export const READONLY_HOOK_METHOD_EXCLUSIONS: readonly { method: string; reason: string }[] = [ + { + method: 'create', + reason: + 'this rule reads L2 bodies, which run in QuickJS, and the VM-side ctx.api.object() installs no ' + + '`create` leaf (installCtx in runtime/src/sandbox/quickjs-runner.ts: insert / update / delete / ' + + 'updateMany / deleteMany / upsert) - so a body calling .create() is `TypeError: not a function` on ' + + 'its first run, a LOUD failure, not the silent no-op this rule reports. The host ObjectRepository ' + + 'does alias create() to insert(), but no body reaches the host repository. A fact about the ' + + 'SANDBOX, not about INSERT: the same payload spelled .insert() IS judged', + }, +]; /** * Methods whose payload carries the row ADDRESS rather than only field data. @@ -312,6 +348,9 @@ export function validateReadonlyHookWrites(stack: AnyRec): ReadonlyHookWriteFind if (reported.has(dedupeKey)) continue; const call = `ctx.api.object('${objectName}').${method}(...)`; + // The static branch judges every subject method; the conditional one + // only those with a prior record to lock on. + const isCreate = !CONDITIONAL_SUBJECT_METHODS.has(method); if (meta.readonly) { reported.add(dedupeKey); @@ -320,23 +359,36 @@ export function validateReadonlyHookWrites(stack: AnyRec): ReadonlyHookWriteFind rule: HOOK_API_UPDATE_READONLY_FIELD, where, path, - // The static-`readonly` write-path strip is #2948; the id stays here, - // out of the message an author reads and cannot resolve. - message: - `body writes field '${w.field}' through ${call}, and object '${objectName}' declares it ` + - `readonly:true. A hook's ctx.api is a ScopedContext over the TRIGGERING operation's context, so ` + - `on every non-system trigger the engine strips readonly keys from that UPDATE payload - ` + - `the write never lands, while the call still returns success.`, - hint: - `If automation is meant to maintain '${w.field}', stamp it on the record's OWN hook - ` + - `ctx.input.${w.field} = ... in beforeInsert/beforeUpdate survives the strip, and is the ` + - `recommended shape. To keep writing it CROSS-OBJECT from here, declare runAs: 'system' on ` + - `this hook: the strip skips a system context, so the write lands, and the triggering user ` + - `is still stamped on the record. Note that ctx.api.sudo() is NOT an option from a body: ` + - `sudo() lives on the in-process ScopedContext and is not marshalled into the sandbox, so ` + - `calling it here is a TypeError at run time. Otherwise drop readonly:true from '${w.field}'.`, + // The static-`readonly` write-path strip is #2948 on UPDATE and, since + // the 2026-09-03 ruling, #14147 on INSERT; the ids stay here, out of + // the message an author reads and cannot resolve. + message: isCreate + ? `body writes field '${w.field}' through ${call}, and object '${objectName}' declares it ` + + `readonly:true. A hook's ctx.api is a ScopedContext over the TRIGGERING operation's context, so ` + + `on every non-system trigger the engine strips readonly keys from that INSERT payload exactly ` + + `as it does from an UPDATE - the row is created WITHOUT this column (it falls back to the ` + + `field's defaultValue), while the call still returns success.` + : `body writes field '${w.field}' through ${call}, and object '${objectName}' declares it ` + + `readonly:true. A hook's ctx.api is a ScopedContext over the TRIGGERING operation's context, so ` + + `on every non-system trigger the engine strips readonly keys from that UPDATE payload - ` + + `the write never lands, while the call still returns success.`, + hint: isCreate + ? `Seeding a readonly column at create time is a SYSTEM act. To keep writing it from here, ` + + `declare runAs: 'system' on this hook: the strip skips a system context, so the write lands, ` + + `and the triggering user is still stamped on the record. Or stamp it on '${objectName}''s OWN ` + + `beforeInsert hook - ctx.input.${w.field} = ... is a server value and survives the strip. ` + + `Note that ctx.api.sudo() is NOT an option from a body: sudo() lives on the in-process ` + + `ScopedContext and is not marshalled into the sandbox, so calling it here is a TypeError at ` + + `run time. Otherwise drop '${w.field}' from this payload, or drop readonly:true from the field.` + : `If automation is meant to maintain '${w.field}', stamp it on the record's OWN hook - ` + + `ctx.input.${w.field} = ... in beforeInsert/beforeUpdate survives the strip, and is the ` + + `recommended shape. To keep writing it CROSS-OBJECT from here, declare runAs: 'system' on ` + + `this hook: the strip skips a system context, so the write lands, and the triggering user ` + + `is still stamped on the record. Note that ctx.api.sudo() is NOT an option from a body: ` + + `sudo() lives on the in-process ScopedContext and is not marshalled into the sandbox, so ` + + `calling it here is a TypeError at run time. Otherwise drop readonly:true from '${w.field}'.`, }); - } else if (meta.readonlyWhen) { + } else if (meta.readonlyWhen && !isCreate) { reported.add(dedupeKey); findings.push({ severity: 'warning', From 1b75e4ea94f87d78a04eccf14bf1db2b1289b15a Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 6 Sep 2026 07:02:14 +0000 Subject: [PATCH 2/3] fix(lint): report a non-system create of a static-readonly field on the flow and hook readonly rules Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01Vbw3RPgdtqesx4azk9SbW8 --- packages/lint/src/validate-readonly-flow-writes.test.ts | 4 +++- packages/lint/src/validate-readonly-flow-writes.ts | 6 ++++-- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/packages/lint/src/validate-readonly-flow-writes.test.ts b/packages/lint/src/validate-readonly-flow-writes.test.ts index c6989c8ac2..ad2263e88a 100644 --- a/packages/lint/src/validate-readonly-flow-writes.test.ts +++ b/packages/lint/src/validate-readonly-flow-writes.test.ts @@ -361,7 +361,9 @@ describe('validateReadonlyFlowWrites', () => { // — the row is created without the column — not the update sentence. expect(findings[0].message).toContain('INSERT'); expect(findings[0].message).toContain('created WITHOUT this column'); - expect(findings[0].message).toContain('#14147'); + // No tracker id in the string an author reads (`check:doc-authoring`); + // the ruling's id lives in the rule's comment. + expect(findings[0].message).not.toMatch(/#\d{4,}/); expect(findings[0].message).not.toContain('UPDATE payload (#2948)'); // The remedy names the create verb, the system channel and the own-object // beforeInsert stamp. diff --git a/packages/lint/src/validate-readonly-flow-writes.ts b/packages/lint/src/validate-readonly-flow-writes.ts index 890b5b04ba..2ca36a788f 100644 --- a/packages/lint/src/validate-readonly-flow-writes.ts +++ b/packages/lint/src/validate-readonly-flow-writes.ts @@ -240,10 +240,12 @@ export function validateReadonlyFlowWrites(stack: AnyRec): ReadonlyFlowWriteFind path: `${nodePath}.config.fields.${fieldName}`, message: isCreate ? // The create-side strip is the 2026-09-03 ruling (#14147): the - // same `stripReadonlyFields`, now run by `engine.insert` too. + // same `stripReadonlyFields`, now run by `engine.insert` too. The + // id stays in this comment, out of the string an author reads + // and cannot resolve (`check:doc-authoring`). `writes field '${fieldName}', which object '${objectName}' declares readonly:true. Under ` + `runAs:'${runAs}' the engine silently strips readonly fields from the INSERT payload too ` + - `(#14147 — the same strip the UPDATE path runs), so the row is created WITHOUT this column ` + + `(the same strip the UPDATE path runs), so the row is created WITHOUT this column ` + `(it falls back to the field's defaultValue) — while the create_record step still reports ` + `success, with only a run-time warning naming the dropped field.` : `writes field '${fieldName}', which object '${objectName}' declares readonly:true. Under ` + From 9b76e9a495a4e9c3d83d604371fb724b98a05f68 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 6 Sep 2026 07:33:58 +0000 Subject: [PATCH 3/3] fix(lint): the create-verb readonly finding skips platform objects, which the engine's insert strip does not judge Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01Vbw3RPgdtqesx4azk9SbW8 --- .changeset/lint-readonly-create-scan-gap.md | 1 + .../src/validate-readonly-flow-writes.test.ts | 25 +++++++++++++++ .../lint/src/validate-readonly-flow-writes.ts | 31 ++++++++++++++++++- .../src/validate-readonly-hook-writes.test.ts | 18 +++++++++++ .../lint/src/validate-readonly-hook-writes.ts | 7 ++++- 5 files changed, 80 insertions(+), 2 deletions(-) diff --git a/.changeset/lint-readonly-create-scan-gap.md b/.changeset/lint-readonly-create-scan-gap.md index 1b763dc6b9..0f7d3cedaa 100644 --- a/.changeset/lint-readonly-create-scan-gap.md +++ b/.changeset/lint-readonly-create-scan-gap.md @@ -17,6 +17,7 @@ The rule ids and severities are the update ones — one id per shape, not per ve - No `readonlyWhen` (conditional) finding on a create, on either surface: a conditional lock is evaluated against the record being written over, which a create does not have, and the engine runs no conditional strip on INSERT ("INSERT stays exempt"). A warning there would state something false about a write that lands. - The hook rule judges `.insert()` only, not `.create()`. The host `ObjectRepository` aliases `create()` to `insert()`, but L2 bodies run in QuickJS and the VM-side `ctx.api.object()` installs no `create` leaf — a body calling `.create()` throws `TypeError: not a function` on its first run, a loud failure rather than the silent drop this rule reports. The silence is recorded as a reasoned method exclusion (`READONLY_HOOK_METHOD_EXCLUSIONS`) and pinned. +- No create finding on a **platform object** — one declaring `managedBy`, or in the reserved `sys_` namespace. The engine's create-side strip does not judge those at all (`staticReadonlyInsertSubject`: their own ADR-0086 write guard governs them), so a finding there would describe a strip that never runs. The update verb keeps judging them, exactly as the engine's update path does. - `validate-readonly-action-writes` is unchanged: an action body runs system-elevated by design, so its create genuinely lands. **Migration.** If your build reds on the new finding, the fix is one of: declare `runAs: 'system'` on the flow or hook when seeding the `readonly` column is the intent (the intended channel — `readonly` governs the end-user/API surface, not trusted system writers); remove the key from the `create_record` `fields` / `insert()` payload when it is not; or stamp it in a `beforeInsert` hook on the target object (`ctx.input. = …`), which is a server value the strip does not touch. Measured over this repository's shipped examples (`app-crm`, `app-showcase`, `app-todo`): zero in-repo flows or hooks go red — the two `create_record` nodes that target an object carrying a `readonly` field write none of its `readonly` fields, and the one flow that creates unauthenticated already declares `runAs: 'system'`; no shipped hook body inserts through `ctx.api`. diff --git a/packages/lint/src/validate-readonly-flow-writes.test.ts b/packages/lint/src/validate-readonly-flow-writes.test.ts index ad2263e88a..a118567e1e 100644 --- a/packages/lint/src/validate-readonly-flow-writes.test.ts +++ b/packages/lint/src/validate-readonly-flow-writes.test.ts @@ -465,6 +465,31 @@ describe('validateReadonlyFlowWrites', () => { expect(findings[0].where).toBe('flow "fan_out" › loop "Each" › body › node "C"'); }); + // The engine's create-side strip does not judge a PLATFORM object at all + // (`staticReadonlyInsertSubject`: `managedBy` set, or a `sys_` name — its + // own 403 write guard governs it), so a create finding there would + // describe a strip that never runs. The UPDATE path applies no such + // exclusion, which the update controls pin. + it.each([ + ['a sys_ object', { name: 'sys_audit_entry', fields: { verdict: { type: 'text', readonly: true } } }], + ['a managedBy object', { name: 'audit_entry', managedBy: 'engine-owned', fields: { verdict: { type: 'text', readonly: true } } }], + ])('does NOT flag a create_record into %s — outside the create-side strip; the same update_record is still flagged', (_label, platformObject) => { + const create = { + name: 'seed_audit', + runAs: 'user', + nodes: [{ id: 'c', type: 'create_record', label: 'C', config: { objectName: platformObject.name, fields: { verdict: 'ok' } } }], + edges: [], + }; + expect(validateReadonlyFlowWrites({ objects: [platformObject], flows: [create] })).toEqual([]); + const update = { + ...create, + nodes: [{ id: 'u', type: 'update_record', label: 'U', config: { objectName: platformObject.name, filter: { id: '{id}' }, fields: { verdict: 'ok' } } }], + }; + const control = validateReadonlyFlowWrites({ objects: [platformObject], flows: [update] }); + expect(control).toHaveLength(1); + expect(control[0].rule).toBe(FLOW_UPDATE_READONLY_FIELD); + }); + it('skips a templated objectName and a non-literal fields map on create, as on update', () => { expect(validateReadonlyFlowWrites({ objects: [opportunityObject], flows: [createFlow({ approval_status: 'x' }, { runAs: 'user' }, { objectName: '{target}' })] })).toEqual([]); expect(validateReadonlyFlowWrites({ objects: [opportunityObject], flows: [createFlow('{payload}' as unknown as Record, { runAs: 'user' })] })).toEqual([]); diff --git a/packages/lint/src/validate-readonly-flow-writes.ts b/packages/lint/src/validate-readonly-flow-writes.ts index 2ca36a788f..213191a130 100644 --- a/packages/lint/src/validate-readonly-flow-writes.ts +++ b/packages/lint/src/validate-readonly-flow-writes.ts @@ -149,6 +149,30 @@ export function buildReadonlyIndex(objects: AnyRec[]): Map { + const exempt = new Set(); + for (const obj of objects) { + const name = typeof obj.name === 'string' ? obj.name : undefined; + if (!name) continue; + if (obj.managedBy || name.startsWith('sys_')) exempt.add(name); + } + return exempt; +} + /** * The target object of an `update_record` / `create_record` node, when * statically knowable. Both node configs anchor the object on the same key @@ -177,7 +201,9 @@ export function validateReadonlyFlowWrites(stack: AnyRec): ReadonlyFlowWriteFind const flows = recordsOf(stack.flows); if (flows.length === 0) return findings; - const roIndex = buildReadonlyIndex(recordsOf(stack.objects)); + const objects = recordsOf(stack.objects); + const roIndex = buildReadonlyIndex(objects); + const insertStripExempt = buildInsertStripExemptObjects(objects); flows.forEach((flow, flowIndex) => { // `runAs` defaults to 'user' (schema default). Only an explicit 'system' @@ -207,6 +233,9 @@ export function validateReadonlyFlowWrites(stack: AnyRec): ReadonlyFlowWriteFind 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 + // A platform object is outside the create-side strip entirely (see + // `buildInsertStripExemptObjects`); the update branch is not. + if (isCreate && insertStripExempt.has(objectName)) return; const fields = config.fields; // A non-literal write map (templated string, spread, array) is not diff --git a/packages/lint/src/validate-readonly-hook-writes.test.ts b/packages/lint/src/validate-readonly-hook-writes.test.ts index e6cd7b9197..d3c6f9241e 100644 --- a/packages/lint/src/validate-readonly-hook-writes.test.ts +++ b/packages/lint/src/validate-readonly-hook-writes.test.ts @@ -341,6 +341,24 @@ describe('validateReadonlyHookWrites - GREEN: what a create is NOT judged on', ( // `.create()` therefore throws `TypeError: not a function` on its first run — // a loud failure, not the silent no-op this rule reports — so a finding would // be false in exactly the way the sudo() hint used to be (#14010). + // The engine's create-side strip does not judge a PLATFORM object at all + // (`staticReadonlyInsertSubject`: `managedBy` set, or a `sys_` name — its + // own 403 write guard governs it); the UPDATE path applies no such + // exclusion. Pinned in both directions per object shape. + it.each([ + ['a sys_ object', { name: 'sys_activity', fields: { verdict: { type: 'text', readonly: true } } }], + ['a managedBy object', { name: 'activity', managedBy: 'append-only', fields: { verdict: { type: 'text', readonly: true } } }], + ])('never flags insert() into %s — outside the create-side strip; the same update() is still flagged', (_label, platformObject) => { + const hook = (source: string) => ({ + objects: [platformObject], + hooks: [{ name: 'log', object: 'crm_case', events: ['afterInsert'], body: { language: 'js', source } }], + }); + expect(validateReadonlyHookWrites(hook(`await ctx.api.object('${platformObject.name}').insert({ verdict: 'ok' });`))).toEqual([]); + const control = validateReadonlyHookWrites(hook(`await ctx.api.object('${platformObject.name}').update({ id: x, verdict: 'ok' });`)); + expect(control).toHaveLength(1); + expect(control[0].rule).toBe(HOOK_API_UPDATE_READONLY_FIELD); + }); + it('never flags create() — the sandbox has no such leaf, so the call throws rather than silently dropping', () => { expect( validateReadonlyHookWrites( diff --git a/packages/lint/src/validate-readonly-hook-writes.ts b/packages/lint/src/validate-readonly-hook-writes.ts index 8210949e15..c3ec36f083 100644 --- a/packages/lint/src/validate-readonly-hook-writes.ts +++ b/packages/lint/src/validate-readonly-hook-writes.ts @@ -158,7 +158,7 @@ import { extractHookBodyWriteSet, type BodyWritePatternExclusion, } from './validate-hook-body-writes.js'; -import { buildReadonlyIndex } from './validate-readonly-flow-writes.js'; +import { buildReadonlyIndex, buildInsertStripExemptObjects } from './validate-readonly-flow-writes.js'; import { recordsOf } from './object-graph.js'; export type ReadonlyHookWriteSeverity = 'error' | 'warning'; @@ -284,6 +284,7 @@ export function validateReadonlyHookWrites(stack: AnyRec): ReadonlyHookWriteFind // Built lazily: a stack whose hooks are all L1/handler-based never pays it. let roIndex: ReturnType | null = null; + let insertStripExempt: Set | null = null; hooks.forEach((hook, hookIndex) => { const body = hook.body; @@ -320,6 +321,7 @@ export function validateReadonlyHookWrites(stack: AnyRec): ReadonlyHookWriteFind if (writes.length === 0) return; roIndex ??= buildReadonlyIndex(recordsOf(stack.objects)); + insertStripExempt ??= buildInsertStripExemptObjects(recordsOf(stack.objects)); const hookName = typeof hook.name === 'string' && hook.name ? hook.name : `#${hookIndex}`; const where = `hook "${hookName}" > body`; @@ -351,6 +353,9 @@ export function validateReadonlyHookWrites(stack: AnyRec): ReadonlyHookWriteFind // The static branch judges every subject method; the conditional one // only those with a prior record to lock on. const isCreate = !CONDITIONAL_SUBJECT_METHODS.has(method); + // A platform object is outside the create-side strip entirely (see + // `buildInsertStripExemptObjects`); an update of it is still judged. + if (isCreate && insertStripExempt.has(objectName)) continue; if (meta.readonly) { reported.add(dedupeKey);