diff --git a/.changeset/readonly-action-api-write-lint.md b/.changeset/readonly-action-api-write-lint.md new file mode 100644 index 0000000000..7e48be859f --- /dev/null +++ b/.changeset/readonly-action-api-write-lint.md @@ -0,0 +1,23 @@ +--- +'@objectstack/lint': minor +--- + +Add `validateReadonlyActionWrites` — an author-time warning on an action body writing a `readonlyWhen` field through `ctx.api`. + +The action surface is the third write surface in the readonly family, after `flow-update-readonly-field` and `hook-api-update-readonly-field`, and it is the one where the family's answer differs. An action body's `ctx.api` is `createContext({ ...callerEnvelope, isSystem: true })` — elevated by design, so RLS/FLS-bypassing trusted execution is the documented posture — and the engine's **static** readonly strip runs only for non-system callers. Measured against a real engine over a memory driver: + +| channel | static `readonly` | `readonlyWhen`, predicate TRUE | +| --- | --- | --- | +| action body `ctx.api` | lands | **stripped** | +| hook body `ctx.api`, non-system trigger | stripped | stripped | +| `ctx.api.sudo()` | lands | **stripped** | + +So exactly one shape is a silent no-op on this surface, and that is what the new rule reports: + +- `action-api-update-readonly-when-field` — **warning**. A literal `ctx.api.object('…').update()` / `.updateById()` in an action body writing a field the named object declares `readonlyWhen`. The conditional strip takes no `isSystem` exemption, so elevation is not a workaround and the hint does not offer one: confirm the call only targets records whose predicate is FALSE, or derive the field in a `beforeUpdate` hook on the target object (a hook-written value is not caller-supplied and does land). + +A static-`readonly` counterpart is deliberately **not** shipped: an elevated action write lands on such a field, so the finding would state a falsehood and, at the hook rule's `error` grade, would gate a build over working code. + +Wired through `REFERENCE_INTEGRITY_RULES`, so it runs on `os validate`, `os lint` and `os compile` at once. It reuses the existing machinery rather than adding any: `buildReadonlyIndex` from the flow rule for the field metadata, and `collectActionBodies` from the action rule for the body walk (both registration sites, with the merged-action de-duplication that walk owns). + +`ctx.record` is excluded from the match set, and that exclusion is the rule's load-bearing decision: an action's `ctx.record` is a dead snapshot the runtime never writes back, so no readonly strip is ever consulted on it and a readonly verdict there would be false on every occurrence. `action-record-write-discarded` already owns that shape and states its real reason. Also skipped, each for a stated reason: `insert` / `create` (INSERT is exempt from both strips), `ctx.input` writes (an action's `ctx.input` is its params bag), dynamic object names, non-literal payloads, objects this stack does not declare, fields the object does not declare, and `id` in an `update` payload (the row address, not a field write). diff --git a/content/docs/automation/hook-bodies.mdx b/content/docs/automation/hook-bodies.mdx index 368b0d3a55..2aceb592e3 100644 --- a/content/docs/automation/hook-bodies.mdx +++ b/content/docs/automation/hook-bodies.mdx @@ -265,6 +265,8 @@ The dropped case is the dangerous one: nothing fails, the step reports success, Only literal object names and literal payload keys are seen; a `sudo()` chain, a dynamic object name, an object this stack does not declare, and `insert`/`create` are all skipped, so the rule has no opinion on them. The flow surface has carried the same gate as `flow-update-readonly-field` since [#3425](https://github.com/objectstack-ai/objectstack/issues/3425). +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. + ### Errors from `ctx.api` A rejected `ctx.api` call gives your body the host error's `name` and `message`, plus two structured properties when the host supplied them: diff --git a/packages/lint/src/index.ts b/packages/lint/src/index.ts index d20fa78693..28d07afb47 100644 --- a/packages/lint/src/index.ts +++ b/packages/lint/src/index.ts @@ -136,6 +136,17 @@ export type { ReadonlyHookWriteSeverity, } from './validate-readonly-hook-writes.js'; +export { + validateReadonlyActionWrites, + ACTION_API_UPDATE_READONLY_WHEN_FIELD, + READONLY_ACTION_WRITE_PATTERN_IDS, + READONLY_ACTION_WRITE_EXCLUSIONS, +} from './validate-readonly-action-writes.js'; +export type { + ReadonlyActionWriteFinding, + ReadonlyActionWriteSeverity, +} from './validate-readonly-action-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/reference-integrity-suite.test.ts b/packages/lint/src/reference-integrity-suite.test.ts index 0d0c75ab88..04da0f0949 100644 --- a/packages/lint/src/reference-integrity-suite.test.ts +++ b/packages/lint/src/reference-integrity-suite.test.ts @@ -42,6 +42,10 @@ describe('reference-integrity suite — membership', () => { // `ctx.api` update to a declared-`readonly` field, placed beside the flow // twin that asks the identical question one surface over. 'validateReadonlyHookWrites', + // [#13770] The third write surface. Same question, and the one place the + // family's answer differs: an action body is elevated, so only the + // CONDITIONAL half of the readonly judgement survives there. + 'validateReadonlyActionWrites', 'validateReactPageProps', ]); }); @@ -68,6 +72,12 @@ describe('reference-integrity suite — every member actually runs', () => { fields: { name: { type: 'text', label: 'Name' }, locked: { type: 'boolean', label: 'Locked', readonly: true }, + // validateReadonlyActionWrites (#13770): a CONDITIONAL lock, which is + // the one readonly shape an ACTION body cannot write — an action runs + // elevated, and `isSystem` exempts the static strip but never the + // conditional one. A separate field from `locked` on purpose: the two + // rules must be able to go silent independently. + frozen_note: { type: 'text', label: 'Frozen note', readonlyWhen: "record.locked == true" }, // validateSortableFields (#9257): a virtual field, so it is a REAL // field name (existence passes) with no stored column behind it. days_open: { type: 'formula', label: 'Days Open' }, @@ -118,6 +128,22 @@ describe('reference-integrity suite — every member actually runs', () => { "ctx.record.name = 'scored'; await ctx.api.object('crm_lead').update({ lead_score: 100 });", }, }, + // validateReadonlyActionWrites (#13770): `frozen_note` EXISTS on crm_lead + // and is `readonlyWhen`, so this is not an existence question — on a + // record whose predicate is TRUE the engine drops the key from the UPDATE + // payload and the action still returns success. A SEPARATE action from + // `score_now` on purpose, mirroring the hook split above: one body + // carrying both defects would let either rule go silent behind the + // other's finding. + { + name: 'freeze_now', + label: 'Freeze Now', + objectName: 'crm_lead', + body: { + language: 'js', + source: "await ctx.api.object('crm_lead').update({ id: ctx.recordId, frozen_note: 'x' });", + }, + }, ], views: [ { @@ -308,6 +334,7 @@ describe('reference-integrity suite — every member actually runs', () => { expect(rules).toContain('flow-node-write-unknown-field'); expect(rules).toContain('flow-update-readonly-field'); expect(rules).toContain('hook-api-update-readonly-field'); + expect(rules).toContain('action-api-update-readonly-when-field'); expect(rules).toContain('react-prop-missing-required'); }); diff --git a/packages/lint/src/reference-integrity-suite.ts b/packages/lint/src/reference-integrity-suite.ts index fe8ec8aed7..e3772a3ce2 100644 --- a/packages/lint/src/reference-integrity-suite.ts +++ b/packages/lint/src/reference-integrity-suite.ts @@ -97,6 +97,7 @@ import { validateActionBodyWrites } from './validate-action-body-writes.js'; import { validateFlowNodeWrites } from './validate-flow-node-writes.js'; import { validateReadonlyFlowWrites } from './validate-readonly-flow-writes.js'; import { validateReadonlyHookWrites } from './validate-readonly-hook-writes.js'; +import { validateReadonlyActionWrites } from './validate-readonly-action-writes.js'; import { validateReactPageProps } from './validate-react-page-props.js'; export type ReferenceIntegritySeverity = 'error' | 'warning'; @@ -292,6 +293,25 @@ export const REFERENCE_INTEGRITY_RULES: readonly ReferenceIntegrityRule[] = [ // caller-supplied values (#5591) — so the rule keys on the write CHANNEL, // and both directions are pinned in its tests. { name: 'validateReadonlyHookWrites', run: validateReadonlyHookWrites }, + // [#13770] The THIRD write surface, and the one place the family's answer + // differs. An action body's `ctx.api` is `createContext({ ...ec, isSystem: + // true })` — elevated by design (#3914) — so the engine's STATIC readonly + // strip, which runs only under `!opCtx.context?.isSystem`, is skipped and a + // `readonly:true` write LANDS here. The conditional strip is not skipped + // (`isSystem` is explicitly not an exemption for it, #9107 LOCK 2), so what + // this member reports is exactly one shape: a `readonlyWhen` field written + // through a literal `ctx.api` update, which silently does not land on records + // whose predicate is TRUE. It advises rather than gates for the reason the + // flow and hook siblings advise on the same shape — the outcome depends on + // the ROW, not on anything this stack declares. + // + // `ctx.record` is excluded from its match set entirely, and that exclusion is + // the rule's load-bearing decision rather than an omission: an action's + // `ctx.record` is a dead snapshot the runtime never writes back, so no strip + // is ever consulted and a readonly verdict there would be false on every + // occurrence. `action-record-write-discarded` already owns that shape and + // states its real reason. + { name: 'validateReadonlyActionWrites', run: validateReadonlyActionWrites }, // The `kind:'react'` page surface. Every prop a react block binds BY FIELD // NAME is resolved against the object it names (#4340) — ``, // ``, `` through the SAME diff --git a/packages/lint/src/validate-action-body-writes.ts b/packages/lint/src/validate-action-body-writes.ts index 9fc4bc685d..bc372c3ae9 100644 --- a/packages/lint/src/validate-action-body-writes.ts +++ b/packages/lint/src/validate-action-body-writes.ts @@ -206,8 +206,14 @@ function asArray(v: unknown): AnyRec[] { return []; } -/** One L2 action body found in the stack, with the location to report it at. */ -interface ActionBodySite { +/** + * One L2 action body found in the stack, with the location to report it at. + * + * Exported alongside {@link collectActionBodies} for + * `validate-readonly-action-writes.ts` (#13770), which walks the identical set + * of bodies to ask a different question about them. + */ +export interface ActionBodySite { name: string; source: string; path: string; @@ -259,8 +265,16 @@ function actionObjectBinding(action: AnyRec, parentObject?: string): string | un * a non-`script` body here would produce advice about writes that provably * never happen — noise pointing at metadata whose real defect is the `type`, * which the publish gate already names with its own prescription. + * + * Exported for `validate-readonly-action-writes.ts` (#13770) — shared rather + * than copied, for the reason `buildReadonlyIndex` is shared with the hook + * rule: two readings of "which bodies are there, and where do I report them?" + * that drift produce two rules disagreeing about the same body, and the + * disagreement is silent. Every subtlety above (both registration sites, the + * by-VALUE de-duplication of a merged action, the `type: 'script'` default, the + * authored-location path) is one this rule's sibling must get identically right. */ -function collectActionBodies(stack: AnyRec): ActionBodySite[] { +export function collectActionBodies(stack: AnyRec): ActionBodySite[] { const sites: ActionBodySite[] = []; const seen = new Set(); diff --git a/packages/lint/src/validate-readonly-action-writes.test.ts b/packages/lint/src/validate-readonly-action-writes.test.ts new file mode 100644 index 0000000000..1910d87957 --- /dev/null +++ b/packages/lint/src/validate-readonly-action-writes.test.ts @@ -0,0 +1,510 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// Both directions of the #13770 judgement, pinned together — plus the ONE case +// that separates this rule from its hook sibling. +// +// The action surface differs from the hook surface in exactly one way that +// matters here, and it is measurable rather than arguable: an action body's +// `ctx.api` is `ql.createContext(buildActionExecutionContext(ec))` and +// `buildActionExecutionContext` is `{ ...ec, isSystem: true }`, so an action +// body runs ELEVATED. Driving a real ObjectQL engine over a memory driver with +// exactly that context: +// +// [action ctx.api] static readonly -> the value LANDS +// [action ctx.api] readonlyWhen -> the value is STRIPPED +// [ctx.api.sudo()] readonlyWhen -> still STRIPPED +// +// which is the engine's own documented asymmetry: the static strip runs under +// `if (!opCtx.context?.isSystem)`, the conditional one does not and takes no +// `isSystem` exemption at all (#9107 LOCK 2). So this rule reports the +// conditional shape and deliberately says NOTHING about the static one — a +// static-`readonly` finding here would tell an author their write never lands +// when it does. `flags nothing on a static readonly field` below is that +// measurement's pin: if the engine ever stops exempting system callers, this is +// the test that should be revisited first. +import { describe, expect, it } from 'vitest'; + +import { HOOK_BODY_WRITE_PATTERNS } from './validate-hook-body-writes.js'; +import { + validateReadonlyActionWrites, + ACTION_API_UPDATE_READONLY_WHEN_FIELD, + READONLY_ACTION_WRITE_PATTERN_IDS, + READONLY_ACTION_WRITE_EXCLUSIONS, +} from './validate-readonly-action-writes.js'; + +/** + * A stack shaped like the shipped showcase invoice (a state lock: once an + * invoice is paid its money columns freeze), reproduced here rather than + * imported — tests do not read `examples/**`, so an example-app sweep can never + * pull a fixture out from under a rule (maintainer ruling, 2026-08-13). + */ +const invoiceStack = (source: string) => ({ + objects: [ + { + name: 'showcase_invoice', + fields: { + // Writable by anyone — the control that proves the rule keys on the + // declaration and not merely on the channel. + status: { type: 'text', label: 'Status' }, + // Conditionally locked: the shape this rule reports. + tax_rate: { type: 'number', label: 'Tax rate', readonlyWhen: "record.status == 'paid'" }, + // Statically locked: the shape this rule deliberately stays silent on, + // because an elevated action write LANDS on it. + invoice_number: { type: 'text', label: 'Invoice number', readonly: true }, + }, + }, + ], + actions: [ + { + name: 'settle_invoice', + label: 'Settle', + objectName: 'showcase_invoice', + body: { language: 'js', source }, + }, + ], +}); + +describe('validateReadonlyActionWrites - RED: a ctx.api write to a readonlyWhen field', () => { + it('flags ctx.api.object(...).update() writing a readonlyWhen field', () => { + const findings = validateReadonlyActionWrites( + invoiceStack("await ctx.api.object('showcase_invoice').update({ id: ctx.recordId, tax_rate: 8 });"), + ); + expect(findings).toHaveLength(1); + expect(findings[0].rule).toBe(ACTION_API_UPDATE_READONLY_WHEN_FIELD); + expect(findings[0].severity).toBe('warning'); + expect(findings[0].where).toBe('action "settle_invoice" > body'); + expect(findings[0].path).toBe('actions[0].body.source'); + expect(findings[0].message).toContain("'tax_rate'"); + expect(findings[0].message).toContain('showcase_invoice'); + }); + + it('flags updateById, whose payload is argument 1', () => { + const findings = validateReadonlyActionWrites( + invoiceStack("await ctx.api.object('showcase_invoice').updateById(ctx.recordId, { tax_rate: 8 });"), + ); + expect(findings).toHaveLength(1); + expect(findings[0].rule).toBe(ACTION_API_UPDATE_READONLY_WHEN_FIELD); + }); + + it('does NOT offer elevation as the remedy — the action body is already elevated', () => { + // The measured difference from the hook sibling's hint. An action body runs + // under `{ ...ec, isSystem: true }` and the readonlyWhen lock still applies, + // so telling this author to reach for `sudo()` would send them at a change + // that provably does nothing. + const [finding] = validateReadonlyActionWrites( + invoiceStack("await ctx.api.object('showcase_invoice').update({ tax_rate: 8 });"), + ); + expect(finding.hint).toContain('already system-elevated'); + expect(finding.hint).toContain('changes nothing'); + // The two remedies that DO work, both named. + expect(finding.hint).toContain('predicate is FALSE'); + expect(finding.hint).toContain('beforeUpdate hook'); + }); + + it('reaches an action declared on an object as well as a top-level one', () => { + // The positive control for the shared walk: `collectActionBodies` registers + // BOTH sites the runtime reads (`bundle.actions` and `objects[].actions`), + // and a rule that only saw the first would be silently half-blind. + const findings = validateReadonlyActionWrites({ + objects: [ + { + name: 'showcase_invoice', + fields: { tax_rate: { type: 'number', readonlyWhen: "record.status == 'paid'" } }, + actions: [ + { + name: 'freeze', + body: { + language: 'js', + source: "await ctx.api.object('showcase_invoice').update({ tax_rate: 0 });", + }, + }, + ], + }, + ], + }); + expect(findings).toHaveLength(1); + expect(findings[0].where).toBe('action "freeze" > body'); + expect(findings[0].path).toBe('objects[0].actions[0].body.source'); + }); + + it('reports a merged action ONCE, not once per registration site', () => { + // `mergeObjectActions` appends the action to its object's array while + // PRESERVING the top-level entry, so the same body is genuinely reachable + // twice. The shared walk collapses it by value and reports the authored + // location; a rule that re-implemented the walk would double-report here. + const body = { + language: 'js', + source: "await ctx.api.object('showcase_invoice').update({ tax_rate: 8 });", + }; + const findings = validateReadonlyActionWrites({ + objects: [ + { + name: 'showcase_invoice', + fields: { tax_rate: { type: 'number', readonlyWhen: "record.status == 'paid'" } }, + actions: [{ name: 'settle', objectName: 'showcase_invoice', body }], + }, + ], + actions: [{ name: 'settle', objectName: 'showcase_invoice', body }], + }); + expect(findings).toHaveLength(1); + expect(findings[0].path).toBe('actions[0].body.source'); + }); + + it('flags each distinct readonlyWhen field once, however many times it is written', () => { + const findings = validateReadonlyActionWrites( + invoiceStack( + "await ctx.api.object('showcase_invoice').update({ tax_rate: 1 }); " + + "await ctx.api.object('showcase_invoice').update({ tax_rate: 2 });", + ), + ); + expect(findings).toHaveLength(1); + }); + + it('reads the array `fields` authoring shape as well as the map shape', () => { + const findings = validateReadonlyActionWrites({ + objects: [ + { + name: 'showcase_invoice', + fields: [ + { name: 'status', type: 'text' }, + { name: 'tax_rate', type: 'number', readonlyWhen: "record.status == 'paid'" }, + ], + }, + ], + actions: [ + { + name: 'settle', + body: { + language: 'js', + source: "await ctx.api.object('showcase_invoice').update({ tax_rate: 8 });", + }, + }, + ], + }); + expect(findings).toHaveLength(1); + }); +}); + +describe('validateReadonlyActionWrites - the STATIC readonly half is deliberately absent', () => { + it('flags nothing on a static readonly field — an elevated action write LANDS', () => { + // THE measurement this rule is shaped by, stated as a test rather than as + // prose. Driving a real ObjectQL engine with the context an action body + // actually gets — `{ userId, tenantId, isSystem: true }`, the output of + // `buildActionExecutionContext` — a write to a `readonly: true` column + // persists, because the engine's static strip runs only under + // `if (!opCtx.context?.isSystem)`. Reporting it would state a falsehood and + // (at the hook rule's `error` grade) would gate a build over working code. + expect( + validateReadonlyActionWrites( + invoiceStack("await ctx.api.object('showcase_invoice').update({ invoice_number: 'INV-1' });"), + ), + ).toEqual([]); + }); + + it('reports a field carrying BOTH flags, since the conditional lock still applies', () => { + // `isSystem` exempts the static strip but not the conditional one, so a + // field with both declarations is still conditionally dropped — the one + // place the two halves do not simply cancel. + const findings = validateReadonlyActionWrites({ + objects: [ + { + name: 'showcase_invoice', + fields: { + locked: { type: 'boolean', readonly: true, readonlyWhen: "record.status == 'paid'" }, + }, + }, + ], + actions: [ + { + name: 'settle', + body: { + language: 'js', + source: "await ctx.api.object('showcase_invoice').update({ locked: true });", + }, + }, + ], + }); + expect(findings).toHaveLength(1); + expect(findings[0].rule).toBe(ACTION_API_UPDATE_READONLY_WHEN_FIELD); + expect(findings[0].severity).toBe('warning'); + }); +}); + +describe('validateReadonlyActionWrites - GREEN: ctx.record is not a write channel', () => { + // The card's named must-answer, raised by triage to a hard requirement: + // an action's `ctx.record` is a dead snapshot the runtime never writes back, + // so no readonly strip is ever consulted on it and a readonly verdict there + // would be a false positive on every occurrence. `action-record-write-discarded` + // owns that shape and states the true reason. + it('never flags a ctx.record assignment to a readonlyWhen field', () => { + expect( + validateReadonlyActionWrites(invoiceStack("ctx.record.tax_rate = 8;")), + ).toEqual([]); + }); + + it('never flags a ctx.record assignment to a static readonly field', () => { + expect( + validateReadonlyActionWrites(invoiceStack("ctx.record['invoice_number'] = 'INV-1';")), + ).toEqual([]); + }); + + it('stays silent even when ctx.record ESCAPES into a live api write', () => { + // The escaping shape is the one where a record mutation really does reach + // the engine — and it is still not this rule's finding, because the payload + // is an identifier rather than a literal, so no field name is statically + // knowable. A missed finding, never a false one: the alternative is + // guessing which of the snapshot's keys the update carried. + expect( + validateReadonlyActionWrites( + invoiceStack( + "ctx.record.tax_rate = 8; await ctx.api.object('showcase_invoice').update(ctx.record);", + ), + ), + ).toEqual([]); + }); +}); + +describe('validateReadonlyActionWrites - GREEN: ctx.input is the params bag', () => { + it('never flags ctx.input writes, whatever the name collides with', () => { + expect( + validateReadonlyActionWrites(invoiceStack("ctx.input.tax_rate = 8; ctx.input['invoice_number'] = 'x';")), + ).toEqual([]); + }); + + it('never flags Object.assign(ctx.input, ...)', () => { + expect( + validateReadonlyActionWrites(invoiceStack("Object.assign(ctx.input, { tax_rate: 8 });")), + ).toEqual([]); + }); +}); + +describe('validateReadonlyActionWrites - GREEN: INSERT is exempt from both strips', () => { + // Measured on the same harness: an elevated insert seeding a `readonly` AND a + // `readonlyWhen`-locked column keeps both values. A `readonlyWhen` predicate + // has no prior record to evaluate on a create, which is also why the flow + // sibling never reads a `create_record` node. + it('never flags insert()', () => { + expect( + validateReadonlyActionWrites( + invoiceStack("await ctx.api.object('showcase_invoice').insert({ tax_rate: 8 });"), + ), + ).toEqual([]); + }); + + it('never flags create()', () => { + expect( + validateReadonlyActionWrites( + invoiceStack("await ctx.api.object('showcase_invoice').create({ tax_rate: 8 });"), + ), + ).toEqual([]); + }); +}); + +describe('validateReadonlyActionWrites - GREEN: nothing statically unknowable is guessed', () => { + it('skips a sudo chain (structurally invisible to the extractor)', () => { + // Not a remedy on this surface — the body is elevated already — but the + // shape must stay silent for the same structural reason it does on the hook + // side: `api-crud-literal` requires a literal `ctx.api` receiver. + expect( + validateReadonlyActionWrites( + invoiceStack("await ctx.api.sudo().object('showcase_invoice').update({ tax_rate: 8 });"), + ), + ).toEqual([]); + }); + + it('skips a dynamic object name', () => { + expect( + validateReadonlyActionWrites(invoiceStack("await ctx.api.object(target).update({ tax_rate: 8 });")), + ).toEqual([]); + }); + + it('skips an object this stack does not declare', () => { + expect( + validateReadonlyActionWrites( + invoiceStack("await ctx.api.object('other_pkg_object').update({ tax_rate: 8 });"), + ), + ).toEqual([]); + }); + + it('skips an object that declares no fields at all (external / introspected)', () => { + // An empty field map answers has(anything) === false, which would read as + // "no such field" for every key — the #4383 false-positive generator. + expect( + validateReadonlyActionWrites({ + objects: [{ name: 'ext_invoice', fields: {} }], + actions: [ + { + name: 'settle', + body: { + language: 'js', + source: "await ctx.api.object('ext_invoice').update({ tax_rate: 8 });", + }, + }, + ], + }), + ).toEqual([]); + }); + + it('leaves a field the object does not declare to the unknown-field rule', () => { + expect( + validateReadonlyActionWrites( + invoiceStack("await ctx.api.object('showcase_invoice').update({ no_such_column: 1 });"), + ), + ).toEqual([]); + }); + + it('never flags a writable field', () => { + expect( + validateReadonlyActionWrites( + invoiceStack("await ctx.api.object('showcase_invoice').update({ status: 'paid' });"), + ), + ).toEqual([]); + }); + + it('treats `id` in an update payload as the row ADDRESS, not a field write (#8141)', () => { + expect( + validateReadonlyActionWrites({ + objects: [ + { + name: 'showcase_invoice', + fields: { + id: { type: 'text', readonlyWhen: "record.status == 'paid'" }, + status: { type: 'text' }, + }, + }, + ], + actions: [ + { + name: 'settle', + body: { + language: 'js', + source: "await ctx.api.object('showcase_invoice').update({ id: invoiceId });", + }, + }, + ], + }), + ).toEqual([]); + }); + + it('still judges `id` when it is the PAYLOAD of updateById, not the address', () => { + // `updateById(id, data)` addresses the row in argument 0, so an `id` key in + // argument 1 is an ordinary field write and the address exclusion must not + // swallow it. + const findings = validateReadonlyActionWrites({ + objects: [ + { + name: 'showcase_invoice', + fields: { id: { type: 'text', readonlyWhen: "record.status == 'paid'" } }, + }, + ], + actions: [ + { + name: 'settle', + body: { + language: 'js', + source: "await ctx.api.object('showcase_invoice').updateById(x, { id: 'forged' });", + }, + }, + ], + }); + expect(findings).toHaveLength(1); + }); + + it('stays silent on an unparseable body, leaving it to action-body-source-unparseable', () => { + expect( + validateReadonlyActionWrites( + invoiceStack("await ctx.api.object('showcase_invoice').update({ tax_rate: 8 }); if ("), + ), + ).toEqual([]); + }); + + it('ignores a non-script action, a non-js body and a body-less action', () => { + expect( + validateReadonlyActionWrites({ + objects: [ + { + name: 'showcase_invoice', + fields: { tax_rate: { type: 'number', readonlyWhen: "record.status == 'paid'" } }, + }, + ], + actions: [ + { name: 'declarative', type: 'flow', body: { language: 'js', source: "ctx.api.object('showcase_invoice').update({ tax_rate: 8 })" } }, + { name: 'other_lang', body: { language: 'cel', source: "ctx.api.object('showcase_invoice').update({ tax_rate: 8 })" } }, + { name: 'handler_backed' }, + ], + }), + ).toEqual([]); + }); + + it('returns nothing for a stack with no actions', () => { + expect(validateReadonlyActionWrites({ objects: [] })).toEqual([]); + expect(validateReadonlyActionWrites({})).toEqual([]); + }); +}); + +describe('READONLY_ACTION_WRITE_PATTERN_IDS - ledger partition', () => { + // The declared answer to "which shared body write shapes does this rule + // judge?". Their union must BE the shared ledger, so a fifth pattern landing + // on the hook side fails here until someone classifies it — rather than being + // silently assumed into (or out of) this rule. + it('partitions the shared body-write ledger exactly - no phantom, no unclassified id', () => { + const shared = HOOK_BODY_WRITE_PATTERNS.map((p) => p.id).sort(); + const classified = [ + ...READONLY_ACTION_WRITE_PATTERN_IDS, + ...READONLY_ACTION_WRITE_EXCLUSIONS.map((e) => e.id), + ].sort(); + expect(classified).toEqual(shared); + }); + + it('assigns each ledger shape to exactly one side', () => { + const excluded = READONLY_ACTION_WRITE_EXCLUSIONS.map((e) => e.id); + expect(READONLY_ACTION_WRITE_PATTERN_IDS.filter((id) => excluded.includes(id))).toEqual([]); + }); + + it('gives every exclusion a non-empty reason', () => { + for (const exclusion of READONLY_ACTION_WRITE_EXCLUSIONS) { + expect(exclusion.reason.length, `exclusion '${exclusion.id}' carries no reason`).toBeGreaterThan(0); + } + }); + + it('keeps every consumed pattern reachable through the cheap prefilter', () => { + // The rule skips any body with no `api` identifier before it parses. A + // consumed pattern whose canonical example does not survive that filter + // would be silently unchecked, so the filter is pinned against the ledger + // rather than trusted. + for (const pattern of HOOK_BODY_WRITE_PATTERNS) { + if (!READONLY_ACTION_WRITE_PATTERN_IDS.includes(pattern.id)) continue; + expect(/\bapi\b/.test(pattern.example.source), `pattern '${pattern.id}' fails the prefilter`).toBe(true); + } + }); + + it('consumes only the ctx.api shape - every excluded shape stays green on a readonlyWhen field', () => { + // Drives the exclusion ledger rather than restating it: each excluded + // pattern's own canonical example is run against a stack where every field + // it writes is declared readonlyWhen. A rule that started consuming one of + // them would light up here. + for (const pattern of HOOK_BODY_WRITE_PATTERNS) { + if (READONLY_ACTION_WRITE_PATTERN_IDS.includes(pattern.id)) continue; + const fields = Object.fromEntries( + pattern.example.writes.map((w) => [w.field, { type: 'text', readonlyWhen: 'true' }]), + ); + const objectNames = [ + ...new Set(pattern.example.writes.map((w) => w.object).filter((o): o is string => typeof o === 'string')), + ]; + const findings = validateReadonlyActionWrites({ + objects: [ + { name: 'showcase_invoice', fields }, + ...objectNames.map((name) => ({ name, fields })), + ], + actions: [ + { + name: 'probe', + objectName: 'showcase_invoice', + body: { language: 'js', source: pattern.example.source }, + }, + ], + }); + expect(findings, `excluded pattern '${pattern.id}' produced a finding`).toEqual([]); + } + }); +}); diff --git a/packages/lint/src/validate-readonly-action-writes.ts b/packages/lint/src/validate-readonly-action-writes.ts new file mode 100644 index 0000000000..7543585433 --- /dev/null +++ b/packages/lint/src/validate-readonly-action-writes.ts @@ -0,0 +1,281 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// Build-time guardrail: an L2 ACTION body that writes a field the target object +// declares `readonlyWhen` THROUGH `ctx.api` is a SILENT NO-OP on every record +// whose predicate is TRUE (#13770). +// +// The action-surface sibling of `validate-readonly-hook-writes.ts` (#13653) and +// `validate-readonly-flow-writes.ts` - the same question ("is this declared +// field writable through THIS channel?") on the third write surface. It reuses +// both halves of the existing machinery rather than growing its own: the flow +// rule's `buildReadonlyIndex` for the field metadata, and the action rule's +// `collectActionBodies` for the body walk (top-level `actions` plus +// `objects[].actions`, with the merged-action de-duplication that walk owns). +// +// --- WHY THIS RULE CARRIES ONLY THE CONDITIONAL HALF ----------------------- +// +// The hook rule reports TWO shapes: `error` for a static `readonly` field and +// `warning` for a `readonlyWhen` one. On the action surface only the second is +// true, and the difference is not a judgement call - it is the run identity the +// two surfaces execute under, measured on this tree against a real ObjectQL +// engine over a memory driver: +// +// channel static `readonly` `readonlyWhen` +// ---------------------------------------- ----------------- -------------- +// action body `ctx.api` LANDS STRIPPED +// hook body `ctx.api`, non-system trigger STRIPPED STRIPPED +// `ctx.api.sudo()` LANDS STRIPPED +// +// An action body's `ctx.api` is `ql.createContext(buildActionExecutionContext(ec))` +// and `buildActionExecutionContext` is `{ ...ec, isSystem: true }` - the action +// body is elevated BY DESIGN (#2849/#3914; "TRUSTED - system-elevated, +// RLS/FLS-bypassing by design" is the comment on the REST assembly site), and +// both production dispatch paths build it that way: REST `/actions` in +// `domains/actions.ts` and MCP `run_action` in `action-execution.ts`. The +// engine's static strip runs under `if (!opCtx.context?.isSystem)`, so it is +// SKIPPED for an action body. The conditional strip is not: it runs before that +// guard, over the caller-supplied keys, and `isSystem` is explicitly NOT an +// exemption there (#9107's LOCK 2, pinned in +// `engine-readonly-when-derived-writes.test.ts`). +// +// So a static-`readonly` finding on this surface would state something FALSE - +// "the write never lands" - about a write that does land, and would gate a build +// over working code. That is the failure #8141 removed from the engine's own +// log, and it is not worth re-manufacturing here. +// +// The residual, recorded rather than guessed at: the third `executeAction` +// caller, ObjectQL's `ObjectRepository.execute()`, supplies neither `api` nor +// `executionContext`, so the sandbox falls back to a context-less repo facade +// and the static strip DOES run on that path. A gate whose truth depends on +// which of three dispatchers invoked the action is not a statically decidable +// fact, and the honest fix for that path is to give it the identity the other +// two already have - not to lint every action body as though it had none. +// #13770 carries that escalation. +// +// --- SCOPE - deliberately narrow, so a finding is worth reporting ---------- +// +// - Only `update` / `updateById`. INSERT is exempt from BOTH strips: a +// `readonlyWhen` predicate has no prior record to evaluate on a create, and +// the static one is skipped for the author-declared reason the flow sibling +// skips `create_record` (#3043/#3413). Measured on the same harness - an +// elevated `insert` seeding a `readonly` AND a `readonlyWhen`-locked column +// keeps both values. +// +// - Only the `api-crud-literal` shape. `ctx.record` is not a write surface at +// all here, and that is the whole false-positive class this rule had to +// answer before choosing its match set - see +// {@link READONLY_ACTION_WRITE_EXCLUSIONS}. +// +// - Only a LITERAL object name and a LITERAL payload key; only a field the +// named object DECLARES; and `id` in an `update` payload is the write's +// ADDRESS, not a field write (#8141). The same three bails as the hook +// sibling, for the same reasons. +// +// --- SEVERITY - `warning`, matching the ruling and the two shipped siblings -- +// +// `flow-update-readonly-when-field` and `hook-api-update-readonly-when-field` +// both grade the conditional shape `warning`, and triage confirmed the action +// surface aligns with the hook side rather than re-arguing it. The grade also +// matches the epistemics: `readonlyWhen` strips per RECORD STATE, so the write +// may or may not land depending on the row - a conditional fact, which is what +// an advisory severity is for. +// +// Wired via REFERENCE_INTEGRITY_RULES so it runs on `os validate`, `os lint` and +// `os compile` at once - never hand-wired into individual commands, which is the +// divergence that let `os lint` PASS a flow `os validate` refused. + +import { collectActionBodies, type ActionBodySite } from './validate-action-body-writes.js'; +import { + extractHookBodyWriteSet, + type BodyWritePatternExclusion, +} from './validate-hook-body-writes.js'; +import { buildReadonlyIndex } from './validate-readonly-flow-writes.js'; + +export type ReadonlyActionWriteSeverity = 'warning'; + +export interface ReadonlyActionWriteFinding { + /** Advisory by contract - the conditional strip is per record state. */ + severity: ReadonlyActionWriteSeverity; + rule: string; + /** Human-readable location, e.g. `action "freeze_invoice" > body`. */ + where: string; + /** Config path, e.g. `actions[0].body.source`. */ + path: string; + message: string; + hint: string; +} + +/** Rule id (registry entry). */ +export const ACTION_API_UPDATE_READONLY_WHEN_FIELD = 'action-api-update-readonly-when-field'; + +/** + * The `HOOK_BODY_WRITE_PATTERNS` shapes THIS rule consumes. + * + * Declared as data rather than implied by a branch, for the reason the sibling + * rules declare theirs: a write with no `object` is not a single thing (both + * `ctx.input` and `ctx.record` shapes carry none), so a future ledger addition + * must not be able to land silently in a branch never written for it. + */ +export const READONLY_ACTION_WRITE_PATTERN_IDS: readonly string[] = ['api-crud-literal']; + +/** Ledger shapes this rule leaves alone, each with its reason. */ +export const READONLY_ACTION_WRITE_EXCLUSIONS: readonly BodyWritePatternExclusion[] = [ + { + // The card named this the question to answer BEFORE choosing the match set, + // and triage raised it to a hard requirement: covering `ctx.record` would + // manufacture a whole class of false positives. + id: 'record-property-assign', + reason: + "an action's ctx.record is a DEAD SNAPSHOT - `buildActionSandboxContext` binds " + + '`record: unwrapProxyToPlain(actionCtx?.record)` and `boundActionHandler` returns `result.value` ' + + 'with no write-back (`applyMutationsToInput` is the hook path’s alone), so the assignment never ' + + 'reaches the engine and no readonly strip is ever consulted. A readonly verdict on a write that ' + + 'reaches no write path at all would be a false positive on every occurrence. The shape is not left ' + + 'unreported: `action-record-write-discarded` owns it, and states the true reason (the write is ' + + 'discarded for DECLARED and undeclared fields alike)', + }, + { + id: 'input-property-assign', + reason: + "an action's ctx.input is its PARAMS BAG (`input: unwrapProxyToPlain(actionCtx?.params)`), not a " + + 'record payload - the names it writes are declared parameters, which object field metadata cannot judge', + }, + { + id: 'input-object-assign', + reason: 'same surface as input-property-assign - Object.assign(ctx.input, ...) targets the params bag', + }, +]; + +const APPLICABLE_PATTERN_IDS: ReadonlySet = new Set(READONLY_ACTION_WRITE_PATTERN_IDS); + +/** + * `ctx.api` write methods whose payload is subject to the conditional strip. + * + * `insert` / `create` are absent BY DECISION, not by omission: INSERT is exempt + * from both strips, which is the same reason the flow sibling never looks at a + * `create_record` node. + */ +const STRIP_SUBJECT_METHODS: ReadonlySet = new Set(['update', 'updateById']); + +/** + * Methods whose payload carries the row ADDRESS rather than only field data. + * `ObjectRepository.update(data)` takes no separate id - it travels inside the + * payload - while `updateById(id, data)` addresses the row in argument 0. + */ +const PAYLOAD_ADDRESSED_METHODS: ReadonlySet = new Set(['update']); + +/** The address key excluded on {@link PAYLOAD_ADDRESSED_METHODS} (#8141). */ +const ADDRESS_KEY = 'id'; + +type AnyRec = Record; + +const isRec = (v: unknown): v is AnyRec => !!v && typeof v === 'object' && !Array.isArray(v); + +/** Coerce an array-or-name-keyed-map collection to an array (name injected). */ +function asArray(v: unknown): AnyRec[] { + if (Array.isArray(v)) return v.filter((x): x is AnyRec => isRec(x)); + if (isRec(v)) { + return Object.entries(v).map(([name, def]) => ({ name, ...(isRec(def) ? def : {}) })); + } + return []; +} + +/** + * Validate L2 action-body `ctx.api` writes against target-object `readonlyWhen` + * declarations. Pure `(stack) => Finding[]` (ADR-0019); safe on pre- or + * post-parse stacks. + */ +export function validateReadonlyActionWrites(stack: AnyRec): ReadonlyActionWriteFinding[] { + const findings: ReadonlyActionWriteFinding[] = []; + if (!isRec(stack)) return findings; + + const sites: ActionBodySite[] = collectActionBodies(stack); + if (sites.length === 0) return findings; + + // Built lazily: a stack whose action bodies never reach `ctx.api` never pays it. + let roIndex: ReturnType | null = null; + + for (const site of sites) { + // Cheap prefilter, narrower than the extractor's own: the single consumed + // pattern is rooted at `ctx.api`, so a body carrying no `api` identifier + // cannot match and must not pay the ~9 MB TypeScript load. Pinned by the + // ledger test - a consumed pattern whose example fails this filter fails + // there, rather than going quietly unchecked here. + if (!/\bapi\b/.test(site.source)) continue; + + const extracted = extractHookBodyWriteSet(site.source); + // A body that did not parse yields whatever error recovery left readable. + // The author is not left in silence - `validate-action-body-writes.ts` + // reports the unparseable body itself (`action-body-source-unparseable`), + // which is the finding that actually describes the problem. Skip rather + // than guess at what the unread part wrote. + if (extracted.parseFailure) continue; + + const writes = extracted.writes.filter( + (w) => + APPLICABLE_PATTERN_IDS.has(w.patternId) && + typeof w.object === 'string' && + w.method !== undefined && + STRIP_SUBJECT_METHODS.has(w.method), + ); + if (writes.length === 0) continue; + + roIndex ??= buildReadonlyIndex(asArray(stack.objects)); + + const where = `action "${site.name}" > body`; + const reported = new Set(); + + for (const w of writes) { + const objectName = w.object as string; + const method = w.method as string; + + // The write's address, not a field write (#8141). + if (w.field === ADDRESS_KEY && PAYLOAD_ADDRESSED_METHODS.has(method)) continue; + + // An object this stack does not declare - or one that declares no fields + // at all - cannot be judged; an empty field map answers "no such field" + // for EVERY key, which is a false-positive generator (#4383). + const fieldMap = roIndex.get(objectName); + if (!fieldMap || fieldMap.size === 0) continue; + + // A field the object does not declare is + // `action-body-write-unknown-field`'s question, never this one - the two + // must not double-report one key. + const meta = fieldMap.get(w.field); + if (!meta) continue; + + // A static-`readonly` field is deliberately NOT reported: an action body + // is elevated, so the static strip does not run and the write LANDS. See + // this file's header for the measurement and the escalation. + if (!meta.readonlyWhen) continue; + + const dedupeKey = `${objectName} ${w.field}`; + if (reported.has(dedupeKey)) continue; + reported.add(dedupeKey); + + const call = `ctx.api.object('${objectName}').${method}(...)`; + findings.push({ + severity: 'warning', + rule: ACTION_API_UPDATE_READONLY_WHEN_FIELD, + where, + path: site.path, + // The conditional strip is #3042; that `isSystem` is not an exemption + // for it is #9107's LOCK 2. Both ids stay in this comment. + message: + `body writes field '${w.field}' through ${call}, and object '${objectName}' declares it ` + + `readonlyWhen. An action body runs elevated, which exempts it from the STATIC readonly strip but ` + + `NOT from the conditional one - on records whose predicate is TRUE that UPDATE still drops the ` + + `field, so this write may silently not land depending on the record's state.`, + hint: + `Elevation is not a workaround here: an action body is already system-elevated and the ` + + `readonlyWhen lock still applies, so ctx.api.sudo() changes nothing. Either confirm this call ` + + `only targets records whose readonlyWhen predicate is FALSE, or derive '${w.field}' in a ` + + `beforeUpdate hook on '${objectName}' (a hook-written value is not caller-supplied and does land, ` + + `even on a locked record). Otherwise drop '${w.field}' from this payload. This warning never ` + + `blocks a build.`, + }); + } + } + + return findings; +}